Repository navigation
fix(api): plan/run-now/revoke follow-ups from the #360/#370/#381 reviews (ramp shape change, lock, error mapping, revoke-any scope) #386
Description
Follow-ups from the reviews of #360, #370 and #381
- (fix(api): PUT /api/plans/{id} resets the ramp schedule and re-buys completed steps #360)
internal/api/handler_plans.go:326-330: when a PUT changes an in-progress ramp's schedule shape, the plan restarts at step 0 and forgets the percentage already bought. For example, weekly-25% at step 3 switched to monthly-10% schedules step 1 at now+30d with the effective target dropping. Either carry the completed percentage into the new schedule (ceil(done/newPct) steps already complete), or reject shape changes on an in-progress ramp with a clear 409. - (fix(api): PUT /api/plans/{id} resets the ramp schedule and re-buys completed steps #360)
handler_plans.go:273-305: updatePlan does a read-modify-write outside the per-plan ramp lock that CompletePlanStep/createPlannedPurchasesTx take, so a step completing between the read and the write is overwritten with a stale CurrentStep. Take the same lock (or do a conditional update on CurrentStep). - (fix(api): "Run now" flips a purchase to running but nothing executes it #370)
handler_purchases.go:327-329: every RunPlannedPurchaseNow error becomes a 409 "cannot be started", including a provider failure or a multi-account partial completion where money moved. Map CAS/4-eyes errors to 409/403 and execution failures to a 5xx "execution failed" (directExecutePurchase has the same shape). Also stamp ExecutedAt/ExecutedByUserID the way directExecutePurchase does, and return the row's actual status instead of a hardcoded "completed". - (sec(api): apply account-scope gate to approve/cancel/retry/revoke purchase endpoints #381 review)
internal/api/handler_purchases_revoke.go:362-377authorizeSessionRevoke: the purchase-history revoke-any branch (executed-purchase return / marketplace listing) applies no account scope, so a scoped user holding revoke-any can revoke an executed purchase in another account. Apply the same scope check the revoke-own branch uses (fail closed on a nil CloudAccountID). This is the same class as sec(api): purchase approve/cancel/retry/revoke never apply allowed_accounts, unlike pause/resume/run/delete #92.
Each item needs a regression test that fails before the fix.
Activity
fix(api): map a run-now failure after the claim to 502, not 409
cristim commentedon Oct 6, 2026
Item 4 (revoke-any account scope) is already fixed on main by 8761425 (authorizeSessionRevoke now calls checkRevokeAccountAccess on every path and fails closed on a nil account id), with tests in handler_purchases_revoke_test.go (TestAuthorizeSessionRevoke_RevokeAny_AccountScope, TestRevokePurchase_RevokeAnyOutOfScopeNeverCallsAzure). Items 1-3 remain open.
cristim commentedon Oct 6, 2026
Checked against main e49e469: items 2 and 3 are also already fixed. Item 2 by 19cfb35 (conditional write on the persisted update token, UpdatePurchasePlanTx, ErrPurchasePlanConflict maps to 409, handler_plans.go ~143). Item 3 by runNowError (handler_purchases.go ~368: four-eyes 403, lost claim 409, execution failure 502 with the row's actual status, ExecutedAt/ExecutedByUserID stamped in internal/purchase/approvals.go). Only item 1 remains and needs a product decision: carry the completed percentage into the new schedule on a shape change, or reject the shape change on an in-progress ramp with 409. directExecutePurchase/approve error mapping is tracked in #532.
cristim commentedon Oct 9, 2026
Status of the four items against main (checked 2026-10-09):
This issue closes when #689 merges.
test(api): pin the first completed step and the percent-only ramp change
fix(api): refuse a ramp shape change on a plan with completed steps (#…
cristim commentedon Oct 9, 2026
Item 1 is fixed by #689 (squash-merged as 2a4fe44): PUT /api/plans/{id} now returns 409 when the stored plan has CurrentStep > 0 and the requested ramp shape differs (including a PUT that omits ramp_schedule). Items 2-4 were already fixed (19cfb35, ad84d14 + 8cb5c78, 8761425), so nothing is left open here. Carrying the completed percentage into a new schedule remains an option if the owner wants it; it would be a new issue.