Repository navigation
fix(hook): split compound commands on newlines for rewriting - #3274
breisnerlopez wants to merge 2 commits into
Conversation
The hook rewriter split command lines on `&&`, `||`, `;` and `|` but not on bare newlines, so a multi-line command such as `cd /worktree\ngrep foo` was treated as a single unrewritable segment and escaped rewriting entirely (0% rtk coverage for cd-prefixed / worktree workflows). Tokenize with newlines enabled in `rewrite_compound` and re-emit `\n` verbatim (like `;`). Also: include `\n`/`\r` in the `has_compound` fast-path so a command that starts with `rtk` still rewrites later lines, and collapse CRLF to a single LF separator. Command-substitution safety is unaffected: the hook/permission layer defers on `$(...)`/backticks before rewriting (regression test added), and the allow-per-segment permission verdict already splits on newlines.
|
This is the highest-impact of my open PRs on the token side: without it, compound commands split across newlines ( |
|
Friendly nudge — this one's been open ~10 days with green CI and CLA signed. @aeppling @KuSh, when you have a spare cycle, would you mind taking a look? It's the highest token-impact of my open PRs: without it, compound commands split across newlines ( For batch review, my other single-focus PRs on |
| let normalized: Cow<str> = if cmd.contains('\r') { | ||
| Cow::Owned(cmd.replace("\r\n", "\n").replace('\r', "\n")) | ||
| } else { | ||
| Cow::Borrowed(cmd) | ||
| }; | ||
| let cmd = normalized.as_ref(); |
There was a problem hiding this comment.
This seems a bit too broad and could replace content inside quoted arguments, for example grep 'a\rb' file.txt with a raw CR byte inside the single quotes.
Suggestion: collapse CR/CRLF only on the newline tokens the tokenizer already isolates, since it already skips quoted content correctly.
It would also be worth adding a test for that.
There was a problem hiding this comment.
Good catch, thank you — fixed exactly as you suggested in 33d4823. Instead of the global replace(), the collapse now happens on the newline token the tokenizer already isolates: tokenize_inner consumes a \r\n pair (and a lone \r) as a single Operator("\n") token, so CRLF still yields exactly one separator with no spurious blank clause, and a CR/LF inside quotes is never touched — it's consumed by the quote branch before the newline arm is ever reached. The global normalization and its now-orphan Cow import are gone.
Added the test you asked for plus a couple more:
test_rewrite_compound_cr_inside_quotes_not_a_separator— raw CR and CRLF inside single/double quotes stay a single rewritten segment.- On the shared-tokenizer / permission-gating side:
test_split_perms_crlf,test_split_perms_cr_inside_quotes_not_split, and a CRLF case intest_attestable_subshell_and_separators, confirming a command hidden after a CRLF is still split into its own segment while a quoted CR can't smuggle one past the per-segment check.
cargo fmt --check + clippy --all-targets clean, full suite green (2511/0). CI's rerunning on the push.
Review feedback on rtk-ai#3274: rewrite_compound normalized CRLF/lone-CR to LF across the whole command before tokenizing, which also rewrote a raw CR *inside* quoted arguments (e.g. `grep 'a\rb' file.txt`) into a clause separator, corrupting them. Handle it where the tokenizer already isolates newlines instead: tokenize_with_newlines now consumes a `\r\n` pair (and a lone `\r`) as a single newline Operator token, so CRLF still yields exactly one separator with no spurious blank clause, while a CR/LF inside quotes is left untouched (it never reaches the newline arm). Drop the global replace and the now-unused Cow import. Tests: raw CR / CRLF inside quotes stays one segment in both rewrite_compound and the permission layer (split_for_permissions, contains_unattestable_construct).
|
Hi @breisnerlopez, unfortunately fc8054e already solves the exact same problem your PR targets. The only thing your PR still adds is support for bare You have two options:
Sorry I didn’t catch that sooner |
|
Thanks @KuSh — makes sense, fc8054e covers the main case cleanly and I hadn't seen it land. Going with option 2: closing this in favor of a small, focused follow-up for just the lone- |
2 participants
Problem
The hook rewriter splits a command line on
&&,||,;and|before rewriting each clause, but not on a bare newline. So a multi-line command such asis treated as a single unrewritable segment and escapes rewriting entirely —
grep/git/find/… on the following lines never becomertk grep/… This is common incd <dir>-prefixed and worktree-oriented workflows, where it drops rtk coverage on those commands to ~0%.Fix
tokenize_with_newlines(thin wrapper over the existingtokenize_inner(_, true)) and use it inrewrite_compound, so a bare\nis treated as a clause separator like;. The\nis re-emitted verbatim in the rejoin (no surrounding spaces), preserving the original layout.\n/\rin thehas_compoundfast-path so a command that starts withrtkbut has more lines (rtk gain\nls -la) still falls through and rewrites the later lines.\r\n/lone\rto a single\nseparator so CRLF input doesn't emit a spurious blank line.Safety
Command-substitution safety is unaffected: the permission/hook layer already defers on
$(...)/backticks (and other unattestable constructs) before rewriting, and the allow-per-segment permission verdict already splits on newlines (the existing newline-bypass CVE guards). This change only touches the rewriter, which runs after and independently of the permission verdict — it can't elevate a decision or change how a dangerous command is treated. Verified end-to-end thatgit status\nrm -rf …is byte-identical to before (no auto-allow) and that multi-line$(...)defers with no rewrite.Tests
New unit tests for:
cd-prefixed newline, both-sides rewrite, newline+pipe, trailing newline,rtk-prefixed multi-line, CRLF collapse; plus a hook-level regression test that multi-line command substitution defers.cargo fmt --checkandcargo clippy --all-targetsclean; full suite green.