[cristim]

Scope

Platform #631 task C5 (Q7 option A): multi-step ramps do not support Savings Plans.

• PurchasePlan.CheckNoMultiStepSavingsPlans (config/validation.go) is the one check. "Multi-step" = total_steps > 1; "armed" = enabled and auto_purchase; an enabled SP service (common.IsSavingsPlan).
• PurchasePlan.Validate calls it, so create, PUT and PATCH (arm) return 400 multi-step ramps do not support Savings Plans: ... from the shared validation, not three copies.
• planstep.Precheck calls the same check, and attachStep now routes ErrUnsupportedPlan to failStep: one failed row plus notice instead of an error every tick.
• Single-step (total_steps 0 or 1, incl. immediate) SP plans, unarmed plans and valid multi-step non-SP plans are unchanged. Previously unsupported configurations using the same ErrUnsupportedPlan route now record a permanent failed step rather than repeatedly stalling. Resolve sizing, C3 wiring and D1 untouched.

Evidence

Unit tests (mocks, no live DB): handler tests for 400 on create, PUT (mergeUpdatedPlan) and PATCH-arm; 200 for single-step SP, single-step custom SP, multi-step RDS, unarmed multi-step SP; boundary table at total_steps 0/1/2; attach test shows a visible failed row. 8 mutation probes on the new condition, all killed. go build, go vet and tests for config, planstep, purchase, api pass. Fresh independent verification also exercised native PostgreSQL-backed plan handlers and two manager ticks for all five supported SP slugs; the details and fixture/mock boundaries are recorded in the exact-head gate comments.

Active deployment inventory

Completed before the normal merge: authorized read-only AWS DEV query returned zero affected plans. No active prod deployment was found in the actual configured AWS/GCP/Azure scope, as recorded in the gate comments. This does not claim absence across hypothetical unmapped installations. Run the following read-only query before releasing to any subsequently identified prod deployment (also in runbooks/armed-multi-step-savings-plans-plans.md); any hit requires remediation before that deployment:

SELECT p.id, p.name, p.ramp_schedule->>'type' AS ramp_type,
       (p.ramp_schedule->>'total_steps')::int AS total_steps,
       (p.ramp_schedule->>'current_step')::int AS current_step,
       p.next_execution_date, array_agg(s.key) AS sp_services
FROM purchase_plans p
CROSS JOIN LATERAL jsonb_each(p.services) AS s(key, val)
WHERE p.enabled AND p.auto_purchase
  AND COALESCE((p.ramp_schedule->>'total_steps')::int, 0) > 1
  AND COALESCE((s.val->>'enabled')::boolean, false)
  AND (s.val->>'service' = 'savingsplans' OR s.val->>'service' LIKE 'savings-plans-%')
GROUP BY p.id, p.name, p.ramp_schedule, p.next_execution_date;

Refs #631

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 8 billable files and costs up to $2.00.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 2 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 62 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: c50f3473-5c7c-4d25-9ffe-6024dcae385c

📥 Commits

Reviewing files that changed from the base of the PR and between 4446814 and 847f140.

📒 Files selected for processing: 8

• internal/api/handler_plans_sp_multistep_test.go
• internal/config/validation.go
• internal/config/validation_test.go
• internal/planstep/precheck.go
• internal/planstep/precheck_test.go
• internal/purchase/plan_step_attach.go
• internal/purchase/plan_step_attach_test.go
• runbooks/armed-multi-step-savings-plans-plans.md

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Gate verdict: CLEAN at 847f140 (not merged: awaiting the owner's DEV/prod armed-plans query)

Independent adversarial review (money path) of the full diff against task C5 / Q7(A) of #631.

Attacked and how

• New failures for already-armed plans. Only plans that are armed (enabled and auto_purchase), have total_steps > 1 AND an enabled SP service (any of savingsplans, savings-plans-{compute,ec2instance,sagemaker,database} per common.IsSavingsPlan) now get a failed row. That includes the mixed SP+RDS case (the whole plan fails) and plans mid-ramp (the next step fails). This is the intended Q7 scope. Single-step SP (total_steps 0 or 1, immediate), plans with a disabled SP service, unarmed plans and multi-step non-SP plans are unchanged. total_steps 0 is not a bypass: IsComplete() is already true at step 0, so CompletePlanStep never advances and only one step runs (store_postgres.go:723,784; the ramp CTE also uses > 1). An immediate type with total_steps > 1 is refused. That is conservative and correct, because the ramp would advance through every step.
• Routing ErrUnsupportedPlan to failStep also changes the existing filter and immediate-percent refusals, plus checkStoredSPCount (resolve.go:357, stored SP row with count != 1). Today all of these stall: no purchase and an error every tick. After this PR they get one failed row with NoSubmission=true (notifications.go:266, so the C2 ledger does not count it), the step is blocked, and nothing is bought either way. Visible is better than a silent stall. Non-blocking note: a stored-SP-count anomaly is data, not plan config. Today it could clear on a later recommendation refresh; now it needs operator action (item 72: no automatic retry).
• Arm paths: create, PUT (mergeUpdatedPlan) and PATCH call Validate. The only other plan writes are disable, the next-execution-date bump and the ramp advance, and none of them arms a plan. Any bypass is still caught at attach, because Resolve calls Precheck first (resolve.go:88) and the adapter returns the error unwrapped. Disarming a grandfathered plan by PATCH still passes Validate.
• Mutation probes: 10 of 10 killed. Probes: Validate skips the check; <=1 changed to <=2 and to <1; the auto_purchase guard dropped; the enabled guard dropped; the svc.Enabled guard dropped; IsSavingsPlan replaced by an exact "savingsplans" match; Precheck skips the check; attach routing removed; attach routed to noMatch. The first probe stands in for fails-before on the parent: the API 400 tests fail without the check.
• Runbook query: run under BEGIN READ ONLY on a throwaway local PG17 (865, all 118 up-migrations applied, cluster removed afterwards) against 9 fixture plans. It returned exactly the 2 expected hits (4-step SP mid-ramp; 2-step compute SP + RDS listing only the SP key). Rows with 1 or 0 steps, a disabled SP service, an unarmed plan, RDS only, a missing total_steps or a draft with services = null were excluded, and none of them raised an error. Table and column names match 000001_initial_schema and the JSON tags. runbooks/ is an existing convention (2 runbooks), and the spec (C5 iii) asked for this runbook.

Local evidence (git archive of the head, GOTOOLCHAIN=go1.26.9): go build ./..., go vet, and go test for config, planstep, purchase and api passed (4588 tests). This is mock- and fixture-based evidence. Local integration tests were not run because Docker is unavailable here; the CI Integration Tests job passed at this SHA.

CI: all checks green at 847f140, mergeStateStatus CLEAN, 0 commits behind main.

Nits (non-blocking): the runbook filename doubles a word (...-savings-plans-plans.md); "7 days" in the comment and runbook is the default grace, which is configurable.

[cristim]

Independent Sol-medium adversarial review, two passes, exact SHA 847f140: no actionable source findings.

Checked the enabled + auto_purchase + total_steps > 1 + enabled common.IsSavingsPlan predicate against the pinned common model (five exact supported service slugs), create/PUT/PATCH validation reachability, Resolve precheck ordering, error identity, permanent step failure before attachment/suppression, supported single-step/unarmed/non-SP controls, security, duplication and scope.

Fresh macOS local evidence at the exact revision: Go 1.26.9 go build ./... passed. Config, planstep, purchase and API package tests passed. The initial purchase test attempt was blocked by sandbox localhost socket permissions; the socket-authorized rerun passed. Focused race checks passed for config and planstep. Purchase/API race builds were blocked by no space left on device in the Go temporary build directory, so no race verdict is claimed for those packages. Scratch server stopped after verification; data and logs are preserved.

Independent fixture evidence on native PostgreSQL 17, migrated by the exact production migrations to schema 118: actual create rejects armed multi-step SP with domain HTTP 400 and leaves the row count unchanged; supported create controls persist three rows. Actual PUT rejects changing an immediate SP plan to armed multi-step and stored total_steps remains 1. Actual PATCH refuses arming a multi-step SP and stored enabled/auto_purchase remain false; single-step SP and multi-step RDS PATCH controls succeed. Authentication is mocked; handlers and PostgreSQL store are production code.

Actual Manager.SetRecommendationSource -> Resolve -> Precheck -> failStep -> PostgreSQL persistence was exercised for all five pinned SP service slugs. Two ticks per plan leave exactly one failed row with NoSubmission=true, the SP rejection reason, empty recommendations, no token/rotation, no recommendation-source call and no ramp advance. Inputs and the strict recommendation-source boundary are synthetic fixtures; no provider or email call occurs. This is native database evidence, not a live provider test or live environment write.

Mutation probe: deleting only the ErrUnsupportedPlan -> failStep branch in the scratch copy makes the native test fail because the unsupported error propagates instead of returning the quiet failed-step sentinel. The branch was restored immediately. All 913 tracked files under internal/, cmd/, go.mod and go.sum were byte-compared with the reviewed commit afterwards: zero differences. The archive-only independent harnesses are not production changes.

Scope caveats: previously attached nonempty pending/notified roots can bypass Resolve/Precheck through adoption, so this proof covers new attachment and does not establish blanket prevention of purchases from every preexisting execution. The owner inventory/remediation gate before deployment remains necessary. Valid multi-step non-SP behavior is preserved; previously unsupported non-SP configurations now fail permanently under the shared ErrUnsupportedPlan route.

Separate owner-authorized live DEV read-only evidence must remain labelled separately from these fixtures. Production inventory/remediation, current CI and final merge/deployment policy are separate gates; this local code review does not authorize a purchase or deployment.

Authorized live AWS DEV inspection completed separately: schema118 dirty=false, valid unique11-column recommendation arbiter, and armed multi-step Savings Plans aggregate count0. The read-only transaction ended with ROLLBACK and the owned SSM session was cleaned up.

Active-prod discovery: configured AWS account/region runtime inventory has0 matching production Lambda/RDS/ECS resources; known production Terraform state prefixes contain0 objects; GitHub production deployment records and releases are empty. The configured GCP project is freshly confirmed DELETE_REQUESTED. No actual Azure production backend/resource locator was established, so cross-cloud absence is not inferred.

[cristim]

Active-project inventory gate satisfied at exact PR head 847f140.

Authorized read-only AWS DEV database inspection found zero armed multi-step Savings Plans; schema118 is clean. The query covered all five service slugs recognized by the pinned common.IsSavingsPlan model. No remediation or purchase was performed.

Known active production scope: no matching AWS production Lambda/RDS/ECS resources in the configured project account/region; zero objects under its known production Terraform state prefixes; no GitHub production deployment records or releases. The configured GCP project is DELETE_REQUESTED. Azure's configured cudly-prod namespace has zero resource groups/resources in the existing authenticated profile. GCP and Azure deployment workflows are disabled. No active project production deployment was identified. This statement is scoped to those actual configured routes; it does not claim absence across hypothetical unmapped installations or unrelated subscriptions. If a production deployment is later identified, run the documented read-only inventory before its release and resolve any positive result.

Fresh independent Sol-medium two-pass source review and native PostgreSQL/API/manager verification are recorded above. All five SP slugs failed one unattached step on two ticks without recommendations, submission, token, rotation, or ramp advancement. Source byte comparison matched all913 inspected tracked files. Existing attached executions remain outside this proof, which is why the actual deployment inventory mattered. Local purchase/API race compilation was limited by disk exhaustion; normal package tests and config/planstep race tests passed. No cloud purchase, forced deployment, or existing database write is authorized or performed.

Normal merge requires the unchanged named head, current CLEAN/MERGEABLE status, successful required checks, and the existing repository lock. CodeRabbit is optional under the standing independent-review policy; no check or approval bypass is permitted.