fix(plugin-loader): auto-detect new dependencies via requirements.txt hash (#355)

* fix(plugin-loader): detect new deps via requirements.txt hash instead of empty marker

The .dependencies_installed marker was an empty file, so adding a new
package to requirements.txt (e.g. astral in ledmatrix-weather v2.3.0)
never triggered a pip re-install on existing installs — the file existed
so the check returned early.

The marker now stores a SHA-256 hash of requirements.txt. On every plugin
load, the loader compares the current hash to the stored one; a mismatch
(or missing marker) triggers pip install and writes the new hash.
store_manager._install_dependencies() also writes the hash marker after a
store install/update so the loader skips a redundant pip run on next boot.

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

* fix(plugin-loader): address CodeQL path expression and I/O error handling

- Add explicit relative_to() containment check after path resolution so
  CodeQL recognizes the plugin directory boundary (fixes 4 CodeQL alerts:
  Uncontrolled data used in path expression, lines 168/172/189/205)
- Wrap requirements_file.read_bytes() in try/except OSError — on Raspberry
  Pi with flaky SD card storage this can fail; returns False with a clear log
- Wrap marker_path.read_text() in try/except OSError — a corrupted marker
  falls through to a clean reinstall instead of crashing
- Wrap both marker_path.write_text() calls in try/except OSError — pip
  already succeeded at this point so a marker write failure should not
  return False or propagate through the generic exception handler

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

* fix(plugin-loader): use realpath+startswith containment check for CodeQL path-injection

Replace relative_to() (not recognised by CodeQL as a path sanitiser) with
the os.path.realpath() + startswith() pattern that CodeQL explicitly models
as sanitising py/path-injection.

- Add plugins_dir optional param to install_dependencies() and load_plugin()
- PluginManager.load_plugin() passes self.plugins_dir as the trusted anchor;
  install_dependencies() validates that the resolved plugin_dir starts with
  the resolved plugins_dir before any file I/O
- Replace all Path.read_bytes/read_text/write_text/exists with open() and
  os.path.isfile() so the sanitised string paths flow directly to file ops
  without re-introducing taint through Path object conversion

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

* 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>

* fix(plugin-loader): use basename+reconstruct to satisfy CodeQL py/path-injection

startswith() is a validation check in CodeQL's model, not a sanitiser —
taint still flows through plugin_dir_real to the file operations.

os.path.basename() IS in CodeQL's recognised sanitiser list: it strips all
directory components so the result cannot contain traversal sequences.
Reconstructing the plugin path from the trusted plugins_dir base joined with
the basename-sanitised directory name produces a path CodeQL considers
untainted, breaking the taint chain from the plugin_dir parameter.

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

* fix(plugin-loader): guard against empty basename when plugin_dir resolves to fs root

If plugin_dir somehow resolves to '/' or a bare drive root, os.path.basename()
returns '', causing safe_plugin_dir to equal plugins_dir_real and the isdir()
check to pass incorrectly. Reject early with a clear error in that case.

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

---------

Co-authored-by: Chuck <chuck@example.com>
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,6 +139,7 @@ class PluginLoader:
         self,
         plugin_dir: Path,
         plugin_id: str,
+        plugins_dir: Optional[Path] = None,
         timeout: int = 300
     ) -> bool:
         """
@@ -146,6 +148,7 @@ class PluginLoader:
         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:
@@ -154,26 +157,67 @@ class PluginLoader:
         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:
+            # Reconstruct the plugin path from a trusted base + a sanitised
+            # directory name.  os.path.basename() is CodeQL's recognised
+            # py/path-injection sanitiser: it strips all directory components
+            # so the result cannot contain traversal sequences.  Joining it
+            # with the resolved, trusted plugins_dir produces a path that
+            # CodeQL considers untainted.
+            plugins_dir_real = os.path.realpath(str(plugins_dir))
+            safe_dir_name = os.path.basename(plugin_dir_real)
+            if not safe_dir_name:
+                self.logger.error("Could not determine plugin directory name for %s", plugin_id)
+                return False
+            safe_plugin_dir = os.path.join(plugins_dir_real, safe_dir_name)
+            if not os.path.isdir(safe_plugin_dir):
+                self.logger.error(
+                    "Plugin directory for %s not found inside plugins dir", plugin_id
+                )
+                return False
+        else:
+            safe_plugin_dir = plugin_dir_real
+            if not os.path.isdir(safe_plugin_dir):
                 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)
+        requirements_file = os.path.join(safe_plugin_dir, "requirements.txt")
+        marker_file = os.path.join(safe_plugin_dir, ".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,
@@ -181,10 +225,12 @@ class PluginLoader:
             )
 
             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 +245,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,7 +593,8 @@ 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.
@@ -557,6 +608,7 @@ 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)
@@ -566,7 +618,12 @@ class PluginLoader:
         """
         # 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: