Skip to content

feat(web-shell): run read-only info commands immediately mid-turn - #8496

Merged
wenshao merged 13 commits into
QwenLM:mainfrom
wenshao:feat/web-shell-mid-turn-info-commands
Aug 5, 2026
Merged

feat(web-shell): run read-only info commands immediately mid-turn#8496
wenshao merged 13 commits into
QwenLM:mainfrom
wenshao:feat/web-shell-mid-turn-info-commands

Conversation

@wenshao

@wenshao wenshao commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

In the Web Shell, the read-only info commands /stats, /about (same handler as /status) and /context now run immediately while a turn is streaming. Previously they were silently swallowed mid-turn: the composer cleared and nothing happened until the user re-ran the command after the turn finished. The command echo (the /stats user row) is still only appended when no turn is in flight, because that user row acts as a turn boundary and would split the active turn; only the echo is skipped mid-turn, and the command output itself renders inline as before.

Why it's needed

Long turns are exactly when users want to check token usage or session info, and today typing /stats or clicking the status-bar context indicator mid-turn produces zero feedback — the command just vanishes. It is safe to run these commands during a turn: they are read-only queries, and their results render as status blocks, which are not turn boundaries in the transcript turn-collapse logic, are not counted in turn metrics (tool/thinking/token counters), and stay visible when the turn is collapsed — identical to their behavior when idle. Skipping the echo also avoids finalizing the in-flight assistant block mid-stream, which appending a local user message would do.

Reviewer Test Plan

How to verify

  1. Open the Web Shell against a daemon session and send a prompt that keeps the model busy for a while (e.g. a multi-step task).
  2. While the turn is still streaming, type /stats (or /about, /context, or click the context indicator in the status bar) and press Enter.
  3. Expected: the stats/about/context output appears inline in the transcript immediately; the active turn's tool/thinking/token counters are unaffected and the turn is not split into two. The /stats echo row is absent mid-turn (it only appears when running the command while idle).
  4. Run the same commands while idle: behavior is unchanged — the echo row appears followed by the output.
  5. Unit tests cover the new echo helper: cd packages/web-shell && npx vitest run client/utils/localCommandQueue.test.ts (7 tests pass), plus the full App suite npx vitest run client/App.test.tsx (303 tests pass), npm run typecheck, ESLint and Prettier.

Evidence (Before & After)

Before (mid-turn): typing /stats cleared the composer with no toast, no output, and no queueing — the command was dropped. After (mid-turn): the stats output renders inline immediately with no echo row; idle behavior unchanged. No screenshots captured; verified via the unit suites above.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Local automated verification on macOS: vitest unit tests, TypeScript typecheck, ESLint, Prettier. No manual browser session was recorded.

Risk & Scope

  • Main risk or tradeoff: mid-turn output has no command echo row above it (the echo is client-side transient state and never enters session history, so this only affects the live transcript); status blocks appearing inside the active turn are kept visible on collapse, same as when idle.
  • Not validated / out of scope: other echo-style local commands keep the current behavior — /tools (bare listing), /bug, /model --voice, /extensions install usage errors are still suppressed mid-turn; daemon-forwarded commands still go through the queue/blocked paths. No Playwright e2e was added.
  • Breaking changes / migration notes: none.

Linked Issues

None.

中文说明

本 PR 做了什么

在 Web Shell 中,只读信息类命令 /stats/about(与 /status 同一处理分支)和 /context 现在可以在回合进行中立即执行。此前它们在回合中会被静默吞掉:输入框被清空,但什么都不发生,用户必须等回合结束后重新输入。命令回显(即 /stats 那行用户消息)仍然只在没有回合进行时才追加,因为该用户行会作为回合边界把正在进行的回合切成两段;回合中只跳过回显,命令输出本身照常内联渲染。

为什么需要

长回合进行时恰恰是用户最想查看 token 用量或会话信息的时机,而现在回合中输入 /stats 或点击状态栏的 context 指示器没有任何反馈——命令直接消失。这些命令在回合中执行是安全的:它们都是只读查询,其结果以 status 块渲染,而 status 块在 transcript 的回合折叠逻辑中不是回合边界、不计入回合统计(tool/thinking/token 计数),回合折叠后也保持可见——与空闲时的行为完全一致。跳过回显还避免了在流式中途收尾正在进行的 assistant 块(追加本地用户消息会触发这一行为)。

Reviewer Test Plan

如何验证

  1. 用 Web Shell 连接一个 daemon 会话,发送一个会让模型忙较长时间的提示(例如多步任务)。
  2. 回合仍在流式输出时,输入 /stats(或 /about/context,或点击状态栏的 context 指示器)并回车。
  3. 预期:stats/about/context 输出立即内联出现在 transcript 中;当前回合的 tool/thinking/token 计数不受影响,回合不会被切成两段。回合中不会出现 /stats 回显行(回显只在空闲时执行命令时出现)。
  4. 空闲时执行相同命令:行为不变——先出现回显行,再出现输出。
  5. 单元测试覆盖新的回显 helper:cd packages/web-shell && npx vitest run client/utils/localCommandQueue.test.ts(7 个测试通过),以及完整 App 套件 npx vitest run client/App.test.tsx(303 个测试通过)、npm run typecheck、ESLint、Prettier。

Evidence (Before & After)

改动前(回合中):输入 /stats 后输入框被清空,无 toast、无输出、不排队——命令被丢弃。改动后(回合中):stats 输出立即内联渲染,无回显行;空闲时行为不变。未截图,以上述单元测试套件验证为准。

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

macOS 本地自动化验证:vitest 单元测试、TypeScript typecheck、ESLint、Prettier。未录制手动浏览器会话。

Risk & Scope

  • 主要风险或权衡:回合中的输出上方没有命令回显行(回显是客户端瞬态,不进会话历史,只影响当前 transcript 显示);出现在活跃回合内的 status 块在折叠后保持可见,与空闲时一致。
  • 未验证 / 不在范围内:其他回显型本地命令保持现状——/tools(无参列表)、/bug/model --voice/extensions install 用法错误仍在回合中被抑制;转发 daemon 的命令仍走排队/阻塞路径。未新增 Playwright e2e。
  • 破坏性变更 / 迁移说明:无。

Linked Issues

无。

/stats, /about (/status) and /context were silently swallowed while a
turn was streaming, because their local user echo would act as a turn
boundary in applyTurnCollapse and split the active turn. Their output
is a status block, which is not a turn boundary and is not counted in
turn metrics, so only the echo needs to be skipped mid-turn.

Add appendLocalUserEchoIfIdle, which echoes when idle and skips the
echo while streaming without blocking the command, and switch these
three commands to it so their results render inline immediately even
during an active turn.
@wenshao

wenshao commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /takeover

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

🖼️ web-shell visual preview

Rendered against a mock daemon (no real backend): the PR base vs this PR head 15f3e00. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

Screenshots · before / after

ℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:

  • packages/web-shell/client/App.tsx

Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to packages/web-shell/client/e2e/visuals/screenshots.spec.ts that seeds whatever state the UI is gated on; it then appears here as a head-only (NEW) capture.

Full-resolution recordings (.webm) are attached to the workflow run.

Qwen Code · web-shell visuals

@qwen-code-dev-bot qwen-code-dev-bot added the autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) label Aug 4, 2026
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the autofix/takeover label (or comment @qwen-code /takeover stop) to release.

中文说明

🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。本 PR 来自 fork,首轮处理将由下一次定时扫描执行(通常几分钟内)。移除 autofix/takeover 标签(或评论 @qwen-code /takeover stop)即可释放。

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Re-run on the post-update-branch head: 15f3e00 is the merge of current main the previous pass was waiting for. Checked the merge commit's file list — it brings in main only; no PR-owned file changed, so the gate assessment of the feature diff stands.

Template ✓ — all required sections present, bilingual.

Problem: observed, not theoretical. Confirmed in the base code: while a turn is streaming, echoOrDeferLocalCommand reports "suppressed" and every affected caller bails out, so mid-turn /stats, /about//status, /context (and the status-bar context indicator) clear the composer and silently do nothing.

Direction: aligned. Long turns are exactly when users want to check token usage or session info, and status blocks are already turn-collapse-safe (not turn boundaries, not counted in turn metrics, visible when collapsed). Web-shell surface, no core-mission tension.

Size: 206 production lines (sdk-typescript 15, web-shell 191) vs 426 test lines. Cross-package (sdk-typescript + web-shell) but feat-type and well under the awareness threshold.

Approach: minimal. Reuses the pre-existing clearActiveText opt-out on appendStatusBlock instead of inventing a new path — three call sites plus one shared dispatch helper, no drive-by changes. The consolidation rounds folded the intermediate echo helper back into echoOrDeferLocalCommand and added /status routing coverage.

Risk: no elevated risk signals — the Stage 1e high-risk-path scan is clean.

Moving on to code review. 🔍

中文说明

在 update-branch 之后的 head 上重跑:15f3e00 即上一轮等待的"合并当前 main"。已查看合并提交的文件列表——只带入 main,未改动任何本 PR 的文件,因此对功能 diff 的门禁结论不变。

模板完整 ✓ —— 所有必需小节齐全,中英双语。

问题:已观测到,不是理论问题。在基线代码中确认:回合进行中 echoOrDeferLocalCommand 返回"被抑制",所有受影响的调用方随即直接退出,因此回合中的 /stats/about//status/context(以及状态栏的 context 指示器)只会清空输入框,然后什么都不发生。

方向:对齐。长回合恰恰是用户最想查看 token 用量或会话信息的时候,且 status 块本身对回合折叠是安全的(不是回合边界、不计入回合统计、折叠后保持可见)。属于 web-shell 表面,与核心使命无冲突。

规模:生产代码 206 行(sdk-typescript 15 行,web-shell 191 行),测试 426 行。跨包(sdk-typescript + web-shell)但属于 feat 类型,远低于需维护者关注的阈值。

方案:最小化。复用 appendStatusBlock 上已有的 clearActiveText opt-out,而不是新造路径——三个调用点加一个共享 dispatch helper,无顺手改动。整合各轮已把中间的 echo helper 合并回 echoOrDeferLocalCommand 并补充了 /status 路由覆盖。

风险:无升级风险信号 —— Stage 1e 高风险路径扫描未命中。

进入代码审查。🔍

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code review

Reviewed 15f3e00 in full (the complete diff, end to end). It is the same feature diff reviewed at f7d04f6 — the new commit is the update-branch merge of current main, and the merge commit's file list confirms no PR-owned file was touched (no conflict resolutions crept in). I re-verified the load-bearing claims against the base code rather than carrying the previous pass forward on faith:

  • Turn splitting: isTurnStartMessage (MessageList.tsx:790) matches only user / user_shell — a status block can never split the active turn.
  • Backward compatibility: in the base, status/debug fall through to the shared appendStatusBlock call with no opts, so clearActiveText(state) always ran. The PR splits status/debug into their own case passing event.clearActiveText; daemon-emitted events leave it unset → identical behavior. Only the web shell's client-dispatched events opt out.
  • Reducer opt-out: with clearActiveText: false the status block skips finishAssistant (protecting the in-flight assistant/thought block and its usage frames — applyAssistantUsage drops usage when there is no active assistant block) but still resets activeUserBlockId via the new else branch. That reset is necessary: without it a later mergeable user.text.delta (a peer client's prompt echo) would append onto the command echo block. The new reducer test pins exactly that sequence, and it fails under mutation of the else branch.
  • Pre-existing caller: the other clearActiveText: false site (trimmed-tool-output notice, transcript.ts:757) also gains the user-pointer reset — same semantics; no realistic regression path is constructible.
  • Error paths, tested: /stats's .catch(() => {}) and the unguarded /about chain now surface failures via reportError, which toasts without dispatching a transcript block — the failure path can't split the turn either.

Non-blocking items that remain (unchanged from the previous pass; both are acknowledged in the author's own review):

  1. Situational streaming UX — with clearActiveText: false, while prose is actively streaming the status panel lands mid-turn and continuing deltas merge into the block above it; resumeChatBottomFollow anchors the viewport to the panel, so the transcript can look frozen until the next tool/assistant block lands. Not a correctness bug — the inherent cost of not finalizing the in-flight block — but worth a docs sentence or a follow-up to skip bottom-follow mid-stream.
  2. Stale test-plan line in the PR body — it cites client/utils/localCommandQueue.test.ts as covering "the new echo helper", but the consolidated diff doesn't touch that file.

No critical blockers found.

Test evidence — the PR's own CI at the reviewed commit

Green. The checkout-wiring failure that red-flagged f7d04f6 is resolved exactly as predicted: the takeover loop merged current main (which carries .github/actions/verify-checkout-head) and the ubuntu test lane now passes on this head. web-shell E2E Smoke runs again too (it was a collateral skip before) and passes. macOS/Windows lanes remain skipped, as throughout this PR's history.

Final CI results for 15f3e00:

Check Conclusion
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
Capture web-shell visuals (ubuntu-latest, Node 22.x) ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Classify PR ✅ success

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

The unit tests are load-bearing for the change: the App tests assert dispatch with clearActiveText: false and no echo while streamingState is responding, and the reducer tests fail under mutation of the new else branch. The live-browser rendering claim rests on jsdom-level tests plus the mock-daemon E2E smoke, not a recorded session — sandboxed verification would settle that if anyone wants it before merge: @qwen-code /verify — an A/B pass would pin the mid-turn rendering the unit suite cannot reach. (@yiliang114 has already approved this head; noting the lane for completeness.)

中文说明

代码审查

完整审查了 15f3e00(diff 全文通读)。与在 f7d04f6 上审查的功能 diff 相同——新提交只是 update-branch 合并当前 main,合并提交的文件列表证实未触及任何本 PR 的文件(没有夹带冲突解决)。关键论断重新对着基线代码核实,而不是沿用上一轮结论:

  • 回合切分isTurnStartMessage(MessageList.tsx:790)只匹配 user / user_shell —— status 块不可能切断活跃回合。
  • 向后兼容:基线中 status/debug 落入无 opts 的共用 appendStatusBlock 调用,clearActiveText(state) 恒执行。本 PR 把 status/debug 拆为独立分支并传入 event.clearActiveText;daemon 发出的事件不设置该字段 → 行为完全一致,只有 web shell 客户端派发的事件才 opt-out。
  • Reducer opt-outclearActiveText: false 时 status 块跳过 finishAssistant(保住进行中的 assistant/thought 块及其 usage 帧——没有活跃 assistant 块时 applyAssistantUsage 会丢弃 usage),但新的 else 分支仍重置 activeUserBlockId。该重置必要:否则后续可合并的 user.text.delta(peer 客户端的 prompt 回显)会并入命令回显块。新的 reducer 测试恰好固定了这一序列,且对该 else 分支做变异时测试失败。
  • 既有调用点:另一处 clearActiveText: false(tool 输出被裁剪提示,transcript.ts:757)同样获得 user 指针重置——语义一致;构造不出真实的回归路径。
  • 错误路径(有测试)/stats.catch(() => {})/about 的裸链现在经 reportError 暴露失败;只弹 toast 不写 transcript,失败路径同样不会切分回合。

剩余非阻断项(与上一轮相同;作者自评中均已确认):

  1. 特定场景下的流式 UX——clearActiveText: false 下文字仍在流式输出时,status 面板落在回合中间,后续增量并入其上方的块;resumeChatBottomFollow 又把视口锚定在面板上,直到下一个 tool/assistant 块出现前 transcript 看起来可能像"卡住"。不是正确性缺陷——这是不收尾进行中块的固有代价——但值得在文档补一句,或后续在流式期间跳过 bottom-follow。
  2. PR 描述中的测试计划过期——其中称 client/utils/localCommandQueue.test.ts 覆盖"新的 echo helper",但整合后的 diff 并不改动该文件。

未发现关键阻断问题。

测试证据 —— 该 PR 自身在被审提交上的 CI

绿色。 上一轮 f7d04f6 的 checkout 接线失败已按预期解决:takeover 循环合并了当前 main(其中带有 .github/actions/verify-checkout-head),ubuntu 测试车道在本 head 上通过。web-shell E2E Smoke 也恢复运行(此前是连带跳过)并通过。macOS/Windows 车道与本 PR 历史一致,保持跳过。

CI 明细见上方表格。单元测试对本改动是有效的:App 测试在 streamingStateresponding 时断言 dispatch 携带 clearActiveText: false 且不产生回显,reducer 测试在对新增 else 分支做变异时失败。真实浏览器渲染这一论断依据的是 jsdom 级测试加 mock-daemon E2E smoke,而非录制的真实会话——如果有人在合并前想补上这一点,沙箱验证可以覆盖:@qwen-code /verify —— A/B 验证能固定单元测试无法覆盖的回合中渲染行为。(@yiliang114 已批准本 head;此行仅为完备性记录。)

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean review across every stage and green CI on the exact head; the two open items are non-blocking nits (streaming-order UX worth a docs sentence, stale test-plan line in the body), not doubts about the change.

This is the re-run the previous pass asked for. The takeover loop merged current main, the checkout-wiring failure cleared, and the ubuntu lane plus the web-shell E2E smoke are green on 15f3e00. The feature diff itself is unchanged from the commit reviewed last time — I re-read it end to end anyway and re-verified every load-bearing claim against the base code: the status block cannot split a turn, daemon-emitted status events behave exactly as before, the opt-out protects the in-flight assistant block and its usage frames while still resetting the user pointer, and the tests pin all of it (they fail under mutation).

Stepping back: the problem was real and observable — mid-turn /stats just vanished. The solution is the minimal one: skip only the echo (the one piece that acts as a turn boundary), reuse the reducer's existing clearActiveText opt-out, and handle the single subtlety that opt-out exposes. Error paths that used to be swallowed silently now surface, without gaining any turn-splitting power of their own. Nothing in the diff goes beyond the stated goal, and the author's own deep review, the autofix rounds, and @yiliang114's approval on this head all converge on the same read.

The one thing I'd keep on the follow-up list is the streaming-order behavior (Stage 2, note 1): while prose is actively streaming, the panel lands mid-turn and the viewport anchors to it, which can read as frozen until the next block lands. An acceptable trade for not splitting the turn — just worth a docs sentence or a small follow-up.

Housekeeping: this approval supersedes the bot's stale CHANGES_REQUESTED on 508e2b0 (test-count wording, self-marked "not a blocker"); nothing from that review applies to the current diff. ✅ Approving, pinned to this commit.

中文说明

信心:4/5 —— 各阶段审查干净、被审 head 上 CI 全绿;两个遗留项均为非阻断提示(流式顺序 UX 值得补一句文档、PR 描述中的测试计划一行过期),不代表对改动本身有任何怀疑。

这正是上一轮等待的重跑:takeover 循环合并了当前 main,checkout 接线失败消除,ubuntu 车道与 web-shell E2E smoke 在 15f3e00 上变绿。功能 diff 与上次审查的提交完全一致——我仍然重新通读了全部 diff,并把每个关键论断重新对着基线代码核实:status 块不可能切分回合;daemon 发出的 status 事件行为与之前完全一致;opt-out 保住进行中的 assistant 块及其 usage 帧,同时仍重置 user 指针;测试把所有这些都固定住了(变异即失败)。

退一步看:问题真实可见——回合中的 /stats 直接消失。方案就是最小方案:只跳过回显(唯一充当回合边界的部分),复用 reducer 现有的 clearActiveText opt-out,并处理该 opt-out 暴露的唯一细节。原本被静默吞掉的错误路径现在被暴露,且失败路径本身不会获得切分回合的能力。diff 中没有超出既定目标的内容;作者的深度自评、autofix 各轮、以及 @yiliang114 在本 head 上的批准都指向同一结论。

唯一建议留在跟进清单上的是流式顺序行为(Stage 2 注 1):文字仍在流式输出时,面板落在回合中间且视口锚定其上,看起来可能像"卡住",直到下一个块出现。这是不切分回合的可接受代价——但值得在文档补一句或做一个小的后续跟进。

备忘:本次批准取代机器人在 508e2b0 上的过期 CHANGES_REQUESTED(测试数量措辞,自标"非阻断");该 review 没有任何内容适用于当前 diff。✅ 批准,固定于本提交。

Qwen Code · qwen3.8-max

Reviewed at 15f3e00259f508e09169133bcce45248a2406986 · 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.

LGTM, looks ready to ship — CI landed green after the review. ✅

@qwen-code-dev-bot

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

Copy link
Copy Markdown
Collaborator

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

中文说明

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

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下:

Autofix review round — no code change required

The PR is already approved ("LGTM, looks ready to ship — CI landed green") with all checks green and no failing or still-red checks. The only new feedback item is the automated web-shell visual preview informational comment, which notes that this PR edits a render-shaping file (packages/web-shell/client/App.tsx) but produces no screenshot diff, and suggests adding a scenario to packages/web-shell/client/e2e/visuals/screenshots.spec.ts.

After inspecting the change and the visual harness, I'm declining that suggestion with evidence rather than adding a scenario, for the reasons below. No source change is warranted this round.

Why a visual screenshot scenario is not appropriate for this change

1. This is a timing/interaction change, not a static visual change. The PR makes the read-only info commands (/stats, /about, /context) run immediately mid-turn instead of being silently dropped, and skips only the user echo row while streaming. The status block those commands render is byte-for-byte the same component already rendered in the idle case — the PR does not change how the result looks, only when it appears and whether the echo row precedes it. A before/after static-frame comparison cannot express "the command now runs instead of vanishing mid-turn."

2. The only mid-turn visual state is byte-nondeterministic. To show the changed behavior, a scenario would have to hold the session mid-turn so StreamingStatus renders. That component is driven by:

  • a braille spinner advanced by a 250 ms setInterval (SPINNER_FRAMES / setDotFrame in StreamingStatus.tsx),
  • a loading phrase picked via Math.random() and rotated on a timer,
  • an elapsed-time counter ticking on wall clock.

The harness's freezeLoopingAnimations() only pauses WAAPI / CSS @keyframes animations found via document.getAnimations(); its own comment warns that a spinner driven by a hand-rolled JS loop "would NOT be caught, and the flake would silently return." A setInterval + React-state spinner, a Math.random() phrase, and a wall-clock timer are all outside its reach, so any mid-turn capture would differ between identical runs and surface false-positive "changed" previews unrelated to the PR.

3. This matches the existing codebase convention. The split view scenario in the same file deliberately asserts the restore path without capturing a screenshot for exactly this reason: "the capture is byte-nondeterministic between identical runs — a flaky, redundant view that surfaces false-positive 'changed' previews unrelated to the PR under review." The established pattern for a nondeterministic state is a behavioral assertion, not a screenshot.

4. The behavior is already covered. The new appendLocalUserEchoIfIdle logic (echo when idle, skip-but-don't-block while streaming) is unit-tested in packages/web-shell/client/utils/localCommandQueue.test.ts, and the independent triage verified each mechanism against the base code and approved at confidence 4/5, explicitly marking the remaining coverage reservations as non-blocking.

Adding a static screenshot scenario here would introduce a flaky, diff-noise-producing test and grow the diff of an already-approved PR without asserting the actual behavior — the opposite of what the visual suite is designed for. If mid-turn behavioral coverage is wanted later, a recorded flows.spec.ts flow (where motion is the point) would be the right vehicle, but that is net-new scope beyond this feedback and is left to a maintainer to request.

中文说明

Autofix 评审轮次 —— 无需修改代码

本 PR 已被批准("LGTM,看起来可以合入 —— 评审后 CI 全绿"),所有检查均为绿色,没有失败或持续失败的检查。唯一的新反馈条目是自动化的 web-shell 视觉预览信息性评论:它注意到本 PR 修改了一个影响渲染的文件(packages/web-shell/client/App.tsx),但没有产生任何截图差异,因此建议向 packages/web-shell/client/e2e/visuals/screenshots.spec.ts 添加一个场景。

在检查了改动内容与视觉测试脚手架(harness)之后,我基于如下证据婉拒该建议,而不是添加场景。本轮不值得修改源码。

为什么视觉截图场景不适合本改动

1. 这是一个时序/交互改动,而非静态视觉改动。 本 PR 让只读信息命令(/stats/about/context)在回合进行中立即执行,而不是被静默丢弃,并且仅在流式输出时跳过用户回显行。这些命令渲染的状态块(status block)与空闲(idle)情况下已渲染的组件逐字节相同 —— 本 PR 并未改变结果的外观,只改变了它何时出现以及回显行是否出现在其前面。静态帧的前后对比无法表达"该命令现在会执行而不是在回合中消失"。

2. 唯一的回合中视觉状态是字节级不确定的。 要展示被改变的行为,场景必须让会话停留在回合中,使 StreamingStatus 渲染出来。而该组件由以下因素驱动:

  • 一个由 250 毫秒 setInterval 推进的盲文点阵加载动画(StreamingStatus.tsx 中的 SPINNER_FRAMES / setDotFrame),
  • 一个通过 Math.random() 选取并定时轮换的加载短语,
  • 一个按实时时钟计时的已用时间计数器。

harness 的 freezeLoopingAnimations() 只会暂停通过 document.getAnimations() 找到的 WAAPI / CSS @keyframes 动画;它自己的注释就警告:由手写 JS 循环驱动的加载动画"不会被捕获,flake(不稳定)会悄悄回归"。一个由 setInterval + React 状态驱动的动画、一个 Math.random() 短语以及一个实时时钟计时器全都超出其能力范围,因此任何回合中的截图在完全相同的多次运行之间都会不同,从而产生与本 PR 无关的假阳性"已变化"预览。

3. 这与现有代码库约定一致。 同一文件中的 split view 场景刻意只断言恢复路径而截图,原因正是:"该截图在相同运行之间是字节级不确定的 —— 一个不稳定、冗余的视图,会产生与本 PR 无关的假阳性'已变化'预览。" 对于不确定状态的既定做法是行为断言,而非截图。

4. 该行为已有测试覆盖。 新增的 appendLocalUserEchoIfIdle 逻辑(空闲时回显、流式时跳过但不阻塞)已在 packages/web-shell/client/utils/localCommandQueue.test.ts 中做了单元测试;独立的 triage 也已对照基础代码逐一验证了各个机制,并以 4/5 的信心批准,明确将剩余的覆盖保留意见标记为非阻塞。

在此添加一个静态截图场景,只会引入一个不稳定、只会产生差异噪声的测试,并使一个已被批准的 PR 的 diff 膨胀,却无法断言实际行为 —— 这与视觉测试套件的设计初衷背道而驰。如果之后需要回合中的行为覆盖,录制式的 flows.spec.ts flow(其中"动态过程"本身就是重点)才是合适的载体,但那属于超出本反馈的全新范围,留给维护者决定是否提出。

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


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

@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): 7 tests pass — this review observed 2764 passed; 303 tests pass — this review observed 2764 passed.

中文说明

Test Plan(非阻断):7 tests pass — this review observed 2764 passed; 303 tests pass — this review observed 2764 passed

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

Comment thread packages/web-shell/client/utils/localCommandQueue.ts Outdated
Comment thread packages/web-shell/client/utils/localCommandQueue.ts Outdated
Comment thread packages/web-shell/client/App.tsx Outdated
Comment thread packages/web-shell/client/App.tsx
wenshao and others added 4 commits August 4, 2026 20:12
…fo commands

Address PR review. A status dispatch finalizes the active assistant
block by default, so running /stats, /about or /context mid-turn would
fragment the streaming answer around the status card and drop later
usage frames. Add an optional clearActiveText flag to the status event
and pass false from these three command dispatches, covered by
reducer-level tests.

Also collapse the echo gate into a single body (the new helper now
delegates to appendOrDeferLocalUserMessage), add App-level wiring tests
for the responding/idle behavior of /stats and /about, and surface
failed getStats via reportError instead of swallowing them.
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

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

中文说明

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

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下:

No code change this round — every finding is already fixed at HEAD, re-verified with full checks

All four inline findings from the round-1 review run (anchored at 508e2b04) were implemented before this round ran, by the PR author's own commit 867249815 ("fix(web-shell): keep streaming assistant block intact for mid-turn info commands", committed 2026-08-04 12:12 UTC — three minutes before the reply comments below were posted). This round re-verified each fix against HEAD 7206e8e (which since merged main) and re-ran the trusted verification commands. Everything passes; there is nothing left to change, so this round commits nothing.

Point-by-point dispositions

1. rc:3709814016 — [Critical] mid-turn status dispatch finalizes the in-flight assistant block → already fixed in code, re-verified.

  • The exact suggested fix was taken: a clearActiveText opt-out on the status event (DaemonUiStatusEvent.clearActiveText in the SDK types), forwarded by the reducer's case 'status' / case 'debug' into appendStatusBlock, whose opts.clearActiveText !== false guard preserves the default finalize for daemon-emitted events.
  • All three converted entry points dispatch with clearActiveText: false: /context + status-bar indicator (showContextUsage, App.tsx:5899), /stats (App.tsx:7594), /status + /about (App.tsx:7667). No other dispatch site of these commands exists.
  • Reducer-level tests were added covering both the default finalize and the opt-out (including that a mid-stream assistant.usage frame now lands on the kept-active block): packages/sdk-typescript/test/daemon-ui-transcript.test.ts — 3/3 pass locally.

2. rc:3709814017 — [Suggestion] duplicated idle gate with inverted polarity → already fixed in code, re-verified.
appendLocalUserEchoIfIdle now delegates (return !appendOrDeferLocalUserMessage(isStreaming, text, undefined, sink)), so the idle gate lives in exactly one body; the doc comment states the inverted polarity explicitly. Helper suite 7/7 pass locally.

3. rc:3709814023 — [Suggestion] wiring untested at the App level → already fixed in code, re-verified.
New App read-only local commands mid-turn suite in App.test.tsx: for /stats and /about, a responding state asserts the command runs, the echo is NOT appended, and the dispatch carries clearActiveText: false; an idle state asserts the echo is appended. This kills the surviving-mutation scenario (inverted predicate) named in the finding. App suite 307/307 pass locally.

4. rc:3709814027 — [Suggestion] failed /stats fully silent → already fixed in code, re-verified.
.catch((error: unknown) => { reportError(error, 'Failed to load stats'); }) (App.tsx:7599-7601), matching showContextUsage's catch.

5. Review-level note "Test Plan (not a blocker)" — informational, no change warranted.
The PR body's numbers were the focused-suite counts at writing time: localCommandQueue.test.ts = 7 tests (still 7 — confirmed), App.test.tsx = 303 tests then (307 now, after the 4 wiring tests above — confirmed). The review's 2764 passed is a broader vitest scope, not a contradiction of the focused-suite claims.

6. Maintainer reply comments rc:3712312856 / rc:3712313081 / rc:3712313381 / rc:3712313647 — content not recoverable, no action taken from them.
Each of these four replies — one per finding thread — has a body that is only a reference to a file on the commenter's machine (@/tmp/autofix/reply-r1.mdreply-r4.md). The files do not exist on this runner or anywhere in the repository, so the intended text cannot be read by anyone viewing the PR. Given the same author's fix commit 867249815 landed three minutes earlier and covers all four findings point-for-point, these read as the reply artifacts of that local fix run posted by reference instead of by content. If any of them contained something beyond what that commit already implements, please repost the body text.

Thread-resolution note

Because this round commits nothing, the workflow's deterministic guard deliberately does not auto-resolve any review thread this round (threads are only resolved while the live PR head is the exact commit a round pushed and verified). The four finding threads therefore stay open even though all four findings are fixed at HEAD — a maintainer may resolve them directly, and any future round that pushes a commit can resolve them through the normal path.

Verification (commands actually run this round, at HEAD 7206e8e)

  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint — passed
  • vitest packages/sdk-typescript test/daemon-ui-transcript.test.ts (reducer, finding 1) — 3 passed
  • vitest packages/web-shell client/utils/localCommandQueue.test.ts (finding 2) — 7 passed
  • vitest packages/web-shell client/App.test.tsx (findings 3-4) — 307 passed
  • No commit this round, so no bundle/integration re-run and no settings-schema regeneration were applicable (no settings source touched; CI on 7206e8e is green).
中文说明

本轮无代码改动 —— 所有 finding 均已在 HEAD 修复,并已重新完整验证

第 1 轮 review(锚定于 508e2b04)提出的 4 条 inline finding,在本轮运行之前已由 PR 作者本人的提交 867249815("fix(web-shell): keep streaming assistant block intact for mid-turn info commands",提交于 2026-08-04 12:12 UTC——比下方的回复评论早 3 分钟)全部实现。本轮在 HEAD 7206e8e(其后已合入 main)上逐条复核了这些修复,并重新运行了可信验证命令。全部通过;没有需要改动的内容,因此本轮不产生任何提交。

逐条处理结论

1. rc:3709814016 —— [Critical] 回合中 status dispatch 会收尾正在流式的 assistant 块 → 已在代码中修复,复核通过。

  • 采用的正是建议中的修复:为 status 事件增加 clearActiveText 选项(SDK 类型中的 DaemonUiStatusEvent.clearActiveText),reducer 的 case 'status' / case 'debug' 将其转发给 appendStatusBlock;其中 opts.clearActiveText !== false 的守卫保证 daemon 发出的事件仍保持默认的收尾行为。
  • 三个被改造的入口全部以 clearActiveText: false 派发:/context + 状态栏指示器(showContextUsage,App.tsx:5899)、/stats(App.tsx:7594)、/status + /about(App.tsx:7667)。这些命令不存在其它派发点。
  • 已补充 reducer 级测试,同时覆盖默认收尾与选项豁免(包括流式中途到达的 assistant.usage 帧如今能落在保持活跃的块上):packages/sdk-typescript/test/daemon-ui-transcript.test.ts —— 本地 3/3 通过。

2. rc:3709814017 —— [Suggestion] 重复的空闲闸门且布尔极性相反 → 已在代码中修复,复核通过。
appendLocalUserEchoIfIdle 现改为委托实现(return !appendOrDeferLocalUserMessage(isStreaming, text, undefined, sink)),空闲闸门只存在于一处;doc 注释明确说明了相反的极性。helper 测试本地 7/7 通过。

3. rc:3709814023 —— [Suggestion] 接线缺少 App 层测试 → 已在代码中修复,复核通过。
App.test.tsx 新增 App read-only local commands mid-turn 套件:对 /stats/aboutresponding 状态断言命令执行、回显未被追加、dispatch 携带 clearActiveText: falseidle 状态断言回显被追加。这恰好杀死了 finding 指出的可存活变异(谓词反转)。App 套件本地 307/307 通过。

4. rc:3709814027 —— [Suggestion] /stats 失败完全静默 → 已在代码中修复,复核通过。
.catch((error: unknown) => { reportError(error, 'Failed to load stats'); })(App.tsx:7599-7601),与 showContextUsage 的 catch 保持一致。

5. review 级备注 "Test Plan (not a blocker)" —— 信息性说明,无需改动。
PR 描述中的数字是撰写时聚焦套件的测试数:localCommandQueue.test.ts = 7 个(现仍为 7,已确认)、App.test.tsx 当时 = 303 个(在新增上述 4 个接线测试后为 307,已确认)。review 观察到的 2764 passed 是更大范围的 vitest 运行结果,与聚焦套件的数字并不矛盾。

6. 维护者回复评论 rc:3712312856 / rc:3712313081 / rc:3712313381 / rc:3712313647 —— 内容无法恢复,未据此采取任何行动。
这 4 条回复分别位于 4 个 finding 线程内,但每条的正文都只是一个指向评论者本机文件的引用(@/tmp/autofix/reply-r1.mdreply-r4.md)。这些文件既不在本运行器上,也不在仓库中,任何查看 PR 的人都无法读到其真实内容。鉴于同一作者的修复提交 867249815 早 3 分钟落地、且逐点覆盖了全部 4 条 finding,这些评论看起来是该次本地修复运行生成的回复文件被以引用而非正文的形式贴出。若其中任何一条包含超出该提交已实现内容之外的意见,请重新贴出正文。

关于线程 resolve

由于本轮没有任何提交,工作流的确定性守卫在本轮不会自动 resolve 任何 review 线程(只有当 PR 的实时 head 恰为本轮推送并通过验证的提交时才会 resolve)。因此这 4 个 finding 线程将保持打开状态——尽管它们都已在 HEAD 修复;维护者可以直接手动 resolve,未来任何产生提交的轮次也可以通过正常路径 resolve。

验证(本轮在 HEAD 7206e8e 实际运行的命令)

  • npm run build — 通过
  • npm run typecheck — 通过
  • npm run lint — 通过
  • vitest packages/sdk-typescript test/daemon-ui-transcript.test.ts(reducer,对应 finding 1)— 3 通过
  • vitest packages/web-shell client/utils/localCommandQueue.test.ts(对应 finding 2)— 7 通过
  • vitest packages/web-shell client/App.test.tsx(对应 finding 3-4)— 307 通过
  • 本轮无提交,因此无需重跑 bundle/集成测试,也无需重新生成 settings schema(未触及 settings 源;7206e8e 的 CI 为绿色)。

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


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

@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. Not reviewed: verification and reverse audit — each ran and opened its brief, but neither was launched with the prompt the CLI built — the launches were written by hand, so the posted findings cannot be counted as verified, and what the agents were actually asked is not what this skill certifies. Test Plan (not a blocker): client/App.test.tsxno such file or directory; 7 tests pass — this review observed 1446, 2782 passed; 303 tests pass — this review observed 1446, 2782 passed.

中文说明

已审查。 建议见行内评论。 未审查:验证与反向审计——两者都运行并打开了各自的 brief,但都不是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的,发布的发现不能算作已验证,agent 实际被要求做的也不是本 skill 所认证的内容。 Test Plan(非阻断):client/App.test.tsxno such file or directory; 7 tests pass — this review observed 1446, 2782 passed; 303 tests pass — this review observed 1446, 2782 passed

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

Comment on lines +344 to +346
appendStatusBlock(next, event.type, event.text, event, {
clearActiveText: event.clearActiveText,
});

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] The clearActiveText: false opt-out skips the entire clearActiveText(state) call — including state.activeUserBlockId = undefined. All three new dispatch sites pass the flag unconditionally, also when idle. Pre-PR, an idle status dispatch reset activeUserBlockId; post-PR, running /stats, /about or /context (or clicking the status-bar context indicator) while idle leaves the local command echo block as the active user block indefinitely.

Failure scenario: Web Shell client B is idle; its user runs /stats → echo block E becomes activeUserBlockId; the result dispatch leaves the pointer at E. A peer client (TUI or a second web tab on the same daemon session) then submits a prompt: the bridge echo arrives as a mergeable user.text.delta (no sourceRecordIds, no qwenDiscreteMessage), canMergeTextDelta passes, and the peer's prompt text is appended onto E — rendering /stats<peer prompt> in one user block. Since applyTurnCollapse bounds turns by user messages, the peer's entire turn (assistant text, tool steps, token usage) groups under the /stats echo's turn, corrupting turn boundaries and per-turn metrics. Verified by a reducer probe at the reviewed commit: the PR arm merged into one user block (/statsfix the bug) where the pre-PR control arm produced separate user blocks.

Suggested fix — probe-verified (flips the repro back to separate blocks while keeping this PR's reducer tests green): in appendStatusBlock, keep the assistant/thought block but drop the user pointer on the opt-out path:

  appendBlock(state, block);
  if (opts.clearActiveText !== false) clearActiveText(state);
  else state.activeUserBlockId = undefined;

(Alternative: pass clearActiveText: false from App.tsx only while streaming — idle dispatches have no streaming block to protect.)

中文说明

clearActiveText: false 选项跳过了整个 clearActiveText(state) 调用——包括 state.activeUserBlockId = undefined。三处新的 dispatch 都无条件传入该标志,空闲时也是如此。本 PR 之前,空闲时的 status dispatch 会重置 activeUserBlockId;现在,空闲时运行 /stats/about/context(或点击状态栏的 context 指示器)会让本地命令回显块无限期地保持为活跃用户块。

失败场景:Web Shell 客户端 B 空闲时运行 /stats → 回显块 E 成为 activeUserBlockId;结果 dispatch 使指针一直停留在 E。此时同一 daemon 会话上的对端客户端(TUI 或第二个网页标签页)提交提示词:桥的回显以可合并的 user.text.delta 到达(无 sourceRecordIds、无 qwenDiscreteMessage),canMergeTextDelta 通过,对端的提示词文本被追加到 E 上——一个用户块渲染出 /stats<对端提示词>。由于 applyTurnCollapse 以用户消息为回合边界,对端的整个回合(assistant 文本、工具步骤、token 用量)都会归入 /stats 回显所在的回合,破坏回合边界与逐回合统计。已在被审提交上用 reducer 探针验证:PR 分支合并为一个用户块(/statsfix the bug),而 PR 前的对照组产生独立的用户块。

建议修复(已用探针验证——复现恢复为独立块,且本 PR 的 reducer 测试仍全绿):在 appendStatusBlock 中保留 assistant/thought 块,但在 opt-out 路径上清掉用户指针:

  appendBlock(state, block);
  if (opts.clearActiveText !== false) clearActiveText(state);
  else state.activeUserBlockId = undefined;

(备选方案:仅在流式时才从 App.tsx 传 clearActiveText: false——空闲 dispatch 没有需要保护的流式块。)

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

Comment on lines +5890 to 5892
echoLocalCommandIfIdle(commandText);
sessionActions
.getContextUsage({ detail })

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] /context — the third command this PR converts to mid-turn execution — has no test anywhere in web-shell for this new wiring (no references to getContextUsage/showContextUsage in any test file), while /stats and /about each got four new App-level tests. All three entry points (keyboard /context, the status-bar context indicator, and the in-chat context-detail click) funnel through showContextUsage.

Concrete cost: a future edit reverting showContextUsage to echoOrDeferLocalCommand(commandText) (suppressing /context mid-turn again) or dropping clearActiveText: false from its dispatch leaves the entire suite green — the exact regression this PR fixes would ship.

Suggested fix — mirror the new /stats tests in the App read-only local commands mid-turn describe:

// streamingState = 'responding': submit /context
//   -> expect getContextUsage called, appendLocalUserMessage NOT called,
//      dispatch receives objectContaining({ type: 'status', clearActiveText: false })
// streamingState = 'idle': submit /context -> expect the echo appended
中文说明

/context 是本 PR 改造为回合中执行的第三个命令,但 web-shell 中没有任何测试覆盖这段新接线(任何测试文件中都没有 getContextUsage/showContextUsage 的引用),而 /stats/about 各新增了四个 App 级测试。三个入口(键盘 /context、状态栏 context 指示器、聊天内 context 详情点击)都汇聚到 showContextUsage

具体代价:未来某次编辑把 showContextUsage 改回 echoOrDeferLocalCommand(commandText)(重新在回合中抑制 /context),或从其 dispatch 中删掉 clearActiveText: false,整个测试套件仍会全绿——本 PR 所修复的回归会原样合入。

建议修复——在 App read-only local commands mid-turn describe 中仿照新增的 /stats 测试:

// streamingState = 'responding':提交 /context
//   -> 断言 getContextUsage 被调用、appendLocalUserMessage 未被调用、
//      dispatch 收到 objectContaining({ type: 'status', clearActiveText: false })
// streamingState = 'idle':提交 /context -> 断言回显被追加

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

Comment on lines +7599 to +7601
.catch((error: unknown) => {
reportError(error, 'Failed to load stats');
});

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] This failure path changed from swallowed (.catch(() => {})) to reported via reportError, but no test pins the new reporting behaviour — there is no 'Failed to load stats' assertion and no getStats rejection mock in the suite.

Failure scenario: a revert to .catch(() => {}) passes the suite and load failures become invisible again (no toast, no console) — the very regression the previous review round fixed on this line. With the command no longer waiting for idle, getStats() rejection is now a realistic mid-turn race. The suite already establishes this guard pattern for /goal ("reports a failure to open a goal's session instead of swallowing it").

Suggested fix:

mockSessionActions.getStats.mockRejectedValueOnce(new Error('stats unavailable'));
// submit /stats, then assert the error reaches reportError (console.error/toast spy),
// matching the existing goal-failure test's style
中文说明

这条失败路径从被吞掉(.catch(() => {}))改为通过 reportError 上报,但没有任何测试固定新的上报行为——套件中既没有对 'Failed to load stats' 的断言,也没有 getStats 的 rejection mock。

失败场景:把代码改回 .catch(() => {}) 也能通过套件,加载失败会再次变得不可见(无 toast、无 console)——这正是上一轮评审在这行代码上修复的回归。由于命令不再等待空闲,getStats() reject 现在是现实的回合中竞态。套件中已有同样的守卫模式(/goal 的 "reports a failure to open a goal's session instead of swallowing it")。

建议修复:

mockSessionActions.getStats.mockRejectedValueOnce(new Error('stats unavailable'));
// 提交 /stats,然后断言错误到达了 reportError(console.error/toast spy),
// 与现有 goal 失败测试的写法保持一致

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

@doudouOUC doudouOUC 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 — no blockers. Suggestions are inline. Test Plan (not a blocker): client/App.test.tsxno such file or directory.

中文说明

已审查——无阻断问题。 建议见行内评论。 Test Plan(非阻断):client/App.test.tsxno such file or directory

— claude-opus-4-6 via Qwen Code /review (v0.21.5)

Comment on lines +7605 to 7607
echoLocalCommandIfIdle(text);
Promise.all([
workspaceActions.loadPreflight().catch(() => null),

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 /about handler's Promise.all(...).then(...) chain has no .catch(), while the adjacent /stats handler was explicitly updated in this same diff to use reportError. — Concrete cost: if serializeStatusMessage, store.dispatch, or resumeChatBottomFollow throws synchronously inside the .then() callback, the rejection is unhandled — no error toast, no console diagnostic, silent failure. The /about path is the lone outlier among the three read-only display commands after this diff.

Append .catch((error: unknown) => { reportError(error, 'Failed to load status info'); }) to the Promise chain, matching /stats and /context.

中文说明

/about 处理器的 Promise.all(...).then(...) 链缺少 .catch(),而相邻的 /stats 处理器在本 diff 中已显式更新为使用 reportError。— 具体代价:如果 .then() 回调内的 serializeStatusMessagestore.dispatchresumeChatBottomFollow 同步抛出异常,该 rejection 将无人处理——没有错误 toast、没有控制台诊断信息,静默失败。本 diff 后 /about 是三个只读显示命令中唯一的遗漏。

在 Promise 链末尾追加 .catch((error: unknown) => { reportError(error, 'Failed to load status info'); }),与 /stats/context 保持一致。

— claude-opus-4-6 via Qwen Code /review (v0.21.5)

…aming (QwenLM#8496)

The clearActiveText: false opt-out skipped the whole clearActiveText call,
leaving the local command echo as the active user block indefinitely. A
peer client's prompt echo then merged into it, corrupting turn boundaries.
Keep the streaming assistant/thought block open on the opt-out path but
still drop the user pointer.

Also report /about load failures like /stats and /context already do, and
pin the new /context wiring plus the /stats failure path with tests.
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

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

Round summary

Addressed all four inline findings from this review round: one Critical and three Suggestions. Committed as 8782aad1f on top of the PR head.

Feedback and dispositions

  1. [Critical] clearActiveText: false opt-out leaves the command echo as the active user block (rc:3714828018)Fixed. Verified against the reviewed code: the opt-out skipped the entire clearActiveText(state) call, including state.activeUserBlockId = undefined, so an idle /stats, /about or /context dispatch left the local echo block mergeable, and a peer client's prompt echo (a mergeable user.text.delta with no sourceRecordIds) would append onto it — /stats<peer prompt> in one user block, corrupting applyTurnCollapse turn boundaries. Applied the reviewer's suggested reducer fix: on the opt-out path appendStatusBlock still keeps the streaming assistant/thought block open but now resets state.activeUserBlockId. This also applies to the pre-existing trimmed-tool-notification opt-out, matching the flag's documented intent (protect the assistant/thought block only). Added a regression test reproducing the probe scenario: after a clearActiveText: false status dispatch the user pointer is cleared, and a following mergeable user.text.delta opens its own user block instead of merging into the echo.
  2. [Suggestion] no test covers the new /context wiring (rc:3714828024)Fixed. Added getContextUsage to the mocked session actions (mock factory plus the beforeEach reset) and two tests in the App read-only local commands mid-turn describe mirroring the /stats pair: mid-turn /context runs immediately (getContextUsage called, echo skipped, dispatch carries { type: 'status', clearActiveText: false }), and idle /context appends the echo.
  3. [Suggestion] /stats failure reporting is unpinned (rc:3714828033)Fixed. Added a test that makes getStats reject and asserts the failure reaches reportError (console.error('[web-shell]', ...) with the error message), following the existing goal-failure guard test's style. Reverting to .catch(() => {}) now fails the suite.
  4. [Suggestion] /about Promise chain has no .catch() (rc:3714914972)Fixed. Appended .catch((error: unknown) => { reportError(error, 'Failed to load status info'); }) to the chain, matching /stats and /context; reportError was already a dependency of the enclosing callback. Adding the chain tail made Prettier re-indent the /about .then body (member-chain style) — a formatting-only change.

The review-level notes ("verification and reverse audit were not launched with the CLI-built prompt"; Test Plan client/App.test.tsxno such file or directory) are process observations, not code findings: the file lives at packages/web-shell/client/App.test.tsx and passes when run from the package directory. No code action.

Conflict

None (--conflict false; no merge performed).

Verification

  • cd packages/sdk-typescript && npx vitest run test/daemon-ui-transcript.test.ts — 4 passed (includes the new regression test)
  • cd packages/sdk-typescript && npx vitest run test/unit/daemonUi.test.ts — 279 passed (covers the pre-existing trimmed-tool opt-out path)
  • cd packages/sdk-typescript && npx vitest run — 31 files, 1447 passed (reviewer-observed baseline 1446; +1 new test)
  • cd packages/web-shell && npx vitest run client/App.test.tsx -t "read-only local commands mid-turn" — 7 passed (4 existing + 3 new)
  • cd packages/web-shell && npx vitest run client/App.test.tsx — 310 passed
  • cd packages/web-shell && npx vitest run — 166 files, 2785 passed (reviewer-observed baseline 2782; +3 new tests)
  • npm run build — passed (exit 0)
  • npm run typecheck — passed (exit 0)
  • npm run lint — passed (exit 0)
  • npx prettier --check on the four touched files — clean
  • Integration tests: not run — the touched behavior is exercised by unit tests in both packages, not only through the bundled CLI or the integration harness.
中文说明

本轮总结

本轮处理了全部四条行内评审发现:一条 Critical、三条 Suggestion。已以 8782aad1f 提交到 PR head 之上。

反馈与处理

  1. [Critical] clearActiveText: false 选项使命令回显块残留为活跃用户块(rc:3714828018)——已修复。 已在被审代码上核实:该选项跳过了整个 clearActiveText(state) 调用(包括 state.activeUserBlockId = undefined),导致空闲时运行 /stats/about/context 后本地回显块仍可被合并;此时对端客户端的提示词回显(无 sourceRecordIds 的可合并 user.text.delta)会追加到该块上——一个用户块渲染出 /stats<对端提示词>,破坏 applyTurnCollapse 的回合边界。采用评审建议的 reducer 修复:opt-out 路径上 appendStatusBlock 仍保持流式 assistant/thought 块打开,但现在会重置 state.activeUserBlockId。该修复同样适用于既有的"工具输出被裁剪"通知 opt-out 路径,符合该标志的文档语义(仅保护 assistant/thought 块)。新增回归测试复现探针场景:clearActiveText: false 的 status dispatch 之后用户指针被清空,随后的可合并 user.text.delta 会新建自己的用户块,而不是并入回显块。
  2. [Suggestion] 新 /context 接线没有任何测试覆盖(rc:3714828024)——已修复。 在 mock 的 session actions 中补充 getContextUsage(mock 工厂 + beforeEach 重置各一处),并在 App read-only local commands mid-turn describe 中仿照 /stats 新增两个测试:回合中提交 /context 立即执行(getContextUsage 被调用、跳过回显、dispatch 携带 { type: 'status', clearActiveText: false });空闲时提交 /context 追加回显。
  3. [Suggestion] /stats 失败上报未被测试固定(rc:3714828033)——已修复。 新增测试:让 getStats reject,断言失败到达 reportErrorconsole.error('[web-shell]', ...) 且包含错误信息),写法与既有的 goal 失败守卫测试一致。若改回 .catch(() => {}),套件将失败。
  4. [Suggestion] /about 的 Promise 链缺少 .catch()(rc:3714914972)——已修复。 在链尾追加 .catch((error: unknown) => { reportError(error, 'Failed to load status info'); }),与 /stats/context 保持一致;reportError 本就在外层回调的依赖数组中。链尾的新增使 Prettier 按成员链风格对 /about.then 主体重新缩进——纯格式变更。

评审层面的备注("verification 与 reverse audit 不是用 CLI 构建的 prompt 启动的";Test Plan 中 client/App.test.tsxno such file or directory)属于流程观察,不是代码发现:该文件位于 packages/web-shell/client/App.test.tsx,在包目录下运行全部通过。无需代码改动。

冲突

无(--conflict false;未执行合并)。

验证

  • cd packages/sdk-typescript && npx vitest run test/daemon-ui-transcript.test.ts — 4 通过(含新增回归测试)
  • cd packages/sdk-typescript && npx vitest run test/unit/daemonUi.test.ts — 279 通过(覆盖既有的"工具输出被裁剪" opt-out 路径)
  • cd packages/sdk-typescript && npx vitest run — 31 个文件、1447 通过(评审观察到的基线为 1446;新增 1 个测试)
  • cd packages/web-shell && npx vitest run client/App.test.tsx -t "read-only local commands mid-turn" — 7 通过(4 个既有 + 3 个新增)
  • cd packages/web-shell && npx vitest run client/App.test.tsx — 310 通过
  • cd packages/web-shell && npx vitest run — 166 个文件、2785 通过(评审观察到的基线为 2782;新增 3 个测试)
  • npm run build — 通过(exit 0)
  • npm run typecheck — 通过(exit 0)
  • npm run lint — 通过(exit 0)
  • 对四个改动文件运行 npx prettier --check — 无格式问题
  • 集成测试:未运行——本次改动的行为由两个包的单元测试覆盖,并非只能通过打包后的 CLI 或集成测试框架验证。

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

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


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

@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 — no blockers. Suggestions are inline. Test Plan (not a blocker): client/App.test.tsxno such file or directory; 7 tests pass — this review observed 1448, 2785 passed; 303 tests pass — this review observed 1448, 2785 passed.

中文说明

已审查——无阻断问题。 建议见行内评论。 Test Plan(非阻断):client/App.test.tsxno such file or directory; 7 tests pass — this review observed 1448, 2785 passed; 303 tests pass — this review observed 1448, 2785 passed

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

Comment thread packages/web-shell/client/App.tsx Outdated
Comment on lines +7593 to +7594
// Mid-turn runs must not finalize the streaming block.
clearActiveText: false,

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 read-only command result dispatch — store.dispatch([{ type: 'status', text, ..., clearActiveText: false }]) followed by resumeChatBottomFollow('smooth') — is duplicated verbatim at the three sites this diff adds (showContextUsage ~5899, /stats ~7594, /status|about ~7669), each copy carrying the load-bearing flag. — Concrete cost: the invariant "a read-only result dispatch must pass clearActiveText: false" is enforced only by convention at three sites. When a fourth read-only command gains mid-turn support, or one of the copies is edited and drops the flag, the in-flight assistant/thought block silently finalizes mid-stream — the exact regression this PR exists to fix (the streaming answer splits at the status block; subsequent assistant.usage frames no longer attach to the active block). Suggested fix — centralize the flag + follow-resume pair in one callback next to echoLocalCommandIfIdle, called from all three .then(...) bodies (serializers stay at the call sites):

const dispatchReadOnlyStatus = useCallback(
  (text: string) => {
    store.dispatch([{ type: 'status', text, clearActiveText: false }]);
    resumeChatBottomFollow('smooth');
  },
  [store, resumeChatBottomFollow],
);
中文说明

只读命令的结果派发——store.dispatch([{ type: 'status', text, ..., clearActiveText: false }]) 后跟 resumeChatBottomFollow('smooth')——在本 diff 新增的三处调用点(showContextUsage ~5899、/stats ~7594、/status|about ~7669)被逐字复制,每处都携带这个关键标志。具体代价:「只读结果派发必须传 clearActiveText: false」这一不变量仅靠三处调用点的约定维持。当第四个只读命令获得回合中执行支持、或某一处副本被编辑而漏掉该标志时,会在流式中途悄悄收尾正在进行的 assistant/thought 块——正是本 PR 要修复的回归(流式回答在 status 块处被切断,后续 assistant.usage 帧不再挂到活跃块上)。建议修复——把「标志 + 恢复底部跟随」收进 echoLocalCommandIfIdle 旁的一个回调中(见英文部分代码),三个 .then(...) 主体统一调用它,序列化仍留在各调用点。

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

Comment on lines +7674 to +7676
.catch((error: unknown) => {
reportError(error, 'Failed to load status info');
});

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] This new /about (/status) error-handling branch has no test, while the sibling new /stats catch is pinned by 'reports /stats load failures instead of swallowing them'. — Failure scenario: if a follow-up change drops this catch or replaces reportError with a swallow, the suite stays green and a failing /about regresses to an unhandled promise rejection with zero user feedback — the exact pre-PR behaviour this diff fixes for /stats only. Note: each Promise.all member catches to null, so this catch is reachable only via a throw inside the .then body — mirror the /stats failure test by making collectSystemInfo/serialization throw once, then assert console.error is called with '[web-shell]' and a message containing 'Failed to load status info'.

中文说明

新增的 /about/status)错误捕获没有测试覆盖,而同期新增的 /stats 捕获已有 'reports /stats load failures instead of swallowing them' 测试固定。失败场景:后续改动若删除此 catch 或将 reportError 换回静默吞掉,测试套件仍为绿色,/about 失败会退化为无任何用户反馈的未处理 promise rejection——正是本 diff 仅为 /stats 修复的改动前行为。注意:Promise.all 的每个成员都已 .catch(() => null),因此这个 catch 只能通过 .then 主体内部抛错来触达——测试应仿照 /stats 失败用例,让 collectSystemInfo/序列化过程抛错一次,然后断言 console.error'[web-shell]' 和包含 'Failed to load status info' 的消息被调用。

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

QwenLM#8496)

Address the round-3 review feedback on the mid-turn read-only commands:

- The read-only result dispatch (status block with clearActiveText:
  false plus the follow-resume) was copied verbatim at the /context,
  /stats, and /about sites, leaving the load-bearing flag enforced by
  convention at three places. Centralize it in one
  dispatchReadOnlyStatus callback next to echoLocalCommandIfIdle; the
  three .then bodies now call it with their serialized text.
- Pin the /about catch the way the sibling /stats catch is pinned:
  make collectSystemInfo throw once and assert the failure surfaces
  through console.error instead of becoming an unhandled rejection
  with zero user feedback.
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

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

Round summary

Round 3 on PR #8496 raised two inline Suggestions from the automated reviewer; both are resolved in commit 548879c3a on branch feat/web-shell-mid-turn-info-commands. No base conflict (--conflict false, no merge performed).

Feedback points and decisions

  1. [Suggestion] rc:3715931383 — duplicated read-only result dispatch (App.tsx ~7594) — RESOLVED.
    Verified: the dispatch of a status block carrying the load-bearing clearActiveText: false flag followed by resumeChatBottomFollow('smooth') was copied verbatim at the three sites this PR added (showContextUsage for /context, /stats, /status|about). Centralized the flag + follow-resume pair in one dispatchReadOnlyStatus callback placed next to echoLocalCommandIfIdle; the three .then(...) bodies now call it with their serialized text (serializers stay at the call sites, as suggested). The flag's "why" comment now lives once on that callback instead of three times at the call sites, and the change is a net 12-line shrink. Dependency arrays updated accordingly: showContextUsage drops store/resumeChatBottomFollow (no longer referenced in its body), and handleSubmit gains dispatchReadOnlyStatus while keeping store/resumeChatBottomFollow for its other, non-read-only dispatches (/tools, /bug, etc.). Existing tests pin the dispatched shape ({ type: 'status', clearActiveText: false }) and still pass unchanged.

  2. [Suggestion] rc:3715931394 — no test for the new /about (/status) error branch (App.tsx ~7676) — RESOLVED.
    Added 'reports /about load failures instead of swallowing them', mirroring the sibling 'reports /stats load failures instead of swallowing them' pin. As the finding notes, each Promise.all member catches to null, so the catch is reachable only via a throw inside the .then body: the test mocks ./utils/systemInfo so collectSystemInfo throws once, then asserts console.error is called with '[web-shell]' and the failure message. One deliberate deviation from the suggested assertion text: formatError prefers error.message over the fallback whenever the thrown value is an Error instance, so the assertion pins the thrown message ('status unavailable') rather than the 'Failed to load status info' fallback — the same shape as the /stats test, and it pins the identical catch → reportError wiring.
    The mock follows this test file's conventions: a hoisted vi.fn(), a full module replacement (App.tsx imports only collectSystemInfo from that module), and a default re-pinned in beforeEach (an all-empty SystemInfo, exactly what the real function returns for the default null preflight/env) — required because afterEach runs vi.restoreAllMocks(), which wipes any factory-set implementation.

No finding was declined, deferred, or escalated.

Verification

  • npx prettier --check packages/web-shell/client/App.tsx packages/web-shell/client/App.test.tsx — passed
  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint (eslint . --ext .ts,.tsx && eslint integration-tests) — passed
  • vitest packages/web-shell (client/App.test.tsx + client/utils/localCommandQueue.test.ts) — 318 passed, 2 files; re-run on the committed tree after the pre-commit hook — 318 passed
  • vitest packages/web-shell (full package suite) — 2786 passed, 166 files
  • git commit pre-commit hook (lint-staged) — passed

Integration tests after npm run bundle: not run — the touched behavior is web-shell client dispatch logic, exercised by the package's unit harness, not only through the bundled CLI or integration harness. npm run generate:settings-schema: not needed — no settings source changed.

中文说明

本轮概要

PR #8496 的第 3 轮审查中,自动审查器提出了两条行内建议,均已在分支 feat/web-shell-mid-turn-info-commands 的提交 548879c3a 中解决。无基线冲突(--conflict false,未执行合并)。

反馈点与处理决定

  1. [建议] rc:3715931383 — 重复的只读结果派发(App.tsx ~7594) — 已解决。
    已核实:携带关键标志 clearActiveText: falsestatus 块派发、以及随后的 resumeChatBottomFollow('smooth'),在本 PR 新增的三处调用点(/contextshowContextUsage/stats/status|about)被逐字复制。现将「标志 + 恢复底部跟随」收进一个 dispatchReadOnlyStatus 回调,置于 echoLocalCommandIfIdle 旁;三个 .then(...) 主体改为以各自的序列化文本调用它(按建议,序列化仍留在各调用点)。该标志的 "why" 注释如今只存在于这个回调上,不再在三处调用点重复,整体净减少 12 行。依赖数组同步更新:showContextUsage 移除 store/resumeChatBottomFollow(其函数体不再引用它们),handleSubmit 新增 dispatchReadOnlyStatus,同时保留 store/resumeChatBottomFollow(其余非只读派发如 /tools/bug 仍在使用)。现有测试固定了派发形状({ type: 'status', clearActiveText: false }),未改动仍全部通过。

  2. [建议] rc:3715931394 — 新增 /about/status)错误分支缺少测试(App.tsx ~7676) — 已解决。
    新增 'reports /about load failures instead of swallowing them',仿照同期的 'reports /stats load failures instead of swallowing them' 固定用例。正如该发现所述,Promise.all 的每个成员都已 .catch(() => null),因此这个 catch 只能通过 .then 主体内部抛错来触达:测试 mock 了 ./utils/systemInfo,让 collectSystemInfo 抛错一次,然后断言 console.error'[web-shell]' 和失败消息被调用。相对建议中的断言文本有一处刻意偏差:只要抛出值是 Error 实例,formatError 就会优先使用 error.message 而非兜底文案,因此断言固定的是抛出的消息('status unavailable')而不是 'Failed to load status info' 兜底文案——与 /stats 测试同形,且固定的是同一条 catch → reportError 链路。
    该 mock 遵循本测试文件的惯例:一个 hoisted 的 vi.fn()、整体模块替换(App.tsx 仅从该模块导入 collectSystemInfo),并在 beforeEach 中重新固定默认实现(全空的 SystemInfo,与真实函数在默认 null preflight/env 下的返回完全一致)——这是必须的,因为 afterEach 会执行 vi.restoreAllMocks(),清除工厂中设置的任何实现。

没有发现被拒绝、推迟或升级处理。

验证

  • npx prettier --check packages/web-shell/client/App.tsx packages/web-shell/client/App.test.tsx — 通过
  • npm run build — 通过
  • npm run typecheck — 通过
  • npm run linteslint . --ext .ts,.tsx && eslint integration-tests)— 通过
  • vitest packages/web-shellclient/App.test.tsx + client/utils/localCommandQueue.test.ts)— 318 通过,2 个文件;pre-commit 钩子后在已提交树上重跑 — 318 通过
  • vitest packages/web-shell(整包测试套件)— 2786 通过,166 个文件
  • git commit pre-commit 钩子(lint-staged)— 通过

npm run bundle 后的集成测试:未运行——所触及的行为是 web-shell 客户端派发逻辑,由该包的单元测试覆盖,并非只能通过打包后的 CLI 或集成测试框架验证。npm run generate:settings-schema:无需运行——未改动任何 settings 源。

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

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


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

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run.

中文说明

🔀 已更新 base:红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main,CI 将重新运行。

@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 — no blockers. Suggestions are inline. Test Plan (not a blocker): client/App.test.tsxno such file or directory; 7 tests pass — this review observed 1447, 2794 passed; 303 tests pass — this review observed 1447, 2794 passed.

中文说明

已审查——无阻断问题。 建议见行内评论。 Test Plan(非阻断):client/App.test.tsxno such file or directory; 7 tests pass — this review observed 1447, 2794 passed; 303 tests pass — this review observed 1447, 2794 passed

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

]);
});

it('keeps the assistant block active when clearActiveText is false', () => {

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] R4-3: The clearActiveText: false opt-out is only tested with an active assistant block; the thought-block half of the documented contract (types.ts: "without finalizing the active assistant/thought block") is unpinned — all three new reducer tests use assistant.text.delta only. — Failure scenario: named surviving mutation — adding clearActiveThought(state); to the reducer's opt-out else-branch keeps the entire sdk-typescript suite green (verified by applying the mutation: 1449 tests still pass), while a mid-turn status insertion would then finalize a streaming thought block (a thinking model still streaming thought when the user runs /stats), splitting it around the command output — the exact regression the flag exists to prevent. Suggested fix: add a mirror test with thought events.

it('keeps the thought block active when clearActiveText is false', () => {
  // user.text.delta → thought.text.delta → { type: 'status', clearActiveText: false }
  // → thought.text.delta → assistant.done
  // assert: a single thought block with merged text, still active after the status event
});
中文说明

clearActiveText: false 选项目前只用「活跃的 assistant 块」场景测试过;文档约定(types.ts:"不会收尾活跃的 assistant/thought 块")中 thought 块那一半没有任何测试固定——三条新 reducer 测试全部只使用 assistant.text.delta。失败场景:已点名的可存活变异——在 reducer 的 opt-out else 分支中加入 clearActiveThought(state);,整个 sdk-typescript 套件仍然全绿(已实际施加该变异验证:1449 个测试全部通过),而回合中插入 status 块此时会收尾正在流式的 thought 块(思考模型仍在流式输出 thought 时用户运行 /stats),把思考内容切成围绕命令输出的碎片——这正是该标志要避免的回归。建议修复:补充一个 thought 事件的镜像测试。

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

});

describe('App read-only local commands mid-turn', () => {
it('runs /stats immediately while streaming and skips the echo', async () => {

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] R4-4: All three mid-turn tests pin only the dispatch envelope (type + clearActiveText), never the text payload — and nothing else pins it: serializeStatsMessage / serializeStatusMessage / serializeContextUsageMessage have zero unit tests, and the fully mocked store means no render-level assertion sees the text either. — Failure scenario: named surviving mutation — replacing dispatchReadOnlyStatus(serializeStatsMessage(result, statsView)) (and the /about / /context equivalents) with dispatchReadOnlyStatus('') keeps all eight new tests green while /stats//about//context render empty status blocks mid-turn — the user-visible output of this feature ships untested. Suggested fix: pin the serialized payload in at least one test per command, e.g.

expect(mockStore.dispatch).toHaveBeenCalledWith([
  expect.objectContaining({
    type: 'status',
    clearActiveText: false,
    text: serializeStatsMessage(statsFixture, 'summary'),
  }),
]);
中文说明

三条回合中测试都只固定了 dispatch 的外壳(type + clearActiveText),从未固定 text 载荷——也没有其它测试固定它:serializeStatsMessage / serializeStatusMessage / serializeContextUsageMessage 没有任何单元测试,且 store 被完全 mock,渲染层断言同样看不到这段文本。失败场景:已点名的可存活变异——把 dispatchReadOnlyStatus(serializeStatsMessage(result, statsView))(以及 /about/context 的对应调用)替换为 dispatchReadOnlyStatus(''),全部 8 个新测试仍然通过,而回合中 /stats//about//context 会渲染出空的 status 块——本功能的用户可见输出在无人测试的情况下上线。建议修复:每条命令至少在一个测试中固定序列化后的载荷,例如断言 text 等于对已知 fixture 调用序列化函数的结果(或至少包含某个已知字段)。

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

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

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

Review feedback round — summary

Two inline suggestions from the automated reviewer (round 4), both naming a
concrete surviving mutation in this PR's new tests. Both are addressed with
test-only changes; no production code was modified. No conflicts
(--conflict false; no merge performed).

R4-3 — thought-block half of the clearActiveText: false contract unpinned

Decision: addressed (packages/sdk-typescript/test/daemon-ui-transcript.test.ts).
Added the mirror test: user.text.delta → thought.text.delta → status (clearActiveText: false) → thought.text.delta → assistant.done must keep a
single thought block with merged text (thinking more), so the documented
"without finalizing the active assistant/thought block" contract is now
pinned for thought blocks too. Verified against the reviewer's named
mutation: adding clearActiveThought(state) to the reducer's opt-out
else-branch makes the new test fail (1 failed | 4 passed); reverting it
passes.

R4-4 — mid-turn tests pin only the dispatch envelope, never the text payload

Decision: addressed (packages/web-shell/client/App.test.tsx). The three
mid-turn tests now pin the serialized payload:

  • /stats resolves a typed DaemonSessionStatsStatus fixture and asserts
    text: serializeStatsMessage(fixture, 'overview');
  • /about asserts text: serializeStatusMessage({ ... }) with the exact
    StatusInfo the handler builds from the mocked connection/system info
    (cliVersion 1.2.3, model qwen, sessionId session-1, empty runtime
    fields);
  • /context resolves a typed DaemonSessionContextUsageStatus fixture and
    asserts text: serializeContextUsageMessage(fixture).

Verified against the reviewer's named mutation: replacing all three
dispatchReadOnlyStatus(serialize...) calls with dispatchReadOnlyStatus('')
fails exactly those three tests (3 failed | 5 passed); reverting passes.

Review-level Test Plan note (not a blocker)

The review's Test Plan note flagged a stale relative path and stale test
counts in the PR's Test Plan text. Informational only — the reviewer's own
run observed the real suites (1447 sdk-typescript / 2794 web-shell); no code
change involved.

Verification

Commands actually run this round (all from the repository checkout):

  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint — passed
  • npx prettier --check on the two changed files — passed
  • npx vitest run test/daemon-ui-transcript.test.ts (sdk-typescript, touched file) — 5 passed
  • npx vitest run (sdk-typescript package, touched) — 31 files, 1448 tests passed
  • npx vitest run client/App.test.tsx -t 'mid-turn' (web-shell, touched area) — 8 passed
  • npx vitest run (web-shell package, touched) — 166 files, 2794 tests passed
  • Mutation checks: R4-3's named mutation fails the new sdk test; R4-4's named
    mutation (all three commands) fails exactly the three mid-turn tests; both
    mutations reverted before committing
  • Commit: eb6f075f6 test: pin thought opt-out and serialized mid-turn status payloads (#8496) (test-only: 2 files, +101/−3)
中文说明

审查反馈轮次——总结

自动审查者(第 4 轮)提出两条行内建议,均点名了本 PR 新增测试中一个具体可存活的变异。两条均已处理,且只改测试、未改动任何生产代码。无冲突(--conflict false,未执行合并)。

R4-3 — clearActiveText: false 约定中 thought 块那一半未被固定

决定:已处理packages/sdk-typescript/test/daemon-ui-transcript.test.ts)。新增镜像测试:user.text.delta → thought.text.delta → status (clearActiveText: false) → thought.text.delta → assistant.done 必须保持单个 thought 块且文本合并(thinking more),从而把文档约定「不会收尾活跃的 assistant/thought 块」中 thought 块那一半也固定下来。已按审查者点名的变异验证:在 reducer 的 opt-out else 分支中加入 clearActiveThought(state) 会使新测试失败(1 failed | 4 passed);还原后通过。

R4-4 — 回合中测试只固定 dispatch 外壳,从未固定 text 载荷

决定:已处理packages/web-shell/client/App.test.tsx)。三条回合中测试现在都固定了序列化后的载荷:

  • /stats 返回一个类型完整的 DaemonSessionStatsStatus fixture,并断言 text: serializeStatsMessage(fixture, 'overview')
  • /about 断言 text: serializeStatusMessage({ ... }),其 StatusInfo 与 handler 基于 mock 的连接/系统信息构造出的完全一致(cliVersion 1.2.3、model qwen、sessionId session-1,运行时字段为空);
  • /context 返回一个类型完整的 DaemonSessionContextUsageStatus fixture,并断言 text: serializeContextUsageMessage(fixture)

已按审查者点名的变异验证:把三处 dispatchReadOnlyStatus(serialize...) 全部替换为 dispatchReadOnlyStatus(''),恰好使这三条测试失败(3 failed | 5 passed);还原后通过。

审查级 Test Plan 备注(非阻断)

审查的 Test Plan 备注指出 PR 描述中测试计划的路径与测试数量已过时。仅供参考——审查者自己的运行已观察到真实套件结果(sdk-typescript 1447 / web-shell 2794),不涉及代码改动。

验证

本轮实际执行的命令(均在仓库检出目录中运行):

  • npm run build — 通过
  • npm run typecheck — 通过
  • npm run lint — 通过
  • 对两个改动文件执行 npx prettier --check — 通过
  • npx vitest run test/daemon-ui-transcript.test.ts(sdk-typescript,改动文件)— 5 通过
  • npx vitest run(sdk-typescript 包,改动包)— 31 个文件、1448 个测试全部通过
  • npx vitest run client/App.test.tsx -t 'mid-turn'(web-shell,改动区域)— 8 通过
  • npx vitest run(web-shell 包,改动包)— 166 个文件、2794 个测试全部通过
  • 变异检查:R4-3 点名变异会使新增 sdk 测试失败;R4-4 点名变异(三条命令全部)恰好使三条回合中测试失败;两个变异均已在提交前还原
  • 提交:eb6f075f6 test: pin thought opt-out and serialized mid-turn status payloads (#8496)(仅测试:2 个文件,+101/−3)

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

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


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

@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. Not reviewed: verification — a verifier ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and the posted findings cannot be counted as verified against it. Test Plan (not a blocker): client/App.test.tsxno such file or directory; 7 tests pass — this review observed 1448, 2794 passed; 303 tests pass — this review observed 1448, 2794 passed.

中文说明

已审查。 建议见行内评论。 未审查:验证——有验证 agent 运行并打开了自己的 brief,但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的,发布的发现不能算作经它验证。 Test Plan(非阻断):client/App.test.tsxno such file or directory; 7 tests pass — this review observed 1448, 2794 passed; 303 tests pass — this review observed 1448, 2794 passed

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

Comment thread packages/web-shell/client/App.tsx Outdated
Comment on lines 5897 to 5898
// is revealed even when the click comes while scrolled up.
const showContextUsage = useCallback(

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 doc comment above showContextUsage still attributes the panel reveal to the local echo ("Echoes the command as a local user message first — that also makes the transcript follow the tail (MessageList Rule 4), so the panel is revealed even when the click comes while scrolled up") — but this diff makes the echo conditional: mid-turn echoLocalCommandIfIdle skips it, and the reveal then depends entirely on resumeChatBottomFollow('smooth') inside the new dispatchReadOnlyStatus. Verified at this commit: MessageList Rule 4 fires only on a new user message (absent mid-turn), and the auto-scroll driver does not scroll while the user is scrolled up (followPausedByUserRef) — so the comment's causal chain is wrong exactly for the case it promises. The comment inside the function was updated; this one was not. — Failure scenario: a maintainer refactoring dispatchReadOnlyStatus reads this comment, concludes the echo row already guarantees tail-follow/reveal, and removes the resumeChatBottomFollow call; a mid-turn click on the status-bar context indicator while scrolled up then renders the /context result off-screen with no scroll.

Suggested fix (the two preceding comment lines sit outside this hunk — update them too):

  // Shared by the /context slash command and the status-bar context
  // indicator. Echoes the command when idle — that also makes the transcript
  // follow the tail (MessageList Rule 4). Mid-turn the echo is skipped and
  // dispatchReadOnlyStatus's resumeChatBottomFollow resumes bottom-follow,
  // so the panel is revealed even when the click comes while scrolled up.
中文说明

[建议] showContextUsage 上方的文档注释仍然把面板显示归因于本地回显("Echoes the command as a local user message first — that also makes the transcript follow the tail (MessageList Rule 4), so the panel is revealed even when the click comes while scrolled up")——但本 diff 使回显变成有条件的:回合中 echoLocalCommandIfIdle 会跳过回显,此时面板显示完全依赖新增 dispatchReadOnlyStatus 内的 resumeChatBottomFollow('smooth')。已在该提交上核实:MessageList Rule 4 只在出现新用户消息时触发(回合中不存在),且自动滚动驱动在用户向上滚动时(followPausedByUserRef)不会滚动——因此注释描述的因果链恰恰在它承诺的场景下已不成立。函数内部的注释已更新,这一处没有。——失败场景:未来重构 dispatchReadOnlyStatus 的维护者读到这条注释,误以为回显行已保证跟随尾部/显示面板,从而移除 resumeChatBottomFollow 调用;此后在向上滚动状态下于回合中点击状态栏的 context 指示器,/context 结果会渲染在屏幕外且不滚动。

建议修复(上面两行注释位于本 hunk 之外,需一并更新):见英文部分代码块。

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

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

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

中文说明

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

@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 — no blockers. Suggestions are inline. Test Plan (not a blocker): client/App.test.tsxno such file or directory; 7 tests pass — this review observed 1452, 2794 passed; 303 tests pass — this review observed 1452, 2794 passed.

中文说明

已审查——无阻断问题。 建议见行内评论。 Test Plan(非阻断):client/App.test.tsxno such file or directory; 7 tests pass — this review observed 1452, 2794 passed; 303 tests pass — this review observed 1452, 2794 passed

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

Comment on lines +5017 to +5024
const echoLocalCommandIfIdle = useCallback(
(text: string): void => {
appendLocalUserEchoIfIdle(streamingStateRef.current !== 'idle', text, {
append: (value: string) => store.appendLocalUserMessage(value),
});
},
[store],
);

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] echoLocalCommandIfIdle duplicates the adjacent echoOrDeferLocalCommand's idle gate (streamingStateRef.current !== 'idle') and echo-sink construction; for the echo side effect it is behaviourally identical to void echoOrDeferLocalCommand(text). The new appendLocalUserEchoIfIdle util's inverted-polarity boolean return also has no production reader — only its own unit tests read it, and its JSDoc overstates the consolidation (the gate expression is still duplicated at the callback layer). — Concrete cost: the idle gate and echo sink now live in two adjacent callbacks, so any future change to either (a new busy state, image support, or telemetry on the local echo) must touch both; missing one silently diverges the deferred-command path from the read-only path.

Suggested change
const echoLocalCommandIfIdle = useCallback(
(text: string): void => {
appendLocalUserEchoIfIdle(streamingStateRef.current !== 'idle', text, {
append: (value: string) => store.appendLocalUserMessage(value),
});
},
[store],
);
const echoLocalCommandIfIdle = useCallback(
(text: string): void => {
void echoOrDeferLocalCommand(text);
},
[echoOrDeferLocalCommand],
);

This also makes appendLocalUserEchoIfIdle (and its test block) unnecessary; if the named util is kept, at least drop the unread inverted return and return void.

中文说明

echoLocalCommandIfIdle 重复了相邻 echoOrDeferLocalCommand 的空闲闸门(streamingStateRef.current !== 'idle')与回显 sink 构造;就回显副作用而言,它与 void echoOrDeferLocalCommand(text) 行为完全等价。新增的 appendLocalUserEchoIfIdle 工具函数的反向极性布尔返回值在生产代码中也没有任何读取者——只有它自己的单元测试读取该返回值,且其 JSDoc 夸大了整合程度(闸门表达式在 callback 层仍然是重复的)。具体代价:空闲闸门与回显 sink 现在存在于两个相邻 callback 中,未来对任何一方的修改(新的忙碌状态、图片支持、本地回显遥测等)都必须同时修改两处;遗漏其一会让延迟命令路径与只读命令路径悄悄分叉。

建议修复:委托给已有的 echoOrDeferLocalCommand,使闸门与 sink 只存在于一处(见上方 suggestion 块);这样 appendLocalUserEchoIfIdle(及其测试块)也不再必要;若保留该命名工具,至少去掉无人读取的反向布尔返回值、改为返回 void

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

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

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

Autofix review-response summary — PR #8496 (round 7)

Commit: acaa5556drefactor(web-shell): consolidate read-only command echo into echoOrDeferLocalCommand (#8496) (3 files changed, 11 insertions, 59 deletions).

No base-conflict resolution was needed (--conflict false; origin/main was not merged).

Feedback dispositions

  1. [rc:3718055853] Stale doc comment above showContextUsage — ACCEPTED. Verified at this commit: the echo is now conditional (echoLocalCommandIfIdle skips it mid-turn), MessageList Rule 4 only fires on a new user message (absent mid-turn), and the auto-scroll driver stays paused while the user is scrolled up (followPausedByUserRef) — so the panel reveal mid-turn depends entirely on dispatchReadOnlyStatus's resumeChatBottomFollow('smooth'), exactly as the finding states. Updated the comment above showContextUsage to describe the actual causal chain (idle echo → Rule 4; mid-turn → resumeChatBottomFollow).

  2. [rc:3719094897] echoLocalCommandIfIdle duplicates echoOrDeferLocalCommand's idle gate and echo sink — ACCEPTED. Verified the delegation is behaviourally identical for the echo side effect: both paths gate on streamingStateRef.current !== 'idle' and append through the same store.appendLocalUserMessage sink (images is an unused parameter on the helper). echoLocalCommandIfIdle now delegates via void echoOrDeferLocalCommand(text), the idle gate and sink live in one place, and appendLocalUserEchoIfIdle plus its test block are removed (its inverted boolean return had no production reader). Module doc comment in localCommandQueue.ts and the callback comment in App.tsx were updated to stop referencing the removed util. Net: −48 lines.

Review-body notes (not inline findings; no thread to resolve)

  • The round-5/6 "Test Plan (not a blocker)" observations concern the PR description text (a client/App.test.tsx path that only resolves relative to packages/web-shell, and test counts that predate later rounds). The file exists at packages/web-shell/client/App.test.tsx and is exercised in the verification below; the PR description is owned by the workflow and was not edited here.
  • The round-5 note about the verifier launch being written by hand describes workflow internals outside this checkout; no code change is possible here.

Verification

Commands actually run on this checkout after the change:

  • npm run build — passed (exit 0)
  • npm run typecheck — passed (exit 0)
  • npm run lint — passed (exit 0)
  • npx vitest run client/utils/localCommandQueue.test.ts client/App.test.tsx (from packages/web-shell, the touched package) — 2 test files, 317 tests passed (0 failed). This includes the /stats, /about, /context mid-turn echo-skip and idle-echo behavior tests that exercise the changed delegation path.
  • npx prettier --check on the three touched files — passed
  • Integration tests after npm run bundle — not run: the changed behavior lives in the web-shell client and is fully covered by the package's vitest suite above, not only through the bundled CLI or integration harness.
  • npm run generate:settings-schema — not needed: no settings source changed.
中文说明

Autofix 审查回应总结 — PR #8496(第 7 轮)

提交:acaa5556drefactor(web-shell): consolidate read-only command echo into echoOrDeferLocalCommand (#8496)(3 个文件变更,新增 11 行,删除 59 行)。

无需解决 base 冲突(--conflict false;未合并 origin/main)。

反馈处理

  1. [rc:3718055853] showContextUsage 上方的过时文档注释 — 已采纳。 已在该提交上核实:回显现在是有条件的(echoLocalCommandIfIdle 在回合中跳过回显),MessageList Rule 4 只在出现新用户消息时触发(回合中不存在),且用户向上滚动时自动滚动驱动保持暂停(followPausedByUserRef)——因此回合中的面板显示完全依赖 dispatchReadOnlyStatus 内的 resumeChatBottomFollow('smooth'),与该发现所述完全一致。已更新 showContextUsage 上方的注释,描述真实的因果链(空闲时回显 → Rule 4;回合中 → resumeChatBottomFollow)。

  2. [rc:3719094897] echoLocalCommandIfIdle 重复了 echoOrDeferLocalCommand 的空闲闸门与回显 sink — 已采纳。 已核实委托在回显副作用上行为完全一致:两条路径都以 streamingStateRef.current !== 'idle' 为闸门,并通过同一个 store.appendLocalUserMessage sink 追加(images 是该工具函数上未使用的参数)。echoLocalCommandIfIdle 现在通过 void echoOrDeferLocalCommand(text) 委托,空闲闸门与 sink 只存在于一处,appendLocalUserEchoIfIdle 及其测试块已删除(其反向布尔返回值在生产代码中没有读取者)。localCommandQueue.ts 的模块文档注释与 App.tsx 的 callback 注释已同步更新,不再引用被删除的工具函数。净效果:−48 行。

审查正文备注(非行内发现,无线程可 resolve)

  • 第 5/6 轮的 "Test Plan(非阻断)" 观察针对的是 PR 描述文本(client/App.test.tsx 路径仅在相对于 packages/web-shell 时可解析,以及早于后续轮次的测试数量)。该文件位于 packages/web-shell/client/App.test.tsx,并已在下方验证中执行;PR 描述由工作流管理,此处未做修改。
  • 第 5 轮关于 verifier 启动为手写的备注描述的是本 checkout 之外的工作流内部行为,此处无法做代码修改。

验证

变更后在本 checkout 上实际运行的命令:

  • npm run build — 通过(exit 0)
  • npm run typecheck — 通过(exit 0)
  • npm run lint — 通过(exit 0)
  • npx vitest run client/utils/localCommandQueue.test.ts client/App.test.tsx(在 packages/web-shell,即被修改的包内执行)— 2 个测试文件,317 个测试全部通过(0 失败)。其中包含 /stats/about/context 的回合中跳过回显与空闲回显行为测试,覆盖了本次变更的委托路径。
  • npx prettier --check(三个被修改文件)— 通过
  • npm run bundle 后的集成测试 — 未运行:变更行为位于 web-shell 客户端,已由上述包内 vitest 套件完整覆盖,并非只通过打包 CLI 或集成测试框架执行。
  • npm run generate:settings-schema — 无需:未修改任何 settings 源。

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

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


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

@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 — no blockers. Suggestions are inline. Test Plan (not a blocker): client/App.test.tsxno such file or directory; 7 tests pass — this review observed 1452, 2792 passed; 303 tests pass — this review observed 1452, 2792 passed.

中文说明

已审查——无阻断问题。 建议见行内评论。 Test Plan(非阻断):client/App.test.tsxno such file or directory; 7 tests pass — this review observed 1452, 2792 passed; 303 tests pass — this review observed 1452, 2792 passed

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

Comment on lines 7599 to +7600
if (cmd === 'status' || cmd === 'about') {
if (echoOrDeferLocalCommand(text, images)) return true;
echoLocalCommandIfIdle(text);

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] R7-1: /status has no test anywhere in web-shell — only its /about alias exercises this shared handler, so the cmd === 'status' half of the routing condition is unguarded.

— Failure scenario: a future edit that drops or mistypes 'status' in this condition compiles and passes the entire suite; /status then falls through to the "Forward slash commands as prompts" fallback and is sent to the model as literal prompt text instead of rendering local status info. Probe-verified at the reviewed commit: cloning the /about mid-turn test for /status fails when this condition is mutated to cmd === 'about', while all 8 pre-existing tests in the describe block stay green.

Suggested fix: add a /status test pair mirroring the existing /about tests (mid-turn run + idle echo), at minimum asserting loadPreflight is called and the dispatch carries { type: 'status', clearActiveText: false }.

中文说明

web-shell 中没有任何测试覆盖 /status——只有其别名 /about 会验证这个共享处理器,因此路由条件中 cmd === 'status' 这一半没有测试守卫。

— 失败场景:未来若有编辑删掉或误写该条件中的 'status',代码能编译并通过整个测试套件;/status 会落入「将斜杠命令作为提示词转发」的兜底分支,被当作字面提示词发送给模型,而不是渲染本地状态信息。已在被审提交上用探针验证:把 /about 的回合中测试克隆为 /status 版本后,将该条件变异为 cmd === 'about',探针失败,而 describe 块中既有的 8 个测试仍全部通过。

建议修复:仿照既有的 /about 测试为 /status 增加一对测试(回合中执行 + 空闲回显),至少断言 loadPreflight 被调用且 dispatch 携带 { type: 'status', clearActiveText: false }

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

Comment on lines +5014 to +5016
const echoLocalCommandIfIdle = useCallback(
(text: string): void => {
void echoOrDeferLocalCommand(text);

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] R7-2: this caller deliberately discards echoOrDeferLocalCommand's return value, but that function's contract comment four lines above still says "callers must then stop and not run the command's inline side effects" — the intentional exception is documented on this helper and in the localCommandQueue.ts module docstring, but not on either contract owner.

— Failure scenario: a maintainer fixing a turn-splitting bug reads the absolute contract above, concludes this discarded return value is the bug, and "restores" if (echoOrDeferLocalCommand(text)) return; — silently regressing mid-turn execution for /stats, /about and /context. (appendOrDeferLocalUserMessage's JSDoc @returns clause in localCommandQueue.ts repeats the same absolute contract and needs the same note.)

Suggested fix: extend the echoOrDeferLocalCommand docstring with one sentence, e.g. "Exception: read-only display commands wrap this in echoLocalCommandIfIdle and intentionally ignore the suppression signal — their status-block output does not split the active turn."

中文说明

此处调用方有意丢弃 echoOrDeferLocalCommand 的返回值,但上方四行处该函数的契约注释仍写着「调用方必须停止,不得执行命令的内联副作用」——这一有意例外已记录在本 helper 自身的注释和 localCommandQueue.ts 的模块 docstring 中,但没有记录在契约本体上。

— 失败场景:维护者在修复回合切割 bug 时读到上面的绝对契约,会认为这里丢弃返回值的写法就是 bug,从而「恢复」if (echoOrDeferLocalCommand(text)) return;——悄悄回归 /stats/about/context 的回合中执行能力。(localCommandQueue.ts 中 appendOrDeferLocalUserMessage 的 JSDoc @returns 子句重复了同样的绝对契约,也需要相同的补充说明。)

建议修复:在 echoOrDeferLocalCommand 的 docstring 中补一句,例如「例外:只读展示命令通过 echoLocalCommandIfIdle 包装本函数并有意忽略抑制信号——它们的 status 块输出不会切割活跃回合。」

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

@wenshao

wenshao commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Review — feat(web-shell): run read-only info commands immediately mid-turn

Verdict: no blockers. The load-bearing claims hold — I checked them against the code and re-ran the tests rather than trusting the description. One situational UX consequence is worth a look before merge, plus a few small cleanups.

What it does

/stats, /status//about and /context (slash command, status-bar indicator, in-chat "context detail") stop being silently swallowed while a turn streams. They now run immediately; only the user-message echo is skipped mid-turn, and the result is dispatched with a new clearActiveText: false opt-out so the status block does not finalize the in-flight assistant/thought block.

Claims I verified against the code

  • Not a turn boundaryisTurnStartMessage (MessageList.tsx:786) only matches user / user_shell. ✅
  • Not counted in turn metricsitemAssistantUsage, itemToolCallCount, terminalTurnTimestamp (:1085) and assistantContentTimestamp all skip a system row with no source. ✅
  • Stays visible when collapsedisHideableStep (:569) hides system rows only when isMidTurnInjectedDebugMessage, which this isn't. ✅
  • Doesn't hijack the final-answer detectionisExecutionWorkStep (:1032) is tool_group / plan only, so the trailing status block does not push findFinalAnswerIndex to -1 and force the turn permanently expanded. ✅
  • clearActiveText: false really does protect token countsapplyAssistantUsage (transcript.ts:530) drops the frame outright when activeAssistantBlockId is undefined, so finalizing mid-stream would lose that round's tokens, not just split the block. The justification in the comment is accurate.
  • RPC isn't serialized behind the turn_qwen/session/context_usage and the stats path go through requireOwned, not withMutableOwned (dispatch.ts:2836), so they can answer while a prompt is in flight. The feature is actually reachable mid-turn.
  • Local-only dispatchstore.dispatch (sdk ui/store.ts:54) just reduces client state; nothing is sent to the daemon or to peer clients.

Verification I ran locally

  • packages/sdk-typescriptvitest run test/daemon-ui-transcript.test.ts — 5/5 pass.
  • Fresh worktree at acaa555packages/web-shellvitest run client/App.test.tsx -t "read-only local commands mid-turn" — 8/8 pass.
  • Mutation check on the new reducer line: replacing else state.activeUserBlockId = undefined; with a no-op fails resets the active user block even when clearActiveText is false. The test is load-bearing, not decorative.
  • prettier --stdin-filepath packages/web-shell/client/App.tsx — clean.

Findings

1. (Medium, situational) The output does not stay at the tail while text is streaming.

clearActiveText: false keeps activeAssistantBlockId pointing at a block that sits above the new status block, so every subsequent delta merges into that earlier block. I ran the reducer on a realistic sequence (text → /stats → more text → tool call → more text):

user      "question"
assistant "part one. part two. "   ← "part two." was streamed AFTER /stats
status    "<<STATS>>"
tool      read_file
assistant "part three."

Two consequences worth weighing:

  • The stats panel lands before the following tool call and after all prose of that assistant segment, i.e. not where the user typed it.
  • dispatchReadOnlyStatus calls resumeChatBottomFollow('smooth'), which anchors the viewport to the last block — the status panel. The continuing prose grows above the fold, so until the next tool block or new assistant block appears, the transcript looks frozen on the stats output while the model is still writing.

Note this only bites when text is actively streaming; when the model is between steps (no active assistant block) the block lands at the tail and everything after it renders below, which is the common case. Not a correctness bug and arguably an acceptable trade for not splitting the turn — but it isn't described in the PR body and no test pins it. Worth either documenting the ordering explicitly or considering whether bottom-follow should be skipped when an assistant block is mid-stream.

2. (Nit) echoOrDeferLocalCommand's JSDoc now contradicts one of its callers.

acaa555 removed appendLocalUserEchoIfIdle — and with it the doc that explained why the echo is skipped — leaving void echoOrDeferLocalCommand(text) in echoLocalCommandIfIdle. The helper's own comment (App.tsx:4995) still reads "Returns true when suppressed — callers must then stop and not run the command's inline side effects", which is exactly what the new caller deliberately doesn't do. The void makes the contradiction easy to miss on a later edit. Please update that comment (and the @returns on appendOrDeferLocalUserMessage) to say the signal is advisory for read-only commands.

3. (Low) The reducer change also alters a pre-existing call site.

else state.activeUserBlockId = undefined; applies to every clearActiveText: false caller, including the trimmed-tool-output error in upsertToolBlock (transcript.ts:761). If a peer's user.text.delta were streaming when that fires, the user row now splits in two — and since user rows are turn boundaries, that fabricates an extra turn. Very unlikely in practice (needs multi-delta peer user text plus max-block trimming in the same instant), but it is a behavior change outside the stated scope and the comment only mentions the command-echo case.

4. (Low, layering) clearActiveText is a client-render flag on a daemon→client wire type.

DaemonUiStatusEvent is what the daemon emits. The doc says "Daemon-emitted events leave this unset", but nothing enforces it — a daemon that sets clearActiveText: false now suppresses assistant-block finalization on every connected client. A dispatch-level option (store.dispatch(events, { clearActiveText: false })) or stripping the field on SSE ingest would keep the wire contract honest. Low risk given the daemon is trusted; flagging the layering.

5. (Nit) Test plan is stale. The body cites client/utils/localCommandQueue.test.ts (7 tests) as covering "the new echo helper", but acaa555 deleted that helper and its 21 lines of tests — the file is no longer touched by this PR. Several bot reviews already flagged the count mismatch. Worth a body edit so the plan matches what actually landed.

6. (Pre-existing, FYI) getStats() / getContextUsage() resolve into store.dispatch with no connectionRef.current.sessionId re-check. That pattern is used at ~20 sites in App.tsx already, but a long turn widens the window in which the user can switch sessions before the promise settles.

Things done well

  • Replacing .catch(() => {}) on /stats and the bare .then() on /about with reportError fixes a genuinely swallowed failure (and an unhandled rejection when collectSystemInfo throws) — and both paths now have tests.
  • reportError only toasts/logs; it does not dispatch a transcript block, so an error on this path can't split the turn either. Nice that the invariant holds on the failure path too.
  • The reducer tests cover assistant, thought, and the user-pointer reset, and they fail under mutation.
中文说明

结论:无阻断问题。 PR 的关键论断我都对着代码核实过,并在本地重跑了测试,而不是照搬描述。有一个与场景相关的 UX 后果值得在合并前确认,另有几处小清理。

已核实:status 块不是回合边界(isTurnStartMessage 只认 user/user_shell);不计入回合统计;折叠后保持可见;不会劫持 final-answer 判定(isExecutionWorkStep 只匹配 tool_group/plan);clearActiveText: false 确实能保住 token 计数(applyAssistantUsage 在没有活跃 assistant 块时会直接丢弃 usage 帧);context_usagerequireOwned 而非 withMutableOwned,回合中确实能返回;store.dispatch 纯客户端本地状态。

本地验证:sdk daemon-ui-transcript.test.ts 5/5 通过;在 acaa555 的干净 worktree 里跑 web-shell 新增的 8 个用例全通过;对 else state.activeUserBlockId = undefined; 做变异后对应用例失败,说明测试是有效的;prettier 干净。

主要发现

1.(中,视场景)文字仍在流式输出时,status 块不会停在末尾——后续 delta 会并入位于其上方的 assistant 块。实测顺序为 user / assistant("part one. part two. ") / status / tool / assistant("part three.")。加上 resumeChatBottomFollow 把视口锚定到最后一块(即 stats 面板),正在生成的正文会长在视口上方,看起来像"卡住"了,直到下一个 tool 块出现。仅在有活跃 assistant 块时才会发生;不是正确性缺陷,但 PR 描述未提及、也无测试固定该顺序。
2.(提示)acaa555 删掉 appendLocalUserEchoIfIdle 后,echoOrDeferLocalCommand 的注释仍写着"返回 true 表示被抑制,调用方必须停止",而新调用方恰恰相反,void 掩盖了这层矛盾,建议更新注释。
3.(低)新增的 else 分支同样影响既有调用点 upsertToolBlock 的 trimmed-tool 错误路径;极端情况下会把 peer 的 user 行切成两段,从而伪造一个额外回合。
4.(低,分层)clearActiveText 是纯渲染标志,却挂在 daemon→client 的 wire 类型上,没有任何强制约束;建议做成 dispatch 层选项或在 SSE 入口剥离。
5.(提示)描述里的 Test Plan 已过期:localCommandQueue.test.ts 已不在本 PR 改动范围内。
6.(既有问题)异步 dispatch 未复查 sessionId,长回合放大了切会话的窗口。

做得好的地方:把 /stats.catch(() => {})/about.then() 换成 reportError,修掉了真实被吞掉的失败和一处未处理拒绝,并且都有测试;reportError 只弹 toast 不写 transcript,失败路径同样不会切分回合。

@wenshao

wenshao commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 96 passed · 0 failed · 96 total

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:96 通过 · 0 失败 · 96 总计

Verification report

PR #8496 — feat(web-shell): run read-only info commands immediately mid-turn

Verdict: merge-ready — 96/96 scripted assertions passed, 0 unexpected failures.
Verified head: acaa5556dd6fd293cf1d4bf99ae1ddeb20943556 (merge-ref checkout 877b5c305, base tip 93cd01908).

中文摘要
  • 结论merge-ready。96 条脚本化断言全部通过,0 个意外失败。
  • A/B 结论:中心主张成立且 load-bearing。把 head 的测试文件嫁接到 base(93cd01908)上运行:App 层 mid-turn 块在 base 为 5 失败 / 3 通过(/stats/about/context 回合中被静默吞掉,/about 失败甚至产生 unhandled rejection),在 head 为 8/8 通过;SDK reducer 层在 base 为 2 失败 / 3 通过(流式 assistant/thought 块被 status 卡切断、usage 帧丢失),在 head 为 5/5。对已发布 dist 的 seam 测试(真实 store + App 实际 dispatch 的 payload)head 10/10,回退两个关键 hunk 的 control dist 在场景 A 上 3 项按预期变红(turn 被切成两段、usage 孤儿化)。
  • Findings:无阻塞项。两条信息性观察:(1) 共享的 appendStatusBlock else 分支同时改变了既有的 tool-trimmed 路径(现在也会重置 user 指针)——方向与修复意图一致(防止后续 peer echo 合并进旧块),SDK 全套 1455 测试无回归;(2) clearActiveText 字段声明在同时覆盖 status | debug 的接口上,client dispatch 的 debug 事件理论上也能 opt-out,文档注释只描述了 status。另有一处描述更正:PR 描述中的测试计数过期(localCommandQueue.test.ts 实际 5 个测试而非 7;App.test.tsx 实际 314 而非 303)。
  • 未覆盖范围:真实 daemon + 浏览器的端到端会话(沙箱无凭据,组件级/真实 store 级复现的是行为形状而非活体 turn);逐 commit 归因(shallow checkout,快照 11 个 commit 中本地仅 head 可达);状态栏点击与聊天内 context-detail 点击路径仅通过共享的 showContextUsage 回调被覆盖。

Central claim and A/B

Central claim: /stats, /about (/status) and /context execute immediately while a turn is streaming (previously silently swallowed), with only the local echo skipped mid-turn; the dispatched status uses clearActiveText: false so the streaming assistant/thought block is not finalized, while the user pointer is still reset so a peer prompt echo cannot merge into the command echo.

Method: head's own new test files were grafted verbatim onto a scratch worktree at the base tip (tmp/base-tree at 93cd01908, nested node_modules symlinked; all @qwen-code/* imports in the App suite are vi.mocked and the SDK suite imports by relative path, so neither arm can load the other tree's changed code — realpath of the workspace links was asserted and quoted in Methodology). The same tests therefore run against base and head production code.

Witness: evidence/01-ab-app-midturn-base-fails-head-passes.png

cell oracle result
App, BASE + head tests 5 mid-turn assertions (getStats/loadPreflight/getContextUsage called, dispatch payload with clearActiveText:false, no echo) + 2 error-reporting assertions 5 failed, 3 passed (idle echoes pass; base also emits an unhandled rejection for /about — the missing .catch the PR adds)
App, HEAD same 8 passed
SDK reducer, BASE + head tests opt-out keeps assistant/thought block active 2 failed, 3 passed (blocks split to [user, assistant, status, assistant] / [user, thought, status, thought])
SDK reducer, HEAD same 5 passed

Witness: evidence/02-ab-sdk-reducer-base-fails-head-passes.png

Integration seam (the one surface neither suite covers: App mocks the store, SDK tests hand-build events). seam-harness.mjs drives the real createDaemonTranscriptStore from the shipped dist with the exact payload dispatchReadOnlyStatus sends ([{ type: 'status', text, clearActiveText: false }]), injected mid-stream, plus the round-3 corruption scenario (idle echo → opt-out dispatch → peer user.text.delta).

Witness: evidence/03-seam-harness-head-dist-vs-revert-control.png

cell result
head dist (dist/daemon/index.js) 10/10 — no split, text merged across the status card, usage {3,5,0} on the original assistant block, peer prompt opens its own user block
revert-control dist (same bundle, both hunks reverted to base semantics) Scenario A 3 reds exactly as the PR's commit message predicts: blocks [user, assistant, status, assistant], text truncated at the status card, usage undefined (orphaned); Scenarios B/C green

Mutation matrix (on HEAD source, each reverted afterwards; raw log logs/mutation-matrix.log).

Witness: evidence/04-mutation-matrix-kills.png

mutant suite result classification
M1 omit clearActiveText: false in dispatchReadOnlyStatus App mid-turn block 3 killed (the three dispatch-payload assertions), 5 pass pinned
M2 delete else state.activeUserBlockId = undefined; SDK suite 1 killed ("resets the active user block…"), 4 pass pinned (round-3 fix)
M2 (same) App mid-turn block 8/8 green expected blind spot: App suite mocks the store; the seam harness covers this axis (Scenario B fails on the control dist)
M3 restore base /stats early return App mid-turn block 1 killed ("runs /stats immediately…"), 7 pass pinned
unmutated control both suites 8 pass / 5 pass green

No survivors. The coarse whole-PR mutant is the base arm itself (5+2 reds).

Reviewer Test Plan walkthrough

  1. Open Web Shell against a daemon, send a long prompt — not executable in this credential-free container; reproduced the shape at the component level (streamingState='responding' + onSubmit('/stats')) and at the real-store level (seam harness). This reproduces the handling, not a live daemon turn.
  2. Type /stats (or /about, /context, status-bar click) mid-turn — exercised; all three keyboard paths flip base→head (5 reds → green). Status-bar and in-chat context-detail clicks share the tested showContextUsage callback by construction.
  3. Output appears inline; counters/turn unaffected; no echo mid-turn — reducer asserts no split and usage on the original block; no echo asserted by App tests. The collapse-side claims are pinned by pre-existing MessageList tests the PR does not touch: "keeps system rows (errors/output) visible while hiding tool steps" (MessageList.test.ts:1392), "does not collapse a turn whose only response is a system row" (:1422), "ignores non-step system timestamps when recording elapsed" (:1534); isTurnStartMessage (MessageList.tsx:790) admits only user/user_shell, so a status block cannot be a turn boundary.
  4. Idle behavior unchanged — the three idle-echo tests pass on both arms.
  5. Unit suites — ran; counts differ from the description (see Corrections).

Corrections

  • The PR description says localCommandQueue.test.ts has 7 tests covering "the new echo helper". The file (untouched by this PR) has 5 tests covering the pre-existing choke point appendOrDeferLocalUserMessage/isCommandPrompt; the new echoLocalCommandIfIdle lives in App.tsx and is covered by App.test.tsx. App.test.tsx is 314 tests at the verified head, not 303 (description predates the last two commits).
  • The description's "Before" behavior ("no toast") is accurate; base additionally produced an unhandled promise rejection on /about load failure (captured in 01-…png), which the description does not mention and the PR fixes.

Findings

  1. (informational) The shared appendStatusBlock change also alters the pre-existing trimmed-tool path (transcript.ts:761 passed { clearActiveText: false } before this PR): it now resets activeUserBlockId too. Analysis: on tool-first turns the prompt echo keeps the user pointer live through the tool phase, so the old behavior exposed that path to the same peer-echo merge corruption the round-3 commit fixed; the new behavior is strictly safer. Full SDK suite (1455 tests, incl. the T-series scoped-clearActiveText tests) green. No action required; noted for the maintainer's awareness.
  2. (nit) clearActiveText?: boolean is declared on DaemonUiStatusEvent, whose type is 'status' | 'debug' — a client-dispatched debug event can also opt out, while the doc comment only describes status. Harmless (no dispatcher sends it), surface-level.

Neither is blocking; both are documented behavior-consistent with the PR's intent.

Not covered

  • Live daemon + browser E2E (no credentials in the sandbox; no Playwright e2e exists for this surface). The component/reducer runs reproduce the shape of the reported behavior, not the live-turn trigger.
  • Per-commit attribution: shallow checkout — snapshot lists 11 commits, only acaa5556d is locally reachable (git rev-list HEAD^1..HEAD^2 = 1). Verified the aggregate HEAD^1..HEAD diff only.
  • Snapshot baseRefOid (da37110e) had drifted from the merge-ref base tip (93cd01908); per the merge-ref contract, HEAD^1 is authoritative and is what the A/B used.
  • Status-bar / in-chat click entry points exercised only via the shared callback; no separate click-path test.
  • Repo-wide gates beyond the two affected workspaces (PR's own CI covers those).

Methodology

Environment: node:22-bookworm CI container, merge-ref checkout at depth 2; npm ci + build pre-run at head. Base arm: scratch worktree at HEAD^1 with per-package node_modules symlinked from the head install (lockfile untouched by the PR); contamination-neutralized because the App suite vi.mocks every @qwen-code/* module (realpath of node_modules/@qwen-code/{sdk,webui} asserted to point at the head tree, quoted here) and the SDK suite imports sources by relative path. Harnesses: grafted-suite A/B (ab-app.sh, ab-sdk.sh), dist-level seam harness (seam-harness.mjs against dist/daemon/index.js and a two-hunk revert control built from the shipped bundle), mutation matrix (mutation-matrix.sh, trap-restored). Gates: full web-shell suite 2815/2815, full sdk-typescript suite 1455/1455, per-package tsc --noEmit, eslint on changed files (liveness proven by a planted two-error probe), prettier check on all six changed files. Raw logs in logs/, captures in evidence/.

Evidence images

01-ab-app-midturn-base-fails-head-passes

02-ab-sdk-reducer-base-fails-head-passes

03-seam-harness-head-dist-vs-revert-control

04-mutation-matrix-kills

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

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

Autofix review response — PR #8496 (round 7)

Commit: f7d04f6c3test(web-shell): cover /status routing and document the echo-suppression exception (#8496)

Feedback dispositions

  • [rc:3720152068] R7-1 — /status routing has no test (Suggestion): addressed.
    Verified the finding at the reviewed commit: the routing condition is cmd === 'status' || cmd === 'about', only /about was tested, and unmatched slash commands fall through to the "Forward slash commands as prompts" fallback, so dropping 'status' would silently send /status to the model as prompt text. Added a /status test pair mirroring the existing /about tests in the App read-only local commands mid-turn describe block:

    • runs /status immediately while streaming and skips the echo — asserts loadPreflight is called, no local echo is appended mid-turn, and the dispatch carries { type: 'status', clearActiveText: false, text: serializeStatusMessage(...) }.
    • echoes /status when idle — asserts the /status echo is appended.

    A mutation probe confirmed the tests are load-bearing: narrowing the condition to cmd === 'about' fails both new tests while all pre-existing tests stay green.

  • [rc:3720152074] R7-2 — echo-suppression contract omits the deliberate exception (Suggestion): addressed.
    Added one sentence to each contract owner: the echoOrDeferLocalCommand contract comment in App.tsx now states that read-only display commands wrap it in echoLocalCommandIfIdle and intentionally ignore the suppression signal, and the appendOrDeferLocalUserMessage JSDoc @returns clause in localCommandQueue.ts carries the same note pointing at the module docstring. Comments only; no behavior change.

No conflicts (--conflict false); origin/main was not merged.

Verification

  • npm run build — passed (exit 0). Note: the first focused vitest run failed to resolve @qwen-code/sdk/daemon because this checkout had no built dist/; the build restored it and every command below ran against the final sources.
  • npm run typecheck — passed (exit 0)
  • npm run lint — passed (exit 0)
  • npx vitest run client/App.test.tsx in packages/web-shell (touched) — 314 passed (312 pre-existing + 2 new /status tests)
  • npx vitest run --config vitest.config.ts in packages/web-shell (full touched package) — 166 files / 2794 passed
  • npx prettier --check on the three touched files — clean
  • Mutation probe on the routing condition (temporary, reverted before commit) — both new tests fail as expected
  • Integration tests: not applicable — the change is web-shell client tests and comments only, exercised by the package Vitest suite, not the bundled CLI or integration harness
中文说明

Autofix 审查回应 — PR #8496(第 7 轮)

提交:f7d04f6c3test(web-shell): cover /status routing and document the echo-suppression exception (#8496)

反馈处理结果

  • [rc:3720152068] R7-1 — /status 路由没有任何测试(建议):已处理。
    已在被审提交上核实该发现:路由条件为 cmd === 'status' || cmd === 'about',此前只有 /about 被测试覆盖,而未匹配的斜杠命令会落入「将斜杠命令作为提示词转发」的兜底分支,因此一旦删掉 'status'/status 会被悄悄当作提示词文本发给模型。在 App read-only local commands mid-turn describe 块中仿照既有的 /about 测试新增了一对 /status 测试:

    • runs /status immediately while streaming and skips the echo — 断言 loadPreflight 被调用、回合中没有追加本地回显,且 dispatch 携带 { type: 'status', clearActiveText: false, text: serializeStatusMessage(...) }
    • echoes /status when idle — 断言空闲时追加 /status 回显。

    变异探针确认这两个测试确实起作用:将条件收窄为 cmd === 'about' 后,两个新测试均失败,而所有既有测试保持全绿。

  • [rc:3720152074] R7-2 — 回显抑制契约遗漏了有意例外(建议):已处理。
    在两个契约本体上各补了一句:App.tsxechoOrDeferLocalCommand 的契约注释现在说明只读展示命令通过 echoLocalCommandIfIdle 包装本函数并有意忽略抑制信号;localCommandQueue.tsappendOrDeferLocalUserMessage 的 JSDoc @returns 子句也补充了相同说明并指向模块 docstring。仅注释变更,无行为变化。

无冲突(--conflict false),未合并 origin/main

验证

  • npm run build — 通过(exit 0)。说明:首次聚焦 vitest 运行因本检出缺少构建产物而无法解析 @qwen-code/sdk/daemon;build 恢复了产物,以下所有命令均针对最终源码运行。
  • npm run typecheck — 通过(exit 0)
  • npm run lint — 通过(exit 0)
  • packages/web-shellnpx vitest run client/App.test.tsx( touched 包)— 314 通过(312 个既有 + 2 个新 /status 测试)
  • packages/web-shellnpx vitest run --config vitest.config.ts(完整 touched 包)— 166 个文件 / 2794 通过
  • 对三个改动文件运行 npx prettier --check — 干净
  • 对路由条件做变异探针(临时修改,提交前已还原)— 两个新测试按预期失败
  • 集成测试:不适用 — 本次变更仅为 web-shell 客户端测试与注释,由包的 Vitest 套件覆盖,不涉及打包后的 CLI 或集成测试框架

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

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


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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Triage re-run completed without a new review.

⚠️ The bot has neither a verdict nor a deferral on f7d04f6c3d047f810f87985c321ba1214d122e9a — no APPROVED, CHANGES_REQUESTED, or COMMENTED review of its own. A DISMISSED one does not count: dismiss_stale_reviews voids the bot's approval on every push, which is exactly when a fresh one is needed. If this re-run was meant to review or approve, it did not, and an approval left by another account is a separate vote that does not count as the bot's own.

⚠️ 机器人在 f7d04f6c3d047f810f87985c321ba1214d122e9a既没有裁决也没有 defer —— 没有属于它自己的 APPROVEDCHANGES_REQUESTEDCOMMENTED 评审。DISMISSED 不算:dismiss_stale_reviews 会在每次推送时作废机器人的批准,而那恰恰是需要一次新批准的时刻。如果这次重跑本应评审或批准,那么它没有做到;而其他账号留下的批准是另一张票,不能算作机器人自己的。

The stage comments above were updated with the latest result. View workflow run.

上方各阶段评论已更新为最新结果。查看工作流运行

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

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

中文说明

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

@yiliang114 yiliang114 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.

LGTM, no blockers. Read-only info commands (/stats /about /status /context) now run immediately mid-turn with only the echo skipped — correct since they only query state and never mutate the session or send prompts. The clearActiveText:false status block inserts without finalizing the streaming assistant/thought block (no split answer / orphaned usage frames), while still resetting activeUserBlockId so a peer prompt echo opens its own block. Daemon-emitted status events keep default finalize behavior. Bonus: the swallowed .catch(()=>{}) on /stats and /status is now reportError. Tests cover mid-turn immediate run, idle echo, and failure reporting for each command. Clean fix for the dev:daemon mid-turn command blocking.

@wenshao

wenshao commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 35 passed · 0 failed · 35 total

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:35 通过 · 0 失败 · 35 总计

Verification report

<!-- qwen-triage:verify -->
<!-- qwen-triage:verify-substantive -->

Sandboxed verification: ✅ passed — merge-ready (agent verdict) (follow-up round)

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 35 passed · 0 failed · 35 total

中文 — 判定:✅ 通过 · 可合入(agent 判定,跟进轮)

沙箱验证在隔离、无凭证容器中复测了该 PR 在新 head(15f3e0025,= 上轮验证 head acaa5556d + 测试提交 f7d04f6c + 合入最新 main)与新 base(c73b5ed88)上的行为。脚本化断言全部通过,0 个意外失败(计数见报告头行)。

  • A/B 结论:中心主张在新 base 上依旧 load-bearing——head 测试嫁接到 base 后按预期变红、head 全绿,数字见 "Central claim and A/B" 表;对已发布 dist 的 seam harness 重建复跑,回退 control 按预期变红,见 seam 表与 02-…png
  • 上轮 findings 状态:两条信息性观察均 stands(复测确认未变),描述测试计数过期的更正同样 stands——见 "Previous-finding status table"。
  • 本轮增量:新增 /status 路由测试非空转(删 'status' 路由的突变恰好杀死这 2 个测试,见 mutation matrix 表);main 合入未改变变更面行为(MessageList 折叠侧 pin 行号未动,门禁全绿,见 gate 表)。
  • 未覆盖范围:真实 daemon + 浏览器端到端(沙箱无凭据);逐 commit 归因(shallow checkout,快照 13 个 commit 中本地仅 merge head 可达)——见 "Not covered"。
Verification report

PR #8496 — feat(web-shell): run read-only info commands immediately mid-turn (follow-up round)

Verdict: merge-ready — 35/35 scripted assertions passed, 0 unexpected failures.
Verified head: 15f3e00259f508e09169133bcce45248a2406986 (merge-ref checkout 3b6aae854, base tip c73b5ed88).
Previous round verified head acaa5556d at base 93cd01908 (run 31006376910, verdict merge-ready, 96/96).

Previous-finding status table

# finding (previous round) severity status at 15f3e0025 re-measurement
1 shared appendStatusBlock else-branch also alters the pre-existing trimmed-tool path (resets activeUserBlockId there too) informational stands else-branch present at transcript.ts:1266; trimmed-tool call site unchanged (head :761 == base :757); full SDK suite 1455/1455 green at new head
2 clearActiveText?: boolean declared on DaemonUiStatusEvent whose type is 'status' | 'debug'; doc comment only describes status nit stands types.ts:287 unchanged; still only App.tsx:5032 dispatches it, always type: 'status'; reducer routes status+debug through the shared case (transcript.ts:342-345)
C1 description's test counts stale (claimed 7 / 303; actual 5 / 314) correction stands description still claims 7 and 303; re-measured actuals: localCommandQueue.test.ts = 5 tests (file untouched), App.test.tsx = 316 tests per vitest (314 + the two new /status tests). Agree with the previous round; the description was not updated

Neither standing item is blocking; both were explicitly non-blocking last round and the delta commits (f7d04f6c test+docs, 15f3e0025 merge of main) did not move them.

Delta since the previous round

  • f7d04f6c — adds the two /status routing tests (mid-turn + idle) to the App read-only local commands mid-turn block (8 → 10 tests) and documents the echo-suppression exception in App.tsx/localCommandQueue.ts comments. Behavioral delta: none (comments + tests only).
  • 15f3e0025 — merge of current main; base moved 93cd01908c73b5ed88. This round's snapshot baseRefOid equals the merge-ref base tip (no drift, unlike last round).

New probes were scoped to this delta: the /status vacuity check (M3 below) and a full re-measurement of every carried measurement at the new head/base (no input-closure shortcut taken — the base changed, so everything was re-run).

Central claim and A/B

Central claim (unchanged): /stats, /about (/status) and /context execute immediately while a turn is streaming (previously silently swallowed), with only the local echo skipped mid-turn; the dispatched status uses clearActiveText: false so the streaming assistant/thought block is not finalized, while the user pointer is still reset so a peer prompt echo cannot merge into the command echo.

Method (same as previous round, re-run at new head/base): head's test files grafted verbatim onto a scratch worktree at the base tip (tmp/base-tree at c73b5ed88, per-package node_modules symlinked from the head install — lockfile untouched by the PR). Contamination-neutralized and asserted: readlink -f of node_modules/@qwen-code/{qwen-code-core,sdk} from the base tree points into the head tree, and neither arm can load that code — the App suite vi.mocks both @qwen-code/* specifiers App.tsx imports (App.test.tsx:363, :409; the import type at :13 erases), the SDK suite imports sources by relative path.

Witness: evidence/01-ab-cells-base-fails-head-passes.png

cell oracle result
App, BASE + head tests 10 tests: 4 mid-turn (getStats/loadPreflight/getContextUsage called, dispatch payload with clearActiveText:false, no echo) + 4 idle echoes + 2 error-reporting 6 failed, 4 passed (idle echoes pass; base also emits an unhandled rejection for /about — the missing .catch the PR adds)
App, HEAD same 10 passed (file total 316 tests, 306 filtered out)
SDK reducer, BASE + head tests 5 tests: opt-out keeps assistant/thought block active, usage on original block, user pointer reset 2 failed, 3 passed (assistant/thought blocks split to [user, assistant, status, assistant] / [user, thought, status, thought])
SDK reducer, HEAD same 5 passed

Integration seam (reconstructed and re-run at the new head): seam-harness.mjs drives the real createDaemonTranscriptStore from the shipped dist (dist/daemon/index.js) with the exact payload dispatchReadOnlyStatus sends ([{ type: 'status', text, clearActiveText: false }]), injected mid-stream, plus the peer-echo scenario and a default-behavior guard. Two control bundles built from the shipped bundle by reverting the exact compiled hunks (each anchor verified unique, 1 occurrence): control-base (status/debug fall through to error handling; opt-out does nothing) and control-m2 (routing kept, else activeUserBlockId = undefined removed).

Witness: evidence/02-seam-head-dist-vs-controls.png

cell result
head dist (shipped) 6/6 green — no split, text merged across the status card, usage {3,5,0} on the original assistant block, peer prompt opens its own user block, default status still finalizes
control-base dist Scenario A 3 reds exactly as the PR's commit message predicts: blocks [user, assistant, status, assistant], text truncated to answering, usage undefined (orphaned); Scenarios B/C green (base's full clear also reset the user pointer)
control-m2 dist Scenario B 2 reds: blocks [user, status], texts ['/statsfix the bug', 'stats output'] — peer echo merged into the command echo; A/C green

Mutation matrix

On HEAD source, each mutant applied, scoped suite run, restored via git checkout (restoration byte-exact, tree clean afterwards). Witness: evidence/03-mutation-matrix-kills.png. Raw per-mutant logs in logs/mutant-*.log.

mutant suite result classification
unmutated control (app) App mid-turn block 10 passed positive control: harness can go green
unmutated control (sdk) SDK file 5 passed positive control
M1 omit clearActiveText: false in dispatchReadOnlyStatus App mid-turn block 4 killed (the four mid-turn dispatch-payload assertions), 6 pass pinned
M3 drop 'status' from the about branch (delta-test vacuity check) App mid-turn block 2 killed (the two new /status tests, mid-turn + idle; failure mode expected "spy" to be called at least once on loadPreflight — the command never reaches its handler), 8 pass pinned — the new tests are not vacuous
M2 delete else state.activeUserBlockId = undefined; SDK file 1 killed (expected 'user-1' to be undefined), 4 pass pinned; corroborated by an independent instrument (seam control-m2 dist red on Scenario B)

No survivors. The coarse whole-PR mutant is the base arm itself (6+2 reds).

Reviewer Test Plan walkthrough (re-measured)

  1. Open Web Shell against a daemon, send a long prompt — not executable in this credential-free container; shape reproduced at component level (streamingState='responding' + onSubmit(...)) and at the real-store level (seam harness). Reproduces the handling, not a live daemon turn.
  2. Type /stats (or /about, /context, status-bar click) mid-turn — exercised; all four keyboard paths flip base→head (6 reds → green). New this round: /status has its own two tests and they pin the routing (M3). Status-bar and in-chat context-detail clicks share the tested showContextUsage callback by construction (App.tsx:5911).
  3. Output appears inline; counters/turn unaffected; no echo mid-turn — reducer asserts no split and usage on the original block; no echo asserted by App tests. Collapse-side claims remain pinned by pre-existing MessageList tests the PR does not touch, verified at identical line numbers at the new head: "keeps system rows (errors/output) visible while hiding tool steps" (MessageList.test.ts:1392), "does not collapse a turn whose only response is a system row" (:1422), "ignores non-step system timestamps when recording elapsed" (:1534); isTurnStartMessage (MessageList.tsx:790) admits only user/user_shell, so a status block cannot be a turn boundary.
  4. Idle behavior unchanged — the four idle-echo tests pass on both arms.
  5. Unit suites — ran; counts differ from the description (see Corrections).
  6. Scope boundary — verified: echoLocalCommandIfIdle is used only by /stats (App.tsx:7610), /status+/about (:7624) and /context via showContextUsage (:5911); /model-related (:7184), /tools (:7323), /bug (:7692) still early-return through echoOrDeferLocalCommand — matching the description's out-of-scope list.

Corrections

  • The description still claims localCommandQueue.test.ts has 7 tests and App.test.tsx 303. Re-measured at the new head: 5 (file untouched by the PR; it covers the pre-existing choke point, not the new helper) and 316 per vitest. The description was not updated since the previous round; the correction stands.

Findings

No new findings. The two informational items from the previous round stand unchanged (see status table); neither blocks.

Not covered

  • Live daemon + browser E2E (no credentials in the sandbox; no Playwright e2e exists for this surface). The component/reducer/seam runs reproduce the shape of the behavior, not a live-turn trigger.
  • Per-commit attribution: shallow checkout — snapshot lists 13 commits, only the merge head 15f3e0025 is locally reachable (git rev-list HEAD^1..HEAD^2 = 1). Verified the aggregate HEAD^1..HEAD diff only. The delta commits' content was nonetheless identified by diffing the effective diff against the previous round's verified state (two new tests + doc comments + main merge).
  • Status-bar / in-chat click entry points exercised only via the shared showContextUsage callback; no separate click-path test.
  • Repo-wide gates beyond the two affected workspaces (PR's own CI covers those).

Methodology

Environment: node:22-bookworm CI container, merge-ref checkout at depth 2; npm ci + build pre-run at head; snapshot baseRefOid matched the merge-ref base tip this round. Base arm: scratch worktree at HEAD^1 (c73b5ed88) with per-package node_modules symlinked (lockfile untouched); workspace-link realpath asserted and quoted above. Harnesses (all in this artifact dir, rerunnable): grafted-suite A/B (ab-assert.mjs over logs/ab-cells.log), dist-level seam harness (seam-harness.mjs against the shipped bundle plus two revert controls in controls/), mutation matrix (mutation-matrix.mjs, trap-free: restores via git checkout and verifies byte-exactness), gate accounting (gate-assert.mjs). Gates: web-shell full suite 2817/2817 (170 files), sdk-typescript full suite 1455/1455 (32 files), per-package tsc --noEmit, prettier --check on all six changed files, eslint on the changed files with liveness proven by a planted two-error probe (no-unused-vars + no-explicit-any caught, exit 1; restored file clean, exit 0). Assertion accounting: A/B cells 4 + seam 18 + matrix 5 + gates 8 = 35, all scripted and executed; expected base-arm reds count as passes. Raw logs in logs/, captures in evidence/.

Evidence images

01-ab-cells-base-fails-head-passes

02-seam-head-dist-vs-controls

03-mutation-matrix-kills

04-gate-accounting

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

Evidence images

01-ab-cells-base-fails-head-passes

02-seam-head-dist-vs-controls

03-mutation-matrix-kills

04-gate-accounting

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

@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.

LGTM, looks ready to ship. ✅

@wenshao
wenshao added this pull request to the merge queue Aug 5, 2026
Merged via the queue into QwenLM:main with commit 6b4a629 Aug 5, 2026
49 of 50 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.6.

@QwenLM QwenLM deleted a comment Aug 5, 2026
@QwenLM QwenLM deleted a comment Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants