diff --git a/.graphifyignore b/.graphifyignore index 2ade0ae0..74cd6a46 100644 --- a/.graphifyignore +++ b/.graphifyignore @@ -45,6 +45,26 @@ cmd/migration-check/testdata/ # Frontend tooling config -- one node per JSON key, no architecture signal web/tsconfig.json web/components.json +web/package.json + +# Tests and generated code: ~60% of graph edges were test plumbing (testing.T, +# NewTestPool) and sqlc output, drowning real architecture. Source is findable by name. +*_test.go +*.test.ts +*.test.tsx +internal/db/sqlc/ + +# Per-plan execution logs for every phase; CONTEXT/RESEARCH/UI-SPEC keep the decisions +.planning/phases/*/*-PLAN.md +.planning/phases/*/*-SUMMARY.md +.planning/phases/*/*-REVIEW.md +.planning/phases/*/*-REVIEW-FIX.md +.planning/phases/*/*-UAT.md +.planning/phases/*/*-PATTERNS.md +.planning/phases/*/*-VERIFICATION.md +.planning/phases/*/*-SECURITY.md +.planning/phases/*/*-VALIDATION.md +.planning/phases/*/*-DISCUSSION-LOG.md # Maintenance: when a milestone completes, add that milestone's phase # directory under .planning/milestones/ to this list before the next diff --git a/.planning/REQUIREMENTS.md b/.planning/REQUIREMENTS.md index 87099f25..6a85496c 100644 --- a/.planning/REQUIREMENTS.md +++ b/.planning/REQUIREMENTS.md @@ -35,11 +35,11 @@ ### Bulk Edit -- [ ] **BULK-01**: User can select multiple artists on the Watchlist, including "select all visible" -- [ ] **BULK-02**: User can add or remove a tag on all selected artists in one action -- [ ] **BULK-03**: User can set release-type filters and muted event types on all selected artists in one action -- [ ] **BULK-04**: User can remove all selected artists after a confirmation stating the count; the removal is all-or-nothing -- [ ] **BULK-05**: After a bulk action the selection clears and the watchlist reflects the result without a page reload +- [x] **BULK-01**: User can select multiple artists on the Watchlist, including "select all visible" +- [x] **BULK-02**: User can add or remove a tag on all selected artists in one action +- [x] **BULK-03**: User can set release-type filters and muted event types on all selected artists in one action +- [x] **BULK-04**: User can remove all selected artists after a confirmation stating the count; the removal is all-or-nothing +- [x] **BULK-05**: After a bulk action the selection clears and the watchlist reflects the result without a page reload ### Bulk Add @@ -96,11 +96,11 @@ Which phases cover which requirements. Updated during roadmap creation. | WLVW-05 | Phase 25 | Complete | | WLVW-06 | Phase 25 | Complete | | HIST-02 | Phase 25 | Complete | -| BULK-01 | Phase 26 | Pending | -| BULK-02 | Phase 26 | Pending | -| BULK-03 | Phase 26 | Pending | -| BULK-04 | Phase 26 | Pending | -| BULK-05 | Phase 26 | Pending | +| BULK-01 | Phase 26 | Complete | +| BULK-02 | Phase 26 | Complete | +| BULK-03 | Phase 26 | Complete | +| BULK-04 | Phase 26 | Complete | +| BULK-05 | Phase 26 | Complete | | IMPT-01 | Phase 27 | Pending | | IMPT-02 | Phase 27 | Pending | | IMPT-03 | Phase 27 | Pending | diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index f28dc47f..c7cfd4ac 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -89,7 +89,7 @@ Full phase-by-phase detail for every shipped milestone is archived under `.plann - [x] **Phase 24: Artist Tags & Notes** - Free-form, case-insensitive tags with autocomplete and a plain-text note on each Watchlist card, global tag rename/merge/delete, and tags that belong to the artist so they survive a remove and re-add (completed 2026-10-06) - [x] **Phase 25: Find & Filter — Watchlist and History** - Client-side name search, stable sorts (including latest release), and tag/preference filters over the fully loaded watchlist, plus a retention-aware tag filter on the History feed (completed 2026-10-12) -- [ ] **Phase 26: Bulk Edit & Remove** - Multi-select on the Watchlist with one-action tag add/remove, preference changes, and a count-confirmed all-or-nothing remove, backed by set-based transactional endpoints +- [x] **Phase 26: Bulk Edit & Remove** - Multi-select on the Watchlist with one-action tag add/remove, preference changes, and a count-confirmed all-or-nothing remove, backed by set-based transactional endpoints (completed 2026-10-19) - [ ] **Phase 27: Paste-a-List Bulk Add** - Paste names one per line, watch them resolve one search at a time within the existing rate limits, review best matches by confidence, and add the confirmed set in one request - [ ] **Phase 28: Tags on Discord Notifications** - Tags on real-time embeds and digest lines, with markdown escaping added to the real-time path and the digest's chunking guarantees re-proven against longer lines @@ -284,7 +284,48 @@ Plans: 4. Removing the selection first asks for confirmation stating how many artists will be removed. Confirming removes all of them, or none of them if any part fails. 5. After any bulk action the selection clears and the Watchlist shows the result immediately, with no page reload and no checkbox left pointing at a removed artist. -**Plans**: TBD +**Plans:** 12/12 plans complete + +Plans: +**Wave 1** + +- [x] 26-01-PLAN.md — `POST /watchlist/bulk/delete` as one atomic DELETE (D-03), the `WatchlistBulkStore`/`WithWatchlistBulk` seam (D-20), shared id and body bounds, and gate/CSRF/503/precedence tests + +**Wave 2** *(blocked on Wave 1 completion)* + +- [x] 26-02-PLAN.md — `POST /watchlist/bulk/tags/add`: an ordered pre-lock plus a count-guarded insert that names at-cap artists and rolls back an orphan tag (D-01, D-19), concurrency tests, and the ADR 0004 amendment +- [x] 26-03-PLAN.md — Card checkboxes, the sticky action bar, and the count-stating route-level remove confirm with its copy and failure paths (D-09, D-10, D-16, D-17) + +**Wave 3** *(blocked on Wave 2 completion)* + +- [x] 26-04-PLAN.md — `Select all visible`, the derived (not trimmed) selection with hidden selections inert, Shift ranges, and the in-flight lock (D-09, D-11) +- [x] 26-05-PLAN.md — `PATCH /watchlist/bulk/preferences`: a locked-CTE per-type delta in canonical order with the nil-slice guard and the D-21 400s + +**Wave 4** *(blocked on Wave 3 completion)* + +- [x] 26-06-PLAN.md — Close → clear → focus fallback (U1), selection announcements and Esc, and the measured bar clearance for the page, toasts, and focus (U2, U3) +- [x] 26-07-PLAN.md — `POST /watchlist/bulk/tags/remove` by tag id with a 404 for a missing tag (D-22) and an `applied`/`carried` outcome for Undo + +**Wave 5** *(blocked on Wave 4 completion)* + +- [x] 26-08-PLAN.md — Bulk add-tag from the bar's inline picker (D-12), with named cap skips, gone notes, and sticky cards (D-13) + +**Wave 6** *(blocked on Wave 5 completion)* + +- [x] 26-09-PLAN.md — The entry-free `TagCombobox` shared by card and bar, and the inline add-tag mode's Esc/Cancel, pending, and vocabulary contract (D-12) + +**Wave 7** *(blocked on Wave 6 completion)* + +- [x] 26-10-PLAN.md — The vendored popover and the tri-state Preferences editor, which sends only changed types (D-05…D-08) + +**Wave 8** *(blocked on Wave 7 completion)* + +- [x] 26-11-PLAN.md — Bulk remove-tag from the bar (carried tags with counts) and Undo through bulk add-tag (U4) + +**Wave 9** *(blocked on Wave 8 completion)* + +- [x] 26-12-PLAN.md — Embedded bundle rebuild, the phase-closing Definition of Done (`make db-up` + `make test`, or their recipes), and the blast-radius gate + **UI hint**: yes **Notes for the phase planner** @@ -363,10 +404,23 @@ Phases execute in numeric order: 24 → 25 → 26 → 27 → 28 |-------|----------------|--------|-----------| | 24. Artist Tags & Notes | 14/14 | Complete | 2026-10-06 | | 25. Find & Filter — Watchlist and History | 21/21 | Complete | 2026-10-12 | -| 26. Bulk Edit & Remove | 0/? | Not started | - | +| 26. Bulk Edit & Remove | 12/12 | Complete | 2026-10-19 | | 27. Paste-a-List Bulk Add | 0/? | Not started | - | | 28. Tags on Discord Notifications | 0/? | Not started | - | ## Backlog -*(none currently)* +### Phase 999.1: Terraform + Azure ephemeral deploy (BACKLOG) + +**Goal:** Terraform-managed Azure environment (Container Apps + Postgres Flexible Server) run as an apply -> smoke test -> destroy cycle, locally first, then from GitHub Actions. Gives the deferred Phase 17 deploy/health-gate work a target without a VPS or domain. + +**Plans**: TBD (proposed slices) + +- (a) Bootstrap module: state storage account + GitHub OIDC app registration, never destroyed +- (b) Root module: resource group, Container App, Postgres Flexible Server, budget alert; applied locally +- (c) Smoke-test script: `/health` + `/ready`, then POST /watchlist and re-GET +- (d) CI: `terraform fmt`/`validate`, tflint, config scan, plan comment on PRs +- (e) `workflow_dispatch` cycle: apply, smoke test, destroy under `if: always()`, image tag from the release job +- (f) README/ADR covering cost and teardown + +Context: `.planning/notes/2026-10-17-terraform-azure-ephemeral-deploy.md`; open questions in `.planning/research/questions.md`. diff --git a/.planning/STATE.md b/.planning/STATE.md index c555a4c7..32a25d3b 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -2,20 +2,20 @@ gsd_state_version: "1.0" milestone: v1.6 milestone_name: Watchlist Organization -current_phase: 26 -current_phase_name: Bulk Edit & Remove +current_phase: 27 +current_phase_name: Paste-a-List Bulk Add status: planning -stopped_at: Phase 25 complete, ready to plan Phase 26 -last_updated: "2026-10-13T02:17:52.713Z" -last_activity: 2026-10-12 -last_activity_desc: Phase 25 complete, transitioned to Phase 26 -state_head: aa235f1e00be1b78be6cbea284f996b550c2fd43 +stopped_at: Phase 26 complete, ready to plan Phase 27 +last_updated: "2026-10-19T18:12:58.677Z" +last_activity: 2026-10-19 +last_activity_desc: Phase 26 complete, transitioned to Phase 27 +state_head: ad1cd5326e7e95181f3651ffae7d5644d0d39e38 progress: total_phases: 5 - completed_phases: 2 - total_plans: 35 - completed_plans: 35 - percent: 40 + completed_phases: 3 + total_plans: 47 + completed_plans: 47 + percent: 60 --- # Project State @@ -25,20 +25,20 @@ progress: See: .planning/PROJECT.md (updated 2026-10-06) **Core value:** A single Go binary that reliably detects and notifies on new releases for watched artists, built and shipped through a CI/CD pipeline rigorous enough to demonstrate real DevOps practice. -**Current focus:** Phase 25 — Find & Filter — Watchlist and History +**Current focus:** Phase 26 — Bulk Edit & Remove ## Current Position -Phase: 26 — Bulk Edit & Remove +Phase: 27 — Paste-a-List Bulk Add Plan: Not started Status: Ready to plan -Last activity: 2026-10-12 — Phase 25 complete, transitioned to Phase 26 +Last activity: 2026-10-19 — Phase 26 complete, transitioned to Phase 27 ## Performance Metrics **Velocity:** -- Total plans completed: 134 +- Total plans completed: 146 - Average duration: - min - Total execution time: 0 hours @@ -71,6 +71,7 @@ Last activity: 2026-10-12 — Phase 25 complete, transitioned to Phase 26 | 23 | 4 | - | - | | 24 | 14 | - | - | | 25 | 21 | - | - | +| 26 | 12 | - | - | **Recent Trend:** @@ -184,6 +185,18 @@ Last activity: 2026-10-12 — Phase 25 complete, transitioned to Phase 26 | Phase 25 P19 | 2 min | 2 tasks | 4 files | | Phase 25 P20 | 10 min | 3 tasks | 7 files | | Phase 25 P21 | 9 min | 3 tasks | 4 files | +| Phase 26 P01 | 14 min | 3 tasks | 9 files | +| Phase 26 P02 | 14min | 3 tasks | 10 files | +| Phase 26 P03 | 8 min | 2 tasks | 8 files | +| Phase 26 P04 | 10min | 3 tasks | 5 files | +| Phase 26 P05 | 12min | 3 tasks | 8 files | +| Phase 26 P06 | 12min | 3 tasks | 8 files | +| Phase 26 P07 | 12 min | 2 tasks | 9 files | +| Phase 26 P08 | 11 min | 2 tasks | 8 files | +| Phase 26 P09 | 14min | 2 tasks | 6 files | +| Phase 26 P10 | 35 min | 3 tasks | 9 files | +| Phase 26 P11 | 40min | 3 tasks | 9 files | +| Phase 26 P12 | 7 min | 2 tasks | 23 files | ## Accumulated Context @@ -429,6 +442,18 @@ Recent decisions affecting current work: - [Phase 25]: [Phase 25-19]: G-25-11 scroll anchor fallback widened to the nearest surviving card (no distance cap, next before previous at each distance), user decision 2026-10-12 - [Phase 25]: G-25-12 (2026-10-12): keyboard-only focus move (click detail 0) to fallback card's chip label, else Your artists heading, via useScrollAnchor onFocusLost/useEffectEvent - [Phase 25]: G-25-13 (user decision 2026-10-12): disabled Deezer button described via aria-describedby to a per-row useId sr-only hint; no visual change +- [Phase 26]: 26-01: bulk delete is POST /watchlist/bulk/delete, max 1000 ids, 64 KiB body cap; max returned in the 400 body — Keeps the SPA independent of the constants; watchlist rows only, artists/tags/pending events survive (D-17, D-18) +- [Phase 26]: [Phase 26-02] Bulk add-tag lives on tags.Service/TagStore; all-capped or all-gone batches roll back and report tag id 0 so no orphan tags row +- [Phase 26]: 26-03: confirmRemove records the outcome in a ref; the dialog close handler applies it (close, then clear) so the route-hosted dialog never unmounts with the bar +- [Phase 26]: 26-04: Select all visible name comes from its wrapping label text only; disabled Base UI checkboxes expose data-disabled +- [Phase 26]: 26-05: bulk preferences is PATCH /watchlist/bulk/preferences; omitted axis means empty lists; validation precedes empty-ids shortcut +- [Phase 26]: 26-06: finalFocus function reads a close-kind ref so focus target is chosen at close; pendingFocus effect is a backstop +- [Phase 26]: 26-07: bulk remove-tag lives on tags.Service/TagStore with no transaction; missing tag is 404, applied is exactly what Undo re-adds +- [Phase 26]: 26-08: bar picker onClose ignored while pending; Cancel exits on pointer-down and click; remember(tag) only when applied is non-empty +- [Phase 26]: [26-09] Vocabulary memory in bar tests is observed via the reopened picker, not a provider spy +- [Phase 26]: 26-10: popover.tsx hand-written from registry source; shadcn CLI tried to pnpm add a bogus cn package +- [Phase 26]: 26-10: Add tag disabled while Preferences popover is open (one-surface rule) +- [Phase 26]: 26-11: Remove-tag Undo re-adds by name to the remove response's applied ids only; picker owns its query and filters with filter={null}; DISMISS_REASONS shared from TagCombobox ### Pending Todos @@ -489,6 +514,7 @@ _Closed 2026-09-05: Phase 16 gap G-16-1 (n1-boot guard-adoption skip) — quick | 261007-lc4 | Phase 24 comment cleanup: comment-only trim to 1-3 line why-notes with one doc ref, fixed inaccurate WithTags/merge-trigger/buildTagSuggestions comments, removed plan narration | 2026-10-07 | be98a62 | [261007-lc4-phase-24-comment-cleanup](./quick/261007-lc4-phase-24-comment-cleanup/) | | 261011-gq7 | Amend Phase 26 CONTEXT and UI-SPEC with grill decisions Q1–Q10 (derived selection, outcome-only bulk responses + refresh, ordered-lock bulk tag attach, single-statement remove/preferences, pending-alert disclosure) and add withdraw-pending-events todo | 2026-10-11 | 6408d28 | [261011-gq7-amend-phase-26-docs-with-grill-decisions](./quick/261011-gq7-amend-phase-26-docs-with-grill-decisions/) | | 261011-moj | Amend Phase 26 CONTEXT and UI-SPEC with UI review U1–U4 (route-level confirm + close→clear→focus order, scroll-padding focus clearance, one-row phone bar, remove-tag Undo) | 2026-10-11 | 9ac478c | [261011-moj-amend-phase-26-ui-spec-and-context-with-](./quick/261011-moj-amend-phase-26-ui-spec-and-context-with-/) | +| 261019-jkv | Refactor phase 26 bulk UI: shared preference options, popover and outcome-messages modules, useBulkSelection hook | 2026-10-19 | 66e1990 | [261019-jkv-refactor-phase-26-bulk-ui-preferenceopti](./quick/261019-jkv-refactor-phase-26-bulk-ui-preferenceopti/) | ### Roadmap Evolution @@ -524,8 +550,8 @@ Items acknowledged and carried forward from previous milestone close: ## Session Continuity -Last session: 2026-10-12T22:25:32.998Z -Stopped at: Phase 25 complete, ready to plan Phase 26 +Last session: 2026-10-16T23:59:43.847Z +Stopped at: Phase 26 complete, ready to plan Phase 27 Resume file: None ## Operator Next Steps diff --git a/.planning/debug/knowledge-base.md b/.planning/debug/knowledge-base.md index 1e1f5126..24b1ac1c 100644 --- a/.planning/debug/knowledge-base.md +++ b/.planning/debug/knowledge-base.md @@ -33,3 +33,13 @@ A match here is a **hypothesis candidate, not a diagnosis** — test it first, b - **Why not caught:** No gate existed for this class, and one gate actively **pinned the bug as intended behaviour**. `internal/detection/detector_test.go:TestDetector_SecondCycleLeavesNotifiedAtNull` asserted, in its name, exactly the mechanism that produced the flood — correct when written (seed-mode *was* the whole design and detection was single-cycle), silently obsolete the moment phase 13 made the guest-feature pass multi-cycle. Phase 13's own tests were green and *could not* have caught it: they verified the cap defers work correctly, which it does. Nothing tested the **interaction** between "detection is now multi-cycle" and "suppression is single-cycle" — the two contracts live in different files (`musicbrainz.go`'s cap, `detector.go`'s latch) and neither's tests knew the other existed. `notifier.suppresses()`' surrounding delivery decision had zero release-age coverage because no such concept existed in the notify path. `go vet`/`golangci-lint` cannot see a semantic contract conflict between two files. Phase 13's verify/UAT step asked "does the cap work, does polling stay under rate limits" — the right questions for that change — not "does an artist with >20 guest features now leak their back catalogue", because the symptom only manifests hours later, in production, for large-catalogue artists, through an **external side effect (Discord) that no test observes**. The general lesson: when a change converts a previously single-pass operation into a multi-pass one, every invariant that silently assumed "one pass" becomes a latent bug, and existing green tests asserting the old timing-based behaviour are the least likely thing anyone thinks to re-read. - **Recurrence guard:** Real automated gates, all mutation-verified (each was confirmed to fail when the fix is reverted, not merely to pass when it is present). Primary: `internal/detection/notifygate_test.go:TestDetectGuestFeatures_PastLookupCap_BacklogNeverGoesPending` — runs against real Postgres on an artist explicitly **out** of seed mode and past `maxNewGuestFeatureLookupsPerCycle`, the exact condition the old code had no defence for, asserting zero rows go pending; reverting the insert-side gate fails it with `"back-catalogue guest features queued for Discord delivery = 20, want 0"`, reproducing the precise production drip inside the suite. Supporting: `TestDetectMusicBrainz_NonSeedCycle_Backlog...` (same class, new_release pass); `TestNotifyGate_NotifiedAt`'s 13-case seed/freshness truth table mirrored case-for-case by `internal/notifier/suppress_test.go:TestStaleReleaseDate` against a fixed cutoff, so the two halves of the gate are provably in agreement and **cannot drift apart** — which is what would silently re-open the AND-gate; `TestStaleReleaseDate/absent_date_(SQL_NULL)_is_suppressed` pins the undated-suppresses decision specifically (restoring "undated = deliver" fails it); `internal/notifier/notifier_test.go:TestNotifyPending_StaleRowsAckedWithoutSending` models the production outbox at ship time (backlog + undated + partial-date + one fresh row) and asserts exactly one Discord request plus that all four rows are acked — an unacked stale row would be re-suppressed forever; disabling the delivery-side gate fails it with 4 requests, the captured bodies literally showing a 2015 release being posted. Runtime observability guard: the `"notify pass suppressed stale events"` per-pass Info summary makes over-suppression — the one real risk this gate carries — visible in production instead of silent. Plus this KB entry, for the transferable pattern **"a per-cycle cap or deferral added to a pass whose correctness invariant assumed a single cycle"**, and the sharper diagnostic **"suppression keyed on *when* a row was inserted rather than *what* the row is"** — a distinction to reach for on any 'used to work, started recently, only affects large/slow datasets' symptom. Known accepted narrowing, flagged not fixed: `detectDeluxeChanges` is bounded by a 90-day recheck window on the group's first-release date while the notify gate applies the tighter 7-day window to the winning release's date, so a deluxe edition of an 8–90-day-old album is delivered only if MusicBrainz dates the deluxe release itself recently. Zero `deluxe_change` rows have ever existed in production so this narrows nothing observable; exempting the event type is a product decision, deliberately out of scope here, documented in code at the deluxe insert site and to be revisited if `deluxe_change` starts firing and alerts go missing. --- + +## shift-click-range-select — Shift+click selected only the clicked card because a keydown reset cleared the "Shift held" flag on auto-repeat +- **Date:** 2026-10-19 +- **Error patterns:** no error message, shift+click range selects only one item, modifier flag lost between pointerdown and click, shiftRef cleared, onKeyDownCapture, key auto-repeat, held Shift, tests green but real browser fails, user-event never repeats keydown +- **Root cause(s):** `onKeyDownCapture` on the card label in `WatchlistRow.tsx` reset `shiftRef` on every keydown, including the repeated Shift keydown while Shift is held. Focus moves to the checkbox inside its own label on mousedown, so the repeat reached the label and cleared the flag before the change handler read it. +- **Fix:** keydown reset skips modifier keys (Shift/Control/Alt/Meta); Space/Enter still clear a stale pointer flag. +- **Files changed:** web/app/components/watchlist/WatchlistRow.tsx, web/app/routes/watchlist.bulk.test.tsx +- **Why not caught:** user-event never emits repeated keydown events, so the suite could not reproduce the held-key path. +- **Recurrence guard:** `watchlist.bulk.test.tsx` "keeps the range when the held Shift auto-repeats mid-click" (red before fix). Pattern: any flag set on pointerdown and cleared on keydown must ignore modifier keys. +--- diff --git a/.planning/debug/resolved/shift-click-range-select.md b/.planning/debug/resolved/shift-click-range-select.md new file mode 100644 index 00000000..23ed19f2 --- /dev/null +++ b/.planning/debug/resolved/shift-click-range-select.md @@ -0,0 +1,99 @@ +--- +status: awaiting_human_verify +trigger: "shift+click range selection selects only clicked card (phase 26 UAT test 2)" +created: 2026-10-19T00:00:00Z +updated: 2026-10-19T01:00:00Z +goal: find_and_fix +--- + +## Current Focus + +bug_class: Mandelbug (input-timing dependent; green in jsdom, fails in a real browser when the OS auto-repeats the held Shift keydown) +hypothesis: CONFIRMED. WatchlistRow's label onKeyDownCapture resets shiftRef on ANY keydown, including the auto-repeating keydown of the held Shift key. After mousedown on the checkbox span focuses it (inside the target label), a repeated Shift keydown bubbles through the target label's capture handler and clears shiftRef before the click fires, so handleSelectChange gets shift=false and toggles only the clicked id. +reasoning_checkpoint: + hypothesis: "Resetting shiftRef on every keydown (including modifier keydowns, notably the auto-repeating Shift keydown) loses the pointer-down Shift flag between pointerdown and click, so a Shift+click is handled as a plain toggle" + confirming_evidence: + - "Real Chrome via CDP, real route: with a repeated Shift keydown (repeat=true) between mousePressed and mouseReleased on the checkbox hit area, console showed 'keydown 5 Shift true' then 'checkedChange 5 true false' (shift lost) and selection stayed at 2 of the expected 4" + - "Same run without the repeated keydown (A, B, C, E): 'pointerdown 5 true' -> 'checkedChange 5 true true' -> range of 4 selected; label-margin path and span path both work, anchor and applyRange are correct" + - "Windows re-sends keydown for a held Shift (JDK bug write-up cited in search); user is on Windows 11; jsdom user-event never auto-repeats so the suite stayed green" + falsification_test: "Fix the reset to ignore modifier keydowns; the same CDP scenario D must then select the 4-card range, and a jsdom test that fires a repeat Shift keydown between pointer press and release must go red->green" + fix_rationale: "The keydown reset exists to drop a stale pointer flag before a keyboard toggle (Space/Enter). Modifier keys never activate the checkbox, so skipping Shift/Control/Alt/Meta keeps that protection and stops the held Shift from erasing its own flag" + blind_spots: "Could not inject real OS typematic repeat (SendInput does not auto-repeat), so the repeat premise rests on the CDP-simulated autoRepeat keydown plus the Windows behavior reference; Firefox/Safari not exercised" + candidate_causes: + - "code: keydown reset clobbers the Shift flag (confirmed)" + - "code: anchor lost / applyRange wrong (eliminated: anchor=3 and applyRange spans correctly in the non-repeat runs)" + - "environment: label-synthesized click drops shiftKey (eliminated in Chrome: INPUT click carried shiftKey=true on both label and span paths)" + - "data: visible order differs from id order (not a bug; sorted by name, range follows visible order)" + and_gate: "no - a single condition (repeated Shift keydown reaching the target label while the pointer is down) is sufficient; the margin path is unaffected only because focus never lands inside the label" +next_action: user re-runs Phase 26 UAT test 2 in their real browser (hold Shift, click the card checkbox area three rows below an anchor); on "confirmed fixed" commit the two web files, archive this session, append the knowledge-base entry + +## Symptoms + +expected: Click one card checkbox, Shift+click a card three rows down on the 44px label area -> inclusive range in visible order is selected. After a sort change the anchor resets and only the clicked card toggles. +actual: Shift+click selects only the clicked box, not the boxes between. (Part B, retained prior selection after sort change, is by design.) +errors: none reported +timeline: found in phase 26 UAT (.planning/phases/26-*/26-UAT.md test 2); 26-04 backstop +reproduction: Watchlist page; click a card checkbox, Shift+click a card a few rows down via the label area + +## Eliminated + +- hypothesis: applyRange / anchor handling in handleSelectChange is wrong + evidence: in real Chrome the non-repeat runs logged handleSelectChange(5, true, shift=true, anchor=3, sel="3") and selected the full 4-card range; anchorRef survives between clicks + timestamp: 2026-10-19T00:25:00Z + +- hypothesis: the label's synthesized click path (click on the 44px label margin) loses Shift + evidence: CDP scenario A/C/E (mouse on label margin): pointerdown shift=true, checkedChange shift=true, range selected; the synthesized INPUT click also carried shiftKey=true in Chrome + timestamp: 2026-10-19T00:25:00Z + +- hypothesis: my first CDP "failure" (only 2 selected) was a product bug + evidence: harness error - I picked the 4th artist by mock-array order, but the list is sorted by name, so Drake and Kendrick were adjacent; re-run using visible DOM order selected the 4-card range + timestamp: 2026-10-19T00:22:00Z + +## Evidence + +- timestamp: 2026-10-19T00:05:00Z + checked: web/app/lib/bulkSelection.ts applyRange + bulkSelection.test.ts + found: applyRange is a correct pure inclusive-span helper; unit tests cover forward/backward/no-anchor/unknown-anchor + implication: pure logic is not the fault; the shift flag or anchor is lost upstream of applyRange + +- timestamp: 2026-10-19T00:06:00Z + checked: web/app/routes/watchlist.tsx handleSelectChange (L529-548), anchorRef effects (L252-259), filterKeyOf (L92) + found: shift -> applyRange(order, anchorRef.current, id, next, selectedIds); anchor set to id after every change; anchor reset only on filterKey/query change or empty selection; filterKeyOf returns a string so the effect is stable across renders + implication: anchor handling looks right for the described flow; shift=false would produce exactly the reported symptom (only clicked id toggles) + +- timestamp: 2026-10-19T00:07:00Z + checked: web/app/components/watchlist/WatchlistRow.tsx L46-77 + found: shiftRef set on label onPointerDownCapture; label onKeyDownCapture sets shiftRef=false on ANY keydown; onCheckedChange reads+clears shiftRef + implication: pressing/holding Shift is itself a keydown; auto-repeat of a held Shift fires keydown (repeat=true) on the focused element, which after mousedown is the target's own checkbox span inside the label + +- timestamp: 2026-10-19T00:08:00Z + checked: web/app/routes/watchlist.bulk.test.tsx shiftClick helper (L377-382) + found: test clicks the role=checkbox span via user-event with {Shift>} held; user-event never emits auto-repeat keydown + implication: jsdom test cannot see the repeat-keydown path, which explains why the suite is green while UAT fails + +- timestamp: 2026-10-19T00:09:00Z + checked: node_modules/@base-ui/react/checkbox/root/CheckboxRoot.js (v1.7.0) + found: span onClick -> preventDefault + dispatchClickWithModifiers(input, event); input onChange fires from the native click, details.event = nativeEvent + implication: the click that reaches onCheckedChange carries shiftKey in Chrome; the pointerdown ref is a belt-and-braces path + +- timestamp: 2026-10-19T00:24:00Z + checked: real Chrome 155 (headless, CDP) against vite dev server with CDP-mocked API (port 5199, real Go server on 8080 untouched), temporary console.log in WatchlistRow/handleSelectChange (reverted) + found: scenario A (label margin), B (span hit area), C (repeat keydown before press), E (label margin + repeat mid-click) -> 4 selected. Scenario D (span hit area + repeat Shift keydown between press and release) -> logs 'pointerdown 5 true', 'keydown 5 Shift true' x2, 'checkedChange 5 true false', selected = 2 (anchor + clicked only) + implication: root cause reproduced in a real browser; focus lands inside the target label only on the span path, so repeat keydowns reach the label's capture handler there + +## Resolution + +root_cause: WatchlistRow's label onKeyDownCapture clears shiftRef on every keydown, including the OS auto-repeat keydown of the held Shift key. After mousedown focuses the checkbox (inside the label), a repeated Shift keydown wipes the pointer-down Shift flag before click, so onCheckedChange reports shift=false and handleSelectChange toggles only the clicked card. +fix: WatchlistRow label onKeyDownCapture now clears shiftRef only for non-modifier keys (MODIFIER_KEYS = Shift/Control/Alt/Meta). Space/Enter still void a stale pointer flag; a held Shift no longer erases its own flag. Left uncommitted in the working tree. +verification: | + guardrail (manual, no committed mutation tooling run): + - red/green: new test 'keeps the range when the held Shift auto-repeats mid-click' failed before the fix ('3 selected' not found) and passes after + - mutation guard: removing the keydown reset entirely makes 'treats a keyboard toggle as plain after an abandoned Shift press' fail, so the reset is still needed and still covered + - real Chrome 155 via CDP, same 5 scenarios before/after: D (span hit area + repeat Shift keydown mid-click) 2 selected -> 4 selected; A/B/C/E unchanged at 4 + - prettier --check clean, vitest 34 files / 782 tests pass, tsc (pnpm typecheck) clean + - not run: Go gates (no backend change), real OS typematic repeat (SendInput does not auto-repeat), Firefox/Safari + guardrail_verdict: accepted pending human verify + oracle_type: specified (UAT test 2 expected: inclusive range in visible order) +files_changed: + - web/app/components/watchlist/WatchlistRow.tsx + - web/app/routes/watchlist.bulk.test.tsx diff --git a/.planning/notes/2026-10-17-terraform-azure-ephemeral-deploy.md b/.planning/notes/2026-10-17-terraform-azure-ephemeral-deploy.md new file mode 100644 index 00000000..6dac754a --- /dev/null +++ b/.planning/notes/2026-10-17-terraform-azure-ephemeral-deploy.md @@ -0,0 +1,26 @@ +--- +title: Terraform + Azure ephemeral deploy cycle +date: 2026-10-17 +context: /gsd-explore session; $100 Azure student credit, goal is Terraform/IaC + Azure experience, not a public product +--- + +## Decisions + +- **Purpose:** DevOps practice (Terraform, Azure as second cloud, pipeline-driven deploy). No domain, no permanent public hosting. +- **Lifecycle:** ephemeral. Cycle is `terraform apply` -> check -> smoke test -> `terraform destroy`. +- **Order:** local CLI cycle first, then the same modules driven from GitHub Actions (`workflow_dispatch`, smoke test, destroy under `if: always()`). +- **Shape:** Azure Container Apps (consumption) pulling the public ghcr.io image + Azure Database for PostgreSQL Flexible Server (B1ms). +- **State:** Azure Storage blob backend in a separate bootstrap root module that is applied once and never destroyed. +- **Auth:** GitHub Actions -> Azure via OIDC federated credentials, no stored secrets. +- **Guardrails as code:** `azurerm_consumption_budget` alerts + manual destroy workflow. +- **Data:** empty DB each cycle; app migrates on boot; smoke test seeds itself (POST /watchlist + re-GET, as `n1-boot` does). + +## Relationship to existing work + +- Supersedes the blocker on deferred Phase 17 (automated VPS deploy, DPLY-01...08): that phase waits on a VPS + domain. This gives a deploy target without either. Phase 17's `17-CONTEXT.md` and the v1.4 `/ready` probe (health gate) are reusable inputs. +- Pipeline currently ends at `release` (ghcr push, SBOM, svu tag) in `.github/workflows/full-pipeline.yml`; a deploy job would gate on it and pass the `vX.Y.Z` tag as a Terraform variable. + +## Unresolved (not stood behind) + +- Flexible Server B1ms ~ $0.02/hr and "stopped server auto-restarts after 7 days" - unverifiable: prior knowledge, no primary source consulted. +- Total credit runway estimates (~100+ sessions) - derived from the unverified hourly figure above. diff --git a/.planning/phases/26-bulk-edit-remove/26-01-PLAN.md b/.planning/phases/26-bulk-edit-remove/26-01-PLAN.md new file mode 100644 index 00000000..a5553e66 --- /dev/null +++ b/.planning/phases/26-bulk-edit-remove/26-01-PLAN.md @@ -0,0 +1,396 @@ +--- +phase: 26-bulk-edit-remove +plan: 01 +type: execute +wave: 1 +depends_on: [] +files_modified: + - queries/bulk.sql + - internal/db/sqlc/bulk.sql.go + - internal/db/sqlc/querier.go + - internal/watchlist/bulk.go + - internal/watchlist/bulk_test.go + - internal/httpserver/bulk.go + - internal/httpserver/bulk_test.go + - internal/httpserver/server.go + - cmd/server/main.go +autonomous: true +requirements: [BULK-04] + +estimate: + tokens: 80000 + raw_tokens: 80000 + tasks: 3 + confidence: low + +must_haves: + truths: + - "POST /watchlist/bulk/delete with `{\"ids\":[...]}` deletes every listed watchlist row in one `DELETE FROM watchlist WHERE id = ANY(...) RETURNING id` statement with no explicit transaction, and answers 200 `{\"removed\":[...],\"already_gone\":[...]}` (D-03, grill Q6)." + - "An id that no longer exists counts as already removed: it is listed in `already_gone`, every other id is deleted, and the response is still 200 (D-03)." + - "Bulk remove deletes watchlist rows only: the artists row and its artist_tags links survive, the note goes with the row, and the removed artist's undelivered events keep `notified_at IS NULL`, so pending alerts are not withdrawn (D-17, D-18, grill Q4)." + - "The route is registered inside registerDataRoutes: with no session it answers 401, and with a session but no `X-Requested-With` it answers 403; in both cases the store is never called (PITFALLS #4)." + - "`httpserver.WatchlistBulkStore` is a new narrow interface that `*watchlist.Service` implements and `WithWatchlistBulk` supplies. Without the option the route answers 503 `watchlist bulk not available`, and `watchlist.Store` gains no method (D-20, grill Q5)." + - "`POST /watchlist/bulk/delete` reaches the bulk handler and never a `/watchlist/{id}` route (chi static-segment precedence, RESEARCH Pitfall B3)." + - "[EDGE BULK-04 boundary] Exactly 1000 ids are accepted. 1001 answer 400 `{\"error\":\"too many ids\",\"code\":\"bulk_too_many_ids\",\"max\":1000}`, and a body over 64 KiB answers 400 `invalid request body`. Both are rejected before the store is called." + - "[EDGE BULK-04 empty] A missing, null, or empty `ids` answers 400 with code `bulk_ids_required`. The service, given no ids, returns empty non-nil slices without touching the database. A batch whose ids are all already gone answers 200 with `removed: []`." + - "[EDGE BULK-04 adjacency] Duplicate ids collapse to their first occurrence before the store call, so each id is deleted and reported once." + - "[EDGE BULK-04 ordering] `removed` and `already_gone` are each sorted ascending by id." + - "[EDGE BULK-04 precision] Ids decode as int64. An id below 1 answers 400 with code `bulk_invalid_id`, and a fractional, string, or out-of-range id fails decoding with 400 `invalid request body`." + - "[EDGE BULK-04 idempotency] Repeating an identical bulk delete answers 200 with `removed: []` and every id in `already_gone`." + - "[EDGE BULK-04 concurrency] An id deleted by another request before the statement runs is reported in `already_gone` while the rest are deleted. The single statement never leaves a partial delete." + - statement: "[EDGE BULK-04 concurrency] Two bulk deletes racing over overlapping ids both answer 200, and each id appears in exactly one response's `removed` list. Row locks serialize them, and RESEARCH A2 accepts a rare 40P01 as a retryable 500." + verification: backstop + - "Every response slice serializes as `[]`, never `null`. Error bodies carry fixed text, and raw downstream errors go only to the `watchlist_bulk_error` log attribute." + artifacts: + - path: queries/bulk.sql + provides: "BulkDeleteWatchlist, the single atomic bulk delete (RESEARCH Example 5)" + contains: "-- name: BulkDeleteWatchlist :many" + - path: internal/db/sqlc/bulk.sql.go + provides: "Generated BulkDeleteWatchlist(ctx, ids []int64) ([]int64, error)" + contains: "func (q *Queries) BulkDeleteWatchlist" + - path: internal/watchlist/bulk.go + provides: "BulkRemoveResult, Service.BulkRemove, the missingIDs helper" + contains: "func (s *Service) BulkRemove" + - path: internal/httpserver/bulk.go + provides: "WatchlistBulkStore, WithWatchlistBulk, the shared id bound and dedupe, handleBulkDelete" + exports: ["WatchlistBulkStore", "WithWatchlistBulk"] + - path: internal/httpserver/server.go + provides: "watchlistBulk field and the POST /watchlist/bulk/delete route inside registerDataRoutes" + contains: "/watchlist/bulk/delete" + - path: cmd/server/main.go + provides: "Production wiring of the bulk store" + contains: "httpserver.WithWatchlistBulk(store)" + key_links: + - from: "internal/httpserver/server.go registerDataRoutes" + to: "internal/httpserver/bulk.go handleBulkDelete" + via: "a literal POST route inside the gated group" + pattern: "r\\.Post\\(\"/watchlist/bulk/delete\", s\\.handleBulkDelete\\)" + - from: "cmd/server/main.go" + to: "internal/httpserver/bulk.go WithWatchlistBulk" + via: "the same *watchlist.Service the poller and the watchlist routes use" + pattern: "WithWatchlistBulk\\(store\\)" + - from: "internal/watchlist/bulk.go BulkRemove" + to: "internal/db/sqlc/bulk.sql.go BulkDeleteWatchlist" + via: "s.q.BulkDeleteWatchlist, one statement, no transaction" + pattern: "s\\.q\\.BulkDeleteWatchlist\\(" + prohibitions: + - statement: "MUST NOT leave a partial removal: a bulk delete that fails removes nothing, and a success removes every selected row that still exists (BULK-04, D-03)." + requirement_id: BULK-04 + category: safety + status: resolved + verification: test +--- + + +Give the server its first bulk action, plus the shared seams every later bulk route reuses. The tracer deletes three +selected entries over HTTP in one statement and proves the outcome shape. Task 2 pins the service semantics: what +survives, already-gone ids, and the disclosed pending-alert behavior. Task 3 pins the HTTP contract: auth, CSRF, the 503 +fallback, route precedence, and the id and body bounds. + +Purpose: BULK-04's server half, plus the `WatchlistBulkStore` / `WithWatchlistBulk` seam (D-20) and the id-validation +helper that plans 26-02, 26-05, and 26-07 reuse. +Output: `queries/bulk.sql`, generated sqlc code, `internal/watchlist/bulk.go`, `internal/httpserver/bulk.go`, the route, the +production wiring, and Go tests. + +Locked inputs: D-03 (one DELETE, no transaction), D-04 (outcome only, no entries), D-17/D-18 (watchlist rows only; pending +events not withdrawn), D-20 (narrow store, 503 when absent, `watchlist.Store` untouched). Claude's discretion, settled +here: the path is `POST /watchlist/bulk/delete`, at most 1000 ids per request, and a 64 KiB body cap (RESEARCH Example 7, +assumption A4). No migration is needed, so `internal/db/migrations/README.md` is not triggered. + +Tooling on this box: `make`, `sqlc`, and `golangci-lint` may be missing from the bash PATH. Use `export PATH="$HOME/go/bin:$PATH"` +(sqlc lives at `~/go/bin/sqlc.exe`), or run a Makefile target's underlying commands (25-01 precedent). `go test -race` does +not work on this Windows box; CI's Linux job is the race gate. New comments are 1–3 line why-comments with at most one +decision reference (CLAUDE.md). + + + +The assumption-delta detector fired on a pluralization cue ("Also decide what all-or-nothing means…" in the ROADMAP notes). +- Primary noun: the watchlist entry id. Bulk routes act on a set of the same ids the single-entry routes use, and tags stay + artist-scoped (TAG-07). +- Decision: **no-change**. The single-entry routes keep their published contracts: the 201/200 attach, the idempotent 204 + detach, and the 404 on an unknown PATCH id. The bulk routes are set-based siblings with their own outcome shape (D-04). + Re-implementing the single routes on top of the bulk ones would change those contracts and add nothing. +- What would force a promote: a third caller (Phase 27's confirm step, for example) needing the same per-id outcome shape. + Grill Q7 deliberately leaves that open. + + +## Flagged assumptions + +- **A2 (RESEARCH Assumptions Log):** two overlapping bulk deletes may lock rows in different physical orders. For a + single-user app this residual deadlock risk is accepted, because grill Q6 locks the plain single statement. It appears as + the backstop truth above, and a `40P01` surfaces as a 500 that the client reports as "nothing was removed". +- **A4:** `maxBulkIDs = 1000` and the 64 KiB body cap are assumed generous for a realistic watchlist. The 400 body carries + `max`, so the SPA never depends on the constant (UI-SPEC Data Contract). +- **Canon referral (prohibition probe):** the auth/CSRF gating of every bulk route and XSS-safe rendering of names are canon + (OWASP). Each plan's threat model and `/gsd-secure-phase` cover them, so they are not minted as prohibitions. + + +@~/.claude/gsd-core/workflows/execute-plan.md +@~/.claude/gsd-core/templates/summary.md + + + +@.planning/phases/26-bulk-edit-remove/26-CONTEXT.md +@.planning/phases/26-bulk-edit-remove/26-RESEARCH.md +@.planning/phases/26-bulk-edit-remove/26-PATTERNS.md +@internal/httpserver/server.go +@internal/httpserver/tags.go + + +Existing seams this plan builds on (read from the repo at planning time): +- `internal/watchlist/service.go`: `type Service struct { q sqlc.Querier; ... }`; `func NewService(q sqlc.Querier, opts ...Option) *Service`; `func (s *Service) Remove(ctx context.Context, id int64) error` (the single-row precedent, lines 492–501); `var _ Store = (*Service)(nil)`. +- `internal/httpserver/watchlist.go`: `func decodeJSONBody(r *http.Request, dst any) error` (rejects unknown fields and trailing values); `func writeError(w http.ResponseWriter, status int, msg string)`; `func writeErrorCode(w http.ResponseWriter, status int, code, msg string)`; `type domainError struct { err error; status int; code string }`; `func writeDomainError(w http.ResponseWriter, table []domainError, err error) bool`. +- `internal/httpserver/tags.go`: the `WithTags` option and the `s.tags == nil` 503 guard (lines 65–72, 99–102); `tagCollisionResponse` (lines 186–191) is the precedent for a 4xx body with an extra field. +- `internal/httpserver/server.go`: `Server` (lines 31–46), `serverConfig` (51–63), field copy in `New` (173–180), `registerDataRoutes` (264–293). +- `cmd/server/main.go` line 233: `store := watchlist.NewService(sqlc.New(pool), watchlist.WithArtistArt(...))`, and lines 310–313 wire `WithSettings`/`WithTags`. +- Test helpers in package `httpserver_test`: `testMBID(t)`, `discardLogger()` (health_test.go:51), `stubStore{}` (watchlist_test.go:46), `stubEventsStore{}` (events_test.go:37), `noopPinger{}` (watchlist_test.go:112), `settingsTestPassphrase` and `loginForSettings(t, ts)` (settings_test.go:348, 414). + + + + +Created in this plan: +- SQL: `BulkDeleteWatchlist :many` in the new `queries/bulk.sql` +- Generated: `func (q *Queries) BulkDeleteWatchlist(ctx context.Context, ids []int64) ([]int64, error)` and its `Querier` method +- Go (`internal/watchlist/bulk.go`): `type BulkRemoveResult struct { Removed []int64; AlreadyGone []int64 }`, `func (s *Service) BulkRemove(ctx context.Context, ids []int64) (BulkRemoveResult, error)`, `func missingIDs(requested, found []int64) []int64` +- Go (`internal/httpserver/bulk.go`): `const maxBulkIDs = 1000`, `const maxBulkBodyBytes = 65536`, codes `codeBulkIDsRequired = "bulk_ids_required"`, `codeBulkTooManyIDs = "bulk_too_many_ids"`, `codeBulkInvalidID = "bulk_invalid_id"`; sentinels `errBulkIDsRequired`, `errBulkTooManyIDs`, `errBulkInvalidID`; `type WatchlistBulkStore interface { BulkRemove(ctx context.Context, ids []int64) (watchlist.BulkRemoveResult, error) }`; `func WithWatchlistBulk(store WatchlistBulkStore) Option`; `func validateBulkIDs(ids []int64) ([]int64, error)`; `func writeBulkIDsError(w http.ResponseWriter, err error)`; `type bulkTooManyResponse`; `type bulkDeleteRequest`; `type bulkDeleteResponse`; `func (s *Server) handleBulkDelete(w http.ResponseWriter, r *http.Request)` +- Server: `Server.watchlistBulk` and `serverConfig.watchlistBulk` fields; route `POST /watchlist/bulk/delete` +- Wiring: `httpserver.WithWatchlistBulk(store)` in `cmd/server/main.go` +- Tests: `TestBulk_DeleteEndToEnd`, `TestBulk_Gated401NoCookie`, `TestBulk_GatedForbiddenWithoutCSRFHeader`, `TestBulk_UnavailableWithoutOption`, `TestBulk_DeleteReachesBulkHandler`, `TestBulk_DeleteIDValidation`, `TestBulk_DeleteStoreErrorIs500`, the `fakeBulkStore` double and the `newBulkServer` helper (internal/httpserver/bulk_test.go); `TestService_BulkRemove` and `TestService_BulkRemove_KeepsArtistTagsAndPendingEvents` (internal/watchlist/bulk_test.go) + +Consumed by sibling plans: 26-02, 26-05, and 26-07 reuse `validateBulkIDs`, `writeBulkIDsError`, `maxBulkBodyBytes`, and +`newBulkServer`. 26-05 extends `WatchlistBulkStore` and `fakeBulkStore`. 26-03 calls the route from the SPA. + + + + + + Task 1: Bulk-delete three selected entries over HTTP in one statement + The dev Postgres fixture answers on localhost:5432 (`docker compose ps postgres` reports it healthy; `make db-up` or `docker compose up -d postgres` otherwise). + queries/bulk.sql, internal/db/sqlc/bulk.sql.go, internal/db/sqlc/querier.go, internal/watchlist/bulk.go, internal/httpserver/bulk.go, internal/httpserver/server.go, cmd/server/main.go, internal/httpserver/bulk_test.go + + - `.planning/phases/26-bulk-edit-remove/26-RESEARCH.md` Example 5, Example 7, Pattern 3, and Pitfalls B3 and B6. + - `queries/notification_settings.sql` lines 26–40 (`AckEventsOnly` / `AckDigestBatch`): the `= ANY(...::bigint[])` precedent. + - `internal/watchlist/service.go` `Remove` (lines 483–501) and the `Service` struct (173–192). + - `internal/httpserver/tags.go` lines 53–130: the TagStore/WithTags option shape, the 503 guard, MaxBytesReader, and decode. + - `internal/httpserver/watchlist.go` lines 50–128: `errorResponse`, `writeError`, `writeErrorCode`, `domainError`, `writeDomainError`, `decodeJSONBody`. + - `internal/httpserver/server.go` lines 31–63, 161–181, 264–293. + - `internal/httpserver/tags_test.go` `TestTags_AttachEndToEnd` (lines 44–90): real services behind `httpserver.New` on an isolated schema. + - `Makefile` `sqlc-check` (lines 126–128): it diffs generated code against the git index. + + +Per D-03 (grill Q6), create `queries/bulk.sql` holding one statement, `-- name: BulkDeleteWatchlist :many`, with the +RESEARCH Example 5 body: delete from watchlist where id equals any of `@ids::bigint[]`, returning id. Above it goes a +comment of at most two lines saying one statement is all-or-nothing and that ids it does not return were already gone. +Don't wrap it in a transaction and don't add a pre-lock. Regenerate with `sqlc generate` (`~/go/bin/sqlc.exe generate` when +it is not on PATH). The generated method must read `BulkDeleteWatchlist(ctx context.Context, ids []int64) ([]int64, error)`, +and `querier.go` gains the same method. Never hand-edit generated code; stage `internal/db/sqlc` before `make sqlc-check`. + +Create `internal/watchlist/bulk.go`: +- `BulkRemoveResult` with `Removed []int64` and `AlreadyGone []int64`. +- `missingIDs(requested, found []int64) []int64`: the distinct requested ids absent from found, ascending, never nil. + Plan 26-05 reuses it. +- `(s *Service) BulkRemove(ctx, ids []int64) (BulkRemoveResult, error)`. With no ids it returns two empty non-nil slices + and makes no database call. Otherwise it calls `s.q.BulkDeleteWatchlist` once, sorts `Removed` ascending, and sets + `AlreadyGone = missingIDs(ids, removed)`. A query error is wrapped as `bulk delete watchlist: %w`. +- Do not add the method to the `Store` interface (D-20). +- Keep comments to 1–3 lines with one decision reference (D-03). + +Create `internal/httpserver/bulk.go`: +- The constants `maxBulkIDs = 1000` and `maxBulkBodyBytes = 65536`, plus the three code constants and three unexported + sentinels listed in the artifacts section. +- `validateBulkIDs(ids)`. Empty or nil returns `errBulkIDsRequired`. More than `maxBulkIDs` raw entries return + `errBulkTooManyIDs`, checked before dedupe so the bound applies to what was sent. Any id below 1 returns + `errBulkInvalidID`. Otherwise it returns the ids deduped in first-occurrence order. +- `writeBulkIDsError(w, err)`. Required becomes 400 via `writeErrorCode` with message `ids are required`. Invalid becomes + 400 `invalid watchlist id` with `bulk_invalid_id`. Too many becomes 400 with a `bulkTooManyResponse{Error: "too many ids", + Code: codeBulkTooManyIDs, Max: maxBulkIDs}` body (JSON keys `error`, `code`, `max`), following `tagCollisionResponse`. +- `WatchlistBulkStore` with the single `BulkRemove` method, and `WithWatchlistBulk(store)` setting + `serverConfig.watchlistBulk`. Its comment states that absent, the bulk watchlist routes answer 503, like `WithTags`. +- `bulkDeleteRequest` with `IDs []int64` (JSON key `ids`), and `bulkDeleteResponse` with `Removed []int64` (JSON key + `removed`) and `AlreadyGone []int64` (JSON key `already_gone`). +- `handleBulkDelete`, in this order: + 1. If `s.watchlistBulk` is nil, answer 503 `watchlist bulk not available`. + 2. Bound the body with `http.MaxBytesReader(w, r.Body, maxBulkBodyBytes)`. + 3. Decode with `decodeJSONBody`; failure answers 400 `invalid request body`. + 4. Run `validateBulkIDs`, then `writeBulkIDsError` on failure. + 5. Call the store. An error sets `httplog.SetAttrs(r.Context(), slog.String("watchlist_bulk_error", err.Error()))` and + answers 500 `internal error`. + 6. Otherwise answer 200 JSON, replacing a nil result slice with an empty one so the body never carries `null`. + +In `internal/httpserver/server.go`, add `watchlistBulk WatchlistBulkStore` to both `Server` and `serverConfig` next to +`tags`, and copy it in `New`. Register `r.Post("/watchlist/bulk/delete", s.handleBulkDelete)` inside `registerDataRoutes`, +after the tag routes, under a one-line comment: literal bulk paths inherit the gate and outrank `/watchlist/{id}`. + +In `cmd/server/main.go`, add `httpserver.WithWatchlistBulk(store),` after `WithTags`, with a one-line comment. `store` is +the existing `*watchlist.Service`. + +Create `internal/httpserver/bulk_test.go` (package `httpserver_test`) with: +- `var _ httpserver.WatchlistBulkStore = (*watchlist.Service)(nil)`. +- `TestBulk_DeleteEndToEnd` on `testutil.NewIsolatedTestPool(t, "bulk_http_test")`, wired with + `watchlist.NewService(sqlc.New(pool))` and `httpserver.New(pool, svc, stubEventsStore{}, nil, discardLogger(), httpserver.WithWatchlistBulk(svc))`. + It adds three entries through `svc.Add` (MBIDs from `testMBID(t)` plus a suffix), then POSTs + `{"ids":[a,b,999999999]}` with `Content-Type: application/json`. Assert 200 and a decoded body of + `removed == [a,b]` and `already_gone == [999999999]`. A follow-up GET `/watchlist` lists only the third entry. + + + TEST_DATABASE_URL='postgres://drop_tracker:drop_tracker@localhost:5432/drop_tracker?sslmode=disable' go test ./internal/httpserver/ -run 'TestBulk_DeleteEndToEnd' -count=1 -v + non-zero exit, a "--- FAIL" line, or "testing: warning: no tests to run" (the test name did not match) + export PATH="$HOME/go/bin:$PATH" && go build ./... && go vet ./... && git add internal/db/sqlc && make sqlc-check + non-zero exit, any vet diagnostic, or sqlc-check printing a diff under internal/db/sqlc/ + + + - `grep -c -- '-- name: BulkDeleteWatchlist :many' queries/bulk.sql` prints `1`. + - `grep -c 'func (q \*Queries) BulkDeleteWatchlist(ctx context.Context, ids \[\]int64) (\[\]int64, error)' internal/db/sqlc/bulk.sql.go` prints `1`. + - `grep -c 'func (s \*Service) BulkRemove(ctx context.Context, ids \[\]int64) (BulkRemoveResult, error)' internal/watchlist/bulk.go` prints `1`, and `awk '/^type Store interface/,/^}/' internal/watchlist/service.go | grep -c 'ctx context.Context'` still prints `5` (D-20: `Store` gains no method). + - `grep -c 'r.Post("/watchlist/bulk/delete", s.handleBulkDelete)' internal/httpserver/server.go` prints `1`. + - `grep -c 'httpserver.WithWatchlistBulk(store)' cmd/server/main.go` prints `1`. + - `TestBulk_DeleteEndToEnd` passes, asserting the exact `removed` and `already_gone` arrays and the surviving third entry. + + One POST deletes the selected rows in one statement, reports already-gone ids, is wired in production, and is committed. + + + + Task 2: Pin what bulk remove deletes, keeps, and reports + The dev Postgres fixture answers on localhost:5432. + internal/watchlist/bulk_test.go + + - Three existing ids: all three rows are gone, `Removed` holds the three ids ascending, and `AlreadyGone` is `[]` (non-nil). + - Requested `{c, a, 999999999, a}`: `Removed == [a, c]` and `AlreadyGone == [999999999]` (duplicates and order normalized). + - An id deleted directly with SQL beforehand (the "another tab" case) lands in `AlreadyGone`, and the rest are removed. + - A repeat of the same call: `Removed == []`, and every id is in `AlreadyGone`. + - An empty id slice gives two empty non-nil slices and no error. + - After removal the `artists` row still exists, its `artist_tags` link to a seeded tag still exists, and an `events` row seeded for that artist with `notified_at IS NULL` is still present and still unnotified (D-17, D-18). + - Re-adding the same MBID through `Service.Add` returns an entry whose `Note` is nil (the note went with the row, D-18). + + + - `internal/watchlist/bulk.go` as Task 1 left it. + - `internal/watchlist/service_test.go` lines 60–80 (`testMBID`) and the note tests around line 2128 (`watchlist_note_test`), for how an entry with a note is seeded through `Service.UpdateNote`. + - `internal/httpserver/events_test.go` `insertTestEventAt` (lines 388–404), for the events INSERT column list (artist_id, source, event_type, external_id, title, artist_name, release_date, created_at). + - `internal/tags/service_test.go` lines 19–40, for raw-SQL tag/link seeding shapes. + + +Create `internal/watchlist/bulk_test.go` (package `watchlist_test`) on `testutil.NewIsolatedTestPool(t, "watchlist_bulk_test")` +with `watchlist.NewService(sqlc.New(pool))`. Add `TestService_BulkRemove` as a set of subtests, one per behavior line from +the first five bullets. Each subtest adds its own entries through `svc.Add`, with MBIDs derived from `testMBID(t)` plus a +suffix, so subtests never collide. + +Add `TestService_BulkRemove_KeepsArtistTagsAndPendingEvents`: +- Add an entry and give it a note through `svc.UpdateNote`. +- Seed a tag and link it to the entry's artist with raw SQL: `INSERT INTO tags (name) VALUES ($1) RETURNING id`, then + `INSERT INTO artist_tags (artist_id, tag_id)`. +- Seed one `new_release` event for the artist with `notified_at` left NULL. Give it an external id unique to the test, + because `events_dedup_key` is (event_type, source, external_id). +- Bulk-remove the entry. Assert with raw SQL: the artists row count for that id is 1, the link count is 1, and the + events count for that artist with `notified_at IS NULL` is 1. +- Re-add the same MBID through `svc.Add` and assert the returned `Note` is nil. + +Use `t.Fatalf` with the got/want values. No production code changes are expected. If a behavior fails, fix +`internal/watchlist/bulk.go` within this task. + + + TEST_DATABASE_URL='postgres://drop_tracker:drop_tracker@localhost:5432/drop_tracker?sslmode=disable' go test ./internal/watchlist/ -run 'TestService_BulkRemove' -count=1 -v + non-zero exit, a "--- FAIL" line, or "testing: warning: no tests to run" + + + - `grep -c 'func TestService_BulkRemove' internal/watchlist/bulk_test.go` prints `2`. + - `go test -v` output lists at least 5 `--- PASS: TestService_BulkRemove/` subtest lines. + - The keeps-test asserts the surviving artist row, the tag link, the unnotified event, and the nil note after re-add. + + Bulk remove's survivors, already-gone reporting, idempotency, and the disclosed pending-alert behavior are pinned and committed. + + + + Task 3: Pin the bulk route's gate, fallback, precedence, and bounds + internal/httpserver/bulk_test.go + + - No session on a gated server: POST /watchlist/bulk/delete answers 401. + - A session cookie but no `X-Requested-With`: 403, and the fake store's call counter stays 0. + - A server built without `WithWatchlistBulk`: 503 with body error `watchlist bulk not available`. + - With a fake store and a `stubStore` whose `removeFunc` fails the test if called, POST /watchlist/bulk/delete reaches the bulk handler (counter 1) and the response carries the `removed` key. + - Bodies: `{}`, `{"ids":null}`, and `{"ids":[]}` give 400 `bulk_ids_required`. `{"ids":[0]}` and `{"ids":[-3]}` give 400 `bulk_invalid_id`. `{"ids":[1.5]}`, `{"ids":["1"]}`, `{"ids":[9223372036854775808]}`, `{"ids":[1],"note":"x"}`, and two concatenated JSON objects give 400 `invalid request body`. A body over 65536 bytes (valid JSON padded with whitespace) gives 400 `invalid request body`. 1001 ids give 400 with code `bulk_too_many_ids` and `max` 1000. None of these call the store. + - Exactly 1000 distinct ids reach the store once with all 1000. `{"ids":[5,5,7]}` reaches it with `[5,7]`. + - A store returning nil slices produces a body of exactly `{"removed":[],"already_gone":[]}`. + - A store error gives 500 `internal error`, and the body never contains the error's text. + + + - `internal/httpserver/bulk_test.go` as Task 1 left it. + - `internal/httpserver/tags_test.go` lines 590–695 (`fakeTagStore` with `*int32` counters, `tagsServerOpts` / `newTagsServer`) and lines 936–1005 (the gated 401/403 pair). + - `internal/httpserver/settings_test.go` lines 340–430 (`settingsTestPassphrase`, `loginForSettings`). + - `internal/httpserver/watchlist_test.go` lines 40–115 (`stubStore`, `errorBody`, `noopPinger`). + + +Extend `internal/httpserver/bulk_test.go`: +- `fakeBulkStore`, a func-field double with `removeFunc func(ctx context.Context, ids []int64) (watchlist.BulkRemoveResult, error)` + and `removeCalls *int32`, plus a recorder for the last ids it received. Add + `var _ httpserver.WatchlistBulkStore = fakeBulkStore{}`. +- `bulkServerOpts{passphrase string; store httpserver.WatchlistBulkStore; watchlist watchlist.Store; omitBulk bool}` and + `newBulkServer(t, o)`. It mirrors `newTagsServer`: an optional `WithAuthGate(o.passphrase, false, nil)`, then + `WithWatchlistBulk` unless `omitBulk`, built on `noopPinger{}`, `o.watchlist` (defaulting to `stubStore{}`), + `stubEventsStore{}`, and `discardLogger()`, with both `srv.Close` and `ts.Close` registered in `t.Cleanup`. Plans 26-05 and + 26-07 reuse this helper. + +Add the tests named in the artifacts section, one per behavior group: +- `TestBulk_Gated401NoCookie` +- `TestBulk_GatedForbiddenWithoutCSRFHeader`, which logs in with `loginForSettings` and asserts + `atomic.LoadInt32(&calls) == 0` +- `TestBulk_UnavailableWithoutOption` +- `TestBulk_DeleteReachesBulkHandler`, whose watchlist double is a `stubStore` with a `removeFunc` that calls `t.Errorf` +- `TestBulk_DeleteIDValidation`, a table over every body above, asserting status, decoded `code` where one is expected, + and that the counter is unchanged on rejection +- `TestBulk_DeleteStoreErrorIs500` + +Build the 1000- and 1001-id bodies in a loop. For the oversize body, append 70000 spaces before the closing brace. No +production change is expected. If a behavior fails, fix `internal/httpserver/bulk.go` within this task. + + + TEST_DATABASE_URL='postgres://drop_tracker:drop_tracker@localhost:5432/drop_tracker?sslmode=disable' go test ./internal/httpserver/ -run 'TestBulk_' -count=1 -v + non-zero exit, a "--- FAIL" line, or "testing: warning: no tests to run" + TEST_DATABASE_URL='postgres://drop_tracker:drop_tracker@localhost:5432/drop_tracker?sslmode=disable' go test ./internal/watchlist/ ./internal/httpserver/ -count=1 && golangci-lint run + non-zero exit, a "FAIL" package line, or any golangci-lint finding + + + - `grep -cE 'func TestBulk_(Gated401NoCookie|GatedForbiddenWithoutCSRFHeader|UnavailableWithoutOption|DeleteReachesBulkHandler|DeleteIDValidation|DeleteStoreErrorIs500)\(' internal/httpserver/bulk_test.go` prints `6`. + - `grep -c 'var _ httpserver.WatchlistBulkStore = fakeBulkStore{}' internal/httpserver/bulk_test.go` prints `1`. + - The validation table covers all twelve rejected bodies, plus the 1000-id and duplicate accepted cases. + - Both full packages pass, and golangci-lint (including gosec) reports nothing. + + The bulk route's auth, CSRF, 503, precedence, and every id/body bound are pinned with zero store calls on rejection; committed. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| Browser → POST /watchlist/bulk/delete | Untrusted JSON body, the session cookie, and the CSRF header cross into the gated group | +| httpserver → watchlist.Service → Postgres | Validated, deduped ids flow into one parameterized DELETE | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan | +|-----------|----------|-----------|----------|-------------|-----------------| +| T-26-01 | Spoofing / Elevation | POST /watchlist/bulk/delete without a session | high | mitigate | Registered inside `registerDataRoutes`, so `gate.Authenticate` runs first; `TestBulk_Gated401NoCookie` | +| T-26-02 | Tampering (CSRF) | Cross-site mass delete | high | mitigate | `gate.RequireCSRFHeader` on the gated group; `TestBulk_GatedForbiddenWithoutCSRFHeader` asserts 403 and zero store calls | +| T-26-03 | Denial of Service | Oversized id array or body | medium | mitigate | `maxBulkIDs = 1000` checked on the raw array plus a 64 KiB `MaxBytesReader`, both before any store call; validation table test | +| T-26-04 | Tampering | Malformed ids, over-posted keys, concatenated values | medium | mitigate | `decodeJSONBody` (DisallowUnknownFields + single value), ids ≥ 1, int64 decode, first-occurrence dedupe | +| T-26-05 | Information Disclosure | Error bodies | low | mitigate | Fixed messages via `writeError`/`writeErrorCode`; raw error only in the `watchlist_bulk_error` log attribute; 500 test asserts no leak | +| T-26-06 | Tampering / Integrity | Partial delete on failure | medium | mitigate | One `DELETE ... = ANY RETURNING id` statement (D-03), and nothing references `watchlist` by foreign key; service tests | +| T-26-07 | Denial of Service (self) | Two overlapping bulk deletes locking rows in different orders | low | accept | RESEARCH A2: single-user app and a single statement; a rare 40P01 is a 500 the user retries (backstop truth) | + + + +- `TestBulk_DeleteEndToEnd`, `TestService_BulkRemove*`, and every `TestBulk_*` test pass against real Postgres. +- `go build ./...`, `go vet ./...`, `golangci-lint run`, and `make sqlc-check` (after `git add internal/db/sqlc`) are clean. + + + +The server deletes a bounded, deduped set of watchlist entries in one atomic statement behind the auth gate and CSRF +check. It reports already-gone ids, keeps artists, tags, and pending events, and exposes the bulk store seam every later +bulk route reuses. + + + +Create `.planning/phases/26-bulk-edit-remove/26-01-SUMMARY.md` when done. + diff --git a/.planning/phases/26-bulk-edit-remove/26-01-SUMMARY.md b/.planning/phases/26-bulk-edit-remove/26-01-SUMMARY.md new file mode 100644 index 00000000..17fd456b --- /dev/null +++ b/.planning/phases/26-bulk-edit-remove/26-01-SUMMARY.md @@ -0,0 +1,158 @@ +--- +phase: 26-bulk-edit-remove +plan: 01 +subsystem: api +tags: [go, chi, sqlc, postgres, bulk-actions] + +requires: + - phase: 24-artist-tags-and-notes + provides: "WithTags option shape, domainError/writeErrorCode helpers, tags/notes tables" +provides: + - "POST /watchlist/bulk/delete: one atomic DELETE ... = ANY RETURNING id, outcome {removed, already_gone}" + - "httpserver.WatchlistBulkStore + WithWatchlistBulk seam (503 when absent), watchlist.Store untouched" + - "validateBulkIDs / writeBulkIDsError / maxBulkBodyBytes / newBulkServer helpers reused by 26-02, 26-05, 26-07" + - "watchlist.BulkRemoveResult, Service.BulkRemove, missingIDs" +affects: [26-02, 26-03, 26-05, 26-07] + +actuals: + tokens: 7000 + tasks: 3 + commits: 3 +plan_head_before: db223423c069ee43bc4d6f7e79994557d0107ce6 + +tech-stack: + added: [] + patterns: + - "Bulk routes are literal POST paths inside registerDataRoutes: inherit gate and CSRF, outrank /watchlist/{id}" + - "Single-statement set delete reported via RETURNING; ids not returned are already gone" + +key-files: + created: + - queries/bulk.sql + - internal/db/sqlc/bulk.sql.go + - internal/watchlist/bulk.go + - internal/watchlist/bulk_test.go + - internal/httpserver/bulk.go + - internal/httpserver/bulk_test.go + modified: + - internal/db/sqlc/querier.go + - internal/httpserver/server.go + - cmd/server/main.go + +key-decisions: + - "Path POST /watchlist/bulk/delete, at most 1000 ids and a 64 KiB body cap; the 400 body carries max so the SPA never hardcodes the bound" + - "The id bound is checked on the raw array before dedupe so it applies to what was sent" + - "Bulk remove deletes watchlist rows only: artists, artist_tags and undelivered events survive, the note goes with the row" + +patterns-established: + - "WatchlistBulkStore: narrow per-feature store interface with a With* option, mirroring TagStore/WithTags" + +requirements-completed: [BULK-04] + +coverage: + - id: D1 + description: "POST /watchlist/bulk/delete removes the selected rows in one statement and reports removed and already-gone ids" + requirement: BULK-04 + verification: + - kind: integration + ref: "internal/httpserver/bulk_test.go#TestBulk_DeleteEndToEnd" + status: pass + - kind: integration + ref: "internal/watchlist/bulk_test.go#TestService_BulkRemove" + status: pass + human_judgment: false + - id: D2 + description: "Bulk remove keeps the artist row, tag links and pending events; the note goes with the entry" + requirement: BULK-04 + verification: + - kind: integration + ref: "internal/watchlist/bulk_test.go#TestService_BulkRemove_KeepsArtistTagsAndPendingEvents" + status: pass + human_judgment: false + - id: D3 + description: "The route is gated (401/403), 503 without the option, reaches the bulk handler, and enforces id and body bounds" + requirement: BULK-04 + verification: + - kind: unit + ref: "internal/httpserver/bulk_test.go#TestBulk_Gated401NoCookie" + status: pass + - kind: unit + ref: "internal/httpserver/bulk_test.go#TestBulk_GatedForbiddenWithoutCSRFHeader" + status: pass + - kind: unit + ref: "internal/httpserver/bulk_test.go#TestBulk_DeleteIDValidation" + status: pass + human_judgment: false + - id: D4 + description: "Two bulk deletes racing over overlapping ids both succeed with each id in exactly one removed list (backstop truth; a rare 40P01 surfaces as a retryable 500)" + requirement: BULK-04 + verification: [] + human_judgment: true + rationale: "Accepted residual risk (RESEARCH A2) with no automated race test; single-user app, plan marks it backstop" + +duration: 14min +completed: 2026-10-16 +status: complete +--- + +# Phase 26 Plan 01: Bulk delete server half Summary + +**POST /watchlist/bulk/delete deletes a bounded, deduped set of watchlist entries in one `DELETE ... = ANY RETURNING id` statement behind the auth gate and CSRF check, reporting already-gone ids and keeping artists, tags and pending events.** + +## Performance + +- **Duration:** ~14 min +- **Completed:** 2026-10-16 +- **Tasks:** 3 (1 tracer, 2 TDD-flagged test tasks) +- **Files modified:** 9 (6 created, 3 modified) + +## Accomplishments + +- `BulkDeleteWatchlist :many` in `queries/bulk.sql`, generated with sqlc v1.31.1; `Service.BulkRemove` sorts `Removed` and derives `AlreadyGone` through `missingIDs` (no transaction, no database call for empty input). +- `internal/httpserver/bulk.go` adds the `WatchlistBulkStore` seam, `WithWatchlistBulk`, the shared id validator (raw-array bound, ids >= 1, first-occurrence dedupe) and `handleBulkDelete`; the route sits inside `registerDataRoutes` so it inherits `Authenticate` and `RequireCSRFHeader`; `cmd/server/main.go` wires the same `*watchlist.Service`. +- Tests pin survivors, already-gone reporting, idempotency, gate/CSRF, the 503 fallback, literal-path precedence, every id/body bound, empty-array serialization and the 500 no-leak rule. + +## Task Commits + +1. **Task 1: Bulk-delete three selected entries over HTTP in one statement (tracer)** - `22d2d0f` (feat) +2. **Task 2: Pin what bulk remove deletes, keeps, and reports** - `2bdedc4` (test) +3. **Task 3: Pin the bulk route's gate, fallback, precedence, and bounds** - `aabb51d` (test) + +## Verification + +- Tracer verify re-run after commit: `TestBulk_DeleteEndToEnd` passes; `go build`, `go vet`, `golangci-lint run` (0 issues), sqlc regenerate shows no diff in `internal/db/sqlc/`. +- Full suite `go test ./...` passes against real Postgres; backend coverage 90.49% (gate 80%). +- All task acceptance greps pass (1/1/1, `Store` still 5 `ctx` methods, route and wiring 1/1, 6 `TestBulk_*` names, 1 interface assertion, 2 `TestService_BulkRemove*`, 5 subtests). + +## Decisions Made + +- Path, 1000-id bound and 64 KiB body cap as planned (RESEARCH Example 7, A4). +- `watchlist.Store` gained no method; the bulk seam is its own interface (D-20). + +## Deviations from Plan + +None - plan executed exactly as written. + +TDD note: Tasks 2 and 3 carry `tdd="true"` but their implementation shipped in the Task 1 tracer, so their tests passed on first run and no separate RED commit was possible; both are `test(...)` commits, no production change needed. + +## Issues Encountered + +- `go test -race` is unavailable on this Windows box (`-race requires cgo`); the full suite and coverage run were done without it. CI's Linux job is the race gate. +- `make` is not installed; the Makefile target bodies (`sqlc-check`, `test-integration` without `-race`, `coverage-gate`) were run directly. +- `gofmt -l` lists `internal/httpserver/server.go` (struct field alignment already off before this plan, likely line endings); left as is, `golangci-lint` is clean. + +## Known Stubs + +None. + +## Threat Flags + +None. New surface (the bulk route) is the one in the plan's threat model (T-26-01 to T-26-07); T-26-01/02/03/04/05/06 are covered by the tests above, T-26-07 is accepted. + +## Next Phase Readiness + +Ready for 26-02. `validateBulkIDs`, `writeBulkIDsError`, `maxBulkBodyBytes`, `missingIDs` and `newBulkServer` are in place for the sibling bulk routes. + +## Self-Check: PASSED + +Created files exist (`queries/bulk.sql`, `internal/db/sqlc/bulk.sql.go`, `internal/watchlist/bulk.go`, `internal/watchlist/bulk_test.go`, `internal/httpserver/bulk.go`, `internal/httpserver/bulk_test.go`); commits `22d2d0f`, `2bdedc4`, `aabb51d` exist. diff --git a/.planning/phases/26-bulk-edit-remove/26-02-PLAN.md b/.planning/phases/26-bulk-edit-remove/26-02-PLAN.md new file mode 100644 index 00000000..ee0d66db --- /dev/null +++ b/.planning/phases/26-bulk-edit-remove/26-02-PLAN.md @@ -0,0 +1,366 @@ +--- +phase: 26-bulk-edit-remove +plan: 02 +type: execute +wave: 2 +depends_on: [26-01] +files_modified: + - queries/bulk.sql + - internal/db/sqlc/bulk.sql.go + - internal/db/sqlc/querier.go + - internal/tags/bulk.go + - internal/tags/bulk_test.go + - internal/httpserver/tags.go + - internal/httpserver/tags_test.go + - internal/httpserver/bulk.go + - internal/httpserver/server.go + - docs/adr/0004-per-artist-tag-cap-trigger.md +autonomous: true +requirements: [BULK-02] + +estimate: + tokens: 95000 + raw_tokens: 95000 + tasks: 3 + confidence: low + +must_haves: + truths: + - "POST /watchlist/bulk/tags/add with `{\"ids\":[...],\"name\":\"...\"}` runs three steps in one transaction: `GetOrCreateTag`, then `LockWatchlistArtistsForBulkTag` (`ORDER BY a.id FOR NO KEY UPDATE OF a`), then one count-guarded `BulkAttachTag` insert with `ON CONFLICT DO NOTHING` (D-01, grill Q3)." + - "The tag lands on every selected artist with fewer than 10 links. An artist that already carries it counts as applied and gets no duplicate link, so already-tagged artists are unaffected (SC2)." + - "An artist at 10 links that lacks the tag is skipped with reason `cap` and its artist name. It is never skipped silently (D-01, SC2)." + - "A selected id with no watchlist row is skipped with reason `gone`, and the rest still apply (D-02)." + - "When no selected artist carries the tag afterwards, the transaction rolls back, so a tag created on the fly leaves no `tags` row, and the result's tag id is 0 (D-01, 24 D-29, D-19)." + - "The response is 200 `{\"tag\":{\"id\",\"name\"},\"applied\":[...],\"skipped\":[{\"id\",\"name\",\"reason\"}]}` with no entries. `name` appears only on `cap` skips (D-04, grill Q2)." + - "The tag name runs through `tags.NormalizeName` before any database call, and its errors map through the existing `tagErrors` table to 400 `tag_name_required`, `tag_name_too_long`, or `tag_name_invalid` (D-12)." + - "The ADR 0004 trigger stays the backstop. A trigger `check_violation` on `artist_tags_max_per_artist` still maps to `ErrTagCapReached` (409 `tag_cap_reached`), and the existing `internal/db` trigger tests still pass." + - "Bulk add-tag is a method on `tags.Service` exposed by extending `httpserver.TagStore`. `watchlist.Store` gains nothing, and `internal/tags` does not import `internal/watchlist` (D-20, grill Q2/Q5)." + - "`docs/adr/0004-per-artist-tag-cap-trigger.md` records the ordered pre-lock for bulk insert and the all-capped rollback (D-19)." + - "The route sits inside registerDataRoutes. With no session it answers 401, and with no `X-Requested-With` it answers 403; the store is never called in either case. Without `WithTags` it answers 503 `tags not available`, and the path never reaches `POST /watchlist/{id}/tags`." + - "[EDGE BULK-02 adjacency] An artist with exactly 9 links gets the tag as its 10th, and one with exactly 10 links is skipped as `cap`. One with 10 links that already carries the tag is `applied`, not `cap`." + - "[EDGE BULK-02 empty] Empty or missing ids answer 400 `bulk_ids_required` and create no tag. A batch whose ids are all gone answers 200 with `applied: []` and every id skipped as `gone`, and leaves no `tags` row." + - "[EDGE BULK-02 ordering] `applied` and `skipped` are each sorted ascending by watchlist id." + - "[EDGE BULK-02 idempotency] Repeating an identical bulk add returns the same `applied` ids and adds no link." + - "[EDGE BULK-02 concurrency] Two overlapping bulk adds of different tags with reversed id lists both succeed with no `40P01`. A bulk add waiting behind a concurrent uncommitted 10th link on one artist skips that artist as `cap`, with no error, after the other transaction commits (D-19)." + artifacts: + - path: queries/bulk.sql + provides: "LockWatchlistArtistsForBulkTag and BulkAttachTag (RESEARCH Examples 1–2)" + contains: "FOR NO KEY UPDATE OF a" + - path: internal/db/sqlc/bulk.sql.go + provides: "LockWatchlistArtistsForBulkTagRow{EntryID, ArtistID, Name} and BulkAttachTagRow{ArtistID, HadTag, Inserted}" + contains: "func (q *Queries) BulkAttachTag" + - path: internal/tags/bulk.go + provides: "SkipReasonCap, SkipReasonGone, BulkSkip, BulkAttachResult, Service.BulkAttach" + contains: "func (s *Service) BulkAttach" + - path: internal/httpserver/tags.go + provides: "TagStore.BulkAttach" + contains: "BulkAttach(ctx context.Context, entryIDs []int64, name string) (tags.BulkAttachResult, error)" + - path: internal/httpserver/bulk.go + provides: "bulkSkipWire, toSkipWire, bulkAddTagRequest/Response, handleBulkAddTag" + contains: "func (s *Server) handleBulkAddTag" + - path: docs/adr/0004-per-artist-tag-cap-trigger.md + provides: "Bulk-attach amendment" + contains: "ORDER BY" + key_links: + - from: "internal/httpserver/server.go registerDataRoutes" + to: "internal/httpserver/bulk.go handleBulkAddTag" + via: "literal POST route inside the gated group" + pattern: "r\\.Post\\(\"/watchlist/bulk/tags/add\", s\\.handleBulkAddTag\\)" + - from: "internal/tags/bulk.go BulkAttach" + to: "queries/bulk.sql LockWatchlistArtistsForBulkTag" + via: "qtx.LockWatchlistArtistsForBulkTag runs before qtx.BulkAttachTag inside one tx" + pattern: "qtx\\.LockWatchlistArtistsForBulkTag\\(" + - from: "internal/tags/bulk.go BulkAttach" + to: "the deferred tx.Rollback" + via: "returns without Commit when no entry carries the tag" + pattern: "tx\\.Commit\\(ctx\\)" + prohibitions: + - statement: "MUST NOT skip an at-cap artist silently: every artist the addition would push past 10 tags comes back as a `cap` skip carrying its name (D-01, SC2)." + requirement_id: BULK-02 + category: transparency + status: resolved + verification: test +--- + + +Let the server add one tag to many artists in one transaction that cannot race the 10-tag cap. The tracer bulk-adds a tag +over HTTP to a selection that mixes room, at-cap, already-tagged, and gone artists, and reads the outcome back. Task 2 pins +the concurrency and rollback guarantees D-19 demands. Task 3 amends ADR 0004 and pins the route's HTTP contract. + +Purpose: BULK-02's add half on the server. +Output: two new statements in `queries/bulk.sql`, generated code, `internal/tags/bulk.go`, the `TagStore` extension, the +handler and route, tests, and the ADR amendment. + +Locked inputs: D-01 (mechanism, named cap skips, rollback when nothing applied), D-02 (gone skips), D-04 (outcome only), +D-12 (Phase 24 normalization and get-or-create), D-19 (two required tests plus the ADR amendment), D-20 (methods on +`tags.Service`, exposed through `TagStore`). RESEARCH Pattern 1 and Examples 1–2 hold the verified SQL. Use it verbatim, +including the `t.artist_id::bigint` cast that keeps sqlc from typing the unnest column as `interface{}` (Pitfall B2). + +Tooling on this box: `make`, `sqlc`, and `golangci-lint` may be missing from the bash PATH. Use `export PATH="$HOME/go/bin:$PATH"`, +or the Makefile targets' underlying commands. `go test -race` does not work on this Windows box. New comments are 1–3 +line why-comments with at most one decision reference. + + + +@~/.claude/gsd-core/workflows/execute-plan.md +@~/.claude/gsd-core/templates/summary.md + + + +@.planning/phases/26-bulk-edit-remove/26-CONTEXT.md +@.planning/phases/26-bulk-edit-remove/26-RESEARCH.md +@.planning/phases/26-bulk-edit-remove/26-01-SUMMARY.md +@internal/tags/service.go +@internal/httpserver/tags.go +@docs/adr/0004-per-artist-tag-cap-trigger.md + + +From the repo at planning time: +- `internal/tags/service.go`: `type DB interface { sqlc.DBTX; Begin(ctx context.Context) (pgx.Tx, error) }`; `type Service struct { db DB; q *sqlc.Queries }`; `type Tag struct { ID int64; Name string }`; `func NormalizeName(raw string) (string, error)`; `Attach` (lines 91–130) is the tx scaffold: `s.db.Begin`, deferred `tx.Rollback`, `qtx := s.q.WithTx(tx)`, `qtx.GetOrCreateTag(ctx, normalized)`, `mapTagError(err)`, `tx.Commit`; `MaxTagsPerArtist = 10`. +- `internal/httpserver/tags.go`: `TagStore` (lines 56–63), `tagErrors`/`writeTagError` (34–51), `tagResponse{ID, Name}` (92–95). +- `internal/httpserver/tags_test.go`: `fakeTagStore` (lines 593–665) ends with `var _ httpserver.TagStore = fakeTagStore{}`, and `newTagsServer(t, tagsServerOpts{passphrase, store, omitTags})` (672–694). +- From plan 26-01 (`internal/httpserver/bulk.go`): `maxBulkBodyBytes`, `validateBulkIDs(ids []int64) ([]int64, error)`, `writeBulkIDsError(w, err)`. +- `internal/tags/service_test.go`: `seedEntry(t, ctx, pool, name) (entryID, artistID int64)` (lines 19–31) and `countTagsByLowerName(t, ctx, pool, name) int` (33–40). +- `internal/db/tags_schema_test.go` lines 167–240: the forced-interleaving lock-wait pattern (`txB.Conn().PgConn().PID()`, polling `pg_stat_activity`). + + + + +Created in this plan: +- SQL (`queries/bulk.sql`): `LockWatchlistArtistsForBulkTag :many` and `BulkAttachTag :many` +- Generated: `LockWatchlistArtistsForBulkTag(ctx, entryIds []int64) ([]LockWatchlistArtistsForBulkTagRow, error)`, `BulkAttachTag(ctx, BulkAttachTagParams{ArtistIds []int64; TagID int64}) ([]BulkAttachTagRow, error)`, and both `Querier` methods +- Go (`internal/tags/bulk.go`): `const SkipReasonCap = "cap"`, `const SkipReasonGone = "gone"`, `type BulkSkip struct { ID int64; Name string; Reason string }`, `type BulkAttachResult struct { Tag Tag; Applied []int64; Skipped []BulkSkip }`, `func (s *Service) BulkAttach(ctx context.Context, entryIDs []int64, name string) (BulkAttachResult, error)` +- Go (`internal/httpserver`): `TagStore.BulkAttach`; `type bulkSkipWire struct` (JSON `id`, `name,omitempty`, `reason`); `func toSkipWire(skips []tags.BulkSkip) []bulkSkipWire`; `type bulkAddTagRequest` (JSON `ids`, `name`); `type bulkAddTagResponse` (JSON `tag`, `applied`, `skipped`); `func (s *Server) handleBulkAddTag`; route `POST /watchlist/bulk/tags/add` +- Test double: `fakeTagStore.bulkAttachFunc` and `fakeTagStore.bulkAttachCalls` +- Tests: `TestBulkTags_AddEndToEnd`, `TestBulkTags_AddGated401NoCookie`, `TestBulkTags_AddGatedForbiddenWithoutCSRFHeader`, `TestBulkTags_AddUnavailableWithoutTags`, `TestBulkTags_AddReachesBulkHandler`, `TestBulkTags_AddValidation`, `TestBulkTags_AddSkipWireShape` (internal/httpserver/tags_test.go); `TestService_BulkAttach`, `TestService_BulkAttach_AllCappedLeavesNoTag`, `TestService_BulkAttach_AllGoneLeavesNoTag`, `TestService_BulkAttach_ConcurrentOverlappingNoDeadlock`, `TestService_BulkAttach_WaitsOnConcurrentTenthLink`, and the `seedLinks` helper (internal/tags/bulk_test.go) +- Doc: the "Bulk attach" amendment in ADR 0004 + +Consumed by sibling plans: 26-07 extends `internal/tags/bulk.go`, `TagStore`, and `fakeTagStore`, and reuses `bulkSkipWire`. +26-05 reuses `bulkSkipWire`/`toSkipWire`. 26-08 and 26-11 call the route from the SPA. + + + + + + Task 1: Bulk-add one tag over HTTP to a selection mixing room, cap, already-tagged, and gone artists + The dev Postgres fixture answers on localhost:5432, and plan 26-01's `validateBulkIDs` exists in `internal/httpserver/bulk.go`. + queries/bulk.sql, internal/db/sqlc/bulk.sql.go, internal/db/sqlc/querier.go, internal/tags/bulk.go, internal/httpserver/tags.go, internal/httpserver/tags_test.go, internal/httpserver/bulk.go, internal/httpserver/server.go + + - `.planning/phases/26-bulk-edit-remove/26-RESEARCH.md` Pattern 1, Examples 1, 2, and 6, and Pitfalls B2, B4, B5, and B7. + - `queries/tags.sql` lines 1–20 (`GetWatchlistArtistID`, `GetOrCreateTag`, `AttachTag`). + - `internal/tags/service.go` lines 1–130 and 281–299 (`mapTagError`). + - `internal/httpserver/tags.go` in full. + - `internal/httpserver/tags_test.go` lines 44–90 (`TestTags_AttachEndToEnd`) and 590–695 (`fakeTagStore`, `newTagsServer`). + - `internal/httpserver/bulk.go` as plan 26-01 left it. + + +Per D-01 and grill Q3, append two statements to `queries/bulk.sql`, copying RESEARCH Examples 1 and 2 verbatim: +- `LockWatchlistArtistsForBulkTag :many` selects `w.id AS entry_id, a.id AS artist_id, a.name` from watchlist joined to + artists where `w.id = ANY(@entry_ids::bigint[])`, `ORDER BY a.id FOR NO KEY UPDATE OF a`. +- `BulkAttachTag :many` is the `targets`/`had`/`ins` CTE. It inserts only where the artist lacks the tag and has fewer than + 10 links, `ON CONFLICT (artist_id, tag_id) DO NOTHING`, and returns `artist_id::bigint`, `had_tag`, and `inserted` per + target, ordered by artist id. +Each statement gets a comment of at most two lines (the lock is the ADR 0004 bulk amendment; the literal 10 mirrors +`MaxTagsPerArtist`). Regenerate with `sqlc generate`. `BulkAttachTagRow.ArtistID` must be `int64`, and `HadTag` and +`Inserted` must be `bool`. If sqlc types any of them as the empty interface, fix the SQL rather than the generated code. + +Create `internal/tags/bulk.go` with the constants and types in the artifacts section and +`BulkAttach(ctx, entryIDs, name)`: +1. Normalize the name with `NormalizeName`, returning its error before any database call. +2. With no ids, return a result with a zero `Tag` and empty non-nil slices, and no database call. +3. Begin a transaction with a deferred `Rollback` and `qtx := s.q.WithTx(tx)`. Get or create the tag (`mapTagError` on + failure), then lock with `qtx.LockWatchlistArtistsForBulkTag(ctx, entryIDs)`. +4. Every distinct requested id missing from the lock rows becomes a `BulkSkip{ID, Reason: SkipReasonGone}`. +5. If no rows were locked, skip the insert and finish at step 7. +6. Call `qtx.BulkAttachTag` with the locked artist ids and the tag id (`mapTagError` on failure). Map each result row back + to its entry through the lock rows (entry and artist are 1:1 through `watchlist_artist_id_key`). `HadTag || Inserted` + adds the entry id to `Applied`; neither adds `BulkSkip{ID, Name: , Reason: SkipReasonCap}`. +7. Sort `Applied` and `Skipped` ascending by id. If `Applied` is empty, return without committing, so the deferred rollback + discards a freshly created tag, and set the result's `Tag` to `{ID: 0, Name: }`. + Otherwise commit (`commit bulk attach tx: %w` on failure) and return the tag as stored. + +Extend `TagStore` in `internal/httpserver/tags.go` with +`BulkAttach(ctx context.Context, entryIDs []int64, name string) (tags.BulkAttachResult, error)`. In the same task, extend +`fakeTagStore` in `tags_test.go` with `bulkAttachFunc` and a `bulkAttachCalls *int32` counter (Pitfall B7). Its default +returns `Tag{ID: 1, Name: name}`, `Applied: entryIDs`, and an empty `Skipped`. + +In `internal/httpserver/bulk.go`, add the wire types: +- `bulkSkipWire` (`ID int64` as `id`, `Name string` as `name` with omitempty, `Reason string` as `reason`), and + `toSkipWire`, which never returns nil. +- `bulkAddTagRequest` (`IDs []int64` as `ids`, `Name string` as `name`). +- `bulkAddTagResponse` (`Tag tagResponse` as `tag`, `Applied []int64` as `applied`, `Skipped []bulkSkipWire` as `skipped`). + +`handleBulkAddTag` then runs in this order: +1. If `s.tags` is nil, answer 503 `tags not available`. +2. Bound the body with `MaxBytesReader(maxBulkBodyBytes)` and decode it; failure is 400 `invalid request body`. +3. Validate with `validateBulkIDs` (`writeBulkIDsError` on failure), then call `s.tags.BulkAttach`. Errors go to + `writeTagError`. +4. Answer 200 JSON with non-nil `applied`/`skipped`. + +Register `r.Post("/watchlist/bulk/tags/add", s.handleBulkAddTag)` in `registerDataRoutes` next to the 26-01 bulk route. + +In `tags_test.go`, add `TestBulkTags_AddEndToEnd` on `testutil.NewIsolatedTestPool(t, "bulk_tags_http_test")`, wired like +`TestTags_AttachEndToEnd`. Add four entries through the watchlist service: A has no tags; B holds 10 tags through ten +`tagsSvc.Attach` calls named `cap-00`…`cap-09`; C already carries `rap`; D is deleted after adding, so its id is gone. +POST `{"ids":[A,B,C,D],"name":" Rap "}`. Assert 200, `tag.name == "rap"`, `applied == [A, C]`, and `skipped == +[{id: B, name: , reason: "cap"}, {id: D, reason: "gone"}]`, in ascending id order. Then GET `/watchlist`: A and C +carry `rap`, and B still has exactly 10 tags. + + + TEST_DATABASE_URL='postgres://drop_tracker:drop_tracker@localhost:5432/drop_tracker?sslmode=disable' go test ./internal/httpserver/ -run 'TestBulkTags_AddEndToEnd' -count=1 -v + non-zero exit, a "--- FAIL" line, or "testing: warning: no tests to run" + export PATH="$HOME/go/bin:$PATH" && go build ./... && go vet ./... && git add internal/db/sqlc && make sqlc-check + non-zero exit, any vet diagnostic, or sqlc-check printing a diff under internal/db/sqlc/ + + + - `grep -v '^[[:space:]]*--' queries/bulk.sql | grep -c 'FOR NO KEY UPDATE OF a'` prints `1` (comment lines excluded). + - `grep -cE 'ArtistID +int64' internal/db/sqlc/bulk.sql.go` prints at least `2`, and `grep -c 'interface{}' internal/db/sqlc/bulk.sql.go` prints `0`. + - `grep -c 'r.Post("/watchlist/bulk/tags/add", s.handleBulkAddTag)' internal/httpserver/server.go` prints `1`. + - `grep -c 'internal/watchlist' internal/tags/bulk.go` prints `0` (tags never imports watchlist). + - `TestBulkTags_AddEndToEnd` passes with the exact `applied` and `skipped` arrays and B left at 10 tags. + + One request adds a tag where it fits, names the at-cap artist, reports the gone id, and is committed. + + + + Task 2: Pin the cap race, deadlock, idempotency, and orphan-tag guarantees + The dev Postgres fixture answers on localhost:5432. + internal/tags/bulk_test.go + + - A mixed selection (room, at cap without the tag, already carrying the tag, gone): `Applied` is room plus already-carrying, ascending. Skipped is the cap skip with the artist's name and the gone skip, ascending. The already-carrying artist's link count is unchanged. + - Exactly 9 links: applied, and the artist now has 10. Exactly 10 links already including the tag: applied, not cap. + - The same call twice: identical `Applied`, and the total `artist_tags` count unchanged by the second call. + - Name `" "`: `ErrNameRequired`, and `countTagsByLowerName` for the empty-trimmed name stays 0. + - Empty ids: nil error, a zero `Tag`, and empty non-nil slices. + - A new tag name where every selected artist is at the cap: `Applied` empty, two cap skips with names, `Tag.ID == 0`, and `countTagsByLowerName(name) == 0` (D-19). + - Every requested id gone: `Applied` empty, all `gone`, and no tag row. + - Two goroutines, four rounds, five shared artists, distinct tag names per goroutine, id lists in opposite orders, started together: every call returns nil error with all five applied, and each artist ends with 8 links (D-19, no 40P01). + - With an artist at 9 links, another transaction holds an uncommitted 10th link. `BulkAttach([that, other], "late")` blocks until that transaction commits, then returns nil error with `Applied == [other]` and a cap skip for the first artist, which keeps exactly 10 links. + + + - `internal/tags/bulk.go` as Task 1 left it. + - `internal/tags/service_test.go` lines 1–60 (imports, `seedEntry`, `countTagsByLowerName`). + - `internal/db/tags_schema_test.go` lines 167–240 (forced interleaving: pid capture and the `pg_stat_activity` wait poll). + - `.planning/phases/26-bulk-edit-remove/26-RESEARCH.md` Pitfalls B4 and B5 and the Validation Architecture rows for BULK-02. + + +Create `internal/tags/bulk_test.go` (package `tags_test`) on `testutil.NewIsolatedTestPool(t, "tags_bulk_test")` with +`tags.NewService(pool)`. Reuse `seedEntry` and `countTagsByLowerName` from `service_test.go`. Add +`seedLinks(t, ctx, pool, artistID int64, n int, prefix string)`, which inserts `n` tags named `prefix-00…` and links each to +the artist with raw SQL. + +Write `TestService_BulkAttach` with one subtest per behavior line through "Empty ids", plus the two named tests +`TestService_BulkAttach_AllCappedLeavesNoTag` and `TestService_BulkAttach_AllGoneLeavesNoTag`. + +`TestService_BulkAttach_ConcurrentOverlappingNoDeadlock` runs four rounds. Each round launches two goroutines released by a +shared closed channel, collects their errors on a buffered channel, and uses a context with a 15-second timeout. Assert +every error is nil; an error containing SQLSTATE `40P01` must fail with a message naming the deadlock. + +`TestService_BulkAttach_WaitsOnConcurrentTenthLink`: +- Insert the 10th tag outside the transaction. Begin `txA` on the pool, insert the 10th link inside it, and capture + `pidA := txA.Conn().PgConn().PID()`. +- Start `BulkAttach` in a goroutine. +- Poll `SELECT count(*) FROM pg_stat_activity WHERE $1 = ANY(pg_blocking_pids(pid))` with `pidA` every 50ms, for up to 5s, + until it is at least 1. Fail if the bulk call never blocked; that proves the pre-lock serialized behind the trigger's lock. +- Commit `txA`, then read the goroutine's result. + +No production change is expected. If a guarantee fails, fix `internal/tags/bulk.go` or the SQL within this task. + + + TEST_DATABASE_URL='postgres://drop_tracker:drop_tracker@localhost:5432/drop_tracker?sslmode=disable' go test ./internal/tags/ -run 'TestService_BulkAttach' -count=1 -v + non-zero exit, a "--- FAIL" line, "40P01" in the output, or "testing: warning: no tests to run" + + + - `grep -cE 'func TestService_BulkAttach(_AllCappedLeavesNoTag|_AllGoneLeavesNoTag|_ConcurrentOverlappingNoDeadlock|_WaitsOnConcurrentTenthLink)?\(' internal/tags/bulk_test.go` prints `5`. + - `grep -c 'pg_blocking_pids' internal/tags/bulk_test.go` prints `1`. + - `go test -v` lists at least 6 `--- PASS: TestService_BulkAttach/` subtest lines and all five named tests passing. + + D-19's two pinned cases and the cap-race, idempotency, and rollback guarantees have passing tests; committed. + + + + Task 3: Amend ADR 0004 and pin the bulk add-tag HTTP contract + docs/adr/0004-per-artist-tag-cap-trigger.md, internal/httpserver/tags_test.go + + - No session on a gated server: POST /watchlist/bulk/tags/add answers 401. A session without `X-Requested-With`: 403, with `bulkAttachCalls` still 0. + - Built without `WithTags`: 503 `tags not available`. + - On a fake store, the bulk path reaches `BulkAttach` once and never `Attach` (`attachCalls` 0). + - `{"ids":[],"name":"rap"}` gives 400 `bulk_ids_required`. 1001 ids give 400 `bulk_too_many_ids` with `max` 1000. `{"ids":[1],"name":"rap","extra":1}` gives 400. In none of these is the store called. + - Fake errors: `tags.ErrNameRequired` gives 400 `tag_name_required`, `tags.ErrNameTooLong` gives 400 `tag_name_too_long`, `tags.ErrTagCapReached` gives 409 `tag_cap_reached`, and an unmapped error gives 500 `internal error` without its text. + - A fake result with a cap skip (name `Drake`) and a gone skip encodes the cap skip with `"name":"Drake"` and the gone skip with no `name` key. A result with nil slices encodes `"applied":[]` and `"skipped":[]`. + + + - `docs/adr/0004-per-artist-tag-cap-trigger.md` in full. + - `.planning/phases/26-bulk-edit-remove/26-RESEARCH.md` Pattern 1 (the reproduced snapshot race, the lock order, and "a transaction never conflicts with itself"). + - `internal/httpserver/tags_test.go` lines 936–1005 (gated pair), plus the `fakeTagStore` changes from Task 1. + + +Per D-19, amend ADR 0004 without rewriting it. After the existing `## Consequences`, add `## Amendment (Phase 26): bulk +attach`, of about eight lines, stating: +- Bulk attach first locks the target artist rows with `FOR NO KEY UPDATE`, ordered by artist id, in its own statement. +- It takes the same lock mode the trigger takes. A transaction never conflicts with itself, so the trigger's lock is free. +- The fixed id order means two overlapping bulk attaches cannot deadlock. +- The guarded insert is a separate statement, so its link count reads a fresh READ COMMITTED snapshot after the locks are + granted. Without the pre-lock, a concurrent 10th link aborted the whole batch (reproduced in Phase 26 research). +- The trigger remains the backstop. +- When no selected artist ends up carrying the tag, the transaction rolls back, so a tag created on the fly is never + orphaned. +- Tests pin the overlapping no-deadlock case and the all-capped no-tag-row case. + +Leave the `status: accepted` front matter and every existing section unchanged. + +In `tags_test.go`, add the six named `TestBulkTags_Add*` tests from the artifacts section, one per behavior group, reusing +`newTagsServer`, `loginForSettings`, and the fake counters. Decode the skip wire with `map[string]any` so the absent +`name` key is observable. No production change is expected. If a behavior fails, fix `handleBulkAddTag` within this task. + + + TEST_DATABASE_URL='postgres://drop_tracker:drop_tracker@localhost:5432/drop_tracker?sslmode=disable' go test ./internal/httpserver/ -run 'TestBulkTags_|TestTags_' -count=1 -v + non-zero exit, a "--- FAIL" line, or "testing: warning: no tests to run" + TEST_DATABASE_URL='postgres://drop_tracker:drop_tracker@localhost:5432/drop_tracker?sslmode=disable' go test ./internal/tags/ ./internal/db/ ./internal/httpserver/ -count=1 && golangci-lint run + non-zero exit, a "FAIL" package line (including the existing ADR 0004 trigger tests in internal/db), or any golangci-lint finding + + + - `grep -c '## Amendment (Phase 26): bulk attach' docs/adr/0004-per-artist-tag-cap-trigger.md` prints `1`, and `grep -c 'status: accepted' docs/adr/0004-per-artist-tag-cap-trigger.md` prints `1`. + - `grep -cE 'func TestBulkTags_Add(EndToEnd|Gated401NoCookie|GatedForbiddenWithoutCSRFHeader|UnavailableWithoutTags|ReachesBulkHandler|Validation|SkipWireShape)\(' internal/httpserver/tags_test.go` prints `7`. + - The tags, db, and httpserver packages all pass, and golangci-lint reports nothing. + + The ADR records the bulk pre-lock, and the route's auth, CSRF, 503, precedence, validation, and wire shape are pinned; committed. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| Browser → POST /watchlist/bulk/tags/add | Untrusted ids and tag name cross into the gated group | +| tags.Service → Postgres | One transaction holding artist row locks while it inserts links under the ADR 0004 trigger | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan | +|-----------|----------|-----------|----------|-------------|-----------------| +| T-26-08 | Tampering | 10-tag cap bypass through a set-based insert | high | mitigate | Ordered `FOR NO KEY UPDATE` pre-lock plus the count-guarded insert, with the ADR 0004 trigger as backstop; `TestService_BulkAttach_WaitsOnConcurrentTenthLink` and the internal/db trigger tests | +| T-26-09 | Denial of Service | Deadlock between overlapping bulk adds | medium | mitigate | Locks always taken in artist-id order (`ORDER BY a.id`); `TestService_BulkAttach_ConcurrentOverlappingNoDeadlock` | +| T-26-10 | Tampering / Integrity | Orphan tag rows from an all-capped create | low | mitigate | Return without commit when `Applied` is empty; `TestService_BulkAttach_AllCappedLeavesNoTag` | +| T-26-11 | Spoofing / Tampering (CSRF) | The bulk add-tag route | high | mitigate | Registered inside `registerDataRoutes`; 401/403 tests assert `bulkAttachCalls == 0` | +| T-26-12 | Tampering | Tag name input | medium | mitigate | `tags.NormalizeName` before any DB call, DB CHECKs as backstop, parameterized sqlc statements only | +| T-26-13 | Information Disclosure | Artist names in `cap` skips | low | accept | Single-tenant owner; the same names are already served by GET /watchlist behind the same gate | +| T-26-14 | Denial of Service | Id/body size, lock hold time | medium | mitigate | Shared `validateBulkIDs` (≤1000) and the 64 KiB body cap; the locks live for one short transaction | + + + +- `TestBulkTags_AddEndToEnd`, every `TestService_BulkAttach*`, and every `TestBulkTags_Add*` test pass against real Postgres. +- The existing `internal/db` ADR 0004 trigger tests still pass. +- `go build ./...`, `go vet ./...`, `golangci-lint run`, and `make sqlc-check` are clean. + + + +One request adds a tag wherever it fits, names every at-cap artist, skips gone ids, never orphans a new tag, and stays +correct and deadlock-free under concurrent attaches. The ADR records why. + + + +Create `.planning/phases/26-bulk-edit-remove/26-02-SUMMARY.md` when done. + diff --git a/.planning/phases/26-bulk-edit-remove/26-02-SUMMARY.md b/.planning/phases/26-bulk-edit-remove/26-02-SUMMARY.md new file mode 100644 index 00000000..954b96c6 --- /dev/null +++ b/.planning/phases/26-bulk-edit-remove/26-02-SUMMARY.md @@ -0,0 +1,174 @@ +--- +phase: 26-bulk-edit-remove +plan: 02 +subsystem: api +tags: [go, chi, sqlc, postgres, bulk-actions, tags, locking] + +requires: + - phase: 26-bulk-edit-remove + provides: "validateBulkIDs / writeBulkIDsError / maxBulkBodyBytes, queries/bulk.sql, bulk route placement in registerDataRoutes" + - phase: 24-artist-tags-and-notes + provides: "tags.Service, GetOrCreateTag, the ADR 0004 cap trigger, TagStore/WithTags" +provides: + - "POST /watchlist/bulk/tags/add: one transaction, ordered FOR NO KEY UPDATE pre-lock, count-guarded insert; outcome {tag, applied, skipped}" + - "tags.Service.BulkAttach with SkipReasonCap/SkipReasonGone, BulkSkip, BulkAttachResult" + - "TagStore.BulkAttach, bulkSkipWire/toSkipWire reused by 26-05 and 26-07" + - "ADR 0004 bulk-attach amendment" +affects: [26-05, 26-07, 26-08, 26-11] + +actuals: + tokens: 8700 + tasks: 3 + commits: 3 +plan_head_before: 1c33bf95294aba59c5ace6dff258773135104f96 + +tech-stack: + added: [] + patterns: + - "Lock artist rows in id order in their own statement, then run the guarded set insert as a second statement so it reads a fresh READ COMMITTED snapshot" + - "Return without Commit when nothing applied so a tag created on the fly rolls back" + +key-files: + created: + - internal/tags/bulk.go + - internal/tags/bulk_test.go + modified: + - queries/bulk.sql + - internal/db/sqlc/bulk.sql.go + - internal/db/sqlc/querier.go + - internal/httpserver/bulk.go + - internal/httpserver/tags.go + - internal/httpserver/tags_test.go + - internal/httpserver/server.go + - docs/adr/0004-per-artist-tag-cap-trigger.md + +key-decisions: + - "Bulk add-tag lives on tags.Service and TagStore; watchlist.Store gains nothing and internal/tags does not import internal/watchlist" + - "All-capped or all-gone batches roll back and report tag id 0, so no orphan tags row is left (D-19)" + - "Service dedupes requested ids when computing gone skips, so callers that skip validateBulkIDs still get one skip per id" + +patterns-established: + - "Cap skips carry the artist name; gone skips omit name (omitempty) so the SPA can name only the full artists" + +requirements-completed: [BULK-02] + +coverage: + - id: D1 + description: "One request adds a tag to every selected artist with room, treats already-tagged as applied, names at-cap artists as cap skips, and reports gone ids" + requirement: BULK-02 + verification: + - kind: integration + ref: "internal/httpserver/tags_test.go#TestBulkTags_AddEndToEnd" + status: pass + - kind: integration + ref: "internal/tags/bulk_test.go#TestService_BulkAttach" + status: pass + human_judgment: false + - id: D2 + description: "All-capped and all-gone batches leave no tags row and report tag id 0" + requirement: BULK-02 + verification: + - kind: integration + ref: "internal/tags/bulk_test.go#TestService_BulkAttach_AllCappedLeavesNoTag" + status: pass + - kind: integration + ref: "internal/tags/bulk_test.go#TestService_BulkAttach_AllGoneLeavesNoTag" + status: pass + human_judgment: false + - id: D3 + description: "Overlapping bulk adds with reversed id lists never deadlock, and an add waiting behind an uncommitted 10th link skips that artist as cap" + requirement: BULK-02 + verification: + - kind: integration + ref: "internal/tags/bulk_test.go#TestService_BulkAttach_ConcurrentOverlappingNoDeadlock" + status: pass + - kind: integration + ref: "internal/tags/bulk_test.go#TestService_BulkAttach_WaitsOnConcurrentTenthLink" + status: pass + human_judgment: false + - id: D4 + description: "The route is gated (401/403, store never called), 503 without WithTags, reaches the bulk handler not /watchlist/{id}/tags, validates ids and body, maps tag errors, and encodes the skip wire shape" + requirement: BULK-02 + verification: + - kind: unit + ref: "internal/httpserver/tags_test.go#TestBulkTags_AddGated401NoCookie" + status: pass + - kind: unit + ref: "internal/httpserver/tags_test.go#TestBulkTags_AddGatedForbiddenWithoutCSRFHeader" + status: pass + - kind: unit + ref: "internal/httpserver/tags_test.go#TestBulkTags_AddValidation" + status: pass + - kind: unit + ref: "internal/httpserver/tags_test.go#TestBulkTags_AddSkipWireShape" + status: pass + human_judgment: false + +duration: 14min +completed: 2026-10-16 +status: complete +--- + +# Phase 26 Plan 02: Bulk add-tag server half Summary + +**POST /watchlist/bulk/tags/add adds one tag to many artists in a single transaction: an ordered `FOR NO KEY UPDATE` pre-lock plus a count-guarded insert keeps it race- and deadlock-free against the 10-tag cap, names every at-cap artist, skips gone ids, and rolls back so a new tag is never orphaned.** + +## Performance + +- **Duration:** ~14 min +- **Completed:** 2026-10-16 +- **Tasks:** 3 (1 tracer, 2 TDD-flagged test/doc tasks) +- **Files modified:** 10 (2 created, 8 modified) + +## Accomplishments + +- `LockWatchlistArtistsForBulkTag` and `BulkAttachTag` added to `queries/bulk.sql` from the verified RESEARCH examples; sqlc generated `ArtistID int64`, `HadTag`/`Inserted bool` with no `interface{}`. +- `tags.Service.BulkAttach` runs get-or-create, the ordered lock, then one guarded insert; entries map back through the lock rows. `HadTag || Inserted` is applied, otherwise a named `cap` skip; missing ids are `gone`. Results are sorted by id and an empty `Applied` returns without commit. +- `TagStore.BulkAttach`, wire types (`bulkSkipWire` with `name,omitempty`, never-nil `toSkipWire`) and `handleBulkAddTag` registered next to the 26-01 bulk route inside `registerDataRoutes`. +- ADR 0004 gained a "Phase 26: bulk attach" amendment; front matter and existing sections untouched. +- Tests pin the mixed room/cap/carrier/gone outcome end to end, 9 and 10 link adjacency, idempotency, blank name, empty ids, all-capped and all-gone rollback, four rounds of reversed-order concurrent adds (no 40P01), a forced wait behind a concurrent uncommitted 10th link (`pg_blocking_pids`), and the full HTTP contract. + +## Task Commits + +1. **Task 1: Bulk-add one tag over HTTP to a mixed selection (tracer)** - `4ab80d9` (feat) +2. **Task 2: Pin the cap race, deadlock, idempotency, and orphan-tag guarantees** - `1b8a8e5` (test) +3. **Task 3: Amend ADR 0004 and pin the bulk add-tag HTTP contract** - `1818a4b` (docs) + +## Verification + +- `go build ./...`, `go vet ./...`, `golangci-lint run` (0 issues); `sqlc generate` leaves `internal/db/sqlc/` clean after commit. +- Full suite `go test ./...` passes against real Postgres, including the `internal/db` ADR 0004 trigger tests; backend coverage 90.52% (gate 80%). +- Acceptance greps: one `FOR NO KEY UPDATE OF a` outside comments, `ArtistID int64` x2, zero `interface{}`, one route registration, zero `internal/watchlist` imports in `internal/tags/bulk.go`, 5 `TestService_BulkAttach*` funcs, 1 `pg_blocking_pids`, 7 `TestBulkTags_Add*` funcs, 6 `TestService_BulkAttach/` subtests passing. + +## Decisions Made + +- Followed the plan's SQL verbatim, including the `t.artist_id::bigint` cast (Pitfall B2). +- `BulkAttach` dedupes requested ids itself when computing `gone`, so it stays correct without the handler's dedupe. + +## Deviations from Plan + +None - plan executed exactly as written. + +TDD note: Tasks 2 and 3 carry `tdd="true"` but their behavior shipped in the Task 1 tracer, so the tests passed on first run and no separate RED commit was possible; no production change was needed after Task 1. The Task 3 commit is typed `docs` because it carries the ADR amendment alongside the tests. + +## Issues Encountered + +- `go test -race` is unavailable on this Windows box (no cgo); CI's Linux job is the race gate. +- `make` is not installed; Makefile target bodies (`test-integration` without `-race`, `coverage-gate`, `sqlc-check`) were run directly. +- A first attempt to append the Task 3 tests via a Python heredoc failed in the shell before writing anything; the tests were appended from a scratch file instead. + +## Known Stubs + +None. + +## Threat Flags + +None. The new route is the one in the plan's threat model (T-26-08 to T-26-14); T-26-08 to T-26-12 and T-26-14 are covered by the tests above, T-26-13 is accepted. + +## Next Phase Readiness + +Ready for 26-03 and later siblings. 26-07 can extend `internal/tags/bulk.go`, `TagStore`, and `fakeTagStore`, and reuse `bulkSkipWire`/`toSkipWire`; 26-08 and 26-11 can call `POST /watchlist/bulk/tags/add` from the SPA. + +## Self-Check: PASSED + +Created files exist (`internal/tags/bulk.go`, `internal/tags/bulk_test.go`); commits `4ab80d9`, `1b8a8e5`, `1818a4b` exist. diff --git a/.planning/phases/26-bulk-edit-remove/26-03-PLAN.md b/.planning/phases/26-bulk-edit-remove/26-03-PLAN.md new file mode 100644 index 00000000..0e216301 --- /dev/null +++ b/.planning/phases/26-bulk-edit-remove/26-03-PLAN.md @@ -0,0 +1,346 @@ +--- +phase: 26-bulk-edit-remove +plan: 03 +type: execute +wave: 2 +depends_on: [26-01] +files_modified: + - web/app/lib/api.ts + - web/app/lib/api.test.ts + - web/app/lib/format.ts + - web/app/lib/format.test.ts + - web/app/components/watchlist/WatchlistRow.tsx + - web/app/components/watchlist/BulkActionBar.tsx + - web/app/routes/watchlist.tsx + - web/app/routes/watchlist.bulk.test.tsx +autonomous: true +requirements: [BULK-01, BULK-04, BULK-05] + +estimate: + tokens: 70000 + raw_tokens: 70000 + tasks: 2 + confidence: low + +must_haves: + truths: + - "Every Watchlist card has an always-visible leading `Select {artist}` checkbox inside a 44px `