[IAmJSD]

Root cause

Three test doubles implemented the whole of config.StoreInterface (145 methods) by hand: internal/mocks.MockConfigStore, internal/analytics.mockConfigStore and internal/server.mockConfigStoreForHealth. Each new store method therefore needed a stub in all three places. Nothing stopped a fourth copy from appearing.

Fix (first slice)

• internal/analytics/collector_test.go: mockConfigStore now embeds mocks.MockConfigStore and overrides only GetActivePurchaseHistory, the one config-store read the collector makes (collector.go:96). The default stays nil, nil, as before. 527 lines of stubs are gone. The four other func fields (getPurchaseHistoryFunc, getAllPurchaseHistoryFunc, getPurchaseHistoryByPurchaseIDFunc, markPurchaseRevokedFunc) were never set by any test, so they go too. No test call site changed.
• internal/mocks/config_store_copies_guard_test.go (new): a repo-wide syntactic guard, TestNoHandRolledConfigStoreMocks. It reads the StoreInterface method set from internal/config/interfaces.go. It fails when any type declared in a _test.go file defines more than half of those methods. Today the largest partial override defines 24 of 145 (internal/server.mockConfigStoreForExchange), and the threshold is 73, so there is plenty of margin. internal/server.mockConfigStoreForHealth is allowlisted until it is folded. The allowlist can only shrink: the guard fails if an entry no longer hand-rolls the interface or no longer exists.

No production code changed. internal/mocks/stores.go is untouched, to keep conflicts with the open branches that add store methods to a minimum.

Refs #76

Verification

All of this is unit/mock evidence. The change is test-only.

export PATH=$HOME/.local/go/bin:$HOME/go/bin:$HOME/.local/bin:$PATH
export AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null
go build ./...                                  # ok
go vet ./...                                    # ok
go vet -tags integration ./...                  # ok (analytics integration tests still compile against the new mock)
go test ./internal/mocks/ -count=1              # ok
go test ./internal/analytics/ -count=1          # ok
golangci-lint run --allow-parallel-runners ./internal/analytics/ ./internal/mocks/                     # 0 issues
golangci-lint run --allow-parallel-runners --build-tags integration ./internal/analytics/            # 0 issues

Fail-before / pass-after for the guard:

• With main's internal/analytics/collector_test.go restored: go test ./internal/mocks/ -run TestNoHandRolledConfigStoreMocks FAILS and reports internal/analytics.mockConfigStore as a hand-rolled config.StoreInterface test double.
• With this branch: PASS.

Mutation probes:

• Replacing the allowlist entry with a nonexistent type makes the guard FAIL twice: once for the stale entry, and once because mockConfigStoreForHealth is now reported as a copy.
• Deleting the GetActivePurchaseHistory override from the analytics wrapper makes TestCollectorCollect FAIL with a panic. The shared mock dispatches that method through m.Called without a default, so the wrapper's override really is what the collector tests exercise.

No non-test Go files changed, so gocyclo has nothing to check.

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

Branch	Tested head
fix/issue-47	62feb4f
fix/issue-76	3b8c241
fix/issue-1233	e1638b7
fix/issue-153	8729074
fix/issue-532	0971624
fix/issue-64	fefdc43
fix/issue-54	91827b9

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 (changed packages): golangci-lint v2.10.1 found 0 issues, with and without --build-tags integration, and gocyclo -over 10 is silent on the changed non-test files. These ran at b61a9c30, which differs from 77d116c6 only by one .gitallowed line on fix/issue-1233.
• Integration tests, every changed package, one package at a time (-tags integration -count=1):
  • These packages pass: cmd/cleanup-lambda, internal/analytics, internal/auth, internal/database/postgres/migrations and internal/purchase.
  • internal/api passes, except two tests whose Postgres container didn't start, or whose migrations hit a timeout, under machine load. Rerun alone on the same tree, both pass: TestDeleteManagedPlanByExecutionStatus and TestNotifiedRow_RescheduleDoesNotOverwriteConcurrentCancel.
  • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
  • Packages were run with a 60-minute timeout (rather than CI's 20) because the VM was shared with other test runs. Some reruns used TESTCONTAINERS_RYUK_DISABLED=true to avoid a testcontainers reaper race under load.
• 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: 137 PASS, 0 FAIL.
• Nothing was applied to any cloud.

Out of scope / remaining for #76

• internal/server/test_helpers_test.go mockConfigStoreForHealth (145 methods, about 520 lines) still needs folding. It is left for a follow-up to keep this diff small. It needs more care than the analytics copy:
  • Roughly 60 MockConfigStore methods (for example GetServiceConfig, SaveGlobalConfig, GetPendingExecutions, GetRIExchangeDailySpend, ReserveRIExchange) call m.Called unconditionally, so they panic when no expectation is set.
  • The health mock returns benign values instead, and several server tests and embedders (handler_ladder_test.go, handler_ri_exchange_test.go) rely on that implicitly.
  • The follow-up has to keep overrides for every such method the server tests reach, and must not let a recovered panic turn into an expected error path.
  • When it lands, delete its entry from handRolledConfigStoreAllowlist and change this to Closes #76.
• Audit finding A15-007 (the duplicated Azure MockHTTPClient, under providers/azure/...) belongs to the cloud-commitments-go repository. That code does not exist in this repo, so it is not addressed here.

Maintainer: please mirror the issue's triage labels on this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/this-quarter, effort/m.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 2 billable files and costs up to $0.50.}

Or wait 59 minutes for your next included review.

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: a59740d
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 98215a87-b56f-4951-91aa-893a06a199da

Commits

Reviewing files that changed from the base of the PR and between a59740d and 091c979.

Files selected for processing: 2

• internal/analytics/collector_test.go
• internal/mocks/config_store_copies_guard_test.go

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

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

[IAmJSD]

Merged origin/main into this branch twice: first 8f186a8, then c1f1781, because main moved while the first merge was being checked. New head: 9e7fd37.

Conflict resolution (both merges, same file):

• internal/analytics/collector_test.go: main kept adding stubs to the hand-rolled analytics mockConfigStore:

  • HistorySummaryAggregate (feat(api): scope-aware SQL totals for the History summary cards #1200)
  • ClaimCollectionInvocation (fix(scheduler): scope the collection marker release to the invocation, not the marker #1099)
  • the ReplaceRecommendations removal (fix(db): remove ReplaceRecommendations and its unscoped DELETE #1209)
  • CancelExecutionAtomicTokenPinned (sec(purchase): pin the approval token in the token approve/cancel CAS; compare revoke token first #1242)
  • the pgx import those stubs need

  This branch replaces that mock with one that embeds mocks.MockConfigStore, which already has every one of those methods on main. I kept this branch's version, and main's stub edits drop out. This is the duplicate-stub churn test: consolidate hand-rolled ConfigStore mocks (3 copies, taxes every new store method) #76 is about.

Checks on the merged head:

• build, vet (also with -tags integration) and e2e vet pass.
• golangci-lint found 0 issues with and without the integration tag on analytics and mocks.
• go test -race -short passes on both packages. That includes TestNoHandRolledConfigStoreMocks, which still finds no new hand-rolled copies on main.
• go test -race -tags integration ./internal/analytics passes.
• The secrets scan is clean and the planted key was flagged. The allowlist test passes.

[IAmJSD]

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

Blocking:

• internal/analytics/collector_test.go: the branch conflicts with main (GitHub shows CONFLICTING/DIRTY). Since the merge base, main's fix(marketplace): recover pending listings through cancel and never record an empty or revived state #1299 added GetMarketplaceListingAttempt and ResolveMarketplaceListingAttempt stubs to the hand-rolled mockConfigStore, and main also dropped GetPendingExecutionsTx there, in the very block this PR deletes. GitHub won't merge it until that's resolved. The resolution is easy: take this branch's side of the file, since the embedded mocks.MockConfigStore on main already has both new methods. I checked that resolution on top of origin/main: go vet (plain and -tags integration) passes on analytics and mocks, go test -race -short on analytics and mocks passes, and TestNoHandRolledConfigStoreMocks passes. Please merge origin/main with that resolution. Apart from the conflict, the content is approvable as is.

Non-blocking:

• The guard is a heuristic: it only catches a copy that defines more than half of StoreInterface's 147 methods on one type in a _test.go file. A copy split across embedded helper types, or one in a non-test helper file, would get past it. That's fine as a tripwire, and the allowlist-can-only-shrink checks are a nice touch.

Attacked:

• Analytics: the collector's only config-store read is GetActivePurchaseHistory, and the override keeps its nil, nil default. Every other method now comes from mocks.MockConfigStore, which panics through m.Called if it is reached unexpectedly, so a new collector read would show up loudly rather than silently returning nil. The four removed func fields have no setters anywhere, including integration-tagged files (go vet -tags integration is clean).
• Guard fail-before: with main's collector_test.go restored, TestNoHandRolledConfigStoreMocks fails and names internal/analytics.mockConfigStore. With the PR's version it passes. Restored afterwards. StoreInterface has no embedded interfaces, so the parsed method set is complete.
• Ran: go build ./..., go vet (with and without -tags integration) on analytics and mocks, golangci-lint v2.10.1 (both tag sets, 0 issues), go test -race -short -count=1 on analytics and mocks, and go test -race -tags integration -count=1 ./internal/analytics/ (real Postgres). All pass at the head. Then git merge-tree against origin/main, which conflicts as described above.

Body-stale:

• "Fix (first slice) ... 527 lines of stubs are gone": the stub block on main has since changed (two methods added, one removed), so after the merge the count differs slightly. Not material.

_{Reviewer: rB12. 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 current main into the branch. New head: 091c979.

• internal/analytics/collector_test.go conflicted. Main changed stubs in the hand-rolled mockConfigStore that this branch deletes: fix(marketplace): recover pending listings through cancel and never record an empty or revived state #1299 added GetMarketplaceListingAttempt and ResolveMarketplaceListingAttempt, and chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217 removed GetPendingExecutionsTx. I kept the branch's version, the embedded mocks.MockConfigStore plus the GetActivePurchaseHistory override. Both main PRs made the same changes to mocks.MockConfigStore, so the embedded mock already matches. The file is identical to the previous branch head.
• TestNoHandRolledConfigStoreMocks still passes against main's current StoreInterface, and the mockConfigStoreForHealth allowlist entry is still valid.

Checks:

• go build and go vet (default, integration, e2e) pass. golangci-lint reports 0 issues with and without the integration tag.
• go test -race passes on internal/analytics and internal/mocks. The internal/analytics integration tests also pass with -race.
• The git-secrets scan is clean, and the allowlist test gives 137 PASS.

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• Merge-order note: feat(api): page the History view with a keyset cursor (has_more, next_cursor) #1277 (History paging) adds ListHistoryExecutionsPage/ListPurchaseHistoryPage stubs to this same mockConfigStore in internal/analytics/collector_test.go. Whichever PR merges second will conflict there. If this one lands first, the right resolution is to drop feat(api): page the History view with a keyset cursor (has_more, next_cursor) #1277's stubs, since the embedded mocks.MockConfigStore gets them from feat(api): page the History view with a keyset cursor (has_more, next_cursor) #1277's stores.go change. The same applies to any other open branch that stubs new store methods in the analytics copy.
• internal/mocks/config_store_copies_guard_test.go: the guard only counts methods declared directly on one named type. A copy split across an embedded helper type, or one built from method values, would slip past. That is fine for a tripwire, but worth a comment so nobody reads it as exhaustive.
• The body cites 145 methods and a threshold of 73. StoreInterface has 148 methods on this head, so the threshold is 75. The margin argument still holds.

Attacked:

• Panic risk: switching to the shared mock means every method except the override now goes through m.Called, which panics when no expectation is set. I checked that the collector reads the config store only through GetActivePurchaseHistory (internal/analytics/collector.go:96). mockConfigStore is used only in collector_test.go. The analytics integration tests compile and pass against it.
• Ran: go build ./..., and go vet on analytics and mocks with and without -tags integration (ok). go test -race -short -count=1 ./internal/analytics/ ./internal/mocks/ passed, go test -race -tags integration -count=1 ./internal/analytics/ passed, and golangci-lint reported 0 issues.
• Fail-before: with main's collector_test.go restored, TestNoHandRolledConfigStoreMocks fails and names internal/analytics.mockConfigStore. I restored the file afterwards.
• Refs #76 (not Closes) is right: internal/server.mockConfigStoreForHealth is still hand-rolled and on the allowlist.

Body-stale:

• "145 methods" / "the threshold is 73": the counts are now 148 and 75, as above. Otherwise accurate.

_{Reviewer: rB18. 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 091c9797768d6f489a1df830ae89501c05150e5e merged onto main (de56f95b), together with #1114, #1142, #1254, #1274, #1278, #1279, #1280, #1282, #1414 in that order (combined tree e1bf26a0, 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 091c9797, green CI, and the combined-tree verification above. Squash-merged at exactly 091c9797768d6f489a1df830ae89501c05150e5e.