Skip to content

fix(cli): wait for the startup chat before an OpenTUI turn sends (#11042) - #11046

Merged
wenshao merged 5 commits into
mainfrom
autofix/issue-11042
Sep 5, 2026
Merged

fix(cli): wait for the startup chat before an OpenTUI turn sends (#11042)#11046
wenshao merged 5 commits into
mainfrom
autofix/issue-11042

Conversation

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

What this PR does

A prompt typed into the OpenTUI renderer in the first moments of a session was silently dropped. The composer was already on screen and already accepting input, but the session behind it had not finished starting, so the turn ended immediately with Chat not initialized and nothing was ever sent to the model. The user saw their own text echoed, an error mark, and then nothing — no request, no retry, no queue.

This makes an OpenTUI turn wait for the session that startup is still building before it sends, on the same bounded budget the renderer already uses to self-heal its slash-command registry through the identical window. When the session is ready the wait costs nothing: it checks once and proceeds.

Why it's needed

The E2E Interactive - OpenTUI renderer (bun) leg has been going red on main with no commit to blame, and the auto-filed issues point at whatever commit happened to be at the head of the run. The run history shows the leg failing before the commit named in the linked issue and passing after it, and one SHA both failing and passing thirty minutes apart — so this is a standing defect in the renderer, not a regression from any single change.

The reason only this leg sees it is that the renderer starts up in a different order than ink. ink holds its composer closed until the session is fully initialized, so an early submit simply cannot happen there. The OpenTUI renderer mounts its composer first and starts session initialization from that same mount, which means the composer is accepting input while initialization is still in flight. A turn started inside that window is told initialization is already done, proceeds, and then finds there is no session to send to.

This is a real user-facing bug, not only a CI annoyance: anyone who types quickly at launch under the OpenTUI renderer loses their first prompt. It is also the leg's only failure mode in these runs — every test passes and the run then exits non-zero, which is why the detector had no test name to dedupe on.

Reviewer Test Plan

How to verify

The defect reproduces deterministically on a machine where session initialization takes long enough to overlap the first submit, which is the case on a loaded CI runner and in a container.

  1. Build the bundle, then run the interactive suite under the OpenTUI renderer with a pinned bun, exactly as the CI leg does.
  2. Before this change, four interactive specs fail on all three vitest attempts and the run exits non-zero. The captured terminal screen shows the typed prompt followed by Chat not initialized, and the scripted model server records zero requests — the prompt never became a turn.
  3. Run the same four specs under the ink renderer on the same machine: they pass. That differential is what pins the defect to this renderer rather than to the tests or the machine.
  4. After this change, the four specs pass and the full leg exits zero.
  5. For the unit-level witness, remove the new wait and re-run the renderer's live-turn spec: the new case fails with Error: Chat not initialized. Restore it and the whole file passes. That is the guard's coverage proof.

Expected: the leg is green and an early submit produces a normal turn. Observed before: the leg exits non-zero with no failing test line, and an early submit is dropped.

Evidence (Before & After)

Before — full leg under the OpenTUI renderer, CI's own command and environment:

Test Files  4 failed | 6 passed | 1 skipped (11)
     Tests  4 failed | 15 passed | 2 skipped (21)
  Duration  460.55s        exit 1

Terminal screen captured from the failing run, where the first prompt should have opened a turn:

Type your message or @path/to/file> Start the review.✖︎ Chat not initialized
midTurn: false
All tool calls found: []

Differential, same machine, same four files, ink renderer: 4 passed (4), 7 passed (7), 34.81s, exit 0.

After — full leg under the OpenTUI renderer, same command and environment:

Test Files  10 passed | 1 skipped (11)
     Tests  19 passed | 2 skipped (21)
  Duration  182.83s        exit 0

Unit witness, mutation probe: guard removed → the new case fails Error: Chat not initialized (1 failed | 44 passed); guard restored → 45 passed.

Tested on

OS Status
🍏 macOS ⚠️ not tested
🪟 Windows ⚠️ not tested
🐧 Linux ✅ tested

Environment (optional)

Linux runner. The bundle from a full build, driven under bun pinned to the same version the CI leg uses, installed into a temporary prefix outside the repository so the working tree and lockfile are untouched. Interactive specs ran against the repository's scripted model server; the one live-model spec ran against the configured endpoint.

Risk & Scope

  • Main risk or tradeoff: the wait is bounded rather than event-driven, because the session's in-flight initialization promise is not exposed outside the core package and this change deliberately does not touch core. The budget matches the one the renderer already uses for the same window elsewhere, and a session that never finishes initializing still surfaces its own error rather than hanging the prompt.
  • The wait does not consult the abort signal, again matching the existing sibling self-heal. Worst case an interrupt issued inside the startup window is honoured up to the bound late, and is then applied by the send path as usual.
  • Starting the session directly instead of waiting was rejected: the client's own initialize guards only on an already-built session, so a second caller inside the window would build a second one.
  • Not validated / out of scope: macOS and Windows, where this leg does not run. The live-model compression spec keeps its own model-latency exposure and needed vitest retries both before and after this change; that is a separate, pre-existing class. Other OpenTUI code does not read the model session during this window — its history reads come from the renderer's own transcript store — so the turn path was the only exposed site and no wider gate was added.
  • Breaking changes / migration notes: none. Behaviour when the session is already ready is byte-for-byte the previous path; the 44 pre-existing cases in the touched spec file are unchanged and green.

Linked Issues

Fixes #11042

The issue is filed per commit and names the rewind classifier refactor at 39a84c9e1d. That commit is not the cause: the same leg failed at an ancestor of it and passed at a descendant carrying the same code, and one unrelated SHA both failed and passed. The defect this PR closes is what made those runs red.

中文说明

这个 PR 做了什么

在 OpenTUI 渲染器下,会话刚启动的头几秒里输入的 prompt 会被静默丢弃。输入框已经显示在屏幕上并且已经可以接收输入,但它背后的会话还没有完成启动,于是这一轮对话立刻以 Chat not initialized 结束,什么都没有发给模型。用户看到自己输入的文字被回显、一个错误标记,然后就没有下文了 —— 没有请求,没有重试,也没有进入队列。

本 PR 让 OpenTUI 的一轮对话在发送之前等待启动过程仍在构建的会话,使用的有界时长与该渲染器已有的、用于穿过完全相同时间窗自愈斜杠命令注册表的机制一致。当会话已经就绪时,这个等待不产生任何开销:只判断一次就继续执行。

为什么需要它

E2E Interactive - OpenTUI renderer (bun) 这个 leg 一直在 main 上变红,却没有一个可以归责的 commit,而自动创建的 issue 会指向该次运行头部恰好所在的那个 commit。运行历史显示:这个 leg 在关联 issue 所点名的 commit 之前就失败过,在其之后又通过了;并且有一个 SHA 在相隔三十分钟的两次运行中既失败又成功 —— 所以这是渲染器中长期存在的缺陷,而不是任何单个改动引入的回归。

只有这个 leg 会遇到的原因是:该渲染器的启动顺序与 ink 不同。ink 会在会话完全初始化之前保持输入框关闭,因此在 ink 下根本不可能出现过早提交。OpenTUI 渲染器先挂载输入框,并从同一个挂载时机开始会话初始化,这意味着初始化仍在进行时输入框就已经在接收输入了。在这个时间窗内开始的一轮对话会被告知初始化已经完成,于是继续执行,然后发现根本没有可发送的会话。

这是一个真实的、面向用户的 bug,而不只是 CI 的烦恼:任何在 OpenTUI 渲染器下启动后快速输入的人都会丢失自己的第一个 prompt。它也是这些运行中该 leg 唯一的失败形态 —— 所有测试都通过,然后运行以非零码退出,这正是检测器拿不到测试名去做去重的原因。

审阅测试计划

如何验证

只要一台机器上的会话初始化耗时长到足以与第一次提交重叠,这个缺陷就能确定性复现;负载较高的 CI runner 和容器环境就是这种情况。

  1. 构建 bundle,然后像 CI leg 一样,用锁定版本的 bun 在 OpenTUI 渲染器下运行交互式测试套件。
  2. 在本次修改之前,四个交互式 spec 会在 vitest 的三次尝试中全部失败,运行以非零码退出。抓取到的终端屏幕显示:输入的 prompt 后面跟着 Chat not initialized,而脚本化的模型服务器记录到的请求数为零 —— 这个 prompt 根本没有成为一轮对话。
  3. 在同一台机器上用 ink 渲染器运行同样的四个 spec:它们全部通过。正是这个对照实验把缺陷钉在了这个渲染器上,而不是测试或机器上。
  4. 在本次修改之后,这四个 spec 通过,完整 leg 以 0 退出。
  5. 单元层面的见证:移除新增的等待,再运行该渲染器的 live-turn spec,新用例会以 Error: Chat not initialized 失败;恢复后整个文件通过。这就是该防护的覆盖证明。

预期:leg 为绿色,过早的提交会产生一轮正常对话。修改前观察到:leg 以非零码退出且没有任何失败测试行,过早的提交被丢弃。

证据(修改前与修改后)

修改前 —— 在 OpenTUI 渲染器下运行完整 leg,使用 CI 自己的命令与环境:

Test Files  4 failed | 6 passed | 1 skipped (11)
     Tests  4 failed | 15 passed | 2 skipped (21)
  Duration  460.55s        exit 1

从失败运行中抓取的终端屏幕,位置正是第一个 prompt 本应开启一轮对话的地方:

Type your message or @path/to/file> Start the review.✖︎ Chat not initialized
midTurn: false
All tool calls found: []

对照实验,同一台机器、同样四个文件、ink 渲染器:4 passed (4)7 passed (7)、34.81 秒、exit 0。

修改后 —— 在 OpenTUI 渲染器下运行完整 leg,命令与环境相同:

Test Files  10 passed | 1 skipped (11)
     Tests  19 passed | 2 skipped (21)
  Duration  182.83s        exit 0

单元见证与变异探针:移除防护 → 新用例以 Error: Chat not initialized 失败(1 failed | 44 passed);恢复防护 → 45 通过。

测试平台

OS Status
🍏 macOS ⚠️ 未测试
🪟 Windows ⚠️ 未测试
🐧 Linux ✅ 已测试

环境(可选)

Linux runner。bundle 来自完整构建,在与 CI leg 相同版本的 bun 下驱动;bun 安装在仓库之外的临时前缀目录中,因此工作区与 lockfile 未被触碰。交互式 spec 运行在仓库自带的脚本化模型服务器上;唯一使用真实模型的 spec 运行在已配置的端点上。

风险与范围

  • 主要风险或取舍:这个等待是有界的轮询而不是事件驱动,因为会话在途初始化的 promise 没有在 core 包之外暴露,而本次修改刻意不触碰 core。所使用的时长与该渲染器在别处针对同一时间窗已经使用的时长一致;一个始终无法完成初始化的会话仍然会报出自己的错误,而不是把 prompt 挂住。
  • 该等待不查询 abort 信号,同样是为了与已有的同类自愈机制保持一致。最坏情况是:在启动时间窗内发出的中断最多延迟到该上界才被响应,之后仍会由发送路径正常处理。
  • "直接启动会话而不是等待"这个方案被否决了:客户端自己的 initialize 只对"会话已经建好"做了保护,所以在时间窗内的第二个调用方会再建一个会话。
  • 未验证 / 超出范围:macOS 与 Windows,该 leg 不在这些平台运行。使用真实模型的压缩 spec 仍有其自身的模型延迟风险,在本次修改前后都需要 vitest 重试;那是另一个已存在的类别。OpenTUI 的其他代码不会在这个时间窗内读取模型会话 —— 它的历史读取来自渲染器自己的 transcript 存储 —— 因此对话轮次路径是唯一暴露的位置,也没有添加更大范围的门禁。
  • 破坏性变更 / 迁移说明:无。当会话已经就绪时的行为与之前的路径完全一致;被修改的 spec 文件中已有的 44 个用例保持不变且全部通过。

关联 Issue

Fixes #11042

该 issue 是按 commit 创建的,并点名了 39a84c9e1d 的 rewind 分类器重构。那个 commit 并不是原因:同一个 leg 在它的一个祖先 commit 上就失败过,在一个包含相同代码的后代 commit 上又通过了,并且有一个无关的 SHA 既失败又成功。本 PR 关闭的缺陷才是让这些运行变红的原因。

)

The `E2E Interactive - OpenTUI renderer (bun)` leg goes red on main with no
commit to blame. It failed at 0dd5bf2 and at 39a84c9 — the rewind
classifier refactor this issue names — and passed at 74fe3a6, which carries
that same refactor; 56f75ad failed run 33834473606 and passed run
33836390526 on an identical SHA. The job uploads no artifact and its log is
auth-gated, so the failing test is not readable from the run.

Reproduced locally instead, under CI's own bun pin and its exact command: four
interactive specs fail all three vitest attempts (exit 1, 460s) while the same
four pass under ink on the same machine in 34.8s. The rendered screen names the
cause — `Chat not initialized` sitting where the first prompt should have
opened a turn — and the scripted model server records zero requests, so the
prompt never became a turn at all.

OpenTUI mounts its composer and, from that mount effect, loads the command
registry, which calls `config.initialize()`. `Config.initialize()` sets its own
`initialized` guard before the work runs and reaches
`llmClient.initialize()` -> `startChat()` only near the end, so a prompt
submitted inside that window makes the turn's own `initialize()` throw
"already initialized" — which the turn's catch reads as "already done" and
proceeds. The client has no chat yet, the send dies in `getChat()`, and the
prompt is dropped. ink cannot reach this state: #11000 gates `isInputActive` on
`isConfigInitialized`, so its composer accepts nothing until initialization
completes, and OpenTUI has no equivalent gate.

`OpenTuiSlashDispatcher.ensureCommandsLoaded` already self-heals the same
window for the registry with a bounded poll; the turn path never got one. Wait
for the chat the in-flight initialization creates, on the same budget, so a
config that never finishes still reports its own error instead of hanging the
prompt. Starting the chat directly is not the fix: `LlmClient.initialize()`
guards only on an already-built chat, so a second caller inside the window
would build a second one.

Witness: a turn whose `initialize()` throws "already initialized" against a
client that reports no chat and throws from the send until one appears. With
the wait removed the case fails `Error: Chat not initialized`; restored, the
send runs once and the file's 45 cases pass.

Measured: the leg now exits 0 with 10 files passed and 1 skipped (19 tests),
against 4 failed files before.
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator Author

E2E report — issue #11042 (Main CI failed: E2E Tests on 39a84c9e1db4)

What the issue reported

Run 33901880790 on main at 39a84c9e1d failed one job: E2E Interactive - OpenTUI renderer (bun), step Run interactive E2E tests (OpenTUI). The other 11 jobs in the run were green or skipped.

The named commit is not the cause

The issue is filed per commit, which points at the rewind classifier refactor. The run metadata rules that out:

Commit Run OpenTUI leg Note
56f75adf29 33834473606 (03:47) failure same SHA as the next row
56f75adf29 33836390526 (04:19) success identical code, opposite outcome
0dd5bf2876 33899304179 failure an ancestor of the issue's commit
39a84c9e1d 33901880790 failure the issue's commit
74fe3a659d 33906547642 success a descendant carrying the same refactor

The leg failed before the refactor landed and passed after it, and one SHA both failed and passed. This is an intermittent defect in the leg itself, not a regression from 39a84c9e1d.

The job uploads no artifact and GET /actions/jobs/<id>/logs returns 403 without credentials, so the failing test name is not readable from the run. The annotation is only Process completed with exit code 1.

Local reproduction

The leg was reproduced exactly: bun installed at CI's pinned 1.3.14 into a temp prefix outside the repository (working tree untouched), the bundle from npm run build && npm run bundle, and CI's own command and environment.

Before the fixQWEN_E2E_RENDERER=opentui KEEP_OUTPUT=true VERBOSE=true QWEN_SANDBOX=false:

Test Files  4 failed | 6 passed | 1 skipped (11)
     Tests  4 failed | 15 passed | 2 skipped (21)
  Duration  460.55s        exit 1

All four failed on all three vitest attempts, so this was not a coin flip locally:

  • file-system-interactiveExpected to find a read_file tool call
  • mid-turn-submit-interactiveHeld response never reached the screen
  • protocol-tags-interactiveExpected visible summary marker after HTTP retries
  • submitted-prompt-provenanceFake model turn did not complete

Differential under ink, same machine, same four files, minutes apart: 4 passed (4), 7 passed (7), 34.81s, exit 0. The defect is OpenTUI-only, which is why only this leg goes red.

The captured PTY screen names the cause. Where the first prompt should have opened a turn:

Type your message or @path/to/file> Start the review.✖︎ Chat not initialized
midTurn: false
All tool calls found: []

The scripted model server recorded zero requests — the prompt never became a turn at all.

Root cause

  1. OpenTUI mounts its composer, and that mount effect calls loadInteractiveCommands(), which calls config.initialize() (ui/opentui/slash-dispatch.ts).
  2. Config.initialize() sets its own initialized guard synchronously, before the work runs (core/src/config/config.ts:3008), and only reaches llmClient.initialize()startChat() near the end of initializeOnce() (config.ts:3496). this.chat is assigned inside startChat() (core/src/core/client.ts:2261).
  3. A prompt submitted inside that window makes the turn's own await config.initialize() in livePromptEvents throw 'Config was already initialized'. Its catch {} reads that as "already done" and proceeds.
  4. The send then reaches GeminiClient.getChat(), which throws 'Chat not initialized' (client.ts:652-655). The turn dies and the user's prompt is dropped.

ink cannot reach this state: #11000 gates isInputActive on isConfigInitialized, so its composer accepts nothing until initialization completes. OpenTUI has no equivalent gate — isConfigInitialized does not appear anywhere under ui/opentui/.

OpenTuiSlashDispatcher.ensureCommandsLoaded() already documents and self-heals this exact window for the command registry with a bounded 15 s / 100 ms poll. The turn path never got one.

Fix

livePromptEvents now waits for the chat the in-flight initialization creates, bounded by the same budget the registry self-heal uses, so a config that never finishes still reports its own error instead of hanging the prompt. When the chat already exists the loop exits on its first check with no delay — all 44 pre-existing cases in the file are unchanged and green.

Starting the chat directly is not the fix: LlmClient.initialize() guards only on an already-built chat (client.ts:529), so a second caller inside the window would build a second one. Waiting is also why the change stays out of packages/coreConfig.initialize()'s throw-on-reentry contract is unchanged and no core file is touched.

Two files, 45 insertions, 1 deletion. No CI, workflow, or configuration change.

Verification

  • npm run build — passed (exit 0)
  • npm run bundle — passed (exit 0); the wait is present in dist/chunks/start-opentui-ui-*.js
  • npm run typecheck — passed (exit 0), re-run after the final test edit — passed (exit 0)
  • npm run lint — first run failed: require-yield on the new witness's generator fake. Fixed by yielding the stream's finished event; full re-run passed (exit 0)
  • cd packages/cli && vitest run src/ui/opentui/live-session.test.ts45 passed (1 file), including the new witness at 304 ms
  • Mutation probe — with the wait removed, the new case fails Error: Chat not initialized (1 failed | 44 passed); restored, 45 passed. The guard has a real witness.
  • Integration, the four failing files under OpenTUI after the fix — exit 0, 4 passed (4), 7 passed (7), 57.71s (was exit 1, 460.55s)
  • Integration, the full OpenTUI leg with CI's exact command and env after the fix — exit 0, Test Files 10 passed | 1 skipped (11), Tests 19 passed | 2 skipped (21), 182.83s (was 4 failed | 6 passed | 1 skipped, exit 1)
  • npm run generate:settings-schema — not applicable; no settings source changed

Environment note: every check above ran on this Linux runner. The leg itself runs only on ubuntu-latest, so no CI-only or Docker-only check was unavailable. bun was installed into /tmp at CI's pinned version rather than into the repository, so the commit contains no toolchain or lockfile change.

Risk and scope

  • The wait does not consult the abort signal, matching the sibling registry self-heal. Worst case, an interrupt issued inside the startup window is honoured up to 15 s late; the interrupt is then applied by the send path as usual.
  • Not validated: macOS and Windows (the leg does not run there). The live-model spec context-compress-interactive keeps its own model-latency exposure, unrelated to this fix — it needed vitest retries both before and after.
  • Other OpenTUI code does not reach getChat() during this window: its getHistory() calls read the renderer's own transcript store, not the model chat. The turn path was the only exposed site.
中文说明

E2E 报告 — issue #11042Main CI failed: E2E Tests on 39a84c9e1db4

问题描述的内容

main 分支上 39a84c9e1d33901880790 这次运行只有一个 job 失败:E2E Interactive - OpenTUI renderer (bun),失败步骤为 Run interactive E2E tests (OpenTUI)。该次运行的其余 11 个 job 均为绿色或被跳过。

被点名的 commit 并不是原因

该 issue 是按 commit 自动创建的,因此指向了 rewind 分类器重构。运行的元数据可以排除这一猜测:

Commit 运行 OpenTUI leg 说明
56f75adf29 33834473606(03:47) 失败 与下一行是同一个 SHA
56f75adf29 33836390526(04:19) 成功 代码完全相同,结果相反
0dd5bf2876 33899304179 失败 是本 issue commit 的祖先
39a84c9e1d 33901880790 失败 本 issue 的 commit
74fe3a659d 33906547642 成功 后代,包含同一份重构

这个 leg 在重构合入之前就失败过,在重构合入之后又通过了,并且同一个 SHA 既失败又成功。因此这是该 leg 自身的间歇性缺陷,而不是 39a84c9e1d 引入的回归。

该 job 不上传任何 artifact,并且 GET /actions/jobs/<id>/logs 在没有凭证时返回 403,所以无法从运行中读到失败的测试名。注解信息只有 Process completed with exit code 1.

本地复现

该 leg 被完整复现:bun 按 CI 锁定的 1.3.14 安装到仓库之外的临时目录(工作区未被改动),bundle 来自 npm run build && npm run bundle,并使用 CI 自己的命令与环境变量。

修复前 —— QWEN_E2E_RENDERER=opentui KEEP_OUTPUT=true VERBOSE=true QWEN_SANDBOX=false

Test Files  4 failed | 6 passed | 1 skipped (11)
     Tests  4 failed | 15 passed | 2 skipped (21)
  Duration  460.55s        exit 1

四个文件在 vitest 的三次尝试中全部失败,所以在本地这并不是概率问题:

  • file-system-interactive —— Expected to find a read_file tool call
  • mid-turn-submit-interactive —— Held response never reached the screen
  • protocol-tags-interactive —— Expected visible summary marker after HTTP retries
  • submitted-prompt-provenance —— Fake model turn did not complete

ink 下的对照实验:同一台机器、同样的四个文件、相隔几分钟:4 passed (4)7 passed (7)、34.81 秒、exit 0。缺陷只存在于 OpenTUI,这也正是只有这个 leg 变红的原因。

抓取到的 PTY 屏幕直接指出了原因。在第一个 prompt 本应开启一轮对话的位置:

Type your message or @path/to/file> Start the review.✖︎ Chat not initialized
midTurn: false
All tool calls found: []

脚本化的假模型服务器记录到的请求数为 —— 这个 prompt 根本没有成为一次对话轮次。

根因

  1. OpenTUI 挂载它的输入框(composer),而该挂载 effect 会调用 loadInteractiveCommands(),其中调用了 config.initialize()ui/opentui/slash-dispatch.ts)。
  2. Config.initialize() 会在真正的工作开始之前同步地设置自己的 initialized 标志(core/src/config/config.ts:3008),而直到 initializeOnce() 接近结尾时才执行 llmClient.initialize()startChat()config.ts:3496)。this.chat 是在 startChat() 内部才被赋值的(core/src/core/client.ts:2261)。
  3. 在这个时间窗内提交的 prompt,会使 livePromptEvents 中自己的 await config.initialize() 抛出 'Config was already initialized'。它的 catch {} 把这理解为"已经初始化完成"并继续往下执行。
  4. 随后发送流程走到 GeminiClient.getChat(),抛出 'Chat not initialized'client.ts:652-655)。这一轮对话终止,用户的 prompt 被丢弃。

ink 不会进入这个状态:#11000isInputActive 依赖 isConfigInitialized,因此在初始化完成之前它的输入框不接受任何输入。OpenTUI 没有等价的门禁 —— ui/opentui/ 目录下完全没有出现 isConfigInitialized

OpenTuiSlashDispatcher.ensureCommandsLoaded() 已经记录并用一个有界的 15 秒 / 100 毫秒轮询自愈了完全相同的时间窗(针对命令注册表)。而对话轮次这条路径一直没有对应的处理。

修复

livePromptEvents 现在会等待在途初始化创建出 chat,使用与注册表自愈相同的有界时长,因此一个始终无法完成初始化的 config 仍然会报出自己的错误,而不是把 prompt 挂住。当 chat 已经存在时,循环在第一次判断就退出,没有任何延迟 —— 该文件中已有的 44 个用例保持不变且全部通过。

直接去启动 chat 不是正确的修法:LlmClient.initialize() 只对"chat 已经建好"做了保护(client.ts:529),所以在时间窗内的第二个调用方会再建一个 chat。也正因为选择"等待",这个改动没有进入 packages/core —— Config.initialize() 的重入即抛错契约保持不变,没有触碰任何 core 文件。

两个文件,45 行新增,1 行删除。没有任何 CI、workflow 或配置改动。

验证

  • npm run build —— 通过(exit 0)
  • npm run bundle —— 通过(exit 0);新增的等待逻辑存在于 dist/chunks/start-opentui-ui-*.js
  • npm run typecheck —— 通过(exit 0);在最后一次测试文件修改后重跑 —— 通过(exit 0)
  • npm run lint —— 第一次运行失败:新增见证测试的 generator 假对象触发 require-yield。通过让它 yield 流的 finished 事件修复;完整重跑通过(exit 0)
  • cd packages/cli && vitest run src/ui/opentui/live-session.test.ts —— 45 通过(1 个文件),其中新增见证用例耗时 304 毫秒
  • 变异探针(mutation probe) —— 移除该等待后,新用例以 Error: Chat not initialized 失败(1 failed | 44 passed);恢复后 45 通过。该防护有真实的见证测试。
  • 集成测试,修复后在 OpenTUI 下运行原先失败的四个文件 —— exit 04 passed (4)7 passed (7)、57.71 秒(此前为 exit 1、460.55 秒)
  • 集成测试,修复后使用 CI 完全一致的命令与环境运行完整的 OpenTUI leg —— exit 0Test Files 10 passed | 1 skipped (11)Tests 19 passed | 2 skipped (21)、182.83 秒(此前为 4 failed | 6 passed | 1 skipped、exit 1)
  • npm run generate:settings-schema —— 不适用;没有修改任何 settings 源文件

环境说明:以上所有检查都在这台 Linux runner 上运行。该 leg 本身只在 ubuntu-latest 上运行,因此不存在无法执行的 CI 专用或 Docker 专用检查。bun 是按 CI 锁定的版本安装到 /tmp 而不是仓库中,所以提交里不含任何工具链或 lockfile 改动。

风险与范围

  • 该等待不会查询 abort 信号,这与同类的注册表自愈保持一致。最坏情况是:在启动时间窗内发出的中断最多延迟 15 秒被响应;之后中断仍会由发送路径正常处理。
  • 未验证:macOS 与 Windows(该 leg 不在这些平台运行)。使用真实模型的 context-compress-interactive 仍有其自身的模型延迟风险,与本次修复无关 —— 它在修复前后都需要 vitest 重试。
  • OpenTUI 的其他代码不会在这个时间窗内访问 getChat():它的 getHistory() 读取的是渲染器自己的 transcript 存储,而不是模型 chat。对话轮次路径是唯一暴露的位置。

🧠 Handled by Qwen Code · model/模型 qwen3.8-max-2026-09-02

@github-actions github-actions Bot added the review/self-reported The linked issue was opened by the PR author (self-reported) label Sep 4, 2026
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Deferred approval not posted — the PR head moved (or the PR closed) after the review of ca59b73; approving now would attest to unreviewed code. Re-run @qwen-code /triage on the new head. finalize run

⚠️ 延迟审批未提交 —— 审查 ca59b73 之后 PR head 已变更(或 PR 已关闭),此时审批会为未审查的代码背书。请在新 head 上重新运行 @qwen-code /triage查看 finalize 运行

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR — this one is unusually well argued, and the argument holds up against the code.

Template ✓ — all required sections present, including Before/After evidence and the Tested-on matrix.

Problem: observed, not theoretical. I went and checked the causal chain rather than taking the description's word for it, and every link is real:

  • Config.initialize() sets this.initialized = true at config.ts:3008 and only then awaits initializeOnce() at config.ts:3012. So a second caller inside that window really does get Config was already initialized while startChat() has not run.
  • livePromptEvents swallows exactly that throw — the bare try { await config.initialize() } catch {} at the top of the function, whose comment reads /* already initialized by command loading / startup */ — and proceeds straight to getGeminiClient().
  • getChat() throws Chat not initialized when this.chat is undefined (client.ts:652-657), and isInitialized() at client.ts:659 is exactly this.chat !== undefined — so the predicate this PR polls is precisely "the chat the send needs exists". Nothing between the catch and the send loop would stop the throw.
  • The reason only this renderer sees it is confirmed too: ink's isInputActiveForState() requires isConfigInitialized (AppContainer.tsx:330-350), and shouldDrainMessageQueue gates on it as well, so an early submit cannot happen there. input-prompt.tsx has no equivalent gate.

The in-tree precedent is the clincher — commands-dispatch.ts:342-352 already documents this identical window in its own words ("the second initialize() call throws 'already initialized', the catch proceeds") and self-heals it with the same bounded poll at the same 15s/100ms budget (commands-dispatch.ts:193-194).

Direction: aligned. CHANGELOG shows this is an actively maintained line of work, not a drive-by: fix(ci): Take the OpenTUI interactive e2e leg out of CI (#10829), fix(cli): close OpenTUI submit-path gaps, restore its E2E leg (#10831), fix(cli): Align OpenTUI mid-turn submit and exit with ink (#10883). A dropped first prompt under the OpenTUI renderer sits squarely in that sequence.

Size: not applicable — packages/cli/src/ui/opentui/ is not a core path and the change is single-package. 17 production lines, 29 test lines.

Approach: the scope feels right, and I want to be explicit that I looked for a cheaper path and didn't find one that stays outside core. My own first instinct was to gate the composer the way ink does, which removes the window instead of waiting inside it — but that means plumbing readiness state plus a queue for the held submit, a materially bigger change than this bug warrants. The other obvious move, awaiting the in-flight initialization promise, is genuinely unavailable: initializationPromise is private on Config with no accessor, read only by shutdownResources. Exposing it means editing core. So the bounded poll reusing the sibling's exact budget is the right call here. Two non-blocking notes follow in the code review.

Risk: no elevated risk signals. Neither changed file matches the revert-correlated path set. Linked issue #11042 is open, so no duplicate-fix concern.

Moving on to code review. 🔍

中文说明

感谢贡献!这个 PR 的论证相当扎实,而且对照代码之后完全站得住。

模板 ✓ —— 各必需章节齐全,包含 Before/After 证据与测试平台表格。

问题: 是已观测到的缺陷,不是理论性加固。我没有只采信描述,而是逐环核对了因果链,每一环都真实存在:

  • Config.initialize()config.ts:3008 就把 this.initialized = true 置位,直到 config.ts:3012await initializeOnce()。所以在这个时间窗内的第二个调用方确实会拿到 Config was already initialized,而此时 startChat() 还没跑。
  • livePromptEvents 恰好吞掉了这个异常 —— 函数开头那个空的 try { await config.initialize() } catch {},注释写的是 /* already initialized by command loading / startup */ —— 然后直接走到 getGeminiClient()
  • this.chat 为 undefined 时 getChat() 抛出 Chat not initializedclient.ts:652-657);而 client.ts:659isInitialized() 就是 this.chat !== undefined —— 也就是说本 PR 轮询的判据正好就是"发送所需要的那个 chat 已经存在"。从 catch 到发送循环之间没有任何东西能拦住这个异常。
  • "只有这个渲染器会遇到"这一点也得到了印证:ink 的 isInputActiveForState() 要求 isConfigInitializedAppContainer.tsx:330-350),shouldDrainMessageQueue 同样以此为门禁,所以在 ink 下过早提交根本不可能发生;input-prompt.tsx 没有对应的门禁。

最有说服力的是仓库内已有的先例 —— commands-dispatch.ts:342-352 的注释已经用自己的话描述了这个完全相同的时间窗("第二次 initialize() 调用抛出 already initialized,catch 之后继续执行"),并用同样的有界轮询、同样的 15s/100ms 时长自愈(commands-dispatch.ts:193-194)。

方向: 对齐。CHANGELOG 显示这是一条正在持续维护的工作线,而不是顺手改动:fix(ci): Take the OpenTUI interactive e2e leg out of CI#10829)、fix(cli): close OpenTUI submit-path gaps, restore its E2E leg#10831)、fix(cli): Align OpenTUI mid-turn submit and exit with ink#10883)。OpenTUI 渲染器下丢失第一个 prompt,正处在这个序列之中。

规模: 不适用 —— packages/cli/src/ui/opentui/ 不属于核心路径,且改动只涉及单个 package。生产代码 17 行,测试 29 行。

方案: 范围合理。我想明确说明:我找过更省的路子,但在"不碰 core"的前提下没有找到。我自己的第一反应是像 ink 那样给输入框加门禁 —— 那是消除时间窗而不是在窗口里等待,但那需要把就绪状态和"被扣住的提交的队列"一起打通,相对这个 bug 而言改动明显更大。另一个看似显然的做法是等待在途的初始化 promise,而它确实不可用:initializationPromiseConfig 上是 private,没有任何访问器,只被 shutdownResources 读取。暴露它就得改 core。所以沿用同类机制完全相同的时长上限做有界轮询,在这里是正确选择。另有两点非阻塞意见,见代码审查。

风险: 无升级风险信号。两个改动文件都不匹配与 revert 相关的路径集合。关联 issue #11042 仍处于 open 状态,因此不存在重复修复的问题。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at ca59b73884382ef1af44c080e487e1a760a62069 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Code review

No critical blockers and no AGENTS.md violations. I read the surrounding code rather than just the hunk, and the change fits it: client.isInitialized() is exactly the right predicate (this.chat !== undefined, the same field getChat() guards on), the constants sit next to the function that uses them, and the comment explains the non-obvious why — that initialize() flips its guard before the work runs — which is the kind of comment AGENTS.md actually wants.

The test really does pin the guard, but not through its assertion. Worth spelling out, because on the surface it looks like it doesn't. expect(sendMessageStream).toHaveBeenCalledTimes(1) would pass with the wait deleted: the mock is invoked to build the generator before any throw, and a vi.fn(function* () {...}) body only runs on the first next(). What actually fails the spec is that for await (const ev of stream) has no enclosing try/catch anywhere in livePromptEvents, so Chat not initialized propagates out through drain() and rejects the test — which matches the mutation probe in the description. The fragility is that the witness is an uncaught throw rather than an assertion: if the send loop ever grows a catch that folds errors into error events, this spec silently stops testing the wait and still goes green. Asserting the readiness value observed at send time (or that no error event was yielded) would make it robust. Non-blocking.

The wait does not consult the abort signal — disclosed under Risk & Scope, and I agree it is not a blocker, but it is free to fix. signal is already in scope as the parameter at that point; abort is only derived after the loop. So adding !signal?.aborted to the loop condition is a one-condition change that lets an Esc issued inside the startup window take effect immediately instead of up to the bound late. The sibling's lack of a signal is not a reason to match it here — ensureCommandsLoaded() has no signal parameter at all, whereas this function is handed exactly the thing it needs.

I ran the reuse ladder and am deliberately not flagging duplication. There are now two bounded polls over the same startup window in this directory with identical constants (commands-dispatch.ts:193-194 and this one). Extracting a shared helper would be premature under AGENTS.md: they wait on different predicates (getSkillManager?.() vs isInitialized()), take different recovery actions (reload a registry vs proceed with the send), and one is a private method on a dispatcher class. Two sites with different semantics is below the threshold where a helper pays for itself. There is also nothing existing to reuse — Config exposes no readiness accessor, which is precisely why this has to be a poll.

Scope claim checks out. "The turn path was the only exposed site" holds as far as I can trace it: the only other unguarded getGeminiClient() in the renderer is commands-dispatch.ts:776 (optional-chained, and its window is already covered by that file's own self-heal), and session-switch.ts calls getGeminiClient()?.initialize?.() on an already-built session, which is a different lifecycle. slash-dispatch.ts:59 has the same swallow-and-proceed shape but reads the skill manager, not the chat. So no wider gate was needed, and adding one would have been scope creep.

The whole PR hinges on an ordering window that a 17-line diff does not show, so here it is:

sequenceDiagram
    participant P1 as OpenTUI composer
    participant P2 as Command registry load
    participant P3 as livePromptEvents turn
    participant P4 as GeminiClient chat
    P1->>P2: mount kicks off the initialize flight
    Note over P2: Config.initialized flips true at once, startChat still running
    P1->>P3: early submit lands inside that window
    P3->>P3: own initialize throws already initialized, catch proceeds
    P3->>P4: isInitialized
    P4-->>P3: false, so poll every 100ms up to a 15s bound
    P4-->>P3: true once startChat has built the chat
    P3->>P4: sendMessageStream
    P4-->>P3: turn streams normally instead of being dropped
Loading

Without the new loop, the last four lines collapse into a single getChat() throw and the prompt is lost — which is the ✖︎ Chat not initialized screen in the description.

Testing

What this section carries: the PR's own CI results, read through the API for the reviewed commit. I did not build or execute any PR-derived code — the review is static, per the gate's rules.

No check is red. Two pull_request workflow runs are still in flight and the legs most relevant to this change are among them, so nothing below is settled yet. Test (macos-latest) and Test (windows-latest) are skipped, consistent with the Tested-on matrix and with the leg only running on Linux.

Final CI results for ca59b73 (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Test (ubuntu-latest, Node 22.x) 🚫 cancelled
web-shell E2E Smoke (ubuntu-latest, Node 22.x) 🚫 cancelled
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
OpenTUI no-flicker gate ✅ success
Secret scan (TruffleHog) ✅ success
TUI parity snapshots (ink vs opentui) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

The important gap: the leg that reproduces this bug does not run on PR CI at all. E2E Interactive - OpenTUI renderer (bun) is defined at e2e.yml:459-460, but that workflow's on: block is push (to main and feat/e2e/**), schedule, and workflow_dispatch — there is no pull_request trigger, and the file says why in its own header: E2E is slow and flaky, so it runs post-merge plus nightly rather than gating the queue. The job-level if: mentioning pull_request is a dormant fork guard, not an activation.

I checked whether some other PR-CI job picks up the slack, and none does:

  • Integration Tests (no-AK, No Sandbox) (ci.yml:1749) runs npm run test:integration:no-ak:sandbox:none, which is a fixed whitelist of 21 files — none of them under interactive/.
  • QWEN_E2E_RENDERER appears zero times in ci.yml, so no PR-triggered job ever selects the OpenTUI renderer.
  • The repo already ships the exact script this would need — test:integration:interactive:opentui:sandbox:none (package.json:67) — and no PR workflow calls it.

Two consequences worth stating plainly:

  • It corroborates the description's account of why the auto-filed issues name an arbitrary commit. A post-merge-only leg fails against whatever SHA happens to sit at the head of the run, which is exactly the "one SHA both failing and passing thirty minutes apart" symptom. The attribution problem is structural, not confusion on the author's part.
  • It means a fully green run on this PR proves nothing about the fix. The new unit spec in live-session.test.ts does run under Test (ubuntu-latest, Node 22.x), so the guard's unit witness is covered — but the end-to-end behaviour the guard exists for is only exercised after merge.

Sandboxed verification would settle this, and it is the only way to settle it before merge: @qwen-code /verify — that the new wait is load-bearing rather than incidental, i.e. that an OpenTUI turn submitted while startChat() is in flight produces a real request where the base build fails with Chat not initialized; and @qwen-code /tmux — the renderer surface itself, typing a prompt in the first moments of a session and watching it become a normal turn. The author has write access, so both lanes are available directly and neither needs sponsoring.

Not verified: the interactive-leg numbers, the terminal capture, and the mutation probe in the description. Those are the author's own results, run locally on Linux — not evidence I reproduced, and per the above no CI run on this PR can reproduce them. Real-scenario tmux testing is N/A here: this is an unattended CI run, where the gate never drives the product.

中文说明

代码审查

没有阻塞性问题,也没有违反 AGENTS.md 的地方。我读了改动周围的代码而不只是那个 hunk,这个改动是贴合上下文的:client.isInitialized() 正是合适的判据(this.chat !== undefined,与 getChat() 所守护的是同一个字段),常量紧挨着使用它的函数,注释解释的是那个不显然的原因 —— initialize() 会在真正的工作跑完之前就翻转自己的门禁 —— 这正是 AGENTS.md 希望看到的那类注释。

这个测试确实钉住了防护,但不是靠它的断言。 这一点值得说清楚,因为表面上看不像。expect(sendMessageStream).toHaveBeenCalledTimes(1) 在删掉等待之后依然会通过:mock 是在任何异常抛出之前被调用以构建 generator 的,而 vi.fn(function* () {...}) 的函数体只在第一次 next() 时才执行。真正让用例失败的是:livePromptEvents 里的 for await (const ev of stream) 外层没有任何 try/catch,所以 Chat not initialized 会一路穿出 drain() 让测试 reject —— 这与描述里的变异探针结果一致。脆弱之处在于:这个见证是一个未被捕获的异常而不是一个断言。如果将来发送循环加上了把错误折叠成 error 事件的 catch,这个用例就会在悄悄不再检验该等待的同时依然变绿。断言"发送时刻观测到的就绪值"(或断言没有产出 error 事件)会让它更稳固。非阻塞。

该等待没有查询 abort 信号 —— Risk & Scope 里已经披露,我也同意这不是阻塞项,但修它是零成本的。在那个位置上 signal 作为参数已经在作用域内,而 abort 是在循环之后才派生出来的。所以在循环条件里加上 !signal?.aborted 只是一个条件的改动,就能让在启动时间窗内按下的 Esc 立刻生效,而不是最多延迟到上界。同类机制没有处理信号在这里不构成理由 —— ensureCommandsLoaded() 根本没有 signal 参数,而这个函数手里正好拿着它需要的东西。

我走了一遍复用阶梯,并且刻意不把重复算作问题。 现在这个目录里有了两处针对同一启动时间窗、常量完全相同的有界轮询(commands-dispatch.ts:193-194 与本处)。按 AGENTS.md,抽一个共享 helper 属于过早抽象:它们等待的判据不同(getSkillManager?.()isInitialized()),采取的补救动作不同(重载注册表 与 继续发送),而且其中一个是 dispatcher 类的私有方法。语义不同的两处,还没到 helper 能回本的门槛。另外也确实没有现成的东西可复用 —— Config 没有暴露任何就绪访问器,这恰恰是这里只能轮询的原因。

范围声明成立。 就我能追踪到的程度,"对话轮次路径是唯一暴露的位置"是对的:该渲染器里另一处未加保护的 getGeminiClient()commands-dispatch.ts:776(用了可选链,且它的时间窗已由该文件自己的自愈机制覆盖),而 session-switch.ts 调用的是已建好会话上的 getGeminiClient()?.initialize?.(),属于另一个生命周期。slash-dispatch.ts:59 有同样的"吞掉异常继续执行"形状,但它读的是 skill manager 而不是 chat。所以不需要更大范围的门禁,加了反而是范围蔓延。

整个 PR 的关键在于一个 17 行 diff 看不出来的时序窗口,所以把它画了出来(图见上,中文不重复)。没有新增的循环时,图中最后四行会塌缩成一次 getChat() 抛异常,prompt 就此丢失 —— 也就是描述里那个 ✖︎ Chat not initialized 屏幕。

测试

本节承载的内容: 该 PR 自己的 CI 结果,通过 API 读取所审查 commit 的数据。我没有构建或执行任何来自 PR 的代码 —— 按门禁规则,本次审查是静态的。

没有检查项是红的。有 2 个 pull_request workflow run 仍在进行中,而与本次改动最相关的几个 leg 正在其中,所以下面的结论都还没有落定。Test (macos-latest)Test (windows-latest) 被跳过,这与测试平台表格以及该 leg 只在 Linux 运行是一致的。(CI 表格见上,中文不重复)

关键的缺口:复现这个 bug 的那个 leg 在 PR CI 上根本不会运行。 E2E Interactive - OpenTUI renderer (bun) 定义在 e2e.yml:459-460,但该 workflow 的 on: 只有 pushmainfeat/e2e/**)、scheduleworkflow_dispatch —— 没有 pull_request 触发器,文件头部的注释也说明了原因:E2E 又慢又不稳定,所以放在合并后加每晚跑,而不是卡在合并队列上。那个 job 级别提到 pull_requestif: 是一个休眠的 fork 保护,不是启用条件。

我查了是否有别的 PR CI job 补上了这一块,结论是没有:

  • Integration Tests (no-AK, No Sandbox)ci.yml:1749)跑的是 npm run test:integration:no-ak:sandbox:none,那是一个固定的 21 个文件白名单 —— 其中没有一个在 interactive/ 下。
  • QWEN_E2E_RENDERERci.yml 中出现 0 次,所以没有任何 PR 触发的 job 会选择 OpenTUI 渲染器。
  • 仓库里其实已经有这件事所需的现成脚本 —— test:integration:interactive:opentui:sandbox:nonepackage.json:67)—— 只是没有 PR workflow 调用它。

有两点值得直说:

  • 印证了描述中关于"自动创建的 issue 会指向任意 commit"的解释。一个只在合并后运行的 leg,失败时对应的就是那次运行头部恰好所在的 SHA,这正是"同一个 SHA 在相隔三十分钟内既失败又成功"这个症状的来源。归因问题是结构性的,不是作者搞混了。
  • 它意味着这个 PR 上全绿的运行并不能证明修复有效live-session.test.ts 里新增的单元用例确实会在 Test (ubuntu-latest, Node 22.x) 下运行,所以该防护的单元见证是有覆盖的 —— 但它所服务的端到端行为只在合并之后才会被执行到。

沙箱验证可以解决这一点,而且是合并前唯一的解决办法:@qwen-code /verify —— 证明新增的等待是承重的而非附带的,即在 startChat() 在途时提交的 OpenTUI 轮次能产生真实请求,而 base 构建会以 Chat not initialized 失败;以及 @qwen-code /tmux —— 渲染器界面本身,在会话开始的头几秒输入 prompt,看着它变成一轮正常对话。作者有 write 权限,所以两条通道都可以直接使用,无需赞助运行。

未验证:描述中的交互式 leg 数字、终端抓取,以及变异探针。那些是作者自己在 Linux 上本地跑出的结果,不是我复现的证据 —— 而且按上面所说,这个 PR 上没有任何 CI 运行能复现它们。真实场景 tmux 测试在此为 N/A:本次是无人值守的 CI 运行,门禁在这种模式下从不驱动产品。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at ca59b73884382ef1af44c080e487e1a760a62069 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — the diagnosis is right and I verified it independently; the two nits are real but neither changes what the code does.

Stepping back: I came into this expecting to push back. It's an autofix-bot PR against a CI complaint, which is the shape that most often turns out to be a plausible-looking patch for a problem nobody actually hit. So I tried to break the premise first — and couldn't. Every link in the chain is in the code: initialize() flips its guard before awaiting the work, livePromptEvents swallows the resulting throw, getChat() then has nothing to return, and ink never reaches that state because isInputActiveForState() gates input on isConfigInitialized. The renderer genuinely has no such gate. This isn't a hypothetical.

Against my own independent proposal, the PR lands in a better place than I expected. I would have gated the composer like ink does, which removes the window instead of waiting inside it — but that means plumbing readiness state and a queue for the held submit, a much bigger change than a dropped first prompt warrants. My second choice, awaiting the in-flight initialization promise, turns out to be genuinely unavailable: initializationPromise is private with no accessor, so reaching it means editing core. The bounded poll is the smallest fix that stays on the right side of that line, and it reuses a budget and shape already established two files over for the same window. In six months the comment above the constants will tell whoever touches this exactly why the wait exists, which is more than most startup-ordering code manages.

The diff is minimal — no drive-by refactors, no formatting churn, and the test-helper change is the least invasive way to add the new case while keeping the existing 44 on their old path via a () => true default.

My one real reservation is evidentiary rather than technical, and I want to be clear it isn't a reason to hold this PR: the behavioural proof is entirely the author's, run locally, and nothing CI runs on a PR can reproduce it — the leg that would reproduce it is post-merge only. That is a property of how this repo wires E2E, not a defect in this change, and the author could not have fixed it here without widening scope considerably. Deferring over it would punish the wrong party. So the honest position is: approve, and name the lanes so a maintainer who wants proof before merge can get it in one comment.

Two follow-ups I'd want tracked rather than fixed here, both non-blocking:

  • The wait ignores the abort signal even though signal is in scope at that point. Cheap to make responsive.
  • The new spec's witness is an uncaught throw, not an assertion, so it would silently stop pinning the guard if the send loop ever starts catching. Worth hardening.

Neither is a reason to widen this diff. Per the repo's own guidance about review rounds ballooning a PR, both are better as a follow-up than as another pass here.

Approval is deferred until CI lands green on ca59b73884382ef1af44c080e487e1a760a62069 — PR CI is still in flight, including the unit leg that carries the new spec. I'm not approving against a result that doesn't exist yet.

中文说明

信心度:4/5 —— 诊断是正确的,而且我独立核对过;两点小意见真实存在,但都不改变代码的行为。

退一步看:我一开始是准备反驳的。这是一个 autofix bot 针对 CI 抱怨提的 PR,而这种形状最容易变成"看起来合理、实际在修一个没人真遇到过的问题"的补丁。所以我先去攻击它的前提 —— 结果没攻破。因果链的每一环都在代码里:initialize() 在 await 真正的工作之前就翻转了自己的门禁,livePromptEvents 吞掉了由此产生的异常,接着 getChat() 无物可返回,而 ink 永远到不了这个状态,因为 isInputActiveForState()isConfigInitialized 把输入挡住了。这个渲染器确实没有对应的门禁。这不是一个假想场景。

对照我自己的独立方案,这个 PR 的落点比我预期的更好。我本来会像 ink 那样给输入框加门禁 —— 那是消除时间窗而不是在窗口里等待,但那需要打通就绪状态和一个用来扣住提交的队列,相对"丢失第一个 prompt"这个问题而言改动大得多。我的第二选择是等待在途的初始化 promise,结果它确实拿不到:initializationPromise 是私有的、没有访问器,所以要碰它就得改 core。这个有界轮询是"不越过那条线"前提下最小的修法,而且复用了两个文件之外、针对同一时间窗已经确立的时长与形状。六个月之后,常量上方那段注释会准确告诉动这块代码的人这个等待为什么存在 —— 这比大多数启动时序代码做得都好。

diff 是最小的 —— 没有顺手重构,没有格式化噪音,而测试 helper 的改动是"新增用例、同时通过 () => true 默认值让已有的 44 个用例留在原路径上"这种侵入性最低的做法。

我唯一真正的保留是证据层面的,而不是技术层面的,而且我想说清楚它不构成压住这个 PR 的理由:行为证据完全来自作者本地运行,而 PR CI 上跑的任何东西都无法复现它 —— 能复现它的那个 leg 只在合并后运行。这是本仓库 E2E 接线方式的属性,不是这个改动的缺陷,作者在这里也无法在不大幅扩张范围的前提下修好它。为此而 defer 会惩罚错的一方。所以诚实的立场是:批准,并把通道点名出来,让想在合并前拿到证据的维护者用一条评论就能拿到。

两点我希望被记录下来、而不是在这里修掉的后续项,都是非阻塞的:

  • 该等待忽略了 abort 信号,尽管 signal 在那个位置就在作用域内。让它可响应很便宜。
  • 新用例的见证是一个未捕获的异常而不是一个断言,所以一旦发送循环将来开始 catch,它就会悄悄不再钉住这个防护。值得加固。

两者都不构成扩大这个 diff 的理由。按仓库自己关于"review 轮次让 PR 膨胀"的提醒,这两点作为后续处理比在这里再走一轮更好。

批准推迟到 CI 在 ca59b73884382ef1af44c080e487e1a760a62069 上全绿之后 —— PR CI 仍在进行中,其中包含承载新用例的单元测试 leg。我不会针对一个还不存在的结果去批准。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at ca59b73884382ef1af44c080e487e1a760a62069 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test Plan (not a blocker): Tests 19 passed — this review observed 28503 passed; 15 passed — this review observed 28503 passed; 4 passed — this review observed 28503 passed; 7 passed — this review observed 28503 passed; 44 passed — this review observed 28503 passed; and 1 more.

中文说明

Test Plan(非阻断):Tests 19 passed — this review observed 28503 passed; 15 passed — this review observed 28503 passed; 4 passed — this review observed 28503 passed; 7 passed — this review observed 28503 passed; 44 passed — this review observed 28503 passed; and 1 more。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment on lines +581 to +584
const chatDeadline = Date.now() + STARTUP_CHAT_WAIT_MS;
while (!client.isInitialized() && Date.now() < chatDeadline) {
await new Promise((resolve) => setTimeout(resolve, STARTUP_CHAT_POLL_MS));
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R1-1: [certifies-falsely] This wait gates on chat existence (client.isInitialized() is exactly this.chat !== undefined), not on initialization settlement, and that predicate is wrong in two demonstrated directions. First, startChat() assigns this.chat (client.ts:2261) before awaiting fireSessionStartHook, applySessionStartContext() and setTools() (client.ts:2288-2309), and this 100ms-granularity loop releases within one tick of that assignment: a user with a SessionStart hook slower than ~100ms (hooks run arbitrary commands) who submits the first prompt during the startup window — exactly the window this PR targets — sends a first request whose tools come from chat.generationConfig, populated only by setTools() (llm-chat.ts:5162), so the first prompt reaches the model with zero tool declarations and no session-start context, silently degrading the turn with no error shown. Second, when the boot initialization flight fails before llmClient.initialize() (Config.initialize() flips initialized = true before the work runs, config.ts:3008, and never rolls it back; slash-dispatch.ts:57-60 proceeds with partial commands), isInitialized() is false forever and every prompt burns the full 15s budget before dying with the generic Chat not initialized instead of failing fast — each retry pays another 15s.

Witness:

Probe (a) — real LlmClient.startChat(), modeled 800ms SessionStart hook:
ARM_A_loop_exit    atMs=100 tools="UNDEFINED" hasSessionStartCtx=false
ARM_B_after_flight atMs=801 tools=[{"functionDeclarations":[]}] hasSessionStartCtx=true
Probe (b) — livePromptEvents with a post-failed-init shaped config:
PR:   elapsedMs=15019 sendMessageStreamCalls=1 error=Chat not initialized
BASE: elapsedMs=1     sendMessageStreamCalls=1 error=Chat not initialized

Gate the wait on initialization settlement rather than on chat existence — a predicate that only turns true once startChat() has fully completed (initializedSessionId is assigned at client.ts:605 after the full flight), raced against the same STARTUP_CHAT_WAIT_MS deadline — and fail fast (no 15s wait) when the flight has already settled without creating a chat. Two facts the fix must not violate: the STARTUP_CHAT_WAIT_MS bound must remain for a config that genuinely never settles (live-session.ts:547-550), and isInitialized() is also read by LlmClient.initialize()'s re-entry guard (client.ts:529) and by setTools()'s early return (client.ts:1100), so any predicate change or this.chat reset must keep those readers consistent. Please add a test variant in which chat existence flips early but initialization settles later, asserting sendMessageStream is not called until settlement — plus a failed-flight variant asserting the turn settles promptly with the chat error rather than waiting out the 15s budget — and confirm the first goes red when the settlement gate is removed.

中文说明

[Critical] R1-1: [certifies-falsely] 这个等待以「chat 已存在」为就绪判据(client.isInitialized() 就是 this.chat !== undefined),而不是以「初始化已落定」为判据,而该判据在两个已被证实的方向上都是错的。第一,startChat()this.chat = chat(client.ts:2261)之后仍要 await fireSessionStartHookapplySessionStartContext()setTools()(client.ts:2288-2309),而这个 100ms 粒度的循环在该赋值后的一个 tick 内就会放行:如果用户的 SessionStart hook 耗时超过约 100ms(hook 可以执行任意命令),且用户恰好在启动窗口内 —— 也就是本 PR 针对的窗口 —— 提交了第一个 prompt,那么第一个请求读取的 tools 来自 chat.generationConfig,而它只由 setTools() 填充(llm-chat.ts:5162),于是第一个 prompt 会在没有任何工具声明、也没有 session-start 上下文的情况下发给模型,这一轮对话被静默降级且不会有任何报错。第二,当启动初始化在 llmClient.initialize() 之前失败时(Config.initialize() 在真正工作开始之前就把 initialized = true 置位,config.ts:3008,且失败后从不回滚;slash-dispatch.ts:57-60 会带着不完整的命令继续执行),isInitialized() 永远为 false,每个 prompt 都要白白等满 15 秒预算,最后以笼统的 Chat not initialized 结束,而不是快速失败 —— 每次重试都要再付 15 秒。

证据:

探针 (a) —— 真实 LlmClient.startChat(),模拟 800ms 的 SessionStart hook:
ARM_A_loop_exit    atMs=100 tools="UNDEFINED" hasSessionStartCtx=false
ARM_B_after_flight atMs=801 tools=[{"functionDeclarations":[]}] hasSessionStartCtx=true
探针 (b) —— 用「初始化失败后」形态的 config 驱动 livePromptEvents:
PR:   elapsedMs=15019 sendMessageStreamCalls=1 error=Chat not initialized
BASE: elapsedMs=1     sendMessageStreamCalls=1 error=Chat not initialized

建议把这个等待改为以「初始化落定」为判据,而不是以 chat 是否存在为判据 —— 使用一个只有在 startChat() 完全结束后才为真的谓词(initializedSessionId 在整个流程结束时才被赋值,client.ts:605),并与同一个 STARTUP_CHAT_WAIT_MS 时限竞速;当该初始化流程已经落定但没有创建出 chat 时应立即失败(不再等 15 秒)。修复不得违反两个既有事实:对于始终无法完成初始化的 config,STARTUP_CHAT_WAIT_MS 上界必须保留(live-session.ts:547-550);isInitialized() 同时被 LlmClient.initialize() 的重入门禁(client.ts:529)和 setTools() 的提前返回(client.ts:1100)读取,因此任何谓词变更或对 this.chat 的重置都必须保持这三个读取方的一致。请新增一个测试变体:chat 存在性提前翻转、但初始化稍后才落定,断言在落定之前 sendMessageStream 不会被调用 —— 外加一个「初始化失败」变体,断言该轮对话立即以 chat 错误结束,而不是等满 15 秒预算;并请通过移除落定门禁来确认第一个新用例会变红。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direction 1 (early release) — reproduced, then fixed. Direction 2 (fail fast) — verified real, deferred with evidence.

Reproduced first. The new witness holds the first send until the startup chat has tool declarations (chat existence flips at once, tools lands later) run against the pre-fix code:

Tests  1 failed | 45 passed (46)
 ❯ live-session.test.ts:336   expect(sendMessageStream).not.toHaveBeenCalled()
   AssertionError: expected "spy" to not be called at all, but actually been called 1 times

Fix. The wait now releases on chat readiness rather than existence:

const chatReady = () =>
  client.isInitialized() &&
  client.getChat().getGenerationConfig().tools !== undefined;

setTools() is startChat()'s last awaited stage (client.ts:2308-2310) and always writes tools (client.ts:1119 → llm-chat.ts:5162-5163) even when the registry is empty, so its presence marks a chat the flight actually finished — session-start context included, because that is applied earlier (client.ts:2289-2303). Both facts the finding said the fix must not violate still hold: the STARTUP_CHAT_WAIT_MS bound is unchanged (and now has its own witness, per R1-2), and isInitialized() itself is untouched, so its other readers — the re-entry guard (client.ts:529) and setTools()'s early return (client.ts:1100) — keep their semantics. No packages/core change, so the PR's footprint is unchanged.

Witnesses and mutation probes — one per condition of the predicate; every mutant red, every restore green, live-session.test.ts 47 passed intact:

condition removed witness test mutant result
tools !== undefined holds the first send until the startup chat has tool declarations expected "spy" to not be called at all, but actually been called 1 times
client.isInitialized() waits for the chat an in-flight startup initialization creates Error: Chat not initialized
Date.now() < chatDeadline reports the client error once the startup chat wait is spent Test timed out in 15000ms — hangs instead of rejecting

Direction 2 — verified real, not implementable from this package, recorded as a deferred finding. A flight that settles without creating a chat leaves isInitialized() false forever, so the bounded wait is spent and the turn reports Chat not initialized about 15s later instead of immediately. Confirmed against the code: Config keeps settlement private — initializationPromise / initializationSucceeded / initializationSettled (config.ts:2418-2420) are read only by shutdownResources (config.ts:5944-5948) — and initializeOnce's catch (config.ts:3064) rethrows without recording anything publicly observable. From packages/cli a failed flight is therefore indistinguishable from an in-flight one, so there is no predicate to fail fast on. Closing it needs one public core accessor (Config.isInitializationSettled() returning this.initializationSettled) plus && !config.isInitializationSettled?.() in the loop condition — a packages/core change this PR never touched, which is why it goes to the follow-up queue instead of expanding the footprint here. If a maintainer would rather carry it in this PR, say so and the next round lands it.

中文说明

方向一(提前放行)—— 先复现,再修复。方向二(快速失败)—— 已确认属实,附带证据转入后续队列。

先复现。 新增的见证用例 holds the first send until the startup chat has tool declarations(chat 存在性立刻翻转、tools 稍后才落地)在修复前的代码上运行结果:

Tests  1 failed | 45 passed (46)
 ❯ live-session.test.ts:336   expect(sendMessageStream).not.toHaveBeenCalled()
   AssertionError: expected "spy" to not be called at all, but actually been called 1 times

修复。 这个等待现在以 chat 的可用状态为放行条件,而不是以它是否存在:

const chatReady = () =>
  client.isInitialized() &&
  client.getChat().getGenerationConfig().tools !== undefined;

setTools()startChat() 最后一个被 await 的阶段(client.ts:2308-2310),并且即使注册表为空也总会写入 tools(client.ts:1119 → llm-chat.ts:5162-5163),因此它的存在正好标记「初始化流程确实完成了这个 chat」—— 也包含 session-start 上下文,因为那一步更早执行(client.ts:2289-2303)。该 finding 提出修复不得违反的两个事实依然成立:STARTUP_CHAT_WAIT_MS 上界未变(并且按 R1-2 的要求现在有了自己的见证用例);isInitialized() 本身未被改动,因此它的其他读取方 —— 重入门禁(client.ts:529)与 setTools() 的提前返回(client.ts:1100)—— 语义保持不变。没有改动 packages/core,PR 的 footprint 保持不变。

见证用例与变异探针 —— 谓词的每个条件各有一个;每个变异体都变红、每次恢复都变绿,live-session.test.ts 完整状态下 47 通过:

被移除的条件 见证用例 变异体结果
tools !== undefined holds the first send until the startup chat has tool declarations expected "spy" to not be called at all, but actually been called 1 times
client.isInitialized() waits for the chat an in-flight startup initialization creates Error: Chat not initialized
Date.now() < chatDeadline reports the client error once the startup chat wait is spent Test timed out in 15000ms —— 表现为挂起而不是以错误结束

方向二 —— 已确认属实,但无法在本包内实现,已记录为延后 finding。 一个「落定但没有创建出 chat」的初始化流程会让 isInitialized() 永远为 false,于是有界等待被耗尽,该轮对话在约 15 秒后才报出 Chat not initialized,而不是立即失败。已对照代码确认:Config 把落定状态保持为私有 —— initializationPromise / initializationSucceeded / initializationSettled(config.ts:2418-2420)只被 shutdownResources 读取(config.ts:5944-5948)—— 而 initializeOnce 的 catch(config.ts:3064)只是重新抛出,没有记录任何公开可观测的状态。因此在 packages/cli 看来,「初始化失败」与「初始化仍在进行中」无法区分,也就没有可供快速失败的谓词。要闭合它需要一个公开的 core 访问器(Config.isInitializationSettled(),返回 this.initializationSettled),并在循环条件中加上 && !config.isInitializationSettled?.() —— 那是对 packages/core 的改动,而本 PR 从未触及该区域,所以它进入后续队列,而不是在这里扩大 footprint。如果维护者更希望在本 PR 内一并处理,请回复说明,下一轮就会落地。

Comment thread packages/cli/src/ui/opentui/live-session.ts Outdated
@qwen-code-dev-bot

qwen-code-dev-bot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

AutoFix round 2 finishedview run. See this round's report below.

中文说明

AutoFix 第 2 轮已完成 —— 查看运行。本轮报告见下方。

)

Chat existence is not readiness: startChat() assigns the chat and only
then awaits the SessionStart hook, its context and setTools(), so a wait
polling isInitialized() released the session's first prompt before the
tool declarations existed and the turn reached the model with no tools.
Release on the generation config's tools instead — setTools() is the
flight's last stage and always writes them — and give the wait's
deadline branch its own fake-timer witness.
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator Author

🤖 Addressed the latest review feedback (round 1/10). What changed, and what I pushed back on: · 已处理最新评审反馈(第 1/10 轮)。改动内容与我反驳保留之处如下:

Repair round — PR #11046 (issue #11042)

Same-run verification repair. The supplied rejection was the runner's disk, not the committed fix: npm run build re-run on the byte-identical tree exits 0 with zero ENOSPC. No follow-up commit — the rejected commit 07fe398784 is preserved untouched, because there is no code cause to fix.

1. The deterministic rejection, diagnosed from its own output

The gate rejected the previous pass with build failed on the agent-committed fix. Its detail is a write-side filesystem error, repeated for every file it could not emit:

error TS5033: Could not write file '…/packages/cli/dist/src/acp-integration/skill-management.test.d.ts': ENOSPC: no space left on device, write.
error TS5033: Could not write file '…/packages/cli/dist/src/acp-integration/skill-source-download.test.js.map': ENOSPC: no space left on device, write.
Error: Command failed: tsc --build   (scripts/build_package.js:38)

Three measured facts put this on the host rather than in the diff:

  1. TS5033 is emit, not diagnosis. tsc accepted the sources and failed while writing its output. No error names anything this round changed.
  2. Every unwritable file is outside this PR's footprint. All six are packages/cli/dist/src/acp-integration/skill-{management,source-download}.test.{d.ts,js,js.map}. The branch diff is two files under packages/cli/src/ui/opentui/ (git diff --name-only origin/main...HEAD), so nothing in it can make an unrelated directory's emit unwritable.
  3. The host was below this repository's own pre-job disk floor. check-disk-floor.sh — added by ci: prevent transient ENOSPC on high-concurrency self-hosted runners #10035 for exactly this failure, its header reading "ENOSPC failed npm ci mid-install on a saturated host while the workspace cleanup timer skipped busy runners" — defaults DISK_FLOOR_MIN_FREE_KB to 2097152 KiB (2 GiB). Samples of /dev/nvme0n1p3 taken on this runner during the repair pass:
sample KiB free GiB free use%
round start (df -h) 2.2 100%
immediately before npm run build, 00:08:35Z 1 515 392 1.45 100%
during the build, consecutive samples 1 350 456 → 1 055 640 1.29 → 1.01 100%
during the build, after a concurrent host job released space 9 441 900 9.00 100%
at build exit 0, 00:17:33Z 7 161 372 6.83 100%

The build started 0.55 GiB under the 2 GiB floor, fell to 1.01 GiB, and finished only after headroom appeared from outside this job. This checkout is not what fills the device: the repository is 3.9 GiB of the 965 GiB used, and _work holds no second workspace.

For the maintainer — out of scope, deliberately not done here. ci.yml runs the floor guard before five heavy jobs (test at ci.yml:601-603, lint_and_static at 1072, web_shell_e2e_smoke at 1385, integration_no_ak at 1919, integration_cli at 2134). qwen-autofix.yml calls it nowhere — a grep for check-disk-floor, DISK_FLOOR, and disk pressure across that file returns nothing — so this loop is the heavy lane with no pre-job floor: a saturated host surfaces as a build failure charged to the round instead of a clean, retryable reschedule onto a host with headroom, which is precisely what the guard was written to do. Wiring it in is a .github/ change, outside this PR's footprint and outside what a review round may touch, so it is reported rather than implemented.

2. Why this round adds no commit

The repair directive is to preserve the rejected commit and add one verified follow-up commit fixing the rejection. This rejection has no code cause, so there is nothing to write: HEAD is still 07fe398784b3e257a19c154585ecef298e65c981, not amended, rebased, or reset; git status --porcelain=v1 --untracked-files=all is empty; and the exact rejected command now passes (section 5). Inventing a source edit to satisfy the shape would add a change that fixes nothing — a full host disk is not reachable from packages/cli/src/ui/opentui/ — and would fail Simplicity First.

The gate's routing accepts this state. Its no-commit early-exit keys on git diff --quiet origin/<branch>...<branch>, which is false here: 07fe398784 is still the one commit ahead of origin/autofix/issue-11042, having never been pushed. The round therefore takes the branch-changed path with this summary as its report, and the verified head that gets pushed is the preserved commit.

3. Feedback dispositions (all carried by the preserved commit, re-verified this pass)

No feedback point changed state — the code they were judged against is byte-identical — so the dispositions stand, now with this pass's runs as fresh evidence:

  • [rc:3938176016] Critical R1-1direction 1 fixed in code; direction 2 verified and deferred. The wait releases on chat readiness (client.isInitialized() && client.getChat().getGenerationConfig().tools !== undefined) rather than chat existence, so the session's first prompt can no longer leave before setTools() writes the declarations. Witness holds the first send until the startup chat has tool declarations: red on the pre-fix code (1 failed | 45 passed (46), expected "spy" to not be called at all, but actually been called 1 times), green since — 47 passed this pass. Direction 2 (a flight that settles without creating a chat still spends the 15 s budget before failing) is verified real but not implementable from packages/cli: settlement is private on ConfiginitializationPromise / initializationSucceeded / initializationSettled at config.ts:2418-2420, re-confirmed this pass — read only by shutdownResources, so from this package a failed flight is indistinguishable from an in-flight one. Recorded in deferred-findings.json (it needs one public core accessor, i.e. a packages/core change outside this PR's footprint) and answered on its thread, which stays open; it is therefore not in resolved-comments.txt, so a partly-deferred Critical is never read as silently dropped.
  • [rc:3938176022] Suggestion R1-2implemented. reports the client error once the startup chat wait is spent holds readiness permanently false under vi.useFakeTimers(), advances past the now-exported STARTUP_CHAT_WAIT_MS, and asserts drain() rejects with the stand-in's Chat not initialized. Listed in resolved-comments.txt.
  • [rv:5118198661] Test Plan numbers (review body, marked "not a blocker") — no code change; scope mismatch. The quoted figures are this PR's focused runs at their own scope (live-session.test.ts 45 → now 47; the E2E files that reproduced the bug 15 / 4 / 7; the OpenTUI leg 19 and 44), while 28503 passed is the whole-repo unit suite that review ran. Both are correct at their own scope; no focused count was presented as a full-suite result. This pass's counts are in section 5.
  • Failed checksone correction to the previous pass's diagnosis, plus the evidence this runner can actually reach. All results below come from the workflow-supplied checks.json, and all belong to head ca59b73884 — the only head CI has ever seen. The current head 07fe398784 has never been pushed, so CI has not evaluated it.
    • Test (ubuntu-latest, Node 22.x) FAILURE, 20:37:59Z → 21:00:51Z (22m52s), job log …/actions/runs/33917081264/job/101166955590. It passed its own pre-job disk floor (ci.yml:601-603), so it was not rejected for saturation at admission; it then ran the suite under --retry=2 (VITEST_RETRY default '2', ci.yml:700 → 753 → 756, then test:scripts at 759) and failed inside the test step, well under its 110-minute budget — a test failure, not a timeout. The Test job samples disk pressure every 10 s (ci.yml:749) and uploads disk-pressure-samples.log on failure (ci.yml:771-776), so whether the host degraded mid-run is answerable from that job's own artifacts. This runner has no GitHub credentials (verified: no GITHUB_TOKEN / GH_TOKEN in the environment), so neither the failing test name nor that artifact is readable here. This is not labelled pre-existing or unrelated — only "not reproducible locally on either head".
    • web-shell E2E Smoke (ubuntu-latest, Node 22.x) CANCELLED, 21:00:55Z → 21:21:27Z (20m32s). Correction: the previous pass called this a needs: consequence of the Test failure. That is wrong. Its if: (ci.yml:1224-1230) requires only !cancelled() and needs.test.outputs.ci_profile == 'full' (line 1229) — never needs.test.result == 'success' — so the Test failure did not skip it, and it started 4 s after the Test job finished. It then ran 20m32s against timeout-minutes: 20 (ci.yml:1232) and was cancelled at that bound: its own timeout, on the same self-hosted fleet.
    • Green on that same head, and the lanes that actually exercise this change: TUI parity snapshots (ink vs opentui) SUCCESS, OpenTUI no-flicker gate SUCCESS, Integration Tests (no-AK, No Sandbox) SUCCESS, Lint & Static (ubuntu-latest, Node 22.x) SUCCESS, Secret scan / Dependency CVE audit SUCCESS. Only the whole-repo ubuntu unit lane is red, and Test (macos/windows) were SKIPPED by the profile.
    • What this pass adds locally: the gate's own package-test command is green on the current head (section 5). The workflow's independent CI run after this push remains the deciding evidence for both checks.

4. Changes

None this pass — zero source lines, zero test lines. The branch still carries exactly what 07fe398784 added: packages/cli/src/ui/opentui/live-session.ts (+13 / −2) and packages/cli/src/ui/opentui/live-session.test.ts (+89 / −1). Footprint unchanged; no packages/core, CI, workflow, config, or lockfile change; no test deleted or weakened.

5. Conflict notes

--conflict falseorigin/main was not merged; the branch stays on its own head. No conflicts encountered.

Verification

Every command below was actually run this pass on HEAD 07fe398784 with a clean working tree.

  • npm run buildpassed (exit 0), 00:08:35Z → 00:17:33Z; grep -c ENOSPC over its 798-line log = 0. This is the exact command the gate rejected.
  • npm run typecheck — passed (exit 0)
  • npm run lint — passed (exit 0)
  • cd packages/cli && npx vitest run src/ui/opentui/live-session.test.ts47 passed (1 file), exit 0
  • cd packages/cli && npx vitest run src/ui/opentui1135 passed (67 files), exit 0
  • npm run test --workspace packages/cli --if-present -- --changed origin/main --passWithNoTests — the gate's own package-test command — 225 passed (5 files: llm.test.tsx 85, cli.test.ts 78, live-session.test.ts 47, live-turn.test.ts 12, start-opentui-ui.test.tsx 3), exit 0
  • git rev-parse HEAD07fe398784b3e257a19c154585ecef298e65c981; git status --porcelain=v1 --untracked-files=all → empty; git log --oneline origin/autofix/issue-11042..HEAD → the single preserved commit
  • df -kP samples — section 1's table
  • Read-only verification of every citation above: ci.yml lines 601-603, 700, 749, 752-759, 771-776, 1221-1232, 1385; config.ts:2418-2420; .github/scripts/check-disk-floor.sh; .github/scripts/run-autofix-review-verification.sh (build check at line 1030, no-commit routing at 624-650, success path outcome=fixed + verified_head at 1362-1363); .github/scripts/autofix-push-and-report.sh (push precondition at 504-519); workflow-supplied checks.json
  • Not re-run this pass, with reasons. Mutation probes: no source line changed, so there is no new guard to witness; the preserved commit's three probes (one per condition of the readiness predicate) are recorded in the previous pass's summary and their witnesses are green above. OpenTUI interactive E2E leg: the previous pass ran CI's exact command green on this identical SHA (Test Files 10 passed | 1 skipped, Tests 19 passed | 2 skipped, 207.04 s) and no code changed since; the gate does not re-run it. npm run generate:settings-schema: not applicable — no settings source changed. Whole-repo unit suite: outside this skill's allowed command set, and the workflow re-runs it independently after the push.
中文说明

修复轮次 —— PR #11046(issue #11042

同一次运行内的验证修复(same-run verification repair)。本次给出的驳回原因是 runner 的磁盘,而不是已提交的修复:在逐字节完全相同的代码树上重跑 npm run build,退出码为 0,ENOSPC 出现 0 次。本轮不追加 commit —— 被驳回的 commit 07fe398784 原样保留,因为不存在需要修复的代码成因。

1. 依据驳回自身的输出完成诊断

门(gate)以 build failed on the agent-committed fix 驳回了上一轮。它的详情是写入侧的文件系统错误,对每个无法写出的文件重复出现:

error TS5033: Could not write file '…/packages/cli/dist/src/acp-integration/skill-management.test.d.ts': ENOSPC: no space left on device, write.
error TS5033: Could not write file '…/packages/cli/dist/src/acp-integration/skill-source-download.test.js.map': ENOSPC: no space left on device, write.
Error: Command failed: tsc --build   (scripts/build_package.js:38)

三条经过测量的事实把问题定位在宿主机,而不是在这次 diff 里:

  1. TS5033 属于产物写出阶段,不是类型诊断。 tsc 接受了源码,是在写出产物时失败的。没有任何一条错误指向本轮改动的内容。
  2. 所有写不出来的文件都在本 PR 的 footprint 之外。 六个文件全部是 packages/cli/dist/src/acp-integration/skill-{management,source-download}.test.{d.ts,js,js.map}。分支的 diff 只有 packages/cli/src/ui/opentui/ 下的两个文件(git diff --name-only origin/main...HEAD),因此它不可能让一个无关目录的产物变得无法写入。
  3. 宿主机当时低于本仓库自己设定的作业前磁盘下限。 check-disk-floor.sh —— 由 ci: prevent transient ENOSPC on high-concurrency self-hosted runners #10035 为完全相同的故障而加入,其文件头写着「ENOSPC failed npm ci mid-install on a saturated host while the workspace cleanup timer skipped busy runners」—— 把 DISK_FLOOR_MIN_FREE_KB 默认设为 2097152 KiB(2 GiB)。修复轮期间在本 runner 上对 /dev/nvme0n1p3 的采样:
采样点 剩余 KiB 剩余 GiB 使用率
轮次开始(df -h 2.2 100%
npm run build 之前,00:08:35Z 1 515 392 1.45 100%
build 期间的连续采样 1 350 456 → 1 055 640 1.29 → 1.01 100%
build 期间,宿主机上另一个并发作业释放空间之后 9 441 900 9.00 100%
build 退出 0 时,00:17:33Z 7 161 372 6.83 100%

build 启动时比 2 GiB 下限少 0.55 GiB,中途降到 1.01 GiB,直到本作业之外出现空间才跑完。占满这块设备的不是本次 checkout:已用的 965 GiB 中仓库只占 3.9 GiB,且 _work 下没有第二个 workspace。

给维护者的一条说明 —— 超出范围,此处刻意未做。 ci.yml 在五个重负载 job 之前运行了这个下限防护(test 在 ci.yml:601-603、lint_and_static 在 1072、web_shell_e2e_smoke 在 1385、integration_no_ak 在 1919、integration_cli 在 2134)。而 qwen-autofix.yml 完全没有调用它 —— 在该文件中 grep check-disk-floorDISK_FLOORdisk pressure 均无结果 —— 因此这条重负载通道没有作业前下限:宿主机饱和时会表现为「算在这一轮头上的 build 失败」,而不是干净地重试并调度到有余量的宿主机上,而后者正是这个防护被写出来要做的事。把它接进去属于 .github/ 改动,既在本 PR 的 footprint 之外,也在评审轮次允许触碰的范围之外,所以此处只做上报而不实现。

2. 本轮为什么不追加 commit

修复指令要求保留被驳回的 commit,并追加一个经过验证、能修掉该驳回的 commit。但这个驳回没有代码成因,因此没有可写的内容:HEAD 仍是 07fe398784b3e257a19c154585ecef298e65c981,没有 amend、rebase 或 reset;git status --porcelain=v1 --untracked-files=all 输出为空;而被驳回的那条命令现在通过了(见第 5 节)。为了满足形式而凭空造一处源码改动,只会增加一个什么都修不了的变更 —— 从 packages/cli/src/ui/opentui/ 是够不着宿主机磁盘写满这种状况的 —— 并且不符合 Simplicity First。

门自身的路由也接受这种状态。它的「无 commit」提前退出以 git diff --quiet origin/<branch>...<branch> 为判据,而此处该判据为假:07fe398784 仍然是领先 origin/autofix/issue-11042 的那一个 commit,且从未被推送。因此本轮走的是「分支已变更」的路径,以这份摘要作为轮次报告,而被推送的已验证 head 就是那个被保留的 commit。

3. 各条反馈的处理(全部由被保留的 commit 承载,本轮重新验证)

没有任何反馈点的状态发生变化 —— 它们所针对的代码逐字节相同 —— 因此处理结论维持原样,并以本轮的运行结果作为新证据:

  • [rc:3938176016] Critical R1-1 —— 方向一已在代码中修复;方向二已确认属实并延后。 这个等待现在以 chat 的可用状态放行(client.isInitialized() && client.getChat().getGenerationConfig().tools !== undefined),而不是以 chat 是否存在,因此本次会话的第一个 prompt 不会在 setTools() 写入工具声明之前就发出。见证用例 holds the first send until the startup chat has tool declarations:在修复前的代码上为红(1 failed | 45 passed (46)expected "spy" to not be called at all, but actually been called 1 times),此后为绿 —— 本轮 47 通过。方向二(一次「落定但没有创建出 chat」的初始化流程仍会耗尽 15 秒预算才失败)已确认属实,但无法在 packages/cli 内实现:落定状态在 Config 上是私有的 —— initializationPromise / initializationSucceeded / initializationSettled 位于 config.ts:2418-2420,本轮已重新确认 —— 只被 shutdownResources 读取,因此在本包看来「初始化失败」与「初始化仍在进行」无法区分。它已记录到 deferred-findings.json(需要一个公开的 core 访问器,即 packages/core 改动,超出本 PR 的 footprint),并在其 thread 上作了回复,该 thread 保持 open;因此它没有写进 resolved-comments.txt,这样一个部分延后的 Critical 不会被读成被静默丢弃。
  • [rc:3938176022] Suggestion R1-2 —— 已实现。 reports the client error once the startup chat wait is spentvi.useFakeTimers() 下让可用状态恒为 false,把虚拟时间推进到超过现在已导出的 STARTUP_CHAT_WAIT_MS,并断言 drain() 以替身的 Chat not initialized 结束。已写入 resolved-comments.txt
  • [rv:5118198661] Test Plan 数字(review 正文,已标注「非阻断」)—— 无代码改动;口径不一致。 被引用的数字是本 PR 在各自口径下的定向运行结果(live-session.test.ts 45 → 现在 47;复现该 bug 的 E2E 文件 15 / 4 / 7;OpenTUI leg 19 与 44),而 28503 passed 是那次评审运行的全仓库单测总数。两者在各自口径上都正确,没有任何定向数字被当作全量结果呈现。本轮的实际数字见第 5 节。
  • 失败的检查 —— 对上一轮诊断的一处更正,外加本 runner 实际能够取得的证据。 下面所有结果都来自 workflow 提供的 checks.json,且全部属于 head ca59b73884 —— CI 唯一见过的 head。当前 head 07fe398784 从未被推送,因此 CI 还没有评估过它。
    • Test (ubuntu-latest, Node 22.x) FAILURE,20:37:59Z → 21:00:51Z(22 分 52 秒),job 日志 …/actions/runs/33917081264/job/101166955590。它通过了自己的作业前磁盘下限(ci.yml:601-603),所以并不是在准入阶段因饱和被拒;随后它在 --retry=2 下运行测试套件(VITEST_RETRY 默认 '2',ci.yml:700 → 753 → 756,之后是 759 的 test:scripts),并在测试步骤内部失败,远低于其 110 分钟预算 —— 是测试失败,不是超时。Test job 每 10 秒采样一次磁盘压力(ci.yml:749),并在失败时上传 disk-pressure-samples.log(ci.yml:771-776),因此「宿主机是否在运行中途恶化」可以从该 job 自己的产物中得出答案。本 runner 没有 GitHub 凭证(已验证:环境中没有 GITHUB_TOKEN / GH_TOKEN),所以失败的测试名和那份产物在此都不可读。这里没有把它标注为既有问题或无关 —— 只能表述为「在两个 head 上都无法本地复现」。
    • web-shell E2E Smoke (ubuntu-latest, Node 22.x) CANCELLED,21:00:55Z → 21:21:27Z(20 分 32 秒)。更正: 上一轮把它说成是 Test 失败导致的 needs: 连带结果。这是错的。它的 if:(ci.yml:1224-1230)只要求 !cancelled()needs.test.outputs.ci_profile == 'full'(第 1229 行),从未要求 needs.test.result == 'success',因此 Test 失败并不会跳过它;它在 Test job 结束 4 秒后就开始了,随后运行 20 分 32 秒,正好撞上 timeout-minutes: 20(ci.yml:1232)而被取消:是它自己的超时,发生在同一个自托管机群上。
    • 在同一个 head 上为绿、且真正覆盖本次改动的通道:TUI parity snapshots (ink vs opentui) SUCCESS、OpenTUI no-flicker gate SUCCESS、Integration Tests (no-AK, No Sandbox) SUCCESS、Lint & Static (ubuntu-latest, Node 22.x) SUCCESS、Secret scan / Dependency CVE audit SUCCESS。只有全仓库 ubuntu 单测通道是红的,而 Test (macos/windows) 被 profile 判为 SKIPPED。
    • 本轮在本地新增的证据:门自己的 package-test 命令在当前 head 上是绿色的(第 5 节)。真正决定这两个检查的,仍是本次 push 之后 workflow 独立重跑的 CI。

4. 改动内容

本轮没有改动 —— 源码 0 行、测试 0 行。分支上仍然只有 07fe398784 加入的内容:packages/cli/src/ui/opentui/live-session.ts(+13 / −2)与 packages/cli/src/ui/opentui/live-session.test.ts(+89 / −1)。footprint 未变;没有改动 packages/core、CI、workflow、配置或 lockfile;没有删除或削弱任何测试。

5. 冲突说明

--conflict false —— 没有合并 origin/main;分支保持在自己的 head 上。没有遇到冲突。

Verification(验证)

下面每条命令都是本轮在 HEAD 07fe398784、工作区干净的状态下实际运行的。

  • npm run build —— 通过(exit 0),00:08:35Z → 00:17:33Z;在其 798 行日志上 grep -c ENOSPC = 0。这正是门所驳回的那条命令。
  • npm run typecheck —— 通过(exit 0)
  • npm run lint —— 通过(exit 0)
  • cd packages/cli && npx vitest run src/ui/opentui/live-session.test.ts —— 47 通过(1 个文件),exit 0
  • cd packages/cli && npx vitest run src/ui/opentui —— 1135 通过(67 个文件),exit 0
  • npm run test --workspace packages/cli --if-present -- --changed origin/main --passWithNoTests —— 即门自己使用的 package-test 命令 —— 225 通过(5 个文件:llm.test.tsx 85、cli.test.ts 78、live-session.test.ts 47、live-turn.test.ts 12、start-opentui-ui.test.tsx 3),exit 0
  • git rev-parse HEAD07fe398784b3e257a19c154585ecef298e65c981git status --porcelain=v1 --untracked-files=all → 空;git log --oneline origin/autofix/issue-11042..HEAD → 只有那个被保留的 commit
  • df -kP 采样 —— 见第 1 节的表格
  • 上文每一处引用都经过只读核对:ci.yml 第 601-603、700、749、752-759、771-776、1221-1232、1385 行;config.ts:2418-2420.github/scripts/check-disk-floor.sh.github/scripts/run-autofix-review-verification.sh(build 检查在第 1030 行、无 commit 路由在 624-650 行、成功路径 outcome=fixed + verified_head 在 1362-1363 行);.github/scripts/autofix-push-and-report.sh(push 前置条件在 504-519 行);workflow 提供的 checks.json
  • 本轮未重跑的项目及其原因。 变异探针:没有改动任何源码行,因此没有新的防护需要见证;被保留 commit 的三个探针(可用性谓词的每个条件各一个)已记录在上一轮的摘要中,而它们的见证用例在上面均为绿色。OpenTUI interactive E2E leg:上一轮已在这个完全相同的 SHA 上用 CI 完全一致的命令跑绿(Test Files 10 passed | 1 skippedTests 19 passed | 2 skipped、207.04 秒),此后代码没有变化;门也不会重跑它。npm run generate:settings-schema:不适用 —— 没有修改任何 settings 源文件。全仓库单测:不在本 skill 允许的命令集合内,且 push 之后 workflow 会独立重跑。

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

🧵 Resolved all 1 selected review thread(s). · 已关闭全部选中的 1 条评审线程。

Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审;第 10 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-2026-09-02

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/ui/opentui/live-session.ts:587 — [probe] the up-to-15s startup wait is silent — no log breadcrumb when a turn stalls here or the budget expires
中文说明

已审查。 建议见行内评论。

收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/cli/src/ui/opentui/live-session.ts Outdated
Comment thread packages/cli/src/ui/opentui/live-session.ts
Comment on lines +551 to +552
export const STARTUP_CHAT_WAIT_MS = 15_000;
const STARTUP_CHAT_POLL_MS = 100;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R2-3: STARTUP_CHAT_WAIT_MS / STARTUP_CHAT_POLL_MS duplicate STARTUP_REGISTRY_WAIT_MS / STARTUP_REGISTRY_POLL_MS (commands-dispatch.ts:193-194) — the comment above declares them "the same budget" bounding "the same window", yet they are stored as two independent literal pairs whose equality is maintained only by prose. When someone tunes STARTUP_REGISTRY_WAIT_MS (e.g. widens the self-heal window for slow skill/extension loading), STARTUP_CHAT_WAIT_MS silently stays at 15s: a prompt submitted inside the widened window expires its wait, falls through before startChat() completes, and is dropped with Chat not initialized — the exact bug this PR fixes — while slash dispatch is still self-healing on the longer budget.

Witness:

witness: not run — the duplication and the prose coupling are static facts enumerated by grep
(live-session.ts:551-552 vs commands-dispatch.ts:193-194, plus the "same budget" comment at live-session.ts:550);
the failure scenario is a hypothetical future tune of one constant, for which no run exists today
(closest capability: sweep — the population is exactly the two literal pairs quoted).

Single-source the budget — export the pair from one module and import it in both:

export const STARTUP_WINDOW_WAIT_MS = 15_000;
export const STARTUP_WINDOW_POLL_MS = 100;
中文说明

[Suggestion] R2-3: STARTUP_CHAT_WAIT_MS / STARTUP_CHAT_POLL_MS 重复了 STARTUP_REGISTRY_WAIT_MS / STARTUP_REGISTRY_POLL_MS(commands-dispatch.ts:193-194)—— 上方注释宣称它们是「同一个预算」、覆盖「同一个时间窗」,但两者被存成了两对独立的字面量,相等关系只靠注释文字维持。一旦有人调整 STARTUP_REGISTRY_WAIT_MS(例如为较慢的 skill/extension 加载放宽自愈窗口),STARTUP_CHAT_WAIT_MS 会悄悄停留在 15 秒:在放宽后的窗口内提交的 prompt 会耗尽等待、在 startChat() 完成前 fall through,以 Chat not initialized 被丢弃 —— 正是本 PR 修复的缺陷 —— 而斜杠命令分发仍在更长的预算下自愈。

证据:

witness: not run —— 重复与注释耦合是 grep 即可枚举的静态事实
(live-session.ts:551-552 对比 commands-dispatch.ts:193-194,外加 live-session.ts:550 的「same budget」注释);
失败场景是对其中一个常量的假想未来调整,今天不存在任何可运行的验证
(最接近的能力:sweep —— 总体恰好是上面引用的两对字面量)。

建议把预算单一来源化:从一个模块导出这对常量并在两处 import(代码见英文部分)。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declined (structural half); the prose half is fixed.

Both literal pairs confirmed at this head. I did not single-source them: packages/cli/src/ui/opentui/ has no shared-constants module, so it means either a new file whose entire content is two numbers, or importing one feature module into the other for them — either direction buys a real dependency for a coincidental equality. The two waits also bound different resources through different probes with different degradations (config.getSkillManager?.() → an "Unknown command" message, vs chat existence + generationConfig.tools → a dropped prompt), and your own witness records not run: nothing behaves wrongly today.

The part that was a defect is the prose asserting an equality nothing enforces, so the comment above the pair no longer claims "the same budget commands-dispatch gives its registry self-heal" (net −1 line) — with that claim gone there is no stated invariant left to drift. The real consolidation is R2-4's: if Config.initialize() ever lets concurrent callers join the in-flight initialization, both polling loops and both budget pairs disappear together, and an intermediate shared constant would be surface that fix deletes.

中文说明

不予采纳(结构性的一半);注释文字的那一半已修复。

两对字面量在当前 head 上均已确认。我没有把它们单一来源化:packages/cli/src/ui/opentui/ 下没有共享常量模块,因此这意味着要么新建一个内容只有两个数字的文件,要么为了这两个数字把一个功能模块 import 进另一个 —— 任一方向都是为了一次偶然的相等而引入真实依赖。这两个等待还通过不同的判据约束不同的资源、并有不同的降级表现(config.getSkillManager?.() → "Unknown command" 提示,对比 chat 是否存在 + generationConfig.tools → prompt 被丢弃),而你自己的证据记录了 not run:今天没有任何行为是错的。

真正构成缺陷的是那段断言了「无人强制的相等关系」的注释,因此这对常量上方的注释不再宣称 "the same budget commands-dispatch gives its registry self-heal"(净减 1 行)—— 该断言去掉后,就不存在会漂移的既定不变量了。真正的收敛属于 R2-4:如果 Config.initialize() 有朝一日允许并发调用方加入正在进行中的初始化,那么两个轮询循环与两对预算常量会一起消失,而中间那层共享常量正是那个修复会删掉的表面。

Comment on lines +588 to +591
const chatReady = () =>
client.isInitialized() &&
client.getChat().getGenerationConfig().tools !== undefined;
while (!chatReady() && Date.now() < chatDeadline) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R2-4: This wait compensates downstream for an upstream defect — Config.initialize() throws Config was already initialized at concurrent callers (config.ts:3002; the guard flips at config.ts:3008 before the work runs) instead of letting them join the in-flight initialization, which stays private (initializationPromise / initializationSettled, config.ts:2418-2420, no public await accessor). Every startup-racing consumer grows its own bounded polling loop with its own cross-layer readiness probe — this is the second copy after commands-dispatch's registry self-heal (commands-dispatch.ts:343-361). The probe reaches client → chat → generation-config and depends on startChat()'s internal stage ordering plus setTools() always writing tools — facts none of the stand-in-based tests pins against the real client. A future core reorder (assigning this.chat last, or pre-initializing generationConfig.tools) silently flips this wait into releasing too early — reintroducing the no-tools first prompt — or never releasing, so every first prompt pays the full 15s and then fails with Chat not initialized anyway.

Witness:

witness: not run — root-cause attribution verified by tracing config.ts:2997-3016 (guard flips before the work),
config.ts:2418-2420 (private fields, read only by shutdown internals at config.ts:5908-5963),
commands-dispatch.ts:343-361 (first polling copy), live-session.ts:587-593 (second copy);
there is no executable oracle for what the core API should be.

Close the class at the owner: dedupe in-flight initialize() calls so concurrent callers await the real flight — e.g. if (this.initializationPromise && !this.initializationSettled) return this.initializationPromise; before the throw in Config.initialize(). That depth is maintainer-gated (packages/core/src/config/**), so treat this as an escalation to the maintainer if the PR cannot carry it; the caller-side loop is acceptable as the interim fix. The dedupe must keep sequential re-calls throwing — docstring Must be called once, throws if called again. (config.ts:2994) and the throw at config.ts:3002 — hence gating on !this.initializationSettled (set at config.ts:3015). A core test asserting that a second initialize() made while the first is in flight resolves when the flight completes (instead of throwing) must go red if the dedupe branch is removed.

中文说明

[Suggestion] R2-4: 这个等待是在下游补偿上游的缺陷 —— Config.initialize() 会对并发调用方抛出 Config was already initializedconfig.ts:3002,守卫在 config.ts:3008 处先于真正的工作翻转),而不是让它们加入正在进行中的初始化;该状态保持私有(initializationPromise / initializationSettledconfig.ts:2418-2420,没有可公开 await 的访问器)。每个与启动竞速的消费者都不得不各自长出一个有界轮询循环和各自的跨层就绪探针 —— 这是继 commands-dispatch 的注册表自愈(commands-dispatch.ts:343-361)之后的第二份拷贝。该探针要穿过 client → chat → generation-config,并依赖 startChat() 的内部阶段顺序以及 setTools() 总会写入 tools —— 这些事实没有任何基于替身的测试对照真实客户端钉住。未来 core 的一次重排(把 this.chat 放到最后赋值,或预先初始化 generationConfig.tools)会悄悄把这个等待变成过早放行 —— 重新引入「第一个 prompt 没有工具声明」的回归 —— 或者永不放行,于是每个第一个 prompt 都要等满 15 秒、然后照样以 Chat not initialized 失败。

证据:

witness: not run —— 根因归属通过追踪以下代码验证:config.ts:2997-3016(守卫先于工作翻转)、
config.ts:2418-2420(私有字段,仅被 config.ts:5908-5963 的 shutdown 内部读取)、
commands-dispatch.ts:343-361(第一份轮询拷贝)、live-session.ts:587-593(第二份);
对「core API 应该长什么样」不存在可执行的预言机。

建议在源头闭合这一类问题:对进行中的 initialize() 调用去重,让并发调用方 await 真实的初始化流程 —— 例如在 Config.initialize() 的抛出之前加上 if (this.initializationPromise && !this.initializationSettled) return this.initializationPromise;。该深度由维护者门禁管辖(packages/core/src/config/**),如果本 PR 不能承载该改动,请作为升级项转交维护者;当前的调用方侧循环作为临时修复可以接受。去重必须保持「顺序的再次调用仍然抛错」—— docstring 为 Must be called once, throws if called again.config.ts:2994),抛错在 config.ts:3002 —— 因此以 !this.initializationSettled 作为门禁(该字段在 config.ts:3015 置位)。core 测试应断言:在第一次初始化仍在进行时发起的第二次 initialize() 会在流程完成时 resolve(而不是抛错),并且移除去重分支后该测试会变红。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred to the follow-up queue — verified real, fix outside this PR's footprint. Thread left open.

Re-traced at this head and your attribution holds exactly: this.initialized = true flips at config.ts:3008, before initializeOnce() is assigned at :3010 and awaited at :3012, with initializationSettled set only in the finally at :3015. initializationPromise / initializationSettled are private (:2418, :2420) and their only readers are the shutdown internals at :5908-5963, so a concurrent caller has no flight to join and grows its own bounded poll — this one being the second copy after commands-dispatch's registry self-heal.

The dedupe you propose lives in packages/core/src/config/**, a workspace this PR never touches and which AGENTS.md gates as maintainer-only core infrastructure, so it is recorded in the deferred-findings queue for a maintainer to schedule rather than carried here. The caller-side loop stays as the interim fix, as you allow. For the queue: landing it would also delete R2-3's duplicated budget pair, since neither caller would need a window to poll.

中文说明

已延后到后续处理队列 —— 已确认真实存在,修复位置超出本 PR 的 footprint。线程保持开启。

在当前 head 上重新追踪,你的根因归属完全成立:this.initialized = trueconfig.ts:3008 处翻转,早于 initializeOnce():3010 被赋值、在 :3012 被 await,而 initializationSettled 只在 :3015finally 中置位。initializationPromise / initializationSettled 是私有的(:2418:2420),其唯一读者是 :5908-5963 的 shutdown 内部逻辑,因此并发调用方没有可加入的初始化流程,只能各自长出一个有界轮询 —— 本处正是继 commands-dispatch 注册表自愈之后的第二份拷贝。

你提出的去重位于 packages/core/src/config/**,这是本 PR 从未触碰的 workspace,且 AGENTS.md 将其列为仅维护者可改的核心基础设施,因此它被记录进延后处理队列交由维护者排期,而不在这里承载。正如你所允许的,调用方侧的循环作为临时修复保留。给队列的补充:该修复一旦落地,也会顺带删掉 R2-3 中重复的预算常量对,因为两个调用方都不再需要可轮询的时间窗。

const chatDeadline = Date.now() + STARTUP_CHAT_WAIT_MS;
const chatReady = () =>
client.isInitialized() &&
client.getChat().getGenerationConfig().tools !== undefined;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R2-5: The readiness marker is a cross-package contract on LlmChat.generationConfig.tools that nothing in packages/core pins. Every test in this PR injects a fake client, so the suite cannot detect the contract drifting: llm-chat.test.ts has zero references to setTools, and client.test.ts's setTools tests stub chat.setTools out (8 sites — e.g. client.test.ts:2148/2321/2412), so the real write at llm-chat.ts:5163 is untested at its source. If a future core refactor stops writing tool declarations into generationConfig.tools, every test added here stays green — the fakes return whatever the tests set — while in production chatReady() stays false for every session: the first turn of every OpenTUI session silently stalls the full 15s budget and then sends anyway.

Witness:

Sweep over the test corpus at HEAD:
llm-chat.test.ts references to setTools: 0
llm-chat.test.ts + client.test.ts references to getGenerationConfig: 0
client.test.ts chat.setTools stub sites: 8

Pin the contract where it lives — e.g. in llm-chat.test.ts, after chat.setTools([{ functionDeclarations: [] }]), assert chat.getGenerationConfig().tools is defined (or the equivalent assertion on the real chat after client.initialize(), with chat.setTools left unmocked). Removing the this.generationConfig.tools = tools write at llm-chat.ts:5163 must make the new test red.

中文说明

[Suggestion] R2-5: 这个就绪标记是对 LlmChat.generationConfig.tools 的跨包契约,而 packages/core 中没有任何测试钉住它。本 PR 的所有测试都注入假客户端,因此该套件无法发现契约漂移:llm-chat.test.tssetTools 的引用为零,client.test.ts 中的 setTools 测试把 chat.setTools 桩掉了(8 处 —— 例如 client.test.ts:2148/2321/2412),于是 llm-chat.ts:5163 处的真实写入在其源头无人测试。如果未来 core 的重构不再把工具声明写入 generationConfig.tools,这里新增的所有测试仍会是绿的 —— 假客户端返回测试设置的任何值 —— 而生产中 chatReady() 会在每个会话里永远为 false:每个 OpenTUI 会话的第一轮对话都会静默卡满 15 秒预算,然后照样发送。

证据:

对 HEAD 测试语料的扫描:
llm-chat.test.ts 中 setTools 的引用数:0
llm-chat.test.ts + client.test.ts 中 getGenerationConfig 的引用数:0
client.test.ts 中 chat.setTools 的桩点数:8

建议在契约所在处钉住它 —— 例如在 llm-chat.test.ts 中,chat.setTools([{ functionDeclarations: [] }]) 之后断言 chat.getGenerationConfig().tools 已定义(或在 client.initialize() 之后对真实 chat 做等价断言,且不 mock chat.setTools)。移除 llm-chat.ts:5163this.generationConfig.tools = tools 写入后,该新测试必须变红。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred to the follow-up queue — verified real, fix outside this PR's footprint. Thread left open.

Re-ran your sweep at this head and it holds: llm-chat.test.ts has 0 references to setTools, neither it nor client.test.ts references getGenerationConfig, and llm-chat.ts:5163 (this.generationConfig.tools = tools) is the only writer of that field in packages/core. Every test added here injects a fake client that returns whatever the test set, so a core refactor that stopped writing tools would leave chatReady() permanently false — stalling the first turn of every OpenTUI session for the full budget and then sending anyway — with this whole suite still green.

The assertion belongs in packages/core/src/core/llm-chat.test.ts, a workspace this PR never touches, so it is queued for a maintainer rather than expanding this round into a second workspace. Worth recording that the maintainer's own local verification on this PR (§7(d)) independently ranks this the residual "with teeth long-term" and prices it at one assertion after setTools(). Until it lands, the marker's soundness rests on llm-chat.ts:5163 staying the sole writer, which is what this round's expiry gate also assumes.

中文说明

已延后到后续处理队列 —— 已确认真实存在,修复位置超出本 PR 的 footprint。线程保持开启。

在当前 head 上重跑了你的扫描,结论成立:llm-chat.test.tssetTools 的引用为 0,它与 client.test.tsgetGenerationConfig 的引用也都为 0,而 llm-chat.ts:5163this.generationConfig.tools = tools)是 packages/core 中该字段唯一的写入处。这里新增的每个测试都注入假客户端、返回测试自己设置的值,因此一旦 core 的重构不再写入 toolschatReady() 就会永远为 false —— 每个 OpenTUI 会话的第一轮都会卡满整个预算、然后照样发送 —— 而本套测试仍然全绿。

该断言应写在 packages/core/src/core/llm-chat.test.ts,这是本 PR 从未触碰的 workspace,因此交由维护者排期,而不是把本轮扩展进第二个 workspace。值得记录的是:本 PR 上维护者自己的本地验证(§7(d))也独立地把这一条列为长期来看「最有分量」的残留问题,并估计其成本只是 setTools() 之后的一行断言。在它落地之前,该标记的可靠性取决于 llm-chat.ts:5163 仍是唯一写入处 —— 这也正是本轮新增的超时门禁所依赖的前提。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed.

4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R2-2 silent tools-less send on expiry fall-through — already reported (comment 3939191924)
  • R2-3 duplicated startup-wait budget pair — already reported (comment 3939191927)
  • R2-4 second hand-rolled poll for the unjoinable Config.initialize() flight — already reported (comment 3939191932)
  • R3-1 silent startup wait lacks a debug breadcrumb — already recorded in round 2's deferral list (review 5119444960)

1 candidate finding(s) this round's reviewers re-derived matched entries already carried on this PR and were set aside before verification (R2-1) — a matched posted finding is ruled in the previous-round status as always, and a matched deferral stays on the standing deferral record.

Test Plan (not a blocker): Tests 19 passed — this review observed 28582 passed; 15 passed — this review observed 28582 passed; 4 passed — this review observed 28582 passed; 7 passed — this review observed 28582 passed; 44 passed — this review observed 28582 passed; and 1 more.

中文说明

已审查。

本轮确认的 4 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

本轮评审重新推导出的 1 条候选发现与本 PR 已携带的条目匹配,已在验证前搁置(R2-1)——被匹配的已发布条目照常在上一轮状态区裁定,被匹配的延后条目仍保留在延后清单记录中。

Test Plan(非阻断):Tests 19 passed — this review observed 28582 passed; 15 passed — this review observed 28582 passed; 4 passed — this review observed 28582 passed; 7 passed — this review observed 28582 passed; 44 passed — this review observed 28582 passed; and 1 more。

— qwen3.8-max via Qwen Code /review (v0.23.0)

@wenshao

wenshao commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Maintainer verification — built and driven locally

I built both arms from this PR head (35975ed3) and its merge-base (e3d26283), drove the real CLI bundle under bun with the OpenTUI renderer, and reproduced both the CI failure and the user-facing bug before checking the fix. Recommend merge. One non-blocking one-liner is worth taking first; I validated it end to end and it is quoted below.

Everything below is a run on this machine, not a re-reading of the description.

Setup. Linux, 16 vCPU, Node 22.22.2, bun 1.3.14 — the version the leg pins. Two bundles from one tree: BASE = this PR's diff reverse-applied (both files), PR = the head as it stands. They are genuinely different builds — the renderer chunk hashes differ (start-opentui-ui-X2XREUAN vs -EOVDYME3) and chatDeadline occurs 0× in the BASE bundle, 2× in the PR bundle.


1. The CI failure reproduces with no artificial lever at all

The description says the leg needs a loaded runner. The cleanest honest stand-in is to give the run one core. Same command the leg uses, taskset -c 2, on the spec that types first:

E2E under one CPU

arm wall slowest case Chat not initialized in the run log
BASE 83.23s holds a slash command back mid-turn… 70809ms (retry x2) 2
PR 19.24s same case, 6232ms, first attempt 0

The two failed attempts print exactly the screen the description quotes, from the repo's own harness rather than from my probe:

> Start the review.
✖︎ Chat not initialized
midTurn: false
All tool calls found: []

Both arms still exit 0 only because integration-tests/vitest.config.ts sets retry: 2. A run whose three attempts all land inside the window is the red leg the issue reports — and it explains the detector's missing test name: the failure is a retried-then-passed case, not a reported one.

Unpinned on 16 cores both arms are green (Test Files 9 passed | 1 skipped, Tests 17 passed | 1 skipped), so on an idle box the leg is not a discriminator. That matches the PR's claim rather than contradicting it. context-compress-interactive.test.ts is excluded from both arms — it needs live model credentials this environment does not have.

2. The user-facing bug, reproduced deterministically

A stdio MCP server whose initialize reply is delayed 6s, plus QWEN_CODE_LEGACY_MCP_BLOCKING=1 so tool-registry discovery is awaited inside Config.initialize(), before llmClient.initialize()startChat(). Supported configuration only; no patched code. Real CLI, real renderer, scripted model server recording every request.

Startup window A/B

arm submitted at first request submit → request tool declarations outcome
BASE 2269ms never ✖︎ Chat not initialized, 0 requests
PR 2270ms 8337ms 6067ms 30 model reply rendered

The prompt really is lost on base — the model server records nothing.

3. It also closes a second, entirely silent defect

This is the direction R1-1 raised and the author fixed in round 1, and it is worth stating that it reproduces in the product, not only in a stand-in. startChat() assigns this.chat (client.ts:2261) before awaiting the SessionStart hook and setTools() (client.ts:2290, 2309), so a prompt submitted in that sub-window finds a chat with no declarations. Lever: a SessionStart hook that sleeps 12s.

arm submitted at first request submit → request tools in the first request
BASE 4773ms 4855ms 82ms absent — the session's first prompt reaches the model with zero tool declarations, no error, turn looks normal
PR 4774ms 14285ms 9511ms 30

An existence-only gate would have shipped this. The tools !== undefined marker is what prevents it, and llm-chat.ts:5162-5163 is its only writer, so the marker is sound as long as that stays true (see R2-5 below).

4. Nothing is paid when the session is ready

Three runs per arm, no lever, submit → first request:

run 1 run 2 run 3
BASE 77ms 73ms 78ms
PR 80ms 77ms 77ms

The predicate is evaluated once and the loop body never runs. Confirmed.

5. Root cause chain, re-checked against this head

Every link independently:

  • config.ts:3008 sets initialized = true; config.ts:3012 awaits the work — a concurrent caller really does get Config was already initialized before startChat() runs.
  • opentui-app-shell.tsx:331dispatcher.loadCommands()slash-dispatch.ts:59 await config?.initialize() owns that flight, from the same mount that already has the composer on screen.
  • client.ts:652-660getChat() throws when chat is undefined and isInitialized() is exactly chat !== undefined. grep confirms this.chat = has one assignment site (client.ts:2261).
  • AppContainer.tsx:347/369 gate ink's input on isConfigInitialized; input-prompt.tsx has no equivalent. The renderer asymmetry is real.
  • initializationPromise / initializationSettled are private with no accessor (config.ts:2418-2420) — the stated reason for a bounded poll instead of awaiting the flight holds.
  • The claim that no other OpenTUI code reads the model session in this window checks out with one clarification: commands-dispatch.ts:776 (load_historysetHistory) would throw the same way, but type: 'load_history' has no producer anywhere in the repo (only the declaration at types.ts:251), so it is unreachable today.

6. The tests are not vacuous

Each condition of the predicate, mutated separately, against live-session.test.ts (47 tests intact):

mutation result witness that caught it
drop client.isInitialized() 2 failed | 45 passed waits for the chat an in-flight startup initialization createsError: Chat not initialized
drop …getGenerationConfig().tools !== undefined 1 failed | 46 passed holds the first send until the startup chat has tool declarations
drop Date.now() < chatDeadline 1 failed | 46 passed reports the client error once the startup chat wait is spentTest timed out in 15000ms
remove the whole wait 2 failed | 45 passed first two cases

Regression sweep: the whole OpenTUI unit tree at this head is 67 files / 1135 tests passed.

7. Residuals — all non-blocking, all reproduced on the real CLI

(a) R2-1, the abort signal — the one item I would take before merge. The oracle is OpenTUI's own behaviour: live-turn.ts submit() echoes a > user row only when the turn is idle; while busy the text goes to the steering queue with no row. So "when does a second prompt appear on screen" measures exactly when the turn released. Never-completing startup, Esc at submit+1.5s, second prompt typed at submit+3.3s:

arm second prompt appears as a > row
BASE submit + 4.8s (the turn had already settled at submit+87ms)
PR as it stands submit + 16.3s — Esc is ignored, the composer stays busy for the whole budget
PR + && !signal?.aborted submit + 4.8s

The suggested one-liner is enough, and it costs nothing else — with it applied I re-ran everything: live-session.test.ts 47 passed; §2's lever 5974ms → 30 tools; §3's lever 9417ms → 30 tools; the 1-CPU E2E green with no retries and zero Chat not initialized.

while (!chatReady() && Date.now() < chatDeadline && !signal?.aborted) {

(b) R2-2, expiry falls through to a tools-less send. Real reproduction: a SessionStart hook sleeping 25s (longer than the budget). The PR waits the full 15133ms and then sends anyway with no tool declarations, and the screen shows an ordinary reply — nothing surfaces. Base does the same thing 15s earlier, so this is an unclosed edge rather than a regression, but the added comment's "surfaces the client's own error" only holds on the no-chat branch.

(c) R1-1 direction 2, fail-fast. Also real: with a startup that cannot finish, Chat not initialized now appears at submit + 15110ms instead of submit + 84ms, and every retry pays it again. Bounded and correctly deferred, but worth knowing it is the price of the fix.

(d) R2-3 / R2-4 / R2-5 are accurate as maintainability observations. R2-5 is the one with teeth long-term: the readiness marker is a cross-package contract on LlmChat.generationConfig.tools and nothing in packages/core pins it, so a future refactor that stops writing it would make chatReady() permanently false and stall the first turn of every session for 15s — with every test in this PR still green. A one-line assertion in llm-chat.test.ts after setTools() would close that cheaply.

8. Merge mechanics

reviewDecision is still CHANGES_REQUESTED and mergeStateStatus is BLOCKED: the round-1 review from qwen-code-ci-bot is the standing verdict because rounds 2 and 3 were COMMENTED, which does not supersede it. Its Critical (R1-1 direction 1) is genuinely closed — §3 is that fix working in the product — so this needs a re-review or a dismissal, not more code.

Verdict

The diagnosis is correct, the fix works on the real renderer, it fixes a second silent defect on the way, it costs nothing on a healthy session, and its tests hold under mutation. Recommend merge, ideally with the one-line !signal?.aborted guard from §7(a), which I verified changes nothing else.

中文版

维护者验证 —— 本地构建并实际运行

我从本 PR head(35975ed3)和它的 merge-base(e3d26283)分别构建了两个产物,用 bun + OpenTUI 渲染器驱动真实的 CLI bundle,先复现 CI 失败与面向用户的缺陷,再验证修复。建议合并。 有一个非阻断的一行改动值得先合入,我已端到端验证,代码见下文。

以下全部是本机的实际运行结果,不是对描述的复述。

环境。 Linux,16 vCPU,Node 22.22.2,bun 1.3.14 —— 与该 leg 锁定的版本一致。两个产物出自同一棵树:BASE = 反向应用本 PR 的 diff(两个文件),PR = 当前 head。两者确实是不同的构建 —— 渲染器 chunk 哈希不同(start-opentui-ui-X2XREUAN-EOVDYME3),且 chatDeadline 在 BASE 产物中出现 0 次、在 PR 产物中出现 2 次。


1. 不用任何人为手段,CI 失败即可复现

描述说这个 leg 需要一台负载较高的 runner。最诚实的替代做法是只给这次运行一个核心。使用该 leg 自己的命令,加上 taskset -c 2,跑那个会立刻输入的 spec:

E2E under one CPU

arm 墙钟时间 最慢用例 运行日志中 Chat not initialized 出现次数
BASE 83.23s holds a slash command back mid-turn… 70809ms(retry x2) 2
PR 19.24s 同一用例,6232ms,第一次尝试即通过 0

两次失败的尝试打印出的正是描述中引用的屏幕内容,而且来自仓库自己的测试工具,不是我的探针:

> Start the review.
✖︎ Chat not initialized
midTurn: false
All tool calls found: []

两个 arm 最终都以 0 退出,只是因为 integration-tests/vitest.config.ts 设置了 retry: 2。当三次尝试全部落进这个时间窗时,就是 issue 报告的那个红色 leg —— 这也解释了检测器为什么拿不到测试名:失败的是「重试后通过」的用例,而不是被报告出来的用例。

不做 CPU 绑定时,16 核上两个 arm 都是绿的(Test Files 9 passed | 1 skippedTests 17 passed | 1 skipped),所以在空闲机器上这个 leg 并不具备区分力。这与 PR 的说法一致,而非相反。context-compress-interactive.test.ts 在两个 arm 中都被排除 —— 它需要本环境没有的真实模型凭据。

2. 面向用户的缺陷,确定性复现

一个 stdio MCP server,其 initialize 响应延迟 6 秒,再加上 QWEN_CODE_LEGACY_MCP_BLOCKING=1,使工具注册表发现被 Config.initialize() 内部、于 llmClient.initialize()startChat() 之前 等待。全部使用受支持的配置,没有改动任何代码。真实 CLI、真实渲染器、记录每一次请求的脚本化模型服务器。

Startup window A/B

arm 提交时刻 首个请求 提交 → 请求 工具声明 结果
BASE 2269ms 从未发出 ✖︎ Chat not initialized0 个请求
PR 2270ms 8337ms 6067ms 30 正常渲染出模型回复

在 base 上 prompt 确实丢失了 —— 模型服务器什么都没收到。

3. 它还顺带关闭了第二个完全静默的缺陷

这正是 R1-1 提出、作者在第 1 轮修掉的那个方向;值得说明的是:它在产品中确实可复现,而不只存在于替身对象里。startChat() 先赋值 this.chatclient.ts:2261),之后 才 await SessionStart hook 与 setTools()client.ts:22902309),因此在这个子时间窗内提交的 prompt 会拿到一个没有工具声明的 chat。手段:一个 sleep 12 秒的 SessionStart hook。

arm 提交时刻 首个请求 提交 → 请求 首个请求中的 tools
BASE 4773ms 4855ms 82ms 不存在 —— 该会话的第一个 prompt 以零工具声明发给模型,没有报错,这一轮看起来完全正常
PR 4774ms 14285ms 9511ms 30

只判断 chat 是否存在的写法会把这个缺陷放行。tools !== undefined 这个标记正是拦住它的东西,而 llm-chat.ts:5162-5163 是它唯一的写入处,所以只要这一点不变,该标记就是可靠的(参见下文 R2-5)。

4. 会话就绪时没有任何额外开销

每个 arm 各跑 3 次,无任何手段,提交 → 首个请求:

第 1 次 第 2 次 第 3 次
BASE 77ms 73ms 78ms
PR 80ms 77ms 77ms

判据只求值一次,循环体从不执行。已确认。

5. 因果链,对照当前 head 逐条复核

逐个独立核对:

  • config.ts:3008 置位 initialized = trueconfig.ts:3012 才 await 实际工作 —— 并发调用方确实会在 startChat() 运行之前拿到 Config was already initialized
  • opentui-app-shell.tsx:331dispatcher.loadCommands()slash-dispatch.ts:59 await config?.initialize() 持有那次 flight,而这个 mount 时机的输入框已经在屏幕上了。
  • client.ts:652-660 —— chat 未定义时 getChat() 抛错,isInitialized() 恰好就是 chat !== undefined。grep 确认 this.chat = 只有 一处 赋值(client.ts:2261)。
  • AppContainer.tsx:347/369 让 ink 的输入受 isConfigInitialized 门控;input-prompt.tsx 没有对应物。渲染器之间的不对称是真实存在的。
  • initializationPromise / initializationSettled 是私有的且没有访问器(config.ts:2418-2420)—— PR 中「只能有界轮询而不能 await 那次 flight」的理由成立。
  • 「这个时间窗内 OpenTUI 其他代码不会读取模型会话」这一说法基本成立,需补充一点:commands-dispatch.ts:776load_historysetHistory)会以同样方式抛错,但 type: 'load_history' 在整个仓库中 没有任何生产者(只有 types.ts:251 的类型声明),因此今天不可达。

6. 测试不是空转的

针对判据的每个条件分别做变异,对照 live-session.test.ts(原状 47 个用例):

变异 结果 捕获它的见证用例
去掉 client.isInitialized() 2 failed | 45 passed waits for the chat an in-flight startup initialization creates —— Error: Chat not initialized
去掉 …getGenerationConfig().tools !== undefined 1 failed | 46 passed holds the first send until the startup chat has tool declarations
去掉 Date.now() < chatDeadline 1 failed | 46 passed reports the client error once the startup chat wait is spent —— Test timed out in 15000ms
整体移除该等待 2 failed | 45 passed 上述前两个用例

回归扫描:当前 head 上整个 OpenTUI 单测树 67 个文件 / 1135 个用例全部通过

7. 残留问题 —— 均非阻断,且都已在真实 CLI 上复现

(a) R2-1,abort 信号 —— 这是我建议合并前先处理的一条。 判据用的是 OpenTUI 自身的行为:live-turn.tssubmit() 只在这一轮空闲时才回显一行 > 用户消息;忙碌时文本会进入 steering 队列且不产生任何行。因此「第二个 prompt 何时出现在屏幕上」恰好度量了这一轮何时释放。场景为「初始化永不完成」,在提交后 1.5 秒按 Esc,提交后 3.3 秒输入第二个 prompt:

arm 第二个 prompt 作为 > 行出现的时刻
BASE 提交 + 4.8 秒(该轮在提交 + 87ms 时就已结束)
PR 当前状态 提交 + 16.3 秒 —— Esc 被无视,输入框在整个预算期间保持忙碌
PR + && !signal?.aborted 提交 + 4.8 秒

评审建议的这一行就足够了,而且不会带来其他代价 —— 打上它之后我把所有验证重跑了一遍:live-session.test.ts 47 通过;第 2 节的手段 5974ms → 30 个工具;第 3 节的手段 9417ms → 30 个工具;单核 E2E 全绿、无重试、零次 Chat not initialized

while (!chatReady() && Date.now() < chatDeadline && !signal?.aborted) {

(b) R2-2,预算耗尽后 fall through 成无工具发送。 真实复现:一个 sleep 25 秒的 SessionStart hook(长于预算)。PR 会等满 15133ms,然后照样发出 不带任何工具声明 的请求,屏幕上显示的是一次普通回复 —— 什么都不会暴露出来。base 只是把同样的事提前 15 秒做了,所以这是一个未关闭的边角,而不是回归;但新增注释里「surfaces the client's own error」只在「没有 chat」那条分支上成立。

(c) R1-1 方向 2,快速失败。 同样真实:当启动无法完成时,Chat not initialized 现在出现在 提交 + 15110ms,而不是 提交 + 84ms,且每次重试都要再付一次。它有界,也是被有意识地推迟的,但值得知道这是修复的代价。

(d) R2-3 / R2-4 / R2-5 作为可维护性观察都是准确的。长期来看最有分量的是 R2-5:就绪标记是对 LlmChat.generationConfig.tools 的跨包契约,而 packages/core 中没有任何测试钉住它;未来某次重构若不再写入它,chatReady() 会永远为 false,每个会话的第一轮都会卡满 15 秒 —— 而本 PR 的所有测试仍然全绿。在 llm-chat.test.tssetTools() 之后加一行断言即可低成本地堵上。

8. 合并流程

reviewDecision 仍是 CHANGES_REQUESTEDmergeStateStatusBLOCKEDqwen-code-ci-bot 第 1 轮的评审仍是有效裁定,因为第 2、3 轮是 COMMENTED,并不会覆盖它。它的 Critical(R1-1 方向 1)确实已经关闭 —— 第 3 节就是这个修复在产品中生效的证据 —— 所以需要的是重新评审或撤销该评审,而不是继续改代码。

结论

诊断正确,修复在真实渲染器上有效,顺带修掉了第二个静默缺陷,在健康会话上零开销,其测试在变异下站得住。建议合并,最好同时带上第 7(a) 节那一行 !signal?.aborted 守卫 —— 我已验证它不改变其他任何行为。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R3-1 silent startup wait lacks a debug breadcrumb — already recorded in round 2's deferral list (review 5119444960)

Not reviewed: reverse audit — stopped before round 2 by the review time budget.

Not explored to full depth (tool budget reached): "agent 1a": trace OpenTUI boot's handling of a rejected config.initialize() flight to confirm the failed-flight session state of finding 2 remains prompt-accepting.

Not reviewed: reverse audit — stopped before round 1 by the review time budget.

Test Plan (not a blocker): Tests 19 passed — this review observed 28582 passed; 15 passed — this review observed 28582 passed; 4 passed — this review observed 28582 passed; 7 passed — this review observed 28582 passed; 44 passed — this review observed 28582 passed; and 1 more.

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查(原文为英文):reverse audit — stopped before round 2 by the review time budget.

未探索到全部深度(达到工具调用预算):"agent 1a"trace OpenTUI boot's handling of a rejected config.initialize() flight to confirm the failed-flight session state of finding 2 remains prompt-accepting

未审查:反向审计——评审时间预算不足,未能开始第 1 轮。

Test Plan(非阻断):Tests 19 passed — this review observed 28582 passed; 15 passed — this review observed 28582 passed; 4 passed — this review observed 28582 passed; 7 passed — this review observed 28582 passed; 44 passed — this review observed 28582 passed; and 1 more。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/cli/src/ui/opentui/live-session.ts Outdated
The wait this branch added is the first multi-second window between an
OpenTUI submit and its send, and it consulted neither the turn's abort
signal nor what its own expiry means.

An Esc pressed inside it was ignored for the whole budget, and nothing
between the loop and sendMessageStream re-checks the signal for text-only
input — applyPromptVisionBridge returns on !hasImageParts before its
abort check, and @-expansion is skipped for non-@ text — so a prompt the
user had already cancelled still fired its UserPromptSubmit hooks and
pushed a history entry. Poll the signal in the loop condition and throw
once it aborts: the throw is what routes the turn through runTurn's
catch, which settles on abort.signal.aborted, where a clean generator
return would have fired onComplete instead.

Expiry also fell through to a send whenever the flight had assigned the
chat but not reached setTools(), so a SessionStart hook slower than the
budget produced a turn answered with zero tool declarations and nothing
on screen to say so. Fail that case with a named error, gated on
isInitialized() so a config that never created a chat still surfaces the
client's own "Chat not initialized".

One existing test changed shape, not assertion: "skips steering when the
turn is aborted" pre-aborted its controller before calling the generator,
a state the production caller cannot reach and which now settles before
the send. It aborts inside the fake stream instead, where a real Esc
arrives. Measured on the pre-round source, neither version witnesses the
tool-response boundary guard — both pass with `|| abort.aborted` deleted
— so no coverage moved.
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator Author

🤖 Addressed the latest review feedback (round 2/10). What changed, and what I pushed back on: · 已处理最新评审反馈(第 2/10 轮)。改动内容与我反驳保留之处如下:

Review round summary — PR #11046 (issue #11042)

Commit: d6c81109d2fix(cli): settle the startup-chat wait on abort and on expiry (#11042)
Changed: packages/cli/src/ui/opentui/live-session.ts, packages/cli/src/ui/opentui/live-session.test.ts — 2 files changed, 96 insertions(+), 6 deletions(-).
--conflict false, so no base merge. No footprint expansion: both files are already this PR's own, and nothing outside packages/cli was touched. No settings source changed, so no schema regeneration.

Dispositions

[rc:3939680672] R2-1 [Critical] — an abort during the startup wait walks a cancelled prompt into the send path → IMPLEMENTED

Reproduced before implementing anything. Every link of the claim checked at this head:

  • the loop condition was !chatReady() && Date.now() < chatDeadline — it never consulted the signal;
  • applyPromptVisionBridge's !hasImageParts(parts) early return (live-session.ts:379) does sit before its signal.aborted check (:384), and @-expansion is gated on isAtCommand(prompt), so text-only input really has no abort gate between the loop and the send;
  • live-turn.ts:215 passes abort.signal; Esc routes to interrupt()abortRef.current?.abort() (:313-315);
  • runTurn's catch keys on abort.signal.aborted (:241-248) and onComplete fires only after a successful drain (:236-238), so the abort has to surface as a rejection — a bare return after the loop would have fired onComplete.

Both halves of your suggestion block are in: && !signal?.aborted in the loop condition plus signal?.throwIfAborted(); after it. Your note that the round-2 loop-condition-only proposal is insufficient is correct, and I measured it rather than taking it on faith — probe P2 below removes only the throw, and the witness then reports promise resolved "[ …(2) ]" instead of rejecting, i.e. the cancelled turn walks all the way into the send and completes as a success. Both constraints you named hold: every check is ?.-safe for the optional signal, and the turn settles through runTurn's catch rather than as a silent completion. signal?.throwIfAborted() is already the house idiom (packages/cli/src/serve/server/session-list.ts alone uses it at 28 sites, including inside poll loops).

Witness added: settles an Esc pressed during the startup wait without sending — never-ready chat (isInitialized: () => false) under fake timers, the passed controller aborted at virtual t=500ms, asserting the generator has settled and rejected by t=1000ms (≈14s before the budget could expire) and that sendMessageStream was never called.

[rc:3939191918], the original Suggestion in this thread, is resolved by the same change.

[rc:3939191924] R2-2 — expiry falls through to a silent tools-less send → IMPLEMENTED

I first declined this and then reversed on the evidence, so the reasoning is worth recording. Premise re-verified at this head: this.chat = chat (client.ts:2261) precedes the awaited SessionStart hook (:2290), its context apply (:2299) and setTools() (:2309), so "chat assigned, tools undefined" is reachable at expiry — and your §7(b) reproduction on the real CLI (a 25s SessionStart hook → a 15133ms wait → a send with no tool declarations → "nothing surfaces") is execution evidence, not inference. What settled it is that the PR's own comment above the constant promises expiry "still reports its own error instead of hanging the prompt": the half-built branch was the one case where that promise was false.

Took the explicit-failure option rather than pinning the fall-through as intentional:

if (client.isInitialized() && !chatReady()) {
  throw new Error(
    `Timed out after ${STARTUP_CHAT_WAIT_MS}ms waiting for the startup chat to become ready`,
  );
}

Gated on client.isInitialized() precisely so the no-chat branch keeps falling through to the client's own Chat not initialized, as you asked — reports the client error once the startup chat wait is spent passes unmodified and still witnesses that path (probe P5 below is what proves the gate clause is load-bearing). Placed after throwIfAborted() so a cancelled turn always surfaces the abort, never a timeout. Failing beats sending here: a tools-less turn is a wrong answer presented as a normal one, while the throw names the cause and a retry after the hook finishes succeeds.

Witness added: fails the turn when the startup wait expires on a chat with no toolsisInitialized: () => true with tools permanently undefined, fake timers advanced past the budget, asserting the drain rejects on the timeout message and sendMessageStream was never called.

[rc:3939191927] R2-3 — STARTUP_CHAT_* duplicates STARTUP_REGISTRY_*DECLINED structurally, prose fixed

Both literal pairs confirmed. Single-sourcing has no clean host: packages/cli/src/ui/opentui/ has no shared-constants module, so it means either a new file whose entire content is two numbers, or importing one feature module into the other for them — either direction buys a real dependency for a coincidental equality. The two waits also bound different resources through different probes with different degradations (config.getSkillManager?.() → "Unknown command", vs chat + generationConfig.tools → a dropped prompt), and your witness records not run. The half that was a defect is the prose asserting an equality nothing enforces, so the comment no longer claims "the same budget commands-dispatch gives its registry self-heal" (net −1 line). Full reason on the thread; the real consolidation is R2-4's.

[rc:3939191932] R2-4 and [rc:3939191936] R2-5 → DEFERRED to the follow-up queue

Both verified real at this head and both recorded in deferred-findings.json, threads left open with the evidence replied in place. R2-4's dedupe lives in packages/core/src/config/** (guard flips at config.ts:3008 before the flight is assigned at :3010 and awaited at :3012; initializationPromise/initializationSettled private at :2418/:2420, read only by the shutdown internals at :5908-5963) — maintainer-gated core infrastructure in a workspace this PR never touches. R2-5's assertion belongs in packages/core/src/core/llm-chat.test.ts (0 setTools references there, 0 getGenerationConfig in either core test file, llm-chat.ts:5163 the sole writer) — same footprint boundary. Carrying either would expand this round into a second workspace.

[rv:5120041433] [CHANGES_REQUESTED] → its only finding, R4-1, is the Critical above

[ic:5549701086] Maintainer verification (@wenshao) — "Recommend merge", one non-blocking one-liner

§7(a), the item flagged as worth taking before merge, is implemented — plus the throwIfAborted() half the round-4 Critical added on top of the one-liner he validated. §7(b) is R2-2, implemented. §7(c) (fail-fast on a startup that cannot finish) is unchanged and still bounded; note R2-2's gate makes the half-built branch more diagnosable, since the message now names the wait instead of the turn silently answering with no tools. §7(d) is R2-3 (declined, prose corrected), R2-4 and R2-5 (both deferred). §8 is merge mechanics — reviewDecision/mergeStateStatus are not mine to change and need a re-review or dismissal, not more code.

Round-2 deferred item (R3-1, no log breadcrumb on the silent wait) → untouched

Listed under Deferred under the convergence posture … recorded, not requested in this round, so it is not actionable here and this round adds no logging.

Failed check: web-shell E2E Smoke (ubuntu-latest Node 22.x)CANCELLED, not actionable

Cancelled rather than failed, and the lane exercises packages/web-shell, which this PR does not touch. The Still-red checks section is empty; Test, Lint & Static, Integration Tests (no-AK, No Sandbox), OpenTUI no-flicker gate and TUI parity snapshots are all SUCCESS on the pre-round head. Integration tests were not re-run this round: the touched behaviour (an abort landing inside the startup wait, and expiry on a half-built chat) has no deterministic lever in that harness, both paths are now unit-witnessed, and both added guards are no-ops when the signal is unaborted and the chat is ready — which is the path the harness drives.

One existing test changed — content evidence

skips steering when the turn is aborted changed setup, not assertion. It pre-aborted its controller before calling the generator; under the fix that state settles the turn before the send (which is the point of the fix), so the test failed with AbortError: This operation was aborted. The production caller cannot reach that state: runTurn builds a fresh controller, and when config is already initialized the turn's own initialize() rejects synchronously, so its catch resumes on a microtask — there is no macrotask boundary between the call and the wait for a keypress to land in. It now aborts inside the fake stream, where a real Esc actually arrives, and still asserts drainSteering was not called.

Measured rather than assumed, because I first believed the change cost coverage:

arm source test result
1 pre-round + || abort.aborted deleted original 1 passed
2 this round + || abort.aborted deleted adapted 1 passed

Neither version witnesses the tool-response boundary guard, so no coverage moved. (An earlier arm appeared to show the original catching that mutation; the AbortError there came from this round's own throwIfAborted(), not from the mutation, and the re-run above isolates it.)

Round mechanics — concurrent run on a shared workdir

/tmp/autofix-review-11046 already held a complete, different set of round outputs when this round went to write its own: an address-summary.md describing commit 3d291b2568 (which does not exist in this repository), plus resolved-comments.txt, comment-replies.json and deferred-findings.json. A second run of this same round is addressing the same feedback from another checkout while sharing this workdir path. Everything in this summary describes the commit this round actually produced, and all four artifacts were overwritten to match it. Flagging it because two runs pushing at the same branch can produce duplicate work, and because a summary read next to the other run's commit would not match.

Verification

Every command below was actually run against the committed tree, in the order the gate re-runs them.

  • npm run buildpassed (exit 0)
  • npm run typecheckpassed (exit 0, 0 error TS lines). First attempt caught a real defect of my own: TS2556 on a spread into the zero-arg fake from oneToolBatchStream; fixed and re-run clean.
  • npm run lintpassed (exit 0)
  • npx prettier --check on both touched files — passed ("All matched files use Prettier code style!")
  • npx vitest run src/ui/opentui/live-session.test.ts src/ui/opentui/live-turn.test.ts (touched modules, packages/cli) — 61 passed (2 files)
  • npx vitest run src/ui/opentui (whole OpenTUI regression sweep, packages/cli) — 67 files / 1137 tests passed (exit 0)
  • Integration tests — not run, reason above
  • npm run generate:settings-schema — not applicable, no settings source changed

Mutation probes — every guard this round adds has its own witness, each confirmed red with the guard removed and green with it restored (live-session.test.ts, 49 tests intact):

probe mutation witness result
P1 drop && !signal?.aborted from the loop condition settles an Esc pressed during the startup wait without sending 1 failedexpected false to be true (never settles early)
P2 drop signal?.throwIfAborted(); same 1 failedpromise resolved "[ …(2) ]" instead of rejecting (cancelled prompt reaches the send)
P4 drop the whole half-built-chat expiry gate fails the turn when the startup wait expires on a chat with no tools 1 failedpromise resolved … instead of rejecting (tools-less fall-through)
P5 drop only client.isInitialized() && from the gate reports the client error once the startup chat wait is spent (pre-existing) 1 failedexpected … 'Chat not initialized' but got 'Timed out after 15000ms…' (gate would hijack the no-chat branch)

Red-first evidence for the Critical itself, before any source change: the new witness on the unmodified PR head ran 1 failed | 47 passed (48), expected false to be true — the abort was ignored and the generator never settled.

中文说明

审查轮次总结 —— PR #11046(issue #11042

提交:d6c81109d2 —— fix(cli): settle the startup-chat wait on abort and on expiry (#11042)
改动文件:packages/cli/src/ui/opentui/live-session.tspackages/cli/src/ui/opentui/live-session.test.ts —— 2 个文件改动,96 行新增、6 行删除。
--conflict false,因此未合并 base。无 footprint 扩张:两个文件本就属于本 PR,packages/cli 之外未被触碰。未改动任何 settings 源文件,因此无需重新生成 schema。

各项裁定

[rc:3939680672] R2-1 [Critical] —— 启动等待期间的 abort 会把一个已取消的 prompt 带进发送路径 → 已实现

动手之前先复现。该结论的每个环节都在当前 head 上核对过:

  • 循环条件原本是 !chatReady() && Date.now() < chatDeadline —— 从不查询信号;
  • applyPromptVisionBridge!hasImageParts(parts) 提前返回(live-session.ts:379)确实位于其 signal.aborted 检查(:384之前,而 @ 展开以 isAtCommand(prompt) 为门禁,因此纯文本输入在循环与发送之间确实没有任何 abort 门禁;
  • live-turn.ts:215 传入 abort.signal;Esc 路由到 interrupt()abortRef.current?.abort():313-315);
  • runTurn 的 catch 以 abort.signal.aborted 为判据(:241-248),而 onComplete 只在整个 drain 成功之后触发(:236-238),因此 abort 必须以抛出错误的形式浮现 —— 循环后裸 return 会触发 onComplete

你建议块中的两半都已落地:循环条件中的 && !signal?.aborted加上其后的 signal?.throwIfAborted();。你指出「第二轮只改循环条件的方案不够」是正确的,而且我是实测确认、并非照单接受 —— 下面的探针 P2 只移除这个 throw,见证用例随即报告 promise resolved "[ …(2) ]" instead of rejecting,也就是被取消的这一轮一路走进发送路径并以成功收尾。你点出的两个约束都成立:所有检查对可选的 signal 都使用 ?.;这一轮经由 runTurn 的 catch 落定,而不是静默完成。signal?.throwIfAborted() 本就是本仓库的既有写法(packages/cli/src/serve/server/session-list.ts 一个文件就有 28 处使用,包括在轮询循环内部)。

新增见证用例:settles an Esc pressed during the startup wait without sending —— fake timers 下 chat 永不就绪(isInitialized: () => false),在虚拟时间 t=500ms 时 abort 传入的 controller,断言 generator 在 t=1000ms 前已落定且以错误结束(比预算耗尽早约 14 秒),并断言 sendMessageStream 从未被调用。

[rc:3939191918],即该线程中最初的那条 Suggestion,由同一处改动解决。

[rc:3939191924] R2-2 —— 预算耗尽后 fall through 成一次静默的无工具发送 → 已实现

我最初拒绝了这一条,随后基于证据改变了判断,因此值得把推理记录下来。前提在当前 head 上重新核实:this.chat = chatclient.ts:2261)先于被 await 的 SessionStart hook(:2290)、其上下文写入(:2299)与 setTools():2309),因此「chat 已赋值、tools 为 undefined」在预算耗尽时是可达状态 —— 而你 §7(b) 在真实 CLI 上的复现(25 秒的 SessionStart hook → 等待 15133ms → 一次不带任何工具声明的发送 → "nothing surfaces")属于执行证据,而非推断。真正让我改变判断的是:本 PR 自己在常量上方写的注释承诺超时会「still reports its own error instead of hanging the prompt」,而半建成的 chat 这条分支正是该承诺不成立的唯一情形。

采纳了「显式失败」而不是「把 fall through 钉成有意行为」:

if (client.isInitialized() && !chatReady()) {
  throw new Error(
    `Timed out after ${STARTUP_CHAT_WAIT_MS}ms waiting for the startup chat to become ready`,
  );
}

client.isInitialized() 作为门禁,正是为了让「没有 chat」这条分支继续 fall through 到客户端自己的 Chat not initialized,如你要求 —— reports the client error once the startup chat wait is spent 未经修改即通过,并继续见证该路径(下面的探针 P5 正是证明这个门禁子句承重的证据)。它放在 throwIfAborted() 之后,因此被取消的一轮永远浮现 abort,而不会浮现超时。此处失败优于发送:一次无工具的回答是把错误答案伪装成正常答案,而抛错会说明原因,且 hook 结束后的重试可以成功。

新增见证用例:fails the turn when the startup wait expires on a chat with no tools —— isInitialized: () => truetools 恒为 undefined,fake timers 推进超过预算,断言 drain 以超时信息结束,且 sendMessageStream 从未被调用。

[rc:3939191927] R2-3 —— STARTUP_CHAT_*STARTUP_REGISTRY_* 重复 → 结构性部分不予采纳,注释文字已修复

两对字面量均已确认。没有做单一来源化:packages/cli/src/ui/opentui/ 下没有共享常量模块,因此这意味着要么新建一个内容只有两个数字的文件,要么为了这两个数字把一个功能模块 import 进另一个 —— 任一方向都是为了一次偶然的相等而引入真实依赖。这两个等待还通过不同的判据约束不同的资源、并有不同的降级表现(config.getSkillManager?.() → "Unknown command",对比 chat + generationConfig.tools → prompt 被丢弃),而你的证据记录了 not run。真正构成缺陷的那一半是那段断言了「无人强制的相等关系」的注释,因此注释不再宣称 "the same budget commands-dispatch gives its registry self-heal"(净减 1 行)。完整理由见该线程;真正的收敛属于 R2-4。

[rc:3939191932] R2-4 与 [rc:3939191936] R2-5 → 已延后到后续处理队列

两条都在当前 head 上确认为真实存在,都已记录进 deferred-findings.json,线程保持开启并在原处回复了证据。R2-4 的去重位于 packages/core/src/config/**(守卫在 config.ts:3008 翻转,早于 :3010 赋值与 :3012 await;initializationPromise/initializationSettled:2418/:2420 为私有,仅被 :5908-5963 的 shutdown 内部读取)—— 属于维护者门禁管辖的核心基础设施,且在本 PR 从未触碰的 workspace 中。R2-5 的断言应写在 packages/core/src/core/llm-chat.test.ts(该文件对 setTools 的引用为 0,两个 core 测试文件对 getGenerationConfig 的引用都为 0,llm-chat.ts:5163 是唯一写入处)—— 同样是 footprint 边界问题。承载任何一条都会把本轮扩张进第二个 workspace。

[rv:5120041433] [CHANGES_REQUESTED] → 其唯一发现 R4-1 就是上面的 Critical

[ic:5549701086] 维护者验证(@wenshao)—— "Recommend merge",另有一条非阻断的一行改动

§7(a),即他标注为合并前值得先处理的那一条,已实现 —— 并且额外带上了第 4 轮 Critical 在他验证过的那一行之外补充的 throwIfAborted() 一半。§7(b) 即 R2-2,已实现。§7(c)(启动无法完成时快速失败)未改动,仍然有界;注意 R2-2 的门禁让半建成 chat 这条分支可诊断,因为错误信息现在会指名这个等待,而不是让这一轮静默地在没有工具的情况下作答。§7(d) 即 R2-3(不予采纳,注释已修正)、R2-4 与 R2-5(都已延后)。§8 属于合并流程 —— reviewDecision/mergeStateStatus 不由我改动,需要的是重新评审或撤销该评审,而不是继续改代码。

第 2 轮延后项(R3-1,静默等待缺少日志面包屑)→ 未触碰

它列在 Deferred under the convergence posture … recorded, not requested in this round 之下,因此本轮不要求处理,本轮也未新增任何日志。

失败检查:web-shell E2E Smoke (ubuntu-latest Node 22.x) —— CANCELLED,不可行动

它是被取消而非失败,且该 lane 检验的是 packages/web-shell,本 PR 并未触碰。Still-red checks 一节为空;TestLint & StaticIntegration Tests (no-AK, No Sandbox)OpenTUI no-flicker gateTUI parity snapshots 在本轮之前的 head 上全部为 SUCCESS。本轮未重跑集成测试:所触碰的行为(abort 落在启动等待期间,以及半建成 chat 上的预算耗尽)在该测试工具中没有确定性的手段可驱动,两条路径现在都有单元测试见证,而且新增的两个门禁在信号未被 abort、chat 已就绪时都是空操作 —— 而那正是集成测试所走的路径。

有一个既有测试被改动 —— 内容证据

skips steering when the turn is aborted 改动的是前置构造,不是断言。它原本在调用 generator 之前就 abort 了自己的 controller;在修复之后,这种状态会在发送之前就让该轮落定(而这正是修复的目的),因此该测试以 AbortError: This operation was aborted 失败。生产调用方无法到达这种状态:runTurn 新建一个 controller,而当 config 已初始化时,这一轮自己的 initialize() 会同步抛出,其 catch 在一个 microtask 上恢复 —— 从调用到等待之间不存在任何宏任务边界可供按键落下。它现在改为在假 stream 内部 abort,也就是真实 Esc 到达的位置,并仍然断言 drainSteering 未被调用。

这一点是实测的而非假设的,因为我起初以为这个改动会损失覆盖率:

组别 源码 测试 结果
1 本轮之前 + 删除 || abort.aborted 原始版本 1 passed
2 本轮 + 删除 || abort.aborted 改写版本 1 passed

两个版本都不见证工具响应边界的那个门禁,因此覆盖率没有移动。(更早的一组实验看起来像是原始版本捕获了该变异;但那里的 AbortError 来自本轮自己新增的 throwIfAborted(),而不是来自变异,上面的重跑把这一点隔离了出来。)

轮次机制 —— 共享工作目录上的并发运行

本轮准备写自己的输出时,/tmp/autofix-review-11046 中已经存在一整套不同的轮次输出:一份描述提交 3d291b2568(该提交在本仓库中并不存在)的 address-summary.md,以及 resolved-comments.txtcomment-replies.jsondeferred-findings.json。同一轮次的第二个运行正在另一个 checkout 上处理同一批反馈,同时共享这个工作目录路径。本总结描述的全部内容是本轮实际产出的提交,四份产物都已被覆盖为与其一致。之所以明确指出:两个运行同时向同一分支推送可能产生重复工作,而且把本总结与另一个运行的提交放在一起读会对不上。

验证

下面每条命令都是针对已提交的树真实运行过的,顺序与门禁重跑的顺序一致。

  • npm run build —— 通过(exit 0)
  • npm run typecheck —— 通过(exit 0,error TS 行数为 0)。第一次尝试捕获到了我自己造成的一个真实缺陷:对 oneToolBatchStream 返回的零参数假对象做展开时出现 TS2556;已修复并重跑至干净。
  • npm run lint —— 通过(exit 0)
  • 对两个被触碰文件运行 npx prettier --check —— 通过("All matched files use Prettier code style!")
  • npx vitest run src/ui/opentui/live-session.test.ts src/ui/opentui/live-turn.test.ts(被触碰模块,packages/cli)—— 61 passed(2 个文件)
  • npx vitest run src/ui/opentui(整个 OpenTUI 回归扫描,packages/cli)—— 67 files / 1137 tests passed (exit 0)
  • 集成测试 —— 未运行,原因见上
  • npm run generate:settings-schema —— 不适用,未改动 settings 源文件

变异探针 —— 本轮新增的每个门禁都有自己的见证用例,每个都在移除门禁后确认为红、恢复后确认为绿(live-session.test.ts,原状 49 个用例):

探针 变异 见证用例 结果
P1 从循环条件中去掉 && !signal?.aborted settles an Esc pressed during the startup wait without sending 1 failed —— expected false to be true(不会提前落定)
P2 去掉 signal?.throwIfAborted(); 同上 1 failed —— promise resolved "[ …(2) ]" instead of rejecting(被取消的 prompt 到达发送)
P4 去掉整个半建成 chat 的超时门禁 fails the turn when the startup wait expires on a chat with no tools 1 failed —— promise resolved … instead of rejecting(无工具 fall through)
P5 只从门禁中去掉 client.isInitialized() && reports the client error once the startup chat wait is spent(既有用例) 1 failed —— expected … 'Chat not initialized' but got 'Timed out after 15000ms…'(门禁会劫持无 chat 分支)

Critical 本身的先红证据,在任何源码改动之前:新增见证用例在未修改的 PR head 上运行结果为 1 failed | 47 passed (48)expected false to be true —— abort 被无视,generator 从未落定。

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

🧵 Resolved all 3 selected review thread(s). · 已关闭全部选中的 3 条评审线程。

Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审;第 10 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-2026-09-02

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE (verified at head d6c8110)

Historical blocking items — both Criticals re-verified fixed at this head

  • R1-1 (round-1 CHANGES_REQUESTED, [certifies-falsely]): the wait gated on chat existence, and startChat() assigns the chat before awaiting the SessionStart hook and setTools() — releasing within one poll tick sent the session's first prompt with zero tool declarations. Head gates on readiness instead: client.isInitialized() && client.getChat().getGenerationConfig().tools !== undefined. I verified the load-bearing premise at head rather than trusting the comment: GeminiClient.setTools unconditionally writes tools: [{ functionDeclarations: [...] }] (client.ts:1120-1121) and chat.setTools assigns it into the generation config (llm-chat.ts:5162-5164) — so even a zero-tools session lands the marker and cannot mis-timeout; only a flight still inside the startup gap leaves it undefined. The author's new test carries a red-before witness against the pre-fix code, asserting the send observes a literal tools value, not two compared undefineds.
  • R4-1 (round-4 CHANGES_REQUESTED, [certifies-falsely][new-surface]): an Esc during the wait previously fell through into the send path, which for text-only prompts fires UserPromptSubmit hooks and pushes history with no intervening abort check. Head closes it exactly as prescribed — the poll loop checks signal?.aborted, and signal?.throwIfAborted() runs immediately after the loop and before any send-path side effect; the new test proves both release-on-abort (on a poll tick, not the budget) and no-send. The bounded expiry branch throws rather than silently zero-tool-sending (fail-closed, pinned by its own test), while the never-initialized case still falls through to the client's own error, also pinned.

reviewDecision still shows CHANGES_REQUESTED because the requesting bot's round-4 review predates the fix commit d6c81109 and has not yet re-reviewed to flip the flag — the code both Criticals described no longer exists at this head, and the human maintainer approved after the fixes.

My Critical-only scan

The loop cannot hang the prompt (15 s bound, 100 ms poll, abort-aware), getChat() is only reached behind isInitialized(), getGenerationConfig() returns a shallow copy so the marker read is side-effect-free, and a SessionStart hook slower than 15 s converts today's silent tool-less first turn into a visible bounded error — a disclosed, strictly safer trade-off, not a defect. Test additions cover five distinct state boundaries plus one sharpening of the mid-turn-abort case.

CI at head

14 green, 4 in progress (non-gating), zero failures; two route meta-job cancellations match the fleet pattern seen across unrelated PRs this week.

@wenshao

wenshao commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Re-verification at d6c81109d2 — both residuals closed

Round 2 landed the two things my earlier report measured as open. I rebuilt from the new head and re-ran the whole matrix against the same base bundle (merge-base is unchanged, e3d26283, so the base arm is the same binary as before). Recommend merge. Nothing blocking is left.

What round 2 added to livePromptEvents, all three of which I mutated and probed:

  1. && !signal?.aborted in the loop condition,
  2. signal?.throwIfAborted() after the loop — this is the half my suggested one-liner missed, and the author is right that it is needed: with only the loop guard, a cancelled prompt falls out of the wait and walks into the send path, firing its UserPromptSubmit hooks and landing in history,
  3. an expiry throw gated on client.isInitialized() && !chatReady(), so the "chat exists, no tools" state fails loudly while the no-chat state still surfaces the client's own error.

Full matrix, all three arms, one machine

Same harness as before: real CLI bundle, bun 1.3.14, QWEN_TUI_RENDERER=opentui strict, scripted model server recording every request. Startup window widened with supported configuration only — a stdio MCP server that delays initialize (chat not yet created), or a SessionStart hook that sleeps (chat created, setTools() still pending).

scenario BASE round 1 (35975ed3) round 2 (d6c81109d2)
chat not yet created (MCP 6s) 0 requests, ✖︎ Chat not initialized waits 6067ms → 30 tools waits 5972ms → 30 tools
chat created, setTools() pending (hook 12s) sends in 82ms with no tools waits 9511ms → 30 tools waits 9413ms → 30 tools
startup slower than the budget (hook 25s) sends immediately, no tools waits 15133ms, then sends silently with no tools 0 requests; turn fails at +15058ms with Timed out after 15000ms…
no chat ever (MCP 60s) error at +84ms error at +15110ms error at +15107ms — still Chat not initialized, the client's own
Esc during the wait turn had already settled; next prompt accepted at +4.8s +16.3s — Esc ignored for the whole budget +4.8s
healthy session (3 runs) 74 / 75 / 76 ms 80 / 77 / 77 ms 77 / 77 / 76 ms

startup window A/B

R2-1 is closed. The oracle is OpenTUI's own behaviour — live-turn.ts submit() echoes a > user row only when the turn is idle, so "when does a second prompt appear" measures exactly when the turn released. Esc at submit+1501ms, second prompt typed at submit+3276ms: it appears at submit+4777ms, matching base (+4779ms) instead of round 1's +16282ms.

R2-2 is closed, and this is the one behaviour change worth being deliberate about. Where round 1 waited the budget and then sent a tool-less request that looked like a perfectly normal turn, the new head sends nothing and says so:

expiry fails loudly

I checked the recovery path rather than assuming it: after that failure, retyping the prompt works — the retry at 23063ms waits out the flight and lands at 27254ms with 30 tool declarations and a rendered reply. So this is an honest, recoverable failure, and I agree it beats answering a whole turn with zero tools. It is still a new user-visible failure mode for anyone whose SessionStart hook runs longer than 15s, and the message carries no "try again" hint.

The CI leg

Unchanged conclusion, re-measured on the new head. The leg's own command, pinned to one CPU (the closest honest stand-in for a loaded runner):

E2E under one CPU

arm wall slowest case Chat not initialized Timed out after
BASE 83.23s holds a slash command back mid-turn… 70809ms (retry x2) 2 0
round 2 18.49s same case, 6014ms, first attempt 0 0

Both arms exit 0 only because integration-tests/vitest.config.ts sets retry: 2; a run whose three attempts all land in the window is the red leg the issue reports.

Unpinned, the full interactive leg on the new head is Test Files 9 passed | 1 skipped, Tests 17 passed | 1 skipped, exit 0 (context-compress-interactive.test.ts excluded — it needs live model credentials this environment lacks). The whole OpenTUI unit tree is 67 files / 1137 tests passed.

Every clause is pinned

Eight mutations against live-session.test.ts (49 tests intact), each condition separately — all red:

mutation result witness
drop client.isInitialized() from chatReady 3 failed in-flight-startup, budget-spent, and Esc cases
drop …tools !== undefined 2 failed tool-declarations and expiry cases
drop Date.now() < chatDeadline 2 failed both expiry cases time out at 15000ms
drop && !signal?.aborted 1 failed settles an Esc pressed during the startup wait without sending
drop signal?.throwIfAborted() 1 failed same case — the send fires for a cancelled prompt
drop the expiry throw 1 failed fails the turn when the startup wait expires on a chat with no tools
widen the expiry gate to if (!chatReady()) 1 failed reports the client error once the startup chat wait is spent — the no-chat branch stops surfacing the client's error
chatDeadline = Date.now() (no wait at all) 3 failed the three startup-window cases

That last one matters: the client.isInitialized() && guard on the new throw is itself pinned, so the two expiry branches cannot be collapsed by accident.

Open items — none blocking

  • R2-5 is now worth more than it was. The readiness marker is still an unpinned cross-package contract on LlmChat.generationConfig.tools (llm-chat.ts:5162-5163 is its only writer; nothing in packages/core asserts it). With round 2's throw, a future core refactor that stops writing it no longer makes first turns merely slowchatReady() would be false forever and every first turn of every OpenTUI session would hard-fail after 15s, with every test in this PR still green. The hook-25s row above is that exact state, reproduced. A one-line assertion in llm-chat.test.ts after setTools() closes it cheaply.
  • R2-3 is half-addressed: the constant's comment no longer claims to share commands-dispatch's budget, so the prose coupling is gone; the two literal pairs still duplicate.
  • R2-4 (dedupe the in-flight Config.initialize() at the owner) and R3-1 (no debug breadcrumb) stand as recorded.
  • Not a regression, but worth knowing: the reworked skips steering when the turn is aborted does not pin the !abort.aborted guard at live-session.ts:877 — I removed the guard and all 49 tests stayed green. I checked the version it replaced under the same mutation and that one survived too (44 passed): with an aborted signal, drainSteering is never reached at all, so the assertion passes on a path that never touches the guard. Pre-existing gap, unchanged by this PR.

Merge mechanics

reviewDecision is still CHANGES_REQUESTED and mergeStateStatus BLOCKED, but every blocking review is stale:

reviewer state commit
qwen-code-ci-bot CHANGES_REQUESTED ca59b738
qwen-code-ci-bot CHANGES_REQUESTED 35975ed3
wenshao APPROVED d6c81109d2
qqqys APPROVED d6c81109d2

Both blocking reviews predate the current head, and the Criticals they named are the ones this round closed — verified above, not taken on trust. CI on d6c81109d2 is green across 118 checks (two route entries cancelled as superseded). The bot needs to re-review or the stale reviews need dismissing; there is no code left to change for it.

Verdict

Round 2 closes both residuals without touching anything else: the main fix still behaves identically (5972ms / 9413ms waits, 30 tool declarations, model reply), a healthy session still pays nothing measurable, the CI leg is still fixed under load, and every clause including the new ones is pinned by a witness. Recommend merge.

中文版

d6c81109d2 上重新验证 —— 两项残留均已关闭

第 2 轮落地的正是我在上一份报告中实测为未关闭的两点。我从新 head 重新构建,并针对同一套 base 产物重跑了全部验证矩阵(merge-base 未变,仍是 e3d26283,因此 base arm 与上次是同一个二进制)。建议合并。 没有任何阻断项。

第 2 轮给 livePromptEvents 增加的三处,我都做了变异与实测:

  1. 循环条件中加入 && !signal?.aborted
  2. 循环之后的 signal?.throwIfAborted() —— 这是我建议的一行改动漏掉的一半,作者的判断是对的:只加循环守卫的话,被取消的 prompt 会从等待中掉出来并进入发送路径,触发它的 UserPromptSubmit hook 并写入历史;
  3. client.isInitialized() && !chatReady() 为门的超时抛错,使「chat 存在但没有工具」这一状态显式失败,而「没有 chat」的状态仍然暴露客户端自己的错误。

完整矩阵,三个 arm,同一台机器

与上次相同的工具:真实 CLI bundle、bun 1.3.14、QWEN_TUI_RENDERER=opentui 严格模式、记录每一次请求的脚本化模型服务器。启动时间窗只用受支持的配置来放宽 —— 延迟 initialize 的 stdio MCP server(chat 尚未创建),或 sleep 的 SessionStart hook(chat 已创建、setTools() 未完成)。

场景 BASE 第 1 轮(35975ed3 第 2 轮(d6c81109d2
chat 尚未创建(MCP 6s) 0 个请求,✖︎ Chat not initialized 等待 6067ms → 30 个工具 等待 5972ms → 30 个工具
chat 已创建、setTools() 未完成(hook 12s) 82ms 就发出,无工具 等待 9511ms → 30 个工具 等待 9413ms → 30 个工具
启动慢于预算(hook 25s) 立刻发出,无工具 等待 15133ms 后静默发出无工具请求 0 个请求;该轮在 +15058ms 以 Timed out after 15000ms… 失败
永远没有 chat(MCP 60s) +84ms 报错 +15110ms 报错 +15107ms 报错 —— 仍是客户端自己的 Chat not initialized
等待期间按 Esc 该轮早已结束;下一个 prompt 在 +4.8 秒 被接受 +16.3 秒 —— Esc 在整个预算期间被无视 +4.8 秒
健康会话(3 次) 74 / 75 / 76 ms 80 / 77 / 77 ms 77 / 77 / 76 ms

startup window A/B

R2-1 已关闭。 判据用的是 OpenTUI 自身的行为 —— live-turn.tssubmit() 只在这一轮空闲时才回显 > 用户行,所以「第二个 prompt 何时出现」恰好度量了该轮何时释放。在提交 +1501ms 按 Esc、提交 +3276ms 输入第二个 prompt:它在 提交 +4777ms 出现,与 base(+4779ms)一致,而不是第 1 轮的 +16282ms。

R2-2 已关闭,这也是唯一一处值得刻意确认的行为变化。 第 1 轮会等满预算然后发出一个看起来完全正常、实则没有工具的请求;新 head 则什么都不发,并明确报错:

expiry fails loudly

我没有想当然,而是实测了恢复路径:这次失败之后重新输入 prompt 是可行的 —— 23063ms 的重试等到 flight 完成,27254ms 发出带 30 个工具声明的请求并正常渲染回复。所以这是一次诚实且可恢复的失败,我也认同它优于「用零工具回答一整轮」。但它对任何 SessionStart hook 超过 15 秒的用户来说,确实是一种新的可见失败形态,而且该消息没有给出「重试」提示。

CI leg

结论不变,在新 head 上重新测量。使用该 leg 自己的命令,绑定到单核(对「高负载 runner」最诚实的替代):

E2E under one CPU

arm 墙钟 最慢用例 Chat not initialized Timed out after
BASE 83.23s holds a slash command back mid-turn… 70809ms(retry x2) 2 0
第 2 轮 18.49s 同一用例,6014ms,第一次尝试即通过 0 0

两个 arm 最终都以 0 退出,只是因为 integration-tests/vitest.config.ts 设置了 retry: 2;当三次尝试全部落进这个时间窗时,就是 issue 报告的那个红色 leg。

不绑核时,新 head 上完整交互 leg 为 Test Files 9 passed | 1 skippedTests 17 passed | 1 skipped、exit 0(context-compress-interactive.test.ts 被排除 —— 它需要本环境没有的真实模型凭据)。整个 OpenTUI 单测树为 67 个文件 / 1137 个用例全部通过

每个子条件都被钉住

针对 live-session.test.ts(原状 49 个用例)逐条件做了 8 次变异 —— 全部变红

变异 结果 见证用例
chatReady 去掉 client.isInitialized() 3 failed in-flight 启动、预算耗尽、Esc 三个用例
去掉 …tools !== undefined 2 failed 工具声明与超时两个用例
去掉 Date.now() < chatDeadline 2 failed 两个超时用例都在 15000ms 处超时
去掉 && !signal?.aborted 1 failed settles an Esc pressed during the startup wait without sending
去掉 signal?.throwIfAborted() 1 failed 同一用例 —— 被取消的 prompt 仍会发送
去掉超时 throw 1 failed fails the turn when the startup wait expires on a chat with no tools
把超时门放宽为 if (!chatReady()) 1 failed reports the client error once the startup chat wait is spent —— 无 chat 分支不再暴露客户端错误
chatDeadline = Date.now()(完全不等待) 3 failed 三个启动时间窗用例

最后一条尤其重要:新抛错上的 client.isInitialized() && 这个门本身也被钉住了,因此两条超时分支不会被无意合并。

未决项 —— 均非阻断

  • R2-5 的分量比之前更重了。 就绪标记仍是一个没有被钉住的跨包契约,依赖 LlmChat.generationConfig.toolsllm-chat.ts:5162-5163 是其唯一写入处,packages/core 中没有任何断言)。有了第 2 轮的抛错,未来某次 core 重构若不再写入它,首轮对话就不只是变慢 —— chatReady() 会永远为 false,于是 每个 OpenTUI 会话的第一轮都会在 15 秒后硬失败,而本 PR 的所有测试仍然全绿。上表中 hook-25s 那一行复现的正是这个状态。在 llm-chat.test.tssetTools() 之后加一行断言即可低成本关闭。
  • R2-3 已处理一半:常量的注释不再声称与 commands-dispatch 共用预算,注释耦合已消除;两对字面量的重复仍在。
  • R2-4(在 owner 处对在途 Config.initialize() 做去重)与 R3-1(缺少调试面包屑)维持记录状态。
  • 不是回归,但值得知道: 被改写的 skips steering when the turn is aborted 并没有钉住 live-session.ts:877 处的 !abort.aborted 守卫 —— 我移除该守卫后 49 个用例仍全绿。我用同样的变异检查了它替换掉的旧版本,结果同样存活(44 passed):在 signal 已 abort 的情况下 drainSteering 根本不会被走到,因此该断言是在一条从未触及该守卫的路径上通过的。这是既有缺口,与本 PR 无关。

合并流程

reviewDecision 仍是 CHANGES_REQUESTEDmergeStateStatusBLOCKED,但所有阻断性评审都是过期的:

评审者 状态 commit
qwen-code-ci-bot CHANGES_REQUESTED ca59b738
qwen-code-ci-bot CHANGES_REQUESTED 35975ed3
wenshao APPROVED d6c81109d2
qqqys APPROVED d6c81109d2

两条阻断评审都早于当前 head,且它们点名的 Critical 正是本轮关闭的那些 —— 上文已验证,不是照单全收。d6c81109d2 上的 CI 在 118 项检查中全绿(两条 route 因被取代而取消)。需要的是让 bot 重新评审、或撤销过期评审;代码层面已无需改动。

结论

第 2 轮在不影响其他任何行为的前提下关闭了两项残留:主修复行为完全一致(等待 5972ms / 9413ms、30 个工具声明、正常回复),健康会话仍无可测开销,高负载下 CI leg 依然被修好,且包括新增条件在内的每个子句都有见证用例钉住。建议合并。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R5-4 steering-test guard coverage — already reported and dispositioned (comment 5551151695)
  • R5-5 unpinned cross-package readiness contract — already reported (comment 3939191936, R2-5)
  • R5-6 duplicated bounded startup poll — already reported (comments 3939191927 R2-3 and 3939191932 R2-4)
  • R5-7 no fail-fast on an already-failed initialization flight — already reported (comment 3938176016 R1-1 direction 2; comment 5549701086 sec.7c)

Not explored to full depth (tool budget reached): "agent 6a": whether the OpenTUI submit seam blocks the composer while a slash-command dispatch (e.g. /compress , /chat resume ) is in flight — I traced only streamingRef…; "agent 6a": a specific thrower inside startChat 's window between client.ts:2261 and the setTools() write — I ruled out warmAll() (non-strict allSettled , tool-reg…; "agent 1c": enumerating every way initializeOnce can reject permanently (I verified warmAll strictness and the llmClient.initialize() ordering, but did not walk acti…; "agent reverse-audit (round 2)": whether a picker-dialog-driven /resume or /branch switch runs session-switch.ts:169-193's awaits with OpenTuiSlashGateway.busy === false — the only route ….

Convergence: round 5 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 1 (1 new). Findings keep coming back to the same files: packages/cli/src/ui/opentui/live-session.ts (findings in round 4; 2 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)

中文说明

已审查。 建议见行内评论。

本轮确认的 4 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未探索到全部深度(达到工具调用预算):"agent 6a"whether the OpenTUI submit seam blocks the composer while a slash-command dispatch (e.g. /compress , /chat resume ) is in flight — I traced only streamingRef…"agent 6a"a specific thrower inside startChat 's window between client.ts:2261 and the setTools() write — I ruled out warmAll() (non-strict allSettled , tool-reg…"agent 1c"enumerating every way initializeOnce can reject permanently (I verified warmAll strictness and the llmClient.initialize() ordering, but did not walk acti…"agent reverse-audit (round 2)"whether a picker-dialog-driven /resume or /branch switch runs session-switch.ts:169-193's awaits with OpenTuiSlashGateway.busy === false — the only route …

收敛情况:第 5 轮发布了 2 条行内评论,其中 2 条是首次提出;上一轮发布了 1 条(其中 1 条首次提出)。发现反复回到同一批文件:packages/cli/src/ui/opentui/live-session.ts(第 4 轮已出过发现,本轮又有 2 条)。新发现的产出速度没有下降。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送,或将本 PR 的评审降到 --severity-floor critical,可以避免循环反复推导同一组发现。本轮没有未决的 Critical,因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。(仅为观察——本轮评审未因此扣留任何内容。)

— qwen3.8-max via Qwen Code /review (v0.23.0)

// error. Gated so the no-chat branch still surfaces the client's own error.
if (client.isInitialized() && !chatReady()) {
throw new Error(
`Timed out after ${STARTUP_CHAT_WAIT_MS}ms waiting for the startup chat to become ready`,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] STARTUP_CHAT_WAIT_MS is 15s, but the window it waits out is bounded by DEFAULT_HOOK_TIMEOUT = 60000 (packages/core/src/hooks/hookRunner.ts:45), so a SessionStart hook that is slow but healthy expires this wait and the gate this commit adds drops the session's first prompt instead of delivering it. startChat() assigns the chat at packages/core/src/core/client.ts:2261 and only reaches setTools() at :2308, awaiting the SessionStart hook in between (:2288); hook errors are swallowed, so a slow hook neither aborts nor fails the flight, it just holds tools === undefined past the budget. A project with a 25s SessionStart hook (a remote context fetch, a build step, a slow MCP-backed script) whose user submits at t+2s gets Timed out after 15000ms waiting for the startup chat to become ready at t+17s, rendered under an already-echoed user message, and the prompt has to be retyped even though startup was proceeding normally and would have been ready 10s later. Each queued submission repeats the stall, since runTurn pops the queue in its finally (live-turn.ts:249-271). This is new behaviour at this commit: at the previous head expiry fell through to the send, and the throw that converts it into a dropped prompt is what this commit adds.

Witness:

Probe (threshold sweep, readiness landing at t=R, BOUND=15000):
  R=14000 -> resolved, sends 1        R=15000 -> resolved, sends 1
  R=16000 -> rejected "Timed out after 15000ms...", sends 0, settledBeforeReadiness: true
  R=20000 -> rejected, sends 0        R=59000 -> rejected, sends 0
fix arm BOUND=75000, same rows: 16000 / 20000 / 59000 -> resolved, sends 1 each

Corroborated on the real product at this exact commit by maintainer comment 5551151695
(not run by this review):
  hook 25s -> "0 requests; turn fails at +15058ms with `Timed out after 15000ms...`"

Please rebase before retuning this bound — the poll may not survive the rebase. This branch is 17 commits behind main, and main has since landed fix(core): coalesce concurrent Config.initialize() calls (PR 11037), which makes Config.initialize() join the in-flight run instead of throwing — if (!this.initializationSettled) { options?.signal?.throwIfAborted(); await this.initializationPromise; return; } at origin/main:packages/core/src/config/config.ts:3004. That commit's own comment names this PR's symptom and call site: "callers that swallow the old throw (the OpenTUI submit path, slash-command loading) proceeded on a config whose chat had not started yet, and the first prompt died with 'Chat not initialized'" (issue 11002). After a rebase, await config.initialize() in livePromptEvents blocks until startChat() has settled, so this poll — and with it this bound, and the deferred R2-4 consolidation — may be removable rather than retuned. Worth verifying against the rebased tree before implementing any bound change here.

If the poll does stay, the bound must exceed the hook ceiling it waits behind, and the fix must not violate const DEFAULT_HOOK_TIMEOUT = 60000; (packages/core/src/hooks/hookRunner.ts:45, applied as hookConfig.timeout || DEFAULT_HOOK_TIMEOUT at :731 and hookConfig.timeout ?? DEFAULT_HOOK_TIMEOUT at :992, user-raiseable), which governs the hook awaited between the chat assignment and setTools() (client.ts:2261 -> :2288 -> :2308). The raise is also not free: the no-chat branch polls to the same chatDeadline (live-session.ts:586-591), so at STARTUP_CHAT_WAIT_MS = 75_000 the never-ready probe rows measured 75000ms x 3 prompts = 225s of frozen composer instead of 45s — so land the fail-fast/latch half alongside it, or scope the longer deadline to the chat-assigned branch only.

Please extend holds the first send until the startup chat has tool declarations in live-session.test.ts to advance past the hook ceiling with tools still undefined, assert sendMessageStream is not yet called, then set tools and advance again asserting one send and toolsAtSend equal to the literal declarations — and confirm that case goes red at the current 15s bound (the expiry throw fires first) and green once the bound exceeds the hook ceiling.

中文说明

STARTUP_CHAT_WAIT_MS 是 15 秒,但它所等待的那个时间窗上界是 DEFAULT_HOOK_TIMEOUT = 60000packages/core/src/hooks/hookRunner.ts:45),因此一个「慢但健康」的 SessionStart hook 会耗尽这个等待,而本次提交新增的门禁会丢弃该会话的第一个 prompt,而不是把它发出去。startChat()packages/core/src/core/client.ts:2261 处赋值 chat,直到 :2308 才执行 setTools(),其间要 await SessionStart hook(:2288);hook 的错误会被吞掉,所以慢 hook 既不会中止也不会让流程失败,只会让 tools === undefined 一直持续到预算耗尽。一个配置了 25 秒 SessionStart hook 的项目(远程上下文拉取、一次构建步骤、慢速的 MCP 脚本),用户在 t+2s 提交,就会在 t+17s 得到 Timed out after 15000ms waiting for the startup chat to become ready,并且该错误会渲染在已经回显的用户消息下面;即使启动过程一切正常、再过 10 秒就会就绪,这个 prompt 也必须重新输入。由于 runTurnfinally 中弹出队列(live-turn.ts:249-271),每一个排队的提交都会重复这次停顿。这是本次提交才出现的行为:在上一版 head 上,预算耗尽会 fall through 到发送;把「耗尽」变成「丢弃 prompt」的那个 throw 正是本次提交新增的。

证据:见上方英文部分的探针输出(阈值扫描:就绪时刻 R=14000/15000 正常发送;R=16000/20000/59000 全部被拒绝且发送数为 0;把上界改为 75000 的修复分支下,这三行都恢复正常发送)。维护者在评论 5551151695 中于真实产品上、在完全相同的提交上做了印证(非本评审运行):hook 25 秒 → 「0 个请求;该轮对话在 +15058ms 失败」。

请先 rebase,再调整这个上界 —— rebase 之后这个轮询可能根本不需要保留。 本分支落后 main 17 个提交,而 main 已经合入 fix(core): coalesce concurrent Config.initialize() calls(PR 11037),它让 Config.initialize() 加入在途的初始化流程而不是抛错 —— 见 origin/main:packages/core/src/config/config.ts:3004if (!this.initializationSettled) { options?.signal?.throwIfAborted(); await this.initializationPromise; return; }。该提交自己的注释就点名了本 PR 的症状与调用位置:「吞掉旧抛错的调用方(OpenTUI 提交路径、slash 命令加载)会在 chat 尚未启动的 config 上继续执行,于是第一个 prompt 以 'Chat not initialized' 结束」(issue 11002)。rebase 之后,livePromptEvents 中的 await config.initialize() 会一直阻塞到 startChat() 落定,因此这个轮询 —— 连同这个上界、以及已延后的 R2-4 收敛 —— 可能是可以移除的,而不是需要调参的。建议先对照 rebase 后的代码树验证,再决定是否在这里改动上界。

如果轮询确实保留,上界就必须超过它所等待的 hook 上界,且修复不得违反 const DEFAULT_HOOK_TIMEOUT = 60000;packages/core/src/hooks/hookRunner.ts:45,在 :731hookConfig.timeout || DEFAULT_HOOK_TIMEOUT、在 :992hookConfig.timeout ?? DEFAULT_HOOK_TIMEOUT 应用,用户可上调),它管辖的正是 chat 赋值与 setTools() 之间被 await 的那个 hook(client.ts:2261 -> :2288 -> :2308)。上调也不是没有代价:无 chat 的分支轮询的是同一个 chatDeadlinelive-session.ts:586-591),因此在 STARTUP_CHAT_WAIT_MS = 75_000 下,「永不就绪」的探针实测为 75000ms x 3 个 prompt = 225 秒的界面冻结(而不是 45 秒)—— 所以请同时落地「快速失败 / 一次性闩锁」那一半,或者把更长的时限只限定在 chat 已赋值的分支上。

请在 live-session.test.ts 中扩展 holds the first send until the startup chat has tool declarations:先推进到超过 hook 上界而 tools 仍为 undefined,断言 sendMessageStream 尚未被调用;再设置 tools 并继续推进,断言只发送一次且 toolsAtSend 等于字面量声明 —— 并请确认该用例在当前 15 秒上界下会变红(超时 throw 先触发),在上界超过 hook 上界之后变绿。

— qwen3.8-max via Qwen Code /review (v0.23.0)

const chatReady = () =>
client.isInitialized() &&
client.getChat().getGenerationConfig().tools !== undefined;
while (!chatReady() && Date.now() < chatDeadline && !signal?.aborted) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The PR description's Risk & Scope still asserts the opposite of what this commit does. It says "The wait does not consult the abort signal, again matching the existing sibling self-heal. Worst case an interrupt issued inside the startup window is honoured up to the bound late, and is then applied by the send path as usual" (mirrored in the Chinese fold as "该等待不查询 abort 信号"), but at this head the loop polls signal?.aborted and signal?.throwIfAborted() runs immediately after it, so an Esc inside the startup window rejects the turn before any send-path side effect, on the next 100ms tick rather than at the bound. That is the opposite behaviour, and exactly what round 4's Critical was filed to obtain. The closing line "the 44 pre-existing cases in the touched spec file are unchanged and green" is also stale on "unchanged": this commit reshapes skips steering when the turn is aborted, and the file now holds 49 cases against 44 at the merge base. The cost is concrete rather than cosmetic — the description is the risk map a maintainer reads and the record that ships with the merge, so one reasoning from it expects an interrupt to be delayed up to 15s and then applied by the send path (the UserPromptSubmit hooks and the history push included), and would mis-diagnose an Esc-during-startup report as expected latency instead of a pre-send rejection.

Witness:

Probe (`gh pr view 11046 --json body` at head d6c81109d2):
  line 74:  "The wait does not consult the abort signal ... honoured up to the bound late,
             and is then applied by the send path as usual"
  line 161: "该等待不查询 abort 信号"          (Chinese mirror)

  code at that same head:
    live-session.ts:590  while (!chatReady() && Date.now() < chatDeadline && !signal?.aborted) {
    live-session.ts:597  signal?.throwIfAborted();

  case counts, measured:
    git show e3d26283:...live-session.test.ts | grep -cE '^\s*(it|test)\('  = 44
    at HEAD                                                                = 49   (vitest agrees: 49 passed)
    -> the description's base count 44 is right; what is stale is "unchanged",
       the current total, and the abort bullet

Please update Risk & Scope before merge: replace the abort bullet with the head behaviour (the loop polls signal?.aborted, and throwIfAborted() routes a cancelled turn through runTurn's catch, so no send-path side effect runs and the interrupt is honoured on the next 100ms tick, not at the bound), and correct the test line to name the actual counts (44 pre-existing at the merge base, 49 at head) plus the one reshaped pre-existing case.

中文说明

PR 描述中的「风险与范围」仍然断言了与本次提交相反的行为。它写着「该等待不查询 abort 信号,同样是为了与已有的同类自愈机制保持一致。最坏情况是:在启动时间窗内发出的中断最多延迟到该上界才被响应,之后仍会由发送路径正常处理」(中文折叠部分同样如此),但在当前 head 上,循环会轮询 signal?.aborted,并且 signal?.throwIfAborted() 紧随其后执行 —— 因此在启动时间窗内按下 Esc 会在任何发送路径副作用之前让该轮对话以错误结束,并且是在下一个 100ms 轮询点生效,而不是等到上界。这与描述所说的行为正好相反,而这恰恰是第 4 轮 Critical 要求获得的行为。结尾那句「被修改的 spec 文件中已有的 44 个用例保持不变且全部通过」在「保持不变」这一点上也已过期:本次提交改写了 skips steering when the turn is aborted,并且该文件现在有 49 个用例,而合并基线上是 44 个。代价是具体的而非形式上的 —— 描述是维护者据以判断的风险地图,也是随合并一起留存的记录,因此按它推理的人会认为中断最多被延迟 15 秒、然后由发送路径处理(包括 UserPromptSubmit hook 与历史写入),从而把一份「启动期间按 Esc」的问题报告误判为预期延迟,而不是一次发送前的拒绝。

证据:见上方英文部分的探针输出(在该 head 上通过 gh pr view 11046 --json body 读取,描述第 74 行与中文镜像第 161 行仍在断言旧行为,而同一 head 的代码是 live-session.ts:590 的循环条件与 :597throwIfAborted();用例数实测为合并基线 44、head 49,vitest 同样报告 49 通过 —— 描述里的基线数字 44 是对的,过期的是「保持不变」、当前总数,以及那条 abort 说明)。

请在合并前更新「风险与范围」:把 abort 那一条替换为 head 的实际行为(循环会轮询 signal?.aborted,且 throwIfAborted() 会把已取消的一轮导向 runTurn 的 catch,因此不会执行任何发送路径副作用,中断在下一个 100ms 轮询点即被响应,而不是等到上界),并把测试那一句改为写明实际数量(合并基线 44 个既有用例、head 49 个)以及那个被改写的既有用例。

— qwen3.8-max via Qwen Code /review (v0.23.0)

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator Author

🔀 Base updated: red check(s) [Lint & Static (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run.

中文说明

🔀 已更新 base:红色检查 [Lint & Static (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main,CI 将重新运行。

@wenshao
wenshao enabled auto-merge September 5, 2026 13:48
@wenshao
wenshao added this pull request to the merge queue Sep 5, 2026
Merged via the queue into main with commit 9af3f86 Sep 5, 2026
38 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/self-reported The linked issue was opened by the PR author (self-reported)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Main CI failed: E2E Tests on 39a84c9e1db4

4 participants