[IAmJSD]

Root cause

federationHandler() in internal/api/handler_federation_test.go built a MockAuthService with a ValidateSession stub but never registered AssertExpectations. That meant an On(...) stub that the code under test never called still passed silently, in every federation IaC test that uses the helper (36 call sites across handler_federation_test.go and exchange_helpers_test.go). federationHandlerWithEmail in handler_federation_controlchar_test.go is a copy of the helper with a different session email, and it had the same gap.

Fix (test-only)

• federationHandler(t *testing.T) now registers t.Cleanup(func() { mockAuth.AssertExpectations(t) }) on a plain local mockAuth, and every call site passes t. Subtests pass their own subtest t.
• The shared wiring (the CUDLY_SOURCE_CLOUD default, grantAdmin, NewHandler and the pre-warmed source identity) moved into a new newFederationHandler(mockAuth). It registers no session stub, so a test can choose its own auth expectations.
• federationHandlerWithEmail(t, email) gets the same AssertExpectations registration and reuses newFederationHandler.
• The ValidateSession stub is still a hard expectation. Nothing was blanket-.Maybe()'d. The permission stubs that grantAdmin registers were already .Maybe() and are unchanged, for the reason documented on grantPermissionsScoped.

Tests that needed changes once expectations were asserted

Test	What happened	What changed
TestGetFederationIaC_RequiresAuth	It sends no Authorization header, so requirePermission rejects it with a 401 before any session lookup. The admin-token stub it inherited was never called, and the test failed with FAIL: ValidateSession(string,string).	Not .Maybe(). Skipping the call is the behavior under test, so the test now builds its handler with newFederationHandler and no session stub. It registers AssertExpectations and also asserts mockAuth.AssertNotCalled(t, "ValidateSession", mock.Anything, mock.Anything). This is stronger than before.

Every other federation test, including all federationHandlerWithEmail calls (auth runs first in getFederationIaC, even for combinations that aren't offered), already calls ValidateSession. So no other test needed .Maybe() or a stub removed.

Verification

All of this is unit/mock evidence. The change is test-only, and no real-cloud path is exercised.

Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because the local ~/.aws/config default profile on the dev machine is invalid.

go vet ./internal/api/                                   # clean
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./internal/api/ -count=1
  ok  github.com/LeanerCloud/cloud-commitments-platform/internal/api
go test ./internal/mocks/ -count=1                       # TestNoUnfailableMockAssertions and the other guards
  ok  github.com/LeanerCloud/cloud-commitments-platform/internal/mocks
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/api/...   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/api/       # silent
git-secrets --scan (fresh git init copy of tracked files, isolated HOME, scripts/setup-git-secrets.sh patterns)  # clean
bash scripts/test-git-secrets-allowlist.sh               # 109 PASS, 0 FAIL

Fail before / pass after. I added a probe stub that is never matched, h.auth.(*MockAuthService).On("ValidateSession", mock.Anything, "probe-never-sent").Return(nil, assert.AnError), to TestHandler_getFederationIaC_SuccessWithoutAccountID. I applied the same text to a copy of origin/main (5d4ab6b) and a copy of this branch, and then removed it. It is not in the PR.

• main + probe: --- PASS: TestHandler_getFederationIaC_SuccessWithoutAccountID (the unmatched stub goes unnoticed)
• this branch + probe: handler_federation_test.go:45: FAIL: ValidateSession(string,string) / FAIL: 4 out of 5 expectation(s) were met. / --- FAIL

Also, once AssertExpectations was registered and before TestGetFederationIaC_RequiresAuth was reworked, that test failed with FAIL: ValidateSession(string,string). That was a dead stub the old helper had been hiding.

Out of scope (noted, not changed)

• newFederationHandler still defaults CUDLY_SOURCE_CLOUD with os.Setenv, not t.Setenv, so the value persists across tests in the process. That is the existing behavior. Switching to t.Setenv would change test isolation and interact with t.Parallel, so it belongs in its own change.

Labels

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

Closes #44

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

Or wait 58 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: 2f1dbca
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 9e837a54-7add-4a32-aa2b-5e5b5abb58c2

Commits

Reviewing files that changed from the base of the PR and between 2f1dbca and e70dfba.

Files selected for processing: 3

• internal/api/exchange_helpers_test.go
• internal/api/handler_federation_controlchar_test.go
• internal/api/handler_federation_test.go

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

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

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE e70dfba

Every test still tests what its name says, and nothing was loosened to make the new assertions pass. The new assertion also catches a dropped session lookup that the old tests missed.

Findings (none blocking):

• [non-blocking, possible follow-up] Four federation tests still build their own MockAuthService and never call AssertExpectations:
  • internal/api/handler_federation_test.go:809 (TestGetFederationIaC_PreservesPlusInSessionEmail)
  • internal/api/handler_federation_test.go:836 (TestGetFederationIaC_NoSessionEmail_ShipsBundleWithEmptyContact)
  • internal/api/handler_federation_test.go:1793 (TestGetFederationIaC_BundleTfvarsEscapesSessionEmail)
  • internal/api/handler_federation_azure_escape_test.go:132 (TestGetFederationIaC_AzureDeployScriptEscapesSessionEmail)
  • Each asserts that the session email appears in the output, so a skipped ValidateSession already fails them.
  • The PR title covers only the federationHandler mock, so this is outside its scope.

Attacked:

• No loosening:
  • The diff adds no .Maybe(), .Times or new mock.Anything.
  • The only .Maybe() match is in the new comment (handler_federation_test.go:41), which describes the existing, unchanged grantPermissionsScoped stubs (mocks_test.go:480-490).
  • ValidateSession is now a hard expectation.
  • The 400-path tests (MissingParams, RejectsEmptyFormat, RejectsUnknownFormat, InvalidSource, MissingTarget) still pass, which is correct: getFederationIaC calls requirePermission before federationIaCParams (handler_federation.go:186-194).
  • TestGetFederationIaC_RequiresAuth now uses a mock with no ValidateSession stub plus AssertNotCalled, which matches the 401 path (handler.go:290-292).
  • SubjectClaimThreaded builds the handler in the parent test and uses it in subtests. That is fine, because cleanup runs on the parent's t.
• Mutation probes, on scratch copies of head and base, each restored and confirmed identical with cmp:
  • Probe 1, the session lookup skipped: requirePermission was replaced with a fixed admin@example.com session.
    • On head, 12+ federationHandler tests FAIL on the unmet ValidateSession, among them CFNZip, Bundle_*, AzureARM/Bicep, ARMMatrix and SubjectClaimThreaded.
    • On base, only RequiresAuth and the session-email tests fail. The others passed silently there, because the hard-coded email matched the stub. That is the gap this PR closes.
  • Probe 2, params checked before auth: federationIaCParams moved above requirePermission.
    • On head, MissingParams, InvalidSource, RejectsEmptyFormat, RejectsUnknownFormat and MissingTarget FAIL.
    • On base, all of them pass.
    • So the new assertions also guard the auth-then-params order.
  • Probe 3, ContactEmail hard-coded: ControlChar, PreservesPlus, NoSessionEmail, BundleTfvarsEscapes and AzureDeployScriptEscapes FAIL. PrefillContactEmailFromSession passes only because the probe hard-coded the same email its stub returns.
• Other mocks: the MockConfigStore instances register no stubs, so there is nothing to assert, and an unexpected call would panic.

Verified locally on the head export, with GOTOOLCHAIN=go1.26.10 and no cloud credentials, under the heavy lock:

• gofmt -l on the 3 changed files: clean.
• go build ./... and go vet ./internal/api/: ok.
• go test -count=1 ./internal/api/: ok.
• git merge-tree --write-tree origin/main e70dfba1: clean.
• No migrations.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: APPROVE e70dfba

Base: main (not stacked). git merge-tree --write-tree origin/main e70dfba1 (origin/main 8f186a8): clean, no conflicts. The merged tree has no remaining zero-arg federationHandler() or one-arg federationHandlerWithEmail(...) callers, and the federation tests pass on it.

Fixes #44: yes. federationHandler(t) registers t.Cleanup(mockAuth.AssertExpectations(t)), and every call site passes t. The sibling copy federationHandlerWithEmail got the same fix, and the shared wiring moved to newFederationHandler. ValidateSession stays a hard expectation. Only the grantAdmin permission stubs stay .Maybe(), and they were already optional.

Findings (none blocking):

• nit internal/api/handler_federation_test.go:171 - mockAuth.AssertNotCalled(t, "ValidateSession", ...) is mostly redundant. With no On("ValidateSession") registered, an unexpected call already panics in testify and fails the test. It does no harm and documents intent.
• nit internal/api/handler_federation_test.go:58 - newFederationHandler still sets CUDLY_SOURCE_CLOUD with os.Setenv, which leaks across tests. This is pre-existing, and the PR body notes it as out of scope.

Attacked:

• Read the full diff (3 files, 67+/46-) against issue test(api/federation): register mock.AssertExpectations on federationHandler() mock #44. All 36 call sites are converted. Subtests that build a handler pass their own t, and handlers built at test level outlive their subtests because Cleanup runs after the subtests finish.
• TestGetFederationIaC_FailsClosedOnControlChar builds handlers inside the loop, including for combinations that are not offered. It passes with hard expectations, which confirms auth runs before the combination check on every path.
• Ran go test ./internal/api/ -run 'Federation|ControlChar|Rendered|BuildAzure|RenderTemplate|SingleFile' on the head and on the merged tree: ok. go vet ./internal/api/: ok. go test ./internal/mocks/ (mock-assertion guards): ok.
• Mutation probe 1: added an unmatched ValidateSession(..., "probe-never") stub in federationHandler. TestGetFederationIaC_MissingParams FAILS at handler_federation_test.go:45. Restored.
• Mutation probe 2: probe 1 plus removing the new t.Cleanup line. The same test PASSES, which shows the Cleanup registration is what catches it. Restored.
• Mutation probe 3: added an unmatched stub in federationHandlerWithEmail. TestGetFederationIaC_FailsClosedOnControlChar FAILS. Restored.
• Scope: test-only. No production code is touched, and no t.Parallel interaction exists in the touched files.

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• internal/api/handler_federation_test.go TestGetFederationIaC_RequiresAuth: with no On(...) registered, the AssertExpectations cleanup there always passes. The real guard is the AssertNotCalled(ValidateSession), and that is enough. Note that an unexpected call on a testify mock with no stub panics before the assertion line is even reached, so the test was already strict. The cleanup is harmless consistency.
• (From an earlier review, which I confirmed.) Four federation tests still inline a copy of the helper with no AssertExpectations: handler_federation_test.go:807 (PreservesPlusInSessionEmail), :834 (NoSessionEmail_ShipsBundleWithEmptyContact), :1789 (BundleTfvarsEscapesSessionEmail) and handler_federation_azure_escape_test.go:132. They don't call federationHandler(), so test(api/federation): register mock.AssertExpectations on federationHandler() mock #44's literal scope is met, but each could just become federationHandlerWithEmail(t, email). Good follow-up.
• newFederationHandler still uses os.Setenv for CUDLY_SOURCE_CLOUD. This is pre-existing and acknowledged in the PR body.

Attacked:

• Fail-before: added a never-matched probe stub (On("ValidateSession", mock.Anything, "probe-never-sent")) to TestGetFederationIaC_InvalidSource. On this head the test FAILs at the cleanup (FAIL: ValidateSession(string,string), 4 out of 5 expectation(s) were met). With the three test files reverted to the merge-base 5d4ab6b and the same probe applied, it PASSes. So the gap in test(api/federation): register mock.AssertExpectations on federationHandler() mock #44 is real and is now closed. Reverted afterwards.
• Checked that the stub stays a hard expectation (no new .Maybe()), that subtests that share a parent-built handler (AWSWIF_SubjectClaimThreaded) register cleanup on the parent t and so assert after all subtests, and that the control-char helper makes a new handler per call, each with its own cleanup.
• Rolling-merge safety: git grep 'federationHandler()' on current origin/main finds callers only in the three files this PR edits, which main hasn't touched since the merge-base. git merge-tree --write-tree origin/main HEAD is clean, so a newer main caller can't break the build on merge.
• Commands: go build ./..., go vet ./internal/api/ with and without -tags integration, go test -race -short -count=1 ./internal/api/ ./internal/mocks/ (both ok).

Body-stale: none

_{Reviewer: rB3. 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 e70dfba1120086974f210003ccf6dad45eb11149 merged onto main (5aa3df5b), together with #1155, #1219, #1221, #1223, #1224, #1225, #1226, #1227, #1256, #1262, #1266, #1270, #1272, #1275, #1283, #1286, #1293, #1297, #1299, #1300, #1301, #1304, #1307, #1311, #1313, #1325 in that order (combined tree f3a3331f, 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 e70dfba1 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly e70dfba1120086974f210003ccf6dad45eb11149.