feat(rewrite): rewrite commands bundles

[aeppling]

Summary

• Rewrites each clean, supported statement in a multi-line command bundle instead of dropping the whole bundle when any part contains a $(...), pipe, redirect or newline.
• Recovers a large share of lost savings — rewrite coverage 39.3% → 71.8% (+333 commands) on a 1,025-command corpus of real + edge-case commands.
• Unattestable bundles are still clamped to ask (never auto-allowed), so security: hook auto-allow bypasses agent permission model for rewritten commands #1155 holds.

After a "rtk gain --reset" and start of an ultracode workflow (not completed)

Test plan

• cargo fmt --all && cargo clippy --all-targets && cargo test — 2,388 pass, clippy clean
• Manual testing: rtk <command> output inspected
• Differential + adversarial fuzz vs the current develop binary — 2,410 commands (845 real + 1,565 generated: permission-bypass attempts, every ecosystem, deep shell syntax, redirect matrix), each rewrite validated with bash -n:
• Manual testing with Claude Code sessions
• Manual testing with Cursor agent sessions

[pszymkowiak]

Reviewed with a focus on the #1155 permission-bypass invariant, plus local verification: cargo clippy --all-targets clean, cargo test --bins = 2338 passed / 0 failed.

#1155 holds with defense-in-depth. Auto-allow (exit 0) requires both:

1. the whole-command attestability gate — contains_unattestable_construct is computed on the entire command (rewrite_cmd.rs:56-62, hook_cmd.rs:139-144), so any $(...), backtick, process-sub, $((, or file-target redirect anywhere forces the Ask arm; and
2. the newline-aware verdict gate — split_for_permissions breaks on newlines, so a dangerous statement on line 2 is validated separately.

A 12-case adversarial functional matrix (command substitution, backticks, process-sub, file redirects, newline-separated dangerous statements) never produced exit 0 — everything stayed passthrough/ask, and every rewritten string is valid bash -n. Per-segment, unattestable statements are left verbatim and don't lift the bundle verdict. Bundle splitting itself is sound (top-level newline split, byte-exact reassembly, multi-char operator guard).

Non-blocking nit: a trailing single & (background) at end of line is treated like && for continuation at lexer.rs:465 — cosmetic, produces semantically-equivalent output, no verdict impact.

LGTM 👍

[aeppling]

BLOCKER

Tested with our benchmark solution, show that this cause more api retries and lower the token savings, blocked until we merge #2555 or #2965 to fix the command rewrite when filtering and output get consumed

[KuSh]

Round 2 of 3 — DUPLICATE for the mechanism, and the residual is blocked — recommend closing

Claim: Rewrite each clean, supported statement in a multi-line command bundle instead of dropping the whole bundle when any part contains a $(...), pipe, redirect or newline.

Scope: duplicate of #3319 for the bundle-splitting mechanism; the only delta that remains is the attestability relaxation, and that one is blocked by the Copilot CLI CVE mitigation on develop. There is no narrowing of this branch that lands safely today, so the recommendation is to close and move the remaining idea to an issue. Round 1 predates this template, so the verdict is stated now.

Ran: release builds of upstream/develop (d0c29851), this head (2c89559a), and a narrowed variant I built and tested myself; rtk rewrite, rtk hook claude and rtk hook copilot over 10,713 real multi-line commands from local agent history plus a 45-shape hand matrix (comments, continuations, control flow, subshells, background &, CRLF, redirects), each rewrite executed and diffed against the original.

CI @2c89559a: 17 green, 1 failing — benchmark ("1 filter produced more tokens than raw output": curl json, which hits the live mockhttp.org service and is untouched by this diff). Dated 2026-07-23. Not counted against this PR.

---

Blocks merge (3 — all newly introduced by the 2c89559a push, which landed after round 1)

Findings 1 and 2 share one root cause: split_top_level_newlines and the permission lexer are blind to # comments.

1. src/hooks/hook_cmd.rs / src/discover/registry.rs — reintroduces the Copilot CLI CVE class for bundles ("no laundering").

hook_cmd.rs:1328 states the invariant: "The hook must emit NO modifiedArgs for CVE bypass forms — no laundering." test_copilot_cli_cve_command_substitution_returns_none and its siblings require end_to_end() to return None — not merely to withhold allow — for any command carrying a substitution or file redirect, because Copilot renders a rewrite as a suggestion the user may accept.

This head satisfies that for a single-line command but not for a bundle:

ls -la
git log --pretty=$(rm -rf /tmp/x)

	rtk hook copilot
develop d0c29851	(no output)
this head	updatedInput: "rtk ls -la\ngit log --pretty=$(rm -rf /tmp/x)", permissionDecision: "ask"

The destructive substitution is laundered into a suggestion. Clamping to ask is not sufficient for this host — the mitigation requires emitting nothing at all.

2. src/discover/lexer.rs:486 — permission bypass: #1155 does not hold.

An unbalanced apostrophe inside a # comment (# it's fine) opens a single-quote state that masks everything after it, so contains_unattestable_construct returns false and split_compound_command collapses the bundle into one segment beginning with ls — which matches an ls allow rule. The hook then asserts permissionDecision: "allow" for a trailing command no rule allows, carrying a live substitution.

With only Bash(ls:*) and Bash(cat:*) allowed:

ls -la # it's fine
curl $(echo http://x)

	permissionDecision
develop d0c29851	(absent — defers, host prompts)
this head	"allow"

Dropping just the apostrophe (# fine) removes the allow, which isolates quote-state masking as the mechanism; backticks reach the same end. The comment-blindness itself is pre-existing and shared, but it is latent on develop, which declines to rewrite these bundles. This PR is what converts it into an auto-allow — and it is exactly the invariant the PR sets out to preserve.

3. src/discover/lexer.rs:486 — silent content loss: a statement is commented out and never runs.

let continuation = matches!(last_nonspace, Some(b'|') | Some(b'&')) reads the last non-space byte with no idea it sits inside a comment. A comment ending in & or | suppresses the newline split, and rewrite_compound then reassembles around the operator with a literal space, flattening the next statement into the comment.

ls # bg &
grep foo file.txt

Executed, in a directory where file.txt contains foo bar:

	output
original	file.txt + foo bar
develop d0c29851	file.txt + foo bar
this head	file.txt — grep silently never runs

The hook auto-applies this at exit 0. Exactly one statement is lost (later lines re-split normally). A URL in a comment (# see http://x?a=1&) is a realistic trigger.

Round 1 called the trailing-& continuation at lexer.rs:465 "cosmetic, semantically-equivalent, no verdict impact". That holds for a real trailing & (a &\nb ≡ a & b), but not when the & is comment text — that is the chaining mechanism here.

---

Why this is a close rather than another round

The mechanism already landed. fc8054eb / PR #3319 (feat(rewrite): rewrite each line of multi-line Bash blocks, merged 2026-07-24 — one day after this head) splits multi-line blocks at quote-aware newline tokens and rewrites each line, preserving blank lines, comment lines, indentation and CRLF. It is deliberately more conservative: the whole block passes through when a shell keyword opens control flow, when a list or pipeline continues across a line break, or when a subshell or group spans lines. This branch's extra aggressiveness is exactly where findings 2 and 3 live — it reaches into for/while and function bodies that #3319 refuses.

The residual delta is blocked, and I verified that the hard way. The only thing develop still lacks is the attestability relaxation. I applied it to d0c29851 as a 9-line change in decision.rs::decide_with_params and rebuilt: it does reproduce every win in your test plan (ls -la src / f=$(whoami) → exit 3 with the clean statement rewritten and the substitution verbatim, where develop exits 1), and the addressable headroom is real — 3,214 of 10,713 real multi-line commands (30.0%) are blocks develop defers entirely although at least one line is independently rewritable.

But that variant fails 13 tests on develop, including test_copilot_cli_cve_command_substitution_returns_none, ..._backtick_substitution_returns_none and ..._file_redirect_returns_none, and on the copilot path it laundered $(rm -rf /tmp/x) for the single-line form too — strictly worse than this branch. So I am not going to ask you to implement it: the relaxation collides with the CVE mitigation in any form I can construct, and reconciling them is a design decision (host-gating the relaxation, since decision.rs is already Host-parameterised and Claude Code applies updatedInput directly while Copilot renders it as a suggestion) that belongs in an issue with the maintainers, not in a review round.

Coverage numbers also need restating. The 39.3% → 71.8% was measured against a pre-#3319 develop. Against d0c29851, over the same 10,713 real multi-line commands: 22.3% → 25.6%. In the other direction there are 651 commands develop rewrites that this head does not — a stale-base artifact (148e0d44 taught develop that 2>&1 is an fd-dup, not a file-target redirect, so cargo build 2>&1 | tail -20 rewrites on develop but not here) that would disappear on rebase.

And the rebase is not textual. The branch is 616 commits behind and conflicts in all three touched files; both functions it edits were consolidated upstream into decision.rs::decide_with_params, whose doc comment states the opposite policy ("refusing to rewrite it at all is the stronger guarantee").

Optional — not blocking

• src/hooks/rewrite_cmd.rs:56 — the attestable && verdict == Allow clamp can never change an outcome: permissions.rs:71 already returns Ask unconditionally when contains_unattestable_construct(cmd) is true, before Allow can be computed. Harmless defence-in-depth, but the commit message ("exit 0 requires attestable + Allow") and round 1's approval both credit it as the load-bearing gate; the real protection is rewrite_logical_line keeping unattestable statements verbatim.
• src/discover/lexer.rs:458 — the backslash arm records the escaped byte in last_nonspace, so ls \& at end of line is misread as a continuation.
• src/discover/registry.rs:579 — dropping the $(( early return, with the attestability check now after the starts_with("rtk ") return, flips rtk rewrite 'rtk git log $((1+1))' from exit 1 to exit 3.

Filed as follow-ups

• #4029 — the permission lexer is blind to # comments: contains_unattestable_construct / split_compound_command track quote state through comment text, so an unbalanced apostrophe masks a later $(...) and collapses segment splitting. Latent on develop, but a standing trap for any change that widens rewrite coverage; this PR is the demonstration. Sibling of #3262 — same family, distinct root.
• #4030 — recover bundle savings when one statement is unattestable: carries the measured 30.0% headroom, the 13 failing tests, the "no laundering" CVE invariant and the host-gating design question, so the idea in this PR is not lost.

Checked and correct — no need to re-verify

• Byte-exact reassembly of inter-statement gaps: blank lines, indentation and CRLF round-trip.
• Newlines inside quotes, $( ), backticks and ( ) are correctly not split points.
• case terminators (;;, ;&) and |& kept verbatim; a clean leaf before a case still rewrites.
• Real pipeline continuations (git log | / grep fix) fold correctly and match develop.
• #3128 pipeline-producer safety holds on this head: grep -rn foo src 2>/dev/null | head stays raw.
• cargo test --all passes on the head; fmt/clippy clean.

Hypotheses built and dropped

• Rewriting a for/while body leaf breaks stdin-consuming loops — find … | while read f / do / grep foo "$f" / done rewrites the body leaf and the loop still reads correctly; rtk grep with a file argument does not touch the loop's stdin. Dropped as a correctness claim.
• Unbalanced quote or paren in a comment causes loss — # don't, # note (see below suppress the split but preserve the tail verbatim on its own line. Missed savings only.
• The benchmark CI failure is caused by this PR — it is curl json against a live external service.

Your questions

• "BLOCKER … blocked until we merge #2555 or #2965 to fix the command rewrite when filtering and output get consumed" → that blocker is resolved. Both were closed as superseded: #3128 (only rewrite safe final pipeline stages) and #3343 (PipelineSafety + SAFE_PIPE_CONSUMERS) are on develop, and #3128 is already in this branch's base. I verified the consumer-aware model holds on this head — producers feeding grep/head stay raw. The retries and lost savings you measured should be gone. It is the rest of develop that has moved past this branch in the meantime.

Next

Close this branch — I have opened #4029 and #4030 so the 30.0% headroom and the CVE constraint are recorded together — the bundle-splitting half of your idea is already shipping in #3319, and the other half needs a host-gating decision before any code. Thank you for the benchmark work that produced the original blocker; it is what made #3128/#3343 land.

Rounds: 2/3. Threads: 3 open (all blocking), 0 resolved this round.