[IAmJSD]

Problem

DELETE /api/purchases/planned/{id} (deletePlannedPurchase, internal/api/handler_purchases.go) cancels the execution and then disables its plan. Since #621, the disable runs only when the caller may mutate every account on the plan (requirePlanMutationAccess). For a scoped user whose scope covers only some of the plan's accounts, the cancel still went through and the request returned 200 canceled with plan_disabled=false, but the plan stayed enabled. The scheduler (shouldNotifyPlan, Enabled && AutoPurchase) kept notifying and creating executions for every plan account. The frontend types this call as void, so the user saw a success and nothing else. The Plans page's "Cancel purchase and disable plan" dialog says the plan will be disabled.

Fix: fail closed, not silent

Decision (please confirm): a scoped caller who may not disable the plan is now refused before anything is written. They get a 403 that says why. This reverses #621's choice to "cancel, skip the disable, report plan_disabled=false". I chose refusal because:

• The other options are applying the disable for a partially scoped user (a privilege escalation onto accounts outside their scope, which is what sec(api): remaining tenant-scoping gaps after #499 #520 fixed) or a 200 that silently leaves the plan enabled (this bug).
• The endpoint's documented effect is cancel and disable. Doing half of it and reporting success is how the user ended up misled.
• No frontend change is needed for the message to show on the Plans page. apiRequest copies the JSON error into err.message, and plans.ts shows Failed to disable purchase: <message>.

Trade-off: a partially scoped user can no longer cancel their own account's child execution of a multi-account plan through this endpoint. An administrator with access to every plan account has to do it. If you would rather keep that cancel available, the alternative is a separate cancel-only route. That is a product decision, so I left it out of this PR.

Changes:

• requirePlannedCancelPlanScope runs after the existing permission, execution-scope and ownership gates and before the cancel. Unrestricted sessions return before any store read. Scoped sessions load the execution, and if it has a plan, run requirePlanMutationAccess. A refusal becomes a 403: "canceling this scheduled purchase also disables its plan, which requires access to every account on the plan; nothing was changed. Ask an administrator ...". A store error still surfaces as a 500, and now also before the cancel. Before, the cancel was already committed when the 500 came back.
• Race between the check and the disable. The plan's accounts can change between the pre-check and the disable, for example when an admin attaches an out-of-scope account mid-request. disablePlanAfterCancel runs the every-account check again. If that check now refuses, the request returns 409: "this scheduled purchase was canceled, but its plan was not disabled ...". It does not return a 200, and it does not disable a plan that now reaches an out-of-scope account. A retry is then refused by the pre-check with the 403.
• plan_disabled is kept for compatibility and is now true whenever present (omitted when there is no plan). types.go and openapi.yaml are updated. The operation now documents the 403/409 behavior and lists 409, which cancelOrRecoverExecution could already return.
• Unrestricted callers and executions with no plan behave as before.

Interaction with plan deletion: purchase_executions.plan_id is ON DELETE SET NULL, so a deleted plan shows up as an empty PlanID. In that case the plan check is skipped and the cancel proceeds. A dangling plan ID cannot reach the 403.

Verification

All of this is unit/mock evidence through NewRouter(h).Route with MockConfigStore. Nothing ran against a real database or a real cloud account. Tests were run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's default AWS profile is invalid.

Tests in internal/api/delete_planned_purchase_scope_test.go:

• PartiallyScopedRefusedBeforeCancel: 403 with the reason, zero TransitionExecutionStatus, zero UpdatePurchasePlan, no GetPurchasePlan.
• ZeroAccountPlanRefusedBeforeCancel: same. Zero-account plans are hidden from scoped callers.
• PlanAccountLookupFailureSurfacesBeforeCancel: a store error is a non-client error and nothing is canceled.
• PlanAccountsChangeDuringCancel: the first GetPlanAccounts returns [A] and the second returns [A,B]. Result: one cancel, two lookups, 409 naming the half-done state, plan not disabled.
• ScopedNoPlanCancelsWithoutPlanCheck: a scoped cancel of a plan-less execution succeeds with no plan lookup.
• Unchanged and still passing: fully scoped and unrestricted disables, out-of-scope execution 404 before any write, permission 403 before scope, cancel-before-disable ordering, plan_disabled omitted without a plan.

Fail before (the new tests on unmodified main):

--- FAIL: TestRouterDispatch_DeletePlannedPurchase_PartiallyScopedRefusedBeforeCancel
        Error: Expected nil, but got: &api.DeletePlannedPurchaseResponse{Status:"canceled", PlanDisabled:(*bool)(...)}
--- FAIL: TestRouterDispatch_DeletePlannedPurchase_ZeroAccountPlanRefusedBeforeCancel
--- FAIL: TestRouterDispatch_DeletePlannedPurchase_PlanAccountLookupFailureSurfacesBeforeCancel
        mock: TransitionExecutionStatus was called 1 time(s) ... expected none
--- FAIL: TestRouterDispatch_DeletePlannedPurchase_PlanAccountsChangeDuringCancel

Pass after: all 11 TestRouterDispatch_DeletePlannedPurchase_* pass.

Mutation probes, each applied alone and then reverted. Every one is killed by an assertion, not by a panic:

• Pre-cancel check removed: Partial, ZeroAccount, LookupFailure and AccountsChange fail.
• Post-cancel re-check removed: AccountsChange fails.
• Post-cancel refusal swallowed (return nil): AccountsChange fails.
• Pre-check refusal mapped back to errNotFound: Partial and ZeroAccount fail.
• AllowsAll short-circuit inverted: Partial, ZeroAccount, LookupFailure and AccountsChange fail.

Commands:

go build ./...
go test -short -race ./internal/api/ -count=1            # ok
go test ./internal/api/ -run 'TestRouterDispatch_DeletePlannedPurchase' -count=1 -v
go test ./internal/mocks/ -count=1                       # ok (TestNoUnfailableMockAssertions)
go vet ./internal/api/                                   # clean
gofmt -l internal/api/                                   # clean
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/api/...   # 0 issues
gocyclo -over 10 internal/api/handler_purchases.go internal/api/types.go   # silent
git-secrets --scan (fresh git init, isolated HOME, --register-aws + repo patterns) on the changed files   # clean
scripts/test-git-secrets-allowlist.sh                    # 42 PASS, 0 FAIL
python3 yaml.safe_load(internal/api/openapi.yaml)        # ok

Not run locally: gosec (the hook binary is not installed here), and the repo-wide go test -race ./..., which I skipped because of limited disk on this machine. Only the internal/api package was run with -race.

Out of scope / follow-ups

• Frontend (deliberately not touched here): the dashboard's cancelScheduledPurchase and detail-modal cancel show a generic "Failed to cancel purchase" and drop the server message. Their dialog also says "The plan itself stays in place", which contradicts the backend disabling the plan for every caller (since fix(purchase): reject an empty CommitmentID before saving purchase history #774). The issue's other acceptance points (hide or disable "Disable plan" for scoped users on plans they cannot mutate, a frontend test) belong to a frontend change.
• Refusing a partially scoped cancel even when the plan is already disabled is conservative: the disable would be a no-op. Allowing it would need a plan read and its own race handling, so it is left as a possible refinement.
• Overlap check: fix(api): bound update-any by its Constraints on planned-purchase DELETE #1097 (open, ours) edits authorizePlannedPurchaseCancel in the same file, but not deletePlannedPurchase's body. fix(config): disable plans left with no accounts when their account is deleted #1065 and fix(config): lock a deleted account's plans before disabling them #1087 touch only internal/config. fix(plans): keep a plan's other services on a legacy update #1041 touches plan updates only. I don't expect textual conflicts.

Closes #623

Maintainer: please mirror the issue's triage labels on this PR: triaged, priority/p3, severity/low, urgency/this-quarter, impact/few, effort/s, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 42696ba9-0224-4f5d-8244-93e3b65ed8cf

📥 Commits

Reviewing files that changed from the base of the PR and between 2d822f3 and 9079297.

📒 Files selected for processing: 4

• internal/api/delete_planned_purchase_scope_test.go
• internal/api/handler_purchases.go
• internal/api/openapi.yaml
• internal/api/types.go

+------------------------------------------------------------------------------------------------+
| Sign your work. Craftsmen of an earlier age were proud to sign their work. You should be, too. |
+------------------------------------------------------------------------------------------------+
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

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

[cristim]

Independent review (claude-cc-platform reviewer, cold brief): APPROVE 9079297937a617901a5c1f3e30dbcf14c9f09465

This fixes #623. A scoped caller whose scope does not cover every plan account now gets a 403 before any write: requirePlannedCancelPlanScope runs requirePlanMutationAccess on the stored execution's plan. plan_disabled is always true when present, and the same helper turns a later scope change into a 409 after the cancel.

Authorization checks:

• Store errors: a failed account lookup returns 500 before the cancel, which fails closed.
• Admin and unrestricted: they return before any store read.
• API keys: they use the same getAccountScope, so they are no looser than before.
• Creator: creators still get the 403 when the plan is out of scope.
• Edge cases: an execution with no plan skips the check (tested), and out-of-scope execution ids still return 404.
• Pause/resume: these do not touch the plan, so there is nothing to gate there.

The "disable the plan" step has no plan-update permission gate on this route; only account scope applies. That predates this PR.

Tests: go test -race ./internal/api/ passes at the head and on the tree merged with current main (clean merge, tree b05acc99). go vet is clean. There is no textual overlap with the open #1097. Mutation probes:

• pre-check call dropped: killed (4 tests);
• store error treated as allow: killed;
• wrong plan id (the execution id passed instead of execution.PlanID): survived, see the nit below;
• the getAccountScope error ignored in the pre-check: probably survived (not cleanly isolated);
• the post-cancel 409 swallowed: killed.

Nits:

• The GetPlanAccountsFn stubs in delete_planned_purchase_scope_test.go:160-216 ignore the plan id, so a regression that looks up the wrong plan passes. Asserting the id inside the stub would fix it.
• getAccountScope and the execution load run twice for scoped callers.
• A zero-account plan gets the same 403 message.

Product decision, which you raised: a partially scoped user can no longer cancel their own child execution of a multi-account plan. The frontend still shows a generic "Failed to cancel".

[cristim]

Independent review (claude-fleet): APPROVE 9079297

Findings (none blocking). A further non-blocking follow-up outside this diff was reported privately to the maintainers, per SECURITY.md.

1. [non-blocking, product call] The trade-off the PR body flags is real. A scoped user can no longer cancel their own account's child of a multi-account plan through DELETE /api/purchases/planned/{id}. fix(plans): a scoped user's 'Disable plan' on a multi-account plan silently leaves the plan enabled #623's acceptance criteria asked for the frontend to surface this or hide the button; the PR chose a backend refusal instead.
2. [non-blocking] The PR says the reason is shown to the user. That holds only on the Plans page (frontend/src/plans.ts:883 shows err.message). Both dashboard paths (frontend/src/dashboard.ts:636 and :678) show a generic "Failed to cancel purchase" and drop the 403 reason, and fix(plans): a scoped user's 'Disable plan' on a multi-account plan silently leaves the plan enabled #623 names both dashboard call sites.
3. [nit] Each request now reads the account scope up to 4 times and the execution up to 3 times. That is acceptable, and admin sessions still short-circuit before any store read (handler_purchases.go:659-661).

Entry points:

Path	file:line	Disables plan?	Guarded by the plan-account scope?
DELETE /api/purchases/planned/{id}	handler_purchases.go:588, pre-check :608, re-check :680	yes (the only caller of disablePlan, :757)	yes: before the cancel (403) and after it (409)
POST /api/purchases/cancel/{id} (email token or session)	handler_purchases.go:1555, :1664	no	n/a: execution only
SQS async cancel	purchase/messages.go:197, then approvals.go:480	no	n/a: execution only, token-gated
pause/resume planned	handler_purchases.go:253	no	requireExecutionAccess
revoke scheduled	handler_purchases_revoke.go:240	no	n/a

No other path writes plan.Enabled = false.

Attacked:

• A partially scoped caller gets 403 and nothing is written. A zero-account plan also gets 403.
• A plan-level parent execution is refused earlier by requireExecutionAccess, whose 404 comes first, so plan existence is not leaked.
• A deleted plan: plan_id becomes NULL, so the check is skipped.
• Unrestricted callers and admin keys: no store read, as before.
• A store error is now a 500, raised before the cancel.
• TOCTOU: the re-check after the cancel returns 409 rather than disabling a plan that has gained an out-of-scope account. A retry after a partial 409 is refused by the pre-check with 403.
• The 403 tells the caller only that the plan reaches accounts outside their scope, not which ones.
• Mutation probes on handler_purchases.go, each caught by an assertion and restored byte-identical:
  • Dropping the pre-check: 4 tests FAIL.
  • The read gate instead of the write gate: FAIL.
  • Checking only the execution's own account: 4 FAIL.
  • Dropping the post-cancel re-check: FAIL.
  • Swallowing the 409: FAIL.
  • Skipping fan-out children: 4 FAIL.
  • Swallowing a store error in the pre-check: FAIL.

Verified locally, on a git archive export of HEAD with no network or credentials:

• go build ./..., go vet ./internal/api/ and gofmt -l on the changed files: clean.
• go test -count=1 -race ./internal/api/: ok.
• The new test file against the base export: exactly 4 tests FAIL (PartiallyScoped, ZeroAccount, LookupFailure, AccountsChange).

Reviewed by an independent agent with no author context. Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

Local verification at 9079297937a617901a5c1f3e30dbcf14c9f09465 merged onto current main (dec9ad77; merge-tree c8628a7b; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• Nothing was applied to any cloud.

[cristim]

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at 9079297937a617901a5c1f3e30dbcf14c9f09465).