[IAmJSD]

Root cause

internal/api/handler_ri_exchange.go built its AWS and Azure clients by calling the concrete cloud-commitments-go constructors itself (awsprovider.NewEC2ClientDirect, awsprovider.NewRecommendationsClientDirect, azurecompute.NewClient) and used the concrete provider types by name. That meant the generic HTTP layer imported providers/aws, providers/aws/recommendations, providers/aws/services/ec2, providers/azure/services/compute, the Azure SDK (armreservations) and the AWS config loader. ri_exchange_audit.go, ri_exchange_refusals.go, ri_utilization_cache.go and the Marketplace handlers (handler_marketplace.go, _attempt.go, _cancel.go) also imported provider packages directly. #1310 already moved the scheduler and purchase paths onto the registry and left this part for a follow-up.

Fix

• New internal/api/provider_wiring.go is now the only non-test file in internal/api that imports a concrete cloud-commitments-go/providers/... package. It contains:
  • constructors that return the existing narrow interfaces: newReshapeEC2Client, newReshapeRecsClient, newTargetOfferingsEC2Client, newMarketplaceEC2Client and newAzureExchangeClient. Each one calls the same concrete constructor as before, with the same arguments.
  • loadDefaultAWSConfig, which wraps awsconfig.LoadDefaultConfig.
  • azureReservationTermFromString, moved over unchanged.
  • errAzureUnsupportedAppliedScope, which holds the same sentinel value, so errors.Is behaves exactly as before.
  • Go type aliases (not new types) for the provider data types (ec2ConvertibleRI, riUtilization, azureExchangeableReservation, ...). Values, JSON encoding and the existing tests, which still build ec2svc.*/azurecompute.* values directly, are unchanged.
• The handler files now call these constructors and use the aliases. The injected test factories (reshapeEC2Factory, azureExchangeFactory, marketplaceEC2Factory, ...) behave the same: when a factory is set it still wins. The one direct call in listConvertibleRIs still skips the factories, as it did before.
• I named the results of three functions (authorizeAzureExchangeExecution, awsMarketplaceListingFinder.FindMarketplaceListingByToken, reconcileAttempt) because gocritic's unnamedResult resolves alias types to "" and then reports a false duplicate. Every return in them is still explicit; the only body changes are two := → = swaps where the result variable now already exists.
• New guard test internal/api/provider_wiring_imports_test.go:
  • TestRIExchangeHandlerHasNoConcreteProviderImports: handler_ri_exchange.go must not import cloud-commitments-go/providers/..., github.com/Azure/azure-sdk-for-go/..., aws-sdk-go-v2/config or aws-sdk-go-v2/service/....
  • TestOnlyProviderWiringImportsConcreteProviders: no non-test file in internal/api other than provider_wiring.go may import cloud-commitments-go/providers/....

This is a pure refactor with no behavior change. No existing test file was modified.

Refs #41

Verification

All of this is local unit/static evidence. There are no real cloud calls; everything ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.

• Fail before the fix: I added the guard test first, on main 7b86e386. go test ./internal/api/ -count=1 -run 'TestRIExchangeHandlerHasNoConcreteProviderImports|TestOnlyProviderWiringImportsConcreteProviders' gave FAIL, listing armreservations, aws-sdk-go-v2/config, providers/aws, providers/aws/recommendations, providers/aws/services/ec2 and providers/azure/services/compute as forbidden handler imports, plus "provider_wiring.go must exist".
• Pass after the fix: the same command gives PASS for both tests.
• Mutation probe: I added _ "github.com/LeanerCloud/cloud-commitments-go/providers/aws" to ri_utilization_cache.go. TestOnlyProviderWiringImportsConcreteProviders then failed with [ri_utilization_cache.go: github.com/LeanerCloud/cloud-commitments-go/providers/aws]. I reverted the probe afterwards.
• go build ./..., go vet ./... and go vet -tags integration ./... are clean.
• go test ./internal/api/ -count=1: ok. go test ./internal/mocks/ ./internal/providerfactory/ -count=1: ok.
• golangci-lint run --allow-parallel-runners --build-tags integration ./internal/api/ (v2.10.1): 0 issues.
• gocyclo -over 10 -ignore "_test\.go" on every changed non-test file: no output.
• No integration tests were added or changed, because there is no DB change.

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

Branch	Tested head
fix/issue-712-fargate	2df9723
fix/issue-41-ri-exchange-wiring	bb548fe
fix/issue-349-ci-hygiene	360ae6c
fix/issue-756-next-collection	2cb1b2f
fix/issue-36-go-clock	aaea757
fix/issue-36-fe-waits	7feb01d
fix/issue-35-purchase-asserts	fbafeb0
fix/issue-134-mfa-token	33da813

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. The full suite ran on 644bfcaf, which differs from this tree only by a test-only rename on fix/issue-134-mfa-token (a password-like constant that git-secrets flagged):

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package that has them, one package at a time (-tags integration -count=1): internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase and internal/server all pass. Migration 000152 applies on top of main's 000151.
• Frontend: the full jest suite passes on this tree (175 suites, 4595 tests, 1 skipped).
• 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: 183 PASS, 0 FAIL.
• Terraform .tftest.hcl files were not run on this tree (CI runs them). Nothing was applied to any cloud.

Left out of scope

• Removing the deprecated ProviderConfig.Profile field (pkg/provider/interface.go). That is a cloud-commitments-go change, so ARCH-12: Provider factory/registry bypassed; internal layer hard-codes Azure/GCP constructors and deprecated Profile field #41 stays open for it.
• Raw AWS SDK use in the Marketplace handlers. handler_marketplace.go (sts, ec2types), handler_marketplace_attempt.go and handler_marketplace_poller.go (sdkec2) and handler_marketplace_sts.go call AWS SDK service packages directly for STS AssumeRole and DescribeReservedInstancesListings. Lifting those onto a provider capability needs new library surface. It is not cheap wiring.
• aws.Config and smithy.APIError stay in handler_ri_exchange.go. They are generic SDK value and error types threaded through the AWS handler signatures and the Handler struct. The guard deliberately allows them.
• Other internal/api files that import the Azure SDK directly (handler.go, handler_accounts.go, handler_purchases_revoke.go, handler_recommendations_refresh.go). They are not part of the RI exchange path.
• Note for parallel work: the package-wide guard will fail any future PR that adds a concrete providers/... import to an internal/api file other than provider_wiring.go. That is intended. Such a PR should add its constructor or alias to provider_wiring.go instead.

Expect textual conflicts with held PRs #1246 and #1141 because type names changed in the same files. They should be mechanical to resolve: use the alias names.

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

🤖 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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{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 bb548fe8e5310fb77ff36c2758eb64a03acbc7f6

Blocking:

• none

Non-blocking:

• internal/api/provider_wiring_imports_test.go: the package-wide guard covers only cloud-commitments-go/providers/.... Azure SDK and AWS service-client imports in other handlers (handler.go, handler_accounts.go, the Marketplace STS/listing code) are still allowed, as the body says. That is fine for this slice. When ARCH-12: Provider factory/registry bypassed; internal layer hard-codes Azure/GCP constructors and deprecated Profile field #41 is closed, it is worth saying in the issue that the guard does not cover those.
• Heads-up for the merge order: once this lands, any branch that adds a concrete providers/... import to a non-test internal/api file will fail TestOnlyProviderWiringImportsConcreteProviders after a merge from main. I checked every open PR that touches internal/api/*.go and none adds such an import today.

Attacked:

• Read the whole three-dot diff. Every change outside provider_wiring.go is either a rename to a type alias (=, so it has the same identity, JSON tags and method sets) or a call to a wrapper that calls the same constructor with the same arguments: NewEC2ClientDirect(cfg), NewRecommendationsClientDirect(cfg), azurecompute.NewClient(cred, sub, "") and awsconfig.LoadDefaultConfig(ctx). The injected factories still win when they are set. errAzureUnsupportedAppliedScope is the same sentinel value, so exchangeRefusals and errors.Is behave as before.
• Named results in authorizeAzureExchangeExecution, FindMarketplaceListingByToken and reconcileAttempt: there is no bare return, every return is explicit, and the :=→= swaps assign the same values. The scope check still runs before the client is built, and the source-ownership and constraint checks are unchanged.
• Fail-before: I restored main's versions of the 7 changed non-test files and deleted provider_wiring.go. Both guard tests then FAIL, listing the same forbidden imports the body gives. They pass again on the head.
• Ran on bb548fe: go build ./...; go vet on ./internal/api/ with and without -tags integration; go test -race -short -count=1 ./internal/api/ (ok, 68s); golangci-lint v2.10.1 on ./internal/api/ with and without --build-tags integration (0 issues); gocyclo -over 10 on the changed non-test files (silent). There are no DB changes, so I didn't run the integration tests.

Body-stale:

• none

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

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

Blocking:

• none

Non-blocking:

• internal/api/provider_wiring_imports_test.go: the strict SDK guard (no Azure SDK, AWS config or service/...) covers only handler_ri_exchange.go. ri_exchange_audit.go, ri_exchange_refusals.go and ri_utilization_cache.go are also RI exchange files and are now clean too, so listing them in the strict guard would stop them regressing at no extra cost. The package-wide providers/ guard covers them only for cloud-commitments-go/providers/....
• As the body says, the package-wide guard turns any future providers/... import outside provider_wiring.go into a test failure. Held feat(ri-exchange): pass offered instance types to reshape analysis #1246 and fix(api): four handler bugs found while documenting the API (#1102) #1141 will need to rebase onto the alias names. Expected, but worth flagging to whoever un-holds them.

Attacked:

• Went through every changed line of handler_ri_exchange.go, the three Marketplace files, ri_exchange_audit.go, ri_exchange_refusals.go and ri_utilization_cache.go against provider_wiring.go. Here is why I think behavior is unchanged:
  • Every replaced constructor calls the same concrete function with the same arguments, including the Azure client's empty region.
  • Every type swap is a Go alias (=), so JSON shapes and the existing tests that build ec2svc.* / azurecompute.* values are unchanged.
  • errAzureUnsupportedAppliedScope is the same sentinel value, so the errors.Is refusal mapping holds.
  • loadDefaultAWSConfig is a direct pass-through.
  • Of the three functions that got named results (authorizeAzureExchangeExecution, FindMarketplaceListingByToken, reconcileAttempt), none has a defer or a bare return, so naming them can't change what they return. The two := to = swaps assign the same variables the old := declared.
  • The money-path guardrails (checkAzureExchangeMoneyGuardrails, source ownership, scope checks) are touched only in type names.
• Ran:
  • go build ./...
  • go vet ./internal/api/, with and without -tags integration
  • go test -race -short -count=1 ./internal/api/ ./internal/server/ (ok)
  • go test -race -tags integration -count=1 ./internal/api/ (ok, 188s)
  • golangci-lint v2.10.1 on ./internal/api/, with and without --build-tags integration (0 issues)
  • gocyclo -over 10 on the changed non-test files (silent)
• Fail-before (mutation): I added a blank providers/aws import to ri_utilization_cache.go and an aws-sdk-go-v2/config import to handler_ri_exchange.go. Both guard tests fail and name the offending import. Restored afterwards.
• Refs ARCH-12: Provider factory/registry bypassed; internal layer hard-codes Azure/GCP constructors and deprecated Profile field #41 (not Closes) is right: removing the deprecated ProviderConfig.Profile is a cloud-commitments-go change.

Body-stale:

• none

_{Reviewer: rI3. 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 bb548fe8e5310fb77ff36c2758eb64a03acbc7f6 merged onto main (15dc8feb), together with #1565, #1566, #1567, #1569 in that order (combined tree 189311ee, 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 bb548fe8 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly bb548fe8e5310fb77ff36c2758eb64a03acbc7f6.