fix(plugin-loader): fail-fast when install_dependencies returns False

Previously the boolean result was silently discarded, so a failed pip
install would log a warning but continue attempting to import the plugin
module — resulting in a confusing ModuleNotFoundError instead of a clear
dependency failure message.

Now raises PluginError with plugin_id and plugin_dir if dependency
installation fails, stopping the load before the import is attempted.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

src/plugin_system/plugin_loader.py

@@ -5,6 +5,7 @@ Handles plugin module imports, dependency installation, and class instantiation.
 Extracted from PluginManager to improve separation of concerns.
 """
 
+import hashlib
 import json
 import importlib
 import importlib.util
@@ -138,53 +139,89 @@ class PluginLoader:
         self,
         plugin_dir: Path,
         plugin_id: str,
+        plugins_dir: Optional[Path] = None,
         timeout: int = 300
     ) -> bool:
         """
         Install plugin dependencies from requirements.txt.
-        
+
         Args:
             plugin_dir: Plugin directory path
             plugin_id: Plugin identifier
+            plugins_dir: Trusted base plugins directory for path containment check
             timeout: Installation timeout in seconds
-            
+
         Returns:
             True if dependencies installed or not needed, False on error
         """
         plugin_id = os.path.basename(plugin_id or '')
         if not plugin_id:
             return False
-        # Resolve and validate plugin_dir before constructing any derived paths
-        try:
-            plugin_dir_resolved = plugin_dir.resolve(strict=True)
-        except OSError:
+
+        # Resolve to a canonical absolute path (normalises .. and symlinks)
+        plugin_dir_real = os.path.realpath(str(plugin_dir))
+
+        if plugins_dir is not None:
+            # Validate plugin_dir is within the trusted plugins base directory.
+            # os.path.realpath + startswith is the CodeQL-recognised sanitiser
+            # pattern for path-injection (py/path-injection).
+            plugins_dir_real = os.path.realpath(str(plugins_dir))
+            if not plugin_dir_real.startswith(plugins_dir_real + os.sep):
+                self.logger.error(
+                    "Plugin dir for %s is outside the plugins directory, skipping deps",
+                    plugin_id,
+                )
+                return False
+        elif not os.path.isdir(plugin_dir_real):
             self.logger.error("Plugin directory does not exist: %s", plugin_dir)
             return False
-        requirements_file = plugin_dir_resolved / "requirements.txt"
-        if not requirements_file.exists():
-            return True  # No dependencies needed
-        marker_path = plugin_dir_resolved / ".dependencies_installed"
 
-        # Check if already installed
-        if marker_path.exists():
-            self.logger.debug("Dependencies already installed for %s", plugin_id)
-            return True
-        
+        requirements_file = os.path.join(plugin_dir_real, "requirements.txt")
+        marker_file = os.path.join(plugin_dir_real, ".dependencies_installed")
+
+        if not os.path.isfile(requirements_file):
+            return True  # No dependencies needed
+
+        try:
+            with open(requirements_file, 'rb') as fh:
+                current_hash = hashlib.sha256(fh.read()).hexdigest()
+        except OSError as e:
+            self.logger.error("Failed to read requirements.txt for %s: %s", plugin_id, e)
+            return False
+
+        # Skip if requirements.txt hasn't changed since last install
+        if os.path.isfile(marker_file):
+            try:
+                with open(marker_file, 'r', encoding='utf-8') as fh:
+                    stored_hash = fh.read().strip()
+            except OSError as e:
+                self.logger.warning(
+                    "Could not read dependency marker for %s (%s), will reinstall dependencies",
+                    plugin_id, e
+                )
+            else:
+                if stored_hash == current_hash:
+                    self.logger.debug("Dependencies already installed for %s (requirements unchanged)", plugin_id)
+                    return True
+                self.logger.info("Requirements changed for %s, reinstalling dependencies", plugin_id)
+
         try:
             self.logger.info("Installing dependencies for plugin %s...", plugin_id)
             result = subprocess.run(
-                [sys.executable, "-m", "pip", "install", "--break-system-packages", "-r", str(requirements_file)],
+                [sys.executable, "-m", "pip", "install", "--break-system-packages", "-r", requirements_file],
                 capture_output=True,
                 text=True,
                 timeout=timeout,
                 check=False
             )
-            
+
             if result.returncode == 0:
-                # Mark as installed
-                marker_path.touch()
-                # Set proper file permissions after creating marker
-                ensure_file_permissions(marker_path, get_plugin_file_mode())
+                try:
+                    with open(marker_file, 'w', encoding='utf-8') as fh:
+                        fh.write(current_hash)
+                    ensure_file_permissions(Path(marker_file), get_plugin_file_mode())
+                except OSError as marker_err:
+                    self.logger.debug("Could not write dependency marker for %s: %s", plugin_id, marker_err)
                 self.logger.info("Dependencies installed successfully for %s", plugin_id)
                 return True
             else:
@@ -199,8 +236,12 @@ class PluginLoader:
                         "Assuming they are satisfied: %s",
                         plugin_id, stderr.strip()
                     )
-                    marker_path.touch()
-                    ensure_file_permissions(marker_path, get_plugin_file_mode())
+                    try:
+                        with open(marker_file, 'w', encoding='utf-8') as fh:
+                            fh.write(current_hash)
+                        ensure_file_permissions(Path(marker_file), get_plugin_file_mode())
+                    except OSError as marker_err:
+                        self.logger.debug("Could not write dependency marker for %s: %s", plugin_id, marker_err)
                     return True
                 self.logger.warning(
                     "Dependency installation returned non-zero exit code for %s: %s",
@@ -543,11 +584,12 @@ class PluginLoader:
         display_manager: Any,
         cache_manager: Any,
         plugin_manager: Any,
-        install_deps: bool = True
+        install_deps: bool = True,
+        plugins_dir: Optional[Path] = None,
     ) -> Tuple[Any, Any]:
         """
         Complete plugin loading process.
-        
+
         Args:
             plugin_id: Plugin identifier
             manifest: Plugin manifest
@@ -557,16 +599,22 @@ class PluginLoader:
             cache_manager: Cache manager instance
             plugin_manager: Plugin manager instance
             install_deps: Whether to install dependencies
-            
+            plugins_dir: Trusted base plugins directory forwarded to install_dependencies
+
         Returns:
             Tuple of (plugin_instance, module)
-            
+
         Raises:
             PluginError: If loading fails
         """
         # Install dependencies if needed
         if install_deps:
-            self.install_dependencies(plugin_dir, plugin_id)
+            if not self.install_dependencies(plugin_dir, plugin_id, plugins_dir=plugins_dir):
+                raise PluginError(
+                    f"Dependency installation failed for plugin {plugin_id} in {plugin_dir}",
+                    plugin_id=plugin_id,
+                    context={'plugin_dir': str(plugin_dir)},
+                )
         
         # Load module
         entry_point = manifest.get('entry_point', 'manager.py')

src/plugin_system/plugin_manager.py

@@ -350,7 +350,8 @@ class PluginManager:
                 display_manager=self.display_manager,
                 cache_manager=self.cache_manager,
                 plugin_manager=self,
-                install_deps=True
+                install_deps=True,
+                plugins_dir=self.plugins_dir,
             )
             
             # Store module

src/plugin_system/store_manager.py

@@ -5,6 +5,7 @@ Handles plugin discovery, installation, updates, and uninstallation
 from both the official registry and custom GitHub repositories.
 """
 
+import hashlib
 import os
 import json
 import stat
@@ -1755,6 +1756,12 @@ class PluginStoreManager:
                 timeout=300
             )
             self.logger.info(f"Dependencies installed successfully for {plugin_path.name}")
+            # Write hash marker so plugin_loader skips redundant pip run on next startup
+            try:
+                current_hash = hashlib.sha256(requirements_file.read_bytes()).hexdigest()
+                (plugin_path / ".dependencies_installed").write_text(current_hash, encoding='utf-8')
+            except OSError as marker_err:
+                self.logger.debug("Could not write dependency marker for %s: %s", plugin_path.name, marker_err)
             return True
             
         except subprocess.CalledProcessError as e: