From 85e4ee03bbc7820ff7fde60a9183ab2710ba592c Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 31 Jul 2026 18:48:07 -0400 Subject: [PATCH] feat(bootstrap): make the ambient storage roots a parameter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `EditorState::new` resolves two storage roots from the process environment before it returns: the data root, which the bundled-package materialization then WRITES into unconditionally (outside every `cfg` guard), and the config root, from which `init.lua` is read. The `#[cfg(not(test))]` guard on the second was written to stop the crate's own unit tests picking up a developer's real `init.lua`. It does exactly that and nothing more: `cfg(test)` is set only while compiling the lib's own tests, so an integration test in `tests/` — compiled without it — reads the real config and writes the real data root. On a machine with a real `~/.config/pmacs/init.lua`, that is 11 deterministic failures in `compile_mode_acceptance`, attributed to whatever branch is checked out. Tests cannot fix that themselves: `std::env::set_var` is `unsafe` and this crate is `#![forbid(unsafe_code)]` — the same constraint that produced `Installer::with_install_root_override`. So isolation arrives as a parameter. `BootstrapRoots` names the four storage roots (config, data, state, cache). `ambient()` leaves every one `None` and every resolution goes to the environment exactly as today, so production is unchanged. `new_with_roots` and `open_with_roots` take it — both, because `open` calls `Self::new()` internally and a constructor-only parameter would leave every open-path test ambient. `install_state_dirs` consults it too: it runs after construction, so resolving from the environment there would reopen the hole the constructor closed. The redirected branch changes WHICH directory is read, never WHETHER the block runs. Config loading shares one conditional with `set_init_complete()`, and `tests/m8_2_acceptance.rs:75` documents its dependence on integration-test construction finishing init-complete. `child_env()` translates the same value into the environment a spawned `pmacs` needs. Five variables, not four: `PMACS_STATE_HOME` outranks `XDG_STATE_HOME`, so a child given only the XDG four still resolves an inherited state override — invisible on a machine that exports none. The `src/editor.rs` comment claimed a protection it does not provide and said nothing about the write above it; both are corrected in place. The guard is deliberately NOT widened to cover integration tests. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- src/bootstrap.rs | 391 +++++++++++++++++++++++++++++++++++++++++++++++ src/editor.rs | 123 ++++++++++++++- src/lib.rs | 1 + 3 files changed, 509 insertions(+), 6 deletions(-) create mode 100644 src/bootstrap.rs diff --git a/src/bootstrap.rs b/src/bootstrap.rs new file mode 100644 index 0000000..87857e2 --- /dev/null +++ b/src/bootstrap.rs @@ -0,0 +1,391 @@ +// src/bootstrap.rs --- explicit bootstrap storage roots. + +//! Where pmacs stores things at startup, as a value rather than as an +//! ambient property of the process environment. +//! +//! # Why this exists +//! +//! `EditorState::new` resolves two storage roots from the environment +//! before it returns: +//! +//! * the **data** root, which +//! [`crate::builtin_packages::bundled_runtime_dir`] resolves from +//! `XDG_DATA_HOME` (else `$HOME/.local/share`) and which +//! [`crate::builtin_packages::materialize_all`] then **writes into** +//! --- unconditionally, outside any `cfg` guard; and +//! * the **config** root, which [`crate::config::user_config_dir`] +//! resolves from `XDG_CONFIG_HOME` (else `$HOME/.config`) and from +//! which `init.lua` is read. +//! +//! An integration test in `tests/` links this crate as an ordinary +//! dependency, so it is compiled **without** `cfg(test)`: the +//! `#[cfg(not(test))]` guard around config loading is inactive for +//! every one of them. They read the developer's real `init.lua` and +//! write into the developer's real data root. +//! +//! They cannot fix that themselves. `std::env::set_var` has been +//! `unsafe` since Rust 2024 and this crate is `#![forbid(unsafe_code)]` +//! --- the same constraint that produced +//! [`crate::packages::installer::Installer::with_install_root_override`] +//! and [`crate::lua_bindings::PackageInstallOverride`]. So isolation has +//! to arrive as a **parameter**, which is what this type is. +//! +//! # Contract +//! +//! [`BootstrapRoots::ambient()`] is production: every root stays `None` +//! and every resolution goes to the environment exactly as before. A +//! root that is `Some` replaces the environment lookup for that root and +//! **only** that root. +//! +//! # Scope: storage roots only +//! +//! This type covers the four roots that decide where pmacs *stores* +//! things: config, data, state and cache. It deliberately does not +//! cover: +//! +//! * **`HOME`'s non-storage semantics.** `expand_tilde` +//! ([`crate::editor_core`]) resolves a leading `~` for ordinary path +//! entry, and `tests/find_file_acceptance.rs` consumes `HOME` on +//! purpose to pin that expansion. Redirecting a storage root is the +//! right fix for a storage root and the wrong fix for a +//! path-expansion root. +//! * **`XDG_RUNTIME_DIR`**, which addresses sockets rather than stored +//! data. + +use std::path::{Path, PathBuf}; + +/// The bootstrap storage roots an [`crate::editor::EditorState`] is +/// constructed against. +/// +/// Each field is the *base* directory --- the value `XDG__HOME` +/// would hold --- not the `pmacs/` subdirectory under it. `None` means +/// "resolve from the environment", which is what production does. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct BootstrapRoots { + config: Option, + data: Option, + state: Option, + cache: Option, +} + +/// The version-keyed leaf `bundled_runtime_dir` materializes into. +fn bundled_leaf() -> String { + format!("v{}", env!("CARGO_PKG_VERSION")) +} + +impl BootstrapRoots { + /// Production: every root resolves from the process environment. + #[must_use] + pub fn ambient() -> Self { + Self::default() + } + + /// Every storage root redirected under `base`, into sibling + /// `config/`, `data/`, `state/` and `cache/` directories. + /// + /// The layout mirrors the XDG one so a caller that also spawns a + /// child process can hand the same four paths to `XDG_CONFIG_HOME`, + /// `XDG_DATA_HOME`, `XDG_STATE_HOME` and `XDG_CACHE_HOME` (plus + /// `PMACS_STATE_HOME`, which outranks `XDG_STATE_HOME`) and get the + /// same tree from an in-process and a spawned editor. + #[must_use] + pub fn isolated_under(base: &Path) -> Self { + Self { + config: Some(base.join("config")), + data: Some(base.join("data")), + state: Some(base.join("state")), + cache: Some(base.join("cache")), + } + } + + /// Builder: redirect the config root (the `XDG_CONFIG_HOME` value). + #[must_use] + pub fn with_config_root(mut self, root: PathBuf) -> Self { + self.config = Some(root); + self + } + + /// Builder: redirect the data root (the `XDG_DATA_HOME` value). + #[must_use] + pub fn with_data_root(mut self, root: PathBuf) -> Self { + self.data = Some(root); + self + } + + /// Builder: redirect the state root (the `XDG_STATE_HOME` value). + #[must_use] + pub fn with_state_root(mut self, root: PathBuf) -> Self { + self.state = Some(root); + self + } + + /// Builder: redirect the cache root (the `XDG_CACHE_HOME` value). + #[must_use] + pub fn with_cache_root(mut self, root: PathBuf) -> Self { + self.cache = Some(root); + self + } + + /// True when nothing is redirected --- i.e. this is production's + /// [`Self::ambient`]. + #[must_use] + pub fn is_ambient(&self) -> bool { + self.config.is_none() && self.data.is_none() && self.state.is_none() && self.cache.is_none() + } + + /// The config *base*, if redirected. + #[must_use] + pub fn config_root(&self) -> Option<&Path> { + self.config.as_deref() + } + + /// The data *base*, if redirected. + #[must_use] + pub fn data_root(&self) -> Option<&Path> { + self.data.as_deref() + } + + /// The state *base*, if redirected. + #[must_use] + pub fn state_root(&self) -> Option<&Path> { + self.state.as_deref() + } + + /// The cache *base*, if redirected. + #[must_use] + pub fn cache_root(&self) -> Option<&Path> { + self.cache.as_deref() + } + + /// The directory `init.lua` is read from --- `/pmacs`, the + /// same shape [`crate::config::user_config_dir`] builds. + #[must_use] + pub fn config_dir(&self) -> Option { + self.config.as_ref().map(|p| p.join("pmacs")) + } + + /// Where bundled packages are materialized --- + /// `/pmacs/builtin-packages/v`, the same shape + /// [`crate::builtin_packages::bundled_runtime_dir`] builds. + #[must_use] + pub fn bundled_runtime_dir(&self) -> Option { + self.data.as_ref().map(|p| { + p.join("pmacs") + .join("builtin-packages") + .join(bundled_leaf()) + }) + } + + /// The user-scope package install root --- `/pmacs/packages`. + #[must_use] + pub fn package_install_root(&self) -> Option { + self.data.as_ref().map(|p| p.join("pmacs").join("packages")) + } + + /// The package fetcher's bare-mirror cache --- `/pmacs/git`. + #[must_use] + pub fn package_cache_dir(&self) -> Option { + self.cache.as_ref().map(|p| p.join("pmacs").join("git")) + } + + /// The editor state directory --- `/pmacs`, the same shape + /// [`crate::state::user_state_dir`] builds. + #[must_use] + pub fn state_dir(&self) -> Option { + self.state.as_ref().map(|p| p.join("pmacs")) + } + + /// The minibuffer history directory --- `/pmacs/history`. + #[must_use] + pub fn history_dir(&self) -> Option { + self.state_dir().map(|d| d.join("history")) + } + + /// The environment a **child** `pmacs` process must be given so it + /// resolves the same roots this value names. + /// + /// An in-process caller passes the value; a caller that spawns + /// `pmacs --daemon` (or re-execs a test binary) cannot, and has to + /// go through the environment instead. This is that translation, so + /// the two paths cannot drift. + /// + /// **Five variables, not four.** `PMACS_STATE_HOME` outranks + /// `XDG_STATE_HOME` ([`crate::state::user_state_dir`]), so a child + /// given only the four XDG variables still resolves the *inherited* + /// `PMACS_STATE_HOME` if the launching environment exports one --- a + /// hole that is invisible on a machine that does not. + /// + /// A root left ambient emits no variable, so the child inherits it. + #[must_use] + pub fn child_env(&self) -> Vec<(&'static str, PathBuf)> { + let mut out = Vec::with_capacity(5); + if let Some(p) = &self.config { + out.push(("XDG_CONFIG_HOME", p.clone())); + } + if let Some(p) = &self.data { + out.push(("XDG_DATA_HOME", p.clone())); + } + if let Some(p) = &self.state { + out.push(("XDG_STATE_HOME", p.clone())); + out.push(("PMACS_STATE_HOME", p.clone())); + } + if let Some(p) = &self.cache { + out.push(("XDG_CACHE_HOME", p.clone())); + } + out + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn ambient_redirects_nothing() { + let roots = BootstrapRoots::ambient(); + assert!(roots.is_ambient()); + assert_eq!(roots.config_dir(), None); + assert_eq!(roots.bundled_runtime_dir(), None); + assert_eq!(roots.state_dir(), None); + assert_eq!(roots.package_cache_dir(), None); + } + + #[test] + fn isolated_under_mirrors_the_xdg_layout() { + let roots = BootstrapRoots::isolated_under(Path::new("/scratch")); + assert!(!roots.is_ambient()); + assert_eq!( + roots.config_dir().unwrap(), + Path::new("/scratch/config/pmacs") + ); + assert_eq!( + roots.bundled_runtime_dir().unwrap(), + Path::new("/scratch/data/pmacs/builtin-packages").join(bundled_leaf()) + ); + assert_eq!( + roots.package_install_root().unwrap(), + Path::new("/scratch/data/pmacs/packages") + ); + assert_eq!( + roots.package_cache_dir().unwrap(), + Path::new("/scratch/cache/pmacs/git") + ); + assert_eq!( + roots.state_dir().unwrap(), + Path::new("/scratch/state/pmacs") + ); + assert_eq!( + roots.history_dir().unwrap(), + Path::new("/scratch/state/pmacs/history") + ); + } + + /// A builder that sets one root leaves the other three ambient --- + /// the "only that root" half of the contract. + #[test] + fn a_single_builder_leaves_the_other_roots_ambient() { + let roots = BootstrapRoots::ambient().with_config_root(PathBuf::from("/only/config")); + assert!(!roots.is_ambient()); + assert_eq!(roots.config_dir().unwrap(), Path::new("/only/config/pmacs")); + assert_eq!(roots.bundled_runtime_dir(), None); + assert_eq!(roots.state_dir(), None); + assert_eq!(roots.package_cache_dir(), None); + } + + /// The five-variable contract, asserted as content: naming only the + /// four XDG variables leaves `PMACS_STATE_HOME` --- which outranks + /// `XDG_STATE_HOME` --- pointing wherever the launching environment + /// left it. + #[test] + fn child_env_names_all_five_storage_variables() { + let roots = BootstrapRoots::isolated_under(Path::new("/scratch")); + let env = roots.child_env(); + let names: Vec<&str> = env.iter().map(|(k, _)| *k).collect(); + assert_eq!( + names, + vec![ + "XDG_CONFIG_HOME", + "XDG_DATA_HOME", + "XDG_STATE_HOME", + "PMACS_STATE_HOME", + "XDG_CACHE_HOME", + ] + ); + let value = |name: &str| { + env.iter() + .find(|(k, _)| *k == name) + .map(|(_, v)| v.clone()) + .unwrap() + }; + assert_eq!(value("XDG_CONFIG_HOME"), Path::new("/scratch/config")); + assert_eq!(value("XDG_DATA_HOME"), Path::new("/scratch/data")); + assert_eq!(value("XDG_STATE_HOME"), Path::new("/scratch/state")); + assert_eq!(value("PMACS_STATE_HOME"), Path::new("/scratch/state")); + assert_eq!(value("XDG_CACHE_HOME"), Path::new("/scratch/cache")); + } + + /// An ambient root emits no variable — the child inherits it. A + /// blanket five-variable emission would silently redirect roots the + /// caller deliberately left alone. + #[test] + fn child_env_emits_nothing_for_ambient_roots() { + assert!(BootstrapRoots::ambient().child_env().is_empty()); + let only_config = BootstrapRoots::ambient().with_config_root(PathBuf::from("/only/config")); + assert_eq!( + only_config.child_env(), + vec![("XDG_CONFIG_HOME", PathBuf::from("/only/config"))] + ); + } + + /// `child_env` and the in-process resolvers must describe the same + /// tree: the spawned daemon and the in-process editor are both + /// supposed to land in the isolated roots, and a mismatch would let + /// one of them escape while the other looked fine. + #[test] + fn child_env_agrees_with_the_in_process_resolvers() { + let roots = BootstrapRoots::isolated_under(Path::new("/scratch")); + let env = roots.child_env(); + let value = |name: &str| { + env.iter() + .find(|(k, _)| *k == name) + .map(|(_, v)| v.clone()) + .unwrap() + }; + assert_eq!( + roots.config_dir().unwrap(), + value("XDG_CONFIG_HOME").join("pmacs") + ); + assert_eq!( + roots.package_install_root().unwrap(), + value("XDG_DATA_HOME").join("pmacs").join("packages") + ); + assert_eq!( + roots.state_dir().unwrap(), + value("PMACS_STATE_HOME").join("pmacs") + ); + assert_eq!( + roots.package_cache_dir().unwrap(), + value("XDG_CACHE_HOME").join("pmacs").join("git") + ); + } + + /// The isolated bundled dir must agree with the ambient resolver's + /// shape, version leaf included: a mismatch would make an isolated + /// editor materialize somewhere production never looks. + #[test] + fn bundled_leaf_matches_the_ambient_resolver() { + let roots = BootstrapRoots::isolated_under(Path::new("/scratch")); + let isolated = roots.bundled_runtime_dir().unwrap(); + // `bundled_runtime_dir()` reads the live environment, so compare + // only the tail that does not depend on it. + let ambient = crate::builtin_packages::bundled_runtime_dir(); + let tail = |p: &Path| { + p.components() + .rev() + .take(3) + .map(|c| c.as_os_str().to_string_lossy().into_owned()) + .collect::>() + }; + assert_eq!(tail(&isolated), tail(&ambient)); + } +} diff --git a/src/editor.rs b/src/editor.rs index b089973..bf897b4 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -273,6 +273,14 @@ pub struct EditorState { /// steal or clear another's in-flight gesture, and concurrent drags /// are perfectly legal. window_drag: HashMap, + /// The bootstrap storage roots this session was constructed + /// against (`docs/test-ambient-config-isolation-framing.md`). + /// + /// [`BootstrapRoots::ambient`] in production. Retained past + /// construction because [`Self::install_state_dirs`] resolves a root + /// too and runs *after* the constructor --- resolving it from the + /// environment there would reopen the hole the constructor closed. + bootstrap_roots: crate::bootstrap::BootstrapRoots, } #[derive(Default)] @@ -345,6 +353,24 @@ impl EditorState { /// Panics only if Lua initialization or the builtin command/keymap /// chunks fail to load --- both indicate broken builds. #[must_use] + pub fn new() -> Self { + Self::new_with_roots(&crate::bootstrap::BootstrapRoots::ambient()) + } + + /// Construct a fresh editor against explicit bootstrap storage + /// roots (`docs/test-ambient-config-isolation-framing.md` §1.4). + /// + /// [`Self::new`] is this with [`BootstrapRoots::ambient`], so + /// production behaviour is unchanged. An integration test passes + /// redirected roots instead: it links this crate without + /// `cfg(test)`, so the guard below is live for it, and without a + /// parameter it would read the developer's real `init.lua` and + /// write bundled packages into the developer's real data root. + /// `std::env::set_var` is not an alternative --- it is `unsafe` and + /// this crate is `#![forbid(unsafe_code)]`. + /// + /// [`BootstrapRoots::ambient`]: crate::bootstrap::BootstrapRoots::ambient + #[must_use] #[allow( clippy::too_many_lines, reason = "linear bootstrap sequence: registry → core → LuaHost → \ @@ -352,7 +378,7 @@ impl EditorState { Splitting into helpers fragments the wiring without removing \ any single decision the reader needs to follow." )] - pub fn new() -> Self { + pub fn new_with_roots(roots: &crate::bootstrap::BootstrapRoots) -> Self { // Build the buffer registry first so EditorCore and LuaHost // share the same `Rc`. Both reach buffers through this handle; // multi-window dispatch (T M2.8) requires that ids resolve to @@ -738,9 +764,34 @@ impl EditorState { // Depends on `pmacs.buffer.add_intercept` (T M6.4 Stage 1) // and `pmacs.ansi.parser()` (T M6.4 Stage 2), both available // by the time `attach_editor` returns above. - let bundled_root = crate::builtin_packages::bundled_runtime_dir(); + // + // `materialize_all` CREATES DIRECTORIES AND WRITES FILES, and + // it is outside every `cfg` guard — so this, not config + // loading, is the write half of the ambient-roots exposure + // (`docs/test-ambient-config-isolation-framing.md` §1.6). The + // redirected root is consulted first for exactly that reason. + let bundled_root = roots + .bundled_runtime_dir() + .unwrap_or_else(crate::builtin_packages::bundled_runtime_dir); let bundled_packages = crate::builtin_packages::materialize_all(&bundled_root) .expect("materialize bundled packages"); + // Redirected roots also redirect where `pmacs.packages.install` + // would later fetch and install, so a caller that isolated its + // storage cannot reach the real `$XDG_CACHE_HOME/pmacs/git` or + // `$XDG_DATA_HOME/pmacs/packages` through Lua either. Installed + // before user config runs, so an `init.lua` that installs a + // package lands inside the isolated tree. Production leaves the + // slot empty (ambient roots take this branch not at all). + if !roots.is_ambient() { + let mut override_ = crate::lua_bindings::PackageInstallOverride::new(); + if let Some(cache) = roots.package_cache_dir() { + override_ = override_.with_cache_dir(cache); + } + if let Some(install) = roots.package_install_root() { + override_ = override_.with_user_install_root(install); + } + lua_host.set_package_install_override(override_); + } { let slot = lua_host .lua() @@ -772,17 +823,43 @@ impl EditorState { // to exercise config loading do so explicitly via // [`crate::config::load_user_config_at`]. // + // **`cfg(test)` covers the lib's unit tests and NOTHING ELSE.** + // An integration test in `tests/` links this crate as an + // ordinary dependency, compiled without `cfg(test)`, so this + // block is fully live for all ~96 of them: `cargo test --lib` is + // protected, `cargo test --test ` is not. That is what the + // `roots` parameter is for — an integration test redirects the + // config root instead of relying on a guard that does not reach + // it. (Nor is this the only ambient root: the bundled-package + // materialization above runs unconditionally and WRITES.) See + // `docs/test-ambient-config-isolation-framing.md` §1.2, §1.6. + // + // The guard is deliberately NOT widened to cover integration + // tests: production deciding it is under test is how a suite + // passes against behaviour production never runs (Q#TI1). + // // The init-complete flip happens here too so lifecycle-gated // Lua APIs (e.g. `pmacs.attach`, M5.6d+) become inert after // user config returns. Tests that need post-init semantics flip // the flag explicitly via [`crate::lua::LuaHost::set_init_complete`]; // see the option-(A) discussion in the M5.6c survey. + // + // Redirected roots change *which directory* is read, never + // *whether* the block runs: `tests/m8_2_acceptance.rs:75` + // documents its dependence on integration-test construction + // finishing init-complete, so skipping the block for isolated + // construction would leave every such test permanently in the + // init phase (framing §1.8). #[cfg(not(test))] { - crate::config::load_user_config(&mut lua_host); + match roots.config_dir() { + Some(dir) => crate::config::load_user_config_at(&mut lua_host, &dir), + None => crate::config::load_user_config(&mut lua_host), + } lua_host.set_init_complete(); } Self { + bootstrap_roots: roots.clone(), core, lua_host, dispatchers: HashMap::new(), @@ -879,10 +956,24 @@ impl EditorState { /// `~/.local/state/pmacs`. Honors the `PMACS_STATE_HOME` override /// (see [`crate::state::user_state_dir`]). pub fn install_state_dirs(&self) { - if let Some(dir) = crate::minibuffer::user_history_dir() { + // Redirected roots win over the environment here too. This runs + // after construction, so resolving from the environment would + // hand an isolated session the developer's real state dir --- and + // `PMACS_STATE_HOME` outranks `XDG_STATE_HOME`, so an + // environment-side fix would have to cover five variables, not + // four (framing §1.6a). + let history = self + .bootstrap_roots + .history_dir() + .or_else(crate::minibuffer::user_history_dir); + if let Some(dir) = history { self.core.borrow_mut().minibuffer.history_dir = Some(dir); } - if let Some(dir) = crate::state::user_state_dir() { + let state = self + .bootstrap_roots + .state_dir() + .or_else(crate::state::user_state_dir); + if let Some(dir) = state { self.lua_host .lua() .set_app_data(crate::lua_bindings::StateDir(dir)); @@ -1029,7 +1120,27 @@ impl EditorState { behavioral gain" )] pub fn open(path: PathBuf) -> io::Result { - let mut state = Self::new(); + Self::open_with_roots(path, &crate::bootstrap::BootstrapRoots::ambient()) + } + + /// [`Self::open`] against explicit bootstrap storage roots. + /// + /// `open` calls `Self::new()` internally, so a roots parameter on + /// the constructor alone leaves every open-path test ambient + /// (`docs/test-ambient-config-isolation-framing.md` §1.7). The two + /// entry points therefore gain the parameter together, and `open` + /// stays a thin ambient caller of this — the golden-journey ratchet + /// requires that exact public entry point to have a production + /// caller shape. + #[allow( + clippy::needless_pass_by_value, + reason = "mirrors `open`'s stable signature; see the note there" + )] + pub fn open_with_roots( + path: PathBuf, + roots: &crate::bootstrap::BootstrapRoots, + ) -> io::Result { + let mut state = Self::new_with_roots(roots); let resolved = state .core .borrow_mut() diff --git a/src/lib.rs b/src/lib.rs index e3971a3..43c2a3b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -46,6 +46,7 @@ pub mod attach_dispatch; pub mod attach_reconnect; pub mod audit; pub mod autosave; +pub mod bootstrap; pub mod buffer; pub mod buffer_registry; pub mod builtin_packages;