From 274ea65b39b8ec6d99593625859b6954d655fb06 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 17:46:06 +0000 Subject: [PATCH] fix(sports): harden the B2/B3 capabilities against bad config and subclasses Review pass on the phase B2-B4 changes. Every fix here is the same shape as the crashes this PR already fixed in the hockey and baseball extractors: a config or feed value that is present-but-wrong reaching arithmetic or a comparison on a path with no guard. celebrations: - celebration_duration is coerced and floored at init. It is compared numerically in display() *outside* any try block, so a string from a hand-edited config propagated a TypeError straight out; zero or negative armed a celebration that could never render. - A render failure now disarms instead of staying armed. It previously retried the same broken render on every frame for the rest of the window -- a traceback per frame, and no scorebug either. - prune_score_baselines() for the live set. Only _check_for_win removed entries, so a game that left the live list any other way leaked its baseline and the dict grew all season. - display() reuses has_active_celebration() rather than repeating its window comparison, and log lines carry a [Celebrations] prefix. rotation: - MAX_WEIGHT ceiling. A cycle is sum(weights) long and each step scans every game, so an unbounded weight from a misread config spins the display thread -- on a Pi that stalls rendering outright. - register_rotation_strategy rejects a non-subclass factory at registration instead of failing frames later inside schedule(). - schedule() previews through type(self), so a subclass overriding next_game is previewed with its own ordering -- which is what the method promises. sports_scroll: - scroll_speed / scroll_delay coerced. dict.get(key, default) only helps when the key is absent; present-but-null reached the multiplication inside __init__ and the display failed to construct at all. - update_scroll_position and get_visible_portion moved inside the try. They ran outside it, so a raise there reached the plugin's frame loop despite the comment promising none can. - prepare_and_display guards the subclass call, so one sport's bad payload cannot take down the shared orchestration for the others. - _current_game_type spells "nothing active" as "" in both classes; the manager said None while the display said "". Not taken: the report that baseball's favorite-team debug path still reads event-level status. Verified against current code -- there are no remaining game_event["status"] reads in that file; it was fixed in 2486bdb and the finding is stale. Gates: 747 core unit, 66 plugin safety. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4 --- docs/SPORTS_UNIFICATION.md | 6 +- .../sports/capabilities/celebrations.py | 49 ++++++++- .../sports/capabilities/rotation.py | 30 ++++-- src/common/sports_scroll.py | 62 ++++++++--- test/test_sports_capabilities.py | 100 ++++++++++++++++++ test/test_sports_scroll.py | 57 +++++++++- 6 files changed, 274 insertions(+), 30 deletions(-) diff --git a/docs/SPORTS_UNIFICATION.md b/docs/SPORTS_UNIFICATION.md index 83253b7b..4b9fecdb 100644 --- a/docs/SPORTS_UNIFICATION.md +++ b/docs/SPORTS_UNIFICATION.md @@ -166,9 +166,9 @@ self.rotation = get_rotation_strategy("swrr", weight_for=self._live_weight) `weight_for` is supplied by the host, so the *favorites* policy stays with the plugin and `rotation.py` never learns what a favorite is. An unknown strategy name degrades to `simple` rather than raising: the name comes from user config, -and a typo should cost the boost, not the scoreboard. A plugin needing an -ordering core does not ship calls `register_rotation_strategy` instead of core -growing a branch. +and a typo should cost the boost, not the scoreboard. When a plugin needs an +ordering that core does not ship, it calls `register_rotation_strategy` to add +its own — rather than core growing a branch for it. `test_sports_capabilities.py` checks each strategy against a **verbatim transcription** of the plugin code it replaces, over every live-game shape up to diff --git a/src/base_classes/sports/capabilities/celebrations.py b/src/base_classes/sports/capabilities/celebrations.py index 539f45e1..49d371cd 100644 --- a/src/base_classes/sports/capabilities/celebrations.py +++ b/src/base_classes/sports/capabilities/celebrations.py @@ -51,7 +51,20 @@ class CelebrationMixin: super().__init__(*args, **kwargs) mode_config = getattr(self, "mode_config", {}) or {} self.celebration_enabled = mode_config.get("celebration_enabled", True) - self.celebration_duration = mode_config.get("celebration_duration", 8) + # Coerced and floored at init: this value is compared numerically on the + # display path, where a string from a hand-edited config would raise + # TypeError outside any try block, and a zero or negative value would + # arm a celebration that can never render. + raw_duration = mode_config.get("celebration_duration", 8) + try: + self.celebration_duration = max(1.0, float(raw_duration)) + except (TypeError, ValueError): + self.logger.warning( + "[Celebrations] Unusable celebration_duration %r; using 8s. " + "Set a positive number of seconds.", + raw_duration, + ) + self.celebration_duration = 8.0 # Both spellings: the soccer lineage ships `celebrate_opponent_goals`, # football ships `celebrate_opponent_scores`. Whichever the plugin's # schema declares is the one its users have set. @@ -122,6 +135,25 @@ class CelebrationMixin: # Favorites exist but this team isn't one -> it's the opponent. return self.celebrate_opponent_scores + def prune_score_baselines(self, live_games: List[Dict]) -> None: + """Drop baselines for games no longer live. + + Only :meth:`_check_for_win` removes entries, and it only fires for games + seen to go final. A game that vanishes from the live list any other way + — postponed, dropped by the feed, or simply still live when the board + restarts — leaves its baseline behind forever, so on a board that runs + all season the dict grows without bound. + + Call this from ``update()`` with the current live set, alongside the + equivalent pruning in :meth:`SmoothWeightedRotation.next_game`. + """ + live_ids = {g.get("id") for g in live_games} + self._score_baselines = { + gid: baseline + for gid, baseline in self._score_baselines.items() + if gid in live_ids + } + def has_active_celebration(self) -> bool: """True while a celebration is within its display window.""" celebration = self.active_celebration @@ -255,7 +287,7 @@ class CelebrationMixin: # resumes on it. self.current_game = dict(game) self.logger.info( - f"Celebration ({kind}) armed: {phrase} " + f"[Celebrations] {kind} armed: {phrase} " f"[{game.get('away_abbr')} {away_score}-{home_score} {game.get('home_abbr')}]" ) @@ -323,7 +355,7 @@ class CelebrationMixin: away_logo, (-2, center_y - away_logo.height // 2), away_logo ) except Exception as e: - self.logger.debug(f"Celebration logo load failed: {e}") + self.logger.debug(f"[Celebrations] Logo load failed: {e}") # Phrase across the top, shrunk to fit the panel width. phrase = celebration["phrase"] @@ -365,12 +397,19 @@ class CelebrationMixin: return False celebration = self.active_celebration if celebration: - if time.time() - celebration["started_at"] < self.celebration_duration: + if self.has_active_celebration(): try: self._draw_celebration_layout(celebration, force_clear) return True except Exception as e: - self.logger.error(f"Error drawing celebration: {e}", exc_info=True) + self.logger.error( + f"[Celebrations] Error drawing celebration: {e}", exc_info=True + ) + # Disarm rather than retry: the same render would fail on + # every frame for the rest of the window, logging a + # traceback each time and leaving the scorebug off screen. + self.active_celebration = None + self.last_game_switch = time.time() else: self.active_celebration = None # Reset the dwell so the scorebug resumes on the scoring/winning diff --git a/src/base_classes/sports/capabilities/rotation.py b/src/base_classes/sports/capabilities/rotation.py index 7a8560ba..4135ca9e 100644 --- a/src/base_classes/sports/capabilities/rotation.py +++ b/src/base_classes/sports/capabilities/rotation.py @@ -50,6 +50,12 @@ class RotationStrategy: #: Name this strategy is registered under. Set by :func:`register_rotation_strategy`. name: str = "" + #: Ceiling on a per-game weight. A cycle is ``sum(weights)`` long and each + #: step scans every game, so an unbounded weight — a misread config field, + #: say — would spin the display thread for an unbounded time. On a Pi that + #: stalls rendering outright, so the bound is clamped like the floor is. + MAX_WEIGHT = 16 + def __init__(self, weight_for: Optional[Callable[[Dict], int]] = None): self._weight_for = weight_for or (lambda game: 1) @@ -58,7 +64,8 @@ class RotationStrategy: A weight below 1 is clamped up: a zero or negative weight would starve a game out of the rotation entirely, which no caller means to express - and which would make ``total_weight`` collapse. + and which would make ``total_weight`` collapse. It is clamped down at + :attr:`MAX_WEIGHT` for the reason documented there. """ weights: Dict[str, int] = {} for game in games: @@ -69,7 +76,7 @@ class RotationStrategy: weight = int(self._weight_for(game)) except (TypeError, ValueError): weight = 1 - weights[gid] = max(1, weight) + weights[gid] = min(self.MAX_WEIGHT, max(1, weight)) return weights def schedule(self, games: List[Dict]) -> List[str]: @@ -183,7 +190,11 @@ class SmoothWeightedRotation(RotationStrategy): weights = self.weights(games) if not weights: return [] - preview = SmoothWeightedRotation(self._weight_for) + # type(self), not this class: a subclass that overrides next_game must + # be previewed through its own ordering, or the returned order is not + # the one repeated next_game calls would produce — which is exactly + # what this method promises. + preview = type(self)(self._weight_for) preview._current = dict(self._current) order: List[str] = [] for _ in range(sum(weights.values())): @@ -200,12 +211,19 @@ _REGISTRY: Dict[str, Type[RotationStrategy]] = {} def register_rotation_strategy(name: str, factory: Type[RotationStrategy]) -> None: """Register a rotation strategy under ``name``. - A plugin needing an ordering core does not ship registers it here rather - than core growing a sport-specific branch. Re-registering a name replaces - it, so a plugin may also override a built-in for itself. + When a plugin needs an ordering that core does not ship, it registers its + own here instead of core growing a sport-specific branch. Re-registering a + name replaces it, so a plugin may also override a built-in for itself. """ if not name: raise ValueError("rotation strategy name must be a non-empty string") + # Fail at registration, not at the first schedule() call several frames + # later, where the cause is no longer on the stack. + if not (isinstance(factory, type) and issubclass(factory, RotationStrategy)): + raise TypeError( + f"rotation strategy {name!r} must be a RotationStrategy subclass, " + f"got {factory!r}" + ) factory.name = name _REGISTRY[name] = factory diff --git a/src/common/sports_scroll.py b/src/common/sports_scroll.py index 475d1440..f9a32e37 100644 --- a/src/common/sports_scroll.py +++ b/src/common/sports_scroll.py @@ -214,13 +214,31 @@ class SportsScrollDisplay: self.logger.debug("Ignoring unusable target_fps: %r", raw) return None + def _coerce_float(self, value: Any, default: float) -> float: + """A usable float from config, or ``default``. + + ``dict.get(key, default)`` only helps when the key is *absent*; a key + present with ``null`` or a string returns that value verbatim and blows + up in the arithmetic below — inside ``__init__``, so the whole display + fails to construct. + """ + if value is None: + return default + try: + return float(value) + except (TypeError, ValueError): + self.logger.warning( + "Ignoring unusable scroll setting %r; using %s", value, default + ) + return default + def _configure_scroll_helper(self) -> None: """Apply config to the scroll helper. Safe to call again after a change.""" settings = self._get_scroll_settings() - scroll_speed = settings.get("scroll_speed", 50.0) - scroll_delay = settings.get("scroll_delay", 0.01) - dynamic_duration = settings.get("dynamic_duration", True) + scroll_speed = self._coerce_float(settings.get("scroll_speed"), 50.0) + scroll_delay = self._coerce_float(settings.get("scroll_delay"), 0.01) + dynamic_duration = bool(settings.get("dynamic_duration", True)) self.scroll_helper.set_scroll_delay(scroll_delay) self.scroll_helper.set_dynamic_duration_settings( @@ -277,12 +295,16 @@ class SportsScrollDisplay: if not self.scroll_helper.cached_image: return False - self.scroll_helper.update_scroll_position() - visible = self.scroll_helper.get_visible_portion() - if not visible: - return False - try: + # Inside the try, not before it: advancing the position and cropping + # the visible slice are as capable of raising as the display push, + # and the promise below is that no frame failure reaches the + # plugin's loop. + self.scroll_helper.update_scroll_position() + visible = self.scroll_helper.get_visible_portion() + if not visible: + return False + self.display_manager.image = visible self.display_manager.update_display() self._frame_count += 1 @@ -383,7 +405,11 @@ class SportsScrollDisplayManager: self.logger = custom_logger or logger self.global_config = global_config or {} self._scroll_displays: Dict[str, SportsScrollDisplay] = {} - self._current_game_type: Optional[str] = None + # "" rather than None, matching SportsScrollDisplay's own empty value — + # both are falsy, so `game_type or self._current_game_type` behaved + # either way, but two spellings of "nothing active" across two classes + # is a trap for anyone comparing state between them. + self._current_game_type: str = "" def get_scroll_display(self, game_type: str) -> SportsScrollDisplay: """The display for ``game_type``, created on first use.""" @@ -405,9 +431,19 @@ class SportsScrollDisplayManager: ) -> bool: """Build content for ``game_type`` and make it the active strip.""" scroll_display = self.get_scroll_display(game_type) - success = scroll_display.prepare_scroll_content( - games, game_type, leagues, rankings_cache - ) + try: + success = scroll_display.prepare_scroll_content( + games, game_type, leagues, rankings_cache + ) + except Exception: + # prepare_scroll_content is subclass-implemented and builds cards + # straight from feed data, which is exactly where this PR's other + # crashes came from. One sport's bad payload must not take down the + # shared orchestration for the others. + self.logger.exception( + "Error preparing scroll content for game_type=%s", game_type + ) + return False if success: self._current_game_type = game_type return success @@ -437,7 +473,7 @@ class SportsScrollDisplayManager: """Clear every display and forget which one was active.""" for scroll_display in self._scroll_displays.values(): scroll_display.clear() - self._current_game_type = None + self._current_game_type = "" def get_all_vegas_content_items(self) -> List[Image.Image]: """Every display's Vegas items, for splicing into the marquee.""" diff --git a/test/test_sports_capabilities.py b/test/test_sports_capabilities.py index 31a991c5..cee06ed5 100644 --- a/test/test_sports_capabilities.py +++ b/test/test_sports_capabilities.py @@ -86,6 +86,17 @@ class TestRegistry: with pytest.raises(ValueError): register_rotation_strategy("", SimpleRotation) + @pytest.mark.parametrize("bad", [ + SimpleRotation(), # an instance, not the class + str, # unrelated class + lambda **kw: None, # a factory function + ]) + def test_a_non_strategy_factory_is_rejected(self, bad): + """Fail at registration, not several frames later inside schedule(), + where the cause is no longer on the stack.""" + with pytest.raises(TypeError): + register_rotation_strategy("bad-factory", bad) + def test_weight_for_is_optional(self): """Default weights are equal, so every strategy degenerates to a plain round robin — the pre-boost behavior.""" @@ -111,6 +122,17 @@ class TestWeights: strategy = get_rotation_strategy("weighted", weight_for=lambda g: bad) assert strategy.weights([game("a")]) == {"a": 1} + def test_huge_weights_are_clamped(self): + """A cycle is sum(weights) long and each step scans every game, so an + unbounded weight from a misread config spins the display thread.""" + strategy = get_rotation_strategy("weighted", weight_for=lambda g: 10_000) + assert strategy.weights([game("a")]) == {"a": RotationStrategy.MAX_WEIGHT} + + def test_a_clamped_cycle_stays_bounded(self): + strategy = get_rotation_strategy("weighted", weight_for=lambda g: 10_000) + order = strategy.schedule([game("a"), game("b")]) + assert len(order) == 2 * RotationStrategy.MAX_WEIGHT + class TestSimpleRotation: def test_one_pass_in_feed_order(self): @@ -218,6 +240,20 @@ class TestSmoothWeightedRotation: actual = [strategy.next_game(games)["id"] for _ in range(len(preview))] assert preview == actual + def test_preview_uses_the_subclass_ordering(self): + """schedule() promises the order repeated next_game calls produce. Built + from the base class, a subclass that overrides next_game gets a preview + of the wrong algorithm.""" + class Reversed(SmoothWeightedRotation): + def next_game(self, games): + return super().next_game(list(reversed(games))) + + games = [game("a"), game("b"), game("c")] + strategy = Reversed() + preview = strategy.schedule(games) + actual = [strategy.next_game(games)["id"] for _ in range(len(preview))] + assert preview == actual + def test_empty(self): assert SmoothWeightedRotation().next_game([]) is None assert SmoothWeightedRotation().schedule([]) == [] @@ -462,6 +498,24 @@ class TestCelebrationConfig: "celebrate_opponent_goals": True}) assert manager.celebrate_opponent_scores is False + @pytest.mark.parametrize("bad", ["eight", None, {}, []]) + def test_unusable_duration_falls_back(self, celebrating, bad): + """The duration is compared numerically on the display path, outside + any try block — a string from a hand-edited config would propagate a + TypeError straight out of display().""" + manager = celebrating(mode_config={"celebration_duration": bad}) + assert manager.celebration_duration == 8.0 + + @pytest.mark.parametrize("bad", [0, -5]) + def test_non_positive_duration_is_floored(self, celebrating, bad): + """Zero or negative would arm a celebration that can never render.""" + manager = celebrating(mode_config={"celebration_duration": bad}) + assert manager.celebration_duration == 1.0 + + def test_numeric_string_duration_is_accepted(self, celebrating): + assert celebrating( + mode_config={"celebration_duration": "12"}).celebration_duration == 12.0 + class TestScoreDetection: def test_first_sighting_never_celebrates(self, celebrating): @@ -725,6 +779,52 @@ class TestDisplayTakeover: assert manager.display() is True assert manager.display_calls == [False] + def test_a_render_failure_disarms_rather_than_retrying(self, celebrating): + """Left armed, the same render fails on every frame for the rest of the + window — a traceback per frame, and no scorebug.""" + manager = celebrating() + manager._check_for_score(game("g1", home_score=0)) + manager._check_for_score(game("g1", home_score=1)) + manager._draw_celebration_layout = MagicMock(side_effect=RuntimeError("boom")) + manager.display() + assert manager.active_celebration is None + manager.display() + assert manager._draw_celebration_layout.call_count == 1 + + +class TestBaselinePruning: + """`_score_baselines` gains an entry per game and only _check_for_win ever + removed one, so a board running all season grows the dict without bound.""" + + def test_prunes_games_no_longer_live(self, celebrating): + manager = celebrating() + for gid in ("g1", "g2", "g3"): + manager._check_for_score(game(gid, home_score=1)) + manager.prune_score_baselines([game("g2")]) + assert set(manager._score_baselines) == {"g2"} + + def test_keeps_every_still_live_game(self, celebrating): + manager = celebrating() + for gid in ("g1", "g2"): + manager._check_for_score(game(gid, home_score=1)) + manager.prune_score_baselines([game("g1"), game("g2")]) + assert set(manager._score_baselines) == {"g1", "g2"} + + def test_empty_live_set_clears_everything(self, celebrating): + manager = celebrating() + manager._check_for_score(game("g1", home_score=1)) + manager.prune_score_baselines([]) + assert manager._score_baselines == {} + + def test_pruning_does_not_disturb_a_surviving_baseline(self, celebrating): + manager = celebrating() + manager._check_for_score(game("g1", home_score=2)) + manager.prune_score_baselines([game("g1")]) + manager._check_for_score(game("g1", home_score=3)) + assert manager.active_celebration is not None, ( + "pruning must not drop a live game's baseline and re-trigger the " + "first-sighting suppression") + def test_disabled_manager_renders_nothing(self, celebrating): manager = celebrating() manager.is_enabled = False diff --git a/test/test_sports_scroll.py b/test/test_sports_scroll.py index 51ecd123..9f6f72e8 100644 --- a/test/test_sports_scroll.py +++ b/test/test_sports_scroll.py @@ -178,7 +178,8 @@ class TestScrollSettings: def test_a_null_league_block_is_tolerated(self, build): """`config['nhl'] = None` appears in hand-edited configs.""" display = build({"nhl": None}) - assert display._get_scroll_settings()["scroll_speed"] == 50.0 + assert (display._get_scroll_settings()["scroll_speed"] + == DEFAULT_SCROLL_SETTINGS["scroll_speed"]) # --------------------------------------------------------------------------- @@ -245,6 +246,25 @@ class TestConfigureScrollHelper: display = build(global_config={"target_fps": "120"}) display.scroll_helper.set_target_fps.assert_called_once_with(120.0) + @pytest.mark.parametrize("bad", [None, "fast", {}, []]) + def test_unusable_scroll_speed_degrades_instead_of_crashing(self, build, bad): + """`.get(key, default)` only helps when the key is *absent*. A key + present with null reaches the arithmetic and raises inside __init__, + taking the whole display down before it renders anything.""" + display = build({"nhl": {"scroll_settings": {"scroll_speed": bad}}}) + # 50.0 px/s * 0.01 s/frame == 0.5 px/frame, i.e. the default speed. + display.scroll_helper.set_scroll_speed.assert_called_with(0.5) + + @pytest.mark.parametrize("bad", [None, "slow", {}]) + def test_unusable_scroll_delay_degrades_instead_of_crashing(self, build, bad): + display = build({"nhl": {"scroll_settings": {"scroll_delay": bad}}}) + display.scroll_helper.set_scroll_delay.assert_called_with(0.01) + + def test_numeric_strings_are_accepted(self, build): + display = build({"nhl": {"scroll_settings": { + "scroll_speed": "100", "scroll_delay": "0.02"}}}) + display.scroll_helper.set_scroll_speed.assert_called_with(2.0) + def test_fps_clamping_is_left_to_the_helper(self, build): """Deliberately not clamped here — a second copy of the range would drift from ScrollHelper.set_target_fps.""" @@ -308,6 +328,16 @@ class TestFramePumping: display.display_manager.update_display.side_effect = RuntimeError("boom") assert display.display_scroll_frame() is False + @pytest.mark.parametrize("failing", ["update_scroll_position", + "get_visible_portion"]) + def test_a_scroll_helper_failure_is_contained_too(self, build, failing): + """These ran outside the try, so a raise there reached the caller's + frame loop despite the stated promise that none can.""" + display = build() + display.prepare_scroll_content([{"id": "g1"}], "live", ["nhl"]) + getattr(display.scroll_helper, failing).side_effect = RuntimeError("boom") + assert display.display_scroll_frame() is False + def test_frames_are_counted(self, build): display = build() display.prepare_scroll_content([{"id": "g1"}], "live", ["nhl"]) @@ -407,7 +437,23 @@ class TestManager: def test_failed_prepare_does_not_become_active(self, manager): assert manager.prepare_and_display([], "live", ["nhl"]) is False - assert manager._current_game_type is None + assert not manager._current_game_type + + def test_a_raising_subclass_does_not_escape_the_orchestration(self, manager): + """prepare_scroll_content is subclass code building cards from feed + data. One sport's bad payload must not take down the others.""" + display = manager.get_scroll_display("live") + display.prepare_scroll_content = MagicMock(side_effect=KeyError("status")) + assert manager.prepare_and_display([{"id": "g1"}], "live", ["nhl"]) is False + assert not manager._current_game_type + + def test_empty_game_type_sentinel_matches_the_display(self, manager): + """Both classes must spell 'nothing active' the same way; two spellings + across two classes is a trap for anyone comparing their state.""" + manager.prepare_and_display([{"id": "g1"}], "live", ["nhl"]) + manager.clear_all() + assert (manager._current_game_type + == manager.get_scroll_display("live")._current_game_type == "") def test_display_frame_uses_the_active_type(self, manager): manager.prepare_and_display([{"id": "g1"}], "live", ["nhl"]) @@ -437,7 +483,7 @@ class TestManager: manager.prepare_and_display([{"id": "g1"}], "live", ["nhl"]) manager.prepare_and_display([{"id": "g2"}], "recent", ["nhl"]) manager.clear_all() - assert manager._current_game_type is None + assert not manager._current_game_type for game_type in ("live", "recent"): assert manager.get_scroll_display(game_type)._current_games == [] @@ -526,6 +572,11 @@ class TestAgainstTheRealScrollHelper: real.display_frame() time.sleep(0.0012) + # Asserted separately so a host too slow to sustain the frame rate + # reports a timeout rather than looking like a scrolling defect. + assert time.time() < deadline, ( + "scroll did not finish within 20s — the host may be too slow to " + "sustain the configured frame rate") assert real.is_complete() is True assert display.scroll_helper.scroll_position > 128