Skip logging/setLevel on a Modern-negotiated session - #6184
Conversation
jhrozek
left a comment
There was a problem hiding this comment.
The failing Tests / Test Go Code check isn't a flake, it's a real, deterministic conflict with an existing test.
TestIntegration_Modern_RealBackend_LoggingContract in pkg/vmcp/server/modern_realbackend_integration_test.go (around lines 398-434) pins the exact contract this PR replaces. Its doc comment and both subtests assert that a well-formed Modern logging/setLevel should be rejected as method-not-found (404 + -32601 from dispatchModern), and a malformed one should get 400 + -32020 (HeaderMismatch).
Now that ClassifyRevision treats logging/setLevel as unconditionally Legacy, neither request reaches those code paths anymore. The well-formed case gets routed as Legacy and 400s with a plain-text "protocol version 2026-07-28 is only supported on stateless HTTP servers" body instead of the expected JSON-RPC error, and the malformed case fails at json.Unmarshal for the same reason (the response isn't JSON).
This test (and its doc comment, which explicitly documents the now-superseded contract) needs to be updated to reflect the new Legacy classification before this can merge. Re-running CI won't help since the failure is deterministic, not timing-related.
vMCP tool calls against a dual-era stdio backend failed while the gateway reported healthy. github-mcp-server v1.6.0 negotiates 2026-07-28 over the LEGACY initialize handshake, so the session must then carry that version on every request. go-sdk still sends logging/setLevel as a Legacy-shaped call, producing a Modern protocol header with no Modern _meta -- a shape the classifier rejects with -32020. go-sdk treats that as fatal, closing the session and killing the tool call the logging was only meant to decorate. The 2026-07-28 revision REMOVED logging/setLevel; the per-request io.modelcontextprotocol/logLevel _meta key replaces it, and the Modern path already mints it. Calling an RPC the negotiated revision removed is the defect, so skip it there. Sessions that negotiate a Legacy version still get it. Rejecting the malformed shape is deliberate and pinned by TestIntegration_Modern_RealBackend_LoggingContract, whose comment records that go-sdk closed the upstream report wont-fix-by-design. Loosening the classifier instead would have broken that contract; what the contract's "worse than the failing upstream call" tradeoff did not anticipate is that the rejection is fatal to the whole session, not just to the logging call. legacyInit now returns the negotiated version so the call site can make this decision; the four callers that do not need it discard it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0e99a07 to
4145394
Compare
…methods # Conflicts: # pkg/vmcp/client/revision_realbackend_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6184 +/- ##
=======================================
Coverage 72.46% 72.47%
=======================================
Files 739 739
Lines 76719 76723 +4
=======================================
+ Hits 55594 55602 +8
+ Misses 17160 17155 -5
- Partials 3965 3966 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
vMCP tool calls against a dual-era stdio backend fail while the gateway reports healthy. Reproduced against
github-mcp-serverv1.6.0 over stdio:The chain:
github-mcp-serverv1.6.0 negotiates2026-07-28over the Legacyinitializehandshake, so the session must carry that version on every subsequent request.logging/setLevelwas removed in 2026-07-28 — the per-requestio.modelcontextprotocol/logLevel_metakey replaces it, and the Modern path already mints it (modern.go).setLevelas a Legacy-shaped call, so it arrives with a Modern protocol header and no Modern_meta.-32020, and go-sdk treats the rejection as fatal — the session closes and the tool call dies with it.Skip the RPC when the negotiated version is Modern. Sessions that negotiate a Legacy version still get it.
legacyInitnow returns the negotiated version so the call site can decide; the four callers that do not need it discard it.Type of change
Test plan
task test)task lint-fix)TestEnableBackendLogging_SkipsRemovedRPCOnModernSessionis table-driven over both negotiated versions and asserts against a recording backend: Modern → not sent, Legacy → still sent. Verified to fail without the production change (expected: false, actual: trueon the Modern case).End-to-end A/B, real
tools/callthrough a vMCP gateway togithub-mcp-serverv1.6.0 over stdio:tools/callorigin/mainclient is closing: sending "logging/setLevel"Full
task testrun: only the 10 pre-existingpkg/plugins/pluginsvcfailures, which reproduce identically on cleanorigin/main.pkg/vmcp/server— includingTestIntegration_Modern_RealBackend_LoggingContract— is green.Does this introduce a user-facing change?
Yes. vMCP can call tools on dual-era stdio backends again,
github-mcp-serverv1.6.0 among them. Before this, such a gateway reportedReadywith every health check passing and still failed every tool call.Special notes for reviewers
This PR changed approach in place (force-pushed) rather than opening a new one, so the review history stays here. It previously classified
logging/setLevelas Legacy-only inClassifyRevision; it now leaves the classifier untouched and fixes the caller.Why not loosen the classifier. The original approach classified
logging/setLevelas Legacy-only. @jhrozek correctly caught that it breaksTestIntegration_Modern_RealBackend_LoggingContract, whose comment records a deliberate contract: go-sdk closed the upstream report wont-fix-by-design, so-32020is the standing answer for a caller using a removed RPC on a Modern session. That contract is right and this change leaves it intact. What it did not anticipate is the blast radius — its "worse than the failing upstream call" tradeoff assumed only the logging call fails, but the rejection closes the session and takes the tool call with it. Fixing the caller rather than the classifier resolves that without weakening the contract.Why this hid behind green health checks.
enableBackendLoggingis called only fromlegacyCallTool— not fromListCapabilities, resources or prompts. Health checks andtools/listnever sendsetLevel, so they stayed green while every tool call died. Worth knowing when reading vMCP health status: it does not exercise the tool-call path.Related, and complementary: #6188. That report hits the same
-32020rejection from a different angle — the ChatGPT connector sendstools/callwith a LegacyMCP-Protocol-Version: 2025-11-25header plus a reserved_metasignal key and no_metaprotocolVersion. I verified both shapes against the classifier directly:The two need different fixes and neither subsumes the other. #6188's proposal keys off an explicitly non-Modern header, so it cannot help this case (the header here is Modern); and this caller-side skip cannot help theirs, since
tools/callhas a Modern counterpart and can never be treated as Legacy-only. #6188's own analysis of that is correct.Regression window. The strict classifier shipped in v0.41.0 (#5839/#5884); v0.40.1's proxy answers the same request
{"result":{}}. Verified by curling both builds directly.Generated with Claude Code