fix(permissions): cite matching deny rule for compound and virtual-op shell denials - #11411
Conversation
… shell denials findMatchingDenyRule() matched strictly fewer rules than evaluate(), which makes the actual deny decision. For compound commands and shell commands denied via a virtual file-op rule (e.g. Read(//**/node_modules/**) matching `cat /app/node_modules/...`), the deny message dropped the rule citation and read as tool-scoped, causing the model to abandon the tool. Now findMatchingDenyRule runs the same compound-command and virtual-op passes as evaluate(), and the deny message frames the denial as invocation-scoped and notes that other uses of the tool are still permitted. Fixes QwenLM#11405 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-issue-patrol/jmtt7wpbesc
|
Thanks for the PR! Re-run at the new head Template looks good ✓ — all nine headings from the template are present and filled in, including the Tested-on table, the before/after evidence, and the Chinese translation. Problem: observed, not theoretical. #11405 is open, P2, and carries the reporter's exact Direction: aligned. This is a message-accuracy fix on a bug that makes the model abandon an entire tool over one blocked pattern. The property that makes it safe still holds and I re-verified it at this head: Size: core paths are touched, so the breakdown: 87 production lines ( Approach: scope feels right and genuinely minimal — four files, no drive-by refactors, and the two things deliberately left out (the custom per-rule message feature request, and the The one question I raised last time still stands as a question, not a blocker: the fix teaches Risk: no elevated risk signals — none of the four changed files match the revert-correlated paths. Status of the previous pass: the Prettier blocker is fixed ( Moving on to code review. 🔍 中文说明感谢贡献！本次是在新 head 模板完整 ✓ —— 模板要求的九个小节全部存在且已填写，包括 Tested-on 表格、before/after 证据与中文翻译。 问题：已观测到的问题，不是理论性加固。#11405 处于 open、P2，报告者给出了确切的 方向：对齐。这是一个 P2 bug 的消息准确性修复——一个被单一 pattern 拦截的调用，会让模型放弃整个工具。让它安全的那个性质在当前 head 上依然成立，我重新核实过： 规模：触及核心路径，故给出拆分：生产代码 87 行（ 方案：范围合理且确实最小化——四个文件，没有顺手重构；两处刻意不做的内容（自定义 per-rule 消息的功能请求，以及使用不同机制的 我上轮提的那个问题依然只是问题、不是阻塞项：该修复让 风险：无升级风险信号——四个变更文件均未命中与回滚相关的路径。 上轮结论的处理情况：Prettier 阻塞项已修复（ 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewEvidence carried: static review of the diff against base source in a read-only worktree at Both blockers from the last pass are gone, and one of my four findings was simply wrong. Details on all three below, then what is left. Resolved: Prettier (previous finding 1)
Resolved: catch-all false reassurance (previous finding 2)
The new test pins the three realistic catch-alls. This was the finding I would not have merged past, and it is properly closed. Retracted: the cd-tracking divergence (previous finding 3)This one was wrong, and I want to say so plainly rather than let it sit in the thread. I claimed
Your recursion mirrors the real structure. Re-running the worked example I gave: with Two smaller ordering notes, both non-blocking and neither worth a commit on their own: within a segment your recursion checks virtual ops before base rules while Still open, non-blocking: two implementations of one question (previous finding 4)Unchanged, and I am still not asking for the Two residual nits in the new code
What I verified as correct
CI statusRead once, no polling — the unit suite runs longer than any in-agent wait. The Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The red check is pre-existing The failure is in a package this PR does not touch. From the
Your own tests are green, and I confirmed the new ones actually ran. Per-package attribution from the JUNIT paths in the same log:
Sandboxed verification would settle what CI cannot: Not verified
中文说明所携带的证据：在 上轮的两个阻塞项都已消失，而我上轮四条发现中有一条纯粹是错的。三点分别说明如下，然后是剩下的部分。 已解决：Prettier（上轮发现 1）。 当前 head 上 已解决：catch-all 的错误安抚（上轮发现 2）。 撤回：cd 跟踪分歧（上轮发现 3）。 这一条是错的，我想明确说出来，而不是让它留在线程里。 我当时声称 事实是 你的递归镜像的是真实结构。把我给的例子重算一遍： 另外两点更小的顺序差异，均非阻塞、也不值得单独发一个 commit：在一段之内，你的递归先查虚拟操作再查基础规则，而 仍未处理、非阻塞：同一个问题的两份实现（上轮发现 4）。 没有变化，我依然不要求在本 PR 里做「由 新代码里的两个残留小问题：
已验证正确的部分：
CI 状态：只读取一次，不轮询——单测套件耗时超过任何在 agent 内等待的预算。CI 结束后由 红色检查项是 你自己的测试是绿的，且我确认新增测试确实跑过了。 按同一日志中的 JUNIT 路径归属： 沙箱验证可以补上 CI 补不了的部分： 未验证项：
— Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — both blockers from the last pass are properly closed and the core suite is green including all four new tests; what is left is two nits and a red check that is not yours. @yiliang114 stepping back from the line-by-line: I believe the bug, I believe the root cause, and the fix is aimed at the right place. Re-running the gate on the new head rather than re-reading my old notes changed my mind on two of the four things I told you last time, in opposite directions. The catch-all fix is the one that mattered. The cd-tracking finding I retracted. Going back to the proposal I wrote before reading the diff — I would still have had On the safety question, which is the one that actually matters for a permissions change: I verified again at this head that About the red check. One practical consequence worth knowing before you do it: this approval is pinned to The 中文说明信心度：4/5 —— 上轮两个阻塞项都已妥善收口，core 套件全绿且包含全部四个新测试；剩下的是两个小问题，以及一个不属于你的红色检查项。 @yiliang114 抛开逐行细节来看：我认可这个 bug，认可根因，也认可修复打在了正确的位置。这次我是在新 head 上重新过闸、而不是复读旧笔记，结果我上轮告诉你的四件事里有两件改变了判断——方向还相反。 catch-all 那个修复是关键。 cd 跟踪那条发现我撤回了。 回到我在读 diff 之前写下的方案——我仍然会让 关于安全性问题，也是权限改动真正要紧的那个：我在这个 head 上再次核实了 关于那个红色检查项。 在你动手之前有一个实际后果值得知道：本次批准固定在 当前 head 上已在进行的 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Needs another pass before this can merge — details in the Stage 2 and Stage 3 notes above. Two things I would not merge past:
-
Lint & Staticis red on this PR's own files. Prettier flagspackages/core/src/core/permissionFlow.test.tsandpackages/core/src/permissions/permission-manager.ts— both modified here, while the third modified file is clean and main is green.npx prettier --writeon those two clears it. -
Catch-all rules now get a false reassurance.
rule-parser.tsdocumentsBash(*)as equivalent to a bareBash(tool-wide), but it contains a(, somatchingRule?.includes('(')appends "Other uses of this tool are still permitted." for a tool that is fully blocked.Read(//**)andWebFetch(*)behave the same way. That is a new inaccuracy where the old text was correct, and it reproduces #11405's outcome in the opposite direction — the model keeps retrying a blocked tool. The scoped-vs-tool-wide call is being made from punctuation in the raw string; the parsed rule already knows viarule.specifier. No test covers either case.
Not blocking, but worth a look: the per-segment recursion re-runs the virtual-op pass that evaluate() deliberately runs only on the full command in order to preserve cd tracking, so for cd /tmp && cat foo && rm x the citation can name a rule that did not decide the denial. Explanatory-only — I verified no consumer uses findMatchingDenyRule() for an enforcement decision, so nothing here changes what a user may run.
Test (ubuntu-latest, Node 22.x) was still in progress at review time, so the new unit tests are not yet confirmed green.
中文说明
合并前需要再过一轮——详见上方 Stage 2 与 Stage 3 的记录。两点我不建议直接合并：
-
Lint & Static因本 PR 自己的文件而红。 Prettier 报出packages/core/src/core/permissionFlow.test.ts与packages/core/src/permissions/permission-manager.ts——两者都是本 PR 修改的文件，而第三个被修改的文件是干净的、main 也是绿的。对这两个文件执行npx prettier --write即可解决。 -
通配（catch-all）规则现在会得到错误的安抚。
rule-parser.ts记载Bash(*)等价于裸Bash（工具级），但它含有(，因此matchingRule?.includes('(')会为「已被完全禁用」的工具追加「本工具的其他用法仍被允许」。Read(//**)与WebFetch(*)同理。这是在原本正确的文本处新增的不准确表述，并以相反方向重现了 #11405 的后果——模型会不断重试一个被禁用的工具。「调用级还是工具级」这个判断是靠原始字符串里的标点做出的；解析后的规则通过rule.specifier本来就知道答案。这两种情况目前都没有测试覆盖。
不阻塞，但值得看一眼：逐段递归重跑了虚拟操作 pass，而 evaluate() 刻意只在完整命令上运行它以保住 cd 跟踪，因此对 cd /tmp && cat foo && rm x，引用可能指向一条并未做出该拒绝决定的规则。这只影响解释文本——我已核实没有任何消费方把 findMatchingDenyRule() 用于判定执行，因此这些都不改变用户被允许执行什么。
审查时 Test (ubuntu-latest, Node 22.x) 仍在进行中，因此新增单测尚未确认通过。
— Qwen Code · qwen3.8-max-2026-09-02
Prettier reflows the denyRule construction and the long toContain assertion; no behavioral change. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmttbhaw6si
The deny message decided scoped-vs-tool-wide by sniffing `(` in the raw rule string, so a catch-all like `Bash(*)` (equivalent to bare `Bash`) got a false "Other uses of this tool are still permitted." for a tool that is fully blocked. Use the parsed rule's specifier instead: bare `*` and `//**` catch-alls are tool-wide; only genuinely scoped specifiers get the reassurance. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmttcwqrask
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 126 passed · 6 failed · 132 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：126 通过 · 6 失败 · 132 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11411 — deep verificationVerdict: The central claim is proven load-bearing: deny-rule citation coverage on denied 中文摘要结论： A/B 结论：PR 的核心主张成立且是 load-bearing 的。在「被拒绝的 shell 调用必须引用到判定规则」这一不变量上，base 为 6/23，head 为 23/23，共 17 个用例从"拒绝但不引用规则"翻转为"拒绝且正确引用"。同时 base 与 head 在全部 25 个上下文、22 条规则 × 探针上的 发现的问题：
变异矩阵：未变异对照先确认可测量地为绿（422 + 31 passed，0 failed）。8 个守卫变异中 7 个被"预期测试"精确杀死；组合行 M3（同时删除两个新 pass）杀死两条新测试（2 failed），证明两个 pass 各自都是 load-bearing、并非互相掩护。两个同文件阳性对照均成功使测试变红（M4 → 6 failed，M10 → 3 failed），证明 runner 确实收集并执行了被变异的那个文件，因此"无存活变异"不是 runner 空转的结果。无存活变异。 未覆盖范围：未按 Test Plan 的原始方式跑真实模型的交互式 TUI 会话（已在真实代码路径上复现其渲染的消息，但未验证模型是否真的不再放弃该工具，即只复现了 shape 而非 cause）；浅克隆（depth 2）导致无法逐提交归因（快照列 4 个提交，本地 ScopeCentral claim. Secondary claims. (a) The framing must not reassure for tool-wide catch-alls Out of scope by choice: repo-wide test suites, integration/TUI tests, the bundle, A/B: the central claimControl built with Oracle for every cell: the real compiled
17 cells flip from broken to fixed. See Coverage by mechanism family (base → head):
The two compound cells that are not denied are themselves assertions: The three positive controls stay green on both arms, so the flip is attributable No decision changed anywhereThe strongest safety result of the round: across 25 census contexts and all Citations are not merely non-empty, they are independently sufficient: for 5 Reviewer Test Plan, walked step by stepBoth steps were run with the plan's own
No step of the plan was unperformable at the message level. 8/8 assertions pass at Recursion and scalingThe compound pass adds a recursion into Cost of the two new passes,
Per-segment cost falls across rungs (0.160 → 0.043 ms/segment), so the added work Mutation matrix — vacuity and pinningUnmutated control measurably green first:
Zero survivors. Every guard the PR adds is pinned by a test that fails when the The kills fail the intended behavioural assertion, not an import, a compile, or a
Targeted gates
All four were proven live before being cited: planting an unused CorrectionsThese correct the PR's description and the new code comment. They are not requests The comment " The Risk & Scope claim "Call sites that pass The out-of-scope declaration holds. Findings1. Four catch-all spellings fully block a tool and still promise other uses are permitted — Suggestion
if (rule.specifierKind === 'path') {
return rule.specifier === '//**';
}
return rule.specifier === '*';But "catch-all" is a class of spellings, not two strings. Measured against the
Actual message for the first row: Reproduce: node tmp/pr11411-verify-20260909-015234/h3-guard-census.mjs . head
# OVER-REASSURE (fully blocked but promised other uses): 4This is new in this PR: the base arm measures Severity is Suggestion rather than Critical because the four spellings are Measured minimal fix (preserves the fourth commit's intent) if (rule.specifierKind === 'path') {
// `//` is the rule grammar's absolute-filesystem root. Every remaining
// segment being `**` means no path can escape the rule (`//**`,
// `//**/**`). `/**` stays project-scoped and `//**/*` still misses
// root-level files, so neither is tool-wide.
return /^\/\/\*\*(?:\/\*\*)*$/.test(rule.specifier);
}
// Command and domain specifiers glob, so a specifier made of nothing but
// wildcards (`*`, `**`, `* *`) matches every target.
return /^[\s*]+$/.test(rule.specifier);Applied in a scratch worktree at head and rebuilt (44 s), then re-driven through the
See The path branch needs the The suite is green both with and without this patch, so the axis is unpinned. for (const raw of ['Bash(*)', 'Read(//**)', 'Bash(**)', 'Read(//**/**)']) {The patch should ship with that fixture; without it nothing distinguishes head from 2. The
|
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. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.23.1)
| const stillPermitted = | ||
| matchingRule && !isToolWideDenyRule(matchingRule) | ||
| ? ' Other uses of this tool are still permitted.' | ||
| : ''; |
There was a problem hiding this comment.
[Suggestion] R1-12: The reassurance is gated on whether the cited rule is tool-wide, never on whether the invoked tool is still usable, so a scoped rule cited alongside a catch-all in the same deny set produces a sentence that is simply false.
With permissions.deny: ["Bash(*)", "Read(//**/node_modules/**)"] — catch-all listed first, so no exotic ordering is needed — and the model running cat /app/node_modules/lodash/index.js, the new virtual-op pass cites the cross-kind scoped rule Read(//**/node_modules/**), isToolWideDenyRule classifies it as scoped, and the message appends "Other uses of this tool are still permitted." while Bash(*) denies every shell command the model could issue. Measured in that same config: echo hi is denied and ls is denied. The model keeps trying other shell commands, each denied, spending turns on work the message wrongly certifies as available. The redundant ordering ["Bash(npm view *)", "Bash(*)"] reproduces it, as does the read variant ["Read(./.env)", "Read(//**)"]. Neither new test can see this case, because both mock findMatchingDenyRule to return a single raw string, so no test exercises a two-rule deny set. Widening the spelling list in isToolWideDenyRule (reported separately on that function) does not fix a two-rule configuration.
Witness (real PermissionManager driven through the real evaluatePermissionFlow, same input on both trees):
deny: ["Bash(npm view *)","Bash(*)"] command 'npm view foo'
BASE: Tool "run_shell_command" is denied by permission rules. Matching deny rule: "Bash(npm view *)".
PR: This "run_shell_command" invocation was denied by permission rules. Matching deny rule: "Bash(npm view *)". Other uses of this tool are still permitted.
PR: otherShellUses = { "echo hi": "deny", "ls": "deny", "git status": "deny" }
deny: ["Bash(*)","Read(//**/node_modules/**)"] command 'cat /app/node_modules/lodash/index.js'
BASE: ... Matching deny rule: "Bash(*)".
PR: ... Matching deny rule: "Read(//**/node_modules/**)". Other uses of this tool are still permitted.
PR: otherShellUses = { "echo hi": "deny", "ls": "deny" }
controls, same PR build: deny ["Bash(*)"] alone -> no reassurance;
deny ["Bash(*)","Bash(npm view *)"] -> cites Bash(*), no reassurance
Ask the tool rather than the cited rule: suppress the sentence unless some other use of the invoked tool is actually permitted. That needs a query over the deny set on the manager side (for example a hasToolWideDenyRule(toolName) predicate, or returning every matching deny rule and emitting the sentence only when none of them is tool-wide), called optionally because the memory-scoped shim does not implement listRules().
The catch-all cannot be detected through the existing registry probe: matchesRule returns false for a command-specifier rule when command === undefined (rule-parser.ts:1577), which permission-manager.ts:861-863 relies on — measured, under deny ["Bash(npm view *)","Bash(*)"] getToolRegistrationStatus('run_shell_command') is "registered" and isToolEnabled is true while every command evaluates to deny — so the query has to inspect the deny rules themselves.
A case in permissionFlow.test.ts with deny rules ['Bash(*)', 'Read(//**/node_modules/**)'] asserting the message still cites the Read rule but does not contain "Other uses of this tool are still permitted" would pin this; please confirm that test reds when the gate is reverted to isToolWideDenyRule(matchingRule) alone.
中文说明
安抚句的条件是「被引用的那条规则是否为工具级」，而从未检查「被调用的工具是否还有可用之处」。因此当同一份 deny 集合里同时存在范围规则与全量规则、而引用落到范围规则上时，这句话就是假的。
设 permissions.deny: ["Bash(*)", "Read(//**/node_modules/**)"]（全量规则排在前面，因此并不需要特殊顺序），模型执行 cat /app/node_modules/lodash/index.js：新增的虚拟操作 pass 会引用跨类型的范围规则 Read(//**/node_modules/**)，isToolWideDenyRule 判定其为范围级，于是消息追加「本工具的其他用法仍被允许」——而 Bash(*) 拒绝了模型可能发出的每一条 shell 命令。同一配置下实测：echo hi 被拒、ls 被拒。模型会不断尝试其他 shell 命令、每次都被拒，把回合花在消息错误地宣称为可用的工作上。冗余顺序 ["Bash(npm view *)", "Bash(*)"] 可复现同一问题，读取类变体 ["Read(./.env)", "Read(//**)"] 同理。两个新增测试都看不到这个场景，因为它们都把 findMatchingDenyRule mock 成返回单个字符串，没有任何测试使用两条规则的 deny 集合。放宽 isToolWideDenyRule 的拼写列表（已就该函数单独提出）无法修复「两条规则」这种配置。
证据（真实 PermissionManager 经真实 evaluatePermissionFlow 驱动，两棵代码树使用同一输入）：
deny: ["Bash(npm view *)","Bash(*)"] command 'npm view foo'
BASE: Tool "run_shell_command" is denied by permission rules. Matching deny rule: "Bash(npm view *)".
PR: This "run_shell_command" invocation was denied by permission rules. Matching deny rule: "Bash(npm view *)". Other uses of this tool are still permitted.
PR: otherShellUses = { "echo hi": "deny", "ls": "deny", "git status": "deny" }
deny: ["Bash(*)","Read(//**/node_modules/**)"] command 'cat /app/node_modules/lodash/index.js'
BASE: ... Matching deny rule: "Bash(*)".
PR: ... Matching deny rule: "Read(//**/node_modules/**)". Other uses of this tool are still permitted.
PR: otherShellUses = { "echo hi": "deny", "ls": "deny" }
同一 PR 构建下的对照：deny ["Bash(*)"] 单独 -> 不安抚；
deny ["Bash(*)","Bash(npm view *)"] -> 引用 Bash(*)，不安抚
建议问「工具」而不是问「被引用的规则」：只有在被调用工具确实还存在其他可用用法时才输出这句话。这需要在 manager 侧对 deny 集合做一次查询（例如 hasToolWideDenyRule(toolName) 谓词，或返回全部命中的 deny 规则、仅当其中没有工具级规则时才输出该句），并且要可选调用，因为 memory 作用域的 shim 没有实现 listRules()。
全量规则无法通过现有的注册表探测发现：当 command === undefined 时 matchesRule 对带 command specifier 的规则返回 false（rule-parser.ts:1577），而 permission-manager.ts:861-863 正是依赖这一点——实测在 deny ["Bash(npm view *)","Bash(*)"] 下 getToolRegistrationStatus('run_shell_command') 为 "registered"、isToolEnabled 为 true，而每条命令都判为 deny——因此该查询必须直接检查 deny 规则本身。
在 permissionFlow.test.ts 中补一个 deny 规则为 ['Bash(*)', 'Read(//**/node_modules/**)'] 的用例，断言消息仍引用 Read 规则、但不包含「Other uses of this tool are still permitted」，即可固定该行为；请确认把条件回退为仅 isToolWideDenyRule(matchingRule) 时该测试会变红。
— qwen3.8-max via Qwen Code /review (v0.23.1)
| if (!rule.specifier) { | ||
| // No specifier blocks the whole tool; a param-matcher-only rule | ||
| // (e.g. `Agent(model:opus)`) is still scoped by its matchers. | ||
| return !rule.toolParamMatchers?.length; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-9: Nothing gates the no-specifier branch of isToolWideDenyRule. Replacing its body with return false leaves every test green, so a regression there tells the model other uses are permitted for a bare-name or empty-specifier tool-wide block — the exact misleading reassurance #11405 exists to remove — with the whole suite silent.
The forms at risk are the ones this helper's own doc comment calls tool-wide: a bare tool name (Bash) and an empty specifier (Bash()). If the branch regresses to "scoped", the model reads This "shell" invocation was denied by permission rules. Matching deny rule: "Bash". Other uses of this tool are still permitted. for a tool that is blocked entirely. The toolParamMatchers half is equally unpinned — Agent(model:opus) must stay scoped, and that assertion is the only thing keeping this branch from being a constant. permissionFlow.test.ts is the only file in the repo that asserts on this message.
Witness (mutation probe, measured):
mutant: `return !rule.toolParamMatchers?.length;` -> `return false;`
packages/core/src/core/permissionFlow.test.ts 31/31 GREEN (mutant survived)
positive control, unmutated PR code, both changed test files: 453/453 GREEN
revert probe (base source + the PR's tests): 3 of the 4 added tests RED on assertion, not compile
Extend the existing catch-all loop in permissionFlow.test.ts with 'Bash' and 'Bash()', and add one positive case for a param-matcher-only rule ('Agent(model:opus)') asserting the message does contain "Other uses of this tool are still permitted".
That loop feeds the classifier through a mock returning the raw string verbatim (findMatchingDenyRule: vi.fn().mockReturnValue(raw), permissionFlow.test.ts:125), so parseRule alone decides: new cases must be strings it really parses into the intended shape — bare Bash is "Simple tool name rule (no specifier)" (rule-parser.ts:394) and Agent(model:opus) yields specifier = plainParts.join(',').trim() || undefined with toolParamMatchers set (rule-parser.ts:462-463).
With 'Bash' and 'Bash()' added, that test is green on the PR's code (31/31) and goes red under the return false mutation, reporting Received: "This \"shell\" invocation was denied by permission rules. Matching deny rule: \"Bash\". Other uses of this tool are still permitted." — please confirm that red before landing the cases.
中文说明
isToolWideDenyRule 的「无 specifier」分支没有任何测试把关。把它的函数体换成 return false，全部测试仍然是绿的；因此该分支一旦回归，就会对裸工具名或空 specifier 的工具级封禁告诉模型「其他用法仍被允许」——这正是 #11405 要消除的误导性安抚——而整个测试套件不会有任何声响。
受影响的形式正是本函数自身注释称为工具级的那两种：裸工具名（Bash）与空 specifier（Bash()）。若该分支回归为「范围级」，模型会读到 This "shell" invocation was denied by permission rules. Matching deny rule: "Bash". Other uses of this tool are still permitted.，而该工具其实已被完全封禁。toolParamMatchers 那一半同样没有被固定——Agent(model:opus) 必须保持为范围级，而这一断言正是防止该分支退化成常量的唯一约束。全仓库中只有 permissionFlow.test.ts 对这条消息做断言。
证据（变异探测，实测）：
变异体：`return !rule.toolParamMatchers?.length;` -> `return false;`
packages/core/src/core/permissionFlow.test.ts 31/31 绿（变异体存活）
阳性对照，未变异的 PR 代码，两个被改测试文件：453/453 绿
回退探测（基线源码 + 本 PR 的测试）：4 个新增测试中有 3 个在断言处变红，而非编译失败
建议在 permissionFlow.test.ts 现有的全量规则循环中补上 'Bash' 与 'Bash()'，并补一个仅含 param-matcher 的规则（'Agent(model:opus)'）的正向用例，断言消息确实包含「Other uses of this tool are still permitted」。
该循环通过 mock 原样返回字符串（findMatchingDenyRule: vi.fn().mockReturnValue(raw)，permissionFlow.test.ts:125），因此完全由 parseRule 决定：新增用例必须是它真的会解析成预期形状的字符串——裸 Bash 属于「Simple tool name rule (no specifier)」（rule-parser.ts:394），Agent(model:opus) 则得到 specifier = plainParts.join(',').trim() || undefined 且 toolParamMatchers 被设置（rule-parser.ts:462-463）。
补上 'Bash' 与 'Bash()' 后，该测试在 PR 代码上是绿的（31/31），在 return false 变异下会变红并报出 Received: "This \"shell\" invocation was denied by permission rules. Matching deny rule: \"Bash\". Other uses of this tool are still permitted."——请在合入前确认它确实变红。
— qwen3.8-max via Qwen Code /review (v0.23.1)
| if (rule.specifierKind === 'path') { | ||
| // `//**` resolves to the filesystem root (`/**`) and matches every path. | ||
| return rule.specifier === '//**'; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-1: isToolWideDenyRule decides "blocks the whole tool" by exact string comparison against two hand-listed spellings, so other spellings the matchers do treat as match-everything are classified scoped and still receive the reassurance for a tool that is fully blocked.
This carries forward the concern from the previous review round about deciding tool-wide-vs-invocation-scoped from the raw string. The reported entrances are closed — Bash(*), Read(//**), WebFetch(*), bare Bash and Bash() all now classify correctly. What remains is that the surface is not finitely enumerable, so topping up the list will keep leaving holes: Bash(**) and Bash(* *) compile to match-every-command regexes, and Read(//**/*) and Read(//**/**) resolve to /**/* and /**/** and match every absolute path, yet none of them equals '*' or '//**'. With permissions.deny: ["Bash(**)"] every shell call is denied and cited as Bash(**), and the message still ends with "Other uses of this tool are still permitted." — an affirmative falsehood, which sends the model back into the retry-a-fully-blocked-tool loop #11405 describes, in the opposite direction. It is worth closing structurally rather than entrance by entrance.
Witness (real PermissionManager driven through the real evaluatePermissionFlow at this commit):
Bash(**) denied 12/12 commands (incl. '', 'a', 'multi-line') cited="Bash(**)"
msg="... Matching deny rule: \"Bash(**)\". Other uses of this tool are still permitted."
Bash(* *) denied 12/12 cited="Bash(* *)" ... same reassurance
Read(//**/**) denied 7/7 paths (incl. /x, /.env) ... same reassurance
Read(//**/*) denied 5/7 (misses only files directly at /) ... same reassurance
matchesCommandPattern('**'|'* *', c) = true for git status|echo hi|npm view foo|ls -la|cat /etc/hosts
candidate fix (consult the matcher) flips all four, 8 scoped controls unchanged:
Bash(**) true->false Bash(* *) true->false Read(//**/*) true->false Read(//**/**) true->false
unchanged scoped: Bash(npm view *), Bash(git *), Read(//**/node_modules/**), Read(/src/**),
Write(./dist/**), Read(/**), Write(**), Agent(model:*)
unchanged already-correct: Bash(*), Bash, Bash(), Read(//**), WebFetch -> false
Derive the answer from the matchers instead of listing spellings, and fail closed when the shape is unrecognisable. Putting the predicate in rule-parser.ts keeps it beside the semantics it describes: for command kind ask matchesCommandPattern(rule.specifier, <probe command>); for path kind run resolvePathPattern and ask matchesPathPattern against a probe path outside the project root; for domain and literal kinds return false, since matchesDomainPattern has no wildcard support at all (reported separately on the comment below).
Do not widen the path branch to a star-only test: resolvePathPattern maps only the // prefix to the filesystem root (rule-parser.ts:1214-1216, return specifier.substring(1);) while a prefix-less specifier falls through to toPosixPath(path.join(cwd, specifier)). Measured, with deny ['Read(/**)'], ['Read(./**)'], ['Read(**)'] and ['Write(**)'] a read or write of /etc/passwd evaluates to ask, not deny — those spellings are genuinely scoped, so classifying them scoped is correct today and a star-only test would break it. Also rule-parser.ts:981 (if (pattern === '*') { return true; }) is the only unconditional command catch-all, so Bash(*) must stay tool-wide.
Adding 'Bash(**)' and 'Read(//**/**)' to the raw list of the existing does not reassure for tool-wide catch-all deny rules (#11405) loop is red against the current literal comparison and green once the classifier consults the matcher; please confirm restoring specifier === '*' reds them again.
中文说明
isToolWideDenyRule 是通过与两个手写列出的拼写做精确字符串比较来判断「是否封禁整个工具」的，因此匹配器实际视为「匹配一切」的其他拼写会被判定为范围级，从而对一个已被完全封禁的工具仍然输出安抚句。
这一点承接上一轮审查中关于「靠原始字符串判断工具级还是调用级」的意见。被点名的入口已经关闭——Bash(*)、Read(//**)、WebFetch(*)、裸 Bash 与 Bash() 现在都能正确分类。剩下的是：这个面无法有限枚举，所以继续往列表里补拼写会不断留下缺口——Bash(**) 与 Bash(* *) 会编译成匹配任意命令的正则，Read(//**/*) 与 Read(//**/**) 会解析为 /**/* 与 /**/** 并匹配任意绝对路径，而它们都不等于 '*' 或 '//**'。在 permissions.deny: ["Bash(**)"] 下，每条 shell 调用都被拒并引用 Bash(**)，消息却仍以「本工具的其他用法仍被允许」结尾——这是一句确定的假话，会把模型送回 #11405 所描述的「反复重试一个已被完全封禁的工具」的循环，只是方向相反。建议从结构上关闭它，而不是逐个入口去补。
证据（在本 commit 上用真实 PermissionManager 经真实 evaluatePermissionFlow 驱动）：
Bash(**) denied 12/12 commands (incl. '', 'a', 'multi-line') cited="Bash(**)"
msg="... Matching deny rule: \"Bash(**)\". Other uses of this tool are still permitted."
Bash(* *) denied 12/12 cited="Bash(* *)" ... same reassurance
Read(//**/**) denied 7/7 paths (incl. /x, /.env) ... same reassurance
Read(//**/*) denied 5/7 (misses only files directly at /) ... same reassurance
matchesCommandPattern('**'|'* *', c) = true for git status|echo hi|npm view foo|ls -la|cat /etc/hosts
候选修复（改为询问匹配器）可翻转以上四项，且 8 个范围级对照保持不变：
Bash(**) true->false Bash(* *) true->false Read(//**/*) true->false Read(//**/**) true->false
范围级保持不变：Bash(npm view *)、Bash(git *)、Read(//**/node_modules/**)、Read(/src/**)、
Write(./dist/**)、Read(/**)、Write(**)、Agent(model:*)
原本已正确的保持不变：Bash(*)、Bash、Bash()、Read(//**)、WebFetch -> false
建议从匹配器推导答案而不是列举拼写，并在形状无法识别时 fail closed。把该谓词放进 rule-parser.ts 可以让它与它所描述的语义放在一起：command 类型询问 matchesCommandPattern(rule.specifier, <探测命令>)；path 类型先跑 resolvePathPattern，再用项目根之外的探测路径询问 matchesPathPattern；domain 与 literal 类型返回 false，因为 matchesDomainPattern 完全不支持通配（已就下方注释单独提出）。
不要把 path 分支放宽成「仅由星号组成」的判断：resolvePathPattern 只把 // 前缀映射到文件系统根（rule-parser.ts:1214-1216，return specifier.substring(1);），而无前缀的 specifier 会落到 toPosixPath(path.join(cwd, specifier))。实测在 deny 为 ['Read(/**)']、['Read(./**)']、['Read(**)']、['Write(**)'] 时，对 /etc/passwd 的读写判为 ask 而非 deny——这些拼写确实是范围级的，因此当前把它们判为范围级是正确的，而「仅星号」的判断会破坏这一点。另外 rule-parser.ts:981（if (pattern === '*') { return true; }）是唯一无条件的命令全量匹配，因此 Bash(*) 必须保持为工具级。
在现有 does not reassure for tool-wide catch-all deny rules (#11405) 循环的 raw 列表中加入 'Bash(**)' 与 'Read(//**/**)'，在当前的字面比较下是红的、在改为询问匹配器后是绿的；请确认恢复 specifier === '*' 后它们会再次变红。
— qwen3.8-max via Qwen Code /review (v0.23.1)
| // `*` is the documented catch-all for command and domain specifiers. | ||
| return rule.specifier === '*'; |
There was a problem hiding this comment.
[Suggestion] R1-3: This comment states a rule-grammar fact the engine does not implement. matchesDomainPattern has no wildcard support, so WebFetch(*) can never deny anything and findMatchingDenyRule can never return it — the domain leg of this branch is unreachable through the real engine.
matchesDomainPattern strips an optional domain: prefix and then does exact-or-subdomain comparison only. A maintainer who trusts this new in-repo comment writes deny: ["WebFetch(*)"] to block fetching and gets no denial at all: web_fetch proceeds to every domain while they believe it is blocked. The genuinely tool-wide form is the bare WebFetch, handled by the !rule.specifier branch — and it is the one form the new test does not cover. The new comment lines are the only place in the repository asserting that grammar; the user docs offer only bare WebFetch, WebFetch(api.example.com) and WebFetch(malicious.com). The third entry of the new test loop passes only because findMatchingDenyRule is mocked to hand back that string, so it pins a classification for an input production cannot produce and reports domain coverage the suite does not have.
Witness (executed at this commit):
matchesDomainPattern('*'|'domain:*', 'example.com'|'sub.example.com'|'other.org') = false (all 6)
matchesRule(parseRule('WebFetch(*)'), 'web_fetch', domain='example.com') = false
real PM: deny ['WebFetch(*)'] -> evaluate=default, cited=undefined
deny ['WebFetch(domain:*)'] -> evaluate=default, cited=undefined
deny ['WebFetch'] -> evaluate=deny, cited="WebFetch"
literal-kind sibling on this same branch:
deny ['Skill(*)'] -> evaluate=default, cited=undefined
deny ['Skill'] -> evaluate=deny, cited="Skill"
| // `*` is the documented catch-all for command and domain specifiers. | |
| return rule.specifier === '*'; | |
| // `*` is the documented catch-all for command specifiers. The domain | |
| // matcher has no wildcard support, so bare `WebFetch` is the tool-wide form. | |
| return rule.specifier === '*'; |
Also replace 'WebFetch(*)' with the bare 'WebFetch' in the loop at permissionFlow.test.ts:122, which pins the !rule.specifier branch on an input the matcher can actually produce. If a wildcard domain deny is genuinely wanted, that is a separate change to matchesDomainPattern with its own test, not something this classifier should assume.
With 'WebFetch' (bare) in that loop, deleting the true result the !rule.specifier branch returns for bare names must red that iteration.
中文说明
这条注释陈述了一个引擎并未实现的规则语法事实。matchesDomainPattern 不支持通配，因此 WebFetch(*) 永远无法拒绝任何请求，findMatchingDenyRule 也永远不可能返回它——该分支的 domain 一侧在真实引擎中不可达。
matchesDomainPattern 只会剥掉可选的 domain: 前缀，然后做精确匹配或子域匹配。若维护者相信这条新增的仓库内注释，写下 deny: ["WebFetch(*)"] 想封禁抓取，结果是一条都不会被拒：web_fetch 对任意域名照常放行，而他们认为已经封住了。真正工具级的形式是裸 WebFetch，由 !rule.specifier 分支处理——而这恰恰是新增测试没有覆盖的那一种。这几行新注释是全仓库唯一断言该语法的地方；用户文档只提供裸 WebFetch、WebFetch(api.example.com) 与 WebFetch(malicious.com)。新测试循环的第三项之所以通过，只是因为 findMatchingDenyRule 被 mock 成返回该字符串，因此它固定的是生产环境不可能产生的输入的分类，并对外宣称了套件其实并不具备的 domain 覆盖。
证据（在本 commit 上执行）：
matchesDomainPattern('*'|'domain:*', 'example.com'|'sub.example.com'|'other.org') = false (all 6)
matchesRule(parseRule('WebFetch(*)'), 'web_fetch', domain='example.com') = false
real PM: deny ['WebFetch(*)'] -> evaluate=default, cited=undefined
deny ['WebFetch(domain:*)'] -> evaluate=default, cited=undefined
deny ['WebFetch'] -> evaluate=deny, cited="WebFetch"
同一分支上的 literal 类型同类情况：
deny ['Skill(*)'] -> evaluate=default, cited=undefined
deny ['Skill'] -> evaluate=deny, cited="Skill"
同时请把 permissionFlow.test.ts:122 循环中的 'WebFetch(*)' 换成裸 'WebFetch'，这样固定的才是匹配器确实能产生的输入所对应的 !rule.specifier 分支。如果真的需要通配域名封禁，那应当是对 matchesDomainPattern 的独立改动并配独立测试，而不是由这个分类函数去假定。
在该循环中使用裸 'WebFetch' 后，删除 !rule.specifier 分支对裸名返回 true 的行为，必须使该次迭代变红。
— qwen3.8-max via Qwen Code /review (v0.23.1)
| } | ||
| }); | ||
|
|
||
| it('cites the deny rule for a compound command segment', () => { |
There was a problem hiding this comment.
[Suggestion] R1-7: This test passes equally well if the per-segment recursion it exists to cover is replaced by a flat matchesRule loop, so the one behaviour the recursion adds — a per-segment shell virtual-op pass — is unpinned, even though the in-diff comment at permission-manager.ts:953-954 claims "nested compounds and per-segment virtual ops are covered".
splitCompoundCommand is already flat (it splits on every unquoted operator), so re-splitting a segment yields nothing new and the recursion differs from a flat loop only by re-running extractShellOperationsAcrossCommand per segment. With cwd /project and deny ['Read(//project/node_modules/**)'], the command cd /tmp && cat node_modules/lodash/index.js is cited only because of that per-segment pass. Replacing the recursion with a flat loop — the obvious simplification a future reader makes, since the tested Bash(npm view *) case does not tell the two apart — leaves the whole suite green while the citation becomes undefined and the user-visible message loses the rule entirely, even though evaluate() still denies. That is the #11405 symptom returning with a green suite. Separately, neither new test asserts the coupled invariant that evaluate() actually denies the context the rule is cited for, so a future change that makes evaluate() stop denying these shapes leaves these tests asserting a message no user can ever see.
Witness (flat-loop mutant executed):
MUTANT src/permissions/permission-manager.test.ts (422 tests) PASS <- both new tests green
src/core/permissionFlow.test.ts (31 tests) PASS
src/permissions + src/memory + permissionFlow: 1607 passed
PROBE INTACT: evaluate=deny citation="Read(//project/node_modules/**)"
MUTANT: evaluate=deny citation=undefined <- probe flips
E2E INTACT denyMessage="... Matching deny rule: \"Read(//project/node_modules/**)\". Other uses ..."
MUTANT denyMessage="This \"run_shell_command\" invocation was denied by permission rules."
pass attribution: deny Read(//tmp/node_modules/**) -> FULL-COMMAND cd-tracked pass cites it (unchanged under mutant)
deny Read(//project/node_modules/**) -> only the PER-SEGMENT pass can cite it (undefined under mutant)
Add a compound case whose deciding rule is a path rule reached only via a later segment's virtual op (the cd /tmp && cat node_modules/... shape, with projectRoot/cwd set via makeConfig), and assert await expect(pm.evaluate(ctx)).resolves.toBe('deny') beside the citation in both new tests so the decision and the citation are pinned together. The comment at permission-manager.ts:953-954 is also half-overstated: "nested compounds" adds nothing over the flat split, "per-segment virtual ops" is the load-bearing half.
evaluateCompoundCommand builds each segment context as const subCtx: PermissionCheckContext = { ...ctx, command: subCmd }; (permission-manager.ts:573) and evaluateSingle re-runs the virtual-op extraction per segment with pathCtx?.cwd ?? process.cwd() (permission-manager.ts:454-465), so the new expectation must mirror that un-tracked per-segment cwd, not the cd-tracked cwd of the full-command pass.
The new case must go red (citation undefined instead of the Read(...) raw) when the recursive this.findMatchingDenyRule({ ...ctx, command: subCmd }) call is replaced by a flat per-segment matchesRule loop, and the added evaluate() assertion must go red if the compound pass is removed while the citation still resolves.
中文说明
如果把本测试所要覆盖的逐段递归换成一个扁平的 matchesRule 循环，本测试同样会通过；因此递归真正新增的那一点行为——逐段的 shell 虚拟操作 pass——并没有被固定，尽管 diff 中 permission-manager.ts:953-954 的注释声称「嵌套复合命令与逐段虚拟操作都已覆盖」。
splitCompoundCommand 本身已经是扁平的（它会在每个未被引号包裹的操作符处切分），因此对片段再次切分不会产生任何新东西，递归与扁平循环的唯一差别就是对每段重跑 extractShellOperationsAcrossCommand。设 cwd 为 /project、deny 为 ['Read(//project/node_modules/**)']，命令 cd /tmp && cat node_modules/lodash/index.js 之所以能被引用，正是因为这个逐段 pass。把递归换成扁平循环——这是未来读者很容易做出的「简化」，因为已测试的 Bash(npm view *) 用例分辨不出两者——整个套件仍是绿的，而引用会变成 undefined、用户可见的消息完全失去规则名，尽管 evaluate() 仍然判为拒绝。那就是 #11405 的症状在套件全绿的情况下回归。另外，两个新增测试都没有断言「evaluate() 确实会对被引用规则所对应的上下文判为拒绝」这一耦合不变量，因此将来若 evaluate() 不再对这些形状判拒，这些测试仍会断言一条用户永远看不到的消息。
证据（已执行扁平循环变异体）：
MUTANT src/permissions/permission-manager.test.ts (422 tests) PASS <- 两个新测试仍绿
src/core/permissionFlow.test.ts (31 tests) PASS
src/permissions + src/memory + permissionFlow: 1607 passed
PROBE INTACT: evaluate=deny citation="Read(//project/node_modules/**)"
MUTANT: evaluate=deny citation=undefined <- 探测翻转
E2E INTACT denyMessage="... Matching deny rule: \"Read(//project/node_modules/**)\". Other uses ..."
MUTANT denyMessage="This \"run_shell_command\" invocation was denied by permission rules."
pass 归因：deny Read(//tmp/node_modules/**) -> 由完整命令的 cd 跟踪 pass 引用（变异下不变）
deny Read(//project/node_modules/**) -> 只有逐段 pass 能引用（变异下为 undefined）
建议补一个复合用例，其决定性规则是只能经由后一段的虚拟操作命中的路径规则（即 cd /tmp && cat node_modules/... 这种形状，通过 makeConfig 设置 projectRoot/cwd），并在两个新测试中于引用旁边断言 await expect(pm.evaluate(ctx)).resolves.toBe('deny')，把决定与引用一起固定。permission-manager.ts:953-954 的注释也有一半言过其实：「嵌套复合命令」相对扁平切分没有新增任何东西，「逐段虚拟操作」才是承重的那一半。
evaluateCompoundCommand 以 const subCtx: PermissionCheckContext = { ...ctx, command: subCmd }; 构造每段上下文（permission-manager.ts:573），evaluateSingle 则以 pathCtx?.cwd ?? process.cwd() 对每段重跑虚拟操作提取（permission-manager.ts:454-465），因此新增断言必须对应这种未做 cd 跟踪的逐段 cwd，而不是完整命令 pass 的 cd 跟踪 cwd。
当递归调用 this.findMatchingDenyRule({ ...ctx, command: subCmd }) 被替换为扁平的逐段 matchesRule 循环时，新用例必须变红（引用变为 undefined 而不是 Read(...) 原始串）；而当复合 pass 被移除但引用仍可解析时，新增的 evaluate() 断言必须变红。
— qwen3.8-max via Qwen Code /review (v0.23.1)
| // though the deny rule's toolName (e.g. `read_file`) never matches the | ||
| // shell tool name. Without this pass the citation silently drops. | ||
| if (SHELL_TOOL_NAMES.has(toolName) && command !== undefined) { | ||
| const cwdForOps = pathCtx?.cwd ?? process.cwd(); |
There was a problem hiding this comment.
[Suggestion] R1-10: The cwd resolution in the new virtual-op citation pass is unpinned. Changing pathCtx?.cwd ?? process.cwd() to plain process.cwd() leaves all 453 tests in the two changed files green, so a regression here silently loses the citation for relative paths.
The added test cites through an absolute path (cat /app/node_modules/lodash/index.js), so the cwd never enters into it. If the pathCtx?.cwd half is later dropped, a shell command with a relative path (cat secret.txt) evaluated where the permission cwd differs from the CLI process cwd resolves against the wrong directory, the Read/Edit/Write rule misses, and findMatchingDenyRule returns undefined. The denial still happens — evaluate() resolves with the correct cwd — so the user sees a deny whose message has silently lost its Matching deny rule: "..." citation: the #11405 symptom, back for relative paths, with no red test.
Witness (mutation probe, measured):
mutant: `pathCtx?.cwd ?? process.cwd()` -> `process.cwd()`
both changed test files: 453/453 GREEN (mutant survived)
witness test: makeConfig({ permissionsDeny: ['Read(//project/secret.txt)'], cwd: '/project' })
command 'cat secret.txt'
INTACT: passes (permission-manager.test.ts 423/423)
MUTANT: RED, received undefined
Add a case to the PermissionManager.findMatchingDenyRule block using makeConfig({ permissionsDeny: ['Read(//project/secret.txt)'], cwd: '/project' }) and command: 'cat secret.txt', asserting the citation is 'Read(//project/secret.txt)'. makeConfig already supports it (getCwd: () => opts.cwd ?? '/project', permission-manager.test.ts:1717).
evaluate() resolves the same extractor with const cwd = pathCtx?.cwd ?? process.cwd(); (permission-manager.ts:324), and the comment above it (lines 305-313) makes that pass "the single source of truth for cd tracking", citing deny: ["Write(.qwen/settings.json)"] having to match cd .qwen && bash -lc 'echo > settings.json' — so the citation pass must keep resolving with the identical cwd, or the deny and its citation diverge.
That case passes on the PR's code and goes red (received undefined) when cwdForOps becomes process.cwd() — please confirm that red before landing it.
中文说明
新增虚拟操作引用 pass 中的 cwd 解析没有被测试固定。把 pathCtx?.cwd ?? process.cwd() 改成单纯的 process.cwd()，两个被改文件中的全部 453 个测试仍然是绿的，因此这里一旦回归，相对路径的引用会悄无声息地丢失。
新增测试是通过绝对路径来引用的（cat /app/node_modules/lodash/index.js），所以 cwd 根本不会参与。若日后 pathCtx?.cwd 那一半被去掉，当权限 cwd 与 CLI 进程 cwd 不同时，带相对路径的 shell 命令（cat secret.txt）会按错误的目录解析，Read/Edit/Write 规则落空，findMatchingDenyRule 返回 undefined。拒绝本身仍然发生——evaluate() 用的是正确的 cwd——于是用户看到一次拒绝，而消息里悄悄丢掉了 Matching deny rule: "..." 引用：#11405 的症状在相对路径上回归，且没有任何测试变红。
证据（变异探测，实测）：
变异体：`pathCtx?.cwd ?? process.cwd()` -> `process.cwd()`
两个被改测试文件：453/453 绿（变异体存活）
witness 测试：makeConfig({ permissionsDeny: ['Read(//project/secret.txt)'], cwd: '/project' })
command 'cat secret.txt'
未变异：通过（permission-manager.test.ts 423/423）
变异后：红，received undefined
建议在 PermissionManager.findMatchingDenyRule 区块中补一个用例，使用 makeConfig({ permissionsDeny: ['Read(//project/secret.txt)'], cwd: '/project' }) 与 command: 'cat secret.txt'，断言引用为 'Read(//project/secret.txt)'。makeConfig 已经支持（getCwd: () => opts.cwd ?? '/project'，permission-manager.test.ts:1717）。
evaluate() 用 const cwd = pathCtx?.cwd ?? process.cwd(); 解析同一个提取器（permission-manager.ts:324），其上方注释（305-313 行）把该 pass 定为「cd 跟踪的唯一真相来源」，并举例 deny: ["Write(.qwen/settings.json)"] 必须能匹配 cd .qwen && bash -lc 'echo > settings.json'——因此引用 pass 必须使用完全相同的 cwd 解析，否则拒绝与其引用会发生分歧。
该用例在 PR 代码上通过，而在 cwdForOps 变成 process.cwd() 时变红（received undefined）——请在合入前确认它确实变红。
— qwen3.8-max via Qwen Code /review (v0.23.1)
| for (const rule of denyRules) { | ||
| if ( | ||
| matchesRule(rule, ...opMatchArgs, undefined, undefined, 'canonical') | ||
| ) { | ||
| return rule.raw; | ||
| } | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-6: Composing a one-line deny citation now costs about 96% of a second evaluate(), because the two new passes re-run the whole shell analysis on the same context that evaluate() just analysed — and all of it is synchronous, on the CLI event loop, inside the same denied call.
permission-helpers.ts:136 runs await pm.evaluate(pmCtx) and permissionFlow.ts:107 then runs pm?.findMatchingDenyRule(pmCtx) on the same pmCtx. The dominant cost is this ops x denyRules loop, each pair paying a realpathNearestExisting syscall walk (rule-parser.ts:1316-1358) plus a freshly compiled picomatch matcher with no memoization (rule-parser.ts:1256-1273). A denied call with a large compound command and several path deny rules stalls the TUI for hundreds of milliseconds purely to build denyMessage. This is bounded, which is why it is a Suggestion rather than a blocker: everyday shapes cost 0.03-8ms, and the other two call sites pass no command, gating both new passes off entirely.
Witness (measured at this commit, best-of-3; 10 x Read(//**/neverN/**), 200 cat /nonexistentN/file.txt segments joined by &&):
INTACT find=150.4ms evaluate=156.3ms extract=0.96ms split=0.37ms ops=200 segs=200
1 path rule: find=18.0ms evaluate=23.1ms
Bash(npm view *): find=5.06ms
'cd /tmp && npm view foo' (11 rules): find=0.0322ms cite="Bash(npm view *)"
M0 (both new passes removed = pre-diff shape): find=0.0ms evaluate=162.3ms ; short case cite=undefined
M1 (recursion only, virtual-op pass removed): find=3.4ms
M2 (full-command virtual-op pass only): find=70.3ms
scale sweep: rules=10 -> 5seg=3.84 25seg=20.29 50seg=40.84 100seg=80.24 200seg=170.56ms
rules=20 -> 200seg=320.93ms ; existing-path arm 200seg x 10rules = 95.29ms
Stop paying for the analysis twice: memoize the pure extractShellOperationsAcrossCommand(command, cwd) result in a size-bounded LRU keyed by cwd + '\0' + command so evaluate() and the citation share one parse, and/or cache the canonical-path candidates and picomatch matchers that matchesPathPattern rebuilds per call.
Dropping the per-segment virtual-op pass is not a safe shortcut, and this was confirmed by execution rather than assumed: the flat mutant removed exactly that pass and the citation for cd /tmp && cat node_modules/lodash/index.js went from Read(//project/node_modules/**) to undefined while evaluate() still returned deny, because evaluate()'s own per-segment pass (permission-manager.ts:456-470 -> evaluateShellVirtualOps) resolves each segment against the original pathCtx.cwd. Any memoization must keep per-segment resolution, and a cache keyed on command strings must be size-bounded because those strings are model- and user-controlled.
The two tests this diff adds must stay green under any restructure: cites the deny rule for a compound command segment pins the segment pass and cites the deny rule when a shell command is denied via a virtual file op pins the ops pass. For the memo, please also add an assertion that it is keyed on the parse only — two calls with the same command against different deny-rule sets must each return their own rule — and confirm that assertion reds if the cache is keyed on the outcome instead.
中文说明
现在拼装一行拒绝引用的开销约等于再跑一次 evaluate() 的 96%，因为两个新增 pass 会在 evaluate() 刚刚分析过的同一上下文上把整套 shell 分析重跑一遍——而且全部是同步的，运行在 CLI 事件循环上，处于同一次被拒调用之内。
permission-helpers.ts:136 执行 await pm.evaluate(pmCtx)，随后 permissionFlow.ts:107 在同一个 pmCtx 上执行 pm?.findMatchingDenyRule(pmCtx)。主要开销是这个 ops x denyRules 循环：每一对都要付一次 realpathNearestExisting 系统调用行走（rule-parser.ts:1316-1358），外加一次未经记忆化的、现场编译的 picomatch 匹配器（rule-parser.ts:1256-1273）。当一次被拒调用带有大型复合命令和若干路径类 deny 规则时，仅仅为了构造 denyMessage 就会让 TUI 卡住数百毫秒。这是有边界的，因此定为建议而非阻塞项：日常形状耗时 0.03-8ms，而另外两个调用点不传 command，两个新 pass 在它们那里完全被关闭。
证据（在本 commit 上实测，取三次最优；10 条 Read(//**/neverN/**)，200 段以 && 连接的 cat /nonexistentN/file.txt）：
INTACT find=150.4ms evaluate=156.3ms extract=0.96ms split=0.37ms ops=200 segs=200
1 path rule: find=18.0ms evaluate=23.1ms
Bash(npm view *): find=5.06ms
'cd /tmp && npm view foo' (11 rules): find=0.0322ms cite="Bash(npm view *)"
M0（移除两个新 pass = 改动前形状）：find=0.0ms evaluate=162.3ms；短用例 cite=undefined
M1（仅保留递归，移除虚拟操作 pass）：find=3.4ms
M2（仅保留完整命令虚拟操作 pass）： find=70.3ms
规模扫描：rules=10 -> 5seg=3.84 25seg=20.29 50seg=40.84 100seg=80.24 200seg=170.56ms
rules=20 -> 200seg=320.93ms；路径真实存在的对照 200seg x 10rules = 95.29ms
建议不要把同一份分析付两次：把纯函数 extractShellOperationsAcrossCommand(command, cwd) 的结果放进一个有大小上限的 LRU（键为 cwd + '\0' + command），让 evaluate() 与引用共享一次解析；和/或缓存 matchesPathPattern 每次调用都重建的规范路径候选与 picomatch 匹配器。
去掉逐段虚拟操作 pass 并不是安全的捷径，这一点是经执行确认而非假定的：扁平变异体恰好移除了该 pass，于是 cd /tmp && cat node_modules/lodash/index.js 的引用从 Read(//project/node_modules/**) 变成 undefined，而 evaluate() 仍返回 deny——因为 evaluate() 自己的逐段 pass（permission-manager.ts:456-470 -> evaluateShellVirtualOps）会按原始 pathCtx.cwd 解析每一段。任何记忆化都必须保留逐段解析，且以命令字符串为键的缓存必须有大小上限，因为这些字符串由模型和用户控制。
本 diff 新增的两个测试在任何重构下都必须保持绿色：cites the deny rule for a compound command segment 固定逐段 pass，cites the deny rule when a shell command is denied via a virtual file op 固定 ops pass。对于记忆化方案，请再补一条断言说明它只以解析结果为键——同一命令面对不同的 deny 规则集合时，两次调用必须各自返回自己的规则——并确认若缓存改为以输出为键，该断言会变红。
— qwen3.8-max via Qwen Code /review (v0.23.1)
| for (const subCmd of subCommands) { | ||
| const rule = this.findMatchingDenyRule({ ...ctx, command: subCmd }); |
There was a problem hiding this comment.
[Suggestion] R1-5: This recursion re-enters the public method, so normalizePermissionContext runs again on each segment, while evaluate()'s compound path matches segments verbatim after normalizing once. For the monitor tool a segment that is itself a shell wrapper gets stripped a second time, so the citation can name a rule that did not decide the denial.
With deny ['Bash(rm *)','Bash(touch *)'] and the monitor invocation bash -lc 'bash -c "rm y" && touch x': normalizePermissionContext unwraps the outer wrapper once, evaluate() splits and matches the segments as written, so Bash(touch *) decides the deny. The recursion then re-enters with the segment bash -c "rm y", which normalizeMonitorCommand strips again to rm y, so Bash(rm *) matches first and is cited. The user reads Matching deny rule: "Bash(rm *)" for a denial Bash(touch *) decided, deletes that rule, re-runs the identical command and is denied again by the rule that was never named. The same shape via run_shell_command, which does not normalize, correctly cites Bash(touch *). The reachable surface is narrow — toolName === 'monitor' with a wrapper nested inside a compound — but what it produces is a wrongly named rule, not merely a missing one, and it is new relative to base.
Witness (executed at this commit):
normalizeMonitorCommand("bash -lc 'foo && bash -c \"rm y\"'").safetyCommand = "foo && bash -c \"rm y\""
normalizeMonitorCommand("foo && bash -c \"rm y\"").safetyCommand = "foo && bash -c \"rm y\"" -> idempotent on a non-wrapper
normalizeMonitorCommand("bash -c \"rm y\"").safetyCommand = "rm y" -> SEGMENT stripped again
normalizeMonitorCommand("rm y").safetyCommand = "rm y" -> idempotent
end-to-end via evaluatePermissionFlow, monitor, deny ['Bash(rm *)','Bash(touch *)'],
command `bash -lc 'bash -c "rm y" && touch x'`:
PR: FLOW=deny msg="This \"monitor\" invocation was denied by permission rules. Matching deny rule: \"Bash(rm *)\". Other uses of this tool are still permitted."
BASE: FLOW=deny msg="Tool \"monitor\" is denied by permission rules." (no rule cited)
same shape via run_shell_command (no normalization): PR cited="Bash(touch *)" <- the deciding rule
Normalize once: factor the three passes into a private findMatchingDenyRuleNormalized(ctx) that does not call normalizePermissionContext, have the public method normalize and delegate, and have the recursion call the private form with { ...ctx, command: subCmd }. That closes this wrong citation and the identical divergence in the two sibling methods at once.
hasRelevantRules (permission-manager.ts:1112-1118) and hasMatchingAskRule (:1207-1213) recurse through their own public entry points and carry the identical double normalization, so fixing only findMatchingDenyRule leaves the relevance check and the citation disagreeing for monitor commands — the three either get the same treatment or the divergence is accepted knowingly. evaluate()'s own compound path (evaluateCompoundCommand -> evaluateSingle, permission-manager.ts:572-576) matches segments verbatim, which is the behaviour the citation must mirror.
A new case in the describe('PermissionManager.findMatchingDenyRule') block — toolName 'monitor', deny ['Bash(rm *)','Bash(touch *)'], command bash -lc 'bash -c "rm y" && touch x', asserting the citation is 'Bash(touch *)' — is red with the double normalization and green once the recursion stops re-normalizing; please confirm that red.
中文说明
该递归重新进入公有方法，因此 normalizePermissionContext 会在每一段上再跑一次；而 evaluate() 的复合路径是规范化一次之后按原样匹配各段的。对 monitor 工具而言，若某一段本身就是 shell 包装器，它会被第二次剥壳，于是引用可能指向一条并未做出该拒绝决定的规则。
设 deny 为 ['Bash(rm *)','Bash(touch *)']，monitor 调用为 bash -lc 'bash -c "rm y" && touch x'：normalizePermissionContext 只剥一次外层包装，evaluate() 切分后按原样匹配各段，因此是 Bash(touch *) 决定了拒绝。递归随后以片段 bash -c "rm y" 重新进入，normalizeMonitorCommand 会把它再剥成 rm y，于是 Bash(rm *) 先命中并被引用。用户读到的是 Matching deny rule: "Bash(rm *)"，而实际决定拒绝的是 Bash(touch *)；他删掉那条规则、重跑完全相同的命令，又会被那条从未被点名的规则拒绝。同样的形状若走 run_shell_command（不做规范化），则会正确引用 Bash(touch *)。可触发面较窄——需要 toolName === 'monitor' 且复合命令中嵌套包装器——但它产生的是「引用了错误的规则」，而不仅仅是「缺少引用」，且相对基线是新出现的。
证据（在本 commit 上执行）：
normalizeMonitorCommand("bash -lc 'foo && bash -c \"rm y\"'").safetyCommand = "foo && bash -c \"rm y\""
normalizeMonitorCommand("foo && bash -c \"rm y\"").safetyCommand = "foo && bash -c \"rm y\"" -> 对非包装器幂等
normalizeMonitorCommand("bash -c \"rm y\"").safetyCommand = "rm y" -> 片段被再次剥壳
normalizeMonitorCommand("rm y").safetyCommand = "rm y" -> 幂等
经 evaluatePermissionFlow 端到端，monitor，deny ['Bash(rm *)','Bash(touch *)']，
command `bash -lc 'bash -c "rm y" && touch x'`：
PR: FLOW=deny msg="This \"monitor\" invocation was denied by permission rules. Matching deny rule: \"Bash(rm *)\". Other uses of this tool are still permitted."
BASE: FLOW=deny msg="Tool \"monitor\" is denied by permission rules."（未引用规则）
同一形状走 run_shell_command（不做规范化）：PR cited="Bash(touch *)" <- 决定性规则
建议只规范化一次：把三个 pass 抽成私有的 findMatchingDenyRuleNormalized(ctx)（不调用 normalizePermissionContext），公有方法负责规范化并委托，递归则以 { ...ctx, command: subCmd } 调用私有形式。这样能同时关闭此处的错误引用以及两个同类方法中完全相同的分歧。
hasRelevantRules（permission-manager.ts:1112-1118）与 hasMatchingAskRule（:1207-1213）也是通过各自的公有入口递归，携带完全相同的双重规范化，因此只修 findMatchingDenyRule 会让相关性检查与引用在 monitor 命令上继续不一致——三者要么同样处理，要么明确接受这一分歧。evaluate() 自己的复合路径（evaluateCompoundCommand -> evaluateSingle，permission-manager.ts:572-576）是按原样匹配片段的，这正是引用必须镜像的行为。
在 describe('PermissionManager.findMatchingDenyRule') 区块中补一个用例——toolName 为 'monitor'、deny 为 ['Bash(rm *)','Bash(touch *)']、command 为 bash -lc 'bash -c "rm y" && touch x'，断言引用是 'Bash(touch *)'——在双重规范化下是红的，在递归不再重复规范化后是绿的；请确认该红。
— qwen3.8-max via Qwen Code /review (v0.23.1)
Bring the branch up to date with main to pick up the lint-gate freshness check and the App.test.tsx split-session mock fix. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmttqhfdot6
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
8 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-1 packages/core/src/core/permissionFlow.ts:148 catch-all spelling classifier — already reported (comment 3964391128), still stands, re-confirmed this round by an executed sweep
- R1-3 packages/core/src/core/permissionFlow.ts:147 domain catch-all comment/test — already reported (comment 3964391132), still stands, re-confirmed this round
- R1-5 packages/core/src/permissions/permission-manager.ts:959 monitor double normalization — already reported (comment 3964391163), still stands, re-confirmed this round with a stronger discriminator
- R1-6 packages/core/src/permissions/permission-manager.ts:929 duplicate traversal cost — already reported (comment 3964391157), still stands, re-measured this round
- R1-7 packages/core/src/permissions/permission-manager.test.ts:3851 compound recursion test strength — already reported (comment 3964391145), still stands on byte-identical code
- R1-9 packages/core/src/core/permissionFlow.test.ts:121 unpinned no-specifier branch — already reported (comment 3964391122), still stands, mutation re-run this round
- R1-10 packages/core/src/permissions/permission-manager.ts:930 unpinned cwd resolution — already reported (comment 3964391153), still stands on byte-identical code
- R1-12 packages/core/src/core/permissionFlow.ts:117 cited-rule-vs-invoked-tool gate — already reported (comment 3964391116), still stands, both directions measured this round
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 8 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.23.1)
| * `Bash(npm view *)` is scoped and returns false. | ||
| */ | ||
| function isToolWideDenyRule(raw: string): boolean { | ||
| const rule = parseRule(raw); |
There was a problem hiding this comment.
[Suggestion] R2-2: isToolWideDenyRule assumes the string it is handed is a permission rule, but two PermissionManager implementations in this repo return prose from findMatchingDenyRule instead — ManagedAutoMemory(...) (packages/core/src/memory/memory-scoped-agent-config.ts:432-437) and ManagedSkillReview(...) (skillReviewAgentPlanner.ts:244-247). parseRule reads such a string as a literal rule whose prose colon becomes a key:value param matcher, so specifier is undefined, toolParamMatchers is non-empty, and the classifier answers "scoped" for every one of them.
That is only wrong when a base rule blocks the tool outright, and then it is wrong in the direction this diff exists to remove. With permissions.deny: ["Bash"] — a documented knob, not an exotic one — the auto-memory extractor runs as a forked agent whose getPermissionManager() is the scoped shim and whose own policy allows a read-only ls. The base rule denies, getScopedDenyRule returns its label unconditionally and ahead of basePm, and the agent is told other uses of the shell are still permitted while every one of them is denied. It retries until its budget (maxTurns: 5, maxTimeMinutes: 2) is spent and the extraction fails. The Edit/Write family and the skill-review shim reproduce it. The comment this diff adds at permissionFlow.ts:111-115 says tool-wide blocks "must NOT" get this reassurance; in these rows the tool is blocked entirely and the reassurance is appended anyway.
Witness:
A/B, same input, both trees (BASE = merge base 1919ff97f5; arm proven - the
reassurance string and isToolWideDenyRule are both absent from the BASE dist)
BASE deny[Bash] mem-scoped 'ls'
[deny] Tool "run_shell_command" is denied by permission rules.
Matching deny rule: "ManagedAutoMemory(run_shell_command: read-only only)".
PR deny[Bash] mem-scoped 'ls'
[deny] This "run_shell_command" invocation was denied by permission rules.
Matching deny rule: "ManagedAutoMemory(run_shell_command: read-only only)".
Other uses of this tool are still permitted.
PR deny[Bash] otherShellUses = { ls: deny, echo hi: deny, git status: deny,
cat /etc/hosts: deny, pwd: deny } <- 5/5 denied
PR deny[Edit] otherWriteUses = { a.md: deny, b.md: deny, c.ts: deny } <- 3/3 denied
scoped-only decision for 'ls' with NO base PM = allow <- the deny is the base rule's alone
22/22 real shim labels draw the reassurance
finalPermission = [deny] on BOTH arms for all 3 rows <- enforcement unchanged
not reachable (second entrance closed): allowShell:false ->
isToolEnabled(shell) = false | registration = 'disabled',
so coreToolScheduler.ts:2546-2588 intercepts with a template that appends nothing
A fix must not require the cited rule's tool to equal the invoked tool: permission-manager.ts:931-947 deliberately cites a Read(...) rule for a run_shell_command call, and the new test at permission-manager.test.ts:3869-3878 pins that. The deciding base rule is also not recoverable from the returned string, because the shim returns its own label before consulting basePm. Note that the manager-side query proposed on the open R1-12 thread does not close this entrance either — that fix is explicitly optional-called because the shim does not implement listRules(), so it is skipped here.
The narrowest guard is to classify a citation as scoped only when it really is a rule for a known tool: after parseRule, return true when rule.invalid, or when rule.toolName is neither in TOOL_NAME_ALIASES nor mcp__-prefixed. Measured for discrimination against the real parseRule and TOOL_NAME_ALIASES: prose rejected 9/9, real rules accepted 10/10 (Bash(npm view *), Read(//**/node_modules/**), Bash(*), Bash, Bash(), Read(//**), WebFetch(*), Agent(model:opus), Bash(**), mcp__server__tool). The wider version is to have findMatchingDenyRule return the already-parsed PermissionRule, so no citation string is ever re-parsed as prose. A permissionFlow.test.ts case mocking findMatchingDenyRule to return 'ManagedAutoMemory(run_shell_command: disabled)' and asserting the message does not contain Other uses of this tool are still permitted is red today — that exact label produces the sentence — and green once the guard lands; please confirm that red.
中文说明
isToolWideDenyRule 假定传入的字符串是一条权限规则，但仓库中有两个 PermissionManager 实现从 findMatchingDenyRule 返回的是散文而非规则——ManagedAutoMemory(...)（packages/core/src/memory/memory-scoped-agent-config.ts:432-437）与 ManagedSkillReview(...)（skillReviewAgentPlanner.ts:244-247）。parseRule 会把这类字符串读成 literal 规则，其中的散文冒号被当成 key:value 参数匹配器，于是 specifier 为 undefined、toolParamMatchers 非空，分类函数对它们一律回答「范围级」。
只有当基础规则把整个工具封死时这才成为问题，而一旦如此，它错的方向正是本 diff 要消除的那个。设 permissions.deny: ["Bash"]（这是有文档的常规开关，并不冷门）：自动记忆抽取器以 forked agent 运行，其 getPermissionManager() 是作用域 shim，而它自身的策略对只读的 ls 是允许的。基础规则判拒，getScopedDenyRule 无条件地、且在 basePm 之前返回自己的标签，于是 agent 被告知「本工具的其他用法仍被允许」，而实际上每一种用法都被拒。它会不断重试直到预算耗尽（maxTurns: 5、maxTimeMinutes: 2），抽取任务失败。Edit/Write 系列与 skill-review shim 可复现同一问题。本 diff 在 permissionFlow.ts:111-115 新增的注释明确写着工具级封禁「绝不能」得到这句安抚；而在这些实测行里，工具已被完全封禁，安抚句却照样被追加。
修复不应要求「被引用规则的工具必须等于被调用的工具」：permission-manager.ts:931-947 是刻意为 run_shell_command 调用引用 Read(...) 规则的，且 permission-manager.test.ts:3869-3878 的新测试固定了该行为。决定性基础规则也无法从返回的字符串中恢复，因为 shim 在查询 basePm 之前就返回了自己的标签。另请注意：R1-12 那条未关闭评论中建议的 manager 侧查询同样无法关闭此入口——那个修复被明确写成可选调用，理由是 shim 没有实现 listRules()，因此在这里会被跳过。
最窄的守卫是：只有当引用确实是某个已知工具的规则时才判为范围级——在 parseRule 之后，当 rule.invalid 为真、或 rule.toolName 既不在 TOOL_NAME_ALIASES 中也不带 mcp__ 前缀时返回 true。用真实 parseRule 与 TOOL_NAME_ALIASES 实测其区分度：散文 9/9 被拒，真实规则 10/10 被接受（Bash(npm view *)、Read(//**/node_modules/**)、Bash(*)、Bash、Bash()、Read(//**)、WebFetch(*)、Agent(model:opus)、Bash(**)、mcp__server__tool）。更彻底的做法是让 findMatchingDenyRule 返回已解析的 PermissionRule，这样任何引用字符串都不会被再次当作散文解析。在 permissionFlow.test.ts 中补一个用例，把 findMatchingDenyRule mock 成返回 'ManagedAutoMemory(run_shell_command: disabled)'，断言消息不包含 Other uses of this tool are still permitted：该断言在当前代码下是红的（这个标签确实会产生该句），加上守卫后变绿——请确认这一红一绿。
— qwen3.8-max via Qwen Code /review (v0.23.1)
| const cwdForOps = pathCtx?.cwd ?? process.cwd(); | ||
| const ops = extractShellOperationsAcrossCommand(command, cwdForOps); | ||
| for (const op of ops) { | ||
| const opMatchArgs = [ |
There was a problem hiding this comment.
[Suggestion] R2-1: This new virtual-op citation pass is documented as covering Read/Edit/Write/WebFetch rules, but the only test that exercises it uses a single category — a Read op extracted from cat (permission-manager.test.ts:3866-3881). The domain and write/edit legs of opMatchArgs have no coverage, and nothing pins this pass's precedence against the direct Bash match it now runs ahead of.
The gap is not hypothetical: blanking one field of the tuple leaves the whole suite green while the citation silently disappears. A curl https://evil.com denial under deny: ['WebFetch(domain:evil.com)'] would then cite no rule, and permissionFlow.ts:108-110 degrades to a bare This "run_shell_command" invocation was denied by permission rules. with no Matching deny rule: clause — the unexplained denial #11405 is about, on exactly the network case this pass was added to cover.
Witness:
mutants run in an isolated tree at this commit; suite = both changed test files
INTACT curl ops = [{"virtualTool":"web_fetch","domain":"evil.com"}]
domain deny decision = deny | cited = "WebFetch(domain:evil.com)"
MUTANT op.domain -> undefined in opMatchArgs
Tests 453 passed (453) <- suite stays GREEN
domain deny decision = deny | cited = undefined <- citation gone
INTACT write ops = [{"virtualTool":"write_file","filePath":"/etc/motd"}]
write deny decision = deny | cited = "Write(//etc/**)"
MUTANT op.filePath -> undefined for write_file/edit/notebook_edit
Tests 453 passed (453) <- suite stays GREEN
write deny decision = deny | cited = undefined
precedence, unpinned either way: deny ['Bash(curl *)','WebFetch(domain:evil.com)']
intact -> cites "WebFetch(domain:evil.com)" regardless of rule order
domain mutant -> falls through and cites "Bash(curl *)"
coverage read off describe('PermissionManager.findMatchingDenyRule') (3761-3881):
Bash command rule, no-match, session rule, non-denied tool, bare-tool rule,
symlinked path (direct toolName:'edit', NOT this pass), compound segment,
one Read-via-cat virtual op. No domain case, no write/edit virtual-op case.
A citation test must use rule forms that match under exactly the fields evaluateShellVirtualOps supplies — evaluateSingle({ toolName: op.virtualTool, cwd, filePath, domain }) with no specifier, toolParams or command (permission-manager.ts:498-503) — or it will pin a citation evaluate() itself would never deny on.
One case per op category would close this: a domain case (deny: ['WebFetch(domain:evil.com)'] + curl https://evil.com/x → that raw string) and a write/edit case, plus one precedence case holding both a Bash deny rule and a matching virtual-op deny rule and asserting which raw string is cited. Blanking op.domain must turn the domain case red, and moving the virtual-op pass below the single-context match must turn the precedence case red — please confirm both.
中文说明
这段新增的虚拟操作引用 pass 在注释中声称覆盖 Read/Edit/Write/WebFetch 规则，但唯一真正执行到它的测试只用了一个类别——从 cat 提取出的 Read 操作（permission-manager.test.ts:3866-3881）。opMatchArgs 的 domain 一侧与 write/edit 一侧都没有覆盖，也没有任何测试固定该 pass 相对于它现在抢在前面的 Bash 直接匹配的顺序。
这个缺口不是假设性的：把该元组中的某一个字段置空，整个测试套件仍然是绿的，而引用会悄无声息地消失。此时在 deny: ['WebFetch(domain:evil.com)'] 下拒绝 curl https://evil.com，将不会引用任何规则，permissionFlow.ts:108-110 会退化成一句没有 Matching deny rule: 子句的 This "run_shell_command" invocation was denied by permission rules.——正是 #11405 所说的那种「无从解释的拒绝」，而且恰好发生在该 pass 被加入以覆盖的网络场景上。
引用测试必须使用在 evaluateShellVirtualOps 实际提供的字段下能够命中的规则形态——即 evaluateSingle({ toolName: op.virtualTool, cwd, filePath, domain })，不带 specifier、toolParams 与 command（permission-manager.ts:498-503）——否则固定的会是一条 evaluate() 自己根本不会据此判拒的引用。
每个操作类别补一个用例即可收口：一个 domain 用例（deny: ['WebFetch(domain:evil.com)'] + curl https://evil.com/x → 该原始串）、一个 write/edit 用例，再加一个同时持有 Bash 拒绝规则与命中的虚拟操作拒绝规则、并断言最终引用哪一条原始串的顺序用例。把 op.domain 置空必须使 domain 用例变红；把虚拟操作 pass 移到单一上下文匹配之后必须使顺序用例变红——请确认这两处红。
— qwen3.8-max via Qwen Code /review (v0.23.1)
| result.denyMessage = `Tool "${toolName}" is denied by permission rules.${ruleInfo}`; | ||
| // A specifier-scoped deny (e.g. `Bash(npm view *)`) blocks only this | ||
| // invocation, not the whole tool. Say so explicitly so the model does | ||
| // not abandon the tool entirely (issue #11405). Tool-wide catch-alls |
There was a problem hiding this comment.
[Suggestion] R2-3: This PR closes #11405 with Fixes #11405, but the second half of what that issue asks for has no tracker, and merging will remove the only one it has.
#11405 asks for two things: "The error message must be more informative and include pattern, event better if we can define custom message to tell why exactly this tool is prohibited." The diff delivers the first. The issue's own triage ruled on the second — "Supporting this needs a schema change to allow an object form for rules. Recommend splitting it into a separate type/feature-request issue so the message-accuracy bug above can land independently" — and this PR's Risk & Scope says it "should be tracked separately". Your comment on the issue offers to do that ("happy to track it separately if you'd like"), but it is the last comment in that thread and the reporter never replied, so the split was never made. On merge GitHub auto-closes #11405 and the per-rule-message half of the reporter's expectation is left with nowhere to go. Nothing downstream will notice, because this PR's Stage-1 triage already recorded the deferral as satisfied ("Deferring the feature request to its own issue matches what the linked issue's triage recommended").
Witness:
absence re-verified independently, including three falsification tests
the finder did not run:
gh search issues --repo QwenLM/qwen-code "11405" -> 1 result (the issue itself)
gh issue list --state all --search "created:>=2026-09-08"
-> 56 issues; matching permission|deny|denied|message|rule:
11442 (web-shell hover timestamp), 11405, 11345 (dws group/DM sources)
-> no feature request
keyword searches "custom deny message" / "per-rule message" /
"custom message deny rule" / "deny rule message" -> only #11405
author's issues since 2026-09-01 (21 listed) -> none on this topic
#11405 timeline cross-references -> #11411 (this PR), #585
nearest candidate #2819 "Permission Denial Tracking with Contextual Fallback"
-> asks for denial-COUNT tracking, not a per-rule message: not the tracker
last comment on #11405: author's own, 2026-09-08T22:28:57Z, no reply
The deferred half is a settings-schema change and should not be pulled into this message-accuracy PR: packages/cli/src/config/settingsSchema.ts declares deny as a plain string array, and interface PermissionRule in packages/core/src/permissions/types.ts has no message/reason/description field. So the ask here is only tracking, not code — file the type/feature-request issue and reference its number in Risk & Scope and in a closing comment on #11405, or say explicitly in that closing comment that the component is being dropped rather than deferred. Either closes the gap; this comment is anchored here only because this is the line that names the issue.
中文说明
本 PR 以 Fixes #11405 关闭该 issue，但那个 issue 诉求的后半部分没有任何跟踪项，而合并会把它仅有的那一个也带走。
#11405 提出两件事："The error message must be more informative and include pattern, event better if we can define custom message to tell why exactly this tool is prohibited." 本 diff 完成了第一件。该 issue 自己的 triage 对第二件给出了裁定——"Supporting this needs a schema change to allow an object form for rules. Recommend splitting it into a separate type/feature-request issue so the message-accuracy bug above can land independently"——而本 PR 的 Risk & Scope 也写着它 "should be tracked separately"。你在该 issue 下的评论表示愿意拆分（"happy to track it separately if you'd like"），但那是该讨论串的最后一条评论，报告者始终没有回复，因此拆分从未发生。合并后 GitHub 会自动关闭 #11405，报告者诉求中「per-rule 自定义消息」这一半就无处可去了。下游也不会有人察觉，因为本 PR 的 Stage-1 triage 已经把这次延后记录为已妥善处理（"Deferring the feature request to its own issue matches what the linked issue's triage recommended"）。
被延后的那一半属于 settings schema 变更，不应被拉进这个只修消息准确性的 PR：packages/cli/src/config/settingsSchema.ts 把 deny 声明为普通字符串数组，packages/core/src/permissions/types.ts 中的 interface PermissionRule 也没有 message/reason/description 字段。所以这里要求的只是跟踪、而非代码——建一个 type/feature-request issue，把它的编号写进 Risk & Scope 以及 #11405 的收尾评论；或者在那条收尾评论中明确说明该部分是被放弃而不是被延后。两种做法都能补上这个缺口；本条之所以锚定在这里，只是因为这一行提到了那个 issue。
— qwen3.8-max via Qwen Code /review (v0.23.1)
|
Independent verification — end-to-end deny-message probe with no mocks, plus a 5-arm mutation witness Reviewed at head
5 commits = 3 author commits ( The gap this closesThe two halves of this change are tested separately but never together. So I built that: an 11-cell probe wiring the real
C2 and C3 are the two headline claims (compound segment, and virtual file op reached through a shell command); both produce the correct citation and the correct reassurance. Measured message for C2, verbatim: Mutation witness — is any of the added code load-bearing?A green probe proves nothing on its own, so each added construct was disabled and the specific cell that should break was checked. Every arm's mutation was verified by re-reading the file after writing it (line index + a count of surviving guards), because a mutation that silently fails to apply produces a green run that reads as "this code does nothing" — the dangerous direction.
So every production line this PR adds is load-bearing, and each is witnessed twice — once by an independent probe and once by the test the PR itself added. Both mutated files were restored byte-identical (sha256 The unchanged producers, checked rather than assumedThe new code reads values produced by files this PR does not touch, so those were read too:
CI at head74 check-runs, all Non-blocking observations — both already filed by the automated review; recorded here only to bound themNeither of these is a Critical and neither should hold the merge. I am not claiming them as new findings:
The 11-command population was Two path spellings I expected to be misses and measured to be correct — recorded so a widening of the classifier does not "fix" them into a regression:
Both measured with Also already filed and not repeated here: Two further probes of mine came back void and are disclosed so nobody reads them as evidence in either direction: Instrument disclosure — no tmux run, deliberatelyA denied tool call is only reachable from a model-issued call, so a tmux session would add a live-model dependency in order to sample one rule spelling, and its observation (a string in the transcript) is exactly what the probe above already asserts against the real production functions. What tmux would add that the probe does not is the rendering step; that hop is two lines of direct code ( Conclusion: no Critical found. Both headline claims are confirmed end to end against the real permission stack, every added line is load-bearing under mutation, the unchanged producers line up, and CI is green at head — mergeable. This comment carries no approval. As of the state read immediately before posting it, our approve precondition (a previously posted report from us concluding the PR is mergeable) was not met, because this is that first report. |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Approving on the strength of the independent verification report posted at this same head (comment 5605213719), which recorded that its approve precondition was unmet in its own round precisely because it was the first report. This is that cash-in. It carries no new findings — nothing below changes the report's conclusion; it re-asserts the state the approval depends on, as of the state read immediately before posting.
Re-asserted at the minute of this write (head f69d917ccacd9b9ed45a3fdb859d19e210c10506, unmoved since the report):
- Lifecycle first:
state=open,merged=false. Checked before the head-match test, because a head-match test passes on a merged PR and so cannot discharge an owed write. - Review state at head:
qwen-code-ci-bot APPROVED(review5148988602) is that author's latest verdict-state row at head. Itscommit_id == headis a GitHub re-anchor, not proof the approver saw the final tree:submitted_atis 02:06:07Z while the head commit's committer date is 07:21:07Z, so the row was moved onto a commit that did not exist when the review was filed. The later at-head row5152816887(10:09:41Z) isCOMMENTED, which GitHub excludes fromreviewDecisionand which therefore does not supersede an approval — read rather than assumed: its embedded review ledger records{"round":2,"findings":[{"id":"R2-1","sev":"S",…},{"id":"R2-2","sev":"S",…},{"id":"R2-3","sev":"S",…}],"posted":3,"fresh":3,"prevPosted":8}, i.e. every finding Suggestion-level and zero Criticals, alongside 8 previously-reported Suggestions re-confirmed as still standing and deliberately not re-posted. NoCHANGES_REQUESTEDorDISMISSEDrow from any author at head. - What makes the re-anchor benign is parentage, not the head match: 5 commits = 3 author commits (
28b32bfb9a7a22:27:47Z,0cf2bc8e7d4500:12:12Z,475877f272d800:53:13Z) + 2Merge …maincommits, and the headf69d917ccacdhas parents475877f272d8(the last author commit) and1919ff97f5fb(amaincommit). Zero author commits after the approval, so the merge moved the head without moving the author's contribution. Every lane below ran againstrefs/pull/11411/head, i.e. the post-merge tree, so the main/PR interaction is what the green lanes actually covered. - CI at head, complete census:
total_count = 74, 74 received, 74 unique run ids, zero non-green and zero incomplete. Product lanes green (7):Test (ubuntu-latest, Node 22.x),Lint & Static (ubuntu-latest, Node 22.x),Integration Tests (no-AK, No Sandbox),Desktop Shell (ubuntu-22.04),Desktop Shell (windows-2022),web-shell E2E Smoke (ubuntu-latest, Node 22.x),precheck-pr / precheck. - Product lanes skipped, named as skipped rather than folded into the rollup (3):
Test (macos-latest, Node 22.x),Test (windows-latest, Node 22.x),Integration Tests (CLI, No Sandbox). Read out of.github/workflows/ci.ymlat this head (byte-identical tomain, sha256e64341f263192477…): the twoTestjobs'if:blocks (:1558,:1655) admit onlymerge_group/schedule/workflow_dispatch, andintegration_cli'sif:(:2056) admits onlymerge_group. This is worth stating precisely because this PR comes from a fork (yiliang114/qwen-code), which invites the wrong explanation: the skips are event-type structural, not secret-availability, and would be identical on a same-repo branch. They cannot report on anypull_request, so their absence is not a coverage gap this PR caused or could close. The rollup reads "zero non-green"; the true green product-lane count is seven. reviewDecisionreadsREVIEW_REQUIRED/mergeStateStatusBLOCKED, which is the bot-approval-does-not-satisfy-branch-protection class, not a retraction: theAPPROVEDrow is still stateAPPROVEDat head, notDISMISSED.- Our own three write surfaces at this head immediately before this write: 0 review rows, 1 issue comment (the report), 0 inline comments.
Why the report concluded mergeable (summary; the executed evidence is in the report):
- Both headline claims were confirmed end to end against the real permission stack, and every production line the PR adds was shown load-bearing twice — once by an independent no-mock probe and once by a test the PR itself added — under four mutation arms (guard dropped;
permission-manager.ts:929disabled;:955disabled; both disabled). - The one behaviour that looked like a defect on paper was measured rather than inferred, and the measurement split:
matchesCommandPattern('**', cmd)matched 11/11 probe commands with verdicts identical to the recognised'*'control, so theBash(**)half is real — but it is already on this PR as ci-bot's Suggestion-levelR1-1(inline 3964391128), so the report bounds it with measurements instead of filing it twice. TheRead(//**/*)half is not a catch-all (4 of 6 probe paths;resolvePathPatternmakes/xproject-root-relative and only//xfilesystem-absolute), so reassuring for it is correct — reporting that half would have been a false defect against correct code. - Neither observation is a Critical, and the report says so explicitly: no Critical found.
Local runtime validation — recommend merge ✅I rebuilt this PR locally and verified it against a real CLI, not just unit tests: two full Everything the PR claims reproduces. The decision logic is provably unchanged — only the citation changes. Two nits and one PR-description correction below; none of them block merge. 1. The issue reproduces and the fix lands — real interactive TUI
BEFORE ( AFTER (PR head) — rule cited on both, including the 2. What the model actually receives (wire capture)The TUI rendering is not the thing that changes model behaviour — the tool-result string sent back to the provider is. Captured from the mock's last request body of a headless run (
Two things worth calling out from that run:
3. Citation parity — is
|
| arm | pairs | denials | citation gaps |
|---|---|---|---|
BEFORE 1919ff97f5 |
377 | 91 | 26 (28.6 % of denials lost the rule) |
AFTER f69d917cca |
377 | 91 | 0 |
The denial count is identical in both arms (91 = 91) — across the whole sweep the PR never flips a decision, it only recovers the citation. A separate 23-case hand-built matrix (incl. read_file/web_fetch direct invocations, monitor, nested bash -lc, cd-tracked writes) gives the same result: 20 denials both arms, 9 gaps → 0.
Other checks on that path:
findMatchingDenyRule({ toolName })with nocommand— the shape used by the two other call sites (coreToolScheduler.ts:305and:2553) — is byte-identical in both arms; both new passes are behindcommand !== undefined.- Citation order mirrors
evaluate(): when aBash(...)rule and aRead(...)virtual-op rule both match one command, both functions resolve the virtual-op rule first, so the cited rule is the deciding rule, not just a matching one. - Cost is negligible and bounded — a 201-segment compound command cites in 2 ms (and the function only runs on the deny path).
4. The ACP / daemon path is covered too (the PR body says it isn't)
Driven over real qwen --acp stdio with a ClientSideConnection client; observable is the session/update → tool_call_update frame:
Session.ts:12208 calls the same evaluatePermissionFlow() and destructures the same denyMessage, so the ACP/daemon surface gets the fix for free. Worth editing the "Not validated / out of scope" bullet — it currently under-claims. The !-bang pre-check is genuinely separate (shell-utils.ts:2117, Command '<cmd>' is blocked by permission rules), but that surface is a user-typed command and never reaches the model, so leaving it for a follow-up is the right call.
5. Do the new tests have teeth?
Counterfactual + 6 mutants, all run through vitest:
- Counterfactual (PR's tests vs pre-PR production code): 3 of the 4 new tests fail, each on the assertion it was written for.
- 6 / 6 mutants killed — disabling either new pass, forcing
isToolWideDenyRule()to a constant in either direction, reverting the message prefix, and neutering the'//**'path check each kill exactly one test.
6. Regression sweep
Full packages/core suite on the PR head: 24 030 passed, 3 failed (session-writer-lease, git-branches, skill-curator). All three reproduce identically on the reverted-base arm — they are the usual running-as-root failures (a chmod-based failure injection that root walks straight through), unrelated to this change. No test in the repo pins the old wording, so nothing else needed updating.
Findings (both minor, neither blocking)
1. Bash(**) is tool-wide in effect but still gets the reassurance.
I measured "tool-wide" empirically — does the rule deny every sampled invocation of that tool — and compared it with whether the message reassures:
Bash(**) denies 4/4 sampled commands (matchesCommandPattern treats ** as match-everything) yet isToolWideDenyRule only tests specifier === '*' for the command kind, so the model is told other uses are still permitted when in fact nothing will run. Every other spelling in the table classifies correctly, including the project-root-scoped Read(/**) / Read(**) (correctly not tool-wide — files outside the project root still read). One-line fix if you want it:
// command / domain catch-all
return rule.specifier === '*' || rule.specifier === '**';2. The WebFetch(*) catch-all handling — and its test case — is unreachable.
matchesDomainPattern() (rule-parser.ts:1378) supports only exact and subdomain matches; it has no wildcard branch. So WebFetch(*) and WebFetch(domain:*) deny nothing at all (0/3 sampled URLs in both arms), which means the specifier === '*' branch can never be reached for a domain rule. The comment above isToolWideDenyRule ('*' is the documented catch-all for command and domain specifiers) isn't true for domains today, and the WebFetch(*) entry in does not reassure for tool-wide catch-all deny rules passes vacuously because the test mocks findMatchingDenyRule. Swapping that entry for bare WebFetch (which does deny 3/3) would make the test assert something real.
中文版报告
本地真实环境验证结论 —— 建议合入 ✅
我在本地把这个 PR 构建成真实可运行的 CLI 做了验证，而不只是跑单测：从同一个 worktree 产出两份完整的 dist/cli.js（f69d917cca vs 只把两个生产文件的 hunk 反向 apply 后的同一棵树，即 merge-base 1919ff97f5），配一台 mock OpenAI 服务器，这样模型真正收到的字符串可以在网络层直接观测。
PR 声称的效果全部复现。判定逻辑可证明未变，变的只是规则引用。下面有两个小问题和一处 PR 描述需要更正，都不阻塞合入。
1. 问题复现 + 修复生效 —— 真实交互式 TUI
settings.json 配 permissions.deny: ["Bash(npm view *)", "Read(//**/node_modules/**)"]，隔离 HOME，tmux 提供真实 pty，两个 arm 跑同样两条命令。
BEFORE（main @ 1919ff97f5）：正是 #11405 里的那条消息，不带规则，读起来像"这个工具没了"（见上方英文版 fig1）。
AFTER（PR head）：两次都带上了规则，包括通过 Read(...) 规则被拒的 cat …/node_modules/…（fig2）。
2. 模型实际收到的内容（网络层抓取）
真正影响模型行为的不是 TUI 渲染，而是回传给 provider 的 tool result 字符串。从 headless 运行（-p … --approval-mode yolo）中 mock 收到的最后一个请求体里抓取（fig3）。这一轮里有两点值得注意：
- 第 [3] 次调用（
npm view bar，普通非复合匹配）在 PR 之前就已经带规则了 —— 所以这个 bug 确实只局限在复合命令 / 虚拟文件操作这两种形态，与描述一致。 - 第 [4] 次调用（
echo hello-from-allowed-command）在同一个会话里成功执行、exit 0。所以"Other uses of this tool are still permitted"在这里是一句事实陈述，不只是安慰话术。同时也验证了：作用域级 deny 不会被--approval-mode yolo绕过。
3. 引用一致性 —— findMatchingDenyRule 现在是否覆盖了 evaluate() 所有的 deny？
这是 PR 真正要恢复的不变式，所以我做了全扫而不是抽查：13 条 deny 规则 × 29 条 shell 命令 = 377 组，全部走真实 PermissionManager（tsx 直接跑 packages/core/src），断言 evaluate(ctx) === 'deny' ⇒ findMatchingDenyRule(ctx) !== undefined。
| arm | 组合数 | deny 次数 | 丢失引用 |
|---|---|---|---|
BEFORE 1919ff97f5 |
377 | 91 | 26（28.6% 的 deny 丢了规则） |
AFTER f69d917cca |
377 | 91 | 0 |
两个 arm 的 deny 次数完全相同（91 = 91） —— 整个扫描里 PR 从未改变任何一次判定，只是把引用找回来了。另一份手工构造的 23 用例矩阵（含 read_file/web_fetch 直接调用、monitor、嵌套 bash -lc、带 cd 追踪的写入）结论一致：两 arm 都是 20 次 deny，gap 从 9 降到 0。
其他检查：
- 只传
{ toolName }（另外两个调用点coreToolScheduler.ts:305和:2553用的形态）在两个 arm 下行为完全一致 —— 两个新增 pass 都在command !== undefined之后。 - 引用顺序与
evaluate()一致：当一条Bash(...)规则和一条Read(...)虚拟操作规则同时命中同一条命令时，两个函数都先解析虚拟操作规则，所以引用到的是真正做出判定的那条规则，而不是随便一条命中的规则。 - 开销可忽略且有界 —— 201 段的复合命令 2 ms 完成引用（而且该函数只在 deny 路径上执行）。
4. ACP / daemon 路径同样被覆盖（PR 描述说没有）
用真实 qwen --acp stdio + ClientSideConnection 客户端驱动，观测点是 session/update → tool_call_update 帧（fig7）。
Session.ts:12208 调用的是同一个 evaluatePermissionFlow()，解构的是同一个 denyMessage，所以 ACP/daemon 这个面是顺带修好的。建议改一下"Not validated / out of scope"那条 —— 目前是少报了。! 前缀预检确实是独立路径（shell-utils.ts:2117，Command '<cmd>' is blocked by permission rules），但那个面是用户自己敲的命令、从不进入模型上下文，留作后续处理是合理的。
5. 新增测试有没有牙齿？
反事实 + 6 个变异体，全部通过 vitest 跑（fig5）：
- 反事实（PR 的测试跑在改动前的生产代码上）：4 个新测试里有 3 个失败，且各自失败在它要守护的断言上。
- 6/6 变异体全部被杀 —— 禁用任一新增 pass、把
isToolWideDenyRule()双向强制成常量、回退消息前缀、以及废掉'//**'路径判断，每个都恰好杀死一个测试。
6. 回归扫描
PR head 上跑完整 packages/core 套件：24030 通过，3 失败（session-writer-lease、git-branches、skill-curator）。这 3 个在反向 apply 后的 base arm 上同样失败 —— 是典型的 root 身份运行导致的环境问题（用 chmod 注入的失败被 root 直接绕过），与本改动无关。仓库里没有任何测试固定了旧措辞，因此不需要额外改动。
发现的问题（两个小问题，都不阻塞）
1. Bash(**) 实际是工具级全禁，却仍然给出"其他用法仍被允许"的安慰语。
我用实测方式判定"是否工具级"—— 该规则是否拒绝了该工具所有采样调用 —— 再与消息是否安慰做对照（fig6）。Bash(**) 拒绝了 4/4 条采样命令（matchesCommandPattern 把 ** 当作全匹配），但 isToolWideDenyRule 对 command 类型只判断 specifier === '*'，于是模型被告知其他用法仍可用，而实际上什么都跑不了。表中其他写法分类都正确，包括工程根作用域的 Read(/**) / Read(**)（正确地不判为工具级 —— 工程根之外的文件仍可读）。想修的话一行即可：
// command / domain catch-all
return rule.specifier === '*' || rule.specifier === '**';2. WebFetch(*) 这条 catch-all 分支及其测试用例实际不可达。
matchesDomainPattern()（rule-parser.ts:1378）只支持精确匹配和子域名匹配，完全没有通配符分支。所以 WebFetch(*) 和 WebFetch(domain:*) 什么都拒绝不了（两个 arm 下采样 3 个 URL 均为 0/3），这意味着 domain 规则永远走不到 specifier === '*' 那个分支。isToolWideDenyRule 上方的注释（'*' is the documented catch-all for command and domain specifiers）对 domain 而言目前不成立；而 does not reassure for tool-wide catch-all deny rules 里的 WebFetch(*) 用例因为 mock 掉了 findMatchingDenyRule 而是空跑通过的。把那一项换成裸 WebFetch（它确实 3/3 全拒）测试才有实际断言力。
Verification environment / 验证环境
worktree /var/tmp/pr11411-wt @ f69d917cca (PR head)
base arm same tree, `git apply -R` of the 2 production hunks only -> 1919ff97f5 behaviour
build npm run build:packages + node esbuild.config.js (esbuild compiles core from src,
so the A/B is: revert hunks -> rebundle -> swap dist/)
model mock OpenAI server on 127.0.0.1, scripted run_shell_command tool_calls,
every request body persisted -> the tool-result strings above are on-the-wire, not rendered
surfaces interactive TUI (tmux, real pty) | headless -p | qwen --acp (ClientSideConnection)
node v22.22.2, Linux x86_64, running as root
|
Released in v0.23.3. |
5 participants
What this PR does
When a shell command is denied by a pattern-scoped permission rule (e.g.
Bash(npm view *)orRead(//**/node_modules/**)), the denial message now cites the matching rule and frames the denial as invocation-scoped instead of tool-scoped.Why it's needed
findMatchingDenyRule()matched strictly fewer rules thanevaluate(), which actually makes the deny decision. For compound commands (cd /tmp && npm view foo) and for shell commands denied via a virtual file-op rule (Read(//**/node_modules/**)matchingcat /app/node_modules/...),evaluate()returnsdenybutfindMatchingDenyRule()returnedundefined, so the message dropped the rule citation. The resultingTool "run_shell_command" is denied by permission rules.reads as "the whole tool is unavailable", and paired with the give-up instruction in the system prompt, made the model abandon the tool entirely (issue #11405).Reviewer Test Plan
How to verify
Set
permissions.deny: ["Bash(npm view *)", "Read(//**/node_modules/**)"]in settings.json, then in an interactive session:cd /tmp && npm view foo— the denial message should citeBash(npm view *)and note that other uses of the shell tool are still permitted.cat /app/node_modules/lodash/index.js— the denial message should citeRead(//**/node_modules/**).Expected: the deny message reads
This "run_shell_command" invocation was denied by permission rules. Matching deny rule: "Bash(npm view *)". Other uses of this tool are still permitted.(or theRead(...)rule for the second case).Evidence (Before & After)
Before:
Tool "run_shell_command" is denied by permission rules.(no rule cited) for both compound-segment and virtual-op denials — covered by the two newfindMatchingDenyRuletests, which failed before the fix and pass after.After: rule cited + invocation-scoped framing, asserted by new unit tests in
permission-manager.test.ts(cites the deny rule for a compound command segment,cites the deny rule when a shell command is denied via a virtual file op) andpermissionFlow.test.ts(frames a specifier-scoped deny as invocation-scoped, not tool-scoped).Tested on
Environment (optional)
Unit tests only (
npx vitest rununderpackages/core); no TUI/dev runtime.Risk & Scope
findMatchingDenyRulenow performs the same compound-command and virtual-file-op passes asevaluate(), so it can only start returning rules it previously missed. Call sites that pass{ toolName }without a command are unaffected.!-bang shell pre-check (shell-utils.ts) and ACP/daemon (Session.ts) deny paths, which use different mechanisms and sit outside the interactivepermissionFlowpath the reporter hit.Linked Issues
Fixes #11405
中文说明
这个 PR 做了什么
当 shell 命令被模式级权限规则拒绝时（例如
Bash(npm view *)或Read(//**/node_modules/**)），拒绝消息现在会引用命中的规则，并把拒绝表述为「本次调用」级别而非「整个工具」级别。为什么需要
findMatchingDenyRule()匹配的规则范围严格窄于真正做出拒绝决定的evaluate()。对于复合命令（cd /tmp && npm view foo）以及通过虚拟文件操作规则被拒的 shell 命令（Read(//**/node_modules/**)命中cat /app/node_modules/...），evaluate()返回deny但findMatchingDenyRule()返回undefined，导致消息丢失规则引用。最终输出Tool "run_shell_command" is denied by permission rules.读起来像「整个工具不可用」，叠加系统提示词中的放弃指令，让模型彻底放弃该工具（issue #11405）。验证方式
在 settings.json 配置
permissions.deny: ["Bash(npm view *)", "Read(//**/node_modules/**)"]，然后在交互会话中：cd /tmp && npm view foo—— 拒绝消息应引用Bash(npm view *)，并说明该工具的其他用法仍被允许。cat /app/node_modules/lodash/index.js—— 拒绝消息应引用Read(//**/node_modules/**)。预期：拒绝消息为
This "run_shell_command" invocation was denied by permission rules. Matching deny rule: "Bash(npm view *)". Other uses of this tool are still permitted.（第二种情况同理引用Read(...)规则）。风险与范围
findMatchingDenyRule现在执行与evaluate()相同的复合命令与虚拟文件操作两套 pass，只会开始返回之前漏掉的规则；不带 command 的{ toolName }调用点不受影响。!前缀 shell 预检（shell-utils.ts）与 ACP/daemon（Session.ts）拒绝路径（使用不同机制，且不在报告者触发的交互式permissionFlow路径内）。