Optimize pip detector venv path search with early returns and batch operations

davidkopp · claude · davidkopp · commit 4c11e4bed64c · 2025-09-18T19:23:49.000+02:00
- Refactor _find_venv_path for priority-based search with immediate validation - Add early returns for high-priority paths (explicit, VIRTUAL_ENV, pip location) - Combine local and external venv searches into single efficient batch operation - Fix circular dependency between _find_venv_path and _get_pip_command - Cache pip command to eliminate redundant path_exists calls - Resolve home directory explicitly to fix tilde expansion issues in containers - Remove /usr/local from system-wide fallback search to avoid false positives - Enhance test coverage with system distractor packages validation - Improve test clarity by renaming _install_system_packages to _create_system_distractor_packages - Replace mock-based pip show test with real integration test 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
diff --git a/dependency_resolver/detectors/pip_detector.py b/dependency_resolver/detectors/pip_detector.py
@@ -19,6 +19,7 @@ def __init__(self, venv_path: Optional[str] = None, debug: bool = False):
         self.debug = debug
         self._cached_venv_path: str | None = None
         self._venv_path_searched = False
+        self._cached_pip_command: str | None = None
 
     def is_usable(self, executor: EnvironmentExecutor, working_dir: Optional[str] = None) -> bool:
         """Check if pip is usable in the environment."""
@@ -77,81 +78,125 @@ def get_dependencies(
             return packages, metadata
 
     def _get_pip_location(self, executor: EnvironmentExecutor, working_dir: Optional[str] = None) -> str:
-        """Get the location of the pip environment."""
+        """Get the location of the pip environment, properly classifying system vs project scope."""
         pip_command = self._get_pip_command(executor, working_dir)
         stdout, _, exit_code = executor.execute_command(f"{pip_command} show pip", working_dir)
 
         if exit_code == 0:
             for line in stdout.split("\n"):
                 if line.startswith("Location:"):
-                    return line.split(":", 1)[1].strip()
+                    location = line.split(":", 1)[1].strip()
+
+                    # Check if this is a system location
+                    if location.startswith(("/usr/lib", "/usr/local/lib")):
+                        return "system"
+
+                    return location
 
         return "system"
 
     def _get_pip_command(self, executor: EnvironmentExecutor, working_dir: Optional[str] = None) -> str:
         """Get the appropriate pip command, activating venv if available."""
+        if hasattr(self, "_cached_pip_command") and self._cached_pip_command:
+            return self._cached_pip_command
+
         venv_path = self._find_venv_path(executor, working_dir)
         if venv_path:
             venv_pip = f"{venv_path}/bin/pip"
             if executor.path_exists(venv_pip):
+                self._cached_pip_command = venv_pip
                 return venv_pip
+
+        self._cached_pip_command = "pip"
         return "pip"
 
+    def _validate_venv_path(self, executor: EnvironmentExecutor, path: str) -> str | None:
+        """Check if a path contains pyvenv.cfg and return the venv path if valid."""
+        pyvenv_cfg_path = f"{path}/pyvenv.cfg"
+        if executor.path_exists(pyvenv_cfg_path):
+            self._cached_venv_path = path
+            self._venv_path_searched = True
+            return path
+        return None
+
     def _find_venv_path(self, executor: EnvironmentExecutor, working_dir: Optional[str] = None) -> str | None:
-        """Find virtual environment using batch search for pyvenv.cfg files."""
+        """Find virtual environment using priority-based search for pyvenv.cfg files."""
         # Return cached result if already searched
         if self._venv_path_searched:
             return self._cached_venv_path
 
-        # Build search paths in priority order
-        search_paths = []
-        search_dir = working_dir or "."
-
-        # 1. Explicit venv path (highest priority)
+        # 1. Explicit venv path (highest priority) - check immediately
         if self.explicit_venv_path:
             stdout, _, exit_code = executor.execute_command(f"echo {self.explicit_venv_path}")
             if exit_code == 0 and stdout.strip():
-                search_paths.append(stdout.strip())
+                venv_path = self._validate_venv_path(executor, stdout.strip())
+                if venv_path:
+                    self._cached_venv_path = venv_path
+                    self._venv_path_searched = True
+                    return venv_path
 
-        # 2. VIRTUAL_ENV environment variable (Docker containers)
+        # 2. VIRTUAL_ENV environment variable (Docker containers) - check immediately
         if not isinstance(executor, HostExecutor):
             stdout, _, exit_code = executor.execute_command("echo $VIRTUAL_ENV", working_dir)
             if exit_code == 0 and stdout.strip():
-                search_paths.append(stdout.strip())
-
-        # 3. Local project directory and common venv names
-        search_paths.extend(
-            [
-                search_dir,
-                f"{search_dir}/venv",
-                f"{search_dir}/.venv",
-                f"{search_dir}/env",
-                f"{search_dir}/.env",
-                f"{search_dir}/virtualenv",
-            ]
-        )
+                venv_path = self._validate_venv_path(executor, stdout.strip())
+                if venv_path:
+                    self._cached_venv_path = venv_path
+                    self._venv_path_searched = True
+                    return venv_path
+
+        # 3. Extract venv path from pip show pip location (fallback method) - check immediately
+        # Use basic pip command to avoid circular dependency with _get_pip_command
+        stdout, _, exit_code = executor.execute_command("pip show pip", working_dir)
+        if exit_code == 0:
+            for line in stdout.split("\n"):
+                if line.startswith("Location:"):
+                    pip_location = line.split(":", 1)[1].strip()
+                    # Skip system locations
+                    if not pip_location.startswith(("/usr/lib", "/usr/local/lib")) and "/lib/python" in pip_location:
+                        venv_path = pip_location.split("/lib/python")[0]
+                        # Trust this path since pip is installed there - just cache and return
+                        self._cached_venv_path = venv_path
+                        self._venv_path_searched = True
+                        return venv_path
+                    break
+
+        # 4. Batch search for remaining venv locations
+        # Collect all paths and use single find command for efficiency (fewer executor calls)
+        search_paths = []
+        common_venv_names = ["venv", ".venv", "env", ".env", "virtualenv"]
 
-        # 4. External venv locations (if working_dir specified)
+        # Check working directory and its subdirectories (if working_dir specified)
+        if working_dir:
+            search_paths.append(working_dir)  # working_dir itself
+            for venv_name in common_venv_names:
+                search_paths.append(f"{working_dir}/{venv_name}")
+
+        # Check home directory subdirectories (always check)
+        # Resolve home directory explicitly to avoid tilde expansion issues
+        home_stdout, _, home_exit_code = executor.execute_command("echo $HOME")
+        if home_exit_code == 0 and home_stdout.strip():
+            home_dir = home_stdout.strip()
+            for venv_name in common_venv_names:
+                search_paths.append(f"{home_dir}/{venv_name}")
+
+        # External venv locations (project-specific, only if working_dir specified)
         if working_dir:
             resolved_working_dir = self._resolve_absolute_path(executor, working_dir)
             project_name = os.path.basename(resolved_working_dir)
 
-            # Expand external paths
-            external_locations = [
-                f"~/.virtualenvs/{project_name}",
-                f"~/.local/share/virtualenvs/{project_name}",
-                f"~/.cache/pypoetry/virtualenvs/{project_name}",
-                f"~/.pyenv/versions/{project_name}",
-            ]
-
-            for location in external_locations:
-                stdout, _, exit_code = executor.execute_command(f"echo {location}")
-                if exit_code == 0 and stdout.strip():
-                    search_paths.append(stdout.strip())
+            # Use resolved home directory for external locations too
+            if home_exit_code == 0 and home_stdout.strip():
+                home_dir = home_stdout.strip()
+                external_locations = [
+                    f"{home_dir}/.virtualenvs/{project_name}",
+                    f"{home_dir}/.local/share/virtualenvs/{project_name}",
+                    f"{home_dir}/.cache/pypoetry/virtualenvs/{project_name}",
+                    f"{home_dir}/.pyenv/versions/{project_name}",
+                ]
+                search_paths.extend(external_locations)
 
-        # Single batch find command to locate pyvenv.cfg in all search paths
         if search_paths:
-            # Escape paths and create find command
             escaped_paths = [f"'{path}'" for path in search_paths]
             find_cmd = f"find {' '.join(escaped_paths)} -maxdepth 1 -name 'pyvenv.cfg' -type f 2>/dev/null | head -1"
 
@@ -169,7 +214,7 @@ def _find_venv_path(self, executor: EnvironmentExecutor, working_dir: Optional[s
                 print("pip_detector performing system-wide pyvenv.cfg search in container environment")
 
             stdout, _, exit_code = executor.execute_command(
-                "find /opt /home /usr/local -name 'pyvenv.cfg' -type f 2>/dev/null | head -1"
+                "find /opt /home -name 'pyvenv.cfg' -type f 2>/dev/null | head -1"
             )
 
             if exit_code == 0 and stdout.strip():
diff --git a/tests/detectors/pip/test_pip_venv_detection.py b/tests/detectors/pip/test_pip_venv_detection.py
@@ -163,6 +163,9 @@ def test_virtualenvs_project_name_detection(self, request: pytest.FixtureRequest
             _, _, exit_code = executor.execute_command(f"mkdir -p {virtualenvs_dir}")
             assert exit_code == 0, f"Failed to create virtualenvs directory {virtualenvs_dir}"
 
+            # Create system distractor packages first (should NOT be detected)
+            self._create_system_distractor_packages(executor)
+
             self._create_venv(executor, venv_path)
             self._install_test_packages(executor, venv_path)
 
@@ -177,7 +180,7 @@ def test_virtualenvs_project_name_detection(self, request: pytest.FixtureRequest
 
             self._validate_venv_detection(packages, metadata, expected_scope="project")
 
-            # Verify it found the virtualenvs venv
+            # Verify it found the specific virtualenvs venv path
             assert metadata["location"].startswith(
                 venv_path
             ), f"Expected location to start with {venv_path}, got {metadata['location']}"
@@ -198,6 +201,9 @@ def test_venv_located_at_opt_venv(self, request: pytest.FixtureRequest) -> None:
             container_id = self.start_container(self.PYTHON_DOCKER_IMAGE)
             executor = DockerExecutor(container_id)
 
+            # Create system distractor packages first (should NOT be detected)
+            self._create_system_distractor_packages(executor)
+
             # Create a virtual environment at /opt/venv
             venv_path = "/opt/venv"
             self._create_venv(executor, venv_path)
@@ -225,6 +231,58 @@ def test_venv_located_at_opt_venv(self, request: pytest.FixtureRequest) -> None:
             if container_id:
                 self.cleanup_container(container_id)
 
+    @pytest.mark.skipif(docker is None, reason="Docker not available")
+    def test_pip_show_pip_venv_detection(self, request: pytest.FixtureRequest) -> None:
+        """Test venv detection using pip show pip fallback method with activated venv."""
+        verbose_output = self.setup_verbose_output(request)
+        container_id = None
+
+        try:
+            container_id = self.start_container(self.PYTHON_DOCKER_IMAGE)
+            executor = DockerExecutor(container_id)
+
+            # Create system distractor packages first (should NOT be detected)
+            self._create_system_distractor_packages(executor)
+
+            # Create a venv in an unusual location that won't be found by standard searches
+            unusual_venv_path = "/unusual/location/myvenv"
+            _, _, exit_code = executor.execute_command(f"mkdir -p {os.path.dirname(unusual_venv_path)}")
+            assert exit_code == 0, "Failed to create parent directory for unusual venv path"
+
+            self._create_venv(executor, unusual_venv_path)
+            self._install_test_packages(executor, unusual_venv_path)
+
+            # Activate the venv by using its pip directly (simulates activated environment)
+            venv_pip = f"{unusual_venv_path}/bin/pip"
+            _, _, exit_code = executor.execute_command(f"{venv_pip} --version")
+            assert exit_code == 0, "Venv pip should be usable"
+
+            # Override PATH to make the venv pip the default (simulates activation)
+            _, _, exit_code = executor.execute_command(f"ln -sf {venv_pip} /usr/local/bin/pip")
+            assert exit_code == 0, "Failed to make venv pip the default"
+
+            self.wait_for_container_ready(container_id, "pip --version", max_wait=60)
+
+            # Test detector - should find venv via pip show pip fallback method
+            detector = PipDetector(debug=True)
+            packages, metadata = detector.get_dependencies(executor)
+
+            if verbose_output:
+                self.print_verbose_results("PIP SHOW PIP DETECTION:", {"packages": packages, "metadata": metadata})
+
+            self._validate_venv_detection(packages, metadata, expected_scope="project")
+
+            # Verify it found the unusual venv location via pip show pip
+            assert metadata["location"].startswith(
+                unusual_venv_path
+            ), f"Expected location to start with {unusual_venv_path}, got {metadata['location']}"
+
+            print(f"✓ Successfully detected venv via pip show pip fallback: {unusual_venv_path}")
+
+        finally:
+            if container_id:
+                self.cleanup_container(container_id)
+
     @pytest.mark.skipif(docker is None, reason="Docker not available")
     def test_no_venv_found_returns_empty(self, request: pytest.FixtureRequest) -> None:
         """Test that when no venv is found in working_dir, empty dependencies are returned."""
@@ -282,6 +340,48 @@ def _install_test_packages(self, executor: DockerExecutor, venv_path: str) -> No
             if exit_code != 0:
                 pytest.fail(f"Failed to install {package} in venv: {stderr}")
 
+    def _create_system_distractor_packages(self, executor: DockerExecutor) -> None:
+        """Create fake system packages in /usr/local that should NOT be detected by venv detection.
+
+        This method creates decoy packages to ensure the detector correctly identifies
+        venv packages instead of falling back to system-wide installations.
+        """
+        # Create fake system python site-packages structure in /usr/local
+        system_site_packages = "/usr/local/lib/python3.9/site-packages"
+        _, _, exit_code = executor.execute_command(f"mkdir -p {system_site_packages}")
+        if exit_code != 0:
+            pytest.fail(f"Failed to create system site-packages directory: {system_site_packages}")
+
+        # Create fake system packages with different names (these should NOT be detected)
+        distractor_packages = ["numpy==1.24.0", "pandas==1.5.3"]
+        for package in distractor_packages:
+            package_name = package.split("==", maxsplit=1)[0]
+            package_version = package.split("==", maxsplit=1)[1]
+
+            # Create package metadata files that pip would create
+            dist_info_dir = f"{system_site_packages}/{package_name}-{package_version}.dist-info"
+            _, _, exit_code = executor.execute_command(f"mkdir -p {dist_info_dir}")
+            if exit_code != 0:
+                pytest.fail(f"Failed to create dist-info directory: {dist_info_dir}")
+
+            # Create METADATA file
+            metadata_content = f"""Name: {package_name}
+Version: {package_version}
+"""
+            _, _, exit_code = executor.execute_command(f"echo '{metadata_content}' > {dist_info_dir}/METADATA")
+            if exit_code != 0:
+                pytest.fail(f"Failed to create METADATA file for {package_name}")
+
+        # Also create a fake pyvenv.cfg in /usr/local (should NOT be detected due to /usr/local exclusion)
+        fake_venv_cfg = "/usr/local/pyvenv.cfg"
+        cfg_content = """home = /usr/bin
+include-system-site-packages = false
+version = 3.9.0
+"""
+        _, _, exit_code = executor.execute_command(f"echo '{cfg_content}' > {fake_venv_cfg}")
+        if exit_code != 0:
+            pytest.fail(f"Failed to create fake pyvenv.cfg at {fake_venv_cfg}")
+
     def _validate_venv_detection(
         self, packages: list[Dict[str, Any]], metadata: Dict[str, Any], expected_scope: str
     ) -> None:
@@ -294,6 +394,20 @@ def _validate_venv_detection(
             assert isinstance(metadata["location"], str), f"Location should be string, got {type(metadata['location'])}"
             assert len(metadata["location"]) > 0, "Location should not be empty"
             assert "hash" in metadata, "Project scope should include hash"
+
+            # Verify distractor packages are NOT detected
+            package_names = [pkg["name"] for pkg in packages]
+            distractor_package_names = ["numpy", "pandas"]
+            for distractor_pkg in distractor_package_names:
+                assert (
+                    distractor_pkg not in package_names
+                ), f"Distractor package {distractor_pkg} was incorrectly detected in venv packages"
+
+            # Verify expected venv packages ARE detected
+            expected_venv_packages = ["requests", "click"]
+            for venv_pkg in expected_venv_packages:
+                assert venv_pkg in package_names, f"Expected venv package {venv_pkg} not found in packages"
+
         else:
             # System scope should have empty metadata
             assert not metadata, f"System scope should have empty metadata, got {metadata}"
diff --git a/tests/specialized/pip_root_venv_detection/Dockerfile b/tests/specialized/pip_root_venv_detection/Dockerfile
@@ -0,0 +1,7 @@
+FROM greencoding/gcb_playwright:v20
+
+RUN cd /root && \
+    python3 -m virtualenv venv  && \
+    /root/venv/bin/pip install psutils
+
+CMD ["tail", "-f", "/dev/null"]
diff --git a/tests/specialized/pip_root_venv_detection/test_playwright_container.sh b/tests/specialized/pip_root_venv_detection/test_playwright_container.sh
@@ -0,0 +1,33 @@
+#!/bin/bash
+
+# Build Playwright container and verify psutils is detected in /root/venv/ by dependency_resolver
+
+set -e
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+DOCKERFILE_PATH="$SCRIPT_DIR"
+DEPENDENCY_RESOLVER_PATH="$SCRIPT_DIR/../../.."
+CONTAINER_NAME="test-playwright-$(date +%s)"
+
+echo "Building Docker image from $DOCKERFILE_PATH..."
+docker build -t "$CONTAINER_NAME" -f "$DOCKERFILE_PATH/Dockerfile" "$DOCKERFILE_PATH"
+
+echo "Starting container..."
+docker run --rm -d --name "$CONTAINER_NAME" "$CONTAINER_NAME"
+
+echo "Running dependency_resolver on container $CONTAINER_NAME..."
+cd "$DEPENDENCY_RESOLVER_PATH"
+source venv/bin/activate
+
+echo "Checking for psutils in output..."
+if python3 -m dependency_resolver docker "$CONTAINER_NAME" | grep -q "psutils"; then
+    echo "✓ psutils found in dependency output"
+else
+    echo "✗ psutils NOT found in dependency output"
+fi
+
+echo "Cleaning up..."
+docker stop "$CONTAINER_NAME"
+docker rmi "$CONTAINER_NAME"
+
+echo "Test complete!"
