feat(web-shell): bridge a browser-granted local directory into a session - #10962
feat(web-shell): bridge a browser-granted local directory into a session#10962wenshao wants to merge 16 commits into
Conversation
The reverse tool channel could only register a client-hosted MCP server workspace-wide, which fans out to every active session and is copied onto every session created afterwards. That is the wrong blast radius for a client-hosted server whose tools read and write somebody's own disk: one browser connecting a folder would extend it to sibling conversations, channel-driven conversations and background agents in the same workspace. A registration frame may now carry a session id. When one is present the server is added to that live session only, and a call arriving from any other session is rejected instead of falling back to a shared sender. Teardown is owner-scoped and carries the same session id, so a connection that lost ownership cannot remove a peer's live tools, and a closing socket cannot leave the child holding a dead transport. Session-scoped tools are also loaded eagerly. Left deferred they would sit behind tool search, and the agent would have to guess their names before it could use the bridge at all. Omitting the session id keeps the previous workspace-wide behaviour unchanged, so existing clients are unaffected. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
A daemon running somewhere else cannot reach the developer's own machine, so a conversation against it could only ever see the daemon's filesystem. This adds a sidebar entry that lets the person in the browser grant one directory to one conversation: they pick a folder with the native chooser, and that conversation gains four tools which list, read, search and write inside it, with every path confined to the chosen root and rejected if it tries to leave. The grant is persisted, so a reload reconnects silently while the browser still considers the permission valid and otherwise asks for exactly one click -- re-requesting permission consumes user activation, which an effect cannot supply. One tab owns the bridge at a time. Connecting twice, or navigating away while the native chooser is still open, cannot leave a bridge running that nothing can stop or a second chooser behind the first. Deployment shape varies, so the entry probes its own context and explains itself instead of offering an action that could only fail: a browser without the API, an insecure origin, and a cross-origin frame each get a specific message, and the framed case offers to open a top-level tab because the browser blocks the chooser there. The dev server also needed the reverse-channel route proxied; production does not, since the daemon serves the page and the route is then same-origin. The design document records the measured constraints behind these choices, including the ones that changed the design after measurement. 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 |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
|
Second pass, at Template ✓ — still complete. The Chinese section is a real translation, and the Risk bullets still name the tradeoffs instead of waving at them. Problem: real and evidenced, unchanged. The design doc's measured facts still carry the architecture, and it gained a section (4.1) that reasons the desktop-shell decision out in prose instead of leaving it implicit in a filter. The honest note from the first pass still stands and I am keeping it: there is no linked user request. Direction: aligned, and I would make the same call again. A remote daemon that cannot reach the developer's own checkout is a dead end for the "model and heavy lifting stay remote, files stay local" topology, and the alternatives really are copy-up/copy-back or give up the remote host. Reusing Size: 2,637 production lines / 3,298 test / 234 docs (6,169 total, 26 files). It is cross-package ( Approach: still matches what I proposed from the description alone, and the delta is minimal in exactly the way a fix commit should be: 4 production lines changed in Risk: Stage 1e matched no high-risk paths. It is a new security surface regardless of that heuristic, so I re-verified the mitigations against Moving on to code review. 🔍 中文说明第二轮,基于 模板 ✓——依旧完整。中文部分是真正的翻译,Risk 各条也确实写出了取舍而不是含糊带过。 问题: 真实且有实证,与上轮一致。设计文档里的实测事实仍然支撑着整个架构,而且新增了 4.1 一节,把桌面宿主的决定用文字讲清楚,而不是让它隐含在一个 filter 里。第一轮那条如实说明我继续保留:没有关联的用户诉求。 方向: 对齐,我仍会做同样判断。远端 daemon 碰不到开发者本机的代码,对"模型和重活留在远端、文件留在本地"这种形态是死路,而替代方案真的只有事先拷上去/事后拷回来,或者放弃远端主机。复用 规模: 生产代码 2,637 行 / 测试 3,298 行 / 文档 234 行(合计 6,169,26 个文件)。它跨包( 方案: 依旧与我只看描述时的独立提案一致,而且增量小得正合一个修复提交应有的样子: 风险: Stage 1e 未命中高风险路径。但无论启发式如何,这都是一块新的安全面,所以这一轮我重新对着 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewMy independent proposal from the title and the "Why it's needed" section, written before opening the diff, was the same as last pass: reuse the existing reverse channel and the existing session-scoped bridge method, add an optional session binding to the registration frame, confine paths in a facade over the granted handle, and surface a small popover with explicit degradation states. The PR still matches it and still exceeds it in the two places I had not thought of — the cross-tab Web Lock and the generation counter that invalidates a This pass I reviewed the delta properly and re-verified the load-bearing claims against 1. Closed — the session switch no longer starts a bridge over an ungranted handleFixed in The regression test is a real pin rather than a coverage line. 2. Open, deferred by the author — third byte-identical copy of the WS bearer helperUnchanged, and I re-confirmed it is real rather than restating it. @wenshao deferred this deliberately on the thread to protect the PR's scope, and offered it as follow-up work. That is a reasonable call and it is recorded, so I am leaving it as a noted item rather than a blocker. The design doc itself flags the duplication ("与 3. Open, deferred by the author — the design doc's evidence is not in the repositoryAlso unchanged, but re-reading it against the repo's own conventions softened my read, and I would rather correct myself than let the earlier framing stand. The doc rests its measured facts on That changes the remedy, not the finding: the honest fix is to mark the evidence author-local and drop the re-runnable claim, or move the spike under New in this pass — the desktop-shell gate
One residual, not a findingThe invariant in finding 1 is now enforced at acquisition time, but a grant can still be revoked out of band afterwards — the user clears site permissions while the bridge is up. What I re-verified against
|
| File | What changed |
|---|---|
docs/design/2026-09-03-client-filesystem-bridge.md |
New design doc: goals and non-goals, why the carrier is the reverse channel rather than CDP, the measured facts, the degradation matrix, the session-ownership security argument, and a new section reasoning out the desktop-shell decision. The evidence scripts it cites cannot be committed where they live (finding 3). |
packages/cli/src/serve/acp-http/client-mcp-sender-registry.ts |
Adds the two session-scoped bridge methods to the interface — both already implemented on the concrete bridge — and a registration helper that mirrors the channel-worker sequence, plus a scope parameter on register and unregister. |
packages/cli/src/serve/acp-http/client-mcp-sender-registry.test.ts |
Six new cases: adds to one session only, routes the bound session and hard-rejects every other caller, scoped unregister, peer re-registration left untouched, and both rollback paths. 16 tests total. |
packages/cli/src/serve/acp-http/client-mcp-ws.ts |
Optional sessionId on the register frame, a scope type, per-connection scope memory so teardown removes the same registration, and validation rejecting a malformed sessionId before the provider is touched. |
packages/cli/src/serve/acp-http/client-mcp-ws.test.ts |
Three new cases over the real WS: scope passed through, malformed sessionId rejected early, and dispose carrying the remembered scope. 8 tests total. |
packages/web-shell/client/components/LocalFilesControl.tsx |
The sidebar entry: a popover trigger with a status dot, and a prop-driven panel body mapping each phase to its affordances. Reuses the existing popover, button and spinner primitives. |
packages/web-shell/client/components/LocalFilesPanel.test.tsx |
24 tests on the panel alone: every degradation state, the affordance matrix per phase, and all twelve states rendered in Chinese asserting no raw key leaks. |
packages/web-shell/client/components/sidebar/WebShellSidebar.local-files-footer.test.tsx |
New this pass: three cases over the footer entry — offered by default in a plain browser, hidden by default in the desktop shell, and still reachable there when explicitly configured. |
packages/web-shell/client/components/sidebar/WebShellSidebar.tsx |
Adds the new footer item to the type, the default list and the render, then derives a desktop-shell default list that filters it out while leaving an explicit footer.items in charge. |
packages/web-shell/client/i18n.tsx |
New keys in each of EN and ZH: title, hint, actions, the status strings, the needs-session hint, and the blocker explanations. |
packages/web-shell/client/local-files/bridge-client.ts |
The reverse-channel client: owns one WS for the lifetime of a grant, ACP initialize then register, answers RPC frames, retries a cold ACP child, reconnects with backoff, and takes a cross-tab Web Lock. Carries the duplicated bearer helper (finding 2). |
packages/web-shell/client/local-files/bridge-client.test.ts |
29 tests driving the whole state machine in node with a fake socket: handshake, retry and re-warm, attempt budgets, reconnect backoff, frame correlation, notification silence, foreign-server frames, and lock handover. |
packages/web-shell/client/local-files/capabilities.ts |
Runtime probe for the three things the deployment cannot promise: a Chromium picker, a secure context, and a top-level or same-origin document. Returns the blocker the user can most act on. |
packages/web-shell/client/local-files/capabilities.test.ts |
Seven tests over the probe, including a null top treated as top-level rather than crashing, and blocker precedence. |
packages/web-shell/client/local-files/directory-handle-store.ts |
IndexedDB persistence for the granted handle so a reload reconnects silently. Fails soft everywhere, resolves on transaction commit, closes the database after each operation, validates kind on load. |
packages/web-shell/client/local-files/directory-handle-store.test.ts |
Eight tests with a fake IndexedDB reproducing the real success-then-commit ordering, including open failure, request failure, and a foreign value under the key. |
packages/web-shell/client/local-files/file-system-access.d.ts |
Type surface the DOM lib does not ship yet: the picker entry point, the handle permission methods, and the directory async iterator. |
packages/web-shell/client/local-files/local-directory.ts |
The path-safe facade: segment validation, then list, read with offset and limit, write with intermediate directories, and a breadth-first literal search with file, byte and hit budgets that report what they skipped. |
packages/web-shell/client/local-files/local-directory.test.ts |
38 tests against an in-memory tree: every rejected path shape, truncation fields rather than synthetic entries, budget exhaustion by files, bytes and hits, and binary and unreadable files counted as skipped. |
packages/web-shell/client/local-files/mcp-server.ts |
The browser-side MCP server, hand-rolled for four methods: initialize, tools list, tools call, and empty answers to the prompts and resources probes. Filesystem errors become tool results so the model can act on them. |
packages/web-shell/client/local-files/mcp-server.test.ts |
25 tests on the protocol surface: handshake idempotency, notification silence, unknown method and unknown tool handling, missing required arguments, and the formatting of listings and hits. |
packages/web-shell/client/local-files/pick-directory.ts |
Splits the two paths the browser forces apart: acquiring a grant needs a gesture, recovering one only reports. A dismissed picker is a distinct outcome from a failure. |
packages/web-shell/client/local-files/pick-directory.test.ts |
Eleven tests covering cancellation versus failure, the permission state machine, and that a request only happens when explicitly allowed. |
packages/web-shell/client/local-files/useLocalFilesBridge.ts |
The React lifecycle: probe, silent restore, wait for a gesture, bind to the active session, rebind when it changes, and a generation counter that invalidates a connect outliving the view. Finding 1 was here and is now fixed. |
packages/web-shell/client/local-files/useLocalFilesBridge.test.tsx |
14 tests on the hook, one more than last pass: degradation states, pick and persist, grant kept until a session appears, silent reload, the gesture path, the new ungranted-handle-on-session-switch pin, disconnect, double-click, and three teardown races. |
packages/web-shell/vite.config.ts |
One dev-server proxy route so the browser can reach the daemon WS during development, as an exact-path regex so it cannot shadow a client source module. |
Testing
This is an unattended CI run, so I did not build or execute anything from this PR — the evidence below is the PR's own CI on the reviewed commit, read through the API, plus the per-file results pulled out of the Test (ubuntu-latest, Node 22.x) job log.
Everything is green, including the two checks that were red last pass. Lint & Static and Test (ubuntu-latest, Node 22.x) both failed at 5a890810 on trunk breakage in InputPrompt.tsx; the merge of main brought in #10961's fix, and both now pass. The CLI suite reports Test Files 1003 passed (1003) against last pass's 1 failed | 1002 passed (1003) — so the PR needed a rebase, exactly as I said, and the rebase is done.
Per-file results for every suite this PR adds or changes, quoted from that job's log:
✓ src/serve/acp-http/client-mcp-sender-registry.test.ts (16 tests) 17ms
✓ src/serve/acp-http/client-mcp-ws.test.ts (8 tests) 173ms
✓ components/LocalFilesPanel.test.tsx (24 tests) 133ms
✓ components/sidebar/WebShellSidebar.local-files-footer.test.tsx (3 tests) 152ms
✓ local-files/bridge-client.test.ts (29 tests) 182ms
✓ local-files/capabilities.test.ts (7 tests) 4ms
✓ local-files/directory-handle-store.test.ts (8 tests) 6ms
✓ local-files/local-directory.test.ts (38 tests) 15ms
✓ local-files/mcp-server.test.ts (25 tests) 11ms
✓ local-files/pick-directory.test.ts (11 tests) 8ms
✓ local-files/useLocalFilesBridge.test.tsx (14 tests) 52ms
24 daemon-side tests and 159 Web Shell tests across the new suites. The daemon count matches the PR body's claim exactly. useLocalFilesBridge.test.tsx moving from 13 to 14 is the finding-1 regression test running in CI. Package totals for context: web-shell Test Files 271 passed (271) / Tests 5991 passed (5991). The PR body's "557 Web Shell tests" is the author's own local count of a narrower subset, not a CI figure — attributing it rather than adopting it.
Not verified, and I would rather say so than let it ride:
- The end-to-end byte path still rests on the author's manual run. The PR is candid that the headless leg used a stubbed directory handle because the native chooser cannot be automated, and that the real-handle run was manual on macOS. Nothing in CI moves real bytes from a real granted directory through the daemon to the model and back.
- Windows and Linux are untested — marked
⚠️ by the author, andTest (windows)/Test (macos)are skipped in this run. Path handling is where a platform difference would show up, thoughsplitRelativePathrejecting backslashes outright means a Windows-style path is refused rather than misresolved. - Finding 1 is fixed and pinned by a test, but I still did not observe it in a browser. The static trace and the regression test are strong evidence and the mechanism is unambiguous; a live reproduction was never on the table for an unattended run.
Sandboxed verification would settle the one claim that neither the diff nor this CI can: @qwen-code /verify — specifically that a session-scoped registration keeps the four tools out of a sibling session against a real daemon. That is test-plan step 3, which the PR itself calls the single most important thing to check, and no test in this PR exercises it against a live agent: client-mcp-sender-registry.test.ts proves lookup rejects a foreign sessionId at the registry seam, and channel-worker-group.ts proves the primitives work in production, but the composed path — register into session A, ask session B for its tool list, observe the four tools absent — is asserted only in the manual run. A mock-free A/B harness with a wire oracle is exactly the right instrument for it, and it would also confirm the workspace-wide path did not regress when sessionId is omitted. @qwen-code /tmux is the wrong lane here: this is a browser surface, not the TUI. The real-handle leg cannot be settled by either lane, because no sandbox can drive a native directory chooser — that one stays a manual acceptance item for whoever merges.
| Check | Conclusion |
|---|---|
| Lint & Static (ubuntu-latest, Node 22.x) | success — was failure on trunk at the last reviewed commit |
| Test (ubuntu-latest, Node 22.x) | success — 1003 files passed, was 1 failed at the last reviewed commit |
| Test (macos-latest, Node 22.x) | skipped |
| Test (windows-latest, Node 22.x) | skipped |
| Integration Tests (no-AK, No Sandbox) | success |
| Integration Tests (CLI, No Sandbox) | skipped |
| Serve A/B (ubuntu-latest, Node 22.x) | success |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | success — was in progress at the last reviewed commit |
| Capture web-shell visuals (ubuntu-latest, Node 22.x) | success |
| Desktop Shell (ubuntu-22.04) | success |
| Desktop Shell (windows-2022) | success |
| OpenTUI no-flicker gate | success |
| TUI parity snapshots (ink vs opentui) | success |
| Real daemon E2E / Java 11 | success |
| SDK Java matrix (ubuntu 11/17/21, macos 21, windows 21) | success |
| Dependency CVE audit | success |
| Secret scan (TruffleHog) | success |
| PR orchestration (Classify PR, assign, label, authorize, delay-automatic-review, Remind on force-push) | success |
| ack-review-request / precheck-pr / publish-resolution / resolve-pr / review-config | skipped |
| review-pr | in progress — bot orchestration on pull_request_target, not PR CI |
Every workflow run with event == pull_request on this commit is completed / success — Qwen Code CI, Security Checks, Serve A/B, Web-shell Visuals, tui-parity and SDK Java. The only thing still in flight is the bot's own review orchestration, which is pull_request_target and does not gate this PR.
中文说明
代码审查
我在打开 diff 之前,只根据标题和「为什么需要」写下的独立方案与上轮相同:复用既有反向通道和既有的会话级 bridge 方法,在注册帧上加一个可选的会话绑定,在授权 handle 之上用一个门面限制路径,并给出一个带明确降级状态的小弹层。本 PR 与之一致,并且在两处我没想到的地方做得更好——跨标签页 Web Lock,以及那个让「活得比视图更久的 connect()」失效的 generation 计数器。
这一轮我认真审了增量,并重新对着 main 的源码核实了承重论断,而不是沿用我自己上一轮的阅读。自 5a890810 以来的增量是 5 个文件,其中两个是叠在一次 main 合并之上的 PR 自身提交;compare 里其余的都是主干内容。
1. 已关闭——切换会话不再在未授权 handle 上启动桥
已在 daf26787c7 修复,而且是诚实的那种修法。restore() 在浏览器回答 prompt 时不再把存储句柄写进 handleRef;那行赋值被替换成一段注释,说明不变式本身以及为什么 connect() 改为重读 store。这恢复了重绑 effect 一直静默依赖的性质——handleRef 只持有浏览器已授权的句柄——我逐个追踪了写入点确认它现在成立:唯一赋值的地方是 startBridge(),而 startBridge() 只可能从 restore()(已过 granted 检查)、connect()(已过 granted 检查或刚完成选择)、以及重绑 effect(拿着已授权的 ref)到达;disconnect() 会清空它。
回归测试是真正的钉子,不是凑覆盖率。「does not start a bridge from an ungranted handle on session switch」先布置一个 queryPermission 回答 prompt 的存储句柄,断言 needs-gesture,然后用不同的 sessionId 重渲染,断言 phase 仍然是 needs-gesture 且 expect(h.sockets).toHaveLength(0)。零 socket 正是修复前代码会失败的那条断言——旧路径会走到 connecting。它还多走一步验证恢复动作仍然可用:断言手势之后的 mcp_register 帧带的是 sessionId: 'session-2',也就是点击时的活动会话,而不是 restore 时的那个。CI 确认它在跑:该套件从 13 个测试变成 14 个,且为绿。
2. 未关闭,作者已延后——WS bearer 辅助函数的第三份逐字副本
未变,而且我重新确认了它是真的,不是照抄上轮结论。bridge-client.ts 的 bearerSubprotocols() 与 components/terminal/TerminalPanel.tsx:40-52、voice/useVoiceCapture.ts:90 带着同一个 qwen-bearer. 前缀和同样三个 .replace()。让它成为维护隐患而非小刺的地方在 daemon 侧:serve/acp-http/index.ts:258-265 用注释记录了这份契约,写的是「Kept in sync with the encoder in packages/web-shell/client/voice/useVoiceCapture.ts」——它只点名了一份客户端副本,而现在有三份。WS 鉴权方案一旦变化就是三个目录里的三次编辑,失效模式是一个静默无法鉴权的桥。
@wenshao 在 thread 上为保住本 PR 范围刻意延后了它,并表示愿意作为后续工作。这个判断合理且已记录在案,所以我把它留作已注明事项而非阻塞项。设计文档自己也标出了这处重复(「与 TerminalPanel.tsx:38-57 同方案」),所以这是已知取舍,不是疏漏。
3. 未关闭,作者已延后——设计文档的证据不在仓库里
同样未变,但对照仓库自身约定重读之后我的判断软化了,我宁愿纠正自己也不让上轮的框定继续站着。文档把实测事实建立在 .qwen/scripts/client-fs-spike/spike.mjs 上并称其可重复运行(PASS,可重复运行),我确认两个路径都未被跟踪——但 .gitignore:32 整体忽略了 .qwen/*,而 AGENTS.md 明确把 .qwen/scripts/ 记为按设计不入库的工作产物。所以作者不是忘了提交 spike,而是按仓库约定它待在的地方根本不能提交。
这改变的是补救方式而不是结论:诚实的修法是把证据标注为仅存于作者本地并去掉「可重复运行」的说法,或者如果确实想要可复现,就把 spike 挪到 docs/ 下。提交一份核心测量别人无法重新推导的文档仍是一笔小债——事实 8(注册需要活的 ACP 通道)正是重试/re-warm 循环存在的理由。作者与发现 2 一并延后;两者都不阻塞。
本轮新增——桌面宿主闸门
092ea240fc 与修复一起到来,我单独审了它。它复用 utils/externalOpen.ts:26 既有的 isDesktopShell() 探针而不是新加一个,这个直觉是对的。DESKTOP_DEFAULT_FOOTER_ITEMS 在模块作用域由既有默认列表 filter 得出,所以两份列表不会被人手改到不一致;而且它只在调用方没有显式传 footer.items 时才被采用——逃生口是被保留的,不是写在文档里却失效的。三种情况都有测试(普通浏览器里默认提供、桌面宿主里默认隐藏、桌面宿主里显式配置时仍可达),而论证它的文档一节同时记录了它没有验证的东西:原生选择框在 Windows WebView2 的 app 托管回环源下是否可用,仓库里既无测试也无文档佐证。把未知讲出来而不是糊过去,是好习惯。
一处残留,不是 finding
发现 1 的不变式现在在获取授权那一刻被强制执行,但授权之后仍可能被带外撤销——用户在桥运行时清掉站点权限。那时 handleRef 会持有一个不再被授权的句柄,切换会话就会启动一个调用必然失败的桥,症状与发现 1 相同,只是路径远不那么日常。我不把它作为 finding,因为它失败即封闭、而且失败得很响:toLocalDirectoryError 把 NotAllowedError / SecurityError 映射成 permission_denied,消息是「The directory grant may have been revoked; reconnect it.」,所以模型和用户都拿到可操作的原因,而不是一个静默的错误答案。记下来是因为未来的 bug 报告大概会长这个样子。
我对着 main 重新核实并且成立的部分
这些是整个特性的立身之本,所以我在被审提交上逐条查了源码,而不是沿用上一轮:
- 会话隔离在两个方向上都硬拒绝。
ClientMcpSenderRegistry.lookup(client-mcp-sender-registry.ts:159-190)既拒绝「context.sessionId在会话级名字下没有 sender」的调用,也拒绝「会话级名字却完全没有会话上下文」的调用——因此不存在回落到 workspace 级 sender 的通路。两个分支都早于本 PR。 - bridge 方法本来就有。 本 PR 把
addSessionRuntimeMcpServer/removeSessionRuntimeMcpServer加进ClientMcpBridge接口却没碰任何实现文件,这之所以能编译,是因为具体 bridge 早已实现——已确认,而且在channel-worker-group.ts:270,306,327用于生产。这也是类型检查在没有实现改动的情况下为绿的原因。 - 注册时序是忠实照搬,不是重新发明。
registerSessionScopedClientMcpServer一步一步跟着registerChannelLoopMcp(channel-worker-group.ts:259-312):先记 sender 以便同步发生的发现握手能路由,再 add,检查skipped,检查shadowedSettings,await 之后重新确认ownsSession,然后按 owner 范围回滚。回滚的 owner 范围是关键细节——被取代之后活工具归对端所有,而移除是按名字进行的,所以不限定 owner 的拆除会把它们杀掉。 alwaysLoadTools: true有先例。 注释称 daemon 的浏览器自动化注册出于同样理由设了同一个标志;已在serve/acp-http/index.ts:253确认。- 审批流的说法是真的。
DiscoveredMCPTool.getDefaultPermission()(core/src/tools/mcp-tool.ts:347-356)只在trust === true且isTrustedFolder()时返回'allow'。本 PR 发送的 runtime config 是{ type, [CLIENT_MCP_OVER_WS_CONFIG_FLAG], alwaysLoadTools }——没有trust——所以这四个工具默认'ask',而披露的自动批准注意事项属于既有的 YOLO 行为,不是新增的绕过。 - 路径限制是纵深防御。
splitRelativePath在任何遍历之前拒绝非字符串、反斜杠、绝对路径、盘符、..和控制字符,之后每个存活的段都从授权根经getDirectoryHandle解析——而 File System Access API 本来也无法寻址到它之外。write在碰文件系统之前就执行字节上限,并在finally里关闭 writer,所以不会留下一个流还开着的半成品文件。搜索是字面子串扫描,因此模型提供的 pattern 不存在灾难性回溯的失效模式。 - 每条拆除路径都带着注册时的 scope。
serverScopes的存在就是为了让dispose()、handleUnregister和 await 期间被 dispose 的重新检查拆掉同一次注册;会话级 server 若不带 session id 被移除,就会漏掉该会话的副本,让子进程握着一个死 transport。失败、回滚、dispose 三条路径都同时清理两个 map,而serverScopes.clear()在allSettled拆除之后执行,不是之前。 - 帧的改动向后兼容。
sessionId可选;非字符串或空值会在触达 provider 之前返回invalid_session_id,不带它的客户端保持原有的 workspace 级行为。
路由归属
按本仓库对 daemon 路由的规则:被改动的 mcp_register 路由是 workspace 鉴权、会话定向的。调用方自带任意 sessionId;daemon 校验其形状,requestSessionStatus 校验会话存在——但没有检查这条 WS 连接是否 attach 到该会话。所有下游消费者都与该范围一致:setSession 按会话键控,addSessionRuntimeMcpServer 是会话级的,lookup 硬拒绝跨会话调用。所以本 PR 声称的隔离保证成立且被强制执行;授权保证按设计在范围之外,并已在 Risk 一节披露。maintainer 作者现已在 thread 上表态这是一个有意识的 maintainer 决定、与 /acp 其余部分一致,这正是第一轮所要求的。
一条既有问题,已重新确认,仍不是针对本 diff 的 finding:sessionScopedServerNames 由 setSession 添加却从不被清理——deleteSession(client-mcp-sender-registry.ts:138-146)清掉 sender map 却把名字留在集合里。它在功能上无害,因为两个 sender map 都空了之后 lookup 会提前返回 undefined,所以一个陈旧名字永远无法拒绝任何东西,而且它早于本 PR。但这是该路径第一个浏览器可达的消费者,所以一个不断换用不同 server 名字的客户端会让这个集合在 daemon 生命周期内一直增长。
流程
回程是从 diff 里最难看出来的一部分,所以仍然值得画出来(图同上,参与者与标签均为英文)。
测试
这是无人值守的 CI 运行,所以我没有构建或执行本 PR 的任何东西——下面的证据是被审提交上 PR 自己的 CI(经 API 读取),外加从 Test (ubuntu-latest, Node 22.x) 作业日志里取出的逐文件结果。
全绿,包括上一轮红色的那两个检查。 Lint & Static 与 Test (ubuntu-latest, Node 22.x) 在 5a890810 上都因 InputPrompt.tsx 的主干破损而失败;合并 main 带进了 #10961 的修复,两者现在都通过。CLI 套件报告 Test Files 1003 passed (1003),对比上一轮的 1 failed | 1002 passed (1003)——所以本 PR 需要的是 rebase,正如我上轮所说,而 rebase 已经完成。
本 PR 新增或改动的每一个套件的逐文件结果,引自该作业日志(清单同上文代码块)。
daemon 侧 24 个测试,新套件合计 159 个 Web Shell 测试。daemon 数目与 PR 正文的声称完全一致。useLocalFilesBridge.test.tsx 从 13 变到 14,就是发现 1 的回归测试在 CI 里跑。作为上下文的包级总数:web-shell Test Files 271 passed (271) / Tests 5991 passed (5991)。PR 正文里的「557 个 Web Shell 测试」是作者本地对一个更窄子集的自己计数,不是 CI 数字——我标注出处而不采纳它。
未验证的部分,我宁愿说出来也不让它蒙混过去:
- 端到端字节路径仍建立在作者的人工运行之上。 PR 坦承无头那一腿用的是桩目录句柄,因为原生选择框无法自动化,而真句柄那一腿是在 macOS 上人工跑的。CI 里没有任何东西把真实字节从真实授权目录经 daemon 送到模型再送回来。
- Windows 与 Linux 未测——作者标了
⚠️ ,且本次运行中Test (windows)/Test (macos)是 skipped。路径处理是平台差异最可能显现的地方,不过splitRelativePath直接拒绝反斜杠,意味着 Windows 风格路径是被拒绝而不是被错误解析。 - 发现 1 已修复并被测试钉住,但我仍然没有在浏览器里观察到它。 静态追踪加回归测试是很强的证据,机制也毫不含糊;而对无人值守运行来说,真实复现从来就不在选项里。
沙箱验证能了结那条 diff 与本次 CI 都无法了结的声称:@qwen-code /verify——具体说,就是对着真 daemon 验证一次会话级注册确实把那四个工具挡在兄弟会话之外。那是测试计划第 3 步,PR 自己称它为最该检查的一条,而本 PR 没有任何测试对着活的 agent 验证它:client-mcp-sender-registry.test.ts 在 registry 接缝上证明了 lookup 会拒绝外来 sessionId,channel-worker-group.ts 证明了这些原语在生产中可用,但组合起来的那条路径——注册进会话 A、向会话 B 要工具列表、观察到那四个工具不在——只在人工运行里被断言过。无 mock 的 A/B 装置配上 wire oracle 正是干这件事的合适工具,它还能确认省略 sessionId 时 workspace 级路径没有回归。@qwen-code /tmux 在这里是错误的车道:这是浏览器界面,不是 TUI。真句柄那一腿两条车道都了结不了,因为没有沙箱能驱动原生目录选择框——那一项只能作为合并者的手工验收项保留。
CI 表格同上文(区域标记内),此处不重复。本提交上所有 event == pull_request 的 workflow run 都是 completed / success。唯一还在跑的是 bot 自己的审查编排,属于 pull_request_target,不构成本 PR 的门禁。
— Qwen Code · qwen3.8-max-2026-09-02
Reviewed at daf26787c7da5aef243310100179b43e08b16199 · re-run with @qwen-code /triage
|
Confidence: 4/5 — the one real defect I found is fixed and pinned by a test that fails without it, every check on this commit is green, and what is left is two hygiene items the author deferred on the record plus a product call the maintainer has now made in writing. Not 5/5 because those items are still open and one of them is a debt the next person will pay. Going back to the proposal I wrote before opening the diff: it still matches, and it still beats mine in the two places I had not thought of — the cross-tab Web Lock, and the generation counter that invalidates a What actually moved my verdict from defer to approve is narrow, and I want to be precise about it rather than let a second pass drift into a softer read. Last pass I deferred on three things: a defect, two hygiene items, and a product question. Each has a different status now. The defect is gone, and gone the right way. The temptation on a re-run is to accept "fixed in The two hygiene items are unchanged, and the author deferred both in writing with a reason and an offer to take them as follow-up. On a re-run that is not grounds to keep deferring — I would be holding a 6,000-line feature for a twelve-line duplication and a doc footnote. I do want the duplication on the record once more, because it is the one I would feel first as a maintainer: the daemon's own comment at The product question is the one I have to be most careful with, because it is where a gate either does its job or performs it. My first pass said the authorization model needed a conscious human yes rather than a default: any client holding the workspace token can bind a client-hosted read/write server to any conversation in that workspace. I verified again this pass that the isolation half is real and enforced in both directions — that is not the open question. The open question was whether a human had chosen the authorization half deliberately. The author, who holds admin on this repo, has now answered on the thread that it is a maintainer decision, that it is disclosed in the Risk section, and that it is consistent with the rest of The honest caveat, and it is a small one: that answer also said the authorization question would be addressed "on this thread separately," and the separate answer has not landed. I am reading the substantive position already stated as the call, and I think that reading is fair — but if the fuller answer changes the trust model, this needs a follow-up, and the place to put it is this thread rather than a new PR. I am flagging it rather than waiting on it, because nothing about it suggests the current position is provisional. Is this part of a pattern? The author has a lot of open PRs, so I checked whether volume softened the read. It did not, and the test is whether I went looking for a problem in the place one was likely rather than confirming the ones I had already found: I traced every writer to If I were maintaining this in six months: I would thank them for the design doc, for reusing the registry's existing session primitives instead of building a parallel path, and for the fix commit being four lines with a comment that states the invariant — that comment is what stops the next person from reintroducing the bug while refactoring the hook. I would curse the duplicated bearer helper the first time the WS auth scheme changed. And I would expect the next bug report to arrive in the shape I recorded in Stage 2: a grant revoked out of band while the bridge is up, which fails closed and fails loudly with an actionable message, so it is a support thread rather than a security incident. Two things a reader should know about what this approval does and does not do. It is pinned to Approving. ✅ 中文说明信心:4/5 —— 我找到的那个真实缺陷已经修复,并且被一个「没有它就红」的测试钉住;本提交上所有检查全绿;剩下的只有作者在案延后的两个卫生项,以及 maintainer 已用文字做出的产品判断。不给 5/5,是因为那两项仍未关闭,而其中一项是下一个人要还的债。 回到我在打开 diff 之前写的方案:它仍然与之一致,并且在两处我没想到的地方仍然做得更好——跨标签页 Web Lock,以及那个让「活得比视图更久的 connect()」失效的 generation 计数器。这一轮我重读了两者,都仍然正确。 真正让我从「延后」变成「批准」的东西很窄,我想把它说准确,而不是让第二轮漂移成一次更宽松的解读。上一轮我基于三件事延后:一个缺陷、两个卫生项、一个产品问题。现在三者状态各不相同。 缺陷没了,而且是以对的方式没的。复审时的诱惑是接受一句「已在 修复」就往下走;我没有。修复只删了一行,而它之所以足够,靠的是一条容易说出口、也容易只实现一半的不变式—— 两个卫生项未变,作者都以书面形式连同理由延后,并表示愿意作为后续工作接手。在复审轮里这不构成继续延后的理由——否则我就是为一个 6,000 行的特性扣住一处十二行的重复和一条文档脚注。但我还是想把那处重复再记录一次,因为它是我作为 maintainer 最先会有感觉的一项:daemon 自己在 产品问题是我最需要小心的一项,因为门禁在这里要么尽职,要么只是表演。我第一轮说授权模型需要人类有意识地点头,而不是默认落地:任何持有 workspace token 的客户端都能把一个客户端托管的读写 server 绑定到该 workspace 的任意会话。这一轮我再次确认隔离那一半是真实且在两个方向上被强制执行的——那不是待决问题。待决的是有没有人刻意选择了授权那一半。作者在本仓库持有 admin,现已在 thread 上回答:这是一个 maintainer 决定,已在 Risk 一节披露,且与 如实说明的小保留:那个回答同时说授权问题会「另行在本 thread 表态」,而那份另行表态还没落地。我把已经说明的实质立场读作该决定,我认为这个解读是公道的——但如果更完整的回答改变了信任模型,就需要一次后续处理,而该放的地方是本 thread,不是新开一个 PR。我把它标出来而不是等它,因为没有任何迹象表明当前立场是临时性的。 这是否属于某种模式? 作者有很多开着的 PR,所以我检查了体量是否让解读变软。没有,而检验标准是:我是否去最可能出问题的地方找问题,而不是只确认我已经找到的那些。我在只有一处坏掉的情况下追踪了 如果六个月后由我维护: 我会感谢他们写了设计文档、复用了 registry 既有的会话级原语而不是另建平行路径,也会感谢那个修复提交只有四行、外加一段陈述不变式的注释——正是那段注释能阻止下一个人重构这个 hook 时把 bug 重新引进来。我会在 WS 鉴权方案第一次变化时骂那句重复的 bearer 辅助函数。而我预期下一份 bug 报告会长成我在 Stage 2 记下的那个样子:桥运行时授权被带外撤销——它失败即封闭、失败得很响并带着可操作的消息,所以那是一根支持工单,不是一起安全事件。 关于这次批准做什么、不做什么,读者应该知道两件事。 它通过带 批准。✅ — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
The desktop shell always spawns its own loopback daemon, whose regular tools already reach the local disk, so the bridge adds nothing there; on WebKit webviews the picker is absent and the entry could only render dead. Keep it reachable through an explicit footer configuration.
…witch restore() wrote the stored handle into handleRef even when the browser answered 'prompt', and the session-rebind effect starts a bridge from whatever handleRef holds. Switching conversations therefore registered four tools whose every call the browser rejects, while the popover reported a connected bridge and hid the reconnect gesture. handleRef now only ever holds granted handles; connect() re-reads the store on click.
|
Re the triage escalation: Finding 1 is fixed in Root cause confirmed by static read and now pinned by a regression test: Findings 2 and 3 (hand-rolled MCP protocol surface where the SDK is a dependency; the duplicated bearer helper) are hygiene and I am deferring them deliberately to keep this PR's scope — recording the deferral here so nothing is silently dropped. Happy to take them as follow-up work after this lands. The authorization-model question (any workspace-token holder can bind a client-hosted read/write server to any conversation in that workspace) is a maintainer decision rather than a code change; it is disclosed in the Risk section and consistent with the rest of 中文说明发现 1 已修( 发现 2、3(SDK 已是依赖却手写的 MCP 协议面;重复的 bearer 辅助函数)属卫生项,为控制本 PR 范围刻意延后,记录在此以免静默丢弃,落地后愿意作为后续工作处理。 授权模型问题(任何持有 workspace token 的客户端可把客户端托管的读写 server 绑定到该 workspace 任意会话)是 maintainer 的决定而非代码改动:已在 Risk 一节披露,且与 |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 99 passed · 5 failed · 104 total Flakiness gate: ✅ 11 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:99 通过 · 5 失败 · 104 总计 抖动门:✅ 11 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10962 deep verification —
|
| # | Cell (identical wire input on both arms) | Oracle | base 60161cb6 |
head daf26787 |
|---|---|---|---|---|
| C1 | mcp_register{server, sessionId:'sess-A'} |
which bridge mutation fires | addWorkspace — sessionId silently ignored |
addSession(sessionId='sess-A'), originator conn-A |
| C1 | same | ack frame to the browser | mcp_registered |
mcp_registered, toolCount=4 |
| C2 | tool call from sibling sess-B |
resolve vs reject | resolved — reaches the browser |
rejected: No session-scoped MCP sender for 'local_files' in session 'sess-B'. |
| C3 | tool call from bound sess-A (positive control, run on BOTH arms) |
resolve + real frame on the wire + reply returns | resolved, 1 frame | resolved, 1 frame, round-trip returned |
| C4 | tool call with no session context | resolve vs reject | resolved |
rejected: requires a session context |
| C5 | workspace-level mutations caused by a session-scoped register | count | 1 | 0 |
| C6 | sessionId = '', 123, null |
ack code + bridge mutations | ignored → registers workspace-wide, 1 mutation each | invalid_session_id, 0 mutations each |
| C7 | mcp_unregister{server} (frame carries no sessionId) |
which teardown fires | removeWorkspace |
removeSession(sessionId='sess-A') — recovered from serverScopes |
| C8 | dispose() (tab close) |
which teardown fires | removeWorkspace |
removeSession(sessionId='sess-A') |
| C9 | session-scope → unregister → re-register workspace-wide | does the call resolve | resolved | rejected ← F1 |
| C10 | conn-A workspace-wide, then conn-B session-scopes the same name | does A's live server still serve sess-Z |
resolved | rejected ← F1 (by design, see below) |
| C11 | same collision, opposite arrival order | is the loser told the truth | resolved | acked mcp_registered, toolCount=4, then rejected ← F1 |
The central claim flips 5/5 from broken to correct: C1 (routing), C2 (sibling
rejection), C4 (no-context rejection), C5 (zero workspace fan-out), C6 (malformed
sessionId). On base, the exact frame the browser sends is accepted and the
directory is exposed workspace-wide — which is precisely the blast radius the PR
says it exists to close. head: 36 pass / 3 fail; base: 38 pass / 0 fail. (head runs
one extra assertion — the rejection-message check — hence 39 vs 38.)
C3 is the control that makes C2 meaningful: the bound session's call really does
put a frame on the wire and really does come back, on both arms, so C2's
rejection is a policy decision and not a dead pipe.
3. Findings
Ordered by severity. None is a blocker; F1 is the one worth a decision.
F1 (Low) — a name reserved by a session-scoped registration stays reserved for the daemon's whole lifetime, and a later workspace-wide registration of it is acked as successful and then rejects every call
ClientMcpSenderRegistry.sessionScopedServerNames
(client-mcp-sender-registry.ts:92) is only ever added to (:131, in
setSession). Nothing removes from it — in particular deleteSession (:138-147)
clears the session map but leaves the reservation. lookup (:160-183,
byte-identical to base) then rejects any session-context call for a reserved name
that has no sender for that session.
Reproduce (head arm, cells C9/C10/C11):
node tmp/pr10962-verify-20260904-025452/harness/ab-session-scope.mjs \
--impl tmp/ab/head/pkg --arm head
# C9 ! FAIL workspace-wide registration of a ONCE-session-scoped name still serves a session call
# expected: resolved
# actual: rejected
# C9 rejection: No session-scoped MCP sender for 'local_files' in session 'sess-Z'.What the client experiences: mcp_register without sessionId is acked
{type:'mcp_registered', server, toolCount:4} and then every tools/call for any
session is rejected. The daemon reports success for a registration it cannot serve
— the "silent no-op that reports success" shape, which is a finding even when the
underlying policy is correct.
Attribution — pre-existing policy vs. this PR's contribution, stated separately:
- The reservation semantics are pre-existing, deliberate, and pinned at base.
lookupis byte-identical between the arms, and base already ships the test
never falls back to a global sender for a reserved session server
(client-mcp-sender-registry.test.ts:112-131), which asserts exactly the
rejection C10/C11 observe. C10/C11 are therefore intended behaviour, not a
defect, and the fix below deliberately does not change them. - What is not intended is the permanence. Pre-PR, exactly one name could enter
the set — the fixed internal constantCHANNEL_LOOP_MCP_SERVER_NAME, written
only byserve/channel-worker-group.ts:259. This PR adds a second writer
(client-mcp-sender-registry.ts:258) that lets any client-chosen name on a
public wire protocol enter the same never-cleared set. So a browser connecting
a folder reserveslocal-filesin that daemon's registry for the rest of the
process lifetime. - Consequently the PR body's compat statement — "clients that omit it keep the
previous workspace-wide behaviour unchanged" — is true only for names that
have never been session-scoped since daemon start.
Blast radius / what it is NOT. It fails closed: calls are rejected, never
misrouted to the wrong browser, and no session gains a tool it should not have. It
is not a security escape. Reachability today is narrow: no in-repo client mixes
scopes (the web-shell always sends sessionId; channel-worker-group always uses
the session path), so it needs a third-party/legacy WS client reusing a name, on a
long-lived daemon. The narrowness is why this is Low and not higher.
Measured minimal fix (one line + comment, preserves the pinned test)
--- a/packages/cli/src/serve/acp-http/client-mcp-sender-registry.ts
+++ b/packages/cli/src/serve/acp-http/client-mcp-sender-registry.ts
@@ deleteSession
bySession.delete(sessionId);
if (bySession.size === 0) {
this.sessionSenders.delete(serverName);
+ // Release the name reservation once no session holds it. Without this the
+ // set only ever grows, so a name stays reserved for the daemon's whole
+ // lifetime and a later workspace-wide registration of it is acked
+ // `mcp_registered` and then rejects every call.
+ this.sessionScopedServerNames.delete(serverName);
}
return true;The fix was measured, not eyeballed — applied in a scratch variant
(tmp/ab/head-fix/) and driven through the same harnesses:
- Hostile fixture goes clean: C9 flips
rejected → resolved; head arm goes
36 pass / 3 fail → 37 pass / 2 fail. - Zero collateral: C1–C8 all still PASS, and C10/C11 still fail — the
pinned "never fall back to a global sender" policy is untouched, because a live
session sender keepsbySession.size > 0. - Affected suite counts unchanged: with the fix in the working tree,
npx vitest run src/serve/acp-http/client-mcp-sender-registry.test.ts src/serve/acp-http/client-mcp-ws.test.ts→ 24 passed (24), identical to
unmutated head. The working tree was restored and verified identical to HEAD
afterwards.
A fixture that would pin this axis: register session-scoped, unregister, then
register the same name workspace-wide and assert a session-context call resolves.
Nothing in the suite can currently tell head from head-plus-fix along that axis.
F2 (Low) — the control-character rejection message overstates what it checks
local-directory.ts:181-187 rejects on /[\u0000-\u001f]/ (C0 only) but reports
"Control characters are not allowed in a path." Measured
(harness/path-sweep.mjs, witness 05-path-sweep-siblings-and-ladder.png):
node tmp/pr10962-verify-20260904-025452/harness/path-sweep.mjs
# rejected (C0, matched by /[\u0000-\u001f]/): 3/3
# ACCEPTED despite the message (DEL/C1/LS/PS): 5/5 -> DEL 0x7f, NEL 0x85, 0x9f, LS 0x2028, PS 0x2029Not a confinement failure — the invariant below holds for all five — but the model
is told a rule that is not enforced, and NEL/LS/PS inside a filename are the kind
of thing that renders confusingly in a listing. Either widen the class or narrow
the message.
F3 (Low) — a legitimate relative filename is rejected as a Windows drive path
/^[a-zA-Z]:/ at local-directory.ts:166-171 matches any single leading letter
followed by a colon, so a:b.txt — a legal relative filename on every FS the
grant can point at — is rejected with "Windows drive paths are not allowed".
Measured: strict: letter-colon filename -> reject. Anchoring to a two-character
drive (/^[a-zA-Z]:[\\/]/, or [a-zA-Z]: only when the whole string is not a
bare filename) removes the false positive without admitting C:/foo, which the
sweep confirms is still rejected.
F4 (Low, test quality) — the zh-CN guard test cannot detect a Chinese-only gap, and the documented fallback is wrong
The lookup is i18n.tsx:6998: const message = messages[key] ?? EN[key] ?? key;
— verified by reading the source. The chain is selected locale → EN → raw key.
So a key missing from zh-CN alone renders the English string, not a raw key.
Three places state otherwise: the PR description ("a missing key degrades silently
to a raw key"), the code comment at LocalFilesPanel.test.tsx:99-100, and the
design doc.
The consequence is concrete: LocalFilesPanel.test.tsx:108-137 renders 12 states
under zh-CN and asserts not.toContain('localFiles.'). That assertion catches a
key missing from EN, and is satisfied by English text — so it is structurally
incapable of catching a ZH-only gap. Only :97-105 does better, by asserting
actual Chinese content for one state. Test plan step 6 asks a human reviewer to do
what the suite cannot.
Current parity is clean, so nothing is broken today: I confirmed 23 keys per
locale, 0 gaps (EN block i18n.tsx:1402-1429, ZH block :4881-4908, pure
insertion in the diff). Note the design doc says 27 keys; the code has 23.
F5 (Low, coverage) — the explicit mcp_unregister scope path is unpinned, and no in-repo client uses it
Mutation W2 (const scope = this.serverScopes.get(server) → undefined in
handleUnregister) survived 24/24. Classification: coverage gap, not dead
code and not redundant defence. The PR's new test
tears a session-scoped server down with the scope it was registered with
(client-mcp-ws.test.ts:458) drives teardown via ws.close(), which exercises
dispose() (mutation W3, killed) — not an explicit mcp_unregister frame.
Reachability is real but unused today: the web-shell client never sends
mcp_unregister — the only occurrence of that string in bridge-client.ts is
the inbound case 'mcp_unregistered'; teardown rides WS close. So the path is
live for any other client of the public protocol while being exercised by none.
My C7 cell covers it and the behaviour is correct (session-scoped teardown
naming sess-A, recovered from serverScopes since the frame carries no
sessionId) — this is a test to write, not code to fix.
F6 (Info) — path depth is unbounded on the write path
splitRelativePath caps neither segment count nor total length, and write passes
create:true (local-directory.ts:374-378). Measured: a 5,000-segment path is
accepted and would create 5,000 nested directories inside the grant. Confined,
so not an escape; noted because it is an unbounded write amplification the model
controls.
F7 (Low) — lastModified is collected on every listing and never reaches the model
F7 and F8 are verified by census (exhaustive grep over the tree), not by a
harness, so they are deliberately not counted in assertions.json.
LocalDirectoryEntry.lastModified is declared (local-directory.ts:15), carried on
the internal entry (:89) and populated from the real file on every list
(:282). Its only read sites in the entire package are
local-directory.test.ts:18 and :185. formatListing (mcp-server.ts:186-199)
renders exactly two shapes —
entry.kind === 'directory'
? `dir ${entry.path}/`
: `file ${entry.size === undefined ? '?' : entry.size} ${entry.path}`,— so no mtime ever reaches the model, while the design doc promises
"条目名 + 类型 + 大小 + mtime". Either render it or drop the field; as it stands
it is per-entry work whose result nothing consumes. This is the "for every added
field, grep its read sites" case: declared, populated and tested, read by no
production code.
F8 (Info) — openLocalDirectory is a dead export
pick-directory.ts:42 exports it, documented as "the seam where the real
FileSystemDirectoryHandle meets the structural interfaces". A repo-wide census
(grep -rn openLocalDirectory --include=*.ts --include=*.tsx packages/) returns
4 hits: the definition plus 3 in pick-directory.test.ts (:6, :142, :144).
Production wiring builds the facade directly at useLocalFilesBridge.ts:194.
Harmless, but it is an export whose only consumer is its own test.
4. Corrections to the PR description and design doc
Labelled explicitly as corrections to the text, not requests to change code —
except F1's compat line, which follows from that finding.
- "clients that omit it keep the previous workspace-wide behaviour unchanged"
— conditionally true; see F1. - "a missing key degrades silently to a raw key" — wrong; the chain is
locale → EN → raw key (F4). Same error in the design doc and in a test comment. - Design doc: "服务端仍要再校验一次" (the server re-validates paths) — not
implemented. I verified this myself rather than taking the survey's word: the
daemon validates only the server name (client-mcp-ws.ts:212) and the
sessionIdshape (:217-227), then forwards thetools/callpayload
opaquely throughregistry.lookup(serverName)(payload, context)
(client-mcp-sender-registry.ts:160-183). Path confinement exists only in the
browser. The residual boundary is the FSA grant itself, which is sound — but
the doc asserts a second layer that does not exist, and a reader relying on it
would over-trust a compromised or non-Chromium client. - Design doc: "27 keys × en/zh" — 23.
- "An explicit footer configuration can still turn it on" — accurate but easy
to over-read: the override is the??atWebShellSidebar.tsx:946, i.e. a
React prop supplied by an embedding host, not asettings.jsonoption. No
code path in this repo populates it, so in the shipped desktop app the entry is
always hidden. Test plan step 7 (desktop hides it) holds; there is no user-facing
switch to re-enable it. - Design doc:
list_directoryreturns "条目名 + 类型 + 大小 + mtime" — mtime is
gathered and never rendered (F7). - Two further design-doc statements are superseded later in the same document, so
the doc is self-correcting but internally stale: it first places the entry in
StatusBar.tsx(:183) and later in the sidebar footer (:212, which matches
the code atWebShellSidebar.tsx:6123-6125), and it first says the UI senses
disconnect viamcp_server_removed/mcpVersion(:132) and later that status
comes solely fromLocalFilesBridge.onState(:207, which matches
useLocalFilesBridge.ts:199-201).
5. Concerns I tested that do NOT hold
Reported because a finding that names what it is not is harder to wave away, and
because each of these was the most alarming reading available.
The missing ownsSession check on the register path is not a new capability.
handleRegister never asks whether the connection owns the sessionId it is
given, unlike dispatch.ts:1542/2137/2665 and index.ts:1219/2396. That looked
like a cross-tenant hole. It is not, and the PR's own Risk & Scope disclosure is
accurate:
- There is no per-session credential anywhere. The WS upgrade is authorized
once per listener (index.ts:1727-1753) and bound to a workspace mount by URL
path (:1821-1887) with a workspace-trust check (:1872-1877). ownsSessionis a claim-based gate, and the claim is free:session/loadon
a live session doesattachCount++; registerClient(existing, req.clientId)
(bridge.ts:7682-7684) with a daemon-minted per-connection UUID, not a secret
shared with the creator. One call makes client B a co-owner of A's session.- The REST surface has no connection identity at all:
POST /session/:id/prompt
(routes/session.ts:5231),/cancel(:5686),GET …/transcript(:4393),
…/export(:4349),DELETE /session/:id(:5710), and
_qwen/sessions/delete|archive|unarchive(dispatch.ts:4945-5027, up to 100
arbitrary ids, no ownership check).X-Qwen-Client-Idis validated for shape
only and used for attribution. - The pre-existing workspace-scoped
mcp_registeris strictly broader than
the new session-scoped one:workspaceMcpRuntimeAdd(acpAgent.ts:11879-11911)
adds to the workspace Config and fans out to every active session, with no
ownership notion at all. This PR narrows that. - Cross-workspace is unreachable: the provider is built per mount from that
mount's own registry + bridge (index.ts:1426-1433,server.ts:3307-3313). - An unknown or dying
sessionIdis rejected:requestSessionStatus
(bridge.ts:6460-6477) throwsSessionNotFoundError, and the child's
sessionMcpRuntimeAddthrowsinvalidParamsviasessionOrThrow
(acpAgent.ts:11953-11964).originatorClientIdis never used for
authorization anywhere in that chain — only a debug log line and echo-back.
The one precision worth carrying into review: ownsSession is used as a real
authorization gate elsewhere, so "it's only bookkeeping" would be wrong. The
accurate framing is that it is a claim-based gate whose claim any
workspace-authenticated client can obtain, so it is not a boundary between
clients of the same workspace. Of the eight ownsSession call sites I enumerated,
four are genuine authorization gates (dispatch.ts:1542 via requireOwned,
dispatch.ts:2665 inline in session/permission with httpStatus: 403,
dispatch.ts:5723 for permission votes, index.ts:1219 for the session event
stream) and four are routing or bookkeeping (dispatch.ts:2137 — where the
negation is the interesting value, index.ts:111, index.ts:126,
index.ts:2396). So the residual reviewer question is not "is this a hole" but a
consistency one: this makes mcp_register the only session-touching ACP verb that
acts on a session the connection never claimed. That is worth one sentence of
justification on its own terms rather than resting on the trust-level argument.
Bounded, and not a finding: the session scope does not change the registration
DoS budget. MAX_SERVERS_PER_CONNECTION = 10 (client-mcp-ws.ts:58, enforced at
:240 with too_many_servers) and MAX_INFLIGHT_MCP_DISPATCH
(index.ts:2185-2200) are both per WS connection. I found no per-session cap
anywhere in the two changed files, so several connections can each bind up to 10
servers into the same session. That budget is pre-existing and the PR neither
widens it per connection nor claims to; recorded so the absence is a decision
rather than an oversight.
Path confinement has no divergent twins and no reachable escape. All four tools
call the same exported splitRelativePath (local-directory.ts:249, 303, 355, 417)
and the same resolveDirectory (:232); getDirectoryHandle appears once in
production code (:240). Rejection happens before any filesystem call on all
four. I swept 62 inputs — the 7 shapes the PR's own test pins plus 55 siblings of
the same root cause (..;/, %2e%2e, double-encoded, overlong-UTF8 dots,
fullwidth/two-dot-leader/ellipsis lookalikes, fullwidth and fraction slashes, BOM,
RTL override, UNC, file:///, DEL/NEL/C1/LS/PS, non-string types, emoji/CJK) — and
asserted the invariant that actually matters given the FSA design:
over 31 accepted inputs / 57 segments: I1 no segment is
..— 0
violations; I2 no segment holds/or\— 0; I3 none empty or.—
0; I4 no C0 control — 0.
Since each returned segment becomes one literal getDirectoryHandle(name) child
lookup with no way to name a parent, nothing the blacklist lets through can address
anything above or outside the grant. Notably trim() strips a leading BOM, so
'\ufeff../etc/passwd' is rejected; ..;, ..., %2e%2e, ‥, .. are
accepted as literal names and can only produce not_found.
No ReDoS / no superlinear scaling. The path argument is model-authored, and a
prompt-injected model is an outsider's text, so I ran a ladder rather than one
case: single-segment, max-segment-count, and repeated-../ shapes at 2k / 3k / 5k
/ 20k characters, 12 runs. Slowest rung 1.51 ms (20k chars → 10,000 segments);
repeated ../ is rejected at the first segment regardless of length. Linear
split, no backtracking regex — the scaling finding does not hold.
isDesktopShell() is spoofable but is not a security boundary. The predicate is
a runtime probe of window.__TAURI__.core.invoke (utils/externalOpen.ts:16-28,
pre-existing and untouched), consumed at WebShellSidebar.tsx:941-952. Any page
script can set that global — the PR's own test does (…local-files-footer.test.tsx:156-161).
But the spoof only ever hides a UI affordance and grants nothing:
LocalFilesControl/useLocalFilesBridge perform no desktop check, and real
capability is gated independently by detectLocalFilesCapability
(capabilities.ts:49-66), which yields unsupported-browser when
showDirectoryPicker is absent — the actual WebKit-webview case. Forcing the entry
on in the desktop shell degrades to the unavailable panel.
6. Mutation matrix and vacuity
harness/mutation-matrix.mjs — 14 single-point mutations of the two changed daemon
files, each an exact single-occurrence replacement (an anchor miss is reported as
ANCHOR-MISS, never as "survived"), run against the two changed test files, with
unconditional restore. Witness 04-mutation-matrix-10-killed-4-survived.png; log
logs/mutation-matrix.log. 10 killed / 4 survived / 0 anchor-miss.
The matrix is live in both mutated files, which is what makes the survivors
interpretable: PC-ws (rename the pre-existing not_wired code) → KILLED, 1 red;
PC-reg (make lookup's workspace success path reject) → KILLED, 2 red. Unmutated
control: 24/24 green.
| id | guard removed | verdict | adjudication |
|---|---|---|---|
| R1 | central: ignore scope, route every register workspace-wide |
KILLED (5 red) | pinned |
| R6 | record the sender in the workspace store instead of the session store | KILLED (5 red) | pinned |
| R3 | shadowedSettings conflict check, session path |
KILLED | pinned |
| R4 | skipped-result check, session path |
KILLED | pinned |
| R7 | owner-scoped deleteSession on scoped unregister |
KILLED | pinned |
| W1 | malformed-sessionId guard (invalid_session_id) |
KILLED | pinned |
| W3 | dispose() forgets the recorded scope |
KILLED | pinned |
| W4 | never record the scope at register time | KILLED | pinned |
| W2 | handleUnregister forgets the recorded scope |
SURVIVED | coverage gap → F5 (behaviour verified correct by C7) |
| R2 | post-await ownsSession supersession check |
SURVIVED | coverage gap, load-bearing → see race below |
| R5 | owner guard on the rollback | SURVIVED | coverage gap, load-bearing → see race below |
| PC-reg2 | probe: drop the workspace-path registry.set |
SURVIVED (as predicted) | wrong-reason assertions, see below |
R2 + R5 are layered guards on one hazard — the combination had to be driven
Reverting either alone left 24/24 green, which is exactly the shape that makes a
one-row-per-guard matrix report two coverage gaps that do not exist. So I built the
intermediate variants (harness/build-mutants.mjs → head-R2, head-R5,
head-R2R5, all four verified by grep) and drove the real race they defend: two
clients register the same (server, session) concurrently over two real WS
connections, with the bridge's addSessionRuntimeMcpServer delayed 250 ms so both
are genuinely in flight (harness/race-supersession.mjs, witness
03-supersession-race-chain-not-layers.png, log logs/race-supersession.log).
12 pass / 0 fail.
| build | conn-A ack | conn-B ack | A owns | B owns | teardowns issued by A | sess-X call | frames @A/@B |
|---|---|---|---|---|---|---|---|
| head (as shipped) | mcp_error:register_failed — "was superseded" |
mcp_registered |
false | true | 0 | resolved | 0/1 |
| head-R2 | mcp_registered ← A is lied to |
mcp_registered |
false | true | 0 | resolved | 0/1 |
| head-R5 | mcp_error:register_failed |
mcp_registered |
false | true | 1 ← A destroys B's child-side server | resolved | 0/1 |
| head-R2R5 | mcp_registered |
mcp_registered |
false | true | 0 | resolved | 0/1 |
Both survivors are load-bearing, so both are coverage gaps rather than dead
code — and ranking them by observability, not blast radius:
- R5 is the quiet one and the more serious. conn-B is told
mcp_registered, toolCount=4; conn-A's failed rollback then issues
removeSessionRuntimeMcpServer('sess-X','local_files','conn-A'), destroying
conn-B's child-side server. The registry route survives (deleteSessionis
internally owner-scoped and returns false), so nothing looks wrong from the
daemon side — B's tools simply never fire again. A wrong answer nobody is told
about. - R2 is loud for A but leaves A silently disconnected: no throw means A is
ackedmcp_registeredwhile B owns the sender, so every call for that session
routes to B's browser.
They are a chain, not parallel layers — a correction to my own prediction,
which the harness caught: R5's guard sits in the catch that only runs when R2's
check throws. Remove R2 and the throw never happens, so R5 becomes unreachable and
its hazard disappears (0 teardowns, not 2). The first run of this harness
predicted 1 teardown for head-R2R5 and observed 0; the failure was in my
expectation, not the PR, and the corrected assertion now records the dependency.
A survivor that is a wrong-reason assertion, not a coverage gap
PC-reg2 removes registry.set(serverName, sendSdkMcpMessage, originatorClientId)
from the provider's workspace register path and survives 24/24. The reason is
worth naming because it is not "nothing covers this file":
lookup routes to the registered sender(:33) callsreg.set(...)directly
— it tests the registry class, not the provider.- The three rollback tests (
:134,:150,:166) assert only the negative
post-conditionexpect(registry.serverNames()).toEqual([]), which holds
trivially if the provider never records anything.
So no test in either changed file asserts that a successful workspace-scoped
registerClientMcpServer records a sender. That is a gap on a path this PR did
not change (pre-existing coverage, noted for completeness, not a merge
condition). The session path does not have this weakness: routes the bound session and hard-rejects every other caller (:236) asserts positively that the
call reaches the sender, which is why R6 killed 5 tests.
Vacuity check on the last commit's regression test
daf26787's fix is the absence of an assignment, so the revert re-adds
handleRef.current = stored; to restore()'s not-granted branch
(useLocalFilesBridge.ts:220). Witness
06-vacuity-daf26787-revert-turns-one-test-red.png; log
logs/vacuity-daf26787.log; script harness/vacuity-daf26787.sh (restores
unconditionally via absolute paths and self-verifies).
Result: exactly one test red, 1 failed | 13 passed (14) —
useLocalFilesBridge restore > does not start a bridge from an ungranted handle on session switch, failing with
AssertionError: expected 'connecting' to be 'needs-gesture'. That is the intended
behavioural mismatch (the panel wrongly reports a bridge coming up), not an import,
compile or fixture break, so the test is non-vacuous and pins the commit's actual
claim. The pre-fix consequence, for the record: the popover reported
Connected / 4 tools and hid the reconnect affordance while every tool call
failed in the browser with NotAllowedError.
One process note, reported because it affected a measurement: the first attempt at
this revert used a relative path in its restore trap after a cd, so the tree
was left mutated and the restore silently failed. It was caught by an explicit
git status/git diff HEAD check and repaired; every mutation run afterwards
restores by absolute path and self-verifies. All gates quoted in this report were
run against a tree confirmed identical to HEAD.
7. Not covered
- Per-commit attribution. The metadata lists 5 commits; only 2 exist
locally (5a890810,daf26787) —0bc29f51,6663805d,092ea240are absent.
git rev-list --count HEAD^1..HEAD^2returns 1, not 5, and
--is-shallow-repositoryistrue: the depth-2 graft boundary, so the bare
count is wrong rather than erroring.git show daf26787 --statlists the entire
tree as newly added for the same reason. I verified the aggregate
HEAD^1..HEADdiff only. Thedaf26787delta was reconstructed against the PR
head rather than its own parent. - No real browser, no real File System Access sandbox. The confinement
contract is two-part — a string blacklist plus the FSA guarantee that each
getDirectoryHandle(name)is a child-of-current-handle lookup. I exercised the
first part directly and asserted the invariant the second part depends on, but
never drove Chromium with a real granted directory. This reproduces the
confinement logic, not the browser sandbox's behaviour; in particular the
symlink question is unanswered by anything I ran. - The real
HttpAcpBridgewas not driven.RecordingBridgestood in for it. I
read the accept path instead and confirmed it exists and is already in production
use (bridge.ts:14071→requestSessionStatus→sessionMcpRuntimeAdd;
channel-worker-group.ts:270is a pre-existing caller), and that
originatorClientIdis never an authorization input. Whether the child actually
confines the runtime server to one session was not observed end to end. - Route-level gating not driven by my harness — the
initializedrequirement,
theclientMcpOverWsflag, the rate-limit tier split, and the
MAX_INFLIGHT_MCP_DISPATCHcap all live inindex.ts:2065-2200, above the layer
I compiled. Mitigation: the PR's ownclient-mcp-ws.test.tsdrives the real
mountAcpHttproute over a real WS, and I ran it (24/24). I confirmed by reading
thathandleFrame(parsed)receives the raw parsed JSON, sosessionIdis not
stripped by any schema before reaching the handler. - Repo-wide
npm run lintis not meaningful in this container.eslint .
also walks mytmp/scratch (5,544 files, including the base worktree), which
produced all 4,057 of the reported errors — verified by runningeslint tmp
alone and getting the identical 4,112 problems.tmp/is git-ignored but is not
in the eslint config'signores. The targeted run on the PR's 25 changed
.ts/.tsxfiles is clean (exit 0), and I proved that gate live: a scratch file
withanyand an unused variable → exit 1, both errors reported. (An earlier
liveness probe appeared to pass silently only because I had named the variable
__lintLivenessProbe, and the rule permits unused vars matching/^_/u.) - No trial merge into current
main. Depth-2 clone; nothing beyondHEAD^1
is reachable, so I could not check whethermainhas touched these files since
the merge base. Note also that the metadata'sbaseRefOid
(69c4f1e4bb4f32a28db75f0bcde21c4f884e4d32) differs from the merge-ref
HEAD^1(60161cb6…) used throughout — the PR's base has drifted since the
snapshot was taken. packages/web-shell/vite.config.ts(+7, the dev-server proxy route for the
daemon WS) was not exercised; no dev server was started.- The PR's claimed headless E2E (real daemon + real browser + real model
quoting a random token) was not re-run — it needs a browser and model
credentials this container does not have. Its claimed counts were checked where
they are checkable locally: "24 daemon-side tests" → 24/24 confirmed;
"557 Web Shell tests across the bridge and the sidebar" → the new/changed files
give 159,client/local-files+client/components/sidebargive 29 files,
and the whole package is 5,991; I could not reproduce 557 exactly and did not
try to guess the intended selection. - The five images in the PR body were not fetched (no network in this
container), so the visual claims — including the connected popover showing four
tools — are unverified here. - No injection attempt was observed in the PR title, body, commit messages, or
code comments. The body's own instructions to the reviewer were treated as claims
to test, not directions.
8. Methodology
Everything ran inside the CI verify container (node:22-bookworm, node v22.23.2)
on the merge-ref checkout cb42e3ad, with npm ci and npm run build already
complete. The A/B compared head daf26787 against a scratch worktree at
HEAD^1 = 60161cb6; both arms were produced by the same esbuild transpile-only
pipeline over a mirrored module tree, so the cells differ only in the two
PR-changed source files, and the shared dependency closure (packages/acp-bridge,
packages/core, runtime/validate-server-name.ts) was verified untouched by the
PR or byte-identical between arms, with readlink -f confirming where the internal
workspace symlinks resolve. The daemon harnesses drove the real compiled classes
over real loopback ws://127.0.0.1 sockets with a real WS peer answering real
JSON-RPC round-trips, mirroring production's one-registry-and-one-bridge-per-runtime
topology; only the bridge — a dependency needing a live ACP child — was a recorder,
and it offered both scopes on both arms so the provider's choice was the observable.
The confinement sweep drove the real transpiled splitRelativePath over 62 inputs
plus a timed 12-rung ladder. Mutation, race, and vacuity runs mutated the working
tree in place and restored unconditionally by absolute path, self-verifying with
git diff HEAD after each; the tree is confirmed clean and identical to HEAD.
Harnesses are in harness/, per-cell raw output in logs/, and images in
evidence/; each image is referenced by filename from the section it witnesses.
Flakiness gate log
rounds=5 files=11 skipped=0
file packages/cli/src/serve/acp-http/client-mcp-sender-registry.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/acp-http/client-mcp-sender-registry.test.ts
file packages/cli/src/serve/acp-http/client-mcp-ws.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/acp-http/client-mcp-ws.test.ts
file packages/web-shell/client/components/LocalFilesPanel.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/components/LocalFilesPanel.test.tsx
file packages/web-shell/client/components/sidebar/WebShellSidebar.local-files-footer.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/components/sidebar/WebShellSidebar.local-files-footer.test.tsx
file packages/web-shell/client/local-files/bridge-client.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/local-files/bridge-client.test.ts
file packages/web-shell/client/local-files/capabilities.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/local-files/capabilities.test.ts
file packages/web-shell/client/local-files/directory-handle-store.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/local-files/directory-handle-store.test.ts
file packages/web-shell/client/local-files/local-directory.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/local-files/local-directory.test.ts
file packages/web-shell/client/local-files/mcp-server.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/local-files/mcp-server.test.ts
file packages/web-shell/client/local-files/pick-directory.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/local-files/pick-directory.test.ts
file packages/web-shell/client/local-files/useLocalFilesBridge.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/local-files/useLocalFilesBridge.test.tsx
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/serve/acp-http/client-mcp-sender-registry.test.ts: PPPPP
packages/cli/src/serve/acp-http/client-mcp-ws.test.ts: PPPPP
packages/web-shell/client/components/LocalFilesPanel.test.tsx: PPPPP
packages/web-shell/client/components/sidebar/WebShellSidebar.local-files-footer.test.tsx: PPPPP
packages/web-shell/client/local-files/bridge-client.test.ts: PPPPP
packages/web-shell/client/local-files/capabilities.test.ts: PPPPP
packages/web-shell/client/local-files/directory-handle-store.test.ts: PPPPP
packages/web-shell/client/local-files/local-directory.test.ts: PPPPP
packages/web-shell/client/local-files/mcp-server.test.ts: PPPPP
packages/web-shell/client/local-files/pick-directory.test.ts: PPPPP
packages/web-shell/client/local-files/useLocalFilesBridge.test.tsx: PPPPP
verdict: pass
summary: 11 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/serve/acp-http/client-mcp-sender-registry.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/acp-http/client-mcp-ws.test.ts: P (exit 0)
round 1 · packages/web-shell/client/components/LocalFilesPanel.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/components/sidebar/WebShellSidebar.local-files-footer.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/local-files/bridge-client.test.ts: P (exit 0)
round 1 · packages/web-shell/client/local-files/capabilities.test.ts: P (exit 0)
round 1 · packages/web-shell/client/local-files/directory-handle-store.test.ts: P (exit 0)
round 1 · packages/web-shell/client/local-files/local-directory.test.ts: P (exit 0)
round 1 · packages/web-shell/client/local-files/mcp-server.test.ts: P (exit 0)
round 1 · packages/web-shell/client/local-files/pick-directory.test.ts: P (exit 0)
round 1 · packages/web-shell/client/local-files/useLocalFilesBridge.test.tsx: P (exit 0)
round 2 · packages/cli/src/serve/acp-http/client-mcp-sender-registry.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/acp-http/client-mcp-ws.test.ts: P (exit 0)
round 2 · packages/web-shell/client/components/LocalFilesPanel.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/components/sidebar/WebShellSidebar.local-files-footer.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/local-files/bridge-client.test.ts: P (exit 0)
round 2 · packages/web-shell/client/local-files/capabilities.test.ts: P (exit 0)
round 2 · packages/web-shell/client/local-files/directory-handle-store.test.ts: P (exit 0)
round 2 · packages/web-shell/client/local-files/local-directory.test.ts: P (exit 0)
round 2 · packages/web-shell/client/local-files/mcp-server.test.ts: P (exit 0)
round 2 · packages/web-shell/client/local-files/pick-directory.test.ts: P (exit 0)
round 2 · packages/web-shell/client/local-files/useLocalFilesBridge.test.tsx: P (exit 0)
round 3 · packages/cli/src/serve/acp-http/client-mcp-sender-registry.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/acp-http/client-mcp-ws.test.ts: P (exit 0)
round 3 · packages/web-shell/client/components/LocalFilesPanel.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/components/sidebar/WebShellSidebar.local-files-footer.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/local-files/bridge-client.test.ts: P (exit 0)
round 3 · packages/web-shell/client/local-files/capabilities.test.ts: P (exit 0)
round 3 · packages/web-shell/client/local-files/directory-handle-store.test.ts: P (exit 0)
round 3 · packages/web-shell/client/local-files/local-directory.test.ts: P (exit 0)
round 3 · packages/web-shell/client/local-files/mcp-server.test.ts: P (exit 0)
round 3 · packages/web-shell/client/local-files/pick-directory.test.ts: P (exit 0)
round 3 · packages/web-shell/client/local-files/useLocalFilesBridge.test.tsx: P (exit 0)
round 4 · packages/cli/src/serve/acp-http/client-mcp-sender-registry.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/acp-http/client-mcp-ws.test.ts: P (exit 0)
round 4 · packages/web-shell/client/components/LocalFilesPanel.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/components/sidebar/WebShellSidebar.local-files-footer.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/local-files/bridge-client.test.ts: P (exit 0)
round 4 · packages/web-shell/client/local-files/capabilities.test.ts: P (exit 0)
round 4 · packages/web-shell/client/local-files/directory-handle-store.test.ts: P (exit 0)
round 4 · packages/web-shell/client/local-files/local-directory.test.ts: P (exit 0)
round 4 · packages/web-shell/client/local-files/mcp-server.test.ts: P (exit 0)
round 4 · packages/web-shell/client/local-files/pick-directory.test.ts: P (exit 0)
round 4 · packages/web-shell/client/local-files/useLocalFilesBridge.test.tsx: P (exit 0)
round 5 · packages/cli/src/serve/acp-http/client-mcp-sender-registry.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/acp-http/client-mcp-ws.test.ts: P (exit 0)
round 5 · packages/web-shell/client/components/LocalFilesPanel.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/components/sidebar/WebShellSidebar.local-files-footer.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/local-files/bridge-client.test.ts: P (exit 0)
round 5 · packages/web-shell/client/local-files/capabilities.test.ts: P (exit 0)
round 5 · packages/web-shell/client/local-files/directory-handle-store.test.ts: P (exit 0)
round 5 · packages/web-shell/client/local-files/local-directory.test.ts: P (exit 0)
round 5 · packages/web-shell/client/local-files/mcp-server.test.ts: P (exit 0)
round 5 · packages/web-shell/client/local-files/pick-directory.test.ts: P (exit 0)
round 5 · packages/web-shell/client/local-files/useLocalFilesBridge.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
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Second pass at this commit: the session-switch defect from the first review is fixed, and fixed so that the invariant holds at every writer to handleRef rather than only at the broken one — plus a regression test that asserts zero sockets across a sessionId change, which is the assertion that fails without the fix. Every pull_request workflow run on this commit is green, including the two that were red on trunk at the last reviewed commit.
Two hygiene items stay open and are recorded as deferred by the author: the third copy of the WS bearer-subprotocol helper (the daemon comment at serve/acp-http/index.ts:258-265 still names only one client copy to keep in sync), and the design doc citing evidence scripts that .gitignore excludes by repo convention. Neither blocks.
The one load-bearing behaviour no automated test here exercises against a live agent is the sibling-session isolation claim — @qwen-code /verify would settle it. Details, including the re-verified security claims with line references, are in the stage 2 comment.
qqqys
left a comment
There was a problem hiding this comment.
APPROVE — 独立安全面抽验 @ daf26787
Reviewed at head daf26787c7da5aef243310100179b43e08b16199(26 文件,+6149/−20;新 PR,评审史上仅一条本 PR 二轮复核后的 bot APPROVED,其首轮发现的会话切换缺陷已修复)。
承重安全机制(本 Channel 自行读码核实,不采信声明):
- 会话隔离(fail-closed 三重):
ClientMcpSenderRegistry.lookup(:160-193)对 session-scoped 名字:有context.sessionId但无该会话 sender → 硬拒;无 session context → 硬拒;两条都不回落到 workspace 发送器。注册侧registerSessionScopedClientMcpServer在 await 后重查ownsSession(防并发再注册顶替),并拒绝shadowedSettings遮蔽用户配置的 MCP 服务。 - 路径封闭(
local-directory.ts:160-191): 绝对路径、Windows 盘符、..、控制字符逐一显式拒绝,./空段丢弃;真实访问恒经被授予的FileSystemDirectoryHandle逐段下行,API 本身无法越界。 - 生命周期:socket 关闭/注销路径
deleteSession带 owner 匹配;工具仅存活于绑定的那一个会话,离开即消失。设计文档与实现一致。
首轮评审的修复历史: 会话切换泄漏 socket 的缺陷已在 setSession/回归测试(断言 sessionId 变更后 socket 数为 0)中闭合;两条遗留 hygiene(WS bearer 子协议注释点名、设计文档引用被 gitignore 的脚本)为作者记录的非阻塞项;兄弟会话隔离的活体 agent 端到端验证由 /verify 通道继续,不改变机制代码已核实的事实。
CI(非门禁): 本 head 26 success / 26 skip / 1 在途 / 0 失败。
✅ APPROVE — 无历史阻塞、当前扫描未发现可证明的 Critical,提交批准。
— 衍星 · read-only PR review (posted as qqqys)
ytahdn
left a comment
There was a problem hiding this comment.
What this PR does / 主旨
把浏览器 File System Access API 授权的用户本机目录,桥接成只绑定单个会话的 4 个 MCP 工具(list_directory / read_file / write_file / search_files):页面内手写 MCP server,经既有 /acp 反向通道以 session-scoped 方式注册;daemon 侧新增会话作用域注册(mcp_register 可选 sessionId),并用 Web Locks 做多 tab 单属主。目录不进 workspace、不挂第二个根,关标签页/断开/会话结束即消失。
Bridges a browser-granted local directory into exactly one conversation as four session-scoped MCP tools, via a hand-written in-page MCP server over the existing /acp reverse channel, plus a session-scoped registration variant on the daemon side.
审查方式:纯静态阅读 head tree(daf2678),未运行任何测试/构建。分三批走查(服务端 acp-http / 浏览器路径门面 / 桥客户端+MCP server+UI),每条结论都附 head tree 举证。
🎉 先说做对了的(这个 PR 的安全论证质量明显高于平均)
会话隔离是真的硬拒绝,不是靠约定。 ClientMcpSenderRegistry.lookup 三分支(client-mcp-sender-registry.ts:164-192):有 context.sessionId 但该会话没有 sender 且该 name 是 session-scoped → reject;完全没有 session context → 也 reject("requires a session context"),所以 workspace 级 Config、兄弟会话、后建会话都拿不到;只有非 session-scoped 的名字才回落既有 workspace sender。这正是 §5.1 说的那条"能力能不能安全给出去"的地基,我逐分支核过。
拆除用的是记住的 scope,不是帧里客户端可伪造的字段。 handleUnregister 与 dispose() 都读 serverScopes.get(server)(client-mcp-ws.ts:302-312, 365-375),sessionId 只在 register 时进来一次。registerSessionScopedClientMcpServer 在 await 之后复查 ownsSession 防 peer 抢占、catch 里用 deleteSession(name, sid, owner) 做 owner 作用域拆除 —— 抢占后不会误删别人的活工具。
审批链兜住了"每次调用都问"。 这是我最担心的一条:一个能写用户本机盘的工具面如果自动批准,风险等级完全不同。核实结果是结构性的 —— acpAgent.ts:2616-2638 的 readRuntimeMcpAddRequest 刻意把 trust/cwd/env/headers/includeTools/type 全部解构丢弃(注释直写 "Runtime callers cannot grant trust"),并对 __clientMcpOverWs 强制 type:'sdk';于是 mcp-tool.ts 的 getDefaultPermission() 恒为 'ask',permission-manager.ts 又把 mcp__* 排除在 eager allow 之外。剩余三个可绕开的口子(YOLO / 配置里显式 permissions.allow / 审批弹窗的 "Always Allow")都是用户自己按下的,且文档 §5.4 已如实陈述。
路径门面把穿越做成结构不可达。 splitRelativePath(local-directory.ts:148-195)拒反斜杠/绝对路径/盘符/../控制字符,且四个入口第一句都是它,严格先于任何句柄访问;解析是逐段 getDirectoryHandle(name)(:235-246),不存在"先拼接再解析";search 的 BFS 里 base handle 与 prefix 同步推进(:436-443)。关于"不拒字面 %"这个看着可疑的取舍,我核了前提:全批次 decodeURI|unescape|new URL|%2e 零命中 —— 路径从不做 URL 解码,所以 %2e%2e%2f 只是一个合法文件名,推理成立。上界也都带"上限的上限"(Math.min(requested, this.limits.maxListEntries),:256-257,并有专门测试钉"要再高也不越 cap"),read 的字节上限干脆不接受调用方参数。
设计文档主动披露了自己的残余风险(§5.2 同 workspace 内任意已鉴权 WS 客户端可绑该 workspace 任意会话)和一处刻意不对称(会话路径 skipped 也调 remove、workspace 路径不调),没有藏。文档 §8 记录的两次端到端踩坑(/acp 不在 dev 代理表、StatusBar 因 compact={true} 硬编码导致入口根本不渲染)也是只有真跑贯通才会暴露的东西。
🔴 Critical(建议修改后再合)
(1) "断开"不是原子的:撤销授权之后仍会执行真实的本地读写。
packages/web-shell/client/local-files/bridge-client.ts —— teardown()(:259-268) 只做 this.socket = undefined; socket?.close(),摘不掉 handler(openBrowserSocket 的 setHandlers(next){ handlers = next }(:166-168) 没有 detach 语义),而 this.stopped 守卫写在 :249/:290/:357/:376/:510/:527/:557 —— onMessage(:380) 和 answerRpc(:465) 恰好都没有。
最容易命中的触发条件不需要什么竞态窗口:一次 tools/call 正在 await this.options.server.handle(payload) 期间,用户点了「断开」或组件被卸载(search_files 走大树、write_file 写大文件都是秒级,useLocalFilesBridge.ts:304-316 的 disconnect() 对 generationRef 的 bump 只作废 connect() 流程,管不到已在途的 RPC)。因为 await 之后没有任何复查,这段代码会一路跑到完成 —— 包括真实写进用户本地盘的那一步。随后 send()(:566) 读的是已经置空的 this.socket,回包被静默丢弃,模型侧只能等到自己超时。
mcp_message 帧在 close() 之后被浏览器投递(事件已入队)走的是同一条口子,代价同上。
另外两个由同一次守卫缺失导致的状态机背离:
- 迟到的
mcp_registered(:411) 无条件setState({ phase: 'connected', ... })→ 面板显示"已连接 + 目录名 + 4 tools",而 socket 已关、Web Lock 已释放、bridgeRef.current已置空,没有任何对象能再停它,UI 与事实永久背离。 - 迟到的
mcp_unregistered(:431-438) 把状态钉在reconnecting,而retryRegister(:357) 因stopped立刻 return;reconnecting ∈ BUSY→LocalFilesControl.tsx:74的canConnect=false且 spinner 常转 → 一个既不能重连也不显示失败原因的死面板。
建议修法(两处最小改动,语义与文件里已有的守卫完全一致):
private async onMessage(data: unknown): Promise<void> {
if (this.stopped) return;
...
}
private async answerRpc(frame: InboundFrame): Promise<void> {
const socket = this.socket; // 记住这帧来自哪只 socket
...
reply = await this.options.server.handle(payload as JsonRpcRequest);
if (this.stopped || socket !== this.socket) return; // 撤销后不回包、不跨连接串包
this.send({ type: 'mcp_message', id, server: this.serverName, payload: reply });
}mcp_registered / mcp_unregistered 两个 case 各补一条 if (this.stopped) return;。更彻底的做法是给 WebSocketLike 增加 setHandlers(undefined) 让 teardown() 能真的摘除 handler —— 顺带说明为什么现有测试没抓到:FakeSocket.close() 是同步触发 close handler 的,而且没有"向已 close 的 socket 投帧"的能力,所以这条序列在现有测试里根本构造不出来(见下面"测试覆盖")。
🟡 Important(值得改,不阻塞)
(2) onClose 只判 undefined,不判"是不是这一只 socket"。 bridge-client.ts:506-515:
private async onClose(code: number, reason: string): Promise<void> {
this.clearTimers();
const wasSocket = this.socket; // 读"当前"值,而非事件来源
this.socket = undefined;
if (this.stopped) return;recycleSocket(:522-529) 那条"先摘引用再 close()"的承诺确实做到了,但反向不成立:旧 socket 的 close 事件若晚于 scheduleReconnect → connect() 建立的新 socket 到达(后台标签页节流、移动端、或 reconnectBaseDelayMs 内浏览器尚未派发 close),onClose 会把新 socket 当成自己那只丢掉,于是旧 socket 泄漏在 daemon 侧(它已 initialize 成功、仍是活连接)、同时两条状态机循环并行、registerTimer/initializeTimer 被无关的 close 清掉。请把来源带进闭包:
socket.setHandlers({ ..., close: (code, reason) => { void this.onClose(socket, code, reason); } });
private async onClose(source: WebSocketLike, code: number, reason: string): Promise<void> {
if (source !== this.socket) return; // 迟到的旧 socket 事件(3) 缺 client_mcp_over_ws 能力预检,默认 daemon 上会白跑约 3 分钟。 这条与本 PR 自己的承诺冲突 —— body 里写的是"当部署根本无法支持时,解释原因并给出替代做法,而不是给出一个只会失败的按钮"。事实链:git grep client_mcp_over_ws -- packages/web-shell 零命中(前端完全不看能力位),而 serve/capabilities.ts:730 只在 clientMcpOverWsEnabled === true 才广播,该通道默认关闭。未开启时 mcp_register 落到 acp-http/index.ts 的畸形帧分支,回一个 id: null 的错误帧;bridge-client.ts 的 initialize 匹配要求 frame.id === INITIALIZE_ID,switch (frame.type) 又是 default: 静默忽略 → 客户端拿不到任何可辨识的失败信号,只能按 registerTimeoutMs: 30_000 × maxRegisterAttempts: 6 一轮轮超时,且每轮超时前都先 rewarm()(LocalFilesControl.tsx:205 → actions.preheatAcp)去敲一个不会应答的 daemon。文档 §8-4c 那套重试预算是为"ACP 子进程冷启/被回收"这种暂时失败设计的,"通道根本没开"是永久失败,共用同一预算不合适。建议 LocalFilesControl 顺手从 useWorkspace() 取 capabilities,命中时直接置为 unavailable + 一条专用文案("当前 daemon 未开启本地文件通道"),不进 connecting、不 preheat。
(4) held-elsewhere 是个无出口死角,且旁观 tab 的「断开」会抹掉属主 tab 的授权。 LocalFilesControl.tsx:70-74:active 把 held-elsewhere 也算进去 → canConnect = false(既没有"重试"也没有"接管"按钮,只有给 cross-origin-frame 用的"在新标签页打开"),而 start() 里 ifAvailable 只试一次不排队(bridge-client.ts:231-234),属主 tab 关掉后没有任何机制把旁观 tab 提为新属主,只能刷新页面。更实际的一条:granted = status.rootName !== undefined || busy || active 让旁观 tab 也显示「断开」,而 disconnect()(useLocalFilesBridge.ts:304-316) 无条件 void store?.clear(),directory-handle-store.ts:11-14 用的是 DB_NAME='qwen-local-files' + KEY='directory' 的 per-origin 单条记录、A/B 共享。于是用户在旁观 tab 点了一下"断开",正在工作的属主 tab 当场无感(handle 在内存里),但一刷新就退回 needs-gesture、必须重新走一遍原生目录选择器。建议:held-elsewhere 时把主按钮改成"重试接管"(只重试 bridge.start(),不重选目录),并让该相位的 disconnect() 跳过 store.clear()。
(5) tx.oncomplete 这条安全语义缺回归守卫。 directory-handle-store.ts:50,58 实现是对的(request.onsuccess 只取值不 resolve,tx.oncomplete 才 resolve),文档 §8-4a 也专门写了理由(提交前 ack 会在页面离开时丢掉授权)。但整套 Fake 测试里 settle()(directory-handle-store.test.ts:26)无论成败都会同时安排 oncomplete/onabort,没有任何一条用例制造"onsuccess 已触发而 oncomplete 未触发"的时序 —— 把 :58 改回 request.onsuccess = () => resolve(value),现有测试全绿。代价是 save() 在事务未提交时返回 true,UI 报"已连接",用户切走 tab → Chrome 丢事务 → 下次刷新授权静默消失。加一条断序用例即可钉住。
🟢 Nit
sessionScopedServerNames(client-mcp-sender-registry.ts:92)只增不减:deleteSession(:138-146) 删了sessionSenders却没把 name 从该 Set 移除。场景:某 name 在本 daemon 生命周期内先被会话级注册过,之后任何 workspace 级同名注册会在lookup:176-182被永久硬拒成 "requires a session context",且极难定位。内存增长受 distinct server 名约束,可忽略。- 校验不对称:
server走isValidServerName(≤256 + 非保留属性名),sessionId只校验"非空字符串"(client-mcp-ws.ts:220-228)。作为 Map key 不存在原型污染,代价是一条连接可用超长 sessionId 注册条目、活到断开为止。 search()的maxFiles只统计 file,目录入队不计预算(local-directory.ts:417-499,:441 前无目录计数短路):一个"极大而几乎无文件"的目录骨架能让 BFS 一路走到底,把 tab 挂在这一次 tool call 上。建议补maxDirectories。- 控制字符正则
[\u0000-\u001f](local-directory.ts:181-186)漏掉 DEL\u007f与 C1,与注释"控制字符"口径不完全一致(不构成穿越)。 list()先截断后排序(local-directory.ts:249-293),返回的 N 项是"枚举顺序前 N 再内部排序",不是字典序前 N;模型可能误判已看全目录。建议 truncated 时在 note 里讲清语义。not_a_file是死码:枚举里声明(local-directory.ts:44-52)但toLocalDirectoryError(:212-216) 只把TypeMismatchError映射成not_a_directory。void bridge.start()无.catch(useLocalFilesBridge.ts:204):沙箱化 iframe 里navigator.locks.request会以 SecurityError 拒绝,届时bridgeRef.current指向一只永不动的 bridge 且running停在 true → UI 卡在 connecting 而无 failed 文案。- vite 代理
'^/acp/?$'(vite.config.ts:134-140)不含 query、也没有二级 workspace 条目;对照同文件 :47 的QUALIFIED_VOICE_STREAM_PROXY先例,建议'^/acp(/?)?(\\?.*)?$'并补 qualified 一条。当前buildAcpWsUrl不带 query,故非功能缺陷。 - 文档 §8-4d 称
localFiles.*有 27 个键,实现里 en(i18n.tsx:1402-1428)/zh(:4881-4907)各 23 且完全对齐 —— 不是漏译,是文档超卖,建议同步以免后续按 27 校对时误报。 list_directory未输出 mtime(mcp-server.ts:186-192),与文档 §6 承诺的"类型 + 大小 + mtime"不符;模型因此答不了"最近改动的是哪些文件",容易退化成反复read_file试探。restore()(:213-234) 只做queryPermission不requestPermission(正确),但connect()在请求 activation 之前仍可能await store?.load()(:270);慢盘/主线程拥塞时 transient activation 窗口被消耗,requestPermission()会以 NotAllowedError 失败并 fall through 到pickDirectoryHandle(:285) —— 用户预期"授权一次即可"却突然看到"重新选目录"的原生框。建议 handle 已在内存时先同步 requestPermission,或在restore()完成前禁用按钮。
测试覆盖判断(只读,未运行)
钉得扎实的:bridge-client.test.ts 的 initialize 超时→recycle、注册 6 次预算、mcp_registered 才归零两个计数器、mcp_unregistered→重注册、mcp_error 分流、非本 server 帧被忽略、held-elsewhere;mcp-server.test.ts 的握手幂等 / isError vs JSON-RPC error / 无删除工具;useLocalFilesBridge.test.tsx 的挂载静默恢复、needs-gesture→点击、needs-session→自动重绑、双击只开一次 picker(:489-517)、connect 比视图活得久(:519-544)、connect 与 disconnect 竞态(:546-573)、卸载关 socket(:575-588)。local-directory.test.ts 覆盖 ../内嵌 ../绝对/盘符/反斜杠/控制字符/% 放行/字节上限/二进制 NUL/"非法路径不触文件系统"/上界不可撑开/search 逐文件上限/写失败不留半文件 —— 没看到恒真式假覆盖。
真正的缺口恰好落在 Critical 那条上:
- 上面 (1) 构造不出来:
FakeSocket.close()同步触发 close handler,且无"向已 close 的 socket 投帧"能力。建议让 FakeSocket 支持手动 emit,补三条断言:断开后收到mcp_registered不得变 connected;收到mcp_unregistered不得留在 reconnecting;断开后server.handle的调用增量为 0。 - 没有用例模拟"daemon 不认识
mcp_register"(回id:null畸形帧)并断言不会白跑 6×30s(对应 Important (3))。 useLocalFilesBridge.test.tsx每个用例都显式传locks: null,defaultLocks()与 held-elsewhere 的 hook→UI 全链路零覆盖;而且FakeLocks的拒绝方式是"从不调用回调",真实 Chrome 的ifAvailable是以lock === null调回调一次 —— 两种形状都被 :233 挡住了,但测试只覆盖了与真实浏览器不一致的那一种。- 没有"已 connected 之后 sessionId 变化 → 旧 bridge 被 stop、新 bridge 起来"的用例(只覆盖了 needs-session→首次出现 session)。
- 没有两个并发
tools/call交叉回包来钉 JSON-RPCid关联的用例(逻辑本身正确,answerRpc用frame.id回包,但零防护)。 localFiles.trigger只作aria-label/title,而 12 态中文断言走textContent→ 该键漏译会静默回落成英文。
需要人工确认(低置信,不作为阻塞项)
- 二级 workspace 定址:
buildAcpWsUrl(bridge-client.ts:127-135)只保留 base 的 origin+pathname 再接acp,因此永远打到 primaryBridge(serve/server.ts:3257);而 daemon 同时存在/workspaces/:workspace/acp二级挂载(acp-http/index.ts:1412,1520),dispatch.ts:1239-1255在params.workspaceCwd与挂载绑定工作区不一致时直接抛WorkspaceMismatchError。我无法在 head tree 里证明"web-shell 存在 session 落在二级 workspace runtime 的形态",所以不能断定这是缺陷 —— 请作者确认;若存在该形态,本桥需要限定路径 + 一条对应的 vite 代理。 - token 时效:
LocalFilesControl.tsx:196用 provider 注入的token(源自main.tsx:235的getDaemonToken()),而TerminalPanel.tsx:53-57是连接时实时读getDaemonToken()。启动期两者等价;若将来支持 token 轮换,桥会拿旧 token 静默 401(表现为 close→重连耗尽→failed)。 sendInitialize(:321-339) 只按id === INITIALIZE_ID认回包,不校验protocolVersion/agentCapabilities,协议不兼容时会晚失败。
结论 / Verdict
🔄 请求修改(Request changes) —— 但只差一条:上面 Critical (1) 的 stopped / socket 身份守卫(连同 FakeSocket 补一条能构造该序列的用例)修掉,我会直接批准。
这个 PR 的安全模型是认真的,会话隔离、审批链、路径穿越三道关口我都逐分支验过并成立;文档还主动标了自己的残余风险和刻意不对称,这在同规模 PR 里不多见。Critical 与几条 Important 集中在生命周期的收尾上 —— "用户说停了"到"执行真的停了"之间缺一个原子边界,以及降级矩阵漏了"部署侧根本没开这个通道"那一格。都不大,但都属于这个功能的安全承诺本身。
Conclusion: request changes. The security model holds under verification (session isolation, tool-approval chain, path-traversal facade), and the design doc discloses its own residual risks rather than glossing them. The blocking item is narrowly about lifecycle: stop() / unmount does not atomically cancel an in-flight tools/call, so a real local write can still land after the user revokes, and two delayed frames can drive the UI into a permanently inconsistent state. Fix the stopped / socket-identity guards in bridge-client.ts (plus a FakeSocket test that can express the sequence) and this is good to go.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": tracing useDaemonConnection 's sessionId lifecycle to determine whether it can transiently become undefined while mounted (would make the rebind effect tear ….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
仅完成部分审查,审查缺口已披露。
未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 2)":tracing useDaemonConnection 's sessionId lifecycle to determine whether it can transiently become undefined while mounted (would make the rebind effect tear …。
未审查:反向审计——评审时间预算不足,未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.23.0)
…ridge - stop() during a pending lock grant leaked the cross-tab Web Lock until reload: the lock callback now checks stopped, and run() settles itself when stopped lands between grant and run. - same-tab bridge replacement raced the settling lock release into a terminal held-elsewhere: acquisition retries a few times first. - retryRegister could duplicate mcp_register when the ack landed during rewarm (already_registered is terminal): re-check the phase, and sendRegister clears any live timer before arming a new one. - frames delivered after stop() overwrote the replacement bridge's status through the shared onState: onMessage drops them. - the bridge always targeted the primary /acp mount, so secondary-workspace sessions could never register: add a workspace selector and the workspace-qualified route voice already uses. - TypeMismatchError at file-expecting sites reported not_a_directory for a directory; fractional limits truncated to zero and reported a non-empty directory as empty; enumeration errors escaped as raw -32603 protocol errors; truncated zero-hit search said plain 'No match'. - design doc: correct the traversal, mtime, removal-awareness and slice-status statements the review proved stale or false.
… no-match Matching is per line, so a pattern containing a newline can never hit; scanning to a definitive 'No match' reported a false negative the model would act on. Fail closed with invalid_path and say matching is single-line.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- Round-2 candidate 'explicit mcp_unregister scope passthrough untested (confirmed by mutation)' — already reported in round 1 as R1-13 (inline comment on packages/cli/src/serve/acp-http/client-mcp-ws.ts:307, deferred by author)
- Round-2 candidate 'zh-CN matrix cannot detect a missing Chinese key (confirmed by mutation)' — already reported in round 1 as R1-10 (inline comment on packages/web-shell/client/components/LocalFilesPanel.test.tsx:136, deferred by author)
- Round-2 candidate 'register-side supersession recheck untested (confirmed by mutation)' — already reported in round 1 as R1-14 (inline comment on packages/cli/src/serve/acp-http/client-mcp-sender-registry.ts:300, deferred by author)
- Round-2 candidate 'resolve-on-commit guarantee unpinned in fake IDB (confirmed by mutation)' — already reported in round 1 as R1-17 (inline comment on packages/web-shell/client/local-files/directory-handle-store.ts:58, deferred by author)
- Round-2 candidate 'held-elsewhere Disconnect wipes the origin-shared grant (confirmed by probe)' — already reported in round 1 as R1-40 (inline comment on packages/web-shell/client/local-files/useLocalFilesBridge.ts:316, deferred by author)
1 candidate finding(s) this round's reviewers re-derived matched entries already carried on this PR and were set aside before verification (R1-11) — a matched posted finding is ruled in the previous-round status as always, and a matched deferral stays on the standing deferral record.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/local-files/bridge-client.ts:452 — [review] JSON.parse('null') frame throws an unhandled rejection in onMessage (code-age: unchanged since round 1)packages/web-shell/client/local-files/local-directory.test.ts:422 — [review] Literal-search fixture cannot distinguish literal from regex matching (code-age: unchanged since round 1)packages/web-shell/client/local-files/useLocalFilesBridge.ts:125 — [review] phaseFromBridge failed/reconnecting mapping has no hook-level witness (code-age: unchanged since round 1)packages/web-shell/client/local-files/bridge-client.ts:409 — [review] Permanent register failures burn the rewarm retry budget (code-age: unchanged since round 1)packages/web-shell/client/local-files/local-directory.ts:345 — [review] too_large error advertises 'Narrow the request' but windowing can never help (code-age: unchanged since round 1)packages/web-shell/client/local-files/local-directory.ts:313 — [review] Truncated listings are an arbitrary subset rendered as an alphabetical prefix (code-age: unchanged since round 1)packages/web-shell/client/local-files/mcp-server.test.ts:113 — [review] id-0 requests (the SDK's first initialize) have no witness (code-age: unchanged since round 1)packages/web-shell/client/local-files/mcp-server.ts:326 — [review] Past-EOF reads are indistinguishable from empty files (code-age: unchanged since round 1)packages/web-shell/client/local-files/mcp-server.test.ts:384 (+2 locations) — [review] callTool pass-through branches (search path/maxBytes, list path/limit) unwitnessed (code-age: unchanged since round 1)
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 5 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
本轮评审重新推导出的 1 条候选发现与本 PR 已携带的条目匹配,已在验证前搁置(R1-11)——被匹配的已发布条目照常在上一轮状态区裁定,被匹配的延后条目仍保留在延后清单记录中。
未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查:反向审计——在 3 轮的反审轮数上限内未收敛。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 9 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.23.0)
…ridge - Scope the session-registration rollback to the failing registration via a per-attempt token, so a stale late-failing add cannot tear down a live re-registration on the same connection. - Treat already_registered as benign (the daemon still holds our own earlier frame): wait it out with a budget-neutral probe instead of failing; scope retry continuations to the socket that started them. - Give inbound socket events socket identity so a recycled socket's late close or initialize reply cannot detach or re-register the replacement. - Drop tool replies whose disconnect or recycle landed mid-flight instead of answering on a cleared or replaced socket. - Decode reads strictly (fatal UTF-8) so non-UTF-8 files are refused rather than returned as mojibake that a read-modify-write would cement; reject NUL-bearing search patterns like multi-line ones. - Stop falling through to a gesture-less picker after requestPermission consumed the click's activation; keep the ungranted handle out of handleRef so no rebind can promote it. - Rebind when the workspace selector resolves late, resolve it against the merged workspace list like the voice call sites, and warm the owning runtime on registration retries; proxy the qualified ACP upgrade in dev. - Remove the mcp_unregistered recovery branch: nothing in the daemon emits it spontaneously, so it pinned a frame that can never arrive.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
20 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- search() file budget bypass via failing getFile() — re-derived 3×; already reported as R2-11 (comment 3935972538); re-posted still-standing under R2-11
- sessionScopedServerNames one-way / never cleared — re-derived 3×; already reported as R1-5 (comment 3931485174), author acknowledged, deferred
- held-elsewhere is terminal / no takeover path — re-derived 3×; already reported as R1-6 (comment 3931485236), author acknowledged, deferred
- zh-CN matrix cannot detect missing zh keys — re-derived 2×; already reported as R1-10 (comment 3931485217), author deferred
- fake IndexedDB never invokes onupgradeneeded — re-derived 3×; already reported as R1-11 (comment 3931485255), author acknowledged, deferred
- mcp_unregister frame scope passthrough unpinned — re-derived 2×; already reported as R1-13 (comment 3931485209), author deferred
- search byte-budget termination unpinned — re-derived 2×; already reported as R1-15 (comment 3931485329), author deferred
- resolve-on-tx.oncomplete semantics unpinned — already reported as R1-17 (comment 3931485267), author deferred
- Web Locks callback(null) decline shape unpinned — already reported as R1-35 (comment 3931485224), author deferred
- non-owner tab disconnect wipes shared grant — already reported as R1-40 (comment 3931485397), author acknowledged, deferred
- list() degrade-on-unreadable-file unpinned — already reported as R1-43 (comment 3931485285), author deferred
- non-positive maxFiles/maxBytes zero-scan clamping — already reported as R1-45 (comment 3931485319), author acknowledged, clamping deferred
- write_file created:false branch unpinned — already reported as R1-46 (comment 3931485338), author deferred
- FakeTransaction.delete ignores failRequests — re-derived 2×; already reported as R1-47 (comment 3931485247), author deferred
- openLocalDirectory dead export — re-derived 2×; already reported as R1-53 (comment 3931485351), author acknowledged, deferred
- disconnect() handleRef reset unpinned — already reported as R1-54 (comment 3931485360), author deferred
- disconnect-race handle-not-persisted unpinned — already reported as R1-55 (comment 3931485371), author deferred
- connectInFlightRef finally-reset unpinned — already reported as R1-56 (comment 3931485369), author deferred
- JSON.parse('null') frame unhandled rejection in onMessage — already deferred in round 2 (review 5115523123, code-age deferral list)
- callTool maxBytes/path passthrough unwitnessed — re-derived 2×; already deferred in round 2 (review 5115523123, code-age deferral list)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped at the 3-round cap with round 3 still reporting findings (no dry convergence).
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/local-files/local-directory.ts:484 — [probe] search() has no budget for directory entries; directory-heavy trees are walked in full (code-age: unchanged since round 2)packages/web-shell/vite.config.ts:146 — [review] the bare '^/acp/?$' dev-proxy entry has no test (code-age: entry predates round 2)packages/web-shell/client/local-files/useLocalFilesBridge.test.tsx:263 — [probe] socket frames emitted outside act() produce React act warnings on every run (code-age: unchanged since round 2)packages/web-shell/client/local-files/bridge-client.ts:434 — [probe] the rewarm-rejection path in retryRegister is untested; escaping rejection stalls in registering (code-age: unchanged since round 2)docs/design/2026-09-03-client-filesystem-bridge.md:105 — [review] doc line citations into PR-changed files were not rebased onto HEAD (code-age: doc unchanged since round 2)packages/web-shell/client/local-files/useLocalFilesBridge.test.tsx:350 — [probe] restore()'s generation guards have no witnessing test; mutant survives green (code-age: restore tests unchanged since round 2)packages/web-shell/client/local-files/useLocalFilesBridge.test.tsx:564 — [probe] overlapping act scopes in the double-click test warn on every run (code-age: unchanged since round 2)packages/web-shell/client/local-files/bridge-client.test.ts:187 — [review] the mcp_registered ack's register-timer clear has no witness (code-age: unchanged since round 2)docs/design/2026-09-03-client-filesystem-bridge.md:125 — [review] §6's fact-9 idempotency justification cites the forbidden workspace fan-out (code-age: doc unchanged since round 2)
Convergence: round 3 posted 9 inline comment(s), 8 of them reported for the first time; the previous round posted 13 (13 new). Findings keep coming back to the same files: packages/web-shell/client/components/LocalFilesControl.tsx (findings in rounds 1, 2; 2 more now); packages/web-shell/client/local-files/bridge-client.ts (findings in round 2; 1 more now); packages/web-shell/client/local-files/local-directory.ts (findings in round 2; 1 more now), and 1 more file(s). 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. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 20 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查(原文为英文):reverse audit — stopped at the 3-round cap with round 3 still reporting findings (no dry convergence).
收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 9 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 3 轮发布了 9 条行内评论,其中 8 条是首次提出;上一轮发布了 13 条(其中 13 条首次提出)。发现反复回到同一批文件:packages/web-shell/client/components/LocalFilesControl.tsx(第 1、2 轮已出过发现,本轮又有 2 条);packages/web-shell/client/local-files/bridge-client.ts(第 2 轮已出过发现,本轮又有 1 条);packages/web-shell/client/local-files/local-directory.ts(第 2 轮已出过发现,本轮又有 1 条),另有 1 个文件。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.23.0)
…ridge - Treat a rate_limited mcp_error as the transient shed it is: ignore it while connected, join the retry budget while registering, instead of terminating the bridge and turning one shed frame into a session-level outage. - Close an IndexedDB connection whose open succeeds after being blocked: blocked is not terminal, and the leaked connection would block every future version upgrade. - Re-query permission before the session/selector rebind starts a bridge, so a grant revoked after the original connect cannot re-register tools whose every call the browser rejects; a disconnect landing during the query wins. - Decode search targets with the same fatal UTF-8 decode read() uses, so undecodable files count as skipped instead of scanning as mojibake and reporting a false no-match; count a failed getFile against the file budget so a mid-scan revocation stops at the cap. - Fall back to the legacy preheat only when the qualified ensure route is actually missing (404); any other failure surfaces instead of silently warming the primary runtime for a secondary session. - Pin the replaced socket's close on the rebind path, and update the design doc's registry paragraph to the registration-token rollback it now uses.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
16 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- literal-search fixture cannot distinguish literal from regex matching — already reported as R1-12 (inline comment on packages/web-shell/client/local-files/local-directory.test.ts, author deferred)
- too_large error advertises 'Narrow the request' but windowing can never help — already deferred in round 2 (review 5115523123, code-age deferral list)
- search_files path passthrough unasserted in callTool — already deferred in round 2 (review 5115523123, 'callTool pass-through branches (search path/maxBytes, list path/limit) unwitnessed')
- list_directory limit passthrough unasserted — already deferred in round 2 (review 5115523123, same 'callTool pass-through branches' entry)
- write_file created:false branch unpinned — already reported as R1-46 (inline comment on packages/web-shell/client/local-files/mcp-server.test.ts, author deferred)
- openLocalDirectory dead export — already reported as R1-53 (inline comment on packages/web-shell/client/local-files/pick-directory.ts, author acknowledged, deferred)
- socket frames emitted outside act() produce React act warnings — already deferred in round 3 (review 5118117036, code-age deferral list)
- sessionScopedServerNames one-way / never cleared — already reported as R1-5 (inline comment on packages/cli/src/serve/acp-http/client-mcp-sender-registry.ts, author acknowledged, deferred by maintainer decision; re-derived this round at Cri…
- held-elsewhere terminal with no takeover path — already reported as R1-6 (inline comment on packages/web-shell/client/local-files/bridge-client.ts, author acknowledged, deferred)
- list() degrade-on-unreadable-file guard untested — already reported as R1-43 (inline comment on packages/web-shell/client/local-files/local-directory.test.ts, author deferred)
- non-positive maxFiles/maxBytes zero-scan clamping — already reported as R1-45 (inline comment on packages/web-shell/client/local-files/local-directory.ts, partially mitigated, clamping deferred)
- BUSY-phase affordances for connecting/reconnecting unpinned — already reported as R1-34 (inline comment on packages/web-shell/client/components/LocalFilesPanel.test.tsx, author deferred)
- FakeTransaction.delete ignores failRequests — already reported as R1-47 (inline comment on packages/web-shell/client/local-files/directory-handle-store.test.ts, author deferred)
- JSON.parse('null') frame unhandled rejection in onMessage — already deferred in round 2 (review 5115523123, code-age deferral list)
- maxBytes passthrough promised by test title but never set — already deferred in round 2 (review 5115523123, 'callTool pass-through branches' entry)
- bare '^/acp/?$' dev-proxy entry has no test — already deferred in round 3 (review 5118117036, code-age deferral list)
Not reviewed: reverse audit — stopped at 3-round cap with round 3 still reporting findings (no dry convergence).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": none — but note the connecting phase has no affordance (button-visibility) test anywhere in this file; I did not file it because it duplicates the shape of fi….
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/acp-http/client-mcp-ws.ts:305 — [review] explicit mcp_unregister frame scope passthrough untested (confirmed by mutation)packages/web-shell/client/components/LocalFilesPanel.test.tsx:136 — [review] zh-CN matrix cannot detect a missing Chinese key (confirmed by mutation)packages/web-shell/client/local-files/directory-handle-store.test.ts:140 — [review] fake IndexedDB never invokes onupgradeneeded (confirmed by mutation)packages/web-shell/client/local-files/local-directory.ts:168 — [review] drive-letter rejection branch never exercised alone (confirmed by mutation)packages/web-shell/client/local-files/local-directory.ts:400 — [review] write cap allocates the full byte array before rejecting (code-age: unchanged since round 3)packages/web-shell/client/local-files/local-directory.ts:521 — [review] search bytes budget exercised by no test at either layer (confirmed by mutation)packages/web-shell/client/local-files/bridge-client.ts:533 — [review] already_registered re-probe path never executes in any test (confirmed by mutation)packages/web-shell/client/components/LocalFilesControl.tsx:222 — [review] createLocalFilesRewarm kind:'cwd' branch untested (confirmed by mutation)packages/web-shell/client/local-files/bridge-client.test.ts:71 — [review] no test pins non-blocking lock acquisition (ifAvailable) — confirmed by mutationpackages/web-shell/client/local-files/bridge-client.test.ts:606 — [review] fabricated -32603 error reply id reconstruction unpinned — confirmed by mutationpackages/web-shell/client/local-files/bridge-client.test.ts:659 — [review] mcp_registered streak-reset positive half unpinned — confirmed by mutationpackages/web-shell/client/local-files/bridge-client.test.ts:695 — [review] 15s backoff cap exercised by no test — measured intact vs mutant delay sequencespackages/web-shell/client/components/LocalFilesControl.tsx:136 — [review] reconnecting status.message set but never rendered — confirmed by render probepackages/web-shell/client/local-files/local-directory.test.ts:419 — [review] failed-write test pins close() and its content assertion is vacuous — confirmed by probepackages/web-shell/client/local-files/bridge-client.test.ts:819 — [review] stop during the backoff delay unpinned — confirmed by probe pairpackages/web-shell/client/local-files/bridge-client.ts:557 — [review] drain-time rpcError envelope discarded, burning the register budget — confirmed against the live serve layerpackages/web-shell/client/local-files/local-directory.ts:17 — [review] LocalFileLike.text()/type dead surface — text() is the lossy decode the module refusespackages/web-shell/client/local-files/local-directory.ts:90 — [review] LocalDirectoryEntry.lastModified populated with no production read sitepackages/web-shell/client/local-files/local-directory.ts:284 — [review] control-character entry names corrupt line-based listing output — confirmed by probepackages/web-shell/client/local-files/useLocalFilesBridge.test.tsx:450 — [review] selectorKey normalization unwitnessed — confirmed by value-equal rerender probe
Convergence: round 4 posted 6 inline comment(s), 5 of them reported for the first time; the previous round posted 9 (8 new). Findings keep coming back to the same files: packages/web-shell/client/local-files/local-directory.ts (findings in rounds 2, 3; 2 more now); packages/web-shell/client/local-files/bridge-client.ts (findings in round 3; 1 more now); packages/web-shell/client/local-files/directory-handle-store.ts (findings in round 3; 1 more now), and 1 more file(s). 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. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 16 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查(原文为英文):reverse audit — stopped at 3-round cap with round 3 still reporting findings (no dry convergence).
未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 2)":none — but note the connecting phase has no affordance (button-visibility) test anywhere in this file; I did not file it because it duplicates the shape of fi…。
收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 20 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 4 轮发布了 6 条行内评论,其中 5 条是首次提出;上一轮发布了 9 条(其中 8 条首次提出)。发现反复回到同一批文件:packages/web-shell/client/local-files/local-directory.ts(第 2、3 轮已出过发现,本轮又有 2 条);packages/web-shell/client/local-files/bridge-client.ts(第 3 轮已出过发现,本轮又有 1 条);packages/web-shell/client/local-files/directory-handle-store.ts(第 3 轮已出过发现,本轮又有 1 条),另有 1 个文件。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)
机制健康:本轮未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff,除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上;并会一直如此,直到某一轮的标记重新带上锚点,或落地的嫁接能被运行该轮的评审使用。(仅陈述,不据此行动——这不改变本轮发布的任何内容。)
— qwen3.8-max via Qwen Code /review (v0.23.0)
…ridge - Resolve paths verbatim: a whole-input trim made whitespace-padded names (legal on POSIX, echoed verbatim by list/search) resolve to a different file, so a read-modify-write cycle could clobber the wrong file. - Reject write content with unpaired surrogates: TextEncoder silently replaces them with U+FFFD, committing different bytes than requested while reporting success - the symmetric lossy step read() refuses. - Stop the previous session's bridge when a rebind re-query finds the grant lapsed, and reconcile the in-memory handle against the store so a peer tab's disconnect cannot be silently revived by a session switch. - Withhold the bridge for untrusted/live/ambiguous session workspaces instead of collapsing them onto the primary mount (new 'none' route outcome plus a workspace-ineligible blocker). - Single-flight retryRegister so concurrent continuations cannot double the rewarm/register traffic and halve the consecutive-failure budget.
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
9 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- superseded-after-success registration branch untested — already reported as R1-14 (inline comment on packages/cli/src/serve/acp-http/client-mcp-sender-registry.ts, author deferred)
- zh-CN matrix cannot detect a missing Chinese key — already reported as R1-10 (inline comment on packages/web-shell/client/components/LocalFilesPanel.test.tsx, author deferred)
- JSON.parse('null') frame unhandled rejection in onMessage — already deferred in round 2 (review 5115523123, code-age deferral list)
- fake IndexedDB never invokes onupgradeneeded — already deferred in round 4 (review 5119536520, deferral list)
- search bytes budget exercised by no test at either layer — already deferred in round 4 (review 5119536520, deferral list)
- search() directory traversal uncapped — already deferred in round 3 (review 5118117036, code-age deferral list)
- callTool maxBytes pass-through untested — already deferred in round 2 (review 5115523123, 'callTool pass-through branches' entry)
- bare '^/acp/?$' dev-proxy entry has no test — already deferred in round 3 (review 5118117036, code-age deferral list)
- mcp_unregister frame scope passthrough untested — already reported as R1-13 (inline comment on packages/cli/src/serve/acp-http/client-mcp-ws.ts, author deferred)
Not reviewed: Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally — the agent returned no evidence of its walk twice.
Not reviewed: coverage proof — this session's harness transcript store was deleted externally twice mid-run; two full fan-outs (31 agents each, all receipts substantive) read the diff, but the deterministic coverage gate can no longer prove it from transcripts.
Not reviewed: reverse audit — could not run this round: the review worktree was deleted externally before any audit round could launch.
Not reviewed: build-and-test — build green (18/18 workspaces incl. both changed packages); test suites inconclusive: packages/cli suite hit its harness time budget (infrastructure), packages/web-shell suite crashed on externally-deleted node_modules files mid-run; efficacy probe unmeasured (worktree destroyed).
Not reviewed: coverage — could not read the agents' transcripts (no subagent transcripts at /home/github-runner/actions-runner-hk1-16/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-16--work-qwen-code-qwen-code/subagents/7b799922-7f10-4777-869b-3fd2c6389e56 (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-16/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-16--work-qwen-code-qwen-code/subagents/7b799922-7f10-4777-869b-3fd2c6389e56'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.), so this run cannot show that any of the diff was read.
Not reviewed: verification — could not check that Step 4 and Step 5 ran (no subagent transcripts at /home/github-runner/actions-runner-hk1-16/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-16--work-qwen-code-qwen-code/subagents/7b799922-7f10-4777-869b-3fd2c6389e56 (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-16/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-16--work-qwen-code-qwen-code/subagents/7b799922-7f10-4777-869b-3fd2c6389e56'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.).
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/local-files/bridge-client.ts:249 — [review] stop()/start() restart inherits the exhausted reconnect streak (code-age: unchanged since round 4)packages/web-shell/client/local-files/local-directory.ts:144 — [review] splitRelativePath docstring claims empty segments throw; they are skipped (code-age: unchanged since round 4)
Convergence: round 5 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 6 (5 new). Findings keep coming back to the same files: packages/web-shell/client/local-files/local-directory.ts (findings in round 4; 1 more now); packages/web-shell/client/local-files/useLocalFilesBridge.ts (findings in round 3; 1 more now). 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. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
本轮确认的 9 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally——该 agent 连续两次未返回任何检查过程的证据。
未审查(原文为英文):coverage proof — this session's harness transcript store was deleted externally twice mid-run; two full fan-outs (31 agents each, all receipts substantive) read the diff, but the deterministic coverage gate can no longer prove it from transcripts.
未审查(原文为英文):reverse audit — could not run this round: the review worktree was deleted externally before any audit round could launch.
未审查(原文为英文):build-and-test — build green (18/18 workspaces incl. both changed packages); test suites inconclusive: packages/cli suite hit its harness time budget (infrastructure), packages/web-shell suite crashed on externally-deleted node_modules files mid-run; efficacy probe unmeasured (worktree destroyed).
未审查:覆盖情况——无法读取 agent 的运行记录(no subagent transcripts at /home/github-runner/actions-runner-hk1-16/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-16--work-qwen-code-qwen-code/subagents/7b799922-7f10-4777-869b-3fd2c6389e56 (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-16/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-16--work-qwen-code-qwen-code/subagents/7b799922-7f10-4777-869b-3fd2c6389e56'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.),本次运行无法证明 diff 的任何部分被读过。
未审查:验证——无法检查步骤 4 与步骤 5 是否运行(no subagent transcripts at /home/github-runner/actions-runner-hk1-16/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-16--work-qwen-code-qwen-code/subagents/7b799922-7f10-4777-869b-3fd2c6389e56 (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-16/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-16--work-qwen-code-qwen-code/subagents/7b799922-7f10-4777-869b-3fd2c6389e56'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.)。
收敛姿态下延后(第 5 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 5 轮发布了 3 条行内评论,其中 3 条是首次提出;上一轮发布了 6 条(其中 5 条首次提出)。发现反复回到同一批文件:packages/web-shell/client/local-files/local-directory.ts(第 4 轮已出过发现,本轮又有 1 条);packages/web-shell/client/local-files/useLocalFilesBridge.ts(第 3 轮已出过发现,本轮又有 1 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.23.0)
… bridge - Stop the running bridge and withhold when a blocker activates late: the rebind effect never consulted capability.blocker, so a session switch or a late capabilities resolution into an untrusted/live workspace re-dialed the primary mount instead of reporting unavailable/workspace-ineligible. - Strengthen the zh-CN panel matrix: a missing zh key falls back to the EN string, which the raw-key assertion cannot catch, so every row now pins its actual zh string (mutation-verified by deleting the new key). - Rename the verbatim-resolution alias from trimmed to input so the name cannot invite re-introducing the trim that R4-1 removed.


























What this PR does
When the daemon runs somewhere other than the developer's own machine — a cloud box, a container, a shared host — the agent can only ever see the filesystem the daemon itself can reach. This adds a way for the person sitting in the browser to hand one of their own directories to a single conversation: they pick a folder through the browser's native directory chooser, and from then on that conversation can list, read, search, and write files inside it, with every path resolved relative to the folder they chose.
The directory never becomes part of the daemon's workspace, and it is not mounted as a second root. It surfaces as four extra tools that exist only inside the one conversation the user connected it to, and they disappear when the tab closes, the user disconnects, or the conversation ends. A new entry in the sidebar footer shows what is currently connected and how many tools it exposes, and — when the browser or the deployment cannot support this at all — explains why and what to do instead of offering a button that could only fail.
Getting there needed a change on the daemon side too. The reverse channel that lets a browser host tools for the agent already existed, but it could only register them workspace-wide. This adds a session-scoped variant, so a client-hosted server can be bound to exactly one conversation.
The packaged desktop app is the one host where the entry is hidden by default. It always spawns its own loopback daemon, whose regular tools already reach that machine's disk, so the bridge would add nothing there — and on WebKit webviews the browser picker does not exist, so the entry could only ever render dead. An explicit footer configuration can still turn it on.
Why it's needed
A remote daemon is increasingly the normal way to run this, and a remote daemon cannot touch the developer's own checkout, notes, or data. Until now the choices were to copy files up front and copy results back afterwards, or to run the daemon locally and give up whatever the remote host provides. Neither is a good fit for "let the agent fix something in the project on my laptop while the model and the heavy lifting stay on the remote box."
The session-scoped registration is not just plumbing for this feature — it closes a hole that the workspace-wide path has. A workspace-wide client-hosted server is copied onto every conversation in that workspace, including ones created later, and is therefore reachable from channel-driven conversations and background agents. For a server whose tools read and write somebody's local disk, that is the wrong blast radius: one person connecting a folder would silently extend it to every other conversation sharing the workspace. Binding to a single conversation, and hard-rejecting calls that arrive from any other one, is what makes the capability safe to offer at all.
The reverse channel itself had no remaining client: the browser extension that originally used it moved to a different mechanism, leaving the code path wired and tested but unused. This gives it a real consumer.
Reviewer Test Plan
How to verify
Run a daemon with the reverse channel enabled and open the Web Shell against it in Chrome or Edge, over https or on localhost — a plain-http remote origin is not a secure context and the browser will not grant local file access there.
Evidence (Before & After)
Before: no way for a conversation to reach the browser side's filesystem; the reverse channel had no client and could only register workspace-wide.
After: screenshots of the sidebar entry, the unconnected popover, the connected popover showing the folder name and four tools, the unsupported-browser degradation state, and the Chinese connected state.
The connected captures used a stubbed directory handle, because the native chooser cannot be driven headlessly; the registration against the real daemon and the four discovered tools shown in the popover are real. The manual acceptance run described above used a real handle end to end.
Acceptance evidence beyond the UI: a scripted run against a real daemon, a real browser and a real model placed a file containing a random token in a directory, had the browser grant that directory, and asked the agent to read the file and quote the token back. The token appears nowhere in the prompt and cannot be guessed, so its appearance in the conversation stream proves the bytes travelled disk → browser → daemon → model → back. That run passed, and the daemon log independently shows the conversation attaching, the prompt being accepted, and the turn completing.
Automated coverage on this branch: 24 daemon-side tests for the reverse channel including the new session scope, 568 Web Shell tests across the bridge and the sidebar, and the app-level suite. Type checking, lint and formatting are clean. A headless end-to-end run drives the real daemon and the real Web Shell source in a real browser and asserts that connecting produces a registration the daemon acknowledges with four discovered tools, and that disconnecting tears it down.
Tested on
Environment (optional)
Local daemon started from source with the reverse channel enabled, Web Shell served by the dev server so the TypeScript source is what runs, Chrome for the manual acceptance run and headless Chromium for the scripted end-to-end run.
Risk & Scope
Linked Issues
Related to #5626, which introduced the reverse tool channel this builds on. Does not close it.
中文说明
这个 PR 做了什么
当 daemon 跑在开发者本机之外的地方——云上、容器里、共享主机上——agent 能看到的就只有 daemon 自己够得着的文件系统。这个 PR 让坐在浏览器前的人可以把自己本机的一个目录交给某一个会话:通过浏览器原生的目录选择框选一个文件夹,此后这个会话就能在其中列目录、读文件、搜索、写文件,所有路径都相对于所选目录解析。
这个目录不会成为 daemon 工作区的一部分,也不会被挂载成第二个根。它以四个额外工具的形式出现,且只存在于用户连接它的那一个会话里;标签页关闭、用户断开、或会话结束时,这些工具就消失。侧边栏底部新增一个入口,显示当前连接了什么、暴露了多少工具;当浏览器或部署形态根本不支持时,它会解释原因和替代做法,而不是给一个点了必然失败的按钮。
为此 daemon 侧也需要一处改动。让浏览器为 agent 托管工具的反向通道本来就存在,但它只能做工作区级注册。这里新增了会话级变体,使一个客户端托管的 server 可以只绑定到一个会话。
打包的桌面应用是唯一默认隐藏该入口的宿主:它恒拉起自己的回环 daemon,其常规工具本就够得着那台机器的磁盘,桥在那里没有增量价值;而 WebKit webview 根本没有浏览器目录选择框,入口只能渲染成死物。显式 footer 配置仍可开启。
为什么需要
远端 daemon 正越来越成为常规用法,而远端 daemon 碰不到开发者本机的代码、笔记和数据。此前的选择只有两个:要么事先把文件拷上去、事后再把结果拷回来,要么把 daemon 跑在本地、放弃远端主机提供的一切。对于"让 agent 修我笔记本上的项目,同时模型和重活留在远端"这种诉求,两者都不合适。
会话级注册不只是这个特性的管道——它补上了工作区级路径本身的一个漏洞。工作区级的客户端托管 server 会被复制到该工作区下的每一个会话,包括之后新建的,因此渠道消息驱动的会话和后台 agent 都能调用到。对于一个工具会读写某人本地磁盘的 server 来说,这个影响范围是错的:一个人连接一个文件夹,就会静默地把它扩展给共享同一工作区的所有会话。绑定到单个会话、并硬拒绝来自其他会话的调用,才是这个能力可以被提供出来的前提。
反向通道本身此前已经没有客户端了:最初使用它的浏览器扩展改走了另一套机制,留下这条代码路径接好、测过、但没人用。这个 PR 给了它一个真实消费者。
审阅测试计划
如何验证
启动一个开启了反向通道的 daemon,用 Chrome 或 Edge 打开对着它的 Web Shell——需要 https 或 localhost;纯 http 的远端源不是安全上下文,浏览器不会授予本地文件访问权。
证据(前后对比)
改动前:会话没有任何途径触及浏览器一侧的文件系统;反向通道没有客户端,且只能做工作区级注册。
改动后:侧边栏入口、未连接弹层、显示文件夹名与四个工具的已连接弹层、浏览器不支持时的降级状态、以及中文已连接状态的截图。
已连接态的截图使用桩目录句柄,因为原生选择框无法在无头环境下驱动;其中对着真 daemon 的注册、以及弹层里显示的四个已发现工具是真的。上面描述的人工验收运行用的是真句柄,端到端。
UI 之外的验收证据:一次脚本化运行,对着真 daemon、真浏览器、真模型,把一个含随机 token 的文件放进某目录,让浏览器授权该目录,然后要求 agent 读取该文件并原样引用 token。这个 token 不在 prompt 里、也无法猜测,所以它出现在会话流中就证明字节走过了 磁盘 → 浏览器 → daemon → 模型 → 回来 这条路。该运行通过,daemon 日志独立佐证了会话 attach、prompt 被接受、回合完成。
本分支上的自动化覆盖:反向通道(含新的会话作用域)24 个 daemon 侧测试、桥与侧边栏共 568 个 Web Shell 测试,以及应用级测试套件。类型检查、lint、格式化均干净。一个无头端到端运行用真 daemon 和真 Web Shell 源码在真浏览器里驱动,断言连接会产生一次被 daemon 以"发现四个工具"确认的注册,且断开会将其拆除。
测试环境
环境(可选)
从源码启动的本地 daemon 并开启反向通道,Web Shell 由 dev server 提供(因此跑的就是 TypeScript 源码),人工验收用 Chrome,脚本化端到端用无头 Chromium。
风险与范围
关联 Issue
与 #5626 相关(本 PR 使用的反向工具通道由它引入),不关闭该 issue。