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

* refactor(web): use canonical secret helpers in api_v3; make ConfigManager secret strip/merge array-aware

api_v3.py carried three inline nested copies of find_secret_fields/
separate_secrets (main-config save, plugin-config save, plugin-config
reset). They drifted from each other (one lacked isinstance guards) and
none supported the canonical module's array-item secrets
(accounts[].token). All three endpoints now import from
src/web_interface/secret_helpers.

Adopting the canonical behavior makes array-item secrets reachable, and
their parallel-placeholder shape ([{'token': ...}, {}] alongside the
regular list) was not survivable by ConfigManager's round-trip:
_strip_secrets_recursive dropped the whole key (losing the regular
fields from config.json) and _deep_merge replaced the regular list
wholesale on load. Both are now array-aware:

- strip removes the secret fields from each item and ALWAYS keeps the
  list so indices survive for merge-on-load; whole-key secrets (scalar
  lists, shape mismatches) still drop the key entirely — never leak.
- merge folds each secrets item into the config item at the same index,
  skipping {} placeholders. The regular list's length is authoritative
  in both directions: a user deleting an array item never has it
  resurrected from a stale secrets entry (extras warn and are ignored).

api_v3's own deep_merge intentionally still replaces lists wholesale —
form posts carry complete arrays and index-merging would resurrect
deleted items; a comment now documents that.

Tests: the parity guard flips from 'exactly 3 inline copies' to 'zero,
and the canonical import must exist'; TestArraySecretStripAndMerge
covers the new strip/merge semantics incl. length-mismatch contracts;
new test_api_v3_secret_roundtrip.py drives all three endpoints through
a Flask client with a REAL ConfigManager+SchemaManager over tmp_path,
proving secrets land in config_secrets.json, config.json stays clean,
and a fresh load merges them back into the right array items.

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

* 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

* test: cover the previously untested modules

Nine new suites plus an extension, asserting the Phase-1b fixed behavior
and pinning the quirks deliberately left alone:

- test_logging_config.py: formatters (JSON shape, no record mutation,
  single prefix through two handlers), PluginLoggerAdapter precedence,
  setup_logging handler hygiene and LEDMATRIX_DEBUG, log_error exc_info.
- test_startup_validator.py: exact messages, error-vs-warning split,
  accessor split (load_config vs get_config), cache-dir branches with
  os.access monkeypatched (root can write anything in CI), idempotence,
  raise_on_errors classification precedence.
- test_config_helper.py (full): load/save round trips, dot-notation
  get/set incl. silent-failure contract, post-fix no-aliasing merge,
  schema validation branches, the '{id}_config' key pin, default-enabled
  pin.
- test_saved_repositories.py: three load shapes, bare-list rewrite pin,
  trailing-only .git strip (my.github.io regression), save-failure
  rollback, type-classification case-sensitivity pin.
- test_api_helper.py: rate-limit math, cache-hit short circuit, ESPN
  URL/key formats, exact User-Agent guard, retry adapter, post-fix
  clear_cache against the real CacheManager surface, ttl-dropped pin.
- test_base_odds_manager.py: cache-key/URL construction, no_odds
  sentinel round trip, stale-cache fallback, null-safe extraction,
  ML-only formatting, is_odds_available truth table (ML-blind by
  contract), config key/attr mismatch pin.
- test_dynamic_team_resolver.py: expansion/dedup/slicing, dropped
  unknown-dynamic names (TOP_ substring hazard pinned), genuinely
  shared class cache (second instance: zero HTTP), TTL expiry,
  failure degradation without raising.
- test_display_helper.py (full): the fixed error/no-data renders,
  combined scorebug top line, non-blank ticker with scroll_speed
  no-op pin, composite upconversion, logo bleed positions, square
  orientation pin.
- test_skin_runtime_cache.py: discovery-cache hit/invalidation
  semantics (manifest mtime, .py edits pinned as non-invalidating),
  sys.modules namespacing contract incl. bare-name restore and stdlib
  shadowing, entry-module execute-once, API minor-version tolerance,
  skin_matches_target table.
- test_sports_capabilities.py (extended): _draw_celebration_layout
  executed for real (flash window, matrix-dims fallback, highlight
  alternation, logo-failure isolation), _should_celebrate_for direct,
  strict duration boundary, score_to_int edges, both-teams-score
  precedence, expired-coalesce refire, disabled-win baseline
  preservation, id-less prune.

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

* test: real schedule/dim coverage for DisplayController; fix two vacuous schedule tests

New test_display_controller_schedule.py drives _check_schedule and
_check_dim_schedule on a bare controller stub: same-day and
midnight-crossing windows with inclusive boundaries, global vs per-day vs
legacy-inferred modes (and dim's global-only default — no legacy
inference), per-day disabled days, invalid %H:%M fallbacks, unknown
timezone -> UTC, dim_brightness default 30, inactive-display short
circuit, and the _was_display_active/_was_dimmed transition flags.

test_display_controller.py's test_schedule_disabled and
test_active_hours patched config_service.get_config — which
_check_schedule never reads — so both asserted the init-default value
and could not fail. Rewritten on the test_inactive_hours pattern
(inject controller.config['schedule'], reset the minute gate, flip the
flag to the opposite state first so the assertion has teeth).

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

* ci: raise coverage floor to 48%

Measured 50% with the new suites in place (was 47% baseline when the
gate was introduced at 45); floor stays two points under measured.

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

* fix: address CodeQL alert and review findings

- config_manager: the "secrets list longer than config list" warning now
  interpolates only config-side data (no key name or secrets-derived
  values), resolving the CodeQL clear-text-logging alert.
- base_plugin: validate_config rejects bool display_duration, matching
  get_display_duration (bool is an int subclass and would otherwise pass
  as a positive number).
- config_helper: merge_configs deep-copies override values in the
  non-recursive branch so mutating the merged result cannot reach back
  into override_config.
- saved_repositories: saves are atomic (temp file + fsync + os.replace),
  so a failed write can no longer truncate saved_repositories.json.
- tests: regression cases for each fix, plus a pin that whole-item
  array secrets (key[] + key[].field both marked) strip to empty {}
  skeletons — no secret values can reach config.json.

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

---------

Co-authored-by: Claude <noreply@anthropic.com>

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']