Follow-ups from #441: secret-helper migration, ten more bug fixes, and coverage for every remaining untested module

.github/workflows/test.yml

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

src/base_odds_manager.py

@@ -163,19 +163,25 @@ def _extract_espn_data(self, data: Dict[str, Any]) -> Optional[Dict[str, Any]]:
  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
- # Extract the odds data directly from the item
+ # The ESPN API returns odds data directly in the item, not in a
+ # 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"),
- "spread_odds": item.get("homeTeamOdds", {}).get("current", {}).get("pointSpread", {}).get("value")
+ "money_line": home.get("moneyLine"),
+ "spread_odds": ((home.get("current") or {})
+ .get("pointSpread") or {}).get("value")
  },
  "away_team_odds": {
- "money_line": item.get("awayTeamOdds", {}).get("moneyLine"),
- "spread_odds": item.get("awayTeamOdds", {}).get("current", {}).get("pointSpread", {}).get("value")
+ "money_line": away.get("moneyLine"),
+ "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 +266,13 @@ def format_odds_summary(self, odds_data: Optional[Dict[str, Any]]) -> str:
  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/common/api_helper.py

@@ -272,20 +272,35 @@ def get_cache(self, key: str) -> Optional[Any]:
  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 hasattr(self.cache_manager, 'clear'):
- if pattern:
- # Clear only keys matching pattern
- keys = self.cache_manager.keys()
- for key in keys:
- if pattern in key:
- self.cache_manager.delete(key)
- else:
- self.cache_manager.clear()
+ if not self.cache_manager:
+ return
+ if pattern:
+ if (hasattr(self.cache_manager, 'list_cache_files')
+ and hasattr(self.cache_manager, 'delete')):
+ for entry in self.cache_manager.list_cache_files():
+ key = entry.get('key') if isinstance(entry, dict) else None
+ if key and pattern in key:
+ self.cache_manager.delete(key)
+ 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 @@
 Extracted from LEDMatrix core to provide reusable functionality for plugins.
 """
 
+import copy
 import json
 import logging
 from pathlib import Path
@@ -160,17 +161,20 @@ def merge_configs(self, base_config: Dict[str, Any],
  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
- merged[key] = value
+ # Override with new value — deep-copied so mutating the
+ # 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 @@ def draw_scorebug_layout(self, game_data: Dict[str, Any],
  if home_logo and away_logo:
  self._draw_logos(main_img, home_logo, away_logo)
 
- # Draw status/period text (top center)
- if status_text or period_text:
- status_display = f"{period_text} {status_text}".strip()
- if status_display:
- self._draw_centered_text(draw, status_display, 
- fonts.get('time', fonts.get('status')), 
- y_position=1)
-
- # Draw clock if available
- if clock:
- self._draw_centered_text(draw, clock, fonts.get('time'), y_position=1)
+ # Draw one combined top line (period/status/clock all share y=1 —
+ # drawing them separately overprinted each other).
+ top_line = " ".join(p for p in [period_text, status_text, clock] if p)
+ if top_line:
+ self._draw_centered_text(draw, top_line,
+ fonts.get('time', fonts.get('status')),
+ y_position=1)
 
  # Draw scores (center)
  score_text = f"{away_score}-{home_score}"
@@ -153,26 +149,28 @@ def draw_ticker_layout(self, text: str, font: ImageFont.ImageFont,
  """
  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
- x_position = self.display_width
-
- # Draw text
- self._draw_text_with_outline(draw, text, (x_position, self.display_height // 2 - 6), 
+
+ self._draw_text_with_outline(draw, text, (0, 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 @@ def draw_error_message(self, message: str = "Error") -> Image.Image:
  Returns:
  PIL Image with error message
  """
- img = self.create_base_image((50, 0, 0)) # Dark red background
-
- # Use default font
+ # Dark red background, white text
  font = ImageFont.load_default()
-
- # Draw centered error message
- self._draw_centered_text(message, font, (50, 0, 0), (255, 255, 255))
-
- return img
+ return self.draw_centered_text(message, font, (50, 0, 0), (255, 255, 255))
 
  def draw_no_data_message(self, message: str = "No Data") -> Image.Image:
  """
@@ -234,11 +226,8 @@ def draw_no_data_message(self, message: str = "No Data") -> Image.Image:
  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 img
+ return self.draw_centered_text(message, font, (0, 0, 0), (150, 150, 150))
 
  def get_display_dimensions(self) -> Tuple[int, int]:
  """

src/config_manager.py

@@ -269,20 +269,47 @@ def load_config(self) -> Dict[str, Any]:
  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 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:
+ if key not in secrets:
  # 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 @@ def get_secret(self, key: str) -> Optional[Any]:
  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 @@ def _fetch_ncaa_fb_rankings(self) -> Dict[str, int]:
 
  # Sort by ranking (1, 2, 3, etc.)
  sorted_rankings = dict(sorted(rankings.items(), key=lambda x: x[1]))
-
- # Cache the results
- self._rankings_cache = sorted_rankings
- self._cache_timestamp = current_time
+
+ # Cache the results ON THE CLASS. Assigning through self
+ # would create instance attributes that shadow the shared
+ # 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 @@ def _is_potential_dynamic_team(self, team_name: str) -> bool:
  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."""
- self._rankings_cache = {}
- self._cache_timestamp = 0
+ """Clear the SHARED rankings cache to force fresh data on next
+ request. Writes through the class — assigning via self would only
+ shadow the shared cache for this instance."""
+ DynamicTeamResolver._rankings_cache = {}
+ DynamicTeamResolver._cache_timestamp = 0
  self.logger.info("Cleared dynamic team rankings cache")