fix(mcpcompat): close functional gaps vs mcp-go for loss-free migration (#156)#157
fix(mcpcompat): close functional gaps vs mcp-go for loss-free migration (#156)#157JAORMX wants to merge 6 commits into
Conversation
…sue #156 wave 1) U1 — Map capability flags to go-sdk ServerOptions.Capabilities so vMCP advertises tools/resources/prompts at initialize even with zero pre-registered features (merge blocker). U3 — Rekey the pending-request-context bridge from session ID to a per-POST crypto/rand nonce (X-MCP-Req-Nonce), eliminating the cross-request context bleed / identity-attribution race for concurrent POSTs on the same session (merge blocker, security). go-sdk does not propagate the per-POST HTTP context into session middleware, so the bridge is retained but keyed per-request. U6 — Narrow normalizeObjectSchema to replace only nil/empty/empty-type schemas with {"type":"object"}; pass $ref/oneOf/boolean/object- with-type-omitted through verbatim (matching mcp-go). Recover go-sdk AddTool panics on the per-session path so a bad overlay schema skips the tool instead of crashing the session. Tests: capability advertisement e2e, per-request context isolation under -race (verified to fail without the bridge), schema normalization table, non-object schema panic recovery, nil-schema tools/list e2e. Refs #156
…ssue #156 wave 2) 1a + U8 — Register go-sdk ProgressNotificationHandler and LoggingMessageHandler in the mcpcompat client; route to the compat OnNotification callback. dispatch() now carries notification params via toNotificationParams (JSON round-trip, _meta split), restoring progress and logging notifications end-to-end with their params. 1b — Confirmed go-sdk auto-emits tools/list_changed on SetSessionTools (no production code needed beyond U1); corrected the stale doc comments in notifications.go that claimed list_changed is dropped. Added SetLoggingLevel + mcp.LoggingLevel aliases (required for the logging-notification path; renamed/simplified vs mcp-go's SetLevel, documented). Tests: progress+logging e2e with param assertions, list_changed e2e, toNotificationParams unit table (meta split, error paths, nil guards), handler nil-param guards, method-string coverage for all three list_changed variants, broadcast delivery assertion, early-exit on list_changed. Refs #156
…l-session validate (issue #156 wave 3) 2 — Wire WithHeartbeatInterval to go-sdk ServerOptions.KeepAlive (previously a silent no-op); sessions now close on unanswered pings. 5 — Expose WithDisableLocalhostProtection(bool) so callers can preserve pre-migration behavior for local proxies with custom Host headers (default: protection ON, matching go-sdk). 6 — Add WithPageSize(n int) ServerOption so aggregating servers (vMCP) can raise the tools/list page size above go-sdk's default 1000. 3/U5 — Call SessionIdManager.Validate on local sessions too, not just rehydrated ones; restores per-request liveness and sliding TTL so a session terminated cross-pod or via auth-failure takes effect on the origin pod immediately. Tests: heartbeat eviction e2e, localhost-protection toggle e2e (raw non-localhost Host, both enabled and disabled), page-size pagination e2e with nextCursor, local-session validate-evicts-on-shared-terminate and served-without-manager. Refs #156
…issue #156 wave 4) 4/U7 — Constrain 401/unauthorized/404 string-matching to transport-level errors only by checking the captured HTTP status (from errorbody.go) first and using errors.As(*jsonrpc.Error) to distinguish RPC-level from transport-level errors. A JSON-RPC tool error whose message contains "unauthorized" no longer triggers a false-positive OAuth refresh. Restore mcp-go's any-4xx-on-initialize → ErrLegacySSEServer classification (400/403/404/405 on connect → legacy SSE; 401 stays ErrUnauthorized). String-matching remains as a best-effort fallback for transport failures with no captured body. Tests: false-positive JSON-RPC error isolation, transport 401/404/403 classification by captured status, 4xx-on-initialize legacy SSE restoration, string-matching fallback. Refs #156
…on docs, stale comments (issue #156 wave 5) U4 — Document elicitation delivery under JSONResponse mode (requires client continuous listening); add client-side ElicitationHandler interface, WithElicitationHandler option, and NewStreamableHttpClientWithOpts; rewrite server elicitation.go to mirror mcp-go (SessionWithElicitation, RequestElicitation, ErrNoActiveSession/ErrElicitationNotSupported). U9 — Populate before-hook request objects (mcp.CallToolRequest with tool name + args, mcp.ListToolsRequest with cursor) from go-sdk params; fix latent translateUnknownToolError type assertion (CallToolParamsRaw). U2 — Document the preset backend session ID limitation (upstream-only: go-sdk has no SessionID field on StreamableClientTransport; honored via resume path only, not Initialize). 7 — Fix remaining stale doc comments (WithSessionIdManager, package doc, SessionWithTools). Tests: elicitation e2e (works with continuous listening, fails without), handler error return, ErrNoActiveSession/ErrElicitationNotSupported guards, before-hook tool name+args+cursor, unknown-tool error contains tool name. Refs #156
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
(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.
| // you want unanswered pings to terminate the session. Applies to the Streamable | ||
| // HTTP transport only; stdio and SSE pass 0 (no keep-alive), matching mcp-go. | ||
| func WithHeartbeatInterval(interval time.Duration) StreamableHTTPOption { | ||
| return func(s *StreamableHTTPServer) { s.heartbeat = interval } |
There was a problem hiding this comment.
CRITICAL — this evicts healthy sessions; vMCP will lose every JSON-only client session ~30s after creation. Failure scenario: go-sdk's KeepAlive (shared.go:593-637) sends a ping request each interval and session.Close()s on failure. This handler forces JSONResponse: true, so a server→client request outside an in-flight POST routes to the standalone SSE stream (go-sdk streamable.go:1410-1412); a client that never opened a GET stream — the shim client's own default (DisableStandaloneSSE: true) — has that stream unconnected, the write is rejected immediately ("stream not connected"), the ping errors, and the session is closed on the first tick. toolhive's vMCP sets WithHeartbeatInterval(30s) unconditionally (pkg/vmcp/server/server.go:505), so after #5729 bumps to this release, every session from a JSON-only client dies in ~30s. mcp-go's heartbeat was passive: pings written only to an existing GET stream, never awaited, never terminating (mcp-go streamable_http.go:827-856). TestHeartbeat_WiredToKeepAlive enshrines the failure as expected — its "unresponsive" client is just a normal JSON-only client that could never receive the ping; there is no test that an idle healthy client survives the heartbeat, because under this wiring it can't. Fix options: passive shim-side keepalive (the issue's original SSE-comment design), ping only sessions with a connected standalone stream, or gate the toolhive#5729 merge on removing WithHeartbeatInterval from vMCP. Do not ship both as-is.
There was a problem hiding this comment.
Addressed in d898410. Summary:
1 (CRITICAL — heartbeat eviction): Reverted the ServerOptions.KeepAlive wiring. WithHeartbeatInterval is now a documented no-op — go-sdk's active ping kills JSON-only sessions (no GET stream → ping rejected → session.Close() on first tick), incompatible with JSONResponse mode. Passive SSE-comment design is the correct approach and is left as TODO(issue #156). Replaced TestHeartbeat_WiredToKeepAlive with TestHeartbeat_NoOpDoesNotEvictHealthySession — an idle healthy client survives past the heartbeat interval.
2 (HIGH — global schema panic poisons sync.Once): buildServer now recovers global AddTool panics via addGlobalTool and returns the panic as buildErr, so sync.Once is properly poisoned with an error (not nil-error-nil-handler). Also improved normalizeObjectSchema: type-omitted object schemas with properties/required get type:"object" added (callable, matching mcp-go); truly non-object schemas pass through and are caught by the recover. Tests: TestGlobalTool_NonObjectSchema_DoesNotPoisonServer (first and second request get clean 500, not nil panic), TestGlobalTool_TypeOmittedObjectSchema_Callable.
3 (MEDIUM — transient Validate → split-brain): forgetSession/deleteRehydrated now called only on genuine termination (isTerminated == true). On a transient Validate error, the request is rejected (503) WITHOUT dropping local state — the go-sdk session stays alive, client retries, Redis recovers. Test: TestLocalSession_TransientValidateError_DoesNotForgetSession (flaky manager flakes once → 503, then session still local and served on retry).
4 (MEDIUM — empty-body 4xx): captureErrorBody now records h.status = resp.StatusCode unconditionally (before reading the body), regardless of body length. Empty-body 401/404 are now classified by status. Added empty-body cases to TestMapCallError_Transport401 and TestMapConnectError_4xxOnInitialize_LegacySSE.
5 (LOW — unbounded session growth): Documented the gap on sessions/localSessions — reaping closed sessions is needed and tracked separately.
6 (LOW — bridge misses notification POSTs): Documented at defer clearPendingRequestContext — the bridge covers request/response calls (which block in servePOST) but not notification-only POSTs (immediate 202 → deferred clear can race dispatch), pending go-sdk per-request context propagation.
Review summary (zero functional loss): Acknowledged — notifications/tools/list_changed and notifications/message require toolhive-side companions to #5729 (WithLogging() + feature re-sync in the bridge, WithPageSize in vMCP). The shim now provides the surface (handlers, SetLoggingLevel, WithPageSize); the bridge-side wiring is toolhive's to consume.
Also fixed a pre-existing data race exposed by the timing changes: clientSession.owner was a plain *MCPServer raced between SetSessionTools (before-hook goroutine) and registerAndSync (initialized callback). Converted to atomic.Pointer[MCPServer], matching the existing boundServer/goSession pattern. 10× -race clean.
| // (MCPServer.addSessionTool, used by syncSessionTools) recovers and skips the | ||
| // offending tool so one bad overlay schema cannot crash a live session; the | ||
| // global path (buildServer) lets the panic surface as a construction-time error | ||
| // (a 500 at handler build). |
There was a problem hiding this comment.
HIGH — a single global tool with a non-object schema permanently bricks the server (empirical repro). This comment's claim that the global path surfaces "a construction-time error (a 500 at handler build)" is wrong: buildServer does not recover the AddTool panic, and StreamableHTTPServer.build() runs it inside sync.Once.Do (transports.go:217-247). Once.Do marks itself done even when f panics, so after the first request panics, buildErr is nil and handler is nil — every subsequent request nil-panics at s.handler.ServeHTTP (transports.go:326). Reproduced: register one global tool via NewToolWithRawSchema with {"$ref":...} → request 1 = connection EOF, request 2+ = http: panic serving ... nil pointer dereference, forever. ServeStdio (transports.go:62-68) crashes the bridge process at startup instead. go-sdk panics for any top-level type ≠ "object" (server.go:247-259), which includes the spec-loose but common "type": ["object", "null"] and type-omitted object schemas — under mcp-go these were served verbatim and fully callable; under v0.0.27 they were rewritten but callable; now they brick the server (global) or silently drop the tool (per-session), both worse. Minimum fix: recover in buildServer and return the panic as an error into buildErr; better: normalize non-object schemas while preserving properties/required instead of dropping. Either way, add a global-path test — only the per-session recovery is covered.
There was a problem hiding this comment.
Addressed in d898410 — see reply on the first comment for full details. Global AddTool panics are now recovered in buildServer via addGlobalTool, returning the panic as buildErr so sync.Once is properly poisoned. normalizeObjectSchema also improved: type-omitted object schemas with properties/required get type:"object" added (callable). Tests: TestGlobalTool_NonObjectSchema_DoesNotPoisonServer, TestGlobalTool_TypeOmittedObjectSchema_Callable.
| if sid := r.Header.Get("Mcp-Session-Id"); sid != "" && s.sessionIDMgr != nil && s.mcp.isLocalSession(sid) { | ||
| isTerminated, err := s.sessionIDMgr.Validate(sid) | ||
| if err != nil { | ||
| s.mcp.forgetSession(sid) |
There was a problem hiding this comment.
MEDIUM — a transient Validate error permanently downgrades a local session into split-brain. Failure scenario: Redis blips once → Validate returns err → forgetSession(sid) drops the clientSession (per-session tool overlay included) and the local marker, but the go-sdk StreamableHTTPHandler still holds the live session. The client's retry now takes the !isLocalSession branch into serveRehydrated, which builds a second go-sdk session with the same ID — two live sessions, one ID, one pod; the client's standalone GET stream stays attached to the old one, so notifications/elicitations go nowhere. mcp-go rejected the request and kept state. Only forget on isTerminated == true; on err, just reject the request. This path has no test.
There was a problem hiding this comment.
Addressed in d898410 — forgetSession/deleteRehydrated now called only on isTerminated == true. On transient Validate error, request is rejected (503) without dropping local state, preventing the split-brain. Test: TestLocalSession_TransientValidateError_DoesNotForgetSession.
| return mapTransportError(enrichWithResponseBody(ctx, err)) | ||
| err = enrichWithResponseBody(ctx, err) | ||
| h := capturedErr(ctx) | ||
| if h != nil && h.status >= 400 && h.status < 500 { |
There was a problem hiding this comment.
MEDIUM — status-driven classification silently disabled for empty-body 4xx. captureErrorBody (errorbody.go, not in this diff) records h.status only when the body is non-empty (if err == nil && len(data) > 0). A 4xx with an empty body — common for 401 and 404 — leaves h.status == 0, this branch never fires, and classification falls to the string fallback, which does not map a bare "400 Bad Request"/"404 Not Found" on initialize to ErrLegacySSEServer (mcp-go classified any 4xx from the status line alone). Every wave-4 test seeds a non-empty body, so the hole is untested. One-line fix: record h.status regardless of body length, and add an empty-body case to TestMapConnectError_4xxOnInitialize_LegacySSE.
There was a problem hiding this comment.
Addressed in d898410 — captureErrorBody now records h.status = resp.StatusCode unconditionally (before reading the body), regardless of body length. Empty-body 4xx are now classified by status. Added empty-body cases to TestMapCallError_Transport401 and TestMapConnectError_4xxOnInitialize_LegacySSE.
| @@ -128,46 +156,60 @@ type MCPServer struct { | |||
| // in registerAndSync (which only fires on this instance's initialize path). | |||
| localSessions sync.Map // sessionID -> struct{} | |||
There was a problem hiding this comment.
LOW — unbounded growth: entries here (and in sessions) are never removed when go-sdk closes a session. Only DELETE and the Validate-failure path call forgetSession; sessions closed by keepalive eviction or a vanished client leak their localSessions + sessions entries for the life of the process, and SendNotificationToAllClients keeps iterating the corpses. On a long-lived vMCP pod this grows without bound — and the heartbeat issue above turns it into mass leakage. sessions predates this PR; localSessions is new here. Worth a cleanup hook (e.g. reap on Validate 404 from the go-sdk handler, or periodically drop entries whose goSession is closed).
There was a problem hiding this comment.
Addressed in d898410 — documented the gap on sessions/localSessions (reaping closed sessions needed, tracked separately). Also fixed a pre-existing data race on clientSession.owner (converted to atomic.Pointer[MCPServer], matching the boundServer/goSession pattern) that was exposed by the timing changes.
| nonce := crand.Text() | ||
| s.mcp.setPendingRequestContext(r.Context(), nonce) | ||
| r.Header.Set(reqNonceHeader, nonce) | ||
| defer s.mcp.clearPendingRequestContext(nonce) |
There was a problem hiding this comment.
LOW — the bridge can miss notification POSTs. A notification-only POST gets its 202 immediately, so this deferred clear can run before the session goroutine dispatches the message; the middleware then finds no entry and falls back to the initialize-frozen session context — per-request values (identity, audit, telemetry) are silently absent for notification handling. No cross-request bleed (calls block in servePOST until the response, so their entries are always present), and nothing depends on it today (registerAndSync runs on the initialize call, not on notifications/initialized), but worth a sentence in the pendingReqCtx doc so the next reader doesn't assume the bridge covers notifications.
There was a problem hiding this comment.
Addressed in d898410 — documented at the defer clearPendingRequestContext site that the bridge covers request/response calls (which block in servePOST until the response) but not notification-only POSTs (immediate 202 → deferred clear can race dispatch), pending go-sdk per-request context propagation.
CRITICAL — Revert WithHeartbeatInterval → ServerOptions.KeepAlive wiring: go-sdk's KeepAlive sends an active ping request that closes sessions without a connected standalone SSE stream, incompatible with JSONResponse mode + JSON-only clients. WithHeartbeatInterval is now a documented no-op (passive SSE-comment design is TODO). HIGH — Recover global AddTool panics in buildServer so a non-object schema tool doesn't poison sync.Once (was nil-panic on every subsequent request). Improve normalizeObjectSchema: type-omitted object schemas with properties/required get type:object added (callable, matching mcp-go); truly non-object schemas pass through and are caught by recover. MEDIUM — Only forgetSession on genuine termination (isTerminated), not on transient Validate errors (Redis blip), preventing split-brain where a retry builds a second go-sdk session with the same ID. MEDIUM — captureErrorBody now records h.status unconditionally (regardless of body length) so empty-body 4xx (common 401/404) are classified by status, not lost to the string fallback. LOW — Document unbounded session growth gap and bridge notification POST limitation. Fix pre-existing data race: clientSession.owner converted to atomic.Pointer[MCPServer] (matching boundServer/goSession pattern) — SetSessionTools and registerAndSync were racing on the plain pointer. Tests: healthy session survives heartbeat interval (no eviction), non-object global tool doesn't poison server, type-omitted schema callable, transient Validate error doesn't forget session, empty-body 4xx classification, 10x -race clean. Refs #156
1 participant
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.
b22b8e1ServerOptions.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 panicsa3020893c7ab69WithHeartbeatInterval→ServerOptions.KeepAlive; 5WithDisableLocalhostProtection(bool); 6WithPageSize(n int); 3/U5 per-requestValidatefor local sessions19c482bErrLegacySSEServer884cfd3ElicitationHandler+ serverRequestElicitation); U9 populate before-hook request objects (tool name + args + cursor); U2 document preset session ID limitation (upstream-only); 7 fix remaining stale doc commentsWhat landed (15 items)
Shim fixes (all 15 work items from the issue + 2 audit comments)
WithToolCapabilities/WithResourceCapabilities/WithPromptCapabilitiesmapped to go-sdkServerOptions.Capabilities; vMCP advertises tools/resources/prompts at initialize even with zero pre-registered features.pendingReqCtxrekeyed from session-ID to a per-POSTcrypto/randnonce (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.normalizeObjectSchemanarrows to nil/empty-only;$ref/oneOf/boolean pass through verbatim; per-sessionAddToolpanics recovered and skipped.ProgressNotificationHandler+LoggingMessageHandlerwired in the client;dispatchcarries params viatoNotificationParams(JSON round-trip,_metasplit).tools/list_changedonSetSessionTools(no production code needed beyond U1); corrected stale doc comments.WithHeartbeatIntervalwired toServerOptions.KeepAlive(was a silent no-op).WithDisableLocalhostProtection(bool)exposed (default: protection ON).WithPageSize(n int)exposed (default 0 → go-sdk's 1000).SessionIdManager.Validatecalled for local sessions too; restores per-request liveness + sliding TTL.errors.As(*jsonrpc.Error); false-positive OAuth refresh on tool-error text eliminated; any-4xx-on-initialize →ErrLegacySSEServerrestored.ElicitationHandler/WithElicitationHandler/NewStreamableHttpClientWithOpts; serverSessionWithElicitation/RequestElicitation/ErrNoActiveSession/ErrElicitationNotSupported); JSONResponse mode documented (requires client continuous listening).mcp.CallToolRequestwith name + args,mcp.ListToolsRequestwith cursor); latenttranslateUnknownToolErrortype-assertion bug fixed (*CallToolParams→*CallToolParamsRaw).SessionIDfield onStreamableClientTransport; honored via resume path only).WithSessionIdManager, package doc,SessionWithTools, capability-flag comment,OnNotification,notifications.go,normalizeObjectSchema).Upstream asks (documented, not implemented — tracked in the issue)
ServerSession.StreamableClientTransport(blocks U2).Validate/Terminatehooks.Test coverage
Each item has e2e and/or unit tests that would fail without the fix:
TestAdvertisesToolsCapability_WithNoGlobalTools)-race(verified to fail without the bridge)list_changede2e onSetSessionToolstoNotificationParamsunit table (meta split, error paths, nil guards)nextCursortranslateUnknownToolError)Verification
task lint— 0 issues (golangci-lint + go vet)task test— all packages pass with-racetask license-check— cleanReview 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:
toNotificationParamsunit tests + nil-param guards + broadcast delivery assertiontranslateUnknownToolErrorregression test + elicitation guard branches + doc fixesRefs #156