mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-18 04:49:13 +00:00
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9
This commit is contained in:
co-authored by
Claude Opus 5
parent
217aac780b
commit
45d912142c
@@ -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 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. |
|
| 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. |
|
| 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
|
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
|
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.
|
`latest_version` and nothing else.
|
||||||
|
|
||||||
*Fixed, in two parts.* `install_plugin` gained the gate in #431/#433, which
|
*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
|
`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
|
stayed ungated until `_gate_pulled_commit` closed it — checked after the pull
|
||||||
(the registry carries no floor field, so the incoming floor is unknowable
|
(the registry carries no floor field, so the incoming floor is unknowable
|
||||||
|
|||||||
@@ -2640,8 +2640,10 @@ class PluginStoreManager:
|
|||||||
"reinstall it from the plugin store.", plugin_id)
|
"reinstall it from the plugin store.", plugin_id)
|
||||||
return False
|
return False
|
||||||
|
|
||||||
# Any local changes were stashed before the pull and are not popped on
|
# Safe by construction: update_plugin returns before pulling unless the
|
||||||
# the success path either, so the reset leaves the working tree exactly
|
# 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".
|
# where a successful pull would have. Say "commit", not "changes".
|
||||||
reset = subprocess.run(
|
reset = subprocess.run(
|
||||||
['git', '-C', str(plugin_path), 'reset', '--hard', previous_sha],
|
['git', '-C', str(plugin_path), 'reset', '--hard', previous_sha],
|
||||||
@@ -2937,6 +2939,8 @@ class PluginStoreManager:
|
|||||||
status_result = type('obj', (object,), {'stdout': '', 'stderr': 'Status check timed out'})()
|
status_result = type('obj', (object,), {'stdout': '', 'stderr': 'Status check timed out'})()
|
||||||
|
|
||||||
stash_info = ""
|
stash_info = ""
|
||||||
|
# Whether the pull can be undone without destroying work.
|
||||||
|
tree_is_recoverable = not has_changes
|
||||||
if has_changes:
|
if has_changes:
|
||||||
self.logger.info(f"Stashing local changes in {plugin_id} before update")
|
self.logger.info(f"Stashing local changes in {plugin_id} before update")
|
||||||
try:
|
try:
|
||||||
@@ -2950,12 +2954,37 @@ class PluginStoreManager:
|
|||||||
)
|
)
|
||||||
if stash_result.returncode == 0:
|
if stash_result.returncode == 0:
|
||||||
stash_info = " (local changes were stashed)"
|
stash_info = " (local changes were stashed)"
|
||||||
|
tree_is_recoverable = True
|
||||||
self.logger.info(f"Stashed local changes (including untracked files) for {plugin_id}")
|
self.logger.info(f"Stashed local changes (including untracked files) for {plugin_id}")
|
||||||
else:
|
else:
|
||||||
self.logger.warning(f"Failed to stash local changes for {plugin_id}: {stash_result.stderr}")
|
self.logger.warning(f"Failed to stash local changes for {plugin_id}: {stash_result.stderr}")
|
||||||
except subprocess.TimeoutExpired:
|
except subprocess.TimeoutExpired:
|
||||||
self.logger.warning(f"Stash operation timed out for {plugin_id}, proceeding with pull")
|
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
|
# Pull from the determined remote branch
|
||||||
self.logger.info(f"Pulling from origin/{remote_pull_branch} for {plugin_id}...")
|
self.logger.info(f"Pulling from origin/{remote_pull_branch} for {plugin_id}...")
|
||||||
pull_result = subprocess.run(
|
pull_result = subprocess.run(
|
||||||
|
|||||||
@@ -221,8 +221,18 @@ class TestInstallGate:
|
|||||||
# The git-pull update path — the one route that does not re-download
|
# The git-pull update path — the one route that does not re-download
|
||||||
# --------------------------------------------------------------------------
|
# --------------------------------------------------------------------------
|
||||||
|
|
||||||
_HAS_GIT = subprocess.run(
|
def _git_available() -> bool:
|
||||||
['git', '--version'], capture_output=True).returncode == 0
|
# 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):
|
def _git(*args, cwd):
|
||||||
@@ -361,6 +371,42 @@ class TestGitPullGate:
|
|||||||
assert mgr.update_plugin('gitplug') is True
|
assert mgr.update_plugin('gitplug') is True
|
||||||
assert self._head(work) != before
|
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(
|
def test_rollback_reports_the_recovery_command_when_git_fails(
|
||||||
self, checkout, monkeypatch):
|
self, checkout, monkeypatch):
|
||||||
"""If the reset itself fails the user is left on a version that cannot
|
"""If the reset itself fails the user is left on a version that cannot
|
||||||
|
|||||||
Reference in New Issue
Block a user