Skip to content

Apply review feedback to minimal Cortex query (P0-D)#50

Merged
devlux76 merged 5 commits intop0d/cortex-query-minimalfrom
copilot/sub-pr-49
Mar 13, 2026
Merged

Apply review feedback to minimal Cortex query (P0-D)#50
devlux76 merged 5 commits intop0d/cortex-query-minimalfrom
copilot/sub-pr-49

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 13, 2026

Five issues identified in the P0-D review: unsorted combined results, missing defensive docs on concatVectors, no pagination warning on getAllPages, cramped stub formatting, and missing test cases that P0-D3 claimed complete.

Changes

  • cortex/Query.ts – Sort combined by descending score before building the return value; add JSDoc to concatVectors documenting the non-empty-array precondition
  • storage/IndexedDbMetadataStore.ts – Add TODO on getAllPages() flagging the full-table-scan as unfit for large corpora; should be replaced with paginated/indexed access
  • tests/SalienceEngine.test.ts – Reformat three stub methods crammed on one line to separate lines, consistent with the rest of the mock
  • tests/cortex/Query.test.ts – Add the two tests that P0-D3 listed but were absent: empty-corpus (no results) and descending-score-order (relevance); fix pre-existing TestVectorBackend.kind type error ("test""wasm")
// Before: hotpath hits always preceded cold hits regardless of score
const combined = [...hotpathResults, ...coldResults];

// After: globally ranked
const combined = [...hotpathResults, ...coldResults];
combined.sort((a, b) => b.score - a.score);

💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.

…ting, add missing tests

Co-authored-by: devlux76 <86517969+devlux76@users.noreply.github.com>
Copilot AI changed the title [WIP] [issue #21] Implement minimal Cortex query Apply review feedback to minimal Cortex query (P0-D) Mar 13, 2026
Copilot AI requested a review from devlux76 March 13, 2026 08:41
@devlux76 devlux76 marked this pull request as ready for review March 13, 2026 08:48
Copilot AI review requested due to automatic review settings March 13, 2026 08:48
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR applies prior review feedback to the minimal Cortex query path by improving global result ranking, documenting a key precondition in vector concatenation, flagging an IndexedDB scalability concern, and adding missing unit tests for query behavior.

Changes:

  • Sort merged hotpath + cold query results by descending score before returning.
  • Document concatVectors’ non-empty/equal-length precondition and add a scalability TODO for getAllPages().
  • Add/adjust unit tests for empty-corpus behavior and descending-score ordering; reformat MetadataStore stubs in an existing test.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
cortex/Query.ts Sorts combined results by score and documents concatVectors precondition.
storage/IndexedDbMetadataStore.ts Adds a TODO warning that getAllPages() is a full-table scan unsuitable for large corpora.
tests/SalienceEngine.test.ts Re-formats stubbed methods for readability/consistency.
tests/cortex/Query.test.ts Adds missing query tests and updates the test vector backend kind.

Comment thread tests/cortex/Query.test.ts Outdated
Comment thread storage/IndexedDbMetadataStore.ts Outdated
devlux76 and others added 2 commits March 13, 2026 02:57
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@devlux76 devlux76 merged commit 8c8f099 into p0d/cortex-query-minimal Mar 13, 2026
2 checks passed
@devlux76 devlux76 deleted the copilot/sub-pr-49 branch March 13, 2026 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants