diff --git a/hermes_state.py b/hermes_state.py index 0396567c80b72..2b6f22017e348 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -405,6 +405,57 @@ def _strip_background_review_harness( return out +# Matches a bare protocol/tool-name marker such as "[memory]" or "[skill_manage]". +_STALE_TOOL_CALL_MARKER_RE = re.compile(r"^\[[A-Za-z_][A-Za-z0-9_.-]*\]$") + + +def _is_stale_tool_call_marker_message(msg: Dict[str, Any]) -> bool: + """True when ``msg`` is a persisted assistant turn whose content is a bare + bracketed marker (e.g. ``[memory]``) left over from a tool-call turn. + + Before the #78148 fix in ``agent.conversation_loop``, a local tool-call + template could emit a bare marker as assistant content alongside a real + tool call. The loop cached that marker as a fallback and later replayed + it as the "final response", persisting it into the session. Sessions + written before the fix can still carry these rows. + """ + if not isinstance(msg, dict): + return False + if msg.get("role") != "assistant": + return False + if not msg.get("tool_calls"): + return False + content = msg.get("content") + if not isinstance(content, str): + return False + return bool(_STALE_TOOL_CALL_MARKER_RE.fullmatch(content.strip())) + + +def _strip_stale_tool_call_markers( + messages: List[Dict[str, Any]], +) -> List[Dict[str, Any]]: + """Clear bare protocol-marker content persisted before the #78148 fix. + + Replaying "[memory]" as if the model had actually answered teaches the + model, by example, to keep emitting the same marker in later turns — the + exact symptom the issue reported. Only the stray ``content`` field is + blanked; the tool call and its result are left untouched so provider + tool_call/tool_result pairing stays intact. Sessions with no affected + rows pass through unchanged. + """ + repaired = 0 + for msg in messages: + if _is_stale_tool_call_marker_message(msg): + msg["content"] = "" + repaired += 1 + if repaired: + logger.info( + "Cleared %d stale tool-call marker message(s) while restoring session (#78148)", + repaired, + ) + return messages + + def format_session_db_unavailable(prefix: str = "Session database not available") -> str: """Format a user-facing 'session DB unavailable' message with cause. @@ -7168,6 +7219,13 @@ class SessionDB(SessionSearchMixin, SessionSchemaMixin, SessionPortabilityMixin) # assistant reply immediately following it, so a polluted session # resumes clean even if stray rows exist. messages = _strip_background_review_harness(messages) + # DEFENSE-IN-DEPTH against #78148: before that fix, a bare tool-call + # marker (e.g. "[memory]") could get cached as a fallback and + # persisted as if it were the model's real answer. Sessions written + # before the fix can still carry those rows — clear the stray + # content on load so replaying history doesn't re-teach the model + # to keep emitting the marker. No-op for unaffected sessions. + messages = _strip_stale_tool_call_markers(messages) if repair_alternation and messages: # Lazy import: hermes_state already depends on agent.* (see # sanitize_context above), but keep this optional path from diff --git a/tests/test_stale_tool_call_marker_session_repair.py b/tests/test_stale_tool_call_marker_session_repair.py new file mode 100644 index 0000000000000..e07bef5ea5818 --- /dev/null +++ b/tests/test_stale_tool_call_marker_session_repair.py @@ -0,0 +1,136 @@ +"""Tests for stale tool-call marker session repair (hermes_state, #78148). + +Before the root-cause fix in ``agent.conversation_loop``, a local tool-call +template could emit a bare bracketed marker (e.g. "[memory]") as assistant +content alongside a real tool call. The loop cached that marker as a +fallback and, when the following turn came back empty, replayed it as the +"final response" — persisting it into the session as if the model had +actually answered. + +``_strip_stale_tool_call_markers`` is the load-on-read defense-in-depth +that clears any such stray marker content from sessions written before the +fix, so resuming a polluted session doesn't re-teach the model to keep +emitting the marker. Unaffected sessions pass through unchanged. +""" + +from hermes_state import ( + _is_stale_tool_call_marker_message, + _strip_stale_tool_call_markers, +) + + +class TestIsStaleToolCallMarkerMessage: + def test_matches_bare_marker_with_tool_calls(self): + msg = { + "role": "assistant", + "content": "[memory]", + "tool_calls": [{"id": "1", "function": {"name": "skill_manage", "arguments": "{}"}}], + } + assert _is_stale_tool_call_marker_message(msg) is True + + def test_matches_dotted_marker(self): + msg = { + "role": "assistant", + "content": "[foo.bar]", + "tool_calls": [{"id": "1", "function": {"name": "foo.bar", "arguments": "{}"}}], + } + assert _is_stale_tool_call_marker_message(msg) is True + + def test_ignores_marker_without_tool_calls(self): + # A genuine final response of "[memory]" with no tool call is not + # the contamination signature — leave it alone. + msg = {"role": "assistant", "content": "[memory]"} + assert _is_stale_tool_call_marker_message(msg) is False + + def test_ignores_real_content_with_tool_calls(self): + msg = { + "role": "assistant", + "content": "I'll check that for you.", + "tool_calls": [{"id": "1", "function": {"name": "skill_manage", "arguments": "{}"}}], + } + assert _is_stale_tool_call_marker_message(msg) is False + + def test_ignores_user_role(self): + msg = { + "role": "user", + "content": "[memory]", + "tool_calls": [{"id": "1", "function": {"name": "skill_manage", "arguments": "{}"}}], + } + assert _is_stale_tool_call_marker_message(msg) is False + + +class TestStripStaleToolCallMarkers: + def test_clears_contaminated_content_keeps_tool_calls(self): + messages = [ + {"role": "user", "content": "do the full task"}, + { + "role": "assistant", + "content": "[memory]", + "tool_calls": [{"id": "1", "function": {"name": "skill_manage", "arguments": "{}"}}], + }, + {"role": "tool", "content": "ok", "tool_call_id": "1"}, + ] + out = _strip_stale_tool_call_markers(messages) + assert out[1]["content"] == "" + # Tool call itself must survive — provider tool_call/result pairing. + assert out[1]["tool_calls"] == [{"id": "1", "function": {"name": "skill_manage", "arguments": "{}"}}] + + def test_unaffected_session_passes_through_unchanged(self): + messages = [ + {"role": "user", "content": "What's the weather?"}, + {"role": "assistant", "content": "It's sunny."}, + ] + out = _strip_stale_tool_call_markers(messages) + assert out == messages + + +class TestGetMessagesAsConversationStripsStaleMarkers: + """The load-on-read wiring: get_messages_as_conversation must actually + call _strip_stale_tool_call_markers, so a session polluted with a stale + "[memory]" marker resumes clean end-to-end (not just the pure helper in + isolation).""" + + def test_polluted_session_resumes_without_marker(self): + import tempfile + from pathlib import Path + from hermes_state import SessionDB + + with tempfile.TemporaryDirectory() as tmp: + db = SessionDB(db_path=Path(tmp) / "t.db") + try: + db.create_session(session_id="s1", source="cli") + db.append_message("s1", role="user", content="do the full task") + # Stray contamination written by an older build (pre-#78148 fix). + db.append_message( + "s1", role="assistant", content="[memory]", + tool_calls=[{"id": "1", "function": {"name": "skill_manage", "arguments": "{}"}}], + ) + db.append_message("s1", role="tool", content="ok", tool_call_id="1") + db.append_message("s1", role="assistant", content="Here is the result.") + + conv = db.get_messages_as_conversation("s1") + contents = [m.get("content") for m in conv if m.get("role") == "assistant"] + + assert "[memory]" not in contents + assert "Here is the result." in contents + finally: + db.close() + + def test_clean_session_resumes_unaffected(self): + import tempfile + from pathlib import Path + from hermes_state import SessionDB + + with tempfile.TemporaryDirectory() as tmp: + db = SessionDB(db_path=Path(tmp) / "t.db") + try: + db.create_session(session_id="s1", source="cli") + db.append_message("s1", role="user", content="What's the weather?") + db.append_message("s1", role="assistant", content="It's sunny.") + + conv = db.get_messages_as_conversation("s1") + contents = [m.get("content") for m in conv] + + assert contents == ["What's the weather?", "It's sunny."] + finally: + db.close()