Skip to content

Stop splicing whole SSE buffers into error text - #6079

Merged
jhrozek merged 1 commit into
mainfrom
fix-6066-review-followups
Jul 28, 2026
Merged

Stop splicing whole SSE buffers into error text#6079
jhrozek merged 1 commit into
mainfrom
fix-6066-review-followups

Conversation

@jhrozek

@jhrozek jhrozek commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Two review findings against #6066 that arrived after it merged.

processEventStream splices the whole SSE buffer into its error text. When a backend's text/event-stream body contains no recognized line separator, pkg/mcp/tool_filter.go reported it as fmt.Errorf("unsupported separator: %s", string(buffer)). That buffer is the full pre-filter payload — for a tools/list, the unfiltered tool list — and the error is logged upstream, so the failure produced one unbounded log line carrying exactly the payload the filter exists to scrub. It now reports the length, which is the only diagnostic value the message ever had.

This is the byte-identical sibling of the case #6066 fixed in pkg/authz/response_filter.go. Fixing one and not the other was an oversight, not a scoping decision — the two lines differed only in the variable name.

Scoping it honestly: not client-visible and not a data leak. Both are server-log concerns, reachable only when a backend returns an SSE body with no \r\n, \n, or \r at all. The cost is log bloat and attacker-influenceable log volume, not disclosure.

A comment that contradicted its own function. handleUnauthorized's doc comment said the authorizer error "must never be promoted to a structured field routed to an aggregator beyond this log line" — while the next statement logs it as precisely such a field. The behaviour is correct and deliberate; the wording was wrong. The intent was to forbid copying it into further log lines. Reworded to say that.

Type of change

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

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

go build ./... clean and go test -count=1 ./pkg/mcp/... ./pkg/authz/... green on the merged-main base.

No test needed updating: nothing in the repo asserted the old "unsupported separator" text (checked across pkg/, test/, and docs/). No new test added either — the changed line is an error-message format in a branch that requires a backend to emit a separator-free SSE body, and asserting a %d-formatted length would pin the wording without pinning the property that matters.

Linting not run locally; leaving it to CI.

Does this introduce a user-facing change?

No. Both changes are server-side only — one error message that reaches logs rather than clients, and one comment.

Special notes for reviewers

Two logical changes in one commit, which brushes the one-change-per-PR rule. They are 5 added lines total and share a single origin (post-merge review of #6066), so they are batched rather than split into two PRs of two lines each. Happy to separate them if you would rather.

The alternative was to wait. This could equally have been folded into whatever next touches pkg/mcp/tool_filter.go; the only cost of waiting is that the sibling defect stays live. Raised as its own PR because #6066 just fixed the identical line one directory over, so leaving the twin in place is the kind of thing that reads as deliberate six months later.

🤖 Generated with Claude Code

processEventStream reported an unrecognized line separator by
interpolating the entire buffer into the error string. That buffer is the
full pre-filter payload -- for a tools/list, the unfiltered tool list --
and the error is logged upstream, so a backend response with no
recognized separator produced one unbounded log line containing the very
payload the filter exists to scrub. Report the length instead, which is
the only diagnostic value the message carried.

This is the byte-identical sibling of the case fixed in
pkg/authz/response_filter.go by #6066. Fixing one and not the other was
an oversight, not a scoping decision.

Also correct a comment that contradicted its own function: it said the
authorizer error "must never be promoted to a structured field routed to
an aggregator beyond this log line" while the next statement logs it as
exactly such a field. The intent was to forbid copying it into further
log lines, not to forbid the one that is there.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Jul 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.31%. Comparing base (4a61bd1) to head (44c4c95).

Files with missing lines Patch % Lines
pkg/mcp/tool_filter.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6079      +/-   ##
==========================================
+ Coverage   72.26%   72.31%   +0.04%     
==========================================
  Files         725      725              
  Lines       75358    75358              
==========================================
+ Hits        54460    54495      +35     
+ Misses      17016    16965      -51     
- Partials     3882     3898      +16     

☔ 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 f391d38 into main Jul 28, 2026
48 checks passed
@jhrozek
jhrozek deleted the fix-6066-review-followups branch July 28, 2026 08:20
JAORMX added a commit that referenced this pull request Jul 28, 2026
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
JAORMX added a commit that referenced this pull request Jul 28, 2026
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
JAORMX added a commit that referenced this pull request Jul 28, 2026
* Design MRTR for the vMCP Modern path

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

* Surface Modern input_required rounds as typed values

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

* Flag the MRTR design's draft-only spec dependencies

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

* Reconcile the MRTR design with merged #6061

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

* Regenerate CRD reference docs

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

* Track the MRTR design under issue #6059

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

* Harden the MRTR egress seam per #6074 review

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

* Correct MRTR design claims found false in review

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

* Regenerate CRD reference docs after main merge

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

* Narrow anonymous-auth handle-theft rationale

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

* Name missing sentinel in InputRequiredError

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants