mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 09:30:43 +00:00
fix: silence cron exec completion noise
This commit is contained in:
committed by
Peter Steinberger
parent
017252e4f8
commit
bd60df3e53
@@ -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,
|
||||
|
||||
@@ -148,6 +148,7 @@ let executeNodeHostCommand: typeof import("./bash-tools.exec-host-node.js").exec
|
||||
|
||||
type MockNodeInvokeParams = {
|
||||
command?: string;
|
||||
params?: Record<string, unknown>;
|
||||
};
|
||||
|
||||
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);
|
||||
|
||||
@@ -53,6 +53,7 @@ export type ExecuteNodeHostCommandParams = {
|
||||
approvalRunningNoticeMs: number;
|
||||
warnings: string[];
|
||||
notifySessionKey?: string;
|
||||
notifyOnExit?: boolean;
|
||||
trustedSafeBinDirs?: ReadonlySet<string>;
|
||||
};
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -1628,6 +1628,7 @@ export function createExecTool(
|
||||
approvalRunningNoticeMs,
|
||||
warnings,
|
||||
notifySessionKey,
|
||||
notifyOnExit,
|
||||
trustedSafeBinDirs,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -109,7 +109,10 @@ export type RunEmbeddedPiAgentParams = {
|
||||
bootstrapPromptWarningSignaturesSeen?: string[];
|
||||
/** Last shown bootstrap truncation warning signature for this session. */
|
||||
bootstrapPromptWarningSignature?: string;
|
||||
execOverrides?: Pick<ExecToolDefaults, "host" | "security" | "ask" | "node">;
|
||||
execOverrides?: Pick<
|
||||
ExecToolDefaults,
|
||||
"host" | "security" | "ask" | "node" | "notifyOnExit" | "notifyOnExitEmptySuccess"
|
||||
>;
|
||||
bashElevated?: ExecElevatedDefaults;
|
||||
timeoutMs: number;
|
||||
runId: string;
|
||||
|
||||
@@ -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<CronExecutionResult> {
|
||||
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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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() });
|
||||
|
||||
Reference in New Issue
Block a user