Skip to content

feat(purchase): per-plan step commitment cap, auto-purchase kill switch and notified veto - #825

Merged
cristim merged 2 commits into
mainfrom
feat/631-pr2c-step-cap
Oct 12, 2026
Merged

cristim merged 2 commits into
mainfrom
feat/631-pr2c-step-cap

Conversation

@cristim

@cristim cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member

Refs #631. Safety layer that must merge BEFORE the wiring PR (PR2b-2). It only adds refusals: nothing here makes a plan step buy by itself, and no attach/wiring is included.

What it does

  • Per-plan cap max_step_commitment (nullable column on purchase_plans, migration 000112). NULL = refuse auto-purchase for recommendation-bearing steps (fail closed; costs existing plans nothing since none can buy today). Measured on TOTAL commitment (RecommendationRecord.TotalCommitment() = upfront + monthly * term * 12, moved from api.recTotalCommitment, which now delegates), so Azure monthly recs (upfront 0) are counted. Zero, negative and non-finite commitments are refused.
  • purchase.CheckStepCommitmentCap(plan, recs) error: the function the future attach step will call (fail the step, ramp unchanged). Enforced now, before the claim, in executableByScheduler, which both processOneExecution and the SQS checkAutoExecuteGate call. A cap violation surfaces as a failure (scheduler Failed, SQS dead-letter) and leaves the row untouched.
  • Kill switch: the scheduling.auto_purchase default was never read. It is now global_config.auto_purchase_enabled (default FALSE, GlobalConfig.AutoPurchaseEnabled), checked in shouldNotifyPlan (via the tick) and in the scheduler/SQS gate. A read error fails closed. Only AutoPurchase plans reach these checks.
  • Veto: a recommendation-bearing plan row must be notified to auto-execute on the scheduler and SQS paths (pending is refused). Rows with no recommendations are unchanged (still failed by the existing no-recommendations refusal).
  • API/UI plumbing (tiny): PlanRequest.max_step_commitment (validated > 0 and finite in PurchasePlan.Validate), kept on PUT when omitted, openapi updated. No UI work (follow-up).

Current state to expose (read this)

  • Cap: every existing plan has max_step_commitment = NULL after migration, i.e. auto-purchase of recommendation-bearing steps is refused for all plans until an operator sets a cap.
  • Kill switch: auto_purchase_enabled = FALSE for all deployments after migration. There is no API/UI to flip it yet (follow-up); until then it is set directly in global_config. The PR2b-2 DEV acceptance needs this set to true and a manual /api/scheduled call (no Terraform schedule fires process_scheduled_purchases; only the CloudFormation template does).
  • approved rows (human approval path, RecoverStrandedApprovals) and manual purchases from Opportunities do not go through this gate and are unaffected (tested).

Migration number

000112 = max+1 of origin/main at the time of opening (latest on main: 000111). It renumbers if another migration lands first (the "Check for conflicting migration numbers" hook will flag it). The migration is additive only: one nullable purchase_plans.max_step_commitment NUMERIC(18,2) with a > 0 CHECK, and global_config.auto_purchase_enabled BOOLEAN NOT NULL DEFAULT FALSE (metadata-only on PG 11+). The prod migration workflow applies ALL pending migrations by default (owner item 65), so this ships with the next prod apply; being additive and fail-closed, applying it early is safe.

Behaviour changes worth a reviewer's eye

  • Existing tests that drove pending rec-bearing plan rows through the scheduler/SQS were updated to notified + cap + kill switch on. The TestPauseClaim_* integration tests used pending rows: pause only applies to pending and resume returns to pending, and the veto now refuses a pending row, so a resumed row waits for a notification before it can auto-execute. TestPauseClaim_ConcurrentPauseAndWorker was reshaped accordingly (the pause-vs-claim race no longer exists for rec-bearing plan rows on the auto path).
  • Item (10): ProcessScheduledPurchases fails an empty (rec-less) plan row rather than running it, now pinned for NotificationDaysBefore 0 and 7.
  • Cap-refused rows stay notified and are retried (and re-reported as failures) every tick until the cap is fixed or the row is cancelled. That is the pure-refusal choice; failing the row is a possible follow-up.

Evidence

Labels: fixture/mock = hand-built records and MockConfigStore/pgxmock, no real cloud data.

  • Unit (fixture/mock): every refusal on both paths: NULL cap, over cap, sum over cap, zero/negative/NaN commitment, NaN/Inf/zero/negative cap, Azure-monthly counted on total, kill switch off, kill switch read error (also with a populated config), nil config, pending status; each asserts TransitionExecutionStatus (the claim) is never called. Manual/approved row bypass and non-AutoPurchase / web rows never read the kill switch. Kill switch in shouldNotifyPlan and the notification tick.
  • Integration, real PostgreSQL 17.11 (throwaway local server; testhelper override reverted, not in this diff): migration 000112 up/down, existing plan gets NULL cap, switch defaults off, CHECK rejects 0 and negatives; store round trip of cap (create/get/update/list) and switch; internal/purchase, internal/config, internal/api pause-claim and migrations suites pass with -tags integration.
  • Mutation pass (19 mutants on the cap / kill switch / status / notification lines): 18 killed on first run; the survivor (err != nil read check in autoPurchaseEnabled) was an equivalent-mutant gap, closed with a read-error-with-populated-config case and re-killed. One of the 19 (2**ptr) is a compile error counted as killed.
  • make lint 0 issues; go test ./... green; git-secrets scan clean under BSD and GNU grep 3.12 with --register-aws; pre-commit hooks pass.

Not verified here: Linux (CI), Windows (out of scope), and any real purchase (verification never authorizes one).

Known gaps / follow-ups

  • No UI or API for the kill switch; no UI field for the cap.
  • Size: ~200 non-test lines; the rest is tests and the fixture updates above.

🤖 Generated with Claude Code

…ch and notified veto

Safety layer for plan-step auto-purchase (platform#631 PR2c). It only adds
refusals; nothing here makes a plan step buy by itself.

- max_step_commitment (nullable, migration 000112) on purchase_plans. NULL
  refuses auto-purchase of recommendation-bearing steps. The cap is measured
  on total commitment (upfront + monthly * term * 12), and zero-commitment
  recommendations are refused. CheckStepCommitmentCap is enforced before the
  claim on the scheduler and SQS execute paths.
- global_config.auto_purchase_enabled (default false) backs the previously
  unread scheduling.auto_purchase default as a fail-closed kill switch, read
  in shouldNotifyPlan and the scheduler/SQS gates. Manual and approved
  purchases are unaffected.
- Recommendation-bearing plan rows must be "notified" to auto-execute, so a
  row that never got its notification email cannot buy.
- PlanRequest.max_step_commitment (validated > 0) and openapi.

Refs #631

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim cristim added priority/p1 Next up; this sprint severity/high Significant harm urgency/this-sprint Within the current sprint impact/many Affects most users effort/xl Multi-week / refactor type/feat New capability triaged Item has been triaged labels Oct 12, 2026
@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 30 billable files and costs up to $7.50.

  • 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 1 minute for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 60 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: e8156edf-a25f-4946-a3fa-b8e7aa9346aa

📥 Commits

Reviewing files that changed from the base of the PR and between ca01672 and 5713e4f.


📒 Files selected for processing (30)
  • internal/api/handler_plans.go
  • internal/api/handler_plans_step_cap_test.go
  • internal/api/handler_purchases.go
  • internal/api/handler_purchases_pause_claim_integration_test.go
  • internal/api/openapi.yaml
  • internal/api/types.go
  • internal/config/commitment.go
  • internal/config/store_postgres.go
  • internal/config/store_postgres_complete_step_test.go
  • internal/config/store_postgres_coverage_test.go
  • internal/config/store_postgres_db_test.go
  • internal/config/store_postgres_pgxmock_test.go
  • internal/config/store_postgres_plan_update_test.go
  • internal/config/types.go
  • internal/config/validation.go
  • internal/database/postgres/migrations/000112_plan_step_cap_auto_purchase_switch.down.sql
  • internal/database/postgres/migrations/000112_plan_step_cap_auto_purchase_switch.up.sql
  • internal/database/postgres/migrations/000112_plan_step_cap_auto_purchase_switch_test.go
  • internal/purchase/coverage_extra_test.go
  • internal/purchase/manager.go
  • internal/purchase/manager_test.go
  • internal/purchase/messages.go
  • internal/purchase/money_path_regression_test.go
  • internal/purchase/notifications.go
  • internal/purchase/notifications_test.go
  • internal/purchase/plan_step_no_recs_test.go
  • internal/purchase/ramp_step_progress_integration_test.go
  • internal/purchase/step_cap.go
  • internal/purchase/step_cap_helpers_test.go
  • internal/purchase/step_cap_test.go

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

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

@cristim

cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member Author

Independent gate review (Opus, claude-opus-5-5): APPROVE at 5713e4f

Reviewed ac24de1, then brought the branch up to date with gh pr update-branch (merge, which brought in #824). git diff ac24de10 5713e4f1 on this PR's 30 files is empty; only ladder files from main changed. All evidence below is from the new head.

Checked against amendments 6, 7, 8 and 10 (631-amendments.md)

  • Cap (6): CheckStepCommitmentCap measures the TOTAL commitment (RecommendationRecord.TotalCommitment, upfront + monthlyterm12; api.recTotalCommitment now delegates to it). NULL, 0, negative, NaN or Inf caps refuse. Zero, negative and NaN rec commitments refuse. Commitments are summed across all recs and accounts in the row, and a total exactly equal to the cap passes (>). It is enforced in executableByScheduler, which runs before the claim on both processOneExecution and the SQS checkAutoExecuteGate.
  • Kill switch (7): global_config.auto_purchase_enabled, default FALSE. It is read in the notification tick (shouldNotifyPlan) and in the scheduler/SQS gate. A read error or a nil config fails closed.
  • Veto (8): on the auto paths, a rec-bearing plan row must be notified; pending is refused.
  • Item (10): an empty plan row is failed, not run, with NotificationDaysBefore 0 and 7.
  • Paths that bypass this gate, by design: approved rows (checkAutoExecuteGate returns nil before the gate; TestAutoExecuteGate_ApprovedRowIgnoresKillSwitchAndCap), web-sourced rows (direct execute and retry successors are cudly-web + pending and need the token link), and manual Opportunities purchases.
  • No wiring: AttachResolvedRecommendations and internal/planstep have no non-test callers. Nothing here can make a plan step buy.

Local evidence (macOS, git archive of 5713e4f, go1.26.9, GOMAXPROCS=2, -p 2)

  • go build ./... passed. Unit tests for internal/purchase/... internal/config/... internal/api/... internal/database/... exit 0 (fixture/mock).
  • Real PostgreSQL 17.11, on a throwaway local initdb server. Docker was unavailable, so the testhelper was overridden in scratch only (not in the repo). With -tags integration, internal/purchase, internal/config, internal/database/postgres/migrations (the cancel tests used CUDLY_TEST_ADMIN_DSN) and the api PauseClaim tests all pass. TestMigration_PlanStepCapAndAutoPurchaseSwitch passes: the change is additive, an existing plan gets a NULL cap, the switch defaults to off, the CHECK rejects 0 and -5, and down drops both columns.
  • Original PauseClaim tests (origin/main) against the new code: RunningWorkerCannotBeReclaimed, PausedQueueResumesExactlyOnce and ConcurrentPauseAndWorker fail, each with "not eligible for auto-execution ... not yet notified" (the gate refuses pending rows). RejectsNonPendingStates passes. The changes to these tests follow directly from the veto. Pause applies only to pending, and pending rec-bearing plan rows can no longer be claimed on the auto path. Pending web rows were already refused, and approved and notified rows cannot be paused, so the auto path has no remaining pause-vs-claim race. The CAS in TransitionExecutionStatus is unchanged.
  • Mutation pass (one run, 19 mutants on the cap, kill-switch, status, notification, validation and merge lines): 17 killed. 2 survivors, both non-blocking:
    1. step_cap.go: dropping the per-rec IsInf(c) check. Equivalent mutant: +Inf makes the total Inf, which is greater than the cap, so the step is still refused.
    2. validation.go: !(cap > 0) changed to cap < 0. Test gap only: a 0 cap is still refused by the DB CHECK and by CheckStepCommitmentCap.
  • git ls-files -z | xargs -0 git secrets --scan is clean under BSD grep and GNU grep 3.12.
  • CI: all checks green at 5713e4f, including Integration Tests. mergeStateStatus CLEAN.

Non-blocking findings and corrections

  1. PR body correction: the kill switch CAN be flipped through the API today. PUT /api/config (AuthAdmin, update:config, unrestricted scope) unmarshals the body over GlobalConfig (handler_config.go ~97, inside UpdateGlobalConfigAtomic), and the struct has the json tag auto_purchase_enabled. Unlike the RI-exchange auto mode (requireRIExchangeAutoModeGrant), arming it needs no dedicated grant. On its own this cannot buy (NULL cap, no wiring yet). The owner should decide whether arming needs its own grant before PR2b-2 merges.
  2. Required before PR2b-2 (wiring): the gate checks a pre-claim snapshot, and claimAndExecute then replaces it with the claimed row (*exec = *updated, manager.go ~193). The rec-less early return (len(exec.Recommendations) == 0 -> true) skips the cap and the veto. SetExecutionRecommendationsIfEmptyTx allows attaching to a notified row. So once attach is wired, a rec-less notified row can pass the gate, get recs attached, be claimed, and buy without a cap check. PR2b-2 must enforce the cap in AttachResolvedRecommendations (amendment 6, "at attach", not done in this PR) and re-check the cap and veto on the claimed row after the claim.
  3. Cap-refused rows stay notified and count as Failed every tick. Nothing is emailed, and internal/server/handler.go ~249 does not log result.Errors, so refusals are quiet rather than spammy, but operators cannot see them. A follow-up could make the refusal terminal or visible.
  4. NUMERIC(18,2) vs validation: a cap such as 0.001 passes Validate but rounds to 0.00, the CHECK rejects it, and the request returns a 500 instead of a 400. Rejecting it fails closed, so this is minor.

Migration order

000112 is max+1 on main (latest is 000111). #821 also uses 000112. If this PR merges first, #821 must be renumbered to 000113 before it merges. Prod applies all pending migrations by default, in this order: 000112 (this PR, additive and fail-closed), then #821 as 000113.

@cristim
cristim merged commit ab34f48 into main Oct 12, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xl Multi-week / refactor impact/many Affects most users priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/feat New capability urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant