Classify -32022 and SSE backends as Legacy, sync stale comments#5997
Open
jhrozek wants to merge 3 commits into
Open
Classify -32022 and SSE backends as Legacy, sync stale comments#5997jhrozek wants to merge 3 commits into
jhrozek wants to merge 3 commits into
Conversation
Two backend-revision signals disagreed with the supportedVersions rule adopted for go-sdk v1.7, each able to strand a backend on the wrong path. A -32022 (CodeUnsupportedProtocolVersion) was treated as proof of Modern. It means the opposite: the peer does not support the version we asked for. A backend answering it was cached Modern permanently and enumerated zero capabilities, since isRevisionMismatch never reclassified it. Decode the error's data.supported list instead and keep it Modern-positive only when 2026-07-28 is still advertised, mirroring go-sdk's own reference client (mcp/client.go), whose test for this case is named "unsupported protocol version falls back to initialize". -32020/-32021 stay unconditionally Modern: both require the peer to have parsed Modern _meta. probeRevision was also transport-blind. TransportType "sse" names the deprecated 2024-11-05 two-endpoint transport, whose BaseURL is the GET-only /sse path, and modernCall is a single-endpoint POST, so no Modern endpoint is reachable there. Classify it Legacy without a probe, which also avoids POSTing into a stream and hanging to the client timeout (errModernTransient is uncached, so that cost recurred on every call). Retrying under the corrected Legacy classification stays safe for tools/call: go-sdk rejects an unsupported per-request protocol version before dispatching to any method handler, so the backend never executed the request. Also record that revision-cache self-healing is asymmetric under v1.7 and that the stale value reaches the mcpRevision CRD field (#5992). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five comments justified the hand-rolled Modern shim on the grounds that go-sdk v1.6.1 cannot express the Modern wire shape. The repo is now on v1.7.0-pre.3, which implements the revision, so that premise reads as false to anyone who checks it. The shim is still required, but for a narrower reason: mcpcompat's public Client API has no no-initialize primitive, only the private Legacy-shaped resumeCall. Say that instead. The parser integration test explained its SSE behavior by claiming the harness cannot capture an initialize carried over the SSE message channel. That was self-contradictory, since tools/call and resources/read traverse the same channel and the same middleware and are captured. The real mechanism: mcpcompat's SSE server uses go-sdk's NewSSEHandler, whose transport implements no ProtocolVersionSupporter gate, so it advertises 2026-07-28, discover succeeds, and initialize is never sent on that arm. Assert that directly rather than inferring it, and drop the claim that the streamable arm sends no discover -- it does, and is negotiated down. Note also that HTTP+SSE is deprecated but NOT removed by 2026-07-28; what that revision removed is Streamable HTTP's GET stream and sessions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The errModernNegotiatedDown arm of isRevisionMismatch had no truth-table row despite being the only surviving self-healing direction under go-sdk v1.7, so add both the Modern (mismatch) and Legacy (not a mismatch) cases. The reserved-_meta strip has two call sites but only the httpBackendClient one was tested; add the persistent-session counterpart, asserting reserved keys are stripped, non-reserved caller keys survive, and the caller's map is not mutated. Its fixture uses scalars because maps.Clone is shallow and would not have caught a nested mutation. Two robustness fixes: the SSE fake dropped a queued response after a five second timeout and returned an empty body, turning a failure into a hang against a deadline-free context, so fail loudly instead (both send sites, not just one). And the schema-fidelity test reached its Legacy path only because the fake's catch-all happens to answer discover with a body that lacks resultType; pin the revision so adding a field to that arm cannot silently route the test through the Modern shim and strand the injected clientFactory it exists to exercise. metaFor loses its method parameter (every caller passed tools/call, and a second calling function tripped unparam) and becomes toolCallMeta. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jhrozek
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
blkt,
jerm-dro,
rdimitrov and
tgrunnagle
as code owners
July 25, 2026 19:52
3 tasks
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## adopt-go-sdk-v1.7 #5997 +/- ##
=====================================================
- Coverage 72.06% 72.05% -0.01%
=====================================================
Files 718 718
Lines 74477 74489 +12
=====================================================
+ Hits 53669 53676 +7
- Misses 16975 16980 +5
Partials 3833 3833 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JAORMX
approved these changes
Jul 25, 2026
JAORMX
left a comment
JAORMX
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the full diff (base is adopt-go-sdk-v1.7, not main). Both behavioral fixes are correct and the reasoning holds up:
-32022classification. Dropping it fromisModernProtocolCodeand routing it throughclassifyUnsupportedProtocolVersionis right — a bare-32022means "I don't support the version you asked for," which a peer negotiating down to Legacy also returns, so treating it as unconditional proof-of-Modern was the bug (silently cached Modern + enumerated zero capabilities). Keying the decision ondata.supportedcontainingMCPVersionModern, with best-effort decode → empty → negotiated-down, is the correct fail-safe direction.-32020/-32021staying unconditionally Modern is right (both require the peer to have parsed Modern_meta).- SSE gate in
probeRevision. ClassifyingTransportType == "sse"(the 2024-11-05 two-endpoint transport, GET-only/sseBaseURL) as Legacy without probing is correct —modernCallis a single-endpoint POST that can't reach a Modern endpoint there — and it avoids POSTing into a GET stream and eating the client timeout on every call (sinceerrModernTransientis uncached).TestProbeRevision_SSEGateproves no network call by failing if the handler is hit.
The retry-safety property I paid the most attention to: errModernNegotiatedDown now feeds isRevisionMismatch, so dispatch reclassifies and retries — and via interpretModernResult this can fire on a side-effecting tools/call, unlike the discover-sourced case. The no-double-execution guarantee rests entirely on the peer rejecting the unsupported protocol version before executing the method. That's correct for a spec-compliant peer (per-request version validation precedes method dispatch, as in go-sdk's ServerSession.handle), and you've documented it precisely on the sentinel. Worth being explicit that this is the load-bearing assumption for any non-go-sdk Modern backend — but it's the right call and consistent with the double-exec discipline in the Legacy path.
Comment re-syncs are accurate (the stale v1.6.1-can't-express rationales, the revision-cache asymmetric-self-heal note pointing at #5992, the exact-match TRIPWIRE now naming both sites), and the new session-path strip test + toolCallMeta rename are good. Nice catch on the whole class of "old rule answering the same question" bugs.
Non-blocking: the #5992 staleness (mis-cached Legexpiry→Modern never self-heals and leaks to the mcpRevision CRD field) is real and I agree it's out of scope here — the initializeClient InitializeResult.ProtocolVersion in-band fix you mention sounds like the right follow-up.
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
2 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
Review follow-ups for #5993, stacked on
adopt-go-sdk-v1.7(base is that branch, notmain).#5993 correctly moved backend revision classification onto
server/discover'ssupportedVersions. Reviewing it turned up two other places where the same "is this peer Modern?" question was answered by the old rule, so a backend could still be stranded on the wrong path:-32022was treated as proof of Modern. It means the opposite — the peer does not support the version we asked for. A backend answering it got cached Modern permanently and enumerated zero capabilities, silently:modernListCapabilitiestolerates the error ascaps == nil, andisRevisionMismatchnever reclassified it. Now the error'sdata.supportedlist decides, keeping it Modern-positive only when2026-07-28is still advertised. This mirrors go-sdk's own reference client (mcp/client.go), whose test for the case is named "unsupported protocol version falls back to initialize".-32020/-32021remain unconditionally Modern — both require the peer to have parsed Modern_meta.probeRevisionwas transport-blind.TransportType == "sse"names the deprecated 2024-11-05 two-endpoint transport, whoseBaseURLis the GET-only/ssepath (pkg/transport/url.go:56-57,httpsse/http_proxy.go:274-278), andmodernCallis a single-endpoint POST — so no Modern endpoint is reachable there. It now classifies Legacy without probing, which also avoids POSTing into a stream and hanging to the client timeout (errModernTransientis uncached, so that cost recurred on every call).The rest is comment accuracy and test coverage. Several comments contradicted the code after the v1.7 bump, which
.claude/rules/go-style.md("Keep Comments Synchronized With Code") calls out as worse than no comment — including five that justified the hand-rolled Modern shim on the grounds that go-sdk v1.6.1 cannot express the Modern wire shape, the premise #5993 invalidates.Type of change
Test plan
task test) — targeted runs of./pkg/vmcp/client/...,./pkg/vmcp/session/...,./pkg/mcp/..., all greentask test-e2e)task lint-fix) — 0 issuesNew coverage:
TestProbeRevision_SSEGate(handler fails the test if hit, proving no network call), both-32022branches inTestProbeRevision_TruthTable, twoisRevisionMismatchtruth-table rows, andTestHTTPSession_CallTool_StripsReservedModernMetafor the previously untested session-path strip site.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Changes
pkg/vmcp/client/modern.goclassifyUnsupportedProtocolVersiondecodesdata.supported;Dataadded tomodernRPCError;-32022dropped fromisModernProtocolCodepkg/vmcp/client/client.goprobeRevision; revision-cache and TRIPWIRE docs correctedpkg/vmcp/client/revision_test.go-32022split into with/withoutdata.supportedpkg/vmcp/server/{modern_envelope,modern_dispatch,serve}.gov1.6.1shim rationales correctedpkg/mcp/parser_integration_test.gopkg/vmcp/client/reclassify_test.goerrModernNegotiatedDowntruth-table rows; staleness doc amendedpkg/vmcp/session/internal/backend/*_test.gometaFor→toolCallMetapkg/vmcp/client/{client_test,schema_ingestion_regression_test}.goDoes this introduce a user-facing change?
Yes. A backend that rejects our Modern probe with
-32022, and any backend on thessetransport, is now correctly driven as Legacy (2025-11-25). Previously a-32022backend was driven Modern and advertised no tools, resources, or prompts — so affected backends go from silently empty to working.Special notes for reviewers
-32022.errModernNegotiatedDownfeedsisRevisionMismatch, sodispatchreclassifies and retries — and unlike the discover source, this one can fire on a side-effectingtools/call. It is safe: go-sdk'sServerSession.handlereturns-32022before theswitch req.Methodthat dispatches to a handler, so the backend provably never executed the request. The sentinel's doc previously justified this as "both call sites are read-only", which is no longer true; it now states the real reason.>= 2026-07-28; we exact-matchMCPVersionModern, because vMCP's shim speaks exactly one Modern wire shape. Safe today (a dual-era peer also lists2025-11-25to fall back to), and this is now the second exact-match site, so the existing TRIPWIRE note names both.parser_integration_test.goobserves Modern negotiation over ansse-labelled endpoint whileprobeRevisionnow asserts that can never happen. Different layers: mcpcompat's SSE client reaches/messages, whereas vMCP'smodernCallonly ever POSTs toBaseURL, which is/sse.mcpRevisionCRD status field (vMCP backend revision cache has no success/TTL re-probe (stale on stateless redeploy) #5992). A promising in-band fix exists —initializeClientalready receivesInitializeResult.ProtocolVersionand discards it. Also pending: possible missinglogLevelreserved_metakey,Mcp-Namesentinel encoding,x-mcp-headermirroring, and arch docs for the whole dual-revision subsystem (still undocumented).Generated with Claude Code