Skip to content

Design MRTR for vMCP and land the typed input_required seam - #6074

Merged
JAORMX merged 13 commits into
mainfrom
mrtr-modern-vmcp
Jul 28, 2026
Merged

Design MRTR for vMCP and land the typed input_required seam#6074
JAORMX merged 13 commits into
mainfrom
mrtr-modern-vmcp

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

Why. The 2026-07-28 revision removes server-initiated requests entirely; its replacement is client-polled Multi Round-Trip Requests (MRTR, SEP-2322). vMCP forwards elicitation/sampling only over Legacy (2025-11-25) sessionsgrep -ri mrtr over the tree returned nothing before this PR — and the Modern egress shim declares empty clientCapabilities, so a compliant Modern backend that needs input answers -32021 and the call dies. Now that vMCP serves Modern unconditionally (#6033 direction), there is no elicitation or sampling path on the Modern side at all. docs/arch/10-virtual-mcp-architecture.md documents both limitations as deliberate-for-now (its two "elicitation and sampling are unavailable…" sections); #6061 (merged) landed the honest error surface for the gap. This PR is the design that closes the gap and the first (egress) slice of it.

Part of #6059 ("Support MRTR pass-through for Modern backends in vMCP") — deliberately not Closes: #6059 names this PR's slice as its prerequisite ("the egress half — driving and propagating the loop in pkg/vmcp/client. The errModernInputRequired seam is where that begins"), and the relay it asks for — forwarding inputRequests, carrying requestState across the retry, re-issuing to the same backend — is slices 2–3 of the design and remains unbuilt. What this PR lands of #6059: the propagating half of the prerequisite — the typed input_required payload surfaced through the errModernInputRequired seam, extractable by the future relay. What remains: driving the loop (retry params egress), the ingress envelope, and capability mirroring. (Historical note: the design was originally requested in #5759, closed as not-planned before #6059 was filed; #6059 supersedes it.)

What. Two things, deliberately in one PR:

  1. docs/arch/16-vmcp-mrtr.md — the full MRTR design: the request/response flow through vMCP on both edges, per bridge cell; where round state lives and for how long; the workflow-handle format and owner check (RFC-0083 D5, followed verbatim); the sampling disposition; the toolhive-core/go-sdk gap list; and a collision-aware slice sequencing. It defers to doc 10's landed rationale for the current limitations rather than restating it.
  2. Slice 1 of that sequencing — the typed input_required egress seam in pkg/vmcp/pkg/vmcp/client, zero behavior change (details below).

Design in one paragraph. Modern↔Modern rounds are a stateless pass-through: vMCP mirrors the downstream client's per-request clientCapabilities declaration onto the backend call, relays inputRequests values and requestState verbatim (opaque json.RawMessage, never reinterpreted), and the retry — a fresh request per spec, new JSON-RPC id — re-routes deterministically by capability name. Legacy-client↔Modern-backend bridges 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 v1.7's own serverMultiRoundTripMiddleware performs for its handlers. Modern-client↔Legacy-backend stays deliberately unbridged, permanently served by #6061's error contract.

Two convergences with #6059, filed independently of this design, that strengthen both: it rejects the Legacy-backend→Modern-client bridge on the same grounds (a parked call is a session in disguise; Tasks/SEP-2663 is the sanctioned stateful path), and its scope note reaches the same sampling conclusion — SEP-2577 leaves elicitation as MRTR's only durable consumer.

Durable-state scoping — the distinction that sizes the infrastructure. Plain-tool pass-through rounds and the in-request bridge need no durable state whatsoever: round state lives solely in the backend's own requestState. Only composite-workflow suspend/resume triggers RFC-0083 D5's machinery, followed verbatim: the client-visible requestState becomes a server-minted ≥128-bit opaque handle (never serialized state), WorkflowStatus gains an owner bound to the plaintext (iss, sub) pair via pkg/vmcp/session/binding's Format (D5's explicit not-a-hash decision), owner-checked resume failing closed, TTL at write, audit redaction, Redis store behind a core.Config seam. Nothing of that lands here — it lands with slice 5, its first actual trigger.

Sampling: deprecated pass-through only, no feature work. SEP-2577 (Final) deprecates sampling with "New implementations SHOULD NOT add support for deprecated features unless needed for backward compatibility with existing counterparts." But the MRTR union still carries it — InputRequest = CreateMessageRequest | ListRootsRequest | ElicitRequest (schema/draft/schema.ts), and SEP-2577 mandates that union types "MUST NOT be modified during the deprecation period." vMCP's relay is type-agnostic, so sampling transits for counterparts that still declare it — compatibility plumbing under the carve-out, not added support. Roots (also deprecated, no existing vMCP seam): relay-only transit on pass-through, explicit refusal on the bridge.

Adjacent Modern-path issues, deliberately out of scope, delineated in the doc: #6058 (per-request SSE streaming for progress/log notifications — those are notifications, not server-initiated requests, so MRTR does not touch them; slice 2's input_required envelope may ride that future stream as its final message per the spec); #6064 (ping removal + required resultType on dispatch — pure conformance, orthogonal); #6065 (subscriptions/listen push delivery — a different channel with a fixed four-type subscribable set, structurally disjoint from MRTR rounds).

Refs #5743.

Slice 1: what actually changes, and why it is zero-behavior-change

interpretModernResult collapsed a Modern backend's resultType:"input_required" envelope into the opaque errModernInputRequired sentinel, discarding the payload — so no upper layer could ever relay or fulfill a round. Now it returns a typed error carrying a domain vmcp.InputRequiredResult (inputRequests as opaque raw values + requestState), extractable through arbitrary wrapping via client.InputRequiredFromError — the single branch point slices 2–4 consume.

Zero behavior change, each property pinned by a test:

  • the typed error unwraps to errModernInputRequired, so every existing errors.Is classification (notably probeRevision's Modern-positive signal) is untouched;
  • the message is asserted byte-identical to the previous fmt.Errorf wrapping;
  • an unrecognized resultType keeps pure sentinel semantics (no payload) — per the spec, clients "SHOULD treat unrecognized values as invalid protocol responses";
  • capabilities are still declared empty on egress, so a compliant backend cannot yet send a round at all — the seam becomes load-bearing only at slice 3.

Guard proven by reverting the production hook: the payload-extraction and verbatim-relay tests fail against the old code, all green restored.

The spec is NOT final — do not read this as designed-against-final

Designed against schema/draft/schema.ts (LATEST_PROTOCOL_VERSION = "2026-07-28"), the /specification/draft/basic/patterns/mrtr page, SEP-2322 (Status: Final) and SEP-2577 (Status: Final), as of 2026-07-27, re-checked 2026-07-28: upstream still has no schema/2026-07-28 directory, /specification/2026-07-28/ pages 404, and the newest release is 2026-07-28-RC (prerelease, published 2026-05-29); the draft schema is byte-identical between the two checks. The doc names the four load-bearing points that rest on draft page text rather than the Final SEPs, with the design's exposure to each: (a) the three-method input_required table (SEP-2322 Final also allowed GetTaskPayloadRequest; the page dropped it with tasks→extension); (b) the capability-gating MUST NOT (server requirement 7 — page-only); (c) the at-least-one-of inputRequests/requestState rule; (d) the requestState security language, where the SEP and page differ — the D5 handle design satisfies the stricter reading, so it is robust to either. The drift checklist stays pending the final cut.

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests — go test ./pkg/vmcp ./pkg/vmcp/client ./pkg/vmcp/server all green on this head (the server package included deliberately: it carries Classify Modern mid-call capability refusals per spec #6061's tests, proving coexistence). task test was not used locally — it panics on the known gotestfmt v2.5.0 bug ("BUG: Empty package name encountered"); CI runs the real thing.
  • Linting (task lint-fix) — 0 issues.
  • Manual testing (describe below)

Manual verification: the production hook in modern.go was reverted to the pre-PR fmt.Errorf line; TestInterpretModernResultInputRequired and TestInputRequiredRequestsAreRelayedVerbatim fail (payload gone), the errors.Is/message subtests still pass — demonstrating the guard tests the new property and only the new property. Restored: all green.

CRD docs note: 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; regenerating in a clean origin/main worktree produces no diff, so the committed regeneration is this branch's consequence, not pre-existing staleness.

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.

Changes

File Change
docs/arch/16-vmcp-mrtr.md The MRTR design: protocol recap with citations, the four bridge cells, durable-state scoping, D5 handle spec, sampling/roots disposition, gap list, slice sequencing, draft-only dependency flags, #6061 reconciliation, #6059 tracking + the flagged requestState open decision
docs/arch/README.md Index entry for doc 16
docs/operator/crd-api.md Regenerated (task crdref-gen); see the CRD note in the test plan
pkg/vmcp/mrtr.go Domain InputRequiredResult (opaque raw InputRequests + RequestState) with a fail-closed Methods() probe for the future capability gate
pkg/vmcp/client/modern_mrtr.go Typed inputRequiredError (unwraps to the sentinel, identical message) + InputRequiredFromError extraction seam
pkg/vmcp/client/modern.go One hunk: interpretModernResult's non-complete branch returns the typed error instead of a bare wrap
pkg/vmcp/{mrtr,client/modern_mrtr}_test.go Payload extraction, errors.Is back-compat, byte-identical message pin, verbatim-relay invariant, load-shedding and malformed-payload shapes, foreign-error rejection

Size note, pre-empting the 400-line objection: the production change is 137 lines across 3 files, under the guideline by the repo's own counting rule (which excludes tests/docs); the raw total is dominated by the design doc and tests. Doc and slice ship together because the slice's shape (opaque values, sentinel-compatible error, the specific branch-point function) is only justifiable by the design, and the design's slice-1 section is only credible because the seam it names compiles and is tested — splitting them would leave each half asserting things the reviewer cannot check.

Does this introduce a user-facing change?

No. Wire behavior is byte-identical: the typed error renders the same message, classifies the same, and no backend can send an input_required round until capability mirroring (slice 3) lands.

Special notes for reviewers

  • One flagged divergence from Support MRTR pass-through for Modern backends in vMCP #6059's sketch, not silently diverged: Support MRTR pass-through for Modern backends in vMCP #6059's step 2 wraps the backend's requestState with vMCP routing context; the design relays it verbatim and re-derives routing from the capability name — a vMCP-authored wrapper would make vMCP a state-minting server under MRTR server requirements 4–5 (integrity protection, principal binding), a key-management surface the verbatim relay avoids; the cost is the rerouted-between-rounds edge case, which is client-recoverable. Recorded in the doc as an open decision to resolve at slice 2/3; if the wrapped form wins, it must carry D5-grade protections, not a bare backend ID.
  • Relationship to Classify Modern mid-call capability refusals per spec #6061 (merged): it owns the two-path mid-call capability-refusal contract (-32021 + data.requiredCapabilities at HTTP 200 for undeclared — the documented deviation from the spec-mandated 400, upstream go-sdk#1117 — and an explicit -32603 naming SEP-2322 for declared). This design reconciles TO it: the contract is permanent for the Modern-client/Legacy-backend cell, superseded by slice 3's mirroring only where the backend is Modern. The two classifications are provably disjoint from slice 1's error: a Modern backend cannot send server-initiated requests, a Legacy backend cannot emit input_required. Two observations for Classify Modern mid-call capability refusals per spec #6061's authors, not acted on here: (1) its crafted messages say multi-round retrieval is something "this server does not implement" — once slice 3 lands, that wording needs rescoping to the Legacy-backend limitation it permanently describes; (2) modernClientCapabilitiesKey hand-duplicates pkg/mcp's unexported metaKeyClientCapabilities — slice 3 is the natural moment to export the constant and de-duplicate.
  • toolhive-core gap list (doc section "Gaps", phrased for upstream): mcpcompat drops MRTR fields in both directions — CallToolResult keeps only the unexported resultType for NeedsInput(), no InputRequests/RequestState fields, and CallToolParams cannot carry inputResponses/requestState on a retry (go-sdk's own CallToolResult already exports both); no no-initialize client primitive (Retire the hand-rolled Modern wire shim in vMCP #6018 — this PR narrows that issue: the result structs export the MRTR fields, it is the resultType plumbing and the retry-loop internals that are unexported); assertServerInitiatedRequestAllowed confirmed version-gated only (v1.7.0-pre.3 mcp/server.go:1544-1553) — accounted for, harmless here.
  • Slices 2–5 are sequenced in the doc and deliberately not started: ingress envelope + retry params (hooking writeModernCallFailure), capability mirroring (activates the pass-through; with slice 2, the scope of Support MRTR pass-through for Modern backends in vMCP #6059), the Legacy-client bridge, composite suspend/resume with the D5 store.
  • One open product decision, flagged for a human: whether the pass-through should relay deprecated request types (sampling/roots) at all, or strip to elicitation-only. The design defaults to relay under SEP-2577's backward-compat carve-out; strict-strip is one line in the slice-3 capability gate.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n

JAORMX and others added 4 commits July 28, 2026 05:59
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
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
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
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
@github-actions github-actions Bot added the size/L Large PR: 600-999 lines changed label Jul 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.35%. Comparing base (92c70db) to head (34c4de6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6074      +/-   ##
==========================================
+ Coverage   72.31%   72.35%   +0.04%     
==========================================
  Files         730      732       +2     
  Lines       75630    75670      +40     
==========================================
+ Hits        54689    54754      +65     
+ Misses      17035    16996      -39     
- Partials     3906     3920      +14     

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

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
@JAORMX
JAORMX requested review from blkt and reyortiz3 as code owners July 28, 2026 06:46
@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 28, 2026
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
@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 28, 2026

@jhrozek jhrozek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Read through the design doc and the slice. The doc's factual rigor is the strongest thing here — I spot-checked ten of its concrete code claims against the tree and the module cache and they all held, including the exact upstream line numbers (assertServerInitiatedRequestAllowed at go-sdk v1.7.0-pre.3 mcp/server.go:1544-1552, maxMultiRoundTripRetries = 10, mcpcompat's unexported resultType with no InputRequests/RequestState, binding.Format producing iss + "\x00" + sub). The spec quotations check out verbatim against the live draft too. And the slice really is zero-behaviour-change: Unwrap returns the sentinel directly, so probeRevision's errors.Is at client.go:1151 is unaffected.

Nothing here blocks. My comments cluster on one theme: what the seam promises the consumers that don't exist yet. Two of them (the method restriction, and telling a malformed round from a valid one) are rules the doc itself states but the code can't enforce, and one is about which package the exported contract should live in. All three are a lot cheaper to settle now than after the symbols ship and slice 2 is built on them.

The four I'd actually act on before slice 2:

  1. Gate extraction on the three permitted methods — method is already in scope at the branch.
  2. Give a malformed or empty round a distinguishable outcome instead of ok=true.
  3. Move the carrier and extractor to pkg/vmcp, beside InputRequiredResult.
  4. Fix the comment on errModernInputRequired (modern.go:60-64) — it still says multi-round retrieval "is deferred", which this PR is in the business of undoing. Keeping the message string frozen for compat is fine, but the comment should say that's why, and point MRTR consumers at InputRequiredFromError.

Two things outside the diff: the branch is 10 commits behind main (including #6079, which is relevant to one comment below), and the doc has no Mermaid diagram — a two-edge multi-round flow is the case where README.md:423's "add diagrams" convention earns its keep.

Comment thread pkg/vmcp/client/modern.go Outdated
Comment thread pkg/vmcp/client/modern_mrtr.go Outdated
Comment thread pkg/vmcp/client/modern_mrtr.go Outdated
Comment thread pkg/vmcp/client/modern_mrtr.go Outdated
Comment thread pkg/vmcp/client/modern_mrtr.go Outdated
Comment thread pkg/vmcp/mrtr.go Outdated
Comment thread docs/arch/16-vmcp-mrtr.md
Comment thread docs/arch/16-vmcp-mrtr.md Outdated
Comment thread docs/arch/16-vmcp-mrtr.md Outdated
Comment thread pkg/vmcp/client/modern_mrtr_test.go Outdated
JAORMX and others added 3 commits July 28, 2026 10:27
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
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
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

JAORMX commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the review — the depth of the spot-checking is appreciated. All four "act before slice 2" items plus both outside-the-diff notes are addressed in the commits just pushed (a merge of main, then c92bd2d for the seam, 10d08c7 for the docs). Point by point:

The four pre-slice-2 items

  1. Method gating — agreed; took your cheap version. newInputRequiredError now takes the method and attaches a payload only for tools/call/resources/read/prompts/get; everything else keeps pure sentinel semantics, so InputRequiredFromError reports ok=false instead of inviting a relay that would make vMCP the non-conformant server. The allow-list is deliberately a separate map from pkg/mcp's nameRequiredMethods: the same three methods today, but the MRTR set could grow a fourth (GetTaskPayloadRequest, per the doc's drift list) while the Mcp-Name rule stays put — the doc comment records the coincidence.
  2. Malformed/empty rounds — agreed, and the retry-loop framing (client requirement 1 + server requirement 8, no vMCP-side round counter) is exactly the failure that matters. The decode is now strict (any json.Unmarshal error → sentinel-only), and an envelope violating server requirement 6 — nothing to fulfill, nothing to echo — also fails closed. A wrong-typed requestState no longer silently becomes "".
  3. Package placement — agreed and moved. vmcp.InputRequiredError (exported fields, including the Sentinel it unwraps to) and vmcp.InputRequiredFromError now live beside InputRequiredResult; the client package keeps only the wire decode and its unexported sentinel. Mock-based consumers can construct the error directly (pinned by a test in pkg/vmcp), and slice 2's server-side rendering never imports the concrete HTTP client. On the nothing-calls-it observation: yes, deliberate slicing — the shape now carries the fail-closed gates above precisely so the first consumer can't be built against a looser contract.
  4. errModernInputRequired comment — reworded: the message string is frozen for compatibility, and the comment now points MRTR consumers at vmcp.InputRequiredFromError instead of claiming retrieval is deferred.

Inline comments

  • Unbounded resultType interpolation — capped: Error() interpolates at most 64 bytes and reports the total length past that, following the Stop splicing whole SSE buffers into error text #6079/Use standard JSON-RPC error codes and scrub leaked error text #6066 report-the-length rule. Byte-identical rendering still holds for every realistic value; a new test pins the truncation.
  • SHOULD → MUST — applied, essentially your wording. On "task": behavior kept (sentinel-only is correct — vMCP advertises no extensions), but it no longer hides under "unrecognized": it has its own test case naming the Tasks-extension discriminator and the frozen-message caveat, next to a genuinely-unrecognized case.
  • modernResultTypeInputRequired — added beside modernResultTypeComplete; the doc-comment literals went with it.
  • RequestState absent-vs-empty — agreed; now *string, and the asymmetry with InputRequests is gone. Client requirement 2's "MUST NOT include one in the retry" is quoted at the field.
  • Anonymous-auth owner check — you're right that it's vacuous under incomingAuth: anonymous. The doc now scopes the stronger-than-client-side-AEAD claim to authenticated incoming auth, and answers the slice-5 question explicitly: documented single-tenant caveat, not refuse-to-suspend — anonymous auth already grants any caller the tools themselves, so a stolen handle confers resumption of a workflow the thief could have run outright; refusing would break composites for exactly the single-operator deployments the mode exists for. The text also notes the owner-mismatch audit signal can never fire in that mode. If you think fail-closed is the better posture there, it's a contained slice-5 decision — flagged as open to override rather than settled.
  • Capability gating "page-only" — agreed and upgraded: the drift entry now cites the core _meta section's general MUST (including the mandated -32021 + data.requiredCapabilities), and cell 1's -32021 expectation is called mandated rather than predicted. Same treatment for server requirement 6, which turns out to be even better anchored: it's also in schema/draft/schema.ts's InputRequiredResult doc comment ("At least one of inputRequests or requestState MUST be present").
  • SEP-2577 quotations — both sentences are verbatim from the SEP document itself: seps/2577-deprecate-roots-sampling-and-logging.md (the "Union types" and "Capability negotiation" sections) — which is why the schema, changelog, and versioning page don't carry them. The citations now name that file. One correction that cuts against the doc, though: the SEP's union-freeze list enumerates ClientNotification/ClientResult/ServerRequest/ServerNotification only — InputRequest is not on it (it postdates the SEP) — so the doc no longer leans on the freeze for the MRTR union and instead rests on the schema as published plus the SEP's features-remain-functional guarantee. The logging aside is in too.
  • Test template self-comparison — the canonical case hardcodes the expected message string now, so the production template and the assertion can't drift together.

Outside the diff — merged main (13 commits, bringing #6079's precedent and #6051's doc-10 section in-tree), and added the cell-1 Mermaid sequence diagram for the two-edge multi-round flow.

Also in this push, from a parallel review pass, since it lands in the same files: cell 1's claim that "the end-to-end principal binding survives because vMCP's outgoing auth presents the calling principal's identity on every round" was false for two of the six egress strategies — header_injection (static config-mounted secret) and unauthenticated (no-op) present one shared vMCP identity for every downstream user, so the backend's requestState principal binding cannot protect one downstream user from another. The claim is now scoped to the user-derived strategies, with the consequence stated: under shared-credential egress vMCP must bind the round itself, because no other component can — which turns the wrap-vs-verbatim open decision into a configuration-dependent obligation. That decision also gained a third recorded arm (relay verbatim to the backend, but make the downstream-visible requestState a server-side D5 handle — no keys, at the cost of a durable store on the pass-through path); no winner picked. Doc 10's client-edge section no longer calls the -32021 contract "the planned follow-up" — #6061 landed it, and this being the reconciliation PR, the refresh rides here. The verbatim-relay test also now asserts raw bytes (assert.Equal) as its name promised, rather than JSONEq; and the in-flight list reflects #6050/#6051 having merged.

@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/L Large PR: 600-999 lines changed labels Jul 28, 2026
Main gained CRD-affecting changes (#6092, #6046, #6086) after the
previous merge, so the Generate CRD Docs check fails against the
merge result until docs/operator/crd-api.md is regenerated on top.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n
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
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Jul 28, 2026
jhrozek
jhrozek previously approved these changes Jul 28, 2026

@jhrozek jhrozek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving. I re-verified each of the eleven items against the new blobs rather than taking the reply at face value, and they all hold up.

The structural one is the fix I cared most about and it landed properly: pkg/vmcp/server and pkg/vmcp/core still have zero non-test imports of pkg/vmcp/client, so slice 2's rendering can be built without the layering inversion, and the exported Sentinel makes mock construction work. The three fail-closed gates in newInputRequiredError are exactly the right shape — the method allow-list, the strict decode, and the server-requirement-6 check each close a case where the seam promised more than it enforced.

Two things you found that are better than what I asked for:

  • Server requirement 6 is schema-anchored, not page-only — schema/draft/schema.ts:575, "At least one of inputRequests or requestState MUST be present." Stronger footing than my comment assumed existed.
  • The header_injection/unauthenticated egress scoping is a real defect and I confirmed it independently: strategies/header_injection.go:66 takes _ context.Context and injects a static config value, so it cannot be per-user. Good catch, and turning the wrap-vs-verbatim question into a configuration-dependent obligation is a more honest position than the doc had.

One correction on my side: my "SEP-2577 quotes aren't locatable" comment was wrong. Both sentences are verbatim from seps/2577-deprecate-roots-sampling-and-logging.md (lines 174-176 and 231) — I checked schema.ts, the changelog and the versioning page and drew too much from their absence there. You were right to push back. The useful part was incidental: it surfaced that the union-freeze list covers only ClientNotification/ClientResult/ServerRequest/ServerNotification, with InputRequest absent entirely, which the doc had been leaning on.

Two nits inline, neither blocking — take or leave them. Otherwise this is good to go once the unit tests and the three E2E jobs finish; everything else is already green.

Comment thread docs/arch/16-vmcp-mrtr.md Outdated
Comment thread pkg/vmcp/mrtr.go
JAORMX and others added 2 commits July 28, 2026 11:24
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
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
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Jul 28, 2026
@JAORMX
JAORMX merged commit 746ee70 into main Jul 28, 2026
52 checks passed
@JAORMX
JAORMX deleted the mrtr-modern-vmcp branch July 28, 2026 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants