refactor(which/lookup/workspace): improve workspace fallback robustness and logging

- Added helper functions to unwrap walkdir entries with error logging and to log when no matches are found during fallback.
- Rewrote workspace traversal to be more resilient by filtering unreadable paths and better handling non-UTF-8 paths.
- Simplified collection of executable matches, removing state struct in favor of clearer iteration logic.
- Enhanced logging to aid diagnosis of fallback misses or unexpected latencies.
- Adjusted test code for improved PATH handling and error expectation assertion.

These changes improve the maintainability and robustness of the workspace fallback which resolution mechanism, ensuring smoother operation across platforms and edge cases.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

Author: leynos

src/stdlib/which/lookup/tests.rs

@@ -163,8 +163,13 @@ fn relative_path_entries_resolve_against_cwd(workspace: TempWorkspace) -> Result
     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 = format!("{}:bin:tools", workspace.root());
-    let _guard = VarGuard::set("PATH", std::ffi::OsStr::new(&path_value));
+    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")?;

src/stdlib/which/lookup/workspace/mod.rs

@@ -61,6 +61,45 @@ fn workspace_fallback_enabled() -> bool {
             let normalised = value.to_ascii_lowercase();
             !matches!(normalised.as_str(), "0" | "false" | "off")
         }
-        Err(env::VarError::NotPresent | env::VarError::NotUnicode(_)) => true,
+        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<walkdir::DirEntry, walkdir::Error>,
+    command: &str,
+) -> Option<walkdir::DirEntry> {
+    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) {
+    if matches.is_empty() {
+        tracing::debug!(
+            %command,
+            max_depth = WORKSPACE_MAX_DEPTH,
+            skip = ?WORKSPACE_SKIP_DIRS,
+            "workspace which fallback found no matches",
+        );
     }
 }

src/stdlib/which/lookup/workspace/posix.rs

@@ -5,69 +5,23 @@ use minijinja::{Error, ErrorKind};
 use walkdir::WalkDir;
 
 use super::super::is_executable;
-use super::{WORKSPACE_MAX_DEPTH, WORKSPACE_SKIP_DIRS, should_visit_entry};
+use super::{WORKSPACE_MAX_DEPTH, 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,
 ) -> Result<Vec<Utf8PathBuf>, Error> {
-    let walker = WalkDir::new(&env.cwd)
+    let mut matches = Vec::new();
+
+    for walk_entry in WalkDir::new(&env.cwd)
         .follow_links(false)
         .max_depth(WORKSPACE_MAX_DEPTH)
         .sort_by_file_name()
         .into_iter()
-        .filter_entry(should_visit_entry);
-
-    let matches = collect_matching_executables(walker, command, collect_all)?;
-
-    log_if_empty(&matches, command);
-
-    Ok(matches)
-}
-
-/// Convert a `WalkDir` result into an entry, logging and skipping unreadable
-/// paths to keep workspace traversal resilient.
-fn unwrap_or_log_error(
-    walk_entry: Result<walkdir::DirEntry, walkdir::Error>,
-    command: &str,
-) -> Option<walkdir::DirEntry> {
-    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 log when the workspace traversal yields no executables.
-fn log_if_empty(matches: &[Utf8PathBuf], command: &str) {
-    if matches.is_empty() {
-        tracing::debug!(
-            %command,
-            max_depth = WORKSPACE_MAX_DEPTH,
-            skip = ?WORKSPACE_SKIP_DIRS,
-            "workspace which fallback found no matches",
-        );
-    }
-}
-
-/// Traverse the workspace iterator, collecting executable matches and stopping
-/// early when `collect_all` is `false` and a match is discovered.
-fn collect_matching_executables(
-    entries: impl Iterator<Item = Result<walkdir::DirEntry, walkdir::Error>>,
-    command: &str,
-    collect_all: bool,
-) -> Result<Vec<Utf8PathBuf>, Error> {
-    let mut matches = Vec::new();
-
-    for walk_entry in entries {
+        .filter_entry(should_visit_entry)
+    {
         let Some(entry) = unwrap_or_log_error(walk_entry, command) else {
             continue;
         };
@@ -80,6 +34,8 @@ fn collect_matching_executables(
         }
     }
 
+    log_if_no_matches(&matches, command);
+
     Ok(matches)
 }
 

src/stdlib/which/lookup/workspace/windows.rs

@@ -7,7 +7,10 @@ use minijinja::{Error, ErrorKind};
 use walkdir::WalkDir;
 
 use super::super::is_executable;
-use super::{WORKSPACE_MAX_DEPTH, WORKSPACE_SKIP_DIRS, should_visit_entry};
+use super::{
+    WORKSPACE_MAX_DEPTH, WORKSPACE_SKIP_DIRS, log_if_no_matches, should_visit_entry,
+    unwrap_or_log_error,
+};
 use crate::stdlib::which::env::{self, EnvSnapshot};
 
 pub(super) fn search_workspace(
@@ -16,7 +19,7 @@ pub(super) fn search_workspace(
     collect_all: bool,
 ) -> Result<Vec<Utf8PathBuf>, Error> {
     let match_ctx = WorkspaceMatchContext::new(command, env);
-    let mut collector = CollectionState::new(collect_all);
+    let mut matches = Vec::new();
 
     for walk_entry in WalkDir::new(&env.cwd)
         .follow_links(false)
@@ -29,14 +32,17 @@ pub(super) fn search_workspace(
             continue;
         };
 
-        if collector.try_add(entry, command, &match_ctx)? {
-            break;
+        if let Some(path) = match_workspace_entry(entry, command, &match_ctx)? {
+            matches.push(path);
+            if !collect_all {
+                break;
+            }
         }
     }
 
-    log_if_no_matches(&collector.matches, command);
+    log_if_no_matches(&matches, command);
 
-    Ok(collector.matches)
+    Ok(matches)
 }
 
 #[derive(Clone)]
@@ -46,38 +52,39 @@ struct WorkspaceMatchContext {
     basenames: HashSet<String>,
 }
 
-/// Encapsulates the state and logic for collecting matching executables during
-/// workspace traversal.
-struct CollectionState {
-    matches: Vec<Utf8PathBuf>,
-    collect_all: bool,
-}
-
-impl CollectionState {
-    fn new(collect_all: bool) -> Self {
-        Self {
-            matches: Vec::new(),
-            collect_all,
-        }
+fn match_workspace_entry(
+    entry: walkdir::DirEntry,
+    command: &str,
+    ctx: &WorkspaceMatchContext,
+) -> Result<Option<Utf8PathBuf>, Error> {
+    if !entry.file_type().is_file() {
+        return Ok(None);
     }
 
-    /// 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<bool, Error> {
-        if let Some(path) = process_workspace_entry(entry, command, ctx)? {
-            self.matches.push(path);
-            if !self.collect_all {
-                return Ok(true);
-            }
-        }
-        Ok(false)
+    let file_name = entry.file_name().to_string_lossy().to_ascii_lowercase();
+    let name_matches = if file_name == ctx.command_lower {
+        true
+    } else if ctx.command_has_ext {
+        false
+    } else {
+        ctx.basenames.contains(&file_name)
+    };
+    if !name_matches {
+        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 {
@@ -137,35 +144,3 @@ fn process_workspace_entry(
     })?;
     Ok(is_executable(&utf8).then_some(utf8))
 }
-
-/// Convert a `WalkDir` result into an entry, logging and skipping unreadable
-/// paths to keep workspace traversal robust.
-fn unwrap_or_log_error(
-    walk_entry: Result<walkdir::DirEntry, walkdir::Error>,
-    command: &str,
-) -> Option<walkdir::DirEntry> {
-    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.
-fn log_if_no_matches(matches: &[Utf8PathBuf], command: &str) {
-    if matches.is_empty() {
-        tracing::debug!(
-            %command,
-            max_depth = WORKSPACE_MAX_DEPTH,
-            skip = ?WORKSPACE_SKIP_DIRS,
-            "workspace which fallback found no matches",
-        );
-    }
-}

tests/std_filter_tests/which_filter_tests.rs

@@ -38,7 +38,7 @@ fn test_cache_after_removal(
     let second_result = fixture.render(second_template);
 
     if expect_second_err {
-        let err = second_result.expect("expected fresh which lookup to fail after removal");
+        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?;