[IAmJSD]

Fixes four of the five handler bugs listed in #1102, one commit each. Item 4 (the email revoke form) is left out because it needs an owner decision; see below. The issue therefore stays open.

Refs #1102

Fixes

1. Unsupported analytics interval / dimension returned 500 (217e4534)

Root cause. GET /api/history/analytics and /api/history/breakdown passed interval and dimension straight to the Postgres analytics client. Its allowlist (intervalToTruncUnit, dimensionToColumn) rejected unknown values with a plain error, which the handler wrapped and the router answered with a 500.

Fix. The handlers now run the same allowlist functions before any query and answer 400 with the accepted values. The defaults are unchanged (hourly, service), and so is the case-sensitive matching.

2. Registration approval wasn't atomic (340cf181)

Root cause. approveRegistration flipped the registration to approved, then created the cloud account, then linked them, all in separate statements. If the create failed (a duplicate provider + external_id, or any DB error), the registration stayed approved with no account. Approving it again returned 409 and the UI had no way back. This is the same drift migration 000021 had to clean up once already. A failed link was only logged.

Fix. A new store method, ApproveAccountRegistration, runs three steps in one transaction:

• the conditional status change (WHERE status = 'pending'),
• the account insert,
• the link.

The status change runs first, so its row lock makes any concurrent approve or reject of the same registration wait for the transaction:

• If the approval commits, the waiting request matches no row and gets 409.
• If it rolls back, the waiting request proceeds as if the approval never ran.

The handler maps a conflict or a duplicate account to 409 and anything else to 500. Credentials are still stored after the commit, best effort, as before.

The interface gains one method. It is implemented in internal/mocks and in the two hand-written test stores (internal/server, internal/analytics).

3. PUT /api/accounts/{id} silently dropped provider (bbb5ebae)

Root cause. The handler validated the request's provider and echoed it back, but UpdateCloudAccount never writes the provider column. The change was dropped while the response said it had been saved.

Fix. A provider change is now refused with a 400 that tells the caller to delete and re-add the account. I chose this over persisting the new provider because credentials, service overrides and plan memberships all depend on the provider. Plan membership in particular is only checked against it when an account is added to a plan, so saving a new provider would bypass that check. The check runs after the account-scope check, so a caller who can't see the account still gets a 404. The settings UI always sends the account's own provider (the edit modal is opened from the per-provider list), so it is unaffected. If you'd rather make provider editable, that is a larger change and should be its own issue.

5. Azure revokes returned 500 (106608d4), now verified

Root cause. Confirmed on real Postgres. POST /api/purchases/{id}/revoke asks GetExecutionByID whether the ID is a scheduled execution before looking in purchase_history, and falls through only on ErrNotFound. An Azure purchase ID is an ARM path, and execution_id is a uuid column, so Postgres refused the cast with invalid input syntax for type uuid ... (SQLSTATE 22P02). Every Azure revoke died with a 500 before reaching the Azure return path. The unit tests never saw it because they mock GetExecutionByID as returning ErrNotFound for ARM paths.

Fix. GetExecutionByID now maps SQLSTATE 22P02 to ErrNotFound: no execution can have an ID that is not a UUID. This makes the store honor the contract the handler and its unit tests already assumed, so no existing fixture changes. Every other DB error still surfaces as a 500 (TestRevokePurchase_GetExecutionByIDDBError_Returns500 still passes).

Left out: item 4, the email revoke form's POST always gets 401

Confirmed by reading the code; there is no fix here. The revoke email links straight to GET /api/purchases/revoke/{id}?token=..., which renders a plain HTML form. The form's POST carries no Authorization or CSRF header, so requireSessionBeforeLookup always answers 401. Approve and cancel avoid this because their emails link to SPA routes (/purchases/approve/..., /purchases/cancel/...) that call the API with the session. The frontend has no revoke deep link.

Both possible fixes need a decision from you:

• Token-only authorization for the email path. This widens the auth model, which the handler comments say was deliberately deferred to the "dedicated revocation token" work.
• A frontend /purchases/revoke/:id deep link, with the email pointing at it. This is frontend work, which I'm not touching while the UI work is in progress.

Verification

Run on macOS against commit 340cf181, and again on ffafbd0a (origin/main merged in, plus the 22P02 unit tests below). All tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's default AWS profile is invalid. Integration tests use a throwaway Postgres container (testcontainers). No cloud API was called: the Azure SDK clients are stubbed through the existing azureRevokeClientFactory seam.

Fail-before / pass-after. Each fix was checked by restoring that commit's production file from its parent and rerunning the new tests.

Item	Tests	Evidence	Before the fix	After the fix
1	TestHandler_getHistoryAnalytics_UnsupportedIntervalIs400, TestHandler_getHistoryBreakdown_UnsupportedDimensionIs400	unit (mock analytics client that returns the real allowlist error)	all 8 subtests FAIL (500)	pass
1	..._SupportedIntervalsReachStore, ..._SupportedDimensionsReachStore	unit	pass	pass (no over-rejection)
2	TestApproveRegistration_FailedAccountCreateLeavesRegistrationPending	real Postgres	FAIL: status approved, reviewed_at set, no account	pass: still pending, rejectable
2	TestApproveRegistration_RejectQueuedBehindApproval (commit and rollback subtests)	real Postgres; a test-only trigger slows or fails the account insert; asserts in pg_stat_activity that the reject is blocked on the lock	FAIL (nothing holds the lock; the failure case leaves an orphan)	pass
2	TestApproveRegistration_ConcurrentApprovalsCreateOneAccount (8 goroutines)	real Postgres	pass (the old CAS already prevented double approval)	pass: 1 success, 7 × 409, 1 account, linked
2	TestApproveRegistration_ApprovesAndCreatesInOneStoreCall, ..._StoreErrorsMapToResponses	unit	FAIL	pass
3	TestUpdateAccount_ProviderChangeIsRejected	unit	FAIL: returns the account with Provider: "gcp", no error	pass
3	TestUpdateAccount_SameProviderIsSaved	unit	pass	pass
5	TestGetExecutionByID_NonUUIDIsNotFoundPostgres	real Postgres	FAIL: invalid input syntax for type uuid ... (SQLSTATE 22P02)	pass
5	TestRevokePurchase_AzureARMPurchaseIDOnRealPostgres	real Postgres + stub Azure clients	FAIL (both subtests): revoke: GetExecutionByID /providers/...: ... (SQLSTATE 22P02)	pass: unknown ARM ID gives 404; a real row is returned and revoked_at is persisted
5	TestGetExecutionByID_InvalidTextRepresentationIsNotFound, TestGetExecutionByID_OtherPostgresErrorsAreNotNotFound (57P01, 53300, 42P01, 22P03)	unit (pgxmock on the real PostgresStore)	mapping disabled: FAIL; every PgError mapped to ErrNotFound: FAIL	pass

Commands

go build ./...
go vet ./internal/config/ ./internal/mocks/ ./internal/server/ ./internal/analytics/ ./internal/api/
go vet -tags integration ./internal/config/ ./internal/api/
go test ./internal/api/ ./internal/config/ ./internal/mocks/ ./internal/server/ ./internal/analytics/ -count=1   # all ok (one package at a time)
go test -tags integration ./internal/api/ -run 'TestApproveRegistration_|TestRevokePurchase_AzureARMPurchaseIDOnRealPostgres|TestRevokePurchase_RecordsActorUUIDOnRealPostgres' -count=1   # ok
go test -tags integration ./internal/config/ -run 'TestGetExecutionByID_NonUUIDIsNotFoundPostgres|Registration' -count=1   # ok
golangci-lint run ./internal/api/ ./internal/config/ ./internal/mocks/                       # v2.10.1: 0 issues
golangci-lint run --build-tags integration ./internal/api/ ./internal/config/                # 0 issues
gocyclo -over 10 -ignore "_test\.go" <changed non-test files>                                # silent
bash scripts/gosec-hook.sh <changed non-test files>                                           # exit 0
git secrets --scan <changed files>   # isolated temp repo + HOME, repo patterns: clean; scripts/test-git-secrets-allowlist.sh: 42 PASS

Coverage gaps.

• Nothing ran against a deployed environment or a real Azure subscription.
• The Azure return path is exercised end to end with stubbed CalculateRefund/Return clients only.
• The registration races are tested on Postgres 16 under testcontainers, with READ COMMITTED (the default).

Out of scope, found while reading

• reg_credential_payload is never wiped after approval. The handler set reg.RegCredentialPayload = "" "so we don't keep encrypted credentials longer than needed", but neither UpdateAccountRegistration nor TransitionRegistrationStatus writes that column, so the encrypted payload stays in account_registrations indefinitely. This PR removes the dead in-memory assignment so it no longer claims otherwise, but doesn't clear the column. That deserves its own issue.
• The open PR fix(scheduler): scope the collection marker release to the invocation, not the marker #1099 also adds a method to StoreInterface and MockConfigStore, in different hunks. Whichever lands second may need a trivial merge.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, urgency/this-quarter, impact/few, effort/m, type/bug. The issue has no triaged label.

🤖 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]

VERDICT: REQUEST_CHANGES 340cf18

Findings:

• blocking internal/api/handler_analytics.go:133,199,235-261 - the PR conflicts with origin/main (ca1e8a8). The cause is cb25d99 (fix(api): scope a scoped user's raw analytics account id to its provider #1161), which replaced validateAnalyticsAccountScope and the local accountID with resolveAnalyticsAccountFilter (main handler_analytics.go:232). git merge-tree reports CONFLICT in three hunks. Fix: rebase, drop the accountID := params["account_id"] lines, and keep analyticsInterval/analyticsDimension next to main's resolveAnalyticsAccountFilter. I resolved it that way in a scratch copy: it builds and vets, and every test below passes.
• nit (process) 340cf18: 432 changed lines (+413/-19, 8 files), over the 400-line cap. The other commits are 140, 83 and 148 lines.
• note (pre-existing, not introduced here) internal/config/store_postgres_registrations.go:178,216 - TransitionRegistrationStatus and UpdateAccountRegistration write reg.ReviewedBy raw into the reviewed_by UUID FK column (000017:29). setReviewMetadata sets it from session.UserID, which is the literal "admin-api-key" for API-key sessions. Approving or rejecting with the admin API key should therefore fail with SQLSTATE 22P02 (500), before and after this PR. The actor argument is validated, but reviewed_by is not. Worth its own issue.

Item verification (all on the merged and resolved tree):

1. Analytics 400: analyticsInterval and analyticsDimension (head handler_analytics.go:235+) call the store allowlists intervalToTruncUnit and dimensionToColumn (internal/api/analytics_postgres.go:47,65) before any query. The defaults are unchanged. Fixed.
2. Atomic approval: ApproveAccountRegistration (store_postgres_registrations.go:261) runs the conditional status change, the account insert and the link in one WithTx. Duplicates map to 409 through classifyApproveRegistrationError. Fixed.
3. Provider change: updateAccount refuses req.Provider != existing.Provider after the access check (handler_accounts.go:576-583). UpdateCloudAccount never writes provider. Fixed by rejecting the change.
4. The email revoke form is correctly left out. The PR says "Refs fix(api): small handler bugs found while documenting the API (#262) #1102", not "Closes".
5. Azure revoke: GetExecutionByID maps 22P02 to ErrNotFound (store_postgres.go:1892). Fixed.

Attacked:

• Unit tests: go test ./internal/api/ ./internal/config/ ./internal/mocks/ ./internal/server/ ./internal/analytics/ passed.
• Integration tests: TestApproveRegistration_, TestRevokePurchase_AzureARMPurchaseIDOnRealPostgres, TestRevokePurchase_RecordsActorUUIDOnRealPostgres (api) and TestGetExecutionByID|Registration (config) all passed. heavy.lock was busy for more than 20 minutes (held by another review run), so these ran without the lock.
• go vet and go vet -tags integration on api and config were clean.
• Mutation probes:
  • interval check disabled: KILLED (UnsupportedIntervalIs400)
  • dimension check disabled: KILLED (UnsupportedDimensionIs400)
  • provider check disabled: KILLED (ProviderChangeIsRejected)
  • 22P02 mapping disabled: KILLED (NonUUIDIsNotFoundPostgres, AzureARMPurchaseIDOnRealPostgres)
  • status transition moved outside the transaction: KILLED (FailedAccountCreateLeavesRegistrationPending, RejectQueuedBehindApproval)

[cristim]

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

The approval covers the code. The PR does not merge cleanly into current origin/main, so it needs a mechanical rebase first. The hand-resolved merged tree passes (see below).

Findings:

• [blocking for merge, mechanical rebase only] internal/api/handler_analytics.go:134 and :200 - git merge-tree --write-tree origin/main 340cf18 conflicts in three hunks.
  • On main, cb25d99 replaced validateAnalyticsAccountScope with resolveAnalyticsAccountFilter and dropped the accountID := params["account_id"] lines.
  • Resolution: keep the PR's analyticsInterval/analyticsDimension calls and helpers, drop accountID :=, and keep main's resolveAnalyticsAccountFilter.
  • The resolved tree builds, vets and passes its tests.
• [non-blocking] internal/config/store_postgres.go:1918 - No test pins the pgErr.Code == "22P02" narrowing.
  • Mapping every PgError to ErrNotFound passes all tests, so a real DB outage in GetExecutionByID could silently become a 404.
  • TestRevokePurchase_GetExecutionByIDDBError_Returns500 uses a mock, so it cannot see this.
  • Suggest one integration case (a canceled ctx or a closed pool) that asserts the error is not ErrNotFound.
• [non-blocking] internal/api/handler_registrations.go:325-336 - classifyApproveRegistrationError re-implements the duplicate-key branch of classifyStoreError (handler_accounts.go:276). It could check ErrRegistrationConflict and then delegate.
• The linked issue text was not available to me, so I judged the change against the PR's own description of fix(api): small handler bugs found while documenting the API (#262) #1102 items 1, 2, 3 and 5. Item 4 is left out on purpose. No migration.

Attacked:

• Root cause per item:
  • Item 1: the handler now runs the same allowlist (analytics_postgres.go:47,65) before the query.
  • Item 2: the status transition, the account create and the link share one WithTx.
    • The status-first row lock makes a concurrent reject wait.
    • Under READ COMMITTED, its re-check then sees approved and matches 0 rows, which gives a 409.
  • Item 3: UpdateCloudAccount never writes provider. A provider change is now refused after the scope check, so out-of-scope callers still get 404. The frontend sends the modal's provider (settings.ts:2575).
  • Item 5: 22P02 becomes ErrNotFound. That is safe for the internal callers (store_postgres.go:1348, 1441, 1499) and the purchase/approvals callers.
• Blast radius: StoreInterface has no implementers besides PostgresStore, the mock and two test fakes. The merged tree builds, so main added no implementer that lacks the method.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  1. Default interval changed from hourly to daily: TestHandler_getHistoryAnalytics_ExternalIDOnlyAccount FAILS.
  2. Account insert moved before the status CAS: RejectQueuedBehindApproval FAILS.
  3. Link update dropped: ConcurrentApprovalsCreateOneAccount and RejectQueuedBehindApproval FAIL.
  4. Every PgError mapped to NotFound: all pass. This is the gap above.
  5. Duplicate-key 409 branch removed: StoreErrorsMapToResponses FAILS.
  6. Provider guard disabled: TestUpdateAccount_ProviderChangeIsRejected FAILS.

Verified locally, on git archive exports with no cloud credentials, under the heavy lock, with integration tests on testcontainers:

• Head:
  • go build ./...: OK.
  • go vet on api, config, mocks, server and analytics, and with -tags integration on api and config: OK.
  • gofmt -l: clean.
  • go test -count=1 on the 5 packages: ok.
  • Targeted api and config integration runs (registration, revoke on real Postgres, GetExecutionByID non-UUID): ok.
• Fail-before, with the new tests on base, all of these FAIL:
  • ProviderChangeIsRejected.
  • UnsupportedInterval/DimensionIs400.
  • AzureARMPurchaseIDOnRealPostgres.
  • FailedAccountCreateLeavesRegistrationPending.
  • RejectQueuedBehindApproval.
  • GetExecutionByID_NonUUIDIsNotFoundPostgres.
  • The approve unit tests cannot compile on base; probe 2 covers them.
• Merged tree (origin/main b6a0649 plus head, conflict hand-resolved):
  • Build and vet: OK.
  • Unit tests on the 5 packages: ok.
  • api integration (the PR filter plus Analytics) and config integration: ok.

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

[IAmJSD]

Thanks for both reviews. New head: ffafbd0a1e2757bea49e5ea1432b6694a2f2289d (merge of origin/main c8bdd460, then one test commit). I merged main instead of rebasing, so the four reviewed commits are unchanged.

Review 1 (REQUEST_CHANGES at 340cf18)

• [blocking] Conflict in internal/api/handler_analytics.go with fix(api): scope a scoped user's raw analytics account id to its provider #1161: resolved as you described in merge 4a006412. The local accountID := params["account_id"] lines are dropped, analyticsInterval/analyticsDimension are called at :130 and :193, main's resolveAnalyticsAccountFilter is called at :155 and :199, and the helpers sit at :226/:239 next to main's resolveAnalyticsAccountFilter (:258). validateAnalyticsAccountScope is gone. No other file conflicted.
• [nit, process] 340cf18 is 432 lines. Splitting it would mean rewriting a commit you already reviewed, so I left it as it is.
• [note, pre-existing] reviewed_by gets "admin-api-key" for API-key sessions, which gives 22P02 on approve/reject. This is out of scope here and not changed. It should get its own issue.

Review 2 (APPROVE at 340cf18)

• [blocking for merge] Same conflict, resolved as above.
• [non-blocking] Nothing pinned the 22P02 narrowing. Fixed in ffafbd0a: internal/config/store_postgres_execution_non_uuid_test.go:33,39 adds pgxmock tests on the real PostgresStore. 22P02 gives ErrNotFound. 57P01, 53300, 42P01 and 22P03 do not, and the *pgconn.PgError can still be reached with errors.As. I used pgxmock instead of a canceled ctx or a closed pool because those return non-PgErrors, which would not kill the "every PgError means NotFound" mutant. Mutation probes: every PgError mapped to NotFound, OtherPostgresErrorsAreNotNotFound FAILS; mapping disabled, InvalidTextRepresentationIsNotFound FAILS (the integration test catches that one too).
• [non-blocking] classifyApproveRegistrationError (handler_registrations.go:328) vs classifyStoreError (handler_accounts.go:276): left as is. The only shared logic is the existing isDuplicateKeyError helper. Delegating would relabel approval 500s as accounts: ... instead of registrations: approve: ... and would build the 409 message in two places.

PR body: the verification line now names ffafbd0a, and the item 5 table has a row for the new unit tests. It still says Refs #1102.

Verification on ffafbd0a (macOS, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, no cloud calls)

• go build ./... OK. gofmt -l internal/ is clean.
• go vet on api, config, mocks, server and analytics is clean. go vet -tags integration on api and config is clean.
• go test -count=1, one package at a time, on api, config, mocks, server and analytics: all ok.
• Integration (testcontainers Postgres):
  • api -run 'TestApproveRegistration_|TestRevokePurchase_AzureARMPurchaseIDOnRealPostgres|TestRevokePurchase_RecordsActorUUIDOnRealPostgres|Analytics': ok.
  • config -run 'TestGetExecutionByID|Registration': ok, on main's shared container from test(config): share one migrated Postgres container across integration tests #1165.
• golangci-lint v2.10.1 on the 5 packages, with and without --build-tags integration on api and config: 0 issues.
• gocyclo -over 10 on the changed non-test files: silent.
• git-secrets (fresh temp repo, isolated HOME, scripts/setup-git-secrets.sh patterns) on the changed files: clean.

CI on ffafbd0a (pre-commit, CI - Build & Test) was still running when I posted this. The PR stays a draft.

[IAmJSD]

Merged origin/main (a14dc8f2) to resolve the conflict. New head: a20cfd59b69afedbeaf51fc8d62a7f5f80b91bd7 (merge commit, no rebase; the reviewed commits are unchanged). No cristim review findings were newer than my last reply, so this is the merge only.

Conflict: internal/config/store_postgres.go (the only conflicting file)

• On main, fix(purchase): fail a plan step's root row atomically with its reason and suppression release #1176 (e43ba7c7) moved the body of GetExecutionByID into getExecutionByIDOn(ctx, q, ...) so it can run on a transaction. This PR adds the 22P02 → ErrNotFound mapping on the same line.
• Resolution: keep main's queryExecutionsOn(ctx, q, query, executionID) call and apply the 22P02 mapping right after it (store_postgres.go:1905-1912).
• Tx safety: on a transaction, getExecutionByIDOn is only reached from transitionRejection, after an UPDATE on the same execution_id matched no row. A non-UUID ID makes that UPDATE fail with 22P02 first, so the mapping can never hide an aborted transaction.

Semantic conflict (auto-merged but failed): internal/api/handler_accounts_hostcheck_test.go

• Main's fix(accounts): reject a non-host role_arn account with an empty role ARN on save #1160 (26a60c8a) added TestApproveRegistration_EmptyRoleARNRequiresHostAccount. It mocks TransitionRegistrationStatus/UpdateAccountRegistration and asserts CreateCloudAccount, but approveRegistration now makes the single ApproveAccountRegistration call. Its "host account is accepted" subtest panicked on the unmocked call.
• Fix (:167-197): the rejected cases now assert ApproveAccountRegistration is never called, and the host case mocks it and asserts it was called.
• Main's host check in approveRegistration merged cleanly and still runs before the approval transaction (handler_registrations.go:289). Mutation probe: with the check disabled, all 4 rejected subtests FAIL.

Verification on a20cfd59 (Linux VM, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, no cloud calls; STS in the host-check tests goes to fake transports)

• go build ./..., go vet ./... and go vet -tags integration ./internal/...: OK. gofmt: clean.
• Unit tests, one package at a time: config, api, mocks, server, analytics: all ok.
• Integration tests (testcontainers Postgres), one package at a time:
  • internal/api, full -tags integration: 4421 PASS. 3 tests failed at container start because the testcontainers reaper container was in state removing and one Postgres container didn't start before its deadline. Other agents were running testcontainers on the same Docker host. All 3 pass on rerun: TestDBRateLimiter_WindowExpiry_AtomicReset, TestPlanEditsPreserveConcurrentRampCompletion, TestLegacyServiceB1S1RoutedLedger. All of this PR's integration tests pass (TestApproveRegistration_*, TestRevokePurchase_AzureARMPurchaseIDOnRealPostgres, TestRevokePurchase_RecordsActorUUIDOnRealPostgres).
  • internal/config, full -tags integration: everything passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck. That test fails the same way on an untouched git archive of origin/main a14dc8f2, so it is not from this PR. Its "checked at the change" case (CheckedAt == updated) is accepted and resolves the shared held row to failed, and every case that runs after it in map order then sees failed. The test came from fix(exchange): alarm on stale processing RI exchanges and let an operator resolve them #1109. The resolved path itself passes: TestGetExecutionByID* (including the 22P02 unit and integration tests), TestPostgresStoreDB_TransitionExecutionStatusPinnedTx/Pinned/TokenPinned, and registration tests.
  • internal/purchase (main's fix(purchase): fail a plan step's root row atomically with its reason and suppression release #1176 tests, which reach the resolved function through a transaction): TestFailExistingPlanStep_* (3 tests): pass.
• golangci-lint v2.10.1 on api, config, mocks, server and analytics, plus --build-tags integration on api and config: 0 issues.
• gocyclo -over 10 on the changed non-test files: silent.
• git-secrets: the 17 files in the PR diff, git added into a fresh temp repo with an isolated HOME and the repo's patterns: clean. A planted fake key in the same repo was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.

The PR stays a draft. The body still says Refs #1102.

[cristim]

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

The earlier blocking-for-merge item is resolved. The PR now merges cleanly into origin/main 6af022f, and the merged tree builds and passes everything I ran.

Findings (none blocking):

• [non-blocking, still open] internal/api/handler_registrations.go:329-340 - classifyApproveRegistrationError still repeats the duplicate-key branch of classifyStoreError (handler_accounts.go).
• [addressed] The 22P02 pin. ffafbd0 adds internal/config/store_postgres_execution_non_uuid_test.go (pgxmock against the real PostgresStore).
  • 22P02 maps to ErrNotFound.
  • 57P01, 53300, 42P01 and 22P03 do not.
  • The original *pgconn.PgError stays reachable through errors.As.
• The fix commits 217e453..340cf18 are identical to the ones I approved (range-diff =).

What changed beyond a pure rebase, read with git show --remerge-diff:

• 4a00641, merging main at c8bdd46. The handler_analytics.go conflict is resolved as recommended:
  • the PR's analyticsInterval/analyticsDimension calls and helpers are kept;
  • the dead accountID := lines are dropped;
  • main's resolveAnalyticsAccountFilter stays, and validateAnalyticsAccountScope is gone.
• a20cfd5, merging main at a14dc8f:
  • store_postgres.go: main moved the lookup into getExecutionByIDOn(q). The 22P02 mapping now sits on queryExecutionsOn(ctx, q, ...), so both the pool path and the tx path get it.
  • handler_accounts_hostcheck_test.go: the mocks moved from TransitionRegistrationStatus/CreateCloudAccount to ApproveAccountRegistration, with matching AssertNotCalled/AssertCalled.

Attacked:

• Host check ordering: requireHostForEmptyRoleARN (handler_registrations.go:288) still runs before setReviewMetadata and before the transaction, so a refusal leaves the registration pending.
• 22P02 inside a transaction:
  • The one tx caller, transitionRejection (store_postgres.go:1357), probes only after an UPDATE on the same ID.
  • A non-UUID would already have failed that UPDATE with 22P02, so the mapping cannot hide an aborted transaction.
• Drift on main in the touched files:
  • HistorySummaryAggregate, new on the interface and in the mocks; the merged tree builds.
  • History-predicate edits unrelated to GetExecutionByID.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  • P2, the account insert before the status CAS: RejectQueuedBehindApproval FAILS.
  • P3, the link UpdateAccountRegistration dropped: ConcurrentApprovalsCreateOneAccount and RejectQueuedBehindApproval FAIL.
  • P4, every PgError mapped to NotFound: TestGetExecutionByID_OtherPostgresErrorsAreNotNotFound FAILS, in 4 subtests. This gap was open last round and is now closed.
  • P4b, any class-22 SQLSTATE mapped to NotFound: the 22P03 subtest FAILS.
  • P6, the provider guard disabled: TestUpdateAccount_ProviderChangeIsRejected FAILS.

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

• Merge: git merge-tree --write-tree origin/main a20cfd5 is clean. git diff a14dc8f2 a20cfd5 is byte-identical to the PR diff.
• Build, vet and unit tests:
  • go build ./...: OK.
  • go vet on api, config, mocks, server and analytics, plus -tags integration on api and config: OK.
  • gofmt -l: clean.
  • go test -count=1 on the same 5 packages: ok.
• Integration tests (testcontainers):
  • -tags integration ./internal/api/ -run 'TestApproveRegistration_|TestRevokePurchase_AzureARMPurchaseIDOnRealPostgres|TestRevokePurchase_RecordsActorUUIDOnRealPostgres': PASS.
    • That includes FailedAccountCreateLeavesRegistrationPending, ConcurrentApprovalsCreateOneAccount, RejectQueuedBehindApproval (both subtests), AzureARMPurchaseIDOnRealPostgres and EmptyRoleARNRequiresHostAccount.
  • -tags integration ./internal/config/ -run TestGetExecutionByID: PASS, including NonUUIDIsNotFoundPostgres and the two new pgxmock tests.

Same reviewer as the earlier review, re-reviewing the new head. Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: APPROVE a20cfd5

Re-review of #1141 (Refs #1102, not closing; closingIssuesReferences is []).

Delta (340cf18 -> a20cfd5):

• ffafbd0 adds pgxmock unit tests: only SQLSTATE 22P02 maps to ErrNotFound, and 57P01/53300/42P01/22P03 pass through.
• a20cfd5 merges origin/main a14dc8f, which already contains fix(api): scope a scoped user's raw analytics account id to its provider #1161 (cb25d99). internal/api/handler_analytics.go keeps both the new analyticsInterval/analyticsDimension (:226, :239) and main's resolveAnalyticsAccountFilter (:258, used at :47/:155/:199). My blocker is resolved.
• The merge resolves internal/config/store_postgres.go onto getExecutionByIDOn (:1903). The tx-probe caller transitionRejection (:1357) only runs after an UPDATE with the same ID, which raises 22P02 itself first, so the mapping cannot hide an aborted tx. Main's fix(accounts): reject a non-host role_arn account with an empty role ARN on save #1160 test was ported to ApproveAccountRegistration.

Findings: none blocking.

• pre-existing, not a regression: reg_credential_payload is never cleared. UpdateAccountRegistration (store_postgres_registrations.go:170-197) does not write the column, so the removed reg.RegCredentialPayload = "" was dead on main too. The PR body discloses this. It should get its own issue.

Merge-tree vs origin/main 6af022f: clean (tree 37cf86dc). On the merged tree:

• go build passes, and go vet passes on the touched packages.
• Unit tests in api, config, mocks, server and analytics: 5817 passed.
• Touched integration tests pass: the 3 TestApproveRegistration_* integration tests, TestRevokePurchase_AzureARMPurchaseIDOnRealPostgres and TestGetExecutionByID_NonUUIDIsNotFoundPostgres.
• Wider -tags integration run (Registration|Execution|Revoke|CloudAccount|Transition) over api, config and server: ok.

Mutation probes (fail-before), all killed:

• interval check off: 5 FAIL.
• dimension check off: 5 FAIL.
• provider check off: TestUpdateAccount_ProviderChangeIsRejected FAILs.
• approve without the tx: FailedAccountCreateLeavesRegistrationPending and RejectQueuedBehindApproval FAIL.
• 22P02 mapping off: both Postgres tests FAIL.
• every PgError mapped to not-found: 5 FAIL.
• duplicate-key 409 off: 2 FAIL.

CI on a20cfd5 (run of 2026-10-23, old base a14dc8f on go 1.26.9): 21 pass, 5 fail, 1 skipping. All 5 failures come from the old base:

• Security Scanning: govulncheck flags crypto/x509@go1.26.9, fixed in 1.26.10. Main bumped Go in fix(deps): bump Go to 1.26.10 for GO-2026-6809 #1236.
• Build Docker Image amd64/arm64: image advisories with a published fix (same toolchain).
• Integration Tests: TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, an untouched file fixed on main by fix(config): compare resolve checked_at at the database's microsecond precision #1252. It passes on the merged tree locally.
• CI Success: the aggregate of the four above.

A rebase or merge of current main should turn CI green. Until then the review gate's "CI green on the SHA" is not met.

@IAmJSD CI on this head is red only from the old merge base (govulncheck on go1.26.9, fixed on main by #1236; and the ResolveStaleRIExchange test fixed on main by #1252). Please merge or rebase onto current main so CI runs on a fresh merge ref.

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• Owner attention (item 5): this turns on a money path that has been dead until now. Every Azure revoke used to die with a 500 at GetExecutionByID. With the 22P02 mapping, a session revoke of an Azure reservation now reaches loadAndRevokePurchaseHistory, calls Azure's CalculateRefund/Return and records the refund. That is what fix(api): small handler bugs found while documenting the API (#262) #1102 asks for, and the existing guards (authorizeSessionRevoke, the window check, the expected-refund confirm) still run first. But this path has never run in production, and it has only been exercised here against stub Azure clients.
• internal/api/handler_accounts.go:589: the provider-change refusal runs after requireHostForEmptyRoleARN. So a PUT that will be refused with 400 can still trigger the host-account/STS lookup first. Putting the check right after requireAccountAccess keeps the 404-before-400 ordering and skips that call.
• The 22P02 mapping applies to every GetExecutionByID caller, not just revoke. Approve, cancel, scoping and ladder now answer 404 instead of 500 for a non-UUID id. I checked transitionRejection, the one in-tx caller: it only probes after an UPDATE on the same id already succeeded, so the mapping cannot hide an aborted transaction there.
• TestRevokePurchase_AzureARMPurchaseIDOnRealPostgres and TestGetExecutionByID_NonUUIDIsNotFoundPostgres both use a 90s context. On this loaded machine each one failed repeatedly whenever it was the first test to start the shared Postgres container ("database system is ready ... matched 0 times"). Both passed once the container was warm. The failure is in container startup, not in the tests' logic, but whichever test runs first on a slow runner will hit it.

Attacked:

• Item 2:
  • The status CAS, account insert and link run in one WithTx via txStore := &PostgresStore{db: tx}. TransitionRegistrationStatus, CreateCloudAccount and UpdateAccountRegistration are all single Execs, so nothing escapes the transaction.
  • A concurrent reject waits on the row lock and re-evaluates status = 'pending' after the commit, or proceeds after a rollback.
  • Credentials are stored only after the commit, as before.
• Item 1: the handler reuses the Postgres client's own allowlist, which is the only AnalyticsClientInterface implementation. The frontend sends only hourly/daily/weekly.
• Fail-before:
  • Reverting handler_analytics.go and handler_accounts.go to main fails ..._UnsupportedIntervalIs400, ..._UnsupportedDimensionIs400 and TestUpdateAccount_ProviderChangeIsRejected.
  • Disabling the 22P02 mapping fails TestGetExecutionByID_InvalidTextRepresentationIsNotFound.
  • Reverting handler_registrations.go to main fails TestApproveRegistration_RejectQueuedBehindApproval on real Postgres (both subtests: no backend blocked on the lock).
  • Restored afterwards.
• Ran:
  • go build ./...;
  • go vet ./..., with and without -tags integration, plus the e2e vet;
  • go test -race -short -count=1 on api, config, mocks, server and analytics;
  • -race -tags integration: TestApproveRegistration_* (all pass), plus the Azure ARM revoke test and the non-UUID config test, both of which pass once the container is warm (the context was raised locally only to get past cold start);
  • golangci-lint, with and without integration tags: 0 issues;
  • gocyclo: silent;
  • secrets-scan: clean.

Body-stale:

• none found.

_{Reviewer: rB2. 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.}