[IAmJSD]

First slice of the ARCH-12 cleanup. It is meant to change no behavior.

Root cause

The credentialed Azure and GCP paths in internal/purchase/execution.go (resolveAzureProvider, resolveGCPProvider) and internal/scheduler/scheduler.go (collectAzureForAccount, collectGCPForAccount) skipped the cloud-commitments-go provider registry. They called the concrete constructors directly: azureprovider.NewAzureProvider(&provider.ProviderConfig{Profile: sub}) + SetCredential(cred), and gcpprovider.NewProviderWithCredentials(ctx, project, ts). The Azure path also pinned the subscription through the deprecated ProviderConfig.Profile field.

The pinned pkg/provider registry can already take credentials. ProviderConfig has typed AzureSubscriptionID/GCPProjectID fields and AzureTokenCredential/GCPTokenSource slots, and the registered azure/gcp factories (NewAzureProvider/NewProvider) install them. So the fix needs no change in cloud-commitments-go.

Fix

• New internal/providerfactory package:
  • NewAzure(subscriptionID, cred) and NewGCP(projectID, ts) build the provider with Registry.GetProviderWithConfig. They use only the typed fields and credential slots, never Profile.
  • The package blank-imports the Azure/GCP provider packages, so both factories are always registered.
• The four call sites above now use these helpers. purchase/execution.go no longer imports the concrete Azure/GCP provider packages. The scheduler still imports them for IsPermissionError / AsPartialSubscriptionFailure.
• After this change no non-test code under cmd/ or internal/ sets ProviderConfig.Profile. That clears the platform-side blocker to removing the field in cloud-commitments-go.

Behavior notes (please review)

• Azure: the result is identical to the legacy construction: same subscription ID, same credential, same region. The test compares the whole struct against the real registered factory. A nil credential reaches the factory as a nil slot, which keeps the same ambient fallback as SetCredential(nil).
• GCP: also identical, with one difference. The legacy constructor stored the caller's ctx while the registry factory stores context.Background(), but the GCP provider never reads that field.
• One deliberate difference: NewGCP rejects an empty project ID (and a nil token source) before calling the registry. The registry's GCP factory resolves an empty project ID with a live getDefaultProject lookup. The direct constructor never made that lookup, and it could pin the provider to a project other than the account's. The scheduler refuses an unpinned Azure account for the same reason.
  • Before: an account with credentials but an empty gcp_project_id got a provider for projects/ that failed on first use.
  • Now: the account fails at provider construction, with create provider: providerfactory: GCP project ID is required. On the purchase path the message is credentials: create GCP provider for account ….
  • It fails in both cases, and no purchase can happen in either.

Refs #41

Verification

Unit and fixture evidence only. The tests use fake credentials and token sources. No test calls a real cloud API, and constructing the providers makes no network calls.

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 AWS_EC2_METADATA_DISABLED=true
go build ./... && go vet ./... && go vet -tags integration ./...                       # OK
go test -count=1 ./internal/providerfactory/                                            # ok
go test -count=1 ./internal/purchase/                                                   # ok (32.6s)
go test -count=1 ./internal/scheduler/                                                  # ok
go test -count=1 ./internal/mocks/                                                      # ok
golangci-lint (v2.10.1) run --allow-parallel-runners ./internal/providerfactory/ ./internal/purchase/ ./internal/scheduler/   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/providerfactory/ internal/purchase/execution.go internal/scheduler/scheduler.go   # none over 10

Tests added (internal/providerfactory/):

• TestNewAzure_PassesTypedConfigToRegistry and TestNewGCP_PassesTypedConfigToRegistry use a private registry with recording fake factories. They check that the factory is called once with Name, the typed ID field and the same credential instance (assert.Same). They also check that Profile and ProviderOverride are unset.
• TestNewAzure_NilCredentialLeavesSlotNil, TestNewGCP_RejectsMissingInputsBeforeRegistry (the factory is never called) and TestBuild_PropagatesFactoryErrorsAndNilProviders.
• TestNewAzure_MatchesLegacyConstruction and TestNewGCP_MatchesLegacyConstruction build the provider both ways with the real registered factories and compare the whole structs. The Azure test first calls InvalidateAccountsCache() on the new provider, to match the cache-generation bump that legacy SetCredential makes. Negative cases (a different credential, subscription or project) show the comparison isn't vacuous.
• TestNoRegistryBypassInProductionCode is an AST guard. It fails if any non-test .go file under cmd/ or internal/ refers to the concrete Azure/GCP constructors or puts a Profile key in a provider.ProviderConfig literal.

Fail-before / pass-after:

• Guard test: run against main's execution.go and scheduler.go, it fails with 6 violations:

  • execution.go:549 NewAzureProvider + Profile
  • execution.go:583 NewProviderWithCredentials
  • scheduler.go:912 NewAzureProvider + Profile
  • scheduler.go:980 NewProviderWithCredentials

  It passes on this branch.

• Mutation probes on the helper: each of these makes at least one test fail, and the equivalence tests catch all of them except the Profile swap:

  • dropping the Azure credential
  • dropping the GCP token source
  • changing the GCP project ID
  • setting Profile instead of AzureSubscriptionID

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

Branch	Tested head
fix/issue-204	76f85a4
fix/issue-438	78fe011
fix/issue-838	120b07f
fix/issue-143	aef1256
fix/issue-349	cfbb551
fix/issue-80	63405f9
fix/issue-199	36b9f8f
fix/issue-41	96fc237
fix/issue-299	6af14ed
fix/issue-324	3ab6729

fix/issue-324 and fix/issue-204 both edit .github/workflows/ci.yml next to the azure-kv-access-policy job. The tree kept #324's new "Run Function URL reader self-tests" step and #204's updated job comment; whichever PR merges second takes that same resolution.

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 ed0c7174, which differs from b56afb2b only by one .gitallowed line on fix/issue-324:

• 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, 60-minute local timeout): internal/api, internal/purchase, internal/scheduler and internal/server all pass.
• Secrets: the first run caught a bare placeholder account ID in fix/issue-324's new tftest, now allowlisted with an anchored .gitallowed line. Re-run at b56afb2b: 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 (left for follow-ups under #41)

• internal/api/handler_ri_exchange.go still imports the concrete AWS/Azure packages (providers/aws, providers/aws/recommendations, providers/aws/services/ec2, providers/azure/services/compute). Moving exchange operations onto a capability interface, or into one composition file, is a bigger change. Several in-flight branches also touch that file, so it's deferred.
• The deprecated ProviderConfig.Profile field itself lives in cloud-commitments-go (pkg/provider/interface.go). Removing it, along with the resolveAzureSubscriptionID/resolveGCPProjectID fallbacks, is a go-side change. The platform no longer sets the field, so that change is unblocked here.
• purchase still passes the built provider as ProviderConfig.ProviderOverride into CreateAndValidateProvider, as before. Passing the typed config straight to the factory there would move construction to purchase time, so it is left unchanged.

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

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 5 billable files and costs up to $1.25.}

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: 1c5787a
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: f40564d6-ebe9-4423-8cdc-ab35ebcbb72e

Commits

Reviewing files that changed from the base of the PR and between 1c5787a and b47068a.

Files selected for processing: 5

• internal/providerfactory/bypass_guard_test.go
• internal/providerfactory/providerfactory.go
• internal/providerfactory/providerfactory_test.go
• internal/purchase/execution.go
• internal/scheduler/scheduler.go

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

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

Blocking:

• none

Non-blocking:

• internal/providerfactory/providerfactory.go:58: NewAzure accepts an empty subscription ID, while NewGCP rejects an empty project. That matches the old behavior: the scheduler refuses unpinned Azure accounts earlier, but the purchase path doesn't. A matching ErrMissingAzureSubscriptionID guard would make the two helpers symmetric and would stop a future caller from building an unpinned Azure provider that lists every subscription the credential can see. It's a follow-up, not a regression.
• The one behavior change (an empty gcp_project_id with credentials now fails at construction instead of on first API call) fails closed on both paths. I agree with it.

Attacked:

• Tenant/credential isolation: Registry.GetProviderWithConfig (go pkg@541e05e5) calls the factory fresh each time and caches nothing, so per-account credentials can't leak between accounts through the registry. The Azure factory installs AzureTokenCredential and resolves the subscription from the typed field, identical to Profile+SetCredential; a nil interface cred converts to a nil any, so the ambient fallback is the same. The GCP factory with a non-empty project makes no getDefaultProject call, and Provider.ctx is never read in providers/gcp, so dropping the caller ctx is inert. Both purchase resolvers propagate the new error through resolveAccountProvider; nothing falls back to ambient credentials.
• Fail-before: with main's execution.go/scheduler.go restored over the branch, TestNoRegistryBypassInProductionCode FAILS with exactly the 6 violations (NewAzureProvider/Profile x2, NewProviderWithCredentials x2). Mutation: dropping the Azure credential in newAzure makes go test ./internal/providerfactory/ FAIL. Both were restored afterwards.
• Ran: go build ./...; go vet (with and without -tags integration) on providerfactory/purchase/scheduler; go test -race -short -count=1 on those three packages (ok); go test -race -tags integration -count=1 ./internal/purchase/ ./internal/scheduler/ (ok, 48s and 23s); golangci-lint v2.10.1 with and without --build-tags integration (0 issues); gocyclo -over 10 (clean).

Body-stale:

• none. "Refs ARCH-12: Provider factory/registry bypassed; internal layer hard-codes Azure/GCP constructors and deprecated Profile field #41" is correct, because the handler_ri_exchange.go part of ARCH-12 is still open.

_{Reviewer: rB21. 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 b47068acb31d2e2464340b8678794d434e6df3af

Blocking:

• none

Non-blocking:

• internal/providerfactory/bypass_guard_test.go profileKeys: the guard catches Profile: only as a key inside a provider.ProviderConfig{...} literal. A field assignment (cfg.Profile = sub) or a dot-import would get past it. A grep of non-test code under cmd/ and internal/ finds no .Profile use today, so the body's claim holds. Adding an *ast.SelectorExpr check for .Profile on the LHS of an assignment would make the guard match its doc comment.
• GCP fail-closed change. NewGCP now rejects an empty project ID at construction. On the purchase path this surfaces from resolveAccountProvider as credential resolution failed for account ..., before any purchase call, which is the same outcome (no buy) as the old provider failing on first use. The scheduler gets a per-account fan-out error instead of a later one. Agreed this is the right direction.

Attacked:

• Read the pinned go-side constructors (providers/azure@3def3f89, providers/gcp@3def3f89). The registered azure factory is NewAzureProvider(config). It reads the typed AzureSubscriptionID before Profile and installs AzureTokenCredential directly. IsConfigured short-circuits on a non-nil cred exactly as after SetCredential, and a nil cred falls back to DefaultAzureCredential in both paths. The only lost side effect is the cache-generation bump on a brand-new provider, which is a no-op.
• GCP. The registered factory is NewProvider, which does the live getDefaultProject only when the project ID is empty. NewGCP refuses that case up front, so no new network call can happen. The ctx field it now gets is context.Background(), and no file in the gcp provider package reads p.ctx (the one r.ctx hit is a different type in services/computeengine).
• Purchase money path. resolveAccountProvider errors abort before any purchase, and ProviderOverride handling in CreateAndValidateProvider is unchanged. The scheduler's Azure path still refuses an empty subscription before resolving credentials.
• Commands. go build ./.... go vet, with and without -tags integration, on providerfactory, purchase and scheduler. go test -race -short -count=1 on those three (ok). go test -race -tags integration -count=1 ./internal/purchase/ ./internal/scheduler/ with testcontainers Postgres (ok, 65s and 25s). golangci-lint v2.10.1, with and without the integration tag (0 issues). gocyclo (silent).
• Fail-before. With the merge-base internal/purchase/execution.go and internal/scheduler/scheduler.go restored, TestNoRegistryBypassInProductionCode fails with the 6 violations the body lists (execution.go:549 x2, :583, scheduler.go:912 x2, :980). It passes with the fix.

Body-stale:

• none

_{Reviewer: rB23. 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 b47068acb31d2e2464340b8678794d434e6df3af merged onto main (a9b7eb8f), together with #1247, #1248, #1295, #1303, #1310 in that order (combined tree a016458b, 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 b47068ac (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly b47068acb31d2e2464340b8678794d434e6df3af.