[IAmJSD]

Root cause

setReviewMetadata (internal/api/handler_registrations.go) copied session.UserID into reg.ReviewedBy without checking it. For an admin-API-key request requireAdmin returns the sentinel session UserID = "admin-api-key". The store then wrote that string into account_registrations.reviewed_by, which is UUID REFERENCES users(id) (migration 000017), in both TransitionRegistrationStatus and UpdateAccountRegistration. Postgres rejected it with SQLSTATE 22P02, so every approve or reject made with the admin key returned 500 and left the row pending. transitioned_by already went through validUUIDPtrOrNil (#1009), but reviewed_by did not.

Fix

setReviewMetadata now stores only a real user UUID in reviewed_by, using the same validUUIDPtrOrNil rule. For the admin key reviewed_by stays NULL, reviewed_at is still set, and a log line records the non-user principal and the decision so the audit trail is kept, as the issue suggests. No schema change and no migration.

Concurrency: the status transition is still the existing UPDATE ... WHERE id = $1 AND status = 'pending', so double approval stays prevented. Admin-key approvals could not succeed before this fix, so that race path was never reachable until now. It is covered by a test below.

Closes #1234

Verification

All commands were run in the worktree with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's default AWS profile is invalid.

New real-Postgres integration tests (testcontainers, through Handler.HandleRequest so the router and the admin-key principal middleware run too), in internal/api/handler_registrations_api_key_integration_test.go:

• TestRegistrationReview_AdminAPIKey_Postgres/approve: x-api-key approve returns 200. The row is approved, reviewed_by and transitioned_by are NULL, reviewed_at is set, and cloud_account_id is linked, so the follow-up UpdateAccountRegistration also succeeds.
• TestRegistrationReview_AdminAPIKey_Postgres/reject: returns 200. The row is rejected with reviewed_by NULL, reviewed_at set and the rejection reason stored.
• TestRegistrationReview_SessionReviewerStamped_Postgres: a real session reviewer is still written to reviewed_by and transitioned_by. This catches a fix that nulls the column for everyone.
• TestRegistrationReview_AdminAPIKey_ConcurrentApprove_Postgres: 8 concurrent admin-key approvals of one registration give exactly one 200 and seven 409s, and create exactly one cloud account.

Unit (mock) test: TestHandler_TransitionRegistrationStatus_NonUUIDActorIsNil now also asserts reg.ReviewedBy == nil and reg.ReviewedAt != nil.

Fail before the fix (handler change reverted, tests kept):

go test -tags integration ./internal/api/ -run 'TestRegistrationReview_' -count=1 -v
  ERROR: invalid input syntax for type uuid: "admin-api-key" (SQLSTATE 22P02)
  --- FAIL: TestRegistrationReview_AdminAPIKey_Postgres/approve   expected: 200 actual: 500
  --- FAIL: TestRegistrationReview_AdminAPIKey_Postgres/reject    expected: 200 actual: 500
  --- PASS: TestRegistrationReview_SessionReviewerStamped_Postgres
  --- FAIL: TestRegistrationReview_AdminAPIKey_ConcurrentApprove_Postgres  expected map[200:1 409:7] actual map[500:8]
go test ./internal/api/ -run TestHandler_TransitionRegistrationStatus_NonUUIDActorIsNil -count=1
  --- FAIL: Expected nil, but got: (*string)(0x...)

Pass after the fix:

go build ./...
go vet ./internal/api/ && go vet -tags integration ./internal/api/
go test -tags integration ./internal/api/ -run 'TestRegistrationReview_' -count=1   # ok
go test ./internal/api/ -count=1                                                     # ok
go test ./internal/mocks/ -count=1                                                   # ok
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/api/                   # 0 issues
golangci-lint v2.10.1 run --allow-parallel-runners --build-tags integration ./internal/api/  # 0 issues
gocyclo -over 10 -ignore '_test\.go' internal/api/handler_registrations.go           # silent
gofmt -l internal/api/                                                               # clean
bash scripts/gosec-hook.sh internal/api/handler_registrations.go                     # exit 0
git secrets --scan (isolated HOME, throwaway repo with the changed files added; a planted key was flagged)  # clean
bash scripts/test-git-secrets-allowlist.sh                                           # 137 PASS, 0 FAIL

All of this evidence comes from local Postgres via testcontainers plus mocked auth. Nothing was tested against a deployed environment or a real admin API key.

Out of scope / notes

• The store (UpdateAccountRegistration / TransitionRegistrationStatus) still writes whatever ReviewedBy it receives. The FK-safety rule lives in the handler, which is where the session sentinel is known, and matches how transitioned_by and created_by_user_id are handled. I didn't add a guard at the store level that would silently drop bad values.
• Registrations reviewed with the admin key show no reviewed_by in GET /api/registrations/:id, because the field is omitempty. The frontend declares reviewed_by in frontend/src/api/registrations.ts but never displays it, so no UI change is needed. Showing "reviewed by the admin API key" in the UI would need a separate column, which the issue left optional.

Maintainers: issue #1234 currently has no triage labels, so I had none to mirror. Please triage the issue and copy its labels to this PR.

🤖 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

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

Blocking:

• none

Non-blocking:

• internal/api/handler_registrations.go:306 and :396: validUUIDPtrOrNil(reg.ReviewedBy) at the two TransitionRegistrationStatus call sites, and the comment above each one ("reviewed_by may be "admin-api-key""), are now redundant, because setReviewMetadata already guarantees a UUID or nil. They're harmless. Dropping the stale comment would avoid misleading the next reader.
• Behavior note for the owner: admin-API-key approvals now really create cloud accounts. Before this, they failed with a 500 after the transition was attempted. That is the route's documented intent (AuthAdmin), but it is a newly working path.

Attacked:

• Auth/scope: the route is still AuthAdmin (router.go:350). The fix only changes which value lands in the FK column: validUUIDPtrOrNil keeps real session UUIDs, so session reviewers are still stamped, which TestRegistrationReview_SessionReviewerStamped_Postgres asserts against real Postgres. Double approval is still prevented by the WHERE status='pending' transition, and a losing racer gets 409 from either getPendingRegistration or ErrRegistrationConflict. The 8-way concurrent admin-key test gives exactly 1x200/7x409 and creates one cloud account.
• Fail-before: I reverse-applied only the handler hunk, keeping the tests. TestRegistrationReview_AdminAPIKey_Postgres/approve and /reject FAIL with SQLSTATE 22P02, and TestHandler_TransitionRegistrationStatus_NonUUIDActorIsNil FAILS. The session-reviewer control still passes. After restoring, all pass under -race.
• Ran: go build ./...; go vet ./internal/api/ with and without -tags integration; go test -race -short -count=1 ./internal/api/ (ok); full go test -tags integration -count=1 ./internal/api/ (ok, 72s); golangci-lint v2.10.1 with and without --build-tags integration (0 issues); gocyclo -over 10 on handler_registrations.go (clean).

Body-stale:

• none

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

Blocking:

• none

Non-blocking:

• Audit trail. internal/api/handler_registrations.go setReviewMetadata: for an admin-key review, the only record of who decided is now the Infof log line. The DB row shows reviewed_by NULL, the same as a review whose session lookup failed. The issue allows this. If auditors need to tell "admin key" apart from "unknown" in the DB, that needs a separate column (owner call, as the body says).
• Redundant wrap. Lines 306 and 396 still wrap the value in validUUIDPtrOrNil(reg.ReviewedBy) for transitioned_by. That is now redundant, because ReviewedBy is already filtered. It's harmless and can be left.

Attacked:

• Order of operations. reg.Status is set before setReviewMetadata, so the log line names the right decision. TransitionRegistrationStatus is still the WHERE status = 'pending' CAS, and the follow-up UpdateAccountRegistration (which also writes reviewed_by) now gets NULL instead of the sentinel. A valid-UUID session user is still stamped.
• No auth change. requireAdmin gating is untouched, and only what is persisted changes.
• Concurrency. The new test runs 8 concurrent admin-key approvals. It shows 1x200, 7x409 and exactly one cloud account, through HandleRequest against real Postgres.
• Commands. go build ./..., go vet (with and without -tags integration) on ./internal/api/, go test -race -short -count=1 ./internal/api/ (ok), the new and registration integration tests via testcontainers (ok), golangci-lint v2.10.1 with and without the integration tag (0 issues), and gocyclo (silent). I also merged current origin/main into the scratch checkout and re-ran vet plus the new tests: ok.
• Fail-before. With the merge-base handler_registrations.go restored, TestRegistrationReview_AdminAPIKey_Postgres/approve and /reject get 500 instead of 200. ConcurrentApprove gets map[500:8], with 0 accounts and the row still pending. TestHandler_TransitionRegistrationStatus_NonUUIDActorIsNil fails. SessionReviewerStamped passes both before and after, as intended. Everything 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 c41234a34fde30f6ef585d0fdeb6bcc4cbd96a9c 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 c41234a3 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly c41234a34fde30f6ef585d0fdeb6bcc4cbd96a9c.