fix(agents): drop orphan tool results#3622
fix(agents): drop orphan tool results#3622mickobizzle wants to merge 2 commits intoopenclaw:mainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (role === "toolResult") { | ||
| const id = extractToolResultId(message as Extract<AgentMessage, { role: "toolResult" }>); | ||
| const toolName = id ? pending.get(id) : undefined; | ||
| if (id && !pending.has(id) && dropOrphanToolResults) { | ||
| return undefined as never; | ||
| } |
There was a problem hiding this comment.
[P1] toolResult messages missing an id are still persisted, even though they're effectively "orphan".
The new orphan-drop guard only runs when extractToolResultId() returns a non-null id (if (id && !pending.has(id) && dropOrphanToolResults) ...). If a provider emits a malformed toolResult without toolCallId/toolUseId, it will still be appended and can still corrupt transcripts / trigger downstream "no tool call found" behavior. If the intent is “drop any toolResult that can’t be matched to a pending tool call”, consider also dropping when id === null (or at least when dropOrphanToolResults is enabled).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/session-tool-result-guard.ts
Line: 96:101
Comment:
[P1] `toolResult` messages missing an id are still persisted, even though they're effectively "orphan".
The new orphan-drop guard only runs when `extractToolResultId()` returns a non-null id (`if (id && !pending.has(id) && dropOrphanToolResults) ...`). If a provider emits a malformed `toolResult` without `toolCallId`/`toolUseId`, it will still be appended and can still corrupt transcripts / trigger downstream "no tool call found" behavior. If the intent is “drop any toolResult that can’t be matched to a pending tool call”, consider also dropping when `id === null` (or at least when `dropOrphanToolResults` is enabled).
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
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 AIThis is a comment left during a code review.
Path: src/agents/session-tool-result-guard.ts
Line: 98:103
Comment:
[P2] `pending.get(id)` before the orphan check is redundant and can be misleading.
`toolName` is read from `pending` even when `id` isn’t pending (the orphan-drop branch returns early anyway). It’s minor, but moving the orphan check before reading `toolName` makes the control flow clearer and avoids accidental future use of a `toolName` that didn’t come from a real pending call.
<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. |
|
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. |
thewilloftheshadow
force-pushed
the
main
branch
from
bfc1ccb to
f92900f
Compare
2 participants
Summary
Drops orphan toolResult messages that arrive without a matching pending tool call, preventing corrupted transcripts that can trigger repeated "No tool call found" loops.
Details
Why
A session can end up with a toolResult persisted without its toolCall (e.g., interrupted tool call). Replaying that session repeatedly hits the error and can burn tokens. This prevents new corruption at persistence time.
Tests
pnpm vitest src/agents/session-tool-result-guard.test.tsIssue
Closes #3621
Greptile Overview
Greptile Summary
This PR extends the session tool-result guard to optionally drop “orphan”
toolResultmessages (default enabled) that don’t correspond to a currently-pending tool call, and threads the option through the guard wrapper. It also adds a vitest case asserting that an orphantoolResultis ignored.Overall this fits the existing guard’s responsibility (normalizing transcript/tool-call pairing and optionally synthesizing missing results) and should help prevent persistence-time corruption loops when replaying sessions.
Confidence Score: 4/5
toolResultmessages without an id are still persisted, which may still allow corruption in some edge cases, but the default orphan-drop should mitigate the reported loop for id-bearing orphans.(2/5) Greptile learns from your feedback when you react with thumbs up/down!
Context used:
dashboard- CLAUDE.md (source)dashboard- AGENTS.md (source)