feat(planstep): size ramp steps by the cumulative target

[cristim]

Part of #631 (task C1). Builds on C0 (#860) and C2 (#861).

Scope

planstep.Resolve now sizes a step by the cumulative target instead of a share of what is still shown:

• target = floor(base x coverage x CumulativePercent(step) / 10000), buy = clamp(target - committed, 0, shown).
• base is the frozen baseline entry; at step 1 it is the shown count (post-suppression, never above demand). Identities without a baseline at step > 1 are skipped (reported in Result.Skipped, under-buy).
• step, baseline and committed are parameters; no extra store reads. Result.Baseline (step 1 only) records every matched count identity with shown > 0, including recs that size to 0.
• New ErrTargetMet, decided from a non-empty baseline plus the ledger only, never from shown (item 80 G1). A hidden identity with a gap is ErrNoMatch; an empty step-1 read is ErrNoMatch and returns no baseline (G2).
• Savings Plans keep their per-step sizing (Q7 / C5 is separate). Q4: no forced 100% on the last step.

Not in this PR

Attach wiring, the no-op completion path and its marker/note (C3, G3, G4), SP scope-out (C5), D1 (#859).

Compatibility

The production resolver (planstepResolver) calls Resolve(..., step=1, nil, nil), which is exactly the old formula (immediate plans are already forced to 100 by Precheck), so behavior is unchanged until C3.

Evidence (fixture/mock, in-memory fakes; not real-scenario)

go build ./..., go vet, go test ./internal/planstep ./internal/purchase pass. New tests: weekly-25 cov-80 -> 20,20,20,20 (old formula 20,16,12,10); 3x40 -> 32,32,16; committed >= target -> ErrTargetMet; lagging shown (suppressions expired, shown regrown to 100) still buys exactly the target, total 80; manual purchase clamps to shown; shown=0; k x pps > 100; floor rounding; G1, G2, baseline-with-zero-size, skipped identities.
Mutation probes (9 on the money lines): 6 killed; 3 survived (empty-baseline guard is equivalent to judged > 0; step-1 base.Count > 0 and step-1 shown+committed need a non-empty ledger at step 1, which C3 will not pass). Non-blocking.

Note for C3

At step 1, when every target floors to 0, Resolve returns ErrNoMatch together with a non-nil Result.Baseline. C3 must freeze that baseline on this path, or the ramp strands at step 2.

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 8 billable files and costs up to $2.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 27 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 62 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: 02165097-b19b-4295-a4e1-e2a79c75a193

📥 Commits

Reviewing files that changed from the base of the PR and between 2c91411 and ff8f248.

📒 Files selected for processing: 8

• internal/api/handler_purchases_plan_step_approve_test.go
• internal/planstep/precheck_test.go
• internal/planstep/resolve.go
• internal/planstep/resolve_cumulative_test.go
• internal/planstep/resolve_test.go
• internal/purchase/plan_step_attach.go
• internal/purchase/plan_step_proofs_test.go
• internal/purchase/planstep_azure_pricing_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]

Gate verdict: CHANGES NEEDED (blocker: build break against main), reviewed head 4c66c36

Blocker. The PR is BEHIND main by #859 (2c91411), and #859 added a caller of the old signature: internal/api/handler_purchases_plan_step_approve_test.go:60 calls planstep.Resolve(ctx, src, store, plan, accts) and indexes the result as a slice (:63, :67). Git merges cleanly, but the build does not. CI on this PR (run 38484175260, merge ref) fails Unit Tests and Lint with not enough arguments in call to planstep.Resolve at :60:161, cannot index recs (variable of struct type planstep.Result) at :63:15 and :67:35. I reproduced it locally with git merge-tree --write-tree origin/main 4c66c364 then go vet ./internal/api/. Fix: merge main (normal merge), then adapt that test to res, err := planstep.Resolve(..., 1, nil, nil); recs := res.Recs, the same way plan_step_proofs_test.go was adapted. After that the gate has to run again at the new head.

Review of the PR's own code (no defects found; this carries over if the only change is the merge plus the adapter above):

1. Step-1 equivalence, proven with a differential test. I put the origin/main (bdf1735) planstep package into a scratch copy as planstepold. Then I compared planstepold.Resolve(...) with the new Resolve(..., 1, nil, nil) on 80,925 cases. Ramp types covered: "", weekly, monthly, custom, immediate. Coverage values: -5..100.5. Percent per step: -1..250. Shown counts: 0..2^40, with stored counts both above and below shown. The cases also included multi-account EC2 with one account out of scope and one hidden identity, Savings Plans hourly values (0.004, 0.05, 1.234, 99999.99, 0, not JSON), and empty reads. For each case I compared recs with DeepEqual, the error string, and errors.Is for ErrNoMatch and ErrUnsupportedPlan, and checked that ErrTargetMet never appears. Result: 0 differences. To check that the harness can detect a change, I mutated the old floor to round, and it reported 11,595 differences. Equivalence holds because Precheck (precheck.go:35) forces percent_per_step=100 for immediate ramps, validateInputs (resolve.go:259) bounds percent_per_step to (0,100] so min(100, 1*pps) == pps exactly, the float evaluation order matches, and min(gap, shown) never binds at step 1.
2. Edge cases. Each of these passed in a scratch test:
   • committed above target on one identity while another has a gap: only the gap is bought (15), never a negative buy.
   • demand grows to 150 after step 1: the frozen base still buys 20 per step.
   • step 9 of a 25% ramp: the 100% cap holds and the step returns ErrTargetMet.
   • coverage 0 or negative: an error, never ErrTargetMet.
   • a baseline that only covers a service the plan no longer has: ErrNoMatch (judged=0).
   • a fan-out duplicate across two plan services: an error with empty recs.
   • a 3x33.4% ramp on base 3: total bought is exactly 3.
   • overflow: no int64 multiplication happens. The count is converted to float64 before it is multiplied by coverage*cum (at most 1e4) and divided by 1e4, so the target is never above base. That is the same behaviour as before.
3. Lagging shown: TestCumulativeLaggingShownDoesNotOverBuy holds (20,20,20,20,0,0).
4. G1/G2: a hidden identity with a gap returns ErrNoMatch. So does an identity whose account was removed from the target set while it still has a gap. An empty read at step 1 returns ErrNoMatch with a nil baseline. ErrTargetMet only comes from a non-empty baseline where every judged identity has committed >= target, and shown is never read for that decision.
5. Callers: plan_step_attach.go:38 (production, 1, nil, nil) and two purchase tests. The adapted tests keep the same assertions (Count 2 and the pricing refusal). The only other caller is the new one on main named above.
6. Mutation probes on resolve.go (14 probes, my own run): 12 killed, 1 equivalent, 1 survivor.
   • Killed: the shown clamp, target-committed, the 100 cap, base taken from shown, floor changed to round, the step off-by-one, judged>0, > changed to >=, ErrTargetMet changed to ErrNoMatch, freezing at first sight, keeping an empty baseline, and target-met computed from the clamped buy.
   • Equivalent: removing the len(r.baseline)==0 guard. judged > 0 already covers it.
   • Surviving (non-blocking follow-up): base.Count > 0 changed to >= 0 in countToBuy. With that mutant, a shown rec with Count 0 at step 1 would freeze a 0 baseline, and later steps would see it as target-met. Suggest that TestCumulativeShownZero also assert res.Baseline == nil.

Non-blocking notes for C3:

• At step 1, a non-empty baseline whose floored target is 0 currently returns ErrNoMatch together with a baseline (TestCumulativeFlooredToZeroTargetIsLegitimateNoop). This matches the old production behaviour. C3 has to decide whether to freeze the baseline on that ErrNoMatch. If it does not, step 2 skips every identity and the ramp strands, so it under-buys but fails visibly.
• A plan that mixes a met count service with a hidden SP rec returns ErrTargetMet. SP keeps per-step sizing until C5.

Local evidence (macOS, GOTOOLCHAIN=go1.26.9, git archive of 4c66c36): go build ./... ok. go vet and go test on planstep, purchase, config and api: ok at the head. The fixture-based differential and mutation runs are labelled as fixture evidence. I did not run integration tests because this head has to change anyway.

[cristim]

Gate re-run at ff8f248: review CLEAN, NOT merged yet

What changed since 4c66c36, checked with git diff: the branch now sits on top of main 2c91411 (#859), plus two test edits:

• internal/api/handler_purchases_plan_step_approve_test.go:60 now calls Resolve(..., 1, nil, nil) and uses res.Recs. Its assertions are unchanged.
• TestCumulativeShownZero now also asserts res.Baseline == nil, which kills the mutant that survived in my first pass.

resolve.go and internal/purchase are byte-identical to the head I reviewed before, so the earlier review still applies.

Evidence at this exact head (macOS, GOTOOLCHAIN=go1.26.9, git archive):

• Differential test, old Resolve (bdf1735) against the new Resolve(step 1, nil, nil): 80,925 cases, 0 differences. This is fixture evidence.
• go build ./... ok.
• go vet and go test on internal/api, internal/planstep, internal/purchase and internal/config: all ok.
• CI is green on this SHA, including Unit Tests, Lint, Integration Tests and E2E. mergeStateStatus is CLEAN.

Not merged, for two reasons:

• Local -tags integration for internal/purchase did not run, because the local Docker daemon hung and testcontainers timed out at 600s. internal/planstep integration passed. CI Integration Tests are green at this SHA.
• The merge step itself was refused by the session's permission check.

A human or the coordinator needs to merge, or authorize the merge.