Skip to content

fix(engines): stop a signal-killed child reporting success#37

Merged
ralyodio merged 1 commit into
moshcoder:mainfrom
clawedassistant26:fix/signal-killed-child-reads-as-success
Jul 26, 2026
Merged

fix(engines): stop a signal-killed child reporting success#37
ralyodio merged 1 commit into
moshcoder:mainfrom
clawedassistant26:fix/signal-killed-child-reads-as-success

Conversation

@clawedassistant26

Copy link
Copy Markdown
Contributor

The bug

runCmd resolves { ok: true, code: null, signal } when a child dies on a signal — Node puts the signal name in signal and leaves code null. Three places that consume it treat a null code as a clean exit:

  • src/upgrade.mjs:128const ok = r.ok && (r.code == null || r.code === 0)
  • src/skills.mjs:69 — same expression, status "installed"
  • src/mcp.mjs:115 — same expression, status "added"

So a child killed by the OOM killer, a timeout wrapper's SIGTERM, or Ctrl-C reads as success. moshcode upgrade prints ✓ up to date and counts it in ✓ upgraded N/N; skill install prints installed; mcp add prints added. All three then exit 0, so a wrapper script cannot tell the difference either.

Reproduced on main before touching anything:

runCmd(SIGKILL) -> {"ok":true,"code":null,"signal":"SIGKILL"}
upgrade.mjs ok-expr  => true
runSkillInstall -> [{"key":"claude","status":"installed","code":null}]
runMcpAdd       -> [{"key":"claude","status":"added","code":null}]

The failure lines are unhelpful too: with code == null and no error, upgrade prints a bare ✗ … upgrade failed and integrations.mjs prints a bare failed, with nothing saying what happened.

The fix

Adds two small helpers next to runCmd in src/engines.mjs:

  • ranOk(r)r.ok && r.code === 0. Success now means "exited 0" and nothing else.
  • exitReason(r)"code 128", "SIGKILL", or the spawn error message.

Used at the three status sites. Results now also carry signal, and the failure lines name it, so a killed child reads failed (SIGKILL) instead of an unexplained failed.

Clean exits and non-zero exits behave exactly as before. Display sites that already handled signals (bin/moshcode.mjs, src/tui.mjs, src/cli.mjs, src/commands.mjs) are untouched.

Tests

3 added: 2 in test/engines.test.mjs (one spawns a real bash -c 'kill -9 $$' and asserts the whole {ok, code, signal} shape, one covers clean exit / bad code / ENOENT), 1 each in test/skills.test.mjs and test/mcp.test.mjs.

Verified fail-before / pass-after by stashing only src/:

  • unpatched src: 129 pass / 3 fail
  • with the fix: 145 pass / 0 fail (was 141 on main)

runCmd resolves { ok: true, code: null, signal } when a child dies on a
signal, but the three status checks that consume it treated a null code as
a clean exit:

  r.ok && (r.code == null || r.code === 0)

So an installer killed by the OOM killer, a timeout wrapper's SIGTERM, or
Ctrl-C was reported as success. `moshcode upgrade` printed "up to date"
and counted it in "upgraded N/N"; `skill install` printed "installed";
`mcp add` printed "added". All three then exited 0, so a wrapper script
could not tell the difference either.

Adds ranOk() and exitReason() next to runCmd and uses them at the three
sites, so success now means "exited 0" and nothing else. The results also
carry `signal`, and the failure lines name it, so a killed child says
"failed (SIGKILL)" instead of an unexplained "failed".

Clean exits and non-zero exits behave exactly as before.
@ralyodio
ralyodio merged commit dcf0195 into moshcoder:main Jul 26, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants