fix(purchase): gate sweep/SQS execution on approval+AutoPurchase, fail-loud on delay-config error, safe reaper retry (adversarial-review follow-ups) - #1456
Merged
Conversation
… delay-config error, safe reaper retry F1 (HIGH - approval bypass): add executableByScheduler gate to both ProcessScheduledPurchases cron sweep and handleExecutePurchase SQS path; pending/notified rows with AutoPurchase=false or source=web are skipped/rejected, requiring explicit token-link approval. Gate is fail-closed: a plan-fetch error counts as ineligible. F2 (HIGH - dead auto-create): GetExecutionByPlanAndDate now wraps ErrNotFound on zero rows; getOrCreateExecution branches on errors.Is(ErrNotFound) and defensively treats (nil, nil) as not-found so the create branch is reliably reachable. Pre-fix the create branch was dead code against the real store. F3 (MED-HIGH - fail-open money): both approveViaToken and approvePurchaseViaSession now return 500 when GetGlobalConfig fails instead of silently executing without the configured free-cancel window. Pre-fix: if cfgErr == nil only gated the delay; an error meant immediate execute. F4 (MED - reaper unsafe retry): reapOne only appends "; safe to retry" when allRecsSafeToRedrive returns true. Azure savings-plans have no server-side idempotency key and must not be retried without review. Regression tests added for all four defects; existing tests updated to reflect the new AutoPurchase gate (set AutoPurchase=true on plans that should execute, update ErrNotFound mocks). gocyclo kept under 10 by extracting processOneExecution and checkAutoExecuteGate helpers.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughPurchase approval now fails closed on global-config errors. Scheduled and message-driven execution paths enforce source and AutoPurchase eligibility. Not-found lookup errors are classified for execution creation, and reaper retry messaging now depends on recommendation safety. ChangesPurchase execution behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler as ProcessScheduledPurchases
participant Processor as processOneExecution
participant Gate as executableByScheduler
participant PlanStore as GetPurchasePlan
participant Executor as claimAndExecute
Scheduler->>Processor: process due execution
Processor->>Gate: check source and AutoPurchase
Gate->>PlanStore: load purchase plan
PlanStore-->>Gate: eligibility result
Gate-->>Processor: eligible or rejected
Processor->>Executor: claim and execute eligible row
Executor-->>Processor: execution result
Processor-->>Scheduler: update ProcessResult
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/purchase/manager.go`:
- Around line 492-514: Update executableByScheduler to reject the persisted
"cudly-web" source value before fetching the plan, preserving the fail-closed
approval gate for web-originated executions; then update the related regression
fixtures to use and verify "cudly-web".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96c78f17-ef66-445e-ac76-29bfd2bd1f86
📒 Files selected for processing (14)
internal/api/handler_purchases.gointernal/api/handler_purchases_test.gointernal/config/store_postgres.gointernal/config/store_postgres_pgxmock_test.gointernal/purchase/coverage_extra_test.gointernal/purchase/manager.gointernal/purchase/manager_test.gointernal/purchase/messages.gointernal/purchase/messages_test.gointernal/purchase/money_path_regression_test.gointernal/purchase/notifications.gointernal/purchase/notifications_test.gointernal/purchase/reaper.gointernal/purchase/reaper_test.go
Comment on lines
+492
to
+514
| // executableByScheduler reports whether a pending or notified execution may be | ||
| // auto-executed by the cron sweep or an SQS execute_purchase message, without | ||
| // an explicit human approval action (fail closed on money paths). | ||
| // | ||
| // Rules: | ||
| // - source="web" rows must wait for the token-link approval path; the | ||
| // scheduler and SQS paths must never bypass that gate. | ||
| // - All other pending/notified rows require the owning plan to have | ||
| // AutoPurchase=true. A plan-fetch error is propagated so the caller can | ||
| // fail closed rather than defaulting to "execute". | ||
| // | ||
| // "approved" rows are handled by the session/token approval paths and | ||
| // RecoverStrandedApprovals; this helper is only called for pending/notified. | ||
| func (m *Manager) executableByScheduler(ctx context.Context, exec *config.PurchaseExecution) (bool, error) { | ||
| if exec.Source == "web" { | ||
| return false, nil | ||
| } | ||
| plan, err := m.config.GetPurchasePlan(ctx, exec.PlanID) | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to fetch plan %s for AutoPurchase gate: %w", exec.PlanID, err) | ||
| } | ||
| return plan.AutoPurchase, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Gate the persisted cudly-web source value.
Line 506 checks "web", but PurchaseExecution.Source is documented as "cudly-web". Such rows can therefore pass this shared scheduler/SQS gate and execute without approval when AutoPurchase=true. Handle the persisted value and update the regression fixtures accordingly.
Proposed fix
- if exec.Source == "web" {
+ if exec.Source == "web" || exec.Source == "cudly-web" {
return false, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // executableByScheduler reports whether a pending or notified execution may be | |
| // auto-executed by the cron sweep or an SQS execute_purchase message, without | |
| // an explicit human approval action (fail closed on money paths). | |
| // | |
| // Rules: | |
| // - source="web" rows must wait for the token-link approval path; the | |
| // scheduler and SQS paths must never bypass that gate. | |
| // - All other pending/notified rows require the owning plan to have | |
| // AutoPurchase=true. A plan-fetch error is propagated so the caller can | |
| // fail closed rather than defaulting to "execute". | |
| // | |
| // "approved" rows are handled by the session/token approval paths and | |
| // RecoverStrandedApprovals; this helper is only called for pending/notified. | |
| func (m *Manager) executableByScheduler(ctx context.Context, exec *config.PurchaseExecution) (bool, error) { | |
| if exec.Source == "web" { | |
| return false, nil | |
| } | |
| plan, err := m.config.GetPurchasePlan(ctx, exec.PlanID) | |
| if err != nil { | |
| return false, fmt.Errorf("failed to fetch plan %s for AutoPurchase gate: %w", exec.PlanID, err) | |
| } | |
| return plan.AutoPurchase, nil | |
| } | |
| // executableByScheduler reports whether a pending or notified execution may be | |
| // auto-executed by the cron sweep or an SQS execute_purchase message, without | |
| // an explicit human approval action (fail closed on money paths). | |
| // | |
| // Rules: | |
| // - source="web" rows must wait for the token-link approval path; the | |
| // scheduler and SQS paths must never bypass that gate. | |
| // - All other pending/notified rows require the owning plan to have | |
| // AutoPurchase=true. A plan-fetch error is propagated so the caller can | |
| // fail closed rather than defaulting to "execute". | |
| // | |
| // "approved" rows are handled by the session/token approval paths and | |
| // RecoverStrandedApprovals; this helper is only called for pending/notified. | |
| func (m *Manager) executableByScheduler(ctx context.Context, exec *config.PurchaseExecution) (bool, error) { | |
| if exec.Source == "web" || exec.Source == "cudly-web" { | |
| return false, nil | |
| } | |
| plan, err := m.config.GetPurchasePlan(ctx, exec.PlanID) | |
| if err != nil { | |
| return false, fmt.Errorf("failed to fetch plan %s for AutoPurchase gate: %w", exec.PlanID, err) | |
| } | |
| return plan.AutoPurchase, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/purchase/manager.go` around lines 492 - 514, Update
executableByScheduler to reject the persisted "cudly-web" source value before
fetching the plan, preserving the fail-closed approval gate for web-originated
executions; then update the related regression fixtures to use and verify
"cudly-web".
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
…uteHeadroom Fixes golangci-lint errcheck failure on handler_ri_exchange.go:1351 where `exchange.ParseDecimalRat` error was discarded with `_`. The fix extracts a `checkCapsAndComputeHeadroom` helper that bundles the three cap-validation checks (MaxDailyUSD==0, checkDailyCap, MaxPerExchangeUSD==0) and the headroom calculation, properly returning a reason string on any parse failure (fail closed). This also brings `executeApprovedExchange` back under gocyclo -over 10 (complexity 8) after the helper reduces it from the 12 that errcheck enforcement would have caused. Rebased onto main (after PR #1456 merged; no conflicts in store_postgres.go).
cristim
added a commit
that referenced
this pull request
Jul 19, 2026
…uteHeadroom Fixes golangci-lint errcheck failure on handler_ri_exchange.go:1351 where `exchange.ParseDecimalRat` error was discarded with `_`. The fix extracts a `checkCapsAndComputeHeadroom` helper that bundles the three cap-validation checks (MaxDailyUSD==0, checkDailyCap, MaxPerExchangeUSD==0) and the headroom calculation, properly returning a reason string on any parse failure (fail closed). This also brings `executeApprovedExchange` back under gocyclo -over 10 (complexity 8) after the helper reduces it from the 12 that errcheck enforcement would have caused. Rebased onto main (after PR #1456 merged; no conflicts in store_postgres.go).
cristim
added a commit
that referenced
this pull request
Jul 19, 2026
…d ledger, and headroom (adversarial-review follow-ups) (#1457) * fix(exchange): enforce daily-cap integrity on manual path, actual-paid ledger, and headroom (adversarial-review follow-ups) H1: checkDailyCap fails closed on unparseable paymentDue (was treating as $0). H2: executeApprovedExchange and processAutoExchange bound Execute MaxPaymentDueUSD by min(perExchangeCap, dailyCap-dailySpent) so re-quotes cannot breach headroom. H3: Add CompleteRIExchangeWithPayment to persist the accepted (not pre-execution) amount; both auto and manual paths use the fresh quote from Execute. H4: Retry ledger write 3x after money moves; halt auto run / return 500 on persistent failure to prevent cap bypass via missing ledger rows. M5: GetRIExchangeDailySpend includes processing rows (COALESCE on completed_at/ updated_at) to close concurrent-approval TOCTOU window. Each defect has a regression test that fails pre-fix and passes post-fix. * fix(exchange): handle ParseDecimalRat error, extract checkCapsAndComputeHeadroom Fixes golangci-lint errcheck failure on handler_ri_exchange.go:1351 where `exchange.ParseDecimalRat` error was discarded with `_`. The fix extracts a `checkCapsAndComputeHeadroom` helper that bundles the three cap-validation checks (MaxDailyUSD==0, checkDailyCap, MaxPerExchangeUSD==0) and the headroom calculation, properly returning a reason string on any parse failure (fail closed). This also brings `executeApprovedExchange` back under gocyclo -over 10 (complexity 8) after the helper reduces it from the 12 that errcheck enforcement would have caused. Rebased onto main (after PR #1456 merged; no conflicts in store_postgres.go).
cristim
added a commit
that referenced
this pull request
Jul 19, 2026
…w-up to #1456) (#1458) executableByScheduler gated on the bare literal exec.Source == "web", but web-originated executions are persisted with Source = common.PurchaseSourceWeb ("cudly-web"). The literal never matched, so a pending/notified web row on a plan with AutoPurchase=true could be auto-executed by the cron sweep and the SQS execute_purchase path WITHOUT the required token-link human approval -- a fail-open on a money path (CodeRabbit CRITICAL thread on #1456, left unaddressed). Fix: - manager.go: compare against the typed common.PurchaseSourceWeb constant, not a stringly-typed literal (repo enum convention). - manager_test.go / messages_test.go: the prior regression tests set Source: "web" (matching the buggy gate) and relied on AssertNotCalled("GetPurchasePlan"); the mock's default GetPurchasePlan stub does not record the call, so those tests could never detect the bypass. Use the real persisted value and assert the invariant via a GetPurchasePlanFn sentinel: web rows must short-circuit before the AutoPurchase plan fetch. Verified the tests FAIL on the buggy literal and PASS on the fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ProcessScheduledPurchasescron andhandleExecutePurchaseSQS path now gate onexecutableByScheduler: pending/notified rows withAutoPurchase=falseorsource=webare skipped/rejected without touching the cloud. Gate is fail-closed: a plan-fetch error is treated as ineligible, not as a pass.GetExecutionByPlanAndDatenow wrapsErrNotFoundon zero rows;getOrCreateExecutionbranches onerrors.Is(ErrNotFound)so the create branch is reliably reachable. Pre-fix the branch was dead code against the real store (tests only passed because mocks returned(nil,nil)).approveViaTokenandapprovePurchaseViaSessionnow return 500 whenGetGlobalConfigfails rather than executing without the configured free-cancel window. Pre-fix:if cfgErr == nil && delay > 0silently fell through on transient config errors.reapOneonly appends"; safe to retry"whenallRecsSafeToRedrivereturns true. Azure savings-plans lack server-side idempotency and must not be encouraged to retry without review.Test plan
origin/mainbefore implementing fixesAutoPurchase: trueset on plans that should execute;GetExecutionByPlanAndDatemock behavior unchanged (defensive switch handles both(nil,nil)andErrNotFound)internal/purchase,internal/config,internal/apigo vetclean;gocyclo -over 10(non-test files only, per pre-commit hook) clean;processOneExecutionandcheckAutoExecuteGateextracted to keep affected functions under cyclomatic limit of 10Summary by CodeRabbit