From 16228b74b083f218766e909a8f8c80f3b0e1c171 Mon Sep 17 00:00:00 2001 From: Brandon Harper <6859+devnulled@users.noreply.github.com> Date: Sun, 30 Aug 2026 11:19:28 -0600 Subject: [PATCH 1/8] fix(grep): stop -l and -t shadowing native grep flags rtk's own tuning options on `Commands::Grep` declared short forms that collide with native grep/rg flags of the same letter. This removes the two that remain, and deletes one option outright. `--max-len` loses `-l`. `-l` is grep's --files-with-matches, and the old binding split behavior on whether the PATTERN parsed as a usize: - Non-numeric: clap rejected it, run_fallback re-ran raw grep, and the user got the right answer. Wasteful, not harmful. - Numeric: clap ACCEPTED it, and the result was silently wrong. # hit.txt contains "listen on port 8080 today"; miss.txt does not $ rtk grep -l 8080 hit.txt miss.txt before: (no output) exit=1 <- claims nothing matched after: hit.txt exit=0 `-l 8080` set max_len=8080, leaving hit.txt as the pattern and miss.txt as the only path. With a single file the filename becomes the pattern, no path is left, and rtk falls back to stdin: exit 1 against /dev/null, hangs on a terminal. Port numbers, years and error codes are ordinary patterns, and the Claude Code hook rewrites plain `grep` into `rtk grep` transparently -- so this reached users who never opted in. Note this is not a savings story. `-l` is a has_format_flag passthrough on the fixed path too (search.rs:548), so there is no compaction for this flag either way. The bug was a wrong answer. `--file-type` is removed entirely, with its `-t` short, rather than left as a parse-only no-op. It was destructured as `file_type: _` at the call site and never reached search::run, so it parsed and then did nothing. `rtk rg -t rust` is unaffected: Rg has no such field and forwards `-t` to real ripgrep. Deliberately typed `fix`, not `fix!`, even though a documented long option disappears. The option was inert -- it never filtered anything -- so no working behavior is lost, and #3259 set the precedent for removing a colliding grep short under plain `fix`. Recording the choice here because silence would read as an oversight. User-visible consequence: `rtk grep -t rust` goes from silently returning unfiltered results to surfacing the engine's own error. BSD/macOS grep prints `invalid option -- t`; GNU grep prints `invalid option -- 't'`. `--max`/`-m` is NOT part of this change. #3259 removed that short already; this branch is rebased onto it and preserves its rationale comment verbatim. The struct NOTE deliberately does not claim to finish the job. clap's auto `-h` still shadows grep/rg's --no-filename (that is #2532's scope), and removing a clap short does not make a letter safe end to end: search.rs shares one VALUE_FLAGS_SHORT table across both engines, so a letter taking a value in rg but not grep still eats the next token inside a cluster -- `rtk grep -rt FOO .` consumes FOO as -t's value regardless of clap. Adds clap-layer tests pinning the routing, including characterization tests for behavior that was previously untested. --- docs/usage/FEATURES.md | 5 +- src/main.rs | 201 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 199 insertions(+), 7 deletions(-) diff --git a/docs/usage/FEATURES.md b/docs/usage/FEATURES.md index 19d722632e..008c608a81 100644 --- a/docs/usage/FEATURES.md +++ b/docs/usage/FEATURES.md @@ -215,14 +215,15 @@ rtk grep [chemin] [options] | Option | Court | Defaut | Description | |--------|-------|--------|-------------| -| `--max-len` | `-l` | 80 | Longueur maximale de ligne | +| `--max-len` | | 80 | Longueur maximale de ligne (pas de raccourci, `-l` est reserve a `grep --files-with-matches`) | | `--max` | | 200 | Nombre maximum de resultats (pas de raccourci, `-m` est reserve a `grep --max-count`) | | `--context-only` | | non | Afficher uniquement le contexte du match (pas de raccourci, `-c` est reserve a `grep --count`) | -| `--file-type` | `-t` | tous | Filtrer par type (ts, py, rust, etc.) | | `--line-numbers` | `-n` | oui | Numeros de ligne (toujours actif) | Les arguments supplementaires sont transmis a `rg` (ripgrep). Les flags qui changent le format de sortie (`-c`, `-l`, `-L`, `-o`, `-Z`) passent directement a `rg`/`grep` sans filtrage RTK. +Il n'y a pas d'option `--file-type` : `-t TYPE` est propre a ripgrep et est transmis tel quel au moteur. `rtk rg -t rust` filtre donc par type, tandis que `rtk grep -t rust` remonte l'erreur de grep lui-meme (`invalid option -- 't'`), grep n'ayant pas ce flag. + **Avant / Apres :** ``` # rg "fn run" (20 lignes) # rtk grep "fn run" (10 lignes) diff --git a/src/main.rs b/src/main.rs index 4520e13062..17ef870714 100644 --- a/src/main.rs +++ b/src/main.rs @@ -313,8 +313,21 @@ enum Commands { /// Compact grep - strips whitespace, truncates, groups by file Grep { + // NOTE: rtk's own tuning options here are intentionally long-only. + // Their natural short forms collide with native grep/rg flags of the + // same letter, which users routinely pass. A short option here shadows + // the native flag and either crashes clap or silently applies the wrong + // semantics, defeating the `extra_args` passthrough. Keep them long-only + // so native flags reach src/cmds/system/search.rs, the shared filter + // behind both `rtk grep` and `rtk rg`. (Completes the `-v`/`-n`/pattern/ + // path cleanup from 84616d1.) /// Max line length - #[arg(short = 'l', long, default_value = "80")] + // No short: `-l` is grep's --files-with-matches. Bound to RTK's + // --max-len (a `usize`), `rtk grep -l PATTERN` could not parse at all -- + // clap aborted with exit 2 and rtk fell back to raw grep, losing all + // compaction. Without the short, `-l` flows to extra_args and reaches + // grep/rg, which lists matching filenames as the user intended. + #[arg(long, default_value = "80")] max_len: usize, /// Max results to show // No short: `-m` is GNU grep's --max-count (stop after N matches per @@ -328,9 +341,11 @@ enum Commands { /// Show only match context (not full line) #[arg(long)] context_only: bool, - /// Filter by file type (e.g., ts, py, rust) - #[arg(short = 't', long)] - file_type: Option, + // No --file-type / `-t`: `-t TYPE` is rg's type filter, and the option + // never did anything under `rtk grep` -- it was destructured away at the + // call site and never reached the engine. Removed rather than left as a + // parse-only no-op; `rtk rg -t rust` still filters by type, since it + // forwards `-t` to real ripgrep. /// Pattern, path, and any grep/rg flags (e.g. -v, -i, -A 3, --glob, --version) #[arg(trailing_var_arg = true, allow_hyphen_values = true)] extra_args: Vec, @@ -2005,7 +2020,6 @@ fn run_cli() -> Result { max_len, max, context_only, - file_type: _, extra_args, } => search::run( search::Engine::Grep, @@ -3695,4 +3709,181 @@ mod tests { _ => panic!("Expected Init command"), } } + + // --- grep argument routing (clap layer) --- + // + // The `Grep` variant funnels the pattern, path, and every native grep/rg + // flag into a single `trailing_var_arg` slot so `src/cmds/system/search.rs` + // -- the shared filter behind both `rtk grep` and `rtk rg` -- can parse them + // with full grep/rg semantics. Commit 84616d1 ("fix(grep): stabilize + // argument parsing") moved to this design but left the struct's own short + // options in place, where they still shadowed native flags of the same + // letter. These tests pin the routing. + + /// Parse `rtk grep …` and return the captured `extra_args`, or `None` if + /// clap rejects the invocation (e.g. a colliding short option mis-binds). + fn grep_extra_args(args: &[&str]) -> Option> { + match Cli::try_parse_from(args).ok()?.command { + Commands::Grep { extra_args, .. } => Some(extra_args), + _ => None, + } + } + + // Characterization: behavior that must be PRESERVED (passes today, untested + // until now). Guards the 84616d1 design against regression. + + #[test] + fn test_grep_parse_simple_pattern_path() { + assert_eq!( + grep_extra_args(&["rtk", "grep", "FOO", "src/"]).unwrap(), + vec!["FOO", "src/"] + ); + } + + #[test] + fn test_grep_parse_combined_short_cluster() { + // `-rn` is a native grep cluster (recursive + line-numbers); it must + // reach search.rs intact, not be intercepted by clap. + assert_eq!( + grep_extra_args(&["rtk", "grep", "-rn", "FOO", "src/"]).unwrap(), + vec!["-rn", "FOO", "src/"] + ); + } + + #[test] + fn test_grep_parse_value_flag_after_context() { + // `-A 3` (after-context) — the value must not be stolen by clap. + assert_eq!( + grep_extra_args(&["rtk", "grep", "-A", "3", "FOO", "file"]).unwrap(), + vec!["-A", "3", "FOO", "file"] + ); + } + + #[test] + fn test_grep_parse_invert_match_v_not_shadowed() { + // Regression guard for 84616d1: `-v` (invert-match) must reach grep, + // not be captured as rtk's top-level verbose flag. + assert_eq!( + grep_extra_args(&["rtk", "grep", "-v", "FOO", "file"]).unwrap(), + vec!["-v", "FOO", "file"] + ); + } + + #[test] + fn test_grep_parse_alternation_pattern_intact() { + // Extended-regex alternation must pass through untouched (#1436 lineage). + assert_eq!( + grep_extra_args(&["rtk", "grep", "-nE", "a|b", "file"]).unwrap(), + vec!["-nE", "a|b", "file"] + ); + } + + // The fix: native `-l`/`-m`/`-t` must route to search.rs, not be captured by + // the struct's own short options. `-l` mis-bound to `max_len: usize`, so clap + // rejected the pattern outright; `-m`/`-t` silently swallowed their value with + // the wrong semantics. (`-m` was fixed separately in #3259; pinned here too.) + + #[test] + fn test_grep_parse_files_with_matches_l() { + // Native grep `-l` (files-with-matches). Must parse and pass `-l` through. + assert_eq!( + grep_extra_args(&["rtk", "grep", "-l", "FOO", "src/"]).unwrap(), + vec!["-l", "FOO", "src/"] + ); + } + + #[test] + fn test_grep_parse_max_count_m_forwarded() { + // Native grep `-m N` (max-count). The value `5` must reach search.rs, + // not be eaten as rtk's `--max`. + assert_eq!( + grep_extra_args(&["rtk", "grep", "-m", "5", "FOO", "file"]).unwrap(), + vec!["-m", "5", "FOO", "file"] + ); + } + + #[test] + fn test_grep_parse_type_t_forwarded() { + // `-t TYPE` is rg-only, so it must reach the engine rather than binding + // to an rtk option. search.rs lists `t` in VALUE_FLAGS_SHORT and forwards + // it verbatim: under `rtk rg` it filters by type, under `rtk grep` it + // surfaces grep's own `invalid option -- 't'` instead of being silently + // dropped. + assert_eq!( + grep_extra_args(&["rtk", "grep", "-t", "rust", "FOO", "src/"]).unwrap(), + vec!["-t", "rust", "FOO", "src/"] + ); + } + + // --- additional grep characterization (preserved behavior, was untested) --- + + /// Parse `rtk grep …` into the full `Grep` field set for inspection. + fn parse_grep(args: &[&str]) -> Result<(usize, usize, bool, Vec), clap::Error> { + match Cli::try_parse_from(args)?.command { + Commands::Grep { + max_len, + max, + context_only, + extra_args, + } => Ok((max_len, max, context_only, extra_args)), + _ => unreachable!("parsed a grep command"), + } + } + + #[test] + fn test_grep_long_options_bind_before_pattern() { + // rtk's tuning knobs are long-only (the short forms were removed to stop + // shadowing native grep flags). They must still bind when placed before + // the pattern, and only the pattern/path land in extra_args. + let (max_len, max, context_only, extra_args) = parse_grep(&[ + "rtk", + "grep", + "--max-len", + "40", + "--max", + "5", + "--context-only", + "FOO", + "path", + ]) + .unwrap(); + assert_eq!(max_len, 40); + assert_eq!(max, 5); + assert!(context_only); + assert_eq!(extra_args, vec!["FOO", "path"]); + } + + #[test] + fn test_grep_options_after_pattern_stay_in_extra_args() { + // trailing_var_arg gotcha: once the pattern starts the trailing slot, + // later tokens — even rtk's own `--max-len` — are NOT parsed as options; + // they pass through verbatim. (Native grep ordering: flags come before + // the pattern.) + let (max_len, _, _, extra_args) = + parse_grep(&["rtk", "grep", "FOO", "--max-len", "40"]).unwrap(); + assert_eq!( + max_len, 80, + "option after pattern must NOT bind (default kept)" + ); + assert_eq!(extra_args, vec!["FOO", "--max-len", "40"]); + } + + #[test] + fn test_grep_version_routes_to_extra_args() { + // `--version` is not a subcommand flag (version isn't propagated), so it + // lands in extra_args and grep_cmd::run forwards it to rg --version. + assert_eq!( + grep_extra_args(&["rtk", "grep", "--version"]).unwrap(), + vec!["--version"] + ); + } + + #[test] + fn test_grep_double_dash_consumed_by_clap() { + // clap strips the `--` separator before populating trailing_var_arg. + // core::args_utils::restore_double_dash re-inserts it at runtime — this + // test pins the premise that helper depends on. + let (_, _, _, extra_args) = parse_grep(&["rtk", "grep", "--", "-v", "file"]).unwrap(); + assert_eq!(extra_args, vec!["-v", "file"], "clap must consume the --"); + } } From 87bdb775dca919a0572d7e6dacaf87cefc945061 Mon Sep 17 00:00:00 2001 From: Brandon Harper <6859+devnulled@users.noreply.github.com> Date: Sat, 29 Aug 2026 18:01:43 -0600 Subject: [PATCH 2/8] test(grep): cover --max-len end-to-end after the short-form removal Adds max_len_long_option_still_binds_to_rtk: the surviving long form must still reach rtk's truncation now that `-l` is gone. Mutation-proved. Discarding max_len at the call site (`max_len: _`, so the mutant compiles) turns it red: --max-len must still bind after the short form was removed (narrow=83, default=83) Restored, it passes (narrow=43, default=83). Two things the test has to get right, both found by watching it fail: - It needs a bulky match set. A small result passes through uncompressed under the never-worse-than-plain guard, so a one-line file exercises the passthrough and proves nothing. - It measures matched lines only. rtk's "+N more [see remaining: ...]" trailer is fixed-width chrome that --max-len does not govern, and it is the longest line in either run. No companion `-l` integration test: `-l` mis-binding never changed program output. A clap parse error routes through run_fallback, which re-executes the raw command, and the fixed path sends `-l` to the has_format_flag passthrough -- so both sides print the same file list and exit 0. A test asserting that would pass with or without the fix. The clap-layer test test_grep_parse_files_with_matches_l is the real gate there; it fails when the `-l` short is restored. --- tests/search_compress_test.rs | 49 +++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tests/search_compress_test.rs b/tests/search_compress_test.rs index 74c55aed8c..aa4445c7b2 100644 --- a/tests/search_compress_test.rs +++ b/tests/search_compress_test.rs @@ -316,6 +316,55 @@ fn small_grep_not_worse_than_plain() { ); } +// --- #2628: rtk's own long options still bind after the short forms were removed --- + +#[test] +fn max_len_long_option_still_binds_to_rtk() { + if !rg_available() { + return; + } + // The other half of #2628: dropping the `-l` short must not stop the long + // form from reaching rtk's truncation. Needs a bulky match set -- a small + // result passes through uncompressed under the never-worse-than-plain + // guard, so a one-line file would prove nothing either way. + let body: String = (0..60) + .map(|i| format!("MATCH {i} {}\n", "x".repeat(400))) + .collect(); + let (_dir, path) = write_temp(&body); + let file = path.to_str().unwrap(); + + // Measure matched lines only. rtk's own trailer ("+N more ... [see + // remaining: tail ...]") is fixed-width chrome that --max-len does not + // govern, and it is the longest line in either run. + let widest_match = |args: &[&str]| -> usize { + let out = rtk().args(args).output().expect("rtk grep"); + assert!( + out.status.success(), + "stderr={}", + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8_lossy(&out.stdout) + .lines() + .filter(|l| l.starts_with("MATCH ")) + .map(|l| l.chars().count()) + .max() + .unwrap_or(0) + }; + + let narrow = widest_match(&["grep", "--max-len", "40", "MATCH", file]); + let default = widest_match(&["grep", "MATCH", file]); + + assert!( + narrow > 0 && default > 0, + "expected matched lines in both runs" + ); + assert!( + narrow < default, + "--max-len must still bind after the short form was removed \ + (narrow={narrow}, default={default})" + ); +} + // --- #2543: bundled files-with-matches cluster (-rln / -ln) lists files, not "0 matches" --- #[test] From 187228dc508ebdf7346cc1882b797cd01984553b Mon Sep 17 00:00:00 2001 From: Brandon Harper <6859+devnulled@users.noreply.github.com> Date: Sat, 29 Aug 2026 23:45:42 -0600 Subject: [PATCH 3/8] fix(grep): repair the smoke assertion this PR invalidated scripts/test-all.sh asserted `rtk grep "pub fn" src/ -t rust` exits 0. That only held because --file-type swallowed `-t` and never passed it to the engine, so the assertion was pinning a silent no-op. With --file-type gone, `-t` reaches GNU grep, which has no such flag: $ rtk grep "pub fn" src/ -t rust grep: invalid option -- t exit=2 Replaced with an assert_fails pinning that rejection, plus an assert_ok against rg (which does have -t), guarded on rg being installed. Verified: rtk grep "pub fn" src/ -t rust -> exit=2 (assert_fails wants non-zero) rtk rg "pub fn" src/ -t rust -> exit=0 (assert_ok wants 0) $ bash -n scripts/test-all.sh syntax OK Also drops a `--line-numbers | -n` row from the FEATURES.md grep table. No such option exists on Commands::Grep -- its fields are max_len, max, context_only, extra_args. The `line_numbers` field at src/main.rs:118 belongs to Read. The row predates this PR but sits in the table it edits. Note, NOT fixed here as it is pre-existing and unrelated: the adjacent `assert_ok "rtk grep pattern" rtk grep "pub fn" src/` on the line above also fails, because grep without -r on a directory exits 2. The smoke suite is not wired into CI (referenced only from .github/copilot-instructions.md), which is how it rotted unnoticed. --- docs/usage/FEATURES.md | 1 - scripts/test-all.sh | 10 +++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/docs/usage/FEATURES.md b/docs/usage/FEATURES.md index 008c608a81..0df0632d72 100644 --- a/docs/usage/FEATURES.md +++ b/docs/usage/FEATURES.md @@ -218,7 +218,6 @@ rtk grep [chemin] [options] | `--max-len` | | 80 | Longueur maximale de ligne (pas de raccourci, `-l` est reserve a `grep --files-with-matches`) | | `--max` | | 200 | Nombre maximum de resultats (pas de raccourci, `-m` est reserve a `grep --max-count`) | | `--context-only` | | non | Afficher uniquement le contexte du match (pas de raccourci, `-c` est reserve a `grep --count`) | -| `--line-numbers` | `-n` | oui | Numeros de ligne (toujours actif) | Les arguments supplementaires sont transmis a `rg` (ripgrep). Les flags qui changent le format de sortie (`-c`, `-l`, `-L`, `-o`, `-Z`) passent directement a `rg`/`grep` sans filtrage RTK. diff --git a/scripts/test-all.sh b/scripts/test-all.sh index 8be6e8deca..7d88121173 100755 --- a/scripts/test-all.sh +++ b/scripts/test-all.sh @@ -259,7 +259,15 @@ section "Grep" assert_ok "rtk grep pattern" rtk grep "pub fn" src/ assert_contains "rtk grep finds results" "pub fn" rtk grep "pub fn" src/ -assert_ok "rtk grep with file type" rtk grep "pub fn" src/ -t rust +# `-t` is rg-only. rtk used to swallow it via its own --file-type option, which +# never reached the engine, so this asserted a silent no-op. --file-type is gone; +# `-t` now flows to the engine, so pin it against the engine that has it. +assert_fails "rtk grep -t rejected by grep" rtk grep "pub fn" src/ -t rust +if command -v rg >/dev/null 2>&1; then + assert_ok "rtk rg with file type" rtk rg "pub fn" src/ -t rust +else + skip_test "rtk rg with file type" "rg not installed" +fi section "Grep (extra args passthrough)" From 901838ddae373deb7d3c8460d7f97feab2f7577d Mon Sep 17 00:00:00 2001 From: Brandon Harper <6859+devnulled@users.noreply.github.com> Date: Sat, 29 Aug 2026 23:46:29 -0600 Subject: [PATCH 4/8] fix(bench): stop benchmarking a grep command that errors scripts/benchmark.sh measured `rtk grep -rn 'fn ' src/ -l 40`, intending --max-len 40. Two problems: `-l` is no longer rtk's short for --max-len, and the flags sit AFTER the pattern, where trailing_var_arg passes them through verbatim rather than binding them. So `-l` reached grep as --files-with-matches and `40` was taken as a filename: $ rtk grep -rn 'fn ' src/ -l 40 src/analytics/gain.rs src/analytics/session_cmd.rs grep: 40: No such file or directory The benchmark was timing that error path on develop too -- it is not a regression from this PR, but --max-len is the option this PR just changed, and this is its only consumer in the repo. Now uses the long form, before the pattern, where it binds: $ rtk grep --max-len 40 -rn 'fn ' src/ # widest match line: 77 $ rtk grep -rn 'fn ' src/ # widest match line: 117 $ bash -n scripts/benchmark.sh syntax OK --- scripts/benchmark.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/benchmark.sh b/scripts/benchmark.sh index ffc7301426..5d5f91caf0 100755 --- a/scripts/benchmark.sh +++ b/scripts/benchmark.sh @@ -215,7 +215,7 @@ bench "git show" "git show HEAD --stat 2>/dev/null || true" "$RTK git show HEAD section "grep" bench "grep fn" "grep -rn 'fn ' src/ || true" "$RTK grep -rn 'fn ' src/" bench "grep struct" "grep -rn 'struct ' src/ || true" "$RTK grep -rn 'struct ' src/" -bench "grep -l 40" "grep -rn 'fn ' src/ || true" "$RTK grep -rn 'fn ' src/ -l 40" +bench "grep --max-len 40" "grep -rn 'fn ' src/ || true" "$RTK grep --max-len 40 -rn 'fn ' src/" bench "grep -c" "grep -ron 'fn ' src/ || true" "$RTK grep -rc 'fn ' src/" # =================== From d5cb3b1abb7e16ea2646a4cc72f5d3f597774449 Mon Sep 17 00:00:00 2001 From: Brandon Harper <6859+devnulled@users.noreply.github.com> Date: Sat, 29 Aug 2026 23:50:23 -0600 Subject: [PATCH 5/8] test(grep): close three gaps a mutation audit found 1. The --file-type removal had ZERO coverage. Restoring the option long-only (no `-t`) left the entire suite green, so nothing stopped it creeping back. Added test_grep_file_type_option_is_gone, which pins that the flag routes to extra_args. Note it is NOT a clap error -- unknown long flags fall through to the trailing slot -- so an is_err() assertion would be wrong; grep is what rejects it. Mutation-proved: # --file-type restored long-only (+ file_type: _ so it compiles) assertion `left == right` failed left: ["FOO", "src/"] right: ["--file-type", "rust", "FOO", "src/"] test result: FAILED. 0 passed; 1 failed 2. max_len_long_option_still_binds_to_rtk guarded on rg_available(), but `rtk grep` execs grep, never rg (search.rs: `Engine::Grep => "grep"`). On a box without ripgrep the test skipped and reported success, hiding the mutant it exists to catch. Added grep_available() and switched to it. Proved with the call site discarding max_len and rg off PATH: $ PATH=/usr/bin:/bin search_compress_test max_len_long_option --max-len must still bind after the short form was removed (narrow=83, default=83) test result: FAILED. 0 passed; 1 failed Before this change that invocation printed `ok`. 3. test_grep_options_after_pattern_stay_in_extra_args claimed to demonstrate a `trailing_var_arg` gotcha. Removing trailing_var_arg leaves it green -- `allow_hyphen_values` is what produces the behavior. Comment corrected so it is not read as a guard on trailing_var_arg. Also drops test_grep_parse_max_count_m_forwarded: it duplicates the pre-existing test_try_parse_grep_dash_m_is_max_count (src/main.rs:3012), which dies to the same mutant and asserts strictly more (it also pins `max == 200`). Walks back the Grep NOTE block, which overclaimed. It said this "completes" the shadowed-short cleanup. It does not: - clap's auto `-h` still beats grep/rg's --no-filename before the first positional, and after it reaches search.rs's help check and execs unfiltered. That is #2532's scope, so the comment now says so rather than inviting the next reader to add disable_help_flag here. - removing a clap short does not make the letter safe end-to-end. search.rs shares one VALUE_FLAGS_SHORT table across both engines, so a letter taking a value in rg but not grep still eats the next token in a cluster: `rtk grep -rt FOO .` consumes FOO as -t's value no matter what clap does. Noted as a search.rs defect, not fixed here. Separately, .claude/hooks/rtk-suggest.sh mapped both `rg` and `grep` to `rtk grep`, swapping engines. With --file-type gone, its suggestion for `rg -t rust FOO` became `rtk grep -t rust FOO`, which grep rejects. Now maps rg -> `rtk rg` and grep -> `rtk grep`. --- .claude/hooks/rtk-suggest.sh | 6 ++++- src/main.rs | 48 ++++++++++++++++++++++++----------- tests/search_compress_test.rs | 13 +++++++++- 3 files changed, 50 insertions(+), 17 deletions(-) diff --git a/.claude/hooks/rtk-suggest.sh b/.claude/hooks/rtk-suggest.sh index 80c356582e..dce24910ce 100755 --- a/.claude/hooks/rtk-suggest.sh +++ b/.claude/hooks/rtk-suggest.sh @@ -75,7 +75,11 @@ elif echo "$FIRST_CMD" | grep -qE '^cargo\s+fmt(\s|$)'; then elif echo "$FIRST_CMD" | grep -qE '^cat\s+'; then SUGGESTION=$(echo "$CMD" | sed 's/^cat /rtk read /') elif echo "$FIRST_CMD" | grep -qE '^(rg|grep)\s+'; then - SUGGESTION=$(echo "$CMD" | sed -E 's/^(rg|grep) /rtk grep /') + # Keep the engine. Mapping `rg` -> `rtk grep` swaps ripgrep for GNU grep, so + # rg-only flags break: `rg -t rust FOO` became `rtk grep -t rust FOO`, which + # grep rejects with `invalid option -- t`. `rtk rg` is the ripgrep-backed + # variant and takes rg's flags verbatim. + SUGGESTION=$(echo "$CMD" | sed -E 's/^rg /rtk rg /; s/^grep /rtk grep /') elif echo "$FIRST_CMD" | grep -qE '^ls(\s|$)'; then SUGGESTION=$(echo "$CMD" | sed 's/^ls/rtk ls/') elif echo "$FIRST_CMD" | grep -qE '^tree(\s|$)'; then diff --git a/src/main.rs b/src/main.rs index 17ef870714..06026b1146 100644 --- a/src/main.rs +++ b/src/main.rs @@ -319,8 +319,21 @@ enum Commands { // the native flag and either crashes clap or silently applies the wrong // semantics, defeating the `extra_args` passthrough. Keep them long-only // so native flags reach src/cmds/system/search.rs, the shared filter - // behind both `rtk grep` and `rtk rg`. (Completes the `-v`/`-n`/pattern/ + // behind both `rtk grep` and `rtk rg`. (Continues the `-v`/`-n`/pattern/ // path cleanup from 84616d1.) + // + // This does NOT clear every shadowed short. Clap's auto `-h` still wins + // over grep/rg's --no-filename before the first positional, and after it + // `-h` reaches search.rs's --version/--help check and execs unfiltered. + // Help handling is #2532's scope, not this variant's -- do not "finish" + // it here by adding disable_help_flag without reading that PR first. + // + // Removing a short here also does not make the flag safe end-to-end. + // search.rs shares one VALUE_FLAGS_SHORT table across both engines, so a + // letter that takes a value in rg but not grep (`-t`, `-T`, `-r`) still + // eats the next token inside a cluster: `rtk grep -rt FOO .` consumes + // FOO as -t's value regardless of what clap does. That is a search.rs + // defect, tracked separately. /// Max line length // No short: `-l` is grep's --files-with-matches. Bound to RTK's // --max-len (a `usize`), `rtk grep -l PATTERN` could not parse at all -- @@ -3792,16 +3805,6 @@ mod tests { ); } - #[test] - fn test_grep_parse_max_count_m_forwarded() { - // Native grep `-m N` (max-count). The value `5` must reach search.rs, - // not be eaten as rtk's `--max`. - assert_eq!( - grep_extra_args(&["rtk", "grep", "-m", "5", "FOO", "file"]).unwrap(), - vec!["-m", "5", "FOO", "file"] - ); - } - #[test] fn test_grep_parse_type_t_forwarded() { // `-t TYPE` is rg-only, so it must reach the engine rather than binding @@ -3815,6 +3818,19 @@ mod tests { ); } + #[test] + fn test_grep_file_type_option_is_gone() { + // `--file-type` was removed, not merely un-shorted. Guard against it + // creeping back: an unknown long flag is NOT a clap error here (the + // trailing slot accepts it), it must route to extra_args and be the + // engine's problem. If someone re-adds the option, clap binds it and + // extra_args loses these two tokens. + assert_eq!( + grep_extra_args(&["rtk", "grep", "--file-type", "rust", "FOO", "src/"]).unwrap(), + vec!["--file-type", "rust", "FOO", "src/"] + ); + } + // --- additional grep characterization (preserved behavior, was untested) --- /// Parse `rtk grep …` into the full `Grep` field set for inspection. @@ -3855,10 +3871,12 @@ mod tests { #[test] fn test_grep_options_after_pattern_stay_in_extra_args() { - // trailing_var_arg gotcha: once the pattern starts the trailing slot, - // later tokens — even rtk's own `--max-len` — are NOT parsed as options; - // they pass through verbatim. (Native grep ordering: flags come before - // the pattern.) + // Positional gotcha: once the pattern starts the trailing slot, later + // tokens — even rtk's own `--max-len` — are NOT parsed as options; they + // pass through verbatim. (Native grep ordering: flags come before the + // pattern.) The mechanism is `allow_hyphen_values` on `extra_args`, not + // `trailing_var_arg` — dropping the latter leaves this behavior intact, + // so do not read this test as a guard on `trailing_var_arg`. let (max_len, _, _, extra_args) = parse_grep(&["rtk", "grep", "FOO", "--max-len", "40"]).unwrap(); assert_eq!( diff --git a/tests/search_compress_test.rs b/tests/search_compress_test.rs index aa4445c7b2..c60707d891 100644 --- a/tests/search_compress_test.rs +++ b/tests/search_compress_test.rs @@ -17,6 +17,17 @@ fn rg_available() -> bool { .unwrap_or(false) } +/// `rtk grep` shells out to `grep`, never `rg` (`search.rs`: `Engine::Grep => "grep"`). +/// Guarding a grep-engine test on `rg_available()` makes it silently skip -- and +/// report success -- on a box without ripgrep, which is a false green. +fn grep_available() -> bool { + Command::new("grep") + .arg("--version") + .output() + .map(|o| o.status.success()) + .unwrap_or(false) +} + fn write_temp(content: &str) -> (tempfile::TempDir, std::path::PathBuf) { let dir = tempfile::tempdir().expect("tempdir"); let path = dir.path().join("test.txt"); @@ -320,7 +331,7 @@ fn small_grep_not_worse_than_plain() { #[test] fn max_len_long_option_still_binds_to_rtk() { - if !rg_available() { + if !grep_available() { return; } // The other half of #2628: dropping the `-l` short must not stop the long From cd0c7bc17f9718b9451ba309b19b6b188e6080f7 Mon Sep 17 00:00:00 2001 From: Brandon Harper <6859+devnulled@users.noreply.github.com> Date: Sun, 30 Aug 2026 00:28:06 -0600 Subject: [PATCH 6/8] test(grep): restore the -l end-to-end test, with a numeric pattern I deleted this test earlier on the reasoning that `-l` printed the same thing with and without the fix. That reasoning was wrong, and so was the mutant I used to "prove" it -- a word pattern. The behavior splits on whether the PATTERN parses as a usize: - Non-numeric ("TODO"): clap rejects, run_fallback re-runs raw grep, output and exit code are identical either way. This is the case I tested, which is why the mutant survived. - Numeric: clap ACCEPTS. `-l 8080 hit.txt miss.txt` sets max_len=8080 and leaves hit.txt as the pattern, miss.txt as the only path. No error, no fallback -- just an empty result and exit 1 on a file that does match. numeric_pattern_with_files_with_matches_flag pins it. Mutation-proved by restoring `short = 'l'`: `grep -l ` must find the match (#2628); status=Some(1) stderr= test result: FAILED. 0 passed; 1 failed Single-file form is worse still -- the filename becomes the pattern, no path is left, and rtk falls back to stdin: # mutant, one file $ rtk grep -l 8080 one.txt < /dev/null -> exit=1, no output $ rtk grep -l 8080 one.txt -> hangs on a terminal # fixed $ rtk grep -l 8080 one.txt -> one.txt, exit=0 Corrects the field comment, which repeated the wrong story ("could not parse at all", "losing all compaction"). Nothing is lost to compaction: `-l` is a has_format_flag passthrough on the fixed path too, so there are no savings for this flag either way. The cost was a wrong answer. Also repoints the last stale module reference. test_grep_version_routes_to _extra_args said `grep_cmd::run forwards it to rg --version`; grep_cmd.rs does not exist and the Grep arm routes to search::run, whose --version/--help check execs against the engine unfiltered. `grep -rn grep_cmd src/ tests/ docs/` is now empty. --- src/main.rs | 26 ++++++++++++++----- tests/search_compress_test.rs | 48 +++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/src/main.rs b/src/main.rs index 06026b1146..54d1ad8c32 100644 --- a/src/main.rs +++ b/src/main.rs @@ -335,11 +335,24 @@ enum Commands { // FOO as -t's value regardless of what clap does. That is a search.rs // defect, tracked separately. /// Max line length - // No short: `-l` is grep's --files-with-matches. Bound to RTK's - // --max-len (a `usize`), `rtk grep -l PATTERN` could not parse at all -- - // clap aborted with exit 2 and rtk fell back to raw grep, losing all - // compaction. Without the short, `-l` flows to extra_args and reaches - // grep/rg, which lists matching filenames as the user intended. + // No short: `-l` is grep's --files-with-matches. Binding it to --max-len + // (a `usize`) split behavior on whether the PATTERN parsed as a number. + // + // Non-numeric pattern: clap rejected it, run_fallback re-ran raw grep, + // and the user got the right answer. Wasteful, not harmful -- and note + // `-l` is a has_format_flag passthrough on the fixed path too, so no + // compaction was "lost"; there is none to lose for this flag. + // + // Numeric pattern: clap ACCEPTED it. `rtk grep -l 8080 a.txt b.txt` set + // max_len=8080 and left a.txt as the pattern, b.txt as the only path -- + // no error, no fallback, silently empty output and exit 1 on a file that + // does match. With a single file it is worse: the filename becomes the + // pattern, no path is left, and rtk falls back to reading stdin -- which + // exits 1 on /dev/null and hangs outright on an interactive terminal. + // That silent wrong answer, reached through a hook the user never opted + // into, is the real reason this short had to go. + // Pinned by tests::test_grep_parse_files_with_matches_l (clap layer) and + // numeric_pattern_with_files_with_matches_flag (end to end). #[arg(long, default_value = "80")] max_len: usize, /// Max results to show @@ -3889,7 +3902,8 @@ mod tests { #[test] fn test_grep_version_routes_to_extra_args() { // `--version` is not a subcommand flag (version isn't propagated), so it - // lands in extra_args and grep_cmd::run forwards it to rg --version. + // lands in extra_args, and search::run's --version/--help check execs it + // against the engine unfiltered. assert_eq!( grep_extra_args(&["rtk", "grep", "--version"]).unwrap(), vec!["--version"] diff --git a/tests/search_compress_test.rs b/tests/search_compress_test.rs index c60707d891..4eb1698c1f 100644 --- a/tests/search_compress_test.rs +++ b/tests/search_compress_test.rs @@ -327,6 +327,54 @@ fn small_grep_not_worse_than_plain() { ); } +// --- #2628: a numeric pattern must not bind to rtk's removed `-l` short --- + +#[test] +fn numeric_pattern_with_files_with_matches_flag() { + if !grep_available() { + return; + } + // The pattern MUST parse as a usize for this test to gate anything. With + // `-l` bound to `--max-len: usize`, a non-numeric pattern makes clap fail, + // and run_fallback then re-runs raw grep and prints the right answer -- so + // a word pattern passes with or without the fix. A numeric pattern instead + // binds silently: `-l 8080` sets max_len=8080, leaving the first filename + // as the pattern and the second as the only path. No error, no fallback, + // wrong answer. + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write(dir.path().join("hit.txt"), "listen on port 8080 today\n").expect("write"); + std::fs::write(dir.path().join("miss.txt"), "nothing numeric here\n").expect("write"); + let hit = dir.path().join("hit.txt"); + let miss = dir.path().join("miss.txt"); + + let out = rtk() + .args([ + "grep", + "-l", + "8080", + hit.to_str().unwrap(), + miss.to_str().unwrap(), + ]) + .output() + .expect("rtk grep"); + let stdout = String::from_utf8_lossy(&out.stdout); + + assert!( + out.status.success(), + "`grep -l ` must find the match (#2628); status={:?} stderr={}", + out.status.code(), + String::from_utf8_lossy(&out.stderr) + ); + assert!( + stdout.contains("hit.txt"), + "-l must report the file containing 8080:\n{stdout}" + ); + assert!( + !stdout.contains("miss.txt"), + "-l must not report the non-matching file:\n{stdout}" + ); +} + // --- #2628: rtk's own long options still bind after the short forms were removed --- #[test] From b3ae7be09ed351bc2fbcd983a19afa45da3cb2ac Mon Sep 17 00:00:00 2001 From: Brandon Harper <6859+devnulled@users.noreply.github.com> Date: Sun, 30 Aug 2026 16:47:36 -0600 Subject: [PATCH 7/8] fix(test): make the -t smoke assertion actually gate something Review caught that my replacement assertion was inert. `assert_fails` passes on ANY non-zero exit, and the base command already failed for an unrelated reason: $ rtk grep "pub fn" src/ grep: src/: Is a directory exit=2 So it passed whether or not `-t` was handled correctly. I replaced a wrong assertion with one that cannot fail. Adding `-r` was not enough either: with `-r` first, trailing_var_arg swallows `-t` before clap ever sees it, so the mutant still exits 2. The form has to put `-t` FIRST, use a pattern that MATCHES, and target a single file, so the only reason the command can fail is `-t` itself. Mutation-proved both ways by re-adding `--file-type` with `short = 't'`: mutant: rtk grep -t rust "fn main" src/main.rs -> exit=0 (assert_fails fires) fixed: rtk grep -t rust "fn main" src/main.rs -> exit=2 (assert_fails passes) The comment above the assertion records the three ways to make it inert again, since I hit two of them. Adding `-r` to the two adjacent assertions fixes them too -- they were red on develop because `grep PATTERN ` without `-r` exits 2. Both line 260 and 261 were failing, not just 260 as I said earlier. Corrects two false statements from earlier commit messages in this branch: - The FEATURES.md `--line-numbers | -n` row was STALE, not fabricated. The option existed from 7d69299 (2026-02-15) until 84616d1 (2026-06-10) -- the same commit the struct NOTE credits for the -v/-n cleanup. Removing the row is still right; my justification was wrong. - `scripts/test-all.sh` is not wired into CI, but it is not "referenced only from .github/copilot-instructions.md" either. It is a documented contributor command: CONTRIBUTING.md:253 and CLAUDE.md:46. --- scripts/test-all.sh | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/scripts/test-all.sh b/scripts/test-all.sh index 7d88121173..488fe4e011 100755 --- a/scripts/test-all.sh +++ b/scripts/test-all.sh @@ -257,12 +257,24 @@ fi section "Grep" -assert_ok "rtk grep pattern" rtk grep "pub fn" src/ -assert_contains "rtk grep finds results" "pub fn" rtk grep "pub fn" src/ +# All three need -r: `grep PATTERN ` without it exits 2 ("Is a directory"), +# which made the first two assertions red and the third one INERT -- assert_fails +# passed on the directory error whether or not `-t` was handled correctly. +assert_ok "rtk grep pattern" rtk grep -r "pub fn" src/ +assert_contains "rtk grep finds results" "pub fn" rtk grep -r "pub fn" src/ # `-t` is rg-only. rtk used to swallow it via its own --file-type option, which -# never reached the engine, so this asserted a silent no-op. --file-type is gone; -# `-t` now flows to the engine, so pin it against the engine that has it. -assert_fails "rtk grep -t rejected by grep" rtk grep "pub fn" src/ -t rust +# never reached the engine, so the old assertion here pinned a silent no-op. +# +# This form is deliberate and fragile to get right. The pattern MUST match and +# the path MUST be a single file with `-t` FIRST, so that the only reason the +# command can fail is `-t` itself: +# - re-add --file-type with `short = 't'` -> -t binds, "fn main" matches, +# exit 0, and assert_fails fires. +# - as shipped -> -t reaches grep, exit 2. +# A directory path (exit 2, "Is a directory"), a non-matching pattern (exit 1), +# or `-t` after another flag (trailing_var_arg swallows it before clap sees it) +# all make this assertion inert -- it passes either way. +assert_fails "rtk grep -t rejected by grep" rtk grep -t rust "fn main" src/main.rs if command -v rg >/dev/null 2>&1; then assert_ok "rtk rg with file type" rtk rg "pub fn" src/ -t rust else From eb3f8148726f3725f1de5b3cb546991d2b562df9 Mon Sep 17 00:00:00 2001 From: Nicolas Le Cam Date: Mon, 31 Aug 2026 01:58:35 +0200 Subject: [PATCH 8/8] refactor(grep): trim comments and align the siblings of the flag change Condense the Grep variant and its tests to the comment rules in CONTRIBUTING.md (explain why, not what), and carry the -l/-t change through the files that still assumed the old behavior. - scripts/test-aristote.sh: add -r to the two grep assertions, since `grep PATTERN ` exits 2, and move the type filter to `rtk rg`, the engine where -t is valid. - docs/usage/FEATURES.md: extra args go to the engine actually invoked, not always to rg; drop the per-row shortcut justifications. - tests/search_compress_test.rs: guard grep-engine tests on grep_available() instead of rg_available(), which skipped them into a false green on a box without ripgrep. - .claude/hooks/rtk-suggest.sh: reverted; the rg -> rtk rg fix is correct but belongs in its own PR. BREAKING CHANGE: `rtk grep --file-type` and its `-t` short are removed. The option never reached the engine, so it was a silent no-op; `-t` now flows through, which means `rtk rg -t rust` filters by type while `rtk grep -t rust` returns grep's own `invalid option -- 't'`. `-l` and `-m` under `rtk grep` are likewise the native grep flags now rather than rtk's --max-len and --max. Co-Authored-By: Claude Opus 5 --- .claude/hooks/rtk-suggest.sh | 6 +- docs/usage/FEATURES.md | 14 +++-- scripts/test-all.sh | 19 ++---- scripts/test-aristote.sh | 13 +++- src/main.rs | 110 +++++++++------------------------- tests/search_compress_test.rs | 20 +++---- 6 files changed, 61 insertions(+), 121 deletions(-) diff --git a/.claude/hooks/rtk-suggest.sh b/.claude/hooks/rtk-suggest.sh index dce24910ce..80c356582e 100755 --- a/.claude/hooks/rtk-suggest.sh +++ b/.claude/hooks/rtk-suggest.sh @@ -75,11 +75,7 @@ elif echo "$FIRST_CMD" | grep -qE '^cargo\s+fmt(\s|$)'; then elif echo "$FIRST_CMD" | grep -qE '^cat\s+'; then SUGGESTION=$(echo "$CMD" | sed 's/^cat /rtk read /') elif echo "$FIRST_CMD" | grep -qE '^(rg|grep)\s+'; then - # Keep the engine. Mapping `rg` -> `rtk grep` swaps ripgrep for GNU grep, so - # rg-only flags break: `rg -t rust FOO` became `rtk grep -t rust FOO`, which - # grep rejects with `invalid option -- t`. `rtk rg` is the ripgrep-backed - # variant and takes rg's flags verbatim. - SUGGESTION=$(echo "$CMD" | sed -E 's/^rg /rtk rg /; s/^grep /rtk grep /') + SUGGESTION=$(echo "$CMD" | sed -E 's/^(rg|grep) /rtk grep /') elif echo "$FIRST_CMD" | grep -qE '^ls(\s|$)'; then SUGGESTION=$(echo "$CMD" | sed 's/^ls/rtk ls/') elif echo "$FIRST_CMD" | grep -qE '^tree(\s|$)'; then diff --git a/docs/usage/FEATURES.md b/docs/usage/FEATURES.md index 0df0632d72..8d646693b7 100644 --- a/docs/usage/FEATURES.md +++ b/docs/usage/FEATURES.md @@ -215,17 +215,19 @@ rtk grep [chemin] [options] | Option | Court | Defaut | Description | |--------|-------|--------|-------------| -| `--max-len` | | 80 | Longueur maximale de ligne (pas de raccourci, `-l` est reserve a `grep --files-with-matches`) | -| `--max` | | 200 | Nombre maximum de resultats (pas de raccourci, `-m` est reserve a `grep --max-count`) | -| `--context-only` | | non | Afficher uniquement le contexte du match (pas de raccourci, `-c` est reserve a `grep --count`) | +| `--max-len` | | 80 | Longueur maximale de ligne | +| `--max` | | 200 | Nombre maximum de resultats | +| `--context-only` | | non | Afficher uniquement le contexte du match | -Les arguments supplementaires sont transmis a `rg` (ripgrep). Les flags qui changent le format de sortie (`-c`, `-l`, `-L`, `-o`, `-Z`) passent directement a `rg`/`grep` sans filtrage RTK. +Aucune de ces options n'a de raccourci : `-l`, `-m` et `-c` sont reserves aux flags natifs de meme lettre (`--files-with-matches`, `--max-count`, `--count`) et sont transmis au moteur. -Il n'y a pas d'option `--file-type` : `-t TYPE` est propre a ripgrep et est transmis tel quel au moteur. `rtk rg -t rust` filtre donc par type, tandis que `rtk grep -t rust` remonte l'erreur de grep lui-meme (`invalid option -- 't'`), grep n'ayant pas ce flag. +Les arguments supplementaires sont transmis au moteur reellement invoque -- `grep` pour `rtk grep`, `rg` pour `rtk rg`, RTK ne substituant jamais l'un a l'autre. Un flag propre a ripgrep (`--glob`, `-t`) n'est donc valide que sous `rtk rg`. Les flags qui changent le format de sortie (`-c`, `-l`, `-L`, `-o`, `-Z`) passent directement au moteur sans filtrage RTK. + +Il n'y a pas d'option `--file-type` : `rtk rg -t rust` filtre par type, `rtk grep -t rust` remonte l'erreur de grep (`invalid option -- 't'`). **Avant / Apres :** ``` -# rg "fn run" (20 lignes) # rtk grep "fn run" (10 lignes) +# grep -rn "fn run" (20 lignes) # rtk grep -rn "fn run" (10 lignes) src/git.rs:45:pub fn run(...) src/git.rs src/git.rs:120:fn run_status(...) 45: pub fn run(...) src/ls.rs:12:pub fn run(...) 120: fn run_status(...) diff --git a/scripts/test-all.sh b/scripts/test-all.sh index 488fe4e011..ecba1d9a4b 100755 --- a/scripts/test-all.sh +++ b/scripts/test-all.sh @@ -258,22 +258,13 @@ fi section "Grep" # All three need -r: `grep PATTERN ` without it exits 2 ("Is a directory"), -# which made the first two assertions red and the third one INERT -- assert_fails -# passed on the directory error whether or not `-t` was handled correctly. +# which made the first two assertions red and the third one inert. assert_ok "rtk grep pattern" rtk grep -r "pub fn" src/ assert_contains "rtk grep finds results" "pub fn" rtk grep -r "pub fn" src/ -# `-t` is rg-only. rtk used to swallow it via its own --file-type option, which -# never reached the engine, so the old assertion here pinned a silent no-op. -# -# This form is deliberate and fragile to get right. The pattern MUST match and -# the path MUST be a single file with `-t` FIRST, so that the only reason the -# command can fail is `-t` itself: -# - re-add --file-type with `short = 't'` -> -t binds, "fn main" matches, -# exit 0, and assert_fails fires. -# - as shipped -> -t reaches grep, exit 2. -# A directory path (exit 2, "Is a directory"), a non-matching pattern (exit 1), -# or `-t` after another flag (trailing_var_arg swallows it before clap sees it) -# all make this assertion inert -- it passes either way. +# `-t` is rg-only. The form matters: the pattern must match, the path must be a +# single file, and `-t` must come first, so the only reason to fail is `-t` +# itself -- a directory, a non-matching pattern, or `-t` after another flag all +# make this assertion pass either way. assert_fails "rtk grep -t rejected by grep" rtk grep -t rust "fn main" src/main.rs if command -v rg >/dev/null 2>&1; then assert_ok "rtk rg with file type" rtk rg "pub fn" src/ -t rust diff --git a/scripts/test-aristote.sh b/scripts/test-aristote.sh index 371ce90332..b93aa3ef06 100755 --- a/scripts/test-aristote.sh +++ b/scripts/test-aristote.sh @@ -121,9 +121,16 @@ assert_ok "rtk read --max-lines 10" rtk read --max-lines 10 "$ARISTO section "Grep" -assert_ok "rtk grep import" rtk grep "import" "$ARISTOTE/src" -assert_ok "rtk grep with type filter" rtk grep "useState" "$ARISTOTE/src" -t tsx -assert_contains "rtk grep finds components" "import" rtk grep "import" "$ARISTOTE/src" +# `grep PATTERN ` without -r exits 2 ("Is a directory"). +assert_ok "rtk grep import" rtk grep -r "import" "$ARISTOTE/src" +assert_contains "rtk grep finds components" "import" rtk grep -r "import" "$ARISTOTE/src" + +# `-t` is rg-only, so the type filter belongs to the rg engine. +if command -v rg >/dev/null 2>&1; then + assert_ok "rtk rg with type filter" rtk rg "useState" "$ARISTOTE/src" -t tsx +else + skip_test "rtk rg with type filter" "rg not installed" +fi # ── 4. Git ─────────────────────────────────────────── diff --git a/src/main.rs b/src/main.rs index 54d1ad8c32..c2f9e67327 100644 --- a/src/main.rs +++ b/src/main.rs @@ -313,46 +313,15 @@ enum Commands { /// Compact grep - strips whitespace, truncates, groups by file Grep { - // NOTE: rtk's own tuning options here are intentionally long-only. - // Their natural short forms collide with native grep/rg flags of the - // same letter, which users routinely pass. A short option here shadows - // the native flag and either crashes clap or silently applies the wrong - // semantics, defeating the `extra_args` passthrough. Keep them long-only - // so native flags reach src/cmds/system/search.rs, the shared filter - // behind both `rtk grep` and `rtk rg`. (Continues the `-v`/`-n`/pattern/ - // path cleanup from 84616d1.) - // - // This does NOT clear every shadowed short. Clap's auto `-h` still wins - // over grep/rg's --no-filename before the first positional, and after it - // `-h` reaches search.rs's --version/--help check and execs unfiltered. - // Help handling is #2532's scope, not this variant's -- do not "finish" - // it here by adding disable_help_flag without reading that PR first. - // - // Removing a short here also does not make the flag safe end-to-end. - // search.rs shares one VALUE_FLAGS_SHORT table across both engines, so a - // letter that takes a value in rg but not grep (`-t`, `-T`, `-r`) still - // eats the next token inside a cluster: `rtk grep -rt FOO .` consumes - // FOO as -t's value regardless of what clap does. That is a search.rs - // defect, tracked separately. + // rtk's own options here are long-only: a short form shadows the native + // grep/rg flag of the same letter and captures it before it can reach + // src/cmds/system/search.rs, the shared filter behind `rtk grep` and + // `rtk rg`. /// Max line length - // No short: `-l` is grep's --files-with-matches. Binding it to --max-len - // (a `usize`) split behavior on whether the PATTERN parsed as a number. - // - // Non-numeric pattern: clap rejected it, run_fallback re-ran raw grep, - // and the user got the right answer. Wasteful, not harmful -- and note - // `-l` is a has_format_flag passthrough on the fixed path too, so no - // compaction was "lost"; there is none to lose for this flag. - // - // Numeric pattern: clap ACCEPTED it. `rtk grep -l 8080 a.txt b.txt` set - // max_len=8080 and left a.txt as the pattern, b.txt as the only path -- - // no error, no fallback, silently empty output and exit 1 on a file that - // does match. With a single file it is worse: the filename becomes the - // pattern, no path is left, and rtk falls back to reading stdin -- which - // exits 1 on /dev/null and hangs outright on an interactive terminal. - // That silent wrong answer, reached through a hook the user never opted - // into, is the real reason this short had to go. - // Pinned by tests::test_grep_parse_files_with_matches_l (clap layer) and - // numeric_pattern_with_files_with_matches_flag (end to end). + // No short: `-l` is grep's --files-with-matches. Bound to a `usize` it + // accepted numeric patterns -- `rtk grep -l 8080 a.txt b.txt` set + // max_len=8080, took a.txt as the pattern, and returned empty output + // with no error. #[arg(long, default_value = "80")] max_len: usize, /// Max results to show @@ -367,11 +336,9 @@ enum Commands { /// Show only match context (not full line) #[arg(long)] context_only: bool, - // No --file-type / `-t`: `-t TYPE` is rg's type filter, and the option - // never did anything under `rtk grep` -- it was destructured away at the - // call site and never reached the engine. Removed rather than left as a - // parse-only no-op; `rtk rg -t rust` still filters by type, since it - // forwards `-t` to real ripgrep. + // No --file-type: `-t TYPE` is rg's type filter and the option never + // reached the engine, so `rtk rg -t rust` filters by type while + // `rtk grep -t rust` surfaces grep's own error. /// Pattern, path, and any grep/rg flags (e.g. -v, -i, -A 3, --glob, --version) #[arg(trailing_var_arg = true, allow_hyphen_values = true)] extra_args: Vec, @@ -3738,13 +3705,9 @@ mod tests { // --- grep argument routing (clap layer) --- // - // The `Grep` variant funnels the pattern, path, and every native grep/rg - // flag into a single `trailing_var_arg` slot so `src/cmds/system/search.rs` - // -- the shared filter behind both `rtk grep` and `rtk rg` -- can parse them - // with full grep/rg semantics. Commit 84616d1 ("fix(grep): stabilize - // argument parsing") moved to this design but left the struct's own short - // options in place, where they still shadowed native flags of the same - // letter. These tests pin the routing. + // The `Grep` variant funnels the pattern, path, and every native grep/rg flag + // into one trailing slot so `src/cmds/system/search.rs` parses them with full + // grep/rg semantics. These tests pin that routing. /// Parse `rtk grep …` and return the captured `extra_args`, or `None` if /// clap rejects the invocation (e.g. a colliding short option mis-binds). @@ -3755,9 +3718,6 @@ mod tests { } } - // Characterization: behavior that must be PRESERVED (passes today, untested - // until now). Guards the 84616d1 design against regression. - #[test] fn test_grep_parse_simple_pattern_path() { assert_eq!( @@ -3787,8 +3747,8 @@ mod tests { #[test] fn test_grep_parse_invert_match_v_not_shadowed() { - // Regression guard for 84616d1: `-v` (invert-match) must reach grep, - // not be captured as rtk's top-level verbose flag. + // `-v` (invert-match) must reach grep, not be captured as rtk's + // top-level verbose flag. assert_eq!( grep_extra_args(&["rtk", "grep", "-v", "FOO", "file"]).unwrap(), vec!["-v", "FOO", "file"] @@ -3797,18 +3757,13 @@ mod tests { #[test] fn test_grep_parse_alternation_pattern_intact() { - // Extended-regex alternation must pass through untouched (#1436 lineage). + // Extended-regex alternation must pass through untouched. assert_eq!( grep_extra_args(&["rtk", "grep", "-nE", "a|b", "file"]).unwrap(), vec!["-nE", "a|b", "file"] ); } - // The fix: native `-l`/`-m`/`-t` must route to search.rs, not be captured by - // the struct's own short options. `-l` mis-bound to `max_len: usize`, so clap - // rejected the pattern outright; `-m`/`-t` silently swallowed their value with - // the wrong semantics. (`-m` was fixed separately in #3259; pinned here too.) - #[test] fn test_grep_parse_files_with_matches_l() { // Native grep `-l` (files-with-matches). Must parse and pass `-l` through. @@ -3820,11 +3775,8 @@ mod tests { #[test] fn test_grep_parse_type_t_forwarded() { - // `-t TYPE` is rg-only, so it must reach the engine rather than binding - // to an rtk option. search.rs lists `t` in VALUE_FLAGS_SHORT and forwards - // it verbatim: under `rtk rg` it filters by type, under `rtk grep` it - // surfaces grep's own `invalid option -- 't'` instead of being silently - // dropped. + // `-t TYPE` is rg-only and must reach the engine: `rtk rg` filters by + // type, `rtk grep` surfaces grep's own `invalid option -- 't'`. assert_eq!( grep_extra_args(&["rtk", "grep", "-t", "rust", "FOO", "src/"]).unwrap(), vec!["-t", "rust", "FOO", "src/"] @@ -3833,19 +3785,15 @@ mod tests { #[test] fn test_grep_file_type_option_is_gone() { - // `--file-type` was removed, not merely un-shorted. Guard against it - // creeping back: an unknown long flag is NOT a clap error here (the - // trailing slot accepts it), it must route to extra_args and be the - // engine's problem. If someone re-adds the option, clap binds it and - // extra_args loses these two tokens. + // `--file-type` was removed, not merely un-shorted: an unknown long flag + // routes to extra_args and is the engine's problem. Re-adding the option + // would bind it here and cost extra_args these two tokens. assert_eq!( grep_extra_args(&["rtk", "grep", "--file-type", "rust", "FOO", "src/"]).unwrap(), vec!["--file-type", "rust", "FOO", "src/"] ); } - // --- additional grep characterization (preserved behavior, was untested) --- - /// Parse `rtk grep …` into the full `Grep` field set for inspection. fn parse_grep(args: &[&str]) -> Result<(usize, usize, bool, Vec), clap::Error> { match Cli::try_parse_from(args)?.command { @@ -3861,9 +3809,8 @@ mod tests { #[test] fn test_grep_long_options_bind_before_pattern() { - // rtk's tuning knobs are long-only (the short forms were removed to stop - // shadowing native grep flags). They must still bind when placed before - // the pattern, and only the pattern/path land in extra_args. + // Long-only knobs must still bind when placed before the pattern, with + // only the pattern/path landing in extra_args. let (max_len, max, context_only, extra_args) = parse_grep(&[ "rtk", "grep", @@ -3884,12 +3831,9 @@ mod tests { #[test] fn test_grep_options_after_pattern_stay_in_extra_args() { - // Positional gotcha: once the pattern starts the trailing slot, later - // tokens — even rtk's own `--max-len` — are NOT parsed as options; they - // pass through verbatim. (Native grep ordering: flags come before the - // pattern.) The mechanism is `allow_hyphen_values` on `extra_args`, not - // `trailing_var_arg` — dropping the latter leaves this behavior intact, - // so do not read this test as a guard on `trailing_var_arg`. + // Once the pattern starts the trailing slot, later tokens — even rtk's + // own `--max-len` — pass through verbatim rather than binding as options. + // The mechanism is `allow_hyphen_values`, not `trailing_var_arg`. let (max_len, _, _, extra_args) = parse_grep(&["rtk", "grep", "FOO", "--max-len", "40"]).unwrap(); assert_eq!( diff --git a/tests/search_compress_test.rs b/tests/search_compress_test.rs index 4eb1698c1f..702bfd925b 100644 --- a/tests/search_compress_test.rs +++ b/tests/search_compress_test.rs @@ -39,7 +39,7 @@ fn write_temp(content: &str) -> (tempfile::TempDir, std::path::PathBuf) { #[test] fn single_file_context_shown_without_header() { - if !rg_available() { + if !grep_available() { return; } let long = "x".repeat(120); @@ -64,7 +64,7 @@ fn single_file_context_shown_without_header() { #[test] fn after_context_uses_dash_separator_for_context_lines() { - if !rg_available() { + if !grep_available() { return; } let (_dir, path) = write_temp("MATCH\nafter1\n"); @@ -83,7 +83,7 @@ fn after_context_uses_dash_separator_for_context_lines() { #[test] fn capped_single_file_shows_header() { - if !rg_available() { + if !grep_available() { return; } let filler: String = (0..40).map(|i| format!("w{i} ")).collect(); @@ -103,7 +103,7 @@ fn capped_single_file_shows_header() { #[test] fn true_no_match_exits_1() { - if !rg_available() { + if !grep_available() { return; } let (_dir, path) = write_temp("hello world\n"); @@ -145,7 +145,7 @@ fn no_line_number_flag_produces_output_not_zero_matches() { #[test] fn no_filename_flag_produces_output_not_zero_matches() { - if !rg_available() { + if !grep_available() { return; } let (_dir, path) = write_temp("hello world\n"); @@ -242,7 +242,7 @@ fn count_tokens(s: &str) -> usize { // Covers #545: grep savings are measured against the real grep output. #[test] fn bulky_grep_yields_token_savings() { - if !rg_available() { + if !rg_available() || !grep_available() { return; } let filler: String = (0..50).map(|i| format!("word{i} ")).collect(); @@ -275,7 +275,7 @@ fn bulky_grep_yields_token_savings() { #[test] fn grep_only_flags_fall_back_to_system_grep() { - if !rg_available() { + if !grep_available() { return; } let dir = tempfile::tempdir().expect("tempdir"); @@ -307,7 +307,7 @@ fn grep_only_flags_fall_back_to_system_grep() { #[test] fn small_grep_not_worse_than_plain() { - if !rg_available() { + if !grep_available() { return; } let (_dir, path) = write_temp("foo\n"); @@ -327,7 +327,7 @@ fn small_grep_not_worse_than_plain() { ); } -// --- #2628: a numeric pattern must not bind to rtk's removed `-l` short --- +// --- a numeric pattern must not bind to rtk's removed `-l` short --- #[test] fn numeric_pattern_with_files_with_matches_flag() { @@ -428,7 +428,7 @@ fn max_len_long_option_still_binds_to_rtk() { #[test] fn bundled_files_with_matches_cluster_lists_files() { - if !rg_available() { + if !grep_available() { return; } let dir = tempfile::tempdir().expect("tempdir");