Drop dangling tool calls from a chat turn's history before the model sees it - #333
Drop dangling tool calls from a chat turn's history before the model sees it#333nicklaunches wants to merge 1 commit into
Conversation
…sees it A frontend tool handler torn down mid-run leaves an assistant message whose tool call will never be answered in the agent's live thread. Every retry sends it straight back up as input.messages, and the AI SDK refuses the conversation with AI_MissingToolResultsError naming the same call id each time. Found live: one person's next three messages all failed that way, and the conversation stayed dead until they worked out for themselves to start another one. Routines already sanitize the history they seed for exactly this failure. That sanitizer moves to agents/history-sanitize.ts, re-exported from routines/run-turn.ts so the routines path is unchanged, and now runs on every built-in chat turn as well: on run, and on the clone the runtime makes before each run, which the base class hard-codes to itself. The rule for what counts as answered is the model API's own. A result answers a call only if it lands before the next user or system message, because the API walks the conversation in order and refuses it at that message. A browser handler that resolved late appended its result after the person had typed again, so the call read as answered here while every retry still failed with the same id. A call a resume is about to answer survives the pass, since run appends that result after conversion. Ids are never rewritten and the stored thread is untouched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WaHWJ1niprhBc5NzJ9pxme
kevin9327
left a comment
There was a problem hiding this comment.
Read this one closely, since the routines path used to carry its own repair and I wanted to be sure the shared sanitizer keeps that contract. Three things I could verify, in case they are useful before it lands:
-
The boundary rule is a behaviour change for routines, not a no-op. The old repair in
run-turn.tskept a call whose result arrived after a later user message (itsansweredfilter only checkedat > index);answeredWithin(server/src/agents/history-sanitize.ts:110-113) drops it, and drops the real result with it. The PR's own test pins that down:server/tests/history-sanitize.test.ts:104-129ends with["u1", "u2"], sot1is gone from the model's view as well. That may well be the right rule, but the body says the routines path "behaves exactly as before" and the CHANGELOG says "which is what routines already did". Worth stating the change explicitly? -
Treating
developerandsystemrows as boundaries is stricter than the SDK rule the comment cites. Theaicheck only breaks onuserandsystem, and it runs on the converted messages. With neitherforwardSystemMessagesnorforwardDeveloperMessagesset inbuiltInAgentConfiguration(server/src/copilot.ts:229-264), system and developer rows are removed before validation, so they are not boundaries for the model call. Today every injected system row is immediately followed by a user row (app/src/components/channels/channel-chat.tsx:359-364), so the impact is about zero, but a call answered after a lone system row would be dropped where the request would have succeeded. -
Remote AG-UI Bots bypass the sanitizer.
remoteAgentWithStandingRolereturns beforewithToolsis reached (server/src/copilot.ts:386-414) and forwardsinput.messagesuntouched (server/src/copilot.ts:589-599). A remote Bot that converts with the same SDK still receives the poisoned history. Intentional scope for this PR, or worth a follow-up?
Two smaller ones. Recording every answer position (history-sanitize.ts:83-87) means [tool(X), assistant(call X), tool(X)] now keeps the leading result and sends a tool row ahead of any call; the browser's repair-history.ts:56-73 treats exactly that shape as misplaced and relocates it, and the old pass dropped both. The test at :131-143 uses a single result, so this shape is not covered. And the hop/relay path (conversationOnly in handoff-delivery.ts:451-460 strips every tool row while keeping assistant rows that carry toolCalls) was feeding guaranteed-dangling calls into setMessages; this PR fixes that too, which seems worth a line in the body.
The "not persisted" claim holds as far as I can trace it: the runtime derives what it stores from the request body before the agent runs, and runAgent applies the pre-sanitize input, so nothing audit-relevant is lost. Nice.
The problem
Found live. Three consecutive attempts to say anything in one channel failed with:
A frontend tool handler had been torn down while its call was open, most often a tab closed or
reloaded mid-run. The agent's live messages in the browser kept an assistant message whose tool call
would never be answered, the durable store did not have that result, nothing was going to produce
it, and every retry sent the same message back up as
input.messages. The AI SDK refuses theconversation in
convertToLanguageModelPromptbefore the request ever leaves, so the channel wasfinished until the person worked out for themselves to open another one.
The routines path had already hit this and grown a sanitizer for the history it seeds. Chat had
nothing.
A second shape of the same bug: a handler that resolves late appends its result to the thread after
the person has already typed the next message. The model API walks the conversation in order and
refuses it the moment a user or system message arrives with a call still open, so that late result
answers nothing, and a sanitizer that only asks "does a result for this id exist later" declares the
call answered and lets it through.
What changed
The routines sanitizer moves to
server/src/agents/history-sanitize.tsand is re-exported fromserver/src/routines/run-turn.ts, so the routines path behaves exactly as before.BuiltInAgentWithSaneHistorywrapsBuiltInAgentand runs that sanitizer oninput.messages.The guard has to sit on this side of
run, becauseBuiltInAgent.runconverts the messages itselfwith no seam in between.
cloneis carried by hand: the runtime clones an agent before every runand the base class's clone hard-codes
new BuiltInAgent(this.config), so an inherited clone wouldsend the very first message through an unguarded agent.
The rule for "answered" is now the API's own: a result answers a call only if it lands before the
next user, system, or developer message. Calls that a
resumeentry is about to answer are kept,since
runappends those results after conversion and they would otherwise land on nothing.A dangling call is dropped rather than repaired with a synthetic result, and ids are never
rewritten. The stored thread is not modified, so the transcript still shows what actually happened.
How it was verified
server/tests/history-sanitize.test.tscovers the sanitizer directly: dangling calls dropped,answered calls kept, a resume-answered call kept, a result that arrives after a later user
message treated as no answer, and ids left alone.
server/tests/copilot.test.tscovers the wiring: the agent a request is handed, the clone theruntime makes before every run, and the narrowed path that rebuilds its agent per run.
bun test server/tests/history-sanitize.test.ts server/tests/copilot.test.tsgives 44 pass, 0fail.
bun test server/tests/routine-run-turn.test.tsgives 31 pass, 0 fail, confirming the move leftthe routines path alone.
bun run typecheckclean across app, server and worker;bunx biome checkclean on the changedfiles.
🤖 Generated with Claude Code