Skip to content

fix(grep): treat bare | as literal pipe (BRE), not rg alternation - #6

Merged
ryanmurf merged 1 commit into
masterfrom
fix/grep-bare-pipe-literal
May 30, 2026
Merged

ryanmurf merged 1 commit into
masterfrom
fix/grep-bare-pipe-literal

Conversation

@ryanmurf

Copy link
Copy Markdown

Problem

rtk grep accepts grep BRE syntax, where \| is alternation and a bare | is a literal pipe. The pattern is then handed to ripgrep, whose default (Rust-regex) engine uses the opposite convention: | is alternation and \| is a literal pipe.

The previous code translated only \| → |:

let rg_pattern = pattern.replace(r"\|", "|");

That fixed the alternation case but left a bare | to be reinterpreted by rg as alternation. So a user searching for the literal substring a|b:

rtk grep 'a|b'   # wanted: lines containing the literal "a|b"

silently over-matched — rg returned every line containing a OR b. This is the over-broadening filter-corruption class: wrong, broadened search results fed to the model with no indication anything was changed.

Verified against rg 14.1.1:

pattern grep BRE intent old rtk → rg result
a|b a OR b a|b (alternation) correct (3 matches)
a|b (bare pipe) literal "a|b" a|b (alternation) wrong: 3, wanted 1

(The first row is a backslash-pipe b; the second is a bare-pipe b. GitHub table escaping obscures the literal characters — see the tests for unambiguous cases.)

Fix

Replace the naive replace() with translate_bre_pipes(), a single scanning pass that swaps both pipe meanings correctly and respects backslash escaping (\\ stays a literal backslash; other escapes like \. \( pass through verbatim).

Tests

  • test_bare_pipe_treated_as_literal — the regression: bare pipe becomes an escaped (literal) pipe for rg
  • test_bre_alternation_translated — escaped-pipe alternation still maps to rg alternation
  • test_mixed_literal_and_alternation_pipes — both meanings in one pattern, no clobbering
  • test_escaped_backslash_before_pipe — escaped backslash before a pipe handled correctly
  • test_other_escapes_passthrough — \. \( \) untouched
  • test_bare_pipe_does_not_overmatch_via_rg — end-to-end: runs rg on a fixture and asserts the literal-pipe search matches exactly one line, not the broadened set

Full suite green locally: cargo test (1728 passed), cargo fmt --check, cargo clippy --all-targets -- -D unsafe_code.

`rtk grep` accepts grep BRE syntax where `\|` is alternation and a bare
`|` is a LITERAL pipe. The pattern was passed to ripgrep, whose default
engine has the opposite convention: `|` is alternation and `\|` is a
literal pipe.

The previous translation only rewrote `\|` → `|`, fixing the alternation
case but leaving a bare `|` to be reinterpreted by rg as alternation. So
`rtk grep 'a|b'` (a user searching for the literal substring "a|b")
silently over-matched: rg returned every line containing `a` OR `b`,
feeding extra false matches to the caller. This is exactly the
over-broadening filter-corruption class — wrong data handed to the model.

Replace the naive replace() with translate_bre_pipes(), a single scanning
pass that swaps BOTH pipe meanings correctly and respects backslash
escaping (`\\` stays a literal backslash; other escapes pass through).

Tests: bare-pipe-as-literal, mixed literal+alternation, escaped backslash
before pipe, other-escape passthrough, and an end-to-end rg assertion that
the literal-pipe search matches exactly one line (not the broadened set).
@ryanmurf
ryanmurf enabled auto-merge (squash) May 30, 2026 00:58
@github-actions

Copy link
Copy Markdown

👋 Thanks for the PR! It looks like this targets master, but all PRs should target the develop branch.

Please update the base branch:

  1. Click Edit at the top right of this PR
  2. Change the base branch from master to develop

See CONTRIBUTING.md for details.

@ryanmurf
ryanmurf merged commit f30963e into master May 30, 2026
16 of 17 checks passed
@ryanmurf
ryanmurf deleted the fix/grep-bare-pipe-literal branch May 30, 2026 01:05
ryanmurf added a commit that referenced this pull request May 30, 2026
The wc output filter parsed each line with split_whitespace() and assumed
the filename was a single trailing token (parts.last() / parts[3]). GNU wc
prints the filename verbatim, so `wc -l "my file.txt"` emits `3 my file.txt`
and the filter silently corrupted the name the model then reads:

  - Lines/Words/Bytes/Chars multi-file: "my file.txt" -> "file.txt"
    (only the last path segment survived)
  - Full multi-file: "30 96 978 my file.txt" -> took parts[3] = "my",
    dropping "file.txt" entirely
  - Mixed single-file: leaked a filename fragment into the counts
    ("30 96 my" instead of "30 96")

This is the same class of defect as the grep BRE-pipe bug (#6): a filter
that mangles output the model reasons over.

Fix: add split_counts_and_name(line, max_counts) which peels off exactly the
known number of leading numeric count columns and treats the remainder as one
opaque, verbatim filename (spaces preserved). All multi/single formatters and
the common-prefix scan now use it.

Adds regression tests for every affected mode (proven to fail pre-fix) plus
direct contract tests for the helper, including the count-column cap and
trailing-name edge cases.

Co-authored-by: Claude (tron2 agent) <noreply@anthropic.com>
ryanmurf added a commit that referenced this pull request May 30, 2026
#9)

The tree filter stripped any line matching the loose substring heuristic
`line.contains("director") && line.contains("file")`, intended to drop
tree's trailing summary line (e.g. "5 directories, 23 files"). But this
silently dropped legitimate entries whose filenames contain both words,
e.g. `director-files.txt`, `file_directory.json`, or a path segment like
`my directory with files` — the model never saw those files existed.

Same bug class as the grep BRE-pipe over-match (#6) and the wc
space-filename drop (#8): an output transform that over-matches and
silently strips real content the model reasons over.

Fix: anchor the summary detector to tree's exact summary shape with a
whole-line regex (`^\d+ director(y|ies)(, N files)?(, N links)?$`)
instead of a substring contains-check. Regression tests prove the dropped
filenames now survive and the real summary line is still stripped,
including the `, N links` platform variation.

Co-authored-by: rtk-sweep-agent <agent@herodevs.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant