Expand payment remediation test coverage

Adds tests for chargeFailureStatus retryable-vs-definitive classification (429/408/425 -> 503, 4xx declines -> 402), legacy NULL-key refund resume, tip/discount split-record math, and the shared charge helpers adopted by till and gift-card paths.

backend/handlers/payments/errors_test.go

@@ -0,0 +1,161 @@
+//go:build test && dev
+
+package payments
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"reflect"
+	"testing"
+
+	"crussell/internal/square"
+	"crussell/testutils"
+)
+
+// structuredSquareAPIError returns an error of the SAME concrete type the real
+// Square client produces for structured API errors (the unexported
+// *square.squareAPIError, re-stamped with the given HTTP status). The type is
+// not nameable outside internal/square and there is no exported constructor,
+// so the helper clones the dev mock's real structured 400 error (the only
+// package-visible producer) via reflection and rewrites its status code. This
+// mirrors the existing test's "build through the mock" style while covering
+// status codes the mock cannot produce (429/408/425/422/500).
+func structuredSquareAPIError(t *testing.T, status int) error {
+	t.Helper()
+	mc := square.NewDevClient().(*square.MockClient)
+	_, err := mc.CreatePayment(context.Background(), square.CreatePaymentReq{
+		Amount:   1000,
+		Currency: "GBP",
+		SourceID: "ccof:card_1",
+	})
+	if err == nil {
+		t.Fatal("expected the mock to reject a ccof charge without a customer")
+	}
+	if square.ErrorStatusCode(err) == 0 {
+		t.Fatal("expected the mock's ccof rejection to carry a structured status code")
+	}
+	v := reflect.ValueOf(err)
+	if v.Kind() != reflect.Ptr {
+		t.Fatalf("expected the structured error to be a pointer, got %v", v.Kind())
+	}
+	clone := reflect.New(v.Elem().Type())
+	clone.Elem().Set(v.Elem())
+	clone.Elem().FieldByName("StatusCode").SetInt(int64(status))
+	return clone.Interface().(error)
+}
+
+// TestChargeFailureStatus classifies Square CreatePayment errors into
+// 402 (definitive decline) vs 503 (ambiguous) so the charge handlers surface
+// retryable failures as 503 (the pending record is resumed on a same-key
+// retry) and only definitively-rejected charges as 402.
+func TestChargeFailureStatus(t *testing.T) {
+	ctx := context.Background()
+	mc := square.NewDevClient().(*square.MockClient)
+
+	// The dev mock produces a structured 4xx squareAPIError for a
+	// card-on-file charge missing its required customer (mirrors a real
+	// Square 400 INVALID_REQUEST_ERROR) — exercises the definitive-decline
+	// classification through the real error type.
+	_, structuredErr := mc.CreatePayment(ctx, square.CreatePaymentReq{
+		Amount:   1000,
+		Currency: "GBP",
+		SourceID: "ccof:card_1",
+	})
+	if structuredErr == nil {
+		t.Fatal("expected the mock to reject a ccof charge without a customer")
+	}
+	if square.ErrorStatusCode(structuredErr) == 0 {
+		t.Fatal("expected the mock's ccof rejection to carry a structured status code")
+	}
+
+	tests := []struct {
+		name string
+		err  error
+		want int
+	}{
+		{"structured 4xx decline → 402", structuredErr, http.StatusPaymentRequired},
+		{"plain mock failure (ambiguous) → 503", errors.New("mock: payment declined (simulated failure)"), http.StatusServiceUnavailable},
+		{"context deadline → 503", context.DeadlineExceeded, http.StatusServiceUnavailable},
+		{"context cancelled → 503", context.Canceled, http.StatusServiceUnavailable},
+		{"nil (defensive) → 402", nil, http.StatusPaymentRequired},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := chargeFailureStatus(tt.err); got != tt.want {
+				t.Errorf("chargeFailureStatus(%v) = %d, want %d", tt.err, got, tt.want)
+			}
+		})
+	}
+}
+
+// TestChargeFailureStatus_RetryableCarveOuts locks the 429/408/425 carve-outs:
+// those retryable/ambiguous 4xx statuses must classify as 503 (ambiguous —
+// retry later), never as the 402 (definitive decline) that the generic 4xx
+// branch would produce. True declines (400/422) and 5xx keep their existing
+// classifications.
+func TestChargeFailureStatus_RetryableCarveOuts(t *testing.T) {
+	tests := []struct {
+		name   string
+		err    error
+		want   int
+	}{
+		{"structured 429 rate limited (retryable) → 503", structuredSquareAPIError(t, http.StatusTooManyRequests), http.StatusServiceUnavailable},
+		{"structured 408 request timeout (ambiguous) → 503", structuredSquareAPIError(t, http.StatusRequestTimeout), http.StatusServiceUnavailable},
+		{"structured 425 too early (ambiguous) → 503", structuredSquareAPIError(t, http.StatusTooEarly), http.StatusServiceUnavailable},
+		{"structured 422 unprocessable (definitive) → 402", structuredSquareAPIError(t, http.StatusUnprocessableEntity), http.StatusPaymentRequired},
+		{"structured 400 bad request (definitive) → 402", structuredSquareAPIError(t, http.StatusBadRequest), http.StatusPaymentRequired},
+		{"structured 500 server error (ambiguous) → 503", structuredSquareAPIError(t, http.StatusInternalServerError), http.StatusServiceUnavailable},
+		{"plain error (ambiguous) → 503", errors.New("mock: payment declined (simulated failure)"), http.StatusServiceUnavailable},
+		{"context deadline (ambiguous) → 503", context.DeadlineExceeded, http.StatusServiceUnavailable},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := chargeFailureStatus(tt.err); got != tt.want {
+				t.Errorf("chargeFailureStatus(%v) = %d, want %d", tt.err, got, tt.want)
+			}
+		})
+	}
+}
+
+// TestCreateBookingPayment_AmbiguousSquareFailure_Returns503 verifies the
+// charge-failure classification end to end: the dev mock's simulated failure
+// is a PLAIN error (no structured Square status), so the handler now returns
+// 503 (ambiguous — the pending record stays pending for a same-key retry)
+// instead of 402 (which implied a definitive decline).
+func TestCreateBookingPayment_AmbiguousSquareFailure_Returns503(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+	_, bookingID, userToken := setupPaymentStatusTest(t, ctx, tx, "confirmed")
+
+	origClient := SquareClient
+	mc := square.NewDevClient().(*square.MockClient)
+	mc.ShouldFail = true
+	SquareClient = mc
+	defer func() { SquareClient = origClient }()
+
+	cardToken := "cnon:test-card-nonce"
+	req := CreateBookingPaymentRequest{
+		Amount:         2500,
+		PaymentType:    "deposit",
+		NewCardToken:   &cardToken,
+		IdempotencyKey: "ambiguous-503-" + bookingID,
+	}
+
+	handler := CreateBookingPayment
+	w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx)
+
+	if w.Code != http.StatusServiceUnavailable {
+		t.Fatalf("expected 503 for ambiguous mock Square failure, got %d: %s", w.Code, w.Body.String())
+	}
+
+	// The pending record must be left pending (not failed) so a same-key retry
+	// reuses it instead of creating a second Square charge.
+	var status string
+	err := tx.QueryRow(ctx, `SELECT status FROM payments WHERE booking_id = $1 AND idempotency_key = $2`, bookingID, req.IdempotencyKey).Scan(&status)
+	if err != nil {
+		t.Fatalf("failed to query payment status: %v", err)
+	}
+	if status != "pending" {
+		t.Errorf("expected payment status 'pending' after ambiguous failure, got %q", status)
+	}
+}

backend/handlers/payments/payment_status_test.go

@@ -822,6 +822,54 @@ func (c *mismatchedRefCheckoutClient) GetCheckout(ctx context.Context, checkoutI
 	return c.result, nil
 }
 
+func TestGetCheckoutStatus_EmptyReferenceID_Returns400(t *testing.T) {
+	// A checkout with an EMPTY reference_id was created outside this app (no
+	// booking was attached at creation time) — it must NOT be attachable to a
+	// booking via polling. Fail closed with 400, exactly like a mismatched
+	// reference, so a mis-scoped charge is never recorded.
+	ctx, tx := testutils.SetupTestTx(t)
+	_, bookingID, _ := setupTestData(t, ctx, tx)
+
+	origClient := SquareClient
+	SquareClient = &mismatchedRefCheckoutClient{
+		SquareClient: square.NewDevClient(),
+		result: &square.PaymentResult{
+			ID:          "pay_empty_ref",
+			Status:      "COMPLETED",
+			Amount:      5000,
+			SquarePayID: "pay_empty_ref",
+			ReferenceID: "", // created outside this app — no booking reference
+			CreatedAt:   "2026-07-31T00:00:00Z",
+			UpdatedAt:   "2026-07-31T00:00:00Z",
+		},
+	}
+	defer func() { SquareClient = origClient }()
+
+	checkoutID := "abcd1234ef56" // 12 hex chars, passes the checkout-id validation
+	req := httptest.NewRequest("GET", "/api/admin/payments/"+checkoutID+"/status?booking_id="+bookingID, nil)
+	rctx := chi.NewRouteContext()
+	rctx.URLParams.Add("checkout_id", checkoutID)
+	reqCtx := context.WithValue(req.Context(), chi.RouteCtxKey, rctx)
+	reqCtx = context.WithValue(reqCtx, mw.UserIDKey, "000000000001")
+	reqCtx = context.WithValue(reqCtx, mw.UserRoleKey, "admin")
+	req = req.WithContext(reqCtx)
+
+	w := httptest.NewRecorder()
+	GetCheckoutStatus(w, req)
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("expected 400 for empty reference_id, got %d: %s", w.Code, w.Body.String())
+	}
+
+	// No payment may be recorded for the unreferenced checkout.
+	var rowCount int
+	if err := tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&rowCount); err != nil {
+		t.Fatalf("failed to count payment rows: %v", err)
+	}
+	if rowCount != 0 {
+		t.Errorf("expected no payment rows after empty reference, got %d", rowCount)
+	}
+}
+
 func TestGetCheckoutStatus_ReferenceIDMismatch_Returns400(t *testing.T) {
 	// The terminal checkout's reference_id must match the booking being
 	// polled; a checkout that references a different booking is refused with

backend/handlers/payments/payments_r6_review_test.go

@@ -3,10 +3,13 @@
 package payments
 
 import (
+	"bytes"
 	"context"
 	"fmt"
+	"log/slog"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"sync"
 	"testing"
 
@@ -345,3 +348,66 @@ type failingDeleteClient struct {
 func (c *failingDeleteClient) DeleteCardOnFile(ctx context.Context, cardID string) error {
 	return fmt.Errorf("square: network error disabling card %s", cardID)
 }
+
+// syncBuffer is a mutex-guarded slog writer so log records can be read safely
+// under -race.
+type syncBuffer struct {
+	mu  sync.Mutex
+	buf bytes.Buffer
+}
+
+func (b *syncBuffer) Write(p []byte) (int, error) {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	return b.buf.Write(p)
+}
+
+func (b *syncBuffer) String() string {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	return b.buf.String()
+}
+
+// tokenSafeFailingDeleteClient fails with a token-free error so the
+// log-redaction test isolates redaction of the square_card_id attribute rather
+// than the error string.
+type tokenSafeFailingDeleteClient struct {
+	square.SquareClient
+}
+
+func (c *tokenSafeFailingDeleteClient) DeleteCardOnFile(ctx context.Context, cardID string) error {
+	return fmt.Errorf("square: network error disabling card at Square")
+}
+
+// TestDeletePaymentMethod_LogsRedactCardToken verifies the local card-delete
+// warning logs the redacted tokenPrefix form of the square_card_id (a ccof:
+// token), never the full value (SECURITY: full ccof tokens must not reach
+// server logs).
+func TestDeletePaymentMethod_LogsRedactCardToken(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+	userID, err := fixtures.CreateTestUser(tx)
+	require.NoError(t, err)
+	fullToken := "ccof:secret_token_123456"
+	cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, fullToken, "VISA", "9999")
+	require.NoError(t, err)
+
+	origClient := SquareClient
+	SquareClient = &tokenSafeFailingDeleteClient{SquareClient: square.NewDevClient()}
+	defer func() { SquareClient = origClient }()
+
+	var sb syncBuffer
+	origLogger := slog.Default()
+	slog.SetDefault(slog.New(slog.NewTextHandler(&sb, nil)))
+	defer slog.SetDefault(origLogger)
+
+	svc := NewPaymentService()
+	require.NoError(t, svc.DeletePaymentMethod(ctx, cardID, userID))
+
+	logs := sb.String()
+	if strings.Contains(logs, fullToken) {
+		t.Errorf("full ccof token %q leaked into logs: %q", fullToken, logs)
+	}
+	if !strings.Contains(logs, "ccof:sec...") {
+		t.Errorf("expected redacted token prefix in logs, got %q", logs)
+	}
+}

backend/handlers/payments/payments_test.go

@@ -5,9 +5,11 @@ package payments
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
+	"math"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -607,8 +609,12 @@ func TestRefund_FullRefund(t *testing.T) {
 
 func TestRefund_SameKeyRetry_Dedups(t *testing.T) {
 	// A retry of the same refund (network timeout, double-click) must not
-	// create a second Square refund or a second DB row — the idempotency key
-	// (paymentID + amount) dedups it.
+	// create a second Square refund or a second DB row. Retry dedup runs on
+	// the CLIENT-supplied idempotency key — a UUID generated per refund
+	// attempt and REUSED on retry (hashed+truncated into the stored key), so
+	// the same key dedups. A NO-key refund now carries a fresh random fallback
+	// key per attempt and never dedups against a prior no-key refund — that is
+	// deliberate (see TestRefund_TwoEqualPartialRefunds_NoClientKey_DoNotCollide).
 	t.Parallel()
 	ctx, tx := testutils.SetupTestTx(t)
 
@@ -631,8 +637,9 @@ func TestRefund_SameKeyRetry_Dedups(t *testing.T) {
 	}
 
 	req := RefundRequest{
-		Amount: 5000,
-		Reason: "customer request",
+		Amount:         5000,
+		Reason:         "customer request",
+		IdempotencyKey: "refund-same-key-retry-uuid",
 	}
 
 	handler := RefundPayment
@@ -642,7 +649,8 @@ func TestRefund_SameKeyRetry_Dedups(t *testing.T) {
 		t.Fatalf("first refund: expected 200, got %d. body: %s", w1.Code, w1.Body.String())
 	}
 
-	// Same-key retry (identical amount, reason) — must dedup, not double-refund.
+	// Same-key retry (identical client key, amount, reason) — must dedup, not
+	// double-refund.
 	w2 := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", req, adminToken, ctx)
 	if w2.Code != http.StatusOK {
 		t.Fatalf("retry refund: expected 200, got %d. body: %s", w2.Code, w2.Body.String())
@@ -659,6 +667,71 @@ func TestRefund_SameKeyRetry_Dedups(t *testing.T) {
 	}
 }
 
+// TestRefund_TwoEqualPartialRefunds_NoClientKey_DoNotCollide verifies the P2
+// security fix for the DEFAULT (no client key) path: the fallback idempotency
+// key is now unique per refund attempt, so two DISTINCT partial refunds of the
+// SAME amount against the SAME payment each get their own key and both
+// complete. With the old amount-derived default key (paymentID + "-refund-" +
+// amount) the second would collide with the first and be silently swallowed by
+// the dedup lookup.
+func TestRefund_TwoEqualPartialRefunds_NoClientKey_DoNotCollide(t *testing.T) {
+	t.Parallel()
+	ctx, tx := testutils.SetupTestTx(t)
+
+	_, bookingID, _ := setupTestData(t, ctx, tx)
+
+	adminID, err := fixtures.CreateTestAdminUser(tx)
+	if err != nil {
+		t.Fatalf("failed to create admin user: %v", err)
+	}
+	adminToken := jwt.GenerateTestToken(adminID, "admin")
+
+	paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "in_person_card", "full", "completed")
+	if err != nil {
+		t.Fatalf("failed to create payment: %v", err)
+	}
+	_, err = tx.Exec(ctx, "UPDATE payments SET square_payment_id = 'sqp_test_no_key_equal_refunds' WHERE id = $1", paymentID)
+	if err != nil {
+		t.Fatalf("failed to update payment: %v", err)
+	}
+
+	handler := RefundPayment
+	// Two distinct £20 partial refunds of the same £50 payment, NO client
+	// idempotency key on either — the exact case that collided on the
+	// amount-derived default key before the fix.
+	refund1 := RefundRequest{Amount: 2000, Reason: "partial one"}
+	w1 := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", refund1, adminToken, ctx)
+	if w1.Code != http.StatusOK {
+		t.Fatalf("first refund: expected 200, got %d. body: %s", w1.Code, w1.Body.String())
+	}
+
+	refund2 := RefundRequest{Amount: 2000, Reason: "partial two"}
+	w2 := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", refund2, adminToken, ctx)
+	if w2.Code != http.StatusOK {
+		t.Fatalf("second equal partial refund: expected 200, got %d. body: %s", w2.Code, w2.Body.String())
+	}
+
+	// Both refunds must exist as separate completed rows with DIFFERENT
+	// fallback keys — the second was NOT swallowed by the first's dedup lookup.
+	var completedCount int
+	err = tx.QueryRow(ctx, `SELECT COUNT(*) FROM refunds WHERE payment_id = $1 AND status = 'completed'`, paymentID).Scan(&completedCount)
+	if err != nil {
+		t.Fatalf("failed to query refunds: %v", err)
+	}
+	if completedCount != 2 {
+		t.Errorf("expected 2 completed refund rows, got %d (second was swallowed!)", completedCount)
+	}
+
+	var distinctKeys int
+	err = tx.QueryRow(ctx, `SELECT COUNT(DISTINCT idempotency_key) FROM refunds WHERE payment_id = $1`, paymentID).Scan(&distinctKeys)
+	if err != nil {
+		t.Fatalf("failed to count distinct refund keys: %v", err)
+	}
+	if distinctKeys != 2 {
+		t.Errorf("expected 2 DISTINCT fallback keys for the two same-amount refunds, got %d", distinctKeys)
+	}
+}
+
 // TestRefund_TwoEqualPartialRefunds_ClientKeyDisambiguates verifies the P2 fix:
 // two DISTINCT partial refunds of the same amount against the same payment
 // must both complete. With only the amount-derived key (paymentID-amount) the
@@ -1012,8 +1085,12 @@ func TestRefund_PendingSameKeyRetry_Resumes(t *testing.T) {
 		t.Fatalf("failed to update payment: %v", err)
 	}
 
-	// Seed a pending refund with the same key the handler will compute.
-	key := paymentID + "-refund-2500"
+	// Seed a pending refund with the key the handler derives from the client
+	// key below (sha256-hashed + truncated to 24 hex chars). The retry sends
+	// the SAME client key, so the exact-key dedup resumes the row.
+	clientKey := "refund-pending-resume-uuid"
+	ikHash := sha256.Sum256([]byte(clientKey))
+	key := paymentID + "-refund-" + fmt.Sprintf("%x", ikHash)[:24]
 	_, err = tx.Exec(ctx, `
 		INSERT INTO refunds (payment_id, booking_id, amount, status, reason, idempotency_key, created_at)
 		VALUES ($1, $2, 25, 'pending', 'customer request', $3, NOW())
@@ -1023,8 +1100,9 @@ func TestRefund_PendingSameKeyRetry_Resumes(t *testing.T) {
 	}
 
 	req := RefundRequest{
-		Amount: 2500,
-		Reason: "customer request",
+		Amount:         2500,
+		Reason:         "customer request",
+		IdempotencyKey: clientKey,
 	}
 
 	handler := RefundPayment
@@ -2069,6 +2147,109 @@ func TestBuildSplitRecords_OverflowBeyondTotal_BecomesTip(t *testing.T) {
 	}
 }
 
+// TestBuildSplitRecords_DiscountBooking_TipOverflow_SumNeverExceedsCharge
+// proves the money invariant documented on buildSplitRecords: the split
+// records (deposit + balance + tip) ALWAYS partition the charged amount
+// exactly, so their sum can never exceed what Square actually charged — even
+// when a discount is present. A discount reduces what the customer owes but is
+// booked as a SEPARATE ledger row (payment_method='discount') that
+// GetBookingPaymentInfo excludes from TotalPaid, so buildSplitRecords runs
+// against the full booking total and never inflates the split sum past the
+// charged pence.
+func TestBuildSplitRecords_DiscountBooking_TipOverflow_SumNeverExceedsCharge(t *testing.T) {
+	// Discounted booking: £100 total with a £10 discount (the discount row
+	// exists in the ledger but is excluded from TotalPaid). Tip overflow
+	// charges over and above the booking total.
+	record := makeTestRecord("b-disc-tip", "full", 120)
+	info := &BookingPaymentInfo{
+		StartTime:   clock.Now().Add(48 * time.Hour),
+		TotalAmount: 100,
+		TotalPaid:   0, // the £10 discount payment row is excluded here
+	}
+	records := buildSplitRecords(record, "full", info, 120)
+
+	var sum float64
+	for _, r := range records {
+		sum += r.Amount
+	}
+	// Partition invariant: the sum equals the charged amount exactly — it can
+	// never exceed it, no matter how the tip overflows.
+	if sum > 120 {
+		t.Errorf("split records sum to %.2f, exceeding the charged amount £120", sum)
+	}
+	if math.Abs(sum-120) > 0.005 {
+		t.Errorf("expected split records to partition the charged £120 exactly, got sum %.2f (records: %+v)", sum, records)
+	}
+
+	// Same invariant with real money already paid toward the booking: the
+	// deposit room is consumed, but the sum still partitions the new charge.
+	record2 := makeTestRecord("b-disc-tip-2", "full", 70)
+	info2 := &BookingPaymentInfo{
+		StartTime:   clock.Now().Add(48 * time.Hour),
+		TotalAmount: 100,
+		TotalPaid:   60,
+	}
+	records2 := buildSplitRecords(record2, "full", info2, 70)
+	var sum2 float64
+	for _, r := range records2 {
+		sum2 += r.Amount
+	}
+	if sum2 > 70 {
+		t.Errorf("split records sum to %.2f, exceeding the charged amount £70", sum2)
+	}
+	if math.Abs(sum2-70) > 0.005 {
+		t.Errorf("expected split records to partition the charged £70 exactly, got sum %.2f (records: %+v)", sum2, records2)
+	}
+}
+
+// TestGetBookingPaymentInfo_ExcludesDiscountPayments proves the database half
+// of the invariant: GetBookingPaymentInfo.TotalPaid excludes discount payment
+// rows, so a discounted booking never inflates TotalPaid (which would shrink
+// the tip carve-out / grow the balance allocation into an over-recorded total).
+func TestGetBookingPaymentInfo_ExcludesDiscountPayments(t *testing.T) {
+	t.Parallel()
+	ctx, tx := testutils.SetupTestTx(t)
+
+	userID, err := fixtures.CreateTestUser(tx)
+	require.NoError(t, err)
+	serviceID, err := fixtures.CreateTestService(tx)
+	require.NoError(t, err)
+	bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC))
+	require.NoError(t, err)
+
+	// £60 of real money plus a £10 discount payment row (payment_method='discount').
+	_, err = fixtures.CreateTestPayment(tx, bookingID, 60.00, "online_square", "full", "completed")
+	require.NoError(t, err)
+	_, err = tx.Exec(ctx, `
+		INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, created_by)
+		VALUES ($1, 'partial', 'discount', 10.00, 'completed', $2)
+	`, bookingID, userID)
+	require.NoError(t, err)
+
+	svc := NewPaymentService()
+	info, err := svc.GetBookingPaymentInfo(ctx, bookingID)
+	require.NoError(t, err)
+
+	if info.TotalPaid != 60.00 {
+		t.Errorf("expected TotalPaid to exclude the £10 discount row, got %.2f", info.TotalPaid)
+	}
+
+	// A £70 tip-overflow charge on top must partition exactly into £70 of split
+	// records — the discount can never push the recorded sum past the charge.
+	record := makeTestRecord(bookingID, "full", 70)
+	records := buildSplitRecords(record, "full", info, 70)
+	var sum float64
+	for _, r := range records {
+		sum += r.Amount
+	}
+	if sum > 70 {
+		t.Errorf("split records sum to %.2f, exceeding the charged amount £70", sum)
+	}
+	if math.Abs(sum-70) > 0.005 {
+		t.Errorf("expected split records to partition the charged £70 exactly, got sum %.2f", sum)
+	}
+}
+
 // ---------------------------------------------------------------------------
 // Handler-level atomicity — verify the full handler succeeds with split.
 // ---------------------------------------------------------------------------

backend/handlers/payments/refunds_test.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -2645,6 +2646,103 @@ func TestSweepPendingSquareRefunds_RetriesStaleManualRefund(t *testing.T) {
 	}
 }
 
+// =============================================================================
+// D2b — legacy pending refund rows with NULL idempotency_key resume with a key
+// =============================================================================
+
+// TestResumePendingRefund_LegacyNullKey_DerivesFreshKey verifies that resuming
+// a pending refund whose row has a NULL idempotency_key (a legacy row created
+// before keyed refunds) re-issues the Square refund with a NON-EMPTY derived
+// key. Square's RefundPayment REQUIRES a non-empty idempotency key — re-issuing
+// with "" would return a 400 INVALID_REQUEST_ERROR that classifies as ambiguous
+// and leaves the refund pending forever.
+func TestResumePendingRefund_LegacyNullKey_DerivesFreshKey(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+
+	userID, err := fixtures.CreateTestUser(tx)
+	if err != nil {
+		t.Fatalf("failed to create user: %v", err)
+	}
+	adminID, err := fixtures.CreateTestAdminUser(tx)
+	if err != nil {
+		t.Fatalf("failed to create admin user: %v", err)
+	}
+	serviceID, err := fixtures.CreateTestService(tx)
+	if err != nil {
+		t.Fatalf("failed to create service: %v", err)
+	}
+	bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID,
+		time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC))
+	if err != nil {
+		t.Fatalf("failed to create booking: %v", err)
+	}
+
+	paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 50.00, "online_square", "full", "completed")
+	if err != nil {
+		t.Fatalf("failed to create card payment: %v", err)
+	}
+	_, err = tx.Exec(ctx, "UPDATE payments SET square_payment_id = 'sqp_legacy_null_key' WHERE id = $1", paymentID)
+	if err != nil {
+		t.Fatalf("failed to set square_payment_id: %v", err)
+	}
+
+	// Seed a pending refund row with NO idempotency_key column value → NULL
+	// (legacy row). The (payment_id, amount) pending fallback finds it.
+	var refundID string
+	err = tx.QueryRow(ctx, `
+		INSERT INTO refunds (payment_id, booking_id, amount, status, reason, origin, created_at)
+		VALUES ($1, $2, 50, 'pending', 'customer request', 'manual', NOW())
+		RETURNING id
+	`, paymentID, bookingID).Scan(&refundID)
+	if err != nil {
+		t.Fatalf("failed to insert legacy pending refund with NULL idempotency_key: %v", err)
+	}
+
+	origClient := SquareClient
+	counting := &countingRefundClient{SquareClient: square.NewDevClient()}
+	SquareClient = counting
+	defer func() { SquareClient = origClient }()
+
+	// A NEW client-supplied key makes the exact-key dedup miss, so the pending
+	// (payment_id, amount) fallback resumes the legacy NULL-key row.
+	adminToken := jwt.GenerateTestToken(adminID, "admin")
+	req := RefundRequest{Amount: 5000, Reason: "customer request", IdempotencyKey: "fresh-client-key-legacy"}
+	rec := makePaymentRequest(RefundPayment, "POST", "/api/admin/payments/"+paymentID+"/refund", req, adminToken, ctx)
+
+	if rec.Code != http.StatusOK {
+		t.Fatalf("expected 200 on resume, got %d: %s", rec.Code, rec.Body.String())
+	}
+
+	calls := counting.refundCalls()
+	if len(calls) != 1 {
+		t.Fatalf("expected exactly 1 Square refund call for the resumed legacy refund, got %d", len(calls))
+	}
+	if calls[0].IdempotencyKey == "" {
+		t.Fatal("expected a NON-EMPTY idempotency key for the resumed legacy refund (Square RefundPayment requires one)")
+	}
+	if calls[0].IdempotencyKey == req.IdempotencyKey {
+		t.Errorf("expected a derived key, not the client's fresh key, got %q", calls[0].IdempotencyKey)
+	}
+	// Derived shape: <paymentID>-refund-<pence>-<12 hex chars>.
+	prefix := paymentID + "-refund-5000-"
+	if !strings.HasPrefix(calls[0].IdempotencyKey, prefix) {
+		t.Errorf("expected derived key with prefix %q, got %q", prefix, calls[0].IdempotencyKey)
+	}
+	if len(calls[0].IdempotencyKey) > 45 {
+		t.Errorf("expected derived key within Square's 45-char limit, got %d chars: %q", len(calls[0].IdempotencyKey), calls[0].IdempotencyKey)
+	}
+
+	// The row must resolve to completed (the mock returns COMPLETED).
+	var status string
+	err = db.Conn.QueryRow(ctx, `SELECT status FROM refunds WHERE id = $1`, refundID).Scan(&status)
+	if err != nil {
+		t.Fatalf("failed to query refund status: %v", err)
+	}
+	if status != "completed" {
+		t.Errorf("expected resumed refund status 'completed', got %q", status)
+	}
+}
+
 // =============================================================================
 // D3 — the 23h age guard reconciles against Square before marking failed
 // =============================================================================