Repository navigation
feat(responses): accumulate rich WebSocket progress - #4224
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)
ℹ️ 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 Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 6d6a447eee
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Requesting changes after comprehensive static review of 6d6a447. I independently confirmed the existing P2 shell-bound lookup finding: N shell outputs followed by matching calls and one delta per output cause N scans of the response-wide output map. Per-output revision caching does not avoid those initial scans, and a later call update invalidates every output cache again. Maintain a response-level call-ID index for command bounds, updating it on lifecycle/full-item replacements and command growth, and add coverage with many distinct outputs plus unrelated call updates. This avoids quadratic ingestion for a large response from a custom/untrusted peer.
I am linking the existing thread rather than duplicating an inline finding. Reviewed rich-event routing/type guards, sparse indexes, full replacement authority, per-command arrays including empty arrays, detached snapshots, and regression tests. Independent second review found no additional actionable issues. All hosted checks passed or skipped. No candidate code or local tests executed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73048af654
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-reviewed 73048af. The prior quadratic shell lookup is fixed: command bounds now use direct call-ID lookup with updates on lifecycle/full-item replacement, removal, command growth and reset. Many-distinct-output and unrelated-call coverage matches the issue. The optional-ID preservation and matching-family binding changes also look sound.
Requesting changes for the existing MCP list-tools status finding: response.mcp_list_tools.in_progress/completed/failed are in ResponsesServerEvent but absent from _RICH_STATUS_EVENTS, so add_event silently ignores them. A matching mcp_list_tools item never receives these intermediate status updates despite the new documented MCP status support. Add this family with matching item/type guards and focused coverage for all three statuses. Linking the existing finding rather than duplicating an inline comment. Both static reviewers confirmed the gap and found no additional issues.
Hosted CI: pending test (Python 3.10); other completed checks passed/skipped. No candidate code or local tests executed.
|
@codex review |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed c6ef381 with coordinator and independent static passes. Approved; no blocking findings.
The incremental change clarifies documentation to promise MCP call status updates. With this explicit scope and the author response at #4224 (comment), I am withdrawing my prior blocker: list-tools accumulation is an additional capability, not a requirement of this change. No runtime code changed since the prior review; the shell indexing and optional-ID fixes remain sound.
Hosted CI: all checks passed/skipped. No candidate code or local tests executed.
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
ResponsesWebSocketAccumulatornow tracks reasoning, shell, interpreter and tool status progress, plus queued response metadata. Status accumulation covers code interpreter, file/web search, image generation and MCP calls.detailed_snapshot()exposes these observations; terminal responses remain authoritative.Shell observations use indexed command facts. Done events retain every output row for their command, including empty arrays, outcomes and metadata. Whole-item output stays authoritative without guessing command boundaries or inventing outcomes. The helper buffers fragments and indexes command counts by call ID, avoiding repeated scans of retained output.
Detailed full items retain omitted, null and present ID fields. Function and custom tool records that omit ID bind their first matching progress ID; explicit-null, present-ID and unscaffolded replacement policies remain compatible.
Validation: focused accumulator checks pass with Python 3.10/Pydantic v1 and Python 3.13/Pydantic v2. Ruff, formatting and ty pass. Regressions cover all 28 rich progress/status tags, reasoning parts, queued refresh, sparse shell observations, full completion arrays, metadata presence and identity guards.