test(celebrations): make the expiry tests actually test expiry

CodeRabbit (Major) on the merge re-review: celebration_duration is clamped to
a 1.0s floor, so the two expiry tests that configured 0 and expected instant
expiration never actually hit the expiry branch. They passed only because
_draw_celebration_layout raises in the harness (no real fonts) and its
exception branch clears the celebration the same way -- so they were really
re-testing the render-failure path, not expiry.

Now use a valid 1s duration, backdate started_at past the window, and mock
_draw_celebration_layout with assert_not_called() so an expired celebration
provably does NOT render. Verified discriminating: both fail if
has_active_celebration is forced to never expire.

Production code unchanged -- the expiry logic was already correct; only the
tests were mismodelling it.

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

config/config.template.json

@@ -88,6 +88,7 @@
         }
     },
     "timezone": "America/New_York",
+    "target_fps": 100,
     "location": {
         "city": "Tampa",
         "state": "Florida",

src/base_classes/sports/modes.py

@@ -847,7 +847,7 @@ class SportsLive(SportsCore):
         # feed: {game_id: {"clock": ts, "score": ts, "last_seen": ts}}.
         # Until a subclass writes "last_seen", the staleness branch of
         # _detect_stale_games is inert and only the game-over check applies.
-        self.game_update_timestamps = {}
+        self.game_update_timestamps: Dict[str, Dict[str, float]] = {}
         self.stale_game_timeout = self.mode_config.get("stale_game_timeout", 300)  # 5 minutes default
 
     @abstractmethod

src/element_style.py

@@ -106,6 +106,13 @@ def resolve_font_path(font_name: str) -> Optional[str]:
         return None
     if os.path.isabs(font_name):
         return font_name if os.path.isfile(font_name) else None
+    # A relative name must be a bare filename. font_name comes from plugin
+    # config, which the web UI writes; a value like "../../config/config.json"
+    # would otherwise escape assets/fonts/ once joined and let a config probe
+    # arbitrary paths for existence. os.path.basename collapses any such value
+    # to its last component, so a name that isn't already bare is rejected.
+    if os.path.basename(font_name) != font_name:
+        return None
     candidates = (
         os.path.join(os.getcwd(), _FONTS_SUBDIR, font_name),
         os.path.join(_CORE_ROOT, _FONTS_SUBDIR, font_name),

src/plugin_system/base_plugin.py

@@ -145,6 +145,77 @@ class BasePlugin(ABC):
         """
         raise NotImplementedError("Plugins must implement display()")
 
+    # -------------------------------------------------------------------------
+    # Global (whole-device) configuration
+    # -------------------------------------------------------------------------
+    @property
+    def global_config(self) -> Dict[str, Any]:
+        """
+        The full LEDMatrix configuration, for reading device-wide settings.
+
+        ``self.config`` is only this plugin's own slice, so cross-cutting
+        settings — ``target_fps``, ``timezone``, ``location`` — were previously
+        unreachable from a plugin without reaching into a manager by hand.
+
+        Resolution order mirrors the timezone helpers the sports plugins
+        already ship: ``plugin_manager.config_manager`` first (the cores that
+        hang it there), then ``cache_manager.config_manager``. Returns ``{}``
+        when neither is available, so callers can use plain ``.get()`` without
+        guarding, and a plugin on a core that predates this property still
+        loads — ``getattr(self, 'global_config', {})`` simply yields the
+        default.
+
+        Treat as read-only: the returned dict is the live config the core is
+        using, so mutating it edits every other consumer's view and can be
+        persisted back to disk.
+
+        Assignment is still allowed and wins over the resolved value. Several
+        shipped plugins (news, stock-news, ledmatrix-stocks, ledmatrix-
+        elections, ledmatrix-leaderboard, nfl-draft) set
+        ``self.global_config`` to their own ``config['global']`` sub-dict; a
+        property without a setter would raise AttributeError and stop those
+        plugins loading.
+
+        Example:
+            fps = self.global_config.get('target_fps')
+        """
+        override = getattr(self, '_global_config_override', None)
+        if override is not None:
+            return override
+        for owner in (self.plugin_manager, self.cache_manager):
+            config_manager = getattr(owner, 'config_manager', None)
+            if config_manager is None:
+                continue
+            try:
+                config = config_manager.get_config()
+            except Exception:
+                # A broken or unreadable config must never stop a plugin from
+                # loading; fall through to the next source, then to {}.
+                self.logger.debug(
+                    "Could not read global config from %s",
+                    type(owner).__name__, exc_info=True,
+                )
+                continue
+            # Only a real mapping is usable: callers do .get() on this and feed
+            # the result to numeric code, so handing back whatever a stub or a
+            # half-built manager returned would fail later and further away.
+            #
+            # An empty dict is treated as "nothing here yet" rather than a
+            # valid answer, so resolution continues to the next source. Both
+            # managers default to the same config/config.json, so falling
+            # through cannot pick up a different file's settings -- but it does
+            # rescue the case where the first manager simply hasn't loaded yet,
+            # which would otherwise return {} and silently disable every
+            # setting read through this property.
+            if isinstance(config, dict) and config:
+                return config
+        return {}
+
+    @global_config.setter
+    def global_config(self, value: Dict[str, Any]) -> None:
+        """Let a plugin substitute its own view (see the getter's docstring)."""
+        self._global_config_override = value
+
     # -------------------------------------------------------------------------
     # Adaptive layout support (opt-in)
     # -------------------------------------------------------------------------

test/test_element_style.py

@@ -377,6 +377,22 @@ class TestResolverPlumbing:
         font = load_font("5x7.bdf", 7)
         assert isinstance(font, freetype.Face)
 
+    @pytest.mark.parametrize("hostile", [
+        "../../config/config.json",
+        "../secrets.txt",
+        "sub/dir/font.ttf",
+        "..",
+    ])
+    def test_relative_font_name_with_path_components_is_rejected(self, hostile):
+        # font_name comes from plugin config (web-UI writable); a relative name
+        # carrying path separators would escape assets/fonts/ after os.path.join
+        # and let a config probe arbitrary paths. Only bare filenames resolve.
+        assert resolve_font_path(hostile) is None
+
+    def test_bare_filename_still_resolves(self):
+        # The guard must not reject legitimate bare names.
+        assert resolve_font_path("PressStart2P-Regular.ttf") is not None
+
     def test_schema_manager_expands_on_load(self, tmp_path):
         # The web-UI form path: SchemaManager.load_schema serves the
         # expanded schema so the style blocks actually appear in the UI.

test/test_plugin_system.py

@@ -250,5 +250,125 @@ class TestBasePlugin:
             
         config = {"enabled": True, "live_priority": True}
         plugin = ConcretePlugin("test", config, mock_display_manager, mock_cache_manager, None)
-        
+
         assert plugin.has_live_priority() is True
+
+
+class TestBasePluginGlobalConfig:
+    """global_config exposes device-wide settings that self.config cannot.
+
+    The sports scoreboards read `getattr(self, 'global_config', {})` to find
+    the shared target_fps; before this property existed nothing ever set that
+    attribute, so the lookup silently returned {} and the setting could never
+    take effect on any core.
+    """
+
+    @staticmethod
+    def _plugin(display_manager, cache_manager, plugin_manager=None):
+        from src.plugin_system.base_plugin import BasePlugin
+
+        class ConcretePlugin(BasePlugin):
+            def update(self): pass
+            def display(self, force_clear=False): pass
+
+        return ConcretePlugin(
+            "test", {"enabled": True}, display_manager, cache_manager, plugin_manager
+        )
+
+    @staticmethod
+    def _manager_with(config):
+        """A stand-in manager exposing config_manager.get_config()."""
+        manager = MagicMock()
+        manager.config_manager.get_config.return_value = config
+        return manager
+
+    def test_reads_config_from_plugin_manager(self, mock_display_manager, mock_cache_manager):
+        plugin = self._plugin(
+            mock_display_manager, mock_cache_manager,
+            self._manager_with({"target_fps": 100}),
+        )
+        assert plugin.global_config["target_fps"] == 100
+
+    def test_falls_back_to_cache_manager(self, mock_display_manager):
+        # The core that hangs config_manager off the cache manager instead.
+        cache_manager = self._manager_with({"target_fps": 75})
+        plugin = self._plugin(mock_display_manager, cache_manager, plugin_manager=None)
+        assert plugin.global_config["target_fps"] == 75
+
+    def test_plugin_manager_wins_over_cache_manager(self, mock_display_manager):
+        plugin = self._plugin(
+            mock_display_manager,
+            self._manager_with({"target_fps": 75}),
+            self._manager_with({"target_fps": 100}),
+        )
+        assert plugin.global_config["target_fps"] == 100
+
+    def test_empty_plugin_manager_config_falls_through(self, mock_display_manager):
+        """An empty first source means "not loaded yet", not "the answer".
+
+        Both managers default to the same config/config.json, so falling
+        through cannot pick up a different file. Returning {} here instead
+        would silently disable every setting read through this property --
+        the exact failure this property exists to fix.
+        """
+        plugin = self._plugin(
+            mock_display_manager,
+            self._manager_with({"target_fps": 100}),   # cache_manager
+            self._manager_with({}),                    # plugin_manager: empty
+        )
+        assert plugin.global_config["target_fps"] == 100
+
+    def test_returns_empty_dict_when_no_config_manager(self, mock_display_manager):
+        # Plain objects: no config_manager attribute at all.
+        plugin = self._plugin(mock_display_manager, object(), object())
+        assert plugin.global_config == {}
+
+    def test_unreadable_config_does_not_raise(self, mock_display_manager):
+        # A plugin must still load when the config on disk is broken.
+        broken = MagicMock()
+        broken.config_manager.get_config.side_effect = OSError("unreadable")
+        plugin = self._plugin(mock_display_manager, broken, broken)
+        assert plugin.global_config == {}
+
+    def test_non_dict_config_is_rejected(self, mock_display_manager):
+        # A stub or half-built manager can return a non-mapping; handing that
+        # back would blow up later in numeric code, far from the cause.
+        plugin = self._plugin(
+            mock_display_manager, object(), self._manager_with("not-a-dict")
+        )
+        assert plugin.global_config == {}
+
+    def test_missing_property_degrades_to_default(self, mock_display_manager, mock_cache_manager):
+        # How plugins actually call it, so a plugin written against this core
+        # still loads on one that predates the property.
+        plugin = self._plugin(mock_display_manager, mock_cache_manager, object())
+        assert getattr(plugin, "global_config", {}).get("target_fps") is None
+
+    def test_plugin_may_still_assign_global_config(self, mock_display_manager, mock_cache_manager):
+        # news, stock-news, ledmatrix-stocks, ledmatrix-elections,
+        # ledmatrix-leaderboard and nfl-draft all do exactly this. Without a
+        # setter the property raises "has no setter" and those plugins stop
+        # loading entirely.
+        from src.plugin_system.base_plugin import BasePlugin
+
+        class AssigningPlugin(BasePlugin):
+            def __init__(self, *args, **kwargs):
+                super().__init__(*args, **kwargs)
+                self.global_config = self.config.get("global", {})
+
+            def update(self): pass
+            def display(self, force_clear=False): pass
+
+        plugin = AssigningPlugin(
+            "news", {"enabled": True, "global": {"scroll_speed": 2}},
+            mock_display_manager, mock_cache_manager, self._manager_with({"target_fps": 100}),
+        )
+        # The plugin's own value wins over the resolved config.
+        assert plugin.global_config == {"scroll_speed": 2}
+
+    def test_template_ships_a_global_target_fps(self):
+        # The plumbing is useless if the setting isn't in the shipped config.
+        import json
+        with open("config/config.template.json") as fh:
+            template = json.load(fh)
+        assert template.get("target_fps") == 100

test/test_sports_base_characterization.py

@@ -79,9 +79,10 @@ def _competitor(abbr, team_id, score, home_away, record="30-10-5"):
             "logo": None,
         },
         "records": [{"summary": record}],
-        # The hockey extractor iterates competitor["statistics"] and
-        # returns None for the whole event when the key is absent (see
-        # test_hockey_event_without_statistics_returns_none).
+        # The hockey extractor reads competitor["statistics"] for shot counts;
+        # it now defaults to an empty list when the key is absent rather than
+        # dropping the whole event (see
+        # test_hockey_event_without_statistics_still_extracts).
         "statistics": [],
     }
 
@@ -434,6 +435,17 @@ def build_manager(monkeypatch, tmp_path):
         "src.base_classes.sports.core.get_background_service",
         lambda *args, **kwargs: MagicMock())
 
+    # Rig requests.Session BEFORE any manager is built. Construction creates
+    # both SportsCore.session and the ESPNDataSource.session; replacing only
+    # manager.session after the fact (below) leaves data_source.session real,
+    # so an accidental fetch during or right after construction could reach
+    # the network. Patching the class makes every session created here raise.
+    def _offline_get(*args, **kwargs):
+        raise requests.exceptions.ConnectionError(
+            "characterization tests are offline")
+
+    monkeypatch.setattr(requests.Session, "get", _offline_get)
+
     def build(cls, schedule, **mode_cfg):
         config = {
             "timezone": "UTC",

test/test_sports_capabilities.py

@@ -16,6 +16,7 @@ See docs/SPORTS_UNIFICATION.md.
 """
 
 import sys
+import time
 from unittest.mock import MagicMock
 
 import pytest
@@ -754,20 +755,35 @@ class TestDisplayTakeover:
         manager._draw_celebration_layout.assert_called_once()
 
     def test_expired_celebration_clears_and_defers(self, celebrating):
-        manager = celebrating(mode_config={"celebration_duration": 0})
+        # A short-but-valid duration: celebration_duration is clamped to a 1.0s
+        # floor, so a config of 0 does NOT expire on the next frame. Backdate
+        # started_at past the window to exercise the real expiry branch —
+        # otherwise the celebration is still active and this only passes
+        # because _draw_celebration_layout happens to raise in the harness
+        # (that path is covered by test_a_render_failure_falls_through).
+        manager = celebrating(mode_config={"celebration_duration": 1})
         manager._check_for_score(game("g1", home_score=0))
         manager._check_for_score(game("g1", home_score=1))
+        manager.active_celebration["started_at"] = time.time() - 2  # past the 1s window
+        # Mock the layout so a render can't raise: otherwise the exception
+        # branch clears the celebration too, and this test would pass whether
+        # or not expiry actually fired. An expired celebration must NOT render.
+        manager._draw_celebration_layout = MagicMock()
         assert manager.display() is True
+        manager._draw_celebration_layout.assert_not_called()
         assert manager.active_celebration is None
         assert manager.display_calls == [False]
 
     def test_expiry_resets_the_dwell_clock(self, celebrating):
         """So the scorebug resumes on the scoring game for a full duration
         before rotation can move on."""
-        manager = celebrating(mode_config={"celebration_duration": 0})
+        manager = celebrating(mode_config={"celebration_duration": 1})
         manager._check_for_score(game("g1", home_score=0))
         manager._check_for_score(game("g1", home_score=1))
+        manager.active_celebration["started_at"] = time.time() - 2  # past the 1s window
+        manager._draw_celebration_layout = MagicMock()  # expiry, not a render failure
         manager.display()
+        manager._draw_celebration_layout.assert_not_called()
         assert manager.last_game_switch > 0
 
     def test_a_render_failure_falls_through_to_the_scorebug(self, celebrating):

test/test_web_api.py

@@ -167,6 +167,78 @@ class TestConfigAPI:
             'enabled': True, 'copies': 2, 'axis': 'vertical',
         }
 
+    def test_save_target_fps(self, client, mock_config_manager):
+        """The device-wide scroll frame rate persists as a top-level int."""
+        response = client.post(
+            '/api/v3/config/main',
+            data={'target_fps': '90'},
+            content_type='application/x-www-form-urlencoded',
+        )
+
+        assert response.status_code == 200
+        saved = mock_config_manager.save_config_atomic.call_args[0][0]
+        # Must be the coerced int, not the raw form string -- the generic
+        # remaining-keys loop would otherwise write '90' back over it.
+        assert saved['target_fps'] == 90
+
+    def test_save_target_fps_alone_does_not_reset_other_general_settings(
+            self, client, mock_config_manager):
+        """A target_fps-only POST must not be treated as a full General-tab save.
+
+        The general branch reads web_display_autostart as an unchecked-checkbox
+        (absent means False), so counting target_fps as a general update would
+        silently switch autostart off for anyone setting only the frame rate.
+        """
+        mock_config_manager.load_config.return_value['web_display_autostart'] = True
+
+        response = client.post(
+            '/api/v3/config/main',
+            data={'target_fps': '90'},
+            content_type='application/x-www-form-urlencoded',
+        )
+
+        assert response.status_code == 200
+        saved = mock_config_manager.save_config_atomic.call_args[0][0]
+        assert saved['web_display_autostart'] is True
+
+    @pytest.mark.parametrize('value', [90.5, 90.0, True])
+    def test_save_target_fps_rejects_non_integer_json(self, client, mock_config_manager, value):
+        """int() would truncate silently: 90.5 -> 90, True -> 1.
+
+        Only JSON can carry these; a form post sends '90.5', which int()
+        already rejects.
+        """
+        response = client.post(
+            '/api/v3/config/main',
+            data=json.dumps({'target_fps': value}),
+            content_type='application/json',
+        )
+
+        assert response.status_code == 400
+
+    @pytest.mark.parametrize('value', ['20', '250', 'fast'])
+    def test_save_target_fps_rejects_out_of_range(self, client, mock_config_manager, value):
+        """Values ScrollHelper would silently clamp are reported instead."""
+        response = client.post(
+            '/api/v3/config/main',
+            data={'target_fps': value},
+            content_type='application/x-www-form-urlencoded',
+        )
+
+        assert response.status_code == 400
+
+    def test_save_target_fps_accepts_bounds(self, client, mock_config_manager):
+        """Both endpoints of the documented range are valid."""
+        for value in ('30', '200'):
+            response = client.post(
+                '/api/v3/config/main',
+                data={'target_fps': value},
+                content_type='application/x-www-form-urlencoded',
+            )
+            assert response.status_code == 200, f"{value} should be accepted"
+            saved = mock_config_manager.save_config_atomic.call_args[0][0]
+            assert saved['target_fps'] == int(value)
+
     def test_save_double_sided_unchecked_disables(self, client, mock_config_manager):
         """An omitted 'enabled' checkbox is saved as disabled, not left stale."""
         response = client.post(

web_interface/blueprints/api_v3.py

@@ -747,6 +747,36 @@ def save_main_config():
         if 'timezone' in data:
             current_config['timezone'] = data['timezone']
 
+        # Device-wide scroll frame rate, read by plugins via
+        # BasePlugin.global_config. Bounds match ScrollHelper.set_target_fps,
+        # which clamps silently -- rejecting here instead means a value that
+        # would have been quietly altered is reported rather than appearing to
+        # save and then behaving differently.
+        if 'target_fps' in data and data['target_fps'] not in ('', None):
+            raw_target_fps = data['target_fps']
+            # A JSON body can carry real floats and bools, where int() would
+            # silently truncate: 90.5 would save as 90, and true as 1. Reject
+            # them rather than storing a value the user did not ask for. Form
+            # posts arrive as strings, so '90.5' still fails in int() below.
+            if isinstance(raw_target_fps, (bool, float)):
+                return jsonify({
+                    'status': 'error',
+                    'message': "Invalid value for target_fps: must be an integer"
+                }), 400
+            try:
+                target_fps = int(raw_target_fps)
+            except (ValueError, TypeError):
+                return jsonify({
+                    'status': 'error',
+                    'message': "Invalid value for target_fps: must be an integer"
+                }), 400
+            if not (30 <= target_fps <= 200):
+                return jsonify({
+                    'status': 'error',
+                    'message': "Invalid value for target_fps: must be between 30 and 200"
+                }), 400
+            current_config['target_fps'] = target_fps
+
         # Handle location settings
         if 'city' in data or 'state' in data or 'country' in data:
             if 'location' not in current_config:
@@ -1282,7 +1312,7 @@ def save_main_config():
             if key in ['timezone', 'city', 'state', 'country',
                        'web_display_autostart', 'auto_discover',
                        'auto_load_enabled', 'development_mode',
-                       'plugins_directory']:
+                       'plugins_directory', 'target_fps']:
                 continue
             # Skip fields that are already handled above in their own named sections.
             # Without this, every form field name lands as a top-level config key too.

web_interface/templates/v3/partials/general.html

@@ -49,6 +49,18 @@
             <label for="timezone" class="block text-sm font-medium text-gray-700">Timezone{{ ui.help_tip('Time zone used for clocks, schedules, and time-based content.\nChoose the zone where the display physically lives so on/off schedules fire at the correct local time.', 'Timezone') }}</label>
             <div id="timezone_container" class="mt-1"></div>
         </div>
+
+        <!-- Scroll frame rate (device-wide) -->
+        <div class="form-group" id="setting-general-target-fps" data-setting-key="target_fps">
+            <label for="target_fps" class="block text-sm font-medium text-gray-700">Scroll Frame Rate{{ ui.help_tip('Frames per second for scrolling content, applied across plugins that scroll.\nHigher is smoother but uses more CPU; lower frees CPU but looks steppier.\nRange 30-200. Default: 100.', 'Scroll Frame Rate') }}</label>
+            <input type="number"
+                   id="target_fps"
+                   name="target_fps"
+                   value="{{ main_config.target_fps or 100 }}"
+                   min="30"
+                   max="200"
+                   class="form-control">
+        </div>
         <script>
         (function() {
             // Track if already initialized to prevent re-render