fix(vegas): let the watchdog see stalls that hold the GIL

The watchdog only noticed a late heartbeat, which a whole class of
freeze can never produce: if the loop is inside one long C call that
holds the GIL, this thread cannot run during the stall, and by the time
it does the loop has already checked in. On the dev rig that hid a
recurring 3.2s freeze completely -- twenty minutes of watching produced
one dump, for an unrelated 0.4s stall.

What it can still observe is that its own sleep ran long. A badly
overshot wait is now reported as a stall in its own right. The stacks
are stale by then and the message says so, but knowing the freeze is
GIL-holding is most of the diagnosis: it rules out lock contention and
scheduling, and points at a single long C call.

This also explains why lowering sys.setswitchinterval changed nothing --
the switch interval cannot preempt a C call that never releases the GIL.

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

.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

config/config.template.json

@@ -149,7 +149,7 @@
             "min_plugin_width": 8,
             "lead_in_width": 0,
             "plugins_per_cycle": 6,
-            "max_plugin_width_ratio": 3.0,
+            "max_plugin_width_ratio": 0.0,
             "overflow_mode": "rotate",
             "dynamic_duration_enabled": true,
             "min_cycle_duration": 60,

docs/CONFIG_REFERENCE.md

@@ -127,7 +127,7 @@ Read by `src/vegas_mode/config.py` (`VegasScrollConfig.from_config`). See
 | `min_plugin_width` | int, `8` |
 | `lead_in_width` | int, `0` |
 | `plugins_per_cycle` | int, `6` |
-| `max_plugin_width_ratio` | float, `3.0` |
+| `max_plugin_width_ratio` | float, `0.0` |
 | `overflow_mode` | string, `"rotate"` |
 | `dynamic_duration_enabled` | bool, `true` |
 | `min_cycle_duration` | int, `60` |

docs/widget-guide.md

@@ -206,6 +206,47 @@ To use an existing widget in your plugin's `config_schema.json`, simply add the
 
 The widget will be automatically rendered when the plugin configuration form is loaded.
 
+## Labelling Enum Options (`x-options.labels`)
+
+A plain `enum` renders as a dropdown whose option text is the value with
+underscores replaced and title case applied — `day_first` becomes "Day First".
+That is fine for values that read as their own label, and wrong for values that
+do not: `vs` becomes "Vs", and `abbrev` says nothing about the `Sep 19` it
+actually produces.
+
+Supply `x-options.labels` to set the visible text. This is the same convention
+the `checkbox-group` widget uses:
+
+```json
+{
+  "date_format": {
+    "type": "string",
+    "enum": ["abbrev", "numeric", "day_first"],
+    "default": "abbrev",
+    "x-options": {
+      "labels": {
+        "abbrev": "Sep 19",
+        "numeric": "9/19",
+        "day_first": "19 Sep"
+      }
+    }
+  }
+}
+```
+
+Labels are **display only** — the stored value is still the enum value, so
+adding them never changes a saved config. The map may be partial: any value
+without a label keeps the humanised fallback. Older cores that predate this
+support ignore `x-options` and render the fallback for every option, so a
+plugin can ship labels without requiring a core upgrade.
+
+Array-table columns (`x-widget: array-table`) accept the same
+`x-options.labels` on a column definition, but their fallback is the **raw
+value** rather than the humanised one, because those columns hold values such
+as ticker symbols where `aapl` → "Aapl" would be wrong. Rows added in the
+browser use the labels too (`array-table.js`), so a column reads the same
+before and after a page reload.
+
 ## Marking Fields as Advanced (`x-advanced`)
 
 Add `"x-advanced": true` to any top-level, non-object property to move it out

scripts/check_plugin.py

@@ -41,6 +41,7 @@ from src.plugin_system.testing.loading import (  # noqa: E402
 )
 from src.plugin_system.testing.harness import (  # noqa: E402
     RenderResult, render_plugin_matrix, compare_to_goldens, write_goldens,
+    check_empty_claimed,
     check_scale_up,
 )
 from src.plugin_system.testing.sizes import (  # noqa: E402
@@ -115,6 +116,11 @@ def check_one(plugin_id: str, search_dirs: List[str], sizes, mock_data: Dict,
     declared = load_manifest(plugin_dir).get("display", {}).get("design_size", {})
     design_size = (int(declared.get("width", 128)), int(declared.get("height", 32)))
     fill_strict = spec.get("fill_check") == "strict"
+    # A mode that renders nothing without returning False is never skipped by
+    # the display controller, so it holds a blank panel for its whole duration.
+    # Warn-only by default: a scroll mode's first frame is legitimately its
+    # blank scroll-in buffer.
+    empty_strict = spec.get("empty_check") == "strict"
 
     # Every run: the base config, plus one per harness.json "variant" —
     # a config overlay with its own golden dir (e.g. adaptive layout mode
@@ -142,6 +148,7 @@ def check_one(plugin_id: str, search_dirs: List[str], sizes, mock_data: Dict,
             compare_to_goldens(results, golden_dir)
 
         check_scale_up(results, design_size=design_size, strict=fill_strict)
+        check_empty_claimed(results, strict=empty_strict)
 
         # Tag variant runs so the report and PNG dumps stay distinguishable.
         if variant_name:
@@ -178,6 +185,9 @@ def print_report(all_results: Dict[str, List[RenderResult]]) -> bool:
                     # warn-only underfill: big panel left mostly empty
                     ex, ey = r.fill_extent
                     detail += f" (fill warn: extent {ex:.0%}x{ey:.0%})"
+                if r.empty_claimed and r.empty_ok is None:
+                    detail += (f" (empty warn: drew nothing but display() returned"
+                               f" {r.display_returned!r}, so the mode is not skipped)")
             else:
                 everything_ok = False
                 if r.error is not None:
@@ -191,6 +201,11 @@ def print_report(all_results: Dict[str, List[RenderResult]]) -> bool:
                     ex, ey = r.fill_extent or (0.0, 0.0)
                     status = "FAIL"
                     detail = f" fill: extent {ex:.0%}x{ey:.0%} below required coverage"
+                elif r.empty_ok is False:
+                    status = "FAIL"
+                    detail = (f" drew nothing but display() returned"
+                              f" {r.display_returned!r}; return False so the"
+                              f" controller skips the mode")
                 else:
                     status, detail = "FAIL", ""
             print(f"  [{status}] {r.size_label:>7}  {r.mode}{detail}")

src/base_odds_manager.py

@@ -12,6 +12,8 @@ Follows LEDMatrix configuration management patterns:
 """
 
 import logging
+import time
+
 import requests
 import json
 from typing import Dict, Any, Optional, List
@@ -45,7 +47,14 @@ class BaseOddsManager:
         
         # Configuration with defaults
         self.update_interval = 3600  # 1 hour default
-        self.request_timeout = 30    # 30 seconds default
+        # Well under the plugin executor's 30s operation budget. At 30s a
+        # single stalled ESPN request consumed the entire budget and the whole
+        # update() was killed -- and odds are fetched per live game, inside the
+        # live update loop, with show_odds defaulting on. Losing one game's
+        # odds beats losing the update that carries every game's score.
+        self.request_timeout = 5
+        # Set when a request fails; until then, skip the network entirely.
+        self._skip_network_until = 0.0
         self.cache_ttl = 1800       # 30 minutes default
         
         # Load configuration if available
@@ -73,6 +82,14 @@ class BaseOddsManager:
         except Exception as e:
             self.logger.warning(f"Failed to load BaseOddsManager configuration: {e}")
     
+    # After a network failure, stop trying for this long and serve cache only.
+    # A short per-request timeout bounds one stall, but a full Sunday slate is
+    # ~16 games fetched in a loop, so 16 consecutive timeouts still blow the
+    # budget. When ESPN is unreachable it is unreachable for all of them, so
+    # the first failure is enough to know: skip the rest of this pass and try
+    # again shortly.
+    _FAILURE_COOLDOWN = 60.0
+
     def get_odds(self, sport: str | None, league: str | None, event_id: str,
                  update_interval_seconds: int = None) -> Optional[Dict[str, Any]]:
         """
@@ -101,8 +118,18 @@ class BaseOddsManager:
             self.logger.info(f"Using cached odds from ESPN for {cache_key}")
             return cached_data
 
+        if time.monotonic() < self._skip_network_until:
+            # A recent request failed, so ESPN is very likely still unreachable.
+            # Returning now keeps the caller's update inside its time budget
+            # instead of paying the timeout again for every remaining game.
+            self.logger.debug(
+                "Skipping odds fetch for %s: a recent request failed, holding off "
+                "for another %.0fs", cache_key,
+                self._skip_network_until - time.monotonic())
+            return None
+
         self.logger.info(f"Cache miss - fetching fresh odds from ESPN for {cache_key}")
-        
+
         try:
             # Map league names to ESPN API format
             league_mapping = {
@@ -121,6 +148,8 @@ class BaseOddsManager:
             response.raise_for_status()
             raw_data = response.json()
 
+            self._skip_network_until = 0.0   # reachable again
+
             self.logger.debug(f"Received raw odds data from ESPN: {json.dumps(raw_data, indent=2)}")
             
             odds_data = self._extract_espn_data(raw_data)
@@ -140,7 +169,11 @@ class BaseOddsManager:
             return odds_data
 
         except requests.exceptions.RequestException as e:
-            self.logger.error(f"Error fetching odds from ESPN API for {cache_key}: {e}")
+            self._skip_network_until = time.monotonic() + self._FAILURE_COOLDOWN
+            self.logger.error(
+                "Error fetching odds from ESPN API for %s: %s. Holding off on odds "
+                "for %.0fs so a slate of games does not pay this timeout each.",
+                cache_key, e, self._FAILURE_COOLDOWN)
         except json.JSONDecodeError:
             self.logger.error(f"Error decoding JSON response from ESPN API for {cache_key}.")
         
@@ -163,19 +196,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,9 +299,13 @@ 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 = []
         
         # Add spread information

src/cache/disk_cache.py

@@ -112,6 +112,22 @@ class DiskCache:
                     record_ts = None
             
             now = time.time()
+
+            # An explicit per-entry ttl wins over the caller's max_age. The
+            # caller that wrote the record knows what its data is; max_age is
+            # inferred from substrings in the key ("live", "odds", "stock") and
+            # is only a fallback for records that never said. Until now the ttl
+            # was stored and ignored, so `set(key, data, ttl=...)` did nothing
+            # at all -- 48 plugin call sites and 4 in the core were writing a
+            # number no read path consulted.
+            effective_max_age = max_age
+            if isinstance(record, dict):
+                stored_ttl = record.get('ttl')
+                if isinstance(stored_ttl, (int, float)) and not isinstance(stored_ttl, bool) \
+                        and stored_ttl >= 0:
+                    effective_max_age = stored_ttl
+            max_age = effective_max_age
+
             # max_age=None means "never expires" (mirrors MemoryCache and the
             # cache_manager docstring). Guard it explicitly — otherwise the
             # comparison below raises TypeError and the record is treated as a

src/cache/memory_cache.py

@@ -57,6 +57,16 @@ class MemoryCache:
             if timestamp is None:
                 return None
             
+            # An explicit per-entry ttl wins over the caller's max_age, matching
+            # DiskCache. max_age is inferred from substrings in the key and is
+            # only a fallback for records that did not say what they wanted.
+            record = self._cache[key]
+            if isinstance(record, dict):
+                stored_ttl = record.get('ttl')
+                if isinstance(stored_ttl, (int, float)) and not isinstance(stored_ttl, bool) \
+                        and stored_ttl >= 0:
+                    max_age = stored_ttl
+
             # Check expiration
             if max_age is not None and (now - timestamp) > max_age:
                 # Expired - remove it

src/cache_manager.py

@@ -594,8 +594,10 @@ class CacheManager:
         Args:
             key: Cache key
             data: Data to cache
-            ttl: Optional time-to-live in seconds (stored for compatibility but
+            ttl: Time-to-live in seconds for this entry. Takes precedence over
-                 expiration is still controlled via max_age when reading)
+                 the max_age a reader would otherwise apply, which is inferred
+                 from the key and is only a fallback for entries that did not
+                 say. Omit it to keep that inferred behaviour.
         """
         cache_data = {
             'data': data,

src/common/api_helper.py

@@ -272,20 +272,35 @@ class APIHelper:
     def clear_cache(self, pattern: Optional[str] = None) -> None:
         """
         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,26 +149,28 @@ 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
         """
         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
     
     def draw_centered_text(self, text: str, font: ImageFont.ImageFont,
@@ -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

@@ -167,10 +167,14 @@ 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,24 +66,29 @@ 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 = []
-            
+
             if hasattr(record, 'plugin_id'):
                 context_parts.append(f"[Plugin: {record.plugin_id}]")
-            
+
             if hasattr(record, 'operation_id'):
                 context_parts.append(f"[Op: {record.operation_id}]")
-            
+
             if hasattr(record, 'context') and isinstance(record.context, dict):
                 for key, value in record.context.items():
                     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,15 +98,14 @@ 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:
             if repo.get('url') == repo_url:
                 self.logger.warning(f"Repository already exists: {repo_url}")
                 return False
-        
+
         # Extract name from URL if not provided
         if not name:
             parts = repo_url.split('/')
@@ -88,15 +113,20 @@ class SavedRepositoriesManager:
                 name = parts[-1]
             else:
                 name = repo_url
-        
+
         # Add repository
         self.repositories.append({
             'url': repo_url,
             'name': name,
             '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', '')
+
-        
+        previous = self.repositories
-        original_count = len(self.repositories)
+        remaining = [r for r in previous if r.get('url') != repo_url]
-        self.repositories = [r for r in self.repositories if r.get('url') != repo_url]
+
-        
+        if len(remaining) < len(previous):
-        if len(self.repositories) < original_count:
+            self.repositories = remaining
-            return self._save_repositories()
+            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/plugin_system/testing/harness.py

@@ -73,6 +73,11 @@ class RenderResult:
     golden_ok: Optional[bool] = None
     golden_diff_pixels: int = 0
     golden_max_delta: int = 0
+    # what display() handed back; the controller skips a mode only on False
+    display_returned: Any = None
+    # empty-frame check: rendered nothing while not reporting "no content"
+    empty_claimed: Optional[bool] = None   # True when that happened
+    empty_ok: Optional[bool] = None        # False only in strict mode
     # fill / scale-up check (populated only for sizes >= 2x the design size)
     fill_checked: bool = False
     fill_ok: Optional[bool] = None       # False only in strict mode
@@ -92,6 +97,8 @@ class RenderResult:
             return False
         if self.fill_ok is False:
             return False
+        if self.empty_ok is False:
+            return False
         return True
 
 
@@ -132,21 +139,25 @@ def _instantiate(plugin_id: str, manifest: Dict[str, Any], plugin_dir: Path,
     return plugin_instance
 
 
-def _render_mode(plugin_instance: Any, mode: str) -> None:
+def _render_mode(plugin_instance: Any, mode: str) -> Any:
     """Render a specific screen. Prefer an explicit display_mode kwarg; otherwise
     drive the plugin's internal mode state machine (first display() call renders
-    modes[current_mode_index] when current_display_mode is None)."""
+    modes[current_mode_index] when current_display_mode is None).
+
+    Returns whatever display() returned. The display controller skips a mode
+    whose display() returns False, so that value decides whether an empty mode
+    is rotated past or sat on -- which makes it worth reporting rather than
+    discarding."""
     sig = inspect.signature(plugin_instance.display)
     if "display_mode" in sig.parameters:
-        plugin_instance.display(force_clear=True, display_mode=mode)
+        return plugin_instance.display(force_clear=True, display_mode=mode)
-        return
 
     modes = getattr(plugin_instance, "modes", None)
     if modes and mode in modes:
         plugin_instance.current_mode_index = list(modes).index(mode)
     if hasattr(plugin_instance, "current_display_mode"):
         plugin_instance.current_display_mode = None
-    plugin_instance.display(force_clear=False)
+    return plugin_instance.display(force_clear=False)
 
 
 def _freeze(freeze_time: Optional[str]):
@@ -234,7 +245,7 @@ def _render_size(plugin_id, manifest, plugin_dir, config, mock_data,
                     logger.warning("update() raised a non-connectivity error for %s [%s]: %s",
                                    plugin_id, mode, e)
             if result.error is None:
-                _render_mode(inst, mode)
+                result.display_returned = _render_mode(inst, mode)
                 result.image = dm.get_image()
                 result.overflow = dm.check_overflow()
         except Exception as e:  # noqa: BLE001 — a display crash is a real failure
@@ -341,6 +352,44 @@ def fill_metrics(image: Image.Image) -> Tuple[float, float, float]:
     return (extent_x, extent_y, ink)
 
 
+def check_empty_claimed(results: List[RenderResult],
+                        strict: bool = False) -> List[RenderResult]:
+    """Flag a mode that rendered nothing without reporting "no content".
+
+    The display controller skips a mode whose ``display()`` returns False, and
+    treats anything else -- including None -- as "content was shown". A mode
+    that draws nothing and does not return False therefore holds whatever is on
+    the panel for its whole display duration. Since a mode switch clears first,
+    that is a blank screen. Two sports plugins shipped exactly this: their
+    ``display()`` returned None on every path, so an out-of-season league sat
+    blank for its full duration rather than being rotated past.
+
+    Warn-only by default, because a blank frame is not automatically wrong: a
+    scroll mode whose first frame is its blank scroll-in buffer renders empty
+    and is behaving correctly. ``strict=True`` sets ``empty_claimed`` such that
+    ``RenderResult.ok`` fails -- opt in per plugin via harness.json
+    ``{"empty_check": "strict"}`` once its modes are known to draw on the
+    fixture data.
+
+    Note this can only catch what the fixtures actually render. A plugin whose
+    harness fixture seeds content never exercises its empty path here; the
+    source-level gate in the plugins repo covers that case.
+    """
+    for r in results:
+        if r.image is None or r.error is not None:
+            continue
+        # An explicit False is the plugin correctly saying "nothing to show".
+        if r.display_returned is False:
+            continue
+        if r.image.convert("L").point(
+                lambda p: 255 if p > _LIT_THRESHOLD else 0).getbbox() is not None:
+            continue
+        r.empty_claimed = True
+        if strict:
+            r.empty_ok = False
+    return results
+
+
 def check_scale_up(results: List[RenderResult],
                    design_size: Tuple[int, int] = (128, 32),
                    min_extent: float = _MIN_FILL_EXTENT,

src/startup_validator.py

@@ -37,7 +37,12 @@ class StartupValidator:
             Tuple of (is_valid, errors, warnings)
         """
         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()
         

src/vegas_mode/config.py

@@ -104,10 +104,22 @@ class VegasModeConfig:
     overflow_mode: str = "rotate"
 
     # Cap on one plugin's share of a cycle, as a multiple of display width.
-    # A single ticker returning 7,000px would otherwise hold the panel for over
+    # 0 (the default) disables the cap, so every plugin contributes all of its
-    # two minutes. Overflow is deferred to later cycles rather than discarded.
+    # content and is always entered at its beginning.
-    # 0 disables the cap.
+    #
-    max_plugin_width_ratio: float = 3.0
+    # Capping was the default until it proved to cost more than it bought.
+    # Measured over a 17-plugin fleet on a 512px panel, only four plugins were
+    # ever wide enough to hit a 3.0 cap; for those four it produced two visible
+    # faults. Content resumed mid-item on each appearance (a news ticker entered
+    # at column 6027 of its own strip), and the final window of a rotation was
+    # whatever happened to be left — 348px of a 1840px stocks ticker, seven
+    # seconds of panel time. Both read as the display being broken rather than
+    # as deferral working.
+    #
+    # A wide plugin does hold the panel for a long time uncapped: set the cap
+    # per plugin with vegas_max_width_screens where that matters, rather than
+    # globally where it mostly hurts plugins that were never the problem.
+    max_plugin_width_ratio: float = 0.0
 
     # Plugin management
     plugin_order: List[str] = field(default_factory=list)
@@ -159,7 +171,7 @@ class VegasModeConfig:
             lead_in_width=int(vegas_config.get('lead_in_width', 0)),
             plugins_per_cycle=int(vegas_config.get('plugins_per_cycle', 6)),
             max_plugin_width_ratio=float(
-                vegas_config.get('max_plugin_width_ratio', 3.0)),
+                vegas_config.get('max_plugin_width_ratio', 0.0)),
             overflow_mode=str(vegas_config.get('overflow_mode', 'rotate')),
             plugin_order=list(vegas_config.get('plugin_order', [])),
             excluded_plugins=set(vegas_config.get('excluded_plugins', [])),

src/vegas_mode/coordinator.py

@@ -12,8 +12,11 @@ Supports three display modes per plugin:
 """
 
 import logging
-import time
+import os
+import sys
 import threading
+import time
+import traceback
 from typing import Optional, Dict, Any, List, Callable, TYPE_CHECKING
 
 from src.vegas_mode.config import VegasModeConfig
@@ -29,6 +32,81 @@ if TYPE_CHECKING:
 
 logger = logging.getLogger(__name__)
 
+# A frame is a "hitch" once it takes this many times the typical frame. Two
+# is deliberately forgiving: one dropped frame at 120fps is 8ms and invisible,
+# whereas a marquee moving a steady few pixels per frame shows a stall of
+# twice that as a visible jerk.
+_HITCH_FACTOR = 2.0
+
+# How many recent frames define "typical". Big enough to ride out noise, small
+# enough to track a genuine change in what the loop costs.
+_TYPICAL_SAMPLE = 60
+
+# A stall long enough that a viewer sees the marquee stop dead. Frame-time
+# statistics say one happened but not what did it, and by the time the numbers
+# are logged the stack is long gone -- so a watchdog samples every thread while
+# the loop is still wedged. Off unless LEDMATRIX_STALL_WATCHDOG is set, since
+# it dumps a lot of text.
+_STALL_DUMP_SECONDS = float(os.environ.get('LEDMATRIX_STALL_WATCHDOG', '0') or 0)
+
+
+class _StallWatchdog:
+    """Dumps every thread's stack when the render loop stops checking in."""
+
+    def __init__(self, threshold: float):
+        self.threshold = threshold
+        self._beat = time.time()
+        self._lock = threading.Lock()
+        self._stop = threading.Event()
+        self._dumped_for = 0.0
+        self._thread = threading.Thread(
+            target=self._watch, name="VegasStallWatchdog", daemon=True)
+        self._thread.start()
+
+    def beat(self) -> None:
+        with self._lock:
+            self._beat = time.time()
+
+    def stop(self) -> None:
+        self._stop.set()
+
+    def _watch(self) -> None:
+        poll = self.threshold / 4.0
+        while True:
+            woke_at = time.time()
+            if self._stop.wait(poll):
+                break
+            with self._lock:
+                last = self._beat
+            now = time.time()
+            stalled = now - last
+            # A stall inside a C call that holds the GIL never shows up as a
+            # late beat: this thread cannot run during it, and by the time it
+            # does the loop has already checked in. What it can see is that
+            # its own sleep ran long. Treat a badly overshot wait as a stall
+            # in its own right -- the stacks are stale by then, but knowing
+            # the freeze is GIL-holding is itself the diagnosis.
+            overshoot = (now - woke_at) - poll
+            if overshoot > self.threshold:
+                logger.warning(
+                    "render loop stalled %.2fs holding the GIL -- no Python "
+                    "frames ran, so the stacks below are from after it ended; "
+                    "look for one long C call (a large PIL operation, a "
+                    "compress, a big allocation)", overshoot)
+                stalled = overshoot
+            elif stalled < self.threshold or last == self._dumped_for:
+                continue
+            self._dumped_for = last          # one dump per stall, not per poll
+            frames = sys._current_frames()
+            names = {t.ident: t.name for t in threading.enumerate()}
+            lines = ["render loop stalled %.2fs -- thread stacks:" % stalled]
+            for ident, frame in frames.items():
+                lines.append("  --- %s (%s) ---" % (names.get(ident, "?"), ident))
+                for fn, lineno, func, _text in traceback.extract_stack(frame)[-8:]:
+                    lines.append("    %s:%d in %s" % (fn, lineno, func))
+            logger.warning("\n".join(lines))
+
+
 
 class VegasModeCoordinator:
     """
@@ -119,6 +197,7 @@ class VegasModeCoordinator:
             'static_pauses': 0,
         }
         self._start_time: Optional[float] = None
+        self._stall_watchdog: Optional['_StallWatchdog'] = None
 
         logger.info(
             "VegasModeCoordinator initialized: enabled=%s, fps=%d, buffer_ahead=%d",
@@ -382,6 +461,20 @@ class VegasModeCoordinator:
         fps_log_interval = 5.0  # Log FPS every 5 seconds
         last_fps_log_time = start_time
         fps_frame_count = 0
+        # Stutter is invisible in a mean. At 120fps a 5s window covers ~600
+        # frames, so a 200ms freeze -- plainly visible on a marquee -- moves
+        # the average from 120.0 to 115.4 and reads as healthy. What a viewer
+        # notices is the worst frame, so track that separately.
+        frame_worst = 0.0
+        frame_hitches = 0
+        frame_times: List[float] = []
+        frame_typical = 0.0
+
+        # One per coordinator, not per iteration -- run_iteration is called
+        # repeatedly, so building one here would leak a thread each time.
+        if _STALL_DUMP_SECONDS > 0 and self._stall_watchdog is None:
+            self._stall_watchdog = _StallWatchdog(_STALL_DUMP_SECONDS)
+        watchdog = self._stall_watchdog
 
         logger.info("Starting Vegas iteration for %.1fs", duration)
 
@@ -417,6 +510,25 @@ class VegasModeCoordinator:
             frame_elapsed = time.time() - frame_started
             time.sleep(max(0.0, frame_interval - frame_elapsed))
 
+            # Measured before the sleep, so this is time spent working rather
+            # than time spent pacing. A frame that overruns the budget is one
+            # the viewer sees as a jerk in otherwise smooth motion.
+            if frame_elapsed > frame_worst:
+                frame_worst = frame_elapsed
+            # Measured against what frames actually cost here, not against
+            # the configured target. The target is routinely set above what
+            # the panel can hold so vsync does the pacing -- against that
+            # budget every ordinary frame looks like a hitch, which is how
+            # the first version of this counter reported 250 per window on a
+            # display that was running perfectly smoothly.
+            if frame_typical and frame_elapsed > _HITCH_FACTOR * frame_typical:
+                frame_hitches += 1
+            if len(frame_times) >= _TYPICAL_SAMPLE:
+                frame_typical = sorted(frame_times[-_TYPICAL_SAMPLE:])[_TYPICAL_SAMPLE // 2]
+            frame_times.append(frame_elapsed)
+            if watchdog:
+                watchdog.beat()
+
             # Increment frame count and check for interrupt periodically
             frame_count += 1
             fps_frame_count += 1
@@ -425,12 +537,22 @@ class VegasModeCoordinator:
             current_time = time.time()
             if current_time - last_fps_log_time >= fps_log_interval:
                 fps = fps_frame_count / (current_time - last_fps_log_time)
+                p99 = 0.0
+                if frame_times:
+                    ordered = sorted(frame_times)
+                    p99 = ordered[min(len(ordered) - 1,
+                                      int(len(ordered) * 0.99))]
                 logger.info(
-                    "Vegas FPS: %.1f (target: %d, frames: %d)",
+                    "Vegas FPS: %.1f (target: %d, frames: %d) "
-                    fps, self.vegas_config.target_fps, fps_frame_count
+                    "p99 %.1fms worst %.1fms hitches %d",
+                    fps, self.vegas_config.target_fps, fps_frame_count,
+                    p99 * 1000.0, frame_worst * 1000.0, frame_hitches
                 )
                 last_fps_log_time = current_time
                 fps_frame_count = 0
+                frame_worst = 0.0
+                frame_hitches = 0
+                frame_times.clear()
 
             if (self._interrupt_check and
                     frame_count % self._interrupt_check_interval == 0):

src/vegas_mode/plugin_adapter.py

@@ -68,6 +68,21 @@ class PluginAdapter:
         # always the same opening items.
         self._item_offsets: dict = {}
 
+        # What the matching entry in _item_offsets is an offset *into*, as
+        # (kind, size). An offset only means anything against the content it
+        # was derived from, and there are three incompatible kinds:
+        #
+        #   ('rows', n)  index into a list of n images
+        #   ('cuts', n)  index into the n item boundaries of one image
+        #   ('cols', w)  pixel column in a w-wide image with no item boundaries
+        #
+        # Without this the offsets were reused across kinds — a plugin that
+        # returned one wide image on one fetch and several rows on the next had
+        # a pixel column of 1400 read back as a row index — and across content
+        # changes, where a column recorded against a 9,793px news strip pointed
+        # into unrelated headlines once the strip refreshed to 9,505px.
+        self._offset_shapes: dict = {}
+
         logger.info(
             "PluginAdapter initialized: display=%dx%d",
             self.display_width, self.display_height
@@ -398,6 +413,88 @@ class PluginAdapter:
             return 0
         return int(self.display_width * ratio)
 
+    def _resume_offset(self, plugin_id: str, shape: Tuple[str, int]) -> int:
+        """
+        The plugin's stored rotation offset, if it still applies.
+
+        An offset is only meaningful against content shaped the way it was
+        when the offset was recorded. When the shape has changed — a different
+        number of rows, a re-rendered strip with different item boundaries —
+        the stored value points somewhere arbitrary, so rotation restarts.
+
+        Args:
+            plugin_id: Plugin identifier
+            shape: (kind, size) describing what an offset would index into now
+
+        Returns:
+            The stored offset, or 0 when it no longer applies
+        """
+        if self._offset_shapes.get(plugin_id) != shape:
+            if plugin_id in self._item_offsets:
+                logger.info(
+                    "[%s] Content is %s now, was %s — restarting the rotation "
+                    "rather than resuming at a position that no longer means "
+                    "anything", plugin_id, shape,
+                    self._offset_shapes.get(plugin_id))
+            self._item_offsets.pop(plugin_id, None)
+            self._offset_shapes[plugin_id] = shape
+            return 0
+        return self._item_offsets.get(plugin_id, 0)
+
+    def _record_offset(
+        self, plugin_id: str, offset: int, shape: Tuple[str, int]
+    ) -> None:
+        """Store where the next window should resume, with what it indexes."""
+        if offset:
+            self._item_offsets[plugin_id] = offset
+            self._offset_shapes[plugin_id] = shape
+        else:
+            # A wrapped-to-zero rotation is the same as no state at all, and
+            # keeping the key would report a window as active when the next
+            # pass starts from the top anyway.
+            self._item_offsets.pop(plugin_id, None)
+            self._offset_shapes.pop(plugin_id, None)
+
+    def _clear_offset(self, plugin_id: str) -> None:
+        """Forget any rotation state for a plugin."""
+        self._item_offsets.pop(plugin_id, None)
+        self._offset_shapes.pop(plugin_id, None)
+
+    def _merge_trailing_runt(self, end: int, width: int, budget: int) -> int:
+        """
+        Extend a window to the end of the content when what would be left over
+        is too small to be worth its own pass.
+
+        Windows were placed by walking forward from the last one, which makes
+        the final window whatever happens to remain. Measured on a live panel
+        that produced a 1,840px stocks ticker splitting 1,492 + 348 — the
+        second pass showing seven seconds of content before cutting, which
+        reads as the display failing rather than as a rotation.
+
+        Absorbing the remainder overruns the budget by less than one window
+        floor, which is a better trade than a fragment: the budget is a guard
+        against one plugin holding the panel for minutes, not a hard limit.
+
+        Args:
+            end: Column the window would otherwise end at
+            width: Full content width
+            budget: Width budget being applied
+
+        Returns:
+            ``end``, or ``width`` when the remainder is below the floor
+        """
+        remainder = width - end
+        # Measured against the budget rather than the panel: snapping to item
+        # boundaries means an ordinary window already lands short of the budget
+        # (a 512px budget over 182px-pitch items yields 348px windows), so an
+        # absolute floor would merge windows that were never fragments. Half a
+        # budget separates "a short last pass" from "a sliver", and caps the
+        # overrun this can cause at 1.5 budgets.
+        floor = budget // 2
+        if 0 < remainder < floor:
+            return width
+        return end
+
     def _apply_width_budget(
         self, images: List[Image.Image], plugin_id: str,
         plugin: Optional['BasePlugin'] = None
@@ -435,32 +532,48 @@ class PluginAdapter:
 
         if not budget or total <= budget:
             # Fits, so reset rotation — the whole segment is being shown.
-            self._item_offsets.pop(plugin_id, None)
+            self._clear_offset(plugin_id)
             return images
 
         if len(images) == 1:
             return [self._crop_to_budget(images[0], budget, plugin_id, mode)]
 
+        shape = ('rows', len(images))
         if mode == 'truncate':
             # Ordered content: always show from the top. Deliberately does not
             # advance the offset, so the same opening items appear every time
             # rather than the viewer being shown the middle of a ranked list.
             start = 0
         else:
-            start = self._item_offsets.get(plugin_id, 0) % len(images)
+            start = self._resume_offset(plugin_id, shape) % len(images)
         selected: List[Image.Image] = []
         used = 0
         consumed = 0
 
         # Walk forward from the rotation offset, taking whole items only, so a
         # cut never lands in the middle of one.
+        #
+        # A window may overrun the budget while it is still shorter than the
+        # runt floor, for the same reason _merge_trailing_runt exists on the
+        # single-image path: a pass far shorter than its neighbours reads as
+        # the display failing rather than as a rotation. Rows of 450, 450 and
+        # 100 against a 512px budget used to give the 100 a pass of its own --
+        # two seconds against nine. Wrapping does not prevent that, because it
+        # only helps when the row wrapped to actually fits.
+        floor = budget // 2
         for step in range(len(images)):
             img = images[(start + step) % len(images)]
             cost = img.width
             if selected:
                 cost += self._row_gap(selected[-1], img)
             if selected and used + cost > budget:
-                break
+                # Keep the overrun bounded at the same 1.5 budgets the
+                # single-image path allows. A next row too wide to absorb
+                # leaves a short window standing -- better than a window of
+                # 1.9 budgets, and the same trade the always-take-the-first
+                # rule below already makes.
+                if used >= floor or used + cost > budget + floor:
+                    break
             selected.append(img)
             used += cost
             consumed += 1
@@ -472,7 +585,8 @@ class PluginAdapter:
                 plugin_id, budget, len(selected), len(images), used
             )
         else:
-            self._item_offsets[plugin_id] = (start + consumed) % len(images)
+            self._record_offset(
+                plugin_id, (start + consumed) % len(images), shape)
             logger.info(
                 "[%s] Width budget %dpx: showing %d of %d row(s) (%dpx incl. gaps) "
                 "from offset %d; remainder deferred to a later cycle",
@@ -490,16 +604,13 @@ class PluginAdapter:
 
         The cut is snapped to the nearest blank column so it does not slice
         through a glyph or logo and leave half a character at the panel edge.
-        """
-        if mode == 'truncate':
-            # Always the start of the strip, so a ranked table is never entered
-            # from the middle.
-            offset = 0
-        else:
-            offset = self._item_offsets.get(plugin_id, 0)
-            if offset >= img.width:
-                offset = 0
 
+        Rotation is tracked as an index into the strip's item boundaries rather
+        than as a pixel column, because a ticker re-renders between fetches. A
+        column recorded against one render points at unrelated content in the
+        next as soon as anything ahead of it changes width — a digit in a
+        price, a shorter headline. The Nth boundary stays the Nth boundary.
+        """
         # Cut only where the plugin left a real gap between items. Snapping to
         # any blank column used to pick the single-column gaps between
         # characters, splitting a word and orphaning its tail into the next
@@ -514,9 +625,17 @@ class PluginAdapter:
             # budget exactly. The gap rule exists to protect discrete items
             # (words, ticker entries); it would be wrong to let a solid image
             # escape the cap in its name.
-            end = min(offset + budget, img.width)
+            #
+            # With no items to index, the offset here has to stay a column, so
+            # it is only reusable while the image keeps its width.
+            shape = ('cols', img.width)
+            offset = 0 if mode == 'truncate' else self._resume_offset(
+                plugin_id, shape)
+            end = self._merge_trailing_runt(
+                min(offset + budget, img.width), img.width, budget)
             if mode != 'truncate':
-                self._item_offsets[plugin_id] = 0 if end >= img.width else end
+                self._record_offset(
+                    plugin_id, 0 if end >= img.width else end, shape)
             logger.info(
                 "[%s] Width budget %dpx: cropped continuous %dpx image to "
                 "[%d:%d] (no item gaps of %dpx+ to align to)%s",
@@ -528,8 +647,15 @@ class PluginAdapter:
         # Cut mid-gap so the content either side keeps some breathing room.
         cuts = sorted({0, img.width} | {(a + b) // 2 for a, b in gaps})
 
-        start = max((c for c in cuts if c <= offset), default=0)
+        shape = ('cuts', len(cuts))
-        later = [c for c in cuts if c > start]
+        index = 0 if mode == 'truncate' else self._resume_offset(
+            plugin_id, shape)
+        # Clamped rather than wrapped: a stale index past the end means the
+        # strip shrank, and restarting reads better than landing near the end.
+        start_index = index if 0 <= index < len(cuts) - 1 else 0
+        start = cuts[start_index]
+
+        later = cuts[start_index + 1:]
         if not later:
             end = img.width
         else:
@@ -537,15 +663,22 @@ class PluginAdapter:
             # No boundary inside the budget: take the next one and overrun,
             # because the alternative is cutting through an item.
             end = max(within) if within else min(later)
+        end = self._merge_trailing_runt(end, img.width, budget)
+        # Every candidate for `end` came from `cuts` (which includes img.width),
+        # so this always resolves; the fallback is defensive only.
+        end_index = cuts.index(end) if end in cuts else len(cuts) - 1
 
         if mode != 'truncate':
-            # Next cycle resumes where this one stopped; wrap when the strip ends.
+            # Next cycle resumes at the boundary this one stopped on; wrap when
-            self._item_offsets[plugin_id] = 0 if end >= img.width else end
+            # the strip ends.
+            self._record_offset(
+                plugin_id, 0 if end >= img.width else end_index, shape)
 
         logger.info(
             "[%s] Width budget %dpx: cropped single %dpx image to [%d:%d] "
-            "(%dpx) at item boundaries, %s",
+            "(%dpx) at item boundaries %d-%d of %d, %s",
             plugin_id, budget, img.width, start, end, end - start,
+            start_index, end_index, len(cuts) - 1,
             "showing the start only (overflow=truncate)"
             if mode == 'truncate' else "window advances next cycle"
         )

src/web_interface/error_handler.py

@@ -4,6 +4,7 @@ Centralized error handling for web interface.
 Provides helpers for consistent error responses across API endpoints.
 """
 
+import re
 from typing import Any, Optional
 from flask import jsonify
 
@@ -16,6 +17,78 @@ from src.logging_config import get_logger
 logger = get_logger(__name__)
 
 
+# Credentials that turn up inside exception text. A requests error quotes the
+# URL it failed on, and plugins that authenticate by query string put their key
+# there, so echoing an exception verbatim can hand out an API key. Redact the
+# value, keep the parameter name -- knowing *which* credential was involved is
+# part of the diagnosis.
+_REDACT_CREDENTIAL = re.compile(
+    r'((?:api[_-]?key|access[_-]?token|auth|apikey|key|passwd|password|pwd|'
+    r'secret|sig|signature|token)["\']?\s*[=:]\s*["\']?)([^\s&"\'<>,}]+)',
+    re.IGNORECASE,
+)
+
+# `Authorization: <scheme> <credential>`. The scheme name is kept because it
+# says which kind of credential failed; the credential goes. Any scheme
+# matches, not a fixed list: ApiKey, Negotiate, NTLM, AWS4-HMAC-SHA256 and
+# whatever a plugin's API invents next are all credentials, and a list would
+# silently leak the ones nobody thought of. Not covered by the generic pattern
+# above, whose value part stops at whitespace and so would keep the credential
+# once a space follows the scheme.
+_REDACT_AUTH_HEADER = re.compile(
+    r'((?:proxy-)?authorization["\']?\s*[=:]\s*["\']?\s*'
+    r'(?:[A-Za-z][\w.+-]*[ \t]+)?)'          # optional scheme name, kept
+    r'([^\s,"\'<>}]+)',                       # the credential, redacted
+    re.IGNORECASE,
+)
+
+# Credentials embedded in a URL: https://user:password@host. requests quotes
+# the full URL in its exceptions, so this is a realistic leak. The username is
+# kept -- it identifies which account failed without being the secret.
+_REDACT_URL_USERINFO = re.compile(r'([a-z][a-z0-9+.-]*://[^/\s:@]+:)([^/\s@]+)(@)',
+                                  re.IGNORECASE)
+
+# Long enough for an errno string with a path, short enough not to dump a
+# parser's worth of context into a JSON field.
+_MAX_DETAIL_LENGTH = 400
+
+
+def describe_exception(exc: BaseException,
+                       max_length: int = _MAX_DETAIL_LENGTH) -> str:
+    """
+    One-line, safe-to-return description of an exception.
+
+    The generic "an error occurred; see logs for details" tells a user nothing
+    and, when the failure is bad enough, the logs are unreachable too: a device
+    whose storage was failing returned that message from every endpoint
+    *including* the log viewer, because journalctl could not be executed. The
+    underlying `[Errno 5] Input/output error` named the fault immediately.
+
+    Returns "TypeName: message", credentials redacted and length capped. The
+    type alone is worth carrying -- a bare PermissionError says more than any
+    generic sentence.
+
+    Args:
+        exc: The exception to describe
+        max_length: Truncate beyond this many characters
+
+    Returns:
+        A single-line description, never empty
+    """
+    message = str(exc).strip()
+    text = f"{type(exc).__name__}: {message}" if message else type(exc).__name__
+    # Order matters: the URL and header forms are more specific than the
+    # generic key=value pattern, which would otherwise chew the scheme.
+    text = _REDACT_URL_USERINFO.sub(r'\1<redacted>\3', text)
+    text = _REDACT_AUTH_HEADER.sub(r'\1<redacted>', text)
+    text = _REDACT_CREDENTIAL.sub(r'\1<redacted>', text)
+    # Collapse newlines/tabs so the detail stays one line in a JSON field.
+    text = ' '.join(text.split())
+    if len(text) > max_length:
+        text = text[:max_length - 1].rstrip() + '…'
+    return text
+
+
 def create_error_response(
     error_code: ErrorCode,
     message: str,

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,363 @@
+"""
+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')
+        # The number matters less than the property: a single stalled request
+        # must not be able to consume the plugin executor's 30s operation
+        # budget, since odds are fetched per live game inside update().
+        assert mock_get.call_args.kwargs['timeout'] == 5
+        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 == 5
+        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_cache_ttl_honoured.py

@@ -0,0 +1,130 @@
+"""Tests that a per-entry ttl actually controls expiry.
+
+Regression under test: `CacheManager.set(key, data, ttl=...)` stored the value
+and no read path ever consulted it. Expiry came from a `max_age` inferred from
+substrings in the key ("live", "odds", "stock"), so every caller passing `ttl=`
+-- 48 sites across the plugins and 4 in the core -- was writing a number that
+did nothing. The old docstring admitted as much: "stored for compatibility but
+expiration is still controlled via max_age when reading".
+
+Measured against a real device's cache (8,873 entries carrying a ttl), the
+inferred value and the intended one disagreed almost everywhere:
+
+    stocks    max_age  600  vs ttl   1800   4903 entries
+    news      max_age 3600  vs ttl    600   1770 entries
+    odds      max_age 1800  vs ttl   3600   1301 entries
+    images    max_age  300  vs ttl 2592000    20 entries
+
+No `sports_live` entry carries a ttl, so live scores keep their inferred
+30-second freshness either way.
+"""
+
+import time
+
+import pytest
+
+from src.cache.memory_cache import MemoryCache
+from src.cache.disk_cache import DiskCache
+
+
+@pytest.fixture
+def disk(tmp_path):
+    return DiskCache(cache_dir=str(tmp_path))
+
+
+def _record(ttl=None, age=0.0):
+    rec = {"data": {"v": 1}, "timestamp": time.time() - age}
+    if ttl is not None:
+        rec["ttl"] = ttl
+    return rec
+
+
+class TestDiskCacheHonoursTtl:
+    def test_ttl_longer_than_max_age_keeps_the_entry(self, disk):
+        # The odds case: written wanting an hour, expired at 30 minutes.
+        disk.set("odds_espn_football_nfl_401", _record(ttl=3600, age=1900))
+        assert disk.get("odds_espn_football_nfl_401", max_age=1800) is not None
+
+    def test_ttl_shorter_than_max_age_expires_the_entry(self, disk):
+        # The news case: written wanting 10 minutes, kept for an hour.
+        disk.set("news_NHL_1", _record(ttl=600, age=900))
+        assert disk.get("news_NHL_1", max_age=3600) is None
+
+    def test_without_a_ttl_max_age_still_applies(self, disk):
+        disk.set("plain_key", _record(age=400))
+        assert disk.get("plain_key", max_age=300) is None
+        disk.set("plain_key2", _record(age=100))
+        assert disk.get("plain_key2", max_age=300) is not None
+
+    def test_a_fresh_entry_within_its_ttl_survives(self, disk):
+        disk.set("k", _record(ttl=600, age=10))
+        assert disk.get("k", max_age=30) is not None
+
+    def test_ttl_zero_expires_immediately(self, disk):
+        # 0 means zero seconds, not "forever" -- max_age=None is how a caller
+        # asks for no expiry.
+        disk.set("k", _record(ttl=0, age=1))
+        assert disk.get("k", max_age=99999) is None
+
+    @pytest.mark.parametrize("bad", ["600", None, True, False, -5, {"a": 1}])
+    def test_a_nonsense_ttl_falls_back_to_max_age(self, disk, bad):
+        # Including bools: True is an int in Python and must not become a 1s ttl.
+        rec = _record(age=400)
+        rec["ttl"] = bad
+        disk.set("k_%s" % type(bad).__name__, rec)
+        assert disk.get("k_%s" % type(bad).__name__, max_age=300) is None
+
+
+class TestMemoryCacheHonoursTtl:
+    def test_ttl_longer_than_max_age_keeps_the_entry(self):
+        m = MemoryCache()
+        m.set("k", _record(ttl=3600))
+        m._timestamps["k"] = time.time() - 1900
+        assert m.get("k", max_age=1800) is not None
+
+    def test_ttl_shorter_than_max_age_expires_the_entry(self):
+        m = MemoryCache()
+        m.set("k", _record(ttl=600))
+        m._timestamps["k"] = time.time() - 900
+        assert m.get("k", max_age=3600) is None
+
+    def test_without_a_ttl_max_age_still_applies(self):
+        m = MemoryCache()
+        m.set("k", _record())
+        m._timestamps["k"] = time.time() - 400
+        assert m.get("k", max_age=300) is None
+
+    def test_both_layers_agree(self, tmp_path):
+        """A record must not be live in one layer and expired in the other."""
+        rec = _record(ttl=3600, age=1900)
+        d = DiskCache(cache_dir=str(tmp_path))
+        d.set("k", rec)
+        m = MemoryCache()
+        m.set("k", rec)
+        m._timestamps["k"] = rec["timestamp"]
+        assert (d.get("k", max_age=1800) is not None) == (m.get("k", max_age=1800) is not None)
+
+
+class TestEndToEnd:
+    def test_set_then_get_respects_the_ttl(self, tmp_path, monkeypatch):
+        """The behaviour a caller of CacheManager.set(ttl=...) expects."""
+        from src.cache_manager import CacheManager
+
+        cm = CacheManager()
+        cm._disk_cache_component = DiskCache(cache_dir=str(tmp_path))
+        cm._memory_cache_component = MemoryCache()
+
+        cm.set("odds_espn_football_nfl_401", {"spread": 6.5}, ttl=3600)
+
+        # Age the stored record past the inferred max_age for odds (1800s) but
+        # within the ttl the caller asked for.
+        path = cm._disk_cache_component.get_cache_path("odds_espn_football_nfl_401")
+        import json
+        rec = json.load(open(path))
+        rec["timestamp"] = time.time() - 1900
+        json.dump(rec, open(path, "w"))
+        cm._memory_cache_component.clear() if hasattr(
+            cm._memory_cache_component, "clear") else None
+
+        got = cm.get_with_auto_strategy("odds_espn_football_nfl_401")
+        assert got is not None, "the ttl the caller asked for was ignored"

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_enum_option_labels.py

@@ -0,0 +1,134 @@
+"""Guard: enum dropdowns in the plugin config form honour x-options.labels.
+
+The form derives an option's visible text from its value — underscores
+replaced, title case applied ("day_first" -> "Day First"). That cannot
+express every label a schema needs: "vs" reads as "Vs", and "abbrev" says
+nothing about the "Sep 19" it produces. Schemas can supply x-options.labels
+instead, the same convention the checkbox-group widget already uses.
+
+These tests extract the enum <select> block *out of the shipped template*
+and render that, so they exercise the production expression rather than a
+copy of it. If the fallback or the lookup changes, these tests render the
+changed code and fail — a duplicated fragment here would silently keep
+passing.
+"""
+import re
+from pathlib import Path
+
+import pytest
+from jinja2 import DictLoader, Environment
+
+PROJECT_ROOT = Path(__file__).resolve().parent.parent
+CONFIG_FORM = (PROJECT_ROOT / 'web_interface' / 'templates' / 'v3' / 'partials'
+               / 'plugin_config.html')
+ARRAY_TABLE_JS = (PROJECT_ROOT / 'web_interface' / 'static' / 'v3' / 'js'
+                  / 'widgets' / 'array-table.js')
+
+# The enum branch: from the `{% set enum_labels %}` line through `</select>`.
+ENUM_BLOCK_RE = re.compile(
+    r"(\{%\s*set enum_labels\s*=.*?</select>)", re.S
+)
+
+
+def _shipped_enum_block() -> str:
+    """Return the live enum <select> block lifted from plugin_config.html."""
+    source = CONFIG_FORM.read_text(encoding='utf-8')
+    match = ENUM_BLOCK_RE.search(source)
+    assert match, (
+        'could not find the enum <select> block in plugin_config.html — the '
+        'template changed shape and this guard needs updating'
+    )
+    return match.group(1)
+
+
+def _render(prop: dict, value=None) -> str:
+    """Render the shipped enum block with a minimal fixture."""
+    env = Environment(loader=DictLoader({'f': _shipped_enum_block()}),
+                      autoescape=True)
+    return env.get_template('f').render(
+        prop=prop, value=value, field_id='fid', full_key='k'
+    )
+
+
+def _option_labels(html: str) -> dict:
+    """Map each rendered option's value to its visible text."""
+    return {
+        value: text.strip()
+        for value, text in re.findall(
+            r'<option value="([^"]*)"[^>]*>(.*?)</option>', html, re.S
+        )
+    }
+
+
+def test_labels_are_used_when_supplied() -> None:
+    html = _render({
+        'enum': ['vs', 'date_time'],
+        'x-options': {'labels': {'vs': 'VS', 'date_time': 'Date and time'}},
+    })
+    assert _option_labels(html) == {'vs': 'VS', 'date_time': 'Date and time'}
+
+
+def test_unlabelled_values_keep_the_humanised_fallback() -> None:
+    """Schemas without labels must render exactly as they did before."""
+    html = _render({'enum': ['day_first', 'weekday']})
+    assert _option_labels(html) == {'day_first': 'Day First', 'weekday': 'Weekday'}
+
+
+def test_partial_labels_fall_back_per_value() -> None:
+    """A labels map covering some values leaves the rest humanised."""
+    html = _render({'enum': ['vs', 'day_first'],
+                    'x-options': {'labels': {'vs': 'VS'}}})
+    assert _option_labels(html) == {'vs': 'VS', 'day_first': 'Day First'}
+
+
+def test_option_values_are_unchanged_by_labelling() -> None:
+    """Labels are display-only: the submitted value stays the enum value."""
+    html = _render({'enum': ['abbrev'],
+                    'x-options': {'labels': {'abbrev': 'Sep 19'}}})
+    assert _option_labels(html) == {'abbrev': 'Sep 19'}
+
+
+def test_selected_option_still_tracks_the_current_value() -> None:
+    """Labelling must not disturb which option is marked selected."""
+    html = _render({'enum': ['abbrev', 'numeric'],
+                    'x-options': {'labels': {'abbrev': 'Sep 19'}}},
+                   value='numeric')
+    selected = re.search(r'<option value="([^"]+)"[^>]*selected', html)
+    assert selected and selected.group(1) == 'numeric'
+
+
+@pytest.mark.parametrize('key', ['x-options', 'x_options'])
+def test_both_option_key_spellings_work(key: str) -> None:
+    """The template accepts either spelling, as its other widgets do."""
+    html = _render({'enum': ['vs'], key: {'labels': {'vs': 'VS'}}})
+    assert _option_labels(html) == {'vs': 'VS'}
+
+
+def test_table_column_enum_falls_back_to_the_raw_value() -> None:
+    """Array-table columns must not title-case values that were never labelled.
+
+    Those columns hold values such as ticker symbols, where "aapl" -> "Aapl"
+    would be wrong, so their fallback stays the raw value.
+    """
+    source = CONFIG_FORM.read_text(encoding='utf-8')
+    assert 'col_labels.get(opt, opt)' in source, (
+        'array-table column options must fall back to the raw value, not the '
+        'humanised one'
+    )
+
+
+def test_dynamically_added_table_rows_use_the_same_labels() -> None:
+    """Rows added client-side must label options like the server-rendered ones.
+
+    array-table.js builds new rows in the browser; if it printed the raw value
+    a column would read differently before and after a page reload.
+    """
+    js = ARRAY_TABLE_JS.read_text(encoding='utf-8')
+    assert 'function enumOptionLabel' in js, (
+        'array-table.js lost its enum label helper'
+    )
+    raw_option_text = re.findall(r'o\.textContent\s*=\s*opt\s*;', js)
+    assert not raw_option_text, (
+        'array-table.js renders an enum option as its raw value; it must go '
+        'through enumOptionLabel() so dynamic rows match server-rendered ones'
+    )

test/test_git_pull_resolution.py

@@ -0,0 +1,202 @@
+"""Guard: the update button works on branches without tracking information.
+
+`git pull --rebase` fails outright on a branch with no upstream:
+
+    There is no tracking information for the current branch.
+    Please specify which branch you want to rebase against.
+
+That is easy to land on — checking out a branch by name, restoring a
+backup, or following a guide that names one — and the Tools tab reported it
+as a bare "Update failed; check logs for details", which the user cannot act
+on. resolve_pull_command() falls back to an explicit `origin <branch>` pull
+when that remote branch exists, and returns an actionable message when it
+does not.
+
+These tests build real git repositories in a temp dir, so they exercise git's
+actual behaviour rather than a mock of it.
+"""
+import subprocess
+from pathlib import Path
+
+import pytest
+
+from web_interface.blueprints.api_v3 import (
+    checkout_branch,
+    is_valid_branch_name,
+    resolve_pull_command,
+)
+
+pytestmark = pytest.mark.skipif(
+    subprocess.run(['git', '--version'], capture_output=True).returncode != 0,
+    reason='git not available',
+)
+
+
+def _git(*args, cwd):
+    return subprocess.run(['git', *args], cwd=str(cwd),
+                          capture_output=True, text=True, check=True)
+
+
+@pytest.fixture()
+def repos(tmp_path):
+    """An 'origin' repo with a main branch, and a clone of it."""
+    origin = tmp_path / 'origin'
+    origin.mkdir()
+    _git('init', '--initial-branch=main', '--bare', cwd=origin)
+
+    work = tmp_path / 'work'
+    _git('clone', str(origin), str(work), cwd=tmp_path)
+    _git('config', 'user.email', 'test@example.com', cwd=work)
+    _git('config', 'user.name', 'Test', cwd=work)
+    (work / 'README.md').write_text('hello\n')
+    _git('add', 'README.md', cwd=work)
+    _git('commit', '-m', 'initial', cwd=work)
+    _git('push', '-u', 'origin', 'main', cwd=work)
+    return work
+
+
+def test_branch_with_upstream_uses_a_plain_pull(repos):
+    args, note, error = resolve_pull_command(str(repos))
+    assert error is None
+    assert args == ['git', 'pull', '--rebase']
+    assert note == ''
+
+
+def test_branch_without_upstream_falls_back_to_origin_branch(repos):
+    """The reported bug: a local branch that also exists on origin."""
+    _git('push', 'origin', 'main:audit', cwd=repos)
+    _git('fetch', 'origin', cwd=repos)
+    # A branch created this way has no tracking information.
+    _git('checkout', '-b', 'audit', cwd=repos)
+    assert subprocess.run(['git', 'rev-parse', '--abbrev-ref', '@{u}'],
+                          cwd=str(repos), capture_output=True).returncode != 0
+
+    args, note, error = resolve_pull_command(str(repos))
+    assert error is None
+    assert args == ['git', 'pull', '--rebase', 'origin', 'audit']
+    assert 'audit' in note
+
+
+def test_pull_fallback_actually_succeeds(repos):
+    """The fallback command must work, not merely look right."""
+    _git('push', 'origin', 'main:audit', cwd=repos)
+    _git('fetch', 'origin', cwd=repos)
+    _git('checkout', '-b', 'audit', cwd=repos)
+
+    args, _, error = resolve_pull_command(str(repos))
+    assert error is None
+    done = subprocess.run(args, cwd=str(repos), capture_output=True, text=True)
+    assert done.returncode == 0, done.stderr
+
+
+def test_local_only_branch_reports_an_actionable_message(repos):
+    """No upstream and no origin/<branch>: say so, don't just fail."""
+    _git('checkout', '-b', 'local-experiment', cwd=repos)
+    args, _, error = resolve_pull_command(str(repos))
+    assert args is None
+    assert error and 'local-experiment' in error
+    assert 'no origin/local-experiment' in error
+
+
+def test_detached_head_reports_an_actionable_message(repos):
+    head = subprocess.run(['git', 'rev-parse', 'HEAD'], cwd=str(repos),
+                          capture_output=True, text=True).stdout.strip()
+    _git('checkout', head, cwd=repos)
+    args, _, error = resolve_pull_command(str(repos))
+    assert args is None
+    assert error and 'detached HEAD' in error
+
+
+def test_missing_directory_does_not_raise(tmp_path):
+    """A bad path must return an error, not blow up the request."""
+    args, _, error = resolve_pull_command(str(tmp_path / 'nope'))
+    assert args is None
+    assert error
+
+
+# ── branch switching ────────────────────────────────────────────────────────
+
+
+@pytest.mark.parametrize('name', [
+    'main', 'audit', 'feat/thing', 'release-1.2', 'a_b.c',
+])
+def test_valid_branch_names_accepted(name):
+    assert is_valid_branch_name(name)
+
+
+@pytest.mark.parametrize('name', [
+    '', '   ', 'a b', 'a;rm -rf /', '--upload-pack=evil', '-x',
+    'a..b', 'a\nb', 'x' * 201, 'branch$(whoami)', '../escape',
+])
+def test_unsafe_branch_names_rejected(name):
+    """The value reaches a subprocess argument list, so refuse the exotic."""
+    assert not is_valid_branch_name(name)
+
+
+def test_switch_to_remote_only_branch_creates_it_with_tracking(repos):
+    _git('push', 'origin', 'main:release', cwd=repos)
+    _git('fetch', 'origin', cwd=repos)
+
+    payload, code = checkout_branch(str(repos), 'release')
+    assert code == 200 and payload['status'] == 'success', payload
+
+    assert _git('branch', '--show-current', cwd=repos).stdout.strip() == 'release'
+    upstream = subprocess.run(['git', 'rev-parse', '--abbrev-ref', '@{u}'],
+                              cwd=str(repos), capture_output=True, text=True)
+    assert upstream.stdout.strip() == 'origin/release'
+
+
+def test_switching_attaches_tracking_so_pull_needs_no_fallback(repos):
+    """The whole point: after switching, a plain `git pull` works."""
+    _git('push', 'origin', 'main:audit', cwd=repos)
+    _git('fetch', 'origin', cwd=repos)
+    payload, _ = checkout_branch(str(repos), 'audit')
+    assert payload['status'] == 'success'
+
+    args, note, error = resolve_pull_command(str(repos))
+    assert error is None
+    assert args == ['git', 'pull', '--rebase']
+    assert note == ''
+
+
+def test_unknown_branch_is_reported_not_created(repos):
+    payload, code = checkout_branch(str(repos), 'does-not-exist')
+    assert code == 404
+    assert 'does-not-exist' in payload['message']
+    assert _git('branch', '--show-current', cwd=repos).stdout.strip() == 'main'
+
+
+def test_local_edits_block_the_switch_and_name_the_files(repos):
+    _git('push', 'origin', 'main:other', cwd=repos)
+    _git('fetch', 'origin', cwd=repos)
+    _git('checkout', '-b', 'other', 'origin/other', cwd=repos)
+    (repos / 'README.md').write_text('changed on other\n')
+    _git('add', 'README.md', cwd=repos)
+    _git('commit', '-m', 'diverge', cwd=repos)
+    _git('checkout', 'main', cwd=repos)
+    (repos / 'README.md').write_text('uncommitted local edit\n')
+
+    payload, code = checkout_branch(str(repos), 'other')
+    assert code == 200 and payload['status'] == 'error'
+    assert payload['can_retry_with_stash'] is True
+    assert 'README.md' in payload['detail']
+    assert _git('branch', '--show-current', cwd=repos).stdout.strip() == 'main'
+
+
+def test_stash_option_lets_the_switch_through_and_keeps_the_work(repos):
+    """stash=True must switch *and* leave the edit recoverable."""
+    _git('push', 'origin', 'main:other', cwd=repos)
+    _git('fetch', 'origin', cwd=repos)
+    _git('checkout', '-b', 'other', 'origin/other', cwd=repos)
+    (repos / 'README.md').write_text('changed on other\n')
+    _git('add', 'README.md', cwd=repos)
+    _git('commit', '-m', 'diverge', cwd=repos)
+    _git('checkout', 'main', cwd=repos)
+    (repos / 'README.md').write_text('uncommitted local edit\n')
+
+    payload, code = checkout_branch(str(repos), 'other', stash=True)
+    assert code == 200 and payload['status'] == 'success', payload
+    assert 'stashed' in payload['message']
+    assert _git('branch', '--show-current', cwd=repos).stdout.strip() == 'other'
+    # The edit is not lost — it is on the stash.
+    assert 'switch to other' in _git('stash', 'list', cwd=repos).stdout

test/test_harness_empty_claimed.py

@@ -0,0 +1,103 @@
+"""Tests for the harness empty-frame check (src/plugin_system/testing/harness.py).
+
+The display controller skips a mode whose display() returns False and treats
+anything else -- including None -- as "content was shown". A mode that draws
+nothing without returning False is therefore never skipped, and since a mode
+switch clears the panel first, it sits on a blank screen for its whole display
+duration.
+
+Two sports plugins shipped exactly that: their display() returned None on every
+path, so an out-of-season league held a blank panel instead of being rotated
+past. The harness rendered those modes and passed them, because it discarded
+the return value entirely.
+"""
+
+from PIL import Image
+
+from src.plugin_system.testing.harness import RenderResult, check_empty_claimed
+
+
+def _blank(w=64, h=32):
+    return Image.new("RGB", (w, h), (0, 0, 0))
+
+
+def _drawn(w=64, h=32):
+    img = _blank(w, h)
+    img.paste(Image.new("RGB", (10, 10), (255, 255, 255)), (5, 5))
+    return img
+
+
+def _result(image, returned=None, **kw):
+    return RenderResult("p", 64, 32, "mode", image=image,
+                        display_returned=returned, **kw)
+
+
+class TestCheckEmptyClaimed:
+    def test_blank_frame_returning_none_is_flagged(self):
+        # The shape that shipped: nothing drawn, nothing reported.
+        r = _result(_blank(), returned=None)
+        check_empty_claimed([r])
+        assert r.empty_claimed is True
+
+    def test_blank_frame_returning_true_is_flagged(self):
+        # Just as broken, and more explicit about it.
+        r = _result(_blank(), returned=True)
+        check_empty_claimed([r])
+        assert r.empty_claimed is True
+
+    def test_blank_frame_returning_false_is_fine(self):
+        # The plugin correctly said "no content"; the controller will skip it.
+        r = _result(_blank(), returned=False)
+        check_empty_claimed([r])
+        assert r.empty_claimed is None
+
+    def test_a_drawn_frame_is_fine_whatever_it_returns(self):
+        for returned in (None, True, False):
+            r = _result(_drawn(), returned=returned)
+            check_empty_claimed([r])
+            assert r.empty_claimed is None, returned
+
+    def test_near_black_still_counts_as_drawn(self):
+        # Guard the threshold: content dim enough to look black to the eye is
+        # still content, and flagging it would train people to ignore this.
+        img = _blank()
+        img.paste(Image.new("RGB", (4, 4), (60, 60, 60)), (2, 2))
+        r = _result(img, returned=None)
+        check_empty_claimed([r])
+        assert r.empty_claimed is None
+
+
+class TestWarnVersusStrict:
+    def test_warn_only_by_default(self):
+        # A scroll mode's first frame is legitimately its blank scroll-in
+        # buffer, so this must not fail a run unless opted in.
+        r = _result(_blank(), returned=None)
+        check_empty_claimed([r])
+        assert r.empty_ok is None
+        assert r.ok is True
+
+    def test_strict_fails_the_result(self):
+        r = _result(_blank(), returned=None)
+        check_empty_claimed([r], strict=True)
+        assert r.empty_ok is False
+        assert r.ok is False
+
+    def test_strict_still_allows_an_honest_false(self):
+        r = _result(_blank(), returned=False)
+        check_empty_claimed([r], strict=True)
+        assert r.empty_ok is None
+        assert r.ok is True
+
+
+class TestSkippedResults:
+    def test_a_crashed_render_is_left_alone(self):
+        # error already fails the result; adding a second reason just muddies
+        # the report.
+        r = _result(None, returned=None, error="boom")
+        check_empty_claimed([r], strict=True)
+        assert r.empty_claimed is None
+
+    def test_a_result_with_no_image_is_left_alone(self):
+        r = _result(None, returned=None)
+        check_empty_claimed([r], strict=True)
+        assert r.empty_claimed is None

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_odds_request_budget.py

@@ -0,0 +1,122 @@
+"""Tests that a slow ESPN cannot take a whole plugin update with it.
+
+Odds are fetched per live game from inside SportsLive.update(), with show_odds
+defaulting on, and the plugin executor kills an operation at 30s. The odds
+request timeout was also 30s, so one stalled request consumed the entire budget
+and the update carrying every game's score was killed:
+
+    00:43:43  ERROR  plugin football-scoreboard operation timed out after 30.0s
+    01:43:43  ERROR  plugin football-scoreboard operation timed out after 30.0s
+
+Invisible out of season -- preseason week 1 returns a single game -- and a
+Sunday slate is around sixteen.
+"""
+
+from unittest.mock import Mock
+
+from src.base_odds_manager import BaseOddsManager
+
+PLUGIN_BUDGET = 30.0   # PluginExecutor(default_timeout=30.0)
+
+
+def _manager(cache=None):
+    cache = cache or Mock()
+    cache.get_with_auto_strategy.return_value = None
+    return BaseOddsManager(cache_manager=cache, config_manager=None)
+
+
+class TestRequestTimeout:
+    def test_leaves_room_in_the_operation_budget(self):
+        assert _manager().request_timeout < PLUGIN_BUDGET / 2
+
+    def test_the_timeout_is_the_one_actually_used(self):
+        m = _manager()
+        import src.base_odds_manager as mod
+        real = mod.requests.get
+        try:
+            mod.requests.get = Mock(side_effect=mod.requests.exceptions.Timeout("x"))
+            m.get_odds("football", "nfl", "401")
+            assert mod.requests.get.call_args.kwargs["timeout"] == m.request_timeout
+        finally:
+            mod.requests.get = real
+
+
+class TestSlowEspnCannotKillTheUpdate:
+    def test_one_failure_stops_the_rest_of_the_slate_hitting_the_network(self):
+        m = _manager()
+        import src.base_odds_manager as mod
+        real = mod.requests.get
+        calls = {"n": 0}
+
+        def timeout(*a, **k):
+            calls["n"] += 1
+            raise mod.requests.exceptions.Timeout("timed out")
+
+        try:
+            mod.requests.get = timeout
+            for i in range(16):          # a full slate, one game at a time
+                m.get_odds("football", "nfl", "4018730%02d" % i)
+        finally:
+            mod.requests.get = real
+
+        assert calls["n"] == 1, (
+            "%d games each paid the timeout; the breaker should have stopped "
+            "after the first" % calls["n"])
+
+    def test_worst_case_slate_stays_inside_the_budget(self):
+        m = _manager()
+        assert m.request_timeout * 1 < PLUGIN_BUDGET
+
+    def test_recovery_is_automatic(self):
+        m = _manager()
+        import src.base_odds_manager as mod
+        real_get, real_monotonic = mod.requests.get, mod.time.monotonic
+        clock = {"t": 1000.0}
+        try:
+            mod.time.monotonic = lambda: clock["t"]
+            mod.requests.get = Mock(
+                side_effect=mod.requests.exceptions.Timeout("timed out"))
+            m.get_odds("football", "nfl", "401")
+            assert m._skip_network_until > clock["t"], "breaker did not open"
+
+            clock["t"] += 1
+            before = mod.requests.get.call_count
+            m.get_odds("football", "nfl", "402")
+            assert mod.requests.get.call_count == before, "should not have retried"
+
+            clock["t"] += m._FAILURE_COOLDOWN
+            m.get_odds("football", "nfl", "403")
+            assert mod.requests.get.call_count > before, "never retried"
+        finally:
+            mod.requests.get, mod.time.monotonic = real_get, real_monotonic
+
+    def test_a_healthy_fetch_clears_the_breaker(self):
+        m = _manager()
+        m._skip_network_until = 0.0
+        m._extract_espn_data = Mock(return_value=None)
+        import src.base_odds_manager as mod
+        real = mod.requests.get
+        try:
+            resp = Mock()
+            resp.json.return_value = {}
+            resp.raise_for_status.return_value = None
+            mod.requests.get = Mock(return_value=resp)
+            m.get_odds("football", "nfl", "401")
+        finally:
+            mod.requests.get = real
+        assert m._skip_network_until == 0.0
+
+    def test_the_stale_cache_fallback_still_works(self):
+        # The failing request must still hand back whatever was cached; only
+        # the *subsequent* games skip the network.
+        cache = Mock()
+        cache.get_with_auto_strategy.side_effect = [None, {"details": "stale"}]
+        m = BaseOddsManager(cache_manager=cache, config_manager=None)
+        import src.base_odds_manager as mod
+        real = mod.requests.get
+        try:
+            mod.requests.get = Mock(
+                side_effect=mod.requests.exceptions.Timeout("timed out"))
+            assert m.get_odds("football", "nfl", "401") == {"details": "stale"}
+        finally:
+            mod.requests.get = real

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/test_vegas_density.py

@@ -781,6 +781,20 @@ class TestNewConfigKeys:
         assert cfg.render_width_pct == 100
         assert cfg.min_content_separation == 24
 
+    def test_width_cap_is_off_by_default(self):
+        # Capping made wide plugins resume mid-content on every appearance and
+        # emit runt final windows; it is now opt-in per plugin instead.
+        assert VegasModeConfig().max_plugin_width_ratio == 0.0
+        assert VegasModeConfig.from_config({}).max_plugin_width_ratio == 0.0
+
+    def test_width_cap_is_still_available_when_asked_for(self):
+        # Defaulting the cap off must not remove it: a user who sets a ratio
+        # still gets one, and 0 still means uncapped.
+        cfg = VegasModeConfig.from_config(
+            {'display': {'vegas_scroll': {'max_plugin_width_ratio': 3.0}}})
+        assert cfg.max_plugin_width_ratio == 3.0
+        assert cfg.validate() == []
+
     @pytest.mark.parametrize('overrides,bad_key', [
         ({'render_width_pct': 5}, 'render_width_pct'),
         ({'render_width_pct': 101}, 'render_width_pct'),
@@ -1643,3 +1657,228 @@ class TestPerPluginWidthBudget:
         strip = canvas([(0, 5000)], width=5000)
         adapter.get_content(NativePlugin([strip]), 'ticker')
         assert adapter._item_offsets.get('ticker', 0) > 0
+
+
+def ticker(item_widths, gap=32, height=DISPLAY_H):
+    """
+    A strip of discrete items separated by real gaps, like a news or stocks
+    ticker. Wide enough gaps that blank_runs() sees item boundaries, which is
+    what puts _crop_to_budget on its item-aligned path rather than treating the
+    strip as one continuous block.
+    """
+    width = sum(item_widths) + gap * (len(item_widths) - 1)
+    spans, x = [], 0
+    for w in item_widths:
+        spans.append((x, x + w))
+        x += w + gap
+    return canvas(spans, width=width, height=height)
+
+
+class TestTrailingRuntWindow:
+    """
+    A rotation's last window used to be whatever happened to be left over.
+    Measured on a live 512px panel, a 1,840px stocks ticker against a 1,536px
+    budget split 1,492 + 348 — the second pass showed seven seconds and cut.
+    """
+
+    def test_a_barely_oversized_strip_is_shown_whole(self):
+        adapter = adapter_with(content_padding=0, max_plugin_width_ratio=1.0)
+        # 1.2 budgets wide: splitting it can only ever produce a fragment.
+        strip = ticker([180] * 12)  # 2160 + 352 gaps = 2512px vs 512 budget
+        assert strip.width > DISPLAY_W
+
+        adapter = adapter_with(content_padding=0,
+                               max_plugin_width_ratio=strip.width / DISPLAY_W * 0.9)
+        shown = adapter.get_content(NativePlugin([strip]), 'stocks')[0]
+        assert shown.width == strip.width, "should absorb the runt, not split"
+        assert 'stocks' not in adapter._item_offsets
+
+    def test_no_window_in_a_rotation_is_a_fragment(self):
+        # Walk a long ticker all the way round; every pass must be worth
+        # showing rather than one of them being a leftover sliver.
+        adapter = adapter_with(content_padding=0, max_plugin_width_ratio=1.0)
+        strip = ticker([150] * 40)
+        plugin = NativePlugin([strip])
+
+        widths, seen_offsets = [], set()
+        for _ in range(20):
+            adapter.invalidate_cache('news')
+            widths.append(adapter.get_content(plugin, 'news')[0].width)
+            offset = adapter._item_offsets.get('news', 0)
+            if offset in seen_offsets:
+                break
+            seen_offsets.add(offset)
+
+        assert len(widths) > 1, "a strip this long must take several passes"
+        # Item snapping means an ordinary window lands short of the budget, so
+        # the bar is "not a sliver" rather than "a full budget".
+        assert min(widths) >= DISPLAY_W // 2, (
+            "no window should be a fragment, got %r" % widths)
+        assert max(widths) <= DISPLAY_W * 1.5, (
+            "absorbing a runt must stay bounded, got %r" % widths)
+
+    def test_a_continuous_image_also_absorbs_its_runt(self):
+        # The no-item-gaps path had the same leftover problem.
+        adapter = adapter_with(content_padding=0, max_plugin_width_ratio=1.0)
+        solid = canvas([(0, 700)], width=700)  # 512 budget -> 512 + 188 runt
+        first = adapter.get_content(NativePlugin([solid]), 'chart')[0]
+        assert first.width == 700, "188px tail is not worth its own pass"
+        assert 'chart' not in adapter._item_offsets
+
+    def test_the_reported_stocks_case(self):
+        # The exact numbers logged on a 512px panel: an 1,840px stocks ticker
+        # against a 1,536px budget split 1,492 + 348, so every other appearance
+        # showed seven seconds of stocks and cut. It should now come through in
+        # one piece, 20% over budget being the better of the two outcomes.
+        adapter = adapter_with(content_padding=0, max_plugin_width_ratio=3.0)
+        # 10 items of 152px with 32px gaps = 1520 + 288 = 1808, near enough.
+        strip = ticker([152] * 10)
+        assert DISPLAY_W * 3 < strip.width < DISPLAY_W * 4
+
+        widths = []
+        for _ in range(3):
+            adapter.invalidate_cache('stocks')
+            widths.append(adapter.get_content(
+                NativePlugin([strip]), 'stocks')[0].width)
+
+        assert widths == [strip.width] * 3, (
+            "a strip this close to the budget should be shown whole every "
+            "time, not split into a big pass and a sliver; got %r" % widths)
+
+    def test_a_short_final_row_window_is_not_left_alone(self):
+        # The multi-row path has the same fault as the single-image one, and
+        # wrapping does not save it: rows of 450/450/100 against a 512px budget
+        # gave the 100 a pass of its own, two seconds against nine, because the
+        # row it wrapped to did not fit either.
+        adapter = adapter_with(content_padding=0, max_plugin_width_ratio=1.0,
+                               intra_plugin_gap=0, min_content_separation=0)
+        rows = [canvas([(0, 450)], width=450),
+                canvas([(0, 450)], width=450),
+                canvas([(0, 100)], width=100)]
+
+        widths = []
+        for _ in range(6):
+            adapter.invalidate_cache('rows')
+            shown = adapter.get_content(NativePlugin(list(rows)), 'rows')
+            widths.append(sum(img.width for img in shown))
+
+        assert min(widths) >= DISPLAY_W // 2, (
+            "a row window should not be a sliver, got %r" % widths)
+        assert max(widths) <= DISPLAY_W * 1.5, (
+            "absorbing a short row must stay bounded, got %r" % widths)
+
+    def test_row_rotation_still_covers_every_row(self):
+        # Absorbing a short tail must not drop rows from the rotation.
+        adapter = adapter_with(content_padding=0, max_plugin_width_ratio=1.0,
+                               intra_plugin_gap=0, min_content_separation=0)
+        rows = [canvas([(0, 450)], width=450),
+                canvas([(0, 450)], width=450),
+                canvas([(0, 100)], width=100)]
+
+        seen = set()
+        for _ in range(8):
+            adapter.invalidate_cache('rows')
+            for img in adapter.get_content(NativePlugin(list(rows)), 'rows'):
+                seen.add(img.width)
+        assert seen == {450, 100}, "rotation never showed every row: %r" % seen
+
+    def test_a_row_too_wide_to_absorb_still_bounds_the_overrun(self):
+        # When the next row cannot be taken without blowing past 1.5 budgets,
+        # a short window is the lesser evil — the same trade the always-show-
+        # the-first-row rule already makes.
+        adapter = adapter_with(content_padding=0, max_plugin_width_ratio=1.0,
+                               intra_plugin_gap=0, min_content_separation=0)
+        rows = [canvas([(0, 900)], width=900), canvas([(0, 100)], width=100)]
+        for _ in range(4):
+            adapter.invalidate_cache('wide')
+            shown = adapter.get_content(NativePlugin(list(rows)), 'wide')
+            assert sum(i.width for i in shown) <= 900, (
+                "must not merge a row that overruns the cap")
+
+    def test_a_genuinely_long_strip_still_gets_capped(self):
+        # Absorbing runts must not become "never cap anything".
+        adapter = adapter_with(content_padding=0, max_plugin_width_ratio=1.0)
+        strip = ticker([150] * 60)
+        shown = adapter.get_content(NativePlugin([strip]), 'long')[0]
+        assert shown.width < strip.width
+        assert shown.width <= DISPLAY_W * 2
+
+
+class TestOffsetOutlivesItsContent:
+    """
+    A rotation offset only means something against the content it was recorded
+    against. news re-rendered 9,793px -> 9,505px mid-rotation while its stored
+    column kept advancing, so the window pointed into unrelated headlines.
+    """
+
+    def test_rotation_survives_items_changing_width(self):
+        # Same items, each a little wider — a price gaining a digit. The window
+        # should resume at the same *item*, not at a now-meaningless column.
+        adapter = adapter_with(content_padding=0, max_plugin_width_ratio=1.0)
+        adapter.get_content(NativePlugin([ticker([150] * 40)]), 'stocks')
+        first = adapter._item_offsets.get('stocks')
+        assert first, "the first pass should leave a resume point"
+
+        adapter.invalidate_cache('stocks')
+        adapter.get_content(NativePlugin([ticker([158] * 40)]), 'stocks')
+        assert adapter._item_offsets.get('stocks', 0) > first, (
+            "same item count means the offset still applies and should advance")
+
+    def test_rotation_restarts_when_the_item_count_changes(self):
+        adapter = adapter_with(content_padding=0, max_plugin_width_ratio=1.0)
+        adapter.get_content(NativePlugin([ticker([150] * 40)]), 'news')
+        assert adapter._item_offsets.get('news', 0) > 0
+
+        # A fresh headline set with fewer entries: the old position is
+        # meaningless, so the next pass starts at the top.
+        adapter.invalidate_cache('news')
+        shown = adapter.get_content(NativePlugin([ticker([150] * 25)]), 'news')[0]
+        expected = adapter.get_content(
+            NativePlugin([ticker([150] * 25)]), 'fresh')[0]
+        assert shown.width == expected.width
+
+    def test_a_row_index_is_never_read_back_as_a_pixel_column(self):
+        # The unit collision: _apply_width_budget stores an index into a list
+        # of rows, _crop_to_budget a column in one image, under the same key.
+        adapter = adapter_with(content_padding=0, max_plugin_width_ratio=1.0,
+                               intra_plugin_gap=0, min_content_separation=0)
+        rows = [canvas([(0, 200)], width=200) for _ in range(8)]
+        adapter.get_content(NativePlugin(rows), 'mixed')
+        assert adapter._item_offsets.get('mixed', 0) > 0
+        assert adapter._offset_shapes['mixed'][0] == 'rows'
+
+        # Now the same plugin returns one wide strip instead. The row index
+        # must not be read as a column into it: the strip is entered at the
+        # top, exactly as it would be for a plugin with no history at all.
+        strip = ticker([150] * 40)
+        adapter.invalidate_cache('mixed')
+        carried = adapter.get_content(NativePlugin([strip]), 'mixed')[0]
+        assert adapter._offset_shapes['mixed'][0] == 'cuts'
+
+        clean = adapter_with(content_padding=0, max_plugin_width_ratio=1.0,
+                             intra_plugin_gap=0, min_content_separation=0)
+        assert carried.tobytes() == clean.get_content(
+            NativePlugin([strip]), 'clean')[0].tobytes()
+
+    def test_a_stale_index_past_the_end_restarts(self):
+        adapter = adapter_with(content_padding=0, max_plugin_width_ratio=1.0)
+        strip = ticker([150] * 40)
+        adapter.get_content(NativePlugin([strip]), 'news')
+        # Force an index far beyond anything the current strip has, keeping the
+        # shape intact so the guard does not catch it first.
+        shape = adapter._offset_shapes['news']
+        adapter._item_offsets['news'] = 10_000
+        adapter.invalidate_cache('news')
+        shown = adapter.get_content(NativePlugin([strip]), 'news')[0]
+        assert shown.width > 0
+        assert adapter._offset_shapes['news'] == shape
+
+    def test_content_that_fits_clears_both_offset_and_shape(self):
+        adapter = adapter_with(content_padding=0, max_plugin_width_ratio=1.0)
+        adapter.get_content(NativePlugin([ticker([150] * 40)]), 'shrink')
+        assert 'shrink' in adapter._offset_shapes
+
+        adapter.invalidate_cache('shrink')
+        adapter.get_content(NativePlugin([canvas([(0, 100)], width=100)]), 'shrink')
+        assert 'shrink' not in adapter._item_offsets
+        assert 'shrink' not in adapter._offset_shapes

test/test_vegas_stall_watchdog.py

@@ -0,0 +1,119 @@
+"""Tests the watchdog that catches a stalled render loop in the act.
+
+Frame-time statistics can say a stall happened but not what caused it, and by
+the time the numbers reach the log the stack is long gone. On a live rig the
+Vegas loop showed a 3.2s freeze roughly twice an hour with every other frame
+under 25ms -- invisible in the mean, and unattributable from the log alone.
+This watchdog samples every thread's stack while the loop is still wedged,
+which is how that freeze was traced to a plugin generating a 17,000px scroll
+image, logo PNG decode and all, on the render thread.
+"""
+
+import threading
+import time
+
+import pytest
+
+from src.vegas_mode.coordinator import _StallWatchdog
+
+
+@pytest.fixture
+def watchdog():
+    made = []
+
+    def build(threshold):
+        w = _StallWatchdog(threshold)
+        made.append(w)
+        return w
+
+    yield build
+    for w in made:
+        w.stop()
+    for w in made:
+        w._thread.join(timeout=2.0)
+        assert not w._thread.is_alive(), "watchdog thread outlived its owner"
+
+
+def _dumps(caplog):
+    return [r for r in caplog.records if 'render loop stalled' in r.getMessage()]
+
+
+class TestItFiresOnlyWhenStalled:
+    def test_a_beating_loop_is_never_reported(self, watchdog, caplog):
+        w = watchdog(0.2)
+        deadline = time.time() + 0.9
+        while time.time() < deadline:
+            w.beat()
+            time.sleep(0.02)
+        assert not _dumps(caplog)
+
+    def test_a_stalled_loop_is_reported(self, watchdog, caplog):
+        w = watchdog(0.2)
+        w.beat()
+        time.sleep(0.9)
+        assert _dumps(caplog), "no stall dump for a loop that stopped beating"
+
+    def test_one_dump_per_stall_not_per_poll(self, watchdog, caplog):
+        # The watchdog polls at threshold/4, so a stall lasting many poll
+        # intervals must not flood the log with a dump each time.
+        w = watchdog(0.2)
+        w.beat()
+        time.sleep(1.2)
+        assert len(_dumps(caplog)) == 1, (
+            "%d dumps for one stall" % len(_dumps(caplog)))
+
+    def test_a_later_stall_is_reported_again(self, watchdog, caplog):
+        w = watchdog(0.2)
+        w.beat()
+        time.sleep(0.6)
+        first = len(_dumps(caplog))
+        w.beat()                      # recovered
+        time.sleep(0.6)               # then stalled again
+        assert len(_dumps(caplog)) == first + 1
+
+
+class TestWhatItReports:
+    def test_the_dump_names_threads_and_shows_frames(self, watchdog, caplog):
+        started = threading.Event()
+        release = threading.Event()
+
+        def parked():
+            started.set()
+            release.wait(3.0)
+
+        t = threading.Thread(target=parked, name="CulpritThread", daemon=True)
+        t.start()
+        started.wait(2.0)
+        try:
+            w = watchdog(0.2)
+            w.beat()
+            time.sleep(0.7)
+            dumps = _dumps(caplog)
+            assert dumps
+            text = dumps[0].getMessage()
+            assert "CulpritThread" in text, text
+            assert " in " in text, "no frames in the dump"
+            assert ".py:" in text, "no file:line in the dump"
+        finally:
+            release.set()
+            t.join(timeout=2.0)
+
+    def test_it_reports_how_long_the_stall_ran(self, watchdog, caplog):
+        w = watchdog(0.2)
+        w.beat()
+        time.sleep(0.8)
+        text = _dumps(caplog)[0].getMessage()
+        assert "stalled" in text
+        # Long enough to have tripped, and not an absurd value.
+        stalled = float(text.split("stalled")[1].split("s")[0])
+        assert 0.2 <= stalled <= 3.0, stalled
+
+
+class TestItIsCheapWhenIdle:
+    def test_stop_is_prompt(self, caplog):
+        w = _StallWatchdog(4.0)          # long threshold, long poll interval
+        t0 = time.time()
+        w.stop()
+        w._thread.join(timeout=3.0)
+        assert not w._thread.is_alive(), "stop() did not end the thread"
+        assert time.time() - t0 < 2.0, "stop() waited out the poll interval"

test/test_web_error_detail.py

@@ -0,0 +1,248 @@
+"""Tests for surfacing the underlying error in web responses.
+
+Regression under test: every failing endpoint returned "An error occurred; see
+logs for details" and nothing else. On a device whose storage was failing that
+sentence came back from the restart action, from /system/status, and from
+/logs -- the log viewer itself -- because journalctl could not be executed. The
+exception underneath said `[Errno 5] Input/output error: 'systemctl'`, which
+names the fault outright, and nine handlers were discarding it entirely rather
+than even logging it.
+"""
+
+import pytest
+
+from src.web_interface.error_handler import describe_exception
+
+
+class TestDescribeException:
+    def test_names_the_type_and_message(self):
+        detail = describe_exception(OSError(5, "Input/output error", "systemctl"))
+        assert detail == "OSError: [Errno 5] Input/output error: 'systemctl'"
+
+    def test_the_reported_failure_is_legible(self):
+        # The whole point: this string is the diagnosis.
+        assert "Input/output error" in describe_exception(
+            OSError(5, "Input/output error", "systemctl"))
+
+    def test_a_bare_exception_still_names_its_type(self):
+        # A PermissionError with no message still says more than "unknown".
+        assert describe_exception(PermissionError()) == "PermissionError"
+        assert describe_exception(Exception()) == "Exception"
+
+    def test_message_is_kept_when_present(self):
+        assert describe_exception(ValueError("bad port")) == "ValueError: bad port"
+
+
+class TestCredentialRedaction:
+    """Exception text quotes URLs, and plugins authenticate by query string."""
+
+    @pytest.mark.parametrize("secret_text,leaked", [
+        ("failed: https://api.x.com/v1?api_key=SEC123&city=Tampa", "SEC123"),
+        ("token=abcdef123456 was rejected", "abcdef123456"),
+        ("connect failed password=hunter2", "hunter2"),
+        ("GET /?access_token=zzz999", "zzz999"),
+        ('{"secret": "topsecret"}', "topsecret"),
+        # requests quotes the URL it failed on, and both of these forms turn
+        # up in real client exceptions.
+        ("401 for https://user:hunter2@example.com/api", "hunter2"),
+        ("headers: {'Authorization': 'Bearer eyJ.SECRET.sig'}", "eyJ.SECRET.sig"),
+        ("Authorization: Basic dXNlcjpwYXNzd29yZA==", "dXNlcjpwYXNzd29yZA=="),
+        ("Proxy-Authorization: Bearer ptok999", "ptok999"),
+        # Any scheme, not a fixed list -- a list silently leaks whatever it
+        # does not name, and plugin APIs invent their own.
+        ("Authorization: ApiKey SECRET123", "SECRET123"),
+        ("Authorization: Negotiate YIIZnegotiateblob", "YIIZnegotiateblob"),
+        ("Authorization: NTLM TlRMTVNTUAAB", "TlRMTVNTUAAB"),
+        ("authorization: barecredential", "barecredential"),
+    ])
+    def test_credentials_never_reach_the_response(self, secret_text, leaked):
+        detail = describe_exception(RuntimeError(secret_text))
+        assert leaked not in detail
+        assert "<redacted>" in detail
+
+    def test_the_parameter_name_survives_redaction(self):
+        # Knowing *which* credential was involved is part of the diagnosis.
+        detail = describe_exception(RuntimeError("https://x/y?api_key=SEC123"))
+        assert "api_key" in detail
+
+    def test_unknown_schemes_keep_their_name(self):
+        for scheme in ("ApiKey", "Negotiate", "NTLM", "AWS4-HMAC-SHA256"):
+            detail = describe_exception(
+                RuntimeError("Authorization: %s SECRETVALUE" % scheme))
+            assert scheme in detail, detail
+            assert "SECRETVALUE" not in detail, detail
+
+    def test_auth_scheme_and_username_survive(self):
+        # Which kind of credential, and whose, without the credential itself.
+        assert "Bearer" in describe_exception(
+            RuntimeError("Authorization: Bearer eyJ.SECRET.sig"))
+        assert "user" in describe_exception(
+            RuntimeError("https://user:hunter2@example.com"))
+
+    def test_non_secret_context_is_preserved(self):
+        detail = describe_exception(RuntimeError("https://api.x.com/v1?city=Tampa"))
+        assert "city=Tampa" in detail
+        assert "<redacted>" not in detail
+
+
+class TestBounds:
+    def test_long_messages_are_truncated(self):
+        detail = describe_exception(ValueError("x" * 5000))
+        assert len(detail) <= 400
+
+    def test_newlines_are_collapsed_to_one_line(self):
+        detail = describe_exception(ValueError("line one\nline two\tthree"))
+        assert "\n" not in detail and "\t" not in detail
+        assert detail == "ValueError: line one line two three"
+
+    def test_custom_length_is_honoured(self):
+        assert len(describe_exception(ValueError("y" * 500), max_length=50)) <= 50
+
+
+class TestHandlersCarryDetail:
+    """The response shape callers actually see."""
+
+    def test_no_api_v3_handler_discards_its_exception(self):
+        """Every generic-message handler must log a traceback and return detail.
+
+        Nine of them bound `e` and never used it, so the promised log entry was
+        never written either. Checking merely that *something* was logged is
+        too weak -- a `logger.info("failed")` would satisfy it while throwing
+        the exception away just as completely, so this asserts the two things
+        that actually make the failure diagnosable: an error-level record with
+        the traceback, and the sanitized detail in the response.
+        """
+        import ast
+
+        src = open("web_interface/blueprints/api_v3.py").read()
+        tree = ast.parse(src)
+        generic = "An error occurred; see logs for details"
+
+        def logs_a_traceback(handler):
+            """An error/exception-level log call carrying exc_info."""
+            for call in [n for n in ast.walk(handler) if isinstance(n, ast.Call)]:
+                func = call.func
+                if not isinstance(func, ast.Attribute):
+                    continue
+                if func.attr == "exception":       # implies exc_info
+                    return True
+                if func.attr not in ("error", "critical"):
+                    continue
+                if any(kw.arg == "exc_info" and getattr(kw.value, "value", False) is True
+                       for kw in call.keywords):
+                    return True
+            return False
+
+        def describes_this_exception(node, bound):
+            """A describe_exception(<bound>) call anywhere under `node`."""
+            for call in [n for n in ast.walk(node) if isinstance(n, ast.Call)]:
+                if not (isinstance(call.func, ast.Name)
+                        and call.func.id == "describe_exception"):
+                    continue
+                if bound is None:
+                    return True     # bare `except:` cannot name it; accept
+                if any(isinstance(a, ast.Name) and a.id == bound
+                       for a in call.args):
+                    return True
+            return False
+
+        def returns_the_detail(handler):
+            """The detail must be inside what the handler actually returns.
+
+            Looking anywhere in the handler is too weak: a handler could
+            compute describe_exception(e), drop it on the floor, and return the
+            generic message with no details field, while still passing. So the
+            call has to appear within a `return` expression.
+            """
+            returns = [n for n in ast.walk(handler) if isinstance(n, ast.Return)]
+            if not returns:
+                return False
+            return all(describes_this_exception(r, handler.name) for r in returns)
+
+        offenders = []
+        for h in [n for n in ast.walk(tree) if isinstance(n, ast.ExceptHandler)]:
+            seg = ast.get_source_segment(src, h) or ""
+            if generic not in seg:
+                continue
+            missing = []
+            if not logs_a_traceback(h):
+                missing.append("error-level log with exc_info")
+            if not returns_the_detail(h):
+                missing.append("describe_exception(e) in the response")
+            if missing:
+                offenders.append((h.lineno, missing))
+
+        assert not offenders, (
+            "handlers returning the generic message without %s: %r"
+            % ("both a traceback log and the detail", offenders))
+
+    def test_client_errors_keep_their_own_status(self):
+        """A 405 must not be reported as a server-side UNKNOWN_ERROR.
+
+        Werkzeug's HTTPExceptions subclass Exception, so the catch-all saw them
+        too: a GET on a POST-only route came back 500 "an error occurred",
+        which tells the caller nothing and blames the wrong side. Found while
+        probing a device whose POST-only config endpoints answered every GET
+        with UNKNOWN_ERROR.
+        """
+        from flask import Flask, jsonify
+        from werkzeug.exceptions import HTTPException
+
+        app = Flask(__name__)
+
+        @app.errorhandler(Exception)
+        def handle(error):
+            if isinstance(error, HTTPException):
+                return jsonify({
+                    "status": "error",
+                    "error_code": (error.name or "HTTP_ERROR").upper().replace(" ", "_"),
+                    "message": error.description,
+                }), error.code or 500
+            return jsonify({
+                "status": "error",
+                "error_code": "UNKNOWN_ERROR",
+                "message": "An error occurred; see logs for details",
+                "details": describe_exception(error),
+            }), 500
+
+        @app.route("/only-post", methods=["POST"])
+        def only_post():
+            return jsonify({"ok": True})
+
+        @app.route("/boom")
+        def boom():
+            raise OSError(5, "Input/output error", "systemctl")
+
+        client = app.test_client()
+
+        resp = client.get("/only-post")
+        assert resp.status_code == 405, "a wrong method must stay a 405"
+        assert resp.get_json()["error_code"] == "METHOD_NOT_ALLOWED"
+
+        # A genuine server fault still reports as one, with its detail.
+        resp = client.get("/boom")
+        assert resp.status_code == 500
+        assert "Input/output error" in resp.get_json()["details"]
+
+    def test_global_handler_reports_the_underlying_error(self):
+        from flask import Flask, jsonify
+
+        app = Flask(__name__)
+
+        @app.errorhandler(Exception)
+        def handle(error):
+            return jsonify({
+                "status": "error",
+                "error_code": "UNKNOWN_ERROR",
+                "message": "An error occurred; see logs for details",
+                "details": describe_exception(error),
+            }), 500
+
+        @app.route("/boom")
+        def boom():
+            raise OSError(5, "Input/output error", "systemctl")
+
+        client = app.test_client()
+        body = client.get("/boom").get_json()
+        assert body["error_code"] == "UNKNOWN_ERROR"
+        assert "Input/output error" in body["details"]

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/app.py

@@ -16,6 +16,8 @@ from datetime import datetime, timedelta
 sys.path.insert(0, str(Path(__file__).parent.parent))
 
 from src.config_manager import ConfigManager
+from src.web_interface.error_handler import describe_exception
+from werkzeug.exceptions import HTTPException
 from src.exceptions import ConfigError
 from src.plugin_system.plugin_manager import PluginManager
 from src.plugin_system.store_manager import PluginStoreManager
@@ -391,15 +393,42 @@ def internal_error(error):
     import logging
     logger = logging.getLogger('web_interface')
     logger.error("Internal server error", exc_info=True)
-    return jsonify({
+    payload = {
         'status': 'error',
         'error_code': 'INTERNAL_ERROR',
         'message': 'An internal error occurred; see logs for details',
-    }), 500
+    }
+    # Flask hands the original exception over as `error.original_exception`
+    # when propagation is off; without it there is nothing to describe.
+    original = getattr(error, 'original_exception', None) or (
+        error if isinstance(error, BaseException) else None)
+    if original is not None:
+        payload['details'] = describe_exception(original)
+    return jsonify(payload), 500
 
 @app.errorhandler(Exception)
 def handle_exception(error):
-    """Handle all unhandled exceptions."""
+    """Handle all unhandled exceptions.
+
+    Returning only "see logs for details" is fine until the logs are exactly
+    what you cannot reach. A device with failing storage answered every
+    endpoint with that sentence -- including the log viewer, because journalctl
+    could not be executed -- while the exception underneath said
+    `[Errno 5] Input/output error`. Naming the error costs nothing here and is
+    frequently the whole diagnosis, so include it alongside the log pointer.
+    """
+    # Werkzeug's HTTPExceptions subclass Exception, so this catch-all sees
+    # them too and was reporting every 405, 400, 413 and 415 as a server-side
+    # UNKNOWN_ERROR 500. A GET on a POST-only route came back as "an error
+    # occurred" rather than "method not allowed", which tells the caller
+    # nothing and blames the wrong side. Hand those back as themselves.
+    if isinstance(error, HTTPException):
+        return jsonify({
+            'status': 'error',
+            'error_code': (error.name or 'HTTP_ERROR').upper().replace(' ', '_'),
+            'message': error.description,
+        }), error.code or 500
+
     import logging
     logger = logging.getLogger('web_interface')
     logger.error("Unhandled exception", exc_info=True)
@@ -407,6 +436,7 @@ def handle_exception(error):
         'status': 'error',
         'error_code': 'UNKNOWN_ERROR',
         'message': 'An error occurred; see logs for details',
+        'details': describe_exception(error),
     }), 500
 
 # Captive portal redirect middleware

web_interface/static/v3/js/widgets/array-table.js

@@ -160,6 +160,24 @@
 
     // ─── Cell rendering ─────────────────────────────────────────────────────
 
+    /**
+     * Visible text for one enum option.
+     *
+     * Mirrors the server-rendered table in plugin_config.html: a schema may
+     * supply x-options.labels, and anything unlabelled falls back to the raw
+     * value. Rows added here must match rows rendered by the template, or the
+     * same column would read differently before and after a page reload.
+     *
+     * @param {Object} colDef  column (or property) schema
+     * @param {*}      opt     the enum value
+     * @returns {string} label to display
+     */
+    function enumOptionLabel(colDef, opt) {
+        const options = (colDef && (colDef['x-options'] || colDef['x_options'])) || {};
+        const labels = options.labels || {};
+        return Object.prototype.hasOwnProperty.call(labels, opt) ? labels[opt] : opt;
+    }
+
     /**
      * Create one <td> for a display column.
      */
@@ -219,7 +237,7 @@
                 if (opt === null) return;
                 const o = document.createElement('option');
                 o.value    = opt;
-                o.textContent = opt;
+                o.textContent = enumOptionLabel(colDef, opt);
                 if (String(colValue) === String(opt)) o.selected = true;
                 sel.appendChild(o);
             });
@@ -646,7 +664,7 @@
             enumVals.forEach(opt => {
                 if (opt === null) return;
                 const o = document.createElement('option');
-                o.value = opt; o.textContent = opt;
+                o.value = opt; o.textContent = enumOptionLabel(schema, opt);
                 if (String(currentVal) === String(opt)) o.selected = true;
                 sel.appendChild(o);
             });

web_interface/templates/v3/partials/display.html

@@ -556,11 +556,11 @@
                         </div>
 
                         <div class="form-group" id="setting-display-vegas_max_plugin_width_ratio" data-setting-key="display.vegas_scroll.max_plugin_width_ratio">
-                            <label for="vegas_max_plugin_width_ratio" class="block text-sm font-medium text-gray-700">Max Plugin Width (screens){{ ui.help_tip('Caps how much of one cycle a single plugin may occupy, measured in screen widths (0–20).\nDefault: 3. A long ticker such as a news feed or leaderboard is trimmed to this and the remainder shown on later cycles, so one plugin cannot hold the display for minutes. Set 0 for no limit.', 'Max Plugin Width') }}</label>
+                            <label for="vegas_max_plugin_width_ratio" class="block text-sm font-medium text-gray-700">Max Plugin Width (screens){{ ui.help_tip('Caps how much of one cycle a single plugin may occupy, measured in screen widths (0–20).\nDefault: 0 (no limit) — every plugin shows all of its content and always starts at the beginning.\nSet a limit to stop one long ticker holding the display for minutes: it is cut to this width and the remainder shown on later cycles. The trade-off is that such a plugin then resumes mid-content on each appearance instead of starting fresh.', 'Max Plugin Width') }}</label>
                             <input type="number"
                                    id="vegas_max_plugin_width_ratio"
                                    name="vegas_max_plugin_width_ratio"
-                                   value="{{ main_config.display.get('vegas_scroll', {}).get('max_plugin_width_ratio', 3.0) }}"
+                                   value="{{ main_config.display.get('vegas_scroll', {}).get('max_plugin_width_ratio', 0.0) }}"
                                    min="0"
                                    max="20"
                                    step="0.5"

web_interface/templates/v3/partials/plugin_config.html

@@ -121,14 +121,21 @@
             </label>
             {% endif %}
             
-            {# Enum dropdown #}
+            {# Enum dropdown. Option text comes from x-options.labels when the
+               schema supplies it -- the same convention the checkbox-group
+               widget already uses -- because humanising the raw value cannot
+               express every label: "vs" reads as "Vs", and "abbrev" says
+               nothing about the "Sep 19" it produces. Values without a label
+               fall back to the humanised form, so existing schemas render
+               exactly as before. #}
             {% elif prop.enum %}
+            {% set enum_labels = (prop.get('x-options') or prop.get('x_options') or {}).get('labels') or {} %}
             <select id="{{ field_id }}"
-                    name="{{ full_key }}" 
+                    name="{{ full_key }}"
                     class="form-select w-full rounded-md border-gray-300 shadow-sm focus:border-blue-500 focus:ring-blue-500 bg-white text-black">
                 {% for option in prop.enum %}
                 <option value="{{ option }}" {% if value == option %}selected{% endif %}>
-                    {{ option|replace('_', ' ')|title }}
+                    {{ enum_labels.get(option, option|replace('_', ' ')|title) }}
                 </option>
                 {% endfor %}
             </select>
@@ -569,10 +576,14 @@
                                                        class="block w-20 px-2 py-1 border border-gray-300 rounded text-sm text-center"
                                                        {% if col_def.get('description') %}title="{{ col_def.get('description') }}"{% endif %}>
                                             {% elif col_enum %}
+                                                {# Labels are opt-in here and the fallback stays the raw
+                                                   value: table columns hold things like ticker symbols,
+                                                   which must not be title-cased behind the user's back. #}
+                                                {% set col_labels = (col_def.get('x-options') or col_def.get('x_options') or {}).get('labels') or {} %}
                                                 <select name="{{ full_key }}.{{ item_index }}.{{ col_name }}"
                                                         class="block w-full px-2 py-1 border border-gray-300 rounded text-sm bg-white">
                                                     {% for opt in col_enum %}{% if opt is not none %}
-                                                    <option value="{{ opt }}" {% if col_value == opt or (col_value is none and col_def.get('default') == opt) %}selected{% endif %}>{{ opt }}</option>
+                                                    <option value="{{ opt }}" {% if col_value == opt or (col_value is none and col_def.get('default') == opt) %}selected{% endif %}>{{ col_labels.get(opt, opt) }}</option>
                                                     {% endif %}{% endfor %}
                                                 </select>
                                             {% elif col_xwidget == 'date-picker' %}

web_interface/templates/v3/partials/tools.html

@@ -31,6 +31,24 @@
         </div>
 
         <div class="space-y-4">
+            <!-- Switch branch -->
+            <div class="flex items-start justify-between gap-4">
+                <div class="min-w-0">
+                    <p class="text-sm font-medium text-gray-900">Branch</p>
+                    <p class="text-xs text-gray-500 mt-0.5">Choose which branch this pi follows. Switching attaches tracking, so Pull Latest works afterwards.</p>
+                </div>
+                <div class="shrink-0 flex items-center gap-2">
+                    <select id="branch-select" class="text-sm border border-gray-300 rounded-md px-2 py-2 bg-white max-w-[14rem]">
+                        <option value="">Loading branches…</option>
+                    </select>
+                    <button id="btn-checkout-branch" onclick="checkoutBranch(false)"
+                            class="inline-flex items-center px-3 py-2 border border-gray-300 text-sm font-medium rounded-md text-gray-700 bg-white hover:bg-gray-50">
+                        <i class="fas fa-code-branch mr-2"></i>Switch
+                    </button>
+                </div>
+            </div>
+            <div id="result-checkout-branch" class="hidden"></div>
+
             <!-- Pull latest -->
             <div class="flex items-start justify-between gap-4">
                 <div>
@@ -467,6 +485,14 @@
                     </div>`;
                 }
 
+                if (d.upstream) {
+                    html += `<p class="text-xs text-gray-500 mt-1"><i class="fas fa-link mr-1"></i>tracking <span class="font-mono">${escHtml(d.upstream)}</span></p>`;
+                } else if (d.can_pull) {
+                    html += `<p class="text-xs text-blue-700 mt-1"><i class="fas fa-info-circle mr-1"></i>No upstream set; Pull Latest will use <span class="font-mono">origin/${escHtml(d.branch || '')}</span> and set it.</p>`;
+                } else {
+                    html += `<p class="text-xs text-amber-700 mt-1"><i class="fas fa-triangle-exclamation mr-1"></i>No upstream and no matching branch on origin — Pull Latest cannot run. Switch to a branch that exists on the remote.</p>`;
+                }
+
                 if (d.remote_url) {
                     html += `<p class="text-xs text-gray-400 mt-1"><i class="fas fa-cloud mr-1"></i>${escHtml(d.remote_url)}</p>`;
                 }
@@ -479,6 +505,84 @@
             });
     }
 
+    // ── branch picker ─────────────────────────────────────────────────────
+    // A pi can end up on a branch with no tracking information (checked out by
+    // name, restored from a backup), where `git pull` refuses to run. Being
+    // able to see and change the branch from here beats needing SSH.
+
+    function loadBranches() {
+        const sel = document.getElementById('branch-select');
+        if (!sel) return;
+
+        fetch('/api/v3/system/git-branches')
+            .then(r => r.ok ? r.json() : r.json().then(d => Promise.reject(d.message || `HTTP ${r.status}`)))
+            .then(d => {
+                if (d.status === 'error') {
+                    sel.innerHTML = `<option value="">${escHtml(d.message || 'unavailable')}</option>`;
+                    sel.disabled = true;
+                    return;
+                }
+                sel.innerHTML = '';
+                const add = (name, suffix) => {
+                    const o = document.createElement('option');
+                    o.value = name;
+                    o.textContent = name + (suffix || '');
+                    if (name === d.current) o.selected = true;
+                    sel.appendChild(o);
+                };
+                (d.local || []).forEach(b => add(b, b === d.current ? '  (current)' : ''));
+                // Remote-only branches are checked out on demand.
+                (d.remote_only || []).forEach(b => add(b, '  (remote)'));
+                if (!sel.options.length) add('', 'no branches found');
+            })
+            .catch(err => {
+                sel.innerHTML = `<option value="">${escHtml(String(err))}</option>`;
+                sel.disabled = true;
+            });
+    }
+
+    window.checkoutBranch = function(stash) {
+        const sel = document.getElementById('branch-select');
+        const branch = sel && sel.value;
+        if (!branch) return;
+
+        setBusy('btn-checkout-branch', true);
+        const el = document.getElementById('result-checkout-branch');
+        if (el) el.classList.add('hidden');
+
+        fetch('/api/v3/system/action', {
+            method: 'POST',
+            headers: {'Content-Type': 'application/json'},
+            body: JSON.stringify({action: 'checkout_branch', branch: branch, stash: !!stash})
+        })
+        .then(r => r.json().catch(() => ({status: 'error', message: `HTTP ${r.status}`})))
+        .then(d => {
+            const ok = d.status === 'success';
+            // Show git's own list of blocking files, then offer the single
+            // action that clears it. Stashing is never done unasked.
+            showResult('result-checkout-branch', ok, d.message || '', d.detail || '');
+            if (!ok && d.can_retry_with_stash && el) {
+                const retry = document.createElement('div');
+                retry.className = 'mt-2 flex items-center gap-2';
+                const label = document.createElement('span');
+                label.className = 'text-xs text-gray-700';
+                label.textContent = 'Stash these changes and switch anyway?';
+                const btn = document.createElement('button');
+                btn.className = 'inline-flex items-center px-2 py-1 border border-gray-300 text-xs font-medium rounded text-gray-700 bg-white hover:bg-gray-50';
+                btn.textContent = 'Stash and switch';
+                btn.onclick = function() { window.checkoutBranch(true); };
+                retry.appendChild(label);
+                retry.appendChild(btn);
+                el.appendChild(retry);
+            }
+            // Both panels describe the checkout, so refresh them together.
+            loadGitInfo();
+            loadBranches();
+        })
+        .catch(err => showResult('result-checkout-branch', false, String(err)))
+        .finally(() => setBusy('btn-checkout-branch', false));
+    };
+
     // ── power supply diagnostics panel ────────────────────────────────────────
     // Reuses the same SSE stream (window.statsSource, set up in base.html)
     // that already drives the header badge/banner and Overview card, instead
@@ -810,6 +914,7 @@
 
     // Load on first render; HTMX will have already swapped us in by this point.
     loadGitInfo();
+    loadBranches();
 
     // Plugin health: initial load + periodic refresh. Guard against duplicate
     // timers if this partial is re-swapped in by HTMX; the handler re-resolves