State that recall's limit applies to the backend's ordering - #129
Conversation
A driver that receives more candidates than `limit` and reduces them itself must keep the order the backend returned and take a prefix. The contract said ordering was backend-defined but never said an implementation may not impose its own, and the Cortex driver read that gap the wrong way: it folded, sorted by key, then truncated, so recall returned whichever keys sorted first and discarded the engine's best hits. Fixed there in tinyhumansai#128; this is the rule it should have been able to read. The recall fixture now stores distinct content per key. Identical content leaves a backend free to order ties however it likes, and an arbitrary order is one nothing downstream can assert about. The assertion's docs say plainly that it does *not* check this rule, and why: the suite cannot know how an engine ranks these rows, and a driver that sorts before truncating is self-consistent, so no black-box comparison of two limits separates it from a correct one. That check belongs to each driver that folds before returning, and the doc points at the one in tinymemory-remote as the shape to copy. Better an explicit gap than a fixture that reads as coverage and is not.
How this change flows1 changed behaviour across 4 relationships. 5 surrounding behaviours are shown (60 graph nodes walked). 35 further behaviours left out to keep the diagram readable. flowchart LR
n0["assert_taint_is_preserved<br/>changed"]:::changed
n1["assert_provider"]:::impacted
n2["ns"]:::impacted
n3["cleanup"]:::impacted
n4["Result"]:::impacted
n5["Memory"]:::impacted
n0 -->|calls| n2
n0 -->|calls| n3
n1 -->|calls| n0
n5 -->|uses| n4
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe ChangesRecall contract and conformance alignment
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This PR clarifies recall truncation behavior and updates fixture content without changing production runtime behavior; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Summary
Follow-up to #128, where
searchfolded the engine's answer, sorted it by key,and then truncated to
limit— so recall returned whichever keys sorted firstand threw away the engine's best hits. Ranking is the entire reason that
integration exists, and the driver was discarding it at the last step.
The rule it should have been able to read did not exist. The contract said
result ordering is backend-defined and that callers must not assume a stable
order across backends; it never said an implementation may not impose one.
This states it: a driver that receives more candidates than
limitand reducesthem itself must keep the backend's order and take a prefix of it.
Related issue
None. Follow-up to #128.
API or behavior changes
None. Documentation, plus test-fixture content.
Validation
cargo fmt --all -- --check— cleancargo clippy --all-targets --all-features -- -D warnings— cleancargo build --all-targets --all-features— cleancargo test --all-features— 33 targets, 0 failuresAlso
RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features(clean), andthe live CortexDB lane (passes, 182s).
Tests
The recall fixture now stores distinct content per key instead of
"shared needle text"three times. Identical content leaves a backend free to order tieshowever it likes, and an arbitrary order is one nothing downstream can assert
about. All five drivers and the live lane pass unchanged.
What I did not add, and why it matters that it is written down. I looked at
making
assert_recall_respects_limit_and_namespacecatch the #128 bug directlyand concluded it cannot, so the assertion now says so in its own docs rather
than reading as coverage it does not provide:
cannot name the row that should come back first;
before truncating is self-consistent, so
recall(limit 1)is still thefirst element of
recall(limit 3).What a black-box assertion here would catch is non-deterministic ordering,
which is a narrower fault, and pinning it would make the suite flaky against any
live engine whose ranking is not stable between two calls. That trade is not
worth it in a fixture that gates five drivers.
So the check belongs to each driver that folds or deduplicates before returning
— every append-only backend does, and the fold is exactly where an order gets
imposed. The docs point at
recall_keeps_the_engine_ranking_when_it_truncatesin
tinymemory-remoteas the shape to copy.I checked the other four dialects while I was here:
supermemory,mem0,cogneeandagentmemoryall passlimitdown to their backend and nonereorders what it returned, so none has this bug today. The gap was only ever
reachable by a driver that truncates client-side.
Documentation
Memory::recall's contract docs carry the rule; the conformance assertion'sdocs carry the gap and where it is covered instead.
Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the descriptionSummary by CodeRabbit
Documentation
Tests