mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-04 14:25:08 +00:00
c11fff9760db56f2f92cf33b3c3f3ee030a16886
1983
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 ChuckandClaude Opus 5
|
c11fff9760 |
chore: mark skins unsupported, fix stale docs and preview size, prepare 3.4.0
Skins: no current scoreboard plugin builds on src.base_classes, so the only skin hook (SportsCore._render_game) never runs. The plugin schema endpoint no longer injects the Visual Skin dropdown, the store hides and refuses "type": "skin" registry entries, and GET /api/v3/skins reports supported: false with a message. Stored skin config still loads and saves. src/skin_system/ and its tests are unchanged apart from the support flag. Docs: check_plugin.py/render_plugin.py examples use --plugin; document BasePlugin.get_update_interval() and its interaction with the manifest update_interval; CLAUDE.md drops the stale template line number and recommends display_manager.width/height. Preview size: new src/display_geometry.py holds the size computation and defaults DisplayManager uses (double-sided applied, chain_length default 2). The web preview, /display/current, Starlark magnify default, sync handshake and two dev scripts use it. Release: __version__ 3.4.0, CHANGELOG 3.4.0 section plus a 3.3.0 tag note. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
6d1cbfb70b |
fix(web): plugin config page survives stored values the schema outgrew (#578)
Two stored shapes broke the config form: * A scalar under a field that is now an object. News' dynamic_duration was a boolean and is becoming an object; render_nested_section did `key in true` and the whole page failed to render. Look into dicts only, and carry a legacy boolean over as the object's `enabled`, so the next save upgrades it without switching the feature off. * A custom feed logo with a path but no id. The template always emitted an empty `logo.id` input, which the save route parsed to null, failing the id's string type on every save. Emit it only when there is an id, as custom-feeds.js already does. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
8e6d7c280f |
test(element-style): cover the stateless element_color clamp path (#576)
#569 fixed _normalize_color and #572 covered the resolver path. That test's own docstring notes the resolver "normalizes colour separately from element_color", and the other path had no test: the stateless element_color(), which src.common.sports_card delegates to and which every one of the nine scoreboard plugins takes for each per-element colour it draws. That is the path that regressed. element_color() moved here with the per-element customization framework, the coercion rejected out-of-range components where the reader it replaced clamped them, and a rejection reads as "not configured" -- so one component over 255 painted the element white while the user's colour sat in their config. Every scoreboard's test_element_text_colors.py failed on it, and it took two plugin PRs red on CI to surface. Six cases: clamping, in-range untouched, hex, unparseable fallback, missing element, and agreement with sports_card.coerce_rgb. The last is the point -- the two shared readers disagreed about the same value, so this asserts against coerce_rgb directly rather than restating the arithmetic, and any future move of element_color has to keep them consistent. Verified by mutation: restoring the rejecting coercion fails two of the six, alongside the resolver test from #572. Tests only; no source change. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
dac71afedc |
fix(web): full-height plugin config form and full-width plugin card descriptions (#573)
* fix(web): let the plugin config form use the full page height The form wrapper has carried `max-h-96 overflow-y-auto` since #145, but the class was a no-op until #568 defined `.max-h-96` in app.css. That silently capped the whole config form at 24rem with a nested scrollbar. Drop the cap so the form flows naturally and the page scrolls. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): give installed plugin card descriptions the full card width The enable/disable toggle was a flex sibling of the whole text column (name, metadata, description), so it reserved its width for the full height of the card body. Descriptions wrapped into a narrow strip, leaving blank space under the toggle and making cards very tall. Move the toggle into a header row with just the name and badges, and render the metadata and description below at full width. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
a6b3384032 |
fix(web): show the action script's error in the file-manager widgets (#574)
A failing plugin action returns a 400 whose JSON body carries the script's own message, but both file-manager widgets threw it away: plugin-file-manager's toggle always said "Toggle failed", and json-file-manager's request helper threw "Server error 400" before reading the body. That hid of-the-day's "Category ... not found in config", which is why its toggles looked broken for no reason. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
11bf39cd66 |
fix(web): two plugin config saves that always returned 400 (geochron, news) (#575)
* fix(web): render widget-less arrays of objects as a table, not comma text An array of objects with no x-widget (geochron's `cities`) fell through to the comma-separated text input. Jinja joined each item as a Python dict repr, the save route read them back as a list of strings, and the schema rejected them -- so every save of the plugin returned 400 "Configuration validation failed", whatever setting was changed. Default such arrays to the existing array-table widget, which already edits arrays of objects and posts `field.N.key` inputs the save route rebuilds into a list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): don't leave an empty object stub in array items on save The unchecked-checkbox pass walked into every nested object of an array item looking for booleans, creating it when absent. A news custom feed with no logo came out with `logo: {}`, which fails the logo's `required: [id, path]`, so every save of the news plugin returned 400. Recurse into a scratch dict instead and attach it only if a boolean was actually set in it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Sonnet 5
|
bc60b41445 |
test(element-style): cover ElementStyleResolver's colour clamp path (#572)
* fix(colour): clamp out-of-range text_color components instead of dropping them
_normalize_color returned None for a triple with a component outside 0..255,
and None means "not configured" to element_color -- so configuring
[300, 0, 20] silently handed the element its *default* colour rather than red.
Every scoreboard reads its per-element colours through this path, so the bug
reached all eight.
It is also the odd one out: sports_card.coerce_rgb and
SportsShared._coerce_rgb both clamp, and core's own test is named
test_coerce_rgb_clamps_rather_than_rejecting. The rejecting normaliser arrived
with the shared readers in
|
||
|
ChuckandClaude Opus 5
|
f9b1f87e8d |
fix: clamp colour components, and let the style editor actually take over (#569)
* fix(element-style): clamp out-of-range colour components instead of rejecting
A regression this framework shipped. The eight scoreboards used to read their
colours through sports_card.coerce_rgb, which clamps; routing them through the
shared element_color sent them through _normalize_color, which rejected any
component outside 0..255 and fell back to the default. So a configured
[999, -5, 20] -- a typo'd bright red -- rendered white instead of (255, 0, 20).
Their own test_element_text_colors.py caught it: one case of nineteen, in all
eight plugins, failing only once the core change reached main.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(web): the style editor takes over its own blocks -- and gets to at all
Two defects, both found by rendering the real partial in a browser rather than
by reading the code.
It was losing a race to its own fields. The hand-off guard asked "do any
fallback controls differ from their server-rendered defaults?" as a proxy for
"is someone editing this?". But the fallback holds this block's own font
fields, and the font-selector widget populates them on the same 50ms timer --
so a plain page load, with nobody touching anything, raced into "dirty" and the
editor removed itself, leaving the 701-line accordion form it exists to
replace. Measured: seven customization.*.font selects dirty ~60ms after
injection, clean again by 400ms. The question is whether a *person* typed, and
event.isTrusted answers exactly that; the listeners now go on synchronously,
because the edit worth protecting can happen before initWidget runs.
It took over too much. Taking over removed the whole fallback section, but a
customization block can hold more than styling -- football keeps
favorite_result_colors there -- so that removed the only UI those fields had,
and the editor also rendered them as an element, giving every row an "enabled"
and three colour columns. Core now marks the blocks it recognises as styling
(the compact declaration already did; hand-written adoption did not), the
widget renders only those, and the template drops only the children the widget
reports owning.
Verified on football's real schema: 28 rows across four mode tabs, columns
Element/Font/Size/Colour/X/Y, favorite_result_colors still editable with its
ten inputs, no duplicated field names, no console errors.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(web): style editor no longer drops layout-only fields it never rendered
CodeRabbit flagged elementKeys() in style-editor.js: render() claims the
whole customization.layout child as the widget's own (removing it from the
generic fallback renderer, since posting the same offset twice is worse),
but elementKeys() only listed keys that also have their own top-level style
block. A hand-written schema can put a key under layout that never got one
-- a logo, a timeout indicator, a possession arrow with a position but no
font or colour -- and that key's only control silently disappeared: no row
in the style editor's table (elementKeys never listed it) and no fallback
section either (layout was removed wholesale).
elementKeys() now appends any layout-declared key not already covered by a
style element, so table() renders a row for it (layout columns only, no
style columns) and the wholesale layout ownership claim stays truthful.
Verified against current code before fixing. New regression test
(test/js/unit/test_style_editor_element_keys.js, following this repo's
existing eval-extraction pattern for testing widget JS without a browser)
fails against the reverted function and passes with the fix; added to
run_all.js and the suite table in test/js/README.md.
Full pytest suite: 4887 passed, 62 skipped, 2 failed -- both the
pre-existing Europe/Kiev/Asia/Calcutta tzdata-alias gap on this sandbox,
identical on origin/main, unrelated to this change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dpg3HLWohdCUdzz2QNHanm
* fix(web): style editor no longer strands leaf-valued layout fields
A prior fix on this PR made elementKeys() append any layout-only key with
no style block of its own (a logo, a timeout indicator, a possession
arrow), so table() draws a row for it instead of losing it when the
wholesale `layout` claim removes the generic fallback. That covers a
layout-only key shaped like an object (x_offset/y_offset, ...), because
columnsFor() only ever produced columns from a key's *sub-fields*.
It missed the case where the layout-only key's own value is itself a
leaf -- a plain "show_logo" boolean directly under layout, no x/y object
underneath. elementKeys() still lists it (any row: no matching column),
so it renders as an uneditable blank row and its only control -- the
generic fallback checkbox -- is still gone. Confirmed by executing the
real widget's render() against a synthetic schema in Node (a DOM-stub
harness, not committed): the field's name never appeared as an <input>.
columnsFor() now gives such a leaf key a column keyed to itself
('layout-leaf'), and elementRow() binds it to the leaf's own path
(customization.layout.<key>, matching the name the fallback would have
used) instead of leaving every cell blank.
New regression test (test/js/unit/test_style_editor_layout_leaf_columns.js,
following this PR's existing eval-extraction pattern) checks the leaf
column is produced, is self-keyed, doesn't duplicate, and that a schema
with no leaf-valued layout key is unaffected; wired into run_all.js and
the suite table in test/js/README.md.
test/js/run_all.js: 84 + 6 + 6 = all suites passed (jsdom unavailable
here, DOM suites skip as before). Python suite untouched by this change;
test_style_editor_extra_fields.py, test_style_editor_save_roundtrip.py
and the one PIL-dependent style_editor_takeover.py case fail identically
before this commit -- missing flask/PIL in this sandbox, not this PR.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(web): keep layout-leaf style-editor columns distinct from name collisions
columnsFor() keyed a layout-only leaf field's column by its bare field
name. If an unrelated element's style block or another element's layout
axis block happened to declare a sub-field with that same name, the
`!seen.has(key)` guard skipped creating the leaf's column, silently
dropping its only control again -- the same failure the leaf-column fix
was meant to close, just reached through a name collision (CodeRabbit
review on
|
||
|
ChuckandClaude Opus 5
|
7e580dc005 |
fix(wifi): make Connect work from the setup AP (#571)
* fix(wifi): make Connect work from the setup AP Joining a network from LEDMatrix-Setup has to take the AP down first, which drops the phone that sent the request. The connect endpoint answered only after the attempt finished, so the browser never got a reply and the WiFi tab's Connect button appeared to do nothing. - /wifi/connect answers 202 immediately while the AP is active and connects in a background thread; the result (never the password) is reported via /wifi/status as last_connect_attempt. A second connect while one is pending gets 409. - connect_to_network holds a /tmp flag for the attempt; the monitor daemon skips AP management while it is fresh. Previously the daemon's disconnected counter, accumulated over the whole AP session, re-enabled the AP on its next tick in the middle of the connect. - The WiFi tab and captive setup page explain the handoff up front, and on reopening show why the last attempt failed. The wrong-password message now works: the route sets the error_type the captive page checks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(wifi): serialize connect attempts on both paths Addresses CodeRabbit review on #571: - Check for a pending attempt before branching on AP state. A background attempt takes the AP down long before it finishes, so a second click used to bypass the 409 and start a competing synchronous connect. - Record pending for the synchronous (non-AP) path too, so two requests can't overlap and have the first clear the daemon's in-progress flag while the second is still connecting. - Clear the pending state if the background thread fails to start, rather than refusing every later request until restart. - Say the setup network returns "within a few minutes": a stale flag plus the daemon's grace period can take longer than one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
d51f7ada14 |
chore(scroll): drop the dead sub-pixel path, and two dev-tooling papercuts (#570)
Three independent changes, none of which alter runtime behaviour. 1. Remove ScrollHelper._get_visible_portion_subpixel and _interpolate_subpixel (162 lines). get_visible_portion dispatches only to _blend_visible_portion, so _get_visible_portion_subpixel had no caller, and _interpolate_subpixel was reachable only from inside it -- a closed island. _blend_visible_portion's own docstring already records that the scipy path it replaced was dead; the replacement landed but the corpse stayed. 2. scripts/check_plugin.py: also search ../ledmatrix-plugins/plugins. The scoreboards live in the sibling checkout, so --all silently skipped every one of them and only --plugin-dir reached them. 3. .gitignore: ignore config/.config_secrets.json.tmp.*, which the suite leaves behind several of per run. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
d1e821c625 |
fix(web): harden, polish and optimize the web UI per the Sept 2026 audit (#568)
* fix(web): harden, polish and optimize the web UI per the September 2026 audit Works through docs/archive/WEB_UI_AUDIT_2026-09.md (health 8/20). Implementation integrity (P0) - app.css now defines every utility class the templates and JS use, including .hidden, so the ~145 JS show/hide toggles work. Button reset, and base component rules (.btn, .form-control) wrapped in :where() so utility classes on the same element win. New static-audit test fails when a used utility class has no rule. Accessibility - Focus rings render (the old ring rule referenced undefined variables); one :focus-visible outline everywhere; skip link; labelled nav landmarks. - Shared dialog helper (js/utils/dialog.js): role/aria-modal, focus trap, Escape, focus return, applied to every modal. - Named icon-only buttons and labelled ~70 form fields. - Toasts announced once; errors persist >= 10s; one showNotification. - Captive WiFi page: live region, timeouts, dark mode, 16px inputs. Performance (Pi Zero 2 W) - SSE streams and tab timers pause when hidden or off-tab; the display stream only runs while a preview is visible. app-shell.js deferred. - Widget scripts served as one versioned bundle (/assets/widgets.js): 52 -> 21 script tags, 66 -> 35 requests on first load. - Stdlib gzip fallback when flask-compress is missing: first-load JS/CSS 1358 KB -> 291 KB on the wire. SSE untouched. Theming and responsive - File managers, form fields and Fonts upload on theme tokens; bare inputs themed in dark mode; no more white surfaces. - No horizontal overflow at 375px on any tab; 44px touch targets on coarse pointers; reduced-motion respected; header title truncates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): clear Codacy findings on #568 - json-file-manager: focus-trap releases kept in a Map (no dynamic property access or delete; no value-returning forEach callback) - notification / schedule-picker: style and day-label lookups via Map - app.js: move the pending-queue assignment out of the expression - diff_viewer / error_handler: named function declarations instead of arrow consts No behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: check the OAuth widget ships in the widget bundle base.html no longer tags widget scripts one by one; they load through /assets/widgets.js. Assert the page requests the bundle and the bundle contains google-oauth.js, which is what the test was protecting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): address review feedback on #568 - widget bundle version fingerprints every file (name, mtime_ns, size) - gzip fallback appends Accept-Encoding to an existing Vary header - dialog helper: releasing a non-top dialog no longer moves focus out of the dialog the user is in - labels: file-upload targets its file input; fallback config fields get label for/id pairs; native color input has a fallback name - utility audit also reads class names inside bound :class expressions Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): give the native color-picker input an accessible name CodeRabbit flagged this on PR #568 as an outside-diff finding (never posted inline, so it was missed in the round of fixes that addressed the other 6 review comments). The <input type="color"> only carried a title attribute; screen readers don't reliably announce title, and there's no other label naming the control when showHexInput is false. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): clear Codacy findings in app-shell.js - drop the unused catch binding on the SSE JSON parse - move the pending-notification queue assignment out of the expression No behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): contain plugin widgets/ dir and bound style-editor retries From CodeRabbit review on #568 (code that arrived with the main merge): - serve_plugin_widget resolves widgets/ with resolve_under before resolving the manifest script under it, so a symlinked widgets directory can't become the containment base (CWE-22). New test. - style-editor init stops polling after ~10s when the widget never registers and leaves the plain fallback fields in place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
69d408b321 |
feat(core): one per-element display-customization framework, wired into the web UI (#566)
* fix(sports): rebuild un-shared faces through the pinned layout engine unshare_element_fonts re-instantiates a duplicate font face so two elements can be told apart by id(). It did so through bare ImageFont.truetype, which takes PIL's default layout engine rather than the one src/common/font_layout.py pins. Raqm and Basic disagree on fractional advances -- that disagreement is the reason the pin exists, having broken golden images across machines -- so a rebuilt face could measure differently from the shared face it replaced, on any host where Raqm is installed. These were the only two call sites in src/ bypassing the pin. The guard asserts that the rebuild goes through the pinned loader rather than comparing engine values: where Raqm is absent, bare truetype returns BASIC anyway, so an engine comparison passes whether or not the pin is honoured. The first draft of this test did exactly that and passed with the bug reintroduced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(web): drop the two dead client-side config-form renderers generateConfigForm and generateSimpleConfigForm (580 lines) were defined on the Alpine component and never called: server-side Jinja replaced them, as pages_v3.py:641 records. Nothing in any template invokes them -- there is no x-html in the templates and no bracket access on the component. They carried their own x-widget dispatch, which made them an active trap: the next person adding a widget would reasonably think both renderers needed updating. plugins/config_manager.js (PluginConfigManager, 133 lines) goes for the same reason -- loaded on every page from base.html, referenced only by itself and by an archived doc. Kept, having checked them: widgets/example-color-picker.js is the worked example docs/widget-guide.md points plugin authors at, and widgets/plugin-loader.js is the client half of a documented feature (manifest-declared plugin widgets) whose server route is missing -- soccer-scoreboard already ships a widgets/custom-leagues.js that this loader is meant to fetch. That is an unfinished feature to complete, not dead code to delete. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(web): serve plugin-declared widgets, and actually ask for them LEDMatrixWidgets.loadPluginWidget has always fetched /static/plugin-widgets/<plugin>/<widget>.js, and docs/widget-guide.md has always documented that path, but nothing served it. soccer-scoreboard has shipped a 17KB widgets/custom-leagues.js since August that could never load. Both halves were missing, not just the route: - serve_plugin_widget serves the script from the plugin's widgets/ directory as text/javascript. The manifest is the allowlist -- only a widget the plugin declares is reachable -- so installing a plugin does not publish everything it ships. Path handling mirrors the sibling serve_plugin_web_ui: allowlist regexes, os.path.basename, resolve() + relative_to() containment, and the ledmatrix- prefix fallback. The declared script name is guarded too, since it comes from the plugin rather than the request. - The config form never requested one. Its x-widget dispatch is a hardcoded list of core widget names, so a plugin's own widget fell through to a plain text input. An unrecognised x-widget on a string field now asks ensureWidget() for it. The text input stays as the fallback and is removed only once the widget has actually rendered, so a missing or broken widget costs the user an editor rather than their configured value on the next save. - manifest_schema.json gains "widgets", so the declaration is validated rather than merely tolerated by additionalProperties. Verified in a browser against the real partial: a declared widget loads, registers and renders, and its field posts exactly one value; a field whose widget 404s keeps its text input and still posts its value. Not addressed: loadPluginWidgetsFromManifest still has no caller. The per-field ensureWidget path is lazier and is what the form now uses, so that bulk helper is dead weight -- worth removing, but left alone here rather than inventing a call site for it. Known limitation, documented: only string-typed fields take this path. object/array/boolean/number fields and enums are dispatched by the template's own branches, which still only know core widgets. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(element-style): a wrong-size BDF now keeps its font, not its size BDF fonts are fixed-size bitmap strikes: FreeType accepts only the pixel size baked into the file and raises for anything else. 32 of the 35 shipped fonts are BDF, so a size picked in the web UI usually is not a valid strike -- and load_font caught that failure with its generic "unloadable font" handler, which substitutes PressStart2P. Asking for 5x7.bdf at size 10 therefore rendered a completely different typeface, silently. It now falls back to the file's own native size instead, which is what SportsCore._load_custom_font_from_element_config has always done. The native size is read via FontManager._read_bdf_native_size rather than a fourth copy of that parser, matching how core.py already delegates. Also here, because they are the same code path: - native_bdf_size() is exposed for the web UI, which needs to know when a size field can take effect at all. None means "free choice". - ElementStyle.font_size now reports the size actually realised rather than the one requested. Callers lay out from it, and reserving space for a size nothing was drawn at is how this surfaces. - The module font cache is a bounded LRU (256) instead of an unbounded dict. The display process runs for weeks and every config save can add a (font, size) pair; every other hot cache in the codebase is bounded this way. Untouched configs are unaffected: the shipped classic fonts are the three TTFs, so nothing was hitting the substitution path by default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(element-style): per-mode style and offset overrides Lets one element be styled differently per situation -- a scoreboard's live / upcoming / recent cards, weather's current / hourly / daily screens -- under customization.modes.<mode>. The mode is bound at construction rather than passed per call. That is what makes this cheap to adopt: SportsUpcoming and SportsRecent are already separate instances with distinct SKIN_MODE values, so binding once makes every existing style()/offset_value() call site mode-aware without editing any of them. A per-call mode argument exists for the rare host that renders more than one mode. The two layers answer different questions, deliberately: - The base layer keeps the existing "differs from the schema default" rule, because the save flow writes the full default object into config.json whether or not the user touched it. - A mode layer is pure override -- its fields default to None, so presence is intent. Nothing writes into it unasked, so there is nothing for the stricter rule to protect against. None therefore means inherit, and has to stay distinct from 0: a mode y_offset of 0 means "sit at the base position", not "no preference". This is the same distinction scroll_card.switch_* draws with "inherit". A malformed mode value falls back to the resolved base value rather than to the caller's default -- caught by the degradation tests, which is what they are for: resolving the mode first let one bad string in a mode block silently discard a good base offset. With no modes block, and for every existing caller, resolution is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(element-style): declare per-mode overrides in config_schema.json A plugin adds "x-style-modes": ["live", "upcoming", "recent"] alongside its x-style-elements declaration and gets a customization.modes.<mode> group per mode, with every field of every declared element repeated as an override. Those override fields are typed nullable and default to null, which is the whole trick. The save flow writes schema defaults into config.json wholesale, so giving a mode field the base element's default would make every mode a frozen copy of the base the first time a user pressed Save, and the base would stop reaching them. Null means inherit. The mutation test for this is explicit: with concrete defaults, a base font_size of 14 resolves as 10 with user_forced set. min/max from the declaration carry into the mode blocks, so an out-of-range override is rejected by validation rather than clamped silently at render time. Also: the emitted font field now carries "x-widget": "font-selector". The widget already shipped and the config form already allowlisted it -- the hint was simply never emitted, so the field rendered as a bare text box that the user had to type a font filename into. Verified through the real SchemaManager path -- load_schema, defaults extraction, merge_with_defaults, validation, then resolution -- rather than against a hand-built dict, since the thing at risk is what that pipeline does to a null. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): render the config form from the schema the save route validates The form read config_schema.json with a raw json.load while api_v3.save_plugin_config went through SchemaManager. Those are not the same schema: SchemaManager applies expand_style_elements, which turns a compact customization.x-style-elements declaration into the per-element blocks the form knows how to render. Without it, that customization object has an x-style-elements key and no "properties", so the template's object branch matched nothing and the section rendered as empty space -- while saving still validated against the expanded shape. of-the-day ships the compact form, so its customization section has been invisible in the web UI. pages_v3 gains a schema_manager the way it already has config_manager and plugin_manager. use_cache=False matches the save route, so an edited schema is not served stale during plugin development. The raw read stays as a fallback for callers that register this blueprint without one. Checked before making the change: load_schema does nothing here except read, validate and expand -- inject_skin_selector is a separate method it does not call -- so this is not a behaviour change for schemas without the declaration. The test pair renders the same compact schema with and without a SchemaManager, so it documents exactly what was broken as well as what is fixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(web): style-editor widget -- a row per element instead of 65 accordions Rendered element by element, a realistic scoreboard's customization block is 65 nested sections, and reaching one per-mode font size takes five levels of expanding. The widget collapses that to one compact row per element -- font, size, colour, X, Y -- with a tab per declared mode. It emits ordinary inputs under the same dotted names the generic renderer would produce, so the save/validate/merge pipeline is untouched: no hidden JSON blob and no new server-side parsing. It is driven entirely by the schema block it is handed, so fields added to the schema later appear without editing the widget. If it fails to load or throws, the generic nested rendering it replaces is left in place. Fixing two things the save path got wrong for nullable fields, found by posting what the widget actually emits: - The indexed-array recombiner (text_color.0/.1/.2 -> one list) compared the declared type to the string 'array', so a per-mode colour, typed ["array", "null"], was never reassembled and failed validation on save. _parse_form_value_with_schema had the same comparison. - A blank nullable field became [] rather than None, which then failed the minItems the colour array declares. Null is the inherit sentinel, so it has to survive. And two things the widget itself got wrong, found by looking at it: - An unset base control fell back to the select's first option, so an untouched scoreboard claimed every element used 10x20.bdf -- and the size box then locked itself to that bitmap font's fixed size. Base controls now show the schema default; mode controls stay blank, because blank there means inherit. - Elements arrived alphabetised (Detail and Odds above Score). Flask's JSON provider sorts keys, so declaration order has to be stated explicitly; expand_style_elements now emits x-propertyOrder, which the generic renderer already honoured too. Size is disabled and shown as fixed for a bitmap font, using the scalable/native_size the font catalog now reports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(element-style): visibility, alignment and scale per element Completes the customization vocabulary: hide an element, align it, and resize a logo, alongside the font/size/colour/offset that already existed. All three per mode. They resolve to "change nothing" until the user asks for something -- True, None and 1.0 -- rather than to whatever the schema declares. That is the same invariant the font fields keep: a caller that honours them still renders an untouched config exactly as it did before they existed. A schema default therefore does not count as a choice, which matters because the save flow writes that default into config either way. scale sits in the layout block with the offsets rather than in the element block, because it is geometry: a logo has a scale and no font. The widget's columns come from the schema, so a logo row shows visibility, offsets and scale and no empty font cell. Two bugs found by the tests rather than by reading: - A nullable enum needs null in its enum list, not just in its type. The mode copy of `align` defaulted to null and then failed its own schema, so a plugin declaring any enum field with modes could not save at all. Six tests failed on this before any of them reached what they were testing. - defaults_from_schema only ever extracted font/font_size/text_color, so the schema defaults for the new fields were invisible to the resolver and a declared default read as a user choice. Widget: the table scrolls horizontally and pins the element-name column. Nine columns do not fit the config panel, and clipping them hid the offsets entirely while scrolling them made every row anonymous. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(element-style): resolve elements under the names plugins actually use Two naming conventions collided as the scoreboards grew. Counted across the published schemas: the style block names elements with a _text suffix (score_text, status_text, detail_text), while the layout block mostly uses the bare noun (score, date, time, odds) -- except status_text, which kept the suffix in seven plugins and lost it in two. records vs record splits seven to two the same way. A lookup now tries the exact name first and then the spellings that mean the same thing. Exact-first is what makes this inert for any config that already matches; the aliases only decide cases that resolved to nothing before. This is also what makes migrating to the compact declaration form safe. That form uses one key for both blocks, so a scoreboard adopting it asks for layout.score_text while its users have layout.score saved -- without the aliases, every offset they had dialled in would silently become 0. Applies to the style block, the layout block, the schema defaults and the per-mode overrides, since the drift shows up in all four. Not attempting to canonicalise on write: renaming keys in config.json would break the plugins still reading the old spelling from their own bundled code, and the drift costs a dict miss rather than correctness. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(plugins): BasePlugin.styles -- per-element styling every plugin inherits Adopting the element-style system meant repeating three things in every plugin: a guarded import, finding its own config_schema.json, and rebuilding the resolver when on_config_change swapped the config dict. This is those three things once, on the class all 45 plugins already inherit from. title = self.styles.style('title_text', classic_font='PressStart2P-Regular.ttf', classic_size=8, classic_color=(255, 255, 255)) The classic_* arguments are the adoption contract: with nothing configured they come back verbatim, so a plugin that switches to this renders exactly as before until a user changes something. A plugin with one instance per display mode sets STYLE_MODE on the class and every existing lookup becomes mode-aware without a call site changing -- which is the point of binding the mode to the resolver rather than passing it per call. styles_for() covers a plugin that renders several modes from one instance. Schema discovery reads the concrete class's own module rather than this file, because this file lives in src/plugin_system where no plugin schema exists -- the same trap SportsCore._config_schema_path documents. The first mutation test for that passed anyway: an installed plugin's module directory and its entry under plugins_dir are the same path, so the test could not tell the two apart. The case where they diverge is a plugin symlinked in for development, and the test now forces that shape. Getting discovery wrong is silent rather than loud: with no schema the resolver has no defaults to compare against, so every configured value reads as a deliberate override and the plugin quietly stops honouring its own shipped styling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(element-style): adopt hand-written customization blocks, and widen the font list Nineteen plugins spell their style elements out longhand instead of declaring them -- football's block is 701 lines for seven elements -- and predate this system entirely. Core now recognises that shape, so they pick up the row-per-element editor and the real font picker on a core update rather than on a plugin release. Checked against every published schema: 21 plugins adopt, and the defaults of each still validate against the schema generated for it. Detection requires *every* field in a block to be one this system understands. A looser "has at least one style field" rule sweeps in baseball's `count`, which carries a text_color beside geometry that means nothing here. That distinction took three attempts to test: the first two assertions passed under both rules, because an over-eager rule leaves a fontless block looking untouched and only surfaces as an extra row in the editor. The hardcoded font enum is replaced rather than extended. Football lists five of the thirty-five installed fonts, which is why a font a user uploads can never appear in one. It is not a curated safe set -- it omits some twenty other faces that fit the declared size cap just as well -- it is the fonts that happened to exist when it was written. Widening it does need a guard, though, and not the one the schema already has: a bitmap font ignores font_size and renders at its size baked into the file, so `maximum: 16` cannot stop a 27px face. The picker now filters out fixed-size fonts taller than the element's own declared ceiling, which drops exactly the four that would overflow a 32px panel and keeps the other thirty. Per-mode overrides stay opt-in: core cannot invent a plugin's display modes, so `x-style-modes` remains the one line that unlocks them. Their layout half covers every positionable element rather than only those with a style block -- the two namespaces do not line up in a hand-written schema, and football positions six things (logos, timeouts, possession) that have no style block at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(web): remove the two Fonts-tab panels that reported invented data "Element Font Overrides" let a user configure an override, showed a success toast, and changed nothing. All three endpoints behind it were stubs -- GET returned a hardcoded {}, POST and DELETE returned success without calling anything -- each marked "This would integrate with the actual font system". Wiring them to FontManager would not have fixed it. The machinery there is real (_load_overrides/_save_overrides persist config/font_overrides.json, resolve_font applies them, and the countdown plugin genuinely consumes it), but the panel's element dropdown offered eleven invented keys -- nfl.live.score, clock.time, weather.current -- that no plugin has ever read. An override saved against one of those would have persisted correctly and still done nothing. "Detected Manager Fonts" goes for the same reason. It claimed to show "fonts currently in use by managers (auto-detected)"; its own comment said "we'll simulate this", and it listed every font in the catalog with a hardcoded usage_count of 1 -- the panel beside it, with fabricated numbers attached. Per-element font choice now lives in each plugin's own config editor, against the elements that plugin actually has, and covers size, colour, offsets, visibility, alignment and scale rather than family and size. Kept: the font library (upload, preview, delete), which works, and /fonts/tokens, which is a stub but genuinely feeds the preview's size dropdown. FontManager's override methods are untouched -- countdown uses them. Verified in a browser with the tab's JS running: no console errors, 35 fonts listed, upload and preview intact. Removing the panel meant unwiring it from populateFontSelects too, which would otherwise have bailed out early on the missing select and left the preview dropdown empty. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(sports): one reader for element colours and layout offsets There were two copies of the per-element colour read and three of the layout-offset read. They had already drifted -- the scroll-card renderer carries a comment about having ignored offsets its own schema advertised -- and each new capability had to be added to all of them or silently work in some places and not others. All of them now go through src.element_style, which is what carries the alias handling and the per-mode lookup. That lands immediately for the nine plugins importing these modules: a scoreboard asking for `score_text` offsets finds the `layout.score` its users configured, and a Live instance resolves its own colours through SKIN_MODE without any call site passing a mode. _normalize_color learned "#RRGGBB" in the process. The scoreboards' own readers have always accepted it, so the shared one had to, or consolidating would have quietly dropped a form users' configs may hold. _coerce_offset picked up the non-finite guard the scroll-card reader had and the other two did not. _get_layout_offset is promoted onto SportsCoreSharedMixin. Each plugin still carries its own copy in its bundled sports.py, which wins by MRO -- so adopting this is a deletion in the plugin, and until that deletion nothing changes for it. Note for whoever runs the suite next: test_display_dirty_tracking.py is order-dependent. Fifteen of its tests failed in one full run and passed in the next with no change in between, and pass in isolation. Pre-existing, unrelated to this, but it makes a full-run diff untrustworthy until it is fixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(changelog): record the element-style work under Unreleased This file's own preamble asks for it: a plugin may delete its bundled fallback copy of a core module only when its manifest floors on the first release that shipped that module, which requires the additions to be recorded here against a version. Names a plugin can now import and floor on -- the stateless layout_offset and element_color readers, alias_keys, native_bdf_size, the resolver's mode binding, BasePlugin.styles, and the promoted SportsCoreSharedMixin._get_layout_offset -- plus the schema and web-UI changes, the four fixes and the three removals. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(fonts): log the BDF native-size read failure instead of swallowing it The bdf-native-size lookup in get_fonts_catalog() caught any exception and silently discarded it. Every other guarded read added in this PR (the manifest parse in _declared_widget_script, the SchemaManager fallback in _load_plugin_config_partial) logs before falling through to the same degraded behavior. This one didn't, which is the shape a silent-exception-swallow lint rule flags. Behavior is unchanged -- native_size still comes back None -- but a corrupt or unreadable BDF file now leaves a trace. Verified: font-related tests (140) and the full suite still pass, with only the 2 pre-existing Europe/Kiev/Asia/Calcutta tzdata-alias failures already present on origin/main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address CodeRabbit findings on the style-editor/font-selector PR - Fix _load_font_sized double-wrapping the (font, size) tuple on the missing-font path, which handed callers a tuple instead of a font. - Fix _set_nested_value skipping an explicit None when the key already existed, which silently kept stale overrides when a user cleared a nullable per-mode field or blanked all channels of an indexed color. - Preserve BDF scalable/native_size metadata through fetchFontCatalog's catalog-format mapping so maxFixedSize filtering actually applies. - Stop caching an empty array on a failed font-catalog fetch so a later call can retry instead of being stuck with the failed result. - Keep a saved font selected in the style editor even when it no longer fits a newly declared maxFixedSize, instead of silently deselecting it. - Don't drop in-progress user edits to fallback fields when a plugin widget finishes loading asynchronously and takes over the form. - Tighten the removed font-override endpoint test to assert 405, not just != 200. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): a partial save no longer switches off checkboxes it never showed An HTML checkbox posts nothing when unchecked, so the save route walked the schema and forced every boolean missing from the form to False. That is right for the rendered form and wrong for every other caller: a script, the MQTT bridge or a curl against the documented endpoint never rendered a checkbox, and reading its silence as "all off" turns a one-field save into a mass disable. Found on hardware. Posting four customization.* keys to a live device switched off nfl.enabled, ncaa_fb.enabled and every display-mode toggle in one request. The form now reports the top-level sections it drew (__rendered_section), and inside those an absent checkbox still means unchecked -- including a section whose only fields are checkboxes that are all off, which no heuristic could recover. A post with no marker only touches objects it actually posted a field from. Meta fields are dropped before form keys are treated as config paths, because unknown keys are otherwise written straight into config.json. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(sports): resolve element colour by name, and honour visible/align/scale Two of the three gaps this framework shipped with. Colour by name. A draw resolved its colour by comparing the *identity* of the font object it was handed, which cannot tell two elements apart when they share a face -- so those draws went out white. Every bitmap font is in that case, because a freetype.Face cannot be re-instantiated to un-share it, which is how an element rendered in any of the 32 shipped BDF fonts silently lost a colour its picker had offered all along. _draw_text_with_outline now takes element="score_text" and reads the colour by name; the identity path remains for un-annotated callers, but narrows before giving up -- one configured colour among the sharers is the only thing the user can have meant. Visible, align and scale. The resolver has understood these since the framework landed and nothing consumed them: an element could be marked hidden in the web UI and still render. Adds the stateless readers, the mixin accessors, and a scale parameter on the one shared logo-sizing seam (keyed into the cache, so two elements scaled differently cannot be served each other's image). Naming an element in a draw also honours its visibility. Untouched configs are unaffected: every new parameter defaults to today's behaviour, and all ten affected plugins render pixel-identically to main across every harness size. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(plugins): how to declare styleable elements; harden the widget's lookups The plugin-author guide for the compact x-style-elements declaration -- what each key does, how to read values back without breaking the "user-forced only when it differs from the default" rule, and why a hand-written block needs no changes to be adopted. Also clears the static-analysis findings on style-editor.js. Every lookup in that file is keyed by something out of a schema or a saved config, so a key of __proto__ or constructor would walk the prototype chain and hand back a function instead of a schema; reads now go through an own-property helper. The panel registry became a list, and the flagged vars moved to their function roots. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): clear the remaining static-analysis findings Five, all on lines this branch touched. The Python one is not a new defect: _set_missing_booleans_to_false's first parameter was always named `config`, which shadows the `config` submodule imported for its side effects at the bottom of this module. Editing the signature simply put the existing warning on a changed line. The parameter is the plugin's config dict, so `plugin_config` is what it should have been called anyway; callers pass it positionally and are unaffected. The JavaScript ones are the object-injection rule firing on reads keyed by data. own() now goes through a property descriptor, so the one unavoidable data-keyed read is no longer a computed member access; at() consumes its path instead of indexing it; and the column set is a Map, which has no prototype to pollute and needs no guarded reads at all. Verified the widget still renders identically against football's real schema: 29 element rows, all four mode tabs, values populated, no console errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): drop the hasOwnProperty alias the descriptor read made redundant own() now reads through Object.getOwnPropertyDescriptor, so the alias it used to call has no remaining reference. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
92ac231138 |
fix(fonts): load 4x6 on its pixel grid, from any working directory (#565)
* fix(fonts): load 4x6 on its pixel grid, from any working directory `extra_small_font` loaded 4x6-font.ttf at 6, off the face's 7px grid. Under `draw.fontmode = "1"` the mono rasteriser thresholds each glyph at 50% coverage, so every glyph lost its fourth column and deformed: christmas-countdown rendered "UNTIL" as "VM1JL". The advance is 5px at both sizes, so snapping to 7 reflows nothing. - Sizes in DisplayManager._load_fonts go through crisp_size() instead of literals. crisp_size / FONT_PIXEL_GRID / FONT_NAME_ALIASES move to src/common/font_layout.py; sports_card re-exports them. - Mirror the fix in VisualTestDisplayManager, the harness's fork of _load_fonts. Without it every golden is blessed at the old size. - Resolve bundled font paths against the install root, not the cwd. FontManager._resolve_asset_path now delegates to font_layout.resolve_asset_path (kept by name; plugins probe for it). - The startup banner's middle rung snaps to 7; the 5 rung stays off-grid on purpose (the only size that fits a dotted quad on 64px). - loading.py reads all plugin JSON as UTF-8 (cp1252 on Windows aborted check_plugin.py on a 0x9d byte). - check_plugin.py reports in ASCII and never dies on an unencodable char. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(fonts): resolve relative asset paths from the install root, not the cwd resolve_asset_path checked os.path.exists(relative_path) unconditionally, so a relative asset path was still resolved against the process cwd first -- exactly the dependency this module exists to remove. An unrelated working directory that happens to contain assets/fonts/4x6-font.ttf (a stale checkout, a copied assets folder, another project) would shadow the real bundled font instead of the install root ever being consulted. Only an absolute path is now returned as-is; a relative path always resolves against _INSTALL_ROOT first, matching the docstring's stated contract. FontManager._resolve_asset_path delegates to this function, so it's covered by the same fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
772258f73e |
docs: add PRODUCT.md and September 2026 web UI audit (#567)
* docs: add PRODUCT.md product context for web UI design work Captures durable product truth (users, positioning, operating context, constraints, principles) so design passes on the web control panel share one source. Open decisions (offline-only, CSS build step, WCAG target) are recorded as undecided rather than adopted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: add PRODUCT.md and September 2026 web UI audit PRODUCT.md captures durable product context (users, positioning, operating context, constraints, principles) for web UI design work. Open decisions (offline-only, CSS build step, WCAG target) are recorded as undecided rather than adopted. docs/archive/WEB_UI_AUDIT_2026-09.md records the technical audit of web_interface/ (8/20): the hand-rolled Tailwind subset in app.css leaves 333 used utility classes undefined (including .hidden), focus rings never render, modals lack dialog semantics, and SSE/polling never pause. Includes a verified-and-rejected section so the cache-busting false positive is not re-raised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
9ad7528c9b |
fix(config): stop same-second backups overwriting each other (#564)
* fix(config): stop same-second backups overwriting each other
A backup's version is its identity. save_config_atomic() hands the path
back, rollback_config(backup_version=...) looks that version up, and the
paired secrets backup is found by reusing the same string.
The version was stamped at second granularity, so two saves inside the
same second produced the same filename and the second shutil.copy2()
silently overwrote the first backup. The path a caller was still holding
then pointed at different content, and rolling back to it restored the
wrong config. A user saving twice in quick succession lost a restore
point with no error.
list_backups() made it worse. It parsed the version off Path.stem, which
drops only the last dot-component, so for config.json.backup.20240101_120000
parts was ['config', 'json', 'backup'] and parts[-2] was 'json' -- never
'backup'. The filename branch was unreachable: every backup fell through
to the mtime fallback and reported a second-granularity restamp of its
mtime rather than the name on disk, so a unique filename alone would not
have been enough for rollback to find the right version.
Stamp microseconds, and never overwrite an existing backup -- on a
collision bump a -N suffix rather than lose a restore point. Parse the
version off the exact glob prefix so it round-trips with the filename,
still reading the legacy second-granularity format so restore points that
predate this keep working.
Two tests had encoded the bug:
- test_multiple_config_changes asserted a rollback produced plugin1=45
with plugin2=15, a state no single backup ever held -- 45 was only in
the second backup, 15 only in the first. It passed because the two
saves collided onto one file, so the first version resolved to the
second's content. Corrected to the state that backup actually holds.
- test_backup_rotation asserted against a hardcoded max of 3 while
setUp configured 5, and still passed: every save in its loop collapsed
onto a single filename, so there was only ever one backup to count and
rotation was never exercised. It now asks the manager for its limit
and overshoots it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(config): fold collision suffix into ordering, close backup-path race
_parse_backup_version() stripped any trailing "-segment" unconditionally,
so a collision-suffixed backup parsed to the exact same timestamp as its
sibling and list_backups() had no deterministic way to order them. Only
strip the suffix when it's numeric, and fold it back in as extra
microseconds so same-tick collisions sort newest-first reliably.
_create_backup() also checked backup_path.exists() before shutil.copy2(),
which two concurrent callers can both pass for the same path -- the second
copy2() then silently destroys the first call's restore point. Reserve
each path (config and, when configured, secrets) with exclusive file
creation instead of a check-then-copy, retrying on a real conflict.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
ChuckandClaude Opus 5
|
6b3028ad58 |
test: isolate DisplayManager globals across modules, and name the failure (#563)
Follow-up to #562. That commit fixed the actual cause of the intermittent 15-test failure in test_display_dirty_tracking.py -- the emulator's fixed TCP port 8888, a machine-wide singleton that a concurrent pytest process takes away. This adds the two things that would have made it a five-minute diagnosis instead of a long one, and closes the other door into the same failure. Confirmed the module is order-independent as it stands, on this checkout: pytest test/ -q, three times 115 failed / 4464 passed / 63 skipped, byte-identical failure sets, the module 21/21 passed each time module forced last (197 files first) identical failure set module forced first identical failure set module after each of test_display_manager, test_display_controller, test_display_controller_vegas_tick, test_skin_system, test_sports_scroll, test_initial_update_budget, test_display_double_parity, test_initializing_screen all pass four concurrent processes on the file 21/21 each And reproduced the original, to be sure the diagnosis in #562 is the whole story. Holding 0.0.0.0:8888 from a separate process: HEAD's test/conftest.py 21 passed pre-#562 test/conftest.py 15 failed, 6 passed The 15/6 split is not arbitrary: the six survivors are the only tests in the file that never touch dm.matrix. conftest.py: DisplayManager is a process-wide singleton and the RGBMatrix / RGBMatrixOptions names it constructs through are module globals, bound once at import. All three are shared by every test module in the run, so a module that leaves an instance in _instance -- or leaves patch('src.display_manager. RGBMatrix') standing -- changes what the NEXT module builds, invisibly, and only in a full run. A module-scoped autouse fixture now resets the singleton and restores either binding if a patch outlived its module. Module-scoped rather than per-test so that files sharing one manager across their own tests keep doing so; only the leak across the module boundary is cut. Autouse fixtures are set up ahead of requested ones, so this is finalised after a module's own DisplayManager fixture. Verified with a throwaway pair of probe modules -- one leaks a patch and a singleton, the next asserts both are clean -- which passed and were then removed. test_display_dirty_tracking.py: _setup_matrix() swallows every construction failure and falls back to matrix=None, so a broken environment arrived as fifteen identical "'NoneType' object has no attribute 'SwapOnVSync'" errors naming neither the fixture nor the cause. The fixture now fails once, and says where to look; under a held port it reads DisplayManager fell back to matrix=None: RGBMatrix construction raised... Known causes: the emulator adapter losing a fixed TCP port to another process -- see pytest_configure in test/conftest.py -- or a patch('src.display_manager.RGBMatrix') leaked from an earlier test module. with WinError 10048 in the captured log directly above it. No regressions: full suite with both changes is 115 failed / 4464 passed / 63 skipped, failure set identical to the pre-change baseline. The 115 is the pre-existing Windows-environment baseline (os.geteuid, POSIX modes, fcntl); CI on Linux remains authoritative. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
59997594ac |
test: fix the emulator port collision behind the intermittent suite failures (#562)
* chore: stop tests and rigs writing to shared paths Two shared-state problems, both of which show up as a permanently dirty checkout or an unreproducible test failure. test_display_dirty_tracking.py builds a real DisplayManager, whose _snapshot_path defaults to the fixed /tmp/led_matrix_preview.png that the web UI reads. Every pytest process on the machine shares that one file, so two concurrent runs -- CI shards, a second worktree, an agent running the suite alongside -- overwrite each other's snapshot and the mtime assertions stop meaning anything. The module fixture now points it at a session-unique temp path; the individual tests that care still override it further. To be clear about what this does and does not fix: this is a real shared-path hazard, but it is NOT the cause of the intermittent 15-test failure in that module. That turned out to be the emulator's fixed TCP port, fixed in the follow-up commit. This change stands on its own merits. web_interface/app.py writes data/plugin_operations.json, data/plugin_state.json and data/operation_history.json as the web interface runs, into a directory that ships tracked (data/.gitkeep) and was otherwise unignored. So every rig that ever opened the web UI -- and every test run that constructs the app -- left three untracked files behind and a permanently dirty `git status`. Only data/.gitkeep is tracked under data/, so the negation keeps it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: stop the emulator binding a fixed port, so concurrent runs can't collide This is the cause of the intermittent full-suite failures we have been chasing: runs of identical code landing anywhere between 100 and 130 failures, while every implicated test passed in isolation. Six test modules set EMULATOR=true and build a real DisplayManager. The repo's emulator_config.json selects the "browser" adapter, which binds TCP port 8888 to serve the dev preview. That port is a machine-wide singleton, so a second pytest process -- a CI shard, another worktree, an agent running the suite alongside -- loses the bind. RGBMatrix construction then raises, DisplayManager catches it and falls back to `self.matrix = None`, and every test that subsequently touches the matrix dies with AttributeError: 'NoneType' object has no attribute 'SwapOnVSync' which names neither a port nor a socket, and points at the wrong file entirely. Because test_display_dirty_tracking's fixture is module-scoped, all 15 of its matrix-touching tests fail together or not at all -- the 15-test swing that made the totals look random. Demonstrated rather than assumed. Holding 0.0.0.0:8888 from a separate process and running test_display_dirty_tracking.py: without this change 15 failed, 6 passed with this change 21 passed The "raw" adapter renders in memory and binds nothing. Only display_adapter is overridden, in a throwaway config written per pytest process; the repo's emulator_config.json is untouched and `run.py -e` still opens the browser preview on 8888. Nothing in the suite referenced the adapter, and the tests wrap SwapOnVSync on the matrix object itself, so they are indifferent to what sits underneath. allow_adapter_fallback is forced off -- falling back would land us on the browser adapter and its fixed port, which is the whole problem. CONFIG_PATH is a bare relative filename resolved against the CWD, so it is set to an absolute path: the previous behaviour depended on where pytest was invoked from, and silently wrote a default config into whatever directory that was. Verified no regressions: full suite on this branch and with origin/main's versions of the touched files, same machine, back to back -- 115 failed / 4347 passed on both sides, zero failures unique to either. That 115 is the pre-existing Windows-environment baseline (POSIX file modes, fcntl, shell scripts, Linux-only binaries); CI on Linux remains authoritative. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore: mark the shell entry points executable Eleven scripts shipped as 100644, so `./scripts/install/configure_web_sudo.sh` fails with "Permission denied" and only works if you know to prefix `bash`. That one matters most: the web UI's own error hint, added in #560, tells users to run exactly that path when a system action fails for want of passwordless sudo, and following that instruction verbatim did not work. All eleven carry a shebang and are invoked directly, never sourced. The two sourced libraries -- lib_lowmem.sh and lib_systemd_render.sh -- are deliberately left non-executable, which is what distinguishes a library from an entry point. Mode bits only, no content: 11 files changed, 0 insertions, 0 deletions. Applied with `git update-index --chmod=+x` because this checkout is on Windows, where core.fileMode is off and the working-tree bit is not tracked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
f6367d63ae |
security: triage the CodeQL backlog — 129 alerts, three of them live (#561)
* fix(web): escape quotes in every HTML escaper, not just & < >
The escapers are all `div.textContent = x; return div.innerHTML`. That
round-trip escapes &, < and > -- the only characters the HTML serializer
must escape in a text node -- and leaves quotes alone. Every widget then
interpolates the result into a quoted attribute value:
value="${escapeHtml(v)}" title="${escapeHtml(v)}"
so a value of `x" onmouseover="alert(1)` closes the attribute and adds an
event handler of its own. CodeQL reported this 83 times
(js/incomplete-html-attribute-sanitization) across the widget files.
It is one bug, not 83: the widgets each carry a standalone fallback that
did escape quotes, but they all prefer BaseWidget.escapeHtml when
window.BaseWidget exists -- which it always does in the shipped page -- so
the correct fallbacks were dead code and the incomplete shared one ran.
Fixed at each source instead of at the call sites.
app-shell.js already documented this exact gap in a comment and worked
around it by building DOM nodes by hand; that workaround stays (setting a
property cannot be got wrong), the comment is now accurate.
cache.html's delete button interpolated the cache key into
`onclick="deleteCacheFile('...')"`. Escaping cannot help there -- the
browser HTML-decodes the attribute before parsing it as JS, so `'`
becomes a real `'` again -- so the key moves to a data-cache-key
attribute that the handler reads back.
url-input.js additionally wrote a value straight into an <a href> after
validating it against a schema-supplied protocol list, and that list
accepted any RFC 3986 scheme -- "javascript" included. Scriptable schemes
(javascript, data, vbscript, blob, filesystem) are now refused both when
the list is normalised and when a URL is checked against it, and the
render path routes its href through the same check instead of emitting
whatever was stored (js/xss-through-dom).
test/js/unit/test_html_escaping.js reads each escaper out of the shipped
file and runs it, so losing the quote handling again fails a test rather
than a scan.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(security): stop request-supplied names from reaching paths outside their base
Three of the py/path-injection alerts were live, not lint:
* GET /api/v3/plugins/<plugin_id>/static/<path:file_path> read any file
whose resolved path *string-prefixed* the plugin directory. Flask's
default converter forbids a slash but not dots, and
get_plugin_directory('..') returned the parent of the plugins directory
because it exists -- so every file under the project root then prefixed
that directory, config/config_secrets.json included. The prefix check
was also wrong on its own terms: with plugin dir "plugin-repos/foo",
"../foo-evil/x" resolves to "plugin-repos/foo-evil/x", whose string does
start with "plugin-repos/foo".
* POST /api/v3/plugins/of-the-day/json/delete interpolated the request
body's file_id into f"{file_id}.json" and unlinked it, unvalidated. A
file_id of "../../../../etc/something" deleted that file. This is the
one finding in the batch that destroyed data rather than exposing it.
* POST /api/v3/cache/delete passed the body's key through
CacheManager.clear_cache to DiskCache, which joined it as a filename and
called os.remove. Same shape, same result. The guard goes in
DiskCache.get_cache_path, the single choke point get/set/clear share, so
every caller is covered rather than just this route. Real keys are the
stems of files already flat in the cache directory -- that is how
list_cache_files derives them -- so nothing legitimate is turned away.
The rest of the cluster (web_interface/app.py's asset route, the plugin
update handler, _get_plugin_version, the plugin-schema read in config.py)
was guarded in ways that held, but each had grown its own version of the
check. They now go through one helper, src/common/path_safety.py, which
returns the *sanitised value* rather than a verdict -- so a caller cannot
validate one string and open another, which is how the two real bugs
above were shaped.
Also: WiFiManager.connect_to_network took the SSID and password straight
from POST /api/v3/wifi/connect into nmcli's argv. There is no shell there,
so CodeQL's py/command-line-injection alert overstates the risk -- but
nmcli reads a leading "-" as an option, so an SSID of "--ask" asks nmcli
to run differently rather than to join a network. Both values are now
checked for shape (802.11's 32-octet SSID limit, WPA's 8-63 char
passphrase or 64-char hex key, no control characters, no leading dash)
before any subprocess runs.
test/test_path_traversal_guards.py asserts on the filesystem, not just
the status code: a handler that returns 403 and deletes the file anyway
would pass the weaker check. Twelve of its cases fail against the
unpatched code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(web): refuse a plugin id that is not a plain name, don't truncate it
pages_v3 and scripts/dev_server.py ran request ids through
os.path.basename and carried on with what came out, so "../weather"
rendered the config form for "weather". Nothing escaped the plugins
directory -- the relative_to guards held -- but the handler answered a
request nobody made, and validating one string while the filesystem sees
another is the shape both live traversals earlier in this branch had.
Same treatment as the rest: safe_path_component rejects rather than
truncates, resolve_under returns the path it checked, and the call sites
use what those return. The three handlers that had hand-rolled
resolve-and-relative_to blocks lose about twenty lines to the shared one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(web): say what the plugin web_ui iframe actually is
The docstring claimed the fragment runs "in a sandboxed iframe". The
iframe in plugin_config.html carries no sandbox attribute, so the
fragment runs with the interface's own origin. That is fine -- the file
belongs to an installed plugin, and an installed plugin already runs
Python on the device, so the trust boundary is install rather than this
route -- but a comment promising containment that is not there is worse
than no comment. This is the context for the py/reflective-xss alert on
this handler.
Also drops the now-unused os/os.path imports.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(web): inline url-input's scheme guard at the previewLink.href sink
CodeQL flagged this line as a new high-severity js/xss-through-dom alert
on this PR even though it is already covered by SCRIPTABLE_SCHEMES: the
guard reached the sink through safeHref -> isValidUrl, two function calls
away, which its DOM-based-XSS sanitizer recognition does not trace.
Behavior is unchanged -- same scheme check, same SCRIPTABLE_SCHEMES list,
same allowedProtocols gate -- just inlined directly above the
previewLink.href assignment it guards, so the barrier is visible in the
same scope as the sink.
Added a regression test that runs the shipped onInput handler (not just
the extracted helpers) against a mocked DOM, so a future change that
reintroduces an unguarded previewLink.href assignment fails here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(security): address CodeRabbit findings on the CodeQL triage PR
- src/wifi_manager.py: reject non-ASCII WPA-PSK passphrases before any
credential-saving or connect flow runs. NetworkManager only accepts
printable ASCII passphrases (or a 64-char hex key); a non-ASCII value
was previously saved/attempted before nmcli itself rejected it.
- web_interface/blueprints/api_v3/config.py: fail closed when the
plugin config schema path can't be resolved under the plugins
directory (e.g. a symlinked plugin dir). Previously this fell
through with secret_fields left empty, so submitted credentials for
that plugin were saved as ordinary, unencrypted configuration.
- web_interface/static/v3/js/widgets/plugin-file-manager.js: stop
splicing the JSON day/column key into an inline oninput="..." handler
string. escHtml() escapes quotes for a normal HTML attribute, but the
browser HTML-decodes the attribute before running it as script, which
undoes that escaping and lets a crafted column name (e.g. from an
uploaded JSON file) break out of the JS string and execute. Cell
edits now travel through data-day/data-col attributes read by one
delegated 'input' listener instead.
While in this file: fixed 6 pre-existing missing-')' typos on
multi-line safeSetHTML(...) calls (already flagged by Biome in this
PR's own CodeRabbit run as syntax errors blocking its lint pass).
These predate this PR (present on main too) but made the whole file
fail to parse in any JS engine, which is a bigger problem than the
XSS finding itself and directly touches the same lines.
Added/extended regression tests for each fix; full suites pass
(pytest: 4580 passed, 62 skipped; JS: 84 assertions).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
ChuckandClaude Opus 5
|
5137e86d16 |
feat(tools): MQTT bridge and Pixlet editor, ported onto the api_v3 split (#554)
* feat(tools): manage the MQTT bridge and Pixlet editor from the Tools tab PR #544's change, ported onto the api_v3 package split (#553). Identical behaviour; only the placement of the new code differs. The original added 508 lines to web_interface/blueprints/api_v3.py, which #553 deletes, so every hunk of it would conflict irreconcilably. Ported by AST: 26 new top-level items sorted to where the split puts each kind -- __init__.py 2 imports, 11 constants, 7 helpers starlark.py 4 routes (/starlark/editor/{apps,status,start,stop}) misc.py 2 routes (/integrations/mqtt-bridge{,/config}) Everything outside api_v3.py -- the Tools partial, the installer scripts, the JS tests -- applied unchanged. Routes: 111 from the split plus these 6 = 117, and the url-map snapshot is regenerated to match, which is exactly what test_api_v3_url_map.py is designed to make you do when routes are added. Full Python suite: 4,278 passed, 68 skipped, 0 failed. The JS tests this PR ships could not be run here -- node is not installed on this machine -- so test/js/dom/test_tools_sections.js is unverified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * fix(starlark): don't crash the pixlet editor's start/stop routes, and honor an operator-set PIXLET_EDITOR_HOST The AST-based port of #544 onto the api_v3 package split dropped `time` from starlark.py's import list. start_pixlet_editor() and stop_pixlet_editor() both call time.time()/time.sleep() directly, so every start (NameError building `state['started_at']`) and every stop that has to wait out the EXIT trap crashed with a 500. No test caught it because the route's own tests mock subprocess.Popen but never actually invoked it before now. Also carries over #544's later fix that this port branched before: env['PIXLET_EDITOR_HOST'] = '0.0.0.0' unconditionally overrode an operator who had already pinned PIXLET_EDITOR_HOST to loopback, forcing the unauthenticated `pixlet serve` process onto the LAN regardless (CodeQL CWE-1188). Switched to env.setdefault(...), same as api_v3.starlark.py's siblings already do for _pkg-owned names. Both fixes route the shared _pkg.time reference the rest of the package's route modules already use for anything a test might need to patch, rather than a bare `import time` local to this file. Ported the existing regression test from #544 (TestPixletEditorHostDefaultsButDoesNotOverride) onto this branch's module layout (web_interface.blueprints.api_v3.starlark instead of the old monolithic api_v3 module), which is what caught the NameError. Full suite: 4330 passed, 62 skipped, 2 failed -- identical on this branch and on origin/main (missing tzdata package breaks two timezone-alias tests in test_onboarding_checklist.py, unrelated to this change). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(api-v3): clear the six lint errors this rebase introduced All six were introduced by rebasing this branch onto the merged blueprint split, not by the split itself. Confirmed by diffing pyflakes output against main with line numbers normalised -- everything else it reports is present on main too and is the package's deliberate re-export pattern. starlark.py used _STARLARK_APPS_DIR three times without importing it (F821). The rebase resolved an import-list conflict as a union of both sides, and that symbol was on neither side of the conflict hunk, so it was silently lost. It is defined in __init__.py and is now imported like its neighbours. This was the only one of the six that would fail at runtime rather than merely lint. __init__.py imported contextlib twice (F811): the cherry-pick added one next to the existing import. Removed the duplicate; the original at line 19 is used. __init__.py imported signal purely to re-export it to starlark.py, so pyflakes saw it as unused (F401). signal is stdlib and does not need routing through the blueprint package, so starlark.py imports it directly and __init__.py no longer does. contextlib stays re-exported because this module genuinely uses it. _read_mqtt_bridge_config()'s local `config` shadowed the `config` submodule this module imports at the bottom for its route side effects (F811). Renamed to `settings`, with a comment saying why, since the name is otherwise the obvious one to reach for. Verified: pyflakes now reports nothing on this branch that main does not, the package imports, all nine route modules load, and 117 routes register, matching the pinned URL-map snapshot. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(api-v3): reject MQTT bridge bodies the endpoint cannot apply Two CodeRabbit findings on the bridge settings endpoint, both of which returned 200 while doing something other than what the caller asked. `request.get_json(silent=True) or {}` turned a missing or unparseable body -- and the JSON literals null, [] and false -- into an empty dict, which then satisfied the isinstance(data, dict) guard on the very next line. The guard was there to reject exactly those bodies. Dropping the `or {}` lets None fail it. The same `or {}` on /errors/clear is left alone: its docstring documents the body as optional, so an absent body legitimately means "use the defaults". The difference is that saving settings has nothing sensible to do with no body. `if data.get('clear_password'):` accepted any truthy value, and the string "false" is truthy in Python -- so a client echoing the field back as a string wiped a password it meant to keep. Now coerced through the package's existing _coerce_to_bool, which already maps 'true'/'on'/'1'/'yes' and nothing else. test_mqtt_bridge_config_endpoint.py covers both: five unusable body shapes plus a missing body, and clear_password across truthy and falsy spellings. Verified against the unfixed code -- reverting the body guard fails 5, reverting the coercion fails 3. Not changed here: CodeRabbit also asks this endpoint to reject MQTT credentials when TLS is off (CWE-319). That is a policy decision about the feature rather than a defect -- unencrypted MQTT on a trusted LAN is common and often deliberate -- so it is raised on the PR for a maintainer call instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: work through the remaining review findings on the editor and bridge allow_insecure_mqtt (CWE-319, requested): a password with TLS disabled crosses the network in cleartext. Refused now rather than merely warned about -- but refused, not forbidden, because unencrypted MQTT on a trusted LAN is a normal deliberate setup. allow_insecure_mqtt is the explicit acknowledgement, defaults false, and is coerced like the other booleans so the string "false" cannot switch the guard off. starlark.py:796 -- the supported service runs Flask threaded, so two start requests could each see running=False, each launch an editor, and the second state write replace the first PID, orphaning a process that holds the display down with nothing recording it. The check-launch-write sequence now takes a module-level lock. starlark.py:848 -- if the state write failed the route returned success with an editor running and no PID recorded: status and stop both reported no session while the display stayed down until the timeout expired. It now terminates the process group and returns an error. starlark.py:890 -- SIGKILL gives the script's EXIT trap no chance to run, so nothing hands the display back, yet the response said "the display is restarting". After an escalation the display is now restarted explicitly, and a failure to do so returns an error naming the manual step instead of a success. pixlet_config_editor.sh:184 -- find_pixlet supports Darwin but macOS ships no timeout(1); GNU coreutils installs it as gtimeout. Resolved up front so the failure lands before the display is stopped rather than after. pixlet_config_editor.sh:154 -- wildcard, loopback and an explicit interface address are three cases, not two. Collapsing the last two printed a URL saying "localhost" whenever PIXLET_EDITOR_HOST named a LAN address. tools.html:1254 -- escHtml does not encode single quotes, and the app id was interpolated into an inline onclick="startPixletEditor('...')", so a directory containing an apostrophe could break out of the JS string and run script. The handler binds with addEventListener and reads the id from dataset, where it is only ever parsed as an HTML attribute. Tests: test_mqtt_bridge_config_endpoint.py grows to 23 cases covering the opt-in in both directions. The tools DOM suite gains three guards asserting the edit buttons carry no inline onclick and pass the id via dataset -- those need jsdom and did not run here, so CI verifies them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(api-v3): log the traceback on the editor state-write failure The 848 fix answers 500 when the session state cannot be written, and logged that at error level -- but without exc_info, so the traceback never reached the log. test_web_error_detail.py guards exactly this: a handler returning 5xx must write an error-level record *with* the traceback and return the sanitized detail, because checking that merely something was logged is too weak. Caught by Core unit tests on the previous commit, not locally: the guard parses every module under web_interface/blueprints/api_v3 as one source, so it only fires once the whole package is read together. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ant456
|
a3d505384d | Add render_width/render_height support to Starlark Apps (#552) | ||
|
ChuckandClaude Opus 5
|
bdb9a94033 |
refactor(api-v3): split the 10,469-line blueprint into a package (#553)
* refactor(api-v3): split the 10,469-line blueprint into a package web_interface/blueprints/api_v3.py held 111 routes, 56 helpers and 181 functions in one module -- 9% of the core by line count and three times the next largest file. It becomes a package of nine route modules grouped by path segment, plus __init__.py for the shared imports, constants, Blueprint and helpers. Every route module decorates the SAME api_v3 Blueprint object, so endpoint names stay api_v3.<function>, the URL map is unchanged and app.py is untouched. Verified: 111 routes before, 111 after, byte-identical rules, endpoints and methods, and every endpoint still on the one blueprint. plugins 3,867 config 1,178 starlark 692 system 619 fonts 452 misc 398 wifi 361 display 326 backup 212 __init__ 1,787 (imports, constants, Blueprint, 56 helpers) Two things the URL-map check could not catch, both found by running the suite: 1. PROJECT_ROOT = Path(__file__).parent.parent.parent. Moving the code one directory deeper made that resolve to web_interface/ instead of the project root. Nothing failed at import; it surfaced as ~110 tests failing with 404s and "installation script not found", because every path built from it was one level too shallow. Now parents[3], and test_api_v3_url_map.py asserts PROJECT_ROOT/run.py exists so the next move cannot repeat it. 2. Module-attribute patching. Tests do monkeypatch.setattr(api_v3_module, "_BACKUP_EXPORT_DIR", ...) and a route module that binds such a name by value never sees the patch. The shared code therefore stays in __init__.py rather than moving to a _common submodule -- it has to live on the module the tests patch -- and the eleven names tests patch are read back through the package (_pkg.X) instead of bound by value. Those eleven were found by AST-scanning every setattr in the test tree, not by guessing; "time" is among them, used to drive a fake clock through the second-resolution credential-backup filenames. Test changes are confined to what genuinely moved: patch targets that now name the owning route module, imports of helpers, and six tests that scan the api_v3 source as a file and now read the package directory. Full suite: 4,278 passed, 68 skipped, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * fix(api-v3): address CodeRabbit findings from the blueprint-split review Fixes to the api_v3 package split (PR #553), one per finding verified against the actual code: - __init__.py: _redact_credentials only blanked scalar values under a credential-named key; a bare list of secrets under such a key (e.g. tokens: ["a", "b"]) passed through untouched, since the list branch recursed with no memory that its key looked like a credential. Nested dicts still walk normally (a documented, tested behaviour -- a container like secrets: {api_key: ..., note: ...} is a section name, not a value to blank outright), but any value reached under a credential-shaped key is now actually blanked. - __init__.py: the OAuth helper script's raw stderr/stdout went to logger.error unredacted (CWE-532) right next to a comment claiming this was deliberate; the HTTP response already used the existing redact_text helper. Routed the log line through the same helper. - __init__.py / starlark.py: the standalone Starlark manifest fallback (used when the plugin instance isn't loaded) read-modified-wrote manifest.json with no lock, unlike StarlarkAppsPlugin._update_manifest_safe (plugin-repos/starlark-apps/manager.py), which already holds an flock for the same file when the plugin is loaded. Added _starlark_manifest_lock, mirroring that pattern, and wrapped every standalone read-modify-write call site in it. The app-config update route also wrote config.json and the manifest as two separate, non-transactional writes (a second, distinct finding at the same call site); config.json is now rolled back if the manifest write that follows it fails. - backup.py: restore options used bare bool() on values from the request, so {"restore_secrets": "false"} restored secrets anyway (bool("false") is True). Switched to the existing _coerce_to_bool helper already used for this exact purpose elsewhere in the package. - config.py: an automated import-rewrite mangled four user-facing validation strings and their neighbouring comments -- "Invalid start time" had become "Invalid start _pkg.time" (and likewise for "end time") in both the schedule and dim-schedule per-day validation paths. - display.py: `import _pkg.time as time_module` -- _pkg is a local alias for the package, not a real importable module, so this raised ModuleNotFoundError whenever a caller restarted an already-running display service via /display/on-demand/start, after the on-demand request was already written to cache. Fixed to `import time`. Audited the rest of the package for the same `_pkg.<module>` import mistake; every other `_pkg.` reference is a legitimate attribute read-through (`_pkg.time.time()`, `_pkg._get_starlark_plugin()`, ...), not a broken import statement. - fonts.py: validate_file_upload's max_size_mb parameter is silently unused by that helper (it only checks filename/extension) -- the font upload route saved arbitrarily large files as a result. Added the same seek-and-check pattern already used for the sibling .star upload. - wifi.py: two ad hoc, inconsistent bool coercions. POST /wifi/ap/auto-enable used bare bool(), so a JSON string "false" enabled it. POST /wifi/radio's enabled/force parsing recognized real bool and some strings but not int 1/0 (1 is True is False in Python). Factored one small _parse_bool_ish helper local to this file and used it at all three sites. Not changed: the "unknown/misspelled restore option keys default to True" half of the backup.py finding -- the file's own comment documents that a missing key deliberately means "restore everything," matching the already-existing JSON-parse-failure guard a few lines above it; only the bool-coercion defect was a real bug. Added or extended regression tests for every fix, following each area's existing test conventions. Full suite: 4328 passed, 62 skipped, 2 failed on both this branch and origin/main (missing tzdata package breaks two timezone-alias tests in test_onboarding_checklist.py, unrelated to this change) -- no new failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3bPMESe2TfrGvbs1ef9c5 * fix(api-v3): reject unknown restore option keys CodeRabbit's review of the blueprint split (#553) asked that POST /backup/restore reject option keys outside RestoreOptions' known set. The follow-up commit fixed the bool("false")-is-True bug with _coerce_to_bool but never added the key check: a typo'd or renamed key (e.g. "restoreSecrets") is silently ignored by opts_dict.get(key, True), so the flag stays at its True default and secrets get restored despite the caller's request saying otherwise -- with no indication anything was wrong. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vmcwf5vMgYqdt8bJTZtiwb * fix(api-v3): address CodeRabbit findings on the blueprint split - _redact_credentials: blank scalar descendants of objects reached through a credential-owned list (e.g. tokens: [{"value": "secret"}]) regardless of field name -- the existing name-based walk only protected direct dict values under a credential key, not list items. - wifi.py: reject enabled/force/auto_enable_ap_mode values _parse_bool_ish can't recognize (400) instead of silently treating them as False, which could disable Wi-Fi or the radio itself. - Starlark manifest locking: lock a stable manifest.json.lock sidecar instead of manifest.json itself, in both the standalone route path (_starlark_manifest_lock) and the plugin path (StarlarkAppsPlugin._save_manifest / _update_manifest_safe). manifest.json is replaced by an atomic rename on every write, which swaps in a fresh inode; a lock held on the old inode does not exclude a second locker that opens the path afresh right after the rename and gets the new inode, so two writers could race despite each holding "a lock". A sidecar that no write ever touches always resolves to the same inode for every locker. Skipped as stale: the "serialize the complete manifest read-modify-write" finding at api_v3/__init__.py -- every standalone handler that calls _write_starlark_manifest is already wrapped in _starlark_manifest_lock() on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(api-v3): re-check reconciliation findings by the reconciler's own rules Both CodeRabbit findings on the merge commit, verified against the code first. Major, plugins.py: the stale-findings filter derived its own notion of "in config" and "on disk", and both were looser than the reconciliation module's. set(load_config()) also contains system keys, the secrets-file keys load_config() merges in, and non-dict values; and any directory holding a manifest.json counted as installed even when that manifest does not parse. Either looseness clears a finding that is still true -- and a secrets key read as a plugin is the precise bug the filter exists to stop reporting, so reintroducing that asymmetry while re-checking was the wrong way round. The two extractions now live in state_reconciliation.py as config_plugin_ids() and disk_plugin_ids(), with ignored_config_keys() and secrets_top_level_keys() alongside. _get_config_state() and _get_disk_state() use them too, so there is one definition rather than two that can drift. _get_disk_state() re-reads each manifest for version/name after taking membership from the shared extractor; that costs one extra small read per plugin on a path that runs once per boot. Minor, the new test: the fixture assigned api_v3.config_manager and api_v3.plugin_manager directly. Those live on a module-level blueprint singleton, so the mocks leaked into every later test that imports api_v3 -- pointing at a tmp_path already deleted. Both now go through monkeypatch.setattr, which restores them. This is the same pollution class that made an earlier test in this session break seven unrelated ones, so it is worth getting right. Five cases added for the parity itself: a secrets key, a system key and a non-dict value must not clear an "installed but missing from config" finding, and neither an unparseable manifest nor a .standalone-backup- directory may count as installed. All five fail against the looser version. Linux CI on the preceding commit: Core unit tests, plugin harness, CodeQL and CodeRabbit all pass. Codacy reads action_required on every commit of this branch including the first, so it is pre-existing and not from this work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
0ab95586fb |
fix(web): say when a system action failed for want of passwordless sudo (#560)
* fix(web): say when a system action failed for want of passwordless sudo
POSTing reboot_system to a Pi returns, in full:
{"message": "Action failed; see logs for details", "status": "error"}
The cause is that the web interface runs unprivileged, and its
systemctl/reboot/journalctl calls only work once
scripts/install/configure_web_sudo.sh has granted NOPASSWD. first_time_install.sh
never invokes that script and no user-facing doc mentions it, so on a fresh
device every privileged action fails -- start_display, stop_display, the
autostart toggles, reboot, and the log viewer.
That last one closes the loop: "see logs for details" is unreachable advice
when journalctl is refused for the same reason. This is exactly the failure
src/web_interface/error_handler.py's describe_exception() was written to break,
and /system/action's exception handler was still discarding the cause instead
of using the helper the module already imports.
Two changes, no behaviour change when things work:
- The exception path now returns 'details': describe_exception(e), matching how
the other handlers in this blueprint already report.
- A failure whose stderr or exception text is sudo refusing to prompt ("a
password is required", "no tty present", "a terminal is required") reports
what to do about it, naming configure_web_sudo.sh. Unrelated failures keep
the generic message and their stderr, so a missing unit is not blamed on
sudo.
Granting the sudo rights is left alone deliberately: auto-running a script that
hands out NOPASSWD is a security decision for the maintainer, not something to
slip into an installer. Making the refusal legible is the part that is
unambiguously an improvement.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(web): apply the sudo hint on the on-demand start_display path too
start_display with a mode builds its own response and returns before the shared
nonzero-result path, so a recognized sudo refusal there reported only "Failed to
start display" and said nothing about the passwordless sudo that refused it --
the exact gap the rest of this PR closes everywhere else.
Raised by CodeRabbit on #560 and verified against the code before fixing: the
branch at api_v3.py:2058 does return early past the shared handler.
Three regression cases: the on-demand branch reports the sudo cause, keeps its
"Display started" message on success, and does not blame an unrelated failure on
sudo.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
ChuckandClaude Opus 5
|
39f27d285d |
fix(plugins): stop reconciliation inventing plugins and telling users to delete real config (#557)
On a device running four installed, configured, working plugins, the overview
banner read:
Stale plugin config entries found: football-scoreboard, odds-ticker, data,
ledmatrix-weather, starlark-apps. Remove them from config.json or reinstall
via the Plugin Store.
Every claim in that sentence was wrong, and following its advice would have
deleted 4.9KB of working league settings. Four separate defects combined.
1. Secrets keys became phantom plugins. load_config() merges
config_secrets.json into the config it returns, and the ignore list named
only 'github' and 'youtube'. A 'data' key in that file therefore read as a
plugin id and was reported as "in config but not on disk" forever. Read the
secrets file's own top-level keys instead of hardcoding two of them.
2. The auto-fix clobbered real config. The handler for "on disk but not in
config" assigned `config[plugin_id] = {'enabled': False}` unconditionally,
so whenever detection was wrong it replaced a plugin's entire configuration
with a stub. On the reported device it only failed to do so because the
write hit EACCES. Now it refuses to overwrite an entry that already exists.
3. The banner gave backwards advice. plugin_missing_in_config ("on disk, not in
config") and plugin_missing_on_disk ("in config, not on disk") are opposite
problems, and both were rendered as "stale config entries ... remove them
from config.json" -- which is correct for the second and destructive for the
first. They are now reported separately, each with the advice that fits.
4. A stale verdict was served indefinitely. The result is a snapshot written
once per run to a status file, and a run that fails to apply a fix also
declares it will not retry. A condition that had since resolved kept being
reported for hours. The status endpoint now re-checks stored findings
against current state, dropping only what it can prove stale and keeping
any kind it cannot re-verify.
The secrets-key lookup is deliberately fail-safe: an unreadable, absent,
malformed or non-path secrets location narrows the ignore set rather than
raising. An earlier revision let TypeError escape, which the broad handler in
_get_config_state() swallowed as "Error reading config state" -- emptying the
config state and making every downstream detection wrong. The existing
reconciliation tests caught it; there is now a regression test for it too.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
ChuckandClaude Opus 5
|
aba96e25b3 |
chore: delete three functions nothing calls (#550)
src/base_classes/baseball.py _get_baseball_display_text 45 lines src/web_interface/api_helpers.py validate_request_params 22 web_interface/blueprints/api_v3.py _validate_time_range 14 Each has exactly one occurrence across both repositories -- its own definition. No decorator, no __all__, no getattr dispatch, nothing in templates or JavaScript. A fourth candidate was dropped after checking: _unshare_element_fonts in src/common/sports_shared.py looked unreferenced, but eight scoreboard plugins call SportsCore._unshare_element_fonts directly from their test_element_text_colors.py, plus their own copies at runtime. It is live API. The earlier reading came from a plugins checkout 84 commits behind main, which is a good argument for re-verifying this kind of claim against a fresh tree rather than trusting an earlier scan. Full suite: 4,265 passed, 68 skipped. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
577f5501a6 |
perf(plugins): stop re-deriving a display() signature the caller already cached (#549)
display_controller resolves once, and caches, whether a plugin's display() takes a display_mode keyword -- self._plugin_accepts_display_mode, populated right before the dispatch. It then handed the executor a types.SimpleNamespace wrapping a closure, and execute_display() ran inspect.signature() on that to work out the same thing. Because the SimpleNamespace is rebuilt per call, the callable was new every time, so nothing inside the executor could ever cache it either. Measured at ~39us per dispatch on a Pi 4, for a value the caller had a line earlier. execute_display() now takes accepts_display_mode, falling back to inspecting only when a caller does not pass it, so existing callers are unaffected. Also documents two things that read as bugs and are not: - execute_with_timeout()'s timeout is advisory. Nothing cancels the thread -- Python cannot -- so on expiry the operation runs to completion in the background and only the caller gives up. A permanently hung plugin leaks a daemon thread per attempt. This is why callers holding a lock across the call must release it from inside the wrapped callable, as run()'s _release_display_lock already does. - Only the first display() of each mode goes through the executor; the per-frame loops call display() directly. That is deliberate: a thread per frame would cost more than an advisory timeout buys. Both loops now say so, so the asymmetry does not read as an oversight. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
dcd6e39c96 |
fix(web): report real disk usage and MemAvailable on the live status stream (#558)
The SSE status stream sent 'disk_used_percent': 0 as a literal, so every
consumer of the live view showed 0% disk no matter how full the card was.
/api/v3/system/status computed it correctly; the stream that the dashboard
actually watches did not. On a Pi with a modest SD card that is the warning a
user most needs, and it was guaranteed to never appear.
The stream also omitted memory_available_mb. /api/v3/system/status carries it
with a comment spelling out why it matters: MemAvailable accounts for
reclaimable page cache, so it is what separates a board reading 70% "used" that
is fine from one reading 70% that is about to fail fork(). A 1GB Pi 3B+ can sit
at either. The number that predicts the failure was missing from the live view.
An unreadable disk now reports None rather than 0. The UI already renders null
as '--'; a confident 0 reads as "plenty of room", which is worse than a blank.
Metric collection moves to web_interface/system_metrics.py, with no Flask or app
imports. That is not cosmetic: importing web_interface.app constructs the Flask
application and a CacheManager, and the latter claims the cache directory with a
cleanup thread. The first version of these tests imported the generator directly
and broke test_cache_cleanup_thread_ownership ("one thread per directory") plus
four starlark route tests through that side effect. Reading a CPU percentage
should not boot a web application, and testing it should not either.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
ChuckandClaude Opus 5
|
ad5bc4b819 |
perf(sports): LRU-bound the decoded logo cache (#559)
SportsCore._logo_cache was a plain dict keyed by team abbreviation with no eviction. Its entries are not file bytes but decoded RGBA thumbnails sized to display*1.5 -- roughly 36KB on a 256x64 panel, more for wide wordmarks -- and assets/sports/ncaa_logos ships 307 of them. A plugin that walked a full league held the whole league resident: about 11-18MB per manager instance, and a league runs three (live/recent/upcoming) that each keep their own cache, so the same logos were duplicated across them. On the 1GB Pi 3B+ this was measured on, one board was sitting at 439MB resident with ~290MB available, so tens of megabytes of duplicated league logos is real money. Bounded to 64 entries, which holds a full "other games" cycle (on the order of 20 games, 40 teams) without thrashing while capping the cache well below a 307-team league. Eviction is LRU rather than clear-when-full, using the OrderedDict/popitem pattern the neighbouring caches in this codebase already use (_IMAGE_CACHE_MAX, _FIT_CACHE_MAX, _TEXT_WIDTH_CACHE_MAX). That ordering matters: the logos on screen right now are precisely the ones that must not be discarded, so a cache hit moves the entry to the end. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Sonnet 5
|
fb3b293ace |
fix(plugins): let a plugin ask to be polled faster while it has live content (#555)
* fix(plugins): let a plugin ask to be polled faster while it has live content
Reported: "the football plugin with live games only updates the live game in
progress if I restart the display."
The data path was never the problem. NFLLiveManager fetches ESPN with no cache,
SportsLive.update() refreshes current_game in place when the game IDs are
unchanged, and the scorebug redraws from the game dict every frame -- which is
why the reporter's logs look healthy.
The problem is cadence. _get_plugin_update_interval() read only the manifest's
static update_interval, football's manifest pins that to 60, and the plugin's
own live_update_interval (15s) was invisible to the scheduler. Measured on a rig
during the fourth quarter of the game in the report:
23:21:49 23:22:50 23:23:50 23:24:50 23:25:50 <- exactly 60s apart
A clock and score up to a minute stale during a two-minute drill reads as a
frozen panel, and a restart is the one moment it is ever current.
A single static number cannot say "every 15 seconds while a game is on, every 15
minutes in July", and only the plugin knows which is true. get_update_interval()
lets it say so per tick; returning None means "no opinion" and the existing
manifest/config resolution applies, so every plugin that predates this is
unaffected.
Requests are clamped to MIN_DYNAMIC_UPDATE_INTERVAL (5s): a plugin returning 0
would otherwise be re-entered on every tick of the render loop, busy-waiting
against its own API. A hook that raises or returns a non-number is ignored
rather than propagated -- a scheduler that fails on one plugin's bug stops
updating all the others.
Deliberately NOT changed: the manifest still beats config in the static path.
That looked like the obvious fix -- user config being silently ignored -- until
checking a real rig, where football and baseball both carry update_interval 3600
in config against a manifest 60, and weather 1800 against 60. Those values are
stale precisely because nothing has been honouring them; making config win would
have slowed three plugins by 60x, turning a one-minute lag into an hour. The
dynamic hook makes the flip unnecessary. There is a test pinning the current
precedence with that reasoning attached.
Full suite: 4,283 passed, 68 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9
* test(plugins): drive the real scheduler, not just the interval resolver
test_plugin_dynamic_update_interval.py asserts that
_get_plugin_update_interval() returns the number the plugin asked for. That is
not the same claim as "the plugin gets updated more often", and the gap between
those two is exactly where the original bug lived: the plugin knew it wanted
15s, said so in live_update_interval, and nothing downstream acted on it.
So this ticks the real run_scheduled_updates() through a simulated hour and
counts dispatches. Against pre-fix core it reports "10 updates in 10 minutes of
a live game" -- the 60s manifest cadence, matching what was measured on a rig
during the reported game. Against the fix it reports ~40.
Also pins the regression that would be worse than the bug: an idle hour must
still be ~60 updates, not 240. Asking for the live interval year-round would
poll ESPN four times a minute all summer.
Scope note, since it is easy to over-read this fix: the *switch* display path
already refreshed the manager immediately before drawing, via
_try_manager_display() -> _ensure_manager_updated(), which honours the manager's
own 15s interval. So a switch-mode card was already <=15s stale at draw time
before this change. What this fixes is the background cadence, which is what
live-priority detection, Vegas content and scroll preparation all read.
Full suite: 4,288 passed, 68 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9
* fix(plugins): reject bool and -inf hook results in dynamic interval
get_update_interval() ran bool through float() (bool is an int subclass,
so True/False became 1.0/0.0) and only checked for +inf, not -inf. Both
cases landed on the MIN_DYNAMIC_UPDATE_INTERVAL floor by coincidence
instead of falling back to the static/manifest interval as invalid
input should.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017Tst9cied2ri9bH4QRWa6H
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
ChuckandClaude Opus 5
|
8da13f02f8 |
chore: ignore team logos fetched at runtime (#551)
logo_downloader.py and LogoHelper write into assets/sports/<league>_logos/ whenever a plugin meets a team whose logo is not on disk. Those directories are also tracked -- 209 NCAA logos and 153 soccer ones ship with the repo -- so every rig accumulates untracked files nobody intended to commit. This checkout had 62; hdpi shows the same. The cost is not the files, it is that a permanently dirty `git status` trains everyone to ignore the one signal that says a checkout is not what you think it is. That is how a stale tree sat unnoticed on a rig for hours until a restart surfaced four sports plugins that could no longer import. Ignoring a directory does not untrack what is already in it, so the logos that ship keep shipping -- verified: 209 and 153 still tracked, no deletions in the diff. Only new downloads are hidden. Adding a logo on purpose stays possible and is what the escape hatch in the comment documents. It is also rare: the last deliberate addition was #415, four named NCAA logos a plugin needed, and `git log` finds no other in a year. So the common case is noise and the rare case is explicit, which is the right way round. Untracked files: 62 -> 0. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
28bc79566f |
fix(logo): remember a missing logo instead of re-warning every rotation (#548)
* fix(logo): remember a missing logo instead of re-warning every rotation load_logo() stat'd the path and logged a WARNING on every call, and the positive cache never covered it because a miss returns None and caches nothing. A file that is simply not there therefore produced one warning per rotation for as long as the process ran -- measured on a live rig at 114 lines in 24 hours for a single missing ticker icon, for a file nobody was going to add. Misses are now remembered for 10 minutes: warn once, then return None without touching the disk. Bounded rather than permanent because logo_downloader writes logos at runtime, so a file that appears later must still be picked up without a restart. Downloads through load_logo_with_download() clear the entry outright -- load_logo() consults the miss record before it stats the disk, so without that a freshly downloaded logo would stay invisible for the whole window. This is in the core rather than in ledmatrix-stocks, where it was found, so every plugin that goes through LogoHelper gets it. _cache_order stays a list. Swapping the pair for an OrderedDict would shave an O(n) scan per cache hit, but n is capped at cache_size (100 by default) and test_logo_helper.py pins the current structure; not worth the churn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * fix(logo): make the miss TTL longer than the rotation it is meant to outlast Deployed the previous commit to a live rig and measured it: no change at all. "Logo not found for VOO" stayed at ~6 lines an hour, exactly the baseline. The TTL was 600s and the display rotation is ~618s, so every recheck expired just as the plugin came round again and the negative cache never once got to suppress a warning. The fix was correct in shape and useless in practice, which only measuring on the rig would show. An hour instead. That is safe because the TTL is not the main way an entry clears: load_logo_with_download() drops it the moment a download succeeds and clear_cache() drops all of them. The TTL only covers a file that appeared some other way -- someone copying one in by hand -- and waiting up to an hour for that, or restarting, is a fair trade for not re-warning about a file nobody is going to add. The general lesson is in the comment: a TTL has to be long relative to the loop that does the asking, not merely "a while". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
12f3790994 |
fix(install): render the systemd units from their templates, not from heredocs (#547)
* fix(install): render the systemd units from their templates, not from heredocs The installers carried their own inline copies of units that also exist as templates under systemd/, and the copies drifted. install_service.sh renders ledmatrix.service from the template correctly, then wrote ledmatrix-web.service from a heredoc that predated it -- missing Wants=network-online.target, RestartSec=10, SyslogIdentifier, CacheDirectory, CacheDirectoryMode and Environment=USE_THREADING=1. install_web_service.sh had a third copy, and install_wifi_monitor.sh a fourth, that one already differing from its template (syslog where the template says journal). startup_validator.py compares the installed unit against the template, so a rig installed this way warned on every boot -- and the remedy the warning names, "re-run scripts/install/install_service.sh", reinstalled the same stale copy. The warning could never clear. Reproduced on a live rig running exactly that unit. All three installers now render systemd/*.service through the same placeholder substitution. The template gains a __USER__ placeholder rather than hardcoding User=root, because the web interface runs as whoever installed it. That last point was a second, independent cause of a permanent warning: the validator substituted a fixed "root", so any non-root install reported drift forever. It now reads User= from the installed unit -- an install-time decision, not something the template dictates -- and compares everything else strictly. first_time_install.sh already reads the installed User= the same way. Tests cover a non-root web unit not warning, a genuinely changed directive in that unit still warning, the User= fallback, and a grep-based guard that no installer under scripts/install/ contains an inline unit body. That guard is what found the install_wifi_monitor.sh copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * fix(install): escape sed replacements, use mktemp, and make render failures fatal Address CodeRabbit findings on install_service.sh, install_web_service.sh and install_wifi_monitor.sh: - Values interpolated into each script's sed expression (project root path, username) were not escaped, so a value containing &, \ or the | delimiter would corrupt the rendered systemd unit. Add a shared sed_escape_replacement() helper in the new scripts/install/lib_systemd_render.sh (sourced by all three scripts) and apply it to every sed replacement. - install_service.sh rendered the main and web units to the predictable path /tmp/ledmatrix.service.tmp before installing them -- a symlink/TOCTOU race (CWE-377). Use mktemp for both, with a trap to clean up on exit. - install_service.sh treated a missing template as a mere warning and then checked only whether a unit already existed at the destination before enabling/starting it, so a render failure could silently fall back to enabling a stale, previously-installed unit. Both unit blocks now exit non-zero on a missing template or a failed render. Also rename the ambiguous loop variable `l` to `line` in test/test_systemd_unit_drift.py (Ruff E741); ruff isn't wired into any CI workflow in this repo today, so this isn't currently CI-blocking, but the rename is trivial and correct regardless. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3bPMESe2TfrGvbs1ef9c5 * test(install): cover sed_escape_replacement against sed-special characters CodeRabbit asked for regression coverage using a project path containing an ampersand; the earlier commits on this branch already fixed the escaping, mktemp usage, and enable/start-on-fatal-render-failure findings, and the l->line rename was already applied -- this closes the one remaining gap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
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>
|
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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 «/», 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> |
||
|
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> |
||
|
ChuckandClaude Opus 5
|
968b953a51 |
fix(display): pin one text layout engine, and give the 5x7 BDF face a size (#539)
* fix(display): pin one text layout engine, and give the 5x7 face a size Two ways a font could render differently on two machines running the same code, both found while diagnosing four plugins whose golden images passed on the machine that generated them and failed everywhere else. **Layout engine.** `ImageFont.truetype` picks its engine at load time: Raqm where the host Pillow was built with libraqm, Basic otherwise. The two round fractional glyph advances differently. `PressStart2P-Regular.ttf` at 8px has whole-pixel advances, so they agree — which is why most of the fleet matched everywhere and hid this. `4x6-font.ttf` at 6px does not: glyph positions drift cumulatively along a run, and the four plugins that draw body text in it (geochron, of-the-day, christmas-countdown, ledmatrix-weather's almanac) are exactly the four whose goldens travelled badly. Every core font load now goes through `src/common/font_layout.load_truetype`, which pins the Basic engine, so a render depends on the font file and the size and nothing else. Basic gives up complex-script shaping and kerning pairs; neither applies to bitmap-grid faces on an LED panel. Output is unchanged on a host without libraqm. **Zero font height.** `DisplayManager` built the 5x7 BDF face with `freetype.Face(path)` and never called `set_char_size`, so `face.size.height` stayed 0 and `get_font_height()` returned 0 for it — callers stacking rows by `prev_y + prev_height + gap` drew two lines on top of each other. The start-up line `Calendar font size: 0 pixels` has been printing the symptom all along. `font_manager._load_bdf_font` already called `set_char_size`, so whether measurement worked depended on which path loaded the face. `DisplayManager` now sets it too, and `get_font_height()` falls back to the strike the file declares rather than returning a zero line height. Fixes ChuckBuilds/ledmatrix-plugins#397 Refs ChuckBuilds/ledmatrix-plugins#371, #375, #378, #391 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(display): give the startup banner a rung that fits a full address at 64px CI caught what pinning the layout engine exposed rather than caused. `_fitting_font` walks PressStart2P then 4x6 at 6px, and "255.255.255.255" -- the widest thing the startup banner ever shows -- measures 66px at 4x6/6px against the 62 a 64x32 panel has to give. It used to squeak in only because the measurement depended on which layout engine the host Pillow happened to have; with the engine pinned it does not, so the rung the worst case actually needs is now in the ladder instead of implied: 4x6 at 5px, which measures 51. The fallback was wrong in the same place. When nothing in the ladder fit, it returned `self.font` -- the *widest* option, and precisely how "Initializing" came to run off the side of a 64px panel to begin with. It returns the narrowest face that loaded now. test/test_initializing_screen.py: 34 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(display): name the exceptions the BDF strike read can raise Codacy flagged the try/except/pass. It was already narrow in intent -- a malformed strike table on the measurement path must degrade to "size unknown" rather than take the display down -- but a bare `except Exception: pass` says neither of those things and hides a genuinely broken font behind a silent 8px fallback. It now catches what reading `available_sizes` can actually raise and logs which face failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore: drop logo PNGs the render harness downloaded into the worktree These are fetched at runtime by the logo cache; they are not source, and they rode in on a `git add -A` while I was running check_plugin.py against this branch. Nothing in the change needs them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
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> |
||
|
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> |
||
|
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. |
||
|
ChuckandClaude Opus 5
|
c9289e3a1d |
fix(store): update_plugin silently did nothing for four installed plugins (#536)
install_plugin() deliberately renames a plugin's directory to the MANIFEST id
when it differs from the REGISTRY id, so registry `stocks` lands in
`ledmatrix-stocks/`. Every lookup in _find_plugin_path() is by directory name,
so update_plugin("stocks") found nothing, logged "Plugin not installed", and
returned False.
Nothing surfaced that to the user. Clicking update in the web UI was a no-op
with no error, and the plugin stayed on a stale version indefinitely. Four
installed plugins hit this on a real device -- leaderboard, music, stocks and
weather -- found because a scripted update of eleven plugins failed on exactly
those four.
Adds a manifest-id scan as the LAST step of the resolution chain, so the two
documented lookups above it (configured dir, then the sibling plugins/
fallback) keep their exact meaning and ordering. That ordering is pinned by
test_discovery_path_contract.py, which characterises the divergence between
the three resolvers on purpose; this extends the chain rather than reordering
it. Directories renamed aside with '.standalone-backup-' during an install or
rollback are skipped, since matching one would report a half-finished install
as a live plugin.
Also adds scripts/audit_render_path.py, which walks the call graph from
display() and reports blocking calls reachable from it. display() runs on the
render thread, so anything slow there stalls the panel; on a vsync-paced loop
a single 15ms call drops a frame and a network round trip freezes the marquee.
Two instances were already found the slow way, by reading frame-time
histograms -- odds-ticker reading the scoreboard cache per frame, and
soccer-scoreboard timing out inside update(). The audit finds that shape in
the source instead. It is a heuristic and says so: a hit behind an interval
check may be fine.
It currently flags 23 calls across six plugins. The clearest is
ledmatrix-music, whose display() falls back to an inline
requests.get(timeout=5) when album art has not been prefetched -- a deliberate
"show the art rather than go blank" tradeoff by its author, but up to five
seconds of frozen panel. Reported, not changed; that is its owner's call.
185 store tests pass. Three of the six new tests fail without the fix.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
ChuckandClaude Opus 5
|
d12323e7f1 |
perf(scroll): pace frames to the panel — 44→100 fps, stalls 14% → 0.02% (#523)
* perf(scroll): pace frames to the panel, not to a fixed sleep Scrolling ran at 44-46 fps on a 2x128x64 chain and 14-17% of frames took 41-53ms, which reads as judder. Four independent causes, each measured on the hardware; details and the diagnostic recipe are in docs/SCROLL_PERFORMANCE.md. The high-FPS loop slept a flat 8ms after every render. display() has already blocked on the panel's vsync by then, so that sleep was added to a wait that had happened: ~4ms of render plus 8ms put each iteration at ~12ms against a 10ms refresh grid, so every swap missed a refresh and the loop settled at 50fps while asking for 125 -- with no headroom, so a further 14% of frames slipped again. It now sleeps only the remainder, with a 1ms floor so plugin threads still get the GIL. ScrollHelper stepped position on a wall clock at 1/scroll_delay steps per second. Plugins set scroll_delay to the frame period, so that comparison sat exactly on its own threshold: a frame arriving a hair early moved zero pixels and rendered an identical frame, dirty-tracking skipped the swap, it returned in ~2ms, and the beat repeated. No scroll_delay value tunes that out -- a shorter delay trades stalled frames for periodic double-steps. Both modes now accumulate elapsed time at the same configured speed, so position stays proportional to real time. Sub-pixel blending goes back to off by default. It renders a half-step by mixing two adjacent columns, which on a coarse panel showing pixel-font text alternates crisp and smeared frames and reads as shimmer -- visibly worse than integer stepping on the hardware. Vegas mode still opts in. disk_cache uses orjson when importable, falling back to the stdlib. Encoding a ~1MB record drops from 14.8ms to 5.4ms end-to-end, and that work holds the GIL while a marquee is on screen. display_manager also checksummed the whole framebuffer twice per frame (dirty tracking, then the preview snapshot); the snapshot now takes the checksum the caller already computed. New src/common/scroll_config.py resolves scroll settings in one place. Five ticker plugins each hand-rolled this and disagreed: odds-ticker ranked the deprecated scroll_pixels_per_second above the documented scroll_speed/delay pair, and because that key carries a schema default the documented settings were dead for every user (ChuckBuilds/ledmatrix-plugins#408), while ledmatrix-leaderboard read the same key only as a fallback. The resolver also warns when a speed will not advance a whole number of pixels per refresh, which is the property that actually determines whether a scroll looks smooth. scripts/build_rgbmatrix_nogil.sh rebuilds the rgbmatrix binding so it releases the GIL. Upstream declares SwapOnVSync without nogil, unlike SetPixel/Clear/Fill beside it, so the render thread held the GIL for the whole vsync wait and starved background threads into long uninterruptible bursts. The script patches, builds and self-verifies into a scratch tree; --install backs up the original and rolls back if the service does not come back healthy. Measured after: 100 fps locked, no stalls observed, render thread down from 51% to 19% of one core. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(display): keep the panel swap locked to vsync while scrolling Dirty tracking skipped SwapOnVSync for byte-identical frames. That is the right call for static content, but SwapOnVSync is also what paces the render loop, so skipping it skips the wait for the panel: a duplicate frame returns in ~8ms instead of ~10ms on a 100Hz panel, advances the strip only 0.8px instead of 1.0px, and so makes the next frame more likely to repeat as well. The effect sustains itself once it starts. Measured over 20 minutes on a 2x128x64 chain, both scrollers configured identically at 100 px/s: leaderboard 10ms x35, 11ms x3 (clean) odds-ticker 10ms x26, 8ms x7, 15ms x5 (~20% duplicates mid-scroll) The duplicates were not end-of-cycle idling -- 38% of fast frames fell within 90s of a scroll completion against 35% of normal frames, a null result. The trigger is per-frame work: odds does more of it, and more variably, so it is first to land a frame that advances less than a whole pixel. Pushing an identical frame costs one canvas copy. Falling out of vsync lock costs smooth motion. Static content is untouched, because is_currently_scrolling() expires on its own inactivity threshold -- covered by test_stale_scrolling_state_stops_forcing_pushes so a plugin that stops scrolling without saying so cannot pin the panel into always-push. Also de-flakes test_snapshot_still_written_on_skip, which asserted a strict mtime increase between two writes that can land in the same filesystem tick; it failed about two runs in three on Windows regardless of the code under test. The file is now backdated before the check. 156 tests pass on the Pi. Not yet confirmed by eye on the panel. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scroll): report the frame-time tail, and stop the row-major blit Two problems, both found by looking at the panel rather than the metric. The frame-stats line reported ONE instantaneous frame every 5 seconds -- about 1 frame in 500 -- printed beside a 100-frame average. Both hide exactly the fault they are used to chase: a 2ms duplicate and a 21ms double-wait average to precisely 10ms, so a ticker stalling on half its frames still reports a healthy "Avg FPS: 100.0". That reading cost several rounds of chasing the wrong layer. The line now aggregates every frame since the last log and reports median, p95, max, min, and explicit stall and skip rates (past 1.5x the median missed a refresh; under half never reached the panel, because dirty tracking skipped the swap so the frame never waited on vsync). On the hardware this now reads: leaderboard 100.0 fps over 501 frames | median 10.00ms p95 10.05ms max 10.34ms | stalls 0 (0.0%) skips 0 (0.0%) The binding rebuild's blit patch becomes opt-in (RGB_PATCH_BLIT=1, default off). Reordering that loop to row-major changes what a torn frame looks like: column-major tearing shows as a vertical seam, row-major as a horizontal split between the panel's upper and lower halves. On a 1/32 scan panel that reads as a one-pixel fold across the middle of every panel, which is what was reported on hardware and what went away when the blit was reverted. All of the measured gain comes from the SwapOnVSync change, so the risky half is simply not worth taking; the header says so. Also fixes --install resolving its paths against $HOME, which is /root under sudo, so it looked in /root/rgbmatrix-nogil-build and died with "no built module found" on a machine where the build had just succeeded. It now resolves SUDO_USER's home. Both build paths are verified on the Pi: default yields one GIL-release site, RGB_PATCH_BLIT=1 yields two. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(scroll): let users pick a crisp speed for their own panel Whole-pixel motion was previously only available at multiples of the refresh rate -- 100, 200, 300 px/s on a 100Hz panel. 100 px/s crosses a 256px panel in 2.6s, which is brisk for reading, and everything slower had to blend (blur) or repeat frames unevenly (judder). There was no way to ask for 50 px/s and get clean motion. SwapOnVSync takes a framerate_fraction the display manager never passed. It holds each frame for N panel refreshes; the panel keeps refreshing at its full rate throughout, so holding costs nothing in flicker and only changes how often a NEW image is presented. That turns 50 px/s into one whole pixel every second refresh instead of half a pixel every refresh. The crisp speeds are therefore refresh_hz / hold * pixels_per_frame, and that ladder depends on the panel: a Pi Zero on a long chain has a different set of good speeds from a Pi 4 on a short one. crisp_ladder() enumerates them and solve_crisp() picks the best match for a requested speed. solve_crisp weights motion quality rather than picking the numerically nearest entry, which matters more than it sounds. Asked for 30 px/s, nearest-by-value answers 28.6 -- 2px jumps at 14fps -- over 33.3, which is single-pixel motion at 33fps and obviously better on the panel. The target is also clamped into the ladder's range first, because relative error saturates near 1.0 for a target far outside it and the quality penalty would otherwise answer "10000 px/s" with the slowest entry. configure() snaps to the ladder and applies the hold when given a display manager. Without one the hold silently cannot happen and motion falls back to fractional pixels, so it warns rather than failing quietly. set_frame_hold() resets to 1 when scrolling stops, so one plugin's pacing cannot leak into whatever is on screen next. scripts/scroll_speeds.py is the user-facing part: it prints the ladder for the configured rate, measures what the panel ACTUALLY manages (--measure, for hardware that cannot reach its configured limit), highlights the nearest option to a wanted speed, and demos one live. It never starts or stops the display service itself -- doing that inside a script stranded the panel twice today. Speeds below ~20 px/s remain stepped regardless. That is the pixel pitch, not a software limit. Also fixes the dirty-tracking test spy, which stubbed SwapOnVSync with a single-argument function and would have masked the new call as a failed push, and rewrites a configure() test that had started passing for the wrong reason: it asserted a judder warning, which snapping now prevents, and was matching the unrelated "hold could not be applied" warning instead. 183 tests pass on the Pi. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scroll): tie the frame hold to the scroll, not the plugin The hold applied in configure() never reached the panel. Plugins share one display manager, and set_scrolling_state(False) -- fired whenever ANY other plugin finishes its scroll -- reset the hold to 1. A hold set once at plugin construction was therefore always gone by the time that plugin rendered. The symptom was a log line that lied. ledmatrix-stocks reported Scroll configured: 50.0 px/s (1px every 2 refreshes = 50.0 fps, smooth) while the panel measured 100.0 fps, median 10.00ms. Config, resolution and snapping were all correct; only the pacing silently was not applied. set_scrolling_state(is_scrolling, frame_hold=1) now carries it, so the hold lives exactly as long as the scroll that asked for it. configure() reports the value as ScrollSettings.frame_hold instead of applying it -- applying it behind the caller's back could never have been right on a shared display manager. Existing callers are unaffected; the default keeps one frame per refresh. Verified on hardware: stocks at 50 px/s now measures 50.0 fps over 251 frames | median 20.00ms p95 20.09ms | stalls 0 skips 0 20.00ms being exactly two refreshes, with the panel still refreshing at 100Hz underneath so flicker is unchanged. test_another_plugin_stopping_does_not_strand_a_hold pins the interaction that broke this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scroll,cache): resolve CodeRabbit review on #523 Eight findings, all reproduced before fixing. scroll_config.configure() read the refresh rate *after* resolve() had already used it. resolve() fills in target_fps, pixels_per_frame and the judder warning from that rate, so on a 60Hz panel every one of them described 100Hz -- and with snap_to_crisp=False nothing downstream corrected it, so set_target_fps() paced the helper to 100 FPS. The rate is now settled first, and falls back to the global config rather than straight to the default. refresh_hz_from_config() used `(cfg.get("display") or {}).get(...)`, which raises AttributeError when either level is truthy but not a mapping -- out of a function whose whole contract is a rate or a default. The frame-stats line reported the upper-middle sample as the median and the 96th sorted sample as p95 of 100. Both are also thresholds (stalls at 1.5x the median, skips at 0.5x), so the counts were biased too. The arithmetic is now in frame_stats()/format_frame_stats(), testable without a clock. configure()'s docstring and docs/SCROLL_PERFORMANCE.md still said it applies the frame hold and warns when it cannot. It deliberately does neither since "tie the frame hold to the scroll, not the plugin"; a caller following the old text would omit set_scrolling_state() and slow snapped speeds would still present every refresh. disk_cache had no policy for non-finite floats: orjson writes null, the stdlib writes NaN/Infinity, and orjson then rejects those legacy files so DiskCache.get deleted them as corrupt. One behaviour on both paths now -- write null, keep legacy records readable. allow_nan=False detects the values; the replacement walk runs only when there is one, so the ordinary write path is byte-identical and pays nothing. build_rgbmatrix_nogil.sh picked the build artifact with a glob piped to `head -1`, which sorts cpython-311 ahead of cpython-313, so a stale .so staged in from the source tree was installed as core.so while the GIL check -- which reads the generated core.cpp, not the .so -- still passed. It now requires the current interpreter's exact ABI name and fails closed. Its systemctl calls were also unchecked under `set -uo pipefail`: a failed stop left the old service running, the following start succeeded as a no-op, and the health check reported SUCCESS for a binding that was never loaded. orjson floor raised to 3.11.6 for CVE-2025-67221 (unbounded recursion in dumps); it covers the project's Python 3.10-3.13 range. Adds test/test_cache_nonfinite_floats.py (14) plus regression tests in test_scroll_config.py and test_scroll_helper.py. 9 of the cache tests and 9 of the scroll_config tests fail against the pre-fix code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * test(harness): keep the visual double's signature tied to production Moves set_scrolling_state's frame_hold into the test double here, where DisplayManager gains it, rather than in #534 where it arrived a PR early. CodeRabbit flagged the #534 version correctly: a double that accepts an argument production does not lets the call pass every harness run and raise TypeError on the panel, which is the one failure a safety harness exists to prevent. The drift has now gone both ways across two branches -- double behind production on this branch, double ahead of it on #534 -- so it is pinned instead of remembered. test_display_double_parity.py compares the two signatures and fails with the direction of the drift named. It reads the files with ast rather than importing them, because display_manager imports rgbmatrix at module scope and this check should hold on a laptop and in CI as well as on a Pi. Plugins begin passing frame_hold in ledmatrix-plugins#462, which is why production and the double both need it before that lands. Full suite: 3889 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
ChuckandClaude Opus 5
|
a0d3e64099 |
fix: ten defects found validating the whole plugin fleet on hardware (#534)
* fix(core): register tom_thumb, accept frame_hold in the test double, wire api_v3's managers Three independent fixes found while validating every plugin on a 256x64 rig. FontManager never registered tom_thumb even though assets/fonts/tom-thumb.bdf ships with the core, so every plugin offering it logged "Font family 'tom_thumb' not found" (16 warnings per countdown render) and had to carry a private loader to use a bundled font. Closes #524. VisualTestDisplayManager.set_scrolling_state() lacked the frame_hold parameter that DisplayManager gained, so any plugin passing it died with TypeError at render time and failed every size. Nine plugins now make that call; ledmatrix-stocks and ledmatrix-leaderboard were failing outright and the other seven only passed because their scroll path was unreachable without data. Closes #525. api_v3 declared module-level config_manager/plugin_manager = None that nothing ever assigned -- app.py sets the blueprint attributes, which the other 150+ call sites use. Three sites read the decoys, so /health reported the config unreadable and the plugin system uninitialised (making "degraded" permanent and unreachable-by-design) and /display/current fell back to a hardcoded 128x64 on every rig. The decoys are removed rather than assigned, so a bare name is now a NameError at test time instead of a silent None. The same function's first-call uptime was computed from two separate clock reads and came out negative. Closes #529. Verified on the rig: both previously-failing plugins render, the tom_thumb warnings are gone, /health reports "healthy" with all three checks passing, and /display/current reports the real 256x64. * fix(core): unique snapshot temp name, honour on-demand requests, skip empty starlark The preview snapshot wrote through a fixed "<snapshot>.tmp". /tmp is world-writable and sticky, and the display service runs as a different user from the tooling, so a leftover temp owned by anyone else became unopenable even by root -- fs.protected_regular refuses O_CREAT on a foreign file in a sticky directory. The preview and the health check's liveness proxy then froze until someone deleted the file by hand; on the test rig that meant 23 hours of a healthy display reporting "hardware: stale". Now uses tempfile.mkstemp with cleanup on failure, matching the hardware-status write a few hundred lines above. Closes #528. _poll_on_demand_requests read its mailbox with max_age=3600, and get() defaults the in-memory TTL to max_age -- so the first request was pinned in memory for an hour and every later poll returned that stale copy. No second on-demand request was honoured until the service restarted, while the API kept returning 200. get() already documents memory_ttl=0 for exactly this cross-process case. The consumed request is also now deleted: leaving it on disk meant a restart replayed the previous request, activated it, and ignored the one the caller had just made. Closes #530. starlark-apps returned None from display() when it has no app to show, which is the state of every install without Pixlet and of a fresh one before any app is added. The controller only skips on a boolean False, so that held a black panel for the full display_duration instead of rotating on. Closes #456 (core side). Verified on the rig: two consecutive on-demand requests with no restart between them are both activated, where the second was previously dropped in silence. * perf(harness): share one cache across a plugin's renders _instantiate built a fresh MockCacheManager for every (size, mode), and that mock is a per-instance in-memory dict, so each render was a cold start. A plugin that fetches per game or per player re-fetched everything N times over -- baseball-scoreboard at one size took 840s for nine renders where the arithmetic said ~72s, and at eight sizes it exceeded a 900s timeout. The second and later renders also never exercised the cache-hit path, which is what a running rig executes almost all of the time, so a caching regression could not be caught here. The cache is now built once per render_plugin_matrix call and threaded down. The display manager stays per-render -- the bounds checking depends on that -- so only fetched data is shared. Measured on the rig, same render counts and same goldens: tide-display 2s -> 1s (32 renders) cricket-scoreboard 10s -> 3s (24 renders) No pass/fail change across tide-display, cricket-scoreboard, clock-simple, geochron, christmas-countdown, of-the-day, web-ui-info and incoming-packages. Closes #533. * fix(scripts): run standalone plugin tests instead of collecting nothing run_plugin_tests.py discovered every plugin test file and handed the lot to pytest. Most plugin tests are standalone scripts -- module-level main() plus an `if __name__ == "__main__"` guard, signalling through an exit code -- and pytest collects zero items from those. The run printed how many files it had *found*, then "no tests ran", and exited without executing any of them. On a rig with all 44 first-party plugins that is 151 of 248 files. Files are now classified and each kind runs under the right runner: pytest for real test modules, subprocess for scripts, honouring the 0 pass / 2 skip / 1 fail convention ledmatrix-plugins' own runner established (a script that wants a tty or an LED matrix is a skip, not a regression). Before: $ python3 scripts/run_plugin_tests.py -p countdown -d ~/LEDMatrix/plugin-repos Found 1 test file(s) collected 0 items no tests ran in 0.31s rc=0 After: Found 1 test file(s) -- 0 collectable, 1 standalone script(s) 1 passed, 0 skipped, 0 failed (scripts) rc=0 Verified across three shapes: countdown (1 script), jellyfin-now-playing and pomodoro-timer (pytest only, 16 and 42 tests), and ledmatrix-flights (11 files split 4 collectable / 7 scripts, all seven of which had never run). Closes #532. Running the flights scripts for the first time also surfaced four genuinely failing tests there, hidden by the mirror-image bug in the plugins repo's own runner -- filed as ChuckBuilds/ledmatrix-plugins#464 and #465. * fix(harness): give an empty-looking mode a few frames before warning about it check_plugin's "drew nothing but display() returned X" warning fired on a single frame, rendered with force_clear=True, under a frozen clock. All three defeat a scrolling plugin, whose first frame is legitimately its blank scroll-in buffer. Across 44 first-party plugins, 60 of 76 warnings were false -- the rate at which people stop reading a warning, which matters because the true positives are real: a mode that draws nothing and does not return False holds a blank panel for its whole display duration. An apparently-empty frame is now re-driven for up to 48 more frames with force_clear=False (force_clear means "reset the scroll", so repeating it would redraw frame 1 for ever) and with the clock advancing -- freezegun's factory where time is frozen, a real sleep where it is not, since scroll position is usually a function of elapsed time. The first frame that draws content replaces the result. The clock is moved back afterwards. It is shared by every render in the matrix, so time borrowed by the probe leaked into later modes and drifted their goldens -- f1_upcoming picked up 5 spurious drifts before this was restored. Measured on the rig: empty warns check before after f1-scoreboard 42 0 48 PASS / 0 FAIL, goldens intact ledmatrix-elections 16 0 16 PASS / 0 FAIL on-air 8 8 true positive, kept nfl-draft 8 8 true positive, kept clock-simple/geochron/ 0 0 unchanged christmas-countdown 58 false positives gone, both true positives kept, no golden regressions. Cost is confined to modes that really are blank: plugins that draw immediately are unchanged (clock-simple and tide-display still 2s), while on-air -- eight deliberately blank modes -- goes to 21s. Closes #527. * fix(harness): load nested schema defaults, and merge caller config at leaf level load_config_defaults read only top-level properties. An object property carries its defaults on its children, not on itself, so everything nested was dropped -- 2,386 defaults across 37 of 44 plugins, soccer-scoreboard alone losing 539 of 565. render_plugin_matrix's comment says the plugin then "behaves like a real install", which for most of the fleet it did not. _defaults_from_properties now recurses. merge_config deep-merges the caller's config onto the result so an override lands at the leaf: a shallow merge would let -c '{"nhl": {"enabled": true}}' replace the whole nhl subtree and discard every other nhl default, which is the same class of bug being fixed here. Measured before/after across all 49 installed plugins on the rig: **no render changed** -- identical PASS/FAIL counts, byte-identical output, goldens intact. Plugins already fall back to the same values internally via config.get(key, default), so supplying them explicitly agrees with what they were doing. The defaults really are arriving now: ufc-scoreboard 9 -> 87 defaults ledmatrix-flights 51 -> 95 masters-tournament 10 -> 51 cricket-scoreboard 22 -> 50 tide-display 12 -> 18 and hockey-scoreboard, which used to load nhl.enabled=None, now gets nhl.enabled=True with its full display_modes block. Caveat worth carrying: the eight plugins with the most nested config (soccer, baseball, basketball, hockey, lacrosse, football, afl, nrl -- 1,634 of the 2,386 dropped defaults, 68%) could not be measured. They import src.common.sports_shared, which the test rig's core branch predates, so they fail to load there identically before and after. Re-run this comparison against a core that has that module before trusting the "nothing changed" result for them; those are exactly the plugins whose renders should change most. Closes #531. * refactor: narrow the exception handlers this branch introduced Codacy flagged the new code; it passes on other recent PRs, so the finding is mine. Four of the five broad `except Exception` clauses I added were catching far more than they needed to, which is the same shape as several bugs this branch fixes -- hello-world's TypeError sat invisible for exactly this reason. freezer() / move_to() / tick() -> (AttributeError, TypeError, ValueError) cache_manager.delete() -> (OSError, AttributeError, KeyError) The fifth stays broad and now says why: it wraps a call into a plugin's own display(), which can raise anything, and the first frame has already rendered -- so a failure there must not turn a good result into an error. Verified against a checkout of main: f1-scoreboard 48 PASS / 0 FAIL with 0 empty warnings, on-air keeps its 8 true positives, clock-simple 8 PASS. geochron shows 7 golden drifts both before and after this branch, so it is not from these changes -- its committed goldens predate #521's 1-bit text rendering. * fix: resolve CodeRabbit review and Codacy findings on #534 CodeRabbit raised six; all six were real. The test double had drifted ahead of production. VisualTestDisplayManager accepted set_scrolling_state(frame_hold=...) while DisplayManager did not, so such a call passed every harness run and would raise TypeError on the panel -- the one failure a safety harness exists to prevent. frame_hold belongs to the change that adds it to DisplayManager (#523), so it moves there and the double matches main again. The harness swallowed exceptions from re-rendered frames. _settle_loop re-renders a mode that came back blank, to give a scroll time to draw; returning silently on a crash meant a mode that renders one good frame and then explodes was reported as passing. Recorded on result.error now, keeping the captured frame so the failure stays inspectable. starlark-apps display() returned True after _display_frame() failed, so the controller held a dead frame for the whole display_duration instead of rotating on. _display_frame now returns bool on all three paths. run_plugin_tests.py used env.setdefault for PYTHONPATH and LEDMATRIX_CORE, so an inherited value won and the subprocess imported a different core than the one under test -- ledmatrix-plugins#467 exactly. Prepends PROJECT_ROOT and sets LEDMATRIX_CORE unconditionally. The on-demand mailbox is polled after every frame, ~125x/second on a scrolling mode, and the read is deliberately uncached, so it was that many disk reads per second to find nothing. Floored at 250ms, which is imperceptible for a web-UI click. Consuming it also deleted whatever was present rather than what had just been processed, so a request posted while the previous one was in flight was thrown away and never ran; the delete is now keyed by request_id. That narrows the window rather than closing it -- a true atomic claim needs a primitive the cache layer does not offer, and the code says so rather than implying otherwise. Codacy's 2 criticals were bandit B404/B603 on the subprocess call added to run_plugin_tests.py. Fixed interpreter, argument list, no shell; annotated with the repo's existing nosec convention. Bandit is clean on the file. Adds test/test_on_demand_mailbox.py (8), test_starlark_display_contract.py (4) and two settle cases in test_harness_empty_claimed.py. 4, 4 and 2 of those fail against the pre-fix code. Full suite: 3961 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * chore: satisfy Codacy's subprocess checks on the new test runner Codacy runs Bandit and Opengrep (its Semgrep fork). The new subprocess.run in scripts/run_plugin_tests.py trips three patterns, on two different lines: Bandit B404 on the import, B603 on the call Opengrep dangerous-subprocess-use-audit on the run( line dangerous-subprocess-use-tainted-env-args on the argv line A nosemgrep applies only to its own line, so the call line and the argv line each need one; a single comment on the call covered neither rule fully. Suppression is the right answer here rather than a rewrite: the interpreter is sys.executable, the arguments are a list, and no shell is involved, so there is nothing to word-split or expand. Matches the pair the rest of the repo already uses for this shape -- permission_utils.py, plugin_loader.py, install_dependencies_apt.py. Codacy: 0 new issues, up to standards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * chore: leave visual_display_manager untouched so #523 can merge The only change this branch made to that file was a docstring, and it collided with #523's rewrite of the same method -- so #534 and #523 each merged cleanly against main but conflicted with each other. Reverted to main's text; #523 owns this method and adds frame_hold to it. The note the docstring carried ('frame_hold arrives in #523') would have been stale the moment #523 landed anyway. The parity test in #523 is what actually keeps the two signatures honest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * chore: add the Ruff suppression nosec/nosemgrep do not cover Ruff reports S603 on the same call Bandit and Opengrep do, and none of the three suppressions covers the others. Confirmed the precondition first: path comes from discover_plugin_tests(), which globs test files inside the repo, and the call is a fixed interpreter with a list argv and no shell. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
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> |
||
|
ChuckandClaude Opus 5
|
91d15a8943 |
fix(display): draw text 1-bit, so glyphs stay crisp on the LED grid (#521)
An LED panel has no partial brightness. PIL defaults ImageDraw's fontmode to "L", which anti-aliases TrueType glyphs into a grey fringe the panel can only round off -- a 4px glyph arrives smeared into 3px. DisplayManager creates its shared `draw` in six places and set fontmode at none of them, while _load_fonts loads extra_small_font as 4x6-font.ttf at size 6. Measured at draw time, that face at that size puts 74% of its lit pixels at partial coverage. Every plugin drawing small text through the shared draw inherited the blur; geochron was the case that surfaced it. The harness's VisualDisplayManager had the same gap, which mattered more than it looks: goldens were recording anti-aliased text that production would not produce, so the harness could not have caught this. Fixing only production left geochron still blurry under the harness -- that is how the second site was found. Both are set to "1" so the harness renders what the panel renders. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Chuck
|
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. |
||
|
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.
|
||
|
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
|
||
|
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 |