From 4130feec9982719ff1c54463050cb14f7333d339 Mon Sep 17 00:00:00 2001 From: Nick Launces <1409277+nicklaunches@users.noreply.github.com> Date: Tue, 1 Sep 2026 22:08:40 -0700 Subject: [PATCH] Drop dangling tool calls from a chat turn's history before the model 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, and now runs on every chat turn as well: on a built-in Bot's run, on the clone the runtime makes before each run, which the base class hard-codes to itself, and in the middleware that forwards a remote Bot's messages to its endpoint, since a framework there converting with the same SDK refuses the same conversation. The rule for what counts as answered is the model API's own, and it is stricter than the one routines had. A result answers a call only if it lands after the call and before the next user 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 under the old rule while every retry still failed with the same id; a routine seeding such a history used to keep both halves and fail, and now drops both and runs. Only a user row is a boundary: the API also stops at a system row, but it checks the converted messages, and the runtime drops system and developer rows on the way there unless a forwarding flag this deployment never sets is on, so a call answered after a skill's system row is answered as far as the model call is concerned. A tool result survives only as the answer to a surviving call, so one that sits ahead of its own call, or a second answer to a call already answered, is dropped rather than sent where no provider accepts it. 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. The relay path gets the same fix for free. A Bot answering a relayed question is seeded the asking conversation with every tool row stripped and every assistant row that said something kept, tool calls and all, so each of those calls was a guaranteed dangle until this pass ran. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01H6GuYL1q5gSeMRGUf5R7i9 --- CHANGELOG.md | 20 ++ server/src/agents/history-sanitize.ts | 180 ++++++++++++++++ server/src/copilot.ts | 87 +++++++- server/src/routines/run-turn.ts | 103 +-------- server/tests/copilot.test.ts | 220 +++++++++++++++++++ server/tests/history-sanitize.test.ts | 291 ++++++++++++++++++++++++++ 6 files changed, 799 insertions(+), 102 deletions(-) create mode 100644 server/src/agents/history-sanitize.ts create mode 100644 server/tests/history-sanitize.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index d446e0438..ee1efd803 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,26 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A conversation is no longer stuck after a tool call went unanswered + +A tool that runs in the browser can be torn down while its call is still open, most often because +the tab was closed or reloaded mid-run. The call stayed in the thread with no result, every retry +sent it back up, and the model API refused the whole conversation with `Tool result is missing for +tool call ...`. The next three things the person typed failed identically, and the only way out was +to notice that and start another channel. + +A chat turn now drops a tool call nothing is going to answer before the conversation reaches the +model, on a built-in Bot and on a remote one alike. Routines already did this for the history they +seed, and now share the one filter, with a stricter rule than theirs was: a result counts as an +answer only if it arrives before the next thing the person said, matching what the model API +enforces, so a handler that resolves after the person has typed again no longer looks like an +answer. A routine's seeded history that held such a late result used to keep both halves and fail +at the model; it now drops both and runs. A result that sits ahead of its own call, or a second +answer to a call already answered, is dropped as well rather than sent where no provider accepts +it. Ids are never rewritten and the stored thread is untouched, so the transcript still shows what +happened and a call waiting on a resume still gets its result. The same filter also covers a Bot +answering a relayed question, whose seeded conversation kept every tool call and no tool result. + ### Coworkers are made in a wizard and managed in a dialog Creating a coworker is now a three-step wizard — who it is, who may see it, then where it runs, diff --git a/server/src/agents/history-sanitize.ts b/server/src/agents/history-sanitize.ts new file mode 100644 index 000000000..0c4fa22cb --- /dev/null +++ b/server/src/agents/history-sanitize.ts @@ -0,0 +1,180 @@ +/** + * The one filter that keeps a broken conversation from being replayed at a model provider for ever. + * + * IT LIVES HERE BECAUSE BOTH TURN PATHS NEED IT. A routine's headless turn seeds history itself + * (`routines/run-turn.ts`) and a chat turn is handed history by the browser (`copilot.ts`). Both + * hand that history to a `BuiltInAgent`, which converts it and lets the model provider validate the + * call/result pairing. `run-turn.ts` imports `../copilot`, so this cannot live in either of them + * without one importing the other back. + */ +import type { Message, ToolCall } from "@ag-ui/client"; + +/** Whether a message said nothing at all — no text, no parts, nothing to show a person. */ +function isSilent(message: Message): boolean { + const content = (message as { content?: unknown }).content; + if (content === undefined || content === null) return true; + if (typeof content === "string") return content.length === 0; + if (Array.isArray(content)) return content.length === 0; + return false; +} + +/** + * Refuse to re-present a conversation the model API will reject. + * + * FOUND IN PRODUCTION, TWICE, ON BOTH PATHS. First on routines: two firings of one routine, fifteen + * minutes apart, both failed with `Tool result is missing for tool call + * call_TTbiXzJVNifQt8ioU1JJmj4S.` — the SAME call id both times, so it did not come from the live + * turn: the channel's Intelligence thread held an assistant message carrying a tool call whose + * result message never landed, because an earlier CHAT turn was interrupted mid-call. Then on chat + * itself: `AI_MissingToolResultsError: Tool result is missing for tool call + * chatcmpl-tool-8dd56dc7497c5ea9`, thrown three times in a row on one person's next three attempts + * to say anything. There the damaged message was not even durable: a frontend tool handler was torn + * down mid-run, so the live agent's messages in the browser held the call, the store did not, and + * every retry sent the same unanswerable call back up as `input.messages`. + * + * A model provider validates call/result pairing, so one historical dangle poisons EVERY later turn + * that replays it: on routines until the fatigue rule disables the routine, and on chat until the + * person works out for themselves that the conversation is dead and starts another one. A permanent + * failure grown out of transient damage, and nothing the person did wrong. + * + * WHY DROPPING IS THE RIGHT ANSWER, and not repair. History here is CONTEXT for a turn, not a + * transaction to resume. A dangling call is already permanently unanswerable — the tool run that + * would have answered it ended when that turn did, and there is no result to invent. The only two + * options are to seed a conversation the API refuses, or to seed the same conversation minus a call + * that never completed. The second one loses a fragment of an interrupted exchange; the first one + * takes the conversation away. + * + * WHAT THIS DOES NOT DO. It does not DELETE anything from the platform. The thread still holds every + * row and the person still sees the interrupted exchange in their channel. This is a read-side + * filter on one turn's input and nothing more. + * + * IDS ARE NEVER CHANGED, which is what keeps `persistedInputMessages`' id-subtraction in + * `run-turn.ts` correct: a message this pass stripped a tool call from keeps its id and is still + * subtracted out as historic, and a message it dropped was never a candidate to persist. So + * sanitizing cannot turn a firing into one that re-persists the transcript. + * + * The rules, in order: + * 1. A tool call is ANSWERED if a later message carries it as `toolCallId` before the next user + * message, or if the caller says it is answered elsewhere. Later, not merely present: a result + * ahead of its call is not a pairing any provider accepts either. And before the next user + * message, because that is where the model API stops looking: see `boundaryAfter` below. + * 2. An assistant message keeps only its answered calls. If that leaves it with no calls and + * nothing said, the message is dropped — an empty assistant husk is itself invalid for some + * providers, so stripping the call is not enough. + * 3. A tool result survives only as THE answer to a surviving call: the first one after the call + * and before the boundary. Everything else carrying a `toolCallId` is dropped — a result whose + * call is gone (the mirror-image dangle an interruption leaves in the other order), a result that + * sits ahead of its own call even when a real answer follows later, and a second answer to a + * call already answered. The browser's `repair-history.ts` calls those shapes misplaced and + * relocates them; here they are dropped, because the real answer is already in place. + * + * Order is preserved, the input array is not mutated, and a message the pass does not change is + * returned as the same object — a healthy thread, which is nearly all of them, goes through + * untouched rather than through a re-normalization that could quietly differ. + * + * @param answeredElsewhere Call ids that are about to be answered by something this history cannot + * see, and so must survive. That is the interrupt resume: `BuiltInAgent.run` appends a tool result + * per `input.resume` entry, keyed by `interruptId`, AFTER converting the messages + * (`@copilotkit/runtime/dist/agent/index.mjs`, the `resumeEntries` block in `run`). Dropping the + * call that the resume answers would turn a resumable interrupt into an orphaned result, which is + * the same error seen from the other side. + */ +export function sanitizeSeededHistory( + history: Message[], + answeredElsewhere: ReadonlySet = new Set(), +): Message[] { + /* + * Where a call's answer may still land: before the next thing a person said. + * + * The model API walks the CONVERTED conversation in order and refuses it the moment a user or + * system message arrives while a call is still unanswered (`ai`'s `MissingToolResultsError`, in + * `convertToLanguageModelPrompt`). So a result that turns up after a later user message does not + * answer anything, however real it was. This happened live: a browser tool handler resolved late, + * its result was appended after the person had already typed the next message, and the call read + * as answered here while the API still threw on every retry. + * + * ONLY A USER ROW IS A BOUNDARY, though the API also stops at a system row. That is because it + * checks the converted messages, and `BuiltInAgent` drops every `system` and `developer` row on + * the way there unless `forwardSystemMessages` or `forwardDeveloperMessages` is set, which this + * deployment never does (`builtInAgentConfiguration` in `copilot.ts`). A skill's instructions + * arrive as a system row the browser inserts ahead of the person's message, so a call answered + * after one of those is answered as far as the model call is concerned, and treating the row as + * a boundary would drop a call the request would have carried fine. If either forwarding flag is + * ever turned on, that role becomes a boundary here too. + */ + const boundaryAfter: number[] = new Array(history.length).fill( + history.length, + ); + for ( + let index = history.length - 1, next = history.length; + index >= 0; + index -= 1 + ) { + boundaryAfter[index] = next; + const { role } = history[index] as { role?: string }; + if (role === "user") next = index; + } + + /** Where each call was made: the first assistant row carrying its id. */ + const callAt = new Map(); + for (const [index, message] of history.entries()) { + const { toolCalls } = message as { toolCalls?: ToolCall[] }; + for (const call of toolCalls ?? []) { + if (!callAt.has(call.id)) callAt.set(call.id, index); + } + } + /* + * The one position that answers each call: the first result after the call and before the + * boundary. Recording every position instead would let a result AHEAD of its call survive on + * the strength of a real answer behind it, and send a `tool` row before any call — the exact + * shape the browser's `repair-history.ts` treats as misplaced. + */ + const answerAt = new Map(); + for (const [index, message] of history.entries()) { + const { toolCallId } = message as { toolCallId?: string }; + if (toolCallId === undefined || answerAt.has(toolCallId)) continue; + const called = callAt.get(toolCallId); + if (called === undefined || index <= called) continue; + if (index >= (boundaryAfter[called] ?? history.length)) continue; + answerAt.set(toolCallId, index); + } + + const surviving = new Set(); + const kept: (Message | undefined)[] = history.map((message) => { + const { toolCalls } = message as { toolCalls?: ToolCall[] }; + if (toolCalls === undefined) return message; + + const answered = toolCalls.filter( + (call) => answeredElsewhere.has(call.id) || answerAt.has(call.id), + ); + for (const call of answered) surviving.add(call.id); + + // The husk check goes FIRST so it also catches a row that arrived with no calls and nothing + // said — the same invalid shape, reached without a dangle. + if (answered.length === 0 && isSilent(message)) return undefined; + // The healthy path, and the only one that returns the very same object. + if (answered.length === toolCalls.length) return message; + + /* + * Cast for the same reason `toAgentMessage` casts: `Message` is a union discriminated on `role`, + * and a spread over the union widens past every branch of it. Neither rewrite here can change + * the role or the shape — one narrows the `toolCalls` array, the other removes the key — so + * there is nothing to narrow against and nothing that could stop being a `Message`. + */ + if (answered.length > 0) { + return { ...message, toolCalls: answered } as Message; + } + // Text it did say, minus a call it cannot complete. + const { toolCalls: _dropped, ...rest } = message as Message & { + toolCalls?: ToolCall[]; + }; + return rest as Message; + }); + + return kept.filter((message, index): message is Message => { + if (message === undefined) return false; + const { toolCallId } = message as { toolCallId?: string }; + if (toolCallId === undefined) return true; + return surviving.has(toolCallId) && answerAt.get(toolCallId) === index; + }); +} diff --git a/server/src/copilot.ts b/server/src/copilot.ts index 547e329a7..e850efb7b 100644 --- a/server/src/copilot.ts +++ b/server/src/copilot.ts @@ -14,6 +14,7 @@ import { COMPUTER_GUIDANCE, PROVENANCE_GUIDANCE, } from "../../shared/bot-prompt"; +import { sanitizeSeededHistory } from "./agents/history-sanitize"; import type { AgentActor } from "./agents/profile-types"; import type { AgentFetch, StallGuard } from "./channels/stall-guard"; import type { DeploymentConfig } from "./config"; @@ -419,7 +420,7 @@ async function buildAgent( * what keeps a narrowed run from being told it holds something it was not offered. */ const withTools = (tools: GrantedTool[]) => - new BuiltInAgent( + new BuiltInAgentWithSaneHistory( builtInAgentConfiguration( agent, model, @@ -586,15 +587,29 @@ function remoteAgentWithStandingRole( next: AbstractAgent, ) => { const holdingsMessage = holdingsMessageFor(tools); + /* + * The same guard a built-in Bot gets in `BuiltInAgentWithSaneHistory`, applied here because a + * remote Bot never passes through it: this middleware is the last thing between the browser's + * `input.messages` and the endpoint. A framework at the other end that converts with the same + * SDK refuses a dangling call for the same reason, and one that does not would still be + * shown a call nothing is going to answer. Done inside the middleware rather than by wrapping + * the agent, for the reason given above `remoteAgentWithStandingRole`: `run` skips `.use()`. + */ + const answeredByResume = new Set( + (input.resume ?? []).map((entry) => entry.interruptId), + ); return next.run({ ...input, messages: [ agent.standingMessage, ...(holdingsMessage ? [holdingsMessage] : []), - ...input.messages.filter( - (message) => - message.id !== agent.standingMessage.id && - message.id !== holdingsMessage?.id, + ...sanitizeSeededHistory( + input.messages.filter( + (message) => + message.id !== agent.standingMessage.id && + message.id !== holdingsMessage?.id, + ), + answeredByResume, ), ], /* @@ -666,6 +681,68 @@ function remoteAgentWithStandingRole( return remote; } +/** + * A built-in Bot that will not hand the model provider a conversation it is going to refuse. + * + * FOUND LIVE, ON CHAT. One person's next three messages each failed with + * `AI_MissingToolResultsError: Tool result is missing for tool call chatcmpl-tool-8dd56dc7497c5ea9`, + * thrown out of the AI SDK's `convertToLanguageModelPrompt`. A frontend tool handler had been torn + * down while its call was open, so the agent's live messages in the browser carried an assistant + * message whose tool call never got a result. The durable store did not have it, nothing was going + * to answer it, and every retry sent it straight back up as `input.messages`. The conversation was + * finished until the person worked out for themselves to start another one. + * + * The guard has to be on this side of `run`. `BuiltInAgent.run` converts `input.messages` itself, + * with no seam in between, so wrapping the agent is the only place left to stand. The reasoning for + * why a dangling call is DROPPED rather than repaired, and why ids are never changed, is in + * `agents/history-sanitize.ts`, where the routines path found the same failure first. + * + * A RESUMED CALL IS NOT A DANGLE. `run` appends a tool result for each `input.resume` entry by + * `interruptId` AFTER converting the messages, so a call that a resume is about to answer must + * survive this pass or the appended result lands on nothing. + */ +class BuiltInAgentWithSaneHistory extends BuiltInAgent { + /** + * The configuration, held a second time because the base class keeps its own copy private and + * {@link clone} has to build another one of THIS class rather than of the base. + */ + private readonly configuration: BuiltInAgentConfiguration; + + constructor(configuration: BuiltInAgentConfiguration) { + super(configuration); + this.configuration = configuration; + } + + run(input: RunAgentInput): Observable { + const answeredByResume = new Set( + (input.resume ?? []).map((entry) => entry.interruptId), + ); + return super.run({ + ...input, + messages: sanitizeSeededHistory(input.messages, answeredByResume), + }); + } + + /** + * Carried by hand, for the same reason {@link RunBuiltAgent.clone} is. + * + * The runtime clones an agent before every run, and the base class's clone hard-codes + * `new BuiltInAgent(this.config)`: inherited unchanged, the very first message anybody sends + * would go through an agent that does none of the above. The middleware list is copied because + * the base clone copies it, and it is reached through a cast because `AbstractAgent` declares it + * private. Nothing registers middleware on a built-in Bot today, and this is here so that the day + * something does, it is not lost in a clone. + */ + clone(): BuiltInAgentWithSaneHistory { + const cloned = new BuiltInAgentWithSaneHistory(this.configuration); + type WithMiddlewares = { middlewares: unknown[] }; + (cloned as unknown as WithMiddlewares).middlewares = [ + ...(this as unknown as WithMiddlewares).middlewares, + ]; + return cloned; + } +} + /** * An agent whose tools are decided when the run starts, because that is the first moment anybody * knows what the run is about, and who is asking on whose behalf. diff --git a/server/src/routines/run-turn.ts b/server/src/routines/run-turn.ts index 3a62953d7..e3d5929f9 100644 --- a/server/src/routines/run-turn.ts +++ b/server/src/routines/run-turn.ts @@ -53,9 +53,9 @@ import type { BaseEvent, Message, RunAgentInput, - ToolCall, } from "@ag-ui/client"; import { EventType } from "@ag-ui/client"; +import { sanitizeSeededHistory } from "../agents/history-sanitize"; import { historyOrEmpty } from "../copilot"; import type { TurnRunner } from "./runner"; @@ -196,104 +196,13 @@ function toAgentMessage(message: ThreadHistoryMessage): Message { } as Message; } -/** Whether a message said nothing at all — no text, no parts, nothing to show a person. */ -function isSilent(message: Message): boolean { - const content = (message as { content?: unknown }).content; - if (content === undefined || content === null) return true; - if (typeof content === "string") return content.length === 0; - if (Array.isArray(content)) return content.length === 0; - return false; -} - /** - * Refuse to re-present a conversation the model API will reject. - * - * FOUND IN PRODUCTION. Two firings of one routine, fifteen minutes apart, both failed with - * `Tool result is missing for tool call call_TTbiXzJVNifQt8ioU1JJmj4S.` — the SAME call id both - * times, so it did not come from the live turn: the channel's Intelligence thread held an assistant - * message carrying a tool call whose result message never landed, because an earlier CHAT turn was - * interrupted mid-call. The seeding below hands the whole converted history to the runner, the model - * provider validates call/result pairing, and it rejects the conversation. One historical dangle - * therefore poisons EVERY future firing in that channel until the fatigue rule disables the routine: - * a permanent failure grown out of transient damage, and nothing the person did wrong. - * - * WHY DROPPING IS THE RIGHT ANSWER, and not repair. History here is CONTEXT for a turn, not a - * transaction to resume. A dangling call is already permanently unanswerable — the tool run that - * would have answered it ended when that chat turn did, and there is no result to invent. The only - * two options are to seed a conversation the API refuses, or to seed the same conversation minus a - * call that never completed. The second one loses a fragment of an interrupted exchange; the first - * one disables a routine forever. - * - * WHAT THIS DOES NOT DO. It does not DELETE anything from the platform. The thread still holds every - * row, the person still sees the interrupted exchange in their channel, and a browser turn is - * unaffected. This is a read-side filter on one turn's input and nothing more. - * - * IDS ARE NEVER CHANGED, which is what keeps `persistedInputMessages`' id-subtraction below correct: - * a message this pass stripped a tool call from keeps its id and is still subtracted out as historic, - * and a message it dropped was never a candidate to persist. So sanitizing cannot turn a firing into - * one that re-persists the transcript. - * - * The rules, in order: - * 1. A tool call is ANSWERED if some later message carries it as `toolCallId`. Later, not merely - * present: a result ahead of its call is not a pairing any provider accepts either. - * 2. An assistant message keeps only its answered calls. If that leaves it with no calls and - * nothing said, the message is dropped — an empty assistant husk is itself invalid for some - * providers, so stripping the call is not enough. - * 3. A tool result whose `toolCallId` matches no surviving call is dropped: the mirror-image dangle, - * which is what an interruption between the two rows leaves behind in the other order. - * - * Order is preserved, the input array is not mutated, and a message the pass does not change is - * returned as the same object — a healthy thread, which is nearly all of them, goes through - * untouched rather than through a re-normalization that could quietly differ. + * Re-exported from `agents/history-sanitize.ts`, where it now lives, because a chat turn needs + * it too and this module cannot be imported from `copilot.ts`, since the import already runs the + * other way. Kept as a name on this module because this is where the reasoning was found and where the + * tests that cover the seeding path still reach for it. */ -export function sanitizeSeededHistory(history: Message[]): Message[] { - /** For each answered call id, the earliest position that answers it. */ - const answeredAt = new Map(); - for (const [index, message] of history.entries()) { - const { toolCallId } = message as { toolCallId?: string }; - if (toolCallId === undefined) continue; - if (!answeredAt.has(toolCallId)) answeredAt.set(toolCallId, index); - } - - const surviving = new Set(); - const kept: (Message | undefined)[] = history.map((message, index) => { - const { toolCalls } = message as { toolCalls?: ToolCall[] }; - if (toolCalls === undefined) return message; - - const answered = toolCalls.filter((call) => { - const at = answeredAt.get(call.id); - return at !== undefined && at > index; - }); - for (const call of answered) surviving.add(call.id); - - // The husk check goes FIRST so it also catches a row that arrived with no calls and nothing - // said — the same invalid shape, reached without a dangle. - if (answered.length === 0 && isSilent(message)) return undefined; - // The healthy path, and the only one that returns the very same object. - if (answered.length === toolCalls.length) return message; - - /* - * Cast for the same reason `toAgentMessage` casts: `Message` is a union discriminated on `role`, - * and a spread over the union widens past every branch of it. Neither rewrite here can change - * the role or the shape — one narrows the `toolCalls` array, the other removes the key — so - * there is nothing to narrow against and nothing that could stop being a `Message`. - */ - if (answered.length > 0) { - return { ...message, toolCalls: answered } as Message; - } - // Text it did say, minus a call it cannot complete. - const { toolCalls: _dropped, ...rest } = message as Message & { - toolCalls?: ToolCall[]; - }; - return rest as Message; - }); - - return kept.filter((message): message is Message => { - if (message === undefined) return false; - const { toolCallId } = message as { toolCallId?: string }; - return toolCallId === undefined || surviving.has(toolCallId); - }); -} +export { sanitizeSeededHistory }; /** What a message said out loud, or nothing if it did not say anything. */ function assistantText(message: Message): string | undefined { diff --git a/server/tests/copilot.test.ts b/server/tests/copilot.test.ts index 6d4394649..5733e367e 100644 --- a/server/tests/copilot.test.ts +++ b/server/tests/copilot.test.ts @@ -1,6 +1,8 @@ import { describe, expect, spyOn, test } from "bun:test"; +import type { RunAgentInput } from "@ag-ui/client"; import { HttpAgent } from "@ag-ui/client"; import { BuiltInAgent } from "@copilotkit/runtime/v2"; +import { EMPTY } from "rxjs"; import { PROVENANCE_GUIDANCE } from "../../shared/bot-prompt"; import { buildAgents, @@ -803,3 +805,221 @@ describe("where a Bot says its answer came from", () => { } }); }); + +/** + * The dangling tool call, refused before it reaches the model provider. + * + * FOUND LIVE. Three consecutive attempts to say anything in one conversation failed with + * `AI_MissingToolResultsError: Tool result is missing for tool call chatcmpl-tool-8dd56dc7497c5ea9`. + * A frontend tool handler had been torn down while its call was open, so the browser's live agent + * messages carried an assistant message whose tool call would never be answered, and each retry sent + * it back up as `input.messages`. `BuiltInAgent.run` converts those messages itself, so the only + * place a guard can stand is in front of it, and these are the properties that say it is standing + * there: on the agent a request is handed, on the clone the runtime makes before every run, and on + * the narrowed path, which builds its agent again per run. + */ +describe("a chat turn is not sent a conversation the model API refuses", () => { + const assistant = { + id: "general-assistant", + name: "General Assistant", + type: "built_in" as const, + systemPrompt: "Be helpful.", + }; + const model = { provider: "openai" as const, defaultModel: "gpt-5.6-terra" }; + + /** The messages a run reaches `BuiltInAgent.run` with, without a model call behind them. */ + function captureRuns() { + const seen: RunAgentInput[] = []; + const spy = spyOn(BuiltInAgent.prototype, "run").mockImplementation( + (input: RunAgentInput) => { + seen.push(input); + return EMPTY; + }, + ); + return { seen, restore: () => spy.mockRestore() }; + } + + function input( + messages: unknown[], + resume?: { interruptId: string; status: "resolved" }[], + ): RunAgentInput { + return { + threadId: "thread_1", + runId: "run_1", + messages: messages as RunAgentInput["messages"], + tools: [], + context: [], + forwardedProps: {}, + state: {}, + ...(resume === undefined ? {} : { resume }), + }; + } + + const danglingCall = [ + { id: "m1", role: "user", content: "Save that." }, + { + id: "m2", + role: "assistant", + content: "Saving it.", + toolCalls: [ + { + id: "chatcmpl-tool-8dd56dc7497c5ea9", + type: "function", + function: { name: "saveDocument", arguments: "{}" }, + }, + ], + }, + { id: "m3", role: "user", content: "Did that work?" }, + ]; + + async function builtIn() { + const agents = await buildAgents([assistant], model, "openai-secret"); + return agents["general-assistant"]; + } + + test("the unanswerable call is gone from what the run converts", async () => { + const agent = await builtIn(); + const { seen, restore } = captureRuns(); + + try { + agent?.run(input(danglingCall)); + } finally { + restore(); + } + + const messages = seen[0]?.messages ?? []; + // Everything the person and the Bot said survives. Only the call nothing will ever answer is + // gone, and with it the message that carried nothing else. + expect(messages.map((message) => message.id)).toEqual(["m1", "m2", "m3"]); + expect(messages[1]).not.toHaveProperty("toolCalls"); + // And the caller's own array is untouched, because the browser goes on using it. + expect(danglingCall[1]).toHaveProperty("toolCalls"); + }); + + test("the clone the runtime runs guards it too", async () => { + // `agents[agentId].clone()` happens before every single run, and the base class's clone builds a + // plain `BuiltInAgent`. Inherited unchanged, the guard would never once be reached in production. + const agent = (await builtIn())?.clone(); + const { seen, restore } = captureRuns(); + + try { + agent?.run(input(danglingCall)); + } finally { + restore(); + } + + expect(seen[0]?.messages).toHaveLength(3); + expect(seen[0]?.messages?.[1]).not.toHaveProperty("toolCalls"); + }); + + test("a call the run is about to resume is kept", async () => { + /* + * `run` appends a tool result per `input.resume` entry, keyed by `interruptId`, AFTER converting + * the messages. So an interrupted call is the one dangle that is not a dangle: dropping it would + * leave that appended result pointing at a call no longer in the conversation, which is the same + * error arriving from the other side. + */ + const agent = await builtIn(); + const { seen, restore } = captureRuns(); + + try { + agent?.run( + input(danglingCall, [ + { interruptId: "chatcmpl-tool-8dd56dc7497c5ea9", status: "resolved" }, + ]), + ); + } finally { + restore(); + } + + expect(seen[0]?.messages?.[1]).toMatchObject({ + toolCalls: [{ id: "chatcmpl-tool-8dd56dc7497c5ea9" }], + }); + }); + + test("a remote Bot is not sent the unanswerable call either", async () => { + /* + * A remote Bot never passes through `BuiltInAgentWithSaneHistory`: its middleware forwards the + * browser's messages to the endpoint as they are. A framework there that converts with the + * same SDK refuses the same conversation, so the guard is applied in that middleware, and + * asserted on the wire. + */ + await using endpoint = fakeAgUiEndpoint(); + const agents = await buildAgents( + [ + { + id: "risk", + name: "Risk", + type: "remote_ag_ui" as const, + endpoint: endpoint.url, + standingMessage: standingRoleMessage(riskRow), + }, + ], + model, + null, + ); + + const agent = agents.risk; + agent?.setMessages(danglingCall as never[]); + await agent?.runAgent(); + + const sent = endpoint.requests.at(-1)?.messages as { + id: string; + toolCalls?: unknown[]; + }[]; + expect(sent.map((message) => message.id)).toEqual([ + "standing-role:risk", + "m1", + "m2", + "m3", + ]); + expect(sent[2]).not.toHaveProperty("toolCalls"); + }); + + test("the narrowed path is guarded, because it builds its agent the same way", async () => { + // Tool selection defers the build to the run, so this is a different agent object than the one + // the request was handed. It is built through the same `withTools`, and that is the property. + const granted = Array.from({ length: 3 }, (_, index) => ({ + ref: `drive/tool_${index}`, + name: `mcp__drive__tool_${index}`, + description: `drive tool ${index}`, + })) as never[]; + const agents = await buildAgents( + [assistant], + model, + "openai-secret", + undefined, + async () => granted, + undefined, + undefined, + undefined, + { + loadSkills: async () => [ + { + slug: "drive-audit", + title: "Drive audit", + summary: "Read documents out of Google Drive.", + tools: ["drive/tool_0"], + }, + ], + choose: async () => JSON.stringify({ skills: ["drive-audit"] }), + floor: 0, + }, + ); + const { seen, restore } = captureRuns(); + + try { + // Subscribed, because the narrowing wrapper builds the inner agent lazily on subscription. + await new Promise((resolve) => { + agents["general-assistant"] + ?.run(input(danglingCall)) + .subscribe({ complete: resolve, error: () => resolve() }); + }); + } finally { + restore(); + } + + expect(seen[0]?.messages).toHaveLength(3); + expect(seen[0]?.messages?.[1]).not.toHaveProperty("toolCalls"); + }); +}); diff --git a/server/tests/history-sanitize.test.ts b/server/tests/history-sanitize.test.ts new file mode 100644 index 000000000..450b84782 --- /dev/null +++ b/server/tests/history-sanitize.test.ts @@ -0,0 +1,291 @@ +import { describe, expect, test } from "bun:test"; +import type { Message } from "@ag-ui/client"; +import { sanitizeSeededHistory } from "../src/agents/history-sanitize"; + +/** + * The filter itself, asserted as a function rather than through either turn path. + * + * Both callers reach it through machinery of their own: a routine seeds history off the platform + * (`routine-run-turn.test.ts`) and a chat turn is handed it by the browser (`copilot.test.ts`). Those + * files assert that the guard IS applied where it has to be. The rules it applies are asserted once, + * here, because they are the same rules on both paths and neither path is a good place to enumerate + * them. + */ + +/** History rows are written in the platform's shape and cast once, as the callers do. */ +function history(rows: unknown[]): Message[] { + return rows as Message[]; +} + +function call(id: string, name = "search", args = "{}") { + return { id, type: "function", function: { name, arguments: args } }; +} + +describe("sanitizeSeededHistory", () => { + test("drops an unanswered call and keeps the answered one, with all text intact", () => { + const sanitized = sanitizeSeededHistory( + history([ + { id: "m1", role: "user", content: "Look two things up." }, + { + id: "m2", + role: "assistant", + content: "Looking them up.", + toolCalls: [call("call_answered"), call("call_dangling")], + }, + { + id: "m3", + role: "tool", + content: "found x", + toolCallId: "call_answered", + }, + { id: "m4", role: "assistant", content: "Here is x." }, + ]), + ); + + expect(sanitized.map((message) => message.id)).toEqual([ + "m1", + "m2", + "m3", + "m4", + ]); + expect(sanitized[1]).toMatchObject({ + content: "Looking them up.", + toolCalls: [call("call_answered")], + }); + }); + + test("drops an assistant message whose only content was a dangling call", () => { + // An assistant row with neither text nor tool calls is itself invalid for some providers, so + // stripping the call is not enough: the husk has to go too. + const sanitized = sanitizeSeededHistory( + history([ + { id: "m1", role: "user", content: "Look it up." }, + { id: "m2", role: "assistant", toolCalls: [call("call_dangling")] }, + { id: "m3", role: "user", content: "Anything?" }, + ]), + ); + + expect(sanitized.map((message) => message.id)).toEqual(["m1", "m3"]); + }); + + test("keeps text a message did say, minus the call it cannot complete", () => { + const sanitized = sanitizeSeededHistory( + history([ + { + id: "m1", + role: "assistant", + content: "Let me check.", + toolCalls: [call("call_dangling")], + }, + ]), + ); + + expect(sanitized).toHaveLength(1); + expect(sanitized[0]).toEqual({ + id: "m1", + role: "assistant", + content: "Let me check.", + } as unknown as Message); + }); + + test("drops an orphaned tool result", () => { + // The mirror-image dangle: a result whose call is not in the history at all. + const sanitized = sanitizeSeededHistory( + history([ + { id: "m1", role: "user", content: "Hello." }, + { id: "m2", role: "tool", content: "left over", toolCallId: "gone" }, + { id: "m3", role: "assistant", content: "Hello back." }, + ]), + ); + + expect(sanitized.map((message) => message.id)).toEqual(["m1", "m3"]); + }); + + test("a result that lands after a later user message answers nothing", () => { + const history = [ + { id: "u1", role: "user", content: "do it" }, + { + id: "a1", + role: "assistant", + content: "", + toolCalls: [ + { + id: "late", + type: "function", + function: { name: "x", arguments: "{}" }, + }, + ], + }, + { id: "u2", role: "user", content: "also this" }, + { + id: "t1", + role: "tool", + toolCallId: "late", + content: "arrived too late", + }, + ] as unknown as Message[]; + const out = sanitizeSeededHistory(history); + expect(out.map((m) => m.id)).toEqual(["u1", "u2"]); + }); + + test("a result ahead of its own call answers nothing", () => { + // Position, not mere presence: no provider accepts a result that arrives before the call it + // belongs to, so a history in that order is still a history to be repaired. + const sanitized = sanitizeSeededHistory( + history([ + { id: "m1", role: "tool", content: "early", toolCallId: "call_1" }, + { id: "m2", role: "assistant", toolCalls: [call("call_1")] }, + ]), + ); + + expect(sanitized).toEqual([]); + }); + + test("a result that lands after a system row still answers its call", () => { + /* + * The model API stops at a user OR a system row, but it checks the CONVERTED messages, and a + * built-in Bot drops every system row on the way there because neither forwarding flag is set. + * A skill's instructions arrive as exactly such a row, inserted ahead of the person's message, + * so a boundary here would drop a call the request carries fine. + */ + const sanitized = sanitizeSeededHistory( + history([ + { id: "u1", role: "user", content: "do it" }, + { id: "a1", role: "assistant", toolCalls: [call("call_1")] }, + { id: "s1", role: "system", content: "Use the audit skill." }, + { id: "t1", role: "tool", content: "done", toolCallId: "call_1" }, + { id: "u2", role: "user", content: "and then?" }, + ]), + ); + + expect(sanitized.map((message) => message.id)).toEqual([ + "u1", + "a1", + "s1", + "t1", + "u2", + ]); + }); + + test("a result ahead of its call is dropped even when a real answer follows", () => { + // The trailing result answers the call, so the call and that result survive. The leading one + // answers nothing where it sits, and a `tool` row ahead of any call is a shape a provider + // refuses on its own — the browser's `repair-history.ts` relocates it; here it is dropped. + const sanitized = sanitizeSeededHistory( + history([ + { id: "m1", role: "tool", content: "early", toolCallId: "call_1" }, + { id: "m2", role: "assistant", toolCalls: [call("call_1")] }, + { id: "m3", role: "tool", content: "on time", toolCallId: "call_1" }, + ]), + ); + + expect(sanitized.map((message) => message.id)).toEqual(["m2", "m3"]); + }); + + test("a second answer to a call already answered is dropped", () => { + const sanitized = sanitizeSeededHistory( + history([ + { id: "m1", role: "assistant", toolCalls: [call("call_1")] }, + { id: "m2", role: "tool", content: "first", toolCallId: "call_1" }, + { id: "m3", role: "tool", content: "again", toolCallId: "call_1" }, + ]), + ); + + expect(sanitized.map((message) => message.id)).toEqual(["m1", "m2"]); + }); + + test("a clean history passes through unchanged, object for object", () => { + const clean = history([ + { id: "m1", role: "user", content: "Look it up." }, + { + id: "m2", + role: "assistant", + content: "Looking it up.", + toolCalls: [call("call_1")], + }, + { id: "m3", role: "tool", content: "found", toolCallId: "call_1" }, + { id: "m4", role: "assistant", content: "Here it is." }, + ]); + const snapshot = structuredClone(clean); + + const sanitized = sanitizeSeededHistory(clean); + + // Nothing reordered, nothing rewritten, and not even reallocated, so there is no room for a + // silent normalization to creep in on the overwhelmingly common healthy-thread path. + expect(sanitized).toEqual(snapshot); + for (const [index, message] of sanitized.entries()) { + expect(message).toBe(clean[index] as Message); + } + // And the caller's array was not mutated underneath it. + expect(clean).toEqual(snapshot); + }); +}); + +/* + * The second parameter is the interrupt resume, and it exists because the runtime answers those + * calls AFTER this pass has run. Dropping one would leave the appended tool result pointing at a + * call that is no longer in the conversation: the same error, reached from the other side. + */ +describe("a call answered elsewhere survives", () => { + test("an id named by the caller is kept, and the message is the very same object", () => { + const rows = history([ + { id: "m1", role: "user", content: "Book it." }, + { + id: "m2", + role: "assistant", + content: "Asking first.", + toolCalls: [call("interrupt_1", "confirm")], + }, + ]); + + const sanitized = sanitizeSeededHistory(rows, new Set(["interrupt_1"])); + + expect(sanitized).toHaveLength(2); + expect(sanitized[1]).toBe(rows[1] as Message); + }); + + test("it also saves a message that would otherwise have been dropped as a husk", () => { + const rows = history([ + { id: "m1", role: "assistant", toolCalls: [call("interrupt_1")] }, + ]); + + expect(sanitizeSeededHistory(rows, new Set(["interrupt_1"]))).toHaveLength( + 1, + ); + // And without the resume it is the husk case again, which is what makes the parameter load-bearing. + expect(sanitizeSeededHistory(rows)).toEqual([]); + }); + + test("only the named ids are spared, alongside the ones the history answers", () => { + const sanitized = sanitizeSeededHistory( + history([ + { + id: "m1", + role: "assistant", + content: "Two things.", + toolCalls: [ + call("interrupt_1"), + call("call_answered"), + call("call_dangling"), + ], + }, + { id: "m2", role: "tool", content: "ok", toolCallId: "call_answered" }, + ]), + new Set(["interrupt_1"]), + ); + + expect(sanitized[0]).toMatchObject({ + toolCalls: [call("interrupt_1"), call("call_answered")], + }); + }); + + test("an empty set is the default, and changes nothing", () => { + const rows = history([ + { id: "m1", role: "assistant", toolCalls: [call("call_dangling")] }, + ]); + + expect(sanitizeSeededHistory(rows, new Set())).toEqual( + sanitizeSeededHistory(rows), + ); + }); +});