Test suite overhaul + fixes for the three bugs it uncovered (#441)

* ci: run the whole test tree and make the plugin-safety job assert something real The unit-tests CI job ran an explicit 24-file allowlist that had rotted: 63 of 90 test files (display, vegas, store manager, web API, web_interface) never ran on a PR. The job now runs all of test/ (minus test/plugins, which the plugin-safety job owns) so new test files are enrolled by default and any exclusion needs a visible, commented --ignore. The plugin-safety job was a green no-op: plugins/ is empty in CI, so every test skipped with 'Manifest not found'. It now renders a bundled deterministic fixture plugin (test/fixtures/plugins/ci-fixture-plugin, golden images included for all 8 default sizes) via LEDMATRIX_PLUGINS_DIR, and sets LEDMATRIX_REQUIRE_PLUGINS=1 so discovering zero plugins fails loudly instead of skipping green. The per-plugin suites document that they target dev machines with real plugins installed. Coverage is now measured and enforced in exactly one place — the CI unit-tests step (--cov=src --cov=web_interface --cov-fail-under=45, from a measured 47% baseline). pytest.ini previously declared --cov-fail-under=30 but CI always passed --no-cov, so the gate had never run anywhere; local pytest is now coverage-free and fast. Enabling the 63 unenrolled files surfaced three cases of test rot, fixed here: test_display_controller_vegas_tick.py could not collect without the hardware rgbmatrix module (now uses the emulator convention), the state-reconciliation unrecoverable-cache tests broke when production added the is_plugin_uninstalled tombstone check (bare Mock returned truthy), and test_get_system_status assumed the optional psutil dependency (now installed via requirements-test.txt and guarded by importorskip). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test: replace can't-fail tests with real assertions test_font_manager.py was 5 of 6 tests shaped as 'try: call(); assert True / except: assert True' — running in CI while unable to fail on any regression. Rewritten against the real FontManager API and the bundled assets/fonts: returned font types, cache-hit identity, distinct entries per size, default-font fallback for unknown families and corrupt files (recorded in failed_loads), BDF native-size reading, text measurement, and cache lifecycle. test_display_manager.py's test_draw_text ended in 'assert True'; it now renders onto a known-black canvas and asserts pixels were actually lit — which required un-breaking the fixture's freetype MagicMock so draw_text's isinstance check doesn't silently swallow the draw. test_display_controller.py carried a permanently-skipped test whose skip reason already declared it redundant; deleted. Both display test files now set EMULATOR=true before importing display_manager (the same convention as test_display_dirty_tracking.py) so they collect standalone instead of depending on which test module imports display_manager first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test: cover the untested fragile logic (compatibility gate, secrets, config merges, durations, skin cards) New unit tests for pure or filesystem-only logic that previously had zero direct coverage: - test_compatibility.py: the semver install gate (parse_semver suffix handling, every range operator, TRUSTWORTHY_FLOOR behavior for cores reporting untrustworthy versions, 'more restrictive wins', and the malformed-manifest shapes that used to raise). - test/web_interface/test_secret_helpers.py: the canonical x-secret helpers — find/separate/mask/remove, array-item secrets, no input mutation, and a separate->recombine round-trip. - test/web_interface/test_api_v3_helpers.py: the module-level helpers behind the plugin config save endpoint (_is_plugin_update_available, _coerce_to_bool including the int==1 quirk, deep_merge including its shared-subtree shallowness, _parse_form_value, dotted-key-aware _get_schema_property/_set_nested_value). - test_base_plugin_duration.py: get_display_duration's full coercion ladder (instance attr -> config -> 15.0), including the bool-is-int quirk where display_duration=True means one second. - test_config_manager_secrets.py: the secrets round-trip — deep-merge on load, strip on save, group pruning, the load fast path — and two characterized sharp edges marked SUSPECTED BUG: an unreadable secrets file at save time writes secrets into config.json in plaintext, and a same-mtime-same-size content swap is served stale. - test_schema_manager_merge.py: merge_with_defaults branch behavior (None replacement vs falsey preservation, dict-vs-scalar mismatches, arrays replaced wholesale, defaults never mutated). - test_skin_system.py (extended): render_skin_card shares _render_game's 3-strike counter but never resets it on success — the asymmetry is pinned in both directions, along with card fallthrough and the disable interaction between the two paths. Suspected bugs are characterized, not fixed — each carries a comment so a future behavior change is deliberate rather than accidental. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test: add drift guards for cross-file contracts Three guard suites that pin contracts spanning multiple files, where one side changing unilaterally breaks the other silently: - test_version_comparison_consistency.py: the repo's four version comparators (compatibility.parse_semver, api_v3's packaging-based _is_plugin_update_available, store_manager update_plugin's raw string equality, skin_runtime._major) answer differently on the same inputs. A table pins each one's verdict; update_plugin is driven through its real code path to show the SUSPECTED BUGs: 'v1.2.0' vs '1.2.0' triggers a full reinstall the UI calls unnecessary, and a locally-ahead plugin gets downgraded. A pairwise-ordering check keeps parse_semver agreeing with packaging on plain X.Y.Z. - test/web_interface/test_secret_separation_parity.py: api_v3.py carries three inline copies of find_secret_fields/separate_secrets that lack the canonical module's array-item support. The copy count is asserted exact (it may only go down; new copies must import src/web_interface/secret_helpers), the missing-array-support gap is asserted so it can't grow silently, and the canonical behavior that migration will adopt is documented executably. - test_discovery_path_contract.py: the three 'where is plugin X' resolvers (PluginManager discovery, StoreManager._find_plugin_path, SchemaManager.get_schema_path) agree on the configured directory, and their divergent fallback chains are characterized. Also pins the .standalone-backup- naming contract shared by store rollback and discovery, and _resolve_skin_target's path-traversal rejection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test: address review feedback — fixture lifecycle, test names, ClassVar - ci-fixture-plugin: call display_manager.clear() before rendering (per plugin guidelines — the fixture should model a well-behaved plugin), add a class docstring, and document why Pillow is deliberately not pinned in its requirements.txt (core dependency; harness installs nothing). - Rename two tests whose names contradicted their assertions: test_unparseable_core_version_is_compatible -> test_unparseable_core_with_high_floor_is_blocked, and test_unreadable_secrets_file... -> test_corrupt_secrets_file... - Annotate TestGetSchemaProperty.SCHEMA as ClassVar (RUF012). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * ci: allow manual test.yml runs via workflow_dispatch Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * 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 * fix: harden shared comparator edges from review - is_update_available: reject truthy non-string versions (a malformed manifest can carry a number; packaging raises TypeError on those) by surfacing the mismatch instead of raising. - store_manager.update_plugin: drop the truthiness gate around the comparator so a missing version on either side follows the shared 'no update' verdict, keeping the store consistent with the UI badge; a missing manifest still uses the reinstall recovery path. - config_manager._load_secrets_for_save: catch only expected read/parse failures (OSError/ValueError/RecursionError) so implementation bugs propagate as themselves, and log with traceback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh --------- Co-authored-by: Claude <noreply@anthropic.com>
2026-08-08 12:18:06 +00:00 · 2026-08-07 10:17:30 -04:00
parent d9683e28be
commit d6c5f97c13
43 changed files with 2147 additions and 184 deletions
@@ -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
@@ -106,18 +106,13 @@ 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)
-        
+
        # Use atomic manager to save
        atomic_mgr = self._get_atomic_manager()
        result = atomic_mgr.save_config_atomic(
@@ -290,19 +285,43 @@ class ConfigManager:
                result[key] = value
        return result

+    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:
+                return json.load(f_secrets)
+        # Only the expected read/parse failures — an unexpected implementation
+        # error should propagate as itself, not masquerade as a secrets-file
+        # problem. (JSONDecodeError and UnicodeDecodeError are ValueErrors.)
+        except (OSError, ValueError, RecursionError) as e:
+            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("[Config] %s", error_msg, exc_info=True)
+            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."""
-        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}")
-                # 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.
+        """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)

@@ -180,6 +180,45 @@ 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 not isinstance(installed_version, str) or not isinstance(latest_version, str):
+        # A malformed manifest/registry can carry a number (1.2) or worse;
+        # packaging would raise TypeError. Surface the mismatch instead.
+        return True
+    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)``.

@@ -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,16 @@ 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
+                    # No truthiness gate: the shared comparator already treats
+                    # a missing version on either side as "no update", and the
+                    # store must agree with the UI badge in that case too. A
+                    # missing manifest (not just a missing version field)
+                    # still falls through to the reinstall recovery path.
+                    if 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}")