fix(memory): actually update on near-duplicate dedup (#34)#36
Open
chiruno-9 wants to merge 2 commits into
Open
fix(memory): actually update on near-duplicate dedup (#34)#36chiruno-9 wants to merge 2 commits into
chiruno-9 wants to merge 2 commits into
Conversation
When ``_deduplicate_and_store_facts`` detected a fact whose cosine
similarity to an existing memory exceeded the threshold, it appended the
existing id to ``memories_updated`` but performed no update — the new
content was silently dropped. For factual *updates* ("I work at Google"
→ "I just joined Microsoft"), the two short sentences embed close enough
to be collapsed, so the system retained stale information indefinitely.
Now, on a near-duplicate:
* generate an embedding for the new fact,
* call ``store.update_memory`` to overwrite content + vector + metadata
+ importance on the existing row (timestamps/ids preserved),
* if the store update fails, fall back to inserting so the fact is
never silently lost.
Also expose the threshold as a tunable. The fixed 0.85 was
embedding-model-dependent and would over-merge short paraphrases on some
models. Add ``MemoryManager(dedup_threshold=...)`` and a
``CONFIG.memory_dedup_threshold`` knob so users can calibrate without
patching code. Backwards compatible (default unchanged).
The richer follow-ups noted in the issue (LLM-assisted dedup, adaptive
thresholds, entity-graph merging) are intentionally out of scope here —
this PR fixes the data-loss bug and unblocks per-model tuning.
Refs cyzus#34
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a data-loss bug in the backend memory deduplication flow by ensuring near-duplicate facts actually update the existing memory entry (instead of being silently dropped), and makes the dedup similarity threshold configurable via app config and MemoryManager initialization.
Changes:
- Update near-duplicate handling to call
store.update_memory(...), with fallback to insert if the update fails. - Make the dedup similarity threshold configurable (
ConfigModel.memory_dedup_threshold→MemoryManager(dedup_threshold=...)). - Add focused tests covering update behavior, threshold configurability, and update-failure fallback.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/suzent/memory/manager.py |
Implements near-duplicate update behavior and adds an instance-level configurable dedup threshold. |
src/suzent/memory/lifecycle.py |
Wires the new config value into MemoryManager initialization. |
src/suzent/config/__init__.py |
Adds memory_dedup_threshold to the config model. |
tests/memory/test_dedup_update.py |
Adds regression + behavior tests for near-duplicate update/insert logic and threshold handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+529
to
+533
| existing_id = str(similar[0]["id"]) | ||
| embedding = await self.embedding_gen.generate(fact.content) | ||
| updated = await self.store.update_memory( | ||
| memory_id=existing_id, | ||
| content=fact.content, |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
+531
to
+537
| updated = await self.store.update_memory( | ||
| memory_id=existing_id, | ||
| content=fact.content, | ||
| embedding=embedding, | ||
| metadata=metadata, | ||
| importance=fact.importance, | ||
| ) |
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.
Summary
Fixes the data-loss bug described in #34. In
MemoryManager._deduplicate_and_store_facts, when a newly extracted fact's cosine similarity to an existing memory exceededDEDUPLICATION_SIMILARITY_THRESHOLD(0.85), the code appended the existing id toresult.memories_updatedbut performed no update — the new content was silently dropped.The most damaging case is factual updates: "I work at Google" → "I just joined Microsoft" embed close enough on short sentences that the newer fact gets collapsed into the older one and discarded. The system then retains stale information indefinitely.
Changes
src/suzent/memory/manager.py— the actual fixstore.update_memory(memory_id, content=..., embedding=..., metadata=..., importance=...). The store layer preserves the original row's timestamps; only content/vector/metadata/importance change.update_memoryreports failure, fall back to inserting a fresh row so the fact is never silently lost.DEDUPLICATION_SIMILARITY_THRESHOLD→DEFAULT_DEDUPLICATION_SIMILARITY_THRESHOLDto reflect that it's now a default, not a hard-coded global. Per-instance threshold lives onself.dedup_threshold.Make the threshold tunable (issue's "reason 3": threshold is embedding-model-dependent)
MemoryManager.__init__acceptsdedup_threshold: Optional[float](default = constant).ConfigModel.memory_dedup_threshold: Optional[float] = Noneinsrc/suzent/config/__init__.py.memory/lifecycle.pywiresCONFIG.memory_dedup_thresholdthrough on init.Backwards compatible: if neither callers nor
default.yamlset the new field, behavior is identical to before — except that near-duplicates are now actually updated instead of silently dropped.Tests
tests/memory/test_dedup_update.py— 8 tests, all backend-only with mocked I/O:test_near_duplicate_actually_updates_existing_memory— pins the bug:store.update_memoryis called with the new content + freshly generated embedding + new importance.test_below_threshold_inserts_new_memory— non-duplicate path unchanged.test_no_similar_memories_inserts_new_memory— empty search result path unchanged.test_threshold_is_configurable_per_instance— a tighter threshold (0.99) lets a 0.95 hit be treated as new.test_update_failure_falls_back_to_insert— never lose a fact silently if the store rejects the update.test_constructor_accepts_explicit_threshold[None|0.7|0.92]— public API honors the new arg.Out of scope (deliberately deferred)
The issue's "possible future paths" — LLM-assisted dedup, adaptive thresholds, entity-graph merging — are all bigger directional changes. They're not needed to stop the data loss and would dilute review. Happy to take any of them on as follow-up PRs if you'd like.
Test plan
uv run pytest tests/memory/test_dedup_update.py— 8/8 pass.uv run pytest tests/memory/— 22/22 pass (no regression).uv run ruff checkon changed files — clean.