Skip to content

fix(web-shell): repair dangling mock reference that broke main CI (#11415) - #11416

Closed
qwen-code-dev-bot wants to merge 1 commit into
mainfrom
autofix/issue-11415
Closed

fix(web-shell): repair dangling mock reference that broke main CI (#11415)#11416
qwen-code-dev-bot wants to merge 1 commit into
mainfrom
autofix/issue-11415

Conversation

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

What this PR does

Repairs the two does not rerender App for other split sessions tests in the web-shell suite, which failed on every main-branch CI run with ReferenceError: mockUseDaemonActivePromptBridge is not defined. The three dangling references now point at mockUseDaemonSessionActivityBridge — the mock the file actually defines for the active-prompt hook App renders through. No production code changes.

Why it's needed

Main CI has been red for four consecutive runs (70cf363, 1f89008, 422929b, d670d47), each failing in Run tests and generate reports. The root cause is a squash-merge collision: #11250's branch wired its own mockUseDaemonActivePromptBridge into the session-catalog-hooks module mock, but the merged result kept main's renamed wiring (mockUseDaemonSessionActivityBridge, from #11267/#10941) everywhere except the PR's newest test, which still referenced the old name. A reference error fails deterministically, so the suite's retries could not absorb it.

Reviewer Test Plan

How to verify

Confirm the suite is green where it was red: run cd packages/web-shell && npx vitest run --config vitest.config.ts and expect 0 failures (previously the two split-session rerender tests failed with a ReferenceError at App.test.tsx around line 28940). Expected: the entire web-shell suite passes; observed locally: 6712 passed, 0 failed. Also confirm the assertion still has teeth: the test fails if App re-renders when split panes report pending state for other sessions (verified via a temporary mutation of the pane-pending handler during development, then reverted).

Evidence (Before & After)

N/A (test-only change, no user-visible behavior)

Tested on

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

Environment (optional)

N/A — unit tests only (Vitest).

Risk & Scope

  • Main risk or tradeoff: essentially none — the change renames a mock reference inside one test; the asserted behavior is unchanged.
  • Not validated / out of scope: macOS and Windows test lanes were not run locally; the change is platform-independent test code.
  • Breaking changes / migration notes: none.

Linked Issues

Fixes #11415

中文说明

本 PR 做了什么

修复 web-shell 套件中两个 does not rerender App for other split sessions 测试,它们在每次 main 分支 CI 运行中都因 ReferenceError: mockUseDaemonActivePromptBridge is not defined 而失败。三处悬空的引用现在指向 mockUseDaemonSessionActivityBridge——该文件为 App 渲染所经的 active-prompt hook 实际定义的 mock。未改动生产代码。

为什么需要

main CI 已连续四次运行失败(70cf363395、1f890086f1、422929b3a7、d670d47e),每次都在 Run tests and generate reports 步骤失败。根因是压缩合并冲突:#11250 的分支把自己的 mockUseDaemonActivePromptBridge 接入了 session-catalog-hooks 模块 mock,但合并结果在各处保留了 main 重命名后的接线(mockUseDaemonSessionActivityBridge,来自 #11267/#10941),唯独 PR 最新添加的测试仍引用旧名称。引用错误是确定性失败,套件的重试无法吸收它。

评审者验证计划

如何验证

在原先失败的地方确认套件变绿:运行 cd packages/web-shell && npx vitest run --config vitest.config.ts,预期 0 失败(此前两个分屏会话重渲染测试在 App.test.tsx 约 28940 行处以 ReferenceError 失败)。预期:整个 web-shell 套件通过;本地实测:6712 通过、0 失败。另可确认断言仍然有效:当分屏面板报告其他会话的待处理状态时若 App 发生重渲染,该测试会失败(开发过程中曾通过对 pane-pending 处理器的临时变异验证,随后已还原)。

前后对比证据

N/A(仅测试改动,无用户可见行为)

测试平台

系统 状态
🍏 macOS ⚠️
🪟 Windows ⚠️
🐧 Linux

环境(可选)

N/A——仅单元测试(Vitest)。

风险与范围

  • 主要风险或权衡:基本没有——改动只是在一个测试内重命名 mock 引用;被断言的行为不变。
  • 未验证 / 超出范围:macOS 和 Windows 测试通道未在本地运行;该改动是与平台无关的测试代码。
  • 破坏性变更 / 迁移说明:无。

关联 Issue

Fixes #11415

…1415)

The squash merge of #11250 renamed its mockUseDaemonActivePromptBridge
wiring to main's mockUseDaemonSessionActivityBridge everywhere except the
three references inside the new 'does not rerender App for other split
sessions' test, which then failed with ReferenceError and turned four
consecutive main CI runs red in 'Run tests and generate reports'.

Point the test at the mock main actually defines — the hook App calls is
useDaemonSessionActivityBridge, so the rerender-count assertion is
unchanged in meaning.
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator Author

Autofix E2E Report — Issue #11415 (Main CI failed on d670d47)

Summary

Fixed the main-branch CI failure in Test (ubuntu-latest, Node 22.x) / Run tests and generate reports. The failure was a deterministic ReferenceError: mockUseDaemonActivePromptBridge is not defined in packages/web-shell/client/App.test.tsx, failing the two does not rerender App for other split sessions cases on every run (Vitest retries cannot absorb a reference error).

Root cause

The squash merge of #11250 collided with main's active-prompt rework (#11267 / #10941). The PR branch wired its own mockUseDaemonActivePromptBridge into the ./session-catalog/session-catalog-hooks module mock, but the squash's conflict resolution kept main's wiring (mockUseDaemonSessionActivityBridge — the hook App actually calls on main) and dropped the PR's, while the PR's new test kept three references to the old name. Evidence: the PR head tree (7c392d2) defines and exports the mock at the exact spots where main has the renamed one; only the new test's references survived the rename. The failure first appears on 70cf363 (#11250) and repeats identically on 1f89008, 422929b, and d670d47 — four consecutive red main runs, each failing ~14 minutes into the test step.

Fix

Renamed the three dangling references in the test to mockUseDaemonSessionActivityBridge, the mock main defines for the hook App renders through. No production code changed; the assertion's meaning (App must not re-render when split panes report pending state for other sessions) is unchanged. 3 insertions, 3 deletions in one test file.

Reproduction & verification evidence

  • Reproduced locally before the fix: the web-shell suite failed exactly the two reported tests with the same ReferenceError (6712 tests, 2 failed).
  • Mutation probe: temporarily changed handleSplitPendingPanesChange in App.tsx to toggle state on every report; both repaired tests failed as designed, proving the assertions are live witnesses. App.tsx was restored and the full suite re-run green.
  • Cross-check of the other commits between the last green main run (b2e4db8) and HEAD: their touched test files all pass locally, confirming this single defect explains the CI failure signature.

Verification

  • cd packages/web-shell && npx vitest run --config vitest.config.ts (pre-fix) — 2 failed, 6710 passed (reproduction)
  • cd packages/web-shell && npx vitest run --config vitest.config.ts client/App.test.tsx -t "does not rerender App for other split sessions" (post-fix) — 2 passed
  • Mutation probe (App.tsx handler forced to re-render, then restored) — both repaired tests failed as expected under the mutation
  • cd packages/web-shell && npx vitest run --config vitest.config.ts (post-fix, full suite) — 6712 passed, 0 failed
  • npm run test:scripts — 2310 passed, 17 skipped
  • cd packages/core && npx vitest run src/utils/git-branches.test.ts — 106 passed
  • cd packages/core && npx vitest run src/goals/goal-continuation-prompt.test.ts src/goals/goal-runtime.test.ts — 190 passed
  • cd packages/cli && npx vitest run src/acp-integration/session/Session.test.ts src/nonInteractiveCli.test.ts src/ui/hooks/use-llm-stream.test.tsx src/ui/hooks/useMessageQueue.test.ts src/commands/review/lib/worktree.test.ts src/commands/review/test-efficacy.test.ts — 1558 passed, 1 skipped
  • cd packages/cli && npx vitest run src/commands/review/test-efficacy.integration.test.ts — 37 passed
  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint — passed
中文说明

Autofix E2E 报告 — Issue #11415(d670d47e 上 main CI 失败)

摘要

修复了 main 分支 CI 中 Test (ubuntu-latest, Node 22.x) 任务 Run tests and generate reports 步骤的失败。失败原因是 packages/web-shell/client/App.test.tsx 中确定性的 ReferenceError: mockUseDaemonActivePromptBridge is not defined,导致两个 does not rerender App for other split sessions 用例每次都失败(Vitest 的重试无法吸收引用错误)。

根因

#11250 的压缩合并与 main 上的 active-prompt 重构(#11267 / #10941)发生冲突。PR 分支把自己的 mockUseDaemonActivePromptBridge 接入了 ./session-catalog/session-catalog-hooks 模块 mock,但压缩合并的冲突解决保留了 main 的接线(mockUseDaemonSessionActivityBridge——main 上 App 实际调用的 hook),丢弃了 PR 的接线,而 PR 新增的测试仍保留了三处对旧名称的引用。证据:PR 头提交树(7c392d2c6e)在 main 改为新名称的相同位置定义并导出了该 mock;只有新测试中的引用在重命名中漏网。失败首次出现在 70cf363#11250),并在 1f89008、422929b3a7、d670d47e 上以完全相同的方式复现——连续四次 main 运行失败,每次都在测试步骤运行约 14 分钟后失败。

修复

将测试中三处悬空的引用重命名为 mockUseDaemonSessionActivityBridge,即 main 为 App 渲染所经 hook 定义的 mock。未改动任何生产代码;断言的含义(当分屏面板报告其他会话的待处理状态时,App 不应重新渲染)保持不变。单个测试文件中 3 行插入、3 行删除。

复现与验证证据

  • 修复前本地复现:web-shell 测试套件恰好在这两个上报的用例上失败,报相同的 ReferenceError(6712 个测试,2 个失败)。
  • 变异探针:临时把 App.tsx 中的 handleSplitPendingPanesChange 改为每次报告都翻转状态;两个修复后的测试按预期失败,证明断言是有效的见证。随后恢复 App.tsx 并重跑整套件为绿。
  • 交叉检查上一次 main 全绿(b2e4db8624)与 HEAD 之间的其他提交:它们触及的测试文件在本地全部通过,确认这单一缺陷即可解释 CI 失败特征。

验证

  • cd packages/web-shell && npx vitest run --config vitest.config.ts(修复前)——2 失败,6710 通过(复现)
  • cd packages/web-shell && npx vitest run --config vitest.config.ts client/App.test.tsx -t "does not rerender App for other split sessions"(修复后)——2 通过
  • 变异探针(强制 App.tsx 处理器重新渲染,随后恢复)——两个修复后的测试在变异下按预期失败
  • cd packages/web-shell && npx vitest run --config vitest.config.ts(修复后整套件)——6712 通过,0 失败
  • npm run test:scripts——2310 通过,17 跳过
  • cd packages/core && npx vitest run src/utils/git-branches.test.ts——106 通过
  • cd packages/core && npx vitest run src/goals/goal-continuation-prompt.test.ts src/goals/goal-runtime.test.ts——190 通过
  • cd packages/cli && npx vitest run src/acp-integration/session/Session.test.ts src/nonInteractiveCli.test.ts src/ui/hooks/use-llm-stream.test.tsx src/ui/hooks/useMessageQueue.test.ts src/commands/review/lib/worktree.test.ts src/commands/review/test-efficacy.test.ts——1558 通过,1 跳过
  • cd packages/cli && npx vitest run src/commands/review/test-efficacy.integration.test.ts——37 通过
  • npm run build——通过
  • npm run typecheck——通过
  • npm run lint——通过

🧠 Handled by Qwen Code · model/模型 kimi-k3 · CLI 0.23.1

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

qwen-code-ci-bot commented Sep 9, 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

Copy link
Copy Markdown
Collaborator

Thanks for the PR — but I think this one has already been overtaken by events. Worth reading before anyone spends time on the code review.

Template looks good ✓

Problem: this was a genuinely observed failure, not theoretical hardening — main CI was red across four runs on a deterministic ReferenceError, so a reference error that retries cannot absorb. No quarrel with the diagnosis, and the squash-merge collision story in the description matches what is in the tree.

The catch is that the problem is already fixed on main. #11406 landed as 3a75f37e at 2026-09-09T00:10:49Z, doing exactly this rename, and main's unit suite is green there — Test (ubuntu-latest, Node 22.x) and Lint & Static (ubuntu-latest, Node 22.x) both report success on that commit. So as of this review there is no remaining red for this PR to turn green.

Direction: the intent is right — unblock main CI — but the target has moved. The linked issue #11415 is still open; it is a separate auto-filed "Main CI failed" report (for d670d47e) of the same root cause that #11406 already addressed. Its sibling #11404, the same shape of report for 70cf363395, is closed as completed by #11406.

Size: not applicable — one file, packages/web-shell/client/App.test.tsx, test-only. 0 production logic lines, 6 test lines (3+/3−), no generated/schema lines, no core-module paths, single package.

Approach: scope is minimal and I would not cut anything from it. The problem is not scope, it is that the change is a strict subset of what main already contains. I fetched the file as raw bytes at both refs and diffed them; the only difference is that main has two lines this branch does not:

-      // Clear setup-time calls so the guard below measures only this rerender.
-      mockUseDaemonSessionActivityBridge.mockClear();

There is not a single + line — this branch adds nothing main lacks, and the three renamed references are byte-identical to main's. That pre-clear is not cosmetic, and it is the one part of this that concerns me: without it, expect(mockUseDaemonSessionActivityBridge).toHaveBeenCalled() immediately after rerender() is satisfied by the calls setup already made during renderApp() / flush() / opening the split view, so it does not actually prove the rerender reached the hook. Main clears first and therefore measures only the rerender. Merging this branch over that region would quietly weaken the assertion — the opposite of what the description's "confirm the assertion still has teeth" step intends. (The mutation check described there would have been caught by the not.toHaveBeenCalled() loop below, which does have teeth in both versions; it would not have caught the vacuous toHaveBeenCalled() line above it.)

So the honest question for the author: is there anything left to merge here? If the answer is no, closing this PR and #11415 as already-fixed by #11406 is the right outcome. If there is something #11406 missed, please rebase onto current main and reduce the PR to just that delta — right now the delta is empty-to-negative.

Risk: no elevated risk signals — Stage 1e matched nothing (the only changed file is a test file). The residual risk is the merge-time one named above, not the change itself.

Flagging these before diving deeper, and I will still complete the code review and CI-evidence pass below so the record is complete.

中文说明

感谢贡献!不过这个 PR 恐怕已经被主分支的进展覆盖了,建议在投入代码评审之前先看这一段。

模板完整 ✓

问题: 这是一个真实观测到的失败,不是理论性加固——main CI 连续四次运行因确定性的 ReferenceError 变红,而引用错误是重试无法吸收的。诊断没有问题,描述里的压缩合并冲突说法也和代码树的实际情况一致。

但关键在于:这个问题在 main 上已经修好了#11406 已于 2026-09-09T00:10:49Z 以 3a75f37e 合入,做的正是这个重命名,并且 main 在该提交上单元测试是绿的——Test (ubuntu-latest, Node 22.x)Lint & Static (ubuntu-latest, Node 22.x) 均为 success。所以截至本次评审,已经没有等着这个 PR 去修的红了。

方向: 意图是对的——解除 main CI 阻塞——但目标已经移动。关联的 issue #11415 仍处于 open 状态;它是针对同一根因、由自动化另行提交的一份 "Main CI failed" 报告(对应 d670d47e),而 #11406 已经处理过了。它的同类 #11404(针对 70cf363395 的同形状报告)已由 #11406 关闭为 completed。

规模: 不适用——只有一个文件 packages/web-shell/client/App.test.tsx,纯测试改动。生产逻辑 0 行,测试 6 行(3+/3−),无生成/schema 行,未触及核心模块路径,仅涉及单个 package。

方案: 范围已经最小,我不会再砍任何东西。问题不在范围,而在于这个改动是 main 现有内容的严格子集。我把两个 ref 上的该文件按原始字节取下来做了 diff,唯一的差别是 main 比这个分支多两行:

-      // Clear setup-time calls so the guard below measures only this rerender.
-      mockUseDaemonSessionActivityBridge.mockClear();

一个 + 行都没有——本分支没有任何 main 缺少的内容,而那三处重命名与 main 逐字节一致。这个 pre-clear 并非无关紧要,也正是我最担心的一点:没有它,紧跟 rerender() 之后的 expect(mockUseDaemonSessionActivityBridge).toHaveBeenCalled() 会被 setup 阶段(renderApp() / flush() / 打开分屏视图)已经产生的调用满足,因此并不能真正证明本次 rerender 触达了该 hook。main 先做了 clear,所以度量的只是这次 rerender。如果把本分支合入该区域,会悄悄削弱这条断言——这与描述中"确认断言仍然有效"那一步的意图恰好相反。(描述里提到的变异验证会被下面的 not.toHaveBeenCalled() 循环捕获,那部分在两个版本里都是有效的;但它捕获不到上面那条已经失效的 toHaveBeenCalled()。)

所以想请作者如实回答:这里还有需要合并的内容吗? 如果没有,那么把本 PR 和 #11415 作为"已由 #11406 修复"关闭就是正确的结果。如果确实有 #11406 遗漏的部分,请 rebase 到当前 main,并把 PR 缩减到只剩那部分 delta——目前的 delta 是空的,甚至是负的。

风险: 无升级风险信号——Stage 1e 没有命中(唯一改动的文件是测试文件)。剩余风险是上面提到的合并期风险,而不是改动本身。

先把这些提出来讨论。我仍会完成下面的代码评审与 CI 证据环节,以便留下完整记录。

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

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code review

My independent baseline first. Reading only the title and the "Why it's needed" section, here is what I would have done about a ReferenceError: mockUseDaemonActivePromptBridge is not defined in this suite: find the dangling references, look at what the vi.mock('./session-catalog/session-catalog-hooks') factory in that file actually wires, rename the references to that symbol — and, because the assertion the test makes is "App re-rendered", clear the mock immediately before the rerender() so the positive toHaveBeenCalled() measures only that render and not the setup renders above it. Two parts: fix the reference, keep the assertion honest.

What the PR does. The rename, and only the rename. The three references now point at mockUseDaemonSessionActivityBridge, which is the symbol the file's vi.hoisted() block defines and the module factory wires. That part is correct.

The rename target is the right one — and it is worth saying why, because the description's justification is looser than the reality. useDaemonActivePromptBridge is not a typo for a symbol that never existed; it is a real production hook (session-catalog-hooks.ts:365), and it is a thin wrapper whose body is return useDaemonSessionActivityBridge(client, workspaceCwd, sessionId) (:370). It is called by ChatPane.tsx:289 — a child component. The substituted hook, by contrast, is called by App.tsx:3304, i.e. by App's own render. For a test named does not rerender App for other split sessions, a canary on the hook App itself calls is strictly the better choice; the child-level wrapper would only have detected ChatPane rendering. So the substitution lands on the semantically correct symbol rather than merely the nearest defined one. The description calls it "the mock the file actually defines for the active-prompt hook App renders through" — the mechanism is really the session-activity hook App renders through, with the active-prompt hook sitting one level down as a wrapper. Cosmetic inaccuracy in the prose, not in the code.

The blocking finding is not in the diff — it is between the diff and main. I pulled the changed file as raw bytes at both main and this PR's head and diffed them. The entire difference is two lines that main has and this branch does not:

       await act(async () => report(ownerIds));
-      // Clear setup-time calls so the guard below measures only this rerender.
-      mockUseDaemonSessionActivityBridge.mockClear();
       rerender();

No + lines at all. This branch's content is a strict subset of main's, and the three renamed references are byte-identical to what #11406 already landed as 3a75f37e. The two missing lines are precisely the second half of my independent baseline, and their absence is not cosmetic: without the pre-clear, expect(mockUseDaemonSessionActivityBridge).toHaveBeenCalled() on the line after rerender() is satisfied by the calls already accumulated from renderApp(), flush(), and the split-view click earlier in the same test body. It cannot fail when the rerender does nothing, so it does not establish what it appears to establish. The mockClear() after it means the not.toHaveBeenCalled() loop below still has real teeth in both versions — the weakened assertion is only the positive one.

Merging this branch over that region would therefore delete two lines from main and silently downgrade that assertion. GitHub currently reports the PR as MERGEABLE / BLOCKED, and I have not verified which side a merge would take on that overlapping hunk — I am flagging the exposure, not asserting the outcome.

No other findings. No production code is touched, no AGENTS.md conventions are implicated (test collocated with source, PascalCase.tsx, ESM), no reuse concern — the change adds nothing new. One file, six lines.

No sandboxed lane named, explicitly. This is a test-only change with no production surface and no TUI behaviour, so @qwen-code /verify and @qwen-code /tmux have no claim to settle; the finding above is settled by the byte-level diff, not by running anything. I ran no PR-derived code — per the gate's rules the review is static and all test evidence below comes from the PR's own CI via the API.

Test evidence

What this section carries: the PR's own CI check results for the reviewed commit, fetched through the API, plus the CI result on the main commit that already contains the same fix. I did not build or run anything. Nothing here is the author's local numbers — the description's "6712 passed, 0 failed" is the author's own claim from a Linux-only run and I am not presenting it as evidence.

Zero checks are red on this commit. The decisive jobs are still in flight, so the suite result that this PR exists to demonstrate is not yet in: Test (ubuntu-latest, Node 22.x) and Lint & Static (ubuntu-latest, Node 22.x) are both in_progress, and the macOS and Windows test lanes are skipped for this PR, so a green ubuntu lane would be the only unit-suite signal. I am not polling or predicting them; the finalize job rewrites the table below once CI settles.

The more relevant signal is the one on main. On 3a75f37e — the commit carrying the identical rename plus the two lines this branch lacks — Test (ubuntu-latest, Node 22.x) and Lint & Static (ubuntu-latest, Node 22.x) both report success. Main's unit suite is already green, which is the direct evidence that the failure described in #11415 is fixed and that nothing is waiting on this PR.

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

Check Conclusion
Capture web-shell visuals (ubuntu-latest, Node 22.x) ✅ success
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

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

中文说明

代码评审

先说我独立的方案。 只看标题和"为什么需要"这一节,面对本套件里的 ReferenceError: mockUseDaemonActivePromptBridge is not defined,我会这样做:找到悬空引用,查看该文件里 vi.mock('./session-catalog/session-catalog-hooks') 工厂实际接入了哪个符号,把引用改成那个符号;同时,由于该测试要断言的是"App 发生了重渲染",应在 rerender() 之前立即 clear 这个 mock,让正向的 toHaveBeenCalled() 只度量这一次渲染,而不包含它上面 setup 阶段的渲染。两部分:修好引用,并让断言保持有效。

本 PR 做了什么。 只做了重命名。三处引用现在指向 mockUseDaemonSessionActivityBridge,也就是该文件 vi.hoisted() 块定义、模块工厂接入的那个符号。这部分是正确的。

替换的目标选对了——这一点值得说明,因为描述里的理由比实际情况更含糊。 useDaemonActivePromptBridge 并不是一个从未存在过的符号的笔误;它是真实的生产 hook(session-catalog-hooks.ts:365),而且是个薄封装,函数体就是 return useDaemonSessionActivityBridge(client, workspaceCwd, sessionId):370)。它由 ChatPane.tsx:289 调用,也就是子组件。相比之下,被替换进来的这个 hook 由 App.tsx:3304 调用,即 App 自身的渲染。对一个名为 does not rerender App for other split sessions 的测试来说,把探针挂在 App 自己调用的 hook 上严格更优;子组件层的封装只能探测到 ChatPane 的渲染。所以这次替换落在语义正确的符号上,而不只是最近的、恰好有定义的那个。描述里称它为"该文件为 App 渲染所经的 active-prompt hook 实际定义的 mock"——真实机制是 App 渲染所经的 session-activity hook,而 active-prompt hook 在其下一层作为封装存在。这是措辞上的小瑕疵,代码没有问题。

阻塞性问题不在 diff 里,而在 diff 与 main 之间。 我把改动文件按原始字节分别从 main 和本 PR head 取下来做了 diff,全部差别就是 main 有、本分支没有的两行:

       await act(async () => report(ownerIds));
-      // Clear setup-time calls so the guard below measures only this rerender.
-      mockUseDaemonSessionActivityBridge.mockClear();
       rerender();

完全没有 + 行。本分支的内容是 main 的严格子集,而那三处重命名与 #11406 已经以 3a75f37e 合入的内容逐字节一致。缺的这两行恰恰是我独立方案的后半部分,而且它们的缺失并非无关紧要:没有这个 pre-clear,rerender() 之后那一行的 expect(mockUseDaemonSessionActivityBridge).toHaveBeenCalled() 会被同一个测试体前面 renderApp()flush() 和点击打开分屏视图所累积的调用满足。即使这次 rerender 什么都没做它也不会失败,因此它并不能证明它看起来要证明的事情。它之后的那次 mockClear() 意味着下面的 not.toHaveBeenCalled() 循环在两个版本里都仍然有效——被削弱的只有那条正向断言。

因此,把本分支合入该区域会删掉 main 上的两行,并悄悄降级那条断言。GitHub 目前报告本 PR 为 MERGEABLE / BLOCKED,我没有验证在这个重叠 hunk 上合并会取哪一侧——我是在提示这个暴露面,而不是断言结果。

没有其他发现。 未触及生产代码,不涉及 AGENTS.md 约定(测试与源码同目录、PascalCase.tsx、ESM),也没有复用问题——该改动没有新增任何东西。一个文件,六行。

明确不指定沙箱验证通道。 这是纯测试改动,没有生产接口面,也没有 TUI 行为,所以 @qwen-code /verify@qwen-code /tmux 没有可供判定的主张;上面那个结论由字节级 diff 判定,而不是靠运行任何东西。我没有执行任何来自 PR 的代码——按本门禁规则,评审是静态的,下面所有测试证据均来自 API 读取的 PR 自身 CI。

测试证据

本节携带的内容:通过 API 获取的、针对被评审提交的 PR 自身 CI 检查结果,外加那个已包含相同修复的 main 提交上的 CI 结果。我没有构建或运行任何东西。这里没有作者本地的数字——描述中的"6712 通过、0 失败"是作者自己在仅 Linux 环境下运行得出的说法,我不将其作为证据呈现。

该提交上没有任何检查是红的。但决定性的作业仍在运行中,所以本 PR 想要证明的那个套件结果还没有出来:Test (ubuntu-latest, Node 22.x)Lint & Static (ubuntu-latest, Node 22.x) 均为 in_progress,而本 PR 的 macOS 与 Windows 测试通道是 skipped,因此绿的 ubuntu 通道将是唯一的单元测试信号。我不会轮询或预测它们;CI 落定后 finalize 作业会重写上面的表格区域。

更相关的信号在 main 上。在 3a75f37e——即携带同样重命名、外加本分支缺少的那两行的提交——上,Test (ubuntu-latest, Node 22.x)Lint & Static (ubuntu-latest, Node 22.x) 均为 success。main 的单元测试已经是绿的,这直接证明 #11415 描述的失败已被修复,也说明没有任何事情在等待本 PR。

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

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 2/5 — the three lines are correct as far as they go, but there is nothing left to merge and merging them would take two useful lines back off main.

Stepping back, this is not a close call and it is not a judgement about code quality. The rename is right, and it is right for a better reason than the description gives: the substituted hook is the one App.tsx calls during its own render, while the hook it replaced is a wrapper one level down that only ChatPane calls. Whoever picked that symbol — or whatever did — landed on the semantically correct canary for a test asserting that App does not re-render.

The problem is that main got there first, and got there more completely. #11406 landed as 3a75f37e with the same rename plus a mockClear() before the rerender(), and main's unit suite is green on that commit. Diffing the changed file as raw bytes at both refs returns no added lines at all — this branch is a strict subset of main. So the answer to "does this PR still do anything?" is no, and the answer to "could merging it hurt?" is yes: those two absent lines are what stop the positive toHaveBeenCalled() assertion from being satisfied by setup-time calls. Without them the assertion cannot fail when the rerender does nothing. Six months from now, whoever reads that test will trust a check that checks nothing, and git blame will point at this merge.

If I had to name what actually went wrong here, it is not the diff — it is that this PR was generated against a four-run-old picture of main and never re-checked after #11406 landed. #11415 and #11404 are the same auto-filed "Main CI failed" report for two different red commits of one root cause; the first got a PR that merged, the second got a PR that is now moot. That race is worth a maintainer's attention at the automation level, though nothing about it is this diff's fault and I am not deferring the merge decision on account of it.

The ask is simple, and it is the same one Stage 1 raised: if there is genuinely nothing #11406 missed, close this PR and close #11415 as already fixed. If something was missed, rebase onto current main and reduce the PR to that delta — at which point the pre-clear discussion above becomes the thing to get right, not the rename.

CI note: Qwen Code CI and Web-shell Visuals were still in_progress on the reviewed commit when I fetched (0 failures so far). I am not waiting on them, and no approval is deferred here — the verdict does not turn on a suite result, because the commit that already contains this fix is green on main.

中文说明

Confidence: 2/5 —— 这三行代码本身没问题,但已经没有东西可合并了,而且合并会让 main 少掉两行有用的代码。

退一步看,这不是一个模棱两可的判断,也不是对代码质量的评价。重命名是对的,而且对的理由比描述里给的更好:被替换进来的 hook 是 App.tsx 在自己渲染过程中调用的那个,而被替换掉的 hook 是下一层的一个封装,只有 ChatPane 调用它。选定这个符号的人(或流程)落在了语义正确的探针上——对一个断言 App 不重渲染的测试来说正是如此。

问题在于 main 先到了,而且到得更完整。#114063a75f37e 合入,带了同样的重命名,外加 rerender() 之前的一次 mockClear(),并且 main 的单元测试在该提交上是绿的。把改动文件按原始字节在两个 ref 上 diff,结果是完全没有新增行——本分支是 main 的严格子集。所以"这个 PR 还有作用吗?"的答案是没有;而"合并它会不会有害?"的答案是会:正是它缺少的那两行,才使得正向的 toHaveBeenCalled() 断言不会被 setup 阶段的调用满足。没有它们,即使这次 rerender 什么都没做,该断言也不会失败。六个月后,读到这个测试的人会信任一条什么都不检查的检查,而 git blame 会指向这次合并。

如果要说这里真正出错的是什么,不是 diff,而是这个 PR 是基于四分之一个"main 的旧快照"生成的,并且在 #11406 合入之后从未重新核对。#11415#11404 是同一根因下、针对两个不同红色提交、由自动化提交的同形状 "Main CI failed" 报告;第一个拿到了一个已合并的 PR,第二个拿到的 PR 现在已经没有意义。这个竞态值得维护者从自动化流程层面关注,但它不是本 diff 的问题,我也没有因此把合并决定推给别人。

诉求很简单,和 Stage 1 提出的是同一个:如果确实没有 #11406 遗漏的内容,请关闭本 PR,并把 #11415 作为已修复关闭。如果确实有遗漏,请 rebase 到当前 main 并把 PR 缩减到只剩那部分 delta——到那时,上面关于 pre-clear 的讨论才是需要做对的事情,而不是重命名。

CI 说明:我抓取时,被评审提交上的 Qwen Code CIWeb-shell Visuals 仍为 in_progress(目前 0 失败)。我不等待它们,这里也没有推迟任何批准——结论并不取决于套件结果,因为已经包含该修复的那个提交在 main 上是绿的。

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

Reviewed at 6bf41c93dee21966613f9ffda9e7d352712ea192 · 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.

Needs rethinking rather than review — see my notes above. 🙏

The short version, @qwen-code-dev-bot: the fix this PR makes is already on main. #11406 landed the identical rename as 3a75f37e, and main's unit suite is green there. Diffing the changed file as raw bytes at both refs shows this branch adds no lines main lacks — it is a strict subset, and it is missing the mockClear() before rerender() that main has, which is what keeps expect(mockUseDaemonSessionActivityBridge).toHaveBeenCalled() from being satisfied by setup-time calls. Merging would take two useful lines back off main and leave an assertion that cannot fail.

Two ways forward:

  • Nothing was missed by #11406 → close this PR and close #11415 as already fixed.
  • Something was missed → rebase onto current main and reduce the PR to that delta alone.
中文说明

这个 PR 需要重新考虑,而不是继续评审——请参见上面的说明。🙏

简要版本,@qwen-code-dev-bot:本 PR 要做的修复已经在 main 上了。#11406 已以 3a75f37e 合入了完全相同的重命名,main 的单元测试在该提交上是绿的。把改动文件按原始字节在两个 ref 上 diff,可以看到本分支没有任何 main 缺少的行——它是严格子集,而且缺少 main 上 rerender() 之前的那次 mockClear();正是它让 expect(mockUseDaemonSessionActivityBridge).toHaveBeenCalled() 不会被 setup 阶段的调用满足。合并会让 main 少掉两行有用的代码,并留下一条不可能失败的断言。

两种走法:

  • #11406 没有遗漏任何东西 → 关闭本 PR,并把 #11415 作为已修复关闭。
  • 确实有遗漏 → rebase 到当前 main,并把 PR 缩减到只剩那部分 delta。

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

🖼️ web-shell visual preview

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

Screenshots · before / after

session-workflow-cockpit-dark before/after

session-workflow-cockpit-light before/after

terminal-turn-error-copy-narrow-dark before/after

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

Qwen Code · web-shell visuals

@wenshao

wenshao commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Local verification report — PR #11416

I built a real three-arm harness for this one and ran it end to end. The fix is correct and it does exactly what the description says — but it is already on main, and merging it now is a no-op. My recommendation is to close this PR and close #11415 as fixed by #11406.

One correction to the automated triage note above, because it matters for how this gets closed: merging this branch would not take main's two extra lines back off. I measured it — see §4 below.

Harness

The three refs I compared differ in exactly one file:

arm ref what it is
A d670d47e the PR's merge-base — main while CI was red
B 6bf41c93 this PR's head
C 3a75f37e current main (#11406, which landed 33 min before this PR was opened)

git diff d670d47e 3a75f37e and git diff d670d47e 6bf41c93 both touch only packages/web-shell/client/App.test.tsx, so a single worktree with that one file swapped between runs is a faithful A/B/C — nothing else varies between arms. Worktree pinned at the merge-base, dependencies symlinked from a sibling checkout, acp-bridge / sdk / web-shell dists built once and shared. Vitest 3.2.4, Node 22.22.2, Linux, jsdom.

1. The failure reproduces, and the PR fixes it

arms

Arm A fails deterministically at App.test.tsx:28940 with the exact ReferenceError from the red CI job, both parameterised cases. Arms B and C pass.

Whole suite, same worktree, same command (npx vitest run --config vitest.config.ts from packages/web-shell):

full suite

arm test files tests
A — merge-base 1 failed | 287 passed (288) 2 failed | 6710 passed (6712)
B — this PR 288 passed (288) 6712 passed (6712)
C — current main 288 passed (288) 6712 passed (6712)

The description's "observed locally: 6712 passed, 0 failed" reproduces exactly.

2. The symbol it picked is the right one, not just a convenient one

The description says the references "now point at the mock the file actually defines", which undersells it. App.tsx:3304 calls useDaemonSessionActivityBridge during App's own render; useDaemonActivePromptBridge (session-catalog-hooks.ts:365) is a thin wrapper around it that only ChatPane.tsx:289 calls, and the vi.mock('./session-catalog/session-catalog-hooks') factory in App.test.tsx does not export it at all. So for a test asserting "App re-rendered", the substituted symbol is the semantically correct witness.

I checked the obvious alternative repair — declare mockUseDaemonActivePromptBridge, wire it into the module mock, leave the assertions alone:

counterfactual

It fails: the spy is never called, because App never calls that hook. Renaming was the only repair that works.

3. What the restored assertions actually guard

Two mutations, applied identically to arm B and arm C.

M2 — a real production regression. I made handleSplitPendingPanesChange in App.tsx force a re-render on every pane-pending report, i.e. exactly the bug the test is named after. Both arms go red, both cases. The behavioural guard this PR restores has teeth.

M1 — delete the rerender() call, i.e. break the instrumentation the positive guard is supposed to measure. Here the arms diverge:

mutation matrix

On arm C the mutant is killed in both cases, at the guard line (:28942). On arm B the outer pending: false case passes — the mutant survives — and the other case only dies later, in the negative loop at :28944. The guard line itself never fires.

The reason is visible directly. I instrumented the guard region on both arms (no behaviour change, just a call-count print):

probe

On this branch the spy already holds 5–6 setup-time calls from renderApp() / flush() / the split click when expect(...).toHaveBeenCalled() runs, so that assertion is satisfied no matter what the rerender() did. main's extra mockClear() before rerender() zeroes it, and the guard then measures exactly one render: 0 → 1.

So the description's claim that the assertion "still has teeth" is half right: the negative half does, the positive half does not on this branch. main already fixed the other half.

4. What merging would actually do

merge simulation

  • git merge --squash pr-head onto main → merges cleanly, empty staged diff.
  • git merge-tree --write-tree main pr-head → tree 35a388f0, byte-identical to main.
  • git cherry-pick (the rebase path) → "nothing to commit, working tree clean".

Under every merge strategy this repo uses, landing #11416 changes nothing on main. It is not a regression — it is simply empty.

5. Issue bookkeeping

All four red main commits carry the three dangling references (70cf363395, 1f890086f1, 422929b3a7, d670d47e → 3 occurrences each); 3a75f37e carries 0 and its CI run is green. The description's root-cause narrative checks out.

#11415 is still open, but it is the same failure as #11404 one commit earlier, and #11406 already closed that one. It should be closed pointing at 3a75f37e.

Verdict

Close this PR; nothing to merge. No defect in the change itself — the diagnosis is right, the symbol choice is right, the suite is green, and the PR's own CI Test job passed. It is just 33 minutes late, and main's version of the same fix is strictly better because it also makes the positive guard measurable.

Non-blocking, for the autofix flow rather than for this diff:

  • N1 — a git merge-tree / git log check against current main immediately before opening would have caught this: fix(web-shell): Replace undefined mock in split rerender tests (#11404) #11406 landed at 00:10:32Z, this PR opened at 00:43:25Z.
  • N2 — the Reviewer Test Plan's "the test fails if App re-renders … verified via a temporary mutation of the pane-pending handler" is accurate (that mutation does kill both cases on this branch), but the plan doesn't distinguish the positive guard, which is vacuous here. Worth stating separately when a test carries both a positive and a negative assertion.

What I did not check

Linux only; macOS and Windows lanes were not run locally. Unit tests only (jsdom) — no browser or e2e lane. Dependencies came from a sibling checkout rather than a fresh npm ci, so the absolute run durations are not CI-comparable; the pass/fail outcomes are, and they match this PR's own green CI Test job.

中文说明

本地验证报告 —— PR #11416

我为这个 PR 搭了一套真实的三臂验证环境并完整跑通。修复本身是正确的,也确实做到了描述里说的事——但它已经在 main 上了,现在合入是个空操作。 我的建议是关闭本 PR,并把 #11415 标记为由 #11406 修复后关闭。

对上面自动 triage 结论的一处更正,因为它影响到该怎么关闭这个 PR:合入本分支不会把 main 上多出的那两行拿掉。我实测过——见下面第 4 节。

验证环境

参与对比的三个 ref 只在一个文件上有差异:

ref 说明
A d670d47e 本 PR 的 merge-base —— CI 变红时的 main
B 6bf41c93 本 PR 的 head
C 3a75f37e 当前 main(#11406,比本 PR 早 33 分钟合入)

git diff d670d47e 3a75f37egit diff d670d47e 6bf41c93 都只触及 packages/web-shell/client/App.test.tsx,所以用同一个 worktree、只替换这一个文件来做 A/B/C 是忠实的——各臂之间没有任何其他变量。worktree 固定在 merge-base,依赖从相邻 checkout symlink 过来,acp-bridge / sdk / web-shell 的 dist 只构建一次并共用。Vitest 3.2.4、Node 22.22.2、Linux、jsdom。

1. 故障可复现,且本 PR 修好了它

arms

A 臂在 App.test.tsx:28940 处确定性地抛出与红色 CI 作业完全相同的 ReferenceError,两个参数化用例都挂。B、C 两臂通过。

同一 worktree、同一命令(在 packages/web-shell 下执行 npx vitest run --config vitest.config.ts)跑全量套件:

full suite

测试文件 测试用例
A —— merge-base 1 failed | 287 passed (288) 2 failed | 6710 passed (6712)
B —— 本 PR 288 passed (288) 6712 passed (6712)
C —— 当前 main 288 passed (288) 6712 passed (6712)

描述中「本地实测:6712 通过、0 失败」完全复现。

2. 它选中的符号是语义正确的,而不只是碰巧能过

描述说这些引用「现在指向该文件实际定义的 mock」,这个说法低估了它。App.tsx:3304 在 App 自身渲染过程中调用 useDaemonSessionActivityBridge;而 useDaemonActivePromptBridgesession-catalog-hooks.ts:365)只是它的一层薄封装,只有 ChatPane.tsx:289 会调用,并且 App.test.tsxvi.mock('./session-catalog/session-catalog-hooks') 的工厂根本没有导出它。所以对一个断言「App 是否重渲染」的测试来说,替换后的符号才是语义正确的观测点。

我验证了另一种显而易见的修法——声明 mockUseDaemonActivePromptBridge、把它接进模块 mock、断言原样不动:

counterfactual

结果是失败:该 spy 从未被调用,因为 App 根本不调用那个 hook。重命名是唯一可行的修法。

3. 恢复后的断言究竟守住了什么

两个变异,同样地施加到 B 臂和 C 臂。

M2 —— 真实的生产回归。 我把 App.tsx 里的 handleSplitPendingPanesChange 改成在每次 pane-pending 上报时都强制重渲染,也就是这个测试名字所指的那个 bug。两臂都变红,两个用例都挂。本 PR 恢复的行为守卫是有效的。

M1 —— 删掉 rerender() 调用,也就是破坏正向守卫本应测量的那次渲染。这里两臂出现分歧:

mutation matrix

C 臂两个用例都被杀死,且正是在守卫那一行(:28942)。B 臂的 outer pending: false 用例通过了——变异体存活——另一个用例只是稍后在负向循环 :28944 处才挂。守卫那一行自己从未触发。

原因可以直接看到。我在两臂的守卫区域插了探针(不改变行为,只打印调用次数):

probe

在本分支上,当 expect(...).toHaveBeenCalled() 执行时,spy 已经因为 renderApp() / flush() / 分屏点击累积了 5–6 次调用,所以无论那次 rerender() 做了什么,这个断言都必然成立。main 在 rerender() 之前多加的那次 mockClear() 把它清零,守卫于是恰好测量一次渲染:0 → 1。

所以描述里「断言仍然有效」这句只说对了一半:负向那半有效,正向那半在本分支上是空的。而 main 已经把另一半也补上了。

4. 合入到底会改变什么

merge simulation

  • 在 main 上执行 git merge --squash pr-head → 干净合并,暂存区 diff 为空
  • git merge-tree --write-tree main pr-head → tree 35a388f0与 main 逐字节相同
  • git cherry-pick(rebase 路径)→ 「nothing to commit, working tree clean」。

在本仓库使用的任何一种合并策略下,合入 #11416 都不会改变 main 的任何内容。这不是回归——它只是空的。

5. Issue 归档

四个红色 main 提交都带着那三处悬空引用(70cf3633951f890086f1422929b3a7d670d47e 各 3 处);3a75f37e 为 0,且它那次 CI 是绿的。描述里的根因叙述属实。

#11415 仍然开着,但它与前一个提交上的 #11404 是同一个故障,而 #11406 已经关闭了后者。它应当指向 3a75f37e 后关闭。

结论

关闭本 PR,没有可合入的内容。 改动本身没有缺陷——诊断正确、符号选择正确、套件全绿、本 PR 自己的 CI Test 作业也通过了。它只是晚了 33 分钟,而 main 上同一修复的版本严格更好,因为它还让正向守卫变得可测量。

以下为非阻塞项,针对 autofix 流程而非这份 diff:

  • N1 —— 在开 PR 之前立刻对当前 main 做一次 git merge-tree / git log 检查就能拦住这次:fix(web-shell): Replace undefined mock in split rerender tests (#11404) #11406 于 00:10:32Z 合入,本 PR 于 00:43:25Z 创建。
  • N2 —— Reviewer Test Plan 里「当分屏面板报告其他会话的待处理状态时若 App 发生重渲染,该测试会失败……开发过程中曾通过对 pane-pending 处理器的临时变异验证」这句是准确的(该变异确实在本分支上杀死了两个用例),但这份计划没有把正向守卫单独区分出来,而它在这里是空的。当一个测试同时含有正向与负向断言时,值得分开陈述。

未覆盖的部分

仅 Linux;macOS 与 Windows 通道未在本地运行。仅单元测试(jsdom)——未跑浏览器或 e2e 通道。依赖来自相邻 checkout 而非全新 npm ci,因此绝对耗时不可与 CI 对比;通过/失败结论是可比的,并且与本 PR 自己绿色的 CI Test 作业一致。


🤖 Generated with Claude Code — Claude Opus 5 (1M context)

@wenshao

wenshao commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Closing this as superseded — nothing here is wrong, there is just nothing left to merge.

#11406 landed the identical rename as 3a75f37e 33 minutes before this PR was opened, plus a mockClear() before rerender() that this branch does not have. I verified locally that merging this branch onto today's main is a provable no-op — git merge --squash leaves an empty staged diff, git merge-tree --write-tree produces a tree byte-identical to main, and git cherry-pick reports "nothing to commit". Full details and evidence in my verification report above.

To be explicit for anyone reading the thread later: closing this is not a judgement on the change. The diagnosis was right, the substituted symbol was the semantically correct one, and the whole web-shell suite is green on this head. It was simply overtaken.

中文说明

作为已被取代的 PR 关闭 —— 这里没有任何错误,只是已经没有可合入的内容了。

#11406 在本 PR 创建前 33 分钟就以 3a75f37e 合入了完全相同的重命名,并且多了一处本分支没有的、位于 rerender() 之前的 mockClear()。我在本地验证过:把本分支合入今天的 main 是可证明的空操作 —— git merge --squash 后暂存区 diff 为空,git merge-tree --write-tree 得到的 tree 与 main 逐字节相同,git cherry-pick 报 "nothing to commit"。完整细节与证据见上方的验证报告

为了让后来看这个线程的人清楚:关闭它不是对这份改动的否定。诊断是对的,替换后的符号是语义正确的那个,整个 web-shell 套件在这个 head 上也是全绿的。它只是被抢先了。


🤖 Generated with Claude Code — Claude Opus 5 (1M context)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Main CI failed: Qwen Code CI on d670d47efe3d

3 participants