rick/LEDMatrix
1
0
Fork 0
mirror of https://github.com/ChuckBuilds/LEDMatrix.git synced 2026-08-01 08:48:05 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: plugin_loader retries with --ignore-installed on apt/pip RECORD conflicts (#386)

Browse Source 
* 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx

* 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx

* fix: tolerate a timed-out --ignore-installed retry, dedupe marker-write logic

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx

* fix: revert marker-write dedup helper, resolves CodeQL path-injection alert

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx

---------

Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Chuck
2026-07-08 11:19:37 -04:00
committed by GitHub
co-authored by Claude Sonnet 5
parent 63a233f3ed
commit 85d321cf33
2 changed files with 120 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/plugin_system/plugin_loader.py
+41 -6
View File
@@ -235,16 +235,51 @@ class PluginLoader:
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)
test/test_plugin_loader.py
+77
View File
@@ -4,6 +4,8 @@ Tests for PluginLoader.
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
@@ -220,3 +222,78 @@ class TestPluginLoader:
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