[cristim]

Closes #1777

What

On an armed plan (enabled, auto_purchase, with a cap), a PUT /api/plans/{id} that turns a disabled services entry back on skipped the execute:purchases arming-grant check. planArmEscalates compared providers and services, which ignore Enabled, so a disabled-to-enabled flip was not an escalation and the plan started buying for that entry again with no grant.

A disabled-to-enabled flip on an armed plan now counts as an escalation (reEnablesEntry, internal/api/auto_purchase_grant.go), so it needs the grant like a new service does. An admin-key caller gets the gate's existing refusal. Entries that are new are already caught by the service-set check, so only an existing entry that flips is counted. Bound plans are covered by boundGroupsEscalate too; the clauses are OR'd.

How it is proven

• TestPlanArmEscalates_ReEnabledEntry (disabled to enabled, enabled stays, disabled stays, enabled to disabled) and two PUT-level cases (admin refused, execute:purchases allowed).
• The whole internal/api package passes on the rebased tree.
• Mutation probe: removing the new clause makes 4 tests fail.
• The existing test that documented this gap (TestUpdatePlanGated_RefusesReenablingStrippedEntry) now asserts the gate sees the write; its 409 assertions are unchanged because the binding refusal runs first. The shared stored-plan fixture now has an enabled entry, matching real data.
• Independent adversarial review (three passes over the change and the rebase onto feat(planstep): resolve bound groups and re-check arming when they widen #1834's boundGroupsEscalate): no findings; the reviewer confirmed the PUT merge keeps stored keys, so a re-keyed entry is not an escape.

Notes

Safe default taken for the open decision in the issue: fail closed (re-enable needs the grant).

🤖 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 36 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: fix/1777-reenable-arming-grant
• File: .coderabbit.yaml
• Commit: c07148d
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: a083bbed-2ee1-4b1a-b9f5-159628477da2

Commits

Reviewing files that changed from the base of the PR and between 82fe693 and c07148d.

Files selected for processing: 4

• internal/api/auto_purchase_bound_binding_test.go
• internal/api/auto_purchase_grant.go
• internal/api/auto_purchase_grant_test.go
• internal/api/auto_purchase_scope_test.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 4259c53 by claude-nibblets

Cold review of #1875 (closes #1777, +53/-5, arming-grant path). The logic is right; one blocker, lint.

Blocker: Lint Code is red at this head (run 42661468621; CI Success fails as a result), one issue: internal/api/auto_purchase_grant.go:85, gocritic rangeValCopy: for key, svc := range after.Services copies a 312-byte ServiceConfig per iteration. Iterate by key and read after.Services[key].Enabled (or take a pointer) in reEnablesEntry.

What I checked (the fix itself is sound):

• reEnablesEntry is OR'd into planArmEscalates after the armed-before and armed-after checks, so before is non-nil there; it counts only an entry that exists in both plans and flips from disabled to enabled. Entries that are new are already caught by the provider and service sets (or by boundGroupsEscalate for bound plans).
• Scoped probes (go test -run PlanArm|ReEnabled|AutoPurchaseScope|AutoPurchaseGrant, scratch worktree at this head, baseline ok), each killed: the re-enable clause removed (TestPlanArmEscalates_ReEnabledEntry); every enabled existing entry escalating (TestPlanArmEscalates_ReEnabledEntry, _BoundGroups, _UnmarkedTermAndPaymentEditsDoNotEscalate); disabled to disabled escalating (TestPlanArmEscalates_ReEnabledEntry, _BoundGroups, _ProviderAndServiceWidening).
• Survived, non-blocking: counting an entry that is absent before as a re-enable too. That is behaviourally equivalent for the grant (a new service is already caught by the service set), so it is an unpinned choice, not a hole; a one-row test of "absent entry is not counted" would pin the comment's claim.

Everything else at this head passes (39 pass, 1 skipping); I will re-check once the lint issue is fixed.

[cristim]

APPROVE @ c07148d by claude-nibblets

Delta 4259c53..c07148d is a 2-line change in reEnablesEntry (internal/api/auto_purchase_grant.go): for key, svc := range after.Services becomes for key := range after.Services with after.Services[key].Enabled read inside, which clears the gocritic rangeValCopy finding. The condition is unchanged (ok && enabled-now && !enabled-before), so my earlier probes (re-enable clause removed; every enabled entry escalating; disabled to disabled escalating), all killed by TestPlanArmEscalates_ReEnabledEntry and neighbours, still apply.

CI at this head: Lint Code and Unit Tests pass; 38 pass, 1 pending, 1 skipping.

[cristim]

Note from claude-nibblets at c07148d (my APPROVE above stands for the code): the required check CI Success is red at this head, but not because of the change. In the current pull_request run (42673034131) every real job that finished passed (Unit Tests, Integration Tests (api), Playwright Chromium, the Terraform validations, parity jobs), and the aggregator log lists the remaining jobs as "abandoned" (terraform-validate, security-scan, e2e-tests and others). A workflow_dispatch run of the same workflow (42673063729) started on this ref at 18:19 and finished failed, so this looks like the dispatch run abandoning the PR run through the workflow's concurrency group. It needs a clean re-run of the CI workflow on the PR before it can merge; nothing in the diff needs to change.

[cristim]

Update from claude-nibblets at c07148d: the CI re-run I asked for has completed. CI Success, Lint Code, Unit Tests and Run pre-commit hooks pass (41 pass, 1 skipping) and mergeStateStatus is CLEAN, so the earlier red CI Success was the abandoned-run artefact and the APPROVE above stands with green CI. It still has only one counted approval (mine) and no second reviewer assigned.