Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 44 additions & 5 deletions gateway/platforms/api_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -6129,18 +6129,57 @@ def _response_messages_turn_start_index(
user_message: Any,
result: Dict[str, Any],
) -> int:
"""Detect transcript-shaped result["messages"] and return turn start."""
"""Detect transcript-shaped result["messages"] and return turn start.

Uses role+content matching (ignoring metadata fields like timestamp,
finish_reason, etc.) because the agent modifies messages during the
conversation loop — timestamps are added, content may be truncated,
and fields like finish_reason/reasoning are stamped on. Full dict
equality (``==``) fails on these modifications, causing the prefix
match to return 0 and the full history to be returned instead of just
the current turn. See #89891.
"""
agent_messages = result.get("messages") if isinstance(result, dict) else None
if not isinstance(agent_messages, list) or not agent_messages:
return 0

def _match(expected: Dict[str, Any], actual: Dict[str, Any]) -> bool:
"""Compare role + content, ignoring metadata fields."""
if expected.get("role") != actual.get("role"):
return False
# Compare content (may be str, list, or None)
exp_content = expected.get("content")
act_content = actual.get("content")
if exp_content != act_content:
# Handle string content that may be truncated by agent
if isinstance(exp_content, str) and isinstance(act_content, str):
# Allow prefix match for content (agent may truncate)
if not act_content.startswith(exp_content[:100]):
return False
else:
return False
return True

prior = list(conversation_history)
current_user = {"role": "user", "content": user_message}
expected_prefix = prior + [current_user]
if agent_messages[:len(expected_prefix)] == expected_prefix:
return len(expected_prefix)
if prior and agent_messages[:len(prior)] == prior:
return len(prior)

# Try matching with current user message
if len(agent_messages) >= len(expected_prefix):
if all(
_match(expected, actual)
for expected, actual in zip(expected_prefix, agent_messages[:len(expected_prefix)])
):
return len(expected_prefix)

# Try matching without current user message (edge case)
if prior and len(agent_messages) >= len(prior):
if all(
_match(expected, actual)
for expected, actual in zip(prior, agent_messages[:len(prior)])
):
return len(prior)

return 0

@classmethod
Expand Down
9 changes: 9 additions & 0 deletions hermes_cli/mcp_catalog.py
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,15 @@ def _build_server_config(
cfg["args"] = [_expand_install_dir(a, install_dir) for a in t.args]
if t.env:
cfg["env"] = dict(t.env)
# Wire auth.env credentials into the stdio child's environment.
# install_entry() already saved these to .env via _prompt_env_vars(),
# but without an env-backed reference here, _build_safe_env() would
# exclude them and the child would start without its API key (#89316).
if entry.auth.type == "api_key" and entry.auth.env:
env = cfg.get("env") or {}
for spec in entry.auth.env:
env[spec.name] = f"${{{spec.name}}}"
cfg["env"] = env
elif t.type == "http":
cfg["url"] = t.url
if entry.auth.type == "oauth":
Expand Down
147 changes: 147 additions & 0 deletions tests/gateway/test_api_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -2130,6 +2130,153 @@ async def test_truncation_auto_preserves_non_leading_compaction_summary(self, ad
assert history[-1]["content"] == "msg 147"


# ---------------------------------------------------------------------------
# Turn-start detection — role+content matching (ignoring metadata)
# Regression tests for #89891
# ---------------------------------------------------------------------------


class TestTurnStartDetection:
"""Response-side turn-start detection uses role+content matching
(ignoring metadata) so it survives the agent's in-loop message
modifications (timestamps, content truncation, finish_reason).
"""

def test_timestamps_on_messages_does_not_break_detection(self):
"""Agent adds timestamp fields — must still detect turn start."""
history = [
{"role": "user", "content": "Hello"},
{"role": "assistant", "content": "Hi there!"},
]
result = {
"messages": [
{"role": "user", "content": "Hello", "timestamp": 1000},
{"role": "assistant", "content": "Hi there!", "timestamp": 1001},
{"role": "user", "content": "What is 2+2?", "timestamp": 1002},
{"role": "assistant", "content": "4", "timestamp": 1003},
]
}
assert APIServerAdapter._response_messages_turn_start_index(
history, "What is 2+2?", result
) == 3

def test_exact_match_still_works(self):
"""Full dict equality path still works for unmodified messages."""
history = [
{"role": "user", "content": "Hello"},
{"role": "assistant", "content": "Hi!"},
]
result = {
"messages": [
{"role": "user", "content": "Hello"},
{"role": "assistant", "content": "Hi!"},
{"role": "user", "content": "Follow up"},
{"role": "assistant", "content": "OK"},
]
}
assert APIServerAdapter._response_messages_turn_start_index(
history, "Follow up", result
) == 3

def test_empty_history_matches_first_user(self):
"""Empty history: match the first user message."""
result = {
"messages": [
{"role": "user", "content": "Hello"},
{"role": "assistant", "content": "Hi!"},
]
}
assert APIServerAdapter._response_messages_turn_start_index(
[], "Hello", result
) == 1

def test_tool_calls_with_timestamps(self):
"""Tool call messages with timestamps — detect correctly."""
history = [
{"role": "user", "content": "Hi"},
{"role": "assistant", "content": "Hello!"},
]
result = {
"messages": [
{"role": "user", "content": "Hi", "timestamp": 100},
{"role": "assistant", "content": "Hello!", "timestamp": 101},
{"role": "user", "content": "Compute", "timestamp": 102},
{
"role": "assistant",
"content": None,
"tool_calls": [{"id": "1", "function": {"name": "calc", "arguments": "{}"}}],
"timestamp": 103,
},
{"role": "tool", "content": "42", "tool_call_id": "1", "timestamp": 104},
{"role": "assistant", "content": "42", "timestamp": 105},
]
}
assert APIServerAdapter._response_messages_turn_start_index(
history, "Compute", result
) == 3

def test_truncated_content_matches(self):
"""Agent may truncate long content — prefix match should still work."""
long_content = "A" * 200
history = [
{"role": "user", "content": long_content},
{"role": "assistant", "content": "OK"},
]
truncated = "A" * 150 + "..." # agent truncated
result = {
"messages": [
{"role": "user", "content": truncated, "timestamp": 1},
{"role": "assistant", "content": "OK", "timestamp": 2},
{"role": "user", "content": "Next", "timestamp": 3},
{"role": "assistant", "content": "Done", "timestamp": 4},
]
}
# First 100 chars of expected content match the truncated version
assert APIServerAdapter._response_messages_turn_start_index(
history, "Next", result
) == 3

def test_no_match_returns_zero(self):
"""No prefix match at all — return 0 (use full messages)."""
history = [
{"role": "user", "content": "Completely different"},
]
result = {
"messages": [
{"role": "user", "content": "Something else"},
{"role": "assistant", "content": "???"},
]
}
assert APIServerAdapter._response_messages_turn_start_index(
history, "Something else", result
) == 0

def test_empty_messages_returns_zero(self):
"""Empty or missing messages list — return 0."""
assert APIServerAdapter._response_messages_turn_start_index([], "Hi", {"messages": []}) == 0
assert APIServerAdapter._response_messages_turn_start_index([], "Hi", {}) == 0
assert APIServerAdapter._response_messages_turn_start_index([], "Hi", {"messages": None}) == 0

def test_turn_transcript_messages_returns_current_turn_only(self):
"""_turn_transcript_messages returns only the current turn, not full history."""
history = [
{"role": "user", "content": "Hi"},
{"role": "assistant", "content": "Hello!"},
]
result = {
"messages": [
{"role": "user", "content": "Hi", "timestamp": 100},
{"role": "assistant", "content": "Hello!", "timestamp": 101},
{"role": "user", "content": "What is 2+2?", "timestamp": 102},
{"role": "assistant", "content": "4", "timestamp": 103},
]
}
turn = APIServerAdapter._turn_transcript_messages(history, "What is 2+2?", result)
# Only the assistant's "4" reply should be in the turn transcript
assert len(turn) == 1
assert turn[0].get("content") == "4"


# ---------------------------------------------------------------------------
# Response-side truncation / failure handling (issue #22496)
# ---------------------------------------------------------------------------
Expand Down
81 changes: 81 additions & 0 deletions tests/hermes_cli/test_mcp_catalog.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,87 @@ def test_install_with_api_key_prompts_and_saves(self, catalog_dir, monkeypatch):
assert get_env_value("DEMO_KEY") == "secret-val"
assert "demo" in load_config()["mcp_servers"]

def test_install_stdio_api_key_wires_env_references(self, catalog_dir, monkeypatch):
"""stdio + api_key: auth.env must reach the generated MCP config as
env-backed references so the child process receives the credentials.

Regression test for #89316 — before the fix, _build_server_config
dropped auth.env entirely for stdio transports, so the stdio child
started without its API key even though install_entry() had saved it
to .env.
"""
body = _basic_manifest(
name="example",
transport={"type": "stdio", "command": "/bin/true", "args": []},
auth={
"type": "api_key",
"env": [
{"name": "EXAMPLE_BASE_URL", "prompt": "URL", "secret": False},
{"name": "EXAMPLE_API_KEY", "prompt": "key", "secret": True},
],
},
)
_write_manifest(catalog_dir, "example", body)

from hermes_cli import mcp_catalog

monkeypatch.setattr(
mcp_catalog, "_prompt_input", lambda prompt, **kw: "secret-val"
)

from hermes_cli.mcp_catalog import install_entry
from hermes_cli.config import get_config_path, load_config

install_entry(_entry("example"), enable=True)

server = load_config()["mcp_servers"]["example"]
assert server["command"] == "/bin/true"
# load_config resolves ${VAR} from .env — verify the resolved values
# reach the config (proving the template wired them through).
assert server["env"] == {
"EXAMPLE_BASE_URL": "secret-val",
"EXAMPLE_API_KEY": "secret-val",
}

# The raw file must carry ${...} templates, never the secret itself.
raw = get_config_path().read_text()
assert "${EXAMPLE_API_KEY}" in raw
assert "secret-val" not in raw

def test_install_stdio_api_key_merges_with_transport_env(
self, catalog_dir, monkeypatch
):
"""When both transport.env and auth.env exist, both must be present."""
body = _basic_manifest(
name="example",
transport={
"type": "stdio",
"command": "/bin/true",
"args": [],
"env": {"DEBUG": "1"},
},
auth={
"type": "api_key",
"env": [{"name": "EXAMPLE_KEY", "prompt": "key", "secret": True}],
},
)
_write_manifest(catalog_dir, "example", body)

from hermes_cli import mcp_catalog

monkeypatch.setattr(
mcp_catalog, "_prompt_input", lambda prompt, **kw: "secret-val"
)

from hermes_cli.mcp_catalog import install_entry
from hermes_cli.config import load_config

install_entry(_entry("example"), enable=True)

env = load_config()["mcp_servers"]["example"]["env"]
assert env["DEBUG"] == "1"
assert env["EXAMPLE_KEY"] == "secret-val"

def test_install_http_api_key_writes_bearer_headers(self, catalog_dir, monkeypatch):
body = _basic_manifest(
transport={"type": "http", "url": "https://mcp.example.com/sse"},
Expand Down