File tests answer from the mount and the mode, not from an absent bit - #412
Merged
Conversation
These fail on shipped 0.16 for /v/bin and /v/jobs, and would fail on the abandoned close-the-default branch for /v/probe.txt and /dev/null. Both spellings of every case run through one helper so a case covering only `test` or only `[[ ]]` cannot be written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An absent DirEntry.permissions is not an answer about writability. MemoryFs and DevFs report None and are writable; BuiltinFs and JobFs report None and are not. So the mount's read-only state has to be in the answer, and a read-only LocalFs wrapper over an OS-writable directory shows the converse: the stat bits alone are not the answer either. PathAccess::resolve takes both and is the only constructor, so no caller can consult one of them by accident. with_write_layer exists for copy-on-write overlays, where reads resolve against whichever layer holds the path but writes always land in the upper. The three answers deliberately treat an absent mode differently. Readable: no restriction, so it reads. Writable: no information, so the mount decides. Executable: nothing to hand exec(2), so false — read-only-ness is about writes and says nothing here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Amy's call: an absent mode is absent because nobody polished it, not because it means anything. Fill it in on the writable backends and the ambiguity dissolves. Adds the two cases that ruling creates. `-x /v` and `-x /v/sub` become YES once MemoryFs directories are 0o777 — x on a directory is searchable, which is the POSIX answer and a real user-visible change. `-w /dev` stays NO against a mode of 0o555 rather than Linux's 0755, with `mkdir /dev/newthing` as the receipt: kaish's DevFs::mkdir refuses every caller, so 0755's root exception has nobody to apply to. Five red: /v/bin, /v/jobs, the read-only-wrapper hazard, and the two new mode cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MemoryFs left permissions as None everywhere because nobody went back to fill it in, not because absent meant anything. It does: MemoryFs is writable and reported the same None as JobFs, which is read-only, so `test -w` had no signal to read and had to guess. Directories 0o777, files 0o666, symlinks 0o777. MemoryFs never refuses an operation over a mode, so these describe what it does rather than restrict it — and there is no chmod builtin, so they are observations only. Files are not executable because real_path is None for a memory-backed path and there is nothing for exec(2) to open. Directories at 0o777 makes `[[ -x /v ]]` answer YES where it answered NO. That is the POSIX meaning of x on a directory — searchable — and these directories are searchable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DevFs is the second backend in MemoryFs's position and the one that makes the mount-level answer insufficient on its own: read_only() is deliberately false, because refusing writes would break `> /dev/null`, so nothing but a mode can distinguish a writable device from an unwritable directory. Device nodes are 0666, matching crw-rw-rw-. The /dev directory is 0555 and not Linux's 0755. Amy asked for 0755 on the "real Linux is the honest model" argument, and the argument is right — but the 2 in 0755 is root's, and kaish has no root. DevFs::mkdir and DevFs::remove return PermissionDenied for every caller with no exception, so 0755 would make `test -w /dev` answer YES about a directory that accepts nothing: the same shape of lie this whole change removes. 0555 is what this mount actually does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The premise behind closing the -w default is that every backend still reporting None is read-only. Enumerating them turned up one that is not, and it is not a virtual backend: LocalFs::extract_permissions returns None under cfg(not(unix)), and LocalFs is writable. Closing the default without this would answer NO for every file on Windows. There is one permission fact available there — Permissions::readonly() — so the synthesized mode carries exactly that and nothing else. The x bit is never set on a file: executability is decided by extension on those platforms, not by a permission, and -x already answered false here. Split out as a pure function so it has a test on Linux, where nothing calls it. CI is ubuntu-only, so an untested cfg branch would be a claim with no receipt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
With LocalFs, MemoryFs, DevFs and OverlayFs all reporting modes, the backends still returning None are BuiltinFs and JobFs, and both are read-only. So absent stops meaning "we don't know" and starts meaning "this backend does not model permissions": readable, not writable, not executable. That premise is load-bearing rather than incidental, so it is written into the type's docs with the consequence attached — a writable backend reporting None will be told its paths are unwritable. An embedder adding one reports a mode instead of leaning on a default here. resolve() still takes both facts. Filling the modes in did not make the mount check redundant: a LocalFs::read_only wrapper reports the OS's permissive bits and refuses every write anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Filling the modes in does not finish the job. VfsRouter::read_only() answers "are ALL mounts read-only", which is the wrong question for one path, and find_mount — the thing that resolves a path to its owning mount — is private. Meanwhile LocalFs::stat reports the OS's mode bits and knows nothing about a LocalFs::read_only wrapper around it, so a mode-644 file on a read-only mount looked writable while every write to it failed. path_access lands on Filesystem and on KernelBackend, both defaulted so no existing implementation breaks. The default pairs stat's mode with the implementation's own read_only(), which is right for a uniformly read-only or uniformly writable backend. VfsRouter overrides it to ask the mount that owns the path, LocalBackend forwards to the router, and VirtualOverlayBackend routes it exactly the way it routes stat. OverlayFs overrides it for a different reason: reads resolve against whichever layer holds the path but writes always land in the upper, and OverlayFs::write never consults the lower's mode. A mode-444 lower file is writable through copy-up, and with_write_layer is how that is said. Both file-test sites now call path_access and neither one touches a mode bit. -e/-f/-d still go through stat. The two sites have drifted before, so every case in file_test_writable_tests runs through one helper that asserts both spellings; a case covering only one cannot be written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
I had OverlayFs override path_access so the upper answered `writable` — copy-up ignores the lower's mode, so a mode-444 lower file really is writable through the overlay, and the visible entry says otherwise. Closing the absent-mode default killed it. When the path is not in the upper yet there is no mode to consult, and the override had to invent one for a file that does not exist. Inventing a mode is precisely what this change removes; doing it here to fix a case nobody asked about is not a trade worth making. So OverlayFs keeps the default and the inaccuracy is documented on the trait instead. It is not a regression — the answer is the same one 0.16 gave. Correcting it needs a decision about what mode a not-yet-copied-up path is judged by, and that decision is not in the code today. with_write_layer went with it. It existed for this one caller. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sweeping for backends that still report no mode turned up a gap I had just made. VfsRouter::stat synthesizes a directory for the root and for any ancestor of a mount — `/v` above `/v/jobs` — so those paths exist. path_access went straight to find_mount and errored on exactly those, which made `[[ -e /v ]]` true and `[[ -r /v ]]` false about the same path. Synthesized directories are 0555: readable, searchable, and never writable, because they are derived from the mount table and the router creates nothing in them. The kernel-routed test for this passes without the fix — with_backend wraps the router in a VirtualOverlayBackend, whose own override already answered for /v — so the test with teeth is the router-level one. Kept both: the kernel test pins the behavior an embedder sees, the unit test pins the router contract that produces it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three comment corrections, no behavior change. BuiltinFs's module doc claimed it "presents builtins as executable entries". The code has never done that: stat reports no mode, so `test -x /v/bin/grep` is NO, and real_path is None, so there is nothing for exec(2) to open. Running a builtin goes by name through the ToolRegistry and never touches this filesystem. The doc now says what the code does, and records what would have to change for the original claim to hold — a mode with 0o111 set, which flips `test -x /v/bin/*` and wants its own decision. `read` returns a line starting with `#!`, which is the likeliest reason nobody noticed. A comment that lies is worse than a missing feature, because it stops the next person from noticing the feature is missing. Filesystem::path_access gains a table of who answers from real modes and who synthesizes, plus the instruction that matters: report a mode unless your backend is read-only. The closed -w default is correct only because BuiltinFs and JobFs are the last absent-mode backends and both are read-only. A writable backend reporting None gets every path called unwritable, nothing asserts against it, and the failure is a wrong answer rather than an error — so that backend's own tests will pass. LocalFs's non-Unix arm now says why it is live rather than a Windows courtesy: wasm32-wasip1 is not `unix`, `mod local` is unconditional, and CI builds that target every run while the wasi leg never runs the file tests. Without the synthesis the WASI build would answer "not writable" for every file and nothing would have said so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Amy's rule: when we have unix, do the stat and return a reasonable answer for the kernel's view of the world — if the kernel has read, the VFS sees read. The bug that hides behind mode bits: 0o222 means "some principal may write", not "this process may write". A root-owned 0o644 file has it set, so `test -w /etc/passwd` answered YES for a kaish running as an ordinary user who cannot write a byte of it. That is the same lie this whole change removes, relocated from the mount to file ownership, and nothing guarded it. So LocalFs overrides path_access and asks faccessat with AT_EACCESS — an access check against the effective uid/gid, the same primitive bash's `test -w` uses, so the semantics match what a shell user expects. Read, write and execute are asked separately because the kernel answers them separately. PathAccess::resolve stays exactly as it is and keeps serving MemoryFs, DevFs, BuiltinFs and JobFs. Their modes are ones we chose; there the bits are the whole truth. Only LocalFs has paths with an OS identity to check against, so only LocalFs can ask the real question — that split is documented at both sites. from_effective_access still ANDs in mount_read_only, and still must: a LocalFs::read_only wrapper is a kaish-level restriction the OS cannot see, so the kernel granting write does not settle it. Both facts, one funnel, same contract resolve keeps. rustix, not libc: unsafe_code is denied workspace-wide. It is already a normal dependency here via rustyline, procfs and terminal_size, so this adds no new supply chain. Unix-only; wasm32-wasip1 has no effective-uid model and keeps the synthesized-mode path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The discriminating case turned out to be constructible without root. A file we own at mode 0o022 has the 0o222 mask set — for group and other — while the owner class, the class that applies to us, has no write bit. Unix checks the owner class and stops, so the write fails: raw mask says YES, kernel says NO. That is the root-owned-/etc/passwd shape reproduced as an ordinary user. 0o044 and 0o011 do the same for -r and -x, and a real open() sits next to them as the receipt. Verified empirically before building on it rather than reasoned about: eaccess and a real open agree on all five modes probed. Mutation-tested. Disabling the LocalFs override reddens exactly permission_bits_for_another_principal_do_not_grant_access; dropping the mount AND from from_effective_access reddens exactly read_only_wrapper_over_writable_os_dir_is_not_writable. One targeted failure each, so both halves are falsifiable. Also corrects a comment I wrote last commit and should not have. I was told mod local is declared unconditionally and took it; it is #[cfg(feature = "localfs")], localfs pulls tokio/fs, wasm rejects that feature, and kaish-wasi builds default-features off — so wasm32-wasip1 never compiles LocalFs and the non-Unix arm is Windows, not WASI. The synthesis is still right and still worth keeping; the stakes I claimed for it were not. Checked with a real wasm32-wasip1 build of kaish-wasi, which also confirms the new cfg(unix) rustix dep does not leak there. Changelog folds both flips into one Fixed entry per Amy: a correction toward the right answer is a bug fix and does not earn alarm. Nobody scripted around -x denying a directory is searchable, or -w claiming a root-owned file is writable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
tobert
commented
Aug 27, 2026
tobert
left a comment
tobert
left a comment
Owner
Author
There was a problem hiding this comment.
Shorten the comment as recommended then LGTM.
| /// `read_only() == false` to reporting a mode, and the failure is a wrong | ||
| /// answer rather than an error, so the tests you write for your backend | ||
| /// will pass. If you add a writable backend, either report a mode or come | ||
| /// change `resolve` and this table together. |
Owner
Author
There was a problem hiding this comment.
This is way too long. No storytelling in the comments. Some of this belongs in the EMBEDDING.md.
Amy on the review: "This is way too long. No storytelling in the comments. Some of this belongs in the EMBEDDING.md." The backend-authoring table and the writable-backend warning move to EMBEDDING.md under Custom Backend, where an embedder adding a filesystem will actually be reading. The trait doc keeps the two-facts rule, the OverlayFs inaccuracy, and a pointer. Two doc blocks were also attached to the wrong item: PathAccess's docs sat on EffectiveAccess, leaving PathAccess undocumented, and synthesized_mode's sat on effective_access. Both reattached. The comments that narrated their own edit history -- what an earlier version of the comment claimed, that a claim was checked and removed -- are gone. That belongs here.
# Conflicts: # CHANGELOG.md
Owner
Author
|
Addressed the review. The backend table and the writable-backend warning moved to Two doc blocks turned out to be attached to the wrong item: Also cut the comments that narrated their own edit history — what an earlier version of a comment claimed, that a claim was checked and removed. That belongs in the commit message. Merged main to clear the CHANGELOG conflict; both Added entries kept. Gates green locally: |
tobert
added a commit
that referenced
this pull request
Aug 27, 2026
Same slip as the one on #412: the trim linked has_invalid_leading_zero from Token::NumericLiteral's public docs, and that fn is private. Caught by RUSTDOCFLAGS=-D warnings, which cargo doc alone does not apply.
The trim turned a plain reference to synthesized_mode into a link, and that fn is pub(crate) while path_access is public. RUSTDOCFLAGS=-D warnings caught it; cargo doc alone would not have.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
test -wanswered yes about paths every write fails on, andtest -xanswered no about directories that are searchable. Both were reading a bit that does not carry the answer:DirEntry.permissionswasNonefor MemoryFs and DevFs, which are writable, and alsoNonefor BuiltinFs and JobFs, which are not. No default over an absent mode is right when the absent bit is not the bit being asked about.Two halves. MemoryFs and DevFs now report real modes, so an absent mode means only "this backend does not model permissions" — and every backend still reporting
Noneis read-only. LocalFs answers from the OS instead, askingfaccessatwithAT_EACCESS, because the kernel can answer the real question and mode bits cannot: a root-owned0644file has the0o222mask set and is still unwritable by an ordinary user.A new
path_accessquery combines the owning mount's read-only state with the mode, and is the only constructor, so no caller can answer from one fact alone. It is defaulted on both traits.🤖 Generated with Claude Code