-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(cli): prevent silent VP-mode crash by adding uncaughtException handler and error visibility #8088
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
fix(cli): prevent silent VP-mode crash by adding uncaughtException handler and error visibility #8088
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
1a85670
fix(cli): prevent silent VP-mode crash by adding uncaughtException ha…
1d43cd3
fix(cli): consolidate uncaughtException handler into a single listene…
qwen-code-dev-bot ad2ab69
Merge branch 'main' into fix-vp-silent-crash-v2
qwen-code-dev-bot 16de941
fix(cli): harden uncaughtException handler and scope render-error ech…
c8b38ae
Merge branch 'main' into fix-vp-silent-crash-v2
qwen-code-dev-bot 2d76aee
Merge branch 'main' into fix-vp-silent-crash-v2
wenshao b3e57fb
Merge branch 'main' into fix-vp-silent-crash-v2
qwen-code-dev-bot fe196aa
fix(cli): address review feedback on VP crash handler (#8088)
qwen-code-ci-bot 307442d
Merge branch 'main' into fix-vp-silent-crash-v2
qwen-code-dev-bot 2ea51c4
Merge branch 'main' into fix-vp-silent-crash-v2
qwen-code-dev-bot bb671a4
Merge branch 'main' into fix-vp-silent-crash-v2
qwen-code-dev-bot f71d37d
fix(cli): move uncaught-exception helpers to a leaf module (#8088)
qwen-code-dev-bot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion]
setupUncaughtExceptionHandler(~60 lines) has no dedicated unit tests despite having multiple testable branches. — Concrete cost: handler replacement logic (two-listener conflict), synchronous debug log write (async abandoned byprocess.exit), alternate-screen escape (TTY guard), and PTY race suppression could all silently regress. The SIGHUP and render-error echo paths added in the same PR are tested; this function is not.Suggested tests: (a) PTY race error is suppressed; (b) debug log is written synchronously with correct format; (c) alternate-screen escape sequences are written when
stdout.isTTY; (d) escape sequences are skipped when stdout is not a TTY; (e)process.exit(1)is called; (f) previous handler is removed before new one is installed.— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch —
setupUncaughtExceptionHandlerdoes deserve dedicated coverage for the branches you list (PTY-race suppression, the synchronous debug-log write, theisTTYalternate-screen guard,process.exit(1), and removing the previous handler before installing the new one).Deferring this one for now: the PR has completed five change-producing rounds and is in critical-only mode, so this round lands only the Critical that was dead-bundling the CLI (the entry↔lazy-module cycle, fixed by moving the helpers into
utils/uncaught-exception-handler.ts). Per the repo's review policy, non-Critical suggestions past five rounds are deferred to a follow-up rather than widening the diff here. Leaving this thread open so the six test cases are tracked and not dropped — they'd make a good small follow-up PR.中文说明
说得对——
setupUncaughtExceptionHandler确实值得为你列出的这些分支补专门的覆盖(PTY 竞态抑制、同步写调试日志、isTTY备用屏守卫、process.exit(1),以及在安装新处理器前先移除旧处理器)。先延后这一项:本 PR 已经完成五个产生改动的轮次、进入仅处理 Critical 的模式,因此本轮只落地那个让打包 CLI 失效的 Critical(entry↔懒加载模块成环,已通过将 helper 挪入
utils/uncaught-exception-handler.ts修复)。按仓库的评审政策,超过五轮后的非 Critical 建议延后到后续处理,以免在此处扩大 diff。保持本线程开放,以便跟踪这六个测试用例、不被丢弃——它们很适合作为一个小的后续 PR。