cleanup

2026-08-22 00:34:50 +01:00
parent 95d2ee3ccf
commit 01b20b4420
6 changed files with 0 additions and 1623 deletions
@@ -1,190 +0,0 @@
 # Test Coverage Improvement Report
 **Date**: 2026-07-10
 **Overall Coverage**: 50.4% of statements
 **Test Command**: `go test -tags "test,dev" -coverprofile=coverage.out -covermode=atomic ./...`
 ---
 ## Coverage by Package
 | Package | Coverage | Status |
 |---------|----------|--------|
 | `handlers/webhooks` | 100.0% | ✅ |
 | `internal/images` | 100.0% | ✅ |
 | `internal/jobs` | 93.5% | ✅ |
 | `mw` | 89.3% | ✅ (small gaps) |
 | `internal/validators` | 82.9% | ✅ |
 | `clock` | 80.0% | ✅ |
 | `internal/square` | 74.4% | ✅ |
 | `handlers/scheduling` | 73.1% | ✅ |
 | `handlers/notifications` | 71.5% | ✅ |
 | `handlers/admin` | 68.6% | ✅ |
 | `handlers/auth` | 66.0% | ✅ |
 | `db` | 66.7% | ✅ |
 | `handlers/today` | 64.5% | ✅ |
 | `handlers/payments` | 55.4% | 🟡 Moderate |
 | `handlers/bookings` | 47.7% | 🟡 Low |
 | `handlers/portfolio` | 48.9% | 🟡 Low |
 | `auth` | 45.0% | 🟡 Low |
 | `handlers/user` | 39.1% | 🔴 Very Low |
 | `handlers/services` | 30.3% | 🔴 Very Low |
 | `crussell (root)` | 5.4% | 🔴 Critical |
 | `internal/dav` | 0.0% | ⚫ Zero |
 | `internal/logutil` | 0.0% | ⚫ Zero |
 | `internal/s3` | 0.0% | ⚫ Zero |
 | `internal/zxcvbnjs` | 0.0% | ⚫ Zero |
 ---
 ## Critical Finding: Cross-Package Coverage Blind Spot
 **Several functions show 0% in their own package but HAVE tests in the `admin` package.** Go's per-package coverage (`go test ./handlers/services/`) only counts tests within that package. Cross-package test calls (from `handlers/admin/`) don't count.
 Affected handlers (all tested in `handlers/admin/` but show 0% in their own package):
 | Function | Actual Coverage | Where Tested |
 |----------|----------------|-------------|
 | `handlers/services`: ToggleService, CreateServiceHandler, DeleteServiceHandler, AllServicesHandler | ✅ Tested | `admin/services_test.go` |
 | `handlers/user`: GetAdminUserHandler, ListAdminUsersHandler, GetEligiblePatchTestServicesHandler, AddPatchTestHandler | ✅ Tested | `admin/users_test.go` |
 | `handlers/bookings`: GetAllAdminBookingsHandler, GetAdminBookingHandler, GetAllBookingsByUserHandler, SearchAdminBookingsHandler, etc. | ❌ **Untested** | No admin tests written |
 | `handlers/bookings`: AdminListPendingBookingsHandler, AdminGetInProgressBookingHandler | ❌ **Dead code** | No routes registered in main.go |
 ---
 ## Priority 1: Immediate Wins (<30 min each, no new dependencies)
 ### 1.1 `internal/logutil/logutil_test.go` — 70 lines, pure stdlib
 **Functions**: `ColoredDuration(d time.Duration) string`, `ColoredRows(n int) string`
 **What to test**:
 - `ColoredDuration`: 3 branches (<500ms → green, <5s → yellow, >=5s → red)
 - `ColoredRows`: singular (n=1) vs plural (n≠1)
 - `NO_COLOR` env var behavior (ANSI codes vs empty strings)
 **Lines added**: ~40
 ### 1.2 `internal/zxcvbnjs/zxcvbn_test.go` — 68 Go lines, 1 public function
 **Function**: `Score(password string) (int, error)`
 **What to test**:
 - Known weak passwords (`"password"`, `"123456"`) → score 0-1
 - Strong passwords → score 3-4
 - Empty string → handle gracefully
 - `sync.Once` lazy init works across repeated calls
 **Lines added**: ~30
 ### 1.3 `auth/jwt_test.go` — Refresh token functions (already have handler-level tests)
 **Functions**: `generateRefreshTokenString()`, `GenerateRefreshToken(ctx, userID, role)`, `VerifyRefreshToken(ctx, tokenString)`
 **What to test**:
 - `generateRefreshTokenString`: format (64-char hex) + uniqueness (100 calls)
 - `GenerateRefreshToken`: stores in DB, returns non-empty token
 - `VerifyRefreshToken`: verify + token rotation (2nd call fails), expired, revoked
 **Pattern**: Already works in `handlers/auth/auth_test.go:TestRefreshToken_Generation` — adapt for package-level
 **Test infra**: `auth/testmain_test.go` already sets up `InitJWT` + test DB pool — no setup needed
 ### 1.4 `handlers/bookings/repo_test.go` — 4 trivial DB helpers (~5 lines each)
 **Functions**: `GetBookingStatus`, `GetBookingStartTime`, `BookingExists`, `CountUserBookingsInStatus`
 **What to test**:
 - Happy path: insert booking → call function → assert result
 - Not-found: non-existent ID → assert error
 **Pattern**: `ctx, tx := testutils.SetupTestTx(t)` → `fixtures.CreateTestUser(tx)` → `fixtures.CreateTestService(tx)` → `fixtures.CreateTestBooking(tx, userID, svcID)` → call function directly
 ### 1.5 `mw/response_test.go` — Trivial helpers
 **Functions**: `RespondJSON(w, status, data)`, `RespondError(w, status, message)`
 **What to test**:
 - `RespondJSON`: basic write, error encoding, nil data
 - `RespondError`: confirm JSON shape `{"error": "..."}`
 - **Action**: `RespondError` is dead code (never called anywhere) — either remove it or test + start using it
 **Ease**: 5-line pure functions, `httptest.ResponseRecorder` + standard assertions
 ### 1.6 `internal/dav/types_test.go` — Pure string builders
 **Functions**: `GenerateICalEvent(EventInput) string`, `GenerateVCard(ContactInput) string`
 **What to test**:
 - All-day vs timed events, attendees list, special characters (iCal escaping)
 - vCard formatting with various field combinations
 - Edge cases: empty fields, long strings
 **No DB needed**: Pure functions, no dependencies
 ---
 ## Priority 2: Medium Effort (follow existing test patterns, 1-2h each)
 ### 2.1 `handlers/services/services_test.go` — Jump 30% → ~90%
 **What**: Move/add in-package tests for `ToggleService`, `CreateServiceHandler`, `DeleteServiceHandler`, `AllServicesHandler`
 **Why**: These are already tested in `handlers/admin/services_test.go` — just need to replicate in the `services` package
 **Pattern**: Add helper `makeAdminContextRequest` (5 lines setting `mw.UserIDKey` + `mw.UserRoleKey` on chi context like `admin/test_helpers.go`)
 **Bonus**: Remove dead code on lines 200-208 of `services.go` (unreachable after unconditional `return`)
 ### 2.2 `handlers/user/*_test.go` — Jump 39% → ~55%
 **What**: Add in-package tests for `GetAdminUserHandler`, `ListAdminUsersHandler`, `GetEligiblePatchTestServicesHandler`, `AddPatchTestHandler`
 **Why**: Same cross-package issue — tested in `admin/users_test.go`
 **Also add**:
 - `CreateGuestUserHandler` success path (currently only validation failure tests)
 - `processProfileImage` unit test with a real JPEG file in `testdata/`
 - `GetAdminUserHandler` error paths (not-found, social login query error)
 ### 2.3 `handlers/bookings/admin_*_test.go` — Admin GET handler tests
 **What**: Add HTTP handler tests for uncovered admin GET handlers in `bookings.go`:
 - `GetAllAdminBookingsHandler`, `GetAdminBookingHandler`, `GetAllBookingsByUserHandler`
 - `SearchAdminBookingsHandler`, `GetOverlappingBookings*`, `GetBookingsBy*Range*`
 **Pattern**: `serveChiHandler` with manual admin context injection (already used in `overlap_test.go` and `admin_reserve_test.go`)
 **Verify**: `AdminListPendingBookingsHandler` and `AdminGetInProgressBookingHandler` — confirmed unreachable (no routes). Either remove or add routes + tests.
 ### 2.4 `mw/ratelimit_test.go` — Middleware wrapper tests
 **Functions** (dev stubs): `ProgressiveRateLimit`, `RateLimit`
 **What to test**: Wrap handler with middleware in dev mode, verify pass-through behavior
 **Pattern**: `httptest.NewRecorder` + `httptest.NewRequest` + standard HTTP test
 ---
 ## Priority 3: Heavier Effort (needs mocks or refactoring, 2-4h each)
 ### 3.1 `handlers/portfolio/images_test.go` — UploadImage (currently 7.5%)
 **Blockers**: `s3.Client` is nil in tests → handler returns "Storage not configured" early
 **Fix**: Mock `s3.Uploader` interface (already exists), set `s3.Client = &mockUploader{}` in test setup
 **What to test**: Multi-part form with JPEG data, verify 200, verify DB record inserted with correct URLs
 **Also**: `ListFilters` 31.7% — add tests for `?filter[category]=value` (dual-query path), `?tag=forest`, combined filters
 ### 3.2 `handlers/user/profile_test.go` — UploadProfilePictureHandler (8.8%)
 **Blocker**: Same S3 nil issue as portfolio
 **Fix**: Same mock approach — `s3.Client = &mockUploader{}`
 **Also**: `updateCardDAV` (11.1%) — currently requires `DAV_BASE_URL` env var and HTTP server. Mock the function or use `httptest.NewServer`.
 ### 3.3 `internal/dav/shared_test.go` — DB-backed CardDAV operations
 **Functions**: 14 `BaseService` methods (CRUD for calendars, contacts, events)
 **Requires**: PostgreSQL test DB pool (existing pattern in `testutils/testdb/`)
 **Pattern**: Create test fixtures (calendar, contact records), call methods, assert DB state
 **Note**: Importing `dav` package triggers `service_dev.go`'s `init()` which tries to connect to Postgres. Either set `GO_TESTING` env var or construct `BaseService` directly.
 ### 3.4 `internal/s3/s3_test.go` — S3 storage
 **Approach**: 
 - Unit tests via `Uploader` interface mock
 - `Connect()` integration test requires local S3 (RustFS/MinIO) → tag `//go:build integration`
 - `GetURL` URL-builder logic can be tested independently (currently 0% on both build tags)
 ---
 ## Dead Code Found During Investigation
 | Location | Function | Status |
 |----------|----------|--------|
 | `mw/response.go:19` | `RespondError` | **Defined but NEVER called** — remove or wire up |
 | `bookings/manage.go:309` | `AdminListPendingBookingsHandler` | **No route registered** in main.go |
 | `bookings/manage.go:320` | `AdminGetInProgressBookingHandler` | **No route registered** in main.go |
 | `services/services.go:200-208` | Duplicate-key error block | **Unreachable** — after unconditional `return` on line 198 |
 ---
 ## Summary: Coverage Impact by Action
 | Action | Estimated Coverage Gain |
 |--------|----------------------|
 | Fix cross-package gap (services) | +15-20% in `handlers/services` |
 | Fix cross-package gap (user admin handlers) | +10-15% in `handlers/user` |
 | Add repo.go tests | +2-3% in `handlers/bookings` |
 | Add admin booking GET handler tests | +15-20% in `handlers/bookings` |
 | Auth refresh token tests | +15-20% in `auth` |
 | logutil + zxcvbnjs tests | 0% → 80-100% in those packages |
 | RespondJSON test | 0% → 100% in `mw/response.go` |
 | Portfolio S3 mock + tests | 7.5% → 60-70% in `handlers/portfolio` |
 | dav/types pure func tests | 0% → ~25% in `internal/dav` |
 | Remove dead code | Removes false-negative 0% entries |
 | **Total estimated improvement** | **~50% → ~65-70% overall** |
@@ -1,431 +0,0 @@
 # Plan: Consolidated `jobs` Package for Background Cron Tasks
 **Goal:** Extract 9 side-effect cleanup functions from `GetAvailableHours` HTTP handler into a dedicated `jobs` package with cron-scheduled, parallel execution. Remove the lazy-cleanup antipattern.
 **Current state:**
 - 9 cleanup functions in `backend/handlers/scheduling/time-blockers.go` — all called synchronously in `default-hours.go:364-407` on every `GET /available-hours` request
 - Only `CleanupOldReservations` ALSO runs on a background goroutine (`main.go:432-450`, 5min ticker)
 - 4 other ad-hoc background goroutines exist (JWT cleanup, GDPR cache, login state, rate limiter)
 - `robfig/cron/v3` v3.0.1 is already a `go.mod` dependency (currently used only for parsing time-blocker cron expressions)
 ---
 ## Phase 1: Create `backend/internal/jobs/` Package
 **New package:** `crussell/internal/jobs`
 ### File: `scheduler.go`
 Core scheduler abstraction built on `robfig/cron/v3`:
 ```go
 package jobs
 import (
    "context"
    "log"
    "time"
    "github.com/robfig/cron/v3"
 )
 // Job defines a single periodic task.
 type Job struct {
    Name        string                         // Human-readable name (for logging)
    Schedule    string                         // Standard cron expression ("*/5 * * * *")
    Timeout     time.Duration                  // Per-execution timeout
    Concurrency int                            // Max concurrent runs (0 = unlimited, 1 = serial)
    Handler     func(context.Context) error     // The actual work
 }
 // Scheduler manages all registered cron jobs with parallel execution.
 type Scheduler struct {
    cron       *cron.Cron
    entries    []cron.EntryID
    registry   []Job
    baseCtx    context.Context
    cancel     context.CancelFunc
    semaphores map[string]chan struct{}     // Per-job concurrency limit
 }
 ```
 **Key design decisions:**
 1. Each job runs in its own goroutine (cron v3 default) — parallel by nature
 2. Per-job concurrency control via channel semaphore — prevents overlapping runs of the same job
 3. Panic recovery wrapper — matches existing pattern in `jwt.go`, `gdpr_export.go`, `auth/local.go`
 4. Timeout via `context.WithTimeout` — matches existing `main.go` pattern
 5. Graceful shutdown via `baseCtx` — the cron scheduler is stopped, then active jobs drain
 ```go
 func New() *Scheduler {
    ctx, cancel := context.WithCancel(context.Background())
    return &Scheduler{
        cron:       cron.New(cron.WithLocation(londonLocation)),
        baseCtx:    ctx,
        cancel:     cancel,
        semaphores: make(map[string]chan struct{}),
    }
 }
 func (s *Scheduler) Register(job Job) {
    // Validate cron expression at registration time
    parser := cron.NewParser(cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow)
    if _, err := parser.Parse(job.Schedule); err != nil {
        log.Fatalf("jobs: invalid cron schedule %q for job %q: %v", job.Schedule, job.Name, err)
    }
    s.registry = append(s.registry, job)
 }
 func (s *Scheduler) Start() {
    for _, job := range s.registry {
        j := job // capture
        entryID, err := s.cron.AddFunc(j.Schedule, s.wrapJob(j))
        if err != nil {
            log.Fatalf("jobs: failed to register %q: %v", j.Name, err)
        }
        s.entries = append(s.entries, entryID)
    }
    s.cron.Start()
 }
 func (s *Scheduler) Shutdown() <-chan struct{} {
    ctx := s.cron.Stop() // Stop scheduler (returns ctx that completes when all jobs finish)
    s.cancel()           // Cancel base context so in-flight jobs know to stop
    return ctx.Done()    // Caller can wait for this
 }
 // wrapJob adds panic recovery, timeout, concurrency control, and logging.
 func (s *Scheduler) wrapJob(job Job) func() {
    // Set up semaphore if concurrency limited
    var sem chan struct{}
    if job.Concurrency > 0 {
        sem = make(chan struct{}, job.Concurrency)
        sem <- struct{}{} // Initial slot filled
    }
    return func() {
        // Concurrency guard
        if sem != nil {
            select {
            case <-sem:
                // Acquired — proceed
            default:
                log.Printf("jobs: %q skipped (previous run still in progress)", job.Name)
                return
            }
            defer func() { sem <- struct{}{} }()
        }
        // Panic recovery
        defer func() {
            if r := recover(); r != nil {
                log.Printf("jobs: panic recovered in %q: %v", job.Name, r)
            }
        }()
        // Timeout
        ctx, cancel := context.WithTimeout(s.baseCtx, job.Timeout)
        defer cancel()
        start := time.Now()
        if err := job.Handler(ctx); err != nil {
            log.Printf("jobs: %q failed: %v (duration: %v)", job.Name, err, time.Since(start))
        } else {
            log.Printf("jobs: %q completed (duration: %v)", job.Name, time.Since(start))
        }
    }
 }
 ```
 ### File: `cleanup.go`
 Registration of all 9 scheduling cleanup functions + JWT cleanup.
 ```go
 package jobs
 import (
    "context"
    "crussell/auth"
    "crussell/handlers/scheduling"
    "time"
 )
 // RegisterAll registers every background job.
 func RegisterAll(s *Scheduler) {
    // === HIGH FREQUENCY (every 5 min) ===
    s.Register(Job{
        Name:        "cleanup-reservations",
        Schedule:    "*/5 * * * *",
        Timeout:     30 * time.Second,
        Concurrency: 1,
        Handler:     scheduling.CleanupOldReservations,
    })
    s.Register(Job{
        Name:        "cleanup-expired-deposits",
        Schedule:    "*/5 * * * *",
        Timeout:     30 * time.Second,
        Concurrency: 1,
        Handler:     scheduling.CleanupExpiredDeposits,
    })
    // === MID FREQUENCY (hourly) ===
    s.Register(Job{
        Name:        "cleanup-expired-loyalty-redemptions",
        Schedule:    "0 * * * *",
        Timeout:     30 * time.Second,
        Concurrency: 1,
        Handler:     scheduling.CleanupExpiredLoyaltyRedemptions,
    })
    s.Register(Job{
        Name:        "cleanup-old-idempotency-keys",
        Schedule:    "0 * * * *",
        Timeout:     30 * time.Second,
        Concurrency: 1,
        Handler:     scheduling.CleanupOldIdempotencyKeys,
    })
    s.Register(Job{
        Name:        "cleanup-revoked-jtis",
        Schedule:    "0 * * * *",
        Timeout:     30 * time.Second,
        Concurrency: 1,
        Handler: func(ctx context.Context) error {
            auth.CleanupRevokedJTIs(ctx)
            return nil
        },
    })
    // === LOW FREQUENCY (daily, off-peak) ===
    s.Register(Job{
        Name:        "anonymize-stale-guest-accounts",
        Schedule:    "0 3 * * *",           // 3am
        Timeout:     5 * time.Minute,
        Concurrency: 1,
        Handler:     scheduling.AnonymizeStaleGuestAccounts,
    })
    s.Register(Job{
        Name:        "cleanup-expired-financial-records",
        Schedule:    "0 4 * * *",           // 4am
        Timeout:     10 * time.Minute,
        Concurrency: 1,
        Handler:     scheduling.CleanupExpiredFinancialRecords,
    })
    s.Register(Job{
        Name:        "cleanup-expired-gift-cards",
        Schedule:    "0 5 * * *",           // 5am
        Timeout:     5 * time.Minute,
        Concurrency: 1,
        Handler:     scheduling.CleanupExpiredGiftCards,
    })
    s.Register(Job{
        Name:        "cleanup-idle-accounts",
        Schedule:    "30 3 * * *",          // 3:30am (offset from financial)
        Timeout:     5 * time.Minute,
        Concurrency: 1,
        Handler:     scheduling.CleanupIdleAccounts,
    })
    s.Register(Job{
        Name:        "cleanup-old-name-history",
        Schedule:    "30 4 * * *",          // 4:30am
        Timeout:     30 * time.Second,
        Concurrency: 1,
        Handler:     scheduling.CleanupOldNameHistory,
    })
 }
 ```
 **Staggering rationale:** Daily jobs at 3am/3:30am/4am/4:30am/5am spread the load so heavy operations (financial aggregation, idle account scans) don't contend with each other or with the hourly jobs. High-frequency jobs (reservations, deposits) are decoupled on their own 5-min schedules.
 ---
 ## Phase 2: Wire into `main.go`
 ### Before:
 ```go
 // main.go:142
 auth.StartJTICleanup()
 // main.go:432-450 (background goroutine for reservations)
 cleanupCtx, cleanupStop := context.WithCancel(context.Background())
 go func() {
    ticker := time.NewTicker(reservationCleanupInterval)
    ...
 }()
 // main.go:457-468 (shutdown)
 cleanupStop()
 ```
 ### After:
 ```go
 // At package level, after constants but before init()
 var sched *jobs.Scheduler
 // In main(), after initDB()/initSquare()/etc but before r := chi.NewRouter():
 sched = jobs.New()
 jobs.RegisterAll(sched)
 sched.Start()
 // In main(), shutdown block (replace the old cleanupStop() call):
 quit := make(chan os.Signal, 1)
 signal.Notify(quit, syscall.SIGTERM, syscall.SIGINT)
 go func() {
    <-quit
    log.Println("Shutting down server...")
    // Graceful HTTP shutdown
    ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
    defer cancel()
    if err := srv.Shutdown(ctx); err != nil {
        log.Printf("Server forced to shutdown: %v", err)
    }
    // Graceful job scheduler shutdown
    <-sched.Shutdown()
    log.Println("Background jobs stopped")
 }()
 ```
 **Remove:**
 - `reservationCleanupInterval` and `reservationCleanupTimeout` constants (`main.go:71-72`)
 - `cleanupCtx`/`cleanupStop` variables (`main.go:433`)
 - The goroutine block (`main.go:434-450`)
 - Import `scheduling` from `main.go` (no longer needed there)
 - `auth.StartJTICleanup()` call (`main.go:142`)
 ---
 ## Phase 3: Remove Side-Effects from `GetAvailableHours`
 **File: `backend/handlers/scheduling/default-hours.go`**
 Delete lines 364-407 (the entire cleanup block):
 ```diff
 -	// Clean up old reservations (older than 1 hour)
 -	if err := CleanupOldReservations(r.Context()); err != nil {
 -		log.Printf("Failed to cleanup old reservations: %v", err)
 -	}
 -
 -	// Anonymize stale guest accounts (6+ months after last booking)
 -	if err := AnonymizeStaleGuestAccounts(r.Context()); err != nil {
 -		log.Printf("Failed to anonymize stale guest accounts: %v", err)
 -	}
 -
 -	// Clean up expired loyalty redemptions (pending past expires_at)
 -	if err := CleanupExpiredLoyaltyRedemptions(r.Context()); err != nil {
 -		log.Printf("Failed to cleanup expired loyalty redemptions: %v", err)
 -	}
 -
 -	// Clean up expired financial records (aggregate + delete granular data)
 -	if err := CleanupExpiredFinancialRecords(r.Context()); err != nil {
 -		log.Printf("Failed to cleanup expired financial records: %v", err)
 -	}
 -
 -	// Clean up bookings past deposit deadline (no deposit paid)
 -	if err := CleanupExpiredDeposits(r.Context()); err != nil {
 -		log.Printf("Failed to cleanup expired deposits: %v", err)
 -	}
 -
 -	// Clean up expired gift cards (unused for 24+ months)
 -	if err := CleanupExpiredGiftCards(r.Context()); err != nil {
 -		log.Printf("Failed to cleanup expired gift cards: %v", err)
 -	}
 -
 -	// Clean up idle accounts (2yr no money, 5yr with money)
 -	if err := CleanupIdleAccounts(r.Context()); err != nil {
 -		log.Printf("Failed to cleanup idle accounts: %v", err)
 -	}
 -
 -	// Clean up old idempotency keys (24h+ and non-pending)
 -	if err := CleanupOldIdempotencyKeys(r.Context()); err != nil {
 -		log.Printf("Failed to cleanup old idempotency keys: %v", err)
 -	}
 -
 -	// Clean up old name history (6+ months)
 -	if err := CleanupOldNameHistory(r.Context()); err != nil {
 -		log.Printf("Failed to cleanup old name history: %v", err)
 -	}
 ```
 Also remove `"log"` import from `default-hours.go` if it becomes unused after removal.
 ---
 ## Phase 4: Remove `auth.StartJTICleanup()` (Optional but Recommended)
 **File: `backend/auth/jwt.go`**
 - Keep `CleanupRevokedJTIs(ctx)` — it's used by tests and now by the jobs package
 - Delete `StartJTICleanup()` entirely (no longer needed, replaced by the jobs package)
 - Remove the ticker goroutine
 ---
 ## Phase 5: Update Documentation
 **File: `obsidian/Crussell/Technical Manual.md`**
 - Update lines ~545 and ~999: change "lazy cleanup triggered on availability fetch" to "all cleanup runs on cron schedules via the `jobs` package"
 - Remove the lazy-cleanup justification
 - Add a new section documenting the `jobs` package and its job registry
 **File: `obsidian/Crussell/Future Work - Gap Backlog.md`**
 - Mark item #2 as fully complete (strikethrough)
 - Remove the note about remaining functions still running on `GET /api/availability`
 **File: `README.md`** (optional)
 - Add a line about the background job scheduler
 ---
 ## Effort Estimate
 | Step | Files Changed | Effort |
 |------|--------------|--------|
 | Phase 1a: `scheduler.go` | 1 new file | 2-3h |
 | Phase 1b: `cleanup.go` | 1 new file | 1h |
 | Phase 2: Wire `main.go` | 1 file | 30min |
 | Phase 3: Remove side-effects from handler | 1 file | 15min |
 | Phase 4: Remove `StartJTICleanup` | 1 file | 15min |
 | Phase 5: Update docs | 2-3 files | 30min |
 | Testing & go vet | — | 1h |
 | **Total** | **4-5 files (+2 new)** | **~6h** |
 ---
 ## Risks & Mitigations
 | Risk | Mitigation |
 |------|------------|
 | **Concurrent DB load** from multiple jobs at the same cron tick | Stagger daily jobs across 3am-5am range. High-frequency jobs (5min) are lightweight. Per-job `Concurrency: 1` prevents overlapping runs. |
 | **Job takes longer than interval** (e.g., financial aggregation >5min while scheduled every 5min) | `Concurrency: 1` + skip-logic: if previous run still in-flight, the new invocation is skipped and logged. |
 | **Cron expression parsing differs** from existing time-blocker parser | Both use `cron.NewParser(cron.Minute \| cron.Hour \| cron.Dom \| cron.Month \| cron.Dow)` — identical. |
 | **Tests expect cleanup side-effects** from `GetAvailableHours` | Search for tests that rely on the side-effect calls. Some may need an explicit cleanup call before assertions. |
 | **`CleanupExpiredDeposits` timing gap** — deposit-lapsed slots not freed until next cron tick | 5-min frequency is acceptable. The previous behavior freed on the *next* availability fetch, which could be minutes or hours apart depending on user activity. 5-min max latency is actually *better* than the lazy approach during quiet periods. |
 | **Panic in one job takes down the scheduler** | `wrapJob` has defer-recover. One job's panic cannot affect others (separate goroutines). |
 ---
 ## Edge Cases & Exclusions
 **Not in scope (Phase 1):**
 - `gdpr_export.go:init()` — GDPR cache cleanup (5min ticker). Tightly coupled to package-level state. Leave as-is.
 - `auth/local.go:init()` — Login state cleanup. Tightly coupled. Leave as-is.
 - `mw/ratelimit.go` goroutines — Rate limiter cleanup. Leave as-is.
 - These can be migrated to the scheduler in a follow-up if desired, but require more refactoring.
 **Not a concern:**
 - `backend/handlers/scheduling/time-blockers.go` already uses `robfig/cron/v3` — no new dependency, no version conflict.
 - All 9 cleanup functions have signature `func(context.Context) error` — perfectly uniform for the `Job.Handler` type.
 - The `scheduling` package is already imported in `main.go` — no new import needed for that.
 **Key invariant:** All cleanup functions are idempotent (documented in code). Running them on cron instead of on-demand has zero correctness impact — they produce the same result regardless of how often they run.
@@ -1,397 +0,0 @@
 # Holiday Hours Conflict Resolution Plan
 ## Overview
 Add a conflict resolution flow to the "Create Exception Schedule" modal (similar to the Time Blocker flow) that prevents saving holiday hours until all affected client bookings have been moved out of the newly-closed/rescheduled time periods.
 ---
 ## Key Differences from Time Blocker Flow
 | Aspect | Time Blockers | Holiday Hours |
 |---|---|---|
 | Scope | Single day, specific time range | Entire weeks, full daily schedules |
 | Available-for-reschedule logic | Same working hours apply (no change) | **Dual mode**: For same-week affected days → use **new holiday hours**; for other weeks → use **default hours** |
 | Bypass mechanism | N/A (blocker is hard block) | `out_of_hours` flag on admin booking create/reserve |
 | Existing conflict check | Client-side overlap against one date | Need per-week overlap against up to N weeks |
 | Placeholder blocker pattern | `RESERVATION:placeholder:{bookingId}` | New pattern needed (likely holiday-specific placeholder) |
 ---
 ## Files to Modify
 ### Frontend
 1. **`frontend/src/lib/components/admin/HolidayHours.svelte`** — Add conflict detection UI
 2. **`frontend/src/routes/admin/+page.svelte`** — Wire `openUserModal`, `openBookingModal`, `rescheduleVersion` props to `HolidayHours`
 ### Backend
 3. **`backend/handlers/scheduling/exceptional-hours.go`** — Add conflict-checking API or expand existing
 4. **`backend/handlers/scheduling/default-hours.go`** — Add "preview" mode to GetAvailableHours that accepts proposed exceptional hours for conflict-specific available-slot preview
 5. **`backend/main.go`** — Register new route(s)
 ---
 ## Frontend Design
 ### A. Props Interface (HolidayHours.svelte)
 ```typescript
 interface Props {
    openUserModal?: (userId: string) => void;
    openBookingModal?: (bookingId: string) => void;
    rescheduleVersion?: number;
 }
 ```
 Wire these from `admin/+page.svelte`:
 ```svelte
 <HolidayHours {openUserModal} {openBookingModal} {rescheduleVersion} />
 ```
 ### B. New State Variables
 ```typescript
 let overlappingBookings = $state<OverlappingBooking[]>([]);
 let checkingOverlap = $state(false);
 let hasOverlap = $state(false);
 let conflictWeeks = $state<Map<string, OverlappingBooking[]>>(new Map());
 type OverlappingBooking = {
    id: string;
    start_time: string;
    duration_minutes: number;
    status: string;
    user: { id: string; full_name: string; email: string | null; phone: string | null } | null;
    services: string[];
    week_start: string;        // Which Monday this booking falls in
    day_of_week: number;        // 0=Mon..6=Sun
    is_same_week_as_change: boolean; // Whether this week is being modified
 };
 ```
 ### C. Guard Condition
 ```typescript
 const canSave = $derived.by(() => {
    return isFormValid && !hasOverlap && !checkingOverlap;
 });
 ```
 ### D. Conflict Checking Flow
 When admin opens the modal or changes the week selection:
 1. **Collect all affected dates**: For each week_start in `weekStarts`, expand to the 7 days (Mon-Sun). Figure out which days have changes (different hours from default, or newly closed).
 2. **Fetch conflicting bookings**: Call a new backend endpoint:
   ```
   GET /api/admin/bookings/conflicting-for-exception
       ?week_starts[]=2026-08-03&week_starts[]=2026-08-10
       &proposed_hours=[{weekday:1,start:"10:00",end:"18:00",isOpen:true},...]
   ```
   This returns ONLY bookings that fall within the affected hours (e.g., if Mon is now closed, all Monday bookings in that week conflict; if Mon hours changed from 9-5 to 12-8, bookings at 9am-12pm conflict since they'd be in the newly-closed part).
 3. **Client-side display**: Group conflicting bookings by week, show in amber warning section with:
   - Week label ("Week of 3 Aug 2026")
   - For each conflicting booking: client name, time, services
   - "View Booking" / "View Client" buttons
 ### E. Available-For-Reschedule Display
 For each conflicting booking, the admin needs to know where they **can** reschedule to. This requires a **preview endpoint** that shows available hours **as-if** the holiday hours were already applied:
 - If the booking is in a week being modified by this exception → show available hours **using the new proposed holiday hours** (the hours that will apply once saved)
 - If the booking is in any other week → show available hours **using the existing working hours** (default + any already-applied exceptions)
 Backend endpoint:
 ```
 GET /api/scheduling/preview-available-hours
    ?start=2026-08-03&end=2026-08-10
    &proposed_hours=[{weekday:1,start:"10:00",end:"18:00",isOpen:true},...]
    &proposed_weeks=["2026-08-03"]
    // proposed_hours + proposed_weeks simulate the new exception group
 ```
 This is like `GetAvailableHours` but overrides the working hours resolution with the proposed exception for the specified weeks.
 ### F. Save Flow Changes
 When admin clicks "Create Schedule":
 1. Check if conflicts exist → prevent save
 2. Admin must click "View Booking" to cancel/reschedule each conflicting booking
 3. After each booking is resolved, re-check conflicts
 4. When no conflicts remain → "Create Schedule" button enables
 5. On save: Optionally create placeholder blockers for the conflicting bookings (similar to `RESERVATION:placeholder:` pattern but adapted for holiday hours). These would block the affected SLOTS (not the full week) to prevent re-booking during the transition.
 **Placeholder strategy**: For each affected booking time, create a `RESERVATION:holiday_placeholder:{bookingId}:{weekStart}` entry in time_blockers with TTL = 24 hours (enough for the resolution process).
 ---
 ## Backend Design
 ### A. New Endpoint: `GET /api/admin/bookings/conflicting-for-exception`
 Purpose: Return all bookings that would conflict with a proposed exception group.
 Input:
 ```
 week_starts: string[]  // Mondays of affected weeks (YYYY-MM-DD)
 proposed_hours: [{
    weekday: number     // 0=Mon..6=Sun
    startTime: string   // HH:MM
    endTime: string     // HH:MM
    isOpen: boolean
 }]
 ```
 Logic:
 1. For each week_start, expand to 7 days (Mon-Sun)
 2. For each day, look up the proposed hours for that weekday
 3. Query bookings table for bookings on those dates
 4. Filter bookings that fall OUTSIDE the proposed open hours (if the day is closed → all bookings conflict; if hours changed → bookings before new start or after new end conflict)
 5. Return filtered bookings grouped by week_start with user info
 SQL approach:
 ```sql
 SELECT b.id, b.start_time, b.total_duration_minutes as duration, b.status,
       u.id as user_id, u.fn as full_name, u.email, u.phone
 FROM bookings b
 LEFT JOIN users u ON b.user_id = u.id
 WHERE b.status NOT IN ('completed','client_cancelled','we_cancelled','no_show','deposit_lapsed')
  AND b.start_time >= $1       -- range start
  AND b.start_time < $2        -- range end
  AND (
    (b.start_time::time < $3)  -- starts before new opening time
    OR
    (b.start_time::time + (b.total_duration_minutes || ' minutes')::interval > $4)  -- ends after new closing
  )
 ORDER BY b.start_time
 ```
 ### B. New/Modified Endpoint: `GET /api/scheduling/preview-available-hours`
 Purpose: Show available hours as-if the proposed exception group were already applied.
 This is a modified version of `GetAvailableHours` that:
 1. Accepts the same parameters (start, end)
 2. Accepts additional `proposed_hours` and `proposed_weeks` parameters
 3. For days in proposed_weeks, uses proposed_hours instead of actual exceptional hours
 4. Otherwise, behaves identically to the current logic (including time blocker subtraction, late-night lock, etc.)
 ### C. Admin Override Mechanism
 The existing `out_of_hours` flag on `AdminCreateBookingForUserRequest` and `AdminReserveSlotRequest` already exists. When an admin is rescheduling a booking during the conflict resolution flow:
 - The "available hours" preview from step B shows valid slots
 - If the admin needs to place a booking outside those hours (e.g., there's no good time in the holiday hours), they can use the existing **admin booking creation** with `out_of_hours: true` to bypass working hours restrictions
 - This is already supported in:
  - `AdminCreateBookingForUserHandler` (manage.go:622) — skips exceptional-hours closed check when `out_of_hours=true`
  - `AdminReserveSlotHandler` (admin_reserve.go:118) — skips closing-hours check when `out_of_hours=true`
 - The frontend could expose this via a "Place outside working hours" checkbox on the booking modal (only visible during admin conflict resolution)
 ### D. Time Blocker Integration
 The holiday hours placeholder pattern (`RESERVATION:holiday_placeholder:*`) should use the existing `CleanupOldReservations` mechanism with a custom TTL. Since these are conceptually similar to `RESERVATION:edit_request:*` (24h TTL), we can add a new TTL category:
 ```go
 // In CleanupOldReservations, add:
 // RESERVATION:holiday_placeholder:* -> 24 hours
 ```
 ---
 ## Implementation Steps
 ### Step 1: Backend — Add conflicting bookings endpoint
 1.1 Implement `GetConflictingBookingsForExceptionHandler` in `exceptional-hours.go`
 1.2 Register route: `r.Get("/conflicting-for-exception", scheduling.GetConflictingBookingsForExceptionHandler)` in `main.go`
 1.3 Handle the logic: iterate weeks → expand to days → join with proposed hours → find bookings outside new hours
 ### Step 2: Backend — Add preview available hours endpoint
 2.1 Add `GetPreviewAvailableHours` in `default-hours.go`
 2.2 Accept `proposed_hours` and `proposed_weeks` as query params (JSON-encoded arrays)
 2.3 Override working hours resolution for days in proposed_weeks
 ### Step 3: Frontend — Add conflict UI to HolidayHours.svelte
 3.1 Add `openUserModal`, `openBookingModal`, `rescheduleVersion` props
 3.2 Add `checkConflictingBookings()` function  
 3.3 Add amber warning section for conflicts (reuse pattern from TimeBlockers.svelte)
 3.4 Wire `canSave` guard to `hasOverlap`
 3.5 Group conflicts by week_start for clarity
 ### Step 4: Frontend — Wire props in admin/+page.svelte
 4.1 Pass `openUserModal`, `openBookingModal`, `rescheduleVersion` to `<HolidayHours>`
 ### Step 5: Backend — Add placeholder cleanup category
 5.1 Add `RESERVATION:holiday_placeholder:*` → 24 hours in `CleanupOldReservations`
 ### Step 6: Edge Cases
 6.1 **Existing exception groups stacking**: If multiple exception groups already apply, the new proposed hours should override them for the preview (the newest exception wins)
 6.2 **Partial day changes**: If a day goes from 9-5 to 12-8, only bookings starting before 12pm conflict
 6.3 **Day closing entirely**: All bookings on that day conflict
 6.4 **Week already has an exception**: The proposed exception replaces it entirely for those weeks — check against the new hours, not the current exception
 6.5 **Admin override flow**: When admin clicks "View Booking", the booking modal should let them reschedule with `out_of_hours` option if needed (this may already work through admin booking creation)
 ---
 ## Sequence Diagram (Text)
 ```
 Admin opens "Create Exception Schedule" modal
  → Selects weeks + sets per-day hours
  → For each clicked change, calls checkConflictingBookings()
      → GET /api/admin/bookings/conflicting-for-exception
          ?week_starts=2026-08-03,2026-08-10
          &proposed_hours=[{...}]
      → Returns list of conflicting bookings with user info
  → Shows amber warning with grouped conflicts
  → Admin clicks "View Booking" on a conflict
      → BookingModal opens, admin cancels or reschedules
      → BookingModal calls onReschedule callback
      → Re-check conflicts
  → When all conflicts resolved → "Create Schedule" enables
  → Admin clicks "Create Schedule"
      → Creates placeholder blockers for each resolved conflict
      → POST /api/scheduling/exceptional-groups (existing)
      → Placeholder blockers auto-cleanup after 24h
 ```
 ---
 ## Compatibility Assessment (from 5 parallel investigations)
 ### 1. Timezone & DST Handling — SAFE with one watch
 **Architecture**: UTC-normalised. `clock.Now()` returns UTC. PostgreSQL runs with `timezone = "UTC"`. London conversion applied only where wall-clock rules matter.
 **BST midnight pattern** (critical): Every date-boundary query converts London-midnight to UTC using:
 ```go
 startTime = time.Date(startLondon.Year(), startLondon.Month(), startLondon.Day(), 0, 0, 0, 0, londonLocation).UTC()
 ```
 This ensures a booking at 00:30 BST (23:30 UTC previous day) is included in the correct London date. Found in **9 locations**. The new endpoints MUST follow this pattern.
 **`expandCronOccurrences`** is DST-safe — extracts hour/minute from London time, pins occurrences to London wall-clock.
 **Watch**: Late-night lock (default-hours.go:594-611) uses `now.AddDate(0, 0, 1)` on **UTC** time, not London time, then formats the date string. If `now` is 22:30 BST (21:30 UTC), this is practically safe since 22:30 London doesn't cross a calendar boundary. But for the preview endpoint, replicate the London-midnight boundary pattern directly rather than copying the late-night lock's UTC date logic.
 ### 2. Weekday Mapping — MUST MATCH EXACTLY
 **7 different conversion patterns exist.** The holiday hours code must use the **same patterns** as the existing booking handlers:
 **For Go time → DB weekday** (0=Monday):
 ```go
 weekday := int((localStart.Weekday() + 6) % 7)
 ```
 This is used in all booking handlers (reserve.go, admin_reserve.go, bookings.go, manage.go).
 **For date → weekStart (Monday at UTC midnight)**:
 ```go
 localStart := req.StartTime.In(londonLocation)
 daysToMonday := int(localStart.Weekday())
 if daysToMonday == 0 { daysToMonday = 7 }
 tm := localStart.AddDate(0, 0, -daysToMonday+1)
 weekStart := time.Date(tm.Year(), tm.Month(), tm.Day(), 0, 0, 0, 0, time.UTC)
 ```
 Used in 4 booking handler locations. The UTC-midnight trick is essential — `tm`
 has Location=London from `.In()`, so `.Year()/Month()/Day()` return London calendar values. `time.Date(..., time.UTC)` creates a UTC midnight at that London date, which pgx's DATE codec maps correctly regardless of BST/GMT.
 **Do NOT use** the `daysSinceMonday := int(d.Weekday()) - 1` pattern from `GetWorkingHours`/`GetAvailableHours` — that's used for date iteration loops, not individual booking lookups.
 ### 3. Booking Statuses — Match time-blocker flow (includes `pending_release`)
 **For conflict detection**, use the same filter as the time-blocker flow:
 ```sql
 status NOT IN ('completed', 'client_cancelled', 'we_cancelled', 'no_show', 'deposit_lapsed')
 ```
 This **includes `pending_release`** as a conflicting status. Rationale: `pending_release` represents a client who owes a deposit but hasn't paid on time. The system allows other users to book over the slot until payment is received (guaranteeing the slot for the business — if the original client is a no-show, another booking might fill it). However, if a holiday hours change is applied, the slot will be blocked entirely, defeating that safety mechanism. These clients should be contacted too — they may want to pay or choose a different time before the holiday hours take effect.
 Status breakdown:
 - `pending` — active (needs admin contact)
 - `confirmed` — active (needs admin contact)
 - `in_progress` — active (might overlap with change)
 - `pending_release` — active (client owes deposit, slot still reclaimable via payment)
 - `completed` — excluded (already done)
 - `client_cancelled`/`we_cancelled` — excluded (no client to contact)
 - `no_show` — excluded (no active booking)
 - `deposit_lapsed` — excluded (slot was already reclaimed by another booking)
 ### 4. Overlap SQL Pattern — Use the overlap condition, not the date-range condition
 **Critical distinction**:
 - `GetBookingsByDateRangeHandler` uses `start_time >= $1 AND start_time < $2` — this only finds bookings that **start** within the range. It misses bookings that started before but extend into it.
 - Real overlap checks use `start_time < $2 AND end_time > $1` — this catches all bookings that occupy any part of the range.
 For holiday hours conflict detection, the holiday hours cover FULL DAYS (00:00-23:59 London). Active bookings on those days overlap by definition. The filter should find bookings whose time falls **OUTSIDE** the proposed new open window:
 ```sql
 WHERE status NOT IN ('client_cancelled','we_cancelled','no_show','pending_release','deposit_lapsed')
  AND start_time < day_end_utc
  AND end_time > day_start_utc
  AND (
    start_time::time AT TIME ZONE 'Europe/London' < proposed_open  -- starts before opening
    OR
    end_time::time AT TIME ZONE 'Europe/London' > proposed_close   -- ends after closing
    OR
    proposed_is_open = false                                       -- day is closed entirely
  )
 ```
 ### 5. GetAvailableHours Pipeline — Injection Point for Preview
 The `preview-available-hours` endpoint must mirror `GetAvailableHours` exactly, with proposed hours injected at step 7a:
 ```
 Priority order (highest to lowest):
  1. Proposed holiday hours (new)        <-- inject here
  2. Existing exceptional hours          <-- existing `applied` check
  3. Default working_hours               <-- existing `defaultMap` fallback
  4. Closed (00:00-00:00, isOpen=false)  <-- existing fallback
 ```
 Injection point is at `default-hours.go` lines 549-564 (inside the per-day loop). Add BEFORE the `if applied != nil` block:
 ```go
 if proposed, ok := proposedHours[day.Date]; ok {
    baseStart = proposed.StartTime
    baseEnd = proposed.EndTime
    isOpen = proposed.IsOpen
    day.Source = "proposed"
 } else if applied != nil {
    // existing exceptional hours check...
 ```
 All downstream steps (booking subtraction, blocker subtraction, late-night lock, `out_of_hours` override) remain unchanged — they operate on the resolved `baseStart`/`baseEnd`/`isOpen` values.
 ### Summary: No blockers found
 All 5 investigations confirmed the plan is compatible with the existing timezone, weekday mapping, status filtering, and slot calculation systems. The critical invariants to maintain are:
 1. **London-midnight → UTC conversion** for all date boundaries
 2. **`(weekday + 6) % 7`** for Go→DB weekday mapping
 3. **UTC-midnight weekStart** trick for exceptional hours lookups
 4. **time-blocker status filter** (includes `pending_release`) for conflict detection
 5. **Overlap condition** (`start_time < end AND end_time > start`) not date-range condition
 ---
 ## Risk Assessment
 | Risk | Mitigation |
 |---|---|
 | Race condition: booking created while admin is resolving conflicts | Placeholder blockers prevent this (Step 5) |
 | Admin creates overlapping exceptional hours across multiple groups | Each week_start is unique per group application (DB unique index on `exceptional_group_applications(week_start)`) |
 | Large number of affected weeks overwhelms UI | Paginate conflicts, show week-by-week summary |
 | Same-day bookings at BST midnight boundary fall in wrong date | Use existing London-midnight → UTC boundary pattern (not `GetBookingsByDateRange` approach) |
 | Wrong weekday mapping at DST boundary | Use `(localStart.Weekday() + 6) % 7` + UTC-midnight weekStart trick (same as all booking handlers) |
 | `pending_release` clients contacted unnecessarily | They owe a deposit — treating their booking as active is correct. If they don't pay, the slot would be evictable anyway, but contacting them ensures they have a chance to respond before holiday hours lock it |
 | Late-night lock (22:00-11:00) uses UTC dates inconsistently | Preview endpoint should use London-midnight for all date comparisons, not UTC `AddDate` |
@@ -1,235 +0,0 @@
 # Payments Work Review — Consolidated Report (Round 1)
 **Scope:** commits `503c326..HEAD` (110 commits, ~45k insertions, 178 files) — replacing the dev-only Square mock with a realistic mock + real Square integration, plus hardening.
 **Review method:** 8 specialist agents (Square API contract, Square usage surface, UK consumer law, UK GDPR/PCI, mobile parity, testing gaps, pattern consistency, fix-wide-application) + 5 review-work agents (goal/constraint verification, QA execution, code quality, security, context mining).
 ---
 ## Resolution Status (updated 13 Aug 2026)
 The payments overhaul is now complete (batch 1, 88 commits, code fixed). This section records the resolution status of every round-1 finding as verified in the batch-1 review, with spot-checks re-run against the current tree on 13 Aug 2026. The round-1 findings text below is left untouched.
 | ID | Status | Note |
 |----|--------|------|
 | C1 | RESOLVED | till-sale no-key fallback now derives a deterministic base key (`till-` + sha256) and reuses pending rows |
 | C2 | RESOLVED | `ValidateAmount`/£10,000 cap applied on till sales and gift-card create/top-up/transfer |
 | C3 | RESOLVED | `CancelGiftCard` / `AdminCancelGiftCard` implemented: in-app 14-day cancel, refund to original payment method, reg 34(9) partial-use handling |
 | H1 | RESOLVED | `square_request_snapshot` scrubbed (`NULL`) in all erasure paths; comments/wording per operator intent |
 | H2 | RESOLVED | `CleanupIdleAccounts` snapshots Square IDs pre-anonymize and disables cards + deletes the Square customer (with retry) |
 | H3 | RESOLVED | Square-side deletion retried via scheduled job and surfaced on failure |
 | H4 | RESOLVED | 2FA gate now also on BuyGiftCard SaveCard + CreatePaymentMethod |
 | H5 | RESOLVED | env vars documented, checker fixed |
 | M1 | RESOLVED | 16px font in Square hosted fields (no iOS auto-zoom) |
 | M2 | RESOLVED | dialogs bottom-sheet/max-height on `<sm` |
 | M3 | RESOLVED | `inputmode="decimal"` on cash-received fields |
 | M4 | RESOLVED | terminal inline INSERT |
 | M5 | RESOLVED | idempotency helpers |
 | M6 | RESOLVED | SPV enforced |
 | M7 | RESOLVED | `ConfirmOverflowTip` |
 | M8 | RESOLVED | TODO comment added |
 | M9 | RESOLVED | snapshot encryption (`encryptSnapshot`) |
 | M10 | RESOLVED | 2FA per-IP limiter |
 | M11 | RESOLVED | invoice → 501 |
 | M12 | RESOLVED | shared TipPayment |
 | M13 | RESOLVED | wording |
 | M14 | RESOLVED | truncate |
 | B1 | RESOLVED | shared `releasePaymentLock` helper (`handlers/payments/locks.go`) |
 | B2 | RESOLVED | shared `recordTerminalPaymentTx` core in the sweep |
 | B3 | RESOLVED | unknown money event families now 501 so Square retries (known non-money families still acked) |
 | B4 | RESOLVED | terminal checkout `CustomerID` populated from the user |
 | B5 | RESOLVED | till idempotency key capped at 45 (`validate:"omitempty,max=45"`) |
 | B6 | RESOLVED | `square_request_snapshot` refreshed on pending-row reuse |
 | B7 | RESOLVED | structured Square error code/category matching instead of `err.Error()` strings |
 | B8 | RESOLVED | sweep dynamic SQL via `pgx.Identifier` |
 | B9 | RESOLVED | `delete_guest_user` NULLs `gift_card_transactions.user_id` / `gift_cards.redeemed_by` before delete |
 | B10 | RESOLVED | dispute `state.updated` raises `insertCriticalPaymentNotification` |
 | B11 | RESOLVED | legacy NULL `square_payment_id` refund rows handled in the cancellation/residual passes |
 | F1–F14 | RESOLVED | frontend batch-1 fixes (touch-target sizes, dialog scroll, 2FA autocomplete, tip fix, C3 saved-card message) |
 | S1 | RESOLVED | `CF-Connecting-IP` no longer trusted unconditionally behind nginx |
 | S2 | RESOLVED | nginx CSP strengthened |
 | S3 | RESOLVED | nginx `api_limit` raised to mirror the backend |
 | T1 | OPEN | frontend test framework — being added in batch 2 |
 | T2–T12 | RESOLVED | backend test coverage added (webhook rescue branch, dispatch aliases, concurrent dedup, refund guards, sweep branches, nonce consumption, `acquireAdvisoryXactLockBlocking`, suite isolation, GDPR retention pin) |
 | D1 | RESOLVED | doc comment added to `SweepStalePendingPayments` |
 | D2 | RESOLVED | read-only endpoints confirmed intentional |
 | D3 | RESOLVED | `mw.RespondError` unified in the twofa handler |
 | D4 | RESOLVED | ratelimit build-tag divergence confirmed deliberate and documented |
 | D5 | RESOLVED | gift-card T&C expiry-warning wording reconciled (conditional on email delivery) |
 | D6 | RESOLVED | terms-page idle-account warning wording reconciled |
 | D7 | RESOLVED | gift-card expiry window configurable backend ↔ frontend display reconciled |
 | D8 | RESOLVED | gift-card expiry shown in account |
 | D9 | RESOLVED | README/obsidian test count reconciled (docs updated to 2,269, 13 Aug 2026) |
 | D10 | RESOLVED | refund code verified to return refunds to the original payment method |
 | D11 | RESOLVED | `data_retention_consent` default flipped to genuine opt-in (`DEFAULT FALSE`) |
 | D12 | RESOLVED | Square error-code vocabulary verified against `internal/square` (`VERIFY_CVV_FAILURE`) |
 | D13 | RESOLVED | terminal checkout status vocabulary corrected (valid `COMPLETED`, no `COMPLETED_BY_DEVICE`) |
 | D14 | RESOLVED | `deadline_duration` RFC 3339 parsing verified |
 | D15 | RESOLVED | frontend sandbox/prod Square script URL selection verified |
 Notes:
 - **M4–M14** were tracked in the batch-1 review and are not numbered in the round-1 findings text above; the statuses recorded here come from that review.
 - Spot-checks on 13 Aug 2026 confirmed the tree matches the recorded statuses for C1–C3, H1–H5, B1–B11, D7, D9, D10, D11, D12, D13. Any residual uncertainty is limited to the exact frontend styling diffs (F1–F14, M1–M3), which batch-1 verified; treat those as "verify on final visual pass" if in doubt.
 - The only genuinely open item is **T1** (frontend automated tests), scheduled for batch 2.
 ### Round A re-review (14 Aug 2026)
 A fresh-eyes round-A review (legal/ops/docs + money-path) of commit `e9315c9` re-opened **two entries this table marked RESOLVED**:
 - **F1 — admin overcharge (re-opened).** The round-1 frontend F1–F14 batch (touch-target sizing etc.) is verified, but the round-A **money-path** review found a separate live money bug still present in the same admin payment surface: campaign credit can be applied on terminal/cash/saved-card admin payments (`CreateTerminalPayment` / till / saved-card paths), producing a charge that exceeds the remaining booking balance. Owned by the money-path fix round; not closed by the round-1 batch.
 - **/terms tiers &amp; refund-method wording (D6/D10 — re-opened).** Round-1 marked the terms-page wording reconciliation (D6) and "refunds to the original payment method" verification (D10) RESOLVED, but the round-A legal/docs review found the `/terms` route still contradicted the code: §3 claimed the deposit is forfeited only under 24 hours' notice (the code keeps up to 50% of the subtotal on 24–72h notice and everything under 24h — `CalculateRefundForCancellation`, `handlers/payments/refunds.go`), and §4 overbroadly claimed refunds go to "the original payment method" (cash refunds are credited to the account balance, gift-card refunds to the balance when no `gift_card_id`, only card goes back via Square). The `/terms` route has now been aligned to the verified three-tier schedule and per-method refund disclosures (Aug 2026).
 No other RESOLVED entries were contradicted by round A.
 ---
 ## Overall Verdict
 | Agent | Verdict | Confidence |
 |---|---|---|
 | Goal & Constraint Verification (Oracle) | **FAIL** | HIGH |
 | QA Execution (hand-on) | **PASS** (893 tests green, 16/16 live smoke) | HIGH |
 | Code Quality (Oracle) | **FAIL** | HIGH |
 | Security (Oracle) | **FAIL** (severity HIGH) | HIGH |
 | Context Mining | **FAIL** | HIGH |
 The architecture is fundamentally sound — deterministic idempotency keys, refund locks, sweeps, HMAC webhook verification all verified correct in code — but there are real money-safety gaps, GDPR erasure holes, a legal-doc-vs-code mismatch, and pervasive mobile/touch-target issues. Below, every issue from critical to minor, with suggested fixes.
 ---
 ## CRITICAL / Money-Safety
 ### C1. Till-sale no-key idempotency fallback is still RANDOM (H2 bug class)
 - **File:** `backend/handlers/payments/till.go` (598-600, 661-663, 670-672, 703-705, 713-715); helper `handlers.go:4063-4065`
 - **Issue:** Every other charge path derives a deterministic, request-stable idempotency key. `CreateTillSale` falls back to `uniqueChargeKey("till-")` = `prefix + rand.Text()` when no client key is supplied. A no-key `create` till sale whose response is lost mints a NEW key on retry → second Square charge + second gift-card funding. The `topup` path is rescued by the gift-card pending-resume (till.go:305-326), but `create` cannot (no card id in the request).
 - **Fix:** Derive a deterministic no-key fallback for till sales (e.g. action+amount+redeem-target hash) under the held `crussell:till:` lock, and reuse pending rows by key; or extend the pending-resume to resolve `create` pending rows.
 ### C2. £10,000 amount cap missing on four admin money entry points
 - **File:** `till.go:159-162`; `giftcards.go:382-389` (CreateGiftCard), `488-491` (TopUpGiftCard), `623-626` (TransferGiftCard). Cap lives at `validators.go:22` and is applied everywhere else (`handlers.go:1416, 298, 321, 3418, 2411, 3053`).
 - **Issue:** A till sale or top-up can fund a gift card with an unbounded amount.
 - **Fix:** Run `ValidateAmount` (or pounds-equivalent cap) in each handler on the effective amount.
 ### C3. Gift Card T&C grants a CCR 2013 14-day cancellation right with NO code path
 - **File:** `Gift Card Terms & Conditions.md`; `handlers.go:2473` (RefundPayment rejects gift-card purchases); no cancel/refund gift-card route exists anywhere; the error message points to a non-existent "gift-card section".
 - **Issue:** Online gift-card sales are distance contracts under Consumer Contracts Regs 2013. The T&C (a binding consumer contract) promises a 14-day right to cancel with refund to original payment method, but no execution path exists. The refund route explicitly rejects gift-card purchases.
 - **Fix:** Implement a `CancelGiftCardPurchase` route (within 14 days, refund to original card via Square RefundPayment, reverse gift-card funding) or amend the T&C to remove the promise (recommended: implement, since a small merchant is exposed on this).
 ---
 ## HIGH / GDPR & PII
 ### H1. `square_request_snapshot` retains buyer email in plaintext JSON forever — no erasure path scrubs it
 - **File:** `init-script.sql:692` (payments), `:2144` (till_sales); stored at `handlers.go:1828, 3716; giftcards.go:1237; till.go:831,866`; `anonymize_user` (945-1038), `delete_guest_user` (1046-1070), `AnonymizeStaleGuestAccounts` (time-blockers.go:414-631) all leave it intact.
 - **Issue:** The snapshot embeds `BuyerEmail` (PII) in plaintext JSON retained for the 7-year financial period. Deleted users' emails remain recoverable — GDPR Art.17/5(1)(e) breach. Also the snapshot contains `note`/`verification_token`.
 - **Fix:** `SET square_request_snapshot = NULL` in all three erasure paths (snapshot is only needed for pending-row replay), or strip `buyer_email_address` from the stored JSON.
 ### H2. `CleanupIdleAccounts` anonymises locally but NEVER disables Square cards or deletes the Square customer
 - **File:** `time-blockers.go:1041-1159` vs `account.go:88-112/183-224` (which does it right); `time-blockers.go:437-499`.
 - **Issue:** Idle-account cleanup runs `anonymize_user` (NULLs `square_card_id`/`square_customer_id` first) but never calls `DeleteCardOnFile`/`DeleteCustomer`. Customer name+email stays live at Square; local references already destroyed so the orphans are untraceable. Enabled ccof: cards remain chargeable if leaked.
 - **Fix:** Add the pre-anonymize snapshot + Square card-disable/customer-delete pass to `CleanupIdleAccounts` (mirror `account.go`).
 ### H3. Square-side deletion is fire-and-forget — no retry, no alert, local refs already NULLed
 - **File:** `account.go:181-225` (goroutine + 30s timeout, failure only `log.Printf`).
 - **Issue:** Failed deletion permanently orphans enabled cards + customer profile at Square. Erasure incomplete on transient API failure.
 - **Fix:** Persist Square IDs before erasure; retry via scheduled job (cleanup.go) or at least raise a critical payment notification on failure.
 ### H4. 2FA codes delivered via plaintext server log
 - **File:** `handlers/user/twofa.go:293` (`log.Printf("[2FA] ...")`); unsalted SHA-256 fallback when `TWO_FACTOR_PEPPER` unset (twofa.go:86-87).
 - **Issue:** Anyone with backend log access defeats the 2FA gate on saved-card charges; with pepper unset, pending-code digests are offline-brute-forceable in the 1M space.
 - **Fix:** Fail-closed on missing pepper (reject startup like JWT check); remove plaintext log delivery or gate it to `!dev` builds.
 ### H5. Dispute reason (third-party free text) logged in full, stored, and GDPR-exported
 - **File:** `webhooks/square.go:846,889,909`; `init-script.sql:1436` (export includes `disputes.reason`).
 - **Issue:** Cardholder-typed dispute reason is third-party PII; unnecessary in logs.
 - **Fix:** Truncate/prefix in logs; keep stored (legitimate financial record) but document third-party nature in export.
 ---
 ## HIGH / Frontend Mobile
 ### M1. Square hosted-field iframe uses 14px font → iOS auto-zoom on focus
 - **File:** `SquareCardInput.svelte:16` (`fontSize: '14px'`); sibling `Input` uses `text-base` (16px).
 - **Fix:** Set `fontSize: '16px'` (Square caps at 16 for exactly this reason).
 ### M2. Vertically-centered `max-h-[90vh]` dialogs trapped under iOS keyboard; primary action unreachable
 - **File:** `UserPaymentModal.svelte:580`, `PaymentModal.svelte:784`.
 - **Fix:** On `<sm` switch to bottom-sheet (`rounded-t-xl`, `max-h-[calc(100dvh-4rem)]`), add `pb-[max(1rem,env(safe-area-inset-bottom))]`, or `scrollIntoView` the submit button.
 ### M3. `inputmode="numeric"` on cash-received fields → no decimal key on iOS, £X.XX unenterable
 - **File:** `GiftCardsManagement.svelte:1949` (generate-cash), `:2220` (top-up cash).
 - **Fix:** Change both to `inputmode="decimal"`.
 ---
 ## MEDIUM
 ### Backend
 - **B1. Advisory-lock release inlined at 8+ sites** instead of shared helper. `loyalty.go:116`, `till.go:227,1055`, `refunds.go:871,881,1297`, `sweep.go:1092`, `handlers.go:1141,2528` vs canonical `charge_helpers.go:133-166`. Drift risk — a fix to the release pattern must be replicated 8×. **Fix:** generic `releasePaymentLock(pinConn, key)` helper.
 - **B2. `recordUntrackedTerminalPayment` (sweep.go:1066-1244) is a ~180-line second implementation of the GetCheckoutStatus recorder** (handlers.go:1111-1332) — already drifted (status-IN guards; `insertCriticalPaymentNotification`; `slog` vs `log.Printf`) and its tip-split branch is untested. **Fix:** shared `recordTerminalPaymentTx` core; add tip-split sweep test.
 - **B3. Webhook default case ACKs unknown event types forever** (`webhooks/square.go:260-262`): dedup row committed + 200, event permanently dropped. `payment.completed`/`payment.canceled`/`card.*`/`customer.*` never processed. **Fix:** log WARN + store/notify unhandled events (or 501 so Square retries) instead of silent ack.
 - **B4. Terminal checkout `CustomerID` field exists (types.go:73) but NO handler sets it** (`handlers.go:862`, `till.go:682`). **Fix:** populate from user when present.
 - **B5. Till client idempotency key validated at ≤64 but CreatePayment caps at 45** (`till.go:36`) — a 46-64 char client key 400s at Square. **Fix:** cap at 45.
 - **B6. Replay-snapshot staleness:** pending-row reuse refreshes `square_source_id` but NOT `square_request_snapshot` (`handlers.go:1787,3669; giftcards.go:1149; till.go:729`) → sweep replays a snapshot whose source differs → `IDEMPOTENCY_KEY_REUSED`. Documented at sweep.go:573-584 as CRITICAL/manual-reconcile. **Fix:** refresh snapshot JSON (or replay uses current `square_source_id`).
 - **B7. `CreatePaymentMethod` string-matches `err.Error()`** (`handlers.go:2362-2366`) for "invalid"/"expired" instead of structured `squareAPIError.Code`. **Fix:** use `CARD_EXPIRED`/`INVALID_REQUEST_ERROR` codes.
 - **B8. Sweep dynamic SQL** (`sweep.go:431,446,464`) builds `UPDATE `+table+`` — internal constants only (not injection) but blocks pgx statement caching. **Fix:** `pgx.Identifier`/branch.
 - **B9. `delete_guest_user` can fail on FK** for `gift_card_transactions.user_id` / `gift_cards.redeemed_by` (no ON DELETE action, init-script.sql:2289/2242). **Fix:** `SET user_id/redeemed_by = NULL` before delete.
 - **B10. `dispute.state.updated` for untracked dispute raises no notification** (`square.go:877-885` vs created-path 819-832). **Fix:** mirror `insertCriticalPaymentNotification`.
 - **B11. Legacy manual refund rows with `square_payment_id` NULL never swept** (`refunds.go:1181`). **Fix:** a pre-pass that fails/reconciles them like the cancellation pass (refunds.go:554-583).
 ### Frontend
 - **F1. Shared `Input` is 36px tall (`h-9`)** (`input.svelte:29,45`) — every money/card/tip/2FA field under the 44px touch target. **Fix:** `min-h-11` at base, `md:h-9`.
 - **F2. Dialog close button 32px** (`dialog-content.svelte:40`). **Fix:** `h-10 w-10`.
 - **F3. Till qty steppers 24px** (`TillPurchases.svelte:557,566,577`). **Fix:** `h-9 w-9`.
 - **F4. Till payment-method buttons `py-2 text-xs` in 2/3-col grid** (`TillPurchases.svelte:618-628`); wrong method = wrong charge path. **Fix:** `py-3 text-sm`, `grid-cols-2` below sm.
 - **F5. BookingFlow step-2 loading panel overlays calendar on mobile** (`BookingFlow.svelte:1997-2002`). **Fix:** `hidden md:flex`.
 - **F6. 2FA code inputs missing `autocomplete="one-time-code"`** (`account/+page.svelte:2721,2740`). **Fix:** add it + `pattern="[0-9]*"`.
 - **F7. Password modal has no max-height/scroll** (`account/+page.svelte:2972-3061`). **Fix:** `max-h-[calc(100dvh-2rem)] overflow-y-auto`.
 - **F8. Calendar cell 40px on mobile** (`DatePicker.svelte:37`). **Fix:** 44px base.
 - **F9. Admin service price input 32px×96px** (`PaymentModal.svelte:809`); `(was £…)` clips at 320px. **Fix:** `h-10`, `min-w-0`.
 - **F10. Gift-card code placeholder clips at 320px** (`PaymentModal.svelte:1295-1305`). **Fix:** `text-base`.
 - **F11. Square card-form container no min-height while loading** (`SquareCardInput.svelte:281`) → reflow jumps Pay button. **Fix:** `min-h-[120px]` + skeleton.
 - **F12. `min-h-screen` not `min-h-dvh`** (`+layout.svelte:69`, `tip/+page.svelte:146`, `pay-tip/[id]:131`). **Fix:** `min-h-dvh`.
 - **F13. CardSelection consent checkbox 16px** (`CardSelection.svelte:207`). **Fix:** `h-5 w-5`.
 - **F14. Demo route rows overflow at 320px** (`demo/+page.svelte:228-249`). **Fix:** `min-w-0`.
 ### Security
 - **S1. `clientIP()` trusts client-supplied `CF-Connecting-IP` first** (`ratelimit.go:145-156`); nginx never sets/strips it → spoofable rate-limit bypass (login brute-force, progressive backoff) when not behind Cloudflare. **Fix:** nginx `proxy_set_header CF-Connecting-IP ""` or only honor when trusted-proxy flag.
 - **S2. nginx CSP `script-src 'unsafe-inline'`** (`default.conf:41,159`) weakens PCI SAQ-A / Square SDK CSP requirement. **Fix:** hash/nonce-based.
 - **S3. nginx `api_limit` 20r/m per IP self-DoS** (`default.conf:75`). **Fix:** ≥120r/m to mirror backend.
 ### Tests
 - **T1. Frontend has ZERO automated tests** for all payment-critical UI logic — `square.ts isNonceStale`/`submitPaymentWithRetry`, MockCardForm Luhn/expiry/brand (the f1c4957 expiry bug lived in these 8 files with nothing to catch it). **Fix:** vitest + `@testing-library/svelte`.
 - **T2. Webhook `payment.updated` COMPLETED → pending till-sale rescue branch untested** (`square.go:616-625`).
 - **T3. `payment.created`/`refund.created` switch aliases never dispatched** in tests (`square.go:248,250`) — regression silent.
 - **T4. Concurrent delivery of same webhook event_id untested** (in-memory fast-path + DB dedup).
 - **T5. Manual `RefundPayment` discount/on-the-house 400 guard untested** (`handlers.go:2426-2428`).
 - **T6. Sweep's `recordUntrackedTerminalPayment` tip-split/VAT/completion branches untested** (`sweep_test.go` only charges booking total).
 - **T7. Dev mock doesn't consume `cnon:` nonces on CreatePayment** → pending-retry tests pass trivially; re-tokenization unverified (`square_dev.go:797` only enforces for CreateCardOnFile).
 - **T8. `CreateTerminalPayment`'s `AllowTipping: false` not asserted at handler level.**
 - **T9. `acquireAdvisoryXactLockBlocking` no direct test.**
 - **T10. Webhook dedup global makes suite order-dependent** (`squareWebhookEventsSeen` never reset).
 - **T11. `webhooks_test.go` happy-path dispatches don't exercise dispatch bodies** (only `{"id":...}` → parse-fail branch).
 - **T12. No test pins GDPR erasure PRESERVES financial rows** (7-year retention).
 ### Pattern / Docs
 - **D1. `SweepStalePendingPayments` has no doc comment** (`sweep.go:64`) — rationale sits above the const.
 - **D2. `main.go:516` payment-summary & `:522` giftcards/balance ungated** (read-only, ownership-checked — NITPICK, likely intentional).
 - **D3. `payments/twofa.go:95,99,105` mw.RespondError vs http.Error within one handler** (cosmetic).
 - **D4. ratelimit build-tag scheme `!dev || test` diverges** (deliberate, documented).
 - **D5. Gift Card T&C promises expiry warning emails (1mo/1wk)** (`Gift Card T&C:58-61`) but SMTP unwired + code explicitly omits them (`CleanupExpiredGiftCards`). **Fix:** reconcile T&C wording or implement.
 - **D6. Frontend terms page promises idle-account warning emails** (18/23mo, 4/59mo) but `TODO: Email Integration`. **Fix:** reconcile.
 - **D7. Gift-card expiry window configurable backend (default 24, min 12) vs hardcoded +24-month frontend display.** **Fix:** sync or expose.
 - **D8. Gift-card expiry "displayed in your account" claimed; no UI shows it.** **Fix:** display expiry in account gift-card section.
 - **D9. README test count 2,169 vs actual 2,237; job count 25 vs backlog 23.** **Fix:** reconcile docs.
 - **D10. Refund-to-different-card: terms say "original payment method" — verify refund code enforces original-method only** (`refunds.go`).
 - **D11. `data_retention_consent` default TRUE presented as consent** (`init-script.sql:220`, `gdpr/+page.svelte:775-781`). **Fix:** default FALSE, genuine opt-in.
 - **D12. Square error-code vocabulary drift** (`VERIFY_CVV_FAILURE` vs `VERIFY_CVV_FAILED`, `INTERNAL_SERVER_ERROR` vs `INTERNAL_ERROR`, no `PAYMENT_ALREADY_IN_PROGRESS`). **Verify** against `internal/square/errors.go` + mock.
 - **D13. `COMPLETED_BY_DEVICE` is not a TerminalCheckout status** — verify handling in webhook handler + mock.
 - **D14. `deadline_duration` is RFC 3339 (`PT5M`) not Unix/ISO8601** — verify Terminal parsing.
 - **D15. Frontend sandbox vs prod script URL** (`sandbox.web.squarecdn.com` vs `web.squarecdn.com`) — verify `square.ts` selects correctly per env.
 ---
 ## Residual Risks (documented, accepted)
 - On ambiguous lost-response where the charge landed, a same-key retry with a re-tokenized nonce hits `IDEMPOTENCY_KEY_REUSED`, stranding the row pending for sweep/CRITICAL + blind-fail. No double charge; documented at `square_dev.go:323-325`; swept at `sweep.go:573-585`.
 - Parallel-package test DB migration can deadlock (concurrent `CREATE EXTENSION`) — pre-existing infra flake; run-tests.sh flock documented.
@@ -1,88 +0,0 @@
 # Crussell Payments Review — CONSOLIDATED SESSION REPORT
 **Base:** commit `503c326297e6edc6b9681bd8025cd904a6a4965f` → working tree
 **Scope:** 279 files, +84,719 / −7,584 lines — the Square payments mock→realistic integration + all fixes.
 **Process:** 12 concern-review agents → review-work skill (5 agents: goal/QA/code-quality/security/context) → fix batch 1 (7 agents) → docs+tests batch 2 (2 agents) → re-review loop (regression, adversarial, SCA/2FA) → adversarial round 3 → final hardening. All backend tests run via `backend/run-tests.sh` (flock-wrapped) to prevent test-DB clobber.
 ## FINAL STATE — ALL TESTS GREEN
 - Backend: full suite `go test -tags "test,dev" ./...` — **28 packages pass**, 2,611 test functions.
 - Prod-tag fail-closed suite (`run-prod-tag-tests.sh`, `-tags "test,!dev"`) — passes (real prod 2FA branches now compiled + tested).
 - Frontend: `svelte-check` 0 errors, `vitest` **147 tests pass** (was 130; +policy cross-check, +SCA dialog component test, +mock token-shape parity).
 - `scripts/check-env-docs.py`: OK (39 env vars documented).
 - Runtime smoke (live mock backend): all keystone journeys verified end-to-end.
 ---
 ## EVERY ISSUE FOUND → FIX
 ### CRITICAL
 1. **SCA wire contract broken on all 4 customer charge surfaces** — BookingFlow/UserPaymentModal/TipPayment/account gift-card buy sent `card_id` + `new_card_token` together → backend `ValidateCardInfo` rejected 400. Backend `CreateTipPaymentRequest`/`BuyGiftCardRequest` had no `saved_card_id`. **Fixed:** 3-arg `ValidateCardInfo` accepts saved-ref+tokenize coexistence (matches `resolveChargeSource` + pinned contract); frontend uses `new_card_token: newCardToken ?? verificationToken` precedence everywhere. Verified at runtime (SourceID = `cnon:sca-...`).
 2. **Admin/till SCA token silently dropped** — `CreateTerminalPaymentRequest`/`TillSaleRequest` lacked `new_card_token` → Go JSON decoder discarded it → naked ccof → 402 in enforced env. **Fixed:** field added + routed through `resolveChargeSource`; gate-skip added for terminal/till. Runtime-verified (SourceID carries token).
 3. **`{{SUPPORT_EMAIL}}` + fake contact data shipped live** in terms/privacy/contact pages. **Fixed:** centralised `frontend/src/lib/constants/contact.ts`; placeholders removed; contact page uses constants.
 ### HIGH
 4. **Delete-account always failed** — frontend sent empty DELETE body; backend requires `current_password` + 2FA code. **Fixed:** dialog collects password + on-demand 2FA code (minted via `/api/user/2fa/code`), sent in body. Runtime-verified 204.
 5. **Admin loyalty redemption 404** — `/api/admin/bookings/{id}/apply-redemption` had no route. **Fixed:** route registered under RequireAdmin; `loyalty.go` owner check relaxed to admin-or-owner. Runtime-verified.
 6. **Admin "mark completed" unreachable** — `PUT /admin/bookings/{id}/progress` had zero frontend callers. **Fixed:** wired "Begin appointment" (CurrentAppointment) + "Complete" (BookingModal). Runtime-verified.
 7. **DAV default admin password defeated fail-closed guard** — `.env.example` shipped `changeme-admin-password` not in the weak list → copy-paste deployment exposes all customer vCards. **Fixed:** weak list expanded + entropy gate; `.env.example` ships empty `DAV_ADMIN_PASSWORD=` (compose `:?` fails closed).
 8. **Login lockout DoS with no recovery** — 5-fail lockout, dead reset flow. **Fixed:** dummy-bcrypt on no-user login (kills timing oracle); `password_reset` purpose now clears `failed_attempts`/`locked_until` after code verify.
 9. **User gift-card £500/day cap bypassable by concurrency** — lock keyed on idempotency key. **Fixed:** per-user advisory lock `crussell:giftcard-user-cap:<userID>` held across read-modify-write.
 10. **TopUpGiftCard could top up an EXPIRED card** (reviving forfeited balance). **Fixed:** expiry gate added to admin top-up (till already had it).
 11. **UserPaymentModal infinite fetch loop** — `$effect` re-triggered on store `loading` toggle → 983+ API calls. **Fixed:** one-shot guard + idempotent store.
 12. **Money displayed 100× too small** — `formatCurrency(totalPaid / 100)` where totalPaid already pounds. **Fixed:** `formatCurrency(totalPaid)`.
 13. **postChargeRecheck stranded money silently** — no refund row, no notification. **Fixed:** flood-capped critical notification raised.
 14. **`payment.completed` orphan detection could fail a legit completed charge** (delayed webhook). **Fixed in two passes:** (a) `SweepKeyedReplayAge` age gate; (b) **B1-EVIDENCE GATE** (round 3): failed-mark now requires the sweep's actual B1 markers (`b1_attempts > 0` or a `duplicate charge — sweep replay` refund row); no evidence → origin left pending for the sweep to reconcile. New test locks the no-evidence path.
 ### MEDIUM
 15. **Online card SAVE unusable in enforced env** — gate matched only mock's fictional `cnon:sca-`. **Fixed:** genuineness derived from Square acceptance of any token-like source.
 16. **Payment 2FA gate dead UI + fallback machinery** — frontend sent codes backend never read; `insertTwoFAFallbackAudit`/`reissueTwoFACodeAfterFailedCharge`/`enforceSCAFallbackConsent` unreachable. **Fixed:** dead machinery deleted, `verification_code` removed from charge structs, TwoFactorCodeInput removed from payment flows (account flows intact).
 17. **Privacy policy false statements + Art 13 gaps** — "never store card expiry" (false: last_4/exp_month/exp_year stored), missing processors (Cloudflare/R2/CardDAV/Google Fonts/CARTO), no international transfers. **Fixed:** reworded + added sections.
 18. **No right-to-cancel at gift-card purchase + self-purchase forfeits 14-day right without disclosure.** **Fixed:** disclosure + explicit acknowledgement checkbox + links to gift-card-terms.
 19. **Terms page omitted Distance Contracts & Right to Cancel section.** **Fixed:** ported from obsidian §5.
 20. **Business cancellation full-refund policy vs notice-tier code.** **Fixed:** policies rewritten to the accurate "full refund or free reschedule; refunds under standard tiers unless waived".
 21. **Terms §2.2 contradicted privacy policy on deletion balance.** **Fixed:** rewritten to "balance retained on anonymised record".
 22. **Idle-account warnings promised but commented out.** **Fixed:** docs corrected to "not yet implemented".
 23. **Till responses hand-rolled JSON** (~50 `http.Error` + 6 `json.NewEncoder`) instead of `mw.RespondJSON/RespondError`. **Fixed:** migrated.
 24. **Dev/mock new-card charges broken** — `verify_mock_` overwrote the cnon nonce; mock token ≠ backend contract. **Fixed:** mock mints `cnon:sca-...`; `tokenizeWithVerification` returns `verificationToken: null` for new cards (real-SDK parity) so `save_card` works in dev.
 25. **Gift-card buy (account page) kept the old overwrite pattern + sent dead consent fields.** **Fixed:** explicit precedence + `scaFallbackConsentFields` removed.
 26. **`GetBookingRemainingBalancePence` counted `on_the_house`/discounts as paid.** **Fixed:** excluded.
 27. **refund_failed notification dedup missing `acknowledged_at IS NULL`.** **Fixed.**
 28. **Verification codes stored plaintext + brute-force budget keyed per code value.** **Fixed:** HMAC-peppered hashing at rest (CHAR(64)), per-user budget, `[VERIFY]` log relay in dev / fail-closed in prod.
 ### MEDIUM-HIGH (adversarial round 3)
 29. **£250 online-tip cap bypassable via the overflow-tip carve** — `confirm_overflow_tip=true` + £10,000 minted uncapped tip rows. **Fixed:** gate rejects `req.Amount − realRemaining > maxOnlineTipPence` (£250) in BOTH carve paths + `buildSplitRecords` belt-and-braces returns an error (signature `([]PaymentRecord, error)`). Runtime-verified: £10k → 400; £250 → succeeds with £215 tip carve.
 30. **Completion campaign auto-apply over-redemption race** — read-then-write + non-conditional increments. **Fixed:** all 4 increments atomic reserve-first (`... AND times_redeemed < max_redemptions RETURNING id`), skip-on-ErrNoRows; per-user milestone + anniversary get `ON CONFLICT DO NOTHING`; schema backstops added (`chk_times_redeemed` CHECK + partial unique index `uq_booking_discounts_user_milestone_campaign`). Deterministic race tests added.
 ### LOW / MINOR / NITPICK
 31. **4 gift-card admin handlers lacked in-handler `isAdminRequest` backstop.** **Fixed** (CreateGiftCard/TopUpGiftCard/TransferGiftCard/ClaimExpiredBalance).
 32. **Tip lock key inconsistent** (`crussell:tip:` vs `crussell:payment:`). **Fixed:** aligned.
 33. **Float→int64 overflow in gift-card cap checks.** **Fixed:** NaN/Inf/oversize guard before pence conversion.
 34. **Mock SCA error-shape parity unpinned + prod 2FA branches never compiled** (dead under `test,dev`). **Fixed:** `run-prod-tag-tests.sh` + handler-level verification_required tests + mock parity tests.
 35. **Test-gap fixes:** ApplyScheduledDefaultHours test; webhook tests hardcoded-2025 dates → clock-relative; ValidateCardInfo table tests; listCards 20-page guard test; penceLess/roundingEpsilon boundaries; main.go startup-check tests; policy.ts↔refund_policy.go cross-check tests; vitest svelte-component project (happy-dom) + SCA dialog test.
 36. **Mobile parity (26 findings):** touch targets ≥44px on UserBookingModal/PaymentModal/TipPayment/BookingFlow/OverflowTipConfirm/TillPurchases; dialog close 44px; `active:` pressed feedback; TimeSlotPicker 50dvh+44px; `.no-scrollbar` utility; receipt document.write fields escaped; policyPopover mobile anchor; CSP meta added to app.html.
 37. **Docs parity (17 findings):** refresh-token grace 60s→20s; session 30d→90d; notification reasons 16→17; test counts recomputed; deposit advance 24h→36h; gift_card_expiry default 12→24; patch-test 24-48h→24h; phantom `no_deposit` reason removed; line-ref drift → function-name refs; 5-min header mislabel; route tables + cancellation/gift-card-terms; cash-refund claim verified; flood-cap enumeration; idle-account warnings; M1 VAT undercount; Testing Architecture package counts; `.env` stale `TWO_FACTOR_FALLBACK=true` removed; verification-code delivery channel documented.
 38. **Policy minors:** cancel-confirm dialog overstatement; deposit 20–50% phrasing; obsidian User/Admin Manual inconsistencies (24h/36h, forfeit vs full-refund); gift-card expiry reminders; last-updated date format; footer links; deletion balance in obsidian.
 39. **stale comment/struct-field cleanup:** `twofa.go:197` clock sentinel; sweep-cap coordination comment; `releaseBookingPaymentLock` note; `ConsentVersion/ConsentAccepted` dead struct fields removed from charge structs; stale references to deleted 2FA machinery.
 ---
 ## ADVERSARIAL PASSES — CLEARED (no fix needed)
 - Refund capacity/over-refund/pence rounding — **CLEARED** (exact pence math, per-payment + per-booking caps, idempotency replay returns stored result).
 - Refund webhook replay double-apply — **CLEARED** (status-guarded UPDATEs, event_id dedup).
 - Webhook signature verification — **CLEARED** (HMAC-SHA256 URL+body, constant-time compare).
 - Sweep refund of a just-succeeded payment / cancel of a just-completed checkout — **CLEARED** (age gates + replay-window discriminator).
 - Sweep notification flood caps — **CLEARED** at every insert site.
 - Gift-card transfer TOCTOU / clawback / expiry boundary / per-admin cap — **CLEARED**.
 - Loyalty stamps — **CLEARED** (atomic daily-cap + once-per-booking UPDATE).
 - Route guards — **CLEARED** (admin group + in-handler backstops; all 4 missing backstops now added).
 - SCA/2FA boundary — **VERIFIED-OK**: SCA-only for charges (token required in enforced env), homegrown 2FA only for account/admin actions, fail-closed default, no header-based gate bypass.
 - `snapshot` encryption plaintext fallback — **KNOWN/ACCEPTED** (documented money-safety-over-PII tradeoff; CRITICAL startup warning).
 - App-clock vs DB-clock money gates — **THEORETICAL/LOW**, no exploit path (server clock not client-controlled).
 ## REMAINING KNOWLEDGE (intentional, documented)
 - `SNAPSHOT_ENC_KEY` unset → plaintext snapshot fallback (startup CRITICAL; ops must set it in prod).
 - **2FA/verification-code delivery**: intended channel is email/SMS (P6), not yet wired. Stdout-log delivery (`[2FA]`/`[VERIFY]`) is a **local DEV ONLY feature** — dev/test builds only. Production builds have no delivery channel; code issuance FAILS CLOSED (503) until email/SMS ships. The `TWO_FACTOR_ALLOW_LOG_DELIVERY` production opt-in was **removed entirely**.
 - TRUST_PROXY_HEADERS=false default means rate limiting sees the proxy IP behind nginx (startup warning; ops decision).
 - LocalStorage tokens + no hard CSP on the SvelteKit shell beyond the added meta (residual XSS-exposure risk; documented).
 **Session verdict:** review FAILED at start (2 critical wire-contract breaks + 30+ issues) → after 4 fix/review loops: **all issues fixed, all tests green, all keystone journeys runtime-verified.**
@@ -1,282 +0,0 @@
 # Staged Default Hours Change with Conflict Resolution
 ## Overview
 When an admin edits default working hours, instead of applying changes immediately:
 1. Run conflict resolution (same pattern as holiday hours/time blockers)
 2. Stage the change with a future effective date (admin picks, default +2 weeks)
 3. Current hours continue to apply until the switch-over date
 4. At midnight on the effective date, the change auto-applies and triggers a notification
 5. Contact page shows "These opening hours will change from [date]"
 6. Available hours use current hours before the date, new hours after
 ---
 ## What Changes
 ### New DB Table: `default_hours_scheduled_changes`
 ```sql
 CREATE TABLE default_hours_scheduled_changes (
    id SERIAL PRIMARY KEY,
    effective_date DATE NOT NULL,           -- London midnight date to switch over
    created_by CHAR(12) NOT NULL REFERENCES users(id),
    created_at TIMESTAMPTZ DEFAULT NOW(),
    applied_at TIMESTAMPTZ,                 -- NULL until cron applies it
    cancelled_at TIMESTAMPTZ,               -- NULL unless admin cancels
    hours JSONB NOT NULL                    -- [{weekday, startTime, endTime, isOpen}]
 );
 ```
 Only ONE pending change is allowed at a time. If a pending change exists and the admin tries to create another, they must cancel the existing one first.
 ### New `admin_notification_reason` enum value
 ```sql
 ALTER TYPE admin_notification_reason ADD VALUE 'default_hours_changed';
 ```
 Used by the cron job when it applies the change — creates a single notification for admin review.
 ---
 ## Files to Create/Modify
 ### Backend
 | # | File | Action |
 |---|---|---|
 | 1 | `init-scripts/init-script.sql` | Add new table and enum value |
 | 2 | `backend/handlers/scheduling/default-hours.go` | Add `ScheduleDefaultHoursChange`, `GetScheduledDefaultHoursChange`, `CancelScheduledDefaultHoursChange` handlers |
 | 3 | `backend/handlers/scheduling/default-hours.go` | Modify `GetDefaultHours` — return both current and pending future hours |
 | 4 | `backend/handlers/scheduling/default-hours.go` | Modify `computeAvailableHours` / `GetWorkingHours` — inject future hours for dates >= effective_date |
 | 5 | `backend/handlers/scheduling/scheduled-cleanup.go` | Add `ApplyScheduledDefaultHours` cron handler |
 | 6 | `backend/internal/jobs/cleanup.go` | Register `apply-default-hours` cron job (daily at 00:05) |
 | 7 | `backend/main.go` | Register new routes |
 ### Frontend
 | # | File | Action |
 |---|---|---|
 | 8 | `frontend/src/lib/components/admin/WeeklySchedule.svelte` | Add conflict resolution UI + date picker + staged save flow |
 | 9 | `frontend/src/lib/components/layout/BusinessHours.svelte` | Show upcoming hours change with date |
 | 10 | `frontend/src/routes/admin/+page.svelte` | Wire modal props if needed |
 ---
 ## Detail: Backend Design
 ### Handler: `ScheduleDefaultHoursChange` (POST)
 `PUT /api/admin/default-hours` → replaced with a staging flow:
 ```
 POST /api/admin/default-hours/schedule
 Content-Type: application/json
 {
    "hours": [{"weekday": 0, "startTime": "10:00", "endTime": "18:00", "isOpen": true}, ...],
    "effective_date": "2026-08-17"   // optional, defaults to +14 days from today London
 }
 ```
 **Flow:**
 1. Parse + validate input
 2. Check for existing pending change — return 409 if one exists
 3. Run conflict detection between current bookings and the proposed hours
 4. If conflicts exist → return 409 with `{error: "conflicts exist", bookings: [...]}` (same format as holiday hours)
 5. If no conflicts → store the pending change, return 201 with `{effective_date: "2026-08-17"}`
 ### Handler: `GetScheduledDefaultHoursChange` (GET)
 ```
 GET /api/admin/default-hours/scheduled
 ```
 Returns the pending change or 404:
 ```json
 {
    "effective_date": "2026-08-17",
    "hours": [...],
    "created_at": "...",
    "created_by": "..."
 }
 ```
 ### Handler: `CancelScheduledDefaultHoursChange` (DELETE)
 ```
 DELETE /api/admin/default-hours/scheduled
 ```
 Sets `cancelled_at` on the pending change. Returns 200.
 ### Handler: `ApplyScheduledDefaultHours` (cron)
 Runs at "5 0 * * *" (00:05 daily — 5 minutes after midnight to avoid midnight race conditions).
 **Flow:**
 1. Query `default_hours_scheduled_changes` where `effective_date <= CURRENT_DATE` (London time) AND `applied_at IS NULL` AND `cancelled_at IS NULL`
 2. For each due change:
   a. BEGIN transaction
   b. DELETE all existing `working_hours` rows
   c. INSERT new rows from the change's hours JSON
   d. INSERT an `admin_notifications` with reason `default_hours_changed` (details: "Default hours changed from [old summary] to [new summary]")
   e. SET `applied_at = NOW()`
   f. COMMIT
 ### Modified: `GetDefaultHours`
 Current: returns `SELECT weekday, start_time, end_time, is_open FROM working_hours ORDER BY weekday`
 New: if a pending change exists with `effective_date > today`, include an extra field:
 ```json
 [
    {"weekday": 0, "startTime": "00:00", "endTime": "00:00", "isOpen": false, ...},
    ...
 ]
 ```
 Response changes to include optional `scheduled_change` field:
 ```json
 {
    "current": [...],
    "scheduled_change": {
        "effective_date": "2026-08-17",
        "hours": [...]
    }
 }
 ```
 This is backwards-compatible for existing consumers — they read the array from `current`.
 ### Modified: `computeAvailableHours` / `GetWorkingHours`
 The hours resolution currently goes: proposed > exceptional > default > closed.
 For the staged hours feature, I need to inject the **scheduled future default hours** for dates >= effective_date:
 Priority order: proposed > exceptional > scheduled_future_default > default > closed
 In `computeAvailableHours` (the per-day loop), after looking up the `defaultMap` entry:
 ```go
 // Before falling through to default/closed, check if there's a staged future change
 if scheduledChangeHours != nil && d.In(londonLocation).Format("2006-01-02") >= scheduledEffectiveDate {
    if fh, ok := scheduledChangeHours[weekday]; ok {
        baseStart = fh.StartTime
        baseEnd = fh.EndTime
        isOpen = fh.IsOpen
        day.Source = "scheduled_change"
    }
 } else if def, ok := defaultMap[weekday]; ok {
    // existing default logic...
 }
 ```
 This means:
 - Today → current working hours apply
 - Between today and effective_date → current working hours apply (no change)
 - On/after effective_date → new scheduled hours apply
 - Exceptional hours always override (higher priority)
 For `GetWorkingHours`, same logic applies — it needs to return the correct hours for each date in the range.
 ---
 ## Detail: Frontend Design
 ### WeeklySchedule.svelte — "Edit Schedule" Modal
 The modal gets a new top section (same pattern as HolidayHours conflict resolution):
 **Step 1: Admin opens modal, edits hours**
 Same time picker interface as today. No change to the editing UX.
 **Step 2: Conflict resolution (new)**
 - A date picker for "Apply from" (defaults to +14 days from today in London)
 - A `checkConflictingBookings()` function that:
  - Takes the proposed hours + effective date range (from effective_date to effective_date + 90 days or so)
  - Calls `POST /api/admin/default-hours/schedule` with a dry-run flag or a dedicated conflict endpoint
  - Shows amber warning with conflicting bookings + "View Booking" / "View Client" buttons
 - Guard: submit button disabled while conflicts exist
 **Step 3: Submit**
 - Button text: `Schedule Change for [date]` 
 - On success toast: `Default hours will change at 23:59 on 17/08/2026`
 - The modal closes, admin sees a "Pending change" indicator on the WeeklySchedule card
 **Step 4: Pending change indicator**
 - After a change is scheduled, the WeeklySchedule card shows:
  - An amber banner: "Default hours are scheduled to change on 17/08/2026"
  - A "Cancel" button that calls `DELETE /api/admin/default-hours/scheduled`
 - The `fetchDefaultHours` response now includes `scheduled_change` — display it
 ### BusinessHours.svelte — Contact Page
 Add a new section below the current "Upcoming Holiday Hours" section:
 ```svelte
 {#if scheduledChange}
    <hr class="my-2 border-gray-200" />
    <p class="mb-2 text-center text-xs font-medium text-amber-600">
        Opening hours will change from {formattedDate}
    </p>
    {#each scheduledChange.hours as h}
        <div class="flex items-center justify-between text-sm">
            <span class="font-medium text-gray-700">{dayNames[h.weekday]}</span>
            <span class="text-gray-500">
                {#if h.isOpen}
                    {formatTime(h.startTime)} – {formatTime(h.endTime)}
                {:else}
                    Closed
                {/if}
            </span>
        </div>
    {/each}
 {/if}
 ```
 To load this, the `fetchData` function needs an additional API call:
 ```
 GET /api/admin/default-hours/scheduled  (public, or a new public endpoint)
 ```
 Since this is shown on the public contact page, the endpoint should be under the public read-only group (OptionalAuth) — similar to how `GET /scheduling/working-hours` is public.
 ### admin/+page.svelte
 If `WeeklySchedule` needs `openUserModal` / `openBookingModal` for conflict resolution, wire the same props. Currently `WeeklySchedule` does not accept these props, but the conflict resolution flow needs "View Client" and "View Booking" buttons.
 ---
 ## Conflict Resolution vs Holiday Hours — Reusing the Pattern
 The conflict detection for default hours changes reuses the EXACT same pattern as the holiday hours conflict handler, but with a date range instead of week starts:
 ```
 POST /api/admin/default-hours/conflicting-bookings
 {
    "proposed_hours": [{"weekday": 0, "startTime": "10:00", "endTime": "18:00", "isOpen": true}, ...],
    "start_date": "2026-08-17",
    "end_date": "2026-11-17"   // default: +90 days from effective date
 }
 ```
 This reuses the same `parseTimeToMinutes` comparison logic and the `ActiveBookingStatuses` filter. The response is the same `OverlappingBookingsResponse` format.
 **Note:** The availability-for-reschedule logic is simpler than holiday hours because:
 - Default hours changes apply PERMANENTLY (not per-week like holiday hours)
 - The "what hours apply for rescheduling" is just: current hours until effective_date, future hours after
 - No per-week mapping needed
 ---
 ## Risks
 | Risk | Mitigation |
 |---|---|
 | Admin sets effective_date in the past | Validate: must be >= tomorrow (London date + 1) |
 | Cron miss at midnight doesn't apply change | Cron runs 00:05 to avoid midnight race. Query uses `<= CURRENT_DATE` so it catches any missed days |
 | Two admins try to schedule simultaneously | Unique constraint on `(applied_at IS NULL AND cancelled_at IS NULL)` — use a partial unique index |
 | Notification is an admin-only todo placeholder | Add `default_hours_changed` to the enum, create one notification. Future work: user notification |
 | Scheduling a change far in the future (6+ months) | Conflicts only checked against existing bookings in the window. Long-range changes may need re-checking when new bookings are made — acceptable for v1 |