From 8c1f7a08344c024f3b720910e02c68c4dabf5e12 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Tue, 1 Sep 2026 10:16:13 -0400 Subject: [PATCH] fix(vfs): the ancestor synthesis was unreachable whenever `/` was mounted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `readlink -f` failing on a rooted mount was one caller meeting a router bug. `ls`, `stat`, `cd` and the file tests fail the same way, on the same paths: ls /…/project -> not found [[ -d /…/project ]] -> false where `/…/project/fixture` is a mount and something else covers `/`. The router already knew the answer. `stat`, `lstat`, `list` and `path_access` each carry an ancestor branch that synthesizes a directory for a path above a mount, because a mount at `/a/b/c` implies `/a/b` is a directory the same way a real mount implies its mount point's parents exist. That branch sat on the `Err` arm of `find_mount`. Mounting `/` makes `mount_of` match every path, so `find_mount` always succeeds, the backend covering `/` is asked for `/a`, and its `NotFound` returns to the caller before the ancestor branch can run. The code was correct and unreachable in the one configuration every embedder uses -- a root mount plus a deeper one. The check now runs on the answer rather than on the routing. Only `NotFound` is recovered: any other error is the backend's answer about a path it owns and reaches the caller unchanged, so this cannot mask a permission failure. This supersedes the ancestor half of readlink's own fix. Against this commit alone, 7 of that branch's 8 tests pass with none of its `is_structural` / `owning_mount` code present; the 8th is containment, which is a separate guarantee and stays there. A control pins the boundary: a path that is not an ancestor of any mount is still absent, so synthesizing ancestors did not make the namespace exist. --- CHANGELOG.md | 4 + crates/kaish-kernel/src/vfs/router.rs | 105 ++++++++----- .../tests/router_mount_ancestor_tests.rs | 145 ++++++++++++++++++ 3 files changed, 217 insertions(+), 37 deletions(-) create mode 100644 crates/kaish-kernel/tests/router_mount_ancestor_tests.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ae2b357b..57dc3a9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,10 @@ breaking entries are marked **BREAKING**. second surface cannot drift from the first by omission. ### Fixed +- **A mount point's ancestors are navigable again** — with a backend at `/` and + another mounted deeper, `stat`, `ls`, `cd`, and the file tests answered "not + found" for the directories above the deeper mount. They synthesize as + directories, as they already did when no backend covered `/`. - **`help ` and `kaish-tools ` only rendered one level of subcommands** (`kaish-tools` rendered none), hiding a nested verb (`worktree list`) and its flags. Both now recurse to any depth, one flat diff --git a/crates/kaish-kernel/src/vfs/router.rs b/crates/kaish-kernel/src/vfs/router.rs index 0711f33e..b792c2da 100644 --- a/crates/kaish-kernel/src/vfs/router.rs +++ b/crates/kaish-kernel/src/vfs/router.rs @@ -142,6 +142,34 @@ impl VfsRouter { /// (`/v` over mounts `/v/jobs`, `/v/blobs` → `blobs`, `jobs`). `dir` is /// expected to be a non-root ancestor with no mount of its own; root is /// handled by `list_root`, which also folds in a `/` mount's real contents. + /// Recover a mount point's ancestor from a `NotFound`. + /// + /// A mount at `/a/b/c` implies `/a` and `/a/b` are directories, the way a + /// real mount implies its mount point's parents. The router synthesizes + /// them because no backend owns them. + /// + /// Mounting `/` makes `mount_of` match every path, so the backend covering + /// `/` is asked for `/a`, answers `NotFound`, and that reaches the caller + /// before the ancestor check below the `Err` arm can run. That check is + /// therefore unreachable whenever a root mount exists, which is the + /// ordinary embedder shape. This runs on the answer instead of on the + /// routing. + /// + /// Only `NotFound` is recovered. Any other error is the backend's answer + /// about a path it owns and must reach the caller unchanged. + fn or_synthesized_ancestor( + &self, + path: &Path, + error: io::Error, + synthesize: impl FnOnce() -> T, + ) -> io::Result { + if error.kind() == io::ErrorKind::NotFound && self.has_mount_under(path) { + Ok(synthesize()) + } else { + Err(error) + } + } + fn list_mount_children(&self, dir: &Path) -> Vec { let dir = Self::normalize_mount_path(dir.to_path_buf()); let prefix = format!("{}/", dir.to_string_lossy()); @@ -327,17 +355,15 @@ impl Filesystem for VfsRouter { return self.list_root().await; } - match self.find_mount(path) { + let answer = match self.find_mount(path) { Ok((fs, relative)) => fs.list(&relative).await, - // Not covered by a mount, but an ancestor of one (e.g. `/v` above - // `/v/jobs`): synthesize its child mount directories rather than 404. - Err(e) => { - if self.has_mount_under(path) { - Ok(self.list_mount_children(path)) - } else { - Err(e) - } - } + Err(e) => Err(e), + }; + match answer { + Ok(entries) => Ok(entries), + // An ancestor of a mount lists the mounts beneath it rather + // than 404ing. + Err(e) => self.or_synthesized_ancestor(path, e, || self.list_mount_children(path)), } } @@ -359,16 +385,18 @@ impl Filesystem for VfsRouter { return Ok(DirEntry::directory(name)); } - match self.find_mount(path) { + let answer = match self.find_mount(path) { Ok((fs, relative)) => fs.stat(&relative).await, - // Intermediate ancestor of a mount (e.g. `/v` above `/v/jobs`) - // exists as a synthesized directory. + Err(e) => Err(e), + }; + match answer { + Ok(entry) => Ok(entry), + // An ancestor of a mount (`/v` above `/v/jobs`) is a synthesized + // directory, whether the routing missed or the backend did. Err(e) => { - if self.has_mount_under(path) { - Ok(DirEntry::directory(Self::path_basename(path))) - } else { - Err(e) - } + self.or_synthesized_ancestor(path, e, || { + DirEntry::directory(Self::path_basename(path)) + }) } } } @@ -425,16 +453,18 @@ impl Filesystem for VfsRouter { return Ok(DirEntry::directory(name)); } - match self.find_mount(path) { + let answer = match self.find_mount(path) { Ok((fs, relative)) => fs.lstat(&relative).await, - // Intermediate ancestor of a mount (e.g. `/v` above `/v/jobs`) - // exists as a synthesized directory. + Err(e) => Err(e), + }; + match answer { + Ok(entry) => Ok(entry), + // A synthesized ancestor is a directory, never a symlink, so + // lstat and stat agree about it. Err(e) => { - if self.has_mount_under(path) { - Ok(DirEntry::directory(Self::path_basename(path))) - } else { - Err(e) - } + self.or_synthesized_ancestor(path, e, || { + DirEntry::directory(Self::path_basename(path)) + }) } } } @@ -481,18 +511,19 @@ impl Filesystem for VfsRouter { /// 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 { - match self.find_mount(path) { + let path_str = path.to_string_lossy(); + if path_str.is_empty() || path_str == "/" { + return Ok(PathAccess::resolve(Some(SYNTHESIZED_DIRECTORY_MODE), true)); + } + let answer = 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) - } - } + Err(e) => Err(e), + }; + match answer { + Ok(access) => Ok(access), + Err(e) => self.or_synthesized_ancestor(path, e, || { + PathAccess::resolve(Some(SYNTHESIZED_DIRECTORY_MODE), true) + }), } } diff --git a/crates/kaish-kernel/tests/router_mount_ancestor_tests.rs b/crates/kaish-kernel/tests/router_mount_ancestor_tests.rs new file mode 100644 index 00000000..7c4947bd --- /dev/null +++ b/crates/kaish-kernel/tests/router_mount_ancestor_tests.rs @@ -0,0 +1,145 @@ +//! A mount point's ancestors must be navigable. +//! +//! `VfsRouter::mount_of` picks the longest matching mount, and `/` matches +//! everything. When a backend is mounted several components below `/` — the +//! common embedder shape (`kaibo`, `kaijutsu`) — the components ABOVE that +//! mount point are owned by whichever filesystem covers `/`, which has no +//! entry for them. The router answers for a path it does not own, and the +//! answer is "No such file or directory". +//! +//! In a real filesystem a mount point's ancestors necessarily exist: you +//! cannot mount at `/a/b/c` unless `/a/b` is a directory. The router +//! synthesizes the mount point itself; it must synthesize the path to it too. + +// Test-fixture code: unwrap/expect on known-good setup is the idiom here. +#![allow(clippy::unwrap_used, clippy::expect_used)] +#![cfg(all(feature = "localfs", unix))] + +use std::path::{Path, PathBuf}; +use std::sync::Arc; + +use kaish_kernel::vfs::{LocalFs, MemoryFs, VfsRouter}; +use kaish_kernel::{Kernel, KernelBackend, KernelConfig, LocalBackend}; + +fn tempdir() -> tempfile::TempDir { + tempfile::Builder::new() + .prefix("router-ancestor-") + .tempdir_in(env!("CARGO_TARGET_TMPDIR")) + .expect("tempdir under CARGO_TARGET_TMPDIR") +} + +/// The mount root, several components below `/`, mirroring its own host path. +fn fixture_root(base: &tempfile::TempDir) -> PathBuf { + let root = base.path().join("project").join("fixture"); + std::fs::create_dir_all(&root).expect("mkdir project/fixture"); + std::fs::write(root.join("top.txt"), "top\n").expect("write top.txt"); + root +} + +fn rooted_kernel(root: &Path) -> Kernel { + let mut vfs = VfsRouter::new(); + vfs.mount(root.to_path_buf(), LocalFs::read_only(root.to_path_buf())); + vfs.mount("/", MemoryFs::new()); + let backend: Arc = Arc::new(LocalBackend::new(Arc::new(vfs))); + let config = KernelConfig::isolated().with_cwd(root.to_path_buf()); + Kernel::with_backend(backend, config, |_| {}, |_| {}).expect("with_backend kernel") +} + +async fn run(kernel: &Kernel, line: &str) -> (String, String, i64) { + let result = kernel.execute(line).await.expect("execute"); + ( + result.text_out().trim_end().to_string(), + result.err.trim_end().to_string(), + result.code, + ) +} + +/// Every strict ancestor of the mount point, from `/` down to its parent. +fn ancestors_of(root: &Path) -> Vec { + let mut out = Vec::new(); + let mut current = root.parent(); + while let Some(path) = current { + out.push(path.to_string_lossy().into_owned()); + current = path.parent(); + } + out.reverse(); + out +} + +#[tokio::test] +async fn stat_answers_for_every_ancestor_of_a_mount_point() { + let base = tempdir(); + let root = fixture_root(&base); + let kernel = rooted_kernel(&root); + + for ancestor in ancestors_of(&root) { + let (_out, err, code) = run(&kernel, &format!("stat {ancestor}")).await; + assert_eq!( + code, 0, + "stat must answer for the mount ancestor {ancestor}: {err}" + ); + } +} + +#[tokio::test] +async fn ls_lists_an_ancestor_of_a_mount_point() { + let base = tempdir(); + let root = fixture_root(&base); + let kernel = rooted_kernel(&root); + + let parent = root.parent().expect("mount root has a parent"); + let (out, err, code) = run(&kernel, &format!("ls {}", parent.display())).await; + assert_eq!(code, 0, "ls must list a mount ancestor: {err}"); + let leaf = root.file_name().expect("mount root has a name").to_string_lossy(); + assert!( + out.contains(leaf.as_ref()), + "listing a mount's parent must show the mount itself, got: {out:?}" + ); +} + +#[tokio::test] +async fn cd_into_an_ancestor_of_a_mount_point_succeeds() { + let base = tempdir(); + let root = fixture_root(&base); + let kernel = rooted_kernel(&root); + + let parent = root.parent().expect("mount root has a parent"); + let (_out, err, code) = run(&kernel, &format!("cd {}", parent.display())).await; + assert_eq!(code, 0, "cd into a mount ancestor must succeed: {err}"); +} + +#[tokio::test] +async fn a_file_test_sees_an_ancestor_as_a_directory() { + let base = tempdir(); + let root = fixture_root(&base); + let kernel = rooted_kernel(&root); + + let parent = root.parent().expect("mount root has a parent"); + let (out, err, code) = run( + &kernel, + &format!("if [[ -d {} ]]; then echo IS_DIR; fi", parent.display()), + ) + .await; + assert_eq!(code, 0, "the file test must not error: {err}"); + assert_eq!(out, "IS_DIR", "a mount ancestor must test as a directory"); +} + +/// Control: a path that is NOT an ancestor of any mount is still absent. +/// Synthesizing ancestors must not make the whole namespace exist. +#[tokio::test] +async fn an_unrelated_missing_path_is_still_missing() { + let base = tempdir(); + let root = fixture_root(&base); + let kernel = rooted_kernel(&root); + + let (_out, _err, code) = run(&kernel, "stat /definitely/not/here").await; + assert_ne!(code, 0, "an unrelated missing path must still be missing"); + + let (out, _err, code) = run( + &kernel, + "if [[ -d /definitely/not/here ]]; then echo IS_DIR; else echo ABSENT; fi", + ) + .await; + assert_eq!(code, 0); + assert_eq!(out, "ABSENT", "a non-ancestor must not be synthesized"); +}