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.
This branch has not been deployed
1 participant
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 explicitid:null; 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 retains the required null ID.
DecodeMessagealso accepts explicit null-ID error responses so SDK clients can consume these envelopes: HTTP preserves per-call rejection without closing the session, and IO can read subsequent messages. Missing response IDs, null-ID success responses, and null-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.go test ./... -timeout=10m— PASS.go test -race ./internal/jsonrpc2 ./mcp -timeout=10m— PASS.go vet ./...,go build ./..., andgit diff --check— PASS.External conformance scripts were not run.