fix(purchase): gate sweep/SQS execution on approval+AutoPurchase, fail-loud on delay-config error, safe reaper retry (adversarial-review follow-ups)

internal/api/handler_purchases.go

@@ -612,8 +612,13 @@ func (h *Handler) approveViaToken(ctx context.Context, req *events.LambdaFunctio
 	// Check for Gmail-style pre-fire delay (issue #291 wave-2).
 	// Token/email-link path: no authenticated session UUID is available, so the
 	// scheduled transition is recorded as system-initiated (transitioned_by = NULL).
+	// Fail closed: a config-read error must NOT silently discard the configured
+	// free-cancel window and execute immediately. Return 500 so the caller can retry.
 	globalCfg, cfgErr := h.config.GetGlobalConfig(ctx)
-	if cfgErr == nil && globalCfg.GetPurchaseDelay() > 0 {
+	if cfgErr != nil {
+		return nil, fmt.Errorf("failed to read global config for purchase delay check: %w", cfgErr)
+	}
+	if globalCfg.GetPurchaseDelay() > 0 {
 		return h.approveWithDelay(ctx, execution, globalCfg.GetPurchaseDelay(), actor, nil)
 	}
 	// ApproveExecution now runs the purchase synchronously inside the
@@ -699,8 +704,13 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam
 	// Check for Gmail-style pre-fire delay (issue #291 wave-2). When
 	// PurchaseDelayHours > 0 the SDK call is deferred; the user gets a
 	// "scheduled, revoke before X" email and a window to cancel at $0.
+	// Fail closed: a config-read error must NOT silently discard the configured
+	// free-cancel window and execute immediately. Return 500 so the caller can retry.
 	globalCfg, cfgErr := h.config.GetGlobalConfig(ctx)
-	if cfgErr == nil && globalCfg.GetPurchaseDelay() > 0 {
+	if cfgErr != nil {
+		return nil, fmt.Errorf("failed to read global config for purchase delay check: %w", cfgErr)
+	}
+	if globalCfg.GetPurchaseDelay() > 0 {
 		return h.approveWithDelay(ctx, execution, globalCfg.GetPurchaseDelay(), session.Email, actor)
 	}
 

internal/api/handler_purchases_test.go

@@ -372,6 +372,95 @@ func TestHandler_approvePurchase_SessionExecuteFailureSurfacesAs409(t *testing.T
 	assert.Contains(t, ce.Error(), "could not be approved")
 }
 
+// --- F3 regression: global-config read error must fail closed (not execute) ---
+
+// TestHandler_approveViaToken_GlobalConfigError_FailsClosed pins the F3 fix:
+// when GetGlobalConfig returns an error during the email-link (token) approve
+// path, the handler must return an error rather than silently executing the
+// purchase (which would discard the configured free-cancel window). Pre-fix,
+// both approve paths used "if cfgErr == nil && delay > 0 { delay }" and fell
+// through to execute on any transient config error.
+func TestHandler_approveViaToken_GlobalConfigError_FailsClosed(t *testing.T) {
+	ctx := context.Background()
+	execID := "f3f3f3f3-f3f3-f3f3-f3f3-f3f3f3f3f301"
+	contactEmail := "approver@example.com"
+
+	mockConfig := new(MockConfigStore)
+	exec := approvalTestExec(execID, contactEmail, mockConfig)
+	mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil)
+	// GetGlobalConfig fails on every call. authorizeApprovalAction consumes the
+	// first call best-effort (error silently ignored, globalNotify stays "");
+	// approveViaToken must fail closed on its own call rather than executing
+	// the purchase without the configured delay. Matching all calls is correct
+	// because the first (best-effort) call also fails — contact_email on the
+	// per-account record still matches the approver, so authorizeApprovalAction
+	// succeeds despite the config failure.
+	mockConfig.On("GetGlobalConfig", ctx).Return(nil, errors.New("db transient error"))
+
+	mockAuth := new(MockAuthService)
+	mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil)
+	// No approve-any / approve-own permissions — dispatch falls to token path.
+	mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe()
+	mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe()
+
+	mockPurchase := new(MockPurchaseManager)
+	// Neither approve path must be reached.
+
+	handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth}
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"authorization": "Bearer sess-tok"},
+	}
+
+	_, err := handler.approvePurchase(ctx, req, execID, "valid-token")
+	require.Error(t, err, "config error must propagate; must not execute immediately (F3 token path)")
+	assert.Contains(t, err.Error(), "failed to read global config")
+	mockPurchase.AssertNotCalled(t, "ApproveExecution",
+		mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+	mockPurchase.AssertNotCalled(t, "ApproveAndExecute",
+		mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+}
+
+// TestHandler_approvePurchaseViaSession_GlobalConfigError_FailsClosed pins the
+// F3 fix for the session (dashboard) approve path: same contract as the token
+// path — a transient config error must not discard the free-cancel window.
+func TestHandler_approvePurchaseViaSession_GlobalConfigError_FailsClosed(t *testing.T) {
+	ctx := context.Background()
+	execID := "f3f3f3f3-f3f3-f3f3-f3f3-f3f3f3f3f302"
+	adminEmail := "admin@example.com"
+
+	mockConfig := new(MockConfigStore)
+	exec := &config.PurchaseExecution{
+		ExecutionID:     execID,
+		ApprovalToken:   "valid-token",
+		Status:          "pending",
+		Recommendations: []config.RecommendationRecord{{ID: "r1"}},
+	}
+	mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil)
+	// GetGlobalConfig always fails — approvePurchaseViaSession must return the
+	// error rather than executing the purchase without the configured delay.
+	mockConfig.On("GetGlobalConfig", ctx).Return(nil, errors.New("db transient error"))
+
+	mockAuth := new(MockAuthService)
+	mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil)
+	mockAuth.grantAdmin()
+	// approvePurchaseViaSession enforces CSRF.
+	mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil)
+
+	mockPurchase := new(MockPurchaseManager)
+	// ApproveAndExecute must not be called.
+
+	handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth}
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"authorization": "Bearer sess-tok"},
+	}
+
+	_, err := handler.approvePurchase(ctx, req, execID, "")
+	require.Error(t, err, "config error must propagate; must not execute immediately (F3 session path)")
+	assert.Contains(t, err.Error(), "failed to read global config")
+	mockPurchase.AssertNotCalled(t, "ApproveAndExecute",
+		mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+}
+
 // --- Regression tests for issue #609 (orphan-account guard) ---
 
 // TestHandler_approvePurchase_AzureOrphanRejects409 is the regression guard

internal/config/store_postgres.go

@@ -1391,7 +1391,7 @@ func (s *PostgresStore) GetExecutionByPlanAndDate(ctx context.Context, planID st
 	}
 
 	if len(executions) == 0 {
-		return nil, fmt.Errorf("execution not found for plan %s at %v", planID, scheduledDate)
+		return nil, fmt.Errorf("%w: plan %s at %v", ErrNotFound, planID, scheduledDate)
 	}
 
 	return &executions[0], nil

internal/config/store_postgres_pgxmock_test.go

@@ -2476,3 +2476,40 @@ func TestPGXMock_TransitionExecutionStatus_ProbeHardErrorNotMappedToNotFound(t *
 	assert.ErrorIs(t, err, dbErr)
 	assert.NoError(t, mock.ExpectationsWereMet())
 }
+
+// ─── F2 regression: GetExecutionByPlanAndDate zero-rows wraps ErrNotFound ────
+
+// TestPGXMock_GetExecutionByPlanAndDate_NotFoundWrapsErrNotFound is the
+// regression test for F2: a zero-row result from GetExecutionByPlanAndDate must
+// return an error wrapping config.ErrNotFound so that getOrCreateExecution can
+// distinguish "no existing execution" from a real store failure and create a new
+// one. Before the fix the function returned a plain fmt.Errorf, which caused
+// getOrCreateExecution to treat the not-found case as a fatal error, making the
+// create-execution branch unreachable against the real store.
+func TestPGXMock_GetExecutionByPlanAndDate_NotFoundWrapsErrNotFound(t *testing.T) {
+	mock := newMock(t)
+	store := storeWith(mock)
+	ctx := context.Background()
+
+	scheduledDate := time.Date(2025, 3, 1, 0, 0, 0, 0, time.UTC)
+
+	// Return an empty row set — simulates the "no existing execution" case.
+	cols := []string{
+		"plan_id", "execution_id", "status", "step_number", "scheduled_date",
+		"notification_sent", "approval_token", "recommendations",
+		"total_upfront_cost", "estimated_savings", "completed_at", "error", "expires_at",
+		"cloud_account_id", "source", "approved_by", "cancelled_by", "capacity_percent",
+		"created_by_user_id", "retry_execution_id", "retry_attempt_n",
+		"approval_token_expires_at",
+		"executed_by_user_id", "executed_at", "pre_approval_skip_reason",
+		"idempotency_key", "scheduled_execution_at",
+	}
+	emptyRows := pgxmock.NewRows(cols)
+	mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()).WillReturnRows(emptyRows)
+
+	_, err := store.GetExecutionByPlanAndDate(ctx, "plan-missing", scheduledDate)
+	require.Error(t, err)
+	assert.True(t, errors.Is(err, ErrNotFound),
+		"zero-row GetExecutionByPlanAndDate must wrap ErrNotFound so getOrCreateExecution can create a new execution; got: %v", err)
+	assert.NoError(t, mock.ExpectationsWereMet())
+}

internal/purchase/coverage_extra_test.go

@@ -265,8 +265,9 @@ func TestHandleExecutePurchase_SaveError(t *testing.T) {
 	mockSTS := new(MockSTSClient)
 
 	plan := &config.PurchasePlan{
-		ID:   "plan-save-err",
-		Name: "Plan",
+		ID:           "plan-save-err",
+		Name:         "Plan",
+		AutoPurchase: true,
 	}
 
 	rec := config.RecommendationRecord{

internal/purchase/manager.go

@@ -489,6 +489,84 @@ func (m *Manager) RecoverStrandedApprovals(ctx context.Context) (int, error) {
 	return recovered, nil
 }
 
+// executableByScheduler reports whether a pending or notified execution may be
+// auto-executed by the cron sweep or an SQS execute_purchase message, without
+// an explicit human approval action (fail closed on money paths).
+//
+// Rules:
+//   - source="web" rows must wait for the token-link approval path; the
+//     scheduler and SQS paths must never bypass that gate.
+//   - All other pending/notified rows require the owning plan to have
+//     AutoPurchase=true. A plan-fetch error is propagated so the caller can
+//     fail closed rather than defaulting to "execute".
+//
+// "approved" rows are handled by the session/token approval paths and
+// RecoverStrandedApprovals; this helper is only called for pending/notified.
+func (m *Manager) executableByScheduler(ctx context.Context, exec *config.PurchaseExecution) (bool, error) {
+	if exec.Source == "web" {
+		return false, nil
+	}
+	plan, err := m.config.GetPurchasePlan(ctx, exec.PlanID)
+	if err != nil {
+		return false, fmt.Errorf("failed to fetch plan %s for AutoPurchase gate: %w", exec.PlanID, err)
+	}
+	return plan.AutoPurchase, nil
+}
+
+// processOneExecution runs the full gate+claim+execute pipeline for a single
+// due pending/notified execution. It updates the Processed/Executed/Failed
+// counters and Errors slice on the supplied ProcessResult in place.
+// Extracted from ProcessScheduledPurchases to keep that function under the
+// gocyclo:10 threshold.
+func (m *Manager) processOneExecution(ctx context.Context, exec config.PurchaseExecution, result *ProcessResult) {
+	// AutoPurchase gate: pending/notified rows are only eligible for
+	// automatic execution when the owning plan has AutoPurchase=true AND
+	// the row was not web-submitted (those must go through the token-link
+	// path). Fail closed: a plan-fetch error counts as a failure rather
+	// than defaulting to "execute" (no silent money action on error).
+	eligible, gateErr := m.executableByScheduler(ctx, &exec)
+	if gateErr != nil {
+		result.Failed++
+		result.Errors = append(result.Errors, fmt.Sprintf("%s: AutoPurchase gate check failed: %v", exec.ExecutionID, gateErr))
+		return
+	}
+	if !eligible {
+		logging.Infof("Skipping execution %s (AutoPurchase=false or source=web; requires explicit approval)", exec.ExecutionID)
+		return
+	}
+
+	result.Processed++
+	logging.Infof("Executing scheduled purchase: %s", exec.ExecutionID)
+
+	// Atomically claim the row before executing (issue #1013). Overlapping
+	// cron ticks (a tick that runs longer than the interval, EventBridge
+	// duplicate/overlapping deliveries, or cron racing the SQS path) would
+	// otherwise both execute the same due row. claimAndExecute CASes the row
+	// to "running" and only the winner runs; a lost claim is skipped without
+	// re-executing.
+	claimed, execErr := m.claimAndExecute(ctx, &exec)
+	if !claimed {
+		// execErr != nil here is a real DB error during the claim (count as
+		// failed); execErr == nil is a benign CAS race-loss (skip silently).
+		if execErr != nil {
+			result.Failed++
+			result.Errors = append(result.Errors, fmt.Sprintf("%s: claim failed: %v", exec.ExecutionID, execErr))
+		}
+		return
+	}
+
+	// A multi-account run where at least one account committed is a success
+	// for ack purposes (issue #1014): the per-account rows own the truth and
+	// re-running would double-buy. Only a genuine failure (nothing
+	// committed) is counted/surfaced.
+	if isMultiAccountAckable(execErr) {
+		result.Executed++
+		return
+	}
+	result.Failed++
+	result.Errors = append(result.Errors, fmt.Sprintf("%s: %v", exec.ExecutionID, execErr))
+}
+
 // ProcessScheduledPurchases checks for and executes scheduled purchases.
 func (m *Manager) ProcessScheduledPurchases(ctx context.Context) (*ProcessResult, error) {
 	logging.Info("Processing scheduled purchases...")
@@ -509,10 +587,7 @@ func (m *Manager) ProcessScheduledPurchases(ctx context.Context) (*ProcessResult
 	}
 
 	now := time.Now()
-	processed := 0
-	executed := 0
-	failed := 0
-	var errs []string
+	result := &ProcessResult{Recovered: recovered}
 
 	for i := range executions {
 		exec := executions[i]
@@ -531,44 +606,8 @@ func (m *Manager) ProcessScheduledPurchases(ctx context.Context) (*ProcessResult
 			continue
 		}
 
-		processed++
-
-		logging.Infof("Executing scheduled purchase: %s", exec.ExecutionID)
-
-		// Atomically claim the row before executing (issue #1013). Overlapping
-		// cron ticks (a tick that runs longer than the interval, EventBridge
-		// duplicate/overlapping deliveries, or cron racing the SQS path) would
-		// otherwise both execute the same due row. claimAndExecute CASes the row
-		// to "running" and only the winner runs; a lost claim is skipped without
-		// re-executing.
-		claimed, execErr := m.claimAndExecute(ctx, &exec)
-		if !claimed {
-			// execErr != nil here is a real DB error during the claim (count as
-			// failed); execErr == nil is a benign CAS race-loss (skip silently).
-			if execErr != nil {
-				failed++
-				errs = append(errs, fmt.Sprintf("%s: claim failed: %v", exec.ExecutionID, execErr))
-			}
-			continue
-		}
-
-		// A multi-account run where at least one account committed is a success
-		// for ack purposes (issue #1014): the per-account rows own the truth and
-		// re-running would double-buy. Only a genuine failure (nothing
-		// committed) is counted/surfaced.
-		if isMultiAccountAckable(execErr) {
-			executed++
-			continue
-		}
-		failed++
-		errs = append(errs, fmt.Sprintf("%s: %v", exec.ExecutionID, execErr))
+		m.processOneExecution(ctx, exec, result)
 	}
 
-	return &ProcessResult{
-		Processed: processed,
-		Executed:  executed,
-		Failed:    failed,
-		Recovered: recovered,
-		Errors:    errs,
-	}, nil
+	return result, nil
 }