From 0270044bd33118966cd35789c49f91b419753b07 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 18:39:26 +0100 Subject: [PATCH 1/3] fix(purchase): route AutoPurchase steps to approval when 4-eyes is on With RequireDifferentApprover on, the scheduler's auto path (the cron sweep and the SQS execute_purchase handler, both through executableByScheduler) no longer executes a recommendation-bearing step of an AutoPurchase plan. The step stays notified and waits for a human approval in the approval queue, like a manual plan's step. The approval then runs enforceFourEyesPolicy, which accepts any identified approver for a system-created row. Empty steps route as before. A missing or unreadable global config still fails closed. A step root pre-created through the admin API key has no recorded creator and is not system-created, so nobody can approve it under 4-eyes. It waits, then expires. This follows decision (a) on #1771 and is documented on RequireDifferentApprover. Tests: - real-Postgres: the sweep leaves the row notified with no purchase, and user B's approval then buys once; - a root pre-created by A: A is denied and B succeeds; - with the flag off, the sweep executes as before; - unit tests for the sweep and the SQS gate. Dropping the gate fails them. Part of #1771 Co-Authored-By: Claude Opus 5.5 --- internal/config/types.go | 8 +- .../four_eyes_auto_plan_integration_test.go | 105 ++++++++++++++++++ internal/purchase/manager.go | 31 +++++- internal/purchase/manager_test.go | 43 +++++++ internal/purchase/messages.go | 2 +- internal/purchase/messages_test.go | 21 ++++ 6 files changed, 202 insertions(+), 8 deletions(-) create mode 100644 internal/purchase/four_eyes_auto_plan_integration_test.go diff --git a/internal/config/types.go b/internal/config/types.go index a9c901535..c3f3e63d0 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -117,7 +117,13 @@ type GlobalConfig struct { // mode first (the admin wildcard is NOT exempt from the restriction). // While it is on, unattended RI exchange runs (RIExchangeMode "auto" and // ladder reshapes) run as "manual" and only raise pending records - // (issue #679). + // (issue #679), and recommendation-bearing AutoPurchase steps are not + // auto-executed: they wait for a human approval in the approval queue + // (issue #1771); empty steps still route as before. A step root + // pre-created with the plan keeps its human creator, who then cannot + // approve it. A step root pre-created through the admin API key (no + // creator and not system-created) cannot be approved by anyone while this + // is on: it waits, then expires, and a human recreates the plan. RequireDifferentApprover bool `json:"require_different_approver" dynamodbav:"require_different_approver"` } diff --git a/internal/purchase/four_eyes_auto_plan_integration_test.go b/internal/purchase/four_eyes_auto_plan_integration_test.go new file mode 100644 index 000000000..41a16d581 --- /dev/null +++ b/internal/purchase/four_eyes_auto_plan_integration_test.go @@ -0,0 +1,105 @@ +//go:build integration + +package purchase + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Issue #1771: with 4-eyes on, an armed auto plan's due step waits for a human +// approval instead of being bought by the sweep. Real PostgreSQL; the cloud is +// the labeled fake provider of the ramp-step fixture. + +func fourEyesUser(ctx context.Context, t *testing.T, f *rampStepFixture, name string) string { + t.Helper() + id := uuid.New().String() + require.NoError(t, f.store.WithTx(ctx, func(tx pgx.Tx) error { + _, err := tx.Exec(ctx, `INSERT INTO users (id, email, password_hash, group_ids) VALUES ($1, $2, 'x', ARRAY[$1::uuid])`, id, name+"@example.invalid") + return err + })) + return id +} + +func fourEyesAutoRow(ctx context.Context, t *testing.T, f *rampStepFixture, creator *string) *config.PurchaseExecution { + t.Helper() + row := &config.PurchaseExecution{ + PlanID: f.plan.ID, ExecutionID: uuid.New().String(), Status: "notified", StepNumber: 3, + ScheduledDate: time.Now().Add(-time.Hour), CloudAccountID: &f.accounts[0].ID, + Recommendations: []config.RecommendationRecord{withAccount(rampStepRecommendation()[0], f.accounts[0].ID)}, + CreatedByUserID: creator, CreatedBySystem: creator == nil, + } + require.NoError(t, f.store.SavePurchaseExecution(ctx, row)) + return row +} + +func setFourEyes(ctx context.Context, t *testing.T, f *rampStepFixture, on bool) { + t.Helper() + global, err := f.store.GetGlobalConfig(ctx) + require.NoError(t, err) + global.RequireDifferentApprover = on + require.NoError(t, f.store.SaveGlobalConfig(ctx, global)) +} + +func TestFourEyes_AutoPlanSweepLeavesRowForHumanApproval(t *testing.T) { + ctx := context.Background() + f := newRampStepFixture(ctx, t) + setFourEyes(ctx, t, f, true) + row := fourEyesAutoRow(ctx, t, f, nil) + + result, err := f.manager.ProcessScheduledPurchases(ctx) + require.NoError(t, err) + assert.Equal(t, 0, result.Processed) + assert.Equal(t, 0, f.purchaseCount()) + assert.Equal(t, "notified", rowStatus(ctx, t, f, row.ExecutionID).Status) + + userB := fourEyesUser(ctx, t, f, "b") + _, _, err = f.manager.ApproveAndExecute(ctx, row.ExecutionID, "b@example.invalid", &userB, nil) + require.NoError(t, err) + assert.Equal(t, 1, f.purchaseCount()) + assert.Equal(t, "completed", rowStatus(ctx, t, f, row.ExecutionID).Status) +} + +func TestFourEyes_AutoPlanPreCreatedRootCreatorCannotApprove(t *testing.T) { + ctx := context.Background() + f := newRampStepFixture(ctx, t) + setFourEyes(ctx, t, f, true) + userA := fourEyesUser(ctx, t, f, "a") + userB := fourEyesUser(ctx, t, f, "b") + row := fourEyesAutoRow(ctx, t, f, &userA) + + result, err := f.manager.ProcessScheduledPurchases(ctx) + require.NoError(t, err) + assert.Equal(t, 0, result.Processed) + assert.Equal(t, 0, f.purchaseCount()) + + _, _, err = f.manager.ApproveAndExecute(ctx, row.ExecutionID, "a@example.invalid", &userA, nil) + require.ErrorIs(t, err, ErrFourEyesDenied) + assert.Equal(t, 0, f.purchaseCount()) + assert.Equal(t, "notified", rowStatus(ctx, t, f, row.ExecutionID).Status) + + _, _, err = f.manager.ApproveAndExecute(ctx, row.ExecutionID, "b@example.invalid", &userB, nil) + require.NoError(t, err) + assert.Equal(t, 1, f.purchaseCount()) + assert.Equal(t, "completed", rowStatus(ctx, t, f, row.ExecutionID).Status) +} + +func TestFourEyes_AutoPlanSweepExecutesWhenFlagOff(t *testing.T) { + ctx := context.Background() + f := newRampStepFixture(ctx, t) + setFourEyes(ctx, t, f, false) + row := fourEyesAutoRow(ctx, t, f, nil) + + result, err := f.manager.ProcessScheduledPurchases(ctx) + require.NoError(t, err) + assert.Equal(t, 1, result.Executed) + assert.Equal(t, 1, f.purchaseCount()) + assert.Equal(t, "completed", rowStatus(ctx, t, f, row.ExecutionID).Status) +} diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index e0c9b1122..b719366bc 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -879,6 +879,9 @@ func (m *Manager) RecoverStrandedApprovals(ctx context.Context) (int, error) { // - The global kill switch (GlobalConfig.AutoPurchaseEnabled) must be on. A // read error is propagated. Only AutoPurchase plans reach this check, so // manual purchases are unaffected. +// - With 4-eyes approval mode on (GlobalConfig.RequireDifferentApprover), a +// recommendation-bearing row is never auto-executed: it waits for a human +// approval in the approval queue (#1771). // - A recommendation-bearing row must be "notified" (the notification email is // the human veto window; a "pending" row may never have been emailed) and // must fit the plan's max_step_commitment (CheckStepCommitmentCap, whose @@ -902,16 +905,22 @@ func (m *Manager) executableByScheduler(ctx context.Context, exec *config.Purcha if plan == nil { return false, nil } - enabled, err := m.autoPurchaseEnabled(ctx) + cfg, err := m.autoPurchaseConfig(ctx) if err != nil { return false, err } - if !enabled { + if !cfg.AutoPurchaseEnabled { return false, nil } if len(exec.Recommendations) == 0 { return true, nil } + // With 4-eyes on, no step buys without a second person: an auto plan's + // step waits in the approval queue for a human approval (session approve), + // like a manual plan's (#1771). + if cfg.RequireDifferentApprover { + return false, nil + } if exec.Status != "notified" { return false, nil } @@ -951,14 +960,24 @@ func (m *Manager) autoPurchasePlan(ctx context.Context, exec *config.PurchaseExe // autoPurchaseEnabled reads the global auto-purchase kill switch. It fails // closed: a read error is returned, never read as "enabled". func (m *Manager) autoPurchaseEnabled(ctx context.Context) (bool, error) { + cfg, err := m.autoPurchaseConfig(ctx) + if err != nil { + return false, err + } + return cfg.AutoPurchaseEnabled, nil +} + +// autoPurchaseConfig reads the global config the auto-purchase gates need. A +// read error or a missing config is returned, so no gate reads it as "on". +func (m *Manager) autoPurchaseConfig(ctx context.Context) (*config.GlobalConfig, error) { cfg, err := m.config.GetGlobalConfig(ctx) if err != nil { - return false, fmt.Errorf("failed to read global config for the auto-purchase kill switch: %w", err) + return nil, fmt.Errorf("failed to read global config for the auto-purchase kill switch: %w", err) } if cfg == nil { - return false, errors.New("global config unavailable for the auto-purchase kill switch") + return nil, errors.New("global config unavailable for the auto-purchase kill switch") } - return cfg.AutoPurchaseEnabled, nil + return cfg, nil } // processOneExecution runs the full gate+claim+execute pipeline for a single @@ -979,7 +998,7 @@ func (m *Manager) processOneExecution(ctx context.Context, exec config.PurchaseE return } if !eligible { - logging.Infof("Skipping execution %s (AutoPurchase=false, kill switch off, source=web, or recommendation-bearing row not yet notified; requires explicit approval)", exec.ExecutionID) + logging.Infof("Skipping execution %s (AutoPurchase=false, kill switch off, source=web, 4-eyes on so it needs a human approval in the approval queue, or recommendation-bearing row not yet notified; requires explicit approval)", exec.ExecutionID) return } diff --git a/internal/purchase/manager_test.go b/internal/purchase/manager_test.go index ae9f95d9b..299d4ef78 100644 --- a/internal/purchase/manager_test.go +++ b/internal/purchase/manager_test.go @@ -1275,3 +1275,46 @@ func TestManager_RecoverStrandedApprovals_AWSClaimLost_NoRedrive(t *testing.T) { mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) } + +// #1771: with 4-eyes on, an armed auto plan's rec-bearing notified row is not +// auto-executed; it waits for a human approval in the approval queue. +func TestManager_ProcessScheduledPurchases_FourEyesOnSkipsAutoRow(t *testing.T) { + cfg := autoPurchaseOnCfg() + cfg.RequireDifferentApprover = true + plan := &config.PurchasePlan{ID: "plan-gate", AutoPurchase: true, MaxStepCommitment: f64(1000)} + result := gateHarness(t, plan, dueRecExec("notified", upfrontRec(10)), cfg, nil) + assert.Equal(t, 0, result.Processed) + assert.Equal(t, 0, result.Executed) + assert.Equal(t, 0, result.Failed, "waiting for approval is a skip, not a failure") +} + +// With 4-eyes off the gate is unchanged. +func TestExecutableByScheduler_FourEyesOffUnchanged(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + assertMocksOnCleanup(t, store) + plan := &config.PurchasePlan{ID: "plan-gate", AutoPurchase: true, MaxStepCommitment: f64(1000)} + store.On("GetPurchasePlan", ctx, "plan-gate").Return(plan, nil) + store.On("GetGlobalConfig", ctx).Return(autoPurchaseOnCfg(), nil) + manager := &Manager{config: store} + exec := dueRecExec("notified", upfrontRec(10)) + ok, err := manager.executableByScheduler(ctx, &exec) + require.NoError(t, err) + assert.True(t, ok) +} + +// An empty step buys nothing, so 4-eyes does not park it in limbo. +func TestExecutableByScheduler_FourEyesOnEmptyStepStillRoutes(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + assertMocksOnCleanup(t, store) + cfg := autoPurchaseOnCfg() + cfg.RequireDifferentApprover = true + store.On("GetPurchasePlan", ctx, "plan-gate").Return(&config.PurchasePlan{ID: "plan-gate", AutoPurchase: true}, nil) + store.On("GetGlobalConfig", ctx).Return(cfg, nil) + manager := &Manager{config: store} + exec := dueRecExec("notified") + ok, err := manager.executableByScheduler(ctx, &exec) + require.NoError(t, err) + assert.True(t, ok) +} diff --git a/internal/purchase/messages.go b/internal/purchase/messages.go index fa1a5f1bc..587137910 100644 --- a/internal/purchase/messages.go +++ b/internal/purchase/messages.go @@ -96,7 +96,7 @@ func (m *Manager) checkAutoExecuteGate(ctx context.Context, execution *config.Pu return fmt.Errorf("AutoPurchase gate check failed for execution %s: %w", execution.ExecutionID, err) } if !eligible { - return fmt.Errorf("execution %s (status=%s source=%q) is not eligible for auto-execution: AutoPurchase=false, kill switch off, source=web, or recommendation-bearing row not yet notified - approve via the approval link", + return fmt.Errorf("execution %s (status=%s source=%q) is not eligible for auto-execution: AutoPurchase=false, kill switch off, source=web, 4-eyes on (it needs a human approval in the approval queue), or recommendation-bearing row not yet notified - approve it in the approval queue", execution.ExecutionID, execution.Status, execution.Source) } return nil diff --git a/internal/purchase/messages_test.go b/internal/purchase/messages_test.go index 362b8a8d9..149064f84 100644 --- a/internal/purchase/messages_test.go +++ b/internal/purchase/messages_test.go @@ -9,6 +9,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) func TestManager_ProcessMessage(t *testing.T) { @@ -190,3 +191,23 @@ func TestManager_ProcessMessage(t *testing.T) { assert.NoError(t, err) }) } + +// #1771: an SQS execute_purchase for an auto row is refused while 4-eyes is +// on, with an error that names the reason, and nothing is claimed or bought. +func TestCheckAutoExecuteGate_FourEyesOnRefusesNotifiedRow(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) // TransitionExecutionStatus is not mocked: a claim would panic + assertMocksOnCleanup(t, store) + cfg := autoPurchaseOnCfg() + cfg.RequireDifferentApprover = true + store.On("GetPurchasePlan", ctx, "plan-gate").Return(&config.PurchasePlan{ID: "plan-gate", AutoPurchase: true, MaxStepCommitment: f64(1000)}, nil) + store.On("GetGlobalConfig", ctx).Return(cfg, nil) + manager := &Manager{config: store} + exec := dueRecExec("notified", upfrontRec(10)) + + err := manager.checkAutoExecuteGate(ctx, &exec) + + require.Error(t, err) + assert.Contains(t, err.Error(), "4-eyes") + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} From 658a922a1e1c6dd422250693ff43aa26a3666b42 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 18:49:29 +0100 Subject: [PATCH 2/3] fix(email): say a plan step needs approval when it won't run by itself The upcoming-purchase email told every recipient the step "will be purchased on X unless canceled". That isn't true for a manual plan, or for an AutoPurchase plan while 4-eyes is on. Those steps are bought only once someone approves them, and the email had no approve link. sendPlanNotification now takes needsApproval, computed per tick from the global config the notifier already reads: !AutoPurchase || no config || RequireDifferentApprover. When it's set, the template keeps the totals and cap sentence and says: - the step is due on X and is bought once approved; - to approve it in the approval queue, through the existing Review & Edit link; - that it does not run automatically. When it isn't set, the text is byte-identical to before. Tests: - the helper's modes; - the rendered wording in both modes; - the call-site wiring in the SendUpcomingPurchaseNotifications tests. Forcing the value at the call site fails them. Closes #1771 Co-Authored-By: Claude Opus 5.5 --- internal/email/sender.go | 3 + internal/email/template_renderers_test.go | 18 +++++ internal/email/templates.go | 6 +- internal/purchase/claim_pin_test.go | 2 +- ..._eyes_system_execution_integration_test.go | 11 ++- internal/purchase/notification_data_test.go | 71 ++++++++++++++++++- internal/purchase/notifications.go | 22 ++++-- internal/purchase/notifications_test.go | 18 ++--- .../notified_claim_pin_integration_test.go | 4 +- .../plan_step_attach_integration_test.go | 14 ++-- .../plan_step_crash_integration_test.go | 6 +- ...plan_step_managed_race_integration_test.go | 4 +- .../plan_step_retention_integration_test.go | 2 +- 13 files changed, 141 insertions(+), 40 deletions(-) diff --git a/internal/email/sender.go b/internal/email/sender.go index a09753cec..04ad347fc 100644 --- a/internal/email/sender.go +++ b/internal/email/sender.go @@ -587,6 +587,9 @@ type NotificationData struct { TotalRecurring float64 TotalUnits int StepCap float64 + // NeedsApproval is set when the step does not run on its date but waits + // for a human approval in the approval queue (manual plan, or 4-eyes on). + NeedsApproval bool // RevocationToken is the one-time token embedded in the revocation link // of a post-execution notification email. When non-empty, the template // renders a "Revoke this purchase" CTA that hits diff --git a/internal/email/template_renderers_test.go b/internal/email/template_renderers_test.go index 5f6f66e5a..b98e7b9ae 100644 --- a/internal/email/template_renderers_test.go +++ b/internal/email/template_renderers_test.go @@ -797,3 +797,21 @@ func TestRenderRIExchangePendingApprovalEmail_PlainTextVerbatim(t *testing.T) { assert.NotContains(t, body, "'", "plain-text must not HTML-encode apostrophe") assert.NotContains(t, body, "&", "plain-text must not HTML-encode ampersand") } + +func TestRenderScheduledPurchaseEmail_NeedsApprovalWording(t *testing.T) { + data := NotificationData{ + PlanName: "p", PurchaseDate: "November 3, 2026", DaysUntilPurchase: 3, DashboardURL: "https://d.example", ExecutionID: "e1", + TotalCommitment: 100, StepCap: 500, TotalUnits: 1, Recommendations: []RecommendationSummary{{Count: 1, ResourceType: "m5.large", Region: "r"}}, + } + body, err := RenderScheduledPurchaseEmail(data) + require.NoError(t, err) + assert.Contains(t, body, ", cap $500.00, on November 3, 2026 unless canceled.") + assert.Contains(t, body, "You have 3 days to modify or cancel before automatic execution.") + + data.NeedsApproval = true + body, err = RenderScheduledPurchaseEmail(data) + require.NoError(t, err) + assert.Contains(t, body, ", cap $500.00, due on November 3, 2026. It is bought once approved. Review & Edit below and approve it in the approval queue.") + assert.NotContains(t, body, "unless canceled") + assert.NotContains(t, body, "before automatic execution") +} diff --git a/internal/email/templates.go b/internal/email/templates.go index 27086badd..c5a642dcc 100644 --- a/internal/email/templates.go +++ b/internal/email/templates.go @@ -41,9 +41,9 @@ This is an automated message from CUDly. const scheduledPurchaseTemplate = `CUDly - Scheduled Purchase in {{.DaysUntilPurchase}} Days ========================================= -Based on your Purchase Plan "{{.PlanName}}", the following commitments will be purchased on {{.PurchaseDate}}: +Based on your Purchase Plan "{{.PlanName}}", the following commitments {{if .NeedsApproval}}are due on{{else}}will be purchased on{{end}} {{.PurchaseDate}}: {{if gt .TotalCommitment 0.0}} -Will buy {{len .Recommendations}} recommendation(s) ({{.TotalUnits}} units) for ${{printf "%.2f" .TotalCommitment}} total commitment (upfront ${{printf "%.2f" .TotalUpfrontCost}} + ${{printf "%.2f" .TotalRecurring}} recurring over the terms){{if gt .StepCap 0.0}}, cap ${{printf "%.2f" .StepCap}}{{end}}, on {{.PurchaseDate}} unless canceled. +Will buy {{len .Recommendations}} recommendation(s) ({{.TotalUnits}} units) for ${{printf "%.2f" .TotalCommitment}} total commitment (upfront ${{printf "%.2f" .TotalUpfrontCost}} + ${{printf "%.2f" .TotalRecurring}} recurring over the terms){{if gt .StepCap 0.0}}, cap ${{printf "%.2f" .StepCap}}{{end}}, {{if .NeedsApproval}}due on {{.PurchaseDate}}. It is bought once approved. Review & Edit below and approve it in the approval queue.{{else}}on {{.PurchaseDate}} unless canceled.{{end}} {{end}} {{range .Recommendations}} - {{.Count}}x {{.ResourceType}}{{if .Engine}} ({{.Engine}}){{end}} in {{.Region}}{{if .AvailabilityZone}} ({{.AvailabilityZone}}){{end}}{{if .Platform}} | {{.Platform}}{{end}}{{if .Tenancy}} | {{.Tenancy}}{{end}} @@ -64,7 +64,7 @@ Actions: [Cancel This Purchase] {{.DashboardURL}}/purchases/cancel/{{.ExecutionID}}?token={{urlquery .ApprovalToken}} -You have {{.DaysUntilPurchase}} days to modify or cancel before automatic execution. +{{if .NeedsApproval}}This purchase is not bought automatically. It needs a human approval in the approval queue; you have {{.DaysUntilPurchase}} days to review, modify or cancel it.{{else}}You have {{.DaysUntilPurchase}} days to modify or cancel before automatic execution.{{end}} This is an automated message from CUDly. ` diff --git a/internal/purchase/claim_pin_test.go b/internal/purchase/claim_pin_test.go index 5175a619f..8a6fe18cd 100644 --- a/internal/purchase/claim_pin_test.go +++ b/internal/purchase/claim_pin_test.go @@ -201,7 +201,7 @@ func TestNotification_EmailFailureLeavesRowPendingAndSchedulerRefusesIt(t *testi sender.On("SendScheduledPurchaseNotification", ctx, mock.Anything).Return(errors.New("ses down")) manager := &Manager{config: store, email: sender} - require.False(t, manager.sendPlanNotification(ctx, plan)) + require.False(t, manager.sendPlanNotification(ctx, plan, false)) store.AssertNotCalled(t, "RotatePendingApprovalToken", mock.Anything, mock.Anything, mock.Anything, mock.Anything) // The row as the DB still holds it: pending. The scheduler leaves it alone. diff --git a/internal/purchase/four_eyes_system_execution_integration_test.go b/internal/purchase/four_eyes_system_execution_integration_test.go index 5fead30ff..d1a844095 100644 --- a/internal/purchase/four_eyes_system_execution_integration_test.go +++ b/internal/purchase/four_eyes_system_execution_integration_test.go @@ -20,7 +20,11 @@ func TestFourEyes_SweepMintedRowIsApprovableByIdentifiedApprover(t *testing.T) { plan := attachPlanInDB(ctx, t, store, "four-eyes-sweep", time.Now().Add(24*time.Hour)) m := &Manager{config: store, email: newCountingSender(ctx), resolver: &fakeResolver{recs: []config.RecommendationRecord{attachRec("rec-a", 2)}}} - require.True(t, m.sendPlanNotification(ctx, plan)) + global, err := store.GetGlobalConfig(ctx) + require.NoError(t, err) + global.RequireDifferentApprover = true + require.NoError(t, store.SaveGlobalConfig(ctx, global)) + require.True(t, m.sendPlanNotification(ctx, plan, planNeedsApproval(*plan, global))) rows := stepRowsInDB(ctx, t, store, plan.ID) require.Len(t, rows, 1) @@ -28,11 +32,6 @@ func TestFourEyes_SweepMintedRowIsApprovableByIdentifiedApprover(t *testing.T) { assert.True(t, rows[0].CreatedBySystem, "sweep-minted row must be marked system-created") assert.Nil(t, rows[0].CreatedByUserID) - global, err := store.GetGlobalConfig(ctx) - require.NoError(t, err) - global.RequireDifferentApprover = true - require.NoError(t, store.SaveGlobalConfig(ctx, global)) - require.NoError(t, m.enforceFourEyesPolicy(ctx, rows[0].ExecutionID, "approver@example.com", nil)) require.ErrorIs(t, m.enforceFourEyesPolicy(ctx, rows[0].ExecutionID, "", nil), ErrFourEyesDenied) } diff --git a/internal/purchase/notification_data_test.go b/internal/purchase/notification_data_test.go index 5f3493b7a..9aa26a8dc 100644 --- a/internal/purchase/notification_data_test.go +++ b/internal/purchase/notification_data_test.go @@ -28,7 +28,7 @@ func TestBuildNotificationData_ShowsTotalCommitmentAndCap(t *testing.T) { } m := &Manager{} - data := m.buildNotificationData(plan, exec, "tok", 3, "ops@example.invalid") + data := m.buildNotificationData(plan, exec, "tok", 3, "ops@example.invalid", true) assert.InDelta(t, 1500.0, data.TotalCommitment, 1e-9) assert.InDelta(t, 1200.0, data.TotalRecurring, 1e-9) @@ -42,7 +42,74 @@ func TestBuildNotificationData_ShowsTotalCommitmentAndCap(t *testing.T) { body, err := email.RenderScheduledPurchaseEmail(data) require.NoError(t, err) - assert.Contains(t, body, "Will buy 2 recommendation(s) (5 units) for $1500.00 total commitment (upfront $300.00 + $1200.00 recurring over the terms), cap $5000.00, on November 3, 2026 unless canceled.") + assert.Contains(t, body, "Will buy 2 recommendation(s) (5 units) for $1500.00 total commitment (upfront $300.00 + $1200.00 recurring over the terms), cap $5000.00, due on November 3, 2026. It is bought once approved. Review & Edit below and approve it in the approval queue.") + assert.NotContains(t, body, "unless canceled") + + // Auto plan with 4-eyes off: the original wording is unchanged. + autoData := m.buildNotificationData(plan, exec, "tok", 3, "ops@example.invalid", false) + autoBody, err := email.RenderScheduledPurchaseEmail(autoData) + require.NoError(t, err) + assert.Contains(t, autoBody, "cap $5000.00, on November 3, 2026 unless canceled.") assert.Contains(t, body, "Term: 1 yr monthly | Upfront $0.00 + $100.00/month | Commitment $1200.00") assert.Contains(t, body, "in us-east-1 (us-east-1a) | Linux/UNIX | default") } + +// #1771: a step that waits for a human approval (manual plan, or an auto plan +// while 4-eyes is on) must not tell the approver it buys unless canceled. +func TestPlanNeedsApproval_Modes(t *testing.T) { + on := &config.GlobalConfig{RequireDifferentApprover: true} + off := &config.GlobalConfig{} + cases := []struct { + name string + plan config.PurchasePlan + global *config.GlobalConfig + want bool + }{ + {"manual plan", config.PurchasePlan{}, off, true}, + {"auto plan, 4-eyes on", config.PurchasePlan{AutoPurchase: true}, on, true}, + {"auto plan, 4-eyes off", config.PurchasePlan{AutoPurchase: true}, off, false}, + {"auto plan, nil config fails closed", config.PurchasePlan{AutoPurchase: true}, nil, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, planNeedsApproval(tc.plan, tc.global)) + }) + } +} + +func TestBuildNotificationData_ApprovalWordingByMode(t *testing.T) { + capAmount := 5000.0 + plan := config.PurchasePlan{ID: "plan-1", Name: "ramp", MaxStepCommitment: &capAmount} + exec := &config.PurchaseExecution{ + ExecutionID: "exec-1", PlanID: "plan-1", ScheduledDate: time.Date(2026, 11, 3, 0, 0, 0, 0, time.UTC), + Recommendations: []config.RecommendationRecord{ + {Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100}, + }, + } + m := &Manager{} + const totals = "cap $5000.00, " + for _, tc := range []struct { + name string + needsApproval bool + }{ + {"manual plan or auto with 4-eyes on", true}, + {"auto plan with 4-eyes off", false}, + } { + t.Run(tc.name, func(t *testing.T) { + data := m.buildNotificationData(plan, exec, "tok", 3, "ops@example.invalid", tc.needsApproval) + assert.Equal(t, tc.needsApproval, data.NeedsApproval) + body, err := email.RenderScheduledPurchaseEmail(data) + require.NoError(t, err) + assert.Contains(t, body, totals, "the totals and cap sentence stays in both modes") + if tc.needsApproval { + assert.Contains(t, body, totals+"due on November 3, 2026. It is bought once approved. Review & Edit below and approve it in the approval queue.") + assert.NotContains(t, body, "unless canceled") + assert.NotContains(t, body, "before automatic execution") + } else { + assert.Contains(t, body, totals+"on November 3, 2026 unless canceled.") + assert.Contains(t, body, "You have 3 days to modify or cancel before automatic execution.") + assert.NotContains(t, body, "approval queue") + } + }) + } +} diff --git a/internal/purchase/notifications.go b/internal/purchase/notifications.go index 042f82686..52d080f70 100644 --- a/internal/purchase/notifications.go +++ b/internal/purchase/notifications.go @@ -42,7 +42,7 @@ func (m *Manager) SendUpcomingPurchaseNotifications(ctx context.Context) (*Notif continue } if m.shouldNotifyPlan(plan, autoPurchaseEnabled) || shouldRetryOverduePlan(plan, autoPurchaseEnabled, global, time.Now()) { - if m.sendPlanNotification(ctx, &plan) { + if m.sendPlanNotification(ctx, &plan, planNeedsApproval(plan, global)) { notified++ } } @@ -152,8 +152,17 @@ func (m *Manager) dueManualRootTx(ctx context.Context, tx pgx.Tx, planID string) // plan's NextExecutionDate advances hits it, so it is skipped quietly. var errExecutionNotNotifiable = errors.New("execution is no longer awaiting approval") -// sendPlanNotification sends a notification for a plan and returns true if successful. -func (m *Manager) sendPlanNotification(ctx context.Context, plan *config.PurchasePlan) bool { +// planNeedsApproval reports whether the plan's recommendation-bearing step +// waits for a human approval instead of executing on its date: manual plans +// always do, and auto plans do while 4-eyes mode is on (#1771). A missing +// config fails closed. +func planNeedsApproval(plan config.PurchasePlan, global *config.GlobalConfig) bool { + return !plan.AutoPurchase || global == nil || global.RequireDifferentApprover +} + +// sendPlanNotification sends a notification for a plan and returns true if +// successful. needsApproval selects the approval wording of the email. +func (m *Manager) sendPlanNotification(ctx context.Context, plan *config.PurchasePlan, needsApproval bool) bool { daysUntil := int(time.Until(*plan.NextExecutionDate).Hours() / config.HoursPerDay) logging.Infof("Sending notification for plan %s (purchase in %d days)", plan.Name, daysUntil) @@ -180,7 +189,7 @@ func (m *Manager) sendPlanNotification(ctx context.Context, plan *config.Purchas // The row's own date, not the plan's: an attach for an overdue plan is // scheduled NotificationDaysBefore days out. daysUntil = int(time.Until(execution.ScheduledDate).Hours() / config.HoursPerDay) - data := m.buildNotificationData(*plan, execution, rawApprovalToken, daysUntil, notifyEmail) + data := m.buildNotificationData(*plan, execution, rawApprovalToken, daysUntil, notifyEmail, needsApproval) if err := m.email.SendScheduledPurchaseNotification(ctx, data); err != nil { logging.Errorf("Failed to send notification: %v", err) return false @@ -388,7 +397,9 @@ func bareStepFailure(planID string, step int, reason string) error { // as RecipientEmail so the token-bearing body routes through targeted SES. // rawApprovalToken is the RAW token from getOrCreateExecution -- never read // from exec.ApprovalToken, which holds only the hash (issue #103). -func (m *Manager) buildNotificationData(plan config.PurchasePlan, exec *config.PurchaseExecution, rawApprovalToken string, daysUntil int, notifyEmail string) email.NotificationData { +// needsApproval is decided by the caller from the tick's global config (see +// planNeedsApproval); this function reads no config. +func (m *Manager) buildNotificationData(plan config.PurchasePlan, exec *config.PurchaseExecution, rawApprovalToken string, daysUntil int, notifyEmail string, needsApproval bool) email.NotificationData { data := email.NotificationData{ DashboardURL: m.dashboardURL, ApprovalToken: rawApprovalToken, @@ -400,6 +411,7 @@ func (m *Manager) buildNotificationData(plan config.PurchasePlan, exec *config.P DaysUntilPurchase: daysUntil, PlanName: plan.Name, RecipientEmail: notifyEmail, + NeedsApproval: needsApproval, } if plan.MaxStepCommitment != nil { diff --git a/internal/purchase/notifications_test.go b/internal/purchase/notifications_test.go index c8a39fc26..e8d533418 100644 --- a/internal/purchase/notifications_test.go +++ b/internal/purchase/notifications_test.go @@ -155,7 +155,7 @@ func TestManager_BuildNotificationData(t *testing.T) { }, } - data := manager.buildNotificationData(plan, execution, "token-abc", 5, "notify@example.com") + data := manager.buildNotificationData(plan, execution, "token-abc", 5, "notify@example.com", false) assert.Equal(t, "https://dashboard.example.com", data.DashboardURL) assert.Equal(t, "token-abc", data.ApprovalToken) @@ -521,7 +521,9 @@ func TestManager_SendUpcomingPurchaseNotifications_WithNotification(t *testing.T expectStepRows(mockStore, &plans[0], *pendingExec) mockStore.On("RotatePendingApprovalToken", ctx, "pending-exec-id", mock.Anything, mock.Anything).Return(true, nil) mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil) - mockEmail.On("SendScheduledPurchaseNotification", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil) + mockEmail.On("SendScheduledPurchaseNotification", ctx, mock.MatchedBy(func(data email.NotificationData) bool { + return !data.NeedsApproval // auto plan, 4-eyes off: the original wording + })).Return(nil) mockStore.On("StampPlanNotificationSent", ctx, "plan-123", mock.AnythingOfType("time.Time")).Return(nil) manager := &Manager{ @@ -551,7 +553,7 @@ func TestManager_SendUpcomingPurchaseNotifications_ManualCleanupErrorDoesNotBloc } address := "fixture@example.invalid" store.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) - store.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &address, AutoPurchaseEnabled: true}, nil) + store.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &address, AutoPurchaseEnabled: true, RequireDifferentApprover: true}, nil) store.On("WithTx", ctx, mock.Anything).Return(errors.New("manual cleanup transaction failed")).Once() store.On("WithTx", ctx, mock.Anything).Return(nil).Run(func(args mock.Arguments) { require.NoError(t, args.Get(1).(func(pgx.Tx) error)(nil)) @@ -563,7 +565,7 @@ func TestManager_SendUpcomingPurchaseNotifications_ManualCleanupErrorDoesNotBloc store.On("RotatePendingApprovalToken", ctx, "automatic-root", mock.Anything, mock.Anything).Return(true, nil).Once() store.On("StampPlanNotificationSent", ctx, plans[1].ID, mock.AnythingOfType("time.Time")).Return(nil).Once() sender.On("SendScheduledPurchaseNotification", ctx, mock.MatchedBy(func(data email.NotificationData) bool { - return data.PlanID == plans[1].ID + return data.PlanID == plans[1].ID && data.NeedsApproval // auto plan, 4-eyes on })).Return(nil).Once() manager := &Manager{config: store, email: sender, dashboardURL: "https://dashboard.example.invalid"} @@ -760,7 +762,7 @@ func TestManager_SendPlanNotification_NoRecipientLeavesLiveTokenAlone(t *testing mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) manager := &Manager{config: mockStore, email: mockEmail, dashboardURL: "https://dashboard.example.com"} - assert.False(t, manager.sendPlanNotification(ctx, plan)) + assert.False(t, manager.sendPlanNotification(ctx, plan, false)) assert.Equal(t, config.HashApprovalToken("previously-emailed-token"), existing.ApprovalToken) mockStore.AssertExpectations(t) mockEmail.AssertExpectations(t) @@ -779,7 +781,7 @@ func TestManager_SendPlanNotification_SendFailureLeavesLiveTokenAlone(t *testing mockEmail.On("SendScheduledPurchaseNotification", ctx, mock.AnythingOfType("email.NotificationData")).Return(errors.New("ses down")) manager := &Manager{config: mockStore, email: mockEmail, dashboardURL: "https://dashboard.example.com"} - assert.False(t, manager.sendPlanNotification(ctx, plan)) + assert.False(t, manager.sendPlanNotification(ctx, plan, false)) mockStore.AssertNotCalled(t, "RotatePendingApprovalToken", mock.Anything, mock.Anything, mock.Anything, mock.Anything) mockStore.AssertNotCalled(t, "StampPlanNotificationSent", mock.Anything, mock.Anything, mock.Anything) mockStore.AssertExpectations(t) @@ -812,7 +814,7 @@ func TestManager_SendPlanNotification_PersistsEmailedTokenHashAfterSend(t *testi mockStore.On("StampPlanNotificationSent", ctx, plan.ID, mock.AnythingOfType("time.Time")).Return(nil) manager := &Manager{config: mockStore, email: mockEmail, dashboardURL: "https://dashboard.example.com"} - assert.True(t, manager.sendPlanNotification(ctx, plan)) + assert.True(t, manager.sendPlanNotification(ctx, plan, false)) require.NotEmpty(t, emailedToken) assert.Equal(t, config.HashApprovalToken(emailedToken), persistedHash) assert.NotEqual(t, emailedToken, persistedHash) @@ -831,7 +833,7 @@ func TestManager_SendPlanNotification_StampFailure(t *testing.T) { store.On("RotatePendingApprovalToken", ctx, "exec-rot", mock.Anything, mock.Anything).Return(true, nil) store.On("StampPlanNotificationSent", ctx, plan.ID, mock.Anything).Return(errors.New("stamp failed")) manager := &Manager{config: store, email: sender} - require.False(t, manager.sendPlanNotification(ctx, plan)) + require.False(t, manager.sendPlanNotification(ctx, plan, false)) require.Nil(t, plan.LastNotificationSent) store.AssertExpectations(t) sender.AssertExpectations(t) diff --git a/internal/purchase/notified_claim_pin_integration_test.go b/internal/purchase/notified_claim_pin_integration_test.go index db3d2a9d9..7c67f967f 100644 --- a/internal/purchase/notified_claim_pin_integration_test.go +++ b/internal/purchase/notified_claim_pin_integration_test.go @@ -64,7 +64,7 @@ func TestNotifiedTransition_OnlyAfterEmailSucceeds(t *testing.T) { assertMocksOnCleanup(t, sender) sender.On("SendScheduledPurchaseNotification", ctx, mock.Anything).Return(errors.New("ses down")).Once() manager := &Manager{config: store, email: sender, providerFactory: factory} - require.False(t, manager.sendPlanNotification(ctx, plan)) + require.False(t, manager.sendPlanNotification(ctx, plan, false)) assert.Equal(t, "pending", statusInDB(ctx, t, store, row.ExecutionID).Status) result, err := manager.ProcessScheduledPurchases(ctx) @@ -73,7 +73,7 @@ func TestNotifiedTransition_OnlyAfterEmailSucceeds(t *testing.T) { assert.Equal(t, "pending", statusInDB(ctx, t, store, row.ExecutionID).Status) sender.On("SendScheduledPurchaseNotification", ctx, mock.Anything).Return(nil).Once() - require.True(t, manager.sendPlanNotification(ctx, plan)) + require.True(t, manager.sendPlanNotification(ctx, plan, false)) notified := statusInDB(ctx, t, store, row.ExecutionID) assert.Equal(t, "notified", notified.Status) assert.NotEmpty(t, notified.ApprovalToken) diff --git a/internal/purchase/plan_step_attach_integration_test.go b/internal/purchase/plan_step_attach_integration_test.go index 8909241f5..a10e63b65 100644 --- a/internal/purchase/plan_step_attach_integration_test.go +++ b/internal/purchase/plan_step_attach_integration_test.go @@ -83,7 +83,7 @@ func TestAttachIntegration_NewRowAttachesSuppressesAndBecomesNotified(t *testing sender := newCountingSender(ctx) m := &Manager{config: store, email: sender, resolver: &fakeResolver{recs: []config.RecommendationRecord{attachRec("rec-b", 2), attachRec("rec-a", 3)}}} - require.True(t, m.sendPlanNotification(ctx, plan)) + require.True(t, m.sendPlanNotification(ctx, plan, false)) rows := stepRowsInDB(ctx, t, store, plan.ID) require.Len(t, rows, 1) @@ -112,12 +112,12 @@ func TestAttachIntegration_EmailFailureLeavesPendingAndNextTickDoesNotResolveAga sender.On("SendScheduledPurchaseNotification", ctx, mock.Anything).Return(nil).Once() m := &Manager{config: store, email: sender, resolver: res} - require.False(t, m.sendPlanNotification(ctx, plan)) + require.False(t, m.sendPlanNotification(ctx, plan, false)) rows := stepRowsInDB(ctx, t, store, plan.ID) require.Len(t, rows, 1) assert.Equal(t, "pending", rows[0].Status) - require.True(t, m.sendPlanNotification(ctx, plan)) + require.True(t, m.sendPlanNotification(ctx, plan, false)) assert.Equal(t, "notified", stepRowsInDB(ctx, t, store, plan.ID)[0].Status) assert.EqualValues(t, 1, res.calls.Load(), "the second tick adopts the committed row") } @@ -130,7 +130,7 @@ func TestAttachIntegration_NoMatchAndCap(t *testing.T) { t.Run("no match inside the grace window writes no row", func(t *testing.T) { plan := attachPlanInDB(ctx, t, store, "attach-nomatch-early", time.Now().Add(24*time.Hour)) m := &Manager{config: store, email: newCountingSender(ctx), resolver: &fakeResolver{err: planstep.ErrNoMatch}} - require.False(t, m.sendPlanNotification(ctx, plan)) + require.False(t, m.sendPlanNotification(ctx, plan, false)) assert.Empty(t, stepRowsInDB(ctx, t, store, plan.ID)) }) t.Run("cap lowered between list and lock fails the step on the locked plan", func(t *testing.T) { @@ -142,7 +142,7 @@ func TestAttachIntegration_NoMatchAndCap(t *testing.T) { sender := newCountingSender(ctx) m := &Manager{config: store, email: sender, resolver: &fakeResolver{recs: []config.RecommendationRecord{attachRec("rec-a", 5)}}} - require.False(t, m.sendPlanNotification(ctx, &snapshot)) + require.False(t, m.sendPlanNotification(ctx, &snapshot, false)) assert.Equal(t, 1, sender.failedSends, "the failed step is reported once (issue #1195)") rows := stepRowsInDB(ctx, t, store, plan.ID) @@ -158,7 +158,7 @@ func TestAttachIntegration_NoMatchAndCap(t *testing.T) { require.NoError(t, store.SavePurchaseExecution(ctx, root)) m := &Manager{config: store, email: newCountingSender(ctx), resolver: &fakeResolver{recs: []config.RecommendationRecord{attachRec("rec-a", 2)}}} - require.True(t, m.sendPlanNotification(ctx, plan)) + require.True(t, m.sendPlanNotification(ctx, plan, false)) rows := stepRowsInDB(ctx, t, store, plan.ID) require.Len(t, rows, 1, "no second row") @@ -216,7 +216,7 @@ func TestAttachIntegration_PlansOnOneAccountNeverOverAttach(t *testing.T) { } m := &Manager{config: store, email: sender, resolver: res} wg.Add(1) - go func() { defer wg.Done(); m.sendPlanNotification(ctx, plan) }() + go func() { defer wg.Done(); m.sendPlanNotification(ctx, plan, false) }() } wg.Wait() diff --git a/internal/purchase/plan_step_crash_integration_test.go b/internal/purchase/plan_step_crash_integration_test.go index d90e1e9cd..b4ad68f05 100644 --- a/internal/purchase/plan_step_crash_integration_test.go +++ b/internal/purchase/plan_step_crash_integration_test.go @@ -49,7 +49,7 @@ func TestCrash_BeforeCommitPersistsNothing(t *testing.T) { sender := newCountingSender(ctx) m := &Manager{config: suppressionFailingStore{store}, email: sender, resolver: &fakeResolver{recs: []config.RecommendationRecord{attachRec("rec-a", 3)}}} - require.False(t, m.sendPlanNotification(ctx, plan)) + require.False(t, m.sendPlanNotification(ctx, plan, false)) assert.Empty(t, stepRowsInDB(ctx, t, store, plan.ID), "the row insert rolled back with the failed suppressions") assert.Zero(t, suppressedTotal(ctx, t, pool)) @@ -65,13 +65,13 @@ func TestCrash_AfterEmailBeforeNotifiedResendsAndPromotes(t *testing.T) { res := &fakeResolver{recs: []config.RecommendationRecord{attachRec("rec-a", 3)}} m := &Manager{config: &rotateLosingStore{PostgresStore: store}, email: sender, resolver: res} - require.False(t, m.sendPlanNotification(ctx, plan)) + require.False(t, m.sendPlanNotification(ctx, plan, false)) rows := stepRowsInDB(ctx, t, store, plan.ID) require.Len(t, rows, 1) assert.Equal(t, "pending", rows[0].Status, "the scheduler refuses a pending row") assert.Equal(t, 1, sender.sends) - require.True(t, m.sendPlanNotification(ctx, plan), "the next tick re-sends and promotes") + require.True(t, m.sendPlanNotification(ctx, plan, false), "the next tick re-sends and promotes") rows = stepRowsInDB(ctx, t, store, plan.ID) require.Len(t, rows, 1) assert.Equal(t, "notified", rows[0].Status) diff --git a/internal/purchase/plan_step_managed_race_integration_test.go b/internal/purchase/plan_step_managed_race_integration_test.go index 4c6a128d7..0293d547a 100644 --- a/internal/purchase/plan_step_managed_race_integration_test.go +++ b/internal/purchase/plan_step_managed_race_integration_test.go @@ -39,7 +39,7 @@ func TestClaimSkipsPlanTheCommandConvertedWhileItWaited(t *testing.T) { r := newClaimRace(ctx, t) command := testhelpers.HoldCommandFootprint(ctx, t, r.pool, r.plan.ID) done := make(chan bool, 1) - go func() { done <- r.m.sendPlanNotification(ctx, r.plan) }() // r.plan is the stale, unmanaged copy + go func() { done <- r.m.sendPlanNotification(ctx, r.plan, false) }() // r.plan is the stale, unmanaged copy testhelpers.WaitForLockWait(ctx, t, r.pool, "%FOR UPDATE%") command.SetManaged(ctx, t, r.plan.ID) require.NoError(t, command.Commit(ctx)) @@ -52,7 +52,7 @@ func TestClaimSkipsPlanTheCommandConvertedWhileItWaited(t *testing.T) { func TestCommandCaptureSeesRootTheClaimCommittedFirst(t *testing.T) { ctx := context.Background() r := newClaimRace(ctx, t) - require.True(t, r.m.sendPlanNotification(ctx, r.plan)) + require.True(t, r.m.sendPlanNotification(ctx, r.plan, false)) rows := stepRowsInDB(ctx, t, r.store, r.plan.ID) require.Len(t, rows, 1) diff --git a/internal/purchase/plan_step_retention_integration_test.go b/internal/purchase/plan_step_retention_integration_test.go index f1ea65cd4..30b531495 100644 --- a/internal/purchase/plan_step_retention_integration_test.go +++ b/internal/purchase/plan_step_retention_integration_test.go @@ -167,7 +167,7 @@ func TestCumulativeAttach_UntrustedHistoryCannotSizeOrNotify(t *testing.T) { require.NoError(t, f.pool.QueryRow(f.ctx, `SELECT COUNT(*) FROM purchase_suppressions WHERE account_id = 'other-acct'`).Scan(&others)) require.Equal(t, 1, others, "another execution's suppressions are untouched") } - require.False(t, f.m.sendPlanNotification(f.ctx, f.plan())) + require.False(t, f.m.sendPlanNotification(f.ctx, f.plan(), false)) require.Zero(t, calls, "no new sizing or resizing") require.Zero(t, f.m.email.(*countingSender).sends) require.Zero(t, suppressedTotal(f.ctx, t, f.pool)) From 97f5325173a77bef267b4bfcecb964ab39c57932 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 19:37:12 +0100 Subject: [PATCH 3/3] test(api): update #1761's automatic combined-plan test for 4-eyes routing #1771 changes intended behavior: with RequireDifferentApprover on, an AutoPurchase plan's due step waits for a human approval instead of auto-executing. #1761's TestCombinedPlanPurchase_AutomaticOnScheduledExecution turned on 4-eyes for every combined-plan fixture and asserted auto-execution, so it pinned the old behavior and failed in CI. It is updated for the intended change: - The fixture turns 4-eyes on only for the manual (approval) flows, which still exercise the different-approver path. - TestCombinedPlanPurchase_AutomaticOnScheduledExecution now runs with 4-eyes off and still asserts auto-execution. - New TestCombinedPlanPurchase_AutomaticWithFourEyesWaitsForApproval is the end-to-end proof at the API layer. With 4-eyes on, the due sweep executes nothing, fails nothing and buys nothing, and the step stays notified. A different user's approval then buys it once. Part of #1771 Co-Authored-By: Claude Opus 5.5 --- ...combined_plan_purchase_integration_test.go | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/internal/api/combined_plan_purchase_integration_test.go b/internal/api/combined_plan_purchase_integration_test.go index 84d08d9c4..70cb6768b 100644 --- a/internal/api/combined_plan_purchase_integration_test.go +++ b/internal/api/combined_plan_purchase_integration_test.go @@ -143,7 +143,10 @@ func newCombinedPlanFixtureIn(t *testing.T, automatic bool, capAmount *float64, global, err := f.store.GetGlobalConfig(ctx) require.NoError(t, err) recipient := "fixture@example.invalid" - global.NotificationEmail, global.AutoPurchaseEnabled, global.RequireDifferentApprover = &recipient, automatic, true + // 4-eyes is on for the manual (approval) flows only. With 4-eyes on, an + // AutoPurchase step waits for a human approval instead of auto-executing + // (#1771); internal/purchase/four_eyes_auto_plan_integration_test.go covers that. + global.NotificationEmail, global.AutoPurchaseEnabled, global.RequireDifferentApprover = &recipient, automatic, !automatic global.PurchaseDelayHours = 0 require.NoError(t, f.store.SaveGlobalConfig(ctx, global)) response := combinedRoute(t, f.handler, "POST", "/api/plans", PlanRequest{ @@ -384,6 +387,40 @@ func TestCombinedPlanPurchase_NonUSDApprovalRefusedUnderCap(t *testing.T) { f.assertCompleted(t) } +// With 4-eyes on, an AutoPurchase plan's due step is not auto-executed: it +// waits for a human approval, and a different user's approval then buys it +// (#1771). +func TestCombinedPlanPurchase_AutomaticWithFourEyesWaitsForApproval(t *testing.T) { + capAmount := 2400.0 + f := newCombinedPlanFixture(t, true, &capAmount) + f.assertAttached(t) + ctx := context.Background() + global, err := f.store.GetGlobalConfig(ctx) + require.NoError(t, err) + global.RequireDifferentApprover = true + require.NoError(t, f.store.SaveGlobalConfig(ctx, global)) + _, err = f.pool.Exec(ctx, `UPDATE purchase_executions SET scheduled_date=now()-interval '1 minute' WHERE execution_id=$1`, f.root.ExecutionID) + require.NoError(t, err) + + result, err := f.manager.ProcessScheduledPurchases(ctx) + require.NoError(t, err) + assert.Zero(t, result.Executed, "%+v", result) + assert.Zero(t, result.Failed, "%+v", result) + assert.Empty(t, f.service.recorded(), "4-eyes on: the due step must not be bought automatically") + row, err := f.store.GetExecutionByID(ctx, f.root.ExecutionID) + require.NoError(t, err) + assert.Equal(t, "notified", row.Status, "the step waits in the approval queue") + + approver := uuid.NewString() + _, err = f.pool.Exec(ctx, `INSERT INTO users (id,email,password_hash,salt,active,group_ids,created_at,updated_at) SELECT $1,'approver@example.invalid','x','y',true,ARRAY[g.id],now(),now() FROM groups g ORDER BY g.name LIMIT 1`, approver) + require.NoError(t, err) + response := f.approveAs(t, approver, 0) + require.Equal(t, 200, response.StatusCode, response.Body) + f.assertCompleted(t) + assert.Len(t, f.service.recorded(), 1) +} + +// With 4-eyes off (the fixture's automatic default), the due step auto-executes. func TestCombinedPlanPurchase_AutomaticOnScheduledExecution(t *testing.T) { capAmount := 2400.0 f := newCombinedPlanFixture(t, true, &capAmount)