[cristim]

Summary

• When no account of a multi-account fan-out committed and every account was refused before submission, never started, or finished with nothing bought, the root error now carries the before-submission class, so the root row is stamped no_submission and releases its suppressions. The decision reads the in-memory per-account results (fanOutNotSubmitted), not saved rows, so a panicked or lost-row account never reads as "nothing submitted".
• A failed account-row save strips the class, so a lost audit row never counts as proof.
• First commit adds the per-account fixture the fan-out tests use (tests only).

Verification

• Fail-before/pass-after tests for each ending (refused, never started, panic, unknown outcome, plain error, Success=false, mixed) through the real executeMultiAccount.
• Mutation probes on 7 production lines (ignore started, drop the r.Value guard, drop the empty-results guard, drop the nil-error branch, drop the class strip, drop the wrap, count refused as started) are each killed by a test. Unit and golangci-lint pass locally on macOS; CI covers Linux and the integration suites.
• Suppressions are keyed by execution_id, so a root's release never touches another attempt's rows.
• Credential and pin failures on account rows (S2) are a follow-up; this PR is S0+S1 of the plan.

Part of #1844

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 8 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/1844-fanout-root-proof
• File: .coderabbit.yaml
• Commit: 51810d2
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: ffb0286e-a4db-4e26-bd7a-2f64594a5a4d

Commits

Reviewing files that changed from the base of the PR and between 3c1fad7 and 51810d2.

Files selected for processing: 5

• internal/purchase/execution.go
• internal/purchase/fanout_refusal_fixture_test.go
• internal/purchase/fanout_root_proof_test.go
• internal/purchase/refusal_tally.go
• internal/purchase/refusal_tally_test.go

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

APPROVE @ 16284b5 by claude-nibblets

Adversarial review of #1881 (Part of #1844, S0 and S1, +413/-5, money path). It matters beyond the fan-out: since #1870, a root stamped no_submission at attempt 0 is what lifts the EC2 and Azure savings-plan Retry refusal, so a wrongly stamped root would allow a second buy. I tried to find an ending that gets the stamp without being proven unsent and did not find one.

Read (internal/purchase/refusal_tally.go, execution.go): `fanOutNotSubmitted` runs only when no account committed and reads the in-memory per-account results, not saved rows. It returns true only if the result list is non-empty and every account either bought nothing without error (the nothing-selected case of #339), was refused before submission (`refusedBeforeSubmission`: the class and not `ErrOutcomeUnknown`), or never started (no entry in `started`, so the run context ended first). Any panic, plain error from an account that started, unknown outcome, or bought rec keeps today's unproven failure. A failed account-row save re-joins the cause as text (`errors.New(cause.Error())`), so a lost audit row cannot carry the class into the proof, and `failedPurchasesError` only wraps when `allRefused`. The root error keeps its `%w` chain (`errAllAccountsFailed`). `skippedAccountNote` now uses `RetryRefusalReason`, matching #1870.

Scoped probes (go test -run FanOut|Refusal|RootProof|Tally|MultiAccount|Skipped|Fan in internal/purchase, scratch worktree at this head, baseline ok), each killed: committed account ignored (TestFanOutNotSubmitted_GuardsAreSafeOnTheirOwn); a started account with a plain error counted as unsent (TestExecuteMultiAccount_RootProof, _FanOutFixtureHooks); empty results counted as unsent (_GuardsAreSafeOnTheirOwn); root not wrapped (TestExecuteMultiAccount_AllRefusedRedshiftRootIsNeverSubmitted and two more); lost audit row keeping the class (TestExecuteMultiAccount_RootProof); unknown outcome counted as refused (TestExecuteMultiAccount_RootProof, TestMultiAccount_MixedOutcomesAreNotNoSubmission; my first run of this one did not compile and was redone); an account that bought nothing without error blocking the proof (_GuardsAreSafeOnTheirOwn).

Residual trust, not new here: an account that ends with no purchased rec and no error is read as "bought nothing"; that is the existing #339 meaning of `committed == false` and I did not audit every way a provider call could return without recording an error. The S2 credential and pin failures are a follow-up, as the PR says.

CI at this head: Lint Code and Integration Tests (purchase) pass; Unit Tests and others were still running (26 pass, 11 pending).

[cristim]

CHANGES REQUESTED 16284b5 by claude-nibblets

I am withdrawing my APPROVE at this head. I posted it while Unit Tests was still pending; it has since failed (run 42670342582, CI Success fails as a result), and the failure is in this PR's own new test file.

Failure: TestNoUnfailableMockAssertions (the repo's vacuous-assertion guard) reports 2 assertion sites it cannot check:

• internal/purchase/fanout_root_proof_test.go:67:5 r.store.AssertCalled(t, "DeleteSuppressionsByExecutionTx", ...)
• internal/purchase/fanout_root_proof_test.go:69:5 r.store.AssertNotCalled(t, "DeleteSuppressionsByExecutionTx", ...)
  Both: "receiver is not a plain identifier (e.g. a field or nested selector). Review by hand."

Fix: take the mock into a local first (for example store := r.store before the if tc.wantProof branch, then store.AssertCalled(...) and store.AssertNotCalled(...)), so the guard can resolve the receiver. The production change and my probe results are unaffected by this; once the test is green I will re-approve at the new head.

Everything else in my earlier review stands (the analysis of fanOutNotSubmitted, the seven probes killed).

[cristim]

APPROVE @ 51810d2 by claude-nibblets

Delta 16284b5..51810d2 is test-only: one commit in internal/purchase/fanout_root_proof_test.go (+3/-2) that takes the mock into a local (store := r.store) before AssertCalled and AssertNotCalled, which is the fix I requested; no production file changes. It clears the two sites TestNoUnfailableMockAssertions could not check (fanout_root_proof_test.go:67 and :69). I ran at this head: TestNoUnfailableMockAssertions and TestVacuousAssertionGuardDetects (internal/mocks) pass, and the fan-out tests (-run FanOut|RootProof|MultiAccount|Refusal, internal/purchase) pass. My earlier analysis of fanOutNotSubmitted and the seven killed probes are unaffected, since the production code is the same.

CI at this head: Unit Tests, Lint Code and Run pre-commit hooks pass (12 pass, 3 pending, 2 skipping when I looked). My CHANGES REQUESTED at 16284b5 is superseded by this approval.