fix: a flag a command does not implement is an error, not a filename - #72
Conversation
davydog187
left a comment
There was a problem hiding this comment.
Two lenses were applied to this PR: a behavioural-parity pass (run each claim against the branch and against 82ee9e5, compare to GNU coreutils 9) and a test-strength pass (does the new test suite actually distinguish fixed from unfixed?). Eleven candidate findings went in; seven survive after merging duplicates and refuting the rest. Everything below was reproduced in a worktree at 52562f1, with 82ee9e5 as the baseline.
The core of the change is right, and the GNU wording is faithful. The findings are about the shapes the fix does not cover and about the tests not being able to see them.
1. A cluster containing an implemented value flag is rejected, and the diagnostic names a flag the command supports
major — lib/just_bash/flag_parser.ex:213 (offender search at :219)
try_attached_value_flag/4 only peels an attached value off character 0 of the cluster, so a getopt cluster of booleans followed by a value option falls through parse_combined_flags/4 and try_numeric_flag/4 into unknown_flag/2. There the offender is picked with:
Enum.find(graphemes, flag_str, fn char -> Map.get(lookup, char) not in spec.boolean end)The predicate treats any non-boolean character as the offender. Real getopt stops on the first character not in the option string at all; a value flag inside a cluster consumes the remainder as its argument. So the message names a flag the command does implement — the same "blames something that was never wrong" shape the PR set out to fix for head -q.
Verified in the PR worktree, with /f.txt:
| command | this branch | baseline 82ee9e5 |
GNU coreutils 9 |
|---|---|---|---|
sort -nk2 /f.txt |
exit 2, sort: invalid option -- 'k' |
exit 0, empty | exit 0, sorted |
sort -rk1 /f.txt |
exit 2, sort: invalid option -- 'k' |
exit 0, empty | exit 0, sorted |
sort -rt: -k2 /f.txt |
exit 2, sort: invalid option -- 't' |
exit 0, empty | exit 0, sorted |
sort -t: -rk2 /f.txt |
exit 2, sort: invalid option -- 'k' |
exit 0, empty | exit 0, sorted |
Both -t and -k are implemented by this sort. sort is the only FlagParser caller with both boolean and value flags, so it is the whole blast radius — but every combination is affected (-nk, -rk, -uk, -fk, -rt, -nt). Not a correctness regression (these were silently absorbed before), but the diagnostic now actively misinforms: an agent reading invalid option -- 'k' will drop -k and get differently sorted output rather than learning that clustered value flags are unsupported.
Suggested fix: scan past leading boolean characters in try_attached_value_flag/4 before looking for an attached value, and handle the "value flag is last in the cluster, argument is the next argv" case in parse_flag/4. Failing that, at minimum restrict the offender search to characters that appear in no part of the spec, so the message never names an implemented flag.
Test gap: every new cluster test (sort -rQ, -alQ, -Qal) uses a boolean-only spec. Nothing exercises a cluster containing a :value or :multi_value flag, which is why sort -rt: -k2 ships green.
2. The registry matrix's :strict bucket cannot distinguish rejecting a flag from turning it into a filename
major — test/commands/unknown_flags_test.exs:367
probe/2 classifies purely on exit code and stderr emptiness:
cond do
result.exit_code == 0 -> :exit_zero
result.stderr == "" -> :quiet
true -> :strict
endIt never checks that stdout is empty or that the diagnostic is about an option. So a command that demotes the unknown flag to an operand and then fails to open it is indistinguishable from one that rejects the flag — even though the table comment calls :strict "What every option-parsing command should do."
Concretely, in the PR worktree:
cat -Z /f.txt→ exit 1, stdout"b:2\na:1\n", stderrcat: -Z: No such file or directory→ classified:strictwc -Z /f.txt→ exit 1, stdout" 2 2 8 /f.txt\n…", stderrwc: -Z: No such file or directory→ classified:strict
GNU for cat -Z is cat: invalid option -- 'Z'. Around twenty :strict entries are strict only because the demoted flag names a file that does not exist (cat, wc, sha256sum, shasum, rm, ./source, cd, markdown/md), and comm, env, nl, readlink, which emit invalid option -- '-Z' with GNU's dash duplicated.
The sharper demonstration: I ran the same classification logic on baseline 82ee9e5, before this fix:
ls -Z => :strict (ls: cannot access '-Z': No such file or directory)
head -Z => :strict (head: cannot open '-Z' for reading: …)
tail -Z => :strict (tail: cannot open '-Z' for reading: …)
cp -Z => :strict (cp: missing destination file operand after '-Z')
Four of the seven FlagParser callers already classified :strict on the unfixed tree. Only sort/uniq (:exit_zero) and grep (:quiet) change category. So the matrix certifies as correct exactly the head -q / ls -Q half of issue #68 — the half the PR describes as "the message names the wrong problem."
Suggested fix: have probe/2 also require result.stdout == "" and result.stderr =~ ~r/invalid option|unrecognized option|illegal option/ for :strict, and add a fourth honest bucket (:misblamed?) for the demote-then-fail commands. That keeps the list-can-only-shrink property the table is built around and makes the comment true.
3. tr's new -- clause opens a fresh exit-0 / empty-output path
major — lib/just_bash/commands/tr.ex:38 (catch-all at :138)
The new clause pushes everything after -- into sets without checking the arity tr needs:
defp parse_args(["--" | rest], opts) do
parse_args([], %{opts | sets: Enum.reverse(rest) ++ opts.sets})
endWhen that leaves exactly one set and neither -d nor -s is set, execution falls through every run/2 head into defp run(_input, _opts), do: "" — empty stdout, exit 0, no stderr. That is precisely the failure class issue #68 is about.
Verified:
$ echo abc | tr -- -d branch: exit 0, stdout "", stderr "" baseline: exit 0, stdout "abc\n"
$ echo abc | tr -- x branch: exit 0, stdout "", stderr "" baseline: exit 0, stdout "abc\n"
GNU coreutils 9: exit 1, tr: missing operand after ‘-d’ / Two strings must be given when translating.
So the diff replaced wrong-but-visible output with the silent-empty result, in the same PR whose thesis is that silent-empty is the worst available outcome. The root cause is pre-existing — plain tr abc also reaches the catch-all — but the new -- clause is a new route into it, and the registry matrix's "tr" => :strict classification only holds for the two probe flags it happens to test.
Suggested fix: guard on set count in parse_args/2 — one set with no -d/-s is tr: missing operand after 'X' + Two strings must be given when translating. at exit 1; zero sets is the existing tr: missing operand. That closes the pre-existing hole and the new one together.
4. :integer is declared but never validated, so head -nq raises out of JustBash.exec/2
major — lib/just_bash/flag_parser.ex:242 (spec at lib/just_bash/commands/head.ex:12, test at test/flag_parser_test.exs:172)
defp parse_value(value, flag_atom, spec) do
with true <- flag_atom in Map.get(spec, :integer, []),
{num, ""} <- Integer.parse(value) do
num
else
_ -> value
end
endThe else conflates "this flag is not an integer flag" with "this integer flag got garbage", handing the raw string back to a caller that has just declared the flag numeric. :integer is the one place in the new design that knows the value must be a count, and it declines to say so.
Verified on both branches:
$ head -n abc /f.txt ** (FunctionClauseError) no function clause matching in Enum.take/2
$ head -nq /f.txt ** (FunctionClauseError) no function clause matching in Enum.take/2
$ head -n '' /f.txt ** (FunctionClauseError) …
$ head -c xy /f.txt exit 0, prints the whole file (the quiet twin)
An unhandled exception escaping JustBash.exec/2 — out of a library whose purpose is safely running untrusted shell. GNU: exit 1, head: invalid number of lines: 'abc'.
The crash pre-exists on main, so this is not a regression. Two things make it in-scope anyway: -nq is reachable straight from this PR's own theme (an unimplemented head flag clustered after -n), and the PR rewrites this exact function while adding test "leaves a non-numeric value alone even when declared :integer" (test/flag_parser_test.exs:172) asserting {:ok, %{c: "all"}, []} — which locks the crashing behaviour in as intended.
Suggested fix: distinguish the two else cases and return {:error, {:invalid_value, flag, value}}, rendered as GNU words it. It is the third getopt error class alongside the two the PR did add, and the channel now exists at zero extra cost. Then flip the test at :172 to assert the error.
5. --help and --version are hard errors on all eight commands, and the advice they print is a command that also fails
minor — lib/just_bash/flag_parser.ex:106
$ sort --help
exit 2
sort: unrecognized option '--help'
Try 'sort --help' for more information.
Followed literally, the advice produces the same error again. Identical for --version and for ls, head, tail, uniq, cp, grep, tr. Baseline was not better (sort --help was exit 0/empty; ls --help blamed a missing file), so this is not a regression.
But issue #68's stated motivation is the agent feedback loop — "the model can only learn what a command supports by running it." This PR closes the first dead end and creates a second one, and the new Try '<cmd> --help' line is what advertises it.
Suggested fix: intercept --help in FlagParser.parse/2 (or add a shared FlagParser.usage/2 that renders the spec's own flag list) and exit 0. It costs little, makes the emitted advice honest, and turns the spec into the discoverability surface the issue is asking for.
6. A new test passes on a run that returns exit 0, empty stdout and empty stderr
minor — test/commands/unknown_flags_test.exs:129
test "an option-shaped operand after -- is not rejected as a flag" do
{result, _} = JustBash.exec(bash(), "sort -- -Q")
refute result.stderr =~ "invalid option"
endsort -- -Q actually returns exit 0, stdout "", stderr "" — because get_content/3 (lib/just_bash/commands/sort.ex:58) maps {:error, _} from FS.read_file to {"", bash.fs}. GNU: exit 2, sort: cannot read: -Q: No such file or directory.
The unreadable-file behaviour is a genuinely separate bug and the PR discloses it under "Left out" — not asking for it here. The problem is the test: the headline symptom of issue #68 is still reproducible one -- away from the command the issue quotes, and a test added by this PR walks straight over it without noticing. A refute … =~ against a run that produced no stderr at all asserts nothing.
Suggested fix: assert the positive — that -Q reached sort as an operand, e.g. by pointing it at a file that exists (sort -- /f.txt with the expected sorted stdout), so the test fails if -- handling breaks.
7. sort's parse_key_spec/1 integer clause is now unreachable
minor — lib/just_bash/commands/sort.ex:111
defp parse_key_spec(spec) when is_integer(spec), do: {spec, nil}This existed because the old parse_value/1 coerced every integer-looking value, so sort -k 2 produced [2]. With coercion now gated on :integer and sort's @flag_spec declaring no :integer key, :k values are always binaries; try_numeric_flag/4 only ever writes :n, which sort declares boolean. Verified: sort -k 2 /f.txt and sort -k2 /f.txt both now reach the is_binary clause.
Dialyzer will not flag it (the guard is satisfiable in principle) and no test covers it, so it will sit there implying sort still accepts integer key specs. It also returns {spec, nil}, which the caller indexes as modifiers[:numeric] — safe only by accident, since nil[:numeric] would raise if it were ever reached.
Suggested fix: delete the clause in this PR, since this PR is what made it dead.
Considered and dismissed
"Per-command rejection tests are hand-written one case per command rather than enumerated" (test/commands/unknown_flags_test.exs:15) — dismissed as duplicative. The complaint is real in the abstract (growth-by-example, per issue #70), but every concrete cell it names is already reported above with a traced failure: the missing cluster-with-a-value-flag row is finding 1, and the weak enumeration is finding 2. Restating the shape of the table adds no new information, and "these seven tests should have been a table" without a distinct failing input is a structural preference. Verified separately that the coverage gaps it speculates about are not live bugs: cp -R /f.txt /g.txt and cp --recursive /f.txt /h.txt both exit 0 through the new :aliases merge, so the alias path is untested but not broken.
Two duplicate pairs were merged rather than dropped. Both lenses independently found the cluster-offender bug (flag_parser.ex:213) — merged into finding 1, keeping the sort -rt: -k2 reproduction, which is sharper than sort -nk2 because both flags in it are implemented. Both independently found the registry-matrix weakness — merged into finding 2, keeping the stdout-non-empty evidence and adding the baseline classification run, which is the decisive proof. Both independently found the :integer non-validation — merged into finding 4, keeping the observation that a new test blesses the fallback.
Nothing was refuted outright as factually wrong: every behavioural claim reproduced exactly as described, on the branch and on 82ee9e5.
|
All seven findings reproduced on
1 — clusters (
|
| command | before | now | GNU |
|---|---|---|---|
sort -nk2 /f.txt |
exit 2, invalid option -- 'k' |
exit 0, a 1\nb 2\n |
exit 0, sorted |
sort -rt: -k2 /f.txt |
exit 2, invalid option -- 't' |
exit 0, sorted | exit 0, sorted |
sort -rk 2 /f.txt |
exit 2 | exit 0 (value flag ends cluster, takes next argv) | same |
sort -nQk2 /f.txt |
— | exit 2, invalid option -- 'Q' |
same |
New tests use specs that carry :value/:multi_value flags, which the old cluster tests never did.
2 — registry matrix (ef72579)
probe/2 now requires empty stdout and a diagnostic that names an option (invalid|unrecognized|illegal option) before it calls a command :strict. A fourth bucket, :misblamed, holds the ~20 commands that exit non-zero only because the demoted flag named a file that does not exist.
The decisive check the review asked for — the strengthened classifier run against 82ee9e5:
cp => :misblamed head => :misblamed ls => :misblamed tail => :misblamed
sort => :exit_zero uniq => :exit_zero grep => :quiet tr => [:misblamed, :exit_zero]
All eight are outside :strict on the unfixed tree, where the old classifier certified four of them. The matrix test fails on 82ee9e5 for every caller the branch claims to fix. A new test also pins those eight names to :strict in the table, so the table cannot be edited to quietly demote one.
3 — tr arity (55342c1)
One set with neither -d nor -s is now an error rather than a silent fall-through to defp run(_input, _opts), do: "". This closes the new -- route and the pre-existing one (tr x) together:
$ echo abc | tr -- -d
tr: missing operand after '-d'
Two strings must be given when translating.
Try 'tr --help' for more information. (exit 1, matching GNU)
tr with no sets at all now prints the Try --help line too, as GNU does.
4 — :integer validation (3dcbd0e)
A bad count is now the third getopt error class, {:invalid_value, label, value}, worded as GNU words it, with no Try --help line because GNU prints none for this one:
$ head -n abc /f.txt head: invalid number of lines: 'abc' (exit 1)
$ head -nq /f.txt head: invalid number of lines: 'q' (exit 1)
$ head -n '' /f.txt head: invalid number of lines: '' (exit 1)
$ head -c xy /f.txt head: invalid number of bytes: 'xy' (exit 1)
All three of the first ones previously raised a FunctionClauseError out of JustBash.exec/2; the fourth printed the whole file at exit 0. The noun comes from a new :value_labels spec key, required of every :integer flag. test/flag_parser_test.exs:172 — the test that locked the old fallback in — now asserts the error.
5 — --help (f2f4629)
FlagParser.parse/2 returns :help for --help, and FlagParser.help/2 renders the usage from the spec, so it lists exactly what the parser accepts and cannot drift:
$ sort --help
Usage: sort [OPTION]... [FILE]...
Options this shell implements:
-f
-k VALUE
-n
-r
-t VALUE
-u
All eight commands answer at exit 0, tr included. -- still wins, so sort -- --help is an operand. --version stays an unrecognized option, and the advice it prints is now honest.
6 — the sort -- -Q test (cf3f9f8)
The test was a refute ... =~ against a run with no stderr at all, so it asserted nothing. Fixing it required fixing sort: get_content/3 mapped every read error to {"", fs}.
$ sort -- -Q sort: cannot read: -Q: No such file or directory (exit 2, matching GNU)
$ sort -- /f.txt exit 0, sorted
A lone - is now standard input rather than a file named -, which is both GNU's behaviour and what keeps printf 'b\na\n' | sort - meaningful; that test now asserts the sorted output instead of only the exit code.
7 — dead clause (cf3f9f8)
parse_key_spec/1's is_integer clause removed. Verified unreachable (sort -k 2 and sort -k2 both reach the is_binary clause), and it returned {spec, nil} where the caller does modifiers[:numeric], so it was wrong as well as dead.
Gates
mix compile --warnings-as-errors Compiling 168 files (.ex) / Generated just_bash app (no warnings)
mix format --check-formatted clean
mix credo --strict 5149 mods/funs, found 1 refactoring opportunity
(test/support/banned_fixture_apply.ex - the intentional fixture)
mix test 2 doctests, 62 properties, 4818 tests, 0 failures (5 excluded)
mix dialyzer Total errors: 13, Skipped: 13, Unnecessary Skips: 0 - passed
Noted, not fixed (out of scope for these findings)
echo abc | tr -cs astill returns""at exit 0 (GNU printsabc), andtr a b c/tr -d a bstill reachrun/2's catch-all where GNU saysextra operand. Pre-existing, not reachable through the--clause this branch added, so left alone.test/property_test.exs:423("an option date does not implement never exits 0") is flaky on this branch, independent of these changes:date -Rfexits 1 with theusage: date [-u] ...block, which does not contain thedate:prefix the property asserts. Reproduced identically on52562f1before any of this work.datedoes not useFlagParser; belongs to the fix: date rejects unknown arguments instead of returning today at exit 0 #66 line of work.comm,env,nl,readlinkandwhichclassify:strictbut word the short option asinvalid option -- '-Z', duplicating GNU's dash. Genuine rejections, so the bucket is right; the wording is a separate nit.
Verification of review fixesIndependently verified at Per-finding
Gates (re-run here, not reported)New problem introduced by the fix commits[minor] The exit code is now right and this is a clear improvement over swallowing the error, but the Noted, not counted against the fix
|
|
Closed the last open item: What was wrong
Before (
|
Final verificationIndependent re-verification of the third round on Per-item result
The GNU templates, re-derived hereI checked each error kind against
The symlink-cycle row is worth calling out: The new test can failMutated the fix out twice and confirmed
Working tree restored to a clean Regression sweepRan a 78-command corpus through
What was swept and found clean:
One thing the sweep surfaced that is not attributable to this PR, recorded so it is not It only looks new for Gates (re-run at
|
Round 4:
|
| kind | why read_file/2 cannot return it |
|---|---|
:eexist |
mkdir/symlink/link refusing a name that is already taken |
:enotempty |
rmdir on a directory that still has entries |
:eacces |
Memory.link/3 hard-linking a non-file; no read path consults mode bits |
:erofs |
a read-only mount, which this FS has no way to declare |
:enotsup |
the POSIX shim's symlink/link stubs on backends that have neither |
:einval |
readlink on a non-symlink, and the backend's one computed-kind Error.new/2 |
:exdev |
a rename that crosses a mount boundary |
:eio |
strerror names it for completeness; no backend here constructs it |
The backend has exactly one Error.new/2 whose kind is computed rather than literal — stream_read/3
forwarding a VFS.StreamOptions rejection, which read_file/2 cannot trigger because it passes no
options. The test pins that count at 1, so a second such site fails loudly instead of silently
shrinking what the derivation can see.
Before / after
The new test is genuinely red without the row — it was written first and watched fail:
BEFORE (:eloop row absent, derivation in place)
1) test sort command the unreadable-operand table accounts for every error kind that exists
unaccounted: MapSet.new([:eloop]), stale: MapSet.new([])
code: assert MapSet.union(tabled, accounted) == universe
12 tests, 1 failure
AFTER (:eloop row added)
12 tests, 0 failures
Mutations — the derivation itself is load-bearing
Three mutations, each reverted after:
1. New strerror clause, nothing else def strerror(:enospc), do: "No space left on device"
-> RED: unaccounted: MapSet.new([:enospc])
2. Backend constructs an unlisted kind Error.new(:eisdir,…) -> Error.new(:enospc,…) in stream_read/3
-> RED: unaccounted: MapSet.new([:enospc]) (arm 2 works without arm 1)
3. A second computed-kind call site Error.new(:enotdir,…) -> Error.new(hd([:enotdir]),…)
-> RED: assert Enum.count(constructed, &(&1 == :computed_at_runtime)) == 1
left: 2 right: 1
Mutation 2 matters most: it fails on a kind strerror has no clause for, so the two derivations are
independently effective rather than one shadowing the other.
Gates — all five, at 29ad8ff, clean tree
mix compile --warnings-as-errors --force Compiling 168 files (.ex) / Generated just_bash app
mix format --check-formatted exit 0
mix credo --strict 5160 mods/funs, found 1 refactoring opportunity
(test/support/banned_fixture_apply.ex — the intentional fixture)
mix test 2 doctests, 62 properties, 4820 tests, 0 failures (5 excluded)
Finished in 22.4 seconds
mix dialyzer Total errors: 13, Skipped: 13, Unnecessary Skips: 0
done (passed successfully)
4819 -> 4820 tests: the one new closure test. Credo's first run flagged
Nested modules could be aliased on the new JustBash.FS.Memory reference; fixed with an alias, and
the run above is back to the pre-existing baseline finding only.
Deliberately not fixed here
sort '' -> "sort: read failed: : Is a directory" where GNU says
"cannot read: '': No such file or directory". Root cause is FS.resolve_path(cwd, "") resolving an
empty operand to the cwd — pre-existing and shared (cat '' already misreports at origin/main), so
it belongs in the path layer and would collide with #74.
Final verificationIndependent re-run at Per-item
1.
|
| operand | gsort |
JustBash |
|---|---|---|
/nope |
cannot read: …: No such file or directory, rc=2 |
same |
/d (dir) |
read failed: …: Is a directory, rc=2 |
same |
/file.txt/sub |
cannot read: …: Not a directory, rc=2 |
same |
/loopa |
cannot read: …: Too many levels of symbolic links, rc=2 |
same |
The derivation is real, not decorative. I independently reproduced the two readings the new test
performs: FS.strerror/1 has 12 literal clauses (lib/just_bash/fs/fs.ex:269-280), and
lib/just_bash/fs/memory.ex has 22 Error.new/2 sites of which exactly one (:603, stream_read/3
forwarding a VFS.StreamOptions rejection) computes its kind. 4 tabled + 8 excused = 12 = the
universe. I also grepped every Error.new/2 outside the memory backend (fs.ex, posix.ex,
cp.ex, redirection.ex): the kinds are eisdir, enoent, eloop, einval, enotdir, exdev, enotsup —
all already inside the derived universe, so the derivation is not narrower than reality.
Mutation testing — five independent mutations, each red, each reverted:
| Mutation | Result |
|---|---|
Delete the :eloop row from @unreadable_operands |
RED: unaccounted: MapSet.new([:eloop]) |
Delete the @symlink_cycle seed from the behaviour test |
RED: observed …: No such file or directory vs expected …: Too many levels of symbolic links — the row is exercised, not decorative |
Add def strerror(:enospc), … to JustBash.FS |
RED: unaccounted: MapSet.new([:enospc]) |
Make the backend construct a kind strerror has no clause for (:eloop -> :emlink at memory.ex:474) |
RED on both tests: unaccounted: MapSet.new([:emlink]) |
Add a second computed-kind call site (Error.new(hd([:eisdir]), …)) |
RED: assert Enum.count(constructed, &(&1 == :computed_at_runtime)) == 1 — left 2, right 1 |
Worth stating the one boundary the derivation does have: it reads JustBash.FS.Memory and its
VFS.Mountable impl. A kind constructed only by some other backend and absent from strerror/1
would slip through. No such kind exists today (checked above), and strerror/1's is_atom catch-all
means such a kind would render as a bare atom rather than a wrong sentence.
2. sort '' — dispute upheld
PR head: $ sort '' -> rc=2, "sort: read failed: : Is a directory\n"
origin/main: $ cat '' -> misreports identically
FS.resolve_path(cwd, "") resolves the empty operand to the cwd, so the FS honestly answers
:eisdir. Path-layer bug, shared by every command, would collide with #74. Correctly left alone.
Gates (all re-run here, not quoted)
| Gate | Result |
|---|---|
mix compile --warnings-as-errors --force |
pass, 0 warnings |
mix format --check-formatted |
pass |
mix credo --strict |
1 finding: test/support/banned_fixture_apply.ex:4:16 — the intentional fixture, present at origin/main too |
mix test |
2 doctests, 62 properties, 4820 tests, 0 failures (5 excluded) — Finished in 26.2 seconds |
mix dialyzer |
pass — Total errors: 13, Skipped: 13, Unnecessary Skips: 0 |
Sweep — what was covered, including what came back clean
Test-file timing, the stated concern for a test-only change:
| tests | wall | |
|---|---|---|
parent f624597 |
185 | 0.3s |
head 29ad8ff |
186 | 0.3s |
One test added, no measurable runtime change, no shared fixture: the new symlink cycle lives in a
per-test in-memory FS, so async: true neighbours cannot see /loopa.
Operand-surface sweep: 25 invocations run at both head and origin/main and diffed — multi-operand
reads, operands starting with -, --, --help/--nosuch, and stdin-vs-file ordering across
sort, cat, head, tail, wc, uniq, tac, nl, tr, cut, paste. Every difference between the two is an
improvement this PR intended (sort /nope, sort -Q, sort --help, sort --nosuch,
echo x | sort -). Zero regressions.
Three things came back identical to origin/main and so are not findings against this PR, recorded
so the empty result is legible rather than assumed:
sort /a.txt /nope-> rc=0, sorts/a.txt, says nothing (GNU: rc=2 + diagnostic).sortreads only
its first operand —get_content(bash, [file | _], _stdin)is the same shape atorigin/main.
Same forecho x | sort - /nope.uniq /nope,cut -f1 /nope-> rc=0, silent. Identical atorigin/main; the issue-68 family of
fixes has not reached those commands yet.tr a b /nope-> rc=0, silent (GNU:extra operand). Identical atorigin/main.
Verdict: all clear. Item 1 is genuinely fixed and the coverage genuinely cannot pass without it;
item 2's deferral is correct.
JustBash.FlagParser demoted an unrecognised flag to a positional operand, so
`sort -Q /f.txt` and `uniq -Z /f.txt` read a file named `-Q`, found nothing,
and exited 0 with empty stdout and empty stderr — the file's contents dropped
and the caller told it succeeded.
parse/2 now returns {:ok, flags, rest} or {:error, {:unknown_flag, flag}} /
{:error, {:missing_value, flag}}, and all seven callers (cp, grep, head, ls,
sort, tail, uniq) render it with format_error/3 in GNU's shape: a short option
named by its character, a long option in full, followed by the Try --help line
and coreutils' exit code (2 for sort, ls and grep; 1 for the rest).
tr had its own copy: the combined-flag reducer discarded characters it did not
know, so `tr -dX b` deleted b and reported success, and a single unknown flag
like `-x` fell past the flag clauses and became a character set. It now rejects
both, and honours `--` so a dash-shaped character set still works.
parse_value/1 coerced every integer-looking value, which made `sort -t 1` a
delimiter of 1 and crashed String.split/2. Only flags declared :integer are
converted now; head and tail declare :n and :c.
Per the issue, the ~45 hand-rolled parse_args commands are left alone. The new
registry-wide probe enumerates every name in Commands.Registry against
`--jb-not-a-flag` and `-Z` and pins each one as :strict, :quiet, :operand or
:absorbed, so the list of commands that still swallow a flag can only shrink.
`sort -nk2` is `-n -k 2`, and `sort -rt: -k2` is `-r -t : -k 2`. The parser only peeled an attached value off character 0 of a cluster, so both fell past every clause into the unknown-flag path, where the offender was picked as "the first character that is not a boolean". That named `k` and `t` - flags sort implements - and rejected a command line GNU sorts without complaint. `parse_cluster/5` now walks the cluster the way getopt does: booleans accumulate, a value flag takes the rest of the cluster as its argument or the next argument when it is the last character, and the scan stops on the first character the spec does not describe at all. The diagnostic can no longer name a flag the command has. Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
`head -n abc`, `head -nq` and `head -n ''` raised a FunctionClauseError out of
`JustBash.exec/2` - an unhandled exception escaping a library whose job is
running untrusted shell safely. `head -c xy` was the quiet twin: exit 0 with
the whole file printed. `parse_value/3` conflated "this flag is not an integer
flag" with "this integer flag got garbage" and handed the raw string back to a
caller that had just declared the value a count.
`:integer` now validates. A bad count is the third getopt error class,
`{:invalid_value, label, value}`, worded as GNU words it:
head: invalid number of lines: 'abc'
with no `Try --help` line, because GNU prints none for this one. The noun comes
from a new `:value_labels` spec key, required of every `:integer` flag, so the
message says what was being counted.
Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
The `--` clause pushed everything after the marker into `sets` without checking
how many sets tr needs, so `echo abc | tr -- -d` and `echo abc | tr -- x` left
one set with no -d/-s, fell past every `run/2` head into
`defp run(_input, _opts), do: ""`, and answered with empty stdout at exit 0.
That is the silent-empty result this branch exists to remove, reintroduced by
the clause that was meant to make `--` work.
One set with neither -d nor -s is now GNU's error, which also closes the
pre-existing route to the same catch-all (`echo abc | tr x`):
tr: missing operand after 'x'
Two strings must be given when translating.
Try 'tr --help' for more information.
`tr` with no sets at all now prints the `Try --help` line too, as GNU does.
Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
`get_content/3` mapped every `FS.read_file` error to `{"", fs}`, so
`sort -- -Q` answered with empty stdout, empty stderr and exit 0 - issue #68's
exact symptom, still reachable one `--` away from the command the issue quotes.
The test added for that case only did `refute stderr =~ "invalid option"`
against a run that produced no stderr at all, so it asserted nothing.
sort now reports the unreadable operand the way GNU does, at GNU's exit code:
sort: cannot read: -Q: No such file or directory (exit 2)
A lone `-` is standard input rather than a file named `-`, which is both what
GNU does and what keeps `printf 'b\na\n' | sort -` meaningful; that test now
asserts the sorted output instead of only the exit code.
Also drops `parse_key_spec/1`'s `is_integer` clause. It was reachable only
while `parse_value/1` coerced every integer-looking value; with coercion gated
on `:integer` and sort declaring none, `:k` values are always binaries. It also
returned `{spec, nil}`, which the caller indexes as `modifiers[:numeric]`, so
it would have raised if anything had reached it.
Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
Every rejection this branch added ends with
`Try '<cmd> --help' for more information.`, and following that advice produced
the same class of error again:
$ sort --help
sort: unrecognized option '--help'
Try 'sort --help' for more information.
Issue #68's motivation is the agent feedback loop - "the model can only learn
what a command supports by running it" - so closing one dead end and printing
directions to a second one is not much of a fix.
`FlagParser.parse/2` now returns `:help` for `--help`, and `help/2` renders the
usage out of the spec, which means it lists exactly what the parser accepts and
cannot drift from it:
$ sort --help
Usage: sort [OPTION]... [FILE]...
Options this shell implements:
-f
-k VALUE
-n
-r
-t VALUE
-u
All eight commands answer, `tr` included. `--` still wins, so `sort -- --help`
is an operand. `--version` remains an unrecognized option, and the advice it
prints is now honest.
Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
…a failure `probe/2` classified on exit code and stderr emptiness alone, so a command that demoted the unknown flag to a filename and then failed to open it was indistinguishable from one that rejected the flag - even though the table's own comment calls :strict "what every option-parsing command should do". Running that classifier against 82ee9e5, before any of this branch: ls -Z => :strict (ls: cannot access '-Z': No such file or directory) head -Z => :strict (head: cannot open '-Z' for reading: ...) tail -Z => :strict (tail: cannot open '-Z' for reading: ...) cp -Z => :strict (cp: missing destination file operand after '-Z') Four of the seven FlagParser callers were already certified on the unfixed tree, so the matrix passed for exactly the `head -q` / `ls -Q` half of issue #68 - the half described as "the message names the wrong problem". :strict now requires empty stdout and a diagnostic that names an option (invalid / unrecognized / illegal option), and a fourth bucket, :misblamed, holds the ~20 commands that are non-zero only because the demoted flag named a file that does not exist. `cat -Z /f.txt` is the shape: exit 1, `cat: -Z: No such file or directory`, and the file's contents still on stdout. Against 82ee9e5 the strengthened classifier puts all eight shared-parser commands outside :strict - cp/head/ls/tail :misblamed, sort/uniq :exit_zero, grep :quiet, tr both - so the matrix now fails on the baseline for every caller the branch claims to fix. A new test pins those eight to :strict so the table cannot be edited to demote one. Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
`get_content/3` mapped every `FS.read_file` error onto the ENOENT wording, so
`sort /d` on an existing directory answered
sort: cannot read: /d: No such file or directory
which states something false about a path that exists. Exit 2 was already
right; only the diagnostic lied.
GNU sort uses two templates and the choice is informative: a directory opens
fine and fails when sort reads it, so coreutils says `read failed:`, while an
open failure says `cannot read:`. The reason now comes from `FS.strerror/1` in
both, so a kind we have not enumerated still reports itself accurately.
sort /d sort: read failed: /d: Is a directory
sort /nope sort: cannot read: /nope: No such file or directory
sort /f.txt/sub sort: cannot read: /f.txt/sub: Not a directory
All three match GNU coreutils 9 `gsort` byte for byte at exit 2, and the three
error kinds `FS.read_file` can return are asserted as one table rather than as
separate cases, so a fourth kind cannot arrive wearing the wrong template.
Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
…ng by hand
The comment above @unreadable_operands claimed the table held "the three
error kinds FS.read_file can return". There is a fourth. `:eloop` is
constructed at lib/just_bash/fs/memory.ex:474 and reaches sort today:
ln -s /loopb /loopa; ln -s /loopa /loopb; sort /loopa
-> rc=2, "sort: cannot read: /loopa: Too many levels of symbolic links"
which is byte-for-byte what GNU coreutils 9 gsort prints. The behaviour
was already right - the generic read_error/2 clause and FS.strerror/1
handle it. What was wrong was the table: it asserted exhaustiveness while
missing a reachable kind, so the absent case was indistinguishable from a
passing one (issue #70).
Adding the row alone would leave the next kind to the same hand-count, so
the kind list is now derived rather than transcribed. A new test reads two
things out of the compiled beams - the literal atoms in FS.strerror/1's
clause heads, and every literal atom the memory backend passes to
VFS.Error.new/2 - and fails unless each kind is either a row in the table
or listed in @kinds_read_file_cannot_return with what does produce it.
Kinds sort cannot be handed are named with the reason instead of omitted.
The backend has exactly one Error.new/2 whose kind is computed rather than
literal (stream_read/3 forwarding a VFS.StreamOptions rejection, which
read_file/2 cannot trigger because it passes no options); the test pins
that count at one so a second such site fails loudly rather than silently
shrinking what the derivation can see.
Test-only change; no production code touched.
Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
29ad8ff to
9b8aee0
Compare
Closes #68
What was broken
JustBash.FlagParserturned a flag it did not recognise into a positionaloperand (
lib/just_bash/flag_parser.ex:75), so the flag became a filename.For
sortanduniq— which silently treat an unreadable file as empty —that produced the worst available outcome: the file's contents dropped, and
exit 0 to say it worked.
sort -Q /f.txt"", stderr""sort: invalid option -- 'Q'+Try 'sort --help'…uniq -Z /f.txt"", stderr""uniq: invalid option -- 'Z'+Try 'uniq --help'…head -q /f.txthead: cannot open '-q' for reading…head: invalid option -- 'q'+Try 'head --help'…-qis implemented there)ls -Q /ls: invalid option -- 'Q'+Try 'ls --help'…-Qis implemented there)head -qandls -Qare real coreutils flags that just are not implementedhere; rejecting them names the actual problem instead of blaming a file that
was never on the command line.
Three more, found on the way:
What the fix does
1.
FlagParserrejects instead of demoting.parse/2now returns{:ok, flags, rest}or{:error, {:unknown_flag, flag}}/{:error, {:missing_value, flag}}.format_error/3renders it in GNU'sshape, checked against GNU coreutils 9 on this machine:
invalid option -- 'Q'), because thatis the unit getopt stopped on — out of a cluster,
-rQreportsQ;unrecognized option '--jb-not-a-flag'), becauseinvalid option -- '-'identifies nothing;option requires an argument -- 'n'for a value flag with nothing after it,which used to become a file named
-n;Try '<cmd> --help' for more information.line, and coreutils' exitcodes: 2 for
sort,lsandgrep, 1 forcp,head,tail,uniq.Only
grepprints aUsage:line on a bad option; coreutils does not, soneither do we.
All seven callers (
cp,grep,head,ls,sort,tail,uniq) surfaceit.
--still ends option parsing and a lone-is still an operand.A cluster is parsed the way getopt parses one: booleans accumulate and a value
flag takes the rest of the cluster as its argument, or the next argument when
it is the last character, so
sort -nk2is-n -k 2andsort -rt: -k2sorts. The scan stops on the first character the spec does not describe at
all, so the diagnostic never names a flag the command implements.
A flag declared
:integeris validated.head -n abc,head -nqandhead -n ''used to raise aFunctionClauseErrorout ofJustBash.exec/2andhead -c xyprinted the whole file at exit 0; all four are nowhead: invalid number of lines: 'abc'at exit 1, GNU's wording, with noTry --helpline because GNU prints none for this one. The noun comes from a:value_labelsspec key, required of every:integerflag.--helpanswers. Every rejection printsTry '<cmd> --help' for more information., so that command has to work.FlagParser.help/2renders the usage out of the spec — it lists exactly whatthe parser accepts and cannot drift from it:
All eight commands answer at exit 0.
--still wins, sosort -- --helpis anoperand.
--versionstays an unrecognized option, and the advice it prints nowleads somewhere.
2.
tr's own copy. The combined-flag reducer's catch-all discarded unknowncharacters while consuming the token, and a single-character unknown flag fell
past the
byte_size(flags) > 1guard into the operand clause and became acharacter set. Both now report
tr: invalid option -- 'x'at exit 1.tralsolearned
--, soecho a-b | tr -- '-' '_'givesa_bas in GNU rather thanempty output.
tralso refuses to translate with one set. One set and no-d/-sused tofall past every
run/2head intodefp run(_input, _opts), do: "", soecho abc | tr -- -dandecho abc | tr xanswered with empty stdout at exit0 — the failure this PR exists to remove. Both are now GNU's
tr: missing operand after 'x'+Two strings must be given when translating.at exit 1.
3.
parse_value/1no longer coerces. Only flags a spec declares:integerare converted;headandtaildeclare:nand:c. Everythingelse stays a string, so
sort -t 1 -k2is a delimiter of"1"and sortsinstead of crashing. (The issue cites
cut -d 1for this —cuthas its ownhand-rolled parser and was never affected;
sort -tis the real victim.)4.
sortreports a file it cannot read.get_content/3mapped everyFS.read_fileerror to{"", fs}, sosort -- -Qwas still the exact symptomof the issue: empty stdout, empty stderr, exit 0. It is now
sort: cannot read: -Q: No such file or directoryat exit 2, as in GNU, and alone
-reads standard input rather than naming a file that does not exist.Per the issue's warning, the ~45 hand-rolled
parse_argscommands are notmigrated onto
FlagParserhere.New tests
test/commands/unknown_flags_test.exs(22 tests) — the repro from the issue asexact-string assertions for all eight commands, cluster and long-option
wording, missing values,
--and lone-, and the two value-coercion cases.test/flag_parser_test.exs— updated to the new return shape, plus 12 newtests covering every error path and the
:integeropt-in.The registry-wide probe.
test/commands/unknown_flags_test.exsenumeratesall 92 names in
Commands.Registryagainstcmd --jb-not-a-flagandcmd -Zand compares the observed behaviour of the whole registry to a classification
table, so a new command cannot be added without being classified and none can
change category unnoticed. A second test asserts every rejecting command names
itself in the diagnostic, and a third pins the eight shared-parser commands to
:strictso the table cannot be edited to demote one.A non-zero exit is not evidence that a flag was rejected: demoting it to a
filename and failing to open that file also exits non-zero, and leaves the
command's real output on stdout.
:stricttherefore requires empty stdout anda diagnostic that names the option. Three of the five categories are honest
about the state of the tree:
:strict(36) — the flag was rejected: non-zero exit, nothing on stdout, andinvalid option/unrecognized optionin the diagnostic. Where everycommand should be, and where the eight fixed here now are.
:misblamed(21) — still wrong: non-zero, but the diagnostic is about afile the flag was turned into.
cat -Z /f.txtsayscat: -Z: No such file or directoryand prints the file anyway.:quiet(exit,false,read) — non-zero, no diagnostic; bash is equallysilent, they parse no options.
:operand(:,echo,test,true,yes) — exit 0 is correct, bash alsotreats the argument as data.
:absorbed(27) — still wrong: exits 0 with the flag ignored, exactly theway
sort -Qdid. These are the hand-rolled parsers the issue defers. Theyare listed by name rather than waved at, so fixing one forces an edit to the
table and the list can only shrink.
Run against
82ee9e5the classifier puts all eight shared-parser commandsoutside
:strict—cp/head/ls/tail:misblamed,sort/uniq:exit_zero,grep:quiet,trboth — so the matrix fails on the baselinefor every caller this PR claims to fix.
No exception list was needed for "
-Zlegitimately implemented" — no commandin the registry implements
-Z.Gates
(the single finding is the pre-existing intentional test fixture — identical on
main)4752 tests before, 4818 after, no regressions.
Left out
Nothing from the issue's scope. Three adjacent gaps in
trare noted and nottouched, because none is reachable through anything this PR adds:
tr -cs areturns
""at exit 0 where GNU printsabc, andtr a b c/tr -d a breach
run/2's catch-all where GNU saysextra operand.