feat(audit): stamp actor on execution + RI-exchange state transitions (closes #1009)

[cristim]

Summary

• Adds transitioned_by UUID NULL REFERENCES users(id) ON DELETE SET NULL and transitioned_at TIMESTAMPTZ NULL to purchase_executions, ri_exchange_history, and account_registrations (migration 000066).
• Extends TransitionExecutionStatus, TransitionRIExchangeStatus, and TransitionRegistrationStatus with a trailing actor *string parameter: session UUID for human-initiated transitions, nil for system paths (reaper, scheduler, token approvals).
• Adds validUUIDPtrOrNil helper in internal/api/validation.go to guard non-UUID reviewer IDs (e.g. "admin-api-key") before FK insertion. Mirrors the CancelExecutionAtomic(actor) pattern from feat(purchases): in-app revocation within free-cancel window (closes #290) #804.

Test plan

• go test ./internal/api/... ./internal/config/... ./internal/purchase/... ./internal/analytics/... ./internal/scheduler/... ./internal/server/... -- all pass (2861 passed)
• TestHandler_pausePlannedPurchase_ActorStamped -- session UUID threaded as actor
• TestHandler_expireIfStale_SystemActorIsNil -- system expiry path passes nil
• TestApproveRIExchange_SessionActorStamped and TestRejectRIExchange_TokenPathActorIsNil -- RI exchange actor paths
• TestValidUUIDPtrOrNil_* (3 tests) -- validation helper edge cases
• TestHandler_TransitionRegistrationStatus_ActorStamped and TestHandler_TransitionRegistrationStatus_NonUUIDActorIsNil -- registration actor paths
• Migration 000066 applies and rolls back cleanly (IF NOT EXISTS / IF EXISTS guards)

Closes #1009

Summary by CodeRabbit

Release Notes

• New Features
  • Status transitions for purchases, RI exchanges, and account registrations now record the user who initiated the action and the timestamp of the transition. This enables improved audit tracking and accountability.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fc2bae95-ef97-4d8a-8441-a6e35169b561

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR adds audit actor attribution to cloud commitment state transitions. Three store methods now accept an actor *string parameter to record WHO performed each execution pause/resume/cancel, RI exchange approval/rejection, and account registration approval/rejection. New transitioned_by and transitioned_at columns are added to the schema via idempotent migration. Handlers thread session user IDs through, while system-initiated transitions (scheduler ticks, recovery workers) pass nil. Test coverage includes new regression tests verifying actor stamping behavior.

Changes

Audit actor stamping on state transitions

Layer / File(s)	Summary
Schema migration and store interface contract internal/database/postgres/migrations/000066_audit_actor_stamps.up.sql, 000066_audit_actor_stamps.down.sql, internal/config/interfaces.go	Database migration idempotently adds transitioned_by (UUID FK) and transitioned_at (TIMESTAMPTZ) columns to purchase_executions, ri_exchange_history, and account_registrations. Store interface extends TransitionExecutionStatus, TransitionRIExchangeStatus, and TransitionRegistrationStatus to accept actor *string with documentation explaining nil vs. non-nil semantics.
PostgreSQL store implementation of transition methods internal/config/store_postgres.go, internal/config/store_postgres_registrations.go	Both transition methods updated to accept actor *string and record transition metadata (actor + timestamp) in SQL UPDATE statements when status changes succeed.
UUID validation helper for actor handling internal/api/validation.go	New validUUIDPtrOrNil helper safely converts optional string pointer to UUID pointer (when valid) or nil (when pointer is nil or not a UUID), used by registration handlers to validate reviewed_by before passing as actor.
Planned purchase handlers passing actor to execution transitions internal/api/handler_purchases.go	Handlers pausePlannedPurchase, resumePlannedPurchase, runPlannedPurchase, and deletePlannedPurchase now pass session user ID via resolveCreatorUserID(session) into TransitionExecutionStatus; cancelOrRecoverExecution helper updated to accept and forward actor *string.
Registration approval/rejection handlers with actor stamping internal/api/handler_registrations.go	approveRegistration and rejectRegistration now call TransitionRegistrationStatus with actor derived from reg.ReviewedBy after validation via validUUIDPtrOrNil, allowing non-UUID strings to be converted to nil actor.
RI exchange handlers with context-appropriate actor attribution internal/api/handler_ri_exchange.go	Token-based approval/rejection pass nil actor (system/token-initiated); session-based approval passes resolveCreatorUserID(session) derived from current user.
Expiring stale executions with system-initiated actor internal/api/handler_history.go	expireIfStale now passes explicit nil actor when transitioning pending/notified executions to expired, marking the transition as system-initiated.
Purchase approval and execution manager with system actor stamps internal/purchase/approvals.go, internal/purchase/manager.go	Approval manager's ApproveAndExecute and execution manager's recovery helpers (claimAndRedrive, safeFail) now pass explicit nil actor for system-initiated transitions.
Reaper system transitions with nil actor attribution internal/purchase/reaper.go	reapOne now passes explicit nil actor when transitioning stuck executions, marking the status change as system-initiated by the background reaper process.
Mock store implementations across all test packages internal/api/mocks_test.go, internal/mocks/stores.go, internal/purchase/mocks_test.go, internal/scheduler/scheduler_test.go, internal/server/test_helpers_test.go, test doubles in internal/analytics/collector_test.go	Test mocks updated to include actor *string parameter in method signatures and forward it to mock call recording.
Handler test expectations updated for new transition call signatures internal/api/handler_purchases_test.go, internal/api/handler_ri_exchange_test.go, internal/api/handler_history_test.go, internal/api/handler_per_account_perms_test.go, internal/api/handler_test.go, internal/api/router_660_permission_flips_test.go, internal/api/router_handlers_test.go	Test expectations pass mock.Anything when verifying TransitionExecutionStatus and TransitionRIExchangeStatus calls; updated across pause/resume/run/delete purchase tests, RI-exchange approval/rejection tests, and permission-gate verification tests.
New tests verifying actor attribution on transitions internal/api/handler_history_test.go, internal/api/handler_purchases_test.go, internal/api/handler_ri_exchange_test.go, internal/api/coverage_extras_test.go	New regression tests (issue #1009) assert session-user actor values are correctly stamped in transitioned_by for human-initiated actions; system-initiated transitions pass nil actor. Includes TestHandler_expireIfStale_SystemActorIsNil, TestHandler_pausePlannedPurchase_ActorStamped, TestApproveRIExchange_SessionActorStamped, and TestRejectRIExchange_TokenPathActorIsNil.
Purchase approval and recovery test expectations with actor parameters internal/purchase/approvals_test.go, internal/purchase/coverage_extra_test.go, internal/purchase/manager_test.go	Tests updated to include (*string)(nil) argument in mock expectations for system-initiated transitions in approval processing, CAS recovery, safe-fail stranded execution handling, and reap cycles.
Reaper, store nil-DB, and pgxmock test updates internal/purchase/reaper_test.go, internal/config/store_postgres_nil_db_test.go, internal/config/store_postgres_pgxmock_test.go	Tests updated to pass (*string)(nil) argument when invoking transition methods; pgxmock tests also updated to expect additional bound argument in SQL UPDATE calls.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• LeanerCloud/CUDly#309: Updates per-account permission test expectations for TransitionExecutionStatus calls, directly overlapping at TestPerAccountPerms_PlannedPurchase_AllowedAccountPlanSucceeds which this PR further adjusts with the new actor parameter.
• LeanerCloud/CUDly#590: Modifies the session-auth RI exchange approval path in internal/api/handler_ri_exchange.go, overlapping with this PR's changes to thread actor through RI-exchange transitions.
• LeanerCloud/CUDly#726: Updates permission-gate tests in internal/api/router_660_permission_flips_test.go for the pause/delete planned-purchase paths, which this PR further updates to match the new TransitionExecutionStatus signature with actor parameter.

🐰 Hopping through audit trails, we stamp each state—
Who paused, resumed, cancelled? Now the rows relate!
System or human, nil or user-ID,
Forensics are ready, compliance is free! 🔍✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 56.70% 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
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately summarizes the main change: adding actor/reviewer stamps to state transition methods for audit purposes, and correctly references the closed issue #1009.
Linked Issues check	✅ Passed	The PR successfully implements all coding requirements from issue #1009: schema migration adds transitioned_by/transitioned_at columns; store methods (TransitionExecutionStatus, TransitionRIExchangeStatus, TransitionRegistrationStatus) accept actor parameter; handlers thread session user IDs; system paths pass nil; mocks updated; tests verify actor stamping.
Out of Scope Changes check	✅ Passed	All changes are directly related to the stated objectives of auditing state transitions via actor stamping. No extraneous refactoring, unrelated feature work, or scope creep detected in the changeset.

_{✏️ 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/1009-audit-actor-stamps

---

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

[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.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

internal/config/store_postgres.go (2)

1753-1768: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

RI exchange terminal transitions still leave stale transition metadata.

This stamps pending -> processing/cancelled, but CompleteRIExchange and FailRIExchange below still change status without refreshing transitioned_by / transitioned_at, and internal/api/handler_ri_exchange.go calls both on the system-driven completion/failure paths (Lines 1137-1181). A completed or failed row will therefore keep the earlier approval stamp, or no stamp at all, instead of the actual terminal transition time. Those UPDATEs should null the actor and set transitioned_at = NOW() as well. Based on PR objectives, system-initiated transitions should still stamp transition time even when the actor is nil.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/config/store_postgres.go` around lines 1753 - 1768,
CompleteRIExchange and FailRIExchange currently update status without touching
transitioned_by/transitioned_at, leaving stale or missing transition stamps;
update their UPDATE statements (the ones in PostgresStore methods
CompleteRIExchange and FailRIExchange) to set transitioned_by = NULL (or the
actor param when provided) and transitioned_at = NOW() and adjust query
parameters accordingly so system-driven terminal transitions record
transitioned_at even when actor is nil. Ensure the UPDATE mirrors the
TransitionRIExchangeStatus behavior for stamping transition metadata while
preserving existing status, error, completed_at, and other returned fields.

---

827-847: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

CancelExecutionAtomic still bypasses the new transition audit fields.

TransitionExecutionStatus now stamps transitioned_by / transitioned_at, but the normal purchase-cancel flow in internal/api/handler_purchases.go (Lines 761-770) still goes through CancelExecutionAtomic, whose UPDATE only touches status, cancelled_by, and updated_at. That leaves history/email cancellations with NULL transition audit metadata, so execution cancellation is only fixed for the planned-purchase path. Extend CancelExecutionAtomic with the same nullable UUID actor stamp and thread it through its callers. Based on PR objectives, execution state transitions are supposed to be auditable end-to-end.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/config/store_postgres.go` around lines 827 - 847,
CancelExecutionAtomic currently updates only status, cancelled_by and
updated_at, leaving transitioned_by/transitioned_at NULL; change
CancelExecutionAtomic signature to accept an actor *string (nullable UUID) and
update its SQL to set transitioned_by = $X and transitioned_at = NOW()
(mirroring TransitionExecutionStatus), return the same execution fields as
queryExecutions if applicable, and thread the new actor parameter through all
callers (including the normal purchase cancel flow in
internal/api/handler_purchases.go) so the actor value is supplied when
cancellations occur; ensure parameter ordering in the UPDATE call matches the
new placeholder positions and adjust any invocation sites to compile.

🧹 Nitpick comments (1)

internal/api/handler_per_account_perms_test.go (1)

962-963: ⚡ Quick win

Assert the actor value, not just its presence.

This is a human-initiated transition path, but the test currently accepts any actor value. Match actor to permsScopedUserID to actually verify propagation.

Proposed test tightening

-	mockStore.On("TransitionExecutionStatus", ctx, executionID, mock.Anything, "paused", mock.Anything).
+	mockStore.On("TransitionExecutionStatus", ctx, executionID, mock.Anything, "paused",
+		mock.MatchedBy(func(actor *string) bool {
+			return actor != nil && *actor == permsScopedUserID
+		}),
+	).
 		Return(transitoned, nil)
@@
-	mockStore.AssertCalled(t, "TransitionExecutionStatus", ctx, executionID, mock.Anything, "paused", mock.Anything)
+	mockStore.AssertCalled(t, "TransitionExecutionStatus", ctx, executionID, mock.Anything, "paused",
+		mock.MatchedBy(func(actor *string) bool {
+			return actor != nil && *actor == permsScopedUserID
+		}),
+	)

Also applies to: 982-982

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/handler_per_account_perms_test.go` around lines 962 - 963, The
mock expectation for mockStore.TransitionExecutionStatus currently accepts any
actor value; tighten it to assert the actor equals permsScopedUserID so the test
verifies the human-initiated actor is propagated. Update the mock.On call for
TransitionExecutionStatus (and the similar occurrence around the other
expectation) to use mock.MatchedBy or the specific value permsScopedUserID
instead of mock.Anything for the actor parameter, keeping ctx, executionID and
the "paused" status the same and returning transitoned, nil.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/api/coverage_extras_test.go`:
- Around line 405-423: The test
TestHandler_TransitionRegistrationStatus_ActorStamped is calling
mockStore.TransitionRegistrationStatus directly which bypasses the handler
logic; change the test to exercise the real approve/reject handler (the function
under test that calls TransitionRegistrationStatus) instead of the store method
so the actor-stamping path is validated end-to-end. Set up the same mock
expectation on mockStore.On("TransitionRegistrationStatus", ctx, reg, "pending",
mock.MatchedBy(...)) and then invoke the handler entrypoint (e.g., the HTTP
handler or the handler method that processes registration approvals/rejections)
with a request/context containing the reviewer actor ID (use
validUUIDPtrOrNil(reg.ReviewedBy) or inject the actor into context/header as the
handler expects), then assert no error and mockStore.AssertExpectations(t).

In `@internal/purchase/approvals.go`:
- Around line 147-149: The call to m.config.TransitionExecutionStatus currently
always passes nil for the transitionedBy parameter which clears actor
attribution; change the approval flow so that when the approval is
human/session-authenticated you pass the session user's UUID into
TransitionExecutionStatus (instead of nil) while keeping nil for
token/SQS/system paths—locate the session approval branch that calls
TransitionExecutionStatus (the same method referenced as
m.config.TransitionExecutionStatus with executionID and status args) and thread
the session user ID through to that function call so transitioned_by is
persisted for human-initiated approvals.

---

Outside diff comments:
In `@internal/config/store_postgres.go`:
- Around line 1753-1768: CompleteRIExchange and FailRIExchange currently update
status without touching transitioned_by/transitioned_at, leaving stale or
missing transition stamps; update their UPDATE statements (the ones in
PostgresStore methods CompleteRIExchange and FailRIExchange) to set
transitioned_by = NULL (or the actor param when provided) and transitioned_at =
NOW() and adjust query parameters accordingly so system-driven terminal
transitions record transitioned_at even when actor is nil. Ensure the UPDATE
mirrors the TransitionRIExchangeStatus behavior for stamping transition metadata
while preserving existing status, error, completed_at, and other returned
fields.
- Around line 827-847: CancelExecutionAtomic currently updates only status,
cancelled_by and updated_at, leaving transitioned_by/transitioned_at NULL;
change CancelExecutionAtomic signature to accept an actor *string (nullable
UUID) and update its SQL to set transitioned_by = $X and transitioned_at = NOW()
(mirroring TransitionExecutionStatus), return the same execution fields as
queryExecutions if applicable, and thread the new actor parameter through all
callers (including the normal purchase cancel flow in
internal/api/handler_purchases.go) so the actor value is supplied when
cancellations occur; ensure parameter ordering in the UPDATE call matches the
new placeholder positions and adjust any invocation sites to compile.

---

Nitpick comments:
In `@internal/api/handler_per_account_perms_test.go`:
- Around line 962-963: The mock expectation for
mockStore.TransitionExecutionStatus currently accepts any actor value; tighten
it to assert the actor equals permsScopedUserID so the test verifies the
human-initiated actor is propagated. Update the mock.On call for
TransitionExecutionStatus (and the similar occurrence around the other
expectation) to use mock.MatchedBy or the specific value permsScopedUserID
instead of mock.Anything for the actor parameter, keeping ctx, executionID and
the "paused" status the same and returning transitoned, nil.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0aa755e5-8068-4f4a-a053-3b18de5eb405

📥 Commits

Reviewing files that changed from the base of the PR and between cbdc4be and f468037.

📒 Files selected for processing (33)

• internal/analytics/collector_test.go
• internal/api/coverage_extras_test.go
• internal/api/handler_history.go
• internal/api/handler_history_test.go
• internal/api/handler_per_account_perms_test.go
• internal/api/handler_purchases.go
• internal/api/handler_purchases_test.go
• internal/api/handler_registrations.go
• internal/api/handler_ri_exchange.go
• internal/api/handler_ri_exchange_test.go
• internal/api/handler_test.go
• internal/api/mocks_test.go
• internal/api/router_660_permission_flips_test.go
• internal/api/router_handlers_test.go
• internal/api/validation.go
• internal/config/interfaces.go
• internal/config/store_postgres.go
• internal/config/store_postgres_nil_db_test.go
• internal/config/store_postgres_pgxmock_test.go
• internal/config/store_postgres_registrations.go
• internal/database/postgres/migrations/000066_audit_actor_stamps.down.sql
• internal/database/postgres/migrations/000066_audit_actor_stamps.up.sql
• internal/mocks/stores.go
• internal/purchase/approvals.go
• internal/purchase/approvals_test.go
• internal/purchase/coverage_extra_test.go
• internal/purchase/manager.go
• internal/purchase/manager_test.go
• internal/purchase/mocks_test.go
• internal/purchase/reaper.go
• internal/purchase/reaper_test.go
• internal/scheduler/scheduler_test.go
• internal/server/test_helpers_test.go

[cristim]

Rebase onto feat/multicloud-web-frontend stopped - semantic conflict requires human decision.

Conflict in internal/api/handler_history.go: the PR adds expireIfStale (with nil actor for system transitions), but the base branch no longer has the same surrounding context at that location - likely because issue #1032 (already merged) restructured or removed expireIfStale from the GET path. Resolving this conflict requires deciding:

1. Whether expireIfStale still exists on the post-fix(api): getHistory GET writes (expireIfStale) + drops empty-account rows for scoped users (#621 regression) #1032 base, and if so where it was moved to.
2. Whether the actor-stamping added by this PR should apply to the new post-fix(api): getHistory GET writes (expireIfStale) + drops empty-account rows for scoped users (#621 regression) #1032 location of that logic (or is now moot if the function was removed entirely).

Autopilot agent cannot resolve this safely without risking a regression on the #1032 fix. A human needs to inspect the current state of handler_history.go on feat/multicloud-web-frontend and decide how to land this PR on top of it.

CR findings (2 actionable) remain unaddressed pending conflict resolution:

• store_postgres.go: CompleteRIExchange/FailRIExchange missing transitioned_by/transitioned_at stamps
• store_postgres.go: CancelExecutionAtomic bypasses audit fields (needs actor parameter threaded through callers)

---

Generated by Claude Code

[cristim]

Rebased onto feat/multicloud-web-frontend (conflict resolved, now MERGEABLE) and addressed both unresolved CodeRabbit findings.

Finding dispositions

1. internal/api/coverage_extras_test.go (Major) - handler actor-stamping tests bypass handler logic. Fixed. Both TestHandler_TransitionRegistrationStatus_ActorStamped and ..._NonUUIDActorIsNil now drive the real rejectRegistration handler instead of calling the store mock directly:

• ActorStamped: admin session-token path; asserts transitioned_by equals the session UUID (derived by setReviewMetadata from the session, not a pre-set field).
• NonUUIDActorIsNil: admin-API-key path; requireAdmin returns the "admin-api-key" sentinel UserID, so the handler must pass nil (FK-safe via validUUIDPtrOrNil).

These now fail if the handler ever stops threading the actor.

2. internal/purchase/approvals.go (Major) - session approvals lose transition-actor attribution. Fixed. ApproveAndExecute gained a transitionedBy *string param threaded into TransitionExecutionStatus:

• approvePurchaseViaSession and directExecutePurchase (human session paths) pass validUUIDPtrOrNil(&session.UserID).
• ApproveExecution (token/SQS path) keeps nil.
• Interface updated in internal/api/types.go and internal/server/interfaces.go, plus mocks.
• Added a manager-level regression test asserting the non-nil UUID is threaded for human approvals, and the two direct-execute handler tests now assert the session UUID via mock.MatchedBy.

Rebase fixups

• Resolved handler_history.go / handler_history_test.go conflicts: the base branch moved stale-expiry into expireStaleExecutionsAsync; threaded the nil actor into that call and aligned the async-expire tests (kept the base's done-channel waiting pattern).
• Renumbered migration 000066_audit_actor_stamps -> 000068 to avoid collision with the base branch's 000066/000067.
• Updated purchase/api test expectations to the 5-arg TransitionExecutionStatus signature.

Verification

• go build ./...: pass.
• go vet on touched packages: clean.
• go test ./internal/api/ ./internal/purchase/: 1726 passed.
• gofmt: clean.
• No live Postgres; DB paths covered via mocks/pgxmock.
• Out of scope: TestAuthServiceAdapter_ValidateCSRFToken fails on the clean base tip too (pre-existing, unrelated).

@coderabbitai full review

[coderabbitai]

@cristim, the two major findings are thoroughly addressed:

1. Registration actor-stamping tests — driving the real rejectRegistration handler via both the admin session-token path and the "admin-api-key" sentinel path is the right approach. These tests will now catch regressions if the handler ever stops threading the actor through validUUIDPtrOrNil.

2. ApproveAndExecute actor attribution — adding transitionedBy *string to the ApproveManager interface and threading it from approvePurchaseViaSession / directExecutePurchase (non-nil UUID) while keeping ApproveExecution (token/SQS path) at nil correctly mirrors the nullable-actor contract established by CancelExecutionAtomic.

The rebase fixups (async stale-expiry actor threading, migration renumber to 000068) are clean. Triggering a full review now.

---

✏️ Learnings added

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

Timestamp: 2026-06-08T04:37:49.130Z
Learning: In LeanerCloud/CUDly PR `#1011` (issue `#1009` audit actor stamps), `ApproveAndExecute` in `internal/purchase/approvals.go` gained a `transitionedBy *string` parameter that is threaded into `TransitionExecutionStatus`. Human session paths (`approvePurchaseViaSession`, `directExecutePurchase`) pass `validUUIDPtrOrNil(&session.UserID)`; the token/SQS path (`ApproveExecution`) passes `nil`. The `ApproveManager` interface was updated in both `internal/api/types.go` and `internal/server/interfaces.go`. The `expireStaleExecutionsAsync` function (post-#1032 location of stale-expiry logic in `handler_history.go`) passes `nil` actor for system-initiated transitions. Migration was renumbered to `000068` to avoid collision with base-branch migrations `000066`/`000067`.

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-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-06T06:41:06.841Z
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.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.792Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.792Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), `POST /api/plans/{id}/purchases` (`createPurchaseExecutionsTx`) must stamp `created_by_user_id` via `resolveCreatorUserID(session)` at creation time. Without this, every scheduled purchase created via the Plans-page "Create planned purchases" action ships a NULL creator, which the ownership gate treats as a legacy row and denies management to all non-`update-any` users — including the creator themselves. The admin API key path must set `CreatedByUserID` to nil (not the key identity). Regressions: `TestHandler_createPlannedPurchases_StampsCreator` and `TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil`. Fixed in commit 9ef1b4352.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-08T04:21:35.802Z
Learning: In LeanerCloud/CUDly `frontend/src/recommendations.ts` (PR `#838`, commit d7c5764), `perRecPayments` in `FanOutBucket` must be seeded **sparsely** (override-only: only recs whose resolved payment differs from the bucket-level default). The execute path in `frontend/src/app.ts` resolves each rec's payment as `perRecPayments.get(rec.id) ?? b.payment`; if the map is fully populated the fallback is dead and changing the bucket-level dropdown has no effect on unedited rows. The per-rec change handler deletes the map entry when the user reverts to the bucket default (resuming tracking via the fallback); the bucket-level change handler re-syncs visible per-rec selects that have no override, skipping rows whose service does not support the new payment (mixed-SP buckets). Tests (i) and (j) cover the override-only contract and bucket-level propagation respectively; 322 tests pass, `tsc --noEmit` clean.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:28:28.139Z
Learning: In LeanerCloud/CUDly GCP service clients (computeengine, cloudsql, memorystore, cloudstorage), `GetRecommendations` filters out non-ACTIVE recommendations using the typed `recommenderpb.RecommendationStateInfo_ACTIVE` enum (CLAIMED/SUCCEEDED/FAILED/DISMISSED are skipped). State is accessed nil-safely via the proto getter. Fixed in PR `#1047` commit 95a390f (H-1 finding). Regression tests: `TestGetRecommendations_FiltersNonActiveStates` (5-rec mock, only ACTIVE survives) and `TestGetRecommendations_ActiveRecIncluded`. Existing mock tests in all four services had StateInfo added to satisfy the new filter.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:28:28.139Z
Learning: In LeanerCloud/CUDly `providers/gcp/recommendations.go`, `collectRegion` fans out to all four GCP service clients (computeengine, cloudsql, memorystore, cloudstorage) concurrently (fixed in PR `#1047` commit 95a390f, H-2 finding). `regionResult` gains `cache` and `storage` slices; merge is updated accordingly. The decision was to wire (not trim) memorystore and cloudstorage because both have complete `GetRecommendations` implementations and their no-op `PurchaseCommitment` paths are orthogonal to surfacing advisory recommendations. Regression tests: `TestRegionResult_HasCacheAndStorageFields` and `TestShouldIncludeService_Cache_Storage`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T19:32:26.182Z
Learning: In LeanerCloud/CUDly `providers/aws/services/rds/client.go`, `findOfferingID` validates AZConfig with a switch that has a `default: error` case covering non-empty invalid values (e.g. "typo-az"), not just an empty-string check. A bad non-empty AZConfig previously fell through to `multiAZ==false` in `paginateRDSOfferings` (same mis-buy as the old default). Fixed in PR `#1085` commit 681fd9e81. Regression test added.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:41:06.841Z
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").

✅ Action performed

Full review finished.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.