fix(cli): swallow synchronous goal-persistence throws at the startup gate (#10298) - #10307
Closed
qwen-code-dev-bot wants to merge 2 commits into
Closed
fix(cli): swallow synchronous goal-persistence throws at the startup gate (#10298)#10307qwen-code-dev-bot wants to merge 2 commits into
qwen-code-dev-bot wants to merge 2 commits into
Conversation
Collaborator
Author
What was fixedThe post-merge E2E suite was red on every shard of every run because a class of Root cause
const ready = config.getGoalRuntimeReady(); // throws synchronously
const awaitReady = async () => {
try {
await ready; // catch never sees the throw
} catch (error) { ... }
};
FixMove the readiness call back inside the Evidence
VerificationCommands actually run (all from the repo root unless noted):
Mutation probe: the two new witness tests fail against the pre-fix code and pass Note: the E2E suite still exercises these files through the normal CI lanes; the 中文说明修复内容post-merge 的 E2E 套件在每个 run 的所有 shard 上都变红，原因是一类交互式测试在启动横幅 根因
const ready = config.getGoalRuntimeReady(); // 同步抛出
const awaitReady = async () => {
try {
await ready; // catch 永远看不到这个 throw
} catch (error) { ... }
};当 goal 持久化不可用时， 修复把就绪调用移回 证据
验证实际运行的命令（除注明外均在仓库根目录）：
变异探针：两个新见证测试在修复前的代码上失败、修复后通过，因此该守卫确有覆盖。 说明：E2E 套件仍会通过正常 CI 通道执行这些文件； 🧠 Handled by Qwen Code · model/模型 |
github-actions
Bot
added
the
review/self-reported
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed bug, well evidenced. The main-branch E2E run behind issue #10298 went red on nearly every shard, and the job logs show interactive tests frozen on Direction: aligned — this restores a broken startup path and un-breaks the post-merge E2E matrix; it claims no scope beyond pre-#10128 semantics. Size: not applicable (no core-infrastructure paths — Approach: scope feels exactly right — the minimal fix (move the call back inside the Risk: no elevated risk signals (no high-risk paths matched). Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，证据充分。issue #10298 对应的 main 分支 E2E 运行几乎在每个 shard 上都变红，任务日志显示交互式测试卡在 方向：对齐——恢复损坏的启动路径，修复 post-merge E2E 矩阵；没有声称超出 #10128 之前语义的范围。 规模：不适用（未触及核心基础设施路径——仅 方案：范围恰好——最小修复（把调用移回 风险：无升级风险信号（未命中高风险路径）。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewThe root cause checks out in code. The fix is the minimal one: the call moves back inside the The two new witness tests cover the exact shape that regressed silently — synchronous throw on both the unbounded and the bounded path — mirroring the existing async-rejection tests in style. Comments explain the non-obvious why (the call must stay inside the Testing evidenceFrom the PR's own CI on the reviewed commit — quoted via the API; this run executes no PR code. No failures at review time; the unit suite was still running when this pass was written (the table is updated in place once CI settles). Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The Sandboxed verification would settle this: 中文说明代码审查：根因在代码中核实无误。 修复是最小修复：调用移回 两个新见证测试恰好覆盖悄无声息回归的那个形态——无界与有界路径上的同步抛出——风格与现有异步拒绝测试一致。注释解释了不明显的原因（调用必须留在 测试证据：来自该提交上 PR 自己的 CI，通过 API 读取；本次运行不执行任何 PR 代码。审查时无失败；单元套件在本轮撰写时仍在运行（表格会在 CI 落定后原地更新）。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — clean, minimal fix for a verified main-branch outage; the only non-blocking gap is that the live-TUI hang itself can't be settled by PR CI. This is what a good autofix looks like. The problem is real and costly — every interactive E2E shard on My independent take before reading the diff was identical to what the PR does — there is no simpler path here. The diff carries nothing extra: two files, both directly in scope, and the comments explain the subtle invariant (why the call must stay inside the The one reservation, already flagged in Stage 2: the interactive E2E leg where the hang manifested only runs post-merge, so the unit witnesses pin the mechanism but a maintainer may still want Approval deferred until CI lands green on 中文说明这是一次高质量的自动修复。问题真实且代价高——main 上每个交互式 E2E shard 都变红，每个无 Goal 会话都面临同样的启动死亡风险——而 PR 直击根因而非掩盖它：把那一行生产代码移回 我在读 diff 之前的独立想法与 PR 的做法完全一致——这里不存在更简单的路径。diff 没有任何多余内容：两个文件都直接服务于目标，注释解释了那个微妙的不变量（调用为何必须留在 唯一的保留意见（Stage 2 已指出）：挂起发生的交互式 E2E 通道只在 post-merge 运行，因此单元见证测试钉住了机制，但维护者在合入前可能仍想用 批准推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
previously approved these changes
Aug 27, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): interactive/external-context-mem0-write.test.ts — no such file or directory; 5 tests to pass — this review observed 25349 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：interactive/external-context-mem0-write.test.ts — no such file or directory; 5 tests to pass — this review observed 25349 passed。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Collaborator
Author
|
🐑 Merge conflict with main detected — dispatched the autofix loop to resolve it. / 检测到与 main 的合并冲突，已触发 autofix 处理。 |
Collaborator
Author
# Conflicts: # packages/cli/src/ui/utils/goal-runtime.ts
Collaborator
Author
|
🤖 Addressed the latest review feedback (round 1/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/10 轮）。改动内容与我反驳保留之处如下： Address-review summary — PR #10307 (issue #10298)Round outcome: made a change (merge of Context discovered this roundWhile merging, I found that main already merged this exact fix via
Whether the PR should be closed as superseded by #10290 is a maintainer decision, not one I can make here. Feedback triage[rv:5042651750] COMMENTED review (0 findings) — informational, no code action
No inline comments, no issue-level comments, no failed checks were present this round. Conflict resolution (
|
qwen-code-dev-bot
requested review from
LaZzyMan,
doudouOUC,
qqqys,
tanzhenxin,
wenshao and
yiliang114
as code owners
August 27, 2026 17:53
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 6b": executing packages/cli/src/ui/utils/goal-runtime.test.ts — the review worktree has no node_modules (vitest unresolvable), and npm ci plus the prepare bu….
Test Plan (not a blocker): interactive/external-context-mem0-write.test.ts — no such file or directory; 5 tests to pass — this review observed 25515, 22090, 1794, 1667, 605, 4333, 639 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/utils/goal-runtime.test.ts:47 — [probe] bounded branch lacks a test pinning that non-swallowable errors still reject
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 6b"：executing packages/cli/src/ui/utils/goal-runtime.test.ts — the review worktree has no node_modules (vitest unresolvable), and npm ci plus the prepare bu…。
Test Plan（非阻断）：interactive/external-context-mem0-write.test.ts — no such file or directory; 5 tests to pass — this review observed 25515, 22090, 1794, 1667, 605, 4333, 639 passed。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Comment on lines
+59
to
63
| // The call must stay inside the try: Config.getGoalRuntimeReady() | ||
| // THROWS synchronously (rather than returning a rejected promise) | ||
| // when persistence is unavailable, and escaping the catch turned it | ||
| // into an unhandled rejection that killed the startup effect. | ||
| await config.getGoalRuntimeReady(); |
Collaborator
There was a problem hiding this comment.
[Suggestion] R2-1: The production fix this PR describes no longer comes from this PR — it already landed on main via PR 10290 (commit 1637aa968b), which this branch merged during conflict resolution. Against the current merge base the net production delta is zero: this PR now contributes the explanatory comment above, the witness-test reorganization plus one new unbounded-path test, and a prettier reflow in teamHelpers.test.ts. The closing issue 10298 was already fixed on the base before this PR's merge commit.
The description is now inverted in two places: it says PR 10290 "can be dropped or reverted in favour of this fix" and that this PR "supersedes the quarantine approach" there — but PR 10290 is merged and carries the identical fix. Verification step 1 ("On main (pre-fix)… Observe the test hang") no longer reproduces on current main, so the evidence is unfalsifiable for future readers; and anyone later reverting "the 10307 fix" to undo the behaviour would revert only a comment and tests while believing the production fix is gone, leaving the attribution of the actual fix ambiguous.
Update the description to state that the production fix came via the PR 10290 merge and frame this PR's net contribution as the comment plus the witness tests — or close as superseded by PR 10290.
Witness: not run — the claim is git-history/PR-metadata fidelity, settled by git ancestry (PR 10290's commit is an ancestor of the merge base; the net diff contains zero production-behavior lines) and by the live PR/issue states (PR 10290 merged, issue 10298 closed at the same time).
中文说明
本 PR 描述中的生产修复已不再来自本 PR —— 该修复已通过 PR 10290（提交 1637aa968b）合入 main，而本分支在解决冲突时合入了该提交。相对当前合并基点，本 PR 的生产代码净差量为零：现在只贡献上面这段说明性注释、见证测试的重组加一个新增的无界路径测试，以及 teamHelpers.test.ts 中的一处 prettier 重排。关闭的 issue 10298 在本 PR 的合并提交之前就已经在基点上被修复。
描述现在有两处与事实相反：描述称 PR 10290 "可以放弃或回退，改用本修复"，并称本 PR "取代其中的隔离方案" —— 但 PR 10290 已经合入并且携带了完全相同的修复。验证步骤 1（"在 main（修复前）上……观察测试挂起"）在当前 main 上不再复现，因此这些证据对后来的读者不可证伪；而且如果将来有人想通过回退 "10307 的修复" 来撤销该行为，实际只会回退一段注释和测试，却以为生产修复被移除了，从而让实际修复的归属变得含糊。
建议更新描述，说明生产修复是通过 PR 10290 的合入带来的，并将本 PR 的净贡献定位为注释加见证测试 —— 或者直接以被 PR 10290 取代为由关闭本 PR。
见证：未运行 —— 该结论属于 git 历史 / PR 元数据的保真度问题，由 git 祖先关系（PR 10290 的提交是合并基点的祖先；净差量不含任何生产行为代码行）与线上 PR/issue 状态（PR 10290 已合并、issue 10298 同时关闭）确定。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Collaborator
Author
There was a problem hiding this comment.
Verified against git: 1637aa968b (PR 10290) is an ancestor of the current merge base and carries the identical production fix (the call moved inside the try); this branch's net diff against the merge base contains zero production-behavior lines. The finding stands.
Both remedies are GitHub-level actions this bot cannot perform, and the choice between them is a maintainer decision, so this thread is left unresolved:
- A — close as superseded by PR 10290 (cleanest attribution, zero net delta).
- B — keep this PR with a corrected description: the production fix came via the PR 10290 merge, and this PR contributes the invariant-pinning comment (the call must stay inside the
trybecauseConfig.getGoalRuntimeReady()throws synchronously) plus the unbounded-path witness test (main's synchronous-throw test only exercises thetimeoutMspath).
Recommendation: B for the comment and the extra witness; A if a zero-delta history is preferred. Which way should this go? (For B the PR body also needs editing — the bot cannot do that.)
中文说明
已通过 git 核实：1637aa968b（PR 10290）是当前合并基点的祖先，且携带完全相同的生产修复（调用已移入 try 内部）；本分支相对合并基点的净差量不含任何生产行为代码行。该发现成立。
两种处理方式都是本机器人无法执行的 GitHub 层面操作，且二者之间的取舍属于维护者决策，因此本线程保持未解决状态：
- A — 以被 PR 10290 取代为由关闭（归属最清晰，净差量为零）。
- B — 保留本 PR 但更正描述：生产修复是通过 PR 10290 的合入带来的，本 PR 贡献的是固定约束的注释（调用必须留在
try内，因为Config.getGoalRuntimeReady()会同步抛出异常）以及无界路径的见证测试（main 上的同步抛出测试只覆盖了timeoutMs路径）。
建议：选 B，以保留该注释和额外的见证测试；若偏好零差量的历史则选 A。这个问题应如何处理？（若选 B，还需编辑 PR 正文 —— 机器人无法执行该操作。）
Collaborator
Author
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round for PR #10307 (issue #10298) — no code changesNo code changes were made this round. The single new finding was verified Finding triage[R2-1 / rc:3875106507] PR description no longer matches the PR's net diff — escalated (needs maintainer decision)The finding's factual claims were verified locally against
The requested remedy is a decision this bot may not make, and neither option
Recommendation: B for the invariant comment and the unbounded-path Review-body items — no action requested
Failed checksEvery check listed for this round is CANCELLED, not failed, and no still-red VerificationNo code changed this round, so no build/typecheck/lint/test commands were run.
中文说明PR #10307（issue #10298）自动修复审查轮次 — 无代码变更本轮未做任何代码变更。唯一的新发现已通过 git 证据核实，现升级给维护者决策， 发现分诊[R2-1 / rc:3875106507] PR 描述与 PR 的净差量不再相符 — 已升级（需维护者决策）该发现的事实性论断已在本地对照
所请求的处理方式是一个本机器人无权做出的决定，而且两个选项都无法在检出目录中执行：
建议：选 B，以保留该约束注释和无界路径见证测试 —— 净差量很小，其余均为 审查正文条目 — 本轮不要求处理
失败的检查本轮列出的所有检查均为 CANCELLED（已取消）而非失败，也没有持续红色的检查。 验证本轮没有代码变更，因此未运行任何 build / typecheck / lint / 测试命令。
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Collaborator
|
Closing as superseded by #10290, which already landed the same goal-less startup fix on main. Post-merge E2E now passes the previously stalled interactive suites, so merging another duplicate implementation would add no fix. |
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
3 participants
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.
What this PR does
Post-merge E2E was red on every shard because a class of interactive tests hangs forever at the startup banner
Connecting to MCP servers...and then polls out. This PR fixes the root cause: the goal-runtime startup gate introduced by #10128 hoisted its readiness call outside thetrythat is meant to treatGoalPersistenceUnavailableErroras "settled".Config.getGoalRuntimeReady()throws that error synchronously (not as a rejected promise) whenever goal persistence is unavailable, so the throw escaped the catch, became an unhandled promise rejection, and killed the startup effect before the TUI finished initializing. The fix moves the call back inside thetryso both the synchronous throw and the async rejection are swallowed, and adds witness tests for the synchronous-throw shape that previously shipped untested.Why it's needed
Every integration test launches the CLI with
--no-chat-recording, which disables goal persistence. With #10128's gate, that made every interactive/TUI E2E test hang at the init banner and time out (Poll timed out after ~150 attempts), turning the entire post-merge E2E matrix red onmain. The existing unit tests only mocked the async-rejection shape, so the synchronous-throw shape regressed silently. This restores Goal-less sessions to working order while keeping the bounded-wait behaviour #10128 intended.Reviewer Test Plan
How to verify
Reproduce the hang on the pre-fix code, then confirm the fix clears it:
main(pre-fix), build and bundle, then run one affected file:npm run build && npm run bundle && npx cross-env QWEN_SANDBOX=false vitest run --root ./integration-tests interactive/external-context-mem0-write.test.ts --retry 0. Observe the test hang atConnecting to MCP servers...and fail with a poll timeout; the CLI debug log recordsCRITICAL: Unhandled Promise Rejection! ... GoalPersistenceUnavailableError ... at waitForGoalRuntime.cd packages/cli && npx vitest run src/ui/utils/goal-runtime.test.ts. Expect 8/8. The two new witnesses (allows Goal-less sessions when readiness throws synchronously,swallows a synchronous readiness throw inside the bounded wait too) fail on the pre-fix code, confirming coverage.waitForGoalRuntimecallers are unaffected:cd packages/cli && npx vitest run src/ui/hooks/useResumeCommand.test.ts src/ui/hooks/useBranchCommand.test.ts. Expect 55/55.Evidence (Before & After)
Before:
interactive/external-context-mem0-write.test.tshangs, screen frozen onConnecting to MCP servers... (1/1), debug log shows an unhandledGoalPersistenceUnavailableErrorrejection fromwaitForGoalRuntime; test fails after the poll timeout.After: the same file passes 5/5 (each test ~2.5–3.4s) and
external-context-auto-recall.test.tspasses; no unhandled rejection in the debug log.Tested on
Environment (optional)
Local
npm run build+npm run bundle, integration tests vianpx vitest run --root ./integration-testswithQWEN_SANDBOX=false(thesandbox:noneleg). No sandbox container involved.Risk & Scope
waitForGoalRuntime; the only behavioural change is that a synchronousGoalPersistenceUnavailableErroris now swallowed (treated as settled) exactly as the async rejection already was — matching pre-fix(cli): bound the goal-runtime startup wait and skip the no-op Bun memory relaunch #10128 semantics. Other errors still propagate.Linked Issues
Fixes #10298
References the root-cause tracking issue #10272 and supersedes the quarantine approach in #10290.
中文说明
本 PR 做了什么
post-merge 的 E2E 在每个 shard 上都变红，因为一类交互式测试会在启动横幅
Connecting to MCP servers...处永久挂起，随后轮询超时。本 PR 修复根因：#10128 引入的 goal-runtime 启动门把就绪调用提升到了本应把GoalPersistenceUnavailableError视为“已就绪”的try之外。每当 goal 持久化不可用时，Config.getGoalRuntimeReady()会同步抛出该错误（而不是返回被拒绝的 promise），于是这个抛出逃过 catch，变成未处理的 promise 拒绝，并在 TUI 完成初始化前杀死了启动副作用。修复把调用移回try内部，使同步抛出和异步拒绝都被吞掉，并为以前未经测试就合入的同步抛出形态新增见证测试。为什么需要
每个集成测试都用
--no-chat-recording启动 CLI，这会禁用 goal 持久化。在 #10128 的启动门下，这让每个交互式/TUI E2E 测试都挂在初始化横幅上并超时（Poll timed out after ~150 attempts），导致main上整个 post-merge E2E 矩阵变红。现有单元测试只 mock 了异步拒绝形态，因此同步抛出形态悄无声息地回归了。本修复在无 Goal 会话恢复正常工作的同时，保留了 #10128 预期的有界等待行为。评审者测试计划
如何验证
先在修复前的代码上复现挂起，再确认修复消除了它：
main（修复前）上构建并打包，然后运行一个受影响的文件：npm run build && npm run bundle && npx cross-env QWEN_SANDBOX=false vitest run --root ./integration-tests interactive/external-context-mem0-write.test.ts --retry 0。观察测试挂在Connecting to MCP servers...并以轮询超时失败；CLI 调试日志会记录CRITICAL: Unhandled Promise Rejection! ... GoalPersistenceUnavailableError ... at waitForGoalRuntime。cd packages/cli && npx vitest run src/ui/utils/goal-runtime.test.ts。预期 8/8。两个新见证测试（allows Goal-less sessions when readiness throws synchronously、swallows a synchronous readiness throw inside the bounded wait too）在修复前的代码上失败，证明确有覆盖。waitForGoalRuntime的其他调用方不受影响：cd packages/cli && npx vitest run src/ui/hooks/useResumeCommand.test.ts src/ui/hooks/useBranchCommand.test.ts。预期 55/55。证据（前后对比）
修复前：
interactive/external-context-mem0-write.test.ts挂起，界面冻结在Connecting to MCP servers... (1/1)，调试日志显示来自waitForGoalRuntime的未处理GoalPersistenceUnavailableError拒绝；测试在轮询超时后失败。修复后：同一文件 5/5 通过（每个测试约 2.5–3.4 秒），
external-context-auto-recall.test.ts也通过；调试日志中无未处理拒绝。测试平台
环境（可选）
本地
npm run build+npm run bundle，集成测试通过npx vitest run --root ./integration-tests且QWEN_SANDBOX=false（sandbox:none通道）运行。不涉及沙箱容器。风险与范围
waitForGoalRuntime；唯一的行为变化是同步的GoalPersistenceUnavailableError现在会被吞掉（视为已就绪），与异步拒绝本来的处理一致——符合 fix(cli): bound the goal-runtime startup wait and skip the no-op Bun memory relaunch #10128 之前的语义。其他错误仍会传播。关联 Issue
Fixes #10298
引用根因跟踪 issue #10272，并取代 #10290 中的隔离方案。