Serve MCP 2026-07-28 Modern stateless requests through vMCP#5953
Conversation
JAORMX
left a comment
There was a problem hiding this comment.
Panel review (reviewed at 0016ed50)
I ran this through a multi-lens review — MCP wire-format/JSON-RPC conformance, authorization/security, Go correctness/concurrency, and vMCP architectural fit + test coverage. Overall this is a strong, unusually well-documented change: the re-homed authz gate is substantially correct, the concurrency story holds (the stateless dispatcher genuinely has no shared mutable state, so no keyedMutex needed), response-writer discipline is clean (marshal-before-write, write-once, return after every error write), and core.Discover's single-fan-out reuse of the existing admission filters is a nice consolidation. No blocking correctness or security defects found — in particular, no authorization bypass: every backend-reaching verb is gated, and a dispatch-time ErrAuthorizationFailed reliably maps to 403 + deny message (audited as denied, tested first in writeModernDispatchError).
A few should-fixes below, mostly around strict spec conformance and one test gap. None are merge-blockers on their own, but I'd like to see #1 and #2 addressed (or explicitly deferred with a tracking issue) before I approve.
⚠️ CI: the one red check —TestStandaloneSSE_ListChangedRefiltersThroughExistingMiddlewareinpkg/transport/proxy/streamable— is in a package this PR does not touch (all changes arepkg/authz+pkg/vmcp). Looks unrelated/flaky; worth a re-run to confirm it's green before merge.
Should-fix
1. Spec MUST: a Modern POST that omits the MCP-Protocol-Version header is accepted instead of rejected. (classification.go:38-55, pkg/mcp/revision.go:283-299)
Per the draft Streamable HTTP spec, every POST to the MCP endpoint MUST carry MCP-Protocol-Version; a missing header is a validation failure that MUST return 400 + -32020. Today a body with a reserved _meta key + valid protocolVersion + clientCapabilities but no header classifies (RevisionModern, nil) and dispatches → 200. Relatedly, a reserved-key-only body with no version + no header returns -32602 where the spec wants -32020 (MissingModernMetadataError). Your own TODO at revision.go:292-297 calls this out exactly — and it's now resolvable, since classifyingHandler does have HTTP header context and can enforce header presence for Modern before dispatch.
2. No assembled-server test of the authz-denial path — the very reason the gate is re-homed. (session_management_realbackend_integration_test.go:84-97)
newRealTestHandler builds the server with no Authz config, so authzGateEnabled is false in every modern_realbackend_integration_test.go case. The gate is thoroughly unit-tested (TestDispatchModern_AuthzGate covers pre-dispatch 403, TOCTOU 403, fail-open, non-authz -32603, gate-off), but the middleware-ordering assumption you rely on — "audit is outer, so a dispatcher 403 audits as denied" — is never exercised end-to-end. One real-server test with authz enabled asserting a Cedar-denied Modern tools/call → 403 and a denied audit outcome would lock in the security-critical invariant.
3. -32603 returns err.Error() verbatim to the client. (modern_dispatch.go:125,130,141,152,163,192,426)
Two reviewers independently flagged this. The wrapped chains ("capability aggregation failed: %w", "routing tool %q: %w") can carry internal backend IDs / plumbing detail, which security.md says not to leak. I see your comment (modern_dispatch.go:415-420) arguing this is deliberate parity with the SDK/legacy path (conversion.ErrorToToolResult, serve_handlers.go). That's fair for the call/read/get branches — but could you confirm the legacy tools/list path exposed the same raw text? If not, the list-verb -32603 branches (125/130/141/152/163/192) are newly exposing it, and a generic message (with err logged server-side) would be safer there.
Discussion / judgment
4. Kill-switch removal & rollback story. The "zero Legacy impact" justification is correct but answers a different risk than the one #5910's flag mitigated. Legacy is genuinely untouched. The risk the flag covered is that Modern clients get a hand-rolled envelope mirroring go-sdk v1.7.0-pre.3 — a version this module does not import (it's on v1.6.1), so there's no compile-time check the shapes match and the envelope tests assert against hand-written expected JSON, not real SDK bytes. With the flag, disabling Modern let those requests fall through to the SDK's version downgrade — a strictly safer degraded mode than serving a possibly-wrong pre-release envelope, and it's a runtime lever rather than revert-and-redeploy. I'd lean toward restoring a default-off switch for a hand-rolled pre-release protocol, or at minimum getting explicit maintainer sign-off recorded here that unconditional serving is accepted until the #5837 conformance harness lands.
5. PR size / split. ~3.2k LOC / 20 files is well over the repo norm. The proposed (a) envelope+dispatcher+wiring+tests / (b) discover+authz+completion split is sensible and matches your commit boundaries — I'd take it for reviewability. (Note: PR(a) would need the server/discover + completion/complete switch cases stubbed to -32601 so it doesn't advertise methods it can't serve.)
Nits (non-blocking)
- PR description wording: "fail-open on infra errors" overstates the exposure. Cedar runs in-process and fails closed (→ 403); the only fail-open trigger is a non-authz plumbing error from
CheckToolCall, andCallToolindependently re-aggregates and fails-32603before reaching a backend — so fail-open never yields an unauthorized backend call. Worth rewording so a future reader doesn't "fix" it into a real hole. Also: onlytools/callcan fail open (CheckResourceRead/CheckPromptGetdon't aggregate), yetgateDenied's comment reads as if all three do. pingomitsresultType(modern_dispatch.go:99-107). Draft schema marksResult.resultTypea MUST for a 2026-07-28 server; a strict conformance suite would flag the bare{}. Your SDK-parity rationale is documented and real clients treat absent as"complete", so likely fine — flagging so it's a conscious call.- Gate coverage is duplicated across
call_gate.goandmodern_dispatch.go(two method→gate switches).core.Check*guarantees the decision can't diverge, but coverage could (a verb gated on one path, forgotten on the other). A single method→gate table driving both would remove the drift risk — good follow-up. - Dead nil-guards:
newModernCallToolResult/ReadResource/GetPromptguardresult == nil, but callers derefresult.BackendIDfirst (modern_dispatch.go:227,250,282), so a nil would panic upstream — the guards are unreachable. Either drop them or make the deref defensive. - Explicit
"id": nulldecodes toparsed.ID == nil→ treated as a notification (202). Spec forbids null id, so low-risk, but worth confirming the parser distinguishes absent from explicit-null.
Nice work overall — the doc comments made this genuinely reviewable at this size. I'll keep watching and approve once the header-validation (#1) and denial-path test (#2) are sorted (or explicitly punted with issues).
🤖 Panel review assisted by Claude Code
|
@jhrozek I went ahead and pushed two commits addressing the two should-fixes I flagged, to save you a round-trip — please review/squash/drop as you see fit.
I did not re-code the minor S2 nuance (reserved-key-only + no version + no header still returns
Verified locally: On the discussion items (verbatim 🤖 Pushed with Claude Code |
|
CI note on
🤖 via Claude Code |
Per maintainer review (#5953): serving Modern (2026-07-28) requests unconditionally is risky while the wire envelope is hand-rolled against go-sdk v1.7.0-pre.3 — a version this module does not import, so the shapes have no compile-time check and are validated only against hand-written test JSON, not real SDK bytes. Restore a startup kill-switch so an operator can fall back to the SDK path (a safe version-downgrade) via config rather than revert-and-redeploy, until Modern is conformance-validated. classifyingHandler dispatches a well-formed Modern request to the core only when Config.ModernDispatchEnabled is set (default false); otherwise it falls through to the SDK path. Fed once at the composition root from TOOLHIVE_VMCP_MODERN_STATELESS (unset/invalid -> off, with a warning). Legacy and malformed-Modern behavior is unchanged (the gate sits after classification). The Modern dispatch/integration tests enable the switch; a KillSwitchOff case asserts fall-through. This kill-switch is temporary and tracked for removal by #5959 (once the #5837 conformance harness lands or mcpcompat adopts go-sdk v1.7). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the thorough pass. All points addressed:
CI: the one red check is the |
Per maintainer review (#5953): serving Modern (2026-07-28) requests unconditionally is risky while the wire envelope is hand-rolled against go-sdk v1.7.0-pre.3 — a version this module does not import, so the shapes have no compile-time check and are validated only against hand-written test JSON, not real SDK bytes. Restore a startup kill-switch so an operator can fall back to the SDK path (a safe version-downgrade) via config rather than revert-and-redeploy, until Modern is conformance-validated. classifyingHandler dispatches a well-formed Modern request to the core only when Config.ModernDispatchEnabled is set (default false); otherwise it falls through to the SDK path. Fed once at the composition root from TOOLHIVE_VMCP_MODERN_STATELESS (unset/invalid -> off, with a warning). Legacy and malformed-Modern behavior is unchanged (the gate sits after classification). The Modern dispatch/integration tests enable the switch; a KillSwitchOff case asserts fall-through. This kill-switch is temporary and tracked for removal by #5959 (once the #5837 conformance harness lands or mcpcompat adopts go-sdk v1.7). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
9098754 to
7cce772
Compare
Rebased onto latest
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5953 +/- ##
==========================================
+ Coverage 71.88% 71.96% +0.08%
==========================================
Files 715 717 +2
Lines 73501 73899 +398
==========================================
+ Hits 52835 53185 +350
- Misses 16883 16914 +31
- Partials 3783 3800 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Two core additions supporting the Modern (2026-07-28) dispatch path: - core.VMCP.Discover computes server/discover's capability flags from one aggregatedView, deriving each flag through the same admission filters the List* verbs use, so the flags stay post-admission-filtered per identity and cannot drift. Mirrors the ListBackends(filterUnauthorized) precedent. - ToolCallResult/ResourceReadResult/PromptGetResult gain a BackendID, set by the core from the routed target (empty for composite tools). It is audit-only (json:"-", never serialized) and lets the transport layer label which backend served a call, matching the Serve path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Serve Modern (2026-07-28) stateless requests by bypassing the SDK Serve/ session layer and dispatching straight to the already-stateless vMCP core. classifyingHandler routes a well-formed Modern request to a hand-rolled dispatcher; Legacy and malformed-Modern requests behave exactly as before. The dispatcher hand-rolls the Modern wire envelope (resultType, _meta serverInfo, Cacheable with cacheScope "private" for identity-scoped results) because the imported go-sdk v1.6.1 has no Modern shapes. It serves tools/resources/prompts list + call/read/get, server/discover, ping and completion/complete; notifications -> 202, unknown -> 404/-32601, malformed arguments (and a missing completion argument name) -> 400/-32602. Because it bypasses the SDK server, it re-homes that server's pre-dispatch authorization gate: Check* before dispatch and a dispatch-time ErrAuthorizationFailed both map to HTTP 403 + the matching DenyMessage (so a denial audits as "denied"), fail-open on infra errors. The incoming request context is passed through unmodified so forwarded-header backend auth works. Successful tools/call, resources/read and prompts/get label the audit backend_name (success path only; see comment). server/discover returns post-admission capability flags via core.Discover (no descriptor arrays). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Now that server/discover is served, allow-list it in pkg/authz.Middleware: DiscoverResult carries the same Capabilities/Instructions shape InitializeResult does, and initialize is already always-allowed on this path, so discover adds no new exposure class. This map governs only the single- server / proxy-runner path; vMCP's Modern dispatcher does not consult it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drive real Modern (2026-07-28) JSON-RPC-over-HTTP through the fully assembled server into a real core aggregating a real backend, asserting the wire bytes a client sees. A hand-rolled raw HTTP client sends Modern requests (go-sdk v1.7 cannot be imported without an MVS bump). Covers tools/call round-trip with no Mcp-Session-Id, tools/list and server/discover through the admission seam, completion/complete, ping, notification 202, unknown 404/-32601, malformed arguments 400/-32602. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A request that classifies Modern (2026-07-28) via its reserved io.modelcontextprotocol/* _meta keys but omits the mandatory MCP-Protocol-Version HTTP header was dispatched and answered 200. The draft Streamable HTTP "Server Validation" rules make a missing required standard header a -32020 rejection, so a header-less Modern POST must be refused, not served. ClassifyRevision is transport-agnostic (it also serves header-less stdio) and deliberately defers the header-presence rule to the HTTP layer, per its own TODO. Enforce it in classifyingHandler: once a request classifies Modern with no error, an empty MCP-Protocol-Version header is rejected as -32020 before dispatch. This touches only the otherwise-accepted path, so the existing -32602/-32022/-32020 rejection codes are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79
The Modern stateless dispatcher bypasses the SDK server and its CallGate, so it re-homes the pre-dispatch authorization gate itself. That gate was covered only by dispatchModern unit tests; the assembled-server denial path (audit -> parsing -> classification -> dispatchModern) had no coverage, unlike the Legacy path. Add the Modern counterpart of TestIntegration_CedarAuthzDenialIsAudited: a policy-denied Modern tools/call must return HTTP 403 + JSON-RPC 403 and be audited with outcome "denied", proving the audit middleware wraps the re-homed gate exactly as it wraps the SDK CallGate. Drop the now-constant eventType parameter from the shared audit-log helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79
- List/discover -32603 responses now return a generic "internal error" to the client and log the full error server-side, instead of echoing wrapped aggregation/routing detail (backend IDs, upstream addressing) — the Legacy path never surfaces a list error to the client at all, and security.md forbids leaking internal plumbing. tools/call/resources/read/prompts/get and completion keep err.Error() (documented SDK/Legacy parity). - Drop the unreachable result==nil guards in the call/read/get envelope builders (callers deref result.BackendID first, so a nil panics upstream); newModernComplete keeps its guard, which is live. - Clarify gateDenied's comment: only tools/call's CheckToolCall re-aggregates and can fail open on a non-authz error; CheckResourceRead/CheckPromptGet always classify as ErrAuthorizationFailed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per maintainer review (#5953): serving Modern (2026-07-28) requests unconditionally is risky while the wire envelope is hand-rolled against go-sdk v1.7.0-pre.3 — a version this module does not import, so the shapes have no compile-time check and are validated only against hand-written test JSON, not real SDK bytes. Restore a startup kill-switch so an operator can fall back to the SDK path (a safe version-downgrade) via config rather than revert-and-redeploy, until Modern is conformance-validated. classifyingHandler dispatches a well-formed Modern request to the core only when Config.ModernDispatchEnabled is set (default false); otherwise it falls through to the SDK path. Fed once at the composition root from TOOLHIVE_VMCP_MODERN_STATELESS (unset/invalid -> off, with a warning). Legacy and malformed-Modern behavior is unchanged (the gate sits after classification). The Modern dispatch/integration tests enable the switch; a KillSwitchOff case asserts fall-through. This kill-switch is temporary and tracked for removal by #5959 (once the #5837 conformance harness lands or mcpcompat adopts go-sdk v1.7). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The rebased branch tripped the codespell CI check on the illustrative "ture" typo in the kill-switch comment. Reword it to avoid the flagged token rather than add "ture" to the repo-wide ignore list, which would suppress a genuinely common misspelling everywhere. Also drop the ad-hoc comment markers that are not a ToolHive convention (keeping the explanatory prose) and correct a stale comment that referenced a nextCursor envelope field that does not exist. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79
7cce772 to
efb7421
Compare
|
Rebased onto latest |
Rebasing onto origin/main picked up #5953 (Serve MCP 2026-07-28 Modern stateless requests through vMCP), which intentionally flipped server/discover in pkg/authz/middleware.go from not-allow-listed (default-deny, 403) to always-allowed. The dual-era authz guard, which asserted a 403, correctly failed against the new behavior. Update it to the new contract: server/discover is allowed under authz (200), which is a positive result -- a real Modern client's discover no longer gets 403'd into a Legacy downgrade, so Modern-through-authz works. The policy-permitted echo call is still asserted. Per #5953's own comment, discover is not covered by the response-filter and passes through unfiltered; that is a documented, deliberate tradeoff in that PR, not something this test enforces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rebasing onto origin/main picked up #5953 (Serve MCP 2026-07-28 Modern stateless requests through vMCP), which intentionally flipped server/discover in pkg/authz/middleware.go from not-allow-listed (default-deny, 403) to always-allowed. The dual-era authz guard, which asserted a 403, correctly failed against the new behavior. Update it to the new contract: server/discover is allowed under authz (200), which is a positive result -- a real Modern client's discover no longer gets 403'd into a Legacy downgrade, so Modern-through-authz works. The policy-permitted echo call is still asserted. Per #5953's own comment, discover is not covered by the response-filter and passes through unfiltered; that is a documented, deliberate tradeoff in that PR, not something this test enforces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rebasing onto origin/main picked up #5953 (Serve MCP 2026-07-28 Modern stateless requests through vMCP), which intentionally flipped server/discover in pkg/authz/middleware.go from not-allow-listed (default-deny, 403) to always-allowed. The dual-era authz guard, which asserted a 403, correctly failed against the new behavior. Update it to the new contract: server/discover is allowed under authz (200), which is a positive result -- a real Modern client's discover no longer gets 403'd into a Legacy downgrade, so Modern-through-authz works. The policy-permitted echo call is still asserted. Per #5953's own comment, discover is not covered by the response-filter and passes through unfiltered; that is a documented, deliberate tradeoff in that PR, not something this test enforces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
ToolHive is adding support for the MCP 2026-07-28 ("Modern") stateless revision. The transport proxies already classify-and-forward Modern requests (#5884); this change makes the vMCP server serve them itself — bypassing the SDK Serve/session layer and dispatching straight to the already-stateless vMCP core, so a handshake-less client can talk to a virtual MCP server.
resultType,_metaserverInfo,CacheablewithcacheScope:"private"for identity-scoped results) — the imported go-sdk v1.6.1 has no Modern shapes, so the envelope is reproduced against v1.7.0-pre.3 and checked against its conformance fixtures.classifyingHandlerroutes well-formed Modern requests to the hand-rolled dispatcher; Legacy and malformed-Modern behavior is unchanged.list+call/read/get,server/discover(post-admission capability flags viacore.Discover),ping, andcompletion/complete; notifications → 202, unimplemented → 404/-32601, malformed params → 400/-32602.Check*before dispatch and a dispatch-timeErrAuthorizationFailedboth map to HTTP 403 + the matching deny message (so a denial audits asdenied, not a 200 tool-error or -32603). Cedar itself fails closed (→ 403); the only "fail-open" is a non-authorization plumbing error from the pre-dispatchCheckToolCall(the sole gated verb that re-aggregates), which admits the request — butCallToolindependently re-derives the decision before contacting any backend, so this never yields an unauthorized backend call. Successful backend calls label the auditbackend_name.server/discoveris allow-listed on the single-server authz path (parity withinitialize).Closes #5910
Type of change
Test plan
task test)task lint-fix)Integration tests (
modern_realbackend_integration_test.go) drive real Modern JSON-RPC-over-HTTP through the fully-assembled server into a real core + real backend, asserting the wire bytes: noMcp-Session-Idon Modern, envelope shapes, 202/404/400,server/discoverflags through the admission seam, denial → 403. go-sdk v1.7 could not be imported (it would force an MVS bump of the whole module), so a hand-rolled raw HTTP client is used; independent-client + upstream-conformance validation for Modern is deferred to the #5837 harness.API Compatibility
v1beta1API (no operator/CRD surface is touched).Changes
Four commits:
server/discover(oneaggregatedView, same admission filters as theList*verbs), plus an audit-onlyBackendID(json:"-") on the call/read/get results.classifyingHandlerwiring, and discover/completion/ping serving.pkg/authz.Does this introduce a user-facing change?
Yes — a vMCP server now serves MCP 2026-07-28 ("Modern") stateless requests directly (no
initializehandshake), in addition to the existing Legacy (2025-11-25) path. Modern dispatch is unconditional for well-formed Modern requests.Special notes for reviewers
server/discover+ authz allow-list +completion/complete. Opened as a single PR per request; happy to split for review if preferred.ClassifyRevisionroutes every non-Modern request to the unchanged SDK path before dispatch, and Modern classification requires an exactMCP-Protocol-Version: 2026-07-28header or a reservedio.modelcontextprotocol/*_metakey that no conformant Legacy client emits (initializeis force-Legacy). Routing tests cover this.resultType/serverInfo are set by unexported go-sdk functions inside the dispatch path this bypasses, so a future v1.7 bump will not simply delete it.backend_nameis labeled on the success path only. On a backend-call failure it is omitted (Legacy includes it because it pre-resolves the backend at session registration) — accepted, because the stateless dispatcher cannot pre-route (the backend is resolved inside the core).server/discoverruns one backend fan-out per call (uncached on the stateless path); a cross-request per-identity capability cache is tracked separately (Support MCP caching metadata (ttlMs/cacheScope) in vMCP #5761).🤖 Generated with Claude Code