fix: a trailing slash on a destination requires a directory instead of overwriting the file it names - #75
Conversation
…f overwriting the file it names
POSIX resolves `f/` as `f/.`, so the slash is an assertion that `f` is a
directory — not a second spelling of the regular file `f`. `FS.resolve_path/2`
normalizes the slash away, so every command that only saw the resolved path
wrote straight through it: `cp /a.md /f/` overwrote `/f` and exited 0, and so
did `mv /a.md /f/`, `cat /a.md > /f/`, `tee /f/`, `ln -sf /a.md /f/` (which
unlinked `/f` first) and `touch /f/`.
The rule now lives at the path layer. `FS.directory_spelling?/1` answers
whether an operand's spelling demands a directory — a trailing slash, or the
`.`/`..` components it stands for — and `FS.check_directory_spelling/3` holds
a resolved path to that promise, returning the error a real `stat("f/")`
gives: `:enotdir` for something else already there, `:enoent` for a
destination that is not there at all.
Each destination-taking command combines that with what it is about to put
there, so the promise is judged against the result: a recursive copy or a
directory move creates the directory a slash promised (`cp -r /d /nope/`
still works), while a file copy, a redirect, tee, ln and touch cannot and say
so. `cp -n` no longer quietly "keeps" a destination that the spelling says is
not a file, and `cp /a.md /a.md/` is a failed stat rather than a self-copy.
mv now stats the source before judging the destination, the order bash uses,
so a missing source is still reported first. Redirection names the target the
way it was written rather than where it resolved, which is what bash prints
and the only spelling that can carry the slash.
Wording for every case was taken from GNU coreutils 9.11 and bash 5 and is
quoted next to the test that asserts it.
davydog187
left a comment
There was a problem hiding this comment.
Two lenses were applied to this PR — a POSIX/GNU-oracle correctness lens and a test-design lens. Eleven candidate findings went in; five survived verification against a worktree of d8976dc compared side-by-side against 82ee9e5 (main). Everything below was reproduced by running it.
The fix itself is the right shape — the operand's spelling really is the only place the information survives, and cp, tee, touch, ln and redirection all land correctly. The blocker is confined to mv.
1. mv's same-file refusal is bypassed by any directory spelling, turning it into a silent exit-0 no-op
blocker — lib/just_bash/commands/mv.ex:58 (distinct/3), reached from mv.ex:28
distinct/3 skips the "are the same file" diagnostic whenever FS.directory_spelling?(dest) is true:
if FS.normalize_path(src_resolved) == FS.normalize_path(dest_final) and
not FS.directory_spelling?(dest) do
The guard was written for mv a.md a.md/, where destination_directory/4 reports ENOTDIR instead. But it fires for the ordinary dir/ spelling too. When the destination really is a directory, destination_directory/4 returns :ok, and rename/3 calls FS.mv(fs, src, src) — whose src_norm == dest_norm branch (fs.ex:291) returns {:ok, fs}. Exit 0, empty stdout, empty stderr, nothing moved, no test red.
Measured on the branch vs. main, same fixture (/a.md, /f regular files, /d/keep):
| command | main | this PR | GNU coreutils 9.11 |
|---|---|---|---|
mv /d/keep /d/ |
1, mv: '/d/keep' and '/d/keep' are the same file |
0, silent | 1, are the same file |
mv /d/keep /d/. |
1, same | 0, silent | 1 |
cd /d && mv keep . |
1, same | 0, silent | 1 |
mv /a.md / |
1, same | 0, silent | 1 |
mv /a.md // |
1, same | 0, silent | 1 |
mv /d/keep /d/../d/ |
1, same | 0, silent | 1 |
mv /a.md /f/.. |
1, same | 0, silent | 1, cannot stat 'f/..': Not a directory |
mv /d /f/.. |
1, same | 0, silent | 1 |
mv /d/keep /d (no slash) |
1 | 1 | 1 |
Only the unslashed spelling still refuses. cp is unaffected — cp /d/keep /d/ still exits 1 — so this is mv-specific. A script guarded by mv … || exit now proceeds as if the move happened.
Suggested fix: don't guard distinct/3 at all — reorder the with so the spelling check runs first and distinct/3 only sees destinations whose spelling already held:
with {:ok, src_type} <- source_type(bash, src, src_resolved),
:ok <- destination_directory(bash, {src, src_type}, dest, dest_resolved),
:ok <- distinct(src_resolved, dest_final) do
That restores every row above and, as a bonus, makes the code match the comment two lines up ("bash stats the source … and only then asks whether the two name the same file"). Today mv /nope.md /nope.md reports are the same file where GNU reports mv: cannot stat 'nope.md': No such file or directory, because distinct/3 runs before source_type/3. I checked the reorder against every case in trailing_slash_test.exs: mv /a.md /f/, mv /a.md /a.md/, mv /missing.md /f/, mv /a.md /nope/, mv /d /nope/ all keep their asserted wording.
2. .. is accepted as a directory-demanding spelling but nothing can enforce it
major — lib/just_bash/fs/fs.ex:116 (directory_spelling?/1) and fs.ex:133 (check_directory_spelling/3)
directory_spelling?/1 returns true for a trailing .., its docstring says the . and .. components "say the same thing" as a trailing slash, and trailing_slash_test.exs:377 asserts it. But check_directory_spelling/3 validates the path resolve_path/2 already collapsed lexically: for /f/.. where /f is a regular file, resolved is /, which is a directory, so the check returns {:ok, fs} and every caller is told the promise held.
The .. half of the contract is therefore inert at the command level:
touch /f/..→ exit 0, no output. GNU:touch: cannot touch 'f/..': Not a directory, exit 1. This is the silent-success one.cp /a.md /f/..→cp: '/a.md' and '/f/../a.md' are the same file. GNU:cp: cannot stat 'f/..': Not a directory.echo hi > /f/..→bash: /f/..: Is a directory. bash:Not a directory.echo hi | tee /f/..→tee: /f/..: Is a directory. GNU:Not a directory.ln -s /a.md /f/..→File exists. GNU:Not a directory.
The trailing-slash and /. cases are all correct; only .. is wrong, because . collapses to the same place while .. does not.
And the test suite cannot see it. The . half of the predicate is exercised end-to-end four times (cp /a.md /f/., mv /a.md /f/., echo hi > /f/.); the .. half appears exactly once, in the directory_spelling?/1 unit test at line 377 — the one place where it cannot fail. That is the %-d/%_d/%0d lesson from #70 repeating: the assertion is placed where the bug is invisible. Enumerating {command, dest_spelling, dest_state, expected} tuples and looping over them would have covered .. for every command and caught finding 1 as well.
Suggested fix: either resolve the operand component-by-component before the check (so /f/.. fails on /f the way the kernel does), or narrow directory_spelling?/1's contract to the spellings the check can actually hold — drop .. from the predicate and the docstring — so callers stop believing it is covered. Whichever you pick, add at least one command-level .. case.
3. sed -i still writes through a trailing-slash destination
minor — lib/just_bash/commands/sed.ex:202 (process_single_file_in_place/4)
The PR body says it surveyed "the other commands that write to a named destination" and turned up three (tee, touch, ln -sf). It missed a fourth, and it is destructive:
$ sed -i 's/F/Z/' /f/ # /f contains "F\n"
exit 0, stderr "", /f is now "Z\n"
$ sed -i 's/F/Z/' /f/.
exit 0, /f rewritten
Real sed never reaches that state: open("f/", …) fails with ENOTDIR before any editing, so GNU sed reports a read failure and exits non-zero with the file untouched. sed -i is not on the deliberate-deferral list — it is neither a read-side operand like cat f/ nor one of the network-backed -o writers. FS.check_directory_spelling/3 makes this a one-call wiring job now.
(rm f/ has the same shape and is not raised here: the PR names it explicitly as deferred, with a reason — rm -f precedence — so that is a scoping decision rather than an oversight.)
4. ln -sf reports GNU's non--f wording
minor — lib/just_bash/commands/ln.ex:34, ln.ex:195
Running the check in link/2 before -f unlinks anything is exactly right, and it is the fix that stops ln -sf /a.md /f/ from deleting /f. But both the plain and the -f case report through create_failed/3, while GNU words them differently — with --force (or --backup/--interactive) coreutils lstats the destination first and reports that failure:
$ ln -s a.md f/ ln: failed to create symbolic link 'f/': Not a directory # we match
$ ln -sf a.md f/ ln: failed to access 'f/': Not a directory # we print the line above
The PR's own test comment quotes GNU's failed to access wording and then asserts only =~ "Not a directory", so the divergence is known but unpinned. A create_failed/access_failed split keyed on opts.force closes it and lets the test assert the full string like its neighbours do.
5. Redirect diagnostics now name the operand, and nothing pins it
minor — lib/just_bash/interpreter/executor/redirection.ex:123
Threading target_path through open_target/4/opened/2 so open_failed/2 names the operand as written is correct — it is what bash prints — but it changes the message for every redirect failure, not just slash-spelled ones:
cd /d && echo hi > keep/x main: bash: /d/keep/x: Not a directory
PR: bash: keep/x: Not a directory (bash agrees with the PR)
echo hi > '' main: bash: /home/user: Is a directory
PR: bash: : Is a directory
The PR's justification for the change being safe — "All existing redirect assertions use absolute paths" — is precisely why nothing tests it: with an absolute target the operand and the resolved path are the same string. No test in redirect_preflight_test.exs, not_a_directory_test.exs or the new file uses a relative or empty target, so reverting open_failed(path, …) to open_failed(resolved, …) leaves the whole suite green. One test with a relative target under a non-root cwd pins the improvement.
Considered and dismissed (6 candidates)
rm /f/still deletes the file (rm.ex) — reproduced (exit 0,/fgone;rm -f /f/likewise). Butrm.exis untouched by this diff, the behaviour is identical on main, and the PR names it in "Left out, deliberately" with a stated reason (rm -fprecedence). Scope disagreement, not a defect in the change. Worth its own issue.mv's source-side ENOTDIR wording changed, untested — reproduced (mv /f/x /d: mainmv: cannot move '/f/x' to '/d': Not a directory, PRmv: cannot stat '/f/x': Not a directory). The new wording is the correct one — GNU mv stats the source first — and the:enoentshape of the samesource_type/3path is covered by "reports a missing source before judging the destination", so a revert could not stay green. No live risk.rename/3's:enoentclause is unreachable — I could not reach it (mv /a.md /nope/x, dangling-symlink source, missing nested destination parent all exit 0), but "unreachable" is a claim about every mount backend, not justMemory, and the clause is type-correct defence in depth carried over unchanged from main. Not provable enough to act on.- New checks discard the
fsthey are handed back — true ofCp.keep_or_copy/4's:neverclause,Mv.destination_directory/4,Mv.source_type/3andRedirection.directory_target/2. Real, but no traced cost: the only backends in the tree areFS.MemoryandFS.Posix, neither of which mutates state onstat. Style preference until a hydrating backend exists. - The integration matrix is hand-written and half-empty — the traced part of this (nothing exercises
..at the command level) is folded into finding 2. The rest — counting 45 cells ofcommand × spelling × destination-stateand noting which are empty — is a structural preference; a missingtee f/.case has no demonstrated failure behind it. mvonto a..spelling regressed to exit 0 — same root cause as finding 1 (distinct/3's bypass), merged into it as two table rows rather than repeated.
… lands
`directory_spelling?/1` accepts `..` as a spelling that demands a directory,
but `check_directory_spelling/3` was handed the path `resolve_path/2` had
already collapsed lexically: for `/f/..` with `/f` a regular file the resolved
path is `/`, a directory, so the check passed and every caller was told the
promise held. `touch /f/..` exited 0 having done nothing, `cp /a.md /f/..`
reported "are the same file", and redirection, tee and ln each reported the
wrong errno.
The check now asks about the last component the operand names outright —
everything before the run of `/`, `.` and `..` trailing it — which is the
component the kernel cannot walk up out of. Holding that one path is enough:
every component the run walks through afterwards is an ancestor of it, and an
ancestor of a directory is a directory. That needs the operand's base rather
than its resolved form, so `check_directory_spelling/3` takes
`(fs, base, spelling)` and resolves what it needs itself; the call sites pass
`bash.cwd`.
$ touch f/.. touch: cannot touch 'f/..': Not a directory
$ touch nope/.. touch: cannot touch 'nope/..': No such file or directory
$ cp a.md f/.. cp: cannot stat 'f/..': Not a directory
$ echo hi > f/.. bash: f/..: Not a directory
cp needed one more thing: a destination whose spelling does not hold is not a
directory the copy may land inside, so `dest_kind/2` checks before handing the
operand the source's basename.
The `..` half of the contract was previously exercised only by the
`directory_spelling?/1` unit test — the one place it could not fail. It is now
enumerated across touch, cp, mv, redirection, tee and ln, alongside the
spellings that must still be accepted (`d/..`, a bare `..`).
Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
`distinct/3` skipped the "are the same file" diagnostic whenever the
destination's spelling demanded a directory. The guard was there for
`mv a.md a.md/`, where `destination_directory/3` reports ENOTDIR instead —
but it fired for the ordinary `dir/` spelling too, where the destination
really is a directory and the check passes. `FS.mv/3` then renamed the source
onto itself and returned `{:ok, fs}`: exit 0, empty stderr, nothing moved, no
test red. A script guarded by `mv … || exit` proceeded as if the move had
happened.
$ mv /d/keep /d/ was: exit 0, silent
$ mv /d/keep /d/. "
$ cd /d && mv keep . "
$ mv /a.md / "
$ mv /d/keep /d/../d/ "
The guard is gone. Instead the `with` runs in the order bash does — stat the
source, judge the destination's spelling, and only then ask whether the two
name the same file — so `distinct/2` only ever sees destinations whose
spelling already held and needs to ask nothing about it. That is what the
comment above the `with` already claimed, and it also fixes
`mv /nope.md /nope.md` reporting "are the same file" where GNU reports
"cannot stat".
The eight spellings above are enumerated as a table rather than picked by
hand, each asserting the exact diagnostic and that both files are still where
they were.
Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
`process_single_file_in_place/4` resolved the operand and wrote straight through it, so `sed -i s/F/Z/ /f/` exited 0 with empty stderr and rewrote the regular file `/f` — the same overwrite #58 is about, in the one destination-writing command the original survey missed. Real sed cannot open `f/` at all (ENOTDIR) and leaves the file untouched. The in-place path now holds the operand to its spelling before reading it. The non-`-i` path is left alone: that is a read-side operand, which the PR defers along with `cat f/` and `grep … f/`. Re-running the survey properly, the commands whose command-line operand names a file they write are cp, mv, ln, tee, touch, sed -i and shell redirection — all seven now check — plus, deliberately deferred and unchanged: `rm f/` (a removal operand, and `rm -f` precedence comes with it), `mkdir f/` (already requires a directory; only the wording differs), `curl -o f/` / `wget -O f/` (the write sits behind the HTTP-client mock), and awk's in-program `print > "f/"` (an awk redirection rather than a shell operand, and awk does not resolve it against the cwd either). Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
…reated
Both the plain and the `-f` case reported through `create_failed/3`, but GNU
words them differently: under `--force` (or `--backup`/`--interactive`)
coreutils lstats the destination before unlinking it and reports that failure
instead of the create it never attempts.
$ ln -s a.md f/ ln: failed to create symbolic link 'f/': Not a directory
$ ln -sf a.md f/ ln: failed to access 'f/': Not a directory
$ ln -f a.md f/ ln: failed to access 'f/': Not a directory
$ ln -sf a.md nope/ ln: failed to create symbolic link 'nope/': No such file …
`:enoent` is not a failed lstat — there is simply nothing to unlink — so a
missing destination keeps the create wording, which is why the split is keyed
on the error kind as well as on `force`.
The `ln -sf` test quoted GNU's wording in a comment and then asserted only
`=~ "Not a directory"`; it now asserts the full string like its neighbours,
and `ln -f` and `ln -sf … nope/` are pinned alongside it.
Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
…s ENOENT
Threading `target_path` through `open_target/4` and `opened/2` so
`open_failed/2` names the operand as written is what bash does, but it changed
the message for every redirect failure and nothing covered the change: with an
absolute target the operand and the resolved path are the same string, and
every existing redirect assertion used one. Reverting to
`open_failed(resolved, …)` would have left the suite green.
A relative target under a non-root cwd separates the two, so the improvement
is now pinned:
$ cd d && echo hi > keep/x bash: keep/x: Not a directory
$ cd d && echo hi > keep/ bash: keep/: Not a directory
$ cd d && echo hi > nope/ bash: nope/: No such file or directory
$ cd d && echo hi > ../f/ bash: ../f/: Not a directory
Checked against bash 5, the empty target was wrong: `open("")` is ENOENT, and
naming the working directory the operand resolved to is neither the right
errno nor the right name.
$ echo hi > '' bash: : No such file or directory (was: Is a directory)
Claude-Session: https://claude.ai/code/session_013UQWCDMJdSCZhJVkeFpN8z
|
All five confirmed findings are addressed. Each was reproduced against
1 —
|
Verification of review fixesIndependently re-ran every repro at
Gates (re-run at
|
Closes #58
What was broken
POSIX resolves
f/asf/., so a trailing slash asserts that the path is adirectory.
FS.resolve_path/2normalizes it away, so a command that only seesthe resolved path cannot tell
ffromf/— and wrote straight through it.The repro from the issue:
Real bash / GNU coreutils 9.11:
The sibling shapes named in the issue behaved the same way —
mv /a.md /f/silently overwrote
/fand deleted/a.md, andcat /a.md > /f/truncatedit. Surveying the other commands whose command-line operand names a file they
write turned up four more:
tee /f/overwrote it,touch /f/was a silentno-op,
sed -i s/F/Z/ /f/rewrote it, andln -sf /a.md /f/unlinked/fbefore failing to create the link.
What the fix does
The issue argued the fix belongs at the path layer, and it does — the operand's
spelling is the only place the information survives. Two new functions in
JustBash.FS:directory_spelling?/1— pure: does this operand's spelling demand adirectory? A trailing slash, or the
./..components it stands for.(An empty operand names nothing at all, which is a different complaint.)
check_directory_spelling/3— takes the operand's base and its spelling andholds it to that promise, returning the error a real
stat("f/")gives::enotdirwhen something that is not a directory is already there (namingthe operand as spelled, since that is what gets reported),
:enoentwhennothing is there at all.
The demand is made about the last component the operand names outright —
everything before the run of
/,.and..trailing it — not about whereresolve_path/2says the whole thing lands...is collapsed lexically, so/f/..resolves to/, a directory whatever/fis, while the kernel cannotwalk up out of the regular file
/fat all. Holding that one path is enough:every component the run walks through afterwards is an ancestor of it, and an
ancestor of a directory is a directory.
Each destination-taking command combines that with what it is about to put
there, which is the distinction GNU makes:
cp a.md f/cp: cannot stat 'f/': Not a directorycp: cannot create regular file 'nope/': No such file or directorycp -r d f/cp: cannot stat 'f/': Not a directorymv a.md f/mv: cannot stat 'f/': Not a directorymv: cannot move 'a.md' to 'nope/': No such file or directorymv d f/mv: cannot stat 'f/': Not a directory> f/bash: f/: Not a directorybash: nope/: No such file or directorytee f/tee: f/: Not a directorytee: nope/: No such file or directorysed -i s/x/y/ f/sed: f/: Not a directorysed: nope/: No such file or directoryln -s a.md f/ln: failed to create symbolic link 'f/': Not a directoryNo such file or directoryln -sf a.md f/ln: failed to access 'f/': Not a directoryln: failed to create symbolic link 'nope/': No such file or directorytouch f/touch: cannot touch 'f/': Not a directorytouch: cannot touch 'nope/': No such file or directoryA directory target keeps its own diagnostic:
> d/is stillbash: d/: Is a directory, andtouch d/still exits 0. So doestouch d/..,and a bare
touch ...Details that fall out of matching bash rather than bolting a check on the front:
mvstats the source first, then judges the destination's spelling, and onlythen asks whether the two name the same file — the order bash uses. So
mv /missing.md /f/reports the missing source,mv /nope.md /nope.mdreports
cannot statrather thanare the same file, andmv /d/keep /d/is still refused as a same-file move rather than becoming asilent exit-0 no-op. Its
execute/3body was split into named steps(
source_type→destination_directory→distinct→rename) to makeroom without going past credo's nesting limit.
cp -nno longer quietly "keeps" a destination that the spelling says is nota file, and
cp /a.md /a.md/is a failed stat rather than a self-copy —a.md/is not a name the regular filea.mdhas. A destination whosespelling does not hold is also not a directory the copy may land inside, so
it never gains the source's basename.
lnchecks before-funlinks anything, which is the whole point forln -sf a.md f/. GNU words the--forcecase differently — it lstats thedestination first and reports that failure (
failed to access) — exceptfor
:enoent, where there is nothing to unlink and the create it goes on toattempt is what fails.
resolved (
bash: f/: …, notbash: /w/f: …). That is what bash prints, andit is the only spelling that can carry the slash. An empty target is
bash: : No such file or directory—open("")is ENOENT, not a write tothe working directory that resolving it would produce.
Tests
New
test/just_bash/trailing_slash_test.exs— 75 tests. Every message wastaken from a real shell (GNU coreutils 9.11 / bash 5) and the transcript is
quoted next to the case that asserts it. Each destructive case asserts the
destination is still intact as well as the exit code and diagnostic. The
corpus covers:
cp(plain,-r,-n,-P,f/., self-copy, missingdestination, three-operand target, and symlinks to a file and to a
directory),
mv(file, directory, missing destination, missing-sourceordering, and eight spellings of a destination that resolves back to the
source), redirection (
>,>>,2>,&>, directory target, missing target,relative targets under a non-root cwd, an empty target, and that the command
body never runs),
tee,ln -s,ln -sf,ln -f,sed -i,touch, a{command, expected}table covering..across six commands, plus unit testsfor both
FSfunctions.Regression guards that already passed (copying/moving into a real directory,
cp -r d nope/,cp a.md b.md f/,touch d/,touch d/..,sed -ion aplain name) are kept in the file so the fix cannot over-correct into refusing
legitimate directory destinations.
Left out, deliberately
cat f/,grep … f/,ls f/,sed f/without-i). GNU rejects these too, but the issue is about destinations and theread side is a much wider surface; worth its own issue.
rm f/andmkdir f/.rm /f/still removes/f(GNU:rm: cannot remove 'f/': Not a directory) — same rule, but a removal operand rather thana destination, and it drags
rm -fprecedence in with it.mkdir/rmdiralready require a directory, so only the wording differs there.
curl -o f//wget -O f/. Same shape, but the write happens after anetwork fetch and behind the HTTP-client mock, so it belongs with a change
that can test that path properly.
print > "f/". An awk redirection rather than a shelloperand, and awk does not resolve the filename against the cwd either —
a separate, pre-existing gap.
cp -r src nested/deep/. GNU refuses to create missing parents; wecreate them. Pre-existing and independent of the slash (
cp -r src nested/deepbehaves the same way)...after a non-directory (cp a.md f/../g).resolve_path/2collapses it lexically the same way, so we create
/gwhere GNU reportsNot a directory. That is aresolve_path/2gap rather than a destinationgap, and it needs component-wise resolution to fix properly.
mvnaming each side of "are the same file" as spelled. GNU printsmv: 'd/keep' and 'd/./keep' are the same file; we name both by where theyresolved. Pre-existing, and the refusal is what cp/mv accept a trailing slash on a regular-file destination (POSIX requires a directory) #58 is about.
mix bash_fixtures), so the oracle is quoted in comments instead.Gates
(the one credo finding is the pre-existing intentional test fixture)