fix(core): treat a word-initial # as a comment when splitting shell commands - #11821
yiliang114 wants to merge 9 commits into
Conversation
…ommands `splitCompoundCommandSegments` had no comment state, so `#` was transparent and every operator after it read as a real boundary. `echo 'a' # comment ; echo B` came back as two segments while bash runs a single `echo`, and a commented-out tail the allow rule did not cover — `echo 'a' # comment ; rm -rf /tmp/x` — pulled the whole line into a confirmation prompt, or a deny, for a command bash never runs. Add the missing state: a word-initial `#` outside quotes runs to the end of the physical line. Two details keep this fail-closed: - The scan ignores `escaped`, because a backslash does not continue a line inside a comment. bash really does run the `rm` in `echo hi # foo \` followed by a newline and `rm -rf /`; the existing escape handling folded that into one segment, so this shape gets its deny check back. - The newline that ends the comment stays a boundary, so a comment can never swallow a command sitting on a following line. Heredoc bodies stay out of scope — only `walkCompoundCommand` strips them, and making the other split paths heredoc-aware is #9417. A `#` at the start of a heredoc body line now keeps its operator inside the comment, which drops a deny that fired on text bash hands to `cat` as data and never executes. Fixes #11815 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-issue-patrol/jmu0u98vg3t
|
Re-running the gate on Template looks good ✓ Problem: observed, not theoretical. #11815 is open and carries the measured bash-vs-splitter table, and the description includes the red run on unmodified Direction: aligned, and the upstream signal is unusually direct this round. Claude Code's CHANGELOG treats every analyzer-vs-shell disagreement as a defect and fixes it in the fail-closed direction: 2.1.257 "auto-approving certain Size: core path ( Approach: minimal, and still purely additive — two files, no deletions, no drive-by edits, no unrelated churn. It extends the one shared scanner instead of adding a parallel one, which is what lets a single edit reach all five consumers (the four Bash-rule passes in Risk: no Stage 1e high-risk-path match ( Moving on to code review. 🔍 中文说明在 模板完整 ✓ 问题：已观测到的缺陷，不是理论性加固。#11815 处于 open 状态并附有 bash 与切分器的实测对照表，PR 描述还给出了在未修改的 方向：对齐，而且这一轮上游信号异常直接。Claude Code 的 CHANGELOG 把每一处「分析器与 shell 理解不一致」都当缺陷处理，并朝收紧方向修：2.1.257「 规模：触及核心路径（ 方案：改动最小，且依然是纯新增——两个文件、零删除、无顺手改动、无无关变更。它扩展的是那个共享切分器，而不是新增一个并行实现，因此一次修改就能覆盖全部五个消费方（ 风险：未命中 Stage 1e 高风险路径（ 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewIndependent proposal (written from the title and "Why it's needed" alone, before opening the diff): add a fifth state to Last round's blocker is fixed, and so are the three filed beside itTraced against the code at
I also walked the scan's mechanics, since an off-by-one here is silent: it always advances at least one character (the Reuse check: nothing to flag. One new fail-open shape I found — non-blocking, and I am saying why
The I am deliberately not filing this as a blocker. It requires a brace group inside an expansion default and a The standing Critical: R1-1 still stands at this headThe unresolved thread at
Put together: Honesty on the evidence, because it bounds how strongly I state it. I could not execute anything: shell execution is denied in this triage environment (both my TestingEvidence carried here is the PR's own CI, read through the API for the reviewed commit — I did not build or run anything from this PR. Fetched once, no polling; three lanes were still in flight when this pass was written and nothing was red (zero Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Two things about that table matter for the verdict, and neither is visible from the conclusions alone. The three in-flight lanes include the only unit-test lane that runs for a PR, so at the moment of writing there is no green suite at this head to lean on; the finalize job rewrites the region above once CI settles. And the skipped Windows lane is structural, not incidental — Not verified, and why: cmd.exe behaviour — no Windows runner and no shell execution in this environment; the author's reported local counts ( Sandboxed verification would settle the part static review cannot: 中文说明代码审查独立方案（只看标题与「为什么需要」，在读 diff 之前写下）：给 上一轮的阻断项已修复，同时被提出的另外三项也已修复以下都是对着
我还走了扫描本身的机械细节，因为这里的差一是静默的：它每次至少前进一个字符（ 复用检查：无可指摘。 我新发现的一个朝放开方向的形态——非阻断，并说明理由
我刻意不把它作为阻断项提出。它需要「展开默认值里有花括号组」并且「其后还有 未决的 Critical：R1-1 在当前 head 依然成立
合起来： 关于证据的如实说明，因为它限定了我把话说到多强。我无法执行任何东西：本 triage 环境禁止 shell 执行（我两次 测试这里承载的证据是 PR 自己的 CI，通过 API 读取被审查 commit 的结果——我没有构建或运行本 PR 的任何代码。一次性获取，不轮询；写这段时还有三条通道在跑，且没有任何一项为红（head 上所有 check-run 中 上方表格中， 关于那张表，有两点对结论有影响，而且单看结论看不出来。三条在跑的通道里包含了 PR 唯一会执行的单元测试通道，所以此刻没有绿色套件可依赖。而 Windows 通道的 skipped 是结构性的、不是偶然： 未验证项及原因：cmd.exe 行为——没有 Windows runner，本环境也禁止 shell 执行；作者报告的本地数字（ 沙箱化验证可以解决静态审查看不到的那一半： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 3/5 — the bash side of this is now right as far as I can determine, and every Critical filed against it has been answered with a guard and a pin; the cap is not doubt about the fix, it is one verified fail-open regression on cmd.exe whose scope the author has deliberately handed to a maintainer, and that hand-off is a call I cannot make from the diff. Stepping back. My independent proposal and this implementation still agree on everything except the shell gate, which is the strongest signal I have that the approach is right rather than merely defensible: I would have written the same fifth state, the same raw scan that ignores What stops me is the one item the author declined, and I want to be precise about what I am and am not saying. R1-1 is not a theoretical concern and not a style disagreement: at this head, on cmd.exe, So I am naming my recommendation and then handing the decision over, rather than picking either failure mode — approving past a verified fail-open regression, or stacking a third Two process notes. This is round five, so per AGENTS.md I am holding the line at Critical only: the nested-brace CI, so nobody waits on it: three lanes were still in flight at this head, including the only unit-test lane a PR triggers, so there is no green suite to lean on yet and I would not approve on this run regardless. The Windows unit lane is skipped by event gate, not by this PR, so CI will never cover the open item. The finalize job will update the CI table in Stage 2 once the run settles; it will not approve on green, because this pass deliberately carries no approval instruction — the verdict is defer, and a marker would be a standing approval I cannot withdraw. @qqqys — deferring to you. You are the most recent human reviewer on this PR, your three findings are all fixed and pinned at 中文说明Confidence: 3/5 —— 就我能判断的范围，bash 这一侧现在是对的，针对它提出的每一个 Critical 都用一处防护加一条钉子回答了；这个分数上限不是对修复本身的怀疑，而是一个已核实的、cmd.exe 上朝放开方向的回归，其范围被作者刻意交给了维护者，而这个交接不是我能从 diff 里做出的判断。 退一步看。我的独立方案与这份实现除了 shell 门控之外仍然完全一致，这是我能拿出的、说明方案「正确」而不只是「说得过去」的最强信号：同样的第五个状态、同样不带 拦住我的正是作者拒绝处理的那一项，我想说清楚我在讲什么、不在讲什么。R1-1 不是理论顾虑，也不是风格分歧：在当前 head 上、在 cmd.exe 下， 所以我先给出建议、再把决定交出去，而不是在两种失败模式里挑一个：既不能越过一个已核实的放开方向回归去批准，也不能在一个已经被 gate、且作者明确要求由人来定的 PR 上再叠第三个 两点流程说明。这是第五轮，所以按 AGENTS.md 我只守 Critical 这条线：本轮我发现的嵌套花括号 CI，以免有人白等：当前 head 上还有三条通道在跑，其中包含 PR 唯一会触发的单元测试通道，所以此刻没有绿色套件可依赖，本轮无论如何我都不会批准。Windows 单元测试通道是被事件门控跳过的，与本 PR 无关，所以 CI 永远不会覆盖这个未决项。CI 落定后 finalize 任务会更新 Stage 2 里的表格；但它不会在绿灯时批准，因为本轮刻意没有携带任何批准指令——结论是 defer，一个标记会构成我无法撤回的常设批准。 @qqqys —— 转交给你。你是本 PR 最近一位人类审查者，你提出的三项发现在 — 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 one change before this can land — see the notes above. 🙏
isCommentStart reads the raw previous character, so a backslash-escaped space or ; immediately before a # opens a comment that bash does not, collapsing echo a\ #b ; rm -rf /tmp/x into a single echo-led segment and dropping the Bash(rm *) deny that fires on main. precedingBackslashCount is already in the same file for exactly this. Full trace and suggested patch in the review comment.
Everything else — the design, the scope separation from #11765 / #9417 / #10212, and the tests — looks ready.
中文说明
合并前需要一处修改——详见上方说明。🙏
isCommentStart 读取的是原始的前一个字符，因此紧邻 # 之前的、被反斜杠转义的空格或 ; 会开启一个 bash 并不认为存在的注释，使 echo a\ #b ; rm -rf /tmp/x 塌缩为单个以 echo 开头的片段，从而丢掉在 main 上会生效的 Bash(rm *) deny。同一文件里已有 precedingBackslashCount 正是为此而备。完整追踪与建议补丁见审查评论。
— Qwen Code · qwen3.8-max-2026-09-02
|
Qwen Code review was cancelled before a review could be posted. Nothing failed and nothing is retried automatically: the run was cancelled — by an operator, an upstream event, or the job exceeding its execution time limit. If you still want a review of this PR, request one with |
isCommentStart read the raw previous character, so a backslash-escaped
space or ;/&/| before a # opened a comment bash never starts, merging a
real boundary away and dropping the following command's rule check (a new
fail-open path in the permission scanner). The boundary now requires the
preceding character to be unescaped, reusing precedingBackslashCount. Also
name the heredoc test's expected verdict ('ask') instead of only ruling
out 'deny'.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
"Fixed in 53fd793 — the boundary in |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Scope: packages/core/src/permissions/rule-parser.ts (new isCommentStart function + comment scan, full read) · packages/core/src/permissions/permission-manager.test.ts (new tests, full read) · cross-file: checked splitCompoundCommandSegments caller paths to confirm fail-closed direction · triage: Standard/Deep (correctness and security of a shell permission parser).
Not reviewed: other callers of splitCompoundCommandSegments beyond the permission-manager integration tests; macOS/Windows cross-shell compatibility.
Findings
No blocking findings.
What was checked
isCommentStartcorrectness: The function requires eitherindex === 0, or the preceding character is whitespace or one of[';', '&', '|'], AND not backslash-escaped.8#17in$(( 8#17 ))has a digit before#, soisCommentStartreturnsfalsethere — arithmetic-base notation is not treated as a comment. ✓- Fail-closed direction: A comment masks an operator, collapsing two potential segments into one. Fewer segments → fewer rule checks. That matches bash's semantics — the PR correctly notes that operators inside a comment are inert — so the direction is fail-closed, not a security regression. ✓
- Backslash-before-newline case: The comment scan ignores the
escapedflag and stops at\n, keeping the newline as a real boundary.echo hi # foo \+ newline +rm -rf /→ two segments, so thermstill gets its own rule check. The PR description and test'does not let a trailing backslash extend a comment'pin this. ✓ echo a;#ccase:#right after;meets theCOMMENT_WORD_BOUNDARIESrequirement, so the#c … rmtail becomes a comment segment. This matchesbash -xc "echo a;#c ; rm -rf /tmp/x"which runs onlyecho a. ✓- Quote interior:
echo '# c' ; rm -rf /tmp/xmust still split. The test passes, confirming the comment scan is guarded by the existinginSingle/inDoublestate. ✓ - Cross-check vs
qwen-code-ci-bot: The bot'sCHANGES_REQUESTEDbody is<!-- qwen-triage stage=3-review -->— a pipeline marker, not a substantive finding. No findings to confirm, refute, or miss.
No blocking findings. Approval blockers: none.
Reviewed with AI assistance.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES
Reviewed head 53fd793cf1 against base f647ecf09f. One blocking finding and two non-blocking notes. All three are the same class as the escaped-space case already raised on this PR and fixed here — but a different root cause than that fix covers: isCommentStart asks whether the previous character is JS whitespace or in COMMENT_WORD_BOUNDARIES, and bash's word rule is not that.
Blocker — /\s/ is broader than bash's word separators. \s matches \r, \v, \f and \u00a0 (NBSP), none of which end a bash word. Verified in this shell:
$ bash -xc "$(printf 'echo a\r#c ; echo DANGER')"
+ echo $'a\r#c'
#c
+ echo DANGER # two commands — the ';' is a real boundary
With \r before the #, isCommentStart sees /\s/ → true, so the comment scan swallows ; echo DANGER and the splitter returns ONE segment where bash runs two. The folded segment is then the only thing the rules see — with permissions.allow: ["Bash(echo *)"] it matches the allow pattern while the second command bash executes is never checked on its own. NBSP behaves identically (echo a\u00a0#c ; echo DANGER → two commands in bash, one segment here). Testing the exact characters bash treats as separators — space, tab, newline, plus what is already in COMMENT_WORD_BOUNDARIES — closes this.
Non-blocking (a) — parameter expansion: bash starts no comment after whitespace inside ${…}, but the splitter does. x=; echo ${x:-a #b} ; echo DANGER runs three commands (+ x=, + echo a '#b', + echo DANGER, verified), while the space before #b reads as a boundary here and the line collapses to one segment.
Non-blocking (b) — arithmetic: the comment scan skips the )) handling, so arithmeticDepth stays raised for the rest of the input and a later & is no longer a boundary. echo $(( 1 # 2 )) + newline + echo a & echo DANGER yields two segments here against three commands in bash.
Direction check: I found no input where the splitter now emits MORE segments than bash runs for a single command, so nothing here over-splits harmfully — all of it under-splits.
Reviewed with AI assistance.
`isCommentStart` used `/\s/` to decide whether the character before a `#` ended a bash word. JavaScript's `\s` also matches `\r`, `\v`, `\f` and `\u00a0`, none of which end a bash word, so `echo a\r#c ; echo DANGER` folded into one segment: the `#` read as a comment swallowed the real `;` boundary, and the command bash runs second never got its own rule check. `bash -xc "$(printf 'echo a\r#c ; echo DANGER')"` traces two commands, and the same holds for `\v`, `\f` and `\u00a0`. List the three default `IFS` whitespace characters explicitly in `COMMENT_WORD_BOUNDARIES` instead, so the boundary set is exactly the characters bash treats as word separators. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmu15ornj49
Brings in the base-side change to .github/workflows/ci.yml that the lint-gate freshness check requires. Without it the Lint & Static lane aborts at step 8 and never runs prettier, eslint, typecheck or the test suites, so the word-boundary fix on this branch had no CI validation. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmu15ornj49
|
Two pushes this round, both recorded here so the head movement is traceable.
const COMMENT_WORD_BOUNDARIES = [' ', '\t', '\n', ';', '&', '|'];
|
|
@qwen-code /triage |
|
Sandboxed verification: The verification ran, but its result artifact could not be retrieved for publishing, so there is nothing to report here. The run log still has the agent output; re-run 中文 — 判定：
|
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
[Critical] The new comment scan folds away real command boundaries in two contexts where bash does not start a comment. Both are fail-open regressions against this branch's own base, and both let a command bash really executes escape its own rule check.
Everything below was measured at head f3cff68906ce against base 87437db784f6, driving splitCompoundCommandSegments extracted verbatim from each blob, with GNU bash 4.4.20 as the oracle.
Already fixed at this head — not re-filed
ci-bot's Critical of 07:27:53Z (isCommentStart reading the raw previous character) is addressed here: the helper now returns COMMENT_WORD_BOUNDARIES.includes(previous) && precedingBackslashCount(command, index - 1) % 2 === 0, and both prescribed tests are present. Measured rather than inferred — echo a\ #b ; echo QQDANGER and echo a\;#b ; echo QQDANGER each split into 2 segments at head, exactly as on base. This comment says nothing further about that path.
Finding 1 — (( … )) arithmetic: a # inside it opens a "comment" and swallows a same-line ; (new; no filed feedback names this trigger)
The new branch runs after the (( handler has incremented arithmeticDepth, but it never tests it:
if (ch === '#' && isCommentStart(command, i)) {
while (i < command.length && command[i] !== '\n') {
i++;
}
i--;
continue;
}bash does not recognise comments inside arithmetic, so for (( 1 #2 )) ; <second command> the # is preceded by a space, isCommentStart returns true, and the scan consumes the rest of the line — )) and ; included.
$ bash -c '(( 1 #2 )) ; echo QQDANGER'
bash: ((: 1 #2 : syntax error: invalid arithmetic operator (error token is "#2 ")
QQDANGER
Exit status 0: bash rejects the arithmetic but, because ; separates two commands, it goes on and runs the second one. The splitter now returns a single segment instead:
| input | base 87437db784f6 |
head f3cff68906ce |
|---|---|---|
(( 1 #2 )) ; echo QQDANGER |
['(( 1 #2 ))', 'echo QQDANGER'] |
['(( 1 #2 )) ; echo QQDANGER'] |
(( 1 # 2 )) ; echo QQDANGER |
2 segments | 1 segment |
Counting only fail-open divergences — bash runs the trailing command but the parser gives it no segment of its own — over a 21-shape battery: base 0/21, head 2/21.
Why that is a permission bypass and not just a misparse: both PermissionManager call sites gate their per-command evaluation on subCommands.length > 1, so collapsing to one segment skips it entirely.
permission-manager.ts:956— the per-segmentfindMatchingDenyRuleloop is skipped and control falls through to the single-context match over the whole string.matchesCommandPatterndocuments "Patterns don't match across operator boundaries. We extract only the first simple command before matching" and anchors its regex with^, so a deny rule on the second command (Bash(rm *)→^rm( .*)?$) cannot match a string beginning((. On base the same line splits and the deny fires.permission-manager.ts:336— same branch:evaluateSingleon the whole string instead ofevaluateCompoundCommandper segment.shell-semantics.ts:2266(walkCompoundCommand, reached fromextractShellOperationsAcrossCommand) calls the same splitter, so the virtual-operation pass inherits the fold instead of compensating for it.
This is the failure the branch's own docstring says it exists to avoid: "treating it as a comment would swallow the rest of the line and merge a real ; boundary away, dropping the rule check the following command would have got."
Fix — guard on arithmetic depth:
if (ch === '#' && arithmeticDepth === 0 && isCommentStart(command, i)) {Mutation-witnessed over the same battery: fail-open divergences 2 → 0, and every genuine comment shape still folds to one segment (echo a #c, echo -n hi #c, printf '%s' a #c, declare -i n=1 #c), so the #11815 behaviour this PR exists for is preserved. The $(( … )) expansion forms need no handling either way — bash aborts the whole line with bad substitution, so folding there already matches what bash runs.
Finding 2 — the same guard also closes dev-bot's "Non-blocking (b)", and that ranking understates it
dev-bot's CHANGES_REQUESTED of 11:12:24Z (filed at the older head 53fd793cf186) reports that the comment scan skips the )) handling, so arithmeticDepth stays raised and a later & stops being a boundary. That is still live at this head, and the one-line guard above closes it too — measured on dev-bot's own input:
| input | base | head | with guard |
|---|---|---|---|
echo $(( 1 # 2 )) ⏎ echo a & echo DANGER2 ⏎ ; echo QQDANGER |
4 segments | 3 (echo a & echo DANGER2 merged) |
4 — identical to base |
So findings 1 and 2 share one root cause and one fix; they are not two separate edits.
Finding 3 — dev-bot's "Non-blocking (a)" is fail-open too, and the arithmetic guard does not cover it
For parameter expansion, bash starts no comment after whitespace inside ${…}, but the splitter does. Measured:
| input | bash | base | head | with guard |
|---|---|---|---|---|
echo ${x:-a #b} ; echo QQDANGER |
runs QQDANGER, rc=0, no diagnostic |
2 segments | 1 ⇒ fail-open | 1 ⇒ still fail-open |
x=; echo ${x:-a #b} ; echo QQDANGER |
runs QQDANGER, rc=0 |
3 segments | 2 ⇒ fail-open | 2 ⇒ still fail-open |
Same consequence as finding 1 — a ; boundary bash honours is folded away, so the trailing command never reaches its own deny-rule check — and a regression against base, which split both correctly. It needs its own fix (tracking ${…} nesting the way arithmeticDepth tracks ((…))), and it should not be closed by the arithmetic guard landing.
Test gap
The 11 tests added to permission-manager.test.ts cover an escaped space, an escaped operator, # straight after an operator, a quoted #, a leading #, the comment-ending newline and a trailing backslash — but no arithmetic and no parameter expansion (no added line contains ((). One (( 1 #2 )); <denied command> case and one echo ${x:-a #b}; <denied command> case, driven through PermissionManager and asserting the deny still fires, would pin all three findings in the direction that matters.
Scope, as of the state fetched immediately before posting
The parse divergence is measured from the verbatim-extracted parser source at both refs plus the bash oracle; the consumer skip is read from the three call sites named above. I did not execute the real PermissionManager end-to-end, so this comment makes no claim either way about whether some further runtime layer compensates — the claim is that the splitter's own output regressed fail-open and that both of its consumers branch on segment count. This comment carries no approval of any other aspect of the PR.
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
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 — "Test (windows-latest, Node 22.x)" was skipped in CI and its suite did not run locally on Windows; R1-1 is a cmd.exe-only defect and cmd.exe could not be executed on the Linux review runner.
Not reviewed: build-and-test — "Integration Tests (CLI, No Sandbox)" was skipped in CI and its suite did not run locally; the scoped npm test run covers unit suites only.
Not reviewed: verifier-incidental T1-4 (evaluateCompoundCommand / findMatchingDenyRule divergence in untouched permission-manager.ts) — the budget stop left no verification round to rule on it, so it is carried low-confidence and terminal-only.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): src/code-mode/host.ts — no such file or directory; Tests 444 passed — this review observed 26394, 2070, 31271, 1016, 2025, 587, 8069 passed; 9 passed — this review observed 26394, 2070, 31271, 1016, 2025, 587, 8069 passed; 444 passed — this review observed 26394, 2070, 31271, 1016, 2025, 587, 8069 passed; 921 passed — this review observed 26394, 2070, 31271, 1016, 2025, 587, 8069 passed; and 2 more.
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — "Test (windows-latest, Node 22.x)" was skipped in CI and its suite did not run locally on Windows; R1-1 is a cmd.exe-only defect and cmd.exe could not be executed on the Linux review runner.
未审查（原文为英文）：build-and-test — "Integration Tests (CLI, No Sandbox)" was skipped in CI and its suite did not run locally; the scoped npm test run covers unit suites only.
未审查（原文为英文）：verifier-incidental T1-4 (evaluateCompoundCommand / findMatchingDenyRule divergence in untouched permission-manager.ts) — the budget stop left no verification round to rule on it, so it is carried low-confidence and terminal-only.
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
Test Plan（非阻断）：src/code-mode/host.ts — no such file or directory; Tests 444 passed — this review observed 26394, 2070, 31271, 1016, 2025, 587, 8069 passed; 9 passed — this review observed 26394, 2070, 31271, 1016, 2025, 587, 8069 passed; 444 passed — this review observed 26394, 2070, 31271, 1016, 2025, 587, 8069 passed; 921 passed — this review observed 26394, 2070, 31271, 1016, 2025, 587, 8069 passed; and 2 more。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| * deliberately absent: bash starts a word there too, but leaving them literal | ||
| * only over-splits, which stays fail-closed, and no reported shape needs them. | ||
| */ | ||
| const COMMENT_WORD_BOUNDARIES = [' ', '\t', '\n', ';', '&', '|']; |
There was a problem hiding this comment.
[Critical] R1-1: [certifies-falsely] [regression] This character set is bash's, but the comment state built on it is applied unconditionally — including when run_shell_command spawns cmd.exe on Windows without Git Bash, where # is not a comment character while & still separates commands. The fold therefore removes a real cmd boundary, and a command cmd.exe actually executes loses its own rule check. Nothing under packages/core/src/permissions/ consults the shell type, so the bash assumption reaches every shell.
It bites on any Windows cmd line that contains a # before an &. With permissionsAllow: ['Bash(echo *)'] and permissionsDeny: ['Bash(del *)'], the command echo hi # c & del /f /q C:\Users\me\notes.txt folds into one segment that the anchored ^echo( .*)?$ pattern covers, so the verdict moves from deny at the merge base to allow here and cmd.exe deletes the file with no prompt. With deny-only rules it moves deny to ask, which any auto-approve path bypasses.
Witness:
splitCompoundCommand BASE: ["echo hi # c","del /f /q C:\Users\me\notes.txt"]
splitCompoundCommand HEAD: ["echo hi # c & del /f /q C:\Users\me\notes.txt"]
evaluate allow Bash(echo *) + deny Bash(del *): BASE deny -> HEAD allow
evaluate deny-only Bash(del *): BASE deny -> HEAD ask
CONTROL "echo hi & del /f /q ..." (no #): BASE deny -> HEAD deny
cmd-shaped sweep (BASE segs -> HEAD segs):
"echo 'a' & del /f /q C:\x" 2 -> 2 unchanged
"echo a ; del /f /q C:\x" 2 -> 2 unchanged (`;` over-splits for cmd: pre-existing, fail-CLOSED)
"echo a && del /f /q C:\x" 2 -> 2 unchanged
"echo hi # c & del /f /q ..." 2 -> 1 NEW, fail-OPEN
grep getShellConfiguration|ShellType|isWindows|process.platform|win32
over packages/core/src/permissions/** -> only test-file hits
This is a regression rather than a pre-existing platform mismatch: every other cmd-shaped mismatch measured above is unchanged or over-splits fail-closed, and the comment state is the first thing that removes a real cmd boundary. One limit is worth stating plainly — cmd.exe could not be executed on the Linux review runner, so its tokenizer semantics are triangulated from this repo's own cmd guidance at packages/core/src/tools/shell.ts:5136-5152, whose cmd metacharacter list is &, |, <, >, ^, %VAR% with no # and which says not to use ; or newlines to separate commands in cmd.exe, plus the measured splitter and verdict flip above.
To fix it, make the comment state conditional on the executing shell instead of unconditional — for example an options parameter on splitCompoundCommandSegments and splitCompoundCommand ({ comments?: boolean }, defaulting to true for existing callers) that the permission callers set from getShellConfiguration().shell === 'bash', so cmd input keeps treating # as an ordinary word character.
The gate has to key on the reported shell, not on the operating system: packages/core/src/tools/shell.ts:4793 already reads if (getShellConfiguration().shell !== 'bash') {, and Git Bash or MSYS2 on Windows reports shell: 'bash' (packages/core/src/utils/shell-utils.ts:186-199), so an isWindows()-based gate would strip comment handling from Windows users whose commands really do run under bash and regress the #11815 fix on that platform.
A test that would pin this is a permission-manager.test.ts case on the non-bash path asserting that with comments disabled splitCompoundCommand('echo hi # c & del /f x', { comments: false }) returns ['echo hi # c', 'del /f x'] and that evaluate with permissionsDeny: ['Bash(del *)'] returns deny; please remove the shell gate and confirm that case reds while the bash-side rows added here stay green.
中文说明
[Critical] R1-1：这个字符集是 bash 的，但基于它建立的注释状态被无条件套用——包括在 Windows 上没有 Git Bash 时 run_shell_command 通过 cmd.exe 执行的场景。cmd 里 # 不是注释符，而 & 仍然是命令分隔符，因此这次折叠会移除一个真实的 cmd 边界，让 cmd.exe 确实会执行的命令失去自己的规则检查。packages/core/src/permissions/ 下没有任何代码查询 shell 类型，所以 bash 假设覆盖了所有 shell。
任何在 & 之前含有 # 的 Windows cmd 命令行都会触发。在 permissionsAllow: ['Bash(echo *)'] 与 permissionsDeny: ['Bash(del *)'] 下，命令 echo hi # c & del /f /q C:\Users\me\notes.txt 折叠为一个被锚定模式 ^echo( .*)?$ 覆盖的片段，于是判定从合并基线的 deny 变为这里的 allow，cmd.exe 会毫无提示地删除该文件；只有 deny 规则时则从 deny 变为 ask，任何自动批准路径都会绕过它。
证据（与合并基线 87437db784 做 A/B，运行真实的 splitCompoundCommand 与 PermissionManager.evaluate）：基线切出 2 个片段、判定 deny；本 head 切出 1 个片段、判定 allow（deny-only 时为 ask）；不含 # 的对照组两侧都是 deny。cmd 形态扫描显示 echo 'a' & del、echo a ; del、echo a && del 三种形态片段数不变（其中 ; 对 cmd 属于既有的、朝收紧方向的过度切分），只有 echo hi # c & del 从 2 个片段变成 1 个——新增且朝失效即放开方向。对 packages/core/src/permissions/** grep getShellConfiguration|ShellType|isWindows|process.platform|win32 只命中测试文件。
这是回归而非既有的平台不匹配：上面测到的其他 cmd 形态不匹配要么不变、要么朝收紧方向过度切分，而注释状态是第一个移除真实 cmd 边界的。有一点需要明确说明——Linux 评审机上无法执行 cmd.exe，所以它的分词语义是由本仓库自己的 cmd 指南推得的（packages/core/src/tools/shell.ts:5136-5152：cmd 元字符列表是 &、|、<、>、^、%VAR%，其中没有 #，并写明不要用 ; 或换行分隔 cmd 命令），再加上上面实测的切分与判定翻转。
修复方向：让注释状态依执行 shell 而定，而非无条件生效——例如给 splitCompoundCommandSegments 与 splitCompoundCommand 加一个选项参数（{ comments?: boolean }，对现有调用方默认 true），由权限侧调用方按 getShellConfiguration().shell === 'bash' 设置，使 cmd 输入继续把 # 当普通词字符。
这个开关必须依据上报的 shell 而非操作系统：packages/core/src/tools/shell.ts:4793 已有 if (getShellConfiguration().shell !== 'bash') {，而 Windows 上的 Git Bash 或 MSYS2 上报的是 shell: 'bash'（packages/core/src/utils/shell-utils.ts:186-199），所以基于 isWindows() 的开关会把注释处理从那些命令确实跑在 bash 下的 Windows 用户身上剥掉，反而在该平台上回归 #11815 的修复。
能钉住它的测试是 permission-manager.test.ts 里一个非 bash 路径的用例：在关闭注释时断言 splitCompoundCommand('echo hi # c & del /f x', { comments: false }) 返回 ['echo hi # c', 'del /f x']，且 evaluate 在 permissionsDeny: ['Bash(del *)'] 下返回 deny；请移除该 shell 开关并确认这条用例变红，同时这里新增的 bash 侧用例行保持绿色。
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
Verified the mechanical half of this claim against the code at f3cff68906: the comment branch is entered unconditionally, and grep -rE 'getShellConfiguration|ShellType|isWindows|process.platform|win32' packages/core/src/permissions still hits only test files, so the bash assumption does reach every shell. echo hi # c & del /f /q x is one folded segment there, so an allow on Bash(echo *) does cover a Bash(del *) deny for a cmd.exe user.
I am not fixing this in this PR, and I am not resolving this thread either, because the fix as described is wider than the PR: the flag has to be threaded through splitCompoundCommand / splitCompoundCommandSegments (permissions/rule-parser.ts), extractShellOperationsAcrossCommand / walkCompoundCommand (permissions/shell-semantics.ts) and their callers in permission-manager.ts (four sites) and autoMode.ts - and gating only the comment state would still leave the splitter bash-shaped for cmd in exactly the ways your own sweep measured (; over-splits for cmd at the merge base too). So the open question is a design one, not a missing guard: which owner holds shell attribution for a splitter that is deliberately shell-agnostic. Recorded with your witness in #11882.
Leaving this open for a maintainer call rather than half-gating one of the two consumers.
There was a problem hiding this comment.
[Critical] R1-1: [certifies-falsely] [regression] This character set is bash's, but the comment state built on it is applied unconditionally — including when run_shell_command spawns cmd.exe on Windows without Git Bash, where # is not a comment character while & still separates commands. The fold therefore removes a real cmd boundary, and a command cmd.exe actually executes loses its own rule check. This was filed in round 1 and declined for this PR; it is re-confirmed unchanged at this head, and there is no maintainer ruling on the deferral in the thread.
On Windows with the default ComSpec, getShellConfiguration() returns { executable: comSpec, argsPrefix: ['/d','/s','/c'], shell: 'cmd' } and that same string reaches cmd.exe verbatim. With permissions.allow: ["Bash(echo *)"] and permissions.deny: ["Bash(del *)"], echo hi # c & del /f /q x folds to one segment here where the merge base produced two; matchesCommandPattern('echo *', …) is true and ('del *', …) is false, so the verdict is allow and cmd.exe deletes the file with no prompt. Nothing downstream catches it — the single-segment branch goes to evaluateSingle over the whole string and permission-helpers.ts:135-141 assigns that straight to finalPermission. PowerShell is unaffected, because # is a comment there, so a blanket Windows gate would be wrong.
Witness:
PR "echo hi # c & del /f /q x" segs=1 verdict=allow
BASE "echo hi # c & del /f /q x" segs=2 verdict=deny ["echo hi # c","del /f /q x"]
PR "git status # sync notes & del /q x" segs=1 verdict=allow
BASE "git status # sync notes & del /q x" segs=2 verdict=deny
grep -rn 'getShellConfiguration|ShellType|isWindows|process.platform|win32|ComSpec'
packages/core/src/permissions/*.ts (excluding tests) -> zero hits
witness: not run — cmd.exe itself could not be executed on this Linux review runner, so the
shell-behaviour half is ruled from the in-repo verbatim-spawn path
(shellExecutionService.ts:792-808, windowsVerbatimArguments) plus cmd.exe's documented
tokenization, corroborated by your own reply on comment 4007378571 verifying the mechanism.
Attribute the comment state to the shell that will actually run the command instead of leaving it unconditional: add an optional parameter to both entry points, default it to true so every existing bash row stays green, and have the four PermissionManager call sites pass getShellConfiguration().shell !== 'cmd'. Key it on the resolved shell field, not on process.platform. If the maintainers would rather accept the deferral to #11882, that acceptance needs to be recorded on this PR as an approved workaround — an author-decided scope cut is not the maintainer-approved category, and this should not merge on an unresolved fail-open Critical without it.
export function splitCompoundCommandSegments(
command: string,
options: { comments?: boolean } = {},
): Segment[] {
const comments = options.comments ?? true;
// …
if (
comments &&
ch === '#' &&
arithmeticDepth === 0 &&
paramDepth === 0 &&
isCommentStart(command, i)
) {The new parameter has to be optional with a bash-shaped default, because splitCompoundCommand is called single-argument at permission-manager.ts:336, :956, :1112 and :1210 and at shell-semantics.ts:2266, as well as by every new test row in this diff — a required parameter breaks all five production sites. The gate must also key on getShellConfiguration().shell and never on the OS: shell-utils.ts:184-197 returns shell: 'bash' under Git Bash/MSYS while :222-227 returns 'cmd' only for the plain-cmd Windows default, and tools/shell.ts:4789-4794 states that gating on the active shell rather than the platform is what keeps Windows + Git Bash working.
Please add a permission-manager.test.ts case that resolves the shell to cmd and asserts evaluate returns deny for echo hi # c & del /f /q x under those allow/deny rules, plus a splitCompoundCommand assertion that cmd still yields two segments and bash one — then delete the shell gate and confirm both go red.
中文说明
[Critical] R1-1：这组字符是 bash 的，但基于它建立的注释状态被无条件应用——包括 Windows 上没有 Git Bash 时 run_shell_command 通过 cmd.exe 执行的情况，而 cmd.exe 里 # 不是注释字符、& 仍然是命令分隔符。因此这次折叠会抹掉一个真实的 cmd 边界，让 cmd.exe 确实会执行的命令失去自己的规则检查。该问题在第 1 轮已提出、本 PR 选择暂不修复；在当前 head 上重新确认机制依旧存在，且 thread 中没有维护者对该延期作出裁定。
在 Windows 默认 ComSpec 下，getShellConfiguration() 返回 { executable: comSpec, argsPrefix: ['/d','/s','/c'], shell: 'cmd' }，同一字符串原样送达 cmd.exe。在 permissions.allow: ["Bash(echo *)"] 与 permissions.deny: ["Bash(del *)"] 下，echo hi # c & del /f /q x 在这里折叠为一个片段（合并基线为两个），matchesCommandPattern('echo *', …) 为真而 ('del *', …) 为假，于是判定为 allow，cmd.exe 在没有任何提示的情况下删除文件。下游没有任何环节能拦住它——单片段分支走 evaluateSingle 处理整串，permission-helpers.ts:135-141 直接把它赋给 finalPermission。PowerShell 不受影响（那里 # 就是注释），所以「Windows 一律关闭」的门是错的。
修复方向：把注释状态归属到实际执行命令的 shell，而不是无条件启用——给两个入口加可选参数、默认为 true 以保证现有 bash 用例全绿，并让四个 PermissionManager 调用点传入 getShellConfiguration().shell !== 'cmd'。必须以解析出的 shell 字段为依据，而不是 process.platform。如果维护者更愿意接受延期到 #11882，该认可需要在本 PR 上明确记录为「已批准的兜底方案」——作者自行决定的范围裁剪不属于这一类，而未解决的 fail-open Critical 之上不应合入。
约束：新参数必须是可选的、且默认按 bash 处理，因为 splitCompoundCommand 在 permission-manager.ts:336、:956、:1112、:1210 与 shell-semantics.ts:2266 都是单参数调用，本 diff 的每条新测试也是；必填参数会同时破坏这五个生产调用点。门也必须以 getShellConfiguration().shell 为依据而非操作系统：shell-utils.ts:184-197 在 Git Bash/MSYS 下返回 shell: 'bash'，:222-227 只在纯 cmd 的 Windows 默认情形返回 'cmd'，而 tools/shell.ts:4789-4794 明确说明按活动 shell 而非平台判定才能保证 Windows + Git Bash 用户正常。
请补一个 permission-manager.test.ts 用例：把 shell 解析为 cmd，断言在上述 allow/deny 规则下 evaluate 对 echo hi # c & del /f /q x 返回 deny，并补一条 splitCompoundCommand 断言（cmd 仍切两段、bash 一段）——然后删掉 shell 门，确认两条都变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
Not taking this one inside this PR — asking for a ruling instead, because the fix is the design question this repo already tracks rather than a local patch.
The rule is documented as bash's own at rule-parser.ts (COMMENT_WORD_BOUNDARIES), and that comment records this exact platform gap and the deferral: "the rule is applied on every platform although cmd.exe has no # comment character. Converging the two splitters, and deciding how a shell-agnostic splitter learns the reported shell, is tracked in #11882."
The measurement is not in dispute: folding echo hi # c & del /f /q x into a single segment on a cmd shell is a real fail-open, and your gate keying on getShellConfiguration().shell === 'bash' rather than on the OS is the right key (Git Bash reports bash, and a Windows gate would regress #11815 for those users).
What a closeout pass cannot settle is which of the two designs #11882 should land: an explicit { comments?: boolean } on both splitCompoundCommandSegments and splitCompoundCommand, threaded through every permission caller, or a shell-derived default read inside the splitter — which changes the behaviour of an exported function for every caller, including the non-permission ones, and is precisely why the explicit-option form exists. That decision moves a security-relevant boundary on Windows for both the permission path and the shell-confirmation path, so it wants a maintainer ruling rather than a worker guess. Asking for it here; the head keeps the comment state unconditional, as it was when this was filed in round 1.
There was a problem hiding this comment.
Re-verified at f8f5fefc6a (the commit that landed after my last reply, "keep an arithmetic ) out of the substitution depth"). The measurement is unchanged and the comment state is still unconditional, so I am leaving this thread unresolved.
Witness, running the real splitCompoundCommand from both trees plus PermissionManager.evaluate (merge base 87437db784 vs this head):
base segs ["echo hi # c","del /f /q x"]
head segs ["echo hi # c & del /f /q x"]
evaluate allow Bash(echo *) + deny Bash(del *) = allow (base: deny)
evaluate deny-only Bash(del *) = ask (base: deny)
control "echo hi & del /f /q x" head segs = ["echo hi","del /f /q x"] verdict = deny
The path is intact at this head: permission-manager.ts:336 (also :956, :1112, :1210) takes the single-segment branch into evaluateSingle over the whole string, and shellExecutionService.ts:792-806 spawns getShellConfiguration()'s executable with windowsVerbatimArguments: isWindows && shell === 'cmd', so the same string reaches cmd.exe verbatim. grep -rnE 'getShellConfiguration|ShellType|isWindows|process\.platform|win32|ComSpec' packages/core/src/permissions/*.ts (non-test) still returns zero hits. cmd.exe itself was not executed on this Linux runner — its tokenization remains inferred from the verbatim spawn path and this repo's own cmd metacharacter guidance, as in your round-2 note.
Why this pass does not fix it locally. Nothing but the command string reaches the splitter today (rule-parser.ts:944 splitCompoundCommandSegments(command: string), :1145 splitCompoundCommand(command: string)), so a shell-aware comment state has only two shapes:
- Your explicit
{ comments?: boolean }, threaded throughrule-parser.ts, all fourpermission-manager.tssites andshell-semantics.ts:2266, plus both test files — five files, past the three-file ceiling this closeout pass works under. Gating onlypermission-manager.tswould fit the ceiling but would leave the two consumers of one splitter disagreeing on cmd, which is a new divergence rather than a fix. - Reading ambient
getShellConfiguration()insiderule-parser.ts. That fits the ceiling file-wise, but it changes the behaviour of an exported, deliberately shell-agnostic function for every caller based on process environment, and it makes this PR's bash-shaped rows machine-dependent (there is aTest (windows-latest, Node 22.x)lane atci.yml:1653). That is exactly one of the two designs shell comment semantics: gate on the reported shell, and converge the two compound-command splitters #11882 exists to choose between, so shipping it here would pre-empt the ruling rather than implement it.
One correction to my earlier reply: every call site is in packages/core (permission-manager.ts ×4, shell-semantics.ts ×1), not across two packages — the scope problem is the file count and the design fork, not a package boundary.
#11882 is open (priority/P1, type/bug, category/security, need-discussion) and its section 2 is this fail-open with the same witness and the same threading requirement. The ask is unchanged: either a maintainer ruling on which design lands there (or that this PR should carry the explicit-option form as an approved workaround), or a recorded acceptance of the deferral. I am not resolving a fail-open Critical on my own scope call.
There was a problem hiding this comment.
[Critical] R1-1: [certifies-falsely] [regression] Still standing at this head. The comment state is entered with no shell or platform gate, but on a default Windows host run_shell_command executes through cmd.exe /d /s /c, where # is an ordinary character and & is an unconditional command separator. The fold therefore removes a real cmd.exe boundary that the merge base honoured, so the tail command loses its own rule check and an allow rule on the leading verb covers it.
On a default Windows host with permissionsAllow ['Bash(echo *)'] and permissionsDeny ['Bash(del *)'], the command echo hi # c & del /q C:\temp\x folds to one segment and evaluates to allow with no prompt, while the identical command without the # splits in two and denies; cmd.exe then runs the echo and the del. The command string is model-generated, so a prompt-injected # before an & is a one-token bypass of a configured hard deny. Every one of the four Bash-rule paths is affected, since none consults the resolved shell.
Witness:
Measured on the real built code in both arms, same input: PR `echo hi # c & del /q C:\temp\x` -> segments=1 verdict=allow; BASE -> segments=2 verdict=deny. The comment-free control splits and denies in both arms, so the comparator reports a non-difference where there is one and the delta on the comment rows is real — note this makes the regression deny->allow, stronger than the ask->allow originally reported. `echo ok # done & del /f /q C:\build` behaves identically. Reachability in-tree: shell-utils.ts:223-227 returns `{ executable: comSpec, argsPrefix: ['/d','/s','/c'], shell: 'cmd' }` by default on Windows; shellExecutionService.ts:792-794 hands the same raw string to cmd.exe; and `grep -rn "isWindows|getShellConfiguration|process.platform|ShellType|'cmd'" packages/core/src/permissions/` has zero hits in the permission path. Declaration: cmd.exe itself was not executed (Linux runner, no wine) — its tokenizer rests on Microsoft's documented syntax, this repo's own prompt text at tools/shell.ts:5133-5136, and the diff's own doc comment.
Gate the comment state on the resolved shell rather than shipping it unconditionally: thread ShellType from getShellConfiguration() into splitCompoundCommandSegments and require shell !== 'cmd' before entering the # skip, with an unknown shell falling back to no fold (the fail-closed direction). Key on shell type, not on process.platform, so Windows with Git Bash keeps this fix. If the full plumbing belongs to #11882, ship the gate now and leave the convergence to that issue — the deferred half is the fail-open one.
Fix constraint: shell-utils.ts:92 — export type ShellType = 'cmd' | 'powershell' | 'bash';. PowerShell does treat # as a comment, so the gate must exclude only 'cmd'; a gate written as "only bash folds comments" would re-break the fix for PowerShell users, and a process.platform === 'win32' gate would break Windows with Git Bash, which getShellConfiguration() reports as 'bash'. The in-repo idiom is tools/shell.ts:4951-4956, if (getShellConfiguration().shell !== 'bash') {, under a comment saying to gate on shell type rather than OS platform. The bash-side rows this diff adds are unconditional, so a platform gate must be evaluated at call time rather than captured in a module-level constant.
A permission-manager.test.ts case stubbing the resolved shell to cmd and asserting pm.evaluate is not allow for echo ok # done & del /f /q C:\build under permissionsAllow ['Bash(echo *)'] and permissionsDeny ['Bash(del *)'], plus a splitCompoundCommand row asserting the cmd path still returns two segments while the bash path folds to one. Removing the gate must turn both red.
中文说明
在当前 head 上依旧存在，并且这次是用实测而非阅读重新确认的。注释状态的进入没有任何 shell 或平台前置判断，但在 Windows 默认主机上 run_shell_command 是通过 cmd.exe /d /s /c 执行的，那里 # 是普通字符而 & 仍然是无条件命令分隔符——于是折叠抹掉了一个合并基线本来尊重的真实 cmd.exe 边界，尾部命令失去自己的规则检查，而针对首个动词的 allow 规则把它一并覆盖了。
在 allow ["Bash(echo *)"]、deny ["Bash(del *)"] 下，echo hi # c & del /q C:\temp\x 折叠为一个片段并判定为 allow、完全不提示，而去掉 # 的同一条命令切成两段并判定为 deny；cmd.exe 随后会把 echo 和 del 都执行。四条 Bash 规则路径全部受影响，因为没有一条会查询解析出的 shell。命令串由模型生成，所以在 & 之前注入一个 # 就能用单个 token 绕过一条已配置的硬 deny。
有一处修正让本条比最初提出时更强：无注释的对照组在 head 实测为 deny 而不是 ask，因此差异是 deny 变 allow。
请把注释状态归属到真正会执行该命令的 shell：把 getShellConfiguration() 的 ShellType 传进 splitCompoundCommandSegments，在进入 # 跳扫之前要求 shell !== 'cmd'，未知 shell 则回退为不折叠（失效即收紧的方向），同时保留本 PR 修好的 bash 行为。请以解析出的 shell 字段为依据，绝不要用 process.platform，这样 Windows + Git Bash 仍能享有本修复。如果完整的参数贯通属于 #11882，那就先上这道门、把收敛留给那个 issue——被延期的是失效即放开的那一半。
约束：这道门只能排除 'cmd'：PowerShell 确实把 # 当注释（export type ShellType = 'cmd' | 'powershell' | 'bash'，utils/shell-utils.ts:92），而 getShellConfiguration() 在 Git Bash/MSYS 下返回 shell: 'bash'、只在纯 cmd 的 Windows 默认情形返回 'cmd'（:187-199、:223-227）。本 diff 新增的 bash 侧断言都是无条件的，所以平台门必须在调用时求值，不能固化进模块级常量；仓库内现成写法见 tools/shell.ts:4951-4956，它在一段「按 shell 类型而非操作系统平台判断」的注释下使用 if (getShellConfiguration().shell !== 'bash') {。
请补一个 permission-manager.test.ts 用例：把 shell 解析为 cmd，在上述 allow/deny 规则下断言 evaluate 对 echo ok # done & del /f /q C:\build 不是 allow；再补一条 splitCompoundCommand 断言，cmd 路径仍切两段而 bash 路径折叠为一段——然后删掉这道门，确认两条都变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
The comment state added for #11815 was entered unconditionally, so it also folded the tail of a line bash really runs: - inside $(( )) and (( )), where bash performs no comment recognition, and where swallowing the closing )) also stranded arithmeticDepth above zero, muting every later bare &; - inside a ${ } parameter expansion, where a space-preceded # is literal; - past an unescaped backtick, the closing delimiter of a backtick substitution, which bash finds with a raw scan that does not honour the #. Each one turned a configured deny into ask or allow on a command bash does execute. Gate the branch on the depths the scan already tracks, stop the skip at an unescaped backtick, and pin all of it with splitter- and PermissionManager- level rows, including a quoted # preceded by a space so the quote check's ordering is tested rather than assumed. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmu1gekl14p
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 199 passed · 0 failed · 199 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：199 通过 · 0 失败 · 199 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11821 deep verification —
|
| arm | FAIL-OPEN | OVER-SPLIT | agree | disagrees with bash |
|---|---|---|---|---|
base af4da347 |
1 | 30 | 30 | 31 / 61 |
head 50459aeb |
0 | 6 | 55 | 6 / 61 |
25 cells changed between arms; all 25 moved toward bash (asserted per cell, not in aggregate). Zero cells regressed. Witness: 01-bash-oracle-ab-head-vs-base.png.
Coverage by family: reported-defect shapes (7), # at index 0 (2), newline/continuation (4), non-IFS whitespace \r \v \f \u00a0 (4), IFS and operator boundaries (5), mid-word and escaped boundaries (3), quoted # (4), the #11765 sibling quote-escape shape (1), arithmetic (4), parameter expansion (6), command substitution $( ) (5), backtick (4), process substitution (1), redirection (2), subshell and brace group (2), heredoc (3), controls (4).
Table 2 — randomized differential fuzz, 1500 cells (harness-fuzz-bash-oracle.mjs)
| arm | FAIL-OPEN | OVER-SPLIT |
|---|---|---|
| base | 19 | 463 |
| head | 12 | 226 |
novel-to-head fail-open = 0 — head's fail-open set is a strict subset of base's, which is the precise statement of "this PR introduces no new fail-open". Regressions (base saw the canary, head did not, bash ran it) = 0. Corpus generated from a grammar mixing every context the scanner tracks with every SHELL_OPERATORS entry and every non-IFS whitespace character; deterministic xorshift32 seed 0x5eed1182, so the exact corpus is replayable. Witness: 04-fuzz-1500-no-new-fail-open.png.
Table 3 — destination level: real PermissionManager.evaluate() (harness-permission-manager.mjs)
The splitter is a component boundary; between it and the verdict sit rule matching, the virtual-op pass and a branch on subCommands.length > 1. Measured at the destination, 38 cells, 11 verdict flips:
| flip | count | bash ran rm? |
reading |
|---|---|---|---|
ask → allow |
5 | no | #11815 fixed: Bash(echo *) now covers the line |
deny → ask |
3 | no | false-positive deny removed |
ask → deny |
1 | yes | security fix — echo hi # foo \ + \n + rm -rf /tmp/x |
deny → ask |
2 | no | heredoc false positives (declared tradeoff) |
The single ask → deny flip is claim (b): base let the trailing backslash extend the comment across the newline, so the rm bash really executes lost its own rule check. Asserted as an expectation, so the base arm's red is a pass, not a failure. Witness: 03-permission-manager-verdict-flips.png.
Table 4 — heredoc sibling sweep, the one place the PR removes a deny (harness-heredoc-siblings.mjs)
The PR's accepted-tradeoff list names heredoc bodies, so the unnamed siblings of that mechanism were walked: # on the heredoc start line, a real command after the terminator, quoted delimiter, indented body, <<-, two heredocs, a trailing comment line. 9 assertions, 0 failures. Every deny the PR removes is on a command bash genuinely never ran — bash executed only cat in all 7 of those cases (["cat","cat"] for the two-heredoc cell), never rm — and both cases where bash does run the rm (start-line-#-then-real-cmd, real-cmd-after-terminator) keep deny on head. The tradeoff is correctly characterised.
Vacuity and mutation
| check | result |
|---|---|
| base source + head tests | 11 failed | 456 passed (467) — every red is an expected-vs-actual AssertionError, not an import/compile break |
| base source + base tests | 428 passed (attribution baseline) |
head, src/permissions |
10 files, 944 passed, 0 failed |
| positive control M7 (delete the whole comment state) | KILLED — 11 tests red, in the same file as every other mutant |
| mutants killed | 8 / 9 |
The depth guards added by the final commit cannot be validated by a base revert — on base there is no comment state for them to constrain, so those tests pass trivially there. They are pinned against the intermediate variant, which is what the matrix builds. Each kill is attributed to the exact tests that pin it: M1 arithmetic → 5 red; M2 ${ } → 2 red; M3 backtick stop → 2 red; M4 escape-aware boundary → 2 red; M5 \s widening → 4 red (\r \v \f \u00a0); M6 backslash continuation → 2 red; M9 newline swallowing → 4 red. Witness: 02-mutation-matrix-8-of-9-killed.png.
Gates
| gate | result | proven live? |
|---|---|---|
npx vitest run src/permissions --root packages/core |
10 files, 944 passed, 0 failed | yes — the same command turns 11 red under the source revert above |
npx prettier --check (both changed files) |
clean | yes — planted {a:1, b:"two", → [warn] rule-parser.ts, exit 1 |
npx eslint (both changed files) |
clean | yes — planted unused const → 1781:7 error 'calibUnusedVariable' is assigned a value but never used |
npm run typecheck --workspace=packages/core |
clean, exit 0 | n/a |
Corrections
These are corrections to the PR description, not requests to change the code.
C1 — the test counts in the description and Reviewer Test Plan step 3 are stale. The body states 19 new cases, 444 passed (425 before), and 10 failed | 9 passed on main. Measured at head 50459aeb: permission-manager.test.ts holds 467 tests, the base file holds 428, so the PR adds 39 tests, and reverting only the source turns 11 red (11 failed | 456 passed). The four extra reds beyond the description's list are the ||, | and & comment variants and keeps the # semantics of echo ${x} # c …. The numbers were accurate at commit b2b5077 and were overtaken by the two later fix commits. A reviewer following step 3 will see different numbers than promised and may conclude they ran it wrong.
C2 — the typecheck caveat does not reproduce in CI. The description reports 10 pre-existing errors in src/code-mode/host.ts from missing @jitl/quickjs-singlefile-mjs-release-sync / quickjs-emscripten-core, attributed to the author's sandbox. In this container, with the repo's own npm ci install, npm run typecheck --workspace=packages/core exits 0 with no errors. The caveat is a property of that sandbox, not of the repo, and should not be expected by a maintainer.
Findings
Ordered by severity. None is blocking; the verdict rests on 0 unexpected assertion failures.
F1 — [Suggestion] The test named “treats a leading # as a comment” does not pin the index-0 rule
The suite contains it('treats a leading # as a comment', …) asserting splitCompoundCommand('#!/bin/sh\necho hi') → ['#!/bin/sh', 'echo hi']. Mutant M8 flips isCommentStart's index === 0 branch to return false and all 467 tests still pass. This is not dead code — the branch decides real outcomes — so the test passes for the wrong reason: its fixture is pinned by the newline boundary, which produces the same two segments whether or not the # was read as a comment.
Proven, not inferred (harness-m8-and-scaling.mjs, 5 assertions / 0 failures):
| fixture | head | M8 | identical |
|---|---|---|---|
#!/bin/sh\necho hi (the named test's own) |
["#!/bin/sh","echo hi"] |
["#!/bin/sh","echo hi"] |
yes |
#c ; rm -rf /tmp/x (absent from the suite) |
["#c ; rm -rf /tmp/x"] |
["#c","rm -rf /tmp/x"] |
no |
bash runs nothing for the second fixture, so head is correct and M8 over-splits. The gap is therefore fail-closed — removing the branch would only add a spurious deny, never drop one — which is why this is a Suggestion and not a Critical.
Reproduce:
node_modules/.bin/tsx tmp/pr11821-verify-20260914-170025/harness-m8-and-scaling.mjsFixture that would pin it (measured: turns exactly M8 red)
// `#` at index 0 is a word start even when nothing precedes it, so the whole
// line is inert and bash runs nothing here.
it('keeps a line that starts with # as one segment', async () => {
expect(splitCompoundCommand('#c ; rm -rf /tmp/x')).toEqual([
'#c ; rm -rf /tmp/x',
]);
});Verified against both builds: 1 segment on head, 2 segments under M8, bash ran [] for the fixture.
F2 — [Informational, pre-existing — not introduced by this PR] 12 fail-open cells survive on head
The fuzz found 12 inputs where bash executes the canary and no head segment exposes it to a Bash(rm *) rule. All 12 are also fail-open on base (novel-to-head = 0), so this PR neither caused nor widened them; it narrowed the population from 19 to 12. Attribution matters here, so the cause is named separately from this PR's contribution.
Root cause is the scanner's quote and backtick state, not the comment state: an unterminated ' or an unpaired ` swallows everything to end of input, including real boundaries. Representative cell, confirmed at the destination (headV=ask, baseV=ask, bash ran rm):
#c; true & `a#b"\nrm -rf /tmp/x
head -> ["#c; true & `a#b\"\nrm -rf /tmp/x"] (1 segment)
base -> ["#c","true","`a#b\"\nrm -rf /tmp/x"] (3 segments)
PM -> ask on BOTH arms, while bash executes rm
This is the #11765 family the description explicitly keeps separate, and it is a pre-existing fail-open worth its own issue. Reproduce with harness-fuzz-bash-oracle.mjs (deterministic seed) or the three pinned cells in harness-permission-manager.mjs under group fuzz-validate.
F3 — [Informational] Residual over-splits are all fail-closed and all inside the declared scope
6 of 61 curated cells still disagree with bash on head, and in every one the head and base segmentations are byte-identical — the PR did not touch them. Asserted fail-closed per cell (never FAIL-OPEN). Each maps to a limitation the description already names:
| cell | declared in the PR as |
|---|---|
echo hi >#f ; rm -rf /tmp/x |
> not treated as a word start |
echo hi 2>#f ; rm -rf /tmp/x |
same |
echo a # c ` ; rm -rf /tmp/x — a genuine comment containing a backtick |
"can only over-split a genuine comment containing a backtick" |
cat <<EOF\nrm -rf /tmp/x\nEOF |
heredoc bodies out of scope (#9417) |
echo ${a${b} #c ; rm -rf /tmp/x |
consequence of the paramDepth guard |
echo ${x:-$(date #c ; rm -rf /tmp/x)} |
paramDepth guard suppresses comment reading inside a nested $( ) |
The last two are the only ones not literally spelled out in the description. Both are malformed-or-nested inputs, both fail closed, and both follow directly from the guard the matrix shows is load-bearing (M2). Not worth blocking on; worth a line in the description if the author revises it.
F4 — [Note] The sibling-splitter divergence the diff advertises is real and verified
The new doc comment in rule-parser.ts claims splitCommands in utils/shell-utils.ts "has no # state at all and so disagrees on inputs like echo hi # ; rm -rf /tmp/x". Measured:
"echo hi # ; rm -rf /tmp/x"
splitCommands (shell-confirmation path): ["echo hi #","rm -rf /tmp/x"]
splitCompoundCommand (Bash-rule path) : ["echo hi # ; rm -rf /tmp/x"]
"echo a # c && rm -rf /tmp/x"
splitCommands : ["echo a # c","rm -rf /tmp/x"]
splitCompoundCommand: ["echo a # c && rm -rf /tmp/x"]
The claim is accurate and the direction is fail-closed (the confirmation path stays stricter), so no deny can weaken. The user-visible consequence — whether the #11815 prompt actually disappears, given that the confirmation path still splits — is not traced end-to-end here; see Not covered. The diff already points at #11882 for convergence, which is the right disposition.
F5 — [Nice to have] Scaling is flat; no superlinear risk from the new inner skip loop
The new comment scan adds an inner while loop over model-authored text, so a ladder was run through the real code path at 2 k / 3 k / 5 k / 20 k characters across five hostile shapes (runs of spaces, thousands of #, alternating #/operator, thousands of backticks inside a comment, thousands of backslashes before a #). Every shape is flat within noise — e.g. backtick-in-comment 268 / 294 / 281 / 286 ms and alternating-hash-operator 283 / 271 / 268 / 271 ms, where each rung carries a constant ~270 ms of tsx startup. No rung approached the 30 s cap. precedingBackslashCount inside the skip loop is reached only on a backtick (short-circuited), so it cannot degrade the scan. No action needed; recorded because the rule asks for the ladder.
F6 — [Nice to have] Per-commit attribution is out of reach, so the aggregate diff is what was verified
The snapshot lists 5 commits (b2b5077, 53fd793, 9a977d8, f3cff68, 50459aeb); git rev-list HEAD^1..HEAD^2 returns 1, and git rev-parse --is-shallow-repository is true. At a shallow boundary that count returns a plausible small number instead of erroring, so it was checked against the snapshot rather than trusted. Individual commit claims (notably 53fd793's escaped-boundary fix and 9a977d8's IFS-only fix) are covered behaviourally by mutants M4 and M5, both killed by exactly the tests those commits say they added — which is stronger evidence than per-commit checkout would have been, but it is not the same claim and is reported as such.
Not covered
- Per-commit checkout verification — depth-2 shallow clone; only the merge commit,
HEAD^1andHEAD^2exist locally. AggregateHEAD^1..HEADverified instead (see F6). - Base-OID discrepancy, resolved not ignored. The snapshot's
baseRefOidis87437db7(origin/main), which is not the merge ref'sHEAD^1(af4da347); neither is an ancestor of the other under the grafts, andgit merge-basereturns empty. This was checked rather than assumed away:git diff HEAD^1..HEADandgit diff 87437db7..50459aebare byte-identical (diffof the two patches is empty), so the effective change under test is the same either way and the A/B is unaffected. - Windows / cmd.exe. The comment rule is applied on every platform although cmd.exe has no
#comment character. Linux only here; the diff's own comment flags this for shell comment semantics: gate on the reported shell, and converge the two compound-command splitters #11882. Not measured. - End-to-end user-visible prompt for splitCompoundCommandSegments splits on an operator inside a trailing # comment #11815. Verified to the
PermissionManager.evaluate()verdict (ask → allow), which is the destination for thepermissions.allowpath. Whether the shell tool's separate confirmation path (checkCommandPermissions→splitCommands) still prompts on the same input is not traced — F4 shows the two splitters diverge, so this is a real open question rather than a settled one. - Real model traffic / live CLI. No end-to-end
qwenrun; the harnesses drive the compiled-source functions and the realPermissionManagerdirectly, plus real bash. No network calls were made (none are available in this job). walkCompoundCommand/shell-semantics.tsinteraction. That path strips heredoc bodies before splitting, so it sees a different input than the four Bash-rule paths. Only the Bash-rule paths were A/B'd;shell-semantics.test.ts(109 tests) passes in the gate but was not separately mutated.- Replay calibration against a real emitted artifact. Not applicable — this PR changes no workflow or CI script, so there is no production-emitted artifact to calibrate a replay against.
- Repo-wide gates. Only
src/permissionswas run, per the targeted-gate rule. The description's wider regression runs (src/core/*,src/tools/shell*) were not repeated; the PR's own CI covers them.
Assertion accounting
assertions.json reports {"pass": 199, "fail": 0, "total": 199}. Every one maps to a scripted check that actually executed; nothing is projected or estimated. The 199 is 189 harness assertions plus 10 gate assertions:
| source | pass | fail | what it counts |
|---|---|---|---|
harness-bash-oracle.mjs |
105 | 0 | 3 control-live + 61 A1 no-fail-open + 25 A2 changed-cells-move-toward-bash + 1 A2b change-set-nonempty + 6 A5 residual-is-fail-closed + 8 A3 controls-on-both-arms + 1 A4 oracle-live |
harness-permission-manager.mjs |
57 | 0 | 2 PM-live + 26 cells × 2 (head invariant, no-regression) + 3 load-bearing flip (precondition, base arm expected red, head arm) |
harness-mutation-matrix.mjs |
9 | 0 | 8 mutants killed (incl. positive control M7) + 1 source-restored-byte-identical |
harness-heredoc-siblings.mjs |
9 | 0 | per-case: bash-ran-it ⇒ head denies; deny-removed ⇒ bash never ran it |
harness-m8-and-scaling.mjs |
5 | 0 | 2 M8 probes + 2 direction proofs + 1 source-restored |
harness-fuzz-bash-oracle.mjs |
4 | 0 | F1 no-novel-fail-open, F2 no-regression, F3 corpus-exercised, F4 subset-relation |
| gates | 10 | 0 | vitest head green; vacuity 11 red; base baseline 428; prettier clean; prettier live; eslint clean; eslint live; typecheck clean; permission-manager.ts byte-identical across arms; base tree lacks the new symbols |
| total | 199 | 0 |
Two accounting rules were applied deliberately. Expected reds count as passes: the base arm of the load-bearing flip, the vacuity run's 11 failures and the two gate calibrations are all assertions that a control fails, encoded in the harnesses so a fail always means an unexpected outcome — which is why fail: 0 is compatible with a merge-ready verdict. Survivors are not counted as failures: the single surviving mutant M8 is completeness reporting about test coverage (F1), and charging it to fail would let a coverage observation flip the verdict on a PR whose behaviour is correct.
Methodology
Environment: the CI verify job container (node:22-bookworm), working tree at refs/pull/11821/merge (depth 2), npm ci + npm run build already complete at HEAD; GNU bash 5.2.15; Node v22.23.2. Scratch base worktree at tmp/base-tree (HEAD^1), removed after the A/B cells were captured.
The control is clean by construction and was asserted rather than assumed: the PR touches no package.json or package-lock.json (git diff --name-only HEAD^1..HEAD lists two .ts files), so reusing the root node_modules introduces no dependency-tree confound; permission-manager.ts is byte-identical across both trees (diff -q), so the arms differ only by rule-parser.ts; and grep -c isCommentStart on the base file returns 0. Because node_modules/@qwen-code/qwen-code-core realpaths into the head tree (readlink -f → /__w/qwen-code/qwen-code/packages/core), every harness imports the module under test by absolute path into its own tree and never traverses that workspace link — the internal-symlink trap that silently makes both cells head. Two live-identity assertions in harness-bash-oracle.mjs confirm the arms are genuinely different modules (head returns 1 segment, base returns 2, on the same input).
Four harnesses drove the code, all mock-free with respect to the unit under test: (1) harness-bash-oracle.mjs — 61 curated cells, real TypeScript sources imported via tsx, real bash with PATH-shimmed externals logging invocations to a per-cell file; (2) harness-fuzz-bash-oracle.mjs — 1500 grammar-generated cells through the same oracle, deterministic xorshift32 seed 0x5eed1182; (3) harness-permission-manager.mjs — the real PermissionManager.evaluate() on both arms, cross-checked against bash, including the three fuzz cells needed to validate the first-word predicate against the actual verdict; (4) harness-heredoc-siblings.mjs — 11 heredoc shapes at both the splitter and verdict level. harness-mutation-matrix.mjs applies nine string-level mutations to a pristine copy of rule-parser.ts, runs vitest per mutant, and restores byte-identically (asserted, and independently confirmed by git status --porcelain after every run); harness-m8-and-scaling.mjs adjudicates the single survivor and runs the scaling ladder.
One harness defect was caught and fixed rather than shipped: the first mutation-matrix run reported all nine mutants as survivors including the positive control, with total=0 — vitest interleaves ANSI escapes inside its summary line (\e[1m\e[31m11 failed\e[39m), so the parse silently yielded zero failures, which is indistinguishable from a surviving mutant. Stripping ANSI fixed it; the control killing 11 tests is what proved the fix. The gate calibrations (planted unused variable for eslint, planted formatting break for prettier) exist for the same reason — an unproven green gate is an assumption.
Raw per-cell data lives in logs/bash-oracle-rows.json, logs/fuzz-results.json, logs/permission-manager-rows.json, logs/heredoc-siblings.json and logs/mutation-matrix.json; per-mutant vitest output in logs/mutation-M*.log; full console transcripts in logs/*.log. All harnesses are rerunnable as written from the repo root.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/core/src/permissions/permission-manager.test.ts: (cd packages/core) npx --no-install vitest run ./src/permissions/permission-manager.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/permissions/permission-manager.test.ts: PPPPP
verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/core/src/permissions/permission-manager.test.ts: P (exit 0)
round 2 · packages/core/src/permissions/permission-manager.test.ts: P (exit 0)
round 3 · packages/core/src/permissions/permission-manager.test.ts: P (exit 0)
round 4 · packages/core/src/permissions/permission-manager.test.ts: P (exit 0)
round 5 · packages/core/src/permissions/permission-manager.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
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 — "Test (windows-latest, Node 22.x)" was skipped in CI and its suite did not run locally on Windows; R1-1 is a cmd.exe-only defect and cmd.exe could not be executed on the Linux review runner.
Not reviewed: build-and-test — "Integration Tests (CLI, No Sandbox)" was skipped in CI and its suite did not run locally; the scoped npm test run covers unit suites only.
Not explored to full depth (tool budget reached): "agent 2": could not execute cmd.exe on this Linux runner to confirm the Windows verdict end-to-end (finding 2 is reported at Confidence: low for that reason)..
Not reviewed: "agent reverse-audit (round 3)" — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.
Test Plan (not a blocker): src/code-mode/host.ts — no such file or directory; Tests 444 passed — this review observed 26405, 2070, 31270, 1016, 2025, 587, 8069 passed; 9 passed — this review observed 26405, 2070, 31270, 1016, 2025, 587, 8069 passed; 444 passed — this review observed 26405, 2070, 31270, 1016, 2025, 587, 8069 passed; 921 passed — this review observed 26405, 2070, 31270, 1016, 2025, 587, 8069 passed; and 2 more.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/permissions/rule-parser.ts:897 — [probe] tab, & and | in COMMENT_WORD_BOUNDARIES are pinned by no test; deleting & survives the suite green (946/946) while over-splitting echo a&#c ; rm -rf /tmp/x into a deny on a command …
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — "Test (windows-latest, Node 22.x)" was skipped in CI and its suite did not run locally on Windows; R1-1 is a cmd.exe-only defect and cmd.exe could not be executed on the Linux review runner.
未审查（原文为英文）：build-and-test — "Integration Tests (CLI, No Sandbox)" was skipped in CI and its suite did not run locally; the scoped npm test run covers unit suites only.
未探索到全部深度（达到工具调用预算）："agent 2"：could not execute cmd.exe on this Linux runner to confirm the Windows verdict end-to-end (finding 2 is reported at Confidence: low for that reason).。
未审查："agent reverse-audit (round 3)"——启动 prompt 为它指定了 diff 中的行，但它从未打开：有工具调用，却没有一次读取 diff。
Test Plan（非阻断）：src/code-mode/host.ts — no such file or directory; Tests 444 passed — this review observed 26405, 2070, 31270, 1016, 2025, 587, 8069 passed; 9 passed — this review observed 26405, 2070, 31270, 1016, 2025, 587, 8069 passed; 444 passed — this review observed 26405, 2070, 31270, 1016, 2025, 587, 8069 passed; 921 passed — this review observed 26405, 2070, 31270, 1016, 2025, 587, 8069 passed; and 2 more。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
|
Our Everything below was measured immediately before posting, by extracting 1. Every input our review named now segments exactly as it does on base
The prescribed one-line guard landed verbatim ( The 2. We are not re-filing anythingThe one live head-only fail-open regression we can reproduce is already filed at the exact site by
The mechanism is visible in source at 3. On
|
R1-2, R1-4: the comment state this PR added for #11815 applied to two constructs bash scopes differently. A `}` inside a `$( … )` or backtick body no longer closes an enclosing `${ … }` — bash parses the substitution body first, so the expansion stays open and the space-preceded `#` after it is literal. Counting that `}` as the closer dropped the depth while bash was still inside the expansion, and the literal `#` then swallowed the `;`, folding an `rm -rf` into a segment `Bash(echo *)` covers. The comment skip now stops at a backtick only when the `#` sits inside a backtick body; outside one the backtick is comment text bash has discarded, and stopping there handed the tail of the comment back to the state machine, which rebuilt quote, escape and arithmetic state out of it and folded a real boundary away. Tests: splitter and verdict rows for both shapes, for the `> 0` clamp on `paramDepth` (R2-2), and for the escaped-backtick terminator (R2-1, reachable only inside a backtick body). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmu1r4dew54
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
[Critical] $(( … )) defeats the new commandSubDepth clamp — an arithmetic ) is charged to the enclosing $( … ), so a literal } inside the substitution body closes paramDepth, and a later # then swallows a real ;
Measured at head 84bd8ee2bf33161ab8b78f5c79fc69262cf356a0. This is a fail-open regression against the merge base 87437db784f6, in the same class as R1-2 — which this head does close. It is not a re-statement of R1-2; the shapes below contain arithmetic, and the clamp added for R1-2 is what the arithmetic path defeats.
Location
packages/core/src/permissions/rule-parser.ts at head 84bd8ee2bf33:
:1007—if (ch === '$' && command[i + 1] === '(' && command[i + 2] !== '(')deliberately excludes$((fromcommandSubDepth.:1011-1013—if (ch === ')' && commandSubDepth > 0) { commandSubDepth--; }. The decrement is unconditional on why the)appeared.:1083—if (arithmeticDepth > 0 && ch === ')' && command[i + 1] === ')')consumes arithmetic's))as a pair, later in the same iteration than:1011.:1020-1027— the}clamp, gated oncommandSubDepth === 0 && backtickDepth === 0.:1057-1061— the#gate, which requiresparamDepth === 0.
Trigger condition
echo ${x:-$(echo $((1)) }) #c} ; rm -rf /tmp/x
Four arms of the real exported splitCompoundCommand, each extracted verbatim from its own blob (not paraphrased), driven over one 33-input battery with GNU bash 4.4.20 as the oracle — a marker file decides whether bash really executes the tail:
| arm | segments | bash runs the tail |
|---|---|---|
base 87437db784f6 |
2 | yes |
f3cff68906ce |
1 | yes |
50459aeba12e |
1 | yes |
head 84bd8ee2bf33 |
1 | yes |
Three further shapes of the same class, all base 2 / head 1 with bash running the tail:
echo ${x:-$(echo $((1)) } ) #c} ; rm -rf /tmp/xecho ${x:-$(printf pad $((2)) }) #c} ; rm -rf /tmp/xecho ${x:-$(echo $((1)) $((2)) }) #c} ; rm -rf /tmp/x
Mechanism, traced by instrumenting the extracted head module
TRACE i=21 ch=')' -> commandSubDepth=0 paramDepth=1 arith=1 ctx="-$(echo $((1)) "
TRACE i=24 ch='}' DECREMENTED paramDepth=0 (guard passed: commandSubDepth=0 backtickDepth=0)
TRACE i=25 ch=')' NOOP commandSubDepth=0 paramDepth=0 arith=0
TRACE i=27 ch='#' COMMENT BRANCH ENTERED paramDepth=0 arith=0 backtickDepth=0 cmdSub=0
SEGMENTS=["echo ${x:-$(echo $((1)) }) #c} ; rm -rf /tmp/x"]
i=21 is the first ) of $((1)). :1011 runs before :1083 in the same iteration, so that ) is charged to the enclosing $( … ), which is still open, and drops commandSubDepth to 0. At i=24 the literal } inside the substitution body — ordinary text to bash, not a closer — therefore satisfies the new commandSubDepth === 0 conjunct and closes paramDepth while bash is still inside ${ … }. At i=27 the # passes paramDepth === 0, is read as a comment, and the skip-to-newline swallows the real ;.
Impact
Fail-open, and a regression rather than a pre-existing hole: bash executes rm -rf /tmp/x, but the splitter returns one segment, so the tail never receives a rule check of its own and rides inside the segment whose first word is echo. That is precisely the "folded both commands into one allow-covered segment" failure this PR's own comment at :957-960 describes, on the base arm's correct 2-segment behaviour being lost. Base-arm A/B run per severity discipline: base clean, head fail-open ⇒ introduced by this diff.
Why this is not equivalent feedback to R1-2
R1-2's shapes carry no arithmetic and are fixed at this head. The same harness, run in the same session, returns 2 segments for every one of them:
echo ${x:-$(echo } ) #c} ; rm -rf /tmp/x→ 2echo ${x:-$(echo a } ) #c} ; rm -rf /tmp/x→ 2echo ${x:-`echo $((1)) }` #c} ; rm -rf /tmp/x→ 2 (thebacktickDepthconjunct holds)echo ${x:-$(echo }) #c} ; rm -rf /tmp/x→ 2 (R1-2shape (a))echo ${x:-$(ls {a,b}) # c} ; rm -rf /tmp/x→ 2 (R1-2shape (b))echo ${x:-`echo }` #c} ; rm -rf /tmp/x→ 2 (R1-2shape (c))
So the clamp is right; only the $(( path charges a ) to the wrong construct. Also re-confirmed at this head: our three earlier findings stay closed (base == head on all five of their shapes), and no control row moved in the fail-open direction — echo a #c, echo -n hi #c, printf '%s' a #c, declare -i n=1 #c are 1 segment on all four arms, and BOT-R14b (echo hi # + backtick + \ + newline +rm -rf /tmp/x`) went base 1 / head 2 with bash running the tail, i.e. this head repairs a shape that was failing open on base.
Fix direction — measured, not prescribed blind
Three one-site variants were patched into the extracted head module and driven over the whole 33-case battery. All three close Q1/Q2/Q3/Q7 with zero new fail-opens:
- A — gate the decrement on arithmetic:
if (ch === ')' && commandSubDepth > 0 && arithmeticDepth === 0). Ati=21arithmeticDepthis still1, so the enclosing$( … )keeps its depth. - B — drop the
command[i + 2] !== '('exclusion so$((pushescommandSubDepthtoo. - C — gate on the pair explicitly:
!(arithmeticDepth > 0 && command[i + 1] === ')').
The only other segment-count change on the battery is echo ${x:-$(echo $((1+2)) }) #c ; rm -rf /tmp/x going 1 → 2, where bash exits 1 on an unmatched } and runs no tail, so that is the fail-closed direction. Variant A is the smallest. My battery is a sample, not a cover, so whichever variant is taken belongs with a pin for the four shapes above alongside the rows already added to permission-manager.test.ts.
Posture
Time-scoped to the enumeration read immediately before posting (2026-09-14T22:34:29Z, re-read at the instant of this write): head 84bd8ee2bf33, zero verdict rows at head, zero [Critical] inline rows anchored at head (critAtHead=0 over 27 paginated inline rows, no page cap), the review-pr lane still in_progress, product lanes Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox) and Desktop Shell all success at 22:00:3xZ. Our earlier row 5198522181 is not dismissed here and its three enumerated findings remain spent-and-closed; this review carries exactly one finding, the one above. No approval is expressed or implied by this comment.
The `)` decrement on the `commandSubDepth` counter ran before the pair scan,
unconditionally on why the `)` appeared, so the first `)` of `$((1))` was
charged to the enclosing `$( … )` while the arithmetic pair was only consumed
later in the same iteration. With the depth already back at zero, a literal `}`
satisfied the `commandSubDepth === 0` conjunct of the `${ … }` guard and closed
the expansion while bash is still inside it; the word-initial `#` then passed
the `paramDepth === 0` gate and swallowed the real `;`, folding the trailing
command into one allow-covered segment with no rule check of its own.
`echo ${x:-$(echo $((1)) }) #c} ; rm -rf /tmp/x` was 2 segments before the
`$((` support and 1 at the previous head; three sibling shapes (a `}` before the
substitution's own `)`, two arithmetic expansions, a non-`echo` command)
behaved the same. Gating the decrement on `arithmeticDepth === 0` leaves the
arithmetic `))` pair to return the arithmetic depth and the substitution's own
`)` to decrement the substitution depth; all four shapes are back to 2 segments
and the arithmetic-only and grouping-paren controls are unchanged.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmu1t9j8j57
|
Correction to the posture section of my 03:57Z comment, which was drafted against That comment ends with "Changes are still requested on this PR in substance while if (
ch === '}' &&
paramDepth > 0 &&
commandSubDepth === 0 &&
backtickDepth === 0
) {
paramDepth--;
}So a The three shapes from that comment's fail-open table are pinned as regression tests, each asserting two segments with the destructive tail as its own:
plus the What still stands is one thread, not an open fail-open in the bash path. So the accurate read at this head is: product lanes green, 1 of 13 threads unresolved, and that one is a design question with a tracking issue — not a live bash-path fail-open. Requesting re-review on that basis. I am not dismissing anyone's row myself; |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed the comment-state machine and the new depth guards at this head. One Critical inline. It is the same root cause as the now-closed R1-2 in a body form that fix does not cover, not a re-file of it — no thread on this PR mentions process substitution (checked), and the code contains no handling for it.
What I checked for the negative half of this review: the file defines exactly four depth variables (arithmeticDepth, paramDepth, commandSubDepth, backtickDepth) and contains zero occurrences of <( — so the guards that were added for $( … ) and backticks have no counterpart for process substitution. That is the whole of the finding; details inline.
Verified correct, so they do not need re-arguing: the guard conjuncts themselves are sound for the two bodies they cover, and the # gate's dependence on paramDepth === 0 is the right gate. The PR's own new test rows agree with bash semantics — I re-derived the ${…}-in-${…} rows, the arithmetic rows, the backtick rows, the .-comment rows and echo ${x:-{a,b} # c} ; echo SECOND (which correctly pins the fold) from the scanner's state transitions and they are consistent. \t after a before # folding while \r, \v, \f and \u00a0 split matches bash's IFS as implemented. <#c / 2>#c over-splitting is more eager than bash but fails closed, and the COMMENT_WORD_BOUNDARIES comment discloses it as deliberate. a#b, $#, ${#x}, $(( 8#17 )), an escaped space or operator before #, and an escaped newline all behave as bash does. ${ … } inside $( … ) leaving paramDepth stuck above zero only over-splits, which is the safe direction. The one random-battery shape that still folds (}``'a {|#c`) folds identically at the merge base and involves no comment, so it is pre-existing and out of scope here.
Limits: read-only, and I could not execute the splitter or bash on this host — the repository's own daemon shell guard refused the payload, so the mechanism above is read from the code (guard conjuncts, the paramDepth === 0 gate, and the absent <( handling) and the bash side is the same oracle R1-2 already established for the sibling bodies. The R1-2 thread remains the right place to confirm the counterexample end to end once someone can run it; I am reporting the gap, not re-opening that thread.
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
bash's ${ … } close-scan does not treat <( … ) / >( … ) as a body, so
a `}` inside one still closes the inner expansion and the tail after
the closing `}` is a real top-level comment; the splitter already
agrees. Pin the agreement so charging `<( … )` to commandSubDepth
later cannot reintroduce the over-splitting #11815 removed.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmu304qrn6z
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.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-5 the comment rule lands in only one of the package's two splitters — already reported (comment 4006550073; thread open with the author's recorded scope decision and the deferral to #11882)
- tab / & / | entries of COMMENT_WORD_BOUNDARIES pinned by no test — already reported (round-2 review body 5202105476, published there as a deferred non-blocking note)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: test-efficacy probe — the mutation harness's positive control never produced a verdict (its runner tripped the repo's vitest global-setup prerequisite guard), so the mutant and hunk-necessity classes went unmeasured.
Test Plan (not a blocker): src/code-mode/host.ts — no such file or directory; Tests 444 passed — this review observed 26703, 2134, 31665, 1028, 2044, 567, 8191 passed; 9 passed — this review observed 26703, 2134, 31665, 1028, 2044, 567, 8191 passed; 444 passed — this review observed 26703, 2134, 31665, 1028, 2044, 567, 8191 passed; 921 passed — this review observed 26703, 2134, 31665, 1028, 2044, 567, 8191 passed; and 2 more.
Convergence: round 3 posted 8 inline comment(s), 7 of them reported for the first time; the previous round posted 6 (4 new). Findings keep coming back to the same files: packages/core/src/permissions/rule-parser.ts (findings in rounds 1, 2; 6 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. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：test-efficacy probe — the mutation harness's positive control never produced a verdict (its runner tripped the repo's vitest global-setup prerequisite guard), so the mutant and hunk-necessity classes went unmeasured.
Test Plan（非阻断）：src/code-mode/host.ts — no such file or directory; Tests 444 passed — this review observed 26703, 2134, 31665, 1028, 2044, 567, 8191 passed; 9 passed — this review observed 26703, 2134, 31665, 1028, 2044, 567, 8191 passed; 444 passed — this review observed 26703, 2134, 31665, 1028, 2044, 567, 8191 passed; 921 passed — this review observed 26703, 2134, 31665, 1028, 2044, 567, 8191 passed; and 2 more。
收敛情况：第 3 轮发布了 8 条行内评论，其中 7 条是首次提出；上一轮发布了 6 条（其中 4 条首次提出）。发现反复回到同一批文件：packages/core/src/permissions/rule-parser.ts（第 1、2 轮已出过发现，本轮又有 6 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
| // bash is still inside it, and the word-initial `#` passed the | ||
| // `paramDepth === 0` gate and swallowed the real `;`, folding the tail into | ||
| // one allow-covered segment (#11815). | ||
| if (ch === ')' && arithmeticDepth === 0 && commandSubDepth > 0) { |
There was a problem hiding this comment.
[Critical] R3-1: [certifies-falsely] [regression] The comment state models bash nesting with four flat counters instead of a kind-aware opener stack, so a closer is charged to whatever counter happens to be non-zero rather than to the construct that opened it. A ) closing a subshell, a process substitution, a function definition, an extglob or a case pattern inside a $( … ) body zeroes commandSubDepth while bash is still inside the substitution; a literal } later in that body then satisfies the commandSubDepth === 0 && backtickDepth === 0 guard and closes paramDepth early, so a following word-initial # — literal to bash inside ${ … } — passes the paramDepth === 0 gate and its skip-to-newline folds a real ; or & away. The tail command never receives its own rule check, so a configured hard deny becomes allow on a command bash demonstrably executes.
With permissionsAllow ['Bash(echo *)'] and permissionsDeny ['Bash(rm *)'], the command echo ${x:-$(printf %s <(echo a) }) #c} ; rm -rf /tmp/x returns one folded segment and evaluates to allow at this head, while the merge base returns two segments and denies, and bash runs the tail. Five further constructs reproduce it: >( … ) inside $( … ), a bare subshell $( (echo A); echo } ), <( directly inside ${ … }, >( directly inside ${ … }, and a function definition inside $( … ); a case-pattern ) degrades deny to ask. The same root also fails in the opposite direction: a $( … ) nested inside ${ … } (or inside $(( … ))) can never decrement paramDepth, so the depth strands above zero for the rest of the input including later physical lines, and ls $(echo ${DIR}) # list ; rm -rf /tmp/x returns a hard deny on an rm bash never runs — the false prompt #11815 exists to remove, still shipping.
Witness:
Verifier sweep of 20 bash constructs driving the same string through both arms' real built code and through `bash -xc` with a marker file: 7 of 20 downgrade at default bash options (5 deny->allow, 2 deny->ask) with bash executing the tail, 8 of 20 counting the extglob-enabled arm. Arm proof: `backtickDepth` occurs 4x in head dist and 0x in base; `paramDepth`/`isCommentStart`/`COMMENT_WORD_BOUNDARIES` are 0 in base. Representative rows — PR `echo ${x:-$(printf %s <(echo a) }) #c} ; rm -rf /tmp/x` -> segments=1 verdict=allow; BASE same -> segments=2 verdict=deny; bash oracle `+ touch .../marker_A1` -> marker CREATED. Control that must stay correct and does: `echo ${x:-$(echo $((1)) }) #c} ; rm -rf /tmp/x` -> segments=2 verdict=deny on both arms. The sweep surfaced two entrances no finder had reported (a function definition `f() { …; }` inside `$( )`, and `>(` directly inside `${ … }`), and head rule-parser.ts contains zero occurrences of `<(`.
Close the class rather than adding the next corner. Replace the four flat counters with one stack of open bodies tagged by kind — push on ${, $(, an unescaped backtick, (( and a bare (; pop only when the closer matches the top — so a closer is charged to the construct that opened it, and gate the # branch on the kind of the innermost open body rather than on a flat paramDepth === 0. If a hand-rolled scanner must stay for now, make the fold conditional on the absence of unattributable structure: refuse the comment fold (keeping today's over-split, which is fail-closed) whenever a closer on the line could not be attributed to the construct that opened it. Where a construct genuinely cannot be distinguished without real parsing — a case-pattern ) is one — fail closed and say so in the comment.
Fix constraint: rule-parser.ts:1091 — if (arithmeticDepth > 0 && ch === ')' && command[i + 1] === ')') { consumes arithmetic's )) as a pair, and permission-manager.test.ts:980 (does not let an arithmetic ) close the enclosing substitution) pins that the first ) of $((1)) is not charged to the enclosing $( … ); a stack must preserve that. permission-manager.test.ts:911-915 ('a command substitution', 'echo ${x:-$(echo }) #c} ; rm -rf /tmp/x') requires a } inside a substitution body to stay ordinary text, and the 'no substitution at all' control row echo ${x:-{a,b} # c} ; echo SECOND must keep folding — so a stack must not push on a bare {. The kind-aware gate must also keep 'a nested command substitution' denying, since at that #c the $( … ) has already closed and the innermost opener is param.
A row in the existing comment-like # in %s: deny still fires table in packages/core/src/permissions/permission-manager.test.ts asserting deny for echo ${x:-$(printf %s <(echo a) }) #c} ; rm -rf /tmp/x, plus a splitCompoundCommandSegments row asserting [{ command: 'echo ${x:-$(printf %s <(echo a) }) #c}', terminator: ';' }, { command: 'rm -rf /tmp/x', terminator: '' }]. Both are red at this head (measured: one folded segment, allow). If the fix is a fail-closed refusal rather than correct parsing, pin ask-or-deny and never allow, or the guard can regress silently into a fold.
中文说明
注释状态用四个扁平计数器建模 bash 的嵌套，而不是一个能区分类型的开启符栈，所以闭合符会被记到当时恰好非零的那个计数器上，而不是记到真正开启它的构造上。在 $( … ) 体内，一个用于闭合子 shell、进程替换、函数定义、extglob 或 case 模式的 ) 会把 commandSubDepth 归零，而 bash 此时仍在替换内部；随后体内一个字面量 } 就满足了 commandSubDepth === 0 && backtickDepth === 0 这组守卫并提前关闭 paramDepth，于是后面一个位于词首的 #（在 ${ … } 内对 bash 只是字面量）通过了 paramDepth === 0 门，其跳扫到行尾的逻辑把真实的 ; 或 & 吞掉。尾部命令因此拿不到自己的规则检查，一条已配置的硬 deny 在 bash 确实会执行的命令上变成了 allow。
在 allow ['Bash(echo *)']、deny ['Bash(rm *)'] 下，echo ${x:-$(printf %s <(echo a) }) #c} ; rm -rf /tmp/x 在当前 head 返回一个折叠片段并判定为 allow，合并基线返回两个片段并判定为 deny，且 bash 会执行尾部命令。另有五种构造可复现：$( … ) 内的 >( … )、裸子 shell $( (echo A); echo } )、直接位于 ${ … } 内的 <(、直接位于 ${ … } 内的 >(、以及 $( … ) 内的函数定义；case 模式的 ) 会把 deny 降级为 ask。同一根因也朝反方向失效：嵌套在 ${ … }（或 $(( … ))）内的 $( … ) 永远无法递减 paramDepth，深度于是在整串输入（含后续物理行）上卡在 1 以上，ls $(echo ${DIR}) # list ; rm -rf /tmp/x 会对 bash 根本不执行的 rm 返回硬 deny——#11815 要消除的误报仍在发生。
请关掉整个类别，而不是再补下一个角落：把四个扁平计数器换成按类型标记的开启符栈——在 ${、$(、未转义反引号、(( 和裸 ( 处入栈，仅当闭合符与栈顶匹配时出栈——让闭合符记到真正开启它的构造上；同时让 # 分支依据最内层开启体的类型判断，而不是依据扁平的 paramDepth === 0。若暂时必须保留手写扫描器，就让折叠以「不存在无法归属的结构」为前提：当本行出现一个无法归属到其开启构造的闭合符时拒绝折叠（保留今天的过度切分，那是失效即收紧的方向）。对确实无法在没有真正解析时区分的构造——case 模式的 ) 即一例——请失效即收紧并在注释中写明。
约束：rule-parser.ts:1091 的 if (arithmeticDepth > 0 && ch === ')' && command[i + 1] === ')') { 把算术的 )) 作为一对消费，permission-manager.test.ts:980（does not let an arithmetic ) close the enclosing substitution）钉住了 $((1)) 的第一个 ) 不能记到外层 $( … ) 上，栈必须保留这一点；permission-manager.test.ts:911-915 的 'a command substitution' 行要求替换体内的 } 仍是普通文本，而 'no substitution at all' 对照行 echo ${x:-{a,b} # c} ; echo SECOND 必须继续折叠，所以栈不能在裸 { 上入栈；按类型判断的门还必须让 'a nested command substitution' 继续 deny，因为在那个 #c 处 $( … ) 已闭合、最内层开启体是 param。
请在现有 comment-like # in %s: deny still fires 表中补一行断言 echo ${x:-$(printf %s <(echo a) }) #c} ; rm -rf /tmp/x 为 deny，并补一条 splitCompoundCommandSegments 断言它切成 [{ command: 'echo ${x:-$(printf %s <(echo a) }) #c}', terminator: ';' }, { command: 'rm -rf /tmp/x', terminator: '' }]——两条在当前 head 都是红的（实测：一个折叠片段、allow）。移除修复后两条必须再次变红；若采用失效即收紧的拒绝方案，请钉住 ask 或 deny、绝不钉住 allow，否则这层守卫会悄悄退化回折叠。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| // deny that fired before this change was a false positive on text that is | ||
| // never executed. Making these paths heredoc-aware is #9417's job, not | ||
| // this change's. | ||
| it('heredoc body comment line: no deny on text bash never runs', async () => { |
There was a problem hiding this comment.
[Critical] R3-2: [certifies-falsely] [regression] This test pins a verdict relaxation whose stated premise does not hold in general. The four Bash-rule paths split the raw command and only walkCompoundCommand strips heredoc bodies first, so a # at the start of a heredoc body line is now read as a comment and that line folds. The description justifies the resulting deny-to-ask move as safe because the folded text is something bash hands to cat as data and never executes — which is true for a shell consumer, but the same unstripped path covers every consumer, and a non-shell consumer turns the body into argv and really runs it.
With permissionsDeny ['Bash(rm *)'], the command xargs rm <<EOF / # hi ; rm -rf /tmp/x / EOF returned four segments and deny at the merge base, and returns three segments and ask at this head, because the folded body segment cannot match the anchored ^rm( .*)?$. xargs performs no comment recognition: it splits the body on whitespace and invokes rm with /tmp/x among its arguments, so the deletion happens while the user's hard deny no longer applies. Adding permissionsAllow ['Bash(xargs *)'] covers the head segment, so the folded body segment is the only thing left to catch it.
Witness:
Both arms on the real built code: BASE split `["xargs rm <<EOF","# hi","rm -rf /tmp/x","EOF"]` -> deny; PR split `["xargs rm <<EOF","# hi ; rm -rf /tmp/x","EOF"]` -> ask (still ask, never allow, with an added `Bash(xargs *)` allow rule). The body really executes — `xargs printf '[%s]\n'` over the same heredoc emitted `[#] [hi] [;] [rm] [-rf] [/tmp/x]`, and a sandboxed `xargs rm` over that body deleted the victim directory (exit 0, no diagnostics, because the body's own `-rf` reaches GNU rm which then ignores the junk arguments). The description's own premise was reproduced and holds for `cat`: `bash -xc 'cat <<EOF\n# hi ; rm -rf /\nEOF'` traces only `+ cat`.
Either make the four Bash-rule paths heredoc-aware before splitting — stripHeredocBodies(command), reusing the entry point walkCompoundCommand already uses at shell-semantics.ts:2266 rather than adding a second heredoc scanner — or, if heredoc handling really belongs to #9417, suppress the comment state while scanning inside a heredoc body so the pre-change fail-closed over-splitting is retained there. Note that stripping alone does not restore the base verdict on this witness (see the fix constraint); it needs either argument-level awareness for a non-shell consumer or an explicit recorded decision that ask is acceptable for this shape.
Fix constraint: The row this finding is anchored on asserts toBe('ask') for cat <<EOF\n# hi ; rm -rf /\nEOF; measured, a stripping fix moves exactly this one row in the package — permission-manager.test.ts:2835, expected 'allow' to be 'ask' — because stripped to cat <<EOF\nEOF the command resolves as read-only. So the fix must re-derive that expectation deliberately rather than discover it as a failure. shell-semantics.ts:2266 (const subCommands = splitCompoundCommandSegments(stripHeredocBodies(command));) is the existing heredoc-aware entry point to reuse.
A PermissionManager case pinning deny for xargs rm <<EOF\n# hi ; rm -rf /tmp/x\nEOF under permissionsDeny ['Bash(rm *)']. It is red at this head (measured: ask). Removing the heredoc handling must turn it red again.
中文说明
这条测试钉住了一次判定放宽，而它所述的放宽理由并不普遍成立。四条 Bash 规则路径切分的是原始命令，只有 walkCompoundCommand 会先剥离 heredoc 体，因此 heredoc 体行首的 # 现在会被当作注释、该行随之折叠。PR 描述把由此产生的 deny 变 ask 解释为安全，理由是被折叠的文本是 bash 交给 cat 的数据、从不执行——这对 shell 消费方成立，但同一条未剥离的路径覆盖所有消费方，而非 shell 的消费方会把体内容变成 argv 并真的执行它。
在 deny ['Bash(rm *)'] 下，命令 xargs rm <<EOF / # hi ; rm -rf /tmp/x / EOF 在合并基线返回四个片段并判定 deny，在当前 head 返回三个片段并判定 ask，因为折叠后的体片段无法匹配带锚点的 ^rm( .*)?$。xargs 不做任何注释识别：它按空白切分体内容并以 /tmp/x 作为参数之一调用 rm，于是删除真的发生，而用户的硬 deny 已不再适用。再加上 allow ['Bash(xargs *)'] 会覆盖 head 的首片段，折叠后的体片段就成了唯一还能拦住它的东西。
要么让这四条 Bash 规则路径在切分前感知 heredoc——在那里使用 stripHeredocBodies(command)，复用 walkCompoundCommand 在 shell-semantics.ts:2266 已经在用的入口，而不是再加一个 heredoc 扫描器；要么，如果 heredoc 处理确实属于 #9417，就在扫描 heredoc 体内部时抑制注释状态，从而保留改动前那种失效即收紧的过度切分。请注意单纯剥离并不能在这条证据上恢复基线判定（见下方约束）：它还需要对非 shell 消费方的参数级感知，或者一个明确记录下来的决定，即这种形态下 ask 是可接受的。
约束：本条所锚定的那一行为 cat <<EOF\n# hi ; rm -rf /\nEOF 断言 toBe('ask')；实测一个剥离式修复在整个包里只移动这一行——permission-manager.test.ts:2835，expected 'allow' to be 'ask'——因为剥离后剩下 cat <<EOF\nEOF，该命令会被判为只读。所以修复必须有意识地重新推导这个期望值，而不是把它当作失败发现。shell-semantics.ts:2266（const subCommands = splitCompoundCommandSegments(stripHeredocBodies(command));）是现成的 heredoc 感知入口，应当复用。
请补一个 PermissionManager 用例，在 deny ['Bash(rm *)'] 下断言 xargs rm <<EOF\n# hi ; rm -rf /tmp/x\nEOF 为 deny；它在当前 head 是红的（实测：ask）。移除该 heredoc 处理后它必须再次变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| // An unescaped backtick outside quotes opens or closes a body; quotes and | ||
| // backslashes were handled above, so this only sees a live delimiter. | ||
| if (ch === '`') { | ||
| backtickDepth = backtickDepth === 0 ? 1 : 0; |
There was a problem hiding this comment.
[Critical] R1-4: (fix-induced) [certifies-falsely] [regression] The backtickDepth counter added to close R1-4 is toggled by any unescaped backtick, including one in a heredoc body that bash never parses — the four Bash-rule paths split the raw command and only walkCompoundCommand strips heredoc bodies. A stray backtick in the body leaves the depth at 1, so at a genuine comment on a later physical line the branch takes stopAtBacktick = true, stops at a backtick inside that comment instead of at the newline, and hands the comment's tail back to the quote and escape state machine. The re-scanned apostrophe opens a quote that never closes, masking the newline boundary and folding a following real command into the previous segment, taking its rule check with it.
With permissionsAllow ['Bash(*)'] and permissionsDeny ['Bash(rm *)'], the command cat <<'EOF' / a lone backtick / EOF / echo a # x' + backtick + y' / rm -rf /tmp/x returns four segments at this head with the rm folded into the comment segment, and evaluates to allow with no prompt. The merge base returns five segments with the rm separate and denies. The last physical line really executes, so a configured hard deny is bypassed on a command bash runs. The identical command with no backtick in the heredoc body denies at this head, which isolates the corrupted counter as the cause.
Witness:
Same input through both arms' real built code: BASE split `["cat <<'EOF'","`","EOF","echo a # x'` y'","rm -rf /tmp/x"]` -> {"decision":"deny","denyRule":"Bash(rm *)"}; PR split folds the last line into the comment segment -> {"decision":"allow","denyRule":"Bash(rm *)"}. Control with no backtick in the body -> deny at head. bash oracle with a quoted heredoc delimiter (so the body is pure data) traced `+ cat`, `+ echo a`, `+ touch .../RAN` and the marker was created. Arm proof: `backtickDepth` occurs 4x in head dist, 0x in base. Load-bearing check: reverting only this hunk flips the witness to five segments and deny but breaks exactly four pinned rows — including `comment-like # in a backtick substitution: deny still fires`, which goes deny->allow — so a plain revert is not the fix.
Stop feeding un-parsed heredoc bodies to the splitter on the Bash-rule paths: split stripHeredocBodies(command) there, as walkCompoundCommand already does, so no text bash treats as data can drive backtickDepth, inSingle, inDouble or escaped. Measured, this closes the witness outright (allow -> deny, base restored) and moves exactly one pinned row. Do not simply revert the backtick toggle — that reopens the four rows R1-4's fix pinned.
Fix constraint: shell-semantics.ts:2266 — const subCommands = splitCompoundCommandSegments(stripHeredocBodies(command)); is the existing heredoc-aware entry point; reuse it rather than adding a second heredoc scanner. The four rows that R1-4's fix pinned must stay green: does not let a backtick substitution inside the expansion close it early, does not let a comment swallow a closing backtick, does not stop a comment at an escaped backtick, and comment-like # in a backtick substitution: deny still fires.
A pm.evaluate row asserting deny for the five-line command in the failure scenario above (cat <<'EOF', a lone backtick, EOF, echo a # x' + backtick + y', rm -rf /tmp/x) under permissionsAllow ['Bash(*)'] and permissionsDeny ['Bash(rm *)'], plus a splitCompoundCommand row asserting rm -rf /tmp/x stays its own segment. Both are red at this head (measured: allow, four segments). Reverting to raw splitting must turn both red again.
中文说明
（由修复引入）为关闭 R1-4 而加入的 backtickDepth 计数器会被任意未转义反引号翻转，包括位于 bash 从不解析的 heredoc 体中的那一个——四条 Bash 规则路径切分的是原始命令，只有 walkCompoundCommand 会剥离 heredoc 体。体中一个游离的反引号让该深度停在 1，于是在后续某个物理行的真实注释处，分支取到 stopAtBacktick = true，停在该注释内部的一个反引号处而不是换行处，并把注释的剩余部分交回引号与转义状态机。被重新扫描的单引号开启了一个永不闭合的引号，掩盖了换行边界，把后面一条真实命令折叠进前一个片段，连带取走了它的规则检查。
在 allow ['Bash(*)']、deny ['Bash(rm *)'] 下，命令 cat <<'EOF' / 一个孤立反引号 / EOF / echo a # x' + 反引号 + y' / rm -rf /tmp/x 在当前 head 返回四个片段、rm 被折进注释片段，判定为 allow 且不提示；合并基线返回五个片段、rm 独立、判定为 deny。最后那个物理行确实会执行，所以一条已配置的硬 deny 在 bash 会运行的命令上被绕过。把 heredoc 体中的反引号去掉、其余完全相同的命令在当前 head 判定为 deny，这正好把被污染的计数器隔离为成因。
请停止把未解析的 heredoc 体喂给 Bash 规则路径上的切分器：在那里切分 stripHeredocBodies(command)，就像 walkCompoundCommand 已经做的那样，使 bash 视为数据的文本无法驱动 backtickDepth、inSingle、inDouble 或 escaped。实测这能直接关闭该证据（allow 变回 deny，与基线一致），且只移动一行已钉住的断言。请不要简单回退这个反引号翻转——那会重新打开 R1-4 的修复所钉住的四行。
约束：shell-semantics.ts:2266 的 const subCommands = splitCompoundCommandSegments(stripHeredocBodies(command)); 是现成的 heredoc 感知入口，应复用而非新增第二个 heredoc 扫描器。R1-4 修复所钉住的四行必须保持绿色：does not let a backtick substitution inside the expansion close it early、does not let a comment swallow a closing backtick、does not stop a comment at an escaped backtick、以及 comment-like # in a backtick substitution: deny still fires。
请补一条 pm.evaluate 断言：在 allow ['Bash(*)']、deny ['Bash(rm *)'] 下，上述命令为 deny；再补一条 splitCompoundCommand 断言 rm -rf /tmp/x 仍是独立片段。两条在当前 head 都是红的（实测：allow、四个片段）。改回原始切分后两条必须再次变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| } | ||
| const previous = command[index - 1]!; | ||
| return ( | ||
| COMMENT_WORD_BOUNDARIES.includes(previous) && |
There was a problem hiding this comment.
[Suggestion] R3-3: isCommentStart's escape conjunct treats the backslash of a \<newline> line continuation as escaping the newline word boundary, but bash deletes the whole backslash-newline pair. So the word-start test must be made against the character bash leaves adjacent to the #, not against the newline the continuation consumed. The new escape-aware boundary test gets this one shape backwards.
For the command text echo hi + backslash + newline + # c ; rm -rf /tmp/x, with permissionsDeny ['Bash(rm *)'], this head returns two segments and a hard deny, while bash joins the continuation into echo hi # c ; rm -rf /tmp/x, treats the # as a word-initial comment, and runs only the echo. So a deny fires on an rm bash never executes — and unlike ask, a deny cannot be approved past. The tab-before-backslash spelling behaves the same. The mechanism: previous is the newline, which is in the boundary list, but precedingBackslashCount(command, index - 1) is 1, so the parity conjunct returns false and the # reads as literal.
Witness:
Both arms measured: HEAD split `["echo hi \\\n# c","rm -rf /tmp/x"]` verdict=deny (tab variant identical); BASE identical split and deny, so this is an unfixed shape rather than a regression. A byte-exact `bash -x` oracle with a shimmed rm made 0 calls and exited 0. The paired control `echo a` + backslash + newline + `#c ; rm -rf /tmp/x` (no space before the backslash) still splits at head and bash really runs the tail there (1 shim call), so the finding does not over-claim. A fix candidate confined to `isCommentStart` flipped the witness deny->allow matching the oracle, left the control at deny, and moved nothing: 487/487 in permission-manager.test.ts and 964/964 across src/permissions.
In isCommentStart, when the boundary character is a newline and its backslash run is odd, bash has removed both characters — re-run the boundary test against command[index - 1 - run] with its own parity, repeating while that character is itself an escaped newline, instead of returning false. Keep returning false for every other odd-parity boundary character (a\ #b, a\;#b), which existing rows pin. The walk-back belongs in isCommentStart, not in precedingBackslashCount.
Fix constraint: rule-parser.ts:859 — return precedingBackslashCount(command, j) % 2 === 1; in isAsyncOperator relies on the helper's literal "consecutive backslashes immediately before index" meaning for backslash-redirection detection, so the continuation walk-back must not go into the shared helper. Two rows this diff adds must stay green: does not let a trailing backslash extend a comment (permission-manager.test.ts:859-860, the command echo hi # foo + backslash + newline + rm -rf /tmp/x -> two segments; that backslash is inside the comment and is consumed by the skip, not by isCommentStart) and does not read a comment after an escaped space (:785-786, echo a + backslash + space + #b ; rm -rf /tmp/x -> two segments). Note also that matchesCommandPattern builds new RegExp(regex, 's') (rule-parser.ts:1240), so . crosses a newline and a folded multi-line segment can still match.
A row in the does not split %s, where the operator is inside a comment table for the command echo hi + backslash + newline + # c ; rm -rf /tmp/x expecting one segment, plus the paired control row echo a + backslash + newline + #c ; rm -rf /tmp/x (no space before the backslash) expecting two. The first is red at this head (measured: two segments); dropping the continuation walk-back reddens it, and dropping the control reddens the second.
中文说明
isCommentStart 的转义条件把 \<换行> 续行中的反斜杠当成转义了换行这个词边界，但 bash 会把整个「反斜杠+换行」删掉。因此词首判断必须针对 bash 留在 # 旁边的那个字符，而不是针对被续行消耗掉的换行。这个新的转义感知边界判断在这一种形态上判反了。
对命令文本 echo hi + 反斜杠 + 换行 + # c ; rm -rf /tmp/x，在 deny ['Bash(rm *)'] 下当前 head 返回两个片段并给出硬 deny，而 bash 会把续行拼成 echo hi # c ; rm -rf /tmp/x、把 # 当作词首注释、只执行那个 echo。于是一条 deny 落在 bash 从不执行的 rm 上——而与 ask 不同，deny 是无法被批准放行的。反斜杠前是制表符时行为相同。机制是：previous 是换行、确实在边界表内，但 precedingBackslashCount(command, index - 1) 为 1，奇偶条件返回 false，# 被当作字面量。
请在 isCommentStart 中处理：当边界字符是换行且其反斜杠串长度为奇数时，bash 已把两个字符都删除——改为对 command[index - 1 - run]（bash 留在 # 旁边的那个字符）重跑边界判断并带它自己的奇偶性，若该字符本身又是被转义的换行则继续重复，而不是直接返回 false。对其它所有奇数奇偶性的边界字符（a\ #b、a\;#b）继续返回 false，这些已有断言钉住。这个回退必须写在 isCommentStart 里，不能写进 precedingBackslashCount。
约束：rule-parser.ts:859 的 return precedingBackslashCount(command, j) % 2 === 1;（在 isAsyncOperator 中）依赖该辅助函数「紧邻 index 之前的连续反斜杠」这一字面语义来识别反斜杠重定向，所以续行回退不能放进这个共享辅助函数。本 diff 新增的两行必须保持绿色：does not let a trailing backslash extend a comment（permission-manager.test.ts:859-860，命令 echo hi # foo + 反斜杠 + 换行 + rm -rf /tmp/x 切成两段；那个反斜杠位于注释内部，由跳扫消费而非由 isCommentStart 消费）与 does not read a comment after an escaped space（:785-786，echo a + 反斜杠 + 空格 + #b ; rm -rf /tmp/x 切成两段）。另请注意 matchesCommandPattern 构造的是 new RegExp(regex, 's')（rule-parser.ts:1240），因此 . 会跨换行，折叠后的多行片段仍可能匹配。
请在 does not split %s, where the operator is inside a comment 表中补一行，命令为 echo hi + 反斜杠 + 换行 + # c ; rm -rf /tmp/x，期望一个片段；再补配套对照行 echo a + 反斜杠 + 换行 + #c ; rm -rf /tmp/x，期望两个片段。第一条在当前 head 是红的（实测：两个片段）；去掉续行回退会让第一条变红，去掉对照组会让第二条变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| // Reading that `#` as a comment swallowed the `;` and folded both commands | ||
| // into one allow-covered segment. | ||
| let paramDepth = 0; | ||
| // Nesting depth of the substitutions whose body is scanned without honouring |
There was a problem hiding this comment.
[Suggestion] R3-4: This comment describes commandSubDepth as the nesting depth of the substitutions whose body is scanned without honouring a } — $( … ) and backtick substitutions — but its only increment site counts $( … ) alone. Backtick bodies are tracked by the separately declared backtickDepth. The code is correct because the } guard consults both counters; only the comment is wrong.
A maintainer trusting the comment concludes that commandSubDepth > 0 already means "inside any substitution, backtick included" and drops the backtickDepth === 0 conjunct from the } guard as redundant. A } inside a backtick body — the command echo ${x:- + backtick + echo } + backtick + #c} ; rm -rf /tmp/x, the shape the pinned row does not let a backtick substitution inside the expansion close it early exists for — would then close the enclosing ${ … } while bash is still inside it, the following # would pass the paramDepth === 0 gate and swallow the ;, folding the tail into one allow-covered segment: exactly the fail-open the two-conjunct guard exists to prevent.
Witness:
Mutation run in an isolated copy: dropping `backtickDepth === 0` from the `}` guard at rule-parser.ts:1029-1035 gives `Test Files 1 failed | 14 passed (15) / Tests 1 failed | 1781 passed (1782)` with `FAIL src/permissions/permission-manager.test.ts > splitCompoundCommand > does not let a backtick substitution inside the expansion close it early`. So the conjunct is pinned and the comment is the only defective artifact — the stronger alternative (an unpinned conjunct) was tested for and does not apply. Source confirms the increment site is rule-parser.ts:1006-1008 (`if (ch === '$' && command[i + 1] === '(' && command[i + 2] !== '(') { commandSubDepth++; }`) and `backtickDepth` is declared at :970-973 and toggled at :1026.
Reword so commandSubDepth is described as counting only $( … ), and note that backtick bodies are tracked separately by backtickDepth — both suppress the } closer, which is why the guard tests both.
Fix constraint: Keep the adjacent bash oracle in the comment (bash -xc 'x=""; echo ${x:-$(echo }) #c} ; touch m ; echo SECOND'), which is what makes the guard's purpose checkable rather than asserted.
中文说明
这段注释把 commandSubDepth 描述为「体内扫描时不承认 } 的那些替换的嵌套深度——$( … ) 与反引号替换」，但它唯一的自增点只统计 $( … )；反引号体由单独声明的 backtickDepth 跟踪。代码是正确的，因为 } 守卫会同时查询这两个计数器；错的只有注释。
一位相信该注释的维护者会得出「commandSubDepth > 0 已经意味着处于任何替换内部（含反引号）」的结论，从而把 } 守卫中的 backtickDepth === 0 当作冗余删掉。那时反引号体内的一个 }——echo ${x:- + 反引号 + echo } + 反引号 + #c} ; rm -rf /tmp/x——就会在 bash 仍处于其中时关闭外层 ${ … }，随后的 # 通过 paramDepth === 0 门并吞掉 ;，把尾部折进一个被 allow 覆盖的片段：这正是两个条件共同守卫所要防止的失效即放开。
请把措辞改为：commandSubDepth 只统计 $( … )，并说明反引号体由 backtickDepth 单独跟踪——两者都会抑制 } 这个闭合符，所以守卫要同时检查两者。
约束：请保留注释中紧邻的那条 bash oracle（bash -xc 'x=""; echo ${x:-$(echo }) #c} ; touch m ; echo SECOND'），正是它让这个守卫的意图可被检验，而不是仅凭断言。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| // `paramDepth === 0` gate and swallowed the real `;`, folding the tail into | ||
| // one allow-covered segment (#11815). | ||
| if (ch === ')' && arithmeticDepth === 0 && commandSubDepth > 0) { | ||
| commandSubDepth--; |
There was a problem hiding this comment.
[Suggestion] R3-5: The commandSubDepth > 0 clamp on this decrement has no test, although the diff gives the exactly analogous paramDepth > 0 clamp on the } guard below both a dedicated row and a four-line comment. Relaxing it is a mutation the suite cannot see.
Removing the clamp leaves 1782 of 1782 tests green across the fifteen-file population that can observe the splitter, yet changes behaviour: a stray ) that closes neither a $( … ) nor an arithmetic pair drives commandSubDepth to -1, so a following } fails the commandSubDepth === 0 conjunct, paramDepth stays at 1, a genuine comment is read as literal, and the ; after it becomes a boundary. A configured Bash(rm *) deny then fires on text bash never executes — the false positive this diff's own heredoc row was written to remove — with nothing in the suite noticing.
Witness:
Mutation run in an isolated copy: rule-parser.ts:1020 changed from `if (ch === ')' && arithmeticDepth === 0 && commandSubDepth > 0) {` to `if (ch === ')' && arithmeticDepth === 0) {` -> `Test Files 15 passed (15) / Tests 1782 passed (1782), exit=0`, no row red. Behaviour flip on the same probe, PR as committed vs mutated: `( ls ) ; echo ${x} # c ; rm -rf /tmp/x` goes from `["( ls )","echo ${x} # c ; rm -rf /tmp/x"]` to `["( ls )","echo ${x} # c","rm -rf /tmp/x"]`, and a `case $a in b)` pattern likewise goes from 1 to 3 segments. Correction that strengthens the finding: the originally reported trigger `echo ) ${x} # c ; …` is not valid bash (a bare top-level `)` is a syntax error), so the two valid-bash shapes above are the ones that carry the cost — `bash -x '( echo LS ) ; echo ${x} # c ; echo DANGER_SUBSHELL'` traces `+ echo LS` and `+ echo` and never prints DANGER_SUBSHELL.
Add a row beside the existing stray-} case in keeps the # semantics of %s around a parameter expansion, using a valid-bash stray closer — for example ['( ls ) ; echo ${x} # c ; rm -rf /tmp/x', ['( ls ) ; echo ${x} # c ; rm -rf /tmp/x']].
Fix constraint: rule-parser.ts:1030-1033 — the } guard reads the same counter (ch === '}' && paramDepth > 0 && commandSubDepth === 0 && backtickDepth === 0), so a new row must keep the stray ) outside any real $( … ); placed inside one, commandSubDepth would legitimately be 0 by the time the } is reached and the row would no longer discriminate the clamp.
That row goes red if the commandSubDepth > 0 clamp at rule-parser.ts:1020 is removed — measured, nothing in the file does today.
中文说明
这个自减上的 commandSubDepth > 0 限幅没有任何测试，而本 diff 却给下方 } 守卫上完全类似的 paramDepth > 0 限幅同时写了一条专门断言和一段四行注释。放宽它是一次测试套件看不见的变异。
去掉该限幅后，在能观察到切分器的 15 个文件、1782 条测试中仍然全绿，但行为已经改变：一个既不闭合 $( … ) 也不闭合算术对的游离 ) 会把 commandSubDepth 压到 -1，于是随后的 } 不满足 commandSubDepth === 0 条件，paramDepth 停在 1，一个真实注释被当作字面量，其后的 ; 变成边界。结果是一条已配置的 Bash(rm *) deny 落在 bash 从不执行的文本上——正是本 diff 自己那条 heredoc 断言要消除的误报——而套件里没有任何东西会发现。
请在 keeps the # semantics of %s around a parameter expansion 中、紧挨现有的游离 } 用例补一行，使用一个合法 bash 的游离闭合符，例如 ['( ls ) ; echo ${x} # c ; rm -rf /tmp/x', ['( ls ) ; echo ${x} # c ; rm -rf /tmp/x']]。
约束：rule-parser.ts:1030-1033 的 } 守卫读取同一个计数器（ch === '}' && paramDepth > 0 && commandSubDepth === 0 && backtickDepth === 0），所以新增行的游离 ) 必须位于任何真实 $( … ) 之外；若放进替换内部，到 } 时 commandSubDepth 本就已合法归零，该行将无法区分这个限幅。
去掉 rule-parser.ts:1020 的 commandSubDepth > 0 限幅后，这一行必须变红——实测今天包里没有任何一行会变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| ) { | ||
| i++; | ||
| } | ||
| i--; // -1 because the loop will i++, landing back on the delimiter |
There was a problem hiding this comment.
[Suggestion] R3-6: The comment skip advances i past the comment text but never advances lastSplit, so a comment that is the entire pending text at a boundary is still emitted as a segment of its own. That segment matches no Bash(...) rule and evaluateCompoundCommand takes the most restrictive result across segments, so the whole command drops to ask. The same-line trailing shape is handled, so the gap is specific to a full-line comment — the most common way a comment appears in a multi-line command.
With permissionsAllow ['Bash(npm *)'], the three-line command npm install / # run the tests / npm test returns ask at this head, although bash runs exactly two commands and both are covered by the allow rule. The comment-only segment alone resolves to ask and the aggregation takes the most restrictive. The same shape reproduces for mkdir -p /tmp/x ; # ensure dir followed by cp a /tmp/x, and for git status, a blank line, # check, git diff. This is the false prompt #11815 was filed to remove, surviving on the most common comment placement; the description's Risk & Scope discloses only the same-line residue (echo a;#c leaving a trailing #c segment).
Witness:
Both arms measured: HEAD split `["npm install","# run the tests","npm test"]` verdict=ask; BASE identical split and verdict=ask (so this is an incomplete fix, not a regression). A byte-exact `bash -x` oracle with a shimmed npm made 2 calls and exited 0 — the comment line is inert. Segment attribution: `npm install` -> allow, `npm test` -> allow, `# run the tests` -> ask. Removing the comment line gives allow. A fix candidate in a throwaway copy flipped the split to `["npm install","npm test"]` and the verdict to allow, moving exactly the two predicted pinned rows (permission-manager.test.ts:803-804 and :838-839; 2 failed | 485 passed).
Capture the comment start before the skip and, only when nothing but comment text is pending, drop it from the output: const commentStart = i; before the while, then if (command.slice(lastSplit, commentStart).trim() === '') { lastSplit = i; } before the i--. When the skip stopped at a newline the operator scan then emits an empty segment, which is not pushed; when it ran to end of input the final segment is empty and is not pushed. Segments with real text before the comment keep the comment inline, so npm install # install deps and echo 'a' # comment ; echo B are unchanged.
Fix constraint: Two rows this diff adds pin the current output and must be updated in the same change: permission-manager.test.ts:803-804 (expect(splitCompoundCommand('echo a;#c ; rm -rf /tmp/x')).toEqual(['echo a', '#c ; rm -rf /tmp/x'])) and :838-839 (expect(splitCompoundCommand('#!/bin/sh\necho hi')).toEqual(['#!/bin/sh', 'echo hi'])). Measured, the fix candidate also changes the heredoc pin's split from ['cat <<EOF','# hi ; rm -rf /','EOF'] to ['cat <<EOF','EOF'] while its asserted verdict (ask) still holds.
A splitCompoundCommand row asserting npm install\n# run the tests\nnpm test -> ['npm install', 'npm test'], plus a PermissionManager case with permissionsAllow ['Bash(npm *)'] asserting allow for that command. Both are red at this head (measured: three segments, ask).
中文说明
注释跳扫会把 i 推进到注释文本之后，却从不推进 lastSplit，因此当注释是边界处全部待输出文本时，它仍会作为独立片段被输出。该片段匹配不到任何 Bash(...) 规则，而 evaluateCompoundCommand 取各片段中最严格的结果，于是整条命令降为 ask。同一行的行尾形态是处理好的，所以这个缺口专属于整行注释——而整行注释正是多行命令中最常见的注释写法。
在 allow ['Bash(npm *)'] 下，三行命令 npm install / # run the tests / npm test 在当前 head 返回 ask，尽管 bash 只执行两条命令、且两条都被该 allow 规则覆盖。只有注释的那个片段自身解析为 ask，聚合时取最严格结果。同样的形态在 mkdir -p /tmp/x ; # ensure dir 后接 cp a /tmp/x、以及 git status、空行、# check、git diff 上都能复现。这正是 #11815 要消除的误提示，却在最常见的注释位置上残留；PR 描述的「风险与范围」只披露了同一行的残留（echo a;#c 留下一个尾部 #c 片段）。
请在跳扫前记录注释起点，并且只在待输出内容除注释文本外别无他物时把它从输出中去掉：在 while 之前加 const commentStart = i;，然后在 i-- 之前加 if (command.slice(lastSplit, commentStart).trim() === '') { lastSplit = i; }。当跳扫停在换行处时，操作符扫描随后会产出一个空片段（不会被 push）；当跳扫一直走到输入末尾时，最后那个片段为空（同样不会被 push）。注释前有真实文本的片段仍把注释保留在行内，所以 npm install # install deps 与 echo 'a' # comment ; echo B 不受影响。
约束：本 diff 新增的两行钉住了当前输出，必须与修复一起更新——permission-manager.test.ts:803-804（expect(splitCompoundCommand('echo a;#c ; rm -rf /tmp/x')).toEqual(['echo a', '#c ; rm -rf /tmp/x'])）与 :838-839（expect(splitCompoundCommand('#!/bin/sh\necho hi')).toEqual(['#!/bin/sh', 'echo hi'])）。实测该候选修复还会把 heredoc 那条钉子的切分从 ['cat <<EOF','# hi ; rm -rf /','EOF'] 变成 ['cat <<EOF','EOF']，其断言的判定（ask）仍然成立。
请补一条 splitCompoundCommand 断言 npm install\n# run the tests\nnpm test 等于 ['npm install', 'npm test']，再补一个 PermissionManager 用例，在 allow ['Bash(npm *)'] 下断言该命令为 allow；两条在当前 head 都是红的（实测：三个片段、ask）。移除 lastSplit 推进后两条必须变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
8 participants
What this PR does
Teaches the shell splitter about
#comments.splitCompoundCommandSegments(packages/core/src/permissions/rule-parser.ts:861onmain) tracked four states —inSingle,inDouble,escaped,arithmeticDepth— and had no comment state, so#was transparent and everySHELL_OPERATORSentry after it was taken as a real command boundary. This adds a fifth state: a word-initial#outside quotes starts a comment that runs to the end of the physical line, so operators inside it are inert.Two details are load-bearing for keeping the change fail-closed, and both are pinned by tests:
escapedflag. A backslash does not continue a line inside a comment — bash really does run therminecho hi # foo \+ newline +rm -rf /— so reusingescapedto find the newline would fold that second command into the comment's segment and cost it its own rule check.Quote handling, escape handling and the arithmetic depth counter are untouched, and no heredoc state is added.
Why it's needed
Reported in #11815.
bash -xc "echo 'a' # comment ; echo B"traces a single+ echo a, but the splitter returned two segments,echo 'a' # commentandecho B. The direction is fail-closed — an extra segment can only add a rule that must pass — so this is a usability defect, not a security one: a commented-out tail the allow rule does not cover drags the whole line into a confirmation prompt. Withpermissions.allow: ["Bash(echo *)"],echo 'a' # comment ; rm -rf /tmp/xreturnedaskonmaineven though bash runs only theechothe rule already covers; withpermissions.deny: ["Bash(rm *)"]the same line returneddenyfor anrmbash never executes.One shape was already failing in the unsafe direction, and this fixes it:
echo hi # foo \+ newline +rm -rf /tmp/xcame back as a single segment onmain, because the trailing backslash escaped the newline. bash runs both commands, yetBash(rm *)deny returnedask— thermhad lost its own rule check. It now returnsdeny.Kept separate from #11765 as triage asked. #11765 fixes the sibling defect in the same scanner (a backslash inside
'…'read as an escape, swallowing the closing quote); this PR does not touch that logic. #9417 owns heredoc splitting and #10212 owns env prefixes — neither is reimplemented here.Reviewer Test Plan
How to verify
+ echo a, so everything after the#is inert:npx vitest run src/permissions --root packages/core. Onmainthe 19 new cases are 10 failed / 9 passed; with this PR all 19 pass and the whole file is 444 passed (425 before).The 9 that already passed on
mainare deliberate two-way pins, not padding. Rows 2-4 of the report's table (echo 'a\' # …) were right for the wrong reason there: the backslash inside'…'setescaped, ate the closing quote, and the scanner sat in an unterminated string that masked the operator. They keep passing under this PR, and will keep passing once #11765 makes that backslash literal, because the comment then masks the operator for the right reason.git status # don't ; echo Blikewise now stays one segment because the comment swallows the;, not because the apostrophe indon'topens a quote.Evidence (Before & After)
Non-UI change; the evidence is the red run on unmodified
mainwith the new tests applied (source reverted, tests kept):After (same command, source restored):
Regression runs with this PR applied:
npx vitest run src/permissions/permission-manager.test.ts --root packages/coremain, +19 new)npx vitest run src/permissions --root packages/corenpx vitest run src/core/permissionFlow.test.ts src/core/permission-helpers.test.ts src/core/coreToolScheduler.test.ts src/core/plan-mode-shell-policy.test.ts src/core/nonInteractiveToolExecutor.test.ts src/core/tool-invocation-guard.test.ts src/tools/workflow/workflow.test.ts --root packages/corenpx vitest run src/tools/shell.test.ts src/tools/shell.backgroundStatus.test.ts --root packages/corenpx prettier --check+npx eslinton both changed filesnpm run typecheck --workspace=packages/coresrc/code-mode/host.ts(@jitl/quickjs-singlefile-mjs-release-sync/quickjs-emscripten-coreare not installed in this sandbox); 0 in the changed files. The same 10 errors reproduce fromnpm run build --workspace=packages/coreon a pristine worktree ofmain, before any edit.Tested on
Environment (optional)
Unit tests only (
vitestagainstpackages/core), plus GNU bash on Linux for thebash -xctraces quoted above.Risk & Scope
#only opens a comment at a word start (index 0, after whitespace, or after;/&/|), soecho a#bstill splits and$(( 8#17 ))keeps its base prefix.walkCompoundCommandstrips them (shell-semantics.ts:2266); the four Bash-rule paths split the raw command, so a#at the start of a heredoc body line now keeps its operator inside the comment. That drops aBash(rm *)deny which onmainfired on text bash hands tocatas data and never executes —bash -xc $'cat <<EOF\n# hi ; rm -rf /\nEOF'traces only+ cat— so it removes a false positive rather than opening a hole, but it is a behaviour change and it is pinned by a test. Making those paths heredoc-aware belongs to fix(core): keep heredoc bodies out of permission rule splitting #9417 (with Converge the heredoc permission projection: one quote tracker, stop scanning inert bodies, pin the CRLF state-tracking case #10446's single-quote-tracker cleanup), not here. Also out of scope:echo a;#cstill yields a trailing#csegment, which is pre-existing over-splitting in the fail-closed direction; and>/</(are not treated as word starts, soecho hi >#f ; echo bkeeps splitting — bash comments there too, but leaving it literal only over-splits.splitCompoundCommand/splitCompoundCommandSegmentsreturn fewer segments for commands whose operators sat inside a comment, which is the fix.Linked Issues
Fixes #11815
Related, deliberately not touched: #11765 (sibling scanner defect, must land on its own merits), #9417 (heredoc splitting), #10212 (env prefixes), #10446 (converge the heredoc projection onto one quote tracker).
中文说明
这个 PR 做了什么
让 shell 切分器认识
#注释。main上的splitCompoundCommandSegments（packages/core/src/permissions/rule-parser.ts:861）只维护四种状态——inSingle、inDouble、escaped、arithmeticDepth——没有注释状态，因此#是透明的，其后每个SHELL_OPERATORS成员都被当作真实命令边界。本 PR 加入第五种状态：引号外、位于词首的#开启一段延续到物理行尾的注释，其中的操作符一律失效。有两个细节决定这次改动是否仍然「失效即收紧」，均已由测试钉住：
escaped标志。反斜杠不会在注释内续行——echo hi # foo \加换行再加rm -rf /时，bash 确实会执行那个rm——所以复用escaped去寻找换行会把第二条命令折进注释所在的片段，让它失去自己的规则检查。引号处理、转义处理与算术深度计数均未改动，也没有引入 heredoc 状态。
为什么需要
由 #11815 报告。
bash -xc "echo 'a' # comment ; echo B"只追踪到一条+ echo a，但切分器返回两个片段echo 'a' # comment与echo B。方向是失效即收紧——多出的片段只会增加一条必须通过的规则——所以这是易用性缺陷而非安全缺陷：授权规则未覆盖的注释尾部会把整行拖进确认提示。在permissions.allow: ["Bash(echo *)"]下，echo 'a' # comment ; rm -rf /tmp/x在main上返回ask，尽管 bash 只执行规则本已覆盖的那个echo；在permissions.deny: ["Bash(rm *)"]下同一行返回deny，而那个rmbash 根本不会执行。有一种形态原本就已经朝不安全方向失效，本 PR 顺手修正：
echo hi # foo \加换行加rm -rf /tmp/x在main上返回单个片段，因为行尾反斜杠把换行转义掉了。bash 会执行两条命令，但Bash(rm *)deny 返回ask——那个rm失去了自己的规则检查。现在返回deny。按 triage 的要求与 #11765 分开推进。#11765 修的是同一扫描器上的姊妹缺陷（
'…'内的反斜杠被当作转义、吞掉闭合引号）；本 PR 不碰那段逻辑。heredoc 切分归 #9417，env 前缀归 #10212，这里都没有重复实现。评审测试计划
如何验证
+ echo a，说明#之后的内容全部失效（命令与输出见上方英文部分的 console 块）。npx vitest run src/permissions --root packages/core。在main上，19 个新用例是 10 失败 / 9 通过；打上本 PR 后 19 个全过，整个文件 444 通过（此前 425）。那 9 个在
main上就已通过的用例是有意为之的双向钉子，不是凑数。报告表格的第 2-4 行（echo 'a\' # …）在main上是「答案对、理由错」：'…'内的反斜杠置起escaped、吃掉闭合引号，扫描器卡在未闭合字符串里把操作符挡住了。它们在本 PR 下继续通过，并且在 #11765 把该反斜杠改判为字面量之后仍会继续通过，因为那时是注释以正确的理由挡住了操作符。git status # don't ; echo B同理：现在它保持单片段是因为注释吞掉了;，而不是因为don't里的撇号开启了引号。证据（修复前与修复后）
非 UI 改动；证据是在未修改的
main上（源码回退、只保留新测试）跑出的红色结果，以及修复后的绿色结果，完整输出见上方英文部分。修复前：Tests 10 failed | 9 passed | 425 skipped (444)；修复后：Test Files 1 passed (1) / Tests 444 passed (444)。回归运行的确切数字（4 组 vitest 命令 + prettier/eslint + typecheck）见上方英文部分的表格。typecheck 的 10 个错误全部是既有环境噪音，集中在
src/code-mode/host.ts（本沙箱未安装@jitl/quickjs-singlefile-mjs-release-sync与quickjs-emscripten-core），改动文件里 0 个错误；在改动之前对纯净的mainworktree 跑npm run build --workspace=packages/core会复现同样这 10 个错误。测试平台
仅 Linux 已验证（✅），macOS 与 Windows 未测（⚠️ ）。
环境（可选）
只跑单元测试（针对
packages/core的vitest），外加 Linux 上的 GNU bash 用于上文引用的bash -xc追踪。风险与范围
#只有在词首（下标 0、空白之后、或;/&/|之后）才开启注释，所以echo a#b仍然切分，$(( 8#17 ))的进制前缀也保持字面。walkCompoundCommand会剥离它（shell-semantics.ts:2266），四条 Bash 规则路径切分的是原始命令，因此 heredoc 正文行首的#现在会把该行内的操作符留在注释里。这会让一个Bash(rm *)deny 不再触发——该 deny 在main上命中的是 bash 交给cat当数据、根本不会执行的文本（bash -xc $'cat <<EOF\n# hi ; rm -rf /\nEOF'只追踪到+ cat）——所以它消除的是误报而非开出缺口，但确实是行为变化，已用测试钉住。让这些路径具备 heredoc 感知属于 fix(core): keep heredoc bodies out of permission rule splitting #9417（配合 Converge the heredoc permission projection: one quote tracker, stop scanning inert bodies, pin the CRLF state-tracking case #10446 的单一引号跟踪器收敛），不在本 PR。同样在范围之外：echo a;#c仍会留下一个#c尾片段，这是既有的、朝收紧方向的过度切分；>/</(不作为词首处理，因此echo hi >#f ; echo b仍然切分——bash 在那里同样开启注释，但保持字面只会过度切分。splitCompoundCommand/splitCompoundCommandSegments对「操作符位于注释内」的命令返回更少的片段，这正是本次修复的目的。关联 issue
Fixes #11815
相关但刻意未改动：#11765（姊妹扫描器缺陷，应按自身价值独立合入）、#9417（heredoc 切分）、#10212（env 前缀）、#10446（把 heredoc 投影收敛到单一引号跟踪器）。