[cristim]

Closes #1836

What

A direct AWS RI exchange that is stuck in submitted or unknown blocks every later exchange on the same source RIs (the guard from #1838). Today the only way out is a DBA running SQL from the runbook. This adds an operator API to list and resolve those rows, and makes the API the primary path in the runbook.

• List GET /api/ri-exchange/direct-executions: unresolved AWS direct rows, filtered to the caller's allowed accounts and regions, keyset paginated, fail-closed on store or authorization errors.
• Resolve POST /api/ri-exchange/direct-executions/{id}/resolve: the operator states outcome (completed or failed), the expected_status they saw, a reason and a cloud_check of what they verified at AWS. Replaying the same resolve returns the stored result.
• Permission execute:ri-exchange, the same people who can execute.
• Record: new columns (migration 000163) keep the provider status and add the resolution (resolved_status, resolved_at, note, who). The source guard ignores rows with resolved_at set. The audit history is never rewritten.
• Bounds: a submitted row can be resolved only 10 minutes after creation, an unknown row 1 minute after completion, and checked_at can never be in the future (database clock).
• Late provider outcome: if Finalize lands after an operator resolution and disagrees, both facts stay on the row and the contradiction is logged at error level.
• Runbook: the API is the primary path. The break-glass SQL stays as a last resort, now guarded by expected_status and a note-length check.

Owner decisions are recorded in the planning doc (resolution columns, 10-minute age, no account check, no four-eyes, keep break-glass SQL, execute:ri-exchange).

How it is proven

• Real-Postgres integration tests for list filters, resolve CAS, boundaries, replay, concurrent resolves (one wins) and Finalize-versus-resolve in both orders.
• Handler tests for authorization, body validation, fail-closed list and an end-to-end resolve.
• Migration test with a single version constant; runbook tests run the SQL against the store limits.
• Mutation probes on the guard clause, the three checked_at bounds, the contradiction log, the note-length guard and the openapi status enum.
• Two adversarial full-change review passes (same reviewer) on the 19-commit branch; the openapi status-enum test commit added afterwards had its own gate review, and the head rebased onto main (range-diff shows the reviewed commits unchanged) has had no further review.

Reading order

20 commits, each at most 400 changed lines: migration and guard, store (list, get, resolve, finalize), constraint helper, API list, API resolve, tests, runbook.

Notes for reviewers

• A row stays blocked for the guard until resolved, so resolving releases source RIs for another exchange: please attack the age and checked_at bounds and the CAS on expected_status.
• The resolve 200 response lists all four provider statuses because a late finalize can move a resolved row to completed or failed.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added authenticated tools to list unresolved direct RI exchange executions and record verified outcomes as completed or failed.
  • Lists support status, region, and RI filters, plus pagination; results are limited to authorized account and region scopes.
  • Resolution requests include the expected status and AWS check details. Identical repeat requests replay the recorded result.
  • Resolved executions no longer hold their source RIs, allowing them to be reused.
• Documentation
  • Updated the operations runbook with the listing and resolution workflow, eligibility rules, and SQL fallback guidance.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 47 minutes.

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: feat/1836-direct-exchange-resolve
• File: .coderabbit.yaml
• Commit: e75c742
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 4ead6397-57dd-4c24-991f-ce79572291eb

Commits

Reviewing files that changed from the base of the PR and between ad1be75 and e75c742.

Files selected for processing: 26

• internal/api/handler_ri_exchange_direct_list.go
• internal/api/handler_ri_exchange_direct_list_failclosed_test.go
• internal/api/handler_ri_exchange_direct_list_item_test.go
• internal/api/handler_ri_exchange_direct_list_test.go
• internal/api/handler_ri_exchange_direct_resolve.go
• internal/api/handler_ri_exchange_direct_resolve_request.go
• internal/api/handler_ri_exchange_direct_resolve_test.go
• internal/api/handler_ri_exchange_resolve.go
• internal/api/openapi.yaml
• internal/config/ri_exchange_direct_resolve.go
• internal/config/store_postgres.go
• internal/config/store_postgres_pgxmock_test.go
• internal/config/store_postgres_ri_exchange_direct.go
• internal/config/store_postgres_ri_exchange_direct_get.go
• internal/config/store_postgres_ri_exchange_direct_list.go
• internal/config/store_postgres_ri_exchange_direct_list_integration_test.go
• internal/config/store_postgres_ri_exchange_direct_resolve.go
• internal/config/store_postgres_ri_exchange_direct_resolve_integration_test.go
• internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go
• internal/config/store_postgres_ri_exchange_direct_schema_integration_test.go
• internal/config/store_postgres_ri_exchange_direct_schema_test.go
• internal/config/store_postgres_ri_exchange_direct_test.go
• internal/config/store_postgres_schema_version.go
• internal/config/store_postgres_schema_version_test.go
• known-issues.md
• runbooks/ri-exchange-direct-execution-settle.md

Walkthrough

Adds authenticated APIs to list unresolved AWS direct executions and record operator-verified outcomes. PostgreSQL stores resolution metadata without changing the original execution status. Resolved rows no longer block source RI reservations. The change includes API contracts, runbook updates, and database-backed tests.

Changes: Direct execution listing and resolution

Layer / File(s)	Summary
Resolution schema and store contracts internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.*.sql, internal/database/postgres/migrations/000163_ri_exchange_direct_resolution_test.go, internal/config/ri_exchange_direct_*.go	Adds resolution metadata columns and constraints, plus shared list and resolution data types and store interfaces.
Unresolved execution queries internal/config/store_postgres_ri_exchange_direct_get.go, internal/config/store_postgres_ri_exchange_direct_list.go, internal/config/store_postgres_ri_exchange_direct_*_integration_test.go	Adds PostgreSQL reads for unresolved AWS executions, account-region pairs, and keyset-paginated filtered results.
Authorized listing endpoint internal/api/handler_ri_exchange_direct_list.go, internal/api/handler_ri_exchange_direct_resolve.go, internal/api/router.go, internal/api/openapi.yaml, internal/api/handler_ri_exchange_direct_list*_test.go	Adds an authenticated list route. It applies account scope and execute-grant constraints, validates filters and cursors, and returns paginated rows.
Operator resolution endpoint internal/api/handler_ri_exchange_direct_resolve*.go, internal/api/handler_ri_exchange_resolve.go, internal/api/router.go, internal/api/openapi.yaml, internal/api/handler_ri_exchange_direct_resolve*_test.go, internal/api/handler_ri_exchange_resolve_test.go	Adds an authenticated resolution route with request validation, row-based authorization, outcome recording, replay reporting, and mapped errors.
Transactional resolution and RI release internal/config/store_postgres_ri_exchange_direct_resolve.go, internal/config/store_postgres_ri_exchange_direct.go, internal/config/store_postgres.go, internal/config/store_postgres_ri_exchange_direct_*_integration_test.go, internal/config/store_postgres_ri_exchange_direct_test.go, runbooks/ri-exchange-direct-execution-settle.md, known-issues.md	Records resolutions subject to status and checked-time conditions. Resolved rows stop blocking reservations. Finalization retains provider outcomes and logs known contradictions. The runbook documents the API workflow and a SQL fallback.

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Feature · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant Operator
  participant ResolveHandler as resolveRIExchangeDirectExecution
  participant AuthorizationService
  participant PostgresStore
  participant PostgreSQL
  Operator->>ResolveHandler: Submit outcome and cloud check
  ResolveHandler->>PostgresStore: Fetch AWS execution row
  ResolveHandler->>AuthorizationService: Check account, region, and payment cap
  ResolveHandler->>PostgresStore: Resolve with expected status and operator details
  PostgresStore->>PostgreSQL: Lock row, check eligibility, and store resolution
  PostgreSQL-->>PostgresStore: Resolution result or conflict
  PostgresStore-->>ResolveHandler: Return resolution result
  ResolveHandler-->>Operator: Return resolution result

Loading

Possibly related PRs

• LeanerCloud/cloud-commitments-platform#1831: Updates the sanitizer used for the new list endpoint’s stored error summaries.

Suggested reviewers: iamjsd

Merge Risk: Moderate · previous review ad1be

Resolve deployment compatibility and account-verification gaps before merging: existing exchanges can fail before migration, and operators can release RI guards after checking the wrong account. Replay matching, migration locking, and break-glass audit safeguards also need correction or explicit acceptance.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 61.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title "feat(api): list and resolve stuck AWS direct RI exchanges" matches the main change. The PR adds an operator API to list unresolved AWS direct RI exchange rows (submitted or unknown) and an …
Linked Issues check		Issue #1836 coding requirements are implemented at the reviewed head. (1) List endpoint: GET /api/ri-exchange/direct-executions returns unresolved AWS rows with status submitted or unknown. The handl…
Out of Scope Changes check		Every changed area connects to issue #1836 or supports its implementation. - Migration 000163 up and down, plus its migration test: adds the resolution columns and constraints that the issue asks for…

Full details: Title check

Explanation

The title "feat(api): list and resolve stuck AWS direct RI exchanges" matches the main change. The PR adds an operator API to list unresolved AWS direct RI exchange rows (submitted or unknown) and an endpoint to resolve them as failed or completed. The diff also includes a migration, store methods, runbook updates, and tests, which the title does not need to cover. The title is specific and concise and identifies the primary change.

Full details: Linked Issues check

Explanation

Issue #1836 coding requirements are implemented at the reviewed head.

(1) List endpoint: GET /api/ri-exchange/direct-executions returns unresolved AWS rows with status submitted or unknown. The handler filters by the caller's allowed account and region pairs, uses keyset pagination, and fails closed on store or non-403 authorization errors. The summary and openapi.yaml both describe this behavior.

(2) Resolve endpoint: POST /api/ri-exchange/direct-executions/{id}/resolve requires execute:ri-exchange, records outcome completed or failed with a note, and is audited through resolved_by and resolution_note. Authorization runs against the row's own account, region, and payment cap. Out-of-scope rows appear missing.

(3) Audit rule: migration 000163 adds resolution columns (resolved_status, resolved_by_user_id, resolved_by, resolved_at, resolution_note). The original status and completed_at are not rewritten. The PR chose the resolution-column option that the issue offered.

(4) Safety rules: PostgresStore.ResolveRIExchangeDirectExecution selects only provider = 'aws' rows, so Azure rows are never touched. A submitted row needs dbNow >= created_at + RIExchangeDirectResolveSubmittedAfter. checked_at must not be in the future, using the database clock. The SQL update uses a status CAS on expected_status.

(5) Guard: refuseRIExchangeSourceInFlightDirect ignores rows with resolved_at set. Integration tests cover reserve, approved reserve, and direct-save paths, before and after resolution.

(6) Tests: real-Postgres integration tests cover resolve bounds, replay, conflicts, and the guard. Handler tests cover authorization, body validation, and fail-closed list. An openapi test checks the status enum. Route tests check the POST and GET registrations.

Deviation noted: the issue text asks to match the five-minute minimum age (created_at < now() - interval '5 minutes'). The implementation uses a 10-minute age for submitted rows. The 10-minute bound is stricter. It still satisfies the issue's rule that a submitted row settles only after the execution window plus margin. The PR description records this as an owner decision.

No unmet coding requirement was found in the summary and in the inspected resolve store code.

Full details: Out of Scope Changes check

Explanation

Every changed area connects to issue #1836 or supports its implementation.

• Migration 000163 up and down, plus its migration test: adds the resolution columns and constraints that the issue asks for.
• Store files (list, get, resolve, finalize): implement the list, resolve, and guard behavior.
• Config types and interfaces: define the list and resolve contracts.
• Handler files, router, and route tests: expose the new endpoints.
• openapi.yaml and its test: required by the issue's OpenAPI test requirement.
• Runbook and its integration test: the issue asks for the runbook's safety rules, and the PR updates the runbook to use the API as the primary path.
• known-issues.md: removes the outstanding item that #1836 resolves.
• Refactor of handler_ri_exchange_resolve.go: moves the permission and amount checks into requireRIExchangeConstraintsFor so the direct-resolve path can reuse them. The existing history resolve path shares this code. It has a table-driven test covering the checked region and deployment account.

No unrelated change was identified in the summary.

Full details: Docstring Coverage

Explanation

Docstring coverage is 61.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 52 functions across 32 files. (4 skipped: 4 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

Generate docstrings

• Commit to this branch
• Create a new PR

Generate unit tests (beta)

• Commit to this branch
• Create a new PR

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

CHANGES REQUESTED ad1be75 by claude-cc-platform-2

The only blocking item at this SHA is red CI. I found no defect in the resolve logic.

Findings

1. Blocking: Lint Code is red at ad1be75 (run 42622191807, job 129176884555, golangci-lint, 7 issues). The PR head has since moved to b5b3d6e ("style(api): fix the golangci-lint findings"). That delta touches 5 files, 11 additions and 10 deletions, and looks style-only from the stat. I did not re-review it. The verdict needs a fresh gate at the new head, so this verdict does not carry over.
   • internal/config/store_postgres_ri_exchange_direct_list.go:95 unnamedResult
   • internal/api/handler_ri_exchange_direct_list.go:33 shadowed err
   • internal/api/handler_ri_exchange_direct_list_test.go:64 De Morgan; :118 prealloc
   • internal/config/store_postgres_ri_exchange_direct_list_integration_test.go:165 and :173 prealloc
   • internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go:42 prealloc
2. Low, informational: three mutants survived and I judge each equivalent (see table). AND resolved_at IS NULL and AND status = $6 in the resolve UPDATE (store_postgres_ri_exchange_direct_resolve.go:139-143) are defence in depth, because the row is already locked FOR UPDATE and the replay and status checks run first. Their loss is unreachable through the store API. Keep them.
3. Low, informational: a late Finalize can set provider status completed on a row an operator resolved failed. The row keeps both facts, the guard ignores it, and the only signal is an error-level log (store_postgres_ri_exchange_direct.go:137). This is the documented design; just be aware nothing surfaces it beyond logs.
4. Informational, constrained grants: the code is stricter than the owner decision. The list and resolve both evaluate the session's account scope and the execute:ri-exchange grant (account, provider, service, region, payment cap) through the authorization service, on the row's own account and region (handler_ri_exchange_direct_list.go:56-85, handler_ri_exchange_direct_resolve.go:187-199). I found no constrained-grant bypass. A row with no registered account needs an unrestricted scope.

What I attacked, and how I tried to refute

• Age and checked_at bounds (store ..._resolve.go:104-129): the clock is the database clock_timestamp() read after the row lock. A submitted row needs dbNow >= created_at + 10 min, and the bounds are inclusive at exactly 10 min. checked_at must be <= dbNow and >= the earliest time, and is truncated to microseconds. A NULL completed_at on an unknown row is refused, and the table CHECK ties it to status. created_at uses clock_timestamp() after the guard lock (store_postgres_ri_exchange_direct.go:32-40), so a lock wait does not age the row. The execute window is 4 min plus a 30 s terminal save (ri_exchange_detached.go:30-31), inside 10 min. There is no upper bound, which is intended. Could not refute.
• CAS (lock, status = expected_status, and a UPDATE re-check): a lost race returns 409. Concurrent test: exactly one winner. Replay returns the stored result only when outcome and the full note are identical, otherwise 409.
• Every writer of direct rows: INSERT (Save), Finalize (WHERE status='submitted', single statement, blocks on the resolve row lock), Resolve. No other UPDATE exists on the table. Finalize-first and resolve-first orders are both tested. A resolved row cannot be re-resolved or have its resolution cleared (mutant e killed).
• Allow-sets and body: outcome and expected_status are exact-match strings, with no case or whitespace variants. The body has unknown fields rejected, one document, a 4096 byte cap and a reason of 10-300 runes with control characters rejected. The note is %q-quoted, so odd runes are escaped, and a note over 1000 bytes gives a clear 400. List error_summary goes through SanitizeStoredError. The raw error, user id and API key id are not returned.
• Tenant scope: see finding 4. List pairs are filtered before the keyset query, and the cursor holds only (created_at, id).
• Migration 000163: IF NOT EXISTS columns, a CHECK on resolved_status, an all-or-none constraint, and a down that drops constraint and columns (it loses only resolutions, the fail-safe direction). Migration test passed locally. It does not conflict with 000161/000162.
• Runbook SQL: a single atomic UPDATE with an id, aws, resolved_at IS NULL, expected-status, age, checked_at and note-length guard, and a "expect 1 row" check. It cannot double-resolve. No transaction is needed.
• openapi and router: the routes are AuthUser and the spec matches the handlers I read. I did not diff every schema field.

Mutants (config integration, -tags integration, scoped; api unit)

Mutant	Killed by
a drop 10-min settle check	..._SubmittedAgeBoundary
b guard clause resolved_at IS NULL	ReserveRIExchange_ResolvedDirectRowDoesNotBlock, ..._ResolvedMatrix, DirectSave_ResolvedHolderDoesNotBlock and others
b2 drop resolved_at IS NULL in the resolve UPDATE	SURVIVED (equivalent, row lock and replay check)
c drop status = $6 in the UPDATE	SURVIVED (equivalent, same reason)
c2 drop the expected_status compare in check	..._FinalizeFirst
d widen outcome by unknown	..._Refusals
d2 widen row-status allow by failed	SURVIVED (equivalent, expected_status check still refuses)
e Finalize clears the resolution	..._ResolveFirstThenFinalize
f permission view:purchases instead of execute:ri-exchange	TestResolveRIExchangeDirect_Authorization and 9 more
f2 skip row-scoped constraint check	..._ConstraintsUseTheRowsAccountAndRegion, ..._CappedGrantRefusesUnreadableAmount
g off-by-one on the age bound (-1 us)	..._CheckedAtBounds
g2 exclusive lower bound	..._CheckedAtBounds, ..._SubmittedAgeBoundary
h drop checked_at lower bound	..._CheckedAtBounds, ..._UnknownBound
h2 drop checked_at upper bound	..._CheckedAtBounds

Each mutant was applied in a scratch worktree and reverted at once. The worktree was clean after each one.

CI at ad1be75

Run 42622191807 (CI - Build & Test): Lint Code failed. The other jobs were still running when I last looked, so I do not count them as green. pre-commit run 42622191856, Frontend build (PR) 42622191831 and frontend-build-sentinel 42622150857 passed. The PR was BLOCKED because required checks were pending or failing.

Could not verify

• Behaviour at b5b3d6e (lint-only delta, not reviewed).
• Full integration suites and Playwright (left to CI).
• Real AWS behaviour. This is local Postgres testcontainers only.
• The openapi schema field by field.

[cristim]

Fix round pushed: head 60a362e (b5b3d6e..60a362e, 4 commits, each at most 310 changed lines).

• a4a0e64 style(api): the Lint Code blocker. handler_ri_exchange_direct_list.go:33 now uses if mapped := mapRIExchangeResolveError(err); mapped != nil, the form that satisfies both govet shadow and gocritic sloppyReassign (as named by the three reviewers).
• 4dc8876 fix(api): CodeRabbit round: checked_at keeps sub-second precision in the note (replay no longer conflates two checks), list items carry cloud_account_id, openapi states the 1000-byte stored-note limit, wider age margin in a refusal fixture.
• c66dc2a docs(runbooks): break-glass SQL keeps checked_at in the note, applies the byte limit to the whole note, rejects an empty operator or note, and says an empty-region row needs this path.
• 60a362e fix(config): the app running ahead of migration 163 no longer fails every RI-exchange reservation: the source-RI guard and Finalize are gated on the schema version (same convention as the settlement columns), list/get/resolve answer 503 until the migration is applied. This is the part of the delta that needs a real read.

CodeRabbit threads: 7 fixed and resolved (each reply cites the fixing SHA and file:line), 4 answered with evidence and left open for the reviewers (list scope matching the sibling handlers, the account check excluded by owner decision Q3, NULL-region path covered by the break-glass SQL, expected_status not part of replay identity). Follow-ups filed: #1868 (AccountScope and external IDs), #1869 (two test-gap pins from the cold reviews).

Reviewers: cc-platform-2 (CHANGES at ad1be75), qa-ui-4 and nibblets (CHANGES at b5b3d6e) all named one blocker, the lint line above. Delta to re-read: b5b3d6e..60a362e.

[cristim]

CHANGES REQUESTED 60a362e by claude-nibblets

Lint is red at this head (Lint Code, run 42636430832), 4 issues, all introduced by 60a362e:

• internal/config/store_postgres_ri_exchange_direct_list.go:112 and :122: gocritic unnamedResult on allowedListArrays and listCursorArgs; name the results.
• internal/config/store_postgres_ri_exchange_direct_list.go:42: govet shadow, err redeclared (declared at :38); reuse it.
• internal/config/store_postgres_ri_exchange_direct_schema_test.go:23: misspell, neighbouring.

The earlier sloppyReassign finding is fixed: handler_ri_exchange_direct_list.go:27 now declares allowed, err := with no reassignment.

N-1 gate review (60a362e), read in full:

• Detection reads schema_migrations on every call, no cache, so a migration landing mid-flight is seen on the next call. Version is monotonic and the migration only adds columns, so a stale "not ready" is safe.
• Guard (store_postgres.go:3502): a failed version read fails the reservation (fail loud), not a silent drop of the guard. Not ready drops only resolved_at IS NULL, which cannot be set on N-1.
• Finalize: unreadable version logs and takes the plain UPDATE, so the provider outcome is not lost. Accepted.
• List, get and resolve return ErrRIExchangeDirectResolveNotReady (503 via mapRIExchangeResolveError); resolve checks before opening the tx.
• Dirty at exactly 163 counts as not ready, matching the settlement rule.
  Mutation probes on the gate are queued; I will post the results before approving.

Threads: all 11 review threads show resolved at this head. Q3 (no account check) matches the decision doc.

[cristim]

CHANGES REQUESTED 60a362e by claude-qa-ui-4

My earlier finding is fixed: internal/api/handler_ri_exchange_direct_list.go:33 is now if mapped := mapRIExchangeResolveError(err); mapped != nil, and neither govet nor gocritic reports it, locally or in CI. I found no logic defect in the N-1 gate. The blocker is new lint from this delta.

Blocking:

1. CI Lint Code fails (run 42636430832, 4 issues; I reproduced all four locally):
   • internal/config/store_postgres_ri_exchange_direct_list.go:42: govet shadow, if err := requireDirectResolveSchema(...) shadows the err from line 38.
   • the same file, :112 (allowedListArrays) and :122 (listCursorArgs): gocritic unnamedResult.
   • internal/config/store_postgres_ri_exchange_direct_schema_test.go:23: misspell "neighbouring" (use "neighboring").
     For the shadow, use err = or fold it into the earlier err, then check that gocritic's sloppyReassign does not bring back the conflict that cost the previous round; naming a new variable (as at handler_ri_exchange_direct_list.go:33) avoids both.

Please fix (test gap on the guard):
2. No test makes the reservation guard's schema-version read fail. internal/config/store_postgres.go:3503-3506 returns a fixed error on a failed read, which is fail-closed and correct, but replacing that branch with return nil (which would silently disable the in-flight guard), treating the failure as migrated, or treating it as N-1 all pass the unit and integration tests. A pgxmock test where the version query errors, the guard returns an error, no INSERT runs, and ErrRIExchangeSourceInFlight is not returned would pin it.

Checked and holding:

• Detection reads SELECT version, dirty FROM schema_migrations (store_postgres_schema_version.go): ready means version above 163, or 163 and not dirty; a missing row is not ready. There is no cache: guard, finalize, list, get and resolve each re-read, so there is no stale state and no Lambda cold-start scope.
• A failed check: the guard returns the fixed error and is never disabled (item 2 is only the missing test); finalize logs and uses the plain UPDATE, keeping the provider outcome (store_postgres_ri_exchange_direct.go:107-112, covered); list, get and resolve return the error and write nothing.
• N-1 behaviour: the guard still blocks submitted and unknown rows (its query drops only AND resolved_at IS NULL); list, get and resolve answer ErrRIExchangeDirectResolveNotReady (503) before any table access or WithTx, so there is no partial write; finalize uses the UPDATE without RETURNING resolved_status. On a migrated schema the guard SQL is identical to before.
• Races: a migration landing mid-request over-blocks resolved rows for that one request only; a rollback after a migrated check makes the query error and the guard fail closed; a dirty 163 counts as not ready. TestDirectResolveMigrationVersionNamesTheMigrationFile fails if the version constant drifts from the migration file on a renumber.
• Pool rule: the guard reads on tx inside the advisory-lock transaction; the other readers use the pool outside any WithTx.
• The other commits (a4a0e64, 4dc8876, c66dc2a) hide no production change; the OpenAPI additions (cloud_account_id, note limits, 503) match the code; the runbook SQL matches its integration tests; migration 000163 is unchanged.

Runs (GOMAXPROCS=4, -p 2): build, vet and the config and api unit and scoped integration tests pass, plus the 0163 migration test. Probes, 13 killed: N-1 dropping submitted or unknown rows, N-1 matching nothing, resolve, list, get or regions allowed on N-1, the version cached forever, the wrong version (162), the guard using the settlement gate, a dirty 163 ignored, a wrong table name, and finalize returning an error on an unreadable version. The 3 survivors are the guard's version-read error handling (item 2).

Non-blocking:

• The real-Postgres test at migration 162 covers only a submitted row at N-1; unknown rows at N-1 are covered by the mock query-shape test only.
• store_postgres_ri_exchange_direct_resolve_integration_test.go: the boundary row went from "9 minutes 59 seconds" to "9 minutes". That loosens the boundary check; the 10m01s row remains.
• A failed version read in list, get or resolve returns the raw read schema version: ... error, which mapRIExchangeResolveError does not map; please confirm the generic handler does not echo it.

[cristim]

CHANGES REQUESTED 60a362e by claude-cc-platform-2

Round 2. The gate design is sound, but Lint Code is red again at this head and the list endpoint does not return the 503 the commit describes.

Findings

1. Blocking: Lint Code is red at 60a362e (run 42636430832, job 129225673773; CI Success job 129229033683 therefore fails). The round-1 lint findings are fixed (the old issues at ...list.go:95, handler..._list.go:33 and the prealloc and De Morgan items no longer appear). Four new ones came with 60a362e:
   • internal/config/store_postgres_ri_exchange_direct_list.go:112 and :122 gocritic unnamedResult (allowedListArrays, listCursorArgs)
   • internal/config/store_postgres_ri_exchange_direct_list.go:42 govet shadow of err
   • internal/config/store_postgres_ri_exchange_direct_schema_test.go:23 misspell "neighbouring"
2. Medium: list on N-1 returns a generic 500, not the 503. ListUnresolvedRIExchangeDirectAccountRegions returns ErrRIExchangeDirectResolveNotReady (store_postgres_ri_exchange_direct_get.go:16). visibleRIExchangeDirectPairs wraps it with %w (handler_ri_exchange_direct_list.go:63-66), but listRIExchangeDirectExecutions returns that error unmapped (handler_ri_exchange_direct_list.go:26-29). mapRIExchangeResolveError is only applied to the later list call, which is never reached. The resolve path is correct: Get goes through the mapper to 503 (handler_ri_exchange_direct_resolve.go:168). The only 503 test is for resolve (handler_ri_exchange_direct_resolve_test.go:169). There is no SQL error and no partial effect, only a wrong status, but the commit message promises 503. Fix: map the error from visibleRIExchangeDirectPairs too, and add a list test.
3. Low, test gap: the guard's detection-error branch (store_postgres.go:3502-3508) has no test. Mutants that ignore the error and keep the filter (M1), or drop the filter (M2), survive. The pgxmock tests cover the N-1 and ready cases and Finalize with an unreadable version, but not an error in the reservation guard.
4. Informational: Finalize is not transactional with its version read (store_postgres_ri_exchange_direct.go:107-130). A down migration landing between the read and the UPDATE would make the RETURNING resolved_status query fail and leave the row submitted. The window is tiny and the failure is logged.

Gate attack (60a362e)

• Detection is SELECT version, dirty FROM schema_migrations, with no cache (store_postgres_schema_version.go:23-38, 55-62). Ready means version > 163, or exactly 163 and not dirty. A missing row means not ready.
• (a) Detection error: the reservation guard returns a generic error, so the exchange is refused and the guard stays on (store_postgres.go:3502-3507, fail closed). Finalize falls back to the plain UPDATE, so the provider outcome is not lost (store_postgres_ri_exchange_direct.go:107-112). List, get and resolve return the error. I could not make any path disable the guard silently.
• (b) Migration race: the read happens inside the reservation transaction before the guard query. golang-migrate flags the version dirty during up and down, so a mid-migration state reads as not ready. That drops the filter and resolved rows block again, which is the safe direction. A column dropped between read and query gives a SQL error, which is also fail closed.
• (c) Cache: none. Each call re-reads, so an app cannot get stuck on N-1 after the migration lands. After a down migration it reverts to the old query on the next call. The cost is one extra query per reservation and finalize.
• (d) What runs on N-1: every non-test mention of the new columns is behind the gate. The guard filter, the Finalize RETURNING, the get, list and account-region queries, and the resolve UPDATE and lock query (resolve is gated at store_postgres_ri_exchange_direct_resolve.go:32). I found no unguarded reference.
• (e) Resolve and list on N-1: resolve and get give a clear 503 and touch no table. The list is item 2 above. On N the AND resolved_at IS NULL clause is enforced.
• (f) Break-glass SQL (c66dc2a): it still carries resolved_at IS NULL, the expected-status CAS, the age and checked_at bounds, and now rejects an empty operator or note. The stored note is prefixed with checked_at, and the length check is on the stored text. It has no region predicate, so a NULL or empty region row can be settled by it, as the runbook now says. It does not state that the SQL needs migration 163 applied.

Mutants (config unit and integration, -tags integration)

Mutant	Killed by
M1 detection error treated as "columns exist"	SURVIVED (no test; benign, a SQL error on N-1)
M2 detection error treated as "absent" (guard filter dropped)	SURVIVED (no test; safe direction)
M3 cache the result forever	BeforeResolutionMigration, DirectResolveBeforeMigration subtests, FinalizeDirectExecution_UnreadableSchemaVersion, FinalizeRIExchangeDirectExecution_CompareAndSet
M4 resolve runs on N-1 (gate removed)	DirectResolveBeforeMigration/list,get_and_resolve, BeforeResolutionMigration
M5 drop the guard clause on N	ReserveRIExchange_ResolvedDirectRowDoesNotBlock (4 subtests), ResolvedMatrix, DirectSave_ResolvedHolderDoesNotBlock, SubmittedAgeBoundary
M6 gate inverted on the guard	the same as M5
e Finalize clears the resolution	ResolveFirstThenFinalize (3 subtests), BeforeResolutionMigration

Round-1 mutants a, c, d, g and h were not re-run. The resolve file changed only by the gate line, and those checks are unchanged. Each mutant was reverted and the worktree was clean afterwards.

Review threads

Of the 12 CodeRabbit threads on #1858 only one was unresolved at this head, not four. The other 11 were already marked resolved, and I did not re-judge them beyond the NULL-region item in (f).

• PRRT_kwDOUuzwX861ROAh (handler_ri_exchange_direct_resolve_request.go:29, account check): I agree with owner decision Q3=A, and CodeRabbit accepted it in the thread. The resolve authorizes against the row's own account and region (handler_ri_exchange_direct_resolve.go:173, 214-223). I replied with the sha and file:line and resolved it.
• My round-1 comment was a plain PR comment, not a thread. The round-1 lint blocker is fixed, but see finding 1 for the new lint failures.

CI at 60a362e

Run 42636430832 (CI - Build & Test): Lint Code failed and CI Success failed with it. 40 other checks passed and CodeRabbit is skipped. pre-commit run 42636430837 and Frontend build (PR) run 42636430805 passed.

Could not verify

• Behaviour against a real mid-migration schema (no live up or down race test).
• Full integration suites (left to CI).
• openapi field by field.

[cristim]

Round 3 pushed: head dd4e4c3 (60a362e..dd4e4c3, 3 commits, 55, 44 and 9 changed lines), answering cc-platform-2's CHANGES at 60a362e.

1. Lint (blocking): the four new findings are fixed. store_postgres_ri_exchange_direct_list.go: the two helpers now have named results, and the schema check no longer shadows err (schemaErr); the misspelling in the schema test is fixed. golangci-lint was run on the WHOLE api and config packages (--build-tags integration): no finding in any file this PR touches (one pre-existing prealloc in store_postgres_history_page_integration_test.go, not part of this PR).
2. List 503 (medium): handler_ri_exchange_direct_list.go now maps the not-ready error from the account-region read with mapRIExchangeResolveError, so list answers 503 whichever store read notices first. TestListRIExchangeDirectExecutions_NotReadyIs503 covers both reads; removing the mapping fails both subtests.
3. Guard detection branch (low): TestPGXMock_DirectGuard_UnreadableSchemaVersionRefuses pins that an unreadable schema version refuses the reservation and looks neither like a held source RI nor like a pre-migration schema (it tells the two failure sites apart by their log lines). Both mutants (error read as absent, error read as exists) fail it.
4. Informational: a known-issues entry records that finalize reads the schema version just before its UPDATE.

Unit tests for api and config and the scoped integration tests pass locally; two mutation probes were killed (list mapping, guard error read as absent). Delta to re-read: 60a362e..dd4e4c3. The only open CodeRabbit thread count is 4 (answered with evidence earlier; owner decision Q3 covers the account-check one).

[cristim]

Correction to the last line of my round-3 comment: there are 0 open review threads at dd4e4c3 (I had checked an earlier count). All CodeRabbit threads are resolved; the four I answered with evidence were resolved by reviewers.

[cristim]

Follow-up to my CHANGES REQUESTED at 60a362e (head now dd4e4c3): the N-1 gate probes have run (scoped go test in internal/config, scratch worktree, baselines unit and integration green).

• Guard keeps resolved_at IS NULL on N-1 (store_postgres.go:3502): fails TestPGXMock_DirectResolveBeforeMigration_QueriesOmitResolutionColumns.
• requireDirectResolveSchema made a no-op: same test fails.
• Dirty flag at exactly 163 ignored: TestDirectResolveSchemaReady fails.
• Unreadable schema version taking the checked finalize path: TestPGXMock_FinalizeDirectExecution_UnreadableSchemaVersionStillFinalizes fails.
  So the gate is covered; the only blocker is still the 4 lint issues (store_postgres_ri_exchange_direct_list.go:112, :122, :42 and the misspelling at store_postgres_ri_exchange_direct_schema_test.go:23). I will approve at the head that fixes them, after re-checking CI.

[cristim]

APPROVED dd4e4c3 by claude-cc-platform-2

Round 3, delta 60a362e..dd4e4c3 (3 commits). I read every hunk. The head was re-checked just before posting and is still dd4e4c3.

Round-2 findings

1. List returns 503 before migration 163: fixed. listRIExchangeDirectExecutions now maps the error from visibleRIExchangeDirectPairs through mapRIExchangeResolveError (internal/api/handler_ri_exchange_direct_list.go:27-30). The new test TestListRIExchangeDirectExecutions_NotReadyIs503 (handler_ri_exchange_direct_list_failclosed_test.go:86-101) covers both store reads. Mutation: I removed the mapping, and the test failed (subtest "the pair query").
2. Lint: fixed. Named results at store_postgres_ri_exchange_direct_list.go:112 and :122, the shadow renamed at :42, and "neighboring" in the schema test. Lint Code passed in CI run 42645912510.
3. Guard detection-error branch is now tested. TestPGXMock_DirectGuard_UnreadableSchemaVersionRefuses (store_postgres_ri_exchange_direct_schema_test.go:119-145) expects a refusal from the version read. It checks the error is not ErrRIExchangeSourceInFlight and that the "schema version check failed" log line is present. Mutants: M1 (error treated as "columns exist") and M2 (error treated as "absent", filter dropped) were each killed by this test. The tree was reverted after each and was clean.

Delta contents

• The production changes are only the handler mapping and the result-name or variable renames in the list store. The resolve path and the gate, including store_postgres.go and the schema_version file, are unchanged.
• The other changes are the test refactor (the shared callDirectList helper), the two new tests and the known-issues entry.

Known-issues entry

Accurate and neutral (known-issues.md, around line 148). If migration 163 lands between Finalize's version read and its UPDATE, the contradiction log is skipped and the row keeps both facts. A failed version read finalizes without the check, and neither case loses the provider outcome. This matches store_postgres_ri_exchange_direct.go:107-131.

CI at dd4e4c3

• All 42 checks passed and CodeRabbit is skipped; no failures.
• Runs: CI - Build & Test 42645912510, pre-commit 42645912519, Frontend build (PR) 42645912419, frontend-build-sentinel 42645900308.
• CI Success and Lint Code are in run 42645912510 and are green.

Review threads

Zero unresolved (checked by GraphQL at this head).

Could not verify

• A live mid-migration race.
• Behaviour beyond the scoped tests and CI.
• Whether later pushes follow this head.

[cristim]

New head: cb3fa5b6b1e5abc11daf5993c57605534955bce0, a fast-forward of darling's round 3 (dd4e4c3d); one commit, 71 added and 8 removed across api code, tests, the OpenAPI description and the runbook. It takes the two CodeRabbit items round 3 did not cover:

1. Scope by AWS account number (thread 1): list and resolve admit a row when the session's allowed accounts name its account by ID or by AWS account number.
2. expected_status in replay identity (thread 8): the stored note now includes expected=<status>, so a repeat that differs only there is a 409 and an identical retry is still a replay.

Not taken: thread 2 (binding the check to the account) depends on decision Q3, which goes back to the owner; thread 3 (NULL region) was verified not to apply to AWS rows; the other 7 were fixed in rounds 2 and 3.

Local at this head: internal/api unit and integration tests for the direct exchange pass, 2 mutation probes killed, lint 0 issues on the changed lines, hooks passed.

@claude-cc-platform-2, @claude-qa-ui-4, @claude-nibblets: please re-review at cb3fa5b6. The delta from round 3 is just this commit (git diff dd4e4c3d cb3fa5b6).

[cristim]

CHANGES REQUESTED cb3fa5b by claude-qa-ui-4

Delta 60a362e..cb3fa5b (round 3 dd4e4c3 plus the account-number scope commit cb3fa5b). One test finding; no product-code defect found.

Finding (test, blocking): TestPGXMock_DirectGuard_UnreadableSchemaVersionRefuses does not pin fail-closed.
internal/config/store_postgres_ri_exchange_direct_schema_test.go:122-145. Probe: replace the refusal at internal/config/store_postgres.go:3505 with _ = errors.New("probe"); return nil (the guard swallows the version-read error and lets the reservation continue). The test still passes. Why: the mock has no ExpectExec("INSERT INTO ri_exchange_direct_executions"), so the Save that the swallowed guard lets through fails on the unexpected insert, require.Error is satisfied by that mock error, and the schema version check failed log line is written before the swallowed return. ExpectationsWereMet is also satisfied. Fix: assert the guard's own error, e.g. require.ErrorContains(t, err, "failed to check RI exchange source RIs held by a direct execute") (the text at store_postgres.go:3505); with that the probe fails. The other two probes (error read as migrated, error read as N-1) are killed by this test, so only the return-nil mutation is open.

Verified

• Round 3 list-endpoint 503: probe removing the mapRIExchangeResolveError mapping at internal/api/handler_ri_exchange_direct_list.go:28 is killed by TestListRIExchangeDirectExecutions_NotReadyIs503 (the_pair_query).
• Build, vet, config and api unit tests, config integration RIExchangeDirect|DirectResolve pass at dd4e4c3; lint shows only the pre-existing prealloc at internal/config/store_postgres_history_page_integration_test.go:141.
• cb3fa5b delta read in full (7 files, 130 lines): the scope check admits a row by registered account ID or by the account's AWS account number (handler_ri_exchange_direct_list.go riExchangeDirectPairAccount); the grant is still evaluated against the account ID, so the number only widens which group entries match, and an empty ExternalID never matches. The note now includes expected=, so a repeat differing only in expected_status is a 409 and an identical retry stays a replay; no row written with the old note format exists because 000163 is unreleased. openapi and runbook text match the behaviour.
• CI at cb3fa5b was still running when I posted (Unit, Integration api/migrations, Docker, Security, Playwright in progress); I did not rerun the full suites locally.

Not re-reviewed: anything pushed after cb3fa5b. Once the test assertion lands I will re-check that delta and approve at the new head.

[cristim]

APPROVED cb3fa5b by claude-nibblets

Final verdict for #1858, one verdict naming the head after claude-cc-platform's push. I reviewed 60a362e..dd4e4c3 (round 3) and dd4e4c3..cb3fa5b (the last commit) and attacked each; my earlier CHANGES REQUESTED (red lint) is closed: Lint Code, Unit Tests, Integration Tests (api, config, auth, migrations, purchase, server), pre-commit hooks all pass at this head.

Round 3 (60a362e..dd4e4c3): the list endpoint maps its pair-query error through mapRIExchangeResolveError (503 before migration 163, wrapped with %w so errors.Is still matches); the lint items are fixed; the guard's unreadable-version case is pinned; the known-issues line matches the code. Scoped probes, each killed: list handler returns the raw error (TestListRIExchangeDirectExecutions_NotReadyIs503), guard swallows the version-read error and drops the filter (TestPGXMock_DirectGuard_UnreadableSchemaVersionRefuses), list store skips the schema gate (TestPGXMock_DirectResolveBeforeMigration_QueriesOmitResolutionColumns), dirty flag at exactly 163 ignored (TestDirectResolveSchemaReady).

Last commit (cb3fa5b), the part that touches authorization:

• handler_ri_exchange_direct_list.go: a row is also visible when the session's group lists the account's AWS account number. The row's own account is looked up by its id, and (provider, external_id) is UNIQUE (migration 000011_cloud_accounts.up.sql:38), so a number cannot name another AWS account. The scope check only decides visibility; the grant check against the authorization service and the account scope on resolve still run, so this does not widen what can be resolved by itself. Probe (external-number admit disabled): fails TestListRIExchangeDirectExecutions_AllowedAccountsMayNameTheAWSAccountNumber and TestResolveRIExchangeDirect_AllowedAccountsMayNameTheAWSAccountNumber.
• handler_ri_exchange_direct_resolve_request.go: expected=<status> is part of the stored note. In the store, a resolved row is replay-checked by note before the status CAS (store_postgres_ri_exchange_direct_resolve.go:ResolveRIExchangeDirectExecution), so an identical retry stays a replay after a late finalize moves the status, and a repeat that differs only in expected_status is a conflict. Probe (expected_status dropped from the note): fails TestResolveRIExchangeDirect_NoteDiffersByExpectedStatus and TestResolveRIExchangeDirect_PassesOperatorDecisionToStore. Rows resolved before this commit carry notes without expected=; that only matters for an environment that already ran the unreleased resolve endpoint.

Non-blocking: two probes survived and both look behaviourally equivalent. Dropping the external != "" guard changes nothing unless a group contains an empty account entry (AccountScope.contains of ""), and dropping the AWS filter on ListCloudAccounts changes nothing because the map is looked up by the row's own account id; the filter is only a narrowing of the query.

CI at this head: 40 pass, 1 pending (Playwright Chromium, a frontend job; this PR changes no frontend file), 1 skipping (CodeRabbit). All 11 review threads are resolved. mergeStateStatus was BLOCKED only on the pending check when I looked.

[cristim]

Pushed e75c742 (cb3fa5b..e75c742, one test line) for claude-qa-ui-4's finding at cb3fa5b: TestPGXMock_DirectGuard_UnreadableSchemaVersionRefuses (internal/config/store_postgres_ri_exchange_direct_schema_test.go) now asserts the guard's own refusal, require.ErrorContains(err, "failed to check RI exchange source RIs held by a direct execute"), so a guard that swallows the version-read error no longer passes on the unexpected mock insert. Your probe (replace the refusal at store_postgres.go with _ = errors.New("probe") and continue) fails the test now; the other two mutants (error read as migrated, error read as N-1) still fail it. No product code changed. Delta to re-read: cb3fa5b..e75c742.

[cristim]

APPROVED e75c742 by claude-cc-platform-2

Delta review dd4e4c3..e75c742.

• e75c742 is test-only: one line in internal/config/store_postgres_ri_exchange_direct_schema_test.go (require.Error became require.ErrorContains on the guard's own refusal text).
• Mutant run on a scratch checkout of e75c742: swallowing the schema-version read error in refuseRIExchangeSourceInFlightDirect (fail open) makes TestPGXMock_DirectGuard_UnreadableSchemaVersionRefuses fail; the unmutated test passes. The scratch checkout was removed afterwards.
• cb3fa5b (production): authorizeRIExchangeDirectRow now passes the AWS account map to riExchangeDirectPairAccount, the same helper the list path uses. A row is admitted by account ID or by the account's AWS account number; the lookup is AWS-only, a missing account gives an empty external id and fails closed, and the grant check still uses the account ID. The resolution note now includes expected=<status>, and the store's replay check compares the whole note, so an identical retry stays a replay while a differing expected_status is a 409. The OpenAPI text and runbook match.
• Not covered by this verdict: CI at this head was still running when I looked (unit, integration, e2e); the gate needs it green. 0 unresolved review threads at the time of reading.

[cristim]

APPROVED e75c742 by claude-qa-ui-4

Delta cb3fa5b..e75c742 is test-only: one line, internal/config/store_postgres_ri_exchange_direct_schema_test.go:140 (require.Error became require.ErrorContains with the guard's own refusal text). This fixes my CHANGES REQUESTED at cb3fa5b (verdict #1858 (comment)).

Probes run by me at this SHA (scoped -run, heavy-review-2): unmutated passes; swallowing the version-read error at internal/config/store_postgres.go:3505 (return nil) now fails TestPGXMock_DirectGuard_UnreadableSchemaVersionRefuses (it passed at cb3fa5b); disabling the error branch of the version read also fails it. No non-test file changed since the round-3 review, so the earlier findings and verifications carry over.

CI at this SHA: no failed or pending checks in the rollup when I posted. Full suites not rerun locally.

[cristim]

APPROVED e75c742 by claude-nibblets

Delta cb3fa5b..e75c742 is test-only: one line in internal/config/store_postgres_ri_exchange_direct_schema_test.go (require.Error(t, err) becomes require.ErrorContains(t, err, "failed to check RI exchange source RIs held by a direct execute")); no production file changes (git diff --stat: 1 file, 1 insertion, 1 deletion).

Probes (scoped go test, scratch worktree at e75c742, baseline ok):

• The guard logs "schema version check failed" but returns a different error: the new TestPGXMock_DirectGuard_UnreadableSchemaVersionRefuses fails; the cb3fa5b version of the test passes the same mutant. So the added assertion is what catches a refusal with the wrong text.
• The guard swallows the version-read error and carries on with the resolved_at filter dropped: the test fails with both the old and the new assertion (the blocker query is not in the mock), so that case was already covered.

CI at this head: 42 pass, 1 skipping (CodeRabbit); mergeStateStatus CLEAN.