From bd60df3e53af85eb2ec87cec2918e4a22edf15bc Mon Sep 17 00:00:00 2001 From: Mark Goldenstein Date: Fri, 24 Apr 2026 20:56:34 -0700 Subject: [PATCH] fix: silence cron exec completion noise --- src/agents/bash-process-registry.ts | 6 ++ src/agents/bash-tools.exec-host-node.test.ts | 31 ++++++ src/agents/bash-tools.exec-host-node.ts | 4 +- src/agents/bash-tools.exec-runtime.test.ts | 97 +++++++++++++++++++ src/agents/bash-tools.exec-runtime.ts | 5 +- src/agents/bash-tools.exec.ts | 1 + src/agents/pi-embedded-runner/run/params.ts | 5 +- src/cron/isolated-agent/run-executor.ts | 9 ++ .../run.message-tool-policy.test.ts | 56 +++++++++++ src/cron/isolated-agent/run.ts | 1 + 10 files changed, 212 insertions(+), 3 deletions(-) diff --git a/src/agents/bash-process-registry.ts b/src/agents/bash-process-registry.ts index 486ced8280d..851f10e309d 100644 --- a/src/agents/bash-process-registry.ts +++ b/src/agents/bash-process-registry.ts @@ -1,4 +1,5 @@ import type { ChildProcessWithoutNullStreams } from "node:child_process"; +import type { TerminationReason } from "../process/supervisor/types.js"; import type { DeliveryContext } from "../utils/delivery-context.js"; import { createSessionSlug as createSessionSlugId } from "./session-slug.js"; @@ -51,6 +52,7 @@ export interface ProcessSession { tail: string; exitCode?: number | null; exitSignal?: NodeJS.Signals | number | null; + exitReason?: TerminationReason; exited: boolean; truncated: boolean; backgrounded: boolean; @@ -68,6 +70,7 @@ export interface FinishedSession { status: ProcessStatus; exitCode?: number | null; exitSignal?: NodeJS.Signals | number | null; + exitReason?: TerminationReason; aggregated: string; tail: string; truncated: boolean; @@ -150,10 +153,12 @@ export function markExited( exitCode: number | null, exitSignal: NodeJS.Signals | number | null, status: ProcessStatus, + exitReason?: TerminationReason, ) { session.exited = true; session.exitCode = exitCode; session.exitSignal = exitSignal; + session.exitReason = exitReason; session.tail = tail(session.aggregated, 2000); moveToFinished(session, status); } @@ -209,6 +214,7 @@ function moveToFinished(session: ProcessSession, status: ProcessStatus) { status, exitCode: session.exitCode, exitSignal: session.exitSignal, + exitReason: session.exitReason, aggregated: session.aggregated, tail: session.tail, truncated: session.truncated, diff --git a/src/agents/bash-tools.exec-host-node.test.ts b/src/agents/bash-tools.exec-host-node.test.ts index 4451a49c6da..83030c15436 100644 --- a/src/agents/bash-tools.exec-host-node.test.ts +++ b/src/agents/bash-tools.exec-host-node.test.ts @@ -148,6 +148,7 @@ let executeNodeHostCommand: typeof import("./bash-tools.exec-host-node.js").exec type MockNodeInvokeParams = { command?: string; + params?: Record; }; describe("executeNodeHostCommand", () => { @@ -276,6 +277,36 @@ describe("executeNodeHostCommand", () => { ); }); + it("suppresses node completion events when notifyOnExit is disabled", async () => { + requiresExecApprovalMock.mockReturnValue(false); + + await executeNodeHostCommand({ + command: "bun ./script.ts", + workdir: "/tmp/work", + env: {}, + security: "full", + ask: "off", + defaultTimeoutSec: 30, + approvalRunningNoticeMs: 0, + warnings: [], + agentId: "requested-agent", + sessionKey: "requested-session", + notifyOnExit: false, + }); + + expect(callGatewayToolMock).toHaveBeenNthCalledWith( + 2, + "node.invoke", + expect.anything(), + expect.objectContaining({ + command: "system.run", + params: expect.objectContaining({ + suppressNotifyOnExit: true, + }), + }), + ); + }); + it("denies timed-out inline-eval requests instead of invoking the node", async () => { detectInterpreterInlineEvalArgvMock.mockReturnValue(INLINE_EVAL_HIT); resolveApprovalDecisionOrUndefinedMock.mockResolvedValue(null); diff --git a/src/agents/bash-tools.exec-host-node.ts b/src/agents/bash-tools.exec-host-node.ts index 73e8922a8aa..27d7fd092c4 100644 --- a/src/agents/bash-tools.exec-host-node.ts +++ b/src/agents/bash-tools.exec-host-node.ts @@ -53,6 +53,7 @@ export type ExecuteNodeHostCommandParams = { approvalRunningNoticeMs: number; warnings: string[]; notifySessionKey?: string; + notifyOnExit?: boolean; trustedSafeBinDirs?: ReadonlySet; }; @@ -228,7 +229,8 @@ export async function executeNodeHostCommand( ? "allow-once" : (approvalDecision ?? undefined), runId: runId ?? undefined, - suppressNotifyOnExit: suppressNotifyOnExit === true ? true : undefined, + suppressNotifyOnExit: + suppressNotifyOnExit === true || params.notifyOnExit === false ? true : undefined, }, idempotencyKey: crypto.randomUUID(), }) satisfies Record; diff --git a/src/agents/bash-tools.exec-runtime.test.ts b/src/agents/bash-tools.exec-runtime.test.ts index 54ff1ee05bb..b2c96c3ae7b 100644 --- a/src/agents/bash-tools.exec-runtime.test.ts +++ b/src/agents/bash-tools.exec-runtime.test.ts @@ -2,6 +2,9 @@ import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; const requestHeartbeatNowMock = vi.hoisted(() => vi.fn()); const enqueueSystemEventMock = vi.hoisted(() => vi.fn()); +const supervisorMock = vi.hoisted(() => ({ + spawn: vi.fn(), +})); vi.mock("../infra/heartbeat-wake.js", () => ({ requestHeartbeatNow: requestHeartbeatNowMock, @@ -11,22 +14,38 @@ vi.mock("../infra/system-events.js", () => ({ enqueueSystemEvent: enqueueSystemEventMock, })); +vi.mock("../process/supervisor/index.js", () => ({ + getProcessSupervisor: () => ({ + spawn: supervisorMock.spawn, + }), +})); + +let markBackgrounded: typeof import("./bash-process-registry.js").markBackgrounded; let buildExecExitOutcome: typeof import("./bash-tools.exec-runtime.js").buildExecExitOutcome; let detectCursorKeyMode: typeof import("./bash-tools.exec-runtime.js").detectCursorKeyMode; let emitExecSystemEvent: typeof import("./bash-tools.exec-runtime.js").emitExecSystemEvent; let formatExecFailureReason: typeof import("./bash-tools.exec-runtime.js").formatExecFailureReason; let resolveExecTarget: typeof import("./bash-tools.exec-runtime.js").resolveExecTarget; +let runExecProcess: typeof import("./bash-tools.exec-runtime.js").runExecProcess; beforeAll(async () => { + ({ markBackgrounded } = await import("./bash-process-registry.js")); ({ buildExecExitOutcome, detectCursorKeyMode, emitExecSystemEvent, formatExecFailureReason, resolveExecTarget, + runExecProcess, } = await import("./bash-tools.exec-runtime.js")); }); +beforeEach(() => { + requestHeartbeatNowMock.mockClear(); + enqueueSystemEventMock.mockClear(); + supervisorMock.spawn.mockReset(); +}); + describe("detectCursorKeyMode", () => { it("returns null when no toggle found", () => { expect(detectCursorKeyMode("hello world")).toBe(null); @@ -295,6 +314,84 @@ describe("resolveExecTarget", () => { }); }); +describe("exec notifyOnExit suppression", () => { + async function runBackgroundedExit(params: { + reason: "manual-cancel" | "overall-timeout"; + stdout?: string; + }) { + supervisorMock.spawn.mockImplementationOnce( + async (input: { onStdout?: (chunk: string) => void }) => { + if (params.stdout) { + input.onStdout?.(params.stdout); + } + return { + runId: "run-1", + startedAtMs: Date.now(), + pid: 123, + wait: async () => { + await new Promise((resolve) => setImmediate(resolve)); + return { + reason: params.reason, + exitCode: null, + exitSignal: "SIGKILL", + durationMs: 10, + stdout: "", + stderr: "", + timedOut: params.reason === "overall-timeout", + noOutputTimedOut: false, + }; + }, + cancel: vi.fn(), + }; + }, + ); + + const run = await runExecProcess({ + command: "sleep 999", + workdir: "/tmp", + env: {}, + usePty: false, + warnings: [], + maxOutput: 1000, + pendingMaxOutput: 1000, + notifyOnExit: true, + notifyOnExitEmptySuccess: false, + sessionKey: "agent:main:main", + timeoutSec: null, + }); + markBackgrounded(run.session); + return await run.promise; + } + + it("keeps manual-cancelled no-output background execs silent", async () => { + const outcome = await runBackgroundedExit({ reason: "manual-cancel" }); + + expect(outcome.status).toBe("failed"); + expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + expect(requestHeartbeatNowMock).not.toHaveBeenCalled(); + }); + + it("notifies for manual-cancelled background execs with output", async () => { + await runBackgroundedExit({ reason: "manual-cancel", stdout: "partial output\n" }); + + expect(enqueueSystemEventMock).toHaveBeenCalledWith( + expect.stringContaining("partial output"), + expect.objectContaining({ sessionKey: "agent:main:main" }), + ); + expect(requestHeartbeatNowMock).toHaveBeenCalled(); + }); + + it("still notifies for no-output background exec timeouts", async () => { + await runBackgroundedExit({ reason: "overall-timeout" }); + + expect(enqueueSystemEventMock).toHaveBeenCalledWith( + expect.stringContaining("Exec failed"), + expect.objectContaining({ sessionKey: "agent:main:main" }), + ); + expect(requestHeartbeatNowMock).toHaveBeenCalled(); + }); +}); + describe("emitExecSystemEvent", () => { beforeEach(() => { requestHeartbeatNowMock.mockClear(); diff --git a/src/agents/bash-tools.exec-runtime.ts b/src/agents/bash-tools.exec-runtime.ts index 81554ceb17b..bc9529a454a 100644 --- a/src/agents/bash-tools.exec-runtime.ts +++ b/src/agents/bash-tools.exec-runtime.ts @@ -291,6 +291,9 @@ function maybeNotifyOnExit(session: ProcessSession, status: "completed" | "faile const output = compactNotifyOutput( tail(session.tail || session.aggregated || "", DEFAULT_NOTIFY_TAIL_CHARS), ); + if (status === "failed" && session.exitReason === "manual-cancel" && !output) { + return; + } if (status === "completed" && !output && session.notifyOnExitEmptySuccess !== true) { return; } @@ -783,7 +786,7 @@ export async function runExecProcess(opts: { timeoutSec: opts.timeoutSec, }); - markExited(session, exit.exitCode, exit.exitSignal, outcome.status); + markExited(session, exit.exitCode, exit.exitSignal, outcome.status, exit.reason); maybeNotifyOnExit(session, outcome.status); if (!session.child && session.stdin) { session.stdin.destroyed = true; diff --git a/src/agents/bash-tools.exec.ts b/src/agents/bash-tools.exec.ts index 888cecef362..00dd918b403 100644 --- a/src/agents/bash-tools.exec.ts +++ b/src/agents/bash-tools.exec.ts @@ -1628,6 +1628,7 @@ export function createExecTool( approvalRunningNoticeMs, warnings, notifySessionKey, + notifyOnExit, trustedSafeBinDirs, }); } diff --git a/src/agents/pi-embedded-runner/run/params.ts b/src/agents/pi-embedded-runner/run/params.ts index 49e4fda940e..cfc13931a91 100644 --- a/src/agents/pi-embedded-runner/run/params.ts +++ b/src/agents/pi-embedded-runner/run/params.ts @@ -109,7 +109,10 @@ export type RunEmbeddedPiAgentParams = { bootstrapPromptWarningSignaturesSeen?: string[]; /** Last shown bootstrap truncation warning signature for this session. */ bootstrapPromptWarningSignature?: string; - execOverrides?: Pick; + execOverrides?: Pick< + ExecToolDefaults, + "host" | "security" | "ask" | "node" | "notifyOnExit" | "notifyOnExitEmptySuccess" + >; bashElevated?: ExecElevatedDefaults; timeoutMs: number; runId: string; diff --git a/src/cron/isolated-agent/run-executor.ts b/src/cron/isolated-agent/run-executor.ts index 932dbce77a1..0148d7e966c 100644 --- a/src/cron/isolated-agent/run-executor.ts +++ b/src/cron/isolated-agent/run-executor.ts @@ -69,6 +69,7 @@ export function createCronPromptExecutor(params: { thinkLevel: ThinkLevel | undefined; timeoutMs: number; messageChannel: string | undefined; + deliveryRequested: boolean; resolvedDelivery: { accountId?: string; to?: string; @@ -196,6 +197,12 @@ export function createCronPromptExecutor(params: { bootstrapContextMode: params.agentPayload?.lightContext ? "lightweight" : undefined, bootstrapContextRunKind: "cron", toolsAllow: params.agentPayload?.toolsAllow, + execOverrides: params.deliveryRequested + ? undefined + : { + notifyOnExit: false, + notifyOnExitEmptySuccess: false, + }, runId: params.cronSession.sessionEntry.sessionId, requireExplicitMessageTarget: params.toolPolicy.requireExplicitMessageTarget, disableMessageTool: params.toolPolicy.disableMessageTool, @@ -263,6 +270,7 @@ export async function executeCronRun(params: { isAborted: () => boolean; thinkLevel: ThinkLevel | undefined; timeoutMs: number; + deliveryRequested: boolean; runStartedAt?: number; }): Promise { const resolvedVerboseLevel: VerboseLevel = @@ -286,6 +294,7 @@ export async function executeCronRun(params: { thinkLevel: params.thinkLevel, timeoutMs: params.timeoutMs, messageChannel: params.resolvedDelivery.channel, + deliveryRequested: params.deliveryRequested, resolvedDelivery: params.resolvedDelivery, toolPolicy: params.toolPolicy, skillsSnapshot: params.skillsSnapshot, diff --git a/src/cron/isolated-agent/run.message-tool-policy.test.ts b/src/cron/isolated-agent/run.message-tool-policy.test.ts index 85c9304d8ff..08ad99b169b 100644 --- a/src/cron/isolated-agent/run.message-tool-policy.test.ts +++ b/src/cron/isolated-agent/run.message-tool-policy.test.ts @@ -241,6 +241,7 @@ describe("runCronIsolatedAgentTurn message tool policy", () => { thinkLevel: undefined, timeoutMs: 60_000, messageChannel: "messagechat", + deliveryRequested: false, toolPolicy: { requireExplicitMessageTarget: false, disableMessageTool: false, @@ -270,6 +271,48 @@ describe("runCronIsolatedAgentTurn message tool policy", () => { }); }); + it('suppresses automatic exec completion notifications when delivery.mode is "none"', async () => { + mockRunCronFallbackPassthrough(); + resolveCronDeliveryPlanMock.mockReturnValue({ + requested: false, + mode: "none", + channel: "topicchat", + to: "room#42", + threadId: 42, + }); + resolveDeliveryTargetMock.mockResolvedValue({ + ok: true, + channel: "topicchat", + to: "room#42", + threadId: 42, + accountId: undefined, + error: undefined, + }); + + await runCronIsolatedAgentTurn({ + ...makeParams(), + job: makeMessageToolPolicyJob({ + mode: "none", + channel: "topicchat", + to: "room#42", + threadId: 42, + }), + }); + + expect(runEmbeddedPiAgentMock).toHaveBeenCalledTimes(1); + expect(runEmbeddedPiAgentMock.mock.calls[0]?.[0]).toMatchObject({ + disableMessageTool: false, + forceMessageTool: true, + messageChannel: "topicchat", + messageTo: "room#42", + messageThreadId: 42, + execOverrides: { + notifyOnExit: false, + notifyOnExitEmptySuccess: false, + }, + }); + }); + it("preserves explicit delivery targets for agent-initiated messaging when delivery.mode is none", async () => { mockRunCronFallbackPassthrough(); resolveCronDeliveryPlanMock.mockReturnValue({ @@ -414,6 +457,19 @@ describe("runCronIsolatedAgentTurn message tool policy", () => { }); }); + it("keeps automatic exec completion notifications when announce delivery is active", async () => { + mockRunCronFallbackPassthrough(); + resolveCronDeliveryPlanMock.mockReturnValue(makeAnnounceDeliveryPlan()); + + await runCronIsolatedAgentTurn({ + ...makeParams(), + job: makeAnnounceMessageToolJob(), + }); + + expect(runEmbeddedPiAgentMock).toHaveBeenCalledTimes(1); + expect(runEmbeddedPiAgentMock.mock.calls[0]?.[0]?.execOverrides).toBeUndefined(); + }); + it("disables the message tool when webhook delivery is active", async () => { await expectMessageToolDisabledForPlan({ requested: false, diff --git a/src/cron/isolated-agent/run.ts b/src/cron/isolated-agent/run.ts index 4cb619d33ce..467d5ea472e 100644 --- a/src/cron/isolated-agent/run.ts +++ b/src/cron/isolated-agent/run.ts @@ -977,6 +977,7 @@ export async function runCronIsolatedAgentTurn(params: { isAborted, thinkLevel: prepared.context.thinkLevel, timeoutMs: prepared.context.timeoutMs, + deliveryRequested: prepared.context.deliveryRequested, }); if (isAborted()) { return prepared.context.withRunSession({ status: "error", error: abortReason() });