fix(agent): repair sessions already contaminated with stale tool-call markers (#78148)
The conversation_loop fix (previous commit) stops new "[memory]"-style bare tool-call markers from being cached/persisted, but sessions written before that fix can still carry rows where a bare marker was saved as the assistant's "final response". Add a load-on-read repair pass in hermes_state.py, mirroring the existing _strip_background_review_harness defense-in-depth: on session restore, any assistant row whose content is only a bracketed marker (e.g. "[memory]", "[skill_manage]") AND that carries tool_calls has its content blanked before the history re-enters the model's context. 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 the normal path unchanged.
This commit is contained in:
joaomarcos
kshitij
parent
ba9068c8b6
commit
70d7e4cbdf
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
Loading…
Reference in New Issue