Skip to content

fix(memory): first-person rescue, cap 3->6, semantic dedup - #6

Merged
GurneeshBudhiraja merged 2 commits into
mainfrom
dev/aman/chat-memory-fixes
Jul 25, 2026
Merged

fix(memory): first-person rescue, cap 3->6, semantic dedup#6
GurneeshBudhiraja merged 2 commits into
mainfrom
dev/aman/chat-memory-fixes

Conversation

@aman-a-shah

Copy link
Copy Markdown

update_memory tool hardening, mirroring the web-app auto-memory fixes (knowidea-web-app NousResearch#468) and paired with hermes NousResearch#72 (which bumps the submodule pointer to this commit).

  • _normalize_first_person rescues first-person facts ("I'm the CFO" → "User is the CFO") before the assistant-voice shape filter, instead of the filter silently dropping them. Mirror of the TS normalizeFirstPerson.
  • _MAX_APPEND_BULLETS 3 → 6 — the char ceiling is the real bound; a tight cap truncated multi-fact turns.
  • _semantic_novel_bullets — fail-open dedup pass (MEMORY_SEMANTIC_DEDUP=0 to disable) that drops paraphrases the lexical key misses, on the append path.

Validator parity with the web app is pinned by golden-case suites in both languages (hermes/tests/test_output_contracts.py + agent-memory-helpers.test.ts).

Base is dev/gurneesh/agent-memory because hermes main currently pins the submodule at that branch's tip (d0fda0f); this keeps the pointer on a permanent branch so hermes NousResearch#72's reference doesn't dangle.

🤖 Generated with Claude Code

update_memory tool hardening, mirroring the web-app auto-memory fixes:
- _normalize_first_person rescues first-person facts ("I'm the CFO" ->
  "User is the CFO") before the assistant-voice shape filter, instead of the
  filter silently dropping them. Mirror of the TS normalizeFirstPerson.
- _MAX_APPEND_BULLETS 3 -> 6 (the char ceiling is the real bound; a tight cap
  truncated multi-fact turns).
- _semantic_novel_bullets: fail-open dedup pass (MEMORY_SEMANTIC_DEDUP=0 to
  disable) drops paraphrases the lexical key misses, on the append path.

Validator parity with the web app is pinned by golden-case suites in both
languages (hermes/tests/test_output_contracts.py + agent-memory-helpers.test.ts).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@aman-a-shah
aman-a-shah force-pushed the dev/aman/chat-memory-fixes branch from 6b362d2 to ba956ea Compare July 8, 2026 19:45
@aman-a-shah
aman-a-shah changed the base branch from dev/gurneesh/agent-memory to main July 8, 2026 19:45
@aman-a-shah

Copy link
Copy Markdown
Author

Code review — approve; merge as-is, no rebase needed ✅

Despite being 17 days old and 10 commits behind main, the branch is MERGEABLE/CLEAN and semantically coherent: the only post-branch change to the same file on main (0422da11, the contrastive-examples prompt gate from the over-saving fix) touches a disjoint region and is complementary — it gates what the agent tries to save; this PR shapes/dedups what gets written. The web app already ships the identical trio in production (agent-memory-service.ts: cap 6, normalizeFirstPerson, semanticNovelBullets), so merging restores cross-repo parity.

Merging this also fixes a standing mystery: the baseline failure test_first_person_facts_rescued_to_third_person in hermes exists because that parity test already landed in hermes main while this PR sat unmerged — hermes main's submodule pin (437c13b9) has none of this PR's symbols. All four memory-test assertion sets were run against this branch's file and pass; merge + pin bump turns the red baseline test green.

Findings (real, but faithful mirrors of already-shipped web-app behavior — follow-up in BOTH repos, not blockers here)

  1. Medium — assistant-narration openers get rescued into fake user facts (tools/user_notes_tool.py:476-503). The I'll/I've/I'm rewrites partially defeat the assistant-voice guard whose documented purpose was a live incident: verified "I'll create a bar chart with the totals" → saved as "User will create a bar chart with the totals". The original incident shape is still filtered, and the rewrite list is byte-identical to the web-app's production FIRST_PERSON_REWRITES — but the I'll/I've/I'm entries specifically trade guard protection for identity-fact rescue; the other openers (want/prefer/need/my) carry the signal with far less risk.
  2. Medium — semantic dedup can silently drop genuinely new facts (:522-593). A successful-but-strict classifier reply discards refinements (existing "works in finance leadership" swallows candidate "is the CFO") with no log of what was dropped; and if the model returns index strings (["0","1"]), the isinstance(int) filter empties the keep-set and ALL candidates drop — a fail-closed path inside a documented fail-open function. Coercing via int(n) + logging dropped bullets fixes both — but the same gaps exist in the TS mirror, so fix both repos together or the pinned parity suite breaks.
  3. Low — "I do not" (unelided) gets no rescue while "I don't" does (:489-490) — same gap in the TS mirror; fix both or neither.
  4. Low — cap 3→6 amplifies anything the filters miss but doesn't contradict the later durability-gate work (disjoint mechanisms; web-app already ships 6; _MAX_MEMORY_CHARS=10000 is the real bound).
  5. Low — up-to-6s synchronous classifier call added to memory-saving turns (httpx.Client(timeout=6), :572). Fail-open, one cheap classifier call; note from src.constants.models import ... resolves only inside the hermes deploy — standalone, dedup silently never runs (consistent with the repo's established from src. convention).

Recommendation: merge (merge-commit), bump the hermes submodule pin, and track findings 1-3 as a cross-repo follow-up issue covering both this file and agent-memory-service.ts.

🤖 Generated with Claude Code

@aman-a-shah

Copy link
Copy Markdown
Author

Reopened + re-verified after sync with main ✅

Merged current main (post #10/#11) into the branch: zero conflicts (5e07b35). Post-merge verification:

  • Composes with fix(memory): reject one-off/topic content in update_memory tool prompt #8's over-saving prompt gate: every line 0422da11 added to user_notes_tool.py is present verbatim alongside this PR's _normalize_first_person / _semantic_novel_bullets / cap 6 — the two changes gate different stages (what the agent tries to save vs how it's shaped/deduped) and coexist cleanly.
  • hermes-agent suites: 228 passed (run_agent 195, usage_listener 7, cost_budget 18, memory_consult_scope 8).
  • hermes parity suite (tests/test_output_contracts.py, run against a hermes main worktree with the submodule pointed at this merge): 26/26 pass — including test_first_person_facts_rescued_to_third_person, which fails against the current main pin (verified both ways). Merging this PR + a pin bump is what turns hermes main's baseline green.

Original review verdict stands (approve, merge as merge commit); the two medium findings remain deliberately deferred to the cross-repo follow-up KNOWIDEA-Tech/hermes#103 since the web app ships identical behavior in production.

After merging: hermes needs one more submodule pin bump to pick this up (the pin currently at 414da8a predates it).

🤖 Generated with Claude Code

@GurneeshBudhiraja
GurneeshBudhiraja merged commit 06cc2e9 into main Jul 25, 2026
2 checks passed
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.

2 participants