fix(runners): observe cancellation after completion callbacks - #2591
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 33900061440 --repo openai/openai-node \
--name castiron-custom-code-33900061440-1 --dir /tmp/castiron-custom-code-33900061440-1
git apply --stat /tmp/castiron-custom-code-33900061440-1/custom-code.patch
cat /tmp/castiron-custom-code-33900061440-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin dde19c5c72280516fdfd4e0f4fe987fd1a3eced1 4ae4ac34612f2b0813d86de029726c0c86bcdff4
python3 scripts/castiron/custom_code_report.py report \
--base dde19c5c72280516fdfd4e0f4fe987fd1a3eced1 \
--head 4ae4ac34612f2b0813d86de029726c0c86bcdff4 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-4ae4ac34612f
cat /tmp/castiron-custom-code-4ae4ac34612f/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
## Summary Correct three tokens in the handwritten Chat Streaming guide: - The optional `stream` flag accepted by `chat.completions.stream()` is `true`, not `false`. - The helper returns `ChatCompletionStream`, not `ChatCompletionStreamingRunner` (which is used for streaming tool runners). The current documented `stream: false` call fails type checking with TS2322. The public helper always enables streaming. No SDK implementation, generated files, public declarations, dependencies, or lockfiles change. `docs/helpers.md` is absent from the pinned Castiron generated snapshot. I checked open PRs; openai#2591 edits an unrelated `afterCompletion` section of the same guide and does not address this issue. ## Validation - Compiler reproduction through the public `OpenAI` export: `stream: false` is rejected; omitted `stream` and `stream: true` are accepted. - Public `.stream()` smoke test with synthetic SSE: exact `ChatCompletionStream` prototype, not a `ChatCompletionStreamingRunner`, serialized `stream: true`, successful completion, no live API calls. - `./scripts/test tests/lib/ChatCompletionStream.test.ts`: **40 passed** on Node 22.22.2 and **40 passed** on Node 24.19.0. - Pinned Oxfmt 0.62.0 check and `git diff --check` passed. - Local focused tests used available cached Vitest 4.1.10; the repository pins 4.1.11. A fresh frozen install remains blocked by missing publication-time metadata for `qs@6.16.0` in the configured registry; no installation safeguards or configuration were changed. ## Adversarial self-review Reviewed public types, actual return identity, optional/explicit stream behavior, compatibility, and security implications. Independent review found no actionable issues. This is a documentation-only correction; compiler/runtime verification and the existing stream suite are proportionate, without adding a Markdown-parsing test harness for three tokens.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed the cancellation boundary at all three tool-runner exit paths and the callback-error precedence. The shared helper keeps the check localized, and the focused streaming/non-streaming matrix covers final responses, forced tools, and completion limits.
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Hi, the new post-callback abort path throws a fresh APIUserAbortError() and drops the abort signal's reason. An external controller.abort(reason) therefore loses the cause here even though other client cancellation paths preserve it. Could this construct the abort error from this.controller.signal.reason and add a cause assertion?
|
Addressed the post-callback cause feedback in 718da1c. The new checkpoint now attaches the runner signal's reason by identity to APIUserAbortError.cause. The extended matrix failed 24 cause assertions before the fix; 108 related tests and 276 built CJS/ESM checks pass afterward. This deliberately covers only the newly introduced afterCompletion boundary; #2607 remains responsible for external-signal forwarding and the five preexisting checkpoints, so the changes compose without duplication. |
![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: 718da1c604
ℹ️ 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".
3 participants
Summary
Cancelling a tool runner during its final
afterCompletioncallback could be reported as success:done()resolved and successful final events were emitted even though the runner controller was aborted.This PR observes callback cancellation at the existing post-callback boundary and again at successful final settlement:
APIUserAbortError.cause.The final-settlement check is gated by a private flag set only when
afterCompletionis actually invoked. Omitted/null callbacks, configured-but-never-invoked callbacks, and unrelatedChatCompletionStreambehavior remain unchanged.Scope
Only handwritten
src/lib/AbstractChatCompletionRunner.ts, its existing cancellation test, anddocs/helpers.mdchange. No generated files, dependencies, exported types, or generalEventStreammachinery change.#2607 owns the broader external-signal reason forwarding and five preexisting tool checkpoints. Those changes are not duplicated here. This PR preserves the reason already present on the runner's signal at the new callback boundaries and composes with that separate forwarding fix.
Verification
tsc --noEmit, canonical./scripts/build, pinned Oxfmt 0.62.0, available cached Oxlint 1.76.0, andgit diff --checkpass. Emitted CJS/ESM runner declarations remain byte-identical.oxlint-configpackage-command dependency-verification failure reproduces identically on the cleandde19c5cbase.Review follow-ups
718da1c6adds cause preservation at the new checkpoint.4ae4ac34closes the late-promise-reaction gap with the callback-scoped settlement guard.Local tooling limitation
Fresh frozen installation remains blocked by missing publication-time metadata for
qs@6.16.0in the configured registry. Local checks use available cached Vitest 4.1.10 and Oxlint 1.76.0 rather than pinned 4.1.11 and 1.79.0; TypeScript 6.0.3 and Oxfmt 0.62.0 match their pins. Fresh CI validates the new head with the pinned toolchain. No lockfile, registry configuration, or supply-chain safeguards were changed.