[IAmJSD]

Root cause

newAzureRegionScopedHandler (internal/api/handler_ri_exchange_azure_test.go) hand-wrote a permits closure that copied auth.matchAllRegionsConstraint's all-match rule, then handed its own verdict back to the handler through two mock.MatchedBy expectations. The region property that the tests built on it assert was therefore decided by the test, not by the production matcher. If matchAllRegionsConstraint regressed to containsAny (the any-match bug it was written to fix), every one of those tests stayed green.

Fix (test-only)

• The helper now grants execute:ri-exchange with Constraints{Regions: ["eastus"]} through MockAuthService.grantPermissions. That routes HasPermissionForConstraintsAPI through auth.PermissionsAllowForConstraintSets, so the real matcher evaluates the constraint set the handler actually built.
• The captured pointer is gone. A new submittedConstraintSets(t, mockAuth) reads the constraint set from the mock's recorded calls. It requires exactly one constraint check for user-1 / execute / ri-exchange, so the "which regions did the handler think it touches" assertions still hold.
• The old helper's strict HasPermissionAPI(ctx, "user-1", "execute", "ri-exchange") expectation is still required, now as an AssertCalled in cleanup, because grantPermissions registers the verb check as .Maybe().
• New positive control TestExecuteAzureExchange_RegionScopedCallerAllowedInsideRegion: the same eastus-only grant must allow an exchange whose source and target are both in eastus. Without it, a grant or matcher that denied everything would satisfy the denial tests without testing anything.
• The permittedRegion parameter is dropped (every caller passed "eastus", and unparam flagged it).
• The 13 shape-assertion mock.MatchedBy closures the issue lists as legitimate are unchanged. No production code is changed.

Closes #208

Verification

All evidence below is unit/mock level (MockAuthService plus a testify Azure exchange client mock). No real Azure or AWS calls. Tests were run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because the local machine's default AWS profile is invalid.

Mutation probes. Each one was applied to internal/auth/service_group.go matchAllRegionsConstraint right after the empty-list guard, then reverted. Each test was run on its own with go test ./internal/api/ -count=1 -run 'TestExecuteAzureExchange_<name>$':

Test	no mutation	return containsAny(permRegions, reqRegions) (any-match regression)	return false (deny-all)
SourceRegionOutsidePermissionRefused	PASS	FAIL (money path reached: unexpected CalculateExchange call)	PASS
UnreportedSourceRegionRefused	PASS	FAIL (same)	PASS
RegionScopedCallerAllowedInsideRegion (new)	PASS	PASS	FAIL (403 instead of success)
UnownedSourceDenialsAreIndistinguishable	PASS	PASS	PASS

Before the fix, the origin/main test file under the same containsAny mutation: SourceRegionOutsidePermissionRefused, UnreportedSourceRegionRefused and UnownedSourceDenialsAreIndistinguishable all PASS. That is the gap the issue describes.

UnownedSourceDenialsAreIndistinguishable is expected to be insensitive to the matcher. The ownership gate runs before the constraint check, and the test asserts that two denials look the same, not region enforcement.

CI-equivalent checks (all clean):

go build ./...
go vet ./internal/api/
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./internal/api/ ./internal/mocks/ ./internal/auth/ -count=1   # ok x3
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/api/                                                                # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/api/                                                                               # silent
bash scripts/gosec-hook.sh internal/api/handler_ri_exchange_azure_test.go                                                        # exit 0
bash scripts/test-git-secrets-allowlist.sh                                                                                       # 42 PASS, 0 FAIL
git secrets --scan on the changed file (fresh repo, isolated HOME, repo's setup script + .gitallowed)                            # clean
git diff --check; go mod tidy -diff                                                                                              # clean

No DB, migration or concurrency surface is touched.

Out of scope

• The user-API-key branch of requirePermissionConstraints. HasAPIKeyPermissionForConstraintsAPI is still constant-stubbed by allowConstraintChecks. The issue tracks this separately because the mock needs both key and owner permission sets.
• newAzureExecuteHandler / newAzureExecuteMoneyPathHandler still return a constant true from the constraint check. They model an unconstrained permission and are not region tests, so I left them alone.

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

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 3cdbfad9-0967-4fdd-9f1e-eba515a5a53a

📥 Commits

Reviewing files that changed from the base of the PR and between 2d822f3 and baae966.

📒 Files selected for processing: 1

• internal/api/handler_ri_exchange_azure_test.go

+-----------------------------+
| My other GPU is a Blackwell |
+-----------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)

✨ 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.}

[cristim]

Independent review (claude-cc-platform reviewer, cold brief): APPROVE baae966a030e91b7a9bb9e639ef377a4ee6b7903

No blocking findings. The change is test-only: one file, internal/api/handler_ri_exchange_azure_test.go (+83/-36).

Real matcher: the helper now grants Constraints{Regions:["eastus"]} through MockAuthService.grantPermissions (mocks_test.go:438). The checks reach auth.PermissionsAllowForConstraintSets (service_api.go:484) and then matchAllRegionsConstraint (service_group.go:452), instead of the old hand-written permits closure. The strict HasPermissionAPI requirement stays, asserted in cleanup.

Mutation probes on matchAllRegionsConstraint:

• any-match: SourceRegionOutsidePermissionRefused and UnreportedSourceRegionRefused fail, as the PR's table says;
• deny-all: the positive control fails;
• allow-all: SourceRegionOutsidePermissionRefused fails;
• UnownedSourceDenialsAreIndistinguishable is matcher-insensitive, as the PR says, because the ownership gate runs first.

Claims: accurate. The matcher on main is already all-match, so no production bug was masked.

Determinism: the 4 touched tests pass 20/20 runs each.

Base drift: clean merge onto current main. go test ./internal/api/ -run 'AzureExchange|RequireAzureSource' gives 85 passed at the head and 85 on the merged tree.

Nits:

• In submittedConstraintSets, assert.Equal on args 1-3 inside the loop fails softly, though the following require.Len still catches the count.
• No test pins the matcher's case-insensitivity. That is out of scope, since the handler already lower-cases.
• The PR does not yet carry test(api): newAzureRegionScopedHandler re-implements the all-match region rule instead of asserting it #208's triage labels.

[cristim]

Independent review (claude-fleet): APPROVE baae966

Findings (none blocking):

• internal/api/handler_ri_exchange_azure_test.go:1947 [non-blocking] - submittedConstraintSets checks arguments 1-3 (user, action, resource) but not argument 0 (ctx).
  • The old HasPermissionForConstraintsAPI expectations matched ctx exactly, so a call with the wrong context would have panicked.
  • Failure scenario: the handler passes a detached or background context to the constraint check, and no test notices.
  • The cleanup AssertCalled still pins ctx for the verb check.
• internal/api/handler_ri_exchange_azure_test.go:1994,2062 [non-blocking] - Both region-scoped denial tests send a constraint set with two regions. No test denies a request that names a single foreign region.
  • A probe that makes the matcher return true when len(reqRegions) <= 1 kept all 4 tests green.
  • Failure scenario: a regression that checks only multi-region requests would let a single-region exchange in a foreign region through.
  • The base file had the same gap, and test(api): newAzureRegionScopedHandler re-implements the all-match region rule instead of asserting it #208 does not require this case, so it is a follow-up.
• internal/api/handler_ri_exchange_azure_test.go:1952-1954 [non-blocking, cosmetic] - The assert.Equal inside the loop fails softly. require.Len(submitted, 1) still fails the test.

Attacked:

• Real matcher: the helper calls grantPermissions (internal/api/mocks_test.go:469) and then grantPermissionsScoped. That registers HasPermissionForConstraintsAPI to return auth.PermissionsAllowForConstraintSets(perms, ...) (mocks_test.go:509-514). The call then reaches internal/auth/service_api.go:521, matchConstraints, and matchAllRegionsConstraint (internal/auth/service_group.go:452). No copy of the rule and no stubbed verdict remain in the test.
• Issue test(api): newAzureRegionScopedHandler re-implements the all-match region rule instead of asserting it #208:
  1. The helper is rewritten, and the constraint sets are still recovered from the mock's call records.
  2. The any-match regression is now caught (probes below).
  3. The 13 shape closures are untouched.
• No production code changed: diff -rq between the base and HEAD exports shows only the test file differs.
• Mutation probes on the production matcher (internal/auth/service_group.go), one test per run, each restored and confirmed identical with cmp:
  • any-match (return containsAny after the empty guard): SourceRegionOutsidePermissionRefused and UnreportedSourceRegionRefused FAIL.
  • allow-all (return true): the same two FAIL.
  • Regions dimension removed from matchConstraints: the same two FAIL.
  • deny-all (return false): RegionScopedCallerAllowedInsideRegion FAILS.
  • Case-sensitivity (permitted keys upper-cased): RegionScopedCallerAllowedInsideRegion FAILS.
  • Single-region bypass: no test fails (finding 2).
• The base gap is real: the base test file, run against the any-match mutation, passes all 3 of its tests.
• Removed lines, and where each check now lives:
  • The strict HasPermissionAPI match is now the cleanup AssertCalled at :1924, against the real HasPermission.
  • allowAnyAccountScope is covered by grantPermissions registering GetAllowedAccountsAPI.
  • captured is replaced by submittedConstraintSets, which is stricter (exactly one call).
  • The ctx match is lost (finding 1).
• Isolation: no t.Parallel, per-test mocks, and no globals, network or credentials.
• Duplication and over-engineering: the change reuses the existing grantPermissions. Dropping permittedRegion is fine, because every caller passed "eastus".

Verified locally on a git archive export of HEAD, with no cloud credentials:

• go build ./... && go vet ./internal/api/: clean.
• gofmt -l on the changed file: clean.
• go test -count=1 -race -run 'TestExecuteAzureExchange_(SourceRegionOutsidePermissionRefused|RegionScopedCallerAllowedInsideRegion|UnreportedSourceRegionRefused|UnownedSourceDenialsAreIndistinguishable)$' ./internal/api/: 4 PASS.
• go test -count=1 ./internal/api/, under the heavy lock: ok.
• The PR body's mutation table matches these per-test runs.

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

[cristim]

Local verification (macOS, exact head baae966a030e91b7a9bb9e639ef377a4ee6b7903, detached worktree): go build ./..., go test -race -short -count=1 ./... and make lint pass.

Gate status: two independent reviews approve this SHA (posted above), local verification passes, and CI is green. Merge waits on @IAmJSD marking the PR ready (see #974).

[cristim]

Local verification at baae966a030e91b7a9bb9e639ef377a4ee6b7903 merged onto current main (2f1dbcad; merge-tree a9f1b02f; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• Nothing was applied to any cloud.

[cristim]

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at baae966a030e91b7a9bb9e639ef377a4ee6b7903).