[IAmJSD]

The openapi spec did not match the handler on the RI exchange region. POST /api/ri-exchange/execute listed required: [ri_ids, targets, max_payment_due_usd] without region, but validateExecuteExchangeBody rejects an execute without a region with 400. The quote response was a bare type: object, so the Region it always returns (and that callers must send back on execute) was not documented. This PR fixes both and adds a test that keeps the spec in step with the handler. It also trims the two handler comments that retold the history of #238.

Closes #363

How each item of #363 is now covered

1. TS types (ExchangeQuoteSummary.Region, ExchangeExecuteRequest.region required, call sites fixed, type-level regression test): done in chore(riexchange): make the exchange region required in the TS types #1350.
2. openapi: done here. Execute lists region as required and documents it. The quote 200 response documents Region as a required string that must be sent back as region on execute.
3. History comments: chore(riexchange): make the exchange region required in the TS types #1350 trimmed the frontend one (the riexchange.ts comment the issue named was already gone on main; the remaining one was in types.ts). This PR trims the two in internal/api/handler_ri_exchange.go: the one in getExchangeQuote (it was at lines 1725-1730 when the issue was filed) and the ExchangeQuoteResponse type comment. Both now state only the constraint. No #238 history comment remains in non-test frontend or backend code (grep -rn "#238" frontend/src internal/ returns only test files).

What changed

• internal/api/openapi.yaml: added region to the execute request's required list, with a region property (string) and a description. Added required: [Region] and a Region property to the quote 200 response schema. Both schemas stay open objects, so the other quote fields, which are still undocumented, remain valid.
• internal/api/handler_ri_exchange.go: comment-only changes. No behavior change.
• New internal/api/openapi_ri_exchange_region_test.go (TestOpenAPIRIExchangeRegionRequired):
  • checks that validateExecuteExchangeBody returns 400 "region is required" for an otherwise valid body with no region;
  • takes the field names from the Go json tags (by marshaling ExchangeExecuteRequestBody and ExchangeQuoteResponse), not from hand-copied strings;
  • asserts that the execute schema requires and documents region, and that the quote 200 schema requires and documents Region and tells callers to send it on execute.

Why this approach

This is a spec-only change plus a guard test, in the same style as the existing openapi_*_test.go checks (yaml.v3 over the embedded openapiSpec). Taking the key names from the json tags means a renamed tag fails the test, so the spec cannot drift from the handler without anyone noticing. The quote request's region stays optional, as it is in the handler, which defaults it from the SDK chain.

Verification

Unit and spec-fixture evidence on this branch (head 9c8457a5, on main 3bfc2fe5):

• Fail-before: with the openapi.yaml change stashed, go test ./internal/api/ -count=1 -run TestOpenAPIRIExchangeRegionRequired FAILS with []string{"ri_ids", "targets", "max_payment_due_usd"} does not contain "region" / execute must document the region property.
• Pass-after: the same command passes.
• Mutation probe: removing only required: [Region] from the quote response makes it fail with quote response must list Region as always present.
• go test ./internal/api/ -count=1: ok. go test ./internal/mocks/ -count=1: ok.
• go build ./..., go vet ./..., go vet -tags integration ./...: clean.
• golangci-lint run --allow-parallel-runners ./internal/api/ (v2.10.1): 0 issues. gocyclo -over 10 -ignore "_test\.go" internal/api/handler_ri_exchange.go: no output.
• The repo has no openapi linter in its Makefile, scripts or CI (only pre-commit check-yaml and the Go spec tests). As an extra check, npx @redocly/cli@2.71.1 lint internal/api/openapi.yaml reports "Your API description is valid" with the same 42 warnings as on main (warning counts by rule are identical before and after; no new findings).
• No integration tests: there is no DB or runtime change.

Combined-tree verification at local queue commit 2be43e1e = origin/main b9271e88 + fix/issue-363-next, fix/issue-177-next, fix/issue-919-next, fix/issue-313-next, fix/issue-532-next and fix/issue-316-next merged in that order. Passed: go build ./...; go vet with and without -tags integration; go test -race -short -count=1 ./...; golangci-lint v2.10.1 with and without the integration tag; gocyclo -over 10; go test -race -tags integration on internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase, internal/server and internal/testutil; the git-secrets scan and scripts/test-git-secrets-allowlist.sh.

Out of scope (noted, not changed)

• The execute schema still lists targets as required, but the handler also accepts the legacy target_offering_id + target_count form with no targets. Same for the quote request. If the legacy form is still supported, the spec should document it (for example with oneOf). That was not part of chore(riexchange): make exchange region required in TS types and openapi (follow-up to #355) #363, so it is left alone here.
• Apart from Region, the quote response's fields (the promoted ExchangeQuoteSummary fields) are still undocumented in the spec.

Maintainer: please mirror the issue's triage labels on this PR: type/chore, triaged, priority/p3, severity/low, urgency/this-quarter, impact/few, effort/xs.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

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

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 3317da7
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5dd3fec1-9290-4aee-bba7-cf80e41ab573

Commits

Reviewing files that changed from the base of the PR and between 3317da7 and 9c8457a.

Files selected for processing: 3

• internal/api/handler_ri_exchange.go
• internal/api/openapi.yaml
• internal/api/openapi_ri_exchange_region_test.go

• 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 9c8457a522d04bd5c230f052e4afa54c75063226

Blocking:

• none

Non-blocking:

• internal/api/openapi.yaml (quote 200 Region, "Always set"): getExchangeQuote returns cfg.Region from loadAWSConfigWithRegion. When the caller omits a region and the SDK chain resolves none, Region would be "". In practice the real quoter fails on a missing region before a 200 can go out, so the claim holds. A 400 in the handler when region == "" after resolution would make that explicit instead of leaving it to the SDK. Follow-up, not this PR.
• The spec gaps listed under Out of scope (the legacy target_offering_id/target_count form, and the undocumented quote summary fields) are real and fine to leave.

Attacked:

• Checked the spec against the handler. validateExecuteExchangeBody rejects Region == "" with 400. The quote returns ExchangeQuoteResponse{..., Region} with json key Region. The quote request's region stays optional, which matches the handler's default from the SDK chain. openapiSpec is only served by handler_docs.go and not used for runtime request validation, so making region required cannot change behavior.
• Fail-before: put main's openapi.yaml back and ran go test ./internal/api/ -run TestOpenAPIRIExchangeRegionRequired. It fails ([...] does not contain "region", execute must document the region property). With the PR's spec restored it passes.
• Ran go build ./..., go vet ./internal/api/ with and without -tags integration, and go test -race -short -count=1 ./internal/api/ (ok). golangci-lint v2.10.1 with and without the integration tag: 0 issues. gocyclo -over 10 on handler_ri_exchange.go: clean. The handler change is comments only.
• Closes #363 is right: chore(riexchange): make the exchange region required in the TS types #1350 (merged) made Region/region required in frontend/src/api/types.ts, this PR covers the openapi item, and grep -rn '#238' frontend/src internal/ finds only test files.

Body-stale:

• none

_{Reviewer: rF5. 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 9c8457a522d04bd5c230f052e4afa54c75063226

Blocking:

• none

Non-blocking:

• internal/api/openapi.yaml execute schema: required still lists targets, but validateExecuteExchangeBody (handler_ri_exchange.go:1944) also accepts the legacy target_offering_id + target_count form. The PR body already notes this as out of scope. A follow-up should document the legacy form (oneOf), or drop it if no caller still uses it.

• jsonKeys is a very generic helper name to add at package level in internal/api tests. A later test file that declares the same name breaks the package build. riExchangeJSONKeys would be safer.

• (From the earlier rF5 review, confirmed here.) "Always set" on the quote's Region depends on the SDK chain resolving a region. getExchangeQuote echoes cfg.Region with no empty check. In Lambda AWS_REGION is always set, and the quoter would fail on an empty region before a 200 goes out, so the claim holds in practice. A handler-side 400 on an empty resolved region would make it explicit.

Attacked:

• Checked the spec against the handler. validateExecuteExchangeBody refuses an empty region with 400 unconditionally, after the targets and max-payment checks, so required: [..., region] is correct. ExchangeQuoteResponse.Region is tagged json:"Region" with no omitempty, so required: [Region] on the quote 200 is correct. The quote request's region stays optional, which matches the handler. The two trimmed comments are comment-only, there's no behavior change, and they state the constraint accurately. grep -rn "#238" over non-test frontend/src and internal/ comes back empty. Items 1 and 3 (frontend) landed in chore(riexchange): make the exchange region required in the TS types #1350 (e673bb5 on main), so Closes #363 is justified.
• Fail-before: I replaced openapi.yaml with origin/main's copy and ran go test ./internal/api/ -run TestOpenAPIRIExchangeRegionRequired. It FAILS with []string{"ri_ids", "targets", "max_payment_due_usd"} does not contain "region" and execute must document the region property. After restoring the file, all TestOpenAPI* pass.
• Ran in a scratch worktree at 9c8457a: go build ./..., go vet ./... and go vet -tags integration ./... (clean), go test -race -short -count=1 ./internal/api/ (ok), golangci-lint v2.10.1 on ./internal/api/ with and without --build-tags integration (0 issues), and gocyclo -over 10 on handler_ri_exchange.go (silent). I didn't run integration tests: the change is spec, comment and a unit test only.

Body-stale:

• none

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

[cristim]

VERDICT: APPROVE 9c8457a
PR #1511 is OPEN. I reviewed origin/main (ec78d85)...9c8457a, which is one commit and touches 3 files.

Findings:

• nit internal/api/openapi.yaml:2390 - the region property has no minLength: 1. The handler (handler_ri_exchange.go:1963) rejects "", but the spec accepts it. Failure scenario: a generated client or validator treats "region": "" as valid and gets a 400. Same for the quote Region at openapi.yaml:2326.
• nit internal/api/handler_ri_exchange.go:1963 - this predates the PR. The check is body.Region == "" with no TrimSpace, so a whitespace-only region gets past validation and reaches the AWS SDK. AWS rejects it, so no wrong-region spend.
• nit, pre-existing, not introduced here: frontend/src/api/types.ts:925-927. ExchangeResult.quote is typed ExchangeQuoteSummary, which has a required Region (from chore(riexchange): make the exchange region required in the TS types #1350). The execute handler returns a bare *exchange.ExchangeQuoteSummary (handler_ri_exchange.go:2191-2194, :2048), which has no Region. Failure scenario: a future caller reads result.quote.Region and gets undefined. No caller does today. Worth a known-issues entry or follow-up.
• nit, acknowledged in the PR body: the execute and quote schemas still list targets as required, but validateExecuteExchangeBody (:1944) accepts the legacy target_offering_id + target_count form. This is out of scope for chore(riexchange): make exchange region required in TS types and openapi (follow-up to #355) #363.

Field-by-field check against the handlers:

• Execute request region, required in the spec at openapi.yaml:2377. The Go field is ExchangeExecuteRequestBody.Region with tag json:"region,omitempty" (handler_ri_exchange.go:2169). The server rejects a missing region with 400 at :1963-1965. validateExecuteExchangeBody is called at :1993, before the permission/constraint check at :2005 and before the idempotency claim. The region is passed through unchanged to authorizeAWSExchangeExecution and the executor (:2002, :2027). So required matches the server.
• Execute ri_ids (:2170, validated at :1941) and max_payment_due_usd (:2168, validated at :1956) match. The targets mismatch is the nit above.
• Quote 200 Region, required in the spec at openapi.yaml:2324. The Go field is ExchangeQuoteResponse.Region with tag json:"Region", no omitempty (:2200-2202), so the key is always serialized. The handler sets it from cfg.Region at :1913 and :1932. When the SDK chain resolves no region, the quoter call fails first, so a 200 carries a non-empty value in practice. The embedded library ExchangeQuoteSummary (cloud-commitments-go pkg@9cec521d6531 exchange/exchange.go:27-44) has no Region field, so nothing shadows it. The quote schema stays an open object, so the undocumented promoted fields remain valid.
• The quote request's region stays optional. That is correct: the handler defaults it through loadAWSConfigWithRegion (:1381-1390, :1909).
• The PR only edits comments in handler_ri_exchange.go (:1929-1931, :2196-2198) and makes no behavior change. The new comments are accurate.

Frontend:

• There is one execute call site: frontend/src/riexchange.ts:1637-1641. It sends region: quote.Region.
• The API wrapper is frontend/src/api/riexchange.ts:135. ExchangeExecuteRequest.region: string is required (types.ts:922) and ExchangeQuoteSummary.Region: string is required (types.ts:906), both from chore(riexchange): make the exchange region required in the TS types #1350. So the compiler enforces it.
• No other frontend code calls /ri-exchange/execute. This PR does not touch the frontend, so I ran no jest.

Attacked:

• Ran go test -race -count=1 -run 'TestOpenAPI|Exchange' ./internal/api/ on the archived head: 502 passed. That covers every existing openapi_*_test.go plus the new test. go vet ./internal/api/ is clean and gofmt -l is clean.
• Checked that the head diff against current main for these files is only the PR's own change, so there is no conflict with main.
• Ran four mutation probes on the scratch copy. All four failed as expected, and each file was restored and checked byte-identical with cmp:
  • P1: removed region from execute required. FAIL "does not contain region".
  • P2: disabled the handler check (if false && body.Region == ""). FAIL "An error is expected but got nil".
  • P3: renamed the response tag to json:"region". FAIL on the Region json key guard.
  • P4: removed required: [Region] from the quote 200 response. FAIL "[]string(nil) does not contain Region".
• Refuted my own candidate finding that "Region could be empty on a 200". loadAWSConfigWithRegion leaves cfg.Region empty only when the SDK chain has no region, and then the EC2 quote call errors before the handler returns.
• I found nothing on the security dimension. The PR changes only the spec, comments and a test. The auth and permission order is unchanged.

[IAmJSD]

Local verification at 9c8457a522d04bd5c230f052e4afa54c75063226 merged onto main (ec78d85c), together with #1509, #1511, #1515 in that order (combined tree f2762a5c, 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 3 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: 3 approving independent review(s) on 9c8457a5 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 9c8457a522d04bd5c230f052e4afa54c75063226.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 9c8457a
Note: already MERGED. origin/main (fetched, 2faf67f) contains squash commit 53d165a "docs(api): require the RI exchange region in openapi.yaml (#1511)", whose diff matches this head's 3 files (+126/-12). The test file is byte-identical on main.

Findings:

• internal/api/openapi.yaml:2377 - the execute required list this PR edits still names targets, but validateExecuteExchangeBody (internal/api/handler_ri_exchange.go:1944) also accepts the legacy target_offering_id + target_count instead, and frontend/src/api/types.ts marks targets? optional. target_offering_id and target_count are not in the spec, and targets items are a bare type: object with no offering_id/count. A client generated from the spec would reject a legacy-form body the handler accepts. This was already wrong before the PR, which only added region. [non-blocking, follow-up]
• internal/api/openapi.yaml:2314 - the quote request body is still a bare type: object, so its optional region (ExchangeQuoteRequestBody, handler_ri_exchange.go:2153) and its required ri_ids/targets go undocumented. Only the response side of the quote was in scope here. [non-blocking]
• PR body says the test "derives the field names from the Go json tags". It actually hardcodes "region"/"Region" and asserts the marshalled tags contain them. Same effect (probes below), but the wording overstates it. [nit]

Attacked:

• Spec vs handler. Execute: ri_ids (array of string), max_payment_due_usd (string), region (string, now required) match the json tags at handler_ri_exchange.go:2166-2171. Required region matches the 400 at :1963. Quote 200: Region matches the json:"Region" tag at :2201 (no omitempty, so the key is always emitted, including when the embedded *ExchangeQuoteSummary is nil). The library ExchangeQuoteSummary (pkg@9cec521d6531 exchange/exchange.go:27-44) has no Region field to collide with. The frontend reads quote.Region (frontend/src/riexchange.ts:1640, riexchange-confirm.ts:55; types.ts:906 Region: string), so the casing matches. Error paths return the Error schema with no Region, and the spec claims Region only on 200. An empty Region on a 200 needs an empty base SDK region (loadAWSConfigWithRegion :1381-1389), where the EC2 quote call itself fails first, so I could not make it happen.
• Mutation probes in a scratch copy of the head, using TestOpenAPIRIExchangeRegionRequired. Each one was restored with cp and checked with cmp, and a final diff -r against the head export was identical. All KILLED: execute tag region->exchange_region; quote tag Region->region; region check disabled in validateExecuteExchangeBody; region dropped from execute required; required: [Region] dropped from the quote 200; the whole Region property dropped from the quote 200; execute region type string->integer.
• Non-comment diff of handler_ri_exchange.go (base vs head, with // lines stripped): empty. The behaviour is unchanged.
• Spec validity: the repo has no spectral/redocly config or lint step, and openapi.yaml is go:embed'd (handler_docs.go:11) and checked by the Go tests (dialect, route coverage, password encoding). All 7 TestOpenAPI* tests pass. No generated client or snapshot exists (no oapi-codegen, openapi-generator or openapi-typescript), so nothing needed regenerating. No standalone validator is installed offline, so I did no external schema validation.
• Security: no auth, scope or error-body changes. No migrations.

Verified locally (GOTOOLCHAIN=go1.26.10, AWS config files set to /dev/null, cloud env unset):

• Fail-before: copied the new test into the base export, ran go test -count=1 -run TestOpenAPIRIExchangeRegionRequired ./internal/api/: FAIL ("execute must list region as required", "execute must document the region property").
• Head: gofmt -l on the 2 changed .go files: clean. go vet ./internal/api/: clean. go test -count=1 -run TestOpenAPI ./internal/api/: 7 passed. nice -n 10 lockf -k -t 3600 /tmp/claude/heavy.lock go test -race -count=1 ./internal/api/: ok (31.9s).
• golangci-lint run ./internal/api/ (v2.11.4, repo .golangci.yml, under the heavy lock): 0 issues.
• git merge-tree --write-tree origin/main 9c8457a5... with origin/main at 2faf67f: clean (tree b66c4bfd), as expected for an already-merged change.

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