mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-04 14:25:08 +00:00
perf/slim-espn-windows
130
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 ChuckandClaude Opus 5
|
968b953a51 |
fix(display): pin one text layout engine, and give the 5x7 BDF face a size (#539)
* fix(display): pin one text layout engine, and give the 5x7 face a size Two ways a font could render differently on two machines running the same code, both found while diagnosing four plugins whose golden images passed on the machine that generated them and failed everywhere else. **Layout engine.** `ImageFont.truetype` picks its engine at load time: Raqm where the host Pillow was built with libraqm, Basic otherwise. The two round fractional glyph advances differently. `PressStart2P-Regular.ttf` at 8px has whole-pixel advances, so they agree — which is why most of the fleet matched everywhere and hid this. `4x6-font.ttf` at 6px does not: glyph positions drift cumulatively along a run, and the four plugins that draw body text in it (geochron, of-the-day, christmas-countdown, ledmatrix-weather's almanac) are exactly the four whose goldens travelled badly. Every core font load now goes through `src/common/font_layout.load_truetype`, which pins the Basic engine, so a render depends on the font file and the size and nothing else. Basic gives up complex-script shaping and kerning pairs; neither applies to bitmap-grid faces on an LED panel. Output is unchanged on a host without libraqm. **Zero font height.** `DisplayManager` built the 5x7 BDF face with `freetype.Face(path)` and never called `set_char_size`, so `face.size.height` stayed 0 and `get_font_height()` returned 0 for it — callers stacking rows by `prev_y + prev_height + gap` drew two lines on top of each other. The start-up line `Calendar font size: 0 pixels` has been printing the symptom all along. `font_manager._load_bdf_font` already called `set_char_size`, so whether measurement worked depended on which path loaded the face. `DisplayManager` now sets it too, and `get_font_height()` falls back to the strike the file declares rather than returning a zero line height. Fixes ChuckBuilds/ledmatrix-plugins#397 Refs ChuckBuilds/ledmatrix-plugins#371, #375, #378, #391 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(display): give the startup banner a rung that fits a full address at 64px CI caught what pinning the layout engine exposed rather than caused. `_fitting_font` walks PressStart2P then 4x6 at 6px, and "255.255.255.255" -- the widest thing the startup banner ever shows -- measures 66px at 4x6/6px against the 62 a 64x32 panel has to give. It used to squeak in only because the measurement depended on which layout engine the host Pillow happened to have; with the engine pinned it does not, so the rung the worst case actually needs is now in the ladder instead of implied: 4x6 at 5px, which measures 51. The fallback was wrong in the same place. When nothing in the ladder fit, it returned `self.font` -- the *widest* option, and precisely how "Initializing" came to run off the side of a 64px panel to begin with. It returns the narrowest face that loaded now. test/test_initializing_screen.py: 34 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(display): name the exceptions the BDF strike read can raise Codacy flagged the try/except/pass. It was already narrow in intent -- a malformed strike table on the measurement path must degrade to "size unknown" rather than take the display down -- but a bare `except Exception: pass` says neither of those things and hides a genuinely broken font behind a silent 8px fallback. It now catches what reading `available_sizes` can actually raise and logs which face failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore: drop logo PNGs the render harness downloaded into the worktree These are fetched at runtime by the logo cache; they are not source, and they rode in on a `git add -A` while I was running check_plugin.py against this branch. Nothing in the change needs them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
c9289e3a1d |
fix(store): update_plugin silently did nothing for four installed plugins (#536)
install_plugin() deliberately renames a plugin's directory to the MANIFEST id
when it differs from the REGISTRY id, so registry `stocks` lands in
`ledmatrix-stocks/`. Every lookup in _find_plugin_path() is by directory name,
so update_plugin("stocks") found nothing, logged "Plugin not installed", and
returned False.
Nothing surfaced that to the user. Clicking update in the web UI was a no-op
with no error, and the plugin stayed on a stale version indefinitely. Four
installed plugins hit this on a real device -- leaderboard, music, stocks and
weather -- found because a scripted update of eleven plugins failed on exactly
those four.
Adds a manifest-id scan as the LAST step of the resolution chain, so the two
documented lookups above it (configured dir, then the sibling plugins/
fallback) keep their exact meaning and ordering. That ordering is pinned by
test_discovery_path_contract.py, which characterises the divergence between
the three resolvers on purpose; this extends the chain rather than reordering
it. Directories renamed aside with '.standalone-backup-' during an install or
rollback are skipped, since matching one would report a half-finished install
as a live plugin.
Also adds scripts/audit_render_path.py, which walks the call graph from
display() and reports blocking calls reachable from it. display() runs on the
render thread, so anything slow there stalls the panel; on a vsync-paced loop
a single 15ms call drops a frame and a network round trip freezes the marquee.
Two instances were already found the slow way, by reading frame-time
histograms -- odds-ticker reading the scoreboard cache per frame, and
soccer-scoreboard timing out inside update(). The audit finds that shape in
the source instead. It is a heuristic and says so: a hit behind an interval
check may be fine.
It currently flags 23 calls across six plugins. The clearest is
ledmatrix-music, whose display() falls back to an inline
requests.get(timeout=5) when album art has not been prefetched -- a deliberate
"show the art rather than go blank" tradeoff by its author, but up to five
seconds of frozen panel. Reported, not changed; that is its owner's call.
185 store tests pass. Three of the six new tests fail without the fix.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
ChuckandClaude Opus 5
|
d12323e7f1 |
perf(scroll): pace frames to the panel — 44→100 fps, stalls 14% → 0.02% (#523)
* perf(scroll): pace frames to the panel, not to a fixed sleep Scrolling ran at 44-46 fps on a 2x128x64 chain and 14-17% of frames took 41-53ms, which reads as judder. Four independent causes, each measured on the hardware; details and the diagnostic recipe are in docs/SCROLL_PERFORMANCE.md. The high-FPS loop slept a flat 8ms after every render. display() has already blocked on the panel's vsync by then, so that sleep was added to a wait that had happened: ~4ms of render plus 8ms put each iteration at ~12ms against a 10ms refresh grid, so every swap missed a refresh and the loop settled at 50fps while asking for 125 -- with no headroom, so a further 14% of frames slipped again. It now sleeps only the remainder, with a 1ms floor so plugin threads still get the GIL. ScrollHelper stepped position on a wall clock at 1/scroll_delay steps per second. Plugins set scroll_delay to the frame period, so that comparison sat exactly on its own threshold: a frame arriving a hair early moved zero pixels and rendered an identical frame, dirty-tracking skipped the swap, it returned in ~2ms, and the beat repeated. No scroll_delay value tunes that out -- a shorter delay trades stalled frames for periodic double-steps. Both modes now accumulate elapsed time at the same configured speed, so position stays proportional to real time. Sub-pixel blending goes back to off by default. It renders a half-step by mixing two adjacent columns, which on a coarse panel showing pixel-font text alternates crisp and smeared frames and reads as shimmer -- visibly worse than integer stepping on the hardware. Vegas mode still opts in. disk_cache uses orjson when importable, falling back to the stdlib. Encoding a ~1MB record drops from 14.8ms to 5.4ms end-to-end, and that work holds the GIL while a marquee is on screen. display_manager also checksummed the whole framebuffer twice per frame (dirty tracking, then the preview snapshot); the snapshot now takes the checksum the caller already computed. New src/common/scroll_config.py resolves scroll settings in one place. Five ticker plugins each hand-rolled this and disagreed: odds-ticker ranked the deprecated scroll_pixels_per_second above the documented scroll_speed/delay pair, and because that key carries a schema default the documented settings were dead for every user (ChuckBuilds/ledmatrix-plugins#408), while ledmatrix-leaderboard read the same key only as a fallback. The resolver also warns when a speed will not advance a whole number of pixels per refresh, which is the property that actually determines whether a scroll looks smooth. scripts/build_rgbmatrix_nogil.sh rebuilds the rgbmatrix binding so it releases the GIL. Upstream declares SwapOnVSync without nogil, unlike SetPixel/Clear/Fill beside it, so the render thread held the GIL for the whole vsync wait and starved background threads into long uninterruptible bursts. The script patches, builds and self-verifies into a scratch tree; --install backs up the original and rolls back if the service does not come back healthy. Measured after: 100 fps locked, no stalls observed, render thread down from 51% to 19% of one core. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(display): keep the panel swap locked to vsync while scrolling Dirty tracking skipped SwapOnVSync for byte-identical frames. That is the right call for static content, but SwapOnVSync is also what paces the render loop, so skipping it skips the wait for the panel: a duplicate frame returns in ~8ms instead of ~10ms on a 100Hz panel, advances the strip only 0.8px instead of 1.0px, and so makes the next frame more likely to repeat as well. The effect sustains itself once it starts. Measured over 20 minutes on a 2x128x64 chain, both scrollers configured identically at 100 px/s: leaderboard 10ms x35, 11ms x3 (clean) odds-ticker 10ms x26, 8ms x7, 15ms x5 (~20% duplicates mid-scroll) The duplicates were not end-of-cycle idling -- 38% of fast frames fell within 90s of a scroll completion against 35% of normal frames, a null result. The trigger is per-frame work: odds does more of it, and more variably, so it is first to land a frame that advances less than a whole pixel. Pushing an identical frame costs one canvas copy. Falling out of vsync lock costs smooth motion. Static content is untouched, because is_currently_scrolling() expires on its own inactivity threshold -- covered by test_stale_scrolling_state_stops_forcing_pushes so a plugin that stops scrolling without saying so cannot pin the panel into always-push. Also de-flakes test_snapshot_still_written_on_skip, which asserted a strict mtime increase between two writes that can land in the same filesystem tick; it failed about two runs in three on Windows regardless of the code under test. The file is now backdated before the check. 156 tests pass on the Pi. Not yet confirmed by eye on the panel. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scroll): report the frame-time tail, and stop the row-major blit Two problems, both found by looking at the panel rather than the metric. The frame-stats line reported ONE instantaneous frame every 5 seconds -- about 1 frame in 500 -- printed beside a 100-frame average. Both hide exactly the fault they are used to chase: a 2ms duplicate and a 21ms double-wait average to precisely 10ms, so a ticker stalling on half its frames still reports a healthy "Avg FPS: 100.0". That reading cost several rounds of chasing the wrong layer. The line now aggregates every frame since the last log and reports median, p95, max, min, and explicit stall and skip rates (past 1.5x the median missed a refresh; under half never reached the panel, because dirty tracking skipped the swap so the frame never waited on vsync). On the hardware this now reads: leaderboard 100.0 fps over 501 frames | median 10.00ms p95 10.05ms max 10.34ms | stalls 0 (0.0%) skips 0 (0.0%) The binding rebuild's blit patch becomes opt-in (RGB_PATCH_BLIT=1, default off). Reordering that loop to row-major changes what a torn frame looks like: column-major tearing shows as a vertical seam, row-major as a horizontal split between the panel's upper and lower halves. On a 1/32 scan panel that reads as a one-pixel fold across the middle of every panel, which is what was reported on hardware and what went away when the blit was reverted. All of the measured gain comes from the SwapOnVSync change, so the risky half is simply not worth taking; the header says so. Also fixes --install resolving its paths against $HOME, which is /root under sudo, so it looked in /root/rgbmatrix-nogil-build and died with "no built module found" on a machine where the build had just succeeded. It now resolves SUDO_USER's home. Both build paths are verified on the Pi: default yields one GIL-release site, RGB_PATCH_BLIT=1 yields two. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(scroll): let users pick a crisp speed for their own panel Whole-pixel motion was previously only available at multiples of the refresh rate -- 100, 200, 300 px/s on a 100Hz panel. 100 px/s crosses a 256px panel in 2.6s, which is brisk for reading, and everything slower had to blend (blur) or repeat frames unevenly (judder). There was no way to ask for 50 px/s and get clean motion. SwapOnVSync takes a framerate_fraction the display manager never passed. It holds each frame for N panel refreshes; the panel keeps refreshing at its full rate throughout, so holding costs nothing in flicker and only changes how often a NEW image is presented. That turns 50 px/s into one whole pixel every second refresh instead of half a pixel every refresh. The crisp speeds are therefore refresh_hz / hold * pixels_per_frame, and that ladder depends on the panel: a Pi Zero on a long chain has a different set of good speeds from a Pi 4 on a short one. crisp_ladder() enumerates them and solve_crisp() picks the best match for a requested speed. solve_crisp weights motion quality rather than picking the numerically nearest entry, which matters more than it sounds. Asked for 30 px/s, nearest-by-value answers 28.6 -- 2px jumps at 14fps -- over 33.3, which is single-pixel motion at 33fps and obviously better on the panel. The target is also clamped into the ladder's range first, because relative error saturates near 1.0 for a target far outside it and the quality penalty would otherwise answer "10000 px/s" with the slowest entry. configure() snaps to the ladder and applies the hold when given a display manager. Without one the hold silently cannot happen and motion falls back to fractional pixels, so it warns rather than failing quietly. set_frame_hold() resets to 1 when scrolling stops, so one plugin's pacing cannot leak into whatever is on screen next. scripts/scroll_speeds.py is the user-facing part: it prints the ladder for the configured rate, measures what the panel ACTUALLY manages (--measure, for hardware that cannot reach its configured limit), highlights the nearest option to a wanted speed, and demos one live. It never starts or stops the display service itself -- doing that inside a script stranded the panel twice today. Speeds below ~20 px/s remain stepped regardless. That is the pixel pitch, not a software limit. Also fixes the dirty-tracking test spy, which stubbed SwapOnVSync with a single-argument function and would have masked the new call as a failed push, and rewrites a configure() test that had started passing for the wrong reason: it asserted a judder warning, which snapping now prevents, and was matching the unrelated "hold could not be applied" warning instead. 183 tests pass on the Pi. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scroll): tie the frame hold to the scroll, not the plugin The hold applied in configure() never reached the panel. Plugins share one display manager, and set_scrolling_state(False) -- fired whenever ANY other plugin finishes its scroll -- reset the hold to 1. A hold set once at plugin construction was therefore always gone by the time that plugin rendered. The symptom was a log line that lied. ledmatrix-stocks reported Scroll configured: 50.0 px/s (1px every 2 refreshes = 50.0 fps, smooth) while the panel measured 100.0 fps, median 10.00ms. Config, resolution and snapping were all correct; only the pacing silently was not applied. set_scrolling_state(is_scrolling, frame_hold=1) now carries it, so the hold lives exactly as long as the scroll that asked for it. configure() reports the value as ScrollSettings.frame_hold instead of applying it -- applying it behind the caller's back could never have been right on a shared display manager. Existing callers are unaffected; the default keeps one frame per refresh. Verified on hardware: stocks at 50 px/s now measures 50.0 fps over 251 frames | median 20.00ms p95 20.09ms | stalls 0 skips 0 20.00ms being exactly two refreshes, with the panel still refreshing at 100Hz underneath so flicker is unchanged. test_another_plugin_stopping_does_not_strand_a_hold pins the interaction that broke this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scroll,cache): resolve CodeRabbit review on #523 Eight findings, all reproduced before fixing. scroll_config.configure() read the refresh rate *after* resolve() had already used it. resolve() fills in target_fps, pixels_per_frame and the judder warning from that rate, so on a 60Hz panel every one of them described 100Hz -- and with snap_to_crisp=False nothing downstream corrected it, so set_target_fps() paced the helper to 100 FPS. The rate is now settled first, and falls back to the global config rather than straight to the default. refresh_hz_from_config() used `(cfg.get("display") or {}).get(...)`, which raises AttributeError when either level is truthy but not a mapping -- out of a function whose whole contract is a rate or a default. The frame-stats line reported the upper-middle sample as the median and the 96th sorted sample as p95 of 100. Both are also thresholds (stalls at 1.5x the median, skips at 0.5x), so the counts were biased too. The arithmetic is now in frame_stats()/format_frame_stats(), testable without a clock. configure()'s docstring and docs/SCROLL_PERFORMANCE.md still said it applies the frame hold and warns when it cannot. It deliberately does neither since "tie the frame hold to the scroll, not the plugin"; a caller following the old text would omit set_scrolling_state() and slow snapped speeds would still present every refresh. disk_cache had no policy for non-finite floats: orjson writes null, the stdlib writes NaN/Infinity, and orjson then rejects those legacy files so DiskCache.get deleted them as corrupt. One behaviour on both paths now -- write null, keep legacy records readable. allow_nan=False detects the values; the replacement walk runs only when there is one, so the ordinary write path is byte-identical and pays nothing. build_rgbmatrix_nogil.sh picked the build artifact with a glob piped to `head -1`, which sorts cpython-311 ahead of cpython-313, so a stale .so staged in from the source tree was installed as core.so while the GIL check -- which reads the generated core.cpp, not the .so -- still passed. It now requires the current interpreter's exact ABI name and fails closed. Its systemctl calls were also unchecked under `set -uo pipefail`: a failed stop left the old service running, the following start succeeded as a no-op, and the health check reported SUCCESS for a binding that was never loaded. orjson floor raised to 3.11.6 for CVE-2025-67221 (unbounded recursion in dumps); it covers the project's Python 3.10-3.13 range. Adds test/test_cache_nonfinite_floats.py (14) plus regression tests in test_scroll_config.py and test_scroll_helper.py. 9 of the cache tests and 9 of the scroll_config tests fail against the pre-fix code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * test(harness): keep the visual double's signature tied to production Moves set_scrolling_state's frame_hold into the test double here, where DisplayManager gains it, rather than in #534 where it arrived a PR early. CodeRabbit flagged the #534 version correctly: a double that accepts an argument production does not lets the call pass every harness run and raise TypeError on the panel, which is the one failure a safety harness exists to prevent. The drift has now gone both ways across two branches -- double behind production on this branch, double ahead of it on #534 -- so it is pinned instead of remembered. test_display_double_parity.py compares the two signatures and fails with the direction of the drift named. It reads the files with ast rather than importing them, because display_manager imports rgbmatrix at module scope and this check should hold on a laptop and in CI as well as on a Pi. Plugins begin passing frame_hold in ledmatrix-plugins#462, which is why production and the double both need it before that lands. Full suite: 3889 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
a0d3e64099 |
fix: ten defects found validating the whole plugin fleet on hardware (#534)
* 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. * fix(core): unique snapshot temp name, honour on-demand requests, skip empty starlark The preview snapshot wrote through a fixed "<snapshot>.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. * 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. * 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. * 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. * 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. * 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. * 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
91d15a8943 |
fix(display): draw text 1-bit, so glyphs stay crisp on the LED grid (#521)
An LED panel has no partial brightness. PIL defaults ImageDraw's fontmode to "L", which anti-aliases TrueType glyphs into a grey fringe the panel can only round off -- a 4px glyph arrives smeared into 3px. DisplayManager creates its shared `draw` in six places and set fontmode at none of them, while _load_fonts loads extra_small_font as 4x6-font.ttf at size 6. Measured at draw time, that face at that size puts 74% of its lit pixels at partial coverage. Every plugin drawing small text through the shared draw inherited the blur; geochron was the case that surfaced it. The harness's VisualDisplayManager had the same gap, which mattered more than it looks: goldens were recording anti-aliased text that production would not produce, so the harness could not have caught this. Fixing only production left geochron still blurry under the harness -- that is how the second site was found. Both are set to "1" so the harness renders what the panel renders. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Chuck
|
32d637a446 |
fix(store): read the core version from disk, not from a stale import (#518)
* fix(store): read the core version from disk, not from a stale import
Updating the core to 3.3.0 and then updating plugins refused all eight sports
scoreboards:
Refusing to install nrl-scoreboard: NRL Scoreboard supports LEDMatrix
>=3.3.0, but this system is running 3.2.0.
while src/__init__.py on that machine read 3.3.0. Observed on hardware, not
theorised.
The gate ran `from src import __version__ as core_version`, which binds
whatever the process loaded at start. The plugin store's gate lives in the web
UI, a long-lived service of its own, and the update route deliberately restarts
nothing -- it replaces files on disk and asks the user to restart. Its prompt
named only the *display* service, so a user who followed it left the web
process holding the previous number.
Stale by exactly one release is the case that bites: every plugin flooring on
the release you just installed is refused, blaming a core version that is
already correct on disk. It reads as a broken plugin store. 3.3.0 is the first
release where this hits a whole family at once, since all eight scoreboards
floor there.
compatibility.current_core_version() reads the version from the file instead,
falling back to the imported value on any failure -- so it can only ever be as
correct as before, never worse. All four gate call sites use it: three in
store_manager (install, the git-pull update path, install_from_url) and one in
plugin_loader's advisory warning.
The restart prompt now names both services.
Twelve tests, including the hardware failure itself: a process holding 3.2.0
while disk says 3.3.0 refuses hockey, and reading fresh allows it. The inverse
is asserted too -- a genuinely old core still refuses, so the gate has not
become permissive. One test greps both modules for the old import-bound read;
reintroducing that line fails it, which is what stops this coming back.
Not changed: web_interface/__init__.py also imports __version__, but for
display rather than gating, and the API endpoint already reports a fresh
git describe.
* fix: drop the unused os import
Left over from a first draft that joined paths by hand before this used
pathlib. Flagged by CodeRabbit on #518; confirmed dead -- no os. reference
remains in the module.
|
||
|
ChuckandClaude Opus 5
|
a686932c7e |
fix(store): land the install_from_url gate, which never reached main (#511)
#510 shows as merged, but into fix/gate-git-pull-updates -- #508's branch -- rather than main. #508 reached main first, so the sideload gate was left behind on a branch. Same failure as plugins #350/#351, which merged into each other's bases; worth knowing the pattern, because GitHub reports these as MERGED and `gh pr list` shows nothing outstanding. main today has two of the three routes gated: install_plugin (#431/#433) and update_plugin's git branch (#508). install_from_url validates required manifest fields and then installs whatever it found, never comparing the core version. Cherry-picked unchanged from the orphaned branch -- it applies to main with no conflict. TestSideloadGate pins the three cases the other routes pin: refuses a floor above this core leaving nothing behind, still allows a compatible plugin (the guard against a gate that refuses everything), and does not block a 2.0.0 floor on a core reporting an untrustworthy version. Full suite 3725 passed, 6 skipped. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
9b522d412c |
fix(store): gate the git-pull update path (#508)
* fix(store): gate the git-pull update path `install_plugin` gates every route that re-downloads, `_reinstall_with_rollback` included. `update_plugin` has one branch that re-downloads nothing: a git checkout pulls in place, installs dependencies, and returns True. A pull could therefore deliver a manifest flooring above this core and nothing would notice until the plugin failed to load — which surfaces as one line in the journal and a display that silently stopped appearing. Checked after the pull rather than before it, for the same reason `_install_plugin_impl` checks after the download: the registry carries no compatibility field, so the incoming floor is only knowable once the new commit is on disk. Undone with `git reset --hard` to the pre-pull commit rather than by removing the directory. This is a live checkout, the old commit is still in the object store, and the reset leaves the user on the exact version they were already running — the same promise `_reinstall_with_rollback` makes, reached by the means this path actually has, with no window where the plugin directory does not exist. An unreadable manifest allows: it is not evidence of a floor. Scope, stated plainly: monorepo plugins install as archives and update through `_reinstall_with_rollback`, so they were already gated. Only registry entries with no `plugin_path` reach this branch. It is closed anyway because the sunset rule in the plugins repo's `08-shared-sports-code.md` names, as condition 3, that the core enforces the floor "at install/update time" — and B6 rests on that being true rather than merely written down. `install_from_url` is still ungated; the tests say so rather than letting the next reader assume otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * fix(store): do not pull what the gate cannot un-pull Review of the gate found a data-loss path it had introduced, plus two smaller scope errors. All three from CodeRabbit on #508. **The stash failure was load-bearing and was not treated as one.** update_plugin stashes local changes before pulling; when that stash failed or timed out it logged a warning and pulled anyway. That was harmless while nothing ever undid a pull. It is not harmless now: the gate's rollback is `git reset --hard`, which discards uncommitted tracked edits -- exactly the edits the stash existed to protect. A pull does not refuse on a dirty tree as long as the incoming commit touches other files, so the sequence completed silently: pull succeeds, gate refuses, reset takes the user's work with it. update_plugin now returns before pulling unless the tree was already clean or was successfully stashed. Refusing costs an update in a case that had already gone wrong; the alternative costs data. That also makes `--hard` safe by construction in _gate_pulled_commit, and its comment now says so rather than observing it in passing. Pinned by test_a_failed_stash_stops_the_update_before_pulling, which writes a local edit, forces the stash to fail, and asserts both that HEAD did not move and that the edit is still on disk. Verified it bites: with the new guard removed the file comes back as `class P: pass`, the edit gone. **_HAS_GIT could take the module down instead of skipping it.** With no git on PATH, subprocess.run raises FileNotFoundError, and this runs at import time -- before skipif can act, so the whole file errors rather than skipping. Now catches OSError. **The doc overclaimed the gate's reach.** It said the floor is enforced on "every route that installs or updates" while the same passage notes install_from_url is ungated. Both spots now scope the claim to registry-managed installs and the two supported update paths, and name the sideload exception. Full suite 3720 passed, 6 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
bdced206dc |
fix(plugins): retain state history by age, with the count as a ceiling (#502)
* fix(plugins): retain state history by age, with the count as a ceiling
Follow-up to the cap in this PR. A flat entry count answers the wrong
question: what a reader wants from this history is "the last couple of
hours", and how many transitions that is depends entirely on the plugin's
update interval. On a real board those span 2s to 3600s, so 200 entries is
interval 200 entries covers
2s 3.3 minutes (flights, live)
10s 16.7 minutes (jellyfin)
60s 1.7 hours (default)
300s 8.3 hours (news)
3600s 4.2 days
-- the plugin churning hardest, the one worth looking at, keeps the least.
So transitions are now trimmed by AGE first
(STATE_HISTORY_MAX_AGE_SECONDS, two hours), which makes the retained window
comparable whatever the cadence, and the count cap becomes purely a memory
ceiling for pollers fast enough to exceed it inside that window. The
ceiling rises 200 -> 2000: at ~230 bytes an entry that is ~0.5MB per plugin
worst case, and only plugins updating faster than roughly every 4s can
reach it. Steady-state memory is unchanged for everything slower, since the
age trim binds first.
Two details worth stating:
- The trim reads time.monotonic(), stored alongside each transition,
rather than the datetime already inside it. A DST shift or an NTP step
would otherwise make every entry look ancient and flush the history in
one go. The human-readable timestamp is untouched and still what
get_state_history() returns.
- Trimming happens on append, so a plugin that goes quiet keeps its last
window until it writes again. That is deliberate: it is bounded either
way, and a lazy trim costs nothing on the hot scheduling path. The
guarantee is therefore about the SPAN of retained history, not its age
against the current clock, and the test asserts it that way.
The public shape is unchanged: get_state_history() still returns the same
list of transition dicts, and state_history_count is still the lifetime
total.
test_plugin_state_history_retention.py adds 7 tests. Verified against this
branch with only the age trim removed: 4 fail, 3 pass -- the three that
survive are testing the count ceiling and the monotonic clock, which this
commit does not change. Full suite 3753 passed, 60 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
* fix(plugins): build get_state_info() as one locked snapshot
Every field was read under its own lock, so an unload running concurrently
could be observed half-done: 'state' read before clear_state() removed it
and 'state_history_count' read after, handing PluginManager.get_plugin_info()
a plugin that is ENABLED with zero transitions.
The whole payload is now built in one critical section. _lock is an RLock,
so the helpers called inside it can still take it.
The regression test runs a reader against a thread that repeatedly fills and
clears the same plugin, and fails on the first torn snapshot. Verified by
removing only the lock: fails on 3 of 3 runs, passes on 3 of 3 with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Ron PierceandClaude Opus 5
|
39e7f8cbe0 |
fix(plugins): cap the per-plugin state transition history (#501)
* fix(plugins): cap the per-plugin state transition history
PluginStateManager recorded every state transition in a per-plugin list
and never trimmed it. The only code that removed entries was
clear_state(), called solely from PluginManager.unload_plugin(), so a
plugin that stays loaded -- normal operation -- never released one.
The list is written on the hot scheduling path. Every update cycle
appends twice: _reserve_for_update() sets RUNNING and _finish() sets
ENABLED back again. At the default 60s update interval that is 2,880
entries per plugin per day, and nothing reads them -- get_state_info()
only takes their len(). Pure dead weight.
Measured against the unpatched class, ten plugins on a 60s interval:
sim uptime history entries heap growth
1 day 28,810 7.7 MB
7 days 201,610 53.9 MB
30 days 864,010 230.9 MB (still climbing)
With the cap it is flat at 2,000 entries / 0.5 MB from day one.
On a 1 GB board 231 MB of garbage is fatal on its own, and the failure
is not a clean OOM: once MemAvailable falls far enough fork() starts
returning ENOMEM, so sshd accepts connections and closes them before its
banner while the kernel still answers pings. The board looks like a
hardware fault and needs a power cycle. Same family as the ceilings
added in #464.
Retain the most recent 200 transitions per plugin in a deque and let the
rest age out. state_history_count is surfaced through the web API, so
the lifetime total is tracked separately rather than plateauing at the
cap. get_state_history() now returns a copy under the lock; it was
handing out the manager's own list, which a caller could mutate.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(plugins): copy history entries out, lock clear_state
Review follow-ups on the transition history.
get_state_history() copied only the outer list, so a caller holding a
returned transition could rewrite the manager's record of what happened
-- which contradicted the defensive-copy guarantee in its own docstring.
Copy each entry too. Every value in a transition is immutable, so a
shallow copy per entry is enough. test_get_state_history_entries_are_copies
pins it; without the change it fails with 'tampered' == 'enabled'.
clear_state() mutated five shared dicts without holding _lock, while
every other mutator takes it. A concurrent set_state() could interleave
and leave a plugin with history but no state. Drop the five as one unit.
This does not close the wider unload-vs-worker race, which lives in
PluginManager.unload_plugin() and predates this change: an update worker
still in flight can call set_state() after clear_state() returns and
recreate the entry. Serialising that needs the per-plugin lock held
across worker join in unload_plugin(), which is a separate change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
ChuckandClaude Opus 5
|
c321b94085 |
fix(display): retry a plugin that is enabled but failed to load (#495)
* fix(display): retry a plugin that is enabled but failed to load A plugin whose validate_config() returns False is treated as a hard load failure. The API then reports enabled=true, loaded=false, error=null: the plugin is simply absent, with nothing saying why. hockey-scoreboard sat in that state on a live rig for four days. The recovery path existed but could not be reached. _reconcile_enabled_plugins computes to_add = desired - current, and a plugin that failed to load is never in current, so it stays in to_add and would be retried. But the reconcile is queued by _enabled_set_changed(), which compares only top-level `enabled` flags -- and the edit that actually fixes such a plugin (enabling a league, filling in an API key) is nested inside the plugin's own config section. No top-level flag changes, so no reconcile is queued, and the save that should have fixed it does nothing. Only toggling some unrelated plugin -- which does change a top-level flag -- queues the global reconcile that recovers it. Add a second gate: queue a reconcile when a discovered plugin is enabled in config but absent from the running set. It is deliberately narrow rather than "reconcile on any config change". Reconcile calls discover_plugins(), a ~39-manifest filesystem scan, and it runs on the render thread; doing that on every config save would trade this bug for a frame hitch. Gating on plugin_manifests also keeps non-plugin sections that carry their own `enabled` flag (schedule, display) from queueing a reconcile they can never satisfy. In the steady state -- every enabled plugin loaded -- the new check is False and costs nothing. The same valid-but-unconfigured => hard-fail shape still exists in text-display, youtube-stats, birdnet-go, ledmatrix-flights and mqtt-notifications; this makes all of them recoverable without a restart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(display): snapshot the plugin mappings under their locks Addresses the review finding on the cross-thread reads. _enabled_plugin_not_running runs on the config-watcher thread and read two mappings the render thread mutates. Catching RuntimeError was not a fix: it turned a torn read into a coin flip between an unnecessary discovery scan and a missed retry, which is the bug this PR exists to remove. Both reads are now snapshots taken under the lock that guards their writes: - plugin_manifests via a new PluginManager.discovered_plugin_ids(), which copies the ids while holding the existing _discovery_lock. Discovery rebuilds that mapping entry by entry, so an unsynchronised reader can see it half-populated. - plugin_display_modes under a new controller lock, taken at the only two sites that mutate it (_register_loaded_plugin / _unregister_plugin). The locks are never nested -- each snapshot is taken and released before the next -- so this cannot deadlock against discovery, which holds _discovery_lock while it rebuilds. No cost on the per-frame path. Both mutation sites run during reconcile, which is rare, and every hot-path read of plugin_display_modes is on the render thread itself, same thread as the writes, so those stay lock-free. Tests: the accessor returns a snapshot rather than a live view, and actually takes the discovery lock (proved from a second thread, since an RLock is reentrant on the owning one) so a later refactor cannot quietly drop it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(display): consume the reconcile request before serving it Addresses the second review finding: a lost update on _pending_plugin_reconcile. The flag was cleared after a successful reconcile. Reconcile has already read its config by that point, so a config change arriving mid-flight set a flag that the trailing clear then erased -- a request that was never served, and the newest config never reconciled. That is the same "my save did nothing" symptom this PR exists to remove, so leaving it would have undercut the fix. Consume the request before running it instead, and re-arm only on a retryable failure. A change that lands during reconcile now stays set and is picked up on the next pass. The per-frame read stays lock-free. It is a fast path that can only produce a false negative -- the watcher setting the flag just after it is read is seen on the next iteration -- never a false positive that loses a request. The lock is taken only when a reconcile is actually pending or a config change arrives. Extracted _service_pending_reconcile() so the sequence is testable rather than buried in run()'s loop; the review asked for a regression test that invokes the subscriber during reconciliation, which is not reachable otherwise. Tests: 4 new, covering a request racing in mid-reconcile, the quiet success, the retryable-failure re-arm, and not reconciling when nothing is pending. Two of them fail against the previous clear-after-success semantics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
5f29243e87 |
fix(config): make the device location the default for plugin location fields (#490)
* fix(config): make the device location the default for plugin location fields A user in Kansas City reported their radar centred on Dallas, TX with nothing in config.json to explain it. The radar is the `ledmatrix-weather` plugin's `radar` mode, and it centres on the same coordinates as every other weather mode: `forecast_data` lat/lon, geocoded from the plugin's own `location_city` / `location_state` / `location_country`. Those ship with schema defaults of Dallas / Texas / US. A user who never opened the weather plugin's config form therefore has no `location_city` on disk, and `PluginManager` merges the schema default in at load time — so the whole plugin (not just the radar) silently runs on Dallas. Radar is just the only mode that draws a recognisable map and gives the mismatch away. Meanwhile the device-wide `location` block that General settings writes was read by nothing at all, despite its own help text promising it was "used for weather, sunrise/sunset, and other location-based content". `SchemaManager.generate_default_config()` now substitutes the device `location` into the three fully-namespaced `location_*` keys before handing defaults back, so the promise holds: - Only `location_city` / `location_state` / `location_country` are substituted. A bare `state` key is left alone — `ledmatrix-elections` uses it for a two-letter code, and rewriting it would break that plugin. - A value the user saved on the plugin still wins: this replaces the schema default, and `merge_with_defaults` puts user config on top. - The substitution is applied on the way out of the defaults cache rather than into it, so changing the device location takes effect immediately. - No config manager, no `location` block, or an unreadable config all fall back to the plugin's own schema defaults. Every caller benefits: the plugin loader, the config form (which now pre-fills the user's real city), config save, and reset-to-defaults. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GNLrSZ32FNKpHRaduKEJsg * docs(web): name the exact plugin keys the device location seeds Review follow-up. The General settings help text said the device location was "the default for every plugin that asks for a city", which overstates what the code does: only the fully-namespaced `location_city` / `location_state` / `location_country` keys are substituted. A plugin with a bare `city` key gets nothing — deliberately, since `ledmatrix-elections` uses `state` for a two-letter code. The tips now name the exact keys. Worth noting for anyone editing these: `ui.help_tip(...)` takes a single-quoted Jinja string, so an apostrophe in the tip text has to be escaped or written around. The wording here avoids them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GNLrSZ32FNKpHRaduKEJsg --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
1fbe244e49 |
fix(plugins): say when discovery skips a directory (#489)
* fix(plugins): say when discovery skips a directory A plugin can be enabled in config, enabled in plugin state, present on disk with a valid manifest and an importable entry point -- and simply absent from the running process, with nothing anywhere to say why. That is not hypothetical. hockey-scoreboard on a live rig is enabled in both places, imports cleanly when loaded by hand, and is listed in the Vegas plugin order, but is not among the 22 plugins the process actually holds. Establishing even that much meant comparing cache-file mtimes to find it had last run three days earlier. The journal had nothing, because discovery does not report what it declines to load. Two paths were silent. A directory with no manifest.json was skipped without comment, which is defensible until it is the thing you are trying to explain. Quieter still, a manifest that parsed but carried no "id" was read successfully and then dropped on the floor -- no warning, no trace, and the plugin simply does not exist as far as the rest of the system is concerned. Both now log a warning naming the directory and the reason. This does not explain the rig above; its manifest has an id. It makes the next occurrence diagnosable from the journal instead of from file timestamps. Reverting the change fails both tests. 65 plugin-system tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(plugins): warn once per directory, not once per scan Self-review catch. Discovery runs on every web UI page load and every config reconcile, so warning unconditionally about an unloadable directory would put a line in the journal each time someone opened a page -- the same log-volume problem this change exists to help diagnose. The skip is now reported once per directory per process. The diagnostic value is unchanged: the reason a plugin is missing still appears in the journal, once, where before it appeared nowhere. Test added covering five consecutive scans producing one warning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(plugins): one unusable manifest no longer aborts the whole scan json.load accepts any JSON value, so a manifest.json holding null, [], "text" or 42 parses without complaint and then raises AttributeError on manifest.get('id'). Nothing catches that: the outer handler around the scan takes OSError and PermissionError only. So a single malformed manifest did not skip that one directory -- it aborted _scan_directory_for_plugins outright, and every other plugin on disk, however healthy, silently failed to register. Reproduced with three directories, the middle one holding `null`: SCAN ABORTED -> AttributeError: 'NoneType' object has no attribute 'get' the two valid plugins never registered That is the same failure this PR set out to fix, in its most severe form: a plugin enabled in config, enabled in plugin state, present on disk, and absent from the running process with nothing to say why -- except here it takes every other plugin with it. A manifest that is not a JSON object is now skipped like any other unusable directory, named once, with what it actually was: Skipping bad-null: its manifest.json is NoneType, not a JSON object Skipping bad-list: its manifest.json is list, not a JSON object scan returned: ['aaa-good', 'zzz-good'] Verified: removing the guard fails 6 of the 10 tests. Covers null, list, string, int and bool, and asserts the healthy plugins either side of the bad one still register. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
863e4a1ecd |
consolidate(perf): cut SD writes, log volume, and metrics churn (#486)
* fix(plugins): one bad metrics cache entry should not stop every plugin
Caught live on a rig: every plugin failing, once each, continuously.
ERROR - src.plugin_system.plugin_manager - plugin geochron operation failed:
ResourceMetrics.__init__() got an unexpected keyword argument
'consecutive_failures'
ERROR - ... plugin text-display operation failed: ...
ERROR - ... plugin news operation failed: ...
ERROR - ... plugin odds-ticker operation failed: ...
with /api/v3/health reporting plugin_system: not_initialized while the display
process itself kept running and updating the panel.
`consecutive_failures` is a plugin_health field, not a metrics one.
get_metrics() does ResourceMetrics(**cached), which raises TypeError on a
single unrecognised key, and that exception escapes into plugin_manager and is
reported per plugin. One malformed cache entry takes the whole plugin system
down.
How a health-shaped record came to sit under a plugin_metrics key on that
machine is not established, and I could not finish the diagnosis: the rig went
back into its EIO failure mode partway through -- SSH resetting pre-banner,
systemctl unexecutable -- while the web API kept answering from RAM. Checked
before that: the cache files on disk are correctly shaped and separate, and
CacheManager.get() returns the right record for each key, so it is not a live
key collision. A restored backup mixing two machines' caches is the likeliest
explanation, and that rig had one restored onto it.
Either way the loader should not be brittle enough for the answer to matter.
plugin_health already repairs its records field by field rather than trusting
what is on disk; this does the same. Known fields are kept, unknown ones are
dropped and named once in the log so a genuine schema change stays visible
rather than being silently discarded, and a non-mapping entry no longer raises.
Keeping the known fields matters: discarding the record wholesale would throw
away real call counts and timings because of an unrelated stray key.
Mutation-checked: restoring ResourceMetrics(**cached) fails 6 checks, dropping
the whole record fails the field-preservation check, and dropping unknown
fields silently fails the logging check. 28 tests pass across the resource
monitor and plugin health suites.
* perf(health): stop rewriting a health record on every healthy cycle
Every successful plugin update called record_success(), which persisted the
record unconditionally. In steady state the only fields that had changed were
total_successes and last_success_time -- a counter and a timestamp that
health_monitor surfaces for display and that nothing reads back after a
restart. Nothing alerts on the age of last_successful_update; it is carried in
the metrics dataclass and shown.
Measured on a rig running 24 plugins, all steady-state (0 consecutive
failures, circuit closed): a five-minute sample caught 22 health-file
rewrites, about 4.4 a minute or 6,300 a day. Each write is ~400 bytes through
cache_manager.set(), which writes a file per call, so each one costs a
filesystem block plus an ext4 journal write.
That lands on an SD card, where the unit of cost is an erase-block cycle
rather than the bytes involved, and where wear is what eventually kills the
card. Two cards have already failed on the other rig with the same
signature -- unreadable block device, EIO on exec, sshd unable to read its
host keys.
The circuit breaker still has to survive a restart, so the write is kept for
exactly the fields it is rebuilt from: consecutive_failures, circuit_state,
circuit_opened_time, half_open_start_time. A failure, a circuit opening and a
recovery are all still written the moment they happen. In-memory state is
updated every time either way, so the health API and web UI show what they
always did.
Tested: 100 healthy cycles now perform zero writes after the first, the
counters remain accurate in memory, and a failure, a recovery and a
half-open-to-closed transition each still reach disk. One test kills and
rebuilds the tracker from the cache to prove the breaker's state genuinely
survives what is no longer written.
Mutation-checked both ways: persisting unconditionally again fails the
steady-state test, and widening _DURABLE_FIELDS to include last_success_time
fails it too. The 46 existing health tests pass.
(cherry picked from commit
|
||
|
ChuckandClaude Opus 5
|
0c5b9c57d3 |
fix: keep low-memory boards reachable under load (#464)
* fix(service): survive corrupt health cache and clean exits Three independent failure modes that each end with a dark panel and no automatic recovery. 1. PluginHealthTracker._load_health_state returned the cached value verbatim. If that value is not a dict, every caller raises AttributeError: 'list' object has no attribute 'get' — during DisplayController.__init__, so the process dies before the display loop starts. systemd restarts it, the same bad entry is read back from disk, and it dies again: an unattended restart loop that survives reboots because the cause is persisted. Observed in the field with plugin_health:<id> holding an unrelated plugin's list payload. Now non-dict entries are discarded with a warning and the defaults are rebuilt. 2. ledmatrix.service used Restart=on-failure, so any exit with status 0 left the unit stopped and the panel dark indefinitely — systemd treats it as success and never brings it back. Restart=always. 3. ledmatrix-wifi-monitor.service used StandardOutput=syslog, which systemd has marked obsolete; it warns and rewrites it to journal on every load. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * perf(memory): size the cache to the board and stop reinstalling deps On a 1GB Pi 3B+ the display process settles around 600MB RSS of 905MB total. When the remaining headroom runs out the failure is not a clean crash: fork() starts returning ENOMEM, so sshd accepts connections and closes them before its banner, timer jobs stop running, and the panel goes dark, while already-resident processes keep serving normally. The board looks healthy from outside and cannot be logged into. Only a power cycle clears it. Three contributing causes: - MemoryCache had a fixed 1000-entry ceiling. Entries are parsed API payloads of tens of KB, so one ceiling cannot serve both a 512MB Zero 2 W and an 8GB Pi 5. Now scaled from MemTotal (150 entries at <=1GB, 1500 at >=8GB), overridable with LEDMATRIX_CACHE_MAX_ENTRIES. - requirements_are_satisfied() returned False for any requirement with extras, so a plugin depending on python-socketio[client] re-ran pip on every single start: ~8s, a network dependency, and a 100-200MB spike at the least convenient moment. During a restart loop it repeats for each restart. Extras are now resolved one level deep against installed metadata, keeping the conservative "anything unverifiable falls through to pip" contract. - ledmatrix.service had no memory ceiling. MemoryMax=85% expressed as a percentage so one unit file suits every board. Note this needs the memory cgroup controller, which Pi firmware disables by default; first_time_install.sh now adds cgroup_enable=memory to cmdline.txt, and the unit file documents how to verify it took effect. first_time_install.sh also enables persistent journald storage (capped at 64M). Default storage is volatile, so every reboot destroys the logs that would explain why the board rebooted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: guidance for 512MB and 1GB boards Documents the memory ceiling on small boards and, more usefully, what running into it actually looks like: sshd accepting connections and closing them before the banner, the web UI still responding normally, clean ping, a dark panel, and a wrong clock after the next boot. None of those read as "out of memory", which makes the failure hard to identify from the symptoms. Cross-referenced from SSH_UNAVAILABLE_AFTER_INSTALL.md, since "I can't SSH in any more" is how most people will first meet this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address review findings on the low-memory work Nine CodeRabbit findings, five in code. **Health state (the one that matters).** The non-dict guard did not cover a dict missing fields the callers index directly, which is the shape actually seen in the wild: a record carrying only circuit_state produced `plugin clock-simple operation failed: 'circuit_state'` about fifty times a minute with the panel frozen. The record is now completed against the defaults per field rather than trusted or discarded wholesale. Per field matters: a first pass rejected any incomplete record outright, which reset a tripped breaker and real failure counts to healthy because one optional field was absent -- an existing test caught it. Values of the wrong type (a counter persisted as a string, an unknown circuit_state) fall back individually, valid neighbours survive, and newer fields the schema has grown since (degraded, degraded_reason) are carried through untouched. **Cache ceiling.** MemoryCache.set() accepted entries without bound between cleanup sweeps, which run every 300s by default, so a burst could take the cache far past max_size -- the unbounded growth the limit exists to stop. Eviction now runs under the same lock on every write, sharing one helper with the periodic sweep so the two cannot drift. **Installer, cgroups.** Only cgroup_enable=memory was checked, so a board carrying that without cgroup_memory=1 reported success and got no change, leaving MemoryMax= inert. Each parameter is now checked and appended independently; verified against all four combinations, single line preserved. **Installer, journald.** Persistence was inferred from /var/log/journal being non-empty, which proves neither Storage=persistent nor a size cap -- the directory survives a switch back to volatile. The effective configuration is read instead (systemd-analyze cat-config, falling back to the conf files), and an explicitly configured SystemMaxUse is preserved rather than overwritten. Verified across volatile, persistent-without-cap, persistent-with-user-cap, cap-without-storage, and commented-only configs. **Dependency extras.** _extras_are_satisfied stopped at one level, so a gated dependency that itself requests an extra (requests[socks]) passed on the base distribution's version while the extra's own dependency was missing, and pip was skipped. It now recurses, with a visited (distribution, extras) set so a cycle terminates. Docs: both kernel command-line paths documented (the installer falls back to /boot/cmdline.txt), daemon-reload and restart added after the systemd override example, memory exhaustion added to the SSH summary with its power-cycle-only recovery, and a language on the fenced block for MD040. Tests: five for the health-state repair including the exact wild shape and that record_failure/record_success no longer raise against it, and one for the cache ceiling. Both mutation-checked. Full suite 2927 passed, with the one pre-existing tmpfs failure that also fails on main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix: harden the health-state repair and confirm journald took effect Second review round; all three findings were valid and two were bugs in the repair added last commit. The repair could raise out of itself. An unhashable circuit_state (a list or dict on disk) hit `value in {...}` and raised TypeError -- from the code whose whole job is to stop a malformed record crashing the caller. It now requires a str before the membership test. bool is a subclass of int, so True passed the timestamp check and then compared as 1.0: enough to expire a cooldown the instant the breaker opened, while False would stop the elapsed check firing at all. Timestamps now exclude bool explicitly. The regression test for the original crash was seeded with a record that *contained* circuit_state, so it passed against the old raw-return behaviour too -- the counters are read with .get(), so circuit_state is the only field whose absence used to raise. Reseeded to omit it, and it now fails against raw-return as intended. journald: drop-ins apply in lexical order, so a local file sorting after ledmatrix-persistent.conf still wins and writing ours proves nothing. The effective Storage is re-read afterwards and a warning naming the diagnostic command is printed if persistence is still not active, rather than reporting a success that was not verified. Full suite 2934 passed, same single pre-existing tmpfs failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
0901d044d3 |
fix(plugins): report updates that completed, not ones that were queued (#460)
run_scheduled_updates_with_changes() snapshotted plugin_last_update, called run_scheduled_updates(), and diffed the two to answer "whose data just changed". But run_scheduled_updates() only enqueues. The work runs on the update worker and stamps plugin_last_update there, after this method has already returned, so the two snapshots were always identical and the result was always an empty list. The only path that ever worked was the synchronous kill-switch, where update() runs inline. Vegas is the caller. That empty list is what feeds mark_plugin_updated(), which drops the cached content for a plugin whose data moved -- so a segment kept scrolling whatever it was first built from. It is the failure the coordinator's own comments describe: last night's live game still drawn as live the next morning. On a live rig: zero update ticks in twenty minutes, with weather, stocks and news all updating on schedule. The worker now records each completed update in a ledger and the call drains it, reporting what has finished since the previous poll rather than what this call enqueued. That costs one tick of latency -- Vegas polls every ~4s -- and is correct whichever side of the queue the work lands on. Failure paths are excluded: they stamp the timestamp too, to space out retries, but no fresh data exists. Verified on the rig it was found on: 0 update ticks before, 208 in twenty-five minutes after, naming real plugins. The behavioural tests here would pass with both production call sites deleted, which mutation testing caught -- they drive the ledger directly. So there is also a structural test asserting the invariant at the source: wherever a successful update stamps plugin_last_update, it must record the completion. Writing it immediately caught that _record_update_failure stamps the same field and must not be included. Mutation-checked: removing either call site, removing both, and dropping the drain's clear are all caught. Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
08265c1135 |
feat(vegas): let live content keep its place in the ticker (#457)
* feat(vegas): let live content keep its place in the ticker Live content used to preempt Vegas outright: while any plugin reported live priority the display controller refused to run the ticker at all and showed a full-screen scoreboard instead. Keeping the marquee meant not seeing live scores; seeing live scores meant losing the marquee. Two changes, both off by default. vegas_scroll.live_in_ticker keeps the ticker running through a live game. Three places assumed the takeover and all three now honour it: the controller's gate, the coordinator's per-frame pause, and the rotation switch that would otherwise move current_mode_index underneath a ticker that never yields. And the rotation is no longer a strict round robin. It was one slot per plugin per cycle, so with a dozen plugins enabled a live score came round once a lap and could be minutes old on screen. A plugin can now hold several slots, placed by Smooth Weighted Round-Robin -- the same scheduler the sports plugins already use to rotate their own games. The property that matters is that repeats are spread through the cycle rather than clumped: three in a row and then silence would be worse than no boost at all. Weight comes from the plugin first, via a new optional get_vegas_priority_weight(), then from the core: live content earns live_weight, everything else 1. So existing plugins gain the behaviour without changes, and the hook exists for the one thing the core cannot work out -- the core can see that a game is live but not whose, so only the plugin can say a favorite is playing. Documented in ADVANCED_FEATURES (worked example, why weights are per plugin not per game, and that frequency is not freshness), CONFIG_REFERENCE, PLUGIN_API_REFERENCE, and the config template. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(vegas): carry the new keys through config, and correct two docs Three findings from CodeRabbit, all valid. to_dict() and update() enumerate keys explicitly and had not learned the three new ones, so get_status() never reported them and a live config change never applied -- turning live_in_ticker on in the web UI would have done nothing until a restart. update() clamps the weights exactly as from_config does. The vegas_scroll key count in ADVANCED_FEATURES said 29; the template has 30. My arithmetic, not the reviewer's. The third was a documentation error rather than a code one, and I have fixed it the other way round. The docs claimed a raising get_vegas_priority_weight() is treated as weight 1. The code instead falls through to the core's own live-content check, and that is the better behaviour: the hook is only how a plugin asks for *more* than live_weight, and has_live_priority/has_live_content are separate methods guarded separately, so a plugin with a broken weight calculation should lose the favorite distinction and keep the live boost. Said so in the code, the base-plugin docstring and the API reference. The test fake now fails in each place independently, because the two failures mean different things: a broken hook still earns live_weight, a plugin that cannot say whether it is live has nothing to fall back on and weighs 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ui/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(vegas): stop the heaviest plugin doubling across the cycle seam Smooth Weighted Round-Robin spaces repeats well within a pass, but it schedules the heaviest item first and usually last as well. The strip loops, so those two are neighbours: the marquee showed the same plugin twice running at exactly the one join a within-cycle check cannot see. Observed on a live rig at 28 slots -- gaps of 6, 7, 7, 7 and then 1. Rotating the list does not fix it. Rotation preserves the cyclic order exactly, so it moves where the seam is drawn rather than the adjacency itself; the trailing entry has to be swapped with one from the middle. The first version swapped with the first slot that merely fitted, which undid the spacing this exists to protect -- it moved a repeat from a gap of 7 into a gap of 2, more clumped than the seam had ever been. It now picks the candidate furthest from any other appearance, so the repeat lands in the widest gap. Left alone when no candidate exists. A plugin holding most of the slots has to neighbour itself, and scheduling it is better than refusing to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(vegas): stop the seam repair creating the duplicate it removes Swapping the trailing repeat with a middle slot moves two elements, and the candidate filter only guarded one of them. It checked the neighbours `repeated` would acquire at j, but not what the displaced element would sit beside at the end -- so ['a','b','c','d','x','y','x','a'] came back as [...,'x','x'], the seam duplicate traded for a fresh one. Reported by CodeRabbit with that exact case. Adding the missing condition fixed it and immediately broke something else: schedule[j] is schedule[-2] when j is the second-to-last slot, so that candidate was always excluded, and ['a','b','c','a'] lost the only repair it has. The same class of mistake twice, from reasoning about which neighbours two moved elements end up with. So it no longer reasons. It performs each candidate swap, counts the cyclic duplicates in the result, and keeps the best one that has none -- preferring whichever leaves the boosted plugin most evenly spread. When no such swap exists the schedule is returned untouched, which is the unavoidable case: a plugin holding most of the slots has to neighbour itself. Fuzzed across 6,956 seam schedules: none made worse, none lost an entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
f887063434 |
test(harness): flag a mode that draws nothing without reporting it (#447)
The controller skips a mode whose display() returns False and treats
anything else -- including None -- as "content was shown". A mode that
draws nothing and does not return False is therefore never skipped, and
because a mode switch clears the panel first, it sits on a blank screen
for its whole display duration. Two sports plugins shipped exactly that.
The harness rendered those modes and passed them, because it called
display() and discarded the result. Capture it, and warn when a render
produced no lit pixels while claiming content.
Warn-only by default, and deliberately so: a scroll mode's first frame
is legitimately its blank scroll-in buffer, which is 42 of these on the
F1 scoreboard alone. Plugins whose modes are known to draw on their
fixture data can opt into failing via harness.json {"empty_check":
"strict"}, matching how the fill check is staged.
Worth being clear about the limit: this only sees what the fixtures
render. It would not have caught the sports bug, whose fixture seeds
games so the empty path never renders -- that needs the source-level
gate in the plugins repo. What it does catch is the same mistake in any
plugin whose empty state the harness does happen to reach, which is
coverage there was none of before.
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
ChuckandClaude Fable 5
|
ee59caa577 |
Follow-ups from #441: secret-helper migration, ten more bug fixes, and coverage for every remaining untested module (#444)
* refactor(web): use canonical secret helpers in api_v3; make ConfigManager secret strip/merge array-aware
api_v3.py carried three inline nested copies of find_secret_fields/
separate_secrets (main-config save, plugin-config save, plugin-config
reset). They drifted from each other (one lacked isinstance guards) and
none supported the canonical module's array-item secrets
(accounts[].token). All three endpoints now import from
src/web_interface/secret_helpers.
Adopting the canonical behavior makes array-item secrets reachable, and
their parallel-placeholder shape ([{'token': ...}, {}] alongside the
regular list) was not survivable by ConfigManager's round-trip:
_strip_secrets_recursive dropped the whole key (losing the regular
fields from config.json) and _deep_merge replaced the regular list
wholesale on load. Both are now array-aware:
- strip removes the secret fields from each item and ALWAYS keeps the
list so indices survive for merge-on-load; whole-key secrets (scalar
lists, shape mismatches) still drop the key entirely — never leak.
- merge folds each secrets item into the config item at the same index,
skipping {} placeholders. The regular list's length is authoritative
in both directions: a user deleting an array item never has it
resurrected from a stale secrets entry (extras warn and are ignored).
api_v3's own deep_merge intentionally still replaces lists wholesale —
form posts carry complete arrays and index-merging would resurrect
deleted items; a comment now documents that.
Tests: the parity guard flips from 'exactly 3 inline copies' to 'zero,
and the canonical import must exist'; TestArraySecretStripAndMerge
covers the new strip/merge semantics incl. length-mismatch contracts;
new test_api_v3_secret_roundtrip.py drives all three endpoints through
a Flask client with a REAL ConfigManager+SchemaManager over tmp_path,
proving secrets land in config_secrets.json, config.json stays clean,
and a fresh load merges them back into the right array items.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
* fix: repair broken helper paths across display, cache, odds, logging, resolver, repos, config, validator
Nine fixes for bugs surfaced while writing coverage for previously
untested modules (plus the bool-duration quirk pinned in PR #441):
- base_plugin.get_display_duration: exclude bools from both numeric
branches — display_duration=True no longer reads as a 1-second slot;
it falls through to config, then the 15.0 default.
- display_helper: draw_error_message/draw_no_data_message called
_draw_centered_text with the wrong arguments and crashed with
AttributeError — both now delegate to draw_centered_text.
draw_scorebug_layout drew status and clock at the same y, overprinting
each other — they now share one combined top line.
draw_ticker_layout drew its text starting at x=display_width (fully
off-canvas), returning a blank frame every time — now draws at x=0;
scroll_speed stays accepted-but-unused and is documented as such.
- api_helper.clear_cache guarded on a nonexistent CacheManager.clear()
method, silently never clearing anything; it now uses the real surface
(clear_cache/delete/list_cache_files) and no-ops safely otherwise.
- base_odds_manager._extract_espn_data raised AttributeError when ESPN
sent explicit JSON nulls ("homeTeamOdds": null) — every level now
null-safes with 'or {}'. format_odds_summary gated on
is_odds_available, which deliberately ignores money lines, so
ML-only odds formatted as "No odds available" — it now gates only on
empty/no_odds data and formats money lines.
- logging_config.ContextualFormatter mutated record.msg in place, so a
second handler prepended the context prefix twice; it now formats a
copy. log_error hardcoded exc_info=True and raised TypeError when the
caller passed exc_info — now kwargs.setdefault.
- dynamic_team_resolver wrote its "shared" class cache through self,
creating instance shadows — the cache was per-instance and every
scoreboard refetched rankings. Writes now go through the class.
- saved_repositories cleaned URLs with an unanchored .replace('.git','')
that mangled URLs merely containing '.git' (my.github.io -> myhub.io);
now strips only a trailing suffix. add/remove also roll back the
in-memory list when the save fails, so memory always matches disk.
- config_helper.merge_configs shallow-copied the base, aliasing every
un-overridden nested dict into the result — now deep-copies.
- startup_validator.validate_all accumulated errors/warnings across
calls — now resets both lists per run.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
* test: cover the previously untested modules
Nine new suites plus an extension, asserting the Phase-1b fixed behavior
and pinning the quirks deliberately left alone:
- test_logging_config.py: formatters (JSON shape, no record mutation,
single prefix through two handlers), PluginLoggerAdapter precedence,
setup_logging handler hygiene and LEDMATRIX_DEBUG, log_error exc_info.
- test_startup_validator.py: exact messages, error-vs-warning split,
accessor split (load_config vs get_config), cache-dir branches with
os.access monkeypatched (root can write anything in CI), idempotence,
raise_on_errors classification precedence.
- test_config_helper.py (full): load/save round trips, dot-notation
get/set incl. silent-failure contract, post-fix no-aliasing merge,
schema validation branches, the '{id}_config' key pin, default-enabled
pin.
- test_saved_repositories.py: three load shapes, bare-list rewrite pin,
trailing-only .git strip (my.github.io regression), save-failure
rollback, type-classification case-sensitivity pin.
- test_api_helper.py: rate-limit math, cache-hit short circuit, ESPN
URL/key formats, exact User-Agent guard, retry adapter, post-fix
clear_cache against the real CacheManager surface, ttl-dropped pin.
- test_base_odds_manager.py: cache-key/URL construction, no_odds
sentinel round trip, stale-cache fallback, null-safe extraction,
ML-only formatting, is_odds_available truth table (ML-blind by
contract), config key/attr mismatch pin.
- test_dynamic_team_resolver.py: expansion/dedup/slicing, dropped
unknown-dynamic names (TOP_ substring hazard pinned), genuinely
shared class cache (second instance: zero HTTP), TTL expiry,
failure degradation without raising.
- test_display_helper.py (full): the fixed error/no-data renders,
combined scorebug top line, non-blank ticker with scroll_speed
no-op pin, composite upconversion, logo bleed positions, square
orientation pin.
- test_skin_runtime_cache.py: discovery-cache hit/invalidation
semantics (manifest mtime, .py edits pinned as non-invalidating),
sys.modules namespacing contract incl. bare-name restore and stdlib
shadowing, entry-module execute-once, API minor-version tolerance,
skin_matches_target table.
- test_sports_capabilities.py (extended): _draw_celebration_layout
executed for real (flash window, matrix-dims fallback, highlight
alternation, logo-failure isolation), _should_celebrate_for direct,
strict duration boundary, score_to_int edges, both-teams-score
precedence, expired-coalesce refire, disabled-win baseline
preservation, id-less prune.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
* test: real schedule/dim coverage for DisplayController; fix two vacuous schedule tests
New test_display_controller_schedule.py drives _check_schedule and
_check_dim_schedule on a bare controller stub: same-day and
midnight-crossing windows with inclusive boundaries, global vs per-day vs
legacy-inferred modes (and dim's global-only default — no legacy
inference), per-day disabled days, invalid %H:%M fallbacks, unknown
timezone -> UTC, dim_brightness default 30, inactive-display short
circuit, and the _was_display_active/_was_dimmed transition flags.
test_display_controller.py's test_schedule_disabled and
test_active_hours patched config_service.get_config — which
_check_schedule never reads — so both asserted the init-default value
and could not fail. Rewritten on the test_inactive_hours pattern
(inject controller.config['schedule'], reset the minute gate, flip the
flag to the opposite state first so the assertion has teeth).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
* ci: raise coverage floor to 48%
Measured 50% with the new suites in place (was 47% baseline when the
gate was introduced at 45); floor stays two points under measured.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
* fix: address CodeQL alert and review findings
- config_manager: the "secrets list longer than config list" warning now
interpolates only config-side data (no key name or secrets-derived
values), resolving the CodeQL clear-text-logging alert.
- base_plugin: validate_config rejects bool display_duration, matching
get_display_duration (bool is an int subclass and would otherwise pass
as a positive number).
- config_helper: merge_configs deep-copies override values in the
non-recursive branch so mutating the merged result cannot reach back
into override_config.
- saved_repositories: saves are atomic (temp file + fsync + os.replace),
so a failed write can no longer truncate saved_repositories.json.
- tests: regression cases for each fix, plus a pin that whole-item
array secrets (key[] + key[].field both marked) strip to empty {}
skeletons — no secret values can reach config.json.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
---------
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
ChuckandClaude
|
31d607f6b3 |
fix(backup): make a restored device match the one that was backed up (#439)
* fix(backup): make a restored device match the one that was backed up Found by wiping a working device and reinstalling from scratch. Every problem here is invisible until you actually do that, which is why a green test suite and eleven hours of uptime had not surfaced any of them. **Four enabled plugins vanished on restore.** Weather, stocks, music and leaderboard have a registry `id` that differs from the `id` in their own manifest: the registry calls them `weather`, everything else calls them `ledmatrix-weather`. Installation already prefers the manifest id for the directory name and warns when the two disagree, so on disk, in config.json and in a backup they are `ledmatrix-weather` -- but nothing resolved that in reverse. Restore asked the store for `ledmatrix-weather` and got "Plugin not found in registry", four times, and the device came back missing four plugins the user had enabled. Registry lookup now falls back to matching `plugin_path`, which already records `plugins/ledmatrix-weather`. Renaming the published ids would have orphaned `plugin_state.json` entries keyed on the old ones. Exact id still wins, so a path that collides with another entry's id cannot shadow it. Against the live registry and a real 28-plugin install this takes unresolvable directories from five to one -- the one being starlark-apps, which is genuinely not in the registry. **Secrets could not be restored at all.** A fresh install left config_secrets.json group-readable but not group-writable, and the web interface -- which is what performs a restore -- does not necessarily run as the owner. Every other file in the backup restored; secrets failed with EACCES. Now group-writable, so the account running the web UI can put them back. **A partial restore reported "Restore had errors" and nothing else.** That is the same message whether the whole thing failed or it quietly dropped your API keys. It now names what was restored, what failed, and which plugins were not reinstalled. **ytm_auth.json was never in the backup.** It sits in config/ beside the three files that are, and is pure device-local auth: losing it silently signs the user out of YouTube Music. Backed up and restored with the wifi config, which it resembles. **Backups were written inside the directory a reinstall deletes.** config/backups/exports is destroyed by the reinstall the user was told to make it before. Exports now go beside the install, falling back to the old path when that is not writable. **The installer reboots without asking in non-interactive mode**, which the README did not mention -- easy to hit when piping the install, and alarming when a device you are installing onto disappears. Documented, with --no-reboot-prompt. Its log also claimed root:ledmatrix while printing a hardcoded group name rather than the one it used. Tests: registry resolution gets its own suite, including the collision case and third-party entries with an empty plugin_path. The existing round-trip test passed throughout this because its fixture plugin has a directory name equal to its id -- the one shape that cannot fail -- so it now carries ytm_auth too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * ci: run the backup suites test_backup_manager.py existed but was never enrolled, so the tests that should have guarded backup and restore have not run on a pull request. That is part of why the restore bugs in the previous commit reached a device: the suite was there, it just was not watching. Adds it alongside the new registry-resolution tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(backup): restore must not depend on owning the file it replaces Follow-up from testing the previous commit on real hardware, where the secrets fix turned out to be both too narrow and slightly wrong. Too narrow: config.json, wifi_config.json and ytm_auth.json are installed root-owned and group-readable exactly like the secrets file, so all four were unrestorable by the web service, not just one. `shutil.copy2` opens the destination for writing, which needs permission on the *existing file*; the web user could create files in that directory all day and still not replace them. Slightly wrong: the previous commit loosened the secrets file to group-writable. That was treating the symptom. The real error was deciding ownership from `ledmatrix.service` -- the display service, which runs as root and only ever *reads* secrets -- when the account that *writes* them is the web interface, which deliberately does not run as root. Ownership now follows the web service's user and the mode stays 640. `_copy_file` writes a temporary file alongside the target and renames over it. That needs only directory permission, so a restore no longer cares who owns the destination, and it is atomic: a crash mid-restore can no longer leave a half-written config. The destination's mode is carried across so restoring secrets does not widen them to the umask, and its owner is carried across too when the OS allows it -- only root can hand a file to another user, so a restore run by the web service keeps its own ownership rather than pretending to preserve root's. Verified on a device with all four config files set root-owned 640 and unwritable by the web user: before, every one failed with EACCES; after, the restore reports success with no errors and all four sections restored, mode still 640, root still able to read them and the web service still able to write them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(backup): address CodeRabbit and CodeQL findings on PR #439 - first_time_install.sh: verify chown/chmod succeed and the final owner/group/mode on config_secrets.json before reporting success; exit with a clear error otherwise instead of swallowing failures. - api_v3.py: replace the predictable .writetest probe with an exclusive NamedTemporaryFile to avoid a race with concurrent resolvers; log the preferred/fallback export path and OSError when falling back to the reinstall-deleted directory. - api_v3.py: mark a restore as failed when plugin reinstalls fail, even if file restoration itself succeeded, so the endpoint no longer reports HTTP 200 success on a partial restore. - api_v3.py: stringify plugin IDs before joining them into the error message so a malformed backup's non-string plugin_id can't raise a TypeError and mask the detailed response. - backup_manager.py / api_v3.py: stop putting raw exception text (originating from a user-controlled backup file) into restore results returned to the client; log full details server-side instead. Addresses the CodeQL "stack trace information exposure" alert. - test coverage: add a test for get_plugin_info() resolving a manifest id, and assert the disabled restore_wifi path also skips and omits ytm_auth.json. Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Fable 5
|
d6c5f97c13 |
Test suite overhaul + fixes for the three bugs it uncovered (#441)
* ci: run the whole test tree and make the plugin-safety job assert something real The unit-tests CI job ran an explicit 24-file allowlist that had rotted: 63 of 90 test files (display, vegas, store manager, web API, web_interface) never ran on a PR. The job now runs all of test/ (minus test/plugins, which the plugin-safety job owns) so new test files are enrolled by default and any exclusion needs a visible, commented --ignore. The plugin-safety job was a green no-op: plugins/ is empty in CI, so every test skipped with 'Manifest not found'. It now renders a bundled deterministic fixture plugin (test/fixtures/plugins/ci-fixture-plugin, golden images included for all 8 default sizes) via LEDMATRIX_PLUGINS_DIR, and sets LEDMATRIX_REQUIRE_PLUGINS=1 so discovering zero plugins fails loudly instead of skipping green. The per-plugin suites document that they target dev machines with real plugins installed. Coverage is now measured and enforced in exactly one place — the CI unit-tests step (--cov=src --cov=web_interface --cov-fail-under=45, from a measured 47% baseline). pytest.ini previously declared --cov-fail-under=30 but CI always passed --no-cov, so the gate had never run anywhere; local pytest is now coverage-free and fast. Enabling the 63 unenrolled files surfaced three cases of test rot, fixed here: test_display_controller_vegas_tick.py could not collect without the hardware rgbmatrix module (now uses the emulator convention), the state-reconciliation unrecoverable-cache tests broke when production added the is_plugin_uninstalled tombstone check (bare Mock returned truthy), and test_get_system_status assumed the optional psutil dependency (now installed via requirements-test.txt and guarded by importorskip). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test: replace can't-fail tests with real assertions test_font_manager.py was 5 of 6 tests shaped as 'try: call(); assert True / except: assert True' — running in CI while unable to fail on any regression. Rewritten against the real FontManager API and the bundled assets/fonts: returned font types, cache-hit identity, distinct entries per size, default-font fallback for unknown families and corrupt files (recorded in failed_loads), BDF native-size reading, text measurement, and cache lifecycle. test_display_manager.py's test_draw_text ended in 'assert True'; it now renders onto a known-black canvas and asserts pixels were actually lit — which required un-breaking the fixture's freetype MagicMock so draw_text's isinstance check doesn't silently swallow the draw. test_display_controller.py carried a permanently-skipped test whose skip reason already declared it redundant; deleted. Both display test files now set EMULATOR=true before importing display_manager (the same convention as test_display_dirty_tracking.py) so they collect standalone instead of depending on which test module imports display_manager first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test: cover the untested fragile logic (compatibility gate, secrets, config merges, durations, skin cards) New unit tests for pure or filesystem-only logic that previously had zero direct coverage: - test_compatibility.py: the semver install gate (parse_semver suffix handling, every range operator, TRUSTWORTHY_FLOOR behavior for cores reporting untrustworthy versions, 'more restrictive wins', and the malformed-manifest shapes that used to raise). - test/web_interface/test_secret_helpers.py: the canonical x-secret helpers — find/separate/mask/remove, array-item secrets, no input mutation, and a separate->recombine round-trip. - test/web_interface/test_api_v3_helpers.py: the module-level helpers behind the plugin config save endpoint (_is_plugin_update_available, _coerce_to_bool including the int==1 quirk, deep_merge including its shared-subtree shallowness, _parse_form_value, dotted-key-aware _get_schema_property/_set_nested_value). - test_base_plugin_duration.py: get_display_duration's full coercion ladder (instance attr -> config -> 15.0), including the bool-is-int quirk where display_duration=True means one second. - test_config_manager_secrets.py: the secrets round-trip — deep-merge on load, strip on save, group pruning, the load fast path — and two characterized sharp edges marked SUSPECTED BUG: an unreadable secrets file at save time writes secrets into config.json in plaintext, and a same-mtime-same-size content swap is served stale. - test_schema_manager_merge.py: merge_with_defaults branch behavior (None replacement vs falsey preservation, dict-vs-scalar mismatches, arrays replaced wholesale, defaults never mutated). - test_skin_system.py (extended): render_skin_card shares _render_game's 3-strike counter but never resets it on success — the asymmetry is pinned in both directions, along with card fallthrough and the disable interaction between the two paths. Suspected bugs are characterized, not fixed — each carries a comment so a future behavior change is deliberate rather than accidental. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test: add drift guards for cross-file contracts Three guard suites that pin contracts spanning multiple files, where one side changing unilaterally breaks the other silently: - test_version_comparison_consistency.py: the repo's four version comparators (compatibility.parse_semver, api_v3's packaging-based _is_plugin_update_available, store_manager update_plugin's raw string equality, skin_runtime._major) answer differently on the same inputs. A table pins each one's verdict; update_plugin is driven through its real code path to show the SUSPECTED BUGs: 'v1.2.0' vs '1.2.0' triggers a full reinstall the UI calls unnecessary, and a locally-ahead plugin gets downgraded. A pairwise-ordering check keeps parse_semver agreeing with packaging on plain X.Y.Z. - test/web_interface/test_secret_separation_parity.py: api_v3.py carries three inline copies of find_secret_fields/separate_secrets that lack the canonical module's array-item support. The copy count is asserted exact (it may only go down; new copies must import src/web_interface/secret_helpers), the missing-array-support gap is asserted so it can't grow silently, and the canonical behavior that migration will adopt is documented executably. - test_discovery_path_contract.py: the three 'where is plugin X' resolvers (PluginManager discovery, StoreManager._find_plugin_path, SchemaManager.get_schema_path) agree on the configured directory, and their divergent fallback chains are characterized. Also pins the .standalone-backup- naming contract shared by store rollback and discovery, and _resolve_skin_target's path-traversal rejection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test: address review feedback — fixture lifecycle, test names, ClassVar - ci-fixture-plugin: call display_manager.clear() before rendering (per plugin guidelines — the fixture should model a well-behaved plugin), add a class docstring, and document why Pillow is deliberately not pinned in its requirements.txt (core dependency; harness installs nothing). - Rename two tests whose names contradicted their assertions: test_unparseable_core_version_is_compatible -> test_unparseable_core_with_high_floor_is_blocked, and test_unreadable_secrets_file... -> test_corrupt_secrets_file... - Annotate TestGetSchemaProperty.SCHEMA as ClassVar (RUF012). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * ci: allow manual test.yml runs via workflow_dispatch Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * fix: unify version comparison, refuse secret-leaking saves, reset skin strikes on card success Fixes the three suspected bugs this PR's characterization tests pinned, flipping those tests to assert the corrected behavior: - plugins/store: ONE shared update comparator. New compatibility.is_update_available() (PEP 440 via packaging) is now used by both the web UI's update badge (api_v3._is_plugin_update_available is a thin alias) and store_manager.update_plugin's reinstall decision. Previously update_plugin used raw string equality: 'v1.2.0' vs '1.2.0' triggered a full reinstall the UI called unnecessary, and a locally- ahead plugin (2.0.0 installed, registry 1.9.0) was silently DOWNGRADED. Now equivalent spellings skip the reinstall and locally-ahead versions are never downgraded; unparseable versions still reconcile by reinstalling from the registry. - config: save_config and save_config_atomic now refuse (ConfigError) when config_secrets.json exists but cannot be loaded. Both previously proceeded without stripping, writing the merged secrets into config.json in plaintext. The shared _load_secrets_for_save() helper raises with an actionable message instead; a missing secrets file is still fine (nothing to strip), and _migrate_config's catch-all keeps boot resilient. - skins: render_skin_card resets _skin_failures on both success paths (vegas card returned, or mode renderer handled), mirroring _render_game. Transient card failures no longer accumulate across a session until they permanently disable a working skin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * fix: harden shared comparator edges from review - is_update_available: reject truthy non-string versions (a malformed manifest can carry a number; packaging raises TypeError on those) by surfacing the mismatch instead of raising. - store_manager.update_plugin: drop the truthiness gate around the comparator so a missing version on either side follows the shared 'no update' verdict, keeping the store consistent with the UI badge; a missing manifest still uses the reinstall recovery path. - config_manager._load_secrets_for_save: catch only expected read/parse failures (OSError/ValueError/RecursionError) so implementation bugs propagate as themselves, and log with traceback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
ChuckandClaude Fable 5
|
d9683e28be |
Codebase audit: fix shipping bugs, remove verified-dead code, repair doc drift, add regression guards (#438)
* fix(web): implement delete_cached so the font catalog cache actually invalidates
api_v3.py's font upload/delete handlers import delete_cached from
web_interface.cache, but the function was never defined. The surrounding
except ImportError silently swallowed the failure, so the fonts_catalog
cache entry survived uploads/deletes and newly uploaded fonts did not
appear until the TTL expired or the service restarted.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* fix(web): remove dead weather/stocks partial routes that returned 500
The partial dispatcher still routed 'weather' and 'stocks' to loaders
rendering v3/partials/weather.html and stocks.html — templates that no
longer exist since weather and stocks became store plugins. Requesting
either partial raised TemplateNotFound, which the catch-all turned into
a 500. No template or JS references these partials (the only 'weather'
hit in the front end is a plugin-store category filter option), so the
branches and both loader functions are removed; unknown partials now
fall through to the existing 404 handler.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* fix(deps): align contradictory psutil/Flask-Limiter/freetype-py pins
requirements.txt's optional-install comment recommended psutil>=5.9,<6.0
while web_interface/requirements.txt hard-requires >=6.0,<7.0 — anyone
following the comment ends up with an unsatisfiable pair. The comment now
recommends the same range the web interface requires (all psutil APIs
used — Process, boot_time, cpu_percent, disk_usage, virtual_memory — are
stable in 6.x). Flask-Limiter gains the same <4.0 cap in both files and
freetype-py the same >=2.5.1 floor.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* fix(config): add template keys the code already reads
display.hardware gains pixel_mapper_config, row_address_type,
multiplexing and panel_type (read at display_manager.py with these exact
fallbacks — users on non-standard panels previously had no way to
discover them from the template). vegas_scroll gains
frame_based_scrolling and scroll_delay, the only two of its 27 keys the
template omitted (read in src/vegas_mode/config.py). plugin_system gains
development_mode, which the web UI reads and writes but the template
never declared.
Every added value is byte-identical to the code-side .get() fallback, so
ConfigManager._migrate_config() merging these keys into existing user
configs cannot change behavior on any installed device.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* fix(scripts): repair broken sys.path setup in utility scripts
clear_cache.py and download_nba_logos.py pointed sys.path at a 'src'
directory relative to the script's own folder (scripts/utils/src and
scripts/src — neither exists), so both crashed on import; they now insert
the project root and import via the src package like the other scripts.
debug_web_manual.py resolved 'project root' to scripts/debug/ instead of
two levels up. fix_nhl_cache.sh is removed: it used Python docstring
syntax in a bash script and invoked clear_nhl_cache.py, which does not
exist anywhere in the repo — it cannot ever have worked in its current
location.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* docs: correct stale file:line references and the loader-fallback contradiction
CLAUDE.md and .cursorrules disagreed about plugin-directory fallback
behavior; the code (SchemaManager.get_schema_path) probes plugins/
BEFORE plugin-repos/, and the main discovery path has no fallback at
all — both files now describe the real behavior, preferring symbol names
over line numbers so the references rot slower. REST_API_REFERENCE.md
pointed at app.py:144/:607 for mounts that live at :199/:799 and counted
92 routes where there are 94. PLUGIN_ARCHITECTURE_SPEC.md's historical
banner gains a note that its example imports
(src/plugin_system/base_classes/*_plugin.py) never shipped — the real
base classes are src.base_classes.sports.SportsCore and
src.base_classes.hockey.Hockey.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* docs: fix broken links, phantom script references, and stale CI description
Repairs every broken relative link in active docs (targets renamed or
archived long ago: PLUGIN_DEVELOPMENT.md -> PLUGIN_DEVELOPMENT_GUIDE.md,
API_REFERENCE.md -> REST_API_REFERENCE.md, PLUGIN_STORE_USER_GUIDE.md ->
PLUGIN_STORE_GUIDE.md, plugin_docs/ dir, TROUBLESHOOTING_QUICK_START.md,
and MIGRATION_GUIDE's README link that silently resolved to the docs
index instead of the project README). Replaces commands invoking scripts
that do not exist (scripts/update_stats.py, validate_registry.py,
check_updates.py, fix_permissions.sh) with the real tooling, and
rewrites HOW_TO_RUN_TESTS.md's CI section, which described a
security-audit workflow that was never committed and a pytest workflow
'queued to land' that landed long ago as test.yml.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* docs: complete the docs index and refresh the web interface file tree
docs/README.md's own policy says every page must be linked from the
index, yet five weren't — including the entire skin system
(SKIN_SYSTEM.md, CREATING_SKINS.md), ADAPTIVE_LAYOUT.md,
plugin-safety-harness.md and SPORTS_UNIFICATION.md. Each is now listed
in the section it belongs to, and PLUGIN_ARCHITECTURE_SPEC.md is marked
historical in the index (the doc itself already carries the banner).
web_interface/README.md's static/v3 tree showed only app.css/app.js;
it now reflects the actual contents.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* chore: remove dead modules confirmed unused in-repo and across all store plugins
- src/common/cli.py: imports a 'ledmatrix_common' package that exists
nowhere (not in this repo, any requirements file, or the plugin
monorepo), so it cannot ever have run; its README section claimed
scripts/dev/* used it, which was also untrue.
- src/web_interface/logging_config.py: zero callers — the web app uses
web_interface/logging_config.py (a different module), and nothing
imports the src copy.
- handle_errors decorator in src/web_interface/error_handler.py: zero
call sites (the module's response helpers stay — they are used).
- ConfigManager.get_clock_config(): reads a 'clock' config key that no
longer exists anywhere; only caller was its own unit test.
Deliberately kept despite zero in-repo callers: DisplayError,
src/common/config_helper.py and display_helper.py — all documented as
plugin-facing API (docs/PLUGIN_ERROR_HANDLING.md, src/common/README.md),
and third-party plugins outside the official monorepo cannot be
enumerated. Verified against a fresh clone of ledmatrix-plugins (43
plugins): zero references to any removed symbol.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* chore: remove manager-era NBA test files and one-off debug scripts
The four test_nba_*.py files imported nba_managers, leaderboard_manager
and odds_manager — top-level modules deleted when sports displays became
plugins — inside try/except blocks that swallowed the ImportError, so
they passed while exercising nothing. test_nba_data_structure.py and
debug_nba_api.py (a diagnostic script living in test/) made live ESPN
API calls rather than testing repo code. None were enrolled in CI.
scripts/debug/direct_fix_imports.py and check_imports.py were one-shot
artifacts that edited/inspected a hardcoded ~/LEDMatrix/web_interface/
app.py to fix an import problem solved long ago; nothing references
them.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* chore: remove generate_report.py, which aggregates artifacts of CI jobs that do not exist
The script's only function is to merge JSON artifacts
(bandit/semgrep/pip-audit/safety/gitleaks results) produced by a
security-audit workflow that was never committed —
.github/workflows/ has no such jobs, so there is nothing for it to
aggregate and no way to run it usefully. Its siblings stay:
prove_security.py and audit_plugins.py both run standalone (verified),
and .codacy.yml stays because the Codacy service (README badge) reads it
server-side without a workflow file.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* fix(web): load the three widget scripts store plugins already declare
time-picker.js, file-upload-single.js and plugin-file-manager.js
register widgets that installed store plugins reference in their config
schemas (countdown uses x-widget: time-picker and file-upload-single;
of-the-day uses plugin-file-manager), but base.html never included the
scripts. plugin_config.html renders such fields as an empty container
that polls LEDMatrixWidgets.get(...) on a 50ms loop forever, so those
plugin config fields appeared permanently blank. The audit initially
flagged these files as dead code; the monorepo cross-check proved the
opposite — they were unreachable, not unused.
example-color-picker.js (the documented custom-widget example) gains an
explicit warning that including it in base.html would shadow the
built-in color-picker widget.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* chore: drop the legacy youtube block from the secrets template
No code reads a top-level youtube secrets key: the youtube-stats plugin
receives its API key namespaced under its own plugin id (declared via
x-secret in its config schema), like every other store plugin. The key
survives only in state_reconciliation.py's non-plugin-key exclusion set,
which stays — existing installs still carry the key in their generated
config_secrets.json, and the exclusion prevents it from being
misclassified as a plugin config. New installs simply stop being asked
for a YouTube API key they have nowhere to use.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* chore(deps): remove packages nothing imports, declare direct imports, move mypy to test deps
Removed from requirements.txt: python-socketio, python-engineio,
websockets, websocket-client — zero imports anywhere in this repo, and
the one store plugin that needs Socket.IO (ledmatrix-music) declares it
in its own requirements.txt, which the plugin store installs. Removed
the same quartet plus timezonefinder, geopy, google-auth-oauthlib,
google-auth-httplib2, google-api-python-client, unidecode, icalevents,
python-dateutil, flask-wtf and the werkzeug pin from
web_interface/requirements.txt — all leftovers from the deleted built-in
weather/calendar/music displays (flask-wtf was doubly dead: app.py
explicitly disables CSRF and sets csrf=None). scripts/
install_dependencies_apt.py, which mirrors these lists for the
first-time installer, drops the same packages.
Added: urllib3 (imported directly in four core modules), jinja2 and
markupsafe (imported directly in pages_v3.py) — previously reachable
only as transitives. mypy moves from runtime requirements to
requirements-test.txt.
Verified in a fresh venv: all four requirements files co-install, pip
check is clean, the full CI-enrolled suite (907 tests) and a Flask boot
smoke pass with the trimmed dependency set.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* refactor: single canonical DateTimeEncoder
src/cache_manager.py and src/cache/disk_cache.py each defined an
identical DateTimeEncoder (datetime -> ISO-8601). The disk_cache copy is
the only one actually used for serialization; cache_manager now
re-exports it instead of defining a twin, so the two can never silently
diverge. Import compatibility is preserved — from src.cache_manager
import DateTimeEncoder still works and is the same class object.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* docs(code): document deliberate duplicates instead of merging them
The audit surfaced several near-duplicate implementations that turned
out to be either deliberate forks or behaviorally different — merging
any of them would risk changing behavior on installed devices, so each
now carries an explicit comment stating the relationship:
- VisualDisplayManager: headless fork of DisplayManager; header now
lists the ~15 mirrored methods and warns that DisplayManager changes
must be mirrored.
- normalize_abbreviation: LogoDownloader's version (called directly by
nine scoreboard plugins) replaces filesystem-unsafe characters;
LogoHelper's strips spaces. Logo filenames on existing installs
depend on both behaviors staying put.
- The two PluginTestBase classes: the shipped one is plugin-author
API, the repo's own richer harness lives in test/plugins/ — now
cross-referenced.
Also verified (no change needed): ConfigManager's backup/rollback
methods genuinely delegate to AtomicConfigManager, and SportsCore
already delegates _read_bdf_native_size to FontManager.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* docs: add a unified configuration reference
There was no single place documenting what lives in config.json —
display.* keys were scattered across README sections, vegas_scroll lived
in ADVANCED_FEATURES.md, and dim_schedule, display.double_sided,
sync.follower_position, plugin_system.development_mode and the four
newly-templated hardware keys were documented nowhere. CONFIG_REFERENCE.md
now lists every template key plus the code-read-only keys, each with
type, default, and the code location that reads it, and explains the
secrets file's plugin-id namespacing. Linked from the docs index.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* docs: bring the README's feature tour into the plugin era
The Core Features section still presented clock/weather/sports/stocks/
music displays as built into the project, when all of them are store
plugins installed from the ledmatrix-plugins monorepo — only
starlark-apps and web-ui-info ship in this repo. The intro now says so
(the showcase itself is unchanged; those are real displays available in
the store). The display_durations reference drops its built-in-calendar
example in favor of plugin-id keys, and the Configuration section links
the new CONFIG_REFERENCE.md.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* docs: archive the custom-icons status report, cross-link config docs, document assets/
PLUGIN_CUSTOM_ICONS_FEATURE.md was a 'What Was Implemented' status
report duplicating the actual guide (PLUGIN_CUSTOM_ICONS.md) — moved to
docs/archive/ per the docs index's own policy. The overlapping
plugin-config docs keep their content but PLUGIN_CONFIG_ARCHITECTURE.md
now states up front which doc is canonical for which purpose.
assets/README.md is new and load-bearing: assets/stocks, weather,
news_logos and broadcast_logos have zero references in this repo's code,
which makes them look deletable — but store plugins (ledmatrix-stocks,
ledmatrix-weather, news, odds-ticker) resolve those exact paths at
runtime against the install directory. The README records that evidence
so a future cleanup doesn't break installed plugins.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* test: add regression guards for the bug classes fixed in this PR
Three lightweight static checks, all enrolled in CI's unit-test
allowlist along with the new web-cache test:
- test_template_targets.py: every literal render_template() target must
exist (would have caught the weather/stocks partial 500s at commit
time).
- test_widget_scripts.py: every widget JS file must be script-included
in base.html or explicitly allowlisted with a reason (would have
caught the unloaded time-picker/file-upload-single/plugin-file-manager
widgets), and allowlisted files must NOT be included (prevents the
example widget from shadowing the real color-picker).
- test_doc_links.py: relative markdown links in active docs must
resolve (docs/archive/ exempt).
Each guard was verified to fail against the pre-PR tree and pass now.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
* fix(deps): restore the werkzeug version floor
Commit
|
||
|
ChuckandClaude Opus 5
|
2af41c561b |
fix(plugins): make update scheduling atomic so update() cannot run twice at once (#437)
* fix(plugins): make update scheduling atomic so update() cannot run twice at once Closes #401. `run_scheduled_updates()` decided whether to update a plugin with a check-then-act sequence: `can_execute()` and `set_state(RUNNING)` were separate calls with nothing between them, so two scheduler threads could both observe ENABLED and both go on to call the same plugin's `update()`. `update_all_plugins()` had the identical pattern. Two schedulers really do run at once. The render loop calls `_tick_plugin_updates()`, and Vegas mode fires its own `vegas-plugin-tick` daemon thread that is never joined when `VegasModeCoordinator.play()` returns — a slow `update()` still in flight overlaps the next tick from the main loop. A plugin running `update()` twice concurrently is unsafe unless it happens to be reentrant; shared mutable state, a non-thread-safe HTTP session or cache all break. The async path was already covered by the `_pending_lock` dedup in `_enqueue_update`, so the live exposure was the synchronous kill-switch path and `update_all_plugins()`. Both now claim the plugin through `_reserve_for_update()`, which holds one lock across the eligibility check, the due-time check and the RUNNING transition — and nothing more. Holding it across `execute_update()` would serialize slow plugins behind each other and reintroduce the render stall the async worker exists to avoid. The due-time check moved inside the lock deliberately. Left outside, a thread that had already decided "due" could claim the plugin the instant the winner finished, running `update()` twice within one interval. Two supporting changes fall out of it: - `_enqueue_update()` no longer sets RUNNING (the reservation did), and hands the reservation back if the pending-dedup ever fires. Otherwise a reserved-but-unqueued plugin would sit in RUNNING with nothing left to release it, and `can_execute()` would refuse it forever. - `_finish()` now clears the pending entry *before* flipping the state back to ENABLED. The old order left a window where a scheduler saw ENABLED, reserved the plugin, then had its enqueue silently dropped by the dedup — harmless as a missed tick before, a stuck plugin once a reservation is involved. Regression suite added and enrolled in CI, along with test_async_plugin_updates.py which was not previously run there. The overlap tests delay `can_execute()` to hold every thread inside the check-then-act gap: the real window is a couple of bytecodes wide, so a plain hammering test passes against the unfixed scheduler and proves nothing. With that delay the suite reports `update() ran 8x concurrently` on both affected paths before the fix, and passes after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * test: fix a race in the reservation suite's own wait loops `test_async_path_never_overlaps` failed in CI with "update() ran 0x concurrently" — the test's bug, not the scheduler's. It polled `plugin._active` to wait for the update to finish, but before the worker picks the item up nothing is active yet, so the loop fell straight through and asserted on a plugin that had never run. Both async waits now key on `update_calls >= 1` as well, so they wait for an update to have started *and* finished. The stranded-state test gets the same guard for a second reason: ENABLED is also the starting state, so without it that assertion passes vacuously on a plugin that was never scheduled. Verified over 12 consecutive local runs, 12 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(plugins): roll back the claim when dispatch fails _enqueue_update() reserved the plugin and added it to the pending set, then started the worker and queued the item. Thread.start() raises RuntimeError when the OS refuses a new thread — not hypothetical on a Pi under memory or thread pressure — and nothing is queued at that point to release the plugin. It stayed RUNNING with a stale pending entry, so can_execute() refused it for the rest of the process, and the exception escaped run_scheduled_updates() and skipped every remaining plugin in that tick. That is the same stranded-RUNNING failure the reservation was introduced to prevent, just reached through the dispatch rather than the dedup, so it is handled the same way: discard the pending entry, hand the reservation back, log the cause. Swallowed rather than raised so one plugin failing to queue cannot abort the others' turn. Both new tests fail against the un-rolled-back version — the second on the escaping RuntimeError itself — and pass with it. 74 tests across the reservation, async-update, plugin-system, health, Vegas-adapter and controller-toggle suites still pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WexvwNDtWLVymGVqKD7BGk --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
d305be6089 |
fix(plugins): core's own config keys no longer flag plugins as degraded (#434)
Found while sweeping devpi for issues. Nine of 27 installed plugins were reported degraded in the web UI -- including baseball-scoreboard and f1-scoreboard -- for using a documented core feature. The core reads three tuning keys out of each plugin's own config block: vegas_width_pct and vegas_overflow (vegas_mode/plugin_adapter.py) and vegas_max_width_screens (base_plugin.py). No plugin declares them, and 37 of the 42 published config schemas set "additionalProperties": false -- so schema validation reported them as violations. That is not just log noise. _validate_config_schema_soft sets `degraded` in the health tracker, which the web UI surfaces, so a user who tuned a core Vegas setting saw the plugin marked broken. The keys are stripped before validation. Fixing it plugin-side would mean 42 schema edits and 42 version bumps -- 42 store updates for a contract the core owns. Listed explicitly rather than matched on a `vegas_` prefix: vegas_mode is the opposite case, plugin-owned and declared in schemas, and a prefix rule would silently stop validating it. Verified on devpi: degraded went 9 of 27 -> 0 of 27, schema-mismatch warnings 9 -> 0, 22 plugins still load, no tracebacks. 800 core unit tests pass, 8 of them new -- including that a genuine violation is still reported, so the check has not been turned into a no-op, and that the caller's live config dict is never mutated. Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
53af53b4a1 |
feat(store): evaluate compatible_versions, not just the floor (#433)
* feat(store): evaluate compatible_versions, not just the floor Closes the gap CodeRabbit surfaced on #427. `compatible_versions` is the canonical compatibility contract -- schema/manifest_schema.json marks it required, all 42 published manifests carry it -- and it is the only field that can express an *upper* bound. `ledmatrix_min_version` is a floor and cannot say "not compatible with 4.x". The gate read only the floor, so a plugin declaring ["2.0.0 - 2.9.9"] would be installed on 3.2.0 regardless of having said it stops at 2.x. check() now evaluates both and the more restrictive wins. The array is a set of alternatives (satisfying any one entry suffices), supporting every form the schema permits: >=, <=, >, <, ~, ^, a bare exact version, and an inclusive "A - B" range, with prerelease/build suffixes tolerated. Refusal still requires evidence. Anything unparseable, absent, or below TRUSTWORTHY_FLOOR resolves to compatible. That last point needed a new strict parser. parse_semver is deliberately lenient -- it strips non-digits and yields (0, 0, 0) for a string with no numbers at all. Harmless for a floor (0.0.0 never blocks) but wrong for a range, where the same leniency turned an unreadable spec into a *refusal*: a manifest whose only entry was garbage got compared against 0.0.0 and refused. Range specs are now shape-checked first, so garbage reads as "no evidence". parse_semver itself is unchanged, since the loader depends on its behaviour. Verified: 815 core unit tests pass, 18 of them new. Swept the real registry -- all 42 published manifests, at cores 1.0.0 / 2.0.0 / 3.1.0 / 3.2.0 / 4.0.0 -- and nothing is refused at any of them. The gate stays inert for shipped plugins, which is the property that makes it safe to land ahead of B5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * feat(store): protect the one population the sunset would break The B6 sunset deletes each plugin's guarded-import fallback, so a plugin that floors at 3.2.0 must never reach a core that lacks the 3.2.0 modules. The gate could not stop that for the population most at risk. A device installed from the v3.1.0 release reports __version__ = "1.0.0". The gate treated anything below TRUSTWORTHY_FLOOR as "unknown, do not block" -- correct while every manifest floors at 2.0.0, because blocking would have emptied the plugin store for those users. But after the sunset it hands them a 3.2.0-floored plugin with no fallback, which fails to load with one log line. Nothing else in the system protects them: they cannot be told apart from a genuine 1.0.0 install. On an untrustworthy core the gate now refuses a floor ABOVE 2.0.0 and still allows anything at or below it. A floor above the ecosystem baseline says the plugin needs modules that arrived after 2.0.0, and a core reporting below that -- whether it is the v3.1.0 release or something genuinely ancient -- will not have them. Refusing leaves the user on the version they already run instead of one that cannot load. Measured against all 42 published manifests: today (every manifest floors at 2.0.0) core 1.0.0 / 2.0.0 / 3.1.0 / 3.2.0 / unparseable -> 0 of 42 refused after B6 (same manifests floored at 3.2.0) core 1.0.0 -> 38 refused, core 3.1.0 -> 38 refused, core 3.2.0 -> 0 So nobody loses the store today, and the sunset cannot reach a core that cannot run it. Two older tests asserted the previous "allow everything" behaviour; they now express the new rule with a 2.0.0 floor, which is what their no-lockout intent was actually about. The 38-of-42 in that measurement surfaced a separate B6 trap, recorded here because it will bite whoever raises the floors: four plugins (flights, leaderboard, music, stocks) declare the floor as a TOP-LEVEL `min_ledmatrix_version`, a third spelling, which declared_min_version checks before the versions[] array. For those, editing versions[0] is a silent no-op and the floor stays at 2.0.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(store): address review — malformed manifests, and suffixed versions Both CodeRabbit findings on #433 verified against the code and fixed. 1. Malformed manifest sections raised instead of degrading. `requires` as a list hit AttributeError ('list' object has no attribute 'get') and `versions` as a mapping hit KeyError: 0. Both reproduced. This got worse with the sunset rule in the previous commit: that branch resolves the floor for *every* manifest on an untrustworthy core, where the old code returned early. One hand-edited or third-party file with the wrong shape would have taken down the whole install path rather than just itself. Container types are now validated and an unrecognised shape reads as "no declared floor". 2. Prerelease and build metadata leaked into the version numbers. The digit scrape parsed "3.2.0+build42" as (3, 2, 42) and "3.2.0-rc1" as (3, 2, 1) -- a release candidate ranking above its own release. Both fed reject decisions, and the consequence was demonstrable: a plugin pinned to exactly "3.2.0" refused a core running 3.2.0+build42, which is that same version. The suggested remedy -- use the strict token parser -- would not have fixed it. _parse_strict validates the shape but delegates the numbers to parse_semver, so it returned the same (3, 2, 42). The bug is in the scrape, so suffixes are now dropped before it. Prereleases compare equal to their release rather than below it; full prerelease ordering is more than any caller needs and equal is far closer to right than what it did before. parse_semver is shared with PluginLoader, so its suite was re-run: unchanged, and it only ever gets more correct here. Verified: 839 core unit tests pass, 21 of them new -- six malformed shapes, five suffixed forms, and the two demonstrated regressions. The real-registry sweep is unchanged at 0 of 42 refused across cores 1.0.0, 3.1.0, 3.2.0, 3.2.0+build42 and an unparseable string. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
970ca2d04f |
feat(store): refuse to install a plugin that needs a newer core (re-target of #429) (#431)
* feat(store): refuse to install a plugin that needs a newer core `ledmatrix_min_version` was decoration. The loader logged an advisory warning and continued; the store never compared the core version at all, so a routine "update" delivered a plugin that could not run. That is the gap phase B6 (the sports-unification sunset) cannot be done over: deleting a plugin's bundled fallback while nothing enforces the floor hands un-updated users a scoreboard that raises ModuleNotFoundError at load and is reported only as one line in the journal. The gate lives in install_plugin, after the manifest is on disk and before dependencies are installed. That is the earliest knowable point -- the registry carries no compatibility field, so the floor is not visible until the files are down -- and it is also the chokepoint: _reinstall_with_rollback calls install_plugin, so a refused *update* restores the version the user already had, for free. Floor resolution and the comparison move to src/plugin_system/compatibility.py, shared with the loader so the two cannot drift. Both read all four spellings published manifests use, including the deprecated `ledmatrix_min`. Refusal requires evidence. An undeclared floor, an unparseable version on either side, or a core below TRUSTWORTHY_FLOOR (2.0.0) all allow the install. That last one is deliberate and load-bearing: the v3.1.0 release reports __version__ = "1.0.0" while nearly every published manifest floors at 2.0.0, so a strict gate would lock those users out of the plugin store entirely -- much worse than the problem being solved. They stay unprotected until they update the core, which is also what fixes their version string. Verified: 782 core unit tests pass, including 25 new ones and the existing loader-warning suite unchanged (the refactor is behavior-preserving). The install tests drive the real install_plugin path with the download stubbed -- the allow and refuse cases differ only in the declared floor, so the refusal is demonstrably the gate and not an earlier bail-out. Follow-ups, deliberately not in this PR: surfacing the reason in the store UI rather than only the log, and publishing the floor in plugins.json so the store can refuse before downloading. Phase B4 in docs/SPORTS_UNIFICATION.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(store): a failed install must not destroy the plugin it replaced Found while validating the compatibility gate. `_install_plugin_impl` deletes the existing plugin directory *before* downloading, so any failure after that point leaves the user with nothing. `_reinstall_with_rollback` protects the update path exactly this way; a direct `install_plugin` had no equivalent. The gate made this reachable in a new way: a plugin whose declared floor exceeds the running core is now refused *after* the old copy is already gone. Floors are hand-written and can be over-declared, so the refusal could remove a plugin that had been working fine on that core. install_plugin is now a thin wrapper that renames any existing install aside, delegates to _install_plugin_impl, and restores it on failure -- including when the implementation raises, which is re-raised after the restore. It is a pass-through when nothing is installed and when called from _reinstall_with_rollback, which has already moved the old copy aside; a test pins that so the two mechanisms cannot start nesting. The aside name embeds '.standalone-backup-' because plugin_manager._scan_directory_for_plugins keys on exactly that substring to skip backups. A different name would have made the backup discoverable as a duplicate plugin; a test pins that too. 789 core unit tests pass, including 7 new ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(store): serialize concurrent installs, and make the lock reentrant Second bug found while validating the previous commit on hardware. install_plugin's new set-aside/restore had no lock. The web UI runs Flask threaded, so a double-clicked Install button gives two threads the same plugin_id; interleaved, one thread's restore deletes the other's freshly installed copy. _reinstall_with_rollback already guards exactly this with a per-plugin lock, and install_plugin needs the same one. Taking that lock naively deadlocks. _reinstall_with_rollback holds it across its call to install_plugin, and threading.Lock is not reentrant -- so the request thread hangs forever on the standard monorepo update path (update_plugin -> _reinstall_with_rollback -> install_plugin), which is to say on every plugin update. Verified by reverting to a plain Lock: the regression test times out after 10s instead of passing. The per-plugin locks are now RLocks, and install_plugin holds one for its whole set-aside/install/restore sequence. Verified on devpi (Pi, Python 3.13.5, real registry and network): - update_plugin on an up-to-date plugin: True in 5.4s - update_plugin forced through the full reinstall-with-rollback path: True in 13.1s, correct version restored, old copy replaced, no backup directories left behind - install -> reinstall-over-existing -> failed-reinstall-restores: all pass against real downloads - 22 plugins load, no tracebacks, web API and UI 200, steady-state journal 50 lines/min 791 core unit tests pass, including 2 new concurrency tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * ci: run the new suites, and check tag/version agreement at release time These were split out of #428/#429 because the token pushing them lacked the `workflow` scope. Folding them in here rather than opening a stacked PR -- #429 was merged into its stacked base after that base had already been squash-merged, so its content never reached main, and one such near-miss is enough. All three enrolled suites exist on this branch: test_version_consistency.py came with #428 and is on main; the other two arrive with the commits above. Enrolling them in a separate PR would have either raced with this one on test.yml or briefly pointed CI at files main did not have. - test.yml: enroll test_version_consistency, test_plugin_compatibility_gate and test_install_preserves_existing in the core unit job. Until now these 32 tests existed but nothing ran them automatically. - release-version-check.yml: run scripts/check_release_version.py on pushed v* tags and published releases, plus workflow_dispatch so a tag can be checked *before* it is created. No dependencies -- it reads src/__init__.py and CHANGELOG.md only. Verified: both workflow files parse, and the release check still passes for v3.2.0 against this tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Fable 5
|
82a65ad2a2 |
Sports unification phase 0: safety net, cwd-independent fonts, element_style (#425)
* fix(fonts): resolve asset paths against the install root, not the cwd
FontManager built its catalog from cwd-relative paths ('assets/fonts'),
so any process started outside the install root — the plugin safety
harness on CI being the recurring case — found no fonts and silently
degraded every plugin to PIL's default face. Several plugins grew
per-plugin workarounds for exactly this (countdown, text-display,
tide-display in the plugins monorepo).
Catalog population now falls back to the install root derived from this
module's location when the cwd-relative path is missing; behavior when
running from the install root is unchanged. Verified: resolve_font
returns the real FreeType face from a foreign cwd, and the full unit
suites (266 tests) pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
* docs: seed CHANGELOG.md with the module-availability release discipline
The plugins monorepo's sunset rule ('delete a bundled fallback copy only
when the manifest floors on the first core release shipping the module')
needs core module additions recorded against version numbers. Seeds the
changelog at 3.1.0 and documents the discipline.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
* ci: enroll the core unit suites in a dedicated job
The existing workflow ran only the three plugin-harness suites; the
skin-system, font-manager, data-source, extractor, scroll-helper,
adaptive-layout, and loader-compat suites (266 tests) existed but never
ran in CI, so a refactor of src/base_classes or src/common could regress
them silently. Also enrolls the new sports characterization and
element-style suites landing in this branch.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
* feat: ship src/element_style — the per-element style resolver plugins already expect
Three plugins (of-the-day, ledmatrix-music, football-scoreboard) import
src.element_style behind guarded try/except with classic fallbacks, but
the module never existed in core, so the richer per-element styling UI
those code paths implement has been dormant. This lands it:
- ElementStyleResolver.style() resolves per-element font/size/color with
the key semantic the consumers encode: a config value counts as
user-forced only when it differs from the schema default (the web UI
bakes defaults into config.json on save), and untouched configs
resolve to exactly the caller's classic values — byte-identical
rendering, proven by of-the-day's committed goldens passing unchanged.
- defaults_from_schema_file parses both declaration forms (the compact
x-style-elements map and hand-written customization blocks).
- expand_style_elements() expands x-style-elements into full config
blocks; schema_manager.load_schema() applies it (guarded, no-op for
schemas without the declaration) so the config form and defaults
merging see the expanded UI.
- Fonts resolve cwd-independently with (path, size) caching; .bdf loads
via freetype like FontManager; nothing in the module raises out of
style().
Verified: 31 new unit tests; of-the-day's previously-skipped 9-test
spec suite now runs and passes; football's resolver tests pass (27);
music's 38 plugin tests pass; schema-manager suites pass (43).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
* test: characterization suite for src/base_classes/sports.py ahead of unification
Pins current behavior before the planned merge of the nine drifted
plugin copies back into this ancestor: the _extract_game_details_common
key contract per sport (reusing GUARANTEED_KEYS from the skin tests),
update() flows for upcoming/recent/live against cache-seeded fixtures
under frozen time, rendering smoke per mode class, and guard rails on
the skin-system seam.
Five surprising behaviors are pinned AS-IS and flagged in comments so
the merge changes them knowingly or not at all: is_upcoming also
matching status.type.name; hockey dropping events whose competitors
lack 'statistics'; baseball reading the event-level status for innings;
no past-date filter in upcoming; and favorites-only mode with an empty
favorites list showing nothing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
* ci: restrict the test workflow's GITHUB_TOKEN to contents:read
CodeQL flagged the new unit-tests job for running with the default
unrestricted token; the pre-existing job had the same exposure. Both
jobs only check out the repo and run pytest, so a workflow-level
contents:read is sufficient.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
* Address CodeRabbit review: font-name traversal + offline test guard
Two Minor findings from CodeRabbit's first review of this PR.
- resolve_font_path: reject relative font names carrying path components.
font_name comes from plugin config, which the web UI writes; a value like
"../../config/config.json" escaped assets/fonts/ after os.path.join and let
a config probe arbitrary paths for existence (disclosure unlikely, since
Pillow/freetype reject non-font files, but the probe is real). Relative
names must now be bare filenames (os.path.basename(name) == name); absolute
paths keep their existing isfile() gate. Test confirms the traversal
resolved the real config.json before the guard.
- build_manager fixture: patch requests.Session.get BEFORE constructing the
manager. Construction creates both SportsCore.session and the
ESPNDataSource.session; the old code only replaced manager.session after
the fact, leaving data_source.session real and able to reach the network on
an accidental fetch. Patching the class makes every session built in the
fixture offline.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
---------
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
ChuckandClaude Sonnet 5
|
83f20b64fe |
Give plugins access to device-wide config, and a global scroll frame rate (#424)
* Give plugins access to device-wide config, and a global scroll frame rate
The sports scoreboards read `getattr(self, 'global_config', {})` to find a
shared scroll frame rate, but nothing ever set that attribute: the loader
constructs plugins with only plugin_id/config/display_manager/cache_manager/
plugin_manager (plugin_loader.py:671), `global_config` appears nowhere in
src/, no plugin manager assigns it, and BasePlugin has no __getattr__ to
synthesize it. The lookup always returned {}, so the ten scroll_display.py
copies that thread target_fps through to ScrollHelper could never fire on any
core. There was also no global target_fps to find -- the only one in the
template is display.vegas_scroll.target_fps, which is Vegas-scoped.
Adds the missing half:
- `BasePlugin.global_config` resolves the full config via
plugin_manager.config_manager, then cache_manager.config_manager, then {}.
Same order the sports timezone helpers already use. Exceptions are swallowed
to debug so an unreadable config can never stop a plugin loading, and a
non-dict result is rejected rather than handed to callers that will .get()
it and feed the result to numeric code.
- A top-level `target_fps` (default 100), exposed on the General tab and
validated 30-200 on save to match ScrollHelper.set_target_fps -- which
clamps silently, so a rejected save reports a value that would otherwise
appear to save and then behave differently.
The property has a setter deliberately. news, stock-news, ledmatrix-stocks,
ledmatrix-elections, ledmatrix-leaderboard and nfl-draft all assign
`self.global_config = config.get('global', {})`; without a setter that raises
"property has no setter" and those six plugins stop loading. Reproduced, then
pinned with a test.
target_fps is also kept out of the `is_general_update` key list: that branch
treats a missing web_display_autostart as an unchecked box, so counting a
target_fps-only POST as a General save would silently switch autostart off.
Verified end to end: config.json -> BasePlugin.global_config ->
scroll_display's existing block -> ScrollHelper.target_fps 120 -> 100, with no
plugin-side change needed. Suite 1441 passed; the 4 failures
(test_display_dirty_tracking, test_web_api::test_get_system_status, two in
test_state_reconciliation) are pre-existing and reproduce identically on a
clean tree.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
* Address review findings on target_fps validation and config resolution
- Reject floats and bools before int() in the target_fps save path. A JSON
body can carry them, where int(90.5) silently stored 90 and true stored 1.
Form posts send strings, so '90.5' already failed in int().
- Assert the template's target_fps is 100, not merely an int, so the
documented default is actually pinned.
- Empty-config precedence: keeping the `and config` check deliberately, now
spelled out in the comment and covered by a test. Both managers default to
the same config/config.json, so falling through cannot pick up a different
file's settings; treating {} as an answer would instead return {} when the
first manager simply hasn't loaded yet, silently disabling every setting
read through the property -- the failure this property exists to fix.
Suite 1446 passed. The float-rejection test was checked to fail without the
guard.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
|
||
|
ChuckandClaude Sonnet 5
|
5b45f35888 |
Vegas mode: reclaim dead space and pace the rotation (#423)
* Vegas mode: reclaim dead space and pace the rotation On a wide panel Vegas mode spent much of its time showing black. At 50px/s on a 512px display, one display width of blank is 10.2 seconds, which makes several long-standing behaviours expensive: - ScrollHelper prepended a full display width of black as an "initial gap", charged once per cycle — 10.2s of black at the start of every rotation. - Plugins without get_vegas_content() are captured off a full-display canvas, so their blank margins entered the ticker too. Measured: of-the-day drew 35px of "No Data" on a 512px canvas (92% blank), youtube-stats 142px of content with 185px of black either side. Only the scroll_helper path had any trimming. - Cycle transitions deliberately pushed a blank frame and then recomposed synchronously: 84ms at best, 4.8s at worst, every millisecond of it black. - buffer_ahead doubled as the cycle size, so a 21-plugin install showed 3 plugins per cycle and took ~7 cycles to come around. - separator_width was applied between every image rather than at plugin boundaries, so a per-row ticker like the F1 scoreboard (116 images, which it renders 4px apart internally) got a 32px chasm between each row — and the width budget didn't count those gaps, so the plugin quietly occupied far more of the panel than intended. Changes: - src/vegas_mode/geometry.py: numpy column-ink primitives shared by the trimmer and the audit tool, so the number reported is the number acted on. A Python per-column loop over a 17,000px strip is far too slow for the render path. - PluginAdapter trims every content path, not just scroll_helper. Only outer edges are cropped: interior blank columns are the plugin's own layout (logo left, score right) and closing them would corrupt the design. A plugin on a non-black background is inherently unaffected. - ScrollHelper.create_scrolling_image takes an explicit lead_gap, still defaulting to display_width so the many standalone-ticker callers are unchanged. Vegas passes lead_in_width (default 0). - Cycle end holds the last rendered frame instead of blanking, turning the recompose into a brief freeze rather than the panel switching off. - plugins_per_cycle (default 6) is split from buffer_ahead, which goes back to being only a prefetch low-water mark. - max_plugin_width_ratio (default 3x display width) caps one plugin's share of a cycle. Overflow is deferred, not discarded: a rotation offset advances each fetch so later rows appear on subsequent cycles. Single oversized images are cropped at a blank column so the cut misses glyphs. - Composition groups images by plugin: rows are joined by intra_plugin_gap (default 8) and separator_width applies only between plugins. The width budget now counts those gaps. - Plugin data updates no longer run on the Vegas render path. All new settings are user-configurable in Display -> Vegas Scroll, including min/max cycle duration and dynamic duration, which previously existed in code but were reachable only by hand-editing config.json. Measured with scripts/dev/vegas_audit.py on a 512x64 panel: mean ink coverage 42.7% -> 69.4% fully blank 5.9% -> 0% reads as empty 13.6% -> 0% worst blank stretch 4.8s -> 0s full rotation 414s -> 123s plugins per cycle 3 -> 6 Note the metric choice: a "fully blank" scan (>=95% black viewport) reported only 0.4% and badly understated the problem, because two full-width segments with mid-canvas content never fully blank the viewport — they hold it at ~28%. window_coverage_stats grades every viewport position by how much ink it carries, which is what tracks perceived dead time. Known remaining: cycle transitions still freeze ~3.5s while the next cycle is fetched. Fixing that needs background prefetch, which is deferred because the fallback-capture path mutates the shared display_manager.image and racing it against the render loop risks torn frames. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * Drop unused Optional import from the vegas audit script Flagged by Codacy (F401). Any, Dict and List are all still used. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * Align Vegas API bounds with validate(), fix audit config plumbing Both from review feedback on #423. The web API's accepted ranges disagreed with VegasModeConfig.validate(), which is what actually gates Vegas starting: scroll_speed 1-100 -> 1-200 (a slider value of 150 returned 400) separator_width 0-500 -> 0-128 target_fps 1-200 -> 30-200 buffer_ahead 1-20 -> 1-5 The three loose ones were the dangerous direction: the value saved with a 200, then VegasModeCoordinator.start() failed validation with only a log line, so the ticker silently never ran. The UI already matched validate() in all four cases, so the API was the odd one out. test_vegas_api_bounds_match_validate parses the numeric_fields map out of api_v3 and asserts every bound against validate(), plus that validate() accepts both endpoints and rejects just outside them, so these cannot drift apart again. That test immediately caught a missing upper bound on min_plugin_width, now added — unbounded it would drop every segment and leave a blank ticker. Separately, vegas_audit.py constructed PluginAdapter without the config, so it fell back to VegasModeConfig() defaults and would report trimming and width-budget behaviour that differed from the user's config.json. It now passes the loaded config exactly as the coordinator does. This is the same class of drift the explicit lead_gap and grouping arguments already guard against. Output is unchanged on a rig whose config matches the defaults. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * Vegas mode: render plugins narrower, space rows by measured separation Trimming reclaims blank margins but cannot compact a layout that genuinely spans the display — a five-column forecast, a progress bar drawn at 100% width, a stat block with the panel's whole width between its elements. Those need the plugin to make different layout decisions, which means telling it the screen is narrower while it renders. DisplayManager.render_size() presents a smaller logical canvas for the duration of a Vegas content fetch, reusing the same _LogicalMatrix indirection double-sided mode already relies on so plugins see a consistent size from every accessor. Plugins that size themselves from matrix.width need no changes at all; one that wants to be explicit can read the new BasePlugin.get_vegas_render_width(). Width is a percentage so a single setting travels across panel sizes: vegas_scroll.render_width_pct globally, or vegas_width_pct in an individual plugin's config. Measured on a 512x64 panel with real data: ledmatrix-weather 1536px -> 576px (forecast becomes narrow cards) youtube-stats 353px -> 199px (2% blank left, so genuinely compact) geochron 453px -> 153px (ink density rises to 100%) ledmatrix-flights 950px -> 740px The youtube-stats figure is the clearest evidence the layout itself changed rather than being cropped: at full width the content had to be trimmed from 512px to 353px, whereas at 40% it arrives with almost no blank to reclaim. Row spacing is now measured rather than added. A flat gap gets it wrong in both directions at once — content drawn flush to its own edges ends up nearly touching (reported for recent sports scores, which sat 8px apart), while content already carrying wide margins gets pushed even further out. separation_gap() measures the blank each pair already has and adds only the shortfall, up to min_content_separation (default 24). intra_plugin_gap stays as a floor applied regardless. Two tests shipped in the previous commit encoded the old flat-gap arithmetic and are updated to the measured semantics, including one renamed to reflect that zero intra_plugin_gap alone no longer butts rows together. Also fixes a real bug found while testing: the harness display manager had no render_size(), and because the adapter catches broadly that surfaced as "no content" rather than an error, silently dropping five plugins. Added the context to VisualTestDisplayManager for parity, and _render_at() now degrades to a no-op on any display manager lacking it, so a third-party or older harness loses the narrowing rather than the content. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * Vegas mode: end cycles before the wrap, keep the width budget honest Three fixes, the first a regression from lead_in_width defaulting to 0. get_visible_portion wraps: once scroll_position + display_width passes the end of the strip it fills the right of the frame from the *head* of the same strip. So the final display_width of travel showed the cycle's first plugin re-entering on the right while its last plugin exited on the left, and the recompose that followed replaced both at once. On a 512px panel at 50px/s that was 10.2s of two plugins on screen at once, ending in a hard cut — reported as the ticker "switching mid-scroll" from F1 to news. That used to be invisible because the strip began with a full display_width of blank, so the wrapped-in region was black. Removing that blank (it was 10s of dead panel per cycle) exposed the wrap. Cycles now end one display width earlier, before any wrapped content appears, clamped for strips no wider than the display so they don't complete instantly and spin the recompose loop. Verified on hardware: a 3936px strip now completes at 68.5s, exactly (3936 - 512) / 50. Second, auto_trim=False also skipped the width budget, which is an unrelated concern — turning off margin cropping should not let one plugin hold the panel for minutes. Seen in the field: the F1 scoreboard contributed 116 images and 14,848px untouched, giving a 33,821px cycle (11 minutes of content). The budget now applies regardless of trimming; with it restored that cycle is 6,362px. Third, the budget accounted for row gaps using the flat intra_plugin_gap while the compositor had moved to measured separation, so it under-counted by up to (min_content_separation - intra_plugin_gap) per row and a many-row plugin overran its cap. Both now use the same separation_gap() rule, and a test asserts the composed block fits the budget end to end rather than trusting the two paths to agree. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * Fix IndexError in find_blank_cut when the cut lands on the image edge A cut position after the last column is legitimate — _crop_to_budget asks for min(start + budget, img.width), which equals the width whenever the remaining strip is shorter than the budget. find_blank_cut clamped target to width but then walked leftwards starting at target itself, so ink[width] raised IndexError. Caught on hardware: it killed the ledmatrix-stocks fetch, and because _fetch_plugin_content catches broadly that surfaced as the plugin silently contributing nothing for the cycle. Only reachable on the second or later pass of the rotating window over a single oversized image, which is why the existing tests missed it — they all exercised the first pass, where start is 0 and start + budget is comfortably inside the image. Added TestRotationAcrossMultipleCycles, which walks the window round several times and asserts content is never lost, plus direct coverage of find_blank_cut at and beyond the image edge. Both bounds now stop at width - 1 so neither direction can index past the end. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * Only cut oversized segments at real gaps between items The width-budget crop snapped to the nearest blank column, and in rendered text the gap between two characters is a single column. So a cut routinely landed inside a word: the cycle showed "Wednesda" and the orphaned "y" turned up as a lone floating letter in the next cycle, positioned after whatever plugin happened to precede it. Measured on the clock-simple segment to confirm: its blank runs are [1, 1, 1, 1, 1, 8, 8] — five single-column letter gaps, every one of which find_blank_cut would happily have chosen. Cuts now only land in a run of at least min_cut_gap blank columns (default 6), which excludes letter spacing while still finding the gaps plugins put between items (the stocks ticker uses 32px, baseball 48px). Where no boundary falls inside the budget the cut waits for the next one and overruns, because splitting an item is worse than a slightly long segment. Continuous content is treated differently on purpose: an image with no internal gaps is a map or a chart, where any column is as good as another, so it is still cut to the budget exactly. The gap rule protects discrete items; letting a solid image escape the cap in its name would be wrong. blank_runs() is vectorised — 48ms for a 17,000px strip, against seconds for a per-column Python loop. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * Hold capture_mode for every plugin render, not just narrowed ones The native content path only entered capture_mode when it was also narrowing the canvas, so at full width — which is every plugin without a vegas_width_pct override, i.e. most of them — a plugin calling update_display() while building its Vegas content wrote straight to the hardware. That is a visible flash mid-scroll, and it lines up with the flash reported at cycle transitions, when several plugins are fetched back to back. Suppression is now unconditional; the narrowing context stays separate because it is already a no-op at full width. Both contexts are reached through helpers that degrade to nullcontext when the display manager lacks them. That matters more than it looks: the adapter's handlers are deliberately broad, so an AttributeError from a missing context does not surface as an error — it surfaces as the plugin contributing nothing. Making the call unconditional without this turned 44 tests red for exactly that reason, all of them reporting lost content rather than the real cause. The test double now provides capture_mode and render_size too, so tests exercise the real contexts instead of silently taking the degraded path. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * Vegas mode: one continuous strip instead of swapping cycles A cycle used to be a discrete strip that got replaced: motion stopped, every pixel was substituted at once, and the next group started with the viewport already full. That is the freeze, the flash and the jump. The strip is now extended rather than replaced. ScrollHelper gains append_content(), which adds items on the right without touching scroll_position or total_distance_scrolled, so motion continues and the next group simply arrives from the right. Because completion is measured against total_scroll_width, extending also defers completion — there is no longer a cycle boundary to see. drop_scrolled_prefix() reclaims what has gone past, keeping the strip bounded however long Vegas runs (observed 5,000-11,000px against an unbounded strip otherwise). It shifts total_distance_scrolled and total_scroll_width together so the completion arithmetic is unchanged, and refuses to run while the viewport is wrapping: wrapping reads the head of the strip into the right of the frame, so trimming the head there would visibly change the picture. A test caught that. Groups are prepared off the render thread. The constraint is that the canvas and the matrix proxy are process-wide mutable state, so narrowing or capturing through them from another thread would corrupt the frame the render loop is pushing. get_content() therefore takes offscreen_only: the background thread uses only paths that avoid the canvas, and anything needing it is marked and picked up on the render thread. That puts the expensive work (native renders of leaderboard and baseball cards, seconds each) in the background and leaves the cheap work (display capture, 40-600ms) in the foreground. DisplayManager's capture flag is now thread-local. As a shared flag, a background capture would have suppressed the render loop's own frame pushes for its duration, freezing the panel precisely when the point was to avoid a freeze. Canvas-bound plugins are drained one at a time rather than as a batch: six at once held the render thread for 1.75s. Drains are also spaced by two seconds while the lookahead is healthy, since taking them back to back turns one long stall into a run of short ones. When the strip is genuinely running short the throttle is ignored, because content matters more than smoothness there. Measured on hardware: zero cycle-complete swaps, drains landing 2-4s apart, lookahead holding at 1,200-3,500px, no errors. Set continuous_scroll false to restore the swap behaviour; the old path is intact. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * Pace the Vegas frame loop adaptively: 31.5 -> 78.7 fps The loop slept a fixed frame_interval on top of however long the frame took, so at a measured 31.6ms per frame a flat 8ms of that was pure idle — a quarter of the budget spent not rendering. It now sleeps only the remainder of the budget. Measured on hardware: 31.5 fps to 78.7 fps sustained, with CPU going *down* from 150% to 127%. Scroll speed is unchanged at 49.9px/s against a configured 50, because motion is derived from elapsed time rather than frame count — this buys smoothness, not speed. Worth recording what the bottleneck was not: the per-frame render path measures 0.34ms in total (0.18ms for the numpy slice, 0.17ms for the dirty-tracking digest), which is a theoretical 2900 fps. Optimising any of that would have been wasted effort. The frame was idle, not busy. Also nices the prefetch thread. Its work is PIL and numpy that releases the GIL, so the scheduler can act on the priority, and without it the prefetch competes for the same cores as the render loop. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * Sub-pixel scrolling: motion at the frame rate, not the pixel rate With integer positioning the number of distinct frames per second equals the scroll speed in px/s, however fast the loop renders. Measured at 50px/s and 78.7fps, 36% of frames were byte-identical: the extra frames cost work and bought no motion, and what was left was 50 discrete 1px steps a second. Two things were wrong with the pre-existing sub-pixel support. get_visible_portion never consulted sub_pixel_scrolling — it always took the integer path, so the flag and _get_visible_portion_subpixel were dead code. And that implementation needed scipy.ndimage.shift, which is not installed on the target devices (HAS_SCIPY is False there), so it would not have interpolated even if reached. Verified both: positions 1000.0 and 1000.5 produced identical frames either way. Blending is now wired up and implemented with numpy. Two details make it affordable: slice cached_array directly instead of building two PIL images only to convert them straight back (the naive version measured 15x the integer path), and use fixed-point uint16 multiply-add rather than float32, which suits the Pi's cores and gives finer weighting than the panel can resolve. Result 0.939ms against 0.237ms — 0.70ms added per frame, a 1065fps ceiling. Measured on hardware: 81.2 fps with blending on, against 78.7 with it off, so no cost within noise — and every frame is now a distinct position rather than one in three being a repeat. The trade is a slight horizontal softening of text, since each frame blends two positions. Set smooth_scroll false for maximum crispness. Also benchmarked and cleared as non-issues: extending the strip costs 9.4ms on an 11,000px strip and trimming 2.5ms, both under one frame at this rate. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * Add overflow handling: keep ordered content whole instead of rotating a window The width budget split any oversized plugin by advancing a window each cycle. That is right for interchangeable items — news headlines, odds, stock prices — but wrong for ordered content: a league table showed ranks 1-6, then resumed at 7 two rotations later, which reads as out of order and out of context. Nobody needs rank 23 in a ticker; they need the top of the table, every time. overflow_mode chooses between them: rotate — advance a window each cycle so everything is seen eventually (unchanged default) truncate — always show the start and drop the rest, keeping ordered content coherent. Records no window state, so every pass starts at the top. Per-plugin vegas_overflow overrides the global setting, since one install has both kinds of plugin. Also adds per-plugin vegas_max_width_screens, so content that must stay whole can be given more room — or uncapped with 0 — without lifting the cap on every ticker. Applied on the test rig: f1-scoreboard and ledmatrix-leaderboard set to truncate, and baseball given 4.5 screens because it was showing 8 of 9 games when the whole slate needed only a little more room. Verified: F1 now reports "the first 10 of 116 ... the rest are not shown", baseball has dropped out of the budget log entirely, and stocks, odds-ticker and stock-news still rotate. Also corrects the crop log, which claimed "window advances next cycle" unconditionally and so misreported truncated crops. A test now pins the behaviour behind the message: truncate must leave no offset recorded. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * Stop Vegas mode showing last night's games as if they were live A game that was live in the evening was still being drawn as live the next morning. Two faults combined to freeze plugin visuals indefinitely. PR #291 added a call to plugin_adapter.invalidate_plugin_scroll_cache() so a plugin's own cached scroll image would be rebuilt from fresh data. That method was never implemented. hot_swap_content() wraps the call in a broad except, so every hot swap has raised AttributeError and been swallowed silently ever since — which is why the visuals it was meant to keep fresh never were. Continuous scrolling then removed the only path that reached it at all: should_recompose() and hot_swap_content() are called from the non-continuous branch of run_frame(), and continuous_scroll defaults to True. So on a default install the pending-update flags were set by the update tick, never consumed, and grew without bound. Together these froze content completely, because refetching is not enough on its own: the sports plugins' get_vegas_content() regenerates only "if the cache is empty", so take_next_group() kept receiving the same picture however often it asked. Fixed by: - Implementing invalidate_plugin_scroll_cache(). It covers both layouts — a helper directly on the plugin (stocks, news, odds-ticker) and one owned by a scroll-display manager (the sports scoreboards, which is the shape that produced this bug) — and clears cached_image and cached_array together, since the array is the image's numpy mirror. - Adding StreamManager.invalidate_pending_updates() and calling it from the continuous branch. It only drops the caches; the plugin recomposes when it next comes round in the rotation. process_updates() is wrong here: it refetches synchronously and merges into the active buffer that continuous mode bypasses, and hot_swap_content() rebuilds and repositions the whole strip, which is the freeze-and-jump this mode exists to avoid. Tests assert the fix rather than the implementation: 14 of the 17 new tests fail without it. Includes the wiring itself, since the regression was a call that was simply absent, and a check that the scroll position is untouched so this cannot regress into the swap's visible jump. All Vegas suites pass (355 tests). test_display_controller_vegas_tick.py still cannot be collected off-device for want of rgbmatrix, identically with and without this change. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * Fix two CodeRabbit-flagged test assertions in vegas density tests test_prepared_group_is_used_without_refetching had a tautological final assertion; now checks stream.calls directly. test_no_partial_letter_at_either_edge required both crop edges to be blank, but the left edge here is always the crop's start position with no lead-in gap in word_strip, so it legitimately carries ink — only the right edge is an actual cut and needs the check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
ChuckandClaude Fable 5
|
cdf03fb107 |
Add skin system: user-installable visual overlays for sports scoreboards (#419)
* Add skin system: user-installable visual overlays for sports scoreboards Skins restyle a scoreboard's live/recent/upcoming rendering while the host plugin keeps doing data fetching, scheduling, caching, live priority, and vegas mode — the anti-fork alternative for users who only want a different layout. - src/skin_system/: ScoreboardSkin API, SkinContext (canvas + adaptive layout + logo/font helpers), discovery/loading runtime with API major version gating and per-skin module namespacing - src/base_classes/sports.py: _render_game() seam at the three _draw_scorebug_layout call sites; skin-first with built-in fallback, 3-strikes session disable, slow-render warning; per-mode skin config - scripts/validate_skin.py: headless multi-mode/multi-size validator with bundled per-sport fixtures (no hardware or network needed) - skins/example-classic-baseball/: working reference skin - Web UI: served-schema Visual Skin dropdown (validation never enum-restricted, so uninstalled skins can't invalidate configs) and GET /api/v3/skins - Store: registry entries with type "skin" install to skins/ - docs/SKIN_SYSTEM.md (architecture), docs/CREATING_SKINS.md (author guide incl. Claude Code prompt), view-model contract locked by tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LrCusPasy1qeUN5anK3aA1 * Address review feedback on skin system - skin_runtime: cache the entry module so the 2nd/3rd load of the same skin (live/recent/upcoming hosts) doesn't re-execute it with unbound sibling aliases; rebind cached sibling modules to their bare names around entry import and restore prior bindings after; include per- manifest mtimes in the discovery cache fingerprint so in-place skin updates are picked up - sports.py: count render_skin_card exceptions toward the 3-strike session disable - store_manager: validate skin ids (pattern + resolved-path containment in skins/), reject registry/manifest id mismatches, and stage+validate downloads in a temp sibling before replacing an existing skin - schema_manager: leave the schema untouched when the configured skin value is a per-mode mapping (a string dropdown could overwrite it) - validate_skin.py: reject non-positive sizes and non-object --options at parse time; support --output-dir outside the repo; type annotations - example skin: validate accent_color once at load with logged fallback - fixtures: pregame 0-0 scores in football/hockey upcoming fixtures - api /skins: rely on the self-invalidating discovery cache instead of force_refresh - docs: valid JSON manifest example, load_logo caching semantics spelled out, language ids on fenced blocks - tests: view-model contract test now exercises the real extractor; regression test for repeated same-skin loads with sibling modules Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LrCusPasy1qeUN5anK3aA1 --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
ChuckandClaude
|
6a9d8014e5 |
Tag plugin logs structurally and surface the active plugin in System Logs (#418)
- get_logger() now returns a PluginLoggerAdapter when given a plugin_id,
so every plugin log call is stamped with plugin_id automatically instead
of only calls that explicitly passed extra={'plugin_id': ...}. This makes
the "[Plugin: x]" prefix reliable in the journalctl-backed log stream.
- display_controller publishes the currently active mode/plugin to the
shared cache whenever it changes, exposed via a new
GET /api/v3/display/current-status endpoint.
- System Logs page: adds a "Now showing" banner backed by that endpoint, a
plugin filter dropdown (populated from parsed log lines), a plugin badge
per log entry, and fixes log parsing to handle the short-iso timestamp
format journalctl actually returns (the old regex only matched syslog
timestamps, so level/plugin extraction silently never ran).
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
ChuckandClaude Sonnet 5
|
6499794c12 |
fix(testing): add MockCacheManager.get_cached_data_with_strategy/save_cache (#409)
* fix(testing): add MockCacheManager.get_cached_data_with_strategy/save_cache ledmatrix-leaderboard's data_fetcher.py calls these two real-CacheManager methods (src/cache_manager.py:313,817), but MockCacheManager had neither -- update() always hit an AttributeError, caught by a broad except and logged, so the harness rendered an empty-but-green leaderboard on every test run without ever exercising real standings data. Both mocks delegate to the existing get()/set() -- a mock doesn't need the real strategy's per-data-type max_age/market-hours timing, plugins under test just need the methods to exist and round-trip whatever was cached. * fix(testing): clear get_cached_data_with_strategy_calls in MockCacheManager.reset() reset() cleared get_calls/set_calls/delete_calls but not the newer get_cached_data_with_strategy_calls tracker, so a reused mock (e.g. across test cases sharing a fixture) retained stale strategy-call records after reset(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Chuck
|
3d347a368a |
fix(testing): stop plugin enabled:false schema defaults from silently disabling harness tests (#408)
check_plugin.py, render_plugin.py, and the pytest plugin matrix each built
config as {"enabled": True} then merged in config_schema.json's defaults on
top, letting a plugin's own enabled:false default (a reasonable choice for
a seasonal/opt-in plugin -- 15 of 23 real plugins ship one) silently win.
Every harness/CI render of those plugins was testing "disabled, do
nothing" rather than real behavior.
Extract build_full_config() into testing/loading.py (already the shared
home for plugin-discovery/config-default logic) and use it from all three
call sites: schema defaults, then a forced enabled=True, then harness.json's
config, then the caller's explicit config -- so a test can still
deliberately disable a plugin on purpose, it just can't happen by accident
via the plugin's own shipped schema default anymore.
|
||
|
|
0aca40cf3a |
perf(plugins): run scheduled updates off the render thread (#407)
* perf(plugins): run scheduled updates off the render thread plugin update() executed inline in the render loop — execute_update's internal thread.join(timeout=30) blocked it, so one slow plugin HTTP fetch froze scrolling for the whole fetch (up to 30s; DNS-retry storms made this a regular occurrence on flaky networks). Scheduling stays on the render thread and keeps every existing gate (enabled, circuit breaker, can_execute, interval); due updates are now enqueued to a single background worker (serialized — same one-at-a-time execution as before, no thundering herd). RUNNING is set at enqueue so can_execute blocks re-entry alongside the pending-set dedup. Per-plugin locks make the old implicit update/display no-overlap guarantee explicit: the worker holds the plugin's lock through its update; the display side try-locks and, when the plugin is mid-update, holds the last frame for that iteration — reported as success so a mid-update skip never advances the rotation. Unlike before, the guarantee now also holds across the post-timeout window (previously the lingering update thread overlapped display()). Deadlock-free by construction: the worker takes one lock; display never blocks. Timeout semantics unchanged (lingering daemon thread documented). Kill switch: plugin_system.synchronous_updates: true restores the inline path. 8 new concurrency tests (non-blocking scheduler, overlap assertion under a hammering display loop, lock release on failure/timeout paths, dedup, kill switch); 4-min devpi soak clean (updates completing, rotation advancing, no stuck RUNNING states). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam * fix(plugins): fix skipped-frame health/force_change tracking, config validation, and lock-lifetime gaps in async updates Addresses PR #407 review findings: - display_controller: only clear force_change / record health success when display() actually ran this frame, not when the frame was skipped because the plugin's lock was busy (a skip must preserve a pending mode-switch force_clear). - plugin_manager: replace bool() coercion of synchronous_updates with explicit isinstance validation of plugin_system/synchronous_updates, failing safe to synchronous mode (with a logged reason) on malformed config instead of silently defaulting to async. - plugin_manager: _update_worker_loop now acquires the plugin lock before looking up its instance and re-checks under the lock, so an unloaded plugin's lifecycle state is never resurrected to ENABLED. - plugin_manager + display_controller: move lock ownership (and, for updates, RUNNING/pending lifecycle bookkeeping) into the actual update()/ display() call itself rather than the timeout-wrapped caller, so the lock stays held for the real operation's duration even after PluginExecutor's own join(timeout) elapses and a lingering daemon thread keeps running in the background. - DisplayController.cleanup() now stops the update worker before tearing down display/cache resources; stop_update_worker() logs when the join times out instead of failing silently. - test_async_plugin_updates: rewrite test_unloaded_while_queued_is_harmless to exercise the public unload_plugin() lifecycle (via a deterministic blocker) instead of deleting pm.plugins directly, and add a regression test proving the plugin lock stays held through PluginExecutor's own timeout while the real update() call is still running. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ --------- Co-authored-by: Chuck <chuck@example.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
|
c1fa5094be |
fix(store): plugin updates keep the old install until the new one succeeds (#405)
* fix(store): plugin updates keep the old install until the new one succeeds Both reinstall paths in update_plugin — the monorepo-migration remote switch AND the routine archive update every store user hits — deleted the installed plugin directory BEFORE downloading its replacement. A mid-update failure (bad network, registry error) permanently destroyed the plugin. Seen in the field: a Pi with broken DNS lost 12 plugins in one update pass during the monorepo migration. New _reinstall_with_rollback: rename the old install aside (using the '.standalone-backup-' name pattern plugin discovery already excludes), run install_plugin, remove the aside on success — restore it on ANY failure, clearing partial-download debris first. A stale aside from a previous crash is cleared before starting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam * fix(store): serialize concurrent updates per plugin, check cleanup results CodeRabbit review on #405 flagged two things in _reinstall_with_rollback, both verified against current code: - Real race: the web UI runs Flask with threaded=True and there's a single update route, so two overlapping requests for the same plugin_id (double-click, two tabs) can interleave. The loser could rename the winner's in-progress install aside mid-download, deleting its own rollback safety net — worse than the bug this function exists to fix. Added a lazy per-plugin_id lock dict (mirrors the plugin_manager per-plugin lock pattern) held for the whole function. - _safe_remove_directory's return value was ignored at both call sites. Stale-aside cleanup failure now aborts cleanly instead of falling through to a rename that would fail anyway with a less useful error; post-success backup-removal failure now logs instead of failing silently (still returns True — the update itself succeeded, and the next update self-heals the leftover aside). Left the third nitpick (test_stale_aside_from_previous_crash_is_cleared) addressed by asserting the stale dir is actually gone and that install_plugin was reached, rather than just the end-to-end result. Added a concurrency regression test asserting install_plugin never runs for the same plugin_id while another call is in flight. --------- Co-authored-by: Chuck <chuck@example.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
ChuckandClaude
|
1c7a0cef66 |
fix(vegas): lock plugin_last_update snapshot/diff against concurrent mutation (#398)
* fix(vegas): restore live plugin-update refresh dropped by sync refactor Investigating a user report that Vegas scroll mode doesn't update scores or game status. Root cause: PR #299 (Mar 28) added a mechanism so a live score change reached the ticker within a few seconds instead of waiting for a full scroll cycle -- _tick_plugin_updates_for_vegas() diffed plugin_last_update timestamps to detect which plugins got fresh data and called coordinator.mark_plugin_updated() for each, and should_recompose() checked has_pending_updates_for_visible_segments() to trigger an immediate hot-swap. PR #330 (May 14, multi-display wireless sync) refactored both call sites while adding sync support and silently deleted this entire mechanism -- not just gated it behind the new sync-mode deferral it legitimately needed, but removed it outright. The result: VegasModeCoordinator. mark_plugin_updated() and StreamManager.has_pending_updates_for_visible_ segments() have been fully implemented but never called from anywhere since. Vegas mode's only remaining freshness sources are a 5s content cache TTL (fine) and full recompose at cycle boundaries, which depending on min/max_cycle_duration can be minutes away -- so live scores/status can sit stale far longer than a user would expect from a "live" ticker. Fix: - Restored _tick_plugin_updates_for_vegas() in display_controller.py, wired as the Vegas coordinator's update callback in place of the plain _tick_plugin_updates(). Diffs plugin_last_update before/after the tick and calls vegas_coordinator.mark_plugin_updated(plugin_id) for each plugin that actually got new data (rather than returning the list, since the callback interface no longer consumes a return value). - Restored the has_pending_updates_for_visible_segments() check in render_pipeline.should_recompose(), positioned after (not instead of) the sync-mode early return PR #330 added, so standalone installations regain immediate refresh while synced leader/follower pairs correctly keep deferring hot-swaps to cycle boundaries as PR #330 intended. Test plan: - Added test_display_controller_vegas_tick.py and test_vegas_render_pipeline_recompose.py -- neither area had any prior test coverage, which is very likely why this regression went unnoticed for ~2.5 months. - Verified both new test files fail against the pre-fix code (swapped in the current main versions of both files) with exactly the expected errors -- AttributeError for the deleted method, and the recompose assertion returning False instead of True -- then pass against the fix. - Confirmed the sync-mode deferral this restoration must not break still holds: test_sync_active_defers_pending_updates_to_cycle_boundary. - Full related suite (test_vegas_plugin_adapter, test_vegas_config, test_display_controller_plugin_toggle, test_display_controller_ optimizations, test_plugin_system): 108 passed, 1 pre-existing failure unrelated to this change (test_circuit_breaker, stale mock signature). - Full CI plugin-safety suite (test_harness, test_visual_rendering, test_plugin_matrix): 52 passed, 2 pre-existing skips. * fix(vegas): lock plugin_last_update snapshot/diff against concurrent mutation _tick_plugin_updates_for_vegas() snapshotted and later re-iterated plugin_manager.plugin_last_update from the Vegas background update-tick thread while the main render loop (or other callers) could mutate the same dict concurrently — a real race (unprotected dict iteration/mutation across threads), not just a style nit. Move the snapshot/update/diff into a new locked PluginManager.run_scheduled_updates_with_changes() so all reads and mutations of plugin_last_update happen under one lock, and update DisplayController to use it. The lock is only held around the dict accesses, not the update pass itself, so slow plugin update() calls don't serialize against other callers. Also add a regression test covering that the Vegas coordinator is wired to the Vegas-aware tick callback rather than the plain one. Skipped as not worth the change: - Narrowing the broad `except Exception` around vc.mark_plugin_updated(plugin_id) to specific types: it's a deliberate per-plugin isolation boundary (matches the same pattern used elsewhere in this file for plugin/coordinator calls) and there's no documented, stable set of exceptions that call can raise to narrow to. - Adding an inactive-DisplaySyncManager test to test_vegas_render_pipeline_recompose.py: verified VegasModeCoordinator.set_sync_manager() already normalizes a SyncRole.STANDALONE manager to None before handing it to the render pipeline (src/vegas_mode/coordinator.py:152-156), so should_recompose()'s `is not None` check is correct in practice; the suggested case is already covered by that normalization. --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
|
7f7f0d6464 |
feat: adaptive layout system — size-aware regions, crisp font ladders, image fitting (#393)
* feat(layout): adaptive layout & font scaling system for plugins Add src/adaptive_layout.py — opt-in core helpers so plugins render legibly on any panel size without hand-tuned per-display layouts: - Region: integer rect algebra (bands/columns/weighted splits/centering) that partitions space so text bands can't overlap by construction - Font ladders: ordered (family, size) steps known to render crisply (LADDER_GRID: X11 BDFs at native sizes; LADDER_ARCADE: PressStart2P at 8px multiples) — fitting walks the ladder instead of scaling pixel fonts fractionally - LayoutContext: breakpoint tiers, geometry scale vs. a declared design size, and cached fit_text/fit_lines/font_for_rows queries Generalizes the three patterns proven in the field: f1-scoreboard's scale factor, masters-tournament's tiers, baseball-scoreboard's font fallback ladder. Wiring: BasePlugin gains a lazy .layout property and draw_fit(); FontManager gains get_native_bdf_size() and a cache_generation counter; manifest schema gains display.design_size and requires.display_size max_width/max_height; 96x48 joins DEFAULT_TEST_SIZES; the bounds-check harness records negative-coordinate draws; TextHelper's broken measurement helpers are fixed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(layout): adaptive image fitting + composite region helpers Add src/adaptive_images.py — the image counterpart to fit_text: - fit_image(img, box, mode=contain|cover|fill_height|stretch, crop_to_ink, anchor, resample, upscale) promoting the proven plugin patterns (football's crop-to-ink fill-height logos, masters' cover crop + NEAREST flags, static-image's letterbox). Upscales by default — thumbnail()'s downscale-only behavior is why imagery stays tiny on big panels. - draw_fitted_image() pastes aligned within a Region with alpha mask. - One central Pillow>=9.1 RESAMPLE shim replacing ~15 plugin copies. LayoutContext.fit_image() caches results per (identity, box size, options) with a 64-entry LRU; id()-keyed entries pin the source image. BasePlugin.draw_image() is the one-liner adoption path beside draw_fit. Composites in adaptive_layout.py: Region.offset() (user x/y-offset passthrough), scoreboard_regions() (the two-logos-plus-score card math duplicated across six sports plugins, logo_slot = min(H, W//2)), and media_row() (art-left/text-right). Fix LogoHelper's size-blind cache key (stale sizes on panel change); deprecation note on dead image_utils.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(harness): scale-up fill check, config variants, multi-size dev gallery Quality gates for adaptive layout: - fill_metrics()/check_scale_up() in the safety harness: overflow catches content too big for a panel, but nothing caught content that stays tiny on panels >= 2x the plugin's declared design size. The check measures lit-content extents and warns (or fails, when a plugin opts into "fill_check": "strict" in test/harness.json) below 50% coverage on the doubled axis. Warn-only by default so no existing plugin breaks. - harness.json "variants": extra runs with config overlays and their own golden dirs, so an opt-in mode (e.g. layout_mode: adaptive) is golden- tested beside the classic default. check_plugin.py loops base + variants and labels variant results mode@name. - Dev preview server: GET /api/sizes (harness size sample), POST /api/render-matrix (render at up to 12 sizes in one call), size-preset dropdown, and an "All Sizes" side-by-side gallery in the preview UI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(plugins): adaptive-lib discoverability + advisory version compat warning Discoverability: re-export the adaptive layout/image API from src.common (the blessed-helpers package plugin authors already know) — canonical paths stay src.adaptive_layout / src.adaptive_images so nothing breaks. Document it in src/common/README.md and cross-link ADAPTIVE_LAYOUT.md from the developer docs authors actually read (quick reference, API reference, advanced dev, font manager, dev preview, plugin dev guide); ADAPTIVE_LAYOUT.md gains adaptive-images, composite-layouts and preserving-user-customization sections. Compat: PluginLoader now logs one advisory warning (never raises) when a plugin's manifest declares a min LEDMatrix version newer than the running core, checking the min_ledmatrix_version / requires.* / versions[] spellings found in the wild. Guarded against stale core version numbers. src/__init__.py __version__ bumped 1.0.0 -> 3.1.0 to match the latest release tag (v3.1.0) — it had never been updated and the compat check needs a truthful number. NOTE: verify this matches the intended release numbering before the next tag. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(layout): add measure_font_crispness — verify a ladder rung isn't blurry PIL antialiases TTF outlines by default; a 'pixel-style' font only rasterizes without antialiasing at specific sizes (for PressStart2P: exact multiples of its 8px design grid). A ladder rung at an unverified size silently renders blurry on an LED panel — this exact bug shipped in both text-display's and football-scoreboard's custom TTF ladders (non-8-multiple PressStart2P sizes, and '5by7.regular'/'4x6-font' at sizes that were never actually crisp). measure_font_crispness(font, sample_text) renders the sample and reports the fraction of ink-bbox pixels that are neither pure black nor pure white. BDF fonts (real bitmaps) always score 0.0; TTF ladders should be verified against this before shipping — see the new TestFontFitting::test_ladder_arcade_is_crisp pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(layout): add fit_text_proportional — proportional sizing vs. always-maximize fit_text always picks the largest ladder rung that fits its box. That's right when an element owns dedicated space, but wrong when several independently-fitted elements need to stay visually harmonious as the panel grows: a score's box might have generous room while a neighboring logo scales by a fixed geometry factor via px() — fit_text lets the score balloon out of proportion (even overlapping the logo) even though its individual pick is technically correct. fit_text_proportional(text, box, base_size_px, ladder) instead targets base_size_px * self.scale (the same scale factor px() already uses), picking the nearest ladder rung at or below that target, still capped to what fits the box, floored at the smallest rung when the target is below every rung. Refactored the shared largest-that-fits/ellipsize walk into _walk_ladder() so fit_text and fit_text_proportional don't duplicate it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(layout): fit_text_proportional gains an axis-specific scale override self.scale (min(width_ratio, height_ratio)) is the right conservative default for anything whose aspect ratio matters, but a caller whose surrounding composition already scales along a single axis — e.g. football-scoreboard's logo_slot = min(height, width // 2), which tracks height alone — needs text sized the same way, or it reads as under-scaled next to logos that grew on a panel that only got taller (128x32 -> 128x64: self.scale stays 1.0 since width didn't grow, but logos still double). fit_text_proportional(..., scale=None) now accepts an explicit override; None keeps the existing self.scale default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(layout): scoreboard_regions reserves real center space at 2:1 aspect ratios logo_slot = min(height, width // 2) has a blind spot: at exactly 2:1 aspect ratio (width == 2 * height -- a very common shape: two, four, or more square modules stacked into a taller panel) width // 2 and height are equal, so the two logo slots claim the ENTIRE width and leave zero pixels for a center column, no matter how large the panel gets. Not a 'small panel' problem -- 96x48, 128x64, and 256x128 (all exactly 2:1) hit it identically, while the 128x32 design baseline and panels like 192x48 or 256x32 never do, because height is already the tighter constraint there. Two new parameters fix it in the one shared helper every scoreboard-style plugin composes through: - min_center_fraction / min_center_design_px reserve at least max(width * fraction, design_px * ctx.scale) for the center column, capping logo_slot further when needed. The scaled design-px term matters on small panels where a flat fraction alone reserves too little absolute space. - score_bleed_fraction extends the score's own fit box (not the logo slots themselves) a controlled amount into each side -- the same way real broadcast scoreboards let a big score number's edges cross into the team marks flanking it. Without this the reserve alone can still be too narrow for a short score to render without truncating. score_area is now genuinely narrower than the full card width (previously identical to status_band/detail_band, which still span the full width and overlay the logos -- short text there was never the problem). Verified against the full harness size spread: a real game score like '17-21' never needs ellipsis at any tested 2:1-or-tighter aspect ratio (test_score_never_needs_ellipsis_for_a_short_score), and wide panels (128x32/192x48/256x32-style) are provably unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: document scoreboard_regions' center-reserve and score-bleed params Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: address CodeRabbit review on PR #393 - docs: scope the self.layout note to BasePlugin subclasses (others build a LayoutContext directly) and make explicit that adaptive layout is opt-in — classic rendering stays unless a plugin adopts the APIs. - dev_server: broaden the render-request catch (a bad manifest.json now returns a clean 400 instead of an unhandled 500) and stop echoing raw exception text in the loader-failure responses — full tracebacks go to the dev server's console log instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam * fix(dev-server): allowlist plugin_id before any path lookup CodeQL (py/path-injection): plugin_id arrives in request input and flows into filesystem paths via find_plugin_dir. Gate it with the same ^[a-zA-Z0-9_-]{1,64}$ allowlist the web UI's pages_v3 uses, at the single choke point every route resolves through. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam * fix(dev-server): lexical containment check on resolved plugin dirs CodeQL doesn't recognize the interprocedural allowlist as a path-injection barrier; add the canonical one — normalize (without following symlinks, since dev plugins are commonly symlinked into plugins/) and require the result to stay inside the search dir. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam * fix(dev-server): inline normpath containment barrier before render CodeQL doesn't credit the sanitization inside find_plugin_dir along this flow; apply its documented barrier (normpath + startswith against the allowed roots) inline in _parse_render_request, on the exact path that reaches the render/load sinks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam * fix(dev-server): derive plugin dir from trusted directory listings CodeQL's barrier-guard recognition doesn't see a startswith check inside an any() comprehension, so the normalize-and-prefix approach still flagged. Break the taint outright instead: after lookup, re-derive the directory by enumerating the search dirs (iterdir) and matching by path equality — the Path used for all downstream file access is built solely from trusted listings, never from request input. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam * fix(dev-server): use os.scandir for path-injection barrier, redact stack traces from render responses CodeQL doesn't model Path.iterdir() as a taint-clearing enumeration the way it does os.scandir() -- _trusted_plugin_dir's iterdir-based rebuild still traced plugin_id through to the manifest.json open(). Switched to scandir, matching the pattern already verified clean on PR #396. Also stops surfacing raw exception text (update()/display() failures) in the JSON render response -- logs full detail server-side via exc_info instead, returning only the exception class name to the client. And drops path values from three plugin_loader debug/error logs that CodeQL flags as clear-text-logging of externally-influenced data, keeping plugin_id (not flagged) for context. * fix(dev-server): remove conditional-reassignment ambiguity in plugin_dir resolution CodeQL's path-injection flow still traced through _parse_render_request after the scandir fix -- the tainted find_plugin_dir() result and the scandir-derived _trusted_plugin_dir() result shared the same variable name (plugin_dir), reassigned only on the truthy branch. That merge point apparently isn't treated as a barrier by the flow analysis, so it kept tracing the pre-reassignment value through to the manifest open(). Split into two distinct names -- candidate_dir (tainted, used only to call _trusted_plugin_dir) and trusted_dir (the only name used for any downstream file access) -- so there's no reassigned variable for the flow to walk through. * fix: remove unused imports flagged by Codacy Union in adaptive_images.py and field in adaptive_layout.py are both imported but never used -- the last two Codacy findings on this PR, matching the same fix already applied on PR #396. * fix(layout): bound the fit cache; never alias the source image in fits Two latent issues found in a self-review pass: - LayoutContext._fit_cache was an unbounded dict (the image cache got an LRU cap, the text-fit cache didn't). Cache keys embed the fitted TEXT, so a plugin fitting changing strings — a live game clock, a ticker — on a 24/7 service grows it forever. Now LRU-bounded at 512 entries via the same pattern as the image cache. - fit_image returned the caller's ORIGINAL image object when the source was already RGBA at target size (contain/fill_height, no ink crop). ImageFitResult is documented as an independent copy, and LayoutContext caches results — an aliased image lets later mutations of the source corrupt cached fits (or vice versa). Copy in that branch. Both covered by new regression tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam --------- Co-authored-by: Chuck <chuck@example.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Sonnet 5
|
05e7c43b27 |
fix(plugins): replace dependency marker files with a real satisfaction check (#390)
* fix(plugins): replace dependency marker files with a real satisfaction check The .dependencies_installed hash-marker system only tracked "was this exact requirements.txt hashed before" — not whether the packages it names are actually present. That made it fragile (a wiped venv, a manually removed package, or a lost/corrupted marker forces a needless full pip reinstall or, worse, a false skip) and produced dead weight for the ~10 plugins whose requirements.txt is comment-only (they still paid a pip subprocess on first boot before a marker existed). Replace it with requirements_are_satisfied() in plugin_loader.py, which checks each real requirement line against importlib.metadata directly, so install_dependencies() only shells out to pip when something is actually missing or version-mismatched. Drops the marker file entirely: removed all marker read/write sites in plugin_loader.py and store_manager.py, the now-pointless marker-cleanup step in the git-update path, the unused legacy marker implementation in plugin_manager.py, and the already-stale clear_dependency_markers.sh script. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * fix(security): close path-injection gap in dependency-satisfaction checks CodeQL flagged 2 new high-severity "uncontrolled data used in path expression" alerts at the open() calls inside this PR's new requirements_has_real_deps()/requirements_are_satisfied() -- both are reachable from paths that were never run through the basename+trusted-base sanitiser this codebase already uses elsewhere: - PluginLoader.install_dependencies() only applied that sanitiser when its optional plugins_dir argument was actually passed; the "no plugins_dir" branch trusted plugin_dir_real directly. Made plugins_dir required (not Optional) so that branch can't exist, and added an explicit guard in load_plugin() so install_deps=True without a plugins_dir fails loudly instead of silently. Production's only real caller (PluginManager) always passes plugins_dir already; the harness/dev-server/render-plugin callers all use install_deps=False and are unaffected. - StoreManager._install_dependencies() never sanitised plugin_path at all, and its call sites ultimately derive that path from a plugin's own manifest.json "id" field (install_plugin_from_url) -- a malicious plugin could otherwise point requirements_file outside plugins_dir. Applied the same os.path.basename()-based containment pattern PluginLoader already uses (and that CodeQL recognises as a real sanitiser). Added test_install_dependencies_requires_plugins_dir and test_install_dependencies_rejects_path_outside_plugins_dir to lock in the actual security property, not just quiet the scanner. Verified: all 20 tests in test_plugin_loader.py pass, plus the PR's existing test plan (test_plugin_system.py, test_store_manager_caches.py: 53 passed) and the full CI plugin-safety suite (test_harness.py, test_visual_rendering.py, test_plugin_matrix.py: 52 passed, 2 pre-existing skips) all still pass. * fix(security): replace basename-only sanitiser with a trusted-enumeration check The previous commit's os.path.basename() + os.path.join() pattern (which a pre-existing code comment claimed CodeQL recognises as a sanitiser) did not actually clear the alert -- the next CodeQL run still flagged the same 2 sink lines, plus a new one at the os.path.join() call itself. Taking a substring of tainted data apparently isn't treated as a barrier by this query, whatever the comment assumed. Replaced it with find_trusted_subdir(): enumerate the trusted plugins_dir via os.scandir() and only use a name that scandir itself produced, matched by equality against the caller's requested name. The path is then built from that enumerated entry, not from the caller's string -- a value sourced from iterating a trusted, non-tainted directory carries no taint regardless of what it happens to equal, which is a stronger and more conventional allowlist-style barrier than string-stripping. Applied identically in both PluginLoader.install_dependencies() and StoreManager._install_dependencies(), sharing one implementation. Re-verified: all 65 tests across test_plugin_loader.py (20, including the 2 new security regression tests), test_store_manager_caches.py (35), test_plugin_system.py (10) pass, plus the full CI plugin-safety suite (test_harness.py/test_visual_rendering.py/test_plugin_matrix.py: 52 passed, 2 pre-existing skips). * fix(security): redact URL credentials from pip subprocess output before logging CodeQL flagged 3 clear-text-logging-of-secrets alerts in install_requirements_file() (src/common/permission_utils.py:353,360,371). Pre-existing on main, unrelated to this PR's own diff, but now visible since the path-injection alerts that previously took priority in the annotation list are fixed. The underlying risk is real: pip can echo a private index URL's embedded basic-auth credentials (from a requirements.txt --index-url line or PIP_INDEX_URL) back verbatim in its own stderr/stdout on failure, and this function both logs that output directly and returns it to callers -- store_manager.py's _install_dependencies() logs result.stderr from this same function too. Added _redact_url_credentials(), applied immediately after each of the two subprocess.run() calls (mutating result.stderr/stdout in place) rather than patching each log call site individually. This closes the leak at the source: every downstream use -- the three flagged log lines, the "note" string embedded in the returned stdout, and store_manager.py's own logging of the returned result -- gets the redacted text for free. Verified the fixed-phrase "denied" check (`"a password is required" in result.stderr`) is unaffected, since URL syntax and those phrases don't overlap -- covered explicitly by test_does_not_touch_denied_check_phrases. Added test/test_permission_utils.py (6 tests) covering the redaction helper directly and both subprocess.run() call sites (the sudo-wrapper branch, which this repo's scripts/fix_perms/safe_pip_install.sh makes live, and the no-wrapper fallback branch). All pass. * fix(security): stop interpolating req_file/pip-output into log calls The previous commit's redaction (mutating result.stderr/stdout right after each subprocess.run()) didn't clear CodeQL's clear-text-logging alerts -- same lesson as the path-injection fix earlier in this PR: a static analyzer can't tell "this value was already sanitised two lines up" from "this is still the raw tainted value" just by looking at a single log call in isolation, so it conservatively keeps flagging it regardless of what the redaction function actually does. Removed all dynamic interpolation (req_file, result.stderr) from the 3 flagged logger.warning() calls entirely, replacing them with fixed messages plus (for the one that had it) result.returncode, which is a plain int with no possible taint. The full redacted detail is still available where it actually matters -- in the returned CompletedProcess.stderr/stdout and the "note" text -- just not duplicated into a log line a scanner has to reason about in isolation. Re-verified: all 6 test_permission_utils.py tests still pass (they assert on the returned result, not log call arguments), plus the full test_plugin_loader.py/test_store_manager_caches.py/test_plugin_system.py suite (71 passed, 1 pre-existing deselect, 4 subtests). --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Chuck
|
2ffc57cf40 |
fix(plugin-harness): add no-op process_deferred_updates to test double (#391)
The safety harness's VisualTestDisplayManager (base of BoundsCheckingDisplayManager) doesn't implement process_deferred_updates(), which 5 first-party ledmatrix-plugins call unconditionally between set_scrolling_state() and their scroll-position update: news, odds-ticker, ledmatrix-leaderboard, stock-news, and ledmatrix-stocks. Any of them fails the harness with AttributeError the moment it's touched (surfaced when ledmatrix-plugins#177 had to add a local hasattr guard in ledmatrix-stocks just to pass CI). Add the method as a no-op, mirroring the existing "no-op for testing" pattern already used for set_scrolling_state, so these plugins render under the harness without every touching PR needing its own guard. |
||
|
Chuck
|
aab0e9ade0 |
fix(plugin-manager): fix TypeError breaking every plugin's scheduled update (#392)
run_scheduled_updates()'s resource-monitor branch wrapped the update call
in a closure stored as a *class* attribute on a dynamically-built type
(type('obj', (object,), {'update': monitored_update})()). The descriptor
protocol turns a function found via class-attribute lookup into a bound
method on instance access, silently prepending the synthetic instance as
an implicit first argument -- but monitored_update() takes none, so every
call raised "monitored_update() takes 0 positional arguments but 1 was
given", was caught by run_scheduled_updates' try/except, and recorded as
an update failure.
self.resource_monitor is None by default and was dormant until PR #388
("activate dormant plugin health/metrics subsystem") wired it up in both
display_controller.py and web_interface/app.py -- meaning this bug went
live in every real deployment as of that merge (2026-07-09) despite the
buggy line itself dating back to 2025-12-27. In practice this means no
plugin's update() has succeeded since upgrading past #388: circuit
breakers cycle through half-open -> immediate failure -> reopened every
health-check interval forever, and all plugin data (scores, odds, prices,
etc.) goes stale from whatever was last fetched before the upgrade.
Confirmed live on a running instance: odds-ticker (and stock-news,
ledmatrix-stocks, baseball-scoreboard, ledmatrix-leaderboard, of-the-day)
failing this exact way every 5-minute circuit-breaker retry.
Fixed by using types.SimpleNamespace(update=monitored_update) instead of
a dynamic class: SimpleNamespace stores attributes on the instance
itself, so attribute lookup returns the plain function unchanged --
never routed through the class-attribute descriptor protocol that
injects an implicit self.
Added test_run_scheduled_updates_calls_update_with_resource_monitor to
test/test_plugin_system.py using a real PluginResourceMonitor (not a
mock of it), so the test exercises the actual descriptor-binding
behavior that caused this. Verified the test fails with the exact
reported error against the pre-fix code and passes against the fix.
|
||
|
ChuckandClaude Opus 4.8
|
3b93024993 |
feat: activate dormant plugin health/metrics subsystem and surface it in the web UI (#388)
* feat(plugin-system): activate dormant plugin health & metrics subsystem
PluginManager shipped a fully-built health tracker, resource monitor and
circuit breaker that were never instantiated (health_tracker/resource_monitor
were left as None), so the circuit breaker never engaged and the existing
health/metrics API routes always returned "not available".
- DisplayController now wires a PluginHealthTracker and PluginResourceMonitor
onto the plugin manager, enabling the circuit breaker (a repeatedly-failing
plugin's update() is skipped after consecutive failures, then retried after
a cooldown) and per-plugin execution-time metrics. Both persist to the
shared cache.
- load_plugin() now validates each plugin's config against its JSON schema in
a strictly warn/degrade-only way: a violation logs a warning and flags the
plugin degraded in the health tracker, but never changes whether the plugin
loads or its pass/fail behaviour. Adds PluginHealthTracker.set_degraded(),
which never touches the circuit breaker.
- ResourceMonitor CPU/memory sampling now reuses a cached psutil.Process and
reads cpu_percent(interval=None), so monitoring no longer blocks ~100ms per
call on the display loop's update path.
- Fix DiskCache.get() raising TypeError for max_age=None ("never expires"),
which silently discarded persisted plugin health/metrics on read and thus
broke cross-process and post-restart surfacing.
- Fix two dead PluginManager helpers that called non-existent tracker methods.
Tests: new test_resource_monitor, test_plugin_health,
test_plugin_manager_schema_soft; extended test_cache_manager and
test_display_controller.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UvTav268UXv44ub9K11LYq
* feat(web-ui): surface plugin health, metrics and load state
With the health/metrics subsystem now active in the display service, expose it
in the web UI (which runs as a separate process from the display loop):
- Wire a health tracker / resource monitor backed by the shared on-disk cache
into the web process so /api/v3/plugins/health and /plugins/metrics read the
data the display service persists.
- Build those route responses per installed plugin id (the tracker's in-memory
view is empty in a fresh web process) so cross-process data is included.
- Add state + error_info to /plugins/installed entries so the UI can show why a
plugin isn't running instead of just loaded:false.
- Add a "Plugin Health" panel to the Tools page (circuit status, avg/max update
time, update count, last error) plus PluginAPI.getPluginMetrics().
Tests: route-level tests for the health/metrics endpoints in test_web_api.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UvTav268UXv44ub9K11LYq
* fix(plugin-metrics): refresh cross-process health/metrics reads; type hints
Addresses CodeRabbit review on #388:
- Major: the web process's health/resource trackers cached the first persisted
read in an in-memory dict (and the CacheManager memory tier held max_age=None
entries indefinitely), so a long-lived web process showed the first snapshot
and never reflected the display service's later updates. Add an opt-in
force_reload path (get_health_summary/get_health_state/_load_health_state and
get_metrics_summary/get_metrics) that bypasses the in-memory copy and, via a
new memory_ttl passthrough on CacheManager.get, the cache manager's memory
tier — so each /plugins/health and /plugins/metrics poll reads fresh persisted
state. Default behaviour (force_reload=False) is unchanged for the display
process and existing callers.
- Minor: DiskCache.get type hint is now Optional[int] with the None ("never
expires") semantics documented, matching MemoryCache.get.
Tests: new force_reload staleness cases in test_plugin_health and
test_resource_monitor.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UvTav268UXv44ub9K11LYq
---------
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
ChuckandClaude Sonnet 5
|
85d321cf33 |
fix: plugin_loader retries with --ignore-installed on apt/pip RECORD conflicts (#386)
* fix: plugin_loader retries with --ignore-installed before assuming apt package satisfies pin install_dependencies treated any "uninstall-no-record-file" pip failure as "dependency satisfied" and wrote the success marker without ever attempting --ignore-installed, unlike install_dependencies_apt.py and safe_pip_install.sh (added in #385 for the Plugin Store/first-time-install paths). A plugin pinning a newer version of a system-managed package (e.g. requests) would silently keep running against whatever version apt shipped, while the marker file claimed the pinned requirement was met. Now retries the same install with --ignore-installed on that specific failure so pip actually lays the pinned version down (shadowing the system-managed copy) before falling back to the prior tolerant behavior if the retry itself fails too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx * chore: suppress Codacy finding on new retry subprocess.run call Same generic Bandit/semgrep pattern-match on non-literal subprocess.run argv flagged in #385's install_requirements_file, now on the new --ignore-installed retry call added here: list-form argv (no shell=True), sys.executable is this process's own interpreter, and requirements_file is built internally by find_plugin_directory, never raw external input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx * fix: tolerate a timed-out --ignore-installed retry, dedupe marker-write logic CodeRabbit review caught a real inconsistency: if the --ignore-installed retry itself timed out, subprocess.TimeoutExpired propagated to the outer handler and returned False, failing plugin load — contradicting the intended "tolerate this specific apt/pip conflict" behavior, where a mere non-zero retry return code already returns True. Wraps the retry in its own try/except so a timeout is logged and tolerated the same way as any other retry failure. Also extracts the marker-writing logic (open/write/chmod, ignoring OSError) into _write_dependency_marker, since it was duplicated identically between the direct-success path and the apt-conflict-retry-fallback path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx * fix: revert marker-write dedup helper, resolves CodeQL path-injection alert CodeQL flagged _write_dependency_marker's open(marker_file, ...) as "uncontrolled data used in path expression" (high severity) once the marker-write logic was extracted into its own method. marker_file is actually safe — it's built from safe_plugin_dir, which install_dependencies sanitizes via os.path.basename() (CodeQL's own recognized py/path-injection sanitizer, per the existing comment a few lines above) — but CodeQL's interprocedural analysis doesn't carry that sanitized status across the new method boundary, since the sanitizer call and the open() sink were no longer in the same function. This exact code produced zero CodeQL findings before the extraction (in two duplicated inline blocks) and is unchanged in what data reaches it — only its location moved. Reverting the extraction (keeping CodeRabbit's other, independent timeout-handling fix) restores the previously-clean shape rather than trying to convince the analyzer's cross-function taint tracking that a refactor changed nothing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
ChuckandClaude Sonnet 5
|
63a233f3ed |
fix: dependency installation gaps in Plugin Store and first-time install (#385)
* fix: install plugin dependencies through root-visible installer in Plugin Store install_plugin/update_plugin (store_manager.py) installed requirements.txt with a bare `pip3` off PATH, bypassing the root-visible installer added in #380 for the "Reinstall Plugin Deps" button. Two bugs stacked: (1) `pip3` can resolve to a different Python install than the one that actually runs ledmatrix.service, and (2) even when it resolves correctly, ledmatrix-web runs as a non-root user so the package lands in that user's local site-packages, invisible to root-run ledmatrix.service. Either way the install reports success and writes the .dependencies_installed hash marker, so plugin_loader's own (correct) install-on-load path skips reinstalling — leaving the dependency permanently missing until a user finds and clicks the separate "Reinstall Plugin Deps" tool. This is why users kept hitting "No module named 'astral'" for the weather plugin even after installing it from the Store. Extracts the sudo-wrapper-then-fallback install logic from api_v3.py's _pip_install_requirements into src/common/permission_utils.py as install_requirements_file, and routes store_manager.py's dependency installation through it so the automatic Store install/update path now matches the manual "Reinstall Plugin Deps" path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx * chore: suppress Codacy false-positive on subprocess.run in install_requirements_file Codacy's generic subprocess-security rule (Bandit B603 equivalent) flagged the pip/sudo subprocess.run calls in install_requirements_file for lacking a "static string argument" — the standard pattern-based flag for any subprocess.run() call with a variable in its argv list. Both calls use list-form argv (no shell=True, so no shell-injection surface), and the only dynamic value is req_file, a Path built internally by callers rather than raw external input; safe_pip_install.sh independently re-validates it before installing anything as root. Suppresses with inline `# nosec B603` comments matching this codebase's existing convention (see permission_utils.py's own PROTECTED_SYSTEM_DIRECTORIES, display_manager.py, sync_manager.py, etc.). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx * fix: first-time install script fails on apt-managed requests package web_interface/requirements.txt and requirements.txt both pin requests>=2.33.0,<3.0.0, but Raspberry Pi OS ships an apt-managed python3-requests with no pip RECORD file. Upgrading it via plain `pip install` aborts with "uninstall-no-record-file" because pip refuses to uninstall a package it has no record of, in place — which is exactly the "Some web interface dependencies failed to install" warning first-time install hits. scripts/install_dependencies_apt.py and scripts/fix_perms/safe_pip_install.sh already work around this with --ignore-installed (lets pip lay the new version down in /usr/local, shadowing the apt copy, instead of trying to remove it first). first_time_install.sh's own direct pip invocations — the per-package requirements.txt loop, the web_interface/requirements.txt install, and the requirements_web_v2.txt fallback — didn't have it. Adds --ignore-installed to all three so first-time install no longer fails on this well-known apt/pip conflict. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx * chore: add nosemgrep to subprocess.run calls Codacy still flagged The prior # nosec B603 comments suppressed Bandit's check but Codacy's semgrep-based rule ("subprocess function 'run' without a static string") kept flagging the same two lines as a critical security issue even after that fix landed. install_dependencies_apt.py's _run() already needed both tags together (# nosec B603 B607 ... # nosemgrep) for the identical subprocess.run pattern, so apply the same double suppression here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx * fix: add --ignore-installed to install_requirements_file fallback path CodeRabbit review caught this (confirming a gap already flagged in conversation): the non-sudo fallback pip install in install_requirements_file was missing --ignore-installed, unlike the sudo-wrapper branch and safe_pip_install.sh. Without it, the same apt/pip RECORD-file conflict this PR fixes elsewhere (first_time_install.sh, install_dependencies_apt.py) could still hit installs that fall back to this path (e.g. a plugin's requirements.txt on a host where safe_pip_install.sh isn't set up yet). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Ron PierceandClaude Opus 4.8
|
d22d0a3754 |
fix(plugins): stop core updates from resurrecting uninstalled built-in plugins (#368)
* fix(plugins): stop core updates from resurrecting uninstalled built-in plugins Built-in plugins (e.g. web-ui-info, starlark-apps) are committed into the repo under plugin-repos/. When a user uninstalls one, a subsequent core `git pull` update restores the committed files, so the plugin reappears on every update. The update endpoint stashes the deletion and never pops it, and `git pull` faithfully restores any committed file whose deletion was never committed — so excluding plugin-repos/ from the stash can't fix this (it would only make `git pull --rebase` fail on a dirty tree). Add a persistent uninstall registry (config/uninstalled_plugins.json, gitignored) that survives restarts, unlike the existing in-memory tombstone: - Uninstall records the plugin id; install clears it. - purge_uninstalled_plugins() re-removes any recorded plugin whose directory reappears on disk; called after a successful git-pull update and at web startup (covers manual `git pull` on the Pi too). - The state reconciler also refuses to auto-repair a persistently uninstalled plugin. Wires up mark_recently_uninstalled in the uninstall flow (previously only referenced by tests) via the new persistent record. Adds regression tests covering record/forget/purge lifecycle, persistence across manager instances, and corrupt-registry tolerance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(plugins): validate uninstall-registry ids and lock registry writes Address review feedback on the persistent uninstall registry: - Critical: validate plugin ids on read/record and add a containment guard in purge_uninstalled_plugins. A corrupt or hand-edited registry entry of "" resolves to the plugins root, so purge could have deleted every plugin; traversal ids ("..", "../x") could target paths outside the root. Invalid ids are now dropped on read, refused on record, and never removed unless the path is a direct child of the plugins directory. - Major: guard record/forget read-modify-write with a lock so concurrent install/uninstall requests can't lose updates. - Minor: narrow the startup and post-update purge exception handlers from bare Exception to (OSError, RuntimeError). Adds regression tests for empty-id, traversal-id, and invalid-record cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Ron PierceandClaude Opus 4.8
|
313e35a98f |
Add cross-size/cross-screen plugin safety harness (#361)
* feat(testing): add cross-size/cross-screen plugin safety harness Render every plugin across all supported matrix sizes (64x32, 128x32, 128x64, 256x32) and every declared screen, failing on crashes, content drawn past the panel edge, or visual drift vs committed golden images. - BoundsCheckingDisplayManager: oversized-canvas overflow detection - harness.py: multi-size/multi-screen render engine + golden compare - scripts/check_plugin.py: CLI (functional+bounds, --out-dir, --update-golden, --freeze-time); render_plugin.py refactored onto shared loading helpers - test/plugins/test_harness.py + test_plugin_matrix.py (parametrized, honors per-plugin test/harness.json; skips when no plugins present) - MockCacheManager.cache_dir so cache-dir-using plugins load headlessly - .github/workflows/test.yml + docs/plugin-safety-harness.md Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(testing): address PR review feedback on plugin safety harness - check_plugin: friendly error for non-numeric --sizes; reject non-object --config / --mock-data JSON; sanitize plugin mode before using as a filename; stop --update-golden from masking crash/overflow failures - bounds_display_manager: pad the canvas out to the largest supported panel (not a fixed 16px) so far-overshoot coordinates are caught, not clipped - harness: merge config_schema defaults inside render_plugin_matrix; surface update() failures as a non-fatal warning + result field instead of a debug log; sanitize mode in golden_path - loading: fail fast when harness.json references a missing mock_data fixture - mocks: clean up the per-instance temp cache dir via weakref.finalize - test_plugin_matrix: add a discovery guard that fails when LEDMATRIX_REQUIRE_PLUGINS=1 but none found (still skips locally); type hints - bound test deps with upper version pins for deterministic CI Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(testing): render plugins across arbitrary panel sizes, not a fixed list Addresses maintainer feedback that there is no canonical set of supported panel sizes — a build can be any size/configuration (square, 2x2, 4x4, 8x2, long strips, tall stacks). - sizes.py: SUPPORTED_SIZES -> DEFAULT_TEST_SIZES (back-compat alias kept), reframed as a representative SAMPLE of real panel-grid arrangements rather than an authoritative list; add parse_size_token / coerce_sizes / resolve_test_sizes helpers - sizes are now fully overridable: LEDMATRIX_TEST_SIZES env (global, e.g. test on your exact hardware) > per-plugin harness.json "sizes" > default sample; CLI --sizes unchanged - bounds_display_manager: pad the canvas to the largest panel IN THE CURRENT RUN (via overflow_extent) instead of a hardcoded max, so cross-size overflow detection scales to whatever sizes a run uses - harness: compute per-run extent and thread it into the bounds manager - tests: arbitrary-shape + size-parsing/precedence coverage - docs: rewrite "Supported sizes" -> "Sizes: a sample, not a fixed list" Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(testing): fail the harness on non-connectivity update() errors Addresses the remaining review thread: recording every update() exception as a non-fatal warning still let a real update() regression pass green as long as display() survived. Now update() failures are classified — a tolerated set of connectivity errors (ConnectionError/TimeoutError/socket/ssl/urllib/http/ requests) is recorded non-fatally (expected with no network in CI), while any other exception is treated as a genuine bug and fails that render. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci(security): pin actions to SHAs and disable checkout credential persistence Addresses the CodeRabbit/zizmor workflow-hardening finding: pin actions/checkout and actions/setup-python to full commit SHAs and set persist-credentials: false on checkout to reduce supply-chain and token-exposure risk. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(testing): validate positive sizes; narrow requests import except Two review findings: - sizes.py: parse_size_token / coerce_sizes now reject non-positive dimensions (0x32, -64x32) with a clear message instead of passing invalid sizes downstream (CodeRabbit). - harness.py: the optional `requests` import now catches ImportError specifically and logs instead of `except Exception: pass`, clearing the Codacy medium "Try, Except, Pass" (harness.py L52) and Ruff S110/BLE001. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
|
eedf680a8c |
perf: display pipeline optimizations — caching, logging, scroll, text width (#358)
* docs(core): add module and class docstrings to the 5 undocumented core files
Fills the only significant documentation gaps found during a codebase
audit. All other core files (plugin_system/, logging_config.py, etc.)
already have complete module, class, and function docstrings.
Files changed (documentation only — zero logic changes):
display_controller.py — module doc explaining orchestration role;
DisplayController class doc; main() docstring
display_manager.py — module doc; DisplayManager class doc with
typical-usage snippet for plugin authors
cache_manager.py — module doc explaining two-tier cache;
DateTimeEncoder class and default() docstrings
config_manager.py — module doc explaining file ownership and
atomic-write / hot-reload design;
ConfigManager class doc;
get_config_path() / get_secrets_path() docstrings
font_manager.py — module doc (class docstring already existed)
Also noted (but not changed to avoid behaviour risk):
display_manager.py and font_manager.py use logging.getLogger() directly
instead of the project's get_logger() wrapper. display_manager.py also
calls setLevel(logging.INFO) immediately after, which would be lost if
switched to get_logger().
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* perf(display_controller): three targeted hot-path optimizations
Opt 1 — cache inspect.signature() per plugin_id
inspect.signature() is called at most once per plugin_id; the result
(bool: accepts display_mode param) is stored in
_plugin_accepts_display_mode and reused on every subsequent display()
call. Eliminates all reflection from the display path at runtime.
Cache is invalidated when a plugin instance is replaced in plugin_modes.
Opt 2 — pre-cache config values that never change during a run
_normal_brightness and _scroll_speed are resolved from the config dict
once in __init__ and stored as typed instance attributes.
- Removes 2+ chained dict.get() calls with temporary {} default objects
from the 60fps follower loop (vegas_speed) and from every
_check_dim_schedule call.
- current_brightness init now uses _normal_brightness directly.
Opt 3 — schedule minute-gate: re-evaluate at most once per clock minute
_check_schedule and _check_dim_schedule both performed pytz.timezone(),
datetime.now(), strftime(), and datetime.strptime() on every outer loop
call. Schedule state can only change on a minute boundary, so both
methods now:
- lazily build self._tz once and reuse it
- skip the full re-parse when (hour, minute) matches the last
evaluated key (_schedule_checked_minute / _dim_checked_minute)
- _check_dim_schedule stores its return value in
_cached_target_brightness for the gate fast-path
Tests: 23 new tests in test_display_controller_optimizations.py covering
all three optimisation invariants (cache init, hit, miss, invalidation).
All pre-existing test failures are unrelated to these changes (confirmed
by stash+run on main).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix: resolve 22 pre-existing test failures across 6 groups
Test fixes (tests were asserting wrong values or patching wrong objects):
basketball scoreboard — update display mode assertions from generic
basketball_live/recent/upcoming to league-prefixed nba_live/recent/upcoming
to match the current manifest
display_controller schedule — inject schedule directly into controller.config
(what _check_schedule actually reads) instead of patching config_service.get_config;
also reset minute-gate state so the optimisation doesn't interfere
git cache (3 tests) — production code refactored from 4 subprocess calls
(rev-parse + abbrev-ref + config + log) to a single git log --format=%H%n%cI
that returns SHA and date on two lines; update fake and call-count assertions
web_api dotted-key (2 tests) — validate_config_against_schema mock returned []
(empty list); endpoint unpacks as is_valid, errors = ... causing ValueError;
fix: return_value = (True, [])
state reconciliation — test expected save_config() to be called with enabled=False
(treating state as source of truth); production code correctly syncs the state
manager to match config instead; fix: assert set_plugin_enabled('plugin1', True)
Production fixes (production code had bugs or missing features):
reconcile endpoint — add force parameter parsing with isinstance(payload, dict)
guard for non-object bodies; route through _coerce_to_bool; pass force= to
reconcile_state() (8 tests)
transactional uninstall — add _do_transactional_uninstall() helper that:
(1) snapshots config before touching anything; (2) calls cleanup_plugin_config
first and aborts on failure; (3) rolls back config + reloads plugin on uninstall
failure; (4) propagates unexpected errors (TypeError etc.) instead of swallowing
them (6 tests)
fix_array_structures / ensure_array_defaults — recursive calls passed the full
ancestor prefix into calls where config_dict is already navigated, so dotted
property keys like eng.1 caused parent_parts.split('.') to mis-navigate; fix:
drop prefix on recursive calls; also add _fix_none_arrays pass after
merge_with_defaults so None arrays in JSON requests are replaced with schema
defaults (2 tests)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* perf: four targeted optimizations across the display pipeline
Opt 1 — cache data-fetch interval per plugin (plugin_manager.py)
_get_plugin_update_interval fell back to config_manager.get_config()
(a full dict copy) when the manifest lacked an interval. Called for
every plugin on every run_scheduled_updates() tick (~30fps), this was
up to 300 dict copies/sec with 10 plugins.
Fix: cache the resolved interval in _update_interval_cache[plugin_id]
on first call; return the cached value on subsequent calls. Cache is
cleared on load_plugin and unload_plugin.
Opt 2 — demote noisy per-cycle INFO logs to DEBUG (display_controller.py)
Four logger.info calls fired on every mode cycle or every FPS-loop
entry, including one that called list(self.plugin_modes.keys())
unconditionally (allocating a list every outer loop iteration).
- "Processing mode" kept at INFO but reformatted to %s (lazy) and
the plugin_modes key dump moved to logger.debug
- "Attempting/Got cycle duration" → logger.debug
- "Entering high/normal FPS loop" → logger.debug
Mode name at INFO is preserved for black-screen troubleshooting.
Opt 3 — use Image.frombytes instead of Image.fromarray in scroll hot path
(scroll_helper.py)
Image.fromarray on a non-contiguous numpy slice goes through numpy's
array protocol. Image.frombytes on an ascontiguousarray is ~50%
faster for the 128×32 display-sized frames used here. Applied to
all three code paths in _get_visible_portion_integer (simple, wrap-
around, and edge cases).
Opt 5 — cache get_text_width per (text, font) pair (display_manager.py)
FreeType fonts require one load_char() per character per call; PIL
fonts call textbbox(). Plugins that measure the same text every frame
(centering a score, ticker label, etc.) were re-measuring from scratch
on every display() call.
Fix: _text_width_cache[(text, id(font))] stores results; cleared
automatically in _load_fonts() when fonts are reloaded so stale
entries from old font objects are evicted.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(scroll_helper): fix edge-case bug exposed by frombytes switch
The previous commit replaced Image.fromarray with Image.frombytes in
_get_visible_portion_integer. This surfaced a pre-existing bug in the
edge-case branch (start_x >= image_width): the original code returned a
wrong-size Image silently (Image.fromarray accepts a too-short array);
Image.frombytes raises ValueError instead.
Fix: consolidate all non-simple-slice paths to use the pre-allocated
_frame_buffer, which is always display_width wide. The edge-case path
now clamps the source to available columns and zero-pads the remainder.
Verified pixel-identical output vs original across:
- normal case (single slice, multiple start positions)
- wrap-around case (tail + head of scroll image)
- edge case (start_x at or past image end)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix: address CodeRabbit review comments on PR #358
1. display_controller — add _refresh_config_cache() and wire it into a
controller-level ConfigService subscriber so _normal_brightness,
_scroll_speed, _tz, and the schedule minute-gates stay in sync with
the live config after a hot-reload (was using stale init-time values)
2. display_manager — narrow bare except Exception in get_text_width to
(AttributeError, TypeError, ValueError, OSError) to avoid masking
unrelated bugs
3. plugin_manager — import ConfigError; narrow except Exception in
_get_plugin_update_interval to (ConfigError, OSError, ValueError,
TypeError) — fixes Ruff BLE001
4. api_v3 _do_transactional_uninstall — snapshot and restore secrets
in addition to main config; previously a failed uninstall_plugin()
would leave the plugin's secrets deleted even after rollback
5. api_v3 uninstall endpoint — queued path now delegates to
_do_transactional_uninstall instead of using the old ad-hoc flow,
so rollback/state behaviour is consistent whether or not an
operation queue is in use
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(display_controller): move _plugin_accepts_display_mode init before plugin loop
Codacy HIGH: 'access to member before its definition' — the dict was
initialised at line 441 but accessed at line 364 inside the plugin-
loading loop, both within __init__.
Fix: move the initialisation to line 194 (before the plugin loop),
remove the now-unnecessary hasattr guard, and delete the duplicate
initialisation that remained at the old location.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Chuck <chuck@example.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
||
|
|
cac9644b6d |
fix(plugin-loader): auto-detect new dependencies via requirements.txt hash (#355)
* fix(plugin-loader): detect new deps via requirements.txt hash instead of empty marker The .dependencies_installed marker was an empty file, so adding a new package to requirements.txt (e.g. astral in ledmatrix-weather v2.3.0) never triggered a pip re-install on existing installs — the file existed so the check returned early. The marker now stores a SHA-256 hash of requirements.txt. On every plugin load, the loader compares the current hash to the stored one; a mismatch (or missing marker) triggers pip install and writes the new hash. store_manager._install_dependencies() also writes the hash marker after a store install/update so the loader skips a redundant pip run on next boot. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(plugin-loader): address CodeQL path expression and I/O error handling - Add explicit relative_to() containment check after path resolution so CodeQL recognizes the plugin directory boundary (fixes 4 CodeQL alerts: Uncontrolled data used in path expression, lines 168/172/189/205) - Wrap requirements_file.read_bytes() in try/except OSError — on Raspberry Pi with flaky SD card storage this can fail; returns False with a clear log - Wrap marker_path.read_text() in try/except OSError — a corrupted marker falls through to a clean reinstall instead of crashing - Wrap both marker_path.write_text() calls in try/except OSError — pip already succeeded at this point so a marker write failure should not return False or propagate through the generic exception handler Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(plugin-loader): use realpath+startswith containment check for CodeQL path-injection Replace relative_to() (not recognised by CodeQL as a path sanitiser) with the os.path.realpath() + startswith() pattern that CodeQL explicitly models as sanitising py/path-injection. - Add plugins_dir optional param to install_dependencies() and load_plugin() - PluginManager.load_plugin() passes self.plugins_dir as the trusted anchor; install_dependencies() validates that the resolved plugin_dir starts with the resolved plugins_dir before any file I/O - Replace all Path.read_bytes/read_text/write_text/exists with open() and os.path.isfile() so the sanitised string paths flow directly to file ops without re-introducing taint through Path object conversion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(plugin-loader): fail-fast when install_dependencies returns False Previously the boolean result was silently discarded, so a failed pip install would log a warning but continue attempting to import the plugin module — resulting in a confusing ModuleNotFoundError instead of a clear dependency failure message. Now raises PluginError with plugin_id and plugin_dir if dependency installation fails, stopping the load before the import is attempted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(plugin-loader): use basename+reconstruct to satisfy CodeQL py/path-injection startswith() is a validation check in CodeQL's model, not a sanitiser — taint still flows through plugin_dir_real to the file operations. os.path.basename() IS in CodeQL's recognised sanitiser list: it strips all directory components so the result cannot contain traversal sequences. Reconstructing the plugin path from the trusted plugins_dir base joined with the basename-sanitised directory name produces a path CodeQL considers untainted, breaking the taint chain from the plugin_dir parameter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(plugin-loader): guard against empty basename when plugin_dir resolves to fs root If plugin_dir somehow resolves to '/' or a bare drive root, os.path.basename() returns '', causing safe_plugin_dir to equal plugins_dir_real and the isdir() check to pass incorrectly. Reject early with a clear error in that case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Chuck <chuck@example.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
sarjentandClaude Sonnet 4.6
|
35c540d0e0 |
fix(reconciler): prefer config.json over state manager for enabled mismatch (#353)
* fix(reconciler): prefer config.json over state manager for enabled mismatch When the enabled state in config.json and plugin_state.json diverged, the reconciler was syncing config.json to match plugin_state.json (state manager wins). This silently disabled plugins on every restart whenever the state file had an outdated enabled=false entry — most commonly after an uninstall+reinstall cycle, where the reinstall left the plugin in the installed-but-not-enabled state while config.json still had enabled=true. Flip the sync direction: update plugin_state.json via set_plugin_enabled() to match config.json instead. config.json is the user-editable source of truth; the state file is an internal tracker that should follow it when they disagree. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(reconciler): return set_plugin_enabled result instead of always True Capture the boolean returned by set_plugin_enabled() and propagate it so reconciliation accurately reflects failure to update the state manager. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
|
ea95f37d73 |
fix(reconciler): add sync, github, youtube to _SYSTEM_CONFIG_KEYS (#351)
config_manager.load_config() deep-merges config_secrets.json into the main config before returning it. This means secrets top-level keys (github, youtube) appear alongside structural config keys (sync) in the dict that _get_config_state() iterates. _SYSTEM_CONFIG_KEYS was missing all three, so the reconciler treated them as plugin IDs and flagged them as PLUGIN_MISSING_ON_DISK on every startup, showing the "Stale plugin config entries found" warning banner to users on a fresh install where those plugins have never existed. Add the three keys with brief comments explaining which file each comes from so the distinction is clear when the list grows. Co-authored-by: Chuck <chuck@example.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
|
713539e491 |
fix(web-ui): fix quick actions not firing, add toast feedback, suppress install handler warning (#346)
* fix(web-ui): fix quick actions not firing, add toast feedback, suppress install handler warning - base.html: add htmx:afterSettle listener to set data-loaded on tab containers after HTMX swaps their content, preventing the overview partial from being re-fetched (and handlers lost) on every tab switch - base.html: call htmx.process() in loadOverviewDirect/loadPluginsDirect fallbacks so buttons get HTMX handlers even if HTMX finished its initial body scan before the fallback fetch completed - overview.html + index.html (11 buttons): replace event.detail.xhr.responseJSON (undefined in HTMX 1.9.x) with JSON.parse(event.detail.xhr.responseText) so quick action toast notifications actually fire - plugins_manager.js: add guarded htmx:afterSettle listener that only calls attachInstallButtonHandler when #install-plugin-from-url is in the DOM, eliminating the spurious console warning on non-plugin tab loads Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(web-ui): ensure quick-action toasts always fire even on xhr/parse failure Replace silent catch(e){} in all 11 hx-on:htmx:after-request handlers with a pattern that sets default message/status before the try block and calls showNotification(m,s) unconditionally after it, so a fallback toast is shown whenever xhr is absent or responseText is not valid JSON. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(web-ui): show error toast on non-JSON 4xx/5xx quick-action responses In the catch block of all 11 hx-on:htmx:after-request handlers, check xhr.status >= 400 and downgrade s to 'error' so a failed action that returns an HTML error page (or other non-JSON body) surfaces as an error toast instead of the optimistic 'success'/'info' default. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(web-ui): guard setTimeout fallback for attachInstallButtonHandler The 500ms fallback setTimeout was calling attachInstallButtonHandler() unconditionally even when the plugins partial wasn't in the DOM, causing a spurious console.warn on every page load. Add the same element-existence check already present on the htmx:afterSettle listener. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix backup API 404s, hardware status 500, and HTMX loading race - Add all backup API routes to api_v3.py: preview, list, export, validate, restore (with plugin reinstall), download, delete - Fix PermissionError on /hardware/status: return graceful 200 instead of 500 when the status file is owned by a different user; also fix root cause by writing the file world-readable (0o644) in display_manager - Fix HTMX race: dispatch htmx:ready window event from HTMX onload callback; loadTabContent now waits for that event instead of immediately falling back to direct fetch (eliminating the "HTMX not available" console warning on initial load) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Cancel HTMX fallback timers when htmx:ready fires The 5-second setTimeout fallbacks for plugins and overview were firing before the htmx:ready event arrived, logging spurious warnings. Each timer now self-cancels via htmx:ready so the fallback only triggers when HTMX genuinely fails to load. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Address review feedback: error leaks, ok:false, htmx:ready coverage - Backup endpoints: replace raw str(e) in user-facing responses with a generic message; full exception still logged via exc_info=True - hardware/status: change ok:null to ok:false for PermissionError and json.JSONDecodeError so the UI's hw.ok===false check triggers correctly - base.html: dispatch htmx:ready from the fallback load path so any deferred listeners fire on CDN-fallback loads too - loadTabContent: also listen for htmx-load-failed so overview/wifi/plugins fall back to direct fetch when HTMX is completely unavailable Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Treat system-managed pip packages as satisfied for dependency marker When a plugin's requirements.txt includes a package installed via the system package manager (dnf/apt), pip fails with 'uninstall-no-record-file' because it can't replace the system-tracked copy. The package is present and functional, but the missing marker caused the install to be retried on every service restart. Detect this specific error pattern: if the only pip failure is uninstall-no-record-file, write the .dependencies_installed marker and log a warning instead of returning False, suppressing the repeated warning. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix uninstall-no-record-file detection condition The previous check used a string replacement that left 'error:' in the remaining text, causing the condition to always evaluate false. Simplify to a direct substring check: if 'uninstall-no-record-file' appears in pip stderr the affected package is installed at the system level and we write the marker, suppressing the repeated warning on every restart. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Resolve CodeQL security findings in backup API Path traversal (CWE-22): - backup_download: switch from send_file(user-tainted-path) to send_from_directory(_BACKUP_EXPORT_DIR, filename); Flask uses werkzeug safe_join internally which CodeQL recognises as a sanitizer - backup_delete: enumerate the export directory and match by name so entry.unlink() operates on a filesystem-derived Path rather than one constructed from user input; _safe_backup_path still guards first Information exposure through exceptions (CWE-209): - backup_validate: err_msg from validate_backup() can embed exception strings containing temp-file paths; log the detail, return a generic 'Invalid or corrupted backup file' to the client - Other backup endpoints: already fixed (str(e) -> generic message); CodeQL alerts will clear on next scan plugin_loader.py:185 (path traversal): false positive — requirements_file is constructed from plugin_dir returned by find_plugin_directory() (a filesystem scan), not from raw HTTP request input; no change needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix pre-existing information exposure in version and action endpoints - get_system_version (alert #218): replaced str(e) with generic message; exception still logged via logger.error(exc_info=True) - execute_system_action (alert #216): removed str(e) and full traceback.format_exc() from the HTTP response — the full stack trace was being sent directly to clients; replaced with generic message and proper logger.error call Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix remaining GitHub CodeQL security alerts - py/stack-trace-exposure: Remove str(e) and traceback.format_exc() from all HTTP responses across api_v3.py, pages_v3.py, and app.py; replace with generic messages and logger.error(exc_info=True) - py/reflective-xss: Escape partial_name via markupsafe.escape in the load_partial 404 response - py/path-injection: Add regex validation of plugin_id before filesystem use in _load_plugin_config_partial - py/incomplete-url-substring-sanitization: Replace 'github.com' in substring checks with urlparse hostname comparison in store_manager.py - py/clear-text-logging-sensitive-data: Remove football-scoreboard debug prints and sensitive request-body prints from update endpoint - js/bad-tag-filter: Replace script-only regex in BaseWidget.sanitizeValue with DOM-based textContent stripping that removes all HTML - js/incomplete-sanitization: Fix escapeAttr to properly encode &, ", ', <, > using HTML entities instead of backslash escaping - js/prototype-pollution-utility: Add __proto__/constructor/prototype key guards to deepMerge function in plugins_manager.js - app.py error handlers: Always return generic messages; remove debug-mode branches that could expose tracebacks in production Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix three remaining CodeQL path-injection and info-exposure alerts - plugin_loader.py: resolve plugin_dir with strict=True and validate marker_path with relative_to() before any filesystem writes, giving CodeQL the positive sanitization pattern it requires (py/path-injection) - api_v3.py _safe_backup_path: replace substring negative checks with a strict positive regex (^[a-zA-Z0-9][a-zA-Z0-9._-]{0,200}\.zip$) that CodeQL recognises as sanitising the user-supplied filename (py/path-injection) - api_v3.py backup_validate: whitelist known-safe manifest fields before returning JSON, preventing any exception strings captured inside validate_backup() from reaching the HTTP response (py/stack-trace-exposure) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Resolve 29 open CodeQL security alerts across 5 files py/flask-debug (#214): - debug_web_manual.py: read debug mode from LEDMATRIX_FLASK_DEBUG env var instead of hardcoded True py/stack-trace-exposure (#216, #218): - api_v3.py execute_system_action: remove subprocess stdout/stderr from HTTP responses; log via logger instead - api_v3.py get_git_version: validate output matches safe ref format (^[a-zA-Z0-9._-]+$) before including in response - api_v3.py: remove all remaining traceback.format_exc() dead variables and print() debug calls (replaced with logger.debug/warning) py/reflective-xss (#207, #208, #209, #210, #211, #212): - api_v3.py: remove plugin_id from all error/success response messages (uninstall, install, update, health, not-found responses) - pages_v3.py load_partial: return static "Partial not found" message instead of echoing partial_name - pages_v3.py _load_starlark_config_partial: add app_id regex validation, use static error messages instead of f-strings with app_id py/path-injection (#187–#206): - pages_v3.py _load_plugin_config_partial: resolve plugins_base and validate _plugin_dir with relative_to() before all file operations; same for assets metadata directory - pages_v3.py _load_starlark_config_partial: resolve starlark_base and validate schema_file/config_file paths with relative_to() - plugin_loader.py _find_plugin_directory: resolve plugins_dir and validate strategy-2 candidates with relative_to() - plugin_loader.py install_dependencies: resolve plugin_dir first, then construct requirements_file and marker_path from resolved base - plugin_loader.py load_module: resolve plugin_dir with strict=True and validate entry_file with relative_to() before exec_module Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix 15 remaining CodeQL path-injection and stack-trace-exposure alerts Switch from resolve()+relative_to() to os.path.basename() reassignment, which CodeQL recognizes as a path sanitizer that breaks the taint chain. Also remove exception objects from backup_manager validate_backup return strings to eliminate the stack-trace-exposure taint source. Fixes alerts #227, #233, #234, #235, #237, #238, #239, #240, #241, #242, #243, #244, #245, #246, #247. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix broken logger format string and leaked exception in config save error - pages_v3.py: plain string was used instead of %-style substitution, so every manifest-read failure logged the literal "{plugin_id}" - api_v3.py save_main_config: exception message was still leaking through the error response; replace with generic message (consistent with the rest of the CodeQL sweep in this PR) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Chuck <chuck@example.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |