fix(agent): kept completed tool results from false skipped placeholder
The interrupt-clobber branch in executeToolCalls replaced a tool's real result with the "Skipped due to queued user message" placeholder whenever an interrupt fired, the tool's own signal aborted, and the result was an error. It ignored whether tool.execute() actually completed, so steering while a tool was in flight could discard a genuine error result (e.g. a command exiting non-zero) that the tool had already produced. Gate the clobber on !completedToolExecution so a tool that ran to completion keeps its real result; only tools cut off before returning are reported as skipped. Align the aborted telemetry status the same way. Fixes #4752
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
|
||||
@@ -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<typeof toolSchema, Record<string, never>> = {
|
||||
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<void>();
|
||||
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<AgentEvent, { type: "tool_execution_end" }> => 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;
|
||||
|
||||
Reference in New Issue
Block a user