amadan
booking: plan addenda for spec revisions 4–6
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 file changed,
+61
−1
docs/superpowers/plans/2026-09-07-booking-plan.md+61 −1
diff --git a/docs/superpowers/plans/2026-09-07-booking-plan.md b/docs/superpowers/plans/2026-09-07-booking-plan.md| index f2520b4..933fb0d 100644 |
| --- a/docs/superpowers/plans/2026-09-07-booking-plan.md |
| +++ b/docs/superpowers/plans/2026-09-07-booking-plan.md |
| @@ -8,7 +8,7 @@ |
| **Tech Stack:** Go, rastrillo v0.23.0 (chi, GORM + modernc SQLite, `mail.Sender`, `csrf.Protect`, `carlos.Tick`), html/template + text/template, Playwright for the browser gate. |
| -**Spec:** `docs/superpowers/specs/2026-09-07-booking-design.md` **revision 3**. Read it first, in full, including §12. Every task cites the sections it implements. Where this plan and the spec disagree, the spec wins and this plan is wrong — fix the plan in the same commit. |
| +**Spec:** `docs/superpowers/specs/2026-09-07-booking-design.md` **revision 6**. Read it first, in full, including §12. Tasks 1–3 below were written against revision 3 and Task 1 has landed; the section **"Revision 4–6 addenda"** at the end of this plan amends Tasks 3–8 and is BINDING over the task text where they differ. Every task cites the sections it implements. Where this plan and the spec disagree, the spec wins and this plan is wrong — fix the plan in the same commit. |
| ## Global Constraints |
| @@ -527,3 +527,63 @@ Spec §11, verbatim: rows B1, B2, B4, B5, B8, B11–B17; the Frontend, two Data, |
| - **Spec coverage:** §2 → T1 (all columns, byte limits, `booking_mails`); §2.6 → T1 scoping + T4 terms; §2.7 → T6 `clientIP`; §3 → T2, T5; §4 → T3 (four-part check, `to` bounds start, notice floor, all-day via Occ, lapsed holds, ±1 day); §4.1 → T3 step 1–2; §4.2 → T3 (`day` validation lives in T6's handler; the one-query rule in T6) + T6 bucket; §5.1–5.2 → T6; §5.3–5.4 → T6 + T4 guards; §5.5 → T4 (host authority) + T6 (allowlist struct); §5.6–5.7 → T4, T6; §6 → T4 queue + T6 render/timed + T7 retry; §7 → T4 counts, T6 429s + bucket, T7 forget; §8 → T7; §9 → each Step 1; §11 → T8. |
| - **Type consistency:** `Window` once; `BookingView.Event` is the `Event` ROW (not `EventView`) so public code cannot reach opened notes by accident — the manage/ICS code builds `publicBooking` from allowlisted fields; `HoldInput.Page BookingPageView`; `MailHold…`, `ToBooker/ToHost`, `CancelledBy*` constants match the spec's column values; `RescheduleBooking` takes `rev int` and returns `changed`. |
| - **Placeholders:** none. DST expectations are pinned by observing once and recording, the repository's convention. |
| + |
| + |
| +--- |
| + |
| +## Revision 4–6 addenda (binding over the tasks above) |
| + |
| +Five review rounds after the plan was first written. Each item names the spec section that now governs. Where a task's text above disagrees with an addendum, the addendum wins; where an addendum disagrees with the spec, the spec wins. |
| + |
| +### Landed state |
| + |
| +- Task 1 is on the branch (`6f76ba4`): `models_booking.go` has `Booking.TitleSealed`, `Booking.DescriptionSealed`, `BookingMail.ClaimID`, and `DefaultBufferMin/DefaultNoticeMin/DefaultHorizonDays`. The store does NOT merge windows — callers pass `MergeWindows(ParseAvailability(...))` output. `PersonByAddressForTest` runs `EnsurePersonSetup`. `ValidateBookingPageInput` does not default zero knobs. |
| +- `Window` lives in `slots.go`. |
| + |
| +### Task 3b — the remaining calendar fixes (spec §4.1, B23) |
| + |
| +Do these in `occurrences.go` / `store_range.go` with regressions in `occurrence_test.go` BEFORE Task 4 depends on `EventsInRangeStrict`: |
| +1. `daysBetween` → difference of CIVIL dates: build both ends as `time.Date(y, m, d, 0,0,0,0, time.UTC)` and divide by 24h. Test: weekly Dublin event 2026-03-28 09:00 → 2026-03-30 09:00 still occupies 30 March on its first and on a later occurrence. |
| +2. `rangeRows` master exclusion: `events.until_utc + (events.end_utc - events.start_utc) + 90000 >= ?`. Test: `UNTIL` the night before the fold, final occurrence spanning the fold day, day query on that day shows it. |
| +3. `ExpandComplete(...) ([]Occurrence, bool)` — the existing body, returning `false` when the 100,000 guard fired; `Expand` calls it and drops the flag. `EventsInRangeStrict(ctx, personID, from, to)` — same as `EventsInRange` but returns `ErrIncompleteExpansion` (a package var) if any master's expansion was incomplete. Test: a daily rule with `start_local` in 1700 (insert via `DBForTest`) → strict errs for a 2026 window, tolerant answers. |
| + |
| +### Task 4 addenda (spec §2.4, §2.5, §5.4–5.7, §6, §8, B18, B22, B24) |
| + |
| +- **`BusyFor`** is `func (s *Store) BusyFor(ctx, host Person, from, to int64, skip map[int64]bool) ([]Busy, error)` (Task 3 agent's signature) and MUST use `EventsInRangeStrict`. `FreeSlots(ctx, page, host, now, from, to, excludeEventID)` builds `skip` = {excludeEventID} ∪ {event_id : bookings.state='held' AND hold_expires_at <= now} and calls `ComputeSlots(ruleFor(page, host), busy, now, from, to)`; a `DurationMin` override is needed for reschedule (the EVENT's duration), so give `FreeSlots` a `durationMin int` parameter (0 = the page's). |
| +- **Hold** writes `TitleSealed`/`DescriptionSealed` on the booking from the page (B20). The event title is `page.Title + " — " + name`. |
| +- **Reminder queue identity (B22):** additive migration adds `claim_id TEXT` (nullable) and `retry_at INTEGER` (nullable) to `reminder_queues` (`ReminderQueue.ClaimID *string`, `RetryAt *int64`; regenerate). `ClaimDueReminder`: eligibility `fired_at IS NULL AND due_at <= ? AND (retry_at IS NULL OR retry_at <= ?) AND (claimed_at IS NULL OR claimed_at <= ?)` with `<=`; the claim UPDATE sets `claimed_at` AND a fresh `claim_id` (16 random bytes hex) and the same predicate; `DueReminder` carries `ClaimID`. `MarkReminderSent(ctx, queueID, claimID, now)`, `FailReminder(ctx, queueID, claimID, occStart)`, new `DeferReminder(ctx, queueID, claimID, now)` (`retry_at = now+30s`, `claimed_at = NULL`, `claim_id = NULL`, attempts untouched), and `RescheduleAfterFiring(ctx, reminderID, queueID, claimID, firedStart)` are ALL `WHERE id = ? AND claim_id = ?` and check `RowsAffected`; mark + advance run in ONE transaction. `rescheduleAfter` (any path that rewrites a queue row) clears `claim_id`, `claimed_at`, `retry_at`. Never write `due_at` from a retry. `NextDueAt` reminders term: `MIN(MAX(due_at, COALESCE(retry_at,0), COALESCE(claimed_at + 300, 0)))` over `fired_at IS NULL`. |
| +- **Reschedule (B24):** inside the transaction, after the event UPDATE, for each `Reminder` on the event: recompute the queue row from the NEW start (a `tx` variant of `rescheduleAfter` — factor the body to take `*gorm.DB`), clearing `claim_id/claimed_at/retry_at`. Nothing reminder-related runs after commit. |
| +- **Booking mail claim:** `ClaimBookingMail` sets `claimed_at = now, claim_id = <random>, due_at = now + claimLease WHERE id = ? AND sent_at IS NULL AND due_at <= now` and returns the row with its `ClaimID`. `MarkBookingMailSent(ctx, id, claimID, now)`: `WHERE id=? AND claim_id=?`. `MarkBookingMailFailed(ctx, id, claimID)`: `attempts+1`, same predicate; at `MaxReminderAttempts` also `sent_at = MailGaveUp`. NEW `DeferBookingMail(ctx, id, claimID, now)`: `due_at = now+30s, claimed_at = NULL, claim_id = ''`, attempts untouched. `NextBookingMailDue` = `MIN(due_at) WHERE sent_at IS NULL`. |
| +- **Supersession (B19):** cancel and expire delete ALL unsent `booking_mails` for the booking (not just `rescheduled`); reschedule deletes unsent `rescheduled` rows before queueing. |
| +- **Host authority (B18):** `UpdateEvent` on a booked event (`bookingIDForEvent`): refuse if submitted `StartLocal`, `EndLocal`, `TZ`, `AllDay`, `Kind` or `RRule` differ from the stored row → `ValidationError{"start", "This is a booked meeting. Its time is changed by the person who booked it, or by cancelling and letting them rebook."}`; refuse `Status` other than confirmed/cancelled; refuse `Status == confirmed` when the booking is cancelled/expired → `ValidationError{"status", "This booking was cancelled. Make a new event instead."}`; refuse any update to a soft-deleted event. Replace `ErrBookedEvent` with these. `Status → cancelled` on a confirmed booking → `cancelBookingsForEvent(tx, eventID, CancelledByHost, now)`. |
| +- **`cancelBookingsForEvent`** also deletes ALL unsent mail rows for the booking and queues `(cancelled, booker, seq)` only (the host did it). |
| +- **`ExpireHolds`** deletes unsent mail rows for the expired booking too. |
| +- Tests to ADD to Task 4's list: everything in spec §9 under Reminders, Mail queue (claim identity, defer keeps attempts 0, crash-after-claim leaves NextDueAt at lease end), Zone change, and the host-edit refusals (each of the six fields, reviving, deleted event, title-only succeeds). |
| + |
| +### Task 5 addenda (spec §5.5 B25, §7.4) |
| + |
| +- `templates/event_form.html` + `readEventForm`: when the event is booked (`bookingIDForEvent`), render `start`, `end`, `all_day`, `kind`, `repeat` controls `disabled` with the B18 sentence beside them, AND emit hidden inputs `start`, `end`, `tz` (the EVENT's `TZ`), `all_day`, `kind`, `repeat` carrying the stored values. `readEventForm` uses `r.FormValue("tz")` for `TZ` when non-empty (only the booked form emits it), else `p.Zone`. Test: scriptless title-only edit succeeds, and still succeeds after the host changes their profile zone. |
| +- The event page shows a booking badge ("Booked by {name}") and, when the booking is confirmed, the sentence "To move this meeting, cancel it and let {name} rebook." |
| +- Callers pass `MergeWindows(...)` output to the store (Task 1 does not merge). |
| +- Windows: the form's availability field is optional; an empty page offers nothing. |
| + |
| +### Task 6 addenda (spec §4.2, §5.1–5.8, §6, §7.2) |
| + |
| +- **Public data:** templates receive `publicBooking` (spec §5.5) built from `BookingView`: `Title`/`Description` from the BOOKING's snapshot (`TitleSealed`/`DescriptionSealed`), never the page's or the event's. |
| +- **Semaphore (§4.2):** `var slotSem = make(chan struct{}, 4)`; public handlers `select { case slotSem <- struct{}{}: defer release; default: 503 + Retry-After: 5 }` around `FreeSlots`. `ErrIncompleteExpansion` from `FreeSlots` → 503 + `Retry-After: 60`. |
| +- **Gate (§7.2):** bucket capped at 10,000 keys, evict idle > 5 min on insert; full with no idle key → 429 + `Retry-After: 60` for a NEW key. |
| +- **`boundedSender`** replaces `timedSender`: semaphore of 4 in flight (`ErrMailBusy` immediately when full), wait `min(20s, ctx deadline remaining)` → `ErrMailTimeout`. Wrap the configured sender in `New` so REMINDERS use it too (the recorder stays outermost). `sendBookingMail`: `ErrMailBusy` → `DeferBookingMail`; other error → `MarkBookingMailFailed`; nil → `MarkBookingMailSent`. `sendReminder` callers: `ErrMailBusy` → `DeferReminder`. |
| +- **Hold response:** always 200 `book_sent`; if the immediate try errored, add the lapse sentence. NEVER `FailHold` — that function does not exist. |
| +- **ICS (§5.8):** no `CN=` on ORGANIZER/ATTENDEE; `METHOD` from effective state (booking cancelled OR event cancelled OR event deleted → CANCEL); `held`/`expired` → 404; event row read including soft-deleted. `SUMMARY` = snapshot title. |
| +- **Confirm guard** includes `now < events.start_utc` (read inside the transaction). |
| +- **Reschedule POST** carries hidden `rev`; `RescheduleBooking(..., rev, ...)` writes `tz = host.Zone` with the four time columns (B21) and repairs reminder rows in the transaction (B24). |
| +- Mail templates say the CURRENT state; the `confirmed`-to-host template opens with "This booking has since been {state}." when the booking is no longer confirmed at render time. |
| + |
| +### Task 7 addenda (spec §8) |
| + |
| +- `drain` order: `ExpireHolds(200)` → `ForgetOldIPs(500)` → reminders until `deadline/2` → booking mail until `deadline` (each send bounded by remaining budget via ctx). `DrainOnRequest`: one expiry if due, else one reminder, else one booking mail; 5-second ctx. |
| +- Tests: reminders still progress with 30 queued mail rows behind a slow sender; `DrainOnRequest` does exactly one thing; `NextDueAt` reflects `retry_at` and the lease for reminders. |
| + |
| +### Task 8 addenda |
| + |
| +AGENTS.md changes are spec §11 as of revision 6 (rows B1, B2, B4, B5, B8, B11–B25; the Frontend, Data ×3, Recurrence ×2, Ticks ×3, Time ×2 bullets; leak-list item 5); plus the calendar spec's §7 debt line about `mail.SMTP` and its context. |