fix: treat signal-terminated subprocesses as failures in runCommand#429
Conversation
rajpratham1
left a comment
There was a problem hiding this comment.
This is a small, correct fix for a subtle bug where signal-terminated processes were incorrectly treated as successful.
Why it's correct
Previously:
status: result.status ?? 0
For spawnSync(), if a child is terminated by a signal:
status === null
signal === "SIGKILL" (or similar)
The old code converted this to:
status = 0
meaning downstream code would interpret a killed process as a successful exit.
The new logic:
status: result.status ?? (result.signal ? 1 : 0)
correctly treats signal termination as a non-zero failure while still preserving the signal information.
Impact
This prevents incorrect success detection in helpers such as:
runCommandChecked
binaryAvailable
any future code checking status === 0
Without this fix, a process killed by:
SIGKILL
SIGTERM
SIGINT
could be mistaken for a successful execution.
Test coverage
The added tests are appropriate and cover:
✅ signal-terminated process returns non-zero status
✅ runCommandChecked() throws
✅ successful exit remains 0
✅ non-zero exit codes are preserved
✅ failure formatting still reports the signal
This provides good regression coverage.
Minor note
Using 1 as the synthesized exit status is reasonable since the actual termination signal is preserved separately in result.signal. Consumers that care about the exact cause can inspect signal rather than relying on the synthesized status.
|
@rajpratham1 Thanks for the review! Agreed on the synthesized 1 the real cause stays in result.signal and is surfaced by formatCommandFailure (signal=SIGKILL), so callers that need the exact cause can still get it. |
Problem
spawnSyncreturnsstatus === nullwhen a child process is terminated by a signal (e.g. SIGKILL from the OOM killer, or a manual kill).runCommandcoerced that to0withresult.status ?? 0, sorunCommandChecked(status !== 0) andbinaryAvailabletreated the killed process as a success callers then proceeded with truncated or empty stdout as if the command had completed normally (e.g. agit diffthat was killed mid-run would look empty/clean).The existing
if (result.signal)branch informatCommandFailureshows this was meant to be reported as a failure, but the coercion made that path unreachable for the checked helpers.Fix
Map a signal death to a non-zero status:
result.status ?? (result.signal ? 1 : 0). Only the signal case changes; thesignalfield is still exposed separately and surfaced byformatCommandFailure(signal=SIGKILL).Tests
Added cases to
tests/process.test.mjs: real SIGKILL child reports non-zero status + preserved signal,runCommandCheckedthrows withsignal=SIGKILL, clean exit still0, non-zero exit preserved, andformatCommandFailuresurfaces the signal. Full suite: 95 pass / 0 fail.tsc -p tsconfig.app-server.jsonpasses.Conflicts
No overlap with the other open
process.mjsPRs — #415 doesn't touch this file, and #424 only changes theshell:line andterminateProcessTree(this PR changesrunCommand'sstatusmapping). taskkill's numeric exit codes (128/0) are unaffected.