[cristim]

Part of #1746 (slice A). This is the first of two stacked PRs; the second, A6a and A6b (show bound groups and bound_count in plan responses), follows when a slot frees.

Builds on #1774 (bound plan validation, merged).

What

• A2+A3: resolve a bound group from its selected recommendations (internal/planstep).

  • DerivePlanServices drops a bound entry's #<group_id> suffix, so the arming checks and grant constraints see ec2 for aws/ec2#g1.
  • resolveService keeps only the group's own bound identities from the strict read, before the existing plan-match, suppression and account checks. A matching recommendation that wasn't selected is never bought and never enters the frozen baseline.
  • Bound identities the strict read didn't return come back as Result.MissingBound and are logged at warn level. If some are missing, the rest are sized; if all are missing, the existing ErrNoMatch grace path runs.
  • On a bound plan, planServiceOf matches a baselined identity to the enabled group that bound it, so targetMet judges each group against its own coverage, including groups that differ only in payment. Unmarked plans keep today's first-entry match, with a regression test for the payment split.

• A5: re-check the arming grant when a bound plan's groups widen (internal/api). When the stored or new plan carries the bound_groups marker, these writes to an armed plan count as escalations, so the execute:purchases grant constraints are checked again:

  • a new group;
  • a bound set that isn't a subset of the old one;
  • a group cap raised or removed (conservative: neither resolve.go sizing nor anything in this PR enforces the group cap yet; the per-group step cap lands with the A4 slice. Until then this escalation guards a limit that is not applied, which fails closed);
  • a group's term or payment changing;
  • a disabled group re-enabled;
  • the marker removed.

  Disabled groups are skipped: a disabled group buys nothing, so adding or widening one is not an escalation until it is enabled. Arming a disarmed bound plan is always an escalation, even when the write narrows it.

  Unmarked plans keep today's rules exactly, and a guard test pins that.

Tests

Unit tests for each rule above, including the unmarked-plan guards. The two feature commits ran every pre-commit hook available when they were written (cfn-lint was skipped by name before #1781; no CloudFormation is touched). The third commit (a gocritic rangeValCopy fix found after the rebase) ran every hook.

Verification

Local, macOS, on the branch rebased onto main 71b5e29:

• go build ./... OK.
• go test -p 2 on internal/planstep, internal/api, internal/purchase and internal/config: all pass.
• golangci-lint v2.10.1 on ./...: 0 issues.

This touches no money-moving call: A2+A3 narrow which recommendations a bound group may size, and A5 only adds escalation cases to the existing grant re-check. Integration suites run in CI.

🤖 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 31 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/1746-bound-groups-a
• File: .coderabbit.yaml
• Commit: 56c54f8
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: c87859c5-87e9-4403-870d-d26ec7dcbda7

Commits

Reviewing files that changed from the base of the PR and between 71b5e29 and 56c54f8.

Files selected for processing: 7

• internal/api/auto_purchase_bound_escalation.go
• internal/api/auto_purchase_bound_escalation_test.go
• internal/api/auto_purchase_grant.go
• internal/config/purchase_plan_providers.go
• internal/config/purchase_plan_services_bound_test.go
• internal/planstep/bound_resolve_test.go
• internal/planstep/resolve.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]

CHANGES REQUESTED fea2b01 by claude-ui-fix-1

The production code looks correct. The one blocking item is a missing test on the money path: the probe "let narrowing re-arm" survived. The other findings are non-blocking.

Findings

1. Medium (test gap, blocking per the gate). internal/api/auto_purchase_grant.go:55-63, internal/api/auto_purchase_bound_escalation_test.go:32-67.

   • The probe added if isBoundPlan(before) && isBoundPlan(after) && !boundGroupsEscalate(before, after) { return false } right after the !planArmed(after) check. Every test in TestPlanArm|TestPlanDisarm|TestAutoPurchase|TestPatchPlan|TestUpdatePlan|TestRetarget|TestRunPlanned still passed.
   • So a bound plan that goes from disarmed to armed with narrowed or unchanged groups could skip the grant, and nothing would notice. Every TestPlanArmEscalates_BoundGroups case starts from an already armed before.
   • Fix: add cases where before is a disarmed bound plan and after is the same plan armed, once unchanged and once narrowed (e.g. p.Enabled=false or MaxStepCommitment=nil on before). Assert true.
   • Refutation attempt: today's code is safe, because !planArmed(before) returns first. But the gate requires each probe to fail a named test, and this one did not.

2. Low (over-strict, fails closed). auto_purchase_bound_escalation.go:29-33, 44-62.

   • boundGroupsEscalate and groupWidens ignore next.Enabled. Adding a disabled group, or editing the identities, cap, term or payment of a disabled group, counts as an escalation and needs execute:purchases.
   • It is not a safety issue. Optional fix: skip groups where !next.Enabled. Re-enabling one is already caught at line 45.

3. Info. auto_purchase_bound_escalation_test.go:41. Raising a group's coverage is deliberately not an escalation, which matches today's unmarked rules where the cap is the spend authority. The group cap (MaxStepCommitment) is not enforced in resolve.go by this PR, so its raised/removed escalation is purely conservative. Worth a sentence in the PR body.

What I attacked

• Reuse of feat(config): validate bound plan groups and refuse more stripped-plan writes #1774: no parallel copy.
  • Bound plans reach Resolve only through Precheck → CheckBoundBinding (resolve.go:100, precheck.go:58-67). An enabled bound entry with no identities is refused before the new boundSet sees it, so its nil "admit all" branch (resolve.go:327) is unreachable for bound entries.
  • Key and group uniqueness come from validateBoundGroups and checkGroupKey (validation_bound_groups.go:32-109), so EffectiveGroupID always equals the key suffix.
• Widen triggers:
  • All plan writes use planArmEscalates: PUT handler_plans.go:361, create/PATCH auto_purchase_grant.go:205.
  • Account retarget is gated separately (auto_purchase_grant.go:290).
  • Recommendation churn cannot widen a bound set: boundSet.filter (resolve.go:340) keeps only bound identity keys.
  • Concurrent plan edits are covered by the existing optimistic check plus UpdatePurchasePlanGated (handler_plans.go:372).
  • Marker removal and switching bound to unbound both escalate.
  • Switching unmarked to marked escalates because the group IDs differ, which is conservative.
• Narrowing: narrowing the bound set, lowering or adding a group cap, and disabling a group are all false (not escalations), and the arm/disarm order is unchanged. The test gap is finding 1.
• Tenant/account scope:
  • The identity key includes the account (recommendation_identity.go:111-115).
  • Resolve filters recommendations by the plan's accountIDs before the bound filter (resolve.go:284).
  • The grant is checked against the plan's accounts (planArmAccountIDs).
  • A bound identity in another account shows up only as MissingBound, never as a purchase.
• targetMet: boundServiceOf judges each identity only against its own enabled group (resolve.go:225-233).
• Duplication: canonicalPayment is a small wrapper over NormalizePaymentOption, which is acceptable.

Probe results (scratch worktree, each reverted; tree clean afterwards)

Probe	Result	Failing test
M1 widen check always passes	KILLED	TestPlanArmEscalates_BoundGroups/{new_group, added_bound_identity, bound_to_unbound, raised_group_cap, removed_group_cap}
M2 skip arming re-check	KILLED	same as M1
M3 narrowing re-arms	SURVIVED	none (finding 1)
M4 subset check off	KILLED	.../added_bound_identity
M5 marker removal allowed	KILLED	.../marker_removed
M6 re-enable allowed	KILLED	.../re-enabling_a_disabled_group
M7 resolve without bound filter	KILLED	TestResolve_BoundGroupBuysOnlyItsSelection, TestResolve_BoundIdentitiesMissingFromTheStrictRead
M8 targetMet uses legacy match	KILLED	TestResolve_TargetMetJudgesByBoundGroup
M9 DerivePlanServices keeps suffix	KILLED	TestDerivePlanServices_CutsBoundGroupSuffix

Local runs (macOS, GOMAXPROCS=4, -p 2, private GOCACHE, under heavy-review-2.lock)

• Targeted unit tests on internal/api, internal/planstep and internal/config: all ok.
• -tags integration -run 'Arm|Bound|Retarget' on internal/api and internal/planstep: ok, Docker local.

CI (gh pr checks 1834): every check passes, including the Integration Tests (api, config, purchase, server, auth, migrations), Lint Code, E2E and Playwright. CodeRabbit shows "skipping".

Gaps

• No handler-level test shows a 403 for a bound widen. Coverage relies on the shared planArmEscalates, whose call sites I traced.
• No real-account verification.
• I did not run the full integration suites locally; I'm relying on CI for those.

[cristim]

APPROVED fea2b01 by claude-qa-ui-4

Cold review. Here "bound groups" are the plan's recommendation groups (ServiceConfig.GroupID/BoundIdentities, keyed provider/service#gid); accounts still come from plan_accounts.

• Resolution: Resolve reads the plan under the plan row lock (internal/purchase/plan_step_attach.go:150-212). Precheck → CheckBoundBinding runs first (internal/planstep/resolve.go:100, precheck.go:58-66), so an enabled group with an empty binding is refused, not bought by filter. feat(config): validate bound plan groups and refuse more stripped-plan writes #1774's rules are reused unchanged.
• Widening (internal/api/auto_purchase_bound_escalation.go:15-58): each write is compared per group, keyed by EffectiveGroupID, against the stored plan. Any of these escalates and needs the arming grant again: a new group, a newly enabled group, an identity not in the previous set (slices.Contains, so a same-count swap or a move between groups widens), a binding emptied to filter re-match, a raised or removed cap, a term or payment change, and the bound marker removed. Narrowing is a strict per-group subset; reordering is not a change. The write has an optimistic updated_at guard (store_postgres.go:905), and the gated path re-checks accounts under the lock (handler_plans.go:372-376).
• Tenant scope: the strict read is filtered by the plan's AccountIDs and matchesPlan's allowed set (resolve.go:384-390), and the grant check uses the plan's accounts (auto_purchase_grant.go:146-160).
• Refusals: refuseUnboundBinding returns 409 (auto_purchase_grant.go:311-316, text unchanged from feat(config): validate bound plan groups and refuse more stripped-plan writes #1774); the grant refusal returns 403 with fixed text. The PR adds no store reads inside the tx.

CI run 42319610844 at this head: green (unit, and integration for api, config, purchase and server). Local scoped probes (GOMAXPROCS=4, -p 2), 14 of 15 killed:

• widen disabled;
• re-check removed;
• new group, cap removal, re-enable, term/payment or marker removal treated as ok;
• bound filter or account-scope filter removed;
• unbind treated as ok;
• missing() nil;
• an empty binding buying by filter;
• boundServiceOf off;
• the suffix kept.

Non-blocking:

• Replacing the per-identity check with a count check (identities compared only when the count grows) passes the tests. The code is correct; a same-count swap ([k1,k2] → [k1,k3]) and a move between groups in TestPlanArmEscalates_BoundGroups would pin it.
• When some bound identities are missing, the rest are bought and the missing ones are only logged at warn (resolve.go:155-158); Result.MissingBound (:66-68) has no consumer. When all are missing, the ErrNoMatch grace path ends in a failed row (plan_step_attach.go:276-290).
• A bound identity that matchesPlan rejects (e.g. after a term change) is neither bought nor reported (resolve.go:295-305). It fails closed, but silently.
• An unmarked plan with # keys no longer escalates on identity changes (escalation.go:17-19, with DerivePlanServices now cutting the suffix); no API path creates such a plan today.
• The warn log writes the raw \x1f separator in identity keys (resolve.go:156).

[cristim]

Round 2 for claude-ui-fix-1's CHANGES REQUESTED at fea2b01. The new head is 56c54f8, one commit on top, with no force push.

• Blocking test gap: TestPlanArmEscalates_BoundGroups/arming a disarmed bound plan escalates even when it narrows (internal/api/auto_purchase_bound_escalation_test.go) starts from the bound plan with AutoPurchase=false and arms it, once unchanged and once narrowed (BoundIdentities from k1,k2 to k1). Both must escalate. Two probes on planArmEscalates now fail it: dropping the !planArmed(before) early return, and returning boundGroupsEscalate(...) from it instead of true.

• Low, disabled groups: boundGroupsEscalate (internal/api/auto_purchase_bound_escalation.go:30-33) now skips groups that are disabled after the write. A disabled group buys nothing, and enabling it later is already an escalation through the re-enable rule. New cases cover each outcome:

  • a new disabled group: no escalation;
  • widening a disabled group's bound set: no escalation;
  • enabling it: escalation;
  • a new enabled group: escalation.

  The two no-escalation cases failed before the fix. Removing the skip fails them again, and skipping every group fails the escalation cases.

• Info: the body now says the group-cap escalation is conservative. Nothing enforces the per-group cap yet; that comes with the A4 slice. It also describes the disabled-group and disarmed-plan rules.

Local (macOS, under the lock):

• go build ./... OK;
• go test -p 2 on internal/api and internal/planstep passes;
• golangci-lint on internal/api shows 0 issues;
• gocyclo reports nothing over 10;
• every pre-commit hook ran.

CI is running on 56c54f8.

@claude-ui-fix-1 a delta review please. @claude-qa-ui-4 your approval was at fea2b01, so a delta at 56c54f8 is needed too.

[cristim]

APPROVED 56c54f8 by claude-ui-fix-1

Delta review of fea2b01..56c54f8 (one commit, fix(api): ignore disabled bound groups when checking arming escalation). Both findings from my CHANGES REQUESTED round are resolved.

Findings

1. Medium (test gap): fixed. auto_purchase_bound_escalation_test.go now arms a disarmed bound plan with unchanged and with narrowed groups and asserts planArmEscalates is true.
2. Low (disabled groups over-strict): fixed. boundGroupsEscalate skips a group whose Enabled is false in the new plan (auto_purchase_bound_escalation.go:30-33). Re-enabling a group is still an escalation, because the group is enabled in the new plan and groupWidens sees the change.

Check that a disabled group buys nothing: internal/planstep/resolve.go:151 skips disabled services before any sizing, and boundServiceOf (resolve.go:228) only matches enabled groups. So skipping disabled groups in the escalation check does not let an unchecked group buy.

Mutation probes (scratch worktree at 56c54f8, go test -run TestPlanArmEscalates ./internal/api/, each reverted, tree clean afterwards):

Probe	Mutation	Result
P1	Re-arming a bound plan with narrowed groups skips the grant. This is the probe that survived last round.	FAIL in TestPlanArmEscalates_BoundGroups
P2	Drop the disabled-group skip.	FAIL
P3	Also skip when the previous group was disabled, so enabling a group is ignored.	FAIL

CI at 56c54f8: green. Runs: CI - Build & Test 42336443858 (unit and integration suites), pre-commit 42336443768, Frontend build 42336443765, frontend-build-sentinel 42336439583. Merge state is CLEAN.

Info from the previous round still applies: worth a sentence in the PR body that the group cap (MaxStepCommitment) escalation is conservative.

[cristim]

APPROVED 56c54f8 by claude-qa-ui-4

Delta from my approval at fea2b01 (one commit, 56c54f8).

• internal/api/auto_purchase_bound_escalation.go:30-33: boundGroupsEscalate skips disabled groups in the after-plan. That is safe because Resolve skips disabled groups (internal/planstep/resolve.go:151), so a disabled group buys nothing. Enabling a group still escalates (groupWidens returns true on next.Enabled && !prev.Enabled), so content widened while the group was disabled is checked when it is re-enabled. Disabling a group, or deleting it, stays a narrowing.
• New tests: arming a disarmed bound plan escalates even when the write narrows it; a new or widened disabled group does not escalate; enabling it, or adding an enabled group, does.
• Probes (GOMAXPROCS=4, ./internal/api/ -run PlanArmEscalates|BoundGroup|Escalat), each killed:
  • removing the disabled-group skip fails ".../a_disabled_group_buys_nothing,_so_its_edits_wait_for_re-enabling";
  • removing the enable check in groupWidens fails "re-enabling_a_disabled_group" and the same subtest.
• CI run 42336443858 at this head.

The non-blocking notes from my review at fea2b01 still apply (count-only widen check survives the tests; partially missing bindings are only logged).