From d2e046904c5b2c3760945d320cc3a94d150216e3 Mon Sep 17 00:00:00 2001 From: Nicolas Le Cam Date: Tue, 25 Aug 2026 02:08:10 +0200 Subject: [PATCH 01/11] fix(lexer): lone CR is not a word/command boundary anywhere MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bash's default $IFS is space/tab/newline, never a bare CR. The lexer's newline arm already conflated a lone \r with \n/\r\n as a command separator, and its generic whitespace arm independently treated \r as a word boundary too — two places that had to independently get this right and didn't. Fixes both: - tokenize_inner's newline arm now only splits on \n or the \r of a real CRLF pair; a lone \r falls through and stays glued to its word. - tokenize_inner's whitespace arm no longer treats \r as a boundary, via a new shared is_word_boundary_whitespace() predicate. - permissions.rs::command_matches_pattern now normalizes through that same shared predicate instead of str::split_whitespace() (which is \r-inclusive), so an allow rule like "git status" can no longer auto-approve "git status\rrm -rf ~" by collapsing the embedded CR into a space before matching. - registry.rs's raw_breaks parity check updated to match the corrected newline model (counts \n and CRLF's \r, never a lone \r). Regression tests added in lexer.rs and permissions.rs reproducing the exact divergence. --- src/discover/lexer.rs | 46 ++++++++++++++++++++++++++++++++++++++-- src/discover/registry.rs | 13 +++++++++--- src/hooks/permissions.rs | 39 +++++++++++++++++++++++++++++++--- 3 files changed, 90 insertions(+), 8 deletions(-) diff --git a/src/discover/lexer.rs b/src/discover/lexer.rs index 9a3805599d..ea37baa1e3 100644 --- a/src/discover/lexer.rs +++ b/src/discover/lexer.rs @@ -33,6 +33,15 @@ pub fn tokenize_with_newlines(input: &str) -> Vec { tokenize_inner(input, true) } +/// Bash's default `$IFS` is space/tab/newline — never a bare `\r`. Shared by +/// the lexer's own word-splitting and by permission-pattern normalization +/// (`permissions.rs::command_matches_pattern`) so the two can't independently +/// diverge on what counts as whitespace, which is how a lone-CR bug fixed here +/// once reappeared there as a separate, undetected regression. +pub(crate) fn is_word_boundary_whitespace(c: char) -> bool { + c.is_whitespace() && c != '\r' +} + fn tokenize_inner(input: &str, emit_newline: bool) -> Vec { let mut tokens = Vec::new(); let mut current = String::new(); @@ -253,7 +262,13 @@ fn tokenize_inner(input: &str, emit_newline: bool) -> Vec { }); current_start = byte_pos; } - '\n' | '\r' if emit_newline => { + // `\n`, and the `\r` of a CRLF pair, are command separators. A lone `\r` + // (classic-Mac EOL, not followed by `\n`) is NOT: bash treats a bare CR + // as an ordinary character inside a word — `git statusgit log` runs + // as a single command — so gating on it would over-segment and the + // rewriter would mis-join it. A lone `\r` falls through to the `_` arm + // below like any other non-IFS byte, never a word or command boundary. + c @ ('\n' | '\r') if emit_newline && (c == '\n' || chars.peek() == Some(&'\n')) => { flush_arg(&mut tokens, &mut current, current_start); tokens.push(ParsedToken { kind: TokenKind::Operator, @@ -263,7 +278,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; @@ -1361,5 +1376,32 @@ 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() { + // Plain `tokenize()` (emit_newline=false) treats `\n` as ordinary IFS + // whitespace but never `\r` — a `\r` right before it is not a separator, + // so it stays glued to the word it terminates, same as real bash would + // keep a bare CR byte attached to its 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..dde6d566a3 100644 --- a/src/discover/registry.rs +++ b/src/discover/registry.rs @@ -852,9 +852,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' || (b == b'\r' && bytes.get(i + 1) == Some(&b'\n'))) + .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 diff --git a/src/hooks/permissions.rs b/src/hooks/permissions.rs index a23fd2d307..f6ba3f5f13 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,18 @@ 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 definition of a word boundary (`is_word_boundary_whitespace`) + // instead of `str::split_whitespace()`'s Unicode-whitespace notion, so a bare + // `\r` embedded in `cmd` is never collapsed into a space here — matching how + // the lexer itself now treats a lone `\r` as part of the word, not a boundary. + 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 +956,29 @@ mod tests { ); } + #[test] + fn test_lone_cr_hidden_command_not_auto_allowed() { + // `split_for_permissions` correctly treats a lone `\r` (no `\n`) as part + // of one command, not a boundary. Before command_matches_pattern shared + // the lexer's word-boundary definition, its own `split_whitespace()` + // still collapsed the embedded `\r` into a space, so "git status\rrm -rf + // ~" normalized to "git status rm -rf ~" and matched an allow rule for + // "git status" it should never have matched. + 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_background_hidden_command_denied() { let deny = vec!["rm:*".to_string()]; From d5e9a02e77bced0f73e140aa5117e11af1f817de Mon Sep 17 00:00:00 2001 From: Nicolas Le Cam Date: Tue, 25 Aug 2026 02:11:19 +0200 Subject: [PATCH 02/11] refactor(lexer): mutualize the quote/escape state machine tokenize_inner, QuoteScan (registry.rs), and shell_split each independently tracked "am I inside a quote" - QuoteScan's own comment already admitted it was reimplementing "the same model the lexer applies" from scratch. Extract the one rule all three actually encode (an unescaped quote char opens a span if none is open, only the same character closes it) into a single advance_quote_state() primitive, and drive all three from it: - tokenize_inner's Option quote tracking now calls it instead of duplicating the open/close branching inline. - QuoteScan switches its internal (in_single, in_double) bool pair to the same Option model, built on the same primitive - its external (offset, byte, in_single_before, in_double_before) API and all 5 downstream consumers (comment_start, bracket/test-bracket balance, ansi_c_quote_defeats_lexer, quotes_balanced) are unchanged. - shell_split switches from its own bool-pair toggle to the same primitive, as a byproduct simplifying its match arms to one merged case. Behavior-preserving: all 2708 existing tests pass unchanged, including the ~750 lines of lexer.rs quote/escape coverage - a future quote-state correction now only needs to happen once. --- src/discover/lexer.rs | 51 ++++++++++++++++++++++++---------------- src/discover/registry.rs | 25 ++++++++++---------- 2 files changed, 43 insertions(+), 33 deletions(-) diff --git a/src/discover/lexer.rs b/src/discover/lexer.rs index ea37baa1e3..9be1afaab3 100644 --- a/src/discover/lexer.rs +++ b/src/discover/lexer.rs @@ -33,6 +33,21 @@ pub fn tokenize_with_newlines(input: &str) -> Vec { tokenize_inner(input, true) } +/// Applies one character's effect on quote state, mirroring bash: an +/// unescaped `'` or `"` opens a quote span if none is open, and only the same +/// character closes it (a `"` inside a `'...'` span, or vice versa, is just +/// literal text). Shared by the char-based tokenizer (`tokenize_inner`, +/// `shell_split`) and every byte-based line scanner in `registry.rs` +/// (`QuoteScan`), so the different representations of "am I inside a quote" +/// can't independently drift the way they had before this was extracted. +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 space/tab/newline — never a bare `\r`. Shared by /// the lexer's own word-splitting and by permission-pattern normalization /// (`permissions.rs::command_matches_pattern`) so the two can't independently @@ -70,19 +85,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; @@ -474,23 +481,27 @@ 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; + let mut quote: Option = None; while let Some(c) = chars.next() { match c { - '\\' if !in_single => { + '\\' if quote != Some('\'') => { if let Some(next) = chars.next() { current.push(next); } } - '\'' if !in_double => { - in_single = !in_single; - } - '"' if !in_single => { - in_double = !in_double; + '\'' | '"' => { + // 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 { + current.push(c); + } else { + quote = new_quote; + } } - ' ' | '\t' if !in_single && !in_double => { + ' ' | '\t' if quote.is_none() => { if !current.is_empty() { tokens.push(std::mem::take(&mut current)); } diff --git a/src/discover/registry.rs b/src/discover/registry.rs index dde6d566a3..c91925bd1e 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, shell_split, split_on_operators, tokenize, tokenize_with_newlines, + ParsedToken, PipeKind, TokenKind, }; use super::rules::{IGNORED_EXACT, IGNORED_PREFIXES, RULES}; @@ -640,8 +640,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 +651,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 +667,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); From 9df302261764081f25bafd3cd97367a9926d1228 Mon Sep 17 00:00:00 2001 From: Nicolas Le Cam Date: Tue, 25 Aug 2026 02:13:03 +0200 Subject: [PATCH 03/11] refactor(golangci): retire split_token_spans, use tokenize()'s Arg spans split_token_spans was a from-scratch, quote-blind whitespace splitter with a single caller (golangci-lint's global-flag pre-processing). Replace it with the lexer's tokenize(), reusing its already-tracked byte offsets - genuine dedup, and a real fix: a quoted global-flag value containing a space (`--config "a path/x.yml"`) used to get mis-split at the space inside the quotes, which made parse_golangci_run_parts miss the `run` subcommand entirely and fall back to leaving the command unclassified. New regression test covers the quoted-value case; all other existing golangci-lint classification tests pass unchanged. --- src/discover/registry.rs | 46 +++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 26 deletions(-) diff --git a/src/discover/registry.rs b/src/discover/registry.rs index c91925bd1e..1eba2bcc7c 100644 --- a/src/discover/registry.rs +++ b/src/discover/registry.rs @@ -360,15 +360,15 @@ fn strip_golangci_global_opts(cmd: &str) -> String { /// Parse supported golangci-lint invocations with optional global flags before `run`. fn parse_golangci_run_parts(cmd: &str) -> Option> { - let tokens = split_token_spans(cmd); + let tokens = tokenize(cmd); let first = tokens.first()?; - if first.0 != "golangci-lint" && first.0 != "golangci" { + if first.value != "golangci-lint" && first.value != "golangci" { return None; } let mut i = 1; while i < tokens.len() { - let token = tokens[i].0; + let token = tokens[i].value.as_str(); if token == "--" { return None; @@ -377,11 +377,11 @@ fn parse_golangci_run_parts(cmd: &str) -> Option> { if !token.starts_with('-') { if token == "run" { let global_segment = if i > 1 { - cmd[tokens[1].1..tokens[i].1].trim() + cmd[tokens[1].offset..tokens[i].offset].trim() } else { "" }; - let run_segment = cmd[tokens[i].1..].trim(); + let run_segment = cmd[tokens[i].offset..].trim(); return Some(GolangciRunParts { global_segment, run_segment, @@ -426,27 +426,6 @@ 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 -} - /// Normalize absolute binary paths: `/usr/bin/grep -rn foo` → `grep -rn foo` (#485) /// Only strips if the first word contains a `/` (Unix path). fn strip_absolute_path(cmd: &str) -> String { @@ -3708,6 +3687,21 @@ 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_inline_config_flag_before_run() { assert!(matches!( From 38ed86475b30094a951ed18b40efc8a9fa85225c Mon Sep 17 00:00:00 2001 From: Nicolas Le Cam Date: Tue, 25 Aug 2026 02:16:17 +0200 Subject: [PATCH 04/11] docs(lexer): document and pin the three compound-command segmenters split_for_permissions (permission gate), split_on_operators / split_command_chain (analytics/discovery classification), and rewrite_compound's inline token walk (actual rewrite) all segment the same kind of compound-command string, but deliberately differently: pipe-stop behavior, whether background `&` or `(`/`)` grouping counts as a boundary, and whether a trailing redirect gets truncated all diverge across the three. That's intentional - the permission gate must stay the most conservative - but it was undocumented and untested as a set, so a future edit to any one of them could silently drift further from the other two without anything catching it. - Cross-referencing doc comments on all three functions, including a comparison table on split_for_permissions. - A new segmenter_consistency test module in registry.rs pinning today's actual, verified output for all three across four representative inputs (background &, subshell grouping, pipe+&&, redirect-in-segment) - including a real quirk this surfaced: in `(git status; cargo build)`, the rewritten output only prefixes "cargo build", not "git status", because the leading `(` glued to the first segment defeats rewrite_segment's own command matching while the trailing `)` on the second segment doesn't. Pinned as documented existing behavior, not changed here. No behavior change; pure documentation and test coverage. --- src/discover/lexer.rs | 29 +++++++++- src/discover/registry.rs | 112 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 1 deletion(-) diff --git a/src/discover/lexer.rs b/src/discover/lexer.rs index 9be1afaab3..da83c85140 100644 --- a/src/discover/lexer.rs +++ b/src/discover/lexer.rs @@ -375,8 +375,26 @@ fn redirect_has_file_target(tokens: &[ParsedToken], i: usize) -> bool { } } +/// 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) | +/// /// Like [`split_on_operators`] but also breaks on newline, background `&`, and -/// subshell `( ... )`, and truncates each segment at its first redirect. +/// subshell `( ... )`, 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(); @@ -424,6 +442,15 @@ pub fn split_for_permissions(cmd: &str) -> Vec<&str> { /// (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). +/// +/// Its only current caller (`registry.rs::split_command_chain`, used for +/// analytics/discovery classification) passes `stop_at_pipe: true`, and unlike +/// [`split_for_permissions`] never splits on background `&` or `( ... )` +/// grouping, and never truncates at a redirect — see the comparison table on +/// [`split_for_permissions`] for the full picture across all three +/// compound-command segmenters. That's fine for classification (it only needs +/// to identify supported commands, not defend against a hidden one), but this +/// function must not be repurposed for permission/security decisions as-is. pub fn split_on_operators(cmd: &str, stop_at_pipe: bool) -> Vec<&str> { let trimmed = cmd.trim(); if trimmed.is_empty() { diff --git a/src/discover/registry.rs b/src/discover/registry.rs index 1eba2bcc7c..f2c83f98df 100644 --- a/src/discover/registry.rs +++ b/src/discover/registry.rs @@ -1017,6 +1017,19 @@ fn rewrite_pipeline_final_stage( } /// Rewrite a compound command (with `&&`, `||`, `;`, `|`) by rewriting each segment. +/// +/// This walk is the third of three compound-command segmenters in this +/// codebase — see the comparison table on +/// [`crate::discover::lexer::split_for_permissions`] (the permission gate's +/// segmenter) for the full picture. This one also splits on background `&` +/// (`TokenKind::Shellism if tok.value == "&"` below), but — unlike +/// `split_for_permissions` — does **not** treat standalone `(`/`)` as a +/// segment boundary (only bails out entirely via `has_opaque_grouping` when a +/// pipe and a grouping char coexist) and does not truncate segments at a +/// redirect (redirects are preserved verbatim in the rewritten output, by +/// design). These differences are deliberate: the permission gate must stay +/// the most conservative of the three, this function's job is to reproduce +/// the command's actual shape, not to defend against it hiding something. fn rewrite_compound( cmd: &str, excluded: &[ExcludePattern], @@ -1488,6 +1501,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; From 01113b6948840e6d972d4240b0bdc2bb31d362eb Mon Sep 17 00:00:00 2001 From: Nicolas Le Cam Date: Tue, 25 Aug 2026 02:25:59 +0200 Subject: [PATCH 05/11] fix(golangci): restore quote-aware word splitting, mutualize CRLF check Two follow-ups from /code-review high on this branch: 1. parse_golangci_run_parts's switch to the full shell tokenize() (Phase 3) fixed the intended quoted-value bug but introduced a real regression: tokenize() splits unquoted shell metacharacters (*, ?, `, (, ), {, }, !) into their own tokens even outside quotes, so an unquoted glob value like `--config *.yml` desynced the flag-skip loop and got the whole command misclassified as Unsupported. split_token_spans's actual job - "was there a space here", not full shell syntax - is genuinely different from tokenize()'s, so it should not have been replaced by it. Restored split_token_spans as its own function, now built on the shared advance_quote_state / is_word_boundary_whitespace primitives instead of being quote-blind, which is what actually fixes the original quoted-value bug without the metacharacter regression. New regression test for the unquoted case. 2. Extracted is_crlf_at() and used it in both tokenize_inner's newline-operator guard and registry.rs's raw_breaks parity check - this exact duplication (two independent "is this \r part of a CRLF pair" checks) was flagged as a follow-up during the PR #3600 review that prompted this whole consolidation, and had been reintroduced here without actually being fixed. All 2714 tests pass, clippy clean. --- src/discover/lexer.rs | 15 +++++++- src/discover/registry.rs | 76 +++++++++++++++++++++++++++++++++++----- 2 files changed, 82 insertions(+), 9 deletions(-) diff --git a/src/discover/lexer.rs b/src/discover/lexer.rs index da83c85140..099d9e8a6d 100644 --- a/src/discover/lexer.rs +++ b/src/discover/lexer.rs @@ -57,6 +57,17 @@ pub(crate) fn is_word_boundary_whitespace(c: char) -> bool { c.is_whitespace() && c != '\r' } +/// True if the byte at `i` is a `\r` immediately followed by `\n` — the two +/// bytes of a CRLF pair. Shared by `tokenize_inner`'s newline-operator arm and +/// `registry.rs::rewrite_multiline_block`'s raw-newline-byte parity check, so +/// the two can't independently drift on what counts as a CRLF pair the way +/// they had before this was extracted (flagged during the PR #3600 review +/// that prompted this consolidation: registry.rs's `raw_breaks` re-derived +/// this exact rule via its own byte scan instead of sharing it). +pub(crate) fn is_crlf_at(bytes: &[u8], i: usize) -> bool { + bytes.get(i) == Some(&b'\r') && bytes.get(i + 1) == Some(&b'\n') +} + fn tokenize_inner(input: &str, emit_newline: bool) -> Vec { let mut tokens = Vec::new(); let mut current = String::new(); @@ -275,7 +286,9 @@ fn tokenize_inner(input: &str, emit_newline: bool) -> Vec { // as a single command — so gating on it would over-segment and the // rewriter would mis-join it. A lone `\r` falls through to the `_` arm // below like any other non-IFS byte, never a word or command boundary. - c @ ('\n' | '\r') if emit_newline && (c == '\n' || chars.peek() == Some(&'\n')) => { + c @ ('\n' | '\r') + if emit_newline && (c == '\n' || is_crlf_at(input.as_bytes(), byte_pos)) => + { flush_arg(&mut tokens, &mut current, current_start); tokens.push(ParsedToken { kind: TokenKind::Operator, diff --git a/src/discover/registry.rs b/src/discover/registry.rs index f2c83f98df..a626b83e5d 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::{ - advance_quote_state, shell_split, split_on_operators, tokenize, tokenize_with_newlines, - ParsedToken, PipeKind, TokenKind, + advance_quote_state, is_crlf_at, is_word_boundary_whitespace, shell_split, split_on_operators, + tokenize, tokenize_with_newlines, ParsedToken, PipeKind, TokenKind, }; use super::rules::{IGNORED_EXACT, IGNORED_PREFIXES, RULES}; @@ -360,15 +360,15 @@ fn strip_golangci_global_opts(cmd: &str) -> String { /// Parse supported golangci-lint invocations with optional global flags before `run`. fn parse_golangci_run_parts(cmd: &str) -> Option> { - let tokens = tokenize(cmd); + let tokens = split_token_spans(cmd); let first = tokens.first()?; - if first.value != "golangci-lint" && first.value != "golangci" { + if first.0 != "golangci-lint" && first.0 != "golangci" { return None; } let mut i = 1; while i < tokens.len() { - let token = tokens[i].value.as_str(); + let token = tokens[i].0; if token == "--" { return None; @@ -377,11 +377,11 @@ fn parse_golangci_run_parts(cmd: &str) -> Option> { if !token.starts_with('-') { if token == "run" { let global_segment = if i > 1 { - cmd[tokens[1].offset..tokens[i].offset].trim() + cmd[tokens[1].1..tokens[i].1].trim() } else { "" }; - let run_segment = cmd[tokens[i].offset..].trim(); + let run_segment = cmd[tokens[i].1..].trim(); return Some(GolangciRunParts { global_segment, run_segment, @@ -426,6 +426,47 @@ fn golangci_flag_takes_separate_value(arg: &str, flag: &str) -> bool { true } +/// Quote-aware whitespace word-splitter: a maximal run of non-whitespace +/// characters, or a quoted span (which may itself contain whitespace), is one +/// word. Deliberately *not* the full shell `tokenize()`: golangci-lint +/// flag/value parsing only needs "was there a space here", not shell syntax — +/// an unquoted value like `--config *.yml` must stay one word, not split into +/// `*` and `.yml` the way `tokenize()` would (it treats `*` as its own +/// Shellism token even outside quotes). Built on the same `advance_quote_state` +/// primitive `tokenize_inner`/`shell_split`/`QuoteScan` use, so this can't +/// independently drift on what counts as "inside a quote". +fn split_token_spans(cmd: &str) -> Vec<(&str, usize)> { + let mut tokens = Vec::new(); + let mut start: Option = None; + let mut quote: Option = None; + + for (idx, ch) in cmd.char_indices() { + let was_quoted = quote.is_some(); + quote = advance_quote_state(quote, ch); + if was_quoted || quote.is_some() { + // Inside a quoted span (or this char just opened/closed one): + // never a word boundary, quotes included in the word verbatim. + if start.is_none() { + start = Some(idx); + } + continue; + } + if is_word_boundary_whitespace(ch) { + if let Some(token_start) = start.take() { + tokens.push((&cmd[token_start..idx], token_start)); + } + } else if start.is_none() { + start = Some(idx); + } + } + + if let Some(token_start) = start { + tokens.push((&cmd[token_start..], token_start)); + } + + tokens +} + /// Normalize absolute binary paths: `/usr/bin/grep -rn foo` → `grep -rn foo` (#485) /// Only strips if the first word contains a `/` (Unix path). fn strip_absolute_path(cmd: &str) -> String { @@ -838,7 +879,7 @@ fn rewrite_multiline_block( let raw_breaks = bytes .iter() .enumerate() - .filter(|&(i, &b)| b == b'\n' || (b == b'\r' && bytes.get(i + 1) == Some(&b'\n'))) + .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 @@ -3814,6 +3855,25 @@ mod tests { )); } + #[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. Caught by /code-review high. + 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!( From 7ba29c019dcc0ac78784c84ec2dee8c6a7b3a998 Mon Sep 17 00:00:00 2001 From: Nicolas Le Cam Date: Tue, 25 Aug 2026 02:42:46 +0200 Subject: [PATCH 06/11] refactor(lexer): coalesce tokens into words, rebuild split_token_spans on it Even after the previous consolidation, split_token_spans and shell_split each still ran their own scanning loop over the raw command string, just now sharing the same quote/whitespace *rules* with tokenize_inner rather than the same *scan*. The gap between "one lexer token" and "one bash word" is exactly the tokens tokenize() splits apart (e.g. Shellism("*") + Arg(".yml") for an unquoted "*.yml") that sit with no gap between them in the original string - bash sees one word there. Add coalesce_words(cmd, tokens): merges directly-adjacent tokens from tokenize()'s output into single words. Rebuild split_token_spans as a one-line wrapper around tokenize() + coalesce_words, removing its own quote-aware scanning loop entirely - golangci-lint's flag parser now runs zero bespoke scanning code. Existing quoted-value and unquoted-glob golangci-lint regression tests (added when this exact call site regressed once already) pass unchanged. All 2716 tests green, clippy clean. --- src/discover/lexer.rs | 47 +++++++++++++++++++++++++++++++++++++ src/discover/registry.rs | 50 ++++++++-------------------------------- 2 files changed, 57 insertions(+), 40 deletions(-) diff --git a/src/discover/lexer.rs b/src/discover/lexer.rs index 099d9e8a6d..e5d387b70b 100644 --- a/src/discover/lexer.rs +++ b/src/discover/lexer.rs @@ -68,6 +68,36 @@ 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 lexer tokens that are directly adjacent in `cmd` (no whitespace or +/// other gap between them) into single words — the gap between "one lexer +/// token" and "one bash word": `tokenize()` splits `*.yml` into a +/// `Shellism("*")` and an `Arg(".yml")` for shell-syntax purposes, but bash +/// sees one word since nothing separates them. Callers that only need +/// "was there a space here" (not full shell-operator awareness) build on +/// this instead of re-scanning `cmd` themselves. +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 { let mut tokens = Vec::new(); let mut current = String::new(); @@ -563,6 +593,23 @@ pub fn shell_split(input: &str) -> Vec { 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"); diff --git a/src/discover/registry.rs b/src/discover/registry.rs index a626b83e5d..41316fc1fc 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::{ - advance_quote_state, is_crlf_at, is_word_boundary_whitespace, 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,45 +426,15 @@ fn golangci_flag_takes_separate_value(arg: &str, flag: &str) -> bool { true } -/// Quote-aware whitespace word-splitter: a maximal run of non-whitespace -/// characters, or a quoted span (which may itself contain whitespace), is one -/// word. Deliberately *not* the full shell `tokenize()`: golangci-lint -/// flag/value parsing only needs "was there a space here", not shell syntax — -/// an unquoted value like `--config *.yml` must stay one word, not split into -/// `*` and `.yml` the way `tokenize()` would (it treats `*` as its own -/// Shellism token even outside quotes). Built on the same `advance_quote_state` -/// primitive `tokenize_inner`/`shell_split`/`QuoteScan` use, so this can't -/// independently drift on what counts as "inside a quote". +/// Quote-aware word splitting for golangci-lint's flag/value parsing: "was +/// there a space here", not shell syntax — deliberately not the full +/// `tokenize()` output, since an unquoted glob like `*.yml` must stay one +/// word rather than split on `*` the way shell-operator tokenizing would. +/// Built directly on `tokenize()` + `coalesce_words` (merging any tokens the +/// full tokenizer split apart but that sit with no gap between them), so this +/// runs no bespoke scanning of its own. fn split_token_spans(cmd: &str) -> Vec<(&str, usize)> { - let mut tokens = Vec::new(); - let mut start: Option = None; - let mut quote: Option = None; - - for (idx, ch) in cmd.char_indices() { - let was_quoted = quote.is_some(); - quote = advance_quote_state(quote, ch); - if was_quoted || quote.is_some() { - // Inside a quoted span (or this char just opened/closed one): - // never a word boundary, quotes included in the word verbatim. - if start.is_none() { - start = Some(idx); - } - continue; - } - if is_word_boundary_whitespace(ch) { - if let Some(token_start) = start.take() { - tokens.push((&cmd[token_start..idx], token_start)); - } - } else if start.is_none() { - start = Some(idx); - } - } - - if let Some(token_start) = start { - tokens.push((&cmd[token_start..], token_start)); - } - - tokens + coalesce_words(cmd, &tokenize(cmd)) } /// Normalize absolute binary paths: `/usr/bin/grep -rn foo` → `grep -rn foo` (#485) From 53e237d96cc9ebe521801908d415a5520946f7cc Mon Sep 17 00:00:00 2001 From: Nicolas Le Cam Date: Tue, 25 Aug 2026 02:46:40 +0200 Subject: [PATCH 07/11] refactor(lexer): rebuild shell_split on coalesce_words shell_split's per-char scanning loop duplicated the same quote/escape handling tokenize_inner already does, differing only in two genuinely distinct concerns: it splits on whitespace only (not shell operators), and it resolves quotes/escapes into argv-ready text rather than preserving them. Split that into two composable pieces instead of one bespoke loop: - resolve_word_text(): given one coalesced word's raw text, strips quote characters and resolves backslash escapes - the inverse of what tokenize_inner preserves, built on the same advance_quote_state so it can't drift from the tokenizer's own quote model. - shell_split() becomes tokenize() + coalesce_words() (from the previous commit) + resolve_word_text() per word - no scanning of its own. All 15 existing shell_split tests pass unchanged (verified byte-for- byte before writing this commit). One real, intentional behavior refinement: word boundaries now come from the shared is_word_boundary_whitespace (bash's actual default $IFS - space, tab, newline) instead of shell_split's previous bespoke ' '|'\t'-only check, so an embedded unquoted newline is now correctly treated as a word boundary. Covered by a new explicit test, since this wouldn't have been caught by the existing suite otherwise. Also added a test combining an unquoted glob directly adjacent to a quoted segment (the same token-coalescing case that mattered for split_token_spans, exercised here through shell_split's quote-stripped output). This affects 3 real call sites: hooks/mod.rs::is_claude_hook_command, registry.rs::search_uses_pattern_file, and main.rs's `rtk proxy '...'` argv construction (the highest-risk one - it directly controls what gets exec'd). All covered by the existing suite, all green. 2718 tests pass, clippy clean. --- src/discover/lexer.rs | 84 +++++++++++++++++++++++++++++++++---------- 1 file changed, 66 insertions(+), 18 deletions(-) diff --git a/src/discover/lexer.rs b/src/discover/lexer.rs index e5d387b70b..81663ce3c4 100644 --- a/src/discover/lexer.rs +++ b/src/discover/lexer.rs @@ -75,6 +75,17 @@ pub(crate) fn is_crlf_at(bytes: &[u8], i: usize) -> bool { /// sees one word since nothing separates them. Callers that only need /// "was there a space here" (not full shell-operator awareness) build on /// this instead of re-scanning `cmd` themselves. +/// +/// Callers only use each `ParsedToken`'s `offset`/`value.len()` here — the +/// `String` `tokenize()` heap-allocates per token is discarded once that's +/// read, since the returned words are re-sliced straight from `cmd`. Not +/// worth avoiding: this runs once per single command-line-length input, well +/// inside RTK's <10ms/<5MB targets — a handful of short-lived allocations for +/// maybe a dozen tokens, not a hot loop. Trading the shared `tokenize()` call +/// for a second, offset-only scanning primitive to dodge them would resurrect +/// the exact "one more parallel implementation" problem this consolidation +/// exists to remove, for a gain nothing here actually needs (confirmed via +/// `/code-review high`, which flagged this and rated it not worth fixing). 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; @@ -547,17 +558,23 @@ 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(); +/// Turns one coalesced word's raw text (quotes and backslash escapes still +/// literal, exactly as `tokenize()` preserves them) into the argv-ready text +/// `shell_split`'s callers need: quote characters that actually open/close a +/// span are stripped, backslash escapes are resolved (dropped, keeping the +/// escaped character). The inverse of what `tokenize_inner` preserves — built +/// on the same `advance_quote_state` so it can't drift from the tokenizer's +/// own quote model. +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 quote != Some('\'') => { if let Some(next) = chars.next() { - current.push(next); + result.push(next); } } '\'' | '"' => { @@ -566,27 +583,36 @@ pub fn shell_split(input: &str) -> Vec { // inside `"..."`) — that's literal text, not a toggle. let new_quote = advance_quote_state(quote, c); if new_quote == quote { - current.push(c); + result.push(c); } else { quote = new_quote; } } - ' ' | '\t' if quote.is_none() => { - if !current.is_empty() { - tokens.push(std::mem::take(&mut current)); - } - } - _ => { - 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`, `registry.rs::search_uses_pattern_file`, +/// `main.rs`'s `rtk proxy '...'` arg-splitting). Built on `tokenize()` + +/// `coalesce_words` for the splitting decision, `resolve_word_text` for the +/// text transform — no bespoke scanning of its own. +/// +/// Note: unlike this function's previous implementation, word boundaries now +/// come from the shared `is_word_boundary_whitespace` (bash's real default +/// `$IFS`: space/tab/**newline**), not just literal space/tab — so an +/// embedded unquoted `\n` is now correctly treated as a word boundary too, +/// matching real shell behavior instead of being glued into the word. +pub fn shell_split(input: &str) -> Vec { + coalesce_words(input, &tokenize(input)) + .into_iter() + .map(|(raw, _)| resolve_word_text(raw)) + .collect() } #[cfg(test)] @@ -1254,6 +1280,28 @@ 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. Building shell_split on + // the shared is_word_boundary_whitespace (rather than its previous + // bespoke ' '|'\t'-only check) means an embedded unquoted `\n` is now + // correctly treated as a word boundary too, matching real shell + // behavior instead of being glued into the surrounding word. + assert_eq!(shell_split("a\nb"), vec!["a", "b"]); + } + #[test] fn test_strip_quotes_double() { assert_eq!(strip_quotes("\"hello\""), "hello"); From 627e45fc4747092d76ee1aedd54aebd938cc2b28 Mon Sep 17 00:00:00 2001 From: Nicolas Le Cam Date: Thu, 27 Aug 2026 00:10:29 +0200 Subject: [PATCH 08/11] fix(lexer): address PR #3704 review feedback - Restrict is_word_boundary_whitespace to bash's actual $IFS (space/tab/newline) instead of Rust's char::is_whitespace(), which wrongly counts non-IFS Unicode whitespace like NBSP as a word boundary and broke shell_split on pasted commands containing it. - Restore the permission gate's conservative lone-CR segmentation. tokenize_inner takes a NewlineMode (None/Bash/Conservative) instead of a bool, so split_for_permissions can opt into treating a bare \r as a command separator (matching pre-consolidation behavior) while every other caller keeps the bash-accurate "lone CR stays glued to its word" rule. The mode lives inside the shared char loop, so it stays quote/escape-aware and doesn't split inside quoted text. - Trim doc comments down to their essential rationale: dropped references to review tooling/history, and cut multi-paragraph explanations to one or two lines, letting the code carry the rest. --- src/discover/lexer.rs | 177 ++++++++++++++++++--------------------- src/discover/registry.rs | 29 ++----- src/hooks/permissions.rs | 21 ++--- 3 files changed, 102 insertions(+), 125 deletions(-) diff --git a/src/discover/lexer.rs b/src/discover/lexer.rs index 81663ce3c4..48ae2681e0 100644 --- a/src/discover/lexer.rs +++ b/src/discover/lexer.rs @@ -22,24 +22,33 @@ 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: an -/// unescaped `'` or `"` opens a quote span if none is open, and only the same -/// character closes it (a `"` inside a `'...'` span, or vice versa, is just -/// literal text). Shared by the char-based tokenizer (`tokenize_inner`, -/// `shell_split`) and every byte-based line scanner in `registry.rs` -/// (`QuoteScan`), so the different representations of "am I inside a quote" -/// can't independently drift the way they had before this was extracted. +/// 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), @@ -48,44 +57,23 @@ pub(crate) fn advance_quote_state(quote: Option, c: char) -> Option } } -/// Bash's default `$IFS` is space/tab/newline — never a bare `\r`. Shared by -/// the lexer's own word-splitting and by permission-pattern normalization -/// (`permissions.rs::command_matches_pattern`) so the two can't independently -/// diverge on what counts as whitespace, which is how a lone-CR bug fixed here -/// once reappeared there as a separate, undetected regression. +/// 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 { - c.is_whitespace() && c != '\r' + matches!(c, ' ' | '\t' | '\n') } -/// True if the byte at `i` is a `\r` immediately followed by `\n` — the two -/// bytes of a CRLF pair. Shared by `tokenize_inner`'s newline-operator arm and -/// `registry.rs::rewrite_multiline_block`'s raw-newline-byte parity check, so -/// the two can't independently drift on what counts as a CRLF pair the way -/// they had before this was extracted (flagged during the PR #3600 review -/// that prompted this consolidation: registry.rs's `raw_breaks` re-derived -/// this exact rule via its own byte scan instead of sharing it). +/// 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 lexer tokens that are directly adjacent in `cmd` (no whitespace or -/// other gap between them) into single words — the gap between "one lexer -/// token" and "one bash word": `tokenize()` splits `*.yml` into a -/// `Shellism("*")` and an `Arg(".yml")` for shell-syntax purposes, but bash -/// sees one word since nothing separates them. Callers that only need -/// "was there a space here" (not full shell-operator awareness) build on -/// this instead of re-scanning `cmd` themselves. -/// -/// Callers only use each `ParsedToken`'s `offset`/`value.len()` here — the -/// `String` `tokenize()` heap-allocates per token is discarded once that's -/// read, since the returned words are re-sliced straight from `cmd`. Not -/// worth avoiding: this runs once per single command-line-length input, well -/// inside RTK's <10ms/<5MB targets — a handful of short-lived allocations for -/// maybe a dozen tokens, not a hot loop. Trading the shared `tokenize()` call -/// for a second, offset-only scanning primitive to dodge them would resurrect -/// the exact "one more parallel implementation" problem this consolidation -/// exists to remove, for a gain nothing here actually needs (confirmed via -/// `/code-review high`, which flagged this and rated it not worth fixing). +/// 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; @@ -109,7 +97,7 @@ pub(crate) fn coalesce_words<'a>(cmd: &'a str, tokens: &[ParsedToken]) -> Vec<(& 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; @@ -321,14 +309,11 @@ fn tokenize_inner(input: &str, emit_newline: bool) -> Vec { }); current_start = byte_pos; } - // `\n`, and the `\r` of a CRLF pair, are command separators. A lone `\r` - // (classic-Mac EOL, not followed by `\n`) is NOT: bash treats a bare CR - // as an ordinary character inside a word — `git statusgit log` runs - // as a single command — so gating on it would over-segment and the - // rewriter would mis-join it. A lone `\r` falls through to the `_` arm - // below like any other non-IFS byte, never a word or command boundary. c @ ('\n' | '\r') - if emit_newline && (c == '\n' || is_crlf_at(input.as_bytes(), byte_pos)) => + 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 { @@ -444,11 +429,13 @@ fn redirect_has_file_target(tokens: &[ParsedToken], i: usize) -> bool { /// | 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 `&`, and -/// subshell `( ... )`, 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. +/// 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(); @@ -456,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; @@ -489,22 +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). -/// -/// Its only current caller (`registry.rs::split_command_chain`, used for -/// analytics/discovery classification) passes `stop_at_pipe: true`, and unlike -/// [`split_for_permissions`] never splits on background `&` or `( ... )` -/// grouping, and never truncates at a redirect — see the comparison table on -/// [`split_for_permissions`] for the full picture across all three -/// compound-command segmenters. That's fine for classification (it only needs -/// to identify supported commands, not defend against a hidden one), but this -/// function must not be repurposed for permission/security decisions as-is. +/// 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() { @@ -558,13 +538,9 @@ pub fn strip_quotes(s: &str) -> String { s.to_string() } -/// Turns one coalesced word's raw text (quotes and backslash escapes still -/// literal, exactly as `tokenize()` preserves them) into the argv-ready text -/// `shell_split`'s callers need: quote characters that actually open/close a -/// span are stripped, backslash escapes are resolved (dropped, keeping the -/// escaped character). The inverse of what `tokenize_inner` preserves — built -/// on the same `advance_quote_state` so it can't drift from the tokenizer's -/// own quote model. +/// 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(); @@ -598,16 +574,7 @@ fn resolve_word_text(raw: &str) -> String { /// 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`, `registry.rs::search_uses_pattern_file`, -/// `main.rs`'s `rtk proxy '...'` arg-splitting). Built on `tokenize()` + -/// `coalesce_words` for the splitting decision, `resolve_word_text` for the -/// text transform — no bespoke scanning of its own. -/// -/// Note: unlike this function's previous implementation, word boundaries now -/// come from the shared `is_word_boundary_whitespace` (bash's real default -/// `$IFS`: space/tab/**newline**), not just literal space/tab — so an -/// embedded unquoted `\n` is now correctly treated as a word boundary too, -/// matching real shell behavior instead of being glued into the word. +/// (`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() @@ -1294,14 +1261,19 @@ mod tests { #[test] fn test_shell_split_splits_on_embedded_newline() { - // Bash's default $IFS is space/tab/newline. Building shell_split on - // the shared is_word_boundary_whitespace (rather than its previous - // bespoke ' '|'\t'-only check) means an embedded unquoted `\n` is now - // correctly treated as a word boundary too, matching real shell - // behavior instead of being glued into the surrounding word. + // 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"); @@ -1470,6 +1442,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!( @@ -1540,10 +1533,6 @@ mod tests { #[test] fn test_crlf_in_plain_tokenize_keeps_cr_glued_to_word() { - // Plain `tokenize()` (emit_newline=false) treats `\n` as ordinary IFS - // whitespace but never `\r` — a `\r` right before it is not a separator, - // so it stays glued to the word it terminates, same as real bash would - // keep a bare CR byte attached to its word. let args: Vec = tokenize("git status\r\ngit log") .into_iter() .map(|t| t.value) diff --git a/src/discover/registry.rs b/src/discover/registry.rs index 41316fc1fc..d5a0e87778 100644 --- a/src/discover/registry.rs +++ b/src/discover/registry.rs @@ -427,12 +427,8 @@ fn golangci_flag_takes_separate_value(arg: &str, flag: &str) -> bool { } /// Quote-aware word splitting for golangci-lint's flag/value parsing: "was -/// there a space here", not shell syntax — deliberately not the full -/// `tokenize()` output, since an unquoted glob like `*.yml` must stay one -/// word rather than split on `*` the way shell-operator tokenizing would. -/// Built directly on `tokenize()` + `coalesce_words` (merging any tokens the -/// full tokenizer split apart but that sit with no gap between them), so this -/// runs no bespoke scanning of its own. +/// 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)) } @@ -1027,20 +1023,11 @@ fn rewrite_pipeline_final_stage( }) } -/// Rewrite a compound command (with `&&`, `||`, `;`, `|`) by rewriting each segment. -/// -/// This walk is the third of three compound-command segmenters in this -/// codebase — see the comparison table on -/// [`crate::discover::lexer::split_for_permissions`] (the permission gate's -/// segmenter) for the full picture. This one also splits on background `&` -/// (`TokenKind::Shellism if tok.value == "&"` below), but — unlike -/// `split_for_permissions` — does **not** treat standalone `(`/`)` as a -/// segment boundary (only bails out entirely via `has_opaque_grouping` when a -/// pipe and a grouping char coexist) and does not truncate segments at a -/// redirect (redirects are preserved verbatim in the rewritten output, by -/// design). These differences are deliberate: the permission gate must stay -/// the most conservative of the three, this function's job is to reproduce -/// the command's actual shape, not to defend against it hiding something. +/// 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], @@ -3834,7 +3821,7 @@ mod tests { // 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. Caught by /code-review high. + // Unsupported. assert!(matches!( classify_command("golangci-lint --config *.yml run ./..."), Classification::Supported { diff --git a/src/hooks/permissions.rs b/src/hooks/permissions.rs index f6ba3f5f13..84135d7277 100644 --- a/src/hooks/permissions.rs +++ b/src/hooks/permissions.rs @@ -383,10 +383,8 @@ 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 { - // Shares the lexer's definition of a word boundary (`is_word_boundary_whitespace`) - // instead of `str::split_whitespace()`'s Unicode-whitespace notion, so a bare - // `\r` embedded in `cmd` is never collapsed into a space here — matching how - // the lexer itself now treats a lone `\r` as part of the word, not a boundary. + // 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()) @@ -958,12 +956,6 @@ mod tests { #[test] fn test_lone_cr_hidden_command_not_auto_allowed() { - // `split_for_permissions` correctly treats a lone `\r` (no `\n`) as part - // of one command, not a boundary. Before command_matches_pattern shared - // the lexer's word-boundary definition, its own `split_whitespace()` - // still collapsed the embedded `\r` into a space, so "git status\rrm -rf - // ~" normalized to "git status rm -rf ~" and matched an allow rule for - // "git status" it should never have matched. let allow = vec!["git status".to_string()]; assert_eq!( check_command_with_rules("git status\rrm -rf ~", &[], &[], &allow), @@ -979,6 +971,15 @@ mod tests { )); } + #[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()]; From d86465a37b1ea2aac15adf1c1863f34cc006268c Mon Sep 17 00:00:00 2001 From: Nicolas Le Cam Date: Thu, 27 Aug 2026 00:22:25 +0200 Subject: [PATCH 09/11] docs(discover): document the shared lexer toolkit in README Per review feedback on #3681 (document new shared systems in their module README): lexer.rs's public functions are consumed well outside discover/ (hooks/permissions.rs, hooks/mod.rs, main.rs's rtk proxy), so document them as shared infrastructure rather than leaving them as an internal implementation detail. --- src/discover/README.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) 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: From 0d1d9ed65715741f3683b51d575e675dcc924938 Mon Sep 17 00:00:00 2001 From: Nicolas Le Cam Date: Fri, 28 Aug 2026 02:25:12 +0200 Subject: [PATCH 10/11] fix(lexer): backslash is literal before a non-special char in double quotes shell_split resolved `\` as an escape everywhere outside single quotes, so every backslash in a double-quoted Windows path was eaten: `"C:\Program Files\rtk.exe"` came back as `C:Program Filesrtk.exe`. Bash only lets `\` escape `$`, `` ` ``, `"`, `\` or a newline inside double quotes; before anything else it is a literal character. Match that, which fixes quoted Windows paths for every shell_split caller at once (hooks/mod.rs's hook-install detection, rtk proxy's argv construction, registry.rs::search_uses_pattern_file) instead of per call site. Unquoted backslashes still escape, as bash does. Co-Authored-By: Claude Opus 5 (1M context) --- src/discover/lexer.rs | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/src/discover/lexer.rs b/src/discover/lexer.rs index 48ae2681e0..ae8603d7a9 100644 --- a/src/discover/lexer.rs +++ b/src/discover/lexer.rs @@ -548,7 +548,19 @@ fn resolve_word_text(raw: &str) -> String { while let Some(c) = chars.next() { match c { - '\\' if quote != Some('\'') => { + // 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() { result.push(next); } @@ -1223,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"); From 386e639521377815306b201614b56738cf5c6cda Mon Sep 17 00:00:00 2001 From: Nicolas Le Cam Date: Wed, 2 Sep 2026 22:05:11 +0200 Subject: [PATCH 11/11] test(registry): pin lone-CR behaviour in the rewriter The lexer and permission-gate sides of the lone-CR rule are covered, but `rewrite_multiline_block` had no test for it: a quoted `\r` staying one command, a bare `\r` taking a single prefix while only the `\n` starts a new line, and the raw-break parity count ignoring a quoted lone `\r` instead of reading it as a line the lexer hid. Co-Authored-By: Claude Opus 5 --- src/discover/registry.rs | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/discover/registry.rs b/src/discover/registry.rs index d5a0e87778..ca17c255a1 100644 --- a/src/discover/registry.rs +++ b/src/discover/registry.rs @@ -1647,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!(