feat(store): refuse to install a plugin that needs a newer core (re-target of #429)

[ChuckBuilds]

Re-opens #429 against main. #429 was merged, but into its stacked base push/version-reporting — which had already been squash-merged as #428. So the merge landed on an orphaned branch and none of this reached main. Verified: src/plugin_system/compatibility.py is absent from main, and store_manager.py there has neither _install_plugin_impl nor threading.RLock().

Nothing was lost. These are the same three commits, cherry-picked onto current main (which now also carries #428 and #430). No conflicts — #430 touched installer scripts, not the plugin system.

The review-thread fixes from #427 are already on main and are not repeated here.

---

Why

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. This is the change B6 — the sports-unification sunset — depends on.

The gate

Lives in install_plugin, after the manifest is on disk and before dependencies install — the earliest knowable point, since the registry carries no compatibility field. It is also the chokepoint: _reinstall_with_rollback calls install_plugin, so a refused update restores the version the user already had.

Floor resolution moves to src/plugin_system/compatibility.py, shared with the loader so the two cannot drift.

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 load-bearing: the v3.1.0 release reports __version__ = "1.0.0" while nearly every manifest floors at 2.0.0, so a strict gate would lock those users out of the store entirely. A regression test pins it.

The gate is inert today — all 42 manifests declare exactly 2.0.0. It arms when B5 starts declaring 3.2.0 floors.

Known gap, recorded in #427: the gate reads versions[].ledmatrix_min_version, not compatible_versions — which is the schema-required field and supports upper bounds. Harmless now (no manifest uses one), must close before B6.

Two bugs found while validating on hardware

1. A failed install destroyed the plugin it replaced. _install_plugin_impl deletes the existing directory before downloading; the update path was protected by _reinstall_with_rollback, a direct install was not — and the gate added a new way to fail late. install_plugin is now a wrapper that sets the old copy aside and restores on failure, including on exceptions. The aside name embeds .standalone-backup- because plugin_manager._scan_directory_for_plugins:177 keys on that substring.

2. The fix for Stocks #1 would have deadlocked every plugin update. The wrapper needs the per-plugin lock, but _reinstall_with_rollback holds it across its call to install_plugin and threading.Lock is not reentrant — so update_plugin → _reinstall_with_rollback → install_plugin would hang the request thread. Confirmed by reverting to a plain Lock: the test times out at 10s. Now RLock.

Verified

792 core unit tests pass on this branch against current main.

On devpi (Pi, Python 3.13.5, real registry and network): fresh install, reinstall-over-existing, failed-reinstall-restores, and a real update_plugin through the full rollback path in 13.1s. 22 plugins load, web API and UI 200.

Still outstanding

CI does not run the three new test files — the test.yml enrollment needs the workflow scope, which the pushing token still lacks. That follow-up is unchanged.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5

Summary by CodeRabbit

• New Features

  • Added plugin compatibility checks based on declared minimum core versions.
  • Incompatible official plugins are rejected before installation.
  • Compatibility checks tolerate missing or invalid version information.

• Bug Fixes

  • Existing plugin installations are preserved and restored when updates or reinstalls fail.
  • Improved rollback handling prevents deadlocks and cleans up temporary backups.
  • Concurrent installations of the same plugin are safely serialized.

[coderabbitai]

Warning

Review limit reached

@ChuckBuilds, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 36 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0a75df52-50b6-410d-a353-9211ad23f043

📥 Commits

Reviewing files that changed from the base of the PR and between 1bbf934 and b32563c.

📒 Files selected for processing (2)

• .github/workflows/release-version-check.yml
• .github/workflows/test.yml

📝 Walkthrough

Walkthrough

The PR adds shared plugin compatibility checks, enforces them during official installation, and protects existing installations with hidden backups, restoration handling, reentrant locks, and concurrency regression tests.

Changes

Plugin installation behavior

Layer / File(s)	Summary
Compatibility contract and decisions src/plugin_system/compatibility.py, test/test_plugin_compatibility_gate.py	Adds semantic-version parsing, manifest requirement resolution, the trustworthy core-version floor, compatibility decisions, and coverage for supported and invalid inputs.
Compatibility enforcement during installation src/plugin_system/plugin_loader.py, src/plugin_system/store_manager.py, test/test_plugin_compatibility_gate.py	The loader and store use the shared compatibility module. Incompatible plugins are rejected before dependency installation. Tests verify matching loader and store results.
Backup restoration and concurrent installation safety src/plugin_system/store_manager.py, test/test_install_preserves_existing.py	Existing installations move to hidden backups during installation. Failed installs restore backups, successful installs remove them, and reentrant locks support nested rollback installs.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PluginStoreManager
  participant compatibility.check
  participant PluginDirectory
  participant DependencyInstaller

  PluginStoreManager->>PluginDirectory: Move existing installation to hidden backup
  PluginStoreManager->>compatibility.check: Validate manifest against core version
  compatibility.check-->>PluginStoreManager: Return compatibility verdict
  alt Compatible
    PluginStoreManager->>DependencyInstaller: Install plugin dependencies
    PluginStoreManager->>PluginDirectory: Remove backup after success
  else Incompatible or failed
    PluginStoreManager->>PluginDirectory: Delete partial installation
    PluginStoreManager->>PluginDirectory: Restore hidden backup
  end

Loading

Possibly related PRs

• ChuckBuilds/LEDMatrix#405: Both PRs modify plugin installation and update rollback logic to preserve existing installations with temporary backups.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: refusing plugin installation when the plugin requires a newer core version.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/compatibility-gate-onto-main

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 38 complexity · 2 duplication

Metric	Results
Complexity	38
Duplication	2

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[coderabbitai]

🧹 Nitpick comments (3)

src/plugin_system/plugin_loader.py (1)

685-698: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Confirm _parse_semver has no remaining callers, then remove it.

_warn_if_incompatible now uses compatibility.parse_semver (Line 712, 719) instead of self._parse_semver. This static method appears to duplicate compatibility.parse_semver exactly. Keeping both risks the two copies drifting apart if one is fixed (e.g., the dead-except issue flagged in compatibility.py) without the other being updated.

#!/bin/bash
# Description: Check whether PluginLoader._parse_semver still has any callers.
rg -n '_parse_semver' src/plugin_system/plugin_loader.py
rg -n '\._parse_semver\(' src -g '*.py'

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/plugin_system/plugin_loader.py` around lines 685 - 698, Confirm that no
callers remain for PluginLoader._parse_semver, then remove this static method
and its associated dead code. Preserve _warn_if_incompatible’s use of
compatibility.parse_semver as the single semver parsing implementation.

test/test_plugin_compatibility_gate.py (1)

204-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exercise the real _warn_if_incompatible, not a re-derivation of its logic.

This test recomputes the loader's warn/no-warn decision inline (Lines 220-231) instead of invoking PluginLoader._warn_if_incompatible on an instance. The only real touch of PluginLoader is hasattr(PluginLoader, "_warn_if_incompatible") (Line 232), which only confirms the method exists. If the loader's implementation ever diverges from this reimplementation (e.g., a future change to the debug-log branch), this test keeps passing because it checks its own copy of the logic, not the loader's.

Instantiate PluginLoader with a mocked logger, monkeypatch src.__version__, call the real method, and assert on logger.warning.called to genuinely pin the cross-module agreement this test class is named for.

def test_same_verdict(self, manifest, core, expected, monkeypatch):
    import src
    from src.plugin_system.plugin_loader import PluginLoader

    store_ok, _ = compatibility.check(manifest, core)
    assert store_ok is expected

    monkeypatch.setattr(src, "__version__", core)
    loader = PluginLoader(logger=MagicMock())
    loader._warn_if_incompatible("some-plugin", manifest)
    assert loader.logger.warning.called is (not expected)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/test_plugin_compatibility_gate.py` around lines 204 - 232, Update
TestLoaderAndStoreAgree.test_same_verdict to exercise
PluginLoader._warn_if_incompatible directly instead of recomputing its decision
with compatibility.parse_semver and compatibility.declared_min_version. Add
monkeypatch, set src.__version__ to core, instantiate PluginLoader with a mocked
logger, invoke _warn_if_incompatible with the manifest, and assert
logger.warning.called matches not expected.

test/test_install_preserves_existing.py (1)

45-129: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add coverage for the "no rollback net" fallback branches.

TestFailedInstallPreservesPrevious covers the happy path of setting a backup aside, but two fallback branches in install_plugin (store_manager.py:1203-1267) have no test:

• backup_path.exists() and not self._safe_remove_directory(backup_path) → falls back to installing without a rollback net.
• plugin_path.rename(backup_path) raising OSError → falls back to installing without a rollback net.

The source comments mark these as deliberate degrade-gracefully behavior ("Better to attempt the install than to refuse outright"). A regression that turns either fallback into an outright refusal, or that raises instead of degrading, would pass this suite unnoticed.

Add two tests that mock _safe_remove_directory to return False and mock Path.rename (or the plugin path's .rename) to raise OSError, then assert _install_plugin_impl is still invoked.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/test_install_preserves_existing.py` around lines 45 - 129, Add two tests
to TestFailedInstallPreservesPrevious covering install_plugin’s no-rollback
fallback paths: one with an existing backup where _safe_remove_directory returns
False, and one where the existing plugin path’s rename raises OSError. In both
cases, mock _install_plugin_impl and assert it is still invoked, confirming
installation degrades gracefully instead of refusing or propagating the setup
error.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/plugin_system/plugin_loader.py`:
- Around line 685-698: Confirm that no callers remain for
PluginLoader._parse_semver, then remove this static method and its associated
dead code. Preserve _warn_if_incompatible’s use of compatibility.parse_semver as
the single semver parsing implementation.

In `@test/test_install_preserves_existing.py`:
- Around line 45-129: Add two tests to TestFailedInstallPreservesPrevious
covering install_plugin’s no-rollback fallback paths: one with an existing
backup where _safe_remove_directory returns False, and one where the existing
plugin path’s rename raises OSError. In both cases, mock _install_plugin_impl
and assert it is still invoked, confirming installation degrades gracefully
instead of refusing or propagating the setup error.

In `@test/test_plugin_compatibility_gate.py`:
- Around line 204-232: Update TestLoaderAndStoreAgree.test_same_verdict to
exercise PluginLoader._warn_if_incompatible directly instead of recomputing its
decision with compatibility.parse_semver and compatibility.declared_min_version.
Add monkeypatch, set src.__version__ to core, instantiate PluginLoader with a
mocked logger, invoke _warn_if_incompatible with the manifest, and assert
logger.warning.called matches not expected.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c5b00e9c-07c2-426e-9a37-48cd00689dfb

📥 Commits

Reviewing files that changed from the base of the PR and between f2b246e and 1bbf934.

📒 Files selected for processing (5)

• src/plugin_system/compatibility.py
• src/plugin_system/plugin_loader.py
• src/plugin_system/store_manager.py
• test/test_install_preserves_existing.py
• test/test_plugin_compatibility_gate.py