[IAmJSD]

This finishes the go part of the 2026-07-28 review bundle. Two earlier PRs already fixed three of the four items. This PR fixes the last one (the go half of the cross-repo Existing item) and corrects a doc comment that #605 left stale.

Closes #83

Status of each finding on current main (03f40a3)

#	Finding	Status
1	capOrUnlimited maps a nil reshape cap to math.MaxFloat64 on the auto path (providers/aws/ladder/reshape.go)	Already done in #605 (205cf99): requireCap rejects nil and unlimitedCapUSD is gone. Checked on main: reshape.go:100-135. #605 left the doc on ladder.BufferReshapeConfig saying "nil means no cap"; this PR corrects it (pkg/ladder/capability.go).
2	pkg/exchange runs any mode other than "manual" as auto	Already done in #578 (7da716a): RunAutoExchange accepts only ExchangeModeAuto / ExchangeModeManual and rejects anything else before any store or exchange call (pkg/exchange/auto.go:199-203).
3	A failed CancelPendingExchangesByOrigin is logged and ignored, which leaves two live approval tokens	Already done in #578 (7da716a): cancelStalePending returns the error and the run stops before it creates any exchange (pkg/exchange/auto.go:218-246).
4	Cross-repo: the ladder plan reports gross Existing, but Allocate sizes the gap against net (go half: expose the net figure from Allocate)	Done here (go half). The platform half is platform#243 and needs this module version first.

Root cause and fix (item 4)

Allocate sizes the gap against net existing commitment (existing minus expiring, from computeNetExisting), but it never returned that figure. The platform's assembleLadderPlan therefore adds up gross ExistingUSDPerHour from the layer states. The plan's Existing line in the approval email then doesn't reconcile with its target and gap.

• AllocateResult gains a new NetExistingUSDPerHour *big.Rat field. It holds the exact E the gap was sized against and is set on every successful result, including the target-met and baseline-unavailable holds. The platform reports existing on those holds too.
• Allocate is split into a thin outer function (validate, compute net existing, then call the inner allocate, then set the field) and the unchanged steps 2-8 in allocate. That way every return path gets the field without touching each early exit, and it stays clear of the lines that fix(ladder): stop subtracting existing commitment from residual baselines #441 rewrites (see Overlap).
• Behavior change: net existing is now computed before the baseline-unavailable hold. A snapshot whose expiring figure exceeds its existing figure, combined with a nil baseline, now returns the existing "inconsistent provider snapshot" error instead of a hold. Both outcomes buy nothing. The error is the repo's fail-loud rule for inconsistent provider data, and it means the hold can never report an existing figure built from a bad snapshot.

Verification

All of this is unit-level evidence with in-memory fixtures. No cloud APIs and no DB are involved; the change is pure computation.

Fail-before: I committed the new field without the code that sets it, then ran go test -count=1 -run 'NetExisting|NilBaselineInconsistent' ./ladder/ in pkg:

• TestAllocate_ReportsNetExisting/{allocation,target_met_hold,baseline_unavailable_hold}: FAIL, NetExistingUSDPerHour is nil
• TestAllocate_NetExistingReconcilesWithGap: FAIL, NetExistingUSDPerHour is nil
• TestAllocate_NilBaselineInconsistentSnapshot_Error: FAIL, expected an error for expiring > existing with a nil baseline, got nil

Pass-after: all 5 pass. The fixture has $10/hr existing on one layer, of which $4/hr expires, plus $2/hr on a second layer. Gross existing is $12/hr and net is $8/hr. The reconcile test checks that planned OD gap + NetExistingUSDPerHour equals the $20/hr target, which a gross figure would fail ($24 ≠ $20).

Commands (with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true):

• go build ./..., go vet ./... and go vet -tags integration ./... in each of the five modules: clean
• cd pkg && GOWORK=off go build ./... && GOWORK=off go vet ./ladder/: clean
• cd pkg && go test -count=1 ./ladder/ and the same with GOWORK=off: ok
• cd providers/aws && go test -count=1 ./ladder/ (it calls ladder.Allocate in commitment_rates_sdk_test.go): ok
• golangci-lint v2.10.1 run --allow-parallel-runners ./ladder/... in pkg: 0 issues
• gocyclo -over 10 -ignore "_test\.go" pkg/ladder/allocate.go pkg/ladder/capability.go: nothing over 10
• git merge-tree of this head against open fix(ladder): stop subtracting existing commitment from residual baselines #441, fix(gcp): fail closed on region-less storage SKUs and ambiguous SKU matches #471, fix(azure): count savings plans billed to another subscription in the duplicate check #491 and fix(azure/compute): skip scope checks on terminal VM reservations #439: all clean. I also merged fix(ladder): stop subtracting existing commitment from residual baselines #441 into a throwaway worktree and ran go vet and go test -count=1 ./ladder/ in pkg: ok

CI-equivalent run on the branch head (e669581d, up to date with go main 03f40a3; Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true). It's the only go branch in this round, so the combined tree is this branch on main:

• go test -mod=readonly -race -short -count=1 ./... passes in each module: pkg, providers/aws, providers/azure, providers/gcp.
• pkg integration tests (go test -race -tags=integration -short -count=1 ./...) pass.
• git-secrets with the repo's scripts/setup-git-secrets.sh patterns, in a fresh repo with an isolated HOME: the changed files are clean and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 68 PASS, 0 FAIL.

Overlap with our open PRs

• fix(ladder): stop subtracting existing commitment from residual baselines #441 (residual baseline, issue fix(aws/ladder): residual on-demand baseline subtracts existing commitment again and falsely holds below target #282) rewrites the gap lines in Allocate. This PR leaves those lines alone, and both merge orders are clean (checked above). One semantic point for whichever PR merges second: under fix(ladder): stop subtracting existing commitment from residual baselines #441's residual basis the gap is sized with E = 0 (existingForBasis), while NetExistingUSDPerHour still reports existing minus expiring. That is the right figure to display as existing commitment. Strictly, though, the field's doc ("the E Allocate sized the gap against") holds only on the total basis. After both merge, the doc could say so, or fix(ladder): stop subtracting existing commitment from residual baselines #441 could set the field from gapExisting. Which to do is the owner's call.
• fix(gcp): fail closed on region-less storage SKUs and ambiguous SKU matches #471, fix(azure): count savings plans billed to another subscription in the duplicate check #491 and fix(azure/compute): skip scope checks on terminal VM reservations #439 touch other packages; no overlap.

Out of scope

• The platform half of item 4 (assembleLadderPlan reports allocResult.NetExistingUSDPerHour instead of summing gross layer states) is platform#243. It needs a pkg version that includes this change.
• I did not expose per-layer net existing (the nets map). The platform plan only reports a total, so I left it internal.

Please mirror the issue's triage labels onto this PR: triaged, type/chore, effort/l, priority/p3, severity/low, urgency/eventually, impact/few.

Note: the commit message says Refs #83. The PR body uses Closes #83 because, with #578 and #605 already merged, this PR completes the go part of the issue.

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• 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.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE d516319fc2bf901acbcb665d1270ae97efd899c3

Blocking:

• none

Non-blocking:

• Semantic overlap with fix(ladder): stop subtracting existing commitment from residual baselines #441 (the body raises it too). In the merged tree, a residual-basis run sizes the gap with E = 0 but reports NetExistingUSDPerHour as existing minus expiring. Platform#243 is about to make the plan's Existing reconcile as target - existing = gap. On a residual plan it won't, so the approver sees the same kind of non-reconciling figures chore(purchase/ladder/exchange): medium/low findings from the 2026-07-28 full review [go part] #83 is about. Whichever PR merges second should either reword the field doc ("the E Allocate sized the gap against" holds only on the total basis), or add a separate "gap-sizing E" figure for platform#243 to use. Display-wise, I'd keep net existing as the shown figure and let the residual rationale explain the rest.
• pkg/ladder/allocate.go: moving computeNetExisting ahead of the nil-baseline hold means an inconsistent snapshot plus a nil baseline now errors. That also drops the reshape and maintenance holds that the hold path used to carry for that run. Fail-loud on a bad provider snapshot matches the repo rule, and nothing is bought either way, so this is fine. It's a behaviour change for platform callers that treat an Allocate error differently from a hold; the CHANGELOG says so.

Attacked:

• Every success path goes through the outer Allocate, so the field is set on allocation, target-met, baseline-unavailable and partial holds. Error paths return nil. Field order changes in AllocateResult (the new field is first), but I found no unkeyed literals and it builds in all modules.
• Fail-before 1: I replaced the result.NetExistingUSDPerHour = ... assignment with a no-op. TestAllocate_ReportsNetExisting/{allocation,target_met_hold,baseline_unavailable_hold} and TestAllocate_NetExistingReconcilesWithGap fail.
• Fail-before 2: I restored the old order, where a nil baseline skips the snapshot check. TestAllocate_NilBaselineInconsistentSnapshot_Error fails. I restored the file after both.
• Merge with fix(ladder): stop subtracting existing commitment from residual baselines #441: git merge-tree of d516319 and 3a7257b is clean. I extracted the merged pkg tree, and go vet plus go test ./ladder/ (GOWORK=off) pass.
• Ran in pkg with GOWORK=off: go build ./..., then go vet ./ladder/ with and without -tags integration. go test -race -short -count=1 ./ladder/ is ok. golangci-lint v2.10.1 reports 0 issues with and without --build-tags integration. gocyclo -over 10 on allocate.go and capability.go is silent. In workspace mode, go test ./providers/aws/ladder/... is ok.
• The capability.go doc fix matches the fix(aws/ladder): reject nil reshape payment caps instead of treating them as unlimited #605 behaviour (requireCap rejects nil). Closes #83 holds: items 1-3 are already merged via fix(exchange): fail closed on unknown mode and cancel failure #578/fix(aws/ladder): reject nil reshape payment caps instead of treating them as unlimited #605, and item 4's go half is done here. The platform half is tracked separately in platform#243.

Body-stale:

• none

_{Reviewer: rB7. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Merged origin/main (now 76fbf101) so the branch is up to date, as the ruleset requires. It was a clean merge, and the PR's diff against main is byte-identical (same patch-id), so rB7's review of d516319f carries over. Locally on the merged tree, every module passes go build, go vet and go test -race ./...: pkg, providers/aws, providers/azure, providers/gcp and ci_cd_sanity_tests.

[IAmJSD]

Local verification at 76fbf1014f55fbb18aa4f9640de91d8c6b36ab5d (main merged in; every module builds, vets and passes go test -race). CI Success and pre-commit are green on this head. Merged by IAmJSD's agent at IAmJSD's request (acting owner while the maintainers are away), under the bar IAmJSD set: an approving independent review (rB7, carried over a clean main merge with an identical diff) and green CI. Squash-merged at exactly 76fbf1014f55fbb18aa4f9640de91d8c6b36ab5d.