mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-04-27 22:41:19 +08:00
fix(gateway): pass session messages to shutdown_memory_provider (#15165)
``_cleanup_agent_resources`` previously invoked ``agent.shutdown_memory_provider()`` with no arguments, so every memory provider's ``on_session_end`` hook received an empty list. Providers with an early-return guard on empty input (Holographic, Hindsight) never extracted facts from the conversation, and users hit "抱歉,找不到相關的對話記錄" on the first turn after any gateway restart, session reset, or idle expiry. Forward ``agent._session_messages`` — the transcript the agent itself maintains and refreshes every turn via ``_persist_session`` — so providers see the actual conversation. Falls back to the legacy no-arg call whenever the attribute is absent or not a list (test stubs built via ``object.__new__`` or ``MagicMock``) to preserve backward compatibility with existing suites. ``AIAgent.shutdown_memory_provider`` already accepts ``messages: list = None`` (run_agent.py:4126), so this is a pure caller-side fix. Paths that use ``skip_memory=True`` temporary agents (memory flush, hygiene auto-compress, ``/compress``) are no-ops inside ``shutdown_memory_provider`` because ``self._memory_manager`` is None — no behaviour change for them. Covers Part A of the bug report. Part B (adding ``on_session_end`` to the Hindsight plugin) is a separate concern that would benefit from this fix landing first. Regression test added at ``tests/gateway/test_shutdown_memory_provider_messages.py`` covering: populated messages forwarded, empty list still forwarded, attribute missing falls back, non-list (MagicMock) falls back, provider exceptions don't block ``close()``, None agent no-op, and agent without ``shutdown_memory_provider`` tolerated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1943,7 +1943,21 @@ class GatewayRunner:
|
||||
return
|
||||
try:
|
||||
if hasattr(agent, "shutdown_memory_provider"):
|
||||
agent.shutdown_memory_provider()
|
||||
# Pass the agent's own conversation transcript so memory
|
||||
# providers' ``on_session_end`` hooks see the real messages
|
||||
# instead of the empty default (#15165). ``_session_messages``
|
||||
# is set on ``AIAgent`` (run_agent.py:1518) and refreshed at
|
||||
# the end of every ``run_conversation`` turn via
|
||||
# ``_persist_session``; on an agent built through
|
||||
# ``object.__new__`` (test stubs) the attribute may be
|
||||
# absent, so ``getattr`` with a ``None`` default keeps the
|
||||
# call signature-compatible with the pre-fix behaviour
|
||||
# (``shutdown_memory_provider(messages=None)``).
|
||||
session_messages = getattr(agent, "_session_messages", None)
|
||||
if isinstance(session_messages, list):
|
||||
agent.shutdown_memory_provider(session_messages)
|
||||
else:
|
||||
agent.shutdown_memory_provider()
|
||||
except Exception:
|
||||
pass
|
||||
# Close tool resources (terminal sandboxes, browser daemons,
|
||||
|
||||
148
tests/gateway/test_shutdown_memory_provider_messages.py
Normal file
148
tests/gateway/test_shutdown_memory_provider_messages.py
Normal file
@@ -0,0 +1,148 @@
|
||||
"""Regression tests for #15165 — gateway session shutdown must pass the
|
||||
agent's conversation transcript to ``shutdown_memory_provider`` so memory
|
||||
providers' ``on_session_end`` hooks see the real messages instead of an
|
||||
empty list.
|
||||
|
||||
Before the fix, ``_cleanup_agent_resources`` called
|
||||
``agent.shutdown_memory_provider()`` with no arguments, which in turn
|
||||
invoked ``on_session_end([])`` on every memory provider. Providers with
|
||||
an empty-guard (Holographic, Hindsight, etc.) exited early and never
|
||||
persisted the session's facts, so the next gateway start-up surfaced no
|
||||
memories from the prior conversation.
|
||||
|
||||
The fix reads ``agent._session_messages`` (set on ``AIAgent.__init__``
|
||||
and refreshed every turn via ``_persist_session``) and forwards it to
|
||||
``shutdown_memory_provider``. Test stubs built via ``object.__new__``
|
||||
or plain ``MagicMock()`` still exercise the legacy no-arg path, so the
|
||||
change is backward-compatible with existing suites.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import sys
|
||||
import types
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _mock_dotenv(monkeypatch):
|
||||
"""gateway.run imports dotenv at module load; stub so tests run bare."""
|
||||
fake = types.ModuleType("dotenv")
|
||||
fake.load_dotenv = lambda *a, **kw: None
|
||||
monkeypatch.setitem(sys.modules, "dotenv", fake)
|
||||
|
||||
|
||||
def _make_runner():
|
||||
from gateway.run import GatewayRunner
|
||||
|
||||
runner = object.__new__(GatewayRunner)
|
||||
return runner
|
||||
|
||||
|
||||
# A lightweight stand-in for AIAgent so ``isinstance(..., list)`` correctly
|
||||
# discriminates between "attribute set to a list" and "attribute absent /
|
||||
# MagicMock auto-synthesised". Using MagicMock directly for the agent
|
||||
# would also work for the populated case, but attribute access on a
|
||||
# MagicMock always yields a child MagicMock — we want a real Python
|
||||
# object we can shape per-test.
|
||||
class _FakeAgent:
|
||||
def __init__(self, session_messages=None, has_shutdown=True):
|
||||
if session_messages is not None:
|
||||
self._session_messages = session_messages
|
||||
if has_shutdown:
|
||||
self.shutdown_memory_provider = MagicMock()
|
||||
self.close = MagicMock()
|
||||
|
||||
|
||||
class TestCleanupAgentResourcesPassesMessages:
|
||||
"""_cleanup_agent_resources forwards the agent's session messages."""
|
||||
|
||||
def test_populated_messages_forwarded(self):
|
||||
"""Real-world path: an agent that ran a turn has a populated
|
||||
``_session_messages`` list and the cleanup call forwards it."""
|
||||
runner = _make_runner()
|
||||
transcript = [
|
||||
{"role": "user", "content": "remember my dog is named Biscuit"},
|
||||
{"role": "assistant", "content": "Got it — Biscuit."},
|
||||
]
|
||||
agent = _FakeAgent(session_messages=transcript)
|
||||
|
||||
runner._cleanup_agent_resources(agent)
|
||||
|
||||
# The fix must call shutdown_memory_provider with the exact list
|
||||
# identity — providers iterate it to extract facts.
|
||||
agent.shutdown_memory_provider.assert_called_once_with(transcript)
|
||||
|
||||
def test_empty_list_still_forwarded(self):
|
||||
"""An agent that initialised but ran no turns has an empty list
|
||||
on ``_session_messages``. Forwarding it (rather than falling
|
||||
through to the no-arg path) makes the absence of content
|
||||
explicit to providers and matches the pre-fix observable
|
||||
behaviour (``on_session_end([])``)."""
|
||||
runner = _make_runner()
|
||||
agent = _FakeAgent(session_messages=[])
|
||||
|
||||
runner._cleanup_agent_resources(agent)
|
||||
|
||||
agent.shutdown_memory_provider.assert_called_once_with([])
|
||||
|
||||
def test_missing_attribute_falls_back_to_no_arg(self):
|
||||
"""Test stubs built via ``object.__new__(AIAgent)`` skip
|
||||
``__init__`` and therefore have no ``_session_messages``
|
||||
attribute. The fix must not explode — it falls back to the
|
||||
legacy no-arg call so existing suites keep passing."""
|
||||
runner = _make_runner()
|
||||
agent = _FakeAgent(session_messages=None) # attribute not set
|
||||
|
||||
runner._cleanup_agent_resources(agent)
|
||||
|
||||
agent.shutdown_memory_provider.assert_called_once_with()
|
||||
|
||||
def test_non_list_attribute_falls_back_to_no_arg(self):
|
||||
"""A MagicMock-based agent auto-synthesises ``_session_messages``
|
||||
as a nested MagicMock. ``isinstance(mock, list)`` is False, so
|
||||
we fall back to the no-arg path rather than passing a garbage
|
||||
value to providers that expect ``List[Dict]``."""
|
||||
runner = _make_runner()
|
||||
agent = MagicMock()
|
||||
# No explicit _session_messages assignment — MagicMock will
|
||||
# synthesise one on access.
|
||||
|
||||
runner._cleanup_agent_resources(agent)
|
||||
|
||||
agent.shutdown_memory_provider.assert_called_once_with()
|
||||
|
||||
def test_provider_exception_is_swallowed(self):
|
||||
"""Provider teardown must be best-effort — a raising
|
||||
``shutdown_memory_provider`` must not prevent ``close()`` from
|
||||
running (tool resource leak is worse than a missed memory
|
||||
flush)."""
|
||||
runner = _make_runner()
|
||||
agent = _FakeAgent(session_messages=[{"role": "user", "content": "x"}])
|
||||
agent.shutdown_memory_provider.side_effect = RuntimeError("boom")
|
||||
|
||||
# Must not raise.
|
||||
runner._cleanup_agent_resources(agent)
|
||||
|
||||
# close() still invoked after the swallowed exception.
|
||||
agent.close.assert_called_once()
|
||||
|
||||
def test_none_agent_is_noop(self):
|
||||
"""Defensive: None agent short-circuits (idle sweeps may
|
||||
observe a None entry in the cache during eviction races)."""
|
||||
runner = _make_runner()
|
||||
# Must not raise.
|
||||
runner._cleanup_agent_resources(None)
|
||||
|
||||
def test_agent_without_shutdown_method_is_tolerated(self):
|
||||
"""An agent without ``shutdown_memory_provider`` (old test
|
||||
stub, partial mock) must still have ``close()`` called."""
|
||||
runner = _make_runner()
|
||||
agent = _FakeAgent(has_shutdown=False)
|
||||
# No _session_messages either, to exercise the hasattr guard.
|
||||
|
||||
runner._cleanup_agent_resources(agent)
|
||||
|
||||
agent.close.assert_called_once()
|
||||
Reference in New Issue
Block a user