From 7c6eba54677222a82a6e9c2a0571c66bcdbdaeb8 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 29 Jul 2026 04:22:31 -0400 Subject: [PATCH] fix(agents): reject cancelled foreground commands (#115694) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: 赵旺0668001248 --- src/agents/bash-tools.exec-run.ts | 25 ++++++- .../bash-tools.exec-task-wiring.test.ts | 75 +++++++++++++++++++ src/agents/bash-tools.test.ts | 12 +-- 3 files changed, 102 insertions(+), 10 deletions(-) diff --git a/src/agents/bash-tools.exec-run.ts b/src/agents/bash-tools.exec-run.ts index 6a118a1c23c4..10d04a5da7a1 100644 --- a/src/agents/bash-tools.exec-run.ts +++ b/src/agents/bash-tools.exec-run.ts @@ -2,6 +2,7 @@ * Exec tool policy, host dispatch, and process lifecycle pipeline. */ import { normalizeOptionalString } from "@openclaw/normalization-core/string-coerce"; +import { createAbortError } from "../infra/abort-signal.js"; import { type ExecHost, loadExecApprovals, @@ -548,6 +549,7 @@ export function createExecTool( let yielded = false; let yieldTimer: NodeJS.Timeout | null = null; let registeredAbortSignal: AbortSignal | null = null; + let toolAborted = false; // Tool-call abort should not kill backgrounded sessions; timeouts still must. const onAbortSignal = () => { @@ -562,6 +564,13 @@ export function createExecTool( if (yielded || run.session.backgrounded) { return; } + // Cancellation must win over foreground-to-background promotion while + // the child settles; detached background sessions keep their owner. + toolAborted = true; + if (yieldTimer) { + clearTimeout(yieldTimer); + yieldTimer = null; + } run.kill(); }; @@ -584,6 +593,14 @@ export function createExecTool( } return new Promise>((resolve, reject) => { + const rejectIfAborted = () => { + if (!toolAborted) { + return false; + } + reject(createAbortError("Tool execution was aborted", { cause: signal?.reason })); + return true; + }; + const resolveRunning = () => { cleanupToolRunListeners(); resolve({ @@ -607,7 +624,7 @@ export function createExecTool( }; const onYieldNow = () => { - if (yielded) { + if (yielded || toolAborted) { return; } if (settledOutcome) { @@ -634,7 +651,7 @@ export function createExecTool( resolveRunning(); }; - if (allowBackground && yieldWindow !== null) { + if (!toolAborted && allowBackground && yieldWindow !== null) { if (yieldWindow === 0) { onYieldNow(); } else { @@ -647,7 +664,7 @@ export function createExecTool( run.promise .then((outcome) => { cleanupToolRunListeners(); - if (yielded || run.session.backgrounded) { + if (rejectIfAborted() || yielded || run.session.backgrounded) { return; } resolve( @@ -660,7 +677,7 @@ export function createExecTool( }) .catch((err: unknown) => { cleanupToolRunListeners(); - if (yielded || run.session.backgrounded) { + if (rejectIfAborted() || yielded || run.session.backgrounded) { return; } reject(err as Error); diff --git a/src/agents/bash-tools.exec-task-wiring.test.ts b/src/agents/bash-tools.exec-task-wiring.test.ts index 531e397692be..899c5af16895 100644 --- a/src/agents/bash-tools.exec-task-wiring.test.ts +++ b/src/agents/bash-tools.exec-task-wiring.test.ts @@ -7,6 +7,7 @@ const taskTracking = vi.hoisted(() => ({ vi.mock("./bash-tools.exec-task-tracking.js", () => taskTracking); +import { getFinishedSession } from "./bash-process-registry.js"; import { createExecTool } from "./bash-tools.exec-run.js"; describe("exec background task wiring", () => { @@ -36,4 +37,78 @@ describe("exec background task wiring", () => { outcome: expect.objectContaining({ status: "completed" }), }); }); + + it.each([ + { + label: "foreground execution", + defaults: { allowBackground: false }, + args: {}, + }, + { + label: "foreground execution before the yield timer", + defaults: { allowBackground: true, backgroundMs: 60_000 }, + args: { yieldMs: 60_000 }, + }, + ])("finalizes and rejects an aborted real $label", async ({ defaults, args }) => { + const abortController = new AbortController(); + const abortReason = new Error("operator cancelled the foreground command"); + const onUpdate = vi.fn(() => abortController.abort(abortReason)); + const tool = createExecTool({ + host: "gateway", + security: "full", + ask: "off", + ...defaults, + }); + const command = + `${JSON.stringify(process.execPath)} -e ` + + `"process.stdout.write('ready\\n');setTimeout(() => {}, 30_000)"`; + + await expect( + tool.execute("abort-real-foreground", { command, ...args }, abortController.signal, onUpdate), + ).rejects.toMatchObject({ + name: "AbortError", + message: "Tool execution was aborted", + cause: abortReason, + }); + + expect(onUpdate).toHaveBeenCalledTimes(1); + expect(taskTracking.createBackgroundExecTask).not.toHaveBeenCalled(); + expect(taskTracking.finalizeBackgroundExecTask).toHaveBeenCalledWith({ + handle: null, + outcome: expect.objectContaining({ status: "failed" }), + }); + }); + + it("keeps a real background process running after its tool signal aborts", async () => { + const abortController = new AbortController(); + const tool = createExecTool({ + host: "gateway", + security: "full", + ask: "off", + allowBackground: true, + backgroundMs: 0, + }); + const command = + `${JSON.stringify(process.execPath)} -e ` + + `"setTimeout(() => process.stdout.write('background-survived\\n'), 30)"`; + const result = await tool.execute( + "abort-real-background", + { command, background: true }, + abortController.signal, + ); + + expect(result.details.status).toBe("running"); + if (result.details.status !== "running") { + throw new Error("expected a running background process"); + } + const { sessionId } = result.details; + abortController.abort(); + + await expect + .poll(() => getFinishedSession(sessionId)?.status, { + timeout: 5_000, + interval: 10, + }) + .toBe("completed"); + }); }); diff --git a/src/agents/bash-tools.test.ts b/src/agents/bash-tools.test.ts index 904756fd437b..6d1b066b1ec7 100644 --- a/src/agents/bash-tools.test.ts +++ b/src/agents/bash-tools.test.ts @@ -1086,12 +1086,12 @@ describe("exec backgrounded onUpdate suppression", () => { const abortController = new AbortController(); const onUpdateSpy = vi.fn(() => abortController.abort()); // Run a command that produces output over time. - const command = joinCommands([ - shellEcho("before-abort"), - shortDelayCmd, - shellEcho("after-abort"), - ]); - await execTool.execute(nextCallId(), { command }, abortController.signal, onUpdateSpy); + const beforeAbort = shellEcho("before-abort"); + const afterAbort = shellEcho("after-abort"); + const command = joinCommands([beforeAbort, shortDelayCmd, afterAbort]); + await expect( + execTool.execute(nextCallId(), { command }, abortController.signal, onUpdateSpy), + ).rejects.toMatchObject({ name: "AbortError" }); expect(onUpdateSpy).toHaveBeenCalledTimes(1); // Allow a tick for any straggling stdout data events. await waitOneTurn();