fix(vscode): restore native diff approval after WebShell cutover - #10534
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Re-run on unchanged head
Moving on to code review. 🔍 中文说明在相同 head
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent proposal first: bind each native diff to the pending permission request id that opened it, gate The PR matches that proposal hop for hop, and adds two things I would have wanted only after being burned once: approval diffs open read-only (the approving tool applies its own content — an editable right side would silently discard user edits), and the read-only/writability bit participates in diff dedupe/focus matching so a writable twin can never be refocused for a read-only approval. I re-read the full diff at this head for this re-run, including the vote fan-out over What I verified at this head:
Nothing new blocks. The 15 suggestion-level findings from round 3 (test witnesses for the request-id hops, plus three behavioral nits — latch reset across cycles, SplitView pane not receiving the flag which fails closed, and the host file-open handoff skipping the sequenceDiagram
participant U as User
participant C as qwen.diff.accept command
participant H as WebViewProvider
participant W as Webview EmbeddedApp
participant S as Web Shell App
U->>C: Accept or Reject in native diff
C->>H: respondToPendingPermission (fromDiffEditor, requestId)
H->>H: find owner webview by exact requestId
H->>W: webShellPermissionDecision
W->>W: check source is parent and requestId is current
W->>S: respondToPendingPermission (requestId, decision)
S->>S: exact-id gate, then submit allow_once or reject_once
Files changed (23)
Test evidenceCI on
Sandboxed verification: round 5 passed on exactly this head — 31/31 scripted assertions, flakiness gate green (9 changed test files × 5 rounds, no divergence), the A/B central claim green at head against a red base ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The central behavioural claim is now substantiated at this exact head by the round-5 verification (A/B flip plus mutant kills), which is stronger evidence than the unit suite alone; the still-running round re-confirms it. The author's real-host E2E reports earlier in this thread remain the author's claim, not independently re-run here. 中文说明代码审查独立方案：把每个原生 diff 绑定到打开它的权限请求 id，在 accept/cancel 命令处设门禁，只把决定转发给持有该 id 的 webview，逐跳校验、处处失败即关闭，并保留旧 ACP 路径作为回退。本 PR 与之一致，且额外做对了两件事：审批 diff 以只读打开（审批工具自带内容，可编辑侧会悄悄丢弃用户输入）；只读/可写位参与 diff 去重与聚焦匹配，避免只读审批复用可写孪生体。本次重跑重新通读了该 head 的全部 diff（含对 第三轮 Critical（R3-1）已由 测试证据
— Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review with no blocking findings at this head; the score is capped by policy, not by doubt — cross-package scope means a maintainer makes the final call, not this bot. Stepping back: this PR went through three review rounds, three sandboxed verifications, and a real-host regression report from its own author, and came out the other side with every Critical fixed and pinned. The What has changed since my last pass — two of the three inputs I asked for are now in:
What keeps this at a defer rather than an approval:
Deferring to a maintainer for the merge call: weigh the deferred findings, clear the stale change-requests, and merge when satisfied — the technical evidence (review, CI, sandboxed A/B verification) is all in this thread and all green at 中文说明置信度：3/5 —— 本 head 审查干净、无阻塞项；分数是策略封顶而非真实疑虑：跨包规模意味着最终决定由维护者作出。 回顾整个过程：三轮评审、三次沙箱验证、作者本人的真机回归报告，所有 Critical 均已修复并带固定测试。 上次通过以来的变化——我要求的三项输入已到位两项：
转交维护者而非直接批准的原因：
请维护者作出合并决定：权衡推迟的发现、清理过期 change-request，满意即可合并——技术证据（评审、CI、沙箱 A/B 验证）均已在本帖且在 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "agent 6c": end-to-end confirmation that a live VS Code daemon session actually reaches two concurrent unresolved permission blocks (workflow/agent-team subagents) — eviden…; "agent 6a": could not verify whether the daemon transcript can hold two concurrently unresolved permission blocks (background/subagent confirmation flow), which determines ….
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
CI attribution — run 33285925586 (head
|
Sandboxed iframes inside the panel webview (MCP apps, artifact previews) can postMessage to the webview window, and the webShellPermissionDecision handler could not tell them apart from extension-host messages. Accept only the platform-delivered shape (event.source === null) so untrusted content cannot vote on pending approvals, mirroring the existing event.source validation used for daemon token messages in the web shell. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The web-shell approval bridge resolves the daemon permission with an option id only; the edited right-hand content of a native diff is consumed solely by the IDE-mode resolver (IdeClient.diffResponses), which is not populated when IDE mode is off. An editable right side therefore silently discarded anything the user typed before pressing Accept. Diffs opened with source 'web-shell' are now passed through showDiff with readOnly so the proposed side stays locked, while IDE-mode diffs keep the existing writeable-in-session behavior. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The two-argument showDiff overload takes options where the implementation expects the optional old-content string; widen the implementation parameter and resolve the options object accordingly. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen.diff.isVisible is also true on the user's original file while a permission diff is open (originalFilePath match), and showDiff opens with preserveFocus, so Ctrl+S or the check/cross title buttons on the original file ran qwen.diff.accept/cancel and the vote loop resolved the pending web-shell approval for an edit the user may never have looked at. Pass the triggering document context into respondToPendingPermission and only take the web-shell branch when the trigger is a qwen-diff document; the legacy pendingPermissionResolve vote path is unchanged. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
isEditToolName was the only is*ToolName helper that required callers to pre-lowercase the name; the next caller following the sibling convention and passing the raw wire name would silently never match, leaving a pending edit row auto-expanding and expandable while its native diff approval is outstanding. Normalize internally like the rest of the family and pass the raw name at the call site. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…iant comment The expandability comment above ToolLine's expandable flag claimed content never controls whether the user can open a row, which the pending-edit- approval gating introduced in this PR falsifies. Extend the comment with the exception so the stated invariant matches the code. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Pin the remaining R1-4 coverage: respondToPendingPermission must submit the allow_once option (never allow_always) when an edit approval carries both kinds, and toPermissionOptions must keep both kinds on edit approvals with allow_always first in wire order — the arrangement that makes the App-side kind preference the only guard against a single native Accept escalating to "Allow All Edits". Swapping the kind ladder turns the App test red (mutant-checked). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-5 duplicated tool-name enumeration — already reported (comment 3888422129); author filed follow-up issue 10544
- R1-6 hand-enumerated pending-approval detection — already reported (comment 3888422120); author filed follow-up issue 10544
— qwen3.8-max via Qwen Code /review (v0.22.3)
VS Code Extension Development Host E2E — FAILI ran the same real-host flow against the exact PR base and head. The UI cleanup works, but the core native Diff approval bridge regresses a flow that already succeeds on the base commit, so the current head is not ready to merge. Test target
Before / After: pending Edit presentation
PASS (visual only): the head keeps the native diff and WebShell permission choices, while removing the duplicate empty expanded Edit detail. The waiting elapsed value is no longer shown on the pending Edit row. I did not treat this as proof that cumulative elapsed time is paused after resolution. Before / After: native Accept
FAIL / regression:
After: native Reject / Close
FAIL: the native diff closes and Result matrix
Blocking findings
Non-blocking follow-ups
VerdictThe visual simplification is an improvement, but the current head breaks the primary Accept path, does not safely associate a native diff with one WebShell permission request, and changes shared WebShell behavior for hosts without a native diff owner. I would keep this PR blocked until those issues are fixed, then rerun this exact base/head matrix plus a two-provider or two-pending-diff isolation case and a default-WebShell control case. |
…d tests The read-only permission-diff work added a 4th options argument to diffManager.showDiff in the qwenCode.showDiff command but left two pre-existing assertions expecting the old 3-argument call, turning the package unit suite red. Update both assertions and add a passthrough test pinning that readOnly: true reaches the diff manager. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
… window The webShellPermissionDecision source guard introduced for R1-1 checked for event.source === null, but extension-host messages never arrive that way: they travel over the webview message port and are relayed into the content frame by the webview preload, which posts them from the parent frame. On a real host the guard therefore rejected every Accept/Reject delivered through the native diff flow. Match the shape the host actually produces (event.source === window.parent) — the same convention the web shell already uses for daemon token messages — which keeps nested sandboxed iframes (MCP apps, artifact previews) out because they post from their own child windows, and fail closed on synthetic deliveries with no source. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The fromDiffEditor gate from R1-3 treats any qwen-diff editor as the pending web-shell approval, so while a permission is pending, accepting an unrelated qwen-diff (a stacked approval for another file, or a diff left over from another session) resolved the pending approval for an edit the user never looked at. Ship the paths the web shell is actually waiting on with webShellPermissionState, keep them in sync while the pending flag stays true, resolve them the same way the showDiff command does, and only take the web-shell vote branch when the triggering document matches one of them. The legacy pendingPermissionResolve path is unchanged. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Nothing asserted that handleOpenDiff turns source: 'web-shell' into the readOnly option on qwenCode.showDiff, so the lock that keeps permission diffs from silently discarding hand edits could regress unobserved. Pin both directions through the real command registration. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
showDiff honored the readOnly option only on the fresh-open path; the reuse paths matched on path+content alone, so a web-shell approval could be deduped onto (or refocused to) a writable IDE-mode diff for the same (path, old, new) triple. Hand-edits typed there are silently discarded when the approving daemon tool applies its own proposed content -- the exact harm readOnly was added to prevent. Include the writability in DiffInfo and require it to match in hasExistingDiff/focusExistingDiff. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
webShellPermissionPending was reset on panel dispose, session change and webShellReady, but not when the view-hosted webview was disposed. With an edit approval pending, closing the sidebar chat view left hasPendingPermission() true; an accept from the still-open diff editor then found no active webview, skipped the vote and fell through to the absent legacy resolver, so the vote vanished while acceptDiff had already closed the diff. Reset the flag and paths in webviewView.onDidDispose, mirroring the panel dispose path. Also pin hasPendingPermission() to the webview-pushed state so the extension command gate cannot regress to the legacy-resolver-only check. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Neither the fromDiffEditor derivation from the diff scheme in qwen.diff.accept nor the hasPendingPermission() precondition in front of it was asserted, so flipping the scheme comparison or reverting the gate to the legacy-resolver check shipped green. Drive the registered command handler with a permission-aware provider stub and assert both links. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The webview->host webShellPermissionState post from
closeOpenPermissionDiffs had no test: deleting that post shipped green
and would leave WebViewProvider.webShellPermissionPending true after the
host unmounts. Assert the teardown posts {pending: false, paths: []} and
closes the tracked diff.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Review — PR 10534 · fix(vscode): restore native diff approval after WebShell cutover
Verdict: APPROVE — no blocking findings.
Scope
Source code only. Reviewed 23 changed files across vscode-ide-companion and web-shell packages. I did not review the manually-unverified test screenshots or the PR's VSIX build evidence (not yet submitted per PR body).
Not covered: macOS and Windows Test jobs (SKIPPED) · integration test jobs (SKIPPING/PENDING) · runtime execution (no toolchain available).
CI status
| Check | Result |
|---|---|
| Test (ubuntu-latest, Node 22.x) | PENDING |
| Test (macos-latest, Node 22.x) | SKIPPED |
| Test (windows-latest, Node 22.x) | SKIPPED |
| Integration Tests (CLI, No Sandbox) | SKIPPING |
| Integration Tests (no-AK, No Sandbox) | PENDING |
| Desktop Shell (ubuntu-22.04 / windows-2022) | PASS |
| Secret scan / Dependency CVE audit | PASS |
| Real daemon E2E / Java 11 | PENDING |
The key unit test job (ubuntu) is still pending at time of review. macOS and Windows tests are skipped — no evidence from those platforms.
Class 1 — Contract asymmetry (cross-file checks)
openDiff message producer→consumer:
EmbeddedApp.tsx now always includes requestId in openDiff messages. FileMessageHandler.ts reads data.requestId and passes it as permissionRequestId to showDiff. DiffManager stores it and returns it via getPermissionRequestId(). Symmetric.
closeDiff message backward compatibility:
Old format { path } still works — DiffManager.closeDiff treats permissionRequestId === undefined as "match any". New format { path, requestId } scopes the close to the right permission. Backward-compatible.
toPermissionOptions ordering vs respondToPendingPermission selection:
permissionUtils produces allow_always first, allow_once second. App.tsx::respondToPendingPermission selects by kind (allow_once preferred), not by position. The new test in permissionUtils.test.ts asserts both options are present. No silent "Allow All Edits" escalation path.
diffDocuments Map key: Confirmed from the source at head that diffDocuments is keyed by rightDocUri.toString() (via addDiffDocument). Both hasDiff and getPermissionRequestId use the same key. No mismatch.
Class 2 — API / compatibility
WebShellApi.respondToPendingPermission added as an optional member — existing callers of the interface are not broken.
DiffManager.showDiff overloads — the existing two-argument form (filePath, newContent) is preserved. The three-argument form (filePath, old, new) now accepts an optional fourth ShowDiffOptions arg. All pre-existing call sites in commands/index.ts and FileMessageHandler.ts were updated and their tests reflect the new signatures.
DiffManager.closeDiff signature extended with optional permissionRequestId; default undefined preserves old "close by path" behavior.
qwen.diff.accept / qwen.diff.cancel gate
The commands now only vote on web-shell permissions when isManagedDiff = docUri?.scheme === DIFF_SCHEME && diffManager.hasDiff(docUri) is true. A file: URI can no longer accidentally resolve a pending web-shell approval (confirmed by the new test in extension.test.ts). This is an intentional tightening versus the legacy behavior.
Cross-check against existing reviews (qwen-code-ci-bot rounds 1 and 2)
Both prior CHANGES_REQUESTED reviews were against earlier commits (0e1bada and eb6ff92); the current head is 7ecdea9. I checked each finding against current code:
| Finding | Check at current head | Outcome |
|---|---|---|
| R1-1 iframe postMessage source (Critical) | event.source === window.parent check present; iframe-source test added |
Confirmed fixed |
| R1-3 fromDiffEditor gate too broad (Critical) | Now also gated by diffManager.hasDiff(docUri) — only registered diffs can trigger vote |
Appears addressed |
| R2-1 showDiff 4th arg breaks callers (Critical) | Overload resolved correctly; call sites and tests updated | Appears addressed |
| R1-5 duplicated tool-name enumeration (Suggestion) | Author filed follow-up issue #10544 | Deferred upstream |
| R1-6 hand-enumerated approval detection (Suggestion) | Author filed follow-up issue #10544 | Deferred upstream |
| R2-2 readOnly not applied on focus path (Suggestion) | focusExistingDiff now checks diffInfo.readOnly === readOnly |
Appears addressed |
| R2-4 webShellPermissionState untested (Suggestion) | New tests in WebViewProvider.test.ts cover pending-state tracking and view-dispose clearing |
Appears addressed |
| R2-5 command-level gate untested (Suggestion) | New test in extension.test.ts diff vote command gate covers the key paths |
Appears addressed |
| R2-3 lock stops when user closes read-only diff (Suggestion) | Known limitation; no fix at current head | Cannot rule — pre-existing gap |
R2-3 (the silent lock release when the user manually closes a read-only diff via VS Code's tab UI) remains open. It is suggestion-level and does not block merge: the consequence is that the inline row briefly shows its expand affordance again, not that a permission is silently mis-resolved.
Unreviewable dimensions
- CONTRIBUTING.md: could not fetch (connection error) — AI-review policy unverified.
- macOS and Windows unit tests: SKIPPED — no platform evidence.
- Runtime behavior: no execution environment available.
- PR body notes no local build or test suite run was performed before submission.
Reviewed with AI assistance.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 5319 passed · 0 failed · 5319 total Flakiness gate: ✅ 9 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：5319 通过 · 0 失败 · 5319 总计 抖动门：✅ 9 changed test file(s) x 5 identical rounds, no divergence Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) — round 3, follow-up 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: 5319 passed · 0 failed · 5319 total 中文 — 判定：✅ 通过 · 可合入（agent 判定，第三轮跟进）
Previous-finding status (re-measured at the new head)This is a follow-up round. The input closure is bit-identical to round 2:
No declined/deferred rows existed; nothing required re-measurement of an accepted tradeoff. Central claim + A/BCentral claim: while a WebShell edit permission with a file diff is pending, the native diff editor's Accept/Reject commands resolve that exact permission (allow / reject routed to the owner webview as Secondary claims: (a) permission diffs open read-only and never dedupe onto a writable twin; (b) a single native Accept submits The harness (
Base-side failure modes are behavioral, not import errors: 0 decision messages on every cell, the diff opened writable, and the legacy resolver settling to CorrectionsNone new. The round-2 correction (a commit message described a fail-closed source guard while the then-final code carried a requestId-match disjunct) remains obsolete: the shipped guard at this head is the conjunction the message described, verified by reading FindingsS1 — stacked approvals: a vote on a non-front permission diff is dropped without notice (Suggestion, fails closed; carried, re-measured)Re-measured at this head by harness cell 5: with the owner map holding S2 — the requestId-validation axis is behaviorally correct but unpinned at both layers (Suggestion / completeness; carried, re-measured)Mutant M7 (EmbeddedApp source guard reduced to
Classification: coverage gap (behavior right, nothing asserts it), with the two probe files as the fixtures that would pin it. Not a merge condition — a same-source attacker still needs the unguessable requestId, and the second layer catches what the first misses. Mutation matrix (single-point mutants of PR guards at HEAD)
Positive controls: unmutated suites green at head (804/804 gate run; probe files 19/19 and 586/586 at head); M6 killed by the same EmbeddedApp harness M7 survived. Raw logs Not covered
MethodologyEnvironment: CI verify container ( Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
CI classification (automated sweep): |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 1741 passed · 0 failed · 1741 total Flakiness gate: 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：1741 通过 · 0 失败 · 1741 总计 抖动门： Verification reportSandboxed verification: ✅ passed — merge-ready (agent verdict) — round 4, follow-up Ran the PR in an isolated, token-free container: A/B flip (head test bytes vs base build), mutation checks, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 1741 passed · 0 failed · 1741 total (verified head 中文 — 判定：✅ 通过 · 可合入（agent 判定，第四轮跟进）
Previous-finding status (re-measured at the new head)This is a follow-up round. The input closure is bit-identical to rounds 2 and 3 — what was compared, not just "nothing changed":
No declined/deferred rows existed in round 3; nothing required re-measurement of an accepted tradeoff. Central claim + A/BCentral claim: while a WebShell edit permission with a file diff is pending, the native diff editor's Accept/Reject commands resolve that exact permission (allow/reject routed to the owner webview as Round 4's live A/B is the flip table — head test bytes executed against base code — plus the targeted gates at head. Round 3's end-to-end approval-loop harness (real
Base-side reds are behavioral, not import errors: missing Load-flake audit of CorrectionsNone. The round-2 correction (commit message vs shipped guard shape) remains obsolete: the shipped guard is still the conjunction the message described. FindingsNo new findings. Carried, re-verified: S1 — stacked approvals: a vote on a non-front permission diff is dropped without notice (Suggestion, fails closed; carried, re-verified by inspection)At this head, S2 — requestId-validation axis behaviorally correct but unpinned at both layers (Suggestion / completeness; carried, M7 half re-run live)Mutant M7 (source guard reduced to Mutation checks at HEAD (re-run live this round)Matrix as printed:
Positive controls this round: unmutated gates green (804/804); unmutated EmbeddedApp suite green inside the M6/M7 runs' passing cells; M4's 584 skipped tests are filtered-out, not executed. The M6 first attempt (hook timeout at load 249, zero tests executed) is discarded and not counted. Not covered
MethodologyEnvironment: CI verify container ( Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": executing the new case in packages/cli/src/acp-integration/session/permissionUtils.test.ts — vitest globalSetup guard stopped on unbuilt workspace dist (web-tem…; chunk 4: could not execute EmbeddedApp.test.tsx to confirm green at the reviewed commit — the worktree has no built dist/ and npm run build -w @qwen-code/sdk -w @qw….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
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:2241 — [probe] D3-1 lock test pins only the no-expand-affordance half of the pending-edit lock; the no-auto-expand half has no witness — deferred by the rounds-2-5 code-age ru…packages/vscode-ide-companion/src/diff-manager.test.ts:42 — [probe] D3-2 Uri.with() mock closes over the original uri so derived URIs collide in diffDocuments keying — deferred by the rounds-2-5 code-age rule (mock factory unchanged since t…packages/web-shell/client/components/messages/ToolGroup.tsx:142 — [probe] D3-3 isEditToolName omits notebook_edit, so the row lock never engages for a tool whose bound vote passes the toolKind gate — deferred by the rounds-2-5 code-age rule…
Convergence: round 3 posted 17 inline comment(s), 16 of them reported for the first time; the previous round posted 10 (10 new). Findings keep coming back to the same files: packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx (findings in rounds 1, 2; 3 more now); packages/vscode-ide-companion/src/commands/index.ts (findings in round 2; 2 more now); packages/web-shell/client/components/messages/ToolGroup.tsx (findings in round 2; 2 more now), and 2 more file(s). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
Round-3 bot sweep on unchanged head Context: this head had already cleared the round-1/2 findings (fixes in
Net: 1 thread resolved (re-post), 1 Critical confirmed and left open for human decision, 15 findings deferred under the scope fuse. No |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 31 passed · 0 failed · 31 total Flakiness gate: ✅ 9 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：31 通过 · 0 失败 · 31 总计 抖动门：✅ 9 changed test file(s) x 5 identical rounds, no divergence Verification reportSandboxed verification: ✅ passed — merge-ready (agent verdict) — round 5, follow-up Ran the PR in an isolated, token-free container: A/B flip against the new base, delta-isolation mutants, predicate-divergence wire oracle, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 31 passed · 0 failed · 31 total (verified head 中文 — 判定：✅ 通过 · 可合入（agent 判定，第五轮跟进）
Previous-finding status (re-measured at the new head
|
| # | Finding (round 4) | Severity | Status at 4fee90a6 |
|---|---|---|---|
| F1a | extension.test.ts gate test red at head |
Blocker | fixed (re-run) — gate green 15/15; mutant M9 (legacy fallback dropped) killed live by expected "spy" to be called with arguments: ['allow'], 0 calls (logs/m9-legacy-fallback.txt) |
| F1b | ToolGroup.test.tsx lock test red |
Blocker | fixed (re-run) — gate green 85/85; mutant M5 (no lowercase in isEditToolName) killed live by the mixed-case lock row (lineExpandable present where absent expected) (logs/m5-lowercase.txt) |
| F2 | requestId-match disjunct bypassed the decision source check | Suggestion | fixed (re-run) — shipped guard is still the conjunction at EmbeddedApp.tsx:691-693; mutant M6 (drop window.parent conjunct) killed live by both source-guard tests, 2 red / 16 pass (logs/m6-source-guard.txt) |
| F3 | two shipped tests could not kill mutants along their axis | Reporting | fixed (re-run) — both axes killed live (M5, M9); the delta's new structured test additionally kills M-D1 and M-D3 along its own axes |
| S1 | stacked approvals: vote on a non-front permission diff dropped without notice | Suggestion | stands — re-inspected at new head: WebViewProvider.ts:2415-2436 posts only when an owner webview matches the requestId, then returns unconditionally; a vote for an unowned requestId produces zero messages and no notice. Fail-closed (the stacked diff stays open, nothing resolves); base had no bridge. The shell-layer voteNotApplied notice added by this PR fires only after a vote reaches the shell, so the host-layer drop remains silent |
| S2 | requestId-validation axis unpinned at both layers | Suggestion | stands — mutant M7 (drop requestId conjunct) re-applied live and survived the shipped EmbeddedApp suite 18/18 (logs/m7-requestid-conjunct.txt). Behavior correct by inspection at the new head: the conjunct is present at EmbeddedApp.tsx:691-693 and the shell re-validates request.id !== requestId at App.tsx:9166. Coverage gap, not a merge condition |
Central claim + A/B
Central claim (round 5, the delta commit): a native Accept/Reject vote on a structured edit approval — a pending permission whose tool-call content carries a {type: 'diff', path, oldText|newText} block — resolves that exact request through the shell's respondToPendingPermission(requestId, decision), submitting allow_once (never allow_always) even though the wire order puts allow_always first. Secondary claims: (a) the rest of the bridge (source guard, owner map, read-only diffs, legacy fallback) still holds at the new head after the base move; (b) the merge preserved both sides' hunks in the co-touched App.tsx.
The wire shape is not a fixture invention: the producer is buildPermissionRequestContent (packages/cli/src/acp-integration/session/permissionUtils.ts:155-162), called unconditionally by the main approval path (Session.ts:11660) and the sub-agent path (SubAgentTracker.ts:245) — exactly one top-level diff block per edit approval, warnings wrapped as {type:'content', content:{type:'text'}} (which both diff predicates correctly ignore).
| Cell | Production | Oracle (scripted) | Result |
|---|---|---|---|
| A/B head: structured-edit vote | HEAD 4fee90a6 |
App structured test: resolved === true, submitPermission('req-1','proceed_once') |
1/1 green (logs/ab-head-cell.txt, 01-ab-structured-vote-head-vs-base.png) |
| A/B base: same test bytes | base fca2acd5 (HEAD^1) |
predicted red: no vote endpoint | 1/1 red as predicted — TypeError: shellApi?.respondToPendingPermission is not a function (logs/ab-base-cell.txt) |
M-D1: structured detection severed (hasPermissionDiffPreview → false) at head |
head + mutant | delta-isolation: vote must be declined | KILLED — expected false to be true on resolved: the pre-delta decline reproduced behaviorally (logs/md1-structured-test.txt) |
| M-D3: allow_once-first ladder swapped on requestId path | head + mutant | escalation guard | KILLED — mutant submitted proceed_always (single Accept → "Allow All Edits") (logs/md3-ladder-swap.txt) |
| M-C1: allow_once removed from edit wire options (cli) | head + mutant | wire-order pin | KILLED — expect(options).toEqual([...]) mismatch (logs/mc1-wire-order.txt) |
M-D2: hasDiffPreview !== true clause dropped from requestId gate |
head + mutant | full App suite | SURVIVED 592/592 — classified redundant defence, see Findings (logs/md2-guard-drop-full-app.txt) |
| Gates at head | HEAD 4fee90a6 |
App 592 + ToolGroup 85 + companion 121 + cli 13 | 811/811 green (04-gates-head-all-green.png, logs/gate-*.txt) |
Base-side red is behavioral (missing API), not an import error. The flip control is clean: the PR leaves every package.json/package-lock.json untouched (0-line diff), and the only internal dependency the flipped test resolves (@qwen-code/sdk, realpath packages/sdk-typescript) is byte-identical between base and head (0-line diff on both the PR side and the base-move side).
Corrections
None.
Findings
No new blocking findings. One new non-blocking observation, plus the two carried Suggestions (status table above).
S3 — workflow edit approvals wrap their diff as text, so they never reach the native diff bridge (Suggestion, non-blocking)
The main approval path emits the structured diff block (Session.ts:11660 → permissionUtils.ts:155-162), but the workflow-approval path builds its content with a ternary that wraps fileDiff as a text block instead (Session.ts:2597-2609). Both client predicates (transcriptAdapter.ts:60-73, EmbeddedApp.tsx:271-293) therefore see no diff for workflow edit approvals: no native diff opens and the approval is handled by the permission card only. This is fail-safe (the card flow resolves the permission correctly) and outside the PR's stated scope, but it means the same edit renders through two different approval UXes depending on the path. If the native diff flow is meant to cover workflow edits too, the producer should reuse buildPermissionRequestContent; if not, the asymmetry is worth a comment. Not a merge condition.
M-D2 survival classification — redundant defence, not a coverage gap with behavioral exposure
Dropping request.hasDiffPreview !== true from the shell's requestId gate (App.tsx:9167) survived the full 592-test App suite. The clause is the shell-side backstop against the host and shell diff predicates disagreeing; the divergence probe (below) shows they agree on 17/17 shapes, and the host only mints votes for requestIds it registered from its own predicate, so the clause cannot fire while both predicates match. Classification per the mutation matrix rules: redundant defence — correct as it stands; the shipped suite pinning nothing along this axis is expected, not a defect. The fixture that would pin it is a content shape the two predicates judge differently, and the probe shows none exists among the shapes the producer can emit.
Predicate-divergence wire oracle (new this round)
03-predicate-divergence-17of17.png: the host predicate was sliced verbatim out of the shipped EmbeddedApp.tsx and the shell predicate exercised through the real exported extractPendingPermission; 17 corpus cells (full diff, creation, deletion, no-texts, non-string path, missing path, empty-string texts, case-flipped type, ACP content_block wrapper, nested content key, text block, null/string items, empty/absent/non-array content, missing toolCall) — 17 agree / 0 diverge (harness/divergence-probe.mjs, logs/divergence-probe.txt).
Mutation matrix at HEAD
As printed: 02-mutation-matrix-round5.png.
| Mutant | Guard | Suite | Result |
|---|---|---|---|
| M-D1 | delta: structured content → hasDiffPreview |
App structured test | KILLED — expected false to be true |
| M-D3 | delta: allow_once-first ladder (requestId path) | App structured test | KILLED — submitted proceed_always |
| M-D2 | delta: hasDiffPreview clause in requestId gate |
full App 592 | SURVIVED — redundant defence (17/17 agreement) |
| M9 | legacy fallback in qwen.diff.accept |
extension.test.ts | KILLED — 1 red / 14 pass (F1a) |
| M5 | isEditToolName lowercases internally |
ToolGroup.test.tsx | KILLED — 1 red / 84 pass (F1b) |
| M6 | source guard window.parent conjunct |
EmbeddedApp.test.tsx | KILLED — 2 red / 16 pass (F2) |
| M7 | source guard requestId conjunct | EmbeddedApp.test.tsx | SURVIVED — 18/18 (S2) |
| M-C1 | allow_once present in edit wire options | permissionUtils.test.ts | KILLED — 1 red / 12 pass |
Positive controls: unmutated head gates 811/811 green; the M-D2 and M7 survival runs are green suites under the mutant (592/592, 18/18), proving the harness executed the suites.
Not covered
- Real VS Code host behavior. The container cannot run VS Code; the
vscodeAPI boundary is mocked (package convention). The premise that real host-relayed deliveries always carryevent.source === window.parentremains unverified in-container; the PR states no OS was manually tested. - Per-commit attribution. Depth-2 checkout: the delta's parent (
7ecdea9ca7) is unreachable; only the aggregateHEAD^1..HEADdiff was verified. The delta's file scope is inferred from its message, the new test, and the structured-detection chain it adds; the mutation cells (M-D1/M-D3) isolate its behavioral contribution regardless. - Round-3/4 end-to-end approval-loop harness and probes P1/P2 were not rebuilt this round (closure changed, so they could not be carried). The central claim's load-bearing proof is this round's A/B flip + M-D1/M-D3 instead; S2's behavior-correct half is inspection of both validation sites (
EmbeddedApp.tsx:691-693,App.tsx:9166). - Reviewer Test Plan steps 1–5 are real-host steps. Step 5's stale-session aspect (
webShellSessionChangedclearswebShellPermissionOwners,WebViewProvider.ts:1949) still has no shipped test (grep ofWebViewProvider.test.tsfinds nowebShellSessionChangedcase) — fail-closed behavior, carried note, not a finding. - Web-shell suites beyond App/ToolGroup (sidebar workspace-overview tests added by the base move): the PR does not touch those sources; the merged
App.tsxwas verified to contain both sides' hunks (gitModeIntentMustReset×2 base-side,respondToPendingPermission×4 PR-side) and the base-side App tests pass inside the 592/592 gate. - Workflow approval native-diff coverage — see S3.
- Playwright e2e, VSIX packaging, real-host screenshots (the PR states none were run).
Methodology
Environment: CI verify container (node:22-bookworm, node v22.23.2), merge-ref checkout at depth 2; npm ci/npm run build pre-run at HEAD (the pre-run build is the typecheck gate). Verified head git rev-parse HEAD^2 = 4fee90a68b583392ae058a53ade8cc93515a19a9. Host load was low this round (load1 ≈ 1 on 64 cores), unlike round 4's contention; all vitest runs were sequential regardless. Base worktree at HEAD^1 (fca2acd5) under tmp/base-tree, reusing the root node_modules via walk-up plus a symlink of the package-local (third-party-only) node_modules; the PR leaves all manifests untouched and the one internal dependency resolves to bytes identical on both arms (diffs cited above). The flip copied the head App.test.tsx bytes into the base tree. Mutants applied by exact-string single-hunk replacement (harness/apply-mutant.mjs, fails loud unless exactly one occurrence) and restored via git checkout after each run (working tree confirmed clean after every restore and at the end). The divergence probe transpiles the shipped transcriptAdapter.ts with esbuild and slices the host predicate verbatim out of the shipped EmbeddedApp.tsx source, so both predicates under test are the shipped bytes. Assertion counts come from harness/summary-assertions.mjs (14 log-parsed checks, ANSI-stripped) plus the probe's 17 cell comparisons = 31. Evidence captures produced by node scripts/verify-capture.mjs. The base worktree is removed after this round (git worktree remove --force).
Evidence images
01-ab-structured-vote-head-vs-base.png— A/B cells: green at head, TypeError on base.02-mutation-matrix-round5.png— the matrix as printed.03-predicate-divergence-17of17.png— host vs shell predicates, 17/17 agree.04-gates-head-all-green.png— 811/811 head gates.
Harness scripts and raw logs live beside this report in tmp/pr10534-verify-20260831-045645/.
— Qwen Code · sandboxed verification
Flakiness gate log
rounds=5 files=9 skipped=0
file packages/cli/src/acp-integration/session/permissionUtils.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/session/permissionUtils.test.ts
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/handlers/FileMessageHandler.test.ts: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/webview/handlers/FileMessageHandler.test.ts
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/cli/src/acp-integration/session/permissionUtils.test.ts: PPPPP
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/handlers/FileMessageHandler.test.ts: 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: 9 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/acp-integration/session/permissionUtils.test.ts: P (exit 0)
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/handlers/FileMessageHandler.test.ts: 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/cli/src/acp-integration/session/permissionUtils.test.ts: 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/handlers/FileMessageHandler.test.ts: 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/cli/src/acp-integration/session/permissionUtils.test.ts: 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/handlers/FileMessageHandler.test.ts: 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/cli/src/acp-integration/session/permissionUtils.test.ts: 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/handlers/FileMessageHandler.test.ts: 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/cli/src/acp-integration/session/permissionUtils.test.ts: 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/handlers/FileMessageHandler.test.ts: 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
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 6a": run App.test.tsx "submits allow_once for a native structured edit approval accept" at HEAD (blocked by absent node_modules, not by the call ceiling — environm…; "agent 3c": run the new test submits allow_once for a native structured edit approval accept (packages/web-shell) — the review worktree has no node_modules (vitest cannot….
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/App.tsx:9118 — [review] the unbound respondToPendingPermission overload (allow_always kind fallback, option-id substring heuristic, the whole reject mapping, and the isAskUserPermission guard) has no test anywhere …
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
⏸️ Deferring to @chiga0 — triage remains clean on 中文说明⏸️ 转交 @chiga0 —— triage 对 — Qwen Code · qwen3.8-max Reviewed at |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 36 passed · 0 failed · 36 total Flakiness gate: 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：36 通过 · 0 失败 · 36 总计 抖动门： Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) — round 6, follow-up Ran the PR in an isolated, token-free container: A/B flip against the base build, full live mutation matrix, predicate-divergence wire oracle, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 36 passed · 0 failed · 36 total (verified head 中文 — 判定：✅ 通过 · 可合入（agent 判定，第六轮跟进）
Previous-finding status (re-measured at head
|
| # | Finding (round 5) | Severity | Status at 4fee90a6 |
|---|---|---|---|
| F1a | extension.test.ts gate test red at head |
Blocker | fixed (re-run) — gate green 15/15; mutant M9 (legacy fallback dropped) re-killed live by expected "spy" to be called with arguments: [ 'allow' ], 1 red / 14 pass (logs/m9-legacy-fallback.txt) |
| F1b | ToolGroup.test.tsx lock test red |
Blocker | fixed (re-run) — gate green 85/85; mutant M5 (no lowercase in isEditToolName) re-killed live by the mixed-case lock row, 1 red / 84 pass (logs/m5-lowercase.txt) |
| F2 | requestId-match disjunct bypassed the decision source check | Suggestion | fixed (re-run) — shipped conjunction still at EmbeddedApp.tsx:691-693; mutant M6 (drop window.parent conjunct) re-killed live by both source-guard tests, 2 red / 16 pass (logs/m6-source-guard.txt) |
| F3 | two shipped tests could not kill mutants along their axis | Reporting | fixed (re-run) — both axes re-killed live (M5, M9); the delta's structured test additionally re-kills M-D1 and M-D3 along its own axes |
| S1 | stacked approvals: vote on a non-front permission diff dropped without notice | Suggestion | stands — re-inspected at this head: WebViewProvider.ts:2415-2436 posts only when an owner webview matches the requestId, then returns unconditionally; a vote for an unowned requestId produces zero messages and no notice. Fail-closed (nothing resolves, the stacked diff stays open); the shell-layer voteNotApplied notice fires only after a vote reaches the shell |
| S2 | requestId-validation axis unpinned at both layers | Suggestion | stands — mutant M7 (drop requestId conjunct) re-applied live and survived the shipped EmbeddedApp suite 18/18 (logs/m7-requestid-conjunct.txt). Behavior correct by inspection: conjunct present at EmbeddedApp.tsx:691-693 and the shell re-validates request.id !== requestId at App.tsx:9166. Coverage gap, not a merge condition |
| S3 | workflow edit approvals wrap their diff as text, never reach the native diff bridge | Suggestion | stands — re-inspected: the workflow path's ternary (Session.ts:2597-2613) wraps fileDiff as {type:'content', content:{type:'text'}}; probe cells 09/10 show both predicates judge that shape false, so workflow edits resolve via the permission card only. Fail-safe, outside the PR's stated scope |
Central claim + A/B
Central claim: a native Accept/Reject vote on a structured edit approval — a pending permission whose tool-call content carries a {type: 'diff', path, oldText|newText} block — resolves that exact request through the shell's respondToPendingPermission(requestId, decision), submitting allow_once (never allow_always) even though the wire order puts allow_always first. Secondary claims: (a) the rest of the bridge (source guard, owner map, read-only diffs, legacy fallback) holds; (b) the host and shell diff predicates agree on every shape the producers can emit.
The wire shape is not a fixture invention: the producer is buildPermissionRequestContent (packages/cli/src/acp-integration/session/permissionUtils.ts:130), called by the main approval path (Session.ts:11660) and the sub-agent path (SubAgentTracker.ts:245); the workflow path (Session.ts:2613) is the S3 exception.
| Cell | Production | Oracle (scripted) | Result |
|---|---|---|---|
| A/B head: structured-edit vote | HEAD 4fee90a6 |
App structured test: resolved === true, submitPermission('req-1','proceed_once') |
green (logs/ab-head-cell.txt, 01-ab-head-cell-green.png) |
| A/B base: same test bytes | base fca2acd5 (HEAD^1) |
predicted red: no vote endpoint | red as predicted — TypeError: shellApi?.respondToPendingPermission is not a function (logs/ab-base-cell.txt, 02-ab-base-cell-typeerror.png) |
M-D1: structured detection severed ('diff' → 'diffx') |
head + mutant | delta-isolation: vote must be declined | KILLED — expected false to be true on resolved (logs/md1-structured-test.txt) |
| M-D3: allow_once ladder swapped on requestId path | head + mutant | escalation guard | KILLED — mutant submitted proceed_always (logs/md3-ladder-swap.txt) |
| M-C1: allow_once removed from edit wire options (cli) | head + mutant | wire-order pin | KILLED — 1 red / 12 pass, toEqual mismatch (logs/mc1-wire-order.txt) |
M5: isEditToolName lowercase removed |
head + mutant | mixed-case lock row | KILLED — 1 red / 84 pass (logs/m5-lowercase.txt) |
M9: legacy fallback dropped in qwen.diff.accept |
head + mutant | extension gate | KILLED — 1 red / 14 pass, spy 0 calls (logs/m9-legacy-fallback.txt) |
M6: window.parent conjunct dropped |
head + mutant | source-guard tests | KILLED — 2 red / 16 pass (logs/m6-source-guard.txt) |
| M7: requestId conjunct dropped | head + mutant | EmbeddedApp suite | SURVIVED 18/18 — S2 coverage gap, fail-closed (logs/m7-requestid-conjunct.txt) |
M-D2: hasDiffPreview clause dropped from shell gate |
head + mutant | full App suite | SURVIVED 592/592 — redundant defence, see Findings (logs/md2-guard-drop-full-app.txt) |
| Gates at head | HEAD 4fee90a6 |
App 592 + ToolGroup 85 + companion 121 + cli 13 | 811/811 green (04-gates-head-all-green.png, logs/gate-*.txt) |
Matrix as printed: 03-mutation-matrix-round6.png.
The flip control is clean: the PR leaves every package.json/package-lock.json untouched (0-line diff); the base worktree's internal @qwen-code/* links were re-pointed into the base tree and asserted by realpath (readlink -f …/node_modules/@qwen-code/sdk → tmp/base-tree/packages/sdk-typescript); the two dists carried into the base tree (sdk-typescript, acp-bridge) come from packages with an empty git diff HEAD^1..HEAD, so head-built dist is behaviorally identical to base-built dist; the web-shell lib entry (daemon-react-sdk.js) was rebuilt in the base tree from base sources.
Corrections
None.
Findings
No new blocking findings. The three carried Suggestions stand (status table above); none regressed at this head.
M-D2 survival classification — redundant defence, not a coverage gap with behavioral exposure
Dropping request.hasDiffPreview !== true from the shell's requestId gate (App.tsx:9167) survived the full 592-test App suite, exactly as in round 5. The clause is the shell-side backstop against the host and shell diff predicates disagreeing; the divergence probe (below) shows they agree on 17/17 shapes, and the host only mints votes for requestIds it registered from its own predicate, so the clause cannot fire while both predicates match. Classification: redundant defence — correct as it stands; the fixture that would pin it is a content shape the two predicates judge differently, and the probe shows none exists among the shapes the producers emit.
Predicate-divergence wire oracle
05-predicate-divergence-17of17.png: the host predicate was sliced verbatim out of the shipped EmbeddedApp.tsx (permissionDiffPreview) and the shell predicate exercised through the real exported extractPendingPermission (harness/divergence-probe.mjs); 17 corpus cells (full diff, creation, deletion, no-texts, non-string path, missing path, empty-string texts, case-flipped type, ACP content_block wrapper, nested content key, text block, null/string items, empty/absent/non-array content, missing toolCall) — 17 agree / 0 diverge (logs/divergence-probe.txt).
Not covered
- Real VS Code host behavior. The container cannot run VS Code; the
vscodeAPI boundary is mocked (package convention). The premise that real host-relayed deliveries always carryevent.source === window.parentremains unverified in-container; the PR states no OS was manually tested. - Per-commit attribution. Depth-2 checkout (grafted at
4fee90a6): only the aggregateHEAD^1..HEADdiff was verified;git rev-list HEAD^1..HEAD^2returns 1 at the shallow boundary while the metadata lists 23 commits. - Reviewer Test Plan steps 1–5 are real-host steps. Step 5's stale-session aspect (
webShellSessionChangedclearswebShellPermissionOwners) still has no shipped test — fail-closed behavior, carried note, not a finding. - Round-3/4 end-to-end approval-loop harness was not rebuilt; the central claim's load-bearing proof is this round's A/B flip plus the live M-D1/M-D3 cells.
- Playwright e2e, VSIX packaging, real-host screenshots (the PR states none were run).
- Flakiness gate (9 changed test files × 5 rounds) is run by the workflow separately; this round's gates are single runs.
Methodology
Environment: CI verify container (node:22-bookworm, node v22.23.2), merge-ref checkout at depth 2; npm ci/npm run build pre-run at HEAD (the pre-run build is the typecheck gate). Verified head git rev-parse HEAD^2 = 4fee90a68b583392ae058a53ade8cc93515a19a9. Base worktree at HEAD^1 (fca2acd5) under tmp/base-tree, internal @qwen-code/* links re-pointed into the base tree (realpath asserted), package-local third-party node_modules symlinked, sdk-typescript/acp-bridge dists carried from head (empty-diff proof cited), web-shell lib rebuilt in the base tree. The flip copied the head App.test.tsx bytes into the base tree and ran the single structured test by name filter on both arms. Mutants applied by exact-string single-hunk replacement (harness/apply-mutant.mjs, fails loud unless exactly one occurrence) and restored via git checkout after each run (working tree confirmed clean after every restore and at the end). The divergence probe transpiles the shipped transcriptAdapter.ts with esbuild and slices the host predicate verbatim out of the shipped EmbeddedApp.tsx source, so both predicates under test are the shipped bytes. Assertion counts come from harness/summary-assertions.mjs (18 log/git-parsed checks, ANSI-stripped) plus the probe's 17 cell comparisons and 1 probe summary check = 36. Evidence captures produced by node scripts/verify-capture.mjs. The base worktree is removed after this round (git worktree remove --force).
Evidence images
01-ab-head-cell-green.png— A/B head cell: structured vote green,proceed_oncesubmitted.02-ab-base-cell-typeerror.png— A/B base cell: same test bytes,TypeError … is not a function.03-mutation-matrix-round6.png— the matrix as printed (6 killed, 2 classified survivors).04-gates-head-all-green.png— 811/811 head gates.05-predicate-divergence-17of17.png— host vs shell predicates, 17/17 agree.
Harness scripts and raw logs live beside this report in tmp/pr10534-verify-20260831-101544/.
— Qwen Code · sandboxed verification
Flakiness gate log
rounds=5 files=9 skipped=0
file packages/cli/src/acp-integration/session/permissionUtils.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/session/permissionUtils.test.ts
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/handlers/FileMessageHandler.test.ts: (cd packages/vscode-ide-companion) npx --no-install vitest run ./src/webview/handlers/FileMessageHandler.test.ts
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/cli/src/acp-integration/session/permissionUtils.test.ts: PPP
packages/vscode-ide-companion/src/commands/index.test.ts: PPI
packages/vscode-ide-companion/src/diff-manager.test.ts: PPP
packages/vscode-ide-companion/src/extension.test.ts: PPP
packages/vscode-ide-companion/src/webview/EmbeddedApp.test.tsx: PPP
packages/vscode-ide-companion/src/webview/handlers/FileMessageHandler.test.ts: PPP
packages/vscode-ide-companion/src/webview/providers/WebViewProvider.test.ts: PPP
packages/web-shell/client/App.test.tsx: PPP
packages/web-shell/client/components/messages/ToolGroup.test.tsx: PP
verdict: timeout
summary: 1 invocation(s) ended in a timeout/signal exit — infrastructure, not test nondeterminism, so these rounds carry no flakiness signal
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/acp-integration/session/permissionUtils.test.ts: P (exit 0)
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/handlers/FileMessageHandler.test.ts: 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/cli/src/acp-integration/session/permissionUtils.test.ts: 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/handlers/FileMessageHandler.test.ts: 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/cli/src/acp-integration/session/permissionUtils.test.ts: P (exit 0)
round 3 · packages/vscode-ide-companion/src/commands/index.test.ts: I (exit 124)
--- output tail · round 3 · packages/vscode-ide-companion/src/commands/index.test.ts ---
�[1m�[46m RUN �[49m�[22m �[36mv3.2.7 �[39m�[90m/__w/qwen-code/qwen-code/packages/vscode-ide-companion�[39m
�[32m✓�[39m src/commands/index.test.ts �[2m(�[22m�[2m8 tests�[22m�[2m)�[22m�[32m 11�[2mms�[22m�[39m
�[2m Test Files �[22m �[1m�[32m1 passed�[39m�[22m�[90m (1)�[39m
�[2m Tests �[22m �[1m�[32m8 passed�[39m�[22m�[90m (8)�[39m
�[2m Start at �[22m 09:56:13
�[2m Duration �[22m 848ms�[2m (transform 246ms, setup 0ms, collect 219ms, tests 11ms, environment 0ms, prepare 198ms)�[22m
Session terminated, killing shell... ...killed.
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/handlers/FileMessageHandler.test.ts: 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)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Full review at 427dc2e — no findings. The stale bot CHANGES_REQUESTED (predating 4fee90a and the main merge) was dismissed; all 35 threads are resolved.
Verified the approval chain end to end:
- DiffManager now keys dedupe/focus/reuse on writability AND permissionRequestId, so a read-only approval diff can never be deduped onto or refocused as a writable twin, and two permission requests never share a diff; readOnly diffs skip setActiveEditorWriteableInSession since web-shell approvals cannot round-trip edits.
- Votes from the diff editor are triple-bound by request id: WebViewProvider routes only to the webview owning that exact requestId (webShellPermissionOwners), the embedded shell accepts the message only from window.parent with a requestId matching its managed diff, and App.respondToPendingPermission re-validates request.id plus hasDiffPreview/hostOwnsEditDiffPreview before submitting — with the exact-binding path using only allow_once/reject_once (no fuzzy fallback), and a visible host notice when the shell isn't mounted so the vote cannot die silently.
- closeDiff is scoped per permissionRequestId; getTargetViewColumn reuses the existing diff's tab group with sane fallbacks.
CI: 21 checks pass; only the bot review-pr job still pending. Self-PR: GitHub blocks self-approval, so this needs another maintainer's approve to merge.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings. Approved.
Reviewed with AI assistance.
What I checked
Diff scope: 23 files, +1571/-142. Three subsystems: vscode-ide-companion (DiffManager, commands, extension, EmbeddedApp, WebViewProvider, FileMessageHandler), web-shell (App, ToolGroup, SubAgentPanel, transcriptAdapter, types, customization), and cli (permissionUtils).
Core flow (end-to-end verified against the code path):
- Web shell sees a pending edit permission with a diff →
updateTranscriptpostsopenDiff { source: 'web-shell', requestId }+webShellPermissionState { pending: true, requestId } FileMessageHandleropens the diff withreadOnly: true+permissionRequestIdvia theshowDiffCommandDiffManager.showDiffrecordsreadOnly=true, permissionRequestIdinDiffInfo; skipssetActiveEditorWriteableInSessionso the user can't silently lose edits- VS Code user presses Ctrl+S / Accept →
qwen.diff.acceptfires with the qwen-diff-scheme URI →getPermissionRequestId(docUri)returnsreq-X→provider.respondToPendingPermission('allow', { fromDiffEditor: true, permissionRequestId: 'req-X' }) WebViewProvider.respondToPendingPermissionlooks up the owning webview inwebShellPermissionOwnersand postswebShellPermissionDecision { decision: 'allow', requestId: 'req-X' }EmbeddedAppmessage handler validatesevent.source === window.parent+requestId === webShellPermissionRequestIdRef.current→ callsshellRef.current.respondToPendingPermission(requestId, 'allow')- App.tsx
respondToPendingPermissionvalidatesrequest.id === requestId && hostOwnsEditDiffPreview && request.hasDiffPreview === true→ submits viasessionActions.submitPermission
Security design — two independent validation layers:
- Layer 1 (EmbeddedApp):
event.source === window.parentblocks nested iframes;requestId === webShellPermissionRequestIdRef.currentblocks stale/crafted IDs. Tests "ignores decisions posted by a nested iframe window" and "ignores decisions delivered without a source window" both pin these gates. CI green. - Layer 2 (App.tsx
respondToPendingPermission):request.id !== requestIdrejects wrong-request;hostOwnsEditDiffPreviewmust be true;request.hasDiffPreview !== truerejects non-diff permissions. This second layer ensures the shell side can't be tricked even if the host message were spoofed.
readOnly + dedup consistency:
hasExistingDiff and focusExistingDiff both check readOnly (and permissionRequestId), so a writable IDE-mode diff and a read-only web-shell diff for the same content never share a slot. recentlyShown key is (path, old, new) without readOnly — harmless because recentlyShown is only consulted after hasExistingDiff matches, and mismatched-readOnly diffs don't match. DiffManager tests ("opens a fresh diff instead of reusing a writable twin for a read-only request" and vice versa) cover this. CI green.
Single-permission-at-a-time (intentional behavior change):
updateTranscript now uses blocks.find(...) (first pending) rather than iterating all pending. This keeps webShellPermissionRequestIdRef unambiguous — only one native diff is open at a time, ownership is clear, and stacking is handled by rotating as each resolves. Test "keeps host permission ownership in sync while pending stays true" confirms the rotation.
ToolLine locking is safe under hasDiffPreview=false:
locksPendingEditApproval requires approval.hasDiffPreview === true. When hasDiffPreview is false (daemon didn't include a diff content block), the lock doesn't activate, the tool line stays expandable, and the user can vote via the in-line web-shell UI. No stuck state possible.
Backward compatibility:
respondToPendingPermission(choice, context?)—contextis optional; existing callers without it still hit the legacypendingPermissionResolvepath. WhenwebShellPermissionOwners.size > 0butpendingPermissionResolveis null, calling without context returns early — no spurious web-shell vote.hasPendingPermission()broadened: returns true whenwebShellPermissionOwners.size > 0. The accept/cancel handler guards withif (!isManagedDiff) continueso non-diff keystrokes can't triggerrespondToPendingPermission.permissionUtils.ts— no implementation change, only a regression test pinning the option order.
WebViewProvider ownership cleanup verified complete:
- View dispose:
webShellPermissionOwners.delete(webview)at line 951 - Panel dispose: clears all non-attachedWebview entries at lines 209-212
webShellSessionChanged: delete at line 1954webShellReady: delete at line 1969webShellPermissionState { pending: false }: delete at line 1949
All five paths covered, test "clears the pending flag when the hosting view is disposed" passes. CI green.
Cross-check against existing review comments
R1-1 (original + fix-induced): The current code uses event.source === window.parent (positive equality check, not !== null). The test suite covers both the iframe and null-source gates. Cannot reproduce the reviewer's claimed vulnerability at this head.
R1-2 (edits silently discarded): Addressed by readOnly: true → setActiveEditorWriteableInSession is NOT called for web-shell diffs. The right side stays locked in VS Code's session.
R1-3 (fix-induced: uncontexted vote resolves web-shell approval): Analyzed the path: respondToPendingPermission('allow') without context skips the web-shell route (needs context?.fromDiffEditor && context.permissionRequestId), then checks pendingPermissionResolve. When only web-shell owners exist and no ACP promise is pending, pendingPermissionResolve is null → the call is a no-op. No web-shell vote fires. Cannot confirm this is a real bug at this head.
Critical "4th argument breaks tests" (commands/index.test.ts): Fixed — the tests were updated to pass { readOnly: false, permissionRequestId: undefined } and a new readOnly: true test was added.
R3-1 (hasDiffPreview gate): The gate is intentional: only approvals with a known diff preview get the native-diff path. When hasDiffPreview is false, ToolLine stays expandable and web-shell UI remains the interaction surface — no stuck state.
Suggestion-level coverage gaps (R3-2, R3-3, R3-4 etc.): Valid observations — defined permissionRequestId through the showDiff/closeDiff command hops lacks a dedicated end-to-end test. Not blocking; the individual unit tests for showDiff and closeDiff cover the parameter forwarding.
R1-4 (no test coverage for the permission bridge): Refuted. WebViewProvider.test.ts adds 170 lines of bridge tests covering: routing to request owner, cancel as reject, unknown request ID, fromDiffEditor=false gate, pre-ownership state, view-dispose cleanup, hasPendingPermission tracking. EmbeddedApp.test.tsx adds 150 lines covering: host-relayed decisions, nested iframe isolation, null source, handleShellError on resolved-false. These shipped in the current head.
CI
All substantive checks pass: ubuntu unit tests (25m), Integration Tests, web-shell E2E Smoke, Real daemon E2E, Desktop Shell (ubuntu + windows).
macOS/Windows unit tests: SKIPPED — no platform-specific logic in this change.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Gate APPROVE at head 427dc2e6 on the review conclusion. Verified in the diff: (1) the native-diff bridge is id-bound end to end — WebViewProvider routes accept/reject only through webShellPermissionOwners keyed by the exact requestId, the web-shell side re-validates the id before voting, and the owner map is cleared on session change, webview ready, and panel/view dispose, so a stale or stacked diff cannot resolve another session's approval; (2) diff reuse now matches on (path, old, new, readOnly, permissionRequestId), preventing a writable twin being refocused for a read-only approval; (3) the accept/cancel command narrowing (only managed diffs auto-resolve permissions) removes a pre-existing accidental-approve path rather than adding one; (4) compact pending-Edit rows are host-flag-gated (hostOwnsEditDiffPreview, default false) so non-VS Code surfaces are unchanged. All 35 threads resolved, both earlier CHANGES_REQUESTED rounds fixed and dismissed, CI 22 green with only the review-pr pipeline running. Note for follow-up: the PR's own manual VSIX screenshot pass is still pending — worth completing before the next vscode release cut, not a merge blocker given the unit coverage of the state machine (session-switch, stale-diff, read-only, and timer-pause cases are all pinned).
中文：在 head 427dc2e6 按 Review 结论通过。四点已核：桥接全链路按 requestId 精确绑定并在会话切换/webview 释放时清账，陈旧 diff 无法误批他人审批；diff 复用五元组匹配（含 readOnly 与 requestId），不会把只读批复成可写；accept/cancel 命令收窄为仅托管 diff 才自动投票，反而消除了旧的误批路径；紧凑行由宿主开关控制、默认关，不影响非 VS Code 面。35 条线程全关闭、两轮 RC 均修复后 dismiss，CI 22 绿仅 review-pr 流水线在跑。备注：作者自述手工 VSIX 截图验证未跑，建议在下次扩展发版前补上——状态机分支均有单测钉住，不阻塞合入。
4 participants
What this PR does
This restores the native VS Code Diff approval flow after #9811 moved tool permissions from the legacy ACP-backed UI to WebShell.
When a WebShell Edit permission opens a native VS Code diff, the editor's Accept and Reject commands now resolve the corresponding WebShell permission as allow once or reject. The bridge is active only while an unresolved file-diff permission exists, clears its state across session and panel lifecycle changes, and preserves the legacy ACP fallback.
Pending Edit tools remain as compact transcript rows while approval is required instead of opening a duplicate or empty detail card. Their running timer also pauses while waiting for the user's decision. The WebShell permission card remains available for choices such as Allow All Edits.
Why it's needed
After the WebShell daemon cutover, the native Diff commands still targeted the legacy ACP permission resolver. Accepting or rejecting the editor diff could therefore close the diff without unblocking the WebShell-owned permission request.
Reviewer Test Plan
How to verify
Evidence (Before & After)
The final real-host screenshot pass has not been run yet. Evidence will be added after installing a VSIX built from this branch.
Tested on
Risk & Scope
Linked Issues
Follow-up to #9811.