fix(purchases): record partial success and stamp target account on history (closes #642, #646) - #650
Conversation
…story Partial-failure runs (some recs commit, others fail) were marked the whole execution "failed" and skipped the success notification despite real commitments written to purchase_history with Purchased=true, inviting a re-approve that double-buys the already-purchased recs (#642). The single-account path also stamped the ambient AWS host account on history regardless of the resolved target account or provider (#646). - #642: introduce a "partially_completed" outcome. When at least one rec committed, never mark the row "failed"; record partially_completed, send the confirmation for the recs that purchased, and surface the row in History (flagged IsAuditGap so its execution-level dollars are excluded - the committed dollars come from the per-rec purchase_history rows). Applies to both the single-account path (via a partialPurchaseError sentinel that finalizeExecution maps) and the multi-account fan-out path. - #642 frontend: on a partial fan-out failure, name the buckets that created pending executions so the still-actionable requests are not silent orphans; add a distinct "Partial" history badge. - #646: resolve the target account's ExternalID from the resolved account and stamp that on purchase_history, falling back to the ambient AWS STS identity only when no target account can be identified. Fixes assume-role AWS and direct-execute Azure/GCP stamping. Regression tests: single-account + multi-account partial success (partially_completed status + notification sent), correct account stamping for AWS and Azure, History row synthesis with dollar-exclusion, and the fan-out partial toast naming submitted buckets. Closes #642 Closes #646
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements consistent handling of partial failures across purchase execution flows. When some recommendations succeed and others fail, the backend now records a new ChangesPartial Purchase Execution Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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/execution.go`:
- Around line 260-262: When GetCloudAccount returns nil while a specific
cloudAccountID has been resolved, do not silently return (nil, "", nil) which
allows fallback to ambient credentials; instead detect that cloudAccountID (or
the variable holding the target account ID) is set and return a descriptive
error indicating the target account could not be resolved. Update the nil-check
block around GetCloudAccount/account (the section containing "if account == nil
{ return nil, \"\", nil }") to return an error that includes the cloudAccountID
and mentions unresolved account, so callers cannot continue and accidentally use
ambient credentials.
🪄 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: 85308fdf-8137-4422-997b-61ad77428f1c
📒 Files selected for processing (8)
frontend/src/__tests__/purchase-execution-toast.test.tsfrontend/src/app.tsfrontend/src/history.tsinternal/api/handler_history.gointernal/api/handler_history_test.gointernal/purchase/execution.gointernal/purchase/execution_test.gointernal/purchase/manager.go
…ack to ambient When a single-account purchase resolves a concrete cloudAccountID but GetCloudAccount returns a nil account (the account does not exist), resolveSingleAccountProvider returned (nil, "", nil). The caller then treated the empty target ID as "truly ambient" and fell back to the host AWS STS identity, purchasing and stamping purchase_history against the wrong account (#646). Surface a descriptive error instead so the caller's no-ambient-fallback contract holds once a target account is known. Add a regression test covering the not-found path.
|
Addressed the actionable finding: @coderabbitai review |
|
Triggering a fresh review now. ✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
Fixes two execution-core correctness issues (#642 partial-failure handling, #646 wrong account stamped on history).
#642 — partial success no longer reads as total failure
New
partially_completedstatus, used whenever at least one rec committed a real purchase and at least one failed. Such a row is NEVER markedfailed(the double-spend hazard the issue warns about), and the success notification is sent for the recs that purchased.executeSingleAccountreturning a*partialPurchaseError;finalizeExecutionmaps it viaerrors.Astopartially_completed(preserving any audit-gap note via a sharedappendErrNote).executeForAccount): setspartially_completedinline + notifies, still surfaces per-rec failures to the aggregator.partially_completedadded tohistoryExecutionStatuses, always synthesised, flaggedIsAuditGap(excludes execution-level dollars; committed dollars come from the per-recpurchase_historyrows), clearStatusDescription. Frontend: partial fan-out toast names the still-actionable submitted buckets + a "Partial" history badge.#646 — correct account on history
resolveSingleAccountProvidernow also returns the resolved target account'sExternalID; the single-account path stamps that (matching the fan-out path), falling back to ambient AWS STS only when no target account is identified. Fixes assume-role AWS and direct-execute Azure/GCP.Test plan
partially_completed+ notification; AWS + Azure correct account stamping; History synthesis + dollar exclusionCloses #642, #646.
Note:
TestManager_RecoverStrandedApprovals_LateCompletionNotClobberedfails on the unmodified base branch (confirmed via git stash) — unrelated to this change, tracked separately.Summary by CodeRabbit
Bug Fixes