refactor(cli): throw CliError instead of calling process.exit#473
Draft
refactor(cli): throw CliError instead of calling process.exit#473
Conversation
Replace all direct process.exit() calls across 33 CLI files with centralized cliError() and cliAbort() functions from error.ts. - cliError(message?, exitCode?) for error exits (default code 1) - cliAbort(message?) for clean exits (code 0, SIGINT/cancellation) - checkConfigFile() now always exits on failure (removed exit param) - Added ESLint no-restricted-properties rule to ban future process.exit Made-with: Cursor
Replace the process.exit() calls in cliError/cliAbort with throwing a
CliError exception. process.exit() is now only called in handleCliError
(the top-level catch in cli.ts) and in SIGINT/detached event handlers
where exceptions cannot propagate.
This ensures finally clauses in called functions run when a CLI error
is raised.
- cliError/cliAbort throw CliError
- program.parseAsync().catch(handleCliError) is the single exit point
- docs.ts: Promise now uses reject(new CliError(...)) from event handlers
- self.ts: proc.on("exit") wrapped in awaitable Promise with reject
- replica.ts: detached stderr handler uses process.exit(11) + eslint-disable
- Fix package-lock.json name field (worktree artifact)
Made-with: Cursor
The empty catch{} was there to handle 'which mocv' failing (not installed).
Now that cliError() throws instead of calling process.exit(), the CliError
was being silently swallowed by that same catch block.
Fix by detecting mocv in the try/catch using a flag, then calling cliError()
outside it.
Made-with: Cursor
Catch blocks designed to handle subprocess/runtime errors (execa, spawn) were also swallowing CliError instances thrown by cliError() calls within the same try block. Now that cliError() throws instead of process.exit(), the CliError propagated into these handlers and got wrapped in a new 'Error while running X' message. Fix: add 'if (err instanceof CliError) throw err' at the top of each affected catch block so CliError passes through unchanged. Files affected: build.ts, check.ts, check-candid.ts, lint.ts All 63 tests pass after this fix. Made-with: Cursor
Use the simpler fix: re-throw CliError in the catch block, keeping the original try/catch structure unchanged. Made-with: Cursor
3 tasks
…locks - Add `import process` to error.ts for consistency with rest of codebase - Fix unhandled rejection in test.ts wasi/replica chains: add reject param to outer Promise and use .then(resolve, reject) instead of .then(resolve) - Add CliError re-throw guards to install-mops-dep.ts catch blocks to prevent silent swallowing, consistent with build.ts/check.ts pattern Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
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.
Problem
Direct
process.exit()calls scattered across ~90 sites in the CLI silently terminate the process, bypassing anyfinallyclauses in the call stack. This makes cleanup (lock files, temp dirs, subprocesses) unreliable and makes the code untestable in isolation.Root cause
The CLI had no error-signalling contract — errors were communicated by calling
process.exit()directly wherever an error was detected, rather than by propagating a value up the call stack.Fix
Introduce a
CliErrorclass incli/error.ts. ThecliError()andcliAbort()helpers now throw aCliErrorinstead of callingprocess.exit(). The only placeprocess.exit()is now called is inhandleCliError()— a single catch handler registered at the top-level entry point:This means
finallyblocks throughout the call stack execute correctly when a CLI error is raised.The three categories of remaining
process.exit()calls (all witheslint-disable-next-line no-restricted-propertiescomments):cli/error.ts—handleCliError, the single authorised exit pointtest.tsandinstall-mops-dep.ts— event callbacks where thrown errors cannot propagatereplica.ts— a detachedstderr.on("data")handler for a long-running process, also cannot propagateAn ESLint
no-restricted-propertiesrule onprocess.exitenforces this going forward.Caveats
Catch blocks written to handle subprocess/runtime errors (e.g.
execafailures) also needed updating — they previously were unreachable fromcliError()sinceprocess.exit()terminated synchronously. Now thatcliError()throws, these catch blocks must re-throwCliErrorinstances so they are not wrapped in a new "Error while running X" message. All affected catch blocks inbuild.ts,check.ts,check-candid.ts, andlint.tshave been fixed.Test plan
tsc --noEmit)process.exitgrep confirms no unguarded calls outsideerror.tsand legitimate event handlers