rick/LEDMatrix
1
0
Fork 0
mirror of https://github.com/ChuckBuilds/LEDMatrix.git synced 2026-10-18 12:59:12 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(store): gate the git-pull update path

Browse Source 
`install_plugin` gates every route that re-downloads, `_reinstall_with_rollback`
included. `update_plugin` has one branch that re-downloads nothing: a git
checkout pulls in place, installs dependencies, and returns True. A pull could
therefore deliver a manifest flooring above this core and nothing would notice
until the plugin failed to load — which surfaces as one line in the journal and
a display that silently stopped appearing.

Checked after the pull rather than before it, for the same reason
`_install_plugin_impl` checks after the download: the registry carries no
compatibility field, so the incoming floor is only knowable once the new commit
is on disk.

Undone with `git reset --hard` to the pre-pull commit rather than by removing
the directory. This is a live checkout, the old commit is still in the object
store, and the reset leaves the user on the exact version they were already
running — the same promise `_reinstall_with_rollback` makes, reached by the
means this path actually has, with no window where the plugin directory does
not exist. An unreadable manifest allows: it is not evidence of a floor.

Scope, stated plainly: monorepo plugins install as archives and update through
`_reinstall_with_rollback`, so they were already gated. Only registry entries
with no `plugin_path` reach this branch. It is closed anyway because the sunset
rule in the plugins repo's `08-shared-sports-code.md` names, as condition 3,
that the core enforces the floor "at install/update time" — and B6 rests on
that being true rather than merely written down. `install_from_url` is still
ungated; the tests say so rather than letting the next reader assume otherwise.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9
This commit is contained in:
ChuckBuilds
2026-08-31 12:01:50 -04:00
co-authored by Claude Opus 5
parent cbc540a679
commit 217aac780b
3 changed files with 283 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/SPORTS_UNIFICATION.md
+14 -1
View File
@@ -27,7 +27,7 @@ These are independent concerns. Conflating them is what produces god classes.
| Plugin loads on a core that predates a module | Guarded import with a bundled fallback (`try: from src.X import Y / except ModuleNotFoundError: from y import Y`) |
| Plugin loads on a core that predates a *method* | Capability probing — `hasattr(SportsCore, "_detect_stale_games")` — never a version comparison. The loader's compat check is advisory-only (it logs and continues), so probing is the real protection. |
| Core changes never break a plugin's rendering | The **view-model contract**: `_extract_game_details_common` returns a dict whose `GUARANTEED_KEYS` are frozen by `test/test_skin_system.py::TestViewModelContract`. Keys may be added, never renamed or removed. |
| A plugin can drop its bundled copy safely | The **sunset rule**: its manifest must floor `ledmatrix_min_version` at the first core release shipping the module (recorded in `CHANGELOG.md`) — *necessary but not sufficient*. Nothing enforces that floor today, so the copy also waits for the B6 gate below. |
| A plugin can drop its bundled copy safely | The **sunset rule**: its manifest must floor `ledmatrix_min_version` at the first core release shipping the module (recorded in `CHANGELOG.md`) — *necessary but not sufficient*. The store enforces that floor on every route that installs or updates, but a floor cannot reach a user who never updates, so the copy also waits for the B6 gate below. |
The core API is **additive-only**. A method the plugins call is never removed or
given a new required parameter; new behavior arrives as new methods with
@@ -234,6 +234,19 @@ a floor can be trusted against, and today it is not:
compares the plugin's manifest version against the registry's
`latest_version` and nothing else.
*Fixed, in two parts.* `install_plugin` gained the gate in #431/#433, which
covers every route that re-downloads, `_reinstall_with_rollback` included.
`update_plugin`'s git branch pulls in place and re-downloads nothing, so it
stayed ungated until `_gate_pulled_commit` closed it — checked after the pull
(the registry carries no floor field, so the incoming floor is unknowable
before it) and undone with `git reset --hard` to the pre-pull commit. That
route is rare in practice, since monorepo plugins install as archives; it was
closed because the sunset rule in the plugins repo's
`08-shared-sports-code.md` states as **condition 3** that the core enforces
the floor "at install/update time", and B6 rests on that being true rather
than merely written down. `install_from_url` — sideloading a plugin from a
URL — is still ungated.
So B4 is: tag and release 3.2.0; make the tag, the release, and `__version__`
agree, and keep them agreeing; reconsider the `< 2.0.0` skip; migrate manifests
from `ledmatrix_min` to `ledmatrix_min_version`; and add the install/update
src/plugin_system/store_manager.py
+85
View File
@@ -2583,6 +2583,83 @@ class PluginStoreManager:
self.logger.error(f"Error uninstalling plugin {plugin_id}: {e}")
return False
def _gate_pulled_commit(self, plugin_id: str, plugin_path: Path,
previous_sha: Optional[str]) -> bool:
"""Apply the compatibility gate to a commit that arrived via git pull.
Every other route into an installed plugin goes through
``install_plugin``, which gates in ``_install_plugin_impl``. This one
did not: a ``git pull`` could deliver a manifest flooring above this
core and nothing would notice until the plugin failed to load, which
surfaces as one line in the journal and a scoreboard that silently
stopped appearing.
Checked after the pull rather than before it, for the same reason
``_install_plugin_impl`` checks after the download: the registry
carries no compatibility field, so the incoming floor is only knowable
once the new commit is on disk.
Undone with ``git reset --hard`` rather than by removing the directory.
This is a live checkout, the previous commit is still in the object
store, and the reset leaves the user on the exact version they were
already running -- the same promise ``_reinstall_with_rollback`` makes,
reached by the means this path actually has. It is also the gentler
option: no window in which the plugin directory does not exist, and no
``.standalone-backup-`` debris if the process dies mid-way.
A manifest that cannot be read is not evidence of incompatibility, so
it allows. ``compatibility.check`` refuses only on evidence for the
same reason: a wrong refusal breaks a working install, while a wrong
allowance degrades to exactly the behaviour this path had before the
gate existed.
"""
manifest_path = plugin_path / "manifest.json"
try:
with open(manifest_path, 'r', encoding='utf-8') as mf:
manifest = json.load(mf)
except (OSError, ValueError) as e:
self.logger.warning(
"Could not read %s after updating %s (%s); allowing the "
"update, as an unreadable manifest declares no floor",
manifest_path, plugin_id, e)
return True
from src import __version__ as core_version
from src.plugin_system import compatibility
compatible, reason = compatibility.check(manifest, core_version)
if compatible:
return True
self.logger.error("Refusing the update to %s: %s", plugin_id, reason)
if not previous_sha:
self.logger.error(
"Cannot roll %s back: the commit it was on before the pull is "
"unknown. It is now on a version this core cannot run — "
"reinstall it from the plugin store.", plugin_id)
return False
# Any local changes were stashed before the pull and are not popped on
# the success path either, so the reset leaves the working tree exactly
# where a successful pull would have. Say "commit", not "changes".
reset = subprocess.run(
['git', '-C', str(plugin_path), 'reset', '--hard', previous_sha],
capture_output=True, text=True, timeout=60, check=False)
if reset.returncode != 0:
self.logger.error(
"CRITICAL: could not roll %s back to commit %s: %s. It is left "
"on a version this core cannot run; "
"`git -C %s reset --hard %s` restores it.",
plugin_id, previous_sha[:7],
(reset.stderr or reset.stdout or '').strip(),
plugin_path, previous_sha)
else:
self.logger.info(
"Rolled %s back to commit %s; it stays on the version it was "
"already running.", plugin_id, previous_sha[:7])
return False
def _reinstall_with_rollback(self, plugin_id: str, plugin_path: Path) -> bool:
"""Replace an installed plugin with a fresh install, atomically.
@@ -2901,6 +2978,14 @@ class PluginStoreManager:
elif updated_sha:
self.logger.info(f"Plugin {plugin_id} updated to commit {updated_sha[:7]}{stash_info}")
# The install gate, at the only point on this path where
# it can be answered. Every other route in goes through
# install_plugin, which gates in _install_plugin_impl; this
# one did not, so a pull could deliver a manifest flooring
# above this core and nothing would notice.
if not self._gate_pulled_commit(plugin_id, plugin_path, local_sha):
return False
self._install_dependencies(plugin_path)
return True
test/test_plugin_compatibility_gate.py
+184 -3
View File
@@ -19,6 +19,7 @@ The rules being pinned here, in priority order:
"""
import json
import subprocess
from pathlib import Path
from unittest.mock import MagicMock
@@ -146,9 +147,17 @@ def store(tmp_path, monkeypatch):
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."""
"""`install_plugin` is the chokepoint for every route that re-downloads:
`_reinstall_with_rollback` calls it, so a refused update restores the
version the user already had.
It is not the *only* route in, and saying so here once is cheaper than
rediscovering it. `update_plugin` also has a git branch that pulls in
place and never re-downloads; that one is gated separately by
`_gate_pulled_commit` and pinned in `TestGitPullGate` below. A third
route, `install_from_url`, is still ungated — sideloading from a URL
checks required fields but not the floor.
"""
def _install_with_manifest(self, store, manifest, core_version, monkeypatch):
mgr, plugins_dir = store
@@ -208,6 +217,178 @@ class TestInstallGate:
assert (path / "manifest.json").exists()
# --------------------------------------------------------------------------
# The git-pull update path — the one route that does not re-download
# --------------------------------------------------------------------------
_HAS_GIT = subprocess.run(
['git', '--version'], capture_output=True).returncode == 0
def _git(*args, cwd):
return subprocess.run(['git', *args], cwd=str(cwd),
capture_output=True, text=True, check=True)
@pytest.mark.skipif(not _HAS_GIT, reason='git not available')
class TestGitPullGate:
"""A plugin installed as a git checkout updates by pulling in place, so it
never passes through `install_plugin` and was never gated.
Real git repositories rather than mocks, because the claim under test is
that `git reset --hard` puts the checkout back — a mock of git would only
prove the call was made, which is the easy half.
Only the handful of registry entries with no `plugin_path` reach this path
in practice; monorepo plugins install as archives and update through
`_reinstall_with_rollback`. It is gated anyway because the sunset rule in
`docs/plugin-development/08-shared-sports-code.md` states the core enforces
the floor "at install/update time", and a precondition that is documented
but not true is worse than one that is merely missing.
"""
@pytest.fixture
def checkout(self, tmp_path, monkeypatch):
"""An origin repo holding a plugin, and a clone of it installed as
`plugin-repos/gitplug`, with the store pointed at it."""
from src.plugin_system.store_manager import PluginStoreManager
origin = tmp_path / 'origin'
origin.mkdir()
_git('init', '--initial-branch=main', '--bare', cwd=origin)
seed = tmp_path / 'seed'
_git('clone', str(origin), str(seed), cwd=tmp_path)
_git('config', 'user.email', 'test@example.com', cwd=seed)
_git('config', 'user.name', 'Test', cwd=seed)
(seed / 'manifest.json').write_text(json.dumps({
"id": "gitplug", "name": "Git Plug", "class_name": "P",
"display_modes": ["a"], "min_ledmatrix_version": "2.0.0",
}), encoding='utf-8')
(seed / 'manager.py').write_text("class P: pass\n", encoding='utf-8')
_git('add', '-A', cwd=seed)
_git('commit', '-m', 'initial', cwd=seed)
_git('push', '-u', 'origin', 'main', cwd=seed)
plugins_dir = tmp_path / 'plugin-repos'
plugins_dir.mkdir()
work = plugins_dir / 'gitplug'
_git('clone', str(origin), str(work), cwd=tmp_path)
_git('config', 'user.email', 'test@example.com', cwd=work)
_git('config', 'user.name', 'Test', cwd=work)
mgr = PluginStoreManager(plugins_dir=str(plugins_dir))
mgr.logger = MagicMock()
# No registry entry, so update_plugin takes the plain-pull branch
# rather than the remote-mismatch or already-current shortcuts.
monkeypatch.setattr(mgr, 'fetch_registry', lambda *a, **k: None)
monkeypatch.setattr(mgr, 'get_plugin_info', lambda *a, **k: None)
monkeypatch.setattr(mgr, '_install_dependencies', lambda *a, **k: True)
return mgr, seed, work
def _push(self, seed, manifest_text):
(seed / 'manifest.json').write_text(manifest_text, encoding='utf-8')
_git('add', '-A', cwd=seed)
_git('commit', '-m', 'update', cwd=seed)
_git('push', 'origin', 'main', cwd=seed)
def _head(self, work):
return _git('rev-parse', 'HEAD', cwd=work).stdout.strip()
def test_refuses_a_pulled_commit_that_needs_a_newer_core(
self, checkout, monkeypatch):
mgr, seed, work = checkout
before = self._head(work)
self._push(seed, json.dumps({
"id": "gitplug", "name": "Git Plug", "class_name": "P",
"display_modes": ["a"], "min_ledmatrix_version": "9.9.9",
}))
import src
monkeypatch.setattr(src, '__version__', '3.2.0')
assert mgr.update_plugin('gitplug') is False
assert self._head(work) == before, (
"a refused update must leave the checkout on the commit it was "
"already running, not on the one it cannot load")
floor = json.loads((work / 'manifest.json').read_text())
assert floor['min_ledmatrix_version'] == '2.0.0', (
"the reset must restore the working tree, not just the ref")
def test_allows_a_pulled_commit_the_core_can_run(
self, checkout, monkeypatch):
"""The guard against over-refusing. A gate that refuses everything
passes the test above and breaks every update."""
mgr, seed, work = checkout
before = self._head(work)
self._push(seed, json.dumps({
"id": "gitplug", "name": "Git Plug", "class_name": "P",
"display_modes": ["a"], "min_ledmatrix_version": "3.0.0",
}))
import src
monkeypatch.setattr(src, '__version__', '3.2.0')
assert mgr.update_plugin('gitplug') is True
assert self._head(work) != before
def test_an_unreadable_manifest_after_pull_is_not_a_refusal(
self, checkout, monkeypatch):
"""Rule 1 of this file — refuse only on evidence. A manifest that
will not parse declares no floor, so it is not evidence of anything."""
mgr, seed, work = checkout
self._push(seed, '{ this is not json')
import src
monkeypatch.setattr(src, '__version__', '3.2.0')
assert mgr.update_plugin('gitplug') is True
def test_an_untrustworthy_core_does_not_block_a_2_0_0_floor_on_pull(
self, checkout, monkeypatch):
"""The same regression guard as
`test_untrustworthy_core_does_not_block_installs`, because this path
now shares that rule: a v3.1.0 release reports 1.0.0, and nearly every
published manifest floors at 2.0.0."""
mgr, seed, work = checkout
before = self._head(work)
self._push(seed, json.dumps({
"id": "gitplug", "name": "Git Plug", "class_name": "P",
"display_modes": ["a"], "min_ledmatrix_version": "2.0.0",
"description": "changed",
}))
import src
monkeypatch.setattr(src, '__version__', '1.0.0')
assert mgr.update_plugin('gitplug') is True
assert self._head(work) != before
def test_rollback_reports_the_recovery_command_when_git_fails(
self, checkout, monkeypatch):
"""If the reset itself fails the user is left on a version that cannot
load, so the log line has to carry the command that fixes it —
it is the only thing standing between them and a manual reinstall."""
mgr, seed, work = checkout
self._push(seed, json.dumps({
"id": "gitplug", "name": "Git Plug", "class_name": "P",
"display_modes": ["a"], "min_ledmatrix_version": "9.9.9",
}))
real_run = subprocess.run
def fail_reset(cmd, *a, **k):
if isinstance(cmd, list) and 'reset' in cmd:
return subprocess.CompletedProcess(cmd, 1, '', 'reset boom')
return real_run(cmd, *a, **k)
import src
monkeypatch.setattr(src, '__version__', '3.2.0')
monkeypatch.setattr(
'src.plugin_system.store_manager.subprocess.run', fail_reset)
assert mgr.update_plugin('gitplug') is False
logged = ' '.join(str(c) for c in mgr.logger.error.call_args_list)
assert 'reset --hard' in logged, logged
class TestLoaderAndStoreAgree:
"""Both read the same manifests; a disagreement means one of them is
lying to the user."""