fix(core): allow reading saved plan files without a confirmation prompt#7678
Conversation
The default plans dir (~/.qwen/plans) sits outside the workspace and was in none of ReadFileTool's permission-free roots, so reading a saved plan back landed on an ask confirmation — popped at exactly the moment the user approved the plan and told the agent to start coding, and resolvable as a denial in non-interactive/ACP flows. With the approved-plan pointer (QwenLM#7197) the saved file is the model's only recovery route for the plan text, so the read must not stall on a prompt. Adds config.getPlansDir() to ReadFileTool.getDefaultPermission's allowedRoots and to AcpAgent.buildAcpLocalReadRoots (per the SYNC comment). The dir holds only session plan files; sibling ~/.qwen files such as settings.json stay confirmation-gated, pinned by a new test. Refs QwenLM#6237 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the PR! Template looks good ✓ Problem: observed and well-documented. This is finding #3 from the #7197 review — the approved-plan pointer tells the model to re-read the saved plan file, but Direction: aligned. This closes a gap in the permission model that the #7197 review identified before that trade could be considered complete. Plan files are already written today by Size: 9 production lines (6 in Approach: the scope is exactly right — add Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测且有充分记录。这是 #7197 review 的 finding #3——approved-plan pointer 让模型重读已保存的 plan 文件，但 方向：对齐。这修补了 #7197 review 在该取舍认定完成之前识别出的权限模型缺口。plan 文件今天就由批准时的 规模：9 行生产代码（ 方案：范围恰好——按 SYNC 注释的绑定关系，把 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given the problem (plan files under Comparison: the PR does exactly this. The two production edits land in the correct position in each list — after The test additions are well-structured: the No correctness bugs, security holes, or regressions found. The security model is sound: No findings. ✅ CI Test EvidenceAt review time (
The main ubuntu test suite is still in progress at review time — no failures so far. The author reports 87/87 on This PR touches the 中文说明代码审查独立方案： 针对该问题（ 对比： PR 完全这样做了。两处生产编辑落在各列表的正确位置—— 测试新增结构良好： 未发现正确性 bug、安全漏洞或回归。安全模型可靠： 无发现 ✅ CI 测试证据审查时（ 本 PR 触及 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean across every stage; minimal, correct fix for a confirmed gap in the permission model. This is exactly the follow-up the #7197 review asked for. Nine production lines that add 中文说明置信度：5/5——各阶段均干净；以最小、正确的改动修补了权限模型中已确认的缺口。 这正是 #7197 review 要求的 follow-up。9 行生产代码把 — Qwen Code · qwen3.8-max-preview Reviewed at |
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. ✅
Maintainer local verification — merge referenceI built and ran this PR locally in an isolated worktree against a real compiled Environment: macOS (Darwin 24.6) · Node v22.23.1 · vitest v3.2.4 1. Focused suites (PR head) ✅
Matches the numbers in the PR body exactly. 2. A/B load-bearing check ✅ (are the new tests real?)Reverted only the two source files to
Crucially, the scope-guard test 3. No-mock real-run E2E (before/after) ✅Drove a real
Exactly one cell flips ( 4. Scope / hygiene ✅
Evidence
Verdict: verified — the change is minimal, correct, load-bearing, and narrowly scoped. Good to merge from a verification standpoint. 👍 中文版本（点击展开）维护者本地验证 —— 合并参考我在隔离 worktree 中用真实编译产物 环境： macOS（Darwin 24.6）· Node v22.23.1 · vitest v3.2.4 1. 定向测试（PR head）✅
与 PR 描述中的数字完全一致。 2. A/B「测试是否载荷」检查 ✅只把两个源码文件回退到
关键点：作用域护栏测试 3. 无 mock 真实运行 E2E（前后对照）✅驱动真实
只有一个单元格翻转（plan 文件 4. 范围 / 卫生 ✅
结论： 已验证——改动最小、正确、载荷、范围收敛。从验证角度可以合并。👍 Verification run locally by the maintainer against the PR head ( |
|
Thanks for the verification — the adversarial |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
Maintainer re-verification (round 2) — re-run against today's
|
| round 1 | round 2 (this run) | |
|---|---|---|
| PR head | 1f5c0e18d |
1f5c0e18d (unchanged) |
| base | merge-base 9e7acec8 |
main tip 451bcad9 (+11 commits) |
| verified revision | — | merge result 7adb1827 |
Environment: macOS (Darwin 24.6) · Node v22.23.1 · vitest 3.2.4 · isolated worktree, real compiled dist.
1. Merge is clean, diff unchanged ✅
None of the 11 new commits touch read-file.ts / acpAgent.ts (or config.ts / paths.ts / storage.ts). Merging the PR into main produces no conflicts, and the merged-state diff is byte-for-byte the PR's diff — 4 files, 30 insertions, 0 deletions. No silent semantic drift from the base advance.
2. Focused suites at the new base ✅
| suite | result |
|---|---|
packages/core → src/tools/read-file.test.ts |
87 passed (87) |
packages/cli → src/acp-integration/acpAgent.test.ts |
317 passed (317) |
Both reproduce the round-1 numbers and the PR body exactly.
3. A/B load-bearing check ✅
Reverted only the two source files to origin/main, kept the PR's tests, rebuilt, re-ran:
| suite | on head | source reverted |
|---|---|---|
core read-file.test.ts |
87/87 | 1 failed — should return allow for saved plan files under the plans directory → expected 'ask' to be 'allow' |
cli acpAgent.test.ts |
317/317 | 3 failed — all three ACP fallback-read-root tests |
The scope-guard test should still return ask for ~/.qwen files outside the plans directory stays green on the reverted source. That is the discriminator worth noting: it means the new assertions pin the new behaviour rather than being trivially satisfied, and the settings.json guard is a genuine regression pin (base already gated it) rather than a test written to match whatever the code happened to do.
4. No-mock real-run E2E ✅
Drove a real Config + ReadFileTool from the compiled dist — no vitest, no mocked config object (the shipped unit test stubs getPlansDir, so this exercises the real Config/Storage path instead). The plan file is written by the product's own config.savePlan(), i.e. the exact code that runs when a user approves a plan, under a temp QWEN_HOME:
read_file target |
BEFORE (origin/main) |
AFTER (PR 7678) |
|---|---|---|
saved plan file ~/.qwen/plans/<session>.md |
ask ✗ (stalls) |
allow ✓ ← the fix |
~/.qwen/settings.json |
ask ✓ |
ask ✓ (unchanged) |
~/.qwen/plans-secret/x.md (prefix sibling) |
ask ✓ |
ask ✓ (boundary-safe) |
| workspace file | allow ✓ |
allow ✓ |
| unrelated outside file | ask ✓ |
ask ✓ |
diff of the two full runs is two lines — exactly one permission cell moves. The prefix-sibling case stays ask because isSubpath() uses a path.relative boundary check rather than a string prefix.
5. New this round — can a configured plansDirectory escape the project root? 🔍
The PR's Risk & Scope section claims the widening is effectively the default ~/.qwen/plans only, because a configured plansDirectory must resolve within the project root. Round 1 took that from reading the code; this round I probed it against the real Config:
configured plansDirectory |
outcome |
|---|---|
"plans" (inside root) |
allowed — and already workspace-readable pre-PR, so no new surface |
"../escape" |
rejected — FatalConfigError |
"/etc" |
rejected — FatalConfigError |
"~/.ssh" |
rejected — FatalConfigError |
No configured value escapes the project root, so the newly permission-free surface really is just the default <QWEN_HOME>/plans. The Risk & Scope claim is accurate as written.
6. Gates on the 4 changed files ✅
prettier --check clean · eslint --max-warnings 0 exit 0 · tsc --noEmit clean for both packages/core and packages/cli (round 1 only covered core). Upstream CI on this PR is green.
Evidence
Verdict: re-verified at today's main. Merges clean, every round-1 result reproduces, the permission widening is exactly one path class, and the stated scope limit is now empirically confirmed rather than inferred. No blockers — good to merge from a verification standpoint. 👍
中文版本（点击展开）
维护者复验（第 2 轮）—— 基于今天的 main 重跑
这是对我第 1 轮验证的跟进。PR head 没有变动（1f5c0e18d），但 main 相对该轮 merge-base 已前进 11 个提交，因此我在当前 tip 上从零重新构建并重跑了全部验证，确认结论在合并时点依然成立。
| 第 1 轮 | 第 2 轮（本次） | |
|---|---|---|
| PR head | 1f5c0e18d |
1f5c0e18d（未变） |
| base | merge-base 9e7acec8 |
main tip 451bcad9（+11 提交） |
| 实际验证的修订 | — | 合并结果 7adb1827 |
环境： macOS（Darwin 24.6）· Node v22.23.1 · vitest 3.2.4 · 隔离 worktree、真实编译产物 dist。
1. 合并干净，diff 未漂移 ✅
11 个新提交没有触碰 read-file.ts / acpAgent.ts（也没碰 config.ts / paths.ts / storage.ts）。把 PR 合入 main 无冲突，合并后相对 main 的 diff 与 PR 自身 diff 完全一致——4 文件、30 行新增、0 删除，base 前进没有引入任何语义漂移。
2. 新 base 下的定向测试 ✅
| 测试 | 结果 |
|---|---|
packages/core → src/tools/read-file.test.ts |
87/87 通过 |
packages/cli → src/acp-integration/acpAgent.test.ts |
317/317 通过 |
与第 1 轮及 PR 描述中的数字完全一致。
3. A/B 承重性检查 ✅
只把两个源文件回退到 origin/main，保留 PR 的测试，重新构建后重跑：
| 测试 | PR head | 源码回退后 |
|---|---|---|
core read-file.test.ts |
87/87 | 1 条失败 —— should return allow for saved plan files under the plans directory → expected 'ask' to be 'allow' |
cli acpAgent.test.ts |
317/317 | 3 条失败 —— 三条 ACP 兜底读根测试全部失败 |
关键判别点：作用域守卫测试 should still return ask for ~/.qwen files outside the plans directory 在回退源码上依然通过。这说明新断言钉住的是新行为，而不是恰好为真；同时 settings.json 那条是真正的回归钉（base 本来就已门控），而非照着现状写出来的测试。
4. 无 mock 真实运行 E2E ✅
用编译产物中的真实 Config + ReadFileTool 驱动——不走 vitest、不使用 mock 的 config 对象（PR 自带单测把 getPlansDir 打了桩，本项则走真实 Config/Storage 路径）。plan 文件由产品自身的 config.savePlan() 写入，即用户批准 plan 时真正执行的代码，运行在临时 QWEN_HOME 下：
read_file 目标 |
修复前（origin/main） |
修复后（PR 7678） |
|---|---|---|
已保存 plan 文件 ~/.qwen/plans/<session>.md |
ask ✗（会卡住） |
allow ✓ ← 本次修复 |
~/.qwen/settings.json |
ask ✓ |
ask ✓（不变） |
~/.qwen/plans-secret/x.md（前缀兄弟目录） |
ask ✓ |
ask ✓（边界安全） |
| 工作区内文件 | allow ✓ |
allow ✓ |
| 工作区外无关文件 | ask ✓ |
ask ✓ |
两次完整运行的 diff 只有两行——恰好只有一个权限单元格发生变化。前缀兄弟目录仍为 ask，因为 isSubpath() 用的是 path.relative 边界判断而非字符串前缀匹配。
5. 本轮新增 —— 配置 plansDirectory 能逃出项目根吗？🔍
PR 的「风险与范围」称：由于配置的 plansDirectory 必须解析在项目根内，实际扩大的只有默认 ~/.qwen/plans。第 1 轮我是通过读代码认可这一点的；本轮直接对真实 Config 做了探测：
配置的 plansDirectory |
结果 |
|---|---|
"plans"（根内） |
允许 —— 且本就在 PR 之前即工作区可读，不构成新增读面 |
"../escape" |
拒绝 —— FatalConfigError |
"/etc" |
拒绝 —— FatalConfigError |
"~/.ssh" |
拒绝 —— FatalConfigError |
没有任何配置值能逃出项目根，因此新增的免确认读面确实只有默认的 <QWEN_HOME>/plans。风险与范围一节的表述准确。
6. 4 个改动文件的门禁 ✅
prettier --check 通过 · eslint --max-warnings 0 exit 0 · tsc --noEmit 在 packages/core 与 packages/cli 两侧均通过（第 1 轮仅覆盖 core）。上游 CI 亦为绿。
证据
见上方英文部分的截图。
结论： 已在今天的 main 上复验。合并干净，第 1 轮全部结论复现，权限扩大严格限定为一类路径，且原先靠推断的范围限制现已获实证。无阻塞项，从验证角度可以合并。👍
4 participants
What this PR does
Adds the plans directory (
config.getPlansDir(), default~/.qwen/plans) to the two mirrored permission-free read-root lists:ReadFileTool.getDefaultPermission()'sallowedRootsandAcpAgent.buildAcpLocalReadRoots()(per the SYNC comment binding the two). Reading a saved plan file no longer lands on anaskconfirmation; everything else under~/.qwen(settings.json, credentials) stays confirmation-gated, pinned by a new test.Why it's needed
This is finding #3 from the #7197 review (review comment), which asked for exactly this change and noted it "should land before this trade is considered complete": the approved-plan pointer replaces the in-history plan text with a reference to the saved file, but the default plans dir sits outside the workspace and in none of
ReadFileTool's permission-free roots — so the pointer's own instruction ("read that file if you need to consult it again") stalls on anaskprompt, popped at exactly the moment the user just approved the plan and told the agent to start coding; in non-interactive/ACP flows anaskcan resolve to a denial. Since nothing re-injects the plan after approval, the saved file is the model's only recovery route for the plan text. The change is independently safe to land: plan files are already written today bysavePlanat approval, and reading them back should never have required a prompt.Reviewer Test Plan
How to verify
~/.qwen/plans), have the modelread_filethat path. Before: an ask confirmation pops (or the read is denied in ACP). After: the read proceeds directly. Reading~/.qwen/settings.jsonstill prompts.npx vitest run src/tools/read-file.test.ts(packages/core, 87/87) andnpx vitest run src/acp-integration/acpAgent.test.ts(packages/cli, 317/317).Evidence (Before & After)
E2E drives the compiled
Config+ReadFileToolwith a real plan file written toconfig.getPlanFilePath()under a tempQWEN_HOME, before/after viagit stash+ rebuild:askallow~/.qwen/settings.jsonstays gated (ask)allowTested on
Environment (optional)
macOS (Darwin 24.6), Node v22.23.1; vitest.
npm run typecheck,eslint --max-warnings 0,prettier --checkall clean.Risk & Scope
plansDirectoryis configured it must resolve within the project root (already workspace-readable), so the widening is effectively the default~/.qwen/plansonly. The new settings.json test pins that no sibling~/.qwenpath is exposed.Linked Issues
Refs #6237 (follow-up to the #7197 review, finding #3)
中文说明
本 PR 做了什么
把 plans 目录（
config.getPlansDir()，默认~/.qwen/plans）加入两处互为镜像的免确认读根列表：ReadFileTool.getDefaultPermission()的allowedRoots与AcpAgent.buildAcpLocalReadRoots()（两处由 SYNC 注释绑定）。读取已保存的 plan 文件不再弹ask确认；~/.qwen下的其它文件（settings.json、凭据）仍保持确认门控，并由新测试钉住。为什么需要
这是 #7197 review 的 finding #3（review 明确要求本改动并指出"应在认定该取舍完成之前落地"）：approved-plan pointer 把历史中的 plan 原文换成对已保存文件的引用，但默认 plans 目录在工作区之外、也不在任何免确认读根里——pointer 自带的"需要时可重读该文件"指令会撞上
ask确认框，恰好出现在用户刚批准 plan、让 agent 开始写码的时刻；非交互/ACP 流程中ask还可能解析为拒绝。批准后没有任何机制重新注入 plan，已保存文件是模型找回 plan 文本的唯一途径。本改动可独立安全落地：plan 文件今天就由批准时的savePlan写入，读回它本不该需要确认。审阅测试计划
如何验证
read_file该路径：之前弹确认（ACP 下可能被拒）；之后直接读取。读~/.qwen/settings.json仍会弹确认。packages/core下 read-file 87/87；packages/cli下 acpAgent 317/317。证据（Before & After）
E2E 驱动编译产物
Config+ReadFileTool，在临时QWEN_HOME下用config.getPlanFilePath()写真实 plan 文件，git stash+ 重建对照：未修复时 plan 文件是ask；修复后allow，settings.json 与工作区文件行为不变。新单测在未修复源码上失败；ACP 读根 3 条既有测试重新钉住。测试平台
macOS 已本地验证（✅）；Windows / Linux 依赖 CI（⚠️ ）。
环境
macOS（Darwin 24.6）、Node v22.23.1；vitest；typecheck / eslint / prettier 全绿。
风险与范围
plansDirectory时必须解析在项目根内（本就工作区可读），所以实际扩大的只有默认~/.qwen/plans。新增 settings.json 测试钉住不暴露~/.qwen其它路径。关联 Issue
Refs #6237（#7197 review finding #3 的 follow-up）
🤖 Generated with Claude Code