fix(serve): Allow approved same-host text reads outside workspace

[doudouOUC]

What this PR does

This PR keeps generic ACP text-read delegation enabled by default, while same-host daemon-owned bridges advertise readTextFile: false and writeTextFile: true. Their ACP children therefore use the regular CLI filesystem service for text reads, including direct read_file and shared text pre-reads, while final ACP text writes remain delegated through the workspace filesystem boundary.

It wires the capability choice into the default embedded, primary, static-secondary, and dynamic workspace runtimes; leaves caller-injected bridges under caller control; preserves the defensive workspace-filesystem read adapter; and documents the exact permission, audit, resource, and security boundaries.

Why it's needed

Before this change, qwen serve delegated text reads to its workspace-scoped client filesystem. A direct read_file for a host text file outside the registered workspace was therefore rejected by the workspace filesystem even after the normal CLI tool permission flow approved it, and the model could retry through shell instead. Same-host daemon reads should follow the same explicit permission rules as the regular CLI without weakening HTTP file routes or final ACP text writes.

Reviewer Test Plan

How to verify

1. Confirm a bridge with default options initializes ACP filesystem capabilities as { readTextFile: true, writeTextFile: true }, while each daemon-owned bridge initializes them as { readTextFile: false, writeTextFile: true }.
2. With a real daemon/ACP child and default approval mode, force the model to call read_file on a text file outside the workspace and all managed read roots. Selecting allow_once should place the sentinel content in the next model request and final response; selecting reject_once should never place the sentinel in subsequent model input or output.
3. Confirm the asymmetric filesystem service uses its local fallback for reads while continuing to call the ACP connection for writes.
4. Confirm HTTP reads outside the workspace and external, untrusted, or symlinked delegated writes are still rejected by the workspace filesystem.

Validation on a clean checkout of the latest main passed: npm ci completed the full build and bundle, root typecheck passed, 255 targeted capability/filesystem/wiring tests passed, target-file ESLint and Prettier passed, pre-commit checks passed, and git diff --check passed. The deterministic real daemon/ACP test also passed: allow_once exposed the sentinel to the next model request and final response, while reject_once emitted a failed tool update, completed the turn without another model request, and never exposed the sentinel. The global qwen baseline reproduced the approved-read failure before the fix.

Evidence (Before & After)

Before: a deterministic run against the globally installed qwen failed the approved-content assertion because the workspace filesystem rejected the external text read after approval; the observed product flow could then fall back to shell.

After: the updated bundle's deterministic real-daemon regression passes for both approval and rejection, capability and asymmetric-routing tests pass, and all four daemon construction paths advertise local reads. The PR is based directly on the latest main with one commit; none of the #7311 workspace-runtime stack commits are included.

Tested on

OS	Status
🍏 macOS	✅ tested
🪟 Windows	⚠️ not tested
🐧 Linux	⚠️ not tested

Environment (optional)

macOS, Node.js 26.0.0, clean main worktree, local same-host daemon/ACP child, and a fake OpenAI-compatible server.

Risk & Scope

• Main risk or tradeoff: The capability applies to every FileSystemService.readTextFile consumer, so write, edit, notebook, sed, and artifact pre-reads also move to the child-local CLI filesystem and no longer receive WFS's 256 KiB returned-output and full-snapshot cap, 8 MiB large-text scan cap, read audit, symlink rejection, or read-side TOCTOU protections. This is the explicit same-host, same-UID compatibility tradeoff; direct read_file still applies CLI permissions and core output limits.
• Not validated / out of scope: Windows and Linux were not tested locally, and the complete repository test suite was not run. HTTP capability changes, discovery tools, shell file operations, helper mkdir/file-history behavior, and an OS sandbox are out of scope.
• Breaking changes / migration notes: None. Generic ACP, IDE, remote, virtual-filesystem, and caller-injected bridge behavior keeps delegated text reads by default; only daemon-owned same-host runtime construction opts out.

Linked Issues

Refs #8618

中文说明

此 PR 做了什么

此 PR 保持通用 ACP 文本读取默认委托，同时让 daemon 自己创建的同机 bridge 宣告 readTextFile: false、writeTextFile: true。因此 ACP child 使用普通 CLI 文件系统服务执行文本读取，包括直接 read_file 和共享文本预读；ACP 最终文本写入仍通过 workspace 文件系统边界委托执行。

它将这个 capability 选择接入 default embedded、primary、static-secondary 和 dynamic workspace 四类 runtime；调用方注入的 bridge 仍由调用方控制；防御性的 workspace 文件系统读取 adapter 继续保留；同时文档明确记录权限、审计、资源和安全边界。

为什么需要

改动前，qwen serve 将文本读取委托给 workspace 范围内的 client 文件系统。因此，直接 read_file 读取注册 workspace 之外的主机文本文件时，即使普通 CLI 工具权限流程已经批准，workspace 文件系统仍会拒绝，模型随后可能改走 shell。同机 daemon 的读取应遵循普通 CLI 相同的显式权限规则，同时不能削弱 HTTP 文件路由或 ACP 最终文本写入的保护。

Reviewer 测试计划

如何验证

1. 确认使用默认选项的 bridge 将 ACP 文件系统 capability 初始化为 { readTextFile: true, writeTextFile: true }，而每个 daemon 自有 bridge 初始化为 { readTextFile: false, writeTextFile: true }。
2. 使用真实 daemon/ACP child 和默认审批模式，强制模型对 workspace 与所有托管读取根目录之外的文本文件调用 read_file。选择 allow_once 后，下一次模型请求和最终响应中应出现 sentinel 内容；选择 reject_once 后，后续模型输入或输出中都不应出现 sentinel。
3. 确认非对称文件系统服务的读取使用本地 fallback，而写入继续调用 ACP connection。
4. 确认 workspace 外的 HTTP 读取，以及外部、不受信任或经 symlink 的委托写入，仍被 workspace 文件系统拒绝。

在最新 main 的干净 checkout 上验证通过：npm ci 完成完整 build 和 bundle，根目录 typecheck 通过，255 个定向 capability/filesystem/wiring 测试通过，目标文件 ESLint 和 Prettier 通过，pre-commit 检查通过，git diff --check 通过。确定性的真实 daemon/ACP 测试也已通过：allow_once 将 sentinel 暴露给下一次模型请求和最终响应；reject_once 发出失败的工具更新，不再请求模型便完成 turn，并且始终不暴露 sentinel。修复前，全局 qwen 基线复现了批准读取仍失败的问题。

证据（改动前后）

改动前：针对全局安装 qwen 的确定性运行在批准内容断言处失败，因为 workspace 文件系统在批准之后仍拒绝外部文本读取；观察到的产品流程随后可能回退到 shell。

改动后：更新 bundle 的确定性真实 daemon 回归测试在批准和拒绝两条路径均通过，capability 与非对称路由测试通过，四个 daemon 构造路径都宣告本地读取。PR 直接基于最新 main 且仅有一个提交，不包含 #7311 workspace-runtime 堆栈中的任何提交。

已测试平台

操作系统	状态
🍏 macOS	✅ 已测试
🪟 Windows	⚠️ 未测试
🐧 Linux	⚠️ 未测试

环境（可选）

macOS、Node.js 26.0.0、干净的 main worktree、本地同机 daemon/ACP child，以及假 OpenAI 兼容服务器。

风险与范围

• 主要风险或取舍：该 capability 影响每个 FileSystemService.readTextFile 消费者，因此 write、edit、notebook、sed 和 artifact 的预读也迁移到 child 本地 CLI 文件系统，不再获得 WFS 的 256 KiB 返回输出与全量快照上限、8 MiB 大文本扫描上限、读取审计、symlink 拒绝或读侧 TOCTOU 保护。这是明确接受的同机、同 UID 兼容性取舍；直接 read_file 仍应用 CLI 权限和 core 输出限制。
• 未验证或范围外：未在本地测试 Windows 和 Linux，也未运行完整仓库测试套件。HTTP capability 变更、发现工具、shell 文件操作、辅助 mkdir/file-history 行为和 OS sandbox 均不在范围内。
• 破坏性变更或迁移说明：无。通用 ACP、IDE、远程、虚拟文件系统及调用方注入 bridge 的行为仍默认委托文本读取；只有 daemon 自己创建的同机 runtime 构造会关闭该委托。

关联 Issue

Refs #8618

[doudouOUC]

E2E test report

Baseline

• Ran the deterministic external-text-read scenario against the globally installed qwen before the fix.
• Result: the approved case failed its sentinel assertion because the external text content did not reach the next model request, reproducing the workspace-filesystem rejection that can lead to shell fallback.

Post-change validation

• Recreated the change in a clean worktree based directly on the latest origin/main; the final branch is 0 behind / 1 ahead, and none of the four feat(serve): manage skills per workspace runtime #7311 workspace-runtime stack commits are ancestors of the PR head.
• npm ci completed the full repository build and bundle successfully; root typecheck passed.
• Isolated ACP filesystem service suite: 31/31 passed, including local read fallback with delegated writes.
• ACP capability and daemon wiring tests: 224/224 passed for the default bridge behavior and same-host default, primary, static-secondary, and dynamic workspace construction paths; combined targeted unit/wiring coverage was 255 passing tests.
• ACP bridge package typecheck: passed.
• Target-file Prettier and ESLint: passed; repository pre-commit checks also passed.
• git diff --check: passed.
• The deterministic real daemon/ACP child regression passed: allow_once exposed the sentinel to the next model request and final response; reject_once emitted a failed tool update, completed without another model request, and never exposed the sentinel.

Remaining scope

Windows and Linux were not tested locally, and the complete repository test suite was not run. The integration runner emitted its existing non-blocking tsconfig warning that "//" substitutions should be an array.

[github-actions]

Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration.

中文

请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。

[qwen-code-ci-bot]

✅ Qwen Triage finished — view run. See the stage comments in this thread for the result.

✅ Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

[qwen-code-ci-bot]

Re-run at @wenshao's request on the current head — the gate previously ran against 63a6cc4a, and the PR has been through four review rounds and a maintainer real-daemon verification since.

• Template: complete ✓ — bilingual, reviewer test plan with before/after evidence.
• Problem: observed, not theoretical. Linked issue fix(serve): align same-host daemon text reads with CLI permissions #8618 with a concrete reproduction: under qwen serve, a read_file on a host text file outside the workspace fails with path escapes workspace after the permission flow approved it, and the model then falls back to shell. The maintainer independently reproduced this on the BEFORE arm of a real-daemon A/B.
• Direction: aligned. Same-host daemon-owned reads following the same permission flow as the regular CLI — instead of being rejected by the workspace filesystem boundary after approval — is the behavior a user would expect, and the capability split (readTextFile: false, writeTextFile: true) keeps the write boundary and the HTTP file routes untouched. Generic ACP / IDE / remote / caller-injected bridges keep delegated reads by default, so there is no behavior change outside same-host daemons.
• Size: 533+/32− across 17 files, but only 63 production-logic lines (6 files); 376 lines are tests and 126 are docs/README. Cross-package (acp-bridge + cli), so the Tier-2 confidence bar applies — full diff reviewed below.
• Approach: minimal — one BridgeOptions flag defaulting to true, flipped to false at the four daemon-owned construction sites, plus a clarifying comment on the now-unreachable delegated-read retry. No drive-by changes. The alternative I'd have considered (making the workspace filesystem honor approved external reads) was the worse path: it would push CLI permission semantics into the HTTP-facing boundary.
• Risk: Stage 1e matches acp-integration (high-risk path) — full review depth and CI evidence applied. One standing follow-up from the review history is tracked in the Stage 2 comment (the L4 lexical-allow gap the author promised to file as a separate issue).

Moving on to code review. 🔍

中文说明

应用 @wenshao 的要求在当前 head 上重跑 —— 门禁上次跑在 63a6cc4a，此后 PR 经历了四轮 review 和一次维护者真实 daemon 验证。

• 模板：完整 ✓ —— 双语，含 reviewer test plan 与 before/after 证据。
• 问题：已观测到，非理论性问题。关联 issue fix(serve): align same-host daemon text reads with CLI permissions #8618 有明确复现：qwen serve 下对 workspace 外宿主文本文件的 read_file 在权限流程批准之后仍报 path escapes workspace，模型随后退回 shell。维护者已在真实 daemon 的 BEFORE 分支独立复现。
• 方向：对齐。同机 daemon 自有的读取走与常规 CLI 相同的权限流程（而不是批准后仍被 workspace 文件系统边界拒绝）是用户预期的行为；capability 拆分（readTextFile: false、writeTextFile: true）保留了写边界与 HTTP 文件路由。通用 ACP / IDE / 远程 / 调用方注入的 bridge 仍默认委托读取，同机 daemon 之外无行为变化。
• 规模：17 个文件 533+/32−，但生产逻辑仅 63 行（6 个文件）；376 行为测试、126 行为文档/README。跨包（acp-bridge + cli），适用 Tier-2 置信度要求 —— 下方为完整 diff 审查。
• 方案：最小化 —— 一个默认 true 的 BridgeOptions 开关，在四处 daemon 自有构造点置为 false，另为已不可达的委托读取重试路径补了一段说明性注释。无顺手改动。我考虑过的替代方案（让 workspace 文件系统尊重已批准的外部读取）更差：那会把 CLI 权限语义推进面向 HTTP 的边界。
• 风险：Stage 1e 命中 acp-integration（高风险路径）—— 已按完整深度审查并要求 CI 证据。review 历史中遗留的一个跟进项（作者承诺单独开 issue 的 L4 词法 allow 缺口）记录在 Stage 2 评论中。

进入代码审查 🔍

— Qwen Code · qwen3.8-max

_{Reviewed at 297dea9fde67020c37d016aa8c1c3968de7ca475 · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Code review

My independent proposal for #8618 — advertise readTextFile: false on daemon-owned bridges so the child's reads go through its own CLI filesystem service (and its permission flow), while writes stay delegated through the workspace filesystem — is exactly what this PR does. The diff matches it: one option resolved once beside the other defaults in createAcpSessionBridge, four construction sites flipping it to false (primary, static-secondary, dynamic workspace, default embedded), and the capability surfaced in the ACP initialize frame. I traced the consumer side too: AcpFileSystemService.readTextFile returns the fallback (the child's regular FileSystemService) before any delegated call when the capability is off, so the localReadRoots retry in its catch is genuinely unreachable under serve — the new comment there is accurate.

No blocking findings. The review history did the heavy lifting already — @wenshao's real-daemon A/B on Linux verified all four reviewer-test-plan items (capability frames on the wire, allow/reject sentinel flow, zero fs/read_text_file frames on the AFTER arm, HTTP reads and delegated writes still fail-closed), and his mutation pass killed all four wiring mutants. The repaired integration test has teeth: it fails on the BEFORE build.

Three non-blocking notes:

1. Design doc overstates one mitigation. It says the daemon neutralizes QWEN_ACP_LOCAL_READ_ROOTS by setting it empty for the child — but production serve does not scrub that key (SCRUBBED_CHILD_ENV_KEYS holds only QWEN_SERVER_TOKEN, QWEN_CODE_SIMPLE, and the guard token; only the integration test empties it). Impact today is zero because the retry is unreachable with the capability off, and the doc's own next paragraph says so — but it is a false statement about a mitigation in the document that claims single ownership of this boundary. One line to fix.
2. The L4 follow-up issue was never filed. The author's 02:06 self-audit correctly identified that persisted directory-recursive allow rules match lexically (permission-manager.ts priority-3 loop takes the 'lexical' default), so a symlink planted in an always-allowed directory can be read silently — a pre-existing gap, but one whose last serve-side backstop (WFS symlink rejection) this PR removes. The fix is maintainer-gated core-permission work and rightly out of this PR; the promised standalone issue is not findable via search. File it so it doesn't evaporate.
3. The bot's earlier CHANGES_REQUESTED was grounded on the skipped macOS/Windows Test jobs; those are if: github.event_name == 'merge_group' by design (ci.yml, named jobs so the skipped state still satisfies required checks). That ground does not stand.

Test evidence (this PR's own CI at the reviewed commit)

The one red check is infra noise: ubuntu-latest / Java 11 (SDK Java workflow) died in actions/checkout on a self-hosted runner — EACCES: permission denied, rmdir '.../dist/bundled' while cleaning leftover root-owned files, before any PR code ran. The same workflow failed on main itself and on an unrelated PR within the same hour, and passes on others. Everything relevant to this change is green: unit suite on ubuntu, Serve A/B, and the real-daemon E2E job.

Check	Conclusion
Test (ubuntu-latest, Node 22.x)	✅ success
Serve A/B (ubuntu-latest, Node 22.x)	✅ success
Real daemon E2E / Java 11	✅ success
Desktop Shell (ubuntu-22.04)	✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x)	✅ success
precheck-pr / precheck	✅ success
Test (macos-latest, Node 22.x)	⏭️ skipped (merge-queue-only by design)
Test (windows-latest, Node 22.x)	⏭️ skipped (merge-queue-only by design)
Integration Tests (CLI, No Sandbox)	⏭️ skipped
ubuntu-latest / Java 11 (SDK Java)	❌ failure — runner checkout EACCES, pre-existing (fails on main too)

The central behavioural claim — approved external reads reach the model, rejected ones never do — is substantiated beyond the CI table: the maintainer's tapped-stdio real-daemon A/B on Linux, an integration test that fails on the reverted build, and a green Serve A/B diff. Sandboxed verification (/verify) for this exact head is running as part of this triage run; its report will post to this thread when it completes.

Not verified here: Windows behaviour (author and maintainer both tested macOS/Linux only) and the full repo suite — the targeted suites cover the change, but a red merge-queue run later would be the first broad signal.

中文说明

代码审查

我对 #8618 的独立方案——让 daemon 自有的 bridge 宣告 readTextFile: false，使 child 的读取走自己的 CLI 文件系统服务（及其权限流程），写入继续委托给 workspace 文件系统——与本 PR 完全一致。diff 与方案吻合：一个在 createAcpSessionBridge 中与其他默认值一并解析一次的开关，四处构造点（primary、static-secondary、dynamic workspace、default embedded）置为 false，capability 出现在 ACP initialize 帧中。消费侧也已追验：capability 关闭时 AcpFileSystemService.readTextFile 在任何委托调用之前就返回 fallback（child 的常规 FileSystemService），catch 中的 localReadRoots 重试在 serve 下确实不可达——新增注释表述准确。

无阻塞项。审查历史已完成大部分重活：@wenshao 在 Linux 上的真实 daemon A/B 验证了 reviewer test plan 全部四项，变异测试杀死了全部 4 个接线变异体；修复后的集成测试有牙齿（在 BEFORE 构建上失败）。

三条非阻塞意见：

1. 设计文档对一项缓解措施表述过头。 文档称 daemon 会为 child 清空 QWEN_ACP_LOCAL_READ_ROOTS——但生产 serve 并不清除该 key（SCRUBBED_CHILD_ENV_KEYS 仅含 QWEN_SERVER_TOKEN、QWEN_CODE_SIMPLE 与 guard token；只有集成测试清空了它）。当下影响为零（capability 关闭时重试不可达，文档下一段自己也这么说），但在这份自称唯一持有边界描述的文档里，这是一句关于缓解措施的错误陈述，一行即可修正。
2. L4 跟进 issue 尚未创建。 作者 02:06 的自我审计正确指出：持久化的目录递归 allow 规则按词法匹配（permission-manager.ts 第三优先级循环走 'lexical' 默认），植入 always-allow 目录下的 symlink 可被静默读取——这是既有缺口，但本 PR 移除了它在 serve 侧的最后兜底（WFS symlink 拒绝）。修复属于维护者保留的核心权限改动，不在本 PR 范围内；但承诺单独创建的 issue 搜索不到，请补建以免遗失。
3. bot 早前的 CHANGES_REQUESTED 以 macOS/Windows Test job 被跳过为由；这些 job 按设计带 if: github.event_name == 'merge_group'（ci.yml，具名 job，跳过状态同样满足 required checks）。该理由不成立。

测试证据（本 PR 自身 CI，审查提交点）

唯一红色 check 为基础设施噪声：ubuntu-latest / Java 11（SDK Java workflow）在自托管 runner 的 actions/checkout 阶段清理遗留 root 文件时报 EACCES 而死，未执行任何 PR 代码；同一 workflow 同一小时内在 main 本身和另一无关 PR 上也失败，其余运行通过。与本改动相关的全部绿色：ubuntu 单测、Serve A/B、真实 daemon E2E。

核心行为主张（批准的外部读取可达模型、拒绝的永不可达）由 CI 表之外的证据支撑：维护者在 Linux 上抓取 stdio 的真实 daemon A/B、在回退构建上失败的集成测试、绿色的 Serve A/B diff。针对本 head 的沙箱验证（/verify）正在本次 triage 运行中进行，完成后报告会发到本 thread。

此处未验证：Windows 行为（作者与维护者均只测了 macOS/Linux）、仓库全量测试套件——定向套件已覆盖改动，merge queue 的宽泛跑是首个更广信号。

— Qwen Code · qwen3.8-max

_{Reviewed at 297dea9fde67020c37d016aa8c1c3968de7ca475 · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Confidence: 4/5 — a minimal fix for an observed bug, verified more thoroughly than almost any PR this gate sees; the deduction is two open follow-ups, not doubt about the code.

Stepping back: this PR started as a 63-line production change and survived four review rounds, an adversarial author self-audit, and two maintainer real-daemon A/B runs — and what came out the other end is still a 63-line production change. The scope never ballooned: writes stay delegated, the HTTP routes stay workspace-scoped, generic ACP keeps delegated reads, and the tradeoffs this change does accept (WFS read-side caps, symlink rejection, audit on the read path) are owned in one design doc rather than scattered across comments. The self-audit's blocking items were either resolved by evidence (the symlink-escape concern — @wenshao forced it on a real daemon and the prompt fires, with #8636's canonicalization closing the layer the author worried about) or correctly ruled out of scope (the L4 lexical-allow semantics change is maintainer-gated core work, and the write/edit family reproducing #8618 later in the sequence is a documented, separate concession).

My two reservations are the non-blocking ones from Stage 2: the design doc's inaccurate env-scrub sentence, and the L4 follow-up issue that was promised but never filed. Neither touches the shipped behaviour, and on a re-run neither would justify holding the PR — but the issue should be filed before this merges so the exposure this PR documents doesn't quietly lose its tracking.

On the verdict itself: the problem was real and reproduced, the fix does what it says on both wire and behaviour, the tests pin the wiring in all four sites and fail without the change, and the maintainer who knows this code best approved this exact commit after verifying it on the platform the PR hadn't tested. Approving.

中文说明

置信度：4/5 —— 对一个已观测 bug 的最小修复，验证深度超过本门禁见过的绝大多数 PR；扣分来自两个未闭环的跟进项，而非对代码的怀疑。

退一步看：这个 PR 以 63 行生产改动开始，经历四轮 review、一次作者对抗性自我审计、两次维护者真实 daemon A/B —— 走出来时仍然是 63 行生产改动。范围从未膨胀：写入保持委托、HTTP 路由保持 workspace 作用域、通用 ACP 保持委托读取，而本改动接受的取舍（WFS 读取侧上限、symlink 拒绝、读取审计）由一份设计文档统一持有。自我审计的阻塞项要么被证据解决（symlink 逃逸担忧 —— @wenshao 在真实 daemon 上强制触发，确认提示会出现，#8636 的规范化也闭合了作者担心的层面），要么被正确判定为超出范围（L4 词法 allow 语义变更属于维护者保留的核心工作；write/edit 家族在序列后段复现 #8618 是已记录的另一让步）。

我的两点保留是 Stage 2 的非阻塞项：设计文档中关于 env 清除的不准确表述，以及承诺创建但尚未创建的 L4 跟进 issue。两者都不影响已交付的行为，re-run 规则下也不构成扣住 PR 的理由 —— 但合并前应当补建该 issue，以免本 PR 文档化的暴露面悄悄失去跟踪。

结论本身：问题真实且有复现，修复在线缆与行为两个层面都做到了它声称的事，测试钉住了全部四处接线且去掉改动即失败，最了解这块代码的维护者在验证了他此前未测的平台后批准了同一提交。批准。

— Qwen Code · qwen3.8-max

_{Reviewed at 297dea9fde67020c37d016aa8c1c3968de7ca475 · re-run with @qwen-code /triage}

[wenshao]

Code review

Reviewed at 63a6cc4a. The design docs are unusually candid about the tradeoffs being accepted, which made this easy to review — most of what follows is either a gap the docs don't cover, or a consequence that is daemon-specific rather than a straight "same as the CLI".

Critical

1. A symlink under an always-allow read root now returns arbitrary host files with no confirmation at all. getFileReadDefaultPermission (packages/core/src/tools/file-read-permission.ts:18) resolves with path.resolve, not realpath, and then tests isSubpaths(allowedRoots, filePath) against config.storage.getProjectTempDir(), <projectDir>/subagents, Storage.getGlobalTempDir(), config.storage.getUserSkillsDirs(), Storage.getUserExtensionsDir(), and config.getPlansDir(). So an agent (or a repo-supplied skill) that creates <projectTempDir>/x → ~/.aws/credentials and then calls read_file <projectTempDir>/x gets allow — no prompt — and after this PR the read is served locally and follows the symlink. Before this PR the same call was delegated, WorkspaceFileSystem.resolve(path, 'read') canonicalized it and returned symlink_escape, and the local fallback in AcpFileSystemService only accepted a path whose realpath was under localReadRoots (packages/cli/src/acp-integration/service/filesystem.ts:249-260) — so it failed closed. The compatibility claim in docs/design/daemon-local-text-reads.md ("an unexpected or capability-violating delegated read still reaches the workspace boundary and fails closed for external paths") does not cover this, because nothing is delegated on this path any more, and the behavior claim in ## Behavior ("their default is ask") is false for the allow-listed roots. The underlying lexical-resolve gap lives in core and predates the PR — it just wasn't reachable in the daemon until now. Suggested fix: realpath the requested path in getFileReadDefaultPermission before isSubpaths, matching what workspaceContext.isPathWithinWorkspace already does. That closes it for the plain CLI too.

Major

2. The read/write asymmetry means #8618's actual failure mode still happens for write_file / replace / notebook_edit — just later and more expensively. Reads become local, writes stay delegated. So: read_file /home/u/notes.txt → prompt → approve → succeeds (new behavior, good). Then write_file on the same path: the pre-read succeeds locally, the diff renders, the user approves, and write-file.ts:509 / edit.ts:611,619 call getFileSystemService().writeTextFile → AcpFileSystemService (write capability still true) → bridge → createBridgeFileSystemAdapter.writeText → wfs.resolve(params.path, 'write') → path_outside_workspace. The tool fails after approval, and the model falls back to run_shell_command — the same pathology the PR set out to remove, relocated from step 1 to step 3 with a wasted diff and confirmation round trip in front of it. 07-workspace-filesystem.md states writes stay delegated, but nowhere does the PR acknowledge that the reported bug therefore still reproduces for the edit/write family. Either the docs should say so plainly, or the write side needs the same treatment.

3. External file content reaches every SSE subscriber before anyone approves anything. getFileReadDefaultPermission is wired into exactly two call sites — read-file.ts:124 and zoom-image.ts:67. The shared pre-reads have no read-permission gate: EditToolInvocation.getConfirmationDetails (edit.ts:408) calls calculateEdit (edit.ts:146), which reads the file at edit.ts:199 while building the confirmation payload. The daemon fans that payload — diff and all, now containing the contents of an out-of-workspace file — out to every attached SSE client before the approval decision exists. The docs correctly note the pre-reads lose WFS's limits/audit/symlink checks, but frame the whole slice as "regular CLI read behavior"; in the interactive CLI the pre-approval diff is seen by the one person at the terminal, whereas in the daemon it's seen by every subscriber. edit.ts:848's own comment confirms out-of-workspace writes are a supported, confirmation-gated flow, so this is reachable by design, not a corner case.

Suggestions

4. Unbounded pre-reads can now OOM the ACP child. write-file.ts:203/341/874, edit.ts:199/871/886, and notebook-edit.ts:489/918 call readTextFile({ path }) with no maxOutputBytes (contrast artifact/artifact-tool.ts:143-146, which passes MAX_ARTIFACT_BYTES). readFileWithLineAndLimit (fileUtils.ts:331) takes the windowed readTextRange branch only when a line, a finite limit, or maxOutputBytes is present — otherwise it falls through to readFileWithEncodingInfo and buffers the whole file. Previously WFS refused those windowless reads past its 256 KiB full-snapshot cap. Now a notebook_edit against a multi-GB .ipynb crashes the child, the daemon publishes session_died, and every attached client loses the session. A resource limit that used to fail closed with a typed error now fails as a process death. Passing a cap at these call sites would keep the PR's intent without that edge.

5. No opt-out. delegateReadTextFileToClient: false is hardcoded at all four construction sites (server.ts:978, run-qwen-serve.ts:3894/4294/4819) with no setting, flag, or env override. --hostname 0.0.0.0 --token … is an explicitly documented topology (docs/users/qwen-serve.md:324,386), and for those operators the new statement "treat authenticated daemon clients as the same security principal" is a genuine widening of the trust model, not a restatement of it — the PR's justification ("same machine, one UID") is a property of the child, while the party gaining reach is the client. Worth noting that GET /file still returns 403 for the same path, so the HTTP and ACP halves of one daemon now disagree about the boundary. A settings key to restore delegation looks cheap and would make this a policy choice rather than a fait accompli.

6. adapter.readText has no test and no longer has a caller. The new header comment and both docs lean on "the read path remains a fail-closed boundary for unexpected or capability-violating delegated reads" as the compatibility argument, but after the flip no production path invokes it, so a future refactor that breaks the wfs.resolve(path, 'read') call inside it fails nothing. A direct unit test on createBridgeFileSystemAdapter(...).readText with an external path would anchor the claim.

Nits

• integration-tests/cli/qwen-serve-streaming.test.ts:96-113: findExternalReadBase() tries homedir() first, so beforeAll does mkdtempSync(path.join(realHome, '.qwen-serve-external-read-')). Cleanup lives only in afterAll, so any Ctrl-C, --bail, or CI timeout leaves a hidden directory in the developer's actual home, and each interrupted run adds another. The suite already builds a sandbox homeDir under tmpdir(); a sibling temp root deliberately excluded from the workspace and from the /tmp local-read root would avoid writing into $HOME.
• qwen-serve-streaming.test.ts:656: expect(serializedEvents).toContain('was canceled by the user.') pins a security assertion to a localized, user-facing product string. The two assertions around it (tool_call_update with status === 'failed', and not.toContain(sentinel)) already carry the whole meaning; a copy change or a non-English locale would fail this for reasons unrelated to the capability under test.
• packages/cli/src/acp-integration/service/filesystem.test.ts:969: the case is now titled keeps writes delegated when readTextFile capability is disabled, but its first half is still the original read-fallback assertion — the one that actually protects this PR's core behavior. Someone deleting "the write test" later would silently drop read coverage. Also expect(writeResult).toEqual({ _meta: undefined }) is close to a no-op, since toEqual ignores undefined-valued properties and it passes for {}. Two it blocks, and a defined _meta round trip on the write side.
• docs/users/qwen-serve.md:557: the rewrite drops "Environment isolation between runtimes is not an operating-system security boundary" without restating it anywhere — that sentence was load-bearing for the multi-workspace threat model. The new material is also filed under a bullet whose heading is still about the child's environment, so an operator auditing the security section for filesystem scope is likely to skip past it. It deserves its own bullet.
• docs/design/serve-large-text-range-consistency.md:77 still lists "the injected ACP readTextFile adapter" among the consumers of the large-text-range boundary, and that document's verification section promises concurrent-append / truncation / symlink-replacement rejection for those consumers — guarantees no production ACP read has any more.
• The same ~40-word tradeoff enumeration (256 KiB caps, 8 MiB scan cap, read audit, symlink rejection, read-side TOCTOU) is now duplicated verbatim in docs/design/daemon-local-text-reads.md, docs/developers/daemon/07-workspace-filesystem.md, docs/users/qwen-serve.md, and condensed in packages/acp-bridge/README.md. When one of those limits is tuned, three copies go stale. The design doc should own it and the rest should link, which is the cross-reference pattern the daemon docs already use.

[doudouOUC]

Review notes (at 63a6cc4)

Reviewed the approach and the full diff, and independently validated on a clean checkout of the PR head. Summary: the approach is correct, the implementation is small and focused (~10 production logic lines plus tests/docs), and I found no blocking issues. Two non-blocking suggestions below.

Approach verification

• The capability flip reuses the existing child-side branch: AcpFileSystemService.readTextFile already falls back to the regular CLI filesystem service when capabilities.readTextFile is false, so no new protocol surface is introduced. The fallback path forwards the full CoreReadTextFileRequest (stats, _meta, limit, maxOutputBytes, signal) verbatim, which the updated unit test now pins.
• Permission semantics line up with the regular CLI: getFileReadDefaultPermission returns ask for paths outside the workspace and the managed read roots, so approved external reads return content and rejected ones never do.
• Downstream consumer sweep: createAcpSessionBridge has exactly 4 non-test production call sites (default embedded in server.ts, primary / static-secondary / dynamic in run-qwen-serve.ts), and all 4 set delegateReadTextFileToClient: false with per-site test assertions. The option is declared, read at the single initialize call site (child restart/respawn shares it), and set by all daemon-owned constructors — not a dead switch. Caller-injected deps.bridge and generic ACP/IDE consumers (e.g. vscode-ide-companion) keep delegated reads.
• Defense in depth holds: BridgeClient.readTextFile still routes through the injected WFS adapter first, so a capability-violating delegated read from a misbehaving child still fails closed at the workspace boundary.
• The stated tradeoff scope is accurate: the shared FileSystemService.readTextFile consumers in core are exactly the write/edit/notebook/shell(sed)/artifact/fileUtils pre-reads, and the WFS caps quoted in the docs (256 KiB / 8 MiB) match serve/fs/policy.ts.

Independent validation

On a clean checkout of 63a6cc4 with a fresh npm ci: the 3 targeted test files pass (2 + 33 + 220 = 255 tests, matching the PR description), root npm run typecheck passes, and ESLint on all 8 changed source/test files passes. Note the full CI test matrix has not run yet on this PR — only label/precheck jobs so far.

Suggestions (non-blocking)

1. Integration fixture lives in the real $HOME. findExternalReadBase prefers homedir() of the test process, so a hard-killed suite (signal/OOM skips afterAll) can leave .qwen-serve-external-read-* directories in the developer's real home. mkdtempSync is 0700 and the content is harmless, so the risk is low, but preferring /var/tmp first would keep stray fixtures out of $HOME. The exclusion of /tmp and the repo root is correct and worth keeping (POSIX ACP local-read roots include /tmp, and on Linux tmpdir() is /tmp, which would overlap the fake-HOME allowed roots and break the ask determinism).
2. reject_once asserts exactly one model request. This pins the current cancel semantics where a rejected tool call ends the turn without feeding the rejection back to the model. If that behavior ever changes to a model-visible rejection round, this assertion fails even though the security property (sentinel never exposed) still holds — the sentinel assertions are the load-bearing ones. Fine as-is; just flagging the coupling.

[yiliang114]

LGTM, no blockers. The relaxation is deliberate and gated: production qwen serve sets delegateReadTextFileToClient=false so text reads use the child's regular CLI filesystem service and the normal permission flow (default ask — approval required, rejection blocks), while final writes stay delegated through WorkspaceFileSystem and HTTP routes stay workspace-scoped. The adapter keeps a fail-closed read path for unexpected/capability-violating delegated reads. The documented tradeoff (child-local reads give up WFS read-side symlink/TOCTOU/audit protections in exchange for approved same-host reads, under the one-UID/not-an-OS-sandbox daemon model) is honest and acceptable. Tests cover the capability wiring and fail-closed reads. Non-blocking: keep the design doc's 'accepts CLI read-side limits' caveat visible in user docs so operators know approved outside-workspace reads are less hardened than WFS reads.

[qwen-code-ci-bot]

Reviewed. Suggestions are inline. Not reviewed: CI verification — no "Qwen Code CI" run exists at this SHA (fork PR awaiting CI approval); the ci.yml test job carrying the required no-AK integration gate that collects the PR's new integration test (integration-tests/cli/qwen-serve-streaming.test.ts) never executed in CI. The suite was run locally during this review and passed (real daemon + real ACP child, 5.2s)..

中文说明

已审查。 建议见行内评论。 未审查：CI verification — no "Qwen Code CI" run exists at this SHA (fork PR awaiting CI approval); the ci.yml test job carrying the required no-AK integration gate that collects the PR's new integration test (integration-tests/cli/qwen-serve-streaming.test.ts) never executed in CI. The suite was run locally during this review and passed (real daemon + real ACP child, 5.2s).。

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

[doudouOUC]

@qwen-code /triage

[qwen-code-ci-bot]

Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 1363 passed · 0 failed · 1363 total

中文 — 判定：✅ 通过 · 可合入（agent 判定）

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

脚本断言：1363 通过 · 0 失败 · 1363 总计

Verification report

PR 8620 Deep Verification — fix(serve): Allow approved same-host text reads outside workspace

Verdict: merge-ready — 1363/1363 scripted assertions passed (fail: 0).
Verified head: 63a6cc4a86775f8a4121f54c418d4e59cb274943 (HEAD^2, single commit, matches the metadata snapshot). Base: 8fd0162c6821138ce7fab6472fbae06ca4717a7c (HEAD^1).

中文摘要

• 结论: merge-ready。全部 1363 条脚本化断言通过，无失败；未发现新缺陷。
• A/B 结论: 中心改动被证明是 load-bearing。在 base（8fd0162c6）上，权限流程批准（allow_once）之后，workspace 之外文本文件的 sentinel 内容仍然无法进入模型请求（新增集成测试在 base 上 3/3 次失败于"批准内容断言"）；在 head（63a6cc4a8）上同一场景通过——批准内容进入下一次模型请求与最终响应，拒绝（reject_once）则完全不暴露内容。见 01-ab-base-cell-approved-read-fails.png 与 02-ab-head-cell-approved-read-passes.png。
• 变异矩阵: 3 个关键 hunk 变异全部被新增/扩展测试以预期断言杀死（03-mutation-matrix-all-killed.png），未变异对照为绿色——测试非空泛。
• 边界核查: 写入仍经 WorkspaceFileSystem（适配器 21 项测试锁定外部/符号链接/信任门拒绝）；意外委托读在 WFS 边界 fail-closed；默认与调用方注入 bridge 仍为 readTextFile: true；4/4 daemon-owned 构造点全部接入；HTTP 文件路由零改动。PR 声明的读侧保护取舍（审计、symlink 拒绝、TOCTOU、大小上限）与设计文档一致，未发现未披露的同机制兄弟面。
• 未覆盖范围: Windows；仓库全量测试套件；docker/podman sandbox 模式；真实 stdio 上 initialize 帧的逐字节抓包（以内存通道 wire-payload 断言 + 端到端行为 A/B 双机制覆盖）；base 侧 reject_once 子场景（allow_once 先行失败，符合预期）。本轮在 Linux 容器中完成端到端验证，补上了 PR 描述中"Linux 未测试"的空缺。

Central claim + A/B

Central claim: under qwen serve, a read_file of a host text file outside the registered workspace — previously rejected by the workspace filesystem even after the CLI permission flow approved it — now succeeds, because daemon-owned bridges advertise clientCapabilities.fs = { readTextFile: false, writeTextFile: true } at ACP initialize, moving text reads to the child's regular CLI filesystem service while final ACP writes stay delegated.

Mechanism traced end to end: parent advertises the capability in the per-channel initialize (packages/acp-bridge/src/bridge.ts:2544-2548, driven by BridgeOptions.delegateReadTextFileToClient ?? true); the child's AcpFileSystemService.readTextFile short-circuits to the local CLI filesystem service when the capability is off, while writeTextFile still delegates to the parent (packages/cli/src/acp-integration/service/filesystem.ts:144-146, production code untouched by this PR); all four daemon-owned construction sites (server.ts:978, run-qwen-serve.ts:3894/4294/4819) set the option to false.

The A/B ran the PR's own deterministic end-to-end scenario (real daemon + real qwen --acp child + fake OpenAI server; sentinel file in a writable root outside the workspace and /tmp):

Cell	Environment	Oracle	Result
base	worktree at 8fd0162c6, rebuilt + bundled; purity-checked (delegateReadTextFileToClient absent from all base artifacts); PR's test file copied in verbatim	approved-content assertion: sentinel reaches a later model request + final response	FAIL 3/3 attempts — expected false to be true at qwen-serve-streaming.test.ts:635; permission request was observed and approved, content never delivered
head	pre-verified CI build at ea7c8fc4b (dist/cli.js bundle)	same	PASS (allow_once: ≥2 model requests, sentinel observed; reject_once: exactly 1 request, no sentinel, failed tool update + "was canceled by the user.")

Witnesses: 01-ab-base-cell-approved-read-fails.png, 02-ab-head-cell-approved-read-passes.png; raw logs logs/ab-base-cell.log, logs/ab-head-cell.log. The base failure is the intended control outcome (counted as a passed expectation); it fails on the exact behavioral assertion, not a harness error, and the permission flow itself demonstrably worked on base (request observed, vote accepted) — so the failure is specific to the external read, not a broken base harness.

Control hygiene (per methodology): the base worktree's node_modules/@qwen-code/* links were re-pointed into the base tree and realpath-asserted (readlink -f → /…/tmp/base-tree/packages/acp-bridge, /…/tmp/base-tree/packages/core); nested per-package node_modules and @lydell were mirrored from head (lockfile unchanged by the PR); grep -r delegateReadTextFileToClient over base dist/ + package dist/ returns 0 hits while head's bundle chunks contain it.

Mutation matrix (scratch worktree at head; unmutated control green): see 03-mutation-matrix-all-killed.png, logs/mutation-*.log, rerunnable via mutation-matrix.sh.

Mutant	Suite	Result	Failure mode
A: bridge.ts capability hunk reverted to readTextFile: true	bridge-file-capabilities.test.ts	killed	expected { readTextFile: true } to deeply equal { readTextFile: false }; the default-path test stayed green (mutation touched only the opt-out — correct attribution)
B: 3× delegateReadTextFileToClient: false deleted from run-qwen-serve.ts	run-qwen-serve.test.ts ("advertises workspaces", "dynamic workspace runtime")	killed (both)	expected {…} to match object { delegateReadTextFileToClient: false } at lines 1214 / 1447
C: wiring line deleted from server.ts	server-default-bridge-wiring.test.ts	killed	same shape at line 85

The integration test's own non-vacuity is proven by the A/B itself: it flips from red on base to green on head.

Corrections

None (first round; no prior review claims to correct).

Findings

None. Notes from boundary checks, so the absences are measured rather than assumed:

• Accepted tradeoff boundary checked. The description names what reads lose (WFS 256 KiB output cap, 8 MiB scan cap, read audit, symlink rejection, read-side TOCTOU). Swept the siblings of the same mechanism: FileSystemService exposes only readTextFile / writeTextFile / findFiles (+ optional handle read) — findFiles was already local, image/binary reads are not on this interface, so the tradeoff surface is exactly as documented. Direct read_file retains CLI permission + core output limits (the A/B's reject_once arm exercises the permission path).
• Write side unchanged and pinned. AcpFileSystemService.writeTextFile still delegates with readTextFile disabled (extended unit test asserts the write hits the ACP connection, not the fallback); the adapter suite pins path_outside_workspace for writes and reads, symlink_escape, and trust-gate rejection (21/21). BridgeClient.readTextFile routes unconditionally through the injected WFS adapter when present, so a capability-violating delegated read still fails closed at the workspace boundary — verified against bridgeClient.ts:2600-2614, not just the doc comment.
• No missed construction sites. Exactly four production createAcpSessionBridge call sites exist (grep-verified); all four set the option. createServeApp builds its internal bridge only when deps.bridge / an injected registry bridge is absent (server.ts:952-955), so caller-injected bridges keep caller-controlled capabilities; the generic default stays readTextFile: true (unit-pinned). Capability is re-advertised from the same opts on every channel start, so a child respawn cannot lose it.
• HTTP fs routes: zero diff under packages/cli/src/serve/fs/ — HTTP reads remain workspace-scoped.
• No dead switch. delegateReadTextFileToClient has exactly one read site (bridge.ts) and four production set sites.

Not covered

• Windows (PR marks it untested; this round ran on Linux, which the PR also listed as untested — now covered end to end here).
• Repository-wide test suite and repo-wide typecheck as standalone gates — targeted suites were run instead; the pre-verification CI npm run build at HEAD completed the full typechecked build.
• Sandbox modes (QWEN_SANDBOX=docker/podman); the suite ran with QWEN_SANDBOX=false.
• Byte-level capture of the initialize JSON-RPC frame on real stdio. The wire payload is asserted through the real bridge code via the in-memory channel (initializeCalls[0].clientCapabilities.fs), and the end-to-end A/B proves the child behaved as readTextFile: false over a real stdio ACP connection — two independent mechanisms, but no raw frame dump.
• Base-side reject_once sub-scenario: unreachable there because allow_once fails first; rejection was already safe on base (reads were always denied), so no behavior is unmeasured.
• Per-commit attribution is trivially complete: single commit, HEAD^2 equals the metadata's only commit OID.

Methodology

Environment: CI verify container (node:22-bookworm, Node v22.23.2), working tree = refs/pull/8620/merge at depth 2, npm ci + npm run build completed pre-verification. A/B: base worktree at HEAD^1 rebuilt (npm run build + npm run bundle) with internal workspace links re-pointed into the base tree (realpath-asserted) and purity-checked; the PR's self-contained integration test was copied verbatim into the base tree (helpers byte-identical on both sides) and run through the base bundle; the identical command ran against the head bundle. Two base-build failures along the way were environmental, not PR-caused: fresh worktrees lack npm's nested per-package node_modules, and packages/core/tsconfig.json maps @lydell/node-pty types through ../../node_modules/…, which must exist inside the tree — both were mirrored from head and the forced-rebuild A/A control at HEAD passed clean before trusting either side. Mutation runs used a second scratch worktree at HEAD with head dist/ mirrored (mutations were source-only; vite transforms src directly). Evidence images were rasterised with scripts/verify-capture.mjs; raw per-cell logs live in logs/; harness scripts in ab-harness.sh / mutation-matrix.sh.

Evidence images

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

— Qwen Code · sandboxed verification

[qwen-code-ci-bot]

Triage re-run completed without a new review.

⚠️ The bot's only review on 63a6cc4a86775f8a4121f54c418d4e59cb274943 is a COMMENTED one, which carries no vote — so it has no verdict of its own on this commit, and main needs two approving reviews: an approval left by another account is a separate vote and does not count as the bot's own. Two different things look like this, and the stage-3 comment above says which: the triage skill deferring on purpose at 3/5 — a fork refactor hitting the approval guardrail, or a core change escalated for maintainer awareness, both normal outcomes — or an earlier approval that a push dismissed, leaving only the comment behind, which needs a fresh review.

⚠️ 机器人在 63a6cc4a86775f8a4121f54c418d4e59cb274943 上唯一的评审是 COMMENTED，不带票 —— 因此它在该 commit 上没有自己的裁决，而 main 需要两个批准（其他账号的批准是另一张票）。有两种情况长这样，上方的 stage-3 评论会说明是哪一种：triage skill 在 3/5 时有意 defer（fork refactor 命中审批护栏，或核心改动被升级交由维护者把关，两者都是正常结果）；或者更早的批准被一次推送作废、只剩下这条评论，此时需要重新评审。

The stage comments above were updated with the latest result. View workflow run.

上方各阶段评论已更新为最新结果。查看工作流运行。

[samuelhsin]

Code review (at 63a6cc4, locally verified)

I checked the PR head out into a clean worktree and verified everything below at this exact commit on macOS.

Local verification evidence

• The 255 targeted tests claimed in the PR description, re-run one by one and passing: bridge-file-capabilities 2/2, AcpFileSystemService 31/31, server-default-bridge-wiring 2/2, run-qwen-serve 220/220.
• Full acp-bridge suite 1069/1069 passed (including bridge.test.ts's 479 tests — no regression on the default-delegation path); root npm run typecheck clean across all packages; Prettier + ESLint clean on every changed file.
• Built the bundle and ran the new integration test same-host external text reads against a real daemon + fake model: allow_once surfaces the sentinel into the next model request and final response; reject_once never surfaces it. Both directions pass.
• Wiring verified at this commit: the new option's only read site is the single initialize call in bridge.ts; all four daemon-owned construction sites (server.ts embedded default bridge; primary, static-secondary, and dynamic workspace in run-qwen-serve.ts) set it to false; generic ACP / IDE companion / caller-injected bridges keep the default true; the WFS adapter retains its readText implementation; HTTP file routes untouched.
• Branch hygiene: one commit directly on the latest main at creation time; main has not touched any of the affected files since the PR base, so no conflict.

The core mechanism is sound, the diff is small, and test coverage is good. However, I confirm the Critical in @wenshao's review is real (independently verified in code), plus several Majors — recommendation at the end.

Critical (independently verified)

A symlink under an always-allow read root yields a no-confirmation read of arbitrary host files. getFileReadDefaultPermission (packages/core/src/tools/file-read-permission.ts:18) resolves with path.resolve (lexical, no realpath), then tests isSubpaths against projectTempDir, <projectDir>/subagents, the global temp dir, user skills dirs, the extensions dir, and the plans dir. So <projectTempDir>/x → ~/.aws/credentials gets default allow with no prompt, and after this PR the child serves that read locally and follows the symlink. Before this PR the daemon path was shielded: the delegated read hit WFS (symlink_escape), and AcpFileSystemService's local fallback only accepted paths whose realpath was inside the local read roots. This PR removes that shield. The lexical gap already exists for the plain CLI today; this PR extends the exposure to daemon sessions. Suggested fix: realpath the requested path before isSubpaths inside getFileReadDefaultPermission — one change closes it for both CLI and daemon. (Note the fix lands in core tools, which is maintainer territory per AGENTS.md.)

Majors (verified)

1. The write family still fails after approval. Reads become local while writes stay delegated, so write_file / replace / notebook_edit on an external path: pre-read succeeds locally → diff renders → user approves → writeTextFile → WFS path_outside_workspace → the tool fails after approval and the model falls back to shell — the same pathology #8618 set out to remove, relocated from step 1 to step 3 with a wasted confirmation round trip. The docs should at least acknowledge this plainly, or the write side needs the same treatment.
2. Pre-read content is broadcast before anyone approves. For edits, buildPermissionRequestContent (packages/cli/src/acp-integration/session/permissionUtils.ts:129-136) puts the full oldText into the permission request as a diff content block, which the mediator fans out to every attached SSE client before any decision exists. Pre-PR, WFS refused the external pre-read, so this payload never materialized. The PR's own "reject ⇒ content never surfaces" guarantee holds for read_file but not for edit-family pre-reads.
3. Windowless pre-reads lose WFS's caps. write-file.ts:203/341/874, edit.ts:199/871/886, and notebook-edit.ts:489/918 call readTextFile({ path }) with no maxOutputBytes; readFileWithLineAndLimit (packages/core/src/utils/fileUtils.ts:341-360) falls through to full-file buffering when no line/limit/maxOutputBytes is present. Previously WFS's 256 KiB full-snapshot cap failed these closed with a typed error; now a multi-GB target can OOM the child → session_died for every attached client. Passing a cap at these call sites would keep the PR's intent without the edge.
4. No opt-out, and a topology concern. false is hardcoded at all four sites with no setting/env override. For the explicitly documented --hostname 0.0.0.0 --token topology, authenticated clients gain host-read reach (subject to approval), and GET /file still returns 403 for the very same path — the HTTP and ACP halves of one daemon now disagree about the boundary. A settings key to restore delegation would make this a policy choice; either way it deserves explicit maintainer sign-off.

One rebuttal on the existing review

@wenshao's point #6 ("adapter.readText has no test") does not hold at this commit: packages/cli/src/serve/bridge-file-system-adapter.test.ts has a full readText describe covering the happy path, windowing, size caps, symlink_escape, and external path_outside_workspace rejection (see the boundary enforcement block). The "no production caller anymore" half is accurate, but that is the documented, deliberate defensive posture.

Suggestions / nits

• isPathAtOrWithin in the integration test duplicates the already-exported isPathWithinRoot from core (packages/core/src/utils/workspaceContext.ts, re-exported from the package index) — semantics identical today, argument order swapped. Endorse @qwen-code-ci-bot's suggestion to reuse it; this helper is the fixture-placement guard of a security-relevant test.
• findExternalReadBase() places the fixture dir under the real $HOME, and cleanup lives only in afterAll — Ctrl-C / --bail / CI timeout leaves a .qwen-serve-external-read-* behind on the developer's machine.
• expect(serializedEvents).toContain('was canceled by the user.') pins a security assertion to a user-facing product string; the neighboring tool_call_update status === 'failed' and not.toContain(sentinel) assertions already carry the meaning.
• In filesystem.test.ts, expect(writeResult).toEqual({ _meta: undefined }) is close to a no-op since toEqual ignores undefined-valued properties; also consider splitting the combined it (read fallback + write delegation) so deleting "the write test" later can't silently drop read coverage.
• docs/design/serve-large-text-range-consistency.md still lists "the injected ACP readTextFile adapter" among the large-text-range boundary's consumers — stale after this PR. And the docs/users/qwen-serve.md rewrite drops "Environment isolation between runtimes is not an operating-system security boundary", which was load-bearing for the multi-workspace threat model.

Conclusion

Request changes: the Critical's realpath fix is cheap and should land with this PR (or the maintainer should explicitly accept and track the risk); Majors 1–3 need at minimum documentation or follow-up issues. Otherwise the implementation, wiring coverage, and the deterministic approve/reject integration test are solid — that part passed on my machine exactly as described.

[doudouOUC]

Thanks @wenshao — this was a genuinely useful review. I verified every claim against 63a6cc4a before acting on it; one turned out not to hold, and one has a fix that is more dangerous than the bug. Details below, in your numbering.

Critical

1. Symlink under an always-allow read root — confirmed, split out.

This is the one item in the review that removes an existing defense rather than merely matching plain-CLI behavior. Before this PR a delegated read was canonicalized at the workspace boundary and rejected as symlink_escape, and the child-local fallback accepted only paths whose realpath was under its managed read roots, so the daemon failed closed on exactly this input. After the flip nothing is delegated on that path, and the "still fails closed for external paths" claim in the design doc no longer covers it.

Since the root cause is in the shared read-permission check and reproduces in the plain CLI too, I filed it as #8635 and fixed it in #8636 on current main rather than folding it in here. Two notes for that review, because the obvious patch is not quite right:

• The allow-listed roots have to be canonicalized alongside the candidate. Resolving one side only turns every temp-dir read into a confirmation prompt wherever a root itself sits behind a symlink — /var → /private/var on macOS is the everyday case.
• The auto-memory branch must not get the same treatment. In local-memory mode that root is under a repo-tracked .qwen/, and getAutoMemoryTrustedAnchor documents that the write boundary canonicalizes only the trusted anchor while appending the managed suffix literally, precisely so a checked-in .qwen -> /outside cannot relocate it. Candidate-only resolution there is fail-closed and still refuses a link planted inside the root. isAutoMemPath / isUserAutoMemPath being lexical while isTeamAutoMemPath and isManagedMemoryPath are not looks like an inconsistency to clean up, and is load-bearing.

The new tests were run against the unfixed code as a negative control: nine escape cases report expected 'allow' to be 'ask' — the silent read you described — and the symlinked-root case reports expected 'ask' to be 'allow', which is what pins the both-sides requirement. I would treat #8636 as landing before this PR.

Major

2. The write family still reproduces #8618 — accepted, documented; writes stay delegated.

Confirmed. The adapter resolves writes with a write intent, so an out-of-workspace target is refused after the diff has already rendered and the user has already approved.

I am not moving writes child-local to match. That would give up the trust gate, symlink rejection, TOCTOU protection, atomic temp-and-rename with mode preservation, and the write audit — a materially larger concession than the read change, and not one this PR should make by implication. What was missing is that the docs never said the reported bug therefore survives for the edit and write family. The design doc now has a "What this does not fix" section stating it plainly, and the developer doc points at it.

3. Pre-approval content reaches every SSE subscriber — accepted, documented.

Confirmed: the confirmation payload is built by reading the file, so an out-of-workspace diff is fanned out before the approval decision exists, and in the daemon that means every subscriber rather than one person at a terminal.

You were right that the existing framing buries this. The user-facing security section now carries it in its own bullet — previously this material was appended to a bullet whose heading is about the child's environment, which is exactly where a reader auditing filesystem scope would skip past it — and the design doc has a matching section.

Suggestions

4. Unbounded pre-reads — premise confirmed, but the proposed fix would corrupt files. Deferring.

Your read of the branch condition is exactly right: readFileWithLineAndLimit takes the windowed path only when a positive line, a finite limit, or maxOutputBytes is present, and otherwise falls through to a full-file buffer with no cap. So the failure mode really does change from a typed refusal to process death.

But passing maxOutputBytes at those call sites is not a safe fix. That parameter truncates; the workspace filesystem refused. A truncated pre-read feeding write_file or replace would write the truncated content back and silently delete the rest of the file — a data-loss bug strictly worse than the OOM it prevents. The safe shape is a refusal with a typed error, and those call sites are shared with the plain CLI, where files between the new threshold and the current limit edit fine today and would start failing.

That is a real change with real regression surface, and it is a Suggestion on a PR whose production logic is about ten lines. I would rather do it deliberately in its own change than bolt it on here. Deferring to a follow-up issue; recording the deferral here so it is not silently dropped.

5. No opt-out — agreed in principle, deferring as a feature.

The point stands: binding to a non-loopback host with a token is a documented topology, and for those operators "authenticated clients are the same security principal" widens the trust model rather than restating it. But a settings key is a new user-facing feature, and adding one to a fix PR is how a ten-line change becomes a fifty-line one with its own settings/schema/docs surface. Deferring together with #4.

I did take the adjacent documentation point: the security section now says the HTTP filesystem route still refuses these paths, so the disagreement between the two halves of one daemon is at least written down.

6. adapter.readText has no test — this one does not hold.

bridge-file-system-adapter.test.ts already has the direct unit test you are asking for:

describe('boundary enforcement')
  it('rejects reads outside the bound workspace with path_outside_workspace')
    → adapter.readText({ path: '/etc/passwd', ... })
    → expect(err.kind).toBe('path_outside_workspace')

There is also a symlink-escape case that drives adapter.readText through a link pointing out of the workspace and pins symlink_escape / path_outside_workspace. Both survive the capability flip, since they call the adapter directly rather than through a production read path. The fail-closed claim is anchored. No change made.

Nits

Applied: the fixture base no longer prefers the real $HOME (it is /var/tmp only now, which is outside both the workspace and the /tmp local-read root — you and I flagged this one independently); the assertion pinned to the localized rejection copy is gone, leaving the failed tool_call_update and the sentinel absence to carry the meaning; the combined filesystem case is split into two it blocks so deleting the write half cannot silently drop read coverage, and the write half now round-trips a defined _meta instead of asserting { _meta: undefined }, which toEqual would have accepted for {}; the sentence about environment isolation not being an OS security boundary is restored; the stale consumer list in the large-text-range doc now says the injected adapter is no longer a production consumer and that its concurrent-append / truncation / symlink-replacement guarantees no longer apply to any agent read.

On the duplicated tradeoff enumeration: the design doc now declares itself the owner and the developer and user docs link to it instead of restating the limits. The bridge README turned out not to duplicate it — its addition is an API description of the new option — so I left it alone.

Also folded in the CI bot's suggestion to drop the local isPathAtOrWithin copy in favor of the exported isPathWithinRoot. Worth noting for anyone reviewing that hunk: the argument order is reversed between the two, so the call sites are not a straight substitution.

Process

Still outstanding from the triage bot: pull_request CI has not run at this SHA, and a sandboxed verification run. I will chase both before asking for another look.

中文说明

感谢 @wenshao，这份 review 很有价值。所有论断我都对照 63a6cc4a 逐条核实后才动手；其中一条不成立，另一条按建议修反而比原问题更危险。以下按你的编号说明。

Critical

1. 始终允许根目录下的 symlink —— 确认属实，已拆分处理。

这是本次 review 中唯一真正移除既有防线、而非仅仅对齐纯 CLI 行为的一项。改动前，被委托的读取会在 workspace 边界被规范化并以 symlink_escape 拒绝，child 本地 fallback 也只接受 realpath 位于其托管读取根内的路径，因此 daemon 对这一输入是 fail-closed 的。改动之后该路径不再委托，设计文档中"外部路径仍然 fail closed"的说法也就不再覆盖它。

由于根因位于共享的读取权限检查、且在纯 CLI 上同样复现，我将其提为 #8635，并在基于当前 main 的 #8636 中修复，而非并入本 PR。有两点提醒，因为最直观的补法并不正确：其一，始终允许的根目录必须与候选路径一起规范化，只解析一侧会导致根目录自身位于符号链接之后时所有临时目录读取都变成确认提示（macOS 上 /var → /private/var 是日常情况）；其二，auto-memory 分支不能同样处理，local-memory 模式下该根目录位于仓库跟踪的 .qwen/ 之下，getAutoMemoryTrustedAnchor 已记录写边界只规范化 trusted anchor、按字面拼接托管后缀，正是为防止签入的 .qwen -> /outside 挪动边界。

新增测试在未修复代码上做了反向对照：9 条逃逸用例报 expected 'allow' to be 'ask'，即你描述的静默读取；symlink 根目录那条报 expected 'ask' to be 'allow'，正是它固定了"两侧都要规范化"这一要求。建议 #8636 先于本 PR 合入。

Major

2. 写家族仍复现 #8618 —— 采纳，已写入文档；写入维持委托。

确认属实：adapter 以 write 意图解析路径，workspace 外目标在 diff 已渲染、用户已批准之后才被拒绝。

我不打算把写入也改为 child 本地。那会放弃 trust gate、symlink 拒绝、TOCTOU 防护、保留权限位的原子 temp+rename 以及写入审计——代价远大于读侧的改动，本 PR 不应以隐含方式做出这种让步。真正缺失的是：文档从未说明所报告的 bug 因此对 edit/write 家族仍然存在。设计文档现已新增"What this does not fix"章节明确写出，开发者文档指向它。

3. 批准前内容到达所有 SSE 订阅者 —— 采纳，已写入文档。

确认属实。你指出的"现有表述掩盖了这一点"是对的：用户文档的安全章节现已将其独立成 bullet——此前这段内容被追加在标题讲 child environment 的 bullet 里，正是审计文件系统作用域的读者会略过的位置——设计文档也有对应章节。

Suggestions

4. 无上限预读 —— 前提确认属实，但按建议修会损坏文件，延后处理。

你对分支条件的判断完全正确：无窗口时会走整文件缓冲且无上限，失败模式确实从带类型的拒绝变成了进程死亡。

但在那些调用点传 maxOutputBytes 不是安全的修法。该参数是截断语义，而 workspace 文件系统是拒绝。被截断的预读若喂给 write_file 或 replace，会把截断后的内容写回、静默删除文件其余部分——这是数据丢失，严格来说比它所预防的 OOM 更严重。安全的形态是抛出带类型的拒绝，而这些调用点与纯 CLI 共用，当前介于新阈值与现有上限之间的文件本可正常编辑，届时会开始失败。

这是一个有真实回归面的改动，而它只是一条 Suggestion，本 PR 的生产逻辑约十行。我更倾向于单独、审慎地做，而不是附加在这里。延后到 follow-up issue，并在此记录以免被静默丢弃。

5. 缺少 opt-out —— 原则上同意，作为功能延后。

你的论点成立：绑定非回环地址加 token 是文档化的部署形态，对这类运维者而言"认证客户端即同一安全主体"是信任模型的扩大而非重述。但 settings key 是新的用户可见功能，把它加进一个 fix PR，正是十行改动膨胀成五十行并附带 settings/schema/文档面的方式。与 #4 一并延后。

相邻的文档要点我采纳了：安全章节现已说明 HTTP 文件路由仍然拒绝这些路径，从而把同一 daemon 两半之间的判断分歧至少记录在案。

6. adapter.readText 没有测试 —— 这一条不成立。

bridge-file-system-adapter.test.ts 中已有你所要求的直接单测：describe('boundary enforcement') 下的 it('rejects reads outside the bound workspace with path_outside_workspace')，直接调用 adapter.readText({ path: '/etc/passwd', ... }) 并断言 err.kind === 'path_outside_workspace'。另有一条 symlink 逃逸用例，让 adapter.readText 经由指向 workspace 外的链接执行并固定 symlink_escape / path_outside_workspace。两者都不受 capability flip 影响，因为它们直接调用 adapter 而非经由生产读取路径。fail-closed 的论据是有测试支撑的。未做改动。

Nits

已采纳：fixture 根目录不再优先使用真实 $HOME（现在只用 /var/tmp，它同时位于 workspace 和 /tmp 本地读取根之外——这一点你我各自都提到了）；绑定本地化拒绝文案的断言已删除，语义由失败的 tool_call_update 和 sentinel 缺席承担；混合的 filesystem 用例拆为两个 it，删除写的一半不会再静默带走读覆盖，写的一半改为对已定义的 _meta 做往返断言，而非 { _meta: undefined }（后者对 {} 同样通过）；"环境隔离不是操作系统安全边界"那句已恢复；large-text-range 文档中过期的消费者列表现已说明注入的 adapter 不再是生产消费者，其并发追加 / 截断 / symlink 替换保证不再适用于任何 agent 读取。

关于重复的 tradeoff 枚举：设计文档现已声明为唯一来源，开发者文档与用户文档改为链接而不再复述。bridge README 经核实并未重复——它新增的是该选项的 API 说明——因此未作改动。

同时采纳了 CI bot 关于用导出的 isPathWithinRoot 替换本地 isPathAtOrWithin 副本的建议。提醒 review 该 hunk 的人：两者参数顺序相反，并非直接替换。

流程

triage bot 提出的两点仍未完成：该 SHA 上 pull_request CI 尚未运行，以及沙箱验证。我会在再次请求 review 之前跟进这两项。

[qwen-code-ci-bot]

🩺 serve daemon A/B

Built the PR base vs this PR head 297dea9, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

✅ No response changes against the PR base across 4 scenario(s).

— Qwen Code · serve A/B

[qwen-code-ci-bot]

Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI and the unit suites were run locally on Linux only.

中文说明

未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI and the unit suites were run locally on Linux only。

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

[doudouOUC]

On the CHANGES_REQUESTED grounds — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) are skipped by design on every PR in this repo, not because of anything in this branch.

.github/workflows/ci.yml gates both jobs on if: "${{ !cancelled() && github.event_name == 'merge_group' }}", and the comment directly above them states the intent: "macOS/Windows: slowest/costliest runners, rare platform regressions — run only in the merge queue. Skipped on PR (ubuntu is the fast PR signal)... A skipped named job reports under its exact name and satisfies the required check." They are named jobs rather than a matrix specifically so the skip still satisfies the required context. Those platforms get their real run when the PR enters the merge queue.

So the observation is accurate but the conclusion does not follow: no PR in this repo can show green macOS/Windows unit jobs, and holding this one to that bar would gate it on something its author cannot trigger. Test (ubuntu-latest, Node 22.x) passed at 2aa6748df, along with the rest of the required set.

The genuine merge gate on this PR is the Critical you re-flagged, which I agree with — see the thread on bridge.ts:2548. It closes by #8636 landing first, which is the sequencing you recommended.

中文说明

关于 CHANGES_REQUESTED 的依据 —— Test (macos-latest, Node 22.x) 和 Test (windows-latest, Node 22.x) 在本仓库所有 PR 上都是设计性跳过，与本分支无关。

.github/workflows/ci.yml 对这两个任务的条件是 if: "${{ !cancelled() && github.event_name == 'merge_group' }}"，其上方注释已说明意图："macOS/Windows：最慢/最贵的 runner，平台回归罕见 —— 仅在 merge queue 中运行。PR 上跳过（ubuntu 是快速的 PR 信号）……被跳过的具名任务会以其确切名称上报并满足必需检查。" 它们被写成具名任务而非 matrix，正是为了让跳过仍能满足必需上下文。这两个平台会在 PR 进入 merge queue 时得到真实运行。

因此该观察属实，但结论不成立：本仓库任何 PR 都不可能出现绿色的 macOS/Windows 单元任务，以此为标准会让本 PR 被卡在作者无法触发的条件上。Test (ubuntu-latest, Node 22.x) 在 2aa6748df 上已通过，其余必需检查同样通过。

本 PR 真正的合并门禁是你重新标记的那条 Critical，我认同它 —— 见 bridge.ts:2548 的线程。它将通过先合入 #8636 来解除，正是你建议的顺序。

[wenshao]

Review — fix(serve): Allow approved same-host text reads outside workspace

Overview

Adds BridgeOptions.delegateReadTextFileToClient (default true) and sets it to false at every daemon-owned bridge construction site, so qwen serve children advertise fs: { readTextFile: false, writeTextFile: true } and serve text reads from their own FileSystemService instead of round-tripping through WorkspaceFileSystem. No child-side code change is needed: AcpFileSystemService.readTextFile already early-returns to the fallback when the capability is off, and setupFileSystem still installs the service because fs remains a truthy object. The write side stays delegated.

The mechanism is minimal and the wiring is complete — createServeApp's embedded bridge, the primary runtime, the boot-time static secondary (workspace: [primary, secondary]), and the dynamic POST /workspaces runtime are all set, and each is pinned by an assertion. The docs are unusually honest about what is being given up. The bulk of my comments below are about the security envelope, not the implementation.

---

1. This removes the last read backstop that no approval mode can bypass (main risk)

Before this change, WorkspaceFileSystem was a hard boundary: no allow-rule, --yolo, or auto-accept mode could get an agent read past it. After it, the only gate on host-wide text reads is getFileReadDefaultPermission, which returns ask — and ask is exactly what approval modes and allow rules are designed to short-circuit. A daemon running in an auto-approve mode now grants the agent read access to everything the daemon UID can read: ~/.ssh, ~/.aws/credentials, other workspaces' sources.

The "the agent has shell anyway, so this is not a new capability" argument is the right frame and it does bound the damage — but it isn't airtight, because shell tools can be disabled per-workspace (persistDisabledTools) while read_file normally isn't. A deployment that disabled shell specifically to bound filesystem reach previously still had WFS underneath; it no longer does.

Suggestion: plumb this through as an operator-visible switch (settings key or qwen serve flag) defaulting to the new behavior, rather than hardcoding false at four call sites. The option already exists on BridgeOptions, so this is a small plumb, and it gives hardened deployments a way back to the boundary without a fork.

Related question: when an external read is auto-approved (allow rule or auto-approve mode), is a permission-audit event still emitted? WFS's fs.access audit is gone by design, and if auto-approval also skips the permission audit, host-wide reads become fully unobserved for an operator watching PermissionAuditRing. Worth confirming explicitly in docs/design/daemon-local-text-reads.md under "Resource and audit boundaries".

2. Pre-approval content fan-out is broader than the test shows

docs/design/daemon-local-text-reads.md calls this out, and correctly. But the integration test only demonstrates the benign case: read_file's confirmation payload carries no file content, so expect(serializedEvents).not.toContain(sentinel) passes on rejection for reasons that don't generalize.

For the edit/write family it does not hold. EditTool builds its confirmation by calling getFileSystemService().readTextFile(...) (packages/core/src/tools/edit.ts:199), WriteFileTool the same (write-file.ts:203). That pre-read previously failed for an out-of-workspace path, so no content ever entered the confirmation payload. It now succeeds, so the full prior content of e.g. ~/.aws/credentials is rendered into a diff and fanned out to every attached SSE subscriber before anyone votes — and it is still fanned out when the user rejects. The eventual delegated write is then refused by WFS anyway, so this path yields the exposure with none of the benefit.

Worth considering before merge: suppress or redact the diff body in the confirmation payload when the target resolves outside every registered workspace. That keeps the read fix while closing the one genuinely new exposure this PR creates.

3. "Fails closed" is not quite accurate

docs/design/daemon-local-text-reads.md (Compatibility) and 07-workspace-filesystem.md both state the retained adapter read path makes a capability-violating delegated read "fail closed for external paths". AcpFileSystemService has a pre-existing second bypass: when a delegated read fails with path_outside_workspace or symlink_escape, it retries the read locally if the path sits under buildAcpLocalReadRoots() — which on POSIX includes /tmp unconditionally, plus anything in QWEN_ACP_LOCAL_READ_ROOTS (packages/cli/src/acp-integration/acpAgent.ts:340-360, service/filesystem.ts:150-200). So the boundary is fail-closed only outside those roots.

The doc claims to be "the single owner of that tradeoff list", and this second read-bypass mechanism isn't in it. Two follow-ups worth noting: (a) add a sentence; (b) that fallback path plus its ~15 tests is now unreachable in the daemon (capability check returns before the try), and only lives on for generic ACP hosts — a note in the code saying so would keep a future reader from assuming it still guards daemon reads.

Credit where due: the integration test's choice of /var/tmp over /tmp shows you knew about the /tmp root and deliberately routed around it. QWEN_ACP_LOCAL_READ_ROOTS: '' in the daemon env is the right neutralization for the env-supplied half.

4. The write family now fails later than before

Documented under "What this does not fix", and the scoping decision is defensible — moving writes child-local would give up the trust gate, symlink rejection, atomic temp-and-rename, and the write audit, which is a much larger concession.

But note the UX direction of travel: previously an out-of-workspace replace failed immediately at the pre-read. Now the pre-read succeeds, a diff renders, the user is prompted, the user approves, and then WFS refuses. Failing after an approval round-trip is worse than failing before one, and it is also the exact window in which issue 2's content exposure happens. At minimum, make the terminal WFS rejection message name the cause ("target is outside the registered workspace") so the model doesn't just retry via shell — the doc concedes it will.

5. Test coverage

Strong overall — capability defaults are pinned on both sides, all four wiring sites assert the flag, and the new real-daemon test exercises approve/reject symmetrically against a genuine qwen --acp child. Specific notes:

• Silent skip on a security regression test. findExternalReadBase() iterates a single-element ['/var/tmp'] and the test ctx.skip()s if it isn't writable. Hardened CI images and some containers don't give you /var/tmp, and the failure mode is a green run with zero coverage. Either add a second candidate, or make the skip loud (env-gated hard failure on CI) so a silently-disabled test is noticed.
• Same interrupt-leak the comment warns about. The rationale for avoiding $HOME is that afterAll-only cleanup leaks on Ctrl-C / --bail / timeout. /var/tmp has the identical property — it just leaks somewhere less annoying. Fine as a tradeoff, but the comment reads as if the problem was solved rather than relocated.
• expect(modelRequests).toHaveLength(1) on the reject path is the assertion carrying "the turn ended without another model request". It's the right assertion, but it will flake if a retry or continuation ever adds a marker-bearing request. Consider asserting on the absence of a tool-result-bearing request instead of the raw count.
• Nothing pins the defensive read path end-to-end (capability says false, non-compliant child calls readTextFile anyway, WFS rejects). The adapter-level equivalent already exists (bridge-file-system-adapter.test.ts:454), so this is optional — but that's the property the docs lean on hardest.

6. Nits

• packages/acp-bridge/src/bridgeOptions.ts: "When provided, enabled BridgeClient.readTextFile and BridgeClient.writeTextFile callbacks delegate…" — "enabled" is doing unclear work. Suggest "the enabled readTextFile / writeTextFile callbacks".
• docs/developers/daemon/07-workspace-filesystem.md: the new second paragraph packs eight distinct claims (capability slice, tradeoff pointer, fix(serve): align same-host daemon text reads with CLI permissions #8618 status, permission rules, HTTP scoping, discovery tools, auxiliary paths, sandbox disclaimer) into one block. Bullets would survive future edits better.
• bridge.ts reads opts.delegateReadTextFileToClient ?? true inline at the initialize site. Since the default is load-bearing for every non-daemon consumer, consider resolving it once next to the other option defaults so it can't drift if a second initialize site ever appears.

Performance

Net positive and unmentioned in the PR body: agent text reads no longer make a JSON-RPC round trip to the daemon and back, and large file contents stop being marshalled through the ACP pipe. Every pre-read on the edit/write path saves that hop too.

---

Verdict

The implementation is clean, the wiring is complete, and the documentation is unusually candid about the tradeoff. My blocking-ish concern is item 2 — the pre-approval diff fan-out for the edit/write family is a genuinely new exposure created by this PR, it isn't covered by the test that appears to cover it, and it buys nothing since the write is refused afterward anyway. Item 1 (operator switch) is a strong suggestion rather than a blocker. Items 3–6 are documentation and test-hygiene follow-ups.

[doudouOUC]

Thanks @wenshao — round 2 is sharper than round 1 and two items changed my mind. Pushed 4aeb7225b (also merges current main; the branch had gone CONFLICTING).

Fixed in 4aeb7225b

Item 5, silent skip. You are right that this was the worst kind of test bug — a green run with zero coverage on any image without a writable /var/tmp. findExternalReadBase() now takes a QWEN_TEST_EXTERNAL_READ_BASE override and throws on CI when no candidate works, so a disabled security regression test can no longer pass as green. Locally it still skips.

Item 5, leak comment. Also right, and the comment was worse than the code: it read as if moving off $HOME had solved the interrupt-time leak. It says plainly now that /var/tmp relocates the leak somewhere harmless rather than removing it.

Item 3. Verified and the docs were wrong. defaultAcpOnlyLocalReadRoots() returns /tmp unconditionally on POSIX, and parseAcpLocalReadRootsEnv() adds whatever QWEN_ACP_LOCAL_READ_ROOTS names, so the retained adapter read path is fail-closed only outside those roots. The design doc now bounds its own claim and records that the daemon empties the env-supplied half for the child. Your (b) is in too: filesystem.ts carries a note at the capability branch saying everything below it — the localReadRoots retry included — is unreachable under qwen serve and now guards only generic ACP hosts, so nobody reads it as a live daemon backstop.

Item 6, all three. bridgeOptions.ts reads "the enabled … callbacks"; the developer doc's eight-claim paragraph is bullets; and bridge.ts resolves delegateReadTextFileToClient once beside the other option defaults instead of ?? true inline at the initialize site.

Item 1 — your persistDisabledTools argument lands, but I am still not doing it here

The audit half first, since it was a question: no event is emitted. PermissionAuditPublisher's entire surface is request-lifecycle — recordRequested / recordVoted / recordResolved / recordTimeout / recordForbidden. A decision that never becomes a permission request produces no record at all, so an allow-rule or auto-approve-mode external read is invisible to PermissionAuditRing. That is not new to this PR — it is how auto-approval has always worked — but combined with WFS's fs.access audit going away, this PR is what makes it matter for host-wide paths. Documenting it under "Resource and audit boundaries".

On the switch itself: the persistDisabledTools point is the part I had not weighed. I had been treating "the agent has shell anyway" as bounding the whole thing, and a deployment that disabled shell precisely to bound filesystem reach is a real counterexample — it had WFS underneath and now does not. That moves this from "policy nicety" to "someone's threat model actually regressed".

I am still deferring it, and I want to be explicit that this is a scoping call rather than disagreement. A settings key is a new user-facing surface — schema, precedence, docs, tests — landing on a PR whose production logic is about ten lines, at review round 3, with zero standing approvals. Bolting it on is how this PR stops being reviewable. Filing it as a follow-up with your reasoning quoted, and I would not object to a maintainer making it a merge condition instead.

Item 2 — agreed it is real, and it is the one I am asking for direction on

Your framing is what makes this different from the other items: the pre-approval diff for the edit/write family exposes the full prior content of an out-of-workspace file to every subscriber, it is still fanned out on rejection, and the write is refused afterward anyway — so the exposure buys nothing. That is a genuinely new hole this PR opens, not an inherited one. @samuelhsin reached the same place independently.

I did not fix it in this push because it is not the small change it looks like. buildPermissionRequestContent(confirmation) takes only the confirmation object; it has no workspace knowledge, so gating on "resolves outside every registered workspace" means threading config or the workspace context through that function and its call sites, plus their tests. And redaction is a product decision, not just a security one — blanking oldText makes the approval prompt less informative exactly when the user most needs to judge what they are approving, so the shape (omit / truncate / hash / warn) wants a deliberate choice rather than whatever I pick at round 3.

So: agreed on the finding, deferring on the fix, and flagging that I understand this is your blocking-ish item. If the maintainers want it in this PR I will do it — say so and I will thread the context through and pick omission with an explicit warning block. Otherwise it goes in the follow-up alongside item 1.

Item 4

Taking the message part — a terminal WFS rejection that names the cause is cheap and directly reduces the shell-retry the doc concedes. Not in this push; it belongs with the item 2 work since both live on the edit/write path.

Item 5, remaining

The toHaveLength(1) point is fair and I would rather assert on the absence of a tool-result-bearing request. Leaving it for the same follow-up rather than touching the reject-path assertions in the same push that changes the fixture base — I would rather not move two things under one test at once.

Performance

Fair catch that the PR body never mentions it. Adding a line: reads no longer round-trip through the daemon, and large contents stop being marshalled across the ACP pipe, on every pre-read too.

中文说明

感谢 @wenshao —— 第二轮比第一轮更锐利，其中两条改变了我的判断。已推送 4aeb7225b（同时合并了当前 main，分支此前已变为 CONFLICTING）。

已在 4aeb7225b 修复：

• Item 5 静默跳过：你说得对，这是最糟的一类测试缺陷——在没有可写 /var/tmp 的镜像上会变成"零覆盖的绿色通过"。findExternalReadBase() 现在支持 QWEN_TEST_EXTERNAL_READ_BASE 覆盖，并在 CI 上直接抛错，被禁用的安全回归测试不可能再显示为通过；本地仍然跳过。
• Item 5 泄漏注释：同样成立，而且注释比代码更糟——它读起来像是"离开 $HOME 就解决了中断时的泄漏"。现在明确写出 /var/tmp 只是把泄漏挪到无害的位置，并未消除。
• Item 3：已核实，文档确实错了。defaultAcpOnlyLocalReadRoots() 在 POSIX 上无条件返回 /tmp，parseAcpLocalReadRootsEnv() 还会加入 QWEN_ACP_LOCAL_READ_ROOTS 指定的路径，因此保留的 adapter 读取路径只在这些根之外才是 fail-closed。设计文档现已限定该论断，并记录 daemon 会把 env 那一半置空。你的 (b) 也已加入：filesystem.ts 在 capability 分支处注明其下所有代码（含 localReadRoots 重试）在 qwen serve 下不可达，仅服务于保留委托的通用 ACP host。
• Item 6 三条全部采纳：bridgeOptions.ts 改为 "the enabled … callbacks"；开发者文档中塞了八个论点的段落已改为 bullet；bridge.ts 在其他选项默认值旁一次性解析 delegateReadTextFileToClient，不再在 initialize 处内联 ?? true。

Item 1 —— 你的 persistDisabledTools 论点成立，但我仍不在本 PR 做。

先回答审计问题：不会发出任何事件。 PermissionAuditPublisher 的接口全部是请求生命周期方法（recordRequested / recordVoted / recordResolved / recordTimeout / recordForbidden）。从未成为权限请求的决策不会产生任何记录，因此 allow 规则或自动批准模式下的外部读取对 PermissionAuditRing 完全不可见。这并非本 PR 引入——自动批准一直如此——但叠加 WFS 的 fs.access 审计消失，本 PR 使它开始对主机范围路径产生影响。会补进"资源与审计边界"。

关于开关本身：persistDisabledTools 这一点是我此前没有权衡到的。我一直把"agent 本来就有 shell"当作整体上界，而"为限制文件系统触及范围而专门禁用 shell"的部署是真实的反例——它此前底下还有 WFS，现在没有了。这让该问题从"策略上的锦上添花"变成"确实有人的威胁模型发生了退化"。

我仍然延后，并且想明确这是范围取舍而非分歧。settings key 是新的用户可见面（schema、优先级、文档、测试），落在一个生产逻辑约十行、处于第三轮 review、且零有效批准的 PR 上。硬加进来正是让本 PR 失去可评审性的方式。会作为 follow-up 提出并引用你的论证；如果维护者要把它作为合并条件，我不反对。

Item 2 —— 认同问题真实，这条我需要方向指示。

你的表述才是它区别于其他条目的关键：edit/write 家族的批准前 diff 会把 workspace 外文件的完整原内容暴露给所有订阅者，拒绝时依然被扇出，而写入随后仍会被拒绝——即这次暴露毫无收益。这是本 PR 新开的洞，不是继承来的。@samuelhsin 独立得出了同样结论。

本次未修，是因为它并非看上去那么小。buildPermissionRequestContent(confirmation) 只接收 confirmation 对象，不掌握 workspace 信息，因此要按"解析后位于所有注册 workspace 之外"来门控，就需要把 config 或 workspace context 贯穿该函数及其调用点与相应测试。而且脱敏是产品决策而非纯安全决策——清空 oldText 恰恰在用户最需要判断"我在批准什么"的时候削弱了提示信息，所以采取何种形态（省略 / 截断 / 摘要 / 警告）应当是审慎选择，而不是我在第三轮随手定一个。

因此：结论认同，修法延后，并且我明白这是你标为 blocking-ish 的一条。如果维护者希望它在本 PR 内完成，请明说，我会把上下文贯穿进去并采用"省略 + 显式警告块"的方案；否则它与 item 1 一同进入 follow-up。

Item 4：采纳其中的报错信息部分——让 WFS 的终态拒绝写明原因，成本很低且能直接减少文档所承认的 shell 重试。本次未做，因为它与 item 2 同处 edit/write 路径，应一并处理。

Item 5 其余：toHaveLength(1) 的意见合理，我也更倾向于断言"不存在携带工具结果的请求"。留给同一个 follow-up——我不想在同一次推送里既改 fixture 基址、又改拒绝路径的断言。

性能：确实是 PR 描述里漏掉的一点。会补一句：读取不再往返 daemon，大文件内容不再经 ACP 管道封送，每次预读同样受益。

[yiliang114]

Re-reviewed the updated revision. The intent (per maintainer) is confirmed in code: production qwen serve sets delegateReadTextFileToClient=false so read_file goes through the child's own CLI filesystem service (normal permission flow) instead of ACP client delegation — avoiding the 'ACP permission denied then shell fallback' path. New delta is consistent and safe: bridge resolves the default once (prevents drift across initialize sites), filesystem.ts documents that the localReadRoots retry is unreachable under serve (generic-ACP-only guard), and added tests pin delegateReadTextFileToClient:false across serve wiring. Writes stay delegated through WorkspaceFileSystem; unexpected delegated reads fail closed. No new P0/P1.

[doudouOUC]

Self-audit: must-fix list before this is mergeable

I re-audited this PR adversarially — presuming the diff wrong rather than looking to confirm it — and it does not hold up. Four items must land before merge. Everything below was verified against this branch's HEAD, not inferred from the diff.

The diff itself is small and defaults safe: one BridgeOptions flag defaulting to true, four qwen serve sites setting it false, capability declared in ACP initialize. The problem is not in what the diff adds. It is in what the removed wall was covering for.

1. Symlink reads outside the workspace, with no prompt — blocking

Covered in detail in the reopened thread above. Summary: workspace-file-system.ts rejects any symlink on the text-read path (symlink_escape) before returning content. Under qwen serve that was the only thing containing the L4 lexical-allow hole, and this PR routes reads to the child-local StandardFileSystemService, which has no equivalent check. My earlier "Gate cleared" verified L3, which never runs when an allow rule matches.

Fix, kept in scope: re-check the resolved real path against the workspace and allowedRoots at the child-local read site this PR opens. Not in packages/core/src/permissions/** — changing allow-rule matching semantics is maintainer-gated, overlaps #8627, and does not belong in this PR. The L4 gap goes out as its own issue.

2. The shared text pre-read is now unbounded — blocking

edit.ts, write-file.ts and notebook-edit.ts call readTextFile({ path }) at eight sites with no limit and no maxOutputBytes. In fileUtils, the windowed path requires a line, a finite limit, or maxOutputBytes; with none of them the call falls through to a whole-file fs.promises.readFile.

Before this PR those reads hit WFS's 256 KiB file_too_large rejection. After it they load the entire file into a long-lived multi-session daemon. The path is reachable: the checkPriorRead gate means the file must have been read once, but read_file truncates — a user approves reading a large log, sees the first N lines, the cache is populated, and one replace then pulls the whole file into memory. The same hop also loses WFS's binary_file rejection, so binaries now get decoded into a diff.

The design doc is wrong here too. It says this path "accepts the CLI's read-side limits and behavior", then contrasts it with direct read_file "still applying the core line and output limits". That framing implies limits exist on the shared pre-read. There are none.

Fix: pass an explicit maxOutputBytes (reuse getRangeReadByteLimit) and reject over it rather than silently reading everything; correct the doc to say plainly that the shared pre-read applies no size limit.

3. "Same-host" is asserted nowhere in code — blocking

The title and the design doc both rest on "same-host". All four production sites are unconditional constants. There is no peer-address check, no header check, and no coupling to the bind address — while --hostname 0.0.0.0 --token <t> is a supported, first-class deployment that the daemon prints CORS guidance for. Under that deployment a remote token holder gets host-wide file reads, and /session/:id/approval-mode lets them switch to a mode where even the 'ask' never appears. Before this PR, WFS refused out-of-workspace reads regardless of approval mode; the mode could not breach that wall. Now it can.

There is also no operator kill switch: nothing in settings or flags restores the previous boundary.

Fix: gate the toggle on isLoopbackBind(opts.hostname) so a non-loopback bind keeps delegating (i.e. keeps the old boundary), or require an explicit opt-in flag that defaults off. An assumption that contradicts a supported deployment is not a mitigation.

4. The fixes for 1 and 2 need pinning tests — blocking

The tests added here are good in isolation — the capability defaults are asserted both ways, the integration reject_once arm asserts the sentinel is absent, and findExternalReadBase() throws rather than skipping when no base exists. But not one bypass attempt is tested. At minimum:

• a workspace-internal symlink pointing outside must be refused (or must surface the real path in the prompt);
• with a persisted Read(//<ws>/**) allow rule present, an out-of-workspace symlink read must not be auto-allowed;
• an oversized shared pre-read must be rejected rather than read whole.

Without these, both fixes regress silently.

Should fix, not blocking

• The design doc claims the daemon empties QWEN_ACP_LOCAL_READ_ROOTS for the child. It does not — that only happens in the integration test; SCRUBBED_CHILD_ENV_KEYS does not include the key. Impact today is zero because the retry path is unreachable once the capability is off, but it is a false statement about a mitigation that someone will later rely on. One line either way.
• read_file's confirmation prompt shows the lexical short path, so in the symlink case the user approves docs/notes.md and the read lands elsewhere. That lives in core tool code and is narrowed considerably once item 1 lands, so I am not treating it as blocking — but the approval UI showing something other than what will be read is worth a maintainer's view.

Note on CI

Test (ubuntu-latest, Node 22.x) is failing on this HEAD, but the single failing case is gemini.test.tsx > rejects --json-schema when running in interactive (TUI) mode (an extra writeStderrLine in callOrder), unrelated to this branch — 1 failed / 17853 passed. web-shell E2E Smoke and review-pr also failed and their logs have already expired, so I cannot say whether those are related. I will re-run and check rather than assume.

[wenshao]

Maintainer verification — real daemon on Linux

I built this branch and verified it against a real qwen serve daemon with a real qwen --acp child (mock OpenAI-compatible model server, ACP stdio tapped through QWEN_CLI_ENTRY so the JSON-RPC frames are observable). Linux x86-64, Node v22.22.2 — the platform the PR lists as untested. Single build; the BEFORE arm differs only by reverting the initialize capability hunk in packages/acp-bridge/src/bridge.ts and rebuilding @qwen-code/acp-bridge, so the two arms isolate exactly this change.

Verdict: the production change is correct and I reproduced #8618 before it and its absence after it. One blocking defect in the new E2E test — it cannot execute on any machine, and it takes the four pre-existing tests in the same file down on CI. Everything else below is confirmation.

---

1. Production behaviour — all four reviewer test-plan items hold

Test-plan item	Evidence	Result
1. Default bridge advertises {read:true, write:true}; daemon-owned bridges advertise {read:false, write:true}	initialize frame captured on the real daemon→child stdio. BEFORE arm: {"readTextFile": true, "writeTextFile": true}. AFTER arm: {"readTextFile": false, "writeTextFile": true}	✅
2. External read_file, default approval mode	allow_once → permission request fires, 200, sentinel reaches the next model request and the final response, tool_call_update = completed. reject_once → exactly 1 model request, sentinel absent from every subsequent model input, SSE stream and final output; tool_call_update = failed	✅
3. Asymmetric routing	On the ACP wire, AFTER arm: 1 × fs/write_text_file, zero fs/read_text_file. BEFORE arm: 1 × fs/read_text_file	✅
4. HTTP reads + external/symlinked delegated writes still rejected	GET /file?path=<abs outside ws> and the ../../.. traversal form both 400 {"errorKind":"path_outside_workspace"}. write_file to an external path, after the user approves the diff, fails with path escapes workspace and no file is created	✅

The BEFORE arm is a clean reproduction of #8618: the permission request fires, the user selects proceed_once, the daemon returns 200, and the read still fails with Error reading file …: path escapes workspace (cause: -32603).

All four daemon construction sites carry delegateReadTextFileToClient: false — 3 in run-qwen-serve.ts (primary, static secondary, dynamic workspace) and 1 in server.ts. I verified the primary site on the real ACP wire and the other three through the unit suites plus mutation testing (below).

---

2. Blocking — the new E2E regression test cannot run

REPO_ROOT is referenced twice in integration-tests/cli/qwen-serve-streaming.test.ts but is never declared or imported in that file:

integration-tests/cli/qwen-serve-streaming.test.ts(104,50): error TS2304: Cannot find name 'REPO_ROOT'.
integration-tests/cli/qwen-serve-streaming.test.ts(567,21): error TS2304: Cannot find name 'REPO_ROOT'.

Every other daemon test file declares its own (qwen-serve-routes.test.ts:55, qwen-serve-client-mcp.test.ts:55, …); this one does not. Three consequences, all observed:

1. Developer box (CI unset): the security test silently skips. Line 104 sits inside findExternalReadBase()'s try, so the ReferenceError is swallowed by the bare catch {}, every candidate is discarded, and the test reports ↓ … [no writable fixture root outside the workspace and /tmp] — even though /var/tmp is writable. Instrumenting that catch prints [PROBE] candidate rejected: /var/tmp -> ReferenceError: REPO_ROOT is not defined. The run is green with 5 skipped.
2. CI (CI=1): the whole file fails to collect. The loud-fail branch throws at module scope, so vitest reports Failed Suites 1 / no tests — this also kills the four pre-existing tests in the file (child-crash recovery, multi-client permission fan-out, Last-Event-ID resume, Todo Stop Guard replay).
3. Even with REPO_ROOT defined, the test still fails. Line 567 asks for workspaceCwd: REPO_ROOT, but beforeAll binds the daemon with --workspace workspaceDir, so the session create returns 400 Workspace mismatch: runtime is bound to "…/qwen-serve-streaming-ws-XXXXXX" but request asked for "<repo root>". All six other createOrAttachSession calls in this file pass workspaceDir; line 567 is the only outlier.

Minimal repair (what I ran locally):

 const TOKEN = 'streaming-integ-secret';
+const REPO_ROOT = path.resolve(__dirname, '../..');
@@
-      workspaceCwd: REPO_ROOT,
+      workspaceCwd: workspaceDir,

With those two lines the test is correct and has real teeth — it passes on the AFTER build and fails on the BEFORE build (AssertionError: expected false to be true). So the test design is right; only these two lines are wrong. Please also drop catch {} for something that surfaces the reason, since it is what turned a hard error into a green skip.

3. Why CI would not have caught it

npm run typecheck is npm run typecheck --workspaces --if-present, and integration-tests/ is not a workspace, so nothing runs tsc -p integration-tests/tsconfig.json. npm run lint:ci does run eslint integration-tests, but no-undef is off for TypeScript files, so eslint reports the file clean. The PR description's "root typecheck passed" is therefore accurate and simply does not cover this directory. Worth a follow-up regardless of this PR.

4. Unit-test teeth — every mutant killed

Baseline: 4 suites / 267 tests pass (bridge-file-capabilities 2, acp-integration/service/filesystem 32, server-default-bridge-wiring 2, run-qwen-serve 231).

Mutant applied to production source	Suite	Result
delegateReadTextFileToClient = true (ignore the option)	bridge-file-capabilities.test.ts	1 failed — killed
opts.delegateReadTextFileToClient ?? false (wrong default)	bridge-file-capabilities.test.ts	1 failed — killed
drop all 3 delegateReadTextFileToClient: false in run-qwen-serve.ts	run-qwen-serve.test.ts	2 failed — killed
drop delegateReadTextFileToClient: false in server.ts	server-default-bridge-wiring.test.ts	1 failed — killed

Splitting the combined capability case was the right call — mutant 1 and mutant 2 are each caught by exactly one of the two cases.

5. Documented tradeoffs I re-checked (not defects)

• Symlink escape after approval. A symlink inside an auto-allowed root (project temp dir) pointing at /var/tmp/secret still raises a permission prompt — getFileReadDefaultPermission canonicalizes through realpathNearestExisting, so the lexical-containment bypass I went looking for does not exist. Once approved, the read succeeds and no longer hits WFS's symlink rejection. That matches the design doc; there is no silent path.
• Write/edit family still reproduces fix(serve): align same-host daemon text reads with CLI permissions #8618. External write_file fails after the user approves the diff. The PR says so explicitly and the design doc owns the tradeoff list; flagging only so it is not a surprise post-merge.

6. What I did not cover

Static-secondary and dynamic-workspace runtimes were verified through their unit suites and mutation testing, not on the live ACP wire (my repeated --workspace daemon registered only the primary). Windows is still untested. I did not run the full repository suite.

---

Recommendation: fix the two lines in qwen-serve-streaming.test.ts, then this is good to merge — the production change itself verifies cleanly on Linux and closes #8618 for the read path.

中文版

维护者验证 —— Linux 真实 daemon 环境

我在本地构建了该分支，并在真实 qwen serve daemon + 真实 qwen --acp 子进程上做了验证（mock OpenAI 兼容服务作为模型端，通过 QWEN_CLI_ENTRY 对 ACP stdio 做了抓包，因此 JSON-RPC 帧可直接观测）。环境为 Linux x86-64、Node v22.22.2，正是 PR 中标记为「未测试」的平台。两个 arm 使用同一次构建，BEFORE arm 仅回退 packages/acp-bridge/src/bridge.ts 中 initialize 的 capability 那一处改动并重新构建 @qwen-code/acp-bridge，因此两个 arm 精确隔离了本次变更。

结论：生产代码的改动是正确的，我在 BEFORE arm 复现了 #8618，在 AFTER arm 确认已修复。但新增的 E2E 测试存在一个阻塞性缺陷 —— 它在任何机器上都无法真正执行，并且在 CI 上会连带拖垮同文件中已有的 4 个测试。 其余内容都是确认性结论。

1. 生产行为 —— Reviewer 测试计划的 4 项全部成立

测试计划条目	证据	结论
1. 默认 bridge 为 {read:true, write:true}；daemon 自有 bridge 为 {read:false, write:true}	从真实 daemon→child stdio 捕获的 initialize 帧。BEFORE：{"readTextFile": true, "writeTextFile": true}；AFTER：{"readTextFile": false, "writeTextFile": true}	✅
2. 默认审批模式下的外部 read_file	allow_once → 权限请求出现、返回 200、sentinel 出现在下一次模型请求与最终响应中，tool_call_update 为 completed。reject_once → 只有 1 次模型请求，sentinel 在后续模型输入、SSE 事件流与最终输出中都不存在，tool_call_update 为 failed	✅
3. 非对称读写路由	ACP 线缆上，AFTER：1 × fs/write_text_file，0 个 fs/read_text_file；BEFORE：1 × fs/read_text_file	✅
4. HTTP 读取与外部/symlink 委托写入仍被拒绝	GET /file?path=<workspace 外绝对路径> 与 ../../.. 穿越形式均返回 400 {"errorKind":"path_outside_workspace"}；用户已批准 diff 之后，写入外部路径的 write_file 仍以 path escapes workspace 失败且文件未创建	✅

BEFORE arm 是对 #8618 的干净复现：权限请求正常弹出，用户选择 proceed_once，daemon 返回 200，读取仍以 Error reading file …: path escapes workspace (cause: -32603) 失败。

四个 daemon 构造点都带上了 delegateReadTextFileToClient: false（run-qwen-serve.ts 中 3 处：primary、static secondary、dynamic workspace；server.ts 中 1 处）。其中 primary 我在真实 ACP 线缆上验证，另外三处通过单测 + 变异测试验证（见下）。

2. 阻塞项 —— 新增的 E2E 回归测试无法运行

integration-tests/cli/qwen-serve-streaming.test.ts 中引用了两次 REPO_ROOT，但该文件从未声明也未导入它：

integration-tests/cli/qwen-serve-streaming.test.ts(104,50): error TS2304: Cannot find name 'REPO_ROOT'.
integration-tests/cli/qwen-serve-streaming.test.ts(567,21): error TS2304: Cannot find name 'REPO_ROOT'.

其他 daemon 测试文件都各自声明了这个常量（qwen-serve-routes.test.ts:55、qwen-serve-client-mcp.test.ts:55 等），只有这个文件没有。由此产生三个后果，均已实测：

1. 开发机（未设 CI）：安全测试被静默跳过。 104 行位于 findExternalReadBase() 的 try 内，ReferenceError 被空的 catch {} 吞掉，所有候选目录都被丢弃，测试报告 ↓ … [no writable fixture root outside the workspace and /tmp] —— 但 /var/tmp 明明可写。给这个 catch 加日志后打印出 [PROBE] candidate rejected: /var/tmp -> ReferenceError: REPO_ROOT is not defined。整个运行是绿的，5 个测试全部 skipped。
2. CI（CI=1）：整个文件收集阶段失败。 loud-fail 分支在模块级抛出，vitest 报 Failed Suites 1 / no tests，同时连带杀掉该文件中已有的 4 个测试（child-crash recovery、多客户端权限 fan-out、Last-Event-ID resume、Todo Stop Guard replay）。
3. 即使补上 REPO_ROOT，测试依然失败。 567 行请求 workspaceCwd: REPO_ROOT，但 beforeAll 是以 --workspace workspaceDir 启动 daemon 的，因此创建会话返回 400 Workspace mismatch: runtime is bound to "…/qwen-serve-streaming-ws-XXXXXX" but request asked for "<repo root>"。该文件中其余 6 处 createOrAttachSession 传的都是 workspaceDir，567 行是唯一的例外。

最小修复（我在本地实际使用的）：

 const TOKEN = 'streaming-integ-secret';
+const REPO_ROOT = path.resolve(__dirname, '../..');
@@
-      workspaceCwd: REPO_ROOT,
+      workspaceCwd: workspaceDir,

补上这两行之后，测试本身是正确且有牙齿的：在 AFTER 构建上通过，在 BEFORE 构建上失败（AssertionError: expected false to be true）。所以测试设计没问题，只是这两行写错了。另外建议把 catch {} 换成能暴露原因的写法 —— 正是它把一个硬错误变成了绿色的 skip。

3. 为什么 CI 抓不到

npm run typecheck 实际是 npm run typecheck --workspaces --if-present，而 integration-tests/ 不是 workspace，因此没有任何环节执行 tsc -p integration-tests/tsconfig.json。npm run lint:ci 确实会跑 eslint integration-tests，但 TypeScript 文件的 no-undef 是关闭的，所以 eslint 认为该文件干净。PR 描述里「root typecheck passed」是准确的，只是它本来就覆盖不到这个目录。这一点值得单独跟进，与本 PR 是否合并无关。

4. 单测牙齿 —— 所有变异体均被杀死

基线：4 个 suite / 267 个测试全部通过（bridge-file-capabilities 2、acp-integration/service/filesystem 32、server-default-bridge-wiring 2、run-qwen-serve 231）。

施加于生产代码的变异	Suite	结果
delegateReadTextFileToClient = true（忽略该选项）	bridge-file-capabilities.test.ts	1 failed — 已杀死
opts.delegateReadTextFileToClient ?? false（默认值写反）	bridge-file-capabilities.test.ts	1 failed — 已杀死
删除 run-qwen-serve.ts 中全部 3 处 delegateReadTextFileToClient: false	run-qwen-serve.test.ts	2 failed — 已杀死
删除 server.ts 中的 delegateReadTextFileToClient: false	server-default-bridge-wiring.test.ts	1 failed — 已杀死

把合并的 capability 用例拆开是正确的决定 —— 变异 1 与变异 2 各自只被其中一个用例捕获。

5. 已复核的既定取舍（非缺陷）

• 批准之后的 symlink 逃逸。 放在自动放行根目录（project temp dir）下、指向 /var/tmp/secret 的 symlink，仍然会触发权限确认 —— getFileReadDefaultPermission 会经 realpathNearestExisting 做规范化，我原本怀疑的「词法包含判断被绕过」并不存在。批准之后读取成功，且不再经过 WFS 的 symlink 拒绝，这与设计文档一致，不存在静默路径。
• write/edit 家族仍然复现 fix(serve): align same-host daemon text reads with CLI permissions #8618。 用户批准 diff 之后，外部 write_file 依然失败。PR 已明确说明，设计文档也持有该取舍清单；这里列出只是为了避免合并后被意外发现。

6. 未覆盖的部分

static-secondary 与 dynamic-workspace 两类 runtime 是通过单测 + 变异测试验证的，未在真实 ACP 线缆上验证（我用重复 --workspace 启动的 daemon 只注册了 primary）。Windows 仍未测试。仓库全量测试套件未运行。

---

建议： 修掉 qwen-serve-streaming.test.ts 的这两行后即可合并 —— 生产改动本身在 Linux 上验证干净，并且确实闭合了 #8618 的读取路径。

[doudouOUC]

Thank you for this @wenshao — an A/B against a real daemon with the ACP stdio tapped is far more than a review, and you found the one thing that mattered. You are right on all three counts and the fix is pushed in 297dea9fd.

The defect

REPO_ROOT was never declared in this file. I lifted the fixture-base helper into qwen-serve-streaming.test.ts from a file that does declare it and did not carry the constant across. Your three consequences are exactly what happens, and the third one is the one I would not have found on my own: even with the constant defined, workspaceCwd: REPO_ROOT at line 567 contradicts the --workspace workspaceDir that beforeAll binds, so the session create 400s. Six other createOrAttachSession calls in the file pass workspaceDir; that one was mine and it was wrong.

Taking your repair as written:

• const REPO_ROOT = path.resolve(__dirname, '../..');, matching qwen-serve-routes.test.ts:55 and qwen-serve-client-mcp.test.ts:55.
• workspaceCwd: workspaceDir, with a comment saying why — the read is external because externalReadDir sits outside the bound workspace, not because the session claims a wider one. That was the confusion that produced the bug.

Locally it now executes rather than skips:

✓ cli/qwen-serve-streaming.test.ts (5 tests | 4 skipped) 14582ms
  ✓ qwen serve — same-host external text reads > returns approved content and withholds rejected content  12003ms

I have not re-run your BEFORE/AFTER arm; your AssertionError: expected false to be true on the reverted build is the teeth evidence and I am not going to reproduce it worse.

The catch {}

Also taking this, and it is the part I am least comfortable with in hindsight. I added the CI loud-fail in the previous round specifically so a disabled security test could not pass as green — and then left a bare catch upstream of it that converted a hard ReferenceError into exactly the green skip I was trying to prevent. The guard was defeated by the code three lines above it.

Each candidate's rejection reason is now collected and folded into both branches: the CI throw names why every candidate failed, and the developer-box skip emits a console.warn with the same text instead of vanishing. On your box the message would have read /var/tmp: ReferenceError: REPO_ROOT is not defined, which is the whole diagnosis.

Your §3 — one more layer

Your account of why CI missed it is right, and there is a layer beneath it worth recording for the follow-up: tsc -p integration-tests/tsconfig.json does not merely go unrun, it cannot run. The config carries a "//" documentation key inside compilerOptions.paths, and every value there must be an array, so tsc bails with

integration-tests/tsconfig.json(8,13): error TS5063: Substitutions for pattern '//' should be an array.

before type-checking a single file. esbuild flags it too on every integration-test run (▲ [WARNING] Substitutions for pattern "//" should be an array), which is how visible this has been sitting. Moving that note to a sibling comment makes the config parse — and then surfaces 404 pre-existing errors across the directory, largely because the paths entry resolves @qwen-code/sdk to source that needs DOM lib types the integration-tests config does not pull in. Two of those land in this very file (625 and 790, both TS2322: Type 'unknown' is not assignable), and line 790 is untouched main code — the Last-Event-ID resume test — so they predate this PR.

I am deliberately not fixing that here. Unblocking the config is a one-line move, but it converts "nothing checks this directory" into "404 errors to triage", and that is a follow-up in its own right, not something to bolt onto a PR at review round 4 with the production logic already verified. Flagging it so whoever picks up your §3 knows the first blocker is the config itself, not just the missing wiring.

Everything else

Nothing to add to §1, §4, §5, §6 — they are confirmation and I read them as such. Two notes:

• §5, symlink escape: that path canonicalizes through realpathNearestExisting because fix(core): Resolve symlinks before granting read permission #8636 landed mid-review to make it do so. Before that fix getFileReadDefaultPermission resolved lexically while the isPathWithinWorkspace branch beside it realpath'd, so a symlink planted in an auto-allowed root did get a silent allow. The bypass you went looking for existed; it was closed in fix(core): Resolve symlinks before granting read permission #8636 (issue A symlink under an always-allow read root bypasses the read_file confirmation prompt #8635) three days ago. Your check confirms the fix holds on a real daemon, which is better evidence than the unit tests I shipped with it.
• §2's teeth check — passes on AFTER, fails on BEFORE — is the assertion I most wanted and could not produce myself. Thank you for running it.

中文说明

感谢 @wenshao —— 在真实 daemon 上做 A/B 并抓取 ACP stdio，这已经远超一次 review，而且你找到了唯一真正要紧的问题。三点你都是对的，修复已推送 297dea9fd。

缺陷本身。 REPO_ROOT 在该文件中确实从未声明 —— 我把 fixture base helper 从一个声明了该常量的文件搬过来时漏带了常量。你列出的三个后果完全成立，其中第三条是我自己不会发现的：即使补上常量，567 行的 workspaceCwd: REPO_ROOT 与 beforeAll 中 --workspace workspaceDir 相互矛盾，会话创建会 400。文件里另外 6 处 createOrAttachSession 传的都是 workspaceDir，只有那一处是我写错的。按你给的修复采纳：补 const REPO_ROOT = path.resolve(__dirname, '../..');（与 qwen-serve-routes.test.ts:55、qwen-serve-client-mcp.test.ts:55 一致），567 行改为 workspaceCwd: workspaceDir，并加注释说明——外部性来自 externalReadDir 位于绑定 workspace 之外，而不是会话声明了更大的 workspace，正是这一点当初造成了混淆。

catch {}。 同样采纳，而且这是我事后最不舒服的一处：上一轮我加 CI loud-fail 正是为了让被禁用的安全测试不能显示为绿色，却在它上游三行留了个空 catch，把硬 ReferenceError 转成了我想防止的那种绿色 skip —— 守卫被它上面三行的代码击穿了。现在每个候选的拒绝原因都会被收集，并同时进入两个分支：CI 上 throw 的信息会写明每个候选失败的原因，开发机上的 skip 也会以 console.warn 打印同样内容而不再静默。在你的机器上这条消息会是 /var/tmp: ReferenceError: REPO_ROOT is not defined，即完整诊断。

关于你的 §3，还有一层。 你的分析正确，但底下还有一层值得为 follow-up 记录：tsc -p integration-tests/tsconfig.json 不只是没人跑，而是根本跑不起来。该配置在 compilerOptions.paths 内部放了一个 "//" 文档键，而 paths 的每个值都必须是数组，因此 tsc 会以 TS5063 直接退出，一个文件都不会检查。把这条注释移到 paths 外面之后配置能解析了，但随即暴露出整个目录 404 个既有错误 —— 主要因为 paths 把 @qwen-code/sdk 指向源码，而该源码需要 integration-tests 配置未引入的 DOM lib 类型。其中两个就落在本文件（625 与 790，均为 TS2322: Type 'unknown' is not assignable），而 790 行是 main 上未被本 PR 触碰的代码（Last-Event-ID resume 测试），说明它们早于本 PR。我刻意不在本 PR 修它：解封配置只是一行改动，但它会把「这个目录没人检查」变成「404 个错误待处理」，那本身就是一个独立的 follow-up，不该在生产逻辑已验证、且已到第四轮 review 的 PR 上硬加。提出来是为了让接手你 §3 的人知道：第一个障碍是配置本身，而不只是缺少接线。

其余部分。 §1、§4、§5、§6 是确认性结论，我照此理解，两点补充：§5 的 symlink 一节之所以会经 realpathNearestExisting 规范化，是因为 #8636 在 review 期间合入才让它如此。在那之前 getFileReadDefaultPermission 走词法解析，而紧邻的 isPathWithinWorkspace 分支走 realpath —— 放在自动放行根目录下的 symlink 确实会拿到静默 allow。你去找的那个绕过是存在的，已由 #8636（issue #8635）在三天前关闭。你的复核确认了该修复在真实 daemon 上成立，这比我随修复提交的单测更有说服力。§2 的「AFTER 通过 / BEFORE 失败」正是我最想要却无法自己产出的那条断言，谢谢你跑了它。

[wenshao]

Re-verified at 297dea9 — blocker cleared, LGTM

Same Linux harness as my earlier run (real qwen serve daemon + real qwen --acp child, mock OpenAI, ACP stdio tapped via QWEN_CLI_ENTRY). git diff <previous head>..297dea9 -- packages/ docs/ is empty, so the production change I already verified is byte-identical; this pass re-checks the test fix and re-confirms the suite.

Item	Observed	Result
REPO_ROOT declared (line 56)	tsc clean — no TS2304, no swallowed ReferenceError	✅ fixed
workspaceCwd: workspaceDir (line 577)	no 400 workspace_mismatch; session created against the bound workspace	✅ fixed
bare catch {} → per-candidate diagnostics	rejection reason now surfaces on both the skip and the CI throw	✅ fixed
Whole qwen-serve-streaming.test.ts, developer box	5 passed (5) — was 5 silently skipped	✅ green
Same file with CI=1	1 passed / 4 skipped — was Failed Suites 1 / no tests	✅ green
Teeth: repaired test against the BEFORE build	fails with AssertionError: expected false to be true	✅ has teeth
Unit suites re-run at this head	bridge-file-capabilities 2 · filesystem + server-default-bridge-wiring 34	✅ pass

Two details worth recording:

• The refactored predicate is equivalent. !inTmp && !inRepo → return became inTmp || inRepo → continue; return — De Morgan, same behaviour, and the continue is now explicit rather than relying on loop fall-through.
• The diagnostics fix is load-bearing, and I forced it. I made /var/tmp unusable inside a mount namespace (unshare -m + read-only tmpfs) to drive the branch that previously produced the silent green skip. Non-CI now prints [qwen-serve-streaming] skipping external read tests: no usable external-read fixture base (/var/tmp: Error: EROFS: read-only file system, access '/var/tmp'), and CI=1 throws with that same reason inline instead of the generic tried: /var/tmp. A genuine environment gap and a bug in the helper are no longer indistinguishable — which is exactly what let the original defect ship green.

Everything from my previous comment still stands: BEFORE reproduces #8618, AFTER fixes the read path, writes and HTTP reads remain fail-closed at WorkspaceFileSystem, and all four mutants on the production wiring are killed. The documented tradeoffs (post-approval symlink reads no longer hit WFS symlink rejection; the write/edit family still reproduces #8618) are unchanged and owned by the design doc.

The integration-tests/ typecheck blind spot from §3 of my earlier comment is a repo-level gap, not this PR's — worth a separate follow-up.

LGTM — good to merge.

中文版

已在 297dea9 重新验证 —— 阻塞项已解除，LGTM

沿用上一轮相同的 Linux 环境（真实 qwen serve daemon + 真实 qwen --acp 子进程、mock OpenAI、通过 QWEN_CLI_ENTRY 抓取 ACP stdio）。git diff <上一个 head>..297dea9 -- packages/ docs/ 为空，说明我此前验证过的生产改动逐字节未变；本轮重点复核测试修复，并重跑相关套件。

项目	观测结果	结论
第 56 行声明了 REPO_ROOT	tsc 干净 —— 无 TS2304，也不再有被吞掉的 ReferenceError	✅ 已修复
第 577 行 workspaceCwd: workspaceDir	不再出现 400 workspace_mismatch，会话按绑定的 workspace 正常创建	✅ 已修复
空 catch {} → 逐候选项收集诊断信息	跳过时与 CI 抛错时都会带出真实原因	✅ 已修复
开发机上跑完整 qwen-serve-streaming.test.ts	5 passed (5) —— 此前是 5 个静默跳过	✅ 全绿
同一文件加 CI=1	1 passed / 4 skipped —— 此前是 Failed Suites 1 / no tests	✅ 全绿
牙齿：修复后的测试对 BEFORE 构建	以 AssertionError: expected false to be true 失败	✅ 有牙齿
在新 head 上重跑单测	bridge-file-capabilities 2 · filesystem + server-default-bridge-wiring 34	✅ 通过

两点值得记录：

• 重构后的判断条件是等价的。 !inTmp && !inRepo → return 改成了 inTmp || inRepo → continue; return —— 德摩根律，行为一致，而且 continue 现在是显式的，不再依赖循环自然落空。
• 诊断信息这一处修复是实打实起作用的，我强制触发了它。 我在 mount namespace 里（unshare -m + 只读 tmpfs）把 /var/tmp 变成不可用，以驱动此前产生「静默绿色跳过」的那条分支。非 CI 场景现在会打印 [qwen-serve-streaming] skipping external read tests: no usable external-read fixture base (/var/tmp: Error: EROFS: read-only file system, access '/var/tmp')，CI=1 场景则把同样的原因内联到抛出的错误里，而不再是笼统的 tried: /var/tmp。真实的环境缺失与该 helper 自身的 bug 从此可以区分 —— 而这正是原缺陷得以以绿色状态混过去的原因。

上一条评论中的其余结论依然成立：BEFORE 复现 #8618，AFTER 修复读取路径，写入与 HTTP 读取仍在 WorkspaceFileSystem 处失败关闭，生产代码接线的 4 个变异体全部被杀死。既定取舍（批准之后的 symlink 读取不再经过 WFS 的 symlink 拒绝；write/edit 家族仍复现 #8618）未变，且由设计文档统一持有。

上一条评论第 3 节提到的 integration-tests/ typecheck 盲区属于仓库级问题，与本 PR 无关，建议单独跟进。

LGTM —— 可以合并。

[doudouOUC]

Thanks @wenshao. One thing in there I want to call out, because it is better than what I did: you forced the diagnostics branch with unshare -m + a read-only tmpfs instead of trusting that the new code path works. I verified the fixed test now runs; you verified the failure path now reports. That is the harder half, and it is the exact half whose absence let the original defect ship green — a bug in the helper and a genuine missing /var/tmp were indistinguishable, and now they are not.

Agreed on the De Morgan rewrite being the load-bearing detail worth recording: the continue is explicit now rather than the loop falling through a failed if, which is what made the old shape easy to misread.

And agreed the integration-tests/ typecheck blind spot is repo-level and belongs in its own follow-up, alongside the TS5063 I noted — the config cannot be run at all today, so the first task there is making it parse, not wiring it into CI.

Current state for anyone reading later: the only standing gate is qwen-code-ci-bot's CHANGES_REQUESTED, whose stated grounds were the skipped macOS/Windows Test jobs. Those are if: github.event_name == 'merge_group' at .github/workflows/ci.yml:698 — they run in the merge queue by design and report under their exact names, so the required checks are satisfied. @yiliang114's two approvals were dismissed automatically by the review-fix pushes.

中文说明

感谢 @wenshao。其中有一点我想特别指出，因为它比我做得更好：你用 unshare -m + 只读 tmpfs 强制驱动了诊断分支，而不是默认新代码路径可用。我验证的是「修复后的测试确实会跑」，你验证的是「失败路径确实会报」——后者才是更难的一半，而恰恰是这一半的缺失让原缺陷以绿色状态混了过去：helper 自身的 bug 与真实缺失 /var/tmp 此前无法区分，现在可以了。

同意德摩根改写是值得记录的关键细节：continue 现在是显式的，不再依赖 if 未命中后自然落空，而后者正是旧写法容易被误读的原因。

也同意 integration-tests/ 的 typecheck 盲区属于仓库级问题，应与我提到的 TS5063 一并放进单独的 follow-up —— 该配置今天根本跑不起来，所以第一步是让它能被解析，而不是先接进 CI。

供后续查阅的当前状态：唯一仍然生效的门禁是 qwen-code-ci-bot 的 CHANGES_REQUESTED，其理由是 macOS/Windows 的 Test job 被跳过。这些 job 在 .github/workflows/ci.yml:698 带有 if: github.event_name == 'merge_group'，按设计只在 merge queue 中运行，并以各自的完整名称上报，因此 required checks 是满足的。@yiliang114 的两次批准已被 review 修复推送自动 dismiss。

[wenshao]

@qwen-code /triage

[qwen-code-ci-bot]

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: 802 passed · 0 failed · 802 total

中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）

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

脚本断言：802 通过 · 0 失败 · 802 总计

Verification report

PR 8620 Deep Verification (round 2) — fix(serve): Allow approved same-host text reads outside workspace

Verdict: findings — 802/802 scripted assertions passed (fail: 0); one Low documentation finding (a security-relevant design doc asserts a neutralization mechanism that does not exist in code — the actual security property holds and is A/B proven). No behavioral blockers.
Verified head: 297dea9fde67020c37d016aa8c1c3968de7ca475 (HEAD^2, matches the metadata snapshot's headRefOid). Base: 650e085fec82fe892ece710b144dc35e6f890768 (HEAD^1). Merge commit: 1250a20.

中文摘要

• 结论: findings。802 条脚本化断言全部通过（fail: 0），但发现 1 个 Low 级文档问题：新设计文档声称"daemon 通过把子进程的 QWEN_ACP_LOCAL_READ_ROOTS 置空来中和 env 注入的本地读根"，实际产品代码没有任何地方设置或清洗该变量（唯一的置空在集成测试夹具里）。真正的防护——capability 关闭时的早退使重试路径不可达——是成立的，且已被本轮 A/B 行为证明。建议按 Findings F1 改一句话。无行为阻塞项。
• 上一轮状态: 上一轮（head 63a6cc4a，单提交）判定 merge-ready、零 findings。本轮 head 前进到 297dea9（5 个提交：两轮评审回应 + 两次合并 main + 修复中央集成测试本身）。前一轮所有测量在新 head 上全部重测：中心 A/B 成立、边界检查成立、无恶化项（见状态表）。
• A/B 结论（新 head 重测）: 中心改动仍是 load-bearing。base（650e085，重建 + 重新打包、纯度过检）上 PR 自带的集成测试在"批准内容断言"（test:668，sentinel 未进入任何模型请求）3/3 次失败，且同文件其余 4 个流程在 base 上全绿（harness 健康）；head 上 5/5 全绿，外部读取流程真实执行（无 skip）。见 01-ab-base-cell-approved-external-read-fails.png、02-ab-head-cell-approved-external-read-passes.png。
• 新提交 297dea9 本身被证明 load-bearing: 重建其中间坏态（REPO_ROOT 未声明、外部读会话绑到 REPO_ROOT）后两个后果都复现——CI 模式下模块级抛错导致整个文件收集失败（Tests no tests，exit 1）；开发机模式下 ReferenceError 被吞、测试以绿色 skip 静默失效（exit 0）。而在新 head 上同一流程真实运行并通过。见 05-broken-state-repro-fix-load-bearing.png。
• 变异矩阵（新 head，6/6 杀死，对照绿）：capability hunk、新增的默认值解析常量（?? true→?? false 镜像变异）、3+1 个 wiring 点、"写条件误绑读 capability"、"删读早退"（阳性对照）全部按预期断言被杀；拆分的读/写测试被证明互不牵连（变异写半只红写测试，读覆盖不动）。见 03-mutation-matrix-all-killed-with-attribution.png。
• 门禁: 767 个定向单测全绿（acp-bridge 481 含 bridge.test.ts 479；cli 286 含 adapter 21、run-qwen-serve 231）；HTTP fs 路由零改动；4/4 构造点接入；无死开关。见 04-targeted-gates-and-boundary-checks.png。
• 未覆盖: Windows；仓库全量套件；docker/podman sandbox 模式；真实 stdio 上 initialize 帧的逐字节抓包（以 wire-payload 单测断言 + 真实 stdio 端到端 A/B 双机制覆盖）；base 侧 reject_once 子场景（allow_once 先行失败，符合预期）。

Previous-round finding status

Round 1 (head 63a6cc4a, then a single commit) returned merge-ready with zero findings — so there are no findings to carry forward; the table below tracks round-1's measurements at the new head, per the follow-up re-measurement rule. Nothing was carried forward by shortcut: bridge.ts changed shape since round 1 (the delegation default moved from an inline ?? true at the initialize site to a const resolved once beside its siblings) and the integration test changed materially, so the input closure of every round-1 measurement moved; all were rebuilt and re-run.

#	Round-1 measurement	Status at head 297dea9
1	Central A/B: base red on approved-content assertion, head green	Stands — re-measured: base fails 3/3 at test:668, head 5/5 green (A/B table below)
2	Mutation matrix 3/3 killed	Stands, extended — mutants re-derived for the new code shape; 6/6 killed including two new surfaces (default resolver, split write test)
3	Boundary notes: write side delegated + pinned, 4/4 construction sites, HTTP routes untouched, no dead switch	Stands — each re-checked at the new head (Targeted gates below)
4	"Linux untested" gap in the PR description	Stands (gap filled again) — round 2 also ran end to end on Linux

Central claim + A/B (re-measured at the new head)

Central claim: under qwen serve, a read_file of a host text file outside the registered workspace — rejected by the workspace filesystem even after the CLI permission flow approved it — now succeeds, because daemon-owned bridges advertise clientCapabilities.fs = { readTextFile: false, writeTextFile: true } at ACP initialize, moving text reads to the child's regular CLI filesystem service while final ACP writes stay delegated.

Cell	Environment	Oracle	Result
base	worktree at 650e085, full npm run build + npm run bundle (exit 0); purity-checked (grep -r delegateReadTextFileToClient over base dist/ = 0 hits; head dist/ = 3 files); internal workspace links re-pointed into the base tree and realpath-asserted; PR's test file copied verbatim (cmp clean); helpers byte-identical on both sides	same file, same command as head	1 failed | 4 passed — external-read test red 3/3 attempts (vitest retry: 2) at test:668 expect(modelRequests.some(includes(sentinel))).toBe(true) → expected false to be true, invoked from runExternalRead('allow_once') at test:711. The 4 pre-existing flows all green on base ⇒ harness healthy; permission request fired and was approved (≥2 model requests observed) yet the sentinel never reached any model request
head	pre-built CI tree at merge 1250a20 (dist/cli.js bundle contains the option)	approved content reaches ≥2 model requests + final response; rejected content reaches none, failed tool update emitted	5/5 passed — external-read flow executed (zero skip warnings), both allow_once and reject_once sub-scenarios green

Witnesses: 01-ab-base-cell-approved-external-read-fails.png, 02-ab-head-cell-approved-external-read-passes.png; raw logs logs/ab-base-cell.log, logs/ab-head-cell.log. The base failure is the intended control outcome (counted as a passed expectation); it fails on the exact behavioral assertion, and the other 4 flows passing on base proves the failure is specific to the external read, not the harness.

One environmental note: the first attempt to run both arms concurrently killed both vitest processes with zero output (four vitest instances racing at once); each arm re-run in isolation completed normally. The concurrent-death is a harness artifact of this verification run, not a property of the PR.

What the delta since round 1 adds, and whether it holds

Commit 297dea9 (the new head) repairs the central regression test itself. Reconstructed the broken intermediate state (declaration of REPO_ROOT removed, external-read session bound to REPO_ROOT — two undeclared references, matching the commit message) and drove it both ways:

Mode	Predicted by commit message	Reproduced
CI (CI=true)	loud-fail throws at module scope → file fails collection, pre-existing tests go down with it	yes — Test Files 1 failed (1), Tests no tests, exit 1, diagnostic no usable external-read fixture base (/var/tmp: ReferenceError: REPO_ROOT is not defined) (logs/broken-repro-ci-mode.log)
dev box (CI unset)	ReferenceError swallowed per candidate → green skip, silently disabled security test	yes — Test Files 1 passed (1), `2 passed

At head, the identical scenario runs and passes (head arm above) — the fix is load-bearing, and the rejection-reason collector added on the way now surfaces the real cause in both branches instead of a bare catch. (First reconstruction attempt was unfaithful — a String.replace hit the SIGKILL flow's binding first; corrected with a unique anchor before drawing any conclusion. The CI-mode cell is unaffected by that slip since the module-scope throw fires before any test body.) Witness: 05-broken-state-repro-fix-load-bearing.png.

Split capability tests (filesystem.test.ts): commit message claims "the combined capability case is split so deleting the write half cannot silently remove read coverage." Proven by the mutant pair D/E: keying writeTextFile's delegation condition on the read capability turned only "keeps writes delegated…" red (expect(client.writeTextFile).toHaveBeenCalledWith(...) never satisfied), while the read test stayed green — and the mirror mutant (read early-return deleted) turned only the read test red. The halves are independently pinned.

Refactored default resolver (bridge.ts): the default is now resolved once (opts.delegateReadTextFileToClient ?? true) and used at the initialize site. Mutant A2 (?? true → ?? false) kills the default-path test while the opt-out test stays green; mutant A (initialize hunk forced to true) does the mirror — both ends of the single resolution are pinned, so the refactor did not create a drift surface.

Mutation matrix at head 1250a20

Unmutated controls green first (capabilities, filesystem, run-qwen-serve + server-default-bridge-wiring). Witness 03-mutation-matrix-all-killed-with-attribution.png; rerunnable via mutation-matrix.sh (+ the run-3 repair noted in Methodology); logs logs/mut-*.log.

Mutant	Suite	Result	Failure mode
A: initialize hunk → readTextFile: true	bridge-file-capabilities	killed	opt-out test red; default test stays green (correct attribution)
A2: resolver ?? true → ?? false	bridge-file-capabilities	killed	default test red; opt-out stays green (mirror)
B: 3× delegateReadTextFileToClient: false deleted in run-qwen-serve.ts	run-qwen-serve	killed (both wiring tests)	expected {…} to match object { delegateReadTextFileToClient: false }
C: wiring line deleted in server.ts	server-default-bridge-wiring	killed	same shape
D: write delegation condition keyed on read capability	filesystem	killed	only the write test red — split isolation holds
E: read-capability early return deleted (positive control)	filesystem	killed	only the read test red

Survivors: none. Positive control (E) landed beside the others, so "suite can fail" is proven, not assumed.

Targeted gates + boundary checks (re-measured)

• Unit gates, head (witness 04-targeted-gates-and-boundary-checks.png): bridge-file-capabilities.test.ts 2 ✓, bridge.test.ts 479 ✓ (bridge.ts changed shape, so its own suite gates the refactor), filesystem.test.ts 32 ✓, bridge-file-system-adapter.test.ts 21 ✓, server-default-bridge-wiring.test.ts 2 ✓, run-qwen-serve.test.ts 231 ✓ — 767/767. Build = typecheck: full npm run build completed at HEAD (CI) and at base (this round).
• Reviewer Test Plan: step 1 (default {true,true} vs daemon-owned {false,true}) — capabilities + wiring tests, mutants A/A2/B/C; step 2 (real daemon/ACP child, allow_once/reject_once) — the A/B itself; step 3 (asymmetric service: reads local, writes delegated) — split filesystem tests, mutants D/E; step 4 (HTTP reads outside workspace + external/untrusted/symlinked delegated writes still rejected) — adapter suite 21/21 + zero diff under packages/cli/src/serve/fs/.
• Construction sites: exactly 4 production createAcpSessionBridge call sites (grep); all 4 set the flag (server.ts:978, run-qwen-serve.ts:3911/4311/4836); runtime.createAcpSessionBridge is a pass-through alias of the same function (run-qwen-serve.ts:1157), no option filtering.
• No dead switch: one declaration, one functional read site (bridge.ts resolver + its single use), four set sites.
• Tradeoff single-owner claim: the design doc's numeric caps (256 KiB / 8 MiB) appear only in the design doc itself and in the WFS subsystem's own reference doc/tables (07-workspace-filesystem.md, qwen-serve-protocol.md, etc. — pre-existing owners of the WFS constants, not copies of the tradeoff list); the three PR-touched sibling docs reference the design doc instead of restating numbers. Claim holds within its stated scope.

Corrections

None to round 1 (its descriptions of the code checked out again this round). One correction to this round's own first attempt, recorded for auditability: the initial broken-state reconstruction mutated the wrong test's workspaceCwd (non-anchored String.replace), which produced a spurious SIGKILL-flow failure; it was redone with a unique anchor before being used as evidence.

Findings

F1 (Low, docs): the design doc asserts a daemon neutralization that no code performs. docs/design/daemon-local-text-reads.md ("Compatibility"): "The daemon neutralizes the env-supplied half by setting QWEN_ACP_LOCAL_READ_ROOTS empty for the child." Measured facts: the string appears nowhere in packages/cli/src/serve/ or packages/acp-bridge/src/; it is not in SCRUBBED_CHILD_ENV_KEYS (spawnChannel.ts:262 — only QWEN_SERVER_TOKEN, QWEN_CODE_SIMPLE, the tool-guard token); the only place anything sets it empty is the integration test fixture (qwen-serve-streaming.test.ts:275). In production, an operator's QWEN_ACP_LOCAL_READ_ROOTS passes through to the child via the runtime env. Security consequence today: none — the local-read retry that consumes those roots sits below the capability check, which returns first when the capability is off (filesystem.ts early return), and all four daemon construction sites turn it off; the doc's next paragraph says exactly this and the A/B proves the behavior. But the sentence describes a defense that does not exist, in the security section of the doc that is now the single owner of this tradeoff — a future reader (or a future change re-enabling delegation) could rely on it. Suggested one-line fix, measured against the code facts above: replace the sentence with "The daemon's protection against the env-supplied half is the capability itself: with delegation off, the retry that consults those roots is unreachable (below); nothing scrubs the variable, so a generic ACP host that keeps delegation on inherits the operator's roots by design." Doc-only change; no suite moves either way, which is the unpinned-axis signal — if the maintainer wants it pinned, the fixture would need an assertion that the child env carries the variable through, and that is a test of absence-of-scrubbing, recommended only if the sentence is kept in any "the daemon manages this variable" form.

Not covered

• Windows (PR marks it untested; this round is Linux, same as round 1).
• Repository-wide test suite and standalone repo-wide typecheck — targeted suites + the full CI build at HEAD stand in.
• Sandbox modes (QWEN_SANDBOX=docker/podman); the suite skips there by design (child cannot reach host loopback) and ran with QWEN_SANDBOX=false.
• Byte-level capture of the initialize JSON-RPC frame on real stdio — wire payload asserted via the in-memory channel in the capabilities tests, and the end-to-end A/B proves the child behaved as readTextFile: false over real stdio; no raw frame dump.
• Base-side reject_once sub-scenario: unreachable because allow_once fails first; rejection was already safe on base (reads always denied), so no behavior is unmeasured.
• Per-commit attribution: the checkout is depth 2 (git rev-parse --is-shallow-repository = true); only the merge commit, base tip, and PR head are reachable locally. The intermediate four commits were verified as the aggregate HEAD^1..HEAD diff; their individual boundaries (which review-round change landed in which commit) were not separable. The metadata snapshot's 5-commit commits array matches HEAD^2 and the aggregate diff content.
• The fix(serve): align same-host daemon text reads with CLI permissions #8618 write/edit-family residual failure is documented by the PR as out of scope ("What this does not fix"); the pinned write-side behavior (adapter rejects external writes) is consistent with that claim, but the end-to-end write repro was not driven.

Methodology

Environment: CI verify container (node:22-bookworm, Node v22.23.2), working tree = refs/pull/8620/merge at depth 2, npm ci + npm run build completed pre-verification; /var/tmp present and writable (the fixture base the test probes). A/B: base worktree at HEAD^1 rebuilt (npm run build + npm run bundle, exit 0) with root node_modules shared via symlinks and the @qwen-code/* workspace links recreated as relative links inside the base tree (realpath asserted into tmp/base-tree/packages/*); nested per-package node_modules mirrored from head (lockfile untouched by the PR, verified by diffstat). Dependency state is therefore a clean control. The PR's test file was copied verbatim (cmp clean) and run through the base bundle; the identical command ran against the head bundle. Mutation matrix ran in a second scratch worktree at HEAD with the same link recipe; its first pass had red controls because worktrees lack built dist/ and generated files — repaired by symlinking HEAD's per-package dist/ and src/generated dirs (mutated files are transformed from src by vitest and were never served from the shared dists), after which controls went green and only then were the B/C mutants trusted. Broken-state repro used a faithful reconstruction (unique-anchor replacement verified by grep: exactly 2 undeclared REPO_ROOT references) as a temporary test file, deleted afterwards (working tree confirmed clean). Assertion counting convention: one vitest test case = one scripted assertion unit (each fails if any contained expect fails), plus one unit per scripted shell/expectation check (realpath, purity greps, repro outcomes, control-cell predictions); raw per-cell logs in logs/, harness scripts in mutation-matrix.sh / make-broken-repro.mjs, evidence PNGs rasterised with scripts/verify-capture.mjs.

Evidence images

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

— Qwen Code · sandboxed verification

[qwen-code-ci-bot]

LGTM, looks ready to ship. ✅

[qwen-code-ci-bot]

Released in v0.21.8.