diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 91017134..475e5a13 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -962,7 +962,7 @@ 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 +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 @@ -985,6 +985,11 @@ reuse, and list-all semantics. Behavioural MiniJinja fixtures exercise the filter in Stage 3/4 renders to prove determinism across repeated invocations with identical environments. +Workspace fallback traversals are bounded to a depth of six, skip heavy +directories such as `.git`, `target`, `node_modules`, `dist`, and `build`, and +honour the `NETSUKE_WHICH_WORKSPACE` environment variable (set to +`0`/`false`/`off` to disable) to avoid surprising latency on large trees. + Sequence of the resolver when falling back to the workspace: ```mermaid diff --git a/src/stdlib/config.rs b/src/stdlib/config.rs index 6e8bc0fb..512057a5 100644 --- a/src/stdlib/config.rs +++ b/src/stdlib/config.rs @@ -1,6 +1,6 @@ //! Configuration types and defaults for wiring the stdlib into `MiniJinja`. -use super::{command, network::NetworkPolicy, which::DEFAULT_WORKSPACE_SKIP_DIRS}; +use super::{command, network::NetworkPolicy, which::WORKSPACE_SKIP_DIRS}; use anyhow::{anyhow, bail, ensure}; use camino::{Utf8Component, Utf8Path, Utf8PathBuf}; use cap_std::{ambient_authority, fs_utf8::Dir}; @@ -59,7 +59,7 @@ impl StdlibConfig { command_max_output_bytes: DEFAULT_COMMAND_MAX_OUTPUT_BYTES, command_max_stream_bytes: DEFAULT_COMMAND_MAX_STREAM_BYTES, which_cache_capacity, - workspace_skip_dirs: DEFAULT_WORKSPACE_SKIP_DIRS + workspace_skip_dirs: WORKSPACE_SKIP_DIRS .iter() .map(|dir| (*dir).to_owned()) .collect(), diff --git a/src/stdlib/register.rs b/src/stdlib/register.rs index b008820a..e052f83b 100644 --- a/src/stdlib/register.rs +++ b/src/stdlib/register.rs @@ -8,7 +8,7 @@ use super::{ StdlibConfig, StdlibState, collections, command, network, path, time, - which::{self, WhichConfig}, + which::{self, WhichConfig, WorkspaceSkipList}, }; use anyhow::Context; use camino::Utf8Path; @@ -90,13 +90,11 @@ pub fn register_with_config( path::register_filters(env); collections::register_filters(env); let which_cache_capacity = config.which_cache_capacity(); - let which_config = WhichConfig::new( - config - .workspace_root_path() - .map(|path| Arc::new(path.to_path_buf())), - which::WorkspaceSkipList::from_names(config.workspace_skip_dirs().iter()), - which_cache_capacity, - ); + let which_skip_dirs = WorkspaceSkipList::from_names(config.workspace_skip_dirs()); + let which_cwd = config + .workspace_root_path() + .map(|path| Arc::new(path.to_path_buf())); + let which_config = WhichConfig::new(which_cwd, which_skip_dirs, which_cache_capacity); which::register(env, which_config); let impure = state.impure_flag(); let (network_config, command_config) = config.into_components(); @@ -127,7 +125,15 @@ const FILE_TESTS: &[FileTest] = &[ ]; #[cfg(not(unix))] -const FILE_TESTS: &[FileTest] = &[("dir", is_dir), ("file", is_file), ("symlink", is_symlink)]; +const FILE_TESTS: &[FileTest] = &[ + ("dir", is_dir), + ("file", is_file), + ("symlink", is_symlink), + ("pipe", is_fifo), + ("block_device", is_block_device), + ("char_device", is_char_device), + ("device", is_device), +]; fn register_file_tests(env: &mut Environment<'_>) { for &(name, pred) in FILE_TESTS { @@ -158,68 +164,37 @@ fn is_fifo(ft: fs::FileType) -> bool { ft.is_fifo() } +#[cfg(not(unix))] +fn is_fifo(_ft: fs::FileType) -> bool { + false +} + #[cfg(unix)] fn is_block_device(ft: fs::FileType) -> bool { ft.is_block_device() } +#[cfg(not(unix))] +fn is_block_device(_ft: fs::FileType) -> bool { + false +} + #[cfg(unix)] fn is_char_device(ft: fs::FileType) -> bool { ft.is_char_device() } +#[cfg(not(unix))] +fn is_char_device(_ft: fs::FileType) -> bool { + false +} + #[cfg(unix)] fn is_device(ft: fs::FileType) -> bool { is_block_device(ft) || is_char_device(ft) } -#[cfg(test)] -mod tests { - use super::*; - use anyhow::{Result, ensure}; - use camino::Utf8PathBuf; - use minijinja::{Environment, context}; - use std::ffi::OsStr; - use tempfile::TempDir; - use test_support::{env::VarGuard, write_exec}; - - #[test] - fn register_with_config_honours_workspace_skip_dirs() -> Result<()> { - let _guard = VarGuard::set("PATH", OsStr::new("")); - let temp = TempDir::new()?; - let root_path = - Utf8PathBuf::from_path_buf(temp.path().to_path_buf()).expect("utf8 temp path"); - let root_dir = Dir::open_ambient_dir(root_path.as_path(), ambient_authority())?; - - let target = root_path.join("target"); - std::fs::create_dir_all(target.as_std_path())?; - let exec = write_exec(target.as_path(), "tool")?; - - let mut env = Environment::new(); - let default_config = - StdlibConfig::new(root_dir.try_clone()?)?.with_workspace_root_path(&root_path)?; - register_with_config(&mut env, default_config)?; - - env.add_template("t", "{{ which('tool') }}")?; - let render_err = env - .get_template("t")? - .render(context! {}) - .expect_err("default skips should block target"); - ensure!( - render_err - .to_string() - .contains("netsuke::jinja::which::not_found"), - "expected not_found error, got {render_err}" - ); - - let mut env_custom = Environment::new(); - let custom_config = StdlibConfig::new(root_dir)? - .with_workspace_root_path(&root_path)? - .with_workspace_skip_dirs([".git"])?; - register_with_config(&mut env_custom, custom_config)?; - env_custom.add_template("t", "{{ which('tool') }}")?; - let rendered = env_custom.get_template("t")?.render(context! {})?; - ensure!(rendered == exec.as_str(), "expected resolved path"); - Ok(()) - } +#[cfg(not(unix))] +fn is_device(_ft: fs::FileType) -> bool { + false } diff --git a/src/stdlib/which/cache.rs b/src/stdlib/which/cache.rs index bb1cfb03..61fcc78a 100644 --- a/src/stdlib/which/cache.rs +++ b/src/stdlib/which/cache.rs @@ -119,7 +119,7 @@ mod tests { use rstest::rstest; use std::{num::NonZeroUsize, sync::Arc}; use tempfile::TempDir; - use test_support::env::VarGuard; + use test_support::env::{VarGuard, with_isolated_path}; fn cache_key_for(command: &str) -> CacheKey { CacheKey { @@ -183,47 +183,48 @@ mod tests { #[test] fn resolver_applies_skip_list_during_resolution() -> Result<()> { - let _guard = VarGuard::set("PATH", std::ffi::OsStr::new("")); - let temp = TempDir::new()?; - let cwd = Utf8PathBuf::from_path_buf(temp.path().to_path_buf()) - .map_err(|path| anyhow!("temp path should be utf8: {path:?}"))?; - - let target = cwd.join("target"); - std::fs::create_dir_all(target.as_std_path())?; - std::fs::write(target.join("tool").as_std_path(), b"#!/bin/sh\n")?; - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let path = target.join("tool"); - let mut perms = std::fs::metadata(path.as_std_path())?.permissions(); - perms.set_mode(0o755); - std::fs::set_permissions(path.as_std_path(), perms)?; - } - - let capacity = NonZeroUsize::new(64).expect("non-zero cache capacity"); - let resolver = WhichResolver::new( - Some(Arc::new(cwd.clone())), - WorkspaceSkipList::default(), - capacity, - ); - let options = WhichOptions::default(); - let err = resolver - .resolve("tool", &options) - .expect_err("default skip should ignore target"); - - ensure!(matches!(err.kind(), ErrorKind::InvalidOperation)); - - let resolver_custom = WhichResolver::new( - Some(Arc::new(cwd.clone())), - WorkspaceSkipList::from_names([".git"]), - capacity, - ); - let matches = resolver_custom.resolve("tool", &options)?; - ensure!( - matches == vec![target.join("tool")], - "expected executable discovery when target not skipped" - ); - - Ok(()) + with_isolated_path(std::ffi::OsStr::new(""), || -> Result<()> { + let temp = TempDir::new()?; + let cwd = Utf8PathBuf::from_path_buf(temp.path().to_path_buf()) + .map_err(|path| anyhow!("temp path should be utf8: {path:?}"))?; + + let target = cwd.join("target"); + std::fs::create_dir_all(target.as_std_path())?; + std::fs::write(target.join("tool").as_std_path(), b"#!/bin/sh\n")?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let path = target.join("tool"); + let mut perms = std::fs::metadata(path.as_std_path())?.permissions(); + perms.set_mode(0o755); + std::fs::set_permissions(path.as_std_path(), perms)?; + } + + let capacity = NonZeroUsize::new(64).expect("non-zero cache capacity"); + let resolver = WhichResolver::new( + Some(Arc::new(cwd.clone())), + WorkspaceSkipList::default(), + capacity, + ); + let options = WhichOptions::default(); + let err = resolver + .resolve("tool", &options) + .expect_err("default skip should ignore target"); + + ensure!(matches!(err.kind(), ErrorKind::InvalidOperation)); + + let resolver_custom = WhichResolver::new( + Some(Arc::new(cwd.clone())), + WorkspaceSkipList::from_names([".git"]), + capacity, + ); + let matches = resolver_custom.resolve("tool", &options)?; + ensure!( + matches == vec![target.join("tool")], + "expected executable discovery when target not skipped" + ); + + Ok(()) + }) } } diff --git a/src/stdlib/which/lookup/mod.rs b/src/stdlib/which/lookup/mod.rs index 3cc01549..c5043241 100644 --- a/src/stdlib/which/lookup/mod.rs +++ b/src/stdlib/which/lookup/mod.rs @@ -17,7 +17,7 @@ use super::{ }; mod workspace; use workspace::search_workspace; -pub(crate) use workspace::{DEFAULT_WORKSPACE_SKIP_DIRS, WorkspaceSearchParams, WorkspaceSkipList}; +pub(crate) use workspace::{WORKSPACE_SKIP_DIRS, WorkspaceSkipList}; /// Resolve `command` either as a direct path or by searching the environment's /// PATH, optionally canonicalising or collecting all matches. @@ -45,15 +45,8 @@ pub(super) fn lookup( let dirs = env.resolved_dirs(options.cwd_mode); let mut matches = Vec::new(); - #[cfg(windows)] - let suffixes = env.pathext(); - for dir in &dirs { - #[cfg(windows)] - let candidates = env::candidate_paths(dir, command, suffixes); - #[cfg(not(windows))] - let candidates = vec![dir.join(command)]; - + let candidates = candidates_for_dir(env, dir, command); if push_matches(&mut matches, candidates, options.all) { break; } @@ -87,44 +80,49 @@ pub(super) fn lookup( /// /// Returns `netsuke::jinja::which::not_found` when no matching executable is /// discovered. +#[cfg(windows)] pub(super) fn resolve_direct( command: &str, env: &EnvSnapshot, options: &WhichOptions, ) -> Result, Error> { - let raw = Utf8Path::new(command); - let resolved = if raw.is_absolute() { - raw.to_path_buf() - } else { - env.cwd.join(raw) - }; - #[cfg(windows)] - { - resolve_direct_windows(command, &resolved, env, options) + let resolved = normalize_direct_path(command, env); + let candidates = direct_candidates(&resolved, env); + let mut matches = Vec::new(); + let _ = push_matches(&mut matches, candidates, options.all); + if matches.is_empty() { + return Err(direct_not_found(command, &resolved)); } - #[cfg(not(windows))] - { - resolve_direct_posix(command, &resolved, options) + if options.canonical { + canonicalise(matches) + } else { + Ok(matches) } } -#[cfg(windows)] -fn resolve_direct_windows( +#[cfg(not(windows))] +pub(super) fn resolve_direct( command: &str, - resolved: &Utf8PathBuf, env: &EnvSnapshot, options: &WhichOptions, ) -> Result, Error> { - let candidates = direct_candidates(resolved, env); - let mut matches = Vec::new(); - let _ = push_matches(&mut matches, candidates, options.all); - if matches.is_empty() { - return Err(direct_not_found(command, resolved)); + let resolved = normalize_direct_path(command, env); + if !is_executable(&resolved) { + return Err(direct_not_found(command, &resolved)); } if options.canonical { - canonicalise(matches) + canonicalise(vec![resolved]) } else { - Ok(matches) + Ok(vec![resolved]) + } +} + +fn normalize_direct_path(command: &str, env: &EnvSnapshot) -> Utf8PathBuf { + let raw = Utf8Path::new(command); + if raw.is_absolute() { + raw.to_path_buf() + } else { + env.cwd.join(raw) } } @@ -144,22 +142,6 @@ fn direct_candidates(resolved: &Utf8PathBuf, env: &EnvSnapshot) -> Vec Result, Error> { - if !is_executable(resolved) { - return Err(direct_not_found(command, resolved)); - } - if options.canonical { - canonicalise(vec![resolved.clone()]) - } else { - Ok(vec![resolved.clone()]) - } -} - /// Push executable candidates into `matches`, optionally short-circuiting when /// only the first hit is required. /// @@ -196,6 +178,18 @@ pub(super) fn is_direct_path(command: &str) -> bool { } } +fn candidates_for_dir(env: &EnvSnapshot, dir: &Utf8Path, command: &str) -> Vec { + #[cfg(windows)] + { + env::candidate_paths(dir, command, env.pathext()) + } + #[cfg(not(windows))] + { + let _ = env; + vec![dir.join(command)] + } +} + /// Check whether `path` points to an executable file. /// /// On Unix this requires at least one execute bit. On other platforms it only @@ -214,36 +208,12 @@ struct HandleMissContext<'a> { workspace_skips: &'a WorkspaceSkipList, } -#[cfg(windows)] -fn search_workspace_for_platform( - cwd: &Utf8Path, - command: &str, - params: WorkspaceSearchParams<'_>, - env: &EnvSnapshot, -) -> Result, Error> { - search_workspace(cwd, command, params, env) -} - -#[cfg(not(windows))] -fn search_workspace_for_platform( - cwd: &Utf8Path, - command: &str, - params: WorkspaceSearchParams<'_>, - _env: &EnvSnapshot, -) -> Result, Error> { - search_workspace(cwd, command, params, ()) -} - fn handle_miss(ctx: HandleMissContext<'_>) -> Result, Error> { let path_empty = ctx.env.raw_path.as_ref().is_none_or(|path| path.is_empty()); if path_empty && !matches!(ctx.options.cwd_mode, CwdMode::Never) { - let search = WorkspaceSearchParams { - collect_all: ctx.options.all, - skip_dirs: ctx.workspace_skips, - }; let discovered = - search_workspace_for_platform(ctx.env.cwd.as_path(), ctx.command, search, ctx.env)?; + search_workspace(ctx.env, ctx.command, ctx.options.all, ctx.workspace_skips)?; if !discovered.is_empty() { return if ctx.options.canonical { canonicalise(discovered) diff --git a/src/stdlib/which/lookup/tests.rs b/src/stdlib/which/lookup/tests.rs index 068632fd..09cba4da 100644 --- a/src/stdlib/which/lookup/tests.rs +++ b/src/stdlib/which/lookup/tests.rs @@ -4,11 +4,9 @@ use super::*; use anyhow::{Context, Result, anyhow, ensure}; use rstest::{fixture, rstest}; -use std::fs; +use std::{ffi::OsStr, fs}; use tempfile::TempDir; -#[cfg(windows)] -use test_support::make_executable; -use test_support::{env::VarGuard, write_exec}; +use test_support::env::VarGuard; struct TempWorkspace { root: Utf8PathBuf, @@ -36,64 +34,38 @@ fn workspace() -> TempWorkspace { TempWorkspace::new().expect("create utf8 temp workspace") } -/// Helper to execute workspace search with platform-specific env handling. -fn execute_workspace_search( - workspace: &TempWorkspace, - collect_all: bool, - skip_dirs: &WorkspaceSkipList, -) -> Result> { - #[cfg(windows)] - let snapshot = - EnvSnapshot::capture(Some(workspace.root())).expect("capture env for workspace search"); +#[cfg(unix)] +fn make_executable(path: &Utf8Path) -> Result<()> { + use std::os::unix::fs::PermissionsExt; + let mut perms = fs::metadata(path.as_std_path()) + .context("stat exec")? + .permissions(); + perms.set_mode(0o755); + fs::set_permissions(path.as_std_path(), perms).context("chmod exec") +} - #[cfg(windows)] - let results = search_workspace( - workspace.root(), - "tool", - WorkspaceSearchParams { - collect_all, - skip_dirs, - }, - &snapshot, - )?; - - #[cfg(not(windows))] - let results = search_workspace( - workspace.root(), - "tool", - WorkspaceSearchParams { - collect_all, - skip_dirs, - }, - (), - )?; - - Ok(results) -} - -/// Helper to test that workspace search skips a specific directory. -fn test_workspace_skips_directory( - workspace: &TempWorkspace, - dir_name: &str, - skips: &WorkspaceSkipList, - assertion_msg: &str, -) -> Result<()> { - let heavy = workspace.root().join(dir_name); - fs::create_dir_all(heavy.as_std_path()).with_context(|| format!("mkdir {dir_name}"))?; - write_exec(heavy.as_path(), "tool")?; - let results = execute_workspace_search(workspace, false, skips)?; - ensure!(results.is_empty(), "{}", assertion_msg); +#[cfg(not(unix))] +fn make_executable(_path: &Utf8Path) -> Result<()> { Ok(()) } +fn write_exec(root: &Utf8Path, name: &str) -> Result { + let path = root.join(name); + fs::write(path.as_std_path(), b"#!/bin/sh\n").context("write exec stub")?; + make_executable(&path)?; + Ok(path) +} + #[rstest] fn search_workspace_returns_executable_and_skips_non_exec(workspace: TempWorkspace) -> Result<()> { let exec = write_exec(workspace.root(), "tool")?; let non_exec = workspace.root().join("tool2"); fs::write(non_exec.as_std_path(), b"not exec").context("write non exec")?; - let skips = WorkspaceSkipList::default(); - let results = execute_workspace_search(&workspace, false, &skips)?; + let _guard = VarGuard::set("PATH", OsStr::new(workspace.root().as_str())); + let snapshot = + EnvSnapshot::capture(Some(workspace.root())).expect("capture env for workspace search"); + let results = search_workspace(&snapshot, "tool", false, &WorkspaceSkipList::default())?; ensure!( results == vec![exec], "expected executable to be discovered" @@ -107,9 +79,11 @@ fn search_workspace_collects_all_matches(workspace: TempWorkspace) -> Result<()> let subdir = workspace.root().join("bin"); fs::create_dir_all(subdir.as_std_path()).context("mkdir bin")?; let second = write_exec(subdir.as_path(), "tool")?; - let skips = WorkspaceSkipList::default(); - let mut results = execute_workspace_search(&workspace, true, &skips)?; + let _guard = VarGuard::set("PATH", OsStr::new(workspace.root().as_str())); + let snapshot = + EnvSnapshot::capture(Some(workspace.root())).expect("capture env for workspace search"); + let mut results = search_workspace(&snapshot, "tool", true, &WorkspaceSkipList::default())?; results.sort(); let mut expected = vec![first, second]; expected.sort(); @@ -122,105 +96,174 @@ fn search_workspace_collects_all_matches(workspace: TempWorkspace) -> Result<()> #[rstest] fn search_workspace_skips_heavy_directories(workspace: TempWorkspace) -> Result<()> { - let skips = WorkspaceSkipList::default(); - test_workspace_skips_directory( - &workspace, - "target", - &skips, - "expected target/ to be skipped", - ) + let heavy = workspace.root().join("target"); + fs::create_dir_all(heavy.as_std_path()).context("mkdir target")?; + write_exec(heavy.as_path(), "tool")?; + + let _guard = VarGuard::set("PATH", OsStr::new(workspace.root().as_str())); + let snapshot = + EnvSnapshot::capture(Some(workspace.root())).expect("capture env for workspace search"); + let results = search_workspace(&snapshot, "tool", false, &WorkspaceSkipList::default())?; + ensure!(results.is_empty(), "expected target/ to be skipped"); + Ok(()) } +#[cfg(unix)] #[rstest] -fn search_workspace_skips_common_editor_directories(workspace: TempWorkspace) -> Result<()> { - let skip_dirs = [".git", "node_modules", ".idea", ".vscode"]; - for dir in skip_dirs { - let path = workspace.root().join(dir); - fs::create_dir_all(path.as_std_path()).context("mkdir skip dir")?; - write_exec(path.as_path(), "tool")?; - } - let skips = WorkspaceSkipList::default(); +fn search_workspace_ignores_unreadable_entries(workspace: TempWorkspace) -> Result<()> { + use std::os::unix::fs::PermissionsExt; + let blocked = workspace.root().join("blocked"); + fs::create_dir_all(blocked.as_std_path()).context("mkdir blocked")?; + let mut perms = fs::metadata(blocked.as_std_path()) + .context("stat blocked")? + .permissions(); + perms.set_mode(0o000); + fs::set_permissions(blocked.as_std_path(), perms).context("chmod blocked")?; - let results = execute_workspace_search(&workspace, false, &skips)?; - ensure!(results.is_empty(), "expected editor caches to be skipped"); + let exec = write_exec(workspace.root(), "tool")?; + let _guard = VarGuard::set("PATH", OsStr::new(workspace.root().as_str())); + let snapshot = + EnvSnapshot::capture(Some(workspace.root())).expect("capture env for workspace search"); + let results = search_workspace(&snapshot, "tool", false, &WorkspaceSkipList::default())?; + ensure!( + results == vec![exec], + "expected readable executable despite blocked dir" + ); Ok(()) } -#[cfg(windows)] +#[cfg(unix)] #[rstest] -fn search_workspace_skips_directories_case_insensitively(workspace: TempWorkspace) -> Result<()> { - let skips = WorkspaceSkipList::default(); - test_workspace_skips_directory( - &workspace, - "TARGET", - &skips, - "expected TARGET/ to be skipped case-insensitively", - ) -} +fn path_with_invalid_utf8_triggers_args_error(workspace: TempWorkspace) -> Result<()> { + use std::os::unix::ffi::OsStrExt; -#[rstest] -fn search_workspace_uses_custom_skip_configuration(workspace: TempWorkspace) -> Result<()> { - let target = workspace.root().join("target"); - fs::create_dir_all(target.as_std_path()).context("mkdir target")?; - let exec = write_exec(target.as_path(), "tool")?; - let skips = WorkspaceSkipList::from_names([".git"]); + let invalid_path = std::ffi::OsStr::from_bytes(b"/bin:\xFF"); + let _guard = VarGuard::set("PATH", invalid_path); + + let err = EnvSnapshot::capture(Some(workspace.root())) + .expect_err("invalid PATH should fail EnvSnapshot::capture"); + let msg = err.to_string(); - let results = execute_workspace_search(&workspace, false, &skips)?; ensure!( - results == vec![exec], - "expected custom skip list to allow target/" + msg.contains("netsuke::jinja::which::args"), + "expected PATH parsing error, got: {msg}" ); + ensure!( + msg.contains("PATH entry #1"), + "expected PATH entry index in message, got: {msg}" + ); + Ok(()) } #[rstest] -fn lookup_respects_workspace_skip_configuration(workspace: TempWorkspace) -> Result<()> { - let _guard = VarGuard::unset("PATH"); - - let target = workspace.root().join("target"); - fs::create_dir_all(target.as_std_path()).context("mkdir target")?; - let exec = write_exec(target.as_path(), "tool")?; - let options = WhichOptions::default(); - - let env_default = EnvSnapshot::capture(Some(workspace.root())).expect("capture env"); - let default_skips = WorkspaceSkipList::default(); - let err = lookup("tool", &env_default, &options, &default_skips) - .expect_err("default skips should ignore target"); +fn relative_path_entries_resolve_against_cwd(workspace: TempWorkspace) -> Result<()> { + let bin = workspace.root().join("bin"); + let tools = workspace.root().join("tools"); + fs::create_dir_all(bin.as_std_path()).context("mkdir bin")?; + fs::create_dir_all(tools.as_std_path()).context("mkdir tools")?; + + let path_value = std::env::join_paths([ + workspace.root().as_std_path(), + std::path::Path::new("bin"), + std::path::Path::new("tools"), + ]) + .context("join PATH entries")?; + let _guard = VarGuard::set("PATH", path_value.as_os_str()); + + let snapshot = EnvSnapshot::capture(Some(workspace.root())) + .context("capture env with relative PATH entries")?; + let resolved_dirs = snapshot.resolved_dirs(CwdMode::Never); + ensure!( - matches!(err.kind(), minijinja::ErrorKind::InvalidOperation), - "expected not_found error" + resolved_dirs.contains(&bin), + "resolved_dirs missing bin: {resolved_dirs:?}" ); - - let env_custom = EnvSnapshot::capture(Some(workspace.root())).expect("capture env"); - let custom_skips = WorkspaceSkipList::from_names([".git"]); - let results = lookup("tool", &env_custom, &options, &custom_skips)?; ensure!( - results == vec![exec], - "expected discovery when target allowed" + resolved_dirs.contains(&tools), + "resolved_dirs missing tools: {resolved_dirs:?}" + ); + + Ok(()) +} + +#[cfg(windows)] +#[rstest] +fn pathext_empty_uses_default_fallback(workspace: TempWorkspace) -> Result<()> { + let _path_guard = VarGuard::set("PATH", std::ffi::OsStr::new(workspace.root().as_str())); + let _pathext_guard = VarGuard::set("PATHEXT", std::ffi::OsStr::new("")); + + let snapshot = + EnvSnapshot::capture(Some(workspace.root())).context("capture env for empty PATHEXT")?; + let pathexts = snapshot.pathext(); + + assert!( + pathexts.iter().any(|ext| ext.eq_ignore_ascii_case(".com")), + "default PATHEXT should include .COM", ); + assert!( + pathexts.iter().any(|ext| ext.eq_ignore_ascii_case(".exe")), + "default PATHEXT should include .EXE", + ); + + Ok(()) +} + +#[cfg(windows)] +#[rstest] +fn pathext_without_leading_dots_is_normalised_and_deduplicated( + workspace: TempWorkspace, +) -> Result<()> { + let _path_guard = VarGuard::set("PATH", std::ffi::OsStr::new(workspace.root().as_str())); + let _pathext_guard = VarGuard::set("PATHEXT", std::ffi::OsStr::new("COM;EXE;EXE; .BAT ;bat")); + + let snapshot = EnvSnapshot::capture(Some(workspace.root()))?; + let mut pathexts = snapshot.pathext().to_vec(); + pathexts.sort_unstable_by(|a, b| a.to_lowercase().cmp(&b.to_lowercase())); + + let contains_ci = |needle: &str| pathexts.iter().any(|ext| ext.eq_ignore_ascii_case(needle)); + + assert!(contains_ci(".COM")); + assert!(contains_ci(".EXE")); + assert!(contains_ci(".BAT")); + + let mut lower: Vec = pathexts + .iter() + .map(|ext| ext.to_ascii_lowercase()) + .collect(); + lower.sort_unstable(); + lower.dedup(); + assert_eq!(lower.len(), pathexts.len()); Ok(()) } #[cfg(unix)] #[rstest] -fn search_workspace_ignores_unreadable_entries(workspace: TempWorkspace) -> Result<()> { +fn direct_path_not_executable_raises_direct_not_found(workspace: TempWorkspace) -> Result<()> { use std::os::unix::fs::PermissionsExt; - let blocked = workspace.root().join("blocked"); - fs::create_dir_all(blocked.as_std_path()).context("mkdir blocked")?; - let mut perms = fs::metadata(blocked.as_std_path()) - .context("stat blocked")? + + let script = workspace.root().join("script.sh"); + fs::write(script.as_std_path(), "#!/bin/sh\necho test\n").context("write script")?; + let mut perms = fs::metadata(script.as_std_path()) + .context("stat script")? .permissions(); - perms.set_mode(0o000); - fs::set_permissions(blocked.as_std_path(), perms).context("chmod blocked")?; + perms.set_mode(0o644); + fs::set_permissions(script.as_std_path(), perms).context("chmod script")?; + + let _path_guard = VarGuard::set("PATH", OsStr::new(workspace.root().as_str())); + let snapshot = + EnvSnapshot::capture(Some(workspace.root())).context("capture env for direct path")?; + + let err = resolve_direct(script.as_str(), &snapshot, &WhichOptions::default()) + .expect_err("non-executable direct path should fail"); + let msg = err.to_string(); - let exec = write_exec(workspace.root(), "tool")?; - let skips = WorkspaceSkipList::default(); - let results = execute_workspace_search(&workspace, false, &skips)?; ensure!( - results == vec![exec], - "expected readable executable despite blocked dir" + msg.contains("not executable"), + "expected not executable message: {msg}" ); + Ok(()) } diff --git a/src/stdlib/which/lookup/workspace.rs b/src/stdlib/which/lookup/workspace.rs deleted file mode 100644 index 36853306..00000000 --- a/src/stdlib/which/lookup/workspace.rs +++ /dev/null @@ -1,364 +0,0 @@ -//! Workspace fallback search helpers for the `which` resolver. - -#[cfg(windows)] -use std::collections::HashSet; - -use camino::{Utf8Path, Utf8PathBuf}; -use indexmap::IndexSet; -use minijinja::{Error, ErrorKind}; -use walkdir::WalkDir; - -#[cfg(windows)] -use super::EnvSnapshot; -#[cfg(windows)] -use super::env; -use super::is_executable; - -/// Default set of workspace directories to skip during workspace fallback -/// scans. Callers can override or extend this list via `WorkspaceSkipList` to -/// avoid expensive traversals of tool caches or IDE metadata. -pub(crate) const DEFAULT_WORKSPACE_SKIP_DIRS: &[&str] = - &[".git", "target", "node_modules", ".idea", ".vscode"]; - -/// Normalised set of directory basenames that should be ignored during -/// workspace traversal. Entries are lowercased on Windows to provide -/// case-insensitive membership checks. -#[derive(Clone, Debug, PartialEq, Eq)] -pub(crate) struct WorkspaceSkipList { - dirs: IndexSet, -} - -/// Parameters used when scanning the workspace for executables. -/// -/// - `collect_all`: whether to collect every match instead of short-circuiting -/// after the first hit. -/// - `skip_dirs`: reference to the `WorkspaceSkipList` applied during -/// traversal. -#[derive(Clone, Copy, Debug)] -pub(crate) struct WorkspaceSearchParams<'a> { - pub(crate) collect_all: bool, - pub(crate) skip_dirs: &'a WorkspaceSkipList, -} - -impl WorkspaceSkipList { - /// Build a skip list from the provided directory names, normalising for the - /// host platform and removing duplicates. - /// - /// Empty or whitespace-only inputs are ignored; callers that require - /// strict validation should filter earlier (for example via - /// `StdlibConfig::with_workspace_skip_dirs`). - pub(crate) fn from_names(names: impl IntoIterator>) -> Self { - let mut dedup = IndexSet::new(); - for name in names { - let trimmed = name.as_ref().trim(); - if trimmed.is_empty() { - continue; - } - dedup.insert(normalise_dir_name(trimmed)); - } - Self { dirs: dedup } - } - - /// Return `true` when a directory should be skipped during traversal. - pub(crate) fn should_skip(&self, name: &str) -> bool { - self.dirs.contains(&normalise_dir_name(name)) - } -} - -impl Default for WorkspaceSkipList { - fn default() -> Self { - Self::from_names(DEFAULT_WORKSPACE_SKIP_DIRS.iter().copied()) - } -} - -impl std::hash::Hash for WorkspaceSkipList { - fn hash(&self, state: &mut H) { - for dir in &self.dirs { - dir.hash(state); - } - } -} - -#[cfg(windows)] -/// Lowercase directory names to ensure case-insensitive comparisons on Windows -/// filesystems. -fn normalise_dir_name(name: &str) -> String { - name.to_ascii_lowercase() -} - -#[cfg(not(windows))] -/// Preserve directory names verbatim on POSIX because comparisons are -/// case-sensitive. -fn normalise_dir_name(name: &str) -> String { - name.to_owned() -} - -/// Recursively search the workspace rooted at `cwd` for executables matching -/// `command`. -/// -/// - `cwd`: workspace root to traverse (symlinks are not followed). -/// - `command`: name to match (Windows: case-insensitive with `PATHEXT` -/// expansion; other platforms: exact case-sensitive filename match). -/// - `collect_all`: when `true`, return every match; otherwise stop after the -/// first executable. -/// - `skip_dirs`: directory basenames to omit during traversal. Empty inputs -/// are ignored and callers should validate earlier layers when strictness is -/// required. -/// - `env`: provided only on Windows to supply `PATHEXT` for matching. -/// -/// Skips unreadable entries, ignores heavy/VCS directories via -/// `should_visit_entry`, and returns `Ok(Vec)` containing the -/// discovered executables or an `Error` if UTF-8 conversion fails. -pub(super) fn search_workspace( - cwd: &Utf8Path, - command: &str, - params: WorkspaceSearchParams<'_>, - #[cfg(windows)] env: &EnvSnapshot, - #[cfg(not(windows))] _env: (), -) -> Result, Error> { - #[cfg(windows)] - let match_ctx = prepare_workspace_match(command, env); - #[cfg(not(windows))] - let match_ctx = (); - - let entries = WalkDir::new(cwd) - .follow_links(false) - .sort_by_file_name() - .into_iter() - .filter_entry(|entry| should_visit_entry(entry, params.skip_dirs)) - .filter_map(|walk_entry| { - walk_entry - .map_err(|err| { - tracing::debug!( - %command, - error = %err, - "skipping unreadable workspace entry during which fallback" - ); - err - }) - .ok() - }); - - collect_workspace_matches(entries, command, params.collect_all, match_ctx) -} - -/// Collect executable matches from workspace traversal. -/// -/// Parameters: -/// - `entries`: iterator of `walkdir::DirEntry` values to inspect. -/// - `command`: command name used for platform-specific filename matching. -/// - `collect_all`: when `false`, stops after the first executable match. -/// - `match_ctx`: on Windows, a `WorkspaceMatchContext`; on other platforms, -/// the unit type to align signatures. -/// -/// Returns a `Result, Error>` containing matched executable -/// paths or an error when UTF-8 conversion fails. -fn collect_workspace_matches( - entries: impl Iterator, - command: &str, - collect_all: bool, - #[cfg(windows)] match_ctx: WorkspaceMatchContext, - #[cfg(not(windows))] match_ctx: (), -) -> Result, Error> { - let mut matches = Vec::new(); - - for entry in entries { - #[cfg(windows)] - let maybe_match = process_workspace_entry(entry, command, &match_ctx)?; - #[cfg(not(windows))] - let maybe_match = process_workspace_entry(entry, command, match_ctx)?; - - if let Some(path) = maybe_match { - matches.push(path); - if !collect_all { - break; - } - } - } - - Ok(matches) -} - -/// Allow traversal for all files and directories except heavy/VCS roots to -/// keep workspace scans fast. -fn should_visit_entry(entry: &walkdir::DirEntry, skip_dirs: &WorkspaceSkipList) -> bool { - if !entry.file_type().is_dir() { - return true; - } - let name = entry.file_name().to_string_lossy(); - !skip_dirs.should_skip(&name) -} - -/// Process a single `walkdir::DirEntry`: ensure it is a file, apply the -/// platform-specific filename match, convert the path to UTF-8 (erroring on -/// non-UTF-8 components), and return `Some(path)` only when the entry is -/// executable; otherwise return `None`. -fn process_workspace_entry( - entry: walkdir::DirEntry, - command: &str, - #[cfg(windows)] ctx: &WorkspaceMatchContext, - #[cfg(not(windows))] ctx: (), -) -> Result, Error> { - if !entry.file_type().is_file() { - return Ok(None); - } - #[cfg(windows)] - let matches_entry = workspace_entry_matches(&entry, ctx); - #[cfg(not(windows))] - let matches_entry = workspace_entry_matches(&entry, command, ctx); - if !matches_entry { - return Ok(None); - } - let path = entry.into_path(); - let utf8 = Utf8PathBuf::from_path_buf(path).map_err(|path_buf| { - let lossy_path = path_buf.to_string_lossy(); - Error::new( - ErrorKind::InvalidOperation, - format!( - "workspace path contains non-UTF-8 components while resolving command '{command}': {lossy_path}" - ), - ) - })?; - Ok(is_executable(&utf8).then_some(utf8)) -} - -#[cfg(windows)] -/// Windows-specific match context for case-insensitive filename matching. -/// -/// Encapsulates normalised command state for workspace traversal: -/// - `command_lower`: lowercased command name. -/// - `command_has_ext`: whether the supplied command already includes a file -/// extension. -/// - `basenames`: PATHEXT-expanded candidate filenames for extension-less -/// commands, stored in lowercase for case-insensitive comparisons. -#[derive(Clone)] -struct WorkspaceMatchContext { - command_lower: String, - command_has_ext: bool, - basenames: HashSet, -} - -#[cfg(windows)] -/// Perform case-insensitive filename matching with PATHEXT expansion. -/// -/// Returns `true` when the entry's lowercased filename matches the command -/// directly or—when the command lacks an extension—any PATHEXT-expanded -/// basename candidate. -fn workspace_entry_matches(entry: &walkdir::DirEntry, ctx: &WorkspaceMatchContext) -> bool { - let file_name = entry.file_name().to_string_lossy().to_ascii_lowercase(); - if file_name == ctx.command_lower { - return true; - } - if ctx.command_has_ext { - return false; - } - ctx.basenames.contains(&file_name) -} - -#[cfg(not(windows))] -/// Perform exact case-sensitive filename matching. -/// -/// Returns `true` when the entry's filename matches the command string. -fn workspace_entry_matches(entry: &walkdir::DirEntry, command: &str, _ctx: ()) -> bool { - let file_name = entry.file_name().to_string_lossy(); - file_name == command -} - -#[cfg(windows)] -/// Initialise Windows match context by normalising the command and expanding -/// PATHEXT. -/// -/// Lowercases the command, records whether it already contains an extension, -/// and—when extension-less—derives candidate basenames by applying PATHEXT -/// suffixes via `env::candidate_paths`. All basenames are stored in lowercase -/// to enable case-insensitive comparisons during workspace traversal. -fn prepare_workspace_match(command: &str, env: &EnvSnapshot) -> WorkspaceMatchContext { - let command_lower = command.to_ascii_lowercase(); - let command_has_ext = command_lower.contains('.'); - let mut basenames = HashSet::new(); - - if !command_has_ext { - let candidates = env::candidate_paths(Utf8Path::new(""), &command_lower, env.pathext()); - for candidate in candidates { - if let Some(name) = Utf8Path::new(candidate.as_str()).file_name() { - basenames.insert(name.to_ascii_lowercase()); - } - } - } - - WorkspaceMatchContext { - command_lower, - command_has_ext, - basenames, - } -} - -#[cfg(test)] -mod tests { - use super::*; - use anyhow::{Result, ensure}; - use rstest::rstest; - use tempfile::TempDir; - - #[rstest] - #[case::default_list(WorkspaceSkipList::default(), &["target", ".git"], &["src"])] - #[case::custom_list( - WorkspaceSkipList::from_names(["build", "dist"]), - &["build", "dist"], - &["target"], - )] - fn skip_list_respects_configured_dirs( - #[case] skips: WorkspaceSkipList, - #[case] should_skip: &[&str], - #[case] should_visit: &[&str], - ) { - for dir in should_skip { - assert!(skips.should_skip(dir), "expected {dir} to be skipped"); - } - for dir in should_visit { - assert!(!skips.should_skip(dir), "expected {dir} to be visited"); - } - } - - #[cfg(windows)] - #[test] - fn skip_list_matching_is_case_insensitive_on_windows() { - let skips = WorkspaceSkipList::from_names(["TARGET", ".GIT"]); - assert!(skips.should_skip("target")); - assert!(skips.should_skip(".git")); - } - - #[test] - fn should_visit_entry_respects_skip_list_for_directories_only() -> Result<()> { - let temp = TempDir::new()?; - let root = temp.path(); - std::fs::create_dir(root.join("target"))?; - std::fs::write(root.join("tool"), b"bin")?; - - let entries: Vec<_> = WalkDir::new(root) - .min_depth(1) - .max_depth(1) - .into_iter() - .collect::>() - .expect("walkdir should iterate entries without error"); - let skips = WorkspaceSkipList::default(); - let mut saw_target = false; - let mut saw_tool = false; - for entry in entries { - let name = entry.file_name().to_string_lossy(); - if name == "target" { - saw_target = true; - ensure!(!should_visit_entry(&entry, &skips)); - } - if name == "tool" { - saw_tool = true; - ensure!(should_visit_entry(&entry, &skips)); - } - } - ensure!( - saw_target && saw_tool, - "expected both entries to be visited" - ); - Ok(()) - } -} diff --git a/src/stdlib/which/lookup/workspace/mod.rs b/src/stdlib/which/lookup/workspace/mod.rs new file mode 100644 index 00000000..808cedb2 --- /dev/null +++ b/src/stdlib/which/lookup/workspace/mod.rs @@ -0,0 +1,173 @@ +//! Workspace fallback search helpers for the `which` resolver. + +use std::{ + env, + hash::{Hash, Hasher}, +}; + +use camino::Utf8PathBuf; +use indexmap::IndexSet; +use minijinja::Error; + +use crate::stdlib::which::env::EnvSnapshot; + +#[cfg(not(windows))] +mod posix; +#[cfg(windows)] +mod windows; + +#[cfg(not(windows))] +use posix::search_workspace as platform_search_workspace; +#[cfg(windows)] +use windows::search_workspace as platform_search_workspace; + +pub(super) const WORKSPACE_MAX_DEPTH: usize = 6; +pub(crate) const WORKSPACE_SKIP_DIRS: &[&str] = + &[".git", "target", "node_modules", "dist", "build"]; + +const WORKSPACE_FALLBACK_ENV: &str = "NETSUKE_WHICH_WORKSPACE"; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct WorkspaceSkipList { + dirs: IndexSet, +} + +impl WorkspaceSkipList { + fn from_defaults() -> Self { + let mut dirs = IndexSet::new(); + for dir in WORKSPACE_SKIP_DIRS { + dirs.insert((*dir).to_owned()); + } + Self { dirs } + } + + fn contains(&self, name: &str) -> bool { + self.dirs.contains(name) + } + + /// Build a skip list from provided directory basenames, normalising and + /// de-duplicating entries. + pub(crate) fn from_names(names: impl IntoIterator>) -> Self { + let mut dirs = IndexSet::new(); + for name in names { + let trimmed = name.as_ref().trim(); + if trimmed.is_empty() { + continue; + } + dirs.insert(normalise_name(trimmed)); + } + Self { dirs } + } +} + +impl Hash for WorkspaceSkipList { + fn hash(&self, state: &mut H) { + let mut dirs: Vec<&String> = self.dirs.iter().collect(); + dirs.sort_unstable(); + for dir in dirs { + dir.hash(state); + } + } +} + +impl Default for WorkspaceSkipList { + fn default() -> Self { + Self::from_defaults() + } +} + +pub(super) fn search_workspace( + env: &EnvSnapshot, + command: &str, + collect_all: bool, + skip_dirs: &WorkspaceSkipList, +) -> Result, Error> { + if !workspace_fallback_enabled() { + tracing::debug!( + %command, + env = WORKSPACE_FALLBACK_ENV, + "workspace which fallback disabled via env override", + ); + return Ok(Vec::new()); + } + + tracing::debug!( + %command, + max_depth = WORKSPACE_MAX_DEPTH, + skip = ?skip_dirs, + "using workspace which fallback", + ); + + platform_search_workspace(env, command, collect_all, skip_dirs) +} + +pub(super) fn should_visit_entry(entry: &walkdir::DirEntry, skip_dirs: &WorkspaceSkipList) -> bool { + if !entry.file_type().is_dir() { + return true; + } + let name = entry.file_name().to_string_lossy(); + !skip_dirs.contains(&name) +} + +fn workspace_fallback_enabled() -> bool { + match env::var(WORKSPACE_FALLBACK_ENV) { + Ok(value) => { + let normalised = value.to_ascii_lowercase(); + !matches!(normalised.as_str(), "0" | "false" | "off") + } + Err(env::VarError::NotPresent) => true, + Err(env::VarError::NotUnicode(_)) => { + tracing::warn!( + env = WORKSPACE_FALLBACK_ENV, + "workspace fallback disabled because env var is not valid UTF-8", + ); + false + } + } +} + +/// Convert a walkdir item to an entry, logging and skipping unreadable paths to +/// keep workspace traversal robust across platforms. +pub(super) fn unwrap_or_log_error( + walk_entry: Result, + command: &str, +) -> Option { + match walk_entry { + Ok(entry) => Some(entry), + Err(err) => { + tracing::debug!( + %command, + error = %err, + "skipping unreadable workspace entry during which fallback", + ); + None + } + } +} + +/// Emit a debug message when fallback traversal yields no matches, helping +/// callers diagnose unexpected latency or misses. +pub(super) fn log_if_no_matches( + matches: &[Utf8PathBuf], + command: &str, + skip_dirs: &WorkspaceSkipList, +) { + if matches.is_empty() { + tracing::debug!( + %command, + max_depth = WORKSPACE_MAX_DEPTH, + skip = ?skip_dirs, + "workspace which fallback found no matches", + ); + } +} + +#[cfg(windows)] +fn normalise_name(name: &str) -> String { + name.to_ascii_lowercase() +} + +#[cfg(not(windows))] +fn normalise_name(name: &str) -> String { + name.to_owned() +} diff --git a/src/stdlib/which/lookup/workspace/posix.rs b/src/stdlib/which/lookup/workspace/posix.rs new file mode 100644 index 00000000..7caca884 --- /dev/null +++ b/src/stdlib/which/lookup/workspace/posix.rs @@ -0,0 +1,82 @@ +//! POSIX workspace traversal for the `which` fallback. + +use camino::Utf8PathBuf; +use minijinja::{Error, ErrorKind}; +use walkdir::WalkDir; + +use super::super::is_executable; +use super::{ + WORKSPACE_MAX_DEPTH, WorkspaceSkipList, log_if_no_matches, should_visit_entry, + unwrap_or_log_error, +}; +use crate::stdlib::which::env::EnvSnapshot; + +pub(super) fn search_workspace( + env: &EnvSnapshot, + command: &str, + collect_all: bool, + skip_dirs: &WorkspaceSkipList, +) -> Result, Error> { + let walker = WalkDir::new(&env.cwd) + .follow_links(false) + .max_depth(WORKSPACE_MAX_DEPTH) + .sort_by_file_name() + .into_iter() + .filter_entry(|entry| should_visit_entry(entry, skip_dirs)); + + let matches = collect_matching_executables(walker, command, collect_all, skip_dirs)?; + log_if_no_matches(&matches, command, skip_dirs); + Ok(matches) +} + +fn collect_matching_executables( + walker: impl Iterator>, + command: &str, + collect_all: bool, + skip_dirs: &WorkspaceSkipList, +) -> Result, Error> { + let mut matches = Vec::new(); + + for walk_entry in walker { + let Some(entry) = unwrap_or_log_error(walk_entry, command) else { + continue; + }; + + if let Some(path) = process_workspace_entry(entry, command, skip_dirs)? { + matches.push(path); + if !collect_all { + break; + } + } + } + + Ok(matches) +} + +fn process_workspace_entry( + entry: walkdir::DirEntry, + command: &str, + _skip_dirs: &WorkspaceSkipList, +) -> Result, Error> { + if !entry.file_type().is_file() { + return Ok(None); + } + + let file_name = entry.file_name().to_string_lossy(); + if file_name != command { + return Ok(None); + } + + let path = entry.into_path(); + let utf8 = Utf8PathBuf::from_path_buf(path).map_err(|path_buf| { + let lossy_path = path_buf.to_string_lossy(); + Error::new( + ErrorKind::InvalidOperation, + format!( + "workspace path contains non-UTF-8 components while resolving command '{command}': {lossy_path}", + ), + ) + })?; + + Ok(is_executable(&utf8).then_some(utf8)) +} diff --git a/src/stdlib/which/lookup/workspace/windows.rs b/src/stdlib/which/lookup/workspace/windows.rs new file mode 100644 index 00000000..b4757dbc --- /dev/null +++ b/src/stdlib/which/lookup/workspace/windows.rs @@ -0,0 +1,147 @@ +//! Windows workspace traversal for the `which` fallback. + +use std::collections::HashSet; + +use camino::{Utf8Path, Utf8PathBuf}; +use minijinja::{Error, ErrorKind}; +use walkdir::WalkDir; + +use super::super::is_executable; +use super::{ + WORKSPACE_MAX_DEPTH, WorkspaceSkipList, log_if_no_matches, should_visit_entry, + unwrap_or_log_error, +}; +use crate::stdlib::which::env::{self, EnvSnapshot}; + +/// Encapsulates the state and logic for collecting matching executables during +/// workspace traversal. +struct CollectionState { + matches: Vec, + collect_all: bool, +} + +impl CollectionState { + fn new(collect_all: bool) -> Self { + Self { + matches: Vec::new(), + collect_all, + } + } + + /// Process an entry and add it to matches if valid. Returns `true` if + /// collection should stop (i.e., a match was found and `collect_all` is + /// `false`). + fn try_add( + &mut self, + entry: walkdir::DirEntry, + command: &str, + ctx: &WorkspaceMatchContext, + ) -> Result { + if let Some(path) = match_workspace_entry(entry, command, ctx)? { + self.matches.push(path); + if !self.collect_all { + return Ok(true); + } + } + + Ok(false) + } +} + +pub(super) fn search_workspace( + env: &EnvSnapshot, + command: &str, + collect_all: bool, + skip_dirs: &WorkspaceSkipList, +) -> Result, Error> { + let match_ctx = WorkspaceMatchContext::new(command, env); + let mut collector = CollectionState::new(collect_all); + + for walk_entry in WalkDir::new(&env.cwd) + .follow_links(false) + .max_depth(WORKSPACE_MAX_DEPTH) + .sort_by_file_name() + .into_iter() + .filter_entry(|entry| should_visit_entry(entry, skip_dirs)) + { + let Some(entry) = unwrap_or_log_error(walk_entry, command) else { + continue; + }; + + if collector.try_add(entry, command, &match_ctx)? { + break; + } + } + + log_if_no_matches(&collector.matches, command, skip_dirs); + + Ok(collector.matches) +} + +#[derive(Clone)] +struct WorkspaceMatchContext { + command_lower: String, + command_has_ext: bool, + basenames: HashSet, +} + +fn match_workspace_entry( + entry: walkdir::DirEntry, + command: &str, + ctx: &WorkspaceMatchContext, +) -> Result, Error> { + if !entry.file_type().is_file() { + return Ok(None); + } + + let file_name = entry.file_name().to_string_lossy().to_ascii_lowercase(); + if !name_matches(&file_name, ctx) { + return Ok(None); + } + + let path = entry.into_path(); + let utf8 = Utf8PathBuf::from_path_buf(path).map_err(|path_buf| { + let lossy_path = path_buf.to_string_lossy(); + Error::new( + ErrorKind::InvalidOperation, + format!( + "workspace path contains non-UTF-8 components while resolving command '{command}': {lossy_path}", + ), + ) + })?; + + Ok(is_executable(&utf8).then_some(utf8)) +} + +impl WorkspaceMatchContext { + fn new(command: &str, env: &EnvSnapshot) -> Self { + let command_lower = command.to_ascii_lowercase(); + let command_has_ext = command_lower.contains('.'); + let mut basenames = HashSet::new(); + + if !command_has_ext { + let candidates = env::candidate_paths(Utf8Path::new(""), &command_lower, env.pathext()); + for candidate in candidates { + if let Some(name) = Utf8Path::new(candidate.as_str()).file_name() { + basenames.insert(name.to_ascii_lowercase()); + } + } + } + + Self { + command_lower, + command_has_ext, + basenames, + } + } +} + +fn name_matches(file_name: &str, ctx: &WorkspaceMatchContext) -> bool { + if file_name == ctx.command_lower { + return true; + } + if ctx.command_has_ext { + return false; + } + ctx.basenames.contains(file_name) +} diff --git a/src/stdlib/which/mod.rs b/src/stdlib/which/mod.rs index ae4add82..1d9b9106 100644 --- a/src/stdlib/which/mod.rs +++ b/src/stdlib/which/mod.rs @@ -18,7 +18,7 @@ mod env; mod error; mod lookup; mod options; -pub(crate) use lookup::{DEFAULT_WORKSPACE_SKIP_DIRS, WorkspaceSkipList}; +pub(crate) use lookup::{WORKSPACE_SKIP_DIRS, WorkspaceSkipList}; pub(crate) use cache::WhichResolver; pub(crate) use options::WhichOptions; diff --git a/test_support/src/env.rs b/test_support/src/env.rs index 1b3264d9..b6e3b53f 100644 --- a/test_support/src/env.rs +++ b/test_support/src/env.rs @@ -118,6 +118,24 @@ pub struct VarGuard { inner: EnvGuard, } +/// Run `action` with `PATH` temporarily set to `value`, restoring on drop and +/// serialising mutations with `EnvLock`. +pub fn with_isolated_path(value: &OsStr, action: impl FnOnce() -> T) -> T { + let _lock = EnvLock::acquire(); + let original = std::env::var_os("PATH"); + // SAFETY: EnvLock serialises mutations to global process state. + unsafe { std::env::set_var("PATH", value) }; + + let output = action(); + + match original { + Some(orig) => unsafe { std::env::set_var("PATH", orig) }, + None => unsafe { std::env::remove_var("PATH") }, + } + + output +} + impl VarGuard { /// Set `key` to `value`, returning a guard that resets it on drop. /// diff --git a/tests/std_filter_tests/mod.rs b/tests/std_filter_tests/mod.rs index cc503ec6..da8d4c98 100644 --- a/tests/std_filter_tests/mod.rs +++ b/tests/std_filter_tests/mod.rs @@ -12,6 +12,8 @@ mod io_filters; mod network_functions; #[path = "std_filter_tests/path_filters.rs"] mod path_filters; +#[path = "std_filter_tests/which_filter_common.rs"] +mod which_filter_common; #[path = "std_filter_tests/which_filter_tests.rs"] mod which_filter_tests; #[path = "std_filter_tests/support.rs"] diff --git a/tests/std_filter_tests/which_filter_common.rs b/tests/std_filter_tests/which_filter_common.rs new file mode 100644 index 00000000..883e70c5 --- /dev/null +++ b/tests/std_filter_tests/which_filter_common.rs @@ -0,0 +1,180 @@ +//! Shared fixtures for the `which` filter/function integration tests. + +use anyhow::{Context, Result, anyhow}; +use camino::{Utf8Path, Utf8PathBuf}; +use minijinja::{context, Environment}; +use std::ffi::{OsStr, OsString}; + +use test_support::{env::VarGuard, env_lock::EnvLock}; + +use super::support::{self, fallible}; + +#[derive(Debug, Clone)] +pub(crate) struct ToolName(String); + +impl ToolName { + pub(crate) fn new(name: impl Into) -> Self { Self(name.into()) } + pub(crate) fn as_str(&self) -> &str { &self.0 } +} + +impl From<&str> for ToolName { + fn from(s: &str) -> Self { Self(s.to_owned()) } +} + +impl AsRef for ToolName { + fn as_ref(&self) -> &str { &self.0 } +} + +#[derive(Debug, Clone)] +pub(crate) struct DirName(String); + +impl DirName { + pub(crate) fn new(name: impl Into) -> Self { Self(name.into()) } + pub(crate) fn as_str(&self) -> &str { &self.0 } +} + +impl From<&str> for DirName { + fn from(s: &str) -> Self { Self(s.to_owned()) } +} + +impl AsRef for DirName { + fn as_ref(&self) -> &str { &self.0 } +} + +impl AsRef for DirName { + fn as_ref(&self) -> &OsStr { OsStr::new(&self.0) } +} + +#[derive(Debug, Clone)] +pub(crate) struct Template(String); + +impl Template { + pub(crate) fn new(template: impl Into) -> Self { Self(template.into()) } + pub(crate) fn as_str(&self) -> &str { &self.0 } +} + +impl From<&str> for Template { + fn from(s: &str) -> Self { Self(s.to_owned()) } +} + +impl AsRef for Template { + fn as_ref(&self) -> &str { &self.0 } +} + +pub(crate) struct PathEnv { + _lock: EnvLock, + path_guard: VarGuard, + #[cfg(windows)] + pathext_guard: VarGuard, +} + +impl PathEnv { + pub(crate) fn new(entries: &[Utf8PathBuf]) -> Result { + let lock = EnvLock::acquire(); + let joined = if entries.is_empty() { + OsString::new() + } else { + std::env::join_paths(entries.iter().map(|entry| entry.as_std_path())) + .context("join PATH entries")? + }; + let path_guard = VarGuard::set("PATH", joined.as_os_str()); + #[cfg(windows)] + let pathext_guard = VarGuard::set("PATHEXT", OsStr::new(".cmd;.exe")); + Ok(Self { + _lock: lock, + path_guard, + #[cfg(windows)] + pathext_guard, + }) + } +} + +pub(crate) fn write_tool(dir: &Utf8Path, name: &ToolName) -> Result { + let filename = tool_name(name); + let path = dir.join(Utf8Path::new(&filename)); + let parent = path + .parent() + .context("tool path should have a parent directory")?; + std::fs::create_dir_all(parent.as_std_path()) + .with_context(|| format!("create parent for {path:?}"))?; + std::fs::write(path.as_std_path(), script_contents()) + .with_context(|| format!("write fixture {path:?}"))?; + mark_executable(&path)?; + Ok(path) +} + +#[cfg(unix)] +fn mark_executable(path: &Utf8Path) -> Result<()> { + use std::os::unix::fs::PermissionsExt; + let mut perms = std::fs::metadata(path.as_std_path()) + .with_context(|| format!("stat {path:?}"))? + .permissions(); + perms.set_mode(0o755); + std::fs::set_permissions(path.as_std_path(), perms) + .with_context(|| format!("chmod {path:?}")) +} + +#[cfg(not(unix))] +fn mark_executable(_path: &Utf8Path) -> Result<()> { Ok(()) } + +#[cfg(windows)] +fn tool_name(base: &ToolName) -> String { format!("{}.cmd", base.as_str()) } + +#[cfg(not(windows))] +fn tool_name(base: &ToolName) -> String { base.as_str().to_owned() } + +fn script_contents() -> &'static [u8] { + #[cfg(windows)] + { b"@echo off\r\n" } + #[cfg(not(windows))] + { b"#!/bin/sh\nexit 0\n" } +} + +pub(crate) fn render(env: &mut Environment<'_>, template: &Template) -> Result { + env.render_str(template.as_str(), context! {}) + .map_err(|err| anyhow!(err.to_string())) +} + +pub(crate) struct WhichTestFixture { + _temp: tempfile::TempDir, + pub(crate) env: Environment<'static>, + pub(crate) state: netsuke::stdlib::StdlibState, + pub(crate) paths: Vec, + _path_env: PathEnv, +} + +impl WhichTestFixture { + pub(crate) fn with_tool_in_dirs(tool_name: &ToolName, dir_names: &[DirName]) -> Result { + let (temp, root) = support::filter_workspace()?; + let mut dirs = Vec::new(); + let mut tool_paths = Vec::new(); + for dir_name in dir_names { + let dir = root.join(dir_name.as_str()); +<<<<<<< HEAD + std::fs::create_dir_all(dir.as_std_path()).with_context(|| { + format!("create directory {}", dir) + })?; +======= + std::fs::create_dir_all(dir.as_std_path())?; +>>>>>>> aadfa2e (feat(which): add workspace fallback and improve direct path resolution) + let tool_path = write_tool(&dir, tool_name)?; + dirs.push(dir); + tool_paths.push(tool_path); + } + let path_env = PathEnv::new(&dirs)?; + let (env, state) = fallible::stdlib_env_with_state()?; + Ok(Self { + _temp: temp, + env, + state, + paths: tool_paths, + _path_env: path_env, + }) + } + + pub(crate) fn render(&mut self, template: &Template) -> Result { + self.env + .render_str(template.as_str(), context! {}) + .map_err(|err| anyhow!(err.to_string())) + } +} diff --git a/tests/std_filter_tests/which_filter_tests.rs b/tests/std_filter_tests/which_filter_tests.rs index 1a46f498..61583e5c 100644 --- a/tests/std_filter_tests/which_filter_tests.rs +++ b/tests/std_filter_tests/which_filter_tests.rs @@ -1,190 +1,132 @@ //! Integration tests for the `which` filter/function covering PATH resolution, //! canonicalisation, cwd behaviour, workspace fallback, and diagnostic output. -use anyhow::{Context, Result, anyhow}; -use camino::{Utf8Path, Utf8PathBuf}; -use minijinja::{context, Environment}; -use rstest::rstest; -use std::ffi::{OsStr, OsString}; -use std::env; +use anyhow::{Context, Result}; use netsuke::stdlib::StdlibConfig; +use rstest::rstest; +use serde_json::Value; +use std::{env, path::PathBuf}; use tempfile::tempdir; -use test_support::{env::VarGuard, env_lock::EnvLock}; +use test_support::env_lock::EnvLock; use super::support::{self, fallible}; +use super::which_filter_common::*; -#[derive(Debug, Clone)] -struct ToolName(String); - -impl ToolName { - fn new(name: impl Into) -> Self { Self(name.into()) } - fn as_str(&self) -> &str { &self.0 } -} - -impl From<&str> for ToolName { - fn from(s: &str) -> Self { Self(s.to_owned()) } -} - -impl AsRef for ToolName { - fn as_ref(&self) -> &str { &self.0 } -} - -#[derive(Debug, Clone)] -struct DirName(String); - -impl DirName { - fn new(name: impl Into) -> Self { Self(name.into()) } - fn as_str(&self) -> &str { &self.0 } -} - -impl From<&str> for DirName { - fn from(s: &str) -> Self { Self(s.to_owned()) } -} - -impl AsRef for DirName { - fn as_ref(&self) -> &str { &self.0 } -} - -impl AsRef for DirName { - fn as_ref(&self) -> &OsStr { OsStr::new(&self.0) } -} - -#[derive(Debug, Clone)] -struct Template(String); - -impl Template { - fn new(template: impl Into) -> Self { Self(template.into()) } - fn as_str(&self) -> &str { &self.0 } -} - -impl From<&str> for Template { - fn from(s: &str) -> Self { Self(s.to_owned()) } -} - -impl AsRef for Template { - fn as_ref(&self) -> &str { &self.0 } -} - -struct PathEnv { +struct CurrentDirGuard { + original: PathBuf, _lock: EnvLock, - path_guard: VarGuard, - #[cfg(windows)] - pathext_guard: VarGuard, } -impl PathEnv { - fn new(entries: &[Utf8PathBuf]) -> Result { +impl CurrentDirGuard { + fn change_to(path: &std::path::Path) -> Result { let lock = EnvLock::acquire(); - let joined = if entries.is_empty() { - OsString::new() - } else { - std::env::join_paths(entries.iter().map(|entry| entry.as_std_path())) - .context("join PATH entries")? - }; - let path_guard = VarGuard::set("PATH", joined.as_os_str()); - #[cfg(windows)] - let pathext_guard = VarGuard::set("PATHEXT", OsStr::new(".cmd;.exe")); + let original = env::current_dir().context("capture current working directory")?; + env::set_current_dir(path).context("switch cwd")?; Ok(Self { + original, _lock: lock, - path_guard, - #[cfg(windows)] - pathext_guard, }) } } -fn write_tool(dir: &Utf8Path, name: &ToolName) -> Result { - let filename = tool_name(name); - let path = dir.join(Utf8Path::new(&filename)); - let parent = path - .parent() - .context("tool path should have a parent directory")?; - std::fs::create_dir_all(parent.as_std_path()) - .with_context(|| format!("create parent for {:?}", path))?; - std::fs::write(path.as_std_path(), script_contents()) - .with_context(|| format!("write fixture {:?}", path))?; - mark_executable(&path)?; - Ok(path) -} - -#[cfg(unix)] -fn mark_executable(path: &Utf8Path) -> Result<()> { - use std::os::unix::fs::PermissionsExt; - let mut perms = std::fs::metadata(path.as_std_path()) - .with_context(|| format!("stat {:?}", path))? - .permissions(); - perms.set_mode(0o755); - std::fs::set_permissions(path.as_std_path(), perms) - .with_context(|| format!("chmod {:?}", path)) +impl Drop for CurrentDirGuard { + fn drop(&mut self) { + if let Err(err) = env::set_current_dir(&self.original) { + tracing::warn!( + "failed to restore working directory to {}: {err}", + self.original.display() + ); + } + } } -#[cfg(not(unix))] -fn mark_executable(_path: &Utf8Path) -> Result<()> { - Ok(()) +fn render_and_assert_pure( + fixture: &mut WhichTestFixture, + template: &Template, +) -> Result { + fixture.state.reset_impure(); + let output = fixture.render(template)?; + assert!(!fixture.state.is_impure()); + Ok(output) } -#[cfg(windows)] -fn tool_name(base: &ToolName) -> String { - format!("{}.cmd", base.as_str()) -} +fn test_cache_after_removal( + fixture: &mut WhichTestFixture, + first_template: &Template, + second_template: &Template, + expect_second_err: bool, +) -> Result<()> { + fixture.state.reset_impure(); + let removed_path = &fixture.paths[0]; + let first = fixture.render(first_template)?; + assert_eq!(first, removed_path.as_str()); -#[cfg(not(windows))] -fn tool_name(base: &ToolName) -> String { - base.as_str().to_owned() -} + std::fs::remove_file(removed_path)?; -fn script_contents() -> &'static [u8] { - #[cfg(windows)] - { - b"@echo off\r\n" - } - #[cfg(not(windows))] - { - b"#!/bin/sh\nexit 0\n" + fixture.state.reset_impure(); + let second_result = fixture.render(second_template); + + if expect_second_err { + let err = second_result.expect_err("expected fresh which lookup to fail after removal"); + assert!(err.to_string().contains("not_found")); + } else { + let second = second_result?; + assert_eq!(second, removed_path.as_str()); } -} -fn render(env: &mut Environment<'_>, template: &Template) -> Result { - env.render_str(template.as_str(), context! {}) - .map_err(|err| anyhow!(err.to_string())) + Ok(()) } -struct WhichTestFixture { - _temp: tempfile::TempDir, - env: Environment<'static>, - state: netsuke::stdlib::StdlibState, - paths: Vec, - _path_env: PathEnv, +/// Helper to set up a fixture for cache removal tests and run the test. +fn test_cache_behaviour_after_removal( + second_template_str: &str, + expect_second_err: bool, +) -> Result<()> { + let mut fixture = WhichTestFixture::with_tool_in_dirs( + &ToolName::from("helper"), + &[DirName::from("bin_first"), DirName::from("bin_second")], + )?; + + test_cache_after_removal( + &mut fixture, + &Template::from("{{ 'helper' | which }}"), + &Template::from(second_template_str), + expect_second_err, + ) } -impl WhichTestFixture { - fn with_tool_in_dirs(tool_name: &ToolName, dir_names: &[DirName]) -> Result { - let (temp, root) = support::filter_workspace()?; - let mut dirs = Vec::new(); - let mut tool_paths = Vec::new(); - for dir_name in dir_names { - let dir = root.join(dir_name.as_str()); - std::fs::create_dir_all(dir.as_std_path())?; - let tool_path = write_tool(&dir, tool_name)?; - dirs.push(dir); - tool_paths.push(tool_path); - } - let path_env = PathEnv::new(&dirs)?; - let (env, state) = fallible::stdlib_env_with_state()?; - Ok(Self { - _temp: temp, - env, - state, - paths: tool_paths, - _path_env: path_env, - }) - } +fn test_duplicate_paths( + fixture: &mut WhichTestFixture, + canonical: bool, + expected_count: usize, +) -> Result<()> { + let template = if canonical { + Template::from("{{ 'helper' | which(all=true, canonical=true) | join('|') }}") + } else { + Template::from("{{ 'helper' | which(all=true, canonical=false) | join('|') }}") + }; + + let output = render_and_assert_pure(fixture, &template)?; + let parts: Vec<&str> = output.split('|').collect(); - fn render(&mut self, template: &Template) -> Result { - self.env - .render_str(template.as_str(), context! {}) - .map_err(|err| anyhow!(err.to_string())) + assert_eq!(parts.len(), expected_count); + for part in &parts { + assert_eq!(*part, fixture.paths[0].as_str()); } + + Ok(()) +} + +/// Helper to test cwd_mode resolution with a given case variant. +fn test_cwd_mode_resolution(cwd_mode_value: &str) -> Result<()> { + let (_temp, root) = support::filter_workspace()?; + let tool = write_tool(&root, &ToolName::from("local"))?; + let _path = PathEnv::new(&[])?; + let (mut env, _state) = fallible::stdlib_env_with_state()?; + let template = Template::from(format!("{{{{ which('local', cwd_mode='{cwd_mode_value}') }}}}")); + let output = render(&mut env, &template)?; + assert_eq!(output, tool.as_str()); + Ok(()) } #[rstest] @@ -193,20 +135,31 @@ fn which_filter_returns_first_match() -> Result<()> { &ToolName::from("helper"), &[DirName::from("bin_first"), DirName::from("bin_second")], )?; - fixture.state.reset_impure(); - let output = fixture.render(&Template::from("{{ 'helper' | which }}"))?; + let output = render_and_assert_pure(&mut fixture, &Template::from("{{ 'helper' | which }}"))?; assert_eq!(output, fixture.paths[0].as_str()); - assert!(!fixture.state.is_impure()); Ok(()) } +#[rstest] +fn which_filter_uses_cached_result_when_executable_removed() -> Result<()> { + test_cache_behaviour_after_removal("{{ 'helper' | which }}", false) +} + +#[rstest] +fn which_filter_fresh_bypasses_cache_after_executable_removed() -> Result<()> { + test_cache_behaviour_after_removal("{{ 'helper' | which(fresh=true) }}", true) +} + #[rstest] fn which_filter_all_returns_all_matches() -> Result<()> { let mut fixture = WhichTestFixture::with_tool_in_dirs( &ToolName::from("helper"), &[DirName::from("bin_a"), DirName::from("bin_b")], )?; - let output = fixture.render(&Template::from("{{ 'helper' | which(all=true) | join('|') }}"))?; + let output = render_and_assert_pure( + &mut fixture, + &Template::from("{{ 'helper' | which(all=true) | join('|') }}"), + )?; let expected = format!( "{}|{}", fixture.paths[0].as_str(), @@ -217,55 +170,73 @@ fn which_filter_all_returns_all_matches() -> Result<()> { } #[rstest] -fn which_filter_all_with_duplicates_respects_canonical_false() -> Result<()> { +fn which_filter_all_returns_list() -> Result<()> { let mut fixture = WhichTestFixture::with_tool_in_dirs( &ToolName::from("helper"), - &[DirName::from("bin"), DirName::from("bin")], + &[DirName::from("bin_a"), DirName::from("bin_b")], )?; - fixture.state.reset_impure(); - let output = fixture.render(&Template::from( - "{{ 'helper' | which(all=true, canonical=false) | join('|') }}", - ))?; - let path = fixture.paths[0].as_str(); - let parts: Vec<&str> = output.split('|').collect(); + let output = fixture.render(&Template::from("{{ which('helper', all=true) | tojson }}"))?; + let value: Value = serde_json::from_str(&output)?; + assert!( + value.is_array(), + "expected which(..., all=true) to return a JSON array, got {value:?}", + ); - assert_eq!(parts, vec![path, path]); - assert!(!fixture.state.is_impure()); Ok(()) } #[rstest] -fn which_filter_all_with_duplicates_deduplicates_canonicalised_paths() -> Result<()> { +fn which_filter_all_with_duplicates_respects_canonical_false() -> Result<()> { let mut fixture = WhichTestFixture::with_tool_in_dirs( &ToolName::from("helper"), &[DirName::from("bin"), DirName::from("bin")], )?; - fixture.state.reset_impure(); - let output = fixture.render(&Template::from( - "{{ 'helper' | which(all=true, canonical=true) | join('|') }}", - ))?; - - let path = fixture.paths[0].as_str(); - let parts: Vec<&str> = output.split('|').collect(); + test_duplicate_paths(&mut fixture, false, 2) +} - assert_eq!(parts, vec![path]); - assert!(!fixture.state.is_impure()); - Ok(()) +#[rstest] +fn which_filter_all_with_duplicates_deduplicates_canonicalised_paths() -> Result<()> { + let mut fixture = WhichTestFixture::with_tool_in_dirs( + &ToolName::from("helper"), + &[DirName::from("bin"), DirName::from("bin")], + )?; + test_duplicate_paths(&mut fixture, true, 1) } #[rstest] fn which_function_honours_cwd_mode() -> Result<()> { - let (_temp, root) = support::filter_workspace()?; - let tool = write_tool(&root, &ToolName::from("local"))?; + test_cwd_mode_resolution("always") +} + +#[rstest] +fn which_function_rejects_invalid_cwd_mode() -> Result<()> { + let (_temp, _root) = support::filter_workspace()?; let _path = PathEnv::new(&[])?; let (mut env, _state) = fallible::stdlib_env_with_state()?; - let template = Template::from("{{ which('local', cwd_mode='always') }}"); - let output = render(&mut env, &template)?; - assert_eq!(output, tool.as_str()); + let template = Template::from("{{ which('local', cwd_mode='invalid') }}"); + + let err = render(&mut env, &template) + .expect_err("expected invalid cwd_mode to fail"); + + let message = err.to_string(); + assert!( + message.contains("netsuke::jinja::which::args"), + "expected which args error, got: {message}", + ); + assert!( + message.contains("cwd_mode"), + "expected message to mention cwd_mode, got: {message}", + ); + Ok(()) } +#[rstest] +fn which_function_accepts_case_insensitive_cwd_mode() -> Result<()> { + test_cwd_mode_resolution("ALWAYS") +} + #[rstest] fn which_filter_reports_missing_command() -> Result<()> { let (_temp, _root) = support::filter_workspace()?; @@ -273,7 +244,7 @@ fn which_filter_reports_missing_command() -> Result<()> { let (mut env, _state) = fallible::stdlib_env_with_state()?; let err = env .render_str("{{ 'absent' | which }}", context! {}) - .unwrap_err(); + .expect_err("render should fail for missing command"); let message = err.to_string(); assert!(message.contains("netsuke::jinja::which::not_found")); Ok(()) @@ -300,7 +271,7 @@ fn which_filter_skips_heavy_directories() -> Result<()> { let (mut env, _state) = fallible::stdlib_env_with_state()?; let err = env .render_str("{{ 'helper' | which }}", context! {}) - .unwrap_err(); + .expect_err("render should fail when tool is in skipped directory"); assert!(err.to_string().contains("not_found")); Ok(()) } @@ -311,8 +282,7 @@ fn which_resolver_honours_workspace_root_override() -> Result<()> { let (_temp, root) = support::filter_workspace()?; let tool = write_tool(&root, &ToolName::from("helper"))?; let alt = tempdir().context("create alternate cwd")?; - let orig_cwd = env::current_dir().context("capture cwd")?; - env::set_current_dir(&alt).context("switch cwd")?; + let _cwd_guard = CurrentDirGuard::change_to(alt.path())?; let config = StdlibConfig::new( Dir::open_ambient_dir(&root, ambient_authority()).context("open workspace")?, @@ -320,9 +290,7 @@ fn which_resolver_honours_workspace_root_override() -> Result<()> { .with_workspace_root_path(root.clone())?; let _path = PathEnv::new(&[])?; let (mut env, _state) = fallible::stdlib_env_with_config(config)?; - let render_result = render(&mut env, &Template::from("{{ 'helper' | which }}")); - env::set_current_dir(orig_cwd).context("restore cwd")?; - let output = render_result?; + let output = render(&mut env, &Template::from("{{ 'helper' | which }}"))?; assert_eq!(output, tool.as_str()); Ok(()) }