fix(core): treat grep -n as a flag so its file reads are still checked - #14383

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

✅ Qwen Triage 已完成 —— 1bda967 的 CI 全绿，延迟审批已提交。查看 finalize 运行

Thanks for the PR!

Template looks good ✓

Problem: real and reproduced, not theoretical. -n / --line-number were listed among the grep flags that consume the next token, so in grep -n pattern /etc/hosts the pattern was eaten as the flag's value, the filename became the pattern, and the analysis reported no file operation at all. I traced this through getPositionalArgs and confirmed it — positional ends up as just the filename, slice(1) drops it, and the handler returns []. Since permission checks only escalate from the operations this analysis reports, a Read deny rule on that path never fired for the most common form of grep. GNU grep's -n genuinely takes no argument, so the entry was simply wrong. The before/after in the description plus the new unit test are a proper reproduction.

Direction: aligned. This is a correctness fix in permission analysis, and it fails in the safe direction — the analysis now reports more file operations, so more commands get checked rather than fewer. It does not expand what the shell tool can do.

Size: touches core (packages/core/src/permissions/). 4 production lines (2 added, 2 removed in shell-semantics.ts), 5 test lines, 0 generated/schema. Far below any escalation threshold — no maintainer awareness flag needed on size.

Approach: the scope is right. Deleting two entries from one flag set is the minimal fix, and the comment left behind explains why they must not come back — worth the two lines, since -n taking a value is exactly the mistake someone would re-introduce by analogy with head -n. I checked that head keeps its own separate handler with its own -n, so head -n 10 is unaffected. egrep/fgrep/zgrep/bzgrep delegate to the same handler and inherit the fix, which is also correct — their -n takes no argument either.

One consequence worth naming, not a blocker: combined short flags change too. grep -rn pattern dir previously matched -n through the combined-flag path and swallowed the pattern, reporting nothing; now the pattern stays positional and dir is reported. That is an improvement, but isRecursive only matches exact -r/-R/--recursive tokens, so -rn resolves to read_file rather than list_directory. That is a pre-existing gap this PR makes visible rather than introduces — reasonable to leave for a follow-up.

Risk: no elevated risk signals — neither changed file matches the revert-correlated paths.

Moving on to code review. 🔍

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at 1bda9676a2b7f13f5a534a01b3b6114f4dc696c8 · re-run with @qwen-code /triage}

Code review

No blocking issues. I read the change against getPositionalArgs rather than taking the description's word for it, and the diagnosis holds exactly as stated.

The fix is correct. With -n in the value-flag set, grep -n pattern /etc/hosts skips pattern as the flag's value, leaving positional = ['/etc/hosts']; since there is no -e/-f, slice(1) drops the only real path and the handler returns []. With -n removed it falls through as a plain flag, positional = ['pattern', '/etc/hosts'], and slice(1) yields the file. The direction of the error is the one that matters for a permission gate: the old code under-reported, so a Read deny rule silently did not apply.

It also fixes a second case the description does not mention. grep -n -e pattern file previously consumed -e as -n's value, which left hasPatternFlag true while the pattern stayed positional — so the analysis reported a bogus read_file on the literal string pattern. That goes away too.

Consumers checked. This is a core path, so I traced everything downstream rather than assuming:

• head has its own handler with its own -n set (line 521) — head -n 10 is unaffected, confirming the description's claim.
• egrep/fgrep/zgrep/bzgrep delegate to the same handler and inherit the fix, correctly — their -n takes no argument either.
• rg has a separate flag set and is untouched.
• --line-number appears exactly once in packages/ (the line being removed), so no other code path depends on it.
• The real consumers are permission-manager.ts (4 call sites) and autoMode.ts, both via extractShellOperationsAcrossCommand. The change can only add operations to the returned list, so the effect is strictly more checking, never less. Fail-safe.
• No existing test asserts the old behavior — nothing in the permissions suite uses grep with an n-bearing flag, and the neighbouring cat -n / head -n 10 cases use different handlers.

Two tests outside the permissions suite use grep -rn, and this PR changes what that resolves to. Previously -rn matched -n through the combined-short-flag branch and swallowed the pattern, so the analysis returned nothing; now the pattern stays positional and the trailing directory is reported. I checked both and neither should break:

• daemon-git-worktree-guard.test.ts asserts grep -rn 'git reset' src is allowed: true. That guard keys on git relocation markers and unrecognized program words, not on grep's file ops, and the new op is a relative in-workspace path — so the assertion should still hold. Worth a glance when CI lands, since it is the one place a newly-reported op could plausibly interact with a guard decision.
• session-pr-service.test.ts asserts runs("grep -rn 'gh pr create' .") is false. runs() does program-word detection, not file-op extraction — unaffected.

Non-blocking suggestions, neither of which should hold the PR:

• The diff removes both -n and --line-number but only adds a test for -n. One more row for the long form would be nearly free and would stop a future edit from reintroducing it.
• As noted in the gate comment, grep -rn pattern dir now reports read_file on dir rather than list_directory, because isRecursive only matches exact -r/-R/--recursive tokens and not combined forms. Pre-existing gap, newly visible, fine as a follow-up.

The comment left in the set is the right call — -n taking a value is precisely the mistake someone would reintroduce by analogy with head -n.

Test evidence

Unattended CI run — I did not build or execute any PR code. The evidence below is this PR's own CI on the reviewed commit, fetched once via the API. The unit suite had not finished at the time of this review, so there is no green result to quote yet; the pending jobs are listed as pending rather than guessed at.

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

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

No check has failed, so there is no log excerpt to quote. macOS and Windows unit jobs are skipped by this repo's workflow config, not by anything in the PR — and since the change is pure string-token parsing with no platform-dependent branch, the Linux job is the meaningful signal here. The author reports testing on Linux only; that is the author's claim, not independently re-run, and the change has no OS-specific path that would make it load-bearing.

The new unit test does pin the change — it asserts the exact read_file operation that the old code failed to produce, so removing the fix turns it red. That is a real pin, not a suite that passes either way.

Sandboxed verification would still settle the part the unit test cannot: @qwen-code /verify — that a Read deny rule on a file now actually blocks grep -n pattern <file> end-to-end through the permission manager. The unit test pins extractShellOperations returning the operation; nothing here demonstrates the deny path engaging on it, and that user-visible consequence is the whole point of the PR. The author has read-only access, so this would be a sponsored run — a maintainer's @qwen-code /verify comment approves the head it was written against, and that run carries a pre-execution risk screen and a full workspace wipe. Read the resulting report with the same skepticism as the fork's own CI logs: the code under verification is adversarial input and can shape what the report says, even though the sandbox bounds what it can do.

Real-scenario testing

N/A on this run — unattended CI, so no tmux session was driven and PR code was never executed. The live-behaviour gap is named above with the lane that would close it.

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at 1bda9676a2b7f13f5a534a01b3b6114f4dc696c8 · re-run with @qwen-code /triage}

Confidence: 4/5 — correct, minimal, and fails in the safe direction; the only nits are a missing test for the long-form flag and a pre-existing -rn recursion gap this PR happens to expose.

Stepping back: my independent read of the problem before opening the diff was that -n simply should not be in the value-flag set, and that is precisely the two lines this PR removes. There was no simpler path available — the fix is already the minimal one, and I could not construct an alternative that was smaller or safer.

The reason I am comfortable on a core path is that the change can only add operations to what the analysis reports. Every consumer I traced (permission-manager.ts, autoMode.ts) uses that list to decide what needs checking, so the failure mode of getting this wrong would have been under-enforcement — and that is exactly the bug being fixed, not a new one. The old behaviour silently defeated Read deny rules for the single most common way people write grep. That is worth fixing on its own merits, and the fact that it also removes a spurious read_file on the literal pattern in the -n -e form is a bonus the author did not even claim.

On the "does this problem actually exist" question: yes, and it is checkable by reading the code rather than trusting a report. I walked getPositionalArgs token by token and reproduced the empty result by hand. This is not a hypothetical hardening PR.

Two reservations, neither blocking:

• The diff removes both -n and --line-number but only tests the short form. Cheap to close, and it is the kind of asymmetry that lets a flag get reintroduced later without a test going red.
• grep -rn pattern dir now reports read_file on dir instead of list_directory, because isRecursive does not recognise combined short flags. Pre-existing, out of scope here, and strictly more conservative than the old "report nothing" — but someone should file it.

The one thing I could not settle from the diff is whether a Read deny rule actually engages end-to-end on the newly-reported operation. The unit test pins the extractor; it does not pin the enforcement path. That is named in the review with the lane that would close it.

Approving on the merits. CI has not finished on this commit — approval is deferred until it lands green on 1bda9676a2b7f13f5a534a01b3b6114f4dc696c8, and will be withheld if anything goes red or the head moves.

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at 1bda9676a2b7f13f5a534a01b3b6114f4dc696c8 · re-run with @qwen-code /triage}

Released in v0.26.7.