From e0ed5dc9ed93aaa2d88ace82cf109fe0b33e4770 Mon Sep 17 00:00:00 2001 From: Kshitij Kapoor Date: Thu, 9 Jul 2026 03:14:56 +0530 Subject: [PATCH] refactor: address Phase-2 review findings on /new boundary handoff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Return the boundary snapshot from _launch_session_boundary_memory_flush as a local value instead of staging it on self._session_boundary_snapshot. The instance-attr handoff could leak (no memory manager configured) or mis-fire a stale snapshot on a later /new if an exception hit between staging and consumption. A local variable eliminates the class; the helper also returns None when no memory manager is configured so new_session takes the inline-switch path. - Drop the now-dead session_id kwarg from commit_memory_session: after the redesign no production caller passes it (gateway, TUI, compression all use the default), and speculative params are rejected per AGENTS.md. The explicit-old-session need is served by cli.py's direct engine call + commit_session_boundary_async. - Drop the dead providers snapshot in commit_session_boundary_async (only the emptiness check used it). - Tests updated accordingly (dead-kwarg test removed, snapshot assertion now covered by return-value contract). Phase-2 gates: 2a tests/cli 1048 passed + 6 memory files 137 passed; 2b programmatic live smoke 0.38ms non-blocking caller, end→switch→sync ordering verified; 2c structured 4-angle review — no Criticals, these warnings fixed. --- agent/memory_manager.py | 3 +- cli.py | 36 +++++++++---------- run_agent.py | 11 ++---- tests/cli/test_cli_new_session.py | 2 -- ...st_commit_memory_session_context_engine.py | 13 ------- 5 files changed, 21 insertions(+), 44 deletions(-) diff --git a/agent/memory_manager.py b/agent/memory_manager.py index 342656b4292..32dd8e28884 100644 --- a/agent/memory_manager.py +++ b/agent/memory_manager.py @@ -811,8 +811,7 @@ class MemoryManager: ``_submit_background`` degrades to inline execution — the pre-#16454 synchronous behavior, slow but correct. """ - providers = list(self._providers) - if not providers: + if not self._providers: return snapshot = list(messages or []) diff --git a/cli.py b/cli.py index c394f5545f6..069b3c616c1 100644 --- a/cli.py +++ b/cli.py @@ -4144,9 +4144,6 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin): # Background task tracking: {task_id: threading.Thread} self._background_tasks: Dict[str, threading.Thread] = {} self._background_task_counter = 0 - # History snapshot staged by _launch_session_boundary_memory_flush for - # the /new end→switch boundary task (see new_session()). - self._session_boundary_snapshot: Optional[list] = None def _claim_active_session(self, surface: str = "cli", *, stderr: bool = False) -> bool: """Claim a global active-session slot for this CLI process.""" @@ -6977,7 +6974,7 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin): history_snapshot: list, *, session_id: Optional[str] = None, - ) -> None: + ) -> Optional[list]: """Stage old-session memory extraction so /new stays responsive. The context-engine ``on_session_end`` boundary is delivered @@ -6986,18 +6983,20 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin): rebinds the engine to the new session. The memory-provider half (LLM-bound extraction, seconds) is NOT run - here. It is queued later in ``new_session()`` via + here. The returned snapshot is handed by ``new_session()`` to ``MemoryManager.commit_session_boundary_async`` as a single end→switch task on the manager's serialized background worker, so extraction can never race the provider rebinding (providers key off internal ``_session_id`` state — a late ``on_session_end`` after ``on_session_switch`` would misattribute the old transcript to the - new session). This method just snapshots the history for that call. + new session). + + Returns the history snapshot to queue, or ``None`` when there is + nothing to extract (no agent / empty history / no memory manager). """ - self._session_boundary_snapshot = None agent = getattr(self, "agent", None) if not agent or not history_snapshot: - return + return None engine = getattr(agent, "context_compressor", None) if engine is not None and hasattr(engine, "on_session_end"): @@ -7009,17 +7008,22 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin): exc_info=True, ) - self._session_boundary_snapshot = list(history_snapshot) + # No provider extraction to queue when no memory manager is + # configured — new_session() falls back to the inline switch path. + if getattr(agent, "_memory_manager", None) is None: + return None + return history_snapshot def new_session(self, silent=False, title=None): """Start a fresh session with a new session ID and cleared agent state.""" old_session_id = self.session_id + _boundary_snapshot = None if self.agent and self.conversation_history: - # Trigger memory extraction on the old session without blocking - # session rotation. Snapshot the history and old session id first: - # the background thread may run after ``self.agent.session_id`` is - # updated to the new session below. - self._launch_session_boundary_memory_flush( + # Deliver the context-engine boundary synchronously and get back + # the history snapshot for the deferred provider extraction — + # queued below (after rotation) so /new never blocks on the + # LLM-bound extraction call. + _boundary_snapshot = self._launch_session_boundary_memory_flush( list(self.conversation_history), session_id=old_session_id, ) @@ -7124,11 +7128,7 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin): try: _mm = getattr(self.agent, "_memory_manager", None) if _mm is not None: - _boundary_snapshot = getattr( - self, "_session_boundary_snapshot", None - ) if _boundary_snapshot: - self._session_boundary_snapshot = None _mm.commit_session_boundary_async( _boundary_snapshot, new_session_id=self.session_id, diff --git a/run_agent.py b/run_agent.py index d5b751073e9..6fb7e0903e5 100644 --- a/run_agent.py +++ b/run_agent.py @@ -3300,18 +3300,11 @@ class AIAgent: except Exception: pass - def commit_memory_session( - self, - messages: list = None, - session_id: Optional[str] = None, - ) -> None: + def commit_memory_session(self, messages: list = None) -> None: """Trigger end-of-session extraction without tearing providers down. Called when session_id rotates (e.g. /new, context compression); providers keep their state and continue running under the old session_id — they just flush pending extraction now.""" - ended_session_id = ( - session_id if session_id is not None else (self.session_id or "") - ) if self._memory_manager: try: self._memory_manager.on_session_end(messages or []) @@ -3326,7 +3319,7 @@ class AIAgent: if hasattr(self, "context_compressor") and self.context_compressor: try: self.context_compressor.on_session_end( - ended_session_id, + self.session_id or "", messages or [], ) except Exception: diff --git a/tests/cli/test_cli_new_session.py b/tests/cli/test_cli_new_session.py index c9f5cfea1ea..e869b13c851 100644 --- a/tests/cli/test_cli_new_session.py +++ b/tests/cli/test_cli_new_session.py @@ -194,8 +194,6 @@ def test_new_session_queues_boundary_commit_with_snapshot(tmp_path): assert kwargs["reason"] == "new_session" # The queued path replaces the inline switch — not both. mm.on_session_switch.assert_not_called() - # Snapshot is consumed (a later /new without history must not re-send it). - assert cli._session_boundary_snapshot is None def test_new_session_without_history_switches_inline(tmp_path): diff --git a/tests/run_agent/test_commit_memory_session_context_engine.py b/tests/run_agent/test_commit_memory_session_context_engine.py index 956cf5117be..307814891a2 100644 --- a/tests/run_agent/test_commit_memory_session_context_engine.py +++ b/tests/run_agent/test_commit_memory_session_context_engine.py @@ -56,19 +56,6 @@ def test_commit_memory_session_with_no_messages_passes_empty_list(): ctx.on_session_end.assert_called_once_with("sess-7", []) -def test_commit_memory_session_can_finalize_explicit_old_session_id(): - """Async callers may run after agent.session_id already changed.""" - mm = MagicMock() - ctx = MagicMock() - agent = _make_minimal_agent(mm, ctx, session_id="new-session") - - msgs = [{"role": "user", "content": "old turn"}] - agent.commit_memory_session(msgs, session_id="old-session") - - mm.on_session_end.assert_called_once_with(msgs) - ctx.on_session_end.assert_called_once_with("old-session", msgs) - - def test_commit_memory_session_no_memory_manager_still_notifies_context_engine(): """If only the context engine is configured, it still gets the hook.""" ctx = MagicMock()