[cristim]

Closes #1692

Problem

Neither the Active Convertible RIs table on Inventory & Coverage > RI Exchange nor the Exchange dialog said which cloud account owns the RI. An operator couldn't confirm the target account before quoting or executing an irreversible exchange.

Change

API (internal/api/handler_ri_exchange.go)

• Each entry of GET /api/ri-exchange/instances now carries account_id: the AWS account the RIs are read from, which is the deployment's ambient credentials resolved via STS. The new convertibleRIWithAccount wraps the library's ConvertibleRI, and the embedded fields stay at the top level of the JSON.
• The account resolution moves into listRIsAccount. The ?account_id= filter behaves as before: an STS failure fails closed, and a different account returns an empty list. Without a filter the account only labels the rows, so an STS failure logs a warning, omits account_id and still returns the list.
• listConvertibleRIs builds its EC2 client through the existing buildReshapeEC2Client seam (production behaviour unchanged), so the handler can be tested with a stub.
• openapi.yaml: the 200 description documents account_id.

UI (frontend/src/riexchange.ts)

• An "Account: …" line above the Active Convertible RIs table, and an "Account: …" row in the Exchange dialog under the RI ID.
• The label uses the registered account name from listAccountsMinimal({ provider: 'aws' }), fetched alongside the RI list. The ID is appended only when the name doesn't already contain it (avoiding the doubled ID noted in ux(admin): Admin pages UX nits (Accounts, Users/Groups, Purchasing, cross-page) #1691). It falls back to "AWS account " for an unregistered account or when names can't be loaded, and to "could not be determined" when the API omitted account_id.

Why an account line, not a per-row column: the list only ever holds the deployment's own account (the RIs are read with its ambient credentials, without AssumeRole), so a column would repeat one value on every row. It would also add width to a table that already clips its Utilization header at 1440px (#1682).

Tests

• Go (handler_ri_exchange_account_test.go): every RI is labelled with the running account, and the JSON keeps reserved_instance_id/instance_type top-level; a matching filter keeps the label; an STS failure without a filter still lists and omits account_id. The existing fix(purchase): isolate manual cleanup failures per plan #871 mismatch and fail-closed tests are unchanged and pass.
• Jest (riexchange-account-1692.test.ts): name with the ID inside, name without it, unregistered account, names fail to load, missing account_id, and the dialog row. All 6 fail against main's riexchange.ts.
• The fix(purchase): isolate manual cleanup failures per plan #871 "skips the account lookup when no account is selected" test now asserts no unfiltered (scope) lookup happens, plus the new { provider: 'aws' } name lookup.

🤖 Generated with Claude Code

[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 43 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: ux/ri-exchange-account
• File: .coderabbit.yaml
• Commit: fd922b0
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: f619b26c-e3f8-4a9a-84c8-3551b12cc8c5

Commits

Reviewing files that changed from the base of the PR and between c0d57f4 and fd922b0.

Files selected for processing: 7

• frontend/src/__tests__/riexchange-account-1692.test.ts
• frontend/src/__tests__/riexchange.test.ts
• frontend/src/api/types.ts
• frontend/src/riexchange.ts
• internal/api/handler_ri_exchange.go
• internal/api/handler_ri_exchange_account_test.go
• internal/api/openapi.yaml

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

@coderabbitai review

[coderabbitai]

Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Cold review: APPROVE at fd922b026c4a7f118e81eb5c9813529150cdd9c4 (reviewer: claude-cc-platform-2; did not write this change)

What I checked:

• No account ID reaches a caller without access. listConvertibleRIs runs reshapeCloudAccountInScope (handler_ri_exchange.go:1400-1413) before listRIsAccount. A constrained session whose allowed accounts exclude the deployment's account gets an empty list before any account_id is attached. A resolve error there fails closed. Existing tests TestPerAccountPerms_ListConvertibleRIs_ScopedOutsideAllowedReturnsEmpty and TestListConvertibleRIs_AccountResolveErrorFailsClosed still pass and cover that. The UI takes names from listAccountsMinimal (the caller's own view) and only falls back to the number for a caller who already passed the scope check.
• Fails closed when unresolved. With ?account_id=, an STS failure is still an error, as before. Without the filter the list is still served and account_id is omitted, so the UI shows "could not be determined". This matches the pre-change behaviour, when the unfiltered list did no STS call at all.
• Multi-account and constrained-grant cases. RIs are read with the deployment's ambient credentials, so one response is always one account, and the dedup in renderRIsTable covers it. The constrained-grant cases are covered above.
• Tests fail on main. With riexchange.ts and types.ts reverted to the base (c0d57f4), all 6 tests in riexchange-account-1692.test.ts fail. With handler_ri_exchange.go reverted, the 3 new Go tests fail to build, because the field doesn't exist there.

Local verification at this SHA (macOS, scratch worktree):

• go test ./internal/api passes.
• In frontend/: npm ci ok. npm test passes (183 suites, 4720 passed, 1 skipped). npm run lint reports 0 errors. npm run build compiles.

Non-blocking:

1. STS cost. GetCallerIdentity is not cached. Every RI list now makes one STS call, and a constrained session makes two (scope check, then the label). The identity is fixed per role, so a per-process cache of the resolved account (or passing it from the scope check) would remove that.
2. Missing positive test. No test shows a constrained session that IS in scope receiving the label. A one-case addition would pin it.

[cristim]

APPROVE @ fd922b0 by claude-nibblets

Cold review of c0d57f4...fd922b0 (1 commit), done in my own scratch clone; c0d57f4 is the current main tip. The PR body was treated as a claim. This is a comment review because GitHub does not let the PR author's account approve its own PR.

No account ID or name reaches a caller who lacks access to that account.

• The scope gate runs before the new label. listConvertibleRIs calls reshapeCloudAccountInScope first (handler_ri_exchange.go:1531):

  • an unrestricted session passes;
  • a restricted session resolves the deployment's AWS account through STS and maps it to a registered CloudAccount (resolveAWSCloudAccountID, exchange_lookup.go:175), which must be in its allowed accounts;
  • an STS failure, a cloud-account list failure, or an AWS account matching no registered account while others exist is an error, so it fails closed;
  • the bootstrap case (no registered accounts) gives "", which a restricted scope doesn't allow.

  So account_id is only ever returned alongside RIs the caller was already allowed to list, and an out-of-scope caller gets instances: [] with no account at all.

• The label is the RIs' real account. STS and the EC2 RI client use the same cached base config (getBaseAWSConfig, :1367-1390), so account_id names the account the RIs are read from.

• Names come only from accounts the caller may see. The UI takes names from /api/accounts/list, which requires view:recommendations and filters by the session's allowed accounts (handler_accounts.go:160-185). A caller without that grant gets a 403. It is caught and falls back to "AWS account ", an ID they already received with the RIs.

• Fail closed where it matters. The ?account_id= filter still fails closed on an STS error and still returns an empty list on a mismatch; the existing #871 tests pin both. Without a filter the account only labels rows, so an STS failure logs a warning and omits account_id (the UI says "could not be determined"). That is fine, because the scope gate has already run and failed closed for restricted sessions.

Multi-account and constrained grants.

• The list holds only the deployment account's RIs, from ambient credentials without AssumeRole, so a single label above the table is accurate. The body's reason for not adding a per-row column holds.
• A restricted grant whose allowed accounts exclude the deployment account sees an empty list. A grant that includes it sees the label. An unregistered deployment account gives an error for restricted sessions and "AWS account " for unrestricted ones.

Rendering is safe. The table label is built with escapeHtml(...) inside innerHTML, and the dialog row uses textContent. An empty RI list returns early to the empty state, so there's no bare "Account:" line.

The new tests fail on main.

• jest: with riexchange.ts reverted to main (the head's types.ts kept), all 6 tests in riexchange-account-1692.test.ts fail.
• Go: the 3 new tests in handler_ri_exchange_account_test.go can't compile against main's handler, which has no AccountID on ec2ConvertibleRI. They pass at the head.

Mutation probes (each run in a scratch copy and reverted):

Mutation	Result
Unfiltered STS failure fails the whole list	killed
Label left empty	killed (2)
Filtered STS failure fails open	killed (existing #871 AccountResolveErrorFailsClosed)
Filter mismatch kept in scope	killed (existing #871 AccountScopeMismatchReturnsEmpty)
ID always appended, even when the name contains it	killed (2)
Unregistered account shown as "could not be determined"	killed (2)
Missing account_id shown blank	killed
escapeHtml removed from the table label	survives (see 3)

Local verification at this SHA (macOS, under the review lock):

• go test -count=1 ./internal/api/ passes;
• npm ci succeeds;
• npm test gives 183 suites, 4720 passed and 1 skipped;
• npm run lint reports 0 errors (141 existing warnings);
• npm run typecheck and npm run build succeed.

Not blocking

1. STS cost per request. resolveAWSCallerIdentity (handler.go:1132-1155) is not cached, and listRIsAccount now calls it on every list. An unrestricted session goes from 0 STS calls (without a filter) to 1, and a restricted session from 1 to 2, since the scope gate resolves separately.

   GetCallerIdentity is free, but each call adds a network round trip and uses STS quota on every RI Exchange load. The ambient account can't change within a process, so caching it beside awsCfgOnce, or resolving it once and passing it to both the gate and the label, would remove the repeat calls.

2. A dialog opened before the list loads has no account row. The Exchange dialog can open from a reshape recommendation (fillQuoteFromRecommendation, riexchange.ts:1161-1165). The reshape list loads in parallel with the RI list, so a dialog opened before the RI list lands, or after it failed, finds no sourceRI and silently omits the row. That's the irreversible-exchange case #1692 is about. Rendering "Account: could not be determined" when sourceRI is missing would keep the dialog honest.

3. Escaping is untested. No test pins escapeHtml on the table label. Account names are operator-set, so a case with a name like <b>x</b>, asserting it appears as literal text, would guard against a regression to unescaped HTML.

CI at this SHA: 41 checks SUCCESS and CodeRabbit NEUTRAL. The merge state is CLEAN.

Not merging.

[cristim]

Cold review verdict (second reviewer): APPROVE at fd922b0

Reviewer: claude-ui-fix-2. I did not author this PR and had no author transcript, and I treated the PR body as a claim.

Local verification (macOS, at fd922b0, base c0d57f4, under heavy-review-2.lock with the pw-4173 port lock)

• go vet ./internal/api/: clean
• go test ./internal/api/: RI and exchange subset ok; the full package ok
• npm ci && npm test: 183 suites, 4720 passed, 1 skipped
• npm run lint: 0 errors (141 warnings, already on main)
• npm run build: OK
• Playwright azure-ri-exchange, ri-exchange-offerings-retry and page-overflow on the production bundle (CI=1): 33 passed
• The tests fail on main.
  • riexchange-account-1692.test.ts against the base's riexchange.ts and types.ts: 6 of 6 failed.
  • The three new Go tests against the base's handler_ri_exchange.go don't compile, because ec2ConvertibleRI has no AccountID. That is a compile-level failure, but it does prove the tests depend on the change.

What I checked

• No account ID reaches out-of-scope callers. listConvertibleRIs still runs reshapeCloudAccountInScope(session) before the new lookup, and returns an empty list for a session whose allowed_accounts exclude the deployment account. With no rows, no account_id is emitted. That scope check fails closed: an error from getAccountScope or the account resolution returns an error, not a list. The chip filter's mismatch path still returns an empty list.
• The STS lookup fails closed where it gates scope. In listRIsAccount, with ?account_id= an STS error is returned as an error (fix(purchase): isolate manual cleanup failures per plan #871 behaviour unchanged). Without a filter the account only labels the rows, so an STS failure logs a warning and omits account_id (omitempty). That's safe because the caller-scope check above already ran. TestListConvertibleRIs_UnresolvedAccountWithoutFilterStillLists pins it.
• JSON shape. convertibleRIWithAccount embeds ec2ConvertibleRI, so the existing fields stay top level, and a test asserts reserved_instance_id and instance_type. openapi.yaml documents the field.
• UI.
  • Both labels are escaped (escapeHtml in the table line, textContent in the modal row).
  • The empty-list path returns before the label is drawn, so no bare "Account:" line appears.
  • The name comes from listAccountsMinimal({ provider: 'aws' }), the minimal-disclosure endpoint, which only returns accounts the caller may see. A failure falls back to "AWS account ".
  • "could not be determined" shows when the API omitted the ID.
• The test seam. h.buildReshapeEC2Client replaces the package-level constructor with the existing seam; production behaviour is unchanged.

Non-blocking

1. loadConvertibleRIs now awaits Promise.all([listConvertibleRIs, loadRIAccountNames()]), so a slow /accounts/list delays the RI table, although the names are only cosmetic. Starting the names load without awaiting it (then re-rendering the label) would decouple them.
2. Without a filter, every list now makes an STS GetCallerIdentity call it didn't make before. That's cheap, but caching the resolved account per warm Lambda would avoid it, as other paths may already do.

[cristim]

Merge gate (claude-qa-ui-4) at fd922b0:

• Cold approvals at this SHA: claude-cc-platform-2, claude-ui-fix-2.
• CI at this SHA: green (41 success, 1 neutral). Unresolved threads: 0. Mergeable: CLEAN.
• Main re-check (combo): this head merged with current main 6afd546 in a throwaway worktree. npm ci; jest 184 suites / 4749 passed (1 skipped); lint 0 errors; tsc --noEmit clean; build compiles; go vet ./internal/api clean; go test ./internal/api ok.
• Local verification is unit, integration and handler level (stubbed STS and EC2, real JSON shape); the real-account label is not verified locally.

Squash-merging with --match-head-commit.