From 970ca2d04f0f00e7e813472655dbdee3e98d9deb Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Mon, 3 Aug 2026 16:52:42 -0400 Subject: [PATCH] feat(store): refuse to install a plugin that needs a newer core (re-target of #429) (#431) * feat(store): refuse to install a plugin that needs a newer core `ledmatrix_min_version` was decoration. The loader logged an advisory warning and continued; the store never compared the core version at all, so a routine "update" delivered a plugin that could not run. That is the gap phase B6 (the sports-unification sunset) cannot be done over: deleting a plugin's bundled fallback while nothing enforces the floor hands un-updated users a scoreboard that raises ModuleNotFoundError at load and is reported only as one line in the journal. The gate lives in install_plugin, after the manifest is on disk and before dependencies are installed. That is the earliest knowable point -- the registry carries no compatibility field, so the floor is not visible until the files are down -- and it is also the chokepoint: _reinstall_with_rollback calls install_plugin, so a refused *update* restores the version the user already had, for free. Floor resolution and the comparison move to src/plugin_system/compatibility.py, shared with the loader so the two cannot drift. Both read all four spellings published manifests use, including the deprecated `ledmatrix_min`. Refusal requires evidence. An undeclared floor, an unparseable version on either side, or a core below TRUSTWORTHY_FLOOR (2.0.0) all allow the install. That last one is deliberate and load-bearing: the v3.1.0 release reports __version__ = "1.0.0" while nearly every published manifest floors at 2.0.0, so a strict gate would lock those users out of the plugin store entirely -- much worse than the problem being solved. They stay unprotected until they update the core, which is also what fixes their version string. Verified: 782 core unit tests pass, including 25 new ones and the existing loader-warning suite unchanged (the refactor is behavior-preserving). The install tests drive the real install_plugin path with the download stubbed -- the allow and refuse cases differ only in the declared floor, so the refusal is demonstrably the gate and not an earlier bail-out. Follow-ups, deliberately not in this PR: surfacing the reason in the store UI rather than only the log, and publishing the floor in plugins.json so the store can refuse before downloading. Phase B4 in docs/SPORTS_UNIFICATION.md. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(store): a failed install must not destroy the plugin it replaced Found while validating the compatibility gate. `_install_plugin_impl` deletes the existing plugin directory *before* downloading, so any failure after that point leaves the user with nothing. `_reinstall_with_rollback` protects the update path exactly this way; a direct `install_plugin` had no equivalent. The gate made this reachable in a new way: a plugin whose declared floor exceeds the running core is now refused *after* the old copy is already gone. Floors are hand-written and can be over-declared, so the refusal could remove a plugin that had been working fine on that core. install_plugin is now a thin wrapper that renames any existing install aside, delegates to _install_plugin_impl, and restores it on failure -- including when the implementation raises, which is re-raised after the restore. It is a pass-through when nothing is installed and when called from _reinstall_with_rollback, which has already moved the old copy aside; a test pins that so the two mechanisms cannot start nesting. The aside name embeds '.standalone-backup-' because plugin_manager._scan_directory_for_plugins keys on exactly that substring to skip backups. A different name would have made the backup discoverable as a duplicate plugin; a test pins that too. 789 core unit tests pass, including 7 new ones. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(store): serialize concurrent installs, and make the lock reentrant Second bug found while validating the previous commit on hardware. install_plugin's new set-aside/restore had no lock. The web UI runs Flask threaded, so a double-clicked Install button gives two threads the same plugin_id; interleaved, one thread's restore deletes the other's freshly installed copy. _reinstall_with_rollback already guards exactly this with a per-plugin lock, and install_plugin needs the same one. Taking that lock naively deadlocks. _reinstall_with_rollback holds it across its call to install_plugin, and threading.Lock is not reentrant -- so the request thread hangs forever on the standard monorepo update path (update_plugin -> _reinstall_with_rollback -> install_plugin), which is to say on every plugin update. Verified by reverting to a plain Lock: the regression test times out after 10s instead of passing. The per-plugin locks are now RLocks, and install_plugin holds one for its whole set-aside/install/restore sequence. Verified on devpi (Pi, Python 3.13.5, real registry and network): - update_plugin on an up-to-date plugin: True in 5.4s - update_plugin forced through the full reinstall-with-rollback path: True in 13.1s, correct version restored, old copy replaced, no backup directories left behind - install -> reinstall-over-existing -> failed-reinstall-restores: all pass against real downloads - 22 plugins load, no tracebacks, web API and UI 200, steady-state journal 50 lines/min 791 core unit tests pass, including 2 new concurrency tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * ci: run the new suites, and check tag/version agreement at release time These were split out of #428/#429 because the token pushing them lacked the `workflow` scope. Folding them in here rather than opening a stacked PR -- #429 was merged into its stacked base after that base had already been squash-merged, so its content never reached main, and one such near-miss is enough. All three enrolled suites exist on this branch: test_version_consistency.py came with #428 and is on main; the other two arrive with the commits above. Enrolling them in a separate PR would have either raced with this one on test.yml or briefly pointed CI at files main did not have. - test.yml: enroll test_version_consistency, test_plugin_compatibility_gate and test_install_preserves_existing in the core unit job. Until now these 32 tests existed but nothing ran them automatically. - release-version-check.yml: run scripts/check_release_version.py on pushed v* tags and published releases, plus workflow_dispatch so a tag can be checked *before* it is created. No dependencies -- it reads src/__init__.py and CHANGELOG.md only. Verified: both workflow files parse, and the release check still passes for v3.2.0 against this tree. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 --------- Co-authored-by: Claude Opus 5 (1M context) --- .github/workflows/release-version-check.yml | 40 ++++ .github/workflows/test.yml | 5 +- src/plugin_system/compatibility.py | 102 +++++++++ src/plugin_system/plugin_loader.py | 43 ++-- src/plugin_system/store_manager.py | 121 +++++++++- test/test_install_preserves_existing.py | 222 +++++++++++++++++++ test/test_plugin_compatibility_gate.py | 232 ++++++++++++++++++++ 7 files changed, 734 insertions(+), 31 deletions(-) create mode 100644 .github/workflows/release-version-check.yml create mode 100644 src/plugin_system/compatibility.py create mode 100644 test/test_install_preserves_existing.py create mode 100644 test/test_plugin_compatibility_gate.py diff --git a/.github/workflows/release-version-check.yml b/.github/workflows/release-version-check.yml new file mode 100644 index 00000000..d9c6320a --- /dev/null +++ b/.github/workflows/release-version-check.yml @@ -0,0 +1,40 @@ +name: Release version check + +# A release tag, the CHANGELOG, and src.__version__ must agree. They have not +# always: v3.1.0 was tagged while src/__init__.py still said "1.0.0", which +# silently exempted every device installed from that release from plugin +# compatibility warnings. See docs/SPORTS_UNIFICATION.md (phase B4). +on: + push: + tags: ["v*"] + release: + types: [published] + # Pre-flight: run this against the tag you are about to create. + workflow_dispatch: + inputs: + tag: + description: "Tag to check (e.g. v3.2.0)" + required: true + type: string + +permissions: + contents: read + +jobs: + version-matches-tag: + name: Tag matches src.__version__ + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + persist-credentials: false + + - uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 + with: + python-version: "3.12" + + # No dependencies: the script reads src/__init__.py and CHANGELOG.md only. + - name: Assert the tag, CHANGELOG and src.__version__ agree + run: python scripts/check_release_version.py "${TAG}" + env: + TAG: ${{ inputs.tag || github.ref_name }} diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index cfe31c17..76492edd 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -76,4 +76,7 @@ jobs: test/test_sports_core_promotions.py \ test/test_sports_modes_promotions.py \ test/test_sports_capabilities.py \ - test/test_sports_scroll.py + test/test_sports_scroll.py \ + test/test_version_consistency.py \ + test/test_plugin_compatibility_gate.py \ + test/test_install_preserves_existing.py diff --git a/src/plugin_system/compatibility.py b/src/plugin_system/compatibility.py new file mode 100644 index 00000000..dd3925e7 --- /dev/null +++ b/src/plugin_system/compatibility.py @@ -0,0 +1,102 @@ +"""One place that answers "can this plugin run on this core?". + +Two callers ask that question and they must not drift apart: + +- `PluginLoader._warn_if_incompatible` — at load time, **advisory**. A plugin + already on disk keeps loading regardless, because the guarded-import pattern + means most incompatibilities degrade rather than break. +- `PluginStoreManager.install_plugin` — at install/update time, **blocking**. + This is the point where refusing costs the user nothing (they keep the + version they already had) and allowing can cost them a plugin that fails to + load with only a log line to explain it. + +## The trustworthiness problem + +The core's own `__version__` has not always been right. `v3.1.0` was tagged +2026-05-31 while `src/__init__.py` still said `"1.0.0"`; the bump landed +2026-07-12. Devices installed from that release report `1.0.0` — below the +floor that essentially every published plugin declares. + +So a core reporting a version below `TRUSTWORTHY_FLOOR` is treated as +**unknown, not old**: it neither warns nor blocks. Blocking on it would be far +worse than the problem being solved — nearly every manifest in the ecosystem +floors at `2.0.0`, so a strict gate would stop those users installing *any* +plugin. They are unprotected until they update the core, which is also what +fixes their version string. See `docs/SPORTS_UNIFICATION.md`, phase B4. +""" + +from __future__ import annotations + +from typing import Any, Dict, Optional, Tuple + +# Below this, the core's self-reported version is not evidence of anything. +# See the module docstring. +TRUSTWORTHY_FLOOR: Tuple[int, int, int] = (2, 0, 0) + + +def parse_semver(value: Any) -> Optional[Tuple[int, int, int]]: + """Parse ``X.Y.Z`` (extra parts and suffixes ignored) into a comparable + 3-tuple, or ``None`` when unparseable. A leading ``v`` is tolerated.""" + if not isinstance(value, str): + return None + parts = value.strip().lstrip('v').split('.') + try: + nums = [int(''.join(ch for ch in p if ch.isdigit()) or 0) for p in parts[:3]] + except ValueError: + return None + while len(nums) < 3: + nums.append(0) + return tuple(nums) # type: ignore[return-value] + + +def declared_min_version(manifest: Dict[str, Any]) -> Optional[str]: + """The core version this plugin says it needs, or ``None`` if it doesn't say. + + Checked in order of specificity. `ledmatrix_min` is the deprecated spelling + of `ledmatrix_min_version` (`store_manager._validate_manifest_fields` flags + it); both are read because a large share of published manifests still carry + the old one. + """ + declared = ( + manifest.get('min_ledmatrix_version') + or (manifest.get('requires') or {}).get('min_ledmatrix_version') + ) + if declared: + return declared + + versions = manifest.get('versions') or [] + if versions and isinstance(versions[0], dict): + return (versions[0].get('ledmatrix_min_version') + or versions[0].get('ledmatrix_min')) + return None + + +def check(manifest: Dict[str, Any], core_version: str) -> Tuple[bool, Optional[str]]: + """Return ``(compatible, reason)``. + + ``compatible`` is False **only** when the plugin declares a parseable floor, + the core reports a parseable and trustworthy version, and the floor is + genuinely above it. Every uncertain case resolves to compatible: an + undeclared floor, an unparseable version on either side, or a core whose + version is below `TRUSTWORTHY_FLOOR`. Refusing on a guess would break + working installs, which is the more expensive mistake here. + + ``reason`` is user-facing text, present only when incompatible. + """ + declared = declared_min_version(manifest) + needed = parse_semver(declared) + if needed is None: + return True, None + + current = parse_semver(core_version) + if current is None or current < TRUSTWORTHY_FLOOR: + return True, None + + if needed > current: + name = manifest.get('name') or manifest.get('id') or 'This plugin' + return False, ( + f"{name} requires LEDMatrix {declared} or newer, but this system is " + f"running {core_version}. Update LEDMatrix first, then install it." + ) + + return True, None diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index 733bfde1..bd748ef7 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -702,34 +702,25 @@ class PluginLoader: newer than the running core. Advisory only — never raises — so a plugin that guards optional features with try/except keeps working. """ - declared = ( - manifest.get('min_ledmatrix_version') - or manifest.get('requires', {}).get('min_ledmatrix_version') - ) - if not declared: - versions = manifest.get('versions') or [] - if versions and isinstance(versions[0], dict): - declared = (versions[0].get('ledmatrix_min_version') - or versions[0].get('ledmatrix_min')) - needed = self._parse_semver(declared) - if needed is None: + from src import __version__ as core_version + from src.plugin_system import compatibility + + compatible, _reason = compatibility.check(manifest, core_version) + if compatible: + # Distinguish "fine" from "couldn't tell" for anyone reading logs: + # a core below the trustworthy floor is skipped, not cleared. + current = compatibility.parse_semver(core_version) + if current is None or current < compatibility.TRUSTWORTHY_FLOOR: + self.logger.debug( + "Skipping version compatibility check for %s: core __version__ " + "(%s) is below the ecosystem floor", plugin_id, core_version) return - from src import __version__ as core_version - current = self._parse_semver(core_version) - # Anti-spam guard: if the core's own version number is stale (below - # the ecosystem floor every shipped plugin declares), comparing would - # warn on nearly everything — skip with a debug note instead. - if current is None or current < (2, 0, 0): - self.logger.debug( - "Skipping version compatibility check for %s: core __version__ " - "(%s) is below the ecosystem floor", plugin_id, core_version) - return - if needed > current: - self.logger.warning( - "Plugin %s declares min LEDMatrix version %s but this core is %s — " - "features it relies on may be missing; update the core or expect " - "degraded fallbacks", plugin_id, declared, core_version) + declared = compatibility.declared_min_version(manifest) + self.logger.warning( + "Plugin %s declares min LEDMatrix version %s but this core is %s — " + "features it relies on may be missing; update the core or expect " + "degraded fallbacks", plugin_id, declared, core_version) def load_plugin( self, diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index bcd93875..e13b3e5b 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -149,18 +149,27 @@ class PluginStoreManager: # loser can end up renaming the winner's in-progress install aside # mid-download, stealing its own rollback safety net. Keyed by # plugin_id so unrelated plugins still update concurrently. - self._reinstall_locks: Dict[str, threading.Lock] = {} + # Reentrant: install_plugin takes this lock, and _reinstall_with_rollback + # holds it across its call to install_plugin. A plain Lock would + # self-deadlock on that nesting. + self._reinstall_locks: Dict[str, "threading.RLock"] = {} self._reinstall_locks_guard = threading.Lock() # Ensure plugins directory exists self.plugins_dir.mkdir(exist_ok=True) - def _get_reinstall_lock(self, plugin_id: str) -> threading.Lock: - """Lazily create (or fetch) the per-plugin reinstall lock.""" + def _get_reinstall_lock(self, plugin_id: str): + """Lazily create (or fetch) the per-plugin reinstall lock. + + Reentrant by necessity: `install_plugin` acquires it to protect its + set-aside/restore, and `_reinstall_with_rollback` holds it across its + own call to `install_plugin`. With a plain `Lock` that nesting + deadlocks the request thread. + """ with self._reinstall_locks_guard: lock = self._reinstall_locks.get(plugin_id) if lock is None: - lock = threading.Lock() + lock = threading.RLock() self._reinstall_locks[plugin_id] = lock return lock @@ -1192,6 +1201,90 @@ class PluginStoreManager: return next((p for p in plugins if p.get('id') == plugin_id), None) def install_plugin(self, plugin_id: str, branch: Optional[str] = None) -> bool: + """Install a plugin, keeping any existing install until the new one is + known good. + + `_install_plugin_impl` deletes the existing directory *before* + downloading, so every failure after that point — a dropped connection, a + malformed manifest, or the compatibility gate refusing the new version — + left the user with no plugin at all. `_reinstall_with_rollback` gives the + *update* path exactly this protection; a direct install had none, and the + compatibility gate added a new way to reach it. + + Pass-through when nothing is installed, and when called from + `_reinstall_with_rollback`, which has already moved the old copy aside. + + The aside name embeds '.standalone-backup-' so plugin discovery + (`plugin_manager._scan_directory_for_plugins`) skips it even though it + still holds a manifest.json. + + Held under the per-plugin reinstall lock for the same reason + `_reinstall_with_rollback` is: the web UI runs Flask with + threaded=True, so a double-clicked Install button gives two threads the + same plugin_id. Interleaved, one thread's restore would delete the + other's freshly installed copy. The lock is reentrant because the + rollback path already holds it when it calls in here. + """ + with self._get_reinstall_lock(plugin_id): + plugin_path = self.plugins_dir / plugin_id + if not plugin_path.exists(): + return self._install_plugin_impl(plugin_id, branch) + + backup_path = plugin_path.with_name( + f"{plugin_path.name}.standalone-backup-preinstall") + if backup_path.exists() and not self._safe_remove_directory(backup_path): + # Can't stage a safety net. Better to attempt the install than + # to refuse outright, which is what callers got before this + # existed. + self.logger.warning( + "Could not clear stale pre-install backup for %s at %s; " + "installing without a rollback net", plugin_id, backup_path) + return self._install_plugin_impl(plugin_id, branch) + + try: + plugin_path.rename(backup_path) + except OSError as e: + self.logger.warning( + "Could not set aside existing install of %s (%s); " + "installing without a rollback net", plugin_id, e) + return self._install_plugin_impl(plugin_id, branch) + + try: + installed = self._install_plugin_impl(plugin_id, branch) + except Exception: + self._restore_preinstall_backup(plugin_id, plugin_path, backup_path) + raise + + if installed: + if not self._safe_remove_directory(backup_path): + self.logger.warning( + "Install of %s succeeded but the previous copy at %s " + "could not be removed; it will be cleared on the next " + "install", plugin_id, backup_path) + return True + + self._restore_preinstall_backup(plugin_id, plugin_path, backup_path) + return False + + def _restore_preinstall_backup( + self, plugin_id: str, plugin_path: Path, backup_path: Path + ) -> None: + """Put the previous install back after a failed (re)install.""" + self.logger.error( + "Install of %s failed; restoring the previous version", plugin_id) + try: + if plugin_path.exists(): + # Partial download debris from the failed install. + self._safe_remove_directory(plugin_path) + backup_path.rename(plugin_path) + self.logger.info("Restored previous install of %s", plugin_id) + except OSError as e: + self.logger.error( + "CRITICAL: could not restore %s from %s: %s. The previous " + "install is preserved there — rename it back manually.", + plugin_id, backup_path, e) + + def _install_plugin_impl(self, plugin_id: str, branch: Optional[str] = None) -> bool: """ Install a plugin from the official registry. Always installs the latest commit from the repository's default branch (or specified branch). @@ -1333,6 +1426,26 @@ class PluginStoreManager: self._safe_remove_directory(plugin_path) return False + # Refuse a plugin that needs a newer core than this one. The + # registry carries no compatibility field, so the floor is only + # knowable once the files are down — checking here, before + # dependency installation, is the earliest possible point. + # + # Refusing costs the user nothing: on an update this returns + # False and _reinstall_with_rollback restores the version they + # already had. Allowing it costs them a plugin that raises + # ModuleNotFoundError at load and is reported only as one line + # in the journal. See docs/SPORTS_UNIFICATION.md (phase B4/B6). + from src import __version__ as core_version + from src.plugin_system import compatibility + + compatible, reason = compatibility.check(manifest, core_version) + if not compatible: + self.logger.error( + "Refusing to install %s: %s", plugin_id, reason) + self._safe_remove_directory(plugin_path) + return False + if 'entry_point' not in manifest: manifest['entry_point'] = 'manager.py' manifest_modified = True diff --git a/test/test_install_preserves_existing.py b/test/test_install_preserves_existing.py new file mode 100644 index 00000000..ff5b5554 --- /dev/null +++ b/test/test_install_preserves_existing.py @@ -0,0 +1,222 @@ +"""A failed (re)install must not destroy the working plugin it replaced. + +`_install_plugin_impl` deletes the existing plugin directory *before* it +downloads anything, so any failure after that point used to leave the user with +nothing. The update path was protected — `_reinstall_with_rollback` renames the +old copy aside first — but a direct `install_plugin` was not, and the +compatibility gate added a new way to fail late: a plugin whose declared floor +exceeds the running core is now refused *after* the old copy is already gone. + +Concretely, without the wrapper: a user on core 3.1.0 with a working +hockey-scoreboard clicks Install; the new manifest floors at 3.2.0; the gate +refuses; the plugin they had is deleted. Floors are hand-written and can be +over-declared, so this could remove a plugin that was working fine. +""" + +import json +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +from src.plugin_system.store_manager import PluginStoreManager + + +@pytest.fixture +def store(tmp_path): + plugins_dir = tmp_path / "plugin-repos" + plugins_dir.mkdir() + mgr = PluginStoreManager(plugins_dir=str(plugins_dir)) + mgr.logger = MagicMock() + return mgr, plugins_dir + + +def _existing_install(plugins_dir: Path, plugin_id: str, marker: str) -> Path: + path = plugins_dir / plugin_id + path.mkdir(parents=True) + (path / "manifest.json").write_text( + json.dumps({"id": plugin_id, "name": plugin_id, "class_name": "P", + "display_modes": ["a"], "version": "1.0.0"}), + encoding="utf-8") + (path / "marker.txt").write_text(marker, encoding="utf-8") + return path + + +class TestFailedInstallPreservesPrevious: + def test_failed_install_restores_the_old_copy(self, store, monkeypatch): + mgr, plugins_dir = store + path = _existing_install(plugins_dir, "hockey-scoreboard", "the-original") + + monkeypatch.setattr(mgr, "_install_plugin_impl", lambda *a, **k: False) + + assert mgr.install_plugin("hockey-scoreboard") is False + assert path.exists(), "the previous install must be restored" + assert (path / "marker.txt").read_text() == "the-original" + + def test_raising_install_restores_and_reraises(self, store, monkeypatch): + mgr, plugins_dir = store + path = _existing_install(plugins_dir, "hockey-scoreboard", "the-original") + + def boom(*a, **k): + raise RuntimeError("network died mid-install") + + monkeypatch.setattr(mgr, "_install_plugin_impl", boom) + + with pytest.raises(RuntimeError): + mgr.install_plugin("hockey-scoreboard") + assert path.exists() + assert (path / "marker.txt").read_text() == "the-original" + + def test_successful_install_clears_the_backup(self, store, monkeypatch): + mgr, plugins_dir = store + _existing_install(plugins_dir, "hockey-scoreboard", "the-original") + + def succeed(plugin_id, branch=None): + _existing_install(plugins_dir, plugin_id, "the-new-one") + return True + + monkeypatch.setattr(mgr, "_install_plugin_impl", succeed) + + assert mgr.install_plugin("hockey-scoreboard") is True + assert (plugins_dir / "hockey-scoreboard" / "marker.txt").read_text() == "the-new-one" + leftovers = [p.name for p in plugins_dir.iterdir() if "backup" in p.name] + assert not leftovers, f"backup left behind: {leftovers}" + + def test_backup_name_is_invisible_to_plugin_discovery(self, store, monkeypatch): + """A backup that discovery can see becomes a duplicate plugin entry; + the marker '.standalone-backup-' is what makes it skip.""" + mgr, plugins_dir = store + _existing_install(plugins_dir, "hockey-scoreboard", "the-original") + + seen = {} + + def capture(plugin_id, branch=None): + seen["dirs"] = sorted(p.name for p in plugins_dir.iterdir()) + return False + + monkeypatch.setattr(mgr, "_install_plugin_impl", capture) + mgr.install_plugin("hockey-scoreboard") + + backups = [d for d in seen["dirs"] if d != "hockey-scoreboard"] + assert backups, "expected the old copy to be set aside during install" + for name in backups: + assert ".standalone-backup-" in name, ( + f"{name} would be picked up by " + "plugin_manager._scan_directory_for_plugins as a real plugin") + + def test_fresh_install_is_a_pass_through(self, store, monkeypatch): + """Nothing installed means nothing to protect; don't create stray dirs.""" + mgr, plugins_dir = store + calls = [] + monkeypatch.setattr( + mgr, "_install_plugin_impl", + lambda *a, **k: calls.append(a) or True) + + assert mgr.install_plugin("brand-new") is True + assert calls, "the implementation must still be called" + assert list(plugins_dir.iterdir()) == [] + + def test_stale_backup_from_a_crash_does_not_block(self, store, monkeypatch): + mgr, plugins_dir = store + _existing_install(plugins_dir, "hockey-scoreboard", "the-original") + stale = plugins_dir / "hockey-scoreboard.standalone-backup-preinstall" + stale.mkdir() + (stale / "junk.txt").write_text("from a previous crash", encoding="utf-8") + + monkeypatch.setattr(mgr, "_install_plugin_impl", lambda *a, **k: False) + + assert mgr.install_plugin("hockey-scoreboard") is False + assert (plugins_dir / "hockey-scoreboard" / "marker.txt").read_text() == "the-original" + + +class TestUpdatePathStillWorks: + def test_reinstall_with_rollback_is_not_double_wrapped(self, store, monkeypatch): + """_reinstall_with_rollback moves the plugin aside itself, so by the + time install_plugin runs there is nothing at the original path and the + wrapper must be a pass-through rather than staging a second backup.""" + mgr, plugins_dir = store + path = _existing_install(plugins_dir, "hockey-scoreboard", "the-original") + + observed = {} + + def impl(plugin_id, branch=None): + observed["dirs"] = sorted(p.name for p in plugins_dir.iterdir()) + return False + + monkeypatch.setattr(mgr, "_install_plugin_impl", impl) + + assert mgr._reinstall_with_rollback("hockey-scoreboard", path) is False + # Exactly one aside directory existed during the attempt — rollback's. + assert observed["dirs"] == ["hockey-scoreboard.standalone-backup-migrating"] + # And the user still has their plugin. + assert (plugins_dir / "hockey-scoreboard" / "marker.txt").read_text() == "the-original" + + +class TestConcurrency: + """The web UI runs Flask threaded, so a double-clicked Install button puts + two threads on the same plugin_id. `_reinstall_with_rollback` already + guarded against this; the install wrapper has to as well, or one thread's + restore deletes the other's freshly installed copy.""" + + def test_rollback_calling_install_does_not_deadlock(self, store, monkeypatch): + """The rollback path holds the per-plugin lock across its call to + install_plugin. A non-reentrant lock would hang the request thread + forever — this test would time out rather than fail.""" + import threading + + mgr, plugins_dir = store + path = _existing_install(plugins_dir, "hockey-scoreboard", "the-original") + monkeypatch.setattr( + mgr, "_install_plugin_impl", + lambda pid, branch=None: bool(_existing_install(plugins_dir, pid, "new"))) + + done = threading.Event() + result = {} + + def run(): + result["ok"] = mgr._reinstall_with_rollback("hockey-scoreboard", path) + done.set() + + t = threading.Thread(target=run, daemon=True) + t.start() + assert done.wait(timeout=10), ( + "install_plugin deadlocked when called from _reinstall_with_rollback " + "— the per-plugin lock must be reentrant" + ) + assert result["ok"] is True + + def test_concurrent_installs_serialize(self, store, monkeypatch): + """Two threads installing the same plugin must not interleave their + set-aside/restore, and the survivor must be a complete install.""" + import threading + + mgr, plugins_dir = store + _existing_install(plugins_dir, "hockey-scoreboard", "the-original") + + in_flight = [] + overlap = [] + + def slow_impl(plugin_id, branch=None): + in_flight.append(1) + if len(in_flight) > 1: + overlap.append(1) + threading.Event().wait(0.05) + _existing_install(plugins_dir, plugin_id, "installed") + in_flight.pop() + return True + + monkeypatch.setattr(mgr, "_install_plugin_impl", slow_impl) + + threads = [threading.Thread(target=mgr.install_plugin, + args=("hockey-scoreboard",), daemon=True) + for _ in range(2)] + for t in threads: + t.start() + for t in threads: + t.join(timeout=10) + assert not t.is_alive(), "concurrent install hung" + + assert not overlap, "two installs of the same plugin ran concurrently" + assert (plugins_dir / "hockey-scoreboard" / "marker.txt").exists() + leftovers = [p.name for p in plugins_dir.iterdir() if "backup" in p.name] + assert not leftovers, f"backup left behind: {leftovers}" diff --git a/test/test_plugin_compatibility_gate.py b/test/test_plugin_compatibility_gate.py new file mode 100644 index 00000000..f0f252f1 --- /dev/null +++ b/test/test_plugin_compatibility_gate.py @@ -0,0 +1,232 @@ +"""The install/update gate, and the shared compatibility rules behind it. + +Before this existed, `ledmatrix_min_version` was decoration: the loader logged +an advisory warning and the store never looked at the core version at all, so a +routine store update happily delivered a plugin that could not run. Deleting a +plugin's bundled fallback under those conditions would have handed un-updated +users a scoreboard that fails to load with one line in the journal. + +The rules being pinned here, in priority order: + +1. Refuse only on **evidence**. Undeclared floor, unparseable version on either + side, or a core whose self-reported version is untrustworthy → allow. A + wrong refusal breaks a working install; a wrong allowance degrades to the + behavior we already had. +2. A core below `TRUSTWORTHY_FLOOR` is *unknown*, not old. The v3.1.0 release + reports `1.0.0` while nearly every manifest floors at `2.0.0`; blocking on + that number would stop those users installing anything at all. +3. The loader and the store must agree, because they read the same manifests. +""" + +import json +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +from src.plugin_system import compatibility + + +# -------------------------------------------------------------------------- +# Floor resolution — every spelling published plugins actually use +# -------------------------------------------------------------------------- + +class TestDeclaredMinVersion: + def test_top_level_min_ledmatrix_version(self): + assert compatibility.declared_min_version( + {"min_ledmatrix_version": "3.2.0"}) == "3.2.0" + + def test_requires_block(self): + assert compatibility.declared_min_version( + {"requires": {"min_ledmatrix_version": "3.1.0"}}) == "3.1.0" + + def test_versions_array_new_spelling(self): + assert compatibility.declared_min_version( + {"versions": [{"ledmatrix_min_version": "3.2.0"}]}) == "3.2.0" + + def test_versions_array_deprecated_spelling(self): + """Most published manifests still say `ledmatrix_min`; ignoring it + would silently exempt them from the gate.""" + assert compatibility.declared_min_version( + {"versions": [{"ledmatrix_min": "2.0.0"}]}) == "2.0.0" + + def test_absent(self): + assert compatibility.declared_min_version({"id": "x"}) is None + + def test_requires_present_but_null(self): + assert compatibility.declared_min_version({"requires": None}) is None + + +# -------------------------------------------------------------------------- +# The decision itself +# -------------------------------------------------------------------------- + +class TestCheck: + def test_blocks_when_plugin_needs_a_newer_core(self): + ok, reason = compatibility.check( + {"name": "Hockey Scoreboard", "min_ledmatrix_version": "3.2.0"}, "3.1.0") + assert ok is False + assert "3.2.0" in reason and "3.1.0" in reason + assert "Hockey Scoreboard" in reason + + def test_allows_equal_version(self): + ok, _ = compatibility.check({"min_ledmatrix_version": "3.2.0"}, "3.2.0") + assert ok is True + + def test_allows_newer_core(self): + ok, _ = compatibility.check({"min_ledmatrix_version": "3.2.0"}, "4.0.0") + assert ok is True + + def test_allows_when_no_floor_declared(self): + ok, reason = compatibility.check({"id": "x"}, "3.2.0") + assert ok is True and reason is None + + def test_untrustworthy_core_version_allows_everything(self): + """The v3.1.0 release reports 1.0.0. Nearly every manifest floors at + 2.0.0, so blocking here would stop those users installing any plugin + at all — strictly worse than the problem being solved.""" + ok, reason = compatibility.check( + {"min_ledmatrix_version": "3.2.0"}, "1.0.0") + assert ok is True and reason is None + + def test_unparseable_core_version_allows(self): + ok, _ = compatibility.check({"min_ledmatrix_version": "3.2.0"}, "not-a-version") + assert ok is True + + def test_unparseable_floor_allows(self): + ok, _ = compatibility.check({"min_ledmatrix_version": {"nope": 1}}, "3.2.0") + assert ok is True + + def test_v_prefix_tolerated_on_both_sides(self): + ok, _ = compatibility.check({"min_ledmatrix_version": "v3.3.0"}, "v3.2.0") + assert ok is False + + @pytest.mark.parametrize("floor,core,expected_ok", [ + ("3.2.0", "3.2.1", True), + ("3.2.1", "3.2.0", False), + ("3.10.0", "3.9.0", False), # numeric compare, not lexical + ("3.9.0", "3.10.0", True), + ]) + def test_ordering(self, floor, core, expected_ok): + ok, _ = compatibility.check({"min_ledmatrix_version": floor}, core) + assert ok is expected_ok + + +# -------------------------------------------------------------------------- +# The gate in install_plugin +# -------------------------------------------------------------------------- + +def _write_plugin(plugins_dir: Path, plugin_id: str, manifest: dict) -> Path: + path = plugins_dir / plugin_id + path.mkdir(parents=True) + (path / "manifest.json").write_text(json.dumps(manifest), encoding="utf-8") + (path / "manager.py").write_text("class P: pass\n", encoding="utf-8") + return path + + +@pytest.fixture +def store(tmp_path, monkeypatch): + """A PluginStoreManager whose download step is stubbed to drop a plugin + directory in place, so the test exercises the post-download validation + path without touching the network.""" + from src.plugin_system.store_manager import PluginStoreManager + + plugins_dir = tmp_path / "plugin-repos" + plugins_dir.mkdir() + mgr = PluginStoreManager(plugins_dir=str(plugins_dir)) + mgr.logger = MagicMock() + return mgr, plugins_dir + + +class TestInstallGate: + """`install_plugin` is the chokepoint: `_reinstall_with_rollback` calls it, + so gating there covers updates too, and a refused update restores the + version the user already had.""" + + def _install_with_manifest(self, store, manifest, core_version, monkeypatch): + mgr, plugins_dir = store + plugin_id = manifest["id"] + + monkeypatch.setattr( + mgr, "get_plugin_info", + lambda *a, **k: {"repo": "https://example.invalid/r", + "plugin_path": f"plugins/{plugin_id}", + "branch": "main"}) + # Stand in for the download: put the files where install_plugin expects. + monkeypatch.setattr( + mgr, "_install_from_monorepo", + lambda *a, **k: bool(_write_plugin(plugins_dir, plugin_id, manifest))) + monkeypatch.setattr(mgr, "_install_from_monorepo_api", lambda *a, **k: False) + monkeypatch.setattr(mgr, "_install_dependencies", lambda *a, **k: True) + + import src + monkeypatch.setattr(src, "__version__", core_version) + return mgr.install_plugin(plugin_id), plugins_dir / plugin_id + + def test_refuses_and_leaves_nothing_behind(self, store, monkeypatch): + manifest = { + "id": "needs-newer", "name": "Needs Newer", "class_name": "P", + "display_modes": ["a"], "min_ledmatrix_version": "9.9.9", + } + ok, path = self._install_with_manifest(store, manifest, "3.2.0", monkeypatch) + + assert ok is False, "install must refuse a plugin that needs a newer core" + assert not path.exists(), ( + "a refused install must not leave a half-installed directory — " + "plugin discovery would pick it up and fail to load it") + + def test_allows_a_compatible_plugin(self, store, monkeypatch): + manifest = { + "id": "fine", "name": "Fine", "class_name": "P", + "display_modes": ["a"], "min_ledmatrix_version": "3.0.0", + } + ok, path = self._install_with_manifest(store, manifest, "3.2.0", monkeypatch) + + assert ok is True + assert (path / "manifest.json").exists() + + def test_untrustworthy_core_does_not_block_installs(self, store, monkeypatch): + """Regression guard for the worst possible outcome of this feature: + users on the v3.1.0 release (which reports 1.0.0) must not be locked + out of the plugin store entirely.""" + manifest = { + "id": "floored", "name": "Floored", "class_name": "P", + "display_modes": ["a"], "versions": [{"ledmatrix_min": "2.0.0"}], + } + ok, path = self._install_with_manifest(store, manifest, "1.0.0", monkeypatch) + + assert ok is True, ( + "a core below the trustworthy floor must not block installs — " + "nearly every published manifest floors at 2.0.0") + assert (path / "manifest.json").exists() + + +class TestLoaderAndStoreAgree: + """Both read the same manifests; a disagreement means one of them is + lying to the user.""" + + @pytest.mark.parametrize("manifest,core,expected", [ + ({"min_ledmatrix_version": "3.2.0"}, "3.1.0", False), + ({"versions": [{"ledmatrix_min": "2.0.0"}]}, "3.2.0", True), + ({"versions": [{"ledmatrix_min_version": "9.0.0"}]}, "3.2.0", False), + ({}, "3.2.0", True), + ]) + def test_same_verdict(self, manifest, core, expected): + from src.plugin_system.plugin_loader import PluginLoader + + store_ok, _ = compatibility.check(manifest, core) + assert store_ok is expected + + # The loader resolves the floor through the same helper, so a + # divergence in spelling handling would show up here. + loader_needed = compatibility.parse_semver( + compatibility.declared_min_version(manifest)) + current = compatibility.parse_semver(core) + loader_would_warn = ( + loader_needed is not None + and current is not None + and current >= compatibility.TRUSTWORTHY_FLOOR + and loader_needed > current + ) + assert loader_would_warn is (not expected) + assert hasattr(PluginLoader, "_warn_if_incompatible")