feat(ladder): truncate over-cap plans instead of erroring (L1) - #1366
Conversation
|
@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: 48 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 (3)
📝 WalkthroughWalkthroughChangesThe Allocate pipeline now deterministically truncates actions exceeding Action truncation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Allocate
participant buildResult
participant truncateToMaxActions
participant AllocateResult
Allocate->>buildResult: build allocation result
buildResult->>truncateToMaxActions: apply MaxActionsPerRun
truncateToMaxActions-->>buildResult: retained actions and truncation holds
buildResult->>AllocateResult: assemble actions and holds
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/ladder/allocate_test.go (1)
594-607: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the equal-gap layer tie-breaker.
The current equal-gap allocations are both dropped, so no test verifies which layer survives when they compete for one slot. Add a case with equal
GapUSDPerHourvalues andpurchaseCap=1, asserting the layer-name ordering.Also applies to: 686-699
🤖 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/allocate_test.go` around lines 594 - 607, Add a test case in the allocation test near the existing gap-priority assertions that creates two layers with equal GapUSDPerHour and purchaseCap=1, then verify exactly one allocation remains and it is selected according to the expected layer-name ordering. Ensure the assertion covers both the surviving and dropped tied layers.
🤖 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/allocate_test.go`:
- Around line 594-607: Add a test case in the allocation test near the existing
gap-priority assertions that creates two layers with equal GapUSDPerHour and
purchaseCap=1, then verify exactly one allocation remains and it is selected
according to the expected layer-name ordering. Ensure the assertion covers both
the surviving and dropped tied layers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4dad105d-fad8-4627-95d5-458f77ec4931
📒 Files selected for processing (2)
pkg/ladder/allocate.gopkg/ladder/allocate_test.go
Replace the permanent-stall error when a plan exceeds MaxActionsPerRun with deterministic truncation (B3 / Q-TRUNC). Reshapes are always retained first (risk-reducing, no new spend). Among purchase allocations the largest GapUSDPerHour are kept; the smallest are dropped. Every dropped action emits an ActionHold with a "deferred to a later run: max_actions_per_run=N reached" rationale so no money action is silently lost. Ties broken by layer name for a stable ordering. Implementation extracts truncateReshapes and truncatePurchases helpers to keep cyclomatic complexity under the project limit (gocyclo -over 10 clean).
…l the cap When reshapes alone fill or exceed MaxActionsPerRun, truncateToMaxActions previously returned nil allocations while emitting holds only for dropped reshapes, so every purchase vanished with zero audit trace. This violated the Q-TRUNC rule that every dropped money action must leave a Hold. That branch now converts all allocations to holds (truncatePurchases with purchaseCap 0), naming each dropped layer and amount. Also: document the branch invariant (unreachable in production: at most one buffer reshape and MaxActionsPerRun > 0), rename the shadowing max parameter to maxActions in the truncate helpers, and tighten tests to set-level assertions (exact surviving/held layers). Adds TestAllocate_MaxActions_ReshapesFillCap as the regression for the dropped-purchase-hold gap.
…contract PR #1362 added TestExecuteLadderRun_MaxActionsExceeded asserting the old engine contract (Allocate errors when the split exceeds MaxActionsPerRun). This branch changes that contract to deterministic truncation, so it owns the consumer-test update. Renamed to TestExecuteLadderRun_MaxActionsExceeded_TruncatesAndPersists and rewrote it to the new contract: executeLadderRun succeeds, persists a status=planned run whose plan contains exactly one surviving purchase (the larger-gap $70/hr flex allocation) plus a max_actions_per_run hold for the dropped $10/hr base allocation, TotalHourlyCommit reflects only the kept allocation, and tranches exist for it. The fixture ramp moves to a single future step (AfterDays=30) since an AfterDays=0 step becomes a BuyNow action and produces no tranche rows.
5ed812a to
b7bc832
Compare
|
Rebased onto post-#1362 main (7adacd7); no textual conflicts. Updated the MaxActionsExceeded handler test to the new truncate contract: this PR changes the engine contract (Allocate now truncates at max_actions_per_run instead of erroring), so it owns the consumer-test update. TestExecuteLadderRun_MaxActionsExceeded (added by #1362, asserting the old fail-loud behavior) is now TestExecuteLadderRun_MaxActionsExceeded_TruncatesAndPersists: executeLadderRun succeeds, persists a status=planned run with exactly one surviving purchase (the larger-gap $70/hr flex allocation), a max_actions_per_run hold for the dropped $10/hr base allocation, TotalHourlyCommit reflecting only the kept allocation, and tranches for it. Gates green: go build ./... (0), go vet ./... (0), pkg ladder tests 249 passed (0), internal/server 401 passed (0). |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Merged to main after: Fable review (BLOCK->SHIP across two rounds, including a caught-and-fixed CRITICAL silent purchase-drop when a reshape fills the action cap), CodeRabbit clean, rebase onto post-#1362 main with the MaxActionsExceeded handler test rewritten to the new truncate contract (this PR changed the engine contract so it owned the consumer-test update), and full functional CI green. MaxActionsPerRun now truncates deterministically (largest gap kept, reshapes retained, every dropped action leaves an audit Hold) instead of stalling the config forever. Unblocks L5 (in-flight tranche netting) in the automation chain. |
What
Work item L1 of the auto-laddering full-automation plan (closes gaps B3 / guardrail-stall). Changes the
MaxActionsPerRunguardrail in the ladder allocation engine from error to deterministic truncation.Today, when a plan's action count exceeds
cfg.MaxActionsPerRun,Allocatereturns an error (pkg/ladder/allocate.go). Unattended, an over-cap config then errors on every run forever: a guardrail that behaves as a permanent stall. This truncates the plan instead, deferring the excess to a later run.Behavior (per the locked Q-TRUNC decision)
GapUSDPerHourfirst is kept; the smallest are dropped until the cap is reached.ActionHoldwith amax_actions_per_runrationale naming the layer and amount: nothing vanishes untraced (no silent money-action loss).big.Ratcomparison (not float); Holds excluded from the cap count.Verification (exit 0)
go build ./...,go vet ./...clean;pkggo test ./ladder/...249 pass;gocyclo -over 10clean.Notes
Fresh-eyes review caught a CRITICAL edge in the first cut (a reshape filling the cap dropped all purchases with no audit trace); it is fixed and regression-tested. The reshapes-exceed-cap fallback is production-unreachable today (buffer layers capped at 1,
MaxActionsPerRun > 0validated) and documented as defensive-only.Part of #1336. Tracker #1333.
Summary by CodeRabbit
New Features
Documentation
Tests