2 Commits

Include renames

Author	SHA1	Message	Date
ChuckandClaude Opus 5.5	3a81f38f09	fix(web): uniqueItems saves, /health count, Vegas order wipe; one list-repair helper (#638) ... * fix(web): drop repeats from uniqueItems lists before validating a plugin save dedup_unique_arrays lost its only caller in #330, so submitting a value a uniqueItems list already holds (a stock symbol saved once and posted again) failed the whole save with a validation error. _prepare_plugin_config_for_save runs it again just before validation, which covers both POST /plugins/config and plugin sections posted to /config/main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): /health counts the discovered plugins and logs the checks it fails The plugin check counted plugin_manager.get_available_plugins(), which PluginManager does not have, behind a hasattr guard that made plugin_count 0 on every device. It now counts the discovered manifests, discovering first when nothing has been scanned yet. The config, plugin and hardware checks answered "see logs for details" without logging anything. Each now logs a warning with the traceback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): store refresh no longer claims a commit-metadata refresh POST /plugins/store/refresh read fetch_commit_info (or fetch_latest_versions) only to append "(with refreshed commit metadata from GitHub)" to its message. It never fetched any: the route re-downloads the registry and nothing else. search_plugins takes the flag, but it reads commit info through its cache, so passing it on would not refresh anything either. The flag is ignored now and the message says what happened. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): refuse a malformed Vegas plugin order instead of clearing it A vegas_plugin_order or vegas_excluded_plugins value that was not JSON, or not a list, was stored as [] and the save answered 200, so a bad value wiped the saved order or exclusions. Both now answer 400 and save nothing, the way plugin_rotation_order already did; the three share one parser. A list that holds anything but plugin-id strings is refused as well. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): per-plugin health and metrics read the display service's latest GET /plugins/health/<id> and /plugins/metrics/<id> called get_health_summary and get_metrics_summary without force_reload, so they answered with whatever the web process read first and kept in memory, while the display service kept writing newer state. They now pass force_reload=True, as the list routes do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): plugin config reset saves through the shared atomic save POST /plugins/config/reset called config_manager.save_config directly, so it took no backup, and a failed write escaped as an unhandled exception. It then handed on_config_change the raw stored section, not the prepared config a loaded plugin runs with. It now saves through _save_config_atomic with a backup, answers CONFIG_SAVE_FAILED when that fails, and notifies with _prepared_plugin_config, as POST /plugins/config does. POST /plugins/toggle carried its own copy of _save_config_atomic's save_config_atomic-or-save_config fallback; it calls the shared helper now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): one reading and one "unavailable" for each system metric system_metrics.collect_system_metrics() promised None for a metric it could not read, but returned cpu_temp as 0 off a Pi, and the whole no-psutil fallback as zeros. GET /system/status measured the same numbers a second time with its own code, and answered None there. Now both come from collect_system_metrics(), and "unavailable" is None everywhere. /system/status keeps its 0.1s CPU sample and its 10s cache, and gains nothing it did not already send. Two differences: without psutil it answers 200 with null metrics instead of 503, and a disk it cannot stat is null instead of a 500. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): /display/current sends the snapshot as-is and logs a failed read GET /display/current PIL-decoded the preview snapshot and re-encoded it before base64-ing it, spending CPU on the Pi to send the same picture, and dropped any failure with `except Exception: pass`. The /stream/display SSE stream already passed the PNG's bytes straight through. Both now read through web_interface/display_preview.py and answer with the same payload. A missing snapshot is still a null image; any other read failure is logged as a warning. /health reads the snapshot path from the same module. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(web): one helper puts a submitted plugin config's lists back The plugin-config save turned position-keyed dicts ({"0": ..., "1": ...}) back into lists in five copies: four in the form path's fix_array_structures (whose prefix branches never ran, since no caller passed one), and _fix_json_arrays on the JSON path. It then force-fixed the news plugin's feeds.custom_feeds by name, in case the generic pass had missed it. src/web_interface/config_arrays.coerce_array_shapes now does it for both paths, custom_feeds included. ensure_array_defaults duplicated _fix_none_arrays and is gone. In the same function: the union-type re-checks that the null handling above them made unreachable, the "(temporary)" random_seed debug log, and a commented-out log line are removed. A failed validation is logged once as a warning, not four ERROR lines and a WARNING. Element types are left to normalize_config_values, which already converted them for both paths. One difference: the form path no longer adds an empty {} for a nested object the post left out that has no defaults. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(web): import at module top and log through the module logger The web_interface.cache imports in config.py and fonts.py were wrapped in `except ImportError` fallbacks. It is an in-repo module that imports nothing from the project, so it cannot fail to import; it is imported once at module top, as system.py now does. cache.py's docstring said blueprints import it lazily "to avoid circular imports"; it now says why that is unnecessary. Five logging.error calls in the dim-schedule GET and three logging.warning calls in plugins.py went to the root logger; they use the module logger. Function-local re-imports of json, os, shutil, logging and Path, all already imported by the module, are gone. The `import os` inside two except blocks of save_plugin_config also made os a local name for the whole function. execute_plugin_action's step-1 handler gets a comment saying why it stays: it looks like a copy of the blueprint handler, but without it a TimeoutExpired from the plugin's script would reach the route's own `except subprocess.TimeoutExpired` and be answered as a 408. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(web): app.py loses dead CSRF and reconciliation state, comments fixed - csrf was always None, so `if csrf: csrf.exempt(...)` never ran, and its note that the api_v3 blueprint "is exempted above" named an exemption that does not exist. Both are gone; the reason there is no CSRF protection stays, shortened. - The SSE rate-limit comment called the default "tight" at 20 per minute. The default is 1000 per minute and the streams' 200 is the tighter one; the comment now says so. The limits are unchanged. - _reconciliation_done was written and never read. The docstring that explains why reconciliation runs once keeps its reason, in the present tense. - Removed: a dangling "import cache functions" comment with no import under it, a "security check ... within project_root" label on an existence check, the "(simplified version)" narration, and the note that no redirect route is needed. The preview loop's sleep comment no longer mentions a PIL encode that the loop does not do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(web): api_v3 comments name the package __init__, not a _common module Every route module's docstring said the shared blueprint comes "from ._common", a module the package split never created; they name the package __init__. The PROJECT_ROOT comment described the path from _common.py; it now describes this package and keeps the incident it guards against. The "(corrected) in this commit" note in resolve_pull_command and the /health comment the split's mechanical time -> _pkg.time rewrite garbled ("Stamp the start _pkg.time") read correctly again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(web): drop hasattr checks for attributes PluginManager always has PluginManager.__init__ sets health_tracker and resource_monitor (to None until they are configured), so the seven hasattr(api_v3.plugin_manager, ...) guards in the health, metrics and limits routes were always true. The falsy checks that do the work stay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(web): pages_v3 dispatches partials from a dict with one error handler load_partial chose a loader through a fourteen-branch if/elif, and thirteen of the loaders then wrapped themselves in the same try/except, logging "Error loading partial" without saying which. The route now looks the name up in _PARTIAL_LOADERS and has the one handler, which logs the partial's name. The loaders just render. _load_tools_partial keeps its own messages. The search index's _partial_html already catches a loader that raises. serve_plugin_web_ui repeated _plugin_dir_for inline (containment plus the ledmatrix- prefix fallback); it calls it now. Also removed: the unused markupsafe.escape import, function-local json/Path re-imports, and unused exception bindings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(web): remove unused imports, locals and a try that cannot fail - get_error_aggregator was imported by the api_v3 package and used by no one; seven names config.py imported, and Path in misc.py and logging in plugins.py, likewise. - branch_info in install_plugin was built and never logged; test_config in /health was bound and never read (the load_config call is the check). - An f-string with no placeholders in the asset upload route. - _installed_plugin_ids wrapped list(manifests.keys()) in try/except; _discovered_plugin_manifests always returns a dict. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(web): start.py logs its startup lines and drops unreachable branches The startup banner went to stdout with print(); it goes through a logger now, which the app import has already configured, so it reaches the journal with a level and timestamp like every other line. The "no addresses" branch is gone: get_local_ips() always returns at least "localhost". The except around app.run re-raised "only if it's not a client disconnection error" from inside the branch that had just established it was one, so that raise could not run. It is one check now, on a named tuple of the errnos, which the werkzeug log filter uses too. The comment on threaded=True counts three SSE endpoints, which is how many there are. Trailing whitespace is stripped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(web): save_main_config names its General fields once The General tab's field names were listed twice, once to detect a General form post and again, with four more, to keep the remaining-keys merge from storing them as top-level keys. GENERAL_FIELDS and _MAPPED_TOP_LEVEL_FIELDS hold them now, and the four per-section skip checks are one set. The comment on that merge said plugin configs are handled "here too", and "(including plugin keys)". Plugin sections are handled and removed from the body before it runs; the comment says so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(web): plugin directories come from the plugin manager only Six lookups fell back to PROJECT_ROOT/plugins/<id> when there was no plugin manager: GET /plugins/config's of-the-day data, POST /plugins/action, the plugin static-file route, the calendar credentials upload and the calendar OAuth routes. The loader never scans plugins/ (PluginManager.discover_plugins reads only the configured directory, plugin-repos by default), so what they found there was a plugin that never runs. _plugin_directory() asks the manager and answers None without one, which each route already reports as "not found". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(changelog): web-backend Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>	2026-09-24 17:35:49 -04:00
ChuckandClaude Opus 5	10e75b977f	Cover the next tier of untested modules and endpoints, and fix the 43 bugs that surfaced (#459) ... * test(sync): cover the display sync protocol, and fix what that surfaced DisplaySyncManager had no tests at all — it appeared in the suite only as a MagicMock() stand-in, so none of its framing, handshake, or socket handling was ever exercised. Writing that coverage surfaced three bugs. Both receive loops caught the generic Exception and immediately retried. A socket left in a bad state raises on every call, so the thread spun at 100% CPU logging the same line; the reverted-code run of the new regression test takes 24 seconds where the fixed one takes 0.2. Both now back off briefly before retrying. The follower dispatched on `data[:8] == _RAW_MAGIC or len(data) > 512`. That size threshold is not part of either wire format: a control message over 512 bytes — a hello_ack carrying a long incompatibility error, for instance — went to the image decoder and was dropped, and a raw frame under 512 bytes went to the JSON parser. Both formats are already self-describing, so dispatch on the magic prefix and treat a JSON parse failure as the legacy unmarked PNG, with the shared frame bookkeeping factored into _handle_received_frame(). _oversized_frame_warned was created on first use through getattr(self, ..., False) rather than in __init__, alone among the instance attributes. 75 tests: role parsing, the hello compatibility matrix, watchdog timeouts, both receive loops, the TCP image server's length and dimension caps and decompression-bomb guard, status shape per role, and one end-to-end loopback handshake so the wire format is exercised for real and not only against mocks. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test(logos): cover LogoHelper, and stop bad downloads poisoning the cache Nothing in test/ referenced logo_helper.py, so its caching, resizing and download-fallback logic was entirely unexercised. Two bugs surfaced. _download_logo wrote response.content to disk with no size limit and no check that the bytes were an image. A logo URL is remote input, so the response chose how much went into the assets directory; worse, an undecodable one stayed there, and because load_logo() only reports the decode failure and returns None, every later call re-read the same corrupt file. The download path never retried, so a single bad response made a logo permanently blank rather than falling back to the placeholder. Cap the response, verify it decodes, and delete it if not, which lets the existing fallback in load_logo_with_download do its job. get_cache_stats() divided by self.cache_size with no guard, so a helper built with cache_size=0 raised ZeroDivisionError from what is only a stats call. 37 tests: size-qualified cache keys, LRU eviction and refresh, the four load_logo_with_download paths, download permissions and timeout, placeholder generation, and the abbreviation normalizer — including a test pinning its deliberate divergence from LogoDownloader.normalize_abbreviation, since logo filenames on existing installs depend on both behaviors staying put. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test(web): cover the error and response builders, and stop dropping empty values errors.py and error_handler.py's response builders had no direct tests, though every API response passes through them. Two bugs surfaced. WebInterfaceError set suggested_fixes with `or`, so a caller passing [] to mean "I have no suggestions for this one" got the default list instead. Only None should fall back. create_success_response gated `data` on `is not None` but `message` and `metadata` on truthiness, so an explicitly-passed "" or {} vanished from the response while 0 and False survived — the response shape depended on the value. api_helpers.success_response() then re-gated metadata the same way, which is the path every api_v3 endpoint actually calls, so fixing only the inner function would have changed nothing observable. Both now use `is not None`. That wrapper also merged request timing into the caller's own metadata dict in place. A caller reusing a dict across requests would accumulate previous responses' timings; it now copies before adding. 79 tests: category inference for every error code, mapped vs fallback suggestions, the JSON shape including which keys are omitted when empty, exception-to-code inference, and the success/error builders end to end. Two behaviours are pinned as deliberate rather than fixed: an empty context stays out of the response body, and from_exception's `message` is the fixed per-code string, never the raw exception text. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test(web): cover the input validators, and close three holes in them validators.py had tests for dedup_unique_arrays only; the other eight functions were untested. Three bugs surfaced. validate_image_url checked for '..' only inside its relative-path branch, so http://host/../secret passed validation while /../secret was rejected — the traversal check now runs before the branch split, which is where a safety check on the whole URL belongs. validate_file_upload lowercased the uploaded filename's extension but compared it against the caller's list verbatim, so allowed_extensions of ['.TTF'] rejected every valid .ttf file. Both sides are lowercased now. The one in-tree caller passes lowercase already, so this only widens what future callers can hand it. validate_numeric_range accepted True and False, because bool subclasses int; a boolean then compared as 1 or 0 against the range and validated cleanly. Excluded explicitly, matching how base_plugin.py already handles the same trap for display_duration. 84 tests. Two behaviours are pinned rather than changed: sanitize_plugin_config deliberately does not HTML-escape strings, since escaping at this layer would store the escaped form in config.json — the docstring said "prevent injection", which read as a promise it does not keep, and now says what it actually does. validate_font_awesome_class's second 'fa-' check is unreachable behind its own regex; harmless, so characterized rather than removed. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test(api): cover wifi and registry endpoints, and fix bodyless POSTs The /wifi/* routes drive the host's real networking and the registry routes reach GitHub, and neither had endpoint-level tests. Covering them surfaced a bug affecting six endpoints. Six handlers read their body as `request.get_json() or {}`. The `or {}` says every field is optional and a missing body should fall back to defaults — but get_json() without silent=True raises UnsupportedMediaType when there is no JSON Content-Type, and it raises before `or {}` is ever evaluated. Each handler's catch-all then reported that as a 500. So POSTing with no body — what curl sends by default, and what a fetch() without options sends — failed on /plugins/store/refresh, /display/on-demand/start, /plugins/config/reset, /plugins/of-the-day/json/delete, /plugins/{id}/limits and /plugins/authenticate/spotify. The shipped UI always sends a JSON object, which is why this stayed hidden. All six now use silent=True. test_api_v3_optional_body.py covers the affected endpoints and adds a source check, since the combination of `or <default>` with a non-silent read is self-contradictory wherever it appears and is easier to catch by inspection than by exercising each endpoint by hand. Also adds test/_api_v3_test_helpers.py: the blueprint holds its managers on a module-level singleton rather than in Flask app state, so a test that mocks them leaks into every later test unless the originals are restored. The existing _make_client() does this for unittest classes; this is the pytest-fixture equivalent, for the five suites still to come. 69 endpoint tests: connect/disconnect/AP/radio including the string-aware boolean coercion these endpoints deliberately use, the radio's lockout-refusal path, registry refresh and fetch-from-URL, and a guard that WiFiManager is never constructed for real. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test(api): cover the music auth endpoints, and always clean up the wrapper The Spotify step-2 handler writes a Python wrapper script to a temp file with the user's redirect URL embedded in its source, then executes it. That is the most dangerous shape in the blueprint and had no tests. The wrapper was deleted in the success/failure branch and again in the TimeoutExpired handler. Any other failure from subprocess.run — no interpreter, a fork failure, an interrupted call — reached neither, and left a world-readable temp file containing the user's redirect URL on disk. Cleanup moves to a finally block, which is what "delete this whatever happens" should have been from the start. The injection tests are the point of this file. Eight adversarial redirect URLs (embedded quotes, backslashes, newlines, triple quotes, a full `"; import os; os.system("id"); "`) are each pushed through the endpoint and the generated wrapper is parsed with ast: it must still be valid Python, the URL must still be a single string literal bound to redirect_url, and no os.system call may appear anywhere in the tree. json.dumps holds up, but nothing was checking that it does. 40 tests. Also pins that the two endpoints are not symmetrical despite the matching names — only Spotify has a two-step flow and a wrapper; YTM runs its script directly — so a later change does not "restore" a parity that was never there. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test(api): cover the credentials upload, and stop it hoarding secrets The endpoint that receives the user's Google OAuth credentials file had no tests. Two bugs surfaced. The OAuth-shape check ran inside `except Exception: pass`. A JSON document that parses but is not an object — a bare 42, true, null, a list — makes `'installed' not in creds_data` raise TypeError, which the bare except swallowed, and the file was then written out as credentials.json regardless. The check now decides the outcome instead of being advisory, so anything not credentials-shaped is refused up front rather than failing later inside the calendar plugin. Every overwrite copies the old file to credentials.json.backup.<ts> and nothing removed them, so a user who re-uploaded ten times had ten complete sets of OAuth client credentials sitting in the plugin directory, indefinitely. Keep the newest five. Pruning is housekeeping, so a backup that cannot be removed logs and leaves the upload alone. 27 tests: size and extension limits, malformed JSON, the shape check, 0600 permissions on the written file, backup-on-overwrite, and pruning including the repeated-upload case that stays bounded. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test(api): cover the install endpoints, and make 14 dead guards reachable /plugins/install and /plugins/install-from-url were tested only at the PluginStoreManager layer, so the route logic — the queue-versus-direct branch, schema invalidation, discovery, state and history recording — was unexercised. Covering them surfaced the wider form of the body-parsing bug fixed for the `or {}` handlers in the previous commit. Fourteen handlers read `data = request.get_json()` and immediately guard with `if not data: return 400, 'No data provided'`. That guard cannot run: get_json() without silent=True raises UnsupportedMediaType for a request with no JSON body, so the catch-all answered 500 "an error occurred; see logs for details" where the handler plainly meant to answer 400 and say which field was missing. Every one of these endpoints told a caller who simply forgot the body to go read the server logs. All fourteen now use silent=True, so the guard each author already wrote is the one that runs. This covers /config/raw/main and /config/raw/secrets among them, whose own bodyless case had the same shape. The two remaining bare reads are left alone: neither declares what a missing body should do, so there is no stated intent to honour. 31 install tests plus 17 body tests. The install pair is checked against each other rather than only individually — the same install logic is written twice, once in the queue callback and once in the fallback, so the tests assert both produce identical schema, discovery, state and history effects. They agree today; the one difference is the success message wording, which is characterized rather than changed. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test(api): cover the raw config write endpoints /config/raw/main and /config/raw/secrets write whatever JSON they are given straight to config.json and config_secrets.json, bypassing the secret-separation path the rest of the config surface goes through. Given how carefully that surface keeps secrets out of config.json, the pair that skips it was worth pinning precisely. Backed by a real ConfigManager over tmp_path, so the assertions are against files on disk. 20 tests covering both routes: what lands in which file, that a raw secrets write never touches config.json and vice versa, the GitHub token reload, the uninitialized-manager and empty-body branches, and the ConfigError path that carries config_path through to the response. The bypass itself is pinned as intentional rather than changed — these back the raw JSON editor, so writing the body verbatim is the feature. The test says so explicitly, because the failure mode is someone later routing plugin config through here as a convenience and silently losing secret separation. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test(api): cover backup restore and path containment, and fix restore scope Restore is the most destructive thing the web interface can do — it overwrites config, secrets, WiFi settings and fonts, then reinstalls plugins — and neither it nor the file routes beside it had tests. A malformed `options` field fell back to {}. Every RestoreOptions flag defaults to True, so a caller who asked for a narrow restore and mis-serialized the request got a full one instead, secrets included, and was told it succeeded. Valid JSON that is not an object was worse: `"null"` or `"[1,2]"` reached .get() on a non-dict and raised, so the request died as a generic 500. Both are now refused with a 400 that says what was wrong, and restore_backup is never reached. The other file routes take a filename straight out of the URL and turn it into a path — one to read, one to unlink. _safe_backup_path is the only thing keeping those inside the export directory, and it was untested. No bypass was found; the thirteen traversal shapes are pinned so a later loosening of that pattern has to argue with something. The delete route's by-name enumeration is covered too, including that a directory sharing a backup's name is not removed. 84 tests. Two behaviours are pinned as intentional: a failed plugin reinstall turns the whole restore into an error even though file restoration succeeded, and omitting `options` entirely still means restore everything — that is the documented default, and it is only the mis-serialized case that was wrong. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * ci: raise coverage floor to 52% Measured 54.45% after the Tier 1 and Tier 2 suites, up from 50%. Keeping the same two points of headroom the 45 -> 48 ratchet used. The modules this branch set out to cover: sync_manager 0 -> 97%, logo_helper 0 -> 98%, errors and error_handler 0 -> 100%, validators 0 -> 97%. api_v3 moved less in percentage terms because it is 4,341 statements, but the endpoints covered are the destructive and credential-handling ones. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test(sync): probe for a free port on loopback, not every interface CodeQL flagged the ephemeral-port probe in the handshake test for binding to all interfaces. The probe only needs a free port number, so loopback is both sufficient and correct — a test should not open a port to the network to discover one. The manager under test still binds to all interfaces, which is deliberate and already marked nosec: a follower has to receive the leader's UDP broadcast. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * fix: bound the logo download, and stop malformed input reading as a fault Review findings on the coverage branch. The download size cap I added checked len(response.content), which has already buffered the whole body -- it stopped the bytes reaching disk but not memory, which was the point. A server that omits Content-Length and never stops sending would still exhaust the process. Stream it instead, counting as it arrives, into a sibling .part file that is replaced over the target only once it decodes. A transfer that dies midway now leaves nothing behind rather than a truncated logo for load_logo() to cache. The follower's control-message handler caught three exception types, but two reachable UDP payloads raise others: a bare JSON scalar makes msg.get() raise AttributeError, and an "sx" carrying a non-numeric x raises ValueError or TypeError from float(). Those escaped to the outer handler, skipping the legacy-PNG fallback and -- since this branch added a backoff there -- charging one malformed packet a 0.1s stall on the receive path. The legacy-PNG path also decoded without the dimension cap its TCP counterpart applies, so a crafted 65KB frame could force a large allocation on the render thread; both paths now share one constant. Three repo_url handlers called .strip() on client input without checking it was a string, so {"repo_url": 12345} answered 500. The credentials upload parsed the same file twice, the second time inside a bare except that a preceding parse had already made unreachable. And both raw-config handlers kept a json.JSONDecodeError arm that get_json(silent=True) had turned into dead code, collapsing "sent something unparseable" into "sent nothing" -- they now say which. Two of the new tests were not testing what they claimed. The pruning round-trip wrote ten backups inside one second, so all ten landed on the same int(time.time()) filename and overwrote each other; it never reached the limit it asserted. And the sync clock helper patched attributes on the stdlib time module, freezing time process-wide for every daemon thread earlier tests had left running. Full suite: 3352 passed, coverage 54%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test(sync): probe broadcast by sending, not by listening The broadcast check added in the previous commit bound INADDR_ANY to receive its own probe datagram, and the free-port probe did the same to pick a port. CodeQL flagged both, correctly: a test suite has no reason to open a socket the whole network can reach. Sending is enough for what the probe is actually for. An environment that refuses broadcast raises on sendto, which is the case that occurs in sandboxes and is the one worth skipping over; confirming delivery would have required the listening socket. A network that accepts the send and silently drops it still reaches the assertion, exactly as it did before either commit. The port probe binds loopback -- it only needs a number, and the manager's own bind is the one that has to succeed, with the retry loop already covering a port taken elsewhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * fix: keep callback faults out of the frame-decode fallback Review follow-up on the previous two commits. Widening the control-message except tuple put the callback dispatch inside it, so an _on_new_cycle() that raised ValueError, TypeError or AttributeError sent a perfectly good control packet to the legacy PNG decoder -- which reported it as an image decode error and buried the real fault. Split the two: whether the payload parses as JSON decides frame vs control message, a second guard covers reading the fields of an attacker-shaped body, and the callback fires outside both. It still cannot kill the receive thread; the loop's own handler catches it, and now says what actually went wrong. The logo download's temp file was a fixed "<name>.part". Two plugins asking for the same logo at once would interleave writes into it, publish the mixture, or delete each other's partial. mkstemp gives each download its own name in the same directory, so os.replace stays atomic. Its descriptor is adopted by fdopen before the request runs, since a request that raises before the write would otherwise leak the fd -- quietly, because load_logo_with_download swallows that. Two test fixes: the oversized-frame test replaced PIL.Image.open process-wide, the same hazard the clock helper documents, and Ruff B007 on an unused loop variable. Full suite: 3355 passed, coverage 54%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test(sync): cover the announce loop, and reject non-finite scroll positions Three review findings from the follower receive path. Non-finite scroll x reached follower rendering. json.loads accepts the bare NaN/Infinity literals and float() accepts them as strings, so "x": NaN arrived as a real float and was stored verbatim. NaN loses every comparison the scroll code makes, so a follower given one sits on a position it can never advance past. It now raises through the existing malformed-control-message guard, which logs and drops the packet and leaves the last good position in place. _broadcast_available() only proves the host accepts sendto() for a broadcast; a network that accepts the send and drops the packet would let TestRealSocketHandshake run to its five-second deadline and fail on assertions the code did not break. The deadline now distinguishes the two: if not one packet crossed in either direction, that is the environment, and the test skips rather than reporting a protocol failure. That skip could hide a real regression in the announcing side, so TestFollowerAnnounceLoop covers it on mock sockets, where no network is involved and nothing can skip: hello carries this display's hardware config and goes to the broadcast address, heartbeats follow, an empty hardware config falls back to 32x64x1, hello is not resent before its interval, and a send failure is swallowed rather than killing the loop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh --------- Co-authored-by: Claude <noreply@anthropic.com>	2026-08-21 11:16:52 -04:00