diff --git a/src/discover/README.md b/src/discover/README.md index e6121a04f6..076934c2cf 100644 --- a/src/discover/README.md +++ b/src/discover/README.md @@ -39,6 +39,21 @@ When a hook sends `cargo fmt --all && cargo test 2>&1 | tail -20`: **Result**: `rtk cargo fmt --all && rtk cargo test 2>&1 | tail -20`. Bash handles the `&&` and `|` at execution time — each `rtk` invocation is a separate process. +## Shared Lexer Toolkit + +`lexer.rs` is layer 1 of RTK's command parsing (raw string → quote/operator-aware tokens and words) — layer 2, already-split argv → flags/values, is `core/arg_tokenizer.rs`. It's not private to this module: `hooks/permissions.rs`, `hooks/mod.rs`, and `main.rs`'s `rtk proxy` all build on it instead of re-scanning commands themselves. + +| Function | Purpose | Used outside `discover/` by | +|---|---|---| +| `tokenize(cmd)` | Full shell-syntax tokens: quotes, escapes, operators, pipes, redirects, shellisms | — | +| `tokenize_with_newlines(cmd)` | Like `tokenize`, plus a `\n` `Operator` token per unquoted newline (a lone `\r` stays glued to its word, matching real bash) | — | +| `shell_split(cmd)` | Quote-aware split into argv-ready words (quotes stripped, escapes resolved) | `hooks/mod.rs::is_claude_hook_command`, `main.rs`'s `rtk proxy '...'` | +| `split_for_permissions(cmd)` | Segments a compound command for the **permission gate** — deliberately the most conservative of three segmenters (see its doc comment for the full comparison table) | `hooks/permissions.rs::check_command_with_rules` | +| `split_on_operators(cmd, stop_at_pipe)` | Segments for classification only — not safe for permission/security decisions | `registry.rs::split_command_chain` | +| `contains_unattestable_construct(cmd)` | True for command/process substitution or a file-target redirect — constructs the permission gate can't decompose and must never auto-allow | `hooks/permissions.rs::check_command_with_rules` | + +The permission gate, discover/analytics classification, and rewrite each segment compound commands (`&&`, `;`, `|`, background `&`, subshells) slightly differently on purpose — the gate must never under-segment (a hidden command could evade a deny rule), while rewrite and analytics only need to reproduce or classify the command's actual shape. Don't reuse `split_on_operators` or `rewrite_compound`'s segmenting for a permission/security decision; use `split_for_permissions`. + ## How History Analysis Works `rtk discover` reads Claude Code JSONL session files. Each file contains `tool_use`/`tool_result` pairs for every command the LLM ran. The module: diff --git a/src/discover/lexer.rs b/src/discover/lexer.rs index 9a3805599d..ae8603d7a9 100644 --- a/src/discover/lexer.rs +++ b/src/discover/lexer.rs @@ -22,18 +22,82 @@ pub struct ParsedToken { pub offset: usize, } +/// How `tokenize_inner` treats `\n`/`\r`. +#[derive(Clone, Copy, PartialEq, Eq)] +enum NewlineMode { + /// Ordinary characters, no Operator tokens. + None, + /// `\n`, and the `\r` of a CRLF pair, are Operator boundaries; a lone + /// `\r` stays glued to its word — real bash's behavior. + Bash, + /// Like `Bash`, but a lone `\r` is a boundary too. Only + /// `split_for_permissions` uses this, to stay maximally conservative. + Conservative, +} + pub fn tokenize(input: &str) -> Vec { - tokenize_inner(input, false) + tokenize_inner(input, NewlineMode::None) } /// Like [`tokenize`] but emits a `\n` operator token for each newline that /// sits outside quotes. Newlines inside quoted strings stay part of their /// argument, so callers can use the emitted offsets as safe line-split points. pub fn tokenize_with_newlines(input: &str) -> Vec { - tokenize_inner(input, true) + tokenize_inner(input, NewlineMode::Bash) +} + +/// Applies one character's effect on quote state, mirroring bash: only the +/// quote char that opened a span closes it. Shared by `tokenize_inner`, +/// `shell_split`, and `registry.rs::QuoteScan` so they can't drift. +pub(crate) fn advance_quote_state(quote: Option, c: char) -> Option { + match (quote, c) { + (None, '\'' | '"') => Some(c), + (Some(q), c) if c == q => None, + (q, _) => q, + } +} + +/// Bash's default `$IFS` is exactly space/tab/newline — not Rust's +/// `char::is_whitespace()`, which wrongly includes non-IFS Unicode +/// whitespace like NBSP. Shared with `permissions.rs::command_matches_pattern`. +pub(crate) fn is_word_boundary_whitespace(c: char) -> bool { + matches!(c, ' ' | '\t' | '\n') +} + +/// True if `bytes[i..]` starts a CRLF pair. Shared by `tokenize_inner` and +/// `registry.rs::rewrite_multiline_block`'s raw-newline parity check. +pub(crate) fn is_crlf_at(bytes: &[u8], i: usize) -> bool { + bytes.get(i) == Some(&b'\r') && bytes.get(i + 1) == Some(&b'\n') +} + +/// Merges `tokenize()` tokens that are directly adjacent in `cmd` (no gap) +/// into single words — e.g. `*.yml` tokenizes as `Shellism("*")` + +/// `Arg(".yml")` but is one bash word. For callers that only need "was there +/// a space here", not full shell-operator awareness. +pub(crate) fn coalesce_words<'a>(cmd: &'a str, tokens: &[ParsedToken]) -> Vec<(&'a str, usize)> { + let mut words = Vec::new(); + let mut run_start: Option = None; + let mut run_end: usize = 0; + + for tok in tokens { + if let Some(start) = run_start { + if tok.offset != run_end { + words.push((&cmd[start..run_end], start)); + run_start = None; + } + } + if run_start.is_none() { + run_start = Some(tok.offset); + } + run_end = tok.offset + tok.value.len(); + } + if let Some(start) = run_start { + words.push((&cmd[start..run_end], start)); + } + words } -fn tokenize_inner(input: &str, emit_newline: bool) -> Vec { +fn tokenize_inner(input: &str, newline_mode: NewlineMode) -> Vec { let mut tokens = Vec::new(); let mut current = String::new(); let mut current_start: usize = 0; @@ -61,19 +125,11 @@ fn tokenize_inner(input: &str, emit_newline: bool) -> Vec { continue; } - if let Some(q) = quote { - if c == q { - quote = None; - } - current.push(c); - byte_pos += char_len; - continue; - } - if c == '\'' || c == '"' { - quote = Some(c); - if current.is_empty() { + if quote.is_some() || c == '\'' || c == '"' { + if quote.is_none() && current.is_empty() { current_start = byte_pos; } + quote = advance_quote_state(quote, c); current.push(c); byte_pos += char_len; continue; @@ -253,7 +309,12 @@ fn tokenize_inner(input: &str, emit_newline: bool) -> Vec { }); current_start = byte_pos; } - '\n' | '\r' if emit_newline => { + c @ ('\n' | '\r') + if newline_mode != NewlineMode::None + && (c == '\n' + || newline_mode == NewlineMode::Conservative + || is_crlf_at(input.as_bytes(), byte_pos)) => + { flush_arg(&mut tokens, &mut current, current_start); tokens.push(ParsedToken { kind: TokenKind::Operator, @@ -263,7 +324,7 @@ fn tokenize_inner(input: &str, emit_newline: bool) -> Vec { byte_pos += char_len; current_start = byte_pos; } - c if c.is_whitespace() => { + c if is_word_boundary_whitespace(c) => { flush_arg(&mut tokens, &mut current, current_start); byte_pos += c.len_utf8(); current_start = byte_pos; @@ -353,8 +414,28 @@ fn redirect_has_file_target(tokens: &[ParsedToken], i: usize) -> bool { } } -/// Like [`split_on_operators`] but also breaks on newline, background `&`, and -/// subshell `( ... )`, and truncates each segment at its first redirect. +/// Segments `cmd` for the **permission gate** (`permissions.rs::check_command_with_rules`): +/// every segment this returns is independently checked against deny/ask/allow +/// rules, so this is deliberately the most paranoid of the three compound-command +/// segmenters in this codebase — see [`split_on_operators`] (analytics/discovery +/// classification) and `registry.rs::rewrite_compound`'s inline token walk (actual +/// rewrite) for the other two, which intentionally segment the same kind of input +/// differently: +/// +/// | | here (permission gate) | [`split_on_operators`] (analytics) | `rewrite_compound` (rewrite) | +/// |---|---|---|---| +/// | `&&` / `\|\|` / `;` | splits | splits | splits | +/// | `\|` | always splits | stops at first `\|` | pipeline handled specially | +/// | background `&` | splits (Shellism boundary) | does not split | splits | +/// | `( ... )` grouping | splits (Shellism boundary) | does not split | does not split standalone | +/// | trailing redirect | truncates the segment | kept | kept (rewritten output preserves it) | +/// | lone `\r` (no following `\n`) | splits | does not split | does not split | +/// +/// Like [`split_on_operators`] but also breaks on newline, background `&`, +/// subshell `( ... )`, and a lone `\r` (`NewlineMode::Conservative`), and +/// truncates each segment at its first redirect — deliberately conservative +/// so a hidden command can't evade the gate by hiding behind a construct +/// another segmenter would leave intact. /// Callers must still gate on [`contains_unattestable_construct`] first. pub fn split_for_permissions(cmd: &str) -> Vec<&str> { let trimmed = cmd.trim(); @@ -362,7 +443,7 @@ pub fn split_for_permissions(cmd: &str) -> Vec<&str> { return vec![]; } - let tokens = tokenize_inner(trimmed, true); + let tokens = tokenize_inner(trimmed, NewlineMode::Conservative); let mut results = Vec::new(); let mut seg_start: usize = 0; let mut seg_end: Option = None; @@ -395,13 +476,15 @@ pub fn split_for_permissions(cmd: &str) -> Vec<&str> { results } -/// Split a shell command on operators (`&&`, `||`, `;`) and optionally pipes (`|`), -/// respecting quoted strings via the lexer. +/// Split a shell command on operators (`&&`, `||`, `;`) and optionally pipes +/// (`|`), quote-aware. `stop_at_pipe: true` returns only segments before the +/// first `|` (rewrite's left-side-only case); `false` splits through pipes +/// too (permission checking, every segment validated). /// -/// When `stop_at_pipe` is true, returns only segments before the first `|` -/// (used by command rewriting — only the left side of a pipe gets rewritten). -/// When false, splits through pipes too (used by permission checking — -/// every segment must be validated). +/// For classification only — unlike [`split_for_permissions`] this never +/// splits on background `&`/`( ... )` or truncates at a redirect (see that +/// function's comparison table), so it must not be repurposed for +/// permission/security decisions. pub fn split_on_operators(cmd: &str, stop_at_pipe: bool) -> Vec<&str> { let trimmed = cmd.trim(); if trimmed.is_empty() { @@ -455,48 +538,83 @@ pub fn strip_quotes(s: &str) -> String { s.to_string() } -pub fn shell_split(input: &str) -> Vec { - let mut tokens = Vec::new(); - let mut current = String::new(); - let mut chars = input.chars().peekable(); - let mut in_single = false; - let mut in_double = false; +/// Turns a coalesced word's raw text (quotes/escapes still literal, as +/// `tokenize()` preserves them) into argv-ready text: quote chars that +/// open/close a span are stripped, backslash escapes resolved. +fn resolve_word_text(raw: &str) -> String { + let mut result = String::new(); + let mut chars = raw.chars().peekable(); + let mut quote: Option = None; while let Some(c) = chars.next() { match c { - '\\' if !in_single => { + // Inside double quotes bash only lets `\` escape `$`, `` ` ``, `"`, + // `\` or a newline; before anything else it is a literal character. + // That is what keeps a quoted Windows path (`"C:\Program Files"`) + // intact instead of eating its separators. + '\\' if quote == Some('"') => match chars.peek() { + Some('$' | '`' | '"' | '\\' | '\n') => { + if let Some(next) = chars.next() { + result.push(next); + } + } + _ => result.push('\\'), + }, + '\\' if quote.is_none() => { if let Some(next) = chars.next() { - current.push(next); + result.push(next); } } - '\'' if !in_double => { - in_single = !in_single; - } - '"' if !in_single => { - in_double = !in_double; - } - ' ' | '\t' if !in_single && !in_double => { - if !current.is_empty() { - tokens.push(std::mem::take(&mut current)); + '\'' | '"' => { + // advance_quote_state leaves `quote` unchanged when `c` is the + // "wrong" quote char for the current span (e.g. a `'` while + // inside `"..."`) — that's literal text, not a toggle. + let new_quote = advance_quote_state(quote, c); + if new_quote == quote { + result.push(c); + } else { + quote = new_quote; } } - _ => { - current.push(c); - } + _ => result.push(c), } } - if !current.is_empty() { - tokens.push(current); - } + result +} - tokens +/// Quote-aware split of a single shell command into argv-ready words: quotes +/// stripped, backslash escapes resolved — for callers that hand the result +/// straight to `Command::new`/exec or compare it against literal words +/// (`hooks/mod.rs::is_claude_hook_command`, `rtk proxy` arg-splitting). +pub fn shell_split(input: &str) -> Vec { + coalesce_words(input, &tokenize(input)) + .into_iter() + .map(|(raw, _)| resolve_word_text(raw)) + .collect() } #[cfg(test)] mod tests { use super::*; + #[test] + fn test_coalesce_words_merges_adjacent_tokens() { + let cmd = "golangci-lint --config *.yml run"; + let words: Vec<&str> = coalesce_words(cmd, &tokenize(cmd)) + .into_iter() + .map(|(w, _)| w) + .collect(); + assert_eq!(words, vec!["golangci-lint", "--config", "*.yml", "run"]); + } + + #[test] + fn test_coalesce_words_preserves_offsets() { + let cmd = "a *.yml b"; + let words = coalesce_words(cmd, &tokenize(cmd)); + assert_eq!(words, vec![("a", 0), ("*.yml", 2), ("b", 8)]); + } + #[test] fn test_simple_command() { let tokens = tokenize("git status"); @@ -1117,6 +1235,22 @@ mod tests { ); } + #[test] + fn test_shell_split_keeps_backslash_in_double_quotes() { + assert_eq!( + shell_split(r#""C:\Program Files\rtk.exe" hook codex"#), + vec![r"C:\Program Files\rtk.exe", "hook", "codex"] + ); + } + + #[test] + fn test_shell_split_double_quote_escapes_only_bash_specials() { + assert_eq!( + shell_split(r#"echo "a\$b" "a\"b" "a\\b" "a\nb""#), + vec!["echo", "a$b", "a\"b", r"a\b", r"a\nb"] + ); + } + #[test] fn test_shell_split_unclosed_quote() { let result = shell_split("echo 'hello"); @@ -1141,6 +1275,33 @@ mod tests { assert_eq!(shell_split("a b c"), vec!["a", "b", "c"]); } + #[test] + fn test_shell_split_coalesces_unquoted_glob_next_to_quoted_segment() { + // An unquoted metacharacter directly adjacent to a quoted segment + // (no space between them) must stay one word — the same + // token-coalescing gap that split_token_spans needed for golangci-lint, + // now exercised through shell_split's output shape (quotes stripped). + assert_eq!( + shell_split(r#"echo *.yml"quoted end""#), + vec!["echo", "*.ymlquoted end"] + ); + } + + #[test] + fn test_shell_split_splits_on_embedded_newline() { + // Bash's default $IFS is space/tab/newline, so an embedded unquoted + // `\n` is a word boundary, same as space or tab. + assert_eq!(shell_split("a\nb"), vec!["a", "b"]); + } + + #[test] + fn test_shell_split_does_not_split_on_nbsp() { + // U+00A0 (NBSP) has Unicode `White_Space = Y` despite not being part + // of bash's $IFS — char::is_whitespace() would wrongly treat it as a + // word boundary. `a\u{a0}b` must stay one word, matching real bash. + assert_eq!(shell_split("a\u{a0}b"), vec!["a\u{a0}b"]); + } + #[test] fn test_strip_quotes_double() { assert_eq!(strip_quotes("\"hello\""), "hello"); @@ -1309,6 +1470,27 @@ mod tests { ); } + #[test] + fn test_split_perms_lone_cr_still_splits() { + assert_eq!( + split_for_permissions("git status\rrm -rf ~"), + vec!["git status", "rm -rf ~"] + ); + // A CRLF pair still splits exactly once, not twice. + assert_eq!( + split_for_permissions("git status\r\ncargo build"), + vec!["git status", "cargo build"] + ); + } + + #[test] + fn test_split_perms_lone_cr_inside_quotes_not_split() { + assert_eq!( + split_for_permissions("echo 'foo\rbar'"), + vec!["echo 'foo\rbar'"] + ); + } + #[test] fn test_split_perms_background_ampersand() { assert_eq!( @@ -1361,5 +1543,28 @@ mod tests { assert_eq!(newline_ops("git status\ngit log"), 1); assert_eq!(newline_ops("echo 'line1\nline2'"), 0); assert_eq!(newline_ops("git status\r\ngit log"), 2); + // A lone `\r` (no following `\n`) is not a separator → no newline operator. + assert_eq!(newline_ops("git status\rgit log"), 0); + } + + #[test] + fn test_lone_cr_is_not_a_word_boundary() { + // Bash's default $IFS is space/tab/newline, never CR: a bare `\r` with no + // following `\n` stays glued into its surrounding word instead of splitting + // it, matching how real bash tokenizes `git statusgit log`. + let args: Vec = tokenize("git status\rgit log") + .into_iter() + .map(|t| t.value) + .collect(); + assert_eq!(args, vec!["git", "status\rgit", "log"]); + } + + #[test] + fn test_crlf_in_plain_tokenize_keeps_cr_glued_to_word() { + let args: Vec = tokenize("git status\r\ngit log") + .into_iter() + .map(|t| t.value) + .collect(); + assert_eq!(args, vec!["git", "status\r", "git", "log"]); } } diff --git a/src/discover/registry.rs b/src/discover/registry.rs index 6469b178d8..ca17c255a1 100644 --- a/src/discover/registry.rs +++ b/src/discover/registry.rs @@ -6,8 +6,8 @@ use std::path::Path; use std::sync::LazyLock; use super::lexer::{ - shell_split, split_on_operators, tokenize, tokenize_with_newlines, ParsedToken, PipeKind, - TokenKind, + advance_quote_state, coalesce_words, is_crlf_at, shell_split, split_on_operators, tokenize, + tokenize_with_newlines, ParsedToken, PipeKind, TokenKind, }; use super::rules::{IGNORED_EXACT, IGNORED_PREFIXES, RULES}; @@ -426,25 +426,11 @@ fn golangci_flag_takes_separate_value(arg: &str, flag: &str) -> bool { true } -fn split_token_spans(cmd: &str) -> Vec<(&str, usize, usize)> { - let mut tokens = Vec::new(); - let mut start = None; - - for (idx, ch) in cmd.char_indices() { - if ch.is_whitespace() { - if let Some(token_start) = start.take() { - tokens.push((&cmd[token_start..idx], token_start, idx)); - } - } else if start.is_none() { - start = Some(idx); - } - } - - if let Some(token_start) = start { - tokens.push((&cmd[token_start..], token_start, cmd.len())); - } - - tokens +/// Quote-aware word splitting for golangci-lint's flag/value parsing: "was +/// there a space here", not shell syntax — an unquoted glob like `*.yml` +/// must stay one word rather than split on `*`. +fn split_token_spans(cmd: &str) -> Vec<(&str, usize)> { + coalesce_words(cmd, &tokenize(cmd)) } /// Normalize absolute binary paths: `/usr/bin/grep -rn foo` → `grep -rn foo` (#485) @@ -640,8 +626,10 @@ const BLOCK_KEYWORDS: &[&str] = &[ struct QuoteScan<'a> { bytes: &'a [u8], i: usize, - in_single: bool, - in_double: bool, + // Same `Option` model `tokenize_inner`/`shell_split` use, driven by + // the shared `advance_quote_state` — not an independently-maintained pair + // of bools, so this can't drift from the lexer's own quote handling. + quote: Option, } impl<'a> QuoteScan<'a> { @@ -649,13 +637,12 @@ impl<'a> QuoteScan<'a> { Self { bytes: s.as_bytes(), i: 0, - in_single: false, - in_double: false, + quote: None, } } fn balanced(&self) -> bool { - !self.in_single && !self.in_double + self.quote.is_none() } } @@ -666,15 +653,13 @@ impl Iterator for QuoteScan<'_> { while self.i < self.bytes.len() { let i = self.i; let b = self.bytes[i]; - if b == b'\\' && !self.in_single { + if b == b'\\' && self.quote != Some('\'') { self.i += 2; continue; } - let item = (i, b, self.in_single, self.in_double); - match b { - b'\'' if !self.in_double => self.in_single = !self.in_single, - b'"' if !self.in_single => self.in_double = !self.in_double, - _ => {} + let item = (i, b, self.quote == Some('\''), self.quote == Some('"')); + if b == b'\'' || b == b'"' { + self.quote = advance_quote_state(self.quote, b as char); } self.i += 1; return Some(item); @@ -852,9 +837,16 @@ fn rewrite_multiline_block( return None; } - // The lexer emits one newline token per `\r` and per `\n` (CRLF = two - // tokens), so the parity check must count both bytes individually. - let raw_breaks = cmd.chars().filter(|c| matches!(c, '\n' | '\r')).count(); + // The lexer emits a newline token for each `\n` and for the `\r` of a CRLF + // pair (CRLF = two tokens), but NOT for a lone `\r` (a bare CR is not a + // separator). Count exactly that set here, so the parity check flags only + // newlines the lexer swallowed via quote state — never a lone CR. + let bytes = cmd.as_bytes(); + let raw_breaks = bytes + .iter() + .enumerate() + .filter(|&(i, &b)| b == b'\n' || is_crlf_at(bytes, i)) + .count(); if raw_breaks != newline_offsets.len() { // Every newline swallowed by quote state with quotes balanced at EOF // is one logical command (a multi-line commit message), not a hidden @@ -1031,7 +1023,11 @@ fn rewrite_pipeline_final_stage( }) } -/// Rewrite a compound command (with `&&`, `||`, `;`, `|`) by rewriting each segment. +/// Rewrite a compound command (with `&&`, `||`, `;`, `|`) by rewriting each +/// segment. Third of three compound-command segmenters — see the comparison +/// table on [`crate::discover::lexer::split_for_permissions`]. Deliberately +/// less conservative than that gate: standalone `(`/`)` isn't a segment +/// boundary, and redirects are preserved verbatim rather than truncated. fn rewrite_compound( cmd: &str, excluded: &[ExcludePattern], @@ -1503,6 +1499,105 @@ mod tests { super::rewrite_command(cmd, excluded, &[]) } + // Three compound-command segmenters look at the same kind of input for + // different, deliberate purposes — split_for_permissions (the permission + // gate, most conservative), split_on_operators/split_command_chain + // (analytics/discovery classification), and rewrite_compound's inline + // token walk (actual rewrite). See the comparison table on + // split_for_permissions's doc comment. These tests pin today's actual, + // intentionally-divergent behavior for each, side by side, so a future + // edit to any one of them that accidentally drifts its policy fails here + // immediately instead of silently diverging further from the other two. + mod segmenter_consistency { + use super::{rewrite_command_no_prefixes, split_command_chain}; + use crate::discover::lexer::split_for_permissions; + + #[test] + fn background_ampersand() { + let cmd = "git status & rm -rf ~"; + // Permission gate: splits on background `&` — both sides checked independently. + assert_eq!(split_for_permissions(cmd), vec!["git status", "rm -rf ~"]); + // Analytics: does not split on `&` at all (only Operator/Pipe kinds). + assert_eq!(split_command_chain(cmd), vec!["git status & rm -rf ~"]); + // Rewrite: does split on `&` (each side is its own rtk-rewrite + // candidate), but only "git status" is a known rtk command family — + // "rm -rf ~" has no rtk equivalent, so it's left unprefixed, not + // because it wasn't segmented. + assert_eq!( + rewrite_command_no_prefixes(cmd, &[]), + Some("rtk git status & rm -rf ~".into()) + ); + } + + #[test] + fn subshell_grouping() { + let cmd = "(git status; cargo build)"; + // Permission gate: strips `(`/`)` as boundaries — both commands checked cleanly. + assert_eq!( + split_for_permissions(cmd), + vec!["git status", "cargo build"] + ); + // Analytics: does not treat `(`/`)` as boundaries, only splits on `;` — + // the parens stay glued to the segment text on each side. + assert_eq!( + split_command_chain(cmd), + vec!["(git status", "cargo build)"] + ); + // Rewrite: same non-splitting-on-parens behavior. The leading `(` + // glued to "git status" defeats rewrite_segment's own command + // matching (it no longer starts with "git"), so that side is left + // unprefixed; the trailing `)` glued after "cargo build" does not + // defeat matching on that side, so it gets prefixed. This asymmetry + // is a real, existing quirk of gluing grouping chars to segment + // text rather than stripping them — pinned here, not fixed here. + assert_eq!( + rewrite_command_no_prefixes(cmd, &[]), + Some("(git status; rtk cargo build)".into()) + ); + } + + #[test] + fn pipe_then_and() { + let cmd = "git status | grep x && cargo build"; + // Permission gate: always splits on `|` — every stage checked independently. + assert_eq!( + split_for_permissions(cmd), + vec!["git status", "grep x", "cargo build"] + ); + // Analytics: split_command_chain stops entirely at the first `|`, + // discarding everything after it (including the later `&&` clause) — + // it only needs to classify what's in front of the pipe. + assert_eq!(split_command_chain(cmd), vec!["git status"]); + // Rewrite: pipelines are handled specially (rewrite_pipeline_final_stage), + // and clauses after the pipeline are still walked and rewritten. + assert_eq!( + rewrite_command_no_prefixes(cmd, &[]), + Some("git status | rtk grep x && rtk cargo build".into()) + ); + } + + #[test] + fn redirect_in_segment() { + let cmd = "git status 2>&1 && cargo build"; + // Permission gate: truncates the segment at its first redirect. + assert_eq!( + split_for_permissions(cmd), + vec!["git status", "cargo build"] + ); + // Analytics: keeps the redirect attached to the segment. + assert_eq!( + split_command_chain(cmd), + vec!["git status 2>&1", "cargo build"] + ); + // Rewrite: also keeps the redirect — rewritten output must + // reproduce the command's actual shape, redirect included. + assert_eq!( + rewrite_command_no_prefixes(cmd, &[]), + Some("rtk git status 2>&1 && rtk cargo build".into()) + ); + } + } + mod multiline_blocks { use super::rewrite_command_no_prefixes; @@ -1552,6 +1647,38 @@ mod tests { ); } + #[test] + fn test_lone_cr_inside_quotes_rewrites_as_one_command() { + // A `\r` inside quotes is part of the argument, not a line break, + // so the block is one logical command with a single prefix. + assert_eq!( + rewrite_command_no_prefixes("git commit -m 'subject\rin body'", &[]), + Some("rtk git commit -m 'subject\rin body'".into()) + ); + } + + #[test] + fn test_lone_cr_line_gets_a_single_prefix() { + // A bare `\r` is not a line break: bash keeps `git log` glued to the + // preceding word, so the whole first line is one command and takes + // one prefix. Only the `\n` starts a new line. + assert_eq!( + rewrite_command_no_prefixes("git status\rgit log\ngit diff", &[]), + Some("rtk git status\rgit log\nrtk git diff".into()) + ); + } + + #[test] + fn test_quoted_lone_cr_does_not_bail_out_the_block() { + // The raw-break parity check counts `\n` and the `\r` of a CRLF pair + // only. Counting a quoted lone `\r` too would make the block look + // like it hid a line from the lexer and send it through unrewritten. + assert_eq!( + rewrite_command_no_prefixes("echo 'a\rb'\ngit log -3", &[]), + Some("echo 'a\rb'\nrtk git log -3".into()) + ); + } + #[test] fn test_unbalanced_swallowed_newline_passes_through() { assert_eq!( @@ -3702,6 +3829,40 @@ mod tests { )); } + #[test] + fn test_classify_golangci_lint_with_quoted_value_flag_before_run() { + // A quoted global-flag value containing a space (`--config "a path/x.yml"`) + // must not be split at the space inside the quotes — split_token_spans + // (whitespace-only, quote-blind) used to mis-split this into "\"a" and + // "path/x.yml\"", which made parse_golangci_run_parts miss `run` entirely. + assert!(matches!( + classify_command(r#"golangci-lint --config "a path/x.yml" run ./..."#), + Classification::Supported { + rtk_equivalent: "rtk golangci-lint run", + .. + } + )); + } + + #[test] + fn test_classify_golangci_lint_with_unquoted_glob_value_flag_before_run() { + // An UNQUOTED global-flag value containing a shell metacharacter + // (`--config *.yml`) must also stay one word. Routing this through the + // full shell tokenize() (rather than a quote-aware but syntax-blind + // word splitter) regressed this: tokenize() treats `*` as its own + // Shellism token even outside quotes, splitting "*.yml" into "*" and + // ".yml" and desyncing the flag-value-skip loop, which then reads + // ".yml" where it expects "run" and misclassifies the whole command as + // Unsupported. + assert!(matches!( + classify_command("golangci-lint --config *.yml run ./..."), + Classification::Supported { + rtk_equivalent: "rtk golangci-lint run", + .. + } + )); + } + #[test] fn test_classify_golangci_lint_with_inline_config_flag_before_run() { assert!(matches!( diff --git a/src/hooks/permissions.rs b/src/hooks/permissions.rs index a23fd2d307..84135d7277 100644 --- a/src/hooks/permissions.rs +++ b/src/hooks/permissions.rs @@ -3,7 +3,7 @@ use super::constants::{ SETTINGS_JSON, SETTINGS_LOCAL_JSON, }; use crate::core::stream::exec_capture; -use crate::discover::lexer::split_for_permissions; +use crate::discover::lexer::{is_word_boundary_whitespace, split_for_permissions}; use serde_json::Value; use std::path::PathBuf; @@ -383,8 +383,16 @@ pub(crate) fn extract_bash_pattern(rule: &str) -> &str { /// - `* suffix`, `pre * suf` → glob matching where `*` matches any sequence of characters /// - `pattern` → exact match or prefix match (cmd must equal pattern or start with `{pattern} `) pub(crate) fn command_matches_pattern(cmd: &str, pattern: &str) -> bool { - let cmd_norm = cmd.split_whitespace().collect::>().join(" "); - let pattern_norm = pattern.split_whitespace().collect::>().join(" "); + // Shares the lexer's word-boundary definition rather than + // str::split_whitespace(), so a bare `\r` in `cmd` never collapses into a space. + let normalize = |s: &str| { + s.split(is_word_boundary_whitespace) + .filter(|part| !part.is_empty()) + .collect::>() + .join(" ") + }; + let cmd_norm = normalize(cmd); + let pattern_norm = normalize(pattern); let cmd = cmd_norm.as_str(); let pattern = pattern_norm.as_str(); @@ -946,6 +954,32 @@ mod tests { ); } + #[test] + fn test_lone_cr_hidden_command_not_auto_allowed() { + let allow = vec!["git status".to_string()]; + assert_eq!( + check_command_with_rules("git status\rrm -rf ~", &[], &[], &allow), + PermissionVerdict::Default + ); + } + + #[test] + fn test_lone_cr_does_not_collapse_to_space_in_pattern_match() { + assert!(!command_matches_pattern( + "git status\rrm -rf ~", + "git status" + )); + } + + #[test] + fn test_lone_cr_segment_still_denied() { + let deny = vec!["rm:*".to_string()]; + assert_eq!( + check_command_with_rules("git status\rrm -rf ~", &deny, &[], &[]), + PermissionVerdict::Deny + ); + } + #[test] fn test_background_hidden_command_denied() { let deny = vec!["rm:*".to_string()];