diff --git a/packages/agent/CHANGELOG.md b/packages/agent/CHANGELOG.md index 113d957a5..213a1525f 100644 --- a/packages/agent/CHANGELOG.md +++ b/packages/agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Tool calls skipped mid-batch to service a queued steering/peer interrupt now carry the `SyntheticToolResultDetails` discriminator (`source: "interrupt_skipped"`, `executed: false`), so UI/telemetry consumers can classify them as "call emitted, not executed" instead of a real tool failure ([#7199](https://github.com/can1357/oh-my-pi/issues/7199)). +- Tool calls skipped mid-batch to service queued steering/peer input now distinguish calls that never entered `tool.execute` (`SyntheticToolResultDetails`, `executed: false`) from in-flight calls that may have performed partial work (`execution: "started"`), allowing UI/telemetry consumers to render normal steering control flow without misreporting execution state ([#7199](https://github.com/can1357/oh-my-pi/issues/7199)). ## [17.2.2] - 2026-07-31 diff --git a/packages/agent/src/agent-loop.ts b/packages/agent/src/agent-loop.ts index f57fe6e6f..409b76445 100644 --- a/packages/agent/src/agent-loop.ts +++ b/packages/agent/src/agent-loop.ts @@ -2454,6 +2454,7 @@ async function executeToolCalls( let isError = false; let caughtError: unknown; let completedToolExecution = false; + let executionStarted = false; await runInActiveSpan(toolSpan, async () => { try { @@ -2487,6 +2488,7 @@ async function executeToolCalls( providerMetadata: toolCall.providerMetadata, }) : undefined; + executionStarted = true; const rawResult = await tool.execute( toolCall.id, executionArgs, @@ -2557,12 +2559,12 @@ async function executeToolCalls( const interrupted = interruptState.triggered; const perToolAborted = record.signal.aborted; 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. + if (interrupted && abortedDuringExecution) { + // This tool's own signal fired AND it failed to produce a result. The + // execution may already have performed partial work before throwing on + // abort, so preserve that distinction in the placeholder metadata. record.skipped = true; - emitToolResult(record, createSkippedToolResult(interruptState.source), true); + emitToolResult(record, createSkippedToolResult(interruptState.source, executionStarted), true); } else { // No interrupt on this signal, or the tool finished before the interrupt landed // (`completedToolExecution`) — even if the signal aborted around completion. Keep @@ -2703,7 +2705,7 @@ async function executeToolCalls( toolName: record.toolCall.name, status: "skipped", }); - emitToolResult(record, createSkippedToolResult(interruptState.source), true); + emitToolResult(record, createSkippedToolResult(interruptState.source, false), true); } } @@ -2741,6 +2743,16 @@ export interface SyntheticToolResultDetails { upstreamError?: string; } +/** + * Metadata for an interrupt-aborted call that entered `tool.execute()` but + * threw before returning a usable result. It may have performed partial work. + */ +interface InterruptedToolResultDetails { + __interrupted: true; + source: "interrupt_skipped"; + execution: "started"; +} + /** * Narrow an {@link AgentMessage} to a synthetic {@link ToolResultMessage} — * a tool_result emitted for a tool call the assistant never invoked (see @@ -2853,7 +2865,8 @@ function createToolSignalAbortedResult(signal: AbortSignal): AgentToolResult { + executionStarted: boolean, +): AgentToolResult { let reason = "pending steering message"; let blocker = "queued message"; if (source === "user") { @@ -2873,6 +2886,8 @@ function createSkippedToolResult( text: `Skipped due to ${reason}. Do not count this skipped result as completed work or verification. After the ${blocker} is handled on the next step, retry the skipped tool if it is still needed.`, }, ], - details: { __synthetic: true, source: "interrupt_skipped", executed: false }, + details: executionStarted + ? { __interrupted: true, source: "interrupt_skipped", execution: "started" } + : { __synthetic: true, source: "interrupt_skipped", executed: false }, }; } diff --git a/packages/agent/test/agent-loop.test.ts b/packages/agent/test/agent-loop.test.ts index 89dab7987..c6e574638 100644 --- a/packages/agent/test/agent-loop.test.ts +++ b/packages/agent/test/agent-loop.test.ts @@ -1500,6 +1500,11 @@ describe("agentLoop with AgentMessage", () => { expect(toolEnds.length).toBe(2); expect(toolEnds[0].isError).toBe(false); expect(toolEnds[1].isError).toBe(true); + expect(toolEnds[1].result.details).toEqual({ + __synthetic: true, + source: "interrupt_skipped", + executed: false, + }); const skippedContent = toolEnds[1].result.content[0]; expect(skippedContent?.type).toBe("text"); if (skippedContent?.type !== "text") throw new Error("skipped tool result must be text"); @@ -1692,6 +1697,66 @@ describe("agentLoop with AgentMessage", () => { ).toBe(true); }); + it("distinguishes an in-flight abort from a never-executed steering skip", async () => { + const toolSchema = type({}); + let steerReady = false; + let drained = false; + + const tool: AgentTool> = { + name: "wait", + label: "Wait", + description: "Starts work, then throws when steering aborts it", + parameters: toolSchema, + interruptible: true, + async execute(_toolCallId, _params, signal) { + steerReady = true; + if (!signal) throw new Error("missing tool abort signal"); + const aborted = Promise.withResolvers(); + if (signal.aborted) { + aborted.resolve(); + } else { + signal.addEventListener("abort", () => aborted.resolve(), { once: true }); + } + await aborted.promise; + throw new Error("aborted after partial work"); + }, + }; + + const context: AgentContext = { systemPrompt: [""], messages: [], tools: [tool] }; + const mock = createMockModel({ + responses: [ + { content: [{ type: "toolCall", id: "tool-1", name: "wait", arguments: {} }] }, + { content: ["done"] }, + ], + }); + const config: AgentLoopConfig = { + model: mock.model, + convertToLlm: identityConverter, + interruptMode: "immediate", + hasSteeringMessages: () => steerReady && !drained, + getSteeringMessages: async () => { + if (!steerReady || drained) return []; + drained = true; + return [createUserMessage("interrupt")]; + }, + }; + + const events: AgentEvent[] = []; + for await (const event of agentLoop([createUserMessage("start")], context, config, undefined, mock.stream)) { + events.push(event); + } + + const toolEnd = events.find( + (event): event is Extract => event.type === "tool_execution_end", + ); + expect(toolEnd?.result.details).toEqual({ + __interrupted: true, + source: "interrupt_skipped", + execution: "started", + }); + expect(toolEnd?.result.details).not.toHaveProperty("executed"); + }); + 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 diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index b61701f25..810f7823f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed mid-turn steering/peer-interrupt tool skips rendering as errors (red ✘, red border/text) in the TUI; the synthetic skip placeholder now renders as a neutral info card, matching its "call emitted, not executed" meaning ([#7199](https://github.com/can1357/oh-my-pi/issues/7199)). +- Fixed mid-turn steering/peer-interrupt tool skips rendering as errors (red ✘, red border/text) in the TUI; pending and in-flight interrupt placeholders now render as neutral info cards while preserving whether `tool.execute` started ([#7199](https://github.com/can1357/oh-my-pi/issues/7199)). ## [17.2.2] - 2026-07-31 diff --git a/packages/coding-agent/src/modes/components/tool-execution.ts b/packages/coding-agent/src/modes/components/tool-execution.ts index cd9add42c..bffd459e9 100644 --- a/packages/coding-agent/src/modes/components/tool-execution.ts +++ b/packages/coding-agent/src/modes/components/tool-execution.ts @@ -1419,14 +1419,18 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac } /** - * True when the settled result is the synthetic placeholder emitted for a - * tool call skipped mid-batch to service queued steering/peer input. Such a - * call never executed, so it must not render as an error (#7199). + * True for a steering/peer-interrupt placeholder. A synthetic placeholder + * identifies a call that never entered `tool.execute`; an interrupted + * placeholder identifies one that started but threw before returning usable + * output. Both are normal steering control flow and render neutrally (#7199). */ #isBenignSkip(): boolean { if (this.#isPartial || !this.#result) return false; - const details = this.#result.details as { __synthetic?: boolean; source?: string } | undefined; - return details?.__synthetic === true && details.source === "interrupt_skipped"; + const details = this.#result.details as + | { __synthetic?: boolean; __interrupted?: boolean; source?: string; execution?: string } + | undefined; + if (details?.source !== "interrupt_skipped") return false; + return details.__synthetic === true || (details.__interrupted === true && details.execution === "started"); } /** diff --git a/packages/coding-agent/test/steering-skip-render.test.ts b/packages/coding-agent/test/steering-skip-render.test.ts index 84b63d9b7..92181ab50 100644 --- a/packages/coding-agent/test/steering-skip-render.test.ts +++ b/packages/coding-agent/test/steering-skip-render.test.ts @@ -23,23 +23,25 @@ function renderSkippedEdit(details: unknown): string { } describe("mid-turn steering skip rendering", () => { - it("renders a synthetic interrupt-skip as info, not an error", async () => { + it("renders both pending and in-flight interrupt skips as info, not errors", async () => { const uiTheme = await getThemeByName("dark"); if (!uiTheme) throw new Error("dark theme missing"); const errorIcon = Bun.stripANSI(formatStatusIcon("error", uiTheme)); const infoIcon = Bun.stripANSI(formatStatusIcon("info", uiTheme)); + const skipDetails = [ + { __synthetic: true, source: "interrupt_skipped", executed: false }, + { __interrupted: true, source: "interrupt_skipped", execution: "started" }, + ]; - const rendered = renderSkippedEdit({ - __synthetic: true, - source: "interrupt_skipped", - executed: false, - }); + for (const details of skipDetails) { + const rendered = renderSkippedEdit(details); - expect(rendered).toContain(infoIcon); - expect(rendered).not.toContain(errorIcon); - // The bespoke edit error frame must be gone — a skip is not a failure. - expect(rendered).not.toContain("╭"); - expect(rendered).toContain("Skipped due to pending peer interrupt"); + expect(rendered).toContain(infoIcon); + expect(rendered).not.toContain(errorIcon); + // The bespoke edit error frame must be gone — a skip is not a failure. + expect(rendered).not.toContain("╭"); + expect(rendered).toContain("Skipped due to pending peer interrupt"); + } }, 15_000); it("still renders a genuine edit failure as an error", async () => {