3 Commits

Include renames

Author	SHA1	Message	Date
Claude	4e91cf1fef	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	2026-09-11 02:40:46 +00:00
ChuckBuildsandClaude Opus 5	60d5d19898	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	2026-09-09 19:41:02 -04:00
ChuckandClaude Opus 5	5713fd20a7	fix(calendar): implement the OAuth and calendar-listing endpoints (#458) ... * fix(calendar): implement the OAuth and calendar-listing endpoints The plugin's config advertised a three-step setup and only step 1 existed. Step 3's picker fetched /api/v3/plugins/calendar/list-calendars, which was never registered, so Flask fell through to the global 404 handler and the user saw "Resource not found" -- a message that names nothing and points nowhere. Step 2 had no endpoint at all, so even a working picker would have found no token to list with. Two routes, following the pattern the spotify and ytm plugins already use for their own auth scripts: POST /plugins/calendar/authenticate two-step Google OAuth GET /plugins/calendar/list-calendars calendars for the picker The authenticate route drives calendar_registration.py, which the plugin already ships and which was written expressly for this -- it reads a redirect URL on stdin and prints one JSON object. It takes two calls because a human has to visit Google in between; the script persists the PKCE verifier from the first call for the second, without which the exchange fails with "Missing code verifier". The listing route reads the token directly rather than shelling out again: the picker is interactive and a subprocess per click is slower than the API call it would wrap. It refreshes an expired token in place, sorts the primary calendar first, and drops entries with no id, which could not be selected anyway. Both name the plugin when it is not installed, rather than reproducing the anonymous 404 that started this. Verified against the live Google API on the dev rig: HTTP 200 with the account's real calendars. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(calendar): one input for the auth code, and a louder warning Two things reported after testing the flow. There were two boxes and no way to tell which to use. The config template's string branch dispatches widgets from an allow-list of names, and anything missing from it falls through to a plain input type=text -- so the field rendered both the widget's own box and a stray one for the same key. google-oauth is now on that list, which is all the widget ever needed to render in place of the fallback rather than beside it. And the warning that the redirect page fails to load was small grey text under a link, which is where it is least likely to be read. It is now an amber callout that leads with "The next page will fail to load. That is expected." The failure lands at exactly the moment the user has to act on it, and it looks precisely like the flow breaking rather than working. The paste box is labelled too, rather than relying on a placeholder that vanishes on focus. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(calendar): redact script diagnostics, and page the calendar list Three findings from CodeRabbit, all valid. Raw subprocess output was being returned to the client -- the script's stderr on one path, and its own error payload on another. CodeQL flagged the same line. That script handles OAuth client secrets and interpolates exceptions into its messages, so either could carry a secret or a path. Both now go to the log unredacted, where they are worth having in full, and reach the client through a redactor. That redactor already existed inside describe_exception, which only takes exceptions. Split out as redact_text: an exception is not the only thing worth returning, and a subprocess's stderr is just as capable of quoting a token. calendarList.list returns 100 entries per page by default, caps at 250, and hands back a nextPageToken when there are more. Reading one page would have hidden calendars from the picker with nothing to say the list was cut short. It now pages, asking for 250 at a time, bounded at ten pages so a malformed token cannot spin. And a test helper was a lambda where ruff wants a def. The five new tests fail against the previous commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(calendar): redact the last two raw exception interpolations CodeQL flagged four exposure paths. Two were mine and genuinely raw: the OSError from failing to spawn calendar_registration.py, which carries the interpreter path and whatever the OS chose to say, and the ImportError for the Google libraries, whose message named the missing module by interpolating the exception directly. Both now go through describe_exception, and the unredacted text goes to the log. The other two are the repo-wide pattern from PR #448 -- 67 handlers on main already return details=describe_exception(e), and these two new handlers follow it. That function is the sanitizer: it strips URL userinfo, auth headers and credential-shaped key=value pairs, collapses to one line and caps the length. CodeQL's taint tracking cannot see a sanitizer it has no model for, so it reports the flow regardless. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(calendar): announce status changes, name the paste box, drop a no-op Three more from the review, all valid. The status line is written after every async call -- the consent link is ready, the exchange failed -- and was a plain paragraph, so a screen reader was told none of it. It is a live region now. The paste box had a visible label that was never associated with it, so its only accessible name was the placeholder, which disappears on focus: precisely when the value is being pasted. The label now points at the input by id. And a conditional in the test helper returned the same value from both branches, which Ruff flags as RUF034. It was left over from making the fake page; one page is all those cases need, and TestPagination builds its own sequences. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * test(calendar): assert the accessibility relationships, not their parts The previous assertions searched for role="status", aria-live, a label `for` and an input `id` independently, so they passed whether or not those belonged together. Two attributes on different elements announce nothing, and a `for` that names something other than the input leaves it just as anonymous. Both attributes are now asserted on the status element itself, and the label and input are checked to go through the same identifier rather than merely both existing. Verified by mutation: a mismatched pair and a displaced aria-live are both caught. Reported by CodeRabbit, against tests I had written two commits earlier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-13 15:25:43 -04:00