From df6dc5cc7a5c8d3b8a8801fbf8d2c5757bd669f6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 14:23:22 +0100 Subject: [PATCH] fix(ladder): plan with a capability that can never buy ladder_run wired the ladder capability's write side from the global ladder_execution_enabled flag, so with both ladder switches on the planner held a capability with real purchase and exchange clients, even though planning only reads. The planner now builds its capability through planningCapability, which takes no flag and always wires the disabled write side; the flag no longer flows into the planning path. The real-client branch of wireLadderWriteSide and the exchange adapter stay for the sweep that reshapes. A test builds the planning capability from the real AWS ladder and checks that PurchaseLayer returns ErrLadderExecutionDisabled. It blanks every AWS credential source first, so a regression that wired real clients fails on credentials instead of buying from a developer machine. Part of #70 Co-Authored-By: Claude Opus 5.5 --- internal/config/types.go | 14 +++---- internal/server/handler_ladder.go | 10 ++--- .../server/handler_ladder_inflight_test.go | 2 +- internal/server/handler_ladder_test.go | 6 +-- internal/server/ladder_compute_cap_test.go | 2 +- internal/server/ladder_write.go | 14 +++---- internal/server/ladder_write_test.go | 40 +++++++++++++++++++ 7 files changed, 63 insertions(+), 25 deletions(-) diff --git a/internal/config/types.go b/internal/config/types.go index 99c7f8982..82e7f9296 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -72,13 +72,13 @@ type GlobalConfig struct { // Set to true to allow per-account configs to activate individually. LadderingEnabled bool `json:"laddering_enabled" db:"laddering_enabled"` - // LadderExecutionEnabled gates the write side of the ladder capability - // (migration 000083). BOTH LadderingEnabled AND LadderExecutionEnabled - // must be true for PurchaseLayer / ReshapeBuffer to be wired with real - // AWS SDK clients. Default false: existing deployments that enable - // laddering produce plans but never call AWS purchase APIs until an - // operator explicitly opts in. Fail-loud: wireLadderWriteSide returns - // a typed ErrLadderExecutionDisabled when this is false. + // LadderExecutionEnabled gates ladder purchases and reshapes (migration + // 000083). BOTH LadderingEnabled AND LadderExecutionEnabled must be true + // for a sweep that buys or reshapes to act. Planning (ladder_run) never + // reads it: its capability always has the disabled write side, which + // returns a typed ErrLadderExecutionDisabled. Default false: deployments + // that enable laddering produce plans but never call AWS purchase APIs + // until an operator explicitly opts in. LadderExecutionEnabled bool `json:"ladder_execution_enabled" db:"ladder_execution_enabled"` // AutoPurchaseEnabled is the global kill switch for AutoPurchase plans diff --git a/internal/server/handler_ladder.go b/internal/server/handler_ladder.go index 5b86b13c8..d9d997f89 100644 --- a/internal/server/handler_ladder.go +++ b/internal/server/handler_ladder.go @@ -130,7 +130,7 @@ func (app *Application) handleLadderRun(ctx context.Context) (*LadderRunResult, } now := time.Now().UTC() - result := app.runLadderConfigs(ctx, allConfigs, ownAccountID, region, term, paymentOpt, now, globalCfg.LadderExecutionEnabled) + result := app.runLadderConfigs(ctx, allConfigs, ownAccountID, region, term, paymentOpt, now) log.Printf("ladder_run done: planned=%d skipped_cadence=%d skipped_disabled=%d skipped_multi_account=%d errored=%d", result.Planned, result.SkippedCadence, result.SkippedDisabled, result.SkippedMultiAccount, result.Errored) @@ -198,11 +198,10 @@ func (app *Application) runLadderConfigs( term pkgladder.Term, paymentOpt pkgladder.PaymentOption, now time.Time, - executionEnabled bool, ) *LadderRunResult { result := &LadderRunResult{} for i := range configs { - result.record(app.processOneLadderConfig(ctx, &configs[i], ownAccountID, region, term, paymentOpt, now, executionEnabled)) + result.record(app.processOneLadderConfig(ctx, &configs[i], ownAccountID, region, term, paymentOpt, now)) } return result } @@ -218,7 +217,6 @@ func (app *Application) processOneLadderConfig( term pkgladder.Term, paymentOpt pkgladder.PaymentOption, now time.Time, - executionEnabled bool, ) ladderConfigOutcome { if !dbCfg.Enabled { log.Printf("ladder_run: config %s: enabled=false, skipping", dbCfg.ID) @@ -256,8 +254,8 @@ func (app *Application) processOneLadderConfig( return outcomeSkippedCadence } - // Build and wire the LadderCapability for this account. - capability, err := app.buildAndWireCapability(ctx, region, cloudAcct.ExternalID, executionEnabled) + // Build the planning capability for this account: its write side is always disabled. + capability, err := app.planningCapability(ctx, region, cloudAcct.ExternalID) if err != nil { log.Printf("ladder_run: config %s: %v", dbCfg.ID, err) return outcomeErrored diff --git a/internal/server/handler_ladder_inflight_test.go b/internal/server/handler_ladder_inflight_test.go index f3fcc80f1..00b3c5693 100644 --- a/internal/server/handler_ladder_inflight_test.go +++ b/internal/server/handler_ladder_inflight_test.go @@ -125,7 +125,7 @@ func TestRunLadderConfigs_InFlightFetchError_CountsErrored(t *testing.T) { }, } - result := app.runLadderConfigs(ctx, []config.LadderConfigDB{dbCfg}, inFlightTestAccount, "us-east-1", pkgladder.Term1Year, pkgladder.PaymentNoUpfront, now, false) + result := app.runLadderConfigs(ctx, []config.LadderConfigDB{dbCfg}, inFlightTestAccount, "us-east-1", pkgladder.Term1Year, pkgladder.PaymentNoUpfront, now) assert.Equal(t, LadderRunResult{Errored: 1}, *result, "an in-flight fetch error must count the config Errored and nothing else") assertNothingPersistedOrSubmitted(t, store, fakeCap) diff --git a/internal/server/handler_ladder_test.go b/internal/server/handler_ladder_test.go index 608c67334..ea3574dc1 100644 --- a/internal/server/handler_ladder_test.go +++ b/internal/server/handler_ladder_test.go @@ -540,7 +540,7 @@ func TestHandleLadderRun_MultiAccountSkip_CountedAndIsolated(t *testing.T) { // Put the foreign config first to prove isolation is order-independent. configs := []config.LadderConfigDB{cfgForeign, cfgHealthy} - result := app.runLadderConfigs(ctx, configs, ownAccount, "us-east-1", pkgladder.Term1Year, pkgladder.PaymentNoUpfront, now, false) + result := app.runLadderConfigs(ctx, configs, ownAccount, "us-east-1", pkgladder.Term1Year, pkgladder.PaymentNoUpfront, now) require.NotNil(t, result) // (a) The foreign config must be counted as SkippedMultiAccount. @@ -981,7 +981,7 @@ func TestProcessOneLadderConfig_CadenceDBError_Errored(t *testing.T) { }, } - result := app.runLadderConfigs(ctx, []config.LadderConfigDB{dbCfg}, ownAccount, "us-east-1", pkgladder.Term1Year, pkgladder.PaymentNoUpfront, now, false) + result := app.runLadderConfigs(ctx, []config.LadderConfigDB{dbCfg}, ownAccount, "us-east-1", pkgladder.Term1Year, pkgladder.PaymentNoUpfront, now) assert.Equal(t, 1, result.Errored, "a cadence lookup error must count the config Errored") assert.Equal(t, 0, result.Planned) @@ -1105,7 +1105,7 @@ func TestHandleLadderRun_MultiConfigIsolation(t *testing.T) { // Order the broken config first to prove a leading failure does not abort // the healthy config that follows. configs := []config.LadderConfigDB{cfgBroken, cfgHealthy} - result := app.runLadderConfigs(ctx, configs, ownAccount, "us-east-1", pkgladder.Term1Year, pkgladder.PaymentNoUpfront, now, false) + result := app.runLadderConfigs(ctx, configs, ownAccount, "us-east-1", pkgladder.Term1Year, pkgladder.PaymentNoUpfront, now) require.NotNil(t, result) assert.Equal(t, 1, result.Planned, "the healthy config must still be planned despite the broken one") diff --git a/internal/server/ladder_compute_cap_test.go b/internal/server/ladder_compute_cap_test.go index 8db44c6c4..9e58e5276 100644 --- a/internal/server/ladder_compute_cap_test.go +++ b/internal/server/ladder_compute_cap_test.go @@ -94,7 +94,7 @@ func runComputeCapConfig(t *testing.T, cap *float64) (*LadderRunResult, *persist dbCfg := validTestDBConfig("cfg-compute-cap") dbCfg.MaxHourlyCommitPerRun = cap result := app.runLadderConfigs(testutil.TestContext(t), []config.LadderConfigDB{dbCfg}, computeCapAccount, "us-east-1", - pkgladder.Term1Year, pkgladder.PaymentNoUpfront, ratesTestNow, false) + pkgladder.Term1Year, pkgladder.PaymentNoUpfront, ratesTestNow) return result, store, reads } diff --git a/internal/server/ladder_write.go b/internal/server/ladder_write.go index 3fc709f7e..bdda39c2f 100644 --- a/internal/server/ladder_write.go +++ b/internal/server/ladder_write.go @@ -95,12 +95,12 @@ func (a *exchangeRunnerAdapter) run(ctx context.Context, cfg exchange.RIExchange }) } -// buildAndWireCapability constructs a LadderCapability via the factory and wires its -// write side. Extracted from processOneLadderConfig to keep that function's cyclomatic -// complexity below the project threshold (10). The returned error carries no config -// ID: the sole caller already prefixes its log line with the config ID, so repeating -// it here would duplicate it in the output. -func (app *Application) buildAndWireCapability(ctx context.Context, region, accountID string, executionEnabled bool) (pkgladder.LadderCapability, error) { +// planningCapability builds the capability ladder_run plans with. Planning never +// buys, so its write side is always the disabled one, whatever +// ladder_execution_enabled says; it takes no flag, so the planner cannot ask for +// real clients (#70). The returned error carries no config ID: the caller +// already prefixes its log line with it. +func (app *Application) planningCapability(ctx context.Context, region, accountID string) (pkgladder.LadderCapability, error) { if app.LadderCapabilityFactory == nil { return nil, errors.New("LadderCapabilityFactory is nil (not wired)") } @@ -108,7 +108,7 @@ func (app *Application) buildAndWireCapability(ctx context.Context, region, acco if err != nil { return nil, fmt.Errorf("failed to build ladder capability: %w", err) } - return app.wireLadderWriteSide(ctx, executionEnabled, region, accountID, capability) + return app.wireLadderWriteSide(ctx, false, region, accountID, capability) } // wireLadderWriteSide wires the write side of a LadderCapability if and only if diff --git a/internal/server/ladder_write_test.go b/internal/server/ladder_write_test.go index 95412cb8a..5dcbce71b 100644 --- a/internal/server/ladder_write_test.go +++ b/internal/server/ladder_write_test.go @@ -88,3 +88,43 @@ func TestWireLadderWriteSide_ExecutionDisabled_RealAWSLadder_RefusesWithoutAWSCa assert.ErrorIs(t, reshapeErr, awsladder.ErrLadderExecutionDisabled, "kill-switch OFF must refuse ReshapeBuffer with ErrLadderExecutionDisabled, not an AWS error") } + +// TestPlanningCapability_RealAWSLadder_RefusesPurchases pins that ladder_run plans +// with a capability whose write side is always the disabled one. Planning must +// never hold a purchase-capable capability, whatever ladder_execution_enabled +// says; only the reshape sweep may wire real clients (#70 S3). +func TestPlanningCapability_RealAWSLadder_RefusesPurchases(t *testing.T) { + // No credentials can be found, so a regression that wired real clients + // fails on a credentials error instead of buying with the caller's account. + // t.Setenv rules out t.Parallel. + for k, v := range map[string]string{ + "AWS_CONFIG_FILE": "/dev/null", "AWS_SHARED_CREDENTIALS_FILE": "/dev/null", + "AWS_PROFILE": "", "AWS_ACCESS_KEY_ID": "", "AWS_SECRET_ACCESS_KEY": "", + "AWS_SESSION_TOKEN": "", "AWS_EC2_METADATA_DISABLED": "true", + "AWS_WEB_IDENTITY_TOKEN_FILE": "", "AWS_ROLE_ARN": "", + "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI": "", "AWS_CONTAINER_CREDENTIALS_FULL_URI": "", + "AWS_CONTAINER_AUTHORIZATION_TOKEN": "", + } { + t.Setenv(k, v) + } + ctx := context.Background() + const ( + region = "us-east-1" + accountID = "123456789012" + ) + app := &Application{LadderCapabilityFactory: func(ctx context.Context, region, accountID string) (pkgladder.LadderCapability, error) { + return awsladder.NewFromAWSConfig(ctx, region, accountID) + }} + capability, err := app.planningCapability(ctx, region, accountID) + require.NoError(t, err) + + rec := pkgcommon.Recommendation{ + ResourceType: "m5.large", + Count: 1, + Term: "1yr", + PaymentOption: "no-upfront", + Details: &pkgcommon.ComputeDetails{InstanceType: "m5.large", Platform: "linux", Tenancy: "default", Scope: "regional"}, + } + _, purchaseErr := capability.PurchaseLayer(ctx, pkgladder.LayerConvertibleRI, rec, pkgcommon.PurchaseOptions{Source: pkgcommon.PurchaseSourceWeb, IdempotencyToken: "ladder-plan-tok"}) + assert.ErrorIs(t, purchaseErr, awsladder.ErrLadderExecutionDisabled, "the planning capability must refuse every purchase") +}