feat(integrations): Hermes memory provider core - #1915
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the MemPalace memory provider integration for Hermes, adding the file_conversation_exchange function, custom drawer ID generation, the MempalaceProvider class with background worker processing and tool dispatching, and a comprehensive test suite. The review feedback focuses on enhancing robustness and idiomatic Python usage, specifically suggesting to handle potential None elements in metadata scans, add type checks for parsed JSON configurations and keyword lists, utilize the context manager protocol for KnowledgeGraph instances, and wrap file system operations in try...except OSError blocks to prevent unexpected crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
This PR introduces the core of a Hermes MemoryProvider implementation backed by MemPalace, enabling Hermes to access MemPalace’s MCP tool surface and persist conversation turns without blocking the agent loop. It also adds a canonical conversation-exchange filing path and a new ID recipe to ensure live integration writes match the existing convo-miner metadata conventions.
Changes:
- Added
MempalaceProviderfor Hermes with lifecycle hooks, bounded background worker, and 27-tool MCP surface exposure. - Added
file_conversation_exchange()as the canonical write path for one verbatim exchange (shared by live integrations/backfills) plus a newmake_exchange_drawer_id()recipe. - Added a comprehensive test suite covering provider/tool behavior, lifecycle/worker behavior, wing routing, and canonical metadata expectations.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/test_hermes_integration.py | New end-to-end and unit coverage for Hermes provider surface, lifecycle, and persistence behavior. |
| mempalace/integrations/hermes/init.py | New Hermes provider implementation and tool schema/dispatch logic. |
| mempalace/ids.py | Adds make_exchange_drawer_id() to avoid upsert collisions for exchange drawers. |
| mempalace/convo_miner.py | Adds file_conversation_exchange() canonical exchange filing helper used by integrations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- _scan_metadatas: fetch cap+1 and compare len > cap, so a collection holding exactly STATUS_SCAN_LIMIT rows is no longer reported as truncated (the view is complete). Callers still get at most cap rows. - status/list_wings/list_rooms: tolerate None metadata entries from legacy palaces / raw writers instead of failing the tool call. - _match_wing_by_keywords: skip non-string keywords so a hand-edited wing_config.json can't break live turn filing. - file_conversation_exchange: extra_metadata can no longer overwrite canonical keys (matches the documented append-only contract), and wing/room are validated with sanitize_name — invalid names fall back to wing_general / conversations rather than dropping the turn, per the verbatim-first mandate. - Fix two stale docstrings left from the pre-split layout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for splitting this out. Overall this is a strong direction: keeping the Hermes provider inside the package makes sense for wheel installs, routing live writes through a shared I don’t think this is quite ready to merge yet, but it feels close. The main blockers I’d like fixed first are:
A couple of smaller follow-ups I noticed:
So: I’m positive on the shape of this PR, but I’d request changes on the two lifecycle/config-path issues before merge. |
|
Split off a fix for the two blockers @igorls raised above (duplicate filing between sync_turn()/on_session_end(), and passthrough tools bypassing the provider's configured palace_path), plus the two smaller follow-ups (on_memory_write default target, backup_paths()). Sent as raman325#2, stacked on this branch. |
|
Thanks for the thorough review @igorls — all four points were real, and all four are now addressed. Point by point: 1. Duplicate filing across the session lifecycle — fixed in e6bb4b8; the capture side split to a follow-up to keep this PR scoped. We took your second option and went a step further: The "file only what 2. Passthrough tools targeting a different palace — fixed in 73b3b31. Three parts:
3. Confirmed against the Hermes source: 4. We searched the Hermes tree: All 58 provider tests pass (3,273 across the suite), lint clean, CI green. 🤖 Generated with Claude Code |
|
Thanks for the detailed writeup, @raman325 - the direction you landed on is cleaner than what I had in raman325#2: making sync_turn the sole filing path (rather than de-duping across two paths) sidesteps the whole fragile correlation problem, and the env-bridge + collection_name unification for point 2 closes a gap I hadn't even caught in #2. Happy to help with the sync_turn/on_session_end correlation follow-up once it's ready for review —-the content-fingerprint approach you outlined sounds like the right way to avoid the exact-text-match trap. |
Live agent integrations and their backfills need to file one conversation exchange at a time, but hand-rolling the upsert leaves drawers without hall / entities / filed_at / extract_mode metadata — silently invisible to hallway traversal, entity search, and the since/before date filters. file_conversation_exchange() builds the same metadata the convo miner writes, and make_exchange_drawer_id() moves the ID construction into ids.py per its single-source-of-truth contract (full-content hash, no prefix collisions; filed_at keeps repeated exchanges distinct). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Hermes provider from feat/hermes-integration, split out per review on MemPalace#1684 — provider + tests only; backfill, the hermes install CLI, and docs follow in a stacked PR. Changes vs the original branch: - _file_turn routes through convo_miner.file_conversation_exchange() instead of a hand-rolled col.upsert, so live turns carry canonical drawer metadata and the ids.py ID recipe. - The backfill/live wing-routing parity test moves to the backfill PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- _scan_metadatas: fetch cap+1 and compare len > cap, so a collection holding exactly STATUS_SCAN_LIMIT rows is no longer reported as truncated (the view is complete). Callers still get at most cap rows. - status/list_wings/list_rooms: tolerate None metadata entries from legacy palaces / raw writers instead of failing the tool call. - _match_wing_by_keywords: skip non-string keywords so a hand-edited wing_config.json can't break live turn filing. - file_conversation_exchange: extra_metadata can no longer overwrite canonical keys (matches the documented append-only contract), and wing/room are validated with sanitize_name — invalid names fall back to wing_general / conversations rather than dropping the turn, per the verbatim-first mandate. - Fix two stale docstrings left from the pre-split layout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
on_session_end and on_pre_compress blind-re-filed the raw message list, duplicating every turn sync_turn had already stored — filed_at is hashed into the drawer id, so upserts cannot collapse the copies. Drop the re-filing (and the pre-compress hint that over-promised persistence); on_session_end keeps only the wake-up cache refresh. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The provider filed and searched self._palace_path while the mcp_server passthrough tools resolved mempalace's global config — a custom Hermes palace_path searched one palace while drawer CRUD, duplicate checks, and tunnels wrote another. Publish the resolved palace to MEMPALACE_PALACE_PATH (mcp_server's own --palace mechanism) with ownership tracking so a stale bridge never outranks edited config and a user-set env var is never touched. The KG tools become native handlers: mcp_server resolves its KG from DEFAULT_KG_PATH unless its own CLI flag was given, which no env bridge can influence. collection_name and the unset-palace default now defer to MempalaceConfig — the same single chain the MCP server itself uses. Also document that hermes backup does not cover ~/.mempalace (no ABC hook exists for contributing external paths). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…raph
Hermes' memory tool defaults to target="memory" (the agent's own
notes); filtering on_memory_write to target == "user" silently dropped
the majority of writes. Mirror both targets under distinct subjects —
user→asserted for user facts, hermes→noted for agent notes — so
kg_query("user") never surfaces environment quirks.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9edcec2 to
fc3beb7
Compare
|
Hey @igorls — just checking in on this one. All four review blockers were addressed back on July 6 (duplicate filing, passthrough tools bypassing palace_path, sync_turn as sole filing path, KG mirroring). The PR is mergeable and has been sitting for two weeks. Is there anything else needed before it can land? Happy to rebase or adjust if something changed on develop in the meantime. |
|
Thanks for pushing this stack forward. I maintain a separately developed native Hermes/MemPalace provider that is running in production on Hermes 0.20.0 and MemPalace 3.6.0 against authenticated Qdrant, with more than 25,000 drawers. I reviewed #1915, #1941, and #1942. I do not intend to open a competing Hermes provider; this series is clearly the right upstream integration point. Our independently tested line may be useful for focused follow-ups in two areas:
The current upstream stack's use of @raman325 @igorls, which sequencing would you prefer?
I can also start with a compact backend contract test so storage abstraction expectations are agreed before implementation. |
* develop: (45 commits) ci: retry transient Chroma reader initialization failure fix(encoding): recover undefined CP1252 continuation bytes fix(encoding): make repair conservative and reversible fix(windows): add legacy encoding repair tool feat(searcher): wire i18n stop words into BM25 tokenizer (MemPalace#973) test: expect dry_run=False on rebuild-index alias call fix(repair): honor --dry-run for repair --mode from-sqlite fix(hooks): ingest only the active transcript fix(embedding): remap unsupported EmbeddingGemma token IDs fix(convos): honor mined state during dry runs fix: harden release polish for bot findings and search errors chore(release): 3.7.0 fix: reopen immutable readers and clear identity on promote fix: address backend ownership edge cases fix: serialize SQLite writes before palace lease fix: take mine-lock before archive in repair --mode from-sqlite fix(chroma): adopt chromadb's own HNSW write defaults fix: retry transient MCP ownership failures fix: address MCP ownership review feedback fix: address single-writer review feedback ... # Conflicts: # mempalace/convo_miner.py # tests/test_convo_miner.py
|
I'd prefer to land my stack and to queue up any additional changes for after as it would reduce merge conflict churn, but I will leave it to your and Igor's discretion! |
igorls
left a comment
There was a problem hiding this comment.
Wave 2 for 3.7.0: LGTM after review. Merging into develop for the release train.
MemoryStack/Layer1 opened a second PersistentClient on the same palace while the Hermes provider already held one for live filing. Concurrent access corrupted local Chroma SQLite (disk I/O / Failed to get segments) and failed CI on develop after MemPalace#1915. Wake-up L1 now scans the long-lived collection under the collection lock, and filing holds that lock for the full upsert. Also rewrite the RFC 001 section-4.4 docstring to avoid the internal §N jargon guard.
Per @igorls I am splitting #1684 into two separate PRs and this is the first — the provider core and tests, as recommended. I attempted to resolve all of your blockers but may have missed e.g. some helpers that are available for use — I gave Fable a good run at it though!
What this adds
A MemPalace memory provider for Hermes (NousResearch/hermes-agent #6323): a Python class implementing Hermes'
MemoryProviderABC, exposing the full 27-tool MCP surface, with lifecycle hooks (sync_turn,on_session_end,on_pre_compress, session-switch bookkeeping), a cron/flush context guard so system-generated turns don't poison the palace, a bounded background worker so filing never blocks the conversation, and an AAAK wake-up cache.The
mempalace hermes installcommand, session backfill, and docs land in a stacked follow-up PR so this one stays reviewable.Beyond the provider itself, this touches two core modules:
convo_miner.py: newfile_conversation_exchange()— a canonical write path for filing one verbatim exchange as a drawer, carrying the same metadata the convo miner writes (hall,entities,authored_at,ingest_mode,extract_mode,normalize_version,id_recipe). This exists so live integration writes and their backfills route through one implementation instead of two hand-rolled copies.ids.py: newmake_exchange_drawer_id()alongside the existing recipes.How the review blockers were addressed
develop— rebuilt on the currentdeveloptip; no conflicts.ChromaBackend.get_or_create_collection()(embedding function stays centralized, no dimension drift; writes inheritmine_palace_lockviaChromaCollection). Live turn filing routes through the newfile_conversation_exchange()instead of a hand-rolledcol.upsert. Tool calls delegate tomempalace.mcp_server.tool_*entry points rather than reimplementing them.file_conversation_exchange()is the single write path both will share; the stacked backfill PR routes through it and has a regression test pinning wing-routing parity.status/list_wings/list_roomsare capped (STATUS_SCAN_LIMIT = 5000) with structured truncation fields (truncated, scan coverage) so the model knows when a breakdown is partial.mempalace/integrations/hermes/so it ships automatically viapackages = ["mempalace"]; nothing depends on top-level files being adjacent to the installed package.Why this lives at
mempalace/integrations/hermes/and not top-levelintegrations/hermes/Top-level
integrations/openclaw/exists and the natural read is "put it next to that one." The asymmetry comes from how each integration is consumed at runtime:SKILL.mdthat Claude Code reads from the source repo. It doesn't need to be importable Python and it doesn't need to ship in the wheel.MemoryProviderABC — Hermes' plugin loader imports the module and calls methods on it.mempalace hermes installhas to find the source files at runtime after a wheel install in order to copy them into~/.hermes/plugins/mempalace/(the install command itself lands in the follow-up PR).Tests
42 new tests in
tests/test_hermes_integration.pycovering the tool surface, lifecycle/worker behavior, wing routing, canonical drawer metadata, and the ChromaBackend round-trip that regression-checks the dimension-mismatch bug from prior in-tree Hermes attempts.Backfill + install + docs PR to follow, stacked on this branch.
Behavior note: wing/room validation falls back instead of erroring
file_conversation_exchange()validateswing/roomwith the samesanitize_namerules the MCP write tools apply (blocks/,.., null bytes, over-length names). Unlike the MCP tools — which return an error to their caller — this path falls back on an invalid name: wing →wing_general, room →conversations, with a warning logged. Rationale: this function files live turns, and dropping a turn over a config typo would violate the verbatim / 100%-recall promise. Misrouted-but-recallable beats gone. (Raised in review; documented in the integration README in the follow-up PR.)