[IAmJSD]

Next slice of #35: the internal/purchase package. #1249 already fixed money_path_regression_test.go. This PR covers the rest of the package's tests, starting with the money-path files: claim_pin, claim_guard, step_cap, armed_redrive and failure_notify*.

Refs #35

Root cause and fix

Many internal/purchase tests registered required .On(...) expectations but never called AssertExpectations. If production stopped making one of those calls (a claim CAS, a history write, a confirmation email), the test still passed whenever its side-channel assertions held.

The fix adds assertMocksOnCleanup(t, mocks...) to mocks_test.go. It registers m.AssertExpectations(t) as a t.Cleanup, so it fires even when a require stops the test early. Each affected test calls it when it builds its mocks. Shared harnesses call it once for all their users: newApproveManager, armedHarness, newDetachFixture, tokenCapture, runFanOutCapturingOpts, purchaseThroughPinnedAzureClient, listedThresholds and managerServing. The change touches only _test.go files. No production code changed.

How the gaps were found

I used a scope-aware AST scan. A test function counts as a gap when it registers a non-.Maybe() .On on a mock and never asserts that mock in the same function. On main at 7b86e38 the scan found 94 such test functions across 26 files in internal/purchase. After this PR, every remaining hit is a false positive:

• The mocks come from a harness that now asserts them: newApproveManager, armedHarness or newDetachFixture.
• post_cas_guarded_save_integration_test.go's TestPostCASSave_ConcurrentStatusChangeSurvives already checks its one .Once() with AssertNumberOfCalls(..., 1). I left it unchanged.

Expectations that never fired

All of these were stubs for a code path the test doesn't take. None was a missed production call, so there is nothing to report as a production bug. Each one is now .Maybe() with a comment, or required only for the case that takes that path:

Where	Unused stub	Why it is unused	Change
claim_pin_test.go newPinManager	GetStaleApprovedExecutions / GetPendingExecutions (cron), GetExecutionByID (SQS)	Each test drives only one entry path	.Maybe() on these three reads. The claim and save calls each test adds stay required
step_cap_test.go gate harnesses	GetGlobalConfig	executableByScheduler refuses a missing plan before it reads the kill switch	expectKillSwitchRead makes the read required unless the plan is nil. Every other row now has to read the kill switch
step_cap_test.go TestScheduler_DirectExecuteRowSkippedWithoutPlanLookup	GetPurchasePlan(ctx, "")	A trap the test already asserts NotCalled	.Maybe()
claim_guard_test.go TestApprove_ClaimsRunningAndStampsGuarded	stubExecuteChain's SavePurchaseExecution	The approve path uses only guarded saves, and the test already asserts NotCalled	Made optional with the new optionalCalls helper
failure_notify_test.go (non-failed statuses)	SendPurchaseFailedNotification	A trap the test already asserts NotCalled	.Maybe()
failure_notify_paths_test.go	Config read and failed-email send	Writers that lost the row must not alert	Optional only when !tc.alerts. The test already asserts Len 0
failure_notify_writers_test.go fanOutRun	Failed-email send	No email goes out for a panicked child. Callers assert the exact set of emailed rows	.Maybe()
reaper_ordering_test.go	The CAS whose from includes "running"	This is the bug path, which must not fire. The test asserts Reaped == 0	.Maybe()
plan_step_attach_test.go attachManager	GetGlobalConfig	Read only for grace periods (no-match and attach paths). Cap and unsupported-plan refusals never read it	.Maybe()
plan_step_attach_test.go no-match before the deadline	SavePurchaseExecution	No row is written inside the grace window, and the test already asserts Empty	Optional for that case only
execute_detached_test.go provider-error case	Fixture's SavePurchaseHistory / SendPurchaseConfirmation	Nothing was bought	optionalCalls in that test only
notified_run_now_test.go row not yet due	newPinManager's plan and kill-switch reads	A row that isn't due never reaches the gate	optionalCalls for that case only

optionalCalls(m, methods...) marks a helper's stubs as .Maybe().Times(0). The Times(0) is needed because testify still reports an unconsumed .Once() even when it is .Maybe().

Verification

All of this is unit/mock evidence plus two narrow real-Postgres runs. Nothing calls a real cloud API. Every command ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.

Mutation probes (fail-before/pass-after). For each probe I removed one collaborator call from production code locally. I ran the same test against main's test files and against this branch's test files, then reverted. git status showed no production change afterwards.

Probe (production change, reverted)	Test	main's tests	this branch
claimAndExecute: drop the TransitionExecutionStatusPinned claim and return an error instead	TestClaimPin_ClaimErrorIsFailure	ok (Failed == 1 still holds)	FAIL: TransitionExecutionStatusPinned(...), 7 out of 8 expectation(s) were met
savePurchaseHistory: skip the SavePurchaseHistory write (audit trail of a bought commitment)	TestMultiAccountFailureNotifiesOncePerFailedAccountNotRoot	ok	FAIL: SavePurchaseHistory(...), 5 out of 6
executeSingleAccount (reached via executeAndFinalize): skip sendPurchaseNotification (purchase confirmation email)	TestApprove_ClaimsRunningAndStampsGuarded	ok	FAIL: SendPurchaseConfirmation(...) unmet

Command per probe: go test ./internal/purchase/ -count=1 -run '^<Test>$'

I also ran a fourth probe that skipped the kill-switch read in executableByScheduler. main's tests already catch it (the kill switch off row fails in both trees), so it shows no difference and I'm not counting it.

Other checks:

• go build ./..., go vet ./..., go vet -tags integration ./...: clean
• go test ./internal/purchase/ -count=5: ok, which shows the new assertions are not flaky
• go test -race ./internal/purchase/ -count=1: ok
• go test ./internal/mocks/ -count=1: ok
• go test -tags integration -count=1 -timeout 20m -run 'TestNotifiedTransition_OnlyAfterEmailSucceeds|TestAttachIntegration_EmailFailureLeavesPendingAndNextTickDoesNotResolveAgain' ./internal/purchase/: both PASS on real Postgres (testcontainers). These are the two integration tests this PR changes.
• gofmt -l internal/purchase: clean
• golangci-lint v2.10.1 run --allow-parallel-runners ./internal/purchase/..., with and without --build-tags integration: 0 issues
• gocyclo: only _test.go files changed, so it doesn't apply

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ c00ef619 = origin/main 15dc8feb plus these heads, merged in this order:

Branch	Tested head
fix/issue-712-fargate	2df9723
fix/issue-41-ri-exchange-wiring	bb548fe
fix/issue-349-ci-hygiene	360ae6c
fix/issue-756-next-collection	2cb1b2f
fix/issue-36-go-clock	aaea757
fix/issue-36-fe-waits	7feb01d
fix/issue-35-purchase-asserts	fbafeb0
fix/issue-134-mfa-token	33da813

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran on 644bfcaf, which differs from this tree only by a test-only rename on fix/issue-134-mfa-token (a password-like constant that git-secrets flagged):

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package that has them, one package at a time (-tags integration -count=1): internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase and internal/server all pass. Migration 000152 applies on top of main's 000151.
• Frontend: the full jest suite passes on this tree (175 suites, 4595 tests, 1 skipped).
• Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.
• Terraform .tftest.hcl files were not run on this tree (CI runs them). Nothing was applied to any cloud.

Out of scope / remaining for #35

• internal/api, internal/auth, internal/scheduler and internal/server are left for later slices, as the issue's slicing plans. The table in test(purchase,email): assert mock expectations in the money-path regression suite #1249 lists the internal/api counts.
• newCountingSender in plan_step_attach_integration_test.go is a counting stub. Its callers assert the counts directly, so I left it unchanged.

Maintainers: please mirror the issue's triage labels onto this PR: impact/internal, type/chore, priority/p3, severity/low, urgency/eventually, effort/s, triaged.

🤖 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 59 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: main
• File: .coderabbit.yaml
• Commit: 15dc8fe
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 8240ebbb-9bd6-4ff4-9a10-f21b93da1639

Commits

Reviewing files that changed from the base of the PR and between 15dc8fe and fbafeb0.

Files selected for processing: 33

• internal/purchase/approvals_test.go
• internal/purchase/armed_redrive_test.go
• internal/purchase/aws_no_token_redrive_test.go
• internal/purchase/azure_savings_plans_alias_proof_test.go
• internal/purchase/claim_guard_test.go
• internal/purchase/claim_pin_test.go
• internal/purchase/claimed_row_check_test.go
• internal/purchase/coverage_extra_test.go
• internal/purchase/empty_selection_notify_test.go
• internal/purchase/execute_detached_test.go
• internal/purchase/execution_test.go
• internal/purchase/failure_notify_paths_test.go
• internal/purchase/failure_notify_test.go
• internal/purchase/failure_notify_writers_test.go
• internal/purchase/finalize_revocations_test.go
• internal/purchase/gcp_history_cost_test.go
• internal/purchase/ladder_execution_recheck_test.go
• internal/purchase/manager_test.go
• internal/purchase/mocks_test.go
• internal/purchase/notified_claim_pin_integration_test.go
• internal/purchase/notified_run_now_test.go
• internal/purchase/partition_guard_test.go
• internal/purchase/plan_step_atomic_test.go
• internal/purchase/plan_step_attach_integration_test.go
• internal/purchase/plan_step_attach_test.go
• internal/purchase/plan_step_proofs_test.go
• internal/purchase/reaper_ordering_test.go
• internal/purchase/reaper_test.go
• internal/purchase/redrive_claimed_row_test.go
• internal/purchase/requested_at_test.go
• internal/purchase/scheduled_fire_test.go
• internal/purchase/step_cap_test.go
• internal/purchase/zonal_az_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

_{Comment @coderabbitai help to get the list of available commands.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE fbafeb0816bff9ddbbfc95a79f74ec577d4c4dda

Blocking:

• none

Non-blocking:

• internal/purchase/mocks_test.go:44-50 (optionalCalls): .Times(0) means "unlimited" in testify, not "never". A .Once() stub that it relaxes therefore stops panicking on a second call and also shadows any later stub for the same method. In notified_run_now_test.go:76, newPinManager's GetPurchasePlan(...).Once() (gatePlan) would then answer every read and hide the claimPlan stub. That only matters if a not-yet-due row regresses into the gate, and the test's wantClaim assertions would still catch a claim, so it is harmless today. Consider saying "unlimited" in the doc comment.
• internal/purchase/execute_detached_test.go:189-191: with the history and confirmation stubs made optional, nothing in TestExecuteAndFinalize_ProviderErrorWithCanceledCaller_PersistsFailed proves that no history row was written for a failed buy. assert.Zero(t, f.history) would turn the comment "Nothing bought" into a check. Main didn't assert it either, so this is not a regression.
• A few helper-only files still register .On without a local assert: step_rows_test.go (expectStepRows), fan_out_shape_test.go (all .Maybe()) and token_rotation_pin_integration_test.go / due_age_cap_integration_test.go (Maybe or counting stubs). These look like the false positives the body describes.

Attacked:

• Checked that no .Maybe() or optionalCalls addition weakens a check main actually enforced. Main never called AssertExpectations on any of these mocks, so every required .On the PR keeps is a net gain, and each newly optional stub sits beside an existing AssertNotCalled / Len 0 / Reaped == 0 assertion or is on a path the test doesn't take. Only _test.go files changed.
• Fail-before: I replaced the SavePurchaseHistory call at execution.go:1051 with a no-op. TestMultiAccountFailureNotifiesOncePerFailedAccountNotRoot fails on this branch (SavePurchaseHistory ... 5 out of 6 expectation(s) were met) and passes with main's test files (I swapped in origin/main's versions of the changed tests). Restored afterwards; git status is clean.
• Ran: go build ./...; go vet ./internal/purchase/ with and without -tags integration; go test -race -short -count=1 ./internal/purchase/ (ok); go test -short -count=3 ./internal/purchase/ (ok, not flaky); go test -race -tags integration -count=1 ./internal/purchase/ (whole package, ok, 90s).
• Refs TEST-03: 19 Go test files register .On() expectations without mock.AssertExpectations, incl. money-path regression suite #35 (not Closes) is right: api, auth, scheduler and server slices remain.

Body-stale:

• none

_{Reviewer: rI3. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE fbafeb0816bff9ddbbfc95a79f74ec577d4c4dda

Blocking:

• none

Non-blocking:

• internal/purchase/mocks_test.go optionalCalls: .Times(0) makes the matched expectations unlimited as well as optional. Every current caller pairs it with an explicit AssertNotCalled/Len 0 check, or uses it on a path that can't reach the call, so this is fine today. A one-line note in the doc comment ("the caller must assert absence itself if it matters") would stop a later reader from using it as a softer .Maybe().
• There are a few helpers that register required expectations on mocks they don't own: staleRefusalStore, expectClaim, expectRedriveClaim, expectStepRows, expectMark, stampLostManager and newAWSCNManager. I checked that each caller I spot-read already asserts that mock (due_age_cap_test, post_cas_guarded_save_test, partition_guard_test), so they are not gaps. This agrees with the body's "every remaining hit is a false positive".

Attacked:

• Weakening check: every new .Maybe()/optionalCalls is on a stub that main never asserted either. So nothing that main's tests caught is lost. Each one is a trap that is already asserted absent, or a read on a path the test doesn't take. expectKillSwitchRead makes the kill-switch read required for every armed-plan gate row, which tightens main.
• Fail-before (my own probe, not the body's): in execution.go I stubbed out both the SavePurchaseHistory write and sendPurchaseNotification's SendPurchaseConfirmation. With main's test files 16 tests fail. With this branch's tests 24 fail. The 8 extra are the ones that now catch the missing audit row or confirmation email, for example TestApprove_ClaimsRunningAndStampsGuarded, TestApproveExecution_MintsRevocationToken, TestManager_DirectExecute_StampsAuditFieldsAfterClaim, TestManager_SavePurchaseHistory_Error, TestManager_ExecuteSinglePurchase_WithEngine and TestAzureSavingsPlansAliasIsDerivedFromTheRedriveToken. I reverted the probe and git status was clean.
• Rough scan of internal/purchase/*_test.go for functions that have a non-.Maybe() .On and no assertion or asserting harness: only helper functions remain, and their callers assert.
• Ran on fbafeb0: go vet on ./internal/purchase/ with and without -tags integration; go test -race -short -count=1 (ok); go test -short -count=3 -shuffle=on (ok, not order-dependent); go test -race -tags integration -count=1 ./internal/purchase/ for the full package on real Postgres (ok, 58s); golangci-lint v2.10.1 with and without --build-tags integration (0 issues). Only test files changed, so gocyclo doesn't apply.

Body-stale:

• none

_{Reviewer: rI2. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at fbafeb0816bff9ddbbfc95a79f74ec577d4c4dda merged onto main (76180820), together with #1571, #1568, #1570 in that order (combined tree cf2e973e, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on fbafeb08 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly fbafeb0816bff9ddbbfc95a79f74ec577d4c4dda.