feat(session list): per-key sort direction for --sort / ?order_by - #777
Merged
Conversation
…tion Introduce SortKey/ResolvedSort plus ParseSortSpec, FormatSortSpec, ApplyFallbackDirection, and ResolveSort so a SessionFilter can carry an ordered list of sort keys, each with its own direction, instead of a single OrderBy/Descending pair. OrderByClause and CursorPredicate now render N columns. The keyset predicate uses the lexicographic OR-expansion that mixed per-key directions require -- a single row-value tuple comparison is only valid when every column shares one direction. Sort expressions are re-rendered per clause so secrets-sort bind parameters stay positionally aligned across dialects. Cursors gain a Keys list. Single-key cursors still populate the legacy Sort/Desc/Value (and EndedAt) fields, and pre-sort legacy cursors still decode, so live pagination tokens keep working. The SQLite, PostgreSQL, and DuckDB stores resolve and render ordering from this one definition. OrderBy/Descending remain as the single-key shorthand for existing callers and as the parsed fallback when Sort is empty.
go fmt under Go 1.26 rewrites the ASCII '' in an activity-report test comment into a Unicode curly quote. The comment is specifically about SQL empty-string literals, and CI's gofmt does not make this change, so revert it to keep the branch diff focused on the sort work.
Generalize ?order_by from a single enum value to a comma-separated list of keys, each optionally suffixed :asc or :desc (e.g. messages:desc,started:asc). A key with no suffix uses the descending param, then its natural direction, so the single-key API stays backward compatible. The service layer parses the spec into the structured SessionFilter.Sort and validates it via ParseSortSpec, returning a 400 for unknown keys, bad direction tokens, duplicates, or empty terms (replacing the dropped Huma enum). ResolveSort keeps honoring a bare descending param against the implicit default recent key. The order_by doc enumerates the valid keys, guarded against registry drift by TestSortKeysDocumented.
--sort now takes a comma-separated list of keys, each optionally suffixed :asc or :desc (e.g. messages:desc,started:asc). --reverse flips the natural direction of any term without an explicit suffix and is folded into the canonical spec string, so an explicit per-key direction is left untouched and the wire form fully captures the ordering. Bad specs are rejected up front with a clear "invalid sort" error.
Add end-to-end multi-key parity tests (messages ascending, started descending, id tie-break) that walk the keyset cursor one page at a time and assert the paginated order matches the full listing. These exercise the lexicographic OR-expansion against the PostgreSQL ::bigint/::timestamptz casts and the DuckDB CAST placeholders, matching the SQLite coverage.
Use strings.Cut for the sort-spec split and a range-over-int loop in the keyset predicate expansion, as golangci-lint --fix suggests.
…le comment `session list --sort "" --reverse` dropped the reversal silently: an empty spec parsed to zero terms, so the --reverse fold had nothing to flip and the default recent sort stayed descending. Materialize the implicit default key (new exported db.DefaultSortKey) before folding so --reverse flips it to ascending, matching the pre-multi-key behavior. Also repoint the sessionSorts comment at the doc-string guard tests that replaced the removed order_by enum and TestSortKeysMatchHumaEnum.
…c guard The session-filter input struct is shared by the list and sidebar-index routes. Dropping the order_by enum left the sidebar route accepting malformed specs like ?order_by=bogus (200, silently ignored) where the enum used to reject them. Validate order_by in dbFilter() too so both routes return 400 on bad input. Replace the substring-based doc guards with one test that parses the "Valid keys:" clause into exact tokens and compares to db.SortKeys() in order, so a key omitted from the clause can no longer be masked by the example text.
roborev: Combined Review (
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #739. On that PR, @cpcloud asked whether sort direction could be specified per key rather than forcing one direction for all sort keys, given the
SessionFilterabstraction. This implements that.What changed
order_by(and--sort) now accept a comma-separated list of sort keys, each optionally suffixed:ascor:desc:Different keys can sort in different directions. A key with no suffix takes its direction from the
descendingparam (HTTP) or--reverse(CLI), then from its own natural default; an explicit:asc/:descalways wins. The single-key forms (order_by=messages&descending=true,--sort messages -r) behave exactly as before, and existing pagination cursors keep working.How it works
SessionFiltergains a structuredSort []SortKey(each key plus an optional direction);OrderBy/Descendingremain as the single-key shorthand and parse fallback.OrderByClauseandCursorPredicatenow render N columns from one definition shared by SQLite, PostgreSQL, and DuckDB.The keyset pagination predicate becomes the lexicographic OR-expansion --
(c1 OPdir1 v1) OR (c1 = v1 AND c2 OPdir2 v2) OR ...-- because a single row-value tuple comparison is only valid when every column shares one direction; mixed per-key directions require the expansion. Theidtie-breaker follows the last sort term's direction (appended unlessidis already a term). Cursors carry a per-keyKeyslist; legacy and single-key cursors still decode, and a cursor reused under a different sort list (different keys, order, or any direction) is rejected as an invalid cursor rather than silently paging wrong rows.The
order_byenum was replaced by free-form parsing with server-side validation (unknown key, bad direction token, duplicate key, empty term -> 400), shared by the list and sidebar-index routes; the valid keys are enumerated in the param doc.Where to look
internal/db/sort.go-- sort registry,ParseSortSpec/ResolveSort, cursor helpersinternal/db/query_dialect.go--OrderByClause/CursorPredicateOR-expansionListSessionsimplementations (internal/db,internal/postgres,internal/duckdb)internal/server/huma_routes_sessions.go,internal/service/direct.go,cmd/agentsview/session_list.goLimitations / possible follow-ups
--descendingflag mirroring HTTP's absolutedescendingfallback ----reverseflips each bare key's own natural direction instead. Easy to add if transport symmetry is wanted.