fix: unify version comparison, refuse secret-leaking saves, reset skin strikes on card success

Fixes the three suspected bugs this PR's characterization tests pinned,
flipping those tests to assert the corrected behavior:

- plugins/store: ONE shared update comparator. New
  compatibility.is_update_available() (PEP 440 via packaging) is now used
  by both the web UI's update badge (api_v3._is_plugin_update_available
  is a thin alias) and store_manager.update_plugin's reinstall decision.
  Previously update_plugin used raw string equality: 'v1.2.0' vs '1.2.0'
  triggered a full reinstall the UI called unnecessary, and a locally-
  ahead plugin (2.0.0 installed, registry 1.9.0) was silently DOWNGRADED.
  Now equivalent spellings skip the reinstall and locally-ahead versions
  are never downgraded; unparseable versions still reconcile by
  reinstalling from the registry.

- config: save_config and save_config_atomic now refuse (ConfigError)
  when config_secrets.json exists but cannot be loaded. Both previously
  proceeded without stripping, writing the merged secrets into
  config.json in plaintext. The shared _load_secrets_for_save() helper
  raises with an actionable message instead; a missing secrets file is
  still fine (nothing to strip), and _migrate_config's catch-all keeps
  boot resilient.

- skins: render_skin_card resets _skin_failures on both success paths
  (vegas card returned, or mode renderer handled), mirroring
  _render_game. Transient card failures no longer accumulate across a
  session until they permanently disable a working skin.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh

src/base_classes/sports/core.py

@@ -383,10 +383,15 @@ class SportsCore(ABC):
             ctx = skin_runtime.build_context(self, game, size=size)
             card = skin.render_vegas_card(ctx, dict(game))
             if card is not None:
+                # A successful render clears accumulated strikes, mirroring
+                # _render_game — transient failures must not add up across
+                # the session and disable a working skin.
+                self._skin_failures = 0
                 return card
             ctx = skin_runtime.build_context(self, game, size=size)
             render = getattr(skin, f"render_{self.SKIN_MODE}")
             if render(ctx, dict(game)):
+                self._skin_failures = 0
                 return ctx.canvas
         except Exception:
             # Card failures count toward the same 3-strike session disable

src/config_manager.py

@@ -106,14 +106,9 @@ class ConfigManager:
         Returns:
             SaveResult with status and details
         """
-        # Load current secrets to preserve them
-        secrets_content = {}
-        if os.path.exists(self.secrets_path):
-            try:
-                with open(self.secrets_path, 'r') as f_secrets:
-                    secrets_content = json.load(f_secrets)
-            except Exception as e:
-                self.logger.warning(f"Could not load secrets file {self.secrets_path} during save: {e}")
+        # Load current secrets to preserve them (raises if unreadable — see
+        # _load_secrets_for_save)
+        secrets_content = self._load_secrets_for_save()
 
         # Strip secrets from main config before saving
         config_to_write = self._strip_secrets_recursive(new_config_data, secrets_content)
@@ -290,19 +285,40 @@ class ConfigManager:
                 result[key] = value
         return result
 
-    def save_config(self, new_config_data: Dict[str, Any]) -> None:
-        """Save configuration to the main JSON file, stripping out secrets."""
-        secrets_content = {}
-        if os.path.exists(self.secrets_path):
+    def _load_secrets_for_save(self) -> Dict[str, Any]:
+        """Load config_secrets.json for stripping before a save.
+
+        A missing secrets file is fine (nothing to strip). But a file that
+        EXISTS and cannot be read or parsed means stripping is impossible —
+        and the in-memory config being saved has secrets deep-merged into it,
+        so proceeding would write them into config.json in plaintext. That
+        was the historical behavior; it is now a hard refusal. The save
+        raises so the caller (and user) fixes the secrets file instead of
+        silently leaking its contents into the world-readable main config.
+        """
+        if not os.path.exists(self.secrets_path):
+            return {}
         try:
             with open(self.secrets_path, 'r') as f_secrets:
-                    secrets_content = json.load(f_secrets)
+                return json.load(f_secrets)
         except Exception as e:
-                self.logger.warning(f"Could not load secrets file {self.secrets_path} during save: {e}")
-                # Continue without stripping if secrets can't be loaded, or handle as critical error
-                # For now, we'll proceed cautiously and save the full new_config_data if secrets are unreadable
-                # to prevent accidental data loss if the secrets file is temporarily corrupt.
-                # A more robust approach might be to fail the save or use a cached version of secrets.
+            error_msg = (
+                f"Refusing to save config: secrets file {self.secrets_path} exists "
+                f"but could not be loaded ({e}). Saving without it would write "
+                f"merged secret values into config.json in plaintext. Fix or "
+                f"remove the secrets file, then retry."
+            )
+            self.logger.error(error_msg)
+            raise ConfigError(error_msg, config_path=self.secrets_path) from e
+
+    def save_config(self, new_config_data: Dict[str, Any]) -> None:
+        """Save configuration to the main JSON file, stripping out secrets.
+
+        Raises ConfigError when the secrets file exists but cannot be loaded,
+        because stripping would be impossible and secrets would leak into
+        config.json.
+        """
+        secrets_content = self._load_secrets_for_save()
 
         config_to_write = self._strip_secrets_recursive(new_config_data, secrets_content)
 

src/plugin_system/compatibility.py

@@ -180,6 +180,41 @@ def declared_min_version(manifest: Dict[str, Any]) -> Optional[str]:
     return None
 
 
+def is_update_available(installed_version: str, latest_version: str) -> bool:
+    """Return True when the registry's ``latest_version`` is strictly newer
+    than the installed version.
+
+    THE shared comparator for "should this plugin be updated?" — used by both
+    the web UI's update badge (`api_v3._is_plugin_update_available`) and the
+    store's `update_plugin` reinstall decision, so the two can never disagree.
+
+    Uses PEP 440-aware comparison (``packaging``), which also normalizes
+    equivalent spellings: ``v1.2.0`` == ``1.2.0`` and ``1.2`` == ``1.2.0``, so
+    cosmetic differences never trigger a reinstall — and a locally modified
+    plugin whose version is *ahead* of the registry is never "updated"
+    (downgraded). If either version string can't be parsed the mismatch is
+    surfaced (True) so the user can reconcile, rather than silently hiding a
+    potential update.
+    """
+    if not installed_version or not latest_version:
+        return False
+    if installed_version == latest_version:
+        return False
+    try:
+        from packaging.version import parse as _parse_version, InvalidVersion
+    except ImportError:
+        # packaging is a core dependency, but if it's somehow unavailable we
+        # can't compare semantically — surface the mismatch we already know
+        # exists (the two strings differ).
+        return True
+    try:
+        return _parse_version(latest_version) > _parse_version(installed_version)
+    except InvalidVersion:
+        # Unparseable version string: we can't tell direction, so surface the
+        # mismatch rather than silently hiding a potential update.
+        return True
+
+
 def check(manifest: Dict[str, Any], core_version: str) -> Tuple[bool, Optional[str]]:
     """Return ``(compatible, reason)``.
 

src/plugin_system/store_manager.py

@@ -2969,7 +2969,10 @@ class PluginStoreManager:
             remote_branch = plugin_info_remote.get('branch') or plugin_info_remote.get('default_branch')
 
             # Compare local manifest version against registry latest_version
-            # to avoid unnecessary reinstalls for monorepo plugins
+            # to avoid unnecessary reinstalls for monorepo plugins. Uses the
+            # same semantic comparator as the web UI's update badge, so
+            # equivalent spellings ("v1.2.0" vs "1.2.0") never trigger a
+            # reinstall and a locally-ahead version is never downgraded.
             try:
                 local_manifest_path = plugin_path / "manifest.json"
                 if local_manifest_path.exists():
@@ -2977,8 +2980,12 @@ class PluginStoreManager:
                         local_manifest = json.load(f)
                     local_version = local_manifest.get('version', '')
                     remote_version = plugin_info_remote.get('latest_version', '')
-                    if local_version and remote_version and local_version == remote_version:
-                        self.logger.info(f"Plugin {plugin_id} already at latest version {local_version}")
+                    from src.plugin_system.compatibility import is_update_available
+                    if (local_version and remote_version
+                            and not is_update_available(local_version, remote_version)):
+                        self.logger.info(
+                            f"Plugin {plugin_id} already at latest version "
+                            f"(installed {local_version}, registry {remote_version})")
                         return True
             except Exception as e:
                 self.logger.debug(f"Could not compare versions for {plugin_id}: {e}")

test/test_config_manager_secrets.py

@@ -4,8 +4,9 @@ Tests for the ConfigManager secrets round-trip and the load_config fast path.
 The contract under test: config_secrets.json values are deep-merged INTO the
 in-memory config at load time, and stripped back OUT before anything is
 written to config.json — so secrets live in exactly one file on disk. This
-suite pins that round-trip plus its known sharp edges (some marked as
-SUSPECTED BUG and characterized rather than fixed).
+suite pins that round-trip plus its sharp edges, including the guard that a
+save REFUSES (ConfigError) when the secrets file exists but can't be loaded,
+rather than leaking merged secrets into config.json in plaintext.
 
 Complements test_config_manager.py, which covers loading/migration/validation.
 """
@@ -16,6 +17,7 @@ import os
 import pytest
 
 from src.config_manager import ConfigManager
+from src.exceptions import ConfigError
 
 
 def make_manager(tmp_path, config=None, secrets=None):
@@ -99,13 +101,11 @@ class TestSaveStripsSecrets:
         on_disk = json.loads((tmp_path / "config.json").read_text())
         assert on_disk == {"timezone": "UTC"}
 
-    def test_corrupt_secrets_file_writes_secrets_to_config_json(self, tmp_path):
-        # SUSPECTED BUG (characterized, not fixed): when the secrets file is
-        # corrupt (or otherwise unloadable) at save time, save_config proceeds without
-        # stripping — writing the merged secrets into config.json in
-        # plaintext. The code comments acknowledge the tradeoff (it prevents
-        # data loss); this test pins the behavior so any future change to it
-        # is deliberate.
+    def test_corrupt_secrets_file_refuses_save_no_plaintext_leak(self, tmp_path):
+        # Regression guard: when the secrets file exists but is corrupt at
+        # save time, stripping is impossible — the save must raise instead of
+        # writing the merged secrets into config.json in plaintext (the
+        # historical behavior).
         manager = make_manager(
             tmp_path,
             config={"weather": {"city": "Austin"}},
@@ -114,10 +114,28 @@ class TestSaveStripsSecrets:
         loaded = manager.load_config()
         (tmp_path / "config_secrets.json").write_text("{corrupt")
 
+        with pytest.raises(ConfigError):
             manager.save_config(loaded)
 
+        # On-disk config untouched: no secret leaked.
         on_disk = json.loads((tmp_path / "config.json").read_text())
-        assert on_disk["weather"].get("api_key") == "s3cret"  # leaked
+        assert "api_key" not in on_disk.get("weather", {})
+
+    def test_corrupt_secrets_file_refuses_atomic_save_too(self, tmp_path):
+        # Same refusal on the atomic save path, which shared the leak.
+        manager = make_manager(
+            tmp_path,
+            config={"weather": {"city": "Austin"}},
+            secrets={"weather": {"api_key": "s3cret"}},
+        )
+        loaded = manager.load_config()
+        (tmp_path / "config_secrets.json").write_text("{corrupt")
+
+        with pytest.raises(ConfigError):
+            manager.save_config_atomic(loaded)
+
+        on_disk = json.loads((tmp_path / "config.json").read_text())
+        assert "api_key" not in on_disk.get("weather", {})
 
 
 class TestLoadFastPath:

test/test_skin_system.py

@@ -455,10 +455,8 @@ class TestExampleSkin:
 class TestRenderSkinCard:
     """render_skin_card (vegas cards) shares _render_game's 3-strike counter.
 
-    The asymmetry pinned here: _render_game resets the counter on success,
-    render_skin_card does NOT — card successes never clear strikes, so
-    failures accumulated across card renders (however far apart) still
-    disable the skin for the session.
+    Both paths reset the counter on success — transient failures must not
+    accumulate across a session and disable a working skin.
     """
 
     def _probe(self, skin):
@@ -518,11 +516,10 @@ class TestRenderSkinCard:
         assert probe.builtin_calls == 1
         assert BrokenCardSkin.calls == 3  # not consulted again
 
-    def test_card_success_does_not_reset_strikes(self):
-        """Characterized asymmetry: unlike _render_game (which resets the
-        counter on success, core.py _render_game), a successful card render
-        leaves accumulated strikes in place — 2 failures + N successes + 1
-        failure still disables the skin."""
+    def test_card_success_resets_strikes(self):
+        """A successful card render clears accumulated strikes (mirroring
+        _render_game) — 2 failures + a success + 1 failure leaves the skin
+        enabled with a single strike, instead of disabling it."""
         card_img = Image.new("RGB", (96, 32), (0, 0, 255))
 
         class FlakyCardSkin(ScoreboardSkin):
@@ -540,17 +537,30 @@ class TestRenderSkinCard:
         assert probe._skin_failures == 2
 
         FlakyCardSkin.fail = False
-        for _ in range(10):
         assert probe.render_skin_card({}, (96, 32)) is card_img
-        assert probe._skin_failures == 2  # successes did NOT clear strikes
+        assert probe._skin_failures == 0  # success cleared the strikes
 
         FlakyCardSkin.fail = True
         probe.render_skin_card({}, (96, 32))
-        assert probe._skin_failures == 3
-        assert probe.render_skin_card({}, (96, 32)) is None  # disabled
+        assert probe._skin_failures == 1  # counting from the reset state
+        FlakyCardSkin.fail = False
+        assert probe.render_skin_card({}, (96, 32)) is card_img  # still enabled
 
-    def test_render_game_success_does_reset_strikes(self):
-        """The other half of the asymmetry, for contrast with the above."""
+    def test_card_success_via_mode_renderer_also_resets_strikes(self):
+        """The fallthrough path (render_vegas_card None -> mode renderer
+        True) resets the counter as well."""
+        class ModeOnlySkin(ScoreboardSkin):
+            def render_live(self, ctx, game):
+                ctx.draw.rectangle([0, 0, 5, 5], fill=(255, 0, 0))
+                return True
+
+        probe = self._probe(ModeOnlySkin({}, {}))
+        probe._skin_failures = 2
+        assert probe.render_skin_card({}, (96, 32)) is not None
+        assert probe._skin_failures == 0
+
+    def test_render_game_success_also_resets_strikes(self):
+        """Same reset contract on the display path, for symmetry."""
         class GoodSkin(ScoreboardSkin):
             def render_live(self, ctx, game):
                 return True

test/test_version_comparison_consistency.py

@@ -1,25 +1,17 @@
 """
-Drift guard: the repo has FOUR version-comparison implementations, and they
-do not agree. This file pins each one's answer on the same inputs so any
-future change to one of them (or a fifth copy appearing) surfaces here.
+Drift guard for version comparison.
 
-The four:
-1. src/plugin_system/compatibility.py  — parse_semver / tuple comparison
-   (install gate).
-2. web_interface/blueprints/api_v3.py  — _is_plugin_update_available, uses
-   packaging.version (update badge in the UI).
-3. src/plugin_system/store_manager.py  — update_plugin's raw STRING EQUALITY
-   for monorepo plugins ("local_version == remote_version").
-4. src/skin_system/skin_runtime.py     — _major, int(major) gate for the
-   skin API.
+There is now ONE shared "should this plugin update?" comparator —
+`src.plugin_system.compatibility.is_update_available` — used by both the web
+UI's update badge (`api_v3._is_plugin_update_available`) and the store's
+`update_plugin` reinstall decision, so the badge and the actual reinstall
+can never disagree. (Historically the store used raw string equality, which
+reinstalled over cosmetic differences like "v1.2.0" vs "1.2.0" and even
+DOWNGRADED locally-ahead plugins; this file's tests killed that.)
 
-SUSPECTED BUG (characterized here, not fixed): #3 disagrees with #2. For
-"v1.2.0" vs "1.2.0" the UI says "no update available" while update_plugin
-performs a full reinstall; for a locally-ahead plugin ("2.0.0" installed,
-registry "1.9.0") the UI says no update but update_plugin DOWNGRADES via
-reinstall. Unifying on one comparator is tracked follow-up work; when that
-lands, the expectations in TestStoreManagerStringEquality flip and this
-file is the reminder to update them deliberately.
+Two other version parsers legitimately remain and are pinned here so they
+don't drift: `compatibility.parse_semver` (the install-compatibility gate,
+range-spec oriented) and `skin_runtime._major` (skin API major gate).
 """
 
 import json
@@ -28,44 +20,40 @@ from unittest.mock import patch
 import pytest
 from packaging.version import parse as pkg_parse
 
-from src.plugin_system.compatibility import parse_semver
+from src.plugin_system.compatibility import is_update_available, parse_semver
 from src.skin_system.skin_runtime import _major
 from src.plugin_system.store_manager import PluginStoreManager
 from web_interface.blueprints.api_v3 import _is_plugin_update_available
 
 
-# (installed, registry) pairs and what each comparator concludes.
+# (installed, registry) -> update available?
 CASES = [
-    # pair                  parse_semver equal?   api_v3 update?   store equal-string?
-    (("1.2.0", "1.2.0"),    True,                 False,           True),
-    (("v1.2.0", "1.2.0"),   True,                 False,           False),
-    (("1.2", "1.2.0"),      True,                 False,           False),
-    (("1.2.0", "1.2.0-rc1"), True,                False,           False),
-    (("1.2.0", "1.3.0"),    False,                True,            False),
-    (("2.0.0", "1.9.0"),    False,                False,           False),
+    (("1.2.0", "1.2.0"), False),   # identical
+    (("v1.2.0", "1.2.0"), False),  # cosmetic v-prefix, semantically equal
+    (("1.2", "1.2.0"), False),     # short form, semantically equal
+    (("1.2.0", "1.2.0-rc1"), False),  # rc of same release is not newer
+    (("1.2.0", "1.3.0"), True),    # registry genuinely newer
+    (("2.0.0", "1.9.0"), False),   # locally ahead — never downgrade
+    (("abc.def", "1.0.0"), True),  # unparseable — surface the mismatch
+    (("", "1.0.0"), False),        # missing either side — nothing to do
+    (("1.0.0", ""), False),
 ]
 
 
-class TestComparatorMatrix:
-    @pytest.mark.parametrize("pair,semver_equal,api_update,store_equal", CASES)
-    def test_parse_semver_equality(self, pair, semver_equal, api_update, store_equal):
-        a, b = pair
-        assert (parse_semver(a) == parse_semver(b)) is semver_equal
-
-    @pytest.mark.parametrize("pair,semver_equal,api_update,store_equal", CASES)
-    def test_api_v3_update_available(self, pair, semver_equal, api_update, store_equal):
+class TestSharedComparator:
+    @pytest.mark.parametrize("pair,expected", CASES)
+    def test_is_update_available(self, pair, expected):
         installed, latest = pair
-        assert _is_plugin_update_available(installed, latest) is api_update
+        assert is_update_available(installed, latest) is expected
 
-    @pytest.mark.parametrize("pair,semver_equal,api_update,store_equal", CASES)
-    def test_store_manager_string_equality(self, pair, semver_equal, api_update, store_equal):
-        # The literal comparison update_plugin performs at its
-        # "already at latest version" check.
-        a, b = pair
-        assert (a == b) is store_equal
+    @pytest.mark.parametrize("pair,expected", CASES)
+    def test_api_v3_helper_agrees(self, pair, expected):
+        # The UI badge helper must be a pure alias of the shared comparator.
+        installed, latest = pair
+        assert _is_plugin_update_available(installed, latest) is expected
 
 
-class TestStoreManagerStringEquality:
+class TestStoreManagerUsesSharedComparator:
     """Drive update_plugin's real code path to its version check."""
 
     def _store(self, tmp_path, local_version, registry_version):
@@ -98,18 +86,30 @@ class TestStoreManagerStringEquality:
         assert result is True
         reinstall.assert_not_called()
 
-    def test_v_prefix_triggers_reinstall_despite_semantic_equality(self, tmp_path):
-        # SUSPECTED BUG: packaging (and api_v3) treat these as equal; the
-        # string comparison does not, so the user gets a full reinstall.
+    def test_v_prefix_equivalent_skips_reinstall(self, tmp_path):
+        # "v1.2.0" == "1.2.0" semantically — no pointless reinstall.
         store, info = self._store(tmp_path, "v1.2.0", "1.2.0")
         result, reinstall = self._run_update(store, info)
+        assert result is True
+        reinstall.assert_not_called()
+
+    def test_locally_ahead_version_is_never_downgraded(self, tmp_path):
+        # A plugin ahead of the registry (local dev build) must not be
+        # "updated" — that would be a downgrade.
+        store, info = self._store(tmp_path, "2.0.0", "1.9.0")
+        result, reinstall = self._run_update(store, info)
+        assert result is True
+        reinstall.assert_not_called()
+
+    def test_registry_newer_triggers_reinstall(self, tmp_path):
+        store, info = self._store(tmp_path, "1.2.0", "1.3.0")
+        result, reinstall = self._run_update(store, info)
         reinstall.assert_called_once()
         assert result is True
 
-    def test_locally_ahead_version_triggers_downgrade_reinstall(self, tmp_path):
-        # SUSPECTED BUG: a plugin ahead of the registry (local dev build) is
-        # "updated" — i.e. downgraded — because inequality is the only test.
-        store, info = self._store(tmp_path, "2.0.0", "1.9.0")
+    def test_unparseable_version_surfaces_via_reinstall(self, tmp_path):
+        # Direction unknowable → reconcile by reinstalling from the registry.
+        store, info = self._store(tmp_path, "abc.def", "1.0.0")
         result, reinstall = self._run_update(store, info)
         reinstall.assert_called_once()
         assert result is True

web_interface/blueprints/api_v3.py

@@ -124,28 +124,13 @@ def _is_plugin_update_available(installed_version: str, latest_version: str) ->
     """Return True when the registry's ``latest_version`` is strictly newer
     than the installed version.
 
-    Uses PEP 440 / semver-aware comparison so a locally modified plugin whose
-    version is *ahead* of the published registry is not flagged as needing an
-    update. If either version string can't be parsed, falls back to a plain
-    inequality check (any difference is surfaced so the user can reconcile).
+    Thin alias for the shared comparator in
+    `src.plugin_system.compatibility.is_update_available` — the store's
+    `update_plugin` uses the same function, so the UI badge and the actual
+    reinstall decision can never disagree.
     """
-    if not installed_version or not latest_version:
-        return False
-    if installed_version == latest_version:
-        return False
-    try:
-        from packaging.version import parse as _parse_version, InvalidVersion
-    except ImportError:
-        # packaging is a core dependency, but if it's somehow unavailable we
-        # can't compare semantically — surface the mismatch we already know
-        # exists (the two strings differ).
-        return True
-    try:
-        return _parse_version(latest_version) > _parse_version(installed_version)
-    except InvalidVersion:
-        # Unparseable version string: we can't tell direction, so surface the
-        # mismatch rather than silently hiding a potential update.
-        return True
+    from src.plugin_system.compatibility import is_update_available
+    return is_update_available(installed_version, latest_version)
 
 def _ensure_cache_manager():
     """Ensure cache manager is initialized."""