fix: repair broken helper paths across display, cache, odds, logging, resolver, repos, config, validator

Nine fixes for bugs surfaced while writing coverage for previously
untested modules (plus the bool-duration quirk pinned in PR #441):

- base_plugin.get_display_duration: exclude bools from both numeric
  branches — display_duration=True no longer reads as a 1-second slot;
  it falls through to config, then the 15.0 default.
- display_helper: draw_error_message/draw_no_data_message called
  _draw_centered_text with the wrong arguments and crashed with
  AttributeError — both now delegate to draw_centered_text.
  draw_scorebug_layout drew status and clock at the same y, overprinting
  each other — they now share one combined top line.
  draw_ticker_layout drew its text starting at x=display_width (fully
  off-canvas), returning a blank frame every time — now draws at x=0;
  scroll_speed stays accepted-but-unused and is documented as such.
- api_helper.clear_cache guarded on a nonexistent CacheManager.clear()
  method, silently never clearing anything; it now uses the real surface
  (clear_cache/delete/list_cache_files) and no-ops safely otherwise.
- base_odds_manager._extract_espn_data raised AttributeError when ESPN
  sent explicit JSON nulls ("homeTeamOdds": null) — every level now
  null-safes with 'or {}'. format_odds_summary gated on
  is_odds_available, which deliberately ignores money lines, so
  ML-only odds formatted as "No odds available" — it now gates only on
  empty/no_odds data and formats money lines.
- logging_config.ContextualFormatter mutated record.msg in place, so a
  second handler prepended the context prefix twice; it now formats a
  copy. log_error hardcoded exc_info=True and raised TypeError when the
  caller passed exc_info — now kwargs.setdefault.
- dynamic_team_resolver wrote its "shared" class cache through self,
  creating instance shadows — the cache was per-instance and every
  scoreboard refetched rankings. Writes now go through the class.
- saved_repositories cleaned URLs with an unanchored .replace('.git','')
  that mangled URLs merely containing '.git' (my.github.io -> myhub.io);
  now strips only a trailing suffix. add/remove also roll back the
  in-memory list when the save fails, so memory always matches disk.
- config_helper.merge_configs shallow-copied the base, aliasing every
  un-overridden nested dict into the result — now deep-copies.
- startup_validator.validate_all accumulated errors/warnings across
  calls — now resets both lists per run.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh

src/base_odds_manager.py

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

src/common/api_helper.py

@@ -273,19 +273,34 @@ class APIHelper:
         """
         Clear cache data.
 
+        Uses CacheManager's real surface (clear_cache / delete /
+        list_cache_files); safely no-ops on managers without it. The old
+        implementation guarded on a nonexistent ``clear`` method, so it
+        silently never cleared anything.
+
         Args:
-            pattern: Optional pattern to match cache keys
+            pattern: Optional substring to match cache keys; only matching
+                entries are deleted.
         """
-        if self.cache_manager:
+        if not self.cache_manager:
-            if hasattr(self.cache_manager, 'clear'):
+            return
         if pattern:
-                    # 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
+                    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 @@ 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,9 +161,11 @@ 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):

src/common/display_helper.py

@@ -115,18 +115,14 @@ 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')),
                                    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}"
         self._draw_centered_text(draw, score_text, fonts.get('score'), 
@@ -153,12 +149,18 @@ class DisplayHelper:
         """
         Draw a ticker/scrolling text layout.
         
+        Renders a single static frame with the text at the left edge; the
+        caller advances the scroll by re-rendering or shifting. The
+        scroll_speed parameter is accepted for API compatibility but does
+        not affect this frame. (Previously the text was drawn starting at
+        x=display_width — entirely off-canvas — so every frame was blank.)
+
         Args:
             text: Text to display
             font: Font to use
             background_color: Background color
             text_color: Text color
-            scroll_speed: Pixels to scroll per frame
+            scroll_speed: Accepted for compatibility; unused per-frame
 
         Returns:
             PIL Image with ticker layout
@@ -166,11 +168,7 @@ class DisplayHelper:
         img = self.create_base_image(background_color)
         draw = ImageDraw.Draw(img)
 
-        # Start text off-screen to the right
+        self._draw_text_with_outline(draw, text, (0, self.display_height // 2 - 6),
-        x_position = self.display_width
-        
-        # Draw text
-        self._draw_text_with_outline(draw, text, (x_position, self.display_height // 2 - 6), 
                                    font, fill=text_color)
 
         return img
@@ -214,15 +212,9 @@ class DisplayHelper:
         Returns:
             PIL Image with error message
         """
-        img = self.create_base_image((50, 0, 0))  # Dark red background
+        # Dark red background, white text
-
-        # Use default font
         font = ImageFont.load_default()
-        
+        return self.draw_centered_text(message, font, (50, 0, 0), (255, 255, 255))
-        # Draw centered error message
-        self._draw_centered_text(message, font, (50, 0, 0), (255, 255, 255))
-        
-        return img
     
     def draw_no_data_message(self, message: str = "No Data") -> Image.Image:
         """
@@ -234,11 +226,8 @@ class DisplayHelper:
         Returns:
             PIL Image with no data message
         """
-        img = self.create_base_image((0, 0, 0))
         font = ImageFont.load_default()
-        self._draw_centered_text(message, font, (0, 0, 0), (150, 150, 150))
+        return self.draw_centered_text(message, font, (0, 0, 0), (150, 150, 150))
-        
-        return img
     
     def get_display_dimensions(self) -> Tuple[int, int]:
         """

src/dynamic_team_resolver.py

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

src/logging_config.py

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

src/plugin_system/base_plugin.py

@@ -364,8 +364,10 @@ class BasePlugin(ABC):
                 # Handle None case
                 if duration is None:
                     pass  # Fall through to config
-                # Try to convert to float if it's a number or numeric string
+                # Try to convert to float if it's a number or numeric string.
-                elif isinstance(duration, (int, float)):
+                # bool is excluded: it's an int subclass, and True would
+                # otherwise read as a 1-second duration.
+                elif isinstance(duration, (int, float)) and not isinstance(duration, bool):
                     if duration > 0:
                         return float(duration)
                     else:
@@ -403,8 +405,9 @@ class BasePlugin(ABC):
         # Fall back to config
         config_duration = self.config.get("display_duration", 15.0)
         try:
-            # Ensure config value is also a valid float
+            # Ensure config value is also a valid float (bool excluded — an
-            if isinstance(config_duration, (int, float)):
+            # int subclass that would otherwise read True as 1 second)
+            if isinstance(config_duration, (int, float)) and not isinstance(config_duration, bool):
                 if config_duration > 0:
                     return float(config_duration)
                 else:

src/plugin_system/saved_repositories.py

@@ -57,6 +57,18 @@ class SavedRepositoriesManager:
             self.logger.error(f"Error saving repositories: {e}")
             return False
     
+    @staticmethod
+    def _clean_url(repo_url: str) -> str:
+        """Normalize a repo URL: strip whitespace, trailing slashes, and a
+        trailing ``.git`` suffix ONLY. (The old ``.replace('.git', '')``
+        was an unanchored substring replace that mangled URLs merely
+        containing ``.git``, e.g. ``https://github.com/user/my.github.io``.)
+        """
+        repo_url = repo_url.strip().rstrip('/')
+        if repo_url.endswith('.git'):
+            repo_url = repo_url[:-4]
+        return repo_url
+
     def get_all(self) -> List[Dict[str, str]]:
         """Get all saved repositories."""
         return self.repositories.copy()
@@ -72,8 +84,7 @@ class SavedRepositoriesManager:
         Returns:
             True if added successfully
         """
-        # Clean URL
+        repo_url = self._clean_url(repo_url)
-        repo_url = repo_url.strip().rstrip('/').replace('.git', '')
 
         # Check if already exists
         for repo in self.repositories:
@@ -96,7 +107,12 @@ class SavedRepositoriesManager:
             'type': 'registry' if 'plugins.json' in repo_url or 'ledmatrix-plugins' in repo_url.lower() else 'single'
         })
 
-        return self._save_repositories()
+        if not self._save_repositories():
+            # Keep memory consistent with disk: a failed save must not leave
+            # a phantom entry that only this process can see.
+            self.repositories.pop()
+            return False
+        return True
     
     def remove(self, repo_url: str) -> bool:
         """
@@ -108,21 +124,25 @@ class SavedRepositoriesManager:
         Returns:
             True if removed successfully
         """
-        # Clean URL
+        repo_url = self._clean_url(repo_url)
-        repo_url = repo_url.strip().rstrip('/').replace('.git', '')
 
-        original_count = len(self.repositories)
+        previous = self.repositories
-        self.repositories = [r for r in self.repositories if r.get('url') != repo_url]
+        remaining = [r for r in previous if r.get('url') != repo_url]
 
-        if len(self.repositories) < original_count:
+        if len(remaining) < len(previous):
-            return self._save_repositories()
+            self.repositories = remaining
+            if not self._save_repositories():
+                # Failed save: restore so memory matches disk.
+                self.repositories = previous
+                return False
+            return True
         else:
             self.logger.warning(f"Repository not found: {repo_url}")
             return False
     
     def has(self, repo_url: str) -> bool:
         """Check if a repository is already saved."""
-        repo_url = repo_url.strip().rstrip('/').replace('.git', '')
+        repo_url = self._clean_url(repo_url)
         return any(r.get('url') == repo_url for r in self.repositories)
     
     def get_registry_repositories(self) -> List[Dict[str, str]]:

src/startup_validator.py

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

test/test_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,7 @@ 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