fix(C20): align hermes_plugin _triple_store cache with active memory#56
Merged
Conversation
THE BUG
hermes_plugin/__init__.py held two module-level caches that should
always point at the same SQLite database — `_memory_instance` and
`_triple_store`. Two issues let them drift apart:
1. `_get_memory(session_id)` rebound `_memory_instance` whenever
session_id changed, but never reset `_triple_store`. After a session
or bank or data-dir switch, triples wrote to the original DB while
memories wrote to the new DB. Cross-bank pollution; silent failure.
2. `_get_triples()` called `_get_memory()` with no arguments, which
defaults session_id to HERMES_SESSION_ID env (or "hermes_default")
and can rebind `_memory_instance` to a DIFFERENT session than the
active caller. Even on a fresh DB, this routed triple writes to
the env-default session's DB, regardless of the explicit session
the caller had set up.
A debug repro:
hermes_plugin._get_memory("session_a") → memory at /tmp/a.db
hermes_plugin._get_triples() → triples at ~/.hermes/.../mnemosyne.db
→ write goes to env-default DB, not /tmp/a.db.
CHANGES (5 lines in hermes_plugin/__init__.py)
1. `_get_memory()`: when the rebuild branch fires, also `_triple_store = None`
so the next `_get_triples()` rebuilds aligned.
2. `_get_triples()`: read `_memory_instance` directly when present (instead
of calling `_get_memory()` and triggering a possible rebind). Add a
defensive `db_path` mismatch check that rebuilds the cache if memory
ever ends up at a different DB than the cached store.
REGRESSION TESTS
tests/test_hermes_plugin_session.py — 2 tests:
1. `test_get_triples_follows_active_memory_after_session_switch`:
monkeypatches Mnemosyne so each session_id resolves to a distinct
db_path; asserts that after a session change, _get_triples() returns
a TripleStore aligned with the new memory. RED before fix, GREEN
after.
2. `test_triple_writes_route_to_new_db_after_session_switch`: end-to-end
through the public `mnemosyne_triple_add` tool. Writes one triple in
session_a, switches to session_b, writes another. Asserts each triple
landed in its correct DB and did not leak across.
PRE-EXISTING TEST ORDERING NOTE
test_plugins.py followed by test_hermes_llm_adapter.py causes 14 failures
in test_hermes_llm_adapter due to plugin-manager state leakage. Not
related to this fix; alphabetical pytest discovery puts them in the
non-leaking order (llm_adapter first), so the full suite passes.
VERIFICATION
tests/test_hermes_plugin_session.py: 2 passed
full suite excluding LLM-dependent tests: 444 passed (+2 from new tests)
LEDGER
.hermes/ledger/memory-contract.md C20 row will move to PR_OPEN when
this lands.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CROSS-MODEL ADVERSARIAL REVIEW Both Claude and Codex flagged the same primary issue: my initial fix bypassed _get_memory() inside _get_triples() to avoid an unrelated session rebind, but that broke the env-change scenario. If HERMES_SESSION_ID env changed AFTER the first _get_memory(session) call but BEFORE any explicit memory call in the new session, the next triple-only operation still routed to the OLD session's DB. Codex requested changes; Claude noted the same regression as Finding 8. CHANGES 1. _get_triples() — restore _get_memory() call Reverts the over-engineered _memory_instance bypass from commit e59d9dd. Calling _get_memory() means env changes trigger the same session-rebind logic for triple ops as for memory ops. The defensive Path() db_path mismatch check stays as cheap insurance for any future code path that might mutate memory's db_path. 2. _get_memory() — build before assign (constructor-failure protection) Both reviewers flagged: previous code set _current_session_id = X BEFORE Mnemosyne(...) succeeded. A construction failure (DB locked, embedding init error, etc.) would leave _current_session_id pointing at a session whose memory never finished initializing. Next call sees session match, returns the stale prior _memory_instance. Fix: build into a local first, only assign to globals after the constructor returns. _triple_store reset (the C20 fix) still happens on the success path. NEW REGRESSION TEST tests/test_hermes_plugin_session.py:: test_get_triples_honors_env_change_without_explicit_memory_call Locks in the env-honoring behavior the reviewers warned was at risk. Sets env to session_a, calls _get_memory(session_a), captures _get_triples() at db_a. Then changes env to session_b WITHOUT calling _get_memory(session_b). Asserts the next _get_triples() call routes to db_b. Would fail under the bypass design from commit e59d9dd. DEFERRED (per /review notes, pre-existing) - TOCTOU race on module globals (GIL-mitigated, hits all multi-threaded usage of the plugin singleton; out of scope for this fix). - Shared TripleStore connection across threads with check_same_thread=False (TripleStore lacks the thread-local pattern Mnemosyne uses; out of scope). VERIFICATION tests/test_hermes_plugin_session.py: 3 passed (was 2, +1 env-honoring) full suite excluding LLM-dependent tests: 445 passed Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
hermes_plugin/__init__.pyheld two module-level singletons —_memory_instance(the Mnemosyne wrapper) and_triple_store(the TripleStore that backs the knowledge-graph triple tools) — that should always point at the same SQLite database. The session-change invalidation logic existed for_memory_instancebut was never extended to_triple_store. After a session or bank switch, memory writes routed to the new DB while triple writes silently kept going to the old DB._triple_store = Nonewhenever_memory_instanceis rebound, plus a defensivePath(db_path)mismatch check in_get_triples()as cheap insurance, plus a build-before-assign in_get_memory()so a partial constructor failure can't poison_current_session_idand leave the cache in an inconsistent state.mnemosyne_triple_addtool, and an env-change scenario that locks in env-honoring behavior so an earlier over-engineered version of this fix can't silently come back.The bug in detail
hermes_plugin/__init__.pyexposes the Mnemosyne knowledge graph through three module-level globals:_get_memory(session_id)rebinds_memory_instancewhenever the requestedsession_iddiffers from_current_session_id:_get_triples()was a one-shot lazy initializer:The first call captures
mem.db_pathinto the cachedTripleStoreand the cache is never invalidated. Every later_get_triples()returns the same store regardless of where memory has moved to.What the user actually sees go wrong
Take a long-running Hermes plugin process that handles a session in the default bank, then is asked to switch to a project-specific bank for a different conversation context:
A triple written through the
mnemosyne_triple_addtool while inproject_aends up in the default bank's database. The user sees no error.mnemosyne_triple_queryagainst project_a returns nothing for that triple. The default bank quietly accumulates project_a's data for the rest of the process lifetime.The damage compounds over a long-running plugin process: every session/bank switch widens the gap between where memory thinks it is and where triples are actually being written. The data lives, but in the wrong place. Restoring it after the fact would require manual SQL surgery.
Why it slipped through
The cache-invalidation logic was added when
_memory_instancewas first introduced as a session-aware singleton._triple_storewas added later but the invalidation pattern wasn't extended. There was no test for "switch session, write triple, verify destination" — most tests instantiateMnemosynedirectly with an explicitdb_pathand never exercise the plugin singletons.tests/conftest.pyresets all three globals between tests as a hygiene fixture, which actively masks the bug in CI: it tests the cold-start path, never the session-switch path.This is the same anti-pattern as C26 (internals tested, public-surface wrapper untested). The plugin singleton state has practically no direct test coverage. PR adds the first.
The fix
Two small changes in
hermes_plugin/__init__.py:_get_memory()— reset the cache on rebuild + harden against partial init failures:The build-before-assign is independent of the C20 cache fix but addresses a related fragility flagged in cross-model review: previously,
_current_session_idadvanced before the newMnemosyne(...)finished constructing. If construction raised mid-way (locked DB, embedding model fail, etc.),_current_session_idwould already point at the new session while_memory_instancestayed on the prior one, and_triple_store = Nonenever executed. The next call would see session-match and return the stale memory plus stale triple store — silently wrong. Building into a local variable first means the globals only update if the constructor returns successfully._get_triples()— defensive check, env-honoring:The function still calls
_get_memory()(which readsHERMES_SESSION_IDenv when called with no arguments), so an env-change between calls still triggers the same session-rebind logic for triple operations as for memory operations. The addedPath()mismatch check is cheap insurance against any future code path that might mutatemem.db_pathwithout going through_get_memory()(none today, but defensive).Options considered, and what got reverted
The first commit on this branch took a more aggressive approach: have
_get_triples()read_memory_instancedirectly to avoid calling_get_memory()at all. That came from a debugging detour where I chased a test that called_get_memory("session_a")without settingHERMES_SESSION_IDenv to match — the internal_get_memory()no-arg call defaulted to env, didn't match the explicit session, and rebound away.Both Claude and Codex adversarial reviewers caught that the bypass introduced a real regression: if
HERMES_SESSION_IDenv changes after a session has been bound but before any explicit_get_memory(new_session)call, a triple-only operation would still route to the old session's DB. The bypass had removed the env-honoring path that the original code (and Hermes's actual session-change flow) relied on.The second commit reverts the bypass and updates the test to set env consistently with the explicit session — matching how Hermes coordinates env and explicit calls in production. A new regression test (
test_get_triples_honors_env_change_without_explicit_memory_call) explicitly locks in the env-honoring behavior so a future regression of this design choice would surface in CI.Tests
tests/test_hermes_plugin_session.py— 3 regression tests:test_get_triples_follows_active_memory_after_session_switch— monkeypatchesMnemosyneso each session_id resolves to a distinctdb_path. Asserts that after a session change,_get_triples()returns a TripleStore aligned with the new memory's db_path. RED before the fix, GREEN after.test_triple_writes_route_to_new_db_after_session_switch— end-to-end through the publicmnemosyne_triple_addtool. Writes one triple in session_a, switches to session_b, writes another. Reads each DB directly (bypassing the plugin cache) and asserts each triple landed in its correct DB and did not leak across.test_get_triples_honors_env_change_without_explicit_memory_call— locks in env-honoring. Sets env to session_a, calls_get_memory(session_a), captures_get_triples()at db_a. Then changes env to session_b WITHOUT calling_get_memory(session_b). Asserts the next_get_triples()call routes to db_b. Would fail under the bypass design from the first commit.Deferred
Both adversarial reviewers flagged pre-existing concerns that are out of scope for this fix:
if _memory_instance is None or _current_session_id != session_id: _memory_instance = ...) is racy if multiple threads call_get_memory()concurrently. The GIL mitigates the worst cases but doesn't eliminate them. Pre-existing in the singleton design.TripleStore.connis shared across threads withcheck_same_thread=False. SQLite-Python documents thatcheck_same_thread=Falsedoes not provide concurrency safety; the caller is expected to serialize. Pre-existing design choice; out of scope here.If the plugin is single-threaded in practice (Hermes's typical deployment), neither hits in production. They're worth a follow-up if a future deployment introduces concurrency.
Test plan
uv run pytest tests/test_hermes_plugin_session.py -q(3 passed locally)uv run pytest -q --ignore=tests/test_local_llm.py --ignore=tests/test_llm_backends.py(445 passed locally)mnemosyne_triple_add, verify it lands in the new bank's DB (not the old one).HERMES_SESSION_IDenv mid-process, call a triple tool without first calling a memory tool, verify the triple lands in the new session's DB.Verification
```
tests/test_hermes_plugin_session.py: 3 passed (new file, all RED before fix)
full suite excluding LLM-dependent tests: 445 passed
```
Note on the unrelated
test_hermes_llm_adapter.pyfailures: those manifest only whentest_plugins.pyruns first (state leakage in the PluginManager registry, unrelated to this fix). Alphabetical pytest discovery putstest_hermes_llm_adapterbeforetest_plugins, so the full suite passes. Pre-existing test isolation issue, not introduced by this branch.