Skip to content

Wire MCP 2026-07-28 revision classification into vMCP - #5913

Merged
jhrozek merged 7 commits into
stacklok:mainfrom
jhrozek:vmcp-stateless-classify-decode-seam
Jul 23, 2026
Merged

Wire MCP 2026-07-28 revision classification into vMCP#5913
jhrozek merged 7 commits into
stacklok:mainfrom
jhrozek:vmcp-stateless-classify-decode-seam

Conversation

@jhrozek

@jhrozek jhrozek commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Part of #5756 (RFC THV-0082/0083, Phase 1). Before vMCP can route Modern
(2026-07-28) requests differently in a later phase, it needs to recognize
them at the decode seam and reject malformed Modern requests with the
correct JSON-RPC error — without changing anything about how Legacy
traffic is handled today.

  • Wire the already-shipped mcp.ClassifyRevision/mcp.WriteClassificationError
    into a new classificationMiddleware at the vMCP decode seam
    (pkg/vmcp/server/classification.go), composed into the handler chain in
    pkg/vmcp/server/server.go.
  • Add mcp.ValidateHeaderConsistency (pkg/mcp/revision.go) to validate the
    Mcp-Method/Mcp-Name request headers against the decoded body, including
    base64-sentinel decoding for Mcp-Name (=?base64?<payload>?= per the
    draft spec) — this didn't exist anywhere in the repo before this PR.
  • Telemetry runs before classification in the chain so a rejected request is
    still recorded (metrics/traces), not silently dropped.

No routing/dispatch change: Legacy and well-formed Modern requests both fall
through to the same handler unchanged. server/discover remains
unallow-listed in the authz layer, untouched by this PR.

Closes #5909

Type of change

  • New feature (non-breaking change which adds functionality)

Test plan

  • Unit tests added/updated (pkg/mcp/revision_test.go,
    pkg/vmcp/server/classification_test.go)
  • Integration test added (TestIntegration_RealBackend_ModernRequestRejectedByClassification
    in session_management_realbackend_integration_test.go) proving the
    wiring end-to-end through the real handler chain, not just the
    classification middleware in isolation
  • Manually verified the classifier truth table (Legacy passthrough,
    Modern header+_meta passthrough, each error code) via the new tests

Special notes for reviewers

  • RequestHeaderMismatchError reuses the existing CodeHeaderMismatch
    (-32020) constant rather than introducing a new code, per the draft spec's
    "Server Validation" section, which mandates -32020 for header/body
    validation failures generally (not just MCP-Protocol-Version).
  • This slice deliberately does not enforce presence of Mcp-Method/Mcp-Name
    on Modern requests, only consistency when present — the classifier has a
    pre-existing TODO (pkg/mcp/revision.go) about not yet being able to tell
    stdio's "no header concept" apart from HTTP's "header omitted"; the new
    code cross-references that TODO rather than presenting as complete.
  • The classified Revision is deliberately not stashed anywhere (no new
    telemetry label, no context plumbing) since nothing downstream reads it
    yet — that lands with actual Modern dispatch in a later phase.

Generated with Claude Code

jhrozek and others added 4 commits July 22, 2026 13:06
vMCP's stateless decode seam needs to reject Modern (2026-07-28)
requests whose Mcp-Method/Mcp-Name headers contradict the parsed
JSON-RPC body, per the draft spec's Server Validation rules. Add
ValidateHeaderConsistency alongside the existing ClassifyRevision,
reusing the -32020 HeaderMismatch code and decoding the draft spec's
base64 sentinel wrapper for Mcp-Name before comparing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Introduce classificationMiddleware to reject malformed Modern
(2026-07-28) requests at the vMCP decode seam using the existing
ClassifyRevision/ValidateHeaderConsistency helpers, ahead of wiring
it into the server's middleware chain. Not yet composed into
server.go: Legacy traffic is unaffected until that follow-up step.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Compose classificationMiddleware between MCP-parsing and telemetry so
malformed Modern (2026-07-28) requests are rejected before dispatch,
while Legacy traffic and well-formed Modern requests fall through
unchanged. Add an end-to-end test through the real handler chain
covering the wiring itself, complementing the unit-level table added
alongside the middleware.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Classification previously wrapped telemetry, so a rejected Modern
request short-circuited before telemetry ever ran, silently dropping
request/error metrics and traces for exactly the requests worth
observing. Reorder so telemetry wraps classification instead: it
still sees the parsed MCP context and now also records the outcome
of rejected requests.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added the size/M Medium PR: 300-599 lines changed label Jul 22, 2026

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid, well-scoped wiring — and the sentinel decode is done right. A three-lens panel (MCP-conformance · security · Go/reuse) reviewed it against 35cd727; it's close to approvable, with one conformance point worth your call before I do.

Security — safe (no blockers). The middleware is purely additive rejection: it can only reject more traffic, never authorize or route more. Verified there's no confused-deputy — both authz (CallGate) and dispatch key on the body-derived Method/ResourceID, so the header-consistency check is defense-in-depth, not the sole guard. The base64-sentinel decode fails closed (decode error → reject), can't panic, and is header-size-bounded (no DoS). Auth stays outermost; classification is innermost. No secret logging.

MCP conformance — mostly correct, one gap. The =?base64?...?= sentinel format, case-sensitive lowercase markers, standard-padded base64, -32020, case-sensitive compare, and body-field targets (params.name/params.uri) all match the draft spec. MAJOR (inline on classification.go:39): presence of the required Mcp-Method/Mcp-Name isn't enforced for Modern HTTP — only consistency-when-present — so a Modern request omitting them is dispatched rather than -32020'd, and the deferral's stated rationale (stdio/HTTP ambiguity) doesn't hold at this HTTP-only seam. Not a security hole (authz keys on body), but a real conformance gap; gating on the classified RevisionModern is the natural fix (and also resolves the Legacy-runs-too MINOR below).

Go / reuse — clean. Good call not using mime.WordDecoder — RFC 2047 is =?charset?encoding?text?= (3 fields) while the draft sentinel is =?base64?payload?= (2 fields), so the stdlib decoder would misparse it; the hand-rolled base64.StdEncoding + trims is correct (edge cases verified). Reuses ClassifyRevision + WriteClassificationError + the context-stashed ParsedMCPRequest (no re-parse), and RequestHeaderMismatchError follows the existing typed-CodedError pattern with the keep-in-sync -32020 comment.

🟡 Minor / test coverage

  • Consistency check runs on Legacy too (classification.go:34-39) — unconditional after the discarded classification; gate on RevisionModern (folds into the inline fix).
  • The new header-consistency/sentinel path isn't exercised end-to-end — the unit test injects a pre-built ParsedMCPRequest (bypassing ParsingMiddleware), and the integration test covers only the proto-version mismatch, not ValidateHeaderConsistency. Add one integration case with a real Mcp-Method: resources/read header on a tools/call body so the ParsingMiddleware → classificationMiddleware → -32020 flow is actually proven.
  • Header consistency is enforced at only 1 of the 3 classifier seams (vMCP here; streamable/transparent run ClassifyRevision but don't validate the headers, since they don't populate MCPMethodHeader/MCPNameHeader). Defensible for scope — worth a note / follow-up so the divergence is intentional.
  • Telemetry-before-classification ordering is untested (server.go:624) — it's the reason the middleware sits where it does; a guard test would lock it.

⚪ Nits

  • classification_test.go:26 re-declares "2026-07-28" — reuse the exported mcp.MCPVersionModern.
  • Malformed-sentinel is reported as a value "mismatch" (wire code -32020 is correct; cosmetic message).

Everything here is addressable without a redesign; the only thing I'd want resolved before approving is the MAJOR (enforce presence for Modern, or correct the rationale + track it). Nice work on the sentinel decoding in particular.

🤖 AI-assisted panel review via Claude Code (MCP-spec · security · Go/reuse). Line numbers verified against 35cd727.

Comment thread pkg/vmcp/server/classification.go Outdated
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 22, 2026
Address review feedback: ValidateHeaderConsistency ran unconditionally
(even for Legacy requests) and only checked header/body consistency
when a header was present, never rejecting a Modern request for
omitting a required one. The classifier's stdio-vs-HTTP ambiguity
excuse for deferring this never applied here, since these headers are
only ever populated from real HTTP headers.

Gate the check on the classified revision, require Mcp-Method on
every Modern request and Mcp-Name on tools/call, resources/read, and
prompts/get, and give missing/malformed/mismatched headers distinct
error messages (same -32020 wire code throughout). Add an end-to-end
Mcp-Method mismatch test and a regression test locking in that
telemetry still records requests classification rejects.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 22, 2026
@jhrozek

jhrozek commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Addressed all of this in a4552f8 (pushed). Presence is now enforced for Modern requests, Mcp-Method always required, Mcp-Name for tools/call/resources/read/prompts/get, gated on the classified revision so Legacy is exempt from the whole check. Added an end-to-end test for a real Mcp-Method mismatch through the full ParsingMiddleware -> classificationMiddleware chain, plus a regression test locking in that telemetry still records requests classification rejects. Also cleaned up the two nits: classification_test.go reuses mcp.MCPVersionModern instead of a local constant now, and the error message distinguishes missing/malformed/mismatch instead of always saying "does not match" (wire code stays -32020 in all three cases). Thanks for the thorough pass, the point about the stdio excuse not applying at an HTTP-only seam was a good catch.

golangci-lint's exhaustive check flagged the missing explicit case
for headerMismatchReasonValue, which in turn made the constant look
unused since it was only ever reached via default.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Jul 22, 2026
@codecov

codecov Bot commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.98246% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.67%. Comparing base (fbf2f6d) to head (98b76d4).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
pkg/mcp/revision.go 90.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5913      +/-   ##
==========================================
+ Coverage   71.63%   71.67%   +0.04%     
==========================================
  Files         698      701       +3     
  Lines       71541    71980     +439     
==========================================
+ Hits        51245    51591     +346     
- Misses      16598    16692      +94     
+ Partials     3698     3697       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jhrozek
jhrozek merged commit e8d243d into stacklok:main Jul 23, 2026
74 of 76 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Large PR: 600-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wire MCP 2026-07-28 revision classification into the vMCP client edge

2 participants