Skip to content

fix(retrieval): order the tier-2 vector scan window by recency, not physical position - #2310

Open
simpleqt wants to merge 2 commits into
MemTensor:mainfrom
simpleqt:fix/tier2-recency-ordered-scan
Open

fix(retrieval): order the tier-2 vector scan window by recency, not physical position#2310
simpleqt wants to merge 2 commits into
MemTensor:mainfrom
simpleqt:fix/tier2-recency-ordered-scan

Conversation

@simpleqt

Copy link
Copy Markdown

Fixes #2233

Problem

scanAndTopK bounded the cosine candidate set with a bare LIMIT <hardCap> — no ORDER BY — so the window was SQLite's arbitrary physical scan order. The globally best vector could be excluded from similarity computation purely by physical position (cosine 1.0, zero recall probability). With the v2.0.14 defaults (tier2TopK=5, candidatePoolFactor=4) the effective vector scan cap was ~80–100 arbitrary rows per channel.

While writing the regression test I also confirmed the arbitrariness empirically: the same query over the same rows inserted in a different physical order returns a different winner under the unordered cap.

Fix

  • scanAndTopK takes an optional orderBy (applied before the hardCap LIMIT); the option's contract is repo-internal constant strings, never request input.
  • All four vector repos pass their recency column: tracests DESC, id DESC; policies / world_model / skillsupdated_at DESC, id DESC. Every one of these is already served by an existing DESC index (idx_traces_owner/share/session_ts, idx_policies_owner/status/..., idx_skills_owner), so the bounded window stays index-friendly.
  • The bounded-scan semantics are documented on scanAndTopK: the window is now a deterministic candidate policy (most recent qualifying rows first) rather than a physical accident.

This is the "deterministic, meaningful candidate policy" alternative the issue lists — it does not change the bounded-scan approximation itself, it makes the bound well-defined.

Testing

New case in tests/unit/storage/vector-stream.test.ts: two perfect-match vectors (one old, one new) plus orthogonal fillers; with orderBy: "ts DESC, id DESC" and hardCap: 1, the window deterministically resolves to the newest perfect match in both physical row layouts (insertion order and reversed). The unordered path is deliberately left un-asserted — which row it returns is layout-dependent, which is exactly the bug.

  • vitest run tests/unit → 1550 passed, 2 skipped (179 files)
  • tsc -p tsconfig.json --noEmit → clean

…hysical position

scanAndTopK bounded the cosine candidate set with a bare LIMIT — SQLite
returned rows in whatever order its physical scan visited them, so the
globally best vector could be excluded from similarity computation purely
by physical position. With the v2.0.14 defaults (tier2TopK=5,
candidatePoolFactor=4) the effective window was 80-100 arbitrary rows.

The scan now takes an orderBy option and all four vector repos pass their
recency column (traces: ts DESC; policies/world_model/skills:
updated_at DESC, all DESC-indexed), making the bounded window a
deterministic, meaningful candidate policy — most recent qualifying rows
first — that existing indexes already serve.

Fixes MemTensor#2233
Copilot AI lite review requested due to automatic review settings August 31, 2026 01:57
@Memtensor-AI Memtensor-AI added area:plugin OpenClaw & Hermes status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Aug 31, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Memtensor-AI

Memtensor-AI commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

🤖 Open Code Review

Target: PR #2310
Task: d7dd1c5d708f6868
Base: main
Head: fix/tier2-recency-ordered-scan

🔍 OpenCodeReview found 2 issue(s) in this PR.


1. apps/memos-local-plugin/core/storage/vector.ts (L226-L227)

The allowlist regex rejects most injection attempts, but \s+ matches any whitespace including newlines and tabs. A value like "updated_at\nDESC" would pass the regex, and while SQLite normalises embedded newlines harmlessly today, it is surprising behaviour and a fragile assumption. Tighten to [ ]+ (a plain space or \x20+) so only a single-space separator is valid — this matches all current caller strings and closes the whitespace ambiguity.

Suggestion:
/^[a-z_][a-z0-9_]( +(asc|desc))?(, [a-z_][a-z0-9_]( +(asc|desc))?)$/i


2. apps/memos-local-plugin/core/storage/vector.ts (L257-L258)

The error message includes the raw orderBy value via JSON.stringify. If orderBy ever originates from a context where it could contain sensitive schema information (column names reflecting internal data classifications, etc.), logging or surfacing it in an unhandled-rejection handler could leak that. Consider omitting the value and logging only a fixed string, or truncating to a safe length, since the calling code already knows what it passed.

Suggestion:
throw new Error(scanAndTopK: orderBy value failed safety validation);

Generated by cloud-assistant via Open Code Review.

The open code review on MemTensor#2310 flagged that interpolating an unvalidated
public-interface option into SQL is one careless future caller away from
injection. Reject anything that is not an identifier list with optional
ASC/DESC at the boundary; the repos' constant strings pass unchanged.
@simpleqt

Copy link
Copy Markdown
Author

Both open-code-review findings addressed in 8e038cc: scanAndTopK now validates orderBy against an identifier-list allowlist (optional ASC/DESC per column) and throws unsafe orderBy before any SQL is built, so the public option can't become an injection vector even for a future caller that passes request-derived input. The repos' constant strings are unchanged, and a new test pins the guard (a ts DESC; DROP TABLE bench -- value throws and leaves the table untouched).

vitest run tests/unit/storage/ → 85 passed; tsc -p tsconfig.json --noEmit → clean.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

✅ Automated Test Results: PASSED

All tests passed (20/20 executed). memos_local_plugin/unit: 20/20. Duration: 5s [advisory, non-gating] AI-generated tests on branch test/auto-gen-d7dd1c5d708f6868-20260831101902: 93/93 passed — these do NOT affect the PR verdict; review the branch manually.

Branch: fix/tier2-recency-ordered-scan

@Memtensor-AI Memtensor-AI added status:ready Ready for implementation; waiting for assignee or AI dispatch | 可进入实现,等待认领或派发 and removed status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:plugin OpenClaw & Hermes status:ready Ready for implementation; waiting for assignee or AI dispatch | 可进入实现,等待认领或派发

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: Tier-2 vector recall applies unordered LIMIT before cosine ranking, causing false negatives

4 participants