From c4c49c59dbd0410b4b4256dc2d4cc995e62c4651 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 14:06:40 +0000 Subject: [PATCH 1/4] fix: plugin_loader retries with --ignore-installed before assuming apt package satisfies pin install_dependencies treated any "uninstall-no-record-file" pip failure as "dependency satisfied" and wrote the success marker without ever attempting --ignore-installed, unlike install_dependencies_apt.py and safe_pip_install.sh (added in #385 for the Plugin Store/first-time-install paths). A plugin pinning a newer version of a system-managed package (e.g. requests) would silently keep running against whatever version apt shipped, while the marker file claimed the pinned requirement was met. Now retries the same install with --ignore-installed on that specific failure so pip actually lays the pinned version down (shadowing the system-managed copy) before falling back to the prior tolerant behavior if the retry itself fails too. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx --- src/plugin_system/plugin_loader.py | 31 +++++++++++++---- test/test_plugin_loader.py | 53 ++++++++++++++++++++++++++++-- 2 files changed, 76 insertions(+), 8 deletions(-) diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index 49d2e4042..1e6762133 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -235,16 +235,35 @@ def install_dependencies( return True else: stderr = result.stderr or "" - # uninstall-no-record-file means the package is already present at the - # system level (e.g. installed via dnf/apt without a pip RECORD file). - # pip can't replace it, but it IS installed — write the marker so we - # don't retry on every restart. + # uninstall-no-record-file means a system-managed copy of a package + # (e.g. apt's python3-requests, which ships no pip RECORD file) is in + # the way of the version this requirements.txt pins. Retry with + # --ignore-installed so pip lays the pinned version down alongside + # the system copy instead of trying to replace it — matching the + # retry already used by install_dependencies_apt.py / safe_pip_install.sh. + # Without this retry, the plugin would silently keep running against + # whatever version the system happened to ship, even though the + # marker below claims the requirement is satisfied. if "uninstall-no-record-file" in stderr: self.logger.warning( - "Dependencies for %s include system-managed packages (no pip RECORD). " - "Assuming they are satisfied: %s", + "Dependencies for %s conflict with a system-managed package " + "(no pip RECORD); retrying with --ignore-installed: %s", plugin_id, stderr.strip() ) + retry_result = subprocess.run( + [sys.executable, "-m", "pip", "install", "--break-system-packages", + "--ignore-installed", "-r", requirements_file], + capture_output=True, + text=True, + timeout=timeout, + check=False + ) + if retry_result.returncode != 0: + self.logger.warning( + "Retry with --ignore-installed also failed for %s; assuming the " + "system-managed version satisfies the requirement: %s", + plugin_id, (retry_result.stderr or "").strip() + ) try: with open(marker_file, 'w', encoding='utf-8') as fh: fh.write(current_hash) diff --git a/test/test_plugin_loader.py b/test/test_plugin_loader.py index 043bf1168..6d0875b0d 100644 --- a/test/test_plugin_loader.py +++ b/test/test_plugin_loader.py @@ -216,7 +216,56 @@ def test_install_dependencies_failure(self, mock_subprocess, plugin_loader, tmp_ requirements_file.write_text("package1==1.0.0\n") mock_subprocess.return_value = MagicMock(returncode=1) - + result = plugin_loader.install_dependencies(plugin_dir, "test_plugin") - + assert result is False + + @patch('subprocess.run') + def test_install_dependencies_retries_with_ignore_installed_on_apt_conflict( + self, mock_subprocess, plugin_loader, tmp_plugins_dir + ): + """An apt-managed package with no pip RECORD file triggers a retry with + --ignore-installed rather than silently assuming the old version satisfies + the requirement.""" + plugin_dir = tmp_plugins_dir / "test_plugin" + plugin_dir.mkdir() + requirements_file = plugin_dir / "requirements.txt" + requirements_file.write_text("requests>=2.33.0,<3.0.0\n") + + first_attempt = MagicMock( + returncode=1, + stderr="ERROR: Cannot uninstall requests 2.32.3\nuninstall-no-record-file" + ) + retry_attempt = MagicMock(returncode=0, stderr="") + mock_subprocess.side_effect = [first_attempt, retry_attempt] + + result = plugin_loader.install_dependencies(plugin_dir, "test_plugin") + + assert result is True + assert mock_subprocess.call_count == 2 + retry_cmd = mock_subprocess.call_args_list[1][0][0] + assert "--ignore-installed" in retry_cmd + + @patch('subprocess.run') + def test_install_dependencies_apt_conflict_retry_also_fails( + self, mock_subprocess, plugin_loader, tmp_plugins_dir + ): + """Still tolerates the failure (returns True) if the --ignore-installed + retry itself fails, matching the prior soft-fallback behavior.""" + plugin_dir = tmp_plugins_dir / "test_plugin" + plugin_dir.mkdir() + requirements_file = plugin_dir / "requirements.txt" + requirements_file.write_text("requests>=2.33.0,<3.0.0\n") + + first_attempt = MagicMock( + returncode=1, + stderr="ERROR: Cannot uninstall requests 2.32.3\nuninstall-no-record-file" + ) + retry_attempt = MagicMock(returncode=1, stderr="some other pip error") + mock_subprocess.side_effect = [first_attempt, retry_attempt] + + result = plugin_loader.install_dependencies(plugin_dir, "test_plugin") + + assert result is True + assert mock_subprocess.call_count == 2 From 293e60bf29622853f7300d75e3cdd3296913932a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 14:09:50 +0000 Subject: [PATCH 2/4] chore: suppress Codacy finding on new retry subprocess.run call Same generic Bandit/semgrep pattern-match on non-literal subprocess.run argv flagged in #385's install_requirements_file, now on the new --ignore-installed retry call added here: list-form argv (no shell=True), sys.executable is this process's own interpreter, and requirements_file is built internally by find_plugin_directory, never raw external input. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx --- src/plugin_system/plugin_loader.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index 1e6762133..7a18bf79d 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -250,7 +250,10 @@ def install_dependencies( "(no pip RECORD); retrying with --ignore-installed: %s", plugin_id, stderr.strip() ) - retry_result = subprocess.run( + # sys.executable is this process's own interpreter (not + # attacker-influenced), and requirements_file is a path built + # internally by find_plugin_directory, never raw external input. + retry_result = subprocess.run( # nosec B603 - no shell invoked (list-form argv) # nosemgrep [sys.executable, "-m", "pip", "install", "--break-system-packages", "--ignore-installed", "-r", requirements_file], capture_output=True, From 87cfbda7be8e8a47cbeff7023b29aafc0ac48b0c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 15:10:09 +0000 Subject: [PATCH 3/4] fix: tolerate a timed-out --ignore-installed retry, dedupe marker-write logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review caught a real inconsistency: if the --ignore-installed retry itself timed out, subprocess.TimeoutExpired propagated to the outer handler and returned False, failing plugin load — contradicting the intended "tolerate this specific apt/pip conflict" behavior, where a mere non-zero retry return code already returns True. Wraps the retry in its own try/except so a timeout is logged and tolerated the same way as any other retry failure. Also extracts the marker-writing logic (open/write/chmod, ignoring OSError) into _write_dependency_marker, since it was duplicated identically between the direct-success path and the apt-conflict-retry-fallback path. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx --- src/plugin_system/plugin_loader.py | 67 ++++++++++++++++++------------ test/test_plugin_loader.py | 28 +++++++++++++ 2 files changed, 68 insertions(+), 27 deletions(-) diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index 7a18bf79d..c0be7ffff 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -135,6 +135,16 @@ def find_plugin_directory( return None + def _write_dependency_marker(self, marker_file: str, current_hash: str, plugin_id: str) -> None: + """Write the dependency hash marker file, used by both the direct-success + and apt-conflict-retry-fallback paths in install_dependencies.""" + 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) + def install_dependencies( self, plugin_dir: Path, @@ -225,12 +235,7 @@ def install_dependencies( ) if result.returncode == 0: - 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._write_dependency_marker(marker_file, current_hash, plugin_id) self.logger.info("Dependencies installed successfully for %s", plugin_id) return True else: @@ -250,29 +255,37 @@ def install_dependencies( "(no pip RECORD); retrying with --ignore-installed: %s", plugin_id, stderr.strip() ) - # sys.executable is this process's own interpreter (not - # attacker-influenced), and requirements_file is a path built - # internally by find_plugin_directory, never raw external input. - retry_result = subprocess.run( # nosec B603 - no shell invoked (list-form argv) # nosemgrep - [sys.executable, "-m", "pip", "install", "--break-system-packages", - "--ignore-installed", "-r", requirements_file], - capture_output=True, - text=True, - timeout=timeout, - check=False - ) - if retry_result.returncode != 0: + # Wrapped in its own try/except so a retry timeout is + # tolerated the same way as a retry failure, instead of + # propagating to the outer handler and returning False + # (which would contradict the "assume satisfied" fallback + # below). + try: + # sys.executable is this process's own interpreter (not + # attacker-influenced), and requirements_file is a path + # built internally by find_plugin_directory, never raw + # external input. + retry_result = subprocess.run( # nosec B603 - no shell invoked (list-form argv) # nosemgrep + [sys.executable, "-m", "pip", "install", "--break-system-packages", + "--ignore-installed", "-r", requirements_file], + capture_output=True, + text=True, + timeout=timeout, + check=False + ) + if retry_result.returncode != 0: + self.logger.warning( + "Retry with --ignore-installed also failed for %s; assuming the " + "system-managed version satisfies the requirement: %s", + plugin_id, (retry_result.stderr or "").strip() + ) + except subprocess.TimeoutExpired: self.logger.warning( - "Retry with --ignore-installed also failed for %s; assuming the " - "system-managed version satisfies the requirement: %s", - plugin_id, (retry_result.stderr or "").strip() + "Retry with --ignore-installed timed out for %s; assuming the " + "system-managed version satisfies the requirement", + plugin_id ) - 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._write_dependency_marker(marker_file, current_hash, plugin_id) return True self.logger.warning( "Dependency installation returned non-zero exit code for %s: %s", diff --git a/test/test_plugin_loader.py b/test/test_plugin_loader.py index 6d0875b0d..c1da6f5e2 100644 --- a/test/test_plugin_loader.py +++ b/test/test_plugin_loader.py @@ -4,6 +4,8 @@ Tests plugin directory discovery, module loading, and class instantiation. """ +import subprocess + import pytest from unittest.mock import MagicMock, patch from src.plugin_system.plugin_loader import PluginLoader @@ -269,3 +271,29 @@ def test_install_dependencies_apt_conflict_retry_also_fails( assert result is True assert mock_subprocess.call_count == 2 + + @patch('subprocess.run') + def test_install_dependencies_apt_conflict_retry_times_out( + self, mock_subprocess, plugin_loader, tmp_plugins_dir + ): + """A retry timeout must be tolerated the same way as a retry failure + (return True), not propagate to the outer TimeoutExpired handler and + return False.""" + plugin_dir = tmp_plugins_dir / "test_plugin" + plugin_dir.mkdir() + requirements_file = plugin_dir / "requirements.txt" + requirements_file.write_text("requests>=2.33.0,<3.0.0\n") + + first_attempt = MagicMock( + returncode=1, + stderr="ERROR: Cannot uninstall requests 2.32.3\nuninstall-no-record-file" + ) + mock_subprocess.side_effect = [ + first_attempt, + subprocess.TimeoutExpired(cmd="pip", timeout=300), + ] + + result = plugin_loader.install_dependencies(plugin_dir, "test_plugin") + + assert result is True + assert mock_subprocess.call_count == 2 From ea8469dd6a40f688d752c538160f50be39af0888 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 15:13:29 +0000 Subject: [PATCH 4/4] fix: revert marker-write dedup helper, resolves CodeQL path-injection alert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL flagged _write_dependency_marker's open(marker_file, ...) as "uncontrolled data used in path expression" (high severity) once the marker-write logic was extracted into its own method. marker_file is actually safe — it's built from safe_plugin_dir, which install_dependencies sanitizes via os.path.basename() (CodeQL's own recognized py/path-injection sanitizer, per the existing comment a few lines above) — but CodeQL's interprocedural analysis doesn't carry that sanitized status across the new method boundary, since the sanitizer call and the open() sink were no longer in the same function. This exact code produced zero CodeQL findings before the extraction (in two duplicated inline blocks) and is unchanged in what data reaches it — only its location moved. Reverting the extraction (keeping CodeRabbit's other, independent timeout-handling fix) restores the previously-clean shape rather than trying to convince the analyzer's cross-function taint tracking that a refactor changed nothing. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx --- src/plugin_system/plugin_loader.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index c0be7ffff..1001f59b6 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -135,16 +135,6 @@ def find_plugin_directory( return None - def _write_dependency_marker(self, marker_file: str, current_hash: str, plugin_id: str) -> None: - """Write the dependency hash marker file, used by both the direct-success - and apt-conflict-retry-fallback paths in install_dependencies.""" - 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) - def install_dependencies( self, plugin_dir: Path, @@ -235,7 +225,12 @@ def install_dependencies( ) if result.returncode == 0: - self._write_dependency_marker(marker_file, current_hash, plugin_id) + 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: @@ -285,7 +280,12 @@ def install_dependencies( "system-managed version satisfies the requirement", plugin_id ) - self._write_dependency_marker(marker_file, current_hash, plugin_id) + 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",