Repository navigation
feat(purchase): per-plan step commitment cap, auto-purchase kill switch and notified veto - #825
Conversation
…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>
|
Warning Review limit reached
This review includes 30 billable files and costs up to $7.50.
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. View limit details
Comment |
Independent gate review (Opus, claude-opus-5-5): APPROVE at 5713e4fReviewed ac24de1, then brought the branch up to date with Checked against amendments 6, 7, 8 and 10 (631-amendments.md)
Local evidence (macOS, git archive of 5713e4f, go1.26.9, GOMAXPROCS=2, -p 2)
Non-blocking findings and corrections
Migration order
|
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
max_step_commitment(nullable column onpurchase_plans, migration000112). 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 fromapi.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, inexecutableByScheduler, which bothprocessOneExecutionand the SQScheckAutoExecuteGatecall. A cap violation surfaces as a failure (schedulerFailed, SQS dead-letter) and leaves the row untouched.scheduling.auto_purchasedefault was never read. It is nowglobal_config.auto_purchase_enabled(default FALSE,GlobalConfig.AutoPurchaseEnabled), checked inshouldNotifyPlan(via the tick) and in the scheduler/SQS gate. A read error fails closed. Only AutoPurchase plans reach these checks.notifiedto auto-execute on the scheduler and SQS paths (pendingis refused). Rows with no recommendations are unchanged (still failed by the existing no-recommendations refusal).PlanRequest.max_step_commitment(validated > 0 and finite inPurchasePlan.Validate), kept on PUT when omitted, openapi updated. No UI work (follow-up).Current state to expose (read this)
max_step_commitment = NULLafter migration, i.e. auto-purchase of recommendation-bearing steps is refused for all plans until an operator sets a cap.auto_purchase_enabled = FALSEfor all deployments after migration. There is no API/UI to flip it yet (follow-up); until then it is set directly inglobal_config. The PR2b-2 DEV acceptance needs this set to true and a manual/api/scheduledcall (no Terraform schedule firesprocess_scheduled_purchases; only the CloudFormation template does).approvedrows (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 nullablepurchase_plans.max_step_commitment NUMERIC(18,2)with a> 0CHECK, andglobal_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
notified+ cap + kill switch on. TheTestPauseClaim_*integration tests used pending rows: pause only applies topendingand resume returns topending, and the veto now refuses a pending row, so a resumed row waits for a notification before it can auto-execute.TestPauseClaim_ConcurrentPauseAndWorkerwas reshaped accordingly (the pause-vs-claim race no longer exists for rec-bearing plan rows on the auto path).ProcessScheduledPurchasesfails an empty (rec-less) plan row rather than running it, now pinned forNotificationDaysBefore0 and 7.notifiedand 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.TransitionExecutionStatus(the claim) is never called. Manual/approved row bypass and non-AutoPurchase / web rows never read the kill switch. Kill switch inshouldNotifyPlanand the notification tick.000112up/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/apipause-claim and migrations suites pass with-tags integration.err != nilread check inautoPurchaseEnabled) 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 lint0 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
🤖 Generated with Claude Code