2052 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
ChuckandClaude Opus 5	2df273ecfc	fix(web): default web_display_autostart to true, as the installer already does (#556) ... start_web_conditionally.py read the flag with `config_data.get("web_display_autostart", False)`, so a config that simply lacked the key got no web interface. Both config/config.template.json and first_time_install.sh ship the key as true, so the code default contradicted the shipped default in two places: absence means an older or hand-edited config, not a request to stay down. The failure mode was silent in the worst way. The "not starting" path exits 0, so `systemctl status ledmatrix-web` reported the unit as successfully started while nothing was listening on the port, and the only trace was one journal line saying the flag was "false or not set" -- which reads as a deliberate setting rather than a missing key. Also start the web interface when config.json is missing or unparseable, instead of exiting. The web interface is how a config gets created and repaired, so a broken config is exactly when the user needs it most; leaving it down means there is no way back in. Only an explicit false/off disables autostart now, and the disabled message says "explicitly disabled" so the journal distinguishes a real setting from a default. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-10 17:42:00 -04:00
ChuckandClaude Opus 5	a29c84208e	fix(scroll): advance whole pixels per frame, not per wall-clock second (#545) ... * fix(scroll): advance whole pixels per frame, not per wall-clock second Smooth motion is not a frame-rate property, and measuring it as one is why this survived three rounds of fixes. odds-ticker's frame timing is excellent -- 100.0 fps, 10.00ms median, 0% stalls, worst in-scroll frame 19.95ms -- and it still visibly stuttered. What the eye judges is whether the strip advances the same number of whole pixels on every presented frame. update_scroll_position derived position from scroll_speed * delta_time and get_visible_portion truncated it with int(), so jitter in delta_time decided which side of a pixel boundary the position landed on. The live windows show why that matters: a rock-steady 100.0 fps whose individual frames still range 5.6ms to 15.2ms, which at 100 px/s is 0.57px to 1.44px of movement. Run the measured frame times through the real helper and 5.8% of frames advance 0 or 2 pixels instead of 1 -- about six hitches a second. A frame that moves nothing followed by one that jumps two is exactly what micro-stutter looks like. It is worst at a crisp speed, which is the part that stings: at 100 px/s on a 100Hz panel the accumulator sits exactly on integer boundaries, so sub-millisecond jitter flips it either way and the motion beats at around 50Hz. Snapping to the crisp ladder fixes the average and the wall clock then throws away the per-frame uniformity the ladder was bought for. So when scroll_config snaps to a crisp speed it now also puts the helper in fixed-step mode: each presented frame advances exactly pixels_per_frame and no clock is consulted. 100% of frames move by the same amount, whatever the jitter. This is only correct because SwapOnVSync blocks until the panel has taken the frame, which makes the frame count a truer clock than time.time(). Before the swap was locked to vsync it would have run at whatever speed the loop spun at. Related: frame-based mode used to step discretely and was converted to elapsed-time accumulation earlier in this series, because its threshold comparison flipped on jitter. That was right for the code as it stood -- but it treated the symptom, replacing a broken discrete step with a smooth-looking accumulator instead of asking why a wall clock was involved at all. Non-crisp speeds keep pacing off time, and set_scroll_speed() clears the fixed step so a legacy caller changing speed is not silently ignored. Trade-off worth naming: speed is now tied to the presentation rate rather than to real time. If the loop cannot keep up with the panel the scroll runs slow rather than jumping to catch up. That is the better failure -- uniform motion at a slightly wrong speed beats correct average speed with a hitch six times a second -- and a loop that cannot hit the resolved rate is a measurement problem for the crisp ladder, not something to paper over with uneven steps. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(scroll): make the time-based pin actually pin something Review caught that test_time_based_stepping_is_what_it_replaces could pass against perfectly uniform motion, and it was right. update_scroll_position sets last_update_time on its way through, so the very first call sees a delta_time of zero and moves nothing in time-based mode. _advances counted that synthetic frame, which put a guaranteed zero in every histogram -- enough on its own to satisfy "uneven > 0". The test asserting the defect exists would have passed after the defect was gone. The first call is now primed and discarded, and the assertion is a proportion rather than "more than zero": against these frame times the old path misses roughly one frame in twenty, so 1% is well below the real rate and far above anything a stray frame could produce. Re-measured with the artefact removed, the numbers in the PR description are unchanged: 5.85% of frames uneven before (114 zero-advance and 120 double frames in 4000), 0.00% after. Also fills in the docstrings the review flagged: everything in the new test file, plus three pre-existing one-liners in scroll_config that the diff touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-09 16:25:45 -04:00
ChuckandClaude Opus 5	1198615d19	test: install PyYAML so the starlark route tests can load the plugin (#546) ... Tests has been red on main since #535. All 13 failures in test/web_interface/test_starlark_pixlet_routes.py are the same ModuleNotFoundError: No module named 'yaml'. The test loads plugin-repos/starlark-apps/tronbyte_repository.py by path -- deliberately, "the way the blueprint does", since the core web blueprint really does exec that plugin module -- and the plugin imports yaml. Nothing is undeclared. The plugin's own requirements.txt already pins PyYAML>=6.0.2, and on a real rig the plugin store installs it. CI installs only requirements.txt and requirements-test.txt, so a core test that reaches into a plugin gets none of the plugin's dependencies. PyYAML goes in the test requirements rather than the core ones because it is not a core dependency: nothing in src/ or web_interface/ imports yaml. This is the same shape as the psutil entry directly above it -- a package the core does not require, installed so a test can exercise a real path instead of a stub. Verified locally: with yaml available the file goes from 13 failures to 64 passing. (One unrelated failure remains on Windows only, where os.geteuid does not exist.) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-09 16:25:30 -04:00
ChuckandClaude Opus 5	f8e2e89edc	refactor(sports): put the scoreboards on the shared scroll resolver (#542) ... * refactor(sports): put the scoreboards on the shared scroll resolver Eight sports scoreboards -- afl, baseball, basketball, football, hockey, lacrosse, nrl, soccer -- scrolled through this module's own pacing while the other eleven scrolling plugins went through src/common/scroll_config. Two implementations of the same job, and this one was on the losing side of every difference. It never called set_scrolling_state. Two consequences, both of which this release's work was about: - The frame hold is applied through that call, so a speed the crisp ladder could render in whole pixels still presented a new frame every refresh. - Core only runs deferred updates while nothing is scrolling. Believing nothing was, it ran blocking work in the middle of these scrolls. The default is non-crisp today: scroll_speed 50.0 with scroll_delay 0.01 is 50 px/s, which on a 100Hz panel is half a pixel per refresh. That cannot render as motion -- it alternates 0px and 1px steps and judders at a 50Hz beat, on every scoreboard, out of the box. Resolved through the ladder it stays 50 px/s and holds each frame for two refreshes: same speed, whole-pixel motion. The stepping disagreement that used to justify a separate module is gone. scroll_config avoided frame-based mode because it stepped on a wall clock at 1/scroll_delay with scroll_delay set to the frame period, so the decision sat on its own threshold and flipped on sub-millisecond jitter. That branch now accumulates elapsed time, identical arithmetic to the time-based one, so the two differ only in the units the speed arrives in. What is NOT shared, and must not be: the two modules read identically-named keys with different meanings. Here scroll_speed is px/SECOND and scroll_delay only converts to px/frame; in scroll_config scroll_speed is px per STEP, so px/s is speed/delay. Passing this module's settings dict to the resolver turns 50 px/s into 5000, clamped to 500 -- a tenfold speed-up everywhere. So _get_scroll_settings keeps sole ownership of reading sports config, including the league merging, and hands the resolver a plain px/s. A test pins that specific number, because it is the mistake the refactor invites. MIN/MAX_PIXELS_PER_FRAME are gone; the resolver bounds speed and the helper clamps FPS. _resolve_target_fps stays, re-purposed: under the old model that key was the rate frames were presented at, so it is the faithful translation into the refresh the ladder is computed against, used when no hardware refresh is configured. Speed changes for panels that are not 100Hz: 50 px/s becomes 60 at 60Hz (+20%) and 48 at 120Hz (-4%). At 100Hz it is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(display): drop the frame hold when a scroll times out, not just when it says so set_scrolling_state(False) clears the hold. The other way a scroll ends is is_currently_scrolling() deciding, after scroll_inactivity_threshold of silence, that it is over -- which is what happens when the rotation moves on mid-scroll or a plugin is torn down. That path cleared the flag and kept the hold, so every later plugin, scrolling or static, was presented at refresh/N by whoever scrolled last, until something called the explicit stop. The method's own docstring already states the rule this breaks: the hold "must not outlive the scroll that asked for it". The timeout was the exception it did not cover. Pre-existing, but reachable by three plugins before and eleven after the sports scoreboards moved onto the shared resolver, so it belongs with that change. The test ages the activity timestamp past the threshold rather than sleeping. Also adds scripts/sports_scroll_check.py. The sports scroll path is per-league opt-in, so a rig showing static game cards never constructs a SportsScrollDisplay and none of its pacing can be observed from a normal run -- which is exactly what happened when this change was first put on hardware: 26 minutes, zero sports scroll lines. The script drives the path directly with synthetic games and asserts the three things the resolver is meant to buy: the speed lands on whole pixels, the hold is published, and it is released after. It never starts or stops the display service, matching scroll_speeds.py, so a crash here cannot leave the panel dark. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scripts): refuse to grab the panel while the display service has it The module docstring already said to stop ledmatrix first. Nothing enforced it, and running the script against a live service is not a harmless mistake: rpi-rgb-led-matrix configures GPIO directions and the hardware PWM inside RGBMatrix(), and when the root check fails it calls exit() from C with no cleanup. The service keeps rendering and swapping onto pins that have been reconfigured underneath it, so the panel goes black while every diagnostic says the display is healthy -- fresh framebuffer, every pixel lit, "RGB Matrix initialized successfully", nothing in the log. A restart fixes it, once you work out that is what happened. Found the hard way: this is what took the panel down on the test rig, not the change the script was written to verify. --fallback skips the check, since it never opens the matrix. --force is there for anyone who means it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(scripts): annotate the subprocess call the way this repo already does Codacy fails a PR on one new issue, and bandit B404 fires on any subprocess import. scripts/run_plugin_tests.py carries the same suppression with the same justification -- list-form argv, no shell -- so this follows it rather than inventing a second convention. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-09 16:13:04 -04:00
ChuckandClaude Opus 5	4423ec33d5	feat(plugins): search, filter and sort for Installed Plugins, on a shared ListFilter helper (#540) ... * feat(plugins): add search, filter and sort to Installed Plugins, on a shared helper The Installed Plugins grid had no way to narrow it down: no search, no way to see only what's enabled, disabled, or out of date. On a rig with a couple dozen plugins that means scrolling the whole grid to find one. The two sections below it already solved this, twice, independently — the Plugin Store and Starlark Apps carried a copy-paste fork of the same ~600 lines (filter state, apply-filters-and-sort, page renderer, pagination strip, active-filter badge, listener wiring). Rather than add a third copy, this extracts the shared machinery and builds the new toolbar on it. New: web_interface/static/v3/js/plugins/list_filter.js — ListFilter.create() owns debounced search, filter axes, sort, the active-filter count, Clear, and optional pagination/persistence. Callers keep their own card markup via a `render` callback. Three control types cover every axis the page uses: pills (new), select (store category, starlark author) and cycle (the tri-state All -> Installed -> Not Installed button). Installed Plugins gets a compact toolbar: search box, one-click All / Enabled / Disabled / Updates pills, and a sort dropdown (A-Z, Z-A, updates first, recently updated, category). Filters reset on load, so you never come back to a mysteriously short list. No new CSS — this is the first consumer of the .filter-pill rules already sitting unused in app.css. renderInstalledPlugins() is split so it still publishes canonical state while renderInstalledCards() draws only the visible subset; the filtered list is never assigned to window.installedPlugins, which the toggle handler, isStorePluginInstalled(), runUpdateAllPlugins() and the Alpine config tabs all read as their source of truth. Toggling a plugin while filtered pins its card so it doesn't vanish from under the cursor. The Store and Starlark migrations are behaviour-preserving: same element ids, same localStorage keys (storeSort/storePerPage, starlarkSort/starlarkPerPage), same tri-state button markup, same pagination. Verified by differential tests that run the old and new implementations side by side against identical fixtures and compare every observable after each interaction. The only visible change is the pagination attribute (data-store-page/data-starlark-page -> data-list-page), which nothing outside its own click handler referenced. Net -156 lines in plugins_manager.js while adding a feature. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * fix(plugins): keep raw search text, and stop the store search refetching Two review findings from CodeRabbit on #540. Do not write the trimmed search value back into the input. setSearch() trimmed before storing, and syncControls() then copied that trimmed value back over what the user had typed. Pausing longer than the debounce after typing a space deleted the space (and reset the caret), making multi-word terms effectively untypable. The raw text is now kept alongside the trimmed one: filtering and activeCount() still use the trimmed value, while the input keeps exactly what was typed. Remove the legacy #plugin-search / #plugin-category listeners in initializePlugins(). They bound searchPluginStore as the event handler, so the DOM event arrived as its `fetchCommitInfo` argument — always truthy, which skipped the cached-filter fast path and refetched /api/v3/plugins/store/list with commit info on every keystroke burst and category change. The store's ListFilter controller already filters the cached list, which is what those two controls should do. This double-binding predates this PR (the old code guarded with _listenerSetup and _storeFilterInit, two different flags, so both sets stayed live); it is fixed here because the refactor owns that wiring now. Both fixes are covered by tests that fail without them: the trailing-space regressions in the installed-plugins DOM suite, and a new whole-file jsdom test that counts fetches while typing (1 request at init, 0 thereafter; previously 1 -> 2 -> 3 -> 5). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * fix(plugins): build pagination via DOM APIs, drop computed member access Addresses the five Codacy security findings, all in list_filter.js. Pagination no longer assembles an HTML string (3 findings: 2 critical + 1 high, "unsafe assignment to innerHTML"). The interpolated values were only page integers and local class constants, so there was no injection path, but concatenating markup into innerHTML is the pattern the scanners flag and createElement is no less clear. Each button now also owns its click listener directly instead of the container being re-queried afterwards, and the strip is cleared with textContent = '' rather than by assigning empty markup. No innerHTML assignment remains in the file. haystack() now walks Object.entries(item) and keeps the configured fields, instead of reading item[field] per field ("generic object injection sink"). Field order no longer drives the haystack order, which is irrelevant to the substring test. matches() iterates controls with for...of instead of an index ("variable assigned to object injection sink"). The rendered pagination is unchanged: same buttons, labels, page numbers, disabled states and classes. The old-vs-new differential tests now compare pagination structurally (tag, text, page, disabled, sorted class list) rather than as an HTML string, since building nodes legitimately serialises differently — «/» as characters rather than &laquo;/&raquo;, disabled="" rather than a bare attribute. That comparison is stronger than the string one it replaces, and the real-DOM suite still drives the actual page buttons. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * fix(plugins): keep configured field order when building the search haystack The previous commit swapped item[field] for Object.entries(item) to clear a static-analysis object-injection warning, and in doing so changed the order of the haystack: entries follow the object's own key insertion order, not the configured `fields` order. Since the values are concatenated, that order decides which values end up adjacent, so a multi-word query spanning a field boundary matched differently. For store fields [name, description, author, id, ...] and API objects keyed {id, name, description, author, ...}, "bob plugin-01" matched before and stopped matching after. That contradicted the behaviour-preservation claim for the store and starlark migrations, and the differential tests missed it because every fixture query was a single word. Values now come out of a Map built from Object.entries, iterated in `fields` order: the original haystack is restored, and there is still no computed member access for the analyser to flag. Regression coverage for the ordering itself, at both levels: - unit: phrases spanning name->id and category->tags, plus the reverse (object-key) order asserted NOT to match - differential: the same class of query compared old-vs-new, with a guard that the phrase actually matches something so a mutual zero-result cannot pass vacuously Verified both fail without this fix (3 unit, 2 differential) and pass with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * test(web): add JS suites for ListFilter and the plugin-manager grids No JS toolchain exists in this repo, so these are plain node scripts with no framework: each prints ok/FAIL lines and exits non-zero. `node test/js/run_all.js` runs everything, skipping the DOM suites (rather than failing) when jsdom is absent or nothing is listening, so it stays useful in a bare checkout. unit/test_list_filter.js ListFilter search/filter/sort/count/sticky, driven through the installed-plugins config eval'd verbatim out of plugins_manager.js so the test cannot drift from the real configuration unit/test_render_cards.js renderInstalledCards markup, both empty states, and escaping of hostile plugin metadata dom/test_installed_dom.js the toolbar in a real DOM, including the HTMX partial re-swap and a getComputedStyle check that .filter-pill[data-active] matches what we emit dom/test_store_dom.js store pagination, per-page, category, tri-state Installed button, persistence across a re-boot dom/test_no_double_fetch.js loads the whole plugins_manager.js and counts requests, so a keystroke cannot refetch the store The DOM suites deliberately fetch the partial and the plugin data from a running web interface instead of using fixtures, so a renamed element id or a changed payload shape fails them loudly. Point them at a rig with a full plugin set when it matters (BASE=http://host:5000); a dev box with two plugins installed passes while exercising very little. Several assertions exist to stop specific bugs recurring: trailing spaces surviving the search debounce, a query spanning two adjacent search fields (haystack field order is load-bearing), and window.installedPlugins staying at full length while the grid is filtered. Others guard against passing vacuously — counting only non-skeleton cards, and checking a search phrase matches something before comparing two result sets. The old-vs-new differential suites that verified the store and starlark migrations are not included: they compared against the pre-refactor code, which now exists only in git history. 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>	2026-09-08 20:04:47 -04:00
ChuckandClaude Opus 5	26769ee37f	fix(starlark): the store authenticated with a key nothing writes (#541) ... #535 restored the thirteen routes, so the store stopped answering 404 -- and still would not load. Confirmed against a running device before anything was changed: /repository/browse answers 200 with 1000 apps in 27s, so the routes are fine. Two things underneath them are not. **The store never used the token the user configured.** The three repository routes read `github_token` off config.json. Nothing writes that key -- it is not in config.template.json, no setting offers it, and it appears nowhere else in the codebase. The configured token goes to config_secrets.json as `github.api_token`, which PluginStoreManager loads and every other GitHub caller uses. So the store could never be authenticated: 60 requests/hour, on the same per-IP budget 48 installed plugins spend on update checks, while the 5000 the user had already configured sat unused. On the device, /plugins/store/github-status reported authenticated with a limit of 5000 at the same moment /starlark/repository/browse reported 60, with 18 left. The store going blank was that 60 running out. **Every failure looked identical.** list_all_apps_cached turned any listing failure -- rate limit, DNS, timeout, non-200 -- into an empty app list, and the route sent that out as `status: success`, so a rate limit and an empty repository drew the same blank grid with no error anywhere. It now returns the reason, the route answers 502 with it, and a failure is no longer cached as an empty repository for two hours. The guard for a bad response was itself a crash: _make_request catches `(json.JSONDecodeError, ValueError)` but `json` was never imported, so evaluating the tuple raises NameError and the guard written for exactly this case never ran. Reachable whenever something on the path answers with HTML -- a captive portal, a proxy page, a DNS-hijacking router. Seventeen handlers answered 5xx with no detail at all. test_no_api_v3_handler_discards_its_exception is meant to prevent that across api_v3, but it matched one exact message string, and all thirteen Starlark routes wrote their own wording. The guard now keys on the shape that matters: if it returns 5xx, it says why. The 15 pre-existing non-Starlark functions are listed as a set that may shrink, never grow. **The listing was capped at 1000 and did not say so.** The contents API truncates a directory silently; tronbyt/apps has 1075 app directories, so the store showed a truncated repository and looked complete doing it. Now listed via the git trees API, which reports `truncated`, with the contents API kept as a fallback. Not addressed: the 27-second cold load -- 1075 manifests fetched five at a time behind skeleton placeholders -- which is probably the largest part of what "does not load" feels like, and wants its own change. 25 new tests. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-08 20:04:27 -04:00
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>	2026-09-08 09:40:00 -04:00
ChuckandClaude Opus 5	e23f1f45d3	feat(starlark,on-demand): the third-party fixes worth taking, plus a Home Assistant MQTT bridge (#538) ... * feat(starlark,on-demand): the third-party fixes worth taking, plus an MQTT bridge Analysis of ant456/ledmatrix-fixes-repo, a third-party collection of patches and services built while running this project on Starlark apps under MQTT control. Its patches are whole-file copies taken against an older tree, so applying them as written would revert #523's frame pacing, #534's display() bool returns and the GitHub token masking in plugins_manager.js. Three of its claimed fixes are already in main, and its api_v3 Starlark routes are #535's. What follows is the rest -- verified against current code, and reimplemented where the patch's approach did not hold up. **On-demand display.** `pinned` reached the controller from the API, was stored on it and republished in the status payload, but never narrowed the rotation -- a pinned request still cycled every mode its plugin owns. Right for a sports plugin, whose modes are views of one subject; wrong for a plugin whose modes are unrelated, which is every Starlark app. Now honoured, and it survives a restart. Restarting while on-demand was active loaded *only* the on-demand plugin, so normal rotation had nothing to return to for the life of the process -- and a restart mid-session is routine, since that is how an update is applied. The panel came back cycling one plugin's modes with no way out but clearing the cache by hand. Every enabled plugin loads now; on-demand still resumes on its saved mode. Stop requests are exempt from the duplicate guards on purpose, so that a second click stops a mode a race left running -- which means consuming the mailbox is the only thing that ends one. It was never consumed, so the same stop was re-read and re-processed on every poll, forever. Both paths now share one compare-before-delete helper. **Starlark rendering.** `extract_schema` parsed the source with a regex, which can only see option lists written out literally: an app whose dropdown is filled from a live API call inside `get_schema()` came back empty, and the config form offered nothing to pick. Now runs `pixlet schema`, which executes the app, and falls back to the parser when Pixlet is absent, too old for the subcommand, or the app fails to run. The third-party patch replaced the parser outright and hardcoded /usr/local/bin/pixlet; this keeps the fallback and the binary search. A `|` in a config value was dropped by a shell-metacharacter filter, though the command is a list with no shell involved -- and apps do use it as a separator inside one value. The key went missing silently and the app rendered its own "not configured" screen with nothing to say why. And a 0-byte render was reported as success: Pixlet exits 0 and writes nothing when an app has no content, which read downstream as a working app drawing a black panel. **Starlark display.** `display()` ignored the mode it was called with, so a specific app could not be addressed. It now accepts `display_mode` -- which is the whole mechanism, since the controller inspects the signature before passing it. Found while there: `_select_next_app` ran only while `current_app` was unset, so with several apps installed the first was picked once and shown forever while the rest were rendered on schedule and never displayed. And `enable_scrolling` was missing, so multi-frame apps were called once per rotation slot and never advanced past frame one. **GET /api/v3/display/modes.** Every mode that can be requested on-demand, with the plugin that owns it. Nothing exposed this, so anything driving the display from outside the web UI read each plugin's manifest.json off disk and reimplemented PluginManager's fallbacks. It also triggers discovery, which is otherwise lazy and normally happens because a person opened the dashboard. **integrations/mqtt_bridge.** Home Assistant control over MQTT Discovery: a mode select, a stop button, power, brightness. Rewritten against the API rather than the filesystem, so it needs no read access to config.json and cannot drift from the web UI. paho-mqtt 2.x VERSION2, TLS, an availability topic that is also the last will, and secrets from the environment. **Two opt-in extras.** A DNS single-request unit, for glibc's parallel A/AAAA lookup stalling ~5s per name on routers that answer only the A query -- which makes any plugin calling an external API slow and Starlark apps, which have a render timeout, fail outright. And a Pixlet config editor: a script you run and Ctrl+C rather than the third-party version's always-on unauthenticated Flask service, since it stops the display for the length of a session. Neither is installed by default. Long Starlark app names now wrap instead of overflowing their card. 115 new tests across 5 files. Also unblocked test_starlark_display_contract.py, which was silently skipping wherever fcntl is absent. Whole suite: no new failures against main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(mqtt_bridge): the five issues Codacy flagged on this branch All in the new bridge, all real: * requests floor was 2.31.0, which carries CVE-2024-35195, CVE-2024-47081 and CVE-2026-25645. Raised to >=2.33.0,<3.0.0, which is what the project's own requirements.txt already pins. * `import time` was never used. * `"mqtt_password": None` in DEFAULTS read as a hardcoded credential. It is the "no password configured" default; marked nosec B105, the convention used elsewhere in the repo. Also dropped an unused `build_app` from the display-modes test imports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: the review findings on this PR Nine of CodeRabbit's ten, plus the CodeQL alert. The tenth is wrong and is answered below. **One bad config section blanked the whole mode list.** `/display/modes` read `full_config.get(plugin_id, {}).get('enabled')`, so a non-dict under a plugin id -- a shape DisplayController already guards, so it happens -- raised AttributeError mid-loop and answered 500 with no modes at all. Every MQTT bridge entity is built from that list. Now skipped with a warning. **The DNS scripts reported success they had not earned.** Three separate paths: `resolvconf -u` failing was swallowed by `|| true`; the systemd-resolved branch exited 0 without applying anything, so the oneshot unit recorded success while the workaround was inactive; and the installer's `|| echo` turned a failed start into "installation complete." with exit 0. All three now fail loudly. `single-request` is a glibc resolv.conf option with no resolved.conf equivalent, so on those hosts the honest answer is that it cannot be applied. A NetworkManager-generated resolv.conf is regenerated on connection changes, not only at boot, and the unit is oneshot with RemainAfterExit -- so the option can vanish mid-boot with nothing to put it back. Now detected and stated plainly rather than implied to be permanent. **`Before=` does not order a manual restart.** It only orders units already in the same transaction, so `systemctl restart ledmatrix` could bypass the fix. install_dns_fix.sh now writes a ledmatrix.service drop-in with Wants= and After=. Wants=, not Requires=: a DNS workaround failing should not stop the display. **The Pixlet editor's `--lan` is gone.** `pixlet serve` has no authentication, and a printed warning is not access control. Loopback only, with the SSH port-forward in the header where the flag used to be documented -- SSH does the authenticating and nothing is left listening. **The MQTT example config now defaults to TLS** on 8883. The installer copies it verbatim, and without TLS the broker password and every command cross the network in cleartext. A plaintext broker is still supported and documented, and the bridge warns once at startup when a password is configured without TLS. **Not taken: "the upstream Pixlet CLI has no `schema` subcommand."** Upstream tidbyt/pixlet has none, but `scripts/download_pixlet.sh` installs `tronbyt/pixlet`, whose `cmd/schema.go` is `schema [PATH]` -> JSON on stdout, built on `runtime.NewAppletFromPath`, so it does execute `get_schema()`. That is exactly what extract_schema_via_pixlet calls. A binary without the subcommand exits non-zero and falls back to the source parser, which is already covered by a test. **CodeQL stack-trace exposure: not taken either.** I removed `details` first and that broke test_web_error_detail.py::test_no_api_v3_handler_discards_its_exception, which enforces `describe_exception` across all ~75 handlers -- written because a device with failing storage answered "see logs for details" from the log viewer itself. describe_exception redacts credentials; the trade-off is the project's and is already made. Restored, with the reasoning in a comment. 11 new tests. Whole suite: no new failures against main, 4127 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-08 08:34:52 -04:00
ChuckandClaude Opus 5	793b988d33	fix(starlark): toggle the app id the list published, and store a relocatable star_file (#537) ... The two review nitpicks left over from #535. Both are still on main after that merge; the five findings alongside them landed with it. **The toggle could not find what the list had just shown.** `_starlark_virtual_plugins` publishes the raw manifest key as `starlark:<key>`, and `_toggle_starlark_app` passed it back through `_validate_and_sanitize_app_id`, which lowercases and rewrites every character outside `[a-z0-9_]`. An app stored as `My-App` was listed as `starlark:My-App` and looked up as `my_app`, so toggling an app the page had drawn a moment earlier answered 404. Keys written by `_install_star_file` are already sanitised, so this only shows up for manifests written by the starlark-apps plugin itself or edited by hand. `_validate_starlark_app_path` rejects traversal without rewriting, so it is the check to use here -- listing and toggling now agree on one key. The updater also uses `setdefault` rather than indexing: the app is loaded but its on-disk entry need not exist, and `_update_manifest_safe` does not catch `KeyError`, so that escaped as a 500 rather than writing the entry. **`star_file` was stored absolute.** Readers join it to the app's own directory -- `_standalone_render_starlark_app` does `app_dir / app_data.get('star_file', f'{app_id}.star')` -- so the key's default is a bare filename and an absolute value gave it a second meaning. Since `Path.__truediv__` discards the left side when the right is absolute, the manifest was pinned to whatever PROJECT_ROOT installed it, and a moved or redeployed install could not find its own file. Storing `dest.name` matches the default and stays relocatable. Read paths are unchanged, so manifests already holding an absolute path keep working. 7 new tests. Whole suite: no new failures against main, 4013 passed against 4007. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-07 16:35:56 -04:00
Chuck	50258635a8	fix(starlark): restore the API routes #330 dropped (#535) ... The Pixlet install button reported "Pixlet install failed: Resource not found" -- Flask's 404 handler, because the route did not exist. #253 added thirteen Starlark routes; #330 rewrote api_v3.py and dropped all of them, along with the `starlark:<app_id>` entries that surface installed apps in the plugins list and the toggle branch that enables them. Restores all thirteen routes, the plugin-list entries and the toggle path, so Pixlet installs, the app store browses and installs, and an installed app can be managed like any other plugin. Not a straight revert. Three error paths stopped returning exception text to the caller; the manifest write moved off a shared temp filename that two concurrent writers could interleave; both dynamic importers stopped leaving half-initialised modules in sys.modules; the config update rolls back when the save fails; the toggle checks that persistence succeeded; and the path check returns the validated path instead of a boolean so callers stop re-joining the raw value. New tests no longer reach GitHub. Verified on a 256x64 Pi: Pixlet installs and runs (v0.53.1), the store lists 1000 apps, install/toggle/uninstall round-trip, and traversal and command-injection probes are rejected at every entry. 25 CodeQL alerts dismissed as verified false positives -- path-injection where traversal is blocked, and one list-form subprocess with no shell. Both classes already present on main. Full core suite: 3981 passed.	2026-09-07 16:06:09 -04:00
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>	2026-09-07 15:55:46 -04:00
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>	2026-09-07 13:37:54 -04:00
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>	2026-09-07 13:37:38 -04:00
3 people	696acdbc7b	feat(render_plugin): add --display-mode so multi-mode plugins can be rendered (#522) ... * feat(render_plugin): add --display-mode so multi-mode plugins can be rendered render_plugin.py always called plugin.display(force_clear=True) with no mode. A plugin that declares one display mode is fine, but the sports scoreboards declare three or more and keep their per-mode state on sub-managers; their no-argument path selects nothing and returns False, so the render came out blank with nothing to say why. Measured on nrl-scoreboard with identical seeded state: live.display() directly True, 1892 lit pixels plugin.display(display_mode="nrl_live") True, 1892 lit pixels plugin.display() False, 0 lit pixels --display-mode passes the requested mode through. It is only passed when asked for, so the many plugins whose display() takes no display_mode keep working untouched, and a plugin that declares modes but does not accept the argument degrades to its default screen with a warning rather than a TypeError. This is what lets the plugin READMEs show a scoreboard at all, and it also unblocks screens like birdnet_stats and the weather plugin's hourly, daily and almanac modes, which could previously only be described in prose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Only fall back when plugin display rejects display_mode --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>	2026-09-06 17:28:22 -04:00
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>	2026-09-04 16:02:04 -04:00
Chuck	6bea1a7c21	fix(sports): say when the schema cannot be read, instead of failing silently (#520) ... _schema_font_size swallowed every exception and cached an empty dict. That is not cosmetic. With no schema, a configured font size can no longer be compared against the schema default, so every size is treated as a deliberate user choice and skips the snap to the font's pixel grid -- which renders 4x6-font.ttf at 6 instead of 7: a 3px-wide glyph instead of 4px. That shipped. On a 256x64 panel it made the odds, the team records and the date row hard to read, and it was found by a user counting pixels on a photo of the panel rather than by anything here. The cause (_plugin_dir returning None under the real plugin loader) is fixed in #519; this makes the same class of failure audible next time: Orphan: could not read config_schema.json (FileNotFoundError: ...); every font size will be treated as user-chosen and will skip its pixel grid snap. Font sizes may render a pixel narrow. The message names the consequence, not just the error, because the error alone does not suggest "your fonts are a pixel narrow". Logged rather than raised: an unreadable schema must not stop a plugin rendering. The cache is built once per class (per schema path in sports_card), so this cannot repeat per frame. Scope deliberately small. An audit of the three shared modules found 23 handlers that swallow and return a default, but all 23 catch specific types -- TypeError, ValueError, ImportError -- turning bad config values into defaults, which is what they are for. Of 77 broad handlers across the font and odds paths, 74 already log. Only these two were both broad and silent.	2026-09-04 16:01:50 -04:00
Chuck	0730d95200	fix(sports): let the plugin declare its own directory, don't deduce it (#519) ... _plugin_dir() returned None on every device. The consequence was silent and reached the panel: _plugin_dir() -> None _schema_font_size() -> None for every element -> a configured size equal to the schema default stops looking like a default and is treated as a deliberate user choice -> the snap to the font's pixel grid is skipped -> 4x6-font.ttf renders at 6 instead of 7: 3px-wide glyphs, not 4px On a 256x64 panel that made the odds, the team records and the date row hard to read. Both `odds` and `detail` were affected -- anything resolving a grid-snapped schema default was a pixel narrow. Why it was invisible here. PluginLoader._namespace_plugin_modules renames every bare module a plugin brought in (sports, game_renderer, ...) to "_plg_<plugin_id>_<module>" and REMOVES the bare sys.modules entry, so two plugins owning a module of the same name cannot collide. A class defined in sports.py still reports __module__ == "sports", but sys.modules["sports"] is gone, so walking the MRO for a module with a __file__ finds nothing. Every test here imported plugins directly, which leaves the bare entry in place, so the walk succeeded. The safety harness loads plugins its own way and never reproduced it either. It was found by a user counting pixels on the panel. The directory is now declared by the plugin (_PLUGIN_DIR) and only deduced as a fallback, for hosts that declare nothing -- the plugins' own probe harnesses build classes with type(). Verified on hardware, which is the only place the original failure appeared: before, the live service logged plugin_dir=None and 4x6-font.ttf@6 for all six football managers; after, plugin_dir resolves and both odds and detail are @7. Five regression tests, including the production shape: a class whose __module__ is absent from sys.modules still resolves via its declared directory, and the precondition that the MRO walk alone returns None is pinned so the test keeps meaning something if the fallback changes.	2026-09-03 17:01:30 -04:00
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. v3.3.1	2026-09-03 16:06:40 -04:00
ChuckandClaude Opus 5	bc2dbf3824	feat(sports): share the sports.py surface that is identical in all eight scoreboards (#515) ... * feat(sports): share the sports.py surface that is identical in all eight Nine scoreboards ship their own sports.py -- 41,326 lines. Comparing executable ASTs across the eight that share a lineage, 48 method bodies are byte-identical in every one: 1,007 lines carried eight times, so 8,056 lines that must be edited eight times to fix once. They are the parts with no sport in them: the selection and rotation engine (_round_robin_favorites, _favorites_first, _compose_selection, _check_ranking_coverage, _game_divisions, _normalise_quality), the font/colour/ date subsystem (_scale_headline_fonts, _scorebug_font, _resolve_font_size, _format_game_date, _font_color), and the switch-mode upcoming card (_draw_upcoming_center_switch). Nothing here knows what an inning is. Mixins rather than free functions: every one of these reads host state, so rewriting 48 bodies into free functions would be a rewrite rather than a move, and it is the move that keeps the renders identical. Three of the 48 are deliberately left in the plugins, because a byte-identical body is not automatically safe to move: - _get_timezone calls resolve_timezone, imported from a per-plugin module (hockey_timezone, soccer_timezone, ...). All eight of those differ -- each carries its own _WRITEBACK_FIXED_IN -- so hoisting the caller would silently bind every scoreboard to one plugin's copy. - _extract_game_details and _fetch_data are @abstractmethod stubs. They are the sport contract; satisfying them from a mixin would let a plugin instantiate without implementing its own sport. _resolve_font_path went the other way: a module-level function, identical in all eight, that _scale_headline_fonts needs -- so it is inlined here. _schema_font_size needed a real change rather than a move. It located the plugin's config_schema.json with __file__, which here is src/common/, so the load failed silently, the cache stayed empty and every element fell back to an unsnapped size -- measured at 81% anti-aliased edges on a panel that should be pixel-crisp. It now recovers the plugin directory from the instance. Note that type(self).__module__ alone is not enough: SportsCore is an ABC, so a subclass built with type(name, bases, ns) -- which the plugins' own tests do -- reports its module as "abc". _plugin_dir walks the MRO past those synthetic classes to the first module sitting beside a config_schema.json. Worth recording: the 176 harness renders did NOT catch that regression. The plugin's own test_fonts_are_crisp.py did. Renders alone were not a sufficient gate here. Not merged with src/common/sports_card.py despite fourteen same-named twins. Only five are provably equivalent by source comparison; the other nine differ in ways inspection cannot settle, and a wrong guess silently changes what every scoreboard draws. That merge needs differential testing and is its own change. * fix(sports): declare the constants the mixins read, and test the contract CodeRabbit found _QUALITY_CHOICES and _RANKING_COVERAGE_SECONDS read by _normalise_quality and _check_ranking_coverage but never defined on a mixin. Confirmed: both are declared by all eight scoreboards, so nothing fails today -- it would only have bitten the ninth plugin to adopt this, at runtime, mid-render. Both are identical everywhere, so they get defaults here; each plugin's own copy still shadows them. Auditing for others showed those two were the only ones, but also that the host-contract docstring was substantially incomplete: it listed 21 attributes where the mixins actually read about 40, and omitted five hooks (_is_favorite_game, _is_game_really_over, _is_ranked_game, _passes_other_filters, _get_timezone). The section is now derived from that audit rather than remembered. test_sports_shared.py covers what is genuinely new, not the moved bodies: - The contract itself. It parses the module for every ALL-CAPS `self.X` the mixins read and asserts each is defined, so the next omission fails here rather than in the field. - _plugin_dir, the only new logic in the move. Including the case that made it necessary: SportsCore is an ABC, so a subclass built with type(name, bases, ns) -- which the plugins' own tests build -- reports __module__ as "abc". The test asserts that precondition before asserting the walk steps past it. - The three SportsLive bodies. Hockey and lacrosse disable live mode in their harness fixtures, so the 176 renders never reach this path; testing the mixin directly means coverage no longer depends on which plugin happens to have a unit test. Two of those tests pin things that would otherwise be silently undone. SportsRecentSharedMixin does carry an __init__ -- SportsRecent.__init__ was one of the 48 byte-identical bodies. Its bare super() binds to where it is defined, now the mixin, so it only reaches the host because the mixin is listed first in the bases. One test proves the chain runs; the next proves that reversing the order silently skips the host constructor. * fix(sports): drop three unused imports and let the matcher narrow Codacy flagged five issues on this file. Three are unused imports: math, abc.abstractmethod and zoneinfo.ZoneInfo. Nothing in the module references any of them -- the timezone work goes through pytz, and the @abstractmethod mention in the module docstring describes the two stubs that deliberately stayed behind in each plugin, not anything declared here. pyflakes agrees; all three are removed. The other two are "team_in is not callable" on the round-robin favourite matcher. That call is already guarded by callable(), so it cannot raise at runtime, but callable() is not a narrowing construct a static analyser follows: the name still carries the None from getattr's default. Normalising a non-callable to None and branching on `is None` gives the analyser a test it does understand, and keeps the guard. Behaviour is unchanged. _round_robin_favorites has no test coverage, so I exercised it directly on both paths -- a host with _team_in (id matching, the NRL case) and one without (abbreviation matching) -- across limits 1 to 4, and the selections are identical before and after. A host whose _team_in is present but not callable still falls back to abbreviation matching rather than raising. test/test_sports_shared.py: 27 passed. The 9 collection errors under `pytest test/ -k sport` reproduce identically on the unmodified branch and are not from this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> v3.3.0	2026-09-03 13:29:37 -04:00
Chuck	cb0545ecb3	release: report 3.3.0, so the version the gate reads matches the tag (#516) ... v3.3.0 is tagged, but src/__init__.py still says "3.2.0" -- and that string, not the git tag, is what the compatibility gate compares (store_manager.py: `from src import __version__ as core_version`). The effect is that every plugin flooring at 3.3.0 is refused on a device running 3.3.0. Checked against the real gate and the real manifest: core __version__ reported to the gate : 3.2.0 hockey floor : 3.3.0 verdict : REFUSE "supports LEDMatrix >=3.3.0, but this system is running 3.2.0" That is all eight sports scoreboards plus calendar 1.2.3, and it would read as a broken plugin store rather than a stale constant. The TRUSTWORTHY_FLOOR escape hatch does not cover this: it exempts cores reporting below 2.0.0 as "unknown rather than old", and 3.2.0 is above it, so the number is trusted and compared. This is the same slip as v3.1.0, which was tagged six weeks before its version string was bumped and shipped __version__ = "1.0.0" -- the reason that escape hatch exists at all. With the bump, the same gate call returns ALLOW for hockey at both the sports_card and sports_shared floors, and for calendar 1.2.3. CHANGELOG.md gains the 3.3.0 section. That file is what plugin authors read to decide which release to floor on, so it records the three new modules -- src/common/sports_card.py, sports_game_renderer.py and sports_shared.py -- against this version, along with the three traps in adopting the mixins. No test pinned the old literal; the ones that care monkeypatch __version__. 135 compatibility tests pass.	2026-09-03 10:32:08 -04:00
Chuck	300cdaa250	feat(sports): share the scroll-card geometry the scoreboards all duplicate (#514) ... * feat(sports): share the scroll-card geometry the scoreboards all duplicate The card helpers moved to src/common/sports_card.py, which shared the eight scoreboards' settings lookups. Their *geometry* stayed duplicated: nine methods deciding how wide the centre strip is, how much room each logo gets, and where an upcoming card's date and time land. Five were byte-identical in all eight plugins; the other four were identical in seven, each with a different single outlier. That shape is why this is a mixin and not free functions. Comparing executable ASTs against the eight plugins, 67 of the 70 method bodies are inherited unchanged and 3 become ordinary overrides -- baseball keeps its own _logo_slot_width and _draw_upcoming_game_status, hockey its own _upcoming_date_and_time. No per-sport branching goes inside the base. It deliberately has no __init__ and no state. The plugins' constructors differ six ways and none of it is worth unifying, so adoption is one line on the class statement plus deleting what now comes from here. Placed in src/common/ rather than src/base_classes/sports/ on purpose: importing that package pulls core.py -> DisplayManager -> rgbmatrix, and this is pure geometry that must not drag a hardware import into every plugin that uses it. It sits next to sports_card.py, which the same plugins already use. Only _SCORE_PROBE varies between plugins, so leagues that reach three digits a side override that one ClassVar; the four gap constants are identical everywhere. The tests drive the mixin through a host that provides exactly the surface the module docstring names and nothing else, so the mixin growing a new self.* dependency the plugins do not have fails the contract test rather than shipping. * fix(sports): reject non-finite card settings before they abort the render A center_gap of inf passes `isinstance(x, (int, float)) and x >= 0` unharmed and then raises OverflowError out of int(). The surrounding guards caught only (TypeError, ValueError), so it escaped and took the whole card render with it. The same holds for center_gap_ratio, the two clamp bounds, and layout offsets, where "inf" arrives as a string and float() is happy to produce it. Four of the five paths crashed; only a NaN ratio happened to survive, by accident of min/max rather than by design. This is pre-existing behaviour -- the bodies moved here verbatim from the eight plugins and every one of them has it today. Fixing it in the mixin fixes it in all eight at once, which is the argument for the mixin. Guarded with math.isfinite() before any int()/round(), falling back to the same defaults the finite paths already use, plus OverflowError added to the except clauses as a backstop. Ordinary settings are untouched: all 192 scroll-card renders (8 plugins x 8 panel sizes x 3 game types) stay byte-identical to pristine main. Found by CodeRabbit on #514 and confirmed by running it before fixing.	2026-09-02 16:22:25 -04:00
ChuckandClaude Opus 5	10da2f97fd	feat(sports): share the card helpers the eight scoreboards each carried (#513) ... Twenty methods were byte-identical in all eight scoreboards' game_renderer.py: the colour pickers, the scroll_card settings lookup, the date and time formatting, the favourite-team rules and the font-size grid snapping. Every fix to any of it had to be made eight times, and a new scoreboard began by copying them a ninth. src/common/sports_card.py holds them once. 245 lines leave each plugin. **Free functions, not a base class.** Every helper takes config/logger/fonts as arguments rather than reading them off an instance, so a plugin keeps its method and delegates the body -- call sites, signatures and override points are all untouched. Adoption is therefore per-function and reversible, which is what let all eight move with byte-identical renders. The bodies are the plugins' code moved, not rewritten. Two deliberate differences, both verified: - crisp_size takes the seven-plugin guard (`not desired`) rather than football's. They agree on every real input; the extra guard only stops a None size raising TypeError, so adopting it is a no-op for seven plugins and removes a crash path for the eighth. - schema_font_size caches per schema PATH. The plugins cached on their own class, which is the same distinction expressed without a class to hang it on; two plugins never share an entry. The path has to be passed in because the plugins derived it from __file__, and __file__ here is the core's. Verified before any plugin was touched: 534 differential comparisons of the helpers against afl's originals and 704 more of the font-sizing chain against all eight plugins' originals -- 1,238 comparisons, zero differences. Writing the constant tables by hand introduced two errors that check caught: the tie colour was (255,255,0) instead of the plugins' (255,200,0), and a "five_by_seven" alias that does not exist. Both are now taken from the plugins verbatim. 43 tests pin the contract, including the cases the plugins' own comments record as having bitten: a three-character string must not iterate into a colour, a shared font face must give up rather than guess an element, a font_size equal to the schema default carries no intent, and a bad timezone falls back to UTC rather than blanking the card. Full suite 3768 passed, 6 skipped. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-09-02 16:22:09 -04:00
ChuckandClaude Opus 5	92f9d06af9	fix(logos): stop a failed download pinning a team to a grey box forever (#512) ... * fix(logos): stop a failed download pinning a team to a grey box forever When a logo download fails, create_placeholder_logo writes a 64x64 grey PNG under the *real* logo's filename. Every later call then hits `if filepath.exists(): return True` and reports success, so the real logo is never attempted again. One transient failure -- no network at boot, ESPN blipping -- permanently costs that team its logo. This is not hypothetical. Five of the eleven cached AFL logos in my checkout were 384-byte stubs written in a single bad minute, and they had stayed that way ever since; the scoreboard rendered COLL, FRE, NMFC, PORT and SYD as grey text boxes on every card. Placeholders are now stamped with a `ledmatrix_placeholder` PNG text chunk carrying their creation time, and `is_placeholder_logo` recognises them. It also matches on the placeholder's exact geometry and background colour, so the stubs already sitting on users' disks are picked up too -- without that, this fix would only help teams whose logos break in future. Verified against the real stubs: all five detected, all six real logos untouched. `download_missing_logo` now treats an existing placeholder as the failed download it is and retries, rather than as a satisfied request. The retry is rate-limited to PLACEHOLDER_RETRY_SECONDS (6h) so this does not trade a permanent grey box for an ESPN request every frame; a failed retry rewrites the placeholder, restarting the clock. The age comes from the stamp rather than mtime, so a backup restore, an rsync, or a permissions script cannot silently reset it. `download_missing_logos_for_league` gets the same treatment -- a bulk pass is exactly where a previously failed logo should get another chance -- and `LogoHelper.load_logo_with_download` no longer accepts a stale placeholder as a cache hit. That import is lazy and guarded so the module still works against a core build predating the marker. `LogoHelper._create_placeholder_logo` needs no change: it returns an in-memory image and never writes it to disk, which is the behaviour this bug argues for. Tests cover marked and legacy-unmarked detection, the two false-positive cases (a real 500x500 logo, and a 64x64 image that is merely the same size), the retry, the rate limit, and that the age survives an mtime touch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(logos): address review — unify eligibility, invalidate cache, restart back-off Three findings from the review on #512, all confirmed against the code: 1. The three download sites each had their own idea of "already have it". download_missing_logos_for_league() retried *any* placeholder, ignoring the back-off entirely, while download_all_ncaa_football_logos() was never updated and still skipped placeholders forever. They now share one should_attempt_download(), which also covers force_download, so the sites cannot drift apart again. download_missing_logo() reads through the same helper. 2. LogoHelper.load_logo_with_download() answered from the in-memory cache before touching the disk, so after a stale placeholder was successfully replaced the *cached placeholder image* was still returned -- the real logo would not have appeared until the process restarted. The cache entry for that file (every size of it) is now dropped after a successful download. 3. A failed retry left the stale placeholder on disk with its old timestamp, so the next call saw it as stale again and retried immediately: a download attempt per call, which is precisely what the back-off exists to prevent. refresh_placeholder_timestamp() restamps it, and the helper calls that on the failure path. It refuses to touch anything that is not a placeholder. Tests cover both bulk loops in both directions (fresh placeholder skipped, stale one retried), the eligibility rule including force_download, the timestamp refresh, and the two LogoHelper paths -- including that a freshly-downloaded logo is actually what comes back rather than the cached placeholder. Two of the new bulk-loop tests initially passed for the wrong reason: the fetch_teams_data stub returned {}, which is falsy, so the loops bailed before reaching the eligibility check at all. Fixed to return a truthy payload. Re-verified end to end: with both halves in place, rendering the AFL scoreboard took FRE.png from a 362-byte stub to a 12,928-byte logo. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-02 13:21:07 -04:00
4 people	6e361e05cc	docs(sports): record B6 as done, and what running it found (#435) ... * docs(sports): record where B6 stands, and why it is waiting The phase table had B4 as "next" and B5 as "after B4" while both had shipped, and described B6 as blocked on B4's gate — which is now merged and released. A plan that misreports which phase it is in is worse than no plan: the next person reads it and repeats finished work. Corrected, and three things that were only ever decided in conversation are now written down: * **B6 is deliberately held.** 3.2.0 published 2026-08-03; 3.1.0 ran nine months before it. B6's premise is that cores without the module are gone, and there is no release-asset count or install telemetry to show that. Running it now strands users on their current plugin versions. The gate that makes it safe is already built and tested — it is the calendar that is missing, and no amount of further code changes that. * **Stop adopting further shared modules** (data_sources, game_renderer, base_odds_manager) until B6 closes. Each adoption adds a copy to keep in step against a payoff contingent on B6. * **A B5 retrospective**, because "the adoption went fine" is not what happened: four of eight shipped with scroll mode broken on a 3.2.0 core. The bundled fallback did not protect against it — the break was on the modern path — which is an argument for the sunset, not against it. Records the ledger too: net negative on disk until B6 runs. Also replaces the "what's next" list, whose first five items were all done, with what actually remains. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix: apply CodeRabbit auto-fixes Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai> * docs(sports): stop a wrapped PR reference reading as a heading A line wrapped onto "#433), the newest manifest entry ...", which markdownlint reads as a malformed ATX heading (MD018). Reflowed so the line starts with "(#431, #433)" instead. Not the suggested fix: adding a space after the hash would have turned the PR reference into "# 433". The B5 safety claim raised alongside this was already corrected in ac44b5a. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * docs(sports): scope the B5 safety claim to the fallback The heading read "B5 — adoption is safe by construction", which this same document disproves two sections later: four of the eight adopted plugins shipped with scroll mode broken on a 3.2.0 core and were repaired in plugins #251. The body was already careful -- it says fallback compatibility is what is guaranteed, and that correctness on a core which *does* ship the module needs object-level and scroll-mode validation. The heading was not, and a heading is what a reader scanning the plan actually takes away. Retitled to name both halves, with a sentence up front saying why the unqualified claim is false and pointing at the retrospective that shows it. The phase intro said "one of them is safe by construction and the other is not"; that now says what it actually means -- one cannot break a user on an old core, the other can. The second review point, MD018 on the ATX heading at line 409, does not reproduce: that line now begins "(#431, #433)" rather than "#433)", so there is no bare-hash heading. `grep -cE '^#+[^ #]'` returns 0 for the whole file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * docs(sports): re-check the hold, and close two items that are already done The remaining-work list had two entries that finished without the doc noticing, which is the failure mode this file exists to prevent. - The stale plugin-test tranche is gone. run_plugin_tests.py --all now reports 174 passed, 2 skipped, 0 failed across the whole fleet. Recorded how to re-check it too: these are standalone scripts, not a pytest suite, and one calls sys.exit(1) at import, so pointing pytest at a plugin directory collapses into an INTERNALERROR that looks nothing like the real state. - CLAUDE.md already says eight panel sizes. That leaves the hardware soaks as the only open item needing work rather than calendar time. B6's prerequisite is now built -- core test/test_sports_sunset_matrix.py (#505) -- so the phase table and the regression-test section say so, and the two modelling traps it had to work through are recorded for whoever touches it next: the copy-removed shape must be an unguarded import or the failure names scroll_display_legacy instead of the core module, and only the leaf module may be hidden because a pre-3.2.0 core still ships src/common/. The hold itself is re-checked and unchanged: v3.2.0 is still latest, __version__ is still 3.2.0, no 3.3.0, 23 days rather than the few months the gate asks for. Also worth stating plainly -- the core updates by git pull, not by downloading a release, so release-asset counts would not measure uptake even if we had them. Whatever unblocks this has to come from the store side. 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> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai>	2026-09-02 11:34:18 -04:00
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>	2026-09-02 09:51:27 -04:00
ChuckandClaude Opus 5	154525beb8	fix(background): release the payload after every callback, not after each one (#509) ... Callers that join an in-flight fetch share one FetchResult. #499 released the payload inside the delivery loop, so the first callback got the data and every joiner got `result.data is None`. That is not a quiet degradation. Consumers read `result.data.get('events')`, so they raise AttributeError -- which the delivery loop catches and logs. The entire failure surfaced as one line: ERROR - src.background_data_service - Error in callback for request nhl_2026_...: 'NoneType' object has no attribute 'get' and a manager that silently never received its schedule. Seen on hardware: NHLRecentManager logs "Background fetch completed for 2026: 1000 events" and the very next line is the error, from NHLUpcomingManager's callback on the same request -- which had already logged "No events found in shared data." Deduplication is the normal case, not a corner. A sport's recent, upcoming and live managers all want the same season schedule, so the second and third are joiners on almost every cycle. _release_payload's own docstring said "once A callback has been handed the data", singular, which is the assumption that broke: the loop above it was written for many, and says so. Moved after the loop, and guarded on `callbacks` being non-empty. The guard matters: a request submitted without a callback must keep its payload, because polling get_result() is then the only way to collect it. The per-delivery release got that right by accident -- an empty list never entered the loop body -- and the existing test for it caught the omission. test_background_payload_release.py gains TestJoinersAllGetTheData: two submitters on one in-flight cache_key, asserting both are handed a populated payload, plus that the memory fix still happens once they have all had it. test_background_fetch_dedupe.py already proved the joiner's callback FIRES; it never checked what the callback received, which is the gap that let this through. Verified the new test bites: restoring the release inside the loop fails it with "'second' was handed a released payload". Full suite 3716 passed, 6 skipped. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-09-01 09:54:04 -04:00
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>	2026-08-31 17:35:00 -04:00
Chuck	cbc540a679	fix(assets): drop case-colliding duplicate league logos (#506) ... Four league fallback logos were each tracked at two paths differing only in case: assets/sports/mlb_logos/MLB.png + mlb.png assets/sports/nba_logos/NBA.png + nba.png assets/sports/nfl_logos/NFL.png + nfl.png assets/sports/nhl_logos/NHL.png + nhl.png On Windows and default macOS the filesystem is case-insensitive, so both index entries map to one physical file. Whichever git writes last wins and the other entry reports as permanently modified, so `git status` can never be clean and any `git pull` flips which one is dirty. For MLB/NBA/NFL the two entries pointed at the same blob, so the collision was only cosmetic. NHL was not: NHL.png is 1439x1621 (672455 B) and nhl.png is 768x768 (107184 B), so which resolution the league fallback logo loaded depended on checkout order rather than on the code. Keep the uppercase path in each pair. Every logo lookup uppercases the abbreviation before building a filename -- LogoDownloader .normalize_abbreviation and .get_logo_filename_variations (src/logo_downloader.py) and LogoHelper.normalize_abbreviation (src/common/logo_helper.py) all do -- and nothing in the tree requests a lowercase league logo, so the uppercase name is what the code actually asks for. For NHL that is also the higher-resolution asset. Removed with `git update-index --force-remove` so the literal index entry is dropped without the case-insensitive working tree deleting the survivor.	2026-08-30 09:08:42 -04:00
Chuck	4aeb0033e0	fix(install): don't abort when journald settings are absent (#507)	2026-08-29 18:32:50 -04:00
ChuckandClaude Opus 5	eae063700f	test(sports): pin the four-case sunset matrix before B6 runs (#505) ... B5 and B6 make different promises about an adopted scoreboard, and only the second is obvious. The table: | bundled copy present | bundled copy removed pinned old core | loads (the fallback) | ERROR, naming the module current core | loads, using core code | loads, using core code The top-left cell is the one worth having. Nothing in the suite proved that an adopted plugin still runs on a core predating src.common.sports_scroll, and that claim is the entire basis for having shipped B5 ahead of B6's gate. The bottom-left cell is asserted through PluginManager.load_plugin rather than a bare import, deliberately. The manager catches the ModuleNotFoundError, so a test written around pytest.raises would pass against a core where the module is merely broken rather than absent, and would say nothing about what the user meets: a plugin parked in ERROR and one log line. The assertion is the ERROR state plus an error naming the module, which is what makes the log actionable. Modelling notes, both of which were wrong first time and matter: - The copy-removed shape is an UNGUARDED import, not the guarded one with the legacy file deleted. Keeping the guard while removing its fallback only mislabels the failure -- the plugin reports a missing scroll_display_legacy and never mentions the core module that is actually absent. - Only the leaf module is hidden. A pre-3.2.0 core still ships src/common/; hiding the package would be a harsher core than any that shipped, and would make the failure name the package instead of the module. Ablation: disabling the old-core simulation fails three cells, and dropping the error object from load_plugin's set_state fails the fourth, so none of it passes vacuously. The install gate -- the other half of the guarantee -- stays in test_plugin_compatibility_gate.py rather than being duplicated here. Refs docs/SPORTS_UNIFICATION.md, "B6 -- why the sunset needs more than a version floor". Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-26 11:25:10 -04:00
ChuckandClaude Opus 5	af96bd5cb0	fix(memory): join an in-flight fetch instead of starting a duplicate (#503) ... * fix(memory): join an in-flight fetch instead of starting a duplicate submit_fetch_request() had no notion of "already fetching this". request_id embeds a millisecond timestamp, so every submit looked new, and active_requests is keyed by that id rather than by what is being fetched. Two submits for the same cache_key therefore started two identical fetches. It is not a rare race. _fetch_data in the sports managers branches: the Live manager fetches only today's games, but Recent and Upcoming both pull the full season schedule under the SAME cache_key. On a cache miss both miss, both submit, and nothing stops the second. On a running 512x64 board: 138 background fetches in 24 hours, arriving in pairs at identical millisecond timestamps, roughly hourly: 2 2026-08-25 11:47:26.612 2 2026-08-25 10:46:28.064 2 2026-08-25 08:01:55.962 Half of them redundant. Each duplicate costs a second download, a second JSON parse -- the expensive part on a Pi -- and a second parsed copy resident at the same time. Schedules on that board run 256KB to 20MB, 106MB across all sports. Because the pairs land in the same millisecond they also occupy two of the three executor slots with identical work, which is what makes two large parses peak simultaneously. A submit for a cache_key already in flight now joins that request: its callback is added to the existing one and the existing request_id is returned, so get_result() works for both. Different keys are untouched, and dedupe applies only while a fetch is in flight -- a submit after completion fetches again, because this is not a second cache layer. Three details: - The in-flight entry is dropped and the callback list snapshotted in the SAME critical section as filing the result. Otherwise a submitter could join a fetch whose callbacks had already run and never be called back. - Cancellation is the other way a request leaves active_requests, so it releases the key too. And the join path looks the request up rather than trusting the id, so an entry stranded any other way cannot wedge a key permanently -- it is dropped and a fresh fetch starts. - One callback raising no longer prevents the others being delivered. Previously there was only ever one. Interaction with #499, whichever merges second: that PR releases the payload after the callback runs. With several callbacks the release must happen after ALL of them, and must not happen at all if a joined submitter passed no callback, since polling get_result() would then be its only delivery path. The callback list built here is the hook for that. test_background_fetch_dedupe.py -- 8 tests, covering the join, callback delivery to both submitters, one callback raising, distinct keys not being coalesced, a post-completion submit fetching again, cancellation releasing the key, a stranded entry not wedging one, and the reported count. Verified non-vacuous by removing only the join branch: 3 fail. The callback tests assert the ids coalesced, without which they would pass trivially on two independent requests. Full suite: 3698 passed, 60 skipped, 1 failure that reproduces identically on unmodified main (test_install_lowmem, environment-dependent: /var/tmp is disk-backed on this machine). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(memory): discard a cancelled fetch instead of letting it commit Review follow-up on the dedupe. Cancelling releases the cache_key, so a replacement fetch for that key can start immediately. But _fetch_data_worker() had no cancellation check: the cancelled worker still wrote its response to the cache, flipped its own status from CANCELLED to COMPLETED, and ran its callbacks. The stale response could therefore land on top of the replacement's fresher data. The worker cannot abort an HTTP call in flight, so the response is discarded on return instead: no cache write, no callbacks, status left CANCELLED. The check sits immediately before the cache write, which is the first side effect. Also fixed, found by the new test rather than by reading: request_id was f"{sport}_{year}_{milliseconds}", which is not unique. Two submits inside the same millisecond produced the SAME id -- the test's two sequential fetches collided on a fast mocked response, and one request silently replaced the other in active_requests and completed_requests. Rare before this PR; load-bearing now, because dedupe hands that id back to every joiner as their handle for get_result(). A per-service counter is appended. Two test problems of my own, both fixed here rather than left to flake: - The cancellation test synchronised with time.sleep(0.4). A slow worker would have made it pass for the wrong reason. It now waits for the request to be filed in completed_requests. - The id-uniqueness test patched session.get, but submits are async: the 50 workers outlived the patch and made real DNS calls to the dummy host. It stubs the executor instead, which is what a submit-time test should exercise. 20 consecutive runs of the dedupe file: 0 failures. Full suite: 3700 passed, 60 skipped, 1 failure that reproduces identically on unmodified main (test_install_lowmem, environment-dependent). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(memory): make cancellation terminal, not advisory Three paths wrote request.status without checking whether the request had already been cancelled, so a cancel could be silently undone and the work it was meant to stop went ahead anyway. - A request cancelled while queued had CANCELLED overwritten with IN_PROGRESS the moment its worker started, defeating the discard check entirely: it downloaded, cached and called back for work the caller had withdrawn. It now skips the fetch outright, which is also the cheapest possible cancel. - The cancelled-check and the cache write were separate critical sections, so a cancel landing between them left the payload in the cache with the callbacks suppressed -- every submitter that joined the fetch waited for a call that never came. The worker now claims the commit in the same critical section that reads the status, and cancel_request refuses once claimed. The write stays outside the lock: it serialises a multi-megabyte payload to the SD card, and holding the service lock across that would stall every submit, status query and cancel behind it. - A cancelled request that then failed was relabelled FAILED, which slipped past the CANCELLED-only callback gate and delivered a spurious error callback. The except path now leaves CANCELLED alone. get_request_status() also reported a cancelled request as FAILED, since it inferred status from result.success; the final status is now recorded on the result. Both early returns assign to `result` so completed_requests files the outcome that was reported rather than the untouched placeholder. Tests cover cancellation before worker start, during the commit, and during an HTTP failure; all three fail against the unfixed code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * test(memory): reach the exception path as a cancelled request test_a_failure_after_cancelling_stays_cancelled cancelled the request while its worker was still queued, so once the pre-start branch landed the worker returned there and never reached the exception handler the test is named for. It passed against the unfixed code only because that branch did not exist yet; with it, the test passed for the wrong reason and reverting the except-path guard did not fail it. Cancel while the worker is parked inside the HTTP call instead, and assert the fetch actually started so the test cannot silently degrade into the pre-start case again. Reverting each of the three guards now fails exactly one test. Also read the payload inside the callback rather than off the FetchResult afterwards: #499 releases result.data once delivery is done, so the later read saw the released object and not what the caller was handed. 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>	2026-08-26 10:03:43 -04:00
Chuck	5e5979973e	Update README.md (#504) ... Signed-off-by: Chuck <33324927+ChuckBuilds@users.noreply.github.com>	2026-08-26 09:13:23 -04:00
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>	2026-08-26 08:56:45 -04:00
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>	2026-08-25 09:45:17 -04:00
ChuckandClaude Opus 5	f90638a9ec	feat(web): show available memory in Tools diagnostics (#500) ... * feat(web): show available memory in Tools diagnostics System Diagnostics reported memory as used-percent plus used/total GB. Neither distinguishes a healthy board from one about to fail, because page cache counts as used and is reclaimable on demand -- a Pi can read 70% used and be fine, or read the same and be minutes from trouble. MemAvailable is the kernel's own estimate of what a new allocation can actually obtain, and it is the number that tracked the failure on a 1GB Pi 3B+: healthy running sat above 500MB, and the crash came at 73MB. By that point fork() was failing, so sshd could not spawn a session and systemd could not respawn the display, while the kernel carried on answering pings at 0% loss. Used-percent gave no warning at any point on the way there; available memory fell steadily for hours. /api/v3/system/status now returns memory_available_mb from psutil.virtual_memory().available, and Tools renders it as its own tile, coloured against the thresholds that failure implies: red under 150MB, amber under 300MB, green above. The existing memory tile is left alone -- used/total is still what you want when sizing a workload; this answers the different question of how much room is left right now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): round available memory once, so the tile agrees with itself The colour was classified from the raw value while the label was rounded, and the API sends one decimal place. At the boundaries the two disagreed: 149.6 rendered as "150 MB" in red, and 299.6 as "300 MB" in amber -- each contradicting the threshold its own colour claims to apply ("red under 150MB"). A reader checking the tile against the documented thresholds would conclude the readout was broken. Rounding once and using that number for both restores agreement. It moves those two boundary cases up a band, which does not matter: the thresholds come from a measured failure at 73MB, so which side of the line a spare 0.4MB falls on carries no information. The tile agreeing with itself does. Null handling and the thresholds themselves are unchanged. 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>	2026-08-25 08:40:28 -04:00
ChuckandClaude Opus 5	333fd17d28	fix(memory): release fetched payloads once they have been delivered (#499) ... * fix(memory): release fetched payloads once they have been delivered BackgroundDataService kept the fetched body on the FetchResult it filed in completed_requests, which is swept hourly and capped at 500 entries by count. For a status record that costs nothing; for a season schedule it costs a tenth of the board. Measured on a 1GB Pi 3B+ with a 1-second RSS profile: the display process sat at 404MB after plugin load, then stepped +21MB when NFL fetched its season and +90MB when NCAA football fetched 946 games for 2026 -- and stayed at 494MB. Not a leak; a staircase that never came down. When a later fetch landed while headroom was low, available memory reached ~70MB, fork() began failing, and the board stopped being able to start a process at all: sshd accepted connections and closed them before its banner, systemd could not respawn the display, and the panel went dark while the kernel carried on answering pings. The cache-hit path was the worse of the two. It runs once per update interval per sport, mints a fresh request_id each time, and files whatever the cache returned. The memory tier is capped at 150 entries on a 1GB board, so a miss re-parses the payload from disk into a genuinely new object -- separate copies accumulating toward the 500-entry cap, not shared references. Releasing is safe: the payload is written to the cache under the request's cache_key before the result is built, the callback is handed the object directly, and consumers read it back from the cache afterwards (the plugins' callbacks use it only in passing, to log a count, before reading the cache). Nothing is lost -- it moves from RAM to the disk cache that was already holding it. Requests submitted without a callback keep their payload, since polling get_result() is then the only way to collect it. That keeps the existing contract, and the existing tests covering it, intact. Not addressed here: max_workers=3 allows three concurrent fetches, so three large parses can peak at once, and there is no in-flight dedupe by cache_key -- a second submit for a key already being fetched starts a second fetch. Both bound the transient peak rather than what stays resident, and both are behaviour changes worth their own review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(memory): file the cache-hit result before running its callback Restores the original ordering. Releasing the payload after the callback meant filing the result after it too, so a callback that queried get_result() or is_request_complete() for its own request would not have found it -- a behaviour change unrelated to the memory fix. The dict holds a reference to the same object, so releasing after filing still clears the payload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: wait for the payload release, not just the filing The callback test waited on is_request_complete(), which goes true as soon as the worker files the result in completed_requests. The worker then runs the cleanup pass, then the callback, then releases the payload. Both of the test's assertions therefore raced the worker: `seen` is populated by the callback, and `data is None` only after the release that follows it. It passes today because a one-line callback usually finishes inside the 20ms poll interval. Confirmed by making the callback sleep 0.4s: _wait() returns with seen == {} and the payload still resident. _wait_for_release() polls for the released payload instead. Release happens strictly after the callback returns, so a released payload also means the callback has finished and one wait covers both assertions. Verified against the same 0.4s callback. _wait() stays for the other three fetch-path tests, which assert only what is already true when the result is filed -- the success flag, the error, and the cache write that happened during the fetch itself. Its docstring now says so, so the next reader picks the right one. 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>	2026-08-25 08:32:08 -04:00
ChuckandClaude Opus 5	a4a55a23fc	fix(web): reject non-finite JSON numbers instead of raising (#497) ... * fix(web): reject non-finite JSON numbers instead of raising POST /api/v3/config/dim-schedule with {"dim_brightness": Infinity} answered 500. So did /api/v3/errors/clear with max_age_hours, and /api/v3/config/main with multiplexing or row_address_type. json.loads accepts Infinity/-Infinity/NaN by default -- they are not valid JSON, but Python's parser emits them -- and Flask's get_json passes them straight through. int(float('inf')) raises OverflowError, which is neither ValueError nor TypeError, so validation blocks that carefully caught those let it past and Flask turned it into a 500. The status code was not the real damage. dim-schedule answered with CONFIG_SAVE_FAILED and suggested "Check file permissions on config directory" and "Check available disk space" for what was an invalid number. Every one of these sites already had a correct 400 response written; they just never reached it. NaN already returned 400, because int(nan) raises ValueError. That is why this only ever showed up for the infinities, and why it survived: the obvious test case passes. OverflowError is now caught alongside ValueError/TypeError at the 27 sites in this file whose try block performs a numeric coercion. An AST sweep confirms no int()/float() of request-derived data is left outside a block that catches it. Verified end to end through Flask's test client rather than by reasoning about the parser: all four routes returned 500 before and 400 after. Tests: five Infinity cases (which fail against the previous except tuples), two NaN cases pinned so narrowing the tuple cannot quietly break them, and a check that ordinary input is not rejected by the widened guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * test(web): assert 400 exactly, and prove valid input is accepted Both review points were right, and the first is the failure mode this file exists to catch. Accepting any 4xx meant a 404 would have passed. Renaming one of these routes would have left the test green while it tested nothing -- the same "looks like coverage, points somewhere safe" shape that hid the composer injections. Now asserts exactly 400. Both infinity signs are exercised for every route. int() raises OverflowError either way, but only +Infinity was in the original report, and a guard that special-cased the sign would have passed a one-sided test. The valid-input test previously asserted "not a 400", which did not show what it claimed: the mocked save path fails for any input, so that assertion held whether or not validation had accepted the value. It now gives load_config a real dict and stubs _save_config_atomic, so the endpoint reaches its success response and the test can assert 200 -- which only happens if the value passed validation. 8 of the 11 checks fail with OverflowError removed from the except tuples. 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>	2026-08-23 11:44:23 -04:00
ChuckandClaude Opus 5	085fb93a87	test(logging): stop the location assertion matching the clock (#496) ... test_location_toggle asserted that ":42" -- a bare colon plus the record's hardcoded lineno -- is absent from a line formatted with include_location=False. But every formatted line starts with an HH:MM:SS.mmm timestamp, so ":42" also matches the clock whenever the minute or the second is 42. The test fails for roughly 3% of runs with nothing wrong: 2026-08-22 08:05:42.274 - INFO - test.logger - hello ^^^ matches ":42" Assert on the whole "module.funcName:lineno" token the format string actually emits ('%(module)s.%(funcName)s:%(lineno)d') instead of a fragment of it. That cannot collide with a timestamp, and it checks the thing the test is named for. Confirmed by formatting a record stamped 08:42:42 -- both minute and second colliding: the old assertion fails, the new one passes. Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-23 11:44:00 -04:00
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>	2026-08-23 11:43:39 -04:00
ChuckandClaude Opus 5	5a1f121e6b	Stop array-item secrets being wiped, and logging them (#493) ... Three review findings from #485 that I missed when addressing that PR; it has since merged, so they land here. 1. Array-item secrets destroyed by any unrelated save (data loss). remove_empty_secrets recursed into dicts but let a list fall through to the scalar branch and kept it verbatim. Lists merge by *replacement*, so the blanks the masked form posts back went straight over the stored array: stored [{"name":"a","token":"REAL-A"}, {"name":"b","token":"REAL-B"}] posted [{"name":"a","token":""}, {"name":"b","token":""}] merged [{"name":"a","token":""}, {"name":"b","token":""}] -> both credentials gone Same failure as the scalar api_key case fixed earlier, one container deeper. Lists now prune element-wise, and a list with nothing real in it is dropped so the stored one is left alone. Where one entry does change, the new merge_secrets merges by index instead of replacing. Two details the first attempt got wrong, both caught by existing tests: - An emptied dict item must stay {}, not None. ConfigManager's _strip_secrets_recursive treats a secrets list as *parallel* to the regular one ({} = "item i has no secrets"); a None makes it stop looking parallel, and it then drops the whole key from the main config -- silently deleting the items' non-secret fields too. - The incoming list's length wins. The regular config's list is authoritative about how many items exist, so preserving surplus stored entries would let the two fall out of step and make deleting an entry impossible. 2. Submitted credentials written to the journal (security). save_plugin_config logged `Full config: {plugin_config}` at INFO and `Config that failed: {plugin_config}` at ERROR. Both run before separate_secrets, so plugin_config still held the values just typed into the form. Now keys only. Swept the rest of web_interface/ and src/ for the same shape -- these were the only two. 3. Restart banner kept stale wording. showRestartPending() cleared the stored custom text but left the DOM element alone, so a config save could show the previous update's message. The default is read back from the server-rendered copy rather than duplicated in JS, so the template stays the one owner of the string. Verified: 556 passed, 1 skipped across the web suite. Mutation-checked -- reverting api_v3 fails the logging guard and the array-merge test; reverting either half of the secret_helpers change fails the unit tests. New end-to-end coverage drives the real endpoint, not just the helpers. Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-23 11:43:08 -04:00
ChuckandClaude Opus 5	6138a3cbef	test(install): stop assuming pytest's tmp_path is on disk (#492) ... test_returns_nothing_when_tmpdir_is_already_disk_backed asserted that lm_disk_backed_tmpdir prints nothing when TMPDIR is already disk-backed, and used pytest's tmp_path as the "disk-backed" directory: # tmp_path is on the regular filesystem, so the default must be kept. assert call("lm_disk_backed_tmpdir", env={"TMPDIR": str(tmp_path)}) == "" That premise is false on the platform the helper was written for. Debian 13 mounts /tmp as tmpfs -- which is the entire reason lm_disk_backed_tmpdir exists -- and pytest puts tmp_path under /tmp. So on the target platform TMPDIR is memory-backed, the helper correctly answers /var/tmp, and the test fails: E AssertionError: assert '/var/tmp' == '' The helper is right; the test was wrong. Reproduced on a box where /tmp is tmpfs and / is ext4. The test now looks for a directory whose backing store is actually disk -- tmp_path, else a scratch dir under /var/tmp, else beside the library -- using the same findmnt lookup the helper itself uses, and skips only if no disk-backed directory exists anywhere. An earlier version of this fix skipped whenever tmp_path was tmpfs, which made it skip on every machine with a tmpfs /tmp; that is barely better than asserting the wrong thing, so it now searches instead of giving up. Verified: 31 passed, 0 skipped. Mutation-checked -- deleting the "is the current TMPDIR memory-backed?" guard from lm_disk_backed_tmpdir fails this test, so it still catches the regression it is there for. Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-23 11:42:51 -04:00
ChuckandClaude Opus 5	568cb6d77f	perf(vegas): report the frame rate when it is worth reporting (#487) ... Vegas logged an FPS line at INFO every five seconds for the whole of every run. Measured over two hours on a rig: 1410 samples, 98.5% of them within 10% of target. The 1.5% that were not included a reading of 8.6fps against a target of 60 -- a real stall, completely invisible inside 1389 lines reading "59.6". INFO is now reserved for a shortfall, the recovery from one, and a slow heartbeat so a healthy marquee still shows a pulse. Scroll-progress tracing drops to DEBUG for the same reason: it runs for the whole of every scroll and is what you turn debug on to watch. Three review findings, all fixed here. 1. Per-frame timing used the wall clock (critical). The loop sleeps the remainder of each frame budget: frame_elapsed = <now> - frame_started time.sleep(max(0.0, frame_interval - frame_elapsed)) These devices have no RTC, so the clock jumps by however wrong boot time was when NTP first syncs. A backward step makes frame_elapsed negative, `frame_interval - frame_elapsed` then exceeds the whole budget, and the render loop stalls for the size of the correction. A forward step instead inflates the p99 and worst-frame figures this telemetry exists to report. Both per-frame timestamps are monotonic now. start_time stays wall-clock: it is only used for the iteration duration report, where a human-readable clock is the point. 2. FPS health state reset every iteration. last_fps_health_log and was_degraded were locals of run_iteration(), which is called once per cycle. Starting at 0.0 against a monotonic clock, `due` was true on the first sample of every iteration, so the 300s heartbeat degenerated into one report per cycle -- reintroducing the noise this change is about. A recovery that crossed an iteration boundary was never reported either, since was_degraded had already gone back to False. Both now live on the coordinator and reset in start(). 3. The degraded threshold read as an off-by-one. 90% of target is deliberate -- a marquee jitters constantly, so "anything below target" would report forever and mean nothing -- but nothing said so, leaving 55fps-against-60 looking like a missed case. The constant now states the band and gives that exact example. Also drops two soccer logo PNGs that a `git add -A` had swept into the first commit. They are unreferenced, unrelated to frame-rate telemetry, and 210KB. Verified: each fix mutation-checked -- restoring the wall clock on either per-frame timestamp, or making the health state local again, fails the new tests. 566 passed across the vegas, coordinator and scroll suites. Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-23 11:42:33 -04:00
ChuckandClaude Opus 5	6b74506695	fix(sports): fetch odds for the games shown, not the whole schedule window (#494) ... * fix(sports): fetch odds for the games shown, not the whole window SportsUpcoming.update() walked every upcoming game in the schedule window and called _fetch_odds() on each one inside that collection loop, narrowing to upcoming_games_to_show only afterwards. Each call is a separate sequential ESPN request. The comment sitting above it said odds were fetched "only for games that will be displayed". The only narrowing it actually applied was show_favorite_teams_only, which is not the default, so in the usual configuration nothing narrowed it at all. Measured on devpi, where the football plugin has the same shape: 467 odds requests in one 35s burst, 467 distinct events 315 NFL + 152 college-football -- roughly a whole season plugin football-scoreboard operation timed out after 30.0s The burst repeats each time the 1h odds TTL expires: 67 -> 327 -> 957 -> 1261 requests/hour across four consecutive hours. Between expiries the cache works and the rate is zero, so this is a thundering herd on expiry, not a caching failure. The fetch now runs after selection, over team_games -- the list already cut to upcoming_games_to_show. This mirrors the fix the football plugin already carries; the shared base class never got it. SportsLive is deliberately left as it is: it walks the raw event list because it has to find which games are live, but only fetches odds for a game that has already passed the is_live/is_halftime test, so its fan-out is bounded by how many games are actually in progress. The test pins that distinction rather than assuming it. The test reads the AST rather than the source text, and asserts the full set of call sites, so a new one has to be classified deliberately instead of inheriting whichever behaviour it happens to land in. Writing it that way is what turned up the SportsLive site, which I had missed. Verified: reverting the fix fails the test with the offending iterable named ("iterates over 'events'"). 525 passed, 9 skipped across the sports and odds suites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * test(sports): check the odds guard structurally, not by its text Review caught that _guards_above() collected an `if` test even when the call sat in that if's `else`, so moving _fetch_odds() into the else of the is_live/is_halftime test would still pass -- while fetching odds for exactly the non-live games the guard exists to exclude. Verifying that turned up a wider hole in the same assertion. It matched substrings of the *unparsed source*, so a negated condition satisfied it too: if not (details["is_live"] or details["is_halftime"]): self._fetch_odds(details) # every non-live game Both names still appear in that text, so `"is_live" in guards` held and the test passed on code doing the opposite of what it claims to check. The guard test is now structural. It walks the AST for an enclosing `if` whose *body* (never its `else`) contains the call, and whose test references both names without either sitting under a `not`. Verified by mutation: fetching odds for non-live games now fails with "does not sit in the true branch of a test requiring the game to be in progress". Moving the call into the else of the *favourites* test still passes, which is correct -- the game there is still live, so the in-progress contract holds and the fan-out stays bounded by how many games are actually in play. 525 passed, 9 skipped across the sports and odds suites. 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>	2026-08-21 16:24:15 -04:00
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>	2026-08-21 16:22:48 -04:00
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>	2026-08-21 16:19:33 -04:00
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 14abea2d24) (cherry picked from commit 0f77bd2345) * perf(vegas): trace the content path at DEBUG instead of INFO plugin_adapter narrates every step of acquiring content from every plugin -- "Has get_vegas_content", "Native: calling get_vegas_content()", "Native content returned None", "Has scroll_helper", per-item sizes -- once per plugin per cycle, all at INFO. Measured on a live rig: 13,408 log lines an hour, of which 13,366 were INFO and 35 were WARNING. Roughly 223 lines a minute of string formatting on a Pi that is also driving the panel, written through journald to the SD card, with the 35 lines that actually indicate a problem buried among them. Top repeated messages in that hour: 717 Scroll progress: elapsed=... total_scrolled=.../... px 399 [plugin] --> INCLUDED in Vegas scroll 323 [plugin] content_type=static, display_mode=fixed 195 [plugin] Has get_vegas_content: True 195 [plugin] Native: calling get_vegas_content() 168 [plugin] Native: get_vegas_content() returned None 168 [plugin] Native content returned None <- the same fact, twice 54 logger.info calls in plugin_adapter become logger.debug, along with the per-frame scroll-progress line in scroll_helper. Together those are 3,174 of the 13,408 lines an hour, a 23% cut, and the ~3,600 odds-manager lines are addressed separately by ledmatrix-plugins#300. Nothing is lost: the 19 warning/error/exception calls in the module are untouched, so real failures still surface at their own level. This is a logging-level change only -- no control flow, no behaviour. One INFO call is deliberate and stays. The padding-strip message picks its level at runtime (`logger.warning if (left and right) else logger.info`) and test_vegas_plugin_adapter.py pins that choice; it survives because it is not a direct logger.info call site. That test still passes. Mutation-checked both ways: reintroducing a single INFO trace fails the guard, and demoting the warning/error calls along with the trace fails a second guard written for exactly that mistake. 537 vegas and scroll tests pass. (cherry picked from commit e496d95dfe) (cherry picked from commit 8d1e43c15a) * fix(logging): give the journal the real severity of each line Everything this process writes to stdout reaches the journal as PRIORITY=6, whatever the Python level was, because journald has nothing else to go on. Measured on a live rig over 24 hours: lines containing " - ERROR - " 55 lines containing " - WARNING - " 13 journald PRIORITY recorded 6, for every one of them So `journalctl -p err -u ledmatrix` returns nothing while errors are being logged, and `-p warning` likewise. Triage falls back to grepping message text, which is slower and unreliable: during this audit a search for "oom" matched the radar logging "zoom=9" twenty-four times and briefly looked like the OOM killer had been firing. systemd reads a leading "<N>" on each stdout line and takes it as the priority (sd-daemon(3)), so a formatter that prefixes one costs no dependency. Every line of a multi-line record is tagged, not just the first -- the journal splits them, and an untagged continuation reverts to the default, which would leave the body of a traceback filed as informational while its first line was an error. Applied only when JOURNAL_STREAM is set, which systemd sets for services whose output it captures. Run from a terminal, in the emulator or under pytest the prefixes would be literal noise, and the file handler keeps the plain formatter for the same reason. Mutation-checked three ways: prefixing unconditionally fails the outside-systemd test, prefixing only the first line fails the multi-line test, and mapping ERROR to 6 fails the level mapping. 39 tests pass across the logging suites. (cherry picked from commit 780fca6365) * fix(logging): let callers see through the journald formatter wrapper CI caught what local testing could not: two existing tests in test_logging_config.py assert that setup_logging() selected a StructuredFormatter or a ContextualFormatter, by checking the console handler's formatter directly. Wrapping that formatter to tag each line with its syslog priority makes those assertions false. They passed locally and failed on the runner because the wrapper is applied only when JOURNAL_STREAM is set -- absent in a terminal, present in CI. An environment-dependent break, which is the kind that gets shipped. The wrapper now exposes the formatter it delegates to, and those two tests look through it. They are about which formatter format_type selects, and that behaviour is unchanged; only the object they have to reach for moved. Verified both ways this time: 39 tests pass with JOURNAL_STREAM set and with it unset. * perf(plugins): stop rewriting a plugin's metrics file on every call Plugin metrics were persisted to the cache inside monitor_call, so every call by every plugin rewrote a small JSON file. Measured on a running rig: one plugin's plugin_metrics file changed nine times a minute, with fourteen such files active. Each is around 350 bytes, which on ext4 costs a 4KB block plus a journal entry, so the cost is dominated by the write itself rather than the payload. Cache writes accounted for essentially all of that device's 2.4 MB/min of SD traffic, on a card that wears out and has already failed twice on the other rig. Metrics cannot be de-duplicated the way health state can, because call_count changes on every call and the timings usually do too. So they are rate-limited instead: at most one write per plugin per 30 seconds. The in-memory copy stays authoritative and exact -- a plugin's call_count is still precise the instant after it runs. Only the cross-process snapshot the web UI reads is delayed, and telemetry up to half a minute old is still a fair description of a long-running plugin. reset_metrics clears the throttle timestamp, so a reset is not left showing a deleted key for the rest of the interval. Extrapolating the sampled rate, this takes metric writes from roughly 126 a minute to 28. Health persistence, the other half of the churn, is handled separately in #475. Verified by reverting the throttle: the churn test then reports 50 writes for 50 calls. 88 tests pass across resource monitor, plugin system and web API. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix: use a monotonic clock and only mark metrics persisted once written Two review findings on the throttle, both right. The interval compared wall-clock timestamps. These devices have no RTC, so the clock jumps by however far off boot-time was the moment NTP first syncs -- a forward jump would allow an early write, a backward one would stall the snapshot well past the interval. time.monotonic() is not subject to either. The timestamp was also recorded before cache_manager.set(). A set() that raised would buy the next interval's silence without leaving a snapshot behind, which is the one case where skipping the write is least affordable. Recorded after the write lands instead, so a failure is retried on the next call. Verified by restoring the original ordering: the new test then reports one write where two are expected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * Address all six review findings on the perf consolidation CodeRabbit reported six; this is all six, checked against its own "Actionable comments posted: 6" rather than against what I happened to scroll past. Three are real defects in the code: 1. _under_systemd() trusted the presence of JOURNAL_STREAM. systemd publishes JOURNAL_STREAM as "dev:ino", and every child process inherits it -- including one whose stdout has been redirected to a pipe or a file. The variable outlives the descriptor it describes, so a subprocess would decide it was talking to the journal and emit the "<N>" priority prefixes as literal noise into that captured output. That is exactly the noise the function exists to prevent. It now parses the pair and fstats stdout, per systemd's own guidance, and returns False for missing, malformed, mismatched, or unusable descriptors. 2. Cached metrics were not type-checked. A dataclass does not enforce its annotations, so ResourceMetrics(call_count="not a number") builds happily and only fails later, deep inside monitor_call: TypeError: can only concatenate str (not "int") to str Values are now coerced to their declared type at load, where there is still a cache key to name in the warning, and a value that cannot be coerced starts the plugin fresh instead of arming a delayed failure. A numeric string is accepted rather than discarded -- a JSON round-trip can widen an int, and that is recoverable. 3. The first metrics snapshot was skipped for the first 30s of uptime. _persist_metrics used 0.0 as the "never written" default. monotonic() is time since boot on Linux and systemd starts this service at boot, so `now - 0.0 < 30` was true for the first half-minute of every run: the throttle swallowed the very first write, the one that matters most after a restart. The sentinel is now None and the interval is only applied when a previous write exists. Three are tests that could pass without testing anything: 4. test_health_write_churn's fake cache stored by reference, so the tracker kept mutating the object already in the store -- a record could look persisted when no write had happened, which is precisely what test_durable_state_survives_a_restart exists to detect. Both directions now deep-copy, like a cache that serialises to a file. Verified: disabling the one real cache write now fails three tests. 5. test_values_of_the_wrong_type_do_not_raise asserted only that a dataclass had been constructed, which was true with the bad value still in it. It now asserts the loaded metrics are usable -- the field is numeric, and arithmetic on it does not raise -- across four kinds of bad value. 6. test_vegas_log_volume counted "logger.error(" in the source text, which also matches comments, docstrings and string literals -- including that module's own docstring, which names those levels. A real error call could be demoted with the tally unmoved. It now walks the AST, reusing the helper already in the file. Verified: demoting all 18 warning/error/exception calls now fails the test. Verified: every fix mutation-checked by reverting it and confirming the matching test fails. 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>	2026-08-21 15:50:55 -04:00
ChuckandClaude Opus 5	9c0c0dc851	consolidate(web): credential exposure, secret loss, and the update path (#485) ... * fix(web): stop /config/main handing out every credential it holds The endpoint returned the raw config to anyone who could reach the port, and this web interface has no authentication of any kind. An unauthenticated request against a live rig returned: github.api_token 40 chars incoming-packages.ha_token 183 chars jellyfin-now-playing.api_key 32 chars ledmatrix-weather.api_key 32 chars on-air.mqtt_password 8 chars youtube.api_key 20 chars youtube-stats.api_key 39 chars A GitHub token and a Home Assistant long-lived token among them. Anything on that LAN could read them. The x-secret masking the plugin config endpoints use does not reach here: this route never consults a schema, and core keys such as github.api_token have no schema to carry the marker. Several of the fields above *are* tagged x-secret in their plugin's schema and were still returned in full, which is what rules out the schema route as the fix for this endpoint. Credential-named fields are now blanked. Matching on the name is blunt, and for a whole-config dump that is the right default: anything named like a credential should not leave the process, and a new plugin adding a differently-shaped secret is covered without anyone remembering to tag it. Blanked rather than removed, and safe to blank: POST /config/main merges into the freshly loaded config and writes only the keys it was given, so a client that round-trips this response cannot erase a secret it never saw. The web API suites confirm it -- 81 passing, unchanged. On the test that matters: the first version of this suite exercised the two helpers and nothing else, and reverting the single line that wires the redactor into the route passed all thirty of them. A property asserted on a helper is not a property asserted on the endpoint, and it is the endpoint that is exposed to the network. The added test goes through the view function, and it does fail on that revert. This also corrects an earlier claim of mine. I reported that GET /api/v3/config did not expose these values; that path 404s, so the check proved nothing. The real route is /config/main and it exposed all of them. * fix(web): stop an unrelated config edit from erasing a plugin's secret Saving any field on a plugin's config form destroyed that plugin's stored credential. On a rig with a weather API key, changing the city silently emptied the key, and the plugin stopped working at the next fetch with no indication why. The path had no guard at any step. The config partial masks secrets before rendering (pages_v3.py:740), so the browser posts them back blank; _parse_value deliberately preserves "" for optional string fields; separate_secrets routes that "" into secrets_config, which is a truthy dict; deep_merge writes it over the stored value; save_raw_file_content persists it. The blank does not even need the round-trip. merge_with_defaults injects the schema's api_key default ("") into every save, so a client that never sends the field at all still erases it. test_secret_count_message_counts_top_level_keys was counting exactly that injected blank as a saved secret field -- the visible edge of the bug, pinned as expected behaviour. remove_empty_secrets() already existed for this, with seven unit tests and a docstring describing this precise scenario ("clients will send those empty strings back ... so that existing stored secrets are not overwritten with blanks"). It was never wired into a call site. This wires it into both save paths that merge into the secrets file. A blank now means "unchanged" rather than "delete", which is the same contract the helper's tests already describe. The cost is that a secret can no longer be cleared by emptying the field; clearing needs its own affordance, since a control that erases credentials as a side effect of ordinary edits is not one. Verified by reverting the guard: the new round-trip test then fails with the stored key read back as ''. 262 web tests pass with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(web): stop dumping the config and request headers to the journal save_main_config logged its entire POST body and the full request headers at ERROR on every save. The body is the configuration itself, and the headers carry the session cookie, so a routine settings change wrote both to the journal -- at a level that guarantees they survive any sane log filter. The lines are leftover debug output: they say "DEBUG:" in the message while calling logging.error, and they went through the root logger rather than the module logger, bypassing the level configured for this blueprint. Replaced with a debug-level line recording the shape of the request, which is the part with diagnostic value. The local `import logging` went with them; it shadowed a module-level import that was already there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(web): stop /config/secrets handing out every credential it holds GET /api/v3/config/secrets returned config_secrets.json in full to anyone who could reach the port, and this interface has no authentication. Probed against a real rig it produced six populated credential fields: a 40-character GitHub token, a 183-character Home Assistant token, and Jellyfin and weather API keys. This is the second door onto the same credentials; #477 closes the first. Masking the response alone would have been worse than the leak. The only client fetches every secret, edits one field and posts all of them back, and save_raw_file_content replaces the file wholesale -- so a masked GET followed by the client's own save would write the mask over every credential the user had not touched. That is why this was left open when the leak was found; it needs both halves. Read side: mask_all_secret_values(), which already existed for exactly this endpoint -- its docstring names it -- and had never been wired to a call site. It leaves empty values and YOUR_* placeholders alone, so a client can still tell "set" from "not set" without being told the secret. Write side: strip the echoed mask and blanks from the submission, then merge onto what is stored, so "unchanged" means unchanged. The cost is that a secret can no longer be cleared by blanking it; that wants its own affordance, since a control that erases credentials as a side effect of saving an unrelated one is not one. Browser side: the token field is now left empty rather than filled from the response. Filling it with the mask would have stored eight bullet characters as the token the next time the user pressed Save, and filling it with the real value is the thing being fixed. It reports whether a token is saved instead. Verified end to end through the Flask endpoints, not the helpers. Reverting the masking fails the leak tests; reverting the merge fails the preservation tests; both halves are independently guarded. 278 web tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(web): stop reporting "no update" when the update check could not run check-update returned update_available=False whenever git failed. The banner is the only route to the update button, so a checkout git refuses to touch looked exactly like a current one -- permanently, with nothing on screen to act on and only a log line recording why. The common cause is an install performed as root. scripts/install/one-shot-install.sh clones into ${HOME}/LEDMatrix, never consults SUDO_USER, and contains no chown at all, while its own error text suggests running the whole thing under sudo. The result is a root-owned checkout, and on a rig this is what every git command in it does: fatal: detected dubious ownership in repository at '...' including the fetch this endpoint runs. Verified on real hardware rather than assumed. A failed check now reports check_failed with a message the user can act on -- for dubious ownership, the chown that fixes it. The banner shows that message instead of hiding itself, with the update button suppressed since updating cannot work until the cause is fixed. The success path is untouched. This does not fix the installer, which is the real cause; it stops the symptom being invisible. The installer needs SUDO_USER handling and a chown, and its suggestion to run as root should go. Reverting the endpoint change fails four of the five new tests; the fifth guards the success path and correctly does not move. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(web): stop the installer chmod stripping exec bits on every update git tracks five scripts as mode 644 that first_time_install.sh then chmods to 755 (start_display.sh, stop_display.sh, the two install_*_service.sh, and one-shot-install.sh does the same to first_time_install.sh). With core.fileMode true, the default on Linux, git reports all five as modified from then on, in files the user never touched. The update button stashes local changes before pulling, so it is not blocked by this. But it never pops that stash -- stash pop and stash apply appear nowhere in the update flow -- so the mode change is stashed away and left there, and the files revert: === file modes after the update button's stash === 664 first_time_install.sh <- installer had made these 755 664 start_display.sh 664 stop_display.sh 664 scripts/install/install_service.sh So every web-UI update silently strips the executable bit from the installer's own scripts, and leaves a stash entry holding the difference. start_display.sh and stop_display.sh stop working from the shell afterwards. A manual `git pull --rebase` over SSH fails outright, since nothing stashes for it: "cannot pull with rebase: You have unstaged changes". That is the likely source of the reports, since plenty of people update that way. Tracking the five as 755 -- what they should always have been, as the installer chmodding them attests -- removes the spurious mode change entirely: nothing to stash, nothing stripped, no stash entry, and manual pulls work. The pull also passes --autostash, for the case the code explicitly tolerates: when the stash fails it logs a warning and pulls anyway, and that pull is what then fails. Autostash also pops what it stashes, which the manual stash does not. Note that `git add -A` after `git update-index --chmod=+x` silently reverts the index to the on-disk mode, so the modes here were set by chmodding the files themselves. Regression test asserts the five stay tracked executable; reverting any one of them fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(web): ask for the restart that makes an update take effect The update button pulls new code and restarts nothing. There is no systemctl, restart, reload or reboot anywhere in the 172-line git_pull handler -- it stashes, pulls, installs changed requirements, re-removes plugins the user had uninstalled, and returns "Code updated successfully." Meanwhile both services go on running the code they loaded at boot. So the display keeps rendering the old build, the web interface keeps serving the old build, and the user is told the update worked. Nothing on screen suggests otherwise, and the next reboot is what actually applies it -- whenever that is. The affordance for this already exists: the restart-pending banner, raised after main-config saves, with a Restart Now button wired to the display service. A code update is a stronger reason to show it than a config save is. The response now reports restart_required, and applyUpdate raises the banner with wording for a code update rather than a config save. The banner's message became a parameter and is persisted next to the flag, since it outlives the page that raised it. restart_required is only true when the pull actually moved HEAD. "Already up to date" is a success too, and prompting after a no-op would train users to dismiss the prompt unread. This covers the display service, which is what the Restart Now button drives and what users notice. The web interface still picks up its own new code on its next restart; restarting it from inside a request it is serving is a larger change than this one. Reverting the flag fails the test that a pull which moved HEAD asks for a restart. 290 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * Mask list-shaped secrets element-wise, close two vacuous tests mask_all_secret_values treated any non-empty list as a scalar, so a secrets file holding "accounts": [{"name": "a", "token": "tok-a"}, {...}] came back as a single "••••••••". The caller could not see how many entries existed, and the raw editor was handed a string where the file holds an array. Recurse into lists in both _mask_value and _contains_mask. Lists merge by replacement, not key-wise, so strip_masked_values now drops a list outright if any element still carries the mask -- storing a half-masked list would discard the untouched entries. Two tests could pass without exercising what they claim to check: - test_git_pull_resolution asserted modes only for paths git ls-files returned. A renamed or deleted installer target is simply absent from that output, so its mode was never checked. Assert every CHMODDED path is tracked first. - test_config_secrets_masking never checked the POST status. A 500 leaves the old file in place, which satisfies every assertion that follows. Assert 200 before reading the file back. 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>	2026-08-21 14:30:29 -04:00
ChuckandClaude Opus 5	71739d85d1	harden: cap malloc arenas, warn on unit drift, and grant the portal's sudo commands (#476) ... * perf(systemd): cap glibc malloc arenas on the display service Measured on a live rig 2.5 hours after start: RSS 1030 MB Private_Dirty 988 MB anonymous mappings > 10 MB 23 largest 104, 79, 66, 63, 63 MB, on 64 MB-aligned addresses threads 9 cores 3 -> glibc ceiling = 8 x 3 = 24 arenas 23 against a ceiling of 24, all 64 MB-aligned: these are glibc's per-thread malloc arenas, not live objects. The data the process was actually holding accounts for perhaps 15 MB -- the widest scroll strip observed was 35,746 x 64, about 7 MB as RGB and the same again for its numpy mirror. It is bloat rather than a leak: sampled four times over 135 seconds, RSS sat between 990 and 1030 MB rather than climbing. glibc gives each allocating thread its own arena, grows them to hold peak demand, and never gives them back. A process that builds and drops large images across several threads is exactly the shape that produces this. The device had 59 MB free at the time, on 1845 MB total. MALLOC_ARENA_MAX=2 trades a little allocator concurrency for that resident memory. It is a tuning knob rather than a fix for a defect, so the rationale and the measurements sit next to it in the unit file, and a test asserts they stay there -- a bare environment variable invites removal by whoever meets it next. Two things this is NOT, both checked rather than assumed: - Not an OOM problem today. A grep for "oom" in the service journal returned 24 matches, all of which were the radar logging zoom=9 and zoom=7. The kernel OOM killer has not fired: dmesg has zero matches. - Not currently capped by the unit's MemoryMax=85% either. That directive is in this file but absent from the unit actually installed on the rig, which reports MemoryMax=infinity, so nothing is enforcing a ceiling there. The saving is unmeasured on hardware: applying it needs a service restart, which blanks the panel, so that is the user's call rather than something to do mid-audit. If p99 frame time regresses -- it sits at 18.4 ms against a 16.7 ms budget for 60 FPS, so there is not much headroom -- raise the value rather than remove it. (cherry picked from commit 446207ffbc) * test(systemd): pin the arena value instead of accepting a range Review follow-up. The range check accepted 1, 3 and 4, so a change to 4 -- which hands most of the resident saving back -- passed a test whose whole purpose is to notice that. Pinned to the value the unit ships, in one named constant. Raising it is still a legitimate response to a frame-time regression, but it should be a visible edit here rather than silent drift, and the failure message says so. Mutation-checked: changing the unit to 4 now fails. (cherry picked from commit 73fff8d2d5) * fix(startup): warn when an installed systemd unit has drifted from the repo's Nothing re-applies systemd units after the first install. `git pull` -- which is what the web UI's update button runs -- brings a new template into the checkout, but no code in web_interface/ or src/ copies it to /etc/systemd/system, and nothing anywhere runs `systemctl daemon-reload`. The unit that actually runs is whatever first_time_install.sh wrote on day one. So every hardening added to a unit is inert on existing installs, silently. Measured on a live rig: installed /etc/systemd/system/ledmatrix.service 2026-08-06 template systemd/ledmatrix.service 2026-08-19 contents differ with the practical result that the MemoryMax=85% the repo's template specifies was not being enforced at all -- `systemctl show` reported MemoryMax=infinity. Anyone reading the template would reasonably believe the service was capped. Startup now compares each installed unit against its substituted template and warns when they differ, naming install_service.sh as the remedy. A warning, not an error, and deliberately not a silent rewrite: editing files under /etc and restarting services is the installer's job, not something a display process should do to a machine while it is booting. Making it fatal would also brick every development checkout whose unit is legitimately absent or hand-edited. Comparison ignores comments, blank lines and ordering. The template carries explanatory comments the installed copy will not have, and systemd does not care about order within a section, so a literal comparison would warn on every boot and be ignored within a week. Mutation-checked three ways: never reporting drift fails, making it fatal fails, and -- after the first attempt missed it -- comparing raw text now fails too. That last gap is worth noting: the comment-insensitivity tests originally exercised the helper directly, so a comparison that stopped calling the helper passed them all. The test that catches it goes through _validate_systemd_units. 29 startup-validator tests pass. (cherry picked from commit cf521bdfd8) * fix(install): grant the sudo commands the captive portal actually runs The installers write two allow-lists, /etc/sudoers.d/ledmatrix_web and ledmatrix_wifi. Anything the code runs under sudo that is not in one of them needs a password, which a service cannot supply, so the call fails. Five commands were being run and none of them granted: sysctl -w net.ipv4.ip_forward=0|1 wifi_manager.py:788, 883 nft add|delete table ip ledmatrix wifi_manager.py:835, 895 rfkill unblock wifi wifi_manager.py:1811 iptables ... wifi_manager.py:796, 813, 818, 871 mkdir -p .../dnsmasq-shared.d wifi_manager.py:922 Together these are the captive portal: unblock the radio, bring up the AP, add the redirect, turn on forwarding, and undo all of it afterwards. Without the grants a hardened install would associate clients to the access point and then fail to route them. Why it has gone unnoticed: a stock Raspberry Pi image ships /etc/sudoers.d/010_pi-nopasswd granting the default user <user> ALL=(ALL) NOPASSWD: ALL which satisfies every one of these regardless of what the allow-lists say. Confirmed on a live rig -- `sudo -n -l` permits sysctl there, and the blanket rule is why. The allow-lists are effectively decorative on a default image and only start mattering once that rule is removed or the service runs as another user. test_sudo_allowlist_covers_calls.py extracts every argv-style sudo call in src/ and web_interface/ and asserts an installer grants it, so the next command added without a rule fails here rather than on someone's hardened box. Getting that test honest took three passes, each worth recording: - Matching the literal "systemctl" against rules written as `$SYSTEMCTL_PATH enable ...` reported six gaps that did not exist. Binary path variables are now normalised before comparing. - Scanning the whole installer let `NFT_PATH=$(command -v nft)` -- a variable definition, not a grant -- satisfy the check on its own, so deleting the actual nft rules still passed. Only NOPASSWD lines are considered now. - `sudo -n <tool>` reported "-n" as the binary. sudo's own flags are skipped. Each of the five grants is individually mutation-checked: removing any one fails the suite. (cherry picked from commit a372b43cd1) * fix(install): drop the iptables wildcard, and pin each grant properly Review follow-up. Two findings, both right, and the first is a hole I opened myself. `NOPASSWD: iptables *` is a root shell for the web user by another name. `iptables --modprobe=/path/to/anything` runs that path as root, so a wildcard grant on iptables escalates rather than restricts. I added that rule while fixing a permissions gap, which is a worse outcome than the gap. It is gone, and a test now fails on any trailing-wildcard grant to a tool that can execute another program -- iptables, nft, tcpdump, find, awk, sed, perl, python, env. The other finding: checking only the binary made the coverage test far weaker than it looked. With `sysctl` present anywhere in the allow-list, deleting the `net.ipv4.ip_forward=0` grant still passed -- and the portal would then be unable to restore forwarding on teardown. Each required command is now matched in full, and each is mutation-checked individually, including that exact single-line case. Scope pulled in deliberately. The first version of this test tried to assert that *every* sudo call in the codebase is granted. Run honestly, it showed the portal also runs iptables, nft, `ip addr`, `ip link` and `cp` with arguments built at runtime -- an interface name, a port. Those cannot be granted safely in a sudoers file: the rule needs a trailing wildcard, and that is the escalation above. Closing that half needs a privileged helper that builds the rules itself and takes only an interface and a port, granted the way safe_plugin_rm.sh already is. That is a design decision, not a one-line grant, so the test now pins the four commands this change actually grants and the docstring says plainly what it does not cover. Better a narrow test that is true than a broad one that is not. (cherry picked from commit 500cfbc9f4) * fix(install): pin PATH, keep unit order, and tighten the sudoers assertions Three review findings, all correct. The installer resolved binaries through an inherited PATH and wrote whatever it found into sudoers as NOPASSWD grants. first_time_install.sh re-execs itself with `sudo -E`, which preserves the caller's environment, so a writable directory early in PATH turned a compromise of the low-privilege web user into permanent root -- via a file the installer itself wrote. PATH is now pinned to the system directories before anything is resolved, and every resolved binary must be root-owned and unwritable by anyone else before it reaches the sudoers file. _unit_body() sorted a unit's lines before comparing. Order is not noise in a systemd unit: repeated ExecStartPre=/ExecStartPost= run in the order they appear, and a directive that moves between [Unit], [Service] and [Install] means something different where it lands. The drift check reported no drift for units that had genuinely changed. Order is preserved now. Two of that check's own tests asserted the wrong thing -- test_reordered_directives_are_not_drift said so in its name -- and are inverted, with a second covering a directive moved between sections. The cosmetic-difference test now varies comments, blank lines and indentation, which is what the installer actually drops, rather than reversing the file. The sudoers assertions matched command prefixes, so `sysctl -w net.ipv4.ip_forward=0 *` satisfied the requirement while granting the caller arbitrary trailing arguments as root. They are exact now. The wildcard check also normalises ${NFT_PATH} the same way as $NFT_PATH; the brace is not a word boundary, so that spelling was skipped entirely. Verified by reintroducing each: a widened required grant fails the exact match, `${NFT_PATH} *` fails the wildcard check, and require_trusted_binary refuses a non-root-owned, world-writable, or missing binary. 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>	2026-08-21 14:29:45 -04:00
ChuckandClaude Opus 5	cc258aaffd	fix(install): tag the secondary installer's journalctl grants too (#491) ... Review was right on all three counts, and the first is the one that matters: scripts/install/configure_web_sudo.sh writes the same three wildcard journalctl rules as first_time_install.sh and none of them carried NOEXEC. So this PR closed the pager escape on one installer path and left it open on the other, which is close to no fix at all -- a rig configured through that script still hands out a root shell via less's "!command". The test could not have caught it, for two independent reasons. INSTALLERS did not list the file. And even listed, _grant_lines() kept the raw source line: that installer echoes its rules, so each one ends in a quote rather than the wildcard, and the trailing-* check skipped every one of them. Either alone would have hidden it. Both fixed: the file is covered, and an echoed rule is unwrapped to the sudoers line it actually emits. The selector test now covers -t ledmatrix as well. It asserted only the two -u forms, so deleting the -t rule would have passed. Verified by removing NOEXEC again from the secondary installer: four of the six tests fail, where before the suite passed with the vulnerability present. Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-21 14:28:23 -04:00
Chuck	fe5a3aa99d	harden(install): tag the journalctl sudo grants NOEXEC (#472) ... journalctl starts a pager when its output is a terminal, and from less a "!sh" is a shell with whatever privileges journalctl was given. That is the standard journalctl escalation, and these rules end in a wildcard: <user> ALL=(ALL) NOPASSWD: /usr/bin/journalctl -u ledmatrix * Nothing this project runs needs the pager -- both call sites pass --no-pager, in web_interface/app.py and api_v3.py. But a sudoers rule cannot require a flag that sits in the middle of a command line, and reasoning about what a trailing wildcard does and does not admit is exactly the kind of subtlety that produces a hole. sudo's NOEXEC tag stops the command executing another program at all, which closes it without depending on that reasoning. NOEXEC works by LD_PRELOAD, so it applies to dynamically linked binaries. Checked on the target hardware: journalctl there is dynamically linked. The generated rules were run through `visudo -c` -- parsed OK. Found while auditing the pre-existing wildcard grants, prompted by review catching a far worse one I had added myself in the same area: `iptables *`, where --modprobe runs an arbitrary path as root. Reachability, stated plainly: on a stock Raspberry Pi image none of this matters, because 010_pi-nopasswd already grants the default user `ALL=(ALL) NOPASSWD: ALL`. It matters on a hardened install, or where the service runs as a user without that blanket rule. Two mutation checks: dropping NOEXEC from a rule fails, and deleting the rules rather than tagging them fails too -- that second one matters, since "make the test pass" and "remove the feature" would otherwise look the same.	2026-08-21 13:03:04 -04:00