fix(test): MockConfigStore isExpected short-circuit makes 20 AssertNotCalled assertions unfailable

[cristim]

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

Where

• internal/mocks/stores.go:1553-1560 - the isExpected helper, applied at the top of 28 MockConfigStore method bodies
• Proven-vacuous call sites (each confirmed by whole-file absence of any .On("<method>") registration):
  • internal/api/handler_purchases_test.go:2622, 2641, 2661, 2690, 2726, 2804, 3191, 3209, 3227, 3241, 3266, 3306, 3362, 3484 - AssertNotCalled(t, "WithTx"), 14 sites; the file registers .On("WithTx") zero times
  • internal/api/handler_purchases_test.go:4074, 4121, 5682 - "GetUserEmailByID"
  • internal/api/handler_purchases_revoke_test.go:703 - "CancelExecutionAtomic"
  • internal/api/handler_plans_test.go:690 - "UpdatePurchasePlanTx"
  • internal/scheduler/scheduler_test.go:857 - "MarkCollectionStarted"
• ~20 further sites (GetGlobalConfig, ClearCollectionStarted, UpsertNotificationMute, GetCloudAccountByExternalID, DeleteSuppressionsByExecutionTx, ClearRevocationInFlight, GetPurchasePlan, SetRecommendationsCollectionError) are vacuous per test function: the file registers the method somewhere, so only the tests that do not register it are affected
• The proof case: buildSessionCancelHandler at internal/api/handler_purchases_test.go:2494-2515

What

28 MockConfigStore methods begin with if !isExpected(&m.Mock, "X") { return <default> }. isExpected scans m.ExpectedCalls, which is populated only by .On(...). When a test does not register the method, the method body returns its hardcoded default without ever reaching m.Called, so nothing is appended to m.Calls.

AssertNotCalled reads m.Calls. So for any method in the isExpected guard list, in any test that does not register it, AssertNotCalled is structurally incapable of failing. It asserts that an empty slice does not contain an entry, which is true no matter what the production code did.

Failure scenario

Verified concretely against the cancel-permission matrix. buildSessionCancelHandler (handler_purchases_test.go:2494-2515) registers only GetExecutionByID. The RBAC-deny tests then assert AssertNotCalled(t, "WithTx") and AssertNotCalled(t, "CancelExecutionAtomic").

Now suppose cancelPurchase were changed so the RBAC denial path also opened a transaction and stamped cancelled_by / status before returning its error. That is the exact "guard on the primary mutation, unguarded secondary write path" shape that produced PR #1516. Every one of those 14 WithTx assertions would still pass: the mock's default WithTx silently forwards fn(nil), and the default CancelExecutionAtomic returns (true, "canceled", nil). The suite would report a clean permission boundary while an unauthorized write landed on the purchase record.

There is no exploit here today. What is broken is the detector: 20 assertion sites that exist specifically to prove "no write happened on the deny path" cannot detect a write on the deny path.

Why this is a prerequisite, not a cleanup task

Every future claim of the form "fixed, and the regression test proves the deny path does not write" is unverifiable while this holds, because the standard idiom for making that claim in this codebase is exactly AssertNotCalled against a MockConfigStore method in the guard list. This should be treated as a prerequisite for trusting any "fixed with green tests" claim on the purchase, plan or scheduler paths, and fixed before the next such claim is made rather than after.

Fix direction

Either:

• Make the recording unconditional: call m.Called(...) in every method and fall back to the default only on an ErrNotFound-style lookup miss, so m.Calls always reflects what happened; or
• Add a package-level lint (or a test in internal/mocks) that fails when AssertNotCalled names any method appearing in the isExpected guard list, so the vacuous idiom cannot be written.

The first is the real fix; the second is a cheap guard that could land first. After either, re-run the affected suites: some of the 20 sites will start failing, and each failure is a real behaviour that was never verified.

Related

• test: consolidate hand-rolled ConfigStore mocks (3 copies, taxes every new store method) cloud-commitments-platform#76 (test: consolidate hand-rolled ConfigStore mocks) - overlapping surface. That issue is about three duplicate mock implementations taxing new store methods; this one is about the surviving mock's unregistered-method policy making assertions unfailable. Consolidating onto a mock that keeps isExpected would carry this defect into all three call sites, so this should be settled before or during that consolidation.
• feat(api): owner-token compare-and-clear for collection in-flight marker (closes #261) #1516 - the secondary-write-path class this test idiom is meant to catch.
• A related MEDIUM from this review (MockConfigStore mixes two incompatible unregistered-method policies, so adjacent AssertNotCalled lines are one real and one vacuous with nothing at the call site to distinguish them) is in the grouped chore(test) issue.