docs: HANDOFF review followup + move reset review report into notes/

Append a 'Review followup' section to HANDOFF.md documenting the three
Gemini-review fixes (clock-skew toggle, snapshot race, transactional
persist) and the two nits, with file-by-file scope, verification output
(427 tests, biome clean, 0 svelte-check errors), and the design-pushback
items deliberately deferred (snapshot polling, DST drift).

Move the root-level review report (claude-report.md) into
notes/claude-reset-review.md to match the existing convention from
4e63651 (root .md files collected under notes/). Renamed from
'claude-report.md' to disambiguate from the unrelated cache-miss
notes/claude-report.md.

2 files changed, 112 insertions, 0 deletions

diff --git a/HANDOFF.md b/HANDOFF.md
index efaf30b..54fd6c4 100644
--- a/HANDOFF.md
+++ b/HANDOFF.md
@@ -309,3 +309,67 @@ route tests for the 4-slot wake schedule.)
     `recoverScheduleEntry` unit tests and the route surface end-to-end
     via HTTP tests. A follow-up could refactor `loadScheduleFromDB` to
     take a `db` parameter and write a fixture-backed integration test.
+
+---
+
+## Review followup (Gemini review pass — `notes/claude-reset-review.md`)
+
+A Gemini code-review pass after the initial 4-slot work surfaced 3 real
+bugs (2 High, 1 Medium) and 2 nits. All are now fixed on this branch.
+
+| # | Sev | Where | Symptom | Fix |
+|---|---|---|---|---|
+| 1 | High | `models.ts` POST toggle | `raw <= now` rejected legitimate toggles whenever client clock skew or request latency made an imminent slot land in the past → 400 → UI toggle silently fails | Dropped the `<= now` check; kept `Number.isFinite`. The scheduler's `recoverScheduleEntry` already fires within `MISSED_WAKE_GRACE_MS` and rolls forward. |
+| 2 | High | `ClaudeReset.svelte` | Per-hour `inFlightSeq` couldn't stop an older snapshot from clobbering a newer one when the two requests covered *different* hours (or initial-load racing a click). `applySnapshot` replaces the whole `schedule` → newest click vanishes. | Replaced per-hour counter with a single global `SnapshotSequencer` (`src/lib/snapshot-sequencer.ts`) used by `loadFromServer` AND `postToggle`. Older `accept(seq)` calls return false and are dropped. |
+| 3 | Med | `models.ts` `persistSchedule` | `DELETE` + N `INSERT`s with no transaction; an insert failure left the table empty (DELETE already committed) → schedule silently wiped on next boot, error swallowed. | Wrapped both in `db.transaction(...)`. On failure the DELETE rolls back and the previously persisted snapshot stays intact. |
+| 4 | Nit | `ClaudeReset.svelte` | `inFlightSeq` was effectively dead code for user clicks (the `pendingHours.has(hour)` early-return blocks them) but still mattered for initial-load vs first-click. | Subsumed by the new global `SnapshotSequencer` (cleaner than two parallel mechanisms). |
+| 5 | Nit | `models.ts` `schedulerTick` | Boot-recovery `reason` was masked whenever boot recovery + due slots coincided in the same tick. | Capture `bootFireRequested` before clearing the flag and append `" (boot recovery)"` to the reason. |
+
+### Files added/changed in the followup
+
+- **New:** `packages/frontend/src/lib/snapshot-sequencer.ts` — 47 LoC, the
+  reusable "most-recent request wins" race guard. Pure class, no Svelte
+  deps; usable from any component that fans out snapshot-style HTTP calls.
+- **New:** `packages/frontend/tests/snapshot-sequencer.test.ts` — 8 unit
+  tests covering the core race, the initial-load-vs-click race, monotonic
+  ordering, equal-seq idempotency, and the watermark inspector.
+- **New:** `notes/claude-reset-review.md` — the original review (kept for
+  audit trail).
+- **Modified:** `packages/api/src/routes/models.ts` — fixes #1, #3, #5
+  (~30 LoC delta).
+- **Modified:** `packages/frontend/src/lib/components/ClaudeReset.svelte`
+  — fix #2 / nit #4 (~20 LoC delta).
+- **Modified:** `packages/api/tests/routes.test.ts` — replaced the
+  "POST toggle rejects past timestamp" test with two new tests:
+  - "POST toggle ACCEPTS a slightly-past timestamp (clock skew / latency)"
+    regression-guards finding #1.
+  - "POST toggle rejects NaN / Infinity / non-number slot values" guards
+    that we still reject *malformed* inputs.
+  - Plus "snapshot remains consistent across toggle round-trips" guards
+    finding #3 (the transactional persist path).
+
+### Verification (after followup)
+
+```
+$ bun run check
+Checked 144 files in 167ms. No fixes applied.
+
+$ bun run test
+Test Files  26 passed (26)
+     Tests  427 passed (427)
+
+$ bun run --cwd packages/frontend typecheck
+svelte-check found 0 errors and 0 warnings
+```
+
+### Still deferred (not addressed in followup)
+
+These were noted in the review as design pushback rather than bugs and
+remain as documented in §"Assumptions / known gaps" above:
+
+- **Snapshot polling.** UI may show stale "Retrying…" forever if a retry
+  eventually succeeds in the background without user interaction. Adding
+  a slow 60s poll is still a one-liner; left off to avoid quiet background
+  traffic. (Documented in gap #8.)
+- **DST drift.** Adding `24h` to an absolute Unix ts ignores DST
+  transitions; documented in gap #1.
diff --git a/notes/claude-reset-review.md b/notes/claude-reset-review.md
new file mode 100644
index 0000000..f38ce39
--- /dev/null
+++ b/notes/claude-reset-review.md
@@ -0,0 +1,48 @@
+# Review: Dispatch — Claude Wake Schedule (r1/claude-reset-fix)
+
+## Executive Summary
+
+The overall architecture of this rewrite is surprisingly elegant, especially the separation of pure scheduling logic (`wake-scheduler.ts`) and the coalescing tick loop. The DB schema migration is handled correctly per the requirements, and the Svelte 5 implementation correctly avoids common reactivity pitfalls. 
+
+However, **I recommend a HOLD** on shipping this branch as-is. There are two high-severity issues: one that will cause legitimate user toggles to be rejected by the server (due to network latency/clock skew interacting with a strict time validation), and another race condition where the global UI state can be overwritten by stale data if the user interacts quickly. There is also a data-loss risk in the SQLite persistence logic due to a missing transaction boundary.
+
+## Findings
+
+### 1. Clock skew / latency rejects valid toggles (High)
+**Location:** `packages/api/src/routes/models.ts` (~line 918)
+**Issue:** When toggling an hour on, the client generates four timestamps for the slots using `nextOccurrenceAt`, which bases its math on the client's `Date.now()`. When the server receives the request, it validates each timestamp strictly: `if (... raw <= now) return 400;`. 
+If a user toggles an hour that is imminent (e.g., they click 9:00 at 8:59:59.999), or if their local clock is just a few milliseconds behind the server, the generated `9:00:00` timestamp can be slightly in the past by the time the server evaluates `Date.now()`. The server will reject the entire request with a 400 Bad Request, breaking the UI toggle.
+**Fix:** The backend scheduler's `recoverScheduleEntry` is already perfectly equipped to handle past timestamps by firing them immediately and rolling them forward. Remove the `raw <= now` validation on the POST route, or change it to allow a generous grace period (e.g., `raw <= now - MISSED_WAKE_GRACE_MS`).
+
+### 2. Race condition in global snapshot application (High)
+**Location:** `packages/frontend/src/lib/components/ClaudeReset.svelte` (`postToggle` and `loadFromServer`)
+**Issue:** The frontend replaces its entire `schedule` state whenever it receives a response from the server (`applySnapshot`). While the author added a per-hour `inFlightSeq` lock to prevent rapid clicks *on the same hour* from causing issues, this does not protect against toggles across *different* hours.
+If a user quickly clicks "9 AM" and then "10 AM", two concurrent POST requests are fired. If the network delivers the response for 9 AM *after* the response for 10 AM, the older global snapshot (which only knows about 9 AM) will blindly overwrite the UI state, causing the 10 AM mark to disappear visually. The exact same race condition exists between the initial `$effect` `loadFromServer()` call and a rapid user toggle.
+**Fix:** Use a single global `snapshotSeq` counter for all `/models/wake-schedule` responses, or implement selective merging of the returned `schedule` data into the local state rather than replacing the whole object.
+
+### 3. Missing transaction boundary causes data loss (Medium)
+**Location:** `packages/api/src/routes/models.ts` (`persistSchedule` function)
+**Issue:** The `persistSchedule` function performs a `db.run("DELETE FROM wake_schedule")` followed by a loop of `insert.run(...)`. If any insertion fails (e.g., due to disk space, bad data, or arbitrary SQLite errors), the `catch` block simply ignores the error. However, the `DELETE` has already been committed, completely wiping out the user's persistent wake schedule upon the next boot.
+**Fix:** Wrap the `DELETE` and `INSERT` loop inside a single `db.transaction(...)` so that any failure safely rolls back the deletion.
+
+### 4. Redundant concurrency logic (Nit)
+**Location:** `packages/frontend/src/lib/components/ClaudeReset.svelte` (`toggleHour` vs `postToggle`)
+**Issue:** The author notes in `HANDOFF.md` that `inFlightSeq` handles rapid out-of-order double clicks. However, they also implemented a strict UI lock: `if (pendingHours.has(hour)) return;` at the very start of `toggleHour`. This makes it impossible to dispatch a second request for the same hour until the first completes. Consequently, `inFlightSeq` is completely unreachable dead code.
+**Fix:** Remove `inFlightSeq` entirely and rely on the `pendingHours` lock, or remove the `pendingHours` early-return if optimistic UI interaction is preferred.
+
+### 5. Masked boot recovery reason (Nit)
+**Location:** `packages/api/src/routes/models.ts` (`schedulerTick`)
+**Issue:** If `needsBootFire` is true and `due.length > 0`, the `reason` string for the retry tracker correctly joins the due slot names but drops the fact that a boot recovery also occurred.
+**Fix:** Append `" (plus boot recovery)"` to the `reason` string if `needsBootFire` is true.
+
+## Design / Architectural Concerns
+
+*   **No Snapshot Polling:** As noted in the handoff, the frontend explicitly does not poll for backend state changes. This means that if a retry loop runs in the background and eventually succeeds, the UI will permanently say "Retrying..." or display a stale error state until the user interacts with the panel again or refreshes the page. A slow background poll (e.g., every 60s) or SSE feed is recommended.
+*   **DST Transition Drift:** Adding `24 * 60 * 60 * 1000` (`DAILY_INTERVAL_MS`) to an absolute Unix timestamp ignores Daylight Saving Time transitions. A scheduled 9:00 AM wake will drift to 10:00 AM or 8:00 AM when the user's local clock changes. The author documented this limitation, but it's a regression in usability vs a true calendar-aware cron.
+
+## Things the author got right
+
+*   **Pure Logic Extraction:** Extracting the math for `nextDailyAfter` and `recoverScheduleEntry` into pure functions with 100% test coverage makes the most critical parts of this feature verifiable without SQLite/Hono overhead.
+*   **Tick Coalescing:** Using the `isTickRunning` lock and advancing all due slots *before* awaiting the upstream `fireWake()` call correctly avoids re-entrancy bugs and retry storms.
+*   **Svelte 5 Reactivity:** The usage of Svelte 5 `$derived.by` for `fadedHours` and `$derived` for `currentHour` is idiomatic and correctly avoids the stale reactivity bugs that plagued the previous implementation.
+*   **Destructive Migration:** Handling the schema change by checking `PRAGMA table_info` for the missing column and dropping the table is clean and respects the boundary constraints.
\ No newline at end of file