[IAmJSD]

Root cause

ri_exchange_history.account_id has been VARCHAR(20) NOT NULL CHECK (account_id ~ '^\d{12}$') since 000009, which is the exact shape of an AWS account ID. Two separate barriers block any other cloud:

1. Width. A 36-character Azure subscription GUID overflows VARCHAR(20) (SQLSTATE 22001).
2. CHECK. Any value that isn't AWS-shaped is rejected (SQLSTATE 23514). That includes a GCP project ID short enough to fit 20 characters, and an Azure GUID even after widening.

No migration has touched either barrier since 000009. Today this is latent: only the AWS exchange path writes this table. The first Azure or GCP writer would hit it and lose the audit row of an exchange that has already been billed, the same failure as the sibling purchase_history column (cloud-commitments-cli#1603, fixed by 000095).

Fix

Migration 000137 (internal/database/postgres/migrations/000137_ri_exchange_history_account_id_multicloud.{up,down}.sql)

• Widens account_id to VARCHAR(255), matching cloud_accounts.external_id, savings_snapshots.account_id and purchase_history.account_id. It leaves unbounded varchar and anything wider than 255 alone.
• Drops the AWS-only CHECK. The migration finds it in the catalog, not by name: any single-column CHECK on account_id that contains the ^\d{12}$ regex. No replacement CHECK is added: AWS account IDs, Azure GUIDs and GCP project IDs share no shape, and the table has no provider column a CHECK could key on. purchase_history.account_id also has no CHECK.
• Each half runs only if the catalog shows it is still needed, and the column is resolved through regclass, the way 000095 does it. So fresh, deployed, re-run and partially fixed databases all converge, whether the column is widened but still CHECKed, unCHECKed but still narrow, or carries the CHECK under another name. A missing column is a hard error, and both halves are read back from the catalog afterwards, so the migration can't be recorded as applied without the work done.
• Concurrency. The migration takes LOCK TABLE ... ACCESS EXCLUSIVE before reading the catalog. Without it, a second run that starts while the first is uncommitted (for example auto-heal or a manual replay outside golang-migrate's advisory lock) reads the stale catalog, waits on the first run's lock, and then fails with constraint "ri_exchange_history_account_id_check" ... does not exist (SQLSTATE 42704). With the lock it waits and then does nothing. Widening a varchar is binary-coercible, so there is no table rewrite and no rebuild of idx_ri_exchange_history_account.
• Down. It takes the same lock first, then refuses (and names the row count) while any account_id isn't 12 digits, because narrowing would truncate or drop exchange audit rows. Otherwise it narrows from exactly VARCHAR(255) and restores the CHECK under its original name.

Go boundary check (internal/server/handler_ri_exchange.go)

Dropping the CHECK alone would quietly change live AWS behavior. When STS fails, resolveAccountID returns the placeholder "unknown", and today the CHECK rejects that row. configExchangeStoreAdapter, the only writer of this table, is AWS-only, so its SaveRIExchangeRecord and ReserveRIExchange now refuse any account ID that isn't 12 ASCII digits before calling the store. AWS rows keep exactly the guarantee the CHECK gave them, and the check now sits at the boundary that knows the provider, as the issue suggests. Five mock test fixtures that used a 9-digit "123456789" (plus one with an empty ID) were updated to "123456789012".

Closes #144

Verification

All evidence comes from local unit tests plus integration tests on a real Postgres testcontainer (macOS, Docker). Nothing touched a real cloud account. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

Fail-before. TestMigration137_RIExchangeHistoryAccountIDMultiCloud pins the schema at previousMigrationVersion(t, 137) and asserts the bug there: an Azure GUID insert fails with 22001, and cudly-prod-0042 (fits the width) fails with 23514. After 000137, AWS, Azure, 30-character GCP and short GCP IDs all insert and round-trip. A re-run of the up migration is a no-op.

Mutation probes. Each one was applied to the migration or adapter, run, then reverted:

Mutant	Result
Widen only (no DROP CONSTRAINT)	fails main test, 4/5 partial-state cases, both concurrency cases, both down tests
Drop CHECK only (no widen)	fails main test, the narrow partial-state cases, concurrency from 000009, both down tests
No LOCK TABLE in up	fails TestMigration137_ConcurrentRunsSerialize/from_widened_but_still_checked with SQLSTATE 42704
Down: no refuse guard	fails both TestMigration137_DownRefusesNonAWSRows cases
Down: no CHECK re-add	fails TestMigration137_DownRestoresAWSConstraintWhenSafe
Adapter guard disabled	fails TestConfigExchangeStoreAdapter_RefusesNonAWSAccountID

The concurrency test is deterministic. Run 1 executes the up migration inside an open transaction. Run 2 starts and is confirmed blocked through pg_locks. Run 1 commits, and run 2 must then succeed.

One note on the refusal assertion: the migrate error embeds the whole SQL file, so the test matches the formatted 1 row(s) hold ... text, which only the raised message contains. A plain message-literal assertion passed even with the guard removed.

Commands:

go build ./...
go test ./internal/server/ -count=1
go test -tags integration ./internal/database/postgres/migrations/ -count=1   # whole package, incl. FullStackIdempotent and DownAllRoundTrip: ok (203s)
go test -tags integration ./internal/config/ ./internal/server/ -run 'Exchange' -count=1
go test ./internal/mocks/ -count=1
go vet ./internal/server/ && go vet -tags integration ./internal/database/postgres/migrations/ ./internal/server/ ./internal/config/
golangci-lint v2.10.1 run --allow-parallel-runners [--build-tags integration] ./internal/server/ ./internal/config/ ./internal/database/postgres/migrations/   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/server/handler_ri_exchange.go   # silent
bash scripts/gosec-hook.sh <changed .go files>   # exit 0
git secrets --scan (fresh repo, isolated HOME, scripts/setup-git-secrets.sh) + scripts/test-git-secrets-allowlist.sh   # exit 0, 42 PASS

Three TestPostgresStoreDB_ReserveApprovedRIExchange_* tests skip locally because they need a dedicated RI_EXCHANGE_TEST_DATABASE_URL. That skip predates this PR.

Out of scope (left for follow-ups)

• Audit finding A04-015. ri_exchange_history.cloud_account_id is never populated or projected, so exchange-history scoping still matches on the provider account string. Not changed here.
• Audit finding A04-007 / fix(exchange): interactive AWS/Azure RI exchange endpoints persist no ri_exchange_history row, so their spend bypasses the daily-cap accounting #145. The Azure exchange path still writes no ri_exchange_history row and has no daily cap. This PR only removes the schema barrier that ledger write would hit. It does not add an Azure writer, and it does not relax the AWS adapter, which stays 12-digit-only. A future Azure or GCP writer needs its own provider-aware validation at its own boundary.

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

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

• 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 452e2df

Migration order. This PR adds 000137, and the highest migration on origin/main is 000133. Migrations merge strictly in number order:

• 134 (fix(purchase): scope purchase suppressions to the AZ, platform and tenancy bought #1110), 135 (fix(auth): bind group account scope entries to account IDs #1116), 136 (fix(plans): migrate stored pre-#608 Azure/GCP service ids to canonical slugs #1140)
• then this PR (137)
• then fix(auth): reject sessions issued before the user's last password change #1197 (138) and chore(db): backfill purchase_history.cloud_account_id from (provider, external_id) #1201 (139)

The PR is based on a main from before 131, but git merge-tree against current origin/main is clean. No migration on main other than 000009 touches ri_exchange_history.account_id; 000131 adds a separate table on purpose.

Up and down. They are symmetric on the normal path: VARCHAR(20) plus CHECK becomes VARCHAR(255) with no CHECK, and the down reverses it.

• The down refuses loudly while any row is not AWS-shaped, as its header documents.
• It narrows only from exactly VARCHAR(255), so a column that started wider or unbounded is left alone. That asymmetry is intended and documented.

Findings (none blocking):

• [non-blocking] 000137 up/down - LOCK TABLE ... ACCESS EXCLUSIVE has no lock_timeout. If a long transaction holds a lock on ri_exchange_history, the request waits and queues every reader behind it. The repo has no lock_timeout convention, the table is small, and the up is catalog-only.
• [non-blocking] 000137 down - Removing its LOCK TABLE leaves every test green (probe P4), so the race it guards against (an insert between the count and the ALTER) is untested.

Attacked:

• Lock and rewrite cost: widening a varchar is binary-coercible, so there is no table rewrite and no rebuild of idx_ri_exchange_history_account, and dropping the CHECK is catalog-only. The down's narrowing does rewrite the table and rebuild the index, and its comment says so.
• Data: round-trips of AWS ids, a 36-character Azure GUID, and 30-character and short GCP ids are tested.
• Dependents: no view, matview, rule or policy references ri_exchange_history, and its only index is kept by ALTER TYPE.
• Down with Azure/GCP rows present: it raises before any DDL and names the row count. The documented CUDLY_FORCE_MIGRATION_VERSION recovery path exists (migrate.go:115/311).
• Go code still assuming 12 digits: none found.
  • The adapter (internal/server/handler_ri_exchange.go:327/339) is the only non-mock caller of SaveRIExchangeRecord/ReserveRIExchange, so its new guard fully replaces the dropped CHECK. The guard also blocks resolveAccountID's "unknown" fallback (:151).
  • No Go code branches on SQLSTATE 23514 or 22001.
• Probes, each restored and confirmed identical with cmp:
  • P1, an unanchored regex: 2 adapter subtests FAIL.
  • P2, the Reserve guard removed: all 8 FAIL.
  • P3, the down skipping the narrowing: FAILS.
  • P4, the down without LOCK: passes (finding 2).
  • P5, the up widening only to VARCHAR(36): 5 tests FAIL.
  • P6, the down guard checking only length > 20: the short-GCP refusal FAILS.

Verified locally, on a git archive export of HEAD with no cloud credentials, under the heavy lock:

• go build ./..., go vet ./internal/server/ ./internal/config/ and go vet -tags integration ./internal/database/postgres/migrations/: clean. gofmt -l: clean.
• go test -count=1 ./internal/server/: ok.
• go test -tags integration -count=1 ./internal/database/postgres/migrations/ -run 'TestMigration137|DownAllRoundTrip|FullStackIdempotent': all pass.
• go test -tags integration -count=1 ./internal/config/ -run Exchange: ok.
• On the merged tree (origin/main + PR), the full -tags integration migrations package and ./internal/server/ pass.
• Fail-before: the new adapter test FAILS all 8 subtests on base. The migration test also asserts the bug itself at version 136 (SQLSTATE 22001 for the Azure GUID, 23514 for the short GCP id) before applying 137.

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 452e2df

PR #1170 (draft, @IAmJSD), fix(db): let ri_exchange_history.account_id hold Azure and GCP identifiers. Closes #144 (open).
Reviewed head: 452e2df, one commit, merge base e304ebe. git merge-tree --write-tree against origin/main f4387e4: clean.

Migration number: origin/main's latest migration is 000133. 000137 does not collide with main or with any open PR:

• fix(purchase): scope purchase suppressions to the AZ, platform and tenancy bought #1110 holds 000134
• fix(auth): bind group account scope entries to account IDs #1116 holds 000135
• fix(plans): migrate stored pre-#608 Azure/GCP service ids to canonical slugs #1140 holds 000136
• fix(auth): reject sessions issued before the user's last password change #1197 holds 000138
• chore(db): backfill purchase_history.cloud_account_id from (provider, external_id) #1201 holds 000139

Main has had gaps before (60->63, 67->70, 81->83, 83->86), and previousMigrationVersion tolerates them, so the number is not a blocker.

Findings

• medium (merge order, not a code defect) internal/database/postgres/migrations/000137_*.up.sql: if this PR merges before fix(purchase): scope purchase suppressions to the AZ, platform and tenancy bought #1110, fix(auth): bind group account scope entries to account IDs #1116 or fix(plans): migrate stored pre-#608 Azure/GCP service ids to canonical slugs #1140, a deployed DB at version 137 will never run 134-136. golang-migrate only moves forward, and there is no out-of-order guard in internal/database. Whichever of those PRs merges after this one must renumber above the current max, or this PR must merge after them.
• nit (process) commit 452e2df: 591 changed lines (579+, 12-) against the 400-line cap. Non-blocking. Of those, 344 are tests and 212 are SQL, mostly comments.
• nit 000137 up.sql:82 and down.sql:35: LOCK TABLE ... ACCESS EXCLUSIVE has no lock_timeout. If a long-running reader holds the table, the lock request queues and blocks every later reader and writer until it is granted. The table is low-traffic and no other migration sets lock_timeout either, so this is a nit only.
• nit down.sql:16-19: the down rewrites the table and rebuilds idx_ri_exchange_history_account under ACCESS EXCLUSIVE. That is fine for a rollback, and the file documents it.

Checked

• Up widens only when typmod-4 < 255. It never touches unbounded varchar. It drops only single-column CHECKs on account_id that contain ^\d{12}$, and it re-reads both conditions afterwards. varchar(20)->varchar(255) is binary-coercible, so there is no rewrite, and the index is kept.
• Down symmetry: down refuses while any row is not 12 digits, so it cannot truncate data or fail ADD CONSTRAINT mid-way. It narrows only from exactly typmod 259 and re-adds the CHECK under the 000009 name. The column is NOT NULL, so NULLs cannot slip past !~.
• Down can fail once wider values are stored, by design. It raises before making any change, and the file documents recovery via CUDLY_FORCE_MIGRATION_VERSION=137.
• Go writers: the only non-test INSERT is PostgresStore.SaveRIExchangeRecord / ReserveRIExchange (store_postgres.go:3129), reached only through configExchangeStoreAdapter. Both of its entry points now refuse non-12-digit ids (internal/server/handler_ri_exchange.go:322-343), so AWS rows, including the STS-failure "unknown" placeholder, keep the guarantee the CHECK gave them. ReserveApprovedRIExchange and the other UPDATEs never write account_id.
• Readers: scoping (internal/api exchangeRecordInScope) is AWS-keyed. Non-AWS ids would match only literal allow-list entries, which is fail-closed. This is out of scope, as the PR notes (A04-015).

Mutation probes (scratch export, each restored and verified with cmp)

• Up with no DROP CONSTRAINT: all 5 TestMigration137_* tests FAIL.
• Up widening to VARCHAR(20) instead of 255: all 5 FAIL.
• Down with the non-AWS guard disabled: DownRefusesNonAWSRows FAILS.
• Up with no LOCK TABLE: ConcurrentRunsSerialize FAILS.
• Adapter regex check disabled: TestConfigExchangeStoreAdapter_RefusesNonAWSAccountID FAILS.
• Guard removed from ReserveRIExchange only: the same test FAILS in 4 subcases.

Local runs on the exact head (macOS, Docker)

• go build ./...: ok.
• go vet -tags integration ./internal/...: ok.
• go test ./internal/server/ ./internal/mocks/: 599 passed.
• lockf -k -t 3600 /tmp/claude/heavy.lock go test -tags integration ./internal/database/... -count=1: ok. migrations took 199s; database and testhelpers ok.
• TestMigration137_* -v: 5/5 PASS.
• go test -tags integration ./internal/config/ ./internal/server/ -run Exchange: ok.