docs(review-todo): check off items addressed in 2026-05-23 autonomous pass
Mark devices.py PATCH fix, WLED route-level test, IPv6 regression test, IconSelect XSS audit, PEP-604 sweep, magic-number constants, api/auth except specificity, and the (window as any) static-access cleanup as done. Defer items are unchanged: performance items keep their "profile first" caveat, Hue cert pinning + CSP keep the design- sensitive note, architecture refactors keep the multi-day banner, and i18n parity is now annotated with the exact missing-key counts (328 ru / 325 zh) so the next translator pass has a clear scope.
This commit is contained in:
+66
-48
@@ -18,6 +18,40 @@ redirects, single source of truth for IP classification in
|
||||
`utils/net_classify.py`, allowlist + parity test for inbound WS events,
|
||||
typed `Window` globals, and more).
|
||||
|
||||
## Items completed in the follow-up autonomous pass (2026-05-23)
|
||||
|
||||
- [x] **devices.py PATCH-without-url processor desync** — `update_device`
|
||||
now falls back to `existing.url` so a rename / icon-only edit
|
||||
always tells the processor the current address.
|
||||
- [x] **WLED scheme integration test** on `/api/v1/devices` — covers
|
||||
bare IPv4 (`http://`), public hostname (`https://`), and trailing-slash
|
||||
normalisation; lives in `tests/api/routes/test_devices_routes.py`.
|
||||
- [x] **IPv6 regression test** — `tests/test_url_scheme.py` now pins
|
||||
public IPv6 → `https://`, ULA → `http://`, and documents the
|
||||
Python-`ipaddress` documentation-prefix classification quirk.
|
||||
- [x] **IconSelect XSS audit + defence-in-depth** — every caller
|
||||
audited (all feed `icon` from constants or lookup tables); added
|
||||
`sanitiseIcon` that rejects `<script>`, `javascript:`, `on*=`,
|
||||
`<iframe>`, `<embed>`, `<object>` and warns to the console.
|
||||
- [x] **`Optional[T]` → `T | None` (PEP 604)** — 55 sites cleaned via
|
||||
`ruff --fix UP007`. The remaining `Union[…]` aliases for
|
||||
pixel/colour/device-config typing converted by hand. `UP007` now
|
||||
lives in `pyproject.toml` so the rule fires on new code.
|
||||
- [x] **Hot-path magic numbers → named constants** — `processed_stream`
|
||||
gains `_FILTER_RECHECK_EVERY_N_FRAMES`; `wled_target_processor`
|
||||
gains `_SKIP_REPOLL_SLEEP_SECONDS`, `_DIAGNOSTICS_REPORT_INTERVAL_SECONDS`,
|
||||
`_CSPT_RECHECK_EVERY_N_ITERATIONS`.
|
||||
- [x] **`api/auth.py` `except Exception` tightening** — every WS send /
|
||||
close site is now `except _WS_SEND_BENIGN_EXC` (a narrow tuple of
|
||||
WebSocketDisconnect / RuntimeError / ConnectionError / OSError).
|
||||
The auth-receive path catches the same set plus a final
|
||||
`logger.exception` catch-all for observability on truly unexpected
|
||||
shapes.
|
||||
- [x] **`(window as any)` cleanup** — 59 static-property accesses
|
||||
migrated to typed `window.<name>` against `global-types.d.ts`. The
|
||||
remaining 7 sites use dynamic string indexing (`window[fnName]`)
|
||||
and intentionally keep the cast (documented in the typedef file).
|
||||
|
||||
---
|
||||
|
||||
## Architecture refactors (multi-day — own session)
|
||||
@@ -92,12 +126,8 @@ typed `Window` globals, and more).
|
||||
because the UI uses inline event handlers / Jinja templates.
|
||||
Mis-set CSP would break the app silently. Defer until templates can
|
||||
move to event-delegated handlers, then add a strict policy.
|
||||
- [ ] **`api/auth.py` exception specificity** — 9 `except Exception:`
|
||||
sites. Most are intentional best-effort `websocket.send_json`
|
||||
swallows (the WS is already closed or about to be), but the auth
|
||||
decision path itself could be tightened to specific types
|
||||
(`jwt.InvalidTokenError`, `OSError`) + `logger.exception` for
|
||||
observability.
|
||||
- [x] **`api/auth.py` exception specificity** — done in the 2026-05-23
|
||||
pass; see top of file.
|
||||
- [ ] **Hue bridge cert pinning** — `httpx.AsyncClient(verify=False)` for
|
||||
Hue bridge (self-signed cert by design). Should record the
|
||||
certificate fingerprint at pairing time and pin it on subsequent
|
||||
@@ -105,58 +135,46 @@ typed `Window` globals, and more).
|
||||
|
||||
## Mechanical / code-quality (low risk, high line-count)
|
||||
|
||||
- [ ] **i18n parity** — **328** keys missing in `ru.json`, **325** missing
|
||||
in `zh.json`. Examples: `section.hide`, `filters.hsl_shift`,
|
||||
`filters.contrast`, `filters.temporal_blur`,
|
||||
`filters.audio_filter_template.desc`. Russian and Chinese users
|
||||
currently see raw keys for these. This is translation work, not
|
||||
code work.
|
||||
- [ ] **`Optional[T]` → `T | None`** (PEP 604) — large mechanical refactor
|
||||
across the codebase. Can be auto-fixed via `ruff check --fix
|
||||
--select UP007`. Worth doing once the file splits land.
|
||||
- [ ] **i18n parity** — confirmed **328** keys missing in `ru.json` and
|
||||
**325** missing in `zh.json` against the canonical English file.
|
||||
Translation work — needs a native speaker, not a machine-translation
|
||||
pass. Run `py scripts/diff_locale_keys.py` (or copy the diff block
|
||||
out of the 2026-05-23 pass log) to get the exact key list.
|
||||
- [x] **`Optional[T]` → `T | None`** — done; `UP007` now enforced via
|
||||
`pyproject.toml` so the rule prevents regressions.
|
||||
- [ ] **Hot-path `logger.error(f"...")` → `logger.error("... %s", e)`**
|
||||
lazy-eval — mostly cosmetic; ~200 sites. The f-string still builds
|
||||
the message even when DEBUG is off.
|
||||
- [ ] **Remaining `(window as any)` sites** — typed `global-types.d.ts`
|
||||
is in place and new code uses `window.foo` directly, but ~80
|
||||
existing sites still have the cast. Per-site mechanical cleanup.
|
||||
Add `eslint`-equivalent guard (TS rule) to prevent new ones.
|
||||
- [ ] **Magic numbers → named constants** in processing hot paths —
|
||||
`_FILTER_RECHECK_EVERY_N_FRAMES = 30` in
|
||||
`core/processing/processed_stream.py:159`; `5 ms` / `5 s` /
|
||||
`30 iterations` literals in `wled_target_processor.py:890,893,915`.
|
||||
- [ ] **Standardise `from __future__ import annotations`** across the
|
||||
codebase. Some modules use the future-annotation form, others stick
|
||||
with `Optional[...]`. Enforce one via ruff `FA` rules.
|
||||
lazy-eval — 658 sites flagged by `ruff --select G004`. Deferred
|
||||
because it is genuinely cosmetic at ERROR level (always emitted)
|
||||
and the cumulative cost is negligible. Worth doing if/when ruff
|
||||
gains a safe autofix, or as a Codemod in a dedicated session.
|
||||
- [x] **Remaining `(window as any)` sites** — 59 migrated to typed
|
||||
`window.<name>` access; the 7 surviving sites use dynamic string
|
||||
indexing and are documented as the legitimate exception.
|
||||
- [x] **Magic numbers → named constants** — done; see `processed_stream`
|
||||
and `wled_target_processor` constants at the top of each module.
|
||||
- [ ] **Standardise `from __future__ import annotations`** — partially
|
||||
mooted by the UP007 cleanup. Files that previously relied on
|
||||
`Optional`/`Union` no longer need the future import; the few that
|
||||
already use `__future__` keep it for forward-reference convenience.
|
||||
A blanket policy would still help — leave as a stylistic followup.
|
||||
|
||||
## Test gaps
|
||||
|
||||
- [ ] **Route-level integration test** for the WLED scheme inference —
|
||||
POST `/api/v1/devices` with `{"url": "192.168.1.42",
|
||||
"device_type": "wled"}` and assert the stored device has
|
||||
`url == "http://192.168.1.42"`. The helper is exhaustively
|
||||
unit-tested but no integration test exercises the create/update
|
||||
flow end-to-end.
|
||||
- [ ] **IPv6 public address regression** — extend `test_url_scheme.py`
|
||||
with explicit assertions for `2001:db8::1` and similar public IPv6
|
||||
literals (the bare-label fallback used to misclassify these). The
|
||||
helper does the right thing today via the IPv6 probe added during
|
||||
the hardening pass, but no test pins it.
|
||||
- [x] **Route-level integration test** for the WLED scheme inference —
|
||||
done; covers create + update in `tests/api/routes/test_devices_routes.py::TestWLEDSchemeInference`.
|
||||
- [x] **IPv6 public address regression** — done; pinned in
|
||||
`tests/test_url_scheme.py` for both bracketless and bracketed forms.
|
||||
|
||||
## Pre-existing issues surfaced during the audit (not in our diff)
|
||||
|
||||
These were flagged by the auditors but predate the review session — kept
|
||||
here as a future-work backlog:
|
||||
|
||||
- [ ] **`icon-select.ts:_buildGrid` `item.icon` is interpolated raw** —
|
||||
documented as "trusted SVG by design". If callers ever feed
|
||||
user-supplied icon strings, that's an XSS sink. Audit every caller
|
||||
that builds `IconSelectItem.icon` from non-constant data and
|
||||
reject HTML there.
|
||||
- [ ] **`devices.py:461` `manager.update_device_info(device_url=update_data.url)`**
|
||||
receives `None` when a PATCH omits `url` (rename / icon-only edit).
|
||||
The processor never re-syncs in that case. Should pass
|
||||
`existing.url` (after normalization) or skip the call.
|
||||
- [x] **`icon-select.ts:_buildGrid` `item.icon` is interpolated raw** —
|
||||
audited; all callers pass project-owned literals or table-lookup
|
||||
results. Added a runtime sanitiser as defence-in-depth.
|
||||
- [x] **`devices.py` `manager.update_device_info(device_url=update_data.url)`**
|
||||
None-on-PATCH path — fixed; now falls back to `existing.url`.
|
||||
- [ ] **`asyncio.gather` over uncapped client lists** in preview broadcasts
|
||||
— slow clients block the loop. Already noted under Performance
|
||||
above; pre-existing.
|
||||
|
||||
Reference in New Issue
Block a user