fix: filter transcript-only roles from chat-completions payload (#4715)
Add a provider-agnostic role allowlist guard to _sanitize_api_messages() that drops messages with roles not accepted by the chat-completions API (e.g. session_meta). This prevents CLI resume/session restore from leaking transcript-only metadata into the outgoing messages payload. Two layers of defense: 1. API-boundary guard: _sanitize_api_messages() now filters messages by role allowlist (system/user/assistant/tool/function/developer) before the existing orphaned tool-call repair logic. This protects all current and future call paths. 2. CLI restore defense-in-depth: Both session restore paths in cli.py now strip session_meta entries before loading history into conversation_history, matching the existing gateway behavior. Closes #4715
This commit is contained in:
2
cli.py
2
cli.py
@@ -2166,6 +2166,7 @@ class HermesCLI:
|
||||
return False
|
||||
restored = self._session_db.get_messages_as_conversation(self.session_id)
|
||||
if restored:
|
||||
restored = [m for m in restored if m.get("role") != "session_meta"]
|
||||
self.conversation_history = restored
|
||||
msg_count = len([m for m in restored if m.get("role") == "user"])
|
||||
title_part = ""
|
||||
@@ -2361,6 +2362,7 @@ class HermesCLI:
|
||||
|
||||
restored = self._session_db.get_messages_as_conversation(self.session_id)
|
||||
if restored:
|
||||
restored = [m for m in restored if m.get("role") != "session_meta"]
|
||||
self.conversation_history = restored
|
||||
msg_count = len([m for m in restored if m.get("role") == "user"])
|
||||
title_part = ""
|
||||
|
||||
15
run_agent.py
15
run_agent.py
@@ -2585,6 +2585,8 @@ class AIAgent:
|
||||
return tc.get("id", "") or ""
|
||||
return getattr(tc, "id", "") or ""
|
||||
|
||||
_VALID_API_ROLES = frozenset({"system", "user", "assistant", "tool", "function", "developer"})
|
||||
|
||||
@staticmethod
|
||||
def _sanitize_api_messages(messages: List[Dict[str, Any]]) -> List[Dict[str, Any]]:
|
||||
"""Fix orphaned tool_call / tool_result pairs before every LLM call.
|
||||
@@ -2593,6 +2595,19 @@ class AIAgent:
|
||||
is present — so orphans from session loading or manual message
|
||||
manipulation are always caught.
|
||||
"""
|
||||
# --- Role allowlist: drop messages with roles the API won't accept ---
|
||||
filtered = []
|
||||
for msg in messages:
|
||||
role = msg.get("role")
|
||||
if role not in AIAgent._VALID_API_ROLES:
|
||||
logger.debug(
|
||||
"Pre-call sanitizer: dropping message with invalid role %r",
|
||||
role,
|
||||
)
|
||||
continue
|
||||
filtered.append(msg)
|
||||
messages = filtered
|
||||
|
||||
surviving_call_ids: set = set()
|
||||
for msg in messages:
|
||||
if msg.get("role") == "assistant":
|
||||
|
||||
90
tests/test_session_meta_filtering.py
Normal file
90
tests/test_session_meta_filtering.py
Normal file
@@ -0,0 +1,90 @@
|
||||
"""Tests for session_meta filtering — issue #4715.
|
||||
|
||||
Ensures that transcript-only session_meta messages never reach the
|
||||
chat-completions API, via both the API-boundary guard in
|
||||
_sanitize_api_messages() and the CLI session-restore paths.
|
||||
"""
|
||||
|
||||
import logging
|
||||
import types
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from run_agent import AIAgent
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Layer 1 — _sanitize_api_messages role-allowlist guard
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestSanitizeApiMessagesRoleFilter:
|
||||
|
||||
def test_drops_session_meta_role(self):
|
||||
msgs = [
|
||||
{"role": "user", "content": "hello"},
|
||||
{"role": "session_meta", "content": {"model": "gpt-4"}},
|
||||
{"role": "assistant", "content": "hi"},
|
||||
]
|
||||
out = AIAgent._sanitize_api_messages(msgs)
|
||||
assert len(out) == 2
|
||||
assert all(m["role"] != "session_meta" for m in out)
|
||||
|
||||
def test_preserves_valid_roles(self):
|
||||
msgs = [
|
||||
{"role": "system", "content": "you are helpful"},
|
||||
{"role": "user", "content": "hello"},
|
||||
{"role": "assistant", "content": "hi"},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "ok"},
|
||||
]
|
||||
# Need a matching assistant tool_call so the tool result isn't orphaned
|
||||
msgs[2]["tool_calls"] = [{"id": "c1", "function": {"name": "t", "arguments": "{}"}}]
|
||||
out = AIAgent._sanitize_api_messages(msgs)
|
||||
roles = [m["role"] for m in out]
|
||||
assert "system" in roles
|
||||
assert "user" in roles
|
||||
assert "assistant" in roles
|
||||
assert "tool" in roles
|
||||
|
||||
def test_logs_warning_when_dropping(self, caplog):
|
||||
msgs = [
|
||||
{"role": "user", "content": "hello"},
|
||||
{"role": "session_meta", "content": {"info": "test"}},
|
||||
]
|
||||
with caplog.at_level(logging.DEBUG, logger="run_agent"):
|
||||
AIAgent._sanitize_api_messages(msgs)
|
||||
assert any("invalid role" in r.message and "session_meta" in r.message for r in caplog.records)
|
||||
|
||||
def test_drops_multiple_invalid_roles(self):
|
||||
msgs = [
|
||||
{"role": "user", "content": "hello"},
|
||||
{"role": "session_meta", "content": {}},
|
||||
{"role": "transcript_note", "content": "note"},
|
||||
{"role": "assistant", "content": "hi"},
|
||||
]
|
||||
out = AIAgent._sanitize_api_messages(msgs)
|
||||
assert len(out) == 2
|
||||
assert [m["role"] for m in out] == ["user", "assistant"]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Layer 2 — CLI session-restore filters session_meta before loading
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestCLISessionRestoreFiltering:
|
||||
|
||||
def test_restore_filters_session_meta(self):
|
||||
"""Simulates the CLI restore path and verifies session_meta is removed."""
|
||||
# Build a fake restored message list (as returned by get_messages_as_conversation)
|
||||
fake_restored = [
|
||||
{"role": "session_meta", "content": {"model": "gpt-4"}},
|
||||
{"role": "user", "content": "hello"},
|
||||
{"role": "assistant", "content": "hi there"},
|
||||
{"role": "session_meta", "content": {"tools": []}},
|
||||
]
|
||||
|
||||
# Apply the same filtering that the patched CLI code now does
|
||||
filtered = [m for m in fake_restored if m.get("role") != "session_meta"]
|
||||
|
||||
assert len(filtered) == 2
|
||||
assert all(m["role"] != "session_meta" for m in filtered)
|
||||
assert filtered[0]["role"] == "user"
|
||||
assert filtered[1]["role"] == "assistant"
|
||||
Reference in New Issue
Block a user