[IAmJSD]

Part A of #35: internal/api test files whose name sorts up to and including handler_config_test.go. Part B (the rest of internal/api, plus internal/auth, internal/scheduler, internal/server and internal/email) is a separate branch. This PR changes tests only.

Root cause

Most internal/api tests registered testify expectations with .On(...) but never checked them. A handler could stop calling a collaborator, such as skipping RevokeAPIKeyAPI or RequestPasswordReset, and the success-path tests stayed green. On main, 22 of the 39 in-range files with .On( had no AssertExpectations at all. 240 functions registered expectations on a mock that the function never asserted.

Fix

• Assert at construction. Each testify mock now gets a t.Cleanup(func() { m.AssertExpectations(t) }) where it is built. This follows the package's existing pattern; part B was told not to touch shared helper files, so there is no new shared assert helper.
• Shared constructors assert their own mocks, so every caller is covered: scopedHandler (its callers include part-B files, which pass on this branch), newScopeTestHandler (callers that pass a bare new(...) are covered too), newAPIKeyRoundtripHandler, sharedNameHandler and analyticsParamsCall.
• Helpers that built mocks without a *testing.T now take one: hostCheckHandler, setupRouterForDispatch, sessionApproveHandler and tokenApproveHandler. All their callers are in-range files.
• Callers of csrfGuardAuth and of adminAnalyticsReq in range add the cleanup. adminAnalyticsReq keeps its signature because part-B files call it.

Expectations that never fired

No production code was wrong. Each case was a stub for a path the test does not take:

Test(s)	What never fired	Why	Change
TestHandler_approvePurchase_EmptyToken	GetExecutionByID	approvePurchase now runs requireSessionBeforeLookup first, so a caller with no session gets a 401 before the lookup. The test comment described the old order.	Stale stub removed, comment fixed, 401 now asserted. If the store were read, the call panics and the existing recover fails the test.
TestExecutedNotification_DirectExecutePath, ..._DirectExecute_NilNotifierNoPanic	SavePurchaseExecution	5780b34 moved the audit stamping into DirectExecute, after its claim. The handler no longer saves the row.	Stale stubs removed, comment fixed.
TestHandler_updateServiceConfig_CommitmentOptsOutsideTransaction (refused cases)	SaveServiceConfig	A refused PUT does not save.	.Maybe(), and new checks: an accepted PUT saves exactly once, a refused one never saves.
analyticsParamsCall (all analytics scope/account_ids tests)	per-endpoint Query*, GetAllowedAccountsAPI	Each endpoint runs only its own queries. A refused request runs none, and a malformed id is refused before the allow-list is read.	.Maybe(), and new check: a served (200) request must read the allow-list and run exactly its endpoint's queries (analyticsPathQueries).
TestAutoPurchaseGate_CapValidation/quoted number	GetPurchasePlan	The body fails to decode before the plan is read.	Optional for that one case only (decodeRefused).
TestApproveRegistration_EmptyRoleARNRequiresHostAccount (rejected cases)	ApproveAccountRegistration	It is a trap the test already pins with AssertNotCalled.	.Maybe().
Direct gate calls: requireExecutionAccess, approveRegistration, the shared-name and editor-external-ID gate tests, getAccount with an invalid UUID	ValidateSession (and HasPermissionAPI)	The method is called directly with a session, or refuses before the session is read. There is no bearer token to validate.	relaxUnreachedStubs on those methods, in those tests only.
TestPlannedPurchases_HidesPartiallyScopedParent, ..._PermissionRefusedBeforeScope, ..._FourEyesOn_RefusedBeforeAnyWrite	GetExecutionByID / ListStoredRecommendations	The list does not read by ID. The refusal comes before the lookup, which the test already pins with AssertNotCalled or no-write checks.	relaxUnreachedStubs, in those tests only.
API-key list and refused-create tests	GetGroup, CreateAPIKey (and GetUserByID for the zero cap)	Listing creates nothing. A refused cap stops before the owner is read or a key is stored; the existing *stored == nil check pins the second part.	relaxUnreachedStubs, in those tests only. The create tests keep all three required.

relaxUnreachedStubs(m *mock.Mock, methods...) lives in handler_accounts_test.go. It marks a shared fixture's expectations for the named methods .Maybe().Times(0), which also clears a .Once() count. It is the same idea as optionalCalls in internal/purchase (#1570), under a name that will not collide with part B.

Left unasserted on purpose

These mocks have only .Maybe() expectations, so AssertExpectations would check nothing:

• apgCaller.handlerAndReq: part-B files call it, so its signature stays.
• newAuthFreeTxHandler, newMePermsAuth, meScopeAuth and apgConfigCall.
• bastion_resolve_test.go, which uses Fn fields only.

After this change, 26 functions still register .On without asserting in the same function. Each is one of these .Maybe()-only helpers, or a helper whose mocks the caller or the constructor asserts. At file level, 2 of the 39 files are left: discover_org_scope_test.go (asserted via scopedHandler) and handler_auth_me_permissions_apikey_test.go (.Maybe() only).

Refs #35

Verification

All of this is unit, mock and fixture evidence. There is no real-account path involved.

• go build ./..., go vet ./..., go vet -tags integration ./...: clean.
• go test ./internal/api/ -count=1: ok.
• go test -race ./internal/api/ -count=1: ok, no data races.
• go test -race ./internal/api/ -count=4: ok. This checks that the cleanups do not flake against async calls.
• go test ./internal/mocks/ -count=1: ok.
• Changed integration tests only, on Postgres via testcontainers: go test -tags integration -count=1 -timeout 20m -run '^(TestAnalyticsAccountIDs_SumsSelectedAccountsOnly_DB|TestCombinedPlanPurchase_ManualOffRoutedApproval|TestCombinedPlanPurchase_NonUSDApprovalRefusedUnderCap|TestCombinedPlanPurchase_AutomaticOnScheduledExecution|TestCombinedPlanPurchase_AutomaticAttachmentOverCapRefuses|TestCombinedPlanPurchase_PlannedProjectionReportsUnits|TestUpdateConfigPropagatesOnlyChangedDefaults|TestUpdateConfigPaymentDefaultSkipsAzureAndGCP|TestUpdateServiceConfig_ConcurrentPartialPutsKeepBothKeys|TestUpdateServiceConfig_ConcurrentFirstSavesKeepBothKeys|TestUpdateServiceConfig_ParallelSettingsSaveDoesNotExhaustPool)$' ./internal/api/: all 11 pass.
• golangci-lint run --allow-parallel-runners ./internal/api/, with and without --build-tags integration: 0 issues.
• No non-test files changed, so gocyclo does not apply.

Mutation probes. Each production change was applied temporarily to origin/main and to this branch, the package was run with go test -count=1 ./internal/api/, and the change was reverted. The tests listed below survive on main and fail here:

Probe	Tests that survive on main but fail on this branch
revokeAPIKey skips RevokeAPIKeyAPI and returns success	TestHandler_revokeAPIKey_Success
deleteAPIKey skips DeleteAPIKeyAPI	TestHandler_deleteAPIKey_Success
forgotPassword skips RequestPasswordReset	TestHandler_forgotPassword_Success, TestHandler_forgotPassword_ErrorStillReturnsSuccess
analytics trends skips QueryByService	main: 1 test fails. Branch: 6, including TestHandler_getAnalyticsTrends_MonthlyCommitmentAndUsage and the account_ids/scope endpoint matrices through analyticsPathQueries

On main, these were caught only by error-path tests elsewhere, if at all. The success-path tests now prove the call happened.

Combined-tree verification. Tested on a local, never-pushed tree: queue/platform @ 594ac3ec = origin/main ae6854b3 plus:

Branch	Tested head
fix/issue-357-delete-cleanup-lambda	56989d8
fix/issue-36-archera-gate	2d22986
fix/issue-35-assert-api-a	f793689
fix/issue-35-assert-api-b	fac72f3
fix/issue-120-stale-rec-sweep	07c2e9c
fix/issue-981-summary-sql	7056746

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.

• 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 (-tags integration -count=1, one package at a time): internal/api, internal/auth, internal/config, internal/scheduler, internal/server and internal/testutil pass.
• Secrets: git-secrets on the changed files found nothing, and a planted fake key was flagged; scripts/test-git-secrets-allowlist.sh passes.

Out of scope

• Part B (internal/api files after handler_config_test.go, internal/auth, internal/scheduler, internal/server, internal/email): separate branch. TEST-03: 19 Go test files register .On() expectations without mock.AssertExpectations, incl. money-path regression suite #35 stays open until both parts merge.
• Production code: no changes.
• The review note on TEST-03: 19 Go test files register .On() expectations without mock.AssertExpectations, incl. money-path regression suite #35 that handler_coverage_test.go asserts nothing about response bodies is a separate issue. That file is in part B's range anyway.

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

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• 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 f79368914c109bca1149d6c164ecef9b12612e71

Blocking:

• none

Non-blocking:

• internal/api/handler_accounts_test.go:63: relaxUnreachedStubs is a package-wide helper living in an accounts test file. It is called from 7 other files, so a neutral home (for example mocks_test.go, once part B has merged) would make it easier to find. The .Times(0) reset is correct for testify (0 means unlimited, which clears a .Once()).
• Two part-B helpers that this half's tests call (historyScopeIDs, newTokenApprove) start asserting only when test(api,auth,scheduler,server,email): assert mock expectations in the remaining tests #1594 lands. I ran the halves merged together (below) and they pass, so the order the two merge in doesn't matter.
• This is a partial fix, and the PR correctly says Refs #35. TEST-03: 19 Go test files register .On() expectations without mock.AssertExpectations, incl. money-path regression suite #35 can close only after both test(api): assert mock expectations in the first half of the api tests #1593 and test(api,auth,scheduler,server,email): assert mock expectations in the remaining tests #1594 merge.

Attacked:

• Read the whole diff (40 test files, no non-test file changed). Searched every removed line for weakened assertions. The only removals are helper-signature changes (hostCheckHandler, setupRouterForDispatch, sessionApproveHandler, tokenApproveHandler now take t) and the stale GetExecutionByID/SavePurchaseExecution/SaveServiceConfig stubs listed in the body. TestHandler_approvePurchase_EmptyToken is now stronger: it asserts a 401 and that the store is never read. The .Maybe() changes in analyticsParamsCall are paired with a new 200-path check (analyticsPathQueries plus AssertCalled GetAllowedAccountsAPI), so the scope tests still prove the queries ran.
• Fail-before (mutation): I made forgotPassword skip RequestPasswordReset and revokeAPIKey skip RevokeAPIKeyAPI. On origin/main (ae6854b), TestHandler_forgotPassword_Success and TestHandler_revokeAPIKey_Success both PASS. On this head both FAIL. Restored afterwards.
• Ran: go build ./..., go vet ./..., go vet -tags integration ./... (clean); go test -race -short -count=1 ./internal/api/ ./internal/mocks/ (ok). Re-counted the in-range files: 39 have .On(, and 2 still lack AssertExpectations (discover_org_scope_test.go, handler_auth_me_permissions_apikey_test.go), as the body says.
• Combined tree (my own local merge of test(api): assert mock expectations in the first half of the api tests #1593 + test(api,auth,scheduler,server,email): assert mock expectations in the remaining tests #1594 + perf(api): page recommendations with every list filter in SQL and an aggregate summary #1596 onto ae6854b, never pushed): build/vet clean; go test -race -short -count=1 ./internal/... all ok; go test -tags integration -count=1 for internal/api (378s), internal/scheduler, internal/auth and internal/server all ok.

Body-stale:

• none

_{Reviewer: rQ4. 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 f79368914c109bca1149d6c164ecef9b12612e71

Blocking:

• none

Non-blocking:

• internal/api/handler_accounts_test.go:63-66 relaxUnreachedStubs sets .Maybe().Times(0). Times(0) makes the stub optional and also unlimited. That is no weaker than main, where these stubs were never asserted. But where a comment says a call is "never reached" and nothing else pins it, a regression that starts making the call would still pass. Examples: TestGetAccount_InvalidUUID (:482, "refused before the session is read") and the direct-gate calls in account_scope_shared_name_test.go:232,285, execution_scope_test.go:423 and handler_accounts_hostcheck_test.go:198,216. An AssertNotCalled there, or not registering the stub at all, would turn those comments into checks. This is a possible follow-up, not a gap this PR opens.
• One commit of about 660 changed lines, over the team's 400-line commit guideline. It is mechanical and test-only, so this does not matter if the PR is squash-merged.

Attacked:

• Read the full three-dot diff. No non-test files changed. Every non-mechanical hunk keeps or tightens behaviour: removing a stub turns an unasserted call into a panic, and .Maybe() keeps what main already allowed. The rewritten checks are the 401 in TestHandler_approvePurchase_EmptyToken, the save-exactly-once/never check in CommitmentOptsOutsideTransaction (each case also asserts its own error code), and analyticsPathQueries with ElementsMatch on served requests only. The file counts in the body are right: 22 of 39 in-range files had no assert on main, and 2 remain.
• Fail-before (my own probe, not one from the PR): createAPIKey stops calling the rate limiter (if false && h.rateLimiter != nil). On main only TestHandler_createAPIKey_RateLimited fails. On this head TestHandler_createAPIKey_Success fails too, because its AllowWithUser expectation is now asserted. The file was restored afterwards.
• On the head: go build ./..., go vet ./internal/... with and without -tags integration, go test -race -short -count=1 ./internal/api ok, go test -tags integration -count=1 ./internal/api ok (whole package, 212s), go test ./internal/mocks ok (the vacuous-assertion guard), golangci-lint v2.10.1 with and without --build-tags integration: 0 issues.
• Combined with test(api,auth,scheduler,server,email): assert mock expectations in the remaining tests #1594 (they touch no common files and merge cleanly): -race -short on api/auth/scheduler/server/email/mocks and -tags integration on api/scheduler/auth/server/email all pass.

Body-stale:

• none. TEST-03: 19 Go test files register .On() expectations without mock.AssertExpectations, incl. money-path regression suite #35 closes only when this PR and test(api,auth,scheduler,server,email): assert mock expectations in the remaining tests #1594 have both merged. "Refs TEST-03: 19 Go test files register .On() expectations without mock.AssertExpectations, incl. money-path regression suite #35" is correct.

_{Reviewer: rQ3. 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 f79368914c109bca1149d6c164ecef9b12612e71 merged onto main (ae6854b3), together with #1593, #1594 in that order (combined tree f666a447, 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 f7936891 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly f79368914c109bca1149d6c164ecef9b12612e71.