Fix silent tip/gift-card money loss on pending retry; terminal checkout wire; sentinel error; docs

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)

backend/handlers/payments/giftcards.go

@@ -891,16 +891,27 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
 
 	paymentService := NewPaymentService()
 
+	// Idempotency: only short-circuit when the existing record is 'completed'.
+	// A 'pending' record means the previous Square call failed — returning it
+	// as 200 would show a success without ever charging. Re-attempt below with
+	// the same key (Square dedups safely) and reuse the pending record.
+	reusePendingID := ""
 	if req.IdempotencyKey != "" {
 		existing, err := paymentService.CheckIdempotencyByKey(ctx, req.IdempotencyKey)
 		if err != nil {
 			log.Printf("Failed to check idempotency: %v", err)
 		}
 		if existing != nil {
-			if err := json.NewEncoder(w).Encode(existing); err != nil {
-				log.Printf("Failed to encode JSON response: %v", err)
+			if existing.Status == "completed" {
+				if err := json.NewEncoder(w).Encode(existing); err != nil {
+					log.Printf("Failed to encode JSON response: %v", err)
+				}
+				return
+			}
+			if existing.Status == "pending" {
+				reusePendingID = existing.ID
+				log.Printf("[PAYMENTS] Reusing pending payment %s for idempotent gift-card retry (key %s)", existing.ID, req.IdempotencyKey)
 			}
-			return
 		}
 	}
 
@@ -958,43 +969,62 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
 	}()
 
 	var buyPaymentID string
-	fees := paymentService.CalculateFees(req.Amount, "online")
-	record := PaymentRecord{
-		PaymentType:     "full",
-		PaymentMethod:   "online_square",
-		Status:          "pending",
-		Amount:          amountPounds,
-		SquarePaymentID: nil,
-		IdempotencyKey:  &req.IdempotencyKey,
-		Fees:            float64(fees) / 100.0,
-		UserSavedCardID: savedCardID,
-		CreatedAt:       clock.Now(),
-		UpdatedAt:       clock.Now(),
-		CreatedBy:       &userID,
-	}
-	err = tx.QueryRow(ctx, `
-		INSERT INTO payments (payment_type, payment_method, status, amount, square_payment_id, idempotency_key, fees, user_saved_card_id, created_by, created_at, updated_at)
-		VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11)
-		RETURNING id
-	`, record.PaymentType, record.PaymentMethod, record.Status, record.Amount, record.SquarePaymentID, record.IdempotencyKey, record.Fees, record.UserSavedCardID, record.CreatedBy, record.CreatedAt, record.UpdatedAt).Scan(&buyPaymentID)
-	if err != nil {
-		log.Printf("Failed to insert payment record: %v", err)
-		http.Error(w, "internal server error", http.StatusInternalServerError)
-		return
-	}
+	if reusePendingID != "" {
+		// Reusing the pending record from a failed prior attempt — do not
+		// insert a duplicate (idempotency_key is UNIQUE). Proceed straight to
+		// the Square call, which dedups on the same key.
+		buyPaymentID = reusePendingID
+	} else {
+		fees := paymentService.CalculateFees(req.Amount, "online")
+		record := PaymentRecord{
+			PaymentType:     "full",
+			PaymentMethod:   "online_square",
+			Status:          "pending",
+			Amount:          amountPounds,
+			SquarePaymentID: nil,
+			IdempotencyKey:  &req.IdempotencyKey,
+			Fees:            float64(fees) / 100.0,
+			UserSavedCardID: savedCardID,
+			CreatedAt:       clock.Now(),
+			UpdatedAt:       clock.Now(),
+			CreatedBy:       &userID,
+		}
+		err = tx.QueryRow(ctx, `
+			INSERT INTO payments (payment_type, payment_method, status, amount, square_payment_id, idempotency_key, fees, user_saved_card_id, created_by, created_at, updated_at)
+			VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11)
+			RETURNING id
+		`, record.PaymentType, record.PaymentMethod, record.Status, record.Amount, record.SquarePaymentID, record.IdempotencyKey, record.Fees, record.UserSavedCardID, record.CreatedBy, record.CreatedAt, record.UpdatedAt).Scan(&buyPaymentID)
+		if err != nil {
+			log.Printf("Failed to insert payment record: %v", err)
+			http.Error(w, "internal server error", http.StatusInternalServerError)
+			return
+		}
 
-	// Apply VAT to the pending payment
-	vatCfg, vatErr := GetVATConfig(ctx, tx)
-	if vatErr == nil && vatCfg.IsVATRegistered && vatCfg.VoucherType == "SPV" {
-		if _, vatExecErr := tx.Exec(ctx, "SELECT apply_vat_to_payment($1, $2)", buyPaymentID, vatCfg.DefaultVATRate); vatExecErr != nil {
-			log.Printf("Failed to apply VAT to buy gift card payment %s: %v", buyPaymentID, vatExecErr)
+		// Apply VAT to the pending payment
+		vatCfg, vatErr := GetVATConfig(ctx, tx)
+		if vatErr == nil && vatCfg.IsVATRegistered && vatCfg.VoucherType == "SPV" {
+			if _, vatExecErr := tx.Exec(ctx, "SELECT apply_vat_to_payment($1, $2)", buyPaymentID, vatCfg.DefaultVATRate); vatExecErr != nil {
+				log.Printf("Failed to apply VAT to buy gift card payment %s: %v", buyPaymentID, vatExecErr)
+			}
+		}
+
+		if err := tx.Commit(ctx); err != nil {
+			log.Printf("Failed to commit buy transaction: %v", err)
+			http.Error(w, "internal server error", http.StatusInternalServerError)
+			return
 		}
 	}
 
-	if err := tx.Commit(ctx); err != nil {
-		log.Printf("Failed to commit buy transaction: %v", err)
-		http.Error(w, "internal server error", http.StatusInternalServerError)
-		return
+	// Commit the (nested/savepoint) transaction in the reuse path too. It is
+	// empty, but committing releases the savepoint so the deferred rollback
+	// becomes a no-op — otherwise that rollback would undo the status UPDATE
+	// executed later on the same connection.
+	if reusePendingID != "" {
+		if err := tx.Commit(ctx); err != nil {
+			log.Printf("Failed to commit buy transaction (reuse): %v", err)
+			http.Error(w, "internal server error", http.StatusInternalServerError)
+			return
+		}
 	}
 
 	// Step 2: DB transaction committed — safe to call Square now.

backend/handlers/payments/handlers.go

@@ -626,7 +626,7 @@ func GetCheckoutStatus(w http.ResponseWriter, r *http.Request) {
 
 	paymentResult, err := SquareClient.GetCheckout(r.Context(), checkoutID)
 	if err != nil {
-		if err.Error() == "checkout pending" {
+		if errors.Is(err, square.ErrCheckoutPending) {
 			if err := json.NewEncoder(w).Encode(PaymentStatusResponse{Status: "PENDING"}); err != nil {
 				log.Printf("Failed to encode JSON response: %v", err)
 			}
@@ -1900,18 +1900,30 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) {
 		}
 	}()
 
-	// Check idempotency inside the transaction — same pattern as CreateBookingPayment.
+	// Check idempotency inside the transaction.
+	// Only short-circuit when the existing record is 'completed'. A 'pending'
+	// record means the previous Square call failed — returning it as 200 would
+	// show a success toast without ever charging. Re-attempt the charge below
+	// with the same idempotency key (Square dedups safely) and reuse the
+	// existing record.
 	var existingID sql.NullString
 	var existingBookingID sql.NullString
 	var existingPaymentType sql.NullString
 	var existingStatus sql.NullString
 	var existingAmount sql.NullFloat64
 	var existingCreatedAt sql.NullTime
-	if err := tx.QueryRow(r.Context(), `
+
+	err = tx.QueryRow(r.Context(), `
 		SELECT id, booking_id, payment_type, status, amount, created_at
 		FROM payments
 		WHERE booking_id = $1 AND idempotency_key = $2
-	`, bookingID, idempotencyKey).Scan(&existingID, &existingBookingID, &existingPaymentType, &existingStatus, &existingAmount, &existingCreatedAt); err == nil {
+	`, bookingID, idempotencyKey).Scan(&existingID, &existingBookingID, &existingPaymentType, &existingStatus, &existingAmount, &existingCreatedAt)
+
+	paymentID := ""
+	reusePendingRecord := false
+	switch {
+	case err == nil && existingStatus.String == "completed":
+		// Idempotent dedup — return the already-completed payment.
 		if err := json.NewEncoder(w).Encode(PaymentResponse{
 			ID:          existingID.String,
 			BookingID:   existingBookingID.String,
@@ -1923,32 +1935,42 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) {
 			log.Printf("Failed to encode JSON response: %v", err)
 		}
 		return
-	} else if !errors.Is(err, pgx.ErrNoRows) {
+	case err == nil && existingStatus.String == "pending":
+		// Previous Square call failed — reuse the pending record and re-attempt.
+		paymentID = existingID.String
+		reusePendingRecord = true
+	case err != nil && !errors.Is(err, pgx.ErrNoRows):
 		log.Printf("Failed to check tip idempotency: %v", err)
 	}
 
-	record := PaymentRecord{
-		BookingID:      bookingID,
-		PaymentType:    "tip",
-		PaymentMethod:  "online_square",
-		Status:         "pending",
-		Amount:         float64(req.Amount) / 100.0,
-		IdempotencyKey: &idempotencyKey,
-		Fees:           0,
-		UserSavedCardID: savedCardID,
-		CreatedAt:      clock.Now(),
-		UpdatedAt:      clock.Now(),
-		CreatedBy:      &userID,
+	if !reusePendingRecord {
+		record := PaymentRecord{
+			BookingID:      bookingID,
+			PaymentType:    "tip",
+			PaymentMethod:  "online_square",
+			Status:         "pending",
+			Amount:         float64(req.Amount) / 100.0,
+			IdempotencyKey: &idempotencyKey,
+			Fees:           0,
+			UserSavedCardID: savedCardID,
+			CreatedAt:      clock.Now(),
+			UpdatedAt:      clock.Now(),
+			CreatedBy:      &userID,
+		}
+
+		paymentID, err = service.CreatePaymentRecordTx(r.Context(), tx, record, nil)
+		if err != nil {
+			log.Printf("Failed to create payment record: %v", err)
+			http.Error(w, "internal server error", http.StatusInternalServerError)
+			return
+		}
+		ApplyVATToBookingPayment(r.Context(), tx, paymentID)
 	}
 
-	paymentID, err := service.CreatePaymentRecordTx(r.Context(), tx, record, nil)
-	if err != nil {
-		log.Printf("Failed to create payment record: %v", err)
-		http.Error(w, "internal server error", http.StatusInternalServerError)
-		return
-	}
-	ApplyVATToBookingPayment(r.Context(), tx, paymentID)
-
+	// Always commit the (possibly nested/savepoint) transaction. In the reuse
+	// path the savepoint is empty, but committing it releases the savepoint so
+	// the deferred rollback becomes a no-op — otherwise that rollback would
+	// undo the status UPDATE executed later on the same connection.
 	if err := tx.Commit(r.Context()); err != nil {
 		log.Printf("Failed to commit transaction: %v", err)
 		http.Error(w, "internal server error", http.StatusInternalServerError)

backend/handlers/payments/payments_test.go

@@ -1933,6 +1933,62 @@ func TestTipPayment_WithSavedCard(t *testing.T) {
 	assert.NotEmpty(t, resp.CardLast4)
 }
 
+func TestTipPayment_RetryPending_ReattemptsCharge(t *testing.T) {
+	t.Parallel()
+	ctx, tx := testutils.SetupTestTx(t)
+
+	userID, bookingID, _ := setupTestData(t, ctx, tx)
+	userToken := jwt.GenerateUserToken(userID)
+
+	// Create a completed payment so the tip is allowed.
+	payID, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "online_square", "full", "completed")
+	require.NoError(t, err)
+	squarePayID := "sqp_test_retry"
+	_, err = tx.Exec(ctx, "UPDATE payments SET square_payment_id = $1 WHERE id = $2", squarePayID, payID)
+	require.NoError(t, err)
+
+	// Create a saved card for this user.
+	var savedCardID string
+	err = tx.QueryRow(ctx, `
+		INSERT INTO user_saved_cards (user_id, square_card_id, brand, last_4, exp_month, exp_year, fingerprint)
+		VALUES ($1, 'ccof_mock_retry', 'VISA', '1111', 12, 2030, 'sqfp_mock_retry')
+		RETURNING id
+	`, userID).Scan(&savedCardID)
+	require.NoError(t, err)
+
+	// Simulate a failed prior attempt: a PENDING tip record with the same
+	// idempotency key the frontend will send on retry. The handler must NOT
+	// short-circuit on this — it must re-attempt the Square charge.
+	key := "tip-retry-key-123"
+	_, err = tx.Exec(ctx, `
+		INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, idempotency_key, created_at, updated_at, created_by)
+		VALUES ($1, 'tip', 'online_square', 'pending', 10.00, $2, NOW(), NOW(), $3)
+	`, bookingID, key, userID)
+	require.NoError(t, err)
+
+	req := CreateTipPaymentRequest{
+		Amount:         1000,
+		CardID:         &savedCardID,
+		IdempotencyKey: key,
+	}
+
+	handler := CreateTipPayment
+	w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx)
+	require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String())
+
+	var resp PaymentResponse
+	require.NoError(t, parsePaymentResponseBody(w, &resp))
+	assert.Equal(t, "completed", resp.Status, "retry of a pending record must re-attempt and complete, not return the stale pending record")
+
+	// Exactly one payment record for this key, now completed.
+	var count int
+	var status string
+	err = tx.QueryRow(ctx, `SELECT COUNT(*), MAX(status) FROM payments WHERE idempotency_key = $1`, key).Scan(&count, &status)
+	require.NoError(t, err)
+	assert.Equal(t, 1, count, "must reuse the pending record, not insert a duplicate")
+	assert.Equal(t, "completed", status)
+}
+
 func TestTipPayment_TransactionFailure_SkipsSquare(t *testing.T) {
 	t.Parallel()
 	ctx, tx := testutils.SetupTestTx(t)

backend/handlers/payments/till.go

@@ -522,7 +522,7 @@ func GetTillCheckoutStatus(w http.ResponseWriter, r *http.Request) {
 
 	paymentResult, err := SquareClient.GetCheckout(r.Context(), checkoutID)
 	if err != nil {
-		if err.Error() == "checkout pending" {
+		if errors.Is(err, square.ErrCheckoutPending) {
 			if err := json.NewEncoder(w).Encode(PaymentStatusResponse{Status: "PENDING"}); err != nil {
 				log.Printf("Failed to encode JSON response: %v", err)
 			}

backend/internal/square/square_dev.go

@@ -261,7 +261,7 @@ func (m *MockClient) GetCheckout(ctx context.Context, checkoutID string) (*Payme
 	}
 
 	if checkout.Status == "PENDING" {
-		return nil, fmt.Errorf("checkout pending")
+		return nil, ErrCheckoutPending
 	}
 
 	result, ok := m.completed[checkoutID]

backend/internal/square/square_http_client.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"crypto/sha256"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -13,6 +14,12 @@ import (
 	"time"
 )
 
+// ErrCheckoutPending is returned by GetCheckout when a terminal checkout has
+// not yet completed. Handlers use errors.Is(err, ErrCheckoutPending) rather
+// than string comparison, so behaviour is identical across the mock and the
+// real HTTP client.
+var ErrCheckoutPending = errors.New("checkout pending")
+
 // ---------------------------------------------------------------------------
 // Square REST API constants.
 // ---------------------------------------------------------------------------
@@ -175,14 +182,18 @@ type sqFee struct {
 type sqTerminalCheckoutRequest struct {
 	IdempotencyKey string                    `json:"idempotency_key"`
 	Checkout       sqTerminalCheckoutPayload `json:"checkout"`
-	DeviceID       string                    `json:"device_id,omitempty"`
 }
 
 type sqTerminalCheckoutPayload struct {
-	AmountMoney sqMoney `json:"amount_money"`
-	ReferenceID string  `json:"reference_id,omitempty"`
-	Note        string  `json:"note,omitempty"`
-	CustomerID  string  `json:"customer_id,omitempty"`
+	AmountMoney  sqMoney                 `json:"amount_money"`
+	ReferenceID  string                  `json:"reference_id,omitempty"`
+	Note         string                  `json:"note,omitempty"`
+	CustomerID   string                  `json:"customer_id,omitempty"`
+	DeviceOptions *sqDeviceOptions       `json:"device_options,omitempty"`
+}
+
+type sqDeviceOptions struct {
+	DeviceID string `json:"device_id"`
 }
 
 type sqTerminalCheckoutResponse struct {
@@ -238,8 +249,8 @@ type sqCreateCardRequest struct {
 }
 
 type sqCardPayload struct {
-	ExpMonth       int    `json:"exp_month"`
-	ExpYear        int    `json:"exp_year"`
+	ExpMonth       *int   `json:"exp_month,omitempty"`
+	ExpYear        *int   `json:"exp_year,omitempty"`
 	CardholderName string `json:"cardholder_name,omitempty"`
 	CustomerID     string `json:"customer_id,omitempty"`
 }
@@ -295,7 +306,9 @@ func createCheckoutHTTP(ctx context.Context, req CreateCheckoutReq) (*CheckoutRe
 			Note:        req.Note,
 			CustomerID:  req.CustomerID,
 		},
-		DeviceID: req.DeviceID,
+	}
+	if req.DeviceID != "" {
+		body.Checkout.DeviceOptions = &sqDeviceOptions{DeviceID: req.DeviceID}
 	}
 	var resp sqTerminalCheckoutResponse
 	if err := hc.doJSON(ctx, http.MethodPost, "/v2/terminals/checkouts", body, &resp); err != nil {
@@ -312,6 +325,9 @@ func getCheckoutHTTP(ctx context.Context, checkoutID string) (*PaymentResult, er
 	}
 	tc := tcResp.Checkout
 	if tc.Status != "COMPLETED" {
+		if tc.Status == "PENDING" || tc.Status == "IN_PROGRESS" {
+			return nil, ErrCheckoutPending
+		}
 		return nil, fmt.Errorf("square: checkout %s is %s (not COMPLETED)", checkoutID, tc.Status)
 	}
 	if len(tc.PaymentIDs) == 0 {
@@ -351,8 +367,6 @@ func createCardOnFileHTTP(ctx context.Context, userID, cardToken string) (*CardO
 		SourceID:       cardToken,
 		Card: sqCardPayload{
 			CustomerID: userID,
-			ExpMonth:   0,
-			ExpYear:    0,
 		},
 	}
 	var resp sqCreateCardResponse