Skip to content

test(purchase): regression guard for parallel multi-account execution isolation (closes #210) - #233

Merged
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
test/issue-210-multi-account-isolation
May 3, 2026
Merged

test(purchase): regression guard for parallel multi-account execution isolation (closes #210)#233
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
test/issue-210-multi-account-isolation

Conversation

@cristim

@cristim cristim commented May 3, 2026

Copy link
Copy Markdown
Member

Summary

  • Gap closed: spec E-2 (specs/multi-account-execution/acceptance.md) required that one account's failure not fail others during parallel multi-account fan-out, but no automated test guarded this invariant — an inadvertent refactor (e.g., swapping the errgroup for a serial loop, or propagating a per-account error up the group) would silently regress the fault-tolerance promise.
  • New test added: TestExecuteMultiAccount_PartialFailure_IsolatesAccounts in internal/purchase/execution_test.go seeds two accounts (V=valid credentials, I=invalid credentials), calls executePurchase, and asserts account-V completes while account-I is recorded as failed, with full result-data isolation between the two.
  • Test-only PR: zero production code changes.

What the test pins

Assertion Guards against
Two SavePurchaseExecution calls fire Silent skip of a failed account's record
account-V: status=completed, PurchaseID set Failure of I poisoning V's success path
account-I: status=failed, error non-empty Silent swallowing of credential failures
account-I error does NOT contain account-V's access key Cross-account credential data leak (log sanitisation)
account-I error does NOT contain account-V's CommitmentID Result-data cross-contamination
executePurchase returns an error (I's failure is aggregated) errgroup silently discarding per-account errors
Base exec.CloudAccountID remains nil Fan-out accidentally mutating the shared base record

Spec coverage

Satisfies specs/multi-account-execution/acceptance.md scenario E-2.

Test plan

  • go test ./internal/purchase/... -v -count=1 -run TestExecuteMultiAccount_PartialFailure_IsolatesAccounts — 1/1 pass
  • go test ./internal/purchase/... -count=1 — 100/100 pass
  • go vet ./... — clean
  • gofmt -l . — empty output
  • Three consecutive clean verification passes

Summary by CodeRabbit

  • Tests
    • Added regression test for multi-account purchase execution scenarios, validating proper error handling and data isolation when credential resolution fails in some accounts while succeeding in others.

… isolation (closes #210)

Adds TestExecuteMultiAccount_PartialFailure_IsolatesAccounts to pin spec
E-2: when account-I has invalid credentials and account-V has valid
credentials, account-V's execution must complete with status=completed
and its own CommitmentID, while account-I is recorded as failed.

Assertions cover: both SavePurchaseExecution calls fire (no silent skip),
account-V's record is completed with a purchase, account-I's record is
failed with a non-empty error, no cross-account credential material leaks
into account-I's error (log-sanitisation guard), and account-V's
CommitmentID does not appear anywhere in account-I's record
(result-data independence). The base exec CloudAccountID stays nil,
confirming per-account copies are used throughout the fan-out.

Test-only change; no production code modified.
@cristim

cristim commented May 3, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f3b6a807-e383-4792-bd45-6f6bf5aedcb3

📥 Commits

Reviewing files that changed from the base of the PR and between c84fd02 and 3491157.

📒 Files selected for processing (1)
  • internal/purchase/execution_test.go

📝 Walkthrough

Walkthrough

A regression test is added that validates multi-account purchase execution with proper isolation when one account fails credential resolution while another succeeds. The test verifies per-account error handling, cross-account data sanitization, and persistence of execution records.

Changes

Multi-Account Execution Isolation Test

Layer / File(s) Summary
Regression Test
internal/purchase/execution_test.go
New test TestExecuteMultiAccount_PartialFailure_IsolatesAccounts validates that credential resolution failure in one AWS account results in a failed execution record for that account, while the valid account's record is marked completed; confirms cross-account data isolation by asserting that error and recommendation messages do not leak the valid account's access key or commitment ID; requires two SavePurchaseExecution calls; and ensures the base execution record remains untagged with CloudAccountID == nil.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

Poem

🐰 Two accounts hop through, one stumbles and falls,
Yet data stays safe within separate walls,
The test hops along to ensure they're kept clean,
No secrets escape to be wrongly seen.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: a regression test for multi-account execution isolation.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/issue-210-multi-account-isolation

Review rate limit: 0/5 reviews remaining, refill in 50 minutes and 49 seconds.

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

@coderabbitai

coderabbitai Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim cristim added priority/p1 Next up; this sprint severity/high Significant harm urgency/this-sprint Within the current sprint impact/all-users Affects every user effort/m Days type/chore Maintenance / non-user-visible triaged Item has been triaged labels May 3, 2026
@cristim

cristim commented May 3, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/all-users Affects every user priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant