fix(mcpcompat): close functional gaps vs mcp-go for loss-free migration (#156)

[JAORMX]

Summary

Closes #156 — closes the mcpcompat functional gaps vs mcp-go so the ToolHive migration (stacklok/toolhive#5729) lands with zero functional loss.

Five commits, one per wave, each reviewed by an independent panel (spec + standards + domain specialists + QA) with findings iterated before commit.

Wave	Commit	Items
1	b22b8e1	U1 (merge blocker) capability flags → ServerOptions.Capabilities; U3 (merge blocker, security) per-request context bridge rekeyed to crypto/rand nonce; U6 normalize only nil/empty input schemas + recover per-session AddTool panics
2	a302089	1a+U8 register progress/logging notification handlers + carry params; 1b confirm list_changed auto-emitted by go-sdk on feature mutation
3	3c7ab69	2 WithHeartbeatInterval → ServerOptions.KeepAlive; 5 WithDisableLocalhostProtection(bool); 6 WithPageSize(n int); 3/U5 per-request Validate for local sessions
4	19c482b	4/U7 scope 401 string-matching to transport-level errors (no false-positive OAuth refresh on JSON-RPC tool errors); restore any-4xx-on-initialize → ErrLegacySSEServer
5	884cfd3	U4 elicitation surface (client ElicitationHandler + server RequestElicitation); U9 populate before-hook request objects (tool name + args + cursor); U2 document preset session ID limitation (upstream-only); 7 fix remaining stale doc comments

What landed (15 items)

Shim fixes (all 15 work items from the issue + 2 audit comments)

• ✅ U1 — WithToolCapabilities/WithResourceCapabilities/WithPromptCapabilities mapped to go-sdk ServerOptions.Capabilities; vMCP advertises tools/resources/prompts at initialize even with zero pre-registered features.
• ✅ U3 — pendingReqCtx rekeyed from session-ID to a per-POST crypto/rand nonce (X-MCP-Req-Nonce); eliminates the cross-request context-bleed / identity-attribution race. go-sdk does not propagate per-POST context into session middleware (verified), so the bridge is retained but keyed per-request.
• ✅ U6 — normalizeObjectSchema narrows to nil/empty-only; $ref/oneOf/boolean pass through verbatim; per-session AddTool panics recovered and skipped.
• ✅ 1a + U8 — ProgressNotificationHandler + LoggingMessageHandler wired in the client; dispatch carries params via toNotificationParams (JSON round-trip, _meta split).
• ✅ 1b — Confirmed go-sdk auto-emits tools/list_changed on SetSessionTools (no production code needed beyond U1); corrected stale doc comments.
• ✅ 2 — WithHeartbeatInterval wired to ServerOptions.KeepAlive (was a silent no-op).
• ✅ 5 — WithDisableLocalhostProtection(bool) exposed (default: protection ON).
• ✅ 6 — WithPageSize(n int) exposed (default 0 → go-sdk's 1000).
• ✅ 3/U5 — SessionIdManager.Validate called for local sessions too; restores per-request liveness + sliding TTL.
• ✅ 4/U7 — 401 string-matching scoped to transport-level errors via captured HTTP status + errors.As(*jsonrpc.Error); false-positive OAuth refresh on tool-error text eliminated; any-4xx-on-initialize → ErrLegacySSEServer restored.
• ✅ U4 — Elicitation surface added (client ElicitationHandler/WithElicitationHandler/NewStreamableHttpClientWithOpts; server SessionWithElicitation/RequestElicitation/ErrNoActiveSession/ErrElicitationNotSupported); JSONResponse mode documented (requires client continuous listening).
• ✅ U9 — Before-hook request objects populated (mcp.CallToolRequest with name + args, mcp.ListToolsRequest with cursor); latent translateUnknownToolError type-assertion bug fixed (*CallToolParams → *CallToolParamsRaw).
• ✅ U2 — Preset session ID limitation documented loudly (upstream-only: go-sdk has no SessionID field on StreamableClientTransport; honored via resume path only).
• ✅ 7 — All stale doc comments fixed (WithSessionIdManager, package doc, SessionWithTools, capability-flag comment, OnNotification, notifications.go, normalizeObjectSchema).

Upstream asks (documented, not implemented — tracked in the issue)

• Public generic notification sender on ServerSession.
• Preset session ID on StreamableClientTransport (blocks U2).
• Per-request session Validate/Terminate hooks.
• Typed HTTP-status errors from transports.
• Per-request context propagation to stateful session handlers (U3 root cause).

Test coverage

Each item has e2e and/or unit tests that would fail without the fix:

• Capability advertisement e2e (TestAdvertisesToolsCapability_WithNoGlobalTools)
• Per-request context isolation under -race (verified to fail without the bridge)
• Schema normalization table + non-object schema panic recovery
• Progress/logging notification e2e with param assertions
• list_changed e2e on SetSessionTools
• toNotificationParams unit table (meta split, error paths, nil guards)
• Heartbeat eviction e2e
• Localhost-protection toggle e2e (raw non-localhost Host)
• Page-size pagination e2e with nextCursor
• Local-session validate-evicts-on-shared-terminate
• False-positive JSON-RPC error isolation + transport 401/404/403 classification + 4xx-on-initialize legacy SSE
• Elicitation e2e (works with continuous listening, fails without) + handler error return + guard branches
• Before-hook tool name + args + cursor
• Unknown-tool error contains tool name (regression test for translateUnknownToolError)

Verification

• task lint — 0 issues (golangci-lint + go vet)
• task test — all packages pass with -race
• task license-check — clean

Review process

Each wave was independently reviewed by a panel (spec adherence, standards conformance, secure-code-reviewer, software-architect, devex-reviewer, QA test-expert) in fresh contexts. Cross-confirmed findings were iterated before commit:

• Wave 1: U6 per-session panic recovery (cross-confirmed by devex + security) + U3 test verification
• Wave 2: toNotificationParams unit tests + nil-param guards + broadcast delivery assertion
• Wave 3: localhost test green-when-broken fix + variadic-bool → plain bool + heartbeat doc
• Wave 4: security-verified no 401 swallowing path
• Wave 5: translateUnknownToolError regression test + elicitation guard branches + doc fixes

Refs #156

[JAORMX]

(Submitted as COMMENT — GitHub rejects REQUEST_CHANGES on one's own PR; treat this as request-changes.) Verified each fix against go-sdk v1.6.1 and mcp-go v0.55.1 sources; U1/U3/U9/5/6 and the 4/U7 core are correct. Four issues must change first — see inline comments (heartbeat session eviction, global schema panic → poisoned sync.Once, Validate-error forgetSession, empty-body 4xx capture). Separately, the "zero functional loss" claim needs reconciling: notifications/tools/list_changed and notifications/message are still lost end-to-end through ToolHive's stdio bridge — the bridge never mutates its feature set (so go-sdk never auto-emits) and doesn't call WithLogging (so clients never setLevel and ServerSession.Log drops everything; the tests only pass because they call SetLoggingLevel first). Those need toolhive-side companions to #5729: WithLogging() + feature re-sync on upstream list_changed in the bridge, and WithPageSize in vMCP.

[jhrozek]

Reviewed the full diff (17 files, ~3.1k LOC) plus ran go build, go vet, go test -race ./mcpcompat/..., golangci-lint, and task license-check — all green. The PR is high quality: the per-request nonce context bridge (U3), the 401-string-scoping to transport-level errors (wave 4), the capability-flag wiring (U1), and the schema-normalization narrowing (U6) are all correctly reasoned and well-documented. Findings below are all non-blocking nits/minor gaps; none block merge.

One additional note that I couldn't anchor inline (the relevant code is pre-existing, not in this PR's diff): newClientSession (mcpcompat/server/session.go:98-110) spawns a drain goroutine for range cs.notifCh that never terminates and is never stopped on forgetSession. So every clientSession ever created leaks a goroutine for the process lifetime. This is distinct from the documented sessions/localSessions map-growth gap (server.go:148-174): even if the planned reaper drops map entries, the goroutine stays alive because nothing closes notifCh. If/when the reaper lands, closing notifCh in forgetSession (session.go:316) would let the drain goroutine exit — but it needs a sync.Once or LoadAndDelete guard against double-close, since sessionFor can re-create an entry for the same ID. Not a blocker for this PR since it's pre-existing, but worth tracking alongside the reaper work.

[jhrozek]

Review of the mcpcompat functional-gaps changes. Build, go test -race ./mcpcompat/..., and golangci-lint are all green, and the PR is unusually well-documented. The inline comments below are gaps/edge-cases worth a look before merge; my one blocking concern is the mapConnectError asymmetry (first comment).

Overall: request a look at the first inline comment before merge; the rest are non-blocking (a question, a leak worth folding in, and follow-up/doc notes).

---

Additional non-inline note — pre-existing goroutine leak (not introduced by this PR, but the fix would fit naturally here):

newClientSession (server/session.go:98-110) spawns go func(){ for range cs.notifCh }(), and notifCh is never closed anywhere in the package. forgetSession (session.go:316-319) drops the map entries but doesn't close the channel, so the drain goroutine — plus the clientSession it references — leaks for the process lifetime, one per session ever created. This compounds the documented finding-5 (unbounded sessions/localSessions growth). Both the drain goroutine and forgetSession predate this PR (introduced in bcdfd8b, already on main), so it's strictly out of scope — flagging because forgetSession is the natural teardown point to close notifCh (via sessions.LoadAndDelete so a DELETE-then-Validate double call can't double-close), and this PR is in the neighborhood. Fine to defer to a follow-up.

Skipped as INFO-only: (a) logging capability advertised only with WithLogging — matches mcp-go and delivery isn't capability-gated, just worth confirming log-emitting servers call WithLogging; (b) errorbody.go:72-78 comment says it "restores" resp.Body on a second error response but installs an empty one — traced unreachable in the current Connect flow.

[JAORMX]

Wave 6 — addresses jhrozek's review findings

Pushed eb17064, all findings addressed. task lint (0 issues), task test (-race, all green), task license-check (clean).

1. 5xx false-positive OAuth refresh (MEDIUM — errors.go:129)

Fixed. mapTransportError now surfaces a captured 5xx unchanged — the string fallback only runs when no status was captured at all (h == nil || h.status == 0). A 5xx body containing "401"/"unauthorized" no longer triggers ToolHive's OAuth refresh.

Refactored mapTransportError into mapStatusError (status-driven) + mapStringError (string fallback) for readability and to stay under gocyclo 15.

Tests: TestMapTransportError_5xxWithUnauthorizedBodyNotMisclassified (500/502/503 cases), TestMapCallError_5xxSurfacesUnchanged.

2. Rehydrated transient Validate error inconsistency (MEDIUM — transports.go:343)

Fixed. serveRehydrated now distinguishes a transient error on a cached rehydrated session (→ 503, preserving the cache, matching the local path) from an unknown session that was never cached (→ 404 "Invalid session ID", unchanged). The distinction is getRehydrated(sid) != nil: if we have a cached reconstruction, the session WAS valid, so the error is likely transient.

Test: TestRehydratedSession_TransientValidateError_Returns503 (flaky manager flakes once → 503, then session still cached and served on retry).

3. extractResourceMetadataURL stub (MINOR — errors.go:77)

Fixed — no longer blocked by the go-sdk. The shim's own headerRoundTripper has direct access to resp.Header and now captures WWW-Authenticate values into errBody.wwwAuthHdrs (in captureErrorBody). extractResourceMetadataURL reads from the captured header and parses the RFC 9728 §5.1 resource_metadata parameter using a direct port of mcp-go's well-tested extractResourceMetadataURLs parser (new file rfc9728.go). AuthorizationRequiredError.ResourceMetadataURL is now populated end-to-end through both mapCallError and mapConnectError.

Tests: TestExtractResourceMetadataURL_FromCapturedHeader (5 cases including quoted/token values, multi-param, empty), TestMapCallError_Transport401_PopulatesResourceMetadataURL (e2e), TestMapConnectError_Transport401_PopulatesResourceMetadataURL (e2e), TestCaptureErrorBody_CapturesWWWAuthenticate (e2e through the RoundTripper).

4. normalizeObjectSchema branch unification (NIT — server.go:816)

Fixed. Unified the two branches (empty-type-string and no-type-key) into a single hasType && typeIsStr && typeStr == "" / !hasType check, preserving the semantic distinction: empty-string type is mcp-go's "no schema declared" sentinel (always normalize), while an absent type without properties/required (e.g. {$ref}, {oneOf}, {title}) passes through verbatim. A non-string type value (e.g. ["object","null"]) also passes through. All existing TestNormalizeObjectSchema cases still pass.

5. notifCh drain goroutine leak (pre-existing — session.go)

Fixed. forgetSession now closes notifCh via sync.Once (guarding against double-close when sessionFor re-creates an entry for the same ID). The drain goroutine in newClientSession exits when the channel is closed. A re-created session gets a fresh channel.

Test: TestForgetSession_ClosesNotifChannel (verifies close, double-forget safety, fresh channel on re-create).

6. rehydrate lock scope (LOW — transports.go:443)

Documented (per reviewer: "not worth restructuring now"). Added a scalability note to the rehydrate doc comment explaining the cross-sid serialization cost and the fix path if rehydration volume rises.

General note (notifCh drain goroutine)

The drain goroutine leak you flagged in the general review note is fixed by item 5 above — forgetSession now closes notifCh, which lets the drain goroutine exit. The sync.Once guard addresses the double-close concern you raised.

[JAORMX]

Wave 7 — addresses jhrozek's second review (the one I missed)

My apologies — jhrozek posted a second set of 5 inline comments at 09:27:55Z that I completely missed in wave 6. All 5 are now addressed in c469b88. task lint (0 issues), task test (-race, all green), task license-check (clean).

1. [BLOCKING] mapConnectError missing JSON-RPC guard (errors.go:58)

Fixed. mapConnectError now has the symmetric *jsonrpc.Error guard that mapCallError has: when h.status == 0 and err is a *jsonrpc.Error, the error is surfaced unchanged. An app-level JSON-RPC error at initialize (200 OK + error body) with "unauthorized"/"401" text is no longer misclassified as ErrUnauthorized → no false OAuth refresh.

Test: TestMapConnectError_DoesNotMisclassifyJSONRPCErrors (3 cases).

2. [Question] SetLevel compat alias (client.go:271)

Fixed — went with the compat layer. Added Client.SetLevel(ctx, mcp.SetLevelRequest) error (forwards to SetLoggingLevel) and re-exported SetLevelRequest/SetLevelParams from the mcp package. Downstream code using c.SetLevel(ctx, mcp.SetLevelRequest{...}) now compiles unchanged.

Test: TestSetLevel_CompatAlias (e2e).

3. [Compat gap] OnNotification drops resources/updated + elicitation/complete (client.go:583)

Fixed. Wired opts.ResourceUpdatedHandler (→ notifications/resources/updated with uri param) and opts.ElicitationCompleteHandler (→ notifications/elicitation/complete with elicitationId param). Updated OnNotification doc to list all 7 forwarded notification types.

Test: TestResourceUpdatedAndElicitationCompleteHandlers.

4. [Follow-up] No keep-alive (transports.go:176)

Documented + acknowledged. Strengthened WithHeartbeatInterval doc: the passive keep-alive is load-bearing (elicitation + all server→client notifications depend on the idle SSE stream), should be prioritized before internet-facing vMCP use. Tracked as TODO(issue #156).

5. [Doc note] 503 diverges from mcp-go's 404 (transports.go:358)

Documented. Added a compat-note comment at the 503 site: 503 is not mapped by the client classifier (errors.Is(err, ErrSessionTerminated) is false), so callers keying re-init off mcp-go's 404→ErrSessionTerminated path won't re-init on a transient blip. Intentional (avoids split-brain), but callers should be aware.

[jhrozek]

Approving — all review findings are resolved. Verified against c469b88 (build + client/mcp/server tests green, including the new TestMapConnectError_DoesNotMisclassifyJSONRPCErrors).

• mapConnectError JSON-RPC guard — fixed with the symmetric errors.As(&wireErr) guard + regression test.
• SetLevel drop-in parity — fixed via Client.SetLevel(ctx, mcp.SetLevelRequest) wrapper + SetLevelRequest/SetLevelParams aliases.
• notifCh goroutine leak — fixed (wave 6) with a sync.Once close in forgetSession.
• OnNotification dropped notifications — fixed by wiring ResourceUpdatedHandler and ElicitationCompleteHandler.
• 503 vs 404 divergence — compat note added at the 503 site.
• Keep-alive gap — acknowledged and documented with a prioritized TODO (passive SSE keep-alive). Reasonable to land as a tracked follow-up; worth prioritizing before internet-facing vMCP use, alongside the session-reaping / max-session-cap work also tracked under issue #156.

Nice, thorough iteration across the waves. 🚀