[CLI] Replace process.exit() with thrown errors in library code paths#3478
[CLI] Replace process.exit() with thrown errors in library code paths#3478gcsecsey wants to merge 5 commits intoWordPress:trunkfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR changes the CLI error-handling behavior so that library consumers of runCLI() receive thrown errors instead of having their process terminated via process.exit(1).
Changes:
- Replace
process.exit(1)calls inrun-cli.tsandworker-thread-v2.tswith thrown errors. - Move final
process.exit(1)behavior to the CLI entrypoint (cli.ts). - Update the “port already in use” test to assert a rejected promise instead of mocking
process.exit.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/playground/cli/src/run-cli.ts | Converts error exits to thrown errors in parseOptionsAndRunCLI()/runCLI() and replaces an exit-on-reset path with a thrown error. |
| packages/playground/cli/src/cli.ts | Ensures standalone CLI still exits with code 1 by catching parseOptionsAndRunCLI() rejection. |
| packages/playground/cli/src/blueprints-v2/worker-thread-v2.ts | Converts mount failure from stderr+exit to a thrown error with cause. |
| packages/playground/cli/tests/run-cli.spec.ts | Updates port-in-use test from exit-mocking to rejection assertion; small formatting change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for submitting this PR, @gcsecsey! This change makes sense to me, and I think it would be good to go further to remove all process.exit() calls from run-cli.ts exports. That way, the run-cli module exports are consistent, and we can know that all process.exit() calls are handled by cli.ts instead. In addition, I think it is better to explicitly return nonzero exit codes, where possible, instead of throwing in the case of all errors. This way, control flow remains explicit, and the return types reflect more of the possible results. Where possible, exceptions should be exceptional. They are not meant to be an implicit result type. So I had Claude make the above changes and add tests for the different return value possibilities. I pushed the changes but will have to finish reviewing them tomorrow or on Monday. |
The catch in runCLI() was printing the error via cliOutput.printError() before re-throwing. parseOptionsAndRunCLI() already formats and prints errors in its own catch block, so this resulted in duplicate output.
The wrapped error stored the original error as `cause`, but callers that only read `.message` would lose the root cause (e.g. ENOENT, EACCES). Include a short summary in the message itself so the default output remains actionable.
Thank you for making these changes @brandonpayton! I also addressed the smaller feedbacks from copilot in the meantime. |
Motivation for the change, related issues
Related to Automattic/studio#2805
Studio imports
runCLI()from@wp-playground/clias a library API. WhenrunCLI()encounters errors, it callsprocess.exit(1)internally, which kills Studio's child process without giving it a chance to handle the error gracefully. We discussed in the Studio PR review,runCLIshouldn't callprocess.exit(), as that's not a safe operation for a function exposed as a library API.Implementation details
process.exit(1)calls inrun-cli.tsandworker-thread-v2.tswith thrown errors, so library consumers can catch and handle failuresprocess.exit(1)to the CLI entry point (cli.ts), so it's only reached when Playground is invoked as a standalone CLIprocess.exitTesting Instructions
npx nx run playground-cli:test-playground-clinpx nx dev playground-cli server) and verify it still exits with code 1 on errors (eg. binding to an already-used port)runCLIas a library and verify errors are now thrown instead of killing the process