oficina / calendar Public

Whole-branch review — merge/native-testflight @ 9ebcd95 vs origin/main 5b9f0b6

Reviewed by Claude Fable 5.1 (claude-fable-5-1), session 21a32889, as the AGENTS.md whole-branch reviewer. Read as one diff (origin/main...origin/merge/native-testflight), in a scratch worktree; the branch was not edited.

VERDICT: APPROVE-WITH-NITS

What I checked and how

• Nothing main fixed after the fork is reverted. Files main changed since the fork (9c44bc1..5b9f0b6: 4af5cb6, 9c7f78b, docs) intersect this diff only at app.go and routes.go, and git diff origin/main origin/merge/native-testflight on those two is 17 pure insertions — no main-side line removed or altered. No migrations touched.
• app.go conflict resolution. Main's newScheduledWork/start()/h.scheduled block is byte-identical; wireNative() runs before it, after a.home, a.auth, a.db, a.cfg.Origin are all set (it needs all four). A wiring failure closes the DB and returns before the timer goroutine exists — correct.
• Reminder-wake middleware covers the native routes. /api/native/* are inside r, and mux.Handle("/", a.scheduled.middleware(r)) wraps all of r. I flipped it (mux.Handle("/", r)) and TestNativeCreateArmsTheWakeForItsReminder fails with "timer was not armed"; restored, it passes. The test exercises the fix.
• *Auth/authorisation on every `/api/native/ route.** All six sit inside the session group behind RequireSession/home.sessions.Require → requirePerson → drainOpportunistically, the same chain as the web pages. Every store call takes p.ID: EventsInRange(p.ID) (grant join), Event(p.ID, id) (grant join, 404 on no grant), CreateEvent(p.ID, …) (Calendar(p.ID, calID) + CanWrite), UpdateEvent(p.ID, …) (Writable() → ErrReadOnly 403, column allowlist so calendarid/createdby/series_id cannot move), Calendars(p.ID), DefaultCalendarID(p.ID). nativeSave additionally pins CalendarID, Kind, Status to the stored row on update, and refuses series/booked events (409) so B18 stays with the booking flow. Status codes match the web: no grant → 404, viewer grant → 403. TestNativeCalendarWriteGuards covers foreign (404), viewer (403), series (409); TestNativeCalendarRoundTrip` covers anonymous (non-200) and a foreign calendar id on create (404).
• *Native sign-in (`/native/).** nativeauth is proof-bound (32-byte challenge/verifier, SHA-256, constant-time compare, 5-minute pending window, 256 cap). authorize runs behind the same require chain; exchange re-authenticates the approved cookie through require from a clean context and mints a fresh token — the browser's token is never handed out. Its own csrf.Protect wraps the handler as well as the router's. The second sessions.New in wireNative is stateless (no goroutine, same table, cookie name derived from origin only), so the native Sessions`, the auth plugin's and Home's all read and mint the same rows — the Home fixture exercises the whole request→authorize→exchange→API path.
• Gates, verified rather than trusted. make ci exit 0 in the scratch worktree, no (cached) packages; go test -run TestNative -v shows all 8 internal/calendar native tests plus the new calendartest one actually RUN and pass. swift test on the stacked branch (same Swift sources plus one file): 6/6.
• Pre-existing flake, confirmed on clean main: internal/calendartest on origin/main 5b9f0b6, three runs: pass, TestTransientControlFailureAtBootRecovers "timer was not armed" + "this platform does not support safe scheduled work", pass. Not this branch's doing; not blocking, but it will bite CI reruns.

Findings (none blocking)

1. Nit — internal/calendar/native.go:16. A third sessions.Sessions over one table. rastrillo's own doc says "build exactly one per process and share it". Harmless today because the struct is stateless, but if a future rastrillo adds a sweeper or cache to Sessions this becomes three of them. Suggest reusing a.home.sessions when Home is on and asking rastrillo to expose the auth plugin's otherwise; or a one-line comment saying why a separate one is acceptable.
2. Nit — native.go:78 (nativeEvents). BookingSummaryFor per event is an N+1 over up to 62 days of expanded occurrences. Fine for one team; a batched "which of these ids are booked" would be one query.
3. Nit — native.go:144. A native create is always KindMeeting; edits preserve the stored kind. The web form offers meeting/commitment/note. Not wrong, just a narrower client — note it so nobody reads the API as the whole model.
4. Nit — native.go:66 / native_quick.go:130. nativeViewZone accepts any IANA name up to 128 bytes from the client. AGENTS.md bounds Zone() input on PUBLIC routes; these are signed-in, and LoadLocation refuses ../absolute paths, so it is acceptable — but it is a third place a client-supplied zone is trusted after store_zone.go and the quick parser. Worth a sentence in the comment.
5. Observation, no action. After deploy, a signed-out GET /api/native/events will answer a 303 to /signin (GET/HEAD under RequireSession), and POSTs a 401/403 — the client should treat either as "sign in again". The discussion already notes this; confirm on the deployed canary, not localhost, per AGENTS.md.

Merge-order note for #15: it is stacked on this branch; after this is squashed, merge origin/main into #15 before its own squash (R1-BRIEF, "stacked branches").