Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4779e9b02d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !msg.ID.IsValid() && msg.Error != nil { | ||
| to.ID = json.RawMessage("null") |
There was a problem hiding this comment.
This branch makes unknown-ID errors encode with "id":null, but DecodeMessage cannot consume that wire form: after MakeID(nil), the unchanged if !id.IsValid() branch at internal/jsonrpc2/messages.go:220 returns ErrInvalidRequest even when msg.Error is populated. Consequently the IO reader and the streamable client's non-2xx-body parser treat the new -32600 response as a transport/read failure (potentially tearing down the session) rather than a JSON-RPC error; accept explicit null-ID error responses and cover an encode/decode round trip.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 3e9420c. DecodeMessage now accepts an explicit null-ID error response while still rejecting missing response IDs, null-ID success responses, and null-ID responses with both result and error. Added encode/decode round-trip coverage, an IO response-followed-by-response regression, and an HTTP client regression proving the session survives the rejection and a subsequent call succeeds. All three regressions failed on the previous head; full tests, race for jsonrpc2/mcp, vet and build now pass. The presence check is confined to the unknown-ID error path; ordinary decoding and unmatched-response correlation are unchanged.
| {"null", `{"jsonrpc":"2.0","method":"ping","id":null}`, false, false}, | ||
| } { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| // Arrange. |
There was a problem hiding this comment.
The // Arrange., // Act., and // Assert. labels merely restate the immediately following test structure, and the same labels are repeated throughout both new test files. Remove these phase labels; the root guidance explicitly asks contributors not to add comments unless they are necessary and to prefer self-documenting code.
AGENTS.md reference: AGENTS.md:L45-L47
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 10b0d70: removed the redundant Arrange/Act/Assert phase comments from both new test files. Test structure and behavior are unchanged. The affected tests and git diff --check pass.
| } | ||
| if !bytes.Equal(bytes.TrimSpace(raw.ID), []byte("null")) { | ||
| return nil, ErrInvalidRequest | ||
| } |
There was a problem hiding this comment.
I would not modify anything here.
MCP Spec & Schema (JSONRPCErrorResponse): Unlike base JSON-RPC 2.0, the MCP specification explicitly forbids null IDs (RequestId is string | integer) and specifies id?: RequestId on JSONRPCErrorResponse (omitting id when it cannot be read, rather than emitting "id": null, which fails MCP JSON Schema validation).
There was a problem hiding this comment.
You're right — I conflated base JSON-RPC with MCP. Fixed in 6b17534.
Unknown-ID errors now omit id. The decoder accepts an error-only response with no id, while still rejecting explicit id: null, success responses without an ID, and mixed result/error responses without an ID. Simply reverting the decoder would have broken round-tripping of the corrected response.
I updated the wire expectations and retained the HTTP session-continuation and IO recovery regressions. Verified against the 2025-11-25 and 2026-07-28 JSON schemas. Local checks passed on Go 1.27.1: focused regressions, go test ./..., go test -race ./..., go vet ./..., go build ./..., gofmt, and git diff --check.
skosovsky
changed the title
This branch has not been deployed
2 participants
Fixes #1391.
A single request such as
{"jsonrpc":"2.0","id":true,"method":"ping"}currently produces HTTP 400 text/plain or a terminal IO read error without a response. Return a JSON-RPC -32600 error with the unknownidomitted; the IO transport then continues to the next message without recursive reads. No rejected request reaches application dispatch.The decoder marks rejected request IDs separately from response-ID errors. Error serialization omits unknown IDs, as permitted by the MCP 2025-11-25 and 2026-07-28 schemas.
DecodeMessagealso accepts error-only responses with noidso SDK clients can consume these envelopes: HTTP preserves per-call rejection without closing the session, and IO can read subsequent messages. Explicit null response IDs, success responses without an ID, and unknown-ID responses containing both result and error remain rejected. A presence check is confined to the unknown-ID error path, leaving ordinary message decoding unchanged. Uncorrelated IO responses retain the existing connection behavior; no response is assigned to an arbitrary outstanding call.Existing batch handling, malformed JSON recovery, and absent/null request-ID behavior remain unchanged. This is independent of #1375/#1253: it handles IDs the decoder already rejects and also composes with their numeric validation. #1209/#1210 address a separate syntax-error/resynchronization path.
Validation (Go 1.27.1, darwin/arm64):
d1a225e.4779e9b. The updated omitted-ID regressions pass on6b17534.go test ./...— PASS.go test -race ./...— PASS.go vet ./...,go build ./..., gofmt, andgit diff --check— PASS.External conformance scripts were not run locally. CI for
6b17534fdc8f739178cbb908fd1eed1dee36b1d2passed both server and client conformance, tests on Go 1.25 and 1.26, race tests, lint, and CodeQL.