feat(core): paginate progress run listings - #2531
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Visual recap — skippedThe visual recap job did not run for this pull request. This is informational only and does not block the PR. Recap skipped for |
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Code Review Summary
PR #2531 adds optional keyset pagination to progress-run listings, exposes the cursor through the existing read-only HTTP route, makes ordering deterministic with (started_at DESC, id DESC), and adds a supporting index plus focused tests. The cursor predicate and owner/active filtering are otherwise consistent with the intended page-boundary semantics, and the existing array response shape is preserved. This is a standard-risk core/API change.
Key Findings
- 🟡 MEDIUM — The new composite index is only ordering-compatible when
statusis constrained; default unfiltered listings cannot use it to efficiently produce the requested order. - 🟡 MEDIUM —
Date.parse()accepts some malformed calendar dates by normalizing them, so invalid cursors can be silently treated as different valid timestamps instead of returning HTTP 400.
The route/store validation split is a good defensive boundary, and the added tests cover complete cursor forwarding, incomplete cursor rejection, SQL predicate construction, and invalid timestamp rejection. The cursor behavior should be tightened and the unfiltered traversal index addressed before relying on this for large run histories.
🧪 Browser testing: Skipped — PR only modifies core store/route logic, types, tests, and a changeset; there is no user-facing UI impact.
| await ensureIndexExists( | ||
| "idx_progress_runs_owner_status", | ||
| `CREATE INDEX IF NOT EXISTS idx_progress_runs_owner_status ON progress_runs (owner, status, started_at)`, | ||
| "idx_progress_runs_owner_status_started_id", | ||
| `CREATE INDEX IF NOT EXISTS idx_progress_runs_owner_status_started_id ON progress_runs (owner, status, started_at DESC, id DESC)`, |
There was a problem hiding this comment.
🟡 Add an ordering index for unfiltered pagination
listRuns defaults to activeOnly: false, so the public list route and manage-progress listing execute WHERE owner = ? ... ORDER BY started_at DESC, id DESC. Because status is unconstrained, (owner, status, started_at, id) cannot provide that ordering; the database must scan/sort the owner's rows on default and subsequent cursor pages. Add a compatible (owner, started_at DESC, id DESC) index for unfiltered listings while retaining the status-prefixed index for activeOnly, and cover the unfiltered path with a query-plan or integration test.
Additional Info
Found by 1 of 2 code-review agents; verified against listRuns default activeOnly behavior and the changed index definition.
| if ( | ||
| typeof startedAt !== "string" || | ||
| !Number.isFinite(Date.parse(startedAt)) || |
There was a problem hiding this comment.
🟡 Reject normalized invalid ISO cursor dates
Date.parse accepts malformed calendar dates such as 2026-02-30T00:00:00.000Z and normalizes them to a different finite timestamp. The route therefore accepts an invalid cursor and queries from the normalized date instead of returning the documented 400, which can silently skip or repeat rows. Require the documented ISO-8601 form and validate a round trip (or use a strict date validator), then add a test for an out-of-range calendar date.
Additional Info
Found by 1 of 2 code-review agents; independently confirmed that JavaScript Date.parse normalizes out-of-range calendar dates.
|
thanks @dlmastery! a few comments above worth taking a look at, as well as these:
Minor: renaming the index leaves the old index behind on upgraded databases, adding unnecessary write overhead. |
Summary
Add bounded keyset pagination to progress-run listing while preserving the existing 200-row per-request cap and array response shape.
listRuns(owner, { before: { startedAt, id } })retrieves the next page.GET /_agent-native/runs?beforeStartedAt=<iso>&beforeId=<id>exposes the same cursor over HTTP.(started_at DESC, id DESC).Problem / deterministic reproduction
Current
@agent-native/coreclampslimitto 200 in both the route and store, then runs a single query with no offset or cursor. With 201 owner-scoped rows:This means row 201 cannot be discovered through the public API. The issue surfaced while building a supervision dashboard that must not lose long-lived active work behind newer rows.
With this change, callers pass the final row from page one:
The per-page safety bound remains 200.
Compatibility
AgentRun[]and need no changes.activeOnlyfilters remain part of every page query.Verification
20 passed.736 files passed,10,070 tests passed,1 skipped.742 passedfiles but failed in browser E2E infrastructure withEMFILE: too many open files; the low-worker non-E2E rerun is green.