Test suite overhaul + fixes for the three bugs it uncovered

.github/workflows/test.yml

@@ -4,6 +4,9 @@ on:
   pull_request:
   push:
     branches: [main]
+  # Manual runs against any branch — useful when a PR's automatic run
+  # needs a re-run or didn't get created.
+  workflow_dispatch:
 
 # Both jobs only check out the repo and run pytest.
 permissions:
@@ -13,6 +16,12 @@ jobs:
   plugin-safety:
     name: Plugin safety harness + unit tests
     runs-on: ubuntu-latest
+    env:
+      # The bundled fixture plugin gives the harness at least one real plugin
+      # to render, and REQUIRE_PLUGINS turns "discovered zero plugins" into a
+      # hard failure instead of a silent all-skip green run.
+      LEDMATRIX_PLUGINS_DIR: test/fixtures/plugins
+      LEDMATRIX_REQUIRE_PLUGINS: "1"
     steps:
       - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
         with:
@@ -29,12 +38,9 @@ jobs:
           pip install -r requirements.txt -r requirements-test.txt
           pip install RGBMatrixEmulator
 
-      - name: Run harness + visual rendering tests
+      - name: Run plugin safety harness
         run: |
-          pytest --no-cov \
-            test/plugins/test_harness.py \
-            test/plugins/test_visual_rendering.py \
-            test/plugins/test_plugin_matrix.py
+          pytest --no-cov test/plugins/
 
   unit-tests:
     name: Core unit tests
@@ -55,35 +61,15 @@ jobs:
           pip install -r requirements.txt -r requirements-test.txt
           pip install RGBMatrixEmulator
 
-      # Safety net for the shared sports/scroll/style infrastructure. These
-      # suites existed but were not enrolled in CI, so a refactor of
-      # src/base_classes or src/common could regress them silently. Enrolled
-      # explicitly (not `pytest test/`) so known hardware-only suites don't
-      # break CI; grow this list as more suites are made headless.
+      # Run the ENTIRE test tree (except test/plugins, which the
+      # plugin-safety job owns). New test files are enrolled automatically;
+      # excluding anything requires a visible, commented --ignore here.
+      # Coverage is measured and enforced only in this step — pytest.ini
+      # deliberately carries no coverage flags so local runs stay fast.
       - name: Run core unit suites
         run: |
-          pytest --no-cov \
-            test/test_skin_system.py \
-            test/test_font_manager.py \
-            test/test_data_sources.py \
-            test/test_api_extractors.py \
-            test/test_scroll_helper.py \
-            test/test_scroll_helper_continuous.py \
-            test/test_adaptive_layout.py \
-            test/test_loader_compat_warning.py \
-            test/test_sports_base_characterization.py \
-            test/test_element_style.py \
-            test/test_sports_core_promotions.py \
-            test/test_sports_modes_promotions.py \
-            test/test_sports_capabilities.py \
-            test/test_sports_scroll.py \
-            test/test_version_consistency.py \
-            test/test_plugin_compatibility_gate.py \
-            test/test_install_preserves_existing.py \
-            test/test_core_owned_config_keys.py \
-            test/test_async_plugin_updates.py \
-            test/test_plugin_update_reservation.py \
-            test/test_template_targets.py \
-            test/test_widget_scripts.py \
-            test/test_doc_links.py \
-            test/web_interface/test_cache.py
+          pytest -m "not hardware" test/ \
+            --ignore=test/plugins \
+            --cov=src --cov=web_interface \
+            --cov-report=term \
+            --cov-fail-under=45

pytest.ini

@@ -10,16 +10,13 @@ python_functions = test_*
 testpaths = test
 
 # Output options
-# Note: Coverage options require pytest-cov to be installed
-# Run: pip install pytest-cov
-addopts = 
+# Coverage is deliberately NOT configured here: a bare local `pytest` should
+# be fast and dependency-light. Coverage is measured and enforced in exactly
+# one place — the unit-tests job in .github/workflows/test.yml.
+addopts =
     -v
     --strict-markers
     --tb=short
-    --cov=src
-    --cov-report=term-missing
-    --cov-report=html
-    --cov-fail-under=30
 
 # Markers
 markers =

requirements-test.txt

@@ -4,4 +4,6 @@ pytest>=9.0.3,<10.0.0
 pytest-cov>=4.1.0,<5.0.0
 pytest-mock>=3.11.0,<4.0.0
 freezegun>=1.2,<2        # deterministic time for golden-image tests
+psutil>=6.0.0,<8.0.0     # optional at runtime; installed for tests so the
+                         # /system/status endpoint's real path is exercised
 mypy>=1.5.0,<2.0.0       # static type checking (also pinned in .pre-commit-config.yaml)

src/base_classes/sports/core.py

@@ -383,10 +383,15 @@ def render_skin_card(self, game: Dict, size: tuple) -> Optional[Image.Image]:
             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,18 +106,13 @@ def save_config_atomic(
         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 @@ def _strip_secrets_recursive(self, data_to_filter: Dict[str, Any], secrets: Dict
                 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)
 

src/plugin_system/compatibility.py

@@ -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)``.
 

src/plugin_system/store_manager.py

@@ -2969,16 +2969,27 @@ def update_plugin(self, plugin_id: str) -> bool:
             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():
                     with open(local_manifest_path, 'r', encoding='utf-8') as f:
                         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}")

test/fixtures/plugins/ci-fixture-plugin/config_schema.json

@@ -0,0 +1,31 @@
+{
+    "$schema": "http://json-schema.org/draft-07/schema#",
+    "title": "CI Fixture Plugin",
+    "type": "object",
+    "properties": {
+        "enabled": {
+            "type": "boolean",
+            "default": true
+        },
+        "display_duration": {
+            "type": "number",
+            "default": 5
+        },
+        "border_color": {
+            "type": "array",
+            "items": {"type": "integer", "minimum": 0, "maximum": 255},
+            "minItems": 3,
+            "maxItems": 3,
+            "default": [0, 255, 0],
+            "description": "RGB color of the border rectangle."
+        },
+        "diagonal_color": {
+            "type": "array",
+            "items": {"type": "integer", "minimum": 0, "maximum": 255},
+            "minItems": 3,
+            "maxItems": 3,
+            "default": [255, 0, 0],
+            "description": "RGB color of the diagonals."
+        }
+    }
+}

test/fixtures/plugins/ci-fixture-plugin/manager.py

@@ -0,0 +1,44 @@
+"""
+CI fixture plugin.
+
+Exists so the plugin safety harness (test/plugins/test_plugin_matrix.py and
+the plugin-safety CI job) always has at least one real plugin to load and
+render — without it, an empty plugins/ directory turns the whole job into a
+green no-op. The render is deliberately trivial and fully deterministic:
+a border rectangle plus both diagonals, sized from the display manager's
+declared dimensions. No fonts, no network, no time dependence, so golden
+images are stable across platforms.
+"""
+
+from PIL import ImageDraw
+
+from src.plugin_system.base_plugin import BasePlugin
+
+
+class CIFixturePlugin(BasePlugin):
+    """Deterministic CI-only fixture plugin: renders a border + diagonals
+    pattern sized from the display's declared dimensions. Never shipped to
+    devices; exists solely so the plugin safety harness has a real plugin
+    to exercise in CI."""
+
+    def update(self) -> None:
+        """Nothing to fetch — the render is self-contained."""
+
+    def display(self, force_clear: bool = False) -> None:
+        self.display_manager.clear()
+        width = self.display_manager.matrix.width
+        height = self.display_manager.matrix.height
+        border = tuple(self.config.get("border_color", [0, 255, 0]))
+        diagonal = tuple(self.config.get("diagonal_color", [255, 0, 0]))
+
+        image = self.display_manager.image
+        draw = ImageDraw.Draw(image)
+        # Blank only the declared panel area, then draw edge-to-edge content:
+        # the border proves the plugin reads dynamic dimensions (any overflow
+        # or underfill at any size is a harness bug or a dimensions bug), the
+        # diagonals make golden comparisons sensitive to size/offset drift.
+        draw.rectangle([0, 0, width - 1, height - 1], fill=(0, 0, 0))
+        draw.rectangle([0, 0, width - 1, height - 1], outline=border)
+        draw.line([0, 0, width - 1, height - 1], fill=diagonal)
+        draw.line([0, height - 1, width - 1, 0], fill=diagonal)
+        self.display_manager.update_display()

test/fixtures/plugins/ci-fixture-plugin/manifest.json

@@ -0,0 +1,13 @@
+{
+    "id": "ci-fixture-plugin",
+    "name": "CI Fixture Plugin",
+    "version": "1.0.0",
+    "description": "Bundled test fixture so the plugin safety harness always has at least one real plugin to render in CI. Draws a deterministic border + diagonals pattern at any panel size. Not installable from the store and never shipped to devices.",
+    "author": "LEDMatrix",
+    "entry_point": "manager.py",
+    "class_name": "CIFixturePlugin",
+    "display_modes": ["ci-fixture"],
+    "update_interval": 3600,
+    "min_ledmatrix_version": "2.0.0",
+    "compatible_versions": [">=2.0.0"]
+}

test/fixtures/plugins/ci-fixture-plugin/requirements.txt

@@ -0,0 +1,7 @@
+# No dependencies — the fixture must load in any environment.
+#
+# Pillow is deliberately NOT pinned here even though manager.py imports
+# PIL: it is a core LEDMatrix dependency (see the repo-root
+# requirements.txt), so it is always present wherever the harness runs,
+# and the harness loads plugins with install_deps=False anyway. Pinning
+# it here would only invite a needless pip install during test runs.