[cristim]

Part of #70 (S7f). This is the platform side of the go#626 decisions:

• Q1=A: observed rates are used only to convert existing coverage.
• Q2=B: a missing observed rate falls back to the marginal rate.
• Q3=A: rates are per linked account, handled in go.

What

• executeLadderRun asks the capability for observed commitment rates when it implements ladder.ObservedCommitmentRateCapability, which cloud-commitments-go fix(history): truncation banner quotes the wrong cap and a failed class listing does not set truncated #686 added and chore(deps): bump cloud-commitments-go to 790f369f and forward the RI offering type #1796 pinned.
• New ladderConversionRates builds the rate map that converts existing coverage to on-demand dollars:
  • it starts as a copy of the marginal rates (built with make and copy, so a nil marginal map is safe);
  • each observed rate overrides the marginal one for its layer;
  • a capability without observed rates converts at marginal and returns no error;
  • an observed-rates error fails the run, wrapped as observed commitment rates: .... A missing key is a fallback, an error never is.
• Only ladderLayerStatesOnDemand uses the conversion map. Allocate and each tranche's CommitmentRate keep the marginal rate, because new purchases are bought at it.
• logObservedOverrides logs one line per layer the observed rate replaced, with both values ("marginal: none" for an observed-only layer). Nothing in the run row records the conversion rate, so this log line is the record.
• Comments are updated. The missing-rate Hold text now says neither an observed rate nor a new-purchase rate exists; its prefix, amounts and term/payment are unchanged. No frontend or E2E test asserts that sentence.

Tests (ladder_observed_rates_test.go)

Fixtures use a single flex layer and binary-exact rates (0.75 and 0.5). Cases:

• observed wins for conversion while the planner keeps marginal (existing_usd_hr 10; tranche of 7.5 at "3/4");
• an absent observed key falls back to marginal;
• a Compute layer with only an observed rate converts, and Allocate holds it with no tranche;
• a layer with neither rate keeps missing_discount_rate and the new Hold text;
• no observed capability;
• an observed-rates error fails the run;
• a nil marginal map with observed rates.

Verification (local, macOS, throttled)

• Fail-before: at main with only the new tests, 4 tests fail.
• Tests: go test ./internal/server/ and go test -tags integration ./internal/server/ -run Ladder pass. golangci-lint reports 0 issues.
• Deletion probes, each failing named tests:
  • marginal rates into the existing-coverage conversion: ObservedRateConvertsExistingPlannerKeepsMarginal, ObservedOnlyLayerConvertsButPlannerHolds;
  • observed rates into the planner: the same two;
  • swallowing the observed error: ObservedRatesErrorFailsRun;
  • failing the run when the capability is missing: 6 existing ExecuteLadderRun tests;
  • maps.Clone instead of make: LadderConversionRates_NilMarginalWithObserved.
• Not verified: a run against a live account. The ladder run is exercised here with fakes.

🤖 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 46 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: feat/70-t7-observed-rates
• File: .coderabbit.yaml
• Commit: 62c0f73
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7b45a1ef-e7b2-400b-9d26-2f203d99ec62

Commits

Reviewing files that changed from the base of the PR and between 9f98f15 and 62c0f73.

Files selected for processing: 4

• internal/server/handler_ladder.go
• internal/server/ladder_existing_units.go
• internal/server/ladder_missing_rate_hold_test.go
• internal/server/ladder_observed_rates_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]

@coderabbitai review

[coderabbitai]

Action not completed

Review rate limited.

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

[cristim]

APPROVED 62c0f73 by claude-qa-ui-3

Cold review of the full diff (money path), checked against the go#626 owner decisions.

Owner decisions

• Q1 = A (observed rates only convert existing coverage). conv from ladderConversionRates feeds only ladderLayerStatesOnDemand (handler_ladder.go, the executeLadderRun conversion). The planner still gets rates: rates, the marginal rates, so Allocate and each tranche's CommitmentRate never see an observed rate.
• Q2 = B (fall back to the marginal rate).
  • ladderConversionRates (ladder_existing_units.go) copies the marginal map, then overrides only the layers that have an observed rate. A missing observed key falls back to marginal.
  • A capability with no observed rates converts at marginal with no error.
  • An observed-rates read error fails the run. It is never treated as a fallback.
• Q3 = A (per linked account). It is handled in go. The pinned providers/aws ladder/observed_rates.go measures a.cfg.AccountID (GetSPUtilizationForAccount, GetRIFeeUtilization). The platform skips multi-account configs before a run (skipped_multi_account), so each run is single-account.

Attacks

• Missing observed rate: marginal is used. A layer with neither rate still produces a missing_discount_rate Hold with the new text.
• Zero, partial or malformed utilization:
  • go drops ratios outside (0, 1] and malformed RI hours, and ErrNoSPUtilization leaves the layer absent.
  • If a bad value got through anyway, commitmentRateRat (ladder_existing_units.go:182) rejects NaN, Inf, ≤0 and >1, and the run fails loudly.
  • The observed rate counts only the used share of the fee. With partial utilization, existing coverage converts as if fully used, which overstates coverage. A run can only under-buy (the safe direction), never over-buy.
• Observed-only layer: it converts, and Allocate holds it with no tranche because there's no marginal rate.
• Nil marginal map: make plus copy is safe.

Mutation probes (each fails named tests)

• Converting existing coverage at marginal (observed ignored) fails ObservedRateConvertsExistingPlannerKeepsMarginal and ObservedOnlyLayerConvertsButPlannerHolds.
• The planner sizing at the observed rate (rates: conv) fails the same two.
• Marginal winning over observed when both exist fails ObservedRateConvertsExistingPlannerKeepsMarginal.
• Swallowing the observed error fails ObservedRatesErrorFailsRun.
• Removing the marginal fallback fails 4 existing ExecuteLadderRun tests.

Local verification (macOS, at this SHA, throttled)

• Build and go vet -tags integration ./internal/server/ are OK.
• go test ./internal/server/ and go test -tags integration -run Ladder ./internal/server/ pass.
• golangci-lint v2.10.1 reports 0 issues on ./internal/server/....
• Not verified: a run against a live account. The fakes exercise the run path, as the PR says.

[cristim]

APPROVED 62c0f73 by claude-darling

Cold review; I did not write this change. This is a money path. The core logic matches the go#626 owner decisions, and every mutation of the observed/marginal choice is killed. Two low findings, fine as a follow-up.

Decisions (go#626, owner comment of 2026-11-04 and the design comment of 2026-11-05)

Q	Code	Matches
Q1 A: observed rate converts existing coverage only	ladder_existing_units.go:59-81 (ladderConversionRates). conv feeds only ladderLayerStatesOnDemand (handler_ladder.go:353,358). The planner and Allocate get the marginal rates (:367, :449)	yes
Q2 B: missing observed falls back to marginal, else Hold	Marginal is copied first (:65-68), and only present keys are overridden (:77-79). A layer with neither rate still goes to missing (:113-118), giving the missing_discount_rate Hold	yes
Q3 A: per linked account	go providers/aws/ladder/observed_rates.go:40,74 filters by cfg.AccountID. The platform builds one capability per config from ExternalID (handler_ladder.go:257,264) and skips other-account configs (:237-240)	yes, for current scope

The go contract is pkg/ladder/capability.go:15-22: an absent key means no observation, values are in (0,1], and the map is nil on error. Production wiring goes through NewFromConfig (factory.go:98). WithWriteSide returns the same *AWSLadder, so the observed interface survives. The Lambda and Fargate IAM already grant ce:GetSavingsPlansUtilization and ce:GetReservationUtilization.

Findings (low)

1. The account scope passed to the observed-rate call is untested.
   • Repro: replacing scope at handler_ladder.go:353 with another account ID passes all of ./internal/server.
   • Why it's low: in production validateScope (go commitments.go:42-51) rejects the mismatch loudly, and the go queries use cfg.AccountID. So no rate leaks across accounts.
   • Fix: have observedFake record and assert the scope it receives.
2. The rate-source choice isn't visible.
   • Removing the logObservedOverrides call (:357) passes every test.
   • A per-layer fallback to marginal is never logged, and go drops ErrNoSPUtilization silently (observed_rates.go:46).
   • The new log uses log.Printf, while the Hold path uses logging.Warnf (:427).
   • Suggestion: log one line per layer naming the rate source (observed or marginal), and test it.

Note: an observed-rates read error now fails the run (:354-356) instead of continuing at marginal. That's fail-loud and pinned by a test, but a Cost Explorer throttle or AccessDenied now marks the config Errored. Worth a line in the body.

Runs

• go test -count=1 ./internal/server/: exit 0.
• go test -count=1 -tags integration -run Ladder ./internal/server/: exit 0.

Mutation probes (each restored, tree clean after)

• Always marginal: killed (3 tests, including ObservedRateConvertsExistingPlannerKeepsMarginal).
• No marginal fallback: killed (7 tests).
• Observed map replaces the conversion map: killed (AbsentObservedKeyFallsBackToMarginal).
• Planner gets conv: killed (2 tests).
• conv aliases the marginal map: killed (2 tests).
• Survivors: the wrong account scope (finding 1) and the removed log (finding 2).

Checked clean

• Values that are zero, NaN, Inf or above 1 are rejected (ladder_existing_units.go:179-183).
• Go guards divide-by-zero and malformed RI hours (observed_rates.go:79-86,105-110).
• A partially used SP is converted as fully used. That errs toward under-buying, as marginal did.
• The ratio has no units, so there's no unit mismatch.
• big.Rat arithmetic: existing coverage rounds up and expiring rounds down, both toward under-buying.
• In-flight tranches net at their own stored rate.

[cristim]

Merge gate at 62c0f73:

• Approvals at this SHA: claude-qa-ui-3 and claude-darling.
• CI is green at this SHA, there are 0 unresolved threads, and the merge state is CLEAN.
• Combo re-test (local, macOS, throttled): this head merged onto current main 29b1279. go build ./..., go vet, and the unit tests pass for ./internal/server/. Integration tests (-tags integration -run 'Ladder') pass.