fix: grep BRE alternation, missing-path exit code, and flag-level did-you-mean - #84
Conversation
davydog187
left a comment
There was a problem hiding this comment.
Reviewed against real GNU grep 3.11 (ubuntu:24.04). The branch's full suite (5022 tests) and mix bash_fixtures.verify both pass, so everything below is a gap the added tests don't cover.
The one that matters is the -w/-x interaction: enabling \| as real alternation means those flags now wrap an ungrouped pattern, which is a false-positive class this PR itself introduces. The rest are smaller divergences from GNU.
|
|
||
| true -> | ||
| pattern | ||
| flags.w -> "\\b" <> base_pattern <> "\\b" |
There was a problem hiding this comment.
-w/-x wrap an ungrouped alternation, so the anchor binds to only one branch.
Now that \| compiles to a real |, grep -w 'foo\|bar' builds \bfoo|bar\b, which PCRE reads as (\bfoo)|(bar\b).
| this branch | GNU grep 3.11 | |
|---|---|---|
echo foobar | grep -w 'foo|bar' |
matches, exit 0 | no match, exit 1 |
echo foobaz | grep -x 'foo|bar' |
matches, exit 0 | no match, exit 1 |
This is a false-positive class the PR creates: before the change \| was a literal, so -w/-x combined with \| could not produce a spurious match.
flags.w -> "\\b(?:" <> base_pattern <> ")\\b"
flags.x -> "^(?:" <> base_pattern <> ")$"| if flags.e_ext or flags.p_pcre do | ||
| pattern | ||
| else | ||
| String.replace(pattern, "\\|", "|") |
There was a problem hiding this comment.
The substring replace also consumes the pipe after an escaped backslash.
a\\|b in BRE is a literal backslash followed by a literal |. The blind String.replace/3 rewrites it to a\|b, which PCRE reads as the literal string a|b:
$ echo 'a|b' | grep 'a\\|b'
# this branch: matches, exit 0
# GNU grep: no match, exit 1
A scan that skips the character after each backslash — or Regex.replace(~r/\\(.)/, ...) with a replacement function — handles it.
Related, and pre-existing rather than introduced here: bare | is still passed straight through as alternation, so after this PR there is no spelling that matches a literal pipe in default BRE mode. Worth escaping bare | in the same pass.
| do: "grep: #{file}: Is a directory\n" | ||
|
|
||
| defp format_file_error(file, _error), | ||
| do: "grep: #{file}: No such file or directory\n" |
There was a problem hiding this comment.
Every non-:eisdir error is reported as "No such file or directory", which reintroduces exactly the bug FS.strerror/1 exists to prevent.
FS.read_file/2 can return :enotdir, :eacces, :eloop, :erofs, and :eio, and FS.strerror/1 (lib/just_bash/fs/fs.ex:345-358) already accepts a %VFS.Error{} directly. Verified with grep hi /a.txt/x where /a.txt is a regular file:
this branch: grep: /a.txt/x: No such file or directory
GNU grep: grep: /a.txt/x: Not a directory
A chmod 000 file likewise gives GNU Permission denied. Both clauses collapse to:
defp format_file_error(file, error), do: "grep: #{file}: #{FS.strerror(error)}\n"| # otherwise a file-level error outranks "no match found". | ||
| exit_code = | ||
| cond do | ||
| flags.q and any_match -> 0 |
There was a problem hiding this comment.
-q reports errors for files real grep never opens.
The contract is "exit immediately with zero status if any match is found", so GNU stops at the first match and never stats the later operands. With a.txt matching:
$ grep -q hi a.txt nope.txt
# GNU: exit 0, empty stderr
# this branch: exit 0, but "grep: nope.txt: No such file or directory" on stderr
The exit code is right, the diagnostic is spurious. Suppressing stderr when flags.q and any_match is the cheap approximation of the short-circuit.
| cli.commands | ||
| |> resolve_group_commands(group_path) | ||
| |> Enum.reject(&(&1.name == current_name or Command.group?(&1))) | ||
| |> Enum.find(&flag_declared?(&1.flags, flag)) |
There was a problem hiding this comment.
Enum.find names an arbitrary sibling when several declare the same flag.
--verbose, --json, and --force are commonly declared by many leaves in one group. The lookup returns the first sibling in declaration order, so dol log metric --verbose yields a confidently-worded did you mean 'dol log <whichever-is-first>'? that is very likely not the command the caller wanted.
The comment just above makes the case that an exact declaration match is an unambiguous signal — that justification only holds when the match is unique. Suggest Enum.filter and suppressing the hint (or listing all candidates) when more than one sibling matches.
`compile_pattern/3` passed the pattern straight to `Regex.compile/2`
(PCRE), where `\|` is an escaped literal pipe. Real `grep` without
`-E`/`-P` is BRE, where `\|` is alternation. The PCRE pattern compiled
cleanly either way, so the `{:error, _}` fallback never fired — a
pattern like `grep -ril "a\|b"` just silently matched a literal pipe
character that occurs nowhere, producing an empty result and exit 1
instead of a match.
Rewrite only `\|` -> `|` when neither `-E` nor `-P` is given. Full BRE
emulation (bare `(`, `)`, `{` as literals, `\(...\)` as groups, `\{n,m\}`
as bounds, etc.) is a much larger surface with more ways to land
partially right, and is left alone; `\|` is the dominant agent idiom
for alternation and the one that caused a real incident (see
elixir-ai-tools/just_bash's downstream issue: two production agents
independently ran `grep -ril "bodyweight\|weight\|weigh" /memory`,
got an empty result, and concluded a fact wasn't recorded when it was).
Also: a missing or unreadable path was silently swallowed
(`{:error, _} -> {acc, had_match, fs}`), yielding exit 1 with no
stderr — indistinguishable from "no match". Real grep exits 2 and
names the path. Fixed to match, including -q's documented "exit 0 on
a match even if an error was also detected" priority inversion.
Added fixture-corpus cases (recorded against real GNU grep in the
Docker corpus) for both: BRE alternation matching/non-matching/-E
opt-out, and the missing-path exit code and stderr message.
A leaf rejecting an undeclared flag (`unknown option: --weight`) gave
no pointer even when a sibling command in the same group declares
that exact flag. A caller who guessed `dol log metric --weight`
instead of `dol log weight` was one hop from correct and got nothing
to route it — only a usage line for the leaf it happened to guess.
`ArgParser.parse/3` gains an `:on_unknown_flag` hook: called with the
bare flag name whenever it would otherwise be a hard error, its
non-nil return is appended to the message. `JustBash.CLI` wires this
up in `dispatch_leaf/6`, since only the CLI tree has a notion of
"sibling commands in the same group" — `ArgParser` itself only ever
sees one leaf's flag spec. The lookup is an exact declaration match
(a flag either is or isn't declared elsewhere in the group), not the
Jaro-distance fuzzy match `Help.unknown_subcommand/4` uses for
subcommand typos — that's the right tool for a name typo, not for
"which leaf owns this flag".
dol log metric: unknown option: --weight — did you mean 'dol log weight'?
Lands on stderr, which is the recovery point a calling agent actually
reads and self-corrects from.
…ernation, and -q short-circuits Four defects, each recorded against real GNU grep in the Docker corpus before being fixed. `-w`/`-x` wrapped an ungrouped pattern. Now that `\|` compiles to a real `|`, `grep -w 'foo\|bar'` built `\bfoo|bar\b`, which PCRE reads as `(\bfoo)|(bar\b)` — each anchor bound to one branch, so `echo foobar` matched at exit 0 where GNU reports no match. This is a false-positive class the alternation fix created: while `\|` was a literal, `-w`/`-x` crossed with `\|` could not produce a spurious match. Both arms now wrap the pattern in `(?:...)`. `String.replace(pattern, "\\|", "|")` consumed the pipe after an escaped backslash. `a\\|b` in BRE is a literal backslash followed by a literal pipe, and the blind replace rewrote it to `a\|b`, which PCRE reads as the literal string `a|b` — so `echo 'a|b' | grep 'a\\|b'` matched where GNU does not. A left-to-right scan now consumes each backslash together with the character it escapes, so an escaped character is never re-read as syntax. A bare `|` was still passed straight through as PCRE alternation. In BRE it is an ordinary character, so `echo ab | grep 'a|b'` matched "a" and exited 0 where GNU finds no literal `a|b` and exits 1 — and there was no spelling at all that matched a literal pipe in default BRE mode. The same scan escapes it. `-q` reported errors for files real grep never opens. The contract is "exit immediately with zero status if any match is found", so GNU stops at the first matching operand and never stats the rest: `grep -q hi a.txt nope.txt` leaves stderr empty. The reduce now halts there, which is that short-circuit rather than a blanket suppression — an operand read *before* the match still reports its error, as GNU also does, and `error_message_test` records both orders. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`sibling_flag_hint/3` used `Enum.find`, so when several leaves in a group declared the same flag it named whichever came first in declaration order. `--verbose`, `--json` and `--force` are routinely declared by many leaves at once, and there `dol log metric --verbose` produced a confidently-worded `did you mean 'dol log event'?` pointing at an arbitrary sibling. The hint's justification is that an exact declaration match is an unambiguous signal — which only holds while the match is unique. Match on a one-element list so an ambiguous match yields no hint, and leave the unique case exactly as it was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5ba834a to
1bfdc5f
Compare
What broke
A production agent ran
grep -ril "bodyweight\|weight\|weigh" /memory, gotan empty result at exit 1, and concluded the fact wasn't recorded — it was,
in two of the files under
/memory. Full writeup, including a seconddownstream fix for the flag-level recovery path, is in
the downstream issue.
1.
grep's\|is BRE alternation, not an escaped literal pipecompile_pattern/3(lib/just_bash/commands/grep.ex) passed the patternstraight to
Regex.compile/2(PCRE), where\|is an escaped literal pipe.Real
grepwithout-E/-Pis BRE, where\|is alternation. The PCREpattern compiles cleanly either way, so the existing
{:error, _} -> Regex.escape(...)fallback never fires — the pattern just silently matchesa literal pipe character that occurs nowhere:
Fix shipped: rewrite only
\|->|when neither-Enor-Pisgiven.
\|is the dominant agent idiom for alternation (and the one thatcaused the incident above); full BRE emulation (bare
(,),{asliterals,
\(...\)as groups,\{n,m\}as bounds, etc.) is a much largersurface with more ways to land partially right, so it's deliberately left
alone rather than half-translated.
Also fixed on the same path: a missing or unreadable file was silently
swallowed (
{:error, _} -> {acc, had_match, fs}), producing exit 1 with nostderr — indistinguishable from "no match" from the caller's side. Real
grep exits 2 and names the path (
grep: FILE: No such file or directory).Fixed to match, including
-q's documented "exit 0 on a match even if anerror was also detected on another file" priority inversion.
Fixture-corpus cases recorded against real GNU grep (Ubuntu 24.04, in the
Docker corpus) cover both: alternation matching/non-matching,
-Eopting out of the rewrite, and the missing-path exit code/stderr message.
2. An unknown flag now names the sibling command that declares it
Separately:
dol log metric --weight 207.8(a plausible guess — the rightcommand is
dol log weight) failed loudly withunknown option: --weightand a usage line, but nothing pointed at the sibling leaf that actually
declares
--weight.allow_unknown_flagsalready defaults tofalse, sothere's no silent-drop bug here — just a missing recovery pointer at the
single point (stderr) an agent reads to self-correct.
ArgParser.parse/3gains an:on_unknown_flaghook, called with the bareflag name whenever it would otherwise be a hard error; a non-nil return is
appended to the message.
JustBash.CLIwires it up indispatch_leaf/6,scanning sibling commands in the same group for one that declares the
flag — an exact declaration match, not the Jaro-distance fuzzy match
Help.unknown_subcommand/4already uses for subcommand-name typos (that'sthe right tool for a name typo, not for "which leaf owns this flag").
New tests in
test/cli/routing_test.exscover: a sibling's long flag, asibling's short flag, no suggestion when no sibling declares the flag, and
no cross-group leakage (a flag declared by a leaf in an unrelated group is
not suggested).
Testing
mix test— full suite green, 5086 passed / 5 excluded (pre-existing:liveexclusions, unrelated to this change)mix bash_fixtures.verify— corpus soundmix format --check-formatted— clean on all changed files