[IAmJSD]

Refs #632

This is the actionable-class part of the "PR2" half of #632's scope decision. #632 stays open after this PR: the 2026-10-08 scope note also put the account-scope pushdown on the completed-history side into PR2, and this PR leaves that side filtering in Go (see "Not in this PR"). PR1 was #684. The other asks are already merged: paging (#674 via #1277), scope-aware SQL totals (#675 via #1200), the frontend Load more (#676), and ramp-step ordering (#680 via #1038).

Root cause

getHistory reads the actionable executions (pending, notified, scheduled, approved, running, paused) with GetExecutionsByStatuses(statuses, MaxListLimit). Only after that does it apply the provider, account and date filters (matchesExecution) and the caller's account scope (historyScope.filter) in Go. Every tenant's actionable rows counted against the 1000-row cap. Once enough newer rows of other accounts, or rows outside the request filters, existed, a pending approval the caller could act on was never read, so it was missing from History. truncated was set, but the row could not be reached at all, because the actionable class is not paged.

Fix

• Store (624df0bf): GetExecutionsByStatusesFiltered(ctx, ExecutionsByStatusesQuery) applies the filters and the scope in the WHERE clause, before the LIMIT. GetExecutionsByStatuses is now the unfiltered case of the same query, so its order (future pending ramp steps last, fix(history): future-dated ramp steps can still crowd a due pending approval out of the actionable fetch #680) and its clean-completed exclusion are shared.
  • Date: scheduled_date between Start and End, inclusive. This is the same as the Go check.
  • Provider: some recommendation has this provider (recommendations JSONB; a non-array value reads as empty).
  • Account: cloud_account_id is in the set, or, when cloud_account_id is NULL, some recommendation's cloud_account_id is.
  • Plan: plan_id equals the plan-history view's plan_id (fix(history): apply the plan_id filter the plan-history view sends #1445 / fix(history): apply the plan_id filter the plan-history view sends #1404), the same exact match ListHistoryExecutionsPage uses.
  • Scope: the same account test against every value the scope can allow, or, for a NULL cloud_account_id, an execution created by the caller. An empty scope keeps only the caller's own unattributed rows, so it fails closed.
  • Each predicate is a superset of the Go check, and the Go checks still run unchanged, so SQL never drops a row Go would keep.
• Handler (070a6af0, plan in a30bf154): fetchExecutionsAsHistory builds the query from the parsed filters, plan_id included, and the request's historyScope. Main's fix(history): apply the plan_id filter the plan-history view sends #1445 had routed a plan-filtered actionable read through ListHistoryExecutionsPage; that read is now this one query with PlanID set, so a plan view keeps its SQL plan filter and the scope pushdown together. matchesExecution's plan check from fix(history): apply the plan_id filter the plan-history view sends #1445 still runs on the result.
  • The scope is sent as its literal entries plus the UUID and external number of every registered account it allows. historyScope.allows can match a row by the name of the account that either identifier resolves to, so both are needed.
  • Unrestricted callers send no scope.
  • The account filter sends the UUIDs and every resolved external number.
• Stale-approval sweep is not narrowed with it. The History GET is the only place stale pending/notified approvals get expired (and their suppressions released, feat(plans): plan steps resolve real recommendations and show real counts and prices (tier B of #609) #631). Scoping the actionable read would otherwise stop one user's load from expiring another account's stale rows. So when the read is filtered, plan-filtered or scoped (isNarrowed counts PlanID too), the stale rows come from a separate unfiltered pending/notified read past the 7-day window. That read is capped at DefaultListLimit (100) stale rows, newest first, where an unfiltered load used to see every stale row among the 1000-row class read. A larger backlog drains over successive loads. An unfiltered, unrestricted load reuses the class it already read, as before.
• Mock: MockConfigStore.GetExecutionsByStatusesFiltered answers from a GetExecutionsByStatuses expectation when it has none of its own. That is a valid superset, because the handler filters again, so the existing History tests are unchanged.

Verification

• TestHandleRequest_History_ScopedPendingNotCrowdedOutByOtherAccounts drives HandleRequest as a user scoped to one account. The unfiltered read returns 1000 newer pending rows of another account, and the scoped read returns the user's own row. Fail-before: with handler_history.go at main (the store and mock changes kept), the response is empty. With the fix it holds exactly the user's row, with truncated=false.
• TestHandleRequest_History_PlanPendingNotCrowdedOutByOtherPlans: an unrestricted caller loads a plan view. The plan-less read returns 1000 newer pending rows of another plan, and the read carrying the plan returns the plan's own row. TestHandleRequest_History_PlanViewStillSweepsAllStaleApprovals: a plan view still issues the unfiltered stale sweep (no plan, scope or filters). TestIsNarrowed_PlanID pins the predicate. Fail-before: with PlanID left out of actionableExecutionsQuery and isNarrowed, all three fail (empty response with truncated=true; no sweep query).
• TestIntegration_GetExecutionsByStatusesFiltered_PlanID (testcontainers Postgres): the plan's row is the only one returned, also with Limit: 1 while newer rows of another plan and of no plan exist; the plan is ANDed with the scope; an unknown plan returns nothing. Fail-before: without the plan_id predicate it fails, and so does main's TestIntegration_HistoryPlanFilter_ActionableOverCap (fix(history): apply the plan_id filter the plan-history view sends #1445), which now runs through this query.
• TestHandleRequest_History_ActionableQueryCarriesFilters checks that provider, start and end reach the query, the limit is 1000, and an unrestricted caller sends no scope.
• TestHandleRequest_History_StaleSweepIgnoresFiltersAndScope checks both cases: a narrowed load issues the unfiltered stale read (no scope or filters, End = now - 7d), and an unnarrowed load does not.
• TestHistoryScope_ExecutionScope and TestActionableExecutionsQuery_AccountValues cover how the scope and the account filter are translated.
• TestIntegration_GetExecutionsByStatusesFiltered runs the real SQL on testcontainers Postgres over every account shape: cloud_account_id set, accounts only on recs (single, mixed, external number), no recs, null recs, and a non-requested status. It checks provider, account, external number, inclusive date bounds, scope (by account, own unattributed rows, NULL creator), filters combined with scope, and that the LIMIT counts only rows in scope. The existing GetExecutionsByStatuses integration and pgxmock tests pass unchanged.
• Local checks (on the merged head, after git merge origin/main):
  • go build ./..., go vet ./..., go vet -tags integration ./... and the e2e vet pass.
  • golangci-lint v2.10.1 reports 0 issues on api, config, mocks and server, with and without --build-tags integration.
  • gocyclo -over 10 is clean.
  • go test -race -short passes on api, config, mocks and server.
  • go test -race -tags integration passes on api, config, mocks and server (testcontainers Postgres).
  • The secrets scan is clean, and scripts/test-git-secrets-allowlist.sh passes.

Not in this PR

• Completed-history side (why this is Refs #632). The terminal-execution and purchase_history stream (ListHistoryExecutionsPage / ListPurchaseHistoryPage) still applies provider, account and scope in Go (its plan and date bounds are already in SQL). Since feat(api): page the History view with a keyset cursor (has_more, next_cursor) #1277 it walks keyset batches past hidden rows, so no row is lost there. The only cost is extra batches for a scoped user whose rows are sparse. The 2026-10-08 scope note on fix(history): pending approvals can fall off the first page because filters are applied after the executions cap; add paging and push provider/account/date filters into the executions SQL #632 put this pushdown in PR2 ("completed-history side too"), so fix(history): pending approvals can fall off the first page because filters are applied after the executions cap; add paging and push provider/account/date filters into the executions SQL #632 stays open for it. It can reuse this predicate builder in a follow-up, or the owner can drop it on fix(history): pending approvals can fall off the first page because filters are applied after the executions cap; add paging and push provider/account/date filters into the executions SQL #632.
• Stale sweep behaves as described above. The cost is one extra small query on a filtered or scoped first-page load.

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

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

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

[IAmJSD]

Pushed 070a6af (head 070a6af). Two commits: 624df0bf adds the store query GetExecutionsByStatusesFiltered, its mock and the Postgres integration test; 070a6af0 wires the History handler to it and keeps the stale-approval sweep unfiltered, with the handler tests. Local build, vet, lint, gocyclo, race unit tests, integration tests (api, config, mocks, server) and the secrets scan all pass.

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: REQUEST_CHANGES 070a6af099af5d3d43cb11c44c9d8e6944474868

Blocking:

• internal/api/handler_history.go:197-230 (fetchExecutionsAsHistory / fetchExecutionsByClass): the PR conflicts with main (GitHub: CONFLICTING/DIRTY). fix(history): apply the plan_id filter the plan-history view sends #1445 (b7d1465, plan-history plan_id filter) changed the same two functions on main: fetchExecutionsByClass(ctx, statuses, planID, limit) now routes a plan-filtered actionable read through ListHistoryExecutionsPage with PlanID. This is not a mechanical resolve. Taking either side loses something. If the PR's side wins, the plan view loses its SQL plan filter, and one plan's pending row is crowded out of the 1000 cap by other plans' rows again (the fix(history): apply the plan_id filter the plan-history view sends #1445 regression). If main's side wins, the fix(history): pending approvals can fall off the first page because filters are applied after the executions cap; add paging and push provider/account/date filters into the executions SQL #632 pushdown is dropped. The fix probably adds PlanID to ExecutionsByStatusesQuery / executionsByStatusesWhere, counts it in isNarrowed so the stale sweep stays unfiltered for plan views, and adds a plan case to TestIntegration_GetExecutionsByStatusesFiltered. The resolved head needs a fresh review.
  Non-blocking:
• Closes vs Refs: the 2026-10-08 scope decision on fix(history): pending approvals can fall off the first page because filters are applied after the executions cap; add paging and push provider/account/date filters into the executions SQL #632 put the account-scope pushdown for the "completed-history side too" in PR2. The PR leaves the terminal stream filtering provider, account and scope in Go. It argues, plausibly, that feat(api): page the History view with a keyset cursor (has_more, next_cursor) #1277's keyset walk already means no row is lost there. That is an acceptance call for the acting owner. Either confirm on fix(history): pending approvals can fall off the first page because filters are applied after the executions cap; add paging and push provider/account/date filters into the executions SQL #632 that the terminal pushdown is dropped, or switch to Refs and open the follow-up the body mentions.
• internal/api/handler_history_actionable.go:79-83: when the load is narrowed, the stale sweep now reads only DefaultListLimit (100) stale rows, newest first. Before, it saw everything in the 1000-row class read. A large backlog still drains over successive loads, so this is not a correctness issue, but the body's "one extra small query" doesn't mention the lower per-load cap.
  Attacked: I checked that every SQL predicate is a superset of the Go check it front-runs: matchesExecution, accountMatchesFilters (UUID or external number, rec fallback through collapseRecommendationAccount), and historyScope.allows (literal entries, name resolution through historyRowAccountName by ID or external ID, unattributed rows only for their creator, "" creator against NULL). I also checked the JSON keys against RecommendationRecord tags, the inclusive date bounds and µs truncation, empty-scope fail-closed, and that the stale cutoff matches isStaleExecution. Ran: go build ./..., go vet with and without -tags integration on api/config/mocks/server, go test -race -short -count=1 on those four packages (all pass), and go test -race -tags integration -run 'Executions|History' ./internal/config/ (passes). Fail-before: with handler_history.go reset to the merge-base (store and mock kept), ScopedPendingNotCrowdedOutByOtherAccounts and ActionableQueryCarriesFilters fail. I then ran a trial git merge origin/main, which conflicts in handler_history.go (aborted).
  Body-stale: "The other asks are already merged" is still true, but the body predates fix(history): apply the plan_id filter the plan-history view sends #1445. Its description of fetchExecutionsByClass and the verification list don't cover the plan_id filter now on main.

_{Reviewer: rE3. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Pushed a30bf15 (head a30bf15), answering rE3's review of 070a6af:

Blocking: conflict with #1445's plan_id filter

• 03f0e18c merges origin/main (990f493). In fetchExecutionsAsHistory / fetchExecutionsByClass the resolution keeps the GetExecutionsByStatusesFiltered read, so the ListHistoryExecutionsPage branch from fix(history): apply the plan_id filter the plan-history view sends #1445 is gone from the actionable class. The terminal stream still uses it with PlanID.
• a30bf154 puts the plan into that query. ExecutionsByStatusesQuery gains PlanID, and executionsByStatusesWhere adds plan_id = $N, the same exact match ListHistoryExecutionsPage uses. actionableExecutionsQuery sets it from the parsed filters. The Go post-filter (matchesExecution's plan check from fix(history): apply the plan_id filter the plan-history view sends #1445) is unchanged and still runs on the result.
• isNarrowed now counts PlanID, so a plan view still runs the separate unfiltered stale sweep. Its comment says every filter field must be listed there.
• Tests, each failing before and passing after:
  • TestIntegration_GetExecutionsByStatusesFiltered_PlanID (real Postgres). It is a separate function, so the existing table keeps its seed. It checks that only the plan's row comes back, also with Limit: 1 while newer rows of another plan and of no plan exist, that the plan is ANDed with the scope, and that an unknown plan returns nothing. Without the predicate it fails. Main's TestIntegration_HistoryPlanFilter_ActionableOverCap (fix(history): apply the plan_id filter the plan-history view sends #1445) now goes through this query, and it fails without the predicate too.
  • TestHandleRequest_History_PlanPendingNotCrowdedOutByOtherPlans, TestHandleRequest_History_PlanViewStillSweepsAllStaleApprovals and TestIsNarrowed_PlanID. With PlanID taken out of actionableExecutionsQuery and isNarrowed, all three fail (empty response with truncated=true, no sweep query).

Non-blocking

• Closes vs Refs: the body now says Refs #632. The completed-history pushdown from the 2026-10-08 scope note isn't in this PR, and the body says so. I'll leave a note on fix(history): pending approvals can fall off the first page because filters are applied after the executions cap; add paging and push provider/account/date filters into the executions SQL #632 that it remains.
• The stale sweep cap: the body now states it. A narrowed load reads at most DefaultListLimit (100) stale rows, newest first, and a larger backlog drains over successive loads.
• Body-stale: the Fix and Verification sections now cover the plan_id filter and the merge.

Checks on a30bf15 (merged with 990f493. Main has since moved to 08105c8, which still merges cleanly.)

• go build ./..., go vet ./..., go vet -tags integration ./... and the e2e vet pass.
• golangci-lint v2.10.1 found 0 issues on api, config, mocks and server, with and without --build-tags integration. gocyclo -over 10 is clean.
• go test -race -short -count=1 and go test -race -tags integration -count=1 pass on api, config, mocks and server.
• The secrets scan is clean and the planted key was flagged. test-git-secrets-allowlist.sh gives 183 PASS, 0 FAIL.

The resolved head needs a fresh review.

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 9b426881442e9214af41ff2ed00ed6e204db9582

Blocking:

• none

Non-blocking:

• internal/api/handler_history_actionable.go:81 (staleExecutionsToSweep): every narrowed first-page load (any filter, a plan view, or any scoped user) now issues a second store read just for the sweep. It's cheap (pending/notified, scheduled_date <= now-7d, LIMIT 100), and the effective cap is unchanged because expireStaleExecutions already truncated to DefaultListLimit. It's still a per-load query that only exists to drive a write side effect from a GET, so it's worth a line on fix(history): pending approvals can fall off the first page because filters are applied after the executions cap; add paging and push provider/account/date filters into the executions SQL #632 if the sweep moves to the scheduler later.
• internal/api/handler_history_actionable.go:60 (executionScope): a registered account with an empty ExternalID puts "" into the bound array. That's harmless, because SQL may keep a rec with cloud_account_id: "" and Go then drops it on the creator-only path. Skipping empty values would keep the array clean.
• fix(auth): bind group account scope entries to account IDs #1116 (bind group scope entries to IDs) rewrites AccountScope and touches the history scope tests. Whichever of the two merges second needs a re-run of TestHistoryScope_ExecutionScope and the scoped History tests on the merged tree. The superset argument below holds for ID-only scopes too.

Attacked:

• Superset proof, SQL vs Go, so the pushdown can never hide a row Go would show:
  • Scope: historyScope.allows returns true either by a literal entry, which is bound as-is, or by the name of a registered account. That name requires Allows(a.ID, a.Name), and that account's ID and ExternalID are bound. The row's AccountID is exec.cloud_account_id, or else the collapsed rec account, so it's always in the bound set. The unattributed case maps to COALESCE(created_by_user_id::text,'') = user, the same as p.CreatedByUserID == s.userID, including an empty user.
  • Account filter, provider (json:"provider") and inclusive date bounds: each is checked against matchesExecution and accountMatchesFilters, and SQL keeps a superset in every case.
• Stale sweep: isStaleExecution keys on ScheduledDate, the same column as the End bound, so the unfiltered sweep read can't miss a stale row the old class read would have found.
• Ran: go build ./...; go vet (with and without -tags integration) on api, config, mocks; go test -race -short -count=1 on api, config, mocks (pass); go test -race -tags integration -run 'GetExecutionsByStatuses|History' on config and api against testcontainers Postgres (pass).
• Fail-before: with internal/api/handler_history.go checked out from origin/main, TestHandleRequest_History_ScopedPendingNotCrowdedOutByOtherAccounts and TestHandleRequest_History_StaleSweepIgnoresFiltersAndScope fail. Restored afterwards.
• Refs #632 (not Closes) is correct: the completed-history pushdown from the 2026-10-08 scope note is still open.

Body-stale:

• none

_{Reviewer: rE7. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 9b426881442e9214af41ff2ed00ed6e204db9582 merged onto main (43182c00), together with #1464 in that order (combined tree 0c19ef85, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on 9b426881, green CI, and the combined-tree verification above. Squash-merged at exactly 9b426881442e9214af41ff2ed00ed6e204db9582.