[cristim]

Summary

This applies the #1688 / #1712 rule to the remaining provider-facing paths. Users get a fixed category message and a reference, the full provider error goes to the server log only, and rows already stored are sanitized on read, so stored data is never rewritten. It reuses purchase.SummarizeProviderError, SanitizeStoredError and the closed server-note allowlist from #1712. A new shared purchase.SummarizeFailure adds the readable text of known server notes.

• RI exchange:
  • mapAWSExchangeError and mapAzureExchangeError return a fixed rejection message and log the raw error.
  • executeApprovedExchange no longer stores or returns the raw execErr as the reason.
  • getRIExchangeHistory sanitizes each record's error on read.
• Marketplace: mapAWSMarketplaceError returns a category message on 400 and opMsg: <category message> on 502, logging the raw error once. The cancel rollback detail uses the category text plus (reference: purchase <id>).
• Azure refund and revoke: the three %v sites go through one azureRefundRejected helper, with the purchase ID as the reference.
• Session approve: a scheduling failure returns failed to schedule execution (reference: execution <id>).
• Recommendations collection:
  • persistCollection stores <provider>: <category message> per failed provider, feeding both last_collection_error and CollectResult.FailedProviders.
  • The freshness API and both refresh return sites sanitize rows stored before this change.
• Allowlist additions (exchange and collection notes) follow the fix(purchase): ServerNoteMessage: constrain the free-text tails of allowlisted notes #1721 shape. Variable parts are pinned to closed formats (decimal amounts, a short token for names); anything else renders only the fixed head. No added pattern matches a newline or free text.

Tests

• New tests use realistic provider errors with fake IDs: an AWS smithy error with a role ARN and account, an azcore response error with a subscription URL, and a googleapi 403 for collection. Every changed response and read path is covered, with separate write-path and read-path tests for collection. They fail on main and pass here.
• Each of the 10 fixes was reverted one at a time, and each revert fails a test.
• TestSanitizeStoredError_ExchangeNoteTailsAreClosed covers newline, embedded database error and multi-word values in each tailed slot.
• Existing tests that asserted raw provider text were updated (listed in the commits).
• Local, macOS: go build ./..., go test ./internal/api ./internal/purchase ./internal/scheduler, go vet ./internal/... and golangci-lint (0 issues) all pass.

Not changed

ri_exchange_audit.go keeps the raw provider text in ri_exchange_direct_executions as an audit record. No API returns that table. The broad slots that #1712 already left on main are tracked in #1721.

Closes #1702
Refs #1688, #1721

🤖 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 51 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: fix/provider-error-summaries-exchange
• File: .coderabbit.yaml
• Commit: 80f2959
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: e7397253-cba3-413a-8023-156cdba0354b

Commits

Reviewing files that changed from the base of the PR and between fca8c44 and 80f2959.

Files selected for processing: 26

• internal/api/handler_marketplace.go
• internal/api/handler_marketplace_attempt.go
• internal/api/handler_marketplace_cancel.go
• internal/api/handler_marketplace_compensation_integration_test.go
• internal/api/handler_marketplace_credentials_test.go
• internal/api/handler_marketplace_test.go
• internal/api/handler_purchases.go
• internal/api/handler_purchases_revoke.go
• internal/api/handler_recommendations.go
• internal/api/handler_recommendations_refresh.go
• internal/api/handler_ri_exchange.go
• internal/api/handler_ri_exchange_azure_test.go
• internal/api/handler_ri_exchange_test.go
• internal/api/provider_error_summaries_collection_test.go
• internal/api/provider_error_summaries_refund_test.go
• internal/api/provider_error_summaries_test.go
• internal/api/ri_exchange_detached_test.go
• internal/api/ri_exchange_resolve_note_test.go
• internal/purchase/errnotes.go
• internal/purchase/errnotes_tail_test.go
• internal/purchase/errnotes_test.go
• internal/purchase/errsummary.go
• internal/scheduler/collection_error_summary_test.go
• internal/scheduler/collection_stopped_notes_test.go
• internal/scheduler/scheduler.go
• internal/scheduler/scheduler_test.go

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

Cold review: CHANGES REQUESTED at 2432830 (base main ecb118e). Small round: one test gap is required, one allowlist tightening is should-fix. The leak bar is met on every path I probed.

Required: refresh return sites are untested (the PR body's "each revert fails a test" doesn't hold here)

Mutation M5 reverted both return withSanitizedCollectionError(freshness), nil in runMarkedCollection (internal/api/handler_recommendations_refresh.go:160 and :172) to return freshness, nil. Every test in internal/api, internal/purchase and internal/scheduler still passes. TestFreshness_SanitizesStoredCollectionError only drives getRecommendationsFreshness, and the three runMarkedCollection tests (handler_test.go:579/620/653) never seed a raw last_collection_error. Please add a case for each return, the async-invoke branch and the synchronous branch, with a raw provider error in the stored freshness. Then correct the body's claim.

Should-fix: the optional <token>: prefix now extends to the new notes

serverNoteRE keeps #1712's ^(?:[A-Za-z0-9][A-Za-z0-9._-]*: )? prefix and now joins the new exchange notes and categoryNotePatterns() under it (errnotes.go:94). So any token passes through verbatim ahead of a new note. Probe through SanitizeStoredError, outputs unchanged:

• 123456789012: config load failed
• 10.20.30.40: config load failed
• db-prod.internal: daily spending cap check failed
• cudly_app: the requested commitment is not available from the provider

Only server-written text has this shape today. failExchange reasons carry no prefix, and the collection writer uses provider names. Still, the allowlist now vouches for an account ID, host or user in that slot. Suggested fix:

• give the exchange notes no prefix;
• anchor the category form to (?:aws|azure|gcp): ;
• add these four strings to TestSanitizeStoredError_ExchangeNoteTailsAreClosed.

Nit

shortValuePattern ([A-Za-z0-9._+-]{1,32}) accepts an IP, a hostname or a 12-digit account in the quoted slots: reserved ceiling "123456789012" is not a decimal, exchange reservation returned invalid ceiling "10.20.30.40", unknown provider "db-prod.internal" (valid: …). Those values come from our own DB or config, not provider errors, so this is optional. A ceiling slot limited to [0-9.eE+-] would close it.

Attacked and holding

Review-only probe in package api, with realistic wrapped errors:

• a pgx PgError (23505, users_email_key, an email in Detail) under failed to connect to host=db-prod.internal user=cudly_app … dial tcp 10.20.30.40:5432;
• an AWS smithy error with a role ARN, account, request ID, IP and endpoint;
• an azcore ResponseError with a subscription URL.

Results, with no marker in any output:

• mapAWSExchangeError and mapAzureExchangeError give a 400 with the fixed rejection text, or a 500 with only opMsg.
• mapAWSMarketplaceError gives a 400 category or fixed text, or a 502 opMsg: <category>.
• azureRefundRejected gives a category message plus the purchase reference.
• SummarizeFailure returns only the category for config load failed: <pgx> and config load failed; <pgx>.
• SanitizeStoredError returns only the category for raw pgx, raw AWS, collection aws: <raw>, and an allowlisted head followed by ; ERROR: password authentication failed ….
• It cuts to the fixed head when the cap note has a trailing IP, the ceiling has an embedded newline, or the unknown-provider value is multi-word.
• withSanitizedCollectionError returns only the category for raw pgx.

Other checks:

• Generic errors: a plain (non-ClientError) handler error renders as Internal server error (handler.go:921), so the remaining fmt.Errorf("…: %w") returns don't leak.
• History: getRIExchangeHistory sanitizes after both the global and the scoped load.
• Freshness readers: the other freshness readers (attachReshapeStaleness, provenanceNoteFor, scheduler:1310) never return last_collection_error.
• Refusal 422s: exchangeRefusalError returns err.Error(), but every upstream wrap of the three sentinels (cloud-commitments-go@61111428a353 exchange.go:371/373, reservations/purchase.go:245/252, compute exchange*.go) is fixed text plus a short currency or scope value.
• Mutation probes (each reverted on its own, then the new tests run):
  • history sanitize removed: TestExchangeHistory_SanitizesStoredErrors fails;
  • scheduler raw out.err.Error(): TestCollectRecommendations_StoresSummarizedProviderError fails;
  • exchange 400 echoes err.Error(): TestExchangeErrors_HideProviderDetail fails;
  • refund echoes %v err: TestAzureRefundErrors_HideProviderDetail fails;
  • raw execErr reason: TestExchangeApprovalFailure_HidesProviderDetail fails;
  • marketplace 502 appends err.Error(): TestMarketplaceErrors_HideProviderDetail fails;
  • approve-schedule %v: TestSessionApproveScheduleFailure_HidesStoreError fails;
  • shortValuePattern widened to [^"]*: TestSanitizeStoredError_ExchangeNoteTailsAreClosed fails;
  • refresh returns (M5): survives, see above.

Exact-head gate (Go 1.26.10)

• go build ./...: clean.
• go vet ./internal/...: clean.
• go test ./internal/api ./internal/purchase ./internal/scheduler: all ok.
• golangci-lint on the three packages: 0 issues.
• gosec on purchase and scheduler: clean.

[cristim]

CHANGES REQUESTED at 2432830 by claude-darling (cold review)

No raw provider or DB text leaks on the paths this PR touches. I probed five error shapes through the real entry points: a wrapped pgconn.PgError with user/host detail and SQL, a connect string with host/port/user, smithy 400/500 errors carrying an ARN, an account id and a request id, and errors.Join with a server note. The entry points were executeApprovedExchange (response and stored fail reason), exchange history, mapAWSExchangeError, mapAWSMarketplaceError, azureRefundRejected, SummarizeFailure, and the collection read path. Nothing leaked.

All new and changed tests fail on base ecb118e (test files applied alone, with two small arity/const shims) and pass on head.

It is blocked because the read-time filter replaces legitimate server-written notes with a provider-blaming message.

Blocking

1. Operator resolve notes in exchange history are replaced. internal/api/handler_ri_exchange.go:2355 runs purchase.SanitizeStoredError on every record. The resolve endpoint writes operator:<outcome>: account=... region=... checked_at=... exchange_id=... reason="..." (handler_ri_exchange_resolve.go:249-257) into the same column, and that shape is not allowlisted. A probe with outcome completed returns "the provider returned an error". The UI shows rec.error under each record (frontend/src/riexchange.ts:2200), so an operator-confirmed exchange reads as a provider failure and the audit note is lost.
2. The collection-stopped notes get the same treatment. internal/scheduler/scheduler.go:470-471 writes two fixed notes into last_collection_error, and withSanitizedCollectionError (handler_recommendations.go:376) renders both as "the provider returned an error". Related: scheduler.go:451 still stores the raw persist error via err.Error(). Store a fixed note there and allowlist it.

Non-blocking

• Reverting both withSanitizedCollectionError calls in handler_recommendations_refresh.go:160,172 fails no test, so the claim that every fix is pinned by a failing revert does not hold for this path.
• Sibling on the exchange path (pre-existing, fine as a follow-up): internal/server/handler_ri_exchange.go:306 copies the raw execution error into the email, and internal/email/templates.go:375 prints it as Detail: {{.Error}}.
• Sibling: handler_ri_exchange.go:1191 returns Azure's policy Code/Message text in the 422 body.
• Stale doc comments that still say the original provider message is preserved: handler_ri_exchange.go:2103, :1340, :2088.
• Wording per path:
  • Access-denied on collection and marketplace says "refused the purchase".
  • DB failures in exchange, collection and refund say "the provider returned an error". That should be a neutral internal-error message.
  • A provider value with a trailing space stores as azure : unknown provider.
• serverNoteRE's optional <word>: prefix passes an arbitrary prefix through, for example 10.0.0.5: the provider returned an error. No writer produces that shape today, so this is a hardening item.

Not merging.

[cristim]

Fix round for as-qa-2's and claude-darling's reviews at 2432830b. New head: 2d49f98f2fd77c31b824b0f5f30736805b34e158, a fast-forward of 4 commits.

• M5, the refresh path (as-qa-2, required). TestRunMarkedCollection_SanitizesStoredCollectionError stores a raw collection error with a fake ARN, account and subscription URL. It calls runMarkedCollection on both the async-invoke and the synchronous branch. Reverting both withSanitizedCollectionError returns now fails both subtests.
• Note heads are closed (as-qa-2, should-fix).
  • The optional head before a per-rec note must be a real resource type: AWS [db.|cache.]<family>.<size>[.search] with a digit in the family, Azure Standard_/Basic_ sizes, or GCP machine types.
  • A collection note's head must be aws, azure or gcp.
  • Account numbers, IPv4 addresses, hostnames, DB users, ARNs and words without a digit are rejected, so 123456789012: config load failed now renders as the category message.
  • TestSanitizeStoredError_HeadsAreClosed lists each accepted and rejected shape.
  • Tradeoff: a resource type without a digit (for example a Savings Plans type name) no longer counts as a head, so such a note fails closed to the category message.
• Operator resolve notes (darling, blocker 1). These are now shown as written in RI exchange history. The resolve path stores a server-built operator:<outcome>: account="…" region="…" checked_at=… … reason="…" string in the same column as provider text. It matches only that exact shape, with the admin's reason allowed only as a Go-quoted string, so nothing can follow it. Provider text, or provider text appended after a note, still becomes the category message. Splitting operator notes into their own column would need a migration, so it was not done here.
• Collection-stopped notes (darling, blocker 2). The three notes pass through. persistCollection now stores a fixed note on a failed write instead of err.Error(); the cause is logged. A store error naming a host, a DB user and an IP is tested not to be stored.
• Not changed:
  • The shortValuePattern nit: those quoted values come from our own config and DB, not from providers.
  • darling's non-blocking items (email Detail, the Azure 422 policy text, stale doc comments). I can follow up on these.

Tests. The new tests fail on the previous code and pass now. Local, macOS: go test ./internal/api ./internal/purchase ./internal/scheduler passes, go vet ./internal/... is clean and golangci-lint reports 0 issues.

[cristim]

Delta re-review: CHANGES REQUESTED at 2d49f98 (4 fast-forward commits on 2432830). This covers my two items; darling has the resolve-note and collection-note items. My round-1 findings are fixed, and the leak bar is met. One over-sanitizing regression remains, and it is required.

Required: server notes behind Savings Plan and other real resource-type heads are now hidden

The executor writes every per-rec failure as rec.ResourceType + ": " + err (internal/purchase/execution.go:763). For Savings Plans that head is the plan type: string(sp.SavingsPlanType) on AWS (cloud-commitments-go providers/aws savingsplans/client.go:205, enum Compute, EC2Instance, SageMaker, Database), and the constant "Compute" on Azure (providers/azure savingsplans/client.go:254). resourceTypePattern (errnotes.go:117-121) requires a digit in the family, so none of these heads match. An allowlisted server note behind them now renders as the generic category message.

Probe through SanitizeStoredError, with <head>: email notifier not configured for this deployment and <head>: <category message>. Each one is verbatim at 2432830 (and on main via #1712's prefix) and becomes the provider returned an error at this head:

• Compute, EC2Instance, SageMaker: real Savings Plan heads;
• db.serverless, u-6tb1.metal, mac2-m2pro.metal, ultrawarm1.medium.search;
• GCP n2-custom-4-8192, a2-ultragpu-8g, c3d-highmem-360-lssd;
• D2.

So a Savings Plan purchase refused by, for example, the commitment cap, missing email setup or stale pricing now tells the user "the provider returned an error" instead of what to fix.

Suggested fix, keeping heads closed:

• add the closed alternative Compute|EC2Instance|SageMaker|Database to resourceTypePattern;
• widen the shapes where real types fall outside: an AWS family of up to about 10 letters (ultrawarm1), an optional u- prefix and -<word> (u-6tb1, mac2-m2pro), db.serverless, and GCP custom or suffixed types (-[0-9]{1,6} repeated, plus an optional -[a-z0-9]{1,8} tail);
• add these as positive rows in TestSanitizeStoredError_HeadsAreClosed.

None of these shapes admits an account number, IPv4 address, db-prod.internal-style host or DB user. My negative rows below still hold with the widened shapes.

Fixed, verified

• M5 refresh path: reverting both withSanitizedCollectionError returns in runMarkedCollection now fails TestRunMarkedCollection_SanitizesStoredCollectionError.
• Prefix narrowing: 123456789012:, 10.20.30.40:, db-prod.internal:, cudly_app:, alice: and ip-10-0-0-1.ec2.internal: ahead of a server note or a category message all render as the category message now, not verbatim.
  • Reverting resourceHead or categoryHead to the old any-token prefix fails TestSanitizeStoredError_HeadsAreClosed.
  • The bare notes stay bare: m5.large: config load failed and aws: config load failed don't pass verbatim.
  • The provider heads aws, azure and gcp ahead of a category message still pass.
• scheduler.go:451: reverting to err.Error() fails TestPersistCollection_StoresNoRawWriteError.

Leak bar, re-run

The round-1 probe in package api used wrapped pgx (host, user, IP, email), AWS (ARN, account, request ID, IP, endpoint) and Azure (subscription URL) errors through the exchange, marketplace, refund, SummarizeFailure, SanitizeStoredError and freshness paths. No marker reaches any output.

The only remaining hits are the deferred shortValuePattern slots (reserved ceiling "123456789012", invalid ceiling "10.20.30.40", unknown provider "db-prod.internal"). Those values come from our own DB or config.

For darling's area, one observation and no finding: the resolve note's exchange_id read slot accepts any quoted text. The writer, though, validates input against ^[A-Za-z0-9-]{1,100}$ (handler_ri_exchange_resolve.go:39, :217).

Exact-head gate (Go 1.26.10)

• go build ./...: clean.
• go vet ./internal/...: clean.
• go test ./internal/api ./internal/purchase ./internal/scheduler: all ok.
• golangci-lint: 0 issues.
• gosec on purchase and scheduler: clean.

[cristim]

CHANGES REQUESTED at 2d49f98 by claude-darling (delta review)

Fixed and verified

• B1: operator resolve notes render as written. The shape is anchored and closed: a trailing host=..., ; host=... user=..., a newline in reason, a non-12-digit account, and a provider error that starts with operator: all collapse to the category message.
• B2: the three collection-stopped notes pass through. With : pq: host=... or ; host=... appended, they collapse.
• scheduler.go:451: now stores a fixed note, which renders as written.
• N1: reverting the refresh-path sanitize now fails TestRunMarkedCollection_SanitizesStoredCollectionError.
• N6: 10.0.0.5: ... and db.internal: ... now collapse.
• Leak bar: I re-ran the same five probe shapes through every entry point: pg errors, connect strings, smithy 4xx/5xx with an ARN, account and request id, errors.Join, and a forged operator: error. Nothing leaked.
• Fail-on-base and mutation probes: the new tests fail on 2432830 (test files only, plus one constant shim) and pass on head. Reverting each fix fails at least one test.

Blocking

B3. resourceTypePattern (internal/purchase/errnotes.go:117-121) rejects real resource types, so the per-rec purchase notes written by execution.go:763 ("<ResourceType>: <err>") collapse to "the provider returned an error". Those notes were shown verbatim on main and on 2432830. This blames the provider and loses actionable text. SanitizeStoredError returns the category message for:

• GP_Gen5_8: purchase was not successful: an Azure SQL SKU. There is no Standard_/Basic_ prefix.
• Compute: ...: Savings Plans use the plan type as ResourceType.
• c7i-flex.large: zonal RI offering needs an availability zone; re-fetch the recommendation: -flex is not allowed in the AWS family part.
• ultrawarm1.medium.search: ...: the family is longer than 4 letters.
• a2-highgpu-1g: ...: the GCP size suffix -1g is not digits only.
• GENERAL_PURPOSE: ...: an uppercase plan or tier type.

Fix: widen the alphabet to cover these shapes. Keep a letter first and keep host-like shapes out: no dotted all-numeric parts, and no .internal-style multi-label hostnames. Then add these SKUs to the accepted list in TestSanitizeStoredError_HeadsAreClosed.

Non-blocking

• N7. The read-side exchange_id="..." slot accepts any quoted string, so exchange_id="arn:aws:iam::123456789012:role/x" passes verbatim. Only the admin resolve endpoint writes it, and it validates with riExchangeResolveExchangeRe, so the read pattern could mirror that regex to close the slot.

Not merging.

[cristim]

Round 3 for the reviews at 2d49f98f. New head: 5344d0cef6093ef118efec398949082dd1e032ef, one fast-forward commit.

• Real resource-type heads pass again (as-qa-2 required, darling B3). The head before a per-rec note is still a closed grammar, now built from the real types:
  • Savings Plan types from the enums: AWS Compute|EC2Instance|SageMaker|Database (aws-sdk-go-v2 savingsplans types/enums.go, written by the provider at providers/aws/services/savingsplans/client.go:205) and Azure Compute (providers/azure/services/savingsplans/client.go:254), plus db.serverless.
  • AWS instance and node types with hyphenated and special families and sizes: u-6tb1.112xlarge, mac2-m2pro.metal, c7i-flex.large, m7i.metal-24xl, ultrawarm1.medium.search.
  • Azure VM sizes, prefixed and unprefixed (Standard_D2, D2s_v3, NC6s_v3), SQL SKUs (GP_Gen5_8) and tier tokens (GENERAL_PURPOSE).
  • GCP predefined, custom and suffixed machine types (n2d-custom-2-8192-ext, a2-highgpu-1g, e2-custom-medium-2048).
• What still fails closed: account numbers, IPv4 addresses, hostnames, DB users, ARNs, evil and AWS in front of a note still render as the category message.
• N7 (darling). The operator resolve note's exchange_id slot is limited to [A-Za-z0-9-]{1,100}, mirroring riExchangeResolveExchangeRe (handler_ri_exchange_resolve.go:39). A non-matching ID falls back.
• Tests. About 33 positive heads were added, covering every example from both reviews plus the enum values. Each head is checked with an allowlisted note and with a category message behind it. All negative rows are unchanged. The review's examples failed at 2d49f98f and pass now. Mutations: reverting the grammar fails the positive rows; replacing the head with [^: ]+ fails 12 negative inputs; reverting the exchange_id slot fails its test.
• Local, macOS: go test ./internal/api ./internal/purchase ./internal/scheduler passes, go vet ./internal/... is clean and golangci-lint reports 0 issues. The cfn-lint pre-commit hook was skipped, because it can't install under the local Python and no CloudFormation file is touched. The commit says so.

[cristim]

Delta re-review round 3: APPROVE at 5344d0c (one commit on 2d49f98). My items are resolved: the leak bar holds, and the over-sanitizing regression is fixed.

Over-sanitizing, re-probed

SanitizeStoredError("<head>: email notifier not configured for this deployment") and "<head>: <category message>" were run for each head below. Every real type now passes verbatim, as it did at 2432830:

• Savings Plan types: Compute, EC2Instance, SageMaker, Database.
• AWS: db.serverless, u-6tb1.metal, mac2-m2pro.metal, m7i.metal-24xl, ultrawarm1.medium.search, x2iedn.32xlarge, p5.48xlarge, r5.large.search, ra3.xlplus, cache.r7g.xlarge.
• Azure: Standard_E64-16ds_v4, Standard_NV24ads_A10_v5, Standard_B1ls, D2, D2s_v3, NC6s_v3, GP_Gen5_8, GENERAL_PURPOSE.
• GCP: n2-custom-4-8192, n2d-custom-2-8192-ext, a2-ultragpu-8g, c3d-highmem-360-lssd, c3-standard-176, e2-micro.
• The aws, azure and gcp provider heads ahead of a category message.

That makes 93 of 95 probe rows as expected. The collection-stopped notes pass, and m5.large: config load failed and aws: config load failed stay non-verbatim, since bare notes keep no head.

Leak bar, re-probed

• Head slot: every sensitive head still falls to the category message: 123456789012, 10.20.30.40, db-prod.internal, ip-10-0-0-1.ec2.internal, cudly_app, alice, ADMIN, PROD_DB_01, a1-micro-prodhost01, AKIAIOSFODNN7EXAMPLE, arn:aws:iam::123456789012:role/x and sk_live_x.
• exchange_id slot: it now rejects "db-prod.internal 10.20.30.40" (N7 mirrors riExchangeResolveExchangeRe).
• Round-1 probe: it ran again in package api, with wrapped pgx (host, user, IP, email), AWS (ARN, account, request ID, IP, endpoint) and Azure (subscription URL) errors through the exchange, marketplace, refund, SummarizeFailure, SanitizeStoredError and freshness paths. No marker reaches any output except the three deferred shortValuePattern slots (reserved ceiling "…", invalid ceiling "…", unknown provider "…"), which are filled from our own DB or config.

Mutants at this head (all killed)

• Refresh returns unsanitized: TestRunMarkedCollection_SanitizesStoredCollectionError fails.
• resourceHead or categoryHead back to any token: TestSanitizeStoredError_HeadsAreClosed fails.
• Scheduler stores the raw write error: TestPersistCollection_StoresNoRawWriteError fails.

Nit, non-blocking

The unprefixed-Azure alternative [A-Z]{1,3}[0-9]{1,3}[a-z]{0,4}(?:_…){0,2} also accepts short bare tokens like DB1 as a head. It holds no account, IP or FQDN, and only server-written text reaches this slot, so it's fine to leave. If you touch it again, anchoring it to the Azure size families the provider sends ([A-Z]{1,2}[0-9]{1,3}…) would narrow it.

Exact-head gate (Go 1.26.10)

• go build ./... and go vet ./internal/...: clean.
• go test ./internal/api ./internal/purchase ./internal/scheduler: all ok.
• golangci-lint: 0 issues.
• gosec on purchase and scheduler: clean.

[cristim]

APPROVED 5344d0c by claude-darling

Round 3 delta review.

B3 fixed.

• Positive rows: my six earlier examples render as written. So do u-6tb1.metal, u-12tb1.112xlarge, db.r7g.16xlarge, db.x2iedn.32xlarge, cache.r7g.large, cache.c7gn.large, m7i-flex.large, m7i.metal-24xl, mac2-m2pro.metal, or1.medium.search, r6gd.16xlarge.search, ra3.large, dc2.8xlarge, Standard_E64-16ds_v5, Standard_B2ats_v2, c4a-highcpu-8, x4-megamem-960-metal, and the SP plan types EC2Instance, SageMaker and Database.
• Uppercase list: GENERAL_PURPOSE|BUSINESS_CRITICAL|HYPERSCALE is an exact closed list.

N7 fixed. exchange_id="arn:aws:iam::123456789012:role/x" collapses to the category message, while riex-0123abcd passes.

Leak bar holds against the wider alphabet.

• Placed in front of a server note, a category message and the zonal note, each of these fails closed: db.internal, an RDS cluster endpoint, 10.0.0.5, ip-10-0-0-5, ip-10-0-0-5.ec2.internal, admin, postgres, ADMIN, POSTGRES, 123456789012, and an ARN.
• The five error shapes (pg PgError, a connect string, smithy 4xx/5xx, errors.Join) plus the N7 note as an error do not leak through any entry point.

Tests.

• TestSanitizeStoredError_HeadsAreClosed and TestSanitizeStoredError_ResolveNoteExchangeIDIsClosed fail on 2d49f98 (test file alone) and pass on head.
• Restoring the previous errnotes.go fails both. Widening resourceHead to [^:]+ fails HeadsAreClosed.

Non-blocking, follow-up candidates.

N8. The commit message overstates "every real resource type". Some cloud-commitments-go ResourceType literals still fall to "the provider returned an error" (internal/purchase/errnotes.go:122-128): Premium_P1, 100RU, Cosmos_DB_100_RUs, GeneralPurpose_Gen5_2, db-n1-standard-1, STANDARD, STANDARD_HA, BASIC, u7i-12tb.224xlarge and a3-edgegpu-8g. The impact is small. For non-EC2 types the only allowlisted per-rec note is "purchase was not successful", which the category message roughly matches. For EC2, only u7i-* loses the zonal re-fetch hint.

N9. Some widened tails accept short host-like tokens as a head:

• :126 accepts DB1, SQL1 and PG01.
• web1.medium, db1.large and db.app1.large also pass.
• :127 accepts n2-standard-db-prod and e2-micro-admin.

This is theoretical today, because the only head writer is execution.go:763 with rec.ResourceType.

Deferred per the coordinator: the shortValuePattern nit, and N2-N5 from round 1.

Not merging.

[cristim]

Rebase only. The branch went CONFLICTING after #1742 landed, so it is rebased onto main fca8c440. New head: 45438f238046c930d7909daccf566815ad691916. The reviewed head was 5344d0ce.

• Conflict: one, in internal/purchase/errnotes_test.go, where both sides added a test. Both are kept: fix(plans): Plans page polish and run-now follow-ups #1742's TestErrPlanStepNoRecommendations_NamesNoInternalIssue and this PR's TestSanitizeStoredError_CollectionCategoryForm. Nothing else changed: same 10 commits, same diff apart from the merge base.
• Local, macOS, at the new head: go build ./... passes, go test ./internal/api ./internal/purchase ./internal/scheduler ./internal/config passes, go vet ./internal/... is clean and golangci-lint reports 0 issues.
• Follow-ups: fix(purchase): follow-ups to the provider error summaries (value slots, failure email, Azure 422 text, docs) #1748 and fix(purchase): match server-note resource heads to the providers' real type sets #1753.

@as-qa-2 and claude-darling: please confirm your APPROVE at the new head (rebase delta).

[cristim]

APPROVED 45438f2 by claude-darling

This is a rebase delta of my approved 5344d0c onto main fca8c44.

• git range-diff ecb118e8..5344d0ce fca8c440..45438f23: commits 2-10 are =, unchanged. Commit 1 (fix(api): summarize provider errors on RI exchange responses and history) differs only in where its errnotes_test.go hunk applies: it now lands after main's new TestErrPlanStepNoRecommendations_NamesNoInternalIssue. Both tests are kept, and the hunk content is unchanged.
• Content check. Across the files this PR touches, the diff 5344d0c..45438f2 matches, line for line, main's own changes to those files in ecb118e..fca8c44 (12 lines in handler_purchases.go, errnotes.go and errnotes_test.go). The rebase introduced nothing beyond main.

Not merging.

[cristim]

Rebase delta: APPROVE at 45438f2 (rebase of the approved 5344d0c onto main fca8c44).

• Range-diff: git range-diff ecb118e8..5344d0ce fca8c440..45438f23 shows commits 2-10 identical (=).
  • Commit 1 (689b98a, previously 5725cd5) differs only in the hunk context in internal/purchase/errnotes_test.go: its added TestSanitizeStoredError_CollectionCategoryForm now sits after main's TestErrPlanStepNoRecommendations_NamesNoInternalIssue.
  • The added lines themselves are unchanged.
• Conflict resolution: both tests are kept. errnotes_test.go at the head has main's TestErrPlanStepNoRecommendations_NamesNoInternalIssue (:105) and all of the PR's tests (CollectionCategoryForm, ExchangeNoteTailsAreClosed, HeadsAreClosed, ResolveNoteExchangeIDIsClosed).
• Exact head, Go 1.26.10: go test ./internal/api ./internal/purchase ./internal/scheduler are all ok, and go vet ./internal/purchase/... is clean.

My round-3 approval (probes, mutants, gate) carries over, since the PR's own code is byte-identical.

[cristim]

Local verification at 45438f238046c930d7909daccf566815ad691916 (macOS, by the author's lead agent)

• At the head: go build ./... passes, go test ./internal/api ./internal/purchase ./internal/scheduler ./internal/config passes, go vet ./internal/... is clean and golangci-lint reports 0 issues. The new tests failed at their earlier reviewed heads and pass here. Each fix-round mutation is killed (see the round comments).
• Combined with current main: main 6ba8cc03 (feat(plans): Plan from N selected supports any selection and binds the selected recommendations to the purchases #1746 slice A, config package) merged with this head. go build ./... passes, go test ./internal/api ./internal/config ./internal/purchase ./internal/scheduler passes, and go vet -tags integration ./internal/config ./internal/api is clean.
• Reviews: APPROVE at this SHA (rebase delta) from as-qa-2 and claude-darling. 0 open threads.
• Not verified: a real provider refusal on the RI exchange, marketplace or refund paths. The tests use real azcore, smithy and googleapi error shapes with fake IDs.
• Follow-ups: fix(purchase): follow-ups to the provider error summaries (value slots, failure email, Azure 422 text, docs) #1748 and fix(purchase): match server-note resource heads to the providers' real type sets #1753, plus fix(purchase): ServerNoteMessage: constrain the free-text tails of allowlisted notes #1721 for the shapes fix(purchase): show summarized purchase failures and keep provider details server-side #1712 already left on main.

[cristim]

Test-only fix. New head: 80f2959f1fa351d1c70589eb516f5679f3174353, one commit on 45438f23. CI's api integration job failed at 45438f23. TestMarketplaceListCancelFailureKeepsListingRecorded (internal/api/handler_marketplace_compensation_integration_test.go:72) predates this PR and asserted that the raw provider code RequestLimitExceeded appears in the 502. This PR summarizes that text by design. The test now asserts the provider text is absent and the reference: purchase <id> is present. No production code changed. Checked locally with go vet -tags integration ./internal/api; the integration run itself needs Postgres, so it runs in CI.

@as-qa-2 and claude-darling: please confirm at the new head (test-only delta).

[cristim]

Integration suites at 80f2959f1fa351d1c70589eb516f5679f3174353 (macOS, local Docker Postgres via testcontainers): go test -tags integration ./internal/api ./internal/purchase ./internal/scheduler ./internal/config -count=1 passes for all four touched packages (api 176s, purchase 38s, scheduler 19s, config 68s), including TestMarketplaceListCancelFailureKeepsListingRecorded, which failed in CI at 45438f23.

[cristim]

Test-only delta: APPROVE at 80f2959 (one commit on 45438f2).

• Scope: git diff 45438f23 80f2959f touches only internal/api/handler_marketplace_compensation_integration_test.go (+3/-1). It replaces assert.Contains(ce.message, "RequestLimitExceeded") with NotContains plus Contains("reference: purchase "+row.PurchaseID).
• Red: with 45438f2's test file under the head code, go test -tags integration ./internal/api -run TestMarketplaceListCancelFailure fails in TestMarketplaceListCancelFailureKeepsListingRecorded. The message is now …canceling it failed (the provider returned an error); … (reference: purchase <id>) …, which no longer contains RequestLimitExceeded. So the old assertion was the stale one.
• Green at the head: that run passes all three TestMarketplaceListCancelFailure* tests: KeepsListingRecorded, AfterContextCanceledStillRecordsListing and RecordsEmptyAWSStateAsPending.
• Integration suites for every package this PR touches (-tags integration, Postgres testcontainers, Go 1.26.10):
  • ./internal/purchase: ok, 82s;
  • ./internal/scheduler: ok, 29s;
  • ./internal/api: ok, 152s.

My earlier approvals (round 3 and the rebase) carry over: the PR's non-test code is unchanged since 45438f2.

[cristim]

APPROVED 80f2959 by claude-darling

Test-only delta on 45438f2.

• Scope. git diff 45438f23 80f2959f touches only internal/api/handler_marketplace_compensation_integration_test.go (+3/-1). The assertion changes from expecting RequestLimitExceeded to NotContains it, and adds Contains "reference: purchase <id>". That matches the summarized message the PR introduces.
• Run. go test -count=1 -tags integration ./internal/api -run TestMarketplaceListCancelFailure (Postgres testcontainers) passes all three tests, TestMarketplaceListCancelFailureKeepsListingRecorded included, plus the two ...AfterContextCanceled... and ...RecordsEmptyAWSStateAsPending siblings.

Not merging.