fix(cli): report the real exit code when the binary is killed by a signal - #932
fix(cli): report the real exit code when the binary is killed by a signal#932mittalpk wants to merge 2 commits into
Conversation
…gnal process.exit(result.status ?? (result.error ? 1 : 0)) exits 0 when spawnSync's result has neither status nor error set, which is exactly what happens when the native binary is terminated by a signal (e.g. OOM-killed) -- status is null and there's no spawn error, so a signal-killed run looked like success to any CI or automation gating on exit code. Now exits with 128 + signal number, the same convention shells use, and prints which signal killed it. Extracted the exit-code decision into computeExit() and moved the rest of the script into main(), guarded by require.main === module, so it's actually testable -- the file previously ran its side effects at require time, which made it untestable without spawning a real subprocess.
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| function computeExit(result) { | ||
| if (result.signal) { | ||
| return { | ||
| code: 128 + (os.constants.signals[result.signal] || 1), |
There was a problem hiding this comment.
The fallback to signal number 1 (SIGHUP) when the signal name is unrecognized is a defensive choice, but it could produce misleading exit codes. Consider adding a comment explaining why 1 was chosen, or logging a warning for unrecognized signals to aid debugging edge cases.
| message: `[ERROR] OpenCodeReview binary was terminated by signal ${result.signal}`, | ||
| }; | ||
| } | ||
| return { code: result.status ?? (result.error ? 1 : 0), message: null }; |
There was a problem hiding this comment.
When spawnSync itself fails (e.g., EACCES permission denied, or the binary crashes before exec), result.error contains a descriptive Error object, but its message is silently discarded. The user gets exit code 1 with no explanation at all — no stderr from the binary (it never ran) and no error from the wrapper. Since computeExit already has a message field for the signal case, it should also surface the spawn error details. This would significantly aid debugging runtime failures.
Note: the existing test (ocr.test.js line 32) asserts message is null for this case, so the test would also need updating.
Suggestion:
| return { code: result.status ?? (result.error ? 1 : 0), message: null }; | |
| if (result.error) { | |
| return { code: result.status ?? 1, message: `[ERROR] Failed to run OpenCodeReview binary: ${result.error.message}` }; | |
| } | |
| return { code: result.status ?? 0, message: null }; |
There was a problem hiding this comment.
Fixed in e57dc75, surfaces result.error.message and updated the test.
When spawnSync itself fails (e.g. EACCES, or the binary crashes before exec), result.error held a descriptive Error but its message was silently discarded, leaving the user with exit code 1 and no explanation. Surface it in the reported message instead. Also comments the SIGHUP fallback for unrecognized signal names. Addresses review feedback on alibaba#932.
1 participant
Fixes #931.
process.exit(result.status ?? (result.error ? 1 : 0))exits 0 when spawnSync's result has neitherstatusnorerrorset, which is exactly what happens when the binary gets killed by a signal (OOM etc.) —statusis null and there's no spawn error, so a signal-killed run looked like success to any CI/automation gating on exit code. Now it exits with 128 + the signal number, same convention shells use, and prints which signal killed it.Also pulled the exit-code decision into a small
computeExit()function and moved the rest intomain()guarded byrequire.main === module, since the file previously ran everything at require time and couldn't be tested without spawning a real subprocess. Addedbin/ocr.test.jscovering normal exit, spawn error, and both signal cases.