Skip to content

fix(core): keep heredoc bodies out of permission rule splitting - #9417

Open
he-yufeng wants to merge 1 commit into
QwenLM:mainfrom
he-yufeng:fix/permission-heredoc-split
Open

fix(core): keep heredoc bodies out of permission rule splitting#9417
he-yufeng wants to merge 1 commit into
QwenLM:mainfrom
he-yufeng:fix/permission-heredoc-split

Conversation

@he-yufeng

Copy link
Copy Markdown
Contributor

What this PR does

Fixes #9381. A shell command with a heredoc body was split per line during permission rule evaluation, so python - <<'PY'\nimport os\n...\nPY became four segments and an allow rule like Bash(python *) could never match the whole command. splitCompoundCommand now strips heredoc bodies before splitting, the same treatment the file-operation path (walkCompoundCommand) already applied.

Why it's needed

The per-line fallout was real friction: body lines like import os were evaluated as commands of their own, matching nothing and falling through to per-line prompts. Heredoc bodies are stdin, not shell segments, so they should not be evaluated at all.

Reviewer Test Plan

How to verify

splitHeredocBodies moves unchanged from shell-semantics.ts to rule-parser.ts and splitCompoundCommand strips before it splits. The permission-manager suite covers it at both levels: splitCompoundCommand keeps python - <<'PY'\nimport os\nprint(os.getcwd())\nPY as one segment (plus body-with-operators and <<- variants), and a Bash(python *) allow rule now auto-approves that whole heredoc command end to end. The new tests fail on main without the rule-parser change and pass with it.

Evidence (Before & After)

N/A (permission evaluation internals, no UI)

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

npx vitest run src/permissions/permission-manager.test.ts src/permissions/shell-semantics.test.ts passes 444/444 on macOS.

Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finished — CI landed green on a691ccf and the deferred approval was posted. finalize run

Qwen Triage 已完成 —— a691ccf 的 CI 全绿,延迟审批已提交。查看 finalize 运行

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template: looks good ✓ — all three main sections and the full Reviewer Test Plan are filled in (including the Tested-on table and an honest N/A for before/after evidence on a non-UI change). Minor: the Risk & Scope and Linked Issues headings are absent, but both are substantively covered — the body opens with Fixes #9381 and discusses the tradeoffs — so not worth a respin.

Problem: observed bug with a concrete reproduction, not theoretical hardening. #9381 shows python - <<'PY'\nimport os\nprint(os.getcwd())\nPY split into four segments — import os, print(...) and PY evaluated as standalone commands, per-line prompts, and Bash(python *) allow rules that can never match. Issue triage confirmed the root cause on main by source inspection: splitCompoundCommand() splits without stripping heredoc bodies while the file-operation path (walkCompoundCommand) already strips them.

Direction: aligned — this makes the rule-evaluation path consistent with the file-operation path that already shipped with heredoc stripping, which is exactly the fix the issue prescribes. CHANGELOG: no direct reference, but permission rule matching is core qwen-code surface.

Size: core paths touched (packages/core/src/permissions/). Production logic: 183 lines (rule-parser.ts +95/−3, shell-semantics.ts +4/−81) vs test lines: 38 (permission-manager.test.ts). Well under every threshold.

Approach: the scope is exactly right. stripHeredocBodies/getHeredocDelimiters move unchanged out of shell-semantics.ts into rule-parser.ts (verified byte-identical, only added an export + doc comments), and splitCompoundCommand strips before it splits. No new parsing logic — the existing helper is simply shared with its second caller — and no drive-by edits.

Risk: no elevated risk signals (none of the revert-correlated paths matched). One semantic note for reviewers: heredoc bodies stop being evaluated line-by-line, so a deny rule that happened to match body text (e.g. Bash(rm *) against a cat heredoc containing rm -rf /) no longer fires. That matching was an artifact of the over-splitting this issue reports — the body is stdin, not executed shell — and single-command treatment is the semantics the issue explicitly asks for.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板:完整 ✓ —— 三个主要部分和完整的 Reviewer Test Plan 都已填写(含测试平台表格,非 UI 改动的 before/after 证据如实写了 N/A)。小问题:缺少 Risk & ScopeLinked Issues 标题,但实质内容都有——正文开头就是 Fixes #9381,权衡也有讨论——不值得为此打回。

问题:已观测到的 bug,有具体复现,不是理论性加固。#9381 展示了 python - <<'PY'\nimport os\nprint(os.getcwd())\nPY 被拆成四段——import osprint(...)PY 被当作独立命令求值,逐行弹窗,且 Bash(python *) allow 规则永远无法匹配。issue triage 已通过源码检查在 main 上确认根因:splitCompoundCommand() 拆分时不剥离 heredoc 正文,而文件操作路径(walkCompoundCommand)早已剥离。

方向:对齐——使规则求值路径与已上线的、带 heredoc 剥离的文件操作路径保持一致,正是 issue 指定的修复方式。CHANGELOG 无直接条目,但权限规则匹配是 qwen-code 的核心面。

规模:触及核心路径(packages/core/src/permissions/)。生产逻辑 183 行(rule-parser.ts +95/−3、shell-semantics.ts +4/−81),测试 38 行(permission-manager.test.ts),远低于所有阈值。

方案:范围恰到好处。stripHeredocBodies/getHeredocDelimiters 原样从 shell-semantics.ts 移入 rule-parser.ts(已逐字节核对一致,仅新增 export 和文档注释),splitCompoundCommand 在拆分前先剥离。没有新的解析逻辑——只是把已有 helper 共享给第二个调用方——也没有夹带其他改动。

风险:无升级风险信号(未命中任何与 revert 相关的高风险路径)。一个供 reviewer 留意的语义点:heredoc 正文不再逐行求值,因此恰好匹配正文文本的 deny 规则(例如针对含 rm -rf /cat heredoc 的 Bash(rm *))将不再触发。那种匹配正是本 issue 所述过度拆分的副产物——正文是 stdin,不是被执行的 shell——按单个命令处理正是 issue 明确要求的语义。

进入代码审查。🔍

Qwen Code · qwen3.8-max

Reviewed at a691ccf07ff1612dfc0ec659b26533d4171094c7 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Code review

Independent proposal first: with the issue's root-cause analysis in hand, the fix I'd write is exactly what this PR does — reuse the heredoc stripping the file-op path already has, applied before the rule path splits. The PR matches that baseline; I didn't find a simpler path it missed.

Findings, in order of what I checked:

  • The move is pure. stripHeredocBodies and getHeredocDelimiters are relocated from shell-semantics.ts to rule-parser.ts byte-for-byte identical (only additions: export and doc comments). walkCompoundCommand's call site is unchanged apart from the import, so the file-operation path behaves exactly as before.
  • No dependency cycle. rule-parser.ts imports node builtins, picomatch, shell-quote, utils and ./types.js only — nothing from shell-semantics.ts, which already imports from rule-parser.js (the dependency direction is pre-existing).
  • Consumers named. splitCompoundCommand has exactly three production call sites, all in permission-manager.ts: evaluate (:227), hasRelevantRules (:810), hasMatchingAskRule (:908). All three follow the same shape — multiple segments get per-segment recursion, a single segment falls through to whole-command evaluation (evaluateSingle + resolveDefaultPermission on the raw command). With stripping, a heredoc command now takes the single-segment path, which is the same treatment any one-line command gets.
  • Splitting semantics stay correct. Real compound structure around a heredoc still splits — the cat <<EOF && echo hi\nbody; with && ops\nEOF\necho done test yields cat <<EOF / echo hi / echo done; top-level \n outside heredocs still splits; <<< here-strings and echo "<<EOF" quoted text are still not treated as heredocs (both guarded in the moved code, both already covered by shell-semantics.test.ts).
  • Tests pin the change. Three splitter unit tests (plain heredoc, operators inside the body plus a segment after the terminator, the <<- tab variant) plus one end-to-end PermissionManager.evaluate test where Bash(python *) auto-approves the whole heredoc command. On main without the strip call these fail by construction — the suite is not green-with-or-without-the-diff.

Non-blocking observations:

  • The security nuance from Stage 1 bears repeating here for the merging maintainer: deny rules can no longer match text that sits inside a heredoc body (cat <<EOF containing rm -rf / is evaluated as cat <<EOF only). That's the intended semantics — the body is stdin — and it's consistent with the file-op path, which never inspected bodies. Commands that genuinely execute their stdin (bash <<EOF) don't gain new surface: anyone holding a Bash(bash *) allow rule already authorises arbitrary execution via bash -c.
  • There's a third, simpler heredoc stripper in packages/cli/src/serve/daemon-git-worktree-guard.ts (regex-based, first heredoc per line, explicitly best-effort). Pre-existing, different package, intentionally local — unifying it would mean widening core's public API and is out of scope here. Noting it only so a future cleanup knows it exists.

Testing evidence

Unattended CI run — no PR code is built or executed by this review. Evidence below is the PR's own CI on the reviewed commit, read via the API.

Final CI results for a691ccf (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Secret scan (TruffleHog) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

The unit suite (Test (ubuntu-latest, Node 22.x) — this repo's lint + typecheck + build + unit tests) was still running at review time; per policy this review fetches once and does not poll. The macOS/Windows "Test" entries are skipped matrix placeholders, not PR-caused failures. The author reports 444/444 passing locally on macOS for the two permission suites — author's claim, not independently re-run here.

Sandboxed verification would settle the remaining gap: @qwen-code /verify — that the new permission-manager and splitter tests genuinely fail on the base build and pass with the diff (i.e. they pin the stripping behaviour rather than passing either way). This is a fork PR, so /verify runs as a sponsored run: a maintainer's @qwen-code /verify comment approves the head it's written against, the run carries a pre-execution risk screen and a full workspace wipe, and its report should be read with the same skepticism as the fork's own CI logs. The new tests are deterministic pure-function assertions that fail by construction without the strip call, so this is belt-and-braces rather than a doubt about the suite.

中文说明

代码审查

先写独立方案:基于 issue 的根因分析,我会做的修复正是本 PR 的做法——复用文件操作路径已有的 heredoc 剥离,在规则路径拆分之前应用。PR 与该基线一致,没有找到更简单的路径。

  • 纯移动stripHeredocBodiesgetHeredocDelimitersshell-semantics.ts 逐字节原样移入 rule-parser.ts(仅新增 export 与文档注释);walkCompoundCommand 调用点除 import 外不变,文件操作路径行为与之前完全一致。
  • 无循环依赖rule-parser.ts 只依赖 node 内置模块、picomatchshell-quote、utils 与 ./types.js,不反向依赖 shell-semantics.ts(依赖方向本就如此)。
  • 消费方全部列明splitCompoundCommand 生产调用点恰为三处,均在 permission-manager.tsevaluate :227、hasRelevantRules :810、hasMatchingAskRule :908)。三处形态相同:多段则逐段递归,单段则走整命令求值。剥离后 heredoc 命令走单段路径,与任何单行命令待遇一致。
  • 拆分语义保持正确:heredoc 外围的真实复合结构仍会拆分;heredoc 之外的顶层 \n 仍拆分;<<< here-string 与引号内的 <<EOF 仍不被当作 heredoc(移动的代码中已有防护,shell-semantics.test.ts 已有覆盖)。
  • 测试钉住变更:三个拆分器单测(纯 heredoc、正文含操作符且终止符后还有段、<<- 制表符变体)加一个端到端 PermissionManager.evaluate 测试(Bash(python *) 自动批准整条 heredoc 命令)。在没有 strip 调用的 main 上这些测试必然失败——不是那种有没有 diff 都绿的套件。

非阻塞观察:

  • Stage 1 提到的安全语义值得在合并前再强调一次:deny 规则不再能匹配 heredoc 正文中的文本(含 rm -rf /cat <<EOF 只按 cat <<EOF 求值)。这是预期语义——正文是 stdin——且与从不检查正文的文件操作路径一致。真正执行 stdin 的命令(bash <<EOF)也没有新增面:持有 Bash(bash *) allow 规则者本就可经 bash -c 授权任意执行。
  • packages/cli/src/serve/daemon-git-worktree-guard.ts 中存在第三个更简化的 heredoc 剥离器(正则、每行仅首个 heredoc、明确 best-effort)。既有代码、不同包、有意本地化——统一它需扩大 core 公共 API,超出本 PR 范围。仅提醒未来清理时知道它的存在。

测试证据

无人值守 CI 运行——本审查不构建、不执行 PR 代码。以上证据为本 PR 自身 CI 在被审 commit 上的结果,经 API 读取。单测套件(ubuntu)审查时仍在运行,按策略只抓取一次、不轮询;macOS/Windows 的 "Test" 为被跳过的矩阵占位,并非 PR 导致的失败。作者报告 macOS 本地 444/444 通过——为作者声明,未在此独立复跑。沙箱验证可补齐剩余缺口:@qwen-code /verify(fork PR 下为维护者发起的 sponsored run,带执行前风险筛查与工作区清理)可证明新测试在基线构建上确实失败、带上 diff 后通过。新测试是确定性纯函数断言,缺 strip 调用必然失败,因此这只是双保险。

Qwen Code · qwen3.8-max

Reviewed at a691ccf07ff1612dfc0ec659b26533d4171094c7 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean, minimal fix for a confirmed bug; only non-blocking nits (missing Risk & Scope heading, one semantic note for the merging maintainer).

Stepping back: this is what a good fork PR looks like. The issue came with a root-cause analysis, the PR does exactly the prescribed fix and nothing else — a byte-identical function move plus one application of it — and the tests fail on main by construction, so they actually pin the behaviour. My independent proposal matched the PR's approach completely; I went looking for a simpler path and there isn't one, because the helper already exists and just needed a second caller.

The one thing I'd want the merging maintainer to hold in mind is the deny-rule semantics: after this change, rule matching sees cat <<EOF where it used to also see the body's lines. I satisfied myself that this is correct — the body is stdin, the file-op path never inspected it, and the old line-by-line matching was the very over-splitting #9381 complains about — but it's the kind of tradeoff a human should consciously sign off on a security-labelled issue.

Everything else is verified: consumers are named and consistent, no import cycle, splitting around heredocs still works, here-strings and quoted << text are still handled. The only reason not to approve this instant is that the ubuntu unit suite was still running at review time — approval is deferred until CI lands green on a691ccf07ff1612dfc0ec659b26533d4171094c7.

中文说明

置信度:4/5 —— 对已确认 bug 的干净、最小修复;仅有非阻塞小问题(缺 Risk & Scope 标题、一个供合并维护者留意的语义点)。

整体来看:这是一个优秀 fork PR 的样子。issue 自带根因分析,PR 严格实施指定修复、毫无多余——一次逐字节一致的函数移动加一处应用——测试在 main 上必然失败,确实钉住了行为。我的独立方案与 PR 完全一致;我尝试寻找更简路径但没有,因为 helper 早已存在,只是需要第二个调用方。

唯一希望合并维护者心里有数的是 deny 规则语义:此改动后,规则匹配看到的是 cat <<EOF,而过去还会看到正文行。我确认这是正确的——正文是 stdin,文件操作路径从不检查正文,旧的逐行匹配正是 #9381 抱怨的过度拆分——但这是安全标签 issue 上应由人类有意识拍板的权衡。

其余均已核实:消费方明确且一致、无循环依赖、heredoc 外围拆分正常、here-string 与引号内 << 文本仍正确处理。唯一不立即批准的原因是审查时 ubuntu 单测仍在运行——批准推迟到 CI 在 a691ccf07ff1612dfc0ec659b26533d4171094c7 上变绿之后。

Qwen Code · qwen3.8-max

Reviewed at a691ccf07ff1612dfc0ec659b26533d4171094c7 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship — CI landed green after the review. ✅

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

Not reviewed: reverse audit — stopped before round 3 by the review time budget.

Test Plan (not a blocker): src/permissions/permission-manager.test.tsno such file or directory; src/permissions/shell-semantics.test.tsno such file or directory.

— qwen3.8-max via Qwen Code /review (v0.21.11)

Comment on lines +931 to +933
const commands = splitCompoundCommandSegments(
stripHeredocBodies(command),
).map((segment) => segment.command);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] The heredoc lexer this diff wires into rule evaluation diverges from real bash in several corners, and in each one lines that bash executes are swallowed as "heredoc body" and never reach permission rules: deny/ask rules cannot match them, while a prefix allow rule (compiled with the s flag, so .* crosses newlines) auto-approves the whole multi-line command. Before this change the unstripped splitter returned those lines as their own segments, which were evaluated; the strip removes that last detection layer (the virtual-op pass only escalates from 'default', it never blocks an allow). Six executed entrances: (1) arithmetic — with deny: ["Bash(rm *)"], echo start + SIZE=$((1 << 20)) + rm -rf /important: the << inside $(( )) is a shift, but the lexer registers delimiter 20 and swallows the rm line — PermissionManager returns deny without the arithmetic line and allow with it; (2) comments — echo hi # <<EOF + touch /tmp/pwned + EOF: bash treats # <<EOF as a comment and runs the touch, while evaluate returns allow under Bash(echo *); (3) multi-line quotes — quote state resets per line, so <<EOF inside an unclosed double-quoted string opens a phantom heredoc that swallows a following rm bash executes; (4) continued delimiter — cat <<EO\ + F registers EO, never matches, swallows to end of input; (5) delimiter charset — cat <<A,B truncates the delimiter at ,, never matches the real A,B terminator, swallows to end of input; (6) body-start boundary — cat <<EOF \ followed by && rm -rf /important: bash folds the continuation into the opening logical line and executes the rm, but the stripper queues EOF after the physical line and swallows the rm line — verdict allow post-PR vs deny with only this wiring line reverted. The entrance space is general shell syntax the lexer does not model (comments, arithmetic, continuations, expansions, quoting), so patching entrances one by one will not converge; fail closed instead. Concretely: skip << inside arithmetic contexts ($((/((), stop heredoc scanning at word-start #, carry quote state across lines (or move body consumption into splitCompoundCommandSegments' single-pass scanner, which already tracks quotes across newlines), fold backslash-newline continuations into the opening line before body consumption, and when a delimiter cannot be resolved keep the remaining lines so they are over-evaluated (ask) instead of invisible. Longer-term, defer compound/heredoc structure to a real shell parser.

Witness (executed at this commit):

bash: SIZE=1048576, marker CREATED (the swallowed line ran)
PermissionManager: control 'echo start\nrm -rf /important' => deny
with arithmetic line => segments ["echo start","SIZE=$((1 << 20))"], verdict allow
continuation entrance: PR verdict allow; wiring line reverted =>
  split ["cat <<EOF \\","rm -rf /important","body","EOF"], verdict deny

— qwen3.8-max via Qwen Code /review (v0.21.11)

(segment) => segment.command,
);
const commands = splitCompoundCommandSegments(
stripHeredocBodies(command),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] "Heredoc bodies are stdin, not commands" is correct for cat/grep consumers (the bug being fixed) but not for shell interpreters — bash <<EOF executes its body, and this strip makes that body invisible to every Bash-pattern rule. Measured: with deny: ["Bash(touch *)"], bash <<'EOF' + touch /tmp/r12 + EOF was deny before this change (the body line was its own segment) and is ask after it (the only segment is bash <<'EOF'), and becomes silent allow under an allow rule like Bash(bash *). The silent path itself predates the diff — bash -c 'touch' under the same deny rule returns ask on both sides, because Bash-pattern matching never unwraps wrappers — so this is the semantic note the triage already flagged for the merging maintainer rather than a blocker: either document the trade-off (deny rules cannot see heredoc bodies fed to shells, same as bash -c) or special-case shell-interpreter heredoc openers (bash, sh, zsh, dash, eval) and keep their bodies visible to rule evaluation.

Witness (executed):

BASE (wiring reverted): deny
PR: segments ["bash <<'EOF'"], verdict ask; under Bash(bash *): allow
control: bash -c 'touch' => ask on both arms

— qwen3.8-max via Qwen Code /review (v0.21.11)

});

it('handles the tab-stripping heredoc variant', async () => {
expect(splitCompoundCommand('python <<-PY\n\timport os\n\tPY')).toEqual([

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This <<- test is vacuous against the two code paths that define the variant — the - skip in getHeredocDelimiters and the line.trim() terminator match — because nothing follows the terminator, so whether the heredoc ever closes is unobservable. Both mutants keep the test green while silently dropping trailing commands: deleting the - skip (delimiter becomes -PY) and changing line.trim() to line (tab-indented terminator no longer closes) each still pass this test, while on extended input both return ['python <<-PY'] instead of ['python <<-PY', 'echo done']echo done swallowed. Extend the input with a trailing segment; that single input kills both mutants:

expect(
  splitCompoundCommand('python <<-PY\n\timport os\n\tPY\necho done'),
).toEqual(['python <<-PY', 'echo done']);

— qwen3.8-max via Qwen Code /review (v0.21.11)

* and the delimiter line must survive splitting untouched instead of being
* evaluated as shell segments of its own.
*/
export function stripHeredocBodies(command: string): string {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This PR relocates and exports the heredoc stripper, but two other implementations of the same behavior remain and have already diverged: packages/cli/src/serve/daemon-git-worktree-guard.ts:1867 (regex-based, no quote state, only the first heredoc per line) and the heredoc tokenizer inside detectCommandSubstitution in packages/core/src/utils/shell-utils.ts. The divergence is executed, not hypothetical: for echo "<<EOF" followed by touch /tmp/x, this scanner correctly keeps the touch line as a segment, while the guard's regex matches delimiter EOF through the double quotes and strips it — the same command is segmented differently by the permission manager and the daemon worktree guard. A heredoc fix like this one now has to be re-derived in up to three places, and fixing only one site silently re-diverges them. Since the guard already imports from @qwen-code/qwen-code-core, re-export this function from the core package entry and replace the guard's regex copy (here or in a recorded follow-up).

— qwen3.8-max via Qwen Code /review (v0.21.11)

* "x=$(( a & b ))" → ["x=$(( a & b ))"] (arithmetic, not async)
* "python - <<'PY'\nimport os\nPY" → ["python - <<'PY'"] (heredoc body is stdin)
*/
export function splitCompoundCommand(command: string): string[] {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The sibling docstring this change falsifies: splitCompoundCommandSegments (~lines 833-835, unchanged text) says of this function "this is the same split, and that function is a projection of this one." After this diff the two are no longer the same split — this one strips heredoc bodies first, Segments does not — and the example list now contains the heredoc case, which Segments does not honour. Measured: Segments on "python - <<'PY'\nimport os\nPY" returns ["python - <<'PY'", "import os", "PY"] while this function returns ["python - <<'PY'"]. A future caller needing terminator info reaches for the exported Segments on a raw command, reads "the same split", and resurrects the #9381 bug class on the segment path. Amend the Segments docstring to say that callers passing raw commands must strip heredoc bodies first (stripHeredocBodies) and that this function does it for them — or apply the strip inside Segments and drop it from this wrapper.

— qwen3.8-max via Qwen Code /review (v0.21.11)


for (const line of lines) {
if (pendingDelimiters.length > 0) {
if (line.trim() === pendingDelimiters[0]) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] line.trim() terminator matching is laxer than bash: a heredoc ends only at a line exactly equal to the delimiter (<<- strips only leading tabs), so an indented or padded delimiter-lookalike body line terminates the skip early and the remaining genuine body lines become evaluated command segments. Measured: splitCompoundCommand('cat <<EOF\n EOF\ncat data.txt\nEOF') yields ['cat <<EOF', 'cat data.txt', 'EOF'] while bash treats EOF and cat data.txt as body data — producing a spurious ask (or a deny if a data line resembles a denied pattern) for an otherwise-allowed heredoc command. This errs in the over-evaluation direction, so it is not a bypass — but it is newly reachable in the rule path via this diff and it is the same class of bash divergence the PR is fixing. Match terminators exactly (line === delimiter), stripping only leading tabs when the opener was <<-.

— qwen3.8-max via Qwen Code /review (v0.21.11)

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed. Suggestions are inline.

2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R1-6 daemon-git-worktree-guard keeps a weaker regex copy of the now-exported stripHeredocBodies — already reported (comment 3806112221)
  • R1-3 stale 'projection' JSDoc on splitCompoundCommandSegments — already reported (comment 3806112224)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not reviewed: reverse audit — stopped before round 6 by the review time budget.

Test Plan (not a blocker): src/permissions/permission-manager.test.tsno such file or directory; src/permissions/shell-semantics.test.tsno such file or directory.

— qwen3.8-max via Qwen Code /review (v0.21.13)

Comment on lines +38 to +41
import {
splitCompoundCommandSegments,
stripHeredocBodies,
} from './rule-parser.js';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The heredoc-strip invariant is enforced in the thin wrapper splitCompoundCommand instead of the exported primitive splitCompoundCommandSegments that actually performs the split — this import is added solely to sustain walkCompoundCommand's manual composition splitCompoundCommandSegments(stripHeredocBodies(command)).

Failure scenario: the next caller that reaches for the exported primitive (it has its own dedicated tests, advertising it as a public building block) will silently split heredoc bodies into command segments again — re-instantiating bug #9381 for that caller with no diagnostic. The strip is lossless to push down: CompoundCommandSegment carries only the trimmed command string and terminator operator.

Suggested fix: move stripHeredocBodies(command) inside splitCompoundCommandSegments, revert splitCompoundCommand to projecting the primitive's result, and drop this manual composition (and this import).

— qwen3.8-max via Qwen Code /review (v0.21.13)

Comment on lines +755 to +757
if (line.trim() === pendingDelimiters[0]) {
pendingDelimiters.shift();
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The concurrent-delimiter path of stripHeredocBodies (the pendingDelimiters queue and its shift() ordering) is never exercised by any test, though it is now wired into the permission-rule path via splitCompoundCommand. The one-line mutation shift()pop() passes every existing test (verified: 533/533 pass under the mutation).

Failure scenario: multiple heredocs opened on one line are valid bash (cat <<A <<B). Under the mutation, for cat <<A <<B\nbodyA\nA\nbodyB\nB\nrm -rf / the terminator A pops B, nothing later matches A, and everything after — bodyB, B, and the real trailing command rm -rf / — is swallowed as heredoc body and never evaluated. Nothing in the suite detects this regression class. Current code is correct (probe: segments ["cat <<A <<B", "rm -rf /"]), so this is unpinned coverage on a safety-relevant ordering, not a live bug.

Suggested fix: add a unit test such as expect(splitCompoundCommand('cat <<A <<B\nbodyA\nA\nbodyB\nB && echo done')).toEqual(['cat <<A <<B', 'echo done']).

— qwen3.8-max via Qwen Code /review (v0.21.13)

Comment on lines +721 to +722
const quote = line[wordStart];
const quoted = quote === "'" || quote === '"';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Double-quoted heredoc delimiters (<<"TAG") are untested everywhere in the permissions suite (only <<EOF, <<'PY', and <<-PY forms appear), leaving the double-quote alternative of getHeredocDelimiters unpinned on the new rule-evaluation path. The one-line mutation dropping the || quote === '"' alternative passes all current tests (verified).

Failure scenario: under the mutation, cat <<"TAG"\nrm -rf / && ops\nTAG would no longer be stripped, so the body lines fall back into per-segment evaluation — exactly the #9381 failure mode (spurious prompts, prefix allow rules never matching the whole command) silently reintroduced for the double-quoted delimiter form. Current code handles the input correctly, so this is an unpinned-branch gap, not a live bug.

Suggested fix: extend one of the new tests, e.g. expect(splitCompoundCommand('cat <<"TAG"\nbody && ops\nTAG')).toEqual(['cat <<"TAG"']).

— qwen3.8-max via Qwen Code /review (v0.21.13)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Permission system splits heredoc/multi-line shell commands per line, breaking Bash(prefix) allow rules

3 participants