From 42440de9b3cfb2103aa7f5c9d170b7f0d2c9776b Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:01:19 -0400 Subject: [PATCH 01/17] test: pin what -w must answer on every backend, before the fix 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 --- .../tests/file_test_writable_tests.rs | 304 ++++++++++++++++++ 1 file changed, 304 insertions(+) create mode 100644 crates/kaish-kernel/tests/file_test_writable_tests.rs diff --git a/crates/kaish-kernel/tests/file_test_writable_tests.rs b/crates/kaish-kernel/tests/file_test_writable_tests.rs new file mode 100644 index 00000000..a952fbbb --- /dev/null +++ b/crates/kaish-kernel/tests/file_test_writable_tests.rs @@ -0,0 +1,304 @@ +//! `-w` must answer "can kaish write this path", and an absent mode is not +//! that answer. +//! +//! `DirEntry.permissions` is `None` on MemoryFs (writable), on DevFs +//! (writable — writes discard), on BuiltinFs (read-only) and on JobFs +//! (read-only). Both defaults are therefore wrong for half the backends: +//! shipped 0.16 opened it (`is_none_or`) and said `/v/bin/echo` and +//! `/v/jobs/1/status` were writable; closing it said `/v/probe.txt` was not, +//! about a path the very next line writes. The bit being defaulted is not +//! the bit that answers the question. +//! +//! The answer needs both facts: the owning mount's read-only state AND the +//! mode bits that mount reports. `PathAccess::resolve` is the only place +//! they combine, and `KernelBackend::path_access` is the only thing the two +//! file-test sites call. +//! +//! Every case runs twice — once through the `test` builtin +//! (`tools/builtin/test.rs::file_test`) and once through `[[ ]]` +//! (`kernel.rs::eval_test_async`) — because a fix landing in one site and +//! not the other is the bug class this repo keeps getting caught by. + +// Test-fixture code: unwrap/expect on known-good setup is the idiom here. +#![allow(clippy::unwrap_used, clippy::expect_used)] +#![cfg(feature = "localfs")] + +mod common; + +use std::sync::Arc; + +use kaish_kernel::vfs::{LocalFs, MemoryFs, VfsRouter}; +use kaish_kernel::{Kernel, KernelBackend, KernelConfig, LocalBackend}; + +use common::{kernel_at, run}; + +/// Assert that both spellings of a file test agree, and on what. +/// +/// `setup` runs first (same kernel), so a case can create its fixture in the +/// VFS. Asserting on both spellings in one helper is the point: it is not +/// possible to add a case here that covers only `test` or only `[[ ]]`. +async fn both_spellings(kernel: &Kernel, setup: &str, op: &str, path: &str, expected: bool) { + if !setup.is_empty() { + let (out, code) = run(kernel, setup).await; + assert_eq!(code, 0, "setup failed: {setup}: out={out:?}"); + } + let want = if expected { 0 } else { 1 }; + + let script = format!("if test {op} {path}; then echo YES; else echo NO; fi"); + let (out, _) = run(kernel, &script).await; + assert_eq!( + out, + if expected { "YES" } else { "NO" }, + "`test {op} {path}` disagrees (want exit {want})", + ); + + let script = format!("if [[ {op} {path} ]]; then echo YES; else echo NO; fi"); + let (out, _) = run(kernel, &script).await; + assert_eq!( + out, + if expected { "YES" } else { "NO" }, + "`[[ {op} {path} ]]` disagrees (want exit {want})", + ); +} + +// ── MemoryFs: writable, reports no mode ──────────────────────────────────── + +/// The regression that killed the close-the-default attempt: `/v` is +/// MemoryFs, MemoryFs reports `permissions: None` everywhere, and MemoryFs +/// is writable. Any implementation that reads an absent mode as "not +/// writable" fails here. +#[tokio::test] +async fn memoryfs_file_is_writable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + both_spellings(&kernel, "echo hi > /v/probe.txt", "-w", "/v/probe.txt", true).await; +} + +/// The proof that the answer above is not a guess: the same path really does +/// take a second write. If this fails, the fixture is wrong, not the fix. +#[tokio::test] +async fn memoryfs_file_really_takes_a_second_write() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + let (_, code) = run(&kernel, "echo one > /v/p2.txt; echo two >> /v/p2.txt").await; + assert_eq!(code, 0, "MemoryFs path must accept an append"); + let (out, _) = run(&kernel, "cat /v/p2.txt").await; + assert_eq!(out, "one\ntwo"); +} + +/// A MemoryFs *directory* is writable too — `mkdir`/`touch` land there. +#[tokio::test] +async fn memoryfs_directory_is_writable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + both_spellings(&kernel, "mkdir -p /v/sub", "-w", "/v/sub", true).await; +} + +// ── BuiltinFs: read-only, reports no mode ────────────────────────────────── + +/// `/v/bin` is BuiltinFs, whose `read_only()` is `true` and whose entries +/// report `permissions: None`. Shipped 0.16 answered "writable" here. +#[tokio::test] +async fn builtinfs_entry_is_not_writable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + both_spellings(&kernel, "", "-w", "/v/bin/echo", false).await; +} + +/// Read-only is about writes. A BuiltinFs entry reads fine, so `-r` must +/// stay true — an implementation that routed the mount's read-only state +/// into `readable` as well fails here. +#[tokio::test] +async fn builtinfs_entry_is_still_readable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + both_spellings(&kernel, "", "-r", "/v/bin/echo", true).await; +} + +/// The mount really does refuse the write the test predicts. +#[tokio::test] +async fn builtinfs_entry_really_refuses_a_write() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + let (_, code) = run(&kernel, "echo nope > /v/bin/echo").await; + assert_ne!(code, 0, "BuiltinFs must refuse a write to /v/bin/echo"); +} + +// ── JobFs: read-only, reports no mode ────────────────────────────────────── + +/// `/v/jobs/{id}/status` is JobFs — read-only, `permissions: None`. Same +/// wrong answer as BuiltinFs in shipped 0.16, from a different mount, so a +/// fix that special-cased `/v/bin` would still fail here. +#[tokio::test] +async fn jobfs_node_is_not_writable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + let (out, code) = run(&kernel, "sleep 30 &").await; + assert_eq!(code, 0, "backgrounding a job failed: {out:?}"); + both_spellings(&kernel, "", "-w", "/v/jobs/1/status", false).await; + // and it is readable, for the same reason BuiltinFs is + both_spellings(&kernel, "", "-r", "/v/jobs/1/status", true).await; +} + +// ── DevFs: writable on purpose, reports no mode ──────────────────────────── + +/// `DevFs::read_only()` is deliberately `false` — refusing the write would +/// break `> /dev/null`. `-w /dev/null` must agree with that, so an +/// implementation that read "no mode bits" as "not writable" fails here. +#[tokio::test] +async fn devfs_null_is_writable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + both_spellings(&kernel, "", "-w", "/dev/null", true).await; + let (_, code) = run(&kernel, "echo discard > /dev/null").await; + assert_eq!(code, 0, "/dev/null must accept the write it says it accepts"); +} + +// ── LocalFs: real OS mode bits ───────────────────────────────────────────── + +/// A writable LocalFs mount still has to honour the mode bits — an +/// implementation that answered from the mount alone and ignored the stat +/// would call a mode-444 file writable. +#[cfg(unix)] +#[tokio::test] +async fn localfs_mode_bits_still_decide() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("rw.txt"), b"hi\n").unwrap(); + std::fs::write(tmp.path().join("ro.txt"), b"hi\n").unwrap(); + std::fs::set_permissions( + tmp.path().join("ro.txt"), + std::fs::Permissions::from_mode(0o444), + ) + .unwrap(); + + let kernel = kernel_at(tmp.path()); + let rw = tmp.path().join("rw.txt"); + let ro = tmp.path().join("ro.txt"); + both_spellings(&kernel, "", "-w", &rw.display().to_string(), true).await; + both_spellings(&kernel, "", "-w", &ro.display().to_string(), false).await; +} + +/// `-x` on a real path keeps answering from the mode bits. Pinned so a later +/// symmetry argument cannot quietly move it. +#[cfg(unix)] +#[tokio::test] +async fn localfs_executable_bit_still_decides() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("run.sh"), b"#!/bin/sh\n").unwrap(); + std::fs::write(tmp.path().join("plain.txt"), b"hi\n").unwrap(); + std::fs::set_permissions( + tmp.path().join("run.sh"), + std::fs::Permissions::from_mode(0o755), + ) + .unwrap(); + + let kernel = kernel_at(tmp.path()); + let run_sh = tmp.path().join("run.sh"); + let plain = tmp.path().join("plain.txt"); + both_spellings(&kernel, "", "-x", &run_sh.display().to_string(), true).await; + both_spellings(&kernel, "", "-x", &plain.display().to_string(), false).await; +} + +/// A memory-backed path has no executable to run — no mode bits, and +/// `real_path` is `None`, so there is nothing for exec(2). `-x` says false, +/// and read-only-ness has nothing to do with it. This pins the deliberate +/// answer to the `-x` question rather than leaving it to inference. +#[tokio::test] +async fn memory_backed_paths_are_not_executable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + both_spellings(&kernel, "echo hi > /v/probe.txt", "-x", "/v/probe.txt", false).await; + // read-only mount, same answer, for the same reason + both_spellings(&kernel, "", "-x", "/v/bin/echo", false).await; + // writable mount, same answer + both_spellings(&kernel, "", "-x", "/dev/null", false).await; +} + +// ── The hazard: read-only wrapper over an OS-writable directory ──────────── + +/// Build a kernel whose `/` is a `LocalFs::read_only` wrapper over a real, +/// OS-writable tempdir — the shape kaijutsu embeds. +fn read_only_wrapper_kernel(dir: &std::path::Path) -> Kernel { + let mut vfs = VfsRouter::new(); + vfs.mount("/", LocalFs::read_only(dir)); + vfs.mount("/v", MemoryFs::new()); + let backend: Arc = Arc::new(LocalBackend::new(Arc::new(vfs))); + Kernel::with_backend(backend, KernelConfig::isolated(), |_| {}, |_| {}) + .expect("with_backend kernel") +} + +/// The hazard from the design note: `LocalFs::stat` reports the real OS mode +/// bits and knows nothing about the read-only wrapper around it, so a raw +/// stat check calls a mode-644 file writable while every write to it fails. +/// An implementation that consults only the stat fails here; one that +/// consults only the mount passes here and fails `localfs_mode_bits_still_decide`. +#[cfg(unix)] +#[tokio::test] +async fn read_only_wrapper_over_writable_os_dir_is_not_writable() { + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("file.txt"), b"hi\n").unwrap(); + + // The OS says 0o644. The wrapper says no. + let mode = { + use std::os::unix::fs::PermissionsExt; + std::fs::metadata(tmp.path().join("file.txt")) + .unwrap() + .permissions() + .mode() + }; + assert_ne!(mode & 0o222, 0, "fixture must be OS-writable to be the hazard"); + + let kernel = read_only_wrapper_kernel(tmp.path()); + both_spellings(&kernel, "", "-w", "/file.txt", false).await; +} + +/// The wrapper really does refuse the write — the assertion above is about +/// the same file this one fails to write. +#[tokio::test] +async fn read_only_wrapper_really_refuses_a_write() { + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("file.txt"), b"hi\n").unwrap(); + let kernel = read_only_wrapper_kernel(tmp.path()); + let (_, code) = run(&kernel, "echo nope > /file.txt").await; + assert_ne!(code, 0, "a read-only LocalFs must refuse the write"); +} + +/// The read-only wrapper is still readable, and its `-x` still comes from +/// the OS mode bits — the wrapper is about writes only. +#[cfg(unix)] +#[tokio::test] +async fn read_only_wrapper_keeps_read_and_execute_answers() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("run.sh"), b"#!/bin/sh\n").unwrap(); + std::fs::set_permissions( + tmp.path().join("run.sh"), + std::fs::Permissions::from_mode(0o555), + ) + .unwrap(); + + let kernel = read_only_wrapper_kernel(tmp.path()); + both_spellings(&kernel, "", "-r", "/run.sh", true).await; + both_spellings(&kernel, "", "-x", "/run.sh", true).await; + both_spellings(&kernel, "", "-w", "/run.sh", false).await; +} + +// ── Absent paths ─────────────────────────────────────────────────────────── + +/// A path that does not exist is not readable, writable, or executable — +/// `path_access` reports `stat`'s error and the file test reads that as +/// false, on a writable mount and a read-only one alike. +#[tokio::test] +async fn missing_paths_answer_false_everywhere() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + for op in ["-r", "-w", "-x"] { + both_spellings(&kernel, "", op, "/v/nope.txt", false).await; + both_spellings(&kernel, "", op, "/v/bin/definitely-not-a-builtin", false).await; + } +} From 48d978df87b02dd64bb47b7197f8d876383153ad Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:03:28 -0400 Subject: [PATCH 02/17] =?UTF-8?q?types:=20PathAccess=20=E2=80=94=20the=20o?= =?UTF-8?q?ne=20place=20the=20two=20facts=20combine?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/kaish-types/src/lib.rs | 2 + crates/kaish-types/src/path_access.rs | 144 ++++++++++++++++++++++++++ 2 files changed, 146 insertions(+) create mode 100644 crates/kaish-types/src/path_access.rs diff --git a/crates/kaish-types/src/lib.rs b/crates/kaish-types/src/lib.rs index b5f935dd..5d0cd719 100644 --- a/crates/kaish-types/src/lib.rs +++ b/crates/kaish-types/src/lib.rs @@ -12,6 +12,7 @@ pub mod dir_entry; pub mod job; pub mod kernel; pub mod output; +pub mod path_access; // `plan` is deliberately NOT in the flat re-export block below: its names // are generic (Plan, PlannedCommand, PlannedValue) and would collide at the // crate root. Consumers write `kaish_types::plan::Plan`, like `clock`. @@ -29,6 +30,7 @@ pub use dir_entry::*; pub use job::*; pub use kernel::*; pub use output::*; +pub use path_access::*; pub use result::*; pub use tool::*; pub use value::*; diff --git a/crates/kaish-types/src/path_access.rs b/crates/kaish-types/src/path_access.rs new file mode 100644 index 00000000..bc160025 --- /dev/null +++ b/crates/kaish-types/src/path_access.rs @@ -0,0 +1,144 @@ +//! What the kernel can do with one path. + +/// Whether the kernel can read, write, or execute a path. +/// +/// A file test needs two facts that neither one answers alone: the read-only +/// state of the mount that owns the path, and the mode bits that mount +/// reports for the path itself. +/// +/// Neither fact is sufficient. `DirEntry.permissions` is `None` on MemoryFs +/// (writable), on DevFs (writable — writes discard), on BuiltinFs +/// (read-only) and on JobFs (read-only), so an absent mode carries no +/// information about writability at all. In the other direction a +/// `LocalFs::read_only` wrapper over an OS-writable directory reports real +/// mode bits with the write bit set, because `LocalFs::stat` asks the OS and +/// the OS does not know about the wrapper. +/// +/// [`PathAccess::resolve`] is the only way to build a `PathAccess`, and it +/// takes both facts, so a caller cannot answer from one of them by accident. +/// The struct is `#[non_exhaustive]`: read the fields, do not construct it +/// by literal. +#[non_exhaustive] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct PathAccess { + /// The path's contents can be read. A read-only mount is readable. + pub readable: bool, + /// The path can be written. False whenever the owning mount is read-only, + /// whatever the mode bits say. + pub writable: bool, + /// The path can be executed. False when the mount reports no mode bits — + /// a mount with nothing to hand exec(2) has no executable path on it. + pub executable: bool, +} + +impl PathAccess { + /// Combine a mount's read-only state with the mode bits it reports for + /// one path. + /// + /// `mode` is `DirEntry.permissions` — `None` when the mount does not + /// model Unix modes. `mount_read_only` is that mount's + /// `Filesystem::read_only()`, for the mount that actually owns the path, + /// not the whole router. + /// + /// The three answers do not treat an absent mode the same way, because + /// the three questions are not the same question: + /// + /// - `readable` — an absent mode means the mount does not restrict reads, + /// so it reads. Read-only mounts read. + /// - `writable` — an absent mode says nothing, so the mount decides. Both + /// must agree: a read-only mount is never writable, and a writable + /// mount still honours a mode that clears `0o222`. + /// - `executable` — an absent mode means there is no executable here. + /// Read-only-ness is about writes and contributes nothing; a mount that + /// reports no modes (MemoryFs, JobFs, BuiltinFs, DevFs) also has no real + /// path for exec(2) to open. + pub fn resolve(mode: Option, mount_read_only: bool) -> Self { + Self { + readable: mode.is_none_or(|p| p & 0o444 != 0), + writable: !mount_read_only && mode.is_none_or(|p| p & 0o222 != 0), + executable: mode.is_some_and(|p| p & 0o111 != 0), + } + } + + /// Re-answer `writable` from a different layer than the one that answered + /// `readable` and `executable`. + /// + /// Copy-on-write overlays need this: reads resolve against whichever + /// layer holds the path, but every write lands in the upper layer, so the + /// upper layer decides writability. A lower file whose mode clears `0o222` + /// is still writable through copy-up, because `OverlayFs::write` copies + /// the content up and writes the upper — it never consults the lower's + /// mode. + /// + /// Takes the same pair as [`PathAccess::resolve`], for the write layer. + pub fn with_write_layer(self, mode: Option, mount_read_only: bool) -> Self { + Self { + writable: Self::resolve(mode, mount_read_only).writable, + ..self + } + } +} + +#[cfg(test)] +mod tests { + use super::PathAccess; + + /// MemoryFs and DevFs: no modes, writable mount. + #[test] + fn absent_mode_on_a_writable_mount_is_writable_not_executable() { + let access = PathAccess::resolve(None, false); + assert!(access.readable); + assert!(access.writable); + assert!(!access.executable); + } + + /// BuiltinFs and JobFs: no modes, read-only mount. The shipped 0.16 bug + /// answered `writable` here. + #[test] + fn absent_mode_on_a_read_only_mount_is_readable_only() { + let access = PathAccess::resolve(None, true); + assert!(access.readable); + assert!(!access.writable); + assert!(!access.executable); + } + + /// A writable mount still honours the mode bits it reports. + #[test] + fn mode_bits_decide_on_a_writable_mount() { + assert!(PathAccess::resolve(Some(0o644), false).writable); + assert!(!PathAccess::resolve(Some(0o444), false).writable); + assert!(PathAccess::resolve(Some(0o755), false).executable); + assert!(!PathAccess::resolve(Some(0o644), false).executable); + assert!(!PathAccess::resolve(Some(0o000), false).readable); + } + + /// The hazard: a read-only wrapper over an OS-writable file. The mode + /// says yes and the mount says no; both must agree for a yes. + #[test] + fn read_only_mount_overrides_a_writable_mode() { + let access = PathAccess::resolve(Some(0o755), true); + assert!(!access.writable, "the mount's read-only state must win"); + assert!(access.readable, "read-only is about writes"); + assert!(access.executable, "read-only says nothing about exec"); + } + + /// Copy-up: the lower's mode answers read and exec, the upper answers + /// write. + #[test] + fn write_layer_replaces_only_the_write_answer() { + let lower = PathAccess::resolve(Some(0o444), false); + assert!(!lower.writable); + let overlaid = lower.with_write_layer(None, false); + assert!(overlaid.writable, "copy-up makes a mode-444 lower writable"); + assert!(overlaid.readable); + assert_eq!(overlaid.executable, lower.executable); + } + + /// A read-only upper makes the whole overlay unwritable, whatever the + /// lower reports. + #[test] + fn a_read_only_write_layer_wins() { + let overlaid = PathAccess::resolve(Some(0o755), false).with_write_layer(Some(0o755), true); + assert!(!overlaid.writable); + } +} From 265b2797ba8ee676c7dfe38b484334c48eb972d6 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:10:07 -0400 Subject: [PATCH 03/17] test: reframe for filling the modes in at the source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../tests/file_test_writable_tests.rs | 114 +++++++++++++----- 1 file changed, 83 insertions(+), 31 deletions(-) diff --git a/crates/kaish-kernel/tests/file_test_writable_tests.rs b/crates/kaish-kernel/tests/file_test_writable_tests.rs index a952fbbb..4ae23653 100644 --- a/crates/kaish-kernel/tests/file_test_writable_tests.rs +++ b/crates/kaish-kernel/tests/file_test_writable_tests.rs @@ -1,18 +1,23 @@ -//! `-w` must answer "can kaish write this path", and an absent mode is not -//! that answer. +//! `-w` must answer "can kaish write this path", and it gets that answer from +//! the mount's read-only state AND the mode bits the mount reports. //! -//! `DirEntry.permissions` is `None` on MemoryFs (writable), on DevFs -//! (writable — writes discard), on BuiltinFs (read-only) and on JobFs -//! (read-only). Both defaults are therefore wrong for half the backends: -//! shipped 0.16 opened it (`is_none_or`) and said `/v/bin/echo` and -//! `/v/jobs/1/status` were writable; closing it said `/v/probe.txt` was not, -//! about a path the very next line writes. The bit being defaulted is not -//! the bit that answers the question. +//! Before this change `DirEntry.permissions` was `None` on MemoryFs +//! (writable), DevFs (writable — writes discard), BuiltinFs (read-only) and +//! JobFs (read-only), so an absent mode carried no information and neither +//! default was right: opening it (shipped 0.16) called `/v/bin/echo` and +//! `/v/jobs/1/status` writable; closing it called `/v/probe.txt` unwritable, +//! about a path the very next line writes. //! -//! The answer needs both facts: the owning mount's read-only state AND the -//! mode bits that mount reports. `PathAccess::resolve` is the only place -//! they combine, and `KernelBackend::path_access` is the only thing the two -//! file-test sites call. +//! The fix fills the signal in at the source. MemoryFs and DevFs now report +//! real modes, so the only backends left reporting `None` are read-only ones +//! and "absent" now means "this backend does not model permissions" — +//! readable, not writable, not executable. `permissions` is a read-only +//! observation: there is no `chmod` builtin in this tree. +//! +//! A mode alone is still not enough. `LocalFs::stat` asks the OS, and the OS +//! does not know about a `LocalFs::read_only` wrapper, so a mode-644 file on +//! a read-only mount reports the write bit set while every write to it fails. +//! `PathAccess::resolve` combines the two facts and is the only constructor. //! //! Every case runs twice — once through the `test` builtin //! (`tools/builtin/test.rs::file_test`) and once through `[[ ]]` @@ -64,9 +69,10 @@ async fn both_spellings(kernel: &Kernel, setup: &str, op: &str, path: &str, expe // ── MemoryFs: writable, reports no mode ──────────────────────────────────── /// The regression that killed the close-the-default attempt: `/v` is -/// MemoryFs, MemoryFs reports `permissions: None` everywhere, and MemoryFs -/// is writable. Any implementation that reads an absent mode as "not -/// writable" fails here. +/// MemoryFs and MemoryFs is writable. Closing the absent-mode default is +/// only safe because MemoryFs now reports a real file mode (`0o666`) — an +/// implementation that closed the default without filling the mode in fails +/// here. #[tokio::test] async fn memoryfs_file_is_writable() { let tmp = tempfile::tempdir().unwrap(); @@ -86,18 +92,32 @@ async fn memoryfs_file_really_takes_a_second_write() { assert_eq!(out, "one\ntwo"); } -/// A MemoryFs *directory* is writable too — `mkdir`/`touch` land there. +/// A MemoryFs *directory* is writable too — `mkdir`/`touch` land there — +/// and it is searchable, which is what the `x` bit means for a directory. +/// `-x` on a MemoryFs directory answered NO before this change; `0o777` makes +/// it YES, which is the POSIX answer. This is the user-visible behavior +/// change in this commit. #[tokio::test] -async fn memoryfs_directory_is_writable() { +async fn memoryfs_directory_is_writable_and_searchable() { let tmp = tempfile::tempdir().unwrap(); let kernel = kernel_at(tmp.path()); both_spellings(&kernel, "mkdir -p /v/sub", "-w", "/v/sub", true).await; + both_spellings(&kernel, "", "-x", "/v/sub", true).await; + both_spellings(&kernel, "", "-r", "/v/sub", true).await; + // and the mount root itself + both_spellings(&kernel, "", "-x", "/v", true).await; + both_spellings(&kernel, "", "-w", "/v", true).await; + // the directory really does accept the entry `-w` promises + let (_, code) = run(&kernel, "mkdir -p /v/sub/deeper").await; + assert_eq!(code, 0, "a MemoryFs directory must accept mkdir"); } // ── BuiltinFs: read-only, reports no mode ────────────────────────────────── /// `/v/bin` is BuiltinFs, whose `read_only()` is `true` and whose entries -/// report `permissions: None`. Shipped 0.16 answered "writable" here. +/// report no mode. Shipped 0.16 answered "writable" here. This is one of the +/// two backends that still report `None` after the source fix, and both are +/// read-only — which is what makes the closed default correct. #[tokio::test] async fn builtinfs_entry_is_not_writable() { let tmp = tempfile::tempdir().unwrap(); @@ -126,9 +146,10 @@ async fn builtinfs_entry_really_refuses_a_write() { // ── JobFs: read-only, reports no mode ────────────────────────────────────── -/// `/v/jobs/{id}/status` is JobFs — read-only, `permissions: None`. Same -/// wrong answer as BuiltinFs in shipped 0.16, from a different mount, so a -/// fix that special-cased `/v/bin` would still fail here. +/// `/v/jobs/{id}/status` is JobFs — read-only, and the other backend that +/// reports no mode. Same wrong answer as BuiltinFs in shipped 0.16, from a +/// different mount, so a fix that special-cased `/v/bin` would still fail +/// here. #[tokio::test] async fn jobfs_node_is_not_writable() { let tmp = tempfile::tempdir().unwrap(); @@ -143,8 +164,9 @@ async fn jobfs_node_is_not_writable() { // ── DevFs: writable on purpose, reports no mode ──────────────────────────── /// `DevFs::read_only()` is deliberately `false` — refusing the write would -/// break `> /dev/null`. `-w /dev/null` must agree with that, so an -/// implementation that read "no mode bits" as "not writable" fails here. +/// break `> /dev/null`. So the mount cannot carry the answer here and the +/// mode has to: the device files report `0o666`, matching crw-rw-rw- on +/// Linux. #[tokio::test] async fn devfs_null_is_writable() { let tmp = tempfile::tempdir().unwrap(); @@ -154,6 +176,32 @@ async fn devfs_null_is_writable() { assert_eq!(code, 0, "/dev/null must accept the write it says it accepts"); } +/// A character device is not executable. +#[tokio::test] +async fn devfs_null_is_not_executable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + both_spellings(&kernel, "", "-x", "/dev/null", false).await; + both_spellings(&kernel, "", "-r", "/dev/null", true).await; +} + +/// The `/dev` *directory* is searchable but NOT writable, and that is a +/// deliberate one-bit divergence from Linux's 0755: kaish's `DevFs::mkdir` +/// and `DevFs::remove` refuse unconditionally, for every caller, because +/// kaish has no root user to be the exception 0755 carves out. `0o555` is +/// the mode that tells the truth about this mount. The write below is the +/// receipt — `-w` and `mkdir` have to agree. +#[tokio::test] +async fn devfs_directory_is_searchable_but_not_writable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + both_spellings(&kernel, "", "-x", "/dev", true).await; + both_spellings(&kernel, "", "-r", "/dev", true).await; + both_spellings(&kernel, "", "-w", "/dev", false).await; + let (_, code) = run(&kernel, "mkdir /dev/newthing").await; + assert_ne!(code, 0, "/dev must refuse mkdir, as `-w /dev` said it would"); +} + // ── LocalFs: real OS mode bits ───────────────────────────────────────────── /// A writable LocalFs mount still has to honour the mode bits — an @@ -203,18 +251,22 @@ async fn localfs_executable_bit_still_decides() { both_spellings(&kernel, "", "-x", &plain.display().to_string(), false).await; } -/// A memory-backed path has no executable to run — no mode bits, and -/// `real_path` is `None`, so there is nothing for exec(2). `-x` says false, -/// and read-only-ness has nothing to do with it. This pins the deliberate -/// answer to the `-x` question rather than leaving it to inference. +/// No memory-backed *file* is executable. `real_path` is `None` for these +/// mounts, so there is nothing for exec(2) to open, and the modes say so: +/// MemoryFs files are `0o666` and DevFs devices are `0o666`. This is the +/// deliberate answer to "does `-x` mean anything on a memory-backed path" — +/// for files, no, and the mode is where that is written down. +/// +/// Directories are the exception, and not an inconsistency: `x` on a +/// directory means searchable, not executable, and these directories are +/// searchable. `memoryfs_directory_is_writable_and_searchable` pins that. #[tokio::test] -async fn memory_backed_paths_are_not_executable() { +async fn memory_backed_files_are_not_executable() { let tmp = tempfile::tempdir().unwrap(); let kernel = kernel_at(tmp.path()); both_spellings(&kernel, "echo hi > /v/probe.txt", "-x", "/v/probe.txt", false).await; - // read-only mount, same answer, for the same reason + // a backend that models no permissions at all: not executable either both_spellings(&kernel, "", "-x", "/v/bin/echo", false).await; - // writable mount, same answer both_spellings(&kernel, "", "-x", "/dev/null", false).await; } From 4aa4789ec169170b94315817f5a01d752b238edf Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:10:40 -0400 Subject: [PATCH 04/17] vfs: MemoryFs reports real modes instead of None MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/kaish-vfs/src/memory.rs | 52 ++++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 9 deletions(-) diff --git a/crates/kaish-vfs/src/memory.rs b/crates/kaish-vfs/src/memory.rs index 8fa4dd08..de929625 100644 --- a/crates/kaish-vfs/src/memory.rs +++ b/crates/kaish-vfs/src/memory.rs @@ -114,6 +114,40 @@ impl MemoryFs { } /// Maximum symlink follow depth (matches Linux ELOOP limit). + /// Mode reported for a `MemoryFs` directory: readable, writable, and + /// searchable by everyone. + /// + /// `MemoryFs` has no permission model — it never refuses an operation + /// over a mode — so these constants describe what it actually does + /// rather than restricting anything. Reporting them (instead of `None`) + /// is what lets `test -w` close its absent-mode default: with every + /// writable backend reporting a mode, an absent mode means "this backend + /// does not model permissions", and every backend still in that position + /// is read-only. There is no `chmod` builtin; these are observations. + pub const DIRECTORY_MODE: u32 = 0o777; + + /// Mode reported for a `MemoryFs` file: readable and writable, never + /// executable. `real_path` is `None` for a memory-backed path, so there + /// is nothing for exec(2) to open and the `x` bit would be a lie. + pub const FILE_MODE: u32 = 0o666; + + /// Mode reported for a `MemoryFs` symlink, matching `lrwxrwxrwx` on + /// Linux. Only `lstat` and `list` ever show it — `stat` follows the link + /// and reports the target's mode. + pub const SYMLINK_MODE: u32 = 0o777; + + /// The mode this filesystem reports for an entry of `kind`. + fn mode_for(kind: DirEntryKind) -> Option { + Some(match kind { + DirEntryKind::Directory => Self::DIRECTORY_MODE, + DirEntryKind::File => Self::FILE_MODE, + DirEntryKind::Symlink => Self::SYMLINK_MODE, + // A kind this crate does not know about gets the conservative + // answer rather than a guess: not writable, not executable. + _ => 0o444, + }) + } + const MAX_SYMLINK_DEPTH: usize = 40; /// Read a file, following symlinks with depth limit. @@ -210,7 +244,7 @@ impl MemoryFs { kind: DirEntryKind::Directory, size: 0, modified: Some(system_now()), - permissions: None, + permissions: Some(Self::DIRECTORY_MODE), symlink_target: None, }); } @@ -224,7 +258,7 @@ impl MemoryFs { kind: DirEntryKind::File, size: data.len() as u64, modified: Some(*modified), - permissions: None, + permissions: Some(Self::FILE_MODE), symlink_target: None, }, None, @@ -235,7 +269,7 @@ impl MemoryFs { kind: DirEntryKind::Directory, size: 0, modified: Some(*modified), - permissions: None, + permissions: Some(Self::DIRECTORY_MODE), symlink_target: None, }, None, @@ -246,7 +280,7 @@ impl MemoryFs { kind: DirEntryKind::File, // placeholder, will be overridden size: 0, modified: None, - permissions: None, + permissions: Some(Self::FILE_MODE), symlink_target: None, }, Some(target.clone()), @@ -490,7 +524,7 @@ impl Filesystem for MemoryFs { kind, size, modified, - permissions: None, + permissions: Self::mode_for(kind), symlink_target, }); } @@ -529,7 +563,7 @@ impl Filesystem for MemoryFs { kind: DirEntryKind::Directory, size: 0, modified: Some(system_now()), - permissions: None, + permissions: Some(Self::DIRECTORY_MODE), symlink_target: None, }); } @@ -540,7 +574,7 @@ impl Filesystem for MemoryFs { kind: DirEntryKind::File, size: data.len() as u64, modified: Some(*modified), - permissions: None, + permissions: Some(Self::FILE_MODE), symlink_target: None, }), Some(Entry::Directory { modified }) => Ok(DirEntry { @@ -548,7 +582,7 @@ impl Filesystem for MemoryFs { kind: DirEntryKind::Directory, size: 0, modified: Some(*modified), - permissions: None, + permissions: Some(Self::DIRECTORY_MODE), symlink_target: None, }), Some(Entry::Symlink { target, modified }) => Ok(DirEntry { @@ -556,7 +590,7 @@ impl Filesystem for MemoryFs { kind: DirEntryKind::Symlink, size: 0, modified: Some(*modified), - permissions: None, + permissions: Some(Self::SYMLINK_MODE), symlink_target: Some(target.clone()), }), None => Err(io::Error::new( From cb55304d5297cc36c3720b4d58c51dfb56681823 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:11:07 -0400 Subject: [PATCH 05/17] vfs: DevFs reports real modes; /dev is 0555, not 0755 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/kaish-vfs/src/dev.rs | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/crates/kaish-vfs/src/dev.rs b/crates/kaish-vfs/src/dev.rs index 5c923eca..eb5c1701 100644 --- a/crates/kaish-vfs/src/dev.rs +++ b/crates/kaish-vfs/src/dev.rs @@ -98,13 +98,28 @@ impl DevFs { ) } + /// Mode reported for a device node, matching `crw-rw-rw-` on Linux: + /// everyone reads, everyone writes, nobody executes. `write` accepts and + /// discards for every device, so the write bit is the truth. + pub const DEVICE_MODE: u32 = 0o666; + + /// Mode reported for the `/dev` directory itself: searchable and + /// readable, **not** writable. + /// + /// Linux ships `/dev` as 0755, but that write bit is for root, and kaish + /// has no root: `mkdir` and `remove` below refuse every caller + /// unconditionally. 0755 would make `test -w /dev` answer YES about a + /// directory that accepts nothing, which is the exact failure this mode + /// exists to prevent. 0555 is the mode that tells the truth here. + pub const DIRECTORY_MODE: u32 = 0o555; + fn entry(name: &str) -> DirEntry { DirEntry { name: name.to_string(), kind: DirEntryKind::File, size: 0, modified: None, - permissions: None, + permissions: Some(Self::DEVICE_MODE), symlink_target: None, } } @@ -191,7 +206,7 @@ impl Filesystem for DevFs { kind: DirEntryKind::Directory, size: 0, modified: None, - permissions: None, + permissions: Some(Self::DIRECTORY_MODE), symlink_target: None, }); } @@ -220,7 +235,9 @@ impl Filesystem for DevFs { fn read_only(&self) -> bool { // Writes to the devices "succeed" (they discard), so the mount is not // read-only in the sense the router cares about — refusing a write - // would break `> /dev/null`. + // would break `> /dev/null`. The consequence is that the mount cannot + // answer `test -w` here and the modes have to: DEVICE_MODE is + // writable, DIRECTORY_MODE is not. false } } From 7fdca7770d3ea2baed67e5df8e127fa6b6a09e61 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:11:54 -0400 Subject: [PATCH 06/17] vfs: LocalFs reports a mode on non-Unix too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/kaish-vfs/src/local.rs | 54 +++++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/crates/kaish-vfs/src/local.rs b/crates/kaish-vfs/src/local.rs index 2d179a72..d022f4ef 100644 --- a/crates/kaish-vfs/src/local.rs +++ b/crates/kaish-vfs/src/local.rs @@ -195,9 +195,38 @@ impl LocalFs { Some(meta.permissions().mode()) } + /// Synthesize a Unix-shaped mode from the one permission fact a non-Unix + /// platform exposes. + /// + /// `LocalFs` is writable, so returning `None` here would put it in the + /// same position `MemoryFs` was in: a writable backend reporting an + /// absent mode, which is what makes `test -w` unanswerable. There is + /// exactly one bit to work from — `Permissions::readonly()` — so that is + /// what the mode carries. + /// + /// The `x` bit is never set. Executability is not a permission on these + /// platforms (it is decided by the file extension), so claiming it would + /// be a fabrication; `-x` answered false here before this and still does. #[cfg(not(unix))] - fn extract_permissions(_meta: &std::fs::Metadata) -> Option { - None + fn extract_permissions(meta: &std::fs::Metadata) -> Option { + Some(Self::synthesized_mode(meta.is_dir(), meta.permissions().readonly())) + } + + /// The mode [`extract_permissions`](Self::extract_permissions) reports on + /// a platform with no Unix mode bits. Split out from the `cfg` so it can + /// be tested on every platform, including the Unix ones that never call + /// it. + // Only the non-Unix `extract_permissions` calls this; the tests call it + // on every platform, which is the reason it is split out at all. + #[cfg_attr(unix, allow(dead_code))] + pub(crate) fn synthesized_mode(is_dir: bool, readonly: bool) -> u32 { + match (is_dir, readonly) { + // Searchable, and writable unless the read-only attribute is set. + (true, false) => 0o777, + (true, true) => 0o555, + (false, false) => 0o666, + (false, true) => 0o444, + } } /// Build a [`DirEntry`] for one directory member named by `path`, *without* @@ -699,6 +728,27 @@ mod tests { cleanup(&dir).await; } + // The non-Unix mode synthesis, exercised on every platform. Without it + // `LocalFs` would be a writable backend reporting an absent mode on + // Windows — the same hole MemoryFs had — and `test -w` would answer NO + // about every file there. + #[test] + fn synthesized_mode_keeps_writability_and_never_claims_exec() { + assert_eq!(LocalFs::synthesized_mode(false, false) & 0o222, 0o222); + assert_eq!(LocalFs::synthesized_mode(false, true) & 0o222, 0); + assert_eq!(LocalFs::synthesized_mode(true, false) & 0o222, 0o222); + assert_eq!(LocalFs::synthesized_mode(true, true) & 0o222, 0); + // Everything readable, whatever the read-only attribute says. + for (is_dir, readonly) in [(false, false), (false, true), (true, false), (true, true)] { + assert_ne!(LocalFs::synthesized_mode(is_dir, readonly) & 0o444, 0); + } + // Only directories get the x bit, and it means searchable. + assert_eq!(LocalFs::synthesized_mode(false, false) & 0o111, 0); + assert_eq!(LocalFs::synthesized_mode(false, true) & 0o111, 0); + assert_ne!(LocalFs::synthesized_mode(true, false) & 0o111, 0); + assert_ne!(LocalFs::synthesized_mode(true, true) & 0o111, 0); + } + #[tokio::test] async fn test_read_only() { let (_, dir) = setup().await; From e10ec590202ab87c94a3fa48db0ba925c9340817 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:13:00 -0400 Subject: [PATCH 07/17] types: close the -w absent-mode default now the source is filled in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/kaish-types/src/path_access.rs | 120 ++++++++++++++++---------- 1 file changed, 75 insertions(+), 45 deletions(-) diff --git a/crates/kaish-types/src/path_access.rs b/crates/kaish-types/src/path_access.rs index bc160025..765ef1c1 100644 --- a/crates/kaish-types/src/path_access.rs +++ b/crates/kaish-types/src/path_access.rs @@ -2,32 +2,51 @@ /// Whether the kernel can read, write, or execute a path. /// -/// A file test needs two facts that neither one answers alone: the read-only +/// A file test needs two facts, and neither one answers alone: the read-only /// state of the mount that owns the path, and the mode bits that mount /// reports for the path itself. /// -/// Neither fact is sufficient. `DirEntry.permissions` is `None` on MemoryFs -/// (writable), on DevFs (writable — writes discard), on BuiltinFs -/// (read-only) and on JobFs (read-only), so an absent mode carries no -/// information about writability at all. In the other direction a -/// `LocalFs::read_only` wrapper over an OS-writable directory reports real -/// mode bits with the write bit set, because `LocalFs::stat` asks the OS and -/// the OS does not know about the wrapper. +/// The mode is not enough. A `LocalFs::read_only` wrapper over an +/// OS-writable directory reports real mode bits with the write bit set, +/// because `LocalFs::stat` asks the OS and the OS does not know about the +/// wrapper. Every write to such a path fails; a mode-only check says it +/// would succeed. /// -/// [`PathAccess::resolve`] is the only way to build a `PathAccess`, and it -/// takes both facts, so a caller cannot answer from one of them by accident. -/// The struct is `#[non_exhaustive]`: read the fields, do not construct it -/// by literal. +/// The mount is not enough either. `DevFs::read_only()` is deliberately +/// `false` — refusing writes would break `> /dev/null` — while +/// `mkdir /dev/x` is refused for every caller. Only the mode separates the +/// writable device from the unwritable directory above it. +/// +/// [`PathAccess::resolve`] takes both and is the only constructor, so no +/// caller can answer from one of them by accident. +/// [`PathAccess::with_write_layer`] takes both again, for a copy-on-write +/// overlay whose writes land somewhere other than where its reads resolve. +/// The struct is +/// `#[non_exhaustive]`: read the fields, do not construct it by literal. +/// +/// # What an absent mode means +/// +/// `DirEntry.permissions` is `None` only for a backend that does not model +/// permissions at all. Every backend in this workspace that can be written +/// reports a mode — `LocalFs` on every platform, `MemoryFs`, `DevFs`, and +/// `OverlayFs` through whichever layer holds the path — so the backends +/// still reporting `None` (`BuiltinFs`, `JobFs`) are read-only ones. An +/// absent mode therefore reads as: readable, not writable, not executable. +/// +/// That premise is load-bearing. **A backend that is writable and reports +/// `None` will be told its paths are unwritable.** An embedder adding one +/// should report a mode rather than rely on a default here. #[non_exhaustive] #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct PathAccess { /// The path's contents can be read. A read-only mount is readable. pub readable: bool, - /// The path can be written. False whenever the owning mount is read-only, - /// whatever the mode bits say. + /// The path can be written. Needs both facts to agree: false whenever the + /// owning mount is read-only, whatever the mode says, and false whenever + /// the mode clears `0o222`, whatever the mount says. pub writable: bool, - /// The path can be executed. False when the mount reports no mode bits — - /// a mount with nothing to hand exec(2) has no executable path on it. + /// The path can be executed, or — on a directory — searched. False when + /// the mount reports no mode. pub executable: bool, } @@ -36,26 +55,27 @@ impl PathAccess { /// one path. /// /// `mode` is `DirEntry.permissions` — `None` when the mount does not - /// model Unix modes. `mount_read_only` is that mount's - /// `Filesystem::read_only()`, for the mount that actually owns the path, - /// not the whole router. + /// model Unix modes. `mount_read_only` is the `read_only()` of the mount + /// that actually owns the path, not of the whole router. /// - /// The three answers do not treat an absent mode the same way, because - /// the three questions are not the same question: + /// The three answers treat an absent mode differently, because the three + /// questions are different: /// - /// - `readable` — an absent mode means the mount does not restrict reads, - /// so it reads. Read-only mounts read. - /// - `writable` — an absent mode says nothing, so the mount decides. Both - /// must agree: a read-only mount is never writable, and a writable - /// mount still honours a mode that clears `0o222`. - /// - `executable` — an absent mode means there is no executable here. - /// Read-only-ness is about writes and contributes nothing; a mount that - /// reports no modes (MemoryFs, JobFs, BuiltinFs, DevFs) also has no real - /// path for exec(2) to open. + /// - `readable` — a backend that does not model permissions does not + /// restrict reads, and read-only mounts read. So: readable. + /// - `writable` — every writable backend reports a mode, so an absent one + /// means read-only. Both facts must agree for a yes: a read-only mount + /// is never writable, and a writable mount still honors a mode that + /// clears `0o222`. + /// - `executable` — an absent mode means there is nothing here to run. + /// Read-only-ness contributes nothing; it is about writes. + /// + /// On a directory, `0o111` means searchable, which is the POSIX meaning + /// and what `test -x DIR` should answer. pub fn resolve(mode: Option, mount_read_only: bool) -> Self { Self { readable: mode.is_none_or(|p| p & 0o444 != 0), - writable: !mount_read_only && mode.is_none_or(|p| p & 0o222 != 0), + writable: !mount_read_only && mode.is_some_and(|p| p & 0o222 != 0), executable: mode.is_some_and(|p| p & 0o111 != 0), } } @@ -83,23 +103,32 @@ impl PathAccess { mod tests { use super::PathAccess; - /// MemoryFs and DevFs: no modes, writable mount. + /// A backend that models no permissions reads, and nothing else — the + /// mount being writable does not make up for an absent mode, because + /// every writable backend in the workspace reports one. #[test] - fn absent_mode_on_a_writable_mount_is_writable_not_executable() { - let access = PathAccess::resolve(None, false); - assert!(access.readable); - assert!(access.writable); - assert!(!access.executable); + fn absent_mode_reads_and_nothing_else() { + for mount_read_only in [false, true] { + let access = PathAccess::resolve(None, mount_read_only); + assert!(access.readable, "read-only is about writes"); + assert!(!access.writable, "absent mode means read-only backend"); + assert!(!access.executable, "nothing here to hand exec(2)"); + } } - /// BuiltinFs and JobFs: no modes, read-only mount. The shipped 0.16 bug - /// answered `writable` here. + /// A directory mode of `0o777` (MemoryFs) is writable and searchable; + /// `0o555` (the `/dev` directory) is searchable and not writable. The + /// pair is the whole reason DevFs needed a mode of its own. #[test] - fn absent_mode_on_a_read_only_mount_is_readable_only() { - let access = PathAccess::resolve(None, true); - assert!(access.readable); - assert!(!access.writable); - assert!(!access.executable); + fn directory_modes_separate_searchable_from_writable() { + let memory_dir = PathAccess::resolve(Some(0o777), false); + assert!(memory_dir.writable); + assert!(memory_dir.executable, "0o111 on a directory is searchable"); + + let dev_dir = PathAccess::resolve(Some(0o555), false); + assert!(!dev_dir.writable, "/dev accepts no mkdir"); + assert!(dev_dir.executable); + assert!(dev_dir.readable); } /// A writable mount still honours the mode bits it reports. @@ -128,7 +157,8 @@ mod tests { fn write_layer_replaces_only_the_write_answer() { let lower = PathAccess::resolve(Some(0o444), false); assert!(!lower.writable); - let overlaid = lower.with_write_layer(None, false); + // The upper is MemoryFs, so it reports 0o666 for the copied-up file. + let overlaid = lower.with_write_layer(Some(0o666), false); assert!(overlaid.writable, "copy-up makes a mode-444 lower writable"); assert!(overlaid.readable); assert_eq!(overlaid.executable, lower.executable); From 80c1559f6abc27c666e16f08e6bcb1420d947c82 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:18:28 -0400 Subject: [PATCH 08/17] kernel: per-path access query, and both file-test sites read it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/kaish-kernel/src/backend/local.rs | 9 +++++ crates/kaish-kernel/src/backend/overlay.rs | 16 ++++++++ crates/kaish-kernel/src/kernel.rs | 38 ++++++++++++------- crates/kaish-kernel/src/tools/builtin/test.rs | 35 ++++++++++------- crates/kaish-kernel/src/vfs/router.rs | 10 +++++ .../tests/file_test_writable_tests.rs | 29 +++++++++----- crates/kaish-tool-api/src/backend.rs | 26 ++++++++++++- crates/kaish-vfs/src/lib.rs | 2 +- crates/kaish-vfs/src/overlay.rs | 21 +++++++++- crates/kaish-vfs/src/traits.rs | 25 +++++++++++- 10 files changed, 172 insertions(+), 39 deletions(-) diff --git a/crates/kaish-kernel/src/backend/local.rs b/crates/kaish-kernel/src/backend/local.rs index 1aafa72c..9b769940 100644 --- a/crates/kaish-kernel/src/backend/local.rs +++ b/crates/kaish-kernel/src/backend/local.rs @@ -13,6 +13,7 @@ use super::{ }; use crate::tools::{ToolArgs, ToolCtx, ToolRegistry}; use crate::vfs::{DirEntry, Filesystem, MountInfo, VfsRouter}; +use kaish_types::PathAccess; /// Local backend implementation using VfsRouter and ToolRegistry. /// @@ -400,6 +401,14 @@ impl KernelBackend for LocalBackend { self.vfs.read_only() } + /// Asks the router, which asks the mount that owns the path. The trait + /// default would use `read_only()` above — the whole-router answer — + /// which is false whenever any one mount is writable and would call + /// `/v/bin/echo` writable on the strength of `/tmp`. + async fn path_access(&self, path: &Path) -> BackendResult { + Ok(self.vfs.path_access(path).await?) + } + fn backend_type(&self) -> &str { "local" } diff --git a/crates/kaish-kernel/src/backend/overlay.rs b/crates/kaish-kernel/src/backend/overlay.rs index 855e0974..8de3aba6 100644 --- a/crates/kaish-kernel/src/backend/overlay.rs +++ b/crates/kaish-kernel/src/backend/overlay.rs @@ -40,6 +40,7 @@ use super::{ }; use crate::tools::{ToolArgs, ToolCtx}; use crate::vfs::{DirEntry, Filesystem, MountInfo, VfsRouter}; +use kaish_types::PathAccess; /// The final path component, used to name a synthesized directory entry for a /// shared ancestor (`/v` → `v`). Falls back to `/` for a component-less path. @@ -418,6 +419,21 @@ impl KernelBackend for VirtualOverlayBackend { self.inner.read_only() && self.vfs.read_only() } + /// Routes the same way `stat` does, so the answer comes from the layer + /// that owns the path. The trait default would use `read_only()` above, + /// which is the AND of both layers and belongs to neither path. + async fn path_access(&self, path: &Path) -> BackendResult { + if self.is_virtual_path(path) { + Ok(self.vfs.path_access(path).await?) + } else if self.is_shared_ancestor(path) { + // A synthesized ancestor directory exists only to be traversed: + // readable and searchable, and kaish creates nothing in it. + Ok(PathAccess::resolve(Some(0o555), true)) + } else { + self.inner.path_access(path).await + } + } + fn backend_type(&self) -> &str { "virtual-overlay" } diff --git a/crates/kaish-kernel/src/kernel.rs b/crates/kaish-kernel/src/kernel.rs index c881c24a..77df544e 100644 --- a/crates/kaish-kernel/src/kernel.rs +++ b/crates/kaish-kernel/src/kernel.rs @@ -4343,20 +4343,32 @@ impl Kernel { let ctx = self.exec_ctx.read().await; (ctx.resolve_path(&path_str), ctx.backend.clone()) }; - let entry = backend.stat(&resolved).await.ok(); + // `-r`/`-w`/`-x` go through `path_access`, never through + // the raw mode bits: the mount's read-only state is half + // the answer and `stat` does not carry it. The `test` + // builtin's `file_test` reads the same query, and + // `file_test_writable_tests` runs every case through both + // spellings — this mirror has drifted before. Ok(match op { - FileTestOp::Exists => entry.is_some(), - FileTestOp::IsFile => entry.as_ref().is_some_and(|e| e.is_file()), - FileTestOp::IsDir => entry.as_ref().is_some_and(|e| e.is_dir()), - FileTestOp::Readable => entry.as_ref().is_some_and(|e| { - e.permissions.is_none_or(|p| p & 0o444 != 0) - }), - FileTestOp::Writable => entry.as_ref().is_some_and(|e| { - e.permissions.is_none_or(|p| p & 0o222 != 0) - }), - FileTestOp::Executable => entry.as_ref().is_some_and(|e| { - e.permissions.is_some_and(|p| p & 0o111 != 0) - }), + FileTestOp::Exists => backend.stat(&resolved).await.is_ok(), + FileTestOp::IsFile => { + backend.stat(&resolved).await.is_ok_and(|e| e.is_file()) + } + FileTestOp::IsDir => { + backend.stat(&resolved).await.is_ok_and(|e| e.is_dir()) + } + FileTestOp::Readable => backend + .path_access(&resolved) + .await + .is_ok_and(|access| access.readable), + FileTestOp::Writable => backend + .path_access(&resolved) + .await + .is_ok_and(|access| access.writable), + FileTestOp::Executable => backend + .path_access(&resolved) + .await + .is_ok_and(|access| access.executable), }) } TestExpr::StringTest { op, value } => match op { diff --git a/crates/kaish-kernel/src/tools/builtin/test.rs b/crates/kaish-kernel/src/tools/builtin/test.rs index ba15c1af..442e90f3 100644 --- a/crates/kaish-kernel/src/tools/builtin/test.rs +++ b/crates/kaish-kernel/src/tools/builtin/test.rs @@ -338,20 +338,29 @@ async fn file_test(ctx: &ExecContext, op: &str, path: &str) -> bool { return false; } let resolved = ctx.resolve_path(path); - let entry = ctx.backend.stat(&resolved).await.ok(); + // `-r`/`-w`/`-x` go through `path_access`, never through the raw mode + // bits: the mount's read-only state is half the answer and `stat` does + // not carry it. `eval_test_async` reads the same query, and + // `file_test_writable_tests` runs every case through both. match op { - "-e" => entry.is_some(), - "-f" => entry.as_ref().is_some_and(|e| e.is_file()), - "-d" => entry.as_ref().is_some_and(|e| e.is_dir()), - "-r" => entry - .as_ref() - .is_some_and(|e| e.permissions.is_none_or(|p| p & 0o444 != 0)), - "-w" => entry - .as_ref() - .is_some_and(|e| e.permissions.is_none_or(|p| p & 0o222 != 0)), - "-x" => entry - .as_ref() - .is_some_and(|e| e.permissions.is_some_and(|p| p & 0o111 != 0)), + "-e" => ctx.backend.stat(&resolved).await.is_ok(), + "-f" => ctx.backend.stat(&resolved).await.is_ok_and(|e| e.is_file()), + "-d" => ctx.backend.stat(&resolved).await.is_ok_and(|e| e.is_dir()), + "-r" => ctx + .backend + .path_access(&resolved) + .await + .is_ok_and(|access| access.readable), + "-w" => ctx + .backend + .path_access(&resolved) + .await + .is_ok_and(|access| access.writable), + "-x" => ctx + .backend + .path_access(&resolved) + .await + .is_ok_and(|access| access.executable), _ => unreachable!("file_test called with non-file op {op:?}"), } } diff --git a/crates/kaish-kernel/src/vfs/router.rs b/crates/kaish-kernel/src/vfs/router.rs index bbdfeebe..cbeeea24 100644 --- a/crates/kaish-kernel/src/vfs/router.rs +++ b/crates/kaish-kernel/src/vfs/router.rs @@ -3,6 +3,7 @@ //! Routes filesystem operations to the appropriate backend based on path. use super::{DirEntry, Filesystem}; +use kaish_vfs::PathAccess; use async_trait::async_trait; use std::collections::BTreeMap; use std::io; @@ -388,6 +389,15 @@ impl Filesystem for VfsRouter { from_fs.rename(&from_relative, &to_relative).await } + /// Delegates to the mount that owns the path, so the answer is that + /// mount's — not the whole router's. `read_only()` above is the + /// whole-router question and cannot answer for one path: a router with a + /// writable `/` and a read-only `/v/bin` is read-only for neither. + async fn path_access(&self, path: &Path) -> io::Result { + let (fs, relative) = self.find_mount(path)?; + fs.path_access(&relative).await + } + fn read_only(&self) -> bool { // Router is read-only iff every mount is. Empty router returns // false — a router with no mounts isn't meaningfully read-only, diff --git a/crates/kaish-kernel/tests/file_test_writable_tests.rs b/crates/kaish-kernel/tests/file_test_writable_tests.rs index 4ae23653..822d2e7d 100644 --- a/crates/kaish-kernel/tests/file_test_writable_tests.rs +++ b/crates/kaish-kernel/tests/file_test_writable_tests.rs @@ -32,7 +32,7 @@ mod common; use std::sync::Arc; -use kaish_kernel::vfs::{LocalFs, MemoryFs, VfsRouter}; +use kaish_kernel::vfs::{DevFs, LocalFs, MemoryFs, VfsRouter}; use kaish_kernel::{Kernel, KernelBackend, KernelConfig, LocalBackend}; use common::{kernel_at, run}; @@ -163,14 +163,28 @@ async fn jobfs_node_is_not_writable() { // ── DevFs: writable on purpose, reports no mode ──────────────────────────── +/// Mount DevFs explicitly rather than trusting a config to do it. +/// +/// `kernel_at` is Passthrough — `/` is `LocalFs("/")`, so `/dev/null` there +/// is the host's device node and these assertions would pass without DevFs +/// being involved at all. That is how the first draft of this file was +/// vacuously green. +fn devfs_kernel() -> Kernel { + let mut vfs = VfsRouter::new(); + vfs.mount("/", MemoryFs::new()); + vfs.mount("/dev", DevFs::new()); + let backend: Arc = Arc::new(LocalBackend::new(Arc::new(vfs))); + Kernel::with_backend(backend, KernelConfig::isolated(), |_| {}, |_| {}) + .expect("with_backend kernel") +} + /// `DevFs::read_only()` is deliberately `false` — refusing the write would /// break `> /dev/null`. So the mount cannot carry the answer here and the /// mode has to: the device files report `0o666`, matching crw-rw-rw- on /// Linux. #[tokio::test] async fn devfs_null_is_writable() { - let tmp = tempfile::tempdir().unwrap(); - let kernel = kernel_at(tmp.path()); + let kernel = devfs_kernel(); both_spellings(&kernel, "", "-w", "/dev/null", true).await; let (_, code) = run(&kernel, "echo discard > /dev/null").await; assert_eq!(code, 0, "/dev/null must accept the write it says it accepts"); @@ -179,8 +193,7 @@ async fn devfs_null_is_writable() { /// A character device is not executable. #[tokio::test] async fn devfs_null_is_not_executable() { - let tmp = tempfile::tempdir().unwrap(); - let kernel = kernel_at(tmp.path()); + let kernel = devfs_kernel(); both_spellings(&kernel, "", "-x", "/dev/null", false).await; both_spellings(&kernel, "", "-r", "/dev/null", true).await; } @@ -189,12 +202,11 @@ async fn devfs_null_is_not_executable() { /// deliberate one-bit divergence from Linux's 0755: kaish's `DevFs::mkdir` /// and `DevFs::remove` refuse unconditionally, for every caller, because /// kaish has no root user to be the exception 0755 carves out. `0o555` is -/// the mode that tells the truth about this mount. The write below is the +/// the mode that tells the truth about this mount. The `mkdir` below is the /// receipt — `-w` and `mkdir` have to agree. #[tokio::test] async fn devfs_directory_is_searchable_but_not_writable() { - let tmp = tempfile::tempdir().unwrap(); - let kernel = kernel_at(tmp.path()); + let kernel = devfs_kernel(); both_spellings(&kernel, "", "-x", "/dev", true).await; both_spellings(&kernel, "", "-r", "/dev", true).await; both_spellings(&kernel, "", "-w", "/dev", false).await; @@ -267,7 +279,6 @@ async fn memory_backed_files_are_not_executable() { both_spellings(&kernel, "echo hi > /v/probe.txt", "-x", "/v/probe.txt", false).await; // a backend that models no permissions at all: not executable either both_spellings(&kernel, "", "-x", "/v/bin/echo", false).await; - both_spellings(&kernel, "", "-x", "/dev/null", false).await; } // ── The hazard: read-only wrapper over an OS-writable directory ──────────── diff --git a/crates/kaish-tool-api/src/backend.rs b/crates/kaish-tool-api/src/backend.rs index e2837406..7c235d8b 100644 --- a/crates/kaish-tool-api/src/backend.rs +++ b/crates/kaish-tool-api/src/backend.rs @@ -12,7 +12,7 @@ use async_trait::async_trait; use kaish_types::backend::{ BackendResult, MountInfo, PatchOp, ReadRange, ToolInfo, ToolResult, WriteMode, }; -use kaish_types::{DirEntry, ToolArgs}; +use kaish_types::{DirEntry, PathAccess, ToolArgs}; use crate::ctx::ToolCtx; @@ -118,6 +118,30 @@ pub trait KernelBackend: Send + Sync { /// Returns true if this backend is read-only. fn read_only(&self) -> bool; + /// What the kernel can do with one path: the query behind `test -r`, + /// `test -w`, and `test -x`. + /// + /// Neither [`KernelBackend::read_only`] nor `DirEntry.permissions` + /// answers "can this path be written" alone. A read-only wrapper over an + /// OS-writable directory reports permissive mode bits and refuses every + /// write; a `DevFs` mount reports `read_only() == false` (so `> + /// /dev/null` works) while its `/dev` directory accepts nothing. + /// [`PathAccess::resolve`] combines the two, and is the only way to build + /// a `PathAccess` — a caller cannot consult one fact by accident. + /// + /// The default answers from `stat` plus this backend's whole-backend + /// `read_only()`, which is right for a backend that is uniformly + /// read-only or uniformly writable. A backend whose mounts differ — + /// `LocalBackend`, which routes through a `VfsRouter` — overrides this to + /// ask the mount that owns the path. + /// + /// Errors exactly as `stat` does: a path that does not exist is an error, + /// not a `PathAccess` of all-false. + async fn path_access(&self, path: &Path) -> BackendResult { + let entry = self.stat(path).await?; + Ok(PathAccess::resolve(entry.permissions, self.read_only())) + } + /// Returns the backend type identifier (e.g. "local", "kaijutsu"). fn backend_type(&self) -> &str; diff --git a/crates/kaish-vfs/src/lib.rs b/crates/kaish-vfs/src/lib.rs index ddf34d93..24ab61cf 100644 --- a/crates/kaish-vfs/src/lib.rs +++ b/crates/kaish-vfs/src/lib.rs @@ -14,7 +14,7 @@ mod traits; pub use budget::ByteBudget; pub use dev::DevFs; -pub use traits::{DirEntry, DirEntryKind, Filesystem, ReadRange}; +pub use traits::{DirEntry, DirEntryKind, Filesystem, PathAccess, ReadRange}; // `LocalFs` pulls in `tokio/fs`; gated so the in-memory/wasm sandbox build // (which doesn't enable `localfs`) stays free of a real-filesystem dependency. diff --git a/crates/kaish-vfs/src/overlay.rs b/crates/kaish-vfs/src/overlay.rs index c64fffa4..dcbe8c3a 100644 --- a/crates/kaish-vfs/src/overlay.rs +++ b/crates/kaish-vfs/src/overlay.rs @@ -7,7 +7,7 @@ use crate::budget::ByteBudget; use crate::paths::normalize; -use crate::traits::{DirEntry, DirEntryKind, Filesystem, ReadRange}; +use crate::traits::{DirEntry, DirEntryKind, Filesystem, PathAccess, ReadRange}; use async_trait::async_trait; use std::collections::{BTreeMap, HashMap, HashSet}; use std::io; @@ -1091,6 +1091,25 @@ impl Filesystem for OverlayFs { false } + /// Reads resolve against whichever layer holds the path; writes always + /// land in the upper. So the upper answers `writable` and the visible + /// entry answers the rest — a lower file whose mode clears `0o222` is + /// still writable here, because `write` copies it up and writes the + /// upper without consulting the lower's mode. + async fn path_access(&self, path: &Path) -> io::Result { + let path = normalize(path); + // `self.stat` honours whiteouts, so a removed path errors NotFound. + let visible = self.stat(&path).await?; + // Absent from the upper means the write would create it there. + let upper_mode = match self.upper.stat(&path).await { + Ok(entry) => entry.permissions, + Err(error) if is_not_found(&error) => None, + Err(error) => return Err(error), + }; + Ok(PathAccess::resolve(visible.permissions, self.read_only()) + .with_write_layer(upper_mode, self.upper.read_only())) + } + /// Base snapshots plus whatever the upper reports as memory-resident. /// With the conventional private `MemoryFs` upper this is the full 2× of /// copy-up; with a disk-backed upper it's the bases alone — this counter diff --git a/crates/kaish-vfs/src/traits.rs b/crates/kaish-vfs/src/traits.rs index efdda821..6ec0135e 100644 --- a/crates/kaish-vfs/src/traits.rs +++ b/crates/kaish-vfs/src/traits.rs @@ -6,7 +6,7 @@ use std::path::{Path, PathBuf}; use std::time::SystemTime; // DirEntry and DirEntryKind live in kaish-types. -pub use kaish_types::{DirEntry, DirEntryKind, ReadRange}; +pub use kaish_types::{DirEntry, DirEntryKind, PathAccess, ReadRange}; /// Abstract filesystem interface. /// @@ -94,6 +94,29 @@ pub trait Filesystem: Send + Sync { /// Returns true if this filesystem is read-only. fn read_only(&self) -> bool; + /// What the kernel can do with one path on this filesystem. + /// + /// This is the query behind `test -w`, `test -r`, and `test -x`. It + /// exists because neither [`Filesystem::read_only`] nor + /// `DirEntry.permissions` answers "can this path be written" on its own: + /// `MemoryFs` (writable) and `JobFs` (read-only) both report + /// `permissions: None`, and a `LocalFs::read_only` wrapper over an + /// OS-writable directory reports mode bits with the write bit set. + /// [`PathAccess::resolve`] is the only place the two combine. + /// + /// The default asks `stat` for the mode and this filesystem for the + /// read-only state, which is right for any filesystem that is uniformly + /// read-only or uniformly writable. `VfsRouter` overrides it to ask the + /// mount that owns the path, and `OverlayFs` overrides it because writes + /// land in a different layer than reads resolve against. + /// + /// Errors exactly as `stat` does: a path that does not exist is an error, + /// not a `PathAccess` of all-false. + async fn path_access(&self, path: &Path) -> io::Result { + let entry = self.stat(path).await?; + Ok(PathAccess::resolve(entry.permissions, self.read_only())) + } + /// Memory-resident content bytes this filesystem is holding, if it /// tracks them. /// From b3d97ae51d1bf5e541c655855659d8f7e8830017 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:22:34 -0400 Subject: [PATCH 09/17] vfs: leave OverlayFs on the default, and write down why MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/kaish-types/src/path_access.rs | 41 +-------------------------- crates/kaish-vfs/src/overlay.rs | 32 ++++++++------------- crates/kaish-vfs/src/traits.rs | 12 ++++++-- 3 files changed, 23 insertions(+), 62 deletions(-) diff --git a/crates/kaish-types/src/path_access.rs b/crates/kaish-types/src/path_access.rs index 765ef1c1..83b31de7 100644 --- a/crates/kaish-types/src/path_access.rs +++ b/crates/kaish-types/src/path_access.rs @@ -18,10 +18,7 @@ /// writable device from the unwritable directory above it. /// /// [`PathAccess::resolve`] takes both and is the only constructor, so no -/// caller can answer from one of them by accident. -/// [`PathAccess::with_write_layer`] takes both again, for a copy-on-write -/// overlay whose writes land somewhere other than where its reads resolve. -/// The struct is +/// caller can answer from one of them by accident. The struct is /// `#[non_exhaustive]`: read the fields, do not construct it by literal. /// /// # What an absent mode means @@ -80,23 +77,6 @@ impl PathAccess { } } - /// Re-answer `writable` from a different layer than the one that answered - /// `readable` and `executable`. - /// - /// Copy-on-write overlays need this: reads resolve against whichever - /// layer holds the path, but every write lands in the upper layer, so the - /// upper layer decides writability. A lower file whose mode clears `0o222` - /// is still writable through copy-up, because `OverlayFs::write` copies - /// the content up and writes the upper — it never consults the lower's - /// mode. - /// - /// Takes the same pair as [`PathAccess::resolve`], for the write layer. - pub fn with_write_layer(self, mode: Option, mount_read_only: bool) -> Self { - Self { - writable: Self::resolve(mode, mount_read_only).writable, - ..self - } - } } #[cfg(test)] @@ -151,24 +131,5 @@ mod tests { assert!(access.executable, "read-only says nothing about exec"); } - /// Copy-up: the lower's mode answers read and exec, the upper answers - /// write. - #[test] - fn write_layer_replaces_only_the_write_answer() { - let lower = PathAccess::resolve(Some(0o444), false); - assert!(!lower.writable); - // The upper is MemoryFs, so it reports 0o666 for the copied-up file. - let overlaid = lower.with_write_layer(Some(0o666), false); - assert!(overlaid.writable, "copy-up makes a mode-444 lower writable"); - assert!(overlaid.readable); - assert_eq!(overlaid.executable, lower.executable); - } - /// A read-only upper makes the whole overlay unwritable, whatever the - /// lower reports. - #[test] - fn a_read_only_write_layer_wins() { - let overlaid = PathAccess::resolve(Some(0o755), false).with_write_layer(Some(0o755), true); - assert!(!overlaid.writable); - } } diff --git a/crates/kaish-vfs/src/overlay.rs b/crates/kaish-vfs/src/overlay.rs index dcbe8c3a..bf2e926a 100644 --- a/crates/kaish-vfs/src/overlay.rs +++ b/crates/kaish-vfs/src/overlay.rs @@ -7,7 +7,7 @@ use crate::budget::ByteBudget; use crate::paths::normalize; -use crate::traits::{DirEntry, DirEntryKind, Filesystem, PathAccess, ReadRange}; +use crate::traits::{DirEntry, DirEntryKind, Filesystem, ReadRange}; use async_trait::async_trait; use std::collections::{BTreeMap, HashMap, HashSet}; use std::io; @@ -1091,25 +1091,6 @@ impl Filesystem for OverlayFs { false } - /// Reads resolve against whichever layer holds the path; writes always - /// land in the upper. So the upper answers `writable` and the visible - /// entry answers the rest — a lower file whose mode clears `0o222` is - /// still writable here, because `write` copies it up and writes the - /// upper without consulting the lower's mode. - async fn path_access(&self, path: &Path) -> io::Result { - let path = normalize(path); - // `self.stat` honours whiteouts, so a removed path errors NotFound. - let visible = self.stat(&path).await?; - // Absent from the upper means the write would create it there. - let upper_mode = match self.upper.stat(&path).await { - Ok(entry) => entry.permissions, - Err(error) if is_not_found(&error) => None, - Err(error) => return Err(error), - }; - Ok(PathAccess::resolve(visible.permissions, self.read_only()) - .with_write_layer(upper_mode, self.upper.read_only())) - } - /// Base snapshots plus whatever the upper reports as memory-resident. /// With the conventional private `MemoryFs` upper this is the full 2× of /// copy-up; with a disk-backed upper it's the bases alone — this counter @@ -2052,6 +2033,17 @@ mod tests { use super::*; use crate::local::LocalFs; + // A whiteouted path is gone, so it has no access at all — not a + // PathAccess of all-false, an error, the same as stat. + #[tokio::test] + async fn path_access_errors_on_a_whiteouted_path() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("gone.txt"), b"x").unwrap(); + let overlay = OverlayFs::over(Arc::new(LocalFs::read_only(dir.path()))); + overlay.remove(Path::new("gone.txt")).await.unwrap(); + assert!(overlay.path_access(Path::new("gone.txt")).await.is_err()); + } + #[tokio::test] async fn test_real_tree_byte_identical_after_overlay_writes() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/kaish-vfs/src/traits.rs b/crates/kaish-vfs/src/traits.rs index 6ec0135e..0a3a2945 100644 --- a/crates/kaish-vfs/src/traits.rs +++ b/crates/kaish-vfs/src/traits.rs @@ -107,8 +107,16 @@ pub trait Filesystem: Send + Sync { /// The default asks `stat` for the mode and this filesystem for the /// read-only state, which is right for any filesystem that is uniformly /// read-only or uniformly writable. `VfsRouter` overrides it to ask the - /// mount that owns the path, and `OverlayFs` overrides it because writes - /// land in a different layer than reads resolve against. + /// mount that owns the path. + /// + /// `OverlayFs` keeps the default, and inherits one known inaccuracy from + /// it: reads resolve against whichever layer holds the path, but writes + /// always land in the upper and `OverlayFs::write` never consults the + /// lower's mode. So a lower file whose mode clears `0o222` reports + /// unwritable while copy-up would in fact write it. That answer is + /// unchanged from before this query existed, and correcting it means + /// deciding what mode a path that does not exist in the upper yet should + /// be judged by — a question with no answer in the code today. /// /// Errors exactly as `stat` does: a path that does not exist is an error, /// not a `PathAccess` of all-false. From 5b4fc9434d8b2c0e29456634107410d24c862f2e Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:22:56 -0400 Subject: [PATCH 10/17] docs: changelog for the file-test writability fix Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c549c17d..c426f339 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,23 @@ breaking entries are marked **BREAKING**. ## [Unreleased] +### Fixed + +- **`test -w` on a virtual path** — `-w /v/bin/echo` and `-w /v/jobs/1/status` + answered yes about read-only mounts. `MemoryFs` and `DevFs` now report modes, + and `-w` needs the mount and the mode to agree. + +### Changed + +- **`test -x DIR` on a memory-backed directory** — now yes. `x` on a directory + means searchable, which these are; it answered no while modes were absent. + +### Added + +- **`KernelBackend::path_access` and `Filesystem::path_access`** — the per-path + read/write/execute query behind the file tests. Both are defaulted, so + existing implementations keep compiling; not breaking. + ## [0.16.0] - 2026-08-23 ### Added From 031b9d4bd2f7b665a383b6f34acb2c133a8afea0 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:24:46 -0400 Subject: [PATCH 11/17] docs: name the two facts -w reads, and -x on a directory Co-Authored-By: Claude Opus 5 --- docs/LANGUAGE.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/LANGUAGE.md b/docs/LANGUAGE.md index 2873d12d..fd5c9c55 100644 --- a/docs/LANGUAGE.md +++ b/docs/LANGUAGE.md @@ -629,8 +629,8 @@ mkdir /tmp/work && cd /tmp/work && echo "ready" [[ -d /path/dir ]] # is directory [[ -e /path/any ]] # exists [[ -r /path/file ]] # readable -[[ -w /path/file ]] # writable -[[ -x /path/file ]] # executable +[[ -w /path/file ]] # writable — the mount and the mode must agree +[[ -x /path/file ]] # executable; on a directory, searchable # String tests [[ -z $VAR ]] # empty From 33967a49d1da2b73b4a10624cb4d839d5980ac29 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:29:06 -0400 Subject: [PATCH 12/17] kernel: path_access must fall back the way stat does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/kaish-kernel/src/vfs/router.rs | 111 +++++++++++++++++- .../tests/file_test_writable_tests.rs | 40 +++++++ 2 files changed, 148 insertions(+), 3 deletions(-) diff --git a/crates/kaish-kernel/src/vfs/router.rs b/crates/kaish-kernel/src/vfs/router.rs index cbeeea24..ef5338cc 100644 --- a/crates/kaish-kernel/src/vfs/router.rs +++ b/crates/kaish-kernel/src/vfs/router.rs @@ -15,6 +15,12 @@ use std::sync::Arc; // paths keep working. pub use kaish_types::backend::MountInfo; +/// Mode reported for a directory the router synthesizes rather than reads +/// from a mount: the root, and any ancestor of a mount that has no mount of +/// its own. Readable and searchable, never writable — these directories are +/// derived from the mount table and the router creates nothing in them. +const SYNTHESIZED_DIRECTORY_MODE: u32 = 0o555; + /// Routes filesystem operations to mounted backends. /// /// Mount points are matched by longest prefix. For example, if `/mnt` and @@ -390,12 +396,30 @@ impl Filesystem for VfsRouter { } /// Delegates to the mount that owns the path, so the answer is that - /// mount's — not the whole router's. `read_only()` above is the + /// mount's — not the whole router's. `read_only()` below is the /// whole-router question and cannot answer for one path: a router with a /// writable `/` and a read-only `/v/bin` is read-only for neither. + /// + /// Falls back the way `stat` does. `stat` synthesizes a directory for the + /// root and for any ancestor of a mount (`/v` above `/v/jobs`), so those + /// paths exist, and an answer here that errored on them would contradict + /// it: `[[ -e /v ]]` true and `[[ -r /v ]]` false about the same path. + /// A synthesized directory is readable and searchable, and never + /// writable — the router creates nothing in one. async fn path_access(&self, path: &Path) -> io::Result { - let (fs, relative) = self.find_mount(path)?; - fs.path_access(&relative).await + match self.find_mount(path) { + Ok((fs, relative)) => fs.path_access(&relative).await, + Err(e) => { + let path_str = path.to_string_lossy(); + let is_synthesized = + path_str.is_empty() || path_str == "/" || self.has_mount_under(path); + if is_synthesized { + Ok(PathAccess::resolve(Some(SYNTHESIZED_DIRECTORY_MODE), true)) + } else { + Err(e) + } + } + } } fn read_only(&self) -> bool { @@ -632,6 +656,87 @@ mod tests { assert_eq!(result.unwrap_err().kind(), io::ErrorKind::Unsupported); } + // `stat` synthesizes a directory for the root and for any ancestor of a + // mount, so those paths exist. `path_access` has to agree with `stat` + // about the same path — going straight to `find_mount` errors where + // `stat` succeeds, and `[[ -e /v ]]` would be true while `[[ -r /v ]]` + // was false about the identical path. + #[tokio::test] + async fn path_access_agrees_with_stat_on_synthesized_directories() { + let mut router = VfsRouter::new(); + router.mount("/v/docs", MemoryFs::new()); + + for path in ["/", "/v"] { + let path = Path::new(path); + assert!( + router.stat(path).await.is_ok(), + "{} is synthesized by stat", + path.display() + ); + let access = router + .path_access(path) + .await + .unwrap_or_else(|e| panic!("path_access must not error where stat succeeds: {e}")); + assert!(access.readable, "{} must be readable", path.display()); + assert!(access.executable, "{} must be searchable", path.display()); + assert!( + !access.writable, + "the router creates nothing in {}", + path.display() + ); + } + } + + // The synthesis must not swallow a genuinely absent path. + #[tokio::test] + async fn path_access_errors_on_a_path_with_no_mount() { + let mut router = VfsRouter::new(); + router.mount("/v/docs", MemoryFs::new()); + assert!(router.path_access(Path::new("/nope")).await.is_err()); + assert!(router.path_access(Path::new("/v/docs/absent")).await.is_err()); + } + + // A real mount answers for itself, not with the synthesized defaults. + #[tokio::test] + async fn path_access_at_a_mount_point_asks_the_mount() { + let mut router = VfsRouter::new(); + router.mount("/rw", MemoryFs::new()); + router.mount("/ro", BuiltinFsStub); + + assert!(router.path_access(Path::new("/rw")).await.unwrap().writable); + assert!(!router.path_access(Path::new("/ro")).await.unwrap().writable); + assert!(router.path_access(Path::new("/ro")).await.unwrap().readable); + } + + /// A minimal read-only mount that reports no mode, standing in for + /// `BuiltinFs`/`JobFs` without dragging a ToolRegistry into this module. + struct BuiltinFsStub; + + #[async_trait] + impl Filesystem for BuiltinFsStub { + async fn read(&self, _path: &Path) -> io::Result> { + Ok(Vec::new()) + } + async fn write(&self, _path: &Path, _data: &[u8]) -> io::Result<()> { + Err(io::Error::new(io::ErrorKind::PermissionDenied, "read-only")) + } + async fn list(&self, _path: &Path) -> io::Result> { + Ok(Vec::new()) + } + async fn stat(&self, _path: &Path) -> io::Result { + Ok(DirEntry::directory(".")) + } + async fn mkdir(&self, _path: &Path) -> io::Result<()> { + Err(io::Error::new(io::ErrorKind::PermissionDenied, "read-only")) + } + async fn remove(&self, _path: &Path) -> io::Result<()> { + Err(io::Error::new(io::ErrorKind::PermissionDenied, "read-only")) + } + fn read_only(&self) -> bool { + true + } + } + #[tokio::test] async fn read_only_empty_router_returns_false() { let router = VfsRouter::new(); diff --git a/crates/kaish-kernel/tests/file_test_writable_tests.rs b/crates/kaish-kernel/tests/file_test_writable_tests.rs index 822d2e7d..3011799a 100644 --- a/crates/kaish-kernel/tests/file_test_writable_tests.rs +++ b/crates/kaish-kernel/tests/file_test_writable_tests.rs @@ -365,3 +365,43 @@ async fn missing_paths_answer_false_everywhere() { both_spellings(&kernel, "", op, "/v/bin/definitely-not-a-builtin", false).await; } } + +// ── Synthesized directories (no mount of their own) ──────────────────────── + +/// A router with no `/` mount, only a nested one — the shape the +/// `Kernel::with_backend` doc example builds (`vfs.mount_arc("/v/docs", …)`). +fn nested_mount_only_kernel() -> Kernel { + let mut vfs = VfsRouter::new(); + vfs.mount("/v/docs", MemoryFs::new()); + let backend: Arc = Arc::new(LocalBackend::new(Arc::new(vfs))); + Kernel::with_backend(backend, KernelConfig::isolated(), |_| {}, |_| {}) + .expect("with_backend kernel") +} + +/// `VfsRouter::stat` synthesizes a directory for the root and for any +/// ancestor of a mount, so `-e /v` is true even though nothing is mounted +/// there. `-r` and `-x` have to agree with `-e` about the same path: a +/// `path_access` that went straight to `find_mount` would error where `stat` +/// succeeds, and `[[ -e /v ]] && [[ -r /v ]]` would answer true then false. +#[tokio::test] +async fn synthesized_ancestor_directories_are_readable_and_searchable() { + let kernel = nested_mount_only_kernel(); + for path in ["/", "/v"] { + both_spellings(&kernel, "", "-e", path, true).await; + both_spellings(&kernel, "", "-d", path, true).await; + both_spellings(&kernel, "", "-r", path, true).await; + both_spellings(&kernel, "", "-x", path, true).await; + // The router creates nothing in a directory it synthesized. + both_spellings(&kernel, "", "-w", path, false).await; + } +} + +/// The real mount underneath still answers for itself. +#[tokio::test] +async fn a_real_mount_under_a_synthesized_ancestor_still_answers() { + let kernel = nested_mount_only_kernel(); + both_spellings(&kernel, "", "-w", "/v/docs", true).await; + let (_, code) = run(&kernel, "echo hi > /v/docs/note.txt").await; + assert_eq!(code, 0, "the real mount must accept the write"); + both_spellings(&kernel, "", "-w", "/v/docs/note.txt", true).await; +} From 826394ab20fbb9771f223b58f77916abfa81e8b7 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:33:59 -0400 Subject: [PATCH 13/17] docs: correct BuiltinFs's lying comment, instruct the next backend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/kaish-kernel/src/vfs/builtin_fs.rs | 24 +++++++++++++++++-- crates/kaish-vfs/src/local.rs | 12 +++++++--- crates/kaish-vfs/src/traits.rs | 29 +++++++++++++++++++++++ 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/crates/kaish-kernel/src/vfs/builtin_fs.rs b/crates/kaish-kernel/src/vfs/builtin_fs.rs index b5fcc4a5..d226cca1 100644 --- a/crates/kaish-kernel/src/vfs/builtin_fs.rs +++ b/crates/kaish-kernel/src/vfs/builtin_fs.rs @@ -1,4 +1,23 @@ -//! BuiltinFs — read-only VFS that presents builtins as executable entries under `/v/bin/`. +//! BuiltinFs — read-only VFS that lists builtins as file entries under `/v/bin/`. +//! +//! **The entries are not executable, and nothing here ever executes.** This +//! comment used to say "executable entries", which was a claim the code has +//! never made good on: `stat` reports no mode, so `test -x /v/bin/grep` +//! answers NO, and `real_path` returns `None`, so there is no path for +//! exec(2) to open. `read` returns a line that opens with `#!`, which makes +//! the entry *look* executable and is the likeliest reason the claim went +//! unnoticed. +//! +//! Running a builtin goes by name through the `ToolRegistry`; it never routes +//! through this filesystem. `/v/bin` is an inventory an agent can list and +//! read, not a directory of programs. +//! +//! For the original claim to hold, `stat` and `list` would have to report a +//! mode with the `0o111` bits set. That is a deliberate non-change: it would +//! flip `test -x /v/bin/*` from NO to YES, which is a behavior change that +//! wants its own decision. Note also that `read_only()` below is `true`, and +//! the closed `-w` default depends on that staying true — see +//! `Filesystem::path_access`. use std::io; use std::path::{Path, PathBuf}; @@ -9,7 +28,8 @@ use async_trait::async_trait; use crate::tools::ToolRegistry; use super::{DirEntry, Filesystem}; -/// A read-only filesystem that exposes registered builtins as entries. +/// A read-only filesystem that exposes registered builtins as file +/// entries. Listable and readable; not executable — see the module docs. pub struct BuiltinFs { tools: Arc, } diff --git a/crates/kaish-vfs/src/local.rs b/crates/kaish-vfs/src/local.rs index d022f4ef..47d10476 100644 --- a/crates/kaish-vfs/src/local.rs +++ b/crates/kaish-vfs/src/local.rs @@ -198,11 +198,17 @@ impl LocalFs { /// Synthesize a Unix-shaped mode from the one permission fact a non-Unix /// platform exposes. /// + /// This arm is live, not a Windows courtesy: `wasm32-wasip1` is not + /// `unix`, `mod local` is declared unconditionally, and kaish ships and + /// builds that target every CI run. + /// /// `LocalFs` is writable, so returning `None` here would put it in the /// same position `MemoryFs` was in: a writable backend reporting an - /// absent mode, which is what makes `test -w` unanswerable. There is - /// exactly one bit to work from — `Permissions::readonly()` — so that is - /// what the mode carries. + /// absent mode, which `PathAccess::resolve` reads as read-only. Every + /// file test on the WASI build would answer "not writable", and the wasi + /// CI leg compiles without running these tests, so nothing would say so. + /// There is exactly one bit to work from — `Permissions::readonly()` — so + /// that is what the mode carries. /// /// The `x` bit is never set. Executability is not a permission on these /// platforms (it is decided by the file extension), so claiming it would diff --git a/crates/kaish-vfs/src/traits.rs b/crates/kaish-vfs/src/traits.rs index 0a3a2945..0aff3f02 100644 --- a/crates/kaish-vfs/src/traits.rs +++ b/crates/kaish-vfs/src/traits.rs @@ -118,6 +118,35 @@ pub trait Filesystem: Send + Sync { /// deciding what mode a path that does not exist in the upper yet should /// be judged by — a question with no answer in the code today. /// + /// # If you are adding a backend, report a mode + /// + /// Report real modes from `stat` and `list` unless your backend is + /// read-only. Absent modes are not a neutral default here; they are read + /// as a statement. + /// + /// Who answers from what today: + /// + /// | Backend | Modes | + /// |---|---| + /// | `LocalFs` | Real OS bits on Unix; synthesized from `Permissions::readonly()` elsewhere (the live path on `wasm32-wasip1`) | + /// | `MemoryFs` | Constants: dir `0o777`, file `0o666`, symlink `0o777` | + /// | `DevFs` | Constants: device `0o666`, the `/dev` directory `0o555` | + /// | `OverlayFs` | Whichever layer holds the path | + /// | `VfsRouter` | The owning mount; `0o555` for directories it synthesizes | + /// | `BuiltinFs`, `JobFs` | **None** — and both are read-only | + /// + /// That last row is load-bearing. `PathAccess::resolve` treats an absent + /// mode as not writable, and that is correct **only** because every + /// backend still reporting `None` is read-only. A writable backend that + /// reports `None` will have every one of its paths called unwritable — + /// `test -w` says no, and the write that follows succeeds anyway. + /// + /// Nothing catches that for you. There is no assertion tying + /// `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. + /// /// Errors exactly as `stat` does: a path that does not exist is an error, /// not a `PathAccess` of all-false. async fn path_access(&self, path: &Path) -> io::Result { From e00269b5913a040c2c4da21b5be1e74ba566f360 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:38:31 -0400 Subject: [PATCH 14/17] vfs: LocalFs answers file tests from the OS, not from mode bits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- Cargo.lock | 1 + crates/kaish-types/src/path_access.rs | 40 ++++++++++++++++++ crates/kaish-vfs/Cargo.toml | 9 ++++ crates/kaish-vfs/src/lib.rs | 2 +- crates/kaish-vfs/src/local.rs | 61 ++++++++++++++++++++++++++- crates/kaish-vfs/src/traits.rs | 2 +- 6 files changed, 112 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 37e492b1..7b0a5b1e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1168,6 +1168,7 @@ dependencies = [ "async-trait", "getrandom 0.3.4", "kaish-types", + "rustix", "tempfile", "tokio", ] diff --git a/crates/kaish-types/src/path_access.rs b/crates/kaish-types/src/path_access.rs index 83b31de7..ed3d0364 100644 --- a/crates/kaish-types/src/path_access.rs +++ b/crates/kaish-types/src/path_access.rs @@ -33,6 +33,25 @@ /// That premise is load-bearing. **A backend that is writable and reports /// `None` will be told its paths are unwritable.** An embedder adding one /// should report a mode rather than rely on a default here. +/// What the operating system says *this process* may do with a path. +/// +/// The answer to `faccessat(..., AT_EACCESS)` — an access check against the +/// effective uid and gid, which is the same primitive `bash`'s `test -w` uses. +/// +/// This is not the same question as the mode bits. `0o222` means "some +/// principal may write"; a root-owned `0o644` file has it set for a process +/// that cannot write a byte. Only the OS knows the process's identity, so +/// only the OS can answer. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct EffectiveAccess { + /// The process may read the path. + pub read: bool, + /// The process may write the path. + pub write: bool, + /// The process may execute the path, or search it if it is a directory. + pub execute: bool, +} + #[non_exhaustive] #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct PathAccess { @@ -77,6 +96,27 @@ impl PathAccess { } } + /// Combine the OS's effective-access answer with a mount's read-only + /// state. + /// + /// For a backend whose paths are real OS paths, this is the accurate + /// constructor and [`PathAccess::resolve`] is not: `resolve` reads mode + /// bits, and mode bits answer "may some principal do this", while a file + /// test has to answer "may this process do this". Only `LocalFs` can use + /// this, because only its paths have an OS identity to check against. + /// + /// `mount_read_only` is still ANDed into `writable`, and still has to be: + /// a `LocalFs::read_only` wrapper is a kaish-level restriction that the + /// OS cannot see, so the kernel granting write does not settle it. Both + /// facts, one funnel — the same contract `resolve` keeps. + pub fn from_effective_access(access: EffectiveAccess, mount_read_only: bool) -> Self { + Self { + readable: access.read, + writable: !mount_read_only && access.write, + executable: access.execute, + } + } + } #[cfg(test)] diff --git a/crates/kaish-vfs/Cargo.toml b/crates/kaish-vfs/Cargo.toml index 3881c506..a9edda90 100644 --- a/crates/kaish-vfs/Cargo.toml +++ b/crates/kaish-vfs/Cargo.toml @@ -24,6 +24,15 @@ getrandom = { workspace = true } # `MemoryFs` needs only `tokio/sync` (no runtime, works on wasm). tokio = { version = "1", features = [], optional = true } +# `LocalFs::path_access` asks the OS whether THIS process can read/write/exec a +# path (faccessat with AT_EACCESS), which mode bits cannot answer. Unix-only: +# wasm32-wasip1 has no effective-uid model and takes the synthesized-mode path. +# rustix rather than libc because `unsafe_code = "deny"` workspace-wide; it is +# already a normal dependency in this tree via rustyline, procfs and +# terminal_size, so this adds no new supply chain. +[target.'cfg(unix)'.dependencies] +rustix = { version = "1.1", features = ["fs"] } + [features] default = [] # The real-filesystem backend. Off → only the `Filesystem` trait + `DirEntry`, diff --git a/crates/kaish-vfs/src/lib.rs b/crates/kaish-vfs/src/lib.rs index 24ab61cf..8094d960 100644 --- a/crates/kaish-vfs/src/lib.rs +++ b/crates/kaish-vfs/src/lib.rs @@ -14,7 +14,7 @@ mod traits; pub use budget::ByteBudget; pub use dev::DevFs; -pub use traits::{DirEntry, DirEntryKind, Filesystem, PathAccess, ReadRange}; +pub use traits::{DirEntry, DirEntryKind, EffectiveAccess, Filesystem, PathAccess, ReadRange}; // `LocalFs` pulls in `tokio/fs`; gated so the in-memory/wasm sandbox build // (which doesn't enable `localfs`) stays free of a real-filesystem dependency. diff --git a/crates/kaish-vfs/src/local.rs b/crates/kaish-vfs/src/local.rs index 47d10476..8adf786b 100644 --- a/crates/kaish-vfs/src/local.rs +++ b/crates/kaish-vfs/src/local.rs @@ -2,7 +2,7 @@ //! //! Provides access to real filesystem paths, with optional read-only mode. -use crate::traits::{DirEntry, DirEntryKind, Filesystem, ReadRange}; +use crate::traits::{DirEntry, DirEntryKind, EffectiveAccess, Filesystem, PathAccess, ReadRange}; use async_trait::async_trait; use std::io; use std::path::{Path, PathBuf}; @@ -222,6 +222,30 @@ impl LocalFs { /// a platform with no Unix mode bits. Split out from the `cfg` so it can /// be tested on every platform, including the Unix ones that never call /// it. + /// Ask the OS whether this process may read, write, and execute `full`. + /// + /// `faccessat(AT_FDCWD, full, ..., AT_EACCESS)` — an access check against + /// the effective uid/gid, the same primitive `bash`'s `test -w` uses. The + /// three questions are asked separately because the kernel answers them + /// separately. + /// + /// rustix rather than `libc` because `unsafe_code` is denied + /// workspace-wide. + #[cfg(unix)] + fn effective_access(full: &Path) -> EffectiveAccess { + use rustix::fs::{Access, AtFlags, accessat}; + use rustix::fs::CWD; + + let ask = |mode: Access| { + accessat(CWD, full, mode, AtFlags::EACCESS).is_ok() + }; + EffectiveAccess { + read: ask(Access::READ_OK), + write: ask(Access::WRITE_OK), + execute: ask(Access::EXEC_OK), + } + } + // Only the non-Unix `extract_permissions` calls this; the tests call it // on every platform, which is the reason it is split out at all. #[cfg_attr(unix, allow(dead_code))] @@ -564,6 +588,41 @@ impl Filesystem for LocalFs { fs::rename(&from_path, &to_path).await } + /// Answers from the OS, not from the mode bits — the one backend that + /// can, and therefore the one backend that does not use + /// [`PathAccess::resolve`]. + /// + /// `resolve` reads `0o222` and friends, which say whether *some* + /// principal may write. That is the wrong question for a real file: a + /// root-owned `0o644` file has the bit set for a kaish running as an + /// ordinary user who cannot write a byte of it, and `test -w` would have + /// said yes. Every other backend in this crate models modes we chose + /// ourselves, where the bits are the whole truth and `resolve` is right; + /// only `LocalFs` has paths with an OS identity to check against, so only + /// `LocalFs` can ask the real question. + /// + /// The read-only wrapper is still ANDed in by `from_effective_access`. + /// The OS cannot know about it — it is a kaish-level restriction over an + /// OS-writable directory — so the kernel granting write does not settle + /// the answer. + /// + /// On non-Unix the OS has no effective-uid model to ask, so this falls + /// through to the trait default over the synthesized mode; see + /// `synthesized_mode`. + #[cfg(unix)] + async fn path_access(&self, path: &Path) -> io::Result { + let full = self.resolve(path)?; + // Stat first so a missing path errors exactly the way `stat` does — + // `faccessat` would report a plain EACCES/ENOENT with no distinction + // the callers can use, and the trait contract is "errors as stat". + let _ = fs::metadata(&full).await?; + let access = + tokio::task::spawn_blocking(move || Self::effective_access(&full)) + .await + .map_err(io::Error::other)?; + Ok(PathAccess::from_effective_access(access, self.read_only)) + } + fn read_only(&self) -> bool { self.read_only } diff --git a/crates/kaish-vfs/src/traits.rs b/crates/kaish-vfs/src/traits.rs index 0aff3f02..8a367a1d 100644 --- a/crates/kaish-vfs/src/traits.rs +++ b/crates/kaish-vfs/src/traits.rs @@ -6,7 +6,7 @@ use std::path::{Path, PathBuf}; use std::time::SystemTime; // DirEntry and DirEntryKind live in kaish-types. -pub use kaish_types::{DirEntry, DirEntryKind, PathAccess, ReadRange}; +pub use kaish_types::{DirEntry, DirEntryKind, EffectiveAccess, PathAccess, ReadRange}; /// Abstract filesystem interface. /// From a1236da4acc1f00376c5f750e0b59e24e34f662b Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:42:59 -0400 Subject: [PATCH 15/17] test: pin effective access, and correct a WASI claim I did not check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 12 +- .../tests/file_test_writable_tests.rs | 104 +++++++++++++++++- crates/kaish-vfs/src/local.rs | 25 +++-- crates/kaish-vfs/src/traits.rs | 2 +- 4 files changed, 120 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c426f339..fdfd72d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,14 +12,10 @@ breaking entries are marked **BREAKING**. ### Fixed -- **`test -w` on a virtual path** — `-w /v/bin/echo` and `-w /v/jobs/1/status` - answered yes about read-only mounts. `MemoryFs` and `DevFs` now report modes, - and `-w` needs the mount and the mode to agree. - -### Changed - -- **`test -x DIR` on a memory-backed directory** — now yes. `x` on a directory - means searchable, which these are; it answered no while modes were absent. +- **File tests on virtual and real paths** — `-w` claimed read-only mounts and + root-owned files were writable, and `-x` denied that a memory-backed + directory is searchable. `-w`/`-r`/`-x` now answer from the owning mount plus + the OS's effective access. ### Added diff --git a/crates/kaish-kernel/tests/file_test_writable_tests.rs b/crates/kaish-kernel/tests/file_test_writable_tests.rs index 3011799a..88da12fe 100644 --- a/crates/kaish-kernel/tests/file_test_writable_tests.rs +++ b/crates/kaish-kernel/tests/file_test_writable_tests.rs @@ -216,12 +216,26 @@ async fn devfs_directory_is_searchable_but_not_writable() { // ── LocalFs: real OS mode bits ───────────────────────────────────────────── -/// A writable LocalFs mount still has to honour the mode bits — an -/// implementation that answered from the mount alone and ignored the stat -/// would call a mode-444 file writable. +/// True when the environment ignores mode restrictions — running as root, or +/// a filesystem/container that bypasses DAC. Detected empirically by trying +/// the thing, not by checking uid, so it covers every cause. +#[cfg(unix)] +fn dac_is_bypassed(dir: &std::path::Path) -> bool { + use std::os::unix::fs::PermissionsExt; + let probe = dir.join(".dac-probe"); + std::fs::write(&probe, b"x").unwrap(); + std::fs::set_permissions(&probe, std::fs::Permissions::from_mode(0o000)).unwrap(); + let bypassed = std::fs::File::open(&probe).is_ok(); + std::fs::set_permissions(&probe, std::fs::Permissions::from_mode(0o644)).unwrap(); + bypassed +} + +/// A writable LocalFs mount still has to honour the file's permissions — an +/// implementation that answered from the mount alone would call a mode-444 +/// file writable. #[cfg(unix)] #[tokio::test] -async fn localfs_mode_bits_still_decide() { +async fn localfs_permissions_still_decide() { use std::os::unix::fs::PermissionsExt; let tmp = tempfile::tempdir().unwrap(); @@ -233,6 +247,11 @@ async fn localfs_mode_bits_still_decide() { ) .unwrap(); + if dac_is_bypassed(tmp.path()) { + eprintln!("skipping: environment bypasses DAC (root?)"); + return; + } + let kernel = kernel_at(tmp.path()); let rw = tmp.path().join("rw.txt"); let ro = tmp.path().join("ro.txt"); @@ -240,6 +259,83 @@ async fn localfs_mode_bits_still_decide() { both_spellings(&kernel, "", "-w", &ro.display().to_string(), false).await; } +/// **The case that was wrong before this change.** `0o222` means "some +/// principal may write", not "this process may write". +/// +/// A file the running user owns at mode `0o022` has the `0o222` mask set — +/// for group and other — while the owner class, which is the class that +/// applies to us, has no write bit. Unix checks the owner class and stops, +/// so the write fails. A mode-bit test says YES; the kernel says NO; the +/// `open()` below proves the kernel is right. +/// +/// This is the root-owned-`/etc/passwd` shape, reproduced without root: a +/// permissive bit that belongs to a principal we are not. `0o044` and `0o011` +/// do the same for `-r` and `-x`. +#[cfg(unix)] +#[tokio::test] +async fn permission_bits_for_another_principal_do_not_grant_access() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().unwrap(); + if dac_is_bypassed(tmp.path()) { + eprintln!("skipping: environment bypasses DAC (root?)"); + return; + } + + // (mode, op, the answer the raw mask would give, the true answer) + let cases = [ + (0o022u32, "-w", "w.txt"), + (0o044u32, "-r", "r.txt"), + (0o011u32, "-x", "x.txt"), + ]; + + let kernel = kernel_at(tmp.path()); + for (mode, op, name) in cases { + let path = tmp.path().join(name); + std::fs::write(&path, b"hi\n").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(mode)).unwrap(); + + // The raw mask a mode-bit implementation would consult IS set. + let mask = match op { + "-w" => 0o222, + "-r" => 0o444, + _ => 0o111, + }; + assert_ne!( + mode & mask, + 0, + "fixture is pointless unless the raw mask is set for {name}", + ); + + both_spellings(&kernel, "", op, &path.display().to_string(), false).await; + + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + } +} + +/// The kernel really does refuse, so the answers above are not a theory. +#[cfg(unix)] +#[tokio::test] +async fn the_other_principal_fixture_really_is_refused() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().unwrap(); + if dac_is_bypassed(tmp.path()) { + eprintln!("skipping: environment bypasses DAC (root?)"); + return; + } + let path = tmp.path().join("w.txt"); + std::fs::write(&path, b"hi\n").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o022)).unwrap(); + + let opened = std::fs::OpenOptions::new().append(true).open(&path); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + assert!( + opened.is_err(), + "mode 0o022 owned by us must refuse a write despite 0o222 being set", + ); +} + /// `-x` on a real path keeps answering from the mode bits. Pinned so a later /// symmetry argument cannot quietly move it. #[cfg(unix)] diff --git a/crates/kaish-vfs/src/local.rs b/crates/kaish-vfs/src/local.rs index 8adf786b..910eaa25 100644 --- a/crates/kaish-vfs/src/local.rs +++ b/crates/kaish-vfs/src/local.rs @@ -198,17 +198,22 @@ impl LocalFs { /// Synthesize a Unix-shaped mode from the one permission fact a non-Unix /// platform exposes. /// - /// This arm is live, not a Windows courtesy: `wasm32-wasip1` is not - /// `unix`, `mod local` is declared unconditionally, and kaish ships and - /// builds that target every CI run. + /// Reached on a non-Unix target that enables `localfs` — Windows in + /// practice. **Not** WASI: `mod local` is gated on `localfs`, `localfs` + /// pulls `tokio/fs`, and wasm rejects that feature, so `wasm32-wasip1` + /// never compiles `LocalFs` at all. (An earlier version of this comment + /// claimed the WASI build depended on this arm. It does not; the claim + /// was checked and removed rather than left to mislead.) /// - /// `LocalFs` is writable, so returning `None` here would put it in the - /// same position `MemoryFs` was in: a writable backend reporting an - /// absent mode, which `PathAccess::resolve` reads as read-only. Every - /// file test on the WASI build would answer "not writable", and the wasi - /// CI leg compiles without running these tests, so nothing would say so. - /// There is exactly one bit to work from — `Permissions::readonly()` — so - /// that is what the mode carries. + /// CI is Linux-only, so nothing here exercises this. That is the reason + /// `synthesized_mode` is split out as a pure function with a test that + /// runs everywhere. + /// + /// `LocalFs` is writable, so returning `None` would put it in the same + /// position `MemoryFs` was in: a writable backend reporting an absent + /// mode, which `PathAccess::resolve` reads as read-only — every file test + /// answering "not writable". There is exactly one bit to work from, + /// `Permissions::readonly()`, so that is what the mode carries. /// /// The `x` bit is never set. Executability is not a permission on these /// platforms (it is decided by the file extension), so claiming it would diff --git a/crates/kaish-vfs/src/traits.rs b/crates/kaish-vfs/src/traits.rs index 8a367a1d..579570f9 100644 --- a/crates/kaish-vfs/src/traits.rs +++ b/crates/kaish-vfs/src/traits.rs @@ -128,7 +128,7 @@ pub trait Filesystem: Send + Sync { /// /// | Backend | Modes | /// |---|---| - /// | `LocalFs` | Real OS bits on Unix; synthesized from `Permissions::readonly()` elsewhere (the live path on `wasm32-wasip1`) | + /// | `LocalFs` | The OS's effective-access answer on Unix (see its `path_access`); synthesized from `Permissions::readonly()` on a non-Unix target that enables `localfs` | /// | `MemoryFs` | Constants: dir `0o777`, file `0o666`, symlink `0o777` | /// | `DevFs` | Constants: device `0o666`, the `/dev` directory `0o555` | /// | `OverlayFs` | Whichever layer holds the path | From 7bed658b78d13af1f36815e09edc03ceb38a65be Mon Sep 17 00:00:00 2001 From: A Tobey Date: Wed, 26 Aug 2026 20:10:14 -0400 Subject: [PATCH 16/17] Cut the path_access comments to the house length 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. --- crates/kaish-kernel/src/vfs/builtin_fs.rs | 26 +++--- crates/kaish-types/src/path_access.rs | 102 ++++++++-------------- crates/kaish-vfs/src/local.rs | 74 +++++----------- crates/kaish-vfs/src/traits.rs | 66 ++++---------- docs/EMBEDDING.md | 34 ++++++++ 5 files changed, 118 insertions(+), 184 deletions(-) diff --git a/crates/kaish-kernel/src/vfs/builtin_fs.rs b/crates/kaish-kernel/src/vfs/builtin_fs.rs index d226cca1..520706b1 100644 --- a/crates/kaish-kernel/src/vfs/builtin_fs.rs +++ b/crates/kaish-kernel/src/vfs/builtin_fs.rs @@ -1,23 +1,17 @@ //! BuiltinFs — read-only VFS that lists builtins as file entries under `/v/bin/`. //! -//! **The entries are not executable, and nothing here ever executes.** This -//! comment used to say "executable entries", which was a claim the code has -//! never made good on: `stat` reports no mode, so `test -x /v/bin/grep` -//! answers NO, and `real_path` returns `None`, so there is no path for -//! exec(2) to open. `read` returns a line that opens with `#!`, which makes -//! the entry *look* executable and is the likeliest reason the claim went -//! unnoticed. +//! The entries are not executable and nothing here ever executes. `stat` +//! reports no mode, so `test -x /v/bin/grep` answers NO, and `real_path` +//! returns `None`, so there is no path for exec(2) to open. `read` returns a +//! line opening with `#!`, which makes an entry look executable. //! -//! Running a builtin goes by name through the `ToolRegistry`; it never routes -//! through this filesystem. `/v/bin` is an inventory an agent can list and -//! read, not a directory of programs. +//! Running a builtin goes by name through the `ToolRegistry` and never routes +//! through this filesystem. `/v/bin` is an inventory to list and read, not a +//! directory of programs. Reporting `0o111` would flip `test -x /v/bin/*` from +//! NO to YES — a behavior change that wants its own decision. //! -//! For the original claim to hold, `stat` and `list` would have to report a -//! mode with the `0o111` bits set. That is a deliberate non-change: it would -//! flip `test -x /v/bin/*` from NO to YES, which is a behavior change that -//! wants its own decision. Note also that `read_only()` below is `true`, and -//! the closed `-w` default depends on that staying true — see -//! `Filesystem::path_access`. +//! `read_only()` is `true`, and the closed `-w` default depends on it staying +//! true — see `Filesystem::path_access`. use std::io; use std::path::{Path, PathBuf}; diff --git a/crates/kaish-types/src/path_access.rs b/crates/kaish-types/src/path_access.rs index ed3d0364..3429b2a6 100644 --- a/crates/kaish-types/src/path_access.rs +++ b/crates/kaish-types/src/path_access.rs @@ -1,47 +1,12 @@ //! What the kernel can do with one path. -/// Whether the kernel can read, write, or execute a path. -/// -/// A file test needs two facts, and neither one answers alone: the read-only -/// state of the mount that owns the path, and the mode bits that mount -/// reports for the path itself. -/// -/// The mode is not enough. A `LocalFs::read_only` wrapper over an -/// OS-writable directory reports real mode bits with the write bit set, -/// because `LocalFs::stat` asks the OS and the OS does not know about the -/// wrapper. Every write to such a path fails; a mode-only check says it -/// would succeed. -/// -/// The mount is not enough either. `DevFs::read_only()` is deliberately -/// `false` — refusing writes would break `> /dev/null` — while -/// `mkdir /dev/x` is refused for every caller. Only the mode separates the -/// writable device from the unwritable directory above it. -/// -/// [`PathAccess::resolve`] takes both and is the only constructor, so no -/// caller can answer from one of them by accident. The struct is -/// `#[non_exhaustive]`: read the fields, do not construct it by literal. -/// -/// # What an absent mode means -/// -/// `DirEntry.permissions` is `None` only for a backend that does not model -/// permissions at all. Every backend in this workspace that can be written -/// reports a mode — `LocalFs` on every platform, `MemoryFs`, `DevFs`, and -/// `OverlayFs` through whichever layer holds the path — so the backends -/// still reporting `None` (`BuiltinFs`, `JobFs`) are read-only ones. An -/// absent mode therefore reads as: readable, not writable, not executable. -/// -/// That premise is load-bearing. **A backend that is writable and reports -/// `None` will be told its paths are unwritable.** An embedder adding one -/// should report a mode rather than rely on a default here. /// What the operating system says *this process* may do with a path. /// /// The answer to `faccessat(..., AT_EACCESS)` — an access check against the -/// effective uid and gid, which is the same primitive `bash`'s `test -w` uses. -/// -/// This is not the same question as the mode bits. `0o222` means "some -/// principal may write"; a root-owned `0o644` file has it set for a process -/// that cannot write a byte. Only the OS knows the process's identity, so -/// only the OS can answer. +/// effective uid and gid, the same primitive `bash`'s `test -w` uses. Mode +/// bits answer a different question: `0o222` means "some principal may +/// write", and a root-owned `0o644` file has it set for a process that +/// cannot write a byte. Only the OS knows the process's identity. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct EffectiveAccess { /// The process may read the path. @@ -52,6 +17,24 @@ pub struct EffectiveAccess { pub execute: bool, } +/// Whether the kernel can read, write, or execute a path. +/// +/// A file test needs two facts and neither answers alone. A +/// `LocalFs::read_only` wrapper over an OS-writable directory reports mode +/// bits with the write bit set, because `LocalFs::stat` asks the OS and the +/// OS does not know about the wrapper. `DevFs::read_only()` is `false` so +/// that `> /dev/null` works, while `mkdir /dev/x` is refused for every +/// caller — only the mode separates those two. +/// +/// [`PathAccess::resolve`] is the only constructor, so no caller answers +/// from one fact by accident. Read the fields; the struct is +/// `#[non_exhaustive]`. +/// +/// An absent `DirEntry.permissions` reads as readable, not writable, not +/// executable. Every writable backend here reports a mode, so `None` means a +/// backend that does not model permissions, and all of those are read-only. +/// **A writable backend reporting `None` will be told its paths are +/// unwritable** — see `docs/EMBEDDING.md`, "Reporting file permissions". #[non_exhaustive] #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct PathAccess { @@ -70,24 +53,14 @@ impl PathAccess { /// Combine a mount's read-only state with the mode bits it reports for /// one path. /// - /// `mode` is `DirEntry.permissions` — `None` when the mount does not - /// model Unix modes. `mount_read_only` is the `read_only()` of the mount - /// that actually owns the path, not of the whole router. - /// - /// The three answers treat an absent mode differently, because the three - /// questions are different: + /// `mode` is `DirEntry.permissions`; `mount_read_only` is the + /// `read_only()` of the mount that owns the path, not of the whole + /// router. /// - /// - `readable` — a backend that does not model permissions does not - /// restrict reads, and read-only mounts read. So: readable. - /// - `writable` — every writable backend reports a mode, so an absent one - /// means read-only. Both facts must agree for a yes: a read-only mount - /// is never writable, and a writable mount still honors a mode that - /// clears `0o222`. - /// - `executable` — an absent mode means there is nothing here to run. - /// Read-only-ness contributes nothing; it is about writes. - /// - /// On a directory, `0o111` means searchable, which is the POSIX meaning - /// and what `test -x DIR` should answer. + /// An absent mode still reads: a backend that does not model permissions + /// does not restrict reads. Writes need both facts to agree. Exec needs a + /// mode — absent means there is nothing here to run. On a directory + /// `0o111` is searchable, which is what `test -x DIR` answers. pub fn resolve(mode: Option, mount_read_only: bool) -> Self { Self { readable: mode.is_none_or(|p| p & 0o444 != 0), @@ -99,16 +72,12 @@ impl PathAccess { /// Combine the OS's effective-access answer with a mount's read-only /// state. /// - /// For a backend whose paths are real OS paths, this is the accurate - /// constructor and [`PathAccess::resolve`] is not: `resolve` reads mode - /// bits, and mode bits answer "may some principal do this", while a file - /// test has to answer "may this process do this". Only `LocalFs` can use - /// this, because only its paths have an OS identity to check against. + /// The accurate constructor for a backend whose paths are real OS paths, + /// which is only `LocalFs`. A file test asks "may this process do this", + /// and [`PathAccess::resolve`]'s mode bits cannot answer that. /// - /// `mount_read_only` is still ANDed into `writable`, and still has to be: - /// a `LocalFs::read_only` wrapper is a kaish-level restriction that the - /// OS cannot see, so the kernel granting write does not settle it. Both - /// facts, one funnel — the same contract `resolve` keeps. + /// `mount_read_only` is still ANDed into `writable`: a `LocalFs::read_only` + /// wrapper is a kaish-level restriction the OS cannot see. pub fn from_effective_access(access: EffectiveAccess, mount_read_only: bool) -> Self { Self { readable: access.read, @@ -116,7 +85,6 @@ impl PathAccess { executable: access.execute, } } - } #[cfg(test)] @@ -170,6 +138,4 @@ mod tests { assert!(access.readable, "read-only is about writes"); assert!(access.executable, "read-only says nothing about exec"); } - - } diff --git a/crates/kaish-vfs/src/local.rs b/crates/kaish-vfs/src/local.rs index 910eaa25..531163c3 100644 --- a/crates/kaish-vfs/src/local.rs +++ b/crates/kaish-vfs/src/local.rs @@ -199,41 +199,23 @@ impl LocalFs { /// platform exposes. /// /// Reached on a non-Unix target that enables `localfs` — Windows in - /// practice. **Not** WASI: `mod local` is gated on `localfs`, `localfs` - /// pulls `tokio/fs`, and wasm rejects that feature, so `wasm32-wasip1` - /// never compiles `LocalFs` at all. (An earlier version of this comment - /// claimed the WASI build depended on this arm. It does not; the claim - /// was checked and removed rather than left to mislead.) + /// practice. Not WASI: `localfs` pulls `tokio/fs`, which wasm rejects, so + /// `wasm32-wasip1` never compiles `LocalFs` at all. /// - /// CI is Linux-only, so nothing here exercises this. That is the reason - /// `synthesized_mode` is split out as a pure function with a test that - /// runs everywhere. - /// - /// `LocalFs` is writable, so returning `None` would put it in the same - /// position `MemoryFs` was in: a writable backend reporting an absent - /// mode, which `PathAccess::resolve` reads as read-only — every file test - /// answering "not writable". There is exactly one bit to work from, - /// `Permissions::readonly()`, so that is what the mode carries. - /// - /// The `x` bit is never set. Executability is not a permission on these - /// platforms (it is decided by the file extension), so claiming it would - /// be a fabrication; `-x` answered false here before this and still does. + /// `LocalFs` is writable, so returning `None` would make every file test + /// answer "not writable" (`PathAccess::resolve` reads an absent mode as + /// read-only). `Permissions::readonly()` is the only bit to work from. + /// The `x` bit is never set: executability is decided by file extension + /// on these platforms, so claiming it would be a fabrication. #[cfg(not(unix))] fn extract_permissions(meta: &std::fs::Metadata) -> Option { Some(Self::synthesized_mode(meta.is_dir(), meta.permissions().readonly())) } - /// The mode [`extract_permissions`](Self::extract_permissions) reports on - /// a platform with no Unix mode bits. Split out from the `cfg` so it can - /// be tested on every platform, including the Unix ones that never call - /// it. /// Ask the OS whether this process may read, write, and execute `full`. /// /// `faccessat(AT_FDCWD, full, ..., AT_EACCESS)` — an access check against - /// the effective uid/gid, the same primitive `bash`'s `test -w` uses. The - /// three questions are asked separately because the kernel answers them - /// separately. - /// + /// the effective uid/gid, the same primitive `bash`'s `test -w` uses. /// rustix rather than `libc` because `unsafe_code` is denied /// workspace-wide. #[cfg(unix)] @@ -251,8 +233,9 @@ impl LocalFs { } } - // Only the non-Unix `extract_permissions` calls this; the tests call it - // on every platform, which is the reason it is split out at all. + /// The mode [`extract_permissions`](Self::extract_permissions) reports on + /// a platform with no Unix mode bits. Split out from the `cfg` so it can + /// be tested everywhere, including the Unix targets that never call it. #[cfg_attr(unix, allow(dead_code))] pub(crate) fn synthesized_mode(is_dir: bool, readonly: bool) -> u32 { match (is_dir, readonly) { @@ -593,27 +576,17 @@ impl Filesystem for LocalFs { fs::rename(&from_path, &to_path).await } - /// Answers from the OS, not from the mode bits — the one backend that - /// can, and therefore the one backend that does not use - /// [`PathAccess::resolve`]. - /// - /// `resolve` reads `0o222` and friends, which say whether *some* - /// principal may write. That is the wrong question for a real file: a - /// root-owned `0o644` file has the bit set for a kaish running as an - /// ordinary user who cannot write a byte of it, and `test -w` would have - /// said yes. Every other backend in this crate models modes we chose - /// ourselves, where the bits are the whole truth and `resolve` is right; - /// only `LocalFs` has paths with an OS identity to check against, so only - /// `LocalFs` can ask the real question. + /// Answers from the OS, not from mode bits — the one backend that can, + /// and so the one backend that does not use [`PathAccess::resolve`]. /// - /// The read-only wrapper is still ANDed in by `from_effective_access`. - /// The OS cannot know about it — it is a kaish-level restriction over an - /// OS-writable directory — so the kernel granting write does not settle - /// the answer. + /// Mode bits say whether *some* principal may write. A root-owned `0o644` + /// file has the bit set for a kaish running as an ordinary user who cannot + /// write a byte, and `test -w` would have said yes. Every other backend + /// models modes we chose ourselves, where the bits are the whole truth. /// - /// On non-Unix the OS has no effective-uid model to ask, so this falls - /// through to the trait default over the synthesized mode; see - /// `synthesized_mode`. + /// `from_effective_access` still ANDs in the read-only wrapper, which the + /// OS cannot see. On non-Unix this falls through to the trait default over + /// [`synthesized_mode`](Self::synthesized_mode). #[cfg(unix)] async fn path_access(&self, path: &Path) -> io::Result { let full = self.resolve(path)?; @@ -798,10 +771,9 @@ mod tests { cleanup(&dir).await; } - // The non-Unix mode synthesis, exercised on every platform. Without it - // `LocalFs` would be a writable backend reporting an absent mode on - // Windows — the same hole MemoryFs had — and `test -w` would answer NO - // about every file there. + // Without this, `LocalFs` on Windows would be a writable backend + // reporting an absent mode, and `test -w` would answer NO about every + // file there. #[test] fn synthesized_mode_keeps_writability_and_never_claims_exec() { assert_eq!(LocalFs::synthesized_mode(false, false) & 0o222, 0o222); diff --git a/crates/kaish-vfs/src/traits.rs b/crates/kaish-vfs/src/traits.rs index 579570f9..8842fed3 100644 --- a/crates/kaish-vfs/src/traits.rs +++ b/crates/kaish-vfs/src/traits.rs @@ -96,56 +96,24 @@ pub trait Filesystem: Send + Sync { /// What the kernel can do with one path on this filesystem. /// - /// This is the query behind `test -w`, `test -r`, and `test -x`. It - /// exists because neither [`Filesystem::read_only`] nor - /// `DirEntry.permissions` answers "can this path be written" on its own: - /// `MemoryFs` (writable) and `JobFs` (read-only) both report + /// The query behind `test -r`, `test -w`, and `test -x`. Neither + /// [`Filesystem::read_only`] nor `DirEntry.permissions` answers on its + /// own — `MemoryFs` (writable) and `JobFs` (read-only) both report /// `permissions: None`, and a `LocalFs::read_only` wrapper over an - /// OS-writable directory reports mode bits with the write bit set. - /// [`PathAccess::resolve`] is the only place the two combine. - /// - /// The default asks `stat` for the mode and this filesystem for the - /// read-only state, which is right for any filesystem that is uniformly - /// read-only or uniformly writable. `VfsRouter` overrides it to ask the - /// mount that owns the path. - /// - /// `OverlayFs` keeps the default, and inherits one known inaccuracy from - /// it: reads resolve against whichever layer holds the path, but writes - /// always land in the upper and `OverlayFs::write` never consults the - /// lower's mode. So a lower file whose mode clears `0o222` reports - /// unwritable while copy-up would in fact write it. That answer is - /// unchanged from before this query existed, and correcting it means - /// deciding what mode a path that does not exist in the upper yet should - /// be judged by — a question with no answer in the code today. - /// - /// # If you are adding a backend, report a mode - /// - /// Report real modes from `stat` and `list` unless your backend is - /// read-only. Absent modes are not a neutral default here; they are read - /// as a statement. - /// - /// Who answers from what today: - /// - /// | Backend | Modes | - /// |---|---| - /// | `LocalFs` | The OS's effective-access answer on Unix (see its `path_access`); synthesized from `Permissions::readonly()` on a non-Unix target that enables `localfs` | - /// | `MemoryFs` | Constants: dir `0o777`, file `0o666`, symlink `0o777` | - /// | `DevFs` | Constants: device `0o666`, the `/dev` directory `0o555` | - /// | `OverlayFs` | Whichever layer holds the path | - /// | `VfsRouter` | The owning mount; `0o555` for directories it synthesizes | - /// | `BuiltinFs`, `JobFs` | **None** — and both are read-only | - /// - /// That last row is load-bearing. `PathAccess::resolve` treats an absent - /// mode as not writable, and that is correct **only** because every - /// backend still reporting `None` is read-only. A writable backend that - /// reports `None` will have every one of its paths called unwritable — - /// `test -w` says no, and the write that follows succeeds anyway. - /// - /// Nothing catches that for you. There is no assertion tying - /// `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. + /// OS-writable directory reports the write bit set. [`PathAccess::resolve`] + /// is where the two combine. + /// + /// The default is right for a filesystem that is uniformly read-only or + /// uniformly writable. `VfsRouter` overrides it to ask the mount that owns + /// the path. + /// + /// A backend that can be written must report a mode; an absent one is read + /// as read-only, and nothing checks that for you. See `docs/EMBEDDING.md`, + /// "Reporting file permissions". + /// + /// `OverlayFs` keeps the default and inherits its one inaccuracy: writes + /// always land in the upper, so a lower file whose mode clears `0o222` + /// reports unwritable while copy-up would write it. /// /// Errors exactly as `stat` does: a path that does not exist is an error, /// not a `PathAccess` of all-false. diff --git a/docs/EMBEDDING.md b/docs/EMBEDDING.md index 70a4daea..a95a89b9 100644 --- a/docs/EMBEDDING.md +++ b/docs/EMBEDDING.md @@ -371,6 +371,40 @@ vfs.mount("/", MemoryFs::with_budget(budget.clone())); // budget.used() / budget.remaining() are observable at any time. ``` +### Reporting file permissions (`path_access`) + +`test -r`, `test -w`, and `test -x` ask the mount that owns the path, through +`Filesystem::path_access`. The default implementation combines two facts — +`DirEntry.permissions` from `stat`, and the mount's own `read_only()` — in +`PathAccess::resolve`. `VfsRouter` overrides it to route the question to the +owning mount; `LocalFs` overrides it to ask the OS for an effective-access +answer, because mode bits say "some principal may write" where a file test has +to answer "may this process write". + +Report real modes from `stat` and `list` unless your backend is read-only. An +absent mode is not a neutral default here — it is read as a statement. Who +answers from what today: + +| Backend | Modes | +|---|---| +| `LocalFs` | The OS's effective-access answer on Unix; synthesized from `Permissions::readonly()` on a non-Unix target | +| `MemoryFs` | Constants: dir `0o777`, file `0o666`, symlink `0o777` | +| `DevFs` | Constants: device `0o666`, the `/dev` directory `0o555` | +| `OverlayFs` | Whichever layer holds the path | +| `VfsRouter` | The owning mount; `0o555` for directories it synthesizes | +| `BuiltinFs`, `JobFs` | None — and both are read-only | + +That last row is load-bearing. `PathAccess::resolve` treats an absent mode as +not writable, and that is correct only because every backend still reporting +`None` is read-only. **A writable backend that reports `None` will have every +one of its paths called unwritable** — `test -w` says no, and the write that +follows succeeds anyway. + +Nothing catches that for you. No assertion ties `read_only() == false` to +reporting a mode, and the failure is a wrong answer rather than an error, so +your backend's own tests will pass. If you add a writable backend, either +report a mode or change `resolve` and this table together. + ### Output Limits and Spill Mode (`OutputLimitConfig`) `KernelConfig::output_limit` caps how much a single command's output can grow From 3e3b5a5170886014ab4895079983e649023d3d5b Mon Sep 17 00:00:00 2001 From: A Tobey Date: Wed, 26 Aug 2026 20:15:33 -0400 Subject: [PATCH 17/17] Link a private item by name, not by intra-doc link 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. --- crates/kaish-vfs/src/local.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/kaish-vfs/src/local.rs b/crates/kaish-vfs/src/local.rs index 531163c3..110d32ea 100644 --- a/crates/kaish-vfs/src/local.rs +++ b/crates/kaish-vfs/src/local.rs @@ -586,7 +586,7 @@ impl Filesystem for LocalFs { /// /// `from_effective_access` still ANDs in the read-only wrapper, which the /// OS cannot see. On non-Unix this falls through to the trait default over - /// [`synthesized_mode`](Self::synthesized_mode). + /// `synthesized_mode`. #[cfg(unix)] async fn path_access(&self, path: &Path) -> io::Result { let full = self.resolve(path)?;