fix(ai,agent): close fail-open Responses tool-call identity/lifecycle edge cases - #4276
Conversation
![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: 23de1bac1e
ℹ️ 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".
| if (hasIndex(outputIndex)) { | ||
| const idxK = idxKey(outputIndex); | ||
| if (!items.has(idxK)) items.set(idxK, entry); |
There was a problem hiding this comment.
When two active function calls share an output_index and their deltas omit item_id, retaining the first index alias routes every delta to that first call rather than failing closed. For streamed-only relays whose terminal items contain empty arguments, the first tool can consequently execute arguments intended for the second while the second is rejected as malformed; after the first finalizes, its alias is also deleted and later index-only deltas cannot reach the remaining call. Treat a duplicated index as ambiguous instead of selecting one occupant.
Useful? React with 👍 / 👎.
… edge cases (#4274) processResponsesStream was fail-open for five malformed/out-of-contract Responses wire shapes, all pre-existing on 44f4e75 and verified to reproduce on the post-#4264 tree: 1. Duplicate call_id: two function_call items sharing a call_id collapsed onto one alias; the first stayed executable while the later was flagged. Now both are marked incompleteArguments (ambiguous identity). 2. item_id/call_id namespace collision: a delta keyed on a string that is one item's id and another's call_id was silently routed to the id match. Now both entries are marked ambiguous and the delta is dropped. 3. Duplicate output_item.done: emitted a second toolcall_end for an already-finalized item. Now the duplicate is ignored (no live entry). 4. Orphan output_item.done (no preceding added): emitted toolcall_end at contentIndex -1. Now it is skipped — no phantom block. 5. Two active items sharing one output_index: the second item stole the idx alias and the first (custom tool) lost its live input delta. Now the first occupant keeps the idx alias; both stay addressable by their own id/call_id. Additionally, incompleteArguments now carries a typed reason (truncated|malformed|conflicting|ambiguous) across the ToolCall contract, all providers that set the flag, and the agent-loop guidance message. The agent-loop now gives reason-specific recovery guidance instead of always suggesting "split the work" (truncation-only) for a malformed, conflicting, or ambiguous-identity failure. The boolean remains the primary signal; callers that read only incompleteArguments are unaffected. Fixes #4274 Lore-id: 4274-responses-fail-closed Constraint: must not regress #4264 decoded semantic comparison or execution block Constraint: valid interleaved multi-tool streams must be preserved Constraint: backward-safe — callers reading only incompleteArguments must work Rejected: throwing on ambiguous identity | breaks single-call relay streams that recover via terminal Confidence: high Scope-risk: moderate Reversibility: trivial Tested: all 5 fail-open shapes reproduced on base then closed; 42 targeted tests pass Not-tested: live provider round-trip (offline adversarial harness only) Supersedes: none
23de1ba to
465c4cf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 465c4cf874
ℹ️ 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".
| if (existing && existing !== entry) { | ||
| existing.ambiguousIdentity = true; | ||
| entry.ambiguousIdentity = true; |
There was a problem hiding this comment.
When duplicated or colliding identities belong to custom_tool_call entries, these lines set ambiguousIdentity, but the custom-tool finalization branch never checks that flag and emits an executable tool call without incompleteArguments. Consequently, a custom tool such as apply_patch can still execute input whose identity was explicitly detected as ambiguous; propagate the ambiguous rejection to custom-tool finalization as well.
Useful? React with 👍 / 👎.
| const callK = callKey(item.call_id); | ||
| const existing = items.get(callK); |
There was a problem hiding this comment.
When an earlier item has id: "shared", a later tool has call_id: "shared", and the later tool's terminal event arrives first, this lookup checks only call:shared and misses the existing id:shared entry. The later call therefore finalizes as executable and removes its alias, after which the earlier item also finalizes cleanly, so the collision never fails closed unless an event happens to resolve the shared key first; perform the symmetric idKey(item.call_id) collision check during registration.
Useful? React with 👍 / 👎.
| * Absent when `incompleteArguments` is not set. Existing callers that read only | ||
| * `incompleteArguments` continue to work. | ||
| */ | ||
| incompleteArgumentsReason?: "truncated" | "malformed" | "conflicting" | "ambiguous"; |
There was a problem hiding this comment.
This adds a public ToolCall field and changes recovery behavior in both packages/ai and packages/agent, but the commit adds no entry under either package's ## [Unreleased] section. Add changelog entries for both affected packages so the release notes include the new contract and behavior.
AGENTS.md reference: AGENTS.md:L186-L186
Useful? React with 👍 / 👎.
1 participant
Fixes #4274
processResponsesStreamwas fail-open for five malformed/out-of-contract Responses wire shapes, all pre-existing on44f4e75b0eand verified to reproduce on the post-#4264 tree. This PR closes all five and adds a typedincompleteArgumentsReasonacross theToolCallcontract into agent-loop guidance.Reproduction matrix (all five on exact base
678e26c4)call_idambiguous)id/call_idcollisionoutput_item.donetoolcall_endemittedoutput_item.donetoolcall_endatcontentIndex: -1output_indexTyped incomplete reason
incompleteArgumentsnow carriesincompleteArgumentsReason: "truncated" | "malformed" | "conflicting" | "ambiguous". The agent-loop gives reason-specific recovery guidance instead of always suggesting "split the work". The boolean remains the primary signal; callers reading onlyincompleteArgumentsare unaffected.Contract evidence
ToolCall.incompleteArgumentsReasonadded topackages/ai/src/types.ts(optional, backward-safe)incompleteArgumentsfor truncation now setreason: "truncated":anthropic,ollama,openai-completions,openai-responses-sharedopenai-responses-sharedsets"malformed"/"conflicting"/"ambiguous"where applicablemockprovider supports the typed reason (defaults to"truncated")agent-loop.tsdeserializes the reason and gives reason-specific error messages#4264's decoded semantic comparison (isEquivalentJsonPayload) and execution block are unchangedTests
openai-responses-multi-toolcall-stream-adversarial.test.ts(one per shape)truncated/malformed/conflicting/ambiguous/backward-compat)Exact head
23de1bac1eonfix/issue-4274-responses-fail-closed, base678e26c4(origin/dev).—
[repo owner's gaebal-gajae (clawdbot) 🦞]