Skip to content

fix(memory): guard against duplicate-entry trap#5

Merged
AIalliAI merged 1 commit into
mainfrom
cherry-pick/60090
Jul 7, 2026
Merged

fix(memory): guard against duplicate-entry trap#5
AIalliAI merged 1 commit into
mainfrom
cherry-pick/60090

Conversation

@AIalliAI

@AIalliAI AIalliAI commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Cherry-pick of PR NousResearch#60090 by Vissirexa.

Fixes: NousResearch#60089
Pipeline: TRIAGE DONE | INTEGRATOR DONE | VERIFY PASS | REVIEWER PENDING | SECURITY PENDING | OPSEC PENDING

When old_text substring-matches two or more entries with different
content, replace() correctly refuses with "Multiple entries matched" —
but if the model's intended replacement text is already present
verbatim as a separate entry, the model typically falls back to add()
and creates a duplicate of that entry. Detect this case in both
MemoryStore.replace() and the memory_tool() dispatcher pre-validation,
and return a targeted hint (use a batch operations call, or narrow
old_text) instead of the generic ambiguous-match error. Exact-string
check only, no semantic matching — a narrow defensive guard on top of
the existing exact-duplicate-add rejection.

Co-authored-by: AIalliAI <285906080+AIalliAI@users.noreply.github.com>

@AIalliAI AIalliAI left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Hermes Agent Code Review — VERDICT: APPROVED ✅

PR: #5 — cherry-pick of NousResearch#60090 by Vissirexa
Files: tools/memory_tool.py (+37), tests/tools/test_memory_tool.py (+43)


What This Fixes

When MemoryStore.replace() hits an ambiguous match (old_text matches multiple entries with different content), the model gets "Multiple entries matched" → falls back to add() → creates a duplicate of an entry that already says what the model wanted to write. This is a repeatable tool-misuse trap.

What Changed

Two call sites, both exact-string-identity checks, scoped to action="replace":

  1. MemoryStore.replace() (line 429): When multiple unique entries match AND the replacement text already exists verbatim as a separate entry, return a targeted error with actionable guidance instead of the generic ambiguous-match message. Includes matches: previews for visibility.

  2. memory_tool() dispatcher (line 1038): Same pre-validation runs before the approval gate for earlier, equally targeted feedback.

✅ Correctness

  • The guard only fires when len(unique_texts) > 1 AND new_content in entries — exactly the trap scenario
  • When replacement text is genuinely new, the original generic error is preserved (verified by test)
  • Edge cases: identical-duplicate matches fall through (safe to operate on first), single match falls through (normal replace path)
  • Edge case where new_content equals one of the matched entries: guard fires correctly, guiding to batch operations
  • Comparison uses stripped strings consistently on both sides
  • Dispatcher check is read-only, no lock needed; benign TOCTOU — store-level catches any race under lock

✅ Tests

All 86 tests pass (83 existing + 3 new). Adjacent files 4 pass.

New tests:

  • test_replace_ambiguous_match_with_duplicate_replacement_gets_targeted_hint — store guard fires with targeted message ✅
  • test_replace_ambiguous_match_without_duplicate_keeps_generic_error — regression: original error unchanged ✅
  • test_replace_dispatcher_catches_duplicate_trap_before_store — dispatcher guard fires ✅

💡 Suggestion (non-blocking)

The dispatcher pre-validation (line 1038) returns a targeted error but does not include the matches key with previews of the matched entries — unlike the store-level guard which includes matches: previews. For consistency and richer model context, consider passing matched entries through store._previews() and including them here too.

Verdict: APPROVED

The fix is correct, well-targeted, non-invasive, and does exactly what it claims. No regressions, good test coverage. Ready to merge.

@AIalliAI
AIalliAI merged commit 80ee0af into main Jul 7, 2026
27 of 30 checks passed
AIalliAI pushed a commit that referenced this pull request Jul 11, 2026
…check (#5)

The sync _session_has_compression_in_flight sat on the message hot path
and blocked the event loop twice: under session_store._lock during
_ensure_loaded_locked (JSON read) and via db.get_compression_lock_holder
(SQLite SELECT). Async-ify the method and offload both sources via
asyncio.to_thread; await the call site in _handle_active_session_busy_message.
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.

[Bug]: memory replace → "Multiple entries matched" → fallback add creates duplicate entries with no signal

2 participants