fix(hermes): address igorls' second review pass on PR #1915

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``.

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