fix(api): detect duplicate registrations by SQLSTATE, not error text

[IAmJSD]

Root cause

POST /api/register decided "this account already has a pending registration" by matching error text in two places:

• PostgresStore.CreateAccountRegistration checked whether the driver error text contained idx_account_registrations_pending_unique or duplicate key, then returned a new fmt.Errorf("duplicate: ...") that did not wrap the driver error.
• submitRegistration then checked strings.Contains(err.Error(), "duplicate").

As a result:

• A reference_token collision (account_registrations_reference_token_key) also contains duplicate key, so it came back as 409 "a pending registration already exists for this account". That answer is wrong, and the collision can be fixed by retrying.
• Any store error whose text contained duplicate was reported as a 409.
• A translated (lc_messages) or reformatted driver message would stop matching. A routine duplicate would then become a 500.

Fix

• internal/config/store_postgres_registrations.go: classifyRegistrationInsertError uses errors.As to find the *pgconn.PgError and switches on SQLSTATE 23505 and ConstraintName:

  • idx_account_registrations_pending_unique returns ErrDuplicatePendingRegistration.
  • account_registrations_reference_token_key returns ErrReferenceTokenCollision.

  Both wrap the driver error with %w, so the PgError can still be reached. Every other error, including a unique violation on any other constraint, stays failed to create account registration: %w. The constraint names are constants next to the sentinels, so a third unique constraint on the table cannot silently fall into an existing branch. This follows the pattern already in internal/auth/store_postgres.go (isEmailDuplicateError).

• internal/api/handler_registrations.go: the new insertRegistration uses errors.Is:

  • ErrDuplicatePendingRegistration returns the same 409 with the same message as before, so the API contract is unchanged, and no admin email is sent.
  • ErrReferenceTokenCollision regenerates the token and re-inserts, up to 3 attempts. The response now returns the token that was actually stored.
  • Anything else is wrapped and goes through handleRequestError as a generic 500 {"error":"Internal server error"}. The DB error is only logged, never returned to the caller.

Closes #137

Verification

All test commands ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, on macOS (darwin/arm64), Go toolchain from go.mod.

• New unit tests, store (store_postgres_registrations_test.go, pgxmock):
  • A PgError 23505 on the pending index returns the duplicate sentinel, and the PgError is still reachable with errors.As.
  • A translated message (no English text) is still detected as a duplicate.
  • A 23505 on the token constraint returns the collision sentinel and is not treated as a duplicate.
  • None of these is treated as a duplicate: a 23505 on account_registrations_pkey, another SQLSTATE that names the pending index, or a plain errors.New whose text is the exact Postgres duplicate-key message.
• New unit tests, handler (handler_registrations_submit_test.go):
  • A duplicate returns 409 with the unchanged message and sends no admin email.
  • These each return 500 with Internal server error, send no email and make one store call: an error whose text contains "duplicate", a 23505 on another constraint, and a different PgError.
  • A token collision retries with a fresh token and returns that token.
  • A token collision that keeps happening gives up after 3 attempts as a 500, not a 409.
• New real-Postgres integration test (store_postgres_registrations_integration_test.go, testcontainers and the real migrations):
  • Inserting the same (provider, external_id) twice returns ErrDuplicatePendingRegistration, and the real PgError carries code 23505 and constraint idx_account_registrations_pending_unique.
  • Reusing a token for a different account returns ErrReferenceTokenCollision. This confirms the real constraint name is account_registrations_reference_token_key.
  • A primary-key collision is neither sentinel.
  • After the first registration leaves pending, the same account can register again.
• Fail before, pass after: with the old string-matching logic put back (new tests kept), every new store and integration test fails. Every new handler test fails except the "other database error" control. With the fix, all pass.
• Mutation probes: each mutation made at least one test fail:
  • store: treat any 23505 as a duplicate
  • store: drop the SQLSTATE check
  • store: stop wrapping the driver error
  • store: wrong token constraint name
  • handler: no retry
  • handler: collision mapped to 409
  • handler: strings.Contains(err.Error(), "duplicate") restored
  • handler: response returns the original token instead of the stored one
• Checks:
  • go build ./..., go vet ./... and go vet -tags integration ./... are clean.
  • go test ./internal/api/ ./internal/config/ -count=1 and go test ./internal/mocks/ -count=1 (repo-wide guards) pass.
  • golangci-lint v2.10.1 reports 0 issues over ./..., both with and without --build-tags integration.
  • gocyclo -over 10 on the changed non-test files reports nothing.
  • git-secrets --scan with the AWS patterns, run on a temporary copy with an isolated HOME, is clean. bash scripts/test-git-secrets-allowlist.sh gives 42 PASS, 0 FAIL.
• go test -v -tags integration ./internal/config/ -count=1 passed (635 tests passed, 799s). 11 tests were skipped because they need a dedicated test database URL in the environment (for example LADDER_ARMING_TEST_DATABASE_URL). None of them is related to registrations. The internal/api integration suite was not run, because this change adds no integration tests there.

Out of scope (follow-ups)

These are other places that branch on error-message text, found by grepping the repo. They are not on the registration path and are left unchanged:

• internal/api/handler_accounts.go:263 isDuplicateKeyError: matches "duplicate key" / "23505" in err.Error(). Audit A02-019.
• internal/api/handler_config.go:210 mergeServiceConfig: matches "not found". Audit A02-019 calls this the most important one.
• internal/api/handler_auth.go:304 isResetPasswordClientError: matches a list of message fragments. Audit A02-019.
• internal/auth/store_postgres.go already uses a typed check (SQLSTATE 23505 plus constraint name). It is listed here only as the pattern to copy.

Also not addressed here: the fail-open register rate-limit bucket and the per-replica in-memory limiter (LeanerCloud/cloud-commitments-cli#1575). The issue lists these as compounding factors, not as part of this defect.

🤖 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

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 6e3b0529-8604-44f5-b2d8-c7df722d637a

📥 Commits

Reviewing files that changed from the base of the PR and between 70a6457 and 7829b80.

📒 Files selected for processing: 5

• internal/api/handler_registrations.go
• internal/api/handler_registrations_submit_test.go
• internal/config/store_postgres_registrations.go
• internal/config/store_postgres_registrations_integration_test.go
• internal/config/store_postgres_registrations_test.go

• 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 verdict: CLEAN at 7829b80

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief, acting as a security reviewer. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Constraints that can fire on insert (migration 000017):

• id PK: a server-generated UUID, so a client cannot trigger it.
• account_registrations_reference_token_key: unique on reference_token.
• idx_account_registrations_pending_unique: partial unique on provider and external_id where status is pending.

The PR matches 23505 plus ConstraintName. Only the pending index maps to the duplicate sentinel and 409. A token collision becomes ErrReferenceTokenCollision, which is retried 3 times and then returns 500. Any other 23505 is a generic 500. No false "already registered" was found.

Wrap chain (no %v anywhere):

Layer	Where	Wrap
pgx Exec	store_postgres_registrations.go ~:110	raw *pgconn.PgError
Classify	store_postgres_registrations.go ~:48-52	%w: %w (sentinel and driver error), fallback %w
Handler	handler_registrations.go:139-156	errors.Is: 409 for duplicate, otherwise 500

Enumeration: the 409 text and status are unchanged, so nothing is newly exposed. The token-collision case no longer returns a misleading "already exists", which is better. The public endpoint still reveals that a pending registration exists; that is existing behaviour, noted only.

Remaining text matcher (outside this PR's scope, as its body says): internal/api/handler_accounts.go:261-268 (isDuplicateKeyError) still uses strings.Contains. internal/auth/store_postgres.go:144-158 already uses the typed pattern.

What was attacked and how:

• go vet ./internal/... is clean. go test -race -count=1 ./internal/api/... ./internal/config/... passes.
• go test -v -tags integration -run Registration ./internal/config/ against Docker Postgres: 10 PASS, 0 SKIP.
• Mutation probes, both killed:
  • wrapping the driver error with %v fails the pending-unique test;
  • treating any 23505 as a duplicate fails the token and non-duplicate tests.
• Fail-before: with the non-test files reverted, the tests fail to compile on the new sentinels.

Gate:

• Labels are mirrored from sec(api): registration uniqueness is enforced by string-matching "duplicate" in the driver error text #137.
• Needs the author to mark the draft ready, then green CI and CodeRabbit.

[cristim]

Gate record for 7829b80 (origin/main CLAUDE.md "Review gate"):

• Independent review (claude-cc-reviewer, non-author): CLEAN at 7829b80 (fix(api): detect duplicate registrations by SQLSTATE, not error text #1019 (comment)). The verdict SHA equals the head.
• CI on 7829b80: CI Success and Run pre-commit hooks pass. The merge driver refreshes CI if main changed a directory this PR touches after CI ran.
• Local verification: as recorded in the independent review comment (fixture or mock evidence, labelled there). No real-account run.
• CR skipped where quota-limited; covered by the independent review plus local verification.
• Labels mirrored from sec(api): registration uniqueness is enforced by string-matching "duplicate" in the driver error text #137.

Marking ready (owner-authorized) and merging at 7829b80 with --match-head-commit.

[cristim]

Gate record for 7829b80 (origin/main CLAUDE.md "Review gate"):

• Independent review (claude-cc-reviewer, non-author): CLEAN at 7829b80 (fix(api): detect duplicate registrations by SQLSTATE, not error text #1019 (comment)). The verdict SHA equals the head.
• CI on 7829b80: CI Success and Run pre-commit hooks pass. The merge driver refreshes CI if main changed a directory this PR touches after CI ran.
• Local verification: as recorded in the independent review comment (fixture or mock evidence, labelled there). No real-account run.
• CR skipped where quota-limited; covered by the independent review plus local verification.
• Labels mirrored from sec(api): registration uniqueness is enforced by string-matching "duplicate" in the driver error text #137.

Merging at 7829b80 with --match-head-commit.