feat(ladder): core types and capability interfaces for commitment laddering - #1338
Conversation
… laddering New pkg/ladder package with typed enums (layers, roles, actions, modes, cadence, run/tranche statuses), LadderConfig and RampSchedule validation, LadderPlan with auditable PlannedActions and Explain() email rendering, and provider-neutral LadderCapability and LadderStore interfaces. Part of #1334.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a new ChangesLadder package contracts and validation
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Note: the red "CI - Build & Test" check is pre-existing on main (red since 2026-06-11, identical failures on main's own runs) and unrelated to this PR, which only adds an inert new package. Tracked and being fixed in #1339. All other checks (pre-commit, AWS/Azure sanity, frontend sentinel) are green on this PR. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
pkg/ladder/plan_test.go (1)
12-144: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing coverage for Term/PaymentOption on Hold/Reshape.
Given the documented invariant that Term/PaymentOption must be empty for Hold/Reshape (see plan.go review), consider adding a case like
hold with non-empty Termonce that validation is implemented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ladder/plan_test.go` around lines 12 - 144, TestPlannedActionValidate is missing coverage for the invariant that Term and PaymentOption must be empty on ActionHold and ActionReshape. Update the table-driven cases in TestPlannedActionValidate to add negative tests for non-empty Term and non-empty PaymentOption on those action types, and ensure PlannedAction.Validate enforces these fields are only allowed for ActionPurchase.pkg/ladder/plan.go (2)
32-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
DataSourcesis never surfaced inExplain().The field is documented as aiding auditability, but the approval-email renderer omits it entirely, so an approver reading
Explain()output cannot see which data feeds backed the decision.Also applies to: 157-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ladder/plan.go` around lines 32 - 34, The `DataSources` field in `Plan` is documented for auditability but is not included in `Explain()` output or the approval-email rendering path. Update the `Explain()` implementation and the approval-email renderer logic to surface `DataSources` alongside the rest of the decision details so approvers can see which feeds backed the action.
120-130: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
big.Rat.FloatString(2)for USD/hr outputFloat64()can shift exact values by a cent on halfway cases (for example,199/200renders as$0.99/hr), whileFloatString(2)preserves the exact rational and keeps the approval text deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ladder/plan.go` around lines 120 - 130, The USD/hr formatter in formatUSDPerHour is converting the rational through Float64, which can round halfway values inconsistently; update this helper to format the *big.Rat directly with FloatString(2) so approval text stays deterministic, while preserving the existing nil-as-"unknown" behavior.pkg/ladder/store.go (1)
78-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
Validate()methods forRunRecord/Tranchefor consistency with the rest of the package.Other ladder types (e.g.,
LadderConfig,PlannedActionper the PR summary) exposeValidate().RunRecord/Tranchehave invariants worth enforcing at the boundary (e.g., non-emptyID/RunID,CompletedAtonly set for terminalStatusvalues, validStatus/TrancheStatus), but currently have none.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ladder/store.go` around lines 78 - 100, Add Validate() methods for RunRecord and Tranche to match the package’s existing validation pattern (like LadderConfig and PlannedAction). Implement boundary checks for required fields such as non-empty ID/RunID, valid Status/TrancheStatus values, and consistency rules like CompletedAt only being set for terminal RunStatus values and FiredAt only being set when a tranche has actually fired. Keep the validation logic close to the RunRecord and Tranche types in store.go so callers can enforce invariants before persistence or use.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/ladder/capability.go`:
- Around line 37-46: The PurchaseLayer contract references
ErrCommitmentPurchaseNotSupported, but that sentinel is missing from package
ladder, so callers cannot rely on errors.Is. Add a package-level
ErrCommitmentPurchaseNotSupported error in ladder/capability.go (or the
appropriate package-level declarations) and make PurchaseLayer return or wrap it
for unsupported providers, keeping the documented matching behavior consistent.
In `@pkg/ladder/plan.go`:
- Around line 53-63: Validate() for PlannedAction currently only enforces nil
AmountUSDPerHour for ActionHold and ActionReshape, but it also needs to reject
any non-empty Term and PaymentOption to match the documented invariant. Update
the switch in Validate() to check those fields for both hold/reshape actions and
return a descriptive error when either is set, using the PlannedAction fields
and action type in the message. Also add or extend tests in plan_test.go to
cover hold/reshape cases with non-empty Term and PaymentOption, alongside the
existing amount validation.
In `@pkg/ladder/store.go`:
- Around line 91-119: The RunID is duplicated between Tranche.RunID and the
SaveTranches method parameter, which creates ambiguity about which value store
implementations should use. Update the LadderStore contract to have a single
source of truth by either removing Tranche.RunID from Tranche or removing the
separate runID argument from SaveTranches and validating that all tranche
entries share the same RunID. Make the change consistently in the Tranche type
and the SaveTranches signature so callers and implementations cannot diverge.
---
Nitpick comments:
In `@pkg/ladder/plan_test.go`:
- Around line 12-144: TestPlannedActionValidate is missing coverage for the
invariant that Term and PaymentOption must be empty on ActionHold and
ActionReshape. Update the table-driven cases in TestPlannedActionValidate to add
negative tests for non-empty Term and non-empty PaymentOption on those action
types, and ensure PlannedAction.Validate enforces these fields are only allowed
for ActionPurchase.
In `@pkg/ladder/plan.go`:
- Around line 32-34: The `DataSources` field in `Plan` is documented for
auditability but is not included in `Explain()` output or the approval-email
rendering path. Update the `Explain()` implementation and the approval-email
renderer logic to surface `DataSources` alongside the rest of the decision
details so approvers can see which feeds backed the action.
- Around line 120-130: The USD/hr formatter in formatUSDPerHour is converting
the rational through Float64, which can round halfway values inconsistently;
update this helper to format the *big.Rat directly with FloatString(2) so
approval text stays deterministic, while preserving the existing
nil-as-"unknown" behavior.
In `@pkg/ladder/store.go`:
- Around line 78-100: Add Validate() methods for RunRecord and Tranche to match
the package’s existing validation pattern (like LadderConfig and PlannedAction).
Implement boundary checks for required fields such as non-empty ID/RunID, valid
Status/TrancheStatus values, and consistency rules like CompletedAt only being
set for terminal RunStatus values and FiredAt only being set when a tranche has
actually fired. Keep the validation logic close to the RunRecord and Tranche
types in store.go so callers can enforce invariants before persistence or use.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0203a84-d658-4d00-b90c-bbe22679f48f
📒 Files selected for processing (6)
pkg/ladder/capability.gopkg/ladder/plan.gopkg/ladder/plan_test.gopkg/ladder/store.gopkg/ladder/types.gopkg/ladder/types_test.go
- Enforce empty Term/PaymentOption on hold and reshape actions (doc already claimed the invariant); add negative test cases. - Fix PurchaseLayer godoc to reference the existing common.ErrCommitmentPurchaseNotSupported sentinel (errors.Is). - Drop the redundant runID parameter from LadderStore.SaveTranches; Tranche.RunID is the single source of run linkage. - Add RunRecord.Validate and Tranche.Validate with RunStatus.IsTerminal and FiredAt/CompletedAt lifecycle consistency checks, plus table tests. - Surface deduplicated action DataSources in LadderPlan.Explain output. - Format money via big.Rat FloatString(2) instead of Float64 so halfway cases round half away from zero (199/200 renders $1.00/hr).
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/ladder/store.go`:
- Around line 142-163: Tranche.Validate currently misses the FireAfter field, so
a zero-value time.Time can slip through and be treated as valid. Update
Tranche.Validate in store.go to reject an unset FireAfter alongside the existing
ID, RunID, StepIndex, Status, and FiredAt checks, using the Tranche.Validate
method and FireAfter field as the key location to patch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7c8d729-406a-49c9-836c-423c1c215d71
📒 Files selected for processing (5)
pkg/ladder/capability.gopkg/ladder/plan.gopkg/ladder/plan_test.gopkg/ladder/store.gopkg/ladder/store_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/ladder/plan.go
- pkg/ladder/plan_test.go
Tranche.Validate now rejects a zero FireAfter timestamp: a zero value would make the tranche immediately eligible to fire (year-1 timestamp is always in the past), silently defeating the ramp schedule. Add zero- rejected and set-accepted test cases.
|
@coderabbitai review |
✅ Action performedReview finished.
|
NaN compares false against every bound, so exclusion-style range checks
("v <= 0 || v > 1") silently admitted NaN in RampStep.Fraction,
TargetCoveragePct, BufferFraction, BaselinePercentile, and the
MaxHourlyCommitPerRun deref. Downstream big.Rat.SetFloat64(NaN) returns
nil and panics on first use.
- Rewrite all float range checks in NaN-hostile inverted form with an
explicit math.IsNaN guard (withinExclIncl helper for the (lo, hi]
intervals).
- Guard the ramp fraction sum against NaN as defense in depth.
- Reject non-finite (NaN and +/-Inf) MaxHourlyCommitPerRun, since
SetFloat64 returns nil for Inf as well.
- Regression tests: NaN fraction in single- and multi-step schedules
(asserting the step index in the error), NaN per config field, and
infinite hourly cap.
…en email rendering - GetLayerStates doc now matches the enforced contract: explicit zeros for measured-zero money fields, nil reserved for unmeasured metrics. - SupportedLayers doc states the role-cardinality contract (exactly one flex, at most one base, at most one buffer, base+buffer merge only). - New typed enums Term (1yr/3yr) and PaymentOption (all-upfront/ partial-upfront/no-upfront) with Validate/Parse; PlannedAction adopts them and purchase validation checks enum validity instead of non-empty. - Explain() passes every interpolated free-form field through a new sanitizeLine helper (strips control chars) to prevent line spoofing in the approval email; "Actions: none" drops the speculative reason; percentile renders via %g. - RunRecord.Validate gains Scope validation and a zero-CreatedAt check; LadderPlan.Validate gains Scope validation and a zero-GeneratedAt check. - BufferReshapeConfig money caps switch *big.Rat -> *float64 for consistency with LadderConfig.MaxHourlyCommitPerRun (config-boundary floats, converted where exact math is needed). - Scope doc notes GCP validates but fails loud at layer validation until GCP layers exist.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/ladder/types.go (1)
161-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated Validate/Parse enum boilerplate.
Term/PaymentOption(and the five prior enums) each hand-roll an identicalValidate/Parse*pair. A small generic helper (e.g.,parseEnum[T ~string](s string, valid ...T) (T, error)) could collapse this to one implementation, but given the low call count and Go's preference for explicit switch statements here, this is optional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ladder/types.go` around lines 161 - 214, The Term and PaymentOption validation/parsing logic duplicates the same Validate/Parse pattern used by the earlier enum types, so consolidate it to reduce boilerplate. Add a shared generic helper such as parseEnum for string-backed enums and have Term.Validate/ParseTerm and PaymentOption.Validate/ParsePaymentOption reuse it, while keeping the existing enum names and error messages consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/ladder/types.go`:
- Around line 161-214: The Term and PaymentOption validation/parsing logic
duplicates the same Validate/Parse pattern used by the earlier enum types, so
consolidate it to reduce boilerplate. Add a shared generic helper such as
parseEnum for string-backed enums and have Term.Validate/ParseTerm and
PaymentOption.Validate/ParsePaymentOption reuse it, while keeping the existing
enum names and error messages consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e959180-827a-4f24-bd74-786c000ddd84
📒 Files selected for processing (7)
pkg/ladder/capability.gopkg/ladder/plan.gopkg/ladder/plan_test.gopkg/ladder/store.gopkg/ladder/store_test.gopkg/ladder/types.gopkg/ladder/types_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/ladder/store_test.go
- pkg/ladder/store.go
- pkg/ladder/plan.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
New
pkg/ladderpackage: the public type contract for the commitment-laddering engine. Types only plus plan rendering; no allocation algorithm, no engine orchestrator, no provider code.types.go: typed enums withValidate()/Parse*()(LayerType, LayerRole, ActionType, LadderMode, LadderCadence),Scope(typedcommon.ProviderType),LadderConfig+RampSchedulewith fail-loud boundary validation,UsageBaseline/LayerStatewith pointer fields (absent != zero), named default constants.plan.go:PlannedAction(mandatory rationale, purchase requires positive*big.Ratamount + term + payment option),LadderPlanwithValidate()and deterministicExplain()rendering for the approval email body (nil money renders as "unknown", never "$0.00").capability.go: provider-neutralLadderCapabilityinterface (list commitments, layer states, usage baseline, purchase, buffer reshape) plusBufferReshapeConfig/ReshapeSummary.store.go: narrowLadderStorecontract (SaveRun,LatestRunStartedAt,SaveTranches) withRunRecord/Trancheand typedRunStatus/TrancheStatuslifecycles.types_test.go/plan_test.go: table-driven coverage of every enum, every config boundary, action invariants, and Explain() determinism.Part of #1334 (phase 1 of #1333).
Test plan
go test ./ladder/ -count=1frompkg/).go build ./...,go vet ./ladder/,gofmt -l ladder/all clean.golangci-lint run --config ../.golangci.yml ./ladder/...: 0 issues.Summary by CodeRabbit