diff --git a/mempalace/integrations/hermes/__init__.py b/mempalace/integrations/hermes/__init__.py index 541bf8c273..60651342f7 100644 --- a/mempalace/integrations/hermes/__init__.py +++ b/mempalace/integrations/hermes/__init__.py @@ -71,6 +71,13 @@ class MemoryProvider: # type: ignore[no-redef] logger = logging.getLogger("mempalace.hermes") +# ``mempalace.mcp_server`` resolves its palace path from ``MEMPALACE_PALACE_PATH`` +# / its own ``~/.mempalace/config.json`` — a process-global, not this provider's +# ``self._palace_path``. Guards the env-var pin in ``_dispatch_mcp_passthrough`` +# so two provider instances (e.g. two Hermes profiles with different configured +# palaces) can't race each other's passthrough calls. +_PASSTHROUGH_ENV_LOCK = threading.Lock() + def _match_wing_by_keywords(text: str, wing_config: Dict[str, Any]) -> str: """Return the first wing whose keywords match a whole word in ``text``. @@ -499,6 +506,12 @@ def __init__(self) -> None: self._session_id: str = "" self._hermes_home: str = "" self._turn_count = 0 + # Turns already filed live via sync_turn() this session. on_session_end + # receives the full transcript and would otherwise re-mine (and + # re-file, as a *new* drawer — filed_at makes ids non-idempotent) every + # turn sync_turn already persisted. Skipped in _mine_session below. + self._synced_turns = 0 + self._synced_turns_lock = threading.Lock() # ChromaDB access through mempalace's own backend (matches embedding # function, fixes the dim-mismatch bug from prior PRs). @@ -683,6 +696,7 @@ def sync_turn( *, session_id: str = "", messages: Optional[List[Dict[str, Any]]] = None, + _count_synced: bool = True, ) -> None: if self._cron_skipped or not self._initialized: return @@ -701,6 +715,12 @@ def sync_turn( }, ) ) + # Delegation turns (see on_delegation) are synthetic bookkeeping + # that never appears in the ``messages`` transcript on_session_end + # receives, so they must not count against the skip below. + if _count_synced: + with self._synced_turns_lock: + self._synced_turns += 1 except queue.Full: # Loud, not silent: the verbatim invariant is what mempalace sells. # If the queue saturates we want operators to see it. @@ -721,11 +741,17 @@ def on_session_end(self, messages: List[Dict[str, Any]]) -> None: # tasks that can never drain. if self._cron_skipped or not self._initialized: return + with self._synced_turns_lock: + already_synced, self._synced_turns = self._synced_turns, 0 try: self._worker_queue.put_nowait( ( "session_end", - {"messages": list(messages or []), "session_id": self._session_id}, + { + "messages": list(messages or []), + "session_id": self._session_id, + "already_synced": already_synced, + }, ) ) except queue.Full: @@ -754,6 +780,8 @@ def on_session_switch( self._session_id = new_session_id or "" if reset: self._turn_count = 0 + with self._synced_turns_lock: + self._synced_turns = 0 def on_pre_compress(self, messages: List[Dict[str, Any]]) -> str: """File the messages about to be discarded and signal verbatim persistence. @@ -795,7 +823,10 @@ def on_memory_write( self._cron_skipped or not self._initialized # worker isn't running; queueing leaks or action != "add" - or target != "user" + # Hermes' memory tool defaults ``target`` to "memory" when the + # caller omits it — that's the ordinary write path and must be + # mirrored, not just the explicit "user" target. + or target not in ("user", "memory") or not content ): return @@ -823,6 +854,7 @@ def on_delegation( f"[delegated task]\n{task}", f"[subagent {child_session_id} returned]\n{result}", session_id=self._session_id, + _count_synced=False, ) # ----- Tool dispatch --------------------------------------------------- @@ -870,12 +902,11 @@ def handle_tool_call(self, tool_name: str, args: Dict[str, Any], **kwargs: Any) return json.dumps(self._tool_diary_read(int(args.get("n", 10)))) # Tools that delegate directly to ``mempalace.mcp_server``'s - # public ``tool_*`` entry points. These share mempalace's own - # config for palace_path resolution rather than this plugin's - # ``self._palace_path`` — a known asymmetry that the original - # eight tools above don't share. In the common case (default - # palace at ``~/.mempalace/palace``) both resolve to the same - # place. + # public ``tool_*`` entry points. ``_dispatch_mcp_passthrough`` + # pins ``MEMPALACE_PALACE_PATH`` to ``self._palace_path`` for the + # duration of the call so these resolve against the same palace + # as the eight tools above rather than mempalace's own global + # config. # New tools (everything that has a matching ``tool_*`` in # mempalace.mcp_server) dispatch by name derivation. One # mempalace asymmetry to remap: ``mempalace_traverse`` maps to @@ -946,7 +977,22 @@ def _dispatch_mcp_passthrough(self, tool_name: str, args: Dict[str, Any]) -> Opt # miner-ingested ones. if tool_name == "mempalace_add_drawer": args.setdefault("added_by", "hermes") - return json.dumps(func(**args)) + # mempalace.mcp_server's tool_* functions resolve their palace from + # MEMPALACE_PALACE_PATH / ~/.mempalace/config.json — a process-global + # config, not this provider's self._palace_path. Pin the env var for + # the call so a Hermes profile with a custom palace_path doesn't read + # mempalace_search results from one palace and write drawers/tunnels + # via this passthrough to another. + with _PASSTHROUGH_ENV_LOCK: + prior = os.environ.get("MEMPALACE_PALACE_PATH") + os.environ["MEMPALACE_PALACE_PATH"] = self._palace_path + try: + return json.dumps(func(**args)) + finally: + if prior is None: + os.environ.pop("MEMPALACE_PALACE_PATH", None) + else: + os.environ["MEMPALACE_PALACE_PATH"] = prior # ----- Setup wizard integration ---------------------------------------- @@ -991,6 +1037,26 @@ def post_setup(self, hermes_home: str, config: Dict[str, Any]) -> None: print(" 2. (optional) edit ~/.mempalace/identity.txt to seed L0 wake-up context") print() + # ----- Backup integration ----------------------------------------------- + + def backup_paths(self) -> List[str]: + """Directories ``hermes backup`` should include beyond ``HERMES_HOME``. + + MemPalace's actual state — the palace, diary, knowledge graph, + wing config — lives under ``~/.mempalace`` by default, outside + ``HERMES_HOME``, so ``hermes backup`` would silently miss it without + this. + """ + roots: List[str] = [] + if self._palace_path: + roots.append(str(Path(self._palace_path).parent)) + identity_root = str( + Path(self._config.get("identity_path", self.DEFAULT_IDENTITY_PATH)).expanduser().parent + ) + if identity_root not in roots: + roots.append(identity_root) + return [root for root in roots if Path(root).exists()] + # ----- Shutdown -------------------------------------------------------- def shutdown(self) -> None: @@ -1107,10 +1173,20 @@ def _file_turn(self, payload: Dict[str, Any]) -> None: def _mine_session(self, payload: Dict[str, Any]) -> None: messages = payload.get("messages", []) or [] session_id = payload.get("session_id", "") or "" + # Leading user turns already filed live by sync_turn() — re-mining them + # here would double-file the same exchange under a second, distinct + # drawer id (make_exchange_drawer_id includes filed_at). Only the tail + # sync_turn never got a chance to see (e.g. the last turn before the + # session boundary) needs mining. + already_synced = payload.get("already_synced", 0) or 0 + skipped = 0 try: for idx, msg in enumerate(messages): if msg.get("role") != "user": continue + if skipped < already_synced: + skipped += 1 + continue # Same content normalization ``sync_turn`` and ``pre_compress`` # use — list-shaped Anthropic content must not be persisted as # its ``repr``. diff --git a/tests/test_hermes_integration.py b/tests/test_hermes_integration.py index 2c67a306ab..92670ab8b1 100644 --- a/tests/test_hermes_integration.py +++ b/tests/test_hermes_integration.py @@ -702,3 +702,88 @@ def test_match_wing_by_keywords_ignores_non_string_keywords(integration_module): wing_config = {"wing_dev": {"keywords": [None, 3, "python"]}} assert fn("write some python code", wing_config) == "wing_dev" assert fn("unrelated chatter", wing_config) == "wing_general" + + +# --------------------------------------------------------------------------- +# Review findings (PR #1915): duplicate filing, passthrough palace scoping, +# on_memory_write default target, backup_paths(). +# --------------------------------------------------------------------------- + + +def test_on_session_end_skips_turns_already_synced_live(initialized_provider): + """sync_turn() files turn 1 live; on_session_end's transcript replays it + plus one trailing turn sync_turn never saw. Only the trailing turn should + be filed again — replaying turn 1 would double-file it under a second + drawer id (make_exchange_drawer_id includes filed_at, so it isn't + idempotent).""" + initialized_provider.sync_turn("what's the plan?", "ship the PR") + initialized_provider._worker_queue.join() + assert initialized_provider._collection.count() == 1 + + initialized_provider.on_session_end( + [ + {"role": "user", "content": "what's the plan?"}, + {"role": "assistant", "content": "ship the PR"}, + {"role": "user", "content": "any blockers?"}, + {"role": "assistant", "content": "none"}, + ] + ) + initialized_provider._worker_queue.join() + + assert initialized_provider._collection.count() == 2 + + +def test_on_delegation_does_not_count_toward_session_end_skip(initialized_provider): + # Delegation turns are synthetic bookkeeping that never appears in the + # ``messages`` transcript on_session_end receives — counting them would + # skip (and lose) that many genuine leading turns instead. + initialized_provider.on_delegation("research X", "found Y", child_session_id="child-1") + initialized_provider._worker_queue.join() + assert initialized_provider._collection.count() == 1 + assert initialized_provider._synced_turns == 0 + + initialized_provider.on_session_end( + [ + {"role": "user", "content": "what's the plan?"}, + {"role": "assistant", "content": "ship the PR"}, + ] + ) + initialized_provider._worker_queue.join() + + assert initialized_provider._collection.count() == 2 + + +def test_passthrough_tool_uses_provider_configured_palace_path(initialized_provider): + """mempalace_add_drawer must land in the provider's configured palace, not + mempalace's own global-config default palace (a different directory in + this fixture).""" + result = json.loads( + initialized_provider.handle_tool_call( + "mempalace_add_drawer", + {"wing": "wing_general", "room": "notes", "content": "hello from hermes"}, + ) + ) + assert result.get("success", True) is not False + assert initialized_provider._collection.count() == 1 + + +def test_on_memory_write_mirrors_default_memory_target(initialized_provider): + # Hermes' memory tool omits ``target`` (defaults to "memory") on ordinary + # writes — those must be mirrored, not silently dropped. + initialized_provider.on_memory_write("add", "memory", "likes dark roast coffee") + initialized_provider._worker_queue.join() + + db_path = str(Path(initialized_provider._palace_path).parent / "knowledge_graph.sqlite3") + from mempalace.knowledge_graph import KnowledgeGraph + + kg = KnowledgeGraph(db_path=db_path) + try: + relations = kg.query_entity("user") + finally: + kg.close() + assert any(r.get("object") == "likes dark roast coffee" for r in relations) + + +def test_backup_paths_includes_palace_and_identity_roots(initialized_provider, palace_path): + roots = initialized_provider.backup_paths() + assert str(Path(palace_path).parent) in roots