[IAmJSD]

Refs #76

Why

internal/server's mockConfigStoreForHealth stubbed all ~150 config.StoreInterface methods by hand. Every new store method needed another stub there. That per-method tax is what #76 is about, and the type was the last entry on handRolledConfigStoreAllowlist in the hand-rolled-copy guard.

What

• internal/server/test_helpers_test.go: mockConfigStoreForHealth now embeds mocks.MockConfigStore. Only two overrides are left, CleanupOldExecutions and GetStaleProcessingExchanges, both returning "nothing". The shared mock panics on those two without an expectation, and the scheduled-task tests here (handleCleanupExpiredRecords, alarmStaleRIExchanges) call them without one. The embedders in handler_ladder_test.go and handler_ri_exchange_test.go compile and pass unchanged.
• internal/mocks/config_store_copies_guard_test.go: handRolledConfigStoreAllowlist is now empty, so TestNoHandRolledConfigStoreMocks rejects any new hand-rolled copy.
• Two commits so each stays under 400 changed lines. The first embeds the mock and drops 68 stubs; the type still defines more than half of the interface, so the guard still passes with the allowlist entry. The second drops the remaining 80 stubs and empties the allowlist.

How I picked the two overrides: before the change, I called every StoreInterface method on both the old hand-rolled type and a bare MockConfigStore through reflection, with zero arguments. 86 methods differed, 75 of them because the shared mock panics without an expectation. I then ran the internal/server suite (unit, and -race -tags integration) against the embedded mock with no overrides. I added back the old default only for methods a test actually reached, and only those two came up. The comparison script was throwaway and is not committed.

What remains on #76

• A15-007 (the duplicated Azure MockHTTPClient in the cache, cosmosdb, database and search client tests) lives in LeanerCloud/cloud-commitments-go (providers/azure), not in this repo, so it needs a PR there.
• The issue body also names internal/analytics/collector_test.go's mockConfigStore. It already embeds mocks.MockConfigStore on main, so nothing is left to do for it.

Verification

• Fail before: the guard with an empty allowlist against main's test_helpers_test.go fails with hand-rolled config.StoreInterface test doubles ...: internal/server.mockConfigStoreForHealth. Main's guard (allowlist still set) against the new helper fails with internal/server.mockConfigStoreForHealth no longer hand-rolls config.StoreInterface; remove it from handRolledConfigStoreAllowlist. Both pass after.
• go build ./... && go vet ./... && go vet -tags integration ./... && (cd tests/e2e && go vet -tags e2e ./...): pass.
• golangci-lint v2.10.1 on ./internal/server/... ./internal/mocks/..., with and without --build-tags integration: 0 issues. No non-test files changed, so gocyclo doesn't apply.
• go test -race -short -count=1 ./internal/server/ ./internal/mocks/: ok, at both commits.
• go test -race -tags integration -count=1 ./internal/server/: on the first full run everything passed except TestEnsureDB_IsLambdaSingleSourceOfTruth (template clone timed out) and TestCheckConfigStore_DBFailureHidesRawError (Postgres container start timed out), both on a heavily loaded shared host. On rerun, the first passed once and the second passed three times in a row.
• Secrets scan: clean (planted key flagged). scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.

🤖 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

_{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 88958969b896befcd1c78bdc9b34933a3c734c92

Blocking:

• none

Non-blocking:

• none

Attacked:

• Behaviour drift: the old type answered every method with a silent default. The embedded MockConfigStore keeps a soft default only for some methods and panics on the rest when no expectation is set. I listed every app.Config.<Method> call in internal/server non-test code (GetGlobalConfig, GetCloudAccount, GetLadderConfigs, PlanAndSaveLadderRun, ClaimCollectionInvocation, ClearCollectionStarted, CleanupOldExecutions, GetStaleProcessingExchanges) and compared the old stub's default with the shared mock's. The only one that changes is GetCloudAccount (nil before, a stub AWS account now), and ladderTestStore overrides it, so the ladder tests behave as before. The two overrides the PR adds are the two methods the shared mock would panic on.
• Hidden panics: the only recover() in internal/server is the migration runner, and net/http's recovery would break the client call in http_test.go. A -v run of the package has no mock-panic or "closest call" output.
• Fail-before: put main's test_helpers_test.go back in the scratch tree and ran the new guard. TestNoHandRolledConfigStoreMocks fails with internal/server.mockConfigStoreForHealth. Restored, it passes.
• Ran: go build ./...; go vet on internal/server and internal/mocks, with and without -tags integration; go test -race -short -count=1 ./internal/server/ ./internal/mocks/ (ok); go test -race -tags integration -count=1 ./internal/server/ (ok, 55s); golangci-lint v2.10.1 on both packages, with and without the integration tag (0 issues). Refs test: consolidate hand-rolled ConfigStore mocks (3 copies, taxes every new store method) #76 is right, since the A15-007 half lives in cloud-commitments-go.

Body-stale:

• none

_{Reviewer: rE2. 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]

Merged origin/main in. New head: 2f7bb0816943101abfc1b662dec32eb54f66042f.

Conflict: internal/server/test_helpers_test.go. Main (#1240) added a hand-written ListStoredRecommendationsChunk stub to the old mockConfigStoreForHealth. That stub returns RecommendationChunk{Done: true}. The embedded mocks.MockConfigStore already returns RecommendationChunk{Done: true}, nil when no expectation is set. So I kept this branch's shared-mock version, still with only the CleanupOldExecutions and GetStaleProcessingExchanges overrides, and dropped the stub. Main's behavior is unchanged.

Checks:

• TestNoHandRolledConfigStoreMocks passes with the empty allowlist.
• build, vet (default, integration and e2e tags), and golangci-lint v2.10.1 on ./internal/server/... ./internal/mocks/... with and without --build-tags integration: 0 issues.
• go test -race -short and go test -race -tags integration on ./internal/server/ ./internal/mocks/: ok.
• Secrets scan: clean (planted key flagged). scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• none. Refs #76 is right: A15-007 lives in cloud-commitments-go, and the analytics mock already embeds the shared one on main.

Attacked: The risk in a refactor like this is that tests keep passing vacuously because MockConfigStore's defaults differ from the old hand-written stubs. I compared the removed stub bodies with the shared mock's defaults: GetGlobalConfig, GetRecommendationsFreshness and WithTx keep the same defaults, and the rest now panic without an expectation, so a call a test no longer covers fails loudly. The only recover() in internal/server is in the migration runner. I then ran go test -race -short -count=1 -v ./internal/server/ ./internal/mocks/ on the merge base and on the head and diffed the sorted PASS/FAIL/SKIP lines. Both have 872 lines and 0 failures, and they are identical apart from t.TempDir names. Fail-before: with the merge-base test_helpers_test.go and this branch's empty allowlist, TestNoHandRolledConfigStoreMocks fails, naming internal/server.mockConfigStoreForHealth. Also ran go build ./..., go vet (plain and integration) on server and mocks, and go test -race -tags integration -count=1 ./internal/server/ (ok).

Body-stale:

• none

_{Reviewer: rE5. 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 2f7bb0816943101abfc1b662dec32eb54f66042f merged onto main (08105c87), together with #1460, #1469 in that order (combined tree 45490995, 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 1 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: 1 approving independent review(s) on 2f7bb081, green CI, and the combined-tree verification above. Squash-merged at exactly 2f7bb0816943101abfc1b662dec32eb54f66042f.