fix(agents): validate tool_use exists before synthetic result creation#8294
fix(agents): validate tool_use exists before synthetic result creation#8294gavinbmoore wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Fixes openclaw#8264 The transcript repair mechanism was creating synthetic tool_result entries for tool_use_ids that didn't actually exist as valid tool_use blocks in the assistant message content. This would permanently corrupt sessions, causing HTTP 400 errors on every subsequent request. Changes: - Added validateToolCallExistsInContent() to verify tool_use block actually exists before creating synthetic tool_result - Skip synthetic result creation if tool_use block is missing or malformed (safer than creating mismatched references) - Added test case for malformed tool_use blocks (issue openclaw#8264 scenario) - Added test case for assistant messages with no valid tool_use blocks
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Validate that the tool_use block actually exists in the assistant message | ||
| // before creating a synthetic result. This prevents creating tool_result entries | ||
| // that reference non-existent tool_use_ids, which would permanently corrupt | ||
| // the session (API rejects mismatched tool_use_id references). | ||
| // See: https://github.com/clawdbot/clawdbot/issues/8264 | ||
| if (!validateToolCallExistsInContent(assistant, call.id)) { | ||
| // Skip creating synthetic result - the tool_use block is missing or malformed. | ||
| // This is safer than creating a result that would corrupt the transcript. | ||
| continue; |
There was a problem hiding this comment.
[P2] Redundant validation function duplicates existing extraction logic
extractToolCallsFromAssistant() already filters by block type and requires id to be a non-empty string, so validateToolCallExistsInContent(assistant, call.id) will always be true for any call coming from that extractor. As a result, this new check doesn’t change behavior and won’t prevent synthetic results from being created for “parsed IDs that don’t correspond to valid tool call blocks” unless toolCall IDs are sourced from somewhere else.
If the intended fix is to guard against IDs coming from a different extraction path, it may be worth either (a) validating at the source of ID extraction, or (b) removing the redundant scan and relying solely on the extractor’s invariants.
Also appears in: src/agents/session-transcript-repair.ts:79-102 (the new helper).
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/session-transcript-repair.ts
Line: 308:316
Comment:
[P2] Redundant validation function duplicates existing extraction logic
`extractToolCallsFromAssistant()` already filters by block `type` and requires `id` to be a non-empty string, so `validateToolCallExistsInContent(assistant, call.id)` will always be `true` for any `call` coming from that extractor. As a result, this new check doesn’t change behavior and won’t prevent synthetic results from being created for “parsed IDs that don’t correspond to valid tool call blocks” unless toolCall IDs are sourced from somewhere else.
If the intended fix is to guard against IDs coming from a different extraction path, it may be worth either (a) validating at the source of ID extraction, or (b) removing the redundant scan and relying solely on the extractor’s invariants.
Also appears in: `src/agents/session-transcript-repair.ts:79-102` (the new helper).
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| it("does not create synthetic result for malformed tool_use block (issue #8264)", () => { | ||
| // This test covers a session corruption scenario where a tool call ID is | ||
| // extracted but the block itself is malformed (wrong type or missing). | ||
| // Creating a synthetic result for such IDs would permanently corrupt the | ||
| // session because the API would reject the mismatched tool_use_id. | ||
| const input = [ | ||
| { | ||
| role: "assistant", | ||
| content: [ | ||
| { type: "text", text: "thinking..." }, | ||
| // Malformed: has id but wrong type (should be toolCall/toolUse/functionCall) | ||
| { type: "text", id: "call_malformed", text: "this is not a tool call" }, | ||
| // Valid tool call | ||
| { type: "toolCall", id: "call_valid", name: "read", arguments: {} }, | ||
| ], |
There was a problem hiding this comment.
[P1] New test case doesn’t exercise the described corruption path
In the “malformed tool_use block” test, the malformed block uses type: "text" with an id, but extractToolCallsFromAssistant() ignores non-tool-call block types, so call_malformed is never treated as a tool call and the new validation is never hit. This test will pass even if the bug described in #8264 regresses.
To cover the real scenario, the input needs to make extractToolCallsFromAssistant() return an ID that won’t survive the “properly-formed block” check (e.g., if the extractor is broader than it should be, or if the malformed block is still tagged as a tool call type but violates other required shape that the API enforces).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/session-transcript-repair.test.ts
Line: 116:130
Comment:
[P1] New test case doesn’t exercise the described corruption path
In the “malformed tool_use block” test, the malformed block uses `type: "text"` with an `id`, but `extractToolCallsFromAssistant()` ignores non-tool-call block types, so `call_malformed` is never treated as a tool call and the new validation is never hit. This test will pass even if the bug described in #8264 regresses.
To cover the real scenario, the input needs to make `extractToolCallsFromAssistant()` return an ID that *won’t* survive the “properly-formed block” check (e.g., if the extractor is broader than it should be, or if the malformed block is still tagged as a tool call type but violates other required shape that the API enforces).
How can I resolve this? If you propose a fix, please make it concise.
|
We've been investigating the underlying cause of the tool result loss bug and submitted a fix in #13746. The root cause is a race condition: pi-agent-core's This PR addresses a symptom but not the timing issue that causes tool results to be lost in the first place. The fix in #13746 adds See detailed analysis in our comment on #8643. |
|
Closing as duplicate of #8345. If this is incorrect, comment and we can reopen. |
3 participants
Summary
Fixes #8264
The transcript repair mechanism was creating synthetic
tool_resultentries fortool_use_ids that didn't actually exist as validtool_useblocks in the assistant message content. This would permanently corrupt sessions, causing HTTP 400 errors on every subsequent request:Root Cause
The
repairToolUseResultPairingfunction extracts tool call IDs from assistant messages and creates synthetic error results for any missing tool results. However, if the tool call block was malformed or the extraction logic parsed IDs that don't correspond to validtoolCall/toolUse/functionCallblocks, the synthetic result would reference a non-existenttool_use_id, permanently corrupting the session.Fix
Added
validateToolCallExistsInContent()function that verifies thetool_use_idactually exists as a properly-formed tool call block (toolCall,toolUse, orfunctionCalltype) in the assistant message content before creating a synthetic result.If the tool_use block is missing or malformed, we now skip creating the synthetic result rather than creating one that would corrupt the transcript.
Changes
validateToolCallExistsInContent()validation functionTesting
Greptile Overview
Greptile Summary
This PR hardens the session transcript repair logic so it won’t insert synthetic
toolResultentries that reference atool_use_idnot present in the assistant message content, preventing permanently corrupted transcripts and repeated 400s from strict provider APIs. It adds avalidateToolCallExistsInContent()guard inrepairToolUseResultPairing()and extends the Vitest suite with new regression/edge-case coverage around malformed tool blocks and assistant turns without tool calls.Confidence Score: 3/5