From 6decdc7fc9db2ebeca16adabc6d056d68c93d9ab Mon Sep 17 00:00:00 2001 From: Jonathan Date: Mon, 13 Jul 2026 15:04:19 +0200 Subject: [PATCH] docs: implementation plan for cleanup batch (Tier A + B) Co-Authored-By: Claude Opus 4.8 (1M context) --- .../plans/2026-07-13-cleanup-batch.md | 806 ++++++++++++++++++ 1 file changed, 806 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-13-cleanup-batch.md diff --git a/docs/superpowers/plans/2026-07-13-cleanup-batch.md b/docs/superpowers/plans/2026-07-13-cleanup-batch.md new file mode 100644 index 0000000..8f0f6f4 --- /dev/null +++ b/docs/superpowers/plans/2026-07-13-cleanup-batch.md @@ -0,0 +1,806 @@ +# Cleanup Batch (Tier A + B) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Close the deferred backlog's fix-items — PATCH allowlist hardening + a11y/copy polish (Tier A), and the two design tradeoffs the user chose to fix properly: per-seed discovery scoring and a persisted scan worklist (Tier B). + +**Architecture:** Tier A is small web edits to two Next.js route handlers and one client component. Tier B adds two additive tables via one Prisma migration: `DiscoverySeedContribution` (normalizes discovery scores so a suggestion's score = sum of every seed's latest contribution, independent of chunk/sweep boundaries) and `ScanWorkItem` (a frozen worklist so `scan_chunk` pops by indexed cursor instead of re-walking the tree each chunk). All worker DB access is raw psycopg SQL. + +**Tech Stack:** Next.js (App Router, TypeScript, vitest, node test env), Prisma (migration source of truth), Python worker (psycopg, pytest against `lyra_test`). + +## Global Constraints + +- Tests run against `lyra_test` only; a guard refuses non-`*_test` DBs. Never point at live `lyra`. +- Web vitest env is **node, not jsdom** — no DOM-render tests. UI-only changes (aria-label, copy) are verified by `tsc` + build + Playwright, not unit tests. +- Preserve existing `aria-label`s / button text — web route tests and Playwright key off them. +- Worker SQL uses quoted PascalCase table/column identifiers (`"DiscoverySuggestion"`, `"seedMbid"`). +- Discovery `_upsert_artist`/`_upsert_album` MUST keep their `WHERE "DiscoverySuggestion".status = 'pending'` guard (dismissed/wanted rows are never revived). +- Frequent commits: one per task minimum, following the TDD step order. +- Migration reaches live `lyra` via the web entrypoint's `migrate deploy` on rebuild; no `down -v`, ever. + +--- + +## File Structure + +**Tier A (web):** +- `web/src/app/api/monitor/config/route.ts` — PATCH allowlist → `hasOwnProperty`. +- `web/src/app/api/discover/config/route.ts` — PATCH allowlist → `hasOwnProperty`. +- `web/src/app/api/monitor/config/route.test.ts` — prototype-chain rejection test. +- `web/src/app/api/discover/config/route.test.ts` — prototype-chain rejection test. +- `web/src/app/api/config/route.test.ts` — add `constructor` case. +- `web/src/app/settings/settings-form.tsx` — Monitor toggle `aria-label` + eyebrow copy. + +**Tier B (schema + worker):** +- `web/prisma/schema.prisma` — add both models. +- `web/prisma/migrations/_add_contributions_and_scan_worklist/migration.sql` — CREATE TABLE ×2. +- `worker/tests/conftest.py` — clean the two new tables between tests. +- `worker/lyra_worker/discovery.py` — contributions write + recompute. +- `worker/tests/test_discovery.py` — new cross-chunk / partial-sweep / drop-off tests. +- `worker/lyra_worker/scan.py` — `build_worklist` / new `scan_chunk` / `clear_worklist` / reimplemented `scan_library`. +- `worker/lyra_worker/main.py` — `maybe_run_scan` drives the worklist by `scan.id`. +- `worker/tests/test_scan_chunk.py` — rewritten to the worklist API. + +--- + +## Task 1: Tier A1 + A3 — PATCH allowlist hardening + tests + +**Files:** +- Modify: `web/src/app/api/monitor/config/route.ts:26-28` +- Modify: `web/src/app/api/discover/config/route.ts:28-30` +- Test: `web/src/app/api/monitor/config/route.test.ts` +- Test: `web/src/app/api/discover/config/route.test.ts` +- Test: `web/src/app/api/config/route.test.ts` (add one case) + +**Interfaces:** +- Consumes: nothing new. +- Produces: nothing consumed by later tasks. + +**Background:** Both PATCH routes filter with `([k]) => k in DEFAULTS`, which walks the prototype chain, so a body key of `__proto__` or `constructor` passes the filter and writes a junk `Config` row (harmless — no delete — but inconsistent with the DELETE handler at `web/src/app/api/config/route.ts:51`, which already uses `Object.prototype.hasOwnProperty.call(...)`). + +- [ ] **Step 1: Write the failing test — monitor PATCH rejects prototype-chain keys** + +Append to `web/src/app/api/monitor/config/route.test.ts` (mirror the file's existing PATCH test style — read the top of the file for its `patchReq` helper; if none exists, add one like below): + +```ts +function patchReq(body: unknown) { + return new Request("http://localhost/api/monitor/config", { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +it("ignores prototype-chain keys (__proto__, constructor) and writes no Config row", async () => { + const before = await prisma.config.count(); + const res = await PATCH(patchReq({ __proto__: "x", constructor: "y" })); + expect((await res.json()).updated).toBe(0); + expect(await prisma.config.count()).toBe(before); +}); +``` + +Ensure `PATCH` and `prisma` are imported at the top of the test file (`import { GET, PATCH } from "./route";` and `import { prisma } from "@/lib/db";`). + +- [ ] **Step 2: Run it to verify it fails** + +Run: `cd web && npx vitest run src/app/api/monitor/config/route.test.ts` +Expected: FAIL — `updated` is `2` (or a Config row was written), because `__proto__`/`constructor` pass `k in DEFAULTS`. + +- [ ] **Step 3: Harden both PATCH routes** + +In `web/src/app/api/monitor/config/route.ts` change the filter: + +```ts + const entries = Object.entries((body ?? {}) as Record).filter( + ([k]) => Object.prototype.hasOwnProperty.call(DEFAULTS, k), + ); +``` + +Apply the identical change in `web/src/app/api/discover/config/route.ts`. + +- [ ] **Step 4: Add the mirror test for discover PATCH** + +Append the same test (with a `patchReq` pointing at `/api/discover/config` and importing this route's `PATCH`) to `web/src/app/api/discover/config/route.test.ts`. + +- [ ] **Step 5: Add the `constructor` case to the DELETE allowlist test** + +In `web/src/app/api/config/route.test.ts`, the test at line 85 ("rejects prototype-chain field names") only exercises `__proto__`. Add `constructor` alongside it: + +```ts + for (const bad of ["__proto__", "constructor"]) { + const res = await DELETE(delReq(bad)); + expect(res.status).toBe(400); + } + // Must not have collapsed to deleteMany({ where: {} }) and wiped the table. + expect(await prisma.config.count()).toBe(before); + expect(await prisma.config.findUnique({ where: { key: "qobuz.password" } })).not.toBeNull(); +``` + +(Replace the existing single `__proto__` DELETE block; keep the surrounding `before`/PUT setup.) + +- [ ] **Step 6: Run all three affected test files to verify they pass** + +Run: `cd web && npx vitest run src/app/api/monitor/config/route.test.ts src/app/api/discover/config/route.test.ts src/app/api/config/route.test.ts` +Expected: PASS. + +- [ ] **Step 7: Commit** + +```bash +git add web/src/app/api/monitor/config/route.ts web/src/app/api/discover/config/route.ts \ + web/src/app/api/monitor/config/route.test.ts web/src/app/api/discover/config/route.test.ts \ + web/src/app/api/config/route.test.ts +git commit -m "fix(web): harden config PATCH allowlists against prototype-chain keys" +``` + +--- + +## Task 2: Tier A2 — Monitor toggle aria-label + Settings eyebrow copy + +**Files:** +- Modify: `web/src/app/settings/settings-form.tsx:159` (eyebrow) and `:273-277` (toggle) + +**Interfaces:** +- Consumes: nothing. +- Produces: nothing. + +No unit test (node vitest env can't render this component; verified by build + Playwright later). + +- [ ] **Step 1: Add an aria-label to the Monitor enable checkbox** + +At `web/src/app/settings/settings-form.tsx:273`, add `aria-label` to the `` inside the Monitor `