Skip to content

fix(grep): correctly handle all flag shapes and never exceed raw output#2550

Merged
aeppling merged 2 commits into
developfrom
fix/grep-context-flags
Jun 23, 2026
Merged

fix(grep): correctly handle all flag shapes and never exceed raw output#2550
aeppling merged 2 commits into
developfrom
fix/grep-context-flags

Conversation

@aeppling

Copy link
Copy Markdown
Contributor

Summary

Make rtk grep produce correct output for every valid grep invocation and never cost more tokens than plain grep.

The #2333 arg-parsing rework routed many flag shapes through the grouping reparse, which silently dropped unparseable lines and returned success, producing wrong/empty results (and grep failed on --include) that pushed agents to unfiltered rtk proxy grep.

  • Context (-A/-B/-C): grouped + compressed instead of dropped; header counts matches only.
  • Unrepresentable output (-N, -I, --color, -p, --heading, --field-match-separator, binary notices): passed through verbatim, never a false "0 matches".
  • Format/shape flags (-c/-l/-o/-Z/--column/-b/--vimgrep/--null-data): clean passthrough, no NUL leak.
  • grep-only syntax (--include/--exclude/…): ripgrep fast path, falls back to system grep on rg error.
  • Never-worse guard: never emits more than plain file:line:content grep.

Related

Test plan

  • cargo fmt --all && cargo clippy --all-targets && cargo test --all — clean, 2242 tests pass.
  • New integration tests: context compression, safety-net passthrough, NUL-free format/shape, grep-only-flag fallback, token savings, never-worse guard.
  • Correctness battery vs bare grep across 18 shapes — no NUL, no false 0-matches, no dropped matches, never bigger than raw.
  • Real hook path verified in tracking DB — routed (not proxy), 52% on bulky, 0% (never negative) on small.

The grep argument-parsing rework routed many flag shapes through the
grouping reparse, which silently dropped any line it could not parse and
returned a success exit. Common invocations produced wrong or empty
output -- and grep-only syntax like `--include` failed outright -- which
pushed agents back to unfiltered `rtk proxy grep`.

Every valid grep invocation now produces correct output:

- Context (`-A`/`-B`/`-C`) is grouped and compressed instead of dropped;
  the header counts matches only and context lines are shown.
- Output the reparse cannot represent (`-N`, `-I`, `--color`, `-p`,
  `--heading`, binary-file notices, ...) is passed through verbatim
  instead of reported as a false "0 matches".
- Format/shape flags (`-c`, `-l`, `-o`, `-Z`, `--column`, `-b`,
  `--vimgrep`, `--null-data`) pass through cleanly with no NUL leak.
- grep-only syntax (`--include`, `--exclude`, ...) works: ripgrep is the
  fast path and rtk falls back to system grep when ripgrep rejects a flag.
- rtk grep never emits more than plain grep: if grouping is not smaller
  than `file:line:content`, the raw form is shown.

Resolves #2543.
@alexisLefebvre

Copy link
Copy Markdown

Nice — the unparsed_signal safety net looks like it'd catch this case by falling back to passthrough. One small thing I noticed: I couldn't spot a regression test for the exact #2543 shape, a bundled files-with-matches cluster (-rln/-ln). The new tests all use standalone flags (--column, -c, -o, -N, -I, …), so -l-inside-a-cluster isn't directly exercised. Might be worth a small test asserting rtk grep -rln <pat> <dir> lists the files rather than 0 matches, just to pin the original report — but no strong opinion if it's already covered elsewhere.

Repro + a 0.42.2 confirmation, in case useful: #2543 (comment)

🤖 Generated with Claude Code

Review feedback on #2550: the flag-shape tests all used standalone flags, leaving the exact #2543 repro -- a bundled -rln/-ln files-with-matches cluster -- unexercised. Assert rtk grep -rln <pat> <dir> lists matching files instead of a false '0 matches'.
@aeppling

aeppling commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @alexisLefebvre , good catch. Added a regression test for the exact #2543 shape in 3a59d23: bundled_files_with_matches_cluster_lists_files asserts rtk grep -rln <pat> <dir> lists every matching file (and excludes non-matching ones) instead of a false 0 matches.

@aeppling aeppling merged commit ee9e2f8 into develop Jun 23, 2026
17 checks passed
This was referenced Jun 23, 2026
@EtaCassiopeia

Copy link
Copy Markdown

@aeppling Hey, just wondering if there was a reason my PR wasn't merged and the work was redone instead. Does the project accept external contributions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

grep: -r / -rn rejected on 0.42.4 (and -rln returns a false "0 matches" on latest develop)

4 participants