fix(agent-core): harden tool_use/tool_result exchange integrity - #1340
Conversation
…d tool calls - system.md: add a diagnose-before-retrying paragraph next to the existing permission-denial guidance, covering failed tool calls - permission: when the user rejects an approval on the main agent, tell the model not to re-attempt the exact same call (sub agents already had an equivalent hint)
…tool_use ids A turn that dies between a recorded tool.call and its paired tool.result (e.g. a transcript write failure mid-batch) used to leave pendingToolResultIds open forever: every later message was stranded in deferredMessages and user input was silently swallowed. - runOneTurn now defensively closes any dangling tool calls when a turn ends (completed, cancelled, or failed), synthesizing an error result that names the cause, with a warn log and a tool_exchange_abandoned telemetry event - the projector drops assistant tool calls whose id already appeared earlier (first occurrence wins): a duplicate id is wire-invalid on strict providers and not repairable by the strict resend; reported via the existing projection-repair log and telemetry - resume-side closePendingToolResults now logs what it closes (warn for a mid-history gap, info for the routine trailing interruption)
🦋 Changeset detectedLatest commit: 7335d49 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![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: c8633ca62d
ℹ️ 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 (seenToolCallIds.has(toolCall.id)) { | ||
| onAnomaly?.({ kind: 'duplicate_tool_call_dropped', toolCallId: toolCall.id }); | ||
| return false; |
There was a problem hiding this comment.
When a provider reuses a toolCall.id in a later step/turn and that later call records its own tool.result, this filter removes only the later assistant tool_use; the later result is still kept because dropOrphanToolResults checks only whether the id exists anywhere in the request. The projected history can therefore contain a tool message whose originating assistant call was removed (or have that result associated with the earlier call), so strict resend can still be rejected or show the model the wrong tool outcome; the duplicate call's paired result needs to be dropped/remapped along with the call.
Useful? React with 👍 / 👎.
RealKai42
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5316887bad
ℹ️ 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 (seenToolCallIds.has(toolCall.id)) { | ||
| onAnomaly?.({ kind: 'duplicate_tool_call_dropped', toolCallId: toolCall.id }); | ||
| return false; |
There was a problem hiding this comment.
When a provider reuses a tool call id in a later assistant step after the first exchange already has a result, this filter removes the later tool_use but leaves that step's tool result in the message list. dropOrphanToolResults only checks for any matching call id, so the second tool message is kept and strict-provider conversion can still send extra tool_result blocks for the single remaining tool_use, leaving the repaired request invalid. Please also drop/de-dupe tool results for removed duplicate ids, or keep the call/result pair you intend to preserve.
Useful? React with 👍 / 👎.
Unconditional dedup regressed providers that emit per-response counter ids (e.g. call_0 in every step) and accept their own duplicates: later tool exchanges silently vanished from the projected history, and a duplicate call's own recorded result was left dangling. - the dedupe pass is now opt-in via dedupeDuplicateToolCalls and enabled only in strictMessages, so the normal projection keeps the history the provider produced - the pass also drops every tool result after the first for an id, so no dangling tool message survives; when the kept call has no result of its own, the surviving one is reattached by the adjacency repair - kosong now classifies the Anthropic "tool_use ids must be unique" 400 as a recoverable request-structure error so it triggers the strict resend
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7335d49de4
ℹ️ 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".
| const closed = this.agent.context.closeAbandonedToolExchange( | ||
| abandonedToolResultOutput(ended), | ||
| ); |
There was a problem hiding this comment.
When a tool.result dispatch throws after tool.call.started has already been emitted (the scenario this repair targets), this path appends synthetic results directly to ContextMemory. Because it bypasses buildDispatchEvent/mapLoopEvent, SDK/TUI listeners never receive the matching tool.result; event consumers see an unterminated tool call until replay even though history was repaired. Please publish the same synthetic result events, or run the closed ids through the normal dispatcher.
Useful? React with 👍 / 👎.
1 participant
Summary
tool.calland its pairedtool.result(e.g. a transcript write failure mid-batch) leftpendingToolResultIdsopen forever — every later message was stranded indeferredMessagesand user input was silently swallowed.runOneTurnnow defensively closes dangling calls on every exit path, synthesizing an error result that names the cause (cancelled / failed + error), with a warn log and atool_exchange_abandonedtelemetry event. The teardown is itself guarded so a still-broken persistence layer cannot escalate.tool_useid recovery via the strict resend: a provider that emits twotool_useblocks with the same id (buggy providers, or per-response counter ids likecall_0) produces a request strict providers reject with "tool_useids must be unique" — previously unrecoverable. The strict-resend projection now dedupes duplicate call ids (first occurrence wins) together with their extra recorded results so no dangling tool message survives, and the error classifier recognizes the uniqueness 400 so it actually triggers the strict resend. The normal projection intentionally keeps duplicates verbatim: the lax provider that produced them accepts them, and deduping there would silently erase its later tool exchanges.closePendingToolResultsnow reports what it closes — warn for a mid-history gap (results lost before a step boundary, worth investigating), info for the routine trailing interruption closed at end of resume.Test plan
appendthat throws once on the firsttool.resultrecord shows the turn failing mid-batch and a follow-up user message being swallowed; the teardown closes both dangling calls and the message lands in historystrictMessageswiring test and a classifier test for the uniqueness 400pnpm typecheckclean inagent-coreandkosong; full suites green (agent-core 3436 passed / 215 files, kosong 1137 passed / 48 files).