[IAmJSD]

Root cause

monthlyCostFromConvertibleRI existed twice, verbatim, in internal/api/handler_ri_exchange.go (reshape recommendations) and internal/server/handler_ri_exchange.go (auto-exchange and ladder paths). Each copy had its own doc comment and a bare * 730. The two copies could drift, and the dollar-units pre-filter depends on both sides computing the same number.

Fix

• New leaf package internal/ricost (monthly_cost.go) holds the only implementation, ricost.MonthlyCostFromConvertibleRI, plus HoursPerMonth = 730.0. Its comment explains that this is AWS's canonical hours-per-month convention (8760 h / 12) and matches hoursPerMonth in cloud-commitments-go's providers/aws/recommendations. That constant is unexported in another module, so it can't be imported here. The 3600 seconds-to-hours divisor is named too (secondsPerHour).
• The function body is the old body with the literals swapped for the named constants. The expression, its evaluation order, the non-positive-duration guard and the error text are all unchanged.
• In both internal/api and internal/server, monthlyCostFromConvertibleRI is now a one-line delegator to the shared function. I kept the package-local name on purpose. The call sites (convertToExchangeTypes, convertForAutoExchange) and the existing tests in both packages stay as they are. Open PR fix(ri-exchange): exclude convertible RIs with no pricing from cost math #1191 also adds tests in both packages that call this name, and they keep compiling after a merge.
• Neither of the two existing packages could hold the shared code. internal/server already imports internal/api, but putting cost math into the HTTP handler package just so the scheduler side can reach it would couple them the wrong way. The new package depends only on the go library's ec2svc.ConvertibleRI.

Refs #337

Refs, not a closing keyword, because the issue's frontend acceptance criterion (730 vs 1/720) is not part of this PR. See the follow-up below.

Verification

All of this is unit-level evidence. No cloud API was called. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.

New test internal/ricost/monthly_cost_test.go:

• TestMonthlyCostFromConvertibleRI is a table test covering 1y and 3y all-upfront, no-upfront, partial-upfront, usage-price-only, all three components, zero prices, a 1-second term, and zero duration, negative duration, and zero duration plus zero prices (the guard). It compares the float64 results exactly (assert.Equal, not InDelta) and checks the error text exactly. The expected values were taken by running the old code on origin/main (a14dc8f) through both old copies. The api and server outputs were identical to each other.
• TestHoursPerMonth checks that the constant is 730 = 365*24/12.

Before/after evidence:

• Before: the same table, retargeted at the old package-local monthlyCostFromConvertibleRI, was run in a throwaway worktree at origin/main a14dc8f. It passes in both internal/api and internal/server. This is a refactor, so the test is meant to pass before and after; it guards against behavior change rather than reproducing a bug.
• After: the table passes against ricost.MonthlyCostFromConvertibleRI. A temporary capture test printed every case's result (strconv.FormatFloat(v, 'g', -1, 64)) and error through each package's monthlyCostFromConvertibleRI, before and after the change. The output was byte-identical in both packages, and the temporary file was then deleted.
• Mutation probes on internal/ricost/monthly_cost.go (each was reverted afterwards):
  • HoursPerMonth 730 to 720: FAIL
  • secondsPerHour 3600 to 3660: FAIL
  • guard <= 0 to < 0: FAIL
  • dropping UsagePrice: FAIL
  • dropping RecurringHourlyAmount: FAIL
  • Gap: an algebraically equivalent reassociation (FixedPrice*730/hours/730) still passes, because it rounds the same on these inputs. The fixed cases don't catch every possible rounding-order change. The shipped expression is textually the old one, so this only matters for future edits.

Commands (all passed at ff3420c):

go build ./...
go vet ./... && go vet -tags integration ./...
go test ./internal/ricost/ -count=1
go test ./internal/api/ -count=1
go test ./internal/server/ -count=1
go test ./internal/mocks/ -count=1
golangci-lint (v2.10.1) run --allow-parallel-runners ./internal/ricost/... ./internal/api/ ./internal/server/   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/ricost/monthly_cost.go internal/api/handler_ri_exchange.go internal/server/handler_ri_exchange.go   # silent
gofmt -l (changed files)   # clean

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 936996ad = origin/main a14dc8f2 plus these heads, merged in this order:

Branch	Tested head
fix/issue-337	ff3420c
fix/issue-169	7d75eb5
fix/issue-342	66b221f
fix/issue-310	5e11c21
fix/issue-326	5f0ea59
fix/issue-243	365e959
fix/issue-343	6c94036
fix/issue-312	d2848a8
fix/issue-316	acd60dd
fix/issue-339	75ea9b9
fix/issue-339-commitment-type	73bdb19
fix/issue-353	5d25fcd
fix/issue-313	3156bc4
fix/issue-357	53d9e58
fix/issue-368	0ba727d
fix/issue-419	61648c9
fix/issue-111	b04c372
fix/issue-686	0ad90b7
fix/issue-674	793095d
fix/issue-1055	4ec96f1
fix/issue-559	dfa9525
fix/issue-558	034eed4
fix/issue-874	1720539
fix/issue-666	1e120da
fix/issue-668	fb26345
fix/issue-568	ce04173
fix/issue-311	e54f250
fix/issue-177	0cbb88a
fix/issue-151	c21377c
fix/issue-166	41fb77b
fix/issue-27	2d67622
fix/issue-323	69288ae
fix/issue-59	36463e8
fix/issue-43	35e4069
fix/issue-158	6f76f8a
fix/issue-104	bb00491
fix/issue-55	971ef1b
fix/issue-361	89ac220
fix/issue-120	b458719
fix/issue-36	42bac6e
fix/issue-525	57c97dd
fix/issue-712	e5bcda0
fix/issue-72	6cc63ed

fix/issue-72 and fix/issue-312 conflict on one line in internal/api/handler_dashboard.go (both change the calculateCommitmentMetrics call). The tree used #72's side with #312's validated provider variable; whichever PR merges second takes that same resolution.

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:

• Passed at 936996ad: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks at 936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. tests/e2e is a separate module, so it was vetted under its own e2e tag. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package, one package at a time (-tags integration -count=1, 60-minute local timeout because the VM was shared):
  • At 936996ad, these pass: internal/server, internal/credentials, internal/scheduler, internal/database/postgres/migrations, internal/purchase and internal/api.
  • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
  • These were run on the first build of this tree (26795376, same branches at earlier heads): internal/analytics, internal/auth, internal/commitmentopts, internal/database and cmd/lambda/clear-rate-limit all pass. The later fixes don't touch those packages.
• What the combined run caught, each fixed on its own branch before this PR opened:
  • a compile break between fix/issue-104's test and fix/issue-326's httpToLambdaRequest signature change;
  • fix/issue-316 rejecting legacy-keyed plans;
  • fix/issue-120 versus main's global-filter test.
• Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• Nothing was applied to any cloud.

Overlap with other open PRs

• fix(ri-exchange): exclude convertible RIs with no pricing from cost math #1191 (fix/issue-1188, exclude convertible RIs with no pricing) rewrites the bodies and doc comments of both monthlyCostFromConvertibleRI copies. It adds a !(cost > 0) check, and in api it wraps the errors in the sentinels errRINonPositiveDuration / errRINoPricing. Those are the same lines this PR replaces, so whichever merges second will get a textual conflict in both handler files. Once both land, the pricing check belongs in ricost.MonthlyCostFromConvertibleRI, so it exists in one place. The sentinels would then move to (or be exported from) internal/ricost, so api's excludedRIReason can still use errors.Is. fix(ri-exchange): exclude convertible RIs with no pricing from cost math #1191's tests call the package-local monthlyCostFromConvertibleRI, which this PR keeps, so they need no rename. Note that TestMonthlyCostFromConvertibleRI's "zero prices" row pins today's 0, nil result. fix(ri-exchange): exclude convertible RIs with no pricing from cost math #1191 makes that an error on purpose, so that row has to change to expect an error when the two are combined.
• fix(ri-exchange): answer the go exchange refusals with 422 instead of 500 #1212 touches internal/api/handler_ri_exchange.go only around lines 1312 and 2020 (the exchange refusal handling). There is no overlap with this PR's hunks (the import block and the function around line 1600).
• feat(ri-exchange): pass offered instance types to reshape analysis #1246 adds an internal/api import to internal/server/handler_ri_exchange.go between ec2svc and config. This PR adds the ricost import after email, two lines away, so it should merge cleanly. The other open PRs that touch these files (fix(config): validate ri_exchange_mode on PUT /api/config #1243, fix(api): answer a missing RI exchange like a wrong token on approve and reject #1175, fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172, fix(db): let ri_exchange_history.account_id hold Azure and GCP identifiers #1170, fix(auth): bind group account scope entries to account IDs #1116, fix(ri-exchange): run unattended exchanges as manual while 4-eyes is on #1108) edit unrelated hunks.

Out of scope

• I didn't change how an RI with zero prices is handled: it still costs 0 with no error. That is fix(ri-exchange): exclude convertible RIs with no pricing from cost math #1191's subject.
• The 730 comments and the basisNote string in internal/archera/dto.go ("730-hour monthly rates") describe the convention in user-facing text. They aren't arithmetic, so I left them alone.

Frontend follow-up for the UI owner

This PR changes nothing under frontend/. The issue's frontend criterion is still open:

• frontend/src/modules/savings-history.ts:25 uses HOURS_PER_MONTH = 730 for monthly to hourly. Its comment // 365.25 * 24 / 12 is wrong: that works out to 730.5. 730 comes from 365 * 24 / 12.
• frontend/src/recommendations.ts:1655 (PERIOD_FACTOR.hourly: 1 / 720 // 24 × 30 hrs/mo, used by scaleCost) converts the same API monthly figures to hourly with 720. The backend and the AWS-derived monthly figures use 730-hour months, so this view's hourly numbers come out about 1.4% higher (730/720) than savings-history's for the same monthly value. Suggested fix: export a single HOURS_PER_MONTH = 730 and use 1 / HOURS_PER_MONTH in PERIOD_FACTOR. The same table's daily: 1 / 30 makes the same 30-day assumption (730 h = 30.42 days), so it's worth deciding on it at the same time.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p3, severity/low, urgency/eventually, impact/internal, effort/s, type/chore, triaged.

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• internal/ricost/monthly_cost_test.go:31: the "zero prices" row pins 0, nil. The PR body already says that when fix(ri-exchange): exclude convertible RIs with no pricing from cost math #1191 lands, that row has to change to expect an error and fix(ri-exchange): exclude convertible RIs with no pricing from cost math #1191's pricing check has to move into ricost. Whoever merges second needs to make both changes. Otherwise the shared function would be missing the check, and the api/server wrappers would grow apart again.
• The body correctly uses Refs #337: the issue's frontend criterion (730 vs 1/720 in savings-history.ts and recommendations.ts) is still open. Someone should own that as a follow-up, because the backend half does nothing about the 1.4% hourly discrepancy users see.

Attacked:

• Behavior equivalence: the new body is the old expression with 3600 replaced by an untyped const secondsPerHour = 3600 and 730 by an untyped HoursPerMonth = 730.0. Both convert to float64 at the same point in the expression, so the result is bit-identical, and the guard and error text are unchanged. Both packages' monthlyCostFromConvertibleRI now just call the shared function, and their only callers (convertToExchangeTypes and convertForAutoExchange) are unchanged. A grep found no other bare 730 arithmetic left under internal/ or cmd/. I checked that hoursPerMonth = 730.0 really is in cloud-commitments-go providers/aws/recommendations/parser_sp.go:98 at the pinned version, so the doc comment's claim holds.
• This is a refactor, so there's no fail-before. As a mutation probe I changed HoursPerMonth to 720, and 7 subtests of the new exact-equality table failed. I then restored it.
• Ran go build ./..., go vet with and without -tags integration, and go test -race -short -count=1 on internal/ricost, internal/api and internal/server, all ok. golangci-lint v2.10.1 found 0 issues with and without --build-tags integration. New package internal/ricost imports only ec2svc, so there's no import cycle.

Body-stale:

• none (the verification section cites ff3420c; the head is now merge commit 2497cdf, which changes nothing in these files)

_{Reviewer: rB10. 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 2497cdf5388c67415610b086d3652a8bf3aa9178

Blocking:

• none

Non-blocking:

• internal/ricost/monthly_cost.go:22-23: the doc comment says the formula matches "the AWS effectiveMonthlyCost function". No function by that name exists in this repo or in the pinned cloud-commitments-go modules (pkg, providers/aws); the offering-side comment lives in pkg/exchange/reshape.go. The reference was carried over from the old copies, but the new single home is a good place to point at the real counterpart.
• TestHoursPerMonth's int(HoursPerMonth) assertion means a non-integral value fails to build rather than fail the test. Harmless, just noisier.

Attacked:

• Bit-identity: secondsPerHour is an untyped const, so float64(ri.Duration) / secondsPerHour divides by float64 3600 exactly as / 3600 did, and * HoursPerMonth (untyped 730.0) equals * 730. The expression order is unchanged, as are the error text and the <= 0 guard. Both internal/api and internal/server now delegate, so there's one implementation. HoursPerMonth = 730.0 matches hoursPerMonth in cloud-commitments-go/providers/aws/recommendations/parser_sp.go:98.
• This is a refactor, not a bug fix, so there's no fail-before. As a mutation probe, I set HoursPerMonth to 730.5. The ricost tests break, and so do the pre-existing api/server tests TestMonthlyCostFromConvertibleRI_NonPositiveDurationErrors and TestConvert*_ExcludesNonPositiveDuration. Restored afterwards.
• Ran go build ./..., go vet on ricost/api/server with and without -tags integration, go test -race -short -count=1 on ricost/api/server (ok), and go test -race -tags integration -count=1 on ./internal/server/ (ok) and ./internal/api/ (ok on a quiet run). The first api integration run failed one test, TestCreatePlannedPurchases_ConcurrentCreatesMintOneStepEach (500 "failed to load plan" instead of 409). That was while a second package's tests ran in parallel. The test is unrelated to this diff and passed 5/5 alone and in the full rerun, so it's a load-sensitive flake on main's plans path, not this PR. golangci-lint on the three packages: 0 issues.
• Refs #337 (not Closes) is correct: the issue's frontend 730-vs-1/720 criterion is untouched, as the body says.

Body-stale:

• none

_{Reviewer: rB13. 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 2497cdf5388c67415610b086d3652a8bf3aa9178 merged onto main (5f717aa4), together with #1260, #1261, #1263, #1264, #1269, #1271, #1273, #1308, #1416 in that order (combined tree 62127c84, 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 2497cdf5 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 2497cdf5388c67415610b086d3652a8bf3aa9178.