Repository navigation
fix(assistants): reject root IDs in run-step deltas - #2698
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 34257752171 --repo openai/openai-node \
--name castiron-custom-code-34257752171-1 --dir /tmp/castiron-custom-code-34257752171-1
git apply --stat /tmp/castiron-custom-code-34257752171-1/custom-code.patch
cat /tmp/castiron-custom-code-34257752171-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin bb0f51bed84bf60b0639af7a894fcf538264e435 99110fc2713aa382f4162c79d9e8d42f96d51d4c
python3 scripts/castiron/custom_code_report.py report \
--base bb0f51bed84bf60b0639af7a894fcf538264e435 \
--head 99110fc2713aa382f4162c79d9e8d42f96d51d4c --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-99110fc2713a
cat /tmp/castiron-custom-code-99110fc2713a/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
There was a problem hiding this comment.
Reviewed the exact 2-file patch against the requested outcome: reject root run-step delta IDs before raw SSE/serialized dispatch and again after ordinary raw-listener mutation immediately before accumulation, while preserving callback object identity, nested tool-call IDs, extension fields, and existing alias behavior. The new checks are placed at both mutation boundaries and do not broaden into Proxy/getter hardening outside the documented application-authority scope. The full seven-suite AssistantStream target passes locally: 278 tests. Git diff check passes, no public API or generated/runtime surface beyond the 14-line accumulator validation changes, no unresolved threads, and every exact-head lint/build/Node 22/24/26/ecosystem/CodeQL/Castiron check is green. No blocking findings.
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed 99110fc against merge-base 80ce89f. No actionable correctness or structural findings.
Both checks are justified: decoded root IDs must fail before raw dispatch, while ordinary listeners can modify or replace the delta before accumulation. Capturing and checking that delta immediately before merging preserves valid callback identity, nested tool-call IDs, extension fields, and the existing canonical-ID/alias handling. I read the repository security model and evaluated its protocol-data/application-authority distinction. A shared recursive prohibition would incorrectly reject nested IDs; a single check would lose one timing guarantee. No additional ownership framework is warranted. File sizes are 1,025→1,036 lines for AssistantStream and 647→745 for the identity tests; neither newly crosses 1,000 lines.
Fresh validation: 278 tests across seven AssistantStream suites passed on Node 24.20.0; all 51 identity tests passed on exact Node 22.0.0. Independent SSE/NDJSON probes reproduced root-ID accumulation using the merge-base AssistantStream and confirmed protocol/listener rejection, error/end ordering, and valid identity/nested-ID/extension preservation at this head. Build passed on Node 24.20.0; AssistantStream CJS/ESM imports passed on both runtimes. Diff whitespace checks passed.
Validation limits: internal js installation and pinned pnpm installation were blocked by tooling/registry access, so execution used cached dependencies (Vitest 4.1.11, TypeScript 6.0.3). Canonical lint passed with cached Oxlint 1.79.0 rather than pinned 1.80.0. Full type checking could not pass because example dependencies dotenv, @azure/identity, and express were unavailable. These are validation limits, not patch findings. No heap-exhaustion experiment was rerun.
Automated Release PR --- ## [7.12.0](openai/openai-node@v7.11.0...v7.12.0) (2026-09-08) ### Features * **api:** add GPT Image 2.5 models and image options ([openai#2705](openai#2705)) ([6500a16](openai@6500a16)) ### Bug Fixes * **assistants:** reject root IDs in run-step deltas ([openai#2698](openai#2698)) ([63dfa4c](openai@63dfa4c)) * **files:** use a monotonic clock for processing timeout ([openai#2657](openai#2657)) ([bb0f51b](openai@bb0f51b)) * preserve suppressed workload authorization ([openai#2699](openai#2699)) ([57bbfd3](openai@57bbfd3)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: openai-sdks[bot] <284451331+openai-sdks[bot]@users.noreply.github.com>
Supersedes #2685.
Run-step deltas must not carry a root
id. Reject that field before raw-event dispatch and recheck the captured delta immediately before accumulation, so an ordinary raw listener cannot introduce it between validation and merging. A listener-added root ID rejects the event instead of being silently stripped.This keeps run-step identity out of delta accumulation while preserving valid callback objects, nested tool-call IDs, extension properties, and existing alias handling. The production change is 14 additions and 3 deletions in
AssistantStream.ts.Scope
The security guarantee covers decoded SSE/NDJSON protocol data. The pre-merge check also covers ordinary listener additions and replacements. It follows the repository's documented distinction between protocol data and executable application authority; arbitrary hostile getters, Proxies, and callback code running with application authority are outside this change's security guarantee.
Validation
./scripts/test tests/lib/AssistantStream.test.ts tests/lib/assistant-stream.pnpm lint, fullpnpm exec tsc, andpnpm buildpass with the pinned dependencies.Validation uses focused rejection tests; the original heap-exhaustion experiment was not rerun.