fix(ladder): convert amend ceiling amounts by the amended tranche's basis

[cristim]

Summary

The ladder amend ceiling now converts the amended tranche's old and new amounts into on-demand-equivalent (OD) dollars using that tranche's own amount_basis and commitment_rate, and compares against the OD ceiling exactly in big.Rat.

Why

The ceiling is LEAST(target, baseline*coverage) - existing, which is in OD. Once tranches are stored in commitment dollars (after the go bump, cloud-commitments-go#349), comparing a raw commitment amount against it would let an amend raise exceed target coverage by 1/rate. The independent plan review found this. Fixing it before any commitment writer exists keeps main safe.

Changes

• checkLadderConfigCeiling nets in-flight and settled rows per row in OD, building on fix(ladder): net in-flight tranches per row basis in on-demand-equivalent dollars #968. It converts old/new by the amended row's basis and rate, and compares exactly against the ceiling in micros.
• Amend path only: a NULL basis (rows from before migration 000121) is assumed OD, so today's amend behaviour is preserved. feat(ladder): preserve tranche amount provenance #886 deliberately left those rows unknown, so the code says "assumed". Netting still fails loud on NULL.
• known_issues: until legacy NULL and OD tranches are made non-executable, nothing stops a future executor from claiming one (also noted on fix(ladder): existing Savings Plan coverage is commitment dollars but the target is on-demand-equivalent #946).

Verification

• Fail-before: on origin/main, raising a rate-0.5 commitment row beyond the OD ceiling is allowed ("expected error, got nil"). This PR refuses it.
• Further tests:
  • the raise is allowed when its OD is within the ceiling;
  • NULL-basis amend behaves as today;
  • checkLadderRunTotal on a single-basis run.
• Mutation: dropping the old/new conversion is killed by TestLadderAmendCeiling_RaiseOnCommitmentRowNetsInOnDemand.
• go build ./... and go vet -tags integration ./... are clean.
• -run 'LadderAmend|LadderNetting' integration tests pass against a native local PostgreSQL 17 on loopback, using the same overlay as fix(ladder): net in-flight tranches per row basis in on-demand-equivalent dollars #968.
• Local golangci-lint could not run (toolchain mismatch), so CI lint is authoritative.

Stack: base is #968 (fix/ladder-basis-aware-netting). This PR retargets to main after #968 merges. Refs #799, refs LeanerCloud/cloud-commitments-go#349.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 4 billable files and costs up to $1.00.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

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

Or wait 55 minutes for your next included review.

Check out review usage here.

View limit details

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

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 5a7406ba-0966-4364-b788-91aad35c1ea7

📥 Commits

Reviewing files that changed from the base of the PR and between f064294 and 2952deb.

📒 Files selected for processing: 4

• internal/config/store_postgres_ladder_amend.go
• internal/config/store_postgres_ladder_amend_ceiling.go
• internal/config/store_postgres_ladder_basis_integration_test.go
• known_issues/05_config_store_postgres.md

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

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

[cristim]

Automated review of 2c390a1: no blocking findings and CI is green. Ready to merge.
The amend ceiling now converts the amended row's old and new amounts by that row's own basis and rate, using the existing exact-rational helpers. I found no correctness, security or scope problems, and CI is green. The three new integration tests cover commitment-basis raises, NULL-basis rows and run totals.

• (optional) internal/config/store_postgres_ladder_amend_ceiling.go:64: The ceiling is still rounded to micro-dollars through float64 (toLadderMicros) while the committed total is now exact. This is harmless, since the ceiling is already truncated to 6 decimals in SQL, but reading it as a numeric string would be cleaner.
• (optional) known_issues/05_config_store_postgres.md:225: The new known-issue entry is unrelated to the netting fix, though it is a reasonable record of the NULL-basis assumption. Check that it references the actual tracking issue number.

Posted by a read-only review sweep. It stays silent on this commit and re-reviews only after a new push.

[cristim]

Independent review verdict (ladder stack #968 → #969 → #972): CLEAN

• fix(ladder): net in-flight tranches per row basis in on-demand-equivalent dollars #968 CLEAN at 201b877
• fix(ladder): convert amend ceiling amounts by the amended tranche's basis #969 CLEAN at 2c390a1
• fix(ladder): show timeline amounts in on-demand-equivalent dollars per row basis #972 CLEAN at c0ae02f

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief, covering each PR's own diff and the whole stack against main. The PR bodies were treated as claims. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Non-blocking notes (follow-ups before the go bump):

• NULL basis is handled inconsistently across the stack.
  • Netting (nettingOnDemand, internal/config/ladder_provenance.go) fails loud on a NULL basis.
  • The amend ceiling (assumeOnDemandIfUnknown) and the fix(ladder): show timeline amounts in on-demand-equivalent dollars per row basis #972 timeline (ladderTimelineODMicro) treat NULL as OD.
  • This is not an understatement path. The timeline is display-only, and an amend can only succeed on a config whose next planning run would fail loud. NULL basis with a non-NULL rate fails loud in all three.
• store_postgres_ladder_amend.go:155 checkLadderRunTotal sums native amount_usd_hr. That is correct only while a run holds a single basis, which one Allocate call guarantees today. After the bump, a mixed-basis run would be silently wrong.
• fix(ladder): show timeline amounts in on-demand-equivalent dollars per row basis #972: the scope total runs an unbounded jsonb_agg over every in-scope tranche on each page request. It is fine at current volume and worth watching for performance.
• fix(ladder): convert amend ceiling amounts by the amended tranche's basis #969 adds a known-issues note that a future executor could claim a legacy OD row, tracked on fix(ladder): existing Savings Plan coverage is commitment dollars but the target is on-demand-equivalent #946.

What was attacked and how (local macOS):

• OD direction: checked against go at the pinned module (pkg@v0.0.0-20261015143912-740dfbd72456). ladder/allocate_units.go:84 does od = c / r, and allocate.go:36-44 defines CommitmentRates as commitment dollars per OD dollar, in (0,1]. The platform's total.Quo(total, rate) uses the same direction.
• Rounding never understates:
  • fix(ladder): net in-flight tranches per row basis in on-demand-equivalent dollars #968: LadderInFlightRow rounds the commitment up and the rate down. The exact big.Rat sum is then rounded up via nettingFloat.
  • fix(ladder): show timeline amounts in on-demand-equivalent dollars per row basis #972: od_usd_hr is rounded up to micros per row, and the total is the sum of the rounded rows from the same SQL statement that pages them.
  • Legacy rate-1 rows are bit-identical (TestLadderNetting_LegacyOnlyRowsKeepTheExactTotalBitForBit).
• fix(ladder): convert amend ceiling amounts by the amended tranche's basis #969 units: the old and new amounts are converted with the amended row's own basis and rate, then compared exactly against the micros ceiling. The UPDATE still writes native amounts, so OD is never written into a commitment column.
• fix(ladder): show timeline amounts in on-demand-equivalent dollars per row basis #972 frontend: the chart, table, cumulative and selection total use OD, while the editor and amend use native amounts. The od_usd_hr fallback applies only when the field is absent (an older API, all OD), so deploy skew is safe. A malformed rate gives ErrLadderTimelineRate, which maps to a 500 with a fixed message and no fallback.
• Deleted GetInFlightLadderCommitUSDHr: it has no callers left in the repo, with all build tags checked.
• Tests:
  • At each head, go build ./... && go vet ./... && go test -count=1 ./internal/config/... ./internal/api/... ./internal/server/... ./internal/purchase/... passes.
  • Integration at fix(ladder): show timeline amounts in on-demand-equivalent dollars per row basis #972 (Docker Postgres): go test -tags integration -run Ladder -v ./internal/config/ gave 79 PASS, 0 FAIL. The one SKIP, TestPostgresStoreDB_LadderArming, is unrelated. The mixed-basis timeline and amend-ceiling integration tests ran and passed.
  • Frontend at fix(ladder): show timeline amounts in on-demand-equivalent dollars per row basis #972: npx jest ladder passed 3330 tests (1 skipped), and npx tsc --noEmit is clean.
• Mutation probes (all killed): Quo→Mul (fix(ladder): net in-flight tranches per row basis in on-demand-equivalent dollars #968), dropping the netting round-up (fix(ladder): net in-flight tranches per row basis in on-demand-equivalent dollars #968), dropping the timeline round-up (fix(ladder): show timeline amounts in on-demand-equivalent dollars per row basis #972), and converting the amend amount without the row's basis (fix(ladder): convert amend ceiling amounts by the amended tranche's basis #969).

Gate: labels are set on each PR (they reference #799, with no closing issue). CI must be green at each head. Merge order is #968, then #969 rebased onto main, then #972 rebased onto main. Each rebase needs an empty git range-diff of the PR's own commits to keep this verdict.

[cristim]

Independent review at 2c390a1

Verdict: no actionable findings in the code of this PR (it inherits the lint blocker and test-strength gaps from #968, reviewed at 201b877, which must land first).

Observations

1. Info: the raise test $1::numeric > $2::numeric is native, which is correct because old and new are the same row (same basis and rate, monotonic). Equal amounts pass without a ceiling check, as before.
2. Info: checkLadderRunTotal stays native; correct for a single-basis run, which is what writers produce. Tested on a commitment-basis run. A mixed-basis run would need a conversion; no writer produces one.
3. Info: stale comment names settledLadderCommit in store_postgres_ladder_lock.go (ladderSettleAnchor doc) and store_postgres_ladder_settled_integration_test.go:24; the function is now settledLadderRows.

Attacked

• Amend NULL handling: assumeOnDemandIfUnknown applies only when both basis and rate are NULL; a rate without basis stays fail-loud (unit test). Only the amend path passes assumeOD=true; netting passes false. Mutant that disables the assumption is killed; mutant that applies it in netting is killed.
• Conversion: old and new amounts both convert by the amended row's own basis and rate; mutants leaving the old or the new amount unconverted are both killed, as is flipping the old-amount sign.
• Ceiling comparison: exact big.Rat against the ceiling in micros; mutant > to >= killed (a test amends to exactly the ceiling and passes). Raise detection > to >= killed. Dropping settled rows from the amend sum killed.
• Overflow guard checks committed, old OD and new OD individually against 1e12 before subtraction; rates are bounded to (0,1] so no divide-by-zero.
• Transaction and locking: queries run on the same tx as the amend, under the config lock; the in-flight read precedes the settled read as in planning; cancelOverdueLadderTranchesTx still runs first with the same now.
• Legacy behaviour: for six-digit amounts the exact Rat comparison equals the old micros comparison.
• Scope: 4 files, +85/-30, within the 400-line cap. gocyclo max 10.

Mutation probes

Killed: NULL assumption disabled; netting assumes NULL; old amount unconverted; new amount unconverted; ceiling >=; raise >=; drop settled in amend; old-OD sign. No survivor in this PR's own code. (Survivors from #968 are listed there.)

Local verification (native local PostgreSQL 17 integration with synthetic data)

At this SHA in a fresh detached scratch worktree: GOWORK=off go build ./... OK; go vet -tags integration ./... OK; unit tests for config, server, mocks, analytics pass; -tags integration -run Ladder for internal/config, internal/purchase, internal/server pass on my own native PostgreSQL 17 cluster with a go test -overlay helper stub. Not real-account verification. CI lint is authoritative.

[cristim]

Rebase + delta: verdict carried to the new heads

The stack was rebased onto main (picks up #960) by claude-cc-impl.

Range-diffs against the reviewed heads:

#968: 1: 201b877d = 1: f838030c fix(ladder): net in-flight tranches per row basis ...
      -: -------- > 2: f4a286af style(ladder): keep the unparam suppression on the builder func line
#969: 1: 2c390a18 = 1: 6cc1b901
#972: 1: c0ae02f3 = 1: 57c3dd56

Delta review of f4a286a (same independent reviewer): CLEAN.

• The commit is +2/-2 in internal/config/store_postgres_ladder_lock_integration_test.go. It moves //nolint:unparam // matches LadderPlanBuilder onto the func-literal line, where unparam reports. That is justified, because the closure must keep the LadderPlanBuilder signature.
• git diff f838030c f4a286af shows nothing else changed.
• golangci-lint v2.10.1 (the CI pin) with --build-tags integration ./internal/config/... reports 0 issues.

The CLEAN stack verdict above therefore covers #968 f4a286a, #969 6cc1b90 and #972 57c3dd5. Merge order is #968, then #969, then #972, each when its CI is green. (claude-cc-reviewer)

[cristim]

Retargeted to main after #968 merged (f064294) and rebased with git rebase --onto origin/main f4a286af. git range-diff: 1: 6cc1b901 = 1: 2952deb0, so the commit is identical. go build ./... and go vet ./internal/... pass on the rebased tree. The CLEAN verdict carries over to 2952deb. Merge waits on CI at this head. (claude-cc-reviewer)