Skip to content

fix(vscode): give the edit back when a permission diff is closed - #11171

Merged
yiliang114 merged 21 commits into
mainfrom
fix/vscode-permission-diff-dismissal
Sep 11, 2026
Merged

yiliang114 merged 21 commits into
mainfrom
fix/vscode-permission-diff-dismissal

Conversation

@yiliang114

@yiliang114 yiliang114 commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Closing a host-owned permission diff by hand left the user having to approve or reject an edit they could no longer look at.

The gap, end to end:

  • onDidCloseTextDocument routes the close to DiffManager.cancelDiff, which fires ide/diffClosed on onDidChange. The only consumer of that event is ide-server.ts — IDE-mode MCP transports. The web shell that asked for the diff is not one, so it was never told.
  • EmbeddedApp kept the request in openPermissionDiffsRef, so updateTranscript never re-posted openDiff for it.
  • ToolGroup kept the row locked, because hostOwnsEditDiffPreview says the host owns the edit preview — and ToolApproval therefore renders no diff.

DiffManager now also fires a typed onDidClosePermissionDiff when the diff it closed had a permissionRequestId. extension.ts fans it out to the permission-aware providers — the same registry qwen.diff.accept and qwen.diff.cancel already use — and WebViewProvider posts it to its webview.

EmbeddedApp treats that as the host handing the preview back, not as a vote: it drops the request from openPermissionDiffsRef, stops passing hostOwnsEditDiffPreview, and does not reopen the tab the user just closed. The row unlocks, the web shell renders the diff inline, and the approval can be answered against something visible. Ownership returns to the host on the next permission request, or when the pending diffs are torn down.

This is direction 1 and 2 from the issue combined, and it needs no new customization surface: hostOwnsEditDiffPreview is already a boolean, and only one permission is pending at a time.

Two paths that deliberately do not trigger it

Both are pinned by tests, because either one firing would be worse than the bug:

  • qwen.diff.accept / qwen.diff.cancel never reach cancelDiff for a request-bound diff — they are guarded by !permissionRequestId and route the vote through respondToPendingPermission instead.
  • closeDiffEditor deletes the map entry before closing the tab, so the onDidCloseTextDocumentcancelDiff hop that follows a web-shell-initiated closeDiff finds nothing. Without this, a close the web shell asked for would echo back as a dismissal and the diff would reopen in a loop.

Why it's needed

The approval flow lost the artifact it was asking the user to judge. Once the host opened the diff, hostOwnsEditDiffPreview made the web shell render no diff of its own, so the native tab was the only place the change could be read. Closing that tab was a normal, user-initiated act — but nothing told the web shell it had happened, so the tool row stayed locked and the pending approval stayed live. The user was left with a vote to cast and no way to see what they were voting on: approving blind, rejecting blind, or abandoning the turn.

Two follow-on gaps are fixed in the same PR because both were found while witnessing this one:

Reviewer Test Plan

How to verify

  1. Start an edit that needs approval in the VS Code companion. The host opens the diff and the tool row is locked.
  2. Close the diff tab with ✕ without voting. Expected: the row unlocks and shows the diff inline; the tab does not spring back open.
  3. Approve or reject from the row. Expected: the vote lands, exactly as it would have from the diff editor.
  4. Approve from the diff editor instead (qwen.diff.accept). Expected: unchanged behavior — no unlock, no reopen.
  5. Trigger a second permission request. Expected: the host opens its diff again and the row locks again.

What was and was not executed locally:

  • Run: packages/vscode-ide-companionsrc/diff-manager.test.ts 18 passed, src/webview/EmbeddedApp.test.tsx 31 passed, src/commands/index.test.ts 10 passed. packages/web-shellclient/App.test.tsx 746 passed (the full file, since Restore VS Code message edit and rewind after the WebShell cutover #9911's fix touches a shared submit path), client/components/messages/ToolGroup.test.tsx 106 passed.
  • Not run: src/extension.test.ts@qwen-code/qwen-code-core has no build on this machine, so the file cannot be collected here. The change to extension.ts is a 7-line additive subscription inside the existing context.subscriptions.push(...) list, against an event that exists on the real DiffManager (that file's tests use the real class and spy only on hasDiff / getPermissionRequestId), but the activation path is CI's to confirm.
  • Not run: any build, typecheck, or the rest of the suite.

Evidence (Before & After)

Outstanding — no visual capture exists for this PR, and the automated witnesses below are not a substitute for one. Steps 2 and 5 are host-side behaviors (the native diff tab staying closed, then reopening on the next request) that need a VS Code Extension Development Host session to record; none was run. This is stated plainly rather than papered over with pass counts.

What the suites do pin, at the unit level:

  • Before: closing a request-bound diff left the entry in openPermissionDiffsRef and left hostOwnsEditDiffPreview asserted, so ToolApproval rendered no diff and the row stayed locked. After: diff-manager.test.ts (18 passed) witnesses onDidClosePermissionDiff firing only for a diff carrying a permissionRequestId, and EmbeddedApp.test.tsx (31 passed) witnesses the request being dropped, the flag no longer passed, and the tab not being reopened.
  • The two must-not-fire paths in "Two paths that deliberately do not trigger it" each have a test that fails if the guard is removed.
  • ToolGroup.test.tsx (106 passed) witnesses the row unlocking and rendering the diff inline once the host hands the preview back.
  • For Restore VS Code message edit and rewind after the WebShell cutover #9911, App.test.tsx (746 passed) witnesses the toast firing on a rejected preflight, including the session-switch case that pins the sessionId captured before the await in prepareSubmit.

Tested on

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

✅ tested · ⚠️ not tested · N/A

Linux is where the unit suites above were executed. macOS and Windows were not tested locally, and at this commit the Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) lanes were skipped in CI, so their suites did not run there either. That matters for a VS Code companion change shipping to all three platforms, and it matters again because the round-2 Critical R2-1 was a Windows-only test failure no Linux lane could catch — platform confirmation is owed by CI before merge.

Environment (optional)

N/A — unit tests only. No Extension Development Host session, no Docker/Podman sandbox, no local daemon runtime.

Risk & Scope

  • Main risk or tradeoff: hostOwnsEditDiffPreview becomes stateful in the companion instead of a constant true. If it were ever left false after the pending permission resolved, later requests would render inline instead of in a native diff — hence the reset on both the request-change path and closeOpenPermissionDiffs, and a test for each. The user-visible behavior change is that closing a permission diff no longer means losing the edit; nothing about voting from the diff editor changes.
  • Not validated / out of scope: no local build or typecheck was run; src/extension.test.ts was not collected locally (missing core/acp-bridge builds) so the activation fan-out and its R3-7 / R3-8 witnesses are CI's to confirm; no macOS or Windows execution locally or in CI at this commit; no Before/After recording of the host-side diff tab. Also out of scope: the related single-slot onExit concern from the same review round, already fixed on main (see vscode-ide-companion: superseded daemon child still fires onExit, showing a false crash banner #10378); and R3-14, verified as a real observation and deliberately not fixed because it is a behaviour decision recorded on the issue. Whether Extension Development Host screenshots are still wanted for Restore VS Code message edit and rewind after the WebShell cutover #9911 now that the interaction is not new code remains an open question for the maintainer.
  • Breaking changes / migration notes: none. No public API, setting, or persisted format changes; onDidClosePermissionDiff is additive on DiffManager, and the composer.editUnavailable / composer.editExpired strings already existed and were merely given a consumer.

Also in this PR

#10585 — all twelve test-witness gaps. Two commits: the companion set (commands/index.test.ts, diff-manager.test.ts, webview/EmbeddedApp.test.tsx, extension.test.ts) and the web-shell set (App.test.tsx, ToolGroup.test.tsx). Ten were run here; R3-7 and R3-8 live in extension.test.ts, which pulls @qwen-code/qwen-code-core and @qwen-code/acp-bridge — neither has a build on this machine — so those two are CI's to confirm. R3-14 was verified as a real observation and deliberately not fixed; it is a behaviour decision, recorded on the issue.

#9911 — the two items that survived the audit. The issue's premise is stale: per-message edit/rewind shipped with the cutover, daemon-backed through getRewindSnapshots / rewindSession, so the ACP contract it was opened to design was never needed. What was actually missing:

  • A rejected preflight was silent. Both submit paths cancelled the prompt with a console.warn and nothing else. The companion throws composer.editUnavailable and composer.editExpired here — fully localized, English and Chinese — and nothing else in the repository consumed them. They were written to be read by a user and could never reach one. A failed rewind looked like the edit did nothing, with the composer still in editing mode.
  • Nothing tested the flow. getRewindSnapshots and rewindSession appeared in EmbeddedApp.test.tsx only as mock stubs.

The toast fires only while the user is still on the session the submission belonged to, and that guard is deliberately narrower than the existing submissionOwnerIsCurrent — the full guard tracks composer identity, and submitting is what moves it, so reusing it would suppress the very message the user needs. The first draft did reuse it; the new test caught that the toast never fired.

Remaining on #9911: whether the Extension Development Host screenshots are still wanted now that the interaction is not new code. All three rewind cases the audit named are now witnessed: the chosen-turn and expired-snapshot ones, plus the session-switch case (a switch between getRewindSnapshots and rewindSession), which pins the sessionId capture taken before the await in prepareSubmit.

Linked Issues

Fixes #10557
Refs #10585
Refs #9911

Follow-up from the #9811 WebShell cutover.

中文说明

这个 PR 做了什么

手动关掉一个由宿主(host)持有的权限 diff 之后,用户仍然被要求批准或拒绝一个自己已经看不到的改动。

完整的缺口链路:

  • onDidCloseTextDocument 把关闭事件路由到 DiffManager.cancelDiff,后者在 onDidChange 上发出 ide/diffClosed。这个事件唯一的消费者是 ide-server.ts——也就是 IDE 模式的 MCP 传输层。发起这个 diff 的 web shell 不属于这一类,所以它从来没有被告知。
  • EmbeddedApp 把该请求继续留在 openPermissionDiffsRef 里,于是 updateTranscript 再也不会为它重新投递 openDiff
  • ToolGroup 让这一行保持锁定,因为 hostOwnsEditDiffPreview 声明宿主拥有编辑预览——于是 ToolApproval 根本不渲染 diff。

现在 DiffManager 在关闭的 diff 带有 permissionRequestId 时,还会额外发出一个带类型的 onDidClosePermissionDiffextension.ts 把它分发给具备权限能力的 provider——用的正是 qwen.diff.acceptqwen.diff.cancel 已经在用的那套注册表——WebViewProvider 再把它投递给自己的 webview。

EmbeddedApp 把它当作宿主把预览权交还回来,而不是一次投票:它把请求从 openPermissionDiffsRef 中移除,不再传 hostOwnsEditDiffPreview,并且不会把用户刚关掉的标签页重新打开。这一行随即解锁,web shell 内联渲染 diff,用户可以在看得见的东西上做出批准决定。宿主的所有权会在下一次权限请求时恢复,或者在待处理 diff 被销毁时恢复。

这是 issue 里方向 1 和方向 2 的合并实现,且不需要引入任何新的可定制面:hostOwnsEditDiffPreview 本来就是布尔值,而且同一时刻只会有一个待处理权限。

两条刻意不触发它的路径

两条都有测试钉住,因为它们任意一条被触发都比原来的 bug 更糟:

  • qwen.diff.accept / qwen.diff.cancel 对绑定了请求的 diff 永远不会走到 cancelDiff——它们被 !permissionRequestId 拦住,改为通过 respondToPendingPermission 走投票路径。
  • closeDiffEditor 会在关闭标签页之前先删掉 map 里的条目,因此由 web shell 主动发起 closeDiff 之后紧跟着的 onDidCloseTextDocumentcancelDiff 这一跳找不到任何东西。少了这一步,web shell 自己要求的关闭就会被回声成一次「宿主撤销」,diff 会陷入反复重开的循环。

为什么需要它

审批流程把用户需要判断的那个对象弄丢了。宿主一旦打开 diff,hostOwnsEditDiffPreview 就让 web shell 不再渲染自己的 diff,于是那个原生标签页成了唯一能读到改动内容的地方。关掉这个标签页是完全正常的用户操作——但没有任何机制把这件事告诉 web shell,所以工具行仍然锁定、待处理的审批仍然存活。用户被留在一个必须投票却无从查看投票对象的处境里:要么盲批,要么盲拒,要么放弃这一轮。

同一个 PR 里还修掉了两个连带缺口,因为它们都是在为上面这个问题补测试见证时发现的:

审阅者测试计划

如何验证

  1. 在 VS Code companion 里发起一个需要批准的编辑。宿主打开 diff,工具行锁定。
  2. 不投票,直接用 ✕ 关掉 diff 标签页。预期: 该行解锁并内联展示 diff;标签页不会自己弹回来。
  3. 从该行里批准或拒绝。预期: 投票正常生效,与在 diff 编辑器里投票完全一致。
  4. 改为在 diff 编辑器里批准(qwen.diff.accept)。预期: 行为不变——不解锁、不重开。
  5. 再触发第二个权限请求。预期: 宿主再次打开自己的 diff,该行再次锁定。

本地实际执行与未执行的部分:

  • 已运行: packages/vscode-ide-companionsrc/diff-manager.test.ts 18 passedsrc/webview/EmbeddedApp.test.tsx 31 passedsrc/commands/index.test.ts 10 passedpackages/web-shellclient/App.test.tsx 746 passed(跑了整个文件,因为 Restore VS Code message edit and rewind after the WebShell cutover #9911 的修复动到了一条共享的提交路径)、client/components/messages/ToolGroup.test.tsx 106 passed
  • 未运行: src/extension.test.ts——本机上 @qwen-code/qwen-code-core 没有构建产物,该文件无法被收集。对 extension.ts 的改动是在既有 context.subscriptions.push(...) 列表里新增 7 行订阅,订阅的事件在真实的 DiffManager 上确实存在(该文件的测试用的是真实类,只对 hasDiff / getPermissionRequestId 打桩),但激活路径需要由 CI 确认。
  • 未运行: 任何构建、类型检查,以及套件的其余部分。

证据(改动前后)

尚缺——本 PR 没有任何可视化录证,下面这些自动化见证不能替代它。 第 2 步和第 5 步是宿主侧行为(原生 diff 标签页保持关闭、随后在下一次请求时重新打开),需要一次 VS Code Extension Development Host 会话才能录制;这个会话没有跑过。这里如实写明,而不是用通过数糊过去。

套件在单元层面确实钉住的内容:

  • 改动前: 关闭一个绑定请求的 diff 后,条目仍留在 openPermissionDiffsRef 中,hostOwnsEditDiffPreview 仍被断言为真,于是 ToolApproval 不渲染任何 diff,该行保持锁定。改动后: diff-manager.test.ts(18 passed)见证 onDidClosePermissionDiff 只对带 permissionRequestId 的 diff 触发;EmbeddedApp.test.tsx(31 passed)见证该请求被移除、该标志不再传递、标签页没有被重开。
  • 「两条刻意不触发它的路径」各自都有一条测试,去掉守卫就会失败。
  • ToolGroup.test.tsx(106 passed)见证宿主交还预览权之后该行解锁并内联渲染 diff。
  • Restore VS Code message edit and rewind after the WebShell cutover #9911App.test.tsx(746 passed)见证预检被拒时 toast 会触发,其中包含会话切换这一例,钉住了 prepareSubmit 里在 await 之前捕获的 sessionId

测试环境

OS 状态
🍏 macOS ⚠️
🪟 Windows ⚠️
🐧 Linux

✅ 已测试 · ⚠️ 未测试 · N/A

Linux 是上面那些单元测试套件实际执行的系统。macOS 与 Windows 本地都没有测试,而且在当前这个 commit 上,CI 里的 Test (macos-latest, Node 22.x)Test (windows-latest, Node 22.x) 两条 lane 都被 skipped,所以它们在 CI 上也没有跑。这一点对一个要发往三个平台的 VS Code companion 改动很重要;而且本轮再次重要,因为第 2 轮的 Critical R2-1 正是一个只有 Windows 才会暴露、任何 Linux lane 都抓不到的测试失败——平台确认在合并前仍然欠着,需要由 CI 补上。

环境(可选)

N/A——只跑了单元测试。没有 Extension Development Host 会话,没有 Docker/Podman 沙箱,没有本地 daemon 运行时。

风险与范围

  • 主要风险或取舍:hostOwnsEditDiffPreview 在 companion 里从恒为 true 的常量变成了有状态的量。如果它在待处理权限结束后被留在 false,后续请求就会内联渲染而不是走原生 diff——因此在「请求变化」路径和 closeOpenPermissionDiffs 上都做了复位,并且各配了一条测试。用户可见的行为变化是:关掉权限 diff 不再等于丢掉这次编辑;从 diff 编辑器投票的一切行为都不变。
  • 未验证 / 超出范围:本地没有跑构建和类型检查;src/extension.test.ts 本地未被收集(缺 core / acp-bridge 构建产物),因此激活分发路径及其 R3-7 / R3-8 见证需要 CI 确认;本地与 CI 在当前 commit 上都没有 macOS / Windows 执行记录;没有宿主侧 diff 标签页的改动前后录证。同样超出范围:同一轮 review 里提到的相关单槽 onExit 问题,已在 main 上修好(见 vscode-ide-companion: superseded daemon child still fires onExit, showing a false crash banner #10378);以及 R3-14,已核实是真实观察但刻意不修,因为它是一个记录在 issue 上的行为决策。至于 Restore VS Code message edit and rewind after the WebShell cutover #9911 在这个交互已不算新代码之后是否仍然需要 Extension Development Host 截图,留给维护者决定。
  • 破坏性变更 / 迁移说明:无。没有改动任何公共 API、配置项或持久化格式;onDidClosePermissionDiff 是在 DiffManager 上的新增项,而 composer.editUnavailable / composer.editExpired 这两条文案本来就存在,只是终于有了消费者。

本 PR 还包含

#10585——全部十二条测试见证缺口。 两个 commit:companion 那一组(commands/index.test.tsdiff-manager.test.tswebview/EmbeddedApp.test.tsxextension.test.ts)和 web-shell 那一组(App.test.tsxToolGroup.test.tsx)。其中十条在本机跑过;R3-7 与 R3-8 位于 extension.test.ts,该文件会引入 @qwen-code/qwen-code-core@qwen-code/acp-bridge——本机两者都没有构建产物——所以这两条需要 CI 确认。R3-14 已核实为真实观察但刻意未修;它是一个行为决策,已记录在 issue 上。

#9911——审计后仍然成立的两项。 该 issue 的前提已经过时:按消息粒度的编辑/rewind 随 cutover 一起上线了,由 daemon 通过 getRewindSnapshots / rewindSession 支撑,所以它当初要设计的那份 ACP 契约从来就不需要。真正缺失的是:

  • 预检被拒时是静默的。 两条提交路径都只用一句 console.warn 取消提示,别无其他。companion 在这里抛出 composer.editUnavailablecomposer.editExpired——中英文都已完整本地化——而仓库里没有任何其他地方消费它们。它们是写给用户看的,却永远到不了用户眼前。一次失败的 rewind 看起来就像这次编辑什么都没做,而 composer 仍停在编辑态。
  • 这个流程完全没有测试。 getRewindSnapshotsrewindSessionEmbeddedApp.test.tsx 里只以 mock 桩的形式出现过。

toast 只在用户仍停留在该次提交所属会话时才触发,而这个守卫刻意比既有的 submissionOwnerIsCurrent 更窄——完整守卫跟踪的是 composer 身份,而提交动作本身就会改变它,所以复用它会恰好压掉用户最需要看到的那条消息。第一版确实复用了它;是新加的测试发现 toast 从来没触发过。

#9911 上还剩的事项:在这个交互已不算新代码之后,是否仍然需要 Extension Development Host 截图。审计点名的三个 rewind 场景现在都有见证:选定轮次那一例、快照过期那一例,以及会话切换那一例(在 getRewindSnapshotsrewindSession 之间切换会话),后者钉住了 prepareSubmit 中在 await 之前捕获的 sessionId

关联 Issue

Fixes #10557
Refs #10585
Refs #9911

承接 #9811 WebShell cutover 的后续工作。

Closing a host-owned permission diff by hand left the user having to approve
or reject an edit they could no longer look at.

`onDidCloseTextDocument` routes the close to `DiffManager.cancelDiff`, which
fires `ide/diffClosed` on `onDidChange` — and the only consumer of that is
`ide-server.ts`, i.e. IDE-mode MCP transports. The web shell that asked for the
diff is not one, so it never learned. `EmbeddedApp` kept the request in
`openPermissionDiffsRef`, so `updateTranscript` never re-posted `openDiff`, and
`ToolGroup` kept the row locked because `hostOwnsEditDiffPreview` says the host
owns the preview.

DiffManager now also fires a typed `onDidClosePermissionDiff` when the closed
diff had a `permissionRequestId`. `extension.ts` fans it out to the
permission-aware providers, the same registry the `qwen.diff.accept` and
`qwen.diff.cancel` commands already use, and `WebViewProvider` posts it to its
webview.

`EmbeddedApp` treats that as the host handing the preview back rather than as a
vote: it drops the request from `openPermissionDiffsRef`, stops passing
`hostOwnsEditDiffPreview`, and does not reopen the tab the user just closed. The
row unlocks and the web shell renders the diff inline, so the edit is visible
again and the approval can be answered. Ownership returns to the host on the
next permission request, or when the pending diffs are torn down.

Two paths deliberately do not trigger it: `qwen.diff.accept` / `qwen.diff.cancel`
never reach `cancelDiff` for a request-bound diff (they route the vote through
`respondToPendingPermission`), and `closeDiffEditor` drops the map entry before
the tab closes, so a close the web shell itself asked for does not echo back as
a dismissal. Both are pinned by tests.

Run here: `diff-manager.test.ts` 8 passed, `webview/EmbeddedApp.test.tsx` 21
passed. `extension.test.ts` could not run on this machine — `@qwen-code/qwen-code-core`
has no build here — so the activation-time subscription is CI's to confirm. No
build or typecheck was run.

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

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

@yiliang114 the change reads as tightly scoped — 6 files, ~79 production lines against 192 test lines, all inside the VS Code companion — and #10557 documents the gap end to end. But the PR body doesn't follow the PR template, so I have to stop here and ask for a restructure first.

Missing:

  • ## Why it's needed — the motivation is currently folded into ## What this PR does (the "The gap, end to end" bullets). It already reads well, it's just in the wrong section; pull it out.
  • ### How to verify — your numbered 5-step plan under ## Reviewer Test Plan is exactly what this subsection asks for, it just has no heading. ### What was and was not run here can stay as an extra subsection.
  • ### Evidence (Before & After) — this is the substantive gap. Closing the diff tab and watching the row unlock and render the diff inline is user-visible behaviour in the companion, so the template wants before-and-after evidence here (screenshots or a short recording), not N/A. Pass counts from diff-manager.test.ts and EmbeddedApp.test.tsx don't show the tab staying closed or the row actually unlocking.
  • ### Tested on — the macOS / Windows / Linux table. Worth filling in for an extension change; the body says which suites ran but not where they ran.
  • the <details><summary>中文说明</summary> block with the Chinese translation of the body.

## Risk & Scope and ## Linked Issues are both present and substantive — no change needed there. ### Environment (optional) is genuinely optional.

To be clear about what this is: a process gate, not a judgment on your fix. The substantive code review happens once the body is complete — add the missing sections and re-run @qwen-code /triage.

中文说明

@yiliang114 改动本身范围很小(6 个文件,约 79 行生产代码对 192 行测试代码,全部在 VS Code companion 内),#10557 也把这个缺口从头到尾描述清楚了。但 PR 描述没有遵循本仓库的 PR 模板,所以我只能先停在这里,请你调整结构。

缺少的部分:

  • ## Why it's needed —— 动机目前写在 ## What this PR does 里("The gap, end to end" 那几条)。内容本身写得不错,只是放错了小节,拆出来即可。
  • ### How to verify —— 你在 ## Reviewer Test Plan 下的 5 步编号清单正是这个小节要的内容,只是没有标题。### What was and was not run here 可以作为额外小节保留。
  • ### Evidence (Before & After) —— 这是真正缺的一块。关闭 diff 标签页后工具行解锁、内联渲染 diff,属于 companion 里用户可见的行为,模板要求这里给出 before/after 证据(截图或短录屏),不能写 N/Adiff-manager.test.tsEmbeddedApp.test.tsx 的通过数量无法证明"标签页没有重新弹开、工具行确实解锁了"。
  • ### Tested on —— macOS / Windows / Linux 表格。扩展类改动值得填一下;目前描述只说了跑了哪些测试,没说在哪个系统上跑的。
  • <details><summary>中文说明</summary> 中文翻译段落。

## Risk & Scope## Linked Issues 都在,内容也充实,不需要改动。### Environment (optional) 确实是可选的。

需要说明的是:这是流程性拦截,不是对你这个修复本身的判断。等描述补全后才会进行实质性的代码审查 —— 补齐缺失部分后重新运行 @qwen-code /triage 即可。

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

Nine of the twelve test-witness gaps in #10585, all in the companion. No
production code changes.

- R3-2 / R3-3 (commands/index.test.ts): both diff command hops exercised with
  a DEFINED permissionRequestId. Every existing case left it undefined, so the
  binding that ties a diff to one approval was never crossed.
- R3-9 (diff-manager.test.ts): all three halves of that binding — stored by
  showDiff, read back by getPermissionRequestId/hasDiff, and used by closeDiff
  to refuse a diff a different approval owns.
- R3-4 / R3-5 / R3-6 / R3-16 (webview/EmbeddedApp.test.tsx): the cleanup loop's
  requestId-scoped closeDiff; the host-side gate refusing a decision bound to
  another request; the 'reject' half of the decision vocabulary; and the count
  assertion for opening a native diff only for the FIRST pending permission.
- R3-7 / R3-8 (extension.test.ts): the !permissionRequestId guard on both vote
  commands — a request-bound diff must reach the approval owner rather than
  acceptDiff/cancelDiff, and an unbound one must still resolve locally.

Run here: commands/index.test.ts 10 passed, diff-manager.test.ts 13 passed,
webview/EmbeddedApp.test.tsx 25 passed. extension.test.ts could NOT be run on
this machine — it pulls @qwen-code/qwen-code-core and @qwen-code/acp-bridge,
neither of which has a build here — so R3-7 and R3-8 are CI's to confirm.

R3-11, R3-13 and R3-15 are in web-shell and are not in this commit. The three
behavioural observations (R3-10, R3-12, R3-14) are untouched: #10585 says to
verify each before writing a fix, and none has been verified.

Refs #10585
The last three test-witness gaps in #10585, closing the set.

- R3-15 (App.test.tsx): the exact-request-id gate in respondToPendingPermission
  was only ever crossed with a matching id. A stale native diff left over from
  an approval that already moved on must not resolve the current request; the
  test votes with a wrong id, asserts the refusal, then votes with the right one
  to show the approval is still live.
- R3-11 (ToolGroup.test.tsx): a nested edit approval keeps the sub-agent row
  open, so the edit the user is asked to approve stays visible. Paired with a
  control that the same agent with nothing pending stays collapsed, so the
  assertion is about the approval rather than about agent rows always rendering
  their sub-tools.
- R3-13 (EmbeddedApp.test.tsx): the host file-open hand-off, which had zero
  coverage in either package, routed to the extension as an openFile message.

Run here: App.test.tsx (filtered) 1 passed, ToolGroup.test.tsx 106 passed,
EmbeddedApp.test.tsx 26 passed.

R3-14 verified while writing R3-13 and NOT fixed here: the onWorkspaceFileOpen
hand-off in App.tsx does early-return before the pre-existing stat() guard, so
directories and missing files reach the host path. That is a behaviour change to
decide, not a witness gap; recorded on #10585.

Refs #10585
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 6, 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 6e73fd1. 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 2 render-shaping files:

  • packages/web-shell/client/App.tsx
  • packages/web-shell/client/components/messages/ToolGroup.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

Two hardening fixes on the notification this PR adds, both matching what the
code immediately next to it already does.

The webview handler accepted `permissionDiffClosed` from any window. MCP apps
and artifact previews run in scriptable sandboxed iframes inside this webview
and can postMessage into it — which is exactly why the decision handler two
branches below checks `event.source === window.parent`, with two tests pinning
it. Handing the edit preview back is not a vote, so this is not an escalation,
but it is a spoofable state flip and it should not be reachable from a sandboxed
frame. Same gate, and a test that dispatches from a real nested iframe and from
a sourceless event.

The extension-side fan-out ran unguarded inside a VS Code event handler, so one
disposed or half-torn-down surface would take down the emitter and with it every
other surface's notification. The two vote commands above it already wrap their
fan-out in try/catch with a warn; this now matches.

Run here: webview/EmbeddedApp.test.tsx 27 passed.
Closes the two items #9911 still owns after the audit. Its premise is stale —
per-message edit/rewind shipped with the cutover, daemon-backed via
getRewindSnapshots/rewindSession — but its verification gate never ran and its
failure path was silent.

**A rejected preflight is no longer silent.** Both submit paths cancelled the
prompt with nothing but a console.warn. Hosts put user-facing text in these
errors: the companion's rewind throws composer.editUnavailable and
composer.editExpired, both fully localized in English and Chinese, and nothing
in the repository consumed them other than the throws. They were written to be
read by a user and could never reach one — a rewind that failed because the
snapshot aged out looked to the user like the edit did nothing, with the
composer still in editing mode and no explanation.

The toast fires only while the user is still on the session the submission
belonged to. That guard is deliberately narrower than submissionOwnerIsCurrent:
the full guard also tracks composer identity, and submitting is itself what
moves that, so reusing it would suppress the very message the user needs. The
first draft of this change did reuse it, and the new test caught that the toast
never fired.

**The rewind flow now has tests.** getRewindSnapshots and rewindSession
previously appeared in EmbeddedApp.test.tsx only as mock stubs, with no case
calling them. Added: a rewind that resolves the snapshot for the edited turn
rather than the newest one (the fixture lists turns out of order, so a
max/last-element bug fails), and the expired-snapshot rejection asserting no
rewind is attempted. A third case was written and then dropped because it passed
for the wrong reason — it exercised the expired path under a name about session
readiness.

Run here: web-shell App.test.tsx 746 passed (full file, since this changes a
shared submit path), vscode-ide-companion webview/EmbeddedApp.test.tsx 29 passed.

Refs #9911

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI and their suites did not run locally; Agent 7 built and tested on Linux only, so no verification of this VS Code companion change ran on the two other platforms it ships to, which matters because the path-normalization concern in R1-3 is Windows-specific.

Test Plan (not a blocker): src/diff-manager.test.tsno such file or directory; src/webview/EmbeddedApp.test.tsxno such file or directory; src/commands/index.test.tsno such file or directory; client/App.test.tsxno such file or directory; client/components/messages/ToolGroup.test.tsxno such file or directory; and 6 more.

[Critical] Still-standing blocker re-checked from the project's own triage gate (review 5124755540, CHANGES_REQUESTED, 2026-09-06) — this is a ruling on an existing live review, NOT a new code defect found by this review, and it is about the pull request description rather than the diff. That review stops the pull request at a process gate: "the PR body doesn't follow the PR template, so I have to stop here and ask for a restructure first", and "The substantive code review happens once the body is complete". Checked against the description as it stands at bfce659, all five sections it named as missing are still absent: ## Why it's needed (the motivation is still folded into ## What this PR does), ### How to verify (the numbered 5-step plan is still under ## Reviewer Test Plan with no heading of its own), ### Evidence (Before & After) (still no screenshot or recording of the diff tab staying closed and the tool row unlocking — the pass counts under ### What was and was not run here do not show that), ### Tested on (still no macOS / Windows / Linux table, which matters for an extension change), and the <details><summary>中文说明</summary> Chinese-translation block. ## Risk & Scope and ## Linked Issues are both present and substantive, and ### Environment (optional) is genuinely optional, so nothing else is outstanding. To clear it: restructure the description into the template and re-run @qwen-code /triage. The code findings in this review are all recommendations and are independent of this gate — none of them is a merge blocker on its own.

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

Comment thread packages/vscode-ide-companion/src/commands/index.test.ts
Comment thread packages/vscode-ide-companion/src/diff-manager.ts
Comment thread packages/vscode-ide-companion/src/diff-manager.ts Outdated
Comment thread packages/vscode-ide-companion/src/extension.ts Outdated
Comment thread packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx
Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/components/messages/ToolGroup.test.tsx
Comment thread packages/web-shell/client/components/messages/ToolGroup.test.tsx
Comment thread packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx
…rite

The dismissal chain added for #10557 had three unpinned hops, and the web
shell's two edit-name tables disagreed about one alias:

- shouldAutoExpand listed write_file/writefile/edit/editfile while
  isEditToolName also matches bare `write`. Now that EmbeddedApp hands
  hostOwnsEditDiffPreview back as state, a pending `write` approval
  unlocked without ever auto-expanding, so the edit the user was asked to
  approve stayed off screen. Reuse isEditToolName so the two sets cannot
  drift apart again.
- notifyPermissionDiffClosed re-implemented sendMessageToWebView instead
  of calling it; route it through the funnel so any cross-cutting
  behaviour added there covers dismissals too.
- Add witnesses for the posted payload key (requestId), which the webview
  receiver is the only thing that cares about, and for the emitter
  teardown in DiffManager.dispose().

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtq0vr74ni

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

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

  • R1-1 inert new test file (packages/vscode-ide-companion/src/commands/index.test.ts:261) — already reported (comment 3944217715)
  • R1-3 unread filePath on the new event payload (packages/vscode-ide-companion/src/diff-manager.ts:479) — already reported (comment 3944217721)
  • R1-4 unwitnessed dismissal fan-out subscription (packages/vscode-ide-companion/src/extension.ts:251) — already reported (comment 3944217722)
  • R1-5 duplicated permission-block fixture (packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1066) — already reported (comment 3944217727)
  • R1-6 rewind flow cases partly landed (packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1198) — already reported (comment 3944217729)
  • R1-7 unpinned hostOwnsEditDiffPreview resets (packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx:506) — already reported (comment 3944217732)
  • R1-8 untested openPermissionDiffsRef cleanup (packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx:708) — already reported (comment 3944217733)
  • R1-10 silent drop points on the dismissal chain (packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts:2394) — already reported (comment 3944217736)
  • R1-12 web-shell half of the R3-13 witness missing (packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1108) — already reported (comment 3944217742)
  • R1-13 hand-rolled preflight toasts (packages/web-shell/client/App.tsx:9246) — already reported (comment 3944217744)
  • R1-14 predicate duplicating an existing guard's conjuncts (packages/web-shell/client/App.tsx:9973) — already reported (comment 3944217746)
  • R1-15 queued-submit preflight catch unwitnessed (packages/web-shell/client/App.tsx:10033) — already reported (comment 3944217749)
  • R1-16 ParallelAgentsGroup conditional untested (packages/web-shell/client/components/messages/ToolGroup.test.tsx:2743) — already reported (comment 3944217755)
  • R1-17 control render passes the same flag value as the case render (packages/web-shell/client/components/messages/ToolGroup.test.tsx:2777) — already reported (comment 3944217759)

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI at this commit and their suites did not run locally; Agent 7 built and tested on Linux only, which matters because R2-1 is a Windows-only test failure that no lane run here could have caught.

Test Plan (not a blocker): src/diff-manager.test.tsno such file or directory; src/webview/EmbeddedApp.test.tsxno such file or directory; src/commands/index.test.tsno such file or directory; client/App.test.tsxno such file or directory; client/components/messages/ToolGroup.test.tsxno such file or directory; and 6 more.

[Critical] R1-19 Still-standing blocker re-checked from the project's own triage gate (review 5124755540, CHANGES_REQUESTED, 2026-09-06). This is a ruling on an existing live review, not a new code defect found by this round, and it is about the pull request description rather than the diff. That review stops the pull request at a process gate: "the PR body doesn't follow the PR template, so I have to stop here and ask for a restructure first", and "The substantive code review happens once the body is complete". Checked against the description as it stands at dea09dc, all five sections it named as missing are still absent: ## Why it's needed (the motivation is still folded into ## What this PR does), ### How to verify (the numbered 5-step plan is still under ## Reviewer Test Plan with no heading of its own), ### Evidence (Before & After) (still no screenshot or recording of the diff tab staying closed and the tool row unlocking — the pass counts under ### What was and was not run here do not show that, and the web-shell visual preview comment renders mock-daemon screenshots of the web shell rather than the companion interaction), ### Tested on (still no macOS / Windows / Linux table, which matters for an extension change and matters more this round because R2-1 is a Windows-only test failure that no lane run here could catch), and the <details><summary>中文说明</summary> Chinese-translation block. ## Risk & Scope and ## Linked Issues are both present and substantive, and ### Environment (optional) is genuinely optional, so nothing else is outstanding. To clear it: restructure the description into the template and re-run @qwen-code /triage. The code findings in this review are all independent of this gate.

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

Comment thread packages/vscode-ide-companion/src/diff-manager.test.ts Outdated
Comment thread packages/web-shell/client/components/messages/ToolGroup.test.tsx
yiliang114 and others added 4 commits September 7, 2026 08:54
The payload carries the normalized path showDiff stored, not the callers
argument, so pinning a raw POSIX literal made the assertion red on Windows
(path.normalize turns /workspace/foo.ts into a backslash form) while the
pull-request lane never showed it. Verified both ways with a win32 probe:
the old form fails on the emitted filePath, the derived form passes under
win32 and posix (14 tests).

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtqfvvmto2
The fixture carried no content, so extractDiff returned an empty string and
the expanded card rendered nothing: both assertions passed with no edit on
screen. Dropping the bare write alias from the detail renderer kept all 107
tests in the file green; with the fixture content and the new assertion that
mutant is caught (1 failed | 106 passed).

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtqfvvmto2
…event

`onDidClosePermissionDiff` carried `filePath` alongside the request id, and
the only consumer — the fan-out in extension.ts — destructures just
`permissionRequestId`. The field had no reader anywhere in the repo, and a
deep-equality test froze it, so it looked load-bearing while being dead
weight in a different path space from every consumer.

Narrow the payload to the id the fan-out actually needs and keep the
assertion deep-equal, so a field re-added without a reader fails the test.
Drops the now-unused node:path import.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtqqlolgof
…n base

`submissionSessionIsCurrent` restated the first four conjuncts of
`submissionOwnerIsCurrent` verbatim five lines below it, so the two could
drift apart while looking independent. Build the wider guard on the narrow
base instead.

Every operand is a pure ref read (`getComposerWorkspaceCwd` only reads
`pendingSessionContextRef`/`connectionRef`/`workspacesRef`), so evaluating
the shared four before the write-block and composer-version checks returns
the identical boolean for every state. No behaviour change.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtqqlolgof
@yiliang114

Copy link
Copy Markdown
Collaborator Author

CI attribution for the three red checks at 006ced2492

None of the three is caused by this PR's diff. Evidence per job:

1. Lint & Static (ubuntu-latest, Node 22.x) — job 101589157932, conclusion=failure

The 16m35s runtime is not the cancellation shape, so I pulled the log. The only error in it is:

npm error Missing script: "check:core-subpath-exports"
##[error]Process completed with exit code 1.

The step runs npm run build --workspace=packages/core and npm run check:core-subpath-exports. This is a stale-branch failure, not a code defect:

  • merge-base is b423df8f8f; check:core-subpath-exports is absent from package.json at that merge-base
  • main added it afterwards in 703678136a perf(cli): import core modules directly instead of the package root (perf(cli): import core modules directly instead of the package root #10957), together with the CI step at .github/workflows/ci.yml:1224
  • this PR touches neither package.json nor .github/ (git diff --name-only origin/main...HEAD)
  • scripts/check-core-subpath-exports.mjs does not exist at the PR head
  • the branch is 14 commits behind main

So CI runs main's newer workflow (which calls the script) against this branch's older package.json (which lacks it). There is no lint rule violation, no TypeScript error and no formatting diff anywhere in the log, and nothing names a file from this PR's 13-file diff. Remedy is merging main into the branch — not a code change here.

2. Test (ubuntu-latest, Node 22.x) — job 101589157918, conclusion=cancelled

Ran 2h0m58s, i.e. the 2-hour job ceiling; --log-failed is empty, so no test failed — the job was killed at the wall. Same shape already established on #11080 and #11101.

3. web-shell E2E Smoke (ubuntu-latest, Node 22.x) — job 101607664394, conclusion=failure

This one is a real failure rather than cancelled, but the vitest stage passed:

✓ chat-transcript-document.test.ts (5 tests) 76988ms
 Test Files  1 passed (1)
      Tests  5 passed (5)

Playwright then could not start its webServer:

[WebServer] Error: EMFILE: too many open files, watch '.../packages/web-shell/package.json'
[WebServer] npm error Lifecycle script `dev` failed with error:
Error: Process from config.webServer was not able to start. Exit code: 1

EMFILE is file-descriptor exhaustion on the self-hosted runner (actions-runner-hk3-17), before any E2E spec ran. No test belonging to this PR's diff failed.

I am not blind-fixing any of these inside this PR.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

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

  • R1-4 unwitnessed dismissal fan-out subscription (packages/vscode-ide-companion/src/extension.ts:251) — already reported (comment 3944217722)
  • R1-6 rewind flow cases partly landed (packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1198) — already reported (comment 3944217729)
  • R1-7 unpinned hostOwnsEditDiffPreview resets (packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx:506) — already reported (comment 3944217732)
  • R1-8 untested openPermissionDiffsRef cleanup (packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx:708) — already reported (comment 3944217733)
  • R1-10 silent drop points on the dismissal chain (packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts:2394) — already reported (comment 3944217736)
  • R1-15 queued-submit preflight catch unwitnessed (packages/web-shell/client/App.tsx:10033) — already reported (comment 3944217749)
  • R1-17 control render passes the same flag value as the case render (packages/web-shell/client/components/messages/ToolGroup.test.tsx:2777) — already reported (comment 3944217759)

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI at this commit and their suites did not run locally; Agent 7 built and tested on Linux only, which matters because this is a VS Code companion change and the previous round's R2-1 was a Windows-only test failure no Linux lane could catch.

Not explored to full depth (tool budget reached): "agent 1a": verifying whether a legacy-ACP chat surface and a web-shell surface (or an IDE-mode MCP client and a web-shell approval) can be live simultaneously — the reacha…; "agent 1a": running the full packages/web-shell/client/App.test.tsx suite (746 tests) rather than the two new cases by name filter, so a regression elsewhere in that file….

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

Test Plan (not a blocker): src/diff-manager.test.tsno such file or directory; src/webview/EmbeddedApp.test.tsxno such file or directory; src/commands/index.test.tsno such file or directory; client/App.test.tsxno such file or directory; client/components/messages/ToolGroup.test.tsxno such file or directory; and 6 more.

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

  • packages/web-shell/client/components/messages/ToolGroup.test.tsx:2838 — [probe] Hand-back witness never flips the prop on a mounted row
  • packages/web-shell/client/App.test.tsx:25855 — [probe] The !hostOwnsEditDiffPreview conjunct has no witness

[Critical] R1-19 Still-standing blocker re-checked from the project's own triage gate (review 5124755540, CHANGES_REQUESTED, 2026-09-06). This is a ruling on an existing live review, not a new code defect found by this round, and it is about the pull request description rather than the diff. That review stops the pull request at a process gate: "the PR body doesn't follow the PR template, so I have to stop here and ask for a restructure first", and "The substantive code review happens once the body is complete". Checked against the description as it stands at 9d0df1c, all five sections it named as missing are still absent: ## Why it's needed (the motivation is still folded into ## What this PR does as "The gap, end to end"), ### How to verify (the numbered 5-step plan is still under ## Reviewer Test Plan with no heading of its own), ### Evidence (Before & After) (still no screenshot or recording of the diff tab staying closed and the tool row unlocking — the pass counts under ### What was and was not run here do not show that, and the web-shell visual preview comment renders mock-daemon screenshots of the web shell rather than the companion interaction), ### Tested on (still no macOS / Windows / Linux table, which matters for an extension change and matters again this round because the windows-latest and macos-latest test lanes were skipped in CI at this commit and their suites did not run locally either), and the <details><summary>中文说明</summary> Chinese-translation block. ## Risk & Scope and ## Linked Issues are both present and substantive, and ### Environment (optional) is genuinely optional, so nothing else is outstanding. To clear it: restructure the description into the template and re-run @qwen-code /triage. The code findings in this review are all independent of this gate.

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

yiliang114 and others added 2 commits September 8, 2026 00:07
The Lint & Static job failed at "Check core subpath exports resolve" with
`npm error Missing script: "check:core-subpath-exports"`: main added that
script and its CI step in 7036781 (#10957) after this branch's last
merge, and CI checks out refs/pull/<n>/head, so the step ran against a
package.json that predates it. Merging main brings both the script and
scripts/check-core-subpath-exports.mjs.

The gate scans packages/{cli,acp-bridge,sdk-typescript}/src for
`@qwen-code/qwen-code-core/<subpath>` specifiers; this branch touches only
vscode-ide-companion and web-shell and imports the core package root, so
it contributes no specifiers to the checked set.

Merge is clean (no conflicts). Verified locally: the PR's per-file numstat
against main is identical before and after the merge, so nothing was lost
across the 37-commit gap. Tests not run: no dependency tree on this
machine matches this lockfile.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-conflict/jmtrcr08gpe
With the missing `check:core-subpath-exports` script resolved by the
previous merge, the Lint & Static job now reaches Run Prettier and fails
there on this branch's own file:

  [warn] packages/web-shell/client/components/messages/ToolGroup.test.tsx
  [warn] Code style issues found in 1 file.

The fixture line is 82 columns against printWidth 80, so prettier splits
the object literal. Formatting only — no fixture value changes. Verified
with prettier 3.6.1, the version this branch's lockfile pins.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-conflict/jmtrcr08gpe

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

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

  • R1-1 inert new command-forwarding tests (packages/vscode-ide-companion/src/commands/index.test.ts:261) — already reported (comment 3944217715)
  • R1-4 unwitnessed dismissal fan-out subscription (packages/vscode-ide-companion/src/extension.ts:251) — already reported (comment 3944217722)
  • R1-5 duplicated permission-block fixture (packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1155) — already reported (comment 3944217727)
  • R1-7 unpinned hostOwnsEditDiffPreview resets (packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx:512) — already reported (comment 3944217732)
  • R1-8 untested openPermissionDiffsRef cleanup (packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx:714) — already reported (comment 3944217733)
  • R1-10 silent drop points on the dismissal chain (packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts:2463) — already reported (comment 3944217736)
  • R1-13 hand-rolled preflight toasts bypassing reportError (packages/web-shell/client/App.tsx:9288) — already reported (comment 3944217744)
  • R1-15 queued-submit preflight catch unwitnessed (packages/web-shell/client/App.tsx:10076) — already reported (comment 3944217749)
  • D3-1 hand-back witness never flips the prop on a mounted row (packages/web-shell/client/components/messages/ToolGroup.test.tsx:2840) — already reported (round 3 deferred list, review 5131595846)
  • D3-2 the !hostOwnsEditDiffPreview conjunct has no witness (packages/web-shell/client/App.test.tsx:25855) — already reported (round 3 deferred list, review 5131595846)

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI at this commit and their suites did not run locally; Agent 7 built and tested on Linux only, which matters because this is a VS Code companion change that ships to all three platforms.

Not explored to full depth (tool budget reached): "agent 1b": none — but two things I verified statically rather than by execution, for the record: I did not run packages/web-shell/client/App.test.tsx or packages/vscode….

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

Test Plan (not a blocker): src/diff-manager.test.tsno such file or directory; src/webview/EmbeddedApp.test.tsxno such file or directory; src/commands/index.test.tsno such file or directory; client/App.test.tsxno such file or directory; client/components/messages/ToolGroup.test.tsxno such file or directory; and 6 more.

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

  • packages/web-shell/client/App.tsx:10020 — [probe] The two preflight toast gates disagree on identical input and the comment's stated premise is disproved by measurement

[Critical] R1-19 Still-standing blocker re-checked from the project's own triage gate (review 5124755540, CHANGES_REQUESTED, 2026-09-06). This is a ruling on an existing live review, not a new code defect found by this round, and it is about the pull request description rather than the diff. That review stops the pull request at a process gate: "the PR body doesn't follow the PR template, so I have to stop here and ask for a restructure first", and "The substantive code review happens once the body is complete". Checked against the live description at head 211f0b7, line by line against .github/pull_request_template.md, all five sections it named as missing are still absent: ## Why it's needed (template line 11 — the motivation is still folded into ## What this PR does as "The gap, end to end"), ### How to verify (template line 24 — the numbered 5-step plan is still under ## Reviewer Test Plan with no heading of its own), ### Evidence (Before & After) (template line 28 — still no screenshot or recording of the diff tab staying closed and the tool row unlocking; the pass counts under ### What was and was not run here do not show that, and the web-shell visual preview comment renders mock-daemon screenshots of the web shell rather than the companion interaction), ### Tested on (template line 32 — still no macOS / Windows / Linux table, which matters for an extension change and matters again this round because the windows-latest and macos-latest test lanes were skipped in CI at this commit and their suites did not run locally either), and the <details><summary>中文说明</summary> block (template tail — "完整翻译上面的英文正文,逐段对应,不要省略或缩写"). ## What this PR does, ## Reviewer Test Plan, ## Risk & Scope and ## Linked Issues are all present and substantive, and ### Environment (optional) is genuinely optional, so nothing else is outstanding. To clear it: restructure the description into the template and re-run @qwen-code /triage. The code findings in this review are all independent of this gate. The fix rests on one premise worth naming: this gate is the triage review's own, not this review's invention, and that review states "The code findings in this review are all recommendations and are independent of this gate" — so clearing the code findings does not clear it, and clearing it does not dismiss them.

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

Comment thread packages/vscode-ide-companion/src/diff-manager.ts
closeDiff() matches by path alone when no request id is given, which is how
the IDE-mode MCP tool calls it. That close deleted a permission-bound entry
before the tab closed, so the onDidCloseTextDocument -> cancelDiff hop found
nothing and onDidClosePermissionDiff never fired: the web shell kept a locked
approval row for an edit the user could no longer see, the #10557 symptom
through a second door.

Fire the dismissal from closeDiff itself when the caller supplied no id but
the matched entry carries one. A caller that passed the id is the surface
holding the request and has already cleared its own state, so that close still
does not echo back.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtrow8ztpw

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

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

  • R1-4 unwitnessed dismissal fan-out subscription (packages/vscode-ide-companion/src/extension.ts:251) — already reported (comment 3944217722)
  • R1-10 no success-path log on the dismissal chain (packages/vscode-ide-companion/src/diff-manager.ts:407) — already reported (comment 3944217736, anchored at WebViewProvider.ts:2463)

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI at this commit and their suites did not run locally; Agent 7 built and tested on Linux only, which matters because this is a VS Code companion change that ships to all three platforms and the round's own findings are about test fidelity that no Linux lane can fully rule on.

Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": could not finish tracing whether a companion-attached web shell can ever receive a non-UUID permission requestId — packages/acp-bridge/src/bridgeClient.ts:952 ….

Test Plan (not a blocker): src/diff-manager.test.tsno such file or directory; src/webview/EmbeddedApp.test.tsxno such file or directory; src/commands/index.test.tsno such file or directory; client/App.test.tsxno such file or directory; client/components/messages/ToolGroup.test.tsxno such file or directory; and 6 more.

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

  • packages/vscode-ide-companion/src/diff-manager.ts:169 — [review] dispose() has no production caller, so the added emitter disposal never runs, and the new test comment attributes reload safety to the wrong mechanism
  • packages/vscode-ide-companion/src/diff-manager.ts:490 — [probe] A hand-close in the window after a native vote fires a dismissal for an already-decided request and re-exposes the vote affordance

Convergence: round 5 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 1 (1 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)

[Critical] R1-19 Still-standing blocker re-checked from the project's own triage gate (review 5124755540, CHANGES_REQUESTED, 2026-09-06). This is a ruling on an existing live review, not a new code defect found by this round, and it is about the pull request description rather than the diff. That review stops the pull request at a process gate: "the PR body doesn't follow the PR template, so I have to stop here and ask for a restructure first", and "The substantive code review happens once the body is complete". Checked line by line against .github/pull_request_template.md at the live description for head c04b22d, all five sections it named as missing are still absent: ## Why it's needed (the motivation is still folded into ## What this PR does as "The gap, end to end"), ### How to verify (the numbered 5-step plan is still under ## Reviewer Test Plan with no heading of its own), ### Evidence (Before & After) (still no screenshot or recording of the diff tab staying closed and the tool row unlocking — the pass counts under ### What was and was not run here do not show that, and the web-shell visual preview comment renders mock-daemon screenshots of the web shell rather than the companion interaction), ### Tested on (still no macOS / Windows / Linux table, which matters for an extension change and matters again this round because the windows-latest and macos-latest test lanes were skipped in CI at this commit and their suites did not run locally either), and the <details><summary>中文说明</summary> block (the template tail asks to translate the English body paragraph by paragraph without omission or abbreviation). ## What this PR does, ## Reviewer Test Plan, ## Risk & Scope and ## Linked Issues are all present and substantive, and ### Environment (optional) is genuinely optional, so nothing else is outstanding. To clear it: restructure the description into the template and re-run @qwen-code /triage. The code findings in this review are all independent of this gate. The fix rests on one premise worth naming: this gate is the triage review's own, not this review's invention, and that review states "The code findings in this review are all recommendations and are independent of this gate" — so clearing the code findings does not clear it, and clearing it does not dismiss them.

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

Comment thread packages/vscode-ide-companion/src/diff-manager.test.ts
Comment thread packages/vscode-ide-companion/src/diff-manager.test.ts
Four review findings from the last /review round, all of them gaps where a
production line this PR ships could be reverted with the suite still green:

- diff-manager.test.ts: the vscode.Uri mock rendered a `with()` copy with the
  original's scheme and query, so the left and the right side of one diff shared
  a single map key and every witness about which document a dismissal is keyed
  on was vacuous. `with()` now derives a new uri, `Uri.parse` exists for
  `closeAll()`, and a new case pins that the entry is keyed on the writable side.
- diff-manager.test.ts: both id-less dismissal cases passed
  `suppressNotification = true`, leaving the default arm -- the one
  `IdeClient.disconnect()` and the MCP closeDiff tool actually send -- unpinned.
- EmbeddedApp.test.tsx: the teardown half of the recovery path had no witness.
  One case now dismisses, tears the pending diffs down through the automatic
  approval mode, and asserts ownership returns to the host, no second closeDiff
  goes out for the tab the user already closed, and the same request can own a
  native diff again.

Mutation-checked locally: gating the dismissal fire on `suppressNotification`
fails 1/18, keying `addDiffDocument` on `leftDocUri` fails 5/18, dropping the
two reset lines in `closeOpenPermissionDiffs` fails 1/31, and dropping the
`openPermissionDiffsRef` delete in the dismissal handler fails 1/31.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmts86p0gqr

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

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

  • the rewind sessionId assertion cannot discriminate submission.sessionId from the runtime.sessionId fallback (packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1341) — already reported as R1-6 (comment 3944217729)
  • duplicated permission-block fixture (packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:999) — already reported as R1-5 (comment 3944217727), which the author declined with recorded reasons

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI and their suites did not run locally; Agent 7 built and tested on Linux only, which matters because this is a VS Code companion change that ships to all three platforms.

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

Test Plan (not a blocker): src/diff-manager.test.tsno such file or directory; src/webview/EmbeddedApp.test.tsxno such file or directory; src/commands/index.test.tsno such file or directory; client/App.test.tsxno such file or directory; client/components/messages/ToolGroup.test.tsxno such file or directory; and 6 more.

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

  • packages/vscode-ide-companion/src/diff-manager.test.ts:61 — [probe] The added Uri.parse mock member is unreachable and its…
  • packages/vscode-ide-companion/src/diff-manager.test.ts:99 — [probe] R5-1: (fix-induced) lastOpened*Uri() helpers return the…
  • packages/vscode-ide-companion/src/diff-manager.test.ts:354 — [probe] hasExistingDiff's request-id comparison has no test at all
  • packages/vscode-ide-companion/src/diff-manager.test.ts:444 — [probe] Nothing pins which same-path entry an id-less close selects
  • packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:46 — [probe] RewindSnapshotStub hand-copies an exported SDK type…
  • packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:55 — [probe] Hoisted rewind mocks lost their defaults and leak…
  • packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1024 — [probe] The typeof-requestId half of the new dismissal gate has no…
  • packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1085 — [probe] Request-id keying of the reopen suppression is never…
  • packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1102 — [probe] The ownership-recovery reset is unwitnessed for its…
  • packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1114 — [probe] The modeChanged else arm and the modeInfo fallback are…
  • packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1134 — [probe] The teardown witness dispatches modeChanged with no…
  • packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1173 — [probe] The forged-source witness misses the map delete behind the…
  • packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1334 — [probe] The newest-snapshot reduce arm of prepareSubmit is dead in…
  • packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1354 — [probe] The rewind witness cannot pin the clientId leg of the call

[Critical] R1-19 Still-standing blocker re-checked from the project's own triage gate (review 5124755540, CHANGES_REQUESTED, 2026-09-06). This is a ruling on an existing live review, not a new code defect found by this round, and it is about the pull request description rather than the diff. That review stops the pull request at a process gate: "the PR body doesn't follow the PR template, so I have to stop here and ask for a restructure first", and "The substantive code review happens once the body is complete". Checked heading by heading against .github/pull_request_template.md at the live description for head 637a3e9, the same five sections are still absent: ## Why it's needed (the motivation is still folded into ## What this PR does as "The gap, end to end"), ### How to verify (the numbered 5-step plan is still under ## Reviewer Test Plan with no heading of its own), ### Evidence (Before & After) (still no screenshot or recording of the diff tab staying closed and the tool row unlocking — the pass counts under ### What was and was not run here do not show that, and the web-shell visual preview comment renders mock-daemon screenshots of the web shell rather than the companion interaction), ### Tested on (still no macOS / Windows / Linux table, which matters for an extension change and matters again this round because the windows-latest and macos-latest test lanes did not run here either), and the <details><summary>中文说明</summary> block (the template tail asks to translate the English body paragraph by paragraph without omission or abbreviation). ## What this PR does, ## Reviewer Test Plan, ## Risk & Scope and ## Linked Issues are all present and substantive, and ### Environment (optional) is genuinely optional, so nothing else is outstanding. To clear it: restructure the description into the template and re-run @qwen-code /triage. The code findings in this review are all independent of this gate. The fix rests on one premise worth naming: this gate is the triage review's own, not this review's invention, and that review states "The code findings in this review are all recommendations and are independent of this gate" — so clearing the code findings does not clear it, and clearing it does not dismiss them. The gate is the triage review's own and is independent of the code findings — review 5125593364 states "The code findings in this review are all recommendations and are independent of this gate". Clearing the code findings does not clear it, and clearing it does not dismiss them. The triage gate itself: re-running @qwen-code /triage after the restructure must clear review 5124755540's process gate rather than stopping at it again.

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

…ast + chain logs

- extension.test.ts: drive the listener registered on onDidClosePermissionDiff
  and assert the fan-out reaches every permission-aware provider (the only
  product code connecting the two ends of the dismissal chain).
- App.test.tsx: witness the queued-submit half of the #9911 preflight toast,
  which the immediate-path case never exercised.
- extension.ts / WebViewProvider.ts: log the two silent drop points on the
  dismissal chain (no provider, no webview) so a "closed the tab, row stayed
  locked" field report can be triaged.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmttf1wj0so
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ❌ not passed — findings reported (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: 37 passed · 0 failed · 37 total

Flakiness gate: ✅ 7 changed test file(s) x 5 identical rounds, no divergence

中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)

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

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

抖动门:✅ 7 changed test file(s) x 5 identical rounds, no divergence

Verification report

Sandboxed verification: ❌ not passed — findings reported (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: 37 passed · 0 failed · 37 total

中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)· 后续轮

本轮为后续轮:上一轮验证于 926e9167,本轮验证于 8b1ebb20(新增 1 个 merge + 1 个测试 commit)。核心行为仍然正确且承重:A/B 三臂复测为 base+base 117 绿 / base+HEAD 测试 12 红 / head 149 绿(见 "A/B 承重证明" 与 01-ab-base-vs-head.png),17 条突变全部按预测落点。

  • 上一轮发现状态:F1(窄 guard 无测试钉住)仍成立——A1 突变 835/835 全绿;F2(用户 ✕ 触发链无见证)仍成立——X3 突变 149/149 全绿,且同文件正控 X4 被杀,证明 harness 有效;F3(fan-out try/catch 无钉住)仍成立;F4(shouldAutoExpand 放宽到裸 write仍成立且确为有意(T1 被杀)。两条对描述的更正(窄 guard 的真实机制、测试计数过期)在本轮复测后仍然成立
  • 新发现(本轮 delta):commit 19 的新测试确实钉住了 request-id 门(D1 被杀)与 answers 实参(D2 被杀,2 红);但同一门的另外两个合取项 !hostOwnsEditDiffPreviewrequest.hasDiffPreview !== true 无任何测试(D3/D4 均 835/835 存活;App.test.tsxhostOwnsEditDiffPreview: false 出现 0 次、hasDiffPreview 出现 0 次)。前者正是本 PR 从常量变为有状态的那个标志——hand-back 之后宿主中继投票应被拒绝,但没有任何测试见证这一点。属覆盖缺口,非缺陷。
  • 未覆盖:wire-handoff harness 本轮未复测(预算);F1/F2 的候选修复本轮未复测;真实 VS Code / TUI;web-shell playwright e2e;与当前 main 的 trial merge;逐 commit 归因(浅克隆)。
Verification report

PR #11171 — deep verification (follow-up round)

Verdict: findings — assertions 37 pass / 0 fail / 37 total. Verified head: 8b1ebb20766aea307d8761b7e28e3ab617f0bf52 (git rev-parse HEAD^2); base tip e09c461a9211b0069b83c359a8b832b1b3edad0d (HEAD^1). This is a follow-up to the round that verified 926e9167dd72d3f55e1bb356b2b61e3e8a137e69; the delta since then is commit 18 (a merge of origin/main) and commit 19 (test(web-shell): assert the answers argument submitPermission forwards). The central claim is re-proven load-bearing and correct at the new head, and every carried-forward measurement below was re-run against rebuilt arms — none was carried by hash shortcut, because both head and base moved. The findings are coverage gaps and description drift, not behavioural defects: no executed assertion failed.

Previous-finding status (re-measured at 8b1ebb20)

# finding severity status at new head evidence this round
F1 the narrow queued-toast guard (submissionSessionIsCurrent) is unpinned; the description's claim that a test caught its reuse is contradicted by measurement medium stands mutation A1 (swap in the full submissionOwnerIsCurrent) SURVIVED 835 passed (835), up from 799/799; sibling A2 on the immediate path also SURVIVED 835/835
F2 the user's ✕ (onDidCloseTextDocumentcancelDiff) is unwitnessed: deleting the trigger leaves every new test green medium stands mutation X3 (delete the subscription body) SURVIVED 149 passed (149); in-file positive control X4 (delete the fan-out loop) KILLED 1 failed | 148 passed, so the harness demonstrably can make extension.test.ts red
F3 the dismissal fan-out's try/catch is unpinned, and the shipped test wraps listener calls in its own try/catch low stands mutation X1 (rethrow) SURVIVED 149/149
F4 shouldAutoExpand now auto-expands a bare write tool; real, intentional, tested, absent from the description low stands mutation T1 (revert to the two base lines) KILLED by hands a pending bare-write row back to the shell already expanded (1 failed | 108 passed); isEditToolName at ToolGroup.tsx:153 matches edit|editfile|write|write_file|writefile
X2 empty-registry log + early return unpinned stands, correctly SURVIVED 149/149; observability-only (an empty for loop is the same behaviour)
W1 notifyPermissionDiffClosed's no-active-webview guard unpinned stands, correctly SURVIVED 149/149; in-file control W2 (rename the message type) KILLED, so the file is collected and the survivor is real
C1 correction: the description's mechanism for the narrow guard ("submitting moves composer identity") is wrong; the operative conjunct is the write-block term stands re-censused: the 6 composerSourceVersionRef.current += 1 writers are at App.tsx 3567, 12090, 12208, 12392, 12818, 16902 (line numbers shifted by the merge), all session/workspace-switch events, none on the submit path
C2 correction: test counts in the description are stale stands, wider at this head: companion 149 (5 files), App.test.tsx 835, ToolGroup.test.tsx 109 (web-shell total 944); the body still says 31 / 746 / 106
R3-14 declined by the author as a behaviour decision recorded on the issue declined-with-rationale; I agree unchanged; not re-litigated

The previous round's two measured candidate fixes (App.guard.test.tsx for F1, extension.trigger.test.ts for F2) were not re-applied this round; both findings stand on their own mutations, and the fixes remain candidates, not measurements, at this head. Recorded under Not covered.

Central claim and A/B load-bearing proof

Central claim. Closing a host-owned permission diff by hand (✕, no vote) hands the edit preview back to the web shell: the tool row unlocks, renders the diff inline, and the tab does not spring back open.

Secondary claims. (a) voting from the diff editor must not trigger the dismissal; (b) a web-shell-initiated closeDiff must not echo back as a dismissal; (c) the #9911 preflight-rejection toast fires only while the user is still on the submission's session.

All three arms were re-run live for the capture, not reprinted from saved logs:

arm code tests result oracle
ARM 0 (A/A control) base e09c461a base tests 117 passed (5 files) base tree healthy; the A/B is not an environment artifact
ARM 1 (control) base e09c461a HEAD tests 12 failed | 137 passed (149) the new behaviour is absent at base; failures name values (expected true to be false on hostOwnsEditDiffPreview, expected 0 to be greater than 0 on fan-out call count), not import errors
ARM 2 (head) head 8b1ebb20 HEAD tests 149 passed (5 files) the fix restores every flipped test

Witness: evidence/01-ab-base-vs-head.png — the three arms as printed, with the 12 flipped test names and both trees reported clean afterwards. Raw per-arm logs: raw/ab-arm0-base-base.log, raw/ab-arm1-base-headtests.log, raw/gate-companion-head.log.

The 12 flips are exactly the dismissal chain: 7 in diff-manager.test.ts, 3 in EmbeddedApp.test.tsx, 1 in extension.test.ts, 1 in WebViewProvider.test.ts — the same breakdown as the previous round.

Control hygiene. The base worktree (tmp/base-tree, removed after the cells were captured) had no node_modules, so @qwen-code/* would have resolved into the HEAD tree. @qwen-code/web-shell is changed by this PR, so it was re-pointed into the base tree and the realpath asserted (readlink -f…/tmp/base-tree/packages/web-shell, distinct from head's …/packages/web-shell). core/acp-bridge/sdk are untouched by the PR (no package.json/lockfile in the diff) and were left resolving to the head builds. The base tree's web-shell lib entry had to be built (vite.lib.config.ts, raw/build-base-webshell-lib.log) because the base EmbeddedApp.test.tsx resolves the package entry at transform time; the base app build fails in a bare worktree on a tailwind resolution error (raw/build-base-webshell.log), which is an environment fact about the worktree, not about the PR. Per-package node_modules were symlinked from the head tree — safe because the PR changes no dependency manifest, and neither contains @qwen-code links, so the base interception still wins (asserted). Base production files were asserted byte-identical to HEAD^1 by sha256 (diff-manager.ts, extension.ts, EmbeddedApp.tsx, WebViewProvider.ts, App.tsx, ToolGroup.tsx), and base dist/index.js sha256 differs from head's.

Sibling sweep (diff-manager.sibling.test.ts in this artifact dir, 6 probes appended to a copy of the shipped suite; file total 24 passed, 0 red): acceptDiff on a request-bound diff stays quiet and really drops the entry; closeAll() stays quiet, really drops the entry, and a later cancelDiff hop still finds nothing; with two diffs open an id-less close reports exactly the request that lost its diff, once, leaving the unowned neighbour untouched; an id-less close on the unowned path stays quiet; a repeated cancelDiff plus the id-less door on the same gone diff fires exactly once; a matching-id close on a diff no approval owns stays quiet. All green at head.

Destination check (re-measured). hostOwnsEditDiffPreview plumbs App.tsx:2947 (default false) → customization App.tsx:3218useWebShellCustomization() at ToolGroup.tsx:1163locksPendingEditApproval (1169/1217) → approval={hostOwnsEditDiffPreview ? approval : undefined} at ToolGroup.tsx:1284. Unchanged from the previous round.

Corrections to the description

  • C1 stands (see status table): the narrow toast guard's load-bearing conjunct is the write-block term, not composer identity. The refactor that composes submissionOwnerIsCurrent on submissionSessionIsCurrent (App.tsx:10247-10256) removes the drift risk the description worried about, but not the coverage gap F1 names.
  • C2 stands and widened: every count in the body's "Run:" list is stale at this head (companion 149 vs "18+31+10", App.test.tsx 835 vs 746, ToolGroup.test.tsx 109 vs 106). Expected drift across 19 commits; noted for the record.

Findings

F5 (new, medium-low) — the exact-request-id gate's two stateful conjuncts are unpinned, and one of them is the flag this PR makes stateful

Commit 19's new test pins the gate's id conjunct and the answers argument (see the delta section below). The same gate at App.tsx:12683-12688 has two further conjuncts that nothing exercises:

node tmp/mut-run.mjs … D3   # delete `!hostOwnsEditDiffPreview ||`        -> SURVIVED 835 passed (835)
node tmp/mut-run.mjs … D4   # delete `request.hasDiffPreview !== true`   -> SURVIVED 835 passed (835)

Census: hostOwnsEditDiffPreview: false occurs 0 times in App.test.tsx (2 occurrences of : true), and hasDiffPreview occurs 0 times in App.test.tsx. Both conjuncts are live in production — hasDiffPreview is produced by transcriptAdapter.ts:46, and hostOwnsEditDiffPreview is exactly the value this PR turns from a constant true into state — so these are coverage gaps, not dead clauses. The sharp case: after a hand-back the flag is false, and the gate then refuses a host-relayed vote (correct — the host's tab is gone and the user votes inline); if a future refactor dropped that conjunct, a stale native vote would resolve an approval the host can no longer display and all 835 tests would stay green. No test votes through respondToPendingPermission after a hand-back.

Suggested fix (candidate, not applied, not measured this round)

One test in App.test.tsx: render with a pending permission, post permissionDiffClosed to hand the preview back, then call shellApi.respondToPendingPermission('req-1', 'allow') and assert it resolves false with submitPermission not called; and a second case with a block whose adapter record has hasDiffPreview: false, asserting the same refusal. Per the vacuity rule this ships with its mutation (D3/D4 must go SURVIVED → KILLED); neither was applied here, so the fixture that would pin this axis is named but not yet written.

F1 (carried, medium) — the narrow queued-toast guard is still unpinned

Re-measured: A1 SURVIVED 835 passed (835). The shipped queued test sets streamingState='responding' but never write-blocks the session, so the full guard evaluates true in the test and the two guards are indistinguishable there. The previous round's measured fixture (mid-flight write-block) remains the fix candidate; not re-applied this round.

F2 (carried, medium) — the user's ✕ is still unwitnessed

Re-measured: X3 SURVIVED 149 passed (149). The whole dismissal chain hangs on the pre-existing onDidCloseTextDocumentcancelDiff subscription at extension.ts:242; every shipped test calls cancelDiff directly. The in-file positive control X4 (delete the fan-out loop) was KILLED (1 failed \| 148 passed, red = forwards a closed permission diff to every permission-aware provider), which is what makes the survivor credible: the harness can make this file red. Pre-existing code, so the author is not blamed — but this PR is what makes the subscription load-bearing.

F3 (carried, low) — the fan-out try/catch is still unpinned

X1 (rethrow) SURVIVED 149/149. Mirrors the pre-existing vote fan-outs; a pre-existing pattern, not a PR-introduced hazard.

F4 (carried, low) — the bare-write auto-expand widening is real and pinned, but absent from the description

T1 KILLED by hands a pending bare-write row back to the shell already expanded. Deliberate and load-bearing for the hand-back; still a user-visible change for every web-shell host that the description never mentions. Note, not a defect.

The delta since the last round (commit 19)

Commit 19 adds one assertion: expect(mockSessionActions.submitPermission).toHaveBeenCalledWith('req-1', 'proceed_once', undefined) inside refuses a native edit approval vote bound to a different request id. It is load-bearing and correctly attributed:

mutation change result red test(s)
D1 delete request.id !== requestId from the gate KILLED 1 failed | 834 passed refuses a native edit approval vote bound to a different request id (expected true to be false)
D2 host-relayed vote forwards an answers object (handleConfirm(id, option, {relayed:'host'})) KILLED 2 failed | 833 passed the commit-19 test and the pre-existing submits allow_once for a native structured edit approval accept; assertion expected "spy" to be called with arguments: [ 'req-1', 'proceed_once', undefined ]

D2's two reds are the point: the third argument is asserted as undefined by construction, so a host-relayed vote that ever started carrying answers would go red on both the new and the old test. (The matrix's killer-hit column prints NO for D2 only because that column string-matches the killer against a test name and my D2 killer string names the assertion; verified by hand from raw/mut-D2.log.)

Not covered

  • Wire-handoff harness not re-measured this round. The previous round's two-process harness (real WebViewProvider.notifyPermissionDiffClosed → JSON round-trip → real EmbeddedApp MessageEvent) was not rebuilt at this head; budget went to the delta mutations and the F5 census instead. The property it proved is still pinned transitively: W2 (rename the producer's message type) is KILLED by relays a permission diff dismissal to the webview under requestId, and M2/M3 pin the consumer's handling of exactly that payload shape. What is not re-proven this round is the structured-clone fidelity of the transport.
  • F1/F2 candidate fixes not re-applied at this head (see their rows).
  • Real VS Code Extension Development Host / real TUI. Every harness drives the shipped fake vscode boundary or a synthetic MessageEvent; steps 2 and 5 of the Reviewer Test Plan (native tab staying closed, reopening on the next request) remain host-side and unrecorded, as the author states.
  • Playwright e2e (test:e2e*) not run; repo-wide suite not run (only the two affected workspaces).
  • No trial merge into current main. The snapshot's baseRefOid (1919ff97…) is not present locally and had already drifted from HEAD^1; merge freshness is unmeasured.
  • Per-commit attribution. Depth-2 checkout: only the merge commit, HEAD^1, and HEAD^2 exist; the snapshot lists 19 commits. All results are for the aggregate HEAD^1..HEAD diff.
  • macOS / Windows. Not executed here or, per the author, in CI at this commit; F2-class and path-separator behaviour on those platforms remains CI's to confirm.
  • A harness bug of mine, disclosed. The first mutation runner classified every mutation SURVIVED because it anchored on /^\s*Tests/ while vitest colourises that label (ESC[2m Tests ESC[22m). The runs themselves were valid (exit codes and mutatedShaDiffers were correct); only the classification was broken. tmp/reparse-muts.mjs recomputes from the raw logs and additionally requires a KILLED row to name a failing test and an assertion, so an exit 1 from a collection error cannot masquerade as a kill. mut-run.mjs in the artifact dir carries the fix. D1 was re-run after its anchor was corrected (my indentation error, occurrences=0), and is reported from the re-run.

Methodology

Environment: the CI verify container (node v22.23.2, no GitHub token), tree = refs/pull/11171/merge at depth 2 with npm ci + npm run build pre-existing at HEAD. The A/B used one scratch git worktree under tmp/ at HEAD^1 with internal @qwen-code/* links re-pointed and realpath-asserted per arm, its web-shell lib built in place, removed after the cells were captured. Harnesses drove real production code with only the external vscode API and the webview transport faked: the real DiffManager, real activate() listener, real WebViewProvider, real EmbeddedApp render, real web-shell ToolGroup. The mutation runner applies one exact-string mutation, asserts the anchor occurs exactly once, runs the named suite, records exit/status/failing-test/assertion lines, then restores and re-verifies by sha256; every row is in raw/mut-*.jsonl and raw/mut-*.log, and the corrected matrix is raw/mutation-matrix.txt with its witness at evidence/02-mutation-matrix.png. Gates: vitest per affected workspace (companion 149, web-shell 944), tsc --noEmit per package (both exit 0), eslint over all 13 changed files (exit 0) with a planted-unused-variable liveness control that was reported and then restored by sha256. The fail: 0 in assertions.json is honest: no executed assertion failed; the verdict is findings because F5 and the carried F1–F4 are concrete, reviewer-relevant coverage and description gaps. The 37 counted assertions are: 8 A/B and control-hygiene checks (three arm outcomes, ARM 1's failures naming values, base realpath, six base-file sha256 identities, base-vs-head dist sha, post-arm tree cleanliness); 6 gates (companion 149, web-shell 944, two tsc --noEmit, eslint over the 13 changed files, eslint planted-violation liveness); 15 mutation rows whose recorded status matched a stated expectation (M1–M4, X1–X4, W1, W2, T1, A1, A2, D1, D2); the 6 sibling probes S1–S6; and 2 census checks (hostOwnsEditDiffPreview: false and hasDiffPreview each occurring 0 times in App.test.tsx). The D3/D4 mutation runs are probes with no prior expectation, so they are reported as F5's evidence rather than counted as pass/fail assertions.

Evidence images

01-ab-base-vs-head

02-mutation-matrix

Harness scripts (mut-run.mjs, reparse-muts.mjs, mutations.json, diff-manager.sibling.test.ts) and raw logs are in this directory.

Qwen Code · sandboxed verification

Flakiness gate log

rounds=5 files=7 skipped=0
file packages/vscode-ide-companion/src/commands/index.test.ts: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/commands/index.test.ts
file packages/vscode-ide-companion/src/diff-manager.test.ts: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/diff-manager.test.ts
file packages/vscode-ide-companion/src/extension.test.ts: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/extension.test.ts
file packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/webview/EmbeddedApp.test.tsx
file packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/webview/providers/WebViewProvider.test.ts
file packages/web-shell/client/App.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/App.test.tsx
file packages/web-shell/client/components/messages/ToolGroup.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/components/messages/ToolGroup.test.tsx


per-file results (P=pass F=fail I=infra-exit, one letter per run):
  packages/vscode-ide-companion/src/commands/index.test.ts: PPPPP
  packages/vscode-ide-companion/src/diff-manager.test.ts: PPPPP
  packages/vscode-ide-companion/src/extension.test.ts: PPPPP
  packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: PPPPP
  packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: PPPPP
  packages/web-shell/client/App.test.tsx: PPPPP
  packages/web-shell/client/components/messages/ToolGroup.test.tsx: PPPPP

verdict: pass
summary: 7 changed test file(s) x 5 identical rounds, no divergence

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/vscode-ide-companion/src/commands/index.test.ts: P (exit 0)
round 1 · packages/vscode-ide-companion/src/diff-manager.test.ts: P (exit 0)
round 1 · packages/vscode-ide-companion/src/extension.test.ts: P (exit 0)
round 1 · packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: P (exit 0)
round 1 · packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: P (exit 0)
round 1 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/components/messages/ToolGroup.test.tsx: P (exit 0)
round 2 · packages/vscode-ide-companion/src/commands/index.test.ts: P (exit 0)
round 2 · packages/vscode-ide-companion/src/diff-manager.test.ts: P (exit 0)
round 2 · packages/vscode-ide-companion/src/extension.test.ts: P (exit 0)
round 2 · packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: P (exit 0)
round 2 · packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: P (exit 0)
round 2 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/components/messages/ToolGroup.test.tsx: P (exit 0)
round 3 · packages/vscode-ide-companion/src/commands/index.test.ts: P (exit 0)
round 3 · packages/vscode-ide-companion/src/diff-manager.test.ts: P (exit 0)
round 3 · packages/vscode-ide-companion/src/extension.test.ts: P (exit 0)
round 3 · packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: P (exit 0)
round 3 · packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: P (exit 0)
round 3 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/components/messages/ToolGroup.test.tsx: P (exit 0)
round 4 · packages/vscode-ide-companion/src/commands/index.test.ts: P (exit 0)
round 4 · packages/vscode-ide-companion/src/diff-manager.test.ts: P (exit 0)
round 4 · packages/vscode-ide-companion/src/extension.test.ts: P (exit 0)
round 4 · packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: P (exit 0)
round 4 · packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: P (exit 0)
round 4 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/components/messages/ToolGroup.test.tsx: P (exit 0)
round 5 · packages/vscode-ide-companion/src/commands/index.test.ts: P (exit 0)
round 5 · packages/vscode-ide-companion/src/diff-manager.test.ts: P (exit 0)
round 5 · packages/vscode-ide-companion/src/extension.test.ts: P (exit 0)
round 5 · packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: P (exit 0)
round 5 · packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: P (exit 0)
round 5 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/components/messages/ToolGroup.test.tsx: P (exit 0)

Evidence images

01-ab-base-vs-head

02-mutation-matrix

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 commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Re-running at dc665a60 — the first head since 8b1ebb2076 that a fresh verdict can actually supersede, which was the reason you gave for triggering it. That reason holds: dismiss_stale_reviews_on_push is false in the main ruleset, so my old CHANGES_REQUESTED reviews are still live purely because nothing dismissed them, and a new state-bearing review at this head replaces them.

Template ✓ — I checked the live body heading by heading against .github/pull_request_template.md. All nine required sections are present, plus the optional ### Environment (optional), plus a paragraph-matched 中文说明. R1-19, the gate that stopped this PR on 2026-09-06, does not stand and has not stood since the body was restructured on 2026-09-09.

Problem: observed, not theoretical. #10557 is open and carries type/bug / priority/P2 / scope/vscode / scope/web-shell / roadmap/ide-integration. I also traced the mechanism at this head instead of taking the description's word for it. Down: EmbeddedApp posts openDiff with data.requestId, FileMessageHandler.handleOpenDiff maps that to permissionRequestId, and showDiff stores it on DiffInfo. Up: onDidCloseTextDocumentDiffManager.cancelDiff fires ide/diffClosed on onDidChangeEmitter, and the only consumer of that emitter is ide-server.ts — IDE-mode MCP transports. A web shell is not one, so it genuinely had no channel to learn the tab closed. hostOwnsEditDiffPreview was a bare true at the EmbeddedApp call site, so the locked row had nothing to fall back to. The bug is real and the causal chain is exactly as described.

Direction: aligned. This is a correctness fix in two first-party surfaces — the VS Code companion and the web shell — on the permission flow, which is the thing users actually feel. Nothing here adds a customization knob or a new public contract; onDidClosePermissionDiff is a new event on an existing class and permissionDiffClosed is a new message type on an existing channel. The composer.editUnavailable / composer.editExpired half of #9911 is the opposite of speculative surface: those localized strings already existed with no consumer anywhere in the repo, and this wires them up rather than adding new ones.

Size: core scope, by the cross-package rule — the diff spans packages/vscode-ide-companion and packages/web-shell. Breakdown at this head:

files lines
production logic 6 163
test 7 1191
generated / schema 0 0

163 production lines is well under the 500-line escalation threshold, and the title is fix, so Stage 0 Tier 1 does not apply. No maintainer-awareness escalation from Stage 0, and no 1000+ large-PR advisory. 13 files reads as broad but is not: seven of them are tests, and the six production files are 3–42 lines each.

Approach: the scope feels right, and it matches what I would have written independently. Faced with "the host owns the preview and the preview just vanished", the minimal fix is a typed close event on DiffManager, fanned out through the registry qwen.diff.accept / qwen.diff.cancel already use, handled as a hand-back rather than a vote. That is what this does. The alternatives are worse: polling the host is strictly more code and strictly laggier, refusing to let the host own the preview duplicates the diff in two places, and auto-dismissing the permission on tab close throws the edit away — which is the bug with extra steps. Reusing the existing permission-aware provider registry instead of inventing a second notification path is the right call.

Two scope notes, neither a blocker and neither worth a tenth round:

Risk: no elevated risk signals. Stage 1e matched nothing — none of the changed files are in the revert-correlated set (geminiChat, shell.ts, mcp-client, acpConnection, sandbox.ts, and the rest).

Moving on to code review. 🔍

中文说明

dc665a60 上重跑 —— 这是自 8b1ebb2076 以来第一个「新判决能够真正取代旧判决」的 head,也正是你触发重跑的理由。这个理由成立:main 的 ruleset 里 dismiss_stale_reviews_on_pushfalse,所以我之前那些 CHANGES_REQUESTED review 仍然存活,纯粹是因为没有任何东西 dismiss 它们;而在当前 head 上一次新的、带状态的 review 就会取代它们。

模板 ✓ —— 我把线上正文逐个标题对照 .github/pull_request_template.md 核过。九个必需章节全部存在,外加可选的 ### Environment (optional),以及逐段对应的中文说明。R1-19(2026-09-06 拦住这个 PR 的那道门禁)不成立,而且自 2026-09-09 正文重构后就不成立了。

问题: 已观测到的,不是理论性的。#10557 处于 open,带有 type/bug / priority/P2 / scope/vscode / scope/web-shell / roadmap/ide-integration。我也在当前 head 上亲自追了这条机制,而不是采信描述的说法。下行:EmbeddedApp 发出带 data.requestIdopenDiffFileMessageHandler.handleOpenDiff 把它映射成 permissionRequestIdshowDiff 存进 DiffInfo。上行:onDidCloseTextDocumentDiffManager.cancelDiffonDidChangeEmitter 上发出 ide/diffClosed,而这个 emitter 的唯一消费者是 ide-server.ts —— IDE 模式的 MCP transport。web shell 不是其中之一,所以它确实没有任何通道能得知标签页被关掉了。hostOwnsEditDiffPreviewEmbeddedApp 的调用点上是一个裸的 true,因此被锁住的那一行没有任何可回退的东西。bug 是真的,因果链与描述完全一致。

方向: 对齐。这是两个第一方界面(VS Code companion 与 web shell)上权限流程的正确性修复,而权限流程正是用户真正会感知到的东西。这里没有新增任何定制开关或公共契约;onDidClosePermissionDiff 是既有类上的新事件,permissionDiffClosed 是既有通道上的新消息类型。#9911composer.editUnavailable / composer.editExpired 那一部分恰恰与「凭空造界面」相反:这两条本地化文案本来就存在、在仓库里没有任何消费者,这里是把它们接上,而不是新增。

规模: 属于核心范围,依据是跨包规则 —— diff 横跨 packages/vscode-ide-companionpackages/web-shell。当前 head 的拆分见上表:生产逻辑 6 个文件 163 行,测试 7 个文件 1191 行,生成/schema 0 行。

163 行生产代码远低于 500 行的升级阈值,标题是 fix,所以 Stage 0 Tier 1 不适用。Stage 0 没有触发维护者知会升级,也没有 1000+ 的大 PR 建议。13 个文件看起来面广,实际不是:其中七个是测试,六个生产文件每个只有 3–42 行。

方案: 范围合理,也与我独立会写出的方案一致。面对「宿主拥有预览、而预览刚刚消失了」这个问题,最小修复就是在 DiffManager 上加一个带类型的关闭事件,通过 qwen.diff.accept / qwen.diff.cancel 已经在用的那个 registry 分发出去,并把它当作「交还」而不是「投票」来处理。这个 PR 做的正是这件事。其他选项都更差:轮询宿主严格来说代码更多、延迟更大;不允许宿主拥有预览会在两个地方重复渲染 diff;而在关闭标签页时自动撤销权限,等于把这次编辑丢掉 —— 那是原 bug 再加一步。复用既有的 permission-aware provider registry、而不是另造一条通知路径,是正确的选择。

两点范围说明,都不是阻断项,也不值得为此走第十轮:

风险: 无升级风险信号。Stage 1e 没有命中任何一项 —— 改动的文件都不在与 revert 相关的那组路径里(geminiChatshell.tsmcp-clientacpConnectionsandbox.ts 等)。

进入代码审查 🔍

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

Reviewed at dc665a602f686b935e080479bdeec3b7b2d83f16 · 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

No Critical findings at dc665a60. I am deliberately not minting new Suggestions on a PR at round ten — AGENTS.md says land only Critical fixes past ~5 rounds and defer the rest, and the ten Suggestions the last /review confirmed are already recorded on this thread. What follows is what I verified, because the value of this pass is closing the gap the last run left open, not adding to the pile.

My independent proposal, written before reading the diff: put a typed close event on DiffManager, fan it out through the registry the vote commands already use, and handle it on the shell side as a hand-back rather than a vote — dropping the request from the open-diffs map, stopping the row from being locked, and not reopening a tab the user just closed. The PR does that. I did not find a simpler path it missed.

The chain, walked end to end. The last run's reason for deferring was that the host-side behaviour had no witness and "no lane in this repository can produce one". Walking the actual code narrows that a great deal, because almost none of the chain is new:

  1. EmbeddedApp posts openDiff with data.requestIdFileMessageHandler.handleOpenDiff maps it to permissionRequestIdshowDiffCommandshowDiff stores it on DiffInfo. All pre-existing; the PR does not touch this direction.
  2. User clicks the tab close → onDidCloseTextDocument (gated on DIFF_SCHEME) → cancelDiff. Pre-existing wiring, unchanged.
  3. cancelDiff reads diffInfo before closeDiffEditor deletes it, fires ide/diffClosed — and now also fires onDidClosePermissionDiff off the same diffInfo. This is the load-bearing point: the new fire is gated on exactly the object the shipped ide/diffClosed notification already depends on, so if the CLI's existing close-diff flow works, this fires.
  4. extension.tschatProviderRegistry.getPermissionAwareProviders()notifyPermissionDiffClosedsendMessageToWebViewgetActiveWebview().postMessage. Same call, same channel, as the already-working webShellPermissionDecision and permissionResolved. No relay work needed, no whitelist to miss the new type.
  5. EmbeddedApp handler is source-gated on event.source === window.parent, matching the decision handler — correct, since MCP apps and artifact previews are scriptable iframes inside this webview.
  6. hostOwnsEditDiffPreview=falseApp customization → ToolGroup.isHostOwnedEditApproval=falselocksPendingEditApproval=false → the useEffect that already lists locksPendingEditApproval in its deps recomputes expanded → the row opens and approval reaches the inline renderer.

So the only genuinely new host-side dependency is one EventEmitter.fire inside a function that is already reached on tab close. That is a much smaller unverified surface than "the host-side behaviour".

The three paths that must not fire — each checked, not assumed.

  • qwen.diff.accept / qwen.diff.cancel: both call acceptDiff / cancelDiff only under if (docUri && isManagedDiff && !permissionRequestId). A request-bound diff never reaches cancelDiff, so a vote from the diff editor cannot echo back as a dismissal. Guard is pre-existing, not added here.
  • Web-shell-initiated closeDiff: closeDiffEditor deletes the map entry before vscode.window.tabGroups.close, so the onDidCloseTextDocumentcancelDiff hop that follows finds no diffInfo and returns early. Independently, closeDiff's own new fire is gated on permissionRequestId === undefined, and this caller passes the id. Two separate reasons it cannot loop.
  • A stale dismissal arriving after the vote: webShellPermissionRequestIdRef.current === requestId gates the unlock, and updateTranscript resets both dismissedPermissionDiffIdRef and hostOwnsEditDiffPreview as soon as permissionToFocus moves off that id. Worst case is a one-frame flicker on an already-resolved row, and it self-heals.

I am striking the round-8 deferred item about closeAll(), and I checked it rather than repeating the last run's reasoning. qwen.diff.closeAll is not in contributes.commands in package.json — no palette entry, no keybinding — so a user cannot invoke it. Its single caller is WebViewProvider's permissionResponse handler, which runs after this.pendingPermissionResolve?.(optionId). Firing the dismissal there would unlock a row whose permission is already resolved. Not firing is correct.

Why the newly-dynamic hostOwnsEditDiffPreview is lower risk than it looks. Every consumer of that flag — App.tsx:3249 (customization context), App.tsx:12863 (the id-targeted host-decision guard), ToolGroup.tsx:1170/1218/1285, ParallelAgentsGroup.tsx:587 — reads false as "the shell owns the preview". And false is the default in App.tsx:2970, which is what every non-VS-Code host already runs: the browser web shell and the desktop shell have been exercising that branch in production all along. The VS Code companion was the only consumer pinning it to true. This PR does not switch on a dead branch; it lets one host join the configuration the others already use. That is the single strongest safety argument here and it was missing from the earlier rounds.

Non-blocking, noted and not requested. The two #9911 toast gates still disagree: the prepareSubmit path uses admissionOwnerIsCurrent(), which includes the composerSourceVersionRef conjunct, while the queued path uses the narrower submissionSessionIsCurrent(). I confirmed the first is not dead — startPreparing() does not bump the version, and the only bumps are composer edits and session switches — so the toast does fire in the ordinary case; it is just suppressed if the user started typing while the preflight was in flight. That is D4-1, reported in round 4, and defensible either way. Also worth a line in the description: the shouldAutoExpandisEditToolName swap in ToolGroup.tsx is load-bearing for the bare write alias, not cleanup (see Stage 1).

The hand-back flow

sequenceDiagram
    participant P1 as User
    participant P2 as DiffManager
    participant P3 as extension.ts
    participant P4 as WebViewProvider
    participant P5 as EmbeddedApp
    participant P6 as ToolGroup
    P1->>P2: closes the native diff tab by hand
    P2->>P2: onDidCloseTextDocument then cancelDiff
    P2->>P3: fire onDidClosePermissionDiff with requestId
    P3->>P4: notifyPermissionDiffClosed
    P4->>P5: postMessage permissionDiffClosed
    P5->>P5: drop the request, mark it dismissed
    P5->>P6: hostOwnsEditDiffPreview false
    P6->>P1: row unlocks and renders the diff inline
    Note over P5,P6: the next permission request resets ownership to the host
Loading
Files changed (13 of 13)
File What changed
packages/vscode-ide-companion/src/diff-manager.ts New onDidClosePermissionDiff emitter, fired from cancelDiff (hand close) and from the id-less branch of closeDiff; disposed in dispose(). 38 lines, all additive.
packages/vscode-ide-companion/src/extension.ts One subscription in the existing context.subscriptions.push(...) list: fans the dismissal to permission-aware providers, logs when there is nobody to tell, try/catch so one dead surface cannot take down the others.
packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts notifyPermissionDiffClosed — posts permissionDiffClosed over the existing sendMessageToWebView channel, with a drop-point log mirroring the open direction.
packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx hostOwnsEditDiffPreview becomes state (default true, matching the old constant) plus a dismissedPermissionDiffIdRef; handles the new message, suppresses reopening a dismissed diff, resets ownership on request change and on teardown.
packages/web-shell/client/App.tsx The #9911 half: toasts a rejected preflight on both submit paths, and splits submissionSessionIsCurrent out of submissionOwnerIsCurrent so the narrower gate composes on the same base.
packages/web-shell/client/components/messages/ToolGroup.tsx 1 line: shouldAutoExpand reuses isEditToolName instead of four literals, which adds the bare write alias so a handed-back row actually expands.
packages/vscode-ide-companion/src/diff-manager.test.ts +253 — the two fire sites and the paths that must not fire.
packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx +462 — hand-back handling, no-reopen, ownership reset, and the #9911 rewind witnesses.
packages/vscode-ide-companion/src/extension.test.ts +170 — the fan-out subscription, including the no-provider log path.
packages/vscode-ide-companion/src/commands/index.test.ts +56 — the #10585 companion witnesses.
packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts +14 — notifyPermissionDiffClosed posts and logs.
packages/web-shell/client/App.test.tsx +130 — the preflight toast paths, including the session-switch case.
packages/web-shell/client/components/messages/ToolGroup.test.tsx +104 — sub-agent row kept open under host ownership, and the bare-write hand-back expanding with content on screen.

Test evidence — the PR's own CI at dc665a60

I did not build or run anything from this PR; per the triage rules the review is static and the test signal comes from the PR's own CI, read through the API. 42 checks on the head commit: 15 success, 27 skipped, 0 failure, 0 pending. No failing job, so there is no log excerpt to quote.

Check Conclusion
Test (ubuntu-latest, Node 22.x) success
Lint & Static (ubuntu-latest, Node 22.x) success
Integration Tests (no-AK, No Sandbox) 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
Desktop Shell (windows-2022) success
Classify PR success
Test (macos-latest, Node 22.x) skipped
Test (windows-latest, Node 22.x) skipped
Integration Tests (CLI, No Sandbox) skipped

The other 31 checks are skipped bot-orchestration jobs (assign, route, label, authorize, review-pr, publish-resolution, and similar). Table region maintained by the finalize workflow.

On the two skipped platform lanes — this is a repo condition, not a gap in this PR. Both prior runs treated the missing macOS and Windows confirmation as something owed before merge. It is not owed by this author, and it cannot be: test_macos and test_windows in ci.yml are gated on merge_group || schedule || workflow_dispatch, so they never run on pull_request at all. The workflow's own comment says why, and it is worth quoting because it settles the question:

The macOS and Windows lanes' ONLY remaining trigger, and therefore this repository's only signal about a host that is not Linux with a GNU userland. Those two are otherwise gated on merge_group, and the merge queue is not enabled here — no queue run since 2026-07-02 — while their pull-request trigger is off until the standing Windows failures are fixed. A regression therefore surfaces here, one day later, on main, and nowhere else: treat a red nightly as a blocker, not as noise.

There is no merge queue, so merge_group never fires either. The nightly on main is the only place those two lanes run, for every PR in this repository. Deferring this PR for platform evidence would be asking the author for something no lane can produce, and the Windows trigger is off precisely because main already has standing Windows failures that are not this PR's. Note also that Desktop Shell (windows-2022) is green but is not evidence for these files — that lane compiles and tests the Tauri crate, and this diff contains no Rust.

Against that, what does exist: the Linux unit lane covers all thirteen changed files; the author ran the five vscode-ide-companion suites on macOS arm64 / Node 22 at this head and reports 163 passed (their claim, not something I re-ran); and the two web-shell files pass in the green Linux lane. Reviewing the diff, nothing in it is platform-sensitive — the additions are an EventEmitter, React state and refs, a postMessage, and a lowercased string comparison. The pre-existing path handling in closeDiff is untouched.

Not verified, and why: there is no capture from a VS Code Extension Development Host, so steps 2 and 5 of the test plan — the native tab staying closed, then reopening on the next request — rest on the code walk above rather than on an observation. I am satisfied by that walk for the reason in the first section: the trigger hop is pre-existing and already load-bearing for ide/diffClosed. Windows is unverified by anyone, and per the above the nightly is the designed place it would surface.

Sandboxed verification would settle part of this: @qwen-code /verify — that the web-shell half of the hand-back (row unlocks, diff renders inline, the vote still lands from the row) is load-bearing against the base build is assertable A/B, and it would also confirm the #9911 toast actually fires rather than being gated away, which no current test pins on the false branch. It cannot reach the native tab: nothing in this repository drives an Extension Development Host, so the host-side half stays a code-walk conclusion no lane can upgrade. You have write access, so /tmux is also available, but this is a VS Code webview surface rather than a TUI one and it would not add signal here.

中文说明

代码审查:在 dc665a60 上未发现 Critical。 在一个已经走到第十轮的 PR 上我刻意不再新增 Suggestion —— AGENTS.md 的规定是超过约 5 轮之后只落地 Critical 修复、其余延后,而上一轮 /review 确认的那十条 Suggestion 已经记录在这个 thread 里了。下面写的是我核实过的东西,因为这一轮的价值在于补上上一轮留下的缺口,而不是往堆里再加东西。

我在读 diff 之前独立写下的方案是:在 DiffManager 上加一个带类型的关闭事件,通过投票命令已经在用的那个 registry 分发出去,并在 shell 一侧把它当作「交还」而不是「投票」处理 —— 从 open-diffs map 里移除该请求、让那一行不再被锁、并且不重新打开用户刚刚关掉的标签页。这个 PR 就是这么做的。我没有找到它漏掉的更简路径。

整条链路我逐段走过。 上一轮延后的理由是宿主侧行为没有见证、而「本仓库没有任何 lane 能产出这样的见证」。真去读代码之后,这个说法要收窄很多,因为这条链上几乎没有一段是新的:

  1. EmbeddedApp 发出带 data.requestIdopenDiffFileMessageHandler.handleOpenDiff 映射成 permissionRequestIdshowDiffCommandshowDiff 存进 DiffInfo。全部既有,本 PR 没有碰这个方向。
  2. 用户点标签页关闭 → onDidCloseTextDocument(以 DIFF_SCHEME 为门)→ cancelDiff。既有接线,未改动。
  3. cancelDiffcloseDiffEditor 删除条目之前读到 diffInfo,发出 ide/diffClosed —— 现在还用同一个 diffInfo 发出 onDidClosePermissionDiff。这是承重点:新的 fire 恰好挂在已上线的 ide/diffClosed 通知所依赖的那个对象上,所以只要 CLI 现有的关闭 diff 流程是通的,它就一定会触发。
  4. extension.tschatProviderRegistry.getPermissionAwareProviders()notifyPermissionDiffClosedsendMessageToWebViewgetActiveWebview().postMessage。与已经在工作的 webShellPermissionDecisionpermissionResolved 是同一个调用、同一条通道。不需要新增中继,也不存在漏掉新类型的白名单。
  5. EmbeddedApp 的处理器以 event.source === window.parent 做来源门禁,与决策处理器一致 —— 这是对的,因为 MCP app 和 artifact 预览是这个 webview 内部可执行脚本的 iframe。
  6. hostOwnsEditDiffPreview=falseApp customization → ToolGroup.isHostOwnedEditApproval=falselocksPendingEditApproval=false → 那个已经把 locksPendingEditApproval 列进依赖数组的 useEffect 重算 expanded → 该行展开,approval 抵达内联渲染器。

所以真正新增的宿主侧依赖,只是一个已经会在标签页关闭时被抵达的函数里多了一次 EventEmitter.fire。这比「宿主侧行为」这个说法所暗示的未验证面要小得多。

三条不该触发的路径 —— 每一条都是查过的,不是假设的。

  • qwen.diff.accept / qwen.diff.cancel:两者都只在 if (docUri && isManagedDiff && !permissionRequestId) 下才调用 acceptDiff / cancelDiff。绑定请求的 diff 永远到不了 cancelDiff,所以从 diff 编辑器投票不会作为撤销回声传回来。这个守卫是既有的,不是本 PR 新加的。
  • web shell 主动发起的 closeDiffcloseDiffEditorvscode.window.tabGroups.close 之前删掉 map 条目,因此随后的 onDidCloseTextDocumentcancelDiff 找不到 diffInfo,提前返回。另外,closeDiff 自己新增的 fire 以 permissionRequestId === undefined 为门,而这个调用方传了 id。两个独立的理由说明它不会成环。
  • 投票之后才姗姗来迟的撤销:解锁以 webShellPermissionRequestIdRef.current === requestId 为门,而 updateTranscript 一旦看到 permissionToFocus 离开该 id,就会同时重置 dismissedPermissionDiffIdRefhostOwnsEditDiffPreview。最坏情况是在一个已经解决的行上闪一帧,并且会自愈。

第 8 轮那条关于 closeAll() 的延后项,我划掉它,而且是我自己查过才划的,不是复述上一轮的理由。qwen.diff.closeAll 不在 package.jsoncontributes.commands 里 —— 没有命令面板入口、没有快捷键 —— 所以用户无法调用它。唯一的调用方是 WebViewProviderpermissionResponse 处理器,而它运行在 this.pendingPermissionResolve?.(optionId) 之后。在那里触发撤销,会解锁一个权限已经解决的行。不触发才是对的。

为什么变成动态的 hostOwnsEditDiffPreview 风险比看上去低。 这个 flag 的每一个消费者 —— App.tsx:3249(customization context)、App.tsx:12863(按 id 定向的宿主决策守卫)、ToolGroup.tsx:1170/1218/1285ParallelAgentsGroup.tsx:587 —— 都把 false 读作「shell 拥有预览」。而 false 正是 App.tsx:2970 里的默认值,也就是所有非 VS Code 宿主一直在跑的取值:浏览器 web shell 和桌面 shell 长期以来都在生产环境里走这条分支。VS Code companion 是唯一把它钉死为 true 的消费者。所以这个 PR 不是打开了一条死分支,而是让一个宿主加入其他宿主早已在用的配置。这是这里最有力的一条安全性论证,而它在之前几轮里是缺失的。

非阻断,仅记录、不作为要求。 两处 #9911 toast 的门禁仍然不一致:prepareSubmit 那条路径用 admissionOwnerIsCurrent(),其中包含 composerSourceVersionRef 这一项;而排队那条路径用更窄的 submissionSessionIsCurrent()。我确认了前者不是死代码 —— startPreparing() 不会递增该版本号,唯一的递增点是 composer 编辑与会话切换 —— 所以常规情况下 toast 确实会触发;只是在预检在飞行途中用户开始打字时会被压掉。这就是 D4-1,第 4 轮已报告,两种取舍都说得通。另外描述里值得补一句:ToolGroup.tsxshouldAutoExpandisEditToolName 的替换对裸名 write 别名是承重的,不是清理(见 Stage 1)。

测试证据。 我没有构建或运行本 PR 的任何代码;按 triage 规则,审查是静态的,测试信号来自 PR 自己的 CI、通过 API 读取。head commit 上共 42 个 check:15 success、27 skipped、0 failure、0 pending,没有失败的 job,因此没有日志摘录可引。表格见上。

关于两条被跳过的平台 lane —— 这是仓库层面的状况,不是本 PR 的缺口。 之前两轮都把缺失的 macOS 与 Windows 确认当作合并前欠着的东西。它不欠在这位作者身上,也不可能由他来还:ci.yml 里的 test_macostest_windowsmerge_group || schedule || workflow_dispatch 为门,因此在 pull_request 上根本不会跑。workflow 自己的注释说明了原因,值得直接引用(原文见上方英文部分),因为它把这个问题结掉了:合并队列没有启用(自 2026-07-02 起就没有),所以 merge_group 也不会触发;这两个 lane 唯一的触发点是每日夜间跑在 main 上的那次,对本仓库的每一个 PR 都是如此。而 Windows 的 PR 触发器之所以关着,正是因为 main 上已经有与本 PR 无关的、长期存在的 Windows 失败。为了平台证据而延后这个 PR,等于要求作者提供一个没有任何 lane 能产出的东西。另外请注意 Desktop Shell (windows-2022) 虽然是绿的,但对这些文件不构成证据 —— 那条 lane 编译并测试的是 Tauri crate,而本 diff 不含 Rust。

在此前提下,现有的证据是:Linux 单元 lane 覆盖了全部十三个改动文件;作者在本 head 上于 macOS arm64 / Node 22 跑了 vscode-ide-companion 的五个套件并报告 163 passed(这是作者的说法,不是我复跑的结果);两个 web-shell 文件在绿色的 Linux lane 中通过。就 diff 本身来看,其中没有任何平台敏感的东西 —— 新增的是一个 EventEmitter、React state 与 ref、一次 postMessage,以及一个小写化后的字符串比较。closeDiff 里既有的路径处理未被改动。

未验证的部分及原因: 没有 VS Code Extension Development Host 的录证,所以测试计划的第 2 步和第 5 步 —— 原生标签页保持关闭、并在下一次请求时重新打开 —— 依赖的是上面那次代码走查,而不是一次观测。我对这个走查是满意的,理由见第一节:触发它的那一跳是既有的,并且已经承载着 ide/diffClosed。Windows 无人验证,而按上述说明,夜间 lane 正是它按设计会暴露的地方。

沙箱验证能结掉其中一部分:@qwen-code /verify —— 交还机制在 web-shell 这一半(行解锁、diff 内联渲染、投票仍能从该行落地)相对基线构建是否承重,是可以做 A/B 判定的;它还能确认 #9911 的 toast 真的会触发、而不是被门禁挡掉,这一点目前没有任何测试钉住 false 分支。它够不到原生标签页:本仓库没有任何东西驱动 Extension Development Host,所以宿主侧那一半仍然是一个没有 lane 能升级的代码走查结论。你有写权限,因此 /tmux 也可用,但这是 VS Code webview 界面而非 TUI 界面,它在这里不会增加信号。

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

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean across every stage; the one point I am withholding is the platform evidence no lane in this repo can produce, and that is a repo condition rather than a doubt about this diff.

Stepping back. The production change is 163 lines across six files and it does one thing: it gives a closed permission diff a channel back to the surface that was waiting on it. Everything I traced behaves the way the description says, including the three paths that must not fire — and two of those three are guarded by code that predates this PR, which is the strongest thing I can say about a fix like this: it hangs off invariants that already had to hold.

The question that made me defer at 8b1ebb2076 was whether the host-side behaviour works, on the grounds that nothing here could witness it. Having now walked the chain link by link instead of reasoning about it in the abstract, I think I framed that gap too broadly last time. The tab-close hop (onDidCloseTextDocumentcancelDiff) is pre-existing and already load-bearing: cancelDiff fired ide/diffClosed before this PR, and that is how the IDE-mode CLI learns a diff was closed. The new fire reads the same diffInfo object that the shipped notification reads. And the delivery channel is the same sendMessageToWebView call that already carries webShellPermissionDecision and permissionResolved — no new relay, no message-type whitelist that could silently drop it. What is genuinely new on the host side is one EventEmitter.fire in a function that is already reached. That is a bet I am willing to record, and I am recording it as a code-walk conclusion rather than as an observation, which is what the Stage 2 comment says too.

The second thing that moved me is one I should have caught earlier. hostOwnsEditDiffPreview reads as a scary change — a constant becoming state, on a flag five components read. But false is the default in App.tsx, and it is what the browser web shell and the desktop shell have been running in production all along. The VS Code companion was the only consumer pinning it to true. This PR does not switch on an unexercised branch; it lets one host join a configuration the others already use, and the reset paths (request change, teardown, auto-approval mode) all put it back. Every consumer of the flag does something sensible in the false state, and I named each of them in Stage 2 rather than assuming the prop threading was complete.

On scope. #10585 belongs here — twelve witnesses on exactly the paths this changes. #9911 does not, strictly: it is a different user-visible behaviour on a shared submit path, and the two toast gates still disagree about when to speak. I am not asking for a split at round ten, and I am not deferring over it either. The re-run rule is explicit that scope and hygiene concerns get noted and the PR gets approved, and that is the right call here: the cost of an eleventh round exceeds the value of a tidier history, and D4-1 has been on the record since round four without anyone arguing it is a defect.

The stale-review loop, which is the reason you re-triggered. Your reading was correct and mine was over-cautious. dismiss_stale_reviews_on_push is false on main, so my CHANGES_REQUESTED from 8b1ebb2076 was never going to clear on its own, and last time I declined to supersede it on the grounds that reversing my own blocking review is a call a human should make deliberately. You have now made that call deliberately — in writing, on the thread, naming the review and the reason — and the one Critical it stood on is verifiably cleared. So this run approves at the current head, which replaces that review as my latest state-bearing verdict. Nothing about that is silent.

Two things I want on the record rather than buried, neither of which blocks:

  1. Nobody has verified this on Windows, and no lane will. test_windows and test_macos are gated to merge_group || schedule || workflow_dispatch, the merge queue has been off since 2026-07-02, and the Windows PR trigger is off because main already has standing Windows failures. The nightly on main is the only signal this repository has for a non-Linux host, and its own comment says to treat a red nightly as a blocker. That is a repo-wide exposure worth a separate conversation; it is not something to hold this PR for, and I reviewed the diff for platform sensitivity and found none.
  2. The host-side half of the test plan still has no capture. Steps 2 and 5 need an Extension Development Host session. @qwen-code /verify would settle the web-shell half A/B against the base build and confirm the Restore VS Code message edit and rewind after the WebShell cutover #9911 toast actually fires rather than being gated away — worth doing if you want the last bit of certainty, and cheap. It cannot reach the native tab.

If I had to maintain this in six months I would read the event name, read the two guards, and know what it does. The comments explain the why at exactly the three places where the why is non-obvious — the id-less close branch, the source gate, and the narrower toast guard — and nowhere else. That is the house style, and it is what makes a state-handoff fix across an extension boundary reviewable at all. Approving. ✅


Postscript — main moved while this review was being written, and the PR is now conflicting.

main advanced to 782c22bff531 at 09:41:52Z and #11628 merged at 09:42:09Z; my approval was submitted at 09:56:55Z. The PR head did not move, so the code I reviewed is the code I approved and the verdict above stands. But mergeable is now CONFLICTING / mergeStateStatus: DIRTY, and four files are touched by both sides:

file landed on main via
packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx #11628 fix(vscode): localize rewind preflight errors
packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx #11628
packages/web-shell/client/App.tsx #11509 feat(web-shell): improve composer and context usage presentation
packages/web-shell/client/App.test.tsx #11509

#11628 is complementary to this PR's #9911 half, not a duplicate of it. I checked instead of assuming, because the titles overlap, the subject is the same, and it is by the same author. #11628 localized the throw sites in the companion's rewind preflight and added a new composer.editFailed string. Its own description says the only consumer is "the web-shell preflight catch, which console.warns the error and cancels the prompt" — and I confirmed that is still true on main at 782c22bff531: both catch sites (App.tsx:9652 and :10423) warn and return, with no toast. So the preflight now throws four localized failures across three distinct strings (editUnavailable at EmbeddedApp.tsx:1609, editFailed at :1618, editExpired at :1633, editFailed at :1641) and not one of them reaches the user. The display half is still missing and still this PR's to land. #9911 does not need to move out after all — main just did the other half of it.

One correction falls out of that merge: the description says composer.editUnavailable / composer.editExpired are the unconsumed strings. There are now three, composer.editFailed among them. The toast surfaces all of them, which is the right behaviour — only the enumeration needs updating.

How to read the approval, given this. dismiss_stale_reviews_on_push is false on main, so my APPROVED at dc665a60 will survive the conflict-resolution push instead of being dismissed by it. That is a hazard worth naming out loud: the resolution lands in precisely the #9911 code path where main just changed the thrown-error contract, plus two large test files. Please read the approval as covering dc665a60 and nothing after it, and re-run @qwen-code /triage at the post-merge head. I would much rather re-read a four-file conflict resolution than have this green checkmark silently inherited by one.

The merge is not gated on me in any case. reviewDecision moved from CHANGES_REQUESTED to REVIEW_REQUIRED, so the stale block from 8b1ebb2076 is cleared — but the ruleset's require_code_owner_review still needs a human who owns these paths.

中文说明

Confidence: 4/5 —— 每个阶段都干净;我扣掉的那一分是平台证据,而本仓库没有任何 lane 能产出它,那是仓库层面的状况,不是对这个 diff 的疑虑。

退一步看整体。生产代码改动是六个文件里的 163 行,只做一件事:给一个被关闭的权限 diff 一条回到正在等待它的那个界面的通道。我追过的每一处都按描述所说的方式行为,包括那三条不该触发的路径 —— 而这三条里有两条是由早于本 PR 的代码守卫着的。对于这样一个修复,这是我能给出的最强评价:它挂在一些本来就必须成立的不变量上。

让我在 8b1ebb2076 上选择延后的那个问题,是宿主侧行为到底有没有效,理由是这里没有任何东西能见证它。而现在我逐环走过这条链、而不是抽象地推理之后,我认为上次我把这个缺口划得太宽了。标签页关闭那一跳(onDidCloseTextDocumentcancelDiff)是既有的,而且已经承重:cancelDiff 在本 PR 之前就会发出 ide/diffClosed,那正是 IDE 模式的 CLI 得知 diff 被关掉的方式。新增的 fire 读的是已上线通知所读的同一个 diffInfo 对象。投递通道也是同一个 sendMessageToWebView 调用,它已经在承载 webShellPermissionDecisionpermissionResolved —— 没有新中继,也不存在可能悄悄丢掉它的消息类型白名单。宿主侧真正新增的东西,是一个已经会被抵达的函数里的一次 EventEmitter.fire。这个赌注我愿意记录下来,而且我把它记为一次代码走查的结论、而不是一次观测 —— Stage 2 的评论里也是这么写的。

第二件让我改变判断的事,是我本该更早发现的。hostOwnsEditDiffPreview 看起来是个吓人的改动 —— 一个常量变成了状态,而这个 flag 有五个组件在读。但 falseApp.tsx 里的默认值,也是浏览器 web shell 和桌面 shell 一直在生产环境里跑的取值。VS Code companion 是唯一把它钉死为 true 的消费者。所以这个 PR 不是打开了一条没被走过的分支,而是让一个宿主加入其他宿主早已在用的配置,而各条重置路径(请求变化、销毁、自动批准模式)都会把它放回去。这个 flag 的每一个消费者在 false 状态下都做出合理的行为,而我在 Stage 2 里把它们逐个点了名,而不是假设 prop 的串联是完整的。

关于范围。#10585 属于这里 —— 十二处见证,正好在本 PR 改动的路径上。严格说 #9911 不属于:它是共享提交路径上另一处用户可见行为,而且两处 toast 门禁对于「什么时候该说话」至今仍不一致。我在第十轮不要求拆分,也不会为此延后。重跑规则写得很清楚:范围与整洁性问题记下来、然后批准 PR,而这里这正是对的判断 —— 第十一轮周期的代价超过了一段更整洁历史的收益,而 D4-1 自第四轮起就在记录上,从来没有人主张它是一个缺陷。

关于那个过期 review 的循环,也就是你重新触发的原因。你的解读是对的,我上次过于谨慎了。main 上的 dismiss_stale_reviews_on_pushfalse,所以我那条来自 8b1ebb2076CHANGES_REQUESTED 本来永远不会自己消失;而上次我拒绝取代它,理由是推翻自己的阻断 review 应当由人有意识地来做。现在你已经有意地做了这个决定 —— 以书面形式、在这个 thread 上、点名了那条 review 和理由 —— 而它所依据的那唯一一个 Critical 已被可验证地解除。所以这一轮在当前 head 上批准,这会取代那条 review、成为我最新的带状态判决。这件事没有任何部分是悄悄发生的。

有两件我要写进记录、而不是埋起来的事,都不构成阻断:

  1. 没有人验证过 Windows,也不会有任何 lane 去验证。 test_windowstest_macosmerge_group || schedule || workflow_dispatch 为门,合并队列自 2026-07-02 起就关着,而 Windows 的 PR 触发器之所以关着,是因为 main 上已经有长期存在的 Windows 失败。跑在 main 上的夜间任务是本仓库对非 Linux 宿主唯一的信号,而它自己的注释就写着:红色的夜间跑要当作阻断项,而不是噪声。这是一个值得单独讨论的仓库级暴露面;它不是扣住这个 PR 的理由,而且我审过 diff 的平台敏感性,没有发现任何一处。
  2. 测试计划的宿主侧那一半仍然没有录证。 第 2 步和第 5 步需要一次 Extension Development Host 会话。@qwen-code /verify 能就 web-shell 那一半与基线构建做 A/B 判定,并确认 Restore VS Code message edit and rewind after the WebShell cutover #9911 的 toast 真的会触发、而不是被门禁挡掉 —— 如果你想要最后那一点确定性,这值得做,而且成本很低。它够不到原生标签页。

如果六个月后这东西要由我来维护,我读一下事件名、读一下那两处守卫,就知道它做什么。注释恰好在「为什么」并不显而易见的三个地方解释了为什么 —— id-less 的关闭分支、来源门禁、以及更窄的那个 toast 守卫 —— 别处一句都没有。这就是本仓库的风格,也正是一个跨扩展边界的状态交接修复之所以还能被审查的原因。批准。✅


附记 —— 在这次审查撰写期间 main 前进了,本 PR 现在处于冲突状态。

main09:41:52Z 前进到 782c22bff531#1162809:42:09Z 合入;我的批准提交于 09:56:55Z。PR 的 head 没有动,所以我审查的代码就是我批准的代码,上面的判决依然成立。但 mergeable 现在是 CONFLICTINGmergeStateStatusDIRTY,并且有四个文件被双方同时改动:packages/vscode-ide-companion/src/webview/EmbeddedApp.tsxEmbeddedApp.test.tsx(经由 #11628 fix(vscode): localize rewind preflight errors),以及 packages/web-shell/client/App.tsxApp.test.tsx(经由 #11509 feat(web-shell): improve composer and context usage presentation)。

#11628 与本 PR 的 #9911 那一半是互补关系,不是重复。 我是查过才这么说的,而不是假设 —— 因为两者标题重叠、主题相同、而且是同一位作者。#11628 把 companion 里 rewind 预检的抛出点本地化了,并新增了一条 composer.editFailed 文案。它自己的描述说唯一的消费者是「web-shell 预检的 catch,它把错误 console.warn 出来然后取消提示」—— 我确认了在 782c22bff531main 上这仍然成立:两个 catch 点(App.tsx:9652:10423)都是 warn 后返回,没有 toast。所以现在预检会抛出四个已本地化的失败、涉及三条不同的文案(editUnavailableEmbeddedApp.tsx:1609editFailed:1618editExpired:1633editFailed:1641),而其中没有一条能到达用户眼前。展示的那一半仍然缺失,仍然要由本 PR 来落地。#9911 终究不需要移出去 —— 只是 main 先把它的另一半做掉了。

由此得出一处需要更正的地方:描述里说 composer.editUnavailable / composer.editExpired 是无人消费的那两条文案。现在是三条,其中多了 composer.editFailed。toast 会把它们全部呈现出来,这个行为是对的 —— 需要更新的只是那处枚举。

在这种情况下该如何看待这次批准。 main 上的 dismiss_stale_reviews_on_pushfalse,所以我在 dc665a60 上的 APPROVED 会挺过解决冲突的那次推送,而不会被它 dismiss。这个隐患值得明说:解决方案恰好落在 #9911 这条代码路径上,而 main 刚刚改动了这里的抛错契约,另外还涉及两个大测试文件。请把这次批准备读作只覆盖 dc665a60、不覆盖其后的任何东西,并在合并后的 head 上重跑 @qwen-code /triage。与其让这个绿色的勾被一次四文件冲突解决悄悄继承过去,我更愿意重新读一遍那次解决。

无论如何,合并并不卡在我这里。reviewDecision 已从 CHANGES_REQUESTED 变为 REVIEW_REQUIRED,所以来自 8b1ebb2076 的那道过期阻断已经解除 —— 但 ruleset 里的 require_code_owner_review 仍然需要一位拥有这些路径所有权的真人。

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

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

@yiliang114
yiliang114 requested a review from doudouOUC September 9, 2026 15:21

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

Round 9 re-review at 8b1ebb2

Scope: Static re-review of the same HEAD as round 8 (no new commits since then). severityFloor: auto → critical (round 9).

Standing Critical (R1-19): RESOLVED

The triage gate's template check (R1-19) is cleared. The PR body now carries all nine required headings — including ## Why it's needed, ### How to verify, ### Evidence (Before & After), ### Tested on with the macOS/Windows/Linux status table, and the <details><summary>中文说明</summary> block — updated at 14:52Z on 2026-09-09 after round 8 had already run. I confirm this by heading-by-heading comparison against .github/pull_request_template.md at the reviewed commit: each section named in the template is present. The two remaining CHANGES_REQUESTED reviews (5124755540, the template gate, and the round reviews behind it) should be dismissed by a maintainer so R1-19 stops re-arming on future rounds.

New Criticals: None found

The codebase is identical to round 7/8. I read the full diff (1705 lines across 13 files, +1382/−28) and examined every new code path in diff-manager.ts, extension.ts, EmbeddedApp.tsx, WebViewProvider.ts, App.tsx, and ToolGroup.tsx. No Critical-level issues were found — I concur with round 8's assessment.

Concurrence with round 8

Round 8 (qwen-code-ci-bot, CHANGES_REQUESTED) found no Criticals and reported 9 Suggestion-level findings, which are below the critical posting floor for round 9. I have verified the same production code and agree:

  • The new onDidClosePermissionDiff event path is additive and scoped correctly
  • The three must-not-fire guards (qwen.diff.accept/cancel blocked by !permissionRequestId; closeDiffEditor delete-before-close) are each pinned by tests
  • The hostOwnsEditDiffPreview state management resets correctly on both the request-change path and closeOpenPermissionDiffs

Round 8 deferred three decisions to me (doudouOUC):

  1. Dismiss stale CHANGES_REQUESTED reviews — the code findings in those reviews are not Criticals, the template gate is cleared, and a maintainer should dismiss them to stop the process loop.
  2. Host-side evidence before merge — the unit witnesses for the web-shell half are solid (tool-group unlocking, inline diff rendering, no reopen). The host-side behaviour (native tab staying closed, reopening on next request) has no automated witness because no lane in this repository drives a VS Code Extension Development Host. Whether that blocks merge is a maintainer judgement call; I do not consider it a review blocker given the test coverage and the additive nature of the change.
  3. #9911 toast riding along — this is a separate user-visible behaviour on a shared submit path. The toast is narrow (fires only for the session the submission belonged to) and is pinned by two tests including the session-switch case. I would keep it in the PR as a quality-of-life fix on the same code path; splitting it out is a judgement call for the author and maintainer.

Test coverage

The test-to-production ratio (~7.5:1) is appropriate for a state-handoff fix across an extension boundary. diff-manager.test.ts (18 passed), EmbeddedApp.test.tsx (31 passed), commands/index.test.ts (10 passed), ToolGroup.test.tsx (106 passed) each witness distinct paths. extension.test.ts (R3-7, R3-8) was not collected on this machine (missing core/acp-bridge builds) and remains CI's to confirm.

Verdict: No Criticals. R1-19 resolved. Concur with round 8.

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

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

Two-stage review summary for PR #11171

Round 1 (deepseek-v4-flash): no new Critical findings; one standing triage gate (R1-19) is resolved.
Round 2 (qwen3.8-max): no new Critical findings; four Suggestion-level items and two severity corrections are noted below.

Overall verdict: ISSUES_FOUND_R2 — blocker-free, but the second pass surfaces additional concerns the first pass missed.

Suggestions from round 2

  1. App.tsx:10247 — misleading comment for the load-bearing guard.
    The comment attributes the narrower submissionSessionIsCurrent() guard to composer-identity movement caused by submitting, but that mechanism does not exist on the submit path. The narrowing is genuinely load-bearing through sessionWriteBlocked = Boolean(connection.loadingTranscript) (App.tsx:3234). The code is correct; the comment should describe the actual conjunct or be removed.

  2. Toast tests only exercise the TRUE branch.
    App.test.tsx:20116 and :20142 render an unswitched session, so removing or forcing the guard true leaves both tests green. The suppression path (the guard's purpose) has no coverage.

  3. extension.ts:251try wraps the whole provider loop.
    A throw from an early provider silently skips later providers in the same fan-out. While consistent with qwen.diff.accept/cancel, the comment overstates the protection.

  4. Silent-drop log branches are unwitnessed.
    extension.ts:260 (empty provider list), extension.ts:271 (catch), and WebViewProvider.ts:2469 (no active webview) have no test coverage.

Corrections to the standing record

  • closeAll() never firing the dismissal is real code-wise but has no reachable failure path; treat as latent, not user-facing.
  • Ungated modeChanged revoking a dismissal is confirmed, but the test suite now witnesses the teardown as intentional behavior — this is a threat-model asymmetry rather than an oversight.

Already resolved since round 8

  • diff-manager.ts:407 "no test sends an id" — covered by diff-manager.test.ts:257/276.
  • R1-19 triage gate — PR body now carries every required heading.
  • Source gate and negative requestId cases are genuinely witnessed.

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Closeout sweep — pinning down the one remaining blocker, because it is now a one-click maintainer action rather than author work.

The sole standing Critical on this PR is R1-19, the PR-body template gate. It no longer stands.

The bot already ruled on this itself in triage stage 1 (comment, 2026-09-09T15:12:39Z): "The template gate that stopped this PR on 2026-09-06 (review 5124755540) is cleared: I checked the live body heading by heading against .github/pull_request_template.md at 8b1ebb2076, and all nine required sections are present plus a full paragraph-by-paragraph 中文说明 … It does not stand any more."

Re-verified independently at the same head 8b1ebb2076, rather than taking the ruling on trust — each of the five sections R1-19 named as absent is present exactly once:

R1-19 claim Live body at 8b1ebb2076
## Why it's needed absent present (motivation no longer folded into ## What this PR does)
### How to verify absent present, as its own heading under ## Reviewer Test Plan
### Evidence (Before & After) absent present
### Tested on absent present, with a populated macOS / Windows / Linux table
<details><summary>中文说明</summary> absent present

All nine template headings plus the optional ### Environment (optional) are there.

Why reviewDecision is still CHANGES_REQUESTED anyway — a timeline artefact, not an open defect:

  • Blocking review 5154433504qwen-code-ci-bot, CHANGES_REQUESTED, submitted 2026-09-09T12:49:01Z at commit 8b1ebb20766a.
  • The body was restructured at 2026-09-09T14:52:18Z2h03m after that review was submitted.
  • So the blocking review is at the live head, but it judged the pre-restructure body. Its one standing Critical was answered by an edit that landed after it.

What is needed: a maintainer to dismiss review 5154433504. /triage cannot do it — that review is at the current head, and the triage fired at 14:47:36Z already ran to completion (stages 1–3 posted at 15:12:39–41Z) without submitting a superseding verdict review.

Deliberately not firing either bot command again:

  • Not /triage — it already ran against this exact head and this exact body, and confirmed the gate cleared. A re-run cannot supersede a review that sits at the same head.
  • Not /review — the round-5 ledger records that the new-finding rate on this PR was not falling (posted:2 fresh:2 prevPosted:1). A fresh full-diff scan would mint findings on already-reviewed code rather than close this out.

One genuine gap that remains, and it is not the body gate: the ### Tested on table honestly marks macOS and Windows as ⚠️ not tested. The bot's own stage 3 put it exactly right — "Confidence: 3/5 — the code is clean and I would maintain it without cursing anyone; what a human needs to second is the evidence, not the implementation." The Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) lanes were skipped in CI at this commit, and for a VS Code companion change shipping to all three platforms that confirmation is owed before merge. That needs a CI lane or a human with the platform, not another review round.

Resolve diff-manager closeDiff conflict: keep main's resolvedFilePath exact-form matching and add the PR's permission-diff dismissal via openDiff.permissionRequestId; adapt the refactored test mock (vi.hoisted + missing EventEmitter.dispose / openTextDocument default) to the PR's disposal coverage.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-conflict/jmtvxsd4xwa

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

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

  • R1-1 inert new commands/index.test.ts cases - already reported (comment 3944217715)
  • R1-5 duplicated permission-block fixture and permissionBlock name collision - already reported (comment 3944217727)
  • R1-10 per-provider drop-point log fires on success and is mis-tagged [Extension] - already reported (comment 3944217736)
  • R1-15 both new toast guards witnessed only on the true branch - already reported (comment 3944217749)
  • R1-16 ParallelAgentsGroup conditional has no witness for the newly dynamic prop - already reported (comment 3944217755)
  • R1-17 sub-agent control render passes the same flag value as the case render - already reported (comment 3944217759)
  • D3-1 hand-back witness never flips the prop on a mounted row - already reported (round-3 deferred list, review 5131595846)
  • D4-1 the two preflight toast gates disagree on identical input - already reported (round-4 deferred list, review 5135150418)
  • D5-1 dispose() has no production caller, so the added line and its witness cover a dead path - already reported (round-5 deferred list, review 5135780307)
  • D6-1 the newest-snapshot reduce arm of prepareSubmit is untested - already reported (round-6 deferred list, review 5140664973)

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally; this review built and tested on Linux only, which matters for a VS Code companion change that ships to three platforms and because the round-2 Critical R2-1 was a Windows-only test failure no Linux lane could catch.

Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally; this review built and tested on Linux only.

Not reviewed: verifier probes — qwen review scratch-tree was unavailable (the repository's local git config carries includeIf entries pointing at a missing git-credentials file); verifiers hand-rolled isolation in /tmp copies and in-memory vitest mutants and confirmed the shared worktree stayed clean, but two findings (T9-4, T9-6) could not be probed at all and rest on reading.

Not explored to full depth (tool budget reached): "agent 1d": none — I did not run vitest or tsc over the touched packages (static scan only, per my dimension); test-execution evidence belongs to the walk/verifier agen…; "agent reverse-audit (round 5)": I did not walk WebViewProvider.webShellPermissionOwners registration and teardown ( :205-215 , :940-960 , :1940-1975 ) against the new dismissal routing end…; "agent reverse-audit (round 5)": I did not read the modeChanged handler in EmbeddedApp.tsx that the new R5-3 test drives, so the teardown-reset assertions in that test are unverified agains…; "agent reverse-audit (round 5)": I ran no test suite and no check-types ; every claim above is from reading source at the reviewed state, not from execution.; "agent reverse-audit (round 4)": did not execute the web-shell or vscode-ide-companion suites, so the t('composer.editExpired') → 'The original message can no longer be edited.' string the ….

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

Test Plan (not a blocker): src/diff-manager.test.tsno such file or directory; src/webview/EmbeddedApp.test.tsxno such file or directory; src/commands/index.test.tsno such file or directory; client/App.test.tsxno such file or directory; client/components/messages/ToolGroup.test.tsxno such file or directory; and 6 more.

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

  • packages/vscode-ide-companion/src/commands/index.test.ts:277 — [review] the openDiff wire-key to permissionRequestId hop (FileMessageHandler.handleOpenDiff) has no test
  • packages/web-shell/client/App.tsx:10427 — [review] the queued-path catch wraps enqueuePreparedPrompt, so a post-enqueue host failure toasts 'not submitted'
  • packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx:1158 — [review] the two new describes re-derive latestProps()/dismiss() helpers the file already has
  • packages/vscode-ide-companion/src/diff-manager.test.ts:489 — [review] four byte-identical createManager() copies plus two identical new beforeEach blocks

Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)

中文说明

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

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

未审查(原文为英文):build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally; this review built and tested on Linux only, which matters for a VS Code companion change that ships to three platforms and because the round-2 Critical R2-1 was a Windows-only test failure no Linux lane could catch.

未审查(原文为英文):build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally; this review built and tested on Linux only.

未审查(原文为英文):verifier probes — qwen review scratch-tree was unavailable (the repository's local git config carries includeIf entries pointing at a missing git-credentials file); verifiers hand-rolled isolation in /tmp copies and in-memory vitest mutants and confirmed the shared worktree stayed clean, but two findings (T9-4, T9-6) could not be probed at all and rest on reading.

未探索到全部深度(达到工具调用预算):"agent 1d"none — I did not run vitest or tsc over the touched packages (static scan only, per my dimension); test-execution evidence belongs to the walk/verifier agen…"agent reverse-audit (round 5)"I did not walk WebViewProvider.webShellPermissionOwners registration and teardown ( :205-215 , :940-960 , :1940-1975 ) against the new dismissal routing end…"agent reverse-audit (round 5)"I did not read the modeChanged handler in EmbeddedApp.tsx that the new R5-3 test drives, so the teardown-reset assertions in that test are unverified agains…"agent reverse-audit (round 5)"I ran no test suite and no check-types ; every claim above is from reading source at the reviewed state, not from execution."agent reverse-audit (round 4)"did not execute the web-shell or vscode-ide-companion suites, so the t('composer.editExpired') → 'The original message can no longer be edited.' string the …

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

Test Plan(非阻断):src/diff-manager.test.tsno such file or directory; src/webview/EmbeddedApp.test.tsxno such file or directory; src/commands/index.test.tsno such file or directory; client/App.test.tsxno such file or directory; client/components/messages/ToolGroup.test.tsxno such file or directory; and 6 more。

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

机制健康:本轮未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff,除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上;并会一直如此,直到某一轮的标记重新带上锚点,或落地的嫁接能被运行该轮的评审使用。(仅陈述,不据此行动——这不改变本轮发布的任何内容。)

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

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Closeout follow-up. Two things changed since my 2026-09-10 sweep, and one of them removes the reason I gave for not re-running /triage.

The blocking review is no longer at head. Review 5154433504 was submitted at 8b1ebb20766a; head is now dc665a602f68 (Merge origin/main, 2026-09-10T19:58:08Z). My previous argument — that a re-run cannot supersede a review sitting at the same head — stopped applying when that merge landed. A fresh verdict at the current head can now supersede it, so I am re-triggering triage rather than asking for a manual dismissal.

Current state at dc665a602f68: 0 unresolved review threads, mergeable: MERGEABLE, and every non-skipped check green (15 success, 27 skipped, 0 failure).

macOS evidence, since the ### Tested on table marked it ⚠️ and the Test (macos-latest, Node 22.x) lane is skipped at this commit. Ran the PR's five vscode-ide-companion test files locally on macOS arm64 / Node 22 at dc665a602f68:

cd packages/vscode-ide-companion && npx vitest run src/commands/index.test.ts src/diff-manager.test.ts src/extension.test.ts src/webview/EmbeddedApp.test.tsx src/webview/providers/WebViewProvider.test.ts
Test Files  5 passed (5)
     Tests  163 passed (163)

That covers the VS Code companion half — the platform-sensitive part of this change. The two web-shell files were not run locally: packages/web-shell now depends on @datafe-open/markdown-chart, which is declared at this head but absent from the shared local node_modules, so client/App.test.tsx and client/components/messages/ToolGroup.test.tsx fail to resolve an import before collecting. That is an install-state gap in my checkout, not a defect — both files pass in the green Test (ubuntu-latest, Node 22.x) lane at this commit. Windows remains unverified for the same reason as before: the lane is skipped and I do not have the platform.

Nothing in the diff changed. No new commits from me.

@yiliang114

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ❌ not passed — findings reported (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: 55 passed · 0 failed · 55 total

Flakiness gate: ⚠️ timeout — only 4 of 5 rounds fit the 15-minute budget; the completed rounds agreed

中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)

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

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

抖动门:⚠️ timeout — only 4 of 5 rounds fit the 15-minute budget; the completed rounds agreed

Verification report

PR #11171 — deep verification (follow-up round 3)

Verdict: findings — assertions 55 pass / 0 fail / 55 total. Verified head: dc665a602f686b935e080479bdeec3b7b2d83f16 (git rev-parse HEAD^2); base tip ae78d5b800a275b526d3e707ce3112926d34b156 (HEAD^1). Previous round verified 8b1ebb20766aea307d8761b7e28e3ab617f0bf52. No executed assertion failed; the verdict is findings because carried-forward coverage gaps F1/F2/F3/F5 all still stand and one of them (F1) is wider at this head than last round.

中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)· 后续轮 3

本轮验证于 dc665a60,上一轮验证于 8b1ebb20自上一轮以来 PR 自身没有新增任何代码:commit 20 是一次纯粹的 Merge origin/main(用 git cat-file -p dc665a60 读出真实父提交 8b1ebb20 + c46cb85c;浅克隆 graft 让 rev-list --parents 看不到它们)。因此本轮的重点是合并保真度与合并交互,而不是新功能。

  • 核心行为仍承重:三臂 A/B 全部本轮实测重跑(见「A/B 承重证明」与 01-ab-base-vs-head.png)。ARM0 base+base:companion 131 绿 / web-shell 988 绿;ARM1 base+HEAD 测试:companion 12 红 | 151 绿 (163)、web-shell 3 红 | 990 绿 (993);ARM2 head+HEAD:companion 163 绿、web-shell 993 绿。15 条测试由红转绿,红侧断言报的是具体值而非 import 错误。
  • 合并保真(新):PR 的净贡献对 c46cb85c 与对 ae78d5b8 两个 base 完全一致——内容行(+/-)排序后 sha256 相同(f3617609…),13 文件同为 1345 insertions, 9 deletions,13 个文件均无冲突标记,App.tsx 仅整体位移 +8 行。本轮 HEAD 本身就是合入当前 base tip 的 merge,上一轮「未做 trial merge」这一缺口已闭合
  • 合并交互(新,最高价值探针):main 在 diff-manager.ts 落地了 resolvedFilePath/originalFilePath 拆分(resolveWorkspacePath),恰好就在本 PR 关闭通知钩子所依赖的那段匹配逻辑下面。实测两点:(1) cancelDiffURI 字符串为键(diffDocuments.get(rightDocUri.toString())),不受路径重构影响,所以用户 ✕ 的触发链未被合并破坏;(2) 新增 6 条路径形态 sibling 探针(相对/绝对/./ 点号/跨形态开关/绑定 id 静默/兄弟文件不受影响)全部通过。该风险经测量被证伪,不是缺陷。
  • 上一轮发现状态:F1 仍成立且变宽(A1 存活 884/884;新增 A2 探针显示立即提交路径的同类 guard 同样无测试钉住,884/884);F2 仍成立(X3 存活 163/163,同文件正控 X4 被杀);F3 仍成立(X1 存活 163/163);F4 仍成立且确为有意(T1 被杀);F5 仍成立(D3/D4 均存活 884/884;App.test.tsxhostOwnsEditDiffPreview: falsehasDiffPreview 各出现 0 次)。更正 C1、C2 复测后仍然成立,并新增 C3。
  • 本轮新增正面结论:E0 突变证明 permissionDiffClosediframe 来源门 event.source === window.parent 有测试钉住(红:ignores a dismissal posted by a nested iframe window);E1 证明「不重开」合取项也有钉住(expected […] to have a length of 1 but got 2),因此 E3 组合行冗余——两层防御各自单独可杀,不存在被互相遮蔽的层次。
  • 突变矩阵:23 行,KILLED=15 / SURVIVED=8,预期不符=0,每行 mutated=y restored=y(见 02-mutation-matrix.png)。
  • 门禁tsc --noEmit 两包均 exit 0;eslint 对 13 个变更文件 exit 0,并用植入违规(debugger / 未使用局部量 / var)证明门禁有效——报出 4 个错误、exit 1,随后按 sha256 还原。companion 全包 573 例中 5 红,全部位于 src/ide-server.test.ts(PR 未触碰),且在 base 上失败测试名逐字节相同,属 base tip 既有失败。
  • 未覆盖:真实 VS Code / 真实 TUI;playwright e2e;逐 commit 归因(浅克隆);macOS/Windows;F1/F2/F5 的候选修复未编写未实测;上一轮的 wire-handoff 双进程 harness 本轮未重建。

Previous-finding status (all re-measured at dc665a60; nothing carried by hash shortcut)

The input closure moved on both sides — base tip e09c461aae78d5b8, head 8b1ebb20dc665a60, and main rewrote 11 of the 13 changed files in between — so the proven-identical-closure shortcut does not apply to any row. Every measurement below was re-run against rebuilt/re-read code at this head.

# finding severity status at new head evidence this round
F1 the narrow queued-toast guard (submissionSessionIsCurrent) is unpinned medium stands, and is wider A1 (swap in the full submissionOwnerIsCurrent) SURVIVED 0 failed | 884 passed; in-file control A3 (delete the toast) KILLED 1 failed | 883 passed, so the harness demonstrably can make this file red. New: sibling A2 on the immediate path (admissionOwnerIsCurrent()admissionSourceIsCurrent()) also SURVIVED 884/884, with its own control A4 KILLED — so the gap covers both toast sites, not one
F2 the user's ✕ (onDidCloseTextDocumentcancelDiff) is unwitnessed medium stands X3 (gut the subscription body) SURVIVED 0 failed | 163 passed; in-file control X4 (delete the fan-out loop) KILLED 1 failed | 162 passed, red = forwards a closed permission diff to every permission-aware provider, assertion expected "spy" to be called with arguments: [ 'req-1' ]
F3 the dismissal fan-out's try/catch is unpinned low stands X1 (catch → throw err) SURVIVED 163/163
F4 shouldAutoExpand widened to a bare write; real, intentional, tested, absent from the description low stands T1 (revert to the two base lines) KILLED by hands a pending bare-write row back to the shell already expanded, assertion expected null not to be null; isEditToolName matches edit|editfile|write|write_file|writefile
F5 the request-id gate's two stateful conjuncts are unpinned medium-low stands D3 (delete !hostOwnsEditDiffPreview) SURVIVED 884/884; D4 (delete request.hasDiffPreview !== true) SURVIVED 884/884. Census re-run: in App.test.tsx, hostOwnsEditDiffPreview: false occurs 0 times (: true occurs 2), hasDiffPreview occurs 0 times. D1 (delete request.id !== requestId) KILLED, so the gate itself is live
X2 empty-registry log + early return unpinned stands, correctly SURVIVED 163/163; observability-only — an empty for loop is the same behaviour
W1 notifyPermissionDiffClosed's no-active-webview guard unpinned stands, correctly SURVIVED 163/163; in-file control W2 (rename the message type) KILLED, so the file is collected and the survivor is real
C1 correction: the code comment's stated mechanism for the narrow guard is wrong stands re-censused: the 6 composerSourceVersionRef.current += 1 writers are at App.tsx 3599, 12302, 12421, 12605, 13031, 17217 — every one inside a session/workspace-switch context (primaryCwd selection, setPendingSessionContext, selectedWorkspaceCwdRef assignment, standalone-context reset). None is on the submit path
C2 correction: test counts in the description are stale stands, wider see C2 below
R3-14 declined by the author as a behaviour decision recorded on the issue declined-with-rationale; I agree unchanged; not re-litigated

The previous round's two candidate fixes (App.guard.test.tsx for F1, extension.trigger.test.ts for F2) were not re-applied; both findings stand on their own mutations. Recorded under Not covered.

Central claim and A/B load-bearing proof

Central claim. Closing a host-owned permission diff by hand (✕, no vote) hands the edit preview back to the web shell: the tool row unlocks, renders the diff inline, and the tab does not spring back open.

Secondary claims. (a) voting from the diff editor must not trigger the dismissal; (b) a web-shell-initiated closeDiff must not echo back as a dismissal; (c) the #9911 preflight-rejection toast fires only while the user is still on the submission's session.

arm code tests companion web-shell oracle
ARM 0 (A/A control) base ae78d5b8 base tests 131 passed (5 files) 988 passed (2 files) both trees healthy; the A/B is not an environment artifact
ARM 1 (control) base ae78d5b8 HEAD tests 12 failed | 151 passed (163) 3 failed | 990 passed (993) the new behaviour is absent at base; failures name values, not import errors
ARM 2 (head) head dc665a60 HEAD tests 163 passed (5 files) 993 passed (2 files) the fix restores every flipped test

15 tests flip red → green between ARM 1 and ARM 2. Witness: evidence/01-ab-base-vs-head.png — the two decisive companion cells re-run live for the capture, with the four remaining cells printed from their saved logs and each cell labelled [LIVE] or [from saved log] so provenance is visible per row. Raw logs: raw/ab-arm0-companion-base-base.log, raw/ab-arm1-companion-base-headtests.log, raw/ab-arm2-companion-head-headtests.log, and the three web-shell equivalents.

The 12 companion flips are exactly the dismissal chain — 7 in diff-manager.test.ts, 3 in EmbeddedApp.test.tsx, 1 in extension.test.ts, 1 in WebViewProvider.test.ts — the same breakdown as the previous round at a larger suite. The 3 web-shell flips are surfaces a rejected preparation instead of cancelling silently and surfaces a queued preparation rejection instead of cancelling silently (both expected "spy" to be called with arguments: [ 'error', …(1) ] — the pushToast) plus hands a pending bare-write row back to the shell already expanded (expected null not to be null).

Control hygiene. One scratch git worktree at HEAD^1 under tmp/, removed after the cells were captured. The PR changes no dependency manifest, so the base tree shares the head's node_modules (root plus both per-package dirs, symlinked). That leaves @qwen-code/web-shell resolving into the head tree (readlink -f from inside the base worktree → /__w/qwen-code/qwen-code/packages/web-shell), which would normally be a confound because the PR changes web-shell. It is inert here, and I verified why rather than assuming it:

  • the companion's EmbeddedApp.test.tsx replaces @qwen-code/web-shell with a total vi.mock factory (no importOriginal), so the real module never executes;
  • web-shell's own vitest.config.ts aliases @qwen-code/web-shell/daemon-react-sdk to ./client/daemon-react-sdk.ts, i.e. to the base tree's own source via __dirname, and App.test.tsx additionally mocks that specifier totally — so no head dist is loaded either. (This is why last round's base web-shell lib build was unnecessary this round.)
  • @qwen-code/sdk does resolve to the head build; the PR does not touch packages/sdk-typescript and changes no manifest, so it is identical on both arms.

Base production files were asserted byte-identical to HEAD^1 by sha256 (all 6: diff-manager.ts, extension.ts, EmbeddedApp.tsx, WebViewProvider.ts, App.tsx, ToolGroup.tsx), and the 7 head test files copied into the base tree were asserted byte-identical to head (raw/control-hygiene-sha.txt).

Merge fidelity and merge interaction (new this round)

Commit 20 is a merge, so the round's real question is not "does the new code work" but "did the merge preserve the old code, and does it still work on the newer base it landed on".

Fidelity — proven. git cat-file -p dc665a60 shows parents 8b1ebb20 (last round's verified head) and c46cb85c (the snapshot's baseRefOid); both are hidden from rev-list --parents because dc665a60 is a shallow graft, which is why this needed cat-file rather than the usual rev syntax. The PR's net contribution is identical against both main OIDs: 13 files changed, 1345 insertions(+), 9 deletions(-) for c46cb85c..dc665a60 and for HEAD^1..HEAD, and the sorted multiset of all 1354 content lines (+/-, headers and context excluded) has the same sha256 f3617609db940cafb8f94a77a1d64da81265ffc3db04e270f9ba3877a54c93f8 on both sides. Only hunk offsets moved — uniformly +8 across all five App.tsx hunks (9644→9652, 9838→9846, 10376→10389, 10415→10445, 10430→10473), which is what a clean re-application looks like. No conflict markers in any of the 13 files at HEAD. Because HEAD is the merge into the current base tip, this also closes last round's "no trial merge into current main" gap: the merge is conflict-free and content-preserving.

Interaction — probed, risk disproved. Between the previous merge point and c46cb85c, main landed a resolvedFilePath / originalFilePath split in diff-manager.ts (resolveWorkspacePath, utils/file-path.ts), rewriting exactly the closeDiff matching that this PR's id-less-close dismissal hook reads, plus hasExistingDiff and onActiveEditorChange. Two things could have broken silently:

  1. Could the ✕ trigger stop finding its entry? No. cancelDiff looks up by URI stringthis.diffDocuments.get(rightDocUri.toString()) — not by path, so main's path-key refactor cannot reach it. (This is a static fact about the merged code, not an inference from the diff.)
  2. Could the id-less close stop matching across path forms? Probed with 6 new siblings (diff-manager.mergesibling.test.ts in this artifact dir, appended to a copy of the shipped suite; file total 35 passed, 0 red), all with workspaceFolders = [{ uri: { fsPath: '/test/ws' } }] — a configuration the shipped dismissal suite never sets, since it uses absolute /workspace/... paths throughout: relative open + relative close fires once with req-1; relative open + absolute close fires; absolute open + relative close fires; relative open + ./foo.ts dotted close fires; an id-bound close stays quiet for every form; and a sibling bar.ts request is untouched when foo.ts closes (toHaveBeenCalledTimes(1), id req-1). All green at head.

So the merge is faithful and behaviour-preserving on the newer base. This is the measurement that a two-cell A/B against base cannot supply, because both arms would carry the same main-side refactor.

Corrections to the description

  • C1 stands. The comment this PR adds above submissionSessionIsCurrent says the full guard "also tracks composer identity, and submitting is itself what moves that — so reusing it here would suppress the very message the user needs". The census says otherwise: all six composerSourceVersionRef.current += 1 writers are session/workspace-switch events, none on the submit path. Corroborating evidence from this round's matrix: the immediate path's toast is guarded by admissionOwnerIsCurrent(), which does include composerSourceVersionRef.current === admissionSource.sourceVersion (App.tsx:9614), and A4 (deleting that toast) is KILLED by surfaces a rejected preparation instead of cancelling silently — i.e. that test passes at head with the composer-version conjunct in the guard, so submitting does not move composer identity in a way that suppresses a toast. The refactor composing submissionOwnerIsCurrent on submissionSessionIsCurrent removes the drift risk the comment worries about; it does not fix the coverage gap F1 names, and the stated mechanism is still not the operative one.

  • C2 stands and widened further. Every count in the body's "Run:" list is stale at this head:

    body claims measured at dc665a60
    diff-manager.test.ts 18 passed 29
    EmbeddedApp.test.tsx 31 passed 33
    ToolGroup.test.tsx 106 passed 109
    App.test.tsx 746 passed 884
    extension.test.ts "Not run — cannot be collected here" collected and passing: 18 tests
  • C3 (new). The body's "Not run: src/extension.test.ts@qwen-code/qwen-code-core has no build on this machine, so the file cannot be collected here" is a statement about the author's machine, but it reads as a coverage limitation of the change. In this container the file collects and runs (18 tests, green at head), and it is precisely where F2's control X4 lands. The dismissal fan-out in extension.ts is therefore not uncollected — it is collected, green, and still unwitnessed for the ✕ trigger. Worth correcting so a reader does not discount F2 as "that file was never run".

Findings

F1 (carried, medium, wider this round) — both preflight-toast guards are unpinned

node tmp/mut-run.mjs tmp/mutations.json A1   # queued guard -> full submissionOwnerIsCurrent  -> SURVIVED 0F|884P
node tmp/mut-run.mjs tmp/mutations.json A2   # immediate guard -> admissionSourceIsCurrent    -> SURVIVED 0F|884P
node tmp/mut-run.mjs tmp/mutations.json A3   # CONTROL: delete the queued toast               -> KILLED   1F|883P
node tmp/mut-run.mjs tmp/mutations.json A4   # CONTROL: delete the immediate toast            -> KILLED   1F|883P

A1 is the carried finding: the shipped queued test sets streamingState='responding' but never write-blocks the session, so the narrow and full guards evaluate identically there and nothing distinguishes them. A2 is new: the immediate path has the same shape — admissionOwnerIsCurrent() versus the fuller admissionSourceIsCurrent() (which adds !sessionWriteBlockedRef.current and the write-block-generation match) — and swapping in the fuller guard also leaves all 884 green. So the untested axis is "a toast the user should have seen was suppressed because the session was write-blocked mid-flight", and it is untested on both submission paths, not one. The two controls A3/A4 land in the same file as their mutants and both go red, so this is a real coverage gap rather than a harness that never collected App.test.tsx.

Classification: coverage gap, not a defect — the guards are correct as written.

Suggested fix (candidate, not applied, not measured)

One test per path in App.test.tsx: drive a preflight rejection while sessionWriteBlockedRef is set (or the write-block generation has advanced) between submission and rejection, and assert pushToast was not called — then the same scenario without the write block, asserting it was. Per the vacuity rule this ships with its mutations: A1 and A2 must both go SURVIVED → KILLED. Neither was applied this round, so the fixture that would pin this axis is named but not written.

F2 (carried, medium) — the user's ✕ is still unwitnessed

X3 SURVIVED 0 failed | 163 passed. The whole dismissal chain hangs on the pre-existing onDidCloseTextDocumentcancelDiff(doc.uri) subscription at extension.ts:242; every shipped test calls cancelDiff directly. Control X4 KILLED (1 failed \| 162 passed), which is what makes the survivor credible. Pre-existing code, so the author is not blamed — but this PR is what makes the subscription load-bearing. Note the merged-code fact that bounds it: cancelDiff is URI-keyed, so main's path refactor cannot silently break this hop (see Merge interaction).

F5 (carried, medium-low) — the gate's two stateful conjuncts are still unpinned

D3 and D4 both SURVIVED 884/884; census unchanged (0 occurrences of hostOwnsEditDiffPreview: false and of hasDiffPreview in App.test.tsx). Both conjuncts are live in production — hostOwnsEditDiffPreview is exactly the value this PR turns from a constant true into state, and it is plumbed App.tsx:2970 (default false) → App.tsx:3249 customization → ToolGroup. The sharp case is unchanged: after a hand-back the flag is false, so the gate correctly refuses a host-relayed vote; if a future refactor dropped that conjunct, a stale native vote would resolve an approval the host can no longer display and all 884 tests would stay green. Coverage gap, not a defect.

F3 (carried, low) — the fan-out try/catch is still unpinned

X1 SURVIVED 163/163. Mirrors the pre-existing vote fan-outs; a pre-existing pattern, not a PR-introduced hazard.

F4 (carried, low) — the bare-write auto-expand widening is real and pinned, but absent from the description

T1 KILLED by hands a pending bare-write row back to the shell already expanded. Deliberate and load-bearing for the hand-back; still a user-visible change for every web-shell host that the description never mentions. Note, not a defect.

Positive results worth recording (new this round)

  • The iframe source gate is pinned. E0 (delete && event.source === window.parent from the permissionDiffClosed branch) is KILLED by ignores a dismissal posted by a nested iframe window (expected false to be true). This is the guard that stops a scriptable sandboxed MCP-app iframe inside the webview from flipping who owns the edit preview. It has a witness; I probed it because an unpinned security guard would have been the sharpest finding available here, and it is not one.
  • No hidden layered guard. E1 (delete the dismissedPermissionDiffIdRef.current !== pendingPermission.requestId do-not-reopen conjunct) is KILLED with expected [ { type: 'openDiff', …(1) }, …(1) ] to have a length of 1 but got 2 — the tab springs back open, which is the exact bug the PR exists to fix. E2 (revert hostOwnsEditDiffPreview to the constant) is KILLED with 3 reds. Since both halves die alone, the E3 combination row (both reverted together, 3 reds) adds no new information: this is not a defence-in-depth case where singles mask each other, and I report that explicitly so the combination row is not read as the only thing that caught it.
  • Teardown and next-request reclaim are pinned. E4 KILLED by returns preview ownership to the host when the pending diffs are torn down; E5 KILLED by takes the preview back for the next permission request.
  • The diff-manager guards are pinned. M1 (drop permissionRequestId === undefined) KILLED by does not echo a close the chat surface asked for; M2 (drop the diffInfo.permissionRequestId guard) KILLED by stays quiet for a diff that no approval is waiting on; M3 (delete the emitter .dispose()) KILLED by stops notifying once the manager is disposed. All three with expected "spy" to not be called at all, but actually been called 1 times.

Full matrix: raw/mutation-matrix.txt, witness evidence/02-mutation-matrix.png, per-row JSON in raw/mut-<id>.json and logs in raw/mut-<id>.log. 23 rows, KILLED=15, SURVIVED=8, expectations-not-met=0. Every survivor is classified above as a coverage gap or observability-only; none is dead code and none is a defect.

Pre-existing base failures, attributed

The whole companion package at head is 5 failed | 567 passed | 1 skipped (573), 1 failed | 44 passed (45 files). All 5 reds are in src/ide-server.test.ts, which the PR does not touch. An A/A control ran that file at base: 5 failed | 7 passed | 1 skipped (13), exit 1, and after ANSI-stripping, the five failing test names are byte-identical on both arms (should set environment variables and workspace path on start with multiple folders, should set a single folder path, should set an empty string if no folders are open, should update the path when workspace folders change, should clear env vars and delete lock file on stop). Delta attributable to the PR: +0 failing. Logs: raw/gate-companion-head.log, raw/aa-ide-server-base.log.

Not covered

  • F1/F2/F5 candidate fixes not written or measured. The suggested fixtures above are named, not applied; per the unpinned-axis rule, a suite that is green with and without a candidate fix proves nothing, so none is claimed as evidence.
  • The previous round's two-process wire-handoff harness was not rebuilt (real WebViewProvider.notifyPermissionDiffClosed → JSON round-trip → real EmbeddedApp MessageEvent). The property is pinned transitively — W2 is KILLED by relays a permission diff dismissal to the webview under requestId, and E0/E1/E2 pin the consumer's handling of that payload — but structured-clone fidelity of the transport is not re-proven this round.
  • Real VS Code Extension Development Host and real TUI. Every harness drives the shipped fake vscode boundary or a synthetic MessageEvent. Reviewer Test Plan steps that require a native tab to stay closed, or to reopen on the next request, remain host-side and unrecorded.
  • Playwright e2e (test:e2e*) not run. Repo-wide suite not run — only the two affected workspaces. The whole-package companion run is reported above with its pre-existing failures attributed; the whole-package web-shell run was not done (only the two changed files).
  • Per-commit attribution. Depth-2 checkout: git rev-list HEAD^1..HEAD^2 returns 1 locally reachable commit while the snapshot lists 20, and git rev-parse --is-shallow-repository is true. All results are for the aggregate HEAD^1..HEAD diff. (The merge's own parents were recoverable via git cat-file -p, which is how the delta was scoped — but the 19 earlier commits are not individually exercisable.)
  • macOS / Windows. Not executed here. resolveWorkspacePath uses vscode.Uri.joinPath(...).fsPath and path.win32.isAbsolute, and my sibling sweep's fake joinPath is a plain ${base.fsPath}/${filePath} string join — so the path-form siblings prove the matching logic, not Windows separator behaviour. That remains CI's to confirm.
  • A harness incident of mine, disclosed. A tool-level timeout killed the mutation runner mid-D4, leaving App.tsx mutated on disk. I detected it via git status (not by assuming the restore had run), restored with git checkout --, and sha256-verified the file back to HEAD (a9ac27e8…) before continuing. One eslint run had already executed during that window; it was discarded and the gate re-run on the verified-clean tree, and the number reported above is from the re-run. tmp/mut-run.mjs now restores in a finally and on SIGINT/SIGTERM so an interrupt cannot leave residue. D4 was re-run to completion in the background afterwards and is reported from that clean run.
  • The first eslint liveness control was inconclusive and is not counted. I planted an unused private readonly class field; eslint exited 0 and reported nothing, so that violation class is not owned by this config. I re-planted debugger; + an unused local + var, which eslint reported as 4 errors with exit 1 (raw/eslint-planted2.log), and only that run is cited as proving the gate live. The failed first attempt is preserved as raw/eslint-planted.log.

Methodology

Environment: the CI verify container (node v22.23.2, no GitHub token), tree = refs/pull/11171/merge at depth 2 with npm ci + npm run build pre-existing at HEAD. The delta was scoped first, from git cat-file -p on the grafted head, which showed commit 20 to be a pure merge of origin/main onto the previously verified head — that is what made merge fidelity and merge interaction, rather than new-code correctness, the round's central questions. The A/B used one scratch git worktree at HEAD^1 under tmp/ with node_modules symlinked from the head tree; the @qwen-code/web-shell link pointing into head was proven inert by showing both consumers mock it totally and that web-shell's own vitest aliases daemon-react-sdk to base source, and base production files were sha256-asserted equal to HEAD^1; the worktree was removed after the cells were captured. Merge fidelity was measured as a sorted-multiset sha256 over the 1354 +/- content lines of the PR's diff against two different main OIDs, plus a per-hunk offset comparison and a conflict-marker scan. Harnesses drove real production code with only the external vscode API faked: the real DiffManager, real activate() listener, real WebViewProvider, real EmbeddedApp render, real web-shell ToolGroup and App. The mutation runner (tmp/mut-run.mjs, mutations in tmp/mutations.json) applies one or more exact-string edits, asserts each anchor occurs exactly once, runs the named suite, records exit/status/failing-test/assertion lines, then restores in a finally and re-verifies by sha256; classification requires a KILLED row to name a failing test, so a collection error cannot masquerade as a kill. Gates: vitest per affected workspace, tsc --noEmit per package (both exit 0), eslint over all 13 changed files (exit 0) with a planted-violation liveness control that was reported and then restored by sha256. The fail: 0 in assertions.json is honest — no executed assertion failed, and ARM 1's reds are encoded as expected control outcomes, so they count as passes; the verdict is findings because F1 (widened), F2, F3 and F5 are concrete, reviewer-relevant coverage gaps and C1/C2/C3 are description corrections. The 55 counted assertions are: 6 A/B arm outcomes; 13 control-hygiene sha256 identities (7 test copies + 6 base production files); 2 merge-fidelity checks (content-line sha256 identity, conflict-marker scan); 1 A/A ide-server failing-name identity; 20 mutation rows whose recorded status matched a stated expectation (X1–X4, W1, W2, M1–M3, E2–E5, A1, A3, A4, T1, D1, D3, D4); 6 sibling probes S1–S6; 4 gates (two tsc --noEmit, eslint clean, eslint liveness); and 3 census checks (hostOwnsEditDiffPreview: false = 0, hasDiffPreview = 0, six composer-version writers all off the submit path). The E0, E1, A2 and E3 mutation runs were probes with no prior expectation, so they are reported as evidence rather than counted as pass/fail assertions.

Flakiness gate log

rounds=5 files=7 skipped=0
file packages/vscode-ide-companion/src/commands/index.test.ts: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/commands/index.test.ts
file packages/vscode-ide-companion/src/diff-manager.test.ts: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/diff-manager.test.ts
file packages/vscode-ide-companion/src/extension.test.ts: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/extension.test.ts
file packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/webview/EmbeddedApp.test.tsx
file packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/webview/providers/WebViewProvider.test.ts
file packages/web-shell/client/App.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/App.test.tsx
file packages/web-shell/client/components/messages/ToolGroup.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/components/messages/ToolGroup.test.tsx


per-file results (P=pass F=fail I=infra-exit, one letter per run):
  packages/vscode-ide-companion/src/commands/index.test.ts: PPPPP
  packages/vscode-ide-companion/src/diff-manager.test.ts: PPPPP
  packages/vscode-ide-companion/src/extension.test.ts: PPPPP
  packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: PPPPP
  packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: PPPPP
  packages/web-shell/client/App.test.tsx: PPPP
  packages/web-shell/client/components/messages/ToolGroup.test.tsx: PPPP

verdict: timeout
summary: only 4 of 5 rounds fit the 15-minute budget; the completed rounds agreed

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/vscode-ide-companion/src/commands/index.test.ts: P (exit 0)
round 1 · packages/vscode-ide-companion/src/diff-manager.test.ts: P (exit 0)
round 1 · packages/vscode-ide-companion/src/extension.test.ts: P (exit 0)
round 1 · packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: P (exit 0)
round 1 · packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: P (exit 0)
round 1 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/components/messages/ToolGroup.test.tsx: P (exit 0)
round 2 · packages/vscode-ide-companion/src/commands/index.test.ts: P (exit 0)
round 2 · packages/vscode-ide-companion/src/diff-manager.test.ts: P (exit 0)
round 2 · packages/vscode-ide-companion/src/extension.test.ts: P (exit 0)
round 2 · packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: P (exit 0)
round 2 · packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: P (exit 0)
round 2 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/components/messages/ToolGroup.test.tsx: P (exit 0)
round 3 · packages/vscode-ide-companion/src/commands/index.test.ts: P (exit 0)
round 3 · packages/vscode-ide-companion/src/diff-manager.test.ts: P (exit 0)
round 3 · packages/vscode-ide-companion/src/extension.test.ts: P (exit 0)
round 3 · packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: P (exit 0)
round 3 · packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: P (exit 0)
round 3 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/components/messages/ToolGroup.test.tsx: P (exit 0)
round 4 · packages/vscode-ide-companion/src/commands/index.test.ts: P (exit 0)
round 4 · packages/vscode-ide-companion/src/diff-manager.test.ts: P (exit 0)
round 4 · packages/vscode-ide-companion/src/extension.test.ts: P (exit 0)
round 4 · packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: P (exit 0)
round 4 · packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: P (exit 0)
round 4 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/components/messages/ToolGroup.test.tsx: P (exit 0)
round 5 · packages/vscode-ide-companion/src/commands/index.test.ts: P (exit 0)
round 5 · packages/vscode-ide-companion/src/diff-manager.test.ts: P (exit 0)
round 5 · packages/vscode-ide-companion/src/extension.test.ts: P (exit 0)
round 5 · packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: P (exit 0)
round 5 · packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: P (exit 0)

Evidence images

01-ab-base-vs-head

02-mutation-matrix

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

Qwen Code · sandboxed verification

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

Agent-assisted review at dc665a602f686b935e080479bdeec3b7b2d83f16 — Partial review — coverage gaps; no new confirmed Critical in the inspected paths.

Reviewed the complete current production diff (6 production files, approximately 163 changed lines), with surrounding permission-diff lifecycle and submit functions, plus the changed dismissal/request-binding/rewind/toast tests. Traced onDidClosePermissionDiff through the registry, provider and parent-frame message handler into the existing Web Shell preview consumers, including nested agent display. Closing is not a vote: request-bound accept/cancel commands route through respondToPendingPermission, whereas dismissal changes preview ownership. Request-bound programmatic close deletes the map entry before the close callback; id-less closes independently emit dismissal. The receiver requires parent-frame provenance and the current request ID before handing ownership back; transcript updates suppress reopening the dismissed request, and request change/teardown restore host ownership.

Historical reassessment:

  • R2-1 (Windows-specific assertion, thread 3945351861) no longer applies: the current event and the deep-equality assertion at packages/vscode-ide-companion/src/diff-manager.test.ts:370–373 carry only permissionRequestId, not the former platform-dependent filePath. This is a source-level check, not a Windows test execution.
  • R1-19 was a PR-template/process concern, not a code Critical. The current body contains the previously missing review sections; it should not be carried as a current correctness defect.
  • The existing outer-loop fan-out catch, toast-guard explanation and missing negative/logging witnesses remain deferred Suggestions, not newly promoted blockers. Adjacent vote commands also use an outer catch; the earlier claim of a different adjacent pattern was incorrect.

The immediate and queued preflight catches preserve their existing admission gates and surface errors only through the respective ownership checks. The companion's producer captures the rewind target session before awaiting snapshots. No new daemon endpoint is introduced.

Coverage gaps: no Extension Development Host run, real window/tab teardown race exercise, Windows/macOS run, build, typecheck, tests or mutation probes. Not every existing Web Shell regression or lifecycle interleaving was re-reviewed; historical reviewer execution claims are not my test evidence. Therefore this is not a full runtime pass or a CLEAN verdict. Bug-fix scope is below the size gate; no maintainer exemption is assumed.

Comment only; no approval implied.

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

Resolve the EmbeddedApp.test.tsx conflict by keeping both test blocks:
- the PR's "permission diff dismissal" / "request-id wiring" / "message edit rewind" suites
- main's "rewind preflight localization" suite

and adopt main's typed getRewindSnapshots/rewindSession mocks.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-conflict/jmtwtxrtzxr

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

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

  • the two new commands/index.test.ts id-forwarding cases gate nothing this diff introduces (re-derived this round by Agent 7's test-efficacy revert probe: all 10 tests in the file still pass with every source change reverted) - already report…
  • closeAll() is a third close path that never fires the new dismissal event - already reported (round-8 deferred list, review 5154433504); @doudouOUC recorded it in round 9 as latent rather than user-facing
  • DiffManager.dispose() has no production caller, so the added emitter disposal and its new witness cover a dead path - already reported as D5-1 (round-5 deferred list, review 5135780307)
  • the extension.test.ts fan-out witness drives every listener on every emitter built during activate, so it cannot discriminate the loop from a first-element-only call - already reported as R1-4 (round-7 deferred list, review 5150914176)
  • the per-provider drop-point log is mis-tagged [Extension] inside WebViewProvider.ts, fires once per dead provider on a successful delivery and carries no request id - already reported as R1-10 (comment 3944217736)
  • both new preflight toast guards are witnessed only on their true branch, so deleting either guard leaves the tests green - already reported as R1-15 (comment 3944217749)
  • the guard-split comment names a mechanism the code disproves and the two preflight toast gates disagree on identical input - already reported as D4-1 (round-4 deferred list, review 5135150418)
  • the openDiff wire-key to permissionRequestId hop in FileMessageHandler.handleOpenDiff has no test, so the whole dismissal chain is witnessed from both ends but never across the inbound seam - already reported (round-9 deferred list, review …

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally; Agent 7 built and tested on Linux only, which matters for a VS Code companion change that ships to all three platforms and because the round-2 Critical R2-1 was a Windows-only test failure no Linux lane could catch.

Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally; Agent 7 built and tested on Linux only.

Not reviewed: issue-fidelity — GitHub's strong closing-issue metadata could not be resolved (review issue-context reported gh >= 2.72.0 is required for closing-issue references), so the closing-issue set is unknown rather than empty and discovery relied on the PR body's explicit Fixes #10557 / Refs #10585 / Refs #9911; all three were fetched and read in full and both motivating incidents were replayed against the merged world, but a target issue linked only through closing metadata and not named in the body would have been missed.

Not explored to full depth (tool budget reached): "agent 1a": full package test suites for vscode-ide-companion and web-shell (only the six touched test files plus the whole of App.test.tsx were run); "agent reverse-audit (round 1)": I did not read FileMessageHandler.handleOpenDiff (FileMessageHandler.ts:611-640) in full — I confirmed via grep that it maps data.requestId to permissionRe…; "agent reverse-audit (round 3)": the remaining composerSourceVersionRef.current += 1 sites (App.tsx:3621, 12525, 17323) — I read :12406 and :13135 and both are session-open transitions that i…; "agent 1b": none — I read the entire 1616-line diff (offsets 0–1615), extracted the full deletion set two ways ( ^-[^-] and ^-$ ), and verified the one non-obvious replac….

Test Plan (not a blocker): src/diff-manager.test.tsno such file or directory; src/webview/EmbeddedApp.test.tsxno such file or directory; src/commands/index.test.tsno such file or directory; client/App.test.tsxno such file or directory; client/components/messages/ToolGroup.test.tsxno such file or directory; and 6 more.

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

  • packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx:675 — [probe] the hand-back stickiness conjunct has no witness: deleting it leaves all 35 of the PR's tests green while a later transcript tick re-locks the row and reopens the close…

Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)

中文说明

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

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

未审查(原文为英文):build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally; Agent 7 built and tested on Linux only, which matters for a VS Code companion change that ships to all three platforms and because the round-2 Critical R2-1 was a Windows-only test failure no Linux lane could catch.

未审查(原文为英文):build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally; Agent 7 built and tested on Linux only.

未审查(原文为英文):issue-fidelity — GitHub's strong closing-issue metadata could not be resolved (review issue-context reported gh >= 2.72.0 is required for closing-issue references), so the closing-issue set is unknown rather than empty and discovery relied on the PR body's explicit Fixes #10557 / Refs #10585 / Refs #9911; all three were fetched and read in full and both motivating incidents were replayed against the merged world, but a target issue linked only through closing metadata and not named in the body would have been missed.

未探索到全部深度(达到工具调用预算):"agent 1a"full package test suites for vscode-ide-companion and web-shell (only the six touched test files plus the whole of App.test.tsx were run)"agent reverse-audit (round 1)"I did not read FileMessageHandler.handleOpenDiff (FileMessageHandler.ts:611-640) in full — I confirmed via grep that it maps data.requestId to permissionRe…"agent reverse-audit (round 3)"the remaining composerSourceVersionRef.current += 1 sites (App.tsx:3621, 12525, 17323) — I read :12406 and :13135 and both are session-open transitions that i…"agent 1b"none — I read the entire 1616-line diff (offsets 0–1615), extracted the full deletion set two ways ( ^-[^-] and ^-$ ), and verified the one non-obvious replac…

Test Plan(非阻断):src/diff-manager.test.tsno such file or directory; src/webview/EmbeddedApp.test.tsxno such file or directory; src/commands/index.test.tsno such file or directory; client/App.test.tsxno such file or directory; client/components/messages/ToolGroup.test.tsxno such file or directory; and 6 more。

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

机制健康:本轮未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff,除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上;并会一直如此,直到某一轮的标记重新带上锚点,或落地的嫁接能被运行该轮的评审使用。(仅陈述,不据此行动——这不改变本轮发布的任何内容。)

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

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Closeout triage: the latest sandbox report contains coverage/description follow-ups (F1/F2/F3/F5 and F4/C1/C2/C3), not a confirmed implementation defect. The template-gate Critical is already cleared, and the PR currently has zero unresolved review threads with all required CI lanes green. Given the established review history and scope, I am deferring these non-blocking suggestions rather than widening the PR.

@qwen-code-dev-bot qwen-code-dev-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.

APPROVE

已核对 head 6e73fd15c71d3b6d2593822872632b03285f766b(vs origin/main merge-base 20ecdaf6b2)。

历史 Critical 已关闭:R2-1(Windows 上 path.normalize 让断言只能在 POSIX 主机通过)在当前 head 已从根上消失 —— onDidClosePermissionDiff 的载荷只剩 { permissionRequestId }diff-manager.ts:93-110),cancelDiff 与 id-less closeDiff 两处 fire 都只带 request id,测试断言相应改为深比较 expect(closed).toHaveBeenCalledWith({ permissionRequestId: 'req-1' })diff-manager.test.ts:359-374)。我在该 head 的完整 diff 上扫过所有新增断言:只剩这一处对事件的深比较,没有任何「把裸 POSIX 字面量当作实现变换输出」的比较,路径类断言仍走 expect.stringContaining('foo.ts'),因此这一类 Windows-only 失败不会再回来。

其余历史项:24 条线程逐条对过当前 head —— filePath 冗余字段、sendMebageToWebView 漏斗复用、订阅 fan-out 的丢弃点日志、closeDiff 归属判定、isEditToolName 表合并(ToolGroup.tsx:555,现覆盖 edit/editfile/write/write_file/writefile)等均已落地;4 条明确标注为跨包范围外、由作者记为 follow-up。

独立复查未发现新的 Critical:emitter 在 dispose() 中释放(diff-manager.ts:179);closeDiffEditor 先删表,onDidCloseTextDocument → cancelDiff 那一跳查不到条目,因此同一次关闭不会双发;acceptDiff 不触发(投票不是撤销);webview 侧 permissionDiffClosed 处理保留了与投票处理同样的 event.source === window.parent 来源门(EmbeddedApp.tsx:810-821),dismissedPermissionDiffIdRef 在 focus 切换与 reset 两处都会清掉,不会出现粘死。

CI:required 四项均 success(Test (ubuntu-latest, Node 22.x)Lint & StaticIntegration Tests (no-AK, No Sandbox)web-shell E2E Smoke)。

本地验证packages/vscode-ide-companion 四个改动测试文件 Test Files 4 passed (4) / Tests 155 passed (155)。web-shell 客户端用例在本机无法收集(remark-cjk-friendly 未安装在本工作树,属环境缺依赖,非本 PR 引入),该侧以 CI 的 Test (ubuntu-latest) 为准。

不阻塞、请在 follow-up 里保住的事(本轮不要求改动,按 PR 已收敛到第 10 轮的姿态记录,避免静默丢失):

  1. closeAll() 是第三条关闭路径,不触发新事件 —— 人已在第 9 轮记为 latent 而非 user-facing;
  2. DiffManager.dispose() 目前无生产调用方,新增的 emitter 释放及其用例覆盖的是死路径(D5-1);
  3. commands/index.test.ts 两条新增 id 透传用例对本 diff 无门控力(源码全撤回仍 10/10 绿);
  4. extension.test.ts 的 fan-out 见证驱动所有监听器,无法区分「逐个调用」与「只调第一个」;
  5. WebViewProvider.ts 内的丢弃点日志前缀写成 [Extension],且成功投递时也会按 dead provider 逐条打、不带 request id(R1-10);
  6. 两处新增 preflight toast 守卫只在 true 分支有见证,删掉任一守卫用例仍绿(R1-15);
  7. EmbeddedApp.tsx:675 的 hand-back 粘住条件无用例,删掉后 35 条用例仍全绿;
  8. 本轮 ci-bot 亦披露 Test (windows-latest) / Test (macos-latest) 对本 head 为 skip,即合并前拿不到 Windows 侧信号;鉴于 R2-1 正是只有 Windows 才会暴露的那类问题,这一条值得在合并时留意(当前结论:载荷里已不含路径字段,该类失败无载体)。

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

No blocking findings. Approving.

Scope reviewed: all 6 production files (diff-manager.ts, extension.ts, EmbeddedApp.tsx, WebViewProvider.ts, App.tsx, ToolGroup.tsx) plus the 7 companion test patches.

What I checked:

  1. Double-fire analysis (class 1 / class 3): Traced both (Site 2) and id-less path (Site 1) against a shared invariant: closeDiffEditor deletes the map entry synchronously before any tabGroups.close await. A onDidCloseTextDocument → cancelDiff hop that arrives during or after the tab close finds the entry gone and returns early. No double-fire path exists.

  2. Vote-path isolation (class 10): The PR claims qwen.diff.accept / qwen.diff.cancel never reach cancelDiff for request-bound diffs — guarded by !permissionRequestId → routes through respondToPendingPermission instead. R3-7 and R3-8 in extension.test.ts witness this guard directly (getPermissionRequestId returning 'req-1' → acceptDiff not called; returning undefined → called once). The guard is independently traceable statically.

  3. EmbeddedApp state machine (class 8): dismissedPermissionDiffIdRef is cleared by updateTranscript when permissionToFocus changes (new request) or becomes undefined (permissions resolved). hostOwnsEditDiffPreview resets correctly through both paths. The !has(id) && dismissedId !== id guard prevents re-opening the dismissed diff.

  4. submissionOwnerIsCurrent refactor (App.tsx): submissionSessionIsCurrent is the four conjuncts that form the base; submissionOwnerIsCurrent composes on it — boolean AND is commutative and both calls are pure reads. Semantic equivalence holds.

  5. WebViewProvider.notifyPermissionDiffClosed: sendMessageToWebView is wrapped in try/catch in extension.ts. Null chatProviderRegistry is logged and dropped. WebViewProvider.test.ts witnesses the {type: 'permissionDiffClosed', data: {requestId: ...}} payload shape.

Disclosure:

  • extension.test.ts not run locally — R3-7/R3-8 (the extension.ts fan-out witnesses) couldn't be collected on the author's machine (missing @qwen-code/qwen-code-core build). The 7-line binding is simple enough to verify statically and the two ends of the chain are independently tested, but this rung is CI's to confirm.
  • macOS and Windows CI lanes were skipped at this commit — stated by the author; platform confirmation owed by CI before merge.
  • Post-vote close race (deferred from round 5): If a native-vote command doesn't close the diff tab, a subsequent manual tab close fires onDidClosePermissionDiffEmitter for the already-decided request. updateTranscript resets the state on the next transcript change (permission block resolving), so the window is narrow. Not a blocker; already recorded on the thread.

Cross-check: Prior reviews (rounds 1–5 by qwen-code-ci-bot) raised 14 Suggestion-level findings, all addressed. The round 5 deferred items (dispose() caller; post-vote close race) are noted above as observations. Round 9 triage confirmed no Critical findings at dc665a6 and traced the mechanism end-to-end. No finding in the prior reviews is unaddressed or unacknowledged.

Reviewed with AI assistance.

@yiliang114
yiliang114 added this pull request to the merge queue Sep 11, 2026
Merged via the queue into main with commit 78bbd9f Sep 11, 2026
81 checks passed

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Reviewed at 6e73fd1 (base 20ecdaf). The main flow is right: the emitter/dispose wiring is clean, closeDiffEditor deletes the map entry before tabGroups.close, so the onDidCloseTextDocument -> cancelDiff hop can't double-fire, and the vote paths never mis-fire the dismissal. The event.source === window.parent gate is sound and the payload carries only the request id.

This landed before I finished, so treating everything as follow-up rather than blocking. One of them I'd still fix: the dismissal notification routes by active webview instead of by the request's owning webview, so in the panel-open + sidebar-owns-the-request configuration #10557 is unchanged. Details inline.

Two non-blocking notes on the diff overall:

  • Scope: 1341 additions for ~156 production lines, bundling #10557 + #10585 (twelve test witnesses) + #9911, after ~5 review rounds. Per AGENTS.md ("Don't let review rounds balloon the PR"), the #10585 witness set would have been better as its own PR.
  • Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipping at this commit, and R2-1 on this PR was a Windows-only failure — so the platform that previously broke had no signal here.

);
return;
}
this.sendMessageToWebView({

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This routes by active webview, but permission ownership is per-webview: webShellPermissionOwners maps webview -> requestId, and sendMessageToWebView posts to getActiveWebview(), which prefers the panel over the sidebar.

Concrete drop: sidebar webview owns the pending request, user then opens the chat editor-tab panel, then closes the diff. getActiveWebview() returns the panel, the panel's webShellPermissionRequestIdRef.current !== requestId, the flag never flips, and the row stays locked — i.e. #10557 unchanged in that configuration. Panel dispose deliberately keeps the sidebar owner entry (~L209), so the two coexisting is a supported state, not an edge case.

respondToPendingPermission right below (~L2541) already does this correctly — it looks the owner up out of webShellPermissionOwners and posts to that webview directly. Mirroring it here is about three lines.

// leaving the user to approve or reject something they can no longer look
// at (#10557).
diffManager.onDidClosePermissionDiff(({ permissionRequestId }) => {
try {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The try wraps the whole loop, so the first throwing provider skips notification for every provider after it — which is the opposite of what the comment below says it's protecting ("must not take down ... every other surface's notification"). Moving the try inside the for gets the stated behavior.

// Narrower than submissionOwnerIsCurrent below: it answers "is the user
// still looking at the session this submission belonged to", which is
// what decides whether a failure is worth telling them about. The full
// guard also tracks composer identity, and submitting is itself what

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The rationale here doesn't match the code. "submitting is itself what moves that" — composerSourceVersionRef.current += 1 only happens on workspace-trust change, new session, workspace picker, and standalone session; never on submit. And the immediate-failure path a few hundred lines up gates its identical toast on admissionOwnerIsCurrent(), which does include the sourceVersion conjunct this comment says would suppress the message.

Also worth noting the split drops three conjuncts, not one: !sessionWriteBlockedRef.current, the write-block generation match, and the version match. So the two toast paths now have materially different suppression rules for the same user-visible error. Either is defensible, but not both — and the comment should describe whichever one you keep.

if (isAskUserQuestionToolName(tool.toolName)) return true;
if (name === 'write_file' || name === 'writefile') return true;
if (name === 'edit' || name === 'editfile') return true;
if (isEditToolName(name)) return true;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Non-blocking, but this widens behavior beyond the swap it looks like: isEditToolName also matches write, which shouldAutoExpand didn't cover before, so plain write rows now auto-expand for every web-shell host. hasDetailView already includes write so it renders fine — just calling out that it's an unrelated behavior change riding along.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.23.4.

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.

vscode: closing a web-shell permission diff tab leaves the approval row locked without a re-open path

5 participants