@qwen-code /review

@qwen-code /triage

@qwen-code /review

@qwen-code /triage

Verification report — PR 12221 fix(core): recognize sed quiet aliases as read-only

Verdict: merge-ready — 72/72 scripted assertions passed, 0 unexpected failures.
Verified head aaba62f0258b2daa83580f2bc6fa7f419666b71c (fork dvd233/qwen-code, branch fix/12215-sed-long-options); base / true merge-base 34246c66068dc496e937adc05b01661e66bf1e1f. The production change is a 2-line deletion in packages/core/src/utils/shell-safety-rules.ts (blob 1ff179c8 at head, 3402bcb1 at base); the control arm of every A/B below is a rebuild whose source file hash-matches the base blob byte-for-byte, so the arms differ by nothing but this hunk.

1. Central claim and the A/B

Central claim. sed --quiet and sed --silent are GNU sed's documented aliases of -n (confirmed against the real binary, §2), so read-only scripts using them must classify as read-only exactly like -n, which means (a) no confirmation dialog via ShellTool.getDefaultPermission() (packages/core/src/tools/shell.ts:2191, the AST path) and (b) concurrency-safe dispatch via isToolCallConcurrencySafe() (packages/core/src/core/coreToolScheduler.ts:1432, the legacy path). At base a generic long-option guard short-circuited both to unknown before the allowlist could be consulted.

Control fidelity. Both arms are the same worktree rebuilt from a source file whose git hash-object equals the respective commit blob (3402bcb1… base, 1ff179c8… head); git status showed that file as the only difference. Internal node_modules/@qwen-code/* links were realpath-asserted to resolve inside the worktree, so no arm could load the other's code.

All 12 rows that moved carry a literal --quiet or --silent token; 0 non-alias rows moved (H4-B4). The head arm reproduces bit-identically across two independent builds (43/43 rows, H6-G9). Witness: 04-ab-dialog-and-concurrency-flip.png, and at the TUI level 01-e2e-head-auto-approved-no-dialog.png vs 02-e2e-control-confirmation-dialog.png.

Blast-radius census (pure classifier). 6,156 distinct sed argument lists (every long/short option GNU sed 4.9 documents, hostile near-miss spellings, three option positions, two-option combinations, -- terminator forms) run through both commit blobs: 304 verdict deltas, 304 carry an alias, 0 do not; shapes unknown→read-only ×206 and unknown→write ×98 (the latter is a precision gain, never a laxing). Alias↔-n substitution parity: 752 substitutions, 0 divergences at head; the same parity held on only 454/752 at base, proving the parity claim is load-bearing rather than vacuous. Witness: 03-census-6156-arglists-all-deltas-carry-alias.png.

Real-environment E2E (the user-visible surface). Identical scratch project, identical prompt, identical --approval-mode default (the host's global settings are yolo, which would mask everything, so the mode was pinned explicitly):

2. Ground truth from the real GNU sed binary

The PR's premise is that --quiet/--silent are aliases of -n; that was verified from the tool itself, not from documentation: 16 scripts × 3 spellings produced byte-identical stdout and exit codes (H2-G1), and 48 real executions left the scratch directory byte-identical (H2-G2). The filesystem oracle is not blind: all 5 intended writes (-i, w, s///w, W, -i.bak) were detected (H2-G3). Critically, --quiet does not defang a writing script — all 5 writers still wrote when prefixed with --quiet (H2-G4) — which is exactly why the classifier must keep them unsafe, and H1-S1/H3-X1/X2 show it does: no command any arm called read-only ever touched the disk, on either arm.

3. Corrections

None required. The linked issue #12215's root-cause account (the guard at the SAFE_SED_OPTION consumer short-circuits --quiet/--silent to unknown; the allowlist entries were unreachable) matched my measurements exactly, including its claim that --line-length=80 was already read-only at base. The bot review rounds on this PR disclose that no finding was ever independently verified (all carried [unverified]), so there was no earlier substantive round to carry forward; this is the first measurement-backed round.

4. Findings

Ordered by severity. None are blocking; all are either scope/completeness observations or pre-existing conditions proven identical on both arms.

F1 — Suggestion (scope): the PR bundles an undeclared, unrelated change.
packages/cli/src/commands/review/lib/narrow-diff.integration.test.ts gains maxRetries: 5, retryDelay: 100 on the afterAll rmSync (commit test(cli): retry transient review repo cleanup). It is not mentioned anywhere in the PR description, touches a different package and subsystem, and addresses Windows teardown flakiness. It is benign — 32/32 green here (H6-G8) and inert on Linux — but the retry path it targets (EPERM/EBUSY) is Windows-only and no runner in this round or in CI exercised it, the same gap the bot's round-3 comment disclosed. Recommend splitting it into its own PR or declaring it in the description; as it stands a reviewer of "recognize sed quiet aliases" is silently also reviewing review-command teardown policy.
Repro: git diff 34246c66..aaba62f0 -- packages/cli/src/commands/review/lib/narrow-diff.integration.test.ts

F2 — Observation (pre-existing, both arms identical): the two SAFE_SED_OPTION copies have drifted.
shell-safety-rules.ts uses /^(?:-[nElrsuz]|--(?:quiet|silent|line-length(?:=.*)?))$/; shellReadOnlyChecker.ts:92 uses /^(?:-[nErsuz]|--(?:quiet|silent))$/ (no l, no --line-length). Measured consequence: sed --line-length=80 'l' f and sed -l 80 'l' f skip the confirmation dialog (AST says read-only) while the scheduler refuses to run them concurrently (legacy says not) — two inconsistent decisions about one command. Identical on control and head (H4-B7), so not introduced here, but it is the same "two copies of one predicate" hazard this PR's fix had to thread through both. Worth a follow-up that unifies the copies.
Repro: node h3-product-paths.mjs <arm> rows sed --line-length=80 'l' file.txt / sed -l 80 'l' file.txt (assertion X9).

F3 — Observation (pre-existing): isCommandNeedsPermission() has no production caller.
Repo-wide grep finds only its own unit test (shell-utils.test.ts:1310,1315). The real dialog decision is ShellTool.getDefaultPermission() → evaluatePermissionFlow() L3 (packages/core/src/core/permissionFlow.ts:72). The function's name invites exactly the misreading I initially made while wiring this harness. Dead-code candidate; harmless today.
Repro: grep -rn isCommandNeedsPermission --include='*.ts' packages | grep -v test

F4 — Observation (pre-existing, over-conservative, safe direction): sibling shapes still prompt.
sed's y/// (transliterate) is absent from SAFE_SED_COMMAND, so sed -n 'y/ab/AB/' f prompts despite being read-only; likewise the long forms --separate, --unbuffered, --null-data, --regexp-extended prompt while their short forms (-s -u -z -E) are allowlisted. Same class of over-conservatism as the bug this PR fixes, one command down; unchanged by this PR and out of its stated scope. Recorded so the next alias PR has the list.
Repro: node h3-product-paths.mjs head rows tagged preExisting / the --separate etc. rows.

F5 — Observation: GNU sed accepts long-option abbreviations the allowlist rejects.
Real sed 4.9 accepts --qui and --sil (getopt_long unambiguous abbreviation) and rejects --s as ambiguous; the anchored allowlist rejects all of them, so they prompt. Safe direction (over-conservative). Noted because any future "accept abbreviations" change must re-run the write-shape matrix of §2 — an abbreviation that expands to a writer would otherwise be admitted by the alias parity argument.
Repro: sed --qui 'p' file.txt; echo $? → 0; classifier verdict unknown.

F6 — Observation (test completeness, non-blocking): 2 of the PR's 8 new assertions do not discriminate this change.
Reverting the fix (source = base blob) kills 6 of the 8 new assertions — the 4 it.each cases in shellAstParser.test.ts and the 2 read-only expects in shellReadOnlyChecker.test.ts — each with a behavioural expected X to be Y message (H5-M3/M4). The 2 surviving ones are the toBe(false) expects for sed --quiet/--silent 'w output.txt' inside the legacy rejects sed with write command block: at base the verdict is unknown, which also maps to false, so they pass with or without the fix (H5-M6/M7, corroborated by the control arm's own measurement). Classification: redundant defence, not a coverage gap and not dead code — they still catch a future regression that made the alias writable. No action required; noted so the kill-count is not over-read as "all 8 prove the diff".
Repro: node h5-mutation-matrix.mjs (matrix in 05-mutation-matrix-5-killed-1-survivor.png).

5. Not covered

• macOS / Windows. This host is Linux/aarch64. The F1 retry path (EPERM/EBUSY on rmSync) is Windows-specific and was exercised by no runner here or in CI.
• Root integration-tests suite (no-sandbox CLI E2E) not run; the PR's own CI covers it and the targeted suites plus the real-TUI E2E above cover the changed surface.
• Non-GNU sed (busybox, BSD/macOS sed) not tested — the PR explicitly scopes to GNU sed, and H2 pins the premise against GNU sed 4.9 only.
• auto approval mode (the LLM classifier that can auto-approve) not exercised; only default was, which is the mode where this classification is the deciding input.
• Per-commit attribution. The PR has 5 commits including two merges from main; I verified the aggregate base..head diff, not each commit individually.
• Full-repo test/lint sweep not run; gates were targeted to the affected workspaces and proven live (planted type error → TS2322 exit 2; planted unused var → no-unused-vars exit 1).
• F4/F5 gaps are measured as observations only; no fix for them was implemented or tested.

6. Methodology

One isolated git worktree at the PR head inside the repo's tmp/, installed with pnpm install --frozen-lockfile (≈6 min, prepare built every package); the control arm was produced by restoring the deleted 2-line guard so the source file's git hash-object equals the base commit's blob, then rebuilding only packages/core (≈64 s), and reverted the same way afterwards (worktree left clean at head). H1 drives both commit blobs directly through Node's native TS type-stripping for the pure-classifier census; H2 executes real sed 4.9 in per-case scratch directories with a sha256+size+name filesystem snapshot oracle and a positive control; H3 imports the built dist and composes the real product functions in the order shell.ts:2191-2219 and coreToolScheduler.ts:1432 use them, executing every command string for real in the same scratch directory; H4 adjudicates the arm pair; H5 diffs per-test vitest JSON between head and the reverted-fix mutant; H6 re-asserts every gate, the reproducibility check and the two TUI captures from their recorded artifacts; H7 sums the assertions. The TUI A/B ran the real built CLI in tmux at 190×46 with --approval-mode default against a scratch project outside the repo, frames captured with tmux capture-pane -e and rasterised by scripts/verify-capture.mjs. Raw logs, harness sources and JSON results are in this run's artifact directory (tmp/pr12221-verify-20260926-220346/); every number quoted above is reproduced by node h7-aggregate.mjs and the per-harness logs.

Evidence

Hosted on the orphan branch assets-pr12221 of wenshao/qwen-code under pr12221/r1/ (commit 13f9f2ce). Every blob sha was verified against the local git hash-object before its URL was referenced here.

01-e2e-head-auto-approved-no-dialog.png

HEAD (aaba62f0), real TUI, --approval-mode default: sed --quiet '1,3p' file.txt executes immediately (✓ Shell …) with no confirmation dialog. The status bar reads "Ask permissions", proving the mode is default, not yolo.

02-e2e-control-confirmation-dialog.png

CONTROL (source file = base blob 3402bcb1), same command, same scratch project, same mode: the 4-option confirmation dialog blocks execution. This is the user-visible bug the PR removes.

03-census-6156-arglists-all-deltas-carry-alias.png

H1 census over 6,156 distinct sed argument lists: all 304 base→head verdict deltas carry a literal --quiet/--silent token (0 non-alias); alias↔-n substitution parity holds 752/752 at head vs 454/752 at base.

04-ab-dialog-and-concurrency-flip.png

H4 A/B adjudication: dialog-free 4 → 16 and concurrency-safe 2 → 14 (+12/−0 each); the 12 writing commands still ask on both arms and every one of them really wrote to disk.

05-mutation-matrix-5-killed-1-survivor.png

H5 vacuity check: reverting the fix (source = base blob) kills 5/5 of the load-bearing new tests with behavioural expected X to be Y messages; one legacy block survives as redundant defence, explained by measurement.

Local maintainer verification round 1 · Linux/aarch64 · GNU sed 4.9 · Node v24.14.0 · harnesses and raw logs in tmp/pr12221-verify-20260926-220346/

@qwen-code /triage

Verification report — PR 12221, round 2 (macOS, real TUI, non-GNU sed)

Verdict: merge-ready — unchanged from round 1. This round deliberately does not repeat round 1 (Linux/aarch64, GNU sed 4.9). It closes the gaps that round 1 listed as not covered: macOS, non-GNU sed, auto approval mode, end-to-end concurrency, and freshness against today's main. Every production-path claim of the PR held on macOS. I found one pre-existing macOS gap (F1 below). This PR did not introduce it, it does not block this PR, and a one-line candidate fix is measured below.

Arms. The PR head is aaba62f0, unchanged since round 1. CONTROL = main @ a480bc78. PR ARM = local merge of the head into that main (ffa0794a), which is the tree that would actually land. The two arms differ by exactly the PR diff: I compared the changed lines of main..merge with merge-base..head and they are identical. After a full pnpm install + build + npm run bundle in each worktree:

• The compiled packages/core/dist differs in exactly 2 files: the commit stamp and the 3 deleted guard lines in shell-safety-rules.js.
• The guard regex ^--(?!line-length(?:=|$)) is present in 1 bundle chunk on CONTROL and in 0 on the PR ARM.
• Each CLI resolves its own worktree's core (realpath checked).

main moved to c662f78b during the run. The 2 new commits touch only web-shell and managed-agent, and the merge is still clean.

What this round adds

Results

E1 — the user-visible fix, on macOS (default mode, GNU sed first on PATH). Same prompt, same scripted model, --approval-mode default (footer shows Ask permissions). CONTROL stops at Allow execution of: 'sed'?. The PR ARM runs sed --quiet '1,3p' notes.txt directly and shows alpha / beta / gamma. → 01-…png

E2 — stock macOS sed (BSD). BSD sed has no long options. On the PR ARM the command is auto-approved and then fails harmlessly: sed: illegal option -- -, exit 1, and notes.txt is unchanged (sha1 equals the seeded content, no new files). CONTROL shows the dialog; approving it would lead to the same failure. For macOS users without GNU sed, the change therefore only removes a prompt in front of a command that cannot succeed anyway. That is not a safety issue. → 02-…png

E3 — "lost concurrency-safe classification" demonstrated end to end. One model turn emits two parallel calls, sed --quiet p fifo-a and sed --quiet p fifo-b. A writer releases both FIFOs only if both readers are attached at the same moment, and after 4 s falls back to releasing a lone reader as SERIAL. The tool output therefore states what the scheduler did:

• CONTROL: SERIAL / SERIAL. fifo-b's reader attached at 6691 ms, only after fifo-a was released at 6628 ms.
• PR ARM: CONCURRENT / CONCURRENT. The readers attached 22 ms apart.
• Positive control (sed -n): CONCURRENT on both arms, so the rig can see parallelism on main.

→ 03-…png

E4 — auto mode: the classifier fast path. A read-only verdict lets a call skip the LLM safety classifier (coreToolScheduler.ts L5, unless classifyAllShell). The scenario ran three sequential steps.

The PR saves one classifier round trip per alias call, and a writing alias still reaches the classifier. The scripted classifier always answered shouldBlock:false, so this measures whether the classifier is consulted, not how a real model would judge the call. → 04-…png

E5 — census on macOS against real binaries. 5,184 distinct sed command lines were classified by each arm's built core, using the same two predicates the product uses:

• dialog-free: hasShellSubstitution → stripShellWrapper → isShellCommandReadOnlyASTInDirectory, as in shell.ts getDefaultPermission();
• concurrency-safe: isShellCommandReadOnly, as in isToolCallConcurrencySafe().

All 2,057 lines that either arm auto-approves were then executed via bash -c under both BSD sed and GNU sed 4.10. Each run used a fresh directory containing a pre-existing out file, with a sha256 snapshot taken before and after.

730 of the 732 newly dialog-free lines changed nothing under either sed. The other 2 are the F1 spellings. GNU sed 4.10 on macOS also reproduces the PR's premise: --quiet and --silent produce byte-identical output to -n. → 05-…png

Tests and build on macOS. shellAstParser.test.ts + shellReadOnlyChecker.test.ts: 782/782, which matches the PR description. narrow-diff.integration.test.ts: 32/32 in 3 runs on each arm. The retry path it adds (EPERM/EBUSY from rmSync) is not triggered on macOS, so this proves the test is inert here, not that the retry works. The merged tree's full workspace build (tsc --build for all 25 packages) had 0 TypeScript errors.

Findings

F1 — Pre-existing, macOS, not introduced by this PR; non-blocking. The classifier parses sed's -l the GNU way, but stock macOS sed is BSD, so a w script can pass as the -l value.
classifySedCommandSafety treats -l as taking a value (SED_VALUE_OPTIONS; shell-safety-rules.ts if (/^(?:-l|--line-length)$/.test(arg) && !args[++i])). In BSD sed, -l is a bare flag (line-buffered output), and the next word is the script.

For sed -l wimportant.txt 1p:

• The classifier sees -l <value> plus the script 1p → read-only → allow, no dialog.
• BSD sed sees the script wimportant.txt → it truncates important.txt while compiling the script, before it even finds that the input 1p is missing (exit 1).

This reproduced in the real TUI in default mode on both arms: important.txt went from 35 bytes to 0 with no prompt. GNU sed does not write in these shapes (0 in E5), so Linux users are unaffected. The legacy concurrency checker never accepted -l, so the concurrency-safe count is 0 on both arms.

The PR's relationship to it: head adds exactly 2 spellings to the exposed set (sed -l wout --quiet p and --silent). Their -n twin, sed -l wout -n p, is already auto-approved and writing on main, so the PR adds spellings, not a new class.

Candidate fix (measured, not part of this PR): require a numeric value after -l/--line-length.

-    if (/^(?:-l|--line-length)$/.test(arg) && !args[++i]) return 'unknown';
+    if (/^(?:-l|--line-length)$/.test(arg) && !/^\d+$/.test(args[++i] ?? '')) return 'unknown';

• Same census, head vs head + fix: BSD writes through auto-approval 13 → 0, GNU stays 0 → 0.
• Cost: 247 lines return to prompting, and every one of them is -l <non-numeric>. The 792 numeric--l lines and 3,978 lines without -l have 0 verdict changes. Concurrency verdicts: 1,130 → 1,130.
• shellAstParser + shellReadOnlyChecker + shell-utils + tools/shell tests: 1317/1317 with the fix applied.

This was measured at the classifier and unit level only, not end to end. I recommend a separate follow-up issue/PR rather than widening this one. → 06-…png
Repro on stock macOS: printf 'x\n' > f; sed -l wf 1p; wc -c f → 0 f.

F2 — Observation (round-1 F1, still open): the undeclared narrow-diff.integration.test.ts retry hunk. It is green on macOS (above), but its Windows-only target path is still exercised only by the author's own Windows run. The triage bot's approval makes the same point. It is a hygiene issue, not a defect.

Merge mechanics. The template-gate CHANGES_REQUESTED from 09-18 has been superseded by the bot's APPROVED (09-26 17:06 UTC). reviewDecision is now REVIEW_REQUIRED, so a maintainer approval is what remains. On the head, CI shows every executed check passing. Test (macos-latest), Test (windows-latest) and Integration Tests (CLI, No Sandbox) were skipped by CI; this round covers the first of those locally.

Not covered

• Windows: not run here. Round 1 did not run it either; only the author's report covers it.
• Other sed implementations (busybox, toybox): not tested. Only BSD /usr/bin/sed and GNU 4.10 were executed.
• A real classifier model in auto mode: the scripted classifier always allowed. E4 measures whether the classifier is consulted, not its judgement.
• The F1 candidate fix end to end: measured at the classifier and unit level only; no TUI run with a rebuilt bundle.
• Root integration-tests suite: not run (CI no-AK passed on the head).
• Per-commit attribution: I verified the aggregate diff merged onto main, not each of the 5 commits.

Methodology

Two fresh git worktrees in a scratch directory, pnpm install --frozen-lockfile (whose prepare builds every package), then npm run bundle. The TUI runs used env -i, a dedicated tmux socket, an isolated QWEN_HOME with its own settings.json (selectedType: openai), and OPENAI_BASE_URL pointed at a scripted model built on the repo's integration-tests/fake-openai-server.ts. That model:

• tells main-session turns apart from side queries by the declared tool surface;
• detects classifier requests by the forced respond_in_schema tool plus the classifier system prompt;
• logs the command each classifier call reviewed.

sed flavour was chosen per run with a PATH-front symlink (gsed or /usr/bin/sed). Frames were captured with tmux capture-pane -e and rasterised with scripts/verify-capture.mjs. The census imports each arm's packages/core/dist directly. The candidate fix was measured on a copy of the PR ARM's compiled core, and its unit run patched the PR ARM's source temporarily; that file was restored afterwards and blob-checked (1ff179c8).

Evidence

Hosted on wenshao/qwen-code branch assets-pr12221 under pr12221/r2/. The URLs are anchored to commit 8e2eef35, a fast-forward on top of round 1's 13f9f2ce. Every URL was fetched back, returned 200, and its bytes match the local file.

01 — E1: default mode, GNU sed: dialog on CONTROL vs. auto-run on PR ARM

02 — E2: stock macOS BSD sed: PR ARM auto-runs; BSD rejects the long option; nothing written

03 — E3: FIFO rendezvous: SERIAL on CONTROL, CONCURRENT on PR ARM, -n control concurrent on both

04 — E4: auto mode: classifier requests 2 → 1; the writing alias is still reviewed

05 — E5: 5,184-line census, executed under BSD and GNU sed

06 — F1: pre-existing BSD -l truncation (identical on both arms) and the measured candidate fix

-    if (/^(?:-l|--line-length)$/.test(arg) && !args[++i]) return 'unknown';
+    if (/^(?:-l|--line-length)$/.test(arg) && !/^\d+$/.test(args[++i] ?? '')) return 'unknown';

Local maintainer verification round 2 · macOS 26 arm64 · BSD sed + GNU sed 4.10 · Node v24.18.1 · CONTROL main@a480bc78 vs PR ARM main+aaba62f0 = ffa0794a