fix: preserve signal exit codes in CLI child process handling - #6039
Merged
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe CLI tools preserve child-process exit codes and convert signal termination to shell-style codes. ChangesChild-process signal exit codes
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ChildProcess
participant ExitCodeMapper
CLI->>ChildProcess: execute command
ChildProcess-->>CLI: return exit code or signal
CLI->>ExitCodeMapper: normalize termination
ExitCodeMapper-->>CLI: return shell exit code
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying egg with
|
| Latest commit: |
2d8dc30
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cff7f651.egg-cci.pages.dev |
| Branch Preview URL: | https://fix-signal-exit-code.egg-cci.pages.dev |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #6039 +/- ##
==========================================
- Coverage 83.08% 83.08% -0.01%
==========================================
Files 730 731 +1
Lines 22474 22496 +22
Branches 4520 4530 +10
==========================================
+ Hits 18673 18690 +17
- Misses 3297 3299 +2
- Partials 504 507 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
2d8dc30
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://68182d44.egg-v3.pages.dev |
| Branch Preview URL: | https://fix-signal-exit-code.egg-v3.pages.dev |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes CLI child-process exit handling so signal-killed children (which report code=null) are not misclassified as successful exits, and so callers receive conventional shell exit codes (128 + signal number) and clearer error output.
Changes:
- Update
egg-scripts startforeground mode to map(code, signal)to a real exit code and exit viaprocess.exit()to avoid uncaughtExitErrorstacks. - Update
egg-binfork handling to reject with “was killed by signal X”, propagate the child’s exit code, and printForkErrormessages without oclif word-wrapping. - Add regression tests/fixtures for signal-killed processes across
egg-scripts,egg-bin, andcreate-egg, plus local CI workflow documentation for theegg-binbuild-before-test exception.
Reviewed changes
Copilot reviewed 9 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
wiki/workflows/local-ci.md |
Documents that egg-bin tests require a prebuilt tools/egg-bin/dist and how to handle stale dist cleanup. |
wiki/log.md |
Adds a dated log entry summarizing the signal-exit fixes and the egg-bin local CI exception. |
tools/scripts/test/start-unit.test.ts |
Adds a unit test covering foreground exit-code mirroring and signal mapping (note: currently leaves a process.exit spy mocked). |
tools/scripts/src/commands/start.ts |
Maps (code, signal) to an exit code and uses process.exit() for non-zero child exits in foreground mode. |
tools/egg-bin/test/fixtures/demo-app-kill-self/package.json |
New fixture app that uses a framework which self-terminates via SIGTERM. |
tools/egg-bin/test/fixtures/demo-app-kill-self/node_modules/aliyun-egg-kill-self/package.json |
Fixture framework package metadata for the self-terminating framework. |
tools/egg-bin/test/fixtures/demo-app-kill-self/node_modules/aliyun-egg-kill-self/index.js |
Fixture framework implementation that logs and kills itself with SIGTERM. |
tools/egg-bin/test/commands/dev.test.ts |
Adds an e2e test asserting SIGTERM produces exit code 143 and an appropriate stderr message (skipped on Windows). |
tools/egg-bin/src/baseCommand.ts |
Enhances ForkError to carry an oclif exit code; improves fork exit handling for signal deaths and prints raw error messages in catch(). |
tools/create-egg/test/spawn-sync-exit-code.test.ts |
Adds unit tests for signal-aware spawnSync exit-code mapping behavior. |
tools/create-egg/src/index.ts |
Uses a new spawnSyncExitCode() helper so signal-terminated custom commands don’t exit as success. |
.gitignore |
Un-ignores the new demo-app-kill-self fixture’s node_modules for committed test fixtures. |
|
Dependency limit exceeded — report not shown. This pull request scan exceeded the 10,000-dependency limit applied to this scan, so the results are incomplete and may be inaccurate. To avoid reporting false positives, Socket has not posted a report. Upgrade your plan to raise the dependency limit and get complete reports, or view the partial scan in the dashboard. Socket is always free for open source. If this is a non-commercial open source project, contact us to request a free Team account. |
Member
Author
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/scripts/src/commands/start.ts`:
- Around line 373-381: Update the child-lifetime Promise around the
`child.once('exit', ...)` listener to also reject when the spawned child emits
`error`, ensuring spawn failures settle instead of hanging and propagate through
oclif’s normal lifecycle handling. Add a regression test covering `start` with a
nonexistent `--node` executable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ef072d2-fba1-4e69-b81b-51c212d99296
📒 Files selected for processing (7)
tools/create-egg/src/index.tstools/create-egg/src/utils.tstools/create-egg/test/exit-code.test.tstools/egg-bin/src/baseCommand.tstools/egg-bin/test/commands/dev.test.tstools/scripts/src/commands/start.tstools/scripts/test/start-unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/egg-bin/test/commands/dev.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7280c1d3cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tools/egg-bin/src/baseCommand.ts:440
- The ForkError message currently reconstructs the command as
modulePath + forkArgs.join(' '), which dropsprocess.execPath,execArgv, and the quoting you already computed infullCommand. That makes the printed command line less accurate and harder to copy/paste when debugging (and it undercuts the goal of printing the long command verbatim). Prefer usingfullCommand(or another single canonical string) in the error message for both exit-code and signal cases.
const command = modulePath + ' ' + forkArgs.join(' ');
const message =
code === null ? `${command} was killed by signal ${signal}` : `${command} exit with code ${code}`;
const err = new ForkError(message, toExitCode(code, signal));
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/egg-bin/src/baseCommand.ts (1)
431-445: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle fork
errorevents.
forkNode()only subscribes tochild.once('exit'), but a spawned child can emiterrorbefore or instead ofexit. Add a one-shot settlement path for bothexitanderror, normalize the spawn error toForkError, and add a regression test that emitserrorwithoutexit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/egg-bin/src/baseCommand.ts` around lines 431 - 445, Update forkNode() to settle the promise exactly once from either the child process’s exit or error event, preventing duplicate resolve/reject handling. Add an error-event handler that converts the spawn error into a ForkError while preserving the existing exit-code behavior, and add a regression test covering an error emitted without a subsequent exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/scripts/test/start-unit.test.ts`:
- Around line 118-132: Update the foreground spawn failure assertion in
TestStart.run to verify rejection with the exact Error instance passed to the
captured onError callback, rather than matching only its message. Preserve the
existing spawn ENOENT simulation and rejection behavior.
---
Outside diff comments:
In `@tools/egg-bin/src/baseCommand.ts`:
- Around line 431-445: Update forkNode() to settle the promise exactly once from
either the child process’s exit or error event, preventing duplicate
resolve/reject handling. Add an error-event handler that converts the spawn
error into a ForkError while preserving the existing exit-code behavior, and add
a regression test covering an error emitted without a subsequent exit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc7a192a-8a38-4bc1-a712-ec1c91e3fb1d
📒 Files selected for processing (6)
tools/egg-bin/src/baseCommand.tstools/scripts/src/commands/start.tstools/scripts/test/snapshot-start.test.tstools/scripts/test/start-unit.test.tswiki/log.mdwiki/workflows/local-ci.md
🚧 Files skipped from review as they are similar to previous changes (2)
- wiki/log.md
- wiki/workflows/local-ci.md
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
wiki/log.md:314
- This log entry still recommends the Windows-incompatible path-form workspace filter (
-- --workspace ./tools/egg-bin). In this same PR the CI workflow and local-ci guide are updated to use the package-name form (--workspace @eggjs/bin), so the log note should be updated to match (and avoid suggesting a command that fails on Windows).
- note: A child killed by a signal reports `code=null` on its exit event; three CLIs mishandled that (egg-scripts foreground start exited 0, egg-bin forkNode reported "exit with code null" and flattened every child failure to exit 1, create-egg's latent custom-command path ran `process.exit(status ?? 0)`). All three now map signal deaths to the shell convention `128 + signal number`, and egg-bin propagates the child's exit code through `ForkError.oclif.exit`. Durable finding recorded in local-ci.md: egg-bin's coffee tests run the compiled `dist/commands` CLI, so its suite needs `ut run build -- --workspace ./tools/egg-bin` first (the dedicated `test-egg-bin` CI job does exactly this), unlike the rest of the repo which tests unbuilt sources.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 29: Update the focused-workflow guidance in AGENTS.md to show that
`@eggjs/bin` must be built before running its coffee tests, placing `ut run build
--workspace `@eggjs/bin`` before `ut run test --workspace `@eggjs/bin`` while
preserving the package-name workspace form.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52c54151-b54b-43f3-a632-2b54a01da771
📒 Files selected for processing (6)
.github/workflows/ci.ymlAGENTS.mdtegg/plugin/eventbus/test/eventbus.test.tstools/scripts/test/start-unit.test.tswiki/log.mdwiki/workflows/local-ci.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/scripts/test/start-unit.test.ts
- wiki/workflows/local-ci.md
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 66 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tools/egg-bin/src/baseCommand.ts:446
forkNode()builds an error message frommodulePath + forkArgs, which omits theNODE_OPTIONS/execArgvthat were actually used (even thoughfullCommandis already computed). This can make the failure message misleading and undermines the intent to print the full command line verbatim.
wiki/log.md:314- This log entry references the Windows-incompatible path-form build filter (
-- --workspace ./tools/egg-bin), but the durable guidance and CI now use the package-name form (--workspace @eggjs/bin). Updating this keeps the historical note consistent with the current documented workflow.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 64 out of 67 changed files in this pull request and generated no new comments.
Suppressed comments (1)
wiki/log.md:314
- The 2026-08-03 log entry says the egg-bin suite needs
ut run build -- --workspace ./tools/egg-binand claims thetest-egg-binCI job does exactly that, but this PR changes CI and local guidance to the package-name form (--workspace @eggjs/bin). This note is now inconsistent/misleading for readers trying to follow the workflow.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 68 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tools/egg-bin/src/baseCommand.ts:449
- The
ForkErrormessage is described as embedding the full fork command line, but it currently only usesmodulePath + forkArgs.join(' '), which drops NODE_OPTIONS/execArgv and loses quoting for args with spaces. Use the already-computedfullCommandso the message is accurate and copy/pasteable (and soskipOclifErrorHandlingis justified).
wiki/log.md:314 - This log entry says egg-bin needs
ut run build -- --workspace ./tools/egg-binand that thetest-egg-binCI job does exactly that, but the very next entry (2026-08-06) notes this path-form filter fails on Windows and CI now uses the package-name form. Update this line to reference--workspace @eggjs/binso the wiki log is not self-contradictory.
A child process killed by a signal reports code=null on its exit event, and three CLIs mishandled that case: - egg-scripts start (foreground) treated a signal-killed server as a clean exit, and this.exit() threw an ExitError inside the event callback where nothing catches it, so any child failure exited 1. It now exits with the child's code, or 128 + signal number, via process.exit(). - egg-bin forkNode reported "exit with code null" and flattened every child failure to exit 1. It now names the signal, maps it to 128 + signal number, and propagates the child's exit code through ForkError.oclif.exit. - create-egg's custom command path ran process.exit(status ?? 0), masking a signal death as success. It now uses the exported spawnSyncExitCode() helper.
Post-review cleanup, no behavior change to the fixed paths: - ForkError now extends oclif's CLIError, which carries the exit code through the supported constructor option instead of a hand-rolled oclif.exit field; BaseCommand.catch() keys on the standard skipOclifErrorHandling flag rather than instanceof ForkError, so only the long fork command line messages opt out of pretty-printing - egg-scripts foreground start awaits the child inside run(), so the exit code flows through oclif's normal exit path and catch/finally still run; the unit test drives the public command surface instead of spying on process.exit - the 128 + signal number mapping is the same verbatim toExitCode() helper in all three packages; create-egg's copy moved to a leaf module so it is not a published export and its test no longer loads the whole CLI entry
- reject the foreground start promise when the child fails to spawn, with a spawn-error unit test (a spawn failure emits 'error' and may never emit 'exit', which would hang run()) - drive a clean child exit in snapshot-start.test.ts mocks so the four positive-path cases settle now that foreground start awaits the child - restore a public numeric field on ForkError as exitCode (CLIError's own code slot is a string rendered by oclif's pretty-printer) - correct the wiki log and updated_at dates to 2026-08-03 and list the egg-bin package.json and tsconfig.json sources in local-ci.md
- the path form `--workspace ./tools/egg-bin` does not match any workspace on Windows, so the test-egg-bin Windows job never built dist and every coffee test failed with "command dev not found" (same failure on next); use the package-name filter form and give the build its own step for clear failure attribution - the eventbus plugin app-boot beforeAll ran under vitest's default 10s hook timeout (glob projects do not inherit the root config's hookTimeout) and flaked on slow Windows runners; raise it to 30s - assert the exact spawn error instance in the start-unit spawn failure test (review feedback)
- @eggjs/scripts has no build script, so the name-form workspace filter ran nothing and failed; the job is ubuntu-only, keep the root tsdown path filter there - the langchain plugin app-boot beforeAll hit the same default 10s hook timeout on Windows as eventbus; give it the same explicit 30s
vitest glob projects do not inherit the root config's hookTimeout, so every tegg plugin and standalone suite booted its app under the default 10s and flaked on slow Windows CI runners: eventbus, langchain, then mcp-client failed the same way on consecutive runs. Apply the timeout to all 54 async beforeAll hooks instead of chasing them one at a time.
A spawn failure emits 'error' and may never emit 'exit', which would leave the forkNode promise pending and the child ref in the children set. Same pattern as the egg-scripts foreground fix.
The development plugin suites boot mm.cluster (a real master, agent and worker process tree) in beforeAll, which exceeded the 20s hook budget on a slow Windows CI runner during a rerun. The schedule plugin already uses a 60s hook budget for the same reason.
The three egg-ready assertions slept a fixed 5s before reading accumulated stdout, which flaked on a slow macOS CI runner even through vitest's two retries (agent respawn took longer than the sleep). Poll for the expected output with a 30s deadline instead; the test also gets faster on healthy runners because polling returns as soon as the output arrives.
fengmk2
force-pushed
the
fix/signal-exit-code
branch
from
August 7, 2026 08:30
b01e5f8 to
51601d1
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 68 changed files in this pull request and generated no new comments.
Suppressed comments (2)
wiki/log.md:410
- This log entry says egg-bin’s suite needs
ut run build -- --workspace ./tools/egg-binand that thetest-egg-binCI job does exactly that, but the current guidance/CI uses the package-name workspace filter (orworking-directory: tools/egg-bin). Update the command here to match so the wiki log doesn’t reintroduce the Windows-mismatch pitfall.
tools/egg-bin/src/baseCommand.ts:446 - The fork failure message claims to include the full fork command line, but it currently only prints
modulePath + args, omittingNODE_OPTIONS,process.execPath, andexecArgv. This makes failures harder to reproduce/debug (especially when flags are injected) and contradicts the nearby comment about embedding the full command line. Use the already-computedfullCommandfor the error message (and guard against a missing signal).
- the three positive reload assertions slept a fixed 5s after touching a watched file before reading stdout, which flaked on a slow Windows runner; poll with a 30s deadline instead (negative assertions keep their fixed waits, absence needs a bounded window) - the Windows MySQL choco install failed transiently in CI; give it the same 3-attempt retry loop the Memurai step already uses
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 69 out of 72 changed files in this pull request and generated no new comments.
Suppressed comments (1)
wiki/log.md:410
- The 2026-08-03 log entry says egg-bin’s suite needs
ut run build -- --workspace ./tools/egg-binfirst and that the CI job “does exactly this”, but the 2026-08-06 entry + local-ci.md describe the correct approach (dedicated build step and using the package-name workspace filter on Windows). This line is now misleading for readers following the wiki log.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A child process killed by a signal reports
code=nullon itsexitevent. Three CLIs mishandled that:egg-scripts startin foreground mode treated a signal-killed server as success (if (!code) return), andthis.exit()threw an ExitError inside the event callback where nothing catches it, so any child failure exited 1 with a stack trace. The foreground branch now awaits the child insiderun()and exits with the child's code (128 + signal numberfor signal deaths) through oclif's normal exit path, socatch/finallylifecycle still applies.egg-binforkNode()rejected with "exit with code null" and the CLI always exited 1.ForkErrornow extends oclif'sCLIErrorcarrying the child's real exit code, rejects with "was killed by signal X" for signal deaths, and setsskipOclifErrorHandlingon fork failures so the long command line prints verbatim instead of being word-wrapped by oclif's pretty-printer.create-egg's custom command path ranprocess.exit(status ?? 0), masking a signal death as success. The path is latent (no template definescustomCommandyet); it now uses a non-exportedtoExitCode()helper, same fix as the upstream create-vite pattern needs.The
128 + signal numbermapping is the same verbatimtoExitCode()helper in all three packages; a shared@eggjs/utilsexport was considered and skipped to avoid widening a published API for a 4-line convention (create-egg deliberately has no workspace deps).Each fix is covered by a reproducing test written first: a mocked-spawn unit test driving the start command's public surface, an egg-bin dev e2e with a fixture framework that SIGTERMs itself (expects "was killed by signal SIGTERM" and exit code 143, skipped on Windows), and unit tests for
toExitCode. Full egg-bin dev+test suites pass (41 tests, built first as the test-egg-bin CI job does); egg-scriptsstop.test.tsandshow helpfailures on macOS reproduce identically onnextwithout this change.Behavior note: egg-bin now exits with the forked child's actual exit code instead of flattening every failure to 1.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
API note:
ForkError(exported from@eggjs/bin/baseCommand) now extends oclif'sCLIError; its numeric child status moved fromcodetoexitCode(CLIErrorreservescodeas a string display slot) and the constructor takesnumberinstead ofnumber | null.