diff --git a/docs/developers-guide.md b/docs/developers-guide.md index a646f5fe7..8a19b747a 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -2327,6 +2327,86 @@ platform-path form through `resolve_ninja_program`, which itself calls the UTF-8 resolver and converts its result, so no production path constructs a platform `PathBuf` independently of `resolve_ninja_program_utf8_with`. +#### `which` environment capture + +`EnvSnapshot::capture` (`stdlib::which::env`) reads `PATH` on every +platform, and `PATHEXT` on Windows only, through an injected +`mockable::Env` provider rather than straight from the process: + +- `capture` is the production entry point. It delegates to `capture_with_env` + with `mockable::DefaultEnv`, so it is the single site that binds the + resolver's lookups to the live process environment. +- `capture_with_env` takes `&impl mockable::Env`, so tests drive the whole + capture with a `MockEnv` without mutating process-global state. +- An optional `path_override` parameter shadows `PATH` while leaving `PATHEXT` + to the provider. `capture_with_pathext` additionally shadows `PATHEXT`; it is + defined on every platform so the resolver has one capture entry point, and + the override is accepted and discarded off Windows, where nothing consults + the extension list. +- `capture_common` owns the shared working-directory and `PATH` handling, so + the platform-specific `capture_impl` variants differ only in how they obtain + `PATHEXT`. + +Keep the ambient read at that boundary. Adding a `std::env` call elsewhere in +`env.rs` would put it back where no test can reach it, and the module is where +the clippy `disallowed-methods` gate would then fire. + +Both overrides reach the snapshot from configuration rather than from the +process: `StdlibConfig::with_path_override` and +`StdlibConfig::with_pathext_override` are copied into `WhichConfig`, which +`WhichResolver::new` consumes whole — the resolver takes the configuration +rather than its fields so a new environment seam does not lengthen the +signature again. Pinning both is what lets a behavioural test drive `which` +and `command_available` over a temporary directory with a chosen extension +list; see `tests/stdlib_which_pathext_tests.rs`, which is gated to Windows +because `PATHEXT` governs resolution only there. + +That gating has a cost worth stating: CI runs `make test` on `ubuntu-latest` +only, so a `#[cfg(windows)]` test does not gate a merge. Keep host-independent +rules — normalization, the fallback — in the `#[cfg(any(windows, test))]` unit +tests that the Linux suite executes, and reserve the Windows-gated suite for +behaviour that genuinely cannot run elsewhere. + +#### `PATHEXT` normalization + +`stdlib::which::env::parse_pathext` turns a raw `PATHEXT` value into lowercase, +dot-prefixed extensions. It is pure string handling, consulted only by the +Windows snapshot, and compiled under `#[cfg(any(windows, test))]`. + +Ownership and permitted call sites: + +- Owned by `stdlib::which::env` and `pub(super)`. The Windows + `EnvSnapshot::capture_impl` is its only production caller. +- `DEFAULT_PATHEXT` is the single source of the built-in fallback and shares + the same gating. + +Composition rules: + +- Gate platform-only pure logic `#[cfg(any(windows, test))]` rather than + `#[cfg(windows)]`. The latter hides it from the CI host, so its rules go + unverified *and* unlinted. Compiling it unconditionally would instead leave + it dead in a Unix release build, which `-D warnings` rejects. +- A value yielding no usable extension falls back to the built-in list. An + empty result would mean Windows treats nothing as executable, so `which` + would report every command missing. + +The full normalization contract, which the property tests in +`src/stdlib/which/pathext_tests.rs` pin: + +- **Split on `;`.** That is the `PATHEXT` separator on Windows, and unlike + `PATH` it is not the platform path-list separator, so `split_paths` is the + wrong tool here. +- **Trim whitespace** from each segment, then discard the segment if nothing + remains. `".COM; .EXE"` and `".COM;.EXE"` are the same list. +- **Lowercase, then dot-prefix.** Comparison is case-insensitive, and a + segment written without its dot (`COM`) means the same extension as `.com`. +- **First occurrence wins.** De-duplication is by the *normalized* form, so + `.EXE;.exe` yields one entry, positioned where the first appeared. Order is + significant: it is the order `which` tries extensions in. +- **Fall back when nothing usable remains**, including for an absent value — + `parse_pathext(None)` and `parse_pathext(Some("; ;"))` both yield + `DEFAULT_PATHEXT`. + ### Configuration discovery module layout `src/cli/discovery.rs` attaches several small `#[path = "..."]` modules that diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 6cef64f6a..f018f7f48 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -1244,15 +1244,36 @@ Semantics honour platform conventions while enforcing predictable behaviour: - Canonicalization happens after discovery and only when requested so that manifests can balance reproducibility against host-specific absolute paths. +The resolver reads `PATH` and `PATHEXT` through an injected `mockable::Env` +provider rather than straight from the process. `EnvSnapshot::capture` is the +production entry point and binds `mockable::DefaultEnv`; `capture_with_env` +takes the provider explicitly so tests drive a whole capture with a `MockEnv`; +and `capture_with_pathext` additionally shadows `PATHEXT`. That last helper is +defined on every platform — off Windows the override is accepted and discarded, +because nothing there consults the extension list — so the resolver keeps a +single capture entry point instead of forking on the target. + +Both overrides arrive as configuration rather than as ambient state. +`StdlibConfig::with_path_override` and `StdlibConfig::with_pathext_override` +are copied into `WhichConfig`, which `WhichResolver::new` consumes whole; the +resolver takes the configuration rather than its individual fields so that +adding a further environment seam does not lengthen the constructor again. +Pinning both is what allows a behavioural test to drive `which` and +`command_available` over a temporary directory with a chosen extension list +without mutating the process environment. + The resolver keeps a small LRU cache keyed by the command, a fingerprint of -`PATH`/`PATHEXT`, the working directory, and the cache-relevant options (`all`, -`canonical`, `cwd_mode`). Entries are validated once at insertion; cache reads -no longer re-probe executability, keeping the hot path lean. Because `fresh` -only controls bypass behaviour, it is stripped from the cache key so fresh -lookups still repopulate the cache for subsequent calls. The fingerprint means -environment changes invalidate keys without cloning large strings, and the -helper remains pure because all inputs still derive from the manifest or -process environment. Callers can request a bypass with `fresh=true` when they +`PATH`/`PATHEXT`, the working directory, the captured `NETSUKE_WHICH_WORKSPACE` +state, and the cache-relevant options (`all`, `canonical`, `cwd_mode`). +Including the workspace switch keeps a fallback hit cached while the search was +enabled from answering a resolution made with it disabled. Entries are +validated once at insertion; cache reads no longer re-probe executability, +keeping the hot path lean. Because `fresh` only controls bypass behaviour, it is +stripped from the cache key so fresh lookups still repopulate the cache for +subsequent calls. The fingerprint means environment changes invalidate keys +without cloning large strings, and the helper remains pure because all inputs +still derive from the manifest, the stdlib configuration, or the captured +environment. Callers can request a bypass with `fresh=true` when they need to observe recent toolchain changes during a long session. Cache capacity defaults to 64 entries, covering typical PATH sizes without @@ -1330,8 +1351,8 @@ sequenceDiagram participant "SearchWorkspace" as "search_workspace()" "Caller"->>"WhichResolver": "resolve(command, options)" - "WhichResolver"->>"EnvSnapshot": "capture(cwd_override)" - "EnvSnapshot"-->>"WhichResolver": "EnvSnapshot { cwd, raw_path }" + "WhichResolver"->>"EnvSnapshot": "capture_with_pathext(cwd_override, path_override, pathext_override)" + "EnvSnapshot"-->>"WhichResolver": "EnvSnapshot { cwd, raw_path, raw_pathext }" "WhichResolver"->>"Lookup": "lookup(env, command, options)" "Lookup"->>"Lookup": "search PATH directories for matches" alt "matches found" @@ -1380,6 +1401,8 @@ classDiagram +workspace_root_path() -> OptionalPath +workspace_skip_dirs() -> StringList +which_cache_capacity() -> NonZeroUsize + +with_path_override(path: OsString) -> StdlibConfig + +with_pathext_override(pathext: OsString) -> StdlibConfig } class Environment { @@ -1393,15 +1416,30 @@ classDiagram class WhichResolver { -cache: LruCache -cwd_override: OptionalPath + -path_override: OptionalOsString + -pathext_override: OptionalOsString -workspace_skips: WorkspaceSkipList - +new(cwd_override: OptionalPath, skips: WorkspaceSkipList, cache_capacity: NonZeroUsize) -> Result + +new(config: WhichConfig) -> WhichResolver +resolve(command: String, options: WhichOptions) -> Result } class EnvSnapshot { +cwd: Utf8PathBuf +raw_path: OptionalString - +capture(cwd_override: OptionalPath) -> Result + +raw_pathext: OptionalOsString + +capture(cwd: OptionalPath, path: OptionalOsStr) -> Result + +capture_with_env(cwd: OptionalPath, path: OptionalOsStr, env: Env) -> Result + +capture_with_pathext(cwd: OptionalPath, path: OptionalOsStr, pathext: OptionalOsStr) -> Result + } + + class Env { + <> + +os_string(key: String) -> OptionalOsString + +raw(key: String) -> Result + } + + class DefaultEnv { + +os_string(key: String) -> OptionalOsString } class WhichOptions { @@ -1412,7 +1450,11 @@ classDiagram } class WhichConfig { - +new(cwd_override: OptionalPath, skips: WorkspaceSkipList, cache_capacity: NonZeroUsize) -> WhichConfig + +cwd_override: OptionalPath + +path_override: OptionalOsString + +pathext_override: OptionalOsString + +new(cwd: OptionalPath, path: OptionalOsString, skips: WorkspaceSkipList, capacity: NonZeroUsize) -> WhichConfig + +with_pathext_override(pathext: OptionalOsString) -> WhichConfig } class WorkspaceSkipList { @@ -1428,9 +1470,12 @@ classDiagram Environment --> StdlibConfig : uses Environment --> WhichModule : calls register + StdlibConfig --> WhichConfig : copies PATH and PATHEXT overrides StdlibConfig --> WhichModule : provides workspace_root_path, skip dirs, cache capacity - WhichModule --> WhichResolver : constructs via new(cwd_override, skips, cache_capacity) - WhichResolver --> EnvSnapshot : calls capture(cwd_override) + WhichModule --> WhichResolver : constructs via new(config) + WhichResolver --> EnvSnapshot : calls capture_with_pathext(cwd, path, pathext) + EnvSnapshot --> Env : reads PATH and PATHEXT through the provider + DefaultEnv ..|> Env : production adapter bound by capture WhichResolver --> WhichOptions : reads lookup options WhichResolver --> WorkspaceSkipList : reads traversal filters WhichOptions --> CwdMode : uses cwd_mode @@ -1465,14 +1510,17 @@ sequenceDiagram participant WhichResolver participant Cache participant EnvSnapshot + participant Env as "mockable::Env (DefaultEnv in production)" participant Lookup participant Workspace Caller->>WhichResolver: resolve(command, options) activate WhichResolver - WhichResolver->>EnvSnapshot: capture(cwd_override) + WhichResolver->>EnvSnapshot: capture_with_pathext(cwd_override, path_override, pathext_override) activate EnvSnapshot + EnvSnapshot->>Env: os_string("PATH"), os_string("PATHEXT") + Env-->>EnvSnapshot: values (overrides shadow the provider) EnvSnapshot-->>WhichResolver: env snapshot deactivate EnvSnapshot diff --git a/docs/users-guide.md b/docs/users-guide.md index f4377b992..190d4d0f2 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -406,6 +406,24 @@ defaults: `which(name, **kwargs)` returns an executable path and fails when the command is absent. The same helper is also available as a filter. +On Windows, a name without an extension is matched against the effective +`PATHEXT`, the same list the shell uses — so `which('cargo')` finds +`cargo.exe` provided `.exe` is among those entries. A custom `PATHEXT` may +legitimately omit it, in which case it is not a candidate. + +`PATHEXT` falls back to the built-in list only when it is unset or when no +entry survives normalization — that is, every entry is empty or whitespace. +Any other value is used as given, however unusual. The built-in list, in +order: + +`.com`, `.exe`, `.bat`, `.cmd`, `.vbs`, `.vbe`, `.js`, `.jse`, `.wsf`, +`.wsh`, `.msc` + +The fallback exists because an empty effective list would match nothing and +report every command missing. Entries are matched case-insensitively and +tried in the order the list gives them. A name that already carries an +extension is used as written. + `command_available(name, **kwargs)` returns a boolean and is better for complementary branches: diff --git a/proptest-regressions/stdlib/which/pathext_tests.txt b/proptest-regressions/stdlib/which/pathext_tests.txt new file mode 100644 index 000000000..d668705b0 --- /dev/null +++ b/proptest-regressions/stdlib/which/pathext_tests.txt @@ -0,0 +1,9 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 5575495465cd729dcdfb4a8097c53fc11ed0abcc6bd6cc9bb553ed4ffaf65149 # shrinks to stems = ["a"] +cc 115b011e0b401ec9fd6308a474e89994205af1eccea6f5b567a3e6d999aa2fa4 # shrinks to raw = "A" +cc 54be5d538c9ce0e4e4dc6596fc4ec236b4f66e7f9facf5bbd2b653493cca7137 # shrinks to raw = "EXE;exe" diff --git a/src/stdlib/config/mod.rs b/src/stdlib/config/mod.rs index c4b961f7e..08525f346 100644 --- a/src/stdlib/config/mod.rs +++ b/src/stdlib/config/mod.rs @@ -1,6 +1,7 @@ //! Configuration types and defaults for wiring the stdlib into `MiniJinja`. mod ambient; +mod which; use super::config_types::HomeDirectory; pub use super::config_types::{ @@ -13,7 +14,6 @@ use crate::localization::{self, keys}; use anyhow::{anyhow, bail, ensure}; use camino::{Utf8Component, Utf8Path, Utf8PathBuf}; use cap_std::fs_utf8::Dir; -use indexmap::IndexSet; use std::{ffi::OsString, num::NonZeroUsize, sync::Arc}; /// Configuration for registering Netsuke's standard library helpers. @@ -29,6 +29,7 @@ pub struct StdlibConfig { which_cache_capacity: NonZeroUsize, workspace_skip_dirs: Vec, path_override: Option, + pathext_override: Option, command_path_override: Option, home_directory: HomeDirectory, } @@ -73,6 +74,7 @@ impl StdlibConfig { .map(|dir| (*dir).to_owned()) .collect(), path_override: None, + pathext_override: None, command_path_override: None, home_directory: HomeDirectory::Ambient, }) @@ -164,88 +166,6 @@ impl StdlibConfig { Ok(self) } - /// Override the cache capacity for the `which` resolver. - /// - /// # Errors - /// - /// Returns an error when `capacity` is zero. - /// - /// # Examples - /// - /// ``` - /// # use cap_std::{ambient_authority, fs_utf8::Dir}; - /// # use netsuke::stdlib::StdlibConfig; - /// let dir = Dir::open_ambient_dir(".", ambient_authority()) - /// .expect("open ambient workspace"); - /// let _config = StdlibConfig::new(dir) - /// .expect("construct stdlib config") - /// .with_which_cache_capacity(128) - /// .expect("set which cache capacity"); - /// // Config can now be passed to stdlib registration with a larger cache. - /// ``` - pub fn with_which_cache_capacity(mut self, capacity: usize) -> anyhow::Result { - let non_zero_capacity = NonZeroUsize::new(capacity).ok_or_else(|| { - anyhow!( - "{}", - localization::message(keys::STDLIB_WHICH_CACHE_CAPACITY_POSITIVE) - ) - })?; - self.which_cache_capacity = non_zero_capacity; - Ok(self) - } - /// Override the workspace directories skipped by the `which` fallback - /// search to avoid expensive scans. - /// - /// # Errors - /// - /// Returns an error when any entry is empty, navigates (for example `..`), - /// or contains path separators, because skip entries operate on directory - /// basenames. - pub fn with_workspace_skip_dirs(mut self, dirs: I) -> anyhow::Result - where - I: IntoIterator, - S: AsRef, - { - let mut validated = IndexSet::new(); - for dir in dirs { - let candidate = dir.as_ref().trim(); - ensure!( - !candidate.is_empty(), - "{}", - localization::message(keys::STDLIB_SKIP_DIR_EMPTY) - ); - ensure!( - !matches!(candidate, "." | ".."), - "{}", - localization::message(keys::STDLIB_SKIP_DIR_NAVIGATION) - ); - ensure!( - !candidate.contains(['/', '\\']), - "{}", - localization::message(keys::STDLIB_SKIP_DIR_SEPARATOR) - ); - validated.insert(candidate.to_owned()); - } - self.workspace_skip_dirs = validated.into_iter().collect(); - Ok(self) - } - - /// Override the `PATH` environment variable for `which` lookups. - /// - /// When set, the stdlib will use the provided path string instead of - /// reading `PATH` from the process environment. This allows test isolation - /// without mutating global state. - #[must_use] - pub fn with_path_override(mut self, path: impl Into) -> Self { - self.path_override = Some(path.into()); - self - } - - /// Return the configured PATH override, if any. - pub(crate) const fn path_override(&self) -> Option<&OsString> { - self.path_override.as_ref() - } - /// Override the `PATH` supplied to child processes run by command filters. /// /// This seam is intended for callers that need deterministic command @@ -318,12 +238,6 @@ impl StdlibConfig { &self.fetch_cache_relative } - /// Directories skipped during `which` workspace fallback scans. - #[must_use] - pub fn workspace_skip_dirs(&self) -> &[String] { - &self.workspace_skip_dirs - } - /// Consume the configuration and expose component modules with owned state. pub(crate) fn into_components(self) -> (NetworkConfig, command::CommandConfig) { let Self { @@ -389,10 +303,6 @@ impl StdlibConfig { pub(crate) fn workspace_root_path(&self) -> Option<&Utf8Path> { self.workspace_root_path.as_deref() } - - pub(crate) const fn which_cache_capacity(&self) -> NonZeroUsize { - self.which_cache_capacity - } } #[cfg(test)] diff --git a/src/stdlib/config/which.rs b/src/stdlib/config/which.rs new file mode 100644 index 000000000..7070fbf8f --- /dev/null +++ b/src/stdlib/config/which.rs @@ -0,0 +1,145 @@ +//! `which` resolver configuration on [`StdlibConfig`]. +//! +//! The builders and accessors governing executable resolution live together +//! here — cache capacity, workspace skip list, and the `PATH`/`PATHEXT` +//! overrides that let a caller pin the whole search without touching the +//! process environment. Grouping them by feature keeps `config/mod.rs` to the +//! shared configuration surface rather than one module per layer. + +use std::{ffi::OsString, num::NonZeroUsize}; + +use anyhow::{anyhow, ensure}; +use indexmap::IndexSet; + +use super::StdlibConfig; +use crate::localization::{self, keys}; + +impl StdlibConfig { + /// Override the cache capacity for the `which` resolver. + /// + /// # Errors + /// + /// Returns an error when `capacity` is zero. + /// + /// # Examples + /// + /// ``` + /// # use cap_std::{ambient_authority, fs_utf8::Dir}; + /// # use netsuke::stdlib::StdlibConfig; + /// let dir = Dir::open_ambient_dir(".", ambient_authority()) + /// .expect("open ambient workspace"); + /// let _config = StdlibConfig::new(dir) + /// .expect("construct stdlib config") + /// .with_which_cache_capacity(128) + /// .expect("set which cache capacity"); + /// // Config can now be passed to stdlib registration with a larger cache. + /// ``` + pub fn with_which_cache_capacity(mut self, capacity: usize) -> anyhow::Result { + let non_zero_capacity = NonZeroUsize::new(capacity).ok_or_else(|| { + anyhow!( + "{}", + localization::message(keys::STDLIB_WHICH_CACHE_CAPACITY_POSITIVE) + ) + })?; + self.which_cache_capacity = non_zero_capacity; + Ok(self) + } + /// Override the workspace directories skipped by the `which` fallback + /// search to avoid expensive scans. + /// + /// # Errors + /// + /// Returns an error when any entry is empty, navigates (for example `..`), + /// or contains path separators, because skip entries operate on directory + /// basenames. + pub fn with_workspace_skip_dirs(mut self, dirs: I) -> anyhow::Result + where + I: IntoIterator, + S: AsRef, + { + let mut validated = IndexSet::new(); + for dir in dirs { + let candidate = dir.as_ref().trim(); + ensure!( + !candidate.is_empty(), + "{}", + localization::message(keys::STDLIB_SKIP_DIR_EMPTY) + ); + ensure!( + !matches!(candidate, "." | ".."), + "{}", + localization::message(keys::STDLIB_SKIP_DIR_NAVIGATION) + ); + ensure!( + !candidate.contains(['/', '\\']), + "{}", + localization::message(keys::STDLIB_SKIP_DIR_SEPARATOR) + ); + validated.insert(candidate.to_owned()); + } + self.workspace_skip_dirs = validated.into_iter().collect(); + Ok(self) + } + + /// Override the `PATH` environment variable for `which` lookups. + /// + /// When set, the stdlib will use the provided path string instead of + /// reading `PATH` from the process environment. This allows test isolation + /// without mutating global state. + #[must_use] + pub fn with_path_override(mut self, path: impl Into) -> Self { + self.path_override = Some(path.into()); + self + } + + /// Return the configured PATH override, if any. + pub(crate) const fn path_override(&self) -> Option<&OsString> { + self.path_override.as_ref() + } + + /// Override the `PATHEXT` environment variable for `which` lookups. + /// + /// The counterpart to [`Self::with_path_override`] for the second variable + /// Windows executable resolution depends upon, so a caller can pin the + /// whole search — directories *and* extensions — without touching the + /// process environment. `PATHEXT` is meaningless elsewhere, so the + /// override is accepted and ignored off Windows. + /// + /// An empty or whitespace-only value is not an empty extension list: it + /// yields the built-in fallback, because a genuinely empty list would + /// match nothing. + /// + /// # Examples + /// + /// ```rust,no_run + /// use minijinja::Environment; + /// use netsuke::stdlib::{self, StdlibConfig}; + /// let config = StdlibConfig::from_current_dir() + /// .expect("open workspace") + /// .with_pathext_override(".com;.exe"); + /// let mut env = Environment::new(); + /// stdlib::register_with_config(&mut env, config).expect("register stdlib"); + /// // On Windows `which('cargo')` now considers only `.com` and `.exe`; + /// // a `cargo.bat` would no longer be a candidate. + /// ``` + #[must_use] + pub fn with_pathext_override(mut self, pathext: impl Into) -> Self { + self.pathext_override = Some(pathext.into()); + self + } + + /// Return the configured PATHEXT override, if any. + pub(crate) const fn pathext_override(&self) -> Option<&OsString> { + self.pathext_override.as_ref() + } + + /// Directories skipped during `which` workspace fallback scans. + #[must_use] + pub fn workspace_skip_dirs(&self) -> &[String] { + &self.workspace_skip_dirs + } + + pub(crate) const fn which_cache_capacity(&self) -> NonZeroUsize { + self.which_cache_capacity + } +} diff --git a/src/stdlib/register.rs b/src/stdlib/register.rs index 1912b883b..588dd1987 100644 --- a/src/stdlib/register.rs +++ b/src/stdlib/register.rs @@ -102,7 +102,8 @@ pub fn register_with_config( .map(|path| Arc::new(path.to_path_buf())); let which_path = config.path_override().cloned(); let which_config = - WhichConfig::new(which_cwd, which_path, which_skip_dirs, which_cache_capacity); + WhichConfig::new(which_cwd, which_path, which_skip_dirs, which_cache_capacity) + .with_pathext_override(config.pathext_override().cloned()); which::register(env, which_config); let impure = state.impure_flag(); let (network_config, command_config) = config.into_components(); diff --git a/src/stdlib/which/cache.rs b/src/stdlib/which/cache.rs index a489bc8b8..3917807d1 100644 --- a/src/stdlib/which/cache.rs +++ b/src/stdlib/which/cache.rs @@ -4,7 +4,6 @@ use std::{ collections::hash_map::DefaultHasher, ffi::OsString, hash::{Hash, Hasher}, - num::NonZeroUsize, sync::{Arc, Mutex, MutexGuard, Once}, }; @@ -14,6 +13,7 @@ use metrics::{counter, describe_counter}; use tracing::field; use super::{ + WhichConfig, env::EnvSnapshot, lookup::{WorkspaceSkipList, lookup}, options::WhichOptions, @@ -28,21 +28,31 @@ pub(crate) struct WhichResolver { cache: Arc>>, cwd_override: Option>, path_override: Option, + pathext_override: Option, workspace_skips: WorkspaceSkipList, } impl WhichResolver { - pub(crate) fn new( - cwd_override: Option>, - path_override: Option, - workspace_skips: WorkspaceSkipList, - cache_capacity: NonZeroUsize, - ) -> Self { + /// Build a resolver from its configuration. + /// + /// Takes the whole [`WhichConfig`] rather than its fields individually: + /// the overrides travel together, and threading each one as a separate + /// argument grows the signature every time a new environment seam is + /// added. + pub(crate) fn new(config: WhichConfig) -> Self { describe_metrics(); + let WhichConfig { + cwd_override, + path_override, + pathext_override, + workspace_skips, + cache_capacity, + } = config; Self { cache: Arc::new(Mutex::new(LruCache::new(cache_capacity))), cwd_override, path_override, + pathext_override, workspace_skips, } } @@ -59,9 +69,10 @@ impl WhichResolver { error_category = field::Empty, ); let _guard = span.enter(); - let env = match EnvSnapshot::capture( + let env = match EnvSnapshot::capture_with_pathext( self.cwd_override.as_deref().map(Utf8PathBuf::as_path), self.path_override.as_deref(), + self.pathext_override.as_deref(), ) { Ok(env) => env, Err(err) => { @@ -225,12 +236,12 @@ mod tests { #[rstest] fn cache_capacity_bounds_entries() { - let resolver = WhichResolver::new( + let resolver = WhichResolver::new(WhichConfig::new( None, None, WorkspaceSkipList::default(), NonZeroUsize::new(1).expect("non-zero cache capacity"), - ); + )); let first_key = cache_key_for("first"); let first_path = Utf8PathBuf::from("/bin/first"); @@ -318,12 +329,12 @@ mod tests { let capacity = NonZeroUsize::new(64).expect("non-zero cache capacity"); // Use path_override to set empty PATH instead of mutating global env let empty_path = Some(std::ffi::OsString::new()); - let resolver = WhichResolver::new( + let resolver = WhichResolver::new(WhichConfig::new( Some(Arc::new(cwd.clone())), empty_path.clone(), WorkspaceSkipList::default(), capacity, - ); + )); let options = WhichOptions::default(); let err = resolver .resolve("tool", &options) @@ -331,12 +342,12 @@ mod tests { ensure!(matches!(err, ResolveError::NotFound { .. })); - let resolver_custom = WhichResolver::new( + let resolver_custom = WhichResolver::new(WhichConfig::new( Some(Arc::new(cwd.clone())), empty_path, WorkspaceSkipList::from_names([".git"]), capacity, - ); + )); let matches = resolver_custom.resolve("tool", &options)?; ensure!( matches == vec![target.join("tool")], diff --git a/src/stdlib/which/env.rs b/src/stdlib/which/env.rs index daf6e27a4..fba2d3798 100644 --- a/src/stdlib/which/env.rs +++ b/src/stdlib/which/env.rs @@ -3,7 +3,7 @@ use std::ffi::{OsStr, OsString}; use camino::{Utf8Path, Utf8PathBuf}; -#[cfg(windows)] +#[cfg(any(windows, test))] use indexmap::IndexSet; use mockable::{DefaultEnv, Env}; @@ -85,6 +85,10 @@ impl EnvSnapshot { Self::capture_impl(cwd_override, path_override, env) } + /// Capture with an explicit `PATHEXT`, shadowing the process value. + /// + /// Defined on every platform so the resolver has one capture entry point: + /// the caller need not fork on the target to pass an override through. #[cfg(windows)] pub(super) fn capture_with_pathext( cwd_override: Option<&Utf8Path>, @@ -94,6 +98,20 @@ impl EnvSnapshot { Self::capture_impl(cwd_override, path_override, &DefaultEnv, pathext_override) } + /// Capture ignoring the supplied `PATHEXT`. + /// + /// `PATHEXT` has no meaning outside Windows — nothing consults the + /// snapshot's extension list there — so the override is accepted and + /// discarded rather than forcing every caller to gate on the target. + #[cfg(not(windows))] + pub(super) fn capture_with_pathext( + cwd_override: Option<&Utf8Path>, + path_override: Option<&OsStr>, + _pathext_override: Option<&OsStr>, + ) -> Result { + Self::capture(cwd_override, path_override) + } + #[cfg(not(windows))] fn capture_impl( cwd_override: Option<&Utf8Path>, @@ -239,17 +257,48 @@ fn parse_path_entries(raw: Option<&OsStr>, cwd: &Utf8Path) -> Result) -> Vec { +/// Own the built-in list so the fallback has a single construction site. +/// +/// The entries are already lowercase and dot-prefixed, so they need no further +/// normalization. +#[cfg(any(windows, test))] +fn default_pathext() -> Vec { + DEFAULT_PATHEXT.iter().copied().map(String::from).collect() +} + +/// Normalize a raw `PATHEXT` value into lowercase, dot-prefixed extensions. +/// +/// Pure string handling, consulted only by the Windows snapshot but compiled +/// on Windows *and* under `test`. Gated to `#[cfg(windows)]` alone, its +/// normalization — lowercasing, inserting missing leading dots, trimming, +/// de-duplicating, and falling back to the built-in list when the value yields +/// nothing — could not be exercised from the Unix CI host at all, so every rule +/// it implements went unverified on the platform where the suite actually runs. +/// Compiled unconditionally it would instead be dead code in a Unix release +/// build, which `-D warnings` rejects. +/// +/// # Examples +/// +/// ```rust,ignore +/// // Values are lowercased, given a leading dot, and de-duplicated. +/// assert_eq!(parse_pathext(Some(OsStr::new("COM;.com"))), vec![".com"]); +/// ``` +#[cfg(any(windows, test))] +pub(super) fn parse_pathext(raw: Option<&OsStr>) -> Vec { let mut dedup = IndexSet::new(); - let source = raw - .map(|value| value.to_string_lossy().into_owned()) - .unwrap_or_else(|| DEFAULT_PATHEXT.join(";")); + let source = raw.map_or_else( + || DEFAULT_PATHEXT.join(";"), + |value| value.to_string_lossy().into_owned(), + ); for segment in source.split(';') { let trimmed = segment.trim(); if trimmed.is_empty() { @@ -262,7 +311,7 @@ fn parse_pathext(raw: Option<&OsStr>) -> Vec { dedup.insert(normalised); } if dedup.is_empty() { - DEFAULT_PATHEXT.iter().map(|ext| ext.to_string()).collect() + default_pathext() } else { dedup.into_iter().collect() } diff --git a/src/stdlib/which/mod.rs b/src/stdlib/which/mod.rs index 8e9ba4a6f..c6de5ad10 100644 --- a/src/stdlib/which/mod.rs +++ b/src/stdlib/which/mod.rs @@ -33,6 +33,7 @@ const NOT_FOUND_CODE: &str = "netsuke::jinja::which::not_found"; pub(crate) struct WhichConfig { pub(crate) cwd_override: Option>, pub(crate) path_override: Option, + pub(crate) pathext_override: Option, pub(crate) workspace_skips: WorkspaceSkipList, pub(crate) cache_capacity: NonZeroUsize, } @@ -47,19 +48,26 @@ impl WhichConfig { Self { cwd_override, path_override, + pathext_override: None, workspace_skips, cache_capacity, } } + + /// Shadow `PATHEXT` for the resolver this configuration builds. + /// + /// Kept off [`Self::new`] because the override is rare: only callers that + /// deliberately pin the extension list supply one, and every other call + /// site would otherwise pass `None`. + #[must_use] + pub(crate) fn with_pathext_override(mut self, pathext: Option) -> Self { + self.pathext_override = pathext; + self + } } pub(crate) fn register(env: &mut Environment<'_>, config: WhichConfig) { - let resolver = Arc::new(WhichResolver::new( - config.cwd_override, - config.path_override, - config.workspace_skips, - config.cache_capacity, - )); + let resolver = Arc::new(WhichResolver::new(config)); { let filter_resolver = Arc::clone(&resolver); env.add_filter("which", move |value: Value, kwargs: Kwargs| { @@ -312,6 +320,8 @@ pub(super) fn format_path_for_output(path: &Utf8Path) -> String { } } +#[cfg(test)] +mod pathext_tests; #[cfg(test)] mod tests { //! Unit tests for the which module facade, covering the command diff --git a/src/stdlib/which/pathext_tests.rs b/src/stdlib/which/pathext_tests.rs new file mode 100644 index 000000000..3e275f6e8 --- /dev/null +++ b/src/stdlib/which/pathext_tests.rs @@ -0,0 +1,218 @@ +//! Tests for `PATHEXT` normalization. +//! +//! These call `parse_pathext` directly rather than capturing an `EnvSnapshot`, +//! so they mutate nothing and — more importantly — run on the Unix CI host. +//! The rules below were previously behind `#[cfg(windows)]` and so were never +//! executed by the suite that actually gates merges. + +use super::env::{DEFAULT_PATHEXT, parse_pathext}; +use rstest::rstest; +use std::ffi::OsStr; + +fn parse(raw: &str) -> Vec { + parse_pathext(Some(OsStr::new(raw))) +} + +/// The default list spelled out independently of the constant under test. +/// +/// Written by hand so a change to `DEFAULT_PATHEXT` — a dropped entry, a +/// reordering — is caught here rather than silently agreeing with itself. +fn expected_default_pathext() -> Vec { + [ + ".com", ".exe", ".bat", ".cmd", ".vbs", ".vbe", ".js", ".jse", ".wsf", ".wsh", ".msc", + ] + .iter() + .copied() + .map(String::from) + .collect() +} + +#[test] +fn unset_pathext_yields_the_default_list() { + let expected = expected_default_pathext(); + assert_eq!(parse_pathext(None), expected); + let constant: Vec = DEFAULT_PATHEXT.iter().copied().map(String::from).collect(); + assert_eq!(constant, expected); +} + +/// The default list itself must stay usable, not merely be echoed back. +/// +/// Comparing the parse against `DEFAULT_PATHEXT` alone is tautological: were +/// the constant emptied or its entries mangled, that assertion would still +/// hold. These check the properties every consumer relies on. +#[test] +fn the_default_list_is_well_formed() { + assert!( + !DEFAULT_PATHEXT.is_empty(), + "an empty default disables which" + ); + for ext in DEFAULT_PATHEXT { + assert!(ext.starts_with('.'), "{ext} should carry a leading dot"); + assert_eq!(*ext, ext.to_ascii_lowercase(), "{ext} should be lowercase"); + } + for required in [".com", ".exe", ".bat", ".cmd"] { + assert!( + DEFAULT_PATHEXT.contains(&required), + "the default list should include {required}" + ); + } +} + +/// An empty or whitespace-only value must not yield an empty extension list. +/// +/// Windows would then treat nothing as executable, so `which` would report +/// every command missing. The fallback to the built-in list is what prevents a +/// blank `PATHEXT` from disabling resolution entirely. +#[rstest] +#[case::empty("")] +#[case::separators_only(";;;")] +#[case::whitespace_only(" ; ; ")] +fn valueless_pathext_falls_back_to_the_default_list(#[case] raw: &str) { + assert_eq!(parse(raw), DEFAULT_PATHEXT, "{raw:?} should fall back"); +} + +#[test] +fn extensions_are_lowercased() { + assert_eq!(parse(".COM;.EXE"), vec![".com", ".exe"]); +} + +#[test] +fn missing_leading_dots_are_inserted() { + assert_eq!(parse("COM;EXE"), vec![".com", ".exe"]); +} + +#[test] +fn surrounding_whitespace_is_trimmed() { + assert_eq!(parse(" .BAT ;\t.CMD\t"), vec![".bat", ".cmd"]); +} + +/// De-duplication is case-insensitive and survives dot insertion, so `COM`, +/// `.com`, and `.COM` collapse to one entry. +#[test] +fn duplicates_collapse_after_normalization() { + assert_eq!(parse("COM;.com;.COM; com "), vec![".com"]); +} + +/// First occurrence wins, so author-declared precedence is preserved. +#[test] +fn declaration_order_is_preserved() { + assert_eq!(parse(".exe;.bat;.com"), vec![".exe", ".bat", ".com"]); +} + +/// A value contributing nothing usable behaves as if it were absent. +#[test] +fn entries_that_normalize_to_nothing_are_skipped() { + assert_eq!(parse(".exe;; ;.bat"), vec![".exe", ".bat"]); +} + +mod properties { + //! Property coverage for `parse_pathext`. + //! + //! The fixed cases above name specific behaviours; these state the + //! invariants those cases are instances of, over inputs nobody would think + //! to write down — stray whitespace, mixed case, repeated entries, empty + //! segments, and combinations of all four. + + use super::{DEFAULT_PATHEXT, parse}; + use proptest::collection::vec; + use proptest::prelude::*; + + /// One `PATHEXT` segment: optional whitespace, optional dot, mixed case. + /// + /// Deliberately includes segments that normalize to nothing, so the + /// fallback path is generated rather than only reasoned about. + fn segment() -> impl Strategy { + // A deliberately small stem alphabet. With free-form stems, two + // segments almost never collide, so `entries_are_unique` would pass + // without ever seeing a duplicate — the generator, not the parser, + // would be satisfying it. + prop_oneof![ + 3 => ("[ \t]*", prop::bool::ANY, "com|exe|bat|Com|EXE|Bat", "[ \t]*") + .prop_map(|(lead, with_dot, stem, trail)| { + let dot = if with_dot { "." } else { "" }; + format!("{lead}{dot}{stem}{trail}") + }), + 1 => "[ \t]*".prop_map(|s: String| s), + ] + } + + fn raw_value() -> impl Strategy { + vec(segment(), 0..8).prop_map(|parts| parts.join(";")) + } + + proptest! { + /// Every entry is lowercase, dot-prefixed, and non-empty after the dot. + #[test] + fn entries_are_normalized(raw in raw_value()) { + for ext in parse(&raw) { + prop_assert!(ext.starts_with('.'), "missing dot: {ext:?}"); + prop_assert!(ext.len() > 1, "nothing after the dot: {ext:?}"); + prop_assert_eq!(&ext, &ext.to_ascii_lowercase()); + } + } + + /// No two entries name the same extension, compared case-insensitively. + /// + /// Deliberately not "no exact duplicates": the parser collects into an + /// `IndexSet`, so exact uniqueness holds structurally whatever else it + /// does, and asserting it would pass even with normalization removed. + /// Comparing case-insensitively is what makes this a claim about + /// `parse_pathext` rather than about `IndexSet`. + #[test] + fn entries_are_unique(raw in raw_value()) { + let parsed = parse(&raw); + let mut seen = std::collections::HashSet::new(); + for ext in &parsed { + prop_assert!( + seen.insert(ext.to_ascii_lowercase()), + "{ext:?} repeats an earlier entry in {parsed:?} bar case" + ); + } + } + + /// Re-parsing an already-parsed list changes nothing. + /// + /// Idempotence is what lets a caller pass a normalized value back in — + /// and it fails immediately if normalization is order-dependent or if + /// the dot prefix is applied twice. + #[test] + fn parsing_is_idempotent(raw in raw_value()) { + let once = parse(&raw); + let twice = parse(&once.join(";")); + prop_assert_eq!(once, twice); + } + + /// An input with no usable segment yields the built-in list. + /// + /// Generated rather than enumerated: any join of whitespace-only + /// segments must fall back, however many there are. + #[test] + fn unusable_input_falls_back(parts in vec("[ \t]*", 0..6)) { + prop_assert_eq!(parse(&parts.join(";")), DEFAULT_PATHEXT.to_vec()); + } + + /// The first normalized occurrence fixes an entry's position. + /// + /// Order is not cosmetic: it is the order `which` tries extensions in, + /// so a later duplicate must not move an earlier entry. + #[test] + fn first_occurrence_fixes_order(stems in vec("[a-z][a-z0-9]{0,3}", 1..5)) { + let mut expected: Vec = Vec::new(); + for stem in &stems { + let ext = format!(".{stem}"); + if !expected.contains(&ext) { + expected.push(ext); + } + } + // Append an upper-case repeat of every stem: each is a duplicate + // under normalization, so none may appear again or displace another. + let raw = stems + .iter() + .map(|s| format!(".{s}")) + .chain(stems.iter().map(|s| s.to_ascii_uppercase())) + .collect::>() + .join(";"); + prop_assert_eq!(parse(&raw), expected); + } + } +} diff --git a/tests/stdlib_which_pathext_tests.rs b/tests/stdlib_which_pathext_tests.rs new file mode 100644 index 000000000..d9590ac96 --- /dev/null +++ b/tests/stdlib_which_pathext_tests.rs @@ -0,0 +1,153 @@ +//! Behavioural coverage for Windows `PATHEXT` handling through the public +//! `which` and `command_available` helpers. +//! +//! The unit tests in `src/stdlib/which/pathext_tests.rs` pin the normalization +//! rules by calling `parse_pathext` directly; these drive the same rules end to +//! end — template render, resolver, filesystem probe — so a value that +//! normalizes correctly but is never consulted would still fail here. +//! +//! `PATHEXT` arrives through `StdlibConfig::with_pathext_override`, so no test +//! touches the process environment. +//! +//! Be aware of the reach of this file: `PATHEXT` governs resolution only on +//! Windows, and `.github/workflows/ci.yml` runs `make test` on +//! `ubuntu-latest` alone, so nothing here executes on a merge today. It gates +//! for contributors developing on Windows, and would gate for everyone were a +//! Windows job added. The rules that must hold on every host — normalization +//! and the fallback — stay covered on Linux by +//! `src/stdlib/which/pathext_tests.rs`, which is why those tests call +//! `parse_pathext` directly rather than being folded into this suite. +#![cfg(windows)] + +use std::ffi::OsString; + +use anyhow::{Context, Result, anyhow, ensure}; +use camino::{Utf8Path, Utf8PathBuf}; +use cap_std::{ambient_authority, fs_utf8::Dir}; +use minijinja::{Environment, context}; +use netsuke::stdlib::{self, StdlibConfig}; +use rstest::{fixture, rstest}; + +/// A temporary workspace holding a `bin` directory of stub executables. +struct ToolWorkspace { + _temp: tempfile::TempDir, + root: Utf8PathBuf, + bin: Utf8PathBuf, + dir: Dir, +} + +impl ToolWorkspace { + /// Create a stub executable named `filename` in `bin`. + /// + /// The extension is part of the name rather than a separate argument: it + /// is what each test is choosing, so splitting the two would invite a + /// caller to pass a stem that already carries one. + fn write_tool(&self, filename: &str) -> Result { + let path = self.bin.join(filename); + self.dir + .write(format!("bin/{filename}"), b"@echo off\r\n") + .with_context(|| format!("write stub tool {path}"))?; + Ok(path) + } +} + +#[fixture] +fn tool_workspace() -> Result { + let temp = tempfile::tempdir().context("create temp workspace")?; + let root = Utf8PathBuf::from_path_buf(temp.path().to_path_buf()) + .map_err(|path| anyhow!("temp path should be UTF-8: {path:?}"))?; + let dir = Dir::open_ambient_dir(&root, ambient_authority()) + .with_context(|| format!("open workspace {root}"))?; + let bin = root.join("bin"); + dir.create_dir("bin") + .with_context(|| format!("create {bin}"))?; + Ok(ToolWorkspace { + _temp: temp, + root, + bin, + dir, + }) +} + +/// Register the stdlib with `PATH` and `PATHEXT` both pinned by configuration. +fn stdlib_env(workspace: &ToolWorkspace, pathext: &str) -> Result> { + let dir = Dir::open_ambient_dir(&workspace.root, ambient_authority()) + .with_context(|| format!("open workspace {}", workspace.root))?; + let config = StdlibConfig::new(dir)? + .with_workspace_root_path(&workspace.root)? + .with_path_override(OsString::from(workspace.bin.as_str())) + .with_pathext_override(OsString::from(pathext)); + let mut env = Environment::new(); + stdlib::register_with_config(&mut env, config)?; + Ok(env) +} + +/// `which` renders paths with forward slashes on every platform. +fn rendered_form(path: &Utf8Path) -> String { + path.as_str().replace('\\', "/") +} + +fn assert_which_resolves_to( + env: &Environment<'_>, + command: &str, + expected: &Utf8Path, +) -> Result<()> { + let rendered = env.render_str(&format!("{{{{ which('{command}') }}}}"), context! {})?; + let expected = rendered_form(expected); + ensure!( + rendered == expected, + "expected which('{command}') to render {expected}, got {rendered}" + ); + Ok(()) +} + +fn assert_command_unavailable(env: &Environment<'_>, command: &str) -> Result<()> { + let rendered = env.render_str( + &format!("{{{{ command_available('{command}') }}}}"), + context! {}, + )?; + ensure!( + rendered == "false", + "expected command_available('{command}') to be false, got {rendered}" + ); + Ok(()) +} + +/// A blank `PATHEXT` must resolve as though it were unset. +/// +/// Taking the value literally would leave no candidate extensions, so every +/// command would be reported missing. `.cmd` comes from the built-in list, so +/// resolving `helper` here can only be the fallback at work. +#[rstest] +#[case::whitespace_only(" ")] +#[case::separators_only(";;")] +#[case::whitespace_segments(" ; ; ")] +fn blank_pathext_falls_back_to_the_default_list( + #[case] pathext: &str, + tool_workspace: Result, +) -> Result<()> { + let workspace = tool_workspace?; + let tool = workspace.write_tool("helper.cmd")?; + + let env = stdlib_env(&workspace, pathext)?; + + assert_which_resolves_to(&env, "helper", &tool) +} + +/// A `PATHEXT` written with stray spacing and mixed case is normalized, and the +/// normalized list is the one actually searched. +/// +/// `.exe` resolves despite being written as ` .exe `, while `.cmd` — present in +/// the built-in list but absent from this value — does not, which is what +/// distinguishes the injected list from a silent fallback to the default. +#[rstest] +fn normalized_pathext_governs_resolution(tool_workspace: Result) -> Result<()> { + let workspace = tool_workspace?; + let executable = workspace.write_tool("helper.exe")?; + workspace.write_tool("scripted.cmd")?; + + let env = stdlib_env(&workspace, " .COM ; .exe ")?; + + assert_which_resolves_to(&env, "helper", &executable)?; + assert_command_unavailable(&env, "scripted") +}