fix(pagination): fix reverse paging and reject stale-sort cursors - #3131
fix(pagination): fix reverse paging and reject stale-sort cursors#3131mcm wants to merge 2 commits into
Conversation
|
✅ No security or compliance issues detected. Reviewed everything up to dced07b. Security Overview
Detected Code Changes
|
|
@codex review |
daryllimyt
left a comment
daryllimyt
left a comment
There was a problem hiding this comment.
Requesting changes for two cursor-contract gaps: server-issued NULL table-sort cursors cannot be consumed, and the touched inbox provider still accepts cross-sort cursors.
| f"but this request sorts by {sort_column!r}. " | ||
| "Restart pagination without a cursor after changing the sort." | ||
| ) | ||
| if cursor.sort_value is None: |
There was a problem hiding this comment.
This treats every null sort value as a malformed or missing cursor, but TablesService.list_rows() can issue exactly that cursor: custom table columns default to nullable=True, and the cursor encoder passes row.get(sort_column) through unchanged. With more than limit rows sorted descending by a newly added nullable column, PostgreSQL puts the NULL rows first, so page 1 returns a next_cursor containing "sort_value": null; following it now returns 400 and makes all later rows unreachable.
Please distinguish an omitted legacy sort field from an explicit NULL and implement a NULL-aware keyset order and predicate, including the NULL/non-NULL transition, in both directions. Add a regression test for this server-issued cursor.
| cursor=cursor, | ||
| reverse=reverse, | ||
| has_more=has_more, | ||
| encode_cursor=lambda session: self.encode_cursor( |
There was a problem hiding this comment.
This provider now uses the canonical page builder, but its cursor consumers still do not validate cursor_data.sort_column against the requested sort_col in either the ungrouped or grouped path. A created_at cursor submitted with order_by=updated_at is accepted; I reproduced compiled SQL applying that timestamp to agent_session.updated_at < ..., which can skip or duplicate rows instead of returning 400.
Please call validate_cursor_sort_column(..., sort_column=sort_col, expected_type=datetime) in both decode paths before applying the keyset predicate.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c10c8713f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if cursor.sort_value is None: | ||
| raise InvalidCursorError( | ||
| f"Cursor is missing a sort value for column {sort_column!r}. " | ||
| "Restart pagination without a cursor." | ||
| ) |
There was a problem hiding this comment.
When table rows are sorted by a nullable custom column, TablesService.list_rows can legitimately encode sort_value=None from the page boundary (row.get(sort_column)), but this check rejects that service-issued cursor on the next request. Since custom table columns are nullable by default and PostgreSQL places NULLs first for descending sorts, a table with more than one page of unset values returns page 1 successfully and then HTTP 400 instead of continuing; NULL therefore needs an explicit cursor representation and NULL-aware keyset predicate rather than being treated as a missing value.
AGENTS.md reference: tracecat/AGENTS.md:L97-L99
Useful? React with 👍 / 👎.
Reverse (`reverse=true`) cursor pagination was broken across several listings. The cases list inverted its cursor WHERE clause but never inverted the ORDER BY, so the query selected every row preceding the cursor and then kept the first `limit` of them in forward order: paging back from page 3 returned page 1, not page 2. It also never set `prev_cursor` on a reverse response and never swapped `has_more`/`has_previous`, so the Previous control went dead after one step while the response still reported `has_previous=true`. Six other listings had subsets of the same defect: - tables `list_rows`: no ORDER BY inversion, so the same page-1 teleport - case table rows: the reverse ORDER BY was appended to the one already on the statement, making it a no-op, and cursors were derived from the already-reversed page and then swapped a second time - agent skills (skills and versions) and agent preset versions: the page was never reversed back into display order and neither the cursors nor the flags were swapped - workflow management: `has_more`/`has_previous` never swapped - EE inbox agent runs: reverse-mode cursor guards and flags Add `take_cursor_page` and `build_cursor_page` to `tracecat/pagination.py` and route all seven call sites through them so the trim/reverse/swap step has a single definition and a single set of tests. Also reject a cursor whose `sort_column` does not match the sort of the request carrying it. Previously the mismatch was detected and then the cursor filter was dropped entirely, returning the first page while still advertising `prev_cursor` and `has_previous=true`. `InvalidCursorError` subclasses `ValueError`, which the cases and tables routers already map to 400. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses review feedback on TracecatHQ#3131. Table rows sort on user-defined columns, which are nullable by default, so list_rows legitimately encodes a cursor whose sort value is NULL whenever a page boundary lands inside the NULL block. Treating that server-issued cursor as malformed returned 400 on page 2 and made every later row unreachable. CursorData.has_sort_value now distinguishes an explicitly serialized sort_value: null from a legacy cursor that omits the key, and validate_cursor_sort_column grows allow_null for columns that admit NULLs. The new keyset_filter builds the matching predicate: NULL placement follows PostgreSQL's defaults, which invert consistently (ASC NULLS LAST reversed is DESC NULLS FIRST), so an ascending scan treats NULL as greater than every value and a descending scan as less, and only the id tie-breaker orders rows inside the NULL block. list_rows uses it, collapsing four duplicated cursor branches into one scan direction and spelling out NULLS FIRST/LAST in ORDER BY so the ordering matches the predicate. NOT NULL columns pass nullable=False and keep their original two-branch predicate. The agent runs inbox provider validated no cursor at all, so a created_at anchor submitted with order_by=updated_at compiled into a predicate against the wrong column and could skip or duplicate rows. Both the grouped and ungrouped decode paths now validate before applying the keyset predicate. The ungrouped path also normalizes sort_col to the two columns the query can actually order by, matching the grouped path, so an unrecognized order_by cannot mint a cursor labelled with a column the scan never sorted on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c10c871 to
dced07b
Compare
2 participants
Problem
Reverse cursor pagination (
reverse=true) returns the wrong page.prev_cursoritself is computed correctly — it encodes the first row of the current page. The path that consumes it is what's broken. InCasesService.search_cases, the reverse branch inverted the cursorWHEREclause but never inverted theORDER BY: the sort branched only onsort_direction, and the rows were never reversed back before returning. So a backward query selected everything preceding the cursor and then kept the toplimitof it in forward order.With the default
created_at desc, limit=20:Going back from page 2 worked by accident — the preceding set is exactly one page. From page 3 on, you were teleported to the top of the list.
Two more defects in the same block:
prev_cursorwasNoneon every reverse response. Theif params.reverse:branch assignednext_cursorand fell through theelse, soprev_cursorwas never set — you could step back once, then the Previous control was dead, whilehas_previousstill reportedtrue.has_more/has_previouswere not swapped. In reverse modehas_moredescribes rows behind the cursor but was returned with forward semantics.Who was exposed
Not the cases table UI —
use-cursor-pagination.tsxhardcodesreverse: falseand keeps its own client-side cursor stack. Butreverseis public API surface on the cases endpoints and is plumbed through the registry actions (core/cases.py) and the SDK (sdk/cases.py), so workflow authors and agents paginating backwards hit it silently: wrong rows, no error.Scope
The audit found six more listings with subsets of the same defect:
cases/service.pysearch_casestables/service.pylist_rowscases/rows/service.pyagent/skill/service.py(×2)agent/preset/service.pyworkflow/management/management.pyagent_runs.pyTwo worth calling out:
tables/service.pyhas the same page-1 teleport as cases — it reverses the page and swaps the cursors, but theORDER BYonly ever branched onsort_direction.cases/rows/service.pyapplied its reverseORDER BYto a statement that already had one.Select.order_by()appends, so the first key won and the inversion was a no-op. It also derived the cursors from the already-reversed page and then swapped them a second time, pointing both the wrong way.Changes
Shared helpers in
tracecat/pagination.py—take_cursor_page()trims thelimit + 1over-fetch;build_cursor_page()puts the rows back into display order and swaps the cursors and flags. All seven call sites now route through them, so the trim/reverse/swap step has one definition and one set of tests. Sort-aware callers derive the scan direction withscan_ascending = (sort_direction == "asc") != params.reverse.One behavioral refinement: on an empty reverse page,
has_moreis derived fromnext_cursor is not Nonerather thancursor is not None, so an empty backward page can't advertise a page there's no cursor to reach. This mirrors the reasoning already documented in the inbox provider's grouped path.Stale-sort cursors now 400 instead of silently rewinding. The mismatch was already detected (
cursor_data.sort_column == sort_column), but nothing acted on it: the cursor filter was dropped entirely and the query returned the first page while the response still advertisedprev_cursorandhas_previous=true. Tells that this was an unwrittenelserather than a decision:cursor_idwas decoded and then went unused on that path, and the sibling implementation inworkflow/management/management.pydoes write the branch (ID-only fallback, with a comment). Both call sites now callvalidate_cursor_sort_column(), which raisesInvalidCursorError— aValueErrorsubclass, which the cases and tables routers already map to 400, so no router changes were needed.This is the one deliberately behavior-changing part of the PR. It's easy to drop if you'd rather keep the current leniency. Note it also rejects sortless cursors (
sort_column: None), which only pre-#1749 code paths produced; softening that to an ID-only fallback would be a one-line change. The cases table UI is unaffected either way —use-cursor-pagination.tsx:93-100resets pagination state in the same render as any sort change, so it never submits a stale-sort cursor.Tests
tests/unit/test_cursor_page.py(new, 16 tests) covers the helpers directly: the page-3 → back → page-2 walk, cursor round-tripping in both directions, the boundary at page 1, the empty reverse page, and the six validator cases.Service-level regression tests were added where none existed — no test exercised
reverse=Truefor cases at all before this:test_cases_service.py: reverse paging under desc, asc, and enum (priority) sorts, plus stale-cursor rejectiontest_tables_service.py: multi-page backward walk, plus stale-cursor rejectionapi/test_api_cases.py: confirmsInvalidCursorErrorsurfaces as 400 end-to-endVerification
tests/unit -m "not (slow or integration or temporal)": 6547 passed, 24 skipped, 8 failed — all 8 failures aresubprocess.TimeoutExpiredin nsjail sandbox tests that shell out todocker compose run --build, unrelated to this change and failing identically on a clean tree. -- these are almost certainly just from my local environment and should pass on the CI run.ruff check/ruff format --check: clean.basedpyright --warnings: 0 errors, 0 warnings.ORDER BYinversion was additionally checked against compiled SQL for all foursort×reversecombinations.🤖 Generated with Claude Code
Summary by cubic
Fixes reverse cursor pagination across listings so "Previous" returns the correct page, and returns 400 for cursors from a different sort. Also supports NULL sort cursors for table rows and unifies keyset logic for consistent behavior.
Bug Fixes
prev_cursorset in reverse; empty reverse pages don’t advertise an unreachable next page).InvalidCursorError(HTTP 400) instead of returning page 1.sort_value: nullcursors for nullable sort columns and handleNULLS FIRST/LASTviakeyset_filter; EE inbox agent runs validates cross-sort cursors and only allowscreated_at/updated_at. Shared helperstake_cursor_page,build_cursor_page, andvalidate_cursor_sort_columnunify trim/reverse/swap and validation across services.Migration
Written for commit dced07b. Summary will update on new commits.