fix(openai-chat): refuse to emit an unnamed streamed tool call - #1531
Merged
Conversation
Roadmap unit for issues #1514, #1503, #1497, #1409, #1419, grounded in the live tree and closed through three independent audit rounds. Two phases were superseded by the audit: - 030 -> 031: the proposed rangeFullyCovered field was unsound. usage.jsonl appends on request completion while entries carry the request start time, so the oldest retained row does not bound the dropped prefix. - 050 -> 051: installCrashGuards() and cancelBodyOnAbort already ship at 8 sites, so the proposed hardening was a re-proposal of existing code. #1497 and #1409 are downgraded from Closes to evidence-backed dispositions: neither fix meets the reporter's acceptance bar on its own.
An upstream that streams function.arguments deltas without ever sending function.name left the pending call with an empty name, and every flush site emitted it anyway. The Codex tool-call contract then received a call it cannot dispatch and the turn failed downstream (#1514). Fail closed instead. The id stays synthesizable because it is an opaque correlation handle; a function name is a guess at intent, so an unnamed call terminates the turn through the adapter error channel. Dropping it silently was not an option either: #1325 established that a claimed call which disappears can orphan the matching result on the next turn. Ingest now validates the streamed shape at runtime. The tool_calls cast is a TypeScript convenience over upstream JSON, so a truthy non-string name reached the accumulator and later threw from string handling; that path now terminates through the existing invalid-tool-calls error. The buffered parseResponse validator checked the type of name but not its emptiness, so it is tightened the same way. Closes #1514
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
📝 WalkthroughWalkthroughThis PR documents a five-bug investigation and audit process. It adds plans for five remediation phases. It also hardens OpenAI streamed and buffered tool-call handling against blank names and malformed fields. ChangesCampaign evidence and audit scope
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OpenAIStreamParser
participant flushToolCalls
participant AdapterConsumer
OpenAIStreamParser->>flushToolCalls: Flush pending tool-call fragments
flushToolCalls->>flushToolCalls: Validate name, arguments, ID, and function object
flushToolCalls-->>OpenAIStreamParser: Return termination status for invalid calls
OpenAIStreamParser-->>AdapterConsumer: Emit adapter error without tool_call_start or done
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 15c6b53ca5
ℹ️ 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".
Comment on lines
927
to
+930
| for (const call of closeToolCalls()) { | ||
| // Ingest already proved `name` is a string; the typeof guard keeps this branch | ||
| // total so a future ingest change cannot turn a malformed name into a throw. | ||
| if (typeof call.name !== "string" || call.name.trim().length === 0) { |
There was a problem hiding this comment.
When a parallel batch contains a valid named call before an unnamed call, this loop emits the first call's complete tool_call_start/delta/end sequence before discovering the invalid entry. The bridge immediately converts that sequence into a completed output item, so the subsequent error cannot retract it and the client sees an issued tool call on a failed turn. Scan the snapshot for unnamed calls before yielding any events, then either fail the entire batch or emit all calls.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
Owner
Author
|
Full-suite result, as promised in the description.
All four Linux CI shards are green here as well. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@devlog/_plan/260812_five_bug_fix_campaign/010_phase1_issue1514_empty_tool_name.md`:
- Around line 78-82: Add a regression test for the unnamed-tool-call error path
around closeToolCalls(): create at least two pending tool calls, trigger the
empty tool-name validation failure, and assert that every call’s budget
reservation is released, including calls after the offending one. Preserve the
existing early-return behavior while verifying the final closeToolCalls() state.
In
`@devlog/_plan/260812_five_bug_fix_campaign/030_phase3_issue1497_usage_range_truncation.md`:
- Around line 1-5: Mark this plan as superseded by adding a prominent
“SUPERSEDED — see 031” banner near the document heading. Update the acceptance
and delivery sections to clearly identify them as historical only, and remove or
qualify any current target language, including the `rangeFullyCovered` contract
and “Closes `#1497`” conclusion, so implementers follow
`031_phase3_revised_usage_coverage.md` instead.
In
`@devlog/_plan/260812_five_bug_fix_campaign/031_phase3_revised_usage_coverage.md`:
- Around line 54-58: Define a null-safe fallback for the dashboard’s
truncated-history disclosure when historyTruncated is true and snapshot.entries
is empty, so null snapshotWindowStart and snapshotWindowEnd never produce
invalid dates or blank text. Update the range control and Available history
label logic in the affected UI flow to use the fallback, and add coverage for
the empty-snapshot case while preserving the retained-window label when dates
exist.
In
`@devlog/_plan/260812_five_bug_fix_campaign/040_phase4_issue1409_context_window_overrides.md`:
- Around line 81-95: Rewrite the field-ownership rule in the “Field-ownership
matrix” section so carry-over is based on fields that are user-editable and
absent from ProviderPayload, without requiring registry seeding. State
separately that registry-only fields must be excluded, while preserving the
existing carry-over entries and matrix decisions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3d575723-d5f1-4e36-a42a-632b75889e54
📒 Files selected for processing (14)
devlog/_plan/260812_five_bug_fix_campaign/000_research.mddevlog/_plan/260812_five_bug_fix_campaign/001_external_contract_evidence.mddevlog/_plan/260812_five_bug_fix_campaign/002_audit_round1_synthesis.mddevlog/_plan/260812_five_bug_fix_campaign/003_audit_round2_synthesis.mddevlog/_plan/260812_five_bug_fix_campaign/010_phase1_issue1514_empty_tool_name.mddevlog/_plan/260812_five_bug_fix_campaign/020_phase2_issue1503_google_thought_text.mddevlog/_plan/260812_five_bug_fix_campaign/030_phase3_issue1497_usage_range_truncation.mddevlog/_plan/260812_five_bug_fix_campaign/031_phase3_revised_usage_coverage.mddevlog/_plan/260812_five_bug_fix_campaign/040_phase4_issue1409_context_window_overrides.mddevlog/_plan/260812_five_bug_fix_campaign/050_phase5_issue1419_tls_crash_survivability.mddevlog/_plan/260812_five_bug_fix_campaign/051_phase5_revised_issue1419_disposition.mdsrc/adapters/openai-chat.tstests/openai-chat-eof.test.tstests/openai-chat-parallel-stream.test.ts
Comment on lines
+78
to
+82
| `closeToolCalls()` is called before the check, so budget reservations for every | ||
| pending call are released on the error path exactly as they are on the success | ||
| path — including the calls after the offending one, which the early `return` | ||
| skips emitting but which `closeToolCalls()` has already closed. The `finally` | ||
| block's second `closeToolCalls()` remains a no-op safety net. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a regression for reservation release.
The plan states that closeToolCalls() releases every pending reservation before the early return. The acceptance criteria do not verify this behavior.
Create at least two pending tool calls, trigger the unnamed-call error, and assert that all reservations are released. Otherwise, a failed turn can retain budget and reject later calls.
Also applies to: 241-260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@devlog/_plan/260812_five_bug_fix_campaign/010_phase1_issue1514_empty_tool_name.md`
around lines 78 - 82, Add a regression test for the unnamed-tool-call error path
around closeToolCalls(): create at least two pending tool calls, trigger the
empty tool-name validation failure, and assert that every call’s budget
reservation is released, including calls after the offending one. Preserve the
existing early-return behavior while verifying the final closeToolCalls() state.
Comment on lines
+1
to
+5
| # 030 — Phase 3 (#1497): `30d` and `all` must not silently share one byte tail | ||
|
|
||
| Depends on: nothing in 010/020 (disjoint files). Ordered here because it is the | ||
| first phase that changes a **response contract** rather than an adapter's | ||
| internal event stream, so it needs the adapter phases settled first to keep each |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Mark this plan as superseded.
031_phase3_revised_usage_coverage.md rejects rangeFullyCovered because the oldest retained request-start timestamp cannot prove coverage of the dropped prefix. This file still presents that field as the target and ends with Closes #1497``.
Add a prominent SUPERSEDED — see 031 banner and mark the old acceptance and delivery sections as historical only. Otherwise, an implementer can reintroduce the unsound completeness claim.
Also applies to: 96-105, 177-180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@devlog/_plan/260812_five_bug_fix_campaign/030_phase3_issue1497_usage_range_truncation.md`
around lines 1 - 5, Mark this plan as superseded by adding a prominent
“SUPERSEDED — see 031” banner near the document heading. Update the acceptance
and delivery sections to clearly identify them as historical only, and remove or
qualify any current target language, including the `rangeFullyCovered` contract
and “Closes `#1497`” conclusion, so implementers follow
`031_phase3_revised_usage_coverage.md` instead.
Comment on lines
+54
to
+58
| 3. **Make the dashboard stop presenting a truncated view as the requested | ||
| range.** When `historyTruncated` is true, the range control shows that the | ||
| summary is computed over a bounded tail and names the retained window. The | ||
| `Available history` label specifically must stop implying completeness — it | ||
| is the more misleading of the two, because `30d` at least states a bound. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Define the UI fallback for a null snapshot window.
If historyTruncated is true while snapshot.entries is empty, snapshotWindowStart and snapshotWindowEnd are null, but the GUI must still disclose the bounded history.
Specify a null-safe fallback label and test it. Otherwise, the UI can render an invalid date or an empty disclosure.
Also applies to: 86-94, 96-107
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 54-54: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1
(MD029, ol-prefix)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@devlog/_plan/260812_five_bug_fix_campaign/031_phase3_revised_usage_coverage.md`
around lines 54 - 58, Define a null-safe fallback for the dashboard’s
truncated-history disclosure when historyTruncated is true and snapshot.entries
is empty, so null snapshotWindowStart and snapshotWindowEnd never produce
invalid dates or blank text. Update the range control and Available history
label logic in the affected UI flow to use the fallback, and add coverage for
the empty-snapshot case while preserving the retained-window label when dates
exist.
Comment on lines
+81
to
+95
| ## Field-ownership matrix (locked at plan time, audit blocker B10) | ||
|
|
||
| A field needs carry-over when all three hold: (a) user-editable somewhere in the | ||
| product, (b) absent from `ProviderPayload`, (c) registry-seeded by | ||
| `enrichProviderFromRegistry`. Carrying over a registry-only field would freeze | ||
| stale registry metadata into user config — the opposite failure — so (a) must be | ||
| confirmed by reading the editing UI, never assumed. | ||
|
|
||
| | Field | User-editable | In POST payload | Registry-seeded | Carry over? | | ||
| |---|---|---|---|---| | ||
| | `apiKeyPool` | yes | no | no | already carried | | ||
| | `modelCosts` | yes | no | no | already carried | | ||
| | `modelContextWindows` | yes (`Models.tsx:476`) | **no** | yes (`derive.ts:405`) | **add** | | ||
| | `contextWindow` | yes (`Models.tsx:475`) | **no** | yes (`derive.ts:404`) | **add** | | ||
| | `modelInputModalities`, `modelMaxOutputTokens`, `modelReasoningEfforts`, `modelDefaultReasoningEfforts`, `modelReasoningEffortMap`, `reasoningEffortMap`, `noVisionModels`, `noReasoningModels`, `noTemperatureModels`, `defaultMaxOutputTokens` | confirm against the real editing surfaces in B | no | yes | only where (a) is confirmed | |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Clarify the field-ownership rule.
Lines 83-87 state that carry-over requires registry seeding. Lines 91-92 list apiKeyPool and modelCosts as existing carry-overs even though they are not registry-seeded.
Rewrite the rule as: carry over user-editable fields absent from ProviderPayload; separately exclude fields that are registry-only. This keeps the rule consistent with the matrix and prevents future audits from omitting user-owned fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@devlog/_plan/260812_five_bug_fix_campaign/040_phase4_issue1409_context_window_overrides.md`
around lines 81 - 95, Rewrite the field-ownership rule in the “Field-ownership
matrix” section so carry-over is based on fields that are user-editable and
absent from ProviderPayload, without requiring registry seeding. State
separately that registry-only fields must be excluded, while preserving the
existing carry-over entries and matrix decisions.
6 tasks
3 tasks
lidge-jun
added a commit
that referenced
this pull request
Aug 12, 2026
…idation Rebase follow-up on top of @Ingwannu's diagnostic commit. #1531 landed between this PR being written and being rebased, and it moved the validation the diagnostic describes. The streamed path now validates function/name/arguments/id at ingest rather than only checking container and member shapes, so the diagnostic no longer skips per-member inspection in stream mode, and the three new ingest rejections log through the same helper. Blank and whitespace-only names are now rejected on the buffered path; reporting that as name_invalid would claim a type problem for a correctly-typed value, so it gets its own reason code. A diagnostic that describes a boundary the code no longer has is worse than none: it would send provider-compatibility work after the wrong shape.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
An OpenCode Zen / DeepSeek route can stream
function.argumentsdeltas while never sendingfunction.name. The pending call kept its initial empty name and every flush site emitted it anyway, so the Codex tool-call contract received a call it cannot dispatch and the streamed turn failed downstream (#1514).This makes the streamed path fail closed. An unnamed call now terminates the turn through the adapter error channel instead of being emitted.
Three details worth calling out for review:
idis already synthesized when absent, and that is safe because an id is an opaque correlation handle. A function name is a guess at intent, so it is refused instead. This matches the reference implementations:openai-pythonandopenai-nodeaccumulate such a call with an empty name and let the caller fail; LiteLLM forwards it unchanged. None invent a name.function_call_outputorphaned on the next turn. A loud terminal error preserves that invariant rather than weakening it.delta.tool_callscast is a TypeScript convenience over upstream JSON, so a truthy non-stringnamereached the accumulator unvalidated and then threw from string handling. Review caught this in the first draft of the patch (reproduced asTypeError: call.name.trim is not a function). Presentfunction,name,arguments, andidfields are now runtime-checked at ingest and route to the existinginvalidToolCallsEvent.The buffered
parseResponsevalidator checked the type ofnamebut not its emptiness, so it is tightened the same way. A completed Chat Completions tool call requires a name in the official OpenAPI schema, and a blank or whitespace-only name cannot select a dispatch target.Preserved invariants, each covered by a test: raw-EOF truncation still reports truncation rather than the new error; non-array and non-record
tool_callsstill terminate per #1325; a name arriving in a later chunk is still accepted;doneis never emitted after the new error.T7intests/openai-chat-parallel-stream.test.tspreviously asserted the empty-name call was flushed, on a "no silent drop" rationale. That invariant still holds and is strengthened — the call does not vanish, the turn fails loudly — so the test was updated to the new contract rather than deleted.T6(late-arriving name) is untouched.Verification
Run on a dedicated Linux runner (Bun 1.3.14, clean checkout of this branch):
bun x tsc --noEmit— exit 0bun test tests/openai-chat-eof.test.ts tests/openai-chat-parallel-stream.test.ts— 48 pass, 0 failtests/openai-chat*,tests/cl01-openai-chat*,tests/adapter*) — 161 pass, 0 failbun run privacy:scan— passRed-before evidence: with only the test diff applied to unmodified
dev, 5 tests failed. With the full diff, 48/48 pass.The crash path found in review was reproduced directly before and after the fix: a numeric
function.nameproducedTHREW: call.name.trim is not a functionbefore, andEVENTS: errorafter.Budget safety is asserted directly, not inferred: a stream with a valid call, an unnamed call, and a second valid call after it ends with
activeCalls: 0,currentBytes: 0, andhighWaterBytes > 0, provingcloseToolCalls()released every pending reservation including the call after the offender.Full-suite run via
bun run testwas still in progress at the time of opening; it will be reported here when it completes.Checklist
Closes #1514
Summary by CodeRabbit
Bug Fixes
Tests