Repository navigation
fix: merge only identical text in memory deduplication - #1759
Merged
Merged
Conversation
lcm store, promote, passive events and knowledge import merged any candidate whose BM25 rank cleared dedupBm25Threshold into that memory. A merge keeps the canonical content and discards the incoming text, and BM25 grows with the number of matched rare terms, so on a realistic corpus a long or issue-number-heavy text outranked the threshold against an unrelated memory. The new text was dropped and only its tags were unioned into the unrelated memory, which then matched even more queries. The decision now merges only a candidate whose content is byte-identical to the incoming content, whatever its rank. Candidate retrieval, the owner exact-content fallback, canonical choice, confidence and tag merge, and archiving of further identical duplicates are unchanged. Gotchas: compaction.promotionThresholds.dedupBm25Threshold is still accepted but no longer influences the decision; removing the key is a follow-up. Paraphrased or reformatted repeats now create separate rows (recoverable) instead of losing text. Memories damaged by earlier false merges are not repaired. The passive Tier 3 existence gate still admits an event on any lexical hit, which now inserts a new memory. Tests that relied on fuzzy merging now use identical content where their intent is a merge path; nonidentical strong-rank candidates are asserted to insert, including on a mixed candidate page. Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
bcdonadio
enabled auto-merge
October 25, 2026 07:43
bcdonadio
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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.
Suggested title:
fix: merge only identical text in memory deduplicationSummary
The shared deduplication decision (
src/promotion/dedup.ts) now merges a candidate only when its text is exactly the same as the incoming text. A BM25 rank pastdedupBm25Thresholdno longer merges different text. This coverslcm store,lcm promoteand compaction promotion, passive event promotion, and knowledge import, on SQLite, PostgreSQL and the legacy bridge.Motivation / Why
A merge keeps the existing memory's content and throws away the incoming text. The old rule merged any candidate whose FTS rank cleared an absolute BM25 threshold (default 15). BM25 is a sum over matched terms, and with an OR query a long text easily scores past 15 against an unrelated short memory in a corpus of a few hundred entries. On a real project corpus, the issue's reproducer ranked -47.4 against an unrelated memory, which then absorbed it. Every merged-away text was lost, and its tags were unioned into the "magnet" memory, so the magnet matched even more future queries.
Before:
lcm store '<new long note>'returns{"stored": true, "id": "<unrelated existing id>"}, and the note is not searchable.After: a fresh id, and the note is stored. Re-storing exactly the same text still returns the existing id.
Fuzzy alternatives were rejected during plan review: a word-set similarity merges texts with swapped facts (similarity 1.0), and whitespace folding merges distinct code snippets. Exact identity is the only rule that never drops content, and it matches what the docs already promised.
flowchart LR A[incoming text] --> B[ranked candidates + exact-content lookup] B --> C{candidate text identical?} C -- yes --> D[merge: max confidence, union tags] C -- no --> E[insert new memory]Type of change
Release-note title
fix: merge only identical text in memory deduplicationTesting done
test/promotion/dedup.test.ts: added coverage for unrelated strong-rank candidates, whitespace/reorder/case/NFC-NFD variants, mixed candidate pages (only the exact row is touched, through both the repository and legacy paths), and a 300-memory SQLite regression that asserts the unrelated candidate ranks at -15 or stronger and still gets a fresh id. Existing merge-path tests now use identical text; insert-path tests keep nonidentical text.test/portable-knowledge.test.ts,test/storage/postgresql-lexical-search-repository.test.ts: fixtures rewritten to keep their metadata, retry-identity and rank-sign assertions.src/promotion/dedup.tsis at 100% statements, branches, functions and lines. Typecheck, lint and build pass.SqliteStorageBackendFactorywith the store route's exact arguments: the reproducer and four short texts from the issue each got a fresh id, the magnet memories were unchanged, and an exact re-store merged.Related issues
Closes #1727
Follow-ups: #1754 (passive Tier 3 gate admits one-off events on any lexical hit), #1755 (deprecate the now inert
dedupBm25Threshold).Notes for reviewers
src/promotion/dedup.ts: the change is theisDuplicateCandidatepredicate. Retrieval, exact lookup, serialization, canonical choice and the confidence/tag merge are unchanged, and the new rule is a strict subset of the old one.dedupBm25Thresholdis still accepted; its removal is tracked in Deprecate dedupBm25Threshold after exact-identity dedup #1755.