fix: filesystem commands name the actual error kind, and the Oils spec parser reads the format - #74
fix: filesystem commands name the actual error kind, and the Oils spec parser reads the format#74davydog187 wants to merge 11 commits into
Conversation
davydog187
left a comment
There was a problem hiding this comment.
Two lenses were applied to this PR: a conformance lens (does each changed site now match real GNU/bash on this machine — ghead, gtail, gsort, grealpath, gmd5sum, file, bash -c) and a test-strength lens (can the new tests actually detect a regression at each site the diff touched). 13 candidate findings went in; 7 survived verification, 6 were merged or refuted. Everything below was reproduced on bb55214 in a scratch worktree.
The two commits are good and the sweep is the right shape. Nothing here is a blocker — the two major items are a coverage hole and a moduledoc claim that is wrong about the corpus format, and the rest are per-site conformance nits that the new matrix currently pins rather than catches.
1. The parser discards the corpus's bash-keyed expectations, so 323 cases carry another shell's ground truth
major — lib/just_bash/spec_test/parser.ex:145 (and the nil branch at :149)
The moduledoc added by this PR says ## N-I <shells>, ## BUG <shells>, ## OK <shells> are "the expectation for a shell that is not bash. Bash is the oracle here, so these are dropped." In the Oils format that is exactly backwards for the annotations that name bash: ## OK bash status: 0 is bash's recorded status, overriding the unannotated default (which records osh/dash). block_opener/1 classifies ## OK bash STDOUT: as :other_shell and collect_block/2 throws away the body; the single-line forms (## OK bash status: 0) fall through the generic ## clause at :149 and are dropped too.
Measured on the corpus: 440 annotation lines name bash or just-bash, across 89 files, covering 323 of the 2728 cases (~12%). Two of them (loop.test.sh:288-291, var-op-bash.test.sh:489) are ## OK just-bash markers written by this repo and are now silently discarded.
Failure scenario, reproduced:
$ mix run -e 'JustBash.SpecTest.Parser.parse(File.read!("test/command_spec_cases/bash/cases/append.test.sh")) |> ...'
%TestCase{name: "Try to append list to element", expected_stdout: "", expected_status: 2}
The file (append.test.sh:134-148) records ## stdout-json: "" / ## status: 2 for osh, then ## OK bash status: 0 and ## OK bash STDOUT: → ['1', '2 3']. A JustBash that reproduces bash exactly is scored as a failure by Runner.run_test_case/2. Same for loop.test.sh:273 "too many args to continue", which parses to expected_stdout: "" / status: 2 while the file records ## BUG bash STDOUT: a\n-- with ## BUG bash status: 0.
This PR is the stated prerequisite for item 7's ratchet. Building the baseline on ~12% of cases whose expectation is knowably not bash's is the opposite of a ratchet.
Fix: treat an annotation whose shell list contains bash (or just-bash) as an override of the case's default expectation rather than as foreign — parse the shell list out of ## (OK|BUG|N-I|BUG-\d+) <shells> <key>: and apply it when bash ∈ shells. Correct the moduledoc paragraph either way; as written it asserts something false about the format.
2. The strerror matrix pins operand arity to 1 — five changed sites are untestable-by-construction
major — test/commands/error_message_test.exs:97
Every row of @content_readers/@metadata_readers passes exactly one path operand (comm/diff pass two copies of the same one). But head, tail and wc each have two independent error sites — a single-file arm and a reduce over multiple files — and the diff changed both. sha256sum.ex and shasum.ex each have three (hash_files, check_checksums, verify_single_checksum); only hash_files is reachable from the matrix.
Proven by mutation. I replaced #{FS.strerror(error)} with the literal MUTANT at five sites — head.ex:48, tail.ex:48, wc.ex:66, sha256sum.ex:114, shasum.ex:121 — and ran the full suite:
Finished in 24.7 seconds
2 doctests, 62 properties, 4894 tests, 0 failures (5 excluded)
Five of the sites this PR exists to fix can be fully undone without turning the suite red. This is the exact anti-pattern #70 is about: the matrix looks like a cross product, but one axis (operand arity) is pinned to a single value and everything behind the other value is invisible.
Fix: add a second operand to the head/tail/wc rows ("head PATH /f" etc., asserting the stderr line while stdout carries /f), plus one sha256sum -c PATH row and one shasum -c PATH row.
3. md5sum writes its diagnostic to stdout, corrupting the checksum stream
major — lib/just_bash/commands/md5sum.ex:64
The diff rewrote this exact line to render FS.strerror/1 but left it appending to acc_out; compute_hashes/3 then returns stderr: "" unconditionally. sha256sum.ex and shasum.ex — the same shape, touched by the same sweep — already do {out, err <> …}.
$ md5sum /f /nope
rc=1
out="764efa883dda1e11db47671c4a3bbd9e /f\nmd5sum: /nope: No such file or directory\n"
err=""
$ gmd5sum nope # coreutils 9.x
rc=1 out=[] err=[gmd5sum: nope: No such file or directory]
So md5sum /f /nope > sums.txt writes the diagnostic into sums.txt as a malformed checksum line, and md5sum /nope 2>/dev/null still prints it. test/commands/error_message_test.exs:46 records the divergence ({"md5sum PATH", :stdout, …}) with a comment calling it "a separate bug from the message text" — but it is the same line the sweep touched, and recording it pins it for whoever runs the enumerated matrix later. (file.ex:51 has the same shape and is genuinely correct: real file does print to stdout — verified.)
Fix: {acc_out, acc_err <> "md5sum: …"} and thread acc_err into the returned stderr, then flip the matrix row to :stderr.
4. file -b still hardcodes "cannot open" and drops the reason the diff just threaded in
minor — lib/just_bash/commands/file.ex:70
format_error_line/2 became /3 and renders FS.strerror(error) in the non-brief branch, but the brief branch two lines above still returns the constant "cannot open\n". -b suppresses the filename prefix, not the errno text.
$ file -b /f/x -> "cannot open\n" (indistinguishable from ENOENT)
$ file -b /nope -> "cannot open\n"
$ file /f/x -> "/f/x: cannot open (Not a directory)\n" (correct)
$ file -b f/x # real file(1)
cannot open `f/x' (Not a directory)
The matrix only exercises file PATH, never file -b PATH, so the half-fixed function is uncovered.
Fix: "cannot open (#{FS.strerror(error)})\n" in the brief branch, and add a file -b PATH row.
5. head/tail use the open-failure template for EISDIR; GNU uses a different one
minor — lib/just_bash/commands/head.ex:48, :68, lib/just_bash/commands/tail.ex:48, :68
GNU head/tail open(2) a directory successfully and only fail at read(2), so EISDIR gets a different template from ENOENT/ENOTDIR/EACCES. Verified against coreutils 9.x on this machine:
ghead nope -> ghead: cannot open 'nope' for reading: No such file or directory
ghead f/x -> ghead: cannot open 'f/x' for reading: Not a directory
ghead d -> ghead: error reading 'd': Is a directory <-- different template
gtail d -> gtail: error reading 'd': Is a directory
just_bash emits head: cannot open '/d' for reading: Is a directory. The PR's premise is that the kind must be named exactly; here the kind also selects the template, and the sweep's "templates untouched" rule mis-renders it. test/commands/error_message_test.exs:24-25 now asserts the wrong string for the :eisdir row, so this becomes a test to change rather than a test to add.
Same shape, lower priority: tac /nope is GNU tac: failed to open '/nope' for reading: … (just_bash: tac: /nope: …) and tac /d is GNU tac: /d: read error: Is a directory.
Fix: branch on :eisdir in the head/tail templates, or leave the code and add a known_gap-style comment on the matrix row so it doesn't read as ground truth.
6. realpath errors on a missing final component where GNU exits 0 — and the matrix pins it
minor — lib/just_bash/commands/realpath.ex:34, test/commands/error_message_test.exs:55
GNU realpath without -e requires only the parent components to exist; a missing last component is canonicalised and printed with exit 0. just_bash FS.stats the full path and errors.
$ realpath /nope # just_bash
rc=1 err="realpath: /nope: No such file or directory\n"
$ grealpath nope # coreutils 9.x
rc=0 out=/private/tmp/gnuprobe/nope
$ grealpath nope/deep/x # intermediate missing — GNU does error here
rc=1 err=grealpath: nope/deep/x: No such file or directory
The sweep changed this line and the matrix now asserts realpath: /nope: No such file or directory with a non-zero exit for the :enoent row — recording the wrong ground truth for the one kind where realpath is not supposed to fail at all. The :enotdir and :eacces rows are correct as written.
Fix: either stat the parent instead of the full path (and gate the full-path stat behind -e), or drop the :enoent row for realpath with a comment saying why.
7. Bare ## stdout: is not recognised, so 4 corpus cases assert nothing
minor — lib/just_bash/spec_test/parser.ex:116
The clause is collect([{"## stdout: " <> value, _} | …]) — it requires the space after the colon. The Oils format spells an expected single empty line as a bare ## stdout: (the value is empty, so no separating space is written; upstream's KEY_VALUE_RE uses :\s*(.*)). Those lines fall through to the generic ## clause, block_opener/1 returns nil, and the directive is dropped. expected_stdout stays nil and Runner.check_output(nil, _) returns true, so the case passes whatever it printed. The same hole applies to a bare ## stderr:.
Four cases in the corpus are spelled this way; all four parse to expected_stdout: nil:
sh-func.test.sh:10 "Locals don't leak"
var-op-strip.test.sh:34 "Remove const suffix from undefined"
redirect-command.test.sh:141 "Redirect in command sub"
assign.test.sh:134 "Empty env binding"
sh-func.test.sh "Locals don't leak" (f() { local f_var=f_var; }; f; echo $f_var) would still be scored passed: true if the local did leak and stdout were "f_var\n".
The new test/just_bash/spec_test_parser_test.exs pins five corpus-wide counts (2728 / 136 / 247 / 65 / 11), none of which constrains expected_stdout or expected_status — deleting the "## stdout: " <> value clause outright leaves all five holding and the file green. Measured on this branch: 173 cases end with expected_stdout == nil and 67 assert literally nothing (nil stdout, nil stderr, status 0, not skipped).
Fix: match "## stdout:" <> value / "## stderr:" <> value and strip one leading space. Then add a sixth corpus assertion on the assert-nothing count — it would have caught this, and it will fail loudly on any future directive spelling the parser stops recognising (including finding #1).
8. The sweep was grep-driven, so it missed the commands that print no message at all
major, follow-up scope — lib/just_bash/commands/sort.ex:49, cut.ex:90, uniq.ex:32, grep.ex:157; also lib/just_bash/interpreter/executor/redirection.ex:369
Not a defect introduced here — none of these lines are in the diff — but it is a methodology gap worth naming, because it decides whether #70 item 1's family is closed. The sweep was driven by grepping for the literal "No such file or directory", so it reached every module that printed a wrong message and none of the modules that print nothing. Those are the worse members of the family, and they are not in the new 126-cell matrix. All five are named in #70 item 3's own operand list.
Reproduced on this branch with /f a regular file and /d a directory:
sort /nope -> rc=0 out="" err="" | gsort: cannot read: nope: No such file or directory, rc=2
sort /d -> rc=0 out="" err="" | gsort: read failed: d: Is a directory, rc=2
cut -f1 /nope -> rc=0 out="" err="" | gcut: nope: No such file or directory, rc=1
uniq /nope -> rc=0 out="" err="" | guniq: nope: No such file or directory, rc=1
grep x /nope -> rc=1 out="" err="" | grep: nope: No such file or directory, rc=2
cat < /nope -> rc=0 out="" err="" | bash: nope: No such file or directory, rc=1
wc -l < /nope -> rc=0 out="0\n" | bash: nope: No such file or directory, rc=1, no stdout
wc -l < /nope printing a fabricated 0 is the sharpest one. 025673c ("a command whose redirect fails does not run at all") only covered the write side — the docstring at redirection.ex:62 explicitly excludes < from preflight/2 as a redirect that "touches no file", and extract_stdin_content/2 swallows the read error to "".
An agent writing sort report.txt | head -5 against a mistyped path gets a clean exit-0 empty result with nothing to distinguish it from an empty file.
Fix (follow-up PR is fine): thread the error out of get_content/3 in those four commands, and add them as four rows to @content_readers — that is the whole test-side surface. < wants its own treatment: report from the shell with the shell's prefix and skip the command, matching the write-side behaviour 025673c established.
Considered and dismissed (6)
"Unreachable collect_block/2 clause for a case header" (parser.ex:176) — refuted. The claim was that collect_block([{"## " <> _, _} | _], acc) at :174 shadows the "#### " <> _ clause at :176 because "#### foo" starts with "## ". It does not: the prefix is #,#,space, and "#### foo" has # in the third position. Verified — match?("## " <> _, "#### foo") is false. The secondary argument (that extract_test_cases/2 already splits bodies at case_header?/1, so a body can never contain a #### line) is true, which makes the clause defensive rather than dead-and-shadowed. No traced cost; leaving it is fine.
"Corpus-count assertions cannot detect a regression that turns expectations into nil" (spec_test_parser_test.exs:1402) — merged into finding #7. The measurement is correct (173 nil-stdout, 67 assert-nothing, all five pinned counts survive deleting the ## stdout: clause), but on its own it is a suggestion rather than a defect. It is the right fix for #7, so it lives there.
"Parser discards bash-specific expectations" reported twice — the two lenses found the same thing; merged into finding #1, keeping the just-bash-marker detail from one and the append.test.sh repro from the other.
"Bare ## stdout:" reported twice — merged into finding #7.
file.ex:51 printing the diagnostic to stdout — not a bug. Real file(1) writes nope: cannot open ... to stdout and even exits 0; verified locally. Only md5sum (finding #3) is on the wrong stream.
"wc /d /f prints No such file or directory" — the failure scenario attached to finding #2 was wrong; that describes the reverted state. On this branch wc /d /f correctly prints wc: /d: Is a directory. The finding survives on its coverage argument, which the mutation test proves independently.
|
All eight findings are addressed. Nothing disputed — every repro reproduced first, went red as a test, and now matches the oracle.
1 + 8 — the spec parser (
|
Verification of review fixesIndependent re-run of all 8 findings against Per-finding
Gates (re-run here, not taken on report)New problems introduced by these commitsBoth come from 1.
|
Both regressions from the fix round are closed —
|
| before | now | |
|---|---|---|
cut -c1 - |
cut: invalid option '-' |
reads stdin |
head - / tail - |
cannot open '-' for reading |
reads stdin |
wc -l - |
wc: -: No such file... |
1 - |
nl - fold - expand - base64 - md5sum - comm - diff - |
invalid option |
reads stdin |
od -c - xxd - |
unknown option: - |
reads stdin |
sed -n p - awk '{print}' - jq . - |
-: No such file... |
reads stdin |
cat - tac - rev - paste - sha256sum - shasum - file - |
already correct | unchanged |
Ten of them never reached a read path at all: their arg parsers called a bare - an invalid option, which POSIX says it never is. Those get one clause each; the rest go through a new JustBash.Commands.StdinOperand.read/4 that mirrors FS.read_file/2, so each call site keeps the error arm it already had.
Per-operand labels follow GNU, which does not agree with itself:
echo x | grep x - /f -> (standard input):x
/f:x
echo x | head - /f -> ==> standard input <==
==> /f <==
echo x | wc -l - /f -> 1 -
1 /f
One real bug fell out of the audit: base64 - /f silently dropped stdin (read_single_file/4 matched - and returned the accumulator untouched). It now concatenates — eAo= became eAp4Cg==.
Not touched
- fix: a flag a command does not implement is an error, not a filename #72's
sort /dwording. Left alone as instructed. My change tosort.exis confined toget_content/3's read call;read_error/2, which is what fix: a flag a command does not implement is an error, not a filename #72 edits, is untouched, so the two should not collide. tee -. GNU treats it as a literal filename (it creates a file called-), so it is classified as not-a-stdin-operand.echo x > -; cat ./-still works.head's multi-file separator.head /f /gemits one blank line too many between sections. Pre-existing, reproduces with two real files, unrelated to-.
Tests
test/commands/stdin_operand_test.exs, 37 tests. The registry test asserts every command in Registry.list() is either a --reads-stdin row or explicitly listed as not one, so a command added later has to be classified rather than silently skipped. Each row is checked against the same command reading a file holding the same bytes rather than against a literal transcript — the invariant is that the operand names a source, not that nl pads to six columns.
Mutation-checked: deleting the - arm of StdinOperand.read/4 turns 18 red; deleting the /dev/null arm of read_stdin_target/2 turns 4 red.
Gates
$ mix compile --warnings-as-errors --force
Compiling 169 files (.ex)
Generated just_bash app
$ mix format --check-formatted
format exit=0
$ mix credo --strict
Refactoring opportunities
┃ [F] ↘ Avoid `apply/2` and `apply/3` when the number of arguments is known.
┃ test/support/banned_fixture_apply.ex:4:16 #(BannedCallTracer.Fixture.Apply.run)
5204 mods/funs, found 1 refactoring opportunity. # pre-existing, intentional fixture
$ mix test
2 doctests, 62 properties, 5025 tests, 0 failures (5 excluded)
$ mix dialyzer
Total errors: 13, Skipped: 13, Unnecessary Skips: 0
done (passed successfully)
Final verificationIndependent re-run at Per-item verdicts
1. Regression 1 —
|
Round 4 —
|
| mutation | red |
|---|---|
StdinOperand.read/4 loses its - arm |
40 (was 18 before this matrix) |
operands/1 loses its - clause (the old reject) |
3 |
operands/1 loses its -- clause |
1 |
Plus a transcript test for tac's per-operand ordering, which the equality
matrix cannot see.
Divergences found while checking, left for separate changes
Measured, all pre-existing and none about - — the two-operand matrix stays
green on them because it compares - against a file rather than against GNU:
sort - /freads only the first operand; so doessort /a /b. GNU merges.base64 - /fconcatenates;gbase64 - fisextra operand 'f', rc=1.revdoes not newline-terminate an unterminated final line, so operands
still bleed across a file boundary inrev a bwhenahas no trailing
newline. GNU printsba\ndc\nthere; we printdcba\n.tac -b/-r/-sandrev's unknown-option diagnostic, above.
Gates
mix compile --warnings-as-errors clean
mix format --check-formatted clean
mix credo --strict 1 finding: test/support/banned_fixture_apply.ex
(the intentional fixture)
mix test 2 doctests, 62 properties, 5053 tests,
0 failures (5 excluded)
mix dialyzer Total errors: 13, Skipped: 13,
Unnecessary Skips: 0 -- passed successfully
Diff is 5 lib files and 1 test file, +169 / -79.
Final verificationIndependent re-run at Per-item
Note on #6's numbers: the note says "64 tests, 5 failures" against d59d84d. The file as merged is 65 Coverage can failLib reverted to d59d84d, new test file kept — 65 tests, 6 failures, exactly: The other 22 rows stay green there, confirming the earlier classification for the rest of the set. Mutations on the final head (full suite in parentheses):
Matches the claimed 40 / 3 / 1. Restored and re-verified clean after each. SweepIndependent audit of the whole "already-correct" set. All 26 rows run as
Clean, i.e. byte-identical to the oracle in both positions: Pre-existing, not regressions — verified by diffing the four command files Operands beginning with a dash / Test timing: Still open in the same commands (pre-existing, flagged not fixed)
Gates (all re-run here)Verdict: all six claimed fixes verified against the oracle, both |
… saying "No such file or directory"
33 command modules matched `{:error, _}` and threw the error kind away,
then spelled every failure "No such file or directory". `stat /f/x` where
`/f` is a regular file claimed the path did not exist, when it exists and
its parent component is not a directory; the same wrong message came out
of a directory read (`:eisdir`) and an unreadable mount (`:eacces`).
Every one of them now renders `FS.strerror/1` on the error it was already
given, so the message matches the kind. The message templates are
unchanged.
`JustBash.exec_file/2` had the same defect and is fixed with them.
The new test is the cross product of 34 filesystem-touching commands and
the kinds a path can fail with, enumerated off a table rather than
written case by case — 126 cases, 82 of which failed before this change.
`:eacces` is unreachable from the in-memory backend, which has no
permission model, so it comes from a new test-support mount that refuses
every operation with one configured kind.
Refs #70
…reads ## STDERR
`lib/just_bash/spec_test/parser.ex` has no callers, so its four defects had
nothing to catch them:
* `:skip` was never assigned, making `Runner`'s skip guard dead code.
Compounding it, Oils writes `## SKIP` *above* the script and the parser
stopped collecting the script at the first `##` line — so all 247
SKIP-marked cases ran, with an empty script, and passed vacuously.
* `String.trim_leading(line, "#### ")` strips the prefix *repeatedly*, so a
case name that itself starts with the header prefix loses part of itself.
* `String.trim(script)` trims whitespace *characters*, rewriting the last
command of every case whose script ends in a space.
* `## STDERR` was never read, so the 65 cases carrying a stderr expectation
asserted nothing about stderr — the class the silently-ignored-flag bugs
live in.
A case body is now sorted line by line into script or directive, in either
order, so a directive above the script no longer eats it, and a multiline
block keyed to another shell (`## N-I dash STDOUT:`) is consumed to `## END`
instead of falling through into the script. The script is preserved between
its first and last non-blank line: only whole blank lines at the ends are
dropped, because `$LINENO` and bash's `line N:` diagnostics count from the
first surviving line and the corpus asserts on both.
`Runner` reports a skipped case as skipped rather than as an error, and
compares stderr when the case names one.
The suite is still not wired into CI — activating it is a separate roadmap
item. The tests here exercise the parser directly against the real corpus:
it reads 2728 cases from 136 files, marks exactly the 247 SKIP markers the
files contain, and finds exactly 65 stderr expectations.
Refs #70
An Oils annotation naming a shell records what *that shell* actually does.
The parser treated every one of them as foreign and dropped it, so the 323
cases (440 annotation lines, 89 files) that spell bash's behaviour as
'## OK bash STDOUT:' / '## OK bash status: 0' were left pinned to the
unannotated default, which records osh. append.test.sh 'Try to append list
to element' asked for stdout "" and status 2 while the file plainly records
status 0 and ['1', '2 3'] for bash.
Annotations are now parsed into {qualifier, shells, key: value} and applied
per key, weakest first: unannotated default, then bash, then this repo's own
'## OK just-bash' markers, which were being discarded too. 'bash-2' is bash
2.x and stays foreign.
Separately, '## stdout:' with nothing after the colon is how the format
spells one expected empty line; the clause required the separating space, so
four cases parsed to expected_stdout: nil and Runner.check_output(nil, _)
passed them whatever they printed. Same hole for a bare '## stderr:'.
The corpus-wide counts in the test now include the ones that move when a
directive spelling stops being recognised: 2562 cases with a stdout
expectation, 239 with a non-zero status, and 59 - not 67 - that assert
nothing at all.
Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
…tream 'md5sum /f /nope' put 'md5sum: /nope: No such file or directory' into stdout between checksum lines, so 'md5sum a b > sums.txt' wrote a malformed checksum line into sums.txt and '2>/dev/null' did not silence it. gmd5sum writes to stderr with an empty stdout, and sha256sum/shasum already did here. 'md5sum -c FILE' on a checksum file it cannot read also said nothing and exited 0; it now reports the kind and exits 1. An unreadable checksum file is not a checksum that did not match, so it does not feed the WARNING line. The matrix row that recorded the divergence as :stdout now asserts :stderr. Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
GNU head, tail and tac open(2) a directory successfully and only fail at read(2), so EISDIR selects a different template from the kinds that fail to open at all. Verified against coreutils 9.x: ghead d -> ghead: error reading 'd': Is a directory gtac d -> gtac: d: read error: Is a directory gtac nope -> gtac: failed to open 'nope' for reading: No such file or directory The sweep's 'templates untouched' rule mis-rendered all three, and the matrix asserted the wrong string for the :eisdir row rather than catching it. tac was using the wrong template for every kind, not just EISDIR. The matrix rows now carry the kinds they cover, which is what lets one command have two templates; the content-reader/metadata-reader split it replaces was only ever a way to spell 'these kinds'. Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
format_error_line/3 rendered FS.strerror/1 in the non-brief branch but the brief branch still returned the constant 'cannot open', so 'file -b /f/x' and 'file -b /nope' were indistinguishable. -b suppresses the filename prefix, not the errno text. The matrix only ever exercised 'file PATH', which is why the half-fixed function read as covered; it now exercises 'file -b PATH' too. Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
GNU realpath without -e requires only the parent components to exist: 'grealpath nope' exits 0 and prints the canonicalised path, while 'grealpath nope/deep/x' exits 1. just_bash stat'd the whole path, so 'realpath /nope' failed - and the matrix recorded that as ground truth for the one kind where realpath is not supposed to fail. The three modes are now real: default requires the parent to be a directory (so '/f/x' is still ENOTDIR even though '/f' stats cleanly), -e requires the whole path, -m requires nothing. -e and -m were being parsed and discarded. Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
…sites Every row passed exactly one path operand, so the multi-file arms of head, tail and wc and both -c arms of sha256sum and shasum were untestable by construction. Replacing the rendered strerror with the literal MUTANT at wc.ex:66, sha256sum.ex:73, sha256sum.ex:114, shasum.ex:87, shasum.ex:121 and the head/tail reduce arms left the whole suite green at 4923 tests. Rows naming two operands, '-c FILE', and a checksum file listing an unreadable target now cover all seven: the same mutation turns 21 tests red. Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
…t do The sweep was driven by grepping for the literal 'No such file or directory', so it reached every module that printed the wrong message and none that printed nothing. These printed nothing: sort /nope rc=0 silent -> rc=2 sort: cannot read: /nope: ... sort /d rc=0 silent -> rc=2 sort: read failed: /d: Is a directory cut -f1 /nope rc=0 silent -> rc=1 cut: /nope: ... uniq /nope rc=0 silent -> rc=1 uniq: /nope: ... uniq /d rc=0 silent -> rc=1 uniq: error reading '/d': Is a directory grep x /nope rc=1 silent -> rc=2 grep: /nope: ... cat < /nope rc=0 silent -> rc=1 bash: /nope: ... wc -l < /nope rc=0 and '0' -> rc=1 bash: /nope: ..., no stdout An agent writing 'sort report.txt | head -5' against a mistyped path got a clean exit-0 empty result indistinguishable from an empty file; 'wc -l' put a fabricated 0 on stdout, which looks like an answer. grep keeps GNU's rule that -q with a line selected exits 0 even after an error, and cut keeps the files it could read. '<' is now the read side of 025673c: the shell opens the target, so the shell reports the failure and the command does not run. EISDIR is deliberately excluded - open(2) on a directory succeeds, so bash runs the command and the command's own read fails. Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
… paths e1028b6 taught the read paths to report the read they could not do. That is only right for operands that are really paths, and two are not — so the fix turned two silent successes into hard failures: cat < /dev/null rc=0 "" -> rc=1 bash: /dev/null: No such file... echo x | sort - rc=0 "" -> rc=2 sort: cannot read: -: No such file... echo x | uniq - rc=0 "" -> rc=1 uniq: -: ... echo x | grep x - rc=1 "" -> rc=2 grep: -: ... `cmd < /dev/null` is how a command is handed a closed stdin, and `sort -` is how a pipeline names its own input; both now abort, and under `set -e` or `pipefail` they take the pipeline with them. /dev/null was special-cased only on the write side of redirection. The two directions now share one @null_device and agree: `> /dev/null` discards, so `< /dev/null` reads empty. `-` is fixed by reading stdin, not by suppressing the diagnostic — rc=0 with no output is the silent-success class #70 exists to remove. The audit was driven off the registry rather than by hand, which reached nineteen more commands that had the same gap before the sweep and were merely quieter about it: cut, head, tail, wc, nl, fold, expand, base64, md5sum, od, xxd, comm, diff, sed, awk, jq and paste/tac/rev, which already worked. Ten of them never reached a read path at all — their arg parsers called a bare `-` an invalid option, which POSIX says it never is. The per-operand label follows GNU, which does not agree with itself: grep prints "(standard input)", head and tail print "standard input", wc and md5sum print "-". The test enumerates the whole registry and asserts every command is either a `-`-reads-stdin row or explicitly listed as not one, so a command added later has to be classified. Each row is checked against the same command reading a file with the same bytes, rather than a transcript: the invariant is that the operand names a source. Removing the `-` arm of StdinOperand.read/4 turns 18 of them red; removing the /dev/null arm turns 4 red. Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
d59d84d taught nineteen commands that a bare `-` names stdin, and enumerated the registry to prove it. The enumeration only ever built a *single*-operand invocation, and that is blind to the bug it was meant to catch: a parser that deletes `-` from the operand list still looks correct when `-` is the only operand, because deleting it leaves the command falling back to stdin anyway. With a file beside it, four commands the fix note called already-correct still resolved `-` as a path: printf 'SSS\n' | shasum - /f rc=1 shasum: -: No such file or directory printf 'SSS\n' | sha256sum - /f rc=1 sha256sum: -: No such file... printf 'SSS\n' | tac - /f rc=0 "FFF\n" -- stdin dropped printf 'SSS\n' | rev - /f rc=0 "FFF\n" -- stdin dropped Both known shapes. `shasum`/`sha256sum` guard with `files == [] or files == ["-"]` and hand anything else to `FS.read_file/2`, so `shasum - /f` is exactly the failure signature the `-` fix existed to remove. `tac`/`rev` do `Enum.reject(args, &String.starts_with?(&1, "-"))`, the same shape as the `base64 - /f` bug d59d84d already found. Byte-identical at bb55214 and e1028b6: pre-existing, not a regression. That reject is not an option/operand split. It also deletes every operand after `--`, which is the only way to name a file whose name begins with a dash, so `tac -- -f` read nothing and exited 0. Both deletions are silent. `StdinOperand.operands/1` is the split done properly: `-` is an operand, `--` ends the options, everything else is as before. `tac` needed more than the split. GNU reverses each operand's lines on its own and writes the operands in the order given -- `tac a b` is `tac a; tac b`, not `cat a b | tac` -- and this `tac` concatenated first, so it also reversed the operands against each other. `rev` is line-local, so operand-order concatenation already places each operand's lines where they belong. $ printf 'SSS\n' | shasum - /f ba3189fb712d534af1ea2fe27ea107dbdc296775 - fb5d5f20adab4de2ac06bedb9f5d0768186157db /f $ printf '1\n2\n' | tac - /b # /b holds 3\n4\n 2 1 4 3 matching `shasum - f`, `gsha256sum - f` and `gtac - b` byte for byte, in both operand positions. The hole in the test is closed the way the rows are already checked: against the same command reading a real file, not against a transcript. Every row now also runs as `cmd - FILE` and `cmd FILE -`, with `/s` -- a real file holding the stdin bytes -- standing in for `-` in the reference run. Because the reference is itself a two-operand run, a command that reads only its first operand compares equal to itself: what is under test is the classification of `-`, not multi-operand support. The matrix is red on exactly these four against d59d84d, and green on the other twenty-two rows. Removing the `-` arm of `StdinOperand.read/4` now turns 40 rows red, up from 18; removing `operands/1`'s `-` clause turns 3 red; removing its `--` clause turns 1 red. Left alone, and not about `-`: `tac`/`rev` still swallow an unrecognised option rather than diagnosing it (`rev -x f` exits 0 where GNU and BSD exit 1), `tac` still ignores `-b`/`-r`/`-s`, `sort` still reads only its first operand, and `base64` concatenates its operands where GNU calls the second one an extra operand. Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
fe87d76 to
935ca2a
Compare
Two independent pieces of the #70 roadmap. They touch disjoint files, so a
reviewer who wants them split can take either alone:
13dbe9c,e64d948,273ac60,7708142,51b6d47,0d0b54d,e1028b6—item 1's first bullet: the
FS.strerror/1sweep, plus the commands thesweep's grep could not reach because they printed nothing
bb55214,7aa07f8— item 7's prerequisite: the Oils spec parserNothing else from the roadmap is here. No Docker oracle, no alphabet matrices,
no filesystem-shape matrix, and the spec suite is not activated in CI.
Review round two is folded in — six per-site conformance fixes, the coverage
hole in the matrix, and the parser's handling of bash-keyed annotations. The
finding-by-finding reply is
here.
Piece A — the
strerrorsweepWhat was broken
33 command modules matched
{:error, _}, threw the error kind away, andspelled every filesystem failure
No such file or directory. Only 13 calledFS.strerror/1.stat.ex:50was the representative case in the issue:Repro, with
/fa regular file so/f/xisENOTDIR:GNU coreutils prints the kind, not a guess. This is the issue's stated
prerequisite for the rest of the roadmap: it is estimated at 40–60% of all
divergences an enumerated matrix would report, and it is the same diff a
thousand times.
The fix
Every one of them now renders
FS.strerror/1on the error it was alreadyhanded. The message template is untouched wherever GNU uses one template for
every kind, so the diff is one line per site. Two incidental cleanups fall out:
rm's:enoentclause became identical to the general clause below it and isgone, and
source's catch-all no longer sayscannot readfor every kind itdid not enumerate.
JustBash.exec_file/2inlib/just_bash.exhad the same defect and is fixedwith them — it is outside
commands/, so it is the one site the issue's grepdid not name.
Where GNU does not use one template for every kind, the template branches:
head,tail,tac,sortanduniqopen(2)a directory successfully andonly fail at
read(2), so EISDIR gets its own wording.Two more per-site fixes:
file -bnames the reason instead of returning a barecannot open, andrealpathwithout-ecanonicalises a missing finalcomponent and exits 0 the way GNU does —
-eand-mwere being parsed anddiscarded, and all three modes are real now.
The other half of the family: the commands that printed nothing
Grepping for the literal
"No such file or directory"reaches every modulethat printed a wrong message and none that printed no message at all. Those
are the worse members of the family, and every one is on #70 item 3's operand
list:
An agent writing
sort report.txt | head -5against a mistyped path got aclean exit-0 empty result with nothing to distinguish it from an empty file.
wc -l < /nopeprinting a fabricated0is the sharpest one, because thenumber looks like an answer.
grepkeeps GNU's rule that-qwith a line selected exits 0 even after anerror, and
cutkeeps the files it could read.<is the read side of025673c: the shell opens the target, so the shell reports the failure and thecommand never runs. EISDIR is deliberately excluded there —
open(2)on adirectory succeeds, so bash runs the command and the command's own read fails
(
cat: stdin: Is a directory), which is not a shell-level diagnostic and hasno seam here.
md5sumwas writing its diagnostic to stdout, between checksum lines, somd5sum a b > sums.txtwrote a malformed checksum line into the file and2>/dev/nulldid not silence it; it writes to stderr now, andmd5sum -conan unreadable checksum file reports instead of exiting 0.
filewriting tostdout is correct — real
file(1)does the same.The tests
test/commands/error_message_test.exsis the cross product of thefilesystem-touching commands and the kinds a path can fail with — 188 cases
generated from a table, not written one at a time. Each row is the invocation,
the kinds it covers, the stream the diagnostic belongs on, and its template,
with
PATHandMSGfilled per kind:Reading file contents can hit all four of
:enoent/:enotdir/:eisdir/:eacces; a command that only inspects metadatais crossed with three, because a directory is a perfectly good answer for
stat,find,duand friends. A row carries its own kind list rather thansitting in a "content reader" or "metadata reader" bucket, which is what lets
one command have two templates.
Operand arity is part of the cross product too.
head,tailandwceachhave a single-file arm and a reduce over several files, and
sha256sumandshasumhave a third arm behind-c; with every row pinned to one operand,seven of the sites this sweep exists to fix could be reverted with the suite
still green (proven by mutation: replacing the rendered strerror with a literal
at all seven left 4923 tests, 0 failures). Rows naming two operands,
-c FILE,and a checksum file that lists an unreadable target reach them — the same
mutation now turns 21 tests red.
:eaccesis unreachable from the in-memory backend — it has no permissionmodel, and
chmod 000is ignored on read.test/support/failing_backend.exis a
VFS.Mountablewhose every operation fails with one configured kind,mounted at
/mnt. That is also the only route to:erofs/:eio/:enotsupwhen the roadmap gets to them.
82 of the first 126 failed on
main:Piece B —
lib/just_bash/spec_test/parser.exWhat was broken
Nothing calls the parser, so nothing caught any of it.
The four the issue lists
:skipis never assigned.runner.ex:48's guard was dead code.Compounding it, Oils writes
## SKIPabove the script andcollect_script/2stopped at the first##line — so a SKIP-marked casewas parsed with an empty script and passed vacuously. All 247 of them:
parsed as
script: "", skip_reason: nil.String.trim_leading(line, "#### ")removes every leading occurrenceof its argument, not one.
#### #### nestedyieldsnested, not#### nested. No corpus case is spelled that way today, which is why itwent unnoticed — so this one defect is tested on synthetic input while the
others are tested on real files.
String.trim(script)trims whitespace characters. The format meansto drop the blank lines that frame a case; trimming characters also rewrites
the last command of every case whose script ends in a space.
The corpus pins down the correct rule.
builtin-trap-err.test.sh:line=3only holds if the leading blank line is dropped and the interiorone is kept. So the parser now trims at line granularity: whole blank lines
at either end go, everything between the first and last non-blank line is
preserved byte for byte.
## STDERRis never read. 65 of the 2728 cases carry a stderrexpectation, and stderr is where the silently-ignored-flag class lives.
Two more, from review
Every
## OK bash …/## BUG bash …/## N-I bash …annotation wasdropped as "another shell's expectation". It is the opposite: the
unannotated default records osh, and an annotation naming a shell is what
that shell actually does.
append.test.sh"Try to append list toelement" records
## stdout-json: ""/## status: 2and then## OK bash status: 0with## OK bash STDOUT:→['1', '2 3']; theparser produced the osh expectation, so a JustBash that reproduced bash
exactly would have been scored a failure. 440 such lines cover 323 of the
2728 cases across 89 files — including two
## OK just-bashmarkerswritten by this repo.
A bare
## stdout:— the format's spelling of one expected empty line —was not recognised, because the clause required the separating space. Four
cases parsed to
expected_stdout: nil, andRunner.check_output(nil, _)passes a case whatever it printed.
sh-func.test.sh"Locals don't leak"would have been scored
passedif the local had leaked.The fix
A case body is now sorted line by line into script or directive, in either
order, rather than assuming directives only follow the script. A multiline
block keyed to another shell (
## N-I dash STDOUT:…## END) is consumedalong with its body, so lines of expected output no longer fall through into
the script.
TestCasegainsexpected_stderr;Runnerreports a skipped caseas skipped instead of as an error, keeps skips out of the pass rate, and
compares stderr when the case names one.
Annotations are parsed into
{qualifier, shells, key: value}and applied perkey, weakest first: unannotated default →
bash→just-bash. The formatoverrides one key at a time, which is why
append.test.shkeeps its defaultstdout-jsonuntil## OK bash STDOUT:replaces it while## OK bash status: 0replaces the status on its own line.bash-2is bash 2.x and staysforeign.
The tests
test/just_bash/spec_test_parser_test.exs, against the real files intest/command_spec_cases/bash/cases/. The corpus-wide counts independentlyreproduce three numbers the issue states from the other side — 2728 cases,
247 SKIP markers, 65 stderr expectations:
Those four are blind to a directive spelling the parser silently stops
recognising, so three more are pinned — 2562 cases with a stdout
expectation, 239 with a non-zero status, and only 59 that assert
nothing at all — and an independent reader re-derives the annotations straight
from the files without going through the parser: 440 bash-keyed lines, 323
cases, 89 files, and every one of the 124 cases carrying a bash-keyed
status: Nmust parse to that status.Plus: no script contains a
##line; the$LINENOcase above parses to thescript its expectation assumes;
Runnerdoes not execute a skipped case anddoes not count it as a failure.
13 of the 32 fail with the old parser (
git stash push -- parser.ex):The suite is still inert. Activating it — the curated subset, the re-recording
and the ratchet — is the rest of item 7.
Gates
The one credo finding is the pre-existing intentional test fixture, and the 13
dialyzer errors are the pre-existing entries in
.dialyzer_ignore.exs. 4878tests before, 4988 after: +110.
Refs #70 (items 1 and the item-7 parser prerequisite; the rest of the roadmap stays open)