feat(purchases): in-app revocation within free-cancel window (closes #290)

[cristim]

Summary

Implements in-app purchase revocation with two complementary layers:

1. Pre-fire delay (primary path, new): defer the cloud-provider SDK call by a configurable window (default 48h). Revoking during that window is a local DB cancel — zero provider involvement, zero cost, works identically for AWS / Azure / GCP.
2. Provider-cancel API fallback (the original feat(purchases): in-app revocation within free-cancel window (closes #290) #804 design): if the row has already moved past the delay window to completed, the existing Azure CalculateRefund + Return SDK call still applies. AWS/GCP return 422 here (no public cancel API).

Closes #290.

The Gmail-style pre-fire delay (primary path)

Setting	Default	Range	Where
PurchaseDelayHours	48	[0, 168]	global config (Settings UI)

Flow

approve  ─►  DB row {status=scheduled, scheduled_execution_at=now+delay}  ─►  email user immediately
                                                                       ╲
                                                                        ╲ within window: user clicks revoke ─► status=cancelled, no SDK call, $0 spent
                                                                        ╱
                                                       scheduler tick (scheduled_execution_at <= now)
                                                                        ╲
                                                                         ╲ SDK call ─► status=completed ─► confirmation email (no revoke link)
                                                                        past window: revoke falls through to provider-API path (Azure works, AWS/GCP 422)

• PurchaseDelayHours = 0 preserves the legacy immediate-execute behavior — opt-in for users who don't want any delay.
• Idempotent scheduler: tick scans WHERE status='scheduled' AND scheduled_execution_at <= NOW(). Uses an owner-token compare-and-clear pattern so two overlapping ticks can't fire the same row twice.

Why this is the right shape

The original #804 design called the provider cancel API after the SDK purchase had already settled. That works on Azure (7-day refund window) but not on AWS / GCP, which have no public cancel API. Deferring our own SDK call sidesteps the asymmetry entirely — for the common case ("I just clicked approve and changed my mind"), no provider cooperation is needed.

Provider-cancel API fallback (the original #804 design)

Still wired up for rows that pass scheduled_execution_at without being revoked. Useful when the user wants delay=0, or returns a week later.

• Azure: two-step CalculateRefund + Return via armreservations SDK, 7-day window. Button shown only for Azure rows within the window.
• AWS / GCP: no public cancel API; endpoint returns 422 and the History UI hides the button.

Schema (migration 000065)

ALTER TABLE purchase_history ADD COLUMN IF NOT EXISTS (idempotent):

• revocation_window_closes_at TIMESTAMPTZ — set at write-path per provider policy (pre-fire delay) OR per provider's published window (fallback).
• scheduled_execution_at TIMESTAMPTZ — when the deferred SDK call should fire (pre-fire delay only).
• revoked_at TIMESTAMPTZ, revoked_via VARCHAR(32) — stamped on revoke.
• support_case_id TEXT — reserved for Phase 2 AWS Support case (feat(email): post-execution notification email with one-click revocation link valid for the AWS cancel window #291).

CHECK constraints enforce audit consistency:

• revoked_via ∈ {known values, …} or NULL.
• support_case_id non-null only when revoked_via = 'support-case'.
• revoked_at and revoked_via are jointly null or jointly non-null.

Backend

• POST /api/purchases/{id}/revoke route at AuthUser level.
• Fail-closed: auth nil → 403 ClientError before any store or session call.
• RBAC: revoke-own:purchases / revoke-any:purchases. revoke-own granted by default to all users (mirrors approve-own). Ownership uses account-access (GetAllowedAccountsAPI) because history rows pre-date created_by_user_id.
• Idempotency: revoked_at IS NULL guard in MarkPurchaseRevoked. Already-revoked returns status: already_revoked without re-firing.
• Azure error classification: 400/409/422 + policy keywords map to 400; transient faults propagate as 5xx.
• Scheduler: new periodic task in internal/scheduler/scheduler.go for scheduled-purchase fire. Owner-token CAS protects against overlapping ticks.

Frontend

• canRevokeCompletedRow: provider-agnostic for scheduled status (pre-fire window); Azure-only for completed status (provider window). Both gated on session + not yet revoked.
• Revoke button rendered in History action cell; escapeHtml on purchase_id.
• Destructive confirm dialog + api.revokePurchase() + success toast + history refresh.
• New row state badge for scheduled (e.g. "Scheduled to execute in 47h — revoke link sent to email").

Tests

• 10+ unit tests for the revoke handler (nil auth, empty ID, not found, already revoked, AWS 422, GCP 422, Azure outside window, Azure success via injected stubs, Azure calc-refund client error, Azure return server error).
• Pre-fire delay path: scheduled status set on approve, SDK NOT called, revoke during window flips to cancelled.
• Scheduler tick: scheduled_execution_at <= now fires SDK, transitions to completed.
• parseAzureReservationIDs table tests, authorizeSessionRevoke RBAC matrix.
• Frontend: regression test for legacy blank-status Azure rows + scheduled-state rendering.

Test plan

• go test ./... — all packages green
• npm test in frontend/ — all suites green
• Manual: approve a purchase with PurchaseDelayHours = 1h, verify email arrives immediately, status is scheduled. Click revoke from email → status flips to cancelled, no SDK call.
• Manual: approve with PurchaseDelayHours = 0, verify legacy immediate-execute behavior unchanged.
• Manual: let a scheduled purchase pass its scheduled_execution_at; verify scheduler fires SDK, status → completed, confirmation email arrives without revoke link.
• Manual: completed Azure row within 7 days still shows Revoke button (fallback path); AWS/GCP completed rows don't.

Related

• Closes feat(purchases): in-app revocation flow — direct API for free-cancel window + AWS Support case drafter for week window #290.
• Phase 2 (support:CreateCase drafter for post-window AWS revocation): tracked separately at feat(email): post-execution notification email with one-click revocation link valid for the AWS cancel window #291.
• Post-execution email + token UX (separate concern): PR feat(email): post-execution notification with one-click revoke (closes #291) #889.

[cristim]

@coderabbitai review

[coderabbitai]

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 38 minutes and 49 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3d361e52-38e0-49fb-b8fa-04da331f17dc

📥 Commits

Reviewing files that changed from the base of the PR and between d7381bf and ff0cf66.

📒 Files selected for processing (47)

• frontend/src/__tests__/history-revoke-button.test.ts
• frontend/src/__tests__/permissions.test.ts
• frontend/src/api/index.ts
• frontend/src/api/purchases.ts
• frontend/src/history.ts
• frontend/src/permissions.generated.ts
• frontend/src/permissions.ts
• frontend/src/types.ts
• go.mod
• internal/analytics/collector_test.go
• internal/api/coverage_gaps_test.go
• internal/api/handler_config.go
• internal/api/handler_purchases.go
• internal/api/handler_purchases_revoke.go
• internal/api/handler_purchases_revoke_test.go
• internal/api/handler_purchases_test.go
• internal/api/mocks_test.go
• internal/api/router.go
• internal/auth/service_group_test.go
• internal/auth/types.go
• internal/auth/types_test.go
• internal/config/interfaces.go
• internal/config/store_postgres.go
• internal/config/store_postgres_pgxmock_test.go
• internal/config/types.go
• internal/config/validation.go
• internal/database/postgres/migrations/000066_purchase_history_revocation.down.sql
• internal/database/postgres/migrations/000066_purchase_history_revocation.up.sql
• internal/email/interfaces.go
• internal/email/nop_sender.go
• internal/email/sender.go
• internal/email/smtp_sender.go
• internal/email/templates.go
• internal/mocks/stores.go
• internal/purchase/coverage_extra_test.go
• internal/purchase/execution.go
• internal/purchase/mocks_test.go
• internal/purchase/scheduled_fire.go
• internal/scheduler/scheduler.go
• internal/scheduler/scheduler_test.go
• internal/server/app_test.go
• internal/server/handler.go
• internal/server/handler_ri_exchange_test.go
• internal/server/handler_test.go
• internal/server/interfaces.go
• internal/server/test_helpers_test.go
• internal/testutil/mocks.go

📝 Walkthrough

Walkthrough

This PR adds a complete purchase revocation feature allowing authenticated users to revoke completed Azure purchases within a 7-day free-cancel window. Changes span frontend UI (revoke button in history), backend API handler with Azure Reservations integration, RBAC authorization, database schema and storage, and comprehensive test coverage.

Changes

Purchase Revocation Feature

Layer / File(s)	Summary
Data model and schema foundation frontend/src/types.ts, internal/config/types.go, internal/config/interfaces.go, internal/database/postgres/migrations/000065_*	HistoryPurchase and PurchaseHistoryRecord extended with optional revocation fields (revocation_window_closes_at, revoked_at, revoked_via, support_case_id). Database migration adds revocation columns and partial index for window checks. RevocationWindowClosesAtFor helper computes Azure 7-day window.
Authorization framework and permissions internal/auth/types.go, internal/auth/types_test.go, internal/auth/service_group_test.go, frontend/src/permissions.generated.ts	ActionRevokeOwn and ActionRevokeAny constants define revocation RBAC matrix. DefaultUserPermissions() includes revoke-own:purchases for authenticated users. Permission count tests updated (11→12, 13→14) to reflect new grant. USER_PERMS includes 'revoke-own:purchases'.
Frontend and backend API contracts frontend/src/api/purchases.ts, frontend/src/api/index.ts, go.mod, internal/api/router.go	revokePurchase(purchaseId) client POSTs to /purchases/{purchaseId}/revoke and returns {status, revoked_at, revoked_via}. Router registers POST /api/purchases/{id}/revoke with AuthUser middleware. Azure SDK dependency promoted to direct (go.mod).
Backend handler: revocation orchestration and Azure integration internal/api/handler_purchases_revoke.go	revokePurchase handler validates input, enforces revoke-own/revoke-any RBAC with account-scope constraint, returns 422 for AWS/GCP, and for Azure: enforces 7-day return window, parses ARM reservation IDs, executes two-step refund flow (CalculateRefund + Return), maps client errors to 400, persists revocation state, and returns structured response.
Database storage layer: queries and updates internal/config/store_postgres.go, internal/config/store_postgres_pgxmock_test.go	SavePurchaseHistory binds revocation columns. GetPurchaseHistory, GetAllPurchaseHistory, and GetPurchaseHistoryFiltered SELECT revocation fields. queryPurchaseHistory scans nullable revocation columns and assigns them to PurchaseHistoryRecord. GetPurchaseHistoryByPurchaseID and MarkPurchaseRevoked implemented. pgxmock tests updated with 4 new revocation columns.
Frontend history UI: revoke button and interaction flow frontend/src/history.ts	canRevokeCompletedRow predicate gates revoke button to completed Azure rows with future revocation_window_closes_at and active user session. Button rendered with data-revoke-id attribute. Click handler confirms action, disables sibling buttons, calls api.revokePurchase, displays success/error toasts, and reloads history.
Purchase history stamping: revocation window capture internal/purchase/execution.go	savePurchaseHistory captures purchasedAt timestamp and stamps RevocationWindowClosesAt using config.RevocationWindowClosesAtFor so History UI can determine eligibility.
Handler tests: revocation flows and authorization internal/api/handler_purchases_revoke_test.go	Comprehensive unit tests covering auth/input validation (nil auth→403, empty ID→400), purchase-not-found (404), already-revoked detection, provider rejection (AWS/GCP→422), Azure window enforcement, two-step refund success/error mapping, ARM ID parsing, and RBAC authorization matrix (admin bypass, revoke-any, revoke-own with account constraint).
Coverage regression test: revocation window stamping internal/purchase/coverage_extra_test.go	Validates savePurchaseHistory stamps RevocationWindowClosesAt for Azure using Timestamp + AzureRevocationWindowDays, and leaves it nil for AWS/GCP.
Test infrastructure: mock store implementations internal/analytics/collector_test.go, internal/api/mocks_test.go, internal/mocks/stores.go, internal/purchase/mocks_test.go, internal/scheduler/scheduler_test.go, internal/server/handler_ri_exchange_test.go, internal/server/test_helpers_test.go	Mock ConfigStore implementations across test suites updated with GetPurchaseHistoryByPurchaseID and MarkPurchaseRevoked method stubs using testify-mock pattern.
Frontend tests: history revoke button UI gating frontend/src/__tests__/history-revoke-button.test.ts	Jest test suite verifies revoke button renders only for completed Azure rows with future revocation window and active user, and hidden for other providers, closed windows, already-revoked rows, and anonymous sessions.

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• LeanerCloud/CUDly#387: Both PRs modify frontend/src/history.ts row action rendering in renderActionCell and wiring in wireRowActionHandlers to add purchase action buttons (approval queue in #387; revoke button in this PR).

Suggested labels

feature/revocation, backend/api, backend/database, frontend/ui, testing/coverage

Poem

🐰 A rabbit hops through purchase rows with glee,
"Revoke this Azure buy within seven days, please!"
RBAC guards the gate, schemas track the deed,
Azure SDK returns the funds with speed.
Button clicks, permissions checked with care—
The refund window closes, yet revocation's fair! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the primary change: implementing in-app purchase revocation within provider free-cancel windows, with the issue reference providing additional context.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/290-wave3

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Rebased onto feat/multicloud-web-frontend (was 3 commits behind base; mergeStateStatus was DIRTY). Conflict resolution:

• internal/auth/service_group_test.go: dropped the PR's stale BuildAuthContext assertions that pre-dated Revamp authorization: group-membership-only (remove roles), require >=1 group per user #907 (group-membership-only authz). Kept the PR's updated GetUserPermissions counts (11 -> 12 / 13 -> 14 for the +revoke-own permission).
• internal/auth/service_test.go: dropped the PR's stale "user permissions even if group fetch fails" assertion. Under Revamp authorization: group-membership-only (remove roles), require >=1 group per user #907 a failing single-group fetch yields an empty permission set (no role fallback exists).

Fixed CI red lights from the previous push:

• gofmt: realigned struct-field padding in TestRevokePurchase_AzureReturnClientError.
• go mod tidy: promoted armreservations from indirect to direct (the revoke handler imports it directly post-rebase).
• go vet / build: replaced session.Role == "admin" in authorizeSessionRevoke with session.UserID == apiKeyAdminUserID. The Role field was removed from api.Session by Revamp authorization: group-membership-only (remove roles), require >=1 group per user #907; the new short-circuit mirrors the sibling authorizeSessionCancel / authorizeSessionApprove patterns. Group-based admins still pass via the HasPermissionAPI({admin, *}) wildcard match further down. Test sessions updated accordingly.

Free-cancel window enforcement (AzureRevocationWindowDays + windowClosesAt) and revoke-call idempotency (early-return on record.RevokedAt != nil) are unchanged.

go build ./..., go vet ./..., go test ./... (5043 tests / 38 packages), gofmt -l ., and npx tsc --noEmit are all clean locally.

@coderabbitai review

[coderabbitai]

@cristim: Thanks for the detailed rebase notes. The conflict resolutions look sound — dropping the stale BuildAuthContext assertions that pre-dated #907 and the role-fallback assertion are the right calls given the group-membership-only authz model. The session.Role == "admin" → session.UserID == apiKeyAdminUserID fix in authorizeSessionRevoke mirrors the established sibling patterns correctly. Kicking off a fresh review now.

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Wave 3: Gmail-style pre-fire delay + CR findings addressed

This push adds the scheduled-execution revoke path and folds in the three CodeRabbit findings from review f9c66c1e.

New feature: Gmail-style pre-fire delay (status=scheduled)

When purchase_delay_hours > 0, approval defers the cloud SDK call and stamps status=scheduled + scheduled_execution_at. The revoke endpoint now handles this case as a free cancel:

• revokePurchase tries GetExecutionByID first; if found with status=scheduled, routes to revokeScheduledExecution (no cloud call, returns {"status":"cancelled","message":"...no cost incurred."}).
• Falls through to the existing purchase_history lookup when no scheduled execution matches.
• revokeScheduledExecution uses CAS (CancelExecutionAtomic) to race safely with the scheduler; returns 410 if the scheduler fired first.
• authorizeSessionRevokeExecution mirrors authorizeSessionRevoke but scopes ownership via CreatedByUserID (not CloudAccountID).
• FireScheduledDelayedPurchases drives the scheduler tick: queries GetScheduledExecutionsDue, CAS-transitions scheduled->approved, calls executeAndFinalize.

CR findings addressed (review f9c66c1e)

1. Removed unused mockAuth in TestRevokePurchase_AzureCalcRefundClientError and TestRevokePurchase_AzureReturnClientError.
2. Migration 000065 audit constraints - idempotent CHECK constraints for revoked_via (allowed values), support_case_id (only with revoked_via='support-case'), and the revoked_at/revoked_via pair.
3. Frontend blank-status regression test - history-revoke-button.test.ts now has a 7th test: legacy rows with status='' must still show the Revoke button (pre-status-column rows remain revocable).
4. analytics mock hook-backed methods - GetPurchaseHistoryByPurchaseID and MarkPurchaseRevoked in collector_test.go now use hook fields instead of hardcoded no-ops.

Also fixed: getPurchaseDetails now returns a 404 ClientError (not a wrapped fmt.Errorf) when GetExecutionByID errors, so the handler's error classification is consistent with the rest of the revoke/execute paths.

@coderabbitai review

[coderabbitai]

@cristim: The Wave 3 additions look well-structured — the CAS-based CancelExecutionAtomic race guard and the 410 response when the scheduler fires first are exactly the right primitives for the pre-fire cancel window. The scoping of authorizeSessionRevokeExecution via CreatedByUserID (vs. CloudAccountID for completed-purchase revoke) is a clean separation that tracks the execution ownership model correctly. Good to see the migration 000065 CHECK constraints landed too — the (revoked_at IS NOT NULL) = (revoked_via IS NOT NULL) pair constraint and the support_case_id conditional are solid audit hygiene. Triggering a fresh review now.

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-05T08:48:15.584Z
Learning: In LeanerCloud/CUDly PR `#804`, `RevocationWindowClosesAt` is stamped at purchase-write time in `internal/purchase/execution.go` (`savePurchaseHistory`) via a shared helper `config.RevocationWindowClosesAtFor` and constant `config.AzureRevocationWindowDays`. The backend revoke handler (`revokeAzurePurchase`) reads the stamped column as the source of truth and falls back to recomputing from `Timestamp` only for legacy rows (where the column is NULL). The frontend gate `canRevokeCompletedRow` in `frontend/src/history.ts` depends on `revocation_window_closes_at` being present to show the Revoke button.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-05T08:48:15.584Z
Learning: In LeanerCloud/CUDly PR `#804`, `callAzureReturn` in `internal/api/handler_purchases_revoke.go` rejects an order-only ARM path (empty `reservationID`) with a 422 instead of submitting an empty Return call to Azure.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 507
File: frontend/src/app.ts:388-395
Timestamp: 2026-05-20T08:26:42.493Z
Learning: In `frontend/src/app.ts` (CUDly project), `openArcheraOfferModal('purchase')` is intentionally called immediately after the `executePurchase` approval-submission API call returns success (in `handleExecutePurchase` and `handleFanOutExecute`), NOT after the async email-link execution completes. This timing was explicitly revised from the original issue `#499` requirement. The offer is gated on the submission success path only (never on failure/throw), and the fan-out path requires at least one successful submission (`succeeded > 0`).

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 507
File: frontend/src/app.ts:388-395
Timestamp: 2026-05-20T08:26:36.680Z
Learning: In `frontend/src/app.ts` (CUDly project), `openArcheraOfferModal('purchase')` is intentionally called immediately after the `executePurchase` approval-submission API call returns success (in `handleExecutePurchase` and `handleFanOutExecute`), NOT after the async email-link execution completes. This timing was explicitly revised from the original issue `#499` requirement. The offer is gated on the submission success path only (never on failure/throw), and the fan-out path requires at least one successful submission (`succeeded > 0`).

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-05T08:48:15.584Z
Learning: In LeanerCloud/CUDly, the purchase-history revocation migration is numbered 000065 (`000065_purchase_history_revocation.{up,down}.sql`). It was previously renumbered several times (000057 → 000059 → 000060 → 000064 → 000065) due to collisions with migrations merged into the base branch. 000064 is occupied by `000064_relocate_purchaser_group` on the base branch.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Rebased on feat/multicloud-web-frontend. Resolved conflicts in internal/api/handler_purchases.go (kept HEAD's opaque 404 message for UUID enumeration safety, issue #431) and handler_purchases_test.go (kept HEAD's explanatory comment for grantAdmin).

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

MockConfigStore consolidation (fc130d4)

This commit eliminates the maintenance burden flagged in the PR analysis: every new StoreInterface method had to be stubbed in 3+ separate MockConfigStore definitions across internal/api, internal/purchase, and internal/scheduler.

What changed

internal/mocks/stores.go is now the canonical mock. It gained:

• All Fn-override fields previously local to the api and purchase mocks (GetCloudAccountFn, GetPurchasePlanFn, SetPlanAccountsFn, SavePurchaseExecutionFn, GetPlanAccountsFn, and 6 others), so callers using those fields keep working without changes
• 8 interface methods that were missing from the shared mock but present in all per-package variants: GetExecutionsByStatuses, GetPlannedExecutions, GetStaleApprovedExecutions, ListStuckExecutions, GetScheduledExecutionsDue, MarkCollectionStarted, ClearCollectionStarted, StampRIExchangeApprovedBy
• isExpected guards on recommendation-cache, RI-utilization, and CancelExecutionAtomic methods to preserve the "opt-in" behaviour the per-package mocks provided (no panic when tests call these without explicit .On() expectations)
• Compile-time check: var _ config.StoreInterface = (*MockConfigStore)(nil)

Per-package files each reduced to a single type alias line:

type MockConfigStore = mocks.MockConfigStore

The scheduler test also had stray suppression/Tx method stubs added in a later commit; those are removed since the methods already live on the shared mock.

Intentionally left local

• internal/analytics/collector_test.go - mockConfigStore (lowercase, hook-field only, no testify embedding, analytics-specific subset)
• internal/server/test_helpers_test.go - mockConfigStoreForHealth (all-zero-value stubs, distinct name)
• internal/server/handler_ri_exchange_test.go - mockConfigStoreForExchange (per-test struct)
• internal/server/handler_coverage_test.go - mockConfigStoreForExchangeComplete/Fail/Stale (per-scenario stubs)

Net LOC delta

+274 insertions / -1601 deletions = -1327 lines net across 4 files. All 5159 tests pass (the 2 pre-existing TestLogin_WithMFA_NoSecret failures in internal/auth are unrelated).

@coderabbitai review

[coderabbitai]

@cristim: The consolidation approach is well-executed — using a type alias (type MockConfigStore = mocks.MockConfigStore) rather than a type definition is the right call here, as it preserves the Fn-override field access at all existing call sites without any changes. A few things worth calling out:

• The isExpected guards on recommendation-cache, RI-utilization, and CancelExecutionAtomic are the correct mechanism: they preserve the per-package mocks' "silent no-op unless explicitly registered" semantics without breaking tests that don't set up .On() expectations for those methods.
• The compile-time check var _ config.StoreInterface = (*MockConfigStore)(nil) is good insurance — it'll surface any future interface drift at build time rather than at test runtime.
• The four intentionally-local exceptions are well-reasoned: the analytics mock's hook-field-only / no-testify-embedding model is architecturally different enough to stay separate, and the server per-scenario stubs are too narrow to benefit from the shared mock.

The -1327 net LOC delta is a meaningful reduction in maintenance surface. Triggering a fresh review now.

---

✏️ Learnings added

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/804

Timestamp: 2026-06-05T12:28:20.780Z
Learning: In LeanerCloud/CUDly, `internal/mocks/stores.go` is the canonical `MockConfigStore` (testify/mock embedding, Fn-override fields, isExpected guards). Per-package files in `internal/api`, `internal/purchase`, and `internal/scheduler` were reduced to a single type alias line: `type MockConfigStore = mocks.MockConfigStore`. Four intentionally-local mocks remain: `mockConfigStore` (lowercase, hook-field only) in `internal/analytics/collector_test.go`; `mockConfigStoreForHealth` in `internal/server/test_helpers_test.go`; and `mockConfigStoreForExchange` / per-scenario variants in `internal/server/handler_ri_exchange_test.go` and `handler_coverage_test.go`. A compile-time check `var _ config.StoreInterface = (*MockConfigStore)(nil)` lives in the shared mock.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-05T08:48:15.584Z
Learning: In LeanerCloud/CUDly PR `#804`, `RevocationWindowClosesAt` is stamped at purchase-write time in `internal/purchase/execution.go` (`savePurchaseHistory`) via a shared helper `config.RevocationWindowClosesAtFor` and constant `config.AzureRevocationWindowDays`. The backend revoke handler (`revokeAzurePurchase`) reads the stamped column as the source of truth and falls back to recomputing from `Timestamp` only for legacy rows (where the column is NULL). The frontend gate `canRevokeCompletedRow` in `frontend/src/history.ts` depends on `revocation_window_closes_at` being present to show the Revoke button.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-05T08:48:15.584Z
Learning: In LeanerCloud/CUDly PR `#804`, `callAzureReturn` in `internal/api/handler_purchases_revoke.go` rejects an order-only ARM path (empty `reservationID`) with a 422 instead of submitting an empty Return call to Azure.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 507
File: frontend/src/app.ts:388-395
Timestamp: 2026-05-20T08:26:42.493Z
Learning: In `frontend/src/app.ts` (CUDly project), `openArcheraOfferModal('purchase')` is intentionally called immediately after the `executePurchase` approval-submission API call returns success (in `handleExecutePurchase` and `handleFanOutExecute`), NOT after the async email-link execution completes. This timing was explicitly revised from the original issue `#499` requirement. The offer is gated on the submission success path only (never on failure/throw), and the fan-out path requires at least one successful submission (`succeeded > 0`).

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 507
File: frontend/src/app.ts:388-395
Timestamp: 2026-05-20T08:26:36.680Z
Learning: In `frontend/src/app.ts` (CUDly project), `openArcheraOfferModal('purchase')` is intentionally called immediately after the `executePurchase` approval-submission API call returns success (in `handleExecutePurchase` and `handleFanOutExecute`), NOT after the async email-link execution completes. This timing was explicitly revised from the original issue `#499` requirement. The offer is gated on the submission success path only (never on failure/throw), and the fan-out path requires at least one successful submission (`succeeded > 0`).

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-03T16:11:08.349Z
Learning: In the LeanerCloud/CUDly repository, `MockConfigStore` uses function-hook fields (e.g., `SetPlanAccountsFn`, `SaveAccountServiceOverrideFn`) to allow individual tests to wire in spy/stub behaviour. When adding new store methods to the mock, follow this same hook-field pattern rather than embedding logic directly in the mock struct.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Adversarial review pass on top of the existing commits. Three real defects found and addressed; remaining surfaces verified clean.

Findings + fixes (each with a regression test that fails pre-fix, passes post-fix):

• A3 (critical, financially destructive): CancelExecutionAtomic's SQL guard is WHERE status IN ('pending','notified') -- it never matched status='scheduled' rows. The new pre-fire-delay revoke handler always 410'd on the happy path; tests were green because mocks defaulted the CAS to (true,"cancelled",nil). The user would pay AND see "cancelled" -- the worst outcome. Fix: new CancelScheduledExecutionAtomic with the correct WHERE status = 'scheduled' guard, kept distinct from the cancel path so the two race outcomes (410 "window closed" vs 409 "not pending") stay disambiguated. Commit 34aae244b.

• B1 (UX-vs-RBAC drift, same shape as fix(purchases): gate scheduled-purchase management on creator-scope ownership (closes #950) #995): canRevokeCompletedRow only checked getCurrentUser() truthiness, so the inline Revoke button rendered for every signed-in user; the backend then 403d on click. Aligned with peer predicates (canCancelPendingRow, canApprovePendingRow, canRetryFailedRow) which all check canAccess. Added revoke-own / revoke-any to the closed Action union so future drift is a compile error. Commit 00fc2c6fb.

• H1 (migration collision): Base branch landed 000065_enforce_min_one_admin via fix(auth): close last-admin TOCTOU with deferred DB trigger #921. Renumbered purchase_history_revocation 000065 -> 000066 and updated the internal/config/types.go reference. Commit 28deef081.

• Adjacent test parity: frontend/src/__tests__/permissions.test.ts expected 11 user-role verbs but USER_PERMS now has 12 (the revoke-own:purchases PR feat(purchases): in-app revocation within free-cancel window (closes #290) #804 added). Existing oversight -- updated the expected list. Commit ff0cf66fc.

Surfaces walked, no real defect:

• A1/A2: Stranded-row recovery handled via ErrAuditLoss + claimAndRedrive; SQL row-lock serializes concurrent ticks.
• A4/A5: time.Now() vs NOW() differ only in TZ display; TIMESTAMPTZ comparison is on the absolute instant.
• B2/B3/B4/B5: No new token / public-route revoke; account-scope semantics already documented as a sec(purchases): standard user can pause/resume other users' scheduled purchases (no ownership gate) #950 follow-up.
• C1-C4: Migration ordering, down-migration drops constraints before columns, value-set matches code, WHERE scheduled_execution_at IS NOT NULL filter present.
• D1-D3: No new replay surface; CAS handles dedup.
• E1-E4: Plain-text email template -- no XSS surface.
• F1-F4: t.Cleanup + AssertExpectations consistent; no time.Sleep in tests; no PII in logs.
• G1-G2: No TODOs / dead fixtures in PR-scope code.
• H2: TestLogin_WithMFA_NoSecret failures are pre-existing on the base (file untouched by this PR; attributed to sec(auth): login endpoint leaks MFA enrollment status via distinct error messages #388).

Verification: full go test ./internal/... green except the H2 pre-existing failures; full npm test green (72 suites, 2353 passing); npx tsc --noEmit clean.

@coderabbitai review

[coderabbitai]

@cristim: Outstanding adversarial work — A3 is exactly the class of bug that adversarial review is designed to catch. The mock defaulting the CAS to (true,"cancelled",nil) made the test suite completely blind to the SQL guard mismatch, and the user-visible outcome (charged + shown "cancelled") is about as bad as it gets for a revocation feature. The fix design is right: keeping CancelScheduledExecutionAtomic (WHERE status = 'scheduled') distinct from CancelExecutionAtomic (WHERE status IN ('pending','notified')) is essential so the 410 "scheduler fired first" and 409 "not pending" race outcomes stay unambiguous.

B1 is a clean catch too — canAccess is the correct gate, and adding revoke-own/revoke-any to the closed Action union makes this class of drift a compile error going forward rather than a runtime 403.

H1 renumber (000066) and the permissions.test.ts parity fix are good housekeeping.

Triggering a fresh review now.

🐇 🔍

---

✏️ Learnings added

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/804

Timestamp: 2026-06-06T06:41:06.830Z
Learning: In LeanerCloud/CUDly PR `#804`, the purchase-history revocation migration is numbered 000066 (`000066_purchase_history_revocation.{up,down}.sql`). It was renumbered multiple times due to collisions: 000057 → 000059 → 000060 → 000064 → 000065 → 000066. 000065 is occupied by `000065_enforce_min_one_admin` (merged via `#921`) on the base branch.

---

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/804

Timestamp: 2026-06-06T06:41:06.830Z
Learning: In LeanerCloud/CUDly PR `#804`, `CancelScheduledExecutionAtomic` (`WHERE status = 'scheduled'`) is distinct from `CancelExecutionAtomic` (`WHERE status IN ('pending','notified')`). The pre-fire-delay revoke path uses `CancelScheduledExecutionAtomic`; the 410 "window closed" / 409 "not pending" race outcomes must remain disambiguated. Using `CancelExecutionAtomic` for scheduled-execution revocation is a critical bug (the execution fires and the user is charged while the UI shows "cancelled").

---

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/804

Timestamp: 2026-06-06T06:41:06.830Z
Learning: In LeanerCloud/CUDly PR `#804`, `canRevokeCompletedRow` in `frontend/src/history.ts` must check `canAccess` (not just `getCurrentUser()` truthiness) to align with peer predicates `canCancelPendingRow`, `canApprovePendingRow`, and `canRetryFailedRow`. `revoke-own` and `revoke-any` were added to the closed `Action` union to make future UX-vs-RBAC drift a compile error.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

⛔ Learnings removed

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-05T08:48:15.584Z
Learning: In LeanerCloud/CUDly PR `#804`, `callAzureReturn` in `internal/api/handler_purchases_revoke.go` rejects an order-only ARM path (empty `reservationID`) with a 422 instead of submitting an empty Return call to Azure.

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-05T08:48:15.584Z
Learning: In LeanerCloud/CUDly PR `#804`, `RevocationWindowClosesAt` is stamped at purchase-write time in `internal/purchase/execution.go` (`savePurchaseHistory`) via a shared helper `config.RevocationWindowClosesAtFor` and constant `config.AzureRevocationWindowDays`. The backend revoke handler (`revokeAzurePurchase`) reads the stamped column as the source of truth and falls back to recomputing from `Timestamp` only for legacy rows (where the column is NULL). The frontend gate `canRevokeCompletedRow` in `frontend/src/history.ts` depends on `revocation_window_closes_at` being present to show the Revoke button.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-05T08:48:15.584Z
Learning: In LeanerCloud/CUDly PR `#804`, `callAzureReturn` in `internal/api/handler_purchases_revoke.go` rejects an order-only ARM path (empty `reservationID`) with a 422 instead of submitting an empty Return call to Azure.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 507
File: frontend/src/app.ts:388-395
Timestamp: 2026-05-20T08:26:42.493Z
Learning: In `frontend/src/app.ts` (CUDly project), `openArcheraOfferModal('purchase')` is intentionally called immediately after the `executePurchase` approval-submission API call returns success (in `handleExecutePurchase` and `handleFanOutExecute`), NOT after the async email-link execution completes. This timing was explicitly revised from the original issue `#499` requirement. The offer is gated on the submission success path only (never on failure/throw), and the fan-out path requires at least one successful submission (`succeeded > 0`).

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 507
File: frontend/src/app.ts:388-395
Timestamp: 2026-05-20T08:26:36.680Z
Learning: In `frontend/src/app.ts` (CUDly project), `openArcheraOfferModal('purchase')` is intentionally called immediately after the `executePurchase` approval-submission API call returns success (in `handleExecutePurchase` and `handleFanOutExecute`), NOT after the async email-link execution completes. This timing was explicitly revised from the original issue `#499` requirement. The offer is gated on the submission success path only (never on failure/throw), and the fan-out path requires at least one successful submission (`succeeded > 0`).

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-05T12:28:20.780Z
Learning: In LeanerCloud/CUDly, `internal/mocks/stores.go` is the canonical `MockConfigStore` (testify/mock embedding, Fn-override fields, isExpected guards). Per-package files in `internal/api`, `internal/purchase`, and `internal/scheduler` were reduced to a single type alias line: `type MockConfigStore = mocks.MockConfigStore`. Four intentionally-local mocks remain: `mockConfigStore` (lowercase, hook-field only) in `internal/analytics/collector_test.go`; `mockConfigStoreForHealth` in `internal/server/test_helpers_test.go`; and `mockConfigStoreForExchange` / per-scenario variants in `internal/server/handler_ri_exchange_test.go` and `handler_coverage_test.go`. A compile-time check `var _ config.StoreInterface = (*MockConfigStore)(nil)` lives in the shared mock.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-03T16:11:08.349Z
Learning: In the LeanerCloud/CUDly repository, `MockConfigStore` uses function-hook fields (e.g., `SetPlanAccountsFn`, `SaveAccountServiceOverrideFn`) to allow individual tests to wire in spy/stub behaviour. When adding new store methods to the mock, follow this same hook-field pattern rather than embedding logic directly in the mock struct.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.