mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-12 21:53:00 -06:00
bd897a6143
* fix: exec timeout field does not state its unit, so callers pass milliseconds
`exec.timeout` is in seconds, but its sibling `yieldMs` is in milliseconds and
the `process` tool's identically named `timeout` is also in milliseconds. Those
two tools are used together in one workflow: exec backgrounds a long command,
then process polls it.
The unit exists only in the field description, and code mode deliberately defers
descriptions - the model sees `timeout?: number` and nothing else. A caller that
guesses milliseconds passes 900000 intending 15 minutes and gets ~10 days, so
the command never times out.
Add `timeoutSeconds` as the canonical field, matching the convention already
used by the config default (`tools.exec.timeoutSeconds`) and the sibling
(`yieldMs`). Keep `timeout` as a deprecated alias; `timeoutSeconds` wins when
both are supplied. Project the new field onto `nodeExecSchema`, which
hand-picks its properties.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQojYRCUdXhcKFeu4QW5Gh
* fix(ci): satisfy test-type and dependency checks
Two CI failures from the test approach, not the change itself.
- check-test-types: TypeBox's TOptional does not surface `description` on its
static type, so the schema assertions failed under tsgo. Read it through a
narrow accessor instead.
- check-dependencies: knip flagged resolveExecTimeoutSeconds as an unused
export because only the test imported it. Move it beside the schema it
resolves, where the exec runtime imports it, so it is production-reachable.
Verified with the same commands CI runs: `run-tsgo.mjs -p
test/tsconfig/tsconfig.core.test.json` reports no errors in these files, and
knip --production reports no unused exports.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQojYRCUdXhcKFeu4QW5Gh
* fix(ci): fit the exec description budget and satisfy oxlint
- `agent-tools.schema.test.ts` enforces a 550-char budget on the exec tool's
model-facing descriptions; the two new ones pushed it to 640. Shortened both.
The rename is what makes this possible: the unit now lives in the field name,
so the description no longer has to carry it. Extended that budget test to
pin `timeoutSeconds` alongside `timeout`.
- oxlint `no-base-to-string`: the test's description accessor typed the field as
`unknown` and stringified it. Typed as `string | undefined` instead.
Verified: budget test passes (137 tests green across both suites), and all six
behavior tests still fail against origin/main.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQojYRCUdXhcKFeu4QW5Gh
* fix: remove the unit-ambiguous exec timeout field instead of deprecating it
Owner decision on the review's "ambiguous model-visible alias" finding: drop
`timeout` entirely rather than keep it as a deprecated alias.
Keeping it visible defeated the purpose. Code mode renders property names and
types and defers descriptions, so a model would still be offered a bare
`timeout` sitting next to a millisecond-based `yieldMs` and could still pick it.
A deprecation marker in a description the model never sees changes nothing.
Backward compatibility is not required here: every tool call is constructed
fresh by the model from the current schema, and a repository search found no
internal caller passing `timeout` to the exec tool. `timeoutSeconds` is now the
only exec timeout field, on both the main and node-only surfaces.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQojYRCUdXhcKFeu4QW5Gh
* fix: migrate the internal exec callers to timeoutSeconds
Removing `timeout` from the exec schema broke three real internal callers that
the earlier survey missed, plus two test files and the runtime's own reads.
`check-test-types` caught it; my local search had filtered `timeout:` matches to
lines mentioning "exec" and discarded these.
- src/auto-reply/reply/bash-command.ts
- src/auto-reply/reply/commands-diagnostics.ts
- src/auto-reply/reply/commands-export-trajectory.ts
all passed `timeout: timeoutSec` and now pass `timeoutSeconds: timeoutSec`.
- bash-tools.exec-run.ts reads the field through the same typeof narrowing the
third call site already used; `params` is loosely typed, so a raw read is
`unknown`.
- Test fixtures updated. A vitest `waitFor({ timeout })` option in
background-abort.test.ts is NOT an exec param and was left alone.
`run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json` now reports zero errors
in changed files; the 5 remaining `rejectSymlinks` errors are pre-existing on
main. 148 tests pass across the affected suites.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQojYRCUdXhcKFeu4QW5Gh
* docs: synchronize the exec config table with timeoutSeconds
Addresses the P2 finding. The tools.exec.timeoutSeconds row still described the
per-call override as `timeout` and `timeout: 0`, which no longer exist. The
only remaining bare `timeout` reference in this page is the deliberate
contrast with the process tool's millisecond field.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQojYRCUdXhcKFeu4QW5Gh
* fix: migrate the QA lifecycle caller and the shared ExecToolArgs contract
Two findings from the re-review of 6e8c3560e7, both real.
[P1] test/e2e/qa-lab/runtime/openclaw-exec-process-lifecycle.e2e.test.ts:174
still passed `timeout: 0.05`. After the removal that field is accepted and
ignored, so the call silently used the default timeout and could not produce
the asserted 50 ms `overall-timeout` result. Migrated.
[P2] ExecToolArgs in bash-tools.exec-request-preparation.ts:33 still declared
`timeout?: number` while the runtime reads only `timeoutSeconds`, offering
request-preparation users an accepted-but-inert argument. Renamed.
Why the typecheck missed both: ExecToolArgs is
`Record<string, unknown> & {...}`, and that index signature defeats excess
property checking, so a stale member produces no error at any call site. The
e2e file also sits outside the core test tsconfig I had been running locally.
Left alone deliberately: `processTool.execute({ action: "poll", timeout })` is
milliseconds and correct, and `createBashTool` in sessions/tools/bash.ts is a
separate tool surface with its own timeout.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQojYRCUdXhcKFeu4QW5Gh
* fix(test): update the node exec tool property assertion to timeoutSeconds
src/gateway/tool-resolution.exclude.test.ts:413 asserts the exact property list
of the node-forced exec tool and still expected "timeout". Renamed to match
nodeExecSchema.
Found by checks-node-compact-large-5; my local core-test tsconfig does not cover
src/gateway, so it did not surface there.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQojYRCUdXhcKFeu4QW5Gh
* fix: accept the legacy exec timeout as an undocumented alias
Removing the field from the schema does not reject it at runtime: the exec
schema accepts unknown properties, so a call built against an older schema was
accepted and then silently ignored, taking the default deadline instead of the
one it asked for.
Route every read through resolveExecTimeoutSeconds, which prefers the canonical
timeoutSeconds and falls back to a numeric timeout. The alias stays out of
execSchema, nodeExecSchema and ExecToolArgs, so no new caller can adopt it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQojYRCUdXhcKFeu4QW5Gh
* docs: update the background-process guide to timeoutSeconds
The exec parameter table and the inheritance note still taught `timeout` and
`timeout: 0`, which kept the retired spelling publicly discoverable and
contradicted the exec guide.
`poll`'s own millisecond `timeout` is left alone; it is a different tool and a
different unit, which is the collision this rename removes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQojYRCUdXhcKFeu4QW5Gh
* fix(exec): reject removed timeout field
* chore: regenerate plugin SDK API baseline
---------
Co-authored-by: Marvinthebored <262704729+Marvinthebored@users.noreply.github.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Patrick Erichsen <patrick.a.erichsen@gmail.com>
228 lines
8.5 KiB
TypeScript
228 lines
8.5 KiB
TypeScript
import { expect, test } from "vitest";
|
|
import {
|
|
getActiveBackgroundExecSessionCount,
|
|
listRunningSessions,
|
|
} from "../../../../src/agents/bash-process-registry.js";
|
|
import { resetProcessRegistryForTests } from "../../../../src/agents/bash-process-registry.test-support.js";
|
|
import { createExecTool, createProcessTool } from "../../../../src/agents/bash-tools.js";
|
|
|
|
type ExecTool = ReturnType<typeof createExecTool>;
|
|
type ProcessTool = ReturnType<typeof createProcessTool>;
|
|
type ToolResult = Awaited<ReturnType<ExecTool["execute"]>>;
|
|
type ProcessDetails = {
|
|
aggregated?: string;
|
|
exitReason?: string;
|
|
pid?: number;
|
|
sessionId?: string;
|
|
sessions?: Array<{ sessionId: string; status: string }>;
|
|
status?: string;
|
|
timedOut?: boolean;
|
|
};
|
|
|
|
const POLL_OPTIONS = { timeout: 10_000, interval: 25 };
|
|
|
|
function textOf(result: { content: Array<{ type: string; text?: string }> }): string {
|
|
return result.content.find((part) => part.type === "text")?.text ?? "";
|
|
}
|
|
|
|
function shellQuote(value: string): string {
|
|
return `'${value.replaceAll("'", process.platform === "win32" ? "''" : "'\\''")}'`;
|
|
}
|
|
|
|
function nodeEvalCommand(source: string): string {
|
|
const node = shellQuote(process.execPath);
|
|
const script = shellQuote(source);
|
|
return process.platform === "win32" ? `& ${node} -e ${script}` : `${node} -e ${script}`;
|
|
}
|
|
|
|
function requireSession(result: ToolResult): { pid: number; sessionId: string } {
|
|
const details = result.details as ProcessDetails;
|
|
expect(details.status).toBe("running");
|
|
expect(details.sessionId).toEqual(expect.any(String));
|
|
expect(details.pid).toEqual(expect.any(Number));
|
|
return { pid: details.pid as number, sessionId: details.sessionId as string };
|
|
}
|
|
|
|
function pidExists(pid: number): boolean {
|
|
try {
|
|
process.kill(pid, 0);
|
|
return true;
|
|
} catch {
|
|
return false;
|
|
}
|
|
}
|
|
|
|
async function pollTerminal(processTool: ProcessTool, sessionId: string) {
|
|
let terminal: Awaited<ReturnType<ProcessTool["execute"]>> | undefined;
|
|
await expect
|
|
.poll(async () => {
|
|
terminal = await processTool.execute(`poll-${sessionId}`, {
|
|
action: "poll",
|
|
sessionId,
|
|
timeout: 250,
|
|
});
|
|
return (terminal.details as ProcessDetails).status;
|
|
}, POLL_OPTIONS)
|
|
.not.toBe("running");
|
|
if (!terminal) {
|
|
throw new Error(`process ${sessionId} never produced a terminal result`);
|
|
}
|
|
return terminal;
|
|
}
|
|
|
|
async function clearFinished(processTool: ProcessTool, sessionId: string): Promise<void> {
|
|
const cleared = await processTool.execute(`clear-${sessionId}`, {
|
|
action: "clear",
|
|
sessionId,
|
|
});
|
|
expect(cleared.details).toMatchObject({ status: "completed" });
|
|
}
|
|
|
|
test("OpenClaw executes and controls the complete real process lifecycle", async () => {
|
|
resetProcessRegistryForTests();
|
|
const scopeKey = `agent:qa:exec-lifecycle-${process.pid}`;
|
|
const execTool = createExecTool({
|
|
host: "gateway",
|
|
security: "full",
|
|
ask: "off",
|
|
allowBackground: true,
|
|
backgroundMs: 20,
|
|
notifyOnExit: false,
|
|
scopeKey,
|
|
});
|
|
const foregroundExecTool = createExecTool({
|
|
host: "gateway",
|
|
security: "full",
|
|
ask: "off",
|
|
allowBackground: false,
|
|
notifyOnExit: false,
|
|
scopeKey,
|
|
});
|
|
const processTool = createProcessTool({ scopeKey });
|
|
const cleanupPids = new Set<number>();
|
|
|
|
try {
|
|
const shellMarker = `shell-route-${process.pid}`;
|
|
const foregroundCommand =
|
|
process.platform === "win32"
|
|
? `Write-Output -NoNewline ${shellQuote(shellMarker)}; Write-Output -NoNewline "|$env:OPENCLAW_SHELL"`
|
|
: `printf '%s' ${shellQuote(shellMarker)} && printf '|%s' "$OPENCLAW_SHELL"`;
|
|
const foreground = await foregroundExecTool.execute("foreground-shell", {
|
|
command: foregroundCommand,
|
|
});
|
|
expect(foreground.details).toMatchObject({ status: "completed", exitCode: 0 });
|
|
expect(textOf(foreground)).toContain(`${shellMarker}|exec`);
|
|
|
|
const backgroundStart = `background-start-${process.pid}`;
|
|
const backgroundEnd = `background-end-${process.pid}`;
|
|
const background = await execTool.execute("explicit-background", {
|
|
command: nodeEvalCommand(
|
|
`process.stdout.write(${JSON.stringify(backgroundStart + "\n")});` +
|
|
`setTimeout(() => process.stdout.write(${JSON.stringify(backgroundEnd + "\n")}), 350);`,
|
|
),
|
|
background: true,
|
|
});
|
|
const backgroundSession = requireSession(background);
|
|
cleanupPids.add(backgroundSession.pid);
|
|
|
|
const listed = await processTool.execute("list-background", { action: "list" });
|
|
expect((listed.details as ProcessDetails).sessions).toEqual(
|
|
expect.arrayContaining([
|
|
expect.objectContaining({
|
|
sessionId: backgroundSession.sessionId,
|
|
status: "running",
|
|
}),
|
|
]),
|
|
);
|
|
|
|
await expect
|
|
.poll(async () => {
|
|
const log = await processTool.execute("log-background", {
|
|
action: "log",
|
|
sessionId: backgroundSession.sessionId,
|
|
});
|
|
return textOf(log);
|
|
}, POLL_OPTIONS)
|
|
.toContain(backgroundStart);
|
|
|
|
const backgroundTerminal = await pollTerminal(processTool, backgroundSession.sessionId);
|
|
expect(backgroundTerminal.details).toMatchObject({
|
|
status: "completed",
|
|
exitCode: 0,
|
|
});
|
|
expect((backgroundTerminal.details as ProcessDetails).aggregated).toContain(backgroundEnd);
|
|
await clearFinished(processTool, backgroundSession.sessionId);
|
|
cleanupPids.delete(backgroundSession.pid);
|
|
|
|
const yieldedMarker = `yielded-terminal-${process.pid}`;
|
|
const yielded = await execTool.execute("yielded-background", {
|
|
command: nodeEvalCommand(
|
|
`setTimeout(() => process.stdout.write(${JSON.stringify(yieldedMarker + "\n")}), 350);`,
|
|
),
|
|
yieldMs: 20,
|
|
});
|
|
const yieldedSession = requireSession(yielded);
|
|
cleanupPids.add(yieldedSession.pid);
|
|
const yieldedTerminal = await pollTerminal(processTool, yieldedSession.sessionId);
|
|
expect(yieldedTerminal.details).toMatchObject({ status: "completed", exitCode: 0 });
|
|
expect((yieldedTerminal.details as ProcessDetails).aggregated).toContain(yieldedMarker);
|
|
await clearFinished(processTool, yieldedSession.sessionId);
|
|
cleanupPids.delete(yieldedSession.pid);
|
|
|
|
const timedOut = await foregroundExecTool.execute("foreground-timeout", {
|
|
command: nodeEvalCommand("setTimeout(() => {}, 5000);"),
|
|
timeoutSeconds: 0.05,
|
|
});
|
|
expect(timedOut.details).toMatchObject({
|
|
status: "failed",
|
|
exitReason: "overall-timeout",
|
|
timedOut: true,
|
|
});
|
|
expect(textOf(timedOut)).toContain("Command timed out after 0.05 seconds.");
|
|
|
|
const ptyMarker = `pty-route-${process.pid}`;
|
|
const pty = await foregroundExecTool.execute("foreground-pty", {
|
|
command: nodeEvalCommand(
|
|
`process.stdout.write(${JSON.stringify(ptyMarker + ":")} + String(Boolean(process.stdout.isTTY)) + ":" + (process.env.OPENCLAW_SHELL || ""));`,
|
|
),
|
|
pty: true,
|
|
});
|
|
expect(pty.details).toMatchObject({ status: "completed", exitCode: 0 });
|
|
expect(textOf(pty)).toContain(`${ptyMarker}:true:exec`);
|
|
|
|
const killMarker = `kill-target-${process.pid}`;
|
|
const killTarget = await execTool.execute("kill-background", {
|
|
command: nodeEvalCommand(
|
|
`process.stdout.write(${JSON.stringify(killMarker + "\n")});setInterval(() => {}, 1000);`,
|
|
),
|
|
background: true,
|
|
});
|
|
const killedSession = requireSession(killTarget);
|
|
cleanupPids.add(killedSession.pid);
|
|
const killed = await processTool.execute("kill-session", {
|
|
action: "kill",
|
|
sessionId: killedSession.sessionId,
|
|
});
|
|
expect(killed.details).toMatchObject({ status: "failed" });
|
|
const killedTerminal = await pollTerminal(processTool, killedSession.sessionId);
|
|
expect(killedTerminal.details).toMatchObject({ status: "failed" });
|
|
await clearFinished(processTool, killedSession.sessionId);
|
|
await expect.poll(() => pidExists(killedSession.pid), POLL_OPTIONS).toBe(false);
|
|
cleanupPids.delete(killedSession.pid);
|
|
|
|
const finalList = await processTool.execute("list-final", { action: "list" });
|
|
expect((finalList.details as ProcessDetails).sessions).toEqual([]);
|
|
expect(listRunningSessions().filter((session) => session.scopeKey === scopeKey)).toEqual([]);
|
|
expect(getActiveBackgroundExecSessionCount()).toBe(0);
|
|
} finally {
|
|
for (const session of listRunningSessions().filter((entry) => entry.scopeKey === scopeKey)) {
|
|
await processTool.execute(`cleanup-${session.id}`, {
|
|
action: "remove",
|
|
sessionId: session.id,
|
|
});
|
|
}
|
|
await expect.poll(() => [...cleanupPids].filter(pidExists).length, POLL_OPTIONS).toBe(0);
|
|
resetProcessRegistryForTests();
|
|
}
|
|
}, 30_000);
|