diff --git a/packages/agent/CHANGELOG.md b/packages/agent/CHANGELOG.md index 1de3328af..826f635d3 100644 --- a/packages/agent/CHANGELOG.md +++ b/packages/agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed a false "Skipped due to queued user message" tool result that discarded work an interruptible tool had already completed: steering while a tool was in flight aborted its signal, and a tool that then finished with a genuine error result (e.g. a command exiting non-zero) had its real output clobbered by the skip placeholder. Completed tool executions now keep their real result; only tools genuinely cut off before returning are reported as skipped ([#4752](https://github.com/can1357/oh-my-pi/issues/4752)). + ## [16.3.7] - 2026-07-05 ### Fixed diff --git a/packages/agent/src/agent-loop.ts b/packages/agent/src/agent-loop.ts index 4c8f7b64f..42731d0dd 100644 --- a/packages/agent/src/agent-loop.ts +++ b/packages/agent/src/agent-loop.ts @@ -2033,19 +2033,21 @@ async function executeToolCalls( const interrupted = interruptState.triggered; const perToolAborted = record.signal.aborted; - const abortedDuringExecution = perToolAborted && isError; - if (interrupted && perToolAborted && isError) { - // This tool's own signal fired AND it failed — it was cut off before producing - // a usable result, so report it as skipped. + const abortedDuringExecution = perToolAborted && isError && !completedToolExecution; + if (interrupted && perToolAborted && isError && !completedToolExecution) { + // This tool's own signal fired AND it failed to produce a result: `tool.execute()` + // never returned (it threw on the abort), so it was genuinely cut off before + // producing usable output. Report it as skipped. record.skipped = true; emitToolResult(record, createSkippedToolResult(), true); } else { - // No interrupt on this signal, or the tool finished (successfully or with a - // genuine error) before the interrupt landed. Keep its real result: a completed - // tool already ran its side effects, so the model must see what actually - // happened rather than a false "skipped". A peer-IRC interrupt on the batch - // leaves non-interruptible tools' signals untouched — their genuine errors - // survive here instead of being clobbered into "skipped". + // No interrupt on this signal, or the tool finished before the interrupt landed + // (`completedToolExecution`) — even if the signal aborted around completion. Keep + // its real result: a completed tool already ran its side effects, so the model must + // see what actually happened (a genuine non-zero exit / error result) rather than a + // false "skipped" that discards work the tool performed (#4752). A peer-IRC interrupt + // on the batch leaves non-interruptible tools' signals untouched — their genuine + // errors survive here too. emitToolResult(record, result, isError); } diff --git a/packages/agent/test/agent-loop.test.ts b/packages/agent/test/agent-loop.test.ts index 62ec438f4..cec34f9c4 100644 --- a/packages/agent/test/agent-loop.test.ts +++ b/packages/agent/test/agent-loop.test.ts @@ -1261,6 +1261,82 @@ describe("agentLoop with AgentMessage", () => { ).toBe(true); }); + it("keeps a completed error result instead of clobbering it into skipped when a steer aborts the signal (#4752)", async () => { + // A steer lands while an interruptible tool is in flight, aborting its shared + // signal via the mid-batch watch poll. The tool nonetheless runs to completion + // and returns a genuine error result (e.g. `ls` on a missing path exiting + // non-zero). Its real output MUST survive — not be replaced by the "Skipped due + // to queued user message" placeholder, which discards work the tool performed. + const toolSchema = type({}); + let steerReady = false; + let drained = false; + + const tool: AgentTool> = { + name: "lslike", + label: "Lslike", + description: "Completes with an error result even after its signal aborts", + parameters: toolSchema, + interruptible: true, + async execute(_toolCallId, _params, signal) { + steerReady = true; + // Wait for the steering-watch poll to abort our signal, then finish + // anyway with a real error result (no wall-clock sleep — await the abort). + if (!signal?.aborted) { + const { promise, resolve } = Promise.withResolvers(); + signal?.addEventListener("abort", () => resolve(), { once: true }); + await promise; + } + expect(signal?.aborted).toBe(true); + return { + content: [{ type: "text", text: "ls: cannot access X: No such file or directory" }], + details: {}, + isError: true, + }; + }, + }; + + const context: AgentContext = { systemPrompt: [""], messages: [], tools: [tool] }; + const mock = createMockModel({ + responses: [ + { content: [{ type: "toolCall", id: "tool-1", name: "lslike", arguments: {} }] }, + { content: ["done"] }, + ], + }); + const config: AgentLoopConfig = { + model: mock.model, + convertToLlm: identityConverter, + interruptMode: "immediate", + hasSteeringMessages: () => steerReady && !drained, + getSteeringMessages: async () => { + if (steerReady && !drained) { + drained = true; + return [createUserMessage("interrupt")]; + } + return []; + }, + }; + + const events: AgentEvent[] = []; + for await (const event of agentLoop([createUserMessage("start")], context, config, undefined, mock.stream)) { + events.push(event); + } + + const toolEnds = events.filter( + (e): e is Extract => e.type === "tool_execution_end", + ); + expect(toolEnds.length).toBe(1); + const content = toolEnds[0].result.content[0]; + expect(content?.type).toBe("text"); + if (content?.type !== "text") throw new Error("tool result must be text"); + expect(content.text).toContain("No such file or directory"); + expect(content.text).not.toContain("Skipped due to queued user message"); + expect(toolEnds[0].isError).toBe(true); + // The steer is still delivered at the injection boundary. + expect( + events.some(e => e.type === "message_start" && e.message.role === "user" && e.message.content === "interrupt"), + ).toBe(true); + }); + it("drains queued IRC interrupts by aborting an interruptible tool mid-wait", async () => { const toolSchema = type({}); let ircReady = false;