fix: close two silent-exit vectors around unhandled rejections - #1758
Conversation
The crash handler only listened on uncaughtExceptionMonitor, which never fires for a rejection that has a listener — and the TUI always registers one, converting rejections into a silent exit(1) with no telemetry at all. Observe unhandledRejection directly so those crashes still leave a trace. Since registering a real listener suppresses Node's default crash-on-rejection, rethrow when we are the only listener (print / server modes), deduped so the monitor does not double-report.
The Ctrl+V image-paste dispatch chained off the async handler without a rejection branch, so any failure inside it became an unhandled rejection — which the CLI's crash path turns into a silent exit(1). Treat a rejection like "no image available" and paste as text.
🦋 Changeset detectedLatest commit: 5a125d1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![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: 3148a51067
ℹ️ 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".
| const soleListener = process.listenerCount('unhandledRejection') === 1; | ||
| if (!isAbortError(reason)) { | ||
| trackCrash(rejectionErrorType(reason), 'unhandledRejection'); | ||
| if (reason instanceof Error) recordedRejections.add(reason); |
There was a problem hiding this comment.
In print/server modes where this handler is the sole unhandledRejection listener, a rejection like Promise.reject({ code: 'E' }) is tracked here and then rethrown. Because only Error instances are added to recordedRejections, the subsequent uncaughtExceptionMonitor pass is not deduped and records a second crash event for the same failure; primitive/null reasons can also hit the monitor's less-safe error-type path after being rethrown. This corrupts crash counts for non-Error rejection reasons, so the dedupe needs to cover every rethrown reason or the monitor needs to use the same safe classification.
Useful? React with 👍 / 👎.
Non-Error rejection reasons (plain objects, strings, null) were recorded by the rejection handler but not added to the dedupe set, so the uncaughtExceptionMonitor pass after the rethrow reported a second crash for the same failure; null/undefined reasons also crashed the monitor's own error-type extraction. Use a Set so primitives dedupe by value, add every rethrown reason, and classify monitor crashes null-safely.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a125d1a4f
ℹ️ 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".
| if (soleListener) { | ||
| throw reason; |
There was a problem hiding this comment.
When the CLI is run with NODE_OPTIONS=--unhandled-rejections=warn or none, Node's documented behavior from node --help is that warn logs a warning and none silences warnings instead of terminating. Because this handler now always throws whenever it is the only unhandledRejection listener, installing crash telemetry turns those explicitly configured non-fatal rejections into an immediate exit(1), so it no longer preserves the process' unhandled-rejection policy outside the default throw mode.
Useful? React with 👍 / 👎.
…rminal hosts The 'falls back to the text paste path when the image paste handler rejects' test was added in upstream PR MoonshotAI#1758 with a key selector that only handled process.platform === 'win32'. Our fork extends the paste key to also bind Alt+V when isWindowsTerminalHost() is true (WT_SESSION / WSLENV / WSL_DISTRO_NAME set), so the test must send '\u001Bv' on those hosts — otherwise the pasteKey check misses, onPasteImage is never invoked, and onTextPaste never fires. Triggers here in WSL2 (WSL_DISTRO_NAME=Ubuntu-22.04, WT_SESSION set): without this fix the test fails with 'expected onTextPaste to be called once, but got 0 times'.
…rminal hosts The 'falls back to the text paste path when the image paste handler rejects' test was added in upstream PR MoonshotAI#1758 with a key selector that only handled process.platform === 'win32'. Our fork extends the paste key to also bind Alt+V when isWindowsTerminalHost() is true (WT_SESSION / WSLENV / WSL_DISTRO_NAME set), so the test must send '\u001Bv' on those hosts — otherwise the pasteKey check misses, onPasteImage is never invoked, and onTextPaste never fires. Triggers here in WSL2 (WSL_DISTRO_NAME=Ubuntu-22.04, WT_SESSION set): without this fix the test fails with 'expected onTextPaste to be called once, but got 0 times'.
1 participant
Related Issue
No linked issue — the problem is explained below (from the same user report that led to #1757).
Problem
A user on 0.24.2 hit repeated "the CLI exited by itself" events — twice in ~80 minutes, once mid-stream, once idle — with zero diagnostics anywhere: nothing in the log, nothing in crash telemetry, no shell signal message, non-zero exit code. Analysis showed every graceful-exit and signal path is ruled out, leaving the TUI's
uncaughtException/unhandledRejectioncrash path as the only consistent mechanism. That path leaves no evidence by design: the log line is dropped on exit (fixed in #1757), and crash telemetry only listens onuncaughtExceptionMonitor, which never fires for a rejection that has a listener — and the TUI always registers one, sounhandledRejectionexits are completely invisible.While auditing always-on async callbacks that can reject in both streaming and idle states, we also found a concrete crash vector: the Ctrl+V clipboard-image dispatch chains off the async handler with no rejection branch, so any failure inside it becomes an unhandled rejection and takes the whole CLI down via the path above.
What changed
unhandledRejectiondirectly. Registering a real listener would suppress Node's default crash-on-rejection in modes that have no handler of their own (print / server), so when the crash handler is the only listener it rethrows — preserving the default non-zero exit, deduped so the monitor does not double-report. When another handler owns the lifecycle (the TUI), it records the crash without interfering.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.