Make forwarding tests' Legacy dependency explicit - #6051
Merged
Conversation
JAORMX
requested review from
ChrisJBurns,
amirejaz,
jerm-dro,
jhrozek,
rdimitrov and
tgrunnagle
as code owners
July 27, 2026 19:30
github-actions
Bot
added
size/S
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6051 +/- ##
==========================================
- Coverage 72.24% 72.22% -0.03%
==========================================
Files 722 722
Lines 75125 75125
==========================================
- Hits 54277 54257 -20
- Misses 16985 17005 +20
Partials 3863 3863 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
reyortiz3
previously approved these changes
Jul 27, 2026
jhrozek
reviewed
Jul 27, 2026
jhrozek
left a comment
jhrozek
left a comment
Contributor
There was a problem hiding this comment.
Reviewed this as a test-only + docs change. The mechanism works (unit tests are green on a5275b7, and since the RoundTripper short-circuits the probe before it reaches vMCP, that green run does prove the Legacy fall-back actually fires rather than just agreeing with the kill-switch). The disposition table and the mutation verification are genuinely useful and I'd like more PRs to do that.
My concerns are all about the explanation rather than the code, which is awkward given the explanation is the point of the PR. The main one is that the WHY PIN block is wrong about what production does on both sides — see the comment on the RoundTripper. That's worth fixing before merge; the rest are smaller.
One unrelated note: Operator CI / Operator Tests Integration is red, but this diff only touches two pkg/vmcp/server test files and one doc, so it can't be from here. Probably worth a retest.
Skipping a couple of nits I noticed but don't think are worth your time (the doc says legacyPinningRoundTripper is in pkg/vmcp/server when it's really server_test; the arch-doc section is a bit more argumentative in tone than the rest of that file).
github-actions
Bot
added
size/M
JAORMX
added a commit
that referenced
this pull request
Jul 27, 2026
Maintainer review (jhrozek) on #6051, applied: - The pin now answers server/discover with a successful DiscoverResult whose supportedVersions lists only 2025-11-25 - literally what a Legacy-only server sends and the same shape vMCP's SDK path answers today (classification.go exempts the probe rather than rejecting it, so the previous -32022 mimicry described a response production never produces for this method). go-sdk negotiates against the version list and falls back to Legacy initialize either way. - Drop assertions on go-sdk's error-wrapping substrings (the refused request's name reaches the message only via the SDK's "calling %q" wrapper): the NoDownstreamCapability tests now assert structurally (round-trip on the live session + IsError), and the Modern honest-failure test defers the named-request property to the follow-up's vMCP-owned message. The backend-ID characterization stays - that string is ToolHive's. - Compress the per-test disposition comments to pointers at the arch doc, keeping only the two non-obvious facts in place (progress: vMCP cannot stream, Modern has the channel; logging: go-sdk sends a malformed request and the -32020 rejection is correct). Drop a stale line-number citation and an unverifiable build reference. Use "Modern dispatch enabled/disabled" over the ambiguous "kill-switch on/off". - Arch doc: cross-link the Modern-client limitation from the mid-call forwarding section (which otherwise still reads as if forwarding is unconditional), fix the test-package reference, and soften tone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam
github-actions
Bot
added
size/M
ChrisJBurns
previously approved these changes
Jul 27, 2026
9 tasks
github-actions
Bot
added
size/M
This was referenced Jul 27, 2026
Open
ChrisJBurns
approved these changes
Jul 27, 2026
Collaborator
|
/retest |
1 similar comment
Collaborator
|
/retest |
Collaborator
|
@JAORMX Same here I believe, regarding e2e flake |
github-actions
Bot
added
size/M
This was referenced Jul 28, 2026
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
Four-reviewer panel findings on the forwarding-test disposition, applied without changing what the tests disposition: - Correct the pin comment's mechanism claim: vMCP never rejects server/discover (the kill-switch exempts it); it answers 200 with a version-omitting result and the client negotiates down. The RoundTripper substitutes a -32022 rejection to force the same outcome deterministically. - Clone the request in RoundTrip instead of mutating the caller's (net/http RoundTripper contract), and count discover interceptions, asserted after every connect - on a kill-switch base the pin is otherwise behaviorally inert and indistinguishable from dead code. - Name the refused request in the NoDownstreamCapability assertions, closing their vacuous-pass route. - Stop pinning -32603 in the Modern honest-failure test: the spec MUSTs -32021 for the undeclared case, but at HTTP 400, which go-sdk's client escalates to permanent session death - so the code change is deferred to a follow-up serving -32021 at 200 as a documented deviation, and the test asserts the durable contract (explicit error naming the refused request, no input_required, bounded, no hang) rather than the code we know is wrong. The backend-ID leak in the echoed message is characterized with flip-to-NotContains instructions. - Add bounded Modern pins for the two prose-only dispositions: progress is dropped and the call completes (a vMCP streaming-dispatch gap, not a spec absence), and logging/setLevel is -32601 when well-formed and -32020 for the malformed shape go-sdk actually sends. - Give postModern a real deadline so "no hang" is asserted, not implied. - Fix the Tasks citation to SEP-2663 (tasks/get + inputResponses on tasks/update; SEP-1686's blocking tasks/result was removed), rewrite the logging clause that implied setLevel still exists on Modern, and document in the arch doc why the spec-mandated 400 is unshippable and that a clean error does not mean the backend executed nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam
Maintainer review (jhrozek) on #6051, applied: - The pin now answers server/discover with a successful DiscoverResult whose supportedVersions lists only 2025-11-25 - literally what a Legacy-only server sends and the same shape vMCP's SDK path answers today (classification.go exempts the probe rather than rejecting it, so the previous -32022 mimicry described a response production never produces for this method). go-sdk negotiates against the version list and falls back to Legacy initialize either way. - Drop assertions on go-sdk's error-wrapping substrings (the refused request's name reaches the message only via the SDK's "calling %q" wrapper): the NoDownstreamCapability tests now assert structurally (round-trip on the live session + IsError), and the Modern honest-failure test defers the named-request property to the follow-up's vMCP-owned message. The backend-ID characterization stays - that string is ToolHive's. - Compress the per-test disposition comments to pointers at the arch doc, keeping only the two non-obvious facts in place (progress: vMCP cannot stream, Modern has the channel; logging: go-sdk sends a malformed request and the -32020 rejection is correct). Drop a stale line-number citation and an unverifiable build reference. Use "Modern dispatch enabled/disabled" over the ambiguous "kill-switch on/off". - Arch doc: cross-link the Modern-client limitation from the mid-call forwarding section (which otherwise still reads as if forwarding is unconditional), fix the test-package reference, and soften tone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam
The leak fix (vMCP-owned messages in the capability-error contract, #6061) is imminent, so an assertion whose own comment says to flip it shortly is churn that makes the fix noisier rather than clearer. The message is now asserted on nothing at all - deliberately: its only identifying substrings are either go-sdk's error wrapping (a dependency's strings) or the leak itself; both message properties are asserted in #6061 where vMCP owns the text. The HTTP 200 assertion returns in its place, valid across the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam
github-actions
Bot
added
size/M
jhrozek
approved these changes
Jul 28, 2026
This was referenced Jul 28, 2026
JAORMX
added a commit
that referenced
this pull request
Jul 28, 2026
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
JAORMX
added a commit
that referenced
this pull request
Jul 28, 2026
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
JAORMX
added a commit
that referenced
this pull request
Jul 28, 2026
Brings in #6051 (doc 10 client-edge limitation section, which this design defers to), #6061's merged contract, and #6079's error-text hygiene precedent, all referenced by review feedback on #6074. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n
JAORMX
added a commit
that referenced
this pull request
Jul 28, 2026
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
JAORMX
added a commit
that referenced
this pull request
Jul 28, 2026
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
JAORMX
added a commit
that referenced
this pull request
Jul 28, 2026
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
JAORMX
added a commit
that referenced
this pull request
Jul 28, 2026
* 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why. Removing the Modern (2026-07-28) dispatch kill-switch (#5959, PR #6033) changes what the go-sdk client's Modern-first
server/discoverprobe gets back: today the probe is exempted from Modern-dispatch gating (classification.godeliberately lets it through so a client can negotiate down) and answered with a successful discover result whosesupportedVersionsomits 2026-07-28 — so clients negotiate down to Legacy incidentally; post-#6033 they negotiate Modern, and the surface the forwarding integration tests assert vanishes mid-test. That surface (mid-call elicitation/sampling requests, progress/logging notifications relayed over a live session) exists only on a Legacy session: 2026-07-28 removed server-initiated requests (go-sdk'sServerSession.assertServerInitiatedRequestAllowedrefuses them purely by negotiated protocol version — client capability declarations are never consulted), replaced them with client-polled multi-round retrieval (SEP-2322), and SEP-2577 deprecates sampling and logging (and roots) outright. This PR consciously dispositions every affected test — in code, not only in prose — so #6033 can land without touching them.Disposition table — every pinned test, its root cause under Modern, and its resolution:
TestForwarding_Elicitation_RealBackendelicitation/createremoved by 2026-07-28; replacement is client-polled MRTR (SEP-2322), deliberately not servedElicitingToolFailsCleanlyTestForwarding_Sampling_RealBackendElicitingToolFailsCleanlyTestForwarding_Sampling_RealBackend_SessionIsolationTestForwarding_Progress_RealBackenddispatchModernis single-shot and cannot stream — a vMCP streaming-dispatch gap, future workProgressDroppedtestTestForwarding_Logging_RealBackendSetLoggingLevelomits the per-request_metainjection, so vMCP's-32020rejection is correct; (2) the RPC no longer exists on Modern (logLevel_metareplaces it; go-sdk's own server answers-32601); (3) SEP-2577 deprecates logging-32601well-formed /-32020malformed-as-go-sdk-sends-it) pinned by newLoggingContracttest; no vMCP workaroundTestForwarding_Sampling_NoDownstreamCapabilityIsError), closing the vacuity route without asserting go-sdk's error stringsTestForwarding_Elicitation_NoDownstreamCapabilityWhat changed.
legacyPinningRoundTripper(test-side): answers the client'sserver/discoverprobe with a successfulDiscoverResultwhosesupportedVersionslists only 2025-11-25 — literally what a Legacy-only server sends, and the same version-comparison fallback vMCP's own exempted discover answer drives today (per maintainer review; the earlier-32022mimicry described a response production never produces for this method). It clones the request per thehttp.RoundTrippercontract, and counts interceptions, asserted after every connect — deleting the RoundTripper today fails all seven tests, so the pin is self-verifying rather than inert scaffolding. Comment marks it LOAD-BEARING after Replace vMCP Modern kill-switch with a capability gate #6033. Transport-level because mcpcompat cannot pin a protocol version (Handle per-backend MCP revision in vMCP (client-side dual protocol) #5911).TestIntegration_Modern_RealBackend_ElicitingToolFailsCleanly: asserts the durable Modern contract — bounded, HTTP 200, explicit JSON-RPC error, neverinput_required, never a fabricated success. Deliberately pins neither the error code nor anything about the message text: the code changes in PR Classify Modern mid-call capability refusals per spec #6061 (the spec-correct-32021contract), and the message's only identifying substrings today are go-sdk's error wrapping (a dependency's strings, per maintainer review) or the pre-existing backend-ID leak — both message properties are asserted in Classify Modern mid-call capability refusals per spec #6061, where vMCP owns the text.ProgressDroppedandLoggingContracttests convert the two prose-only dispositions into executable claims;postModerngained a real deadline so "no hang" is asserted, not implied.-32021/400 unshippability analysis; the side-effect note (a real backend may have executed up to the elicitation point); a Known limitation (Modern clients) cross-link from### Mid-call forwarding; MRTR Modern↔Modern pass-through as the named future shape (Epic: MCP protocol currency and 2026-07-28 stateless readiness #5743).References #5959, #6033, #5743, #5911. The production half of the honest-error contract is PR #6061 (split out of this PR so its security approval — based on test-and-docs-only — stays valid).
Changed since reyortiz3's approval (
a5275b7e)7f3d1b27(panel findings): RoundTripperClone, interception counter + per-connect assertion, strengthenedNoDownstreamCapabilityassertions, loosened error-code assertion, the two bounded Modern pin tests,postModerndeadline, SEP-2663/logging/mechanism comment corrections, arch-doc contract analysis.80d40d37(maintainer review, jhrozek): the pin now answers a successful Legacy-onlyDiscoverResult; go-sdk-owned substrings dropped from assertions; per-test comment blocks compressed to arch-doc pointers; stale line-number citation and unverifiable build reference dropped; "Modern dispatch enabled/disabled" terminology; mid-call-forwarding cross-link.4eb5db74: the backend-ID leak characterization dropped (the fix, #6061, is imminent; a flip-me-shortly assertion is churn). Still test-and-docs only — no production code. Please re-look.Rebase note for #6033 (load-bearing)
This PR's Modern tests call
newRealModernTestServer, which #6033 folds intonewRealTestServer. Whoever lands second applies one rename in one file (sed s/newRealModernTestServer/newRealTestServer/gonpkg/vmcp/server/modern_realbackend_integration_test.go) — verified: without itserver_testfails to build on #6033; with it, all forwarding + Modern tests pass on #6033's head with these commits cherry-picked.Type of change
Test plan
task test) — only the three known environmental failures (TestNewServer_ReadTimeoutConfigured,TestSecurityHeaders,TestCompositeProvider_RealProviders)task lint-fix) — 0 issues;go vet ./...run separately, cleanManual verification:
origin/pull/6033in a throwaway worktree + the one-file rename — all forwarding and Modern integration tests pass.writeModernDispatchErrorwriting acompleteresult):ElicitingToolFailsCleanlyfails immediately. Restored: passes.Does this introduce a user-facing change?
No.
Special notes for reviewers
TestForwarding_Progress_RealBackendcan time out waiting for the forwarded notification under CI-raceload (lost-notification race the code documents arounddrainServerToClientNotifications' best-effort ping barrier; same lineage as Flaky: TestForwarding_*_RealBackend time out waiting for forwarded notifications under CI load #5962). Observed once on7f3d1b27; not introduced by this diff, which contains no production code.-32021two-path capability-error contract and the backend-ID leak fix (both PR Classify Modern mid-call capability refusals per spec #6061), and conditional capability advertisement to Legacy backends (aggregation blast radius).logging/setLevelfailure go-sdk clients hit is an upstream go-sdk bug — filed separately, not worked around here.🤖 Generated with Claude Code
https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam