From 48a004a0ac8af0e9f7f098c033487d5e3070edb3 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Fri, 4 Sep 2026 20:18:23 -0400 Subject: [PATCH 01/11] fix(core): register tom_thumb, accept frame_hold in the test double, wire api_v3's managers Three independent fixes found while validating every plugin on a 256x64 rig. FontManager never registered tom_thumb even though assets/fonts/tom-thumb.bdf ships with the core, so every plugin offering it logged "Font family 'tom_thumb' not found" (16 warnings per countdown render) and had to carry a private loader to use a bundled font. Closes #524. VisualTestDisplayManager.set_scrolling_state() lacked the frame_hold parameter that DisplayManager gained, so any plugin passing it died with TypeError at render time and failed every size. Nine plugins now make that call; ledmatrix-stocks and ledmatrix-leaderboard were failing outright and the other seven only passed because their scroll path was unreachable without data. Closes #525. api_v3 declared module-level config_manager/plugin_manager = None that nothing ever assigned -- app.py sets the blueprint attributes, which the other 150+ call sites use. Three sites read the decoys, so /health reported the config unreadable and the plugin system uninitialised (making "degraded" permanent and unreachable-by-design) and /display/current fell back to a hardcoded 128x64 on every rig. The decoys are removed rather than assigned, so a bare name is now a NameError at test time instead of a silent None. The same function's first-call uptime was computed from two separate clock reads and came out negative. Closes #529. Verified on the rig: both previously-failing plugins render, the tom_thumb warnings are gone, /health reports "healthy" with all three checks passing, and /display/current reports the real 256x64. --- src/font_manager.py | 3 +- .../testing/visual_display_manager.py | 11 ++++++-- web_interface/blueprints/api_v3.py | 28 ++++++++++++------- 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/src/font_manager.py b/src/font_manager.py index 8be283be..93640808 100644 --- a/src/font_manager.py +++ b/src/font_manager.py @@ -96,7 +96,8 @@ def __init__(self, config: Dict[str, Any]): self.common_fonts = { "press_start": "assets/fonts/PressStart2P-Regular.ttf", "four_by_six": "assets/fonts/4x6-font.ttf", - "five_by_seven": "assets/fonts/5x7.bdf" + "five_by_seven": "assets/fonts/5x7.bdf", + "tom_thumb": "assets/fonts/tom-thumb.bdf" # Note: cozette_bdf removed - font file not available # To re-enable: download cozette.bdf from https://github.com/the-moonwitch/Cozette # and add: "cozette_bdf": "assets/fonts/cozette.bdf" diff --git a/src/plugin_system/testing/visual_display_manager.py b/src/plugin_system/testing/visual_display_manager.py index e33d2309..732d6d61 100644 --- a/src/plugin_system/testing/visual_display_manager.py +++ b/src/plugin_system/testing/visual_display_manager.py @@ -506,9 +506,16 @@ def draw_text_with_icons(self, text: str, icons: List[tuple] = None, # Scrolling state (no-op interface compat) # ------------------------------------------------------------------ - def set_scrolling_state(self, is_scrolling: bool): - """Set the current scrolling state (no-op for testing).""" + def set_scrolling_state(self, is_scrolling: bool, frame_hold: int = 1): + """Set the current scrolling state (no-op for testing). + + ``frame_hold`` mirrors DisplayManager.set_scrolling_state so a plugin + that paces its scroll can be rendered here. Without it every such + plugin raised TypeError at render time and failed every size, which is + invisible until a plugin happens to pass the argument. + """ self._scrolling_state['is_scrolling'] = is_scrolling + self._scrolling_state['frame_hold'] = frame_hold if is_scrolling: self._scrolling_state['last_scroll_activity'] = time.time() diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 35dfd885..845c6884 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -78,8 +78,11 @@ def _scrub_git_remote_url(url: str) -> str: return url # Will be initialized when blueprint is registered -config_manager = None -plugin_manager = None +# NOTE: the managers live on the blueprint object (app.py sets +# api_v3.config_manager / api_v3.plugin_manager). Deliberately not +# mirrored as module globals: a bare `config_manager` used to resolve to +# a None that was never assigned, which silently disabled the /health +# checks and made /display/current fall back to a hardcoded 128x64. plugin_store_manager = None saved_repositories_manager = None cache_manager = None @@ -1598,11 +1601,16 @@ def get_health(): } # Check web interface service + # Stamp the start time before measuring against it -- reading it with a + # fallback of time.time() and only assigning afterwards made the very + # first call subtract two separate clock reads, reporting a small + # negative uptime. + if not hasattr(get_health, '_start_time'): + get_health._start_time = time.time() health_status['services']['web_interface'] = { 'status': 'running', - 'uptime_seconds': time.time() - (getattr(get_health, '_start_time', time.time())) + 'uptime_seconds': time.time() - get_health._start_time } - get_health._start_time = getattr(get_health, '_start_time', time.time()) # Check display service display_service_status = _get_display_service_status() @@ -1613,8 +1621,8 @@ def get_health(): # Check config file accessibility try: - if config_manager: - test_config = config_manager.load_config() + if api_v3.config_manager: + test_config = api_v3.config_manager.load_config() health_status['checks']['config_file'] = { 'status': 'accessible', 'readable': True @@ -1633,9 +1641,9 @@ def get_health(): # Check plugin system try: - if plugin_manager: + if api_v3.plugin_manager: # Try to discover plugins (lightweight check) - plugin_count = len(plugin_manager.get_available_plugins()) if hasattr(plugin_manager, 'get_available_plugins') else 0 + plugin_count = len(api_v3.plugin_manager.get_available_plugins()) if hasattr(api_v3.plugin_manager, 'get_available_plugins') else 0 health_status['checks']['plugin_system'] = { 'status': 'operational', 'plugin_count': plugin_count @@ -2468,8 +2476,8 @@ def get_display_current(): # Get display dimensions from config try: - if config_manager: - main_config = config_manager.load_config() + if api_v3.config_manager: + main_config = api_v3.config_manager.load_config() hardware_config = main_config.get('display', {}).get('hardware', {}) cols = hardware_config.get('cols', 64) chain_length = hardware_config.get('chain_length', 2) From a645327c52a50df6414c6c229d8c35b06b8c5f3e Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Fri, 4 Sep 2026 20:29:50 -0400 Subject: [PATCH 02/11] fix(core): unique snapshot temp name, honour on-demand requests, skip empty starlark The preview snapshot wrote through a fixed ".tmp". /tmp is world-writable and sticky, and the display service runs as a different user from the tooling, so a leftover temp owned by anyone else became unopenable even by root -- fs.protected_regular refuses O_CREAT on a foreign file in a sticky directory. The preview and the health check's liveness proxy then froze until someone deleted the file by hand; on the test rig that meant 23 hours of a healthy display reporting "hardware: stale". Now uses tempfile.mkstemp with cleanup on failure, matching the hardware-status write a few hundred lines above. Closes #528. _poll_on_demand_requests read its mailbox with max_age=3600, and get() defaults the in-memory TTL to max_age -- so the first request was pinned in memory for an hour and every later poll returned that stale copy. No second on-demand request was honoured until the service restarted, while the API kept returning 200. get() already documents memory_ttl=0 for exactly this cross-process case. The consumed request is also now deleted: leaving it on disk meant a restart replayed the previous request, activated it, and ignored the one the caller had just made. Closes #530. starlark-apps returned None from display() when it has no app to show, which is the state of every install without Pixlet and of a fresh one before any app is added. The controller only skips on a boolean False, so that held a black panel for the full display_duration instead of rotating on. Closes #456 (core side). Verified on the rig: two consecutive on-demand requests with no restart between them are both activated, where the second was previously dropped in silence. --- plugin-repos/starlark-apps/manager.py | 14 +++++++++++--- src/display_controller.py | 21 +++++++++++++++++++-- src/display_manager.py | 24 +++++++++++++++++++++--- 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/plugin-repos/starlark-apps/manager.py b/plugin-repos/starlark-apps/manager.py index 3da8c24d..86ad36b6 100644 --- a/plugin-repos/starlark-apps/manager.py +++ b/plugin-repos/starlark-apps/manager.py @@ -681,12 +681,18 @@ def update(self) -> None: if app.is_enabled() and app.should_render(current_time): self._render_app(app, force=False) - def display(self, force_clear: bool = False) -> None: + def display(self, force_clear: bool = False) -> bool: """ Display current Starlark app. This method is called during the display rotation. Displays frames from the currently active app. + + Returns False when there is no app to show -- which is the state of + every install without Pixlet, and of a fresh one before any app is + added. The display controller only skips a mode on a boolean False + (it checks isinstance(result, bool)), so returning None held a black + panel for the full display_duration instead of rotating on. """ try: if force_clear: @@ -699,20 +705,22 @@ def display(self, force_clear: bool = False) -> None: if not self.current_app: # No apps available self.logger.debug("No Starlark apps to display") - return + return False # Render app if needed if not self.current_app.frames: success = self._render_app(self.current_app, force=True) if not success: self.logger.error(f"Failed to render app: {self.current_app.app_id}") - return + return False # Display current frame self._display_frame() + return True except Exception as e: self.logger.error(f"Error displaying Starlark app: {e}") + return False def _select_next_app(self) -> None: """Select the next enabled app for display.""" diff --git a/src/display_controller.py b/src/display_controller.py index bb650698..72100568 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -1271,8 +1271,16 @@ def _poll_on_demand_requests(self) -> None: """Poll cache for new on-demand requests from external controllers.""" try: # Use a long max_age (1 hour) to ensure requests aren't expired before processing - # The request_id check prevents duplicate processing - request = self.cache_manager.get('display_on_demand_request', max_age=3600) + # The request_id check prevents duplicate processing. + # + # memory_ttl=0 is required, not optional: this key is a mailbox the + # web process writes and this process reads. get() defaults the + # in-memory TTL to max_age, so without it the first request read was + # pinned in memory for the full hour and every later poll returned + # that stale copy -- meaning no second on-demand request was honoured + # for an hour, while the API still reported success. + request = self.cache_manager.get('display_on_demand_request', + max_age=3600, memory_ttl=0) except (OSError, RuntimeError, ValueError, TypeError) as err: logger.error("Failed to read on-demand request: %s", err, exc_info=True) return @@ -1318,6 +1326,15 @@ def _poll_on_demand_requests(self) -> None: # Mark as processed BEFORE processing (to prevent duplicate processing) self.cache_manager.set('display_on_demand_processed_id', request_id, ttl=3600) self.on_demand_request_id = request_id + # Consume the mailbox entry. Leaving it on disk meant a restart replayed + # the previous request: the fresh controller read it, activated it and + # cached it, so the request the caller had just made was ignored and the + # panel silently showed the earlier plugin. processed_id still guards + # against double-processing if this delete fails. + try: + self.cache_manager.delete('display_on_demand_request') + except Exception as err: # pragma: no cover - best-effort cleanup + logger.debug("Could not clear the on-demand request mailbox: %s", err) if action == 'start': logger.info("Processing on-demand start request for plugin: %s", request.get('plugin_id')) diff --git a/src/display_manager.py b/src/display_manager.py index e578ad7c..076e9220 100644 --- a/src/display_manager.py +++ b/src/display_manager.py @@ -1453,12 +1453,30 @@ def _write_snapshot_if_due(self) -> None: if parent_dir and str(parent_dir) != '/tmp': # nosec B108 - guard to skip /tmp for permission ops ensure_directory_permissions(parent_dir, get_assets_dir_mode()) self._snapshot_dir_prepared = True - # Write atomically: temp then replace - tmp_path = f"{self._snapshot_path}.tmp" - self.image.save(tmp_path, format='PNG') + # Write atomically: temp then replace. The temp name must be + # unique, not ".tmp": /tmp is world-writable and sticky, + # and this file is written by whichever user the display service + # runs as while tests and tooling run as someone else. A leftover + # fixed-name temp owned by another user is then unopenable even by + # root (fs.protected_regular refuses O_CREAT on a foreign file in a + # sticky dir), which froze the preview and the health check's + # liveness proxy until somebody deleted it by hand. Same pattern as + # the hardware-status write above. + _fd, tmp_path = tempfile.mkstemp( + dir=str(snapshot_path_obj.parent), + prefix=f".{snapshot_path_obj.name}.", suffix=".tmp") try: + with os.fdopen(_fd, "wb") as _f: + self.image.save(_f, format='PNG') + os.chmod(tmp_path, 0o644) os.replace(tmp_path, self._snapshot_path) except Exception: + # Never leave the temp behind -- that is what made the failure + # permanent rather than transient. + try: + os.unlink(tmp_path) + except OSError: + pass # Fallback to direct save if replace not supported self.image.save(self._snapshot_path, format='PNG') # Set proper file permissions after saving From e50a100814fe43342a318dae1ee584f5707944a4 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Fri, 4 Sep 2026 20:32:39 -0400 Subject: [PATCH 03/11] perf(harness): share one cache across a plugin's renders _instantiate built a fresh MockCacheManager for every (size, mode), and that mock is a per-instance in-memory dict, so each render was a cold start. A plugin that fetches per game or per player re-fetched everything N times over -- baseball-scoreboard at one size took 840s for nine renders where the arithmetic said ~72s, and at eight sizes it exceeded a 900s timeout. The second and later renders also never exercised the cache-hit path, which is what a running rig executes almost all of the time, so a caching regression could not be caught here. The cache is now built once per render_plugin_matrix call and threaded down. The display manager stays per-render -- the bounds checking depends on that -- so only fetched data is shared. Measured on the rig, same render counts and same goldens: tide-display 2s -> 1s (32 renders) cricket-scoreboard 10s -> 3s (24 renders) No pass/fail change across tide-display, cricket-scoreboard, clock-simple, geochron, christmas-countdown, of-the-day, web-ui-info and incoming-packages. Closes #533. --- src/plugin_system/testing/harness.py | 38 +++++++++++++++++++++------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/src/plugin_system/testing/harness.py b/src/plugin_system/testing/harness.py index 51c688a2..260b0ecf 100644 --- a/src/plugin_system/testing/harness.py +++ b/src/plugin_system/testing/harness.py @@ -116,14 +116,23 @@ def list_modes(plugin_instance: Any, manifest: Dict[str, Any], plugin_id: str) - def _instantiate(plugin_id: str, manifest: Dict[str, Any], plugin_dir: Path, config: Dict[str, Any], mock_data: Dict[str, Any], - display_manager: Any) -> Any: - """Load and construct a plugin instance with mocked managers.""" + display_manager: Any, cache_manager: Any = None) -> Any: + """Load and construct a plugin instance with mocked managers. + + Pass ``cache_manager`` to share one cache across the renders of a plugin. + Building a fresh one per (size, mode) made every render a cold start, so a + plugin that fetches per game or per player re-fetched everything N times -- + baseball-scoreboard took 840s for nine renders where ~72s was the arithmetic + -- and the cache-hit path, which is what a running rig executes almost + always, was never exercised. + """ from src.plugin_system.plugin_loader import PluginLoader from src.plugin_system.testing import MockCacheManager, MockPluginManager - cache_manager = MockCacheManager() - for key, value in (mock_data or {}).items(): - cache_manager.set(key, value) + if cache_manager is None: + cache_manager = MockCacheManager() + for key, value in (mock_data or {}).items(): + cache_manager.set(key, value) loader = PluginLoader() plugin_instance, _module = loader.load_plugin( @@ -202,25 +211,35 @@ def render_plugin_matrix( # rendering a smaller one, instead of being clipped into a false pass. extent = (max(w for w, _ in sizes), max(h for _, h in sizes)) + # One cache for the whole matrix: see _instantiate. The display manager + # stays per-render (the bounds checking depends on that); only fetched data + # is shared. + from src.plugin_system.testing import MockCacheManager + cache_manager = MockCacheManager() + for key, value in (mock_data or {}).items(): + cache_manager.set(key, value) + with _freeze(freeze_time): for width, height in sizes: results.extend(_render_size( plugin_id, manifest, plugin_dir, config, mock_data or {}, - width, height, run_update, extent, + width, height, run_update, extent, cache_manager, )) return results def _render_size(plugin_id, manifest, plugin_dir, config, mock_data, - width, height, run_update, extent) -> List[RenderResult]: + width, height, run_update, extent, + cache_manager=None) -> List[RenderResult]: """Render every mode at one size. A fresh instance per mode avoids state leaks.""" results: List[RenderResult] = [] # Discover modes once per size (instance build can depend on config). try: probe_dm = BoundsCheckingDisplayManager(width=width, height=height, overflow_extent=extent) - probe = _instantiate(plugin_id, manifest, plugin_dir, config, mock_data, probe_dm) + probe = _instantiate(plugin_id, manifest, plugin_dir, config, mock_data, probe_dm, + cache_manager) modes = list_modes(probe, manifest, plugin_id) except Exception as e: # noqa: BLE001 — surface any load failure as a result return [RenderResult(plugin_id, width, height, "", error=repr(e))] @@ -229,7 +248,8 @@ def _render_size(plugin_id, manifest, plugin_dir, config, mock_data, result = RenderResult(plugin_id, width, height, mode) dm = BoundsCheckingDisplayManager(width=width, height=height, overflow_extent=extent) try: - inst = _instantiate(plugin_id, manifest, plugin_dir, config, mock_data, dm) + inst = _instantiate(plugin_id, manifest, plugin_dir, config, mock_data, dm, + cache_manager) if run_update: try: inst.update() From 371bc69bd8ba7d27a6f057f3e4f9a39b5d1f12ad Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Fri, 4 Sep 2026 21:39:57 -0400 Subject: [PATCH 04/11] fix(scripts): run standalone plugin tests instead of collecting nothing run_plugin_tests.py discovered every plugin test file and handed the lot to pytest. Most plugin tests are standalone scripts -- module-level main() plus an `if __name__ == "__main__"` guard, signalling through an exit code -- and pytest collects zero items from those. The run printed how many files it had *found*, then "no tests ran", and exited without executing any of them. On a rig with all 44 first-party plugins that is 151 of 248 files. Files are now classified and each kind runs under the right runner: pytest for real test modules, subprocess for scripts, honouring the 0 pass / 2 skip / 1 fail convention ledmatrix-plugins' own runner established (a script that wants a tty or an LED matrix is a skip, not a regression). Before: $ python3 scripts/run_plugin_tests.py -p countdown -d ~/LEDMatrix/plugin-repos Found 1 test file(s) collected 0 items no tests ran in 0.31s rc=0 After: Found 1 test file(s) -- 0 collectable, 1 standalone script(s) 1 passed, 0 skipped, 0 failed (scripts) rc=0 Verified across three shapes: countdown (1 script), jellyfin-now-playing and pomodoro-timer (pytest only, 16 and 42 tests), and ledmatrix-flights (11 files split 4 collectable / 7 scripts, all seven of which had never run). Closes #532. Running the flights scripts for the first time also surfaced four genuinely failing tests there, hidden by the mirror-image bug in the plugins repo's own runner -- filed as ChuckBuilds/ledmatrix-plugins#464 and #465. --- scripts/run_plugin_tests.py | 92 +++++++++++++++++++++++++++++++++---- 1 file changed, 84 insertions(+), 8 deletions(-) diff --git a/scripts/run_plugin_tests.py b/scripts/run_plugin_tests.py index 61e59710..baaf1075 100755 --- a/scripts/run_plugin_tests.py +++ b/scripts/run_plugin_tests.py @@ -6,6 +6,9 @@ Supports both unittest and pytest. """ +import os +import re +import subprocess import sys import argparse from pathlib import Path @@ -68,6 +71,65 @@ def _find_tests_in_dir(directory: Path) -> list: return sorted(set(test_files)) +def _is_script_style(path) -> bool: + """True when a test file is a standalone script, not a pytest module. + + Most plugin tests are written as `def main()` plus an `if __name__ == + "__main__"` guard and signal through an exit code. pytest collects zero + items from those, so handing them to pytest printed "no tests ran" and this + runner reported success over work it had not done -- 151 of 248 files on a + fully populated rig. + """ + try: + src = Path(path).read_text(encoding="utf-8", errors="replace") + except OSError: + return False + has_pytest_items = re.search(r"^\s*(def test_|class Test|async def test_)", src, re.M) + has_main_guard = "__main__" in src and "__name__" in src + return bool(has_main_guard and not has_pytest_items) + + +def run_script_tests(test_files: list, verbose: bool = False) -> int: + """Run standalone test scripts, honouring the 0 pass / 2 skip / 1 fail + convention that ledmatrix-plugins' own runner established. + + Scripts opt into skipping by printing "SKIP: " and exiting 2 -- + a script that needs a tty or an LED matrix is not a regression. + """ + env = dict(os.environ) + env.setdefault("PYTHONPATH", str(PROJECT_ROOT)) + env.setdefault("LEDMATRIX_CORE", str(PROJECT_ROOT)) + + passed = skipped = failed = 0 + failures = [] + for path in test_files: + try: + proc = subprocess.run([sys.executable, str(path)], cwd=str(Path(path).parent), + capture_output=True, text=True, env=env, + stdin=subprocess.DEVNULL, timeout=300) + rc = proc.returncode + tail = " | ".join((proc.stdout or proc.stderr or "").strip().splitlines()[-2:])[:200] + except subprocess.TimeoutExpired: + rc, tail = 1, "timed out after 300s" + if rc == 0: + passed += 1 + label = "pass" + elif rc == 2: + skipped += 1 + label = "SKIP" + else: + failed += 1 + label = "FAIL" + failures.append(f"{Path(path).name}: exit {rc} | {tail}") + if verbose or rc != 0: + print(f" [{label}] {Path(path).name}" + (f" -- {tail}" if rc != 0 else "")) + + print(f"\n{passed} passed, {skipped} skipped, {failed} failed (scripts)") + for f in failures: + print(f" - {f}", file=sys.stderr) + return 1 if failed else 0 + + def run_unittest_tests(test_files: list, verbose: bool = False) -> int: """ Run tests using unittest. @@ -186,11 +248,16 @@ def main(): print("No test files found in plugins directory") return 0 - print(f"Found {len(test_files)} test file(s)") + scripts = [f for f in test_files if _is_script_style(f)] + modules = [f for f in test_files if f not in scripts] + + print(f"Found {len(test_files)} test file(s)" + + (f" -- {len(modules)} collectable, {len(scripts)} standalone script(s)" + if scripts else "")) for test_file in test_files: print(f" - {test_file}") print() - + # Determine runner runner = args.runner if runner == 'auto': @@ -199,12 +266,21 @@ def main(): runner = 'pytest' except ImportError: runner = 'unittest' - - # Run tests - if runner == 'pytest': - return run_pytest_tests(test_files, args.verbose, args.coverage) - else: - return run_unittest_tests(test_files, args.verbose) + + # Standalone scripts cannot be collected by pytest or unittest -- run them + # as the scripts they are. Doing this rather than silently collecting zero + # items is the whole point: this runner used to report success having + # executed nothing. + rc = 0 + if scripts: + rc |= run_script_tests(scripts, args.verbose) + + if modules: + if runner == 'pytest': + rc |= run_pytest_tests(modules, args.verbose, args.coverage) + else: + rc |= run_unittest_tests(modules, args.verbose) + return rc if __name__ == '__main__': From c07dbb0bff6d95f3f584e8e0a658a344e46ef762 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Fri, 4 Sep 2026 22:13:11 -0400 Subject: [PATCH 05/11] fix(harness): give an empty-looking mode a few frames before warning about it check_plugin's "drew nothing but display() returned X" warning fired on a single frame, rendered with force_clear=True, under a frozen clock. All three defeat a scrolling plugin, whose first frame is legitimately its blank scroll-in buffer. Across 44 first-party plugins, 60 of 76 warnings were false -- the rate at which people stop reading a warning, which matters because the true positives are real: a mode that draws nothing and does not return False holds a blank panel for its whole display duration. An apparently-empty frame is now re-driven for up to 48 more frames with force_clear=False (force_clear means "reset the scroll", so repeating it would redraw frame 1 for ever) and with the clock advancing -- freezegun's factory where time is frozen, a real sleep where it is not, since scroll position is usually a function of elapsed time. The first frame that draws content replaces the result. The clock is moved back afterwards. It is shared by every render in the matrix, so time borrowed by the probe leaked into later modes and drifted their goldens -- f1_upcoming picked up 5 spurious drifts before this was restored. Measured on the rig: empty warns check before after f1-scoreboard 42 0 48 PASS / 0 FAIL, goldens intact ledmatrix-elections 16 0 16 PASS / 0 FAIL on-air 8 8 true positive, kept nfl-draft 8 8 true positive, kept clock-simple/geochron/ 0 0 unchanged christmas-countdown 58 false positives gone, both true positives kept, no golden regressions. Cost is confined to modes that really are blank: plugins that draw immediately are unchanged (clock-simple and tide-display still 2s), while on-air -- eight deliberately blank modes -- goes to 21s. Closes #527. --- src/plugin_system/testing/harness.py | 101 ++++++++++++++++++++++++++- 1 file changed, 98 insertions(+), 3 deletions(-) diff --git a/src/plugin_system/testing/harness.py b/src/plugin_system/testing/harness.py index 260b0ecf..0e16f584 100644 --- a/src/plugin_system/testing/harness.py +++ b/src/plugin_system/testing/harness.py @@ -14,6 +14,8 @@ import contextlib import http.client import inspect +import time +from datetime import timedelta import socket import ssl import urllib.error @@ -169,6 +171,96 @@ def _render_mode(plugin_instance: Any, mode: str) -> Any: return plugin_instance.display(force_clear=False) +# How many extra frames to drive before believing a mode really draws nothing. +# A scroll starts with its content off-panel, so frame 1 is legitimately blank; +# measured across the fleet, content appears by frame 2-4 (f1-scoreboard), +# frame 4 (ledmatrix-elections) and frame 38 at 64px (ledmatrix-leaderboard). +EMPTY_RECHECK_FRAMES = 48 +# Seconds to advance the clock between those frames. Scroll position is usually +# driven by elapsed time, which a frozen clock never provides. +EMPTY_RECHECK_STEP = 0.05 + + +def _has_content(image) -> bool: + """True when any pixel is lit above the threshold.""" + if image is None: + return False + return image.convert("L").point( + lambda p: 255 if p > _LIT_THRESHOLD else 0).getbbox() is not None + + +def _render_mode_again(plugin_instance: Any, mode: str) -> Any: + """Draw one more frame WITHOUT force_clear. + + _render_mode passes force_clear=True, which for a scrolling plugin means + "reset the scroll to the start" -- so repeating it would redraw frame 1 for + ever. The re-check needs the plugin to advance. + """ + sig = inspect.signature(plugin_instance.display) + if "display_mode" in sig.parameters: + return plugin_instance.display(force_clear=False, display_mode=mode) + return plugin_instance.display(force_clear=False) + + +def _settle_empty_frame(inst, mode, dm, result, freezer) -> None: + """Give an apparently-empty mode a few frames to draw before believing it. + + One frame is not evidence: a scroll's first frame is its blank scroll-in + buffer. Without this, every scrolling plugin was warned about -- 60 of 76 + warnings on a 44-plugin rig were false, which is the rate at which people + stop reading a warning. + """ + if result.error is not None or result.display_returned is False: + return + if _has_content(result.image): + return + # The frozen clock is shared by every render in the matrix, so any time this + # probe borrows has to be given back -- otherwise a mode that scrolls in + # leaves the clock advanced and every later mode renders at the wrong + # instant, drifting its golden. Seen as 5 spurious f1_upcoming drifts. + resume_at = None + if freezer is not None: + try: + resume_at = freezer() + except Exception: # noqa: BLE001 - only to restore, never load-bearing + resume_at = None + try: + _settle_loop(inst, mode, dm, result, freezer) + finally: + if resume_at is not None: + try: + freezer.move_to(resume_at) + except Exception: # noqa: BLE001 + pass + + +def _settle_loop(inst, mode, dm, result, freezer) -> None: + tick = getattr(freezer, "tick", None) if freezer is not None else None + for _ in range(EMPTY_RECHECK_FRAMES): + if tick is not None: + # timedelta rather than a bare float: freezegun has accepted a + # number only since 1.x, and a stale pin would raise here. + try: + tick(timedelta(seconds=EMPTY_RECHECK_STEP)) + except Exception: # noqa: BLE001 - pacing is best-effort + pass + else: + # No frozen clock, so the real one has to do the advancing. Without + # this the 48 frames run in microseconds, elapsed time stays ~0, and + # a scroll driven by elapsed time never moves -- which is exactly + # the plugin this check is trying not to slander. + time.sleep(EMPTY_RECHECK_STEP) + try: + result.display_returned = _render_mode_again(inst, mode) + except Exception: # noqa: BLE001 - the first frame already succeeded + return + image = dm.get_image() + if _has_content(image): + result.image = image + result.overflow = dm.check_overflow() + return + + def _freeze(freeze_time: Optional[str]): """Context manager that freezes wall-clock time when freeze_time is given, so time-dependent plugins (clocks, countdowns) render deterministic goldens.""" @@ -219,11 +311,11 @@ def render_plugin_matrix( for key, value in (mock_data or {}).items(): cache_manager.set(key, value) - with _freeze(freeze_time): + with _freeze(freeze_time) as freezer: for width, height in sizes: results.extend(_render_size( plugin_id, manifest, plugin_dir, config, mock_data or {}, - width, height, run_update, extent, cache_manager, + width, height, run_update, extent, cache_manager, freezer, )) return results @@ -231,7 +323,7 @@ def render_plugin_matrix( def _render_size(plugin_id, manifest, plugin_dir, config, mock_data, width, height, run_update, extent, - cache_manager=None) -> List[RenderResult]: + cache_manager=None, freezer=None) -> List[RenderResult]: """Render every mode at one size. A fresh instance per mode avoids state leaks.""" results: List[RenderResult] = [] @@ -268,6 +360,9 @@ def _render_size(plugin_id, manifest, plugin_dir, config, mock_data, result.display_returned = _render_mode(inst, mode) result.image = dm.get_image() result.overflow = dm.check_overflow() + # A blank first frame is not proof of a blank mode; see + # _settle_empty_frame. + _settle_empty_frame(inst, mode, dm, result, freezer) except Exception as e: # noqa: BLE001 — a display crash is a real failure result.error = repr(e) results.append(result) From 31bd0cb40ae80c392ce4f66cff670f2de97499cd Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Fri, 4 Sep 2026 22:38:04 -0400 Subject: [PATCH 06/11] fix(harness): load nested schema defaults, and merge caller config at leaf level load_config_defaults read only top-level properties. An object property carries its defaults on its children, not on itself, so everything nested was dropped -- 2,386 defaults across 37 of 44 plugins, soccer-scoreboard alone losing 539 of 565. render_plugin_matrix's comment says the plugin then "behaves like a real install", which for most of the fleet it did not. _defaults_from_properties now recurses. merge_config deep-merges the caller's config onto the result so an override lands at the leaf: a shallow merge would let -c '{"nhl": {"enabled": true}}' replace the whole nhl subtree and discard every other nhl default, which is the same class of bug being fixed here. Measured before/after across all 49 installed plugins on the rig: **no render changed** -- identical PASS/FAIL counts, byte-identical output, goldens intact. Plugins already fall back to the same values internally via config.get(key, default), so supplying them explicitly agrees with what they were doing. The defaults really are arriving now: ufc-scoreboard 9 -> 87 defaults ledmatrix-flights 51 -> 95 masters-tournament 10 -> 51 cricket-scoreboard 22 -> 50 tide-display 12 -> 18 and hockey-scoreboard, which used to load nhl.enabled=None, now gets nhl.enabled=True with its full display_modes block. Caveat worth carrying: the eight plugins with the most nested config (soccer, baseball, basketball, hockey, lacrosse, football, afl, nrl -- 1,634 of the 2,386 dropped defaults, 68%) could not be measured. They import src.common.sports_shared, which the test rig's core branch predates, so they fail to load there identically before and after. Re-run this comparison against a core that has that module before trusting the "nothing changed" result for them; those are exactly the plugins whose renders should change most. Closes #531. --- src/plugin_system/testing/harness.py | 6 ++-- src/plugin_system/testing/loading.py | 45 ++++++++++++++++++++++++---- 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/src/plugin_system/testing/harness.py b/src/plugin_system/testing/harness.py index 0e16f584..d46c11c7 100644 --- a/src/plugin_system/testing/harness.py +++ b/src/plugin_system/testing/harness.py @@ -27,7 +27,7 @@ from src.logging_config import get_logger from .bounds_display_manager import BoundsCheckingDisplayManager -from .loading import load_config_defaults, load_manifest +from .loading import load_config_defaults, load_manifest, merge_config from .sizes import DEFAULT_TEST_SIZES, safe_mode_filename, size_label logger = get_logger("[Plugin Harness]") @@ -294,7 +294,9 @@ def render_plugin_matrix( manifest = load_manifest(plugin_dir) # Start from config_schema.json defaults so the plugin behaves like a real # install; explicit caller config still wins over a schema default. - config = {"enabled": True, **load_config_defaults(plugin_dir), **(config or {})} + config = merge_config( + merge_config({"enabled": True}, load_config_defaults(plugin_dir)), + config or {}) sizes = sizes or DEFAULT_TEST_SIZES results: List[RenderResult] = [] diff --git a/src/plugin_system/testing/loading.py b/src/plugin_system/testing/loading.py index 061e95a0..326b3e73 100644 --- a/src/plugin_system/testing/loading.py +++ b/src/plugin_system/testing/loading.py @@ -33,6 +33,45 @@ def load_manifest(plugin_dir: Union[str, Path]) -> Dict[str, Any]: return json.load(f) +def _defaults_from_properties(properties: Dict[str, Any]) -> Dict[str, Any]: + """Defaults for one `properties` block, recursing into nested objects. + + An object property carries its defaults on its children, not on itself, so + reading only the top level dropped everything nested. That is most of the + fleet: config organised by league, or under customization/display_options, + lost 2,386 defaults across 37 of 44 plugins -- soccer-scoreboard alone lost + 539 of 565 -- and the harness rendered them with a config no install would + ever have. + """ + defaults: Dict[str, Any] = {} + for key, prop in (properties or {}).items(): + if not isinstance(prop, dict): + continue + if prop.get('type') == 'object' and isinstance(prop.get('properties'), dict): + nested = _defaults_from_properties(prop['properties']) + if nested: + defaults[key] = nested + elif 'default' in prop: + defaults[key] = prop['default'] + return defaults + + +def merge_config(base: Dict[str, Any], override: Dict[str, Any]) -> Dict[str, Any]: + """Deep-merge override onto base, without dropping sibling defaults. + + A shallow merge would let `-c '{"nhl": {"enabled": true}}'` replace the whole + nhl subtree and silently discard every other nhl default -- the same class of + bug this function exists to fix. + """ + merged = dict(base) + for key, value in (override or {}).items(): + if isinstance(value, dict) and isinstance(merged.get(key), dict): + merged[key] = merge_config(merged[key], value) + else: + merged[key] = value + return merged + + def load_config_defaults(plugin_dir: Union[str, Path]) -> Dict[str, Any]: """Extract default values from a plugin's config_schema.json (empty if none).""" schema_path = Path(plugin_dir) / 'config_schema.json' @@ -40,11 +79,7 @@ def load_config_defaults(plugin_dir: Union[str, Path]) -> Dict[str, Any]: return {} with open(schema_path, 'r') as f: schema = json.load(f) - defaults: Dict[str, Any] = {} - for key, prop in schema.get('properties', {}).items(): - if isinstance(prop, dict) and 'default' in prop: - defaults[key] = prop['default'] - return defaults + return _defaults_from_properties(schema.get('properties', {})) def load_harness_spec(plugin_dir: Union[str, Path]) -> Dict[str, Any]: From d9f6b110bc8ce9615258adf691da36df1b5ff319 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Sat, 5 Sep 2026 17:25:24 -0400 Subject: [PATCH 07/11] refactor: narrow the exception handlers this branch introduced Codacy flagged the new code; it passes on other recent PRs, so the finding is mine. Four of the five broad `except Exception` clauses I added were catching far more than they needed to, which is the same shape as several bugs this branch fixes -- hello-world's TypeError sat invisible for exactly this reason. freezer() / move_to() / tick() -> (AttributeError, TypeError, ValueError) cache_manager.delete() -> (OSError, AttributeError, KeyError) The fifth stays broad and now says why: it wraps a call into a plugin's own display(), which can raise anything, and the first frame has already rendered -- so a failure there must not turn a good result into an error. Verified against a checkout of main: f1-scoreboard 48 PASS / 0 FAIL with 0 empty warnings, on-air keeps its 8 true positives, clock-simple 8 PASS. geochron shows 7 golden drifts both before and after this branch, so it is not from these changes -- its committed goldens predate #521's 1-bit text rendering. --- src/display_controller.py | 4 +++- src/plugin_system/testing/harness.py | 16 ++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/display_controller.py b/src/display_controller.py index 72100568..5acb7b11 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -1333,7 +1333,9 @@ def _poll_on_demand_requests(self) -> None: # against double-processing if this delete fails. try: self.cache_manager.delete('display_on_demand_request') - except Exception as err: # pragma: no cover - best-effort cleanup + except (OSError, AttributeError, KeyError) as err: + # Best-effort: processed_id still guards against reprocessing if the + # mailbox cannot be cleared. logger.debug("Could not clear the on-demand request mailbox: %s", err) if action == 'start': diff --git a/src/plugin_system/testing/harness.py b/src/plugin_system/testing/harness.py index d46c11c7..628e13ce 100644 --- a/src/plugin_system/testing/harness.py +++ b/src/plugin_system/testing/harness.py @@ -222,7 +222,9 @@ def _settle_empty_frame(inst, mode, dm, result, freezer) -> None: if freezer is not None: try: resume_at = freezer() - except Exception: # noqa: BLE001 - only to restore, never load-bearing + except (AttributeError, TypeError, ValueError): + # Not a freezegun factory, or a version whose factory is not + # callable. Only used to restore the clock, never load-bearing. resume_at = None try: _settle_loop(inst, mode, dm, result, freezer) @@ -230,7 +232,7 @@ def _settle_empty_frame(inst, mode, dm, result, freezer) -> None: if resume_at is not None: try: freezer.move_to(resume_at) - except Exception: # noqa: BLE001 + except (AttributeError, TypeError, ValueError): pass @@ -242,7 +244,9 @@ def _settle_loop(inst, mode, dm, result, freezer) -> None: # number only since 1.x, and a stale pin would raise here. try: tick(timedelta(seconds=EMPTY_RECHECK_STEP)) - except Exception: # noqa: BLE001 - pacing is best-effort + except (AttributeError, TypeError, ValueError): + # Pacing is best-effort; a freezegun that will not take a + # timedelta just means this probe runs without advancing time. pass else: # No frozen clock, so the real one has to do the advancing. Without @@ -252,7 +256,11 @@ def _settle_loop(inst, mode, dm, result, freezer) -> None: time.sleep(EMPTY_RECHECK_STEP) try: result.display_returned = _render_mode_again(inst, mode) - except Exception: # noqa: BLE001 - the first frame already succeeded + except Exception: # noqa: BLE001 + # Deliberately broad: this calls a plugin's display(), which can + # raise anything. The first frame already rendered, so whatever + # happens on a re-draw must not turn a good result into an error -- + # keep the frame we have and stop probing. return image = dm.get_image() if _has_content(image): From 80781dda46029094ce8f3509c9431f07eed7d7d5 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Sun, 6 Sep 2026 18:36:44 -0400 Subject: [PATCH 08/11] fix: resolve CodeRabbit review and Codacy findings on #534 CodeRabbit raised six; all six were real. The test double had drifted ahead of production. VisualTestDisplayManager accepted set_scrolling_state(frame_hold=...) while DisplayManager did not, so such a call passed every harness run and would raise TypeError on the panel -- the one failure a safety harness exists to prevent. frame_hold belongs to the change that adds it to DisplayManager (#523), so it moves there and the double matches main again. The harness swallowed exceptions from re-rendered frames. _settle_loop re-renders a mode that came back blank, to give a scroll time to draw; returning silently on a crash meant a mode that renders one good frame and then explodes was reported as passing. Recorded on result.error now, keeping the captured frame so the failure stays inspectable. starlark-apps display() returned True after _display_frame() failed, so the controller held a dead frame for the whole display_duration instead of rotating on. _display_frame now returns bool on all three paths. run_plugin_tests.py used env.setdefault for PYTHONPATH and LEDMATRIX_CORE, so an inherited value won and the subprocess imported a different core than the one under test -- ledmatrix-plugins#467 exactly. Prepends PROJECT_ROOT and sets LEDMATRIX_CORE unconditionally. The on-demand mailbox is polled after every frame, ~125x/second on a scrolling mode, and the read is deliberately uncached, so it was that many disk reads per second to find nothing. Floored at 250ms, which is imperceptible for a web-UI click. Consuming it also deleted whatever was present rather than what had just been processed, so a request posted while the previous one was in flight was thrown away and never ran; the delete is now keyed by request_id. That narrows the window rather than closing it -- a true atomic claim needs a primitive the cache layer does not offer, and the code says so rather than implying otherwise. Codacy's 2 criticals were bandit B404/B603 on the subprocess call added to run_plugin_tests.py. Fixed interpreter, argument list, no shell; annotated with the repo's existing nosec convention. Bandit is clean on the file. Adds test/test_on_demand_mailbox.py (8), test_starlark_display_contract.py (4) and two settle cases in test_harness_empty_claimed.py. 4, 4 and 2 of those fail against the pre-fix code. Full suite: 3961 passed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --- plugin-repos/starlark-apps/manager.py | 20 +++- scripts/run_plugin_tests.py | 19 +++- src/display_controller.py | 39 ++++++- src/plugin_system/testing/harness.py | 11 +- .../testing/visual_display_manager.py | 13 ++- test/test_harness_empty_claimed.py | 58 ++++++++++ test/test_on_demand_mailbox.py | 106 ++++++++++++++++++ test/test_starlark_display_contract.py | 82 ++++++++++++++ 8 files changed, 327 insertions(+), 21 deletions(-) create mode 100644 test/test_on_demand_mailbox.py create mode 100644 test/test_starlark_display_contract.py diff --git a/plugin-repos/starlark-apps/manager.py b/plugin-repos/starlark-apps/manager.py index 86ad36b6..fad2dc7d 100644 --- a/plugin-repos/starlark-apps/manager.py +++ b/plugin-repos/starlark-apps/manager.py @@ -714,9 +714,11 @@ def display(self, force_clear: bool = False) -> bool: self.logger.error(f"Failed to render app: {self.current_app.app_id}") return False - # Display current frame - self._display_frame() - return True + # Display current frame. The result is propagated: a failed frame + # update is not a displayed frame, and returning True regardless + # told the controller the mode had rendered, so it held the dead + # frame for the whole display_duration instead of rotating on. + return self._display_frame() except Exception as e: self.logger.error(f"Error displaying Starlark app: {e}") @@ -843,10 +845,13 @@ def _load_frames_from_cache(self, app: StarlarkApp) -> bool: self.logger.error(f"Error loading frames for {app.app_id}: {e}") return False - def _display_frame(self) -> None: - """Display the current frame of the current app.""" + def _display_frame(self) -> bool: + """Display the current frame of the current app. + + :returns: whether a frame actually reached the display manager. + """ if not self.current_app or not self.current_app.frames: - return + return False try: current_time = time.time() @@ -864,8 +869,11 @@ def _display_frame(self) -> None: ) self.current_app.last_frame_time = current_time + return True + except Exception as e: self.logger.error(f"Error displaying frame: {e}") + return False def install_app(self, app_id: str, star_file_path: str, metadata: Optional[Dict[str, Any]] = None, assets_dir: Optional[str] = None) -> bool: """ diff --git a/scripts/run_plugin_tests.py b/scripts/run_plugin_tests.py index baaf1075..47b9b851 100755 --- a/scripts/run_plugin_tests.py +++ b/scripts/run_plugin_tests.py @@ -8,7 +8,7 @@ import os import re -import subprocess +import subprocess # nosec B404 - runs repo-local test files, never a shell import sys import argparse from pathlib import Path @@ -97,14 +97,25 @@ def run_script_tests(test_files: list, verbose: bool = False) -> int: a script that needs a tty or an LED matrix is not a regression. """ env = dict(os.environ) - env.setdefault("PYTHONPATH", str(PROJECT_ROOT)) - env.setdefault("LEDMATRIX_CORE", str(PROJECT_ROOT)) + # Prepend rather than setdefault. An inherited PYTHONPATH -- a developer's + # shell, a tox run, another checkout -- otherwise wins outright, and the + # subprocess imports a different copy of the core than the one under test. + # That is exactly the failure ledmatrix-plugins#467 describes, and it is + # invisible: the tests pass or fail against a tree nobody meant to test. + inherited = env.get("PYTHONPATH") + env["PYTHONPATH"] = (f"{PROJECT_ROOT}{os.pathsep}{inherited}" + if inherited else str(PROJECT_ROOT)) + env["LEDMATRIX_CORE"] = str(PROJECT_ROOT) passed = skipped = failed = 0 failures = [] for path in test_files: try: - proc = subprocess.run([sys.executable, str(path)], cwd=str(Path(path).parent), + # nosec B603 - fixed interpreter (sys.executable) plus a test path + # this script discovered by globbing the repo; argument list, no + # shell, so nothing is word-split or expanded. + proc = subprocess.run([sys.executable, str(path)], # nosec B603 + cwd=str(Path(path).parent), capture_output=True, text=True, env=env, stdin=subprocess.DEVNULL, timeout=300) rc = proc.returncode diff --git a/src/display_controller.py b/src/display_controller.py index 5acb7b11..0e56580d 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -198,6 +198,10 @@ def _follower_gated_update(): # the main run loop reconciles (loads/unloads) on its own thread so # mutating available_modes never races with rendering. self._pending_plugin_reconcile = False + # Monotonic stamp of the last mailbox disk read; see + # _poll_on_demand_requests. None means "never polled", so the first + # call always goes through. + self._last_on_demand_poll: Optional[float] = None self.on_demand_active = False self.on_demand_mode: Optional[str] = None self.on_demand_modes: List[str] = [] # All modes for the on-demand plugin @@ -1267,8 +1271,22 @@ def _set_on_demand_error(self, message: str) -> None: self.on_demand_schedule_override = False self._publish_on_demand_state() + #: Shortest gap between mailbox disk reads. This is called after every + #: frame -- about 125 times a second on a scrolling mode -- and the read + #: below is deliberately uncached, so without a floor it was 125 disk reads + #: per second to find nothing. An on-demand request comes from a person + #: clicking in the web UI, so a quarter second of latency is not + #: perceptible, and it cuts the read rate by 30x. + ON_DEMAND_POLL_INTERVAL = 0.25 + def _poll_on_demand_requests(self) -> None: """Poll cache for new on-demand requests from external controllers.""" + now = time.monotonic() + if (self._last_on_demand_poll is not None + and now - self._last_on_demand_poll < self.ON_DEMAND_POLL_INTERVAL): + return + self._last_on_demand_poll = now + try: # Use a long max_age (1 hour) to ensure requests aren't expired before processing # The request_id check prevents duplicate processing. @@ -1332,7 +1350,26 @@ def _poll_on_demand_requests(self) -> None: # panel silently showed the earlier plugin. processed_id still guards # against double-processing if this delete fails. try: - self.cache_manager.delete('display_on_demand_request') + # Compare before deleting. The web process can post a newer request + # between the read above and this delete; an unconditional delete + # threw that one away and it was never processed -- the user's + # second click did nothing. Re-reading uncached and only deleting + # our own request_id means a newer request is left in the mailbox + # for the next poll instead. + # + # This narrows the window rather than closing it: a request landing + # between this re-read and the delete is still lost. Closing it + # properly needs an atomic claim (a rename, or a compare-and-delete + # primitive) that the cache layer does not currently offer, so the + # honest fix is a smaller window plus this note, not a bigger lock. + current = self.cache_manager.get('display_on_demand_request', + max_age=3600, memory_ttl=0) + if not current or current.get('request_id') == request_id: + self.cache_manager.delete('display_on_demand_request') + else: + logger.debug("Newer on-demand request %s arrived while processing " + "%s; leaving it in the mailbox", + current.get('request_id'), request_id) except (OSError, AttributeError, KeyError) as err: # Best-effort: processed_id still guards against reprocessing if the # mailbox cannot be cleared. diff --git a/src/plugin_system/testing/harness.py b/src/plugin_system/testing/harness.py index 628e13ce..267774c2 100644 --- a/src/plugin_system/testing/harness.py +++ b/src/plugin_system/testing/harness.py @@ -256,11 +256,14 @@ def _settle_loop(inst, mode, dm, result, freezer) -> None: time.sleep(EMPTY_RECHECK_STEP) try: result.display_returned = _render_mode_again(inst, mode) - except Exception: # noqa: BLE001 + except Exception as e: # noqa: BLE001 # Deliberately broad: this calls a plugin's display(), which can - # raise anything. The first frame already rendered, so whatever - # happens on a re-draw must not turn a good result into an error -- - # keep the frame we have and stop probing. + # raise anything. Recorded rather than swallowed -- a mode that + # renders one good frame and then crashes on the next is broken, + # and returning silently here reported it as passing. The frame + # already captured stays on the result so the failure is still + # inspectable. + result.error = repr(e) return image = dm.get_image() if _has_content(image): diff --git a/src/plugin_system/testing/visual_display_manager.py b/src/plugin_system/testing/visual_display_manager.py index 732d6d61..82cab907 100644 --- a/src/plugin_system/testing/visual_display_manager.py +++ b/src/plugin_system/testing/visual_display_manager.py @@ -506,16 +506,17 @@ def draw_text_with_icons(self, text: str, icons: List[tuple] = None, # Scrolling state (no-op interface compat) # ------------------------------------------------------------------ - def set_scrolling_state(self, is_scrolling: bool, frame_hold: int = 1): + def set_scrolling_state(self, is_scrolling: bool): """Set the current scrolling state (no-op for testing). - ``frame_hold`` mirrors DisplayManager.set_scrolling_state so a plugin - that paces its scroll can be rendered here. Without it every such - plugin raised TypeError at render time and failed every size, which is - invisible until a plugin happens to pass the argument. + Deliberately mirrors DisplayManager.set_scrolling_state exactly. A + double that accepts arguments production does not lets a call pass + every harness run and then raise TypeError on the panel, which is the + one failure a safety harness exists to prevent. ``frame_hold`` arrives + here in the same change that adds it to DisplayManager (#523), not + before. """ self._scrolling_state['is_scrolling'] = is_scrolling - self._scrolling_state['frame_hold'] = frame_hold if is_scrolling: self._scrolling_state['last_scroll_activity'] = time.time() diff --git a/test/test_harness_empty_claimed.py b/test/test_harness_empty_claimed.py index 2791516a..97e6b074 100644 --- a/test/test_harness_empty_claimed.py +++ b/test/test_harness_empty_claimed.py @@ -101,3 +101,61 @@ def test_a_result_with_no_image_is_left_alone(self): r = _result(None, returned=None) check_empty_claimed([r], strict=True) assert r.empty_claimed is None + + +class TestSettleRecordsLaterFailures: + """A mode that renders one good frame and then crashes is broken. + + _settle_loop re-renders a mode that came back blank, to give a scroll or an + animation time to put something on the panel. Swallowing an exception from + those later frames meant the harness reported a passing result for a mode + that crashes as soon as it is asked for a second frame -- exactly the kind + of defect the harness exists to catch. + """ + + class Boom: + """Renders once, then raises.""" + + def __init__(self): + self.calls = 0 + + def display(self, force_clear=False): + self.calls += 1 + raise RuntimeError("second frame exploded") + + def _settle(self, inst, dm, result): + from src.plugin_system.testing import harness + harness._settle_loop(inst, "mode", dm, result, None) + + def test_the_exception_is_recorded_on_the_result(self, monkeypatch): + from src.plugin_system.testing import harness + # Keep the probe short; this test is about the error, not the pacing. + monkeypatch.setattr(harness, "EMPTY_RECHECK_FRAMES", 1) + monkeypatch.setattr(harness, "EMPTY_RECHECK_STEP", 0) + + result = _result(_blank()) + assert result.error is None + self._settle(self.Boom(), _FakeDM(), result) + + assert result.error is not None, "a crash on a later frame was swallowed" + assert "second frame exploded" in result.error + + def test_the_already_captured_frame_is_kept(self, monkeypatch): + from src.plugin_system.testing import harness + monkeypatch.setattr(harness, "EMPTY_RECHECK_FRAMES", 1) + monkeypatch.setattr(harness, "EMPTY_RECHECK_STEP", 0) + + image = _blank() + result = _result(image) + self._settle(self.Boom(), _FakeDM(), result) + assert result.image is image, "the good frame was discarded along with the error" + + +class _FakeDM: + """Minimal display-manager double for _settle_loop.""" + + def get_image(self): + return _blank() + + def check_overflow(self): + return None diff --git a/test/test_on_demand_mailbox.py b/test/test_on_demand_mailbox.py new file mode 100644 index 00000000..dceca2d4 --- /dev/null +++ b/test/test_on_demand_mailbox.py @@ -0,0 +1,106 @@ +"""The on-demand request mailbox: how often it is read, and how it is consumed. + +The mailbox is a cache key the web process writes and the display process +reads. Two properties matter and neither is obvious from the call site: + + * it is polled after every rendered frame, so an uncached read here is a + disk read at frame rate; + * consuming it must not throw away a request that arrived while the previous + one was being processed. +""" + +from unittest.mock import MagicMock + +import pytest + + +class TestPollingIsBounded: + """_poll_on_demand_requests runs ~125x/second on a scrolling mode. + + The read is deliberately uncached (memory_ttl=0) because a cached one + pinned the first request for an hour. That makes the call a real disk read, + so it needs a floor -- without one it was ~125 reads per second to find + nothing at all. + """ + + def test_first_call_always_reads(self, test_display_controller): + c = test_display_controller + c.cache_manager.get = MagicMock(return_value=None) + c._poll_on_demand_requests() + assert c.cache_manager.get.call_count == 1 + + def test_immediate_second_call_does_not_read(self, test_display_controller): + c = test_display_controller + c.cache_manager.get = MagicMock(return_value=None) + c._poll_on_demand_requests() + for _ in range(50): + c._poll_on_demand_requests() + assert c.cache_manager.get.call_count == 1, "polling was not bounded" + + def test_reads_again_once_the_interval_has_passed(self, test_display_controller, monkeypatch): + c = test_display_controller + c.cache_manager.get = MagicMock(return_value=None) + clock = {"t": 1000.0} + monkeypatch.setattr("src.display_controller.time.monotonic", lambda: clock["t"]) + + c._poll_on_demand_requests() + clock["t"] += c.ON_DEMAND_POLL_INTERVAL / 2 + c._poll_on_demand_requests() + assert c.cache_manager.get.call_count == 1, "read before the interval elapsed" + + clock["t"] += c.ON_DEMAND_POLL_INTERVAL + c._poll_on_demand_requests() + assert c.cache_manager.get.call_count == 2 + + def test_the_interval_is_short_enough_to_feel_instant(self, test_display_controller): + # A person clicking in the web UI must not notice the floor. + assert test_display_controller.ON_DEMAND_POLL_INTERVAL <= 0.5 + + +class TestMailboxIsConsumedByIdentity: + """Deleting whatever is in the mailbox loses a request that raced in.""" + + def _arrange(self, controller, first, later): + """Mailbox returns `first`, then `later` on the pre-delete re-read.""" + controller.on_demand_active = False + controller.on_demand_request_id = None + controller._last_on_demand_poll = None + reads = iter([first, later]) + + def fake_get(key, *a, **kw): + if key == 'display_on_demand_request': + return next(reads, later) + return None # processed-id lookup + + controller.cache_manager.get = MagicMock(side_effect=fake_get) + controller.cache_manager.set = MagicMock() + controller.cache_manager.delete = MagicMock() + controller._activate_on_demand = MagicMock() + + REQ_A = {'request_id': 'A', 'action': 'start', 'plugin_id': 'p', 'mode': 'm'} + REQ_B = {'request_id': 'B', 'action': 'start', 'plugin_id': 'p', 'mode': 'm'} + + def test_own_request_is_deleted(self, test_display_controller): + c = test_display_controller + self._arrange(c, self.REQ_A, self.REQ_A) + c._poll_on_demand_requests() + c.cache_manager.delete.assert_called_once_with('display_on_demand_request') + + def test_a_newer_request_is_left_for_the_next_poll(self, test_display_controller): + c = test_display_controller + self._arrange(c, self.REQ_A, self.REQ_B) + c._poll_on_demand_requests() + assert c.cache_manager.delete.call_count == 0, \ + "request B was deleted without ever being processed" + + def test_an_already_empty_mailbox_is_still_cleared(self, test_display_controller): + c = test_display_controller + self._arrange(c, self.REQ_A, None) + c._poll_on_demand_requests() + c.cache_manager.delete.assert_called_once_with('display_on_demand_request') + + def test_the_request_is_still_processed(self, test_display_controller): + c = test_display_controller + self._arrange(c, self.REQ_A, self.REQ_B) + c._poll_on_demand_requests() + c._activate_on_demand.assert_called_once() diff --git a/test/test_starlark_display_contract.py b/test/test_starlark_display_contract.py new file mode 100644 index 00000000..1804d7bf --- /dev/null +++ b/test/test_starlark_display_contract.py @@ -0,0 +1,82 @@ +"""starlark-apps: display() must report what actually reached the panel. + +The display controller skips a mode only on a boolean False. Returning True +after the frame update failed told it the mode had rendered, so it held a dead +frame for the whole display_duration instead of rotating on -- the same class +of defect as a display() that returns None. +""" + +import sys +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +PLUGIN_DIR = Path(__file__).resolve().parent.parent / "plugin-repos" / "starlark-apps" + + +@pytest.fixture(scope="module") +def manager_module(): + if not PLUGIN_DIR.exists(): + pytest.skip("starlark-apps plugin is not checked out") + sys.path.insert(0, str(PLUGIN_DIR)) + try: + import importlib + spec = importlib.util.spec_from_file_location( + "starlark_manager_under_test", PLUGIN_DIR / "manager.py") + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + except Exception as e: # noqa: BLE001 - optional deps (pixlet, fcntl) may be absent + pytest.skip(f"starlark-apps manager is not importable here: {e}") + finally: + sys.path.remove(str(PLUGIN_DIR)) + + +def _plugin(manager_module): + """A manager with __init__ bypassed -- only display paths are under test.""" + cls = manager_module.StarlarkAppsPlugin + inst = cls.__new__(cls) + inst.logger = MagicMock() + inst.display_manager = MagicMock() + inst.current_app = None + return inst + + +class _App: + def __init__(self, frames): + self.frames = frames + self.current_frame_index = 0 + self.last_frame_time = 0.0 + self.app_id = "app" + + +class TestDisplayFramePropagates: + def test_a_failed_update_returns_false(self, manager_module): + p = _plugin(manager_module) + p.current_app = _App([("frame", 100)]) + p.display_manager.update_display.side_effect = RuntimeError("panel gone") + + assert p._display_frame() is False + + def test_a_good_update_returns_true(self, manager_module): + p = _plugin(manager_module) + p.current_app = _App([("frame", 100)]) + + assert p._display_frame() is True + + def test_no_frames_returns_false(self, manager_module): + p = _plugin(manager_module) + p.current_app = _App([]) + + assert p._display_frame() is False + + def test_display_reports_the_frame_failure(self, manager_module): + p = _plugin(manager_module) + p.current_app = _App([("frame", 100)]) + p.display_manager.update_display.side_effect = RuntimeError("panel gone") + + result = p.display() + + assert result is False, "display() claimed success over a failed frame update" + assert isinstance(result, bool), "the controller only skips on a real bool" From b3d9af4bcdadf774bd4f60818ee4d2d6cfdc786e Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Sun, 6 Sep 2026 20:41:13 -0400 Subject: [PATCH 09/11] chore: satisfy Codacy's subprocess checks on the new test runner Codacy runs Bandit and Opengrep (its Semgrep fork). The new subprocess.run in scripts/run_plugin_tests.py trips three patterns, on two different lines: Bandit B404 on the import, B603 on the call Opengrep dangerous-subprocess-use-audit on the run( line dangerous-subprocess-use-tainted-env-args on the argv line A nosemgrep applies only to its own line, so the call line and the argv line each need one; a single comment on the call covered neither rule fully. Suppression is the right answer here rather than a rewrite: the interpreter is sys.executable, the arguments are a list, and no shell is involved, so there is nothing to word-split or expand. Matches the pair the rest of the repo already uses for this shape -- permission_utils.py, plugin_loader.py, install_dependencies_apt.py. Codacy: 0 new issues, up to standards. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --- scripts/run_plugin_tests.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/scripts/run_plugin_tests.py b/scripts/run_plugin_tests.py index 47b9b851..a8f7f072 100755 --- a/scripts/run_plugin_tests.py +++ b/scripts/run_plugin_tests.py @@ -8,7 +8,7 @@ import os import re -import subprocess # nosec B404 - runs repo-local test files, never a shell +import subprocess # nosec B404 - list-form argv only, no shell # nosemgrep import sys import argparse from pathlib import Path @@ -111,13 +111,16 @@ def run_script_tests(test_files: list, verbose: bool = False) -> int: failures = [] for path in test_files: try: - # nosec B603 - fixed interpreter (sys.executable) plus a test path - # this script discovered by globbing the repo; argument list, no - # shell, so nothing is word-split or expanded. - proc = subprocess.run([sys.executable, str(path)], # nosec B603 - cwd=str(Path(path).parent), - capture_output=True, text=True, env=env, - stdin=subprocess.DEVNULL, timeout=300) + # Fixed interpreter (sys.executable) plus a test path this script + # discovered by globbing the repo; argument list, no shell, so + # nothing is word-split or expanded. Same suppression pair the + # rest of the repo uses for this shape (see permission_utils.py). + proc = subprocess.run( # nosec B603 - no shell invoked (list-form argv) # nosemgrep + [sys.executable, str(path)], # nosemgrep + cwd=str(Path(path).parent), + capture_output=True, text=True, env=env, + stdin=subprocess.DEVNULL, timeout=300, + ) rc = proc.returncode tail = " | ".join((proc.stdout or proc.stderr or "").strip().splitlines()[-2:])[:200] except subprocess.TimeoutExpired: From 25d3f35b5580bbe13a9e39fb1d87609a350f0d48 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Mon, 7 Sep 2026 09:56:37 -0400 Subject: [PATCH 10/11] chore: leave visual_display_manager untouched so #523 can merge The only change this branch made to that file was a docstring, and it collided with #523's rewrite of the same method -- so #534 and #523 each merged cleanly against main but conflicted with each other. Reverted to main's text; #523 owns this method and adds frame_hold to it. The note the docstring carried ('frame_hold arrives in #523') would have been stale the moment #523 landed anyway. The parity test in #523 is what actually keeps the two signatures honest. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --- src/plugin_system/testing/visual_display_manager.py | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/plugin_system/testing/visual_display_manager.py b/src/plugin_system/testing/visual_display_manager.py index 82cab907..e33d2309 100644 --- a/src/plugin_system/testing/visual_display_manager.py +++ b/src/plugin_system/testing/visual_display_manager.py @@ -507,15 +507,7 @@ def draw_text_with_icons(self, text: str, icons: List[tuple] = None, # ------------------------------------------------------------------ def set_scrolling_state(self, is_scrolling: bool): - """Set the current scrolling state (no-op for testing). - - Deliberately mirrors DisplayManager.set_scrolling_state exactly. A - double that accepts arguments production does not lets a call pass - every harness run and then raise TypeError on the panel, which is the - one failure a safety harness exists to prevent. ``frame_hold`` arrives - here in the same change that adds it to DisplayManager (#523), not - before. - """ + """Set the current scrolling state (no-op for testing).""" self._scrolling_state['is_scrolling'] = is_scrolling if is_scrolling: self._scrolling_state['last_scroll_activity'] = time.time() From 4a64a37b01d6619496e4608300499ffef11984a8 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Mon, 7 Sep 2026 13:35:18 -0400 Subject: [PATCH 11/11] chore: add the Ruff suppression nosec/nosemgrep do not cover Ruff reports S603 on the same call Bandit and Opengrep do, and none of the three suppressions covers the others. Confirmed the precondition first: path comes from discover_plugin_tests(), which globs test files inside the repo, and the call is a fixed interpreter with a list argv and no shell. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --- scripts/run_plugin_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/run_plugin_tests.py b/scripts/run_plugin_tests.py index a8f7f072..fcaa8482 100755 --- a/scripts/run_plugin_tests.py +++ b/scripts/run_plugin_tests.py @@ -115,7 +115,7 @@ def run_script_tests(test_files: list, verbose: bool = False) -> int: # discovered by globbing the repo; argument list, no shell, so # nothing is word-split or expanded. Same suppression pair the # rest of the repo uses for this shape (see permission_utils.py). - proc = subprocess.run( # nosec B603 - no shell invoked (list-form argv) # nosemgrep + proc = subprocess.run( # noqa: S603 # nosec B603 - no shell invoked (list-form argv) # nosemgrep [sys.executable, str(path)], # nosemgrep cwd=str(Path(path).parent), capture_output=True, text=True, env=env,