Skip to content

Echo the request JSON-RPC id in session-not-found 404s#6031

Open
JAORMX wants to merge 2 commits into
mainfrom
echo-jsonrpc-id-in-session-404
Open

Echo the request JSON-RPC id in session-not-found 404s#6031
JAORMX wants to merge 2 commits into
mainfrom
echo-jsonrpc-id-in-session-404

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes #5945. The transparent proxy's unknown-session guard returned a -32001 404 whose body hardcoded a null id, while the classification-error path on the very same RoundTrip already echoed the incoming id. A client correlating responses by id got null for this one error class.

The cause is scoping, not intent: the id is parsed a few lines above the guard, but inside the classification block, so the guard could not reach it. sawInitialize was already hoisted out of that block for the same reason; the id now is too.

It is set only when parseRPCRequest reports a single request — and that predicate is already method != "" && len(id) > 0 && id != "null", so by construction a set id is present and non-null. Notifications, batches, and bodiless GET/DELETE keep the null id JSON-RPC requires.

Audit of the sibling call sites

The issue names one site; I checked all four before changing anything, because fixing one and leaving three inconsistent would be half a fix. The transparent proxy was the only site discarding an id it actually had:

Site Request shape id available? Verdict
transparent_proxy.go guard POST with JSON-RPC body yes, discarded fixed here
streamable_proxy.go:1096 POST yes already echoes req.ID.Raw()
streamable_proxy.go:433 standalone SSE GET no body nil correct
streamable_proxy.go:484 DELETE no body nil correct
httpsse/http_proxy.go:555 GET, session_id query param no body nil correct

Consequently NotFoundBody's doc comment — which listed "transparent proxy" as an id-unavailable case — became untrue, so it now states when nil is legitimate instead of implying it is a free choice.

Type of change

  • Bug fix

Test plan

  • Unit tests (task test) — pkg/transport/session and pkg/transport/proxy/transparent green with -race
  • Linting (task lint) — 0 issues; go vet ./... clean
  • Manual testing (described below)

TestRoundTrip404EchoesRequestID is the end-to-end pin, table-driven over numeric id, string id, notification, and explicit-null id. It also decodes the body to assert the envelope stays a valid JSON-RPC object, since echoing a raw id could otherwise corrupt it.

Mutation-verified. Setting requestID = nil inside the singleRequest block (semantically pre-fix, build intact) fails exactly the two id-bearing subtests and correctly leaves the two null-id subtests passing. Restored, all four pass. My first attempt at this mutation was invalid — deleting the argument broke the build on an unused variable rather than failing the test — so the reported result is from the second, valid attempt.

Why the existing test could not have caught this: TestRoundTripReturns404ForUnknownSession sends {"method":"tools/list"} with no id, so it renders a null id before and after the fix. That is precisely why the bug survived having a test on this path.

TestNotFoundResponse also became table-driven, covering json.RawMessage ids — the shape the proxy actually passes — so a numeric id stays numeric rather than being coerced to a float or string. It additionally asserts ContentLength matches the body, since a client reading exactly that many bytes would truncate or block if it drifted.

API Compatibility

  • This PR does not break the v1beta1 API.

session.NotFoundResponse gains a requestID parameter. It is an internal Go helper with a single production caller; no operator API surface is touched.

Does this introduce a user-facing change?

Yes, narrowly. A session-not-found 404 from the transparent proxy now echoes the request's JSON-RPC id instead of always sending null, so a client correlating by id can match this error to its request. Requests that carry no id are unchanged.

Special notes for reviewers

The 404 behaviour is deliberately untouched. As the issue notes, the guard keying on Mcp-Session-Id presence rather than the client-forgeable revision is a fail-safe by design — this changes only the id in the error body.

Rebased onto main and retargeted from its original base. It was briefly stacked on my own #6030, which fixed a build break on main; #6029 landed an equivalent (and cleaner) fix first, so #6030 is closed as superseded and this now sits directly on main as a single commit.

Closes #5945

Generated with Claude Code

@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Jul 27, 2026
rdimitrov
rdimitrov previously approved these changes Jul 27, 2026
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.18%. Comparing base (8a860d9) to head (abd7af2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6031      +/-   ##
==========================================
- Coverage   72.18%   72.18%   -0.01%     
==========================================
  Files         721      721              
  Lines       75025    75071      +46     
==========================================
+ Hits        54158    54191      +33     
- Misses      17005    17019      +14     
+ Partials     3862     3861       -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.

@JAORMX
JAORMX force-pushed the echo-jsonrpc-id-in-session-404 branch from d3a7ee8 to 1bd0d48 Compare July 27, 2026 16:22
@JAORMX
JAORMX changed the base branch from fix-main-reserved-meta-test-build to main July 27, 2026 16:22
@JAORMX
JAORMX dismissed rdimitrov’s stale review July 27, 2026 16:22

The base branch was changed.

@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Jul 27, 2026
jhrozek
jhrozek previously approved these changes Jul 27, 2026
The transparent proxy's unknown-session guard returned a -32001 404 whose
body hardcoded a null id, while the classification-error path on the very
same RoundTrip already echoed the incoming id. A client correlating
responses by id got null for this one error class.

The id was parsed a few lines above the guard but scoped to the
classification block, so the guard could not reach it. Hoist it. Only set
it when parseRPCRequest reports a single request, which by construction
means the id is present and non-null -- notifications, batches and
bodiless GET/DELETE keep the null id JSON-RPC requires.

The three sibling WriteNotFound call sites were audited and are correct
as-is: standalone-SSE GET, DELETE, and the httpsse GET all carry no body
and therefore no id. The streamable POST path already echoed it. So this
was the only site discarding an id it had. NotFoundBody's doc comment
listed the transparent proxy as an id-unavailable case, which this makes
untrue, so it is updated to say when nil is legitimate.

Closes #5945
@JAORMX
JAORMX force-pushed the echo-jsonrpc-id-in-session-404 branch from 1bd0d48 to abd7af2 Compare July 27, 2026 17:07
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Jul 27, 2026
The hostile-input proxy spec carried a hostileCase.idNotEchoed flag whose
only true case was the foreign-session 404, and whose doc comment spelled
out the very asymmetry the previous commit removes: session.NotFoundResponse
hardcoding a nil id while ClassificationErrorResponse on the same RoundTrip
echoed it. With the id now threaded through to the guard, the spec's
Expect(resp.ID).To(BeNil()) failed -- correctly.

The flag has no remaining true case: all five rejections are single JSON-RPC
requests posted to /mcp, so RoundTrip parses an id for every one of them.
Drop the field and the conditional so each case asserts the echoed id
uniformly, and record on the foreign-session case why its 404 now carries
one.
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Jul 27, 2026
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Transparent proxy: session-not-found 404 does not echo the request JSON-RPC id

3 participants