diff --git a/CHANGELOG.md b/CHANGELOG.md index f695aa2f8..efef447ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - Groups: `whatsapp.groups`, `telegram.groups`, and `imessage.groups` now act as allowlists when set. Add `"*"` to keep allow-all behavior. ### Fixes +- Skills: harden skill installs with process-group timeouts, clearer timeout messaging, and logged rerun commands when installs fail. - Heartbeat: default interval now 30m with a new default prompt + HEARTBEAT.md template. - Onboarding: write auth profiles to the multi-agent path (`~/.clawdbot/agents/main/agent/`) so the gateway finds credentials on first startup. Thanks @minghinmatthewlam for PR #327. - Docs: add missing `ui:install` setup step in the README. Thanks @hugobarauna for PR #300. diff --git a/src/agents/skills-install.ts b/src/agents/skills-install.ts index 0def4ce96..5cfb3df88 100644 --- a/src/agents/skills-install.ts +++ b/src/agents/skills-install.ts @@ -27,6 +27,7 @@ export type SkillInstallResult = { stdout: string; stderr: string; code: number | null; + command?: string; }; function summarizeInstallOutput(text: string): string | undefined { @@ -55,7 +56,22 @@ function formatInstallFailureMessage(result: { code: number | null; stdout: string; stderr: string; + signal?: NodeJS.Signals | null; + killed?: boolean; + timedOut?: boolean; + timeoutMs?: number; }): string { + const isTimeout = + result.timedOut || result.signal === "SIGKILL" || result.killed === true; + if (isTimeout) { + const seconds = + typeof result.timeoutMs === "number" + ? Math.max(1, Math.round(result.timeoutMs / 1000)) + : undefined; + return seconds + ? `Install timed out after ${seconds}s` + : "Install timed out"; + } const code = typeof result.code === "number" ? `exit ${result.code}` : "unknown exit"; const summary = @@ -127,6 +143,15 @@ function buildInstallCommand( } } +function formatCommand(argv: string[]): string { + return argv + .map((arg) => { + if (/^[A-Za-z0-9_./:=+-]+$/.test(arg)) return arg; + return `'${arg.replace(/'/g, `'\\''`)}'`; + }) + .join(" "); +} + async function resolveBrewBinDir( timeoutMs: number, ): Promise { @@ -194,6 +219,16 @@ export async function installSkill( code: null, }; } + if (!command.argv || command.argv.length === 0) { + return { + ok: false, + message: "invalid install command", + stdout: "", + stderr: "", + code: null, + }; + } + const commandLine = formatCommand(command.argv); if (spec.kind === "brew" && !hasBinary("brew")) { return { ok: false, @@ -201,6 +236,7 @@ export async function installSkill( stdout: "", stderr: "", code: null, + command: commandLine, }; } if (spec.kind === "uv" && !hasBinary("uv")) { @@ -218,6 +254,7 @@ export async function installSkill( stdout: brewResult.stdout.trim(), stderr: brewResult.stderr.trim(), code: brewResult.code, + command: formatCommand(["brew", "install", "uv"]), }; } } else { @@ -227,18 +264,10 @@ export async function installSkill( stdout: "", stderr: "", code: null, + command: commandLine, }; } } - if (!command.argv || command.argv.length === 0) { - return { - ok: false, - message: "invalid install command", - stdout: "", - stderr: "", - code: null, - }; - } if (spec.kind === "go" && !hasBinary("go")) { if (hasBinary("brew")) { @@ -255,6 +284,7 @@ export async function installSkill( stdout: brewResult.stdout.trim(), stderr: brewResult.stderr.trim(), code: brewResult.code, + command: formatCommand(["brew", "install", "go"]), }; } } else { @@ -264,6 +294,7 @@ export async function installSkill( stdout: "", stderr: "", code: null, + command: commandLine, }; } } @@ -277,7 +308,14 @@ export async function installSkill( const result = await (async () => { const argv = command.argv; if (!argv || argv.length === 0) { - return { code: null, stdout: "", stderr: "invalid install command" }; + return { + code: null, + stdout: "", + stderr: "invalid install command", + signal: null, + killed: false, + timedOut: false, + }; } try { return await runCommandWithTimeout(argv, { @@ -286,16 +324,26 @@ export async function installSkill( }); } catch (err) { const stderr = err instanceof Error ? err.message : String(err); - return { code: null, stdout: "", stderr }; + return { + code: null, + stdout: "", + stderr, + signal: null, + killed: false, + timedOut: false, + }; } })(); - const success = result.code === 0; + const success = result.code === 0 && !result.timedOut; return { ok: success, - message: success ? "Installed" : formatInstallFailureMessage(result), + message: success + ? "Installed" + : formatInstallFailureMessage({ ...result, timeoutMs }), stdout: result.stdout.trim(), stderr: result.stderr.trim(), code: result.code, + command: commandLine, }; } diff --git a/src/commands/onboard-skills.ts b/src/commands/onboard-skills.ts index 207480379..b121c6b77 100644 --- a/src/commands/onboard-skills.ts +++ b/src/commands/onboard-skills.ts @@ -163,6 +163,9 @@ export async function setupSkills( spin.stop( `Install failed: ${name}${code}${detail ? ` — ${detail}` : ""}`, ); + if (result.command) { + runtime.log(`Command: ${result.command}`); + } if (result.stderr) runtime.log(result.stderr.trim()); else if (result.stdout) runtime.log(result.stdout.trim()); runtime.log( diff --git a/src/process/exec.test.ts b/src/process/exec.test.ts index 3cb917c3e..0394f4282 100644 --- a/src/process/exec.test.ts +++ b/src/process/exec.test.ts @@ -18,5 +18,16 @@ describe("runCommandWithTimeout", () => { expect(result.code).toBe(0); expect(result.stdout).toBe("ok"); + expect(result.timedOut).toBe(false); + }); + + it("marks timed out processes", async () => { + if (process.platform === "win32") return; + const result = await runCommandWithTimeout( + [process.execPath, "-e", "setTimeout(() => {}, 1000)"], + { timeoutMs: 50 }, + ); + + expect(result.timedOut).toBe(true); }); }); diff --git a/src/process/exec.ts b/src/process/exec.ts index f0ea1c5a4..ce3530c19 100644 --- a/src/process/exec.ts +++ b/src/process/exec.ts @@ -41,6 +41,7 @@ export type SpawnResult = { code: number | null; signal: NodeJS.Signals | null; killed: boolean; + timedOut: boolean; }; export type CommandOptions = { @@ -60,18 +61,42 @@ export async function runCommandWithTimeout( : optionsOrTimeout; const { timeoutMs, cwd, input, env } = options; + const supportsGroupKill = process.platform !== "win32"; + const killGraceMs = 5_000; + // Spawn with inherited stdin (TTY) so tools like `pi` stay interactive when needed. return await new Promise((resolve, reject) => { const child = spawn(argv[0], argv.slice(1), { stdio: [input ? "pipe" : "inherit", "pipe", "pipe"], cwd, env: env ? { ...process.env, ...env } : process.env, + detached: supportsGroupKill, }); let stdout = ""; let stderr = ""; let settled = false; + let timedOut = false; + let killTimer: NodeJS.Timeout | undefined; const timer = setTimeout(() => { - child.kill("SIGKILL"); + if (settled) return; + timedOut = true; + const pid = child.pid; + if (pid) { + try { + if (supportsGroupKill) process.kill(-pid, "SIGTERM"); + else child.kill("SIGTERM"); + } catch { + // ignore + } + killTimer = setTimeout(() => { + try { + if (supportsGroupKill) process.kill(-pid, "SIGKILL"); + else child.kill("SIGKILL"); + } catch { + // ignore + } + }, killGraceMs); + } }, timeoutMs); if (input && child.stdin) { @@ -89,13 +114,22 @@ export async function runCommandWithTimeout( if (settled) return; settled = true; clearTimeout(timer); + if (killTimer) clearTimeout(killTimer); reject(err); }); child.on("close", (code, signal) => { if (settled) return; settled = true; clearTimeout(timer); - resolve({ stdout, stderr, code, signal, killed: child.killed }); + if (killTimer) clearTimeout(killTimer); + resolve({ + stdout, + stderr, + code, + signal, + killed: child.killed, + timedOut, + }); }); }); }