Repository navigation
docs(specs): Spec 113 upstream resilience - OAuth refresh, discovery, call-error taxonomy, health - #1495
Merged
Conversation
… call-error taxonomy, health Spec for closing the verified gaps from the audit against the talk "How Anthropic uses MCP for its own product": serialized OAuth refresh and RFC 6749 error classification (113-a), discovery overrides, offline_access and a discovery cache (113-b), call-error taxonomy on activity records (113-c), call-failure-rate health (113-d), session-terminated re-init (113-e) and explicit cache hints (113-f). CIMD (G4) and server-edition per-user credentials (G10) are deferred with design sketches.
Deploying mcpproxy-docs with
|
| Latest commit: |
1dc5584
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d1b7bc1f.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://113-upstream-resilience.mcpproxy-docs.pages.dev |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 37499209487 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This was referenced Oct 5, 2026
Merged
Merged
Merged
#1498 drops the 'unless a login flow is active' exemption for the DCR clear: tryOAuthAuth holds a connection-wide flow during its own reactive refresh, so the exemption kept a rejected registration forever. The compare-and-clear on client id and refresh token protects anything a login saved.
Dumbris
added a commit
that referenced
this pull request
Oct 6, 2026
… retry once (Spec 113-e) (#1501) ## Problem When a remote Streamable HTTP upstream forgets a session (restart, expiry) it answers HTTP 404 to the session id mcpproxy holds. mcp-go v1.0.0 maps every such 404 to `transport.ErrSessionTerminated` and clears the transport's session id. Today that error text ("terminated") matches `isConnectionError`, so the server flips to Error and goes through a full reconnect, and the failing call is lost. Gap G8 of the Anthropic MCP-proxy talk audit (Spec 113, US5). ## What changed - `core/session_reinit.go`: `SessionSnapshot()` (transport session id + whether the negotiated protocol is stateless) and `ReinitializeSession(ctx)`, a fresh initialize on the same mcp-go client/transport. `connectionEpoch` is never touched. - `managed/session_reinit.go`: single-flight re-init keyed by the stale session id. Concurrent callers share one flight; a caller that finds a newer session proceeds; a request about to go out with an empty session id while one is known joins the flight instead of sending without a session. The flight does initialize plus a synchronous tools/list, refreshes the identity baseline, and schedules the normal discovery callback if the toolset changed. - Wired into: the `tools/call` path, the ListTools leader (so the cached-tool-count path is covered), prompts list/get, and the health ping. List/get/ping are retried once. `tools/call` is retried once only for read-only tools (`readOnlyHint` and not destructive) whose identity hash is unchanged (FR-081); otherwise the call returns `ErrSessionReestablished` (session re-established, call not repeated) and the server is not marked Error. A pinned call whose tool changed gets `ErrConnectionGenerationChanged`. - Only the final error reaches `isConnectionError` (retry also 404, or re-init fails: existing path). - Info log per re-init (session ids as 8-char prefixes) and a per-client counter exposed as `session_reinit_count` in `GetConnectionStatus`. - `docs/setup.md`: troubleshooting note. ## mcp-go v1.0.0 verification (FR-088) Pinned by `TestMCPGo_SessionTerminated_ReinitializeOnSameTransport`: a 404 on a non-initialize POST gives `ErrSessionTerminated` and clears the session id; a request with no session id that is 404'd gives it too; a second `Initialize` on the same client/transport stores a new session id and the client works again. No divergence from the plan. ## Tests run - New: `core` (mcp-go behaviour pin, snapshot/reinit) and `managed` (`TestSessionReinit_*`: single read call = 1 re-init and state stays Ready; 10 concurrent calls = exactly 1 re-init; write tool not repeated and next call works; changed hash not retried and pinned call refused; ListTools retried; ping-first then call sends no session-less request; retry also 404 fails; pinned call epoch unchanged). `-race -count=15` stable. - `go test -race ./internal/upstream/...` green; `go build ./cmd/mcpproxy` and `go build -tags server -o /dev/null ./cmd/mcpproxy` green. - golangci-lint (CI config, bare and `--build-tags server`) on `./internal/upstream/...`: only 2 pre-existing govet `reflect.Ptr` findings in files this PR does not touch (`core/connection_stdio.go:375`, `core/client_secret_test.go:452`). - `./scripts/test-api-e2e.sh` on an isolated port, run alone: 65/66; the one failure ("Audit log: server-edition binary present") is environmental and fails identically on origin/main. Earlier runs overlapped with sibling agents' e2e runs (shared launcher port 39933) and showed launcher failures that vanished when run alone. ## Assumptions - Retrying `tools/call` is limited to read-only tools per FR-081 (a non-conforming server or intermediary could 404 after executing). Spec 018 operation type is not visible to `managed.Client`, so only the `readOnlyHint` annotation is used; tools without that hint get the "re-established, not repeated" error. A follow-up could pass the `call_tool_read` variant down. - FR-083a listener restore: mcpproxy does not enable mcp-go's continuous GET listener on the upstream hop in production (`WithContinuousListening` is only used in an e2e test), so there is no listener to restore; not implemented. - Prometheus counter: no observability seam was touched; counter is in the connection status/diagnostics map only. - `error_class=session_terminated` labelling belongs to 113-c; this PR exposes the sentinel `managed.ErrSessionReestablished` for it to map. - Not covered by a dedicated test: modern-protocol and non-404 no-retry branches (guarded by `SessionSnapshot` modern flag and `errors.Is` check). Spec: #1495 Review: opencode github-copilot/gpt-6.1-sol, 2 rounds, clean; follow-ups: (1) Spec 018 call_tool_read variant not passed down so unannotated read tools are not retried; (2) residual narrow window where a request that snapshotted the old session id is sent on the new id before the flight's tools/list verifies (closing it would serialize sends behind long tool calls); (3) tool-change epoch bump is not a mechanism in this codebase (FR-083 relies on the differential update path); (4) test hardening: RetryAlso404 reaching the retry, listener fixture; (5) a caller ctx deadline while waiting on a flight is classed as a connection error like any other deadline.
Dumbris
added a commit
that referenced
this pull request
Oct 6, 2026
…1504) ## Problem A connected upstream whose tool calls mostly fail still reported `healthy`: health looked only at connection and OAuth state. Gap G7 of the Anthropic MCP-proxy talk audit (Spec 113, FR-060..FR-068). ## What changed - New `internal/upstream/callstats`: a 30 x 10 s bucketed rolling window (5 min) per server, a registry, and a denylist predicate `Classify`. Not counted at all: `isError` tool results, limiter rejections, `ErrConnectionGenerationChanged`, caller cancellation (including the plain-text "context cancelled" form), auth-required errors. Everything else returned by the dispatch is a counted failure; a normal result is a counted success. Proxy policy/quarantine refusals and argument validation happen before dispatch and never reach the recorder. - `upstream.Manager` records at `callTool` (covers direct surface, `call_tool_*`, REST) and `code_execution` records via `RecordClientCallOutcome` (it bypasses the manager). The window and notifier state are dropped in `RemoveServer` inside the same critical section as the client delete. Nothing in `managed/client.go` or `ActivityRecord`. - `health.CalculateHealth` is now a wrapper: only a ready, enabled, healthy result is downgraded to `degraded` when `calls >= 5` and `failures/calls > 0.5`. Summary: "N of M tool calls failed in the last 5 min"; detail names the dominant failure kind; action `view_logs`. Disabled, quarantined, connecting, unhealthy, OAuth and refresh outcomes all take precedence (table tests). Thresholds are constants (`health.CallFailureWindow`, `CallFailureMinSamples`, `CallFailureRatio`); no config field. - The three `HealthCalculatorInput` construction sites (`runtime.go`, `server.go`, `mcp.go`) read `Manager.CallStats`; a drift-guard test fails if a site stops assigning `CallsInWindow`. - T199 (SSE): the existing servers.changed producers are all connection-state driven, so there is no poll-and-diff to ride on. Added a push path: `Manager.SetCallHealthObserver`, wired in `runtime/lifecycle.go` to `emitServersChanged("call_failure_rate")` (already coalesced). Debounced to one per 10 s per server, with a per-bucket recheck timer while the window holds calls so passive recovery or expiry-driven degradation also notifies. - Docs: `docs/designs/2025-12-10-unified-health-status.md`. ## Tests run - TDD: callstats, calculator and manager tests written first and observed failing (compile errors / red assertions) before the implementation. - `go test -race` on `internal/health`, `internal/upstream/...`, `internal/runtime/...`: pass. - `go test -race -tags server -skip "E2E|Binary|MCPProtocol|TestInfoEndpoint|TestGracefulShutdownNoPanic|TestSocketInfoEndpoint" ./internal/server/... ./internal/httpapi/...`: pass. - SC-005 coverage: `TestGetAllServers_HealthDegradesOnCallFailureRate` (runtime projection, 6 of 10 -> degraded), window expiry in `callstats` and notifier tests with an injected clock. - Personal and `-tags server` builds; golangci-lint (both runs): no findings in touched files (the local linter reports 15/18 pre-existing issues in untouched files). - `scripts/test-api-e2e.sh` on an isolated port (18468): 65/66 pass. The one failure, "Audit log: server-edition binary present", needs a prebuilt `mcpproxy-server` binary (environmental, unrelated to this change). - No REST/OAS/config changes, so no `make swagger`. ## Assumptions - Thresholds live in `internal/health` (not `callstats`) so the calculator does not import the managed-client dependency tree; `callstats` takes the window length as its own constant of the same value. - `HealthCalculatorInput` gets a third field, `DominantCallFailureKind`, so the detail text can name the kind without health importing callstats. - The failure kind is derived from the error text (mcp-go discards HTTP status and JSON-RPC codes); 113-c's classifier is not merged, so the local predicate is used as the spec allows. Untyped errors count as failures of kind "other". - A caller deadline that expires after dispatch counts as a timeout failure (only cancellation is excluded). Spec: #1495 Review: opencode github-copilot/gpt-6.1-sol, 5 rounds, clean; earlier rounds fixed text-form cancellation, expiry-driven notification, in-flight resurrection of a removed window and a late-drop wipe; the latest full re-review (3 chunks) found 3 mediums (text-cancel exclusion without a done caller ctx, call window not dropped on config-change client replacement, stale notifier timer callbacks), all fixed, incremental round CLEAN; follow-ups: none.
Dumbris
added a commit
that referenced
this pull request
Oct 6, 2026
…s (Spec 113-f) (#1510) ## Problem Spec 058 FR-019 requires `ttlMs` and `cacheScope` on list/read results. mcp-go v1.0.0 emits them (SEP-2549) with a library default of ttl 0 / private; mcpproxy never configured them explicitly, so scope and TTL were an unpinned library default. Talk gap G9. ## What changed - `cacheHintServerOptions()` in `internal/server/mcp.go`: `WithCacheHints` plus per-method `WithMethodCacheHints` (tools/list, prompts/list, resources/list, resources/templates/list, resources/read, server/discover), all `private`, ttl 5000 ms. - Ticks T063/T064 in specs/058-mcp-2026-upgrade/tasks.md. - `internal/server/cache_hints_test.go`: drives a modern-era request and asserts private scope and the short TTL on each list method. ## Tests - New tests failed to compile before the change (helpers undefined), pass after. - `go test -race` internal/server/... passes, bare and `-tags server`, with the CI skip regex. - personal and server-edition builds OK. golangci-lint (both runs) reports 15/18 issues, all in files this PR does not touch (e.g. tests/oauthserver, SA1019 on Go 1.26); none in changed files. - Not run: `scripts/test-api-e2e.sh` (the change only adds server options; the shared machine was saturated). ## Assumptions - All listings are caller-dependent (profile Spec 108, agent-token scope Spec 105, quarantine), so none is provably caller-independent and none is public. - 5 s TTL is a conservative freshness window; list_changed still notifies connected sessions. - The client-facing transport is pinned to the legacy era (FR-028), where mcp-go does not emit these fields, so the hints take effect only when that pin is lifted. The test therefore uses an unpinned transport. Spec: #1495 Review: opencode github-copilot/gpt-6.1-sol, 2 rounds, clean; follow-ups: hints are inert on the production transport until the FR-028 legacy-era pin is lifted (test uses an unpinned transport)
Dumbris
added a commit
that referenced
this pull request
Oct 6, 2026
…rrors (Spec 113-a) (#1498) ## Problem Gaps G1 and G2 of the Anthropic MCP-proxy talk audit ("How Anthropic uses MCP for its own product", https://www.youtube.com/watch?v=e8DMLtP5ibk), re-verified in the Spec 113 research: - **G1**: OAuth token refresh was not serialized. mcp-go's `OAuthHandler.getValidToken` refreshes with no lock whenever the token store reports an expired token, and the RefreshManager's proactive path (`RefreshOAuthTokenDirect`, two sub-paths) refreshes independently. With an authorization server that rotates refresh tokens, N concurrent callers at expiry sent N refresh requests with the same refresh token. Providers with reuse detection revoke the whole grant, so the user has to sign in again. `PersistentTokenStore.SaveToken` also read the record and wrote it in two transactions, so a concurrent DCR write was lost. The new test reproduces both bugs on `main`. - **G1+**: the RefreshManager looked up token records by display name, but records are keyed by `GenerateServerKey(name, url)`. The token-age log and the `oauth.token_refreshed` event never fired. - **G2**: refresh errors were classified by substring. `invalid_client` fell through to `failed_other`, so a dead DCR client was retried up to 50 times. 5xx and 429 were not told apart from other errors. ## What changed - **`oauth.RefreshCoordinator`** (new, process-wide): runs at most one flight per server key. The flight runs on a detached context with a 30 s timeout. Inside the flight it first re-reads the record, and if the refresh token was already rotated it returns the stored token with no network call. All waiters get the same token or the same error. After a terminal failure it latches, and after a transient failure reactive callers get a cooldown that mirrors the RefreshManager backoff. The latch and cooldown are keyed by grant generation (refresh-token and client-id fingerprint), so a login clears them. A flight whose generation was superseded during the request (for example by a login) has its result discarded and causes no side effects (FR-006a). Each flight logs one Info line with server, trigger, outcome class and whether the network call was skipped. - **Reactive path:** core binds the live handler's refresh function to the token store handed to mcp-go (`c.bindOAuthRefresher(oauthConfig)` after each OAuth `c.client =`). A bound `GetToken`: - refreshes through the coordinator inside the refresh margin; - returns the token with `ExpiresAt` zeroed, so mcp-go's unlocked `refreshToken` can never run; - on terminal failure returns an error wrapping `transport.ErrOAuthAuthorizationRequired`; - on transient failure returns `oauth.ErrTokenRefreshTransient`. Unbound stores (read-only probes) behave as before. The CLI in-memory store uses the same coordinator, keyed by server name. - **Proactive path:** `RefreshOAuthTokenDirect` runs a coordinator flight with the same refresh function. That function uses `handler.RefreshToken` when the handler has a usable client id and the stored DCR credentials otherwise, and never sends an empty `client_id`. - **Storage:** `BoltDB.UpdateOAuthToken` (one read-modify-write transaction) and `ClearOAuthClientCredentialsIf` (compare-and-clear). `SaveToken` and the manual refresh persist through `UpdateOAuthToken`. - **Classification:** `ClassifyRefreshError` reads the RFC 6749 §5.2 code from `transport.OAuthError` and the HTTP status from the typed `RefreshHTTPError` or mcp-go's fixed non-JSON text. It also recognises network error types. Substring matching is kept only as the last fallback, and each pattern is tested. | Class | Handling | |---|---| | `invalid_grant` | Terminal | | `invalid_client`, DCR client | Registration cleared once (compare-and-clear on the client id and the refresh token the request used); terminal with "sign in again to re-register". A second rejection before any successful token response, from refresh or from the login code exchange, is terminal with "rejects new client registrations". | | `invalid_client`, static client | Terminal; credentials untouched; message points at `oauth.client_id` / `oauth.client_secret` | | `unauthorized_client`, `unsupported_grant_type`, `invalid_scope` | Terminal | | `server_error`, `temporarily_unavailable`, 5xx, 429, network | Existing backoff | | Anything else | Existing backoff, then the existing give-up | New metric labels: `failed_invalid_client`, `failed_server_error`. Existing labels are unchanged. - **RefreshManager:** looks up records by server key, treats every terminal class as terminal and reports each one once, and learns about terminal outcomes of reactive flights through the coordinator completion hook. - **Docs:** `docs/features/oauth-authentication.md` covers refresh serialization, the error classes and the metric labels. ## Tests - New: `refresh_errors_test.go` (table covering every class and fallback pattern), `refresh_coordinator_test.go` (single flight with N=50, stale observed token, shared failure, cancelled initiator, parallel keys, terminal latch, transient cooldown, stale flight after login, `invalid_client` DCR/static/connect-flow/compare-and-clear, login-exchange rule), `persistent_token_store_refresh_test.go`, `persistent_token_store_race_test.go`, `memory_token_store_refresh_test.go`, `refresh_manager_invalid_client_test.go`, `storage/bbolt_oauth_update_test.go`. - **SC-001:** `internal/upstream/core/oauth_refresh_race_test.go` uses a rotating `httptest` AS that revokes the grant on reuse, a protected Streamable HTTP MCP server, and a real mcp-go client. 50 concurrent `CallTool`s plus one concurrent `RefreshOAuthTokenDirect` on an expired token produce exactly 1 token request, all calls succeed, and the request carries a non-empty `client_id`. It covers both sub-paths and passes under `-race -count=20`. With the binding disabled, the same test fails: the grant is revoked and the calls get "authorization required". - `go test -race` passed for `internal/oauth/...`, `internal/storage/...`, `internal/upstream/...`, `internal/observability/...`, `internal/runtime/...` and `internal/appctx/...`. The coordinator, store and storage tests also pass with `-count=20`. - `go build ./cmd/mcpproxy` and `go build -tags server -o /dev/null ./cmd/mcpproxy` both succeed. - I ran golangci-lint v2 with `.github/.golangci.yml`, bare and with `--build-tags server`. Neither run reports anything in changed or new files. Both runs do report findings in files this PR does not touch (for example `redactview.go`, `connection_stdio.go` and `tests/oauthserver/jwks.go`). - `./scripts/test-api-e2e.sh` against an isolated instance (`LISTEN_PORT=18347`): passed. On the first run the launcher-lifecycle reconnect check timed out once; the rerun passed fully, as did `origin/main` on the same port. - No REST, OAS or config change, so `make swagger` was not needed (FR-013). ## Assumptions - **Refresh margin for short tokens:** half the lifetime, capped at 30 s. The spec's "half their lifetime or 30 s" is read as the smaller of the two, so a 30 s token is not refreshed on every request. Lifetime is estimated from the record's `Updated` time. - **Still-valid access token:** if a refresh fails while the current access token is still valid (inside the 5 min grace), `GetToken` keeps serving it. The terminal or transient error is returned only once the token has actually expired, so an AS outage does not fail calls early. The RefreshManager is told about terminal outcomes at once through the hook. - **Proactive fresh window:** a proactive refresh within `MinRefreshInterval` (5 s, the RefreshManager's own scheduling floor) of a token save, for a token with more than 5 s left, is answered from storage (see the CI note below). - **No login-flow exemption for the DCR clear (deviation from FR-009's wording):** `tryOAuthAuth` holds a connection-wide OAuth flow while its own reactive refresh runs, so the exemption kept a rejected registration forever (every sign-in reloaded it and skipped DCR). The compare-and-clear on client id plus refresh token already protects any registration or grant a login saved. A code exchange that still uses the cleared client is told to sign in again, not that the AS rejects new registrations. - **Cooldown scope:** the transient cooldown applies to reactive callers only. The proactive path keeps its own unchanged backoff (10 s doubling to a 5 min cap, `DefaultMaxRetries` 50). - **DCR handler mismatch:** when the live handler's client id differs from a newer stored DCR client id, the stored credentials are used. When neither is available the refresh is terminal (`ErrNoClientCredentials`) and never sends an empty `client_id`. - **Binding sites:** the handler is bound at the five OAuth client-creation sites in `connection_oauth.go` (one helper call each), not at a single site as the plan suggested, because there is no shared post-creation point. The three authorization-code exchange sites call a one-line annotation helper, which implements the FR-009 login-flow rule. - **RefreshManager lookup:** `RefreshManager` knows only the display name, so FR-010 matches the record by `DisplayName` (most recently updated wins), after trying a legacy record keyed by the bare name. - **Reconnect fallback:** `upstream.Manager.RefreshOAuthToken` is unchanged and keeps its reconnect fallback on failure. ## CI shuffle failure (run 37307361769) `TestOAuthRefreshRace_OneTokenRequestPerExpiry/stored_DCR_credentials_sub-path` saw 2 token requests, both valid (`rt-0` then `rt-1`, no replay). Root cause was in production code: `RefreshOAuthTokenDirect` reads the record when it runs, not when the RefreshManager timer fired. When the reactive flight had already rotated the token, the proactive caller observed the new refresh token, so the FR-003 "already rotated" check could not tell, and a second flight rotated the just-minted grant. In production that is the timer firing while a reactive refresh (or a login) completes, before `OnTokenSaved` reschedules. Fix: the coordinator records the last token save per key and answers a proactive request within the fresh window from storage. `TestOAuthRefreshRace_LateProactiveAfterReactiveFlight` reproduces the CI ordering deterministically for both sub-paths (it failed before the fix). The race test's AS now holds the token request for 50 ms so callers overlap the flight. Verified with the CI seed (`-shuffle=1791202369572320301 -count=20`), `GOMAXPROCS=1 -count=50` and `-count=50 -cpu 1,2,4` under `-race`. ## Review fixes (opencode gpt-6.1-sol) Stale-flight side effects (latch re-read, DCR compare-and-clear now also on the refresh token, schedule binding for terminal outcomes and retries, logout during a flight or a grace refresh hands out no token, DCR bookkeeping after a concurrent login); DCR recovery during a connect attempt; secret redaction of refresh errors (pattern scrub plus the exact sent credentials in raw, JSON- and URL-escaped spellings, decoded JSON error fields, error code field); short-token handling (CLI memory store save time, proactive window); `oauth.extra_params.client_id` grants stay refreshable; long non-JSON bodies keep their RFC 6749 code, while 5xx/429 stay transient. Spec: #1495 Review: opencode github-copilot/gpt-6.1-sol, 8 rounds, clean (round 8 VERDICT: CLEAN); follow-ups: proactive refreshes are not held by the reactive transient cooldown (by design, RefreshManager has its own backoff); the still-valid access token is served during the refresh margin even after a terminal refresh error (the hook applies the terminal outcome to the schedule at once).
Dumbris
added a commit
that referenced
this pull request
Oct 6, 2026
…Spec 113-b) (#1499) ## Problem Slice 113-b of Spec 113 (gaps G3 and G5 from the Anthropic MCP-proxy talk audit): - G3: mcpproxy never asked for `offline_access`, never warned when the authorization server cannot issue refresh tokens, and had no way to point a server at an authorization server whose metadata is non-standard or advertises an unusable endpoint. - G5: every `createOAuthConfigInternal` call re-fetched the same PRM and authorization server metadata, and `extra_params` injection used a `/token` / `/authorize` path heuristic. ## What changed - New optional per-server `oauth.authorization_endpoint`, `oauth.token_endpoint`, `oauth.registration_endpoint`, `oauth.auth_server_metadata_url` (https, or http on a loopback host; no query, fragment, userinfo or whitespace). Validated at write time only (`OAuthConfig.Validate`, `Config.ValidateDetailed`); an invalid value already on disk is dropped at load with a field-name-only warning, so boot never fails. - Endpoint overrides are applied by rewriting the authorization server metadata inside `OAuthTransportWrapper` (mcp-go has no endpoint config). With any endpoint override mcpproxy always passes mcp-go a metadata URL (override, else discovered, else RFC 8414 URL from the override origin), and when that metadata is unreachable and authorization + token overrides are set the wrapper serves a synthesized document, so mcp-go never falls back to `/authorize`, `/token`, `/register`. - `extra_params` injection and 201 to 200 normalization match on the effective (overridden or discovered) token endpoint URL; the path heuristic remains only when no endpoint is known. - `offline_access` is appended only when the scopes were taken from PRM `scopes_supported` and the AS metadata `scopes_supported` contains it. Explicit `oauth.scopes` and AS-derived scopes are untouched. - One Warn per server per process when AS `grant_types_supported` is present (including an empty list) and lacks `refresh_token`. - Discovery cache (`internal/oauth/discovery_cache.go`): key = hash(server URL, four overrides, kind); 1 h success TTL, 30 s failure TTL, single-flight, LRU 256. Covers the HEAD/POST preflights, PRM fetches, the scope fallbacks and the AS metadata document (shared with the transport wrapper so mcp-go's own metadata GET is served from the cache). A 401 advertising a different `resource_metadata` URL invalidates the server's entries, including in-flight fetches. - OAuth HTTP client redirect policy: refuses https to http on a non-loopback host and any cross-origin redirect of a POST (compared with effective ports). - Config-field checklist: `OAuthConfigChanged`, `MergeOAuthConfig`/`copyOAuthConfig`, `ServerFieldMaskDecisions` rows, contracts OAuth projection + runtime/management plumbing, `make swagger` (oas/ committed; `cmd/generate-types` produced no TS diff), `docs/configuration.md`. Per-server fields, so no `MCPPROXY_` env override. Echoed masks of the four fields are reverted on write (`UnmaskLiveOAuth`), because the generic read view's name rule masks leaves containing "auth"/"token". - `refresh_manager.go` and `persistent_token_store.go` are untouched (owned by slice a). ## Tests run - Written failing first, then implemented: config (round trip, merge, change detection, validation table, load-time sanitize, whitespace), storage round trip, discovery cache (TTLs, keys, single-flight, LRU, invalidation, atomic seeds), transport wrapper (rewrite, synthesis, learned token endpoint, heuristic fallback, redirects, log redaction), end-to-end with a real mcp-go `OAuthHandler` (authorization URL host equals override, exchange + refresh hit the override, advertised `/std/*` endpoints never hit), `createOAuthConfigInternal` integration (5 calls = 1 request per discovery URL, separate keys per override set, failure cached briefly, offline_access table, refresh-grant warning, PRM-URL invalidation, unmask of echoed masks). - `go test -race` on `internal/config`, `internal/storage`, `internal/contracts`, `internal/management`, `cmd/generate-types`, `internal/oauth`: pass. - `go test -race -tags server -skip <CI regex>` on runtime, httpapi, upstream, config, oauth, server/tokens: pass. `internal/server` (same flags, 45m timeout) also passes; a first run with the 20m CI timeout hit the timeout on the loaded shared machine (load average about 9), unrelated to this change. - `go build ./cmd/mcpproxy` and `go build -tags server -o /dev/null ./cmd/mcpproxy`: pass. - golangci-lint v2 with `.github/.golangci.yml`, bare and `--build-tags server`: no findings in touched files (the remaining repo-wide findings are pre-existing Go 1.26 deprecation notices, e.g. `ecdsa.PublicKey.X`). - `./scripts/test-api-e2e.sh` on isolated ports (18177/18179/18180): the only consistent failure is `Audit log: server-edition binary present`, which fails identically on `origin/main` (reproduced on a clean checkout at 18178). Two of the three runs also hit a launcher-test reconnect flake under machine load; the third run was green apart from the audit item. - Review: opencode `github-copilot/gpt-6.1-sol`, 4 rounds (round 1 in three chunks, then three fix-and-re-review rounds). Round 1 found 9 issues, all fixed with tests (in-flight fetch repopulating after invalidation, negative cache bypass, redirected metadata, default-port origin match, POST preflight not recording the PRM URL, transient-PRM-failure fallback pinned for 1 h, empty `grant_types_supported`, whitespace validation bypass, masked echo on write); rounds 2 and 3 found two more cache-seeding issues, fixed by storing seeds atomically with the primary result; round 4 verdict CLEAN. ## Assumptions - Overrides are plain per-server config fields with no env var; PATCH merge keeps the base value when the patch value is empty (same as the other scalar OAuth fields). - The four override fields are labelled `NotSecret` in `ServerFieldMaskDecisions` (typed projection shows them to the UI); the write door still reverts echoed masks from the generic view. - `internal/upstream/core/connection.go` is not edited: the PRM-URL invalidation hook lives in `oauth.handleUnauthorizedResponse` (resource auto-detection already sees every live 401 `resource_metadata`), which keeps the slice inside `internal/oauth`. - An authorization-server fallback result found on the MCP origin after a failed PRM step is cached only for the failure TTL, not an hour, so a transient PRM failure cannot pin a wrong AS. - Known limitation: after a same-origin 307/308 redirect of a token POST, the learned-endpoint match no longer applies the 201 to 200 normalization to the redirect target. - The task text placed the end-to-end override test under `internal/upstream/core`; it lives in `internal/oauth` (`oauth_overrides_e2e_test.go`) because it only needs mcp-go's handler and the config this package builds. Spec: #1495 Review: opencode github-copilot/gpt-6.1-sol, 4 rounds, clean; follow-ups: after a same-origin 307/308 redirect of a token POST the learned-endpoint match no longer normalizes the 201 response
Dumbris
added a commit
that referenced
this pull request
Oct 7, 2026
…t_domain (Spec 113-c) (#1502) ## Problem A tool call that fails shows only `status=error` plus prose in the Activity log. Nobody can tell whether the network dropped, the upstream returned HTTP 502, the upstream answered with a JSON-RPC error, the tool reported a business error (`isError:true`), the session expired, auth failed, or mcpproxy itself refused the call. Gap G6 of the Anthropic MCP-proxy talk audit (Spec 113, slice 113-c). ## What changed - New `internal/callerr`: a pure classifier `Classify(result, err, facts)` returning `error_class` (network, timeout, http, jsonrpc, tool_error, session_terminated, auth, proxy_policy, proxy_internal, cancelled), `fault_domain` (upstream, proxy, client) and `upstream_http_status`. Rules follow FR-041 in order. `Outcome.AuditClass()` is a total mapping onto the frozen Spec 107 audit vocabulary; `auditToolCall` uses it, so the audit line and the activity record agree. - `internal/transport`: a per-call `CallRecorder` round tripper, installed as the outermost layer in `upstreamRoundTripper` (covers every `CreateHTTPClient` branch and SSE). mcp-go flattens non-2xx responses into an untyped string, so the status is recorded at the transport. Requests without a recorder in their context are untouched. - `core.Client.CallTool` marks the call dispatched at the exact point it hands it to mcp-go (every transport) and returns a typed timeout error with an unchanged message (it was a bare `fmt.Errorf` that dropped the chain). - `storage.ActivityRecord` gains `error_class`, `fault_domain`, `upstream_http_status`, all `omitempty`. A pre-113 JSON fixture round-trips byte-identical. `isError` results keep `status=error` and are told apart by `tool_error`. - Stamped at every tool-call completion path through the one funnel (`call_tool_*`, legacy `call_tool`, direct surface, `code_execution` sub-calls, REST `/api/v1/tools/call`); limiter sheds are stamped `proxy_policy/proxy`. - REST: `contracts.ActivityRecord`, SSE completion payload (forwarded verbatim by `/events`), `error_class` / `fault_domain` query filters on list and export. `make swagger` run, `oas/` committed. CLI: `activity list|export --error-class --fault-domain`, `activity show` prints an `Error class` line, `-o json|yaml` carry the fields. Docs updated. ## Tests run - `go test -race` (no skips needed): callerr, transport, storage, contracts, httpapi, audit, upstream/core, cmd/... all green. - `go test -race -tags server` with the CI skip regex: runtime, upstream/..., serveredition/... green; `internal/server` green when run alone (1150 s). A combined run with runtime hit the 20 m timeout from CPU contention on the shared machine. - New: classifier table tests (every FR-041 rule, `isError`, 502 with untyped error, `-32001` after a recorded 200, narrow fallback list), `AuditClass` mapping, recorder installed in every client branch and concurrency isolation, record JSON compatibility, runtime emit->persist, and an `httptest` Streamable HTTP upstream integration test (SC-004) asserting persisted record and `GET /api/v1/activity` JSON for isError, 502 HTML, JSON-RPC -32001, connection reset, timeout, 404 session, 401, pre-dispatch validation and success, plus direct / code_execution / REST write paths. - Both golangci-lint runs (bare and `--build-tags server`): no findings in touched files (remaining findings are pre-existing, in untouched files). - `./scripts/test-api-e2e.sh` on an isolated port with unique results/log paths (another session was running the same script concurrently; shared `/tmp` files and fixture port 39933 made the stock run flaky on both this branch and `origin/main`): 65/66; the one failure, `Audit log: server-edition binary present`, also fails on `origin/main` (no server-edition binary built). ## Assumptions - A call to a server that is not connected, or has no client, is `network/upstream`; an error completion that no path classified is `proxy_internal/proxy`; a `blocked` completion with no noted outcome is `proxy_policy/proxy`. - The fallback text list (FR-049) is `connection refused|reset`, `no such host`, `broken pipe`, `unexpected eof`, `i/o timeout`, `Client.Timeout exceeded`, `context deadline exceeded`, consulted only when the call never left the proxy or HTTP requests went out with no response, so an upstream's own message is never reclassified. - The mcp-go `*transport.Error` wrapper counts as `network` only after the recorded non-2xx rule, because mcp-go wraps HTTP errors in it (found by the integration test). - `cmd/generate-types` does not generate `ActivityRecord`, so the hand-written `frontend/src/types/api.ts` interface got the three optional fields instead. `call_tool_*` wrapper records (`internal_tool_call`) and activity replay are not stamped; the canonical `tool_call` record is. - No config fields added (config-field checklist not applicable). `internal/health/**` and `internal/upstream/managed/client.go` untouched. Spec: #1495 Review: opencode github-copilot/gpt-6.1-sol, 2 rounds, clean; follow-ups: none (round 1 rejected: *transport.Error-before-http ordering is an intentional documented deviation)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds Spec 113 (speckit format: spec, plan, research, tasks, requirements checklist). It drives fixes for the gaps found by the 2026-10-01 audit of mcpproxy against the talk "How Anthropic uses MCP for its own product" (https://www.youtube.com/watch?v=e8DMLtP5ibk). Only spec files and the regenerated
ROADMAP.mdchange. There is no code in this PR.Each gap was checked again against
origin/main@8442428d1. Evidence is inresearch.md§1.RefreshOAuthTokenDirectvia the handler, and the manual stored-DCR path, which writes back a stale whole record.SaveTokenreads and writes in two separate transactions.RefreshManagerlooks up token records by display name instead of server key (refresh_manager.go:363,:666). The refreshed event therefore never fires.invalid_grantis already terminal. The real gaps are thatinvalid_clientand 5xx fall intofailed_otherand retry up to 50 times. mcp-go wrapstransport.OAuthErrorwith%w, so the errors can be classified by type.OAuthConfighas no endpoint fields, so endpoint overrides have to rewrite the metadata document inside the transport wrapper.error_classvocabulary. It is audit-only, and itstransport.HTTPError/JSONRPCErrorbranches have no production producers.HealthCalculatorInputis built at three places.ErrSessionTerminatedand clears the session id. mcpproxy flips the server toErrorbecause of the substring "terminated".ttlMs:0, cacheScope:"private"to modern clients by default. The remaining work is making this explicit and adding tests.serveredition/setup.gosays. Deferred.Slices
Each slice is an independent PR based on
main. Slices do not stack and do not edit the spec files.plan.mdhas a file-ownership table.Assumptions (made autonomously)
ExpiresAtzeroed, and mcpproxy owns expiry. This is the only way to stop mcp-go's unlockedgetValidTokenrefresh without forking mcp-go.cancelledto the 9-class list so that a caller cancelling a call is not counted as an upstream fault.tools/callafter a session re-init only for read-only tools whose identity is unchanged. Write and destructive calls are not repeated, because a 404 does not prove the server never executed them.private. Every listing depends on the caller and on approval state.Review
Reviewer: opencode
github-copilot/gpt-6.1-sol(variant high), split into 3 chunks.GetToken; the reactive path lacked the DCR fallback; no terminal latch or cooldown; DCR clear was not compare-and-clear; override bypass through mcp-go default endpoints; redirect downgrade; Spec 105 pinning on re-init; double execution of non-idempotent calls on a 404; a health ping clearing the session id.errors.Asvalue target, query-string overrides, cache coverage, stdio jsonrpc/dispatched facts, the calculator's early healthy return, sanitisation audit mapping, and registry lifecycle.