fix(sqlite): apply limit after reordering in getUserPromptsByIds (follow-up to #2153)#3347
Open
justindeisler wants to merge 1 commit into
Open
Conversation
Follow-up to thedotmack#2153. That fix added 'relevance' to all three SessionStore hydration methods, but getUserPromptsByIds only received half of the treatment its two siblings got. getObservationsByIds and getSessionSummariesByIds both guard the LIMIT on !preserveIdOrder and slice after the reorder: const limitClause = limit && !preserveIdOrder ? `LIMIT ${limit}` : ''; ... const ordered = ids.map(id => rowMap.get(id)).filter(...); return limit ? ordered.slice(0, limit) : ordered; getUserPromptsByIds has neither. Under orderBy: 'relevance' the ORDER BY clause is empty, so SQLite satisfies the LIMIT straight from the id-index scan in ascending rowid order. The method truncates to the OLDEST n candidates and only then permutes the survivors into caller order -- so a caller passing a ranked id list loses its top hits before the reorder ever runs. With 5 prompts requested highest-id-first and limit 3, it returned a single row instead of the caller's top 3. This is latent rather than user-visible: no caller in the tree passes 'relevance' to this method today. It bites the moment one does, which is exactly what thedotmack#2153 set out to make safe. The method is now byte-identical in shape to its two siblings. Two tests added to the existing thedotmack#2153 suite; the first fails without this change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2153.
That fix added
'relevance'support to all threeSessionStorehydration methods.getObservationsByIdsandgetSessionSummariesByIdsalso got the matching limit handling;getUserPromptsByIdsdid not, and still has the original behaviour.The divergence
Both siblings guard the LIMIT and slice after the reorder:
getUserPromptsByIdshas neither — it appliesLIMITunconditionally and returns straight from the reorder.Why that breaks
Under
orderBy: 'relevance'the ORDER BY clause is deliberately empty, so SQLite is free to satisfy the LIMIT directly from the id-index scan — in ascending rowid order. The method therefore truncates to the oldest n candidates and only then permutes the survivors into caller order. A caller passing a ranked id list loses its top hits before the reorder ever runs.Concretely, with 5 prompts requested highest-id-first and
limit: 3, it returns one row rather than the caller's top 3: SQL keeps the three lowest ids, then the reorder discards the two of those that aren't in the caller's first 3.Scope
This is latent, not user-visible. No caller in the tree passes
'relevance'to this method today, so nothing currently misbehaves — I'd rather say that plainly than oversell it. It bites the moment a caller does, which is the case #2153 set out to make safe, and the asymmetry between the three methods is the kind of thing that's much cheaper to fix now than to debug later.Changes
src/services/sqlite/SessionStore.ts— the two lines that bring the method in line with its siblings.tests/services/sqlite/get-observations-by-ids-relevance.test.ts— two tests appended to the existing Bug: ChromaSearchStrategy 'relevance' ordering silently returns rows in date_desc order #2153 suite. The first (applies limit AFTER reordering) fails onmainand passes with the fix; the second covers the no-limit path.No behaviour change for any existing caller: with
preserveIdOrderfalse the generated SQL and the return value are identical to before.Verification
Branched from
mainat f5633c1 (v13.11.0).