fix(cli): restore goal-less startup broken by the goal-runtime wait refactor - #10290
Conversation
The interactive external-context-mem0-write suite hangs at CLI startup
('Connecting to MCP servers...') on macOS and ecs-qwen pool runners —
bisected to #10128, tracked in #10272, ubuntu-hosted unaffected. Every
push E2E run currently fails on it, masking all other signal.
Follow the cron-interactive precedent (#6986): exclude it from the push
lanes (linux both sandbox legs, macOS) and keep it in the nightly
isolated matrix so the regression stays visible and the fix is verified
when it lands.
|
Gate pass — an observed, well-evidenced CI unblock that follows established precedent. Template: the body uses shortened Problem: observed, not theoretical. Every post-merge E2E run on Direction: aligned. Quarantine-into-nightly is exactly the pattern #6986 established for Size: not applicable — workflow-only ( Approach: minimal and complete. The three sharded push legs (Linux docker, Linux sandbox:none, macOS) are the only lanes that run the full integration suite on the affected runners; I grepped the other workflows — Risk: no elevated risk signals (no high-risk paths touched). Moving on to code review. 🔍 中文说明门禁通过——这是一个有观测证据、记录充分的 CI 解封改动,且遵循已有先例。 模板: PR 正文使用了简化的 问题: 已观测到,不是理论问题。当前 方向: 对齐。隔离到 nightly 正是 #6986 为 规模: 不适用——仅改工作流( 方案: 最小且完整。三个带分片的 push leg(Linux docker、Linux sandbox:none、macOS)是受影响 runner 上仅有的跑全量集成测试的 lane;我检索了其他工作流—— 风险: 无升级风险信号(未触及高风险路径)。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal for "quarantine a platform-specific E2E hang until the root cause is fixed" is exactly what this diff does: exclude the file from every push-lane vitest invocation on affected runners, and keep it alive in the existing Checked against the diff:
One non-blocking nuance: the nightly leg runs on Testing evidence — this PR's own CI, via API (unattended run; no PR code executed)CI on the reviewed commit is still in flight — the Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The problem side is API-verified too: run 33066706127 lists all eight push E2E lanes as Sandboxed lanes don't apply here: the change is workflow YAML, not product behaviour — 中文说明代码审查我对"将平台相关的 E2E 卡死隔离到根因修复为止"的独立方案与这个 diff 完全一致:从受影响 runner 的每条 push-lane vitest 调用中排除该文件,并在现有的 对照 diff 逐项核查:
一个非阻塞的细节:nightly leg 跑在 测试证据 —— 本 PR 自己的 CI,经 API 获取(无人值守运行;未执行任何 PR 代码)被审提交上的 CI 仍在进行中—— (表格见英文部分) 问题侧同样经 API 核实:run 33066706127 中 8 条 push E2E lane 全部为 沙箱验证通道在此不适用:改动是工作流 YAML 而非产品行为—— — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — every factual claim in this PR checked out against the API, the diff is the minimal complete form of the quarantine, and it reuses the exact mechanism the repo already established for this situation. Stepping back: the problem is real and costly — one platform-specific hang is masking every other E2E signal on post-merge main, verified directly (run 33066706127, all eight lanes red). The approach matches my independent proposal line for line; I found nothing simpler that keeps both the push signal and eventual fix visibility. The diff touches exactly the three lanes that need the exclude and nothing else, the paths and globs check out against main, and the revert condition is named both in the PR body and in the workflow comments, so the quarantine won't be forgotten — #10272 stays the tracking issue and nightly keeps exercising the test. My one reservation is a wording nuance, not a defect: the nightly leg runs on ubuntu, where the test already passes, so fix verification will actually happen on the push lanes after the revert. Non-blocking, noted in the Stage 2 comment. Approving — with CI still running, approval is deferred until it lands green on the reviewed commit; the finalize job posts it. 中文说明置信度:5/5 —— 本 PR 的每一项事实性声明都经 API 核实,diff 是该隔离方案最小且完整的形态,并且复用了仓库为此类情况早已建立的机制。 退一步看:问题真实且代价高——一个平台相关的卡死掩盖了 post-merge main 上所有其他 E2E 信号,已直接核实(run 33066706127,8 条 lane 全红)。方案与我的独立提议逐行吻合;我没有找到更简单且能同时保住 push 信号与修复可见性的做法。diff 恰好只动了需要排除的三条 lane,路径与 glob 均与 main 核对无误;revert 条件在 PR 正文和工作流注释中都有写明,隔离不会被遗忘——#10272 继续作为跟踪 issue,nightly 也会持续运行该测试。 唯一的保留意见是措辞细节而非缺陷:nightly leg 跑在 ubuntu 上,而该测试在那里本就通过,因此修复的真正验证会发生在 revert 之后的 push lane。非阻塞,已在 Stage 2 评论中说明。 予以批准——CI 仍在运行,批准将延后到其在被审提交上全绿后由 finalize 任务代为提交。 — Qwen Code · qwen3.8-max Reviewed at |
Run 33069559004 shows the stall class is broader than mem0: on macOS and ecs-qwen pool, external-context-auto-recall, context-compress-interactive and qwen-serve-channel-workers also fail while ubuntu-hosted stays green (#10198's own ubuntu CI passed at merge). Quarantine them with mem0 per the #6986 precedent; nightly keeps them as a canary.
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
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/e2e.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
@qwen-code /triage |
|
E2E dispatch against this PR head ( PR lanes skip E2E ( E2E dispatch 已针对本 PR head( |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 216 passed · 0 failed · 216 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:216 通过 · 0 失败 · 216 总计 抖动门:✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 10290 — fix(cli): restore goal-less startup broken by the goal-runtime wait refactorVerdict: 中文摘要
Central claim + A/BCentral claim: with chat recording disabled, E2E A/B (real PTY,
|
| cell | placeholder | init message cleared | model requests | reply rendered | verdict |
|---|---|---|---|---|---|
base (tmp/base-tree/dist/cli.js) |
+1.3s | NO — stuck ⠦ Initializing... 15s+ |
0 (submitted prompt sat ⏳ 1 queued) |
no | stuck (as predicted) |
head (dist/cli.js) |
+1.3s | yes, +1.8s | 1 | yes | ready |
Witnesses: evidence/03-e2e-base-stuck-initializing.png, evidence/04-e2e-head-ready-reply-rendered.png (live re-runs under verify-capture.mjs; both matched expectations). Raw JSON in logs/e2e-base2.log, logs/e2e-head2.log.
Oracle note: the input placeholder renders even while uninitialized (v1 of my harness misfired on it — see Corrections), so "ready" is defined behaviorally: footer init message clears and a submitted prompt reaches the model and its body flows through the client. Base fails both; head passes both.
Unit A/B on the compiled module (8 branches per arm)
| branch | base | head |
|---|---|---|
| sync-throw-PUE (timeout path) | reject PUE | resolve true |
| sync-throw-PUE (no-timeout path) | reject PUE | resolve true |
| async-reject-PUE | resolve true | resolve true |
| sync-throw-other | reject (propagates) | reject (propagates) |
| async-reject-other | reject (propagates) | reject (propagates) |
| resolves | resolve true | resolve true |
| pending + timeout 150ms | resolve false | resolve false |
| timeoutMs=0 fall-through | resolve true | resolve true |
Only the two regression branches flip; the other six are parity — the fix changes nothing else. Witnesses: evidence/01-unit-parity-base-rejects-sync-throw.png, evidence/02-unit-parity-head-resolves-sync-throw.png; logs logs/unit-parity-*.log.
Bundle integrity (control purity)
Base control = full recompile of HEAD^1 in a scratch worktree (tsc --build per package, 0 TS errors) + esbuild bundle, with node_modules mirrored by symlinks from the root install (PR touches zero package.json/lockfile lines). Realpath assertions: tmp/base-tree/node_modules/@qwen-code/qwen-code-core → tmp/base-tree/packages/core (base's own build); git diff HEAD^1..HEAD -- packages/core = 0 lines. Minified-chunk extraction confirms the two bundles differ by exactly the hunk: base chunk has const ready = config.getGoalRuntimeReady(); outside the closure; head chunk has await config.getGoalRuntimeReady(); inside the try.
Corrections (description-level, not code requests)
- The PR's "Before" evidence says the CLI logged
CRITICAL: Unhandled Promise Rejection!to the terminal. In my PTY runs on base the stall reproduced exactly, but the CRITICAL text never appeared on the PTY:setupUnhandledRejectionHandler(gemini.tsx) routes the message todebugLogger+appEvents, and the TUI listener is not yet subscribed when the rejection fires. The information survives in the debug log channel, not the terminal — the behavioral half of the description is accurate, the log-visibility half overstates what a user sees. - The integration rig's
waitForText('Type your message')readiness oracle is weaker than its name: the placeholder renders even whileconfigInitializedis false (observed on base). The affected E2E files still failed on base — but at the interaction stage (poll timeouts), consistent with E2E: external-context mem0 tests hang at 'Connecting to MCP servers' on macOS and ecs-qwen pool (pass on ubuntu-hosted) #10272's signal mix (Connecting to MCP servers...appears when MCP servers are configured; with none, the footer showsInitializing...).
Findings
None. Non-blocking observations, for the record:
- The second new test (
does not hide synchronous readiness errors) passes on both arms by design — it guards against future over-suppression; the fix is pinned by the first test, which fails on base with the intended assertion (05-vacuity-new-test-fails-on-base.png: 1 failed / 7 passed on base source, the failure citingresolves.toBe(true)vsGoalPersistenceUnavailableErrorthrown at goal-runtime.ts:57, the hoisted call). - Sibling sweep of the same bug class:
core/client.tsloadGoalRuntimecallsgetReadyinside itstry(safe);acpAgent.tswrapsgetGoalRuntimePrepared()in a promise chain with.catch(safe);useResumeCommand/useBranchCommandcallwaitForGoalRuntimewithout timeout but only inside session-swap flows that cannot occur when recording is disabled — and with the fix they now resolvetruethere instead of rejecting, strictly an improvement.
Not covered
- Per-commit attribution: the PR carries 3 commits; the two quarantine commits cancel within the PR (no CI-file changes in the effective diff at either side), so only commit 3 was exercised.
- Full integration/E2E suites and MCP-configured scenarios (my harness configures no MCP servers; the banner variant
Connecting to MCP servers... (x/y)was not reproduced —Initializing...is the zero-server form of the same gate). - Windows (PR table: not tested there).
- The author's request to dispatch an
e2e.ymlrun against this head remains open to a maintainer — confirmede2e.ymlhas nopull_requesttrigger. - The CRITICAL line itself was not harvested from a debug file in my isolated HOME (debug logging appears gated); irrelevant to the verdict given the behavioral and unit evidence.
- The fake model answers non-SSE JSON, so the head cell renders the client's non-SSE notice rather than a clean streamed reply; the readiness oracle is request arrival + body flow, which both held.
Methodology
CI merge-ref checkout (HEAD merge commit, HEAD^1 base, HEAD^2 head). Base arm built in tmp/base-tree (removed after capture): scripts/generate-git-commit-info.js + per-package tsc --build (0 errors) + esbuild bundle; node_modules mirrored via symlinks (same lockfile — PR changes no dependencies) with @qwen-code/* re-pointed into the worktree's own builds and realpath-asserted. Harnesses (in this artifact dir): e2e-ab2.mjs (node-pty + @xterm/headless, fake OpenAI server counting requests, isolated HOME) and unit-parity.mjs (drives compiled packages/cli/dist on each tree, error class imported through the same resolution chain as the unit). Gates: vitest run src/ui/utils/goal-runtime.test.ts on head (8/8) and on base-with-head-tests (7 passed + 1 intended red), useConfigInitMessage.test.ts + AppContainer.test.tsx on head (173/173), tsc --build on head (exit 0). Assertion tally: unit parity 16, E2E 8, vacuity 8, head goal-runtime 8, adjacent suites 173, typecheck 1, bundle-integrity 2 = 216. Raw logs in logs/.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/cli/src/ui/utils/goal-runtime.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/utils/goal-runtime.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/ui/utils/goal-runtime.test.ts: PPPPP
verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/ui/utils/goal-runtime.test.ts: P (exit 0)
round 2 · packages/cli/src/ui/utils/goal-runtime.test.ts: P (exit 0)
round 3 · packages/cli/src/ui/utils/goal-runtime.test.ts: P (exit 0)
round 4 · packages/cli/src/ui/utils/goal-runtime.test.ts: P (exit 0)
round 5 · packages/cli/src/ui/utils/goal-runtime.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Review — verified, approvingFix mechanism verified against main's code: E2E evidence — dispatched twice against this head (
So this PR turns the entire #10272 hang class green everywhere it ran; the three remaining red lanes fail with unrelated, pre-existing-on-main signatures and need their own issues. Notes: branch is 14 commits behind main but none of them touch 中文:修复机制已对 main 代码核实(同步 throw 移回 try/catch 内);对当前 head dispatch 了两次 E2E,结果一致——#10272 挂起类全部转绿(macOS 两 shard + none 2/3、3/3 + docker 2/3);剩余 3 条红 lane 是 main 上已有的其它类(sdk permission/tool-control、channel-workers 任务创建),与本修复无关,建议另开 issue。approve。 |
chiga0
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — no blocking findings.
Scope
Reviewed: goal-runtime.ts (3-line net change) · goal-runtime.test.ts (21-line addition) · callers of waitForGoalRuntime (AppContainer.tsx:1024, useResumeCommand.ts, useBranchCommand.ts — context read, not patched).
Not reviewed: CI workflow files (net zero change in this diff; quarantine added and removed across commits) · macOS/Windows runtime behaviour (no toolchain available).
CI: still in_progress at review time — 9 E2E shards running, no completed build/lint/unit results visible. Prior rounds on earlier SHAs did not surface failures on the changed files.
Fix correctness
The bug: const ready = config.getGoalRuntimeReady() was evaluated eagerly, outside awaitReady's try block. When getGoalRuntimeReady() throws synchronously (as GoalPersistenceUnavailableError does when persistence is disabled), the throw escaped the guard and surfaced as a rejected promise from the outer waitForGoalRuntime, breaking goal-less startup.
The fix: moves the call to await config.getGoalRuntimeReady() inside the try block. In JavaScript, a synchronous throw inside a try block within an async function is caught by the corresponding catch — the async boundary does not change try/catch semantics for code that runs before the first await. The catch re-throws anything that is not GoalPersistenceUnavailableError, preserving the existing contract for other error types.
AppContainer.tsx:1024 is the main caller — it uses timeoutMs: 5000. After the fix, a synchronous GoalPersistenceUnavailableError is correctly swallowed by awaitReady(), which resolves to void, and waitForGoalRuntime returns true as expected.
Test validity
Test 1 (allows Goal-less sessions when readiness throws synchronously): efficacious — expects.resolves.toBe(true) fails if the fix is reverted (the outer async function would reject instead).
Test 2 (does not hide synchronous readiness errors): passes with both old and new code — a non-GoalPersistenceUnavailableError synchronous throw propagates to the caller as a rejected promise in both paths. The test is a correct contract assertion but does not detect the regression; test 1 is the detector.
Unreviewd dimensions
- Execution rungs 1–3 not run (no toolchain in this environment; witness not available).
- macOS and Windows CI jobs: SKIPPED in visible check set.
Reviewed with AI assistance.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 3a": none — wait, per the brief I should not write that line at all when nothing was cut short, so: no checks were cut short..
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
中文说明
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):"agent 3a":none — wait, per the brief I should not write that line at all when nothing was cut short, so: no checks were cut short.。
未审查:反向审计——评审时间预算不足,未能开始第 1 轮。
— qwen3.8-max via Qwen Code /review (v0.22.2)
main independently landed the same fix for the synchronous getGoalRuntimeReady() throw via #10290. Consolidate the duplicate sync-throw tests into main's pair, keeping this PR's extra coverage (both race and no-timeout branches, per-call invocation count) and adding the timeout-path error-propagation assertion requested in review.
) Merge main, whose #10290 landed the same one-line gate fix (moving getGoalRuntimeReady() back inside the try). Resolve the conflict by keeping the load-bearing "must stay inside the try" comment and consolidating the two branches' duplicated tests: one swallow test covering both the unbounded and timed branches, and one escape test that now also pins rejection propagation on the Promise.race branch.
|
Released in v0.22.3. |





What this PR does
Restores goal-less interactive startup by keeping the goal-runtime readiness lookup inside the existing persistence-unavailable guard. The temporary E2E quarantine is removed, so all affected tests continue running on every push lane.
Why it's needed
When chat recording is disabled, the readiness lookup throws
GoalPersistenceUnavailableErrorsynchronously instead of returning a rejected promise. The timeout refactor in #10128 moved that lookup outside the guard, so the startup effect rejects before the initialized flag is set and the TUI remains onConnecting to MCP servers...indefinitely. The integration harness launches interactive sessions with--no-chat-recording, which explains the broad post-merge E2E failures tracked in #10272.This failure is deterministic, not platform-specific.
--no-chat-recordingmakesConfig.getGoalRuntime()throw on its first branch (!chatRecordingEnabled), sogetGoalRuntimeReady()always throws synchronously on every platform. The "ubuntu-hosted passes" observation in #10272 reflects shard composition, not a platform difference: non-interactive (-p) tests never renderAppContainerand are unaffected, so a shard without interactive files stays green anywhere. Signal counts from the latest red main run (33080103209, 7 failing jobs):Connecting to MCP serversx5651,Poll timed outx50, network errors x0 -- the entire red is this one class.Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: the CLI logged
CRITICAL: Unhandled Promise Rejection! Reason: GoalPersistenceUnavailableErrorafter configuration completed, then remained onConnecting to MCP servers...until the E2E poll timed out.After: the focused goal-runtime suite passes 8/8, including both synchronous-error branches. Repository prepare/build and full typecheck pass. The local PTY E2E runner could not start Ink tests (
posix_openpt failed: Device not configured); the non-PTY serve E2E passed 4/4, and the same production fix was independently verified against the mem0 and full interactive suites in #10303.Verification gap: E2E does not run on PR lanes (
Integration Tests (CLI, No Sandbox)is SKIPPED here; e2e.yml has nopull_requesttrigger). Please dispatch an e2e run against this PR head before merging -- that is the only outstanding check.Tested on
Environment (optional)
Node 22 project toolchain; macOS local worktree.
Risk & Scope
Linked Issues
Fixes #10272
Also fixes the auto-filed duplicates of the same regression:
Fixes #10293
Fixes #10298
Fixes #10299
Fixes #10306
Fixes #10308
中文说明
本 PR 做了什么
恢复无 Goal 的交互式启动:goal runtime 就绪查询重新置于现有的“持久化不可用”保护逻辑内。临时 E2E quarantine 已撤销,所有受影响测试继续在每条 push lane 中运行。
为什么需要
关闭聊天记录时,就绪查询会同步抛出
GoalPersistenceUnavailableError,而不是返回 rejected promise。#10128 的超时重构把该查询移到了保护逻辑之外,导致启动 effect 在 initialized 标志写入前 reject,TUI 永久停在Connecting to MCP servers...。集成测试框架通过--no-chat-recording启动交互会话,这解释了 #10272 跟踪的大范围合并后 E2E 失败。该失败是确定性的,与平台无关。
--no-chat-recording使Config.getGoalRuntime()在第一个分支(!chatRecordingEnabled)就抛出,因此getGoalRuntimeReady()在任何平台上 都必定同步抛出。#10272 中"ubuntu-hosted 通过"反映的是分片构成而非平台差异:非交互(-p) 测试不渲染AppContainer,不受影响,因此不含 interactive 文件的分片在任何平台都是绿的。 最近一次 main 红色运行(33080103209,7 个 job 失败)的信号统计:Connecting to MCP servers5651 次、Poll timed out50 次、网络错误 0 次 —— 红的全部是这一个类。评审测试计划
如何验证
前后证据
修复前:配置初始化完成后,CLI 记录
CRITICAL: Unhandled Promise Rejection! Reason: GoalPersistenceUnavailableError,随后一直停在Connecting to MCP servers...,直到 E2E 轮询超时。修复后:聚焦 goal-runtime 测试 8/8 通过,覆盖两类同步异常分支;仓库 prepare/build 和完整 typecheck 通过。本地 PTY E2E runner 无法启动 Ink 测试(
posix_openpt failed: Device not configured);非 PTY serve E2E 4/4 通过,相同生产修复也已在 #10303 中针对 mem0 与完整 interactive 套件独立验证。验证缺口:E2E 不在 PR lane 运行(此处
Integration Tests (CLI, No Sandbox)为 SKIPPED; e2e.yml 无pull_request触发器)。合并前请针对本 PR head 手动 dispatch 一次 e2e 运行 —— 这是唯一尚未闭环的检查。测试环境
环境(可选)
Node 22 项目工具链;macOS 本地 worktree。
风险与范围
关联 Issue
Fixes #10272
同一回归的自动重复报告(一并关闭):#10293、#10298、#10299、#10306、#10308。