From 217aac780b709f9e408d0c4c7a71504ac84a90c5 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Mon, 31 Aug 2026 12:01:50 -0400 Subject: [PATCH 1/2] fix(store): gate the git-pull update path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --- docs/SPORTS_UNIFICATION.md | 15 +- src/plugin_system/store_manager.py | 85 +++++++++++ test/test_plugin_compatibility_gate.py | 187 ++++++++++++++++++++++++- 3 files changed, 283 insertions(+), 4 deletions(-) diff --git a/docs/SPORTS_UNIFICATION.md b/docs/SPORTS_UNIFICATION.md index 4e731278..6e9b5486 100644 --- a/docs/SPORTS_UNIFICATION.md +++ b/docs/SPORTS_UNIFICATION.md @@ -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 diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 5cf31cf8..d9a46fe4 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -2583,6 +2583,83 @@ def uninstall_plugin(self, plugin_id: str) -> bool: 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 @@ def update_plugin(self, plugin_id: str) -> bool: 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 diff --git a/test/test_plugin_compatibility_gate.py b/test/test_plugin_compatibility_gate.py index 26834ffa..3ba15acf 100644 --- a/test/test_plugin_compatibility_gate.py +++ b/test/test_plugin_compatibility_gate.py @@ -19,6 +19,7 @@ """ 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 @@ def test_untrustworthy_core_does_not_block_installs(self, store, monkeypatch): 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.""" From 45d912142cd4795d8248e4d3fa2d7fe0cfcae508 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Mon, 31 Aug 2026 13:49:58 -0400 Subject: [PATCH 2/2] fix(store): do not pull what the gate cannot un-pull Review of the gate found a data-loss path it had introduced, plus two smaller scope errors. All three from CodeRabbit on #508. **The stash failure was load-bearing and was not treated as one.** update_plugin stashes local changes before pulling; when that stash failed or timed out it logged a warning and pulled anyway. That was harmless while nothing ever undid a pull. It is not harmless now: the gate's rollback is `git reset --hard`, which discards uncommitted tracked edits -- exactly the edits the stash existed to protect. A pull does not refuse on a dirty tree as long as the incoming commit touches other files, so the sequence completed silently: pull succeeds, gate refuses, reset takes the user's work with it. update_plugin now returns before pulling unless the tree was already clean or was successfully stashed. Refusing costs an update in a case that had already gone wrong; the alternative costs data. That also makes `--hard` safe by construction in _gate_pulled_commit, and its comment now says so rather than observing it in passing. Pinned by test_a_failed_stash_stops_the_update_before_pulling, which writes a local edit, forces the stash to fail, and asserts both that HEAD did not move and that the edit is still on disk. Verified it bites: with the new guard removed the file comes back as `class P: pass`, the edit gone. **_HAS_GIT could take the module down instead of skipping it.** With no git on PATH, subprocess.run raises FileNotFoundError, and this runs at import time -- before skipif can act, so the whole file errors rather than skipping. Now catches OSError. **The doc overclaimed the gate's reach.** It said the floor is enforced on "every route that installs or updates" while the same passage notes install_from_url is ungated. Both spots now scope the claim to registry-managed installs and the two supported update paths, and name the sideload exception. Full suite 3720 passed, 6 skipped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --- docs/SPORTS_UNIFICATION.md | 5 +-- src/plugin_system/store_manager.py | 33 +++++++++++++++-- test/test_plugin_compatibility_gate.py | 50 ++++++++++++++++++++++++-- 3 files changed, 82 insertions(+), 6 deletions(-) diff --git a/docs/SPORTS_UNIFICATION.md b/docs/SPORTS_UNIFICATION.md index 6e9b5486..4ef30e94 100644 --- a/docs/SPORTS_UNIFICATION.md +++ b/docs/SPORTS_UNIFICATION.md @@ -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*. 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. | +| 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 registry-managed install and on both supported update paths (sideloading via `install_from_url` is not gated), 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 @@ -235,7 +235,8 @@ a floor can be trusted against, and today it is not: `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. + covers every registry-managed install and, through `_reinstall_with_rollback`, + the update path that re-downloads. `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 diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index d9a46fe4..623270e3 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -2640,8 +2640,10 @@ def _gate_pulled_commit(self, plugin_id: str, plugin_path: Path, "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 + # Safe by construction: update_plugin returns before pulling unless the + # tree was clean or successfully stashed, so there are no uncommitted + # tracked edits for --hard to discard. The stash is 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], @@ -2937,6 +2939,8 @@ def update_plugin(self, plugin_id: str) -> bool: status_result = type('obj', (object,), {'stdout': '', 'stderr': 'Status check timed out'})() stash_info = "" + # Whether the pull can be undone without destroying work. + tree_is_recoverable = not has_changes if has_changes: self.logger.info(f"Stashing local changes in {plugin_id} before update") try: @@ -2950,12 +2954,37 @@ def update_plugin(self, plugin_id: str) -> bool: ) if stash_result.returncode == 0: stash_info = " (local changes were stashed)" + tree_is_recoverable = True self.logger.info(f"Stashed local changes (including untracked files) for {plugin_id}") else: self.logger.warning(f"Failed to stash local changes for {plugin_id}: {stash_result.stderr}") except subprocess.TimeoutExpired: self.logger.warning(f"Stash operation timed out for {plugin_id}, proceeding with pull") + # Do not pull what cannot be un-pulled. + # + # The compatibility gate below can refuse the commit this + # pull brings down, and its only way back is `git reset + # --hard`, which discards uncommitted tracked edits. Those + # edits are exactly what the stash above exists to protect, + # so a stash that failed or timed out leaves the rollback + # unable to run without destroying them. + # + # A pull does not necessarily refuse on a dirty tree -- git + # merges happily as long as the incoming commit touches + # different files -- so without this the update would + # succeed, the gate would refuse, and the reset would take + # the user's work with it. Refusing here costs an update in + # a case that already went wrong; the alternative costs + # data. + if not tree_is_recoverable: + self.logger.error( + "Refusing to update %s: it has local changes that could " + "not be stashed, and an incompatible update could then " + "only be rolled back by discarding them. Commit or stash " + "them by hand, then update.", plugin_id) + return False + # Pull from the determined remote branch self.logger.info(f"Pulling from origin/{remote_pull_branch} for {plugin_id}...") pull_result = subprocess.run( diff --git a/test/test_plugin_compatibility_gate.py b/test/test_plugin_compatibility_gate.py index 3ba15acf..91a84b8e 100644 --- a/test/test_plugin_compatibility_gate.py +++ b/test/test_plugin_compatibility_gate.py @@ -221,8 +221,18 @@ def test_untrustworthy_core_does_not_block_installs(self, store, monkeypatch): # 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_available() -> bool: + # OSError, not just a non-zero exit: with no git on PATH subprocess raises + # FileNotFoundError, and this runs at import time -- before skipif can act, + # so the whole module would error out instead of skipping. + try: + return subprocess.run( + ['git', '--version'], capture_output=True).returncode == 0 + except OSError: + return False + + +_HAS_GIT = _git_available() def _git(*args, cwd): @@ -361,6 +371,42 @@ def test_an_untrustworthy_core_does_not_block_a_2_0_0_floor_on_pull( assert mgr.update_plugin('gitplug') is True assert self._head(work) != before + def test_a_failed_stash_stops_the_update_before_pulling( + self, checkout, monkeypatch): + """Uncommitted work is not collateral for the gate. + + The gate's only rollback is `git reset --hard`, which discards + uncommitted tracked edits. update_plugin stashes them first -- but when + that stash fails it used to pull anyway, and a pull touching different + files succeeds on a dirty tree. Refuse, refuse the rollback's rollback, + and the user's edits go with it. + """ + mgr, seed, work = checkout + before = self._head(work) + (work / 'manager.py').write_text( + "class P:\n MINE = 'do not lose this'\n", encoding='utf-8') + 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_stash(cmd, *a, **k): + if isinstance(cmd, list) and 'stash' in cmd: + return subprocess.CompletedProcess(cmd, 1, '', 'stash 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_stash) + assert mgr.update_plugin('gitplug') is False + + assert self._head(work) == before, "must not pull what it cannot undo" + assert 'do not lose this' in (work / 'manager.py').read_text(), ( + "the local edit the stash failed to save must still be there") + 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