Bridge era-mismatched client×backend cells in vMCP

[jhrozek]

Summary

vMCP classifies the client edge per request and each backend edge independently, so a client on
either MCP revision can reach backends on either revision. RFC-0081 marked the two era-mismatched
combinations "Fails" for a pass-through proxy and deferred them here, because vMCP is the component
that terminates the client connection and re-originates backend calls.

Investigating those two cells first changed what this PR needed to be. Both were already
largely functional after #5910/#5911, and their difficulty is inverted from how #5912 describes it:

• Modern client → Legacy backend (the issue's "hard cell", expected to need a synthesized
  session held per request and partitioned by (principal, backend)) already holds by
  construction. legacyCallTool opens a client, initializes, calls and closes per request, so
  nothing is pooled and no credential can bleed. There was nothing to build — only an undefended
  invariant to pin.
• Legacy client → Modern backend (the "easy cell") had the one real defect: the session factory
  attempted a connect that cannot succeed.

So the work is one small behaviour fix, one additive status field, and the coverage that makes all
four cells falsifiable.

• Skip a connect that cannot succeed. Measured, not assumed: go-sdk v1.7's Connect is
  Modern-first, so server/discover succeeds and no Legacy initialize is ever sent — but
  mcpcompat's Initialize unconditionally installs the list-changed handlers, so go-sdk opens a
  subscriptions/listen stream, which a session-less backend answers session not found. That
  fails Connect, tears the connection down, and fails the follow-up tools/list. Every session
  paid a discover + failed subscribe + failed list + teardown per Modern backend, ending in a WARN
  indistinguishable from a dead backend. Note the causality: list_changed is one of the three
  reasons those connections exist, and it is precisely what breaks the connect — vMCP was asking
  for a notification stream the backend cannot give it.
• Surface each backend's negotiated revision on /status. The operator already published it via
  VirtualMCPServer.status.discoveredBackends[].mcpRevision; the HTTP endpoint did not, so no e2e
  tier could assert which era a backend negotiated.
• Pin the bridge's confidentiality invariants so RFC-0083 D6's planned shared connection pool
  cannot break them silently.
• Live e2e for all four cells, at both the CLI and operator tiers.

Closes #5912
Closes #5979

Type of change

• Other (describe): completes a feature — one behaviour fix, one additive /status field, and the live dual-era e2e matrix

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e) — LABEL_FILTER="vmcp && dual-era", 6/6 green
• Linting (task lint-fix)
• Manual testing (described below)

Manual verification beyond the suites:

• Every new assertion was mutation-verified — deliberately broken, observed to fail, restored
  byte-identically. Including two that revealed the assertion itself was wrong:
  • Cell B's first fingerprint ("no request with an empty Mcp-Method") passed with the production
    skip removed, because go-sdk v1.7's client is Modern-first and never sends a raw Legacy
    initialize. Re-based on the presence of subscriptions/listen, which only a full SDK client
    handshake emits.
  • Cell A's _meta pin asserted only that reserved keys were absent, which a blanket _meta
    drop would also satisfy — silently killing progressToken and trace context. Now also asserts a
    non-reserved key survives the same hop.
• The environment-independence of the kill-switch specs was proven both ways: with
  TOOLHIVE_VMCP_MODERN_STATELESS=true exported, the pre-fix helper leaked it and exactly the
  kill-switch spec failed; with the fix, 6/6.
• The Modern probe was verified by hand through the thv proxy before any spec was written
  (server/discover → resultType:"complete", supportedVersions:["2026-07-28","2025-11-25"]),
  because the whole suite rests on that hop working.

API Compatibility

• This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

No operator API surface is touched — no CRD, no api/v1* type. The /status change is additive on
the vMCP HTTP endpoint (mcp_revision, omitempty), so existing consumers see byte-identical JSON
whenever health monitoring is disabled.

Changes

File	Change
pkg/vmcp/session/factory.go	Skip the backend connect for a backend already classified Modern; suppress the all-failed warning when every backend was skipped that way
pkg/vmcp/cli/serve.go	Wire the revision lookup into the session factory at the composition root
pkg/vmcp/server/status.go	Add BackendStatus.MCPRevision, populated from the live health state
pkg/vmcp/server/bridge_regression_test.go	New — pins both bridge cells' confidentiality invariants
pkg/vmcp/session/factory_revision_test.go	New — the skip decision and the post-error re-check
pkg/vmcp/server/status_test.go	Coverage for all three mcp_revision states
test/e2e/vmcp_dual_era_test.go, ..._helpers_test.go	New — live four-cell matrix, CLI tier
test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_backends_test.go	New — mixed-era operator tier (#5979)
test/e2e/thv-operator/virtualmcp/helpers.go	One additive field on the VMCPBackendStatus test mirror, so it stops drifting from server.BackendStatus
docs/arch/10-virtual-mcp-architecture.md	Record that Modern backends hold no per-session connection, and why that is correct rather than a gap

Does this introduce a user-facing change?

Yes. The vMCP /status endpoint's backends[] entries now carry mcp_revision — the backend's
negotiated MCP protocol revision (2025-11-25 or 2026-07-28). It is omitempty and is populated
only from live health state, so it is absent when health monitoring is disabled or a backend has not
been probed yet. The endpoint is unauthenticated by design, so this is a real (if small) widening of what an
unauthenticated caller learns: which MCP revision each backend negotiated. It is a protocol version,
not a secret, and the same value is also published on VirtualMCPServer.status.discoveredBackends[]
— but that is behind cluster RBAC, a different trust boundary, so treat the two as separate
disclosures rather than one. Deployments that care should restrict /status by network policy, as
its own doc comment already advises.

Special notes for reviewers

Two assertions carry most of the value, and both exist because a naive version passed vacuously.

1. BeforeEach gates every CLI-tier spec on /status reporting 2026-07-28 for the stateless
   backend and 2025-11-25 for the other. Without it the 2×2 matrix can silently collapse to 1×2:
   a regressed server/discover probe would classify the stateless backend Legacy and all six
   specs would still pass while testing the Legacy backend edge twice. This is not hypothetical —
   it caught exactly that during a rebase, when Unify e2e yardstick backend on 1.2.0 #6004 changed which yardstick image is Modern-capable.
2. Cell A correlates a per-request nonce (carried in tool arguments) against the credential the
   backend observed. Counting distinct backend sessions is insufficient: under a symmetric swap of
   two principals' credentials the count stays correct. The two channels are independent, so a swap
   makes them diverge.

Health monitoring is opt-in, and that is the sharpest edge in this feature. mcp_revision is
observable only through live health state, and vMCP only starts the monitor when
operational.failureHandling.healthCheckInterval > 0. Both e2e tiers must enable it explicitly;
with the section absent the status payload carries no backends at all and the revision assertions
time out. unhealthyThreshold is mandatory once the interval is set, or the vmcp pod fails at
startup. This bit the work four separate times and is the strongest argument for the TTL re-probe
tracked in #5992.

Known limitations, deliberate:

• The operator-tier spec has not been run against a cluster. It is authored for
  test-e2e-lifecycle.yml and verified by compile + go vet + lint, following the precedent set by
  Add live e2e tests for dual-era stateless proxy behavior #5981's own k8s tier. The one assumption no existing test covers is a yardstick backend inside a
  discovered-mode vMCP group alongside a Legacy backend; if it fails, the "aggregates tools from
  both backends" It times out first.
• The raw e2e client cannot parse SSE, so the CLI-tier bridge is proven only under a non-conformant
  Accept header (recorded in the file). The k8s tier opts into the full header.
• The stale-revision-cache window (vMCP backend revision cache has no success/TTL re-probe (stale on stateless redeploy) #5992) is documented in initOneBackend rather than fixed: a
  backend redeployed stateless→stateful keeps a stale Modern label until a call re-probes.

Follow-ups this unblocks:

• Strip reserved io.modelcontextprotocol/* keys from backend response _meta (shared Legacy+Modern helper) #5986 (strip reserved keys from backend response _meta) is deliberately untouched — this PR
  asserts nothing about response _meta so the two cannot conflict. One trap for whoever takes it:
  conversion.ToMCPMeta is bidirectional, called on both the response path and outbound requests,
  so the strip belongs at the response call sites rather than inside it.
• The two specs Add live e2e tests for dual-era stateless proxy behavior #5981 removed pending this bridge (mixed-era over a shared Redis session store, and
  Redis-outage → 503) are now unblocked. Neither tier here configures Redis, so that coverage is
  still outstanding — the operator-tier file is the natural place to grow it.
• Separately, with the Modern kill switch off, a well-formed Modern request currently receives
  go-sdk's plain-text 400 rather than the -32022 + supported-versions body the draft mandates. A
  fix is ready on a follow-up branch, and the underlying go-sdk defect has been written up for
  upstream.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 92.15686% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.19%. Comparing base (0a9e32a) to head (f547955).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/vmcp/cli/serve.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6006      +/-   ##
==========================================
- Coverage   72.21%   72.19%   -0.02%     
==========================================
  Files         721      721              
  Lines       74987    75021      +34     
==========================================
+ Hits        54152    54162      +10     
- Misses      16956    16982      +26     
+ Partials     3879     3877       -2     

☔ 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.

[JAORMX]

Panel review — dual-era bridge (Closes #5912, #5979)

I ran a multi-axis review (MCP-protocol conformance, security/confidentiality, Go correctness & architecture, test/e2e quality), reading the change against the four-cell matrix defined in #5912, its parent #5756, and the 2026-07-28 draft spec. Short version: this is a genuinely strong PR — the production change is small, correct, and spec-justified, and the confidentiality invariants are faithfully pinned. No blocking correctness or security defect found on any axis. The findings below are one CI fix, a handful of comment/message precision nits, and honest coverage-boundary notes. Inline comments mark the specific lines.

🔴 Must-fix before merge

• CI is red on Spellcheck / Codespell → bridge_regression_test.go:609 (re-declared → redeclared). One character. (Unit + 3 k8s E2E-lifecycle jobs were still running at review time; everything else green.)

🟡 Suggestions (non-blocking)

1. Type-assertion silent-degradation foot-gun + four duplicate revisionReporter decls (serve.go:371). Correctly kept out of the option, but if the concrete client or a future wrapper drops CachedRevision, the Modern-skip silently disappears with no compile error. Suggest one exported RevisionReporter + a co-located var _ RevisionReporter = (*httpBackendClient)(nil) compile-time assertion. Runtime-pinned at bridge_regression_test.go:748, so not a blocker.
2. "All backends failed to initialise; session will have no capabilities" (factory.go:478) now overstates the mixed skip+fail case, and the "no capabilities" half is misleading given caps come from the core.
3. initOneBackend "session not found" wording (factory.go:232) implies the Modern spec requires a session; it's a go-sdk v1.7 pre-release artifact. Reword so it doesn't misinform.
4. /status mcp_revision justification (status.go:44): the "already public via CRD" / "transport is a finer-grained fingerprint" reasoning is imprecise (different trust boundary; orthogonal dimension). Exposure itself is negligible — just don't carry the "no new exposure class" framing forward.

Axis notes

MCP-protocol — clean. Every wire-level detail checks out against the draft: server/discover as the Modern entry point, initialize/session removal, subscriptions/listen as the sole Modern server-push channel (so "list_changed is Legacy-only" is factually accurate, not a hand-wave), the exact ReservedModernMetaKeys set (with progressToken/trace-context correctly surviving the strip), the revision strings, and the -32020/-32021/-32022 error shapes. Two wording items only (#3 above; and the deferred kill-switch-off -32022 path — spec-MUST confirmed, deferral reasonable, but the follow-up should call out that a Modern-only client with no initialize fallback is stranded by the plain-400).

Security — sound, no HIGH/CRITICAL. The #5912 confidentiality requirement (per-(principal, backend) session, calling principal's own credential, no bleed) holds by construction in legacyCallTool (fresh client per request, defer Close(), per-request identity off the live context; no shared credential state), and the regression test genuinely pins it — the symmetric-swap credential↔session correlation is a rigorous defense the cardinality check alone would miss. The out-of-scope token-exchange cache is independently keyed by {backend}:{hash(subject)}:{audience}, so the exclusion is legitimate. Skipping the Modern connect bypasses no data-plane security control (auth Validate, same-host-redirect/SSRF guard, TLS, header-restriction all re-run per call). One note: the Cell A test pins per-request creation, which is stricter than per-principal — deliberate as a tripwire for RFC-0083 D6's shared pool, but that test must be consciously re-baselined when D6 lands.

Go / architecture — no correctness bug. Concurrency is race-free (per-index writes, wg.Wait before reads); the slices.Contains(modernSkipped, false) suppression is logically correct across all cases; the skipped Modern backend's tools are still advertised/routed via the core aggregator (verified through server.New → AdvertiseFromCore → core.CallTool); the double isKnownModern is a legitimate cache-warmed re-check, not the SDK-lifecycle anti-pattern; correctly used a typed-func option instead of widening vmcp.BackendClient (avoids anti-patterns #8/#9). The initOneBackend doc block is exemplary.

Tests — rigorous unit/integration tier; honest coverage boundaries. The Cell B subscriptions/listen fingerprint with its self-validating positive-control sibling is the strongest-designed test here, and the Cell A _meta-survives assertion is a genuine (non-vacuous) positive control. The CLI-tier waitForBackendRevision BeforeEach guard really is load-bearing — it's the only thing stopping the 2×2 matrix silently collapsing to 1×2, and it works. Test-rule compliance (parallel t.Cleanup, timed barriers, no assert-in-goroutine, DEBUG-level log recorder) is clean. Boundaries worth stating plainly:

• Concurrency covers only the two same-era cells; the bridge cells aren't exercised concurrently — Cell B (Legacy→Modern) has no concurrency coverage at any tier (vmcp_dual_era_test.go:155, inline).
• Operator tier has never run against a cluster — compiles/vets clean, but stateless-Modern-inside-discovered-mode-aggregation is a first; watch the readiness/BackendsDiscovered coupling vs the deliberate cold-cache connect skip.
• CLI tier proves the bridge only under a non-conformant Accept header (no SSE parser); the conformant path's intended compensation is the operator tier — which is the unrun one. So no executed test proves conformant-Accept bridging.
• No Redis / cross-pod RestoreSession mixed-era coverage, and the #5992 stale-cache window is documented-not-fixed. Reasonable to defer; largest untested surface the issues imply.

Nice work overall — the documentation-of-intent throughout (especially the SDK failure-chain and the falsifiability guards) is a model for this kind of change.

Reviewed with a multi-agent panel (protocol/security/Go/test axes); findings verified against the code before posting.

[JAORMX]

Approving. Multi-axis review (MCP-protocol, security/confidentiality, Go correctness/architecture, tests) found no blocking correctness or security defect — the confidentiality invariants of the bridge cells hold by construction and are faithfully pinned, the Modern-connect skip is spec-justified and bypasses no data-plane security control, and CI is green on the checks that exercise this change (E2E Core (vmcp) + all three Operator CI tiers, which retires the "operator tier never run against a cluster" concern).

Everything in my earlier review is non-blocking — please treat the following as follow-up rather than merge-blockers:

• The revisionReporter type-assertion silent-degradation foot-gun + four duplicate declarations → the one hardening item I'd genuinely like folded in (an exported interface + a co-located compile-time assertion), but fine to defer.
• Cell B (Legacy→Modern) has no concurrency coverage at any tier — worth a follow-up given #5912's cross-delivery framing.
• Minor comment/message precision nits (the factory.go warn text, the "session not found" wording, the /status exposure justification).

Nice work — the documentation-of-intent throughout (the SDK failure-chain, the falsifiability guards, the disclosed coverage boundaries) is a model for this kind of change.

[jhrozek]

Thanks — this was a genuinely useful review, and two of its findings changed the code rather than just the comments. All items addressed; details below, including two where I disagreed or where the finding turned out to be already-resolved.

🔴 Must-fix

Codespell re-declared → redeclared — fixed. Codespell is green. I installed codespell locally to find it rather than guess; worth noting that 2.4.3 is stricter than the pinned action and also flags two pre-existing pre-selected hits in pkg/tui/update_navigation.go and pkg/oauthproto/tokenexchange/middleware.go. I left those alone as out-of-scope for this PR.

🟡 Suggestions

1. Type-assertion foot-gun + four duplicate revisionReporter decls — done, and you were right where I was wrong.

I had kept the duplicates on the reasoning that an exported interface would create a cycle. That was only true for exporting from pkg/vmcp/client (which imports backendtelemetry) — not for pkg/vmcp, which all four consumers already import and which imports none of them. So there is now one exported vmcp.RevisionReporter, the three local declarations are deleted, and the implementation is pinned with var _ vmcp.RevisionReporter = (*httpBackendClient)(nil) next to the method.

The property you cared about is preserved: it stays off vmcp.BackendClient, so no implementation or mock is forced to satisfy it. One consequence worth flagging — types.go carries a mockgen -source=types.go directive, so this generates a standalone MockRevisionReporter (regenerated and committed). MockBackendClient is untouched.

2. "All backends failed to initialise; session will have no capabilities" — reworded. You were right that it was false as well as overstated: on the Serve path capabilities come from the core, so a session holding zero connections still lists and calls tools normally. What is actually lost is list_changed propagation, and the message and comment now say that. The test pinning the old string was re-pinned to the stable prefix, with a note that the NotContains half is only meaningful while the substring still matches.

3. initOneBackend "session not found" wording — fixed. It now states explicitly that the rejection is a go-sdk v1.7.0-pre.3 artifact and not a spec requirement, since the Modern revision has no sessions at all.

4. /status exposure justification — corrected in the PR description. Your objection was right: the CRD status is behind cluster RBAC, so it is a different trust boundary and not the same disclosure, and transport is an orthogonal dimension rather than a superset. The description now says plainly that this is a real if small widening for an unauthenticated caller, and points at the endpoint's existing network-policy advice.

Axis notes

Cell B concurrency — fixed here rather than deferred. You were precisely right, and it was worse than "no coverage at any tier": the existing concurrency spec races the two same-era cells (Legacy client→Legacy backend, Modern client→Modern backend), so the off-diagonal — the entire point of #5912 — was never concurrent. Added a spec that crosses the tool names, so a Legacy session calls the Modern backend while a stateless Modern client calls the Legacy backend, concurrently, with distinct principals.

That is the configuration where cross-delivery matters most, because the two bridge cells re-originate through different egress paths (stateless modernCall vs. per-request initialize+call+close). Racing two requests down the same path, which is all the old spec did, is a weaker test. The helper already took both tool names as parameters, so this needed no new machinery — only a rename to legacyClientTool/modernClientTool, since they mean "the tool that client calls" rather than "the tool of that era".

Modern-only client stranded by the plain-400 — already resolved, no follow-up needed. This was written while the -32022 path was still deferred; it was not deferred in the end. #6009 merged into this branch, so a well-formed Modern request with the kill switch off now gets -32022 with data.supported: ["2025-11-25"] instead of go-sdk's plain-text 400. A Modern-only client still cannot use that server — correctly, since it genuinely does not serve that revision — but it is told so conformantly and told what the server does speak, which is exactly the draft's requirement. I nearly filed an issue for this before re-checking it against the branch.

Operator tier never run against a cluster — now retired, but it did find a real bug first. CI caught the spec crash-looping the vmcp pod: supplying the failureHandling object makes kubebuilder apply its siblings' CRD defaults, including healthCheckTimeout: 10s, which equals the interval I set and fails validation (must be less than healthCheckInterval). The failure mode was maximally unhelpful — a readiness timeout with no hint of the cause — so the timeout is now set explicitly with a comment naming the trap. All three k8s tiers pass, including both mcpRevision assertions.

Conformant-Accept bridging — your note that no executed test proved it was true when written, because the compensating operator tier had not run. It now runs and uses real SDK clients, so that coverage is executed. The raw-client limitation is still documented in the file, but it is no longer uncompensated.

Redis / cross-pod mixed-era — filed as #6008, parented under #5756, including the two specs #5981 removed pending this bridge and the two traps a implementer will otherwise hit (health monitoring must be enabled explicitly; assert each backend's era rather than trusting the env, since #6004 unified on one image where STATELESS decides).

Cell A's per-request strictness — agreed it is stricter than per-principal and deliberately so. Left as-is; the pin's own comment records that it must be consciously re-baselined when D6's pool lands, rather than filing against an issue that does not exist yet.

Rebase note

Rebased onto 0a9e32ab7 before pushing, so this sits on top of #6010, #6012, #6013 and #6016. No
conflicts, despite overlap on pkg/vmcp/client/client.go and docs/arch/10-virtual-mcp-architecture.md.

One coherence fix from that: #6016 added a "Backend MCP Revision Classification" section to the same
arch doc. My paragraph in the list_changed section said "whose cached revision is known Modern"
without saying how it becomes known — it now cross-references #6016's section, which answers exactly
that. The two are complementary, not overlapping (theirs: how a revision is resolved and cached;
mine: what follows for list_changed).

Also worth flagging for whoever reviews next: #6010 ("Pin forwarding tools/call against a
stale-Legacy stateless backend") is hardening the same revision-staleness area that #5992 tracks and
that this PR's initOneBackend comment documents. Adjacent to the skip logic here, so worth reading
together.

Unrelated flakes seen on this PR

Both earlier CI failures turned green on re-run with no code change: forwarding_realbackend_integration_test.go's progress-notification wait, and VirtualMCPServer Redis-Backed Session Sharing. The first has had three deflake attempts already (#5941, #5963, and the raise to 60s under #5962) and has no tracking issue — happy to file one, since a fourth timeout bump would not distinguish a lost notification from slowness.