[cristim]

Part of #1746 (slice A, step A1c). This PR does not finish the issue. It follows #1752 and #1756.

What

• Roll-forward refusal. planstep.CheckBoundBinding refuses a plan where an enabled entry is bound (a # key, or any entry of a bound_groups plan) but has no bound_identities. That is what an older binary's full-row write leaves behind.
  • Precheck runs it. The new ErrBoundPlanMissingBinding wraps ErrUnsupportedPlan, so attach records a failed step (failStep) instead of buying by filter or retrying every tick.
  • Writes that arm or widen auto-purchase run it too, and answer 409 (in requirePlanArmGrantFor). Disarming, disabling and renaming never reach that gate, so an operator can always stop a stripped plan.
• Rollback sentinel. On a #-keyed entry, min_count: 1 is no longer treated as a filter. Bound entries carry it because older binaries refuse any min_count. Any other filter on a # key, and min_count on a legacy key, is still refused as today.
• config.IsBoundServiceKey / BoundKeySeparator name the # convention.
• Folds in the CodeRabbit nitpick from feat(api): keep the bound_groups marker across ramp edits #1756: the bound-marker test now also checks that the requested ramp shape is saved.

Why it is inert

No stored plan has a # key or the marker until slice B's create writes one. Every new check is gated on one of the two.

Tests

• New behaviour, failing on main first:
  • a marked entry without a binding is refused;
  • a # entry without a binding is refused, even with no marker;
  • both refusals are ErrUnsupportedPlan;
  • min_count: 1 on a # key is accepted;
  • arming a stripped plan gives 409 and writes nothing.
• Guards:
  • min_count: 1 on a legacy key is still refused;
  • other filters on a # key are still refused;
  • a disabled entry is not checked;
  • disarm, disable and rename of a stripped armed plan succeed.
• Mutation probes: 8 run, 8 killed. They removed the Precheck call or the API gate, ignored the marker or the # key, dropped the enabled check, removed or widened the sentinel exemption, and stopped wrapping ErrUnsupportedPlan.

Verification

On macOS, rebased on main 4c02609:

• go build ./... and go test ./internal/... are green.
• golangci-lint v2.10.1 on internal/api, internal/config and internal/planstep reports 0 issues.
• go test -tags integration (Postgres testcontainers) is green for internal/api, internal/config and internal/planstep.

pre-commit passes. cfn-lint was skipped because it can't install here; no CloudFormation files are touched.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 15 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: feat/bound-groups-a1c
• File: .coderabbit.yaml
• Commit: fe7df6d
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: f6e144d4-5ac5-4b25-8b12-d7b76f0d652f

Commits

Reviewing files that changed from the base of the PR and between 4c02609 and fe7df6d.

Files selected for processing: 7

• internal/api/auto_purchase_bound_binding_test.go
• internal/api/auto_purchase_grant.go
• internal/api/handler_plans_bound_marker_test.go
• internal/config/purchase_plan_providers.go
• internal/planstep/bound_binding_test.go
• internal/planstep/bound_groups_mode_test.go
• internal/planstep/precheck.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

Cold review: claude-ui-fix-1, verdict APPROVE at fe7df6d5c49f3788cc58bd7d58f2de57eaf62365

I checked this against #1746 and the code on main 4c026091, treating the PR body as a claim.

Rollback story (marker plus sentinel): holds

• The marker survives an older binary's write. RampSchedule.AllocationMode has existed since fix(plans): allocate marked ramps by exact step count #907 (2e7a535c, 2026-10-16). So any binary that old or newer keeps the bound_groups string on a full-row write, and refuses it in ValidateAllocationMode (internal/config/validation.go:701-712), as types.go:277-280 says. Its struct has no GroupID/BoundIdentities/per-entry cap, so the write drops only those.
• The new binary refuses that state. The marker survives, so every enabled entry of the plan is checked, whether its key is legacy or #. CheckBoundBinding refuses it (internal/planstep/precheck.go, the new function).
• The # key is a second, independent signal. It survives any write (it's a map key), so a plan is covered even with no marker.
• The sentinel. min_count: 1 on a # key is skipped as a filter only for # keys, so a legacy key with min_count: 1 is still refused (test bound_binding_test). An older binary's Precheck refuses any min_count, so after a rollback it refuses to buy instead of buying by filter.
• Execution path. The only plan-step attach is planstep.Resolve → Precheck (internal/purchase/plan_step_attach.go:39, internal/planstep/resolve.go:95), so every attach gets the refusal. It wraps ErrUnsupportedPlan, so the step is recorded as failed, not retried.

An unmarked plan is unaffected

• With no marker and no # key, CheckBoundBinding returns nil, and firstFilterSet(svc, false) is the old switch unchanged.
• sortedServiceKeys keeps the same key order, so Precheck's error text and order are the same.
• In requirePlanArmGrantFor, the new check runs before the grant check and returns nil for these plans, so the 403/grant behaviour is unchanged.

Write paths that arm or widen

• Gated, refused with 409:
  • create (validateAndGatePlan);
  • updatePlan and patchPlan when planArmEscalates is true: arm, cap raise, a new provider, or a new service key (a new # key is a new service, since DerivePlanServices returns ec2#g2).
• Not gated:
  • disarm, disable (also disablePlan in handler_purchases.go:909-930) and rename. These are intended, and covered by TestPlanDisarm_AllowsStrippedBoundPlan.

Findings (non-blocking: execution still refuses)

1. Re-enabling a disabled stripped entry on an armed plan is not refused at write time.
   • planArmEscalates compares DerivePlanServices sets, which ignore Enabled (internal/config/purchase_plan_providers.go).
   • So aws/ec2#g2 going from enabled: false to true, with g2 stripped, on an armed plan reaches UpdatePurchasePlan with no CheckBoundBinding.
   • I reproduced this with a scratch test: planArmEscalates(before, after) == false for exactly that change.
   • The PR body says writes that "arm or widen" are refused, and this one widens.
   • Precheck still refuses at attach, so nothing is bought by filter. The cost is a failed step instead of a 409 at save time.
2. Retargeting an armed stripped plan is not refused at write time either.
   • PUT /api/plans/{id}/accounts runs requireRetargetOfArmedPlan (internal/api/auto_purchase_grant.go:293-303) → requireAutoPurchaseGrantForPlan, and never requirePlanArmGrantFor. So moving the plan to more accounts skips the new check.
   • Same mitigation: Precheck refuses at attach.
   • Suggested fix for both:
     • in updatePlanGated, run CheckBoundBinding when an entry goes from disabled to enabled on a plan left armed;
     • in requireRetargetOfArmedPlan, run it when the plan is armed.
   • Disarm, disable and rename stay unaffected.
3. Note for slice B: the new code never requires the sentinel.
   • A # entry that has identities but no min_count: 1 is accepted. After a rollback, an older binary would then rely only on the marker to refuse it (which it does, see above).
   • Consider having slice B's create validation require min_count == 1 on # keys, so both defences are always present.

Verification (local, macOS, at this SHA, scratch worktree, heavy-review-2 lock)

• Fail-before, with main 4c026091's precheck.go, auto_purchase_grant.go and purchase_plan_providers.go and this PR's tests:
  • TestPrecheck_BoundBinding fails 3 subtests on main's behaviour, with a test-only stub so it compiles: the sentinel case, the marked legacy-key case, and the disabled-entry case.
  • internal/api doesn't compile against main (undefined: planstep.ErrBoundPlanMissingBinding), so the API test's fail-before is compile-level only.
  • The tree was restored and checked clean afterwards.
• Unit: go build ./... and go test ./internal/... pass.
• Integration (-tags integration, Postgres testcontainers):
  • ./internal/api/: ok (264.6s)
  • ./internal/config/: ok (108.7s)
  • ./internal/planstep/: ok
• Lint: golangci-lint v2.10.1 on ./internal/api/..., ./internal/config/... and ./internal/planstep/... reports 0 issues.
• Scratch probe: planArmEscalates returns false when an armed plan's stripped entry is re-enabled (finding 1). The probe was deleted afterwards.

Not merging.

[cristim]

Cold review: APPROVE at fe7df6d

Reviewer: claude-qa-ui-3, a non-author with no author transcript. I reviewed the full diff origin/main...fe7df6d5 (+183/-13 on 4c02609) against the design doc's fail-closed section (points 1-4) and the current write paths.

What I attacked

Every write path that could arm or widen a stripped plan. Every plan write goes through one of three paths:

• Create: validateAndGatePlan, then requirePlanArmGrant.
• PUT/PATCH: updatePlanGated, handler_plans.go:347. When planArmEscalates is true, the write reaches requirePlanArmGrantFor before the gated store write.
• disablePlan: handler_purchases.go:926. It de-escalates only.

requirePlanArmGrantFor now runs CheckBoundBinding(after) first, so off-to-on, a cap raise, and new providers or services all get a 409 before any grant lookup or store write. The 409 is a ClientError. updatePlanGated returns it directly and doesn't retry it, because it isn't errPlanMovedUnderGate. after can't be nil there, because planArmed(nil) is false.

Non-escalating writes skip the check by design, so disarm, disable and rename still work. The unit test covers all three.

Writes that escalate nothing still pass with the binding missing: re-enabling a disabled bound entry of a service the armed plan already has, or lowering the cap. Execution still refuses them, because Resolve runs Precheck (resolve.go:95) and scheduling runs validateScheduledRamp's Precheck (handler_plans.go:696). So the plan records a failed step and never buys by filter.

Rollback (marker plus sentinel).

• Today's overlay (overlayServiceConfig, plan_services_merge.go:103) keeps MinCount. Every binary before this PR refuses MinCount != 0. So the sentinel stops a rolled-back binary from buying, even after an old PUT drops the marker.
• The # key survives the overlay and the singleton-alias branch, so CheckBoundBinding catches the lost binding on roll-forward. It also catches it with the marker gone.
• The marker catches the replace path (plan_services_merge.go:59, legacy key) as long as the marker survives. That's the "marked plan, legacy key without binding" case.
• The residual (replace plus a ramp-shape change before step 1) matches the design doc and its runbook.
• The exemption applies only to MinCount == 1 on # keys, which matches design point 3. min_count: 2 on a # key, and min_count: 1 on a legacy key, are still refused.

Unmarked plans are unaffected. With no # key and no bound_groups marker:

• CheckBoundBinding returns nil.
• firstFilterSet(..., false) takes exactly the old branches.
• Sorting the keys moved into sortedServiceKeys with the same order, so the first-error choice is unchanged.

The only reordering is CheckBoundBinding before the filter loop, and that only matters for bound entries. The gate adds one nil-returning call. IsBoundServiceKey is new and only planstep reads it.

Local verification (macOS, scratch worktree at the SHA, heavy-review-2.lock)

• go build ./... ok.

• Unit: go test ./internal/api/ ./internal/config/ ./internal/planstep/ ok.

• Integration: go test -tags integration ./internal/api/ ./internal/config/ ./internal/planstep/ -count=1 ok (api 571s, config 114s, planstep 2s, Postgres via testcontainers).

• golangci-lint v2.10.1 (CI toolchain go1.26.10): 0 issues.

• CI on the SHA is green.

• Mutation probes: not executed. The fleet paused all builds and tests for low disk before the probe run started. I mapped each probe to the test that must catch it, by reading the tests:

  • removing the gate's CheckBoundBinding: TestPlanArm_RefusesStrippedBoundPlan;
  • 409 changed to 403: the same test;
  • ignoring the marker: "marked plan, legacy key without binding";
  • ignoring # keys: "#-keyed entry without marker or binding";
  • dropping the Enabled guard: "disabled entry without binding is not checked";
  • exempting any min_count: "sentinel other than 1";
  • dropping the exemption: "marked plan with bindings and the sentinel";
  • Precheck skipping the binding check: every missing case.

  Each maps to a test that asserts the opposite outcome. That's a reading, not an execution.

Findings (non-blocking)

1. The retarget path skips the binding check. PUT /api/plans/{id}/accounts on an armed plan (requireRetargetOfArmedPlan, auto_purchase_grant.go:293) calls requireAutoPurchaseGrantForPlan directly. So moving an armed, stripped bound plan to new accounts doesn't get the 409. Precheck still refuses at execution, so nothing is bought. It's only a gap in consistency with "arming or widening writes get a 409". A one-line planstep.CheckBoundBinding(plan) there, plus a test, would close it here or in a follow-up slice.
2. The ordering hazard for the slice that writes bound plans. Resolve doesn't read BoundIdentities yet: resolveService sizes every rec of the service. Before this PR, the sentinel min_count: 1 refused such a plan. After it, a bound entry with identities and the sentinel passes Precheck, and resolution would buy every rec for that service in the plan's accounts. It isn't reachable today: buildServiceConfig (types.go:944) can't produce # keys, min_count or bound_identities, and nothing else writes them. The slice that first writes bound plans must also make Resolve honor BoundIdentities, either in the same PR or after it.
3. Bound keys and service derivation (later slice, fails closed). DerivePlanServices turns aws/ec2#g1 into the service ec2#g1. A services-scoped grant would then refuse to arm, and a second group reads as a new service, which is an escalation. Both fail closed. Worth stripping the # suffix when the writer slice lands.

No blocking findings.

[cristim]

Thanks @ui-fix-1 and @qa-ui-3. The non-blocking notes are folded into the next slice-A step, A1d, so the approvals at fe7df6d stay valid:

• Write-time 409 on two more paths (retarget, raised by both reviewers; re-enable, raised by ui-fix-1).
  • Retargeting an armed plan that lost its binding (PUT /api/plans/{id}/accounts, via requireRetargetOfArmedPlan) now gets the 409 too.
  • So does re-enabling an entry on an armed plan, which planArmEscalates can't see because DerivePlanServices ignores Enabled.
  • Both are covered by tests and mutation probes. Precheck still refuses either case at execution, as before.
• DerivePlanServices keeps the # suffix: planned step A2 cuts the key at #.
• Resolve must honour BoundIdentities before the first bound write: the slice B plan now has a hard gate. B doesn't open until A3 (Resolve restricted to bound identities) is merged and deployed.
• The min_count: 1 sentinel is required on # keys: slice B's create validation will require it.

The merge is waiting for the fleet's disk pause to lift, so the combo re-test on current main can run.

[cristim]

Merge gate at fe7df6d: combo of this head merged onto current main (d5dc240, which adds #1751; that one touches only .github and terraform). go build ./... and go test ./internal/... pass, and go test -tags integration passes for internal/api, internal/config and internal/planstep (macOS). 2 approvals (ui-fix-1, qa-ui-3), CI green, 0 open threads.