Skip to content

fix(api): restore guarded planned purchase creation - #873

Merged
cristim merged 3 commits into
mainfrom
fix/631-ship-guarded-creator
Oct 14, 2026
Merged

cristim merged 3 commits into
mainfrom
fix/631-ship-guarded-creator

Conversation

@cristim

@cristim cristim commented Oct 14, 2026 •

Copy link
Copy Markdown
Member

The authenticated Add Purchases route currently refuses every valid request with409. Restore the existing transactional creator now that plan-step recommendation attachment and execution are available.

Scheduling rechecks full account scope under the plan lock, validates enabled plans/accounts/ramp/veto settings and occupied steps, stamps the creator, and writes roots plus the requested-date clock anchor atomically. Manual plans work with global automatic purchasing OFF; automatic plans require the global switch and a valid cap. Unsupported multi-step Savings Plans remain refused. Requested dates and existing veto rules are preserved.

Refs #631. Seven files, 392 changed lines. UI/graph changes are separate work; this refresh preserves the merged cap-null update behavior and schema migration 119. Corrective PR875 explicitly restores the AWS identity required by existing pause/run-now test fixtures after the shared creator fixture changed to Azure; production credential resolution stays unchanged.

Exact head e25ae316057f67162b6ff0c3e2164fa22ac20024, tree 1cabd2cbd3cb88e514d67d9aa01afeb1439e5a52, base a3bc552aaa85523ffbd96b2f865386bf662a440e:

  • Retained independent source/provenance reviews and fresh cold adversarial reviews each completed two clean passes.
  • Author API race 47.566s, build, pinned integration lint with zero issues and all full-range normal hooks passed on this exact head.
  • Independent exact-head native race passed 33 cases in 35.181s with 40 fresh PostgreSQL migration-119 confirmations. This covers every shared-fixture consumer, all pause/run-now cases, real routed creation, 20 Azure compute units/monthly/one-year, full-term commitment 2400, manual approval/global OFF and automatic/global ON completion, frozen recommendations, durable child/history/ledger, correct next date and no repeat purchase. Merged cap-null and SP cases also passed.
  • Current-parent source reproduced the exact old 409 refusal in 3.315s. Independent deletion probes made the locked-membership and ramp-anchor regressions fail; those function and regression bodies remain unchanged through the rebase.
  • The fixture's exact three-line parent control reproduced the real Azure credential-dispatch error in 2.529s. Corrective PR875 merged normally after its exact-head dispatched CI passed; final PR873 CI remains a separate required gate.

Production router/store/resolver/manager and local PostgreSQL are real. Authentication, credentials, email, global state, clock and recording-only provider are fixtures. No real purchase or live configuration mutation was performed. Root explicitly accepts this proof for code merge; postdeployment DEV verification remains read-only. Existing veto timing still applies; a short requested date is not silently retimed.

Required exact-head CI/protections must pass before normal squash merge. Owner-selected independent Sol review covers this revision; any actual new review finding still requires disposition. Source refs and original worktrees are preserved.

@cristim cristim added 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 labels Oct 14, 2026
@coderabbitai

coderabbitai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

📝 Walkthrough

The planned-purchase API now creates scheduled purchases in a transaction. It checks plan access and scheduling conditions, stores the executions, and updates the plan’s schedule. Tests cover validation, rollback, and concurrent creation.

Changes: Planned Purchase Creation

Layer / File(s) Summary
Transactional creation and schedule validation
internal/api/handler_plans.go
The handler now creates scheduled purchases within a transaction. It rechecks mutation access after locking the plan, validates the plan and ramp settings, and updates the plan’s schedule.
Creation and validation tests
internal/api/plan_step_no_recs_test.go, internal/api/handler_plans_test.go, internal/api/handler_test.go, internal/api/openapi.yaml
Tests cover successful creation, invalid scheduling settings, and schedule anchoring. The handler test expects a successful response, and the OpenAPI description lists conflict conditions.
HTTP, rollback, and concurrency integration tests
internal/api/handler_plans_concurrency_integration_test.go
Integration tests exercise authenticated creation, membership rechecks, rollback after an insert failure, retry behavior, and concurrent step creation.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Handler as createPlannedPurchases
  participant Transaction as createPlannedPurchasesTx
  participant PlanRecords
  participant ExecutionRecords

  Client->>Handler: Submit plan ID, count, and start date
  Handler->>Transaction: Create purchases with session
  Transaction->>PlanRecords: Lock and load plan
  Transaction->>Transaction: Recheck access and validate schedule
  Transaction->>ExecutionRecords: Save scheduled executions
  Transaction->>PlanRecords: Persist updated schedule
  Transaction-->>Handler: Return created count
  Handler-->>Client: Return response
Loading
Merge Risk: ⚪ Minimal · up to e25ae

No concrete merge-blocking risk remains from the reviewed changes. Complete the required exact-head checks before merging.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 6 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: restoring guarded planned purchase creation in the API.
Full details: Docstring Coverage

Explanation

Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 6 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • 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

cristim commented Oct 14, 2026

Copy link
Copy Markdown
Member Author

Independent verdict: no actionable findings in two adversarial passes at b223cc89f527950f7652b3dca8c575fd430b6253, tree dc6e457e0529817251a24717e202b6f9478089af, base c5660356228e4451edeec818c32600ac36cd5e99.

The fresh verifier traced the real handler transaction, plan lock and membership-write interlock; validation and unsupported-SP policy; automatic cap/global-switch controls; creator attribution; occupied steps and atomic date metadata; downstream root adoption; and pinned Azure quantity/payment/term conversion.

Independent executed evidence:

  • Focused API integration suite with race/count1 passed in 19.908s. It covered routed manual/global-OFF approval and automatic/global-ON execution, caps, creator/four-eyes, rollback, concurrent creation, occupied steps and ramp dates.
  • The actual local PostgreSQL/router/store/resolver/manager path attached 20 Azure compute units, monthly payment, one-year term and frozen commitment 2400, then recorded durable child/history/ledger completion, correct next date and no repeat purchase.
  • Exact parent production handler reproduced the old refusal: expected 200, actual 409 at the routed POST, 3.462s.
  • Deleting only the post-lock account check made the membership regression fail with errNotFound expected but no error received, 3.218s. A registered fixture principal prevented unrelated foreign-key failure from masking the forbidden write.
  • Deleting only the ramp date anchor failed both step-0 and step-2 advancement assertions, 1.744s.

Author full API race passed in 50.711s; native creator/combined scenarios in 22.408s; build, pinned integration lint and all normal hooks passed. Retained source/provenance reviews also completed two clean passes; the six owned file contents equal the original reviewed B source.

Authentication, credentials, email, global state, due clock and the recording-only provider are fixtures. No real cloud purchase, live configuration mutation or manual deployment occurred. Initial sandbox connection/cache failures were excluded; approved retries supplied the behavioral evidence above. Owner-selected independent Sol review applies.

The public head/target/labels were verified after creation. Exact-head CI 38648306213 and pre-commit 38648306292 have independent background watchers and remain pending at this record. Normal merge requires current green checks/protections, a fresh main compatibility check and resolution of any actual new finding. Existing future-date/veto semantics are preserved; this does not silently retime an unsafe requested date.

@cristim
cristim force-pushed the fix/631-ship-guarded-creator branch from b223cc8 to 6a2966f Compare October 14, 2026 22:47
@cristim

cristim commented Oct 14, 2026

Copy link
Copy Markdown
Member Author

The refreshed head is 6a2966f794a1c6f43331357a986b142d17d12a45, based on a3bc552aaa85523ffbd96b2f865386bf662a440e; its rebase conflicts are resolved. It is currently held for an integration-test fixture regression.

The creator fixture now persists an Azure account, while existing pause and run-now tests specialize that fixture with AWS credentials and frozen AWS recommendations. Their credential resolution selects Azure and fails before the recording provider. The failure reproduced on this exact head through the real handler and native PostgreSQL migration119, including an independent reproduction. Earlier creator-focused green results did not cover these consumers.

The corrective commit fd9771531dd22ae219b56f2ef0f9d2d296cd6f69 changes only three lines in the specialized execution fixture: persist its AWS identity and assert the provider after readback. Production credential resolution is unchanged. Its expanded native shared-consumer and Azure both-mode race suite passed in40.611s, API unit race in43.629s, build and pinned lint passed, and all normal commit hooks passed. Independent verification of the corrective commit is running.

The correction will land through a separate feature-target PR with explicit build/test CI dispatch before this PR receives its final new-head gates. No live purchases or cloud mutations were performed, and this PR is not ready to merge yet.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔇 Additional comments: 7
internal/api/handler_purchases_pause_claim_integration_test.go: 1

105-106: LGTM!

Also applies to: 116-116

internal/api/handler_plans_test.go: 1

724-745: LGTM!

internal/api/plan_step_no_recs_test.go: 1

38-153: LGTM!

internal/api/handler_test.go: 1

1367-1397: LGTM!

internal/api/openapi.yaml: 1

449-452: LGTM!

internal/api/handler_plans_concurrency_integration_test.go: 1

96-169: LGTM!

internal/api/handler_plans.go: 1

544-546: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Compare start_date by calendar day, not by the current instant.

parseCreatePurchasesRequest parses start_date as midnight UTC. !startDate.After(time.Now()) therefore rejects today's date, which is expected. The check also depends on the server's current time relative to UTC midnight, so there is no time-zone edge for tomorrow: tomorrow at 00:00 UTC is always later than now. This check is correct. The real gap is that the 400 is returned only after the plan lock, account read, and global config read. Move the date check into parseCreatePurchasesRequest so invalid requests fail before the transaction starts.


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 4b03c4a0-dbbd-4673-810e-ed335547d8e2
📥 Commits

Reviewing files that changed from the base of the PR and between a3bc552 and e25ae31.

📒 Files selected for processing: 7
  • internal/api/handler_plans.go
  • internal/api/handler_plans_concurrency_integration_test.go
  • internal/api/handler_plans_test.go
  • internal/api/handler_purchases_pause_claim_integration_test.go
  • internal/api/handler_test.go
  • internal/api/openapi.yaml
  • internal/api/plan_step_no_recs_test.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

@cristim

cristim commented Oct 14, 2026 •

Copy link
Copy Markdown
Member Author

Independent review: PASS at e25ae316057f67162b6ff0c3e2164fa22ac20024, tree 1cabd2cbd3cb88e514d67d9aa01afeb1439e5a52, against base a3bc552aaa85523ffbd96b2f865386bf662a440e. The retained source reviewer and the independent Sol medium verifier each completed two full-range passes with no actionable findings.

Independent exact-head native race verification passed 33 cases in 35.181s, with 40 fresh databases confirming migration 119. It exercised all shared-fixture consumers, pause/run-now cases, creator scope/clock/rollback guards, nullable caps, SP policy and both Azure modes through recorded provider calls to durable completion without rebuying. Author API race passed in 47.566s, build and pinned integration lint passed, and all full-range normal hooks passed. The previous parent failures and scope/clock deletion controls remain valid through verified source identity.

CodeRabbit's review 5526011434 covers this exact head and contains six LGTM notes plus one unverified performance suggestion. We've reviewed that suggestion in two independent passes and kept the future-date check after the plan lock at handler_plans.go:544. A request parsed just before UTC midnight can wait on the lock until its requested date is in the past, so moving the check exclusively into parsing would weaken validation. An extra early check would duplicate the authoritative check without a demonstrated performance problem, so there's no source change for this suggestion. There are no inline findings or unresolved source findings.

Required pre-commit run 38653090026 passed on attempt 2 at this head after attempt 1 failed during Trivy installation before hooks started. The failed job was retried normally with the same source and all checks retained. Full CI run 38653090574 remains pending, so this isn't a merge-ready claim yet. The fixture and recording-provider limits in the PR body still apply, and no live purchase or cloud mutation was performed.

@cristim
cristim merged commit 5134af4 into main Oct 14, 2026
25 of 26 checks passed
@cristim

cristim commented Oct 14, 2026 •

Copy link
Copy Markdown
Member Author

Merged normally as 5134af444026276c6cdbf7a135ca00e54d656ef0 from reviewed head e25ae316057f67162b6ff0c3e2164fa22ac20024. Final checks confirmed the unchanged base, MERGEABLE/CLEAN, all current checks SUCCESS, both required contexts green and no unresolved findings. The merge used the exact-head match without bypassing protections.

The merged whole tree 1cabd2cbd3cb88e514d67d9aa01afeb1439e5a52 matches the independently verified head. Source review, native PostgreSQL API/attachment/execution proof, parent failures and normal local hooks are recorded above. The required Linux pre-commit setup retry passed with the same source, and full CI run 38653090574 completed SUCCESS before merge.

The guarded Add Purchases route is now on main. Its four automatic post-merge workflows, including AWS deployment, have individual background watchers. Deployment and read-only DEV verification are still pending, so this doesn't claim a live plan was changed or a live purchase was made. Original refs, worktrees and proof artifacts remain preserved for the active #631 delivery.

Follow-up issues filed: none; the remaining existing #631 UI date-default and planned-purchase projection work is recorded on the parent issue and coordinated separately.

@cristim

cristim commented Oct 14, 2026

Copy link
Copy Markdown
Member Author

The guarded creator from #873 is now verified in the running deployment. Public GET /version returned HTTP 200 with git_sha=5134af4 and build time 20261014233006. The running image digest matches the image tagged with that commit and timestamp; deployment run 38654357934 succeeded at full commit 5134af444026276c6cdbf7a135ca00e54d656ef0. All four post-merge workflows completed successfully.

This confirms the deployed source version. No live plan or purchase was created, no scheduler/configuration state was changed, and no cloud purchase was performed. Authenticated plan listing was not exercised because this verification lane had no existing API session. The real local API/PostgreSQL and recording-provider execution evidence remains recorded above.

The quantity/resource display correction continues in #876. The existing UI date-default improvement and permanent combined regression proof remain tracked under #631.

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