fix(vegas): stop capping plugin width by default (#445)

* fix(vegas): stop capping plugin width by default

Vegas plugins read as "cut off early" or "starting in the middle". That
was the per-plugin width budget, not the scroll engine:
overflow_mode=rotate is designed to resume mid-content on each
appearance, so the symptom was the feature working as specified.

Measured over a 17-plugin fleet on a 512px panel, the cap was a bad
trade. Only four plugins were ever wide enough to hit the 3.0 default --
leaderboard 11,518px, news 10,021px, odds-ticker 4,643px, hockey
1,508px. Weather is 650px and flights 512px; the cap never touched them
or the other eleven. So it bought nothing on thirteen plugins while
costing two visible faults on four: content entering mid-item (a news
ticker started at column 6027 of its own strip), and a final rotation
window of whatever happened to be left -- 348px of an 1,840px stocks
ticker, seven seconds of panel time.

Default max_plugin_width_ratio to 0 (uncapped), so every plugin
contributes all of its content and is always entered at its beginning.
The cap remains available, and vegas_max_width_screens still caps an
individual plugin -- which is where the knob belongs, since a genuinely
long ticker is a property of that plugin rather than of the fleet.

Verified on a live 512px device: 327 budget crops in the preceding six
hours, none after.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5

* test(vegas): cover the width cap still working when asked for

Defaulting the cap off must not quietly remove it. Asserts that an
explicit ratio is honoured and validates, alongside the existing check
that omitting it means uncapped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

.github/workflows/test.yml

@@ -72,4 +72,4 @@ jobs:
             --ignore=test/plugins \
             --cov=src --cov=web_interface \
             --cov-report=term \
-            --cov-fail-under=45
+            --cov-fail-under=48

src/base_odds_manager.py

@@ -163,19 +163,25 @@ class BaseOddsManager:
             item = data["items"][0]
             self.logger.debug(f"First item keys: {list(item.keys())}")
             
-            # The ESPN API returns odds data directly in the item, not in a providers array
+            # The ESPN API returns odds data directly in the item, not in a
-            # Extract the odds data directly from the item
+            # providers array. ESPN sends explicit JSON nulls for absent
+            # sides ("homeTeamOdds": null), so every level uses `or {}` —
+            # .get's default only applies when the key is missing entirely.
+            home = item.get("homeTeamOdds") or {}
+            away = item.get("awayTeamOdds") or {}
             extracted_data = {
                 "details": item.get("details"),
                 "over_under": item.get("overUnder"),
                 "spread": item.get("spread"),
                 "home_team_odds": {
-                    "money_line": item.get("homeTeamOdds", {}).get("moneyLine"),
+                    "money_line": home.get("moneyLine"),
-                    "spread_odds": item.get("homeTeamOdds", {}).get("current", {}).get("pointSpread", {}).get("value")
+                    "spread_odds": ((home.get("current") or {})
+                                    .get("pointSpread") or {}).get("value")
                 },
                 "away_team_odds": {
-                    "money_line": item.get("awayTeamOdds", {}).get("moneyLine"),
+                    "money_line": away.get("moneyLine"),
-                    "spread_odds": item.get("awayTeamOdds", {}).get("current", {}).get("pointSpread", {}).get("value")
+                    "spread_odds": ((away.get("current") or {})
+                                    .get("pointSpread") or {}).get("value")
                 }
             }
             self.logger.debug(f"Returning extracted odds data: {json.dumps(extracted_data, indent=2)}")
@@ -260,7 +266,11 @@ class BaseOddsManager:
         Returns:
             Formatted odds summary string
         """
-        if not self.is_odds_available(odds_data):
+        # Gate only on truly-empty / negative-cached data. is_odds_available
+        # deliberately ignores money lines (its callers decide whether to
+        # RENDER an odds widget), but a summary of money-line-only odds is
+        # still meaningful — the parts loop below handles them.
+        if not odds_data or odds_data.get('no_odds'):
             return "No odds available"
 
         parts = []

src/common/api_helper.py

@@ -273,19 +273,34 @@ class APIHelper:
         """
         Clear cache data.
 
+        Uses CacheManager's real surface (clear_cache / delete /
+        list_cache_files); safely no-ops on managers without it. The old
+        implementation guarded on a nonexistent ``clear`` method, so it
+        silently never cleared anything.
+
         Args:
-            pattern: Optional pattern to match cache keys
+            pattern: Optional substring to match cache keys; only matching
+                entries are deleted.
         """
-        if self.cache_manager:
+        if not self.cache_manager:
-            if hasattr(self.cache_manager, 'clear'):
+            return
-                if pattern:
+        if pattern:
-                    # Clear only keys matching pattern
+            if (hasattr(self.cache_manager, 'list_cache_files')
-                    keys = self.cache_manager.keys()
+                    and hasattr(self.cache_manager, 'delete')):
-                    for key in keys:
+                for entry in self.cache_manager.list_cache_files():
-                        if pattern in key:
+                    key = entry.get('key') if isinstance(entry, dict) else None
-                            self.cache_manager.delete(key)
+                    if key and pattern in key:
-                else:
+                        self.cache_manager.delete(key)
-                    self.cache_manager.clear()
+            else:
+                self.logger.debug(
+                    "Cache manager lacks list_cache_files/delete; "
+                    "cannot clear by pattern")
+        elif hasattr(self.cache_manager, 'clear_cache'):
+            self.cache_manager.clear_cache()
+        elif hasattr(self.cache_manager, 'clear'):
+            self.cache_manager.clear()
+        else:
+            self.logger.debug("Cache manager exposes no clear method; no-op")
     
     def _get_from_cache(self, key: str) -> Optional[Any]:
         """Get data from cache."""

src/common/config_helper.py

@@ -5,6 +5,7 @@ Handles configuration management and validation for LED matrix plugins.
 Extracted from LEDMatrix core to provide reusable functionality for plugins.
 """
 
+import copy
 import json
 import logging
 from pathlib import Path
@@ -160,17 +161,20 @@ class ConfigHelper:
             override_config: Configuration to merge in (takes precedence)
             
         Returns:
-            Merged configuration dictionary
+            Merged configuration dictionary (fully independent of both
+            inputs — a shallow copy would alias un-overridden nested dicts,
+            so mutating the result would mutate the caller's base config).
         """
-        merged = base_config.copy()
+        merged = copy.deepcopy(base_config)
 
         for key, value in override_config.items():
             if key in merged and isinstance(merged[key], dict) and isinstance(value, dict):
                 # Recursively merge nested dictionaries
                 merged[key] = self.merge_configs(merged[key], value)
             else:
-                # Override with new value
+                # Override with new value — deep-copied so mutating the
-                merged[key] = value
+                # merged result can't reach back into override_config.
+                merged[key] = copy.deepcopy(value)
         
         return merged
     

src/common/display_helper.py

@@ -115,17 +115,13 @@ class DisplayHelper:
         if home_logo and away_logo:
             self._draw_logos(main_img, home_logo, away_logo)
         
-        # Draw status/period text (top center)
+        # Draw one combined top line (period/status/clock all share y=1 —
-        if status_text or period_text:
+        # drawing them separately overprinted each other).
-            status_display = f"{period_text} {status_text}".strip()
+        top_line = " ".join(p for p in [period_text, status_text, clock] if p)
-            if status_display:
+        if top_line:
-                self._draw_centered_text(draw, status_display, 
+            self._draw_centered_text(draw, top_line,
-                                       fonts.get('time', fonts.get('status')), 
+                                   fonts.get('time', fonts.get('status')),
-                                       y_position=1)
+                                   y_position=1)
-        
-        # Draw clock if available
-        if clock:
-            self._draw_centered_text(draw, clock, fonts.get('time'), y_position=1)
         
         # Draw scores (center)
         score_text = f"{away_score}-{home_score}"
@@ -153,12 +149,18 @@ class DisplayHelper:
         """
         Draw a ticker/scrolling text layout.
         
+        Renders a single static frame with the text at the left edge; the
+        caller advances the scroll by re-rendering or shifting. The
+        scroll_speed parameter is accepted for API compatibility but does
+        not affect this frame. (Previously the text was drawn starting at
+        x=display_width — entirely off-canvas — so every frame was blank.)
+
         Args:
             text: Text to display
             font: Font to use
             background_color: Background color
             text_color: Text color
-            scroll_speed: Pixels to scroll per frame
+            scroll_speed: Accepted for compatibility; unused per-frame
 
         Returns:
             PIL Image with ticker layout
@@ -166,11 +168,7 @@ class DisplayHelper:
         img = self.create_base_image(background_color)
         draw = ImageDraw.Draw(img)
 
-        # Start text off-screen to the right
+        self._draw_text_with_outline(draw, text, (0, self.display_height // 2 - 6),
-        x_position = self.display_width
-        
-        # Draw text
-        self._draw_text_with_outline(draw, text, (x_position, self.display_height // 2 - 6), 
                                    font, fill=text_color)
 
         return img
@@ -214,15 +212,9 @@ class DisplayHelper:
         Returns:
             PIL Image with error message
         """
-        img = self.create_base_image((50, 0, 0))  # Dark red background
+        # Dark red background, white text
-
-        # Use default font
         font = ImageFont.load_default()
-        
+        return self.draw_centered_text(message, font, (50, 0, 0), (255, 255, 255))
-        # Draw centered error message
-        self._draw_centered_text(message, font, (50, 0, 0), (255, 255, 255))
-        
-        return img
     
     def draw_no_data_message(self, message: str = "No Data") -> Image.Image:
         """
@@ -234,11 +226,8 @@ class DisplayHelper:
         Returns:
             PIL Image with no data message
         """
-        img = self.create_base_image((0, 0, 0))
         font = ImageFont.load_default()
-        self._draw_centered_text(message, font, (0, 0, 0), (150, 150, 150))
+        return self.draw_centered_text(message, font, (0, 0, 0), (150, 150, 150))
-        
-        return img
     
     def get_display_dimensions(self) -> Tuple[int, int]:
         """

src/config_manager.py

@@ -269,20 +269,47 @@ class ConfigManager:
             self.logger.error(error_msg, exc_info=True)
             raise ConfigError(error_msg, config_path=self.config_path) from e
 
+    @staticmethod
+    def _is_parallel_secrets_list(value: Any) -> bool:
+        """True for the parallel-placeholder list shape emitted by
+        ``secret_helpers.separate_secrets`` for array-item secrets: a
+        non-empty list whose elements are ALL dicts (``{}`` marks an item
+        with no secrets). Any other list-shaped secrets value is a
+        whole-key secret (e.g. a list of secret scalars)."""
+        return (isinstance(value, list) and bool(value)
+                and all(isinstance(item, dict) for item in value))
+
     def _strip_secrets_recursive(self, data_to_filter: Dict[str, Any], secrets: Dict[str, Any]) -> Dict[str, Any]:
         """Recursively remove secret keys from a dictionary."""
         result = {}
         for key, value in data_to_filter.items():
-            if key in secrets:
+            if key not in secrets:
-                if isinstance(value, dict) and isinstance(secrets[key], dict):
-                    # This key is a shared group, recurse
-                    stripped_sub_dict = self._strip_secrets_recursive(value, secrets[key])
-                    if stripped_sub_dict: # Only add if there's non-secret data left
-                        result[key] = stripped_sub_dict
-                # Else, it's a secret key at this level, so we skip it
-            else:
                 # This key is not in secrets, so we keep it
                 result[key] = value
+                continue
+            sec = secrets[key]
+            if isinstance(value, dict) and isinstance(sec, dict):
+                # This key is a shared group, recurse
+                stripped_sub_dict = self._strip_secrets_recursive(value, sec)
+                if stripped_sub_dict: # Only add if there's non-secret data left
+                    result[key] = stripped_sub_dict
+            elif isinstance(value, list) and self._is_parallel_secrets_list(sec):
+                # Parallel-list shape from separate_secrets: sec[i] holds the
+                # secret fields of value[i] ({} = item i has none). Strip each
+                # item and ALWAYS keep the list — indices must survive so the
+                # merge-on-load can realign secrets with their items. The
+                # regular list's length is authoritative: extra secrets
+                # entries are ignored.
+                stripped_items = []
+                for i, item in enumerate(value):
+                    s_item = sec[i] if i < len(sec) else {}
+                    if isinstance(item, dict) and s_item:
+                        stripped_items.append(self._strip_secrets_recursive(item, s_item))
+                    else:
+                        stripped_items.append(item)
+                result[key] = stripped_items
+            # Else: whole-key secret (scalar, list of secret scalars, or a
+            # shape mismatch) -> drop the key entirely. Never leak.
         return result
 
     def _load_secrets_for_save(self) -> Dict[str, Any]:
@@ -358,11 +385,39 @@ class ConfigManager:
             return None
 
     def _deep_merge(self, target: Dict[str, Any], source: Dict[str, Any]) -> None:
-        """Deep merge source dict into target dict."""
+        """Deep merge source dict into target dict.
+
+        Sole call site: merging config_secrets.json into the loaded config.
+        Understands the parallel-list shape separate_secrets emits for
+        array-item secrets (see _is_parallel_secrets_list): each secrets
+        list item is merged into the config list item at the same index
+        ({} placeholders skipped). The config list's length is
+        authoritative — a user deleting an array item from config.json
+        must not have it resurrected from a stale secrets entry."""
         for key, value in source.items():
             if key in target and isinstance(target[key], dict) and isinstance(value, dict):
                 self._deep_merge(target[key], value)
+            elif (key in target and isinstance(target[key], list)
+                    and self._is_parallel_secrets_list(value)):
+                tlist = target[key]
+                for i, s_item in enumerate(value):
+                    if i >= len(tlist):
+                        # Interpolate only config-side data here — nothing
+                        # iterated out of the secrets dict (not even the key
+                        # name) may reach the log.
+                        self.logger.warning(
+                            "A secrets list is longer than the config list it "
+                            "parallels (config has %d item(s)); ignoring the "
+                            "extra entries", len(tlist))
+                        break
+                    if not s_item:
+                        continue  # {} placeholder: item i has no secrets
+                    if isinstance(tlist[i], dict):
+                        self._deep_merge(tlist[i], s_item)
+                    else:
+                        tlist[i] = s_item  # shape drift; the secret wins
             else:
+                # Scalars AND whole-secret scalar arrays: replace (legacy).
                 target[key] = value
 
     def _create_config_from_template(self) -> None:

src/dynamic_team_resolver.py

@@ -168,9 +168,13 @@ class DynamicTeamResolver:
                 # Sort by ranking (1, 2, 3, etc.)
                 sorted_rankings = dict(sorted(rankings.items(), key=lambda x: x[1]))
 
-                # Cache the results
+                # Cache the results ON THE CLASS. Assigning through self
-                self._rankings_cache = sorted_rankings
+                # would create instance attributes that shadow the shared
-                self._cache_timestamp = current_time
+                # class-level cache, making it per-instance — and every
+                # scoreboard constructs its own resolver, so the cache
+                # would never actually be shared.
+                DynamicTeamResolver._rankings_cache = sorted_rankings
+                DynamicTeamResolver._cache_timestamp = current_time
                 
                 self.logger.info(f"Fetched rankings for {len(sorted_rankings)} teams")
                 return sorted_rankings
@@ -216,9 +220,11 @@ class DynamicTeamResolver:
         return any(pattern in team_name.upper() for pattern in dynamic_patterns)
     
     def clear_cache(self):
-        """Clear the rankings cache to force fresh data on next request."""
+        """Clear the SHARED rankings cache to force fresh data on next
-        self._rankings_cache = {}
+        request. Writes through the class — assigning via self would only
-        self._cache_timestamp = 0
+        shadow the shared cache for this instance."""
+        DynamicTeamResolver._rankings_cache = {}
+        DynamicTeamResolver._cache_timestamp = 0
         self.logger.info("Cleared dynamic team rankings cache")
 
 

src/logging_config.py

@@ -5,6 +5,7 @@ Provides consistent logging configuration across the LEDMatrix application.
 Supports structured logging with context information and appropriate log levels.
 """
 
+import copy
 import logging
 import sys
 import os
@@ -65,8 +66,12 @@ class ContextualFormatter(logging.Formatter):
         self.include_context = include_context
     
     def format(self, record: logging.LogRecord) -> str:
-        """Format log record with context."""
+        """Format log record with context.
-        # Add context to message if present
+
+        Works on a shallow copy of the record: a record is formatted once
+        PER HANDLER, so mutating record.msg in place (the old behavior)
+        prepended the context prefix again for every additional handler.
+        """
         if self.include_context:
             context_parts = []
 
@@ -81,6 +86,7 @@ class ContextualFormatter(logging.Formatter):
                     context_parts.append(f"[{key}: {value}]")
 
             if context_parts:
+                record = copy.copy(record)
                 record.msg = ' '.join(context_parts) + ' ' + str(record.msg)
 
         return super().format(record)
@@ -224,8 +230,11 @@ def log_warning(logger: logging.Logger, message: str, **kwargs) -> None:
 
 
 def log_error(logger: logging.Logger, message: str, **kwargs) -> None:
-    """Log error message with context."""
+    """Log error message with context. Defaults exc_info=True; a caller
-    log_with_context(logger, logging.ERROR, message, **kwargs, exc_info=True)
+    passing exc_info explicitly wins (the old hardcoded keyword raised
+    TypeError on that duplicate)."""
+    kwargs.setdefault('exc_info', True)
+    log_with_context(logger, logging.ERROR, message, **kwargs)
 
 
 def log_debug(logger: logging.Logger, message: str, **kwargs) -> None:

src/plugin_system/base_plugin.py

@@ -364,8 +364,10 @@ class BasePlugin(ABC):
                 # Handle None case
                 if duration is None:
                     pass  # Fall through to config
-                # Try to convert to float if it's a number or numeric string
+                # Try to convert to float if it's a number or numeric string.
-                elif isinstance(duration, (int, float)):
+                # bool is excluded: it's an int subclass, and True would
+                # otherwise read as a 1-second duration.
+                elif isinstance(duration, (int, float)) and not isinstance(duration, bool):
                     if duration > 0:
                         return float(duration)
                     else:
@@ -403,8 +405,9 @@ class BasePlugin(ABC):
         # Fall back to config
         config_duration = self.config.get("display_duration", 15.0)
         try:
-            # Ensure config value is also a valid float
+            # Ensure config value is also a valid float (bool excluded — an
-            if isinstance(config_duration, (int, float)):
+            # int subclass that would otherwise read True as 1 second)
+            if isinstance(config_duration, (int, float)) and not isinstance(config_duration, bool):
                 if config_duration > 0:
                     return float(config_duration)
                 else:
@@ -794,10 +797,12 @@ class BasePlugin(ABC):
                 self.logger.error("'enabled' must be a boolean")
                 return False
 
-        # Check display_duration if present
+        # Check display_duration if present. bool is excluded explicitly:
+        # it's an int subclass, and get_display_duration rejects it too.
         if "display_duration" in self.config:
             duration = self.config["display_duration"]
-            if not isinstance(duration, (int, float)) or duration <= 0:
+            if (not isinstance(duration, (int, float))
+                    or isinstance(duration, bool) or duration <= 0):
                 self.logger.error("'display_duration' must be a positive number")
                 return False
 

src/plugin_system/saved_repositories.py

@@ -6,6 +6,7 @@ Manages saved GitHub repository URLs for easy plugin discovery and installation.
 
 import json
 import logging
+import os
 from pathlib import Path
 from typing import List, Dict, Optional
 
@@ -43,20 +44,45 @@ class SavedRepositoriesManager:
             return []
     
     def _save_repositories(self) -> bool:
-        """Save repositories to file."""
+        """Save repositories to file atomically.
+
+        Writes to a temp file in the same directory and os.replace()s it
+        over the target, so a failed write can never truncate or
+        half-overwrite an existing saved_repositories.json.
+        """
+        tmp_path = self.config_path.with_suffix(self.config_path.suffix + '.tmp')
         try:
             # Ensure directory exists
             self.config_path.parent.mkdir(parents=True, exist_ok=True)
 
-            with open(self.config_path, 'w') as f:
+            with open(tmp_path, 'w') as f:
                 json.dump(self.repositories, f, indent=2)
+                f.flush()
+                os.fsync(f.fileno())
+            os.replace(tmp_path, self.config_path)
 
             self.logger.info(f"Saved {len(self.repositories)} repositories to {self.config_path}")
             return True
         except Exception as e:
             self.logger.error(f"Error saving repositories: {e}")
+            try:
+                tmp_path.unlink(missing_ok=True)
+            except OSError:
+                pass
             return False
     
+    @staticmethod
+    def _clean_url(repo_url: str) -> str:
+        """Normalize a repo URL: strip whitespace, trailing slashes, and a
+        trailing ``.git`` suffix ONLY. (The old ``.replace('.git', '')``
+        was an unanchored substring replace that mangled URLs merely
+        containing ``.git``, e.g. ``https://github.com/user/my.github.io``.)
+        """
+        repo_url = repo_url.strip().rstrip('/')
+        if repo_url.endswith('.git'):
+            repo_url = repo_url[:-4]
+        return repo_url
+
     def get_all(self) -> List[Dict[str, str]]:
         """Get all saved repositories."""
         return self.repositories.copy()
@@ -72,8 +98,7 @@ class SavedRepositoriesManager:
         Returns:
             True if added successfully
         """
-        # Clean URL
+        repo_url = self._clean_url(repo_url)
-        repo_url = repo_url.strip().rstrip('/').replace('.git', '')
 
         # Check if already exists
         for repo in self.repositories:
@@ -96,7 +121,12 @@ class SavedRepositoriesManager:
             'type': 'registry' if 'plugins.json' in repo_url or 'ledmatrix-plugins' in repo_url.lower() else 'single'
         })
 
-        return self._save_repositories()
+        if not self._save_repositories():
+            # Keep memory consistent with disk: a failed save must not leave
+            # a phantom entry that only this process can see.
+            self.repositories.pop()
+            return False
+        return True
     
     def remove(self, repo_url: str) -> bool:
         """
@@ -108,21 +138,25 @@ class SavedRepositoriesManager:
         Returns:
             True if removed successfully
         """
-        # Clean URL
+        repo_url = self._clean_url(repo_url)
-        repo_url = repo_url.strip().rstrip('/').replace('.git', '')
 
-        original_count = len(self.repositories)
+        previous = self.repositories
-        self.repositories = [r for r in self.repositories if r.get('url') != repo_url]
+        remaining = [r for r in previous if r.get('url') != repo_url]
 
-        if len(self.repositories) < original_count:
+        if len(remaining) < len(previous):
-            return self._save_repositories()
+            self.repositories = remaining
+            if not self._save_repositories():
+                # Failed save: restore so memory matches disk.
+                self.repositories = previous
+                return False
+            return True
         else:
             self.logger.warning(f"Repository not found: {repo_url}")
             return False
     
     def has(self, repo_url: str) -> bool:
         """Check if a repository is already saved."""
-        repo_url = repo_url.strip().rstrip('/').replace('.git', '')
+        repo_url = self._clean_url(repo_url)
         return any(r.get('url') == repo_url for r in self.repositories)
     
     def get_registry_repositories(self) -> List[Dict[str, str]]:

src/startup_validator.py

@@ -38,6 +38,11 @@ class StartupValidator:
         """
         self.logger.info("Starting startup validation...")
 
+        # Fresh lists each run — without this, calling validate_all() twice
+        # duplicated every message.
+        self.errors = []
+        self.warnings = []
+
         # Validate configuration
         self._validate_config()
         

test/test_api_helper.py

@@ -0,0 +1,275 @@
+"""
+Tests for src/common/api_helper.py (APIHelper).
+
+Covers rate limiting, cached GETs, ESPN URL/cache-key construction,
+session header defaults and per-call merging, the retry adapter, and the
+fixed clear_cache() behavior (real CacheManager surface: clear_cache /
+delete / list_cache_files, with safe no-ops elsewhere).
+
+No real network: helper.session.get/post are always replaced with mocks.
+"""
+
+import types
+from unittest.mock import MagicMock, Mock
+
+import pytest
+import requests
+from freezegun import freeze_time
+
+import src.common.api_helper as api_helper_module
+from src.common.api_helper import APIHelper
+
+
+def _make_response(payload):
+    response = MagicMock()
+    response.json.return_value = payload
+    response.raise_for_status.return_value = None
+    return response
+
+
+@pytest.fixture
+def cache():
+    cache = MagicMock()
+    cache.get.return_value = None
+    return cache
+
+
+@pytest.fixture
+def helper(cache):
+    helper = APIHelper(cache_manager=cache)
+    # Default min interval is 1.0s and would really sleep between requests.
+    helper.set_rate_limit(0)
+    return helper
+
+
+# ---------------------------------------------------------------------------
+# Rate limiting
+# ---------------------------------------------------------------------------
+
+class TestRateLimiting:
+    def test_sleeps_for_remaining_interval(self, helper, monkeypatch):
+        fake_time = MagicMock()
+        fake_time.time.side_effect = [102.0, 105.0]
+        monkeypatch.setattr(api_helper_module, 'time', fake_time)
+
+        helper.set_rate_limit(5)
+        helper._last_request_time = 100.0
+        helper._enforce_rate_limit()
+
+        # 2s elapsed of a 5s interval -> sleep the remaining 3s.
+        fake_time.sleep.assert_called_once()
+        assert fake_time.sleep.call_args[0][0] == pytest.approx(3.0)
+        assert helper._last_request_time == 105.0
+
+    def test_no_sleep_when_interval_elapsed(self, helper, monkeypatch):
+        fake_time = MagicMock()
+        fake_time.time.side_effect = [200.0, 201.0]
+        monkeypatch.setattr(api_helper_module, 'time', fake_time)
+
+        helper.set_rate_limit(5)
+        helper._last_request_time = 100.0
+        helper._enforce_rate_limit()
+
+        fake_time.sleep.assert_not_called()
+        assert helper._last_request_time == 201.0
+
+
+# ---------------------------------------------------------------------------
+# get()
+# ---------------------------------------------------------------------------
+
+class TestGet:
+    def test_cache_hit_skips_request_and_rate_limit(self, helper, cache):
+        cache.get.return_value = {'cached': True}
+        helper.session.get = Mock()
+        rate_spy = Mock()
+        helper._enforce_rate_limit = rate_spy
+
+        result = helper.get('https://example.com/api', cache_key='k')
+
+        assert result == {'cached': True}
+        helper.session.get.assert_not_called()
+        rate_spy.assert_not_called()
+
+    def test_cache_miss_fetches_and_caches_without_ttl(self, helper, cache):
+        cache.get.return_value = None
+        helper.session.get = Mock(return_value=_make_response({'a': 1}))
+
+        result = helper.get('https://example.com/api', cache_key='k',
+                            cache_ttl=999)
+
+        assert result == {'a': 1}
+        # Pin the ttl-dropped contract: CacheManager.set is called with
+        # (key, data) only — the cache_ttl argument is discarded.
+        cache.set.assert_called_once_with('k', {'a': 1})
+
+    def test_request_exception_returns_none_and_caches_nothing(
+            self, helper, cache):
+        helper.session.get = Mock(
+            side_effect=requests.exceptions.RequestException('boom'))
+
+        result = helper.get('https://example.com/api', cache_key='k')
+
+        assert result is None
+        cache.set.assert_not_called()
+
+    def test_timeout_zero_falls_back_to_default(self, helper):
+        # Quirk pin: `timeout or self.default_timeout` treats an explicit
+        # timeout=0 as falsy, so the default (30) is used instead.
+        helper.session.get = Mock(return_value=_make_response({}))
+
+        helper.get('https://example.com/api', timeout=0)
+
+        assert helper.session.get.call_args.kwargs['timeout'] == 30
+
+    def test_per_call_headers_merge_over_session_headers(self, helper):
+        helper.session.get = Mock(return_value=_make_response({}))
+
+        helper.get('https://example.com/api', headers={'X-Custom': 'yes'})
+
+        sent = helper.session.get.call_args.kwargs['headers']
+        # Merged, not replaced: session defaults survive alongside the
+        # per-call header.
+        assert sent['X-Custom'] == 'yes'
+        assert sent['User-Agent'] == (
+            'LEDMatrix/1.0 (+https://github.com/ChuckBuilds/LEDMatrix)')
+        assert sent['Accept'] == 'application/json'
+        # The session's own headers are not polluted by the per-call ones.
+        assert 'X-Custom' not in helper.session.headers
+
+
+# ---------------------------------------------------------------------------
+# ESPN helpers
+# ---------------------------------------------------------------------------
+
+class TestEspnHelpers:
+    @freeze_time('2026-08-07')
+    def test_fetch_espn_scoreboard_url_params_and_cache_key(self, helper):
+        helper.get = Mock(return_value={'ok': 1})
+
+        result = helper.fetch_espn_scoreboard('football', 'nfl')
+
+        assert result == {'ok': 1}
+        helper.get.assert_called_once_with(
+            'https://site.api.espn.com/apis/site/v2/sports/football/nfl/scoreboard',
+            params={'dates': '20260807', 'limit': 1000},
+            cache_key='espn_football_nfl_20260807',
+            cache_ttl=300,
+        )
+
+    def test_fetch_espn_scoreboard_explicit_date(self, helper):
+        helper.get = Mock(return_value=None)
+
+        helper.fetch_espn_scoreboard('basketball', 'nba', date='20250115')
+
+        kwargs = helper.get.call_args.kwargs
+        assert kwargs['params'] == {'dates': '20250115', 'limit': 1000}
+        assert kwargs['cache_key'] == 'espn_basketball_nba_20250115'
+
+    def test_fetch_espn_standings_url_and_cache_key(self, helper):
+        helper.get = Mock(return_value={'ok': 1})
+
+        helper.fetch_espn_standings('football', 'nfl')
+
+        helper.get.assert_called_once_with(
+            'https://site.api.espn.com/apis/site/v2/sports/football/nfl/standings',
+            cache_key='espn_standings_football_nfl',
+            cache_ttl=3600,
+        )
+
+    def test_fetch_espn_rankings_url_and_cache_key(self, helper):
+        helper.get = Mock(return_value={'ok': 1})
+
+        helper.fetch_espn_rankings('football', 'college-football')
+
+        helper.get.assert_called_once_with(
+            'https://site.api.espn.com/apis/site/v2/sports/football/college-football/rankings',
+            cache_key='espn_rankings_football_college-football',
+            cache_ttl=3600,
+        )
+
+
+# ---------------------------------------------------------------------------
+# Session setup
+# ---------------------------------------------------------------------------
+
+class TestSessionSetup:
+    def test_user_agent_exact(self, helper):
+        # Regression guard: ESPN began 403ing other user agents; this exact
+        # string must be sent on every request.
+        assert helper.session.headers['User-Agent'] == (
+            'LEDMatrix/1.0 (+https://github.com/ChuckBuilds/LEDMatrix)')
+
+    def test_retry_adapter_configuration(self):
+        helper = APIHelper(cache_manager=None, max_retries=7)
+
+        retries = helper.session.get_adapter('https://x').max_retries
+        assert retries.total == 7
+        assert {429, 500, 502, 503, 504} <= set(retries.status_forcelist)
+
+
+# ---------------------------------------------------------------------------
+# clear_cache (fixed behavior: real CacheManager surface)
+# ---------------------------------------------------------------------------
+
+class TestClearCache:
+    def test_no_pattern_uses_clear_cache_method(self):
+        manager = types.SimpleNamespace(clear_cache=Mock())
+        helper = APIHelper(cache_manager=manager)
+        helper.set_rate_limit(0)
+
+        helper.clear_cache()
+
+        manager.clear_cache.assert_called_once_with()
+
+    def test_no_pattern_falls_back_to_clear(self):
+        manager = types.SimpleNamespace(clear=Mock())
+        helper = APIHelper(cache_manager=manager)
+        helper.set_rate_limit(0)
+
+        helper.clear_cache()
+
+        manager.clear.assert_called_once_with()
+
+    def test_no_pattern_manager_without_any_clear_is_noop(self):
+        helper = APIHelper(cache_manager=object())
+        helper.set_rate_limit(0)
+
+        helper.clear_cache()  # must not raise
+
+    def test_pattern_deletes_only_matching_keys(self):
+        manager = types.SimpleNamespace(
+            list_cache_files=Mock(return_value=[
+                {'key': 'espn_nfl_x'},
+                {'key': 'other'},
+            ]),
+            delete=Mock(),
+        )
+        helper = APIHelper(cache_manager=manager)
+        helper.set_rate_limit(0)
+
+        helper.clear_cache(pattern='espn')
+
+        manager.delete.assert_called_once_with('espn_nfl_x')
+
+    def test_pattern_manager_without_list_cache_files_is_noop(self):
+        helper = APIHelper(cache_manager=object())
+        helper.set_rate_limit(0)
+
+        helper.clear_cache(pattern='espn')  # must not raise
+
+
+# ---------------------------------------------------------------------------
+# No cache manager
+# ---------------------------------------------------------------------------
+
+class TestNoCacheManager:
+    def test_all_cache_operations_safe_without_manager(self):
+        helper = APIHelper(cache_manager=None)
+        helper.set_rate_limit(0)
+
+        assert helper.get_cache('k') is None
+        assert helper._get_from_cache('k') is None
+        assert helper.set_cache('k', {'a': 1}) is None
+        assert helper.clear_cache() is None
+        assert helper.clear_cache(pattern='espn') is None

test/test_base_odds_manager.py

@@ -0,0 +1,359 @@
+"""
+Tests for src/base_odds_manager.py (BaseOddsManager).
+
+Covers get_odds validation/caching/URL construction, the null-safe
+_extract_espn_data fix (ESPN sends explicit JSON nulls for absent sides),
+the no_odds sentinel, stale-cache fallback on request failure,
+is_odds_available's ML-blind truth table, the fixed format_odds_summary
+gate (money-line-only odds now format), get_odds_for_games, and
+configuration loading.
+
+No real network: src.base_odds_manager.requests.get is always patched.
+"""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+import requests
+
+from src.base_odds_manager import BaseOddsManager
+
+
+FULL_ITEM = {
+    'details': 'DAL -3.5',
+    'overUnder': 47.5,
+    'spread': -3.5,
+    'homeTeamOdds': {'moneyLine': -150, 'current': {'pointSpread': {'value': -3.5}}},
+    'awayTeamOdds': {'moneyLine': 130, 'current': {'pointSpread': {'value': 3.5}}},
+}
+
+FULL_EXTRACTED = {
+    'details': 'DAL -3.5',
+    'over_under': 47.5,
+    'spread': -3.5,
+    'home_team_odds': {'money_line': -150, 'spread_odds': -3.5},
+    'away_team_odds': {'money_line': 130, 'spread_odds': 3.5},
+}
+
+
+def _make_response(payload):
+    response = MagicMock()
+    response.json.return_value = payload
+    response.raise_for_status.return_value = None
+    return response
+
+
+@pytest.fixture
+def cache_manager():
+    cm = MagicMock()
+    # A bare MagicMock returns truthy Mocks from every call, so every
+    # get_odds() would look like a cache hit. Explicitly wire a miss.
+    cm.get_with_auto_strategy.return_value = None
+    return cm
+
+
+@pytest.fixture
+def manager(cache_manager):
+    return BaseOddsManager(cache_manager)
+
+
+@pytest.fixture
+def mock_get():
+    with patch('src.base_odds_manager.requests.get') as m:
+        m.return_value = _make_response({'items': [dict(FULL_ITEM)]})
+        yield m
+
+
+# ---------------------------------------------------------------------------
+# get_odds
+# ---------------------------------------------------------------------------
+
+class TestGetOdds:
+    def test_none_sport_raises(self, manager):
+        with pytest.raises(ValueError):
+            manager.get_odds(None, 'nfl', '1')
+
+    def test_none_league_raises(self, manager):
+        with pytest.raises(ValueError):
+            manager.get_odds('football', None, '1')
+
+    def test_cache_key_and_url(self, manager, cache_manager, mock_get):
+        manager.get_odds('football', 'nfl', '401')
+
+        cache_manager.get_with_auto_strategy.assert_called_once_with(
+            'odds_espn_football_nfl_401')
+        url = mock_get.call_args[0][0]
+        # Event id appears twice: /events/<id>/competitions/<id>/odds
+        assert '/events/401/competitions/401/odds' in url
+        assert url == ('https://sports.core.api.espn.com/v2/sports/football/'
+                       'leagues/nfl/events/401/competitions/401/odds')
+        assert mock_get.call_args.kwargs['timeout'] == 30
+
+    def test_ncaa_fb_maps_to_college_football(self, manager, mock_get):
+        manager.get_odds('football', 'ncaa_fb', '401')
+
+        url = mock_get.call_args[0][0]
+        assert '/leagues/college-football/' in url
+
+    def test_unknown_league_passes_through(self, manager, mock_get):
+        manager.get_odds('football', 'xfl', '401')
+
+        assert '/leagues/xfl/' in mock_get.call_args[0][0]
+
+    def test_cache_hit_skips_http(self, manager, cache_manager, mock_get):
+        cache_manager.get_with_auto_strategy.return_value = {'spread': -3.0}
+
+        result = manager.get_odds('football', 'nfl', '401')
+
+        assert result == {'spread': -3.0}
+        mock_get.assert_not_called()
+
+    def test_cached_no_odds_sentinel_returned_verbatim(
+            self, manager, cache_manager, mock_get):
+        cache_manager.get_with_auto_strategy.return_value = {'no_odds': True}
+
+        result = manager.get_odds('football', 'nfl', '401')
+
+        assert result == {'no_odds': True}
+        mock_get.assert_not_called()
+        assert manager.is_odds_available(result) is False
+
+    def test_success_caches_extracted_data_with_interval_ttl(
+            self, manager, cache_manager, mock_get):
+        result = manager.get_odds('football', 'nfl', '401',
+                                  update_interval_seconds=100)
+
+        assert result == FULL_EXTRACTED
+        cache_manager.set.assert_called_once_with(
+            'odds_espn_football_nfl_401', FULL_EXTRACTED, ttl=100)
+
+    def test_no_odds_caches_sentinel(self, manager, cache_manager, mock_get):
+        mock_get.return_value = _make_response({'count': 0, 'items': []})
+
+        result = manager.get_odds('football', 'nfl', '401')
+
+        assert result is None
+        cache_manager.set.assert_called_once_with(
+            'odds_espn_football_nfl_401', {'no_odds': True}, ttl=3600)
+
+    def test_zero_interval_falls_back_to_default(
+            self, manager, cache_manager, mock_get):
+        # Quirk pin: `update_interval_seconds or self.update_interval`
+        # treats an explicit 0 as falsy, so the 3600 default wins.
+        manager.get_odds('football', 'nfl', '401', update_interval_seconds=0)
+
+        assert cache_manager.set.call_args.kwargs['ttl'] == 3600
+
+    def test_request_exception_falls_back_to_stale_cache(
+            self, manager, cache_manager, mock_get):
+        cache_manager.get_with_auto_strategy.side_effect = [
+            None, {'stale': True}]
+        mock_get.side_effect = requests.exceptions.RequestException('boom')
+
+        result = manager.get_odds('football', 'nfl', '401')
+
+        assert result == {'stale': True}
+        assert cache_manager.get_with_auto_strategy.call_count == 2
+
+
+# ---------------------------------------------------------------------------
+# _extract_espn_data
+# ---------------------------------------------------------------------------
+
+class TestExtractEspnData:
+    def test_full_item_extracts_all_fields(self, manager):
+        result = manager._extract_espn_data({'items': [dict(FULL_ITEM)]})
+        assert result == FULL_EXTRACTED
+
+    def test_explicit_nulls_do_not_raise(self, manager):
+        # Post-fix: ESPN sends explicit JSON nulls for absent sides
+        # ("homeTeamOdds": null, "current": null); extraction must not
+        # raise and yields None fields.
+        payload = {'items': [{
+            'homeTeamOdds': None,
+            'awayTeamOdds': {'moneyLine': 150, 'current': None},
+        }]}
+
+        result = manager._extract_espn_data(payload)
+
+        assert result is not None
+        assert result['home_team_odds']['money_line'] is None
+        assert result['home_team_odds']['spread_odds'] is None
+        assert result['away_team_odds']['money_line'] == 150
+        assert result['away_team_odds']['spread_odds'] is None
+
+    def test_valid_empty_response_returns_none(self, manager):
+        assert manager._extract_espn_data({'count': 0, 'items': []}) is None
+
+    def test_unexpected_structure_returns_none(self, manager):
+        assert manager._extract_espn_data({'unexpected': True}) is None
+
+    def test_item_without_odds_fields_cached_as_data_not_sentinel(
+            self, manager, cache_manager, mock_get):
+        # Characterization pin: an item with no odds fields still extracts
+        # to a truthy dict of all-None values, so get_odds caches it as
+        # real data (NOT the no_odds sentinel) — but is_odds_available
+        # correctly reports False for it.
+        mock_get.return_value = _make_response({'items': [{}]})
+
+        result = manager.get_odds('football', 'nfl', '401')
+
+        assert result == {
+            'details': None,
+            'over_under': None,
+            'spread': None,
+            'home_team_odds': {'money_line': None, 'spread_odds': None},
+            'away_team_odds': {'money_line': None, 'spread_odds': None},
+        }
+        cache_manager.set.assert_called_once_with(
+            'odds_espn_football_nfl_401', result, ttl=3600)
+        assert manager.is_odds_available(result) is False
+
+
+# ---------------------------------------------------------------------------
+# is_odds_available
+# ---------------------------------------------------------------------------
+
+class TestIsOddsAvailable:
+    def test_none_is_false(self, manager):
+        assert manager.is_odds_available(None) is False
+
+    def test_empty_dict_is_false(self, manager):
+        assert manager.is_odds_available({}) is False
+
+    def test_no_odds_sentinel_is_false(self, manager):
+        assert manager.is_odds_available({'no_odds': True}) is False
+
+    def test_spread_is_true(self, manager):
+        assert manager.is_odds_available({'spread': -3.5}) is True
+
+    def test_over_under_is_true(self, manager):
+        assert manager.is_odds_available({'over_under': 47.5}) is True
+
+    def test_nested_home_spread_odds_is_true(self, manager):
+        assert manager.is_odds_available(
+            {'home_team_odds': {'spread_odds': -3.5}}) is True
+
+    def test_nested_away_spread_odds_is_true(self, manager):
+        assert manager.is_odds_available(
+            {'away_team_odds': {'spread_odds': 3.5}}) is True
+
+    def test_moneyline_only_is_false(self, manager):
+        # Pinned ML-blind contract: is_odds_available ignores money lines
+        # (its callers decide whether to render an odds widget). Note that
+        # format_odds_summary deliberately uses a DIFFERENT gate — it will
+        # still format money-line-only odds (see TestFormatOddsSummary).
+        ml_only = {
+            'home_team_odds': {'money_line': -120},
+            'away_team_odds': {'money_line': 100},
+        }
+        assert manager.is_odds_available(ml_only) is False
+
+
+# ---------------------------------------------------------------------------
+# format_odds_summary (fixed gate: empty / no_odds only)
+# ---------------------------------------------------------------------------
+
+class TestFormatOddsSummary:
+    def test_moneyline_only_formats(self, manager):
+        result = manager.format_odds_summary({
+            'home_team_odds': {'money_line': -120},
+            'away_team_odds': {'money_line': 100},
+        })
+        assert result == 'Home ML: -120 | Away ML: 100'
+
+    def test_full_data_formats_all_parts(self, manager):
+        result = manager.format_odds_summary(FULL_EXTRACTED)
+        assert result == 'Spread: -3.5 | O/U: 47.5 | Home ML: -150 | Away ML: 130'
+
+    def test_none_is_no_odds(self, manager):
+        assert manager.format_odds_summary(None) == 'No odds available'
+
+    def test_empty_dict_is_no_odds(self, manager):
+        assert manager.format_odds_summary({}) == 'No odds available'
+
+    def test_no_odds_sentinel_is_no_odds(self, manager):
+        assert manager.format_odds_summary(
+            {'no_odds': True}) == 'No odds available'
+
+
+# ---------------------------------------------------------------------------
+# get_odds_for_games
+# ---------------------------------------------------------------------------
+
+class TestGetOddsForGames:
+    def test_missing_fields_get_none_odds_without_http(self, manager, mock_get):
+        games = [
+            {'sport': 'football'},
+            {'league': 'nfl'},
+            {'id': '9'},
+            {},
+        ]
+
+        result = manager.get_odds_for_games(games)
+
+        assert all(g['odds'] is None for g in result)
+        mock_get.assert_not_called()
+
+    def test_per_game_exception_continues_loop(self, manager, monkeypatch):
+        def fake_get_odds(sport, league, event_id,
+                          update_interval_seconds=None):
+            if event_id == 'bad':
+                raise RuntimeError('boom')
+            return {'spread': -1.0}
+
+        monkeypatch.setattr(manager, 'get_odds', fake_get_odds)
+        games = [
+            {'sport': 'football', 'league': 'nfl', 'id': 'bad'},
+            {'sport': 'football', 'league': 'nfl', 'id': 'ok'},
+        ]
+
+        result = manager.get_odds_for_games(games)
+
+        assert len(result) == 2
+        assert result[0]['odds'] is None
+        assert result[1]['odds'] == {'spread': -1.0}
+
+    def test_input_dicts_mutated_in_place_and_returned(self, manager, mock_get):
+        # Pin: get_odds_for_games mutates the caller's game dicts in place
+        # and returns the same objects, not copies.
+        game = {'sport': 'football', 'league': 'nfl', 'id': '401'}
+
+        result = manager.get_odds_for_games([game])
+
+        assert result[0] is game
+        assert game['odds'] == FULL_EXTRACTED
+
+
+# ---------------------------------------------------------------------------
+# _load_configuration
+# ---------------------------------------------------------------------------
+
+class TestLoadConfiguration:
+    def test_loads_values_from_config(self, cache_manager):
+        config_manager = MagicMock()
+        config_manager.get_config.return_value = {
+            'base_odds_manager': {
+                'update_interval': 100,
+                'timeout': 5,
+                'cache_ttl': 42,
+            }
+        }
+
+        manager = BaseOddsManager(cache_manager, config_manager=config_manager)
+
+        assert manager.update_interval == 100
+        # Key/attr mismatch pin: the config key is 'timeout' but the
+        # attribute is request_timeout.
+        assert manager.request_timeout == 5
+        assert manager.cache_ttl == 42
+
+    def test_get_config_raising_keeps_defaults(self, cache_manager):
+        config_manager = MagicMock()
+        config_manager.get_config.side_effect = RuntimeError('boom')
+
+        manager = BaseOddsManager(cache_manager, config_manager=config_manager)
+
+        assert manager.update_interval == 3600
+        assert manager.request_timeout == 30
+        assert manager.cache_ttl == 1800

test/test_base_plugin_duration.py

@@ -78,10 +78,20 @@ class TestInstanceVariable:
                              instance_duration=[30])
         assert plugin.get_display_duration() == 20.0
 
-    def test_bool_true_is_one_second(self):
+    def test_bool_true_falls_through_like_any_non_number(self):
-        # Characterized quirk: bool is an int subclass, so display_duration =
+        # bool is an int subclass, but a boolean is not a duration: True
-        # True passes the isinstance((int, float)) branch and returns 1.0.
+        # must NOT read as 1 second — it falls through to config/default.
-        assert make_plugin(instance_duration=True).get_display_duration() == 1.0
+        assert make_plugin(instance_duration=True).get_display_duration() == 15.0
+
+    def test_bool_true_falls_through_to_config(self):
+        plugin = make_plugin(config={"display_duration": 20},
+                             instance_duration=True)
+        assert plugin.get_display_duration() == 20.0
+
+    def test_bool_false_still_falls_through(self):
+        plugin = make_plugin(config={"display_duration": 20},
+                             instance_duration=False)
+        assert plugin.get_display_duration() == 20.0
 
 
 class TestConfigFallback:
@@ -108,3 +118,29 @@ class TestConfigFallback:
 
     def test_config_none_uses_default(self):
         assert make_plugin({"display_duration": None}).get_display_duration() == 15.0
+
+    def test_config_bool_uses_default(self):
+        assert make_plugin({"display_duration": True}).get_display_duration() == 15.0
+        assert make_plugin({"display_duration": False}).get_display_duration() == 15.0
+
+
+class TestValidateConfigDuration:
+    # validate_config must agree with get_display_duration about what a
+    # valid duration is — a config it accepts must not then be rejected
+    # (or silently defaulted) when the duration is actually read.
+
+    def test_positive_number_valid(self):
+        assert make_plugin({"display_duration": 20}).validate_config() is True
+
+    def test_zero_and_negative_invalid(self):
+        assert make_plugin({"display_duration": 0}).validate_config() is False
+        assert make_plugin({"display_duration": -5}).validate_config() is False
+
+    def test_bool_invalid(self):
+        # bool is an int subclass; True would otherwise pass as "positive
+        # number" here while get_display_duration rejects it.
+        assert make_plugin({"display_duration": True}).validate_config() is False
+        assert make_plugin({"display_duration": False}).validate_config() is False
+
+    def test_missing_duration_valid(self):
+        assert make_plugin({}).validate_config() is True

test/test_config_helper.py

@@ -0,0 +1,253 @@
+"""
+Tests for src/common/config_helper.py — pins the ConfigHelper contract.
+
+Covers: load/save round trips (missing/malformed files return {} rather
+than raising, non-ASCII preserved via ensure_ascii=False, top-level JSON
+lists returned as-is), dot-notation get/set including the silent-failure
+contract when an intermediate key holds a non-dict, merge_configs deep
+semantics with NO aliasing of the base config (the fixed bug — the old
+shallow copy let mutations of the merged result leak into base's nested
+dicts), simplified schema validation including the caught-TypeError path
+when a schema 'type' is given as a string, plugin config key conventions
+('{plugin_id}_config', enabled defaults True), and required-key checks
+where a key present with value None counts as present.
+"""
+
+import json
+
+import pytest
+
+from src.common.config_helper import ConfigHelper
+
+
+@pytest.fixture
+def helper():
+    return ConfigHelper()
+
+
+class TestLoadConfig:
+    def test_missing_file_returns_empty_dict(self, helper, tmp_path):
+        assert helper.load_config(tmp_path / "nope.json") == {}
+
+    def test_malformed_json_returns_empty_dict(self, helper, tmp_path):
+        path = tmp_path / "bad.json"
+        path.write_text("{ this is not json", encoding="utf-8")
+        assert helper.load_config(path) == {}
+
+    def test_top_level_list_returned_as_is(self, helper, tmp_path):
+        # load_config does not enforce a dict shape: a JSON list comes
+        # straight back. Pinned as a characterization of current behavior.
+        path = tmp_path / "list.json"
+        path.write_text("[1, 2, 3]", encoding="utf-8")
+        assert helper.load_config(path) == [1, 2, 3]
+
+
+class TestSaveConfig:
+    def test_round_trip(self, helper, tmp_path):
+        path = tmp_path / "config.json"
+        config = {'display': {'hardware': {'rows': 32}}, 'timezone': 'UTC'}
+        assert helper.save_config(config, path) is True
+        assert helper.load_config(path) == config
+
+    def test_creates_parent_directories(self, helper, tmp_path):
+        path = tmp_path / "deep" / "nested" / "config.json"
+        assert helper.save_config({'a': 1}, path) is True
+        assert path.exists()
+        assert helper.load_config(path) == {'a': 1}
+
+    def test_non_ascii_survives_round_trip(self, helper, tmp_path):
+        path = tmp_path / "config.json"
+        config = {'city': 'Zürich', 'note': 'météo ☀'}
+        assert helper.save_config(config, path) is True
+        assert helper.load_config(path) == config
+        # ensure_ascii=False: characters are written raw, not \u-escaped
+        assert 'Zürich' in path.read_text(encoding='utf-8')
+
+    def test_directory_path_returns_false_not_raise(self, helper, tmp_path):
+        assert helper.save_config({'a': 1}, tmp_path) is False
+
+
+class TestGetConfigValue:
+    def test_dot_notation_hit(self, helper):
+        config = {'display': {'hardware': {'rows': 32}}}
+        assert helper.get_config_value(config, 'display.hardware.rows') == 32
+
+    def test_missing_returns_default(self, helper):
+        sentinel = object()
+        assert helper.get_config_value({}, 'display.rows', default=sentinel) is sentinel
+
+    def test_intermediate_non_dict_returns_default(self, helper):
+        config = {'display': 'not-a-dict'}
+        assert helper.get_config_value(config, 'display.hardware.rows', default=64) == 64
+
+    def test_required_missing_raises_keyerror(self, helper):
+        with pytest.raises(KeyError):
+            helper.get_config_value({}, 'display.rows', required=True)
+
+
+class TestSetConfigValue:
+    def test_sets_top_level(self, helper):
+        config = {}
+        helper.set_config_value(config, 'timezone', 'UTC')
+        assert config == {'timezone': 'UTC'}
+
+    def test_auto_creates_intermediates(self, helper):
+        config = {}
+        helper.set_config_value(config, 'display.hardware.rows', 32)
+        assert config == {'display': {'hardware': {'rows': 32}}}
+
+    def test_silent_failure_on_non_dict_intermediate(self, helper):
+        # 'a' exists but holds an int; the assignment attempt raises
+        # TypeError internally, which set_config_value swallows and logs.
+        # The config is left unchanged — pinned silent-failure contract.
+        config = {'a': 5}
+        helper.set_config_value(config, 'a.b', 1)
+        assert config == {'a': 5}
+
+
+class TestMergeConfigs:
+    def test_nested_dicts_merge_recursively(self, helper):
+        base = {'display': {'rows': 32, 'cols': 64}, 'timezone': 'UTC'}
+        override = {'display': {'cols': 128, 'brightness': 90}}
+        merged = helper.merge_configs(base, override)
+        assert merged == {
+            'display': {'rows': 32, 'cols': 128, 'brightness': 90},
+            'timezone': 'UTC',
+        }
+
+    def test_scalar_override_wins_over_dict(self, helper):
+        merged = helper.merge_configs({'display': {'rows': 32}}, {'display': 7})
+        assert merged['display'] == 7
+
+    def test_dict_override_wins_over_scalar(self, helper):
+        merged = helper.merge_configs({'display': 7}, {'display': {'rows': 32}})
+        assert merged['display'] == {'rows': 32}
+
+    def test_no_aliasing_of_base(self, helper):
+        # Post-fix: merge deep-copies base, so mutating the result never
+        # leaks back into the caller's base config.
+        base = {'display': {'x': 1}}
+        merged = helper.merge_configs(base, {})
+        assert merged['display'] is not base['display']
+        merged['display']['x'] = 99
+        assert base['display']['x'] == 1
+
+    def test_inputs_unchanged(self, helper):
+        base = {'a': {'b': 1}}
+        override = {'a': {'c': 2}}
+        helper.merge_configs(base, override)
+        assert base == {'a': {'b': 1}}
+        assert override == {'a': {'c': 2}}
+
+    def test_no_aliasing_of_override_values(self, helper):
+        # The non-recursive branch must deep-copy the override value too:
+        # mutating a merged-in list or dict must not reach back into
+        # override_config.
+        override = {'teams': ['A', 'B'], 'nested': {'x': [1]}}
+        merged = helper.merge_configs({}, override)
+        merged['teams'].append('C')
+        merged['nested']['x'].append(2)
+        assert override == {'teams': ['A', 'B'], 'nested': {'x': [1]}}
+
+
+class TestValidateConfig:
+    def test_no_schema_dict_is_valid(self, helper):
+        assert helper.validate_config({'a': 1}) is True
+
+    def test_no_schema_list_is_invalid(self, helper):
+        assert helper.validate_config([1, 2]) is False
+
+    def test_required_key_missing_is_invalid(self, helper):
+        schema = {'rows': {'required': True, 'type': int}}
+        assert helper.validate_config({}, schema) is False
+
+    def test_optional_key_missing_is_valid(self, helper):
+        schema = {'rows': {'required': False, 'type': int}}
+        assert helper.validate_config({}, schema) is True
+
+    def test_wrong_type_is_invalid(self, helper):
+        schema = {'rows': {'type': int}}
+        assert helper.validate_config({'rows': 'thirty-two'}, schema) is False
+        assert helper.validate_config({'rows': 32}, schema) is True
+
+    def test_allowed_values_violation_is_invalid(self, helper):
+        schema = {'mode': {'allowed_values': ['clock', 'weather']}}
+        assert helper.validate_config({'mode': 'stocks'}, schema) is False
+        assert helper.validate_config({'mode': 'clock'}, schema) is True
+
+    def test_string_type_in_schema_is_invalid_via_typeerror(self, helper):
+        # 'type' given as the STRING "int" makes isinstance() raise
+        # TypeError; validate_config catches it and returns False rather
+        # than raising. Pinned characterization.
+        schema = {'rows': {'type': 'int'}}
+        assert helper.validate_config({'rows': 32}, schema) is False
+
+
+class TestPluginConfigHelpers:
+    def test_get_plugin_config_uses_suffixed_key(self, helper):
+        plugin_cfg = {'enabled': True, 'display_duration': 30}
+        assert helper.get_plugin_config({'clock_config': plugin_cfg}, 'clock') == plugin_cfg
+
+    def test_get_plugin_config_bare_id_key_not_found(self, helper):
+        # Only '{plugin_id}_config' is consulted — a bare 'clock' section
+        # is invisible to this helper. Pinned key contract.
+        assert helper.get_plugin_config({'clock': {'enabled': True}}, 'clock') == {}
+
+    def test_create_default_config_wraps_in_suffixed_key(self, helper):
+        defaults = {'enabled': True}
+        assert helper.create_default_config('clock', defaults) == {'clock_config': defaults}
+
+    def test_is_plugin_enabled_defaults_true_for_unknown(self, helper):
+        assert helper.is_plugin_enabled({}, 'clock') is True
+
+    def test_is_plugin_enabled_false_when_disabled(self, helper):
+        config = {'clock_config': {'enabled': False}}
+        assert helper.is_plugin_enabled(config, 'clock') is False
+
+    def test_is_plugin_enabled_ignores_bare_id_key(self, helper):
+        # Disabled under the wrong key -> still reported enabled (default).
+        config = {'clock': {'enabled': False}}
+        assert helper.is_plugin_enabled(config, 'clock') is True
+
+
+class TestSportsAndDisplayHelpers:
+    def test_get_display_config(self, helper):
+        display = {'hardware': {'rows': 32}}
+        assert helper.get_display_config({'display': display}) == display
+        assert helper.get_display_config({}) == {}
+
+    def test_get_sports_config_uses_scoreboard_suffix(self, helper):
+        sport_cfg = {'favorite_teams': ['TB']}
+        config = {'football_scoreboard': sport_cfg}
+        assert helper.get_sports_config(config, 'football') == sport_cfg
+        assert helper.get_sports_config(config, 'hockey') == {}
+
+    def test_get_favorite_teams(self, helper):
+        config = {'football_scoreboard': {'favorite_teams': ['TB', 'DAL']}}
+        assert helper.get_favorite_teams(config, 'football') == ['TB', 'DAL']
+        assert helper.get_favorite_teams({}, 'football') == []
+
+    def test_get_display_modes(self, helper):
+        modes = {'live': True, 'recent': False}
+        config = {'football_scoreboard': {'display_modes': modes}}
+        assert helper.get_display_modes(config, 'football') == modes
+        assert helper.get_display_modes({}, 'football') == {}
+
+
+class TestValidateRequiredKeys:
+    def test_returns_missing_subset(self, helper):
+        config = {'a': 1, 'c': {'d': 2}}
+        missing = helper.validate_required_keys(config, ['a', 'b', 'c.d', 'c.e'])
+        assert missing == ['b', 'c.e']
+
+    def test_dot_notation_present(self, helper):
+        config = {'display': {'hardware': {'rows': 32}}}
+        assert helper.validate_required_keys(config, ['display.hardware.rows']) == []
+
+    def test_empty_requirements(self, helper):
+        assert helper.validate_required_keys({'a': 1}, []) == []
+
+    def test_present_with_none_counts_as_present(self, helper):
+        # _has_key checks key membership, not truthiness — a key set to
+        # None is NOT reported missing. Pinned semantics.
+        assert helper.validate_required_keys({'a': None}, ['a']) == []

test/test_config_manager_secrets.py

@@ -177,3 +177,159 @@ class TestLoadFastPath:
         os.utime(config_file, ns=(1_000_000_000, 1_000_000_000))
 
         assert manager.load_config()["timezone"] == "AAA"  # stale, by design
+
+
+class TestArraySecretStripAndMerge:
+    """Array-item secrets round-trip (parallel-placeholder lists).
+
+    secret_helpers.separate_secrets emits array secrets as a list parallel
+    to the regular list, with {} for items that carry no secrets. Strip
+    must remove the secret fields from config.json while preserving item
+    indices; load must merge them back into the right items. The regular
+    list's length is authoritative in both directions.
+    """
+
+    def test_strip_removes_array_item_secrets_keeps_indices(self, tmp_path):
+        manager = make_manager(tmp_path)
+        data = {"plugin": {"accounts": [
+            {"name": "a", "token": "ta"},
+            {"name": "b"},
+        ]}}
+        secrets = {"plugin": {"accounts": [{"token": "ta"}, {}]}}
+        stripped = manager._strip_secrets_recursive(data, secrets)
+        assert stripped == {"plugin": {"accounts": [{"name": "a"}, {"name": "b"}]}}
+
+    def test_strip_keeps_all_placeholder_items(self, tmp_path):
+        # Even when every item strips to nothing extra, the list survives
+        # with its indices — required for merge-on-load alignment.
+        manager = make_manager(tmp_path)
+        data = {"accounts": [{"token": "t1"}, {"token": "t2"}]}
+        secrets = {"accounts": [{"token": "t1"}, {"token": "t2"}]}
+        stripped = manager._strip_secrets_recursive(data, secrets)
+        assert stripped == {"accounts": [{}, {}]}
+
+    def test_strip_whole_scalar_array_secret_drops_key(self, tmp_path):
+        # A list of secret scalars is a whole-key secret, not the parallel
+        # shape — the key must vanish from config.json entirely.
+        manager = make_manager(tmp_path)
+        data = {"recovery_codes": ["a", "b"], "city": "Austin"}
+        secrets = {"recovery_codes": ["a", "b"]}
+        stripped = manager._strip_secrets_recursive(data, secrets)
+        assert stripped == {"city": "Austin"}
+
+    def test_strip_shape_mismatch_drops_key(self, tmp_path):
+        # Conservative contract: if the shapes disagree, never leak.
+        manager = make_manager(tmp_path)
+        data = {"accounts": {"name": "not-a-list"}}
+        secrets = {"accounts": [{"token": "t"}]}
+        stripped = manager._strip_secrets_recursive(data, secrets)
+        assert stripped == {}
+
+    def test_strip_ignores_extra_secrets_entries(self, tmp_path):
+        # Regular list length is authoritative: a user deleted an item.
+        manager = make_manager(tmp_path)
+        data = {"accounts": [{"name": "a", "token": "ta"}]}
+        secrets = {"accounts": [{"token": "ta"}, {"token": "tb"}]}
+        stripped = manager._strip_secrets_recursive(data, secrets)
+        assert stripped == {"accounts": [{"name": "a"}]}
+
+    def test_merge_restores_array_item_secrets(self, tmp_path):
+        manager = make_manager(tmp_path)
+        target = {"accounts": [{"name": "a"}, {"name": "b"}]}
+        manager._deep_merge(target, {"accounts": [{"token": "ta"}, {}]})
+        assert target == {"accounts": [
+            {"name": "a", "token": "ta"},
+            {"name": "b"},
+        ]}
+
+    def test_merge_ignores_extra_secrets_entries_with_warning(self, tmp_path, caplog):
+        manager = make_manager(tmp_path)
+        target = {"accounts": [{"name": "a"}]}
+        with caplog.at_level("WARNING"):
+            manager._deep_merge(
+                target, {"accounts": [{"token": "ta"}, {"token": "ghost"}]})
+        assert target == {"accounts": [{"name": "a", "token": "ta"}]}
+        assert any("longer than the config list" in r.message for r in caplog.records)
+
+    def test_merge_non_dict_item_replaced_by_secret(self, tmp_path):
+        # Shape drift inside the list: the secret wins for that index.
+        manager = make_manager(tmp_path)
+        target = {"accounts": ["oddball", {"name": "b"}]}
+        manager._deep_merge(target, {"accounts": [{"token": "ta"}, {}]})
+        assert target == {"accounts": [{"token": "ta"}, {"name": "b"}]}
+
+    def test_merge_whole_scalar_array_still_replaces(self, tmp_path):
+        # Legacy behavior preserved: a non-parallel list replaces wholesale.
+        manager = make_manager(tmp_path)
+        target = {"recovery_codes": ["old"]}
+        manager._deep_merge(target, {"recovery_codes": ["new1", "new2"]})
+        assert target == {"recovery_codes": ["new1", "new2"]}
+
+    def test_full_save_load_round_trip(self, tmp_path):
+        # End to end on real files: save strips array secrets out of
+        # config.json; load merges them back into the right items.
+        manager = make_manager(
+            tmp_path,
+            config={"plugin": {"accounts": [
+                {"name": "a", "token": "s3cret-a"},
+                {"name": "b", "token": "s3cret-b"},
+            ]}},
+            secrets={"plugin": {"accounts": [
+                {"token": "s3cret-a"}, {"token": "s3cret-b"},
+            ]}},
+        )
+        loaded = manager.load_config()
+        assert loaded["plugin"]["accounts"][0]["token"] == "s3cret-a"
+
+        manager.save_config(loaded)
+
+        raw = (tmp_path / "config.json").read_text()
+        assert "s3cret" not in raw
+        on_disk = json.loads(raw)
+        assert on_disk["plugin"]["accounts"] == [{"name": "a"}, {"name": "b"}]
+
+        # A fresh manager (constructed directly — make_manager would
+        # overwrite the just-saved config.json) re-merges from the secrets
+        # file on load.
+        fresh = ConfigManager(config_path=str(tmp_path / "config.json"),
+                              secrets_path=str(tmp_path / "config_secrets.json"))
+        fresh.template_path = str(tmp_path / "no-template.json")
+        reloaded = fresh.load_config()
+        assert reloaded["plugin"]["accounts"] == [
+            {"name": "a", "token": "s3cret-a"},
+            {"name": "b", "token": "s3cret-b"},
+        ]
+
+    def test_whole_item_secret_list_never_leaks_values(self, tmp_path):
+        # When the ENTIRE array item is secret (schema marks both key[]
+        # and key[].field), separate_secrets stores the full item dicts in
+        # the secrets file. That shape also matches the parallel-list
+        # discriminator — which is safe: strip drops every leaf key that
+        # appears in the secret item, so only empty {} skeletons (item
+        # count, no values) can reach config.json, and merge-on-load
+        # restores the full items from those skeletons.
+        from src.web_interface.secret_helpers import (
+            find_secret_fields, separate_secrets)
+        schema_props = {"accounts": {
+            "type": "array",
+            "items": {"type": "object", "x-secret": True, "properties": {
+                "id": {"type": "string"},
+                "token": {"type": "string", "x-secret": True},
+            }},
+        }}
+        paths = find_secret_fields(schema_props)
+        assert paths == {"accounts[]", "accounts[].token"}
+        full = {"accounts": [{"id": "i1", "token": "s3cret-a"},
+                             {"id": "i2", "token": "s3cret-b"}]}
+        _, secrets = separate_secrets(full, paths)
+        assert secrets == full  # whole items are secret
+
+        manager = make_manager(tmp_path)
+        stripped = manager._strip_secrets_recursive(full, secrets)
+        assert stripped == {"accounts": [{}, {}]}
+
+        raw = json.dumps(stripped)
+        assert "s3cret" not in raw and "i1" not in raw
+
+        manager._deep_merge(stripped, secrets)
+        assert stripped == full  # round trip restores the items

test/test_display_controller.py

@@ -338,31 +338,48 @@ class TestDisplayControllerSchedule:
     """Test schedule management."""
     
     def test_schedule_disabled(self, test_display_controller):
-        """Test when schedule is disabled."""
+        """schedule.enabled=False keeps the display active even outside the
+        configured window. (This test used to patch config_service, which
+        _check_schedule never reads — it asserted the init default.)"""
         controller = test_display_controller
-        schedule_config = {"schedule": {"enabled": False}}
+        controller.config['schedule'] = {
-        with patch.object(controller.config_service, 'get_config', return_value=schedule_config):
+            "enabled": False,
+            "start_time": "09:00",
+            "end_time": "17:00",
+        }
+        controller._schedule_checked_minute = None
+        controller._tz = None
+        controller.is_display_active = False  # prove the method flips it back
+
+        with patch('src.display_controller.datetime') as mock_datetime:
+            mock_datetime.now.return_value.strftime.return_value.lower.return_value = "monday"
+            mock_datetime.now.return_value.time.return_value = datetime.strptime("20:00", "%H:%M").time()
+            mock_datetime.strptime = datetime.strptime
+
             controller._check_schedule()
             assert controller.is_display_active is True
 
     def test_active_hours(self, test_display_controller):
-        """Test active hours check."""
+        """A time inside the window activates the display. (This test used
+        to patch config_service, which _check_schedule never reads — it
+        asserted the init default.)"""
         controller = test_display_controller
+        controller.config['schedule'] = {
+            "enabled": True,
+            "start_time": "09:00",
+            "end_time": "17:00",
+        }
+        controller._schedule_checked_minute = None
+        controller._tz = None
+        controller.is_display_active = False  # prove the method flips it on
+
         with patch('src.display_controller.datetime') as mock_datetime:
             mock_datetime.now.return_value.strftime.return_value.lower.return_value = "monday"
             mock_datetime.now.return_value.time.return_value = datetime.strptime("12:00", "%H:%M").time()
             mock_datetime.strptime = datetime.strptime
 
-            schedule_config = {
+            controller._check_schedule()
-                "schedule": {
+            assert controller.is_display_active is True
-                    "enabled": True,
-                    "start_time": "09:00",
-                    "end_time": "17:00"
-                }
-            }
-            with patch.object(controller.config_service, 'get_config', return_value=schedule_config):
-                controller._check_schedule()
-                assert controller.is_display_active is True
 
     def test_inactive_hours(self, test_display_controller):
         """Test inactive hours check."""

test/test_display_controller_schedule.py

@@ -0,0 +1,277 @@
+"""
+Behavioral tests for DisplayController._check_schedule and
+_check_dim_schedule — the on/off window and night-dimming logic.
+
+test_display_controller_optimizations.py::TestScheduleMinuteGate already
+covers the once-per-minute gating; this file covers what it doesn't:
+midnight-crossing windows, mode selection (global / per-day / legacy
+inference), per-day disabled days, invalid time strings, unknown
+timezones, boundary equality, and the transition-tracking flags.
+
+Both methods read only self.config and a handful of instance attributes,
+so a bare stub via object.__new__ (the test_display_controller_vegas_tick
+pattern) is enough — no managers needed.
+"""
+
+import os
+from datetime import datetime
+
+from unittest.mock import patch
+
+import pytest
+
+os.environ.setdefault("EMULATOR", "true")
+
+from src.display_controller import DisplayController  # noqa: E402
+
+
+def make_controller(config=None, *, normal_brightness=90):
+    dc = object.__new__(DisplayController)
+    dc.config = config or {}
+    dc._tz = None
+    dc._schedule_checked_minute = None
+    dc.is_display_active = True
+    dc._was_display_active = True
+    dc._normal_brightness = normal_brightness
+    dc._dim_checked_minute = None
+    dc._cached_target_brightness = None
+    dc.is_dimmed = False
+    dc._was_dimmed = False
+    return dc
+
+
+def at(time_str, day="monday"):
+    """Context manager patching the controller module's clock."""
+    patcher = patch("src.display_controller.datetime")
+    mock_dt = patcher.start()
+    mock_dt.strptime = datetime.strptime
+    mock_dt.now.return_value.time.return_value = (
+        datetime.strptime(time_str, "%H:%M").time())
+    mock_dt.now.return_value.strftime.return_value.lower.return_value = day
+    mock_dt.now.return_value.hour = int(time_str.split(":")[0])
+    mock_dt.now.return_value.minute = int(time_str.split(":")[1])
+    return patcher
+
+
+@pytest.fixture
+def clock():
+    patchers = []
+
+    def _at(time_str, day="monday"):
+        patchers.append(p := at(time_str, day))
+        return p
+
+    yield _at
+    for p in patchers:
+        p.stop()
+
+
+def check_at(dc, time_str, day="monday", clock=None):
+    """Run _check_schedule at a mocked wall time, resetting the minute gate."""
+    dc._schedule_checked_minute = None
+    p = at(time_str, day)
+    try:
+        dc._check_schedule()
+    finally:
+        p.stop()
+    return dc.is_display_active
+
+
+def dim_at(dc, time_str, day="monday"):
+    dc._dim_checked_minute = None
+    p = at(time_str, day)
+    try:
+        return dc._check_dim_schedule()
+    finally:
+        p.stop()
+
+
+class TestScheduleWindows:
+    def _config(self, start, end, **extra):
+        return {"schedule": {"enabled": True, "start_time": start,
+                             "end_time": end, **extra},
+                "timezone": "UTC"}
+
+    def test_same_day_window(self):
+        dc = make_controller(self._config("09:00", "17:00"))
+        assert check_at(dc, "12:00") is True
+        assert check_at(dc, "20:00") is False
+        assert check_at(dc, "08:59") is False
+
+    def test_boundaries_are_inclusive(self):
+        dc = make_controller(self._config("09:00", "17:00"))
+        assert check_at(dc, "09:00") is True   # now == start
+        assert check_at(dc, "17:00") is True   # now == end
+
+    def test_midnight_crossing_window(self):
+        # 21:00 -> 07:00: active late evening AND early morning, inactive
+        # mid-day.
+        dc = make_controller(self._config("21:00", "07:00"))
+        assert check_at(dc, "23:00") is True
+        assert check_at(dc, "03:00") is True
+        assert check_at(dc, "12:00") is False
+        assert check_at(dc, "21:00") is True   # boundary
+        assert check_at(dc, "07:00") is True   # boundary
+
+    def test_no_schedule_config_is_always_active(self):
+        dc = make_controller({"timezone": "UTC"})
+        dc.is_display_active = False
+        dc._check_schedule()
+        assert dc.is_display_active is True
+
+    def test_invalid_time_string_falls_back_to_active(self):
+        dc = make_controller(self._config("9 o'clock", "17:00"))
+        dc.is_display_active = False
+        assert check_at(dc, "03:00") is True  # ValueError -> stay on
+
+    def test_unknown_timezone_falls_back_to_utc(self):
+        dc = make_controller({"schedule": {"enabled": True,
+                                           "start_time": "09:00",
+                                           "end_time": "17:00"},
+                              "timezone": "Mars/Olympus_Mons"})
+        assert check_at(dc, "12:00") is True
+        import pytz
+        assert dc._tz is pytz.UTC
+
+
+class TestScheduleModes:
+    DAYS = {
+        "monday": {"enabled": True, "start_time": "10:00",
+                   "end_time": "18:00"},
+        "tuesday": {"enabled": False},
+    }
+
+    def test_global_mode_ignores_days(self):
+        dc = make_controller({"schedule": {
+            "enabled": True, "mode": "global",
+            "start_time": "09:00", "end_time": "17:00",
+            "days": self.DAYS}, "timezone": "UTC"})
+        # 09:30 is inside the global window but outside monday's per-day one.
+        assert check_at(dc, "09:30", day="monday") is True
+
+    def test_per_day_mode_uses_day_window(self):
+        dc = make_controller({"schedule": {
+            "enabled": True, "mode": "per-day",
+            "start_time": "09:00", "end_time": "17:00",
+            "days": self.DAYS}, "timezone": "UTC"})
+        assert check_at(dc, "09:30", day="monday") is False  # before 10:00
+        assert check_at(dc, "12:00", day="monday") is True
+
+    def test_per_day_underscore_spelling_accepted(self):
+        dc = make_controller({"schedule": {
+            "enabled": True, "mode": "per_day",
+            "days": self.DAYS}, "timezone": "UTC"})
+        assert check_at(dc, "12:00", day="monday") is True
+
+    def test_legacy_no_mode_infers_per_day_from_days_config(self):
+        dc = make_controller({"schedule": {
+            "enabled": True,
+            "start_time": "09:00", "end_time": "17:00",
+            "days": self.DAYS}, "timezone": "UTC"})
+        assert check_at(dc, "09:30", day="monday") is False  # per-day won
+
+    def test_per_day_disabled_day_turns_display_off(self):
+        dc = make_controller({"schedule": {
+            "enabled": True, "mode": "per-day",
+            "days": self.DAYS}, "timezone": "UTC"})
+        assert check_at(dc, "12:00", day="tuesday") is False
+
+    def test_per_day_missing_day_falls_back_to_global(self):
+        dc = make_controller({"schedule": {
+            "enabled": True, "mode": "per-day",
+            "start_time": "09:00", "end_time": "17:00",
+            "days": self.DAYS}, "timezone": "UTC"})
+        # Wednesday has no per-day entry -> global window applies.
+        assert check_at(dc, "09:30", day="wednesday") is True
+
+    def test_missing_enabled_key_means_enabled(self):
+        # Backward compat: schedules written before the enabled flag.
+        dc = make_controller({"schedule": {
+            "start_time": "09:00", "end_time": "17:00"}, "timezone": "UTC"})
+        assert check_at(dc, "20:00") is False
+
+
+class TestScheduleTransitions:
+    def test_was_display_active_tracks_state(self):
+        dc = make_controller({"schedule": {"enabled": True,
+                                           "start_time": "09:00",
+                                           "end_time": "17:00"},
+                              "timezone": "UTC"})
+        check_at(dc, "12:00")
+        assert dc._was_display_active is True
+        check_at(dc, "20:00")
+        assert dc._was_display_active is False
+        check_at(dc, "12:05")
+        assert dc._was_display_active is True
+
+
+class TestDimSchedule:
+    def _config(self, start="20:00", end="07:00", **extra):
+        return {"dim_schedule": {"enabled": True, "start_time": start,
+                                 "end_time": end, "dim_brightness": 25,
+                                 **extra},
+                "timezone": "UTC"}
+
+    def test_disabled_by_default(self):
+        dc = make_controller({"dim_schedule": {"start_time": "20:00",
+                                               "end_time": "07:00"},
+                              "timezone": "UTC"})
+        # Unlike the on/off schedule, dimming defaults to DISABLED when the
+        # enabled key is missing.
+        assert dim_at(dc, "23:00") == 90
+        assert dc.is_dimmed is False
+
+    def test_overnight_dim_window(self):
+        dc = make_controller(self._config())
+        assert dim_at(dc, "23:00") == 25
+        assert dc.is_dimmed is True
+        assert dim_at(dc, "03:00") == 25
+        assert dim_at(dc, "12:00") == 90
+        assert dc.is_dimmed is False
+
+    def test_dim_brightness_defaults_to_30(self):
+        dc = make_controller({"dim_schedule": {"enabled": True,
+                                               "start_time": "20:00",
+                                               "end_time": "07:00"},
+                              "timezone": "UTC"})
+        assert dim_at(dc, "23:00") == 30
+
+    def test_inactive_display_short_circuits_undimmed(self):
+        dc = make_controller(self._config())
+        dc.is_display_active = False
+        dc.is_dimmed = True
+        assert dim_at(dc, "23:00") == 90
+        assert dc.is_dimmed is False
+
+    def test_per_day_mode(self):
+        dc = make_controller(self._config(mode="per-day", days={
+            "monday": {"enabled": True, "start_time": "22:00",
+                       "end_time": "06:00"},
+            "tuesday": {"enabled": False},
+        }))
+        assert dim_at(dc, "23:00", day="monday") == 25
+        assert dim_at(dc, "21:00", day="monday") == 90  # before per-day start
+        assert dim_at(dc, "23:00", day="tuesday") == 90  # day disabled
+        assert dc.is_dimmed is False
+
+    def test_no_legacy_inference_for_dim(self):
+        # Unlike _check_schedule, dim mode defaults to GLOBAL even when a
+        # days config exists — no legacy inference.
+        dc = make_controller(self._config(days={
+            "monday": {"enabled": True, "start_time": "22:00",
+                       "end_time": "06:00"},
+        }))
+        # 21:00 is inside the global 20:00-07:00 window but outside monday's
+        # per-day 22:00 start; global mode wins.
+        assert dim_at(dc, "21:00", day="monday") == 25
+
+    def test_invalid_time_string_returns_normal(self):
+        dc = make_controller(self._config(start="late"))
+        assert dim_at(dc, "23:00") == 90
+
+    def test_was_dimmed_tracks_transitions(self):
+        dc = make_controller(self._config())
+        dim_at(dc, "23:00")
+        assert dc._was_dimmed is True
+        dim_at(dc, "12:00")
+        assert dc._was_dimmed is False

test/test_display_helper.py

@@ -0,0 +1,307 @@
+"""Tests for src/common/display_helper.py (DisplayHelper).
+
+Pure-PIL tests, no hardware or mocks required. Pixel assertions rely on
+getbbox()/getpixel() rather than exact text pixel counts, because the
+default-font metrics vary across Pillow versions.
+
+These tests pin the FIXED behaviors on this branch:
+- draw_error_message / draw_no_data_message return a rendered image
+  (they previously crashed with AttributeError),
+- draw_scorebug_layout draws period/status/clock as one combined top
+  line (previously overprinted at the same y),
+- draw_ticker_layout draws at x=0 (previously started at
+  x=display_width, i.e. entirely off-canvas -> blank frames).
+"""
+
+from PIL import Image, ImageDraw, ImageFont
+
+from src.common.display_helper import DisplayHelper
+
+
+def default_font():
+    return ImageFont.load_default()
+
+
+def make_helper(width=128, height=32):
+    return DisplayHelper(width, height)
+
+
+class TestCreateBaseImage:
+    def test_default_is_black_rgb_display_sized(self):
+        helper = make_helper()
+        img = helper.create_base_image()
+        assert img.size == (128, 32)
+        assert img.mode == 'RGB'
+        assert img.getpixel((0, 0)) == (0, 0, 0)
+        assert img.getpixel((127, 31)) == (0, 0, 0)
+        # Entirely black -> no bounding box in luminance
+        assert img.convert('L').getbbox() is None
+
+    def test_custom_background_color(self):
+        helper = make_helper()
+        img = helper.create_base_image(background_color=(10, 20, 30))
+        assert img.getpixel((0, 0)) == (10, 20, 30)
+        assert img.getpixel((64, 16)) == (10, 20, 30)
+
+    def test_mode_rgba_is_honored(self):
+        helper = make_helper()
+        img = helper.create_base_image(mode='RGBA')
+        assert img.mode == 'RGBA'
+        assert img.size == (128, 32)
+
+
+class TestCreateOverlay:
+    def test_overlay_is_transparent_rgba(self):
+        helper = make_helper()
+        overlay = helper.create_overlay()
+        assert overlay.mode == 'RGBA'
+        assert overlay.size == (128, 32)
+        assert overlay.getpixel((0, 0)) == (0, 0, 0, 0)
+        assert overlay.getpixel((127, 31)) == (0, 0, 0, 0)
+
+
+class TestCompositeImages:
+    def test_rgb_inputs_are_upconverted_and_result_is_rgba(self):
+        helper = make_helper()
+        base = Image.new('RGB', (128, 32), (0, 0, 0))
+        overlay = Image.new('RGB', (128, 32), (255, 0, 0))
+        result = helper.composite_images(base, overlay)
+        assert result.mode == 'RGBA'
+        assert result.size == base.size
+        # RGB->RGBA conversion yields a fully opaque overlay
+        assert result.getpixel((0, 0)) == (255, 0, 0, 255)
+
+    def test_transparent_overlay_leaves_base_visible(self):
+        helper = make_helper()
+        base = Image.new('RGB', (128, 32), (5, 6, 7))
+        overlay = helper.create_overlay()
+        result = helper.composite_images(base, overlay)
+        assert result.mode == 'RGBA'
+        assert result.getpixel((64, 16)) == (5, 6, 7, 255)
+
+
+class TestScorebugLayout:
+    def test_full_game_data_renders(self):
+        helper = make_helper()
+        font = default_font()
+        fonts = {'time': font, 'status': font, 'score': font, 'team': font}
+        game_data = {
+            'home_score': 3, 'away_score': 2,
+            'home_abbr': 'NYY', 'away_abbr': 'BOS',
+            'status_text': 'LIVE', 'period_text': 'T9', 'clock': '2:30',
+        }
+        img = helper.draw_scorebug_layout(game_data, fonts)
+        assert img.mode == 'RGB'
+        assert img.size == (128, 32)
+        assert img.convert('L').getbbox() is not None
+
+    def test_empty_game_data_uses_defaults_without_raising(self):
+        helper = make_helper()
+        font = default_font()
+        fonts = {'time': font, 'status': font, 'score': font, 'team': font}
+        img = helper.draw_scorebug_layout({}, fonts)
+        assert img.mode == 'RGB'
+        assert img.size == (128, 32)
+        # Defaults '0'/'HOME'/'AWAY' actually render something
+        assert img.convert('L').getbbox() is not None
+
+    def test_empty_fonts_dict_falls_back_to_default_font(self):
+        # Pin: fonts={} must not raise — PIL falls back to the default
+        # font when font=None is passed through.
+        helper = make_helper()
+        img = helper.draw_scorebug_layout(
+            {'status_text': 'FINAL', 'period_text': 'Q4', 'clock': '0:00'}, {})
+        assert img.size == (128, 32)
+        assert img.convert('L').getbbox() is not None
+
+    def test_top_line_is_one_combined_centered_draw(self):
+        # FIXED behavior: period/status/clock are joined into a single
+        # top line drawn once at y=1 instead of three overprinted draws.
+        helper = make_helper()
+        calls = []
+        original = helper._draw_centered_text
+
+        def spy(draw, text, font, y_position):
+            calls.append({'text': text, 'y_position': y_position})
+            original(draw, text, font, y_position)
+
+        helper._draw_centered_text = spy
+        font = default_font()
+        fonts = {'time': font, 'status': font, 'score': font, 'team': font}
+        helper.draw_scorebug_layout(
+            {'period_text': 'Q4', 'status_text': 'LIVE', 'clock': '2:30'},
+            fonts)
+
+        top_calls = [c for c in calls if c['y_position'] == 1]
+        assert len(top_calls) == 1
+        text = top_calls[0]['text']
+        assert 'Q4' in text
+        assert 'LIVE' in text
+        assert '2:30' in text
+
+    def test_no_top_line_when_all_parts_empty(self):
+        helper = make_helper()
+        calls = []
+        original = helper._draw_centered_text
+
+        def spy(draw, text, font, y_position):
+            calls.append(y_position)
+            original(draw, text, font, y_position)
+
+        helper._draw_centered_text = spy
+        font = default_font()
+        helper.draw_scorebug_layout({}, {'score': font, 'team': font})
+        assert 1 not in calls  # no combined top line drawn
+
+    def test_logo_positions_bleed_off_edges(self):
+        # Home logo pastes at x = width - logo.width + 10 (right edge,
+        # bleeding off-screen right); away at x = -10 (bleeding left).
+        helper = make_helper()
+        home_logo = Image.new('RGBA', (20, 20), (0, 0, 255, 255))   # blue
+        away_logo = Image.new('RGBA', (20, 20), (255, 0, 0, 255))   # red
+        # Empty abbrs/status so text can't land on the probed pixels.
+        game_data = {'home_abbr': '', 'away_abbr': ''}
+        font = default_font()
+        img = helper.draw_scorebug_layout(game_data, {'score': font},
+                                          home_logo=home_logo,
+                                          away_logo=away_logo)
+        # center_y = 16; logos span y 6..25 -> probe y=16 at both edges.
+        assert img.getpixel((0, 16)) == (255, 0, 0)     # away (left edge)
+        assert img.getpixel((127, 16)) == (0, 0, 255)   # home (right edge)
+        # And the off-screen parts are truly clipped: image is still 128 wide
+        assert img.size == (128, 32)
+
+
+class TestTickerLayout:
+    def test_frame_is_not_blank(self):
+        # FIXED behavior: text now starts at x=0. Previously it was drawn
+        # at x=display_width, entirely off-canvas, so frames were blank.
+        helper = make_helper()
+        img = helper.draw_ticker_layout('HELLO WORLD', default_font())
+        assert img.size == (128, 32)
+        assert img.mode == 'RGB'
+        assert img.convert('L').getbbox() is not None
+
+    def test_text_starts_at_left_edge(self):
+        helper = make_helper()
+        img = helper.draw_ticker_layout('HELLO', default_font())
+        bbox = img.convert('L').getbbox()
+        assert bbox is not None
+        # Text is positioned at x=0 (outline extends 1px left, clipped),
+        # so ink begins hugging the left edge. Allow a couple of pixels of
+        # slack for font-dependent left-side bearing.
+        assert bbox[0] <= 2
+
+    def test_scroll_speed_does_not_affect_frame(self):
+        # Pin: scroll_speed is accepted for API compatibility only.
+        helper = make_helper()
+        font = default_font()
+        img1 = helper.draw_ticker_layout('SCROLLING', font, scroll_speed=1)
+        img5 = helper.draw_ticker_layout('SCROLLING', font, scroll_speed=5)
+        assert img1.tobytes() == img5.tobytes()
+
+    def test_custom_colors(self):
+        helper = make_helper()
+        img = helper.draw_ticker_layout('X', default_font(),
+                                        background_color=(0, 0, 40),
+                                        text_color=(0, 255, 0))
+        assert img.getpixel((127, 0)) == (0, 0, 40)  # background corner
+        colors = {img.getpixel((x, y))
+                  for x in range(img.width) for y in range(img.height)}
+        # Text color appears somewhere (anti-aliasing may blend it, so
+        # check for a green-dominant pixel rather than the exact color).
+        assert any(g > 150 and r < 100 for (r, g, b) in colors)
+
+
+class TestCenteredText:
+    def test_renders_centered_text_on_background(self):
+        helper = make_helper()
+        img = helper.draw_centered_text('HI', default_font(),
+                                        background_color=(0, 0, 60),
+                                        text_color=(255, 255, 0))
+        assert img.size == (128, 32)
+        assert img.convert('L').getbbox() is not None
+        # Corners stay pure background
+        assert img.getpixel((0, 0)) == (0, 0, 60)
+        assert img.getpixel((127, 0)) == (0, 0, 60)
+        assert img.getpixel((0, 31)) == (0, 0, 60)
+        assert img.getpixel((127, 31)) == (0, 0, 60)
+
+
+class TestErrorAndNoDataMessages:
+    def test_draw_error_message_returns_rendered_image(self):
+        # FIXED behavior: used to crash with AttributeError; now returns
+        # a rendered image on a dark red background.
+        helper = make_helper()
+        img = helper.draw_error_message('Boom')
+        assert img.size == (128, 32)
+        assert img.mode == 'RGB'
+        assert img.convert('L').getbbox() is not None
+        assert img.getpixel((0, 0)) == (50, 0, 0)  # dark red background
+
+    def test_draw_error_message_default_text(self):
+        helper = make_helper()
+        img = helper.draw_error_message()
+        assert img.size == (128, 32)
+        assert img.getpixel((127, 31)) == (50, 0, 0)
+
+    def test_draw_no_data_message_returns_rendered_image(self):
+        helper = make_helper()
+        img = helper.draw_no_data_message()
+        assert img.size == (128, 32)
+        assert img.mode == 'RGB'
+        assert img.convert('L').getbbox() is not None
+        assert img.getpixel((0, 0)) == (0, 0, 0)  # black background
+
+
+class TestDrawTextWithOutline:
+    def test_fill_color_appears_in_output(self):
+        helper = make_helper()
+        img = Image.new('RGB', (40, 20), (0, 0, 255))
+        draw = ImageDraw.Draw(img)
+        helper._draw_text_with_outline(draw, 'X', (5, 2), default_font(),
+                                       fill=(255, 0, 0))
+        pixels = {img.getpixel((x, y))
+                  for x in range(img.width) for y in range(img.height)}
+        # Anti-aliased fonts blend edge pixels, so look for red-dominant
+        # (fill) and near-black (outline) pixels rather than exact colors.
+        assert any(r > 150 and g < 50 for (r, g, b) in pixels)   # fill
+        assert any(max(p) < 80 for p in pixels)                  # outline
+
+    def test_default_fill_is_white(self):
+        helper = make_helper()
+        img = Image.new('RGB', (40, 20), (0, 0, 255))
+        draw = ImageDraw.Draw(img)
+        helper._draw_text_with_outline(draw, 'X', (5, 2), default_font())
+        pixels = {img.getpixel((x, y))
+                  for x in range(img.width) for y in range(img.height)}
+        # White-dominant pixel present (exact white may be anti-aliased)
+        assert any(r > 200 and g > 200 for (r, g, b) in pixels)
+
+
+class TestOrientationAndDimensions:
+    def test_landscape_display(self):
+        helper = DisplayHelper(128, 32)
+        assert helper.is_landscape() is True
+        assert helper.is_portrait() is False
+
+    def test_portrait_display(self):
+        helper = DisplayHelper(32, 128)
+        assert helper.is_portrait() is True
+        assert helper.is_landscape() is False
+
+    def test_square_display_is_neither(self):
+        # Pin: a square display is neither portrait nor landscape.
+        helper = DisplayHelper(64, 64)
+        assert helper.is_portrait() is False
+        assert helper.is_landscape() is False
+
+    def test_get_center_position(self):
+        assert DisplayHelper(128, 32).get_center_position() == (64, 16)
+
+    def test_get_center_position_floors_odd_dimensions(self):
+        assert DisplayHelper(65, 33).get_center_position() == (32, 16)
+
+    def test_get_display_dimensions(self):
+        assert DisplayHelper(128, 32).get_display_dimensions() == (128, 32)
+        assert DisplayHelper(64, 64).get_display_dimensions() == (64, 64)

test/test_dynamic_team_resolver.py

@@ -0,0 +1,259 @@
+"""
+Tests for src/dynamic_team_resolver.py (DynamicTeamResolver).
+
+Covers dynamic team expansion (AP_TOP_5/10/25), order-preserving dedup,
+unknown dynamic-name dropping, rankings parsing, the fixed genuinely
+class-shared rankings cache (fetch and clear_cache write through
+DynamicTeamResolver._rankings_cache / _cache_timestamp), TTL expiry,
+network-failure resilience, and the resolve_dynamic_teams module function.
+
+No real network: src.dynamic_team_resolver.requests.get is always patched.
+"""
+
+import types
+from unittest.mock import MagicMock, patch
+
+import pytest
+import requests
+
+import src.dynamic_team_resolver as dtr_module
+from src.dynamic_team_resolver import DynamicTeamResolver, resolve_dynamic_teams
+
+
+TOP_TEAMS = ['UGA', 'MICH', 'OSU', 'TEX', 'ALA', 'ORE', 'PSU', 'ND', 'FSU', 'OU']
+
+
+def _rankings_payload(teams=None):
+    teams = TOP_TEAMS if teams is None else teams
+    return {
+        'rankings': [{
+            'name': 'AP Top 25',
+            'ranks': [
+                {'current': i + 1, 'team': {'abbreviation': abbr}}
+                for i, abbr in enumerate(teams)
+            ],
+        }]
+    }
+
+
+def _make_response(payload):
+    response = MagicMock()
+    response.json.return_value = payload
+    response.raise_for_status.return_value = None
+    return response
+
+
+@pytest.fixture(autouse=True)
+def reset_class_cache():
+    """Reset the CLASS-level shared cache between tests."""
+    DynamicTeamResolver._rankings_cache = {}
+    DynamicTeamResolver._cache_timestamp = 0
+    yield
+    DynamicTeamResolver._rankings_cache = {}
+    DynamicTeamResolver._cache_timestamp = 0
+
+
+@pytest.fixture
+def mock_get():
+    with patch('src.dynamic_team_resolver.requests.get') as m:
+        m.return_value = _make_response(_rankings_payload())
+        yield m
+
+
+@pytest.fixture
+def resolver():
+    return DynamicTeamResolver()
+
+
+# ---------------------------------------------------------------------------
+# resolve_teams basics
+# ---------------------------------------------------------------------------
+
+class TestResolveTeamsBasics:
+    def test_empty_list_returns_empty_no_http(self, resolver, mock_get):
+        assert resolver.resolve_teams([]) == []
+        mock_get.assert_not_called()
+
+    def test_no_dynamic_names_passthrough_no_http(self, resolver, mock_get):
+        assert resolver.resolve_teams(['UGA', 'AUB', 'LSU']) == [
+            'UGA', 'AUB', 'LSU']
+        mock_get.assert_not_called()
+
+    def test_expansion_inserted_in_place_order_preserved(
+            self, resolver, mock_get):
+        result = resolver.resolve_teams(['UGA', 'AP_TOP_5', 'AUB'])
+
+        # UGA is also ranked #1, so dedup keeps its first occurrence; the
+        # top-5 expansion lands where AP_TOP_5 appeared, AUB stays after.
+        assert result == ['UGA', 'MICH', 'OSU', 'TEX', 'ALA', 'AUB']
+
+    def test_order_preserving_dedup(self, resolver, mock_get):
+        result = resolver.resolve_teams(['UGA', 'AP_TOP_5', 'UGA'])
+
+        assert result == ['UGA', 'MICH', 'OSU', 'TEX', 'ALA']
+        assert result.count('UGA') == 1
+        assert result[0] == 'UGA'
+
+
+# ---------------------------------------------------------------------------
+# AP_TOP_N slicing
+# ---------------------------------------------------------------------------
+
+class TestSlicing:
+    def test_top_n_counts_and_order(self, resolver, mock_get):
+        teams_25 = [f'T{i:02d}' for i in range(1, 26)]
+        mock_get.return_value = _make_response(_rankings_payload(teams_25))
+
+        top5 = resolver.resolve_teams(['AP_TOP_5'])
+        top10 = resolver.resolve_teams(['AP_TOP_10'])
+        top25 = resolver.resolve_teams(['AP_TOP_25'])
+
+        assert top5 == teams_25[:5]
+        assert top10 == teams_25[:10]
+        assert top25 == teams_25
+
+
+# ---------------------------------------------------------------------------
+# Unknown dynamic-looking names
+# ---------------------------------------------------------------------------
+
+class TestUnknownDynamicNames:
+    def test_unknown_dynamic_looking_names_dropped(self, resolver, mock_get):
+        result = resolver.resolve_teams(
+            ['AP_TOP_100', 'TOP_10', 'RANKED_ALL', 'PLAYOFF_TEAMS'])
+
+        assert result == []
+        mock_get.assert_not_called()
+
+    def test_top_substring_hazard(self, resolver, mock_get):
+        # Hazard pin: _is_potential_dynamic_team matches the substring
+        # 'TOP_' anywhere in the (upper-cased) name, so a team literally
+        # named 'TOP_GUN' is dropped as an unknown dynamic team too.
+        assert resolver.resolve_teams(['TOP_GUN']) == []
+
+
+# ---------------------------------------------------------------------------
+# Rankings parsing
+# ---------------------------------------------------------------------------
+
+class TestRankingsParsing:
+    def test_drops_zero_rank_and_empty_abbreviation_sorts_ascending(
+            self, resolver, mock_get):
+        payload = {
+            'rankings': [{
+                'name': 'AP Top 25',
+                'ranks': [
+                    {'current': 3, 'team': {'abbreviation': 'C3'}},
+                    {'current': 1, 'team': {'abbreviation': 'A1'}},
+                    {'current': 0, 'team': {'abbreviation': 'ZERO'}},
+                    {'current': 4, 'team': {'abbreviation': ''}},
+                    {'current': 2, 'team': {'abbreviation': 'B2'}},
+                ],
+            }]
+        }
+        mock_get.return_value = _make_response(payload)
+
+        rankings = resolver._fetch_ncaa_fb_rankings()
+
+        assert list(rankings.keys()) == ['A1', 'B2', 'C3']
+        assert list(rankings.values()) == [1, 2, 3]
+
+    def test_empty_rankings_returns_empty_and_caches_nothing(
+            self, resolver, mock_get):
+        mock_get.return_value = _make_response({'rankings': []})
+
+        assert resolver._fetch_ncaa_fb_rankings() == {}
+        # Nothing was cached, so the next call hits HTTP again.
+        assert resolver._fetch_ncaa_fb_rankings() == {}
+        assert mock_get.call_count == 2
+
+
+# ---------------------------------------------------------------------------
+# Shared class cache (fixed behavior)
+# ---------------------------------------------------------------------------
+
+class TestSharedCache:
+    def test_cache_shared_across_instances(self, mock_get):
+        resolver1 = DynamicTeamResolver()
+        resolver1.resolve_teams(['AP_TOP_5'])
+        assert mock_get.call_count == 1
+
+        resolver2 = DynamicTeamResolver()
+        result = resolver2.resolve_teams(['AP_TOP_5'])
+
+        # Post-fix: the class-level cache serves the second instance with
+        # ZERO additional HTTP calls.
+        assert result == TOP_TEAMS[:5]
+        assert mock_get.call_count == 1
+
+    def test_ttl_expiry_refetches(self, resolver, mock_get, monkeypatch):
+        resolver.resolve_teams(['AP_TOP_5'])
+        assert mock_get.call_count == 1
+
+        stamp = DynamicTeamResolver._cache_timestamp
+        monkeypatch.setattr(
+            dtr_module, 'time', types.SimpleNamespace(time=lambda: stamp + 3601))
+
+        resolver.resolve_teams(['AP_TOP_5'])
+        assert mock_get.call_count == 2
+
+    def test_clear_cache_through_one_instance_affects_all(self, mock_get):
+        resolver1 = DynamicTeamResolver()
+        resolver1.resolve_teams(['AP_TOP_5'])
+        assert mock_get.call_count == 1
+
+        resolver2 = DynamicTeamResolver()
+        resolver2.clear_cache()
+
+        # Post-fix: clear_cache writes through the class, so resolver1
+        # must refetch even though resolver2 did the clearing.
+        resolver1.resolve_teams(['AP_TOP_5'])
+        assert mock_get.call_count == 2
+
+    def test_module_function_benefits_from_class_cache(self, mock_get):
+        # resolve_dynamic_teams constructs a fresh resolver per call, but
+        # the class-shared cache means only the first call hits HTTP.
+        first = resolve_dynamic_teams(['AP_TOP_5'])
+        second = resolve_dynamic_teams(['AP_TOP_5'])
+
+        assert first == second == TOP_TEAMS[:5]
+        assert mock_get.call_count == 1
+
+
+# ---------------------------------------------------------------------------
+# Failure handling
+# ---------------------------------------------------------------------------
+
+class TestFailureHandling:
+    def test_network_failure_drops_dynamic_keeps_static_caches_nothing(
+            self, resolver, mock_get):
+        mock_get.side_effect = [
+            requests.exceptions.RequestException('boom'),
+            _make_response(_rankings_payload()),
+        ]
+
+        result = resolver.resolve_teams(['UGA', 'AP_TOP_5'])
+
+        # Dynamic name silently dropped, static name kept, nothing raises.
+        assert result == ['UGA']
+
+        # Nothing was cached on failure: a subsequent call refetches and
+        # succeeds.
+        result = resolver.resolve_teams(['UGA', 'AP_TOP_5'])
+        assert result == ['UGA', 'MICH', 'OSU', 'TEX', 'ALA']
+        assert mock_get.call_count == 2
+
+
+# ---------------------------------------------------------------------------
+# sport argument
+# ---------------------------------------------------------------------------
+
+class TestSportArgument:
+    def test_sport_arg_ignored_for_expansion(self, resolver, mock_get):
+        # Pin: the sport argument is effectively ignored — each pattern
+        # carries its own sport ('ncaa_fb'), so passing sport='nfl' still
+        # expands from the college-football rankings.
+        result = resolver.resolve_teams(['AP_TOP_5'], sport='nfl')
+
+        assert result == TOP_TEAMS[:5]
+        assert mock_get.call_count == 1

test/test_logging_config.py

@@ -0,0 +1,274 @@
+"""
+Tests for src/logging_config.py — the formatters, adapter, and setup used
+by every logger in the system (BasePlugin uses get_logger, not stdlib
+logging.getLogger).
+
+Includes regression guards for two fixed bugs: ContextualFormatter used to
+mutate record.msg in place (double-prefixing with two handlers), and
+log_error hardcoded exc_info=True so passing it explicitly raised
+TypeError.
+"""
+
+import json
+import logging
+import sys
+
+import pytest
+
+from src.logging_config import (
+    ContextualFormatter,
+    PluginLoggerAdapter,
+    StructuredFormatter,
+    get_logger,
+    log_debug,
+    log_error,
+    log_info,
+    log_warning,
+    log_with_context,
+    setup_logging,
+)
+
+
+def make_record(msg="hello", level=logging.INFO, **extra):
+    record = logging.LogRecord(
+        name="test.logger", level=level, pathname=__file__, lineno=42,
+        msg=msg, args=(), exc_info=None)
+    for key, value in extra.items():
+        setattr(record, key, value)
+    return record
+
+
+class TestStructuredFormatter:
+    def test_emits_valid_json_with_base_keys(self):
+        out = json.loads(StructuredFormatter().format(make_record()))
+        assert set(out) == {
+            "timestamp", "level", "logger", "message",
+            "module", "function", "line",
+        }
+        assert out["level"] == "INFO"
+        assert out["message"] == "hello"
+        assert out["logger"] == "test.logger"
+
+    def test_optional_keys_only_when_present(self):
+        record = make_record(context={"k": "v"}, plugin_id="clock",
+                             operation_id="op-1")
+        out = json.loads(StructuredFormatter().format(record))
+        assert out["context"] == {"k": "v"}
+        assert out["plugin_id"] == "clock"
+        assert out["operation_id"] == "op-1"
+
+    def test_exception_key_when_exc_info_present(self):
+        try:
+            raise ValueError("kaboom")
+        except ValueError:
+            record = logging.LogRecord(
+                name="t", level=logging.ERROR, pathname=__file__, lineno=1,
+                msg="failed", args=(), exc_info=sys.exc_info())
+        out = json.loads(StructuredFormatter().format(record))
+        assert "kaboom" in out["exception"]
+
+    def test_percent_args_formatted_into_message(self):
+        record = logging.LogRecord(
+            name="t", level=logging.INFO, pathname=__file__, lineno=1,
+            msg="count=%d", args=(7,), exc_info=None)
+        out = json.loads(StructuredFormatter().format(record))
+        assert out["message"] == "count=7"
+
+
+class TestContextualFormatter:
+    def test_context_prefix_prepended(self):
+        record = make_record(plugin_id="clock", operation_id="op-1",
+                             context={"k": "v"})
+        out = ContextualFormatter().format(record)
+        assert "[Plugin: clock] [Op: op-1] [k: v] hello" in out
+
+    def test_include_context_false_leaves_message_bare(self):
+        record = make_record(plugin_id="clock")
+        out = ContextualFormatter(include_context=False).format(record)
+        assert "[Plugin:" not in out
+        assert "hello" in out
+
+    def test_location_toggle(self):
+        record = make_record()
+        with_loc = ContextualFormatter(include_location=True).format(record)
+        without = ContextualFormatter(include_location=False).format(record)
+        assert f":{record.lineno}" in with_loc
+        assert f":{record.lineno}" not in without
+
+    def test_record_not_mutated_no_double_prefix(self):
+        # Regression: a record is formatted once PER HANDLER. The formatter
+        # must not mutate record.msg, or the second handler's format call
+        # prepends the prefix again.
+        record = make_record(plugin_id="clock")
+        formatter = ContextualFormatter()
+        first = formatter.format(record)
+        second = formatter.format(record)
+        assert record.msg == "hello"  # untouched
+        assert first.count("[Plugin: clock]") == 1
+        assert second.count("[Plugin: clock]") == 1
+
+    def test_percent_args_still_format_after_copy(self):
+        record = logging.LogRecord(
+            name="t", level=logging.INFO, pathname=__file__, lineno=1,
+            msg="count=%d", args=(7,), exc_info=None)
+        record.plugin_id = "clock"
+        out = ContextualFormatter().format(record)
+        assert "[Plugin: clock] count=7" in out
+
+    def test_exception_renders_through_two_handlers(self):
+        try:
+            raise ValueError("kaboom")
+        except ValueError:
+            record = logging.LogRecord(
+                name="t", level=logging.ERROR, pathname=__file__, lineno=1,
+                msg="failed", args=(), exc_info=sys.exc_info())
+        record.plugin_id = "clock"
+        formatter = ContextualFormatter()
+        assert "kaboom" in formatter.format(record)
+        assert "kaboom" in formatter.format(record)  # second handler's pass
+
+
+class TestPluginLoggerAdapter:
+    def _capture(self, adapter):
+        records = []
+        handler = logging.Handler()
+        handler.emit = records.append
+        adapter.logger.addHandler(handler)
+        adapter.logger.setLevel(logging.DEBUG)
+        return records
+
+    def test_stamps_plugin_id_on_every_record(self):
+        adapter = get_logger("test.adapter1", plugin_id="clock")
+        records = self._capture(adapter)
+        adapter.info("x")
+        assert records[0].plugin_id == "clock"
+
+    def test_explicit_extra_plugin_id_wins(self):
+        adapter = get_logger("test.adapter2", plugin_id="clock")
+        records = self._capture(adapter)
+        adapter.info("x", extra={"plugin_id": "other"})
+        assert records[0].plugin_id == "other"
+
+    def test_unrelated_extra_keys_preserved(self):
+        adapter = get_logger("test.adapter3", plugin_id="clock")
+        records = self._capture(adapter)
+        adapter.info("x", extra={"custom": 1})
+        assert records[0].plugin_id == "clock"
+        assert records[0].custom == 1
+
+
+class TestGetLogger:
+    def test_plain_logger_without_plugin_id(self):
+        logger = get_logger("test.plain")
+        assert isinstance(logger, logging.Logger)
+        assert logger.name == "test.plain"
+
+    def test_adapter_with_plugin_id(self):
+        adapter = get_logger("test.wrapped", plugin_id="clock")
+        assert isinstance(adapter, PluginLoggerAdapter)
+        assert adapter.logger.name == "test.wrapped"
+
+
+class TestSetupLogging:
+    # conftest's autouse reset_logging restores root handlers after each test.
+
+    def test_installs_single_stdout_handler(self):
+        setup_logging()
+        root = logging.getLogger()
+        assert len(root.handlers) == 1
+        assert isinstance(root.handlers[0], logging.StreamHandler)
+
+    def test_repeat_calls_do_not_accumulate_handlers(self):
+        setup_logging()
+        setup_logging()
+        assert len(logging.getLogger().handlers) == 1
+
+    def test_json_format_selects_structured_formatter(self):
+        setup_logging(format_type="json")
+        assert isinstance(
+            logging.getLogger().handlers[0].formatter, StructuredFormatter)
+
+    def test_readable_format_selects_contextual_formatter(self):
+        setup_logging(format_type="readable")
+        assert isinstance(
+            logging.getLogger().handlers[0].formatter, ContextualFormatter)
+
+    def test_log_file_adds_file_handler(self, tmp_path):
+        log_file = tmp_path / "test.log"
+        setup_logging(log_file=str(log_file))
+        root = logging.getLogger()
+        file_handlers = [h for h in root.handlers
+                         if isinstance(h, logging.FileHandler)]
+        assert len(file_handlers) == 1
+        for h in file_handlers:
+            h.close()
+
+    def test_unwritable_log_file_warns_and_keeps_console(self, tmp_path, capsys):
+        bad_path = tmp_path / "no-such-dir" / "test.log"
+        setup_logging(log_file=str(bad_path))  # must not raise
+        assert len(logging.getLogger().handlers) == 1  # console only
+        assert "Could not set up file logging" in capsys.readouterr().err
+
+    def test_debug_env_true_enables_debug(self, monkeypatch):
+        monkeypatch.setenv("LEDMATRIX_DEBUG", "TRUE")
+        setup_logging()
+        assert logging.getLogger().level == logging.DEBUG
+
+    def test_debug_env_other_values_stay_info(self, monkeypatch):
+        # Pinned: only the literal (case-insensitive) "true" enables debug;
+        # "1" does not.
+        monkeypatch.setenv("LEDMATRIX_DEBUG", "1")
+        setup_logging()
+        assert logging.getLogger().level == logging.INFO
+
+    def test_explicit_level_wins_over_env(self, monkeypatch):
+        monkeypatch.setenv("LEDMATRIX_DEBUG", "true")
+        setup_logging(level=logging.WARNING)
+        assert logging.getLogger().level == logging.WARNING
+
+
+class TestLogWithContext:
+    def _capture(self, name):
+        logger = logging.getLogger(name)
+        records = []
+        handler = logging.Handler()
+        handler.emit = records.append
+        logger.addHandler(handler)
+        logger.setLevel(logging.DEBUG)
+        return logger, records
+
+    def test_context_attrs_stamped(self):
+        logger, records = self._capture("test.lwc1")
+        log_with_context(logger, logging.INFO, "msg",
+                         context={"k": "v"}, plugin_id="clock",
+                         operation_id="op-1")
+        record = records[0]
+        assert record.context == {"k": "v"}
+        assert record.plugin_id == "clock"
+        assert record.operation_id == "op-1"
+
+    def test_wrappers_use_their_levels(self):
+        logger, records = self._capture("test.lwc2")
+        log_debug(logger, "d")
+        log_info(logger, "i")
+        log_warning(logger, "w")
+        assert [r.levelno for r in records] == [
+            logging.DEBUG, logging.INFO, logging.WARNING]
+
+    def test_log_error_defaults_exc_info_true(self):
+        logger, records = self._capture("test.lwc3")
+        try:
+            raise ValueError("kaboom")
+        except ValueError:
+            log_error(logger, "failed")
+        assert records[0].levelno == logging.ERROR
+        assert records[0].exc_info is not None
+
+    def test_log_error_accepts_explicit_exc_info(self):
+        # Regression: the old hardcoded exc_info=True raised
+        # "got multiple values for keyword argument 'exc_info'".
+        logger, records = self._capture("test.lwc4")
+        log_error(logger, "failed", exc_info=False)
+        # Falsy exc_info is stored verbatim on the record; the contract is
+        # simply "no traceback attached".
+        assert not records[0].exc_info

test/test_saved_repositories.py

@@ -0,0 +1,240 @@
+"""
+Tests for src/plugin_system/saved_repositories.py — pins the
+SavedRepositoriesManager contract.
+
+Covers: the three accepted on-disk load shapes (bare list, wrapped
+{"repositories": [...]}, anything else -> []) and that saves always write
+the bare-list form; add/remove/has round trips through a fresh manager;
+URL normalization post-fix (_clean_url strips only a TRAILING '.git' after
+trailing slashes — the old unanchored .replace('.git', '') mangled URLs
+like my.github.io); name derivation and registry-vs-single type
+classification (the ledmatrix-plugins check is lowercased, the
+plugins.json check is case-sensitive); and the post-fix rollback of the
+in-memory list when _save_repositories() fails, so memory never diverges
+from disk.
+"""
+
+import json
+
+from src.plugin_system.saved_repositories import SavedRepositoriesManager
+
+
+def make_manager(path):
+    return SavedRepositoriesManager(config_path=str(path))
+
+
+class TestLoading:
+    def test_missing_file_empty_and_not_created(self, tmp_path):
+        path = tmp_path / "repos.json"
+        manager = make_manager(path)
+        assert manager.get_all() == []
+        assert not path.exists()
+
+    def test_bare_list_shape(self, tmp_path):
+        path = tmp_path / "repos.json"
+        entries = [{'url': 'https://github.com/u/r', 'name': 'r', 'type': 'single'}]
+        path.write_text(json.dumps(entries))
+        assert make_manager(path).get_all() == entries
+
+    def test_wrapped_dict_shape(self, tmp_path):
+        path = tmp_path / "repos.json"
+        entries = [{'url': 'https://github.com/u/r', 'name': 'r', 'type': 'single'}]
+        path.write_text(json.dumps({'repositories': entries}))
+        assert make_manager(path).get_all() == entries
+
+    def test_other_shape_yields_empty(self, tmp_path):
+        path = tmp_path / "repos.json"
+        path.write_text(json.dumps({'x': 1}))
+        assert make_manager(path).get_all() == []
+
+    def test_malformed_json_yields_empty_no_raise(self, tmp_path):
+        path = tmp_path / "repos.json"
+        path.write_text("not json {{")
+        assert make_manager(path).get_all() == []
+
+
+class TestSaveFormat:
+    def test_save_always_writes_bare_list(self, tmp_path):
+        # Even when loaded from the wrapped {"repositories": [...]} form,
+        # the next save normalizes the file to a bare JSON list.
+        path = tmp_path / "repos.json"
+        entries = [{'url': 'https://github.com/u/r', 'name': 'r', 'type': 'single'}]
+        path.write_text(json.dumps({'repositories': entries}))
+        manager = make_manager(path)
+        assert manager.add("https://github.com/u/r2") is True
+        on_disk = json.loads(path.read_text())
+        assert isinstance(on_disk, list)
+        assert len(on_disk) == 2
+
+
+class TestAdd:
+    def test_round_trip_creates_parents_and_reloads(self, tmp_path):
+        path = tmp_path / "sub" / "repos.json"
+        manager = make_manager(path)
+        assert manager.add("https://github.com/user/repo") is True
+        assert path.exists()
+        entry = manager.get_all()[0]
+        assert entry['url'] == "https://github.com/user/repo"
+        assert entry['name'] == "repo"
+        assert entry['type'] == "single"
+        # A fresh manager on the same path sees the persisted entry.
+        fresh = make_manager(path)
+        assert fresh.get_all() == [entry]
+
+    def test_duplicate_returns_false_file_unchanged(self, tmp_path):
+        path = tmp_path / "repos.json"
+        manager = make_manager(path)
+        assert manager.add("https://github.com/user/repo") is True
+        before = path.read_text()
+        assert manager.add("https://github.com/user/repo") is False
+        assert path.read_text() == before
+        assert len(manager.get_all()) == 1
+
+    def test_trailing_git_and_slash_stripped(self, tmp_path):
+        manager = make_manager(tmp_path / "repos.json")
+        assert manager.add("https://github.com/user/repo.git/") is True
+        assert manager.get_all()[0]['url'] == "https://github.com/user/repo"
+
+    def test_interior_dot_git_not_mangled(self, tmp_path):
+        # Regression for the old unanchored .replace('.git', ''): a URL
+        # merely CONTAINING '.git' must be stored verbatim.
+        manager = make_manager(tmp_path / "repos.json")
+        url = "https://github.com/user/my.github.io"
+        assert manager.add(url) is True
+        assert manager.get_all()[0]['url'] == url
+
+
+class TestNameExtraction:
+    def test_name_derived_from_last_path_segment(self, tmp_path):
+        manager = make_manager(tmp_path / "repos.json")
+        manager.add("https://github.com/user/football-scoreboard")
+        assert manager.get_all()[0]['name'] == "football-scoreboard"
+
+    def test_explicit_name_preserved(self, tmp_path):
+        manager = make_manager(tmp_path / "repos.json")
+        manager.add("https://github.com/user/repo", name="My Repo")
+        assert manager.get_all()[0]['name'] == "My Repo"
+
+    def test_url_without_slash_uses_whole_url(self, tmp_path):
+        manager = make_manager(tmp_path / "repos.json")
+        manager.add("standalone")
+        assert manager.get_all()[0]['name'] == "standalone"
+
+
+class TestTypeClassification:
+    def _type_of(self, tmp_path, url):
+        manager = make_manager(tmp_path / "repos.json")
+        assert manager.add(url) is True
+        return manager.get_all()[0]['type']
+
+    def test_plugins_json_url_is_registry(self, tmp_path):
+        url = "https://raw.githubusercontent.com/x/main/plugins.json"
+        assert self._type_of(tmp_path, url) == "registry"
+
+    def test_ledmatrix_plugins_check_is_case_insensitive(self, tmp_path):
+        url = "https://github.com/ChuckBuilds/LEDMATRIX-PLUGINS"
+        assert self._type_of(tmp_path, url) == "registry"
+
+    def test_plugins_json_check_is_case_sensitive(self, tmp_path):
+        # Only the 'ledmatrix-plugins' check is lowercased; the
+        # 'plugins.json' substring check is case-sensitive. Pinned.
+        url = "https://example.com/PLUGINS.JSON"
+        assert self._type_of(tmp_path, url) == "single"
+
+    def test_plain_repo_is_single(self, tmp_path):
+        assert self._type_of(tmp_path, "https://github.com/user/repo") == "single"
+
+    def test_get_registry_repositories_filters(self, tmp_path):
+        manager = make_manager(tmp_path / "repos.json")
+        manager.add("https://github.com/user/repo")
+        manager.add("https://raw.githubusercontent.com/x/main/plugins.json")
+        registries = manager.get_registry_repositories()
+        assert len(registries) == 1
+        assert registries[0]['type'] == "registry"
+
+
+class TestRemove:
+    def test_remove_present_persists(self, tmp_path):
+        path = tmp_path / "repos.json"
+        manager = make_manager(path)
+        manager.add("https://github.com/user/repo")
+        assert manager.remove("https://github.com/user/repo") is True
+        assert manager.get_all() == []
+        assert make_manager(path).get_all() == []
+
+    def test_remove_absent_false_no_write(self, tmp_path):
+        path = tmp_path / "repos.json"
+        manager = make_manager(path)
+        manager.add("https://github.com/user/repo")
+        before = path.read_text()
+        assert manager.remove("https://github.com/user/other") is False
+        assert path.read_text() == before
+
+    def test_remove_with_dirty_url_matches_clean_stored(self, tmp_path):
+        manager = make_manager(tmp_path / "repos.json")
+        manager.add("https://github.com/user/repo")
+        assert manager.remove("https://github.com/user/repo.git/") is True
+        assert manager.get_all() == []
+
+
+class TestHas:
+    def test_has_applies_url_cleaning(self, tmp_path):
+        manager = make_manager(tmp_path / "repos.json")
+        manager.add("https://x/y")
+        assert manager.has("https://x/y.git/") is True
+        assert manager.has("https://x/z") is False
+
+
+class TestSaveFailureRollback:
+    def test_add_rolls_back_on_save_failure(self, tmp_path, monkeypatch):
+        # Post-fix: a failed save must not leave a phantom in-memory entry.
+        path = tmp_path / "repos.json"
+        manager = make_manager(path)
+        monkeypatch.setattr(manager, "_save_repositories", lambda: False)
+        assert manager.add("https://github.com/user/repo") is False
+        assert manager.get_all() == []
+        assert not path.exists()
+
+    def test_remove_rolls_back_on_save_failure(self, tmp_path, monkeypatch):
+        path = tmp_path / "repos.json"
+        manager = make_manager(path)
+        manager.add("https://github.com/user/repo")  # real save
+        monkeypatch.setattr(manager, "_save_repositories", lambda: False)
+        assert manager.remove("https://github.com/user/repo") is False
+        assert manager.get_all() == [
+            {'url': 'https://github.com/user/repo', 'name': 'repo', 'type': 'single'}
+        ]
+        # Disk still has the entry too — memory and disk stay in sync.
+        assert len(json.loads(path.read_text())) == 1
+
+    def test_failed_write_leaves_existing_file_intact(self, tmp_path, monkeypatch):
+        # The save is atomic (temp file + os.replace): a write that dies
+        # mid-serialization must neither truncate the existing file nor
+        # leave a stray .tmp behind.
+        path = tmp_path / "repos.json"
+        manager = make_manager(path)
+        manager.add("https://github.com/user/repo")  # real save
+        before = path.read_text()
+
+        def boom(*args, **kwargs):
+            raise OSError("disk full")
+        monkeypatch.setattr(json, "dump", boom)
+        assert manager.add("https://github.com/user/other") is False
+
+        assert path.read_text() == before
+        assert list(tmp_path.glob("*.tmp")) == []
+
+
+class TestGetAllCopy:
+    def test_get_all_is_shallow_copy(self, tmp_path):
+        # Characterization: get_all() copies the LIST but not the entry
+        # dicts, so mutating a returned entry mutates internal state.
+        # Appending to the returned list, however, does not. Do not "fix"
+        # without auditing callers that rely on list-copy semantics.
+        manager = make_manager(tmp_path / "repos.json")
+        manager.add("https://github.com/user/repo")
+        returned = manager.get_all()
+        returned.append({'url': 'x'})
+        assert len(manager.get_all()) == 1  # list itself is copied
+        manager.get_all()[0]['name'] = 'hacked'
+        assert manager.get_all()[0]['name'] == 'hacked'  # dicts are shared

test/test_skin_runtime_cache.py

@@ -0,0 +1,377 @@
+"""Gap tests for src/skin_system/skin_runtime.py: the discovery cache,
+module namespacing internals, API gating edge cases, and targeting.
+
+test/test_skin_system.py already covers discovery validation, load_skin
+basics, and build_context — nothing here duplicates those.
+
+NOTE: every test uses a UNIQUE skin id. load_skin caches the entry
+module in sys.modules per skin id and never re-executes it, so reusing
+an id across tests would silently serve another test's module.
+"""
+
+import builtins
+import json
+import os
+import sys
+import time
+from pathlib import Path
+from unittest.mock import MagicMock
+
+import pytest
+
+# skin_runtime -> skin_base can transitively reach hardware modules via
+# sports imports in sibling tests' processes; stub the matrix driver
+# before importing, matching test_skin_system.py.
+sys.modules.setdefault("rgbmatrix", MagicMock())
+
+from src.skin_system import skin_runtime
+from src.skin_system.skin_base import SKIN_API_VERSION, ScoreboardSkin
+
+
+DEFAULT_BODY = (
+    "from src.skin_system.skin_base import ScoreboardSkin\n"
+    "class {cls}(ScoreboardSkin):\n"
+    "    def render_live(self, ctx, game):\n"
+    "        return True\n"
+)
+
+
+@pytest.fixture(autouse=True)
+def _clean_runtime_state():
+    """Clear the discovery cache and any skin modules this test creates."""
+    skin_runtime._discovery_cache.clear()
+    before = {k for k in sys.modules if k.startswith("_skin_")}
+    yield
+    skin_runtime._discovery_cache.clear()
+    created = [k for k in sys.modules
+               if k.startswith("_skin_") and k not in before]
+    for k in created:
+        sys.modules.pop(k, None)
+
+
+def make_skin(skins_dir: Path, skin_id: str, *,
+              api_version: str = SKIN_API_VERSION,
+              class_name: str = "TestSkin",
+              body: str = None,
+              extra_files: dict = None,
+              entry_point: str = None,
+              manifest_id: str = None,
+              manifest_extra: dict = None,
+              write_entry: bool = True) -> Path:
+    """Write a skin package directory and return its path."""
+    skin_dir = skins_dir / skin_id
+    skin_dir.mkdir(parents=True, exist_ok=True)
+    manifest = {
+        "id": manifest_id or skin_id,
+        "name": skin_id,
+        "version": "1.0.0",
+        "skin_api_version": api_version,
+        "class_name": class_name,
+    }
+    if entry_point:
+        manifest["entry_point"] = entry_point
+    manifest.update(manifest_extra or {})
+    (skin_dir / "skin.json").write_text(json.dumps(manifest))
+    if write_entry:
+        entry_name = entry_point or "skin.py"
+        (skin_dir / entry_name).write_text(
+            body if body is not None else DEFAULT_BODY.format(cls=class_name))
+    for name, content in (extra_files or {}).items():
+        (skin_dir / name).write_text(content)
+    return skin_dir
+
+
+def counting_read_manifest(monkeypatch):
+    """Wrap skin_runtime._read_manifest with a call counter."""
+    original = skin_runtime._read_manifest
+    counter = {"count": 0}
+
+    def wrapper(skin_dir):
+        counter["count"] += 1
+        return original(skin_dir)
+
+    monkeypatch.setattr(skin_runtime, "_read_manifest", wrapper)
+    return counter
+
+
+def bump_mtime(path: Path, offset: float = 100.0):
+    """Set a distinct, strictly later mtime so the fingerprint changes."""
+    t = time.time() + offset
+    os.utime(path, (t, t))
+
+
+# ---------------------------------------------------------------------------
+# A. Discovery cache
+# ---------------------------------------------------------------------------
+
+class TestDiscoveryCache:
+    def test_second_call_serves_cache(self, tmp_path, monkeypatch):
+        make_skin(tmp_path, "t01-cache-hit")
+        counter = counting_read_manifest(monkeypatch)
+        first = skin_runtime.discover_skins(tmp_path)
+        count_after_first = counter["count"]
+        assert count_after_first >= 1
+        second = skin_runtime.discover_skins(tmp_path)
+        assert counter["count"] == count_after_first  # no re-read
+        assert second == first
+        assert "t01-cache-hit" in second
+
+    def test_manifest_edit_invalidates_without_force_refresh(self, tmp_path):
+        skin_dir = make_skin(tmp_path, "t02-edit")
+        skins = skin_runtime.discover_skins(tmp_path)
+        assert skins["t02-edit"]["name"] == "t02-edit"
+
+        manifest_path = skin_dir / "skin.json"
+        manifest = json.loads(manifest_path.read_text())
+        manifest["name"] = "renamed"
+        manifest_path.write_text(json.dumps(manifest))
+        bump_mtime(manifest_path)
+
+        skins = skin_runtime.discover_skins(tmp_path)  # no force_refresh
+        assert skins["t02-edit"]["name"] == "renamed"
+
+    def test_new_skin_dir_invalidates(self, tmp_path):
+        make_skin(tmp_path, "t03-first")
+        assert set(skin_runtime.discover_skins(tmp_path)) == {"t03-first"}
+
+        new_dir = make_skin(tmp_path, "t03-second")
+        bump_mtime(new_dir / "skin.json")
+        bump_mtime(tmp_path)
+
+        skins = skin_runtime.discover_skins(tmp_path)  # no force_refresh
+        assert set(skins) == {"t03-first", "t03-second"}
+
+    def test_py_file_change_does_not_invalidate(self, tmp_path, monkeypatch):
+        # PIN: the fingerprint only globs */skin.json — editing a skin's
+        # .py file alone does NOT invalidate the cache; the cached
+        # manifests are still served (a code change needs a restart).
+        skin_dir = make_skin(tmp_path, "t04-pyedit")
+        counter = counting_read_manifest(monkeypatch)
+        skin_runtime.discover_skins(tmp_path)
+        count_after_first = counter["count"]
+
+        (skin_dir / "skin.py").write_text("# rewritten\n" +
+                                          DEFAULT_BODY.format(cls="TestSkin"))
+        bump_mtime(skin_dir / "skin.py")
+
+        skins = skin_runtime.discover_skins(tmp_path)
+        assert counter["count"] == count_after_first  # cache still served
+        assert "t04-pyedit" in skins
+
+    def test_force_refresh_rereads_with_unchanged_fingerprint(self, tmp_path,
+                                                              monkeypatch):
+        make_skin(tmp_path, "t05-force")
+        counter = counting_read_manifest(monkeypatch)
+        skin_runtime.discover_skins(tmp_path)
+        count_after_first = counter["count"]
+        skin_runtime.discover_skins(tmp_path, force_refresh=True)
+        assert counter["count"] > count_after_first
+
+    def test_result_mapping_is_copy_but_manifests_shared(self, tmp_path):
+        make_skin(tmp_path, "t06-copy")
+        result = skin_runtime.discover_skins(tmp_path)
+
+        # Mutating the returned mapping does not poison the cache...
+        del result["t06-copy"]
+        again = skin_runtime.discover_skins(tmp_path)  # cache hit
+        assert "t06-copy" in again
+
+        # ...but the inner manifest dicts ARE shared with the cache (pin).
+        again["t06-copy"]["name"] = "mutated-inner"
+        third = skin_runtime.discover_skins(tmp_path)  # cache hit
+        assert third["t06-copy"]["name"] == "mutated-inner"
+
+    def test_missing_directory_returns_empty_and_caches_nothing(self, tmp_path):
+        missing = tmp_path / "not-yet"
+        assert skin_runtime.discover_skins(missing) == {}
+        assert str(missing) not in skin_runtime._discovery_cache
+
+        # Creating the directory later is picked up without force_refresh.
+        make_skin(missing, "t07-late")
+        skins = skin_runtime.discover_skins(missing)
+        assert "t07-late" in skins
+
+    def test_hidden_underscore_and_plain_file_entries_skipped(self, tmp_path):
+        make_skin(tmp_path, ".hidden-skin")
+        make_skin(tmp_path, "_private-skin")
+        (tmp_path / "stray-file").write_text("not a directory")
+        make_skin(tmp_path, "t08-good")
+        skins = skin_runtime.discover_skins(tmp_path, force_refresh=True)
+        assert set(skins) == {"t08-good"}
+
+    def test_manifest_id_mismatch_keys_by_manifest_id(self, tmp_path):
+        make_skin(tmp_path, "t09-dirname", manifest_id="t09-manifest-id")
+        skins = skin_runtime.discover_skins(tmp_path, force_refresh=True)
+        assert "t09-manifest-id" in skins
+        assert "t09-dirname" not in skins
+        assert skins["t09-manifest-id"]["_skin_dir"].endswith("t09-dirname")
+
+    def test_falsy_required_field_drops_skin(self, tmp_path):
+        make_skin(tmp_path, "t10-empty-class", class_name="")
+        skins = skin_runtime.discover_skins(tmp_path, force_refresh=True)
+        assert skins == {}
+
+
+# ---------------------------------------------------------------------------
+# B. Module namespacing (_load_skin_module via load_skin)
+# ---------------------------------------------------------------------------
+
+BODY_WITH_HELPERS = (
+    "import helpers\n"
+    "from src.skin_system.skin_base import ScoreboardSkin\n"
+    "class TestSkin(ScoreboardSkin):\n"
+    "    pass\n"
+)
+
+
+class TestModuleNamespacing:
+    def test_namespaced_sys_modules_keys(self, tmp_path):
+        make_skin(tmp_path, "t11-ns", body=BODY_WITH_HELPERS,
+                  extra_files={"helpers.py": "VALUE = 11\n"})
+        skin = skin_runtime.load_skin("t11-ns", skins_dir=tmp_path)
+        assert skin is not None
+        assert "_skin_t11-ns_skin" in sys.modules
+        assert "_skin_t11-ns_helpers" in sys.modules
+
+    def test_preseeded_bare_name_restored(self, tmp_path, monkeypatch):
+        sentinel = object()
+        monkeypatch.setitem(sys.modules, "helpers", sentinel)
+        make_skin(tmp_path, "t12a-restore", body=BODY_WITH_HELPERS,
+                  extra_files={"helpers.py": "VALUE = 'a'\n"})
+        skin = skin_runtime.load_skin("t12a-restore", skins_dir=tmp_path)
+        assert skin is not None
+        assert sys.modules["helpers"] is sentinel
+
+    def test_absent_bare_name_stays_absent(self, tmp_path):
+        saved = sys.modules.pop("helpers", None)
+        try:
+            assert "helpers" not in sys.modules
+            make_skin(tmp_path, "t12b-absent", body=BODY_WITH_HELPERS,
+                      extra_files={"helpers.py": "VALUE = 'b'\n"})
+            skin = skin_runtime.load_skin("t12b-absent", skins_dir=tmp_path)
+            assert skin is not None
+            assert "helpers" not in sys.modules
+        finally:
+            if saved is not None:
+                sys.modules["helpers"] = saved
+
+    def test_stdlib_shadowing_sibling_leaves_real_module_intact(self, tmp_path):
+        real_json = sys.modules["json"]
+        make_skin(tmp_path, "t12c-json",
+                  extra_files={"json.py": "SKIN_LOCAL = True\n"})
+        skin = skin_runtime.load_skin("t12c-json", skins_dir=tmp_path)
+        assert skin is not None
+        assert sys.modules["json"] is real_json
+        assert not hasattr(sys.modules["json"], "SKIN_LOCAL")
+        assert json.loads('{"ok": 1}') == {"ok": 1}  # stdlib still works
+        # The skin's copy lives only under its namespaced alias.
+        assert getattr(sys.modules["_skin_t12c-json_json"], "SKIN_LOCAL") is True
+
+    def test_entry_module_executed_once_across_loads(self, tmp_path,
+                                                     monkeypatch):
+        executions = []
+        monkeypatch.setattr(builtins, "_t13_skin_executions", executions,
+                            raising=False)
+        body = (
+            "import builtins\n"
+            "builtins._t13_skin_executions.append(1)\n"
+            "from src.skin_system.skin_base import ScoreboardSkin\n"
+            "class TestSkin(ScoreboardSkin):\n"
+            "    pass\n"
+        )
+        make_skin(tmp_path, "t13-cached", body=body)
+        for _ in range(3):
+            skin = skin_runtime.load_skin("t13-cached", skins_dir=tmp_path)
+            assert skin is not None
+        assert len(executions) == 1  # module executed exactly once
+
+    def test_sibling_import_failure_returns_none_and_restores_bare(
+            self, tmp_path, monkeypatch):
+        sentinel = object()
+        monkeypatch.setitem(sys.modules, "helpers", sentinel)
+        make_skin(tmp_path, "t14-sibfail", body=BODY_WITH_HELPERS,
+                  extra_files={"helpers.py": "raise RuntimeError('sibling boom')\n"})
+        assert skin_runtime.load_skin("t14-sibfail", skins_dir=tmp_path) is None
+        assert sys.modules["helpers"] is sentinel
+
+    def test_missing_entry_point_file(self, tmp_path):
+        make_skin(tmp_path, "t15-noentry", write_entry=False)
+        assert skin_runtime.load_skin("t15-noentry", skins_dir=tmp_path) is None
+
+    def test_custom_entry_point(self, tmp_path):
+        make_skin(tmp_path, "t16-custom", entry_point="render.py")
+        skin = skin_runtime.load_skin("t16-custom", skins_dir=tmp_path)
+        assert isinstance(skin, ScoreboardSkin)
+        assert "_skin_t16-custom_render" in sys.modules
+        assert "_skin_t16-custom_skin" not in sys.modules
+
+    def test_class_name_pointing_at_unrelated_class(self, tmp_path):
+        body = "class NotASkin:\n    pass\n"
+        make_skin(tmp_path, "t17a-wrongclass", body=body,
+                  class_name="NotASkin")
+        assert skin_runtime.load_skin("t17a-wrongclass",
+                                      skins_dir=tmp_path) is None
+
+    def test_class_name_pointing_at_instance(self, tmp_path):
+        body = (
+            "from src.skin_system.skin_base import ScoreboardSkin\n"
+            "class MySkin(ScoreboardSkin):\n"
+            "    pass\n"
+            "obj = MySkin({}, {})\n"
+        )
+        make_skin(tmp_path, "t17b-instance", body=body, class_name="obj")
+        assert skin_runtime.load_skin("t17b-instance",
+                                      skins_dir=tmp_path) is None
+
+    def test_constructor_raising_returns_none(self, tmp_path):
+        body = (
+            "from src.skin_system.skin_base import ScoreboardSkin\n"
+            "class TestSkin(ScoreboardSkin):\n"
+            "    def __init__(self, manifest, options):\n"
+            "        raise ValueError('ctor boom')\n"
+        )
+        make_skin(tmp_path, "t18-ctor", body=body)
+        assert skin_runtime.load_skin("t18-ctor", skins_dir=tmp_path) is None
+
+
+# ---------------------------------------------------------------------------
+# C. API gate + targeting
+# ---------------------------------------------------------------------------
+
+class TestApiGateAndTargeting:
+    def test_same_major_higher_minor_loads(self, tmp_path):
+        make_skin(tmp_path, "t19-minor", api_version="1.9.0")
+        skin = skin_runtime.load_skin("t19-minor", skins_dir=tmp_path)
+        assert isinstance(skin, ScoreboardSkin)
+
+    def test_malformed_api_version_refused(self, tmp_path):
+        make_skin(tmp_path, "t20-malformed", api_version="abc")
+        assert skin_runtime.load_skin("t20-malformed",
+                                      skins_dir=tmp_path) is None
+
+    @pytest.mark.parametrize("manifest,sport,sport_key,expected", [
+        # No targets key at all -> matches everything
+        ({"id": "x"}, "baseball", "mlb", True),
+        ({"id": "x"}, None, None, True),
+        # Empty targets dict -> matches everything
+        ({"id": "x", "targets": {}}, "hockey", None, True),
+        # sports family match
+        ({"id": "x", "targets": {"sports": ["baseball"]}},
+         "baseball", None, True),
+        # sport_keys exact match
+        ({"id": "x", "targets": {"sport_keys": ["milb"]}},
+         None, "milb", True),
+        # OR semantics: sport_keys matches even though sports excludes it
+        ({"id": "x", "targets": {"sports": ["hockey"],
+                                 "sport_keys": ["milb"]}},
+         "baseball", "milb", True),
+        # Neither matches
+        ({"id": "x", "targets": {"sports": ["hockey"]}},
+         "baseball", None, False),
+        ({"id": "x", "targets": {"sports": ["hockey"],
+                                 "sport_keys": ["nhl"]}},
+         "baseball", "milb", False),
+    ])
+    def test_skin_matches_target(self, manifest, sport, sport_key, expected):
+        assert skin_runtime.skin_matches_target(
+            manifest, sport, sport_key) is expected

test/test_sports_capabilities.py

@@ -878,3 +878,200 @@ class TestCapabilityExports:
     def test_rotation_strategy_base_requires_a_schedule(self):
         with pytest.raises(NotImplementedError):
             RotationStrategy().schedule([game("a")])
+
+
+# ---------------------------------------------------------------------------
+# Celebrations: rendering + previously untested edges
+# ---------------------------------------------------------------------------
+
+from PIL import Image, ImageDraw, ImageFont  # noqa: E402
+
+
+class _RenderableLive(_FakeLive):
+    """A _FakeLive that can actually execute _draw_celebration_layout:
+    real fonts, a display manager holding a real PIL image, and the two
+    SportsCore drawing seams the mixin calls."""
+
+    def __init__(self, mode_config=None, favorite_teams=None,
+                 width=128, height=32, with_matrix=True):
+        super().__init__(mode_config=mode_config, favorite_teams=favorite_teams)
+        font = ImageFont.load_default()
+        self.fonts = {"time": font, "status": font, "score": font}
+        self.display_width = width
+        self.display_height = height
+        dm = MagicMock()
+        if with_matrix:
+            dm.matrix.width = width
+            dm.matrix.height = height
+        else:
+            dm.matrix = None
+        dm.image = Image.new("RGB", (width, height))
+        self.display_manager = dm
+        self.logo_calls = []
+
+    def _load_and_resize_logo(self, team_id, abbr, path, url):
+        self.logo_calls.append(abbr)
+        logo = Image.new("RGBA", (10, 10), (0, 200, 0, 255))
+        return logo
+
+    def _draw_text_with_outline(self, draw, text, position, font, fill=(255, 255, 255)):
+        draw.text(position, str(text), font=font, fill=fill)
+
+
+class _RenderableCelebrating(CelebrationMixin, _RenderableLive):
+    pass
+
+
+def _armed(manager, *, kind="score", side="home", started_ago=0.0):
+    manager._start_celebration(
+        game("g1", home_score=7, away_score=3), kind,
+        scored_side=side, team_abbr="HOM", away_score=3, home_score=7,
+        points=7,
+    )
+    manager.active_celebration["started_at"] = time.time() - started_ago
+    return manager.active_celebration
+
+
+class TestDrawCelebrationLayout:
+    """The takeover render path, executed for real (previously always
+    mocked out)."""
+
+    def test_renders_and_hands_frame_to_display_manager(self):
+        manager = _RenderableCelebrating()
+        celebration = _armed(manager)
+        manager._draw_celebration_layout(celebration)
+        # The final frame was assigned and pushed.
+        assert isinstance(manager.display_manager.image, Image.Image)
+        assert manager.display_manager.image.mode == "RGB"
+        assert manager.display_manager.image.size == (128, 32)
+        manager.display_manager.update_display.assert_called_once()
+        assert manager.display_manager.image.convert("L").getbbox() is not None
+
+    def test_force_clear_clears_display_first(self):
+        manager = _RenderableCelebrating()
+        celebration = _armed(manager)
+        manager._draw_celebration_layout(celebration, force_clear=True)
+        manager.display_manager.clear.assert_called_once()
+
+    def test_flash_background_within_first_window(self):
+        # elapsed < 1.2 with int(elapsed/0.2) even -> flash color backdrop.
+        manager = _RenderableCelebrating()
+        celebration = _armed(manager, started_ago=0.05)
+        manager._draw_celebration_layout(celebration)
+        flash = manager.display_manager.image
+        # After the flash window: plain black backdrop.
+        celebration["started_at"] = time.time() - 5
+        manager._draw_celebration_layout(celebration)
+        steady = manager.display_manager.image
+        # Corner pixels (away from logos/text) show the two backgrounds.
+        assert flash.getpixel((64, 30)) != steady.getpixel((64, 30)) or \
+            flash.getpixel((3, 0)) != steady.getpixel((3, 0))
+
+    def test_matrix_dims_fallback_to_display_attrs(self):
+        manager = _RenderableCelebrating(width=96, height=48, with_matrix=False)
+        celebration = _armed(manager)
+        manager._draw_celebration_layout(celebration)
+        assert manager.display_manager.image.size == (96, 48)
+
+    def test_highlight_color_alternates_with_elapsed(self):
+        manager = _RenderableCelebrating()
+        celebration = _armed(manager)
+        # int(elapsed*4) % 2 == 0 -> yellow; == 1 -> orange. Force each phase
+        # and diff the frames.
+        celebration["started_at"] = time.time() - 2.0   # 8 -> even
+        manager._draw_celebration_layout(celebration)
+        even = manager.display_manager.image.tobytes()
+        celebration["started_at"] = time.time() - 2.25  # 9 -> odd
+        manager._draw_celebration_layout(celebration)
+        odd = manager.display_manager.image.tobytes()
+        assert even != odd
+
+    def test_logo_failure_still_renders_text(self):
+        manager = _RenderableCelebrating()
+
+        def boom(*a, **k):
+            raise RuntimeError("disk gone")
+
+        manager._load_and_resize_logo = boom
+        celebration = _armed(manager, started_ago=5)  # steady background
+        manager._draw_celebration_layout(celebration)  # must not raise
+        assert manager.display_manager.image.convert("L").getbbox() is not None
+        manager.display_manager.update_display.assert_called_once()
+
+
+class TestCelebrationEdges:
+    def test_should_celebrate_for_three_way_branch(self, celebrating):
+        g = game("g1", home="FAV", away="OPP")
+        favored = celebrating(favorites=["FAV"])
+        assert favored._should_celebrate_for(g, "home") is True   # favorite
+        assert favored._should_celebrate_for(g, "away") is False  # opponent
+        favored.celebrate_opponent_scores = True
+        assert favored._should_celebrate_for(g, "away") is True   # opted in
+        unconfigured = celebrating(favorites=[])
+        assert unconfigured._should_celebrate_for(g, "away") is True  # no favs
+
+    def test_active_celebration_boundary_is_strict(self, celebrating):
+        manager = celebrating(mode_config={"celebration_duration": 3})
+        manager.active_celebration = {"started_at": time.time() - 3.0}
+        # elapsed == duration -> strictly-less-than comparison says done.
+        assert manager.has_active_celebration() is False
+        manager.active_celebration = None
+        assert manager.has_active_celebration() is False
+
+    @pytest.mark.parametrize("value,expected", [
+        ({"value": None}, None),      # int(float(None)) TypeError -> caught
+        ({"value": "abc"}, None),
+        ({"other": 1}, 0),            # neither key -> default 0
+        ([3], None),                  # list -> TypeError -> caught
+        ("-4", None),                 # regex fallback finds digits -> 4? No:
+    ])
+    def test_score_to_int_edges(self, value, expected):
+        result = CelebrationMixin._score_to_int(value)
+        if value == "-4":
+            # int(float("-4")) parses directly: -4.
+            assert result == -4
+        else:
+            assert result == expected
+
+    def test_both_teams_scoring_prefers_away(self, celebrating):
+        manager = celebrating(favorites=[])
+        manager._check_for_score(game("g1", home_score=0, away_score=0))
+        manager._check_for_score(game("g1", home_score=7, away_score=3))
+        assert manager.active_celebration["scored_side"] == "away"
+
+    def test_away_not_celebratable_falls_through_to_home(self, celebrating):
+        manager = celebrating(favorites=["HOM"])  # away is the opponent
+        manager._check_for_score(game("g1", home_score=0, away_score=0))
+        manager._check_for_score(game("g1", home_score=7, away_score=3))
+        assert manager.active_celebration["scored_side"] == "home"
+
+    def test_coalesce_expired_celebration_fires_fresh(self, celebrating):
+        manager = celebrating(cls=_Coalescing,
+                              mode_config={"celebration_duration": 1})
+        manager._check_for_score(game("g1"))
+        manager._check_for_score(game("g1", home_score=6))
+        first = manager.active_celebration
+        assert first is not None
+        first["started_at"] = time.time() - 2  # expired
+        manager._check_for_score(game("g1", home_score=7))
+        # A new celebration replaced the expired one (coalescing only
+        # suppresses while one is actively on screen).
+        assert manager.active_celebration is not first
+        assert manager.active_celebration["home_score"] == 7
+
+    def test_disabled_win_check_preserves_baseline(self, celebrating):
+        manager = celebrating(favorites=["HOM"])
+        manager._check_for_score(game("g1"))
+        assert "g1" in manager._score_baselines
+        manager.celebration_enabled = False
+        manager._check_for_win(game("g1", home_score=7))
+        # Early return BEFORE consuming the baseline: re-enabling later can
+        # still fire for this game.
+        assert "g1" in manager._score_baselines
+
+    def test_prune_drops_baselines_for_idless_live_games(self, celebrating):
+        manager = celebrating()
+        manager._score_baselines = {"g1": {"away": 0, "home": 0}}
+        manager.prune_score_baselines([{"no_id_here": True}])
+        # live ids collapse to {None}; g1 is not live -> dropped.
+        assert manager._score_baselines == {}

test/test_startup_validator.py

@@ -0,0 +1,293 @@
+"""
+Tests for src/startup_validator.py — pins the StartupValidator contract.
+
+Covers: required-key/config error reporting (errors never propagate out of
+validate_all), the load_config/get_config accessor split, cache-directory
+error-vs-warning downgrade behavior, plugin discovery/manifest checks with
+reserved config keys skipped, idempotent validate_all (fresh error/warning
+lists each run — the pre-fix behavior duplicated messages), and the
+exception classification precedence in raise_on_errors (config > cache >
+plugin > fallback ConfigError).
+"""
+
+import copy
+import os
+from unittest.mock import MagicMock
+
+import pytest
+
+from src.exceptions import CacheError, ConfigError, PluginError
+from src.startup_validator import StartupValidator
+
+GOOD_CONFIG = {
+    'display': {'hardware': {'rows': 32, 'cols': 64}},
+    'timezone': 'UTC',
+}
+
+
+def make_config_manager(config):
+    """Config manager whose load_config() and get_config() return `config`."""
+    mgr = MagicMock()
+    mgr.load_config.return_value = copy.deepcopy(config)
+    mgr.get_config.return_value = copy.deepcopy(config)
+    return mgr
+
+
+@pytest.fixture
+def good_cache(monkeypatch, tmp_path):
+    """Patch CacheManager so cache validation sees an existing writable dir.
+
+    _validate_cache_directory does `from src.cache_manager import CacheManager`
+    at call time, so patching the attribute on the module is picked up.
+    """
+    mock_cls = MagicMock()
+    mock_cls.return_value.get_cache_dir.return_value = str(tmp_path)
+    monkeypatch.setattr("src.cache_manager.CacheManager", mock_cls)
+    return tmp_path
+
+
+class TestValidateConfig:
+    """Configuration validation via load_config()."""
+
+    def test_happy_path(self, good_cache):
+        validator = StartupValidator(make_config_manager(GOOD_CONFIG))
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is True
+        assert errors == []
+        assert warnings == []
+
+    def test_missing_required_keys(self, good_cache):
+        validator = StartupValidator(make_config_manager({}))
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is False
+        assert "Missing required configuration key: display" in errors
+        assert "Missing required configuration key: timezone" in errors
+
+    def test_config_error_does_not_propagate(self, good_cache):
+        mgr = make_config_manager(GOOD_CONFIG)
+        mgr.load_config.side_effect = ConfigError("bad json")
+        validator = StartupValidator(mgr)
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is False
+        config_errors = [e for e in errors if e.startswith("Configuration error:")]
+        assert len(config_errors) == 1
+        assert "bad json" in config_errors[0]
+
+    def test_unexpected_error_does_not_propagate(self, good_cache):
+        mgr = make_config_manager(GOOD_CONFIG)
+        mgr.load_config.side_effect = RuntimeError("kapow")
+        validator = StartupValidator(mgr)
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is False
+        unexpected = [e for e in errors
+                      if e.startswith("Unexpected error validating configuration:")]
+        assert len(unexpected) == 1
+        assert "kapow" in unexpected[0]
+
+    def test_accessor_split_get_config_failure_is_warning_only(self, good_cache):
+        # _validate_config uses load_config(); _validate_display_config uses
+        # get_config(). A broken get_config must degrade to a warning, not
+        # crash or produce a config error.
+        mgr = make_config_manager(GOOD_CONFIG)
+        mgr.get_config.side_effect = RuntimeError("accessor broken")
+        validator = StartupValidator(mgr)
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is True
+        assert errors == []
+        assert any(w.startswith("Could not validate display configuration:")
+                   for w in warnings)
+        assert mgr.load_config.called
+        assert mgr.get_config.called
+
+
+class TestDisplayConfig:
+    """Display hardware validation via get_config()."""
+
+    def test_missing_hardware_section_is_error(self, good_cache):
+        config = {'display': {'runtime': {'gpio_slowdown': 2}}, 'timezone': 'UTC'}
+        validator = StartupValidator(make_config_manager(config))
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is False
+        assert "Display hardware configuration is missing" in errors
+
+    def test_missing_rows_cols_are_warnings_not_errors(self, good_cache):
+        config = {'display': {'hardware': {'brightness': 90}}, 'timezone': 'UTC'}
+        validator = StartupValidator(make_config_manager(config))
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is True
+        assert errors == []
+        assert "Display hardware setting 'rows' not specified, using default" in warnings
+        assert "Display hardware setting 'cols' not specified, using default" in warnings
+
+
+class TestCacheDirectory:
+    """Cache directory validation error/warning split."""
+
+    def _patch_cache_dir(self, monkeypatch, cache_dir):
+        mock_cls = MagicMock()
+        mock_cls.return_value.get_cache_dir.return_value = cache_dir
+        monkeypatch.setattr("src.cache_manager.CacheManager", mock_cls)
+
+    def test_nonexistent_cache_dir_is_error(self, monkeypatch, tmp_path):
+        missing = str(tmp_path / "does_not_exist")
+        self._patch_cache_dir(monkeypatch, missing)
+        validator = StartupValidator(make_config_manager(GOOD_CONFIG))
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is False
+        assert any("does not exist" in e and missing in e for e in errors)
+
+    def test_writable_cache_dir_no_errors(self, monkeypatch, tmp_path):
+        self._patch_cache_dir(monkeypatch, str(tmp_path))
+        validator = StartupValidator(make_config_manager(GOOD_CONFIG))
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is True
+        assert not any('cache' in e.lower() for e in errors)
+
+    def test_unwritable_cache_dir_is_error(self, monkeypatch, tmp_path):
+        # Root (common in CI) can write anywhere, so chmod tricks don't
+        # work — force os.access to deny writes for the cache dir only.
+        cache_dir = str(tmp_path)
+        self._patch_cache_dir(monkeypatch, cache_dir)
+        real_access = os.access
+
+        def fake_access(path, mode):
+            if str(path) == cache_dir and mode == os.W_OK:
+                return False
+            return real_access(path, mode)
+
+        monkeypatch.setattr(os, "access", fake_access)
+        validator = StartupValidator(make_config_manager(GOOD_CONFIG))
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is False
+        assert any("is not writable" in e for e in errors)
+
+    def test_none_cache_dir_is_warning_not_error(self, monkeypatch):
+        self._patch_cache_dir(monkeypatch, None)
+        validator = StartupValidator(make_config_manager(GOOD_CONFIG))
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is True
+        assert errors == []
+        assert "Cache directory not available - caching will be disabled" in warnings
+
+    def test_cache_manager_constructor_failure_is_warning(self, monkeypatch):
+        mock_cls = MagicMock(side_effect=RuntimeError("no disk"))
+        monkeypatch.setattr("src.cache_manager.CacheManager", mock_cls)
+        validator = StartupValidator(make_config_manager(GOOD_CONFIG))
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is True
+        assert errors == []
+        assert any(w.startswith("Could not validate cache directory:") for w in warnings)
+
+
+class TestPlugins:
+    """Plugin validation with a plugin manager present."""
+
+    def _config_with_plugins(self):
+        return {
+            'display': {'hardware': {'rows': 32, 'cols': 64}},
+            'schedule': {'enabled': True},  # reserved key that LOOKS enabled
+            'timezone': 'UTC',
+            'plugin_system': {},
+            'known': {'enabled': True},
+            'ghost': {'enabled': True},
+        }
+
+    def test_ghost_plugin_warns_and_reserved_keys_skipped(self, good_cache, tmp_path):
+        pm = MagicMock()
+        pm.discover_plugins.return_value = ['known']
+        known_dir = tmp_path / "known"
+        known_dir.mkdir()
+        (known_dir / "manifest.json").write_text("{}")
+        pm.get_plugin_directory.return_value = str(known_dir)
+
+        validator = StartupValidator(make_config_manager(self._config_with_plugins()), pm)
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is True
+        assert "Plugin 'ghost' is enabled but not found in plugins directory" in warnings
+        # Reserved sections are never treated as plugins, even when they
+        # contain an 'enabled' flag (schedule above).
+        for reserved in ('display', 'schedule', 'timezone', 'plugin_system'):
+            assert not any(f"'{reserved}'" in w for w in warnings)
+
+    def test_enabled_plugin_missing_manifest_is_error(self, good_cache, tmp_path):
+        pm = MagicMock()
+        pm.discover_plugins.return_value = ['known']
+        plugin_dir = tmp_path / "known"
+        plugin_dir.mkdir()  # exists, but no manifest.json inside
+        pm.get_plugin_directory.return_value = str(plugin_dir)
+
+        config = dict(GOOD_CONFIG, known={'enabled': True})
+        validator = StartupValidator(make_config_manager(config), pm)
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is False
+        assert "Plugin 'known' manifest.json not found" in errors
+
+    def test_disabled_plugin_not_checked_for_manifest(self, good_cache, tmp_path):
+        pm = MagicMock()
+        pm.discover_plugins.return_value = ['known']
+        pm.get_plugin_directory.return_value = str(tmp_path / "nowhere")
+
+        config = dict(GOOD_CONFIG, known={'enabled': False})
+        validator = StartupValidator(make_config_manager(config), pm)
+        is_valid, errors, warnings = validator.validate_all()
+        assert is_valid is True
+        assert errors == []
+
+
+class TestIdempotence:
+    """validate_all() resets error/warning state each run (the fixed bug)."""
+
+    def test_repeated_runs_do_not_accumulate(self, good_cache):
+        validator = StartupValidator(make_config_manager({}))
+        first = validator.validate_all()
+        second = validator.validate_all()
+        assert first == second
+        assert len(second[1]) == len(first[1])
+        assert len(second[2]) == len(first[2])
+
+
+class TestRaiseOnErrors:
+    """Exception classification and precedence in raise_on_errors()."""
+
+    def _validator(self, errors):
+        validator = StartupValidator(make_config_manager(GOOD_CONFIG))
+        validator.errors = list(errors)
+        return validator
+
+    def test_no_errors_returns_none(self):
+        assert self._validator([]).raise_on_errors() is None
+
+    def test_config_error(self):
+        msg = "Missing required configuration key: display"
+        with pytest.raises(ConfigError) as excinfo:
+            self._validator([msg]).raise_on_errors()
+        assert excinfo.value.message == "Configuration validation failed"
+        assert msg in excinfo.value.context['errors']
+
+    def test_cache_error(self):
+        msg = "Cache directory does not exist: /nope"
+        with pytest.raises(CacheError) as excinfo:
+            self._validator([msg]).raise_on_errors()
+        assert msg in excinfo.value.context['errors']
+
+    def test_plugin_error(self):
+        msg = "Plugin 'known' manifest.json not found"
+        with pytest.raises(PluginError) as excinfo:
+            self._validator([msg]).raise_on_errors()
+        assert msg in excinfo.value.context['errors']
+
+    def test_unclassified_error_falls_back_to_config_error(self):
+        msg = "Something entirely else went wrong"
+        with pytest.raises(ConfigError) as excinfo:
+            self._validator([msg]).raise_on_errors()
+        assert excinfo.value.message == "Startup validation failed"
+        assert msg in excinfo.value.context['errors']
+
+    def test_precedence_config_beats_cache(self):
+        # A message matching both 'config' and 'cache' substrings raises
+        # ConfigError because config classification is checked first.
+        msg = "config problem touching the cache layer"
+        with pytest.raises(ConfigError) as excinfo:
+            self._validator([msg]).raise_on_errors()
+        assert excinfo.value.message == "Configuration validation failed"
+        assert msg in excinfo.value.context['errors']

test/web_interface/test_api_v3_secret_roundtrip.py

@@ -0,0 +1,259 @@
+"""
+End-to-end secret round-trips through the three api_v3 endpoints that
+separate secrets from regular config (main-config save, plugin-config save,
+plugin-config reset) — now backed by the canonical
+src/web_interface/secret_helpers implementations.
+
+Unlike test_web_api.py (which mocks the config manager), these tests run a
+REAL ConfigManager and a REAL SchemaManager over tmp_path files, so they
+prove the whole chain: endpoint separation -> config_secrets.json write ->
+atomic config.json save (strip) -> load_config (merge back), including the
+array-item secret shape (accounts[].token) the inline copies never
+supported.
+"""
+
+import json
+import sys
+from pathlib import Path
+from unittest.mock import MagicMock
+
+import pytest
+from flask import Flask
+
+project_root = Path(__file__).parent.parent.parent
+sys.path.insert(0, str(project_root))
+
+from src.config_manager import ConfigManager  # noqa: E402
+from src.plugin_system.schema_manager import SchemaManager  # noqa: E402
+from web_interface.blueprints.api_v3 import api_v3  # noqa: E402
+
+
+PLUGIN_ID = "testplugin"
+
+SCHEMA = {
+    "$schema": "http://json-schema.org/draft-07/schema#",
+    "type": "object",
+    "properties": {
+        "enabled": {"type": "boolean", "default": True},
+        "display_duration": {"type": "number", "default": 15},
+        "api_key": {"type": "string", "x-secret": True, "default": ""},
+        "city": {"type": "string", "default": "Austin"},
+        "accounts": {
+            "type": "array",
+            "default": [],
+            "items": {
+                "type": "object",
+                "properties": {
+                    "name": {"type": "string"},
+                    "token": {"type": "string", "x-secret": True},
+                },
+            },
+        },
+    },
+}
+
+
+@pytest.fixture
+def env(tmp_path):
+    """Real ConfigManager + SchemaManager over tmp_path, wired onto the
+    api_v3 blueprint with the remaining managers mocked."""
+    config_file = tmp_path / "config.json"
+    config_file.write_text("{}")
+
+    plugins_dir = tmp_path / "plugins"
+    plugin_dir = plugins_dir / PLUGIN_ID
+    plugin_dir.mkdir(parents=True)
+    (plugin_dir / "config_schema.json").write_text(json.dumps(SCHEMA))
+    (plugin_dir / "manifest.json").write_text(json.dumps({
+        "id": PLUGIN_ID, "name": "Test Plugin", "version": "1.0.0",
+    }))
+
+    config_manager = ConfigManager(
+        config_path=str(config_file),
+        secrets_path=str(tmp_path / "config_secrets.json"))
+    config_manager.template_path = str(tmp_path / "no-template.json")
+
+    schema_manager = SchemaManager(plugins_dir=plugins_dir,
+                                   project_root=tmp_path)
+
+    plugin_manager = MagicMock()
+    plugin_manager.plugin_manifests = {PLUGIN_ID: {"id": PLUGIN_ID}}
+    plugin_manager.plugins_dir = plugins_dir
+    plugin_manager.get_plugin.return_value = None
+
+    api_v3.config_manager = config_manager
+    api_v3.schema_manager = schema_manager
+    api_v3.plugin_manager = plugin_manager
+    api_v3.plugin_store_manager = MagicMock()
+    api_v3.saved_repositories_manager = MagicMock()
+    api_v3.operation_queue = MagicMock()
+    api_v3.plugin_state_manager = MagicMock()
+    api_v3.operation_history = MagicMock()
+    api_v3.cache_manager = MagicMock()
+
+    app = Flask(__name__)
+    app.config["TESTING"] = True
+    app.register_blueprint(api_v3, url_prefix="/api/v3")
+
+    class Env:
+        pass
+
+    e = Env()
+    e.client = app.test_client()
+    e.config_manager = config_manager
+    e.config_file = config_file
+    e.secrets_file = tmp_path / "config_secrets.json"
+    e.tmp_path = tmp_path
+
+    def fresh_load():
+        """Load via a NEW ConfigManager, as the next request/process would.
+
+        The endpoint's manager serves its post-save in-memory config via the
+        mtime fast path, and that copy predates the secrets it just
+        separated out — a pre-existing quirk that applies to scalar secrets
+        too. On-disk truth is what these tests care about.
+        """
+        fresh = ConfigManager(config_path=str(config_file),
+                              secrets_path=str(e.secrets_file))
+        fresh.template_path = str(tmp_path / "no-template.json")
+        return fresh.load_config()
+
+    e.fresh_load = fresh_load
+    return e
+
+
+def _on_disk(path):
+    return json.loads(path.read_text())
+
+
+class TestSaveMainConfig:
+    """Site A: POST /config/main with a plugin-id key."""
+
+    def test_array_and_scalar_secrets_routed_to_secrets_file(self, env):
+        resp = env.client.post("/api/v3/config/main", json={
+            PLUGIN_ID: {
+                "city": "Dallas",
+                "api_key": "s3cret-key",
+                "accounts": [
+                    {"name": "a", "token": "s3cret-a"},
+                    {"name": "b"},
+                ],
+            },
+        })
+        assert resp.status_code == 200, resp.get_json()
+
+        on_disk = _on_disk(env.config_file)
+        assert on_disk[PLUGIN_ID]["city"] == "Dallas"
+        assert "api_key" not in on_disk[PLUGIN_ID]
+        assert on_disk[PLUGIN_ID]["accounts"] == [{"name": "a"}, {"name": "b"}]
+        assert "s3cret" not in env.config_file.read_text()
+
+        secrets = _on_disk(env.secrets_file)
+        assert secrets[PLUGIN_ID]["api_key"] == "s3cret-key"
+        assert secrets[PLUGIN_ID]["accounts"] == [{"token": "s3cret-a"}, {}]
+
+    def test_load_config_merges_secrets_back(self, env):
+        env.client.post("/api/v3/config/main", json={
+            PLUGIN_ID: {"accounts": [{"name": "a", "token": "s3cret-a"}]},
+        })
+        merged = env.fresh_load()
+        assert merged[PLUGIN_ID]["accounts"] == [
+            {"name": "a", "token": "s3cret-a"}]
+
+
+class TestSavePluginConfig:
+    """Site B: POST /plugins/config (JSON body)."""
+
+    def _save(self, env, config):
+        return env.client.post("/api/v3/plugins/config", json={
+            "plugin_id": PLUGIN_ID, "config": config,
+        })
+
+    def test_round_trip_with_array_secrets(self, env):
+        resp = self._save(env, {
+            "enabled": True,
+            "city": "Houston",
+            "api_key": "s3cret-key",
+            "accounts": [
+                {"name": "a", "token": "s3cret-a"},
+                {"name": "b", "token": "s3cret-b"},
+            ],
+        })
+        assert resp.status_code == 200, resp.get_json()
+
+        assert "s3cret" not in env.config_file.read_text()
+        on_disk = _on_disk(env.config_file)
+        assert on_disk[PLUGIN_ID]["accounts"] == [{"name": "a"}, {"name": "b"}]
+
+        secrets = _on_disk(env.secrets_file)
+        assert secrets[PLUGIN_ID]["accounts"] == [
+            {"token": "s3cret-a"}, {"token": "s3cret-b"}]
+
+        merged = env.fresh_load()
+        assert merged[PLUGIN_ID]["accounts"][1]["token"] == "s3cret-b"
+
+    def test_secret_count_message_counts_top_level_keys(self, env):
+        # Pinned: the "(N secret field(s))" message counts TOP-LEVEL keys of
+        # the separated secrets dict. Here that is 2: the posted accounts
+        # array (all its item tokens count as ONE key) plus the schema's
+        # api_key default ("") that merge_with_defaults adds before
+        # separation.
+        resp = self._save(env, {
+            "accounts": [{"name": "a", "token": "t"}],
+        })
+        message = resp.get_json()["message"]
+        assert "(2 secret field(s) saved to config_secrets.json)" in message
+
+    def test_resave_replaces_stored_secrets_list_wholesale(self, env):
+        # Characterized: api_v3's deep_merge intentionally replaces lists,
+        # so a re-save's parallel secrets list is authoritative.
+        self._save(env, {"accounts": [
+            {"name": "a", "token": "old-a"},
+            {"name": "b", "token": "old-b"},
+        ]})
+        self._save(env, {"accounts": [{"name": "only", "token": "new-only"}]})
+
+        secrets = _on_disk(env.secrets_file)
+        assert secrets[PLUGIN_ID]["accounts"] == [{"token": "new-only"}]
+        merged = env.fresh_load()
+        assert merged[PLUGIN_ID]["accounts"] == [
+            {"name": "only", "token": "new-only"}]
+
+
+class TestResetPluginConfig:
+    """Site C: POST /plugins/config/reset."""
+
+    def _seed(self, env):
+        env.client.post("/api/v3/plugins/config", json={
+            "plugin_id": PLUGIN_ID,
+            "config": {"city": "Houston", "api_key": "s3cret-key",
+                       "accounts": [{"name": "a", "token": "s3cret-a"}]},
+        })
+
+    def test_reset_preserving_secrets(self, env):
+        self._seed(env)
+        resp = env.client.post("/api/v3/plugins/config/reset", json={
+            "plugin_id": PLUGIN_ID, "preserve_secrets": True,
+        })
+        assert resp.status_code == 200, resp.get_json()
+
+        on_disk = _on_disk(env.config_file)
+        assert on_disk[PLUGIN_ID]["city"] == "Austin"  # schema default
+        assert on_disk[PLUGIN_ID]["accounts"] == []    # schema default
+
+        # Existing secrets survive (top-level-only preserve merge, pinned).
+        secrets = _on_disk(env.secrets_file)
+        assert secrets[PLUGIN_ID]["api_key"] == "s3cret-key"
+        assert secrets[PLUGIN_ID]["accounts"] == [{"token": "s3cret-a"}]
+
+    def test_reset_without_preserving_secrets(self, env):
+        self._seed(env)
+        resp = env.client.post("/api/v3/plugins/config/reset", json={
+            "plugin_id": PLUGIN_ID, "preserve_secrets": False,
+        })
+        assert resp.status_code == 200, resp.get_json()
+
+        secrets = _on_disk(env.secrets_file)
+        # Replaced with schema-default secrets — the schema declares no
+        # secret defaults, so the plugin's secrets are emptied.
+        assert secrets[PLUGIN_ID] in ({}, {"api_key": ""})

test/web_interface/test_secret_separation_parity.py

@@ -1,18 +1,13 @@
 """
-Drift guard for the duplicated secret-separation logic.
+Drift guard: api_v3 must use the canonical secret helpers.
 
-src/web_interface/secret_helpers.py is the canonical implementation of
+Historically web_interface/blueprints/api_v3.py carried THREE inline
-find_secret_fields / separate_secrets, but web_interface/blueprints/api_v3.py
+nested-function copies of ``find_secret_fields``/``separate_secrets`` (in the
-still carries THREE inline nested-function copies of each (in the plugin
+main-config save, plugin-config save, and plugin-config reset endpoints).
-config GET, POST, and reset endpoints). The copies lack the canonical
+They lacked the canonical module's array-item secret support and drifted from
-module's array-item support (`accounts[].token`), so migrating an endpoint
+each other. They have been migrated onto
-onto the module is a behavior change that must be made deliberately.
+``src/web_interface/secret_helpers`` — this file now guards against copies
-
+REAPPEARING, and keeps the canonical array-item behavior executable.
-This file guards two things:
-1. The copy count can only go DOWN. A fourth copy appearing means someone
-   re-implemented the logic again instead of importing secret_helpers.
-2. The known behavioral gap is documented as an executable fact, so whoever
-   migrates the endpoints knows exactly what changes.
 """
 
 import re
@@ -23,77 +18,44 @@ from src.web_interface.secret_helpers import find_secret_fields, separate_secret
 API_V3_PATH = (Path(__file__).resolve().parents[2]
                / "web_interface" / "blueprints" / "api_v3.py")
 
-# Update DOWNWARD as endpoints migrate onto src/web_interface/secret_helpers.
+# The migration is complete: any inline reimplementation is a regression.
-EXPECTED_INLINE_COPIES = 3
+EXPECTED_INLINE_COPIES = 0
 
 
-class TestInlineCopyCount:
+class TestNoInlineCopies:
     def _count(self, name: str) -> int:
         source = API_V3_PATH.read_text(encoding="utf-8")
         return len(re.findall(rf"^\s*def {name}\(", source, flags=re.MULTILINE))
 
-    def test_find_secret_fields_copy_count(self):
+    def test_no_inline_find_secret_fields(self):
         count = self._count("find_secret_fields")
         assert count == EXPECTED_INLINE_COPIES, (
             f"api_v3.py has {count} inline find_secret_fields definitions, "
-            f"expected {EXPECTED_INLINE_COPIES}. New code must import it from "
+            f"expected {EXPECTED_INLINE_COPIES}. Import it from "
-            f"src/web_interface/secret_helpers instead of re-implementing it; "
+            f"src/web_interface/secret_helpers instead of re-implementing it."
-            f"if you migrated an endpoint, lower EXPECTED_INLINE_COPIES."
         )
 
-    def test_separate_secrets_copy_count(self):
+    def test_no_inline_separate_secrets(self):
         count = self._count("separate_secrets")
         assert count == EXPECTED_INLINE_COPIES, (
             f"api_v3.py has {count} inline separate_secrets definitions, "
-            f"expected {EXPECTED_INLINE_COPIES}. New code must import it from "
+            f"expected {EXPECTED_INLINE_COPIES}. Import it from "
-            f"src/web_interface/secret_helpers instead of re-implementing it; "
+            f"src/web_interface/secret_helpers instead of re-implementing it."
-            f"if you migrated an endpoint, lower EXPECTED_INLINE_COPIES."
         )
 
-    def test_inline_copies_lack_array_item_support(self):
+    def test_canonical_import_present(self):
-        """The documented gap: no inline copy recurses into array `items`
+        # Tripwire: the endpoints still need the helpers, so removing the
-        schemas, so array-item secrets (accounts[].token) are NOT routed to
+        # import means either dead secret handling or a new local copy.
-        config_secrets.json by these endpoints. The canonical module handles
+        source = API_V3_PATH.read_text(encoding="utf-8")
-        them. When an endpoint migrates onto the module that behavior
+        assert re.search(
-        changes (a fix, but a deliberate one).
+            r"from src\.web_interface\.secret_helpers import .*find_secret_fields",
-
+            source,
-        If this fails, an inline copy has grown array support — duplicating
+        ), "api_v3.py no longer imports the canonical secret helpers"
-        the canonical module even harder. Migrate the endpoint onto
-        src/web_interface/secret_helpers instead.
-        """
-        for body in self._inline_bodies("find_secret_fields"):
-            # Array handling requires checking type == 'array'; no inline
-            # copy does. (Can't grep bare "items" — properties.items() the
-            # dict method appears legitimately.)
-            assert "'array'" not in body and '"array"' not in body
-
-    @staticmethod
-    def _inline_bodies(name: str):
-        """Extract each inline def's body from api_v3.py by indentation."""
-        lines = API_V3_PATH.read_text(encoding="utf-8").splitlines()
-        bodies = []
-        i = 0
-        while i < len(lines):
-            match = re.match(rf"^(\s+)def {name}\(", lines[i])
-            if not match:
-                i += 1
-                continue
-            indent = len(match.group(1))
-            body = [lines[i]]
-            i += 1
-            while i < len(lines):
-                line = lines[i]
-                if line.strip() and (len(line) - len(line.lstrip())) <= indent:
-                    break
-                body.append(line)
-                i += 1
-            bodies.append("\n".join(body))
-        assert bodies, f"no inline {name} definitions found"
-        return bodies
 
 
 class TestCanonicalArrayItemBehavior:
-    """Executable documentation of what migrating endpoints will change."""
+    """Executable documentation of the array-item secret contract the
+    endpoints now inherit from the canonical module."""
 
     SCHEMA = {
         "accounts": {

web_interface/blueprints/api_v3.py

@@ -21,6 +21,7 @@ logger = logging.getLogger(__name__)
 # Import new infrastructure
 from src.web_interface.api_helpers import success_response, error_response, validate_request_json
 from src.web_interface.errors import ErrorCode
+from src.web_interface.secret_helpers import find_secret_fields, separate_secrets
 from src.plugin_system.operation_types import OperationType
 from src.web_interface.validators import (
     validate_file_upload
@@ -1196,18 +1197,6 @@ def save_main_config():
                         plugins_dir = PROJECT_ROOT / plugins_dir_name
                 schema_path = plugins_dir / plugin_id / 'config_schema.json'
 
-                def find_secret_fields(properties, prefix=''):
-                    """Recursively find fields marked with x-secret: true"""
-                    fields = set()
-                    for field_name, field_props in properties.items():
-                        full_path = f"{prefix}.{field_name}" if prefix else field_name
-                        if field_props.get('x-secret', False):
-                            fields.add(full_path)
-                        # Check nested objects
-                        if field_props.get('type') == 'object' and 'properties' in field_props:
-                            fields.update(find_secret_fields(field_props['properties'], full_path))
-                    return fields
-
                 if schema_path.exists():
                     try:
                         with open(schema_path, 'r', encoding='utf-8') as f:
@@ -1218,24 +1207,6 @@ def save_main_config():
                         logger.debug("Error reading schema for secret detection: %s", e)
 
                 # Separate secrets from regular config (same logic as save_plugin_config)
-                def separate_secrets(config, secrets_set, prefix=''):
-                    """Recursively separate secret fields from regular config"""
-                    regular = {}
-                    secrets = {}
-                    for key, value in config.items():
-                        full_path = f"{prefix}.{key}" if prefix else key
-                        if isinstance(value, dict):
-                            nested_regular, nested_secrets = separate_secrets(value, secrets_set, full_path)
-                            if nested_regular:
-                                regular[key] = nested_regular
-                            if nested_secrets:
-                                secrets[key] = nested_secrets
-                        elif full_path in secrets_set:
-                            secrets[key] = value
-                        else:
-                            regular[key] = value
-                    return regular, secrets
-
                 regular_config, secrets_config = separate_secrets(plugin_config, secret_fields)
 
                 # PRE-PROCESSING: Preserve 'enabled' state if not in regular_config
@@ -4276,6 +4247,11 @@ def deep_merge(base_dict, update_dict):
     """
     Deep merge update_dict into base_dict.
     For nested dicts, recursively merge. For other types, update_dict takes precedence.
+
+    Lists are intentionally REPLACED wholesale, never index-merged: form posts
+    carry complete arrays, and index-merging would resurrect items the user
+    deleted. This also applies to the parallel secrets lists produced by
+    separate_secrets — a newly saved secrets list is authoritative.
     """
     result = base_dict.copy()
     for key, value in update_dict.items():
@@ -5290,23 +5266,9 @@ def save_plugin_config():
                 # Default to True on error to avoid disabling plugins
                 plugin_config['enabled'] = True
 
-        # Find secret fields (supports nested schemas)
+        # Find secret fields (supports nested schemas and array-item secrets)
         secret_fields = set()
 
-        def find_secret_fields(properties, prefix=''):
-            """Recursively find fields marked with x-secret: true"""
-            fields = set()
-            if not isinstance(properties, dict):
-                return fields
-            for field_name, field_props in properties.items():
-                full_path = f"{prefix}.{field_name}" if prefix else field_name
-                if isinstance(field_props, dict) and field_props.get('x-secret', False):
-                    fields.add(full_path)
-                # Check nested objects
-                if isinstance(field_props, dict) and field_props.get('type') == 'object' and 'properties' in field_props:
-                    fields.update(find_secret_fields(field_props['properties'], full_path))
-            return fields
-
         if schema and 'properties' in schema:
             secret_fields = find_secret_fields(schema['properties'])
 
@@ -5621,29 +5583,8 @@ def save_plugin_config():
                     status_code=400
                 )
 
-        # Separate secrets from regular config (handles nested configs)
+        # Separate secrets from regular config (handles nested configs and
-        def separate_secrets(config, secrets_set, prefix=''):
+        # array-item secrets — see src/web_interface/secret_helpers.py)
-            """Recursively separate secret fields from regular config"""
-            regular = {}
-            secrets = {}
-
-            for key, value in config.items():
-                full_path = f"{prefix}.{key}" if prefix else key
-
-                if isinstance(value, dict):
-                    # Recursively handle nested dicts
-                    nested_regular, nested_secrets = separate_secrets(value, secrets_set, full_path)
-                    if nested_regular:
-                        regular[key] = nested_regular
-                    if nested_secrets:
-                        secrets[key] = nested_secrets
-                elif full_path in secrets_set:
-                    secrets[key] = value
-                else:
-                    regular[key] = value
-
-            return regular, secrets
-
         regular_config, secrets_config = separate_secrets(plugin_config, secret_fields)
 
         # Get current configs
@@ -5891,41 +5832,10 @@ def reset_plugin_config():
         schema = schema_mgr.load_schema(plugin_id, use_cache=True)
         secret_fields = set()
 
-        def find_secret_fields(properties, prefix=''):
-            """Recursively find fields marked with x-secret: true"""
-            fields = set()
-            if not isinstance(properties, dict):
-                return fields
-            for field_name, field_props in properties.items():
-                full_path = f"{prefix}.{field_name}" if prefix else field_name
-                if isinstance(field_props, dict) and field_props.get('x-secret', False):
-                    fields.add(full_path)
-                if isinstance(field_props, dict) and field_props.get('type') == 'object' and 'properties' in field_props:
-                    fields.update(find_secret_fields(field_props['properties'], full_path))
-            return fields
-
         if schema and 'properties' in schema:
             secret_fields = find_secret_fields(schema['properties'])
 
         # Separate defaults into regular and secret configs
-        def separate_secrets(config, secrets_set, prefix=''):
-            """Recursively separate secret fields from regular config"""
-            regular = {}
-            secrets = {}
-            for key, value in config.items():
-                full_path = f"{prefix}.{key}" if prefix else key
-                if isinstance(value, dict):
-                    nested_regular, nested_secrets = separate_secrets(value, secrets_set, full_path)
-                    if nested_regular:
-                        regular[key] = nested_regular
-                    if nested_secrets:
-                        secrets[key] = nested_secrets
-                elif full_path in secrets_set:
-                    secrets[key] = value
-                else:
-                    regular[key] = value
-            return regular, secrets
-
         default_regular, default_secrets = separate_secrets(defaults, secret_fields)
 
         # Update main config with defaults