fix(examples): reject incomplete multi-agent streams - #2666
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. |
Castiron custom code✅ No new custom-code files detected. 32 mixed files remain; 0 existing customizations changed. Compared 32 existing customizations unchanged
A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 34068029038 --repo openai/openai-node \
--name castiron-custom-code-34068029038-1 --dir /tmp/castiron-custom-code-34068029038-1
git apply --stat /tmp/castiron-custom-code-34068029038-1/custom-code.patch
cat /tmp/castiron-custom-code-34068029038-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 7b5d1209090f82f56b4971976147b5c2a859452e 5d64d68922760ed420d35657c1f6a023ac85062c
python3 scripts/castiron/custom_code_report.py report \
--base 7b5d1209090f82f56b4971976147b5c2a859452e \
--head 5d64d68922760ed420d35657c1f6a023ac85062c --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-5d64d6892276
cat /tmp/castiron-custom-code-5d64d6892276/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed exact head 5d64d68.\n\nThe example now requires an explicit coordinator response.completed event before treating normal iterator termination—whether [DONE] or EOF—as success. Child completion/failure/incomplete events remain nonterminal, and the loop deliberately continues after coordinator completion so later root failures, named SSE errors, malformed frames, or transport errors retain precedence rather than being hidden.\n\nThe new terminal error is generic and does not surface response payload or private failure detail. Root ownership handling remains aligned with the existing failed/incomplete logic (omitted, null, or explicit /root).\n\nVerified locally at the exact head:\n- focused executable multi-agent SSE suite: 25/25 passed\n- oxlint passed for both changed files\n- oxfmt check passed\n- git diff --check passed\n\nNo unresolved review threads and no in-scope blocking findings. Hosted Node 22/26 and ecosystem jobs were still running at submission; all completed checks were green or neutral.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed exact head 5d64d68 against base/merge-base 7b5d120. No actionable correctness, security, or maintainability findings.
The coordinator-completion bit is the minimal state needed to reject incomplete EOF/[DONE] without breaking early and hiding later errors. Ownership matches the existing omitted/null/explicit /root contract; child terminal events remain nonterminal. This policy belongs in the example, not the shared SSE parser. Growth is proportionate: example 44→50 lines; existing focused test file 179→223.
Independent validation:
- Actual executable example: 25/25 tests on Node 22.0.0 and 24.20.0.
- Existing SSE sentinel/error/privacy/early-exit suites: 83/83 tests.
- 28 synthetic source-execution cases against built CommonJS and ESM SDKs on Node 22.0.0, 24.20.0, and 26.7.0, including ten base-negative controls, later root failures, malformed/named errors, transport-error identity, cleanup, partial output, child continuation, and preserved sentinel behavior.
- Build, focused type check, changed-file Oxlint/Oxfmt, and diff whitespace check passed.
All fixtures were local/synthetic. Whole-project type checking was limited by missing optional Azure/Express example dependencies; I did not rerun the full handwritten/generated suites or independently compare base/head build bytes. SDK source and dependencies are unchanged.
Workspace: full clone; corrected detached worktree created at freshly fetched default main@518756a59fa9693c0751fb91403d017cfcd0037a, then checked out the exact PR head. Refreshed open/non-draft state, unchanged head, discussion, and complete review history immediately before submission; no prior Mark outcome or hold.
3 participants
Summary
Require a coordinator
response.completedevent before the multi-agent SSE example treats stream termination as success. The production change is six added lines in the handwritten example.Child-agent completion/failure remains nonterminal for the coordinator. The example still consumes the stream after coordinator completion, preserving explicit root failures, named SSE errors, and transport errors rather than hiding them with an early break.
Reproduction
On current main, the actual executable example exits successfully when the server sends
[DONE]or closes the SSE body before the coordinator completes—even with only partial coordinator text or a completed/failed child agent. Ten new executable regressions fail before the source change, while fourteen controls pass.Validation
[DONE], omitted/null/explicit root ownership, child-status continuation, and a named SSE error received after coordinator completion.git diff --checkpass. The entire built SDK is byte-for-byte identical to the base build.Only the example and its existing regression-test file change. No SDK runtime/parser changes, generated files, dependencies, new payload limits, or WebSocket changes. All inputs and responses are synthetic; no live API calls. This complements the existing explicit-terminal-error handling and the separate WebSocket early-close fix.