diff options
| author | Adam Malczewski <[email protected]> | 2026-05-28 10:33:33 +0900 |
|---|---|---|
| committer | Adam Malczewski <[email protected]> | 2026-05-28 10:33:33 +0900 |
| commit | 2eeabc95b78f6624c187e1e3892f9413266b4b9a (patch) | |
| tree | ffdcd754bafb4da036b1fd3dfb22617754067641 /packages/core/src | |
| parent | 1e70f2d12274da833035912206b1ac0b1ee57ef1 (diff) | |
| download | dispatch-2eeabc95b78f6624c187e1e3892f9413266b4b9a.tar.gz dispatch-2eeabc95b78f6624c187e1e3892f9413266b4b9a.zip | |
fix(core): strip stale [USER INTERRUPT] from LLM history; inject into last tool of batch
The interrupt block embedded in a tool-result was persistent in the
assistant message history, so the imperative 'You MUST address these
before continuing' got re-evaluated as fresh on every subsequent LLM
step. Result: the model repeatedly thought about and re-acknowledged
the same interrupt 5-10+ times per chat (verified in production DB
traces — e.g. tab 4c5727aa had 11 thinking chunks quoting a single
interrupt verbatim).
agent.ts (toModelMessages): strip [USER INTERRUPT] from every tool-
result except the one in the freshest tool-batch (last chunk of the
last assistant message, which itself must be the last message). The
strip is a serialization-time transform only — this.messages, the DB
row, and the UI display all keep the full text. The LLM sees the
imperative exactly once: the step immediately after injection.
agent.ts (tool execution loop): batch queued messages across the
group's tool calls and inject them only into the LAST executable tool's
result. Previously the first tool to dequeue won; now the interrupt
lands in a single deterministic spot regardless of timing. Tool-level
handlers (run-shell/youtube/retrieve) are untouched — they still embed
their own interrupt text when they background work.
Also fix pre-existing tabs.test.ts: it referenced a getDescendantIds
function that didn't exist (added: BFS, leaf-first, cycle-safe, skips
archived) and imported bun:sqlite directly which vite couldn't resolve
(rewrote with a minimal FakeDatabase + vi.mock pattern matching the
rest of the suite).
Diffstat (limited to 'packages/core/src')
| -rw-r--r-- | packages/core/src/agent/agent.ts | 141 | ||||
| -rw-r--r-- | packages/core/src/db/tabs.ts | 37 |
2 files changed, 164 insertions, 14 deletions
diff --git a/packages/core/src/agent/agent.ts b/packages/core/src/agent/agent.ts index 7def41c..bb4ee7d 100644 --- a/packages/core/src/agent/agent.ts +++ b/packages/core/src/agent/agent.ts @@ -46,10 +46,65 @@ import type { * LLM, so this case only arises mid-step (where the message hasn't been * round-tripped to the LLM yet) and is benign. */ +/** + * Marker used to identify the start of a `[USER INTERRUPT]` block embedded + * in a tool result. Both the agent-level injection + * (`packages/core/src/agent/agent.ts`) and the tool-level injections in + * `run-shell`, `youtube-transcribe`, and `retrieve` use the same separator + * (`\n\n[USER INTERRUPT]`) before the boilerplate, so a single substring + * search suffices for stripping. + */ +const USER_INTERRUPT_MARKER = "\n\n[USER INTERRUPT]"; + +/** + * Remove the `[USER INTERRUPT]` block (and everything after it) from a tool + * result string. Used when a historical tool-result is being re-serialized + * for the LLM and the model has already had a chance to address that + * interrupt — leaving the imperative "You MUST address these" text in + * place causes the model to re-acknowledge the same interrupt on every + * subsequent step. + * + * The interrupt block is always appended to the end of the tool result, so + * we strip from the marker to end-of-string. + */ +function stripUserInterruptBlock(result: string): string { + const idx = result.indexOf(USER_INTERRUPT_MARKER); + if (idx === -1) return result; + return result.slice(0, idx); +} + function toModelMessages(messages: ChatMessage[], useToolPrefix?: boolean): ModelMessage[] { const result: ModelMessage[] = []; - for (const msg of messages) { - if (msg.role === "system") continue; + + // A `[USER INTERRUPT]` block in a tool-result is "fresh" — i.e., the + // model has not yet seen and responded to it — only when ALL of these + // hold: + // 1. The tool-batch is in the very last message of the history. + // 2. That message is an assistant message (a follow-up user message + // means the user moved on; the interrupt was addressed). + // 3. The tool-batch is the LAST chunk in that message (any later + // text/thinking/tool-batch in the same message represents the + // model's response to the tool results). + // + // All other interrupts get stripped from history because the imperative + // "You MUST address these" otherwise gets re-evaluated as a fresh + // instruction on every subsequent LLM step. + let freshestToolBatchMsgIdx = -1; + let freshestToolBatchChunkIdx = -1; + const lastMsgIdx = messages.length - 1; + const lastMsg = messages[lastMsgIdx]; + if (lastMsg && lastMsg.role === "assistant" && lastMsg.chunks.length > 0) { + const lastChunkIdx = lastMsg.chunks.length - 1; + const lastChunk = lastMsg.chunks[lastChunkIdx]; + if (lastChunk && lastChunk.type === "tool-batch") { + freshestToolBatchMsgIdx = lastMsgIdx; + freshestToolBatchChunkIdx = lastChunkIdx; + } + } + + for (let msgIdx = 0; msgIdx < messages.length; msgIdx++) { + const msg = messages[msgIdx]; + if (!msg || msg.role === "system") continue; if (msg.role === "user") { // User messages in our model can in theory contain non-text chunks, @@ -75,7 +130,9 @@ function toModelMessages(messages: ChatMessage[], useToolPrefix?: boolean): Mode result: string; }> = []; - for (const chunk of msg.chunks) { + for (let chunkIdx = 0; chunkIdx < msg.chunks.length; chunkIdx++) { + const chunk = msg.chunks[chunkIdx]; + if (!chunk) continue; switch (chunk.type) { case "text": parts.push({ type: "text", text: chunk.text }); @@ -92,7 +149,15 @@ function toModelMessages(messages: ChatMessage[], useToolPrefix?: boolean): Mode : {}), }); break; - case "tool-batch": + case "tool-batch": { + // Strip stale `[USER INTERRUPT]` blocks from every + // tool-batch except the freshest one (most recent + // tool-batch in the most recent assistant message). + // Without this, the imperative "You MUST address these" + // text persists in history and the model re-acknowledges + // the same interrupt verbatim on every subsequent step. + const isFreshestToolBatch = + msgIdx === freshestToolBatchMsgIdx && chunkIdx === freshestToolBatchChunkIdx; for (const entry of chunk.calls) { const toolName = useToolPrefix ? prefixToolName(entry.name) : entry.name; // v6: `input` replaces v4's `args` @@ -103,14 +168,18 @@ function toModelMessages(messages: ChatMessage[], useToolPrefix?: boolean): Mode input: entry.arguments, }); if (entry.result !== undefined) { + const resultText = isFreshestToolBatch + ? entry.result + : stripUserInterruptBlock(entry.result); trailingToolResults.push({ toolCallId: entry.id, toolName, - result: entry.result, + result: resultText, }); } } break; + } case "error": case "system": // Strip — not sent back to the LLM. @@ -902,8 +971,29 @@ export class Agent { } } - for (const tc of stepToolCalls) { - if (alreadyResolved.has(tc.id)) continue; + // Identify the index of the last tool in this batch that will + // actually execute. Queued user messages are buffered across + // this batch and injected into ONLY that tool's result, so the + // interrupt appears exactly once per step rather than fragmented + // across whichever tool happened to dequeue first. Tool-level + // interrupt handlers in run-shell/youtube-transcribe/retrieve + // still embed their own interrupt text in their return values — + // that path is independent and remains correct. + let lastExecutableIdx = -1; + for (let i = 0; i < stepToolCalls.length; i++) { + const tcAt = stepToolCalls[i]; + if (tcAt && !alreadyResolved.has(tcAt.id)) lastExecutableIdx = i; + } + + // Accumulator for messages dequeued during this batch. Drained + // only at `lastExecutableIdx`. Destructive dequeue at the queue + // level prevents the same message from appearing in subsequent + // batches. + const batchPendingInjection: { id: string; message: string; timestamp: number }[] = []; + + for (let tcIdx = 0; tcIdx < stepToolCalls.length; tcIdx++) { + const tc = stepToolCalls[tcIdx]; + if (!tc || alreadyResolved.has(tc.id)) continue; const shellOutputQueue: Array<{ data: string; stream: "stdout" | "stderr" }> = []; @@ -952,23 +1042,46 @@ export class Agent { } } - // Check for queued user messages and append them to the tool result - let finalToolResult = toolResult; + // Harvest any queued user messages but DEFER injection until + // the last tool of the batch. This collapses multiple + // queued messages into a single interrupt block on a single + // tool-result instead of fragmenting across the batch. if (this.queueCallbacks) { const queuedMsgs = this.queueCallbacks.dequeueMessages(); if (queuedMsgs.length > 0) { - const userMessages = queuedMsgs.map((m) => m.message).join("\n---\n"); - finalToolResult = { - ...toolResult, - result: `${toolResult.result}\n\n[USER INTERRUPT]\nThe user has sent you message(s) while you were working. You MUST address these before continuing with your current task:\n\n${userMessages}`, - }; + batchPendingInjection.push(...queuedMsgs); } } + let finalToolResult = toolResult; + if (tcIdx === lastExecutableIdx && batchPendingInjection.length > 0) { + const userMessages = batchPendingInjection.map((m) => m.message).join("\n---\n"); + finalToolResult = { + ...toolResult, + result: `${toolResult.result}\n\n[USER INTERRUPT]\nThe user has sent you message(s) while you were working. You MUST address these before continuing with your current task:\n\n${userMessages}`, + }; + batchPendingInjection.length = 0; + } + const trEvent: AgentEvent = { type: "tool-result", toolResult: finalToolResult }; appendEventToChunks(chunks, trEvent); yield trEvent; } + + // Safety net: if `lastExecutableIdx` was never reached (e.g., + // no tools executed because all were already resolved) but + // messages were still dequeued, surface them as a user message + // so they aren't dropped. In practice this is rare — it only + // happens when the entire batch is unavailable-tool synthesized + // errors with a message arriving in that narrow window. + if (batchPendingInjection.length > 0) { + const userMessages = batchPendingInjection.map((m) => m.message).join("\n---\n"); + this.messages.push({ + role: "user", + chunks: [{ type: "text", text: userMessages }], + }); + batchPendingInjection.length = 0; + } } // Build the final assistant message from the accumulated chunks. diff --git a/packages/core/src/db/tabs.ts b/packages/core/src/db/tabs.ts index c0ce809..928f434 100644 --- a/packages/core/src/db/tabs.ts +++ b/packages/core/src/db/tabs.ts @@ -122,3 +122,40 @@ export function archiveTab(id: string): void { $now: Date.now(), }); } + +/** + * Return the IDs of `rootId` plus every OPEN descendant tab, in leaf-first + * order (children before their parent). Archived descendants + * (`is_open = 0`) and their sub-trees are skipped — closing a parent + * shouldn't drag archived branches back into view. + * + * The starting `rootId` is always included in the result, even if no row + * with that id exists in the `tabs` table (graceful handling for stale + * references). + * + * Order matters for the cascade-close path: callers archive descendants + * leaf-first so foreign-key cleanup (messages, etc.) doesn't fail on + * partially-deleted parents. + * + * Cycle-safe: a `visited` set guards against accidental `parent_tab_id` + * loops that would otherwise spin forever. + */ +export function getDescendantIds(rootId: string): string[] { + const db = getDatabase(); + const visited = new Set<string>(); + const order: string[] = []; + const queue: string[] = [rootId]; + while (queue.length > 0) { + const id = queue.shift() as string; + if (visited.has(id)) continue; + visited.add(id); + order.push(id); + const children = db + .query("SELECT id FROM tabs WHERE parent_tab_id = $id AND is_open = 1") + .all({ $id: id }) as Array<{ id: string }>; + for (const child of children) { + if (!visited.has(child.id)) queue.push(child.id); + } + } + return order.reverse(); +} |
