fix(booking): recheck external calendars before creating a booking

[marijnbent]

A direct booking request or a stale booking page can submit a time that an external calendar now blocks. Public, assistant, and MCP booking creation now check external availability again before writing the booking.

The check uses the slot generator's buffer orientation and shares own-Calnode-event subtraction with the slots page. Required hosts must be free; round-robin candidates are filtered; busy optional hosts are omitted. Provider failures stop the booking with HTTP 503 and a retry message, while actual conflicts return HTTP 409. Assistant and MCP callers retain that distinction without exposing provider diagnostics.

Updated with current main, including #63's incomplete-provider-check handling and #64's shared-calendar local conflict checks. Rescheduling remains outside this PR because excluding the booking's own provider event needs a separate design. The external check and database commit are not atomic.

Validation:

• pnpm install --frozen-lockfile, pnpm check, and pnpm build pass.
• go vet ./..., go build ./..., and go test ./... pass.
• Focused race checks pass for the new booking tests.
• HTTP tests cover required/collective hosts, round-robin filtering and exhaustion, optional inclusion/exclusion, and provider failure alongside a healthy provider. Rejected host-selection cases assert no booking was written.
• Six unequal-buffer/boundary cases compare booking acceptance with slots.Generate.
• Tests cover a cancelled booking with a stale external event, preservation of an adjacent external conflict, MCP error mapping with no booking written, and assistant error messages.

[github-actions]

All contributors have signed the CLA. ✅
_{Posted by the CLA Assistant Lite bot.}

[marijnbent]

I have read the CLA Document and I hereby sign the CLA

[pullfrog]

This repo's model isn't fully set up yet.

OPENAI_COMPATIBLE_MODEL env var is required when the model is set to "openai-compatible/byok". set it to the model ID served by your OpenAI-compatible endpoint (e.g. a Cloudflare AI Gateway or DashScope model). see https://docs.pullfrog.com/openai-compatible for setup.

Configure model →

  ^{｜ Rerun failed job ➔ ｜ View workflow run ｜ via Pullfrog ｜ Using OpenAI-compatible ｜ 𝕏}

[marijnbent]

recheck

[shockalotti]

@pullfrog please review this PR

[pullfrog]

Important

The pre-commit calendar check applies event buffers in the opposite orientation from slots.Generate, and it skips the §6.2 own-event subtraction that the slots page does — so it can both accept a start the page never offered and reject a start the page did offer. Since agreeing with the offered slots is the whole point of this PR, these should be fixed before merge.

Reviewed changes

• internal/handler/booking_calendar_check.go (new): a calendarFreeHosts helper that re-fetches FreeBusy per host before committing, keeps required hosts all-free, filters busy rotation candidates (round-robin), drops busy optional guests, and errors if no candidate survives.
• internal/handler/booking_handler.go: wired the new check into the shared createBookingForSlug (MCP + assistant) and the public CreateBooking handler, before bookingSvc.Create.
• internal/handler/booking_calendar_check_test.go (new): one table test asserting a busy provider and a provider error both yield 409 with no row written.
• docs/ARCHITECTURE.md: a note in §9 describing the new pre-commit check.

ℹ️ Test coverage described in the PR body is not in the diff

The description says focused tests cover "a busy required host, round-robin selection, optional hosts, and provider failure", but the added test file contains a single test with two cases (busy, unavailable). There is no round-robin, required-host-vs-rotation, optional-guest, or buffer case — which is exactly why the buffer mismatch above is invisible to CI. Once the code is aligned, those cases are what pin it.

Technical details

# Missing test cases for the new gating logic

## Affected sites
- `internal/handler/booking_calendar_check_test.go:27` — only `TestBookingRejectsExternalCalendarConflicts` with `busy`/`unavailable`; no asymmetric-buffer, round-robin filtering, required-host, or optional-guest assertions.

## Required outcome
- At least one case per branch that can change the commit decision: required host busy → 409; round-robin with one busy rotation candidate → the free candidate wins (booking created); optional guest busy → booking created without that guest; and a buffer case where `buffer_before != buffer_after` matching `slots.Generate`'s verdict.

## Suggested approach (optional)
- Reuse the `conflictCalendar` stub with distinct busy intervals per host id (it currently ignores `id`), and seed `event_type_hosts` + `buffer_before_minutes`/`buffer_after_minutes` directly so a generated-vs-offered comparison can be asserted. Mirroring `TestGenerate_bufferBefore`/`TestGenerate_bufferAfter` fixtures keeps the two engines honest.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using opencode-go/deepseek-v4.1-flash ｜ 𝕏}

[shockalotti]

The core (calendarFreeHosts: buffers on window + overlap check, per-host caching, round-robin filtering, optional-drop, 15s bound) looks sound. Requesting small error-mapping fixes before merge:

1. Provider outage currently reads as “slot taken”. In CreateBooking, every check error maps to 409 “this slot is no longer available”. A flaky provider (especially combined with the new fail-closed policy) will show bookers “slot taken” with only a Warn log for the operator. Please distinguish provider errors (e.g. 502/503 + a distinct message) from genuine busy (409).
2. Assistant/MCP path drops the mapping. createBookingForSlug propagates the raw provider error, which assistantBookError doesn't map — busy-via-recheck falls to generic “could not complete the booking” instead of the “just taken” path. Wrap provider/busy failures as errSlotUnavailable there.
3. PR description vs diff. The body claims tests cover round-robin and optional hosts, but the diff contains only TestBookingRejectsExternalCalendarConflicts (busy + unavailable, single host). Either add those cases or correct the description.

Reschedule staying on the existing path is fine and the acknowledgement is appreciated. Happy to re-review.

[marijnbent]

Hi Marijn - quick heads-up beyond the review: your Sunday batch is the lead story in our v0.10.0 release announcement (ten PRs in sixteen minutes, CLA on all of them). We would name you by your GitHub handle with a link to your profile, nothing beyond what is public here. Cool with that, or prefer we keep it low-key? Either way the code stands on its own. Thanks again for the batch. - Wynne

Hi Wyne, I can't reply to your message in the merged PR, but yes, that's fine. Thank you!

[marijnbent]

Updated in f909ee2 and merged current main.

• Corrected the buffer query window and conflict check to agree with slots.Generate.
• Extracted the own-event query shared with slot listing, including events that start before the checked window. Added cancelled-event and adjacent-conflict cases.
• Public booking now distinguishes provider failure (503, retry message) from a busy slot (409). Provider diagnostics are logged.
• Assistant/MCP provider failures now have a separate retry message. Busy checks already returned errSlotUnavailable, which both callers handled; that mapping is preserved rather than wrapping provider failures as slot conflicts.
• Added the missing required-host, round-robin, optional-host, unequal-buffer, and error-mapping tests. The PR description now lists the coverage actually present.
• fix(calendar): report incomplete availability checks #63 is included through main, so partial provider failures now stop the booking too; a healthy-plus-failing-provider test covers that behavior.

Frontend checks/build, Go vet/build/full tests, and focused booking race checks pass locally. Rescheduling remains outside this change. Ready for another review.

[pullfrog]

✅ No new issues found.

Reviewed changes (incremental delta since the prior pullfrog review at 3bb9e17; prior findings and the human request-changes points are all resolved):

• Aligned the pre-commit window with slots.Generate. calendarFreeHosts now pads to [start - buffer_after, end + buffer_before], which is exactly equivalent to the engine's expandBusy(busy, before, after) conflict rule; TestBookingCalendarBuffersMatchSlots pins the two against each other across unequal-buffer and boundary cases.
• Subtracted own Calnode events from the external busy set. ownCalendarEvents is extracted (now shared with hostAvailability) and its result removed before the overlap test, so a cancelled booking whose provider event still exists no longer rejects a slot the page offered (TestBookingCalendarCancelledOwnEvent).
• Distinguished provider failure from a busy slot. A failed free/busy fetch wraps as errCalendarUnavailable → HTTP 503 with a retry message, while genuine busy stays 409; MCP and the assistant map the sentinel without exposing provider diagnostics.
• Expanded test coverage. Host selection (required/collective, round-robin filtering, optional drop), buffer parity with the engine, cancelled own-event handling, and provider-failure mapping all assert no booking is written on rejection.
• Updated docs/ARCHITECTURE.md §9 to describe the check, its error distinction, and its non-atomicity.

  ^{｜ View workflow run ｜ Using opencode-go/deepseek-v4.1-flash ｜ 𝕏}