Repository navigation
fix(ladder): net locked tranches in on-demand units - #891
Conversation
Normalize verified on-demand and native tranche amounts at their stored rates before the existing aggregate planner. Refuse unknown provenance before building or persisting a new run. Preserve the config lock, in-flight-first read order, grace window and settlement anchor. Convert the exact sum upward at the old SDK boundary so rounding cannot understate already committed coverage. Closes #890
cristim
added
urgency/this-sprint
|
Exact-head gate: independently verified 93d4345, tree c917ea4567fb88092a834ad809e65cbf676774d9. Retained source/staged review and fresh committed native verification are recorded in the PR description. Required CI remains pending. Current-main compatibility was checked against the actual disjoint frontend and purchase-plan helper changes; no SDK pin or capacity-model change is included. |
|
Warning Review limit reached
This review includes 1 billable file and costs up to $0.25.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 36 minutes for your next included review. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 5.88% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 8 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: converting locked ladder tranches to on-demand units during netting. Linked Issues check Issue #890requires mixed-basis netting, provenance validation before planning or persistence, preserved selection boundaries, and one upward-safe OD total for the existing planner. The reviewed chang…Out of Scope Changes check The production changes remain limited to provenance-aware ladder netting and related comments. The incremental changes affect only regression-test fixture provenance and test wait handling. These chan…
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
The docstring coverage advisory is not adopted. This focused change reuses private helpers and names their behavior directly; the repository favors brief comments explaining non-obvious reasoning rather than comments that restate code. The conservative rounding reason is documented, and the actual store/planner behavior is covered by native regression and mutation proofs. Adding docstrings solely to meet this advisory percentage would add unrelated comment churn. |
|
Verified revision: 25865ee (tree 6909527a31e9e3df2deb1787e814a4d154c8d74a). The CI failures reproduced locally: the amendment fixtures omitted their known on-demand basis, and a pre-callback planner error left the concurrency test waiting indefinitely. This follow-up labels only those two fixture constructors and makes that wait fail promptly on planner error or context cancellation. Production behavior and ceiling assertions are unchanged. All normal commit hooks passed. The same independent verifier completed two clean passes on this exact revision, ran the full native PostgreSQL amendment-ceiling suite (21.735s), demonstrated prompt failure after removing the fixture provenance, restored the passing concurrency case, and exercised context cancellation (2.910s). Original production-path verification and five mutation proofs transfer because those files are byte-identical. New CI runs are being watched; merge remains gated on required checks and current findings. |
|
Verified revision: ca47cfa (tree 4c5e5c11ec29d57fd39623ca9634bf19e94564fa). The previous CI run passed the amendment cases and exposed one further settled-planner fixture that omitted its known on-demand basis. This two-line test-only correction preserves every amount, anchor, skew and status assertion; production files remain byte-identical to the originally verified implementation. The actual config planner regression groups (settled, locked netting, lock coordination and amendment ceilings) pass together locally in native PostgreSQL with the race detector (53.285s). All normal commit hooks passed. The same independent verifier completed two clean passes on this exact revision: native settled test passed (4.013s), removing the fixture provenance reproduced the intended unknown-basis runtime failure (1.459s), and restoration passed (2.706s); archived source bytes matched the commit. New CI runs are watched, and normal merge still requires green checks, current findings and protections. |
|
Merged normally as e003025 after required checks passed at reviewed ca47cfa. The current-head findings check found no inline threads; the optional CodeRabbit quota notice did not replace the exact-revision independent review. A read-only comparison of merged main confirms every changed backend/test file, dependency pin and migration is byte-identical to the verified revision; the intervening main delta is the previously inspected frontend and purchase-plan helper work. All four actual main workflows are watched. Deployment identity and terminal post-merge workflow outcomes remain pending. |
|
Post-merge verification complete at e003025: main CI 38754577274, pre-commit 38754577328, frontend sentinel 38754577440 and automatic deployment 38754577322 all completed successfully. The deployment artifact records that exact SHA as both commit and image tag in dev, and Test Deployment passed. Affected backend/test files, dependency pins and migrations match the independently verified revision. No manual deployment or provider purchase was performed. Original workflow watchers are terminal and reaped. Broader allocator adoption remains tracked in #631. |
1 participant
Locked planning currently adds raw stored amounts across different bases. A verified OD tranche of 15 plus a native tranche of 20 at stored rate 1/2 nets 35 instead of 55 OD. Normalize each selected row with its own persisted provenance before the existing aggregate planner; reject unknown units before building or persisting a run.
Keep the config lock, in-flight-first read order, scheduled grace window and settled anchor unchanged. Sum exact rationals and round upward only at the existing float boundary. SDK pins, provider capacity models and the current OD writer remain unchanged; this does not add per-layer netting or activate the newer allocator.
Verification at 93d4345 / tree c917ea4567fb88092a834ad809e65cbf676774d9: two retained independent source/staged passes and fresh independent committed verification cleared the revision. Actual PostgreSQL fixtures exercised store→pinned old planner→persist, unknown refusal, selection boundaries, read errors, config-lock concurrency and the intentional duplicate observation during settlement. Parent regressions and five independent semantic scratch mutations failed at intended assertions, then restored code passed. Affected race suites, full backend build, pinned lint and all normal commit hooks passed. Economics are synthetic fixtures; no provider purchase was performed.
Current main adds disjoint frontend work and purchase-plan write-helper extraction. The ladder paths, DB interface and module pins are unchanged, so the reviewed head is retained without rebase; required PR CI checks the merged result.
Closes #890
Summary by CodeRabbit