Skip to content

fix(api): answer refused managed-plan writes with a 409 code - #1011

Merged
cristim merged 1 commit into
mainfrom
fix/631-guard-409-code
Oct 16, 2026
Merged

cristim merged 1 commit into
mainfrom
fix/631-guard-409-code

Conversation

@cristim

@cristim cristim commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

A legacy PUT, PATCH or plan disable that the config store refused for
an editor-managed plan surfaced as a 500. Map the refusal to 409 with
code editor_reconciliation_required and a message pointing the user to
the schedule editor. Authorization still runs first, so a caller who may
not change the plan gets 403 or 404 and never learns the plan is
managed. The code and the planEditorRefusal helper live in a shared file
the editor routes will reuse.

Refs #631

Part 2 of 5 of the #631 legacy-writer guards; stacked on #1010; merge bottom first.

Review verdict and evidence

Original review, copied from the per-commit evidence record. Commit 33185f6b was reviewed before the rebase onto origin/main; the rebased commit is eabeaf1.

  • Reviewed tree 85fc7ba0481c88f66fda0397e56866ddbd156d8c (commit 33185f6b). +170.
  • Pass 1: one medium (missing authz-precedes-guard proof), fixed; pass 2: clean.
  • Evidence: lint 0; race unit api+config 4161 pass; integration -run Plan ./internal/api/ 389 pass, 0 skip.
  • Probes: drop mapping; 500 instead of 409; wrong code; details nil; 409→400; patchPlan update→view; scope check dropped; arm-grant no-op; disablePlan clears DatesManaged: all fail a named case.
  • RED: the 4 refusal cases returned 500 before the mapping.

Rebase: the five commits were rebased onto origin/main 0ed85aa (27 commits ahead of the old base). git range-diff of the old 5 commits against the rebased 5 shows all five as = (identical patches), so the verdict carries over per the owner's rule. New head SHA of this PR: eabeaf1. No migration is added by any of the five commits.

Review gate

CodeRabbit is not used on this stack (quota; owner decision): the gate is the independent adversarial review above plus the local verification below.

Local verification

Local verification of the rebased stack tip (macOS, Docker Postgres via testcontainers; Linux via CI):

  • go build ./... and go vet ./...: pass
  • CI-pinned golangci-lint v2.10.1: 0 issues
  • go test -race -count=1 ./internal/config/ ./internal/api/ ./internal/purchase/...: config ok, purchase ok, api has one failure, TestAuthPublicRoutes_EveryRouteIsCSRFGuardedOrExempt (route POST /api/purchases/approve/*/details unclassified). It fails identically on a clean origin/main 0ed85aa and is not touched by this stack.
  • Integration (-tags integration -p 1 -count=1) on the same packages: 5838 pass, 1 fail (the same test), 14 skip (the known pre-existing skips: 11 TestPostgresStoreDB_* in config; TestGetFederationIaC_TfvarsAutoLoadedByTerraform/aws-cross-account, TestRevokePurchase_FourEyesApproval, TestRevoke_4EyesMode_TODO in api).

Status on main

The change is dormant on main until the editor Save route ships: only plans marked managed (ramp_schedule.dates_managed) are affected, and only that route sets the marker.

🤖 Generated with Claude Code

@cristim cristim added urgency/this-sprint Within the current sprint triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm impact/many Affects most users effort/xl Multi-week / refactor type/feat New capability labels Oct 16, 2026
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 3 billable files and costs up to $0.75.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 10 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 65 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 80120d58-4f2c-4aec-9e05-6751b7a8f639
📥 Commits

Reviewing files that changed from the base of the PR and between 0117179 and c614ca0.

📒 Files selected for processing: 3
  • internal/api/handler_plans.go
  • internal/api/handler_plans_managed_guard_integration_test.go
  • internal/api/plan_editor_errors.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim
cristim force-pushed the fix/631-guard-plan-update branch from a8c0812 to 4a51fe0 Compare October 16, 2026 17:38
@cristim
cristim force-pushed the fix/631-guard-409-code branch from 658b93d to eabeaf1 Compare October 16, 2026 17:38
@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

Rebased onto main 0a5f790 (picks up #1003, which fixed the CSRF classification test that failed on the old base); git range-diff vs the previous head is empty, so the review verdict in the description carries. New head SHA: eabeaf1.

@cristim cristim left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review of 658b93d: no actionable findings

Scope note: this review covers the pre-rebase SHA. The merge driver will record the range-diff to the new head (eabeaf1); git range-diff on the PR's own commit shows =. Reviewed as the delta over #1010 (a8c0812).

Findings:

  • NON-BLOCKING: internal/api/openapi.yaml is not updated. The new 409 body code editor_reconciliation_required and the managed-plan refusal are not described on PUT/PATCH /api/plans/{id} (409 text at ~364) or POST /api/plans/{id}/purchases (~447). No test enforces it, and the editor is dormant, but the spec should say it before the editor ships. Suggested fix: add the code to those 409 descriptions in a follow-up.

What I attacked:

  • Consistency: the sentinel maps in one place, mapCreatePlanStorageError (handler_plans.go:147), used by PUT, PATCH, create and disablePlan; every store error from the guarded callers (handler_plans.go:328, :811, handler_purchases.go:672) flows through it. Same 409 plus details.code shape as the other NewClientErrorWithDetails 409s (handler_purchases.go:2182 etc.).
  • A legitimate unmanaged plan cannot reach this branch: the sentinel is only produced by the managed guard.
  • Mutation probes (2, reverted): map branch disabled -> TestLegacyWritesOnManagedPlanMapToEditorRefusal and TestManagedPlanRefusalIsHTTP409WithCode fail; code string changed -> TestManagedPlanRefusalIsHTTP409WithCode fails. Real Postgres api Managed tests pass at this SHA.

Gate: CI not applicable until retarget (base is fix/631-guard-plan-update). Labels match the issue set. CodeRabbit: no threads. Additions 170 lines.

@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

The independent review above covers 658b93d. claude-cc-631-driver rebased the stack onto main 0a5f790 (includes #1003).
Pure rebase: git range-diff a8c08123..658b93d0 4a51fe02..eabeaf1f:

1:  658b93d0 = 1:  eabeaf1f fix(api): answer refused managed-plan writes with a 409 code

The PR's own commits are unchanged, so the verdict carries to eabeaf1 (owner #631 rule).

@cristim
cristim changed the base branch from fix/631-guard-plan-update to main October 16, 2026 18:28
A legacy PUT, PATCH or plan disable that the config store refused for
an editor-managed plan surfaced as a 500. Map the refusal to 409 with
code editor_reconciliation_required and a message pointing the user to
the schedule editor. Authorization still runs first, so a caller who may
not change the plan gets 403 or 404 and never learns the plan is
managed. The code and the planEditorRefusal helper live in a shared file
the editor routes will reuse.

Refs #631

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim
cristim force-pushed the fix/631-guard-409-code branch from eabeaf1 to c614ca0 Compare October 16, 2026 18:28
@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

The independent review above covers eabeaf1. Rebased onto main after #1010 merged (0117179); #1011 retargeted to main.
Pure rebase: git range-diff 4a51fe02..eabeaf1f 01171799..c614ca02:

1:  eabeaf1f = 1:  c614ca02 fix(api): answer refused managed-plan writes with a 409 code

The PR's own commits are unchanged, so the verdict carries to c614ca0 (owner #631 rule).

@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

Gate record for c614ca0 (origin/main CLAUDE.md "Review gate"):

Merging at c614ca0 with --match-head-commit. #1012 will be retargeted to main and rebased next.

@cristim
cristim merged commit 173474e into main Oct 16, 2026
25 checks passed
@cristim
cristim deleted the fix/631-guard-409-code branch October 16, 2026 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xl Multi-week / refactor impact/many Affects most users priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/feat New capability urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant