[IAmJSD]

Root cause

The UI can only send the two presets or custom. Since #1183, Edit pre-fills a plan whose stored ramp is a non-preset weekly or monthly (for example weekly 20%/7d, created through the API) as Custom with the same percent and interval, so Save sends type: custom, 20%, 7 days.

rampScheduleShapeUnchanged (internal/api/handler_plans.go) compared the type label, so weekly vs custom counted as a ramp change even with identical percent, interval and step count:

• CurrentStep > 0: preserveRampProgress refused the update with 409 "cannot change the ramp schedule", even for a bare rename.
• CurrentStep == 0: the save went through, but the stored type became custom and StartDate / NextExecutionDate were rebuilt from "now". The type change also made the store's rampBaselineStale (which includes the type) clear the plan's ramp baseline.

Fix

The issue offers two options: match shapes whatever the type label is, or keep the stored type when an equal custom ramp is submitted. This PR does both, limited to the step-based types:

• rampTypesEquivalent: weekly, monthly and custom count as the same type. They only label the same step-based behavior; the only ramp type anything downstream branches on is immediate (CumulativePercent, GetCurrentCoverage, next-execution calculation, step validation). immediate and any unknown label still have to match exactly.
• When the shape is unchanged, preserveRampProgress now keeps the stored Type along with CurrentStep, StartDate, NextExecutionDate, DatesManaged and AllocationMode. Resubmitting an equal ramp leaves the stored schedule exactly as it was, in progress or not, and the ramp baseline is no longer cleared for nothing.
• Real changes are still refused with 409 once a step has run: a different percent, interval or step count, or a switch between immediate and a step-based type.

The change is in the handler only: no store, schema or migration change, and no frontend change. The update keeps its existing expected_updated_at optimistic check and store path, so this adds no new concurrency window.

Verification

Unit tests with a mocked store, driving PUT /api/plans/{id} through HandleRequest (internal/api/handler_plans_ramp_shape_test.go):

• TestUpdatePlan_EquivalentCustomRampKeepsStoredSchedule covers 5 cases: in-progress weekly 20%/7d (step 3) and monthly 5%/30d (step 1) renamed via custom, an in-progress custom 25%/7d renamed via the equal weekly-25pct preset, and not-started (step 0) weekly 20% and monthly 5% renamed via custom. Each case asserts 200 and that the saved RampSchedule equals the stored one (type, step, start date and all), plus the same NextExecutionDate.
• TestUpdatePlan_RealRampChangeOnNonPresetStillRefused covers 6 cases on in-progress plans, each expecting 409 with nothing reaching the store: different percent and step count, different interval, different percent with the same step count (30% vs 33%), weekly to immediate, immediate to custom 100%, and custom 100%/0d/1 step to immediate (the same numbers as immediate, so only the type guard refuses it).
• The existing TestUpdatePlan_RampShapeChange still passes unchanged.

Fail before, pass after:

• On origin/main (7e6ec71) with only the new tests added, all 5 EquivalentCustomRampKeepsStoredSchedule cases fail: the 3 in-progress cases get 409 instead of 200, and the 2 step-0 cases get Type:"custom" with a rebuilt start date. The 6 refusal cases pass, since they guard behavior that must not regress.
• After the fix, all of them pass.

Mutation probes, each reverted afterwards:

• Dropping the Type carry-over fails all 5 equivalence cases.
• Making rampTypesEquivalent always return true fails "custom 100% to immediate".
• Dropping the interval comparison fails "different interval".
• Dropping the percent comparison fails both "percent only" cases.

Commands, all run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid:

go build ./...
go vet ./internal/api/
go test ./internal/api/ -count=1                      # ok
go test ./internal/mocks/ -count=1                    # ok
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/api/...   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/api/handler_plans.go     # silent
bash scripts/gosec-hook.sh internal/api/handler_plans.go internal/api/handler_plans_ramp_shape_test.go  # clean
git-secrets --scan on the changed files in a fresh git init repo with isolated HOME, set up by scripts/setup-git-secrets.sh + .gitallowed  # clean
bash scripts/test-git-secrets-allowlist.sh            # 42 PASS, 0 FAIL

All of this is unit or mock evidence. No real Postgres or cloud run was needed, since the change is pure handler logic over the request and the stored plan.

Out of scope / notes

• internal/api/handler_plans.go was already 851 lines on main, over the 500-line guideline; this PR adds 23. I didn't split it here.
• A stored weekly/monthly plan resubmitted with a different custom shape before its first step still becomes custom, as before. That is a genuine schedule change.

Frontend follow-up for the UI owner

None needed. With #1183 the Edit form already sends the equal custom ramp, and the backend now accepts it and keeps the stored type.

Closes #1184

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

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 2 billable files and costs up to $0.50.}

Or wait 58 minutes for your next included review.

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: main
• File: .coderabbit.yaml
• Commit: b6355a8
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: d7a93d00-4784-4546-88da-54b38d5c492c

📥 Commits

Reviewing files that changed from the base of the PR and between b6355a8 and 00e7430.

📒 Files selected for processing: 2

• internal/api/handler_plans.go
• internal/api/handler_plans_ramp_shape_test.go

• 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 2 (claude-fleet, cold): APPROVE 00e7430

Root cause of #1184:

• rampScheduleShapeUnchanged compared the type label.
• So a stored weekly/monthly ramp resubmitted as an equal custom one counted as a change:
  • once a step had run, every edit of such a plan got a 409;
  • at step 0 the save went through but rebuilt the dates and the type.
• The fix is rampTypesEquivalent, plus keeping the stored type when the shape is unchanged (internal/api/handler_plans.go:411, :440-459).
• It covers both of the issue's proposals and the two tests the issue asks for.

Findings (none blocking):

• [non-blocking] handler_plans.go:458 - isStepBasedRampType hard-codes "weekly"/"monthly"/"custom" instead of deriving them from config.ValidRampScheduleTypes (internal/config/validation.go:277) minus immediate.
  • A future step-based type left out of this list would be refused with 409 again.
  • Nothing breaks today, and the handler already compares string literals elsewhere (:126, :629).
• [non-blocking, test gap] Probes M4 and M5 below show that no test pins exact matching for an unrecognised label, or equal TotalSteps. I tried to refute both:
  • An unknown label: the restored stored type still goes through plan.Validate() (handler_plans.go:390), so it ends in a 400, not a silent save.
  • TotalSteps: the API cannot make it differ while percent and interval are equal. Custom derives it as int(100/pct) (internal/api/types.go:882), and presets are fixed. Only legacy rows could hit it.

Attacked:

• "Nothing branches on weekly/monthly/custom": confirmed by grepping all non-test Go.
  • The only branches are on "immediate": config/types.go:290,301; validation.go:656; handler_plans.go:126,629; api/types.go:933; planstep/precheck.go:39.
  • analytics_postgres.go:53 is a report granularity, unrelated to ramps.
• Store side effect: rampBaselineStale (config/store_postgres_ramp_baseline.go:226) includes the type. Keeping the stored type now stops an equal resubmit from clearing the baseline, which is the desired behaviour, and a real shape change still clears it.
• Frontend path:
  • rampRadioValue (frontend/src/plans.ts:1282) matches presets on type, so a non-preset weekly plan is prefilled as custom.
  • Only whole-number percents are sent (plans.ts:1463).
  • This matches the scenario in the PR.
• Edge cases: a stored type "" is not step-based, so its behaviour is unchanged. Immediate vs custom with the same numbers is still refused.
• Blast radius: the changed functions are unexported and called only from preserveRampProgress and mergeUpdatedPlan. No store, schema, route or migration change.
• Merge:
  • merge-tree with origin/main 7eac792 is clean, and handler_plans.go is identical in the merged tree.
  • internal/api moved on main, so I built and tested the merged tree too.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  • M1, the Type carry-over dropped: all 5 equivalence cases FAIL.
  • M2, rampTypesEquivalent always true: "custom 100% to immediate" FAILS.
  • M3, "custom" dropped from the step-based list: all 5 equivalence cases FAIL.
  • M4, every non-immediate label treated as step-based: passes (gap above).
  • M5, the TotalSteps comparison dropped: passes (gap above).
  • M6, && changed to || in rampTypesEquivalent: "custom 100% to immediate" FAILS.

Verified locally, on git archive exports with no cloud credentials, under the heavy lock:

• Head: go build ./..., go vet ./internal/api/ and gofmt -l on the 2 changed files are clean, and go test -count=1 ./internal/api/ is ok.
• Merged tree: the same checks are clean, and the tests pass.
• Fail-before: with the head's test file on base, all 5 EquivalentCustomRampKeepsStoredSchedule subtests FAIL. The refusal cases and the existing test pass.

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

[cristim]

VERDICT: APPROVE 00e7430

Rebase check: git merge-tree against origin/main 7eac792 is clean (tree d0782b9d). The merged tree builds, and go vet passes with and without -tags integration for ./internal/api/.
Largest commit: 00e7430, 127 changed lines (cap 400). OK.

Findings

• nit internal/api/handler_plans.go:444 - the a.TotalSteps == b.TotalSteps term has no test that fails without it. Replacing it with true keeps every ramp-shape test green. Failure scenario: a stored step-based ramp whose TotalSteps differs from what the request builder derives (int(100/pct), internal/api/types.go:882). One example is an editor-managed equal-count plan. If the term were dropped, the request would be accepted, keep CurrentStep and Type, and silently take the request's TotalSteps, which changes the purchase cadence. This gap predates the PR, but this PR widens the match. Suggest one refusal case with equal percent and interval but a different TotalSteps.
• nit internal/api/handler_plans.go:458 - the "weekly", "monthly" and "custom" literals duplicate config.ValidRampScheduleTypes (internal/config/validation.go:277). A named step-based set in config would keep them in sync.

Equivalence and money safety

• "Equivalent" means the step-based labels weekly, monthly and custom, with equal PercentPerStep, StepIntervalDays and TotalSteps. On the shape-unchanged path, preserveRampProgress now also keeps the stored Type. CurrentStep, StartDate, NextExecutionDate, DatesManaged and AllocationMode were already kept, so the stored schedule is byte-identical apart from the request-owned fields.
• Nothing downstream branches on weekly, monthly or custom. Every Type branch tests only "immediate": internal/config/types.go:290,301; internal/api/handler_plans.go:126,629; internal/api/types.go:933; internal/config/validation.go:656; internal/planstep/precheck.go:39. Cadence and amounts depend only on percent, interval, steps and allocation mode, and all of them must match.
• rampBaselineStale (internal/config/store_postgres_ramp_baseline.go:215) includes Type. Keeping the stored Type stops the baseline being cleared for a no-op edit.
• Frontend: rampRadioValue (frontend/src/plans.ts:1282) maps every non-preset step ramp to "custom" with the same percent and interval, so the backend receives exactly the request shape this PR accepts. The frontend is untouched, so jest and Playwright were not needed.

Attacked

• Merged tree: go test ./internal/api/ -count=1 ok.
• Mutation probes in a scratch export of the merged tree, each restored afterwards:
  • drop Type carry-over: EquivalentCustom FAILS
  • rampTypesEquivalent always true: RealRampChange FAILS
  • "immediate" counted as step-based: RealRampChange FAILS
  • drop the interval compare: RealRampChange FAILS
  • drop "monthly" from the set: EquivalentCustom FAILS
  • drop the TotalSteps compare: SURVIVES (nit above)
• Checked unknown and empty labels (exact match only), immediate to custom 100% and back (refused), and step-0 equal edits (dates kept, no rebuild).

[IAmJSD]

Local verification at 00e7430ef1ae663e0210cc46b75fd430358adfe8 merged onto current main (51da9bd6; merge-tree 99ee50e2; macOS):

• go build ./..., go vet ./... and go vet -tags integration ./... pass.
• go test -race -short -count=1 on the packages this PR changes passes: ./internal/api.
• The full go test -race -short ./... suite passes on the tree with every PR in this merge train applied (tree 99ee50e2).
• Nothing was applied to any cloud.

[IAmJSD]

Marked ready and merged by IAmJSD's agent, at IAmJSD's request, after 2 independent reviews and local verification (see the review and verification comments above, all at 00e7430ef1ae663e0210cc46b75fd430358adfe8).