fix(cmd): fail the run when an account name cannot be resolved for an account filter

[cristim]

Closes #2198

What

GetAccountAlias replaced a name with the account ID (and cached it) when organizations:DescribeAccount failed. The account filters match on names, so a transient lookup error made an excluded account pass the filter, and a --purchase run could buy for it.

• New ResolveAccountName returns the lookup error and does not cache it. GetAccountAlias keeps its display-only behaviour (ID on error) on top of it.
• populateRecommendationAccountNames now takes the config: with --include-accounts or --exclude-accounts set, an unresolved name fails the run (Cannot complete the recommendation fetch: cannot apply --include-accounts/--exclude-accounts: describing account ...). With no account filter the ID stands in for the name as before.
• The CSV path (populateAccountNames) is unchanged: it runs after filtering and is display-only.

Tests

• TestFetchAllRecs_AccountFilterWithUnresolvableNameFailsTheRun (exclude and include variants, plus the no-filter case). With the filtering branch disabled it fails with "An error is expected but got nil."; with the fix it passes.
• TestResolveAccountName_LookupErrorIsReturnedAndNotCached; the old TestGetAccountAlias case that expected the fallback to be cached now expects it not to be.
• go vet ./cmd and go test ./cmd pass (31s; TestQueryMajorEngineVersions_ErrorHandling skipped locally, it hangs in this session independent of this change).

Flaky test

TestReservationExpiryCommand/filtered-subset failed once in CI on cli#2197 with an extra row for an account the filter should have dropped, which fits a failed name lookup (the child runs with AWS_MAX_ATTEMPTS=1). I could not confirm the lookup failure in the log. With this change such a failure is a loud error rather than a silent extra row, but I did not find what makes the fixture's lookup fail intermittently.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 7 billable files and costs up to $1.75.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 34 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 62 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: ec7aa186-4f6c-4eff-b32c-e41b8d32ae07

📥 Commits

Reviewing files that changed from the base of the PR and between 9806c9d and a8898b0.

📒 Files selected for processing: 7

• cmd/cancellation_prepurchase_test.go
• cmd/helpers.go
• cmd/helpers_test.go
• cmd/multi_service.go
• cmd/multi_service_filters.go
• cmd/multi_service_helpers.go
• docs/cli/filtering.md

• 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 (claude-opus-5-5) of bf3d2c6: CHANGES REQUESTED

The fix is correct for the case it targets: a transient lookup failure with an account filter set now fails the run instead of failing open. Two findings block it.

Blocking

1. Failures are never cached, so a persistent failure costs one DescribeAccount call per recommendation (cmd/helpers.go ResolveAccountName; cmd/multi_service_helpers.go populateRecommendationAccountNames).

   I measured this on this head with a scratch test: 100 recommendations of one account, DescribeAccount always failing, no filter set. That produced 100 calls; the old code made 1.

   DescribeAccount works only from the management account or a delegated administrator, per the AWS Organizations API reference. From any other member account it always returns AccessDenied, and in a standalone account it returns AWSOrganizationsNotInUse. Member-account credentials are the normal purchase setup, since cross-account purchases are refused. So in that setup every recommendation now makes a failing Organizations call, serialized under the cache's write lock.

   Suggested fix: remember the failure per account for the run, keeping the error rather than the ID. ResolveAccountName keeps returning the error, so the filter path still fails loud, and GetAccountAlias shows the ID without calling again. The SDK retryer already retries throttling within a call, so retrying across recommendations adds little.

2. Member-account and standalone runs with an account filter now always fail, and the docs still promise the opposite.

   • docs/cli/filtering.md:25 says: "If the alias cannot be resolved, the account ID is used as a fallback."
   • Until now that fallback made ID-based filters work whenever the lookup failed, e.g. --exclude-accounts <account-id>. accountMatchesFilter does a substring match on the name, and the name was the ID.
   • With this PR, the same run aborts. That includes a shared config file carrying an account filter.

   Suggested fix: match the filter against the account ID as well as the name.

   • A filter entry that matches the ID decides the row without a lookup.
   • Fail only when the name is unresolved and no entry matched the ID.
   • Update line 25 either way.

Non-blocking

3. The CSV replay path does apply account filters, on unresolved names.
   • filterAndAdjustRecommendations (cmd/multi_service.go:922) runs applyFilters on the CSV's own AccountName column (cmd/multi_service_csv.go:115), before populateAccountNames (cmd/multi_service.go:771).
   • The claim "display-only, after filtering" is true of populateAccountNames, but not of the filtering itself.
   • A CSV written by a run whose lookup failed carries the ID in AccountName. This PR still writes the ID in the no-filter case. Replaying it with --exclude-accounts <name> fails open, which is the same bug class as fix(cmd): a failed account-name lookup lets --exclude-accounts recommendations through #2198.
   • Fixing it in this PR or in a follow-up are both fine. Re-resolving the name, or matching on the ID per finding 2, would close it.

Coverage of filter sites

passesDimensionFilters (cmd/multi_service_filters.go:105) is reached from three places:

• the main fetch path (fetchAndFilterRegionRecs, now resolved or failing);
• the legacy test-only processRegionRecommendations (now resolved or failing);
• the CSV path (finding 3).

The purchase paths consume these filtered results. No other call to shouldIncludeAccount exists.

Evidence

• go vet ./cmd is clean, and the new and changed tests pass locally.
• Fail-before: disabling the if filtering return makes TestFetchAllRecs_AccountFilterWithUnresolvableNameFailsTheRun fail with "An error is expected but got nil".
• CI is green on this SHA.

The filtered-subset flake behind #2198

The account lookup is the proximate cause; the disk-full warnings are not.

• The CSV writer emits rec.AccountName verbatim (cmd/multi_service_csv.go:268).
• On the live path, AccountName is set only by GetAccountAlias.
• The failing row's AccountName was the raw ID, while the fixture always answers with expiry-account-3. So GetAccountAlias took its fallback.
• The proxy counted all 3 DescribeAccount requests, and recorded no handler or write error. The failure was therefore client-side after the request was sent; the error text was swallowed.

The "no space left on device" lines are not a real disk-full. They come from TestAuditWriteFailureOnDryRunDoesNotStop, whose fake writer returns errors.New("no space left on device") (cmd/audit_stop_2103_test.go:38). They ran about 95 s earlier in the parent test process, not in the filtered-subset child. A dry-run audit write failure only logs and does not touch names or filtering.

Local reproduction failed in 720 runs, with DescribeAccount errors instrumented in 420 of them: 0 failures and 0 lookup errors.

• 50x -race on chore(deps): bump cloud-commitments-go; scale money on extended-support cuts #2197's head and 50x on main (300 subtests each);
• 60x filtered-subset with -race;
• 6 x 60 parallel runs at GOMAXPROCS=1.

With this PR, a recurrence fails loudly with the underlying error, which will name the transport cause.

[cristim]

Independent review (claude-opus-5-5) of 635fed1: APPROVE

This re-review covers the delta since bf3d2c6: commits 01572df, 83e4c9f and 635fed1. Both earlier blockers are resolved, and nothing new blocks.

Checks against the blockers

I wrote scratch tests that are not committed and ran them on this head.

1. One DescribeAccount call per account per run, even on failure. The cache now remembers failures (failed map, cmd/helpers.go). Test setup: 200 recommendations over 2 accounts, every call returning AccessDenied, two populateRecommendationAccountNames passes, then a GetAccountAlias per row. Result: 2 calls, down from 100 on bf3d2c6.
2. ID-based filter under AccessDenied. --exclude-accounts <id> and --include-accounts <id> decide the rows with no error, matching the account ID exactly (shouldIncludeAccountIDOrName).
3. Name-based filter with an unresolved name errors. This holds for --exclude-accounts sandbox, --include-accounts prod, and a mixed ID-plus-name list. Each returns "cannot apply the account filter ...: the name of account ... is unknown". Fail-before: bf3d2c6's tests already pinned this, and disabling the check fails them.
4. CSV replay uses the same matching. filterAndAdjustRecommendations now runs requireFilterInputs, which calls requireResolvedAccountNames before applyFilters.
   • A name filter errors on rows whose AccountName equals the ID, and on rows whose AccountName is empty.
   • An ID filter excludes the right rows.
   • A resolved name excludes by substring.

Live dry run (head binary, payer credentials, no purchase flags)

EC2 RI, 1yr no-upfront, 30d: 24 rows, 17 of them for the largest account.

• --exclude-accounts <its ID>: 7 rows, 0 from that account.
• --exclude-accounts <its name>: the same 7 rows, 0 from that account.

CSV replay with --input-csv:

• Resolved-name CSV: the name filter and the ID filter each give 7 rows, 0 excluded rows.
• The same CSV with AccountName set to the ID:
  • the name filter exits 1 with the "cannot apply the account filter" error;
  • the ID filter gives 7 rows, 0 excluded rows.

Non-blocking

• requireResolvedAccountNames treats any row whose name equals its ID as unresolved, which includes an account whose Organizations name is genuinely missing. It also errors on a mixed ID-plus-name list even when the ID entry alone would decide the row. Both errors are conservative and the message tells the user to filter by ID, so this is acceptable.
• docs/cli/filtering.md now describes the ID/name matching, the one-call-per-account rule and the CSV rule. All three match the code.

State

• go vet ./cmd is clean, and 187 account/CSV/fetch tests pass locally.
• CI is fully green on 635fed1 and mergeability is CLEAN.
• CodeRabbit is rate-limited on this head, with no review content, so per the review gate: CR waived: quota, adversarial review + local verification + green CI, with a retrospective CR pass to be tracked.

[cristim]

Independent review (claude-opus-5-5) of 635fed1: CHANGES REQUESTED

This supersedes my APPROVE above for the same SHA; please do not merge on that line. The functional review there still stands. One test-hygiene point needs a change first.

cmd/cancellation_prepurchase_test.go:21-22 builds the synthetic IDs as split string literals ("4444" + "44444444", "5555" + "55555555") so the git-secrets 12-digit detector does not see them. This repo has a convention for that detector:

• synthetic IDs get path-anchored, whole-line .gitallowed entries (see the existing cmd/helpers_test.go / cmd/savings_plan_purchase_test.go entries);
• the split-literal trick appears nowhere else in cmd/.

Splitting a literal to slip past a secret scanner is the pattern the scanner exists to catch, and copying it normalizes it.

Fix (no .gitallowed change needed): cmd/purchase_account_guard_test.go:17-18, in the same package main and with no build tag, already defines testCallerAccount = "444444444444" and otherAccount = "555555555555". Those are the same two values, and those lines already pass the scan.

1. Delete the acctSandbox / acctOther const block.
2. Use testCallerAccount and otherAccount at the 20 references.

No new 12-digit literal is introduced, and nothing is reused that is not already allowed.

Everything else from my previous comment stands:

• one call per account per run;
• ID filters work under AccessDenied;
• name filters error when the name is unresolved;
• CSV replay uses the same matching;
• the live checks.

A head with only this change gets a quick re-check from me.

[cristim]

Independent review (claude-opus-5-5) of a8898b0: APPROVE

This supersedes my CHANGES REQUESTED for 635fed1.

The delta from 635fed1 is one commit (a8898b0) touching only cmd/cancellation_prepurchase_test.go, +18/-24:

• the split-literal const block (acctSandbox / acctOther) is deleted;
• every reference now uses the existing testCallerAccount / otherAccount from cmd/purchase_account_guard_test.go.

No other line changed, and no split numeric literal remains in the file.

Local checks on this head:

• go vet ./cmd is clean.
• 187 account, CSV and fetch tests pass.

CI is fully green on a8898b0, and mergeability is CLEAN.

The functional review and live evidence in my 635fed1 comments carry over unchanged:

• one DescribeAccount call per account per run;
• ID filters work under AccessDenied;
• name filters error when the name is unresolved;
• CSV replay uses the same matching.