diff --git a/src/engines.mjs b/src/engines.mjs index 08cddf7..d4e72d0 100644 --- a/src/engines.mjs +++ b/src/engines.mjs @@ -194,6 +194,28 @@ export function runCmd(cmd, args = []) { }); } +/** + * True only when a child actually ran and exited 0. Node reports a signal death + * as `code === null` (the signal name lands in `signal` instead), so a killed + * child — OOM, a timeout wrapper's SIGTERM, Ctrl-C — must not read as success + * just because it has no exit code. + */ +export function ranOk(r) { + return Boolean(r?.ok) && r.code === 0; +} + +/** + * Short human reason a `runCmd` result failed: "code 128", "SIGKILL", or the + * spawn error message. Null when it succeeded. + */ +export function exitReason(r) { + if (ranOk(r)) return null; + if (r?.error) return r.error.message || String(r.error); + if (r?.code != null) return `code ${r.code}`; + if (r?.signal) return r.signal; + return "unknown error"; +} + /** * Hand the current process streams to an external CLI. Arguments, cwd, and the * environment are inherited unchanged unless that target explicitly asks for diff --git a/src/integrations.mjs b/src/integrations.mjs index 604d0f2..3929211 100644 --- a/src/integrations.mjs +++ b/src/integrations.mjs @@ -114,7 +114,7 @@ export function printSkillTargets() { function summarize(results) { for (const r of results) { if (r.status === "added" || r.status === "installed") console.log(line(r.key, ok(r.status))); - else if (r.status === "failed") console.log(line(r.key, err(`failed${r.code != null ? ` (code ${r.code})` : ""}`))); + else if (r.status === "failed") console.log(line(r.key, err(`failed${r.code != null ? ` (code ${r.code})` : r.signal ? ` (${r.signal})` : ""}`))); else if (r.status === "not-installed") console.log(line(r.key, ash("not installed — /install " + r.key))); else console.log(line(r.key, ash(`skipped — ${r.reason}`))); } diff --git a/src/mcp.mjs b/src/mcp.mjs index ef4dd6b..6a467a9 100644 --- a/src/mcp.mjs +++ b/src/mcp.mjs @@ -1,7 +1,7 @@ // Register MCP (Model Context Protocol) servers across every engine that // supports them, from one canonical definition. MoshCode drives each engine's // own `mcp add` so the engine owns its config format. See prd/0003. -import { ENGINES, isInstalled, runCmd } from "./engines.mjs"; +import { ENGINES, isInstalled, ranOk, runCmd } from "./engines.mjs"; // Coding engines that can register MCP servers. Aider has no MCP support. export const MCP_ENGINES = ["claude", "gemini", "codex", "opencode"]; @@ -112,7 +112,7 @@ export async function runMcpAdd(plan, { run = runCmd } = {}) { if (item.skip) { results.push({ key: item.key, status: "skipped", reason: item.skip }); continue; } if (!item.installed) { results.push({ key: item.key, status: "not-installed" }); continue; } const r = await run(item.bin, item.argv); - results.push({ key: item.key, status: r.ok && (r.code === 0 || r.code == null) ? "added" : "failed", code: r.code }); + results.push({ key: item.key, status: ranOk(r) ? "added" : "failed", code: r.code, signal: r.signal ?? null }); } return results; } diff --git a/src/skills.mjs b/src/skills.mjs index 4b5905e..c1b3050 100644 --- a/src/skills.mjs +++ b/src/skills.mjs @@ -3,7 +3,7 @@ // source into its personal skills dir. See prd/0003. import os from "node:os"; import path from "node:path"; -import { ENGINES, isInstalled, runCmd } from "./engines.mjs"; +import { ENGINES, isInstalled, ranOk, runCmd } from "./engines.mjs"; // Coding engines with a skills primitive. Codex/OpenCode/Aider have none. export const SKILL_ENGINES = ["claude", "gemini"]; @@ -66,7 +66,7 @@ export async function runSkillInstall(plan, { run = runCmd } = {}) { if (item.skip) { results.push({ key: item.key, status: "skipped", reason: item.skip }); continue; } if (!item.installed) { results.push({ key: item.key, status: "not-installed" }); continue; } const r = await run(item.cmd, item.args); - results.push({ key: item.key, status: r.ok && (r.code === 0 || r.code == null) ? "installed" : "failed", code: r.code }); + results.push({ key: item.key, status: ranOk(r) ? "installed" : "failed", code: r.code, signal: r.signal ?? null }); } return results; } diff --git a/src/upgrade.mjs b/src/upgrade.mjs index c29cb56..aa9b071 100644 --- a/src/upgrade.mjs +++ b/src/upgrade.mjs @@ -4,7 +4,7 @@ import { fileURLToPath } from "node:url"; import path from "node:path"; import fs from "node:fs"; -import { ENGINES, engineStatus, resolveEngine, upgradeSpec, runCmd } from "./engines.mjs"; +import { ENGINES, engineStatus, exitReason, ranOk, resolveEngine, upgradeSpec, runCmd } from "./engines.mjs"; import { TOOLS, resolveTool, toolStatus, toolUpgradeSpec } from "./tools.mjs"; // Self-upgrade re-runs the moshcode installer's `update` path. Defaults to the @@ -125,9 +125,9 @@ export async function runUpgrade(targets = [], io = {}) { rule(); const r = await runCmd(spec.cmd, spec.args); rule(); - const ok = r.ok && (r.code == null || r.code === 0); - log(ok ? `✓ ${name} up to date` : `✗ ${name} upgrade failed${r.code != null ? ` (code ${r.code})` : r.error ? `: ${r.error.message || r.error}` : ""}`); - results.push({ name, ok, code: r.code }); + const ok = ranOk(r); + log(ok ? `✓ ${name} up to date` : `✗ ${name} upgrade failed (${exitReason(r)})`); + results.push({ name, ok, code: r.code, signal: r.signal ?? null }); return ok; }; diff --git a/test/engines.test.mjs b/test/engines.test.mjs index 275f8d1..54d47db 100644 --- a/test/engines.test.mjs +++ b/test/engines.test.mjs @@ -14,7 +14,7 @@ import { fileURLToPath } from "node:url"; import { spawn } from "node:child_process"; import test from "node:test"; -import { ENGINES, agentLaunchArgs } from "../src/engines.mjs"; +import { ENGINES, agentLaunchArgs, exitReason, ranOk, runCmd } from "../src/engines.mjs"; const BIN = fileURLToPath(new URL("../bin/moshcode.mjs", import.meta.url)); // The autonomous-session bypass flags each engine declares (engine.agentArgs). @@ -130,3 +130,30 @@ test("bare engine launch remains a raw passthrough", async () => { assert.deepEqual(JSON.parse(result.stdout), ["--model", "sonnet"]); assert.equal(result.stderr, ""); }); + +test("a signal-killed child is a failure, not a codeless success", async () => { + const r = await runCmd("bash", ["-c", "kill -9 $$"]); + + // Node reports a signal death with code === null — the old `code == null` + // success check read that as "exited cleanly". + assert.equal(r.ok, true); + assert.equal(r.code, null); + assert.equal(r.signal, "SIGKILL"); + + assert.equal(ranOk(r), false); + assert.equal(exitReason(r), "SIGKILL"); +}); + +test("ranOk and exitReason cover clean exits, bad codes, and spawn errors", async () => { + const clean = await runCmd("bash", ["-c", "exit 0"]); + assert.equal(ranOk(clean), true); + assert.equal(exitReason(clean), null); + + const bad = await runCmd("bash", ["-c", "exit 128"]); + assert.equal(ranOk(bad), false); + assert.equal(exitReason(bad), "code 128"); + + const missing = await runCmd("moshcode-does-not-exist-xyz", []); + assert.equal(ranOk(missing), false); + assert.match(exitReason(missing), /ENOENT|not found|spawn/i); +}); diff --git a/test/mcp.test.mjs b/test/mcp.test.mjs index bdd12d0..a1d12ef 100644 --- a/test/mcp.test.mjs +++ b/test/mcp.test.mjs @@ -142,3 +142,13 @@ fs.writeFileSync(path.join(process.env.CAPS, "${name}.json"), JSON.stringify(pro assert.deepEqual(JSON.parse(readFileSync(path.join(caps, "codex.json"), "utf8")), ["mcp", "add", "sentry", "--url", "https://mcp.sentry.dev/mcp"]); assert.deepEqual(JSON.parse(readFileSync(path.join(caps, "opencode.json"), "utf8")), ["mcp", "add", "sentry", "--url", "https://mcp.sentry.dev/mcp"]); }); + +test("an mcp add killed by a signal reports failed, not added", async () => { + const spec = { name: "s", target: "https://x.dev/mcp", env: [], headers: [] }; + const plan = planMcpAdd(spec, { installedSet: new Set(["claude"]) }); + const results = await runMcpAdd(plan, { run: async () => ({ ok: true, code: null, signal: "SIGTERM" }) }); + const claude = results.find((r) => r.key === "claude"); + + assert.equal(claude.status, "failed"); + assert.equal(claude.signal, "SIGTERM"); +}); diff --git a/test/skills.test.mjs b/test/skills.test.mjs index c92c5a5..d256e20 100644 --- a/test/skills.test.mjs +++ b/test/skills.test.mjs @@ -64,3 +64,12 @@ test("runSkillInstall summarizes installed / not-installed", async () => { assert.equal(byKey.gemini, "installed"); assert.equal(byKey.claude, "not-installed"); }); + +test("a skill install killed by a signal reports failed, not installed", async () => { + const plan = planSkillInstall({ source: "https://x/y", name: "y" }, { installedSet: new Set(["gemini"]) }); + const results = await runSkillInstall(plan, { run: async () => ({ ok: true, code: null, signal: "SIGKILL" }) }); + const gemini = results.find((r) => r.key === "gemini"); + + assert.equal(gemini.status, "failed"); + assert.equal(gemini.signal, "SIGKILL"); +});