fix(serve): Allow approved external built-in text writes - #8852
Conversation
E2E test reportEnvironment: macOS, Node.js 24.12.0, repository package 0.21.8, locally built Baseline with the global 0.21.8 CLI:
Verification with this PR:
The deterministic assertions use tool completion status plus exact file readback, not model-input content, so the tool input itself cannot produce a false positive. |
github-actions
Bot
added
the
review/self-reported
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. This is the documented write-side follow-up to the already-merged read-side alignment (#8620, merged Aug 7 by the same author). The merged read-side design doc explicitly tracked this exact failure — "#8618 therefore still reproduces for the Direction: aligned. Same-host Size: core paths are touched ( Approach: the scope still feels right, and the two changes since the last review are exactly what the last review asked for. The marker helpers moved out of Risk: Stage 1e matched the same two high-risk paths as before — Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 已观测到的真实问题，不是理论性加固。这是已合并的读侧对齐（#8620，同一作者，8 月 7 日合并）在写侧的文档化后续。已合并的读侧设计文档明确记录了这个失败——"#8618 therefore still reproduces for the 方向： 对齐。同机 规模： 触及核心路径（ 方案： 范围依然合理，且上次审查以来的两处改动正是上次审查所要求的：标记 helper 从 风险： Stage 1e 命中与上次相同的两个高风险路径—— 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-reviewed the full diff at the rebased head. My independent proposal is unchanged from the previous pass — versioned in-band provenance attached only to final tool writes, strict fail-closed parsing, daemon-owned opt-in only, and a guarded host writer that re-validates every WFS invariant per request, rejecting the allowlist-at-grant-time alternative because of expiry/invalidation/race coupling — and the PR still matches it, including the details that exceeded it (inode-stability race test, BOM de-duplication across UTF-16, audit cardinality aligned with The only code delta since the last review is the bundle-guard fix, and it is exactly the fix the review prescribed. The marker helpers left The properties that matter for a trust-boundary change, re-verified on this SHA:
One standing non-blocking nit for the maintainer's awareness: FlowsequenceDiagram
participant P1 as Built-in tool
participant P2 as AcpFileSystemService
participant P3 as Daemon bridge adapter
participant P4 as WorkspaceFileSystem
participant P5 as Guarded host writer
P1->>P2: final writeTextFile with toolWriteOrigin
P2->>P2: strip caller marker, add versioned marker
P2->>P3: ACP writeTextFile
alt workspace target
P3->>P4: resolve then writeTextOverwrite (unchanged)
else external, valid marker, daemon-owned opt-in
P3->>P5: trust, generation, canonical lock, atomic rename
P5-->>P3: one audit outcome
else unmarked or malformed
P3->>P4: ordinary route, fails closed
P4-->>P3: path_outside_workspace
end
Files changed (30 of 31 shown)
Testing evidence — the PR's own CI at the reviewed commit (unattended run; no PR code executed here)The previous pass's merge blocker is cleared on this commit. The required The red on the old SHA was this PR's own import edge; the green on this SHA is that edge removed. Serve A/B re-ran against the new head and saw no response changes across 4 scenarios; the real-daemon E2E and the SDK matrix are green. One honest gap in the CI profile:
Sandboxed verification is already in flight on this head — the Not verified: Windows/Linux behavior (author's "Tested on" table marks macOS only — the author's claim, not independently re-run; the unit suite itself ran green on ubuntu in CI), and the integration test in CI (skipped by profile — the in-flight 中文说明代码审查在 rebase 后的新 head 上重新审查了完整 diff。我的独立方案与上一轮一致（仅最终工具写入携带带版本带内来源标记、严格 fail-closed 解析、仅 daemon 自建启用、受控 host writer 每次请求重新校验全部 WFS 不变量；拒绝"授权时记录允许列表"的替代方案，因其引入过期/失效/竞态耦合）——PR 仍然与之吻合，包括超出我方案的细节（inode 稳定性竞态测试、UTF-16 的 BOM 去重、与 上次审查以来唯一的代码差异就是 bundle 守卫修复本身，而且正是审查所开的方子。 标记 helper 离开了 对信任边界改动重要的性质，已在本 SHA 重新核验：
一个长期存在的非阻塞提醒： 测试证据（本提交自带的 CI；无人值守运行，未执行任何 PR 代码）上一轮的合并阻塞已在本提交解除。必需检查 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — the previous pass's single blocker is fixed the exact way the review prescribed, the required CI including the bundle guard is green on this commit with the full suite running for the first time, and a fresh read of the diff surfaced nothing new; what remains is non-blocking nits. Stepping back: this is the stronger half of a pair the project already committed to, and it now lands the way the last pass asked. The previous verdict said: fix the import edge, get What has not changed is my read of the code itself, because the code itself barely changed — and that read was already a favorable one. The negative tests carry the weight a trust-boundary change needs: injected factories don't inherit the exception even when they implement the host writer, untrusted runtimes and dangling symlinks fail with exactly one denial, and the approve/reject/YOLO flows assert no shell fallback. The design doc and the boundary docs were updated in the same diff, which is what I'd want to maintain against in six months. The previous pass also asked for two things to travel with any verdict: sandboxed verification and a maintainer's sign-off on the security model. The The non-blocking items, named so they don't get lost: the integration profile skip means the real-daemon flow is pinned by the in-flight Approving, pinned to the reviewed commit. ✅ 中文说明置信度：4/5 —— 上一轮唯一的阻塞项已按审查开出的方子精确修复，必需 CI（含 bundle 守卫）在本提交转绿、完整测试套件首次运行，重新通读 diff 未发现新问题；剩下的都是非阻塞提醒。 退一步看：这是项目已承诺的成对工作的写侧一半，且它以上一轮要求的方式落地。上一轮结论是：修掉导入边，让 我对代码本身的判断没有变，因为代码本身几乎没变——而那个判断本来就是正面的。信任边界改动需要的分量由负面测试承担：注入 factory 即使实现 host writer 也不继承例外、不可信 runtime 与悬空 symlink 恰好一条拒绝审计、批准/拒绝/YOLO 流程断言无 shell 回退。设计文档与边界文档随同一 diff 更新——这是六个月后维护它时我会感谢作者的部分。上一轮还要求两件事与结论同行：沙箱验证与 maintainer 对安全模型的签核。 非阻塞事项，点名以免遗失：integration profile 跳过意味着真实 daemon 流程由进行中的 批准，锚定到已审查的提交。✅ — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
The design and test coverage are genuinely strong, but the required Test (ubuntu-latest, Node 22.x) check is red on this commit: the new value import of parseToolWriteOriginMeta from the @qwen-code/qwen-code-core barrel in packages/cli/src/serve/bridge-file-system-adapter.ts pulls the core tool-runtime chunk (5.45 MB, incl. glob/chokidar/@iarna/toml/fzf and shell.ts) into the serve pre-listen fast path, and check:serve-fast-path-bundle rejects it — see my Stage 2 comment for the traced mechanism and fix direction. The job failed before vitest started, so no CI test evidence exists for this commit yet. Once the import edge is fixed (leaf module or subpath export) and the suite is green — ideally with a @qwen-code /verify run pinning the allow_once/reject_once/YOLO behavior — this is in good shape to come back.
|
Diagnosed the What the gate guards: The trigger in this PR — import { parseToolWriteOriginMeta } from '@qwen-code/qwen-code-core';That's a static top-level import of the core barrel, and The gate's established fix is a dynamic import at the use site: const { parseToolWriteOriginMeta } = await import('@qwen-code/qwen-code-core');
You can reproduce locally with 中文说明诊断了本 PR 上 这个门守护什么: 本 PR 的触发点—— import { parseToolWriteOriginMeta } from '@qwen-code/qwen-code-core';这是对 core **barrel(整包入口)**的静态导入,而该文件在 serve 的 pre-listen 图里——为取一个小纯函数,把整个 core(连带 glob、toml)静态拉进了启动闭包。 该门的标准修法是用点处动态导入(见上方代码)。 本地可用 Handled with Claude Code (Opus 5, 1M context). |
|
Thanks — agreed and fixed in |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM after adversarial review at head 6829bf2. The authorization loop closes without widening any boundary: the four built-in tools attach a core-only toolWriteOrigin field only after the existing permission gate allows, AcpFileSystemService strips caller-supplied markers and serializes a strict versioned one, and the guarded host writer is reachable only from daemon-owned same-host adapters (option defaults off; injected factories verified never routed; HTTP/generic ACP paths never call writeSameHostToolText — single production call site). Marker parsing is fail-closed (exactly two own keys, version === 1 strict, 4-entry source allowlist, 7 negative cases), and the writer re-enforces trust, generation, suspicious-path, symlink/inode, size, and atomic-rename invariants under the canonical path mutex with exactly one audit outcome per path — symlink swap covered twice (inode stability during canonicalization plus fresh lstat inside the lock; final publish is rename, not write-through). The prior ci-bot blocker (fast-path bundle pulling a 5.45 MB chunk) is fixed via the dependency-light toolWriteOrigin subpath export and the bundle-gate test is green on head; Serve A/B reports zero response deltas.
P3 nits only, none blocking: toolWriteOrigin is a plain typed field on the public core request so a future in-process consumer could attach it with its own approval semantics (unexploitable across a boundary today — same-UID child already has shell and the daemon writer enforces all fs invariants — but worth a doc-level prohibition or unforgeable token); a marked write into another runtime's registered workspace bypasses that workspace's WFS context (documented tradeoff, shared lock serializes, current trust still enforced); audit success rows record canonical paths while denials record the original input; Windows/Linux untested by author (pre-existing WFS primitives, low risk). Ship it.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
6829bf2 to
f6d7456
Compare
|
Rebased onto current |
|
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 /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 1085 passed · 0 failed · 1085 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：1085 通过 · 0 失败 · 1085 总计 Verification reportPR 8852 — fix(serve): Allow approved external built-in text writesVerdict: 中文摘要
Central claim and A/BCentral claim: an approved built-in text write ( Load-bearing proof — identical scenario, two builds (head bundle vs base bundle rebuilt in a scratch worktree whose
Denial/acceptance matrix at head ( Findings (non-blocking)
Not covered
MethodologyEnvironment: Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-approving after the rewrite: it is a clean rebase onto updated main whose final security-critical tree is byte-identical to the head I approved (6829bf2) — 14 of 15 security-critical files unchanged, the one differing file (run-qwen-serve.ts) differs only by upstream #8763, and the PR's own wiring is untouched. Re-read at f6d7456: marker strip + strict fail-closed parse, single production call site of writeSameHostToolText, adapter option default-off with the three daemon-owned wiring sites, trust/generation re-checks at entry/resolve/mutex, symlink/inode/size/atomic invariants, single audit outcome, and HTTP/generic ACP never routed — all preserved. The bundle extraction commit moves the provenance block into the dependency-light toolWriteOrigin subpath (type-only ACP import) and the required Test (ubuntu) check including the serve fast-path bundle closure step is green, so the prior 5.45MB-chunk blocker stays resolved. The four carried-over P3 nits (in-process origin field forgeable by future in-repo consumers, cross-runtime workspace tradeoff, audit path asymmetry, Windows unexercised) remain non-blocking. Nothing new found; ship it.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): chunk 3: did not execute the unit tests (worktree has no installed dependencies).; chunk 3: unit-test execution of filesystem.test.ts and bridge-file-system-adapter.test.ts (no installed deps in worktree).
中文说明
未探索到全部深度（达到工具调用预算）：chunk 3：did not execute the unit tests (worktree has no installed dependencies).；chunk 3：unit-test execution of filesystem.test.ts and bridge-file-system-adapter.test.ts (no installed deps in worktree)。
— {{model}} via Qwen Code /review (v0.21.8)
| await fsp.writeFile(realTarget, 'original'); | ||
| await fsp.symlink(realTarget, linkTarget); | ||
| const adapter = createBridgeFileSystemAdapter( |
There was a problem hiding this comment.
[Critical] The new symlink tests reuse the file's existing symlink setup pattern but drop its Windows guard, so they fail during fixture setup on Windows without symlink privilege.
Failure scenario: on a Windows runner without Developer Mode or SeCreateSymbolicLinkPrivilege, fsp.symlink() rejects with EPERM before any adapter assertion runs, making required CI fail for an environment capability rather than product behavior. The pre-existing symlink test in this same file explicitly returns early on win32.
| await fsp.writeFile(realTarget, 'original'); | |
| await fsp.symlink(realTarget, linkTarget); | |
| const adapter = createBridgeFileSystemAdapter( | |
| // Skip on Windows without symlink capability, as the existing symlink tests do: | |
| if (process.platform === 'win32') return; |
— {{model}} via Qwen Code /review (v0.21.8)
| it('rejects content that exceeds the cap only after non-UTF-8 encoding', async () => { | ||
| const adapter = createBridgeFileSystemAdapter( | ||
| buildFactory({ trusted: true }), |
There was a problem hiding this comment.
[Critical] The external write path applies the byte cap to the UTF-8 length of the source string before the requested encoding is applied, so it rejects valid writes whose actually-published encoded size is within the limit.
Failure scenario (probe-verified): 1.8M 你 characters measure 5,400,000 bytes as UTF-8 but 3,600,000 bytes as GBK. A marked external write with encoding: 'gbk' is rejected as file_too_large even though the bytes that would be published fit comfortably under the 5 MiB cap; the post-encoding check in writeEncodedTextTemp is the authoritative one.
| it('rejects content that exceeds the cap only after non-UTF-8 encoding', async () => { | |
| const adapter = createBridgeFileSystemAdapter( | |
| buildFactory({ trusted: true }), | |
| // Skip the UTF-8 precheck when a non-UTF-8 encoding is requested; the | |
| // post-encoding enforcement in writeEncodedTextTemp is authoritative. |
— {{model}} via Qwen Code /review (v0.21.8)
| resolved = await resolveWithinWorkspace( | ||
| request.path, | ||
| workspaces.map((workspace) => workspace.path), | ||
| 'write', |
There was a problem hiding this comment.
[Critical] The same-host marked-write route resolves and probes the requested host path before checking whether the runtime is trusted, so an untrusted runtime's request can trigger host filesystem activity across the trust boundary.
Failure scenario: an untrusted runtime submits a provenance-marked write to an external absolute path; resolveWithinWorkspace calls realpath and walks existing ancestors before writeSameHostToolTextOutsideWorkspace reaches assertTrustedForIntent, which can expose permission/path distinctions or trigger automount/FUSE I/O for a request that should be rejected immediately.
| resolved = await resolveWithinWorkspace( | |
| request.path, | |
| workspaces.map((workspace) => workspace.path), | |
| 'write', | |
| // Assert trust at the top of writeSameHostToolText, inside the audited | |
| // try/catch, before any filesystem resolution runs. |
— {{model}} via Qwen Code /review (v0.21.8)
| const target = await resolveSameHostToolWriteTarget(deps.request.path); | ||
| await deps.pathLocks.runExclusive(target, async () => { | ||
| deps.generationGuard?.assertOpen(); |
There was a problem hiding this comment.
[Critical] The external writer canonicalizes the target before acquiring an in-process-only lock, and publication still resolves through pathnames, leaving an ancestor symlink/junction replacement race that can redirect the write.
Failure scenario: after resolveSameHostToolWriteTarget resolves the parent, another same-host process can rename that directory and replace its pathname with a symlink or junction before the temporary file is created and renamed. The later checks validate the replacement directory and publish there, so bytes land outside the originally resolved directory while the audit records success against the stale canonical path.
| const target = await resolveSameHostToolWriteTarget(deps.request.path); | |
| await deps.pathLocks.runExclusive(target, async () => { | |
| deps.generationGuard?.assertOpen(); | |
| // Bind temp-file creation and publication to an opened canonical parent | |
| // handle (openat/renameat-style) with inode revalidation immediately before | |
| // publish; or refuse external writes under replaceable parent directories. |
— {{model}} via Qwen Code /review (v0.21.8)
| const result = await atomicWriteTextResolvedFile({ | ||
| target, | ||
| content, | ||
| mode: 'overwrite', |
There was a problem hiding this comment.
[Critical] The external overwrite path replaces the target inode preserving the mode but not the uid/gid, so a successful write can revoke the original owner's access.
Failure scenario: a root-run daemon edits an existing service-account-owned 0600 file outside the workspace; the temporary inode is created as the daemon user, only the mode is copied, and the rename publishes a root-owned file the service account can no longer read — while the existing inline ACP writer explicitly restores uid/gid.
| const result = await atomicWriteTextResolvedFile({ | |
| target, | |
| content, | |
| mode: 'overwrite', | |
| // Snapshot the existing target's uid/gid with its mode and apply them to the | |
| // temp handle before publication, best-effort where supported; otherwise | |
| // reject replacing an inode whose ownership cannot be retained. |
— {{model}} via Qwen Code /review (v0.21.8)
| toolCalls: [ | ||
| fakeToolCall('write_file', { | ||
| file_path: pendingExternalWritePath, |
There was a problem hiding this comment.
[Suggestion] The real-daemon E2E exercises only write_file end to end, although the new external-write route is also documented for edit, notebook_edit, and the shell tool's simulated sed editor.
Failure scenario: a provenance or wiring regression specific to one of those three consumers (each is a separate final-write call site with its own toolWriteOrigin) passes the workspace suites and the write_file-only daemon scenario, so users still hit path_outside_workspace for a documented capability.
| toolCalls: [ | |
| fakeToolCall('write_file', { | |
| file_path: pendingExternalWritePath, | |
| // Parameterize the daemon scenario over all four supported built-in write | |
| // paths, at least proving a successful external commit for each. |
— {{model}} via Qwen Code /review (v0.21.8)
| events.some((event) => event.type === 'permission_request'), | ||
| ).toBe(false); | ||
| } | ||
| expect(serializedEvents).not.toContain('"toolName":"shell"'); |
There was a problem hiding this comment.
[Suggestion] The "no shell fallback" assertion is vacuous because the fake model is hard-coded to return external write completed after the first tool result and can never request shell.
Failure scenario: if a production model retries a rejected or failed external write_file through shell, this test still passes — once any tool result exists, no shell call can ever be emitted — so the assertion does not verify the behavior its title claims.
| expect(serializedEvents).not.toContain('"toolName":"shell"'); | |
| // Make the fake server request a shell fallback when the write result | |
| // indicates failure, then assert the intended per-mode policy. |
— {{model}} via Qwen Code /review (v0.21.8)
| it('keeps a marked workspace write on the existing WFS path', async () => { | ||
| const adapter = createBridgeFileSystemAdapter( | ||
| buildFactory({ trusted: true }), |
There was a problem hiding this comment.
[Suggestion] The "keeps a marked workspace write on the existing WFS path" test asserts only outcomes that are identical under both routing branches, so it cannot detect a rerouting regression.
Failure scenario (mutation-probe-verified): forcing writeSameHostToolText to always throw path_outside_workspace — simulating every marked write routed externally — leaves this test green. A real regression of that shape would silently lose existing-file encoding/BOM/line-ending preservation for marked workspace writes.
| it('keeps a marked workspace write on the existing WFS path', async () => { | |
| const adapter = createBridgeFileSystemAdapter( | |
| buildFactory({ trusted: true }), | |
| // Assert the external writer is NOT taken for a workspace target (e.g. pin | |
| // encoding meta or audit shape that only the WFS branch preserves). |
— {{model}} via Qwen Code /review (v0.21.8)
| } | ||
|
|
||
| try { | ||
| await forRequest(ctx).writeTextOverwrite(resolved, request.content); |
There was a problem hiding this comment.
[Suggestion] The in-workspace branch of writeSameHostToolText drops the sanitized encoding metadata, while the sibling outside-workspace branch honors it — the same marked tool call writes different bytes depending on where the target resolves.
Failure scenario (probe-verified): overwriting an existing GBK file with encoding: 'gbk' through a marked write produces corrupted bytes in-workspace (3f8d44) but the correct GBK bytes externally (c4e3bac3); on read failure (large/binary/EACCES) the in-workspace branch falls back to UTF-8/LF, breaking the very scenario #8618 targets.
| await forRequest(ctx).writeTextOverwrite(resolved, request.content); | |
| // Forward the sanitized request meta (bom/encoding/lineEnding) into | |
| // writeTextOverwrite in the in-workspace branch, as the external branch does. |
— {{model}} via Qwen Code /review (v0.21.8)
Local verification on a real stack — PR 8852I rebuilt this PR and its merge base into two independent bundles and ran both against a real Verdict: works as advertised, boundaries hold. LGTM from a verification standpoint. Three non-blocking notes for reviewers at the end. Setup
Bundle sanity: A/B: the authorization loop actually closesSame scripted model, same prompt, same approval action — only the bundle differs.
New files land as Screenshots — real Web Shell UI, identical user action1. Head — the approval request for a path outside the bound workspace
2. Head — after
On-disk readback: 3. Base — same prompt, same approval:
Security boundaries (all on head)
Repo tests
Notes for reviewers (non-blocking)1. The provenance marker is visible on the wire to third-party ACP clients. I ran the built CLI as a plain ACP agent ( // head
"_meta": { "bom": false, "qwen-code/tool-write-origin": { "version": 1, "source": "write_file" } }
// base
"_meta": { "bom": false }It is inert for a generic client (their own fs policy still decides), which matches the PR's framing. But an editor integration such as Zed now sees a new vendor 2. The external writer does not recover existing-file metadata from disk. 3. The opt-in asymmetry in Residual risk already acknowledged in the PR description (approval→write path races on parent directories) is unchanged by this verification; I did not attempt to exercise it. 中文版本PR 8852 本地真实环境验证我把本 PR 与其 merge base 分别构建成两套独立 bundle，各自跑真实 结论：功能符合描述，边界稳固，从验证角度 LGTM。 文末有三条不阻塞的提示。 环境
Bundle 校验： A/B：授权链路确实闭合同一脚本化模型、同一 prompt、同一批准动作，只有 bundle 不同。
新文件以 截图 —— 真实 Web Shell UI，相同用户操作1. Head —— 针对 workspace 外路径的批准请求
2. Head —— 点击
磁盘回读： 3. Base —— 同样的 prompt、同样的批准：
安全边界（均在 head 上验证）
仓库测试
给 reviewer 的提示（不阻塞合并）1. provenance 标记对第三方 ACP 客户端是可见的。 我用最小 stdio 客户端拉起 // head
"_meta": { "bom": false, "qwen-code/tool-write-origin": { "version": 1, "source": "write_file" } }
// base
"_meta": { "bom": false }对通用客户端它是惰性的（对方仍由自己的 fs 策略决定），这与 PR 的定位一致。但 Zed 等编辑器集成此后会在每次内置写入中看到一个新的厂商 2. 外部 writer 不会从磁盘恢复已有文件的元数据。 3. PR 描述中已承认的遗留风险（批准到写入之间父目录的路径竞态）不在本次验证范围内，我没有尝试构造。 |
|
Released in v0.21.9. |
4 participants
What this PR does
This PR lets daemon-owned same-host runtimes complete already-authorized external text writes from the built-in
write_file, edit, notebook edit, and controlled shell sed edit paths without disabling ACP text-write delegation or widening HTTP and generic ACP workspace boundaries. Final built-in writes carry strict versioned internal origin metadata, and only daemon-owned adapters route valid outside-workspace writes to a guarded host writer. Workspace-internal writes continue through the existing workspace filesystem path.The guarded external writer preserves runtime trust and generation checks, canonical-path locking, regular-file and symlink validation, atomic replacement, existing mode preservation or
0600for new files, encoding metadata, the encoded 5 MiB limit, and one success or denial audit result.Why it's needed
In same-host
qwen serve, approving an external built-in text write only passed the core tool permission check. The final delegated ACPwriteTextFilerequest was still rejected by the workspace filesystem withpath_outside_workspace, so the built-in tool failed and the model could retry the same operation through shell. This closes that authorization loop while keeping delegated writes and the existing daemon filesystem safety boundary.Reviewer Test Plan
How to verify
write_fileto write the external absolute path, and approve the exact built-in tool request. Expect the tool to complete, the target content to match exactly, and no shell call.path_outside_workspace; also verify untrusted, stale-generation, oversized, special-file, and external leaf-symlink writes remain denied.Evidence (Before & After)
Before: the deterministic real-daemon flow reported
write_fileas failed withpath escapes workspaceafterallow_once; YOLO failed at the same final ACP write, and the target file was absent.After: the deterministic real-daemon/fake-model test completes
allow_onceand YOLO writes with exact file readback and no shell fallback;reject_onceissues no final write and leaves the target absent. Core tests pass 564/564, CLI tests pass 336/336, the targeted integration test passes, and full build and typecheck pass.Tested on
Environment (optional)
macOS, Node.js 24.12.0, npm workspace install with the repository Ink patch, locally built
dist/cli.js, and a fake OpenAI server driving a realqwen serve/ACP child.Risk & Scope
Linked Issues
Closes #8851
Related to #8618 and #8620.
中文说明
本 PR 做了什么
本 PR 允许 daemon 自建的同机 runtime 完成已经通过授权的外部文本写入，覆盖内置
write_file、edit、notebook edit 和受控 shell sed 编辑路径，同时不关闭 ACP 文本写入委托，也不放宽 HTTP 和通用 ACP 的 workspace 边界。内置工具的最终写入会携带严格、带版本的内部来源元数据，只有 daemon 自建 adapter 才会把带有效标记的 workspace 外写入路由到受控 host writer；workspace 内写入继续走现有 workspace filesystem 路径。受控外部 writer 保留 runtime trust 与 generation 检查、canonical path 锁、普通文件与 symlink 校验、原子替换、已有文件 mode 保留或新文件使用
0600、编码元数据、编码后 5 MiB 上限，以及单个成功或拒绝审计结果。为什么需要
在同机
qwen serve中，批准外部内置文本写入只会通过 core 工具权限检查。最终委托的 ACPwriteTextFile请求仍会被 workspace filesystem 以path_outside_workspace拒绝，因此内置工具失败，模型还可能通过 shell 重试同一操作。本改动闭合该授权链路，同时保留委托写入和现有 daemon 文件系统安全边界。Reviewer 测试计划
如何验证
write_file写入外部绝对路径，并批准准确的内置工具请求。预期工具完成、目标内容精确一致，且没有 shell 调用。path_outside_workspace；同时验证 untrusted、过期 generation、超限、特殊文件和外部叶子 symlink 写入仍被拒绝。证据（改动前后）
改动前：确定性的真实 daemon 流程在
allow_once后仍把write_file报告为失败，错误为path escapes workspace；YOLO 也在同一个最终 ACP 写入处失败，目标文件不存在。改动后：确定性的真实 daemon/fake-model 测试中，
allow_once和 YOLO 写入均完成，文件回读内容精确一致且没有 shell fallback；reject_once不发起最终写入并保持目标不存在。Core 测试 564/564 通过，CLI 测试 336/336 通过，定向集成测试通过，完整 build 与 typecheck 通过。测试平台
环境（可选）
macOS、Node.js 24.12.0、应用仓库 Ink patch 的 npm workspace install、本地构建的
dist/cli.js，以及由 fake OpenAI server 驱动的真实qwen serve/ACP child。风险与范围
关联 Issue
关闭 #8851。
关联 #8618 和 #8620。