From b35e3e4121f8a803c88e019c6f95ef831a75b8c6 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Tue, 10 Feb 2026 15:05:28 -0800 Subject: [PATCH 1/5] fix: ensure workspace environments are found when searchKind is provided --- crates/pet/src/find.rs | 84 +++++++++++++++++++++++++++++++++++++++ crates/pet/src/jsonrpc.rs | 71 ++++++++++++++++++--------------- 2 files changed, 124 insertions(+), 31 deletions(-) diff --git a/crates/pet/src/find.rs b/crates/pet/src/find.rs index 30d9a631..59a1eb9d 100644 --- a/crates/pet/src/find.rs +++ b/crates/pet/src/find.rs @@ -653,4 +653,88 @@ mod tests { "canonicalize() would resolve to target, but path() does not" ); } + + /// Test for https://github.com/microsoft/python-environment-tools/issues/151 + /// Verifies that refresh with searchKind (e.g., "Venv") still finds environments + /// in workspace directories, not just global locations. + /// + /// The bug was that when searchKind was provided, workspace_directories was cleared, + /// preventing discovery of workspace-based environments like venvs. + #[test] + fn test_search_kind_finds_workspace_venvs() { + use super::{find_and_report_envs, SearchScope}; + use crate::locators::create_locators; + use pet_conda::Conda; + use pet_core::os_environment::EnvironmentApi; + use pet_core::python_environment::PythonEnvironmentKind; + use pet_core::Configuration; + use pet_poetry::Poetry; + use pet_reporter::collect; + use std::sync::Arc; + + let tmp = TempDir::new().expect("Failed to create temp dir"); + let workspace = tmp.path().to_path_buf(); + + // Create a venv structure in the workspace + let venv_dir = workspace.join(".venv"); + #[cfg(windows)] + let bin_dir = venv_dir.join("Scripts"); + #[cfg(unix)] + let bin_dir = venv_dir.join("bin"); + fs::create_dir_all(&bin_dir).expect("Failed to create bin dir"); + + // Create pyvenv.cfg (required for venv detection) + fs::write( + venv_dir.join("pyvenv.cfg"), + "home = /usr/bin\nversion = 3.11.0\n", + ) + .expect("Failed to create pyvenv.cfg"); + + // Create python executable + #[cfg(windows)] + let python_exe = bin_dir.join("python.exe"); + #[cfg(unix)] + let python_exe = bin_dir.join("python"); + fs::write(&python_exe, "fake python").expect("Failed to create python exe"); + + // Set up locators and configuration + let environment = EnvironmentApi::new(); + let conda_locator = Arc::new(Conda::from(&environment)); + let poetry_locator = Arc::new(Poetry::from(&environment)); + let locators = create_locators(conda_locator, poetry_locator, &environment); + + let config = Configuration { + workspace_directories: Some(vec![workspace.clone()]), + ..Default::default() + }; + for locator in locators.iter() { + locator.configure(&config); + } + + let reporter = Arc::new(collect::create_reporter()); + + // Call find_and_report_envs with SearchScope::Global(Venv) + // This simulates the behavior when refresh is called with searchKind: "Venv" + find_and_report_envs( + reporter.as_ref(), + config, + &locators, + &environment, + Some(SearchScope::Global(PythonEnvironmentKind::Venv)), + ); + + let environments = reporter.environments.lock().unwrap(); + + // The venv should be discovered even when searching by kind + let venv_found = environments.iter().any(|env| { + env.kind == Some(PythonEnvironmentKind::Venv) + && env.prefix.as_ref().map(|p| p == &venv_dir).unwrap_or(false) + }); + + assert!( + venv_found, + "Venv in workspace should be found when searching by kind. Found environments: {:?}", + *environments + ); + } } diff --git a/crates/pet/src/jsonrpc.rs b/crates/pet/src/jsonrpc.rs index 3b056a21..fa7e04c3 100644 --- a/crates/pet/src/jsonrpc.rs +++ b/crates/pet/src/jsonrpc.rs @@ -211,43 +211,52 @@ pub fn handle_refresh(context: Arc, id: u32, params: Value) { let mut search_scope = None; - // If search kind is provided and no search_paths, then we will only search in the global locations. - if refresh_options.search_kind.is_some() || refresh_options.search_paths.is_some() { - // Always clear this, as we will either serach in specified folder or a specific kind in global locations. + // If search_paths is provided, limit search to those paths. + // If only search_kind is provided (without search_paths), we still search + // workspace directories because many environment types (like Venv, VirtualEnv) + // don't have global locations - they only exist in workspace folders. + // The reporter will filter results to only report the requested kind. + if let Some(search_paths) = refresh_options.search_paths { + // Clear workspace directories when explicit search paths are provided. config.workspace_directories = None; - if let Some(search_paths) = refresh_options.search_paths { - // Expand any glob patterns in the search paths - let expanded_paths = expand_glob_patterns(&search_paths); - trace!( - "Expanded {} search paths to {} paths", - search_paths.len(), - expanded_paths.len() - ); - // These workspace folders are only for this refresh. - config.workspace_directories = Some( - expanded_paths - .iter() - .filter(|p| p.is_dir()) - .cloned() - .collect(), - ); - config.executables = Some( - expanded_paths - .iter() - .filter(|p| p.is_file()) - .cloned() - .collect(), - ); - search_scope = Some(SearchScope::Workspace); - } else if let Some(search_kind) = refresh_options.search_kind { - config.executables = None; - search_scope = Some(SearchScope::Global(search_kind)); - } + // Expand any glob patterns in the search paths + let expanded_paths = expand_glob_patterns(&search_paths); + trace!( + "Expanded {} search paths to {} paths", + search_paths.len(), + expanded_paths.len() + ); + // These workspace folders are only for this refresh. + config.workspace_directories = Some( + expanded_paths + .iter() + .filter(|p| p.is_dir()) + .cloned() + .collect(), + ); + config.executables = Some( + expanded_paths + .iter() + .filter(|p| p.is_file()) + .cloned() + .collect(), + ); + search_scope = Some(SearchScope::Workspace); // Configure the locators with the modified config. for locator in context.locators.iter() { locator.configure(&config); } + } else if let Some(search_kind) = refresh_options.search_kind { + // When only search_kind is provided, keep workspace directories so that + // workspace-based environments (Venv, VirtualEnv, etc.) can be found. + // The search_scope tells find_and_report_envs to filter locators by kind. + search_scope = Some(SearchScope::Global(search_kind)); + + // Re-configure the locators (config unchanged, but ensures consistency). + for locator in context.locators.iter() { + locator.configure(&config); + } } else { // Re-configure the locators with an un-modified config. // Possible we congirued the locators with a modified config in the in the previous request. From 2ac8f880e695c44f482ca3ad4f4658579991b08b Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 11 Feb 2026 08:36:11 -0800 Subject: [PATCH 2/5] fix: canonicalize virtual environment directory for accurate comparison on Windows --- crates/pet/src/find.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/crates/pet/src/find.rs b/crates/pet/src/find.rs index 59a1eb9d..3a6e7074 100644 --- a/crates/pet/src/find.rs +++ b/crates/pet/src/find.rs @@ -725,10 +725,21 @@ mod tests { let environments = reporter.environments.lock().unwrap(); + // Canonicalize the venv_dir for comparison (handles Windows 8.3 short paths) + let venv_dir_canonical = fs::canonicalize(&venv_dir).unwrap_or(venv_dir.clone()); + // The venv should be discovered even when searching by kind + // Use canonicalize to handle Windows short path names (e.g., RUNNER~1 vs runneradmin) let venv_found = environments.iter().any(|env| { env.kind == Some(PythonEnvironmentKind::Venv) - && env.prefix.as_ref().map(|p| p == &venv_dir).unwrap_or(false) + && env + .prefix + .as_ref() + .map(|p| { + let p_canonical = fs::canonicalize(p).unwrap_or(p.clone()); + p_canonical == venv_dir_canonical + }) + .unwrap_or(false) }); assert!( From d6111621f50b2ad9fe514010b960e24a228271a0 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 11 Feb 2026 11:00:24 -0800 Subject: [PATCH 3/5] refactor: extract build_refresh_config into testable helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR review comments: 1. Extract the refresh-option→config/search_scope logic into a helper function uild_refresh_config that can be unit tested 2. Add fast, deterministic unit tests that directly test the bug fix: - test_search_kind_preserves_workspace_directories: verifies the critical fix that workspace_directories are NOT cleared when only searchKind is provided - test_search_paths_replaces_workspace_directories: verifies searchPaths still correctly replaces workspace_directories - test_no_options_preserves_config: verifies default behavior 3. Remove the slow integration-style test from find.rs that was environment-dependent and didn't actually test the bug in handle_refresh Fixes #151 --- crates/pet/src/find.rs | 95 ----------------- crates/pet/src/jsonrpc.rs | 216 ++++++++++++++++++++++++++++---------- 2 files changed, 163 insertions(+), 148 deletions(-) diff --git a/crates/pet/src/find.rs b/crates/pet/src/find.rs index 3a6e7074..30d9a631 100644 --- a/crates/pet/src/find.rs +++ b/crates/pet/src/find.rs @@ -653,99 +653,4 @@ mod tests { "canonicalize() would resolve to target, but path() does not" ); } - - /// Test for https://github.com/microsoft/python-environment-tools/issues/151 - /// Verifies that refresh with searchKind (e.g., "Venv") still finds environments - /// in workspace directories, not just global locations. - /// - /// The bug was that when searchKind was provided, workspace_directories was cleared, - /// preventing discovery of workspace-based environments like venvs. - #[test] - fn test_search_kind_finds_workspace_venvs() { - use super::{find_and_report_envs, SearchScope}; - use crate::locators::create_locators; - use pet_conda::Conda; - use pet_core::os_environment::EnvironmentApi; - use pet_core::python_environment::PythonEnvironmentKind; - use pet_core::Configuration; - use pet_poetry::Poetry; - use pet_reporter::collect; - use std::sync::Arc; - - let tmp = TempDir::new().expect("Failed to create temp dir"); - let workspace = tmp.path().to_path_buf(); - - // Create a venv structure in the workspace - let venv_dir = workspace.join(".venv"); - #[cfg(windows)] - let bin_dir = venv_dir.join("Scripts"); - #[cfg(unix)] - let bin_dir = venv_dir.join("bin"); - fs::create_dir_all(&bin_dir).expect("Failed to create bin dir"); - - // Create pyvenv.cfg (required for venv detection) - fs::write( - venv_dir.join("pyvenv.cfg"), - "home = /usr/bin\nversion = 3.11.0\n", - ) - .expect("Failed to create pyvenv.cfg"); - - // Create python executable - #[cfg(windows)] - let python_exe = bin_dir.join("python.exe"); - #[cfg(unix)] - let python_exe = bin_dir.join("python"); - fs::write(&python_exe, "fake python").expect("Failed to create python exe"); - - // Set up locators and configuration - let environment = EnvironmentApi::new(); - let conda_locator = Arc::new(Conda::from(&environment)); - let poetry_locator = Arc::new(Poetry::from(&environment)); - let locators = create_locators(conda_locator, poetry_locator, &environment); - - let config = Configuration { - workspace_directories: Some(vec![workspace.clone()]), - ..Default::default() - }; - for locator in locators.iter() { - locator.configure(&config); - } - - let reporter = Arc::new(collect::create_reporter()); - - // Call find_and_report_envs with SearchScope::Global(Venv) - // This simulates the behavior when refresh is called with searchKind: "Venv" - find_and_report_envs( - reporter.as_ref(), - config, - &locators, - &environment, - Some(SearchScope::Global(PythonEnvironmentKind::Venv)), - ); - - let environments = reporter.environments.lock().unwrap(); - - // Canonicalize the venv_dir for comparison (handles Windows 8.3 short paths) - let venv_dir_canonical = fs::canonicalize(&venv_dir).unwrap_or(venv_dir.clone()); - - // The venv should be discovered even when searching by kind - // Use canonicalize to handle Windows short path names (e.g., RUNNER~1 vs runneradmin) - let venv_found = environments.iter().any(|env| { - env.kind == Some(PythonEnvironmentKind::Venv) - && env - .prefix - .as_ref() - .map(|p| { - let p_canonical = fs::canonicalize(p).unwrap_or(p.clone()); - p_canonical == venv_dir_canonical - }) - .unwrap_or(false) - }); - - assert!( - venv_found, - "Venv in workspace should be found when searching by kind. Found environments: {:?}", - *environments - ); - } } diff --git a/crates/pet/src/jsonrpc.rs b/crates/pet/src/jsonrpc.rs index fa7e04c3..032f8402 100644 --- a/crates/pet/src/jsonrpc.rs +++ b/crates/pet/src/jsonrpc.rs @@ -204,66 +204,27 @@ pub fn handle_refresh(context: Arc, id: u32, params: Value) { // Ensure we can have only one refresh at a time. let lock = REFRESH_LOCK.lock().expect("REFRESH_LOCK mutex poisoned"); - let mut config = context.configuration.read().unwrap().clone(); + let config = context.configuration.read().unwrap().clone(); let reporter = Arc::new(CacheReporter::new(Arc::new(jsonrpc::create_reporter( refresh_options.search_kind, )))); - let mut search_scope = None; - - // If search_paths is provided, limit search to those paths. - // If only search_kind is provided (without search_paths), we still search - // workspace directories because many environment types (like Venv, VirtualEnv) - // don't have global locations - they only exist in workspace folders. - // The reporter will filter results to only report the requested kind. - if let Some(search_paths) = refresh_options.search_paths { - // Clear workspace directories when explicit search paths are provided. - config.workspace_directories = None; - // Expand any glob patterns in the search paths - let expanded_paths = expand_glob_patterns(&search_paths); + let (config, search_scope) = build_refresh_config(&refresh_options, config); + if refresh_options.search_paths.is_some() { trace!( - "Expanded {} search paths to {} paths", - search_paths.len(), - expanded_paths.len() - ); - // These workspace folders are only for this refresh. - config.workspace_directories = Some( - expanded_paths - .iter() - .filter(|p| p.is_dir()) - .cloned() - .collect(), + "Expanded search paths to {} workspace dirs, {} executables", + config + .workspace_directories + .as_ref() + .map(|v| v.len()) + .unwrap_or(0), + config.executables.as_ref().map(|v| v.len()).unwrap_or(0) ); - config.executables = Some( - expanded_paths - .iter() - .filter(|p| p.is_file()) - .cloned() - .collect(), - ); - search_scope = Some(SearchScope::Workspace); + } - // Configure the locators with the modified config. - for locator in context.locators.iter() { - locator.configure(&config); - } - } else if let Some(search_kind) = refresh_options.search_kind { - // When only search_kind is provided, keep workspace directories so that - // workspace-based environments (Venv, VirtualEnv, etc.) can be found. - // The search_scope tells find_and_report_envs to filter locators by kind. - search_scope = Some(SearchScope::Global(search_kind)); - - // Re-configure the locators (config unchanged, but ensures consistency). - for locator in context.locators.iter() { - locator.configure(&config); - } - } else { - // Re-configure the locators with an un-modified config. - // Possible we congirued the locators with a modified config in the in the previous request. - // & the config was scoped to a particular search folder, executables or kind. - for locator in context.locators.iter() { - locator.configure(&config); - } + // Configure the locators with the (possibly modified) config. + for locator in context.locators.iter() { + locator.configure(&config); } trace!("Start refreshing environments, config: {:?}", config); @@ -506,3 +467,152 @@ pub fn handle_clear_cache(_context: Arc, id: u32, _params: Value) { } }); } + +/// Builds the configuration and search scope based on refresh options. +/// This is extracted from handle_refresh to enable unit testing. +/// +/// Returns (modified_config, search_scope) +pub fn build_refresh_config( + refresh_options: &RefreshOptions, + mut config: Configuration, +) -> (Configuration, Option) { + let mut search_scope = None; + + // If search_paths is provided, limit search to those paths. + // If only search_kind is provided (without search_paths), we still search + // workspace directories because many environment types (like Venv, VirtualEnv) + // don't have global locations - they only exist in workspace folders. + // The reporter will filter results to only report the requested kind. + if let Some(ref search_paths) = refresh_options.search_paths { + // Clear workspace directories when explicit search paths are provided. + config.workspace_directories = None; + // Expand any glob patterns in the search paths + let expanded_paths = expand_glob_patterns(search_paths); + // These workspace folders are only for this refresh. + config.workspace_directories = Some( + expanded_paths + .iter() + .filter(|p| p.is_dir()) + .cloned() + .collect(), + ); + config.executables = Some( + expanded_paths + .iter() + .filter(|p| p.is_file()) + .cloned() + .collect(), + ); + search_scope = Some(SearchScope::Workspace); + } else if let Some(search_kind) = refresh_options.search_kind { + // When only search_kind is provided, keep workspace directories so that + // workspace-based environments (Venv, VirtualEnv, etc.) can be found. + // The search_scope tells find_and_report_envs to filter locators by kind. + search_scope = Some(SearchScope::Global(search_kind)); + } + + (config, search_scope) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::PathBuf; + + /// Test for https://github.com/microsoft/python-environment-tools/issues/151 + /// Verifies that when searchKind is provided (without searchPaths), + /// workspace_directories are NOT cleared. + /// + /// The bug was that handle_refresh cleared workspace_directories when searchKind + /// was provided, preventing discovery of workspace-based environments like venvs. + #[test] + fn test_search_kind_preserves_workspace_directories() { + let workspace = PathBuf::from("/test/workspace"); + let config = Configuration { + workspace_directories: Some(vec![workspace.clone()]), + ..Default::default() + }; + + let refresh_options = RefreshOptions { + search_kind: Some(PythonEnvironmentKind::Venv), + search_paths: None, + }; + + let (result_config, search_scope) = build_refresh_config(&refresh_options, config); + + // CRITICAL: workspace_directories must be preserved when only search_kind is provided + assert_eq!( + result_config.workspace_directories, + Some(vec![workspace]), + "workspace_directories should NOT be cleared when only searchKind is provided" + ); + + // search_scope should be Global with the requested kind + assert!( + matches!( + search_scope, + Some(SearchScope::Global(PythonEnvironmentKind::Venv)) + ), + "search_scope should be Global(Venv)" + ); + } + + /// Test that when searchPaths is provided, workspace_directories ARE replaced. + #[test] + fn test_search_paths_replaces_workspace_directories() { + let original_workspace = PathBuf::from("/original/workspace"); + let config = Configuration { + workspace_directories: Some(vec![original_workspace]), + ..Default::default() + }; + + // Note: search_paths won't exist as directories in tests, so expanded result will be empty + let refresh_options = RefreshOptions { + search_kind: None, + search_paths: Some(vec![PathBuf::from("/search/path")]), + }; + + let (result_config, search_scope) = build_refresh_config(&refresh_options, config); + + // workspace_directories should be replaced (empty since paths don't exist) + assert_eq!( + result_config.workspace_directories, + Some(vec![]), + "workspace_directories should be replaced by search_paths" + ); + + assert!( + matches!(search_scope, Some(SearchScope::Workspace)), + "search_scope should be Workspace" + ); + } + + /// Test that when neither searchKind nor searchPaths is provided, + /// configuration is unchanged. + #[test] + fn test_no_options_preserves_config() { + let workspace = PathBuf::from("/test/workspace"); + let config = Configuration { + workspace_directories: Some(vec![workspace.clone()]), + ..Default::default() + }; + + let refresh_options = RefreshOptions { + search_kind: None, + search_paths: None, + }; + + let (result_config, search_scope) = build_refresh_config(&refresh_options, config); + + assert_eq!( + result_config.workspace_directories, + Some(vec![workspace]), + "workspace_directories should be preserved when no options provided" + ); + + assert!( + search_scope.is_none(), + "search_scope should be None when no options provided" + ); + } +} From ac7693c0e71fa0e2c27698c6fdef84b50cb867de Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 11 Feb 2026 12:11:27 -0800 Subject: [PATCH 4/5] refactor: address PR review comments - Change build_refresh_config from pub to pub(crate) to avoid expanding API surface - Use tempfile::TempDir in test for deterministic is_dir() behavior --- crates/pet/src/jsonrpc.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/crates/pet/src/jsonrpc.rs b/crates/pet/src/jsonrpc.rs index 032f8402..83318c1c 100644 --- a/crates/pet/src/jsonrpc.rs +++ b/crates/pet/src/jsonrpc.rs @@ -472,7 +472,7 @@ pub fn handle_clear_cache(_context: Arc, id: u32, _params: Value) { /// This is extracted from handle_refresh to enable unit testing. /// /// Returns (modified_config, search_scope) -pub fn build_refresh_config( +pub(crate) fn build_refresh_config( refresh_options: &RefreshOptions, mut config: Configuration, ) -> (Configuration, Option) { @@ -560,24 +560,27 @@ mod tests { /// Test that when searchPaths is provided, workspace_directories ARE replaced. #[test] fn test_search_paths_replaces_workspace_directories() { + let temp_dir = tempfile::tempdir().unwrap(); + let search_dir = temp_dir.path().join("search_path"); + std::fs::create_dir(&search_dir).unwrap(); + let original_workspace = PathBuf::from("/original/workspace"); let config = Configuration { workspace_directories: Some(vec![original_workspace]), ..Default::default() }; - // Note: search_paths won't exist as directories in tests, so expanded result will be empty let refresh_options = RefreshOptions { search_kind: None, - search_paths: Some(vec![PathBuf::from("/search/path")]), + search_paths: Some(vec![search_dir.clone()]), }; let (result_config, search_scope) = build_refresh_config(&refresh_options, config); - // workspace_directories should be replaced (empty since paths don't exist) + // workspace_directories should be replaced with the search_paths directory assert_eq!( result_config.workspace_directories, - Some(vec![]), + Some(vec![search_dir]), "workspace_directories should be replaced by search_paths" ); From ea935d68668ec69e58b47cfb9bd21730bffbd43b Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 11 Feb 2026 17:13:29 -0800 Subject: [PATCH 5/5] docs: update JSONRPC.md to reflect searchKind behavior change searchKind now preserves workspace_directories so workspace-based environments (venvs, virtualenvs) can be discovered. --- docs/JSONRPC.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/JSONRPC.md b/docs/JSONRPC.md index e4483e10..d4ed0e5f 100644 --- a/docs/JSONRPC.md +++ b/docs/JSONRPC.md @@ -97,14 +97,15 @@ _Response_: interface RefreshParams { /** * Limits the search to a specific kind of Python environment. - * Ignores workspace folders passed in configuration request. + * Workspace folders from the configuration request are still searched + * to discover workspace-based environments (e.g., venvs, virtualenvs). */ searchKind?: PythonEnvironmentKind; } | { /** * Limits the search to a specific set of paths. * searchPaths can either by directories or Python prefixes/executables or combination of both. - * Ignores workspace folders passed in configuration request. + * Replaces workspace folders from the configuration request. * * Glob patterns are supported: * - `*` matches any sequence of characters in a path component