Skip to content

fix(core): analyse the statements and redirects nested in a heredoc node - #10028

Closed
TianYuan1024 wants to merge 2 commits into
QwenLM:mainfrom
TianYuan1024:fix/heredoc-nested-analysis
Closed

fix(core): analyse the statements and redirects nested in a heredoc node#10028
TianYuan1024 wants to merge 2 commits into
QwenLM:mainfrom
TianYuan1024:fix/heredoc-nested-analysis

Conversation

@TianYuan1024

@TianYuan1024 TianYuan1024 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Makes the classifier look at what a heredoc opener carries with it. tree-sitter parses whatever follows the opener on the same line inside the heredoc_redirect node, beside the body — and the redirected_statement arm filtered every redirect child out before evaluation, so two different kinds of thing written there vanished from the analysis.

A statement: cat <<EOF && rm -rf build classified read-only. So did the ;, |, || and & spellings, and every compound shape written there — for, if, while, a { … } block, a ! negation, a subshell, a case.

A redirect: cat <<EOF >out.txt and cat <<EOF 2>out.txt classified read-only too, for a related but separate reason — evaluateRedirectionSafety only walks the direct children of the redirected_statement, so it never reached inside the heredoc node either.

Both are now evaluated, each on the axis it belongs to: a child that is itself a redirect goes to evaluateRedirectionSafety, anything else goes to evaluateStatementSafety. The inert leaves — the delimiters, the body, a bare file descriptor — are named in a skip-list rather than the statement shapes being named in an allow-list, so a shape nobody anticipated is evaluated and floored at unknown instead of silently dropped. cat <<EOF 2>&1 stays read-only: that names a descriptor, not a file.

Why it's needed

The AST classifier is what Plan Mode and the shell confirmation dialog use to decide whether a command needs approval. A command that classifies read-only runs unattended in Plan Mode. cat <<EOF && rm -rf build is a plain deletion wearing a heredoc, and it ran with no prompt.

The redirect half is the same problem from the other side: a write that the redirection walk could not see because it was parsed one level deeper than the walk looked.

Reviewer Test Plan

How to verify

cd packages/core && npx vitest run src/utils/shellAstParser.test.ts

 Test Files  1 passed (1)
      Tests  568 passed (568)

Before this change, on main:

await isShellCommandReadOnlyAST('cat <<EOF && rm -rf build\nhello\nEOF')  // true
await isShellCommandReadOnlyAST('cat <<EOF >out.txt\nhello\nEOF')         // true
await isShellCommandReadOnlyAST('cat <<EOF 2>out.txt\nhello\nEOF')        // true

After, all three are false, while the inert forms stay true:

await isShellCommandReadOnlyAST('cat <<EOF\nhello\nEOF')        // true
await isShellCommandReadOnlyAST("cat <<'EOF'\n`rm -rf build`\nEOF")  // true
await isShellCommandReadOnlyAST('cat <<EOF 2>&1\nhello\nEOF')   // true

Both arms are mutation-verified rather than merely covered: removing the nested-redirect routing fails 4 tests, removing the nested-statement walk fails 12.

Note: src/permissions/permission-manager.test.ts has 2 failures on main at this commit (resolveToolName exhaustiveness (#9827), about ReportFindings). They are unrelated to this PR and reproduce on a clean origin/main checkout.

Evidence (Before & After)

N/A — no TUI change. The user-visible difference is that these commands now prompt instead of running unattended, covered by the unit tests above.

Tested on

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

Risk & Scope

  • Main risk: commands that previously classified read-only now classify write or unknown. That is the point of the change, but it means a heredoc form someone relied on running silently will start prompting. The skip-list is deliberately small, so the over-refusal is bounded to redirect leaves nobody writes statements in.
  • Not validated / out of scope: the pre-existing permission-manager failures noted above. The deprecated regex fallback in shellReadOnlyChecker.ts is untouched — it keeps its own duplicate logic and prompts more, which fails closed.
  • Breaking changes: none.

Linked Issues

No issue to close. This is a self-contained correctness fix carved out of #9950 — the heredoc-nesting gap surfaced while that PR was under review, and it is filed separately so it can land on its own merits. Referenced without a closing keyword: #9950.

中文说明

这个 PR 做了什么

让分类器看见 heredoc 开启符同行携带的东西。tree-sitter 会把开启符之后同一行的内容解析到 heredoc_redirect 节点内部、与 body 并列,而 redirected_statement 分支在求值前把所有重定向子节点都过滤掉了,于是写在那里的两类东西都从分析中消失了。

语句cat <<EOF && rm -rf build 被判为 read-only;|||& 各种写法同样如此,写在那里的所有复合结构(forifwhile{ … } 代码块、! 取反、子 shell、case)也都如此。

重定向cat <<EOF >out.txtcat <<EOF 2>out.txt 也被判为 read-only——原因相关但独立:evaluateRedirectionSafety 只遍历 redirected_statement 的直接子节点,同样没能进到 heredoc 节点里面。

现在两者都会被求值,且各归其轴:本身是重定向的子节点交给 evaluateRedirectionSafety,其余交给 evaluateStatementSafety。惰性叶子(分隔符、body、裸文件描述符)以跳过清单列出,而不是把语句形态列进白名单,这样没人预料到的形态会被求值并下压到 unknown,而不是被静默丢弃。cat <<EOF 2>&1 仍是只读:它指的是描述符,不是文件。

为什么需要它

AST 分类器正是 Plan Mode 与 shell 确认对话框用来判断命令是否需要批准的依据。被判为 read-only 的命令在 Plan Mode 下无人值守执行。cat <<EOF && rm -rf build 是一次披着 heredoc 外衣的删除,而它不会弹窗。

重定向那一半是同一问题的另一面:一次写操作因为被解析到比遍历深度更深一层而无法被看见。

验证方式

见上文英文部分的命令与前后对比。两个分支都做了变异测试而非仅有覆盖:移除嵌套重定向路由会导致 4 个测试失败,移除嵌套语句遍历会导致 12 个失败。

注:本提交所基于的 main 上,src/permissions/permission-manager.test.ts 本身有 2 个失败(resolveToolName exhaustiveness (#9827),与 ReportFindings 有关),与本 PR 无关,在干净的 origin/main 检出上同样复现。

风险与范围

  • 主要风险: 此前被判 read-only 的命令现在会被判为 writeunknown。这正是本次变更的目的,但意味着有人依赖其静默执行的某种 heredoc 写法会开始弹窗。跳过清单刻意保持精简,过度拒绝被限制在无人会往里写语句的重定向叶子上。
  • 明确不在范围内: 上述 main 自带的失败;shellReadOnlyChecker.ts 中已废弃的正则回退路径不动。
  • 破坏性变更: 无。

关联 Issue

没有需要关闭的 issue。这是从 #9950 中拆分出来的一处独立正确性修复——heredoc 嵌套这个缺口是在那个 PR 评审过程中发现的,单独提出以便独立评审合入。仅作引用、不带关闭关键字:#9950

tree-sitter parses whatever follows a heredoc opener on the same line *inside*
the `heredoc_redirect` node, beside the body. The `redirected_statement` arm
filtered every redirect child out before evaluation, so both kinds of thing
written there vanished from the analysis:

- a statement — `cat <<EOF && rm -rf build` classified `read-only`, as did the
  `;`, `|`, `||`, `&` spellings and every compound shape (`for`, `if`, `while`,
  a block, a negation, a subshell, a `case`); and
- a redirect — `cat <<EOF >out.txt` and `cat <<EOF 2>out.txt` classified
  `read-only` too, because `evaluateRedirectionSafety` only walks the direct
  children of the `redirected_statement` and never reached inside the heredoc
  node.

Both are now evaluated, each on the right axis: a child that is itself a
redirect goes to `evaluateRedirectionSafety`, anything else goes to
`evaluateStatementSafety`. The inert leaves — the delimiters, the body, a bare
file descriptor — are named in a skip-list rather than the statement shapes
being named in an allow-list, so an unanticipated shape is evaluated and
floored at `unknown` instead of silently dropped. `cat <<EOF 2>&1` stays
read-only: that names a descriptor, not a file.

Both arms are mutation-verified: removing the redirect routing fails 4 tests,
removing the statement walk fails 12.
@TianYuan1024

Copy link
Copy Markdown
Contributor Author

Added the ## Linked Issues section (and its Chinese counterpart). There is no issue to close — this PR is a self-contained fix carved out of #9950, so the section references that PR without a closing keyword, per the template's "Otherwise reference without a closing keyword."

Also merged origin/main (a6d30ebc6b), which registers report_findings in the permission alias table and clears the two permission-manager.test.ts > resolveToolName exhaustiveness (#9827) failures this branch was inheriting from main.

Ready for re-run.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

🔄 Qwen Triage is runningwatch live progress. Stage results will post in this thread as they complete.

🔄 Qwen Triage 正在运行 —— 查看实时进度。各阶段结果完成后会更新在本线程。

@TianYuan1024

Copy link
Copy Markdown
Contributor Author

The triage run on this PR never finished — run 32862816555 was cancelled, as was the run on the current head, along with most other runs in the repository between 16:05Z and 16:49Z. That was a capacity event, not a failure on this commit.

Since then the head moved to 197b842385: the ## Linked Issues section is in the body, and origin/main (a6d30ebc6b) is merged in, which clears the two permission-manager.test.ts > resolveToolName exhaustiveness (#9827) failures this branch was inheriting from main. No file in the PR's own diff changed.

@qwen-code /triage

@TianYuan1024

Copy link
Copy Markdown
Contributor Author

@qwen-code /triage

(The earlier request on this PR had the mention at the end of the comment rather than the first line, so it never matched the trigger — resending it correctly.)

Head is 197b842385: the ## Linked Issues section is in the body, and origin/main (a6d30ebc6b) is merged in, which clears the two permission-manager.test.ts > resolveToolName exhaustiveness (#9827) failures inherited from main. No file in the PR's own diff has changed since the last run.

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.

2 participants