Money-safety idempotency fixes (external review bugs 1-3):
- processChargeGroup: aggregated refund key now hashes the sorted pending-row
set (chargeID-square-agg-<sha256 suffix>) so a changed group can never mark
a new row completed against an old smaller refund; >45-char chargeIDs use a
hashed prefix instead of verbatim truncation (which would collide charges on
Square's global key dedup). Same-set crash-retry keeps Square's dedup.
- CreateTerminalPayment saved_card: two-tier idempotency key — client-supplied
per-attempt UUID preferred (distinct identical charges no longer collapse),
deterministic booking+type+amount+card fallback for no-key retry safety.
PaymentModal sends a per-charge UUID cleared after success.
- ensureRefundKey: legacy NULL-key manual refunds persist a generated key to
the row BEFORE the Square call (race-safe AND idempotency_key IS NULL guard),
so a lost-response retry reuses the key and never double-refunds. Wired into
resumeManualPendingRefund and the sweep's manual-retry loop.
Classification + money-safety hardening:
- till.go/sweep.go: structured square.ErrorCode/IsNotFound are authoritative
when present; message-substring matching only for non-structured errors
(dev mock, client-side status errors). Fixes fragile string-matching driving
sweep retries and gift-card clawbacks.
- SaveCardForUser: ON CONFLICT (user_id, square_card_id) DO NOTHING + re-select
(was a latent UNIQUE-violation 500 on save-card retry).
- CreateBookingPayment: partial payments re-validated against remaining balance
inside the advisory lock (closes concurrent-overpayment race).
- InvalidateSquareCustomerCache on GDPR erasure paths (account.go,
time-blockers.go stale-guest anonymization).
- GetUserGiftCardBalanceAdmin: in-handler admin check (defense-in-depth).
- getCheckoutHTTP: warn on multi-payment checkouts instead of dropping
payments[1:].
- Cash/giftcard terminal branch: removed dead idempotency SELECT, "tip-" ->
"till-" prefix.
- UserPaymentModal: removed vestigial polling state; proper interval cleanup.
- account/+page.svelte: gift-card redeem dialog links /terms.
- nginx CSP: allow *.squarecdn.com and js.squareup.com so the Square Web
Payments SDK + card iframe can tokenize behind the proxy.
Tests: +8 regression tests covering changed-set refund keys, legacy NULL-key
single-refund, saved-card client-key dedup/no-dedup, concurrent partials, and
cache invalidation. Full suite + race detector clean via run-tests.sh lockfile.
Re-review (2 Oracle + security + QA + librarian + context-miner) surfaced fixes, all applied: (1) UserPaymentModal and the account-page gift-card buy now key the cached idempotency key on a stable 'new-card' sentinel instead of the cnon: nonce, so clearing the nonce on a failed charge no longer regenerates the key — a lost-response retry now dedups at Square instead of double-charging (the tip flows already keyed on amount only). (2) The create-with-redeem clawback now logs CRITICAL when the guarded balance reversal is blocked (previously silent), and its transaction DELETE is scoped to this sale instead of deleting every transaction on the card. (3) reconcileStalePaymentAtSquare now treats APPROVED/PENDING as non-terminal (leave pending) instead of definitively failed, matching Square's documented state machine. (4) GetTillCheckoutStatus returns 404 for a sale already swept to failed instead of reporting a live state. (5) The critical-payment scan job skips candidates whose booking was hard-deleted, so one orphan can no longer silence all critical alerts.
Adds the scan-critical-payment-logs job (daily 2:45am) that surfaces stale pending payments/till-sales and refunds at the retry cap as admin_notifications with reason='critical_payment_log' — the app has no log/alert pipeline (Gap Backlog T14), so money events that would otherwise sit in un-watched CRITICAL log lines now reach the owner's in-app notification centre. Dedup is NULL-safe (IS NOT DISTINCT FROM) and re-surfaces acknowledged-but-still-unresolved rows. Stopgap until a real alerting pipeline lands.
Square rejects POST /v2/cards without card.customer_id at runtime (confirmed by Square's SDK maintainer); the production client omits an empty id via omitempty and every caller provisions a customer first, so the mock now enforces the same structured 400 INVALID_REQUEST_ERROR to keep sandbox/dev parity with the gate the production code depends on.
square_webhook_events is now CREATE TABLE IF NOT EXISTS, registered as the 24th maintenance job (sweep-square-webhook-events, daily 2:30am) pruning rows older than 90 days, and testdb gives a clear docker compose hint when the admin DB connection fails.
Adds SquareClient.DeleteCustomer for GDPR erasure (DELETE /v2/customers/{id}, NOT_FOUND as no-op), bounds doJSON response reads to 1 MiB with rune-safe 500-byte error snippets, adds validCardID guard to the disable-card URL, makes paymentFromSquare card fields consistent when the card ID is empty, redacts ccof/cnon tokens in all log paths, and fixes the idempotency-key-length comment (45 chars for payments/refunds/cards, 64 only for terminal checkouts).
Add StatusCode/Category/Field to squareAPIError and an IsNotFound helper so 400/401/404/429/5xx are distinguishable structurally instead of by substring. Validate cnon:/ccof: token prefixes in createPayment/createCardOnFile (PCI parity with the mock). Reject ccof charges without customer_id in the mock so dev parity catches the production bug. Emit Deadline as the RFC 3339 duration (PT5M) and correct the deprecated-comment.
R1/R4: saved_card branch in CreateTerminalPayment now mirrors CreateTipPayment
- advisory lock (crussell:payment:<bookingID>) serializes concurrent double-clicks
- deterministic key bookingID-sc-type-amount-cardID (<=45 chars) so a lost-response
retry derives the same key and dedups instead of double-charging
- idempotency switch inside the lock: completed -> dedup, pending -> reuse with
pence amount-guard, failed -> clean 409
- success response includes card_brand/card_last4 (frontend already reads them)
R2: add 'failed' case to all four retry switches (tip, booking, gift card, till)
- a swept/definitively-rejected record returns 409 instead of 500-ing on the
idempotency_key UNIQUE constraint
R3: extend SweepStalePendingPayments to till_sales card rows
- sweeps pending till_sales (online_square/in_person_card) past Square's ~24h
key retention, closing the double-charge window for till sales
- swept rows logged with the same CRITICAL manual-reconciliation marker as the
refund sweep
Webhook fail-closed: reject 503 when SQUARE_WEBHOOK_SIGNATURE_KEY unset, 403 on
bad signature (was: skip verification in dev)
Refund status resolution: refunds now resolve by Square status
(COMPLETED/PENDING/FAILED/REJECTED) instead of assuming completed; real error
codes (REFUND_AMOUNT_INVALID, PAYMENT_NOT_REFUNDABLE, REFUND_ALREADY_PENDING)
added to the definitive/processed classification
HTTP client: CreateCard key truncated to <=45 chars, device_options always sent
(env SQUARE_TERMINAL_DEVICE_ID fallback), processing_fee reads amount_money,
ListCards cursor loop, refund keys hashed to <=45 chars
Other fixes: payment/till/gift-card advisory-lock + FOR UPDATE asymmetries,
GetPaymentByID NULL scans, loyalty redemption lock, card upsert on conflict,
mock ccof: prefix parity, IsValidSquareCheckoutID for real Square IDs,
isAdminRequest defense-in-depth on all 6 admin payment handlers, webhook
signature docs, M8/L5 debug markers removed
Docs: README/FC/TM/Overview updated (22 jobs, 20 CRITICAL sites, 23-section
GDPR export, sweep jobs, webhook fail-closed); P11 plan marks remaining items
(sandbox smoke test, M-8 customer_id, saved-card key dedup trade-off) as
deferred with rationale; gap backlog pruned of completed items
The app does not provision Square customers, so sending the local user ID as
customer_id in Create Card was rejected with CUSTOMER_NOT_FOUND, and filtering
List Cards by it returned nothing. reference_id is Square's free-form client
reference — max 128 chars, no uniqueness constraint — and is echoed in both
Create and List responses.
- Create Card payload: reference_id = local user ID (customer_id absent)
- List Cards: native ?reference_id=<userID> filter (no limit/customer_id,
no client-side filter, no cursor handling needed)
- Mock parity: CreateCardOnFile stores ReferenceID; GetCardsOnFile unchanged
- Regression guards: TestCreateCardOnFileHTTP_IdempotencyKey asserts
reference_id=user_1 and customer_id ABSENT; new
TestGetCardsOnFileHTTP_ReferenceIDFilter asserts the query shape
Refund system (Round 3 fixes + follow-up + alignment):
- Serialize cancellation refunds against the manual handler via
per-payment advisory locks taken before the prior-refunds read
(pg_advisory_xact_lock, ascending, same crussell:refund: key space)
- Aggregate pending cancellation refunds into ONE Square refund per
charge (stable charge-level -square-agg key); atomic group UPDATE
keeps crash-retry amounts identical for Square key-dedup
- Persist paymentID-square-amount idempotency keys on cancellation
refunds; scheduler reads the stored key (legacy fallback for old rows)
- Add sweep-pending-square-refunds cron (*/5, concurrency 1) with
refund_attempts cap; sweep retries stale manual pending refunds with
each row's own stored idempotency key
- Reconcile at Square (GET /v2/refunds ListPaymentRefunds) before every
terminal failed transition: tri-state result leaves rows pending on
reconcile error instead of false-failing; PAYMENT_ALREADY_REFUNDED
resolves to completed
- Move over-refund guard inside the lock, counting completed + pending
(excluding failed); ErrRefundDeclined distinguishes definitive vs
ambiguous outcomes
- forgiveFees now executes a real full refund (forceFullRefund override)
with admin_forgiven_fees reason threaded to Square
- Surface failed card refunds in the admin notification centre
(refund_failed enum, RETURNING-id pre-pass inserts, NOT EXISTS dedup)
- Dedup double-cancel refund inserts via ON CONFLICT (idempotency_key)
DO NOTHING without consuming refundRemaining
Frontend:
- Remove all raw-PAN card entry: zero card_number/card_cvc/new_card_token
in request bodies; gate new-card entry behind CardEntryUnavailable
notice + newCardDisabled prop across all 8 flows
- Delete hand-rolled CardInput.svelte; keep CardSelection saved-card UI
and CardEntryUnavailable fallback
- Update cancellation-policy page to in-person cash pickup wording
Tests:
- Rewrite the two amount-blind dedup tests to assert real money movement
(single call, aggregated amount, shared refund ID)
- Add coverage: manual refund vs cancellation serialization (concurrent
goroutines), reconcile error vs no-match branches, stale manual retry,
forgive-fees real refund row + reason, double-cancel dedup, mock refund
key dedup, ListPaymentRefunds filtering
- Fix time-dependent booking flakes with fixtures.NextWorkingDayAt
- 25/25 packages pass; -race clean on payments/square/db/jobs/bookings
P0 — float truncation: applied math.Round to all remaining int64(x*100)
sites (till penceAmount, refund over-refund guard, GetAlreadyRefundedAmount,
payment summary conversions). A £1.14 till sale previously charged 113p.
P0 — raw PAN stopped at the API edge:
- Deleted CardNumber/CardExpMonth/CardExpYear/CardCVC from TillSaleRequest
and CardNumber/Expiry/CVC from CreatePaymentMethodRequest. Both now accept
card_token (Square nonce) and return 400 when absent. PAN+CVV no longer
transit the application server (PCI-DSS SAQ-A scope).
- Deleted CreateCardOnFileRaw from the SquareClient interface and all
implementations (MockClient, ProdClient, devProdClient).
- Added idempotency_key column to refunds table (UNIQUE).
P0 — RefundPayment hardened: advisory lock on payment ID (prevents two
concurrent refunds passing the over-refund guard), pending-refund-record-
then-Square pattern (scheduler reprocesses on failure), same-key dedup.
P1 — till sale pending-retry now re-attempts the Square charge instead of
returning the stale 'pending' status (gift card was already funded in the
committed tx — silent money loss otherwise). Sale row reused, not duplicated.
P1 — idempotency key caching in frontend: BuyGiftCard and
UserPaymentModal/BookingFlow now cache the key per amount+card, regenerated
on change and cleared on success — matches the tip-flow pattern so a
lost-response retry dedups instead of double-charging.
P1 — CreateTerminalPayment cash/giftcard INSERTs now persist idempotency_key.
Key is unique per payment (booking+type+amount would wrongly dedup two
legitimate identical payments, e.g. two £50 cash receipts).
P1 — gift-card codes no longer logged (spendable credential; value+recipient
only).
Tests: till pending-retry re-attempt, refund same-key dedup, mock CreatePayment
idempotency dedup, CreatePaymentMethod nonce happy path + raw-PAN rejection,
till online_square card_token required/valid.
N1 (HIGH) — BuyGiftCard concurrent same-key retry could double-issue gift
cards (2× value for 1 charge). Added pg_advisory_lock on the idempotency key
(mirroring the tip pattern) acquired before the idempotency check, so
concurrent same-key retries serialize and only one executes gift-card
creation.
N2 — Amount-equality guards in both reuse branches (CreateTipPayment and
BuyGiftCard). A same-key retry with a different amount now returns 400
instead of silently mutating the pending record's books/VAT/refund caps.
N3 — test coverage:
- TestBuyGiftCard_RetryPending_ReattemptsCharge: pending record + same-key
retry re-attempts, reuses the record (count=1), completes, and issues the
gift card exactly once.
- TestCreateCheckoutHTTP_DeviceOptionsWireShape: httptest.Server asserts
device_id is under checkout.device_options (not top-level). Extracted
createCheckoutHTTPWithClient for injectable base URL.
- MockClient.CreatePayment now dedups on idempotency key (paymentByKey map),
matching real Square behaviour.
N4 — Corrected the savepoint comments in handlers.go and giftcards.go: the
savepoint only exists in the test harness; in production db.Conn.Begin is a
plain tx and the status UPDATE runs on a separate pooled connection. Commit
is a harmless no-op in prod but required in tests.
Bonus bug fixed: CheckIdempotencyByKey scanned NULL booking_id/gift_card_id
(gift-card purchases) into plain string, failing with 'cannot scan NULL'.
Now uses sql.NullString.
Docs: Technical Manual.md:53 and Feature Catalog.md (2.1, 2.5) corrected —
no longer claim Web Payments SDK is live; new-card entry is documented as
pending P11, saved-card flow works via ccof tokens, dev mock rejects raw PANs.
CRITICAL — same-amount tip retry silently never charged:
- CreateTipPayment idempotency check now only short-circuits when the
existing record is 'completed'. A 'pending' record (previous Square call
failed) is REUSED and the charge re-attempted with the same key (Square
dedups safely), instead of returning the stale pending record as 200 with
a success toast and no charge.
- Same fix in BuyGiftCard: pending records trigger a re-attempt, not a
false-success response. Unique idempotency_key constraint means the
pending record must be reused, not re-inserted.
- Fixes the savepoint/rollback interaction: the nested tx (savepoint) is
now committed in the reuse path so the deferred rollback doesn't undo the
later status UPDATE on the same connection.
- Regression test: TestTipPayment_RetryPending_ReattemptsCharge verifies a
pending record + same-key retry re-attempts and completes, reusing the
record (count stays 1).
MAJOR — terminal checkout wire contract:
- device_id now sent as checkout.device_options.device_id (Square's required
shape), not a top-level field which Square rejects with 400.
- 'checkout pending' detection now uses typed sentinel ErrCheckoutPending
with errors.Is in both handlers, matching mock and real HTTP client.
MAJOR — exp_month/exp_year omitted from card creation payload when unset
(now *int with omitempty) — Square would 400 on 0/0; expiry comes from the
tokenized source.
Docs:
- README payments/infrastructure sections corrected (Web Payments SDK claim
replaced with accurate P11-backlog note; dev mock parity described)
- Future Work P11 updated to reflect raw-PAN rejection is now enforced in
both mock and prod (new-card flows are a documented dead end)
- Added plans/p11-square-web-payments-sdk.md: full implementation plan +
handoff prompt for the agent picking up P11 (Web Payments SDK nonces)
Previously the dev MockClient was more permissive than production:
- MockClient.CreateCardOnFileRaw processed raw PANs and stored mock cards,
while ProdClient and devProdClient both block raw PANs. A dev testing the
raw-card flow saw it succeed, masking a production failure.
- MockClient.CreateCardOnFile accepted raw PANs as source_id via an
isAllDigits branch. Real Square only accepts cnon:xxx/ccof:xxx tokens.
Now the mock behaves identically to production:
- CreateCardOnFileRaw returns the same PCI error as ProdClient
- CreateCardOnFile validates source_id is token-like (cnon:/ccof:) and
rejects raw PANs
- Removed dead isAllDigits helper
Tests updated to assert the parity behavior:
- TestDevClient_CreateCardOnFileRaw_Rejected_ProdParity (table-driven,
replaces 5 brand-specific raw-PAN tests)
- TestDevClient_CreateCardOnFile_RejectsRawPAN (replaces RawNumber)
- TestCreatePaymentMethod_HappyPath / SecondCardNotDefault now expect
500 instead of 200, documenting the prod block
Money-moving fixes:
- Tip idempotency key regenerates when the tip amount changes after a failed
attempt (all 3 tip flows). Cached key still reused on same-amount retry
(dedup intact) and cleared on success/modal reset. Prevents silent
under-charge when a user retries at a different amount.
- Till replay path returns actual till_sales.status (may be 'pending') instead
of hardcoded 'completed' — no more misreported successful charge.
- BuyerEmail wired for CreateBookingPayment, gift card purchases, and till
sales (saved_card + online_square), matching the tip flow. Email lookup
errors logged, non-fatal.
- Till buyer-email errors now logged (was silently swallowed).
- on_the_house till top-up uses cached getIdempotencyKey() for retry-safe dedup
(was fresh crypto.randomUUID()).
Test/validation fixes:
- Add TestPaymentFromSquare_* unit tests (else-branch + nil card details),
build tag relaxed to 'test' so they run in the standard dev suite.
- Add TestValidateCardInfo table test (7 cases: both/either/neither/empty).
- Add TestCreateTillSale_TwoIdenticalCreateSales_BothSucceed regression test.
- Remove dead mock pre-registration in TestTipPayment_WithSavedCard.
- Correct misleading till regression-test comment.
ESLint cleanup (12 errors -> 0):
- Remove unused loadingCards in tip + pay-tip pages (dead assignments in
loadSavedCards).
- Scoped eslint-disable for {@html} in CardBrandIcon (hardcoded brand SVGs).
- Remove dead confirmSaveDefaultHours + unused rescheduleVersion prop in
WeeklySchedule (and its parent pass-through).
- Replace new Date() with SvelteDate in WeeklySchedule + BusinessHours.
- Fix each-block key in BusinessHours skeleton loader.
- Use void expression for reactivity-tracker reads in effects.
Add ApplyScheduledDefaultHours which checks default_hours_scheduled_changes at midnight, bulk-updates working_hours with staged values, and inserts an admin_notification. Register as the 'apply-default-hours' job in the scheduler (daily at 00:05).
- square_dev.go: return a copy of CheckoutResult to the caller
instead of sharing the map-stored pointer, preventing the goroutine
from racing with the caller on Status field reads
- scheduler_test.go: remove t.Parallel from TestInitHostname_* tests
which modify a shared package-level hostname variable
New test files cover previously untested paths across DAV, validators,
S3, Square, mw, bookings, user, and payments packages.
Includes mock fix: HoldCheckouts flag on MockClient allows tests to
pause auto-complete goroutine for testing PENDING checkout states.
Coverage: 50.4% → 65.0% (+14.6pp)
Restore processImage (images.go) and nonDepositPaymentType (handlers.go) with //nolint:unused — used in test files.
Fix 97 tx.Rollback defers to silently discard expected "tx is closed" error after commit.
Frontend: remove 44 unused shadcn-svelte files, 2 dead components, 9 stale npm deps, prune unused exports.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
errcheck: add proper error handling with slog.Error for tx.Rollback, key generation, and s3/dav operations. Add nolint comments for intentionally discarded DB scan errors and HTTP write errors.
unused: remove dead code (svcRow type, processImage, nonDepositPaymentType, generateSecureCode, colorBold, nGreen, nRed)
gosimple S1021: merge var declaration with assignment in manage.go
ineffassign: remove dead assignments in settings.go, till.go, images.go
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Move m.mu.RUnlock to defer in GetCheckout and remove duplicate RLock/RUnlock around m.completed lookup. This prevents a deadlock when CheckoutPending error is returned but the RLock was already released.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Replace direct *pgxpool.Pool usage with PoolProxy wrapper across the entire backend:
- db.DB renamed to db.Conn (*pgxpool.Pool -> *PoolProxy)
- JWT functions now accept context.Context instead of using context.Background()
- Handler DB calls route through PoolProxy for per-test transaction support
- Fixture/helper/testdb functions accept Querier interface for decoupling
- Query ordering fixed in bookings handlers: COUNT after data query to avoid pgx conn busy
- Time truncation fixed: time.Date instead of Truncate(24*time.Hour) for week start calc
- testmain_test.go files updated with SeedBaseline and NewPoolProxy
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Name history system tracks user name changes and displays former names on bookings, appointments, and admin views until consumed by the first completed booking post-change.
- Add name_history queries to today.go, bookings.go, manage.go, profile.go
- Show previous first/last name on today appointments, pending approvals,
booking details, edit requests, and admin user views
- Paginate bookings by start_time instead of created_at (more intuitive ordering)
- Add name change detection in UpdateProfileHandler with history insert
- Support DAV_BASE_URL env var for configurable CardDAV endpoint
- Add referral_savings to profile response
- Add ParseCursor3 validator for user list cursor pagination
- Consume name_history entries when a booking is completed (ProgressBookingHandler)
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Migrate all test files from resetTestData(t) to testutils.SetupTestDB(t) for isolated per-package test databases.
- Add new feature tests: name history assertions, referral discount preview,
time blockers, email validation, GDPR export, loyalty manual redemption
- Update existing tests to use batch queries and SetupTestDB
- Remove test_helpers.go resetTestData infrastructure
- Add comprehensive user profile tests (442 new lines)
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
S3 dev implementation now uses AWS SDK v2 for RustFS local storage. Prod stub remains placeholder. Both implement Uploader interface with Upload, Download, Delete, GetURL methods.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Backend now stores AVIF, WebP, JPEG, and optional JXL variants for both full-size and thumbnail images. Database schema extended with 7 new columns. Image validation supports AVIF and JXL magic bytes. Comprehensive test coverage for all format URL fields and magic byte detection. Legacy single-URL images remain backward-compatible.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
popertots
