Replace vMCP Modern kill-switch with a capability gate - #6033
Conversation
github-actions
Bot
added
size/M
CI confirms the blocker, and corrects one claim in the description above
Zero specs passed. The
The other six are This is not a flake, and the shape proves itWorth stating explicitly, because this repo has had several genuine flakes today (#6026, #6034, a
Correction to the descriptionThe description says the dual-era e2e suite "would not have caught this" because So the coverage gap is narrower than stated — the Everything else in the description stands, including the baseline control showing this is pre-existing rather than introduced here. |
The server->client forwarding integration tests (mid-call elicitation, sampling, progress, logging) exercise a surface that exists only on a Legacy (2025-11-25) session: the 2026-07-28 revision removed server-initiated requests, replacing them with client-polled multi-round retrieval (SEP-2322), and SEP-2577 deprecates sampling and logging outright. Today these tests land on Legacy only incidentally, because the Modern dispatch kill-switch (#5959) rejects the go-sdk client's Modern-first probe; once the switch is removed (#6033) the client negotiates Modern and all of them fail at connect. Pin the downstream test clients to Legacy explicitly via a transport-level RoundTripper that answers server/discover with the same -32022 rejection the kill-switch produces (mcpcompat cannot pin a protocol version, #5911), and pin the Modern-client contract for the same eliciting backend in a new integration test: an explicit JSON-RPC error naming the refused request, never a hang, a fabricated success, or an input_required envelope. Document the client-edge limitation and the intended future MRTR shape (#5743) in the vMCP architecture doc. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam
dispatchModern's four list verbs returned the full aggregated set and never emitted nextCursor, so a client had no way to page and vMCP silently ignored any cursor it was sent. On the Legacy path the SDK's session-scoped feature store does this split; the Modern dispatcher bypasses the SDK, so it has to do it itself. Modern has no sessions, so a cursor may not denote server-held iteration state. The spec makes cursors opaque to clients -- they MUST NOT parse, modify, or persist them -- which is exactly what lets the server encode position INTO the token instead of remembering it. So the cursor carries the unique key of the last item delivered and the next page is the items sorted strictly above it: keyset pagination, stateless, and the same scheme go-sdk's own server uses. Aggregation across backends needs no per-backend positions. core.List* already returns one flat, conflict-resolved set whose keys are unique across backends, so the cursor encodes a position in the aggregated ordering and adding or removing a backend cannot invalidate it. Page size matches the SDK default (1000) so pages line up across revisions, and an undecodable or wrong-verb cursor is rejected as -32602 rather than being reinterpreted into a plausible but wrong page. The upstream cursor-following in pkg/vmcp/client and the session backend (#5851) is the opposite direction -- vMCP as client -- and is untouched. Refs #5959, #6033
The server->client forwarding integration tests (mid-call elicitation, sampling, progress, logging) exercise a surface that exists only on a Legacy (2025-11-25) session: the 2026-07-28 revision removed server-initiated requests, replacing them with client-polled multi-round retrieval (SEP-2322), and SEP-2577 deprecates sampling and logging outright. Today these tests land on Legacy only incidentally, because the Modern dispatch kill-switch (#5959) rejects the go-sdk client's Modern-first probe; once the switch is removed (#6033) the client negotiates Modern and all of them fail at connect. Pin the downstream test clients to Legacy explicitly via a transport-level RoundTripper that answers server/discover with the same -32022 rejection the kill-switch produces (mcpcompat cannot pin a protocol version, #5911), and pin the Modern-client contract for the same eliciting backend in a new integration test: an explicit JSON-RPC error naming the refused request, never a hang, a fabricated success, or an input_required envelope. Document the client-edge limitation and the intended future MRTR shape (#5743) in the vMCP architecture doc. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam
MCP 2026-07-28 removed the standalone HTTP GET stream, so subscriptions/ listen is the revision's only server->client push channel. vMCP's Modern dispatcher had no case for it, which made vMCP unreachable for any go-sdk v1.7 client: Connect is Modern-first, and once server/discover succeeds it opens a listen stream whenever a list-changed handler is registered -- which mcpcompat's Initialize does unconditionally. The resulting -32601 failed Connect outright and tore the session down. The honored set is computed by intersecting the client's requested notification types against the capability advertisement server/discover publishes, now extracted as newModernCapabilities so the two answers cannot drift. Every push-related flag there is deliberately false, so the honored set is empty today and the acknowledgement says so explicitly. That is the spec's own mechanism for declaring unsupported notification types, not a silent no-op: nothing is ever reported as subscribed and then dropped. No push delivery exists yet (#5743); vMCP must start advertising a push capability before this stream can carry anything. Measured against the kill-switch removal in #6033: 20 regressions before, 6 after. The remaining 6 are unrelated to this channel -- five need server-initiated elicitation/sampling that 2026-07-28 removes outright, and one needs Modern list pagination. Refs #5959, #6033, #5743
dispatchModern's four list verbs returned the full aggregated set and never emitted nextCursor, so a client had no way to page and vMCP silently ignored any cursor it was sent. On the Legacy path the SDK's session-scoped feature store does this split; the Modern dispatcher bypasses the SDK, so it has to do it itself. Modern has no sessions, so a cursor may not denote server-held iteration state. The spec makes cursors opaque to clients -- they MUST NOT parse, modify, or persist them -- which is exactly what lets the server encode position INTO the token instead of remembering it. So the cursor carries the unique key of the last item delivered and the next page is the items sorted strictly above it: keyset pagination, stateless, and the same scheme go-sdk's own server uses. Aggregation across backends needs no per-backend positions. core.List* already returns one flat, conflict-resolved set whose keys are unique across backends, so the cursor encodes a position in the aggregated ordering and adding or removing a backend cannot invalidate it. Page size matches the SDK default (1000) so pages line up across revisions, and an undecodable or wrong-verb cursor is rejected as -32602 rather than being reinterpreted into a plausible but wrong page. The upstream cursor-following in pkg/vmcp/client and the session backend (#5851) is the opposite direction -- vMCP as client -- and is untouched. Refs #5959, #6033
Spec-conformance review found the pagination comment citing the 2025-11-25 page for a Modern implementation. The draft page is what 2026-07-28 ships and it differs in one substantive way: "Don't persist cursors across sessions" is replaced by "Don't make any determination based on cursor value other than whether a non-null value was provided (e.g. an empty string is a valid cursor and thus MUST NOT be treated as the end of results)". Cite the draft page as the rule and SEP-2567 as the reason for the deletion, by path rather than line since that SEP exists in two copies with different numbering. That amendment makes the existing omitempty on nextCursor load-bearing rather than incidental: emitting an empty cursor would have a conformant client re-request with it, which reads as "first page", looping forever on page one. Pinned explicitly now. Reject a cursor that is present but not a JSON string instead of silently reading it as "first page". The draft requires invalid cursors to yield -32602 and schema.ts classifies cursor values under invalid params, so a client with a serialization bug was getting a silent full-list restart in place of a diagnosable error. An empty-string cursor still means "first page", matching go-sdk, since vMCP never mints one. Stop attributing the subscriptionId key, request-id-as-identity, and acknowledgement ordering to the go-sdk: all three are normative in schema/draft/schema.ts. The comment as written invited a maintainer to weaken a MUST. Also restate the delivery-side MUST -- every notification on a listen stream carries the key -- at the #5743 hand-off, since no code here delivers notifications and it would otherwise be lost. Correct the ping comment, which claimed spec support that does not exist: ping was removed in 2026-07-28 and the bare {} also omits the MUSTed resultType. Behavior left unchanged as out of scope; the conformant answer is noted for whoever tightens it. Rename TestPaginateModern_DeliversEveryItemExactlyOnce to say "static set": the exactly-once guarantee holds only while the set does not change mid-walk, which is inherent to keyset paging and out of scope per SEP-2567. Refs #5959, #6033
Review found the paginator asserting a precondition that holds for one of its four callers. Only Tool.Name is unique -- ResolveToolConflicts keys a map -- while resources, resource templates and prompts are plain-appended across backends under "no conflict resolution for these yet". With a bare "resume strictly above lastKey" scan, a key shared at a page boundary made the scan skip every copy, so an item appeared on no page at all. Two honest backends both exposing file:///README.md was enough. A 1100-item corpus delivered 1097. The cursor now carries how many items sharing the last key were already sent, so a page can resume inside a run of equal keys. Items sharing a key are interchangeable for that purpose, which keeps the walk correct without a secondary sort key the generic signature cannot see. The comment claiming uniqueness is corrected rather than merged: it now states which callers can collide and that the paginator does not rely on uniqueness. The test corpus was the reason this shipped. Every case drew from a helper minting strictly unique keys, so the one precondition the code called load-bearing was the only thing untested. Collisions must also be placed by SORTED position -- a distinctively named duplicate key sorts away from the boundary and tests nothing, which a first attempt at the fixture did. Cap an inbound cursor before decoding it. A well-formed 6MB cursor decoded successfully at ~295ms of CPU on a verb that is not rate-limited, and a legitimate cursor is tens of bytes. Skip the capability fan-out in subscriptions/listen when nothing could be honored. core.Discover is un-rate-limited and, while no push capability is advertised, its result cannot change the answer -- so N no-op listens cost N fan-outs, now worse since list verbs page. Probing the ceiling first makes that O(1). This renders the fan-out error path unreachable, so its test is removed rather than left asserting a branch the code cannot enter. Assert the SSE framing. Replacing the blank-line event terminator with a single newline previously left the whole suite green -- including against a real client -- while a conformant parser would dispatch neither frame, so the ack-then-result contract was unverifiable. The helper now parses strictly, and the method name, acknowledgement name, subscriptionId key and jsonrpc field are asserted as literals; mutating any of them was silent. Add the negative capability test. The design rests on the advertisement honoring nothing, which was enforced by no test at all -- only by a runtime WARN that fires in production after the offending line ships. Now asserted across all sixteen presence combinations. Use http.ResponseController rather than a Flusher assertion: every ResponseWriter wrapper in the chain defines Flush unconditionally, so the assertion only established that a wrapper was present. Correct the comment that claimed otherwise, and the one claiming headers were committed on all four failure paths when three precede WriteHeader. Log the honored set by count, not by client-supplied URIs. Update docs/arch: subscriptions/listen is no longer unimplemented, serverStreamRegistry is the seam for delivery rather than for the handler, and both the pagination behaviour and the tools-only uniqueness property are now recorded. Refs #5959, #6033, #5743
Fold in the remaining review findings, including one the panel could not reach and two fixture blind spots. Set X-Accel-Buffering: no on the SSE response. The draft Streamable HTTP page states servers SHOULD send it so a reverse proxy does not accumulate events before forwarding, and this is the only SSE-emitting path in the codebase. The panel missed it because the draft moved transports.mdx into a directory, so every path it tried 404'd; the page is now at docs/specification/draft/basic/transports/streamable-http.mdx. Checking it also settles two open questions: the SSE event name is NOT mandated anywhere, and resumability is explicitly unsupported, which is why no id: field is emitted. Split frame marshalling out of the stream writer. Both marshals could previously fail on a path documented as "headers already committed" when nothing had been written at all, so the client got a bare 200 with an empty body for a request carrying an id. Building first means those failures are still reportable as -32603, and only a genuine mid-stream write failure is unreportable. Shuffle the corpus before paging in drainPages. Deleting the sort from paginateModern entirely used to fail exactly one subtest -- the only one whose input was unsorted -- so the deterministic total order the whole keyset scheme depends on was effectively unverified. The aggregator's real output is unordered anyway, so a shuffled input is also the realistic one. The same mutation now fails four test functions. Cover the remaining duplicate-key shapes, whose loss profiles differ sharply: a single duplicate at the boundary lost one item, one mid-page lost none, a run of 50 lost 50, and an all-identical corpus lost a whole page and returned an empty second page. The rule is that every item sharing the last key delivered on a page was dropped, so loss scaled with the run straddling the boundary -- and a duplicate away from a boundary was harmless, which is what made it easy to miss. Binary-search the resume point instead of scanning, making a full walk O(n log n) rather than O(n^2). The insertion point also gives the since-removed-key behaviour for free. Refs #5959, #6033, #5743
7af536e to
f99964e
Compare
|
Rebased onto current What the rebase needed
Why CI is red, and what makes it greenRemoving the kill-switch makes vMCP serve Modern unconditionally, which exposes gaps that exist on
Verified by building the combined tree and running both suites, in both merge orders — which produce byte-identical trees, so the landing order of #6050 and #6051 does not matter.
So the sequence is #6050 and #6051 in either order, then this. I'll rebase once more after they merge, since landing #6050 introduces the comment conflict noted above. Closes #5959 once it lands. |
github-actions
Bot
added
size/M
The server->client forwarding integration tests (mid-call elicitation, sampling, progress, logging) exercise a surface that exists only on a Legacy (2025-11-25) session: the 2026-07-28 revision removed server-initiated requests, replacing them with client-polled multi-round retrieval (SEP-2322), and SEP-2577 deprecates sampling and logging outright. Today these tests land on Legacy only incidentally, because the Modern dispatch kill-switch (#5959) rejects the go-sdk client's Modern-first probe; once the switch is removed (#6033) the client negotiates Modern and all of them fail at connect. Pin the downstream test clients to Legacy explicitly via a transport-level RoundTripper that answers server/discover with the same -32022 rejection the kill-switch produces (mcpcompat cannot pin a protocol version, #5911), and pin the Modern-client contract for the same eliciting backend in a new integration test: an explicit JSON-RPC error naming the refused request, never a hang, a fabricated success, or an input_required envelope. Document the client-edge limitation and the intended future MRTR shape (#5743) in the vMCP architecture doc. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam
MCP 2026-07-28 removed the standalone HTTP GET stream, so subscriptions/ listen is the revision's only server->client push channel. vMCP's Modern dispatcher had no case for it, which made vMCP unreachable for any go-sdk v1.7 client: Connect is Modern-first, and once server/discover succeeds it opens a listen stream whenever a list-changed handler is registered -- which mcpcompat's Initialize does unconditionally. The resulting -32601 failed Connect outright and tore the session down. The honored set is computed by intersecting the client's requested notification types against the capability advertisement server/discover publishes, now extracted as newModernCapabilities so the two answers cannot drift. Every push-related flag there is deliberately false, so the honored set is empty today and the acknowledgement says so explicitly. That is the spec's own mechanism for declaring unsupported notification types, not a silent no-op: nothing is ever reported as subscribed and then dropped. No push delivery exists yet (#5743); vMCP must start advertising a push capability before this stream can carry anything. Measured against the kill-switch removal in #6033: 20 regressions before, 6 after. The remaining 6 are unrelated to this channel -- five need server-initiated elicitation/sampling that 2026-07-28 removes outright, and one needs Modern list pagination. Refs #5959, #6033, #5743
dispatchModern's four list verbs returned the full aggregated set and never emitted nextCursor, so a client had no way to page and vMCP silently ignored any cursor it was sent. On the Legacy path the SDK's session-scoped feature store does this split; the Modern dispatcher bypasses the SDK, so it has to do it itself. Modern has no sessions, so a cursor may not denote server-held iteration state. The spec makes cursors opaque to clients -- they MUST NOT parse, modify, or persist them -- which is exactly what lets the server encode position INTO the token instead of remembering it. So the cursor carries the unique key of the last item delivered and the next page is the items sorted strictly above it: keyset pagination, stateless, and the same scheme go-sdk's own server uses. Aggregation across backends needs no per-backend positions. core.List* already returns one flat, conflict-resolved set whose keys are unique across backends, so the cursor encodes a position in the aggregated ordering and adding or removing a backend cannot invalidate it. Page size matches the SDK default (1000) so pages line up across revisions, and an undecodable or wrong-verb cursor is rejected as -32602 rather than being reinterpreted into a plausible but wrong page. The upstream cursor-following in pkg/vmcp/client and the session backend (#5851) is the opposite direction -- vMCP as client -- and is untouched. Refs #5959, #6033
Review found the paginator asserting a precondition that holds for one of its four callers. Only Tool.Name is unique -- ResolveToolConflicts keys a map -- while resources, resource templates and prompts are plain-appended across backends under "no conflict resolution for these yet". With a bare "resume strictly above lastKey" scan, a key shared at a page boundary made the scan skip every copy, so an item appeared on no page at all. Two honest backends both exposing file:///README.md was enough. A 1100-item corpus delivered 1097. The cursor now carries how many items sharing the last key were already sent, so a page can resume inside a run of equal keys. Items sharing a key are interchangeable for that purpose, which keeps the walk correct without a secondary sort key the generic signature cannot see. The comment claiming uniqueness is corrected rather than merged: it now states which callers can collide and that the paginator does not rely on uniqueness. The test corpus was the reason this shipped. Every case drew from a helper minting strictly unique keys, so the one precondition the code called load-bearing was the only thing untested. Collisions must also be placed by SORTED position -- a distinctively named duplicate key sorts away from the boundary and tests nothing, which a first attempt at the fixture did. Cap an inbound cursor before decoding it. A well-formed 6MB cursor decoded successfully at ~295ms of CPU on a verb that is not rate-limited, and a legitimate cursor is tens of bytes. Skip the capability fan-out in subscriptions/listen when nothing could be honored. core.Discover is un-rate-limited and, while no push capability is advertised, its result cannot change the answer -- so N no-op listens cost N fan-outs, now worse since list verbs page. Probing the ceiling first makes that O(1). This renders the fan-out error path unreachable, so its test is removed rather than left asserting a branch the code cannot enter. Assert the SSE framing. Replacing the blank-line event terminator with a single newline previously left the whole suite green -- including against a real client -- while a conformant parser would dispatch neither frame, so the ack-then-result contract was unverifiable. The helper now parses strictly, and the method name, acknowledgement name, subscriptionId key and jsonrpc field are asserted as literals; mutating any of them was silent. Add the negative capability test. The design rests on the advertisement honoring nothing, which was enforced by no test at all -- only by a runtime WARN that fires in production after the offending line ships. Now asserted across all sixteen presence combinations. Use http.ResponseController rather than a Flusher assertion: every ResponseWriter wrapper in the chain defines Flush unconditionally, so the assertion only established that a wrapper was present. Correct the comment that claimed otherwise, and the one claiming headers were committed on all four failure paths when three precede WriteHeader. Log the honored set by count, not by client-supplied URIs. Update docs/arch: subscriptions/listen is no longer unimplemented, serverStreamRegistry is the seam for delivery rather than for the handler, and both the pagination behaviour and the tools-only uniqueness property are now recorded. Refs #5959, #6033, #5743
The seven real-backend forwarding tests exercise server-initiated elicitation and sampling, which MCP 2026-07-28 removes outright: go-sdk's assertServerInitiatedRequestAllowed refuses them on negotiated protocol version alone, never consulting client capabilities. They passed only because newRealTestServer leaves Modern dispatch disabled, so the downstream client landed on Legacy incidentally. Pin the downstream client to Legacy explicitly by answering its server/discover probe with a successful Legacy-only DiscoverResult -- what a Legacy-only server actually sends -- and assert the pin fired, so deleting it fails loudly instead of silently relying on a server-side default. Two of the seven passed vacuously without the pin: a sessionless connect error satisfied their lenient assertions regardless of the capability advertisement they exist to exercise. Verified by falsifying their premise -- with the capability granted they pass unpinned and fail pinned. Add Modern-path tests recording today's actual behaviour for progress and logging, so all five dispositions are executable rather than prose, and document why each is what it is: progress is pinned because dispatchModern cannot stream, not because Modern lacks the channel. Refs #5959, #6033, #5743, #6058, #6062.
Serve the MCP 2026-07-28 client-facing surface that vMCP was missing. Add subscriptions/listen as a capability-negative conformant handler. The honored subscription set is computed by intersecting the client's request against the same advertised capabilities server/discover publishes, so the acknowledgement cannot promise what discover denies. vMCP advertises no push capabilities today, so the set is always empty and the stream closes at once rather than holding a socket open with no delivery mechanism behind it. Without this a go-sdk v1.7 client could not complete Connect at all: it is Modern-first, and once server/discover succeeds it opens a listen stream whenever a list-changed handler is registered. Add nextCursor pagination to the four Modern list verbs using keyset cursors. Cursors are opaque to clients, which is what licenses encoding position into the token rather than holding server-side iteration state -- necessary under a revision that removed sessions. The paginator is collision-safe. Keys are unique for tools but not for resources, templates or prompts, which the aggregator appends without conflict resolution; resuming past a key dropped every item sharing it, losing items permanently at page boundaries. Refs #5959, #6033, #5743, #6060, #6065.
Specs that assert session semantics -- Redis cross-pod reconstruction, pod-restart recovery, lazy termination eviction, upstreamInject restore, HMAC session token binding, and session independence -- were building their sessions through the mcpcompat client. That client wraps go-sdk, whose v1.7 Connect is Modern-first (it probes server/discover before initialize and upgrades on advertisement) and cannot be pinned to a protocol version (#5911). Now that vMCP serves 2026-07-28 -- which removed sessions -- whenever the capability gate is open, those clients negotiate Modern, get no session, and every sessionID assertion fails. Port them onto RawMCPClient with per-request Legacy pinning, following the convention virtualmcp_dual_era_redis_test.go's header prescribes and the precedent #6051 set: a spec about sessions now declares, at each call site, that it depends on the Legacy revision. Sessions are Legacy-only semantics under 2026-07-28, so the pin narrows nothing. The shared primitives live in legacy_session_helpers_test.go; the dual-era file's Legacy helpers delegate to them. MCPServer and MCPRemoteProxy scaling specs are untouched: the transparent proxy negotiates against the backend, which stays Legacy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam
13313fd to
269c673
Compare
github-actions
Bot
added
size/XL
With Modern dispatch now unconditional, a rate-limited tools/call reached Modern clients as an opaque -32603 instead of RFC THV-0057's -32029 with data.retryAfterSeconds: writeModernDispatchError laundered every non-authz error into the generic internal code, dropping the code and data that the Legacy seam preserves in structuredContent (conversion.ErrorToToolResult). Clients lost the machine-readable retry metadata exactly on the path that is now the default. Add a CodedError branch mirroring the Legacy seam's posture and #6061's capability-refusal classification: the dispatcher owns the envelope, so it emits the real JSON-RPC error object. HTTP status stays 200 because -32029's natural 429 is in go-sdk's transient retry set and would be silently retried instead of surfaced. The rate-limiting operator spec asserted the Legacy structuredContent rendering through a client that now negotiates Modern; it asserts the Modern surface instead, pinning the full envelope with an era-pinned raw request per the #6051 convention. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n
The era-pinned session specs rewritten off the mcpcompat client (#6051) fail against any pod serving a rehydrated cross-pod session: mcpcompat's rehydration path answers POSTs as text/event-stream regardless of the request's Accept header, because go-sdk v1.7.0-pre.3 does not export the transport-level JSONResponse knob (StreamableServerTransport.jsonResponse) that its top-level handler options set. The old mcpcompat test client tolerated SSE; the raw client left the envelope unparsed, so pod-B assertions failed with an empty result on an HTTP 200. Extract the JSON-RPC response event from an SSE body into the RawResponse envelope, skipping interleaved notifications, mirroring mcpcompat's own rehydration test helper (readFirstResult). Body keeps the raw stream for callers that inspect it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n
|
CI triage for the four failures on 269c673, and fixes pushed in 2b0613f + 63efad9: 1. 2. 3. The Generated with Claude Code |
github-actions
Bot
added
size/XL
* Design MRTR for the vMCP Modern path The 2026-07-28 revision removes server-initiated requests outright and replaces them with client-polled Multi Round-Trip Requests (SEP-2322). vMCP has no MRTR implementation: elicitation and sampling forward only over Legacy sessions, and the Modern egress shim declares empty clientCapabilities, so a compliant Modern backend that needs input answers -32021 and the call surfaces as an opaque protocol error. With the Modern dispatch kill-switch being removed (#5959/#6033), that gap becomes the served behavior. Add the design the closed-as-not-planned #5759 asked for, as its own document so it composes with the in-flight edits to doc 10 (#6050, #6051). Decisions, grounded in the spec text read one day before the revision's finalization and in RFC-0083: - Modern-client/Modern-backend rounds are a stateless pass-through: per-request capability mirroring, verbatim inputRequests/requestState relay, deterministic re-routing of the retry. vMCP adds no state, so no durable store is triggered. - Legacy-client/Modern-backend rounds bridge in-request: vMCP fulfills the backend's inputRequests through the existing ElicitationRequester/SamplingRequester seams and retries the backend call itself, bounded, on one pod — the same bridge go-sdk's own serverMultiRoundTripMiddleware performs for its handlers. - Modern-client/Legacy-backend stays deliberately unbridged, per the costed rejection recorded in doc 10; the Tasks extension is the sanctioned stateful path. - Only composite-workflow suspend/resume needs durable state, and it takes RFC-0083 D5 verbatim: an opaque >=128-bit handle as the client-visible requestState, WorkflowStatus.Owner bound to plaintext (iss, sub) via session/binding.Format, owner-checked resume, TTL, audit redaction, Redis store behind a core.Config seam. - Sampling gets no feature work: SEP-2577 deprecates it, and the relay is type-agnostic, so it transits for existing counterparts only. Roots additionally has no existing vMCP counterpart and is refused on the bridge cell. The gap list names what the pinned go-sdk/mcpcompat cannot express (mcpcompat drops inputRequests/requestState in both directions; the SDK's MRTR internals are unexported), phrased for toolhive-core. Refs #5743, #5759, #6018 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n * Surface Modern input_required rounds as typed values An MCP 2026-07-28 backend that needs more input answers with a resultType:"input_required" envelope (SEP-2322). The Modern egress shim collapsed that into the opaque errModernInputRequired sentinel, discarding the inputRequests and requestState payload — so no upper layer could ever relay or fulfill a round, and the MRTR work designed in docs/arch/16-vmcp-mrtr.md had no seam to build on. Decode the envelope into a domain vmcp.InputRequiredResult carried by a typed error. The values stay opaque json.RawMessage (the pass-through relay must forward them verbatim, never reinterpret them) with a Methods() probe for the capability gate; the error unwraps to errModernInputRequired with a byte-identical message, so revision classification (probeRevision) and every client-visible behavior are unchanged — capabilities are still declared empty, so a compliant backend cannot yet send a round at all. InputRequiredFromError is the single branch point the ingress relay and the Legacy-client bridge (slices 2-4 of the design) will consume. Confined to pkg/vmcp and pkg/vmcp/client, which none of the in-flight Modern-path PRs (#6050, #6033, #6051) touch. Refs #5743, #5759 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n * Flag the MRTR design's draft-only spec dependencies The 2026-07-28 revision finalizes one day after this design was written against schema/draft. The schema types and SEPs it cites are already Final, but four load-bearing points rest on the draft spec page's text, which refines the Final SEP: the three-method table (the SEP also allowed GetTaskPayloadRequest), the capability-gating MUST NOT, the at-least-one-field requirement, and the requestState security language. Name them explicitly with the design's exposure to each, so the re-verification against the final cut is a checklist rather than a re-read. Refs #5759 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n * Reconcile the MRTR design with merged #6061 PR #6061 landed the two-path mid-call capability-refusal contract this design's sequencing anticipated: -32021 with data.requiredCapabilities at HTTP 200 (documented deviation, go-sdk#1117) for an undeclared capability, an explicit -32603 naming SEP-2322 for a declared one. Reconcile rather than duplicate: the contract is permanent for the Modern-client/Legacy-backend cell (the only cell its refusal recorder can fire on — the SDK adapters are the Legacy-session forwarding seams), superseded by capability mirroring only where the backend is Modern. Name writeModernCallFailure as slice 2's rendering hook — the input_required branch and the refusal branch cannot co-occur — and note the declared-case message needs rescoping when slice 3 lands. Also record the finalization re-check: on 2026-07-28 the revision is still not cut (no schema/2026-07-28 directory, /specification/2026-07-28 pages 404, schema/draft byte-identical to the copy designed against), so the draft-only-dependency checklist stays pending. Refs #5759, #6061 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n * Regenerate CRD reference docs task crdref-gen output shifts by two trailing blank lines once this branch's new exported pkg/vmcp types are in the generator's scanned set. Verified the drift is this branch's consequence, not pre-existing staleness: regenerating in a clean origin/main worktree produces no diff. Generated file only; no generator or config changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n * Track the MRTR design under issue #6059 The human filed #6059 for exactly the pass-through this document designs, superseding the closed-not-planned #5759 the doc referenced. Point the design at the live issue, defer to doc 10's landed rationale for both current limitations instead of restating it, and delineate the adjacent Modern-path issues (#6058 streaming, #6064 ping/resultType, #6065 subscriptions) this design deliberately does not cover. One genuine divergence is flagged rather than papered over: #6059 sketches wrapping the backend's requestState with vMCP routing context, while this design relays it verbatim and re-derives routing from the capability name — because a vMCP-authored wrapper makes vMCP a state-minting server under MRTR server requirements 4-5, a key-management surface the verbatim relay avoids. Recorded as an open decision to resolve at slice 2/3. Part of #6059. Refs #5743. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n * Harden the MRTR egress seam per #6074 review Review (jhrozek) found the seam promising more than it enforced. Extraction is now fail-closed on three gates the design already stated but the code did not check: the method allow-list (only tools/call, resources/read, prompts/get may carry a round), strict payload decode (a wrong-typed inputRequests or requestState is a schema violation, not an empty round), and server requirement 6 (an envelope with nothing to fulfill and nothing to echo must not become a retry-forever round). RequestState becomes *string because client requirement 2 makes absent-vs-present-and-empty an observable distinction the retry must preserve. The carrier and extractor move to pkg/vmcp beside InputRequiredResult with exported fields, so slice 2's server-side rendering never has to import the concrete HTTP client and mock-based consumers can construct the error. Classification and message text are unchanged: the typed error still unwraps to the client's sentinel and renders byte- identically — except that a resultType beyond 64 bytes is now truncated to a prefix plus length, the #6079/#6066 rule, since the text reaches the downstream client. Also per review: modernResultTypeInputRequired constant beside the existing complete constant, the unrecognized-resultType comment cites the page's MUST rather than the SEP's SHOULD, the sentinel's comment explains why its message is frozen instead of claiming MRTR is deferred, the message-pinning test hardcodes the expected string so test and production cannot drift together, and the verbatim-relay test asserts raw bytes (assert.Equal) as its name promises. Part of #6059. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n * Correct MRTR design claims found false in review The principal-binding claim in cell 1 was false for two of six egress strategies: header_injection (static env-mounted secret) and unauthenticated (no-op) present one vMCP identity for every downstream user, so the backend's requestState principal binding cannot protect one downstream user from another. Scope the claim to the user-derived strategies and state the consequence — under shared-credential egress vMCP must bind the round itself, because no other component can — which turns wrap-vs-verbatim into a configuration-dependent obligation. Record the third arm the framing missed: a server-side D5 handle over a verbatim backend round, which closes that gap with no keys at the cost of a durable store on the pass-through path. Also from review: the D5 owner check is vacuous under incomingAuth: anonymous (every session stores the unauthenticated sentinel), so slice 5 documents that as a single-tenant caveat and the stronger-than-AEAD claim is scoped to authenticated incoming auth; the drift list gains the fifth SEP-vs-page divergence (unrecognized resultType SHOULD vs MUST) and stops calling requirements 6 and 7 page-only (one is in schema.ts, the other a core-page MUST); SEP-2577's union freeze does not name InputRequest, so that citation is scoped to what the SEP actually says; a cell-1 sequence diagram per the arch-docs convention; and the in-flight list reflects #6050/#6051 having merged. Doc 10's client-edge section still called the -32021 contract 'the planned follow-up' — #6061 landed it, and this PR is the reconciliation pass, so the refresh rides here. Part of #6059. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n * Regenerate CRD reference docs after main merge Same cause as the earlier regen on this branch, not pre-existing staleness: a clean origin/main (92c70db) worktree regenerates with no diff, while this branch's additions to pkg/vmcp — whose shared types the VirtualMCPServer CRD reference renders — make the generator emit two extra blank lines. The merge of main resolved docs/operator/crd-api.md to main's copy, dropping the lines the earlier regen commit had added, so the check failed again on the merge result. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n * Narrow anonymous-auth handle-theft rationale The D5 caveat claimed a stolen resume handle concedes no privilege the anonymous trust model doesn't already grant. That holds for invoking the tools, but not for the data: resuming loads accumulated step outputs produced from the original caller's arguments, which a thief running the same workflow could not necessarily reproduce. Scope the claim to tool-invocation privilege and name the residual data-read exposure, which rests on handle secrecy and the existing redaction requirement. The decision — accept as a single-tenant caveat rather than refuse to suspend — is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n * Name missing sentinel in InputRequiredError InputRequiredError.Sentinel is documented as required, but the fields are exported so callers can build the error without the real client's envelope decode, and nothing enforces it: a literal omitting Sentinel rendered "%!s(<nil>)" into text that reaches the downstream client. Guard both Error() branches with a named fallback so the omission is diagnosable instead of cryptic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Reviewed the whole branch (5 commits) plus two other review passes, and traced every load-bearing claim back to code. Overall this is a good change and I'd merge it: the capability gate is a real improvement on the env var, the production diff is small (9 files, +166/-69), and CI is green including the three E2E Test Lifecycle kind versions and both Operator E2E suites that were red on the earlier heads. The integration test that asserts go-sdk's stateful discover answer empirically, rather than trusting the vendored-SDK reasoning, is the right call on the riskiest assumption in the design.
One thing I'd want before merge is the constructor guard in the first comment below. Everything else is either a comment correction or a follow-up.
A note on the comments, since this PR leans on them heavily: several load-bearing ones assert things that aren't true, and one regressed from correct to incorrect in this diff (modern_pagination.go). That's the main theme of the review. Individual comments below.
Three findings that had nowhere sensible to anchor inline:
-
newModernDiscover(modern_envelope.go:248-258) hardcodesSupportedVersions: [Modern, LATEST]and never consults the gate. Unreachable today becauseclassification.go:115-118routesserver/discoverto the SDK beforedispatchModernwhen gated, but if that exemption is ever removed a gated instance would advertise Modern and then refuse it. Either gate the list or put a line on the function saying it must only be reached with the gate open. -
Modern
ping(modern_dispatch.go:105) returns a bare{}with noresultType, which schema.ts MUSTs. The comment there already says all of this and explicitly defers it, so this is only a note that a second reviewer flagged it independently - the deferral reasoning still looks right to me (a go-sdk Modern client never pings), just noting-32601remains the conformant answer when someone gets to it. -
Commit
5a7bcedcestill carries a "KNOWN BLOCKER, do not merge as-is" banner aboutdispatchModernlackingsubscriptions/listen. That landed in the base via #6050 (modern_subscriptions.go), so the banner is stale. Worth amending before merge since commit messages outlive the PR - anyone bisecting to that commit reads a blocker that no longer exists.
For the record, one of the other review passes rated the modern_gate.go invariant a nit on the grounds that "the gate fails safe." I traced it and it fails open - details in that comment. I'd not let the nit rating stand on that one.
Review on #6033 found the gate fails open for a direct-Serve caller that configures an optimizer without FactoryConfig.AdvertiseFromCore: sessionmanager only surfaces OptimizerFactory() when the flag is set, while the (discarded) decorator path runs when it is not -- so the optimizer indexed the FTS5 store, served nobody, and reported no blocker. sessionmanager.New now rejects that configuration at startup, making a non-nil OptimizerFactory() a faithful "optimizer enabled" signal; the unreachable decorator branch stays until #6103 deletes it. The rest is comment, wording, and consistency corrections from the same review: the wrong 429/go-sdk-retry rationale (go-sdk rejects a non-200 before decoding the body, so a 4xx would discard retryAfterSeconds unread), the -32029 reserved-band note (#6101), the gate contract's loud-refusals bullet (elicitation workflow steps fail Modern clients with an explicit -32021; distinct from #6059) and per-entry issue-ref requirement (#6089), denial-first ordering in conversion.ErrorToToolResult to match the Modern writer, len(data)>0 across the coded-error renderers, HasJSONRPCID gating in writeModernMissingCapability, a case-insensitive SSE Content-Type match plus CRLF/no-space/batch parser tests, the regressed Over1000Tools comment (Legacy-only, #5911), stale "unconditional dispatch" and "cannot parse SSE" comments (#6104), the gate verdict logged at WARN, and the optimizer's Legacy pin documented on the config field, the CRD reference, and the tier table (#6089). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
63efad9 to
1cb7c40
Compare
github-actions
Bot
added
size/XL
|
Thanks @jhrozek — this was an unusually thorough review and it caught a real fail-open plus a systematic comment-rot problem. All findings are addressed on head
Follow-ups filed from this review: #6101 ( One behavioral note added to the PR body: an out-of-tree direct- |
The Optimizer field's new Legacy-pin documentation flows into the controller-gen CRD manifests as well as crd-api.md; the Generate CRDs CI job diffs deploy/charts after task operator-manifests and caught the missed regeneration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
size/XL
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Approving. Re-reviewed the delta on 1cb7c409 + 31cec899 rather than the whole branch, since the trees told me I could: bcd7941c4 is byte-identical to the 63efad9f I reviewed (both 5110751…), so the force-push preserved the reviewed content and my earlier review still applies as-is.
On the one thing I'd asked for before merge — the constructor guard is right, and it's at the layer that makes the invariant structural instead of documented:
if (cfg.OptimizerFactory != nil || cfg.OptimizerConfig != nil) && !cfg.AdvertiseFromCore {OptimizerFactory() now means "optimizer enabled" by construction, so the gate can't fail open. You also fixed the stale session_manager.go comment that claimed server.New leaves AdvertiseFromCore false, which was the comment a future maintainer would have used to check this invariant — and repointed TestOptimizerFactoryGatedOnAdvertiseFromCore at the guard rather than the old nil-factory behavior, so the property is pinned and not just asserted.
Your point that it was worse than I described is correct and I'd missed it: in the misconfigured case the optimizer indexed the FTS5 store, embedding round-trips included, while serving its meta-tools to nobody including Legacy. My trace stopped at the Modern symptom.
Spot-checked the rest of the fix commit: the conversion.ErrorToToolResult flip to denial-first, the -32029 reserved-band note, the len(data) > 0 unification, HasJSONRPCID in writeModernMissingCapability, the case-insensitive Content-Type match with the CRLF/no-space/batch cases, and serve.go being comments plus the WARN. serve.go also picked up stale WorkflowDefs docs I hadn't flagged (they're core-owned, not carried on SessionManagerConfig), which is a nice catch on the way past.
Taking the loud-refusal answer on the elicitation question: documenting the exclusion with the reasoning, and distinguishing it from #6059, resolves what I actually wanted, which was to be able to tell that the case had been considered rather than missed. Same for the per-entry issue-ref requirement — that's the thing that keeps the blocker list from silently accruing entries.
Thanks for the two corrections back at me: ping being removed from 2026-07-28 outright, and the revision still being at -RC upstream. Both make the follow-ups stronger rather than weaker.
One caveat on the timing, purely so it's on the record: I'm approving with 8 checks still pending, including Tests / Test Go Code, Linting, and all three E2E Test Lifecycle kind versions. 12 were green and nothing was failing. The guard adds a new startup failure path to a constructor that test fixtures use, so that unit-test run is the one I'd glance at before hitting merge — you updated optimizer_gate_test.go and serve_lifecycle_test.go for it, so I expect it's fine, but the suite hadn't reported yet when I approved.
2 participants
Summary
Why.
Config.ModernDispatchEnabled(envTOOLHIVE_VMCP_MODERN_STATELESS, default off) was a temporary safety lever gating whether well-formed MCP 2026-07-28 ("Modern") requests are served byclassifyingHandler→dispatchModernor fall through to the SDK path. #5959 asks for its removal before GA.This PR no longer just deletes the switch — it replaces the global env var with a capability-based gate, because two rounds of e2e CI (heads
166cc52e,ff52aed1) proved the switch was masking a genuine gap: with Modern served unconditionally, go-sdk v1.7+ clients — whoseConnectprobesserver/discoverbeforeinitializeand upgrades to whatever the server advertises — landed on the Modern path against optimizer-enabled instances and silently received the raw aggregated tool set instead offind_tool/call_tool(tools/call find_tool→-32603 not found). The optimizer is Serve-layer and session-scoped by design (serve_optimizer.go); the stateless dispatch path cannot serve it.What.
ModernDispatchEnabled/TOOLHIVE_VMCP_MODERN_STATELESSand the switch branch inclassification.go.modernDispatchBlockers()(pkg/vmcp/server/modern_gate.go): an explicit, commented enumeration of enabled features the stateless path cannot serve. Instead of "don't serve Modern until someone flips an env var", vMCP now serves Modern exactly when it can serve it correctly — an instance with no blockers advertises and dispatches 2026-07-28; an optimizer-enabled instance keeps clients on Legacy.server/discoverfalls through to the SDK, whose stateful transport advertises a Legacy-only version list (this steers Modern-first clients onto the Legacy handshake with no error surfaced); every other well-formed Modern request is refused with a conformant400+-32022 UnsupportedProtocolVersionErrorlisting the Legacy version. Legacy traffic is untouched either way.TestModernDispatchBlockers,TestClassifyingHandler_ModernCapabilityGate) and a full-handler integration pair (modern_gate_integration_test.go) in both directions — gated and open. Achieving Modern parity for a feature means deleting one entry and updating those tests.TOOLHIVE_VMCP_MODERN_STATELESSpod-template env fromvirtualmcp_dual_era_redis_test.goand updates its era-pinning comments.ClassifyRevisionremains the owner of every other version-grounds rejection, so a request naming some other protocol version is still refused with a conformant-32022.What deliberately does NOT gate Modern: Redis-backed session sharing. Legacy clients keep their shared, reconstructible sessions while Modern clients are sessionless by design and store nothing — coexistence asserted end-to-end by
virtualmcp_dual_era_redis_test.go(#6056). The gate's contract comment records the rule: an entry is warranted only when a Modern client would silently receive different behavior than the feature promises, not merely because a feature is session-flavored.The second gap the switch was masking — session specs that cannot pin their era. The operator suite's session-semantics specs (Redis cross-pod reconstruction, pod-restart recovery, lazy termination eviction, upstreamInject identity restore, HMAC session token binding, session independence) built their sessions through the mcpcompat client. That client wraps go-sdk, whose v1.7
Connectis Modern-first and cannot be pinned to a protocol version (#5911) — so against a Modern-serving vMCP it negotiates 2026-07-28, which removed sessions, and everysessionIDassertion fails (that is thevirtualmcp_redis_session_test.gofailure on this PR's earlier heads). This PR ports every such spec ontoe2e.RawMCPClientwith per-request Legacy pinning — the conventionvirtualmcp_dual_era_redis_test.go's header prescribes and the precedent #6051 set. The diff is deliberately mechanical and large: each of those specs now declares, at every call site, that it depends on the Legacy revision, so the next place Modern gets enabled doesn't discover the dependency through a red CI shard. Sessions are Legacy-only semantics under 2026-07-28, so the pin narrows no coverage. Shared primitives live inlegacy_session_helpers_test.go; the dual-era file's Legacy helpers delegate to them;CreateInitializedMCPClient's doc comment now warns session specs away from it.Closes #5959
This PR was blocked, and no longer is
It was opened as a draft with a DO-NOT-MERGE banner because removing the switch exposed a real hole:
dispatchModernhad nosubscriptions/listencase, and that method is the 2026-07-28 revision's only server→client push channel. go-sdk v1.7'sConnectis Modern-first and opens a listen stream whenever a notification handler is registered — whichmcpcompat'sInitializedoes unconditionally — so a-32601there tore the session down. 20 regressions, exposed rather than caused: settingModernDispatchEnabled: trueon cleanmainreproduced them identically.Both gaps landed as #6050 (
subscriptions/listen+ Modern list pagination) and #6051 (the forwarding tests' Legacy dependency made explicit). The e2e runs on this PR then exposed the two further gaps described in the Summary — the optimizer and the unpinnable session-spec clients — which this revision addresses with the capability gate and the Legacy port.Type of change
Test plan
task test) — includes new gate tests:TestModernDispatchBlockers,TestClassifyingHandler_ModernCapabilityGate,TestIntegration_ModernGate_OptimizerKeepsClientsOnLegacy,TestIntegration_ModernGate_PlainInstanceAdvertisesModerntask lint) — 0 issues;go vet ./...cleanserver/discoverfalls through to the SDK and answers with a Legacy-onlysupportedVersionslist (asserted by the integration test, which fails loudly if the SDK's stateful discover answer ever changes shape)vmcp_optimizer_test.go,vmcp_cli_features_test.go:485,virtualmcp_optimizer_composite_test.go:201) because their clients now read a Legacy-only discover answer and land on the session path wherefind_tool/call_toolare advertised, and the session specs (virtualmcp_redis_session_test.go) because they now pin Legacy per request instead of depending on an unpinnable client.Changes
pkg/vmcp/server/modern_gate.gomodernDispatchBlockers(): the capability gate's feature enumeration, one commented entry per unservable feature (currently: optimizer)pkg/vmcp/server/classification.goModernDispatchEnabledbranch; add the capability-gate branch (refuse-32022,server/discoverfalls through)pkg/vmcp/server/serve.goModernDispatchEnabledfromServerConfig; log the gate's verdict once at startuppkg/vmcp/server/server.goModernDispatchEnabledfromConfigpkg/vmcp/cli/serve.gopkg/vmcp/server/modern_gate_integration_test.go-32022; plain instance → advertises Modernpkg/vmcp/server/classification_test.goTestModernDispatchBlockers,TestClassifyingHandler_ModernCapabilityGate)test/e2e/thv-operator/virtualmcp/legacy_session_helpers_test.golegacySessionInit,legacySessionListTools,legacySessionCallTool) with the why-not-mcpcompat rationaletest/e2e/thv-operator/virtualmcp/virtualmcp_redis_session_test.gotest/e2e/thv-operator/virtualmcp/virtualmcp_session_management_test.gotest/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.goTOOLHIVE_VMCP_MODERN_STATELESSpod-template env; Legacy helpers delegate to the shared primitives; comments now state that Redis must never close the gatetest/e2e/thv-operator/virtualmcp/helpers.goCreateInitializedMCPClientdoc warns session specs onto the raw primitivesdocs/arch/10-virtual-mcp-architecture.md(plus the mechanical test updates carried from the first revision:
newRealModernTestServer→newRealTestServeret al.)Does this introduce a user-facing change?
Yes. The
TOOLHIVE_VMCP_MODERN_STATELESSenvironment variable no longer exists and is ignored if set. vMCP serves MCP 2026-07-28 to clients whenever the instance's enabled features permit it; an optimizer-enabled instance keeps clients on Legacy (2025-11-25), wherefind_tool/call_toolwork, and refuses direct Modern requests with a conformant-32022listing the supported version.For library consumers only:
sessionmanager.Newnow rejects an optimizer configured withoutFactoryConfig.AdvertiseFromCoreat startup. That configuration previously ran the optimizer indexer without serving its meta-tools to anyone and left the Modern gate open; no in-tree composition ever produced it (server.Newalways sets the flag).Special notes for reviewers
modern_gate.go.server/discoverfalls through when gated instead of being answered bydispatchModernDiscover: the SDK's stateful transport already computes the correct Legacy-only version list (SupportsProtocolVersionexcludes2026-07-28unlessStateless), and duplicating that list by hand would create a second source of truth. The integration test pins the answer's shape.Operator CI / Operator Tests Integrationfailure on the previous head is NOT this PR: its only real failure was the mcp-group suite's AfterSuite envtest teardown ("timeout waiting for process kube-apiserver to stop",testutil/envtest.go:181) with all 5 specs passing — a known environmental flake, please don't chase it here.E2E Test Lifecyclekind-version failures on the previous heads were one bug, the optimizer gap above (virtualmcp_optimizer_composite_test.go:201on all three kind versions); the Redis-session spec failure on v1.35.1 (virtualmcp_redis_session_test.go:426, empty session ID) was the separate era-pinning issue, fixed here by the Legacy port. All other listed spec failures in those runs were[INTERRUPTED]collateral from Ginkgo aborting, not real failures.tools/listin the failing CI run), MCPToolConfig tool filtering/overrides (applied inside the aggregator,default_aggregator.go), and codemode (pkg/vmcp/codemode/decorator.go, a core decorator). Not verified by execution: the MCPToolConfig dynamic-update spec under a Modern client (it was only ever[INTERRUPTED]in CI); by code its updates land below the transport layer and Modern re-aggregates per request, so it should be unaffected.Review follow-through (2026-07-28, head
1cb7c409)All of @jhrozek's findings are addressed; per-thread replies below. The load-bearing ones:
sessionmanager.Newrejects an optimizer withoutAdvertiseFromCore, soOptimizerFactory()is a faithful "optimizer enabled" signal and the gate cannot see a running optimizer as no-blockers. The review's instinct was right twice over: in that configuration the optimizer also served nobody (the Serve layer discards the decorator's tools while the decorator still indexes FTS5, embedding round-trips included). The now-unreachable decorator branch stays until Delete the unreachable legacy optimizer decorator path in sessionmanager #6103 deletes it.checkResponserejects it without retrying. The real reasonwriteModernCodedErrorstays at HTTP 200 — now stated everywhere the claim appeared — is that go-sdk rejects a non-200 before decoding the body, so a 4xx would discard the JSON-RPC error object and itsdata.retryAfterSecondsunread.-32021/-32603; the gate contract now says loud refusals are deliberately out of scope and distinguishes this (vMCP as elicitor) from Support MRTR pass-through for Modern backends in vMCP #6059 (Modern backendinput_required).2026-07-28-RC(pre-release) and the live draft; latest stable remains 2025-11-25. The reviewer's error-code-reservation reading is confirmed verbatim ("Implementations MUST NOT emit any code from this sub-range that is not defined by this specification"), so-32029reallocation is filed as Rate-limit error code -32029 sits in the MCP-spec-reserved band #6101. Our-32022data shape already carries both required fields (supported,requested) and our codes are the post-renumbering ones.-32029reallocation), vMCP backend client cannot parse multi-line SSE data, silently downgrades Modern backends #6102 (readModernSSEmulti-line SSE bug — the silent Legacy downgrade the review spotted), Delete the unreachable legacy optimizer decorator path in sessionmanager #6103 (delete the unreachable legacy optimizer decorator), Run the dual-era e2e specs under the conformant Accept header #6104 (flip the dual-era specs to the conformantAccept). The Modernpingfinding was already tracked as Modern dispatch answers ping, which 2026-07-28 removed, and omits the required resultType #6064.Optimizerconfig field (and the generated CRD reference) and in the vmcp-local tier table, not just indocs/arch/.63efad9fplus the one fix commit.Generated with Claude Code