[cristim]

Why

GET /api/history loaded one list of the 100 newest executions across all twelve statuses, then applied provider/account/date and scope filters in Go. 100+ newer terminal rows (failed, expired, canceled, ...) therefore pushed an older pending approval off the list before any filter ran, and the Approval Queue under-reported (#632, umbrella #248). #630 only added the truncated flag.

What

• internal/api/handler_history.go: executions are fetched as two disjoint classes. Actionable (pending, notified, scheduled, approved, running, paused) at MaxListLimit; terminal (completed, partially_completed, failed, expired, canceled, legacy canceled) at DefaultListLimit. A listing error on one class is logged and that class skipped; the other still renders. truncated fires per class. historyExecutionStatuses is removed (no other Go reference).
• Stale-expiry sweep is bounded to 100 per request (the actionable fetch can now return far more stale rows and Lambda sweeps synchronously); the rest expire on later loads.
• internal/mocks/stores.go: MockConfigStore.GetExecutionsByStatuses now honours statuses, limit, ordering and the clean-completed exclusion like the real store, otherwise two calls would return the same canned rows twice. Three truncation tests that fed 100 pending rows now feed failed rows (same intent: a full terminal class yields the flag).
• types.go and openapi.yaml wording for truncated / executions_limit.

Verification (local, Go, non-race, fixture/mock based; no real database)

• New handler_history_actionable_starvation_test.go (A to E) through the real getHistory path. A (150 newer failed rows + 1 older pending) FAILS on the old handler ("older pending row must survive 150 newer failed rows", total_pending 0 vs 1) and passes now.
• Deletion probe: removing the sweep bound makes E fail (150 transitions vs 100).
• Full go test ./internal/api passes; go vet and gofmt clean; go build ./... clean.
• Not verified: behaviour against a real Postgres (store query unchanged), and the frontend notice (unchanged).

Known limits (deliberately not in this PR)

• Inside the actionable class far-future ramp steps sort first and can still fill the 1000 cap and hide a due pending row (fix(history): future-dated ramp steps can still crowd a due pending approval out of the actionable fetch #680). truncated stays honest because it fires at 1000.
• executions_limit stays 100 (terminal cap), so the existing frontend notice can print "100" when only the actionable class hit its cap (wording follow-up in feat(frontend): Load more on the History view, Approval Queue from all loaded pages #676). Comments mentioning historyExecutionStatuses remain in frontend/src/history.ts, frontend/src/types.ts and migration 000089.
• Provider/account/date and account-scope pushdown (fail closed) is PR2. Paging feat(api): page the History view past the first limit rows (offset, has_more) #674, scope-aware totals feat(api): scope-aware SQL totals for the History summary cards #675, Load more feat(frontend): Load more on the History view, Approval Queue from all loaded pages #676.
• Pre-existing, unchanged: stale rows are collected before scope filtering, so a scoped user's request can expire other tenants' stale rows (state transition only, nothing returned).

Refs #632 (umbrella #248). Not Closes: PR2 is still required.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 6 billable files and costs up to $1.50.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

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

Or wait 55 minutes for your next included review.

Check out review usage here.

View limit details

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

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: cfa7c557-3d40-4d43-85b7-3e24ccfee815

---

📥 Commits

Reviewing files that changed from the base of the PR and between 2a4fe44 and e754f48.

---

📒 Files selected for processing (6)

• internal/api/handler_history.go
• internal/api/handler_history_actionable_starvation_test.go
• internal/api/handler_history_truncation_test.go
• internal/api/openapi.yaml
• internal/api/types.go
• internal/mocks/stores.go

---

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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

[cristim]

Independent full-PR review (cc-cli-w2, Claude Opus 5.5; did not write this PR) at exact head ecb0116d4cbdcd467c933c06ce1a7eb453e89924, base 15a5eea1

Verdict: CLEAN for merge; no blocking findings. There is one low-severity actionable item (UI copy), which can be fixed here or explicitly deferred to the frontend PR. I also have two non-blocking recommendations.

What I checked

No row lost or duplicated between the two queries. actionableExecutionStatuses (6) and terminalExecutionStatuses (6) are disjoint, and their union is exactly the old 12-status historyExecutionStatuses (handler_history.go:113,138). The two lists include all six pending-class statuses: pending, notified, scheduled, approved, running and paused.

The mock is faithful to the real store. filterExecutionsLikeStore (internal/mocks/stores.go:1588-1608) matches PostgresStore.GetExecutionsByStatuses (internal/config/store_postgres.go:1375-1401) on each point:

• empty statuses return nil (:1376);
• limit <= 0 becomes DefaultListLimit (:1379);
• the limit is clamped to MaxListLimit (:1382);
• status = ANY($1) (:1395);
• clean completed rows are excluded via COALESCE(error,'') = '' (:1396; Go's Error == "" is equivalent);
• rows are ordered by scheduled_date DESC (:1397).

The only difference is tie order: the store's is unspecified and the mock's is stable.

Scope and leak. filterPurchaseHistoryByAllowedAccounts still runs on the merged completed + extra slice after the fetch (handler_history.go:69), unchanged. Truncated is still a single boolean (:88) and no count is exposed. The stale sweep is collected before the provider/account/date and scope filters, as before. It still only expires pending or notified rows older than the 7-day window, which is the same predicate as before, and is now capped at 100 per request (:262-265). The old code swept at most the 100 fetched rows, so the bound per request is unchanged.

Other GetExecutionsByStatuses callers. The handler is the only production caller changed. The mock now filters canned rows by status and limit, and every package with tests that use it still passes at the head: internal/api (20.3s), internal/analytics, internal/config, internal/purchase, internal/server and internal/mocks. I ran them one package at a time, without -race, in a scratch git archive of the head.

Reproduced

Probe	Result
Head, go test ./internal/api	ok
Main's handler_history.go + PR tests + a 2-var shim	PendingNotStarvedByNewerTerminalRows FAILS (:67/:69); FetchesDisjointClassesWithTheirLimits FAILS
Head with the staleExecs = staleExecs[:config.DefaultListLimit] line deleted	StaleSweepBoundedToDefaultListLimit FAILS: "expected 100 call(s) to TransitionExecutionStatus, got 150"
Head with the actionable fetch at DefaultListLimit instead of MaxListLimit	only FetchesDisjointClassesWithTheirLimits FAILS (the limit argument). No behavioural test has more than 100 actionable rows (see R2).

Findings

A1 (low, actionable or explicitly deferred). frontend/src/history.ts:396 still tells the user results are capped at "${executions_limit} pending, failed or other non-completed executions". After this PR executions_limit (100) caps only terminal rows, and pending rows have a separate 1000 cap. The banner is now wrong in exactly the case it exists for. Either fix the copy here (it's a frontend file, so it needs the frontend lock) or name it in the PR body as part of the planned frontend PR (#632 PR5).

R1 (non-blocking). A listing error on the actionable class logs, skips the class and leaves truncated=false (fetchExecutionsByClass, :212-219). This is no worse than main, where any error hid every execution with truncated=false. But it is the one remaining way a pending approval can disappear with no signal. Consider setting the cap/truncation flag when a class errors, so the UI shows its incomplete-results banner.

R2 (non-blocking). Add a behavioural test with more than 100 actionable rows (for example, 150 pending plus 1 older pending) to pin the 1000 cap by outcome rather than by the mock-argument assertion.

Known residual, confirmed (tracked in #680). The actionable class is ordered by scheduled_date DESC, so far-future ramp rows fill the 1000 cap first. One plan adds at most 52 rows per request (handler_plans.go:523), so this needs about 20 large plans. When it happens, truncated fires.

Note. A row that changes from actionable to terminal between the two queries can appear in both (an audit-gap completed row) or briefly in neither. This only affects a single response, and the handler is read-only, so it isn't a concern.

The SQL prefilter (provider/account/date/scope pushdown) is #632 PR2 and isn't in this PR, so its superset property is reviewed there.

[cristim]

CI follow-up for ecb0116d: Lint Code fails, so CI Success fails (CI run 37824587098). This blocks merge. golangci-lint reports 3 issues:

• internal/api/handler_history.go:169:16 gocritic appendAssign: executions := append(actionable[:len(actionable):len(actionable)], terminal...). Fix: build it with make([]config.PurchaseExecution, 0, len(actionable)+len(terminal)), then append both, or use slices.Concat(actionable, terminal).
• internal/mocks/stores.go:1597:2 gocritic rangeValCopy (304 bytes): iterate by index (for i := range rows { r := &rows[i] ... }), the same way handler_history.go already does with _rvc.
• internal/api/handler_history_truncation_test.go:76:23 unparam: failedExecutions's n is always config.DefaultListLimit. Drop the parameter, or call it with another value.

Every other check passed: unit, integration, E2E, security, gosec and Terraform validate. The rest of my review verdict stands. These are mechanical fixes, and a new head needs only a delta re-review.

[cristim]

Delta re-review (cc-cli-w2, independent; did not write this PR) of ecb0116d -> 269f3d5fc23510206b4c6258759ed65a51a57357

Verdict: CLEAN, no actionable findings in the delta. My earlier review of ecb0116d still applies. This review is read-only, because the lead paused new test runs under the disk hold.

git diff ecb0116d 269f3d5f is one commit touching 4 files (+26/-9), and every hunk is accounted for:

1. handler_history.go:169-171: make(0, len(a)+len(t)) followed by two appends. It has the same elements in the same order (actionable, then terminal) as the old full-slice-expression append. Both versions give a fresh backing array, so the behaviour does not change.
2. internal/mocks/stores.go:1597-1602: the loop now ranges by index and appends *r. That is the same value copy as before, with the same filter, sort and clamp.
3. handler_history_truncation_test.go: failedExecutions(n) becomes failedExecutionsAtCap(). All 4 callers passed config.DefaultListLimit before, so the inputs are identical.
4. New TestHandler_getHistory_ReturnsMoreThanDefaultLimitPendingRows: I checked it isn't vacuous by reading the code. MockConfigStore in internal/api is an alias of mocks.MockConfigStore (internal/api/mocks_test.go:19), so the faithful filter applies.
   • The test returns 150 pending rows for every call. The terminal-class call filters these down to 0 rows, and the actionable call (limit 1000) returns all 150.
   • If the actionable fetch were capped at DefaultListLimit, the clamp would return 100 rows, and assert.Len(..., 150) would fail.
   • The rows are scheduled now, so none is stale and no expiry call is needed. truncated is false because 150 < 1000 and there are 0 terminal rows.
   • This closes my R2. I agree with deferring A1 and R1 to a follow-up issue.

CI at 269f3d5f: Lint Code passes now, and 23 checks pass. Security Scanning fails, and therefore so does CI Success (run 37836918504). The failing step is npm audit (frontend), on advisories GHSA-8r5x-fm3f-whwj (Handlebars) and GHSA-hp3w-g68c-fv3c (sprintf-js, pulled in through jest/istanbul).

• This PR changes no frontend or npm files, so the failure isn't caused by this diff.
• Main's last completed CI (5b684083, 18:55Z) passed. The current main run (a23772d1) was still in progress when I checked, so I haven't confirmed that main fails too.
• The merge gate still needs green CI at this SHA, so this needs either a repo-level fix (a dependency bump or an audit allowlist, in its own PR) or a lead decision.