Commit Graph
4 Commits
Author SHA1 Message Date
Claude f59274371b Merge refactor/api-v3-blueprint-split into feat/tools-mqtt-bridge-pixlet-editor-on-split
PR #554 was branched from #553 before its CodeRabbit-fix commit
(4e91cf1) landed, leaving GitHub reporting mergeable_state: dirty and
the branch missing eight fixes to shared api_v3 package code —
including a guaranteed ModuleNotFoundError on display.py's on-demand
restart path, a credential-redaction gap, and an unguarded manifest
read-modify-write race.

Resolved the two import-list conflicts (backup.py, starlark.py) by
unioning both branches' additions; the rest of the merge was clean.
Verified the merged tree keeps every usage of the fix branch's new
helpers (_starlark_manifest_lock, _coerce_to_bool) intact, the URL map
fixture still lists all 117 routes, and the full suite passes with only
the two pre-existing tzdata-related failures (unrelated, present on
origin/main too).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GwNEnaWDTzDbFE2M7MDjaq
2026-09-11 06:33:48 +00:00
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 56ba235f4c 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
2026-09-09 19:52:43 -04: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