diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index 49d2e404..1001f59b 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -235,16 +235,51 @@ 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() ) + # 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 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) diff --git a/test/test_plugin_loader.py b/test/test_plugin_loader.py index 043bf116..c1da6f5e 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 @@ -216,7 +218,82 @@ 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 + + @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