Repository navigation
fix(grep): stop -l/-t shadowing native grep flags - #2628
Merged
Merged
Conversation
5 tasks done
kylehgc
added a commit
to kylehgc/rtk
that referenced
this pull request
Aug 1, 2026
…_cmd.rs The cherry-picked rtk-ai#2628 comments reference src/cmds/system/grep_cmd.rs, which upstream has since reworked into search.rs. Comment-only amendment; kept separate from the contributor's commit per adoption policy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kylehgc
added a commit
to kylehgc/rtk
that referenced
this pull request
Aug 1, 2026
fix(grep): stop -l/-m/-t shadowing native grep flags (adopt upstream rtk-ai#2628)
KuSh
requested changes
Aug 29, 2026
Collaborator
There was a problem hiding this comment.
Reviewed the -l/-m/-t short-flag removal. The direction is right and the -l fix is real, but this needs a rebase, and two of the new comments assert things that aren't true on develop.
Verified against develop (a93d63b) with this branch applied: all 8 new clap tests pass, cargo test is green.
1. It conflicts with develop — the -m hunk already landed
develop already dropped the -m short from max in #3259, with its own rationale comment. This branch conflicts in src/main.rs at exactly that block, and its side of the conflict deletes that comment. Only the -l and -t removals are net-new.
2. grep_cmd.rs no longer exists
Both the new NOTE block and the -t test comment point at src/cmds/system/grep_cmd.rs. On develop that file was replaced by src/cmds/system/search.rs, shared by rtk grep and rtk rg. The claim that it "already handles" -t doesn't hold there.
3. -t goes from silently ignored to a hard error
On the merged code, rtk grep -t rust TELEMETRY src/core now prints grep: invalid option -- 't'. Before the change the same command silently returned unfiltered results, because file_type is destructured as file_type: _ at the call site and never reaches search::run.
Erroring is arguably the faithful outcome — rtk rg -t rust still works, since rg does have -t — but it's a user-visible change worth calling out in the PR description. And it leaves --file-type as an option that parses and then does nothing.
4. Docs still advertise the removed shorts
docs/usage/FEATURES.md line 218 (--max-len | -l) and line 221 (--file-type | -t) still list the short forms. Line 219 (--max) is already correct after #3259.
The -l half of the change works exactly as intended: rtk grep -rl TELEMETRY src/core now lists filenames through the has_format_flag passthrough, where before it died in clap on --max-len.
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 rtk-ai#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. rtk-ai#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 rtk-ai#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.
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.
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.
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
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 rtk-ai#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`.
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 <number>` must find the match (rtk-ai#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.
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 <dir>` 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.
devnulled
force-pushed
the
fix/grep-hook-arg-ordering
branch
from
August 30, 2026 23:04
8a028e1 to
b3ae7be
Compare
devnulled
changed the title
Contributor
Author
|
@KuSh all four findings addressed — rebased onto |
KuSh
previously requested changes
Aug 30, 2026
Collaborator
There was a problem hiding this comment.
Second pass on b3ae7be. All four earlier threads are properly fixed — the outright --file-type removal and the new docs paragraph are exactly the right call. Verified locally: cargo test --bin rtk grep 25/25, search_compress_test 16/16, fmt and clippy clean, and the numeric-pattern -l case now gives the right answer end to end.
KuSh
dismissed
their stale review
August 30, 2026 23:53
Superseded — pushing these fixes directly to the branch instead.
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 <dir>` 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 <noreply@anthropic.com>
KuSh
approved these changes
Aug 30, 2026
Merged
This was referenced Sep 26, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
rtk grep's--max-lenlong-only (dropping the-lshort) and removes--file-type/-tentirely, so the native grep/rg flags of those letters reach the engine (src/cmds/system/search.rs) instead of binding to rtk's own options.-mwas already fixed ondevelopby fix(grep): free -m for GNU --max-count instead of rtk --max #3259 — this finishes the cleanup that 84616d1 started and fix(grep): free -m for GNU --max-count instead of rtk --max #3259 continued.-lbug had two faces, split on whether the pattern parsed as a number:rtk grep -l telemetry src/): clap rejectedtelemetryas ausize,run_fallbackre-ran raw grep — right answer, zero savings. Wasteful, not harmful.rtk grep -l 8080 a.txt b.txt): clap accepted it —max_len=8080,a.txtbecame the pattern,b.txtthe only path. No error, no fallback, silently wrong output (and with a single file, a stdin fallback that hangs an interactive terminal). This silent wrong answer, reached through a hook rewrite the user never typed, is the real motivation.-t:-t TYPEis rg-only. Before,rtk grep -t rust PATsilently dropped the flag — it bound to--file-type, which was destructured away at the call site and never reached the engine. Now-tis forwarded verbatim:rtk rg -t rustfilters by type as always, whilertk grep -t rustsurfaces grep's owninvalid option -- 't'. Loud beats silently-ignored, so--file-typeis removed outright rather than left as a parse-only no-op..claude/hooks/rtk-suggest.sh:rg …now suggestsrtk rg …(wasrtk grep …, which swapped ripgrep for GNU grep and broke rg-only flags like-t).scripts/benchmark.sh: the-l 40bench invocation used the removed short; now--max-len 40before the pattern.scripts/test-all.sh: the three grep smoke assertions were red or inert (missing-rmadegrep PAT <dir>exit 2 regardless); rewritten so the-tassertion can only fail because of-titself.docs/usage/FEATURES.md: drops the advertised-l/-tshorts, explains why, and removes a stale--line-numbers | -nrow (theGrepvariant has no such option).Context / how this differs from the other grep PRs
Not the broad "grep rejects
-r/-E/-A" problem from #2614 (largely resolved by 84616d1). This is the residual: rtk's ownGrepshort options shadowing native flags. Complements (does not overlap) #2224's rg-only-flag fallback handling; independent of #2532 (-hpassthrough — deliberately untouched here, see the NOTE in the variant).Test plan
cargo fmt --all --checkclean,cargo clippy --all-targetsclean,cargo test: 2852 passed; 0 failed; 8 ignored plus all integration suites green (incl. the two new end-to-end tests intests/search_compress_test.rs).-l(the silent-wrong-answer case):-t:#[arg(short = 't', long)] file_typefails exactlytest_grep_parse_type_t_forwarded+test_grep_file_type_option_is_gone(15 passed; 2 failed), and flips the smoke command above to exit 0 — so the newassert_failssmoke assertion genuinely fires on regression. Restored tree re-verified green.-rn,-A 3,-v,-nE 'a|b',--version,--all pinned by new clap-layer tests.Refs: #2614, #1604, #1436