fix(desktop): consolidate 0.1.1 regressions - #8896
Merged
Merged
Conversation
…ease-audit # Conflicts: # packages/webui/src/daemon/session/DaemonSessionProvider.test.tsx # packages/webui/src/daemon/session/DaemonSessionProvider.tsx
Collaborator
Author
macOS Desktop 0.1.1 regression reportTested the installed arm64 candidate (
Installed app:
Concurrent session evidence:
Not covered locally: Developer ID notarization, the real 0.1.0 → 0.1.1 updater path, Windows/Linux packages, and sleep/wake recovery. The local signature is ad-hoc, so Gatekeeper assessment is expected to reject it even though structural signature verification passes. |
Collaborator
Collaborator
|
Re-run triggered by
Moving on to code review. 🔍 中文说明由
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewRe-run at No Critical findings stand at this head. The items that blocked earlier rounds, verified against the head code:
The routed-virtual-adapter item, closed out with a correction on the record. My prior pass blocked on "filter by interface type — the Non-blocking follow-ups (none new to this head; listed so nothing is silently dropped):
Also correcting one note from the Test evidence — the PR's own CI (unattended run; PR code not executed here)
One row per check name (latest run at this head); skipped checks omitted. / 每个检查名一行（取本 head 最新一次运行），省略 skipped。 Every check on Sandboxed verification for what CI cannot reach: 中文说明代码审查head 本 head 无 standing Critical。 早先阻塞项按 head 代码逐条核实：关闭/加载双重 cancel——仅在 agent 未确认关闭时发收尾 cancel，错误路径保持保守（不确定仍发 cancel），两条路径均有新测试钉住；click-suppression 泄漏——清除位于接受守卫之前且以 routed 虚拟网卡一项，带一处更正地关闭。 上次我以"按接口类型过滤"阻塞，但该补救在本依赖上不可实现：此处消费的 crate 表面（network-interface 2.0.5）只导出 name/addr/mac_addr/internal，类型信息仅内部用于环回检测、不外露；要按类型过滤须 fork 或替换依赖，超出回归整合 PR 的范围。落地的按平台名称启发式是可用缓解且是严格改进：未过滤时 routed VPN 地址会静默胜出，过滤后具名 VPN/TAP/Hyper-V/vEthernet 网卡被排除，未命中名称表的主机回退到 PR 前行为（不会更差），仍受同子网 403 门与 Local Control 显式未加密、需手动开启的约束。提出该 Critical 的 @doudouOUC 已关闭 thread 并在本 head 批准；我采纳该判断，不再以依赖不支持的补救门禁本 PR。残余（罕见命名的 Windows VPN 网卡）值得跟踪记录，不构成门禁。 非阻塞跟进（均非本 head 新增，列出以免遗漏）：webui 以 测试证据（无人值守运行；此处未执行 PR 代码）（CI 表格见上方标记区域：本 head 所有检查最新一次运行均为 success，含 Linux/Windows 两侧 Desktop Shell 与完整单测套件；早先 cancelled 为被取代的首次尝试。）沙箱验证： — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — every Critical raised across four review rounds is fixed and pinned, and the one residual risk is bounded, disclosed, and accepted by the reviewer who raised it; the remaining items are Minor-level follow-ups, named in Stage 2. Stepping back: this PR consolidates six Desktop 0.1.1 regressions plus a packaged-runtime provenance guard, and it reads like maintenance work done properly — each fix is the smallest shape that closes its finding, and every behavioral fix carries a test that fails without it. My independent proposal and the diff converged on the same three mechanisms; where I'd have preferred something stronger (adapter-type filtering), the dependency doesn't expose the data, and I'm correcting my earlier insistence on that remedy rather than holding the PR to an impossible bar. The honest reservations, all non-blocking: the retryable-error detection leans on a prose suffix instead of the structured code — that coupling can silently undo the close/load fix in a future reword and deserves a follow-up; the packaged-app evidence in this thread is the author's local ad-hoc-signed build (a release dry-run will be the first real outing for the checksum-refresh ordering); and uncommonly-named Windows VPN adapters still bypass the name heuristic — pre-existing exposure for those hosts, bounded by the same-subnet 403 gate and by Local Control being opt-in unencrypted, worth a tracking note. On the gate itself: my account carries a Verdict: approve, pinned to the reviewed commit. 中文说明置信度：4/5 —— 四轮审查中提出的所有 Critical 均已修复并被测试钉住，唯一残余风险有边界、已披露、且由提出它的 reviewer 接受；其余为 Stage 2 列出的 Minor 级跟进项。 整体来看：本 PR 整合六项 Desktop 0.1.1 回归修复与打包运行时溯源校验，是形态端正的维护工作——每项修复都是闭合对应 finding 的最小形状，每项行为修复都有去掉它即失败的测试。我的独立方案与 diff 在三处机制上收敛；在我本想要更强方案（按网卡类型过滤）之处，依赖并不外露该数据——我更正此前对该补救的坚持，不以不可实现的标准门禁本 PR。诚实的保留项（均不阻塞）：可重试错误的识别依赖文案后缀而非结构化 code，该耦合可能在未来改写时静默撤销关闭/加载修复，值得跟进；线程中的打包应用证据是作者本地 ad-hoc 签名构建（release dry-run 将是 checksum 刷新顺序的首次真实运行）；罕见命名的 Windows VPN 网卡仍可绕过名称启发式——对这些主机属既有暴露，受同子网 403 门与 Local Control 手动开启、显式未加密约束，值得跟踪记录。 关于门禁本身：本账号在完全相同的 head 上带有一份早前 结论：批准，钉住所审 commit。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 2 render-shaping files:
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 Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Contributor
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-ci-bot
previously approved these changes
Aug 10, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
Collaborator
Author
Updated macOS Desktop 0.1.1 regression reportThis supersedes the voice result in my earlier report. I rebuilt and installed a fresh arm64 candidate containing PR head
First-click recording state (packaged daemon renderer; no IP/QR or desktop background included):
Installed Desktop window:
Still outside this local candidate's proof: Developer ID notarization and first-run TCC identity, real sleep/wake recovery, the published |
doudouOUC
requested changes
Aug 10, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/desktop-release.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/desktop-release.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
Collaborator
Author
Final installed-candidate follow-upI rebuilt and installed one final arm64 Desktop
Voice state after the first post-release click (packaged daemon renderer with a fake microphone; app content only):
Final installed macOS window (captured by window ID, without the desktop background):
This remains an ad-hoc-signed local candidate. Developer ID/TCC identity, real sleep/wake, the published |
qwen-code-ci-bot
requested changes
Aug 10, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all planned checks completed within budget..
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/desktop-release.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all planned checks completed within budget.。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/desktop-release.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.9)
Collaborator
Author
|
@qwen-code /triage |
ytahdn
previously approved these changes
Aug 11, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
Approve. Comprehensive consolidation of Desktop 0.1.1 regressions. Each sub-fix has corresponding test coverage, and the CI workflow reordering (smoke after signing + checksum refresh) is correct.
Minor comments:
prepare-runtime.js—--refresh-checksumsarg parsing: If the next arg starts with--, it would be misinterpreted as a path. Consider checking!process.argv[refreshChecksums + 1].startsWith('--')before treating it as a path.bridge.ts:4242—Promise.racereturn value: Confirm that the transport-closed reject path resolves to an object with aclosedfield (or thatresponse['closed']readingundefinedand returningfalseis the intended behavior in that edge case).local-control.js— language detection:navigator.language.startsWith('zh')maps allzh-*(includingzh-TW) to simplified Chinese. Acceptable for now, but worth a comment.
Note: overlaps with #8884 — recommend merging #8884 first and rebasing this on top.
Collaborator
Author
|
@qwen-code-review-bot review and approve if no blockers |
Collaborator
|
|
Collaborator
Author
|
@qwen-code-review-bot review and approve if no blockers |
Collaborator
|
|
1 similar comment
Collaborator
|
|
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Collaborator
Author
|
@qwen-code-review-bot review and approve if no blockers |
chiga0
reviewed
Aug 11, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Re-Review at HEAD 87d09b10 (AI Generated)
PR: #8896 fix(desktop): consolidate 0.1.1 regressions
Previous QoderWork AI review: #pullrequestreview-4903539707 at 6f7be7f8
HEAD advanced: yes — 6f7be7f8 -> 87d09b10 (30 commits ahead, 0 behind; includes 01c4ff2f and a fresh main merge landed during this review pass)
All statuses below were verified against the actual file contents at 87d09b10 (contents/<path>?ref=87d09b10), not against commit messages or author claims.
Previous Findings Status
| # | Previous Finding | Status | Evidence at HEAD 87d09b10 |
|---|---|---|---|
| C1 | LocalNetwork sort_unstable(); dedup(); dedupes on (address, netmask), so the routed branch can pick a wider netmask and loosen the same-subnet 403 boundary |
Fixed | local_control.rs:563-564 now reads sort_unstable_by_key(|n| (n.address, std::cmp::Reverse(n.netmask))) + dedup_by_key(|n| n.address). Reverse on Ipv4Addr orders masks numerically descending, so dedup_by_key retains the narrowest mask per address — the conservative direction. Regression test added in 76e1c397 (choose_lan_ipv4 with /16 + /24 on the same address asserts /24 wins). The "multiple local networks" misjudgement is gone for the same reason. |
| C2 | Secondary loadSession guard after await refreshedReplayFieldsFor(...) still threw the default session_not_found, so the close/load race aborted session switching |
Fixed | bridge.ts:5123-5128 — the post-await isClosingOrAuthorizingClose(existing) branch was split out and now throws SessionNotFoundError(req.sessionId, 'The session is closing; retry after close completes', 'session_closing'), matching the pre-await guard at 5101-5106. Commit 76e1c397; bridge.test.ts was tightened from rejects.toBeInstanceOf(SessionNotFoundError) to rejects.toMatchObject({ code: 'session_closing' }). See NEW-1 below for the remaining sliver. |
| M1 | Stale connection.error persists across reconnect after removing the synthetic Reconnecting in ${delayMs}ms message |
Still present | DaemonSessionProvider.tsx:2943-2946 — the backoff setConnection still spreads ...current and only overwrites status. The preceding retriable-error branch (:2908-2919) sets error: message + errorStatus, so both survive into the disconnected reconnect window unchanged. |
| M2 | sign -> --refresh-checksums -> smoke:runtime ordering not pinned in test-release.js |
Still present | scripts/test-release.js at HEAD contains zero occurrences of refresh-checksums, Refresh bundled runtime checksums or Verify bundled runtime. The only ordering asserts are :190-197 (Prepare bundled runtime < Sign bundled vendor binaries (macOS) < Build desktop installers). test-release.js is not in this PR's diff at all. |
| N1 | navigator.language.toLowerCase().startsWith('zh') and zh-TW/zh-HK |
Still present (severity re-scoped) | bootstrap/local-control.js:49 unchanged. On re-examination the original wording was imprecise: 'zh-tw'.startsWith('zh') is true, so Traditional-Chinese locales are not missed — they are silently served the Simplified zh-CN bundle. Downgraded to Nit; still worth an explicit locale map. |
Score: 2 Critical fixed, 2 Minor still open, 1 Nit still open.
New Findings on the Commits Since 6f7be7f8
Fresh pass over 76e1c397, f3dd3ab7, ef1f96b7, 01c4ff2f and the main merges.
| # | Severity | Location | Finding |
|---|---|---|---|
| NEW-1 | Minor | packages/acp-bridge/src/bridge.ts:5120-5122 |
Residual sliver of C2. The byId.get(req.sessionId) !== existing guard still throws a bare session_not_found. If the close completes (rather than merely starts) inside the refreshedReplayFieldsFor / resolveHistoryAnchorRecordId await window, the entry is already removed from byId, so the client gets a non-retryable error even though the session is durable and the cold-restore path 30 lines below would have reloaded it successfully. Either fall through to the cold-restore path, or tag it session_closing like its sibling guard. |
| NEW-2 | Minor | packages/webui/src/daemon/session/DaemonSessionProvider.tsx:4734-4744 |
The whole C2 fix is consumed through isClosingSessionLoadError, which detects the retryable case by body.error.endsWith('The session is closing; retry after close completes') — a literal string coupling to the bridge's prose, not to the structured code: 'session_closing' that SessionNotFoundError now carries and that bridge.test.ts asserts on. Any future reword of that sentence silently reverts C2 with green tests on both sides. Match on the code (or export a shared constant). Three call sites now depend on it (:2767, :3500). |
| NEW-3 | Nit | packages/desktop-shell/src-tauri/src/local_control.rs:495-501 |
Windows arm of is_virtual_interface matches friendly-name substrings (vEthernet, Hyper-V, VPN, TAP). If network-interface ever surfaces the adapter GUID ({4C1B...}) instead of the friendly name on Windows, the entire arm becomes a silent no-op and VPN adapters regain the ability to win the routed match. excludes_virtual_interfaces uses a synthetic new_afinet name, so it cannot catch that. |
| NEW-4 | Nit | packages/desktop-shell/src-tauri/src/local_control.rs:22 |
LocalNetwork still derives Ord/PartialOrd, but nothing uses them now that choose_lan_ipv4 sorts with an explicit key. Leaving the derive in place is exactly what let C1 happen; dropping it makes a future sort_unstable() a compile error rather than a silent security regression. |
| NEW-5 | Nit | packages/desktop-shell/src-tauri/src/local_control.rs:477 |
macOS arm excludes bridge*, which is also the Internet Sharing / Thunderbolt Bridge interface — a host running Internet Sharing loses Local Control availability. Probably acceptable, but it is a silent behaviour change beyond "exclude VPN adapters". |
Verified as genuinely safe (no finding):
f3dd3ab7"remove duplicatenetwork.containscheck" — confirmed the 403 same-subnet boundary is still enforced atlocal_control.rs:175-178(Forbidden (off-network)); only the redundant first copy beforeset_nonblockingwas removed. Security boundary intact.ef1f96b7moved theignoreNextClickRef.current = falsereset above the acceptance guards inVoiceButton.tsx:334-336. This re-raises an earlier "reset before guards" concern, but it is now correctly fenced byholdPointerIdRef.current === null, so a mid-hold secondary pointer cannot clear the flag, and a rejected pointerdown (no hold started) should clear it. Traced the tap / hold / release-outside / stop-click sequences — all land correctly. Not a finding..github/workflows/desktop-release.yml— the newRefresh bundled runtime checksums after signing (macOS)step (:360-363) carries the identicalif: runner.os == 'macOS' && inputs.dry_run == falseguard as the signing step (:328), so the dry-run lane skips both andVerify bundled runtime(:365, unguarded, all platforms) still sees consistent checksums. No Windows/Linux step mutates runtime binaries between prepare and smoke. Ordering itself is correct — M2 is only about pinning it.
Prior Inline Comment Audit
| Comment | Status |
|---|---|
C1 at local_control.rs:509 |
Obsolete — code replaced; the sort_unstable_by_key/dedup_by_key region is no longer even part of this PR's diff (it landed via the main merge of #8866). |
C2 at bridge.ts:5105 |
Obsolete — guard split and re-coded; region is likewise no longer in the PR diff after the main merge. |
M1 at DaemonSessionProvider.tsx:2946/2947 |
Still valid (now :2945). |
M2 at desktop-release.yml:361 |
Still valid. |
local_control.rs:1044 (yiliang114, platform-aware fixture) |
Resolved by 01c4ff2f — the fixture now picks utun3 / vEthernet (Default Switch) / tun0 per cfg!(target_os). |
local_control.rs:173 (blocking mode before rejection) |
Resolved — at HEAD the network.contains rejection sits after set_nonblocking(false). |
Note: because the branch merged main twice during this window, several previously-commented regions are now attributed to upstream rather than to this PR's diff. The code is present at HEAD either way; the audit above reflects HEAD content, not diff membership.
Final Verdict
COMMENT — not blocking, but not clean either. Both Criticals (C1 network-boundary widening, C2 close/load race) are genuinely fixed at HEAD with regression tests that pin the right invariants, and the previously-flagged spawn_proxy and fixture issues are resolved. What remains is two carried-over Minors (M1 stale reconnect error, M2 unpinned release-step ordering) plus two new Minors around the C2 contract (NEW-1 residual session_not_found, NEW-2 message-string coupling). None of these is a release blocker for 0.1.1, but NEW-2 in particular deserves a fix before this lands, since it can silently undo the C2 work later.
Cannot APPROVE while Minor-level findings remain open.
This review was generated by QoderWork AI
qwen-code-ci-bot
added a commit
that referenced
this pull request
Aug 11, 2026
qwen-code-ci-bot
added a commit
that referenced
this pull request
Aug 11, 2026
qwen-code-ci-bot
requested changes
Aug 11, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): PR #8896 consolidates Desktop 0.1.1 regression fixes (Voi...: none — all checks above completed within budget.; chunk 1: none — did not run the full acp-bridge/desktop-shell suites (only the targeted tests above), and did not inspect chunk 2/3 files by design.; PR #8896 consolidates Desktop 0.1.1 regression fixes (Voi...: none (~18 of ~41 calls used)..
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/desktop-release.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Test Plan (not a blocker): Contents/Resources/runtime/qwen-code/manifest.json — no such file or directory; npm run smoke:runtime — no package defines this script.
中文说明
未探索到全部深度（达到工具调用预算）：PR #8896 consolidates Desktop 0.1.1 regression fixes (Voi...：none — all checks above completed within budget.；chunk 1：none — did not run the full acp-bridge/desktop-shell suites (only the targeted tests above), and did not inspect chunk 2/3 files by design.；PR #8896 consolidates Desktop 0.1.1 regression fixes (Voi...：none (~18 of ~41 calls used).。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/desktop-release.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
Test Plan（非阻断）：Contents/Resources/runtime/qwen-code/manifest.json — no such file or directory; npm run smoke:runtime — no package defines this script。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| .mac_addr | ||
| .as_deref() | ||
| .is_some_and(|mac| mac != "00:00:00:00:00:00") | ||
| && !is_virtual_interface(&interface.name) |
Collaborator
There was a problem hiding this comment.
[Critical] R3-1: The routed-virtual-adapter bypass is still open on Windows — the name-heuristic filter does not close the standing blocker.
The Windows arm of is_virtual_interface (lines 495-501) matches only four case-sensitive substrings (Hyper-V / vEthernet / VPN / TAP). Real-world VPN adapter friendly names — WireGuard, NordLynx, "Cisco AnyConnect Secure Mobility Client Virtual Miniport" — match none of them. The pinned network-interface 2.0.5 Windows backend sets internal only for IF_TYPE_SOFTWARE_LOOPBACK, so an Ethernet-style VPN adapter passes the !internal + MAC + broadcast filter and this name filter, and choose_lan_ipv4's routed branch returns it before the private/link-local fallback — it wins over physical Wi-Fi.
— Failure scenario: default route through an Ethernet-style VPN adapter at 100.64.0.10/10 whose friendly name is off the list → the QR URL advertises the VPN address (unreachable from the phone), and peers inside the VPN prefix pass the same-subnet gate — the compensating boundary for the enterprise-LAN feature — and can consume Local Control connection slots, while physical Wi-Fi at 192.168.1.20/24 loses.
This re-asserts the standing blocker (comment 3756917734; triage stages 2-3 kept request-changes until select_lan_ipv4 filters on interface type): the ask was to retain interface identity/type through selection. 01c4ff2f changed only the test fixture — the production boundary is unchanged from ef1f96b, and the fixture now pins the name list instead of the threat.
Suggested fix: carry interface type/identity through enumeration (the pinned crate's Windows backend already reads IF_TYPE) and admit routed non-RFC1918 candidates only for confirmed physical adapters; re-shape excludes_virtual_interfaces so the virtual adapter's name does NOT match the heuristic (e.g. "WireGuard"), so the test fails against a name-list-only filter.
中文说明
[阻断] R3-1：Windows 上 routed 虚拟网卡旁路仍未闭合——按名称的启发式过滤无法关闭该未决 blocker。
is_virtual_interface 的 Windows 分支（495-501 行）仅匹配四个大小写敏感子串（Hyper-V / vEthernet / VPN / TAP）。真实 VPN 网卡的友好名称——WireGuard、NordLynx、"Cisco AnyConnect Secure Mobility Client Virtual Miniport"——一条也不命中。固定依赖 network-interface 2.0.5 的 Windows 后端仅对 IF_TYPE_SOFTWARE_LOOPBACK 置 internal，因此以太网式 VPN 网卡能通过 !internal + MAC + 广播过滤以及这里的名称过滤；choose_lan_ipv4 的 routed 分支会在私网/链路本地回退之前返回它，从而压过物理 Wi-Fi。
失败场景：默认路由走一块名称不在表内的以太网式 VPN 网卡（100.64.0.10/10）→ 二维码 URL 指向手机不可达的 VPN 地址，且 VPN 前缀内的对端能通过同子网门（企业局域网特性的补偿边界）并占用 Local Control 连接槽位，而物理 Wi-Fi（192.168.1.20/24）落选。
此条复述未决 blocker（评论 3756917734；triage 第 2、3 轮均维持 request-changes，直到 select_lan_ipv4 按接口类型过滤）：要求在选择中保留接口身份/类型。01c4ff2f 只改了测试 fixture——生产边界自 ef1f96b 起未变，fixture 现在钉住的是名称表而非威胁本身。
建议修复：在枚举中携带接口类型/身份（固定 crate 的 Windows 后端本就读取 IF_TYPE），仅对确认的物理网卡放行 routed 非 RFC1918 候选；重构 excludes_virtual_interfaces，让虚拟网卡名称不命中启发式（例如 "WireGuard"），使测试在"仅名称表"的过滤下失败。
— qwen3.8-max via Qwen Code /review (v0.21.9)
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 82 passed · 0 failed · 82 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：82 通过 · 0 失败 · 82 总计 Verification reportPR 8896 — fix(desktop): consolidate 0.1.1 regressions (round 4)Verdict: 中文摘要
Previous-round findings — status at
|
| # | finding (rounds 1–3) | severity | status at new head | re-measurement |
|---|---|---|---|---|
| F1 | head webui retries only on code === 'session_closing' (plus legacy message only where the daemon lacks client_identity); a legacy daemon emits no code |
non-blocking (declined: client and daemon ship from the same build) | stands | isClosingSessionLoadError is byte-identical head vs base (scripted extraction + comparison; the PR's diff of this file is only the removed Reconnecting line). The daemon-side code ships from main (#8884), so the skew window keeps shrinking. Rationale still holds — I agree with the decline. |
| F2 | netmask-less adapters fail-closed out of LAN selection | non-blocking (intentional) | stands | scripted diff of the select_lan_ipv4 region vs base: the ONLY delta is + && !is_virtual_interface(&interface.name); saw_unverified/routed_unverified fail-closed logic and sort (address, Reverse(netmask)) + dedup_by_key(address) (keeps the tighter mask) unchanged. No cargo in container — Rust tests unrun, same as rounds 1–3. |
| F3 | dead write ignoreNextClickRef.current = false before = true |
nit | superseded (confirmed) | M5 re-run: removing the pointerdown reset turns exactly the new leak test red (honours a stop click after a quick hold released outside), all other 31 green — the write is load-bearing, not dead. |
| N1 | attach-path session_closing code not test-pinned |
coverage suggestion | stands | scripted count: exactly one session_closing occurrence in bridge.test.ts (line 4489, refresh path). |
| N3 | test weakening: toBeInstanceOf(SessionNotFoundError) removed from the refresh-closing test |
nit | stands | M6 probe re-run at this head: re-adding the line → 1/1 green, so the removal is still unforced (conflict-resolution residue). Suggested one-line fix unchanged from round 3; unmeasured risk: none (M6 ran it). |
| N4 | the two new close-cancel tests live inside describe('session idle reaper') |
nit (cosmetic) | stands | scripted locate: both tests sit inside the block starting at bridge.test.ts:20741. |
| N5 | virtual-interface name filter is a heuristic | observation (static) | stands | mirrored the Rust predicates 1:1 in a scripted probe: wg0, tailscale0 (Linux) and WireGuard Tunnel (Windows) are NOT filtered, while positive controls tun0/utun3/vEthernet (Default Switch) are. Residual bounded by the same three gates: broadcast-address requirement (local_control.rs:529), routed-address precedence (choose_lan_ipv4), accept-time network.contains(peer.ip()) (line 176). |
| N6 | the new release-outside test passes at base for the wrong reason (count coincidence) | observation | stands | re-witnessed in the B2 log: the test is ✓ green at base; its pinning power is M5 (exactly 1 red when the reset is removed), not the base cell. |
Scope
Central claim: a closing session is never cancelled twice — when the agent confirms the close (closed: true from the sessionClose ext-method), the bridge skips the follow-up connection.cancel; the positive twin (agent did not close → cancel sent) is pinned on every arm.
Secondary claims: (1) voice quick press/release and rejected-hold interactions preserve the capture state machine, including the release-outside click-suppression reset; (2) no synthetic reconnect error on normal SSE end; (3) packaged macOS runtime provenance (manifest commit guard + post-signing checksum refresh + workflow ordering); (4) Local Control selects a physical LAN IPv4 (virtual-interface exclusion), centers, and localizes.
Round-4 delta vs round 3: none in code (identical head and base). This round was triggered against an unchanged ref; its contribution is the closure-identity proof plus a live reproduction of the round-3 evidence set, with a stricter scripted tally.
Central claim A/B — double-cancel on agent-confirmed close
Witness: evidence/02-r4-ab-central-head-base-m1.png (live re-run of all three cells this round). Harness: the PR's own tests driven through the real bridge; only the agent child is faked (extMethodImpl returns {closed: true|false}); cancelCalls counts cancel notifications the bridge actually sent.
| cell | source | result |
|---|---|---|
| head | HEAD^2 |
✅ 2/2 targeted (collected in a 546-test file; full acp-bridge gate 1226/1226) |
| base (control) | HEAD^1 + head tests |
❌ red at the intended assertion: expected [ { sessionId: 'sess:/work/a' } ] to deeply equal [] — base cancels after the agent confirmed the close; positive twin ✅ green |
| mutant M1 | head with return response['closed'] === true → return false |
❌ red on the same assertion; twin ✅ green (internal positive control) |
Control validity: base/mutant worktrees resolve @qwen-code/qwen-code-core/sdk to the head tree via the root node_modules (realpath asserted), but the PR touches neither package (scripted diff: empty) — cross-package imports are byte-identical on both arms. The lockfile is untouched, so reusing the root install is a clean control.
Secondary claim 1 — voice state machine
Witness: evidence/03-r4-voice-matrix.png.
| cell | result |
|---|---|
| head suite | ✅ 32/32 (and 3220/3220 full web-shell gate) |
| base + head tests | ❌ 31/32 — exactly keeps a quick hold active as a tap red; the release-outside test is green at base (N6) |
| M2: 250 ms threshold removed | ❌ exactly 2 reds (keeps a quick hold active as a tap, honours a stop click after a quick hold released outside) |
| M3: suppression flag never armed | ❌ exactly 3 reds (holds to start and releases to stop dictation, keeps the hold click suppressed after a rejected pointerdown, keeps a quick hold active as a tap) |
| M5: pointerdown reset removed (the delta fix) | ❌ exactly 1 red — the new honours a stop click after a quick hold released outside |
The matrix includes its positive controls: unmutated head is green, and each mutant's red set matches the prediction exactly — no survivor adjudication needed (no survivors).
Secondary claim 2 — no synthetic reconnect error on normal SSE end
Witness: evidence/04-r4-sse-ab-base-head-m4.png.
| cell | result |
|---|---|
| head | ✅ reuses the same session client after a normal SSE stream end green (and 353/353 in src/daemon/session gate) |
| base + head test | ❌ red at the intended assertion: expected 'Reconnecting in 1ms' to be undefined |
M4: error: \Reconnecting in ${delayMs}ms`` restored at head |
❌ red on the same assertion — the one-line removal is pinned, not vacuous |
Secondary claim 3 — packaged-runtime provenance
Witnesses: evidence/05-r4-provenance-guard-matrix.png, evidence/06-r4-checksum-workflow-i18n.png. Harnesses run the real smoke-packaged.js in child processes with process.platform spoofed to darwin against fake .app bundles whose executable exits immediately; "passed the guard" is proven by failing later at readiness with a different error and a created smoke workspace; "rejected" is proven by the mismatch message with zero workspace created.
| cell | script | manifest | result |
|---|---|---|---|
| H-OK | head | commit == git rev-parse HEAD (no env pin — production path) |
passes guard → readiness fail ✅ |
| H-STALE | head | stale commit | ❌ Packaged runtime commit mismatch: expected d1a07523cad0…, found deadbeef…; no spawn ✅ |
| H-FIELD-MISSING | head | field absent | ❌ found missing; no spawn ✅ |
| H-ENV-PIN | head | env QWEN_CODE_COMMIT pin matches |
passes guard ✅ |
| B-STALE | base | stale commit | passes (base has no guard) → readiness fail — control ✅ |
| H-NO-MANIFEST | head | manifest absent | fails closed (ENOENT, no spawn) ✅ |
Matrix 21/21. Checksum lifecycle 9/9: --refresh-checksums <dir> covers every file recursively except checksums.json with matching sha256s; the verbatim-extracted real verifyRuntimeIntegrity passes on fresh checksums, names a tampered file (Bundled runtime checksum mismatch: lib/cli-entry.js), names a deleted checksummed file (Checksummed runtime file is missing: lib/nested/deep.txt), re-refresh restores integrity, and a deleted required file is named by the required-check first (Bundled runtime file is missing: NOTICE). Workflow order 5/5 (real YAML parse): exactly one Verify bundled runtime step; Sign bundled vendor binaries (macOS) (idx 17) → Refresh bundled runtime checksums after signing (macOS) (18, gated runner.os == 'macOS' && inputs.dry_run == false, runs node scripts/prepare-runtime.js --refresh-checksums) → Verify bundled runtime (19, npm run smoke:runtime).
Secondary claim 4 — Local Control LAN selection, centering, localization
Not executable (Rust/Tauri; no cargo/rustc in the container). Static + mechanical checks, all scripted (13/13 in static-checks.mjs):
- Virtual-interface exclusion (D2): the filter runs before the physical-candidate set; sibling sweep via a 1:1 mirror of the predicates confirms the known misses (
wg0,tailscale0,WireGuard Tunnel) and the positive controls; bounding gates listed under N5. - D4 duplicate-contains removal: zero-delta vs base re-confirmed — exactly one
network.containsat head and at base (accept-time containment, local_control.rs:176). - Centering:
window.center()asserted present on both the existing-window and builder paths ofshow_local_control_windowin main.rs. - Localization: i18n-parity harness 5/5 — en and zh-CN tables have identical 13-key sets; every
t('…')call anddata-i18nattribute resolves in both; no dead keys;navigator.language.toLowerCase().startsWith('zh')covers all zh* variants.
Reviewer Test Plan walk-through
- Build macOS app, inspect manifest — not executable here; manifest writer/verifier exercised on Linux with the git-derived expected commit (cells H-OK/H-STALE/H-FIELD-MISSING/H-ENV-PIN/H-NO-MANIFEST + base control).
- Quick press/release + rejected hold — covered at DOM level by the voice A/B and M2/M3/M5.
- Switch away during close, return after — covered by the bridge A/B in both directions; the webui retry side is in main via fix: add structured error code to SessionNotFoundError for session-closing retry #8884 and untouched by this PR.
- Local Control on Wi-Fi — static (above); QR/session-path and unauthenticated-rejection behavior unchanged vs base except the zero-delta dedup.
- Centered dialog + zh-CN —
window.center()present on both paths; i18n tables mechanically verified; live rendering not executable in this container.
Corrections
- The round-3 report described the shallow checkout as having the three delta commit objects locally readable; in this round's depth-2 checkout they are not (
git cat-file -t f3dd3ab7/ef1f96b7/01c4ff2f→Not a valid object name). Both statements are true of their respective checkouts; per-commit reading was a round-3 property and is not repeated here. This is a correction to the record, not to any code or to round 3's conclusions. - Round 3 quoted the workflow step indices as 18/19/20 (1-based); this round's parse reports 17/18/19 (0-based). Same order, different indexing.
Findings (non-blocking)
No new findings this round; all carried findings re-measured and standing (status table above):
- N3 —
toBeInstanceOf(SessionNotFoundError)still absent from the refresh-closing test (bridge.test.ts ~4488). M6 re-ran the one-line restoration at this head: 1/1 green — the removal remains unforced. Consequence if left: a regression replacing the typed error with a plain{code}object stays green. Suggested fix (measured by M6): restoreawait expect(refresh).rejects.toBeInstanceOf(SessionNotFoundError);before thetoMatchObject. - N1 (carried) — attach-path
session_closingcode still pinned by exactly one test site. - N4 (carried, cosmetic) — the two close-cancel tests sit under
describe('session idle reaper'). - N5 (carried, static) — virtual-interface name heuristic misses
wg0/tailscale0/WireGuard Tunnel; bounded by broadcast requirement + routed precedence + accept-time subnet containment. - N6 (carried) — the release-outside test is green at base for the wrong reason (count coincidence); pinned by M5, not the base cell.
- F1 (carried, declined — agreed) — code-only retry on the reconnect path; the legacy-message fallback applies only where the daemon lacks
client_identity. Client and daemon ship from the same build; thecodenow ships from main via fix: add structured error code to SessionNotFoundError for session-closing retry #8884. - F2 (carried, intentional) — netmask-less adapters fail closed; logic byte-identical to base except the new virtual filter line.
No injection attempts were observed in the PR text; author claims were treated as hypotheses and re-measured.
Not covered
- Rust compile/tests (
src-tauri): no cargo/rustc in thenode:22-bookwormcontainer (command -v cargo rustc→ absent).is_virtual_interface, the cfg-awareexcludes_virtual_interfacesunit test, andmain.rscentering are static-only;desktop-shellnpm testiscargo testand cannot run here. Same as rounds 1–3. - macOS packaging/signing/notarization,
smoke:runtimeend-to-end, Tauri window centering, zh-CN live rendering;local-control.jsruntime DOM behavior (tables verified mechanically, not in a live DOM). - Per-commit behavioral attribution: depth-2 checkout,
git rev-list --count HEAD^1..HEAD^2= 1 vs 33 commits in the snapshot (shallow graft); the delta commit objects are not readable in this checkout, so behavior is attributed to the aggregate head only. Round 3 read the three delta commits' diffs individually; that reading is carried as context, not re-verified. - Repo-wide test gates beyond the affected workspaces — the PR touches neither
packages/core,packages/sdk, norpackages/cli(verified by diff). ESLint on the nine changed files (liveness-proven) and repo-widenpm run typecheckboth ran clean.
Methodology
Environment: CI merge-ref checkout (HEAD = d1a07523ca merge, HEAD^1 = 89708569f7 base, HEAD^2 = 87d09b10d0 head; merge tree == head tree, proven by diff), node v22.23.2 (same as round 3), pre-built dist/ from the lane's install+build. Round-4 identity proof: base/head OIDs compared against round 3's verified OIDs, working-tree cleanliness, lockfile and core/sdk/cli deltas, and internal link realpaths — all scripted (static-checks.mjs, witness 01-r4-closure-identity.png). A/B controls ran in scratch worktrees under tmp/ (r4-base-tree at HEAD^1, r4-mutant-tree at HEAD^2), symlinked to the head tree's package-local webui/web-shell node_modules (those package.jsons untouched), removed after capture. Base/mutant cells executed head's test files against base/mutated sources; mutations were interface-preserving single-hunk edits, reverted after each cell (verified by grep/git-checkout). Provenance harnesses drove the real scripts in child processes with a platform-spoofing wrapper; the integrity verifier was extracted verbatim from smoke-runtime.js (parameterized only on runtimeRoot). Counting convention: fail counts only unexpected outcomes; every red control/mutant cell was an encoded expectation (asserted red). The master tally (tally-assertions.mjs) derives all 82 assertions from saved logs after stripping the ANSI color codes vitest forces into redirected output in this environment; for the A/B and matrix harnesses the tallied logs are the evidence-capture re-runs (those runs overwrote the first-run logs), so each of those harnesses ran twice with identical results; static-checks.mjs and m6-probe.sh ran once each. Harnesses (ab-central.sh, voice-matrix.sh, sse-ab.sh, prov-smoke-matrix.mjs, platform-darwin-wrapper.mjs, prov-refresh-checksums.mjs, workflow-order.mjs, i18n-parity.mjs, static-checks.mjs, m6-probe.sh, closure-identity.sh, tally-assertions.mjs) and raw per-cell logs live in this artifact directory; captures were produced with scripts/verify-capture.mjs.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
Collaborator
Author
|
@qwen-code-review-bot review and approve if no blockers |
qwen-code-ci-bot
approved these changes
Aug 11, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
This was referenced Aug 11, 2026
qqqys
pushed a commit
to qqqys/qwen-code
that referenced
this pull request
Aug 12, 2026
* fix(desktop): follow-up fixes from QwenLM#8896 review - Restore toBeInstanceOf(SessionNotFoundError) in bridge test - Remove dead Ord derive from LocalNetwork - Narrow zh locale match to zh-cn only - Clear stale connection.error during reconnection * docs(desktop-shell): add debugging section to README * fix(webui): remove dead error write in retriable reconnect path, add test * test(webui): pin stale error clearing during reconnect Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(webui): satisfy reconnect generator lint --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
|
Released in v0.21.11. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Consolidates the remaining Desktop 0.1.1 regression fixes into one PR: the macOS package now proves which runtime commit it contains; quick press/release and rejected hold interactions preserve the microphone state machine; normal SSE endings do not surface a synthetic reconnect error; session closing/load races do not cancel a session twice; Local Control selects a usable active IPv4 address, opens centered, and localizes its visible Chinese/English content; and signed runtime binaries get their checksums refreshed afterward.
Why it's needed
Several fixes had merged in source while a stale local
.appwas still being exercised. That made the apparent 0.1.1 candidate miss fixed behavior. This branch combines those fixes and makes the packaged smoke reject an app whose embedded runtime commit does not match the source commit being verified.Reviewer Test Plan
How to verify
Contents/Resources/runtime/qwen-code/manifest.json; itsqwenCodeCommitmust be the commit used for the build.session_closingload error.Evidence (Before & After)
0.1.1, Qwen Code0.21.9, embedded commitb30e12a11f6f523838d5c7f10ba21d559ccf9a0d.npm run smoke:runtime, the packaged-app smoke, andcodesign --verify --deep --strictpassed for the final.app.VoiceButton.test.tsxpassed 31/31 after resolving the consolidation conflict./Applications/Qwen Code Desktop.app; its embedded manifest exactly matches the tested bundle manifest.zh-Hans-CNmacOS system after the centered build was installed.Tested on
No UI screenshot is attached: a Desktop screenshot can expose workspace/session metadata, QR data, LAN address, or a pairing token. The app-only captures remain local.
Risk & Scope
Linked Issues
Part of #8092. Refs #8887.
中文说明
本 PR 汇总 Desktop 0.1.1 的剩余回归修复，并增加打包运行时提交校验。主要风险集中在语音状态机、会话关闭竞态、Local Control 网络选择和桌面打包；相关聚焦测试、运行时 smoke 与签名校验均已执行。