fix(core): read a backslash inside single quotes as literal when splitting - #11765
TianYuan1024 wants to merge 13 commits into
Conversation
…tting A backslash is an ordinary character inside single quotes in bash, so a command ending in one closes its string and an operator after it separates two real commands. The compound-command splitter read that backslash as an escape, consumed the closing quote, and stayed inside the quote to the end of the input, returning the whole line as a single segment. Permission rules are matched per segment, so the carrier command's allow rule then covered whatever followed the operator: with only Bash(echo *) allowed, `echo 'a\' ; rm -rf x` was auto-approved and the rm ran with no prompt. An explicit deny rule was lost the same way, because the command it names never became a segment to match against. The escape now applies outside quotes and inside double quotes only, which is what bash does.
…-quote-backslash-split
github-actions
Bot
added
the
review/self-reported
|
Thanks for the PR — and for filing #11764 with the shell's own trace before writing any code. That made this straightforward to judge. Template looks good ✓ Problem: observed, not theoretical. The issue carries a real reproduction and an A/B table where a single backslash is the only difference between Direction: squarely in scope. Permission-rule matching is a trust boundary, and a saved allow rule that silently reaches past the command it names is about the worst failure mode this subsystem has. The reference CHANGELOG shows this is a recognised bug class upstream too — "Fixed a case where a Read or Edit deny rule did not apply when an Size: 8 production lines (7 added / 1 removed), 58 test lines, 0 generated or schema. Of those 7 added production lines, 6 are the explanatory comment — the behavioural change is a single condition. It touches Approach: minimal, and I could not write a smaller one. Bash gives single quotes no escape mechanism at all — POSIX is explicit that they preserve the literal value of every character within them, and that a single quote cannot occur between single quotes even when preceded by a backslash — so the escape branch simply has to be off while Risk: no elevated risk signals — the Stage 1e revert-correlated path check matched nothing. Worth saying plainly anyway that this is the permission decision path, so I reviewed it at full depth regardless: I named every consumer of the changed function and checked each one for whether more segments could ever produce a more permissive verdict. None can. Moving on to code review. 🔍 中文说明感谢这个 PR——也感谢你在写任何代码之前，先在 #11764 里附上了 shell 自身的执行追踪。这让判断变得很直接。 模板完整 ✓ 问题： 是已观测到的 bug，不是理论性加固。issue 里有真实复现，以及一张 A/B 表格——唯一差别就是一个反斜杠，判定结果却从 方向： 完全在范围内。权限规则匹配是一条信任边界，而一条已保存的授权规则悄悄越过它所指定的命令，是这个子系统里最糟的失效模式。参考 CHANGELOG 表明这在上游也是公认的 bug 类别——「修复了当同一行上存在 规模： 生产代码 8 行（新增 7 / 删除 1），测试 58 行，生成或 schema 文件 0 行。新增的 7 行生产代码里有 6 行是解释性注释——真正的行为改动只有一个条件表达式。它触及 方案： 最小化，我写不出更小的改动。bash 根本不为单引号提供转义机制——POSIX 明确说明单引号保留其中每个字符的字面值，且单引号之间不能再出现单引号，即便前面有反斜杠也不行——所以转义分支在 风险： 无升级风险信号——Stage 1e 的 revert 相关路径检查没有命中任何文件。不过还是要直说：这是权限判定路径，所以我无论如何都按最大深度审查了——我点名了被改函数的每一个调用方，并逐一检查「产生更多片段」是否可能得出更宽松的判定。答案是不可能。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewMy independent proposal first. Reading only the title and the "why", before looking at the diff: bash has no escape mechanism inside single quotes, so a scanner that tracks quoting must disable its escape branch while The fix is correct. Tracing
The double-quote case still escapes, because It cannot break a valid command. This is the part that made me comfortable rather than merely unconvinced: because bash forbids a backslash-escaped single quote inside single quotes altogether, no well-formed bash command depends on the old reading. The familiar Every consumer is monotonically safe. I named all of them and checked whether producing more segments could ever yield a more permissive verdict:
No consumer has an "any segment allowed → allow" shape, so there is no path where the extra segment gets waved through. It also removes an inconsistency. Six other quote-tracking scanners in this repo already guard their escape branch on single-quote state — two in the shell-semantics module, three in the shell utilities, the read-only checker, and the shell tool's comment stripper. Tests. Six new cases across four blocks: the three split shapes ( No Critical blockers and no AGENTS.md violations. The scope declaration in the PR is honest about what it does not touch — command substitution inside an argument and environment assignments that change what an allowed program loads are separate problems, and this PR does not claim to fix them. Not verified: I could not execute a shell probe to reproduce the CI test evidenceFrom the PR's own checks on the reviewed commit, fetched via the API — I did not build or run any PR code. Nothing is red. The load-bearing check is still in flight:
Sandboxed verification would settle what CI cannot: 中文说明先说我自己的独立方案。 只看标题和「为什么需要」、还没看 diff 时，我的判断是：bash 在单引号内根本没有转义机制，所以跟踪引号状态的扫描器必须在 改法是正确的。 以 双引号场景仍然转义，因为那时 它不可能破坏任何合法命令。 这一点让我不只是「没被说服」，而是真正放心：既然 bash 完全禁止在单引号内使用反斜杠转义的单引号，那么没有任何形式正确的 bash 命令依赖旧的读法。常见的 每一个调用方都是单调安全的。 我点名了全部调用方，并检查「产生更多片段」是否可能得出更宽松的判定：
没有任何调用方是「任一片段被允许即允许」的形状，因此不存在多出来的那个片段被顺手放行的路径。 它还消除了一处不一致。 本仓库中另外六个跟踪引号状态的扫描器都已经在转义分支上带了单引号守卫——shell-semantics 模块两个、shell 工具函数三个、只读检查器一个、shell 工具的注释剥离器一个。 测试。 四个代码块中新增六个用例：三种切分形态（ 无 Critical 阻塞项，无 AGENTS.md 违规。PR 对范围的声明是诚实的——参数中的命令替换、以及改变被授权程序加载行为的环境变量赋值，是另外的问题，本 PR 并未声称修复它们。 未验证： 我无法自己执行 shell 探针来复现那份 CI 测试证据（上方表格）：通过 API 读取该 commit 上 PR 自己的检查结果，我没有构建或运行任何 PR 代码。没有红色检查。关键是承载性的那个检查仍在进行中： 沙箱验证可以解决 CI 解决不了的问题： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 5/5 — a one-condition fix to a fail-open permission bug, correct by construction against bash's quoting rules, with every consumer checked and no path to a more permissive verdict. Stepping back: this is what a good security PR looks like. The author found a rule that silently authorised a command it never named, filed the issue with the shell's own trace proving two commands really run, changed one condition, and added a test that stops the fix from over-correcting in the double-quote direction. Six production lines of that change are a comment explaining the constraint; the behavioural edit is The thing that moved me from "looks right" to confident is that the fix cannot break a legitimate command. Bash forbids an escaped single quote inside single quotes outright, so there is no valid command whose correct segmentation depends on the old reading — the change is a strict move toward bash on every input. And the second thing: six sibling scanners in this repo already had this exact guard, so this reads less like a new design decision and more like closing an oversight in the one place that was missed. That is also the most likely explanation for how the bug shipped. I checked the direction of every consequence rather than just the happy path. More segments flows into four call sites in the permission manager and one in the shell-semantics extraction; each takes the most restrictive verdict, the first matching deny, a If I were maintaining this in six months I would thank the author: the comment states the bash rule and the concrete failing input, so the guard will not get "simplified" away by someone who has not read the issue. Two honest caveats, neither a reason to hold this back. The unit job that actually runs the six new cases was still in flight when I wrote this, so my confidence is in the reasoning and the hand-trace, not in a green suite I have seen — approval is therefore deferred rather than posted now. And the mutation-testing evidence that the tests pin the fix is the author's claim; Approval deferred until CI lands green on 中文说明信心度：5/5 —— 用一个条件表达式修复了一个「失效即放行」的权限 bug；对照 bash 的引号规则，它在构造上就是正确的；每一个调用方都已核查，不存在得出更宽松判定的路径。 退一步看整体：这是一个优秀安全 PR 的样子。作者发现了一条会悄悄授权它从未指定的命令的规则，在 issue 里附上了 shell 自身的追踪以证明确实执行了两条命令，只改了一个条件，并补上一个防止修复在双引号方向上矫枉过正的测试。这次改动中的六行生产代码是解释约束的注释，真正的行为改动是 让我从「看起来对」变成「有信心」的关键在于：这个修复不可能破坏任何合法命令。bash 完全禁止在单引号内出现被转义的单引号，因此不存在任何合法命令的正确切分依赖旧的读法——这个改动在所有输入上都是严格地向 bash 靠拢。第二点是：本仓库中另外六个扫描器早已带着这个完全相同的守卫，所以这与其说是一个新的设计决定，不如说是补上了唯一被漏掉的那一处。这大概也是这个 bug 当初为什么会被提交进去的最合理解释。 我核查的是每一个后果的方向，而不只是正常路径。更多片段会流入权限管理器的四个调用点和 shell-semantics 抽取的一个调用点；它们分别取最严格的判定、第一个命中的 deny、一个只会让 UI 更谨慎的 如果六个月后由我来维护这段代码，我会感谢作者：注释写明了 bash 规则和那个具体的失败输入，因此没有读过 issue 的人不会把这个守卫当成多余的东西「简化」掉。 两点如实说明，都不构成压住这个 PR 的理由。真正运行那六个新用例的单测 job 在我写下这段时仍在进行中，所以我的信心来自推理和手工走查，而不是我亲眼看到的绿色套件——因此批准是延后的，而不是现在提交。另外，「测试确实钉住了这个修复」的变异验证证据是作者的说法；把它变成独立证据的方式是受赞助的 批准延后至 CI 在 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical: the !inSingle guard re-opens the same bypass through bash's $'…' ANSI-C quotes — a form that split correctly before this change
Where: packages/core/src/permissions/rule-parser.ts:886, at head dd80fb998b40b55d79c6003373ed37807a2285f2
if (ch === '\\' && !inSingle) {First, the part that is right: the fix does exactly what it claims for plain single quotes. I reproduced the reported case against both arms and it is genuinely closed there — that is not in question.
The problem
Bash has two single-quote forms with opposite backslash semantics:
'…'— a backslash is an ordinary character (the case this PR fixes).$'…'— ANSI-C quoting, where a backslash is an escape, including before the closing quote. So$'a\''is the two-character worda', and it terminates there.
!inSingle applies plain-quote semantics to both. Inside $'…' the backslash is no longer an escape, so the scanner treats the escaped quote as the closing one, then reads the real closing quote as opening a new string — and stays inside it across the operator that bash really does split on.
Trigger
With permissionsAllow: ["Bash(echo *)"] and no deny rule:
echo $'a\'' ; rm -rf /tmp/x
&& and | behave identically. It is the PR's own carrier with one extra character.
Measured, not derived
I extracted splitCompoundCommandSegments from both the merge-base blob (9907978965de) and the head blob (da4500ba33d2), ran the two arms side by side, and used bash's own bash -x trace as the oracle:
| input | bash runs | base | head |
|---|---|---|---|
echo 'a\' ; touch … (this PR's case) |
2 cmds | 1 — BYPASS | 2 — fixed |
echo $'a\'' ; touch … |
2 cmds | 2 — ok | 1 — BYPASS |
echo $'a\'' && touch … |
2 cmds | 2 — ok | 1 — BYPASS |
echo $'a\'' | cat |
2 cmds | 2 — ok | 1 — BYPASS |
echo $'a\\b' ; touch … |
2 cmds | 2 — ok | 2 — ok |
echo a ; touch … |
2 cmds | 2 — ok | 2 — ok |
echo 'a && b' |
1 cmd | 1 — ok | 1 — ok |
echo "a\" ; touch … |
syntax error | n/a | n/a |
Counting only the dangerous direction (fewer segments than bash really runs): base = 1, head = 3. Bash's trace for the ANSI-C row is + echo 'a'\''' followed by + touch …, and the marker file is created, so the second command genuinely executes.
Why that reaches an auto-approval
The consumer chain, all read at head:
permission-manager.ts:336—splitCompoundCommand(command)returns 1 segment.permission-manager.ts:337—if (subCommands.length > 1)is false, soevaluateCompoundCommand's per-segment most-restrictive evaluation is skipped andevaluateSingle(ctx)runs against the whole line.matchesRule(rule-parser.ts:1593) callsmatchesCommandPattern('echo *', <whole line>)— I executed it:true.matchesCommandPattern('rm *', <whole line>):false.- The cross-command virtual-op pass at
permission-manager.ts:325does not rescue it:extractShellOperationsAcrossCommandyields Read/Edit/Write/WebFetch virtual ops, so for aBash(...)-only config it returnsdefault;:363-368requires!== 'default'to escalate, and:369returnsbashDecision— allow.
Result: auto-approved, no confirmation prompt, and the rm runs. That is precisely the impact the PR description states for the bug it fixes.
The per-segment deny pass at permission-manager.ts:956-964 is gated on the same length > 1, so an explicit deny: ["Bash(rm *)"] is lost the same way. Your two new PermissionManager tests assert exactly ask and deny for the plain-quote carrier; substituting $'a\'' for 'a\' in either one yields allow at this head.
Why I am calling this Critical rather than a suggestion
I ran the base arm, so this is a delta and not a pre-existing hole the PR merely fails to close: at the merge base this input split into 2 segments and was handled correctly. The diff introduces the bypass. It is a regression on a security boundary, in the same class the diff claims to close, and the net count moves the wrong way (1 → 3).
Nothing in the suite pins it either — grep -c "\$'" packages/core/src/permissions/permission-manager.test.ts returns 0 at head, and the diff adds no ANSI-C case, so all five new tests pass over it.
This contradicts the triage bot's approve-on-green at the same head — the partition
qwen-code-ci-bot stage 2 (5652164414, 08:16:44Z) and stage 3 (5652165969, 08:17:06Z) both reviewed dd80fb998b40b55d79c6003373ed37807a2285f2 — the identical commit — and concluded "No Critical blockers" at confidence 5/5, with an approve-on-green marker on that sha. Two of its load-bearing claims are what this report disputes, and I want to be explicit about which parts of its review survive, because most of it does.
Void — the premise is true of one quote form and was applied to both. Stage 2: "because bash forbids a backslash-escaped single quote inside single quotes altogether, no well-formed bash command depends on the old reading"; stage 3: "there is no valid command whose correct segmentation depends on the old reading — the change is a strict move toward bash on every input." Bash forbids \' inside plain '…'. It does not forbid it inside $'…', which is a different construct — ANSI-C quoting — where the backslash is an escape. $'a\'' is a well-formed word, and its correct segmentation does depend on the old reading. That is the whole finding.
Void — the direction of the consequence. Stage 2 audited every consumer and concluded "no path where the extra segment gets waved through"; stage 3, "None can turn an extra segment into an approval." That audit is correct and I do not dispute it. But it only covers segments the diff adds. On the ANSI-C form the diff removes one (base 2 → head 1), which is the direction that reaches evaluateSingle on the whole line. A monotonicity argument over "more segments" is silent about "fewer".
Unaffected — everything else. The consumer enumeration itself, the SHELL_OPERATORS longest-match-first ordering, the double-quote over-correction guard, the observation that six sibling scanners already carry this guard, and the test-placement reasoning all stand and are not touched by this report.
Why the gap is explicable rather than careless. Stage 2 discloses it directly: "I could not execute a shell probe to reproduce the bash -x trace myself — the attempt was denied by this environment's permission rules, so the 'bash really runs these as two commands' claim rests on the POSIX single-quote rule." Reasoning from the POSIX single-quote rule is exactly what yields the premise above; the ANSI-C form is only visible by running the shell. Every number in my table was executed, not derived — including the three-arm run of the proposed fix.
The practical consequence is time-sensitive: Test (ubuntu-latest, Node 22.x) was still in flight at both reads, and stage 3 states the deferred approval posts automatically once that lane greens on this sha. A green suite will not detect this — grep -c "\$'" permission-manager.test.ts is 0 at head, so all six new cases pass over the bypass.
Fix direction (verified)
!inSingle is too coarse; the scanner needs to know which quote opened the string. Record whether the opening quote was preceded by $, and suppress backslash escaping only for the plain form:
let inAnsiC = false;
// …
if (ch === '\\' && !(inSingle && !inAnsiC)) {
escaped = true;
continue;
}
if (ch === "'" && !inDouble) {
inAnsiC = !inSingle ? command[i - 1] === '$' : false;
inSingle = !inSingle;
continue;
}I ran this variant over the same table: 0 under-splits across all 8 executable cases, including the three ANSI-C rows and this PR's own reported case, with the five controls unchanged. $"…" needs no separate handling — bash gives it double-quote backslash semantics, which the existing inDouble path already matches.
Please also extend the it.each table, since those rows are what make the fix durable:
["echo $'a\\'' ; touch /tmp/x", ["echo $'a\\''", 'touch /tmp/x']],
["echo $'a\\'' && touch /tmp/x", ["echo $'a\\''", 'touch /tmp/x']],plus the two PermissionManager cases with $'a\'' as the carrier, expecting ask and deny.
Scope note: this is the only finding. Everything else in the diff I checked held up — the change is confined to the one guard, the base/head blobs differ nowhere else, and the double-quoted regression test correctly pins the form that must not be split. As of the repository state read immediately before posting, this comment carries no approval, and it is filed as CHANGES_REQUESTED in deliberate opposition to the bot's deferred approve-on-green on the same sha — a maintainer should decide between the two, and the executable evidence above is what decides it.
The previous commit suppressed the backslash escape for the whole of an open single-quoted string. That is right for a plain string, where a backslash is an ordinary character, but wrong for bash's ANSI-C form: inside $'…' the backslash does escape, including before the closing quote, so $'a\'' is a complete word and an operator after it still separates two commands. Reading it as plain swallowed the real closing quote and glued the line back into one segment — the same fail-open the previous commit closed, reached from the other side, and a regression against the merge base rather than a gap it merely left open. The scanner now records which form opened the string and suppresses the escape only for the plain one. A `$` opens the ANSI-C form only when it is itself unquoted, unescaped and not already spent as the second half of `$$`, so `\$'a\'`, `$$'a\'` and `"$"'a\'` stay plain, as bash reads them; treating those as ANSI-C would reintroduce the original bypass through a different door. Eight cases pin the behaviour across the three directions, each verified against `bash -x` as the oracle and each red under the corresponding mutant.
…-quote-backslash-split
|
Confirmed and fixed in Reproduced. With One correction to the proposed fix, which I did not adopt as written. I ran your variant over these: the first two come back as 1 segment against bash's 2 — an under-split, in the same direction and with the same consequence as the finding you filed. The third is already safe because the So the shipped version tracks the introducer instead of peeking at the previous character: a Measured against bash as the oracle. A 19-line corpus — your three ANSI-C rows, this PR's own case, the three
Tests. Eight new cases in three groups — ANSI-C escapes, the On the partition with the bot's |
The differential run against bash turned up shapes the existing cases do not reach: a plain and an ANSI-C string side by side in either order, `$$$'…'` where the PID expansion is followed by a real ANSI-C string, and `$'\''` whose entire body is an escaped quote. bash runs each of these as two commands; three of the four go red when the escape is suppressed for every single-quoted string, so they pin the same guard the ANSI-C cases do from a different direction. No production change — the split already handled all four.
…-quote-backslash-split
|
The red Two runs of this PR failed on two disjoint tests — Three things rule out this change as the cause of the current failure:
The failing assertion is a A re-run should clear the lane. The branch is current with 中文红着的 本 PR 的两次运行失败在两个互不相干的测试上 —— 第一次是 三条证据排除本改动：
失败的断言是一个 重跑一次应当就能转绿。分支已跟进到 |
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.
Not explored to full depth (tool budget reached): "agent 1b": running the repo's own packages/core vitest ( permission-manager.test.ts ) — this worktree has no packages/core/dist , so the globalSetup guard stopped the r…; "agent 5": a full packages/core suite run in this worktree reports 11 failing files / 77 failing tests (the one visible failure is an atomicWriteFile secrets-path asse….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）："agent 1b"：running the repo's own packages/core vitest ( permission-manager.test.ts ) — this worktree has no packages/core/dist , so the globalSetup guard stopped the r…；"agent 5"：a full packages/core suite run in this worktree reports 11 failing files / 77 failing tests (the one visible failure is an atomicWriteFile secrets-path asse…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.23.3)
A `$` separated from its opening quote by a backslash-newline lost its pending state, so `$\<newline>'a\''` was read as a plain `'…'` string. The plain reading then swallowed the real closing quote and held the scanner inside the string to the end of input, returning one segment for a line bash runs as two commands — the bypass the surrounding exception exists to close, re-entered through a continuation. bash removes a backslash-newline before it decides what `$'` means, so treat the pair as elided and carry the pending `$` across it. The newline is consumed with the backslash because `'\n'` is itself a command separator here; leaving it to the `escaped` flag would split every `echo a\<newline>b` continuation instead. Also cover the separators and shapes the earlier cases did not reach: a bare `&` and a newline as the separator for both quote forms, the line continuation that must not split, and the re-opened quote `echo 'a\'' ; rm x'`, which bash runs as a single command and which the corrected scanner keeps in one segment.
…-quote-backslash-split
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.23.3)
Two comments stated the mechanism wrongly, in the direction that invites someone to undo the fix. The line-continuation branch was justified by `'\n'` being a command separator, as though leaving the pair to the `escaped` flag would split the line. It would not: the `escaped` check precedes the operator loop, so that route consumes the newline too, and deleting the branch keeps `echo a\<newline>b` whole. What the branch is actually for is the top-of-loop `dollarPending` reset, which the `escaped` route runs through and which drops the pending `$`. The ANSI-C comment put the closing quote one position early: `$'a\''` holds three quotes and closes at the third, the second being the escaped one that belongs to the string. Read literally, the old wording is the blanket-escape reading that glues the line back into one segment. Also pin the even backslash run. The introducer rule turns on the parity of the run before the `$` — `\$'` is a plain string, `\\$'` is ANSI-C — and only the odd side was covered, so a refactor reading an even run as escaping the `$` would reopen the bypass with the suite green.
…-quote-backslash-split
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.
Not reviewed: build-and-test — "Integration Tests (CLI, No Sandbox)" and the macos-latest / windows-latest test-matrix lanes were skipped in CI at this commit and their suites did not run locally.
Not explored to full depth (tool budget reached): "agent 6b": did not execute the pre-fix scanner to measure walkCompoundCommand attribution before/after; the pre-fix one-segment answer is inferred from the diff's remove…; "agent 6b": did not typecheck packages/core ( npm run typecheck ); only eslint on the two changed files plus their two vitest suites were run.; "agent reverse-audit (round 1)": base-vs-head differential sweeps ran to payload length 3 for the operator and expansion alphabets; the length 4–6 sweeps were head-only (they compared HEAD agai…; "agent reverse-audit (round 1)": the backtick and heredoc merges were measured with a trailing echo MARK rather than a destructive payload; exploitability follows from the merged segment matc…; "agent reverse-audit (round 2)": base-commit scanner behaviour for the heredoc carrier was derived from the diff's removed if (ch === '\\') { line and its added test comment, not executed aga…, and 3 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/permissions/rule-parser.ts:900 — [review] the ANSI-C half of the quote rule is in only one of the two command splitters — splitCommands still merges echo $'a\'' ; rm -rf /tmp/xpackages/core/src/permissions/permission-manager.test.ts:569 — [review] no test pins the cwd/resolved-path attribution walkCompoundCommand derives from the segmentation this diff changes
Convergence: round 3 posted 2 inline comment(s), 1 of them reported for the first time; the previous round posted 4 (3 new). Findings keep coming back to the same files: packages/core/src/permissions/rule-parser.ts (findings in rounds 1, 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — "Integration Tests (CLI, No Sandbox)" and the macos-latest / windows-latest test-matrix lanes were skipped in CI at this commit and their suites did not run locally.
未探索到全部深度（达到工具调用预算）："agent 6b"：did not execute the pre-fix scanner to measure walkCompoundCommand attribution before/after; the pre-fix one-segment answer is inferred from the diff's remove…；"agent 6b"：did not typecheck packages/core ( npm run typecheck ); only eslint on the two changed files plus their two vitest suites were run.；"agent reverse-audit (round 1)"：base-vs-head differential sweeps ran to payload length 3 for the operator and expansion alphabets; the length 4–6 sweeps were head-only (they compared HEAD agai…；"agent reverse-audit (round 1)"：the backtick and heredoc merges were measured with a trailing echo MARK rather than a destructive payload; exploitability follows from the merged segment matc…；"agent reverse-audit (round 2)"：base-commit scanner behaviour for the heredoc carrier was derived from the diff's removed if (ch === '\\') { line and its added test comment, not executed aga…，另有 3 条。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 2 条行内评论，其中 1 条是首次提出；上一轮发布了 4 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/core/src/permissions/rule-parser.ts（第 1、2 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— deepseek-v4.1-flash via Qwen Code /review (v0.23.3)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Re-derived the scanner's behaviour for the changed states against bash, class by class. No blocking issues found, and the test-mutation table in the PR body (13/14/4/5/16/3 red per reverted guard) matches what the code does.
- Plain
'…': backslash literal —echo 'a\' ; rm xsplits at the;. ANSI-C$'…': backslash escapes —$'a\''only closes at its third quote. The!(inSingle && !inAnsiC)condition separates exactly these two, and nothing else. dollarPending:$$spends both$(second$clears the pending flag viadollarPending = !ansiCIntroducer),\$is escaped before the flag is read,"$"never reaches the flag because theinSingle || inDoublecontinue runs first — all three spellings fall back to a plain string, matching bash.$$$correctly becomes PID expansion + ANSI-C string.- Line continuation: consuming
\⏎in the escape branch and re-armingdollarPendingis what keeps$\⏎'…'ANSI-C; routing it throughescapedwould reset the flag at the top of the next iteration and mis-read the string as plain — the comment at the branch states this and the code matches. Inside a plain'…'the!(inSingle && !inAnsiC)guard keeps\⏎literal, which is also bash. - Re-opened quote:
echo 'a\'' ; rm x'correctly produces one segment — the fix changes segmentation in the other direction here and the test pins it, so a later edit that re-splits this line gets a visible failure rather than a silent spurious prompt.
Heads-up, coordination only: there are sibling splitter PRs open on this same function — #11821 (word-initial # as comment, where I have an open finding about <(/>( process-substitution bodies) and #11865 (draft, isAsyncOperator separators). Your change is in the quote/escape region, theirs in comment/separator regions, but they will collide in rule-parser.ts and whoever lands second should re-run both suites against the merged scanner; my 11821 finding is about a separate guard that does not exist in this PR, so nothing here needs changing for it.
…-quote-backslash-split
…litting The plain-quote rule reads a backslash inside '…' as literal, which is right for a real quoted string but wrong in the regions this scanner does not model: a # comment, a backtick body and a heredoc body, where bash does not read the quote characters as quotes at all. There `'a\''` closed at its second quote, re-opened a string at the third and swallowed the next command into an allow-covered segment, so `echo done # note 'a\''` followed by a newline and `rm -rf x` was auto-approved where main asked, and an explicit deny rule was never reached. Scan the input under both readings and split wherever either finds an operator, so the plain-quote fix never removes a boundary the scanner found before it. Pin the three carriers, and put the plain `'c\'` first in the ANSI-C, even-run and continuation rows so only bash's reading can split them and the coarse `!inSingle` guard still turns them red.
Fold the per-case splitter tests and the per-carrier PermissionManager tests into it.each tables and shorten the explanatory comments. Coverage is unchanged: every guard mutation still turns at least one case red.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 1c": none — the walk completed within budget; no check was cut short..
Convergence: round 4 posted 4 inline comment(s), 3 of them reported for the first time; the previous round posted 2 (1 new). Findings keep coming back to the same files: packages/core/src/permissions/rule-parser.ts (findings in rounds 1, 3; 3 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 1c"：none — the walk completed within budget; no check was cut short.。
收敛情况：第 4 轮发布了 4 条行内评论，其中 3 条是首次提出；上一轮发布了 2 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/core/src/permissions/rule-parser.ts（第 1、3 轮已出过发现，本轮又有 3 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
| * See {@link splitCompoundCommand} for the string-only form and for examples; | ||
| * this is the same split, and that function is a projection of this one. | ||
| * | ||
| * Scanned twice and split wherever either scan finds an operator: comments, |
There was a problem hiding this comment.
[Suggestion] R4-1: The two-scan union also splits where main found no boundary at all — inside a # comment, a backtick body or a heredoc body — so a command main allowed can now come back as a bare deny that names no sub-command. Neither this doc comment nor any test records that direction.
The sentence above this line explains the union by the direction where it recovers a boundary: quotes inside an unmodelled region "can fool bash's backslash reading where the pre-fix reading still splits". The union runs the other way too. With allow ['Bash(cat *)','Bash(echo *)','Bash(git *)'] and deny ['Bash(rm *)'], the command git commit -m 'x' # saved to 'C:\' ; rm draft was 1 segment and allow at the merge base and is 2 segments and deny here — bash runs only the git commit, since everything after # is a comment, and the split-off rm draft segment now matches the deny rule. evaluateCompoundCommand returns the bare literal 'deny' (permission-manager.ts:592-596) naming no sub-command, and deny is not YOLO-overridable (permission-manager.ts:605-611), so a user whose commit message mentions a Windows path gets a refusal nothing in the message explains. The same shape reaches a quoted heredoc: cat > fixture.sh <<'EOF' / echo 'a\' ; rm -rf /tmp/x / EOF goes from 2 segments (allow) at the merge base to 4 (deny) here, one of which is exactly rm -rf /tmp/x, for a command where bash runs only cat with the body as literal stdin because <<'EOF' is quoted. Writing shell fixtures through a heredoc is an ordinary agent action.
The trigger is narrow — it needs a backslash inside single quotes within an unmodelled region, and apostrophe-only heredoc bodies do not move — and the direction is fail-closed, so the cost is blocked work rather than exposure. What ships unnamed is the class itself: the Risk & Scope sentence "Some lines bash runs as one command stay split … That is fail-closed and matches main" does not hold for these two inputs, and the test comment "The last row is one command to bash but stays split, as on main" is scoped to a row where it is true (echo 'a\'' ; rm x' splits identically on both arms), so nothing in the repo says the union can add a boundary main never found.
Witness:
two-arm A/B at the reviewed commit (base blob 238060f0b30a vs head d58ce05780cd;
permission-manager.ts and shell-semantics.ts byte-identical between arms, so the
difference is the splitter alone)
"git commit -m 'x' # saved to 'C:\' ; rm draft"
BASE n=1 verdict=allow HEAD n=2 verdict=deny
HEAD segments ["git commit -m 'x' # saved to 'C:\'", "rm draft"]
"cat > fixture.sh <<'EOF'\necho 'a\' ; rm -rf /tmp/x\nEOF"
BASE n=2 verdict=allow HEAD n=4 verdict=deny
bash as oracle:
bash -c "...git commit -m 'x' # saved to 'C:\' ; echo MARKER-SECOND-RAN" -> marker absent
quoted heredoc -> body written literally into fixture.sh; MARKER-HEREDOC-BODY-RAN absent
sizing (trigger needs a backslash inside single quotes in an unmodelled region):
"cat > deploy.sh <<'EOF'\n# don't forget...\nrm -rf /tmp/cache ; echo done\nEOF" -> ask on BOTH arms
controls unchanged: echo "it's fine" && rm -rf /tmp/x -> deny on both arms
Extend the comment at rule-parser.ts:887-889 to name both directions, and correct the Risk & Scope sentence:
* Scanned twice and split wherever either scan finds an operator: comments,
* backtick bodies and heredocs are not modelled, and quotes inside them can
* fool the 'bash' reading where the 'escape-everywhere' reading still splits.
* The union can also split where bash executes nothing - a backslash inside
* single quotes within one of those regions - so a command bash runs as one
* can newly ask or deny. Deliberate and fail-closed, but not what main did.
A comment-stripping alternative must not lose the boundary the retained rows pin: "echo done # note 'a\''\nrm -rf /tmp/x" must still split to ["echo done # note 'a\''", 'rm -rf /tmp/x'] and stay 'deny' under ['Bash(rm *)'], because bash really does run that rm and dropping everything after # would turn a correct deny into an allow. shell-semantics.ts:2266 already splits stripHeredocBodies(command), and stripHeredocBodies is module-private (shell-semantics.ts:2180) with shell-semantics.ts:38 importing from ./rule-parser.js, so a heredoc fix must not pull that helper into this file without inverting that import.
Please pin the tradeoff with a row asserting splitCompoundCommand("git commit -m 'x' # saved to 'C:\\' ; rm draft") equals ["git commit -m 'x' # saved to 'C:\\'", 'rm draft'], plus a %j with deny %j is %s row asserting 'deny' for it under allow ['Bash(git *)'] / deny ['Bash(rm *)'] — then drop the plain-quote reading and confirm both go red, so the rows pin the tradeoff rather than today's output.
中文说明
两次扫描的并集也会在 main 根本找不到边界的地方切分——即 # 注释、反引号体或 heredoc 体内部——于是 main 本来放行的命令，现在可能得到一个不指明子命令的 deny。这条文档注释和测试都没有记录这个方向。
上面那句话是用「并集找回边界」的方向来解释的：未被建模区域里的引号「会误导 bash 的反斜杠读法，而修正前的读法仍会在此切分」。但并集也会朝另一个方向走。在 allow ['Bash(cat *)','Bash(echo *)','Bash(git *)']、deny ['Bash(rm *)'] 下，命令 git commit -m 'x' # saved to 'C:\' ; rm draft 在合并基线是 1 个片段、判定 allow，在这里是 2 个片段、判定 deny——bash 只执行那个 git commit（# 之后全是注释），而被切出来的 rm draft 片段现在命中了 deny 规则。evaluateCompoundCommand 返回裸的字面量 'deny'（permission-manager.ts:592-596），不指明任何子命令，而 deny 无法被 YOLO 覆盖（permission-manager.ts:605-611），所以提交信息里提到 Windows 路径的用户会得到一个消息本身无法解释的拒绝。同样的形态也出现在带引号的 heredoc 上：cat > fixture.sh <<'EOF' / echo 'a\' ; rm -rf /tmp/x / EOF 从合并基线的 2 个片段（allow）变成这里的 4 个（deny），其中一段正好是 rm -rf /tmp/x；而因为 <<'EOF' 带引号，bash 只执行 cat，正文是字面标准输入。通过 heredoc 写 shell 脚本文件是 agent 的常见动作。
触发条件很窄——需要在未被建模的区域里出现「单引号内的反斜杠」，只有撇号的 heredoc 正文并不会改变判定——而且方向是失效即收紧，所以代价是工作被挡住，而不是暴露风险。真正没有被记录下来的是这个类别本身：Risk & Scope 里那句「有些行 bash 实际只执行一条命令，这里仍会切开……这属于失效即收紧，与 main 的行为一致」对这两个输入并不成立；测试注释「最后一行对 bash 是一条命令，但仍保持切开，与 main 一致」只适用于它确实成立的那一行（echo 'a\'' ; rm x' 在两侧切分结果相同），所以仓库里没有任何地方说明并集也会新增 main 从未找到的边界。
修复方向与证据见上方英文部分（两臂 A/B 测量、bash 作为判定基准、以及建议改写的注释文本）。
约束：任何改为剥离注释的做法都不能丢掉现有用例行钉住的边界——"echo done # note 'a\''\nrm -rf /tmp/x" 仍必须切成 ["echo done # note 'a\''", 'rm -rf /tmp/x']，并在 ['Bash(rm *)'] 下保持 'deny'，因为 bash 确实会执行那个 rm，把 # 之后全部丢弃会把一个正确的 deny 变成 allow。另外 shell-semantics.ts:2266 已经对 stripHeredocBodies(command) 做切分，而 stripHeredocBodies 是模块私有的（shell-semantics.ts:2180），且 shell-semantics.ts:38 是从 ./rule-parser.js 导入的，所以 heredoc 的修复不能在不做反向依赖的情况下把该辅助函数搬进本文件。
请补上用例把这个取舍钉住：断言 splitCompoundCommand("git commit -m 'x' # saved to 'C:\\' ; rm draft") 等于 ["git commit -m 'x' # saved to 'C:\\'", 'rm draft']，并在 %j with deny %j is %s 表中加一行断言 allow ['Bash(git *)'] / deny ['Bash(rm *)'] 下为 'deny'；然后去掉普通引号读法，确认两行都变红，这样用例钉住的是这个取舍而不只是当前输出。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| ): CompoundCommandSegment[] { | ||
| const boundaries = [ | ||
| ...findOperatorBoundaries(command, 'bash'), | ||
| ...findOperatorBoundaries(command, 'escape-everywhere'), |
There was a problem hiding this comment.
[Suggestion] R4-2: This second scan runs unconditionally, so every command pays roughly 2x the splitter cost — including the overwhelming majority that contain no backslash, where the two readings are provably identical.
reading has exactly one read site in this file (rule-parser.ts:956, inside the ch === '\\' condition at 955), so on input with no backslash the 'bash' and 'escape-everywhere' scans cannot differ and the second pass is pure repetition. It is not paid once per check either: permission-manager.ts:336, 956, 1112 and 1210 each call splitCompoundCommand, and shell-semantics.ts:2266 calls splitCompoundCommandSegments, so one evaluation multiplies it. Measured on backslash-free input: a 7 KB heredoc goes 664.9 -> 1564.2 us (2.35x), 79 KB with 10,000 ; operators goes 6586 -> 15400 us (2.34x), and an 800 KB heredoc goes 109.4 -> 243.2 ms (2.22x, +134 ms per call). An agent writing a file through a large run_shell_command heredoc pays that at each of those call sites. For a typical short command the delta is ~5 us and immaterial — this is about the large-payload tail, not the common case.
Witness:
grep -n "reading" rule-parser.ts
-> 285 (prose), 889 (prose), 931 (parameter decl), 956 (the sole read)
single-scan arm vs shipped two-scan arm, same probe, backslash-free input:
7 KB heredoc 664.9 us -> 1564.2 us 2.35x
79 KB / 10k ';' 6586 us -> 15400 us 2.34x
800 KB heredoc 109.4 ms -> 243.2 ms 2.22x (+134 ms per call)
redundancy proved, not assumed:
20,000-input backslash-free differential sweep -> 0 differences
positive control: the same comparator reports 4 of 8 differing rows when
backslashes are present, so it is not a dead comparator
candidate fix applied to a throwaway copy, same probe re-run:
A/B/D sections IDENTICAL; every backslash-bearing fixture row IDENTICAL;
20,000-input sweep IDENTICAL; 7 KB 1564->585 us, 800 KB 243->92 ms
(60-63% recovered, approximately single-scan cost); copy then restored
| ...findOperatorBoundaries(command, 'escape-everywhere'), | |
| // `reading` is consulted only inside the `ch === '\\'` branch, so with no | |
| // backslash in the input the two scans are identical. | |
| ...(command.includes('\\') | |
| ? findOperatorBoundaries(command, 'escape-everywhere') | |
| : []), |
The predicate has to key on a backslash anywhere in the whole input, not on quote or ANSI-C state, because rule-parser.ts:956 is the only place reading is read — a narrower predicate is not what makes the two scans equal. Worth naming the risk the guard introduces: it makes the boundary set depend on a second predicate, so a wrong one would silently lose the "never fewer boundaries than main" invariant that four independent differential sweeps totalling ~9.6M inputs establish. The command.includes('\\') form above was confirmed behaviour-preserving on every backslash-bearing fixture and on the 20,000-input sweep.
No test goes red if this guard is later removed, since it is a pure optimisation, so there is nothing to pin in that direction — what must stay green are the backslash-bearing rows this diff already adds (["echo 'a\\' ; touch /tmp/x", ...] and ["echo 'a\\' ; rm -rf /tmp/x", [], 'ask']), which go red if the skip is ever written too broadly, so please run those after any edit here.
中文说明
第二次扫描是无条件执行的，因此每条命令都要付出大约 2 倍的切分开销——包括绝大多数根本不含反斜杠、两种读法可以证明完全相同的命令。
reading 在本文件中只有一个读取点（rule-parser.ts:956，位于第 955 行的 ch === '\\' 条件内），所以在不含反斜杠的输入上，'bash' 与 'escape-everywhere' 两次扫描不可能不同，第二趟纯属重复。而且这个开销不是每次检查只付一次：permission-manager.ts:336、956、1112、1210 各自都会调用 splitCompoundCommand，shell-semantics.ts:2266 还会调用 splitCompoundCommandSegments，因此一次判定会把它放大若干倍。在不含反斜杠的输入上实测：7 KB heredoc 从 664.9 us 变为 1564.2 us（2.35 倍）；79 KB、含 10,000 个 ; 的输入从 6586 us 变为 15400 us（2.34 倍）；800 KB heredoc 从 109.4 ms 变为 243.2 ms（2.22 倍，每次调用多 134 ms）。通过大段 run_shell_command heredoc 写文件的 agent，会在上述每个调用点各付一次。对典型的短命令，差值约 5 us，无关紧要——本条针对的是大载荷的长尾，不是常见情况。
证据（唯一读取点的 grep、两臂计时、20,000 输入的差异扫描与正向对照、以及在一次性副本上应用候选修复后的复测）见上方英文部分。
建议的改法就是上面的一键 suggestion：只有当整条命令中确实含有反斜杠时才执行第二次扫描。
约束：这个判断条件必须以「整个输入中任意位置是否存在反斜杠」为准，而不能以引号或 ANSI-C 状态为准，因为 rule-parser.ts:956 是 reading 唯一的读取点——更窄的条件并不是让两次扫描相同的那个原因。同时也要说明这个守卫带来的风险：它让边界集合多依赖一个判断条件，因此条件写错会悄悄丢掉「切分出的边界不会比 main 少」这一不变量，而该不变量正是四次共约 960 万输入的独立差异扫描所确立的。上面的 command.includes('\\') 写法已在所有含反斜杠的 fixture 以及 20,000 输入扫描上确认行为不变。
由于这是纯优化，日后删掉这个守卫不会有用例变红，所以这个方向上没有可钉的东西——需要保持绿色的是本 diff 已新增的那些含反斜杠用例行（["echo 'a\\' ; touch /tmp/x", ...] 与 ["echo 'a\\' ; rm -rf /tmp/x", [], 'ask']），一旦跳过条件写得太宽它们就会变红，因此改动此处后请运行这些用例。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| } | ||
| const segment = command.substring(lastSplit, start).trim(); | ||
| if (segment) { | ||
| segments.push({ command: segment, terminator: operator }); |
There was a problem hiding this comment.
[Suggestion] R4-3: This merge loop now decides which scan's operator becomes a segment's terminator, and terminator === '&' alone drives the backgrounded-cd cwd rule — yet every test this diff adds asserts splitCompoundCommand, the projection that throws the terminator away, so nothing pins it.
The behaviour is correct today and was verified end to end: cd 'a\' & echo {} > settings.json yields [["cd 'a\\'","&"],["echo {} > settings.json",""]], and extractShellOperationsAcrossCommand(cmd, '/repo') resolves write_file /repo/settings.json — the protected path — while the && variant correctly resolves to /repo/a/settings.json. Only the union makes that & discoverable at all: at the merge base the quote stays open, there is no boundary, the command is one segment and ops is []. So a protected-path deny rule now depends on this merge keeping the &, and a later edit to it — dropping the second scan, changing the .sort tie-break, or replacing if (start < lastSplit) continue; with something that keeps a shorter overlapping boundary — would make a backgrounded cd look foreground and resolve the following segments' relative writes against the cd target instead of the real cwd, so a protected .qwen/settings.json write stops matching its deny rule while the whole suite stays green. The only terminator assertions in the repo are three pre-existing unquoted rows (permission-manager.test.ts:768-783, present at the merge base as 714-729); shell-semantics.test.ts is untouched by this diff and its backgrounded-cd block (749-756) uses only unquoted cd /tmp &, cd .qwen &, cd "$TARGET" &.
Witness:
"cd 'a\' & echo {} > settings.json"
BASE segments [["cd 'a\' & echo {} > settings.json",""]] BASE ops []
HEAD segments [["cd 'a\\'","&"],["echo {} > settings.json",""]]
HEAD ops [{"virtualTool":"write_file","filePath":"/repo/settings.json"}]
"cd 'a\' && echo {} > settings.json"
HEAD ops [{"write_file","/repo/a/settings.json"}] BASE ops []
control "cd /tmp & echo {} > settings.json"
BASE == HEAD == write_file /repo/settings.json
read at HEAD: shell-semantics.ts:2278 is `const backgrounded = terminator === '&';`
fed from :2272's destructure of splitCompoundCommandSegments (:2266)
Add one row to the existing it.each at shell-semantics.test.ts:749-756 for the quoted-backslash form, expecting the write at /repo/settings.json:
"cd 'a\\' & echo {} > settings.json",shell-semantics.ts:2278 is an exact string compare against '&', so the row must use & and not &&; and the over-correction guard already at shell-semantics.test.ts:771 pins 'cd /tmp && echo {} > settings.json' as foreground, which the new row must not weaken.
Please confirm the row really pins this merge by keeping only findOperatorBoundaries(command, 'escape-everywhere') and checking it goes red — the & boundary disappears, the segment's terminator is no longer '&', and the write no longer resolves to /repo/settings.json.
中文说明
这个合并循环现在决定了「哪一次扫描找到的操作符会成为片段的 terminator」，而 terminator === '&' 单独驱动着「后台执行的 cd」的 cwd 规则——但本 diff 新增的所有用例断言的都是 splitCompoundCommand，也就是那个会丢掉 terminator 的投影，因此这一点没有任何用例钉住。
当前行为是正确的，并且已端到端验证：cd 'a\' & echo {} > settings.json 得到 [["cd 'a\\'","&"],["echo {} > settings.json",""]]，extractShellOperationsAcrossCommand(cmd, '/repo') 解析出 write_file /repo/settings.json（受保护路径），而 && 的变体正确解析为 /repo/a/settings.json。而这个 & 完全是因为并集才可被发现：在合并基线上引号一直处于打开状态，没有任何边界，整条命令是 1 个片段，ops 为 []。也就是说，受保护路径的 deny 规则现在依赖这个合并循环保留 &；而日后对它的任何改动——去掉第二次扫描、改变 .sort 的并列次序处理、或把 if (start < lastSplit) continue; 换成会保留较短重叠边界的写法——都会让后台执行的 cd 看起来像前台执行，从而把后续片段的相对写入解析到 cd 的目标目录而不是真实 cwd，于是一次对受保护 .qwen/settings.json 的写入不再命中它的 deny 规则，而整个测试套件仍然是绿的。仓库中唯一对 terminator 的断言是三行既有的、不带引号的用例（permission-manager.test.ts:768-783，在合并基线上是 714-729）；shell-semantics.test.ts 未被本 diff 触及，其后台 cd 用例块（749-756）只用了不带引号的 cd /tmp &、cd .qwen &、cd "$TARGET" &。
证据（两臂分段与 ops 输出、&& 变体、对照组，以及在 HEAD 读到的 shell-semantics.ts:2278/2272/2266）见上方英文部分。
建议在 shell-semantics.test.ts:749-756 既有的 it.each 中加一行带引号反斜杠的形态，断言写入解析到 /repo/settings.json。
约束：shell-semantics.ts:2278 是与 '&' 的精确字符串比较，所以用例行必须用 & 而不是 &&；并且 shell-semantics.test.ts:771 已有的过度修正守卫钉住了 'cd /tmp && echo {} > settings.json' 属于前台执行，新增行不能削弱它。
请确认这一行确实钉住了这个合并循环：只保留 findOperatorBoundaries(command, 'escape-everywhere')，检查该用例是否变红——& 边界会消失，片段的 terminator 不再是 '&'，写入也不再解析到 /repo/settings.json。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| // but escapes inside ANSI-C `$'…'` (so `$'a\''` closes at the third quote). | ||
| if ( | ||
| ch === '\\' && | ||
| (reading === 'escape-everywhere' || !(inSingle && !inAnsiC)) |
There was a problem hiding this comment.
[Suggestion] R1-3: still stands, and its blast radius grew this round. Open PR #9417 still rewrites these very lines with the coarse !inSingle guard this thread proved insufficient — and it now also predates the two-scan union, so whichever PR merges second can silently drop the ANSI-C discrimination and the second scan that closed R3-1.
The guard on this line is (reading === 'escape-everywhere' || !(inSingle && !inAnsiC)), at what is now rule-parser.ts:955-956; this thread is anchored at the pre-refactor position 936, so the code moved. #9417 is still open and was updated today, and its diff still adds + if (ch === '\\' && !inSingle) {, renames splitCompoundCommandSegments to splitCompoundCommandSegmentsRaw and rewrites it, with no occurrence of findOperatorBoundaries, escape-everywhere or inAnsiC anywhere in it. So merging #9417 second reverts this PR to the coarse guard (reopening #11764's ANSI-C sibling) and deletes the second scan (reopening R3-1's comment, backtick and heredoc carriers); merging this PR second loses #9417's heredoc work. Nothing in this repository's tests catches either direction, because each suite pins only whichever scanner survived. The reconciliation requirement is recorded in this thread — "A reconciliation with #9417 must keep both scans as well as inAnsiC and dollarPending. Dropping either scan reopens R3-1 or #11764" — which states the constraint correctly, but it lives on this PR only, so the merge order can still decide the outcome silently.
Witness:
gh pr view 9417 --repo QwenLM/qwen-code --json state,title,updatedAt
-> {"state":"OPEN",
"title":"fix(core): keep heredoc bodies out of permission rule splitting",
"updatedAt":"2026-09-15T03:38:26Z"}
gh pr diff 9417 | grep -nE "inSingle|splitCompoundCommandSegments|findOperatorBoundaries|escape-everywhere|inAnsiC"
1259:-export function splitCompoundCommandSegments(
1260:+function splitCompoundCommandSegmentsRaw(
1282:+ if (ch === '\\' && !inSingle) { <- the coarse guard this thread proved insufficient
rewrites at 1301 / 1320 / 1328 / 1345
zero hits for findOperatorBoundaries, escape-everywhere, inAnsiC
Please post the reconciliation requirement on #9417 itself rather than only here, naming all three things a merged scanner must keep — the inAnsiC / dollarPending discrimination, both findOperatorBoundaries scans, and #9417's heredoc-body handling — so that whoever lands second sees it on the PR they are about to merge. Better still, land the shared-scanner form so there is one implementation to reconcile instead of two that must be kept in step by hand.
中文说明
R1-3 在当前 commit 下仍然成立，而且本轮它的影响范围变大了。仍处于开启状态的 PR #9417 依然会改写这几行，用的正是本线程已证明不够用的粗粒度 !inSingle 守卫——而它现在还早于两次扫描的并集，因此两个 PR 中后合并的那一个可能同时悄悄丢掉 ANSI-C 判别和关闭 R3-1 的那第二次扫描。
这一行的守卫是 (reading === 'escape-everywhere' || !(inSingle && !inAnsiC))，现在位于 rule-parser.ts:955-956；本线程锚定在重构前的位置 936，所以代码已经移动。#9417 仍然开启，且今天有更新，它的 diff 仍然新增 + if (ch === '\\' && !inSingle) {，把 splitCompoundCommandSegments 改名为 splitCompoundCommandSegmentsRaw 并重写，其中完全没有出现 findOperatorBoundaries、escape-everywhere 或 inAnsiC。因此：#9417 后合并会把本 PR 退回粗粒度守卫（重新打开 #11764 的 ANSI-C 同类问题）并删掉第二次扫描（重新打开 R3-1 的注释、反引号与 heredoc 载体）；本 PR 后合并则会丢掉 #9417 的 heredoc 处理。仓库里的测试抓不到任何一个方向，因为每套用例只钉住了最终存活的那个扫描器。统一要求已记录在本线程中——「与 #9417 的协调必须同时保留两次扫描以及 inAnsiC 和 dollarPending；丢掉任一次扫描都会重新打开 R3-1 或 #11764」——这个约束本身表述正确，但它只存在于本 PR 上，所以合并顺序仍然可能在无人察觉的情况下决定结果。
证据（gh pr view 9417 的状态与更新时间，以及 gh pr diff 9417 中 inSingle 相关行的 grep 结果，显示 findOperatorBoundaries / escape-everywhere / inAnsiC 零命中）见上方英文部分。
请把这条统一要求也发到 #9417 本身，而不只是留在这里，并写明合并后的扫描器必须保留的三件事——inAnsiC / dollarPending 判别、两次 findOperatorBoundaries 扫描、以及 #9417 的 heredoc 正文处理——这样后落地的人在即将合并的那个 PR 上就能看到它。更好的做法是落地共享扫描器的形态，这样只需要协调一份实现，而不是靠人工让两份保持同步。
— qwen3.8-max via Qwen Code /review (v0.23.3)
Maintainer verification — built and run locally against the real CLII verified this branch end to end on a real build, not through unit tests alone: Environment: macOS 26.5 (arm64), Node v24.18.1, npm 11.16.0, GNU bash 5.3.15(1). Bottom line: the bug is real, the fix works, and every claim in the Reviewer Test Plan reproduced — except one sentence in "Breaking changes", which is wrong in a way worth correcting before merge. Details below, with the three open 1. The bypass, reproduced in the real CLI
The same line with
2. Ground truth from the shell itselfEvery split was checked against
The last three 3. Verdicts through the compiled
|
main |
this PR | |
|---|---|---|
echo 'a' ; touch … (control) |
ask / deny | ask / deny |
echo 'a\' ; touch … |
allow / allow | ask / deny |
echo 'a\' && touch … |
allow / allow | ask / deny |
echo 'c\' $'a\'' ; touch … |
allow / allow | ask / deny |
echo \$'a\' ; touch … |
allow / allow | ask / deny |
echo $$'a\' ; touch … |
allow / allow | ask / deny |
echo "$"'a\' ; touch … |
allow / allow | ask / deny |
echo "a\" ; touch …" |
allow / allow | allow / allow (one command) |
echo $'a\'' ; touch … |
ask / deny | ask / deny |
| comment / backtick / heredoc carriers (R3-1) | ask / deny | ask / deny |
R3-1 is closed: all three carriers keep main's boundaries.
4. Suites, mutations, and one thing the last commit cost
packages/core, permissions/ + shell.test.ts + shell-utils.test.ts + monitor.test.ts: 1522 passed / 13 files, with permission-manager.test.ts at 454 as claimed. main's own copy of permission-manager.test.ts run against the new parser: 428 passed — no pinned behaviour regressed. eslint on both changed files is clean and the full tsc build is at 0 errors.
The mutation table reproduces exactly, 7 for 7:
| mutation | red (claimed) | red (measured) |
|---|---|---|
| backslash always escapes | 13 | 13 |
coarse !inSingle (no ANSI-C) |
3 | 3 |
| drop the pre-fix-reading scan | 8 | 8 |
$$ opens ANSI-C |
1 | 1 |
continuation drops the pending $ |
1 | 1 |
| continuation keeps the newline | 2 | 2 |
| merge without skipping overlaps | 29 | 29 |
One thing to look at before merging. f56192fcd3 ("condense splitter comments and quote-handling tests") removes 291 test lines (472 → 454 cases). The parser is byte-identical across that commit — the diff is comments only, I checked — so the same mutants can be scored against both test files, and every kill count drops:
| mutation | at 550a71bab0 |
at f56192fcd3 (head) |
|---|---|---|
| backslash always escapes | 20 | 13 |
coarse !inSingle (no ANSI-C) |
9 | 3 |
| drop the second scan | 8 | 8 |
$$ opens ANSI-C |
1 | 1 |
continuation drops the pending $ |
3 | 1 |
| continuation keeps the newline | 7 | 2 |
| merge without skipping overlaps | 39 | 29 |
Every mutant is still killed, so the net holds and this is not a blocker. But the guard that went from 9 to 3 is the exact shape R1-3 asks to keep pinned against #9417. Three cases is a thin margin for the one line a future merge is most likely to flatten.
5. R1-3 / #9417 — measured, and less dire than stated
I actually ran the merge rather than reasoning about the diffs. git merge probe/pr9417 onto this head conflicts: two hunks in rule-parser.ts, the second being this PR's entire backslash-guard block against an empty side — the shape where "take theirs" looks reasonable. Resolving both hunks in #9417's favour leaves exactly one compiler complaint, TS6133: 'inAnsiC' is declared but its value is never read; deleting those two lines is the obvious fix and the tree then compiles clean. The resulting parser:
| case | main |
this PR | naive merge |
|---|---|---|---|
echo 'a\' ; touch … |
allow | ask | ask |
echo 'c\' $'a\'' ; touch … |
allow | ask | allow |
echo $'a\'' ; touch … |
ask | ask | allow ← worse than main |
echo done # note 'a\''⏎touch … |
ask | ask | allow ← worse than main |
echo `echo 'a\''` ; touch … |
ask | ask | allow ← worse than main |
echo 'a\'' ; rm x' |
deny | deny | allow ← worse than main |
So the hazard is real and the reconciliation note is worth keeping. But the claim that "nothing in this repository's tests catches either direction" does not hold: the two test files auto-merge without conflict, and that merged file (508 cases) goes 16 red on this resolution, naming each reopened class by its own title. The gate exists as long as whoever merges keeps both test files, which git does by default. I would keep the note on both PRs and drop the "undetectable" framing.
6. R4-1 — confirmed, and it is the one user-visible regression
git commit -m 'x' # saved to 'C:\' ; rm draft with allow Bash(git *) and deny Bash(rm *). bash runs exactly one command — git commit -m x — and draft is still on disk after both runs. On main the commit lands. On this branch it is refused.
Two corrections to the finding as written: the refusal does name the matching rule ("Matching deny rule: Bash(rm *)"), so it is not a bare deny; and the user still cannot explain it, because there is no rm anywhere in what they asked for. Fail-closed and narrow, but this is the trade the description should state in the Evidence table rather than only in prose — one row, main allow → this PR deny.
7. R4-2 — reproduced independently
Median of 7 runs, per call, on backslash-free input:
| input | main |
this PR | ratio |
|---|---|---|---|
git status --porcelain |
1.5 µs | 3.0 µs | 2.0× |
| 7 KB heredoc | 508.6 µs | 1005.4 µs | 1.98× |
79 KB, 10,000 ; |
8.67 ms | 18.05 ms | 2.08× |
| 800 KB heredoc | 74.7 ms | 150.6 ms | 2.02× |
Consistently ~2×, a little under the reported 2.2–2.35× on this machine but the same shape. Immaterial for ordinary commands; the large-payload tail is real and multiplies across the call sites. Not a merge blocker — a command.includes('\\') guard around the second scan would recover it exactly, since the two readings are provably identical without a backslash.
8. R4-3 — confirmed, and the union is what creates the protection
cd 'a\' & echo {} > settings.json, against /repo:
main: one segment,extractShellOperationsAcrossCommandreturns[]— no write is seen at all.- this PR:
[{ command: "cd 'a\\'", terminator: '&' }, { command: 'echo {} > settings.json', terminator: '' }], and the op resolves towrite_file /repo/settings.json, the protected path. The&&spelling correctly resolves to/repo/a/settings.json.
So this change adds protected-path coverage here. The gap is real though: the diff adds 0 assertions mentioning terminator and 0 calling splitCompoundCommandSegments, so the merge loop that now decides the terminator is pinned by nothing.
9. Differential fuzz — 350,000 inputs
Two generators against both arms with allow Bash(echo *) Bash(cat *) Bash(git *) and deny Bash(rm *) Bash(touch *): 200,000 random token strings, and 150,000 grammar-built lines filtered to those bash -n accepts.
- this PR produced fewer segments than
main: 0 times — the boundary-set invariant holds. - a
denyverdict was lost: 0 times. - verdict transitions on the 150,000 valid lines:
allow→deny7,767 ·ask→deny2,085 ·allow→ask2,010 ·ask→allow1,698.
That last column means "Breaking changes: none. No input becomes more permissive than on main" is not accurate as written. 1.1% of valid generated commands move from ask to allow. The mechanism is benign, and I checked it against bash: these are lines main could not parse, so it merged real command boundaries into one blob that matched no allow rule and fell through to ask; this PR resolves the boundaries and each resulting command matches an allow rule on its own. For example ls "$"'a\' ; cat "$(date)" $'a\'' — bash runs it as two commands, ls and cat, and both are covered by the configured rules. The split is the correct one. But "used to prompt, now runs unattended" is still a permission change, and it should be in the description rather than discovered later.
Suggested replacement for that line:
Breaking changes: a
denyis never lost and the boundary set is never smaller thanmain's. Where the recovered boundaries are correct, a linemaincould not parse and therefore sent toaskmay now be auto-approved, if every sub-command it really runs matches an allow rule.
Verdict
The defect is real, the fix is correct against bash's own reading, and it holds up under a real build, a real pty session, a bash-traced ground truth, a 7-mutant matrix and 350k differential samples. Nothing here blocks the merge.
Before merging I would ask for two description edits and would take one optional change:
- Required — fix the "Breaking changes" line (§9). It is the only claim that does not survive measurement.
- Required — add the R4-1 row to the Evidence table (§6):
git commit -m 'x' # saved to 'C:\' ; rm draftgoesallow→deny. It is disclosed in prose today, but the table is what a reader scans. - Optional, recommended — restore a few of the rows dropped in
f56192fcd3for the!inSingleguard (§4), enough to put it back above single digits. That is the line fix(core): keep heredoc bodies out of permission rule splitting #9417 will collide with.
findOperatorBoundaries/escape-everywhere/inAnsiC and the two-scan union should stay in whichever of #11765 and #9417 lands second; §5 shows exactly what a "take theirs" resolution costs and that CI will catch it.
中文说明
维护者验证 —— 本地真实构建实测
我没有只跑单测，而是在真实构建上做了端到端验证：在 f56192fcd3 上执行 npm run build && npm run bundle，然后用 pty 驱动打包产物 dist/cli.js，加 --approval-mode default，用脚本化的伪 OpenAI 后端只下发一次 run_shell_command，权限规则写在工作区的 .qwen/settings.json 里。main 那条臂是同一棵树，只把 packages/core/src/permissions/rule-parser.ts 回退到合并基 9f8cb17ca6，重建 core 并重新打包 —— 用 dist/ 里有没有 findOperatorBoundaries 做了双向确认。合并基之后 main 没有动过 packages/core/src/permissions/，所以这个单文件 A/B 就是全部差异。
环境：macOS 26.5（arm64）、Node v24.18.1、npm 11.16.0、GNU bash 5.3.15(1)。
结论：缺陷真实存在，修复有效，Reviewer Test Plan 里的每一条都复现了 —— 只有「破坏性变更」那一句不成立，建议合并前改掉。 下面是细节，三条未决 [Suggestion] 也一并重新实测。
1. 真实 CLI 里复现绕过
echo 'a\' ; touch poc.flag，permissions.allow: ["Bash(echo *)"]。main 上 touch 打着绿色 ✓ 直接执行、没有任何确认，事后 poc.flag 存在；本分支会停下来询问。（图 1）
同一行再加 permissions.deny: ["Bash(touch *)"]：main 上 deny 规则根本没被触达，poc.flag 照样被创建 —— 这才是关键，因为 deny 是用户唯一不可被绕过的控制。（图 2）
场景（allow Bash(echo *)） |
main |
本 PR |
|---|---|---|
echo 'a' ; touch poc.flag（对照） |
询问 | 询问 |
echo 'a\' ; touch poc.flag |
静默执行，poc.flag 被创建 |
询问，poc.flag 不存在 |
再加 deny Bash(touch *) |
静默执行，poc.flag 被创建 |
被 Bash(touch *) 拒绝，poc.flag 不存在 |
2. 以 shell 自身为基准
每个切分都用 GNU bash 5.3.15 的 bash --norc --noprofile -xc 对照，数 bash 实际 trace 出来的命令数：echo 'a\' ; touch、&& 变体、$'…' 混合、以及 \$'a\'、$$'a\'、"$"'a\' 六种写法，bash 都跑两条命令、touch 确实执行，而 main 只切出 1 段（失效即放行），本 PR 切出 2 段。echo "a\" ; touch …" 两边都是 1 段（bash 也只跑一条）。
其中 \$、$$、"$" 这三行是本改动额外堵上的绕过，Evidence 表里没有单独列出，建议补一行。
3. 编译后的 PermissionManager 判决
allow Bash(echo *) / 再加 deny Bash(touch *)：上述六种写法在 main 上全是 allow / allow，本 PR 上全是 ask / deny；对照行与「一条命令」行两边一致；注释、反引号、heredoc 三种载体（R3-1）两边都是 ask / deny，说明 R3-1 已关闭且没有把 main 的切分点弄丢。
4. 测试、变异，以及最后一个 commit 的代价
packages/core 下 permissions/ + shell.test.ts + shell-utils.test.ts + monitor.test.ts：13 个文件、1522 个用例全通过，其中 permission-manager.test.ts 正是 454。把 main 自己的 permission-manager.test.ts（428）跑在新解析器上：428 全通过，已钉住的行为没有回归。两个改动文件 eslint 干净，完整 tsc 构建 0 错误。
变异矩阵 7 条全部复现，数字完全一致（13 / 3 / 8 / 1 / 1 / 2 / 29）。
合并前值得看一眼的一点。 f56192fcd3（"condense splitter comments and quote-handling tests"）删掉了 291 行测试（472 → 454 个用例）。这个 commit 前后解析器逐字节相同（diff 只有注释，我核对过），所以可以用同一批变异体对两份测试文件打分，结果每一项击杀数都下降：
| 变异 | 550a71bab0 |
f56192fcd3（head） |
|---|---|---|
| 反斜杠恒为转义 | 20 | 13 |
粗粒度 !inSingle（丢掉 ANSI-C） |
9 | 3 |
| 去掉第二遍扫描 | 8 | 8 |
$$ 开启 ANSI-C |
1 | 1 |
续行丢掉待定的 $ |
3 | 1 |
| 续行保留换行 | 7 | 2 |
| 合并时不跳过重叠 | 39 | 29 |
所有变异体仍然会被杀掉，安全网还在，这不构成阻塞。但从 9 掉到 3 的那一条，恰好就是 R1-3 要求对着 #9417 钉死的那一行。3 个用例的余量，对一条最可能被未来合并抹平的代码来说偏薄。
5. R1-3 / #9417 —— 实测过，没有说的那么严重
我直接跑了合并，而不是读 diff 推断。git merge probe/pr9417 到本 head 上会冲突：rule-parser.ts 里两个 hunk，第二个正是本 PR 整块反斜杠守卫对上空白的一侧 —— 也就是「take theirs」看起来最合理的形状。两个 hunk 都按 #9417 解决后，编译器只报一条 TS6133: 'inAnsiC' is declared but its value is never read；把那两行删掉是最自然的修法，删完就编译通过了。合并结果：
| 用例 | main |
本 PR | 粗暴合并 |
|---|---|---|---|
echo 'a\' ; touch … |
allow | ask | ask |
echo 'c\' $'a\'' ; touch … |
allow | ask | allow |
echo $'a\'' ; touch … |
ask | ask | allow ← 比 main 还差 |
echo done # note 'a\''⏎touch … |
ask | ask | allow ← 比 main 还差 |
echo `echo 'a\''` ; touch … |
ask | ask | allow ← 比 main 还差 |
echo 'a\'' ; rm x' |
deny | deny | allow ← 比 main 还差 |
所以这个风险是真的，那条调和说明值得保留。但「本仓库的测试抓不住任何一个方向」这句不成立：两个测试文件是无冲突自动合并的，合并后的文件（508 个用例）在这种解决方式下红 16 个，每个用例名字就写着被重开的那一类。只要合并的人保留两份测试文件（git 默认就会），这道门就在。建议两个 PR 上都留说明，但去掉「无法检测」的定性。
6. R4-1 —— 确认，且是唯一一处用户可见的退步（图 3）
git commit -m 'x' # saved to 'C:\' ; rm draft，allow Bash(git *)、deny Bash(rm *)。bash 只跑一条命令 git commit -m x，两条臂跑完 draft 都还在。main 上提交成功，本分支上被拒绝。
对这条 finding 有两点修正：拒绝信息确实给出了命中的规则（"Matching deny rule: Bash(rm *)"），不是裸 deny；但用户依然无法解释它，因为自己写的命令里根本没有 rm。属于失效即收紧、触发面窄，但这个取舍应该进 Evidence 表（main allow → 本 PR deny 一行），而不是只在正文里提。
7. R4-2 —— 独立复现
7 次取中位数、单次调用、无反斜杠输入：git status --porcelain 1.5 → 3.0 µs（2.0×）；7 KB heredoc 508.6 → 1005.4 µs（1.98×）；79 KB / 10000 个 ; 8.67 → 18.05 ms（2.08×）；800 KB heredoc 74.7 → 150.6 ms（2.02×）。本机稳定在 ~2×，比报告的 2.2–2.35× 略低，但形状一致。日常命令可忽略，大 payload 尾部是实打实的，而且会按调用点数量翻倍。不阻塞合并 —— 在第二遍扫描外面加一个 command.includes('\\') 判断即可完全消掉，因为没有反斜杠时两种读法可证等价。
8. R4-3 —— 确认，而且这份保护正是 union 带来的
cd 'a\' & echo {} > settings.json，相对 /repo：main 只切 1 段，extractShellOperationsAcrossCommand 返回 []，根本看不到这次写入；本 PR 切成 cd 'a\'（terminator &）+ echo {} > settings.json，解析出 write_file /repo/settings.json（受保护路径），&& 写法则正确解析到 /repo/a/settings.json。也就是说这个改动在这里增加了保护。但缺口确实存在：本 diff 新增的断言里提到 terminator 的有 0 处，调用 splitCompoundCommandSegments 的也有 0 处，现在决定 terminator 的合并循环没有任何测试钉住。
9. 差分 fuzz —— 35 万条输入
两个生成器对打两条臂，allow Bash(echo *) Bash(cat *) Bash(git *)、deny Bash(rm *) Bash(touch *)：20 万条随机 token 串，15 万条语法生成并用 bash -n 过滤后的合法命令行。
- 本 PR 切出的段数少于
main的次数：0 —— 切分点不会变少这条不变式成立。 - 丢掉
deny判决的次数：0。 - 15 万条合法命令上的判决迁移：
allow→deny7767 ·ask→deny2085 ·allow→ask2010 ·ask→allow1698。
最后一列说明「破坏性变更：无。没有任何输入比 main 更宽松」这句不成立。 1.1% 的合法命令从 ask 变成了 allow。我对着 bash 核过机制，是良性的：这些行 main 解析不了，于是把真实的命令边界糊成一段、匹配不上任何 allow 规则，落到 ask；本 PR 把边界解出来后，每一条真实命令各自都命中了 allow 规则。例如 ls "$"'a\' ; cat "$(date)" $'a\''，bash 就是跑 ls 和 cat 两条，两条都在配置的规则内 —— 切分是对的。但「原本会弹确认、现在直接跑」终究是权限行为的变化，应该写进说明，而不是留给以后被人发现。
建议把那一句改成：
破坏性变更： 不会丢失任何
deny，切分点也不会比main更少。在新恢复出来的边界是正确的地方，某些main解析不了、因而落到ask的行，如果它真正执行的每一条子命令都命中 allow 规则，现在会被自动放行。
结论
缺陷真实，修复方向与 bash 自身的读法一致，并且在真实构建、真实 pty 会话、bash trace 基准、7 个变异体和 35 万条差分样本下都站得住。没有任何一条阻塞合并。
合并前我希望改两处说明，另有一处可选改动：
- 必须 —— 修正「破坏性变更」那一句（§9），这是唯一一条经不起实测的断言。
- 必须 —— 把 R4-1 那一行补进 Evidence 表（§6）：
git commit -m 'x' # saved to 'C:\' ; rm draft从allow变deny。现在只在正文里披露，但读者扫的是表。 - 可选，建议 —— 把
f56192fcd3删掉的部分行为!inSingle守卫补回来（§4），让它重回两位数。这正是 fix(core): keep heredoc bodies out of permission rule splitting #9417 会撞上的那一行。
无论 #11765 和 #9417 哪个后合，findOperatorBoundaries / escape-everywhere / inAnsiC 和两遍扫描的 union 都必须保留在后合的那一侧；§5 给出了「take theirs」具体会付出什么代价，以及 CI 会抓住它。
6 participants
What this PR does
Fixes how
splitCompoundCommandSegmentstracks quotes, so permission rules see every command bash actually runs.'…'a backslash is literal, so'a\'closes the string. Inside ANSI-C$'…'it still escapes.\$,$$and"$"don't open ANSI-C, and$\⏎'…'does.#comments, backtick bodies or heredocs, and quotes inside them can mislead the new reading. So the input is scanned under both the new and the pre-fix reading, and split wherever either finds an operator. Segmentation never loses a boundarymainhad.Why it's needed
With only
Bash(echo *)allowed,echo 'a\' ; rm -rf xwas a single segment, so it was auto-approved and thermran without a prompt. A deny rule on the second command was never checked.Reviewer Test Plan
How to verify
Allow
Bash(echo *)and runecho 'a\' ; touch /tmp/qwen-poc. It should now ask; withdeny: ["Bash(touch *)"]it should deny.bash -xc "echo 'a\' ; touch /tmp/qwen-poc"traces two commands.cd packages/core && npx vitest run src/permissions/permission-manager.test.ts: 454 pass (428 onmain, andmain's test file also passes against this parser). The permissions, shell tool, monitor and shell-utils suites pass too (2316 tests). Each guard is pinned by a mutation that turns tests red:!inSingle(no ANSI-C)$$opens ANSI-C$Evidence (Before & After)
PermissionManager, allowBash(echo *)/ then with denyBash(touch *):The comment, backtick and heredoc carriers (e.g.
echo done # note 'a\''⏎touch …) areask/denyboth before and after, and are pinned.Tested on
Environment (optional)
macOS 26.5.2 (arm64), Node v22.22.1, GNU bash 3.2.57.
Risk & Scope
main. Some lines bash runs as one command stay split, e.g.echo 'a\'' ; rm x'. That is fail-closed and matchesmain.# it's, which already fail onmain(splitCompoundCommandSegments splits on an operator inside a trailing # comment #11815, fix(core): treat a word-initial # as a comment when splitting shell commands #11821, fix(core): keep heredoc bodies out of permission rule splitting #9417). Also not covered: command substitution and env-var tricks. Windows and Linux weren't run locally; the change is platform-independent string scanning.main.Linked Issues
Fixes #11764
中文说明
本 PR 做了什么
修正
splitCompoundCommandSegments的引号跟踪，让权限规则能看到 bash 实际执行的每一条命令。'…'内反斜杠是字面字符，'a\'是闭合的；ANSI-C$'…'内仍是转义。\$、$$、"$"不开启 ANSI-C，$\⏎'…'会开启。#注释、反引号和 heredoc，这些区域里的引号会误导新读法。所以按新旧两种读法各扫描一次，任一次找到分隔符就切分，切分点不会比main少。为什么需要
只授权
Bash(echo *)时，echo 'a\' ; rm -rf x被当成一段，于是自动放行，rm在没有确认提示的情况下就执行了；针对第二条命令的 deny 规则也不会被检查。Reviewer Test Plan
授权
Bash(echo *)后执行echo 'a\' ; touch /tmp/qwen-poc，现在会弹出确认；加上deny: ["Bash(touch *)"]后会被拒绝。permission-manager.test.ts454 个用例通过（main上 428 个，main的测试文件在新解析器上也能通过）；permissions、shell 工具、monitor 与 shell-utils 相关测试共 2316 个通过。每处保护逻辑都有对应的变异测试，见上表。风险与范围
main少。有些行 bash 实际只执行一条命令，这里仍会切开（如echo 'a\'' ; rm x'）。这属于失效即收紧，与main的行为一致。# it's），在main上本来就有问题，分别由 splitCompoundCommandSegments splits on an operator inside a trailing # comment #11815、fix(core): treat a word-initial # as a comment when splitting shell commands #11821、fix(core): keep heredoc bodies out of permission rule splitting #9417 跟踪；命令替换和环境变量等其他绕过方式也不在本 PR 范围内。main更宽松。