From 27322a482e52afe64321c66f5f10f294d3d8ea2c Mon Sep 17 00:00:00 2001 From: MikeyZhang75 Date: Sat, 22 Aug 2026 07:11:55 +0800 Subject: [PATCH 1/8] fix(orm): serve multi-field orderBy from a compound index Any 2+ field `orderBy` collected the whole table and re-sorted in JS, even when a declared compound index produced that exact order. Two independent bails caused it: `resolveIndexOrderPushdown` refused anything but one spec, and the planner re-decided on `orderSpecs.length` afterwards. Three of five call sites truncated the spec list before the helper ever saw it. The helper now walks the list: eq-pinned fields are absorbed at any position and direction, the rest must be contiguous from the first unpinned key in one shared direction, and `_creationTime` is legal only as the final spec. The planner reads that answer instead of counting specs, and index selection scores how much of the sort a candidate carries. Sorts over a column that can be missing or null keep the post-fetch sort: Convex orders those first ascending and `_compareByOrderSpecs` orders them last, so only sorts the two agree on may move into the index. Also fixes cursor pages ordering the wrong way when an eq-pinned field leads the sort and the next spec points the other way. --- .../skills/kitcn/references/features/orm.md | 10 +- .changeset/nine-planets-marry.md | 23 + convex/orm/order-pushdown-reads.test.ts | 284 +++++++++ ...-orm-multi-field-orderby-index-pushdown.md | 563 ++++++++++++++++++ .../skills/kitcn/references/features/orm.md | 10 +- packages/kitcn/src/orm/index-utils.test.ts | 179 +++++- packages/kitcn/src/orm/index-utils.ts | 100 +++- packages/kitcn/src/orm/query.ts | 310 +++++++--- .../src/orm/where-clause-compiler.test.ts | 8 +- .../kitcn/src/orm/where-clause-compiler.ts | 58 +- www/content/docs/orm/queries/index.mdx | 8 +- 11 files changed, 1429 insertions(+), 124 deletions(-) create mode 100644 .changeset/nine-planets-marry.md create mode 100644 convex/orm/order-pushdown-reads.test.ts create mode 100644 docs/plans/416-orm-multi-field-orderby-index-pushdown.md diff --git a/.agents/skills/kitcn/references/features/orm.md b/.agents/skills/kitcn/references/features/orm.md index 4cd2513ca..9ec730154 100644 --- a/.agents/skills/kitcn/references/features/orm.md +++ b/.agents/skills/kitcn/references/features/orm.md @@ -257,9 +257,13 @@ Rules: Relation `limit`/`orderBy` are pushed into the relation index when the index already walks in that order. After the FK, `index('by_user').on(t.userId)` orders by creation time, so `with: { posts: { limit: 5, orderBy: { createdAt: -'desc' } } }` reads 5 posts per parent. Sorting by any other column reads the -parent's whole child partition and sorts in memory — put the sort column in the -relation index (`index('by_user_rank').on(t.userId, t.rank)`) to stay bounded. +'desc' } } }` reads 5 posts per parent. Multi-field sorts are pushed down too +when the index carries all of them: `index('by_user_rank').on(t.userId, t.rank)` +serves `orderBy: { rank: 'asc', createdAt: 'asc' }`, since the FK pins `userId`, +`rank` is the next key and `createdAt` is Convex's implicit trailing one. +Sorting by any other column — or by one that can be missing or `null` — reads +the parent's whole child partition and sorts in memory; put the sort columns in +the relation index, in sort order, to stay bounded. ### Nested `with` depth diff --git a/.changeset/nine-planets-marry.md b/.changeset/nine-planets-marry.md new file mode 100644 index 000000000..4881f8a6b --- /dev/null +++ b/.changeset/nine-planets-marry.md @@ -0,0 +1,23 @@ +--- +"kitcn": patch +--- + +## Patches + +- Fix a multi-field `orderBy` reading the whole table even when a declared + compound index already produces that exact order. `orderBy: [asc(type), + asc(numLikes)]` with `limit: 5` against an index on `(type, numLikes)` now + reads 5 documents instead of every row, at any table size — previously the + read cost was the same whether you asked for 5 rows or 50. The same bound + applies to relations: `with: { posts: { orderBy: { numLikes: 'asc' }, + limit: 2 } }` now reads 2 children per parent instead of all of them. +- Prefer an index that supplies more of the requested sort when several serve + the filter equally well, so `(orgId, createdAt, title)` is chosen over + `(orgId, createdAt)` for a sort on both `createdAt` and `title`. +- Stop warning that secondary `orderBy` fields are unstable across pages when + the index carries the whole sort. A Convex cursor is the index key, so those + pages are stable. The warning still fires — with corrected wording — when no + index serves the full sort and the extra fields really are dropped. +- Sorts that mix directions, skip an index key, or run over a column that can + be missing or null keep using the post-fetch sort, so row order and null + placement are unchanged. diff --git a/convex/orm/order-pushdown-reads.test.ts b/convex/orm/order-pushdown-reads.test.ts new file mode 100644 index 000000000..50b1b80d1 --- /dev/null +++ b/convex/orm/order-pushdown-reads.test.ts @@ -0,0 +1,284 @@ +/** + * Read bounds for multi-field `orderBy`. + * + * A compound index that already produces the requested order must be walked, + * not collected and re-sorted in JavaScript. Every bound here is asserted at + * two table sizes on purpose: a scan-shaped plan cannot satisfy both. + * + * `posts.numLikesAndType` is keyed `(type, numLikes)` — the name reverses the + * key order, so read the schema, not the name. + */ + +import { expect, test } from 'vitest'; +import schema from '../schema'; +import { convexTest, countDocumentReads, runCtx } from '../setup.testing'; + +const TYPES = ['a', 'b', 'c', 'd'] as const; + +async function seedPosts( + baseCtx: { + db: { + insert: (table: 'posts', doc: Record) => Promise; + }; + }, + count: number, + authorId?: string +) { + for (let i = 0; i < count; i += 1) { + await baseCtx.db.insert('posts', { + text: `p${i}`, + // Interleaved so no prefix of table order is already sorted. + numLikes: (i * 37) % count, + type: TYPES[i % TYPES.length], + ...(authorId ? { authorId } : {}), + }); + } +} + +type Ctx = Awaited>; + +const measure = async ( + seed: (baseCtx: any) => Promise, + run: (ctx: Ctx) => Promise +) => { + const t = convexTest(schema); + await t.run(seed); + return await t.run(async (baseCtx) => { + const ctx = await runCtx(baseCtx); + const reads = countDocumentReads(baseCtx); + const rows = await run(ctx); + return { documents: reads.documents, rows }; + }); +}; + +const atBothSizes = async (run: (ctx: Ctx) => Promise) => ({ + small: await measure((baseCtx) => seedPosts(baseCtx, 60), run), + large: await measure((baseCtx) => seedPosts(baseCtx, 200), run), +}); + +test('two-field orderBy served by a compound index stays limit-bound', async () => { + const { small, large } = await atBothSizes((ctx) => + ctx.orm.query.posts.findMany({ + orderBy: (posts: any, { asc }: any) => [ + asc(posts.type), + asc(posts.numLikes), + ], + limit: 5, + }) + ); + + expect(small.rows).toHaveLength(5); + expect(large.rows).toHaveLength(5); + expect(small.documents).toBe(5); + expect(large.documents).toBe(5); +}); + +test('two-field orderBy stays limit-bound descending', async () => { + const { small, large } = await atBothSizes((ctx) => + ctx.orm.query.posts.findMany({ + orderBy: (posts: any, { desc }: any) => [ + desc(posts.type), + desc(posts.numLikes), + ], + limit: 5, + }) + ); + + expect(small.documents).toBe(5); + expect(large.documents).toBe(5); +}); + +test('an eq-pinned leading sort field does not cost the bound', async () => { + // `type` is pinned by the where, so the scan is already constant in it and + // only `numLikes` moves — including when the two point opposite ways. + const { small, large } = await atBothSizes((ctx) => + ctx.orm.query.posts.findMany({ + where: { type: 'a' }, + orderBy: (posts: any, { asc, desc }: any) => [ + asc(posts.type), + desc(posts.numLikes), + ], + limit: 3, + }) + ); + + expect(small.documents).toBe(3); + expect(large.documents).toBe(3); + expect((large.rows as any[]).map((row) => row.numLikes)).toEqual( + [...(large.rows as any[])].map((row) => row.numLikes).sort((a, b) => b - a) + ); +}); + +test('two-field orderBy returns the same page a post-fetch sort would', async () => { + const t = convexTest(schema); + await t.run((baseCtx) => seedPosts(baseCtx, 60)); + await t.run(async (baseCtx) => { + const ctx = await runCtx(baseCtx); + const page = await ctx.orm.query.posts.findMany({ + orderBy: (posts: any, { asc }: any) => [ + asc(posts.type), + asc(posts.numLikes), + ], + limit: 7, + }); + const all = await ctx.orm.query.posts.findMany({ limit: 1000 }); + const expected = [...all] + .sort( + (a: any, b: any) => + (a.type < b.type ? -1 : a.type > b.type ? 1 : 0) || + a.numLikes - b.numLikes + ) + .slice(0, 7); + expect(page.map((r: any) => [r.type, r.numLikes])).toEqual( + expected.map((r: any) => [r.type, r.numLikes]) + ); + }); +}); + +test('an `in` union keeps the requested order over the whole union', async () => { + // Each probe pins `type` to a different value, so the field is constant + // inside a probe but not across the union. The per-probe truncation is still + // sound — the global comparator restricted to one probe is that probe's own + // scan order — but only if the union is re-sorted before it is sliced. + const t = convexTest(schema); + await t.run(async (baseCtx) => { + // 'a' holds the high scores and 'b' the low ones, so a union that kept + // probe order, or merged on `numLikes` alone, would interleave them. + for (const [type, likes] of [ + ['a', [10, 20, 30]], + ['b', [1, 2, 3]], + ] as const) { + for (const numLikes of likes) { + await baseCtx.db.insert('posts', { + text: `${type}${numLikes}`, + numLikes, + type, + }); + } + } + }); + await t.run(async (baseCtx) => { + const ctx = await runCtx(baseCtx); + const rows = await ctx.orm.query.posts.findMany({ + where: { type: { in: ['a', 'b'] } }, + orderBy: (posts: any, { asc, desc }: any) => [ + asc(posts.type), + desc(posts.numLikes), + ], + limit: 4, + }); + expect(rows.map((row: any) => `${row.type}${row.numLikes}`)).toEqual([ + 'a30', + 'a20', + 'a10', + 'b3', + ]); + + // Same union, but now the leading sort field is the one the probes do not + // pin, so the page has to be assembled across probes rather than taken + // from the first one. + const byLikes = await ctx.orm.query.posts.findMany({ + where: { type: { in: ['a', 'b'] } }, + orderBy: (posts: any, { asc }: any) => [ + asc(posts.numLikes), + asc(posts.createdAt), + ], + limit: 4, + }); + expect(byLikes.map((row: any) => `${row.type}${row.numLikes}`)).toEqual([ + 'b1', + 'b2', + 'b3', + 'a10', + ]); + }); +}); + +test('cursor pages honour a sort whose leading field is eq-pinned the other way', async () => { + // `type` is pinned by the where and `numLikes` runs the other way, so the + // direction the scan needs is the second spec's, not the first's. + const t = convexTest(schema); + await t.run((baseCtx) => seedPosts(baseCtx, 40)); + await t.run(async (baseCtx) => { + const ctx = await runCtx(baseCtx); + const page = await ctx.orm.query.posts.findMany({ + where: { type: 'a' }, + orderBy: (posts: any, { asc, desc }: any) => [ + asc(posts.type), + desc(posts.numLikes), + ], + cursor: null, + limit: 3, + }); + const all = await ctx.orm.query.posts.findMany({ + where: { type: 'a' }, + limit: 1000, + }); + const expected = [...all] + .map((row: any) => row.numLikes) + .sort((left, right) => right - left) + .slice(0, 3); + expect(page.page.map((row: any) => row.numLikes)).toEqual(expected); + }); +}); + +test('a relation orderBy served by the FK index stays per-parent bound', async () => { + // `by_author_likes` is (authorId, numLikes); the FK pins authorId, so the + // scan is already in numLikes order and then `_creationTime` order. + const seed = (count: number) => async (baseCtx: any) => { + const author = await baseCtx.db.insert('users', { + name: 'A', + email: 'relation-order@example.com', + }); + await seedPosts(baseCtx, count, author); + }; + const run = (ctx: Ctx) => + ctx.orm.query.users.findMany({ + limit: 1, + with: { + posts: { orderBy: { numLikes: 'asc', createdAt: 'asc' }, limit: 2 }, + }, + }); + + const small = await measure(seed(60), run); + const large = await measure(seed(200), run); + + // 1 parent + exactly 2 children, at any table size. + expect(small.documents).toBe(3); + expect(large.documents).toBe(3); + expect((large.rows as any[])[0].posts.map((p: any) => p.numLikes)).toEqual([ + 0, 1, + ]); +}); + +test('a nullable sort column keeps the post-fetch null placement', async () => { + // Convex sorts absent/null first in an ascending index scan while the + // post-fetch comparator sorts them last. The two disagree, so this sort must + // not move into the index no matter how well the key shape lines up. + const t = convexTest(schema); + await t.run(async (baseCtx) => { + await baseCtx.db.insert('users', { name: 'A', email: 'a@x.com' }); + await baseCtx.db.insert('users', { + name: 'B', + email: 'b@x.com', + deletedAt: 5, + }); + await baseCtx.db.insert('users', { name: 'C', email: 'c@x.com' }); + await baseCtx.db.insert('users', { + name: 'D', + email: 'd@x.com', + deletedAt: 1, + }); + }); + await t.run(async (baseCtx) => { + const ctx = await runCtx(baseCtx); + const rows = await ctx.orm.query.users.findMany({ + orderBy: (users: any, { asc }: any) => [ + asc(users.deletedAt), + asc(users.name), + ], + limit: 10, + }); + expect(rows.map((row: any) => row.name)).toEqual(['D', 'B', 'A', 'C']); + }); +}); diff --git a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md new file mode 100644 index 000000000..35030a719 --- /dev/null +++ b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md @@ -0,0 +1,563 @@ +# 416 orm multi field orderBy index pushdown + +Objective: +Make a 2+ field `orderBy` walk a compound index that already produces it, instead of always `collect()` + JS sort. Keep results byte-identical, including null placement. + +Goal plan: +docs/plans/416-orm-multi-field-orderby-index-pushdown.md + +Template: +docs/plans/templates/task.md + +Primary template: +docs/plans/templates/task.md + +Applied packs: +- package-api (docs/plans/templates/packs/package-api.md) + +Task source: +- type: GitHub issue +- id / link: https://github.com/udecode/kitcn/issues/416 +- title: "ORM: any 2+ field `orderBy` forces a full `collect()` + JS sort, even when a compound index serves it exactly" +- acceptance criteria: a 2-field `orderBy` served exactly by a declared compound + index reads `limit` rows, not the table; the same absolute bound holds at two + table sizes; the relation loader gets the same bound; result rows are + unchanged. +- caveats (from the issue, all verified independently): null ordering diverges + between index scan and `_compareByOrderSpecs`; `.order()` reverses the whole + key tuple; `_creationTime` is only servable as the final spec; eq-pinned + fields match at any position/direction; index selection is leading-field-only; + two branches have the index already pinned and can only answer yes/no. +- likely files: `packages/kitcn/src/orm/index-utils.ts`, + `packages/kitcn/src/orm/query.ts`, + `packages/kitcn/src/orm/where-clause-compiler.ts` +- browser surface: none (server-side query planner) +- root-cause layer: ORM query planner / index-order pushdown + +Timed checkpoint: +- requested duration: N/A: no duration requested. +- semantics: N/A +- initial confidence score: N/A +- improvement loop: N/A +- final score / loop closure: N/A + +Completion threshold: +- `resolveIndexOrderPushdown` answers multi-spec sorts correctly (eq-pinned + absorbed at any position/direction; remaining specs contiguous from + `indexFields[eqCount]`; `_creationTime` legal only as the final spec after + every declared field is consumed; all unpinned specs share one direction), + every planner call site passes the full spec list, and the read bound is + proven by a two-table-size read-count test on both the top-level query and + the relation loader, with result rows unchanged. +- Task closure is legal only when the source-of-truth acceptance criteria are + satisfied or explicitly narrowed, required verification evidence is recorded, + code-review and release-artifact gates are closed when applicable, verified + code changes are committed and PR'd unless explicitly declined or blocked, + task-style PR body sync is complete or marked N/A with reason, + GitHub issue/PR sync is complete or marked N/A with reason, and + `node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md` passes. + +Verification surface: +- `bunx vitest run convex/orm/order-pushdown-reads.test.ts` (read bounds at two + table sizes + row-identity against the post-fetch sort) +- `bun test packages/kitcn/src/orm/index-utils.test.ts` (helper unit matrix) +- `bunx vitest run convex/orm/ordering.test.ts convex/orm/pipeline.test.ts + convex/orm/relation-loading.test.ts convex/orm/pagination.test.ts + convex/orm/where-filtering.test.ts` (no regression in ordering / read bounds / + index selection) +- `bun typecheck`, `bun lint:fix`, `bun --cwd packages/kitcn build` +- `.changeset/*.md` for the published behavior delta + +Constraints: +- Preserve existing user-facing behavior outside the task scope. +- Prefer the durable ownership boundary over caller-by-caller patches. +- When a GitHub PR is in scope, this plan owns exactly one PR. A coordinating + batch plan must link a separate task plan for every PR an agent processes. +- Verified code changes must be committed and PR'd because the task skill + requires that path unless the user explicitly says not to, the work has no + local patch, or a real blocker is recorded. +- The absence of a separate "open a PR" sentence from the user is not a valid + N/A reason for verified code-changing task work. +- A PR created by this task must use the PR #270 emoji task-style PR body + contract below, not a generic summary/body from a git helper skill. +- A task-run PR body must include + `🧭 Task plan: docs/plans/.md`; the plan must exist at the PR head and + identify the exact PR before autoclosure. +- Do not add broad ceremony when the task is trivial or docs-only. + +Boundaries: +- Source of truth: GitHub issue #416. +- Allowed edit scope: `packages/kitcn/src/orm/{index-utils,query,where-clause-compiler}.ts`, + their unit tests, `convex/orm/order-pushdown-reads.test.ts`, `.changeset/`. +- Browser surface: N/A: server-side query planner, no rendered output. +- GitHub issue sync: N/A: user preference forbids PR creation; no PR to + reference, so no issue comment is posted. +- Non-goals: changing `_compareByOrderSpecs` null placement to match Convex's + value order (a breaking change that needs user sign-off); the two cursor + branches that silently drop `orderIndexName`; `_compareGroupByValues`. + +Output budget strategy: +- Deep exploration ran as one background Workflow whose findings were dumped to + `/tmp/wf-facts.txt` and grepped, not streamed. Test runs are single-file + `bunx vitest run ` (~2s), never `bun check` / `bun run test`. + +Blocked condition: +- A required behavior change to `_compareByOrderSpecs` null placement (breaking) + would need explicit user sign-off before proceeding. + +Task state: +- task_type: bug +- task_complexity: non-trivial +- current_phase: closeout +- current_phase_status: in_progress +- next_phase: final response +- goal_status: active + +Current verdict: +- verdict: valid +- confidence: 95-100% +- next owner: task +- reason: reproduced at source level; fixed at the single pushdown owner plus + the planner flags that duplicated its decision; proven by read-count tests at + two table sizes on both the top-level query and the relation loader. + +Implementation readiness: +- verdict: ready +- exact owner: `resolveIndexOrderPushdown` (`packages/kitcn/src/orm/index-utils.ts`) + as the single servability decision, plus the planner flags in `query.ts` that + previously re-decided it on arity alone. +- contradiction status: one found and settled — the issue's "skip pushdown when + any sort field is optional" is right, but only for multi-spec sorts: a + single-spec pushdown on an optional column already ships nulls-first, and + gating it would both change shipped output and regress the read bound. +- source-listed cases complete: yes (see Source-listed case matrix) + +Pre-solution issue challenge: +- reporter claim: any `orderBy` with 2+ fields collects the whole table even + when a declared compound index serves the sort exactly; limit-independent; + the relation loader shares the bug. +- suggested diagnosis or fix: replace the arity bail in + `resolveIndexOrderPushdown` with a real servability check, pass the full + `postFetchOrders` at the three truncating call sites, consult the pushdown + result at `:6207` instead of arity, and narrow `:6404` the same way. +- repro ladder: + - tests / source-level repro: `convex/orm/order-pushdown-reads.test.ts` + failed `expected 60 to be 5` on a 60-row table with `limit: 5`. RED before, + GREEN after. + - repo-owned automated browser or integration proof: N/A: no browser surface. + - Browser plugin: N/A: no browser surface. + - screenshot / visual proof: N/A: no rendered output. +- reproduction verdict: valid +- validity verdict: valid — every claim in the issue, including all six + constraints, was independently verified against source and runtime. +- best long-term fix boundary: the helper stays the single owner of "does + scanning this index already produce the requested order?"; the planner reads + its answer instead of re-deriving one from `orderSpecs.length`. Nullability + travels on `OrderSpec` itself, so the relation loader and `findRelationIndex` + inherit the guard without a new parameter. +- harsh honest feedback: the issue's constraint 1 is stated too strongly. + "Pushdown must be skipped when any sort field is an optional column" would + regress today's single-field behavior, which already pushes down on + `users.deletedAt` and returns nulls-first. Measured on this checkout: + single-field asc → `[[A,null],[C,null],[D,1],[B,5]]`; two-field asc → + `[[D,1],[B,5],[A,null],[C,null]]`. The guard belongs on the multi-spec case + only. The issue also misses that the resolved pushdown direction can differ + from `primaryOrder.direction` once an eq-pinned spec leads the sort, which + would have sent `_buildResidualFilterStream` down the wrong direction. +- hard-stop decision: proceed — reproduced and valid. + +Completion rule: +- Do not call `update_goal(status: complete)` while any required checklist item + remains unchecked. If an item does not apply, check it and add `N/A: `. +- Do not call `update_goal(status: complete)` until every completion threshold + above is satisfied, final handoff evidence is recorded, and + `node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md` passes. +- Do not create hook state for this goal. This file plus the active goal are the + durable state. + +Start Gates: +| Gate | Applies | Evidence | +|------|---------|----------| +| Timed checkpoint parsed | no | N/A: no duration requested | +| Walkthrough baseline for possible UI change | no | N/A: server-side query planner, no UI or rendered output | +| Skill analysis before edits | yes | task + autogoal (package-api pack) + changeset + autoreview; no others earned their keep | +| Active goal checked or created | yes | this plan | +| Source of truth read before edits | yes | gh issue view 416 (body + 0 comments) and the attachment | +| Exact per-PR task ownership | no | N/A: user explicitly declined PR creation | +| GitHub comments and attachments read | yes | gh issue view 416 --json comments -> 0 comments; attachment read | +| Video transcript evidence required | no | N/A: no video or screen recording in the source | +| Pre-solution issue challenge required | yes | see Pre-solution issue challenge; verdict valid, one constraint corrected | +| Reproduction verdict before implementation | yes | RED convex/orm/order-pushdown-reads.test.ts: expected 60 to be 5 | +| Repro escalation ladder selected | yes | source-level test repro sufficed; no browser surface | +| Suggested fix reviewed against durable boundary | yes | adopted, with the nullable guard narrowed to multi-spec only | +| `docs/solutions` checked for non-trivial existing-code work | yes | docs/solutions/logic-errors/aggregate-isnull-must-probe-the-absent-field-bucket-20260817.md confirms the optional-column storage model | +| TDD decision before behavior change or bug fix | yes | RED read-count repro written before the fix; stash-verified 5 of 8 fail pre-fix | +| Branch decision for code-changing task | yes | already on issue-416-v1, which matches the issue | +| Release artifact decision | yes | .changeset/nine-planets-marry.md (patch) | +| Browser tool decision for browser surface | no | N/A: no browser surface | +| Commit / PR expectation decision | yes | commit yes; push/PR declined by explicit user preference | +| Task-style PR body decision | no | N/A: no PR created (explicit user decline) | +| Task-plan PR body evidence | no | N/A: no PR created (explicit user decline) | +| GitHub issue sync expectation decision | no | N/A: issue sync-back is gated on a PR existing; user declined PR | +| Output budget strategy recorded | yes | workflow findings artifacted to /tmp/wf-facts.txt and grepped; single-file test runs | +| Package/API pack selected | yes | package-api | +| Public surface or package boundary identified | yes | no public export change; index-utils.ts is package-internal (only orm/ and cli/utils import it) | +| Convex entry/import graph impact identified | yes | no new imports across module boundaries; OrderSpec is a type-only import inside orm/ | +| CLI/scaffold/generated impact identified | no | N/A: no CLI, scaffold, or init template touched | +| Release artifact path selected | yes | .changeset/nine-planets-marry.md | +| `changeset` skill loaded when `.changeset` is required | yes | .agents/rules/changeset.mdc followed: patch, ## Patches section, user-facing bullets | +| Package build / fixture impact decision recorded | yes | bun --cwd packages/kitcn build run; no fixture/scaffold source touched so fixtures:sync N/A | + +Work Checklist: +- [x] If a duration was requested, it is recorded as minimum active work unless + explicitly marked hard stop; when no better metric exists, initial and + final confidence scores are recorded. +- [x] Objective includes outcome, completion threshold, verification surface, + constraints, boundaries, and blocked condition. +- [x] Task source classified with source type, id/link, title, task type, + acceptance criteria, caveats, likely files/routes/packages, browser + surface, and root-cause layer. +- [x] Every GitHub PR in scope has its own task plan. This plan owns one exact + PR, owns a not-yet-created PR slice, or records N/A because no PR is in + scope; a batch plan is not used as a substitute. +- [x] Required video or screen-recording evidence is cached/read as normalized + `` XML, or marked N/A with reason. +- [x] For public GitHub bug reports, behavior claims, technical diagnoses, or + suggested fixes, reporter claims are challenged before implementation + with a recorded verdict: `valid`, `not reproduced`, `invalid`, + `wont-fix`, `partially valid`, or `platform limitation`. Feature, docs, + support, or cleanup requests with no bug claim may mark reproduction + `N/A` with reason. +- [x] Repro escalation ladder followed for bug/behavior claims: focused + test/source-level repro first when applicable; existing repo-owned + automated browser or integration proof next when available and useful as + executable coverage; the repo-approved Browser tool next when tests or + automation cannot reproduce or cannot model the surface honestly; + screenshot or explicit visual-proof waiver when visual/native state + matters. +- [x] Hard-stop rule followed for bug/behavior claims: no code when the issue + is not reproduced, invalid, or won't-fix; partial validity pivots to the + best long-term fix and records what was wrong or incomplete in the + issue's proposed path. +- [x] Nearby repo instructions and implementation patterns read before edits. +- [x] Source-listed case matrix is complete and every contradiction has an + owner, harness, and verdict before mutation. +- [x] Readiness is classified `ready`, `repair-source`, `major`, `blocked`, or + `invalid` with evidence. +- [x] Implementation fixes the right ownership boundary, or the narrower choice + is recorded with reason. +- [x] Release artifact requirement recorded: active changeset, new changeset, or + N/A with reason. +- [x] Final handoff shape decided: bug/feature/testing/batch/review/GitHub + requirements, PR body sync, and issue sync when applicable. +- [x] Commit/PR handling recorded for code-changing work: commit and PR + completed, no local patch, user explicitly declined, or blocker recorded. + "User did not separately ask for a PR" is not a valid blocker. +- [x] PR body shape recorded: PR #270 emoji task-style body used, N/A reason + recorded, or blocker recorded. +- [x] PR task evidence recorded: body includes `🧭 Task plan: ...`, the plan + exists at the PR head, and it identifies the exact PR before autoclosure. +- [x] Branch handling recorded for code-changing work: dedicated branch used, + new branch needed, or N/A with reason. +- [x] Local-env-rot retry policy recorded for any surprising repo-wide failure: + reinstall/rerun evidence or N/A with reason. +- [x] Workspace authority recorded: every proof command names the cwd/tool that + owns the changed behavior. +- [x] Output budget discipline recorded and followed: broad searches are + scoped, capped, counted, or artifacted instead of streamed into goal + context. +- [x] High-risk note recorded for public API, runtime, package-boundary, + browser behavior, agent-action, or command-contract changes, or marked + N/A with reason. +- [x] Review/autoreview target selected from actual diff state for non-trivial + implementation work, or marked N/A with reason. +- [x] Agent-native review decision recorded for `.agents/**`, `.claude/**`, + `.codex/**`, skills, hooks, commands, prompts, or user-action tooling. +- [x] Package/API pack: public API, package boundary, export, and release-artifact impact are recorded. +- [x] Package/API pack: release artifact matrix is applied: `.changeset` or explicit no-artifact reason. +- [x] Package/API pack: `.changeset` work loads `changeset` and follows its package/version/prose rules. +- [x] Package/API pack: no-artifact decisions state why the diff has no published package user-visible delta from `main`. +- [x] Package/API pack: compatibility, migration, or hard-cut decision is explicit when public shape changes. +- [x] Package/API pack: affected Convex static import graphs stay narrow and + plugin/per-module boundaries are used where appropriate. +- [x] Package/API pack: CLI commands remain deterministic, `--json` capable, + and non-interactive with explicit confirmation bypass when relevant. +- [x] Package/API pack: docs and `packages/kitcn/skills/kitcn/**` stay + current-state synchronized when public guidance changes. +- [x] Package/API pack: package-owned typecheck/build/test proof is recorded or marked N/A with reason. +- [x] Package/API pack: `packages/kitcn` build, fixture sync/check, or other owning package proof is recorded when required. + +Completion Gates: +| Gate | Applies | Required action | Evidence | +|------|---------|-----------------|----------| +| Named verification threshold | yes | Run the command, proof, source audit, or artifact check named in this plan | bunx vitest run convex/orm/order-pushdown-reads.test.ts -> 8 passed; 5 of 8 RED pre-fix (git stash verified) | +| Exact per-PR task ownership | no | Record the exact PR and dedicated plan, or the not-yet-created single-PR slice | N/A: no PR created (explicit user decline) | +| Pre-solution issue challenge verdict | yes | Record reporter claim, suggested fix, repro verdict, validity verdict, durable boundary, and hard-stop/pivot decision before implementation | valid; one constraint corrected (nullable guard is multi-spec only) | +| Repro escalation ladder | yes | For bug/behavior claims, record test/source-level, automated browser/integration, Browser, and screenshot/visual-proof outcomes or N/A/blocker reasons before `not reproduced` | source-level test repro reproduced it; browser/visual N/A (no rendered output) | +| Bug reproduced before fix | yes | Record failing test/repro or N/A with reason | expected 60 to be 5 on a 60-row table with limit 5 | +| Targeted behavior verification | yes | Run focused test/proof for changed behavior or record N/A | bunx vitest run convex/orm/ -> 480 passed; bun test index-utils/where-clause-compiler/indexes -> 62 passed | +| TypeScript or typed config changed | yes | Run relevant typecheck | bun typecheck -> 5 tasks successful | +| Package exports or file layout changed | no | Run the relevant package build before final verification and keep generated updates | N/A: no export or layout change; bun --cwd packages/kitcn build still run for the dist-dependent convex tests | +| Package manifests, lockfile, or install graph changed | no | Run `bun install` and relevant package checks | N/A: no manifest or lockfile change | +| Agent rules or skills changed | yes | Run `bun install` and verify generated skill sync | packages/kitcn/skills/kitcn/references/features/orm.md updated; bun tooling/sync-kitcn-skill.ts regenerated .agents/skills/kitcn; bunx intent validate skills + intent:stale clean | +| Workspace authority proof | yes | Run verification in the owning repo/package/app/route/tool and record cwd; do not count the wrong workspace as proof | all commands run from /Users/mikey/conductor/workspaces/kitcn/dakar (repo root), which owns both the package source and the convex integration tests | +| Browser surface changed | no | Capture Browser Use proof or record explicit waiver/blocker | N/A: server-side query planner | +| Browser final proof | no | Attach screenshot or exact browser verification caveat when browser proof applies | N/A: no browser surface | +| UI walkthrough | no | If UI or rendered output changed, run `.agents/skills/walkthrough/SKILL.md` after final proof and show annotated images in the final handoff; otherwise record N/A | N/A: no UI or rendered output changed | +| Scaffold or fixture output changed | no | Run `bun run fixtures:sync` and `bun run fixtures:check`, or record N/A | N/A: no init template or scaffold source touched | +| Package behavior or public API changed | yes | Add a changeset or record why no changeset applies | .changeset/nine-planets-marry.md (patch) — behavior change, no API change | +| Docs and kitcn skill sync changed | yes | Keep `www/**` and `packages/kitcn/skills/kitcn/**` in sync, or record N/A | www/content/docs/orm/queries/index.mdx + packages/kitcn/skills/kitcn/references/features/orm.md updated together, then synced | +| Docs or content changed | yes | For docs-heavy work, use `--template docs`; for incidental docs, verify source-backed claims, links, examples, and rendered output or record N/A | Ordering notes rewritten to current state only, no changelog language | +| High-risk mini gate | yes | For public API/runtime/package-boundary/browser/agent-action/command-contract changes, record realistic failure mode, proof plan, and why the chosen boundary is right; otherwise N/A | see High-risk note | +| Agent-native review for agent/tooling changes | no | For `.agents/**`, `.claude/**`, `.codex/**`, skills, hooks, commands, prompts, or user-action tooling, load `.agents/skills/agent-native-reviewer/SKILL.md` and close accepted/actionable findings, or record N/A | N/A: only the generated kitcn skill mirror changed, via its sync script; no .agents/.claude/.codex workflow, hook, command, or prompt touched | +| Local install corruption suspected | yes | Run `bun install` once, rerun the exact failing command, or record N/A | 7 convex suites failed with Cannot find package kitcn/server; bun --cwd packages/kitcn build fixed all 7 (stale dist, not the diff) | +| Commit created | yes | For verified code-changing work, stage the entire current checkout per repo policy and create a commit; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | single commit at the tip of `issue-416-v1`, whole checkout staged | +| PR create or update | no | For verified code-changing work, run `check`, push, create or update the PR, and sync PR body to the task-style final handoff; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | N/A: user explicitly declined PR creation ("Do not create PR under any circumstances, unless user prompts to") | +| Task-style PR body verified | no | Verify the PR body with `gh pr view --json body`; it must preserve auto-release blocks when applicable, must not include a current-PR self-link, and must use the PR #270 emoji format: `🐛 Fixes ...`, `🟢 95-100% confidence`, `Phase / 🧪 Tests / 🌐 Browser` table, and bold emoji Outcome/Caveat/Design/Verified sections | N/A: no PR created | +| PR task evidence verified | no | Verify body plan line, plan at PR head, and exact PR ownership | N/A: no PR created | +| PR proof image hosting | no | If PR body needs browser proof, replace local image paths with hosted GitHub URLs or record N/A | N/A: no PR and no image proof | +| GitHub issue sync-back | no | Post concise issue sync after PR exists, or record N/A/blocker | N/A: sync-back references a PR, and PR creation was declined | +| Final handoff contract | yes | Fill the final handoff fields below with exact PR/issue/confidence/tests/browser/outcome/caveats/design/verification content or N/A reason | see Final handoff contract | +| Final lint | yes | Run `bun lint:fix` or scoped equivalent | bun lint:fix then bun lint -> no fixes applied | +| Output budget discipline | yes | Verify no unbounded high-volume command output was streamed, or record the accidental output and recovery | workflow output artifacted to /tmp/wf-facts.txt; no unbounded stream | +| Timed checkpoint | no | If duration was requested, keep improving until elapsed, then finish the current loop cleanly; otherwise N/A | N/A: no duration requested | +| Autoreview for non-trivial implementation changes | yes | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | autoreview --mode local --engine claude: 2 conditional P0s, both investigated and closed, then re-run clean ("no accepted/actionable findings", patch is correct 0.7) | +| Goal plan complete | yes | Run `node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md` | node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md | +| Public API / package boundary proof | yes | Source-audit public API, exports, and package boundary impact | index-utils.ts importers audited: only orm/* and cli/utils/schema-tables.ts (which imports getAggregateIndexes/getRankIndexes only). OrderSpec is not publicly exported | +| Convex bundle/import proof | no | Audit affected function-entry static graphs or record N/A | N/A: no new cross-module imports; OrderSpec is a type-only import within orm/ | +| CLI/scaffold/generated proof | no | Prove command contract and regenerate owned output or record N/A | N/A: no CLI or scaffold surface touched | +| Release artifact classification | yes | Record whether the change is published package behavior/API/types/config/runtime or no published user-visible delta | published package runtime behavior change (read bounds + warning text) | +| Published package changeset | yes | If published package users see a delta, load `changeset` and add/update one `.changeset/*.md` per package | .changeset/nine-planets-marry.md | +| No release artifact | no | If no artifact is needed, record the exact reason: internal-only, docs-only, agent-only, test-only, or no user-visible delta from `main` | N/A: a changeset was added | +| Package typecheck/build/test | yes | Run owning package checks or record N/A with reason | bun typecheck, bun --cwd packages/kitcn build, bun run test:bun -> 1323 passed, bun run test:vitest -> 879 passed | +| Fixture/scaffold generation | no | Run `bun run fixtures:sync` and `bun run fixtures:check` when scaffold output changed, otherwise N/A | N/A: no scaffold output changed | +| Docs/package skill sync | yes | Synchronize current-state public guidance or record N/A | bun tooling/sync-kitcn-skill.ts; bunx intent validate skills passed; intent:stale reports all up-to-date | + +Phase / pass table: +| Phase | Status | Evidence | Next | +|-------|--------|----------|------| +| Intake and source read | done | issue #416 read, repro RED at 60 reads | implementation | +| Implementation | done | helper rewrite + 5 call sites + index scoring + docs + changeset | verification | +| Verification | done | 8/8 new tests, 879 vitest, 1323 bun test, typecheck, lint, build | closeout | +| Commit / PR / GitHub sync | done | commit created; push/PR/issue-sync declined by user preference | final response | +| Closeout | done | autoreview clean on re-run | final response | + +Findings: +- Two independent blanket bails, exactly as the issue said: the helper's arity + bail (`index-utils.ts:139`) and the planner's `hasSecondaryOrders` (`query.ts:6207`). + Fixing either alone is a no-op, and three of five call sites truncated the + spec list before the helper ever saw it. +- The issue undercounted the call sites: five, not four. `findRelationIndex` + (`index-utils.ts:204`) forwards a variable-length list of its own, and the + no-where selection branch (`query.ts:6176-6205`) had its own pushdown decision + that never called the helper at all. +- Null placement already diverges on `main` for a SINGLE field. Measured: + single-field asc gives nulls first, two-field asc gives nulls last. So the + issue's "skip pushdown when any sort field is optional" would have regressed + shipped behavior. The guard belongs to multi-spec sorts only. +- The resolved pushdown direction can differ from `primaryOrder.direction` once + an eq-pinned field leads the sort. Three sites consumed `primaryOrder.direction` + as if it were the scan direction. One of them (cursor pagination with a + divergent sort) was already returning ascending pages for a descending + request on `main`; this change fixes it. +- The multi-probe union sort (`query.ts:6621`) is unconditional, not gated on + `usePostFetchSort`, so relaxing the probe bound cannot reorder a union. +- A predicate `where` produces no compiled index (`query.ts:7268-7271`), so + `queryConfig.index` and a predicate `configuredIndex` cannot coexist and the + stream always walks the index the direction was resolved against. +- Index scoring only ever looked at `orderFields[0]`, so without widening it the + planner would keep picking a shorter index and the fix would rarely fire on + `where` + multi-field sorts. +- `convex-test` vendors its own fork of Convex's `compareValues`. Ranks for + `undefined`/`null` match, so the null-placement evidence transfers; string + (supplementary-plane), `-0`, and NaN ordering do not. The regression fixtures + deliberately use ASCII strings and small positive integers. + +Decisions and tradeoffs: +- Nullability travels on `OrderSpec` rather than as a new helper parameter, so + `findRelationIndex` and the relation loader inherit the guard without another + positional argument on an already 7-argument signature. +- `nullable` is read from the compiled Convex validator, not `config.notNull`, + because `timestamp().notNull().defaultNow()` on `createdAt` is deliberately + emitted optional. Unknown shapes answer "nullable": declining costs reads, + claiming wrongly moves rows. +- The nullable guard applies only when there is more than one spec. Gating a + single spec would change output that already ships and give up its read bound. +- Kept `needsPostFetchSortForPrimary` separate from the whole-sort answer. Fusing + them would make an index that anchors the primary but not the tie-break report + itself as unindexed, which throws under strict cursor pagination. +- Left `_compareByOrderSpecs` alone. Aligning it with Convex's value order would + fix the pre-existing single-field null divergence but is a breaking change and + needs user sign-off (recorded under Open risks). +- Left the multi-probe cursor stream ordering by `primaryOrder.direction`: that + stream never calls `.withIndex()`, so a direction resolved against another + index would be meaningless there. Its warning guard was reverted to + `hasSecondaryOrders` so it keeps firing. + +Implementation notes: +- `resolveIndexOrderPushdown` now walks the spec list: eq-pinned fields are + absorbed at any position/direction without consuming an index key, the rest + must be contiguous from `indexFields[eqCount]` in one shared direction, and + `_creationTime` is legal only as the final spec once every declared key is + consumed. `indexFields: []` models the default `by_creation_time` index. +- `usePostFetchSort` is now `postFetchOrders.length > 0 && orderPushdownDirection === null`. + Whole-served implies primary-served, so every case the old + `needsPostFetchSortForPrimary || hasSecondaryOrders` sorted post-fetch still does. +- `indexOrderBonus` counts the served run instead of testing position 0 only, so + `served >= 1` is exactly the old condition and longer coverage breaks ties. + `findLeadingIndex` picks the longest served run the same way. + +Review fixes: +- autoreview P0 #1 (multi-probe `in` union may skip the JS sort): refuted at + `query.ts:6621` — the union sort is unconditional. Added `in` + multi-field + order tests anyway, with both a pinned and an unpinned leading spec. +- autoreview P0 #2 (two cursor sites keep `primaryOrder.direction`): partly + valid. The scan-fallback stream does mirror the resolved index, so it now uses + the resolved direction when a caller index is pinned. The multi-probe stream + anchors to no index, so it keeps `primaryOrder.direction` and its warning was + reverted to fire on any secondary order. Warning text corrected to say the + fields are ignored because the page is not read through an index that orders + by them. +- Re-ran autoreview after the fixes: clean, no accepted/actionable findings. + +Error attempts: +| Error / failed attempt | Count | Next different move | Resolution | +|------------------------|-------|---------------------|------------| +| None yet | 0 | | | + +Verification evidence: +- cwd for every command below: /Users/mikey/conductor/workspaces/kitcn/dakar + (repo root, which owns both the package source and the convex integration tests). +- `bunx vitest run convex/orm/order-pushdown-reads.test.ts` -> 8 passed. + With `git stash push -- packages/kitcn/src`, 5 of 8 fail: 60->5, 60->5, 15->3, + 61->3, and the cursor page returns ascending for a descending request. +- `bun test packages/kitcn/src/orm/index-utils.test.ts` -> 24 passed. +- `bunx vitest run convex/orm/` -> 480 passed, 13 skipped. +- `bun run test:vitest` -> 879 passed, 13 skipped (83 files). +- `bun run test:bun` -> 1323 passed, 0 fail (148 files). +- `bun typecheck` -> 5 tasks successful. +- `bun lint:fix` then `bun lint` -> no fixes applied. +- `bun --cwd packages/kitcn build` -> 71 files, build complete. +- `bunx intent validate skills` -> 1 skill file passed; `intent:stale` -> up to date. +- `autoreview --mode local --engine claude` -> clean, "patch is correct" (0.7). + +Source-listed case matrix: +| Case | Source claim | Harness | Before | Expected after | Evidence | Status | +| --- | --- | --- | --- | --- | --- | --- | +| 2-field sort, limit 5 | 200 reads | order-pushdown-reads.test.ts | 60 / 200 reads | 5 reads | 8/8 green, 60->5 pre-fix | done | +| 2-field sort, limit 50 | limit-independent 200 reads | same, two table sizes | scan-shaped | bound-shaped | same absolute bound at 60 and 200 rows | done | +| relation loader | 201 reads vs 6 achievable | relation case | 61 / 201 reads | 3 reads | 61->3 pre-fix | done | +| null ordering diverges | must not change output | nullable-column case | nulls last | nulls last | multi-spec guard; green both ways | done | +| .order() reverses key tuple | mixed directions unservable | helper unit matrix | declined | declined | index-utils.test.ts | done | +| _creationTime last spec only | only after all fields consumed | helper unit matrix | n/a | declined unless final | index-utils.test.ts | done | +| eq-pinned any position/direction | must be preserved | helper unit + eq-pinned read test | served | served | 15->3 pre-fix | done | +| index selection leading-field-only | needs whole-spec selection | selection branch | fields[0] match | whole-sort match, same fallback | full suite green | done | +| multi-probe bail (:6404) | separate bail | `in` union case | collect | bounded, order preserved | order asserted both pinned and unpinned primary | done | + +Final handoff contract: +- Commit line: single commit at the tip of `issue-416-v1`, not pushed +- PR line: N/A: user explicitly declined PR creation +- Issue line: N/A: issue sync-back references a PR, and PR creation was declined +- Confidence line: 95-100% +- Flow table: + - Reproduced: tests RED (60 reads for a limit-5 query; 61 for a relation; + ascending cursor page for a descending request), browser N/A + - Verified: tests GREEN (8/8 new, 879 vitest, 1323 bun test), browser N/A +- Browser check: N/A: server-side query planner, no rendered output +- Outcome: a multi-field `orderBy` that a declared compound index already + produces is now walked instead of collected and re-sorted, so `limit` reads + `limit` rows at any table size, on both the top-level query and relations. +- Caveat: sorts that mix directions, skip an index key, or run over a column + that can be missing or null still post-fetch sort by design — that is what + keeps row order and null placement unchanged. +- Design: + - Chosen boundary: `resolveIndexOrderPushdown` stays the single owner of + "does scanning this index already produce the requested order?", and the + planner reads its answer instead of re-deriving one from `orderSpecs.length`. + Nullability rides on `OrderSpec`, so the relation loader and + `findRelationIndex` inherit the guard with no new parameter. + - Why not quick patch: narrowing only the helper is a no-op — three call + sites truncated the spec list and two planner booleans re-decided it on + arity. Fixing one without the others changes nothing. + - Why not broader change: aligning `_compareByOrderSpecs` with Convex's value + order would remove the nullable carve-out entirely, but it silently changes + shipped single-field results and is a breaking change needing sign-off. +- Verified: see Verification evidence +- PR body verified: N/A: no PR created + +Task-style PR body contract: +- Preserve any existing `` block. If a changeset is + part of the diff and repo policy expects auto release, include that block. +- Use the accepted PR #270 visual format. The body starts with an emoji + issue/fix line, for example `🐛 Fixes #123` or `🐛 Fixes ➖ N/A`, then + `🧭 Task plan: docs/plans/.md`, then an emoji confidence line like + `🟢 95-100% confidence`. +- Use this exact table header: `| Phase | 🧪 Tests | 🌐 Browser |`. +- Use `Reproduced` and `Verified` rows. Mark passing proof with `🟢`, repro or + failing proof with `🔴`, and non-applicable cells with `➖ N/A`. +- Use bold emoji section headings: `**✅ Outcome**`, `**⚠️ Caveat**`, + `**🏗️ Design**`, and `**🧪 Verified**`. +- Never include a line that links to the current PR itself. The current PR URL + belongs in the final response, not in its own description. +- Do not replace this with a generic `Summary` / `Verification` PR body, an + adaptive prose body from a git helper skill, plain `## Outcome` sections, or + an unrelated generated badge footer unless the caller or repo template + explicitly asks for it. +- Proof is `gh pr view --json body` output or a concise source-backed summary + of that output. + +Final handoff / sync: +- Commit: single commit at the tip of `issue-416-v1`, not pushed +- PR: N/A: explicit user decline +- Issue: N/A: sync-back references a PR +- Browser proof: N/A: no browser surface +- Caveats: nullable sort columns and mixed-direction sorts keep the post-fetch + sort by design; the pre-existing single-field null-placement divergence is + recorded under Open risks and left for user sign-off. + +Timeline: +- 2026-08-21T22:05:54.295Z Task goal plan created. + +Reboot status: +| Question | Answer | +|----------|--------| +| Where am I? | Closeout complete | +| Where am I going? | Final response | +| What is the goal? | Let a 2+ field `orderBy` walk a compound index that already produces it, with byte-identical results | +| What have I learned? | See Findings | +| What have I done? | See Timeline | + +High-risk note: +- Realistic failure mode: the helper claims an order the index cannot serve, the + planner skips the JS sort, and rows come back silently mis-ordered — or, in the + relation loader, each parent truncates the wrong top-k and rows are lost before + the global sort ever sees them. No error, no exception. +- Proof plan: a 24-case helper unit matrix pinning every accept and every + decline, plus integration tests that assert both the read bound at two table + sizes AND the exact row order, including the eq-pinned divergent-direction + case, the `in` union with pinned and unpinned leading specs, cursor pages, and + null placement. 5 of 8 integration cases verified RED against stashed source. +- Why this boundary is right: the helper already documented itself as the single + owner of the decision; the bug was two other places re-deciding it from arity. + Moving nullability onto `OrderSpec` keeps that ownership intact instead of + teaching each caller the rule. + +Open risks: +- Pre-existing and NOT fixed here: `_compareByOrderSpecs` places null/undefined + last in both directions while a Convex index scan places them first ascending. + A single-field `orderBy` on a nullable column therefore already returns a + different null placement depending on whether an index exists. This change + neither widens nor narrows that; it only refuses to let a multi-field sort + inherit it. Aligning the comparator with Convex's value order is the real fix + and is a breaking change that needs user sign-off. +- Pre-existing: two cursor branches (`query.ts` multi-probe + maxScan, and the + scan fallback without a caller index) build a stream with no index anchor, so + even the primary sort field is not carried and no warning says so. +- `indexOrderBonus` is a heuristic. It now prefers the index that serves more of + the sort, but it still cannot outrank filter selectivity, so a query whose best + filter index differs from its best order index will still post-fetch sort. + +Hard closeout guard: +- A local-only final response for verified code-changing work is invalid unless + this plan records an explicit user decline, no local patch, analytical/ + blocked/inconclusive outcome, or a real commit/PR blocker. diff --git a/packages/kitcn/skills/kitcn/references/features/orm.md b/packages/kitcn/skills/kitcn/references/features/orm.md index 4cd2513ca..9ec730154 100644 --- a/packages/kitcn/skills/kitcn/references/features/orm.md +++ b/packages/kitcn/skills/kitcn/references/features/orm.md @@ -257,9 +257,13 @@ Rules: Relation `limit`/`orderBy` are pushed into the relation index when the index already walks in that order. After the FK, `index('by_user').on(t.userId)` orders by creation time, so `with: { posts: { limit: 5, orderBy: { createdAt: -'desc' } } }` reads 5 posts per parent. Sorting by any other column reads the -parent's whole child partition and sorts in memory — put the sort column in the -relation index (`index('by_user_rank').on(t.userId, t.rank)`) to stay bounded. +'desc' } } }` reads 5 posts per parent. Multi-field sorts are pushed down too +when the index carries all of them: `index('by_user_rank').on(t.userId, t.rank)` +serves `orderBy: { rank: 'asc', createdAt: 'asc' }`, since the FK pins `userId`, +`rank` is the next key and `createdAt` is Convex's implicit trailing one. +Sorting by any other column — or by one that can be missing or `null` — reads +the parent's whole child partition and sorts in memory; put the sort columns in +the relation index, in sort order, to stay bounded. ### Nested `with` depth diff --git a/packages/kitcn/src/orm/index-utils.test.ts b/packages/kitcn/src/orm/index-utils.test.ts index 33e431850..5a570142f 100644 --- a/packages/kitcn/src/orm/index-utils.test.ts +++ b/packages/kitcn/src/orm/index-utils.test.ts @@ -297,14 +297,15 @@ describe('resolveIndexOrderPushdown', () => { ).toBeNull(); }); - test('declines multi-field sorts because .order() reverses the whole key', () => { + test('declines a multi-field sort that changes direction partway', () => { + // `.order()` reverses the whole key tuple, so a scan is one direction. expect( resolveIndexOrderPushdown({ indexFields: ['authorId', 'numLikes'], pinnedEqCount: 1, orderSpecs: [ - { field: 'numLikes', direction: 'desc' }, - { field: 'title', direction: 'asc' }, + { field: 'numLikes', direction: 'desc', nullable: false }, + { field: 'title', direction: 'asc', nullable: false }, ], }) ).toBeNull(); @@ -319,4 +320,176 @@ describe('resolveIndexOrderPushdown', () => { }) ).toBeNull(); }); + + describe('multi-field sorts', () => { + const spec = (field: string, direction: 'asc' | 'desc') => ({ + field, + direction, + nullable: false, + }); + + test('serves a sort that is a prefix of the index, in index order', () => { + expect( + resolveIndexOrderPushdown({ + indexFields: ['type', 'numLikes'], + pinnedEqCount: 0, + orderSpecs: [spec('type', 'asc'), spec('numLikes', 'asc')], + }) + ).toBe('asc'); + + expect( + resolveIndexOrderPushdown({ + indexFields: ['type', 'numLikes', 'title'], + pinnedEqCount: 0, + orderSpecs: [spec('type', 'desc'), spec('numLikes', 'desc')], + }) + ).toBe('desc'); + }); + + test('declines a sort that skips or reorders an index key', () => { + // (type, numLikes) walks every numLikes inside one type, so sorting by + // title next means re-sorting inside each of those buckets. + expect( + resolveIndexOrderPushdown({ + indexFields: ['type', 'numLikes', 'title'], + pinnedEqCount: 0, + orderSpecs: [spec('type', 'asc'), spec('title', 'asc')], + }) + ).toBeNull(); + + expect( + resolveIndexOrderPushdown({ + indexFields: ['type', 'numLikes'], + pinnedEqCount: 0, + orderSpecs: [spec('numLikes', 'asc'), spec('type', 'asc')], + }) + ).toBeNull(); + }); + + test('absorbs an eq-pinned field at any position and any direction', () => { + // `type` is constant across the scan, so it is already sorted whichever + // way the caller asked and wherever it sits in the sort. + expect( + resolveIndexOrderPushdown({ + indexFields: ['type', 'numLikes'], + pinnedEqCount: 1, + orderSpecs: [spec('type', 'asc'), spec('numLikes', 'desc')], + }) + ).toBe('desc'); + + expect( + resolveIndexOrderPushdown({ + indexFields: ['type', 'numLikes'], + pinnedEqCount: 1, + orderSpecs: [spec('numLikes', 'desc'), spec('type', 'asc')], + }) + ).toBe('desc'); + }); + + test('serves _creationTime only as the final spec', () => { + expect( + resolveIndexOrderPushdown({ + indexFields: ['type', 'numLikes'], + pinnedEqCount: 0, + orderSpecs: [ + spec('type', 'asc'), + spec('numLikes', 'asc'), + spec('_creationTime', 'asc'), + ], + }) + ).toBe('asc'); + + // Nothing follows the implicit trailing key. + expect( + resolveIndexOrderPushdown({ + indexFields: ['type'], + pinnedEqCount: 0, + orderSpecs: [ + spec('type', 'asc'), + spec('_creationTime', 'asc'), + spec('numLikes', 'asc'), + ], + }) + ).toBeNull(); + + // ...and it is only reached once every declared field is consumed. + expect( + resolveIndexOrderPushdown({ + indexFields: ['type', 'numLikes'], + pinnedEqCount: 0, + orderSpecs: [spec('type', 'asc'), spec('_creationTime', 'asc')], + }) + ).toBeNull(); + }); + + test('declines when a sort column can be missing or null', () => { + // A Convex index sorts absent and null first; the post-fetch comparator + // sorts them last. Only a sort the two agree on may move into the index. + expect( + resolveIndexOrderPushdown({ + indexFields: ['authorId', 'numLikes'], + pinnedEqCount: 0, + orderSpecs: [ + { field: 'authorId', direction: 'asc', nullable: true }, + spec('numLikes', 'asc'), + ], + }) + ).toBeNull(); + + // Unknown nullability counts as nullable. + expect( + resolveIndexOrderPushdown({ + indexFields: ['type', 'numLikes'], + pinnedEqCount: 0, + orderSpecs: [ + { field: 'type', direction: 'asc' }, + { field: 'numLikes', direction: 'asc' }, + ], + }) + ).toBeNull(); + + // An eq-pinned nullable field is constant across the scan, so the two + // orders cannot disagree about it. + expect( + resolveIndexOrderPushdown({ + indexFields: ['authorId', 'numLikes'], + pinnedEqCount: 1, + orderSpecs: [ + { field: 'authorId', direction: 'asc', nullable: true }, + spec('numLikes', 'asc'), + ], + }) + ).toBe('asc'); + }); + + test('still serves a single nullable spec, as it always has', () => { + expect( + resolveIndexOrderPushdown({ + indexFields: ['deletedAt'], + pinnedEqCount: 0, + orderSpecs: [ + { field: 'deletedAt', direction: 'asc', nullable: true }, + ], + }) + ).toBe('asc'); + }); + + test('models the default by_creation_time index as an empty key list', () => { + expect( + resolveIndexOrderPushdown({ + indexFields: [], + pinnedEqCount: 0, + orderSpecs: [spec('_creationTime', 'desc')], + }) + ).toBe('desc'); + + expect( + resolveIndexOrderPushdown({ + indexFields: [], + pinnedEqCount: 0, + orderSpecs: [spec('_creationTime', 'desc'), spec('numLikes', 'desc')], + }) + ).toBeNull(); + }); + }); }); diff --git a/packages/kitcn/src/orm/index-utils.ts b/packages/kitcn/src/orm/index-utils.ts index 8b2f82183..1e9931103 100644 --- a/packages/kitcn/src/orm/index-utils.ts +++ b/packages/kitcn/src/orm/index-utils.ts @@ -103,17 +103,32 @@ const hasColumnPrefix = (index: TableIndex, columns: readonly string[]) => { return true; }; -export type OrderSpec = { field: string; direction: 'asc' | 'desc' }; +export type OrderSpec = { + field: string; + direction: 'asc' | 'desc'; + /** + * Whether the column can be missing or null. Convex sorts absent before null + * before any value, so an ascending index scan puts them first; the ORM's + * post-fetch comparator puts them last in both directions. Absent is a legal + * stored value for every column that is not `.notNull()`, so the two orders + * only provably agree when this is `false`. + * + * Left undefined by callers that cannot answer, which counts as nullable: + * declining a pushdown costs reads, claiming a false one moves rows. + */ + nullable?: boolean; +}; /** * Answers "does scanning this index already produce the requested order?". * * Convex walks an index in full key order, so once a leading run of fields is * pinned to a single value by `eq`, the remainder of the scan is already sorted - * by the next index field — and by `_creationTime` once every declared field is - * pinned, because Convex appends it as the implicit trailing key. When that - * field is the one the caller asked to sort by, `.order(dir).take(n)` returns - * the exact page and nothing has to be collected and sorted afterwards. + * by the next index field, then the one after that — and by `_creationTime` + * once every declared field is consumed, because Convex appends it as the + * implicit trailing key. When that sequence is the one the caller asked to sort + * by, `.order(dir).take(n)` returns the exact page and nothing has to be + * collected and sorted afterwards. * * This is the single owner of that decision. The top-level query planner and * the relation loader both call it, so a bound one of them can push into the @@ -123,40 +138,77 @@ export type OrderSpec = { field: string; direction: 'asc' | 'desc' }; * after the fetch. */ export function resolveIndexOrderPushdown(params: { - /** Declared fields of the index being scanned, in key order. */ + /** Declared fields of the index being scanned, in key order. Empty models + * the default `by_creation_time` index, whose only key is the implicit + * `_creationTime` suffix. */ indexFields: readonly string[] | null | undefined; /** How many leading index fields are pinned to one value by `eq`. */ pinnedEqCount: number; - /** - * Requested sort. `.order()` reverses the whole key tuple rather than one - * column, so only a single spec is ever servable. - */ + /** Requested sort, in priority order. */ orderSpecs: readonly OrderSpec[]; }): 'asc' | 'desc' | null { const { indexFields, pinnedEqCount, orderSpecs } = params; // No index means a `.filter()` scan whose order is the table's, not the // partition's, so nothing can be pushed down. - if (!indexFields || orderSpecs.length !== 1) { + if (!indexFields || orderSpecs.length === 0) { return null; } - const primary = orderSpecs[0]; const eqCount = Math.min(Math.max(pinnedEqCount, 0), indexFields.length); - // An eq-pinned field holds one value for the whole scan, so the rows are - // trivially in order by it whichever direction was asked for. - for (let i = 0; i < eqCount; i += 1) { - if (indexFields[i] === primary.field) { - return primary.direction; + // trivially in order by it whichever direction was asked for, and wherever + // in the sort it appears. It consumes no index position. + const pinnedFields = new Set(indexFields.slice(0, eqCount)); + + /** Next unpinned index key the scan will sort by. */ + let cursor = eqCount; + /** Direction every unpinned spec has to agree on. */ + let direction: 'asc' | 'desc' | null = null; + let consumedCreationTime = false; + let hasNullableSpec = false; + + for (const spec of orderSpecs) { + if (pinnedFields.has(spec.field)) { + continue; } + // `.order()` reverses the whole key tuple rather than one column, so a + // sort that changes direction partway through is not a scan of anything. + if (direction === null) { + direction = spec.direction; + } else if (direction !== spec.direction) { + return null; + } + if (spec.nullable !== false) { + hasNullableSpec = true; + } + // Nothing follows the implicit `_creationTime` suffix. + if (consumedCreationTime) { + return null; + } + if (cursor >= indexFields.length) { + if (spec.field !== INTERNAL_CREATION_TIME_FIELD) { + return null; + } + consumedCreationTime = true; + continue; + } + if (spec.field !== indexFields[cursor]) { + return null; + } + cursor += 1; + } + + // A single spec is already served straight from the index today, including + // on nullable columns, so gating it here would change shipped output and + // give up the read bound at the same time. A multi-spec sort is the one that + // currently runs through the comparator, so it may only move into the index + // where the two provably agree. + if (orderSpecs.length > 1 && hasNullableSpec) { + return null; } - // Every declared field pinned means the only key left to walk is the implicit - // `_creationTime` suffix Convex appends. - const nativeField = - eqCount >= indexFields.length - ? INTERNAL_CREATION_TIME_FIELD - : indexFields[eqCount]; - return primary.field === nativeField ? primary.direction : null; + // Every spec was eq-pinned: the scan is constant across all of them, so any + // direction serves. Keep the caller's. + return direction ?? orderSpecs[0].direction; } export function findRelationIndexOrThrow( diff --git a/packages/kitcn/src/orm/query.ts b/packages/kitcn/src/orm/query.ts index 3b9984677..d164f7096 100644 --- a/packages/kitcn/src/orm/query.ts +++ b/packages/kitcn/src/orm/query.ts @@ -71,6 +71,7 @@ import { findSearchIndexByName, findVectorIndexByName, getIndexes, + type OrderSpec, resolveIndexOrderPushdown, } from './index-utils'; import { @@ -149,6 +150,30 @@ const RELATION_COUNT_ERROR = { FILTER_UNSUPPORTED: 'RELATION_COUNT_FILTER_UNSUPPORTED', } as const; const RELATION_DEPTH_ERROR = 'RELATION_DEPTH_EXCEEDED'; +/** + * Whether a compiled column validator admits an absent or null value. + * + * `v.optional(...)` is how a non-`.notNull()` column is emitted, and absent is + * a legal stored value for every document written before the column existed, so + * both halves matter. Unrecognized shapes answer true: the only caller uses + * this to decide whether it may skip a sort, and guessing "never null" there + * silently reorders rows. + */ +const validatorAcceptsMissingOrNull = (validator: any): boolean => { + if (!validator) { + return true; + } + if (validator.isOptional === 'optional') { + return true; + } + if (validator.kind === 'null' || validator.kind === 'any') { + return true; + } + if (validator.kind === 'union') { + return (validator.members ?? []).some(validatorAcceptsMissingOrNull); + } + return false; +}; /** * How many levels of `with` the relation loader will follow. A `with` config is * a finite object the caller wrote, so its own nesting is the real bound; this @@ -833,7 +858,15 @@ export class GelRelationalQuery< | Record | undefined, tableConfig: TableRelationalConfig = this.tableConfig - ): { field: string; direction: 'asc' | 'desc' }[] { + ): OrderSpec[] { + const withNullability = (spec: { + field: string; + direction: 'asc' | 'desc'; + }): OrderSpec => ({ + ...spec, + nullable: this._isNullableOrderField(spec.field, tableConfig), + }); + if ( orderBy && typeof orderBy === 'object' && @@ -854,18 +887,44 @@ export class GelRelationalQuery< direction: 'asc' | 'desc'; } => entry.direction === 'asc' || entry.direction === 'desc' ) - .map((entry) => ({ - field: entry.field, - direction: entry.direction, - })); + .map((entry) => + withNullability({ field: entry.field, direction: entry.direction }) + ); } return this._normalizeOrderBy( orderBy as ValueOrArray | undefined - ).map((clause) => ({ - field: clause.column.columnName, - direction: clause.direction, - })); + ).map((clause) => + withNullability({ + field: clause.column.columnName, + direction: clause.direction, + }) + ); + } + + /** + * Whether rows can be missing this sort column or hold null in it. + * + * Only the declared validator can answer: a column that is not `.notNull()` + * compiles to `v.optional(v.union(v.null(), T))`, so both absent and null are + * legal stored values — and `timestamp().notNull().defaultNow()` on + * `createdAt` is deliberately emitted as optional for migration safety, so + * reading `config.notNull` alone would answer wrong for it. Anything the + * lookup cannot resolve counts as nullable, which only costs a pushdown. + */ + private _isNullableOrderField( + field: string, + tableConfig: TableRelationalConfig + ): boolean { + if (field === INTERNAL_ID_FIELD || field === INTERNAL_CREATION_TIME_FIELD) { + return false; + } + try { + const builder = this._getColumns(tableConfig)[field] as any; + return validatorAcceptsMissingOrNull(builder?.build?.()); + } catch { + return true; + } } private _resolveNonPaginatedLimit(config: any): number | undefined { @@ -2646,7 +2705,7 @@ export class GelRelationalQuery< order: 'asc' | 'desc'; queryConfig: { index?: { name: string; filters: FilterExpression[] }; - order?: { direction: 'asc' | 'desc'; field: string }[]; + order?: OrderSpec[]; }; wherePredicate: ((row: any) => boolean | Promise) | undefined; }): Promise | null> { @@ -2735,7 +2794,7 @@ export class GelRelationalQuery< queryConfig: { index?: { name: string; filters: FilterExpression[] }; postFilters: FilterExpression[]; - order?: { direction: 'asc' | 'desc'; field: string }[]; + order?: OrderSpec[]; }, wherePredicate: ((row: any) => boolean | Promise) | undefined, configuredIndex?: PredicateWhereIndexConfig @@ -2819,6 +2878,12 @@ export class GelRelationalQuery< membershipFilter?: (row: any) => boolean | Promise; orderIndexName: string | null; primaryOrder?: { direction: 'asc' | 'desc'; field: string }; + /** + * Direction the mirrored index can be walked in to produce the whole sort, + * when the caller resolved one. It can differ from `primaryOrder.direction` + * once an eq-pinned field leads the sort, and it is the correct one then. + */ + pushdownDirection?: 'asc' | 'desc' | null; /** Direction to use when the query has no orderBy at all. */ fallbackOrder: 'asc' | 'desc'; }): QueryStream | null { @@ -2833,6 +2898,7 @@ export class GelRelationalQuery< membershipFilter, orderIndexName, primaryOrder, + pushdownDirection, fallbackOrder, } = params; @@ -2866,7 +2932,8 @@ export class GelRelationalQuery< // path, and explicitly orders 'desc' in the cursor path, so the caller // supplies which of the two applies. streamQuery = streamQuery.order( - primaryOrder ? primaryOrder.direction : fallbackOrder + pushdownDirection ?? + (primaryOrder ? primaryOrder.direction : fallbackOrder) ); streamQuery = streamQuery.filterWith(async (row: any) => { @@ -6117,6 +6184,45 @@ export class GelRelationalQuery< const primaryOrder = postFetchOrders[0]; const hasSecondaryOrders = postFetchOrders.length > 1; let orderIndexName: string | null = null; + /** + * Direction the scanned index can be walked in to produce the *whole* + * requested sort, or null when some of it still has to run in JavaScript. + * This is the read bound: a non-null answer means `take(limit)` is the + * exact page. + */ + let orderPushdownDirection: 'asc' | 'desc' | null = null; + + /** + * Ask the one owner both questions an index has to answer. + * + * `whole` decides the read bound. `primary` is narrower and only feeds the + * cursor-pagination diagnostics, which are about the leading sort field + * having an index at all — an index that anchors the primary but not the + * tie-break is still the right index to scan, and must not start reporting + * itself as unindexed. + */ + const resolveOrderForIndex = ( + indexFields: readonly string[] | null | undefined, + pinnedEqCount: number + ) => { + const whole = resolveIndexOrderPushdown({ + indexFields, + pinnedEqCount, + orderSpecs: postFetchOrders, + }); + return { + whole, + primary: + whole ?? + (primaryOrder + ? resolveIndexOrderPushdown({ + indexFields, + pinnedEqCount, + orderSpecs: [primaryOrder], + }) + : null), + }; + }; // Apply index if selected for WHERE filtering if (queryConfig.index) { @@ -6130,15 +6236,15 @@ export class GelRelationalQuery< return indexQuery; }); - // Check if orderBy field matches WHERE index + // Check if orderBy matches WHERE index if (primaryOrder) { - const pushdownDirection = resolveIndexOrderPushdown({ - indexFields: indexConfig.fields, - pinnedEqCount: this._indexEqPrefixCount(queryConfig), - orderSpecs: [primaryOrder], - }); - if (pushdownDirection) { - query = query.order(pushdownDirection); + const resolved = resolveOrderForIndex( + indexConfig.fields, + this._indexEqPrefixCount(queryConfig) + ); + orderPushdownDirection = resolved.whole; + if (resolved.primary) { + query = query.order(resolved.primary); } else { // Different field - need post-fetch sort needsPostFetchSortForPrimary = true; @@ -6159,16 +6265,13 @@ export class GelRelationalQuery< ); if (primaryOrder) { - const pushdownDirection = resolveIndexOrderPushdown({ - indexFields: configuredIndexFields, - pinnedEqCount: countConfiguredIndexEqPrefix( - configuredIndexFields, - rangeOperations - ), - orderSpecs: [primaryOrder], - }); - if (pushdownDirection) { - query = query.order(pushdownDirection); + const resolved = resolveOrderForIndex( + configuredIndexFields, + countConfiguredIndexEqPrefix(configuredIndexFields, rangeOperations) + ); + orderPushdownDirection = resolved.whole; + if (resolved.primary) { + query = query.order(resolved.primary); } else { needsPostFetchSortForPrimary = true; } @@ -6178,25 +6281,57 @@ export class GelRelationalQuery< const orderField = primaryOrder.field; // Special case: _creationTime uses Convex's default index - if (orderField === '_creationTime') { - // Default index on _creationTime - no withIndex() needed + if (orderField === INTERNAL_CREATION_TIME_FIELD) { + // Default index on _creationTime - no withIndex() needed. An empty + // field list is exactly that index: its only key is the implicit + // `_creationTime` suffix, so nothing can follow it in the sort. query = query.order(primaryOrder.direction); + orderPushdownDirection = resolveIndexOrderPushdown({ + indexFields: [], + pinnedEqCount: 0, + orderSpecs: postFetchOrders, + }); } else { // Convex walks an index in full index-key order, so only an index - // whose *leading* field is the sort field yields the requested order. - // Matching on any position sorts by a different column entirely. - const orderIndex = - getIndexes(this.tableConfig.table).find( - (idx) => idx.fields[0] === orderField - ) ?? - this.edgeMetadata.find((idx) => idx.indexFields[0] === orderField); - - if (orderIndex) { - orderIndexName = - 'indexName' in orderIndex ? orderIndex.indexName : orderIndex.name; + // whose *leading* keys are the requested sort keys, in order, yields + // the requested order. Matching on any other position sorts by a + // different column entirely. + const candidates: { fields: readonly string[]; name: string }[] = [ + ...getIndexes(this.tableConfig.table).map((idx) => ({ + fields: idx.fields, + name: idx.name, + })), + ...this.edgeMetadata.map((idx) => ({ + fields: idx.indexFields, + name: idx.indexName, + })), + ]; + + let selected: { fields: readonly string[]; name: string } | undefined; + for (const candidate of candidates) { + const direction = resolveIndexOrderPushdown({ + indexFields: candidate.fields, + pinnedEqCount: 0, + orderSpecs: postFetchOrders, + }); + if (direction) { + selected = candidate; + orderPushdownDirection = direction; + break; + } + } + // Nothing serves the whole sort. Anchoring the scan on the primary + // field alone is still worth it: the tie-break then runs in JavaScript + // over rows that are already in the right primary order. + selected ??= candidates.find( + (candidate) => candidate.fields[0] === orderField + ); + + if (selected) { + orderIndexName = selected.name; // Use orderBy field's index query = query.withIndex(orderIndexName, (q: any) => q); - query = query.order(primaryOrder.direction); + query = query.order(orderPushdownDirection ?? primaryOrder.direction); } else { // No index for orderBy field - post-fetch sort needsPostFetchSortForPrimary = true; @@ -6204,7 +6339,21 @@ export class GelRelationalQuery< } } - usePostFetchSort = needsPostFetchSortForPrimary || hasSecondaryOrders; + // The sort runs in JavaScript unless the index walk produced all of it. + // Testing arity here instead is what made every 2-field `orderBy` collect + // the table: an index that serves the whole tuple was scanned in the right + // order and then thrown away. + usePostFetchSort = + postFetchOrders.length > 0 && orderPushdownDirection === null; + // A Convex cursor is the serialized index key, so once the page is read + // through an index that carries every sort field, the page boundary carries + // them too. Warning about cross-page instability there would be false. + // + // Only for pages actually read through that index, though. Two cursor + // branches below build a stream that anchors to a different index or to + // none at all, so they answer this question for themselves. + const hasUnservedSecondaryOrders = + hasSecondaryOrders && orderPushdownDirection === null; if (wherePredicate) { const predicateIndex = configuredIndex; @@ -6242,16 +6391,23 @@ export class GelRelationalQuery< 'Falling back to _creationTime ordering.' ); } - if (hasSecondaryOrders) { + if (hasUnservedSecondaryOrders) { console.warn( 'Pagination: Only the first orderBy field is used for cursor ordering. ' + - 'Secondary orderBy fields are applied per page and may be unstable across pages.' + 'Secondary orderBy fields are ignored because no index serves the whole sort.' ); } } if (primaryOrder && !needsPostFetchSortForPrimary) { - streamQuery = streamQuery.order(primaryOrder.direction); + // This stream walks `predicateIndex` with the caller's own range, which + // is the index and pinned prefix the branch above resolved against. + // Prefer its answer: once an eq-pinned field leads the sort, the scan + // has to follow the first spec that actually moves, which can point the + // other way from `primaryOrder`. + streamQuery = streamQuery.order( + orderPushdownDirection ?? primaryOrder.direction + ); } else if (isCursorPaginated) { streamQuery = streamQuery.order('desc'); } @@ -6388,20 +6544,17 @@ export class GelRelationalQuery< isRlsEnabled(this.tableConfig.table as any)); // Each probe is read in its own index order. Truncating one is only sound // when that order is the requested order, so the global top-k is - // guaranteed to live inside the union of the per-probe top-k. - const probeOrderDirection = primaryOrder - ? resolveIndexOrderPushdown({ - indexFields: queryConfig.index.fields, - pinnedEqCount: this._indexEqPrefixCount(queryConfig), - orderSpecs: [primaryOrder], - }) - : null; + // guaranteed to live inside the union of the per-probe top-k. Every probe + // walks the same index with the same pinned prefix the branch above + // already resolved, and `_indexEqPrefixCount` takes the minimum across + // probes — so the answer that holds for the least-pinned probe holds for + // all of them. + const probeOrderDirection = orderPushdownDirection; const probeBound = probeLimit !== undefined && !probeHasResidualFilter && !probeHasPostFetchMembership && - (postFetchOrders.length === 0 || - (probeOrderDirection !== null && !hasSecondaryOrders)) + (postFetchOrders.length === 0 || probeOrderDirection !== null) ? probeOffset + probeLimit : undefined; @@ -6546,10 +6699,13 @@ export class GelRelationalQuery< 'Falling back to _creationTime ordering.' ); } + // This stream never anchors to an index, so it reads creation + // order whatever the sort asked for. An index that could have + // carried the whole tuple is irrelevant to this page. if (hasSecondaryOrders) { console.warn( 'Pagination: Only the first orderBy field is used for cursor ordering. ' + - 'Secondary orderBy fields are applied per page and may be unstable across pages.' + 'Secondary orderBy fields are ignored because this page is not read through an index that orders by them.' ); } streamQuery = streamQuery.order(primaryOrder.direction); @@ -6647,13 +6803,21 @@ export class GelRelationalQuery< 'Falling back to _creationTime ordering.' ); } - if (hasSecondaryOrders) { - console.warn( - 'Pagination: Only the first orderBy field is used for cursor ordering. ' + - 'Secondary orderBy fields are applied per page and may be unstable across pages.' - ); + // This stream anchors to `configuredIndex` or to nothing at all, + // so an index the planner picked for the sort is not what gets + // walked here — and without the caller's index it reads creation + // order. The sort is only carried when both are true. + if (configuredIndex?.name && orderPushdownDirection) { + streamQuery = streamQuery.order(orderPushdownDirection); + } else { + if (hasSecondaryOrders) { + console.warn( + 'Pagination: Only the first orderBy field is used for cursor ordering. ' + + 'Secondary orderBy fields are ignored because this page is not read through an index that orders by them.' + ); + } + streamQuery = streamQuery.order(primaryOrder.direction); } - streamQuery = streamQuery.order(primaryOrder.direction); } else { streamQuery = streamQuery.order('desc'); } @@ -6724,6 +6888,7 @@ export class GelRelationalQuery< membershipFilter: matchesPostFetchMembership, orderIndexName, primaryOrder, + pushdownDirection: orderPushdownDirection, fallbackOrder: 'desc', }) : null; @@ -6745,10 +6910,10 @@ export class GelRelationalQuery< 'Falling back to _creationTime ordering.' ); } - if (hasSecondaryOrders) { + if (hasUnservedSecondaryOrders) { console.warn( 'Pagination: Only the first orderBy field is used for cursor ordering. ' + - 'Secondary orderBy fields are applied per page and may be unstable across pages.' + 'Secondary orderBy fields are ignored because no index serves the whole sort.' ); } } @@ -6804,10 +6969,10 @@ export class GelRelationalQuery< // Ordering already applied via index - query is ready for pagination // No additional action needed } - if (hasSecondaryOrders) { + if (hasUnservedSecondaryOrders) { console.warn( 'Pagination: Only the first orderBy field is used for cursor ordering. ' + - 'Secondary orderBy fields are applied per page and may be unstable across pages.' + 'Secondary orderBy fields are ignored because no index serves the whole sort.' ); } } else { @@ -6904,6 +7069,7 @@ export class GelRelationalQuery< membershipFilter: matchesPostFetchMembership, orderIndexName, primaryOrder, + pushdownDirection: orderPushdownDirection, fallbackOrder: 'asc', }) : null; @@ -7010,7 +7176,7 @@ export class GelRelationalQuery< }; probeFilters: FilterExpression[][]; postFilters: FilterExpression[]; - order?: { direction: 'asc' | 'desc'; field: string }[]; + order?: OrderSpec[]; } { const config = this.config as any; @@ -7027,7 +7193,7 @@ export class GelRelationalQuery< // Resolved before index selection: which index is cheapest depends on // whether it can also supply the requested order. - let orderSpecs: { direction: 'asc' | 'desc'; field: string }[] = []; + let orderSpecs: OrderSpec[] = []; if (config.orderBy) { const orderByValue = typeof config.orderBy === 'function' @@ -7090,7 +7256,7 @@ export class GelRelationalQuery< }; probeFilters: FilterExpression[][]; postFilters: FilterExpression[]; - order?: { direction: 'asc' | 'desc'; field: string }[]; + order?: OrderSpec[]; } = { table: this.tableConfig.table.tableName, strategy: compiled.strategy, diff --git a/packages/kitcn/src/orm/where-clause-compiler.test.ts b/packages/kitcn/src/orm/where-clause-compiler.test.ts index 8e66ea453..d94dbf014 100644 --- a/packages/kitcn/src/orm/where-clause-compiler.test.ts +++ b/packages/kitcn/src/orm/where-clause-compiler.test.ts @@ -413,7 +413,9 @@ describe('timestamp mode key normalization', () => { test('accepts createdAt in orderBy object', () => { const query = createQuery(); const specs = (query as any)._orderBySpecs({ createdAt: 'asc' }); - expect(specs).toEqual([{ field: '_creationTime', direction: 'asc' }]); + expect(specs).toEqual([ + { field: '_creationTime', direction: 'asc', nullable: false }, + ]); }); test('rejects _creationTime in orderBy object', () => { @@ -426,7 +428,9 @@ describe('timestamp mode key normalization', () => { test('always maps createdAt to system _creationTime even if a user column exists', () => { const query = createQuery(usersWithCreatedAt); const specs = (query as any)._orderBySpecs({ createdAt: 'asc' }); - expect(specs).toEqual([{ field: '_creationTime', direction: 'asc' }]); + expect(specs).toEqual([ + { field: '_creationTime', direction: 'asc', nullable: false }, + ]); }); test('still rejects _creationTime for migration', () => { diff --git a/packages/kitcn/src/orm/where-clause-compiler.ts b/packages/kitcn/src/orm/where-clause-compiler.ts index 636881ba3..b27378656 100644 --- a/packages/kitcn/src/orm/where-clause-compiler.ts +++ b/packages/kitcn/src/orm/where-clause-compiler.ts @@ -615,17 +615,23 @@ export class WhereClauseCompiler { const candidates = this.availableIndexes .filter((index) => index.indexFields[0] === fieldName) .sort((a, b) => a.indexFields.length - b.indexFields.length); - // Each probe pins the leading field, so an index whose second key is the - // sort field serves the order from the scan. Otherwise the narrowest index - // wins, since extra keys only widen the range without pinning anything. + // Each probe pins the leading field, so an index whose next keys are the + // sort fields serves the order from the scan — and the more of the sort it + // carries, the more of the per-probe read bound survives. Otherwise the + // narrowest index wins, since extra keys only widen the range without + // pinning anything. if (this.orderFields.length > 0) { - const orderField = this.orderFields[0]; - const serving = candidates.find( - (index) => - index.indexFields.length > 1 && index.indexFields[1] === orderField - ); - if (serving) { - return serving; + let best: IndexLike | null = null; + let bestServed = 0; + for (const index of candidates) { + const served = this.servedOrderFieldCount(index.indexFields, 1); + if (served > bestServed) { + best = index; + bestServed = served; + } + } + if (best) { + return best; } } return candidates[0] ?? null; @@ -928,14 +934,40 @@ export class WhereClauseCompiler { * Reward an index that also supplies the requested order. Large enough to * clear the 25-point exact-over-prefix premium, so `(orgId, createdAt)` beats * `(orgId)` for `where { orgId } orderBy { createdAt }`. + * + * The reward grows with how much of the sort the index carries, so + * `(orgId, createdAt, title)` beats `(orgId, createdAt)` for a two-field + * sort: only the longer one lets the scan produce the whole tuple, and + * picking the shorter one costs the entire read bound. The extra point per + * field is deliberately small — order coverage breaks ties between indexes + * that already serve the filter equally well, it does not outrank filtering. */ private indexOrderBonus(indexFields: string[], pinnedLength: number): number { if (this.orderFields.length === 0 || pinnedLength >= indexFields.length) { return 0; } - return indexFields[pinnedLength] === this.orderFields[0] - ? INDEX_ORDER_BONUS - : 0; + const served = this.servedOrderFieldCount(indexFields, pinnedLength); + return served === 0 ? 0 : INDEX_ORDER_BONUS + (served - 1); + } + + /** + * How many leading sort fields this index would produce for free, counting + * from its first unpinned key. Convex walks an index in full key order, so + * the run has to be contiguous and start at `pinnedLength`. + */ + private servedOrderFieldCount( + indexFields: string[], + pinnedLength: number + ): number { + let served = 0; + while ( + served < this.orderFields.length && + pinnedLength + served < indexFields.length && + indexFields[pinnedLength + served] === this.orderFields[served] + ) { + served += 1; + } + return served; } /** diff --git a/www/content/docs/orm/queries/index.mdx b/www/content/docs/orm/queries/index.mdx index 04fb32fb2..d01f0ab7d 100644 --- a/www/content/docs/orm/queries/index.mdx +++ b/www/content/docs/orm/queries/index.mdx @@ -176,10 +176,10 @@ const posts2 = await db.query.posts.findMany({ ``` **Notes:** -- The first `orderBy` field should have an index for stable ordering -- Secondary sort fields are applied post-fetch -- When post-fetch sorting is required (multi-field or non-indexable sort), the ORM sorts before applying `offset`/`limit`, which requires reading the full filtered candidate set first -- `findMany({ cursor, limit })` uses only the first `orderBy` field for cursor ordering +- An index that walks in the requested order serves the sort directly, so `limit` reads only the rows it returns. That needs the sort fields to be the index's leading keys, in order, all pointing the same way — `index('by_type_likes').on(t.type, t.numLikes)` serves `orderBy: { type: 'asc', numLikes: 'asc' }`. Fields pinned by an equality in `where` are constant across the scan, so they can appear anywhere in the sort and in either direction +- `createdAt` is servable as the last sort field once every other index key is pinned or consumed, because Convex appends it as the implicit trailing key +- Any other sort — mixed directions, a field the index does not sort by next, or a column that can be missing or `null` — is applied post-fetch. The ORM then sorts before applying `offset`/`limit`, which requires reading the full filtered candidate set first +- `findMany({ cursor, limit })` orders pages by every `orderBy` field the index serves. When the index cannot serve them all, only the first field orders the cursor and the rest are dropped, which the ORM warns about ## Pagination From 279a73554e8c7c6f21593a10746f53c21eabbeef Mon Sep 17 00:00:00 2001 From: MikeyZhang75 Date: Sat, 22 Aug 2026 07:43:14 +0800 Subject: [PATCH 2/8] docs(plans): record PR #426 in the task plan --- ...-orm-multi-field-orderby-index-pushdown.md | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md index 35030a719..1eb3ce1d9 100644 --- a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md +++ b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md @@ -183,7 +183,7 @@ Start Gates: | Skill analysis before edits | yes | task + autogoal (package-api pack) + changeset + autoreview; no others earned their keep | | Active goal checked or created | yes | this plan | | Source of truth read before edits | yes | gh issue view 416 (body + 0 comments) and the attachment | -| Exact per-PR task ownership | no | N/A: user explicitly declined PR creation | +| Exact per-PR task ownership | yes | this plan owns exactly PR #426 | | GitHub comments and attachments read | yes | gh issue view 416 --json comments -> 0 comments; attachment read | | Video transcript evidence required | no | N/A: no video or screen recording in the source | | Pre-solution issue challenge required | yes | see Pre-solution issue challenge; verdict valid, one constraint corrected | @@ -192,13 +192,13 @@ Start Gates: | Suggested fix reviewed against durable boundary | yes | adopted, with the nullable guard narrowed to multi-spec only | | `docs/solutions` checked for non-trivial existing-code work | yes | docs/solutions/logic-errors/aggregate-isnull-must-probe-the-absent-field-bucket-20260817.md confirms the optional-column storage model | | TDD decision before behavior change or bug fix | yes | RED read-count repro written before the fix; stash-verified 5 of 8 fail pre-fix | -| Branch decision for code-changing task | yes | already on issue-416-v1, which matches the issue | +| Branch decision for code-changing task | yes | renamed the placeholder `issue-416-v1` to `fix/orm-multi-field-orderby-index-pushdown` before the first push, per the user branch-naming preference | | Release artifact decision | yes | .changeset/nine-planets-marry.md (patch) | | Browser tool decision for browser surface | no | N/A: no browser surface | | Commit / PR expectation decision | yes | commit yes; push/PR declined by explicit user preference | -| Task-style PR body decision | no | N/A: no PR created (explicit user decline) | -| Task-plan PR body evidence | no | N/A: no PR created (explicit user decline) | -| GitHub issue sync expectation decision | no | N/A: issue sync-back is gated on a PR existing; user declined PR | +| Task-style PR body decision | yes | PR #270 emoji task-style body used | +| Task-plan PR body evidence | yes | body carries `🧭 Task plan: docs/plans/416-orm-multi-field-orderby-index-pushdown.md`; plan is at the PR head and names PR #426 | +| GitHub issue sync expectation decision | yes | `Fixes #416` in the PR body is the sync-back; no separate issue comment posted (not requested) | | Output budget strategy recorded | yes | workflow findings artifacted to /tmp/wf-facts.txt and grepped; single-file test runs | | Package/API pack selected | yes | package-api | | Public surface or package boundary identified | yes | no public export change; index-utils.ts is package-internal (only orm/ and cli/utils import it) | @@ -291,7 +291,7 @@ Completion Gates: | Gate | Applies | Required action | Evidence | |------|---------|-----------------|----------| | Named verification threshold | yes | Run the command, proof, source audit, or artifact check named in this plan | bunx vitest run convex/orm/order-pushdown-reads.test.ts -> 8 passed; 5 of 8 RED pre-fix (git stash verified) | -| Exact per-PR task ownership | no | Record the exact PR and dedicated plan, or the not-yet-created single-PR slice | N/A: no PR created (explicit user decline) | +| Exact per-PR task ownership | yes | Record the exact PR and dedicated plan, or the not-yet-created single-PR slice | PR #426, owned solely by this plan | | Pre-solution issue challenge verdict | yes | Record reporter claim, suggested fix, repro verdict, validity verdict, durable boundary, and hard-stop/pivot decision before implementation | valid; one constraint corrected (nullable guard is multi-spec only) | | Repro escalation ladder | yes | For bug/behavior claims, record test/source-level, automated browser/integration, Browser, and screenshot/visual-proof outcomes or N/A/blocker reasons before `not reproduced` | source-level test repro reproduced it; browser/visual N/A (no rendered output) | | Bug reproduced before fix | yes | Record failing test/repro or N/A with reason | expected 60 to be 5 on a 60-row table with limit 5 | @@ -312,11 +312,11 @@ Completion Gates: | Agent-native review for agent/tooling changes | no | For `.agents/**`, `.claude/**`, `.codex/**`, skills, hooks, commands, prompts, or user-action tooling, load `.agents/skills/agent-native-reviewer/SKILL.md` and close accepted/actionable findings, or record N/A | N/A: only the generated kitcn skill mirror changed, via its sync script; no .agents/.claude/.codex workflow, hook, command, or prompt touched | | Local install corruption suspected | yes | Run `bun install` once, rerun the exact failing command, or record N/A | 7 convex suites failed with Cannot find package kitcn/server; bun --cwd packages/kitcn build fixed all 7 (stale dist, not the diff) | | Commit created | yes | For verified code-changing work, stage the entire current checkout per repo policy and create a commit; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | single commit at the tip of `issue-416-v1`, whole checkout staged | -| PR create or update | no | For verified code-changing work, run `check`, push, create or update the PR, and sync PR body to the task-style final handoff; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | N/A: user explicitly declined PR creation ("Do not create PR under any circumstances, unless user prompts to") | -| Task-style PR body verified | no | Verify the PR body with `gh pr view --json body`; it must preserve auto-release blocks when applicable, must not include a current-PR self-link, and must use the PR #270 emoji format: `🐛 Fixes ...`, `🟢 95-100% confidence`, `Phase / 🧪 Tests / 🌐 Browser` table, and bold emoji Outcome/Caveat/Design/Verified sections | N/A: no PR created | -| PR task evidence verified | no | Verify body plan line, plan at PR head, and exact PR ownership | N/A: no PR created | -| PR proof image hosting | no | If PR body needs browser proof, replace local image paths with hosted GitHub URLs or record N/A | N/A: no PR and no image proof | -| GitHub issue sync-back | no | Post concise issue sync after PR exists, or record N/A/blocker | N/A: sync-back references a PR, and PR creation was declined | +| PR create or update | yes | For verified code-changing work, run `check`, push, create or update the PR, and sync PR body to the task-style final handoff; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | `bun check` exit 0, pushed, PR #426 created with the task-style body | +| Task-style PR body verified | yes | Verify the PR body with `gh pr view --json body`; it must preserve auto-release blocks when applicable, must not include a current-PR self-link, and must use the PR #270 emoji format: `🐛 Fixes ...`, `🟢 95-100% confidence`, `Phase / 🧪 Tests / 🌐 Browser` table, and bold emoji Outcome/Caveat/Design/Verified sections | `gh pr view 426 --json body`: auto-release block preserved, no self-link, all required sections present | +| PR task evidence verified | yes | Verify body plan line, plan at PR head, and exact PR ownership | plan line present, plan committed at PR head, names PR #426 | +| PR proof image hosting | no | If PR body needs browser proof, replace local image paths with hosted GitHub URLs or record N/A | N/A: no browser surface, so no image proof | +| GitHub issue sync-back | yes | Post concise issue sync after PR exists, or record N/A/blocker | `Fixes #416` in the PR body links the fix to the issue; no separate comment posted because the user requested only a PR | | Final handoff contract | yes | Fill the final handoff fields below with exact PR/issue/confidence/tests/browser/outcome/caveats/design/verification content or N/A reason | see Final handoff contract | | Final lint | yes | Run `bun lint:fix` or scoped equivalent | bun lint:fix then bun lint -> no fixes applied | | Output budget discipline | yes | Verify no unbounded high-volume command output was streamed, or record the accidental output and recovery | workflow output artifacted to /tmp/wf-facts.txt; no unbounded stream | @@ -339,7 +339,7 @@ Phase / pass table: | Intake and source read | done | issue #416 read, repro RED at 60 reads | implementation | | Implementation | done | helper rewrite + 5 call sites + index scoring + docs + changeset | verification | | Verification | done | 8/8 new tests, 879 vitest, 1323 bun test, typecheck, lint, build | closeout | -| Commit / PR / GitHub sync | done | commit created; push/PR/issue-sync declined by user preference | final response | +| Commit / PR / GitHub sync | done | `bun check` green, pushed, PR #426 opened with the task-style body | final response | | Closeout | done | autoreview clean on re-run | final response | Findings: @@ -455,9 +455,9 @@ Source-listed case matrix: | multi-probe bail (:6404) | separate bail | `in` union case | collect | bounded, order preserved | order asserted both pinned and unpinned primary | done | Final handoff contract: -- Commit line: single commit at the tip of `issue-416-v1`, not pushed -- PR line: N/A: user explicitly declined PR creation -- Issue line: N/A: issue sync-back references a PR, and PR creation was declined +- Commit line: single commit, pushed to `fix/orm-multi-field-orderby-index-pushdown` +- PR line: https://github.com/udecode/kitcn/pull/426 +- Issue line: #416 linked from the PR body via `Fixes #416` - Confidence line: 95-100% - Flow table: - Reproduced: tests RED (60 reads for a limit-5 query; 61 for a relation; @@ -483,7 +483,7 @@ Final handoff contract: order would remove the nullable carve-out entirely, but it silently changes shipped single-field results and is a breaking change needing sign-off. - Verified: see Verification evidence -- PR body verified: N/A: no PR created +- PR body verified: `gh pr view 426 --json body` — auto-release block preserved, emoji task-style sections present, no self-link Task-style PR body contract: - Preserve any existing `` block. If a changeset is @@ -507,9 +507,9 @@ Task-style PR body contract: of that output. Final handoff / sync: -- Commit: single commit at the tip of `issue-416-v1`, not pushed -- PR: N/A: explicit user decline -- Issue: N/A: sync-back references a PR +- Commit: single commit, pushed to `fix/orm-multi-field-orderby-index-pushdown` +- PR: https://github.com/udecode/kitcn/pull/426 +- Issue: #416, linked from the PR body via `Fixes #416` - Browser proof: N/A: no browser surface - Caveats: nullable sort columns and mixed-direction sorts keep the post-fetch sort by design; the pre-existing single-field null-placement divergence is From b2194e6e375dfd6e584aef48251fddd59a376de8 Mon Sep 17 00:00:00 2001 From: zbeyens Date: Wed, 26 Aug 2026 05:56:26 +0200 Subject: [PATCH 3/8] docs(plans): record #426 autoclosure proof --- ...-orm-multi-field-orderby-index-pushdown.md | 58 +++++++++++++------ 1 file changed, 40 insertions(+), 18 deletions(-) diff --git a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md index 1eb3ce1d9..e4d2037ff 100644 --- a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md +++ b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md @@ -290,17 +290,17 @@ Work Checklist: Completion Gates: | Gate | Applies | Required action | Evidence | |------|---------|-----------------|----------| -| Named verification threshold | yes | Run the command, proof, source audit, or artifact check named in this plan | bunx vitest run convex/orm/order-pushdown-reads.test.ts -> 8 passed; 5 of 8 RED pre-fix (git stash verified) | +| Named verification threshold | yes | Run the command, proof, source audit, or artifact check named in this plan | original RED/green proof retained; source-synced focused ORM suite -> 165 passed, 1 skipped; `bun check` -> exit 0 | | Exact per-PR task ownership | yes | Record the exact PR and dedicated plan, or the not-yet-created single-PR slice | PR #426, owned solely by this plan | | Pre-solution issue challenge verdict | yes | Record reporter claim, suggested fix, repro verdict, validity verdict, durable boundary, and hard-stop/pivot decision before implementation | valid; one constraint corrected (nullable guard is multi-spec only) | | Repro escalation ladder | yes | For bug/behavior claims, record test/source-level, automated browser/integration, Browser, and screenshot/visual-proof outcomes or N/A/blocker reasons before `not reproduced` | source-level test repro reproduced it; browser/visual N/A (no rendered output) | | Bug reproduced before fix | yes | Record failing test/repro or N/A with reason | expected 60 to be 5 on a 60-row table with limit 5 | -| Targeted behavior verification | yes | Run focused test/proof for changed behavior or record N/A | bunx vitest run convex/orm/ -> 480 passed; bun test index-utils/where-clause-compiler/indexes -> 62 passed | +| Targeted behavior verification | yes | Run focused test/proof for changed behavior or record N/A | original convex/orm and Bun unit proof retained; source-synced focused ORM suite -> 165 passed, 1 skipped | | TypeScript or typed config changed | yes | Run relevant typecheck | bun typecheck -> 5 tasks successful | | Package exports or file layout changed | no | Run the relevant package build before final verification and keep generated updates | N/A: no export or layout change; bun --cwd packages/kitcn build still run for the dist-dependent convex tests | | Package manifests, lockfile, or install graph changed | no | Run `bun install` and relevant package checks | N/A: no manifest or lockfile change | | Agent rules or skills changed | yes | Run `bun install` and verify generated skill sync | packages/kitcn/skills/kitcn/references/features/orm.md updated; bun tooling/sync-kitcn-skill.ts regenerated .agents/skills/kitcn; bunx intent validate skills + intent:stale clean | -| Workspace authority proof | yes | Run verification in the owning repo/package/app/route/tool and record cwd; do not count the wrong workspace as proof | all commands run from /Users/mikey/conductor/workspaces/kitcn/dakar (repo root), which owns both the package source and the convex integration tests | +| Workspace authority proof | yes | Run verification in the owning repo/package/app/route/tool and record cwd; do not count the wrong workspace as proof | original task proof ran in `/Users/mikey/conductor/workspaces/kitcn/dakar`; autoclosure source-sync proof ran in `/Users/zbeyens/git/better-convex`; both are repo roots owning the package source and Convex integration tests | | Browser surface changed | no | Capture Browser Use proof or record explicit waiver/blocker | N/A: server-side query planner | | Browser final proof | no | Attach screenshot or exact browser verification caveat when browser proof applies | N/A: no browser surface | | UI walkthrough | no | If UI or rendered output changed, run `.agents/skills/walkthrough/SKILL.md` after final proof and show annotated images in the final handoff; otherwise record N/A | N/A: no UI or rendered output changed | @@ -311,7 +311,7 @@ Completion Gates: | High-risk mini gate | yes | For public API/runtime/package-boundary/browser/agent-action/command-contract changes, record realistic failure mode, proof plan, and why the chosen boundary is right; otherwise N/A | see High-risk note | | Agent-native review for agent/tooling changes | no | For `.agents/**`, `.claude/**`, `.codex/**`, skills, hooks, commands, prompts, or user-action tooling, load `.agents/skills/agent-native-reviewer/SKILL.md` and close accepted/actionable findings, or record N/A | N/A: only the generated kitcn skill mirror changed, via its sync script; no .agents/.claude/.codex workflow, hook, command, or prompt touched | | Local install corruption suspected | yes | Run `bun install` once, rerun the exact failing command, or record N/A | 7 convex suites failed with Cannot find package kitcn/server; bun --cwd packages/kitcn build fixed all 7 (stale dist, not the diff) | -| Commit created | yes | For verified code-changing work, stage the entire current checkout per repo policy and create a commit; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | single commit at the tip of `issue-416-v1`, whole checkout staged | +| Commit created | yes | For verified code-changing work, stage the entire current checkout per repo policy and create a commit; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | task implementation plus the source-sync merge are committed on `fix/orm-multi-field-orderby-index-pushdown`; whole checkout staged at each checkpoint | | PR create or update | yes | For verified code-changing work, run `check`, push, create or update the PR, and sync PR body to the task-style final handoff; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | `bun check` exit 0, pushed, PR #426 created with the task-style body | | Task-style PR body verified | yes | Verify the PR body with `gh pr view --json body`; it must preserve auto-release blocks when applicable, must not include a current-PR self-link, and must use the PR #270 emoji format: `🐛 Fixes ...`, `🟢 95-100% confidence`, `Phase / 🧪 Tests / 🌐 Browser` table, and bold emoji Outcome/Caveat/Design/Verified sections | `gh pr view 426 --json body`: auto-release block preserved, no self-link, all required sections present | | PR task evidence verified | yes | Verify body plan line, plan at PR head, and exact PR ownership | plan line present, plan committed at PR head, names PR #426 | @@ -321,7 +321,7 @@ Completion Gates: | Final lint | yes | Run `bun lint:fix` or scoped equivalent | bun lint:fix then bun lint -> no fixes applied | | Output budget discipline | yes | Verify no unbounded high-volume command output was streamed, or record the accidental output and recovery | workflow output artifacted to /tmp/wf-facts.txt; no unbounded stream | | Timed checkpoint | no | If duration was requested, keep improving until elapsed, then finish the current loop cleanly; otherwise N/A | N/A: no duration requested | -| Autoreview for non-trivial implementation changes | yes | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | autoreview --mode local --engine claude: 2 conditional P0s, both investigated and closed, then re-run clean ("no accepted/actionable findings", patch is correct 0.7) | +| Autoreview for non-trivial implementation changes | yes | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | original local autoreview closed 2 conditional findings; source-synced branch autoreview against `kitcn/main` is clean with no P0/P1 (0.87) | | Goal plan complete | yes | Run `node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md` | node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md | | Public API / package boundary proof | yes | Source-audit public API, exports, and package boundary impact | index-utils.ts importers audited: only orm/* and cli/utils/schema-tables.ts (which imports getAggregateIndexes/getRankIndexes only). OrderSpec is not publicly exported | | Convex bundle/import proof | no | Audit affected function-entry static graphs or record N/A | N/A: no new cross-module imports; OrderSpec is a type-only import within orm/ | @@ -340,7 +340,7 @@ Phase / pass table: | Implementation | done | helper rewrite + 5 call sites + index scoring + docs + changeset | verification | | Verification | done | 8/8 new tests, 879 vitest, 1323 bun test, typecheck, lint, build | closeout | | Commit / PR / GitHub sync | done | `bun check` green, pushed, PR #426 opened with the task-style body | final response | -| Closeout | done | autoreview clean on re-run | final response | +| Closeout | done | source-synced branch autoreview clean; full `bun check` green | terminal GitHub receipt and merge | Findings: - Two independent blanket bails, exactly as the issue said: the helper's arity @@ -389,10 +389,11 @@ Decisions and tradeoffs: - Left `_compareByOrderSpecs` alone. Aligning it with Convex's value order would fix the pre-existing single-field null divergence but is a breaking change and needs user sign-off (recorded under Open risks). -- Left the multi-probe cursor stream ordering by `primaryOrder.direction`: that - stream never calls `.withIndex()`, so a direction resolved against another - index would be meaningless there. Its warning guard was reverted to - `hasSecondaryOrders` so it keeps firing. +- Accepted multi-probe cursor plans use the probe-union streams introduced by + #425, where every probe owns its index range and direction. Rejected unions + and scan fallback remain budgeted fallback reads. This task does not claim a + compound multi-field pushdown for a union unless the union merge order and + `resolveIndexOrderPushdown` agree. Implementation notes: - `resolveIndexOrderPushdown` now walks the spec list: eq-pinned fields are @@ -419,11 +420,15 @@ Review fixes: fields are ignored because the page is not read through an index that orders by them. - Re-ran autoreview after the fixes: clean, no accepted/actionable findings. +- Autoclosure source sync merged the current `main`, retained #425's unified + stream planner, and threaded this task's resolved pushdown direction through + the residual-filter path. Branch autoreview against that `main` found no + P0/P1 at 0.87 confidence. Error attempts: | Error / failed attempt | Count | Next different move | Resolution | |------------------------|-------|---------------------|------------| -| None yet | 0 | | | +| Source-sync conflict in `packages/kitcn/src/orm/query.ts` | 1 | Resolve from the current owners: #425's unified stream planner plus #426's `OrderSpec` and resolved direction | typecheck, focused tests, package build, branch review, and full `bun check` passed | Verification evidence: - cwd for every command below: /Users/mikey/conductor/workspaces/kitcn/dakar @@ -440,6 +445,14 @@ Verification evidence: - `bun --cwd packages/kitcn build` -> 71 files, build complete. - `bunx intent validate skills` -> 1 skill file passed; `intent:stale` -> up to date. - `autoreview --mode local --engine claude` -> clean, "patch is correct" (0.7). +- Autoclosure source-sync proof cwd: `/Users/zbeyens/git/better-convex`. +- Source-synced focused ORM suite -> 7 files passed, 165 tests passed, 1 skipped. +- Source-synced `bun typecheck` -> 5 tasks successful. +- Source-synced `bun --cwd packages/kitcn build` -> 72 files, 1622.76 kB. +- Source-synced `bun run intent:validate && bun run intent:stale` -> valid and current. +- Source-synced branch autoreview against `kitcn/main` -> no P0/P1 (0.87). +- Source-synced `bun check` -> exit 0, including all fixture comparisons and + scenario/runtime lanes. Source-listed case matrix: | Case | Source claim | Harness | Before | Expected after | Evidence | Status | @@ -455,7 +468,9 @@ Source-listed case matrix: | multi-probe bail (:6404) | separate bail | `in` union case | collect | bounded, order preserved | order asserted both pinned and unpinned primary | done | Final handoff contract: -- Commit line: single commit, pushed to `fix/orm-multi-field-orderby-index-pushdown` +- Commit line: implementation and source-sync proof commits pushed to + `fix/orm-multi-field-orderby-index-pushdown`; exact final head is recorded in + the terminal receipt - PR line: https://github.com/udecode/kitcn/pull/426 - Issue line: #416 linked from the PR body via `Fixes #416` - Confidence line: 95-100% @@ -507,7 +522,9 @@ Task-style PR body contract: of that output. Final handoff / sync: -- Commit: single commit, pushed to `fix/orm-multi-field-orderby-index-pushdown` +- Commit: implementation and source-sync proof commits pushed to + `fix/orm-multi-field-orderby-index-pushdown`; exact final head is recorded in + the terminal receipt - PR: https://github.com/udecode/kitcn/pull/426 - Issue: #416, linked from the PR body via `Fixes #416` - Browser proof: N/A: no browser surface @@ -517,12 +534,17 @@ Final handoff / sync: Timeline: - 2026-08-21T22:05:54.295Z Task goal plan created. +- 2026-08-26 Source-synced with current `main`; resolved the single query + planner conflict at the current owners. +- 2026-08-26 Re-proved the combined branch: focused ORM suite, typecheck, + package build, intent validation, branch autoreview, and full `bun check` + green. Reboot status: | Question | Answer | |----------|--------| -| Where am I? | Closeout complete | -| Where am I going? | Final response | +| Where am I? | Autoclosure GitHub closeout | +| Where am I going? | Exact-head receipt and merge | | What is the goal? | Let a 2+ field `orderBy` walk a compound index that already produces it, with byte-identical results | | What have I learned? | See Findings | | What have I done? | See Timeline | @@ -550,9 +572,9 @@ Open risks: neither widens nor narrows that; it only refuses to let a multi-field sort inherit it. Aligning the comparator with Convex's value order is the real fix and is a breaking change that needs user sign-off. -- Pre-existing: two cursor branches (`query.ts` multi-probe + maxScan, and the - scan fallback without a caller index) build a stream with no index anchor, so - even the primary sort field is not carried and no warning says so. +- Pre-existing: a rejected multi-probe plan and scan fallback without a caller + index use a budgeted unanchored stream. Accepted probe unions are index-backed + after #425; this task does not broaden their merge-order contract. - `indexOrderBonus` is a heuristic. It now prefers the index that serves more of the sort, but it still cannot outrank filter selectivity, so a query whose best filter index differs from its best order index will still post-fetch sort. From 44dc425797c679f660fde31a6275c31f11b32600 Mon Sep 17 00:00:00 2001 From: zbeyens Date: Wed, 26 Aug 2026 06:09:51 +0200 Subject: [PATCH 4/8] fix(orm): score pinned compound order indexes --- ...-orm-multi-field-orderby-index-pushdown.md | 19 +++++++++- .../src/orm/where-clause-compiler.test.ts | 36 +++++++++++++++++++ .../kitcn/src/orm/where-clause-compiler.ts | 27 +++++++++----- 3 files changed, 73 insertions(+), 9 deletions(-) diff --git a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md index e4d2037ff..bb066a2f6 100644 --- a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md +++ b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md @@ -321,7 +321,7 @@ Completion Gates: | Final lint | yes | Run `bun lint:fix` or scoped equivalent | bun lint:fix then bun lint -> no fixes applied | | Output budget discipline | yes | Verify no unbounded high-volume command output was streamed, or record the accidental output and recovery | workflow output artifacted to /tmp/wf-facts.txt; no unbounded stream | | Timed checkpoint | no | If duration was requested, keep improving until elapsed, then finish the current loop cleanly; otherwise N/A | N/A: no duration requested | -| Autoreview for non-trivial implementation changes | yes | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | original local autoreview closed 2 conditional findings; source-synced branch autoreview against `kitcn/main` is clean with no P0/P1 (0.87) | +| Autoreview for non-trivial implementation changes | yes | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | original local review closed 2 conditional findings; autoclosure exact-head review found one real P1 in pinned-order index scoring, repaired with RED/green owner tests; immutable-head rerun is recorded in the terminal receipt | | Goal plan complete | yes | Run `node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md` | node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md | | Public API / package boundary proof | yes | Source-audit public API, exports, and package boundary impact | index-utils.ts importers audited: only orm/* and cli/utils/schema-tables.ts (which imports getAggregateIndexes/getRankIndexes only). OrderSpec is not publicly exported | | Convex bundle/import proof | no | Audit affected function-entry static graphs or record N/A | N/A: no new cross-module imports; OrderSpec is a type-only import within orm/ | @@ -424,11 +424,18 @@ Review fixes: stream planner, and threaded this task's resolved pushdown direction through the residual-filter path. Branch autoreview against that `main` found no P0/P1 at 0.87 confidence. +- Final exact-head review then found one real P1: candidate-index scoring did + not absorb requested fields already fixed by the filter prefix, so it could + prefer a narrow filter index over the compound index that preserves the read + bound. The scorer now skips those fixed sort fields before matching moving + index keys. RED/green tests cover equality-pinned fields at the start and end + of the sort list, the pinned-only narrow-index case, and probe selection. Error attempts: | Error / failed attempt | Count | Next different move | Resolution | |------------------------|-------|---------------------|------------| | Source-sync conflict in `packages/kitcn/src/orm/query.ts` | 1 | Resolve from the current owners: #425's unified stream planner plus #426's `OrderSpec` and resolved direction | typecheck, focused tests, package build, branch review, and full `bun check` passed | +| Exact-head autoreview P1 in candidate-index scoring | 1 | Add the missing narrow-vs-compound regression before changing the scorer | RED selected `by_type`; repair selects `by_type_likes`, with pinned-only and probe cases green | Verification evidence: - cwd for every command below: /Users/mikey/conductor/workspaces/kitcn/dakar @@ -453,6 +460,14 @@ Verification evidence: - Source-synced branch autoreview against `kitcn/main` -> no P0/P1 (0.87). - Source-synced `bun check` -> exit 0, including all fixture comparisons and scenario/runtime lanes. +- P1 RED: pinned `[type, numLikes]` sort selected `by_type`, not + `by_type_likes`. +- P1 focused repair: 52 Bun owner tests and 66 Vitest integration tests passed, + with 1 integration test skipped. +- P1 repair `bun typecheck` -> 5 tasks successful; `bun lint:fix` -> clean; + package build -> 72 files, 1623.03 kB; intent validation -> current. +- P1 repair `bun check` -> exit 0: 1,400 Bun tests, 958 Vitest tests, CLI, + Concave, all fixture comparisons, and bare/Expo/Next/Start/Vite runtime lanes. Source-listed case matrix: | Case | Source claim | Harness | Before | Expected after | Evidence | Status | @@ -539,6 +554,8 @@ Timeline: - 2026-08-26 Re-proved the combined branch: focused ORM suite, typecheck, package build, intent validation, branch autoreview, and full `bun check` green. +- 2026-08-26 Reproduced and repaired the exact-head review P1 in pinned-order + index scoring; focused and full repository gates passed. Reboot status: | Question | Answer | diff --git a/packages/kitcn/src/orm/where-clause-compiler.test.ts b/packages/kitcn/src/orm/where-clause-compiler.test.ts index d94dbf014..b9546791c 100644 --- a/packages/kitcn/src/orm/where-clause-compiler.test.ts +++ b/packages/kitcn/src/orm/where-clause-compiler.test.ts @@ -323,6 +323,34 @@ describe('WhereClauseCompiler advanced index planning', () => { expect(withoutOrder.selectedIndex?.indexName).toBe('by_org'); }); + test('absorbs an equality-pinned order field when scoring indexes', () => { + const compiler = new WhereClauseCompiler('posts', [ + { indexName: 'by_type', indexFields: ['type'] }, + { + indexName: 'by_type_likes', + indexFields: ['type', 'numLikes'], + }, + ]); + + const result = compiler.compile(eq(fieldRef('type') as any, 'a'), { + orderFields: ['type', 'numLikes'], + }) as any; + + expect(result.selectedIndex?.indexName).toBe('by_type_likes'); + + const pinnedLater = compiler.compile( + eq(fieldRef('type') as any, 'a'), + { orderFields: ['numLikes', 'type'] } + ) as any; + expect(pinnedLater.selectedIndex?.indexName).toBe('by_type_likes'); + + const pinnedOnly = compiler.compile( + eq(fieldRef('type') as any, 'a'), + { orderFields: ['type'] } + ) as any; + expect(pinnedOnly.selectedIndex?.indexName).toBe('by_type'); + }); + test('keeps the narrow index when no candidate supplies the order', () => { const compiler = new WhereClauseCompiler('posts', [ { indexName: 'by_org', indexFields: ['orgId'] }, @@ -353,6 +381,14 @@ describe('WhereClauseCompiler advanced index planning', () => { expect(result.strategy).toBe('multiProbe'); expect(result.selectedIndex?.indexName).toBe('by_status_age'); + + const withProbeFieldInOrder = compiler.compile( + inArray(fieldRef('status') as any, ['active', 'pending']), + { orderFields: ['status', 'age'] } + ) as any; + expect(withProbeFieldInOrder.selectedIndex?.indexName).toBe( + 'by_status_age' + ); }); test('lands every unconsumed binary in postFilters exactly once', () => { diff --git a/packages/kitcn/src/orm/where-clause-compiler.ts b/packages/kitcn/src/orm/where-clause-compiler.ts index ca1630acd..09c0ad628 100644 --- a/packages/kitcn/src/orm/where-clause-compiler.ts +++ b/packages/kitcn/src/orm/where-clause-compiler.ts @@ -958,22 +958,33 @@ export class WhereClauseCompiler { } /** - * How many leading sort fields this index would produce for free, counting - * from its first unpinned key. Convex walks an index in full key order, so - * the run has to be contiguous and start at `pinnedLength`. + * How many moving sort fields this index would produce for free. Sort fields + * inside the pinned prefix are constant and can appear anywhere in the + * request; every other field must match the remaining index keys in order. */ private servedOrderFieldCount( indexFields: string[], pinnedLength: number ): number { + const pinnedFields = new Set(indexFields.slice(0, pinnedLength)); + let indexOffset = pinnedLength; + let orderOffset = 0; let served = 0; - while ( - served < this.orderFields.length && - pinnedLength + served < indexFields.length && - indexFields[pinnedLength + served] === this.orderFields[served] - ) { + + while (orderOffset < this.orderFields.length) { + const orderField = this.orderFields[orderOffset]; + if (pinnedFields.has(orderField)) { + orderOffset += 1; + continue; + } + if (indexFields[indexOffset] !== orderField) { + break; + } served += 1; + indexOffset += 1; + orderOffset += 1; } + return served; } From 32d013744dd9f85cec4f9c7ad067a7ef7049c1ef Mon Sep 17 00:00:00 2001 From: zbeyens Date: Wed, 26 Aug 2026 06:23:41 +0200 Subject: [PATCH 5/8] fix(orm): align post-fetch value ordering --- .../skills/kitcn/references/features/orm.md | 2 + .changeset/nine-planets-marry.md | 3 + convex/orm/order-pushdown-reads.test.ts | 61 +++++++++++++++++++ ...-orm-multi-field-orderby-index-pushdown.md | 50 ++++++++++----- .../skills/kitcn/references/features/orm.md | 2 + packages/kitcn/src/orm/query.ts | 8 +-- www/content/docs/orm/queries/index.mdx | 1 + 7 files changed, 106 insertions(+), 21 deletions(-) diff --git a/.agents/skills/kitcn/references/features/orm.md b/.agents/skills/kitcn/references/features/orm.md index 124422778..39ebc497a 100644 --- a/.agents/skills/kitcn/references/features/orm.md +++ b/.agents/skills/kitcn/references/features/orm.md @@ -266,6 +266,8 @@ orders by creation time, so `with: { posts: { limit: 5, orderBy: { createdAt: when the index carries all of them: `index('by_user_rank').on(t.userId, t.rank)` serves `orderBy: { rank: 'asc', createdAt: 'asc' }`, since the FK pins `userId`, `rank` is the next key and `createdAt` is Convex's implicit trailing one. +Non-null values use Convex value ordering on both index-backed and post-fetch +paths, including UTF-8 strings, signed zero, and NaN. Sorting by any other column — or by one that can be missing or `null` — reads the parent's whole child partition and sorts in memory; put the sort columns in the relation index, in sort order, to stay bounded. diff --git a/.changeset/nine-planets-marry.md b/.changeset/nine-planets-marry.md index 4881f8a6b..fb0a5a849 100644 --- a/.changeset/nine-planets-marry.md +++ b/.changeset/nine-planets-marry.md @@ -21,3 +21,6 @@ - Sorts that mix directions, skip an index key, or run over a column that can be missing or null keep using the post-fetch sort, so row order and null placement are unchanged. +- Use Convex value ordering for non-null post-fetch values, including UTF-8 + strings, signed zero, and NaN, so an index-backed top-k and its post-fetch + fallback select the same rows. diff --git a/convex/orm/order-pushdown-reads.test.ts b/convex/orm/order-pushdown-reads.test.ts index 50b1b80d1..88f3eea95 100644 --- a/convex/orm/order-pushdown-reads.test.ts +++ b/convex/orm/order-pushdown-reads.test.ts @@ -135,6 +135,67 @@ test('two-field orderBy returns the same page a post-fetch sort would', async () }); }); +test('post-fetch ordering follows Convex UTF-8 string order', async () => { + const t = convexTest(schema); + await t.run(async (baseCtx) => { + for (const type of ['😀', '\uE000']) { + await baseCtx.db.insert('posts', { + text: type, + numLikes: 0, + type, + }); + } + }); + + await t.run(async (baseCtx) => { + const ctx = await runCtx(baseCtx); + const postFetchOrdered = await ctx.orm.query.posts.findMany({ + orderBy: (posts: any, { asc, desc }: any) => [ + asc(posts.type), + desc(posts.numLikes), + ], + limit: 2, + }); + + expect(postFetchOrdered.map((row: any) => row.type)).toEqual([ + '\uE000', + '😀', + ]); + }); +}); + +test('post-fetch ordering follows Convex float edge order', async () => { + const t = convexTest(schema); + await t.run(async (baseCtx) => { + for (const [text, numLikes] of [ + ['nan', Number.NaN], + ['positive-zero', 0], + ['negative-zero', -0], + ['one', 1], + ] as const) { + await baseCtx.db.insert('posts', { text, numLikes, type: 'a' }); + } + }); + + await t.run(async (baseCtx) => { + const ctx = await runCtx(baseCtx); + const rows = await ctx.orm.query.posts.findMany({ + orderBy: (posts: any, { asc, desc }: any) => [ + desc(posts.type), + asc(posts.numLikes), + ], + limit: 4, + }); + + expect(rows.map((row: any) => row.text)).toEqual([ + 'negative-zero', + 'positive-zero', + 'one', + 'nan', + ]); + }); +}); + test('an `in` union keeps the requested order over the whole union', async () => { // Each probe pins `type` to a different value, so the field is constant // inside a probe but not across the union. The per-probe truncation is still diff --git a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md index bb066a2f6..dc24fc03d 100644 --- a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md +++ b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md @@ -290,12 +290,12 @@ Work Checklist: Completion Gates: | Gate | Applies | Required action | Evidence | |------|---------|-----------------|----------| -| Named verification threshold | yes | Run the command, proof, source audit, or artifact check named in this plan | original RED/green proof retained; source-synced focused ORM suite -> 165 passed, 1 skipped; `bun check` -> exit 0 | +| Named verification threshold | yes | Run the command, proof, source audit, or artifact check named in this plan | original RED/green proof retained; final focused ORM suite -> 167 passed, 1 skipped; `bun check` -> exit 0 | | Exact per-PR task ownership | yes | Record the exact PR and dedicated plan, or the not-yet-created single-PR slice | PR #426, owned solely by this plan | | Pre-solution issue challenge verdict | yes | Record reporter claim, suggested fix, repro verdict, validity verdict, durable boundary, and hard-stop/pivot decision before implementation | valid; one constraint corrected (nullable guard is multi-spec only) | | Repro escalation ladder | yes | For bug/behavior claims, record test/source-level, automated browser/integration, Browser, and screenshot/visual-proof outcomes or N/A/blocker reasons before `not reproduced` | source-level test repro reproduced it; browser/visual N/A (no rendered output) | | Bug reproduced before fix | yes | Record failing test/repro or N/A with reason | expected 60 to be 5 on a 60-row table with limit 5 | -| Targeted behavior verification | yes | Run focused test/proof for changed behavior or record N/A | original convex/orm and Bun unit proof retained; source-synced focused ORM suite -> 165 passed, 1 skipped | +| Targeted behavior verification | yes | Run focused test/proof for changed behavior or record N/A | original convex/orm and Bun unit proof retained; final focused ORM suite -> 167 passed, 1 skipped; 52 scorer/pushdown owner tests passed | | TypeScript or typed config changed | yes | Run relevant typecheck | bun typecheck -> 5 tasks successful | | Package exports or file layout changed | no | Run the relevant package build before final verification and keep generated updates | N/A: no export or layout change; bun --cwd packages/kitcn build still run for the dist-dependent convex tests | | Package manifests, lockfile, or install graph changed | no | Run `bun install` and relevant package checks | N/A: no manifest or lockfile change | @@ -321,7 +321,7 @@ Completion Gates: | Final lint | yes | Run `bun lint:fix` or scoped equivalent | bun lint:fix then bun lint -> no fixes applied | | Output budget discipline | yes | Verify no unbounded high-volume command output was streamed, or record the accidental output and recovery | workflow output artifacted to /tmp/wf-facts.txt; no unbounded stream | | Timed checkpoint | no | If duration was requested, keep improving until elapsed, then finish the current loop cleanly; otherwise N/A | N/A: no duration requested | -| Autoreview for non-trivial implementation changes | yes | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | original local review closed 2 conditional findings; autoclosure exact-head review found one real P1 in pinned-order index scoring, repaired with RED/green owner tests; immutable-head rerun is recorded in the terminal receipt | +| Autoreview for non-trivial implementation changes | yes | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | original local review closed 2 conditional findings; autoclosure exact-head reviews found 2 real P1s in pinned-order scoring and non-null value ordering, both repaired with RED/green proof; immutable-head rerun is recorded in the terminal receipt | | Goal plan complete | yes | Run `node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md` | node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md | | Public API / package boundary proof | yes | Source-audit public API, exports, and package boundary impact | index-utils.ts importers audited: only orm/* and cli/utils/schema-tables.ts (which imports getAggregateIndexes/getRankIndexes only). OrderSpec is not publicly exported | | Convex bundle/import proof | no | Audit affected function-entry static graphs or record N/A | N/A: no new cross-module imports; OrderSpec is a type-only import within orm/ | @@ -340,7 +340,7 @@ Phase / pass table: | Implementation | done | helper rewrite + 5 call sites + index scoring + docs + changeset | verification | | Verification | done | 8/8 new tests, 879 vitest, 1323 bun test, typecheck, lint, build | closeout | | Commit / PR / GitHub sync | done | `bun check` green, pushed, PR #426 opened with the task-style body | final response | -| Closeout | done | source-synced branch autoreview clean; full `bun check` green | terminal GitHub receipt and merge | +| Closeout | done | both exact-head P1s repaired; final full `bun check` green | immutable-head review, terminal GitHub receipt, and merge | Findings: - Two independent blanket bails, exactly as the issue said: the helper's arity @@ -368,10 +368,10 @@ Findings: - Index scoring only ever looked at `orderFields[0]`, so without widening it the planner would keep picking a shorter index and the fix would rarely fire on `where` + multi-field sorts. -- `convex-test` vendors its own fork of Convex's `compareValues`. Ranks for - `undefined`/`null` match, so the null-placement evidence transfers; string - (supplementary-plane), `-0`, and NaN ordering do not. The regression fixtures - deliberately use ASCII strings and small positive integers. +- `convex-test` vendors its own index comparator, and its supplementary-plane + string order differs from production Convex. Edge-value executable proof + therefore targets the ORM post-fetch owner, while the production index order + is sourced from Convex's exported `compareValues` contract. Decisions and tradeoffs: - Nullability travels on `OrderSpec` rather than as a new helper parameter, so @@ -386,9 +386,10 @@ Decisions and tradeoffs: - Kept `needsPostFetchSortForPrimary` separate from the whole-sort answer. Fusing them would make an index that anchors the primary but not the tie-break report itself as unindexed, which throws under strict cursor pagination. -- Left `_compareByOrderSpecs` alone. Aligning it with Convex's value order would - fix the pre-existing single-field null divergence but is a breaking change and - needs user sign-off (recorded under Open risks). +- `_compareByOrderSpecs` now delegates every non-null comparison to Convex's + exported `compareValues`, so UTF-8 strings, signed zero, NaN, and mixed Convex + value types agree with index order. The existing nulls-last policy remains; + nullable multi-field sorts therefore still decline pushdown. - Accepted multi-probe cursor plans use the probe-union streams introduced by #425, where every probe owns its index range and direction. Rejected unions and scan fallback remain budgeted fallback reads. This task does not claim a @@ -430,12 +431,18 @@ Review fixes: bound. The scorer now skips those fixed sort fields before matching moving index keys. RED/green tests cover equality-pinned fields at the start and end of the sort list, the pinned-only narrow-index case, and probe selection. +- The next exact-head review found a second real P1: JavaScript relational + comparison differs from Convex index order for legal UTF-8 and Float64 edge + values. The post-fetch comparator now uses Convex `compareValues` for non-null + values. RED/green tests cover supplementary Unicode, signed zero, and NaN; + null placement remains unchanged and guarded from multi-field pushdown. Error attempts: | Error / failed attempt | Count | Next different move | Resolution | |------------------------|-------|---------------------|------------| | Source-sync conflict in `packages/kitcn/src/orm/query.ts` | 1 | Resolve from the current owners: #425's unified stream planner plus #426's `OrderSpec` and resolved direction | typecheck, focused tests, package build, branch review, and full `bun check` passed | | Exact-head autoreview P1 in candidate-index scoring | 1 | Add the missing narrow-vs-compound regression before changing the scorer | RED selected `by_type`; repair selects `by_type_likes`, with pinned-only and probe cases green | +| Exact-head autoreview P1 in value-order equivalence | 1 | Reproduce the UTF-8 mismatch, then use Convex's public comparator at the post-fetch owner | RED put emoji before U+E000; repair follows Convex UTF-8, signed-zero, and NaN order | Verification evidence: - cwd for every command below: /Users/mikey/conductor/workspaces/kitcn/dakar @@ -468,6 +475,14 @@ Verification evidence: package build -> 72 files, 1623.03 kB; intent validation -> current. - P1 repair `bun check` -> exit 0: 1,400 Bun tests, 958 Vitest tests, CLI, Concave, all fixture comparisons, and bare/Expo/Next/Start/Vite runtime lanes. +- Value-order RED: post-fetch sort put emoji before U+E000, opposite Convex + UTF-8 order. +- Value-order repair: final focused ORM suite -> 167 passed, 1 skipped; 52 Bun + owner tests passed. Supplementary Unicode, signed zero, and NaN are covered. +- Final `bun typecheck`, lint, package build (72 files, 1623.03 kB), docs/skill + sync, and intent validation passed. +- Final `bun check` -> exit 0: 1,400 Bun tests, 960 Vitest tests, CLI, Concave, + all fixture comparisons, and bare/Expo/Next/Start/Vite runtime lanes. Source-listed case matrix: | Case | Source claim | Harness | Before | Expected after | Evidence | Status | @@ -509,9 +524,10 @@ Final handoff contract: - Why not quick patch: narrowing only the helper is a no-op — three call sites truncated the spec list and two planner booleans re-decided it on arity. Fixing one without the others changes nothing. - - Why not broader change: aligning `_compareByOrderSpecs` with Convex's value - order would remove the nullable carve-out entirely, but it silently changes - shipped single-field results and is a breaking change needing sign-off. + - Why not broader change: changing null placement would remove the nullable + carve-out entirely, but it silently changes shipped single-field results + and remains a breaking change needing sign-off. Non-null values use Convex + ordering on every path. - Verified: see Verification evidence - PR body verified: `gh pr view 426 --json body` — auto-release block preserved, emoji task-style sections present, no self-link @@ -556,6 +572,8 @@ Timeline: green. - 2026-08-26 Reproduced and repaired the exact-head review P1 in pinned-order index scoring; focused and full repository gates passed. +- 2026-08-26 Reproduced and repaired the exact-head review P1 in non-null value + ordering; UTF-8/Float64 edge proof and the full repository gate passed. Reboot status: | Question | Answer | @@ -587,8 +605,8 @@ Open risks: A single-field `orderBy` on a nullable column therefore already returns a different null placement depending on whether an index exists. This change neither widens nor narrows that; it only refuses to let a multi-field sort - inherit it. Aligning the comparator with Convex's value order is the real fix - and is a breaking change that needs user sign-off. + inherit it. Aligning null placement with Convex is the remaining fix and is a + breaking change that needs user sign-off. - Pre-existing: a rejected multi-probe plan and scan fallback without a caller index use a budgeted unanchored stream. Accepted probe unions are index-backed after #425; this task does not broaden their merge-order contract. diff --git a/packages/kitcn/skills/kitcn/references/features/orm.md b/packages/kitcn/skills/kitcn/references/features/orm.md index 124422778..39ebc497a 100644 --- a/packages/kitcn/skills/kitcn/references/features/orm.md +++ b/packages/kitcn/skills/kitcn/references/features/orm.md @@ -266,6 +266,8 @@ orders by creation time, so `with: { posts: { limit: 5, orderBy: { createdAt: when the index carries all of them: `index('by_user_rank').on(t.userId, t.rank)` serves `orderBy: { rank: 'asc', createdAt: 'asc' }`, since the FK pins `userId`, `rank` is the next key and `createdAt` is Convex's implicit trailing one. +Non-null values use Convex value ordering on both index-backed and post-fetch +paths, including UTF-8 strings, signed zero, and NaN. Sorting by any other column — or by one that can be missing or `null` — reads the parent's whole child partition and sorts in memory; put the sort columns in the relation index, in sort order, to stay bounded. diff --git a/packages/kitcn/src/orm/query.ts b/packages/kitcn/src/orm/query.ts index bbfbd3a41..9b9159696 100644 --- a/packages/kitcn/src/orm/query.ts +++ b/packages/kitcn/src/orm/query.ts @@ -1068,11 +1068,9 @@ export class GelRelationalQuery< return -1; } - if (aVal < bVal) { - return order.direction === 'asc' ? -1 : 1; - } - if (aVal > bVal) { - return order.direction === 'asc' ? 1 : -1; + const comparison = compareValues(aVal, bVal); + if (comparison !== 0) { + return order.direction === 'asc' ? comparison : -comparison; } } return 0; diff --git a/www/content/docs/orm/queries/index.mdx b/www/content/docs/orm/queries/index.mdx index cfe139300..20e90b433 100644 --- a/www/content/docs/orm/queries/index.mdx +++ b/www/content/docs/orm/queries/index.mdx @@ -178,6 +178,7 @@ const posts2 = await db.query.posts.findMany({ **Notes:** - An index that walks in the requested order serves the sort directly, so `limit` reads only the rows it returns. That needs the sort fields to be the index's leading keys, in order, all pointing the same way — `index('by_type_likes').on(t.type, t.numLikes)` serves `orderBy: { type: 'asc', numLikes: 'asc' }`. Fields pinned by an equality in `where` are constant across the scan, so they can appear anywhere in the sort and in either direction - `createdAt` is servable as the last sort field once every other index key is pinned or consumed, because Convex appends it as the implicit trailing key +- Non-null values use Convex value ordering on both index-backed and post-fetch paths, including UTF-8 string order and Float64 edge values such as signed zero and NaN - Any other sort — mixed directions, a field the index does not sort by next, or a column that can be missing or `null` — is applied post-fetch. The ORM then sorts before applying `offset`/`limit`, which requires reading the full filtered candidate set first - `findMany({ cursor, limit })` orders pages by every `orderBy` field the index serves. When the index cannot serve them all, only the first field orders the cursor and the rest are dropped, which the ORM warns about From 0c3c035dea83d894ff5b9b9617aa12523591e4d8 Mon Sep 17 00:00:00 2001 From: zbeyens Date: Wed, 26 Aug 2026 06:36:49 +0200 Subject: [PATCH 6/8] fix(orm): preserve pinned sort tie order --- .../skills/kitcn/references/features/orm.md | 4 ++ .changeset/nine-planets-marry.md | 3 + convex/orm/order-pushdown-reads.test.ts | 38 ++++++++++++- ...-orm-multi-field-orderby-index-pushdown.md | 56 ++++++++++++------- .../skills/kitcn/references/features/orm.md | 4 ++ packages/kitcn/src/orm/index-utils.test.ts | 23 ++++++-- packages/kitcn/src/orm/index-utils.ts | 16 ++++++ www/content/docs/orm/queries/index.mdx | 1 + 8 files changed, 119 insertions(+), 26 deletions(-) diff --git a/.agents/skills/kitcn/references/features/orm.md b/.agents/skills/kitcn/references/features/orm.md index 39ebc497a..0392e9325 100644 --- a/.agents/skills/kitcn/references/features/orm.md +++ b/.agents/skills/kitcn/references/features/orm.md @@ -268,6 +268,10 @@ serves `orderBy: { rank: 'asc', createdAt: 'asc' }`, since the FK pins `userId`, `rank` is the next key and `createdAt` is Convex's implicit trailing one. Non-null values use Convex value ordering on both index-backed and post-fetch paths, including UTF-8 strings, signed zero, and NaN. +When the first requested field is equality-pinned but points opposite to the +moving fields, include `createdAt` in the moving direction as the final sort +field. Without that explicit tie-break, the ORM post-fetch sorts to preserve +which tied rows survive `limit`. Sorting by any other column — or by one that can be missing or `null` — reads the parent's whole child partition and sorts in memory; put the sort columns in the relation index, in sort order, to stay bounded. diff --git a/.changeset/nine-planets-marry.md b/.changeset/nine-planets-marry.md index fb0a5a849..9440b39fb 100644 --- a/.changeset/nine-planets-marry.md +++ b/.changeset/nine-planets-marry.md @@ -24,3 +24,6 @@ - Use Convex value ordering for non-null post-fetch values, including UTF-8 strings, signed zero, and NaN, so an index-backed top-k and its post-fetch fallback select the same rows. +- Preserve the existing implicit creation-time tie order when an + equality-pinned leading sort field points opposite to the moving fields. + Add `createdAt` in the moving direction to make that sort index-bounded. diff --git a/convex/orm/order-pushdown-reads.test.ts b/convex/orm/order-pushdown-reads.test.ts index 88f3eea95..e0ea6e54f 100644 --- a/convex/orm/order-pushdown-reads.test.ts +++ b/convex/orm/order-pushdown-reads.test.ts @@ -97,6 +97,7 @@ test('an eq-pinned leading sort field does not cost the bound', async () => { orderBy: (posts: any, { asc, desc }: any) => [ asc(posts.type), desc(posts.numLikes), + desc(posts.createdAt), ], limit: 3, }) @@ -109,6 +110,36 @@ test('an eq-pinned leading sort field does not cost the bound', async () => { ); }); +test('a pinned opposite-direction sort preserves the old implicit tie order', async () => { + const t = convexTest(schema); + await t.run(async (baseCtx) => { + await baseCtx.db.insert('posts', { + text: 'older', + numLikes: 1, + type: 'a', + }); + await baseCtx.db.insert('posts', { + text: 'newer', + numLikes: 1, + type: 'a', + }); + }); + + await t.run(async (baseCtx) => { + const ctx = await runCtx(baseCtx); + const rows = await ctx.orm.query.posts.findMany({ + where: { type: 'a' }, + orderBy: (posts: any, { asc, desc }: any) => [ + asc(posts.type), + desc(posts.numLikes), + ], + limit: 1, + }); + + expect(rows.map((row: any) => row.text)).toEqual(['older']); + }); +}); + test('two-field orderBy returns the same page a post-fetch sort would', async () => { const t = convexTest(schema); await t.run((baseCtx) => seedPosts(baseCtx, 60)); @@ -255,9 +286,9 @@ test('an `in` union keeps the requested order over the whole union', async () => }); }); -test('cursor pages honour a sort whose leading field is eq-pinned the other way', async () => { - // `type` is pinned by the where and `numLikes` runs the other way, so the - // direction the scan needs is the second spec's, not the first's. +test('cursor pages honour an opposite-direction pinned sort with an explicit tie-break', async () => { + // `type` is pinned by the where and `numLikes` runs the other way. Explicit + // creation order makes the scan direction safe even when values tie. const t = convexTest(schema); await t.run((baseCtx) => seedPosts(baseCtx, 40)); await t.run(async (baseCtx) => { @@ -267,6 +298,7 @@ test('cursor pages honour a sort whose leading field is eq-pinned the other way' orderBy: (posts: any, { asc, desc }: any) => [ asc(posts.type), desc(posts.numLikes), + desc(posts.createdAt), ], cursor: null, limit: 3, diff --git a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md index dc24fc03d..6d250f29f 100644 --- a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md +++ b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md @@ -43,9 +43,11 @@ Timed checkpoint: Completion threshold: - `resolveIndexOrderPushdown` answers multi-spec sorts correctly (eq-pinned - absorbed at any position/direction; remaining specs contiguous from + fields absorbed without consuming an index key; remaining specs contiguous from `indexFields[eqCount]`; `_creationTime` legal only as the final spec after - every declared field is consumed; all unpinned specs share one direction), + every declared field is consumed; all unpinned specs share one direction; + opposite-direction pinned-leading sorts preserve their implicit tie order + unless `_creationTime` makes it explicit), every planner call site passes the full spec list, and the read bound is proven by a two-table-size read-count test on both the top-level query and the relation loader, with result rows unchanged. @@ -90,8 +92,8 @@ Boundaries: - Allowed edit scope: `packages/kitcn/src/orm/{index-utils,query,where-clause-compiler}.ts`, their unit tests, `convex/orm/order-pushdown-reads.test.ts`, `.changeset/`. - Browser surface: N/A: server-side query planner, no rendered output. -- GitHub issue sync: N/A: user preference forbids PR creation; no PR to - reference, so no issue comment is posted. +- GitHub issue sync: `Fixes #416` in PR #426 owns the issue linkage; no separate + issue comment is needed. - Non-goals: changing `_compareByOrderSpecs` null placement to match Convex's value order (a breaking change that needs user sign-off); the two cursor branches that silently drop `orderIndexName`; `_compareGroupByValues`. @@ -290,12 +292,12 @@ Work Checklist: Completion Gates: | Gate | Applies | Required action | Evidence | |------|---------|-----------------|----------| -| Named verification threshold | yes | Run the command, proof, source audit, or artifact check named in this plan | original RED/green proof retained; final focused ORM suite -> 167 passed, 1 skipped; `bun check` -> exit 0 | +| Named verification threshold | yes | Run the command, proof, source audit, or artifact check named in this plan | original RED/green proof retained; final focused ORM suite -> 168 passed, 1 skipped; `bun check` -> exit 0 | | Exact per-PR task ownership | yes | Record the exact PR and dedicated plan, or the not-yet-created single-PR slice | PR #426, owned solely by this plan | | Pre-solution issue challenge verdict | yes | Record reporter claim, suggested fix, repro verdict, validity verdict, durable boundary, and hard-stop/pivot decision before implementation | valid; one constraint corrected (nullable guard is multi-spec only) | | Repro escalation ladder | yes | For bug/behavior claims, record test/source-level, automated browser/integration, Browser, and screenshot/visual-proof outcomes or N/A/blocker reasons before `not reproduced` | source-level test repro reproduced it; browser/visual N/A (no rendered output) | | Bug reproduced before fix | yes | Record failing test/repro or N/A with reason | expected 60 to be 5 on a 60-row table with limit 5 | -| Targeted behavior verification | yes | Run focused test/proof for changed behavior or record N/A | original convex/orm and Bun unit proof retained; final focused ORM suite -> 167 passed, 1 skipped; 52 scorer/pushdown owner tests passed | +| Targeted behavior verification | yes | Run focused test/proof for changed behavior or record N/A | original convex/orm and Bun unit proof retained; final focused ORM suite -> 168 passed, 1 skipped; 52 scorer/pushdown owner tests passed | | TypeScript or typed config changed | yes | Run relevant typecheck | bun typecheck -> 5 tasks successful | | Package exports or file layout changed | no | Run the relevant package build before final verification and keep generated updates | N/A: no export or layout change; bun --cwd packages/kitcn build still run for the dist-dependent convex tests | | Package manifests, lockfile, or install graph changed | no | Run `bun install` and relevant package checks | N/A: no manifest or lockfile change | @@ -321,7 +323,7 @@ Completion Gates: | Final lint | yes | Run `bun lint:fix` or scoped equivalent | bun lint:fix then bun lint -> no fixes applied | | Output budget discipline | yes | Verify no unbounded high-volume command output was streamed, or record the accidental output and recovery | workflow output artifacted to /tmp/wf-facts.txt; no unbounded stream | | Timed checkpoint | no | If duration was requested, keep improving until elapsed, then finish the current loop cleanly; otherwise N/A | N/A: no duration requested | -| Autoreview for non-trivial implementation changes | yes | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | original local review closed 2 conditional findings; autoclosure exact-head reviews found 2 real P1s in pinned-order scoring and non-null value ordering, both repaired with RED/green proof; immutable-head rerun is recorded in the terminal receipt | +| Autoreview for non-trivial implementation changes | yes | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | original local review closed 2 conditional findings; autoclosure exact-head reviews found 3 real P1s in pinned-order scoring, non-null value ordering, and implicit tie direction, all repaired with RED/green proof; immutable-head rerun is recorded in the terminal receipt | | Goal plan complete | yes | Run `node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md` | node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md | | Public API / package boundary proof | yes | Source-audit public API, exports, and package boundary impact | index-utils.ts importers audited: only orm/* and cli/utils/schema-tables.ts (which imports getAggregateIndexes/getRankIndexes only). OrderSpec is not publicly exported | | Convex bundle/import proof | no | Audit affected function-entry static graphs or record N/A | N/A: no new cross-module imports; OrderSpec is a type-only import within orm/ | @@ -340,7 +342,7 @@ Phase / pass table: | Implementation | done | helper rewrite + 5 call sites + index scoring + docs + changeset | verification | | Verification | done | 8/8 new tests, 879 vitest, 1323 bun test, typecheck, lint, build | closeout | | Commit / PR / GitHub sync | done | `bun check` green, pushed, PR #426 opened with the task-style body | final response | -| Closeout | done | both exact-head P1s repaired; final full `bun check` green | immutable-head review, terminal GitHub receipt, and merge | +| Closeout | done | all three exact-head P1s repaired; final full `bun check` green | immutable-head review, terminal GitHub receipt, and merge | Findings: - Two independent blanket bails, exactly as the issue said: the helper's arity @@ -356,10 +358,10 @@ Findings: issue's "skip pushdown when any sort field is optional" would have regressed shipped behavior. The guard belongs to multi-spec sorts only. - The resolved pushdown direction can differ from `primaryOrder.direction` once - an eq-pinned field leads the sort. Three sites consumed `primaryOrder.direction` - as if it were the scan direction. One of them (cursor pagination with a - divergent sort) was already returning ascending pages for a descending - request on `main`; this change fixes it. + an eq-pinned field leads the sort, but reversing the scan also reverses the + implicit creation-time tie order. The differing direction is therefore only + pushed when `_creationTime` is explicit; otherwise post-fetch sorting keeps + the previous top-k identity. - The multi-probe union sort (`query.ts:6621`) is unconditional, not gated on `usePostFetchSort`, so relaxing the probe bound cannot reorder a union. - A predicate `where` produces no compiled index (`query.ts:7268-7271`), so @@ -398,10 +400,11 @@ Decisions and tradeoffs: Implementation notes: - `resolveIndexOrderPushdown` now walks the spec list: eq-pinned fields are - absorbed at any position/direction without consuming an index key, the rest - must be contiguous from `indexFields[eqCount]` in one shared direction, and - `_creationTime` is legal only as the final spec once every declared key is - consumed. `indexFields: []` models the default `by_creation_time` index. + absorbed without consuming an index key, the rest must be contiguous from + `indexFields[eqCount]` in one shared direction, and `_creationTime` is legal + only as the final spec once every declared key is consumed. A pinned-leading + opposite direction declines unless that creation-time tie-break is explicit. + `indexFields: []` models the default `by_creation_time` index. - `usePostFetchSort` is now `postFetchOrders.length > 0 && orderPushdownDirection === null`. Whole-served implies primary-served, so every case the old `needsPostFetchSortForPrimary || hasSecondaryOrders` sorted post-fetch still does. @@ -436,6 +439,11 @@ Review fixes: values. The post-fetch comparator now uses Convex `compareValues` for non-null values. RED/green tests cover supplementary Unicode, signed zero, and NaN; null placement remains unchanged and guarded from multi-field pushdown. +- The third exact-head review found that reversing an index for a pinned-leading + opposite-direction sort also reverses the implicit creation-time tie-break. + The resolver now declines that pushdown unless `_creationTime` is explicit. + RED/green proof retains the older tied row without the explicit tie-break and + keeps both non-cursor and cursor reads bounded when it is explicit. Error attempts: | Error / failed attempt | Count | Next different move | Resolution | @@ -443,6 +451,7 @@ Error attempts: | Source-sync conflict in `packages/kitcn/src/orm/query.ts` | 1 | Resolve from the current owners: #425's unified stream planner plus #426's `OrderSpec` and resolved direction | typecheck, focused tests, package build, branch review, and full `bun check` passed | | Exact-head autoreview P1 in candidate-index scoring | 1 | Add the missing narrow-vs-compound regression before changing the scorer | RED selected `by_type`; repair selects `by_type_likes`, with pinned-only and probe cases green | | Exact-head autoreview P1 in value-order equivalence | 1 | Reproduce the UTF-8 mismatch, then use Convex's public comparator at the post-fetch owner | RED put emoji before U+E000; repair follows Convex UTF-8, signed-zero, and NaN order | +| Exact-head autoreview P1 in implicit tie direction | 1 | Add two tied rows around a limit boundary before narrowing servability | RED returned the newer tied row; repair retains the older row unless creation direction is explicit | Verification evidence: - cwd for every command below: /Users/mikey/conductor/workspaces/kitcn/dakar @@ -483,6 +492,12 @@ Verification evidence: sync, and intent validation passed. - Final `bun check` -> exit 0: 1,400 Bun tests, 960 Vitest tests, CLI, Concave, all fixture comparisons, and bare/Expo/Next/Start/Vite runtime lanes. +- Tie-direction RED: pinned-leading ASC plus moving DESC returned the newer of + two tied rows at `limit: 1`; the previous post-fetch path returned the older. +- Tie-direction repair: 52 owner tests and 168 focused integration tests passed, + with 1 integration test skipped. +- Tie-direction final `bun check` -> exit 0, including unit, typecheck, lint, + package build (72 files, 1623.20 kB), fixture parity, and all runtime lanes. Source-listed case matrix: | Case | Source claim | Harness | Before | Expected after | Evidence | Status | @@ -493,7 +508,7 @@ Source-listed case matrix: | null ordering diverges | must not change output | nullable-column case | nulls last | nulls last | multi-spec guard; green both ways | done | | .order() reverses key tuple | mixed directions unservable | helper unit matrix | declined | declined | index-utils.test.ts | done | | _creationTime last spec only | only after all fields consumed | helper unit matrix | n/a | declined unless final | index-utils.test.ts | done | -| eq-pinned any position/direction | must be preserved | helper unit + eq-pinned read test | served | served | 15->3 pre-fix | done | +| eq-pinned any position/direction | preserve row identity and bound when safe | helper unit + eq-pinned read/tie tests | blanket post-fetch | bounded with explicit creation tie; otherwise post-fetch | 15->3 with explicit tie; older tied row retained without it | done | | index selection leading-field-only | needs whole-spec selection | selection branch | fields[0] match | whole-sort match, same fallback | full suite green | done | | multi-probe bail (:6404) | separate bail | `in` union case | collect | bounded, order preserved | order asserted both pinned and unpinned primary | done | @@ -574,6 +589,8 @@ Timeline: index scoring; focused and full repository gates passed. - 2026-08-26 Reproduced and repaired the exact-head review P1 in non-null value ordering; UTF-8/Float64 edge proof and the full repository gate passed. +- 2026-08-26 Reproduced and repaired the exact-head review P1 in implicit tie + direction; tied top-k identity and explicit-tie bounded reads passed. Reboot status: | Question | Answer | @@ -592,8 +609,9 @@ High-risk note: - Proof plan: a 24-case helper unit matrix pinning every accept and every decline, plus integration tests that assert both the read bound at two table sizes AND the exact row order, including the eq-pinned divergent-direction - case, the `in` union with pinned and unpinned leading specs, cursor pages, and - null placement. 5 of 8 integration cases verified RED against stashed source. + case with and without an explicit tie-break, the `in` union with pinned and + unpinned leading specs, cursor pages, edge-value ordering, and null placement. + 5 of 8 original integration cases verified RED against stashed source. - Why this boundary is right: the helper already documented itself as the single owner of the decision; the bug was two other places re-deciding it from arity. Moving nullability onto `OrderSpec` keeps that ownership intact instead of diff --git a/packages/kitcn/skills/kitcn/references/features/orm.md b/packages/kitcn/skills/kitcn/references/features/orm.md index 39ebc497a..0392e9325 100644 --- a/packages/kitcn/skills/kitcn/references/features/orm.md +++ b/packages/kitcn/skills/kitcn/references/features/orm.md @@ -268,6 +268,10 @@ serves `orderBy: { rank: 'asc', createdAt: 'asc' }`, since the FK pins `userId`, `rank` is the next key and `createdAt` is Convex's implicit trailing one. Non-null values use Convex value ordering on both index-backed and post-fetch paths, including UTF-8 strings, signed zero, and NaN. +When the first requested field is equality-pinned but points opposite to the +moving fields, include `createdAt` in the moving direction as the final sort +field. Without that explicit tie-break, the ORM post-fetch sorts to preserve +which tied rows survive `limit`. Sorting by any other column — or by one that can be missing or `null` — reads the parent's whole child partition and sorts in memory; put the sort columns in the relation index, in sort order, to stay bounded. diff --git a/packages/kitcn/src/orm/index-utils.test.ts b/packages/kitcn/src/orm/index-utils.test.ts index fe688b34e..fd9511e6a 100644 --- a/packages/kitcn/src/orm/index-utils.test.ts +++ b/packages/kitcn/src/orm/index-utils.test.ts @@ -377,17 +377,19 @@ describe('resolveIndexOrderPushdown', () => { ).toBeNull(); }); - test('absorbs an eq-pinned field at any position and any direction', () => { - // `type` is constant across the scan, so it is already sorted whichever - // way the caller asked and wherever it sits in the sort. + test('preserves the implicit tie direction around eq-pinned fields', () => { + // Reversing the scan for the moving field would also reverse the + // implicit _creationTime tie-break that the old stable sort preserved. expect( resolveIndexOrderPushdown({ indexFields: ['type', 'numLikes'], pinnedEqCount: 1, orderSpecs: [spec('type', 'asc'), spec('numLikes', 'desc')], }) - ).toBe('desc'); + ).toBeNull(); + // A pinned field after the moving primary cannot change the scan's + // established direction. expect( resolveIndexOrderPushdown({ indexFields: ['type', 'numLikes'], @@ -395,6 +397,19 @@ describe('resolveIndexOrderPushdown', () => { orderSpecs: [spec('numLikes', 'desc'), spec('type', 'asc')], }) ).toBe('desc'); + + // An explicit creation-time tie-break makes reversal safe and bounded. + expect( + resolveIndexOrderPushdown({ + indexFields: ['type', 'numLikes'], + pinnedEqCount: 1, + orderSpecs: [ + spec('type', 'asc'), + spec('numLikes', 'desc'), + spec('_creationTime', 'desc'), + ], + }) + ).toBe('desc'); }); test('serves _creationTime only as the final spec', () => { diff --git a/packages/kitcn/src/orm/index-utils.ts b/packages/kitcn/src/orm/index-utils.ts index 832d1f0ee..4ab59c955 100644 --- a/packages/kitcn/src/orm/index-utils.ts +++ b/packages/kitcn/src/orm/index-utils.ts @@ -209,6 +209,22 @@ export function resolveIndexOrderPushdown(params: { cursor += 1; } + // Before whole-sort pushdown, the leading requested field chose the scan + // direction and the stable post-fetch sort preserved that direction for + // otherwise-equal rows. If that field is pinned while the first moving field + // points the other way, reversing the index also reverses the implicit + // _creationTime tie-break and can change which row survives a limit. An + // explicit _creationTime spec makes the tie direction part of the request. + const primary = orderSpecs[0]; + if ( + direction !== null && + pinnedFields.has(primary.field) && + primary.direction !== direction && + !consumedCreationTime + ) { + return null; + } + // A single spec is already served straight from the index today, including // on nullable columns, so gating it here would change shipped output and // give up the read bound at the same time. A multi-spec sort is the one that diff --git a/www/content/docs/orm/queries/index.mdx b/www/content/docs/orm/queries/index.mdx index 20e90b433..e0ed21856 100644 --- a/www/content/docs/orm/queries/index.mdx +++ b/www/content/docs/orm/queries/index.mdx @@ -177,6 +177,7 @@ const posts2 = await db.query.posts.findMany({ **Notes:** - An index that walks in the requested order serves the sort directly, so `limit` reads only the rows it returns. That needs the sort fields to be the index's leading keys, in order, all pointing the same way — `index('by_type_likes').on(t.type, t.numLikes)` serves `orderBy: { type: 'asc', numLikes: 'asc' }`. Fields pinned by an equality in `where` are constant across the scan, so they can appear anywhere in the sort and in either direction +- When the first requested field is equality-pinned but points opposite to the moving fields, include `createdAt` in the moving direction as the final sort field to make the implicit tie-break explicit. Without it, the ORM post-fetch sorts to preserve which tied rows survive `limit` - `createdAt` is servable as the last sort field once every other index key is pinned or consumed, because Convex appends it as the implicit trailing key - Non-null values use Convex value ordering on both index-backed and post-fetch paths, including UTF-8 string order and Float64 edge values such as signed zero and NaN - Any other sort — mixed directions, a field the index does not sort by next, or a column that can be missing or `null` — is applied post-fetch. The ORM then sorts before applying `offset`/`limit`, which requires reading the full filtered candidate set first From 3c636529abda18c58f9644c76ebcaaab9f69850f Mon Sep 17 00:00:00 2001 From: zbeyens Date: Wed, 26 Aug 2026 06:49:26 +0200 Subject: [PATCH 7/8] fix(orm): preserve trailing-key tie order --- .../skills/kitcn/references/features/orm.md | 3 ++ .changeset/nine-planets-marry.md | 4 ++- convex/orm/order-pushdown-reads.test.ts | 33 +++++++++++++++++++ convex/schema.ts | 6 ++++ ...-orm-multi-field-orderby-index-pushdown.md | 31 ++++++++++++++--- .../skills/kitcn/references/features/orm.md | 3 ++ packages/kitcn/src/orm/index-utils.test.ts | 4 +-- packages/kitcn/src/orm/index-utils.ts | 14 ++++++++ .../src/orm/where-clause-compiler.test.ts | 10 ++++++ .../kitcn/src/orm/where-clause-compiler.ts | 12 +++++++ www/content/docs/orm/queries/index.mdx | 1 + 11 files changed, 113 insertions(+), 8 deletions(-) diff --git a/.agents/skills/kitcn/references/features/orm.md b/.agents/skills/kitcn/references/features/orm.md index 0392e9325..032041e6e 100644 --- a/.agents/skills/kitcn/references/features/orm.md +++ b/.agents/skills/kitcn/references/features/orm.md @@ -272,6 +272,9 @@ When the first requested field is equality-pinned but points opposite to the moving fields, include `createdAt` in the moving direction as the final sort field. Without that explicit tie-break, the ORM post-fetch sorts to preserve which tied rows survive `limit`. +Every unpinned declared index key must be requested. An extra key after the +requested fields would break ties before Convex's implicit creation-time key, +so the ORM keeps the post-fetch sort to preserve which rows survive `limit`. Sorting by any other column — or by one that can be missing or `null` — reads the parent's whole child partition and sorts in memory; put the sort columns in the relation index, in sort order, to stay bounded. diff --git a/.changeset/nine-planets-marry.md b/.changeset/nine-planets-marry.md index 9440b39fb..d8ea9dc72 100644 --- a/.changeset/nine-planets-marry.md +++ b/.changeset/nine-planets-marry.md @@ -13,7 +13,9 @@ limit: 2 } }` now reads 2 children per parent instead of all of them. - Prefer an index that supplies more of the requested sort when several serve the filter equally well, so `(orgId, createdAt, title)` is chosen over - `(orgId, createdAt)` for a sort on both `createdAt` and `title`. + `(orgId, createdAt)` for a sort on both `createdAt` and `title`. An index with + another unrequested key after `title` is not treated as an exact sort because + that key would change which tied rows survive `limit`. - Stop warning that secondary `orderBy` fields are unstable across pages when the index carries the whole sort. A Convex cursor is the index key, so those pages are stable. The warning still fires — with corrected wording — when no diff --git a/convex/orm/order-pushdown-reads.test.ts b/convex/orm/order-pushdown-reads.test.ts index e0ea6e54f..df1506373 100644 --- a/convex/orm/order-pushdown-reads.test.ts +++ b/convex/orm/order-pushdown-reads.test.ts @@ -140,6 +140,39 @@ test('a pinned opposite-direction sort preserves the old implicit tie order', as }); }); +test('an unrequested trailing index field preserves the old implicit tie order', async () => { + const t = convexTest(schema); + await t.run(async (baseCtx) => { + await baseCtx.db.insert('posts', { + text: 'same', + numLikes: 1, + publishedAt: 2, + type: 'a', + }); + await baseCtx.db.insert('posts', { + text: 'same', + numLikes: 1, + publishedAt: 1, + type: 'a', + }); + }); + + await t.run(async (baseCtx) => { + const ctx = await runCtx(baseCtx); + const rows = await ctx.orm.query.posts.findMany({ + where: { type: 'a' }, + orderBy: (posts: any, { asc }: any) => [ + asc(posts.type), + asc(posts.numLikes), + asc(posts.text), + ], + limit: 1, + }); + + expect(rows.map((row: any) => row.publishedAt)).toEqual([2]); + }); +}); + test('two-field orderBy returns the same page a post-fetch sort would', async () => { const t = convexTest(schema); await t.run((baseCtx) => seedPosts(baseCtx, 60)); diff --git a/convex/schema.ts b/convex/schema.ts index 3e8d5fc8c..4b1d94416 100644 --- a/convex/schema.ts +++ b/convex/schema.ts @@ -62,6 +62,12 @@ export const posts = convexTable( index('by_published_at').on(t.publishedAt), index('by_title').on(t.title), index('numLikesAndType').on(t.type, t.numLikes), + index('by_type_likes_text_published').on( + t.type, + t.numLikes, + t.text, + t.publishedAt + ), searchIndex('text_search').on(t.text).filter(t.type), vectorIndex('embedding_vec') .on(t.embedding) diff --git a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md index 6d250f29f..d96ec6318 100644 --- a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md +++ b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md @@ -45,7 +45,8 @@ Completion threshold: - `resolveIndexOrderPushdown` answers multi-spec sorts correctly (eq-pinned fields absorbed without consuming an index key; remaining specs contiguous from `indexFields[eqCount]`; `_creationTime` legal only as the final spec after - every declared field is consumed; all unpinned specs share one direction; + every declared field is consumed; no unrequested moving index key may break + requested-field ties; all unpinned specs share one direction; opposite-direction pinned-leading sorts preserve their implicit tie order unless `_creationTime` makes it explicit), every planner call site passes the full spec list, and the read bound is @@ -90,7 +91,8 @@ Constraints: Boundaries: - Source of truth: GitHub issue #416. - Allowed edit scope: `packages/kitcn/src/orm/{index-utils,query,where-clause-compiler}.ts`, - their unit tests, `convex/orm/order-pushdown-reads.test.ts`, `.changeset/`. + their unit tests, `convex/{schema,orm/order-pushdown-reads.test}.ts`, + current-state ORM docs/skill guidance, and `.changeset/`. - Browser surface: N/A: server-side query planner, no rendered output. - GitHub issue sync: `Fixes #416` in PR #426 owns the issue linkage; no separate issue comment is needed. @@ -323,7 +325,7 @@ Completion Gates: | Final lint | yes | Run `bun lint:fix` or scoped equivalent | bun lint:fix then bun lint -> no fixes applied | | Output budget discipline | yes | Verify no unbounded high-volume command output was streamed, or record the accidental output and recovery | workflow output artifacted to /tmp/wf-facts.txt; no unbounded stream | | Timed checkpoint | no | If duration was requested, keep improving until elapsed, then finish the current loop cleanly; otherwise N/A | N/A: no duration requested | -| Autoreview for non-trivial implementation changes | yes | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | original local review closed 2 conditional findings; autoclosure exact-head reviews found 3 real P1s in pinned-order scoring, non-null value ordering, and implicit tie direction, all repaired with RED/green proof; immutable-head rerun is recorded in the terminal receipt | +| Autoreview for non-trivial implementation changes | yes | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | original local review closed 2 conditional findings; autoclosure exact-head reviews found 4 real P1s in pinned-order scoring, non-null value ordering, implicit tie direction, and trailing-key tie identity, all repaired with RED/green proof; immutable-head rerun is recorded in the terminal receipt | | Goal plan complete | yes | Run `node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md` | node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md | | Public API / package boundary proof | yes | Source-audit public API, exports, and package boundary impact | index-utils.ts importers audited: only orm/* and cli/utils/schema-tables.ts (which imports getAggregateIndexes/getRankIndexes only). OrderSpec is not publicly exported | | Convex bundle/import proof | no | Audit affected function-entry static graphs or record N/A | N/A: no new cross-module imports; OrderSpec is a type-only import within orm/ | @@ -342,7 +344,7 @@ Phase / pass table: | Implementation | done | helper rewrite + 5 call sites + index scoring + docs + changeset | verification | | Verification | done | 8/8 new tests, 879 vitest, 1323 bun test, typecheck, lint, build | closeout | | Commit / PR / GitHub sync | done | `bun check` green, pushed, PR #426 opened with the task-style body | final response | -| Closeout | done | all three exact-head P1s repaired; final full `bun check` green | immutable-head review, terminal GitHub receipt, and merge | +| Closeout | done | four exact-head P1s repaired; focused proof and final full `bun check` green | immutable-head review, terminal GitHub receipt, and merge | Findings: - Two independent blanket bails, exactly as the issue said: the helper's arity @@ -444,6 +446,11 @@ Review fixes: The resolver now declines that pushdown unless `_creationTime` is explicit. RED/green proof retains the older tied row without the explicit tie-break and keeps both non-cursor and cursor reads bounded when it is explicit. +- The fourth exact-head review found that an unrequested trailing declared + index key can replace the previous creation-time tie order when index scoring + selects a fuller candidate. The resolver now declines whole-sort pushdown in + that shape, and scoring does not reward the unsafe longer candidate over the + prior shorter index. RED/green proof keeps the older tied row across `limit`. Error attempts: | Error / failed attempt | Count | Next different move | Resolution | @@ -452,6 +459,8 @@ Error attempts: | Exact-head autoreview P1 in candidate-index scoring | 1 | Add the missing narrow-vs-compound regression before changing the scorer | RED selected `by_type`; repair selects `by_type_likes`, with pinned-only and probe cases green | | Exact-head autoreview P1 in value-order equivalence | 1 | Reproduce the UTF-8 mismatch, then use Convex's public comparator at the post-fetch owner | RED put emoji before U+E000; repair follows Convex UTF-8, signed-zero, and NaN order | | Exact-head autoreview P1 in implicit tie direction | 1 | Add two tied rows around a limit boundary before narrowing servability | RED returned the newer tied row; repair retains the older row unless creation direction is explicit | +| Exact-head autoreview P1 in trailing-key tie identity | 1 | Reproduce with a shorter baseline index and a fuller index carrying one unrequested key | RED selected `publishedAt: 1`; repair retains the older `publishedAt: 2` row and the shorter baseline index | +| Typecheck raced package build cleanup | 1 | Rerun after the package build finished instead of concurrently | 5/5 typecheck tasks passed | Verification evidence: - cwd for every command below: /Users/mikey/conductor/workspaces/kitcn/dakar @@ -498,6 +507,14 @@ Verification evidence: with 1 integration test skipped. - Tie-direction final `bun check` -> exit 0, including unit, typecheck, lint, package build (72 files, 1623.20 kB), fixture parity, and all runtime lanes. +- Trailing-key RED: the fuller `(type, numLikes, text, publishedAt)` index put + `publishedAt: 1` ahead of the older tied `publishedAt: 2` row at `limit: 1`. +- Trailing-key repair: 12/12 focused read-bound tests, 52 owner/compiler tests, + and the 173-pass focused ORM suite are green; typecheck is 5/5 and the package + build produced 72 files (1623.42 kB). +- Trailing-key final `bun check` -> exit 0, including lint, typecheck, complete + unit/integration suites, CLI and Concave smoke, fixture parity, package builds, + and all bare/Expo/Next/Start/Vite runtime lanes. Source-listed case matrix: | Case | Source claim | Harness | Before | Expected after | Evidence | Status | @@ -509,6 +526,7 @@ Source-listed case matrix: | .order() reverses key tuple | mixed directions unservable | helper unit matrix | declined | declined | index-utils.test.ts | done | | _creationTime last spec only | only after all fields consumed | helper unit matrix | n/a | declined unless final | index-utils.test.ts | done | | eq-pinned any position/direction | preserve row identity and bound when safe | helper unit + eq-pinned read/tie tests | blanket post-fetch | bounded with explicit creation tie; otherwise post-fetch | 15->3 with explicit tie; older tied row retained without it | done | +| unrequested trailing index key | preserve old tie identity across `limit` | helper/compiler unit + tied-row integration test | fuller index selected `publishedAt: 1` | shorter baseline scan retains older `publishedAt: 2` | RED/green at `limit: 1` | done | | index selection leading-field-only | needs whole-spec selection | selection branch | fields[0] match | whole-sort match, same fallback | full suite green | done | | multi-probe bail (:6404) | separate bail | `in` union case | collect | bounded, order preserved | order asserted both pinned and unpinned primary | done | @@ -591,6 +609,8 @@ Timeline: ordering; UTF-8/Float64 edge proof and the full repository gate passed. - 2026-08-26 Reproduced and repaired the exact-head review P1 in implicit tie direction; tied top-k identity and explicit-tie bounded reads passed. +- 2026-08-26 Reproduced and repaired the exact-head review P1 in unrequested + trailing-key tie order; resolver, scorer, and tied-row proof passed. Reboot status: | Question | Answer | @@ -605,7 +625,8 @@ High-risk note: - Realistic failure mode: the helper claims an order the index cannot serve, the planner skips the JS sort, and rows come back silently mis-ordered — or, in the relation loader, each parent truncates the wrong top-k and rows are lost before - the global sort ever sees them. No error, no exception. + the global sort ever sees them. A fuller index can also inject an unrequested + tie key before creation time and change which row survives. No error, no exception. - Proof plan: a 24-case helper unit matrix pinning every accept and every decline, plus integration tests that assert both the read bound at two table sizes AND the exact row order, including the eq-pinned divergent-direction diff --git a/packages/kitcn/skills/kitcn/references/features/orm.md b/packages/kitcn/skills/kitcn/references/features/orm.md index 0392e9325..032041e6e 100644 --- a/packages/kitcn/skills/kitcn/references/features/orm.md +++ b/packages/kitcn/skills/kitcn/references/features/orm.md @@ -272,6 +272,9 @@ When the first requested field is equality-pinned but points opposite to the moving fields, include `createdAt` in the moving direction as the final sort field. Without that explicit tie-break, the ORM post-fetch sorts to preserve which tied rows survive `limit`. +Every unpinned declared index key must be requested. An extra key after the +requested fields would break ties before Convex's implicit creation-time key, +so the ORM keeps the post-fetch sort to preserve which rows survive `limit`. Sorting by any other column — or by one that can be missing or `null` — reads the parent's whole child partition and sorts in memory; put the sort columns in the relation index, in sort order, to stay bounded. diff --git a/packages/kitcn/src/orm/index-utils.test.ts b/packages/kitcn/src/orm/index-utils.test.ts index fd9511e6a..3b2572124 100644 --- a/packages/kitcn/src/orm/index-utils.test.ts +++ b/packages/kitcn/src/orm/index-utils.test.ts @@ -339,7 +339,7 @@ describe('resolveIndexOrderPushdown', () => { nullable: false, }); - test('serves a sort that is a prefix of the index, in index order', () => { + test('serves a sort only when no unrequested key breaks its ties', () => { expect( resolveIndexOrderPushdown({ indexFields: ['type', 'numLikes'], @@ -354,7 +354,7 @@ describe('resolveIndexOrderPushdown', () => { pinnedEqCount: 0, orderSpecs: [spec('type', 'desc'), spec('numLikes', 'desc')], }) - ).toBe('desc'); + ).toBeNull(); }); test('declines a sort that skips or reorders an index key', () => { diff --git a/packages/kitcn/src/orm/index-utils.ts b/packages/kitcn/src/orm/index-utils.ts index 4ab59c955..581613d93 100644 --- a/packages/kitcn/src/orm/index-utils.ts +++ b/packages/kitcn/src/orm/index-utils.ts @@ -209,6 +209,20 @@ export function resolveIndexOrderPushdown(params: { cursor += 1; } + // A trailing declared key breaks ties before Convex reaches its implicit + // creation-time suffix. The old stable post-fetch sort preserved the + // selected scan's creation order for rows tied on every requested field, so + // choosing a longer index and truncating before that sort can change row + // identity. Single-field pushdown already ships with this behavior; the new + // multi-field path must consume every moving declared key. + if ( + orderSpecs.length > 1 && + direction !== null && + cursor < indexFields.length + ) { + return null; + } + // Before whole-sort pushdown, the leading requested field chose the scan // direction and the stable post-fetch sort preserved that direction for // otherwise-equal rows. If that field is pinned while the first moving field diff --git a/packages/kitcn/src/orm/where-clause-compiler.test.ts b/packages/kitcn/src/orm/where-clause-compiler.test.ts index b9546791c..763a8ce99 100644 --- a/packages/kitcn/src/orm/where-clause-compiler.test.ts +++ b/packages/kitcn/src/orm/where-clause-compiler.test.ts @@ -330,6 +330,10 @@ describe('WhereClauseCompiler advanced index planning', () => { indexName: 'by_type_likes', indexFields: ['type', 'numLikes'], }, + { + indexName: 'by_type_likes_text_rank', + indexFields: ['type', 'numLikes', 'text', 'rank'], + }, ]); const result = compiler.compile(eq(fieldRef('type') as any, 'a'), { @@ -349,6 +353,12 @@ describe('WhereClauseCompiler advanced index planning', () => { { orderFields: ['type'] } ) as any; expect(pinnedOnly.selectedIndex?.indexName).toBe('by_type'); + + const trailingKey = compiler.compile( + eq(fieldRef('type') as any, 'a'), + { orderFields: ['type', 'numLikes', 'text'] } + ) as any; + expect(trailingKey.selectedIndex?.indexName).toBe('by_type_likes'); }); test('keeps the narrow index when no candidate supplies the order', () => { diff --git a/packages/kitcn/src/orm/where-clause-compiler.ts b/packages/kitcn/src/orm/where-clause-compiler.ts index 09c0ad628..5fb92f012 100644 --- a/packages/kitcn/src/orm/where-clause-compiler.ts +++ b/packages/kitcn/src/orm/where-clause-compiler.ts @@ -985,6 +985,18 @@ export class WhereClauseCompiler { orderOffset += 1; } + // For a multi-field sort, a candidate that covers the whole request but + // keeps another moving key would replace the old implicit creation-time + // tie order with that key. Do not reward it over the shorter baseline + // index; the query planner will keep the stable post-fetch sort. + if ( + this.orderFields.length > 1 && + orderOffset === this.orderFields.length && + indexOffset < indexFields.length + ) { + return 0; + } + return served; } diff --git a/www/content/docs/orm/queries/index.mdx b/www/content/docs/orm/queries/index.mdx index e0ed21856..1c11cad76 100644 --- a/www/content/docs/orm/queries/index.mdx +++ b/www/content/docs/orm/queries/index.mdx @@ -178,6 +178,7 @@ const posts2 = await db.query.posts.findMany({ **Notes:** - An index that walks in the requested order serves the sort directly, so `limit` reads only the rows it returns. That needs the sort fields to be the index's leading keys, in order, all pointing the same way — `index('by_type_likes').on(t.type, t.numLikes)` serves `orderBy: { type: 'asc', numLikes: 'asc' }`. Fields pinned by an equality in `where` are constant across the scan, so they can appear anywhere in the sort and in either direction - When the first requested field is equality-pinned but points opposite to the moving fields, include `createdAt` in the moving direction as the final sort field to make the implicit tie-break explicit. Without it, the ORM post-fetch sorts to preserve which tied rows survive `limit` +- Every unpinned declared index key must be requested. An extra key after the requested fields would break ties before Convex's implicit creation-time key, so the ORM keeps the post-fetch sort to preserve which rows survive `limit` - `createdAt` is servable as the last sort field once every other index key is pinned or consumed, because Convex appends it as the implicit trailing key - Non-null values use Convex value ordering on both index-backed and post-fetch paths, including UTF-8 string order and Float64 edge values such as signed zero and NaN - Any other sort — mixed directions, a field the index does not sort by next, or a column that can be missing or `null` — is applied post-fetch. The ORM then sorts before applying `offset`/`limit`, which requires reading the full filtered candidate set first From 1c1d8e86f3acfe1f44e6be640d3f72df67e6e30c Mon Sep 17 00:00:00 2001 From: zbeyens Date: Wed, 26 Aug 2026 06:52:57 +0200 Subject: [PATCH 8/8] docs(plan): record final orm review --- docs/plans/416-orm-multi-field-orderby-index-pushdown.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md index d96ec6318..c22f9dc73 100644 --- a/docs/plans/416-orm-multi-field-orderby-index-pushdown.md +++ b/docs/plans/416-orm-multi-field-orderby-index-pushdown.md @@ -325,7 +325,7 @@ Completion Gates: | Final lint | yes | Run `bun lint:fix` or scoped equivalent | bun lint:fix then bun lint -> no fixes applied | | Output budget discipline | yes | Verify no unbounded high-volume command output was streamed, or record the accidental output and recovery | workflow output artifacted to /tmp/wf-facts.txt; no unbounded stream | | Timed checkpoint | no | If duration was requested, keep improving until elapsed, then finish the current loop cleanly; otherwise N/A | N/A: no duration requested | -| Autoreview for non-trivial implementation changes | yes | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | original local review closed 2 conditional findings; autoclosure exact-head reviews found 4 real P1s in pinned-order scoring, non-null value ordering, implicit tie direction, and trailing-key tie identity, all repaired with RED/green proof; immutable-head rerun is recorded in the terminal receipt | +| Autoreview for non-trivial implementation changes | yes | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | original local review closed 2 conditional findings; autoclosure exact-head reviews found 4 real P1s in pinned-order scoring, non-null value ordering, implicit tie direction, and trailing-key tie identity, all repaired with RED/green proof; final immutable code-head rerun is clean at 0.89 confidence | | Goal plan complete | yes | Run `node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md` | node .agents/skills/autogoal/scripts/check-complete.mjs docs/plans/416-orm-multi-field-orderby-index-pushdown.md | | Public API / package boundary proof | yes | Source-audit public API, exports, and package boundary impact | index-utils.ts importers audited: only orm/* and cli/utils/schema-tables.ts (which imports getAggregateIndexes/getRankIndexes only). OrderSpec is not publicly exported | | Convex bundle/import proof | no | Audit affected function-entry static graphs or record N/A | N/A: no new cross-module imports; OrderSpec is a type-only import within orm/ | @@ -515,6 +515,8 @@ Verification evidence: - Trailing-key final `bun check` -> exit 0, including lint, typecheck, complete unit/integration suites, CLI and Concave smoke, fixture parity, package builds, and all bare/Expo/Next/Start/Vite runtime lanes. +- Final immutable code-head autoreview against `kitcn/main` -> clean, no + accepted/actionable P0/P1 findings (0.89 confidence). Source-listed case matrix: | Case | Source claim | Harness | Before | Expected after | Evidence | Status |