[IAmJSD]

Root cause

monthlyCostFromConvertibleRI (in both internal/api/handler_ri_exchange.go and internal/server/handler_ri_exchange.go) computed (FixedPrice / hours_per_term + UsagePrice + RecurringHourlyAmount) * 730 with no check on the result. A convertible RI with all three price fields at zero got MonthlyCost 0. In cloud-commitments-go pkg/exchange/reshape.go, pricingGatePasses returns true for MonthlyCost <= 0 and costComponent scores it as a neutral 0.5, so the RI read as free and every cross-family alternative passed the dollar-units gate. A real convertible RI always has an upfront or hourly charge, so an all-zero price set means AWS returned no pricing.

Fix

This takes the issue's first option and follows the pattern #1187 used for a non-positive duration. The change is platform-side only; the go library is untouched.

• monthlyCostFromConvertibleRI now also returns an error when the computed monthly cost is not positive. That covers an all-zero price set, a negative sum and NaN (!(cost > 0)). The duration check still runs first.
• api: the error wraps a sentinel (errRINonPositiveDuration / errRINoPricing). convertToExchangeTypes uses excludedRIReason(err) so the RI appears in excluded_ris with a pricing-specific reason instead of the term-length one. It is logged with a warning and given no recommendation.
• server: convertForAutoExchange logs a warning and drops the RI, the same as the duration case. It has two callers, the auto-exchange run (handler_ri_exchange.go) and the ladder write path (ladder_write.go), so the change applies to both. The library's AnalyzeReshaping does not read MonthlyCost today, so this is a behavior change on both paths: an underused RI whose pricing is unknown is no longer auto-exchanged or ladder-reshaped. It fails closed (no money moves), and the only signal is the warning log; the auto-exchange result and notifications do not report the skip.
• No Upfront RIs (FixedPrice 0 with a recurring hourly charge) and usage-priced RIs are unaffected. Tests cover both shapes from the issue.
• openapi.yaml's excluded_ris description now says each entry carries its own reason (no term length, or no pricing).
• Frontend follow-up for the UI owner (not changed here): renderExcludedRIsNotice in frontend/src/riexchange.ts hard-codes "left out because AWS returned no term length", so for an unpriced RI the notice will name the wrong cause. Showing each entry's reason (e.g. ${e.ri_id}: ${e.reason}) would fix it. A change was drafted and then reverted in e93092f, because frontend work is left to the UI owner. Merge order: the maintainer review asks for that fix to land in a frontend PR merged before this one (or together with it). Until then the UI notice names the wrong cause for an unpriced RI.
• Test fixtures that modeled RIs with a duration but no price now carry a FixedPrice, because otherwise they would now be excluded. Where a test depends on a cross-family alternative passing the dollar-units gate (TestGetReshapeRecommendations_AlternativeCurrencyFromRIs and the reshape integration tests), the price is small enough (100 over 1y, about 8.33/month) that the seeded alternatives still pass. A comment explains the arithmetic.

This PR doesn't change the approve/reject token paths that #1172 / #1175 edit. The openapi.yaml hunk (around line 1333) and the test-file appends don't overlap their hunks.

Verification

All of the evidence below comes from unit, mock and fixture tests plus local Postgres integration tests. No real AWS account was used: the RI lists come from fakes, and nothing called a cloud API. Go tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

New tests:

• api: TestConvertToExchangeTypes_ExcludesRIWithoutPricing (all-zero and negative excluded with the pricing reason; No Upfront, usage-only and all-upfront kept with the correct cost), TestGetReshapeRecommendations_ReportsUnpricedRIs (handler end to end), TestMonthlyCostFromConvertibleRI_NoPositivePriceErrors (all-zero, negative and NaN; sentinel wrapping; duration checked first).
• server: TestConvertForAutoExchange_ExcludesRIWithoutPricing, TestMonthlyCostFromConvertibleRI_NoPositivePriceErrors.
• integration (Postgres container): TestReshapeRecommendations_Integration_EndToEnd now also seeds an unpriced RI and asserts it is reported with the pricing reason.

Fail-before / pass-after:

• server, with internal/server/handler_ri_exchange.go reverted to origin/main: TestConvertForAutoExchange_ExcludesRIWithoutPricing FAIL (got ri-unpriced with MonthlyCost 0 and ri-negative with -73), TestMonthlyCostFromConvertibleRI_NoPositivePriceErrors FAIL (all zero: ... got cost=0 err=<nil>). Both pass with the fix.
• api: the new tests reference the new sentinels, so instead of a plain revert I ran mutation probes. Each was caught:
  • guard disabled: all 3 new api tests FAIL, and the integration EndToEnd test FAILs (should have 2 item(s), but has 1)
  • reason helper forced to the term-length text: 2 FAIL
  • cost < 0: 3 FAIL
  • cost <= 0 (no NaN handling): the NaN case FAILs

Commands, all passing on the pushed SHA unless noted:

go build ./...
go vet ./internal/api/ ./internal/server/ && go vet -tags integration ./internal/api/ ./internal/server/
go test ./internal/api/ ./internal/server/ ./internal/mocks/ -count=1
go test -tags integration ./internal/api/ -count=1 -run TestReshapeRecommendations_Integration   # testcontainers Postgres
RI_EXCHANGE_TEST_DATABASE_URL=postgres://...@127.0.0.1:<port>/issue41_p1188 go test -tags integration ./internal/server/ -count=1 -run TestRIExchangeAutoWorkers_ConcreteStoreAndFixtureClient   # throwaway postgres:16-alpine container
golangci-lint v2.10.1 run ./internal/api/... ./internal/server/...  (with and without --build-tags integration): 0 issues
gocyclo -over 10 -ignore '_test\.go' internal/api/handler_ri_exchange.go internal/server/handler_ri_exchange.go: silent
bash scripts/gosec-hook.sh internal/api/handler_ri_exchange.go internal/server/handler_ri_exchange.go: exit 0
git-secrets --scan of the changed files in a fresh isolated repo with scripts/setup-git-secrets.sh + .gitallowed: clean; scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL

Gaps: I haven't checked against a real AWS account that DescribeReservedInstances never returns an all-zero price set for a valid RI. The fix relies on the library populating RecurringHourlyAmount from RecurringCharges, which ListConvertibleReservedInstances does in the pinned version.

Out of scope

• The issue also mentions an RI whose FixedPrice is missing while the hourly parts are populated, which ends up silently under-priced. That can't be told apart from a legitimate No Upfront RI using these fields, so this PR keeps such RIs. Telling them apart would need OfferingType checks or the library's "price unknown" option (the issue's alternative fix).
• The library-side alternative, passing an explicit "price unknown" so pricingGatePasses refuses, isn't done here.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, triaged, urgency/this-quarter, impact/few, effort/xs, type/bug.

Closes #1188

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 11 billable files and costs up to $2.75.}

Or wait 54 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: c8bb869
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 56f57342-910c-42b4-9f24-d7368a5fc13b

Commits

Reviewing files that changed from the base of the PR and between c8bb869 and 0a8f0ce.

Files selected for processing: 11

• internal/api/handler_ri_exchange.go
• internal/api/handler_ri_exchange_integration_test.go
• internal/api/handler_ri_exchange_test.go
• internal/api/openapi.yaml
• internal/ricost/monthly_cost.go
• internal/ricost/monthly_cost_test.go
• internal/server/handler_ri_exchange.go
• internal/server/handler_ri_exchange_reservation_integration_test.go
• internal/server/handler_ri_exchange_test.go
• internal/server/ri_exchange_four_eyes_test.go
• internal/server/ri_exchange_zone_test.go

• 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 2 (claude-fleet, cold): APPROVE e93092f

Findings (none blocking):

• [non-blocking] frontend/src/riexchange.ts:572 - renderExcludedRIsNotice hard-codes "left out because AWS returned no term length" and ignores each entry's reason.
  • This PR adds a second exclusion cause, no pricing (internal/api/handler_ri_exchange.go:165-169), so an unpriced RI will appear in the UI notice with the wrong cause.
  • The PR body says this and leaves it to the UI owner. It needs a tracked follow-up issue.
• [non-blocking] internal/server/handler_ri_exchange.go:279-282 - The auto-exchange path, including the ladder caller (internal/server/ladder_write.go:59), now drops RIs whose prices sum to zero or less.
  • Before this PR, AnalyzeReshaping (pkg exchange/auto.go:222) never read MonthlyCost, so such RIs could still be auto-exchanged.
  • The change fails closed: such an RI is skipped and no money moves.
  • The only signal is a Warnf log; the auto result and notifications do not report the skip. That is fine if AWS never returns an all-zero price set for a valid convertible RI, which the PR lists as unverified.
• [non-blocking] internal/server/handler_ri_exchange.go:257-266 and internal/api/handler_ri_exchange.go:1610-1639 - The two copies of monthlyCostFromConvertibleRI (the duplication predates this PR) have drifted further, since only the api copy wraps the sentinels. A future edit could update one and miss the other.

Attacked:

• Root cause: costComponent (reshape.go:220) and pricingGatePasses (reshape.go:679) both treat MonthlyCost <= 0 as "no pricing", which confirms the issue.
  • ListConvertibleReservedInstances (providers/aws ec2/client.go:796-818) sums RecurringCharges into RecurringHourlyAmount, so No Upfront RIs keep a positive cost and are not wrongly excluded.
  • The claim that AnalyzeReshaping does not read MonthlyCost holds: its only readers are reached from AnalyzeReshapingWithRecs.
• Blast radius: convertForAutoExchange has 2 callers and convertToExchangeTypes has 1. Fixtures without prices were updated, including the integration-tagged ones.
• Edge cases: !(cost > 0) rejects NaN and negative sums. The duration check runs before the price check, and the reason and sentinel mapping are correct.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp. All 5 were caught:
  1. server cost < 0: 2 tests FAIL.
  2. api cost <= 0 (NaN let through): NoPositivePriceErrors FAILS.
  3. api reason sentinel swapped: 3 tests FAIL.
  4. server cost <= 0: NoPositivePriceErrors FAILS.
  5. api returns cost*2: 4 tests FAIL.
• Fail-before, with the head's tests on base:
  • In server, TestConvertForAutoExchange_ExcludesRIWithoutPricing FAILS (it keeps the cost-0 and cost-negative RIs), and TestMonthlyCostFromConvertibleRI_NoPositivePriceErrors FAILS.
  • In api, TestConvertToExchangeTypes_ExcludesRIWithoutPricing FAILS (5 kept instead of 3), and TestGetReshapeRecommendations_ReportsUnpricedRIs FAILS (no excluded_ris).
  • The api sentinel test cannot compile on base.
• No migration.

Verified locally, on a git archive export with no cloud credentials, under the heavy lock:

• go build ./...: OK.
• go vet ./internal/api/ ./internal/server/, plain and with -tags integration: OK. gofmt -l: clean.
• go test -count=1 ./internal/api/ ./internal/server/: ok.
• go test -tags integration -count=1 ./internal/api/ -run TestReshapeRecommendations_Integration (testcontainers Postgres): ok.
• Not run: the server reservation integration test, which needs an external DB URL.

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

[cristim]

VERDICT: REQUEST_CHANGES e93092f

Base: origin/main 9fceda3. git merge-tree is clean (merged tree 29fcdb71). The merged tree builds, and vet passes with and without -tags integration.

Findings

• medium frontend/src/riexchange.ts:572 - renderExcludedRIsNotice hard-codes "left out because AWS returned no term length" and never reads e.reason. This PR adds a second exclusion cause (excludedRIReason, internal/api/handler_ri_exchange.go:1634), so an unpriced RI that does have a term length is now shown to the operator with the wrong cause. On a money path, an operator would chase a nonexistent duration problem instead of missing pricing. The fix is about 3 lines (render ${e.ri_id}: ${e.reason} or group by reason) plus the existing jest test. Land it in this PR or in a frontend PR merged first. The PR body acknowledges this and defers it, but the backend change is what makes the UI text wrong.
• low (blast radius, not in body) internal/server/handler_ri_exchange.go:280 - convertForAutoExchange is also the ladder write path (internal/server/ladder_write.go:59). On both the auto-exchange path and the ladder path, an unpriced RI is now dropped with only a log line. The library's AnalyzeReshaping (pkg/exchange/reshape.go:415) does not read MonthlyCost, so this is a behavior change: an underused unpriced RI is no longer auto-exchanged or ladder-reshaped. It is conservative (no money moves) and matches fix(ri-exchange): exclude convertible RIs with a non-positive duration from cost math #1187's duration handling. Please mention the ladder path in the body.
• nit internal/server/handler_ri_exchange.go:263 - the server copy does not wrap the sentinels, so the two monthlyCostFromConvertibleRI copies have drifted again. This is acceptable because the server path has no user-facing reason.
• nit (process) the largest commit is 2351fa7 at 262 changed lines, under the 400 cap. e93092f is 21 lines.

Consistency with #1187: same shape. The duration check runs first; the API path reports the RI in excluded_ris with its own reason; the server path logs and drops it. NaN and negative costs are rejected via !(cost > 0). No Upfront RIs (recurring charge only) and usage-only RIs keep their cost. An excluded RI cannot get a recommendation: AnalyzeReshapingWithRecs iterates ris, not utilization (reshape.go:561), so the !hasSrc gate pass at :679 is unreachable for it.

Attacked:

• Merged tree: go test ./internal/api/ ./internal/server/ -count=1: 4530 passed. Integration (-tags integration, testcontainers Postgres): TestReshapeRecommendations_Integration_*: 4 passed, EndToEnd included.
• Mutation probes (merged tree, scratch copy):
  • api !(cost > 0) changed to cost < 0: 3 tests FAIL.
  • excludedRIReason forced to the term-length text: 2 FAIL.
  • api excluded-append replaced with a silent drop: 4 FAIL.
  • server guard disabled: 2 FAIL.
  • server guard changed to cost <= 0 (NaN): 1 FAIL.
• Fixture edits: added FixedPrice values keep the dollar-units gate arithmetic valid (100/yr gives 8.33/mo; NF 8 x 8.33 < NF 4 x 40/50/60). No assertion was weakened.
• The library pin (ad67030387e4) uses MonthlyCost only in reshape.go (pricingGatePasses, costComponent).

[IAmJSD]

This replies to the VERDICT (REQUEST_CHANGES) and Independent review 2 on e93092f. The new head is 9df76b6.

Merged origin/main (7998620; no rebase). The merge was clean. Main's changes to internal/api/handler_ri_exchange.go (#1092 audit trail, #1230) and to go.mod don't touch the RI cost code.

VERDICT findings

• medium: frontend/src/riexchange.ts:572 renderExcludedRIsNotice ignores e.reason. Not changed here: frontend work is left to the UI owner on this contract. The fix is to render ${e.ri_id}: ${e.reason} (or group by reason) and update the jest test. I added a Merge order note to the body's "Frontend follow-up" section: this PR should merge after that frontend fix or together with it. Merging first, or opening a tracked issue for the fix, is your call.
• low: the ladder path is not mentioned in the body. The body's server bullet now names both callers (auto-exchange and ladder_write.go:59). It says this is a behavior change on both paths: such an RI fails closed, and the only signal is a log line. In code, internal/server/handler_ri_exchange.go:276 documents both callers, and the warning at :287 now says "auto-exchange and ladder reshaping".
• nit: the server copy doesn't wrap the sentinels. Left as is, since the server path has no user-facing reason, as you noted. Each copy now has a "keep in sync" comment that points at the other and says how they differ: internal/server/handler_ri_exchange.go:257 and internal/api/handler_ri_exchange.go:1618.
• nit (process): commit size. No action needed. The new commit 9df76b6 changes 15 lines.

Independent review 2

• Frontend notice: same as above.
• Auto path only logs the skip: now stated in the body. Reporting the skip in the auto result or notifications is out of scope.
• Copies drifting: handled by the cross-reference comments above.

Verification, on the merged tree at 9df76b6 (AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, no cloud calls):

go build ./...                                                          OK
go vet ./internal/api/ ./internal/server/ (plain and -tags integration) OK
go test ./internal/api/ -count=1                                        ok
go test ./internal/server/ -count=1                                     ok
go test ./internal/mocks/ -count=1                                      ok
go test -tags integration ./internal/api/ -run TestReshapeRecommendations_Integration   4/4 PASS (testcontainers Postgres)
golangci-lint v2.10.1 run ./internal/api/... ./internal/server/... (plain and --build-tags integration)   0 issues
gocyclo -over 10 -ignore "_test\.go" on both handler files              silent
git-secrets --scan of both changed files (fresh repo, isolated HOME, setup-git-secrets.sh + .gitallowed)   clean

The PR stays a draft.

[cristim]

VERDICT: REQUEST_CHANGES 9df76b6

Delta e93092f..9df76b6: a merge of main c8bdd46 plus 9df76b6. That commit only adds comments: "keep in sync" notes on both monthlyCostFromConvertibleRI copies, and the convertForAutoExchange doc and log text now mention the ladder path. There is no code or test change, and the frontend is untouched. e93092f (already the old head) is the commit that reverted the frontend fix.

Findings

• blocking frontend/src/riexchange.ts:593 (identical on main and on the merged tree): renderExcludedRIsNotice still hard-codes "left out because AWS returned no term length" and ignores e.reason (ExcludedRI.reason exists, frontend/src/api/riexchange.ts:64-67). After this PR, an unpriced RI (errRINoPricing, internal/api/handler_ri_exchange.go:1646-1650) shows the wrong cause in the UI. No jest test was added. The PR body leaves this to the "UI owner" and asks for a frontend PR merged first, but no such PR exists (search found only fix(ri-exchange): exclude convertible RIs with no pricing from cost math #1191, fix(ri-exchange): exclude convertible RIs with a non-positive duration from cost math #1187 and fix(frontend): seven small recommendations, history and RI exchange bugs #1126). The previous blocker is NOT fixed.
• nit internal/api/handler_ri_exchange.go:1629: !(cost > 0) lets +Inf through (FixedPrice overflow). Not realistic.
• nit: the two monthlyCostFromConvertibleRI copies are still duplicated (pre-existing from fix(ri-exchange): exclude convertible RIs with a non-positive duration from cost math #1187). A cross-reference comment now documents it.

Money path: the formula is unchanged and the guard runs after the duration check. NaN, 0 and negative costs are rejected. No Upfront and usage-only RIs are kept. This is consistent with #1187: same sentinel style and the same exclusion slot. The server copy deliberately has no sentinels, its callers (handler_ri_exchange.go:140, ladder_write.go:59) log and drop, and its comment says so.

Attacked: merge-tree vs origin/main 6af022f is clean (tree 47096e8d). On the merged tree: go test ./internal/api ./internal/server gave 4702 passed. Integration (testcontainers, under lockf): TestReshapeRecommendations_Integration_* 4/4 PASS and TestRIExchangeAutoWorkers_ConcreteStoreAndFixtureClient PASS.
Mutation probes:

• api guard cost < 0: 3 tests FAIL
• excludedRIReason forced to the term-length text: 2 FAIL
• server guard disabled: 2 FAIL
  All were restored.

CI on 9df76b6: Build Docker Image, Security Scanning and CI Success fail on GO-2026-6809 (stdlib, fixed in 1.26.10). That comes from the old base (go 1.26.9 at c8bdd46); main is now 1.26.10. Everything else passes.

[IAmJSD]

Merged current main (c8bb869) into the branch, in two merge commits: fd374a9, then 0a8f0ce for #1426. New head: 0a8f0ce278dfadf64a8234021a54542954927b21.

Merge resolution. Main's #1260 replaced both monthlyCostFromConvertibleRI copies with one shared internal/ricost.MonthlyCostFromConvertibleRI. This branch had added the no-positive-price exclusion (#1188) to each copy, which caused the conflict. Resolution:

• Kept main's single implementation and moved this branch's check into it. !(cost > 0) still rejects 0, negative sums and NaN, and the duration check still runs first.
• The sentinels are now exported as ricost.ErrNonPositiveDuration and ricost.ErrNoPricing. The API's excludedRIReason still uses them to pick each RI's reason.
• Both package wrappers just delegate now, so the two copies can no longer drift. The "keep in sync" comments are gone.
• ricost's table had pinned "zero prices -> 0, no error" as current behavior "rather than endorsing it". That row now expects the no-pricing error, and I added a negative-sum row. Each error row also asserts its sentinel.
• ri_exchange_four_eyes_test.go came in from main (fix(ri-exchange): run unattended exchanges as manual while 4-eyes is on #1108) and used an unpriced RI, which this PR now excludes. I gave it the same FixedPrice: 876 the branch's other fixtures use. No assertion was changed.

@cristim, your REQUEST_CHANGES on 9df76b6, point by point

1. Blocking: renderExcludedRIsNotice hard-codes "AWS returned no term length" and ignores e.reason. Fixed on main by your fix(frontend): show the server's reason for each excluded RI on the exchange page #1408, which is in this merged tree. Excluded RIs are now grouped by the server's reason, and each group renders as "N convertible RI(s) was/were left out because : ids". Only an empty or missing reason falls back to the term-length text. An unpriced RI's reason comes from excludedRIReason(ricost.ErrNoPricing): "AWS returned no pricing (fixed, usage and recurring charges total zero or less), so its monthly cost cannot be computed". So the page shows the real cause, and a duration exclusion shows its own reason in a separate group. TestGetReshapeRecommendations_ReportsUnpricedRIs checks the server side (the reason contains "no pricing" and no recommendation is made for that RI). fix(frontend): show the server's reason for each excluded RI on the exchange page #1408's jest cases check the grouping and the fallback. I verified the frontend by reading the code; this PR changes no frontend files, so I didn't run jest.
2. Nit: !(cost > 0) lets +Inf through. Unchanged. As you noted, it needs a FixedPrice overflow, which isn't realistic. A follow-up can add a math.IsInf guard in ricost if you want one.
3. Nit: the two monthlyCostFromConvertibleRI copies are duplicated. Resolved by refactor(ri-exchange): share the convertible RI monthly cost formula #1260 plus this merge: there is now one implementation in internal/ricost.
4. CI failing on GO-2026-6809 from the old base. The branch now has main's Go 1.26.10 toolchain.

Checks on the merged tree, with no cloud credentials:

• go build ./..., go vet ./..., go vet -tags integration ./... and the e2e vet all pass.
• golangci-lint v2.10.1 (with and without the integration tag) and gocyclo -over 10 are clean on internal/{api,ricost,server}.
• go test -race -short passes on internal/{api,ricost,server}.
• go test -race -tags integration passes on internal/api. On internal/server, every test passed except TestCheckConfigStore_DBFailureHidesRawError, whose Postgres container did not start. That test is unrelated to this PR, and it passed when rerun once.
• The secrets scan prints "clean; planted key flagged", and the allowlist test passes.

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 0a8f0ce278dfadf64a8234021a54542954927b21

Blocking:

• none

Non-blocking:

• internal/server/handler_ri_exchange.go convertForAutoExchange: on both the auto-exchange path and the ladder write path, an unpriced RI is now dropped with only a warning log. The library's AnalyzeReshaping never read MonthlyCost, so this changes what those paths do, not just the cost math. It fails closed, so no money moves. The body already says so. If an AWS shape exists where a real convertible RI reports all-zero prices (for example one produced by an earlier exchange), it would quietly stop being auto-exchanged, and nothing in the auto-exchange result or notification would show it. A skipped count in the result would make that visible. Not a blocker.
• internal/ricost/monthly_cost.go: !(cost > 0) still lets +Inf through. This was raised in an earlier review and is not realistic.

Attacked:

• Every caller of ricost.MonthlyCostFromConvertibleRI goes through one of the two package wrappers. The API path reports the RI in excluded_ris with excludedRIReason. The server path (convertForAutoExchange, used by both auto-exchange and ladder_write.go) drops it. The duration check still runs first, and its error text is unchanged (has %w %d). The pinned go lib (providers/aws 92722e67) fills RecurringHourlyAmount from RecurringCharges, so No Upfront RIs keep a positive cost. Main's frontend (renderExcludedRIsNotice) now groups by e.reason, so the new reason renders correctly.
• Ran in a scratch worktree at 0a8f0ce: go build ./...; go vet with and without -tags integration on internal/{api,server,ricost}; go test -race -short -count=1 on those three (ok); go test -race -tags integration -count=1 ./internal/server/ (full, ok, and TestRIExchangeAutoWorkers_ConcreteStoreAndFixtureClient really ran and passed) and -run 'RIExchange|Reshape' ./internal/api/ (ok); golangci-lint (0 issues). git merge-tree against origin/main 8368330: clean.
• Fail-before: with the guard in ricost disabled (if false && !(cost > 0)), TestConvertToExchangeTypes_ExcludesRIWithoutPricing, TestGetReshapeRecommendations_ReportsUnpricedRIs, both TestMonthlyCostFromConvertibleRI_NoPositivePriceErrors and TestConvertForAutoExchange_ExcludesRIWithoutPricing fail. Restored afterwards. The fixture edits only add a FixedPrice, and no assertion was weakened.
• After writing this verdict I skimmed the earlier reviews. The +Inf nit and the ladder blast-radius point came from cristim's earlier reviews. His blocking frontend finding is resolved on main by fix(frontend): show the server's reason for each excluded RI on the exchange page #1408.

Body-stale:

• "Frontend follow-up for the UI owner ... renderExcludedRIsNotice hard-codes 'left out because AWS returned no term length'" and the Merge order note: no longer true. Main's renderExcludedRIsNotice groups by each entry's reason (fix(frontend): show the server's reason for each excluded RI on the exchange page #1408), so there is no merge-order constraint left.
• "Root cause: monthlyCostFromConvertibleRI (in both internal/api/... and internal/server/...) computed ...": since refactor(ri-exchange): share the convertible RI monthly cost formula #1260 the formula lives once, in internal/ricost/monthly_cost.go, and both are thin wrappers around it. The fix is in ricost, not in two copies.
• "the go library is untouched" is still true. "api: the error wraps a sentinel (errRINonPositiveDuration / errRINoPricing)": the sentinels are now ricost.ErrNonPositiveDuration / ricost.ErrNoPricing.

_{Reviewer: rC5. 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 0a8f0ce278dfadf64a8234021a54542954927b21

Blocking:

• none

Non-blocking:

• Behavior change on a money path. convertForAutoExchange serves both the auto-exchange run and ladder_write.go, and it now silently drops an RI whose prices total zero or less. The only signal is a Warn log; the run result and notifications say nothing. This fails closed, since no money moves. But if AWS ever reports zero prices for a real RI (for example, I couldn't verify what DescribeReservedInstances returns for an RI received through an earlier exchange), that RI will never be auto-exchanged and nobody will notice. Consider a follow-up that surfaces the skipped RIs in AutoExchangeResult.
• internal/ricost/monthly_cost.go: !(cost > 0) lets +Inf through, for example with a pathological 1-second duration. Not realistic (an earlier review raised this too).
• UI wording: fix(frontend): show the server's reason for each excluded RI on the exchange page #1408's notice reads "N convertible RIs were left out because AWS returned no pricing (...), so its monthly cost cannot be computed". The grammar is a bit off for the plural, which is cosmetic.

Attacked:

• Read the diff against the merge-base. The cost formula now has one implementation, ricost.MonthlyCostFromConvertibleRI. Both packages delegate to it, and its only callers are convertToExchangeTypes (api) and convertForAutoExchange (server: auto-exchange plus the ladder). Every ConvertibleRI reaching it comes from the pinned library's ListConvertibleReservedInstances (providers/aws 92722e67), which sums every RecurringCharges amount into RecurringHourlyAmount. So No Upfront RIs keep a positive cost and aren't excluded. The duration check still runs first. NaN and negative costs are rejected.
• Ran go build ./..., go vet with and without -tags integration on internal/{api,server,ricost}, go test -race -short -count=1 on the same three (ok), go test -race -tags integration -run Reshape ./internal/api/ (EndToEnd on testcontainers Postgres) and go test -race -tags integration -run 'TestRIExchangeAutoWorkers|Reservation' ./internal/server/ (ok). The branch merges cleanly into current main.
• Fail-before (mutation, because the tests reference the new sentinels): I disabled the !(cost > 0) guard in ricost. These failed: TestConvertToExchangeTypes_ExcludesRIWithoutPricing, TestGetReshapeRecommendations_ReportsUnpricedRIs, both TestMonthlyCostFromConvertibleRI_NoPositivePriceErrors, TestConvertForAutoExchange_ExcludesRIWithoutPricing, ricost TestMonthlyCostFromConvertibleRI, and the integration TestReshapeRecommendations_Integration_EndToEnd. With the guard restored, all pass.
• Checked the frontend on main. renderExcludedRIsNotice (frontend/src/riexchange.ts:584) has grouped by each entry's reason since fix(frontend): show the server's reason for each excluded RI on the exchange page #1408, so the earlier blocking frontend finding is resolved and the merge-order caveat no longer applies.

Body-stale:

• "Frontend follow-up ... Merge order: ... Until then the UI notice names the wrong cause". fix(frontend): show the server's reason for each excluded RI on the exchange page #1408 already shows each entry's reason on main, so no follow-up or ordering is needed.
• The Root cause, Fix and Fail-before sections place the formula and sentinels in internal/api and internal/server/handler_ri_exchange.go (errRINonPositiveDuration / errRINoPricing). They now live in internal/ricost/monthly_cost.go (ricost.ErrNonPositiveDuration / ricost.ErrNoPricing). Reverting only internal/server/handler_ri_exchange.go to main would no longer fail any test.

_{Reviewer: rC6. 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]

Local verification at 0a8f0ce278dfadf64a8234021a54542954927b21 merged onto main (89e11403), together with #1191, #1257, #1259 in that order (combined tree f2c1ffb3, 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 2 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: 2 approving independent review(s) on 0a8f0ce2 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 0a8f0ce278dfadf64a8234021a54542954927b21.