Merge branch 'main' of github.com:NousResearch/hermes-agent into bb/gui
This commit is contained in:
@@ -178,9 +178,10 @@ class TestMcpRegistrationE2E:
|
||||
complete_event = completions[0]
|
||||
assert isinstance(complete_event, ToolCallProgress)
|
||||
assert complete_event.status == "completed"
|
||||
# rawOutput should contain the tool result string
|
||||
assert complete_event.raw_output is not None
|
||||
assert "hello" in str(complete_event.raw_output)
|
||||
# Completion should contain human-readable output rather than forcing raw JSON panes.
|
||||
assert complete_event.content
|
||||
assert "hello" in complete_event.content[0].content.text
|
||||
assert complete_event.raw_output is None
|
||||
|
||||
def test_patch_mode_tool_start_emits_diff_blocks_for_v4a_patch(self):
|
||||
update = build_tool_start(
|
||||
|
||||
+185
-7
@@ -27,7 +27,10 @@ from acp.schema import (
|
||||
SetSessionModeResponse,
|
||||
SessionInfo,
|
||||
TextContentBlock,
|
||||
ToolCallProgress,
|
||||
ToolCallStart,
|
||||
Usage,
|
||||
UsageUpdate,
|
||||
UserMessageChunk,
|
||||
)
|
||||
from acp_adapter.server import HermesACPAgent, HERMES_VERSION
|
||||
@@ -200,6 +203,8 @@ class TestSessionOps:
|
||||
"context",
|
||||
"reset",
|
||||
"compact",
|
||||
"steer",
|
||||
"queue",
|
||||
"version",
|
||||
]
|
||||
model_cmd = next(
|
||||
@@ -208,6 +213,46 @@ class TestSessionOps:
|
||||
assert model_cmd.input is not None
|
||||
assert model_cmd.input.root.hint == "model name to switch to"
|
||||
|
||||
def test_build_usage_update_for_zed_context_indicator(self, agent, mock_manager):
|
||||
state = mock_manager.create_session(cwd="/tmp")
|
||||
state.history = [{"role": "user", "content": "hello"}]
|
||||
state.agent.context_compressor = MagicMock(context_length=100_000)
|
||||
state.agent._cached_system_prompt = "system"
|
||||
state.agent.tools = [{"type": "function", "function": {"name": "demo"}}]
|
||||
|
||||
with patch(
|
||||
"agent.model_metadata.estimate_request_tokens_rough",
|
||||
return_value=25_000,
|
||||
):
|
||||
update = agent._build_usage_update(state)
|
||||
|
||||
assert isinstance(update, UsageUpdate)
|
||||
assert update.session_update == "usage_update"
|
||||
assert update.size == 100_000
|
||||
assert update.used == 25_000
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_usage_update_to_client(self, agent, mock_manager):
|
||||
state = mock_manager.create_session(cwd="/tmp")
|
||||
state.agent.context_compressor = MagicMock(context_length=100_000)
|
||||
mock_conn = MagicMock(spec=acp.Client)
|
||||
mock_conn.session_update = AsyncMock()
|
||||
agent._conn = mock_conn
|
||||
|
||||
with patch(
|
||||
"agent.model_metadata.estimate_request_tokens_rough",
|
||||
return_value=25_000,
|
||||
):
|
||||
await agent._send_usage_update(state)
|
||||
|
||||
mock_conn.session_update.assert_awaited_once()
|
||||
call = mock_conn.session_update.await_args
|
||||
assert call.kwargs["session_id"] == state.session_id
|
||||
update = call.kwargs["update"]
|
||||
assert isinstance(update, UsageUpdate)
|
||||
assert update.size == 100_000
|
||||
assert update.used == 25_000
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cancel_sets_event(self, agent):
|
||||
resp = await agent.new_session(cwd=".")
|
||||
@@ -238,11 +283,31 @@ class TestSessionOps:
|
||||
{"role": "system", "content": "hidden system"},
|
||||
{"role": "user", "content": "what controls the / slash commands?"},
|
||||
{"role": "assistant", "content": "HermesACPAgent._ADVERTISED_COMMANDS controls them."},
|
||||
{"role": "tool", "content": "tool output should not replay"},
|
||||
{
|
||||
"role": "assistant",
|
||||
"content": "",
|
||||
"tool_calls": [
|
||||
{
|
||||
"id": "call_search_1",
|
||||
"type": "function",
|
||||
"function": {
|
||||
"name": "search_files",
|
||||
"arguments": '{"pattern":"slash commands","path":"."}',
|
||||
},
|
||||
}
|
||||
],
|
||||
},
|
||||
{
|
||||
"role": "tool",
|
||||
"tool_call_id": "call_search_1",
|
||||
"content": '{"total_count":1,"matches":[{"path":"cli.py","line":42,"content":"slash commands"}]}',
|
||||
},
|
||||
]
|
||||
|
||||
mock_conn.session_update.reset_mock()
|
||||
resp = await agent.load_session(cwd="/tmp", session_id=new_resp.session_id)
|
||||
await asyncio.sleep(0)
|
||||
await asyncio.sleep(0)
|
||||
|
||||
assert isinstance(resp, LoadSessionResponse)
|
||||
calls = mock_conn.session_update.await_args_list
|
||||
@@ -257,6 +322,21 @@ class TestSessionOps:
|
||||
assert isinstance(replay_calls[1].kwargs["update"], AgentMessageChunk)
|
||||
assert replay_calls[1].kwargs["update"].content.text.startswith("HermesACPAgent")
|
||||
|
||||
tool_updates = [
|
||||
call.kwargs["update"]
|
||||
for call in calls
|
||||
if getattr(call.kwargs.get("update"), "session_update", None)
|
||||
in {"tool_call", "tool_call_update"}
|
||||
]
|
||||
assert len(tool_updates) == 2
|
||||
assert isinstance(tool_updates[0], ToolCallStart)
|
||||
assert tool_updates[0].tool_call_id == "call_search_1"
|
||||
assert tool_updates[0].title == "search: slash commands"
|
||||
assert isinstance(tool_updates[1], ToolCallProgress)
|
||||
assert tool_updates[1].tool_call_id == "call_search_1"
|
||||
assert "Search results" in tool_updates[1].content[0].content.text
|
||||
assert "cli.py:42" in tool_updates[1].content[0].content.text
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_resume_session_replays_persisted_history_to_client(self, agent):
|
||||
mock_conn = MagicMock(spec=acp.Client)
|
||||
@@ -269,6 +349,8 @@ class TestSessionOps:
|
||||
|
||||
mock_conn.session_update.reset_mock()
|
||||
resp = await agent.resume_session(cwd="/tmp", session_id=new_resp.session_id)
|
||||
await asyncio.sleep(0)
|
||||
await asyncio.sleep(0)
|
||||
|
||||
assert isinstance(resp, ResumeSessionResponse)
|
||||
updates = [call.kwargs["update"] for call in mock_conn.session_update.await_args_list]
|
||||
@@ -278,6 +360,27 @@ class TestSessionOps:
|
||||
for update in updates
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_load_session_schedules_history_replay_after_response(self, agent):
|
||||
"""Zed only attaches replayed updates after session/load has completed."""
|
||||
new_resp = await agent.new_session(cwd="/tmp")
|
||||
state = agent.session_manager.get_session(new_resp.session_id)
|
||||
state.history = [{"role": "user", "content": "hello from history"}]
|
||||
events = []
|
||||
|
||||
async def replay_after_response(_state):
|
||||
events.append("replay")
|
||||
|
||||
with patch.object(agent, "_replay_session_history", side_effect=replay_after_response):
|
||||
resp = await agent.load_session(cwd="/tmp", session_id=new_resp.session_id)
|
||||
events.append("returned")
|
||||
|
||||
assert isinstance(resp, LoadSessionResponse)
|
||||
assert events == ["returned"]
|
||||
await asyncio.sleep(0)
|
||||
await asyncio.sleep(0)
|
||||
assert events == ["returned", "replay"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_resume_session_creates_new_if_missing(self, agent):
|
||||
resume_resp = await agent.resume_session(cwd="/tmp", session_id="nonexistent")
|
||||
@@ -522,6 +625,11 @@ class TestPrompt:
|
||||
assert isinstance(resp, PromptResponse)
|
||||
assert resp.stop_reason == "end_turn"
|
||||
state.agent.run_conversation.assert_called_once()
|
||||
assert state.agent.tool_progress_callback is not None
|
||||
assert state.agent.step_callback is not None
|
||||
assert state.agent.stream_delta_callback is not None
|
||||
assert state.agent.reasoning_callback is not None
|
||||
assert state.agent.thinking_callback is None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_prompt_updates_history(self, agent):
|
||||
@@ -565,12 +673,40 @@ class TestPrompt:
|
||||
prompt = [TextContentBlock(type="text", text="help me")]
|
||||
await agent.prompt(prompt=prompt, session_id=new_resp.session_id)
|
||||
|
||||
# session_update should have been called with the final message
|
||||
# session_update should include the final message (usage_update may follow it)
|
||||
mock_conn.session_update.assert_called()
|
||||
# Get the last call's update argument
|
||||
last_call = mock_conn.session_update.call_args_list[-1]
|
||||
update = last_call[1].get("update") or last_call[0][1]
|
||||
assert update.session_update == "agent_message_chunk"
|
||||
updates = [
|
||||
call.kwargs.get("update") or call.args[1]
|
||||
for call in mock_conn.session_update.call_args_list
|
||||
]
|
||||
assert any(update.session_update == "agent_message_chunk" for update in updates)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_prompt_does_not_duplicate_streamed_final_message(self, agent):
|
||||
"""If ACP already streamed response chunks, final_response should not be sent again."""
|
||||
new_resp = await agent.new_session(cwd=".")
|
||||
state = agent.session_manager.get_session(new_resp.session_id)
|
||||
|
||||
def mock_run(*args, **kwargs):
|
||||
state.agent.stream_delta_callback("streamed answer")
|
||||
return {"final_response": "streamed answer", "messages": []}
|
||||
|
||||
state.agent.run_conversation = mock_run
|
||||
|
||||
mock_conn = MagicMock(spec=acp.Client)
|
||||
mock_conn.session_update = AsyncMock()
|
||||
agent._conn = mock_conn
|
||||
|
||||
prompt = [TextContentBlock(type="text", text="hello")]
|
||||
await agent.prompt(prompt=prompt, session_id=new_resp.session_id)
|
||||
|
||||
updates = [
|
||||
call.kwargs.get("update") or call.args[1]
|
||||
for call in mock_conn.session_update.call_args_list
|
||||
]
|
||||
agent_chunks = [update for update in updates if update.session_update == "agent_message_chunk"]
|
||||
assert len(agent_chunks) == 1
|
||||
assert agent_chunks[0].content.text == "streamed answer"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_prompt_auto_titles_session(self, agent):
|
||||
@@ -708,6 +844,43 @@ class TestSlashCommands:
|
||||
assert "2 messages" in result
|
||||
assert "user: 1" in result
|
||||
|
||||
def test_context_shows_usage_and_compression_threshold(self, agent, mock_manager):
|
||||
state = self._make_state(mock_manager)
|
||||
state.history = [{"role": "user", "content": "hello"}]
|
||||
state.agent.context_compressor = MagicMock(
|
||||
context_length=100_000,
|
||||
threshold_tokens=80_000,
|
||||
)
|
||||
state.agent._cached_system_prompt = "system"
|
||||
state.agent.tools = [{"type": "function", "function": {"name": "demo"}}]
|
||||
|
||||
with patch(
|
||||
"agent.model_metadata.estimate_request_tokens_rough",
|
||||
return_value=25_000,
|
||||
):
|
||||
result = agent._handle_slash_command("/context", state)
|
||||
|
||||
assert "Context usage: ~25,000 / 100,000 tokens (25.0%)" in result
|
||||
assert "Compression: ~55,000 tokens until threshold (~80,000, 80%)" in result
|
||||
assert "Tip: run /compact" in result
|
||||
|
||||
def test_context_says_compression_due_when_past_threshold(self, agent, mock_manager):
|
||||
state = self._make_state(mock_manager)
|
||||
state.history = [{"role": "user", "content": "hello"}]
|
||||
state.agent.context_compressor = MagicMock(
|
||||
context_length=100_000,
|
||||
threshold_tokens=80_000,
|
||||
)
|
||||
|
||||
with patch(
|
||||
"agent.model_metadata.estimate_request_tokens_rough",
|
||||
return_value=82_000,
|
||||
):
|
||||
result = agent._handle_slash_command("/context", state)
|
||||
|
||||
assert "Context usage: ~82,000 / 100,000 tokens (82.0%)" in result
|
||||
assert "Compression: due now (threshold ~80,000, 80%). Run /compact." in result
|
||||
|
||||
def test_reset_clears_history(self, agent, mock_manager):
|
||||
state = self._make_state(mock_manager)
|
||||
state.history = [{"role": "user", "content": "hello"}]
|
||||
@@ -787,7 +960,12 @@ class TestSlashCommands:
|
||||
resp = await agent.prompt(prompt=prompt, session_id=new_resp.session_id)
|
||||
|
||||
assert resp.stop_reason == "end_turn"
|
||||
mock_conn.session_update.assert_called_once()
|
||||
updates = [
|
||||
call.kwargs.get("update") or call.args[1]
|
||||
for call in mock_conn.session_update.call_args_list
|
||||
]
|
||||
assert any(update.session_update == "agent_message_chunk" for update in updates)
|
||||
assert any(update.session_update == "usage_update" for update in updates)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_unknown_slash_falls_through_to_llm(self, agent, mock_manager):
|
||||
|
||||
+232
-5
@@ -52,6 +52,12 @@ class TestToolKindMap:
|
||||
def test_tool_kind_execute_code(self):
|
||||
assert get_tool_kind("execute_code") == "execute"
|
||||
|
||||
def test_tool_kind_todo(self):
|
||||
assert get_tool_kind("todo") == "other"
|
||||
|
||||
def test_tool_kind_skill_view(self):
|
||||
assert get_tool_kind("skill_view") == "read"
|
||||
|
||||
def test_tool_kind_browser_navigate(self):
|
||||
assert get_tool_kind("browser_navigate") == "fetch"
|
||||
|
||||
@@ -110,6 +116,25 @@ class TestBuildToolTitle:
|
||||
title = build_tool_title("web_search", {"query": "python asyncio"})
|
||||
assert "python asyncio" in title
|
||||
|
||||
def test_skill_view_title_includes_skill_name(self):
|
||||
title = build_tool_title("skill_view", {"name": "github-pitfalls"})
|
||||
assert title == "skill view (github-pitfalls)"
|
||||
|
||||
def test_skill_view_title_includes_linked_file(self):
|
||||
title = build_tool_title("skill_view", {"name": "github-pitfalls", "file_path": "references/api.md"})
|
||||
assert title == "skill view (github-pitfalls/references/api.md)"
|
||||
|
||||
def test_execute_code_title_includes_first_code_line(self):
|
||||
title = build_tool_title("execute_code", {"code": "\nfrom hermes_tools import terminal\nprint('done')"})
|
||||
assert title == "python: from hermes_tools import terminal"
|
||||
|
||||
def test_skill_manage_title_includes_action_and_target(self):
|
||||
title = build_tool_title(
|
||||
"skill_manage",
|
||||
{"action": "patch", "name": "hermes-agent-operations", "file_path": "references/acp.md"},
|
||||
)
|
||||
assert title == "skill patch: hermes-agent-operations/references/acp.md"
|
||||
|
||||
def test_unknown_tool_uses_name(self):
|
||||
title = build_tool_title("some_new_tool", {"foo": "bar"})
|
||||
assert title == "some_new_tool"
|
||||
@@ -164,15 +189,23 @@ class TestBuildToolStart:
|
||||
assert "ls -la /tmp" in text
|
||||
|
||||
def test_build_tool_start_for_read_file(self):
|
||||
"""read_file should include the path in content."""
|
||||
"""read_file start should stay compact; completion carries file contents."""
|
||||
args = {"path": "/etc/hosts", "offset": 1, "limit": 50}
|
||||
result = build_tool_start("tc-3", "read_file", args)
|
||||
assert isinstance(result, ToolCallStart)
|
||||
assert result.kind == "read"
|
||||
assert len(result.content) >= 1
|
||||
content_item = result.content[0]
|
||||
assert isinstance(content_item, ContentToolCallContent)
|
||||
assert "/etc/hosts" in content_item.content.text
|
||||
assert result.content is None
|
||||
assert result.raw_input is None
|
||||
|
||||
def test_build_tool_start_for_web_extract_is_compact(self):
|
||||
"""web_extract start should stay compact; title identifies URLs."""
|
||||
args = {"urls": ["https://example.com/docs"]}
|
||||
result = build_tool_start("tc-web-start", "web_extract", args)
|
||||
assert isinstance(result, ToolCallStart)
|
||||
assert result.title == "extract: https://example.com/docs"
|
||||
assert result.kind == "fetch"
|
||||
assert result.content is None
|
||||
assert result.raw_input is None
|
||||
|
||||
def test_build_tool_start_for_search(self):
|
||||
"""search_files should include pattern in content."""
|
||||
@@ -181,6 +214,48 @@ class TestBuildToolStart:
|
||||
assert isinstance(result, ToolCallStart)
|
||||
assert result.kind == "search"
|
||||
assert "TODO" in result.content[0].content.text
|
||||
assert result.raw_input is None
|
||||
|
||||
def test_build_tool_start_for_todo_is_human_readable(self):
|
||||
args = {"todos": [{"id": "one", "content": "Fix ACP rendering", "status": "in_progress"}]}
|
||||
result = build_tool_start("tc-todo", "todo", args)
|
||||
assert result.title == "todo (1 item)"
|
||||
assert "Fix ACP rendering" in result.content[0].content.text
|
||||
assert result.raw_input is None
|
||||
|
||||
def test_build_tool_start_for_skill_view_is_human_readable(self):
|
||||
result = build_tool_start("tc-skill", "skill_view", {"name": "github-pitfalls"})
|
||||
assert result.title == "skill view (github-pitfalls)"
|
||||
assert "github-pitfalls" in result.content[0].content.text
|
||||
assert result.raw_input is None
|
||||
|
||||
def test_build_tool_start_for_execute_code_shows_code_preview(self):
|
||||
result = build_tool_start("tc-code", "execute_code", {"code": "print('hello')"})
|
||||
assert result.kind == "execute"
|
||||
assert result.title == "python: print('hello')"
|
||||
assert "```python" in result.content[0].content.text
|
||||
assert "print('hello')" in result.content[0].content.text
|
||||
assert result.raw_input is None
|
||||
|
||||
def test_build_tool_start_for_skill_manage_patch_shows_diff(self):
|
||||
result = build_tool_start(
|
||||
"tc-skill-manage",
|
||||
"skill_manage",
|
||||
{
|
||||
"action": "patch",
|
||||
"name": "hermes-agent-operations",
|
||||
"file_path": "references/acp.md",
|
||||
"old_string": "old advice",
|
||||
"new_string": "new advice",
|
||||
},
|
||||
)
|
||||
assert result.kind == "edit"
|
||||
assert result.title == "skill patch: hermes-agent-operations/references/acp.md"
|
||||
assert isinstance(result.content[0], FileEditToolCallContent)
|
||||
assert result.content[0].path == "skills/hermes-agent-operations/references/acp.md"
|
||||
assert result.content[0].old_text == "old advice"
|
||||
assert result.content[0].new_text == "new advice"
|
||||
assert result.raw_input is None
|
||||
|
||||
def test_build_tool_start_generic_fallback(self):
|
||||
"""Unknown tools should get a generic text representation."""
|
||||
@@ -205,6 +280,158 @@ class TestBuildToolComplete:
|
||||
content_item = result.content[0]
|
||||
assert isinstance(content_item, ContentToolCallContent)
|
||||
assert "total 42" in content_item.content.text
|
||||
assert result.raw_output is None
|
||||
|
||||
def test_build_tool_complete_for_todo_is_checklist(self):
|
||||
result = build_tool_complete(
|
||||
"tc-todo",
|
||||
"todo",
|
||||
'{"todos":[{"id":"a","content":"Inspect ACP","status":"completed"},{"id":"b","content":"Patch renderers","status":"in_progress"}],"summary":{"total":2,"pending":0,"in_progress":1,"completed":1,"cancelled":0}}',
|
||||
)
|
||||
text = result.content[0].content.text
|
||||
assert "✅ Inspect ACP" in text
|
||||
assert "- 🔄 Patch renderers" in text
|
||||
assert "**Progress:** 1 completed, 1 in progress, 0 pending" in text
|
||||
assert result.raw_output is None
|
||||
|
||||
def test_build_tool_complete_for_skill_view_summarizes_content_without_raw_json(self):
|
||||
result = build_tool_complete(
|
||||
"tc-skill",
|
||||
"skill_view",
|
||||
'{"success":true,"name":"github-pitfalls","description":"GitHub gotchas","content":"# GitHub Pitfalls\\nUse gh carefully.","path":"github/github-pitfalls/SKILL.md"}',
|
||||
)
|
||||
text = result.content[0].content.text
|
||||
assert "**Skill loaded**" in text
|
||||
assert "`github-pitfalls`" in text
|
||||
assert "GitHub gotchas" in text
|
||||
assert "GitHub Pitfalls" in text
|
||||
assert "Use gh carefully" not in text
|
||||
assert "Full skill content is available to the agent" in text
|
||||
assert result.raw_output is None
|
||||
|
||||
def test_build_tool_complete_for_execute_code_formats_output(self):
|
||||
result = build_tool_complete("tc-code", "execute_code", '{"output":"hello\\n","exit_code":0}')
|
||||
text = result.content[0].content.text
|
||||
assert "Exit code: 0" in text
|
||||
assert "hello" in text
|
||||
assert result.raw_output is None
|
||||
|
||||
def test_build_tool_complete_for_skill_manage_summarizes_without_raw_json(self):
|
||||
result = build_tool_complete(
|
||||
"tc-skill-manage",
|
||||
"skill_manage",
|
||||
'{"success":true,"message":"Patched references/hermes-acp-zed-rendering.md in skill \'hermes-agent-operations\' (1 replacement)."}',
|
||||
function_args={
|
||||
"action": "patch",
|
||||
"name": "hermes-agent-operations",
|
||||
"file_path": "references/hermes-acp-zed-rendering.md",
|
||||
},
|
||||
)
|
||||
text = result.content[0].content.text
|
||||
assert "**✅ Skill updated**" in text
|
||||
assert "`patch`" in text
|
||||
assert "`hermes-agent-operations`" in text
|
||||
assert "references/hermes-acp-zed-rendering.md" in text
|
||||
assert "{\"success\"" not in text
|
||||
assert result.raw_output is None
|
||||
|
||||
def test_build_tool_complete_for_read_file_formats_content(self):
|
||||
result = build_tool_complete(
|
||||
"tc-read",
|
||||
"read_file",
|
||||
'{"content":"1|hello\\n2|world","total_lines":2}',
|
||||
function_args={"path":"README.md","offset":1,"limit":20},
|
||||
)
|
||||
text = result.content[0].content.text
|
||||
assert "Read README.md" in text
|
||||
assert "```\n1|hello\n2|world\n```" in text
|
||||
assert result.raw_output is None
|
||||
|
||||
def test_build_tool_complete_for_search_files_formats_matches(self):
|
||||
result = build_tool_complete(
|
||||
"tc-search",
|
||||
"search_files",
|
||||
'{"total_count":2,"matches":[{"path":"README.md","line":3,"content":"TODO: fix this"},{"path":"src/app.py","line":9,"content":"needle"}],"truncated":true}\n\n[Hint: Results truncated. Use offset=12 to see more.]',
|
||||
)
|
||||
text = result.content[0].content.text
|
||||
assert "Search results" in text
|
||||
assert "Found 2 matches" in text
|
||||
assert "README.md:3" in text
|
||||
assert "TODO: fix this" in text
|
||||
assert "Results truncated" in text
|
||||
assert result.raw_output is None
|
||||
|
||||
def test_build_tool_complete_for_process_list_formats_table(self):
|
||||
result = build_tool_complete(
|
||||
"tc-process",
|
||||
"process",
|
||||
'{"processes":[{"session_id":"p1","status":"running","pid":123,"command":"npm run dev"}]}',
|
||||
function_args={"action":"list"},
|
||||
)
|
||||
text = result.content[0].content.text
|
||||
assert "Processes: 1" in text
|
||||
assert "`p1`" in text
|
||||
assert "npm run dev" in text
|
||||
assert result.raw_output is None
|
||||
|
||||
def test_build_tool_complete_for_delegate_task_summarizes_children(self):
|
||||
result = build_tool_complete(
|
||||
"tc-delegate",
|
||||
"delegate_task",
|
||||
'{"results":[{"task_index":0,"status":"completed","summary":"Reviewed ACP rendering.","model":"gpt-5.5","duration_seconds":3.2,"tool_trace":[{"tool":"read_file"}]}],"total_duration_seconds":3.4}',
|
||||
)
|
||||
text = result.content[0].content.text
|
||||
assert "Delegation results: 1 task" in text
|
||||
assert "Reviewed ACP rendering" in text
|
||||
assert "gpt-5.5" in text
|
||||
assert "Tools: read_file" in text
|
||||
assert result.raw_output is None
|
||||
|
||||
def test_build_tool_complete_for_session_search_recent(self):
|
||||
result = build_tool_complete(
|
||||
"tc-session",
|
||||
"session_search",
|
||||
'{"success":true,"mode":"recent","results":[{"session_id":"s1","title":"ACP work","last_active":"2026-05-02","message_count":12,"preview":"Polished tool rendering."}],"count":1}',
|
||||
)
|
||||
text = result.content[0].content.text
|
||||
assert "Recent sessions" in text
|
||||
assert "ACP work" in text
|
||||
assert "Polished tool rendering" in text
|
||||
assert result.raw_output is None
|
||||
|
||||
def test_build_tool_complete_for_memory_avoids_dumping_entries(self):
|
||||
result = build_tool_complete(
|
||||
"tc-memory",
|
||||
"memory",
|
||||
'{"success":true,"target":"user","entries":["private long memory"],"usage":"1% — 19/2000 chars","entry_count":1,"message":"Entry added."}',
|
||||
function_args={"action":"add","target":"user","content":"User likes concise ACP rendering."},
|
||||
)
|
||||
text = result.content[0].content.text
|
||||
assert "Memory add saved" in text
|
||||
assert "User likes concise ACP rendering" in text
|
||||
assert "private long memory" not in text
|
||||
assert result.raw_output is None
|
||||
|
||||
def test_build_tool_complete_for_web_extract_success_stays_compact(self):
|
||||
result = build_tool_complete(
|
||||
"tc-web-extract",
|
||||
"web_extract",
|
||||
'{"results":[{"url":"https://example.com","title":"Example","content":"# Intro\\nThis is extracted content."}]}',
|
||||
)
|
||||
assert result.content is None
|
||||
assert result.raw_output is None
|
||||
|
||||
def test_build_tool_complete_for_web_extract_error_shows_error(self):
|
||||
result = build_tool_complete(
|
||||
"tc-web-extract-error",
|
||||
"web_extract",
|
||||
'{"results":[{"url":"https://example.com","title":"Example","error":"timeout"}]}',
|
||||
)
|
||||
text = result.content[0].content.text
|
||||
assert "Web extract failed" in text
|
||||
assert "https://example.com" in text
|
||||
assert "timeout" in text
|
||||
assert result.raw_output is None
|
||||
|
||||
def test_build_tool_complete_truncates_large_output(self):
|
||||
"""Very large outputs should be truncated."""
|
||||
|
||||
@@ -1836,3 +1836,55 @@ class TestResolveMessagesMaxTokens:
|
||||
result = _resolve_anthropic_messages_max_tokens(0.5, "claude-opus-4-6")
|
||||
assert result > 0
|
||||
assert result != 0
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# convert_tools_to_anthropic — tool dedup at API boundary
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestConvertToolsToAnthropicDedup:
|
||||
"""convert_tools_to_anthropic must deduplicate tool names.
|
||||
|
||||
Anthropic rejects requests with duplicate tool names. This guard converts
|
||||
a hard failure into a warning log. See:
|
||||
https://github.com/NousResearch/hermes-agent/issues/18478
|
||||
"""
|
||||
|
||||
def _make_openai_tool(self, name: str) -> dict:
|
||||
return {
|
||||
"type": "function",
|
||||
"function": {
|
||||
"name": name,
|
||||
"description": f"Tool {name}",
|
||||
"parameters": {"type": "object", "properties": {}},
|
||||
},
|
||||
}
|
||||
|
||||
def test_unique_tools_pass_through(self):
|
||||
tools = [self._make_openai_tool("alpha"), self._make_openai_tool("beta")]
|
||||
result = convert_tools_to_anthropic(tools)
|
||||
assert len(result) == 2
|
||||
names = [t["name"] for t in result]
|
||||
assert names == ["alpha", "beta"]
|
||||
|
||||
def test_duplicate_tool_names_are_deduplicated(self):
|
||||
"""RED test — must fail until dedup guard is added."""
|
||||
tools = [
|
||||
self._make_openai_tool("lcm_grep"),
|
||||
self._make_openai_tool("lcm_describe"),
|
||||
self._make_openai_tool("lcm_grep"), # duplicate
|
||||
self._make_openai_tool("lcm_expand"),
|
||||
self._make_openai_tool("lcm_describe"), # duplicate
|
||||
]
|
||||
result = convert_tools_to_anthropic(tools)
|
||||
names = [t["name"] for t in result]
|
||||
assert len(names) == len(set(names)), (
|
||||
f"Duplicate tool names found: {names}"
|
||||
)
|
||||
assert len(result) == 3 # lcm_grep, lcm_describe, lcm_expand
|
||||
|
||||
def test_empty_tools_returns_empty(self):
|
||||
assert convert_tools_to_anthropic([]) == []
|
||||
|
||||
def test_none_tools_returns_empty(self):
|
||||
assert convert_tools_to_anthropic(None) == []
|
||||
|
||||
@@ -16,6 +16,7 @@ from agent.auxiliary_client import (
|
||||
auxiliary_max_tokens_param,
|
||||
call_llm,
|
||||
async_call_llm,
|
||||
_build_call_kwargs,
|
||||
_read_codex_access_token,
|
||||
_get_provider_chain,
|
||||
_is_payment_error,
|
||||
@@ -1752,3 +1753,143 @@ class TestVisionAutoSkipsKimiCoding:
|
||||
"kimi-coding",
|
||||
"kimi-coding-cn",
|
||||
})
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _build_call_kwargs — tool dedup at API boundary
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestBuildCallKwargsToolDedup:
|
||||
"""_build_call_kwargs must deduplicate tool names before passing to API.
|
||||
|
||||
Providers like Google Vertex, Azure, and Bedrock reject requests with
|
||||
duplicate tool names (HTTP 400). This guard converts a hard failure into
|
||||
a warning log so agent turns succeed even if an upstream injection path
|
||||
regresses. See: https://github.com/NousResearch/hermes-agent/issues/18478
|
||||
"""
|
||||
|
||||
def _make_tool(self, name: str) -> dict:
|
||||
return {
|
||||
"type": "function",
|
||||
"function": {
|
||||
"name": name,
|
||||
"description": f"Tool {name}",
|
||||
"parameters": {"type": "object", "properties": {}},
|
||||
},
|
||||
}
|
||||
|
||||
def test_unique_tools_pass_through_unchanged(self):
|
||||
tools = [self._make_tool("alpha"), self._make_tool("beta")]
|
||||
kwargs = _build_call_kwargs(
|
||||
provider="openai", model="gpt-4o", messages=[], tools=tools,
|
||||
)
|
||||
assert len(kwargs["tools"]) == 2
|
||||
names = [t["function"]["name"] for t in kwargs["tools"]]
|
||||
assert names == ["alpha", "beta"]
|
||||
|
||||
def test_duplicate_tool_names_are_deduplicated(self):
|
||||
"""RED test — must fail until dedup guard is added."""
|
||||
tools = [
|
||||
self._make_tool("lcm_grep"),
|
||||
self._make_tool("lcm_describe"),
|
||||
self._make_tool("lcm_grep"), # duplicate
|
||||
self._make_tool("lcm_expand"),
|
||||
self._make_tool("lcm_describe"), # duplicate
|
||||
]
|
||||
kwargs = _build_call_kwargs(
|
||||
provider="google", model="gemini-2.5-pro", messages=[], tools=tools,
|
||||
)
|
||||
result_tools = kwargs["tools"]
|
||||
names = [t["function"]["name"] for t in result_tools]
|
||||
# Must be deduplicated — no repeated names
|
||||
assert len(names) == len(set(names)), (
|
||||
f"Duplicate tool names found: {names}"
|
||||
)
|
||||
assert len(result_tools) == 3 # lcm_grep, lcm_describe, lcm_expand
|
||||
|
||||
def test_empty_tools_unchanged(self):
|
||||
kwargs = _build_call_kwargs(
|
||||
provider="openai", model="gpt-4o", messages=[], tools=[],
|
||||
)
|
||||
assert kwargs.get("tools") == [] or "tools" not in kwargs
|
||||
|
||||
def test_none_tools_unchanged(self):
|
||||
kwargs = _build_call_kwargs(
|
||||
provider="openai", model="gpt-4o", messages=[], tools=None,
|
||||
)
|
||||
assert "tools" not in kwargs
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _clean_env(monkeypatch):
|
||||
"""Strip provider env vars so each test starts clean."""
|
||||
for key in (
|
||||
"OPENROUTER_API_KEY", "OPENAI_BASE_URL", "OPENAI_API_KEY",
|
||||
):
|
||||
monkeypatch.delenv(key, raising=False)
|
||||
|
||||
|
||||
class TestOpenRouterExplicitApiKey:
|
||||
"""Test that explicit_api_key is correctly propagated to _try_openrouter()."""
|
||||
|
||||
def test_resolve_provider_client_passes_explicit_api_key_to_openrouter(
|
||||
self, monkeypatch
|
||||
):
|
||||
"""
|
||||
When resolve_provider_client() is called with explicit_api_key for OpenRouter,
|
||||
the explicit key should be passed to the OpenAI client instead of falling back
|
||||
to OPENROUTER_API_KEY env var.
|
||||
"""
|
||||
# Set up env var as fallback (should NOT be used when explicit_api_key is provided)
|
||||
monkeypatch.setenv("OPENROUTER_API_KEY", "env-fallback-key")
|
||||
|
||||
# Mock OpenAI to capture the api_key used
|
||||
mock_openai = MagicMock()
|
||||
mock_openai.return_value = MagicMock(name="openrouter-client")
|
||||
|
||||
with patch("agent.auxiliary_client.OpenAI", mock_openai):
|
||||
client, model = resolve_provider_client(
|
||||
provider="openrouter",
|
||||
explicit_api_key="explicit-pool-key",
|
||||
)
|
||||
|
||||
# Verify a client was created
|
||||
assert client is not None
|
||||
# Verify the explicit key was used, not the env var fallback
|
||||
mock_openai.assert_called_once()
|
||||
call_kwargs = mock_openai.call_args[1]
|
||||
assert call_kwargs["api_key"] == "explicit-pool-key", (
|
||||
f"Expected explicit_api_key to be passed, got: {call_kwargs['api_key']}"
|
||||
)
|
||||
assert call_kwargs["api_key"] != "env-fallback-key", (
|
||||
"Should NOT fall back to OPENROUTER_API_KEY when explicit_api_key is provided"
|
||||
)
|
||||
|
||||
def test_resolve_provider_client_without_explicit_api_key_falls_back_to_env(
|
||||
self, monkeypatch
|
||||
):
|
||||
"""
|
||||
When resolve_provider_client() is called WITHOUT explicit_api_key for OpenRouter,
|
||||
it should fall back to OPENROUTER_API_KEY env var.
|
||||
"""
|
||||
# Set up env var as fallback (should be used when explicit_api_key is NOT provided)
|
||||
monkeypatch.setenv("OPENROUTER_API_KEY", "env-fallback-key")
|
||||
|
||||
# Mock OpenAI to capture the api_key used
|
||||
mock_openai = MagicMock()
|
||||
mock_openai.return_value = MagicMock(name="openrouter-client")
|
||||
|
||||
with patch("agent.auxiliary_client.OpenAI", mock_openai):
|
||||
client, model = resolve_provider_client(
|
||||
provider="openrouter",
|
||||
explicit_api_key=None,
|
||||
)
|
||||
|
||||
# Verify a client was created
|
||||
assert client is not None
|
||||
# Verify the env var fallback was used
|
||||
mock_openai.assert_called_once()
|
||||
call_kwargs = mock_openai.call_args[1]
|
||||
assert call_kwargs["api_key"] == "env-fallback-key", (
|
||||
f"Expected env fallback key to be used when explicit_api_key is None, got: {call_kwargs['api_key']}"
|
||||
)
|
||||
|
||||
@@ -348,6 +348,64 @@ def test_load_pool_seeds_env_api_key(tmp_path, monkeypatch):
|
||||
assert entry.access_token == "sk-or-seeded"
|
||||
|
||||
|
||||
|
||||
def test_load_pool_prefers_dotenv_over_stale_os_environ(tmp_path, monkeypatch):
|
||||
"""Regression for #18254: stale OPENROUTER_API_KEY in os.environ (inherited
|
||||
from a parent shell) must NOT shadow the fresh key in ~/.hermes/.env when
|
||||
seeding the credential pool. Before the fix, `get_env_value()` preferred
|
||||
os.environ and silently wrote the stale value into auth.json, causing
|
||||
persistent 401 errors after key rotation.
|
||||
"""
|
||||
hermes_home = tmp_path / "hermes"
|
||||
hermes_home.mkdir()
|
||||
monkeypatch.setenv("HERMES_HOME", str(hermes_home))
|
||||
|
||||
# Simulate the bug: parent shell exported a stale test key
|
||||
monkeypatch.setenv("OPENROUTER_API_KEY", "sk-or-STALE-from-shell")
|
||||
|
||||
# User edited ~/.hermes/.env with the fresh key
|
||||
(hermes_home / ".env").write_text(
|
||||
"OPENROUTER_API_KEY=sk-or-FRESH-from-dotenv\n"
|
||||
)
|
||||
|
||||
_write_auth_store(tmp_path, {"version": 1, "providers": {}})
|
||||
|
||||
from agent.credential_pool import load_pool
|
||||
pool = load_pool("openrouter")
|
||||
entry = pool.select()
|
||||
|
||||
assert entry is not None
|
||||
assert entry.source == "env:OPENROUTER_API_KEY"
|
||||
# The fresh key from .env must win over the stale shell export
|
||||
assert entry.access_token == "sk-or-FRESH-from-dotenv", (
|
||||
f"Expected .env to win, got {entry.access_token!r}"
|
||||
)
|
||||
|
||||
|
||||
def test_load_pool_falls_back_to_os_environ_when_dotenv_empty(tmp_path, monkeypatch):
|
||||
"""When ~/.hermes/.env does not define OPENROUTER_API_KEY (typical Docker /
|
||||
K8s / systemd deployment), seeding must still pick up the key from
|
||||
os.environ. Guards against regressions that would break production
|
||||
deployments relying on runtime-injected env vars.
|
||||
"""
|
||||
hermes_home = tmp_path / "hermes"
|
||||
hermes_home.mkdir()
|
||||
monkeypatch.setenv("HERMES_HOME", str(hermes_home))
|
||||
monkeypatch.setenv("OPENROUTER_API_KEY", "sk-or-from-runtime-env")
|
||||
|
||||
# .env exists but does not define OPENROUTER_API_KEY
|
||||
(hermes_home / ".env").write_text("SOME_OTHER_VAR=unrelated\n")
|
||||
|
||||
_write_auth_store(tmp_path, {"version": 1, "providers": {}})
|
||||
|
||||
from agent.credential_pool import load_pool
|
||||
pool = load_pool("openrouter")
|
||||
entry = pool.select()
|
||||
|
||||
assert entry is not None
|
||||
assert entry.access_token == "sk-or-from-runtime-env"
|
||||
|
||||
|
||||
def test_load_pool_removes_stale_seeded_env_entry(tmp_path, monkeypatch):
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes"))
|
||||
monkeypatch.delenv("OPENROUTER_API_KEY", raising=False)
|
||||
|
||||
@@ -314,3 +314,281 @@ def test_dry_run_skips_snapshot(backup_env, monkeypatch):
|
||||
assert not any(r.get("reason") == "pre-curator-run" for r in rows), (
|
||||
"dry-run must not create a pre-run snapshot"
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# cron-jobs backup + rollback (the part issue #18671's follow-up adds)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _write_cron_jobs(home: Path, jobs: list) -> Path:
|
||||
"""Write a synthetic cron/jobs.json under HERMES_HOME. Returns the path.
|
||||
Mirrors cron.jobs.save_jobs() wrapper shape: `{"jobs": [...], "updated_at": ...}`.
|
||||
"""
|
||||
cron_dir = home / "cron"
|
||||
cron_dir.mkdir(parents=True, exist_ok=True)
|
||||
path = cron_dir / "jobs.json"
|
||||
path.write_text(
|
||||
json.dumps({"jobs": jobs, "updated_at": "2026-05-01T00:00:00Z"}, indent=2),
|
||||
encoding="utf-8",
|
||||
)
|
||||
return path
|
||||
|
||||
|
||||
def _reload_cron_jobs(home: Path):
|
||||
"""Reload cron.jobs so its module-level HERMES_DIR picks up the tmp HOME."""
|
||||
import hermes_constants
|
||||
importlib.reload(hermes_constants)
|
||||
if "cron.jobs" in sys.modules:
|
||||
import cron.jobs as _cj
|
||||
importlib.reload(_cj)
|
||||
else:
|
||||
import cron.jobs as _cj # noqa: F401
|
||||
import cron.jobs as cj
|
||||
return cj
|
||||
|
||||
|
||||
def test_snapshot_includes_cron_jobs(backup_env):
|
||||
"""With a cron/jobs.json present, snapshot writes cron-jobs.json and records it in manifest."""
|
||||
cb = backup_env["cb"]
|
||||
_write_skill(backup_env["skills"], "alpha")
|
||||
_write_cron_jobs(backup_env["home"], [
|
||||
{"id": "job-a", "name": "a", "schedule": "every 1h", "skills": ["alpha"]},
|
||||
{"id": "job-b", "name": "b", "schedule": "every 2h", "skill": "alpha"},
|
||||
])
|
||||
|
||||
snap = cb.snapshot_skills(reason="test")
|
||||
assert snap is not None
|
||||
assert (snap / cb.CRON_JOBS_FILENAME).exists()
|
||||
|
||||
mf = json.loads((snap / "manifest.json").read_text(encoding="utf-8"))
|
||||
assert mf["cron_jobs"]["backed_up"] is True
|
||||
assert mf["cron_jobs"]["jobs_count"] == 2
|
||||
|
||||
|
||||
def test_snapshot_without_cron_jobs_file_still_succeeds(backup_env):
|
||||
"""No cron/jobs.json on disk → snapshot succeeds, manifest records absence."""
|
||||
cb = backup_env["cb"]
|
||||
_write_skill(backup_env["skills"], "alpha")
|
||||
# Deliberately do not create ~/.hermes/cron/jobs.json
|
||||
|
||||
snap = cb.snapshot_skills(reason="test")
|
||||
assert snap is not None
|
||||
assert not (snap / cb.CRON_JOBS_FILENAME).exists()
|
||||
|
||||
mf = json.loads((snap / "manifest.json").read_text(encoding="utf-8"))
|
||||
assert mf["cron_jobs"]["backed_up"] is False
|
||||
assert "cron/jobs.json" in mf["cron_jobs"]["reason"]
|
||||
|
||||
|
||||
def test_snapshot_cron_jobs_malformed_json_still_captured(backup_env):
|
||||
"""Malformed jobs.json is still copied to the snapshot (fidelity over
|
||||
validation); the manifest notes the parse warning."""
|
||||
cb = backup_env["cb"]
|
||||
_write_skill(backup_env["skills"], "alpha")
|
||||
(backup_env["home"] / "cron").mkdir()
|
||||
(backup_env["home"] / "cron" / "jobs.json").write_text("{oh no", encoding="utf-8")
|
||||
|
||||
snap = cb.snapshot_skills(reason="test")
|
||||
assert snap is not None
|
||||
# Raw file was copied even though we couldn't parse it
|
||||
assert (snap / cb.CRON_JOBS_FILENAME).read_text() == "{oh no"
|
||||
|
||||
mf = json.loads((snap / "manifest.json").read_text(encoding="utf-8"))
|
||||
assert mf["cron_jobs"]["backed_up"] is True
|
||||
assert mf["cron_jobs"]["jobs_count"] == 0
|
||||
assert "parse_warning" in mf["cron_jobs"]
|
||||
|
||||
|
||||
def test_rollback_restores_cron_skill_links(backup_env):
|
||||
"""End-to-end: snapshot with job [alpha,beta], curator-style in-place
|
||||
rewrite to [umbrella], then rollback → skills restored to [alpha,beta]."""
|
||||
cb = backup_env["cb"]
|
||||
home = backup_env["home"]
|
||||
_write_skill(backup_env["skills"], "alpha")
|
||||
_write_skill(backup_env["skills"], "beta")
|
||||
_write_skill(backup_env["skills"], "umbrella")
|
||||
|
||||
cj = _reload_cron_jobs(home)
|
||||
cj.create_job(name="weekly", prompt="p", schedule="every 7d",
|
||||
skills=["alpha", "beta"])
|
||||
|
||||
snap = cb.snapshot_skills(reason="pre-curator-run")
|
||||
assert snap is not None
|
||||
|
||||
# Simulate the curator's in-place cron rewrite after consolidation
|
||||
cj.rewrite_skill_refs(
|
||||
consolidated={"alpha": "umbrella", "beta": "umbrella"},
|
||||
pruned=[],
|
||||
)
|
||||
live_after_curator = cj.load_jobs()
|
||||
assert live_after_curator[0]["skills"] == ["umbrella"]
|
||||
|
||||
# Now roll back
|
||||
ok, msg, _ = cb.rollback(backup_id=snap.name)
|
||||
assert ok, msg
|
||||
assert "cron links" in msg
|
||||
|
||||
live_after_rollback = cj.load_jobs()
|
||||
# skills restored; legacy `skill` mirror follows first element
|
||||
assert live_after_rollback[0]["skills"] == ["alpha", "beta"]
|
||||
|
||||
|
||||
def test_rollback_only_touches_skill_fields(backup_env):
|
||||
"""Every field other than skills/skill must remain untouched across rollback.
|
||||
Schedule, enabled, prompt, timestamps — all live state, hands off."""
|
||||
cb = backup_env["cb"]
|
||||
home = backup_env["home"]
|
||||
_write_skill(backup_env["skills"], "alpha")
|
||||
|
||||
# Hand-rolled jobs.json with varied fields (no real create_job — we want
|
||||
# exact field control).
|
||||
_write_cron_jobs(home, [{
|
||||
"id": "stable-id",
|
||||
"name": "original-name",
|
||||
"prompt": "original prompt",
|
||||
"schedule": "every 1h",
|
||||
"skills": ["alpha"],
|
||||
"enabled": True,
|
||||
"last_run_at": "2026-04-01T00:00:00Z",
|
||||
}])
|
||||
snap = cb.snapshot_skills(reason="pre-curator-run")
|
||||
assert snap is not None
|
||||
|
||||
# User/scheduler activity AFTER the snapshot: rename the job, change
|
||||
# the schedule, update timestamps, and (curator) rewrite the skills list.
|
||||
cj = _reload_cron_jobs(home)
|
||||
jobs = cj.load_jobs()
|
||||
jobs[0]["name"] = "renamed-since-snapshot"
|
||||
jobs[0]["schedule"] = "every 30m"
|
||||
jobs[0]["last_run_at"] = "2026-05-01T12:00:00Z"
|
||||
jobs[0]["skills"] = ["umbrella"] # pretend curator did this
|
||||
cj.save_jobs(jobs)
|
||||
|
||||
ok, _, _ = cb.rollback(backup_id=snap.name)
|
||||
assert ok
|
||||
|
||||
after = cj.load_jobs()
|
||||
job = after[0]
|
||||
# skills: restored
|
||||
assert job["skills"] == ["alpha"]
|
||||
# everything else: untouched (live state preserved)
|
||||
assert job["name"] == "renamed-since-snapshot"
|
||||
assert job["schedule"] == "every 30m"
|
||||
assert job["last_run_at"] == "2026-05-01T12:00:00Z"
|
||||
assert job["prompt"] == "original prompt"
|
||||
|
||||
|
||||
def test_rollback_skips_jobs_the_user_deleted(backup_env):
|
||||
"""If the user deleted a cron job after the snapshot, rollback must
|
||||
NOT resurrect it — the user's delete is a later, explicit choice."""
|
||||
cb = backup_env["cb"]
|
||||
home = backup_env["home"]
|
||||
_write_skill(backup_env["skills"], "alpha")
|
||||
|
||||
_write_cron_jobs(home, [
|
||||
{"id": "keep-me", "name": "keep", "schedule": "every 1h", "skills": ["alpha"]},
|
||||
{"id": "delete-me", "name": "gone", "schedule": "every 1h", "skills": ["alpha"]},
|
||||
])
|
||||
snap = cb.snapshot_skills(reason="pre-curator-run")
|
||||
|
||||
# User deletes one job after the snapshot
|
||||
cj = _reload_cron_jobs(home)
|
||||
cj.save_jobs([j for j in cj.load_jobs() if j["id"] != "delete-me"])
|
||||
|
||||
ok, _, _ = cb.rollback(backup_id=snap.name)
|
||||
assert ok
|
||||
|
||||
live_after = cj.load_jobs()
|
||||
live_ids = {j["id"] for j in live_after}
|
||||
assert "keep-me" in live_ids
|
||||
assert "delete-me" not in live_ids # not resurrected
|
||||
|
||||
|
||||
def test_rollback_leaves_new_jobs_untouched(backup_env):
|
||||
"""Jobs created AFTER the snapshot must pass through rollback unchanged."""
|
||||
cb = backup_env["cb"]
|
||||
home = backup_env["home"]
|
||||
_write_skill(backup_env["skills"], "alpha")
|
||||
_write_cron_jobs(home, [
|
||||
{"id": "original", "name": "o", "schedule": "every 1h", "skills": ["alpha"]},
|
||||
])
|
||||
snap = cb.snapshot_skills(reason="pre-curator-run")
|
||||
|
||||
cj = _reload_cron_jobs(home)
|
||||
jobs = cj.load_jobs()
|
||||
jobs.append({"id": "new-after-snapshot", "name": "new",
|
||||
"schedule": "every 15m", "skills": ["brand-new-skill"]})
|
||||
cj.save_jobs(jobs)
|
||||
|
||||
ok, _, _ = cb.rollback(backup_id=snap.name)
|
||||
assert ok
|
||||
|
||||
live = cj.load_jobs()
|
||||
by_id = {j["id"]: j for j in live}
|
||||
assert "new-after-snapshot" in by_id
|
||||
# New job's fields completely preserved
|
||||
assert by_id["new-after-snapshot"]["skills"] == ["brand-new-skill"]
|
||||
assert by_id["new-after-snapshot"]["schedule"] == "every 15m"
|
||||
|
||||
|
||||
def test_rollback_with_snapshot_missing_cron_succeeds(backup_env):
|
||||
"""Older snapshots (created before this feature shipped) have no
|
||||
cron-jobs.json. Rollback must still restore the skills tree and not
|
||||
error out."""
|
||||
cb = backup_env["cb"]
|
||||
home = backup_env["home"]
|
||||
_write_skill(backup_env["skills"], "alpha")
|
||||
|
||||
# No cron/jobs.json at snapshot time — simulates a pre-feature snapshot
|
||||
snap = cb.snapshot_skills(reason="test")
|
||||
assert snap is not None
|
||||
assert not (snap / cb.CRON_JOBS_FILENAME).exists()
|
||||
|
||||
# Later the user created a cron job
|
||||
_write_cron_jobs(home, [
|
||||
{"id": "later-job", "name": "l", "schedule": "every 1h", "skills": ["x"]},
|
||||
])
|
||||
|
||||
ok, msg, _ = cb.rollback(backup_id=snap.name)
|
||||
# Main rollback still succeeds; cron report notes the missing file.
|
||||
assert ok, msg
|
||||
# Jobs.json untouched (nothing to restore from)
|
||||
cj = _reload_cron_jobs(home)
|
||||
jobs = cj.load_jobs()
|
||||
assert jobs[0]["id"] == "later-job"
|
||||
assert jobs[0]["skills"] == ["x"]
|
||||
|
||||
|
||||
def test_restore_cron_skill_links_standalone(backup_env):
|
||||
"""Unit-level test on _restore_cron_skill_links without the full rollback.
|
||||
Verifies the report structure carefully."""
|
||||
cb = backup_env["cb"]
|
||||
home = backup_env["home"]
|
||||
|
||||
# Prime a snapshot dir manually with cron-jobs.json
|
||||
backups_dir = home / "skills" / ".curator_backups" / "fake-id"
|
||||
backups_dir.mkdir(parents=True)
|
||||
(backups_dir / cb.CRON_JOBS_FILENAME).write_text(json.dumps([
|
||||
{"id": "job-1", "name": "one", "skills": ["narrow-a", "narrow-b"]},
|
||||
{"id": "job-2", "name": "two", "skill": "legacy-single"},
|
||||
{"id": "job-gone", "name": "deleted", "skills": ["whatever"]},
|
||||
]), encoding="utf-8")
|
||||
|
||||
# Live jobs: job-1 got rewritten, job-2 unchanged, job-gone deleted
|
||||
_write_cron_jobs(home, [
|
||||
{"id": "job-1", "name": "one", "skills": ["umbrella"], "schedule": "every 1h"},
|
||||
{"id": "job-2", "name": "two", "skill": "legacy-single", "schedule": "every 1h"},
|
||||
{"id": "job-new", "name": "new", "skills": ["x"], "schedule": "every 1h"},
|
||||
])
|
||||
_reload_cron_jobs(home)
|
||||
|
||||
report = cb._restore_cron_skill_links(backups_dir)
|
||||
assert report["attempted"] is True
|
||||
assert report["error"] is None
|
||||
assert report["unchanged"] == 1 # job-2 matched
|
||||
assert len(report["restored"]) == 1 # job-1 got restored
|
||||
assert report["restored"][0]["job_id"] == "job-1"
|
||||
assert report["restored"][0]["to"]["skills"] == ["narrow-a", "narrow-b"]
|
||||
assert len(report["skipped_missing"]) == 1
|
||||
assert report["skipped_missing"][0]["job_id"] == "job-gone"
|
||||
|
||||
@@ -548,3 +548,266 @@ def test_reconcile_model_block_visible_in_full_report(curator_env):
|
||||
md = (run_dir / "REPORT.md").read_text()
|
||||
assert "duplicate content, now a subsection" in md
|
||||
assert "pre-curator junk" in md
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _extract_absorbed_into_declarations — authoritative signal from delete calls
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_extract_absorbed_into_picks_up_consolidation(curator_env):
|
||||
"""Delete call with absorbed_into=<umbrella> yields a declaration."""
|
||||
declarations = curator_env._extract_absorbed_into_declarations([
|
||||
{
|
||||
"name": "skill_manage",
|
||||
"arguments": json.dumps({
|
||||
"action": "delete",
|
||||
"name": "narrow-skill",
|
||||
"absorbed_into": "umbrella",
|
||||
}),
|
||||
},
|
||||
])
|
||||
assert declarations == {
|
||||
"narrow-skill": {"into": "umbrella", "declared": True},
|
||||
}
|
||||
|
||||
|
||||
def test_extract_absorbed_into_empty_string_is_explicit_prune(curator_env):
|
||||
"""absorbed_into='' is recorded as an explicit prune declaration."""
|
||||
declarations = curator_env._extract_absorbed_into_declarations([
|
||||
{
|
||||
"name": "skill_manage",
|
||||
"arguments": json.dumps({
|
||||
"action": "delete",
|
||||
"name": "stale",
|
||||
"absorbed_into": "",
|
||||
}),
|
||||
},
|
||||
])
|
||||
assert declarations == {"stale": {"into": "", "declared": True}}
|
||||
|
||||
|
||||
def test_extract_absorbed_into_missing_arg_ignored(curator_env):
|
||||
"""Delete call without absorbed_into is skipped — fallback to heuristic."""
|
||||
declarations = curator_env._extract_absorbed_into_declarations([
|
||||
{
|
||||
"name": "skill_manage",
|
||||
"arguments": json.dumps({
|
||||
"action": "delete",
|
||||
"name": "legacy-skill",
|
||||
}),
|
||||
},
|
||||
])
|
||||
assert declarations == {}
|
||||
|
||||
|
||||
def test_extract_absorbed_into_ignores_non_delete_actions(curator_env):
|
||||
"""Patch, create, write_file etc. must not leak into declarations."""
|
||||
declarations = curator_env._extract_absorbed_into_declarations([
|
||||
{
|
||||
"name": "skill_manage",
|
||||
"arguments": json.dumps({
|
||||
"action": "patch",
|
||||
"name": "umbrella",
|
||||
"old_string": "...",
|
||||
"new_string": "...",
|
||||
"absorbed_into": "something", # bogus on non-delete, must be ignored
|
||||
}),
|
||||
},
|
||||
])
|
||||
assert declarations == {}
|
||||
|
||||
|
||||
def test_extract_absorbed_into_accepts_dict_arguments(curator_env):
|
||||
"""arguments can arrive as a dict (defensive path) — still works."""
|
||||
declarations = curator_env._extract_absorbed_into_declarations([
|
||||
{
|
||||
"name": "skill_manage",
|
||||
"arguments": {
|
||||
"action": "delete",
|
||||
"name": "narrow",
|
||||
"absorbed_into": "umbrella",
|
||||
},
|
||||
},
|
||||
])
|
||||
assert declarations == {"narrow": {"into": "umbrella", "declared": True}}
|
||||
|
||||
|
||||
def test_extract_absorbed_into_strips_whitespace(curator_env):
|
||||
declarations = curator_env._extract_absorbed_into_declarations([
|
||||
{
|
||||
"name": "skill_manage",
|
||||
"arguments": json.dumps({
|
||||
"action": "delete",
|
||||
"name": " narrow ",
|
||||
"absorbed_into": " umbrella ",
|
||||
}),
|
||||
},
|
||||
])
|
||||
assert declarations == {"narrow": {"into": "umbrella", "declared": True}}
|
||||
|
||||
|
||||
def test_extract_absorbed_into_ignores_non_skill_manage_calls(curator_env):
|
||||
declarations = curator_env._extract_absorbed_into_declarations([
|
||||
{"name": "terminal", "arguments": json.dumps({"command": "ls"})},
|
||||
{"name": "read_file", "arguments": json.dumps({"path": "/tmp/x"})},
|
||||
])
|
||||
assert declarations == {}
|
||||
|
||||
|
||||
def test_extract_absorbed_into_handles_malformed_arguments(curator_env):
|
||||
"""Garbage JSON in arguments must not crash the extractor."""
|
||||
declarations = curator_env._extract_absorbed_into_declarations([
|
||||
{"name": "skill_manage", "arguments": "{not json"},
|
||||
{"name": "skill_manage", "arguments": None},
|
||||
{"name": "skill_manage"}, # no arguments key at all
|
||||
])
|
||||
assert declarations == {}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _reconcile_classification with absorbed_into declarations (authoritative)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_reconcile_absorbed_into_beats_everything_else(curator_env):
|
||||
"""Model declared absorbed_into at delete; YAML/heuristic disagree — declaration wins.
|
||||
|
||||
This is the exact #18671 regression: the model forgets to emit the YAML
|
||||
summary block, the heuristic's substring match misses because the
|
||||
umbrella's patch content doesn't literally contain the old skill's
|
||||
slug. Previously this fell through to 'no-evidence fallback' prune,
|
||||
which dropped the cron ref instead of rewriting. With absorbed_into
|
||||
declared, the model tells us directly.
|
||||
"""
|
||||
out = curator_env._reconcile_classification(
|
||||
removed=["pr-review-format"],
|
||||
heuristic={"consolidated": [], "pruned": [{"name": "pr-review-format"}]},
|
||||
model_block={"consolidations": [], "prunings": []}, # model forgot YAML block
|
||||
destinations={"hermes-agent-dev"},
|
||||
absorbed_declarations={
|
||||
"pr-review-format": {"into": "hermes-agent-dev", "declared": True},
|
||||
},
|
||||
)
|
||||
assert len(out["consolidated"]) == 1
|
||||
assert out["pruned"] == []
|
||||
e = out["consolidated"][0]
|
||||
assert e["name"] == "pr-review-format"
|
||||
assert e["into"] == "hermes-agent-dev"
|
||||
assert "absorbed_into" in e["source"]
|
||||
|
||||
|
||||
def test_reconcile_absorbed_into_empty_is_explicit_prune(curator_env):
|
||||
"""absorbed_into='' takes precedence and routes to pruned, not fallback."""
|
||||
out = curator_env._reconcile_classification(
|
||||
removed=["stale"],
|
||||
heuristic={"consolidated": [], "pruned": [{"name": "stale"}]},
|
||||
model_block={"consolidations": [], "prunings": []},
|
||||
destinations=set(),
|
||||
absorbed_declarations={
|
||||
"stale": {"into": "", "declared": True},
|
||||
},
|
||||
)
|
||||
assert out["consolidated"] == []
|
||||
assert len(out["pruned"]) == 1
|
||||
assert "model-declared prune" in out["pruned"][0]["source"]
|
||||
|
||||
|
||||
def test_reconcile_absorbed_into_nonexistent_target_falls_through(curator_env):
|
||||
"""If the declared umbrella doesn't exist in destinations, fall through to
|
||||
heuristic/YAML logic. Shouldn't happen in practice (the tool validates at
|
||||
delete time) but the reconciler is defensive."""
|
||||
out = curator_env._reconcile_classification(
|
||||
removed=["thing"],
|
||||
heuristic={
|
||||
"consolidated": [{"name": "thing", "into": "real-umbrella", "evidence": "..."}],
|
||||
"pruned": [],
|
||||
},
|
||||
model_block={"consolidations": [], "prunings": []},
|
||||
destinations={"real-umbrella"},
|
||||
absorbed_declarations={
|
||||
"thing": {"into": "ghost-umbrella", "declared": True},
|
||||
},
|
||||
)
|
||||
assert len(out["consolidated"]) == 1
|
||||
assert out["consolidated"][0]["into"] == "real-umbrella"
|
||||
assert "tool-call audit" in out["consolidated"][0]["source"]
|
||||
|
||||
|
||||
def test_reconcile_declaration_preserves_yaml_reason(curator_env):
|
||||
"""When the model both declared absorbed_into AND emitted YAML with reason,
|
||||
the reason carries through so REPORT.md still has it."""
|
||||
out = curator_env._reconcile_classification(
|
||||
removed=["narrow"],
|
||||
heuristic={"consolidated": [], "pruned": []},
|
||||
model_block={
|
||||
"consolidations": [{
|
||||
"from": "narrow",
|
||||
"into": "umbrella",
|
||||
"reason": "duplicate of umbrella's main content",
|
||||
}],
|
||||
"prunings": [],
|
||||
},
|
||||
destinations={"umbrella"},
|
||||
absorbed_declarations={
|
||||
"narrow": {"into": "umbrella", "declared": True},
|
||||
},
|
||||
)
|
||||
assert len(out["consolidated"]) == 1
|
||||
e = out["consolidated"][0]
|
||||
assert e["into"] == "umbrella"
|
||||
assert "absorbed_into" in e["source"]
|
||||
assert e["reason"] == "duplicate of umbrella's main content"
|
||||
|
||||
|
||||
def test_reconcile_without_declarations_preserves_legacy_behavior(curator_env):
|
||||
"""Backward compat: no absorbed_declarations arg → all existing logic intact."""
|
||||
out = curator_env._reconcile_classification(
|
||||
removed=["thing"],
|
||||
heuristic={
|
||||
"consolidated": [{"name": "thing", "into": "umbrella", "evidence": "..."}],
|
||||
"pruned": [],
|
||||
},
|
||||
model_block={"consolidations": [], "prunings": []},
|
||||
destinations={"umbrella"},
|
||||
# no absorbed_declarations — defaults to None → behaves identically to pre-change
|
||||
)
|
||||
assert len(out["consolidated"]) == 1
|
||||
assert out["consolidated"][0]["into"] == "umbrella"
|
||||
|
||||
|
||||
def test_reconcile_mixed_declarations_and_legacy_calls(curator_env):
|
||||
"""Real-world run: some deletes declared absorbed_into, some didn't.
|
||||
Declared ones use the authoritative path; others fall through to YAML/heuristic.
|
||||
"""
|
||||
out = curator_env._reconcile_classification(
|
||||
removed=["declared-cons", "declared-prune", "legacy-cons", "legacy-prune"],
|
||||
heuristic={
|
||||
"consolidated": [
|
||||
{"name": "legacy-cons", "into": "umbrella-a", "evidence": "..."},
|
||||
],
|
||||
"pruned": [{"name": "legacy-prune"}],
|
||||
},
|
||||
model_block={"consolidations": [], "prunings": []},
|
||||
destinations={"umbrella-a", "umbrella-b"},
|
||||
absorbed_declarations={
|
||||
"declared-cons": {"into": "umbrella-b", "declared": True},
|
||||
"declared-prune": {"into": "", "declared": True},
|
||||
},
|
||||
)
|
||||
cons_by_name = {e["name"]: e for e in out["consolidated"]}
|
||||
pruned_by_name = {e["name"]: e for e in out["pruned"]}
|
||||
|
||||
assert "declared-cons" in cons_by_name
|
||||
assert cons_by_name["declared-cons"]["into"] == "umbrella-b"
|
||||
assert "absorbed_into" in cons_by_name["declared-cons"]["source"]
|
||||
|
||||
assert "legacy-cons" in cons_by_name
|
||||
assert cons_by_name["legacy-cons"]["into"] == "umbrella-a"
|
||||
assert "tool-call audit" in cons_by_name["legacy-cons"]["source"]
|
||||
|
||||
assert "declared-prune" in pruned_by_name
|
||||
assert "model-declared prune" in pruned_by_name["declared-prune"]["source"]
|
||||
|
||||
assert "legacy-prune" in pruned_by_name
|
||||
assert "no-evidence fallback" in pruned_by_name["legacy-prune"]["source"]
|
||||
|
||||
@@ -0,0 +1,284 @@
|
||||
"""Tests for OpenRouter response caching header injection."""
|
||||
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# build_or_headers
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestBuildOrHeaders:
|
||||
"""Test the build_or_headers() helper in agent/auxiliary_client.py."""
|
||||
|
||||
def test_base_attribution_always_present(self):
|
||||
"""Attribution headers must always be included regardless of cache setting."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
headers = build_or_headers(or_config={"response_cache": False})
|
||||
assert headers["HTTP-Referer"] == "https://hermes-agent.nousresearch.com"
|
||||
assert headers["X-OpenRouter-Title"] == "Hermes Agent"
|
||||
assert headers["X-OpenRouter-Categories"] == "productivity,cli-agent"
|
||||
|
||||
def test_cache_enabled(self):
|
||||
"""When response_cache is True, X-OpenRouter-Cache header is set."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
headers = build_or_headers(or_config={"response_cache": True})
|
||||
assert headers["X-OpenRouter-Cache"] == "true"
|
||||
|
||||
def test_cache_disabled(self):
|
||||
"""When response_cache is False, no cache header is sent."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
headers = build_or_headers(or_config={"response_cache": False})
|
||||
assert "X-OpenRouter-Cache" not in headers
|
||||
assert "X-OpenRouter-Cache-TTL" not in headers
|
||||
|
||||
def test_cache_disabled_by_default_empty_config(self):
|
||||
"""Empty config dict means no cache headers (response_cache defaults to False)."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
headers = build_or_headers(or_config={})
|
||||
assert "X-OpenRouter-Cache" not in headers
|
||||
|
||||
def test_ttl_default(self):
|
||||
"""Default TTL (300) is included when cache is enabled."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
headers = build_or_headers(or_config={"response_cache": True, "response_cache_ttl": 300})
|
||||
assert headers["X-OpenRouter-Cache-TTL"] == "300"
|
||||
|
||||
def test_ttl_custom(self):
|
||||
"""Custom TTL values within range are sent."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
headers = build_or_headers(or_config={"response_cache": True, "response_cache_ttl": 3600})
|
||||
assert headers["X-OpenRouter-Cache-TTL"] == "3600"
|
||||
|
||||
def test_ttl_max(self):
|
||||
"""Maximum TTL (86400) is accepted."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
headers = build_or_headers(or_config={"response_cache": True, "response_cache_ttl": 86400})
|
||||
assert headers["X-OpenRouter-Cache-TTL"] == "86400"
|
||||
|
||||
def test_ttl_out_of_range_too_high(self):
|
||||
"""TTL above 86400 is silently ignored (no TTL header sent)."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
headers = build_or_headers(or_config={"response_cache": True, "response_cache_ttl": 100000})
|
||||
assert "X-OpenRouter-Cache-TTL" not in headers
|
||||
# But cache is still enabled
|
||||
assert headers["X-OpenRouter-Cache"] == "true"
|
||||
|
||||
def test_ttl_out_of_range_zero(self):
|
||||
"""TTL of 0 is below minimum — no TTL header sent."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
headers = build_or_headers(or_config={"response_cache": True, "response_cache_ttl": 0})
|
||||
assert "X-OpenRouter-Cache-TTL" not in headers
|
||||
|
||||
def test_ttl_negative(self):
|
||||
"""Negative TTL is ignored."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
headers = build_or_headers(or_config={"response_cache": True, "response_cache_ttl": -5})
|
||||
assert "X-OpenRouter-Cache-TTL" not in headers
|
||||
|
||||
def test_ttl_not_a_number(self):
|
||||
"""Non-numeric TTL is ignored."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
headers = build_or_headers(or_config={"response_cache": True, "response_cache_ttl": "five"})
|
||||
assert "X-OpenRouter-Cache-TTL" not in headers
|
||||
|
||||
def test_ttl_float_truncated(self):
|
||||
"""Float TTL values are truncated to int."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
headers = build_or_headers(or_config={"response_cache": True, "response_cache_ttl": 600.7})
|
||||
assert headers["X-OpenRouter-Cache-TTL"] == "600"
|
||||
|
||||
def test_returns_fresh_dict(self):
|
||||
"""Each call returns a new dict so mutations don't leak."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
cfg = {"response_cache": True}
|
||||
h1 = build_or_headers(or_config=cfg)
|
||||
h2 = build_or_headers(or_config=cfg)
|
||||
assert h1 is not h2
|
||||
assert h1 == h2
|
||||
|
||||
def test_none_config_falls_back_to_load_config(self):
|
||||
"""When or_config is None, build_or_headers reads from load_config()."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
fake_cfg = {
|
||||
"openrouter": {"response_cache": True, "response_cache_ttl": 900},
|
||||
}
|
||||
with patch("hermes_cli.config.load_config", return_value=fake_cfg):
|
||||
headers = build_or_headers(or_config=None)
|
||||
assert headers["X-OpenRouter-Cache"] == "true"
|
||||
assert headers["X-OpenRouter-Cache-TTL"] == "900"
|
||||
|
||||
def test_none_config_load_config_fails_gracefully(self):
|
||||
"""When load_config() fails, build_or_headers still returns base headers."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
with patch("hermes_cli.config.load_config", side_effect=RuntimeError("boom")):
|
||||
headers = build_or_headers(or_config=None)
|
||||
# Should have base attribution but no cache headers
|
||||
assert "HTTP-Referer" in headers
|
||||
assert "X-OpenRouter-Cache" not in headers
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Environment variable overrides
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestEnvVarOverrides:
|
||||
"""Test env var precedence over config.yaml for response caching."""
|
||||
|
||||
def test_env_enables_cache(self, monkeypatch):
|
||||
"""HERMES_OPENROUTER_CACHE=true enables cache even when config disables it."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
monkeypatch.setenv("HERMES_OPENROUTER_CACHE", "true")
|
||||
headers = build_or_headers(or_config={"response_cache": False})
|
||||
assert headers["X-OpenRouter-Cache"] == "true"
|
||||
|
||||
def test_env_disables_cache(self, monkeypatch):
|
||||
"""HERMES_OPENROUTER_CACHE=false disables cache even when config enables it."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
monkeypatch.setenv("HERMES_OPENROUTER_CACHE", "false")
|
||||
headers = build_or_headers(or_config={"response_cache": True})
|
||||
assert "X-OpenRouter-Cache" not in headers
|
||||
|
||||
@pytest.mark.parametrize("value", ["1", "true", "TRUE", "yes", "Yes", "on"])
|
||||
def test_truthy_values(self, monkeypatch, value):
|
||||
"""Various truthy strings enable caching."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
monkeypatch.setenv("HERMES_OPENROUTER_CACHE", value)
|
||||
headers = build_or_headers(or_config={})
|
||||
assert headers["X-OpenRouter-Cache"] == "true"
|
||||
|
||||
@pytest.mark.parametrize("value", ["0", "false", "no", "off", "maybe", ""])
|
||||
def test_non_truthy_values(self, monkeypatch, value):
|
||||
"""Non-truthy strings do not enable caching (empty falls through to config)."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
monkeypatch.setenv("HERMES_OPENROUTER_CACHE", value)
|
||||
# Empty string falls through to config; others are explicitly non-truthy
|
||||
if value == "":
|
||||
# Empty env var falls through to config default (False)
|
||||
headers = build_or_headers(or_config={"response_cache": False})
|
||||
else:
|
||||
headers = build_or_headers(or_config={"response_cache": True})
|
||||
assert "X-OpenRouter-Cache" not in headers
|
||||
|
||||
def test_env_ttl_overrides_config(self, monkeypatch):
|
||||
"""HERMES_OPENROUTER_CACHE_TTL overrides config TTL."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
monkeypatch.setenv("HERMES_OPENROUTER_CACHE", "true")
|
||||
monkeypatch.setenv("HERMES_OPENROUTER_CACHE_TTL", "1800")
|
||||
headers = build_or_headers(or_config={"response_cache_ttl": 300})
|
||||
assert headers["X-OpenRouter-Cache-TTL"] == "1800"
|
||||
|
||||
@pytest.mark.parametrize("ttl", ["0", "86401", "abc", "-1", "12.5"])
|
||||
def test_invalid_env_ttl_dropped(self, monkeypatch, ttl):
|
||||
"""Invalid TTL env values are ignored; cache still enabled without TTL."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
monkeypatch.setenv("HERMES_OPENROUTER_CACHE", "1")
|
||||
monkeypatch.setenv("HERMES_OPENROUTER_CACHE_TTL", ttl)
|
||||
headers = build_or_headers(or_config={})
|
||||
assert headers["X-OpenRouter-Cache"] == "true"
|
||||
assert "X-OpenRouter-Cache-TTL" not in headers
|
||||
|
||||
@pytest.mark.parametrize("ttl", ["1", "300", "86400"])
|
||||
def test_valid_env_ttl_boundaries(self, monkeypatch, ttl):
|
||||
"""Boundary TTL values (1, 300, 86400) are accepted."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
monkeypatch.setenv("HERMES_OPENROUTER_CACHE", "yes")
|
||||
monkeypatch.setenv("HERMES_OPENROUTER_CACHE_TTL", ttl)
|
||||
assert build_or_headers(or_config={})["X-OpenRouter-Cache-TTL"] == ttl
|
||||
|
||||
def test_no_env_vars_falls_through_to_config(self, monkeypatch):
|
||||
"""Without env vars, config.yaml controls behavior."""
|
||||
from agent.auxiliary_client import build_or_headers
|
||||
|
||||
monkeypatch.delenv("HERMES_OPENROUTER_CACHE", raising=False)
|
||||
monkeypatch.delenv("HERMES_OPENROUTER_CACHE_TTL", raising=False)
|
||||
headers = build_or_headers(or_config={"response_cache": True, "response_cache_ttl": 600})
|
||||
assert headers["X-OpenRouter-Cache"] == "true"
|
||||
assert headers["X-OpenRouter-Cache-TTL"] == "600"
|
||||
|
||||
class TestDefaultConfig:
|
||||
"""Verify the openrouter config section is in DEFAULT_CONFIG."""
|
||||
|
||||
def test_openrouter_section_exists(self):
|
||||
from hermes_cli.config import DEFAULT_CONFIG
|
||||
|
||||
assert "openrouter" in DEFAULT_CONFIG
|
||||
or_cfg = DEFAULT_CONFIG["openrouter"]
|
||||
assert or_cfg["response_cache"] is True
|
||||
assert or_cfg["response_cache_ttl"] == 300
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _check_openrouter_cache_status
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestCheckOpenrouterCacheStatus:
|
||||
"""Test the _check_openrouter_cache_status method on AIAgent."""
|
||||
|
||||
def _make_agent(self):
|
||||
"""Create a minimal AIAgent-like object with just the method under test."""
|
||||
from run_agent import AIAgent
|
||||
|
||||
# Use object.__new__ to skip __init__, then set the attributes we need
|
||||
agent = object.__new__(AIAgent)
|
||||
agent._or_cache_hits = 0
|
||||
return agent
|
||||
|
||||
def test_hit_increments_counter(self):
|
||||
agent = self._make_agent()
|
||||
resp = SimpleNamespace(headers={"x-openrouter-cache-status": "HIT"})
|
||||
agent._check_openrouter_cache_status(resp)
|
||||
assert agent._or_cache_hits == 1
|
||||
# Second hit increments
|
||||
agent._check_openrouter_cache_status(resp)
|
||||
assert agent._or_cache_hits == 2
|
||||
|
||||
def test_miss_does_not_increment(self):
|
||||
agent = self._make_agent()
|
||||
resp = SimpleNamespace(headers={"x-openrouter-cache-status": "MISS"})
|
||||
agent._check_openrouter_cache_status(resp)
|
||||
assert getattr(agent, "_or_cache_hits", 0) == 0
|
||||
|
||||
def test_no_header_is_noop(self):
|
||||
agent = self._make_agent()
|
||||
resp = SimpleNamespace(headers={})
|
||||
agent._check_openrouter_cache_status(resp)
|
||||
assert getattr(agent, "_or_cache_hits", 0) == 0
|
||||
|
||||
def test_none_response_is_safe(self):
|
||||
agent = self._make_agent()
|
||||
agent._check_openrouter_cache_status(None) # no crash
|
||||
|
||||
def test_no_headers_attr_is_safe(self):
|
||||
agent = self._make_agent()
|
||||
agent._check_openrouter_cache_status(object()) # no crash
|
||||
|
||||
def test_case_insensitive(self):
|
||||
agent = self._make_agent()
|
||||
resp = SimpleNamespace(headers={"x-openrouter-cache-status": "hit"})
|
||||
agent._check_openrouter_cache_status(resp)
|
||||
assert agent._or_cache_hits == 1
|
||||
@@ -125,6 +125,58 @@ class TestScanSkillCommands:
|
||||
assert "/knowledge-brain" in result
|
||||
assert result["/knowledge-brain"]["name"] == "knowledge-brain"
|
||||
|
||||
def test_get_skill_commands_rescans_when_platform_scope_changes(self, tmp_path):
|
||||
"""Platform-specific disabled-skill caches must not leak across platforms.
|
||||
|
||||
Regression test for #14536: a gateway process serving Telegram
|
||||
and Discord concurrently would seed the process-global cache
|
||||
with whichever platform scanned first, and subsequent
|
||||
``get_skill_commands()`` calls from the other platform silently
|
||||
inherited that filter.
|
||||
"""
|
||||
import agent.skill_commands as sc_mod
|
||||
from agent.skill_commands import get_skill_commands
|
||||
|
||||
def _disabled_skills():
|
||||
platform = os.getenv("HERMES_PLATFORM")
|
||||
if platform == "telegram":
|
||||
return {"telegram-only"}
|
||||
if platform == "discord":
|
||||
return {"discord-only"}
|
||||
return set()
|
||||
|
||||
with (
|
||||
patch("tools.skills_tool.SKILLS_DIR", tmp_path),
|
||||
patch("tools.skills_tool._get_disabled_skill_names", side_effect=_disabled_skills),
|
||||
patch.object(sc_mod, "_skill_commands", {}),
|
||||
patch.object(sc_mod, "_skill_commands_platform", None),
|
||||
):
|
||||
_make_skill(tmp_path, "shared")
|
||||
_make_skill(tmp_path, "telegram-only")
|
||||
_make_skill(tmp_path, "discord-only")
|
||||
|
||||
with patch.dict(os.environ, {"HERMES_PLATFORM": "telegram"}):
|
||||
telegram_commands = dict(get_skill_commands())
|
||||
|
||||
assert "/shared" in telegram_commands
|
||||
assert "/discord-only" in telegram_commands
|
||||
assert "/telegram-only" not in telegram_commands
|
||||
|
||||
with patch.dict(os.environ, {"HERMES_PLATFORM": "discord"}):
|
||||
discord_commands = dict(get_skill_commands())
|
||||
|
||||
assert "/shared" in discord_commands
|
||||
assert "/telegram-only" in discord_commands
|
||||
assert "/discord-only" not in discord_commands
|
||||
|
||||
# Switching back to telegram must also rescan — not re-serve
|
||||
# the discord view that was just cached.
|
||||
with patch.dict(os.environ, {"HERMES_PLATFORM": "telegram"}):
|
||||
telegram_again = dict(get_skill_commands())
|
||||
|
||||
assert "/telegram-only" not in telegram_again
|
||||
assert "/discord-only" in telegram_again
|
||||
|
||||
|
||||
def test_special_chars_stripped_from_cmd_key(self, tmp_path):
|
||||
"""Skill names with +, /, or other special chars produce clean cmd keys."""
|
||||
|
||||
@@ -46,6 +46,29 @@ class TestResolveOrigin:
|
||||
job = {"origin": {}}
|
||||
assert _resolve_origin(job) is None
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"non_dict_origin",
|
||||
[
|
||||
"combined-digest-replaces-x-and-y-20260503",
|
||||
123,
|
||||
["telegram", "12345"],
|
||||
("platform", "chat_id"),
|
||||
42.0,
|
||||
],
|
||||
)
|
||||
def test_non_dict_origin_returns_none_instead_of_crashing(self, non_dict_origin):
|
||||
"""Non-dict origins (provenance strings from hand-edited or migrated
|
||||
jobs.json) must be treated as missing instead of crashing the
|
||||
scheduler tick on ``origin.get('platform')`` with
|
||||
``'str' object has no attribute 'get'`` (#18722).
|
||||
|
||||
Before this guard a job in this state crashed every fire attempt
|
||||
forever; ``mark_job_run`` recorded the error but the next tick
|
||||
re-loaded the poisoned origin and crashed identically.
|
||||
"""
|
||||
job = {"origin": non_dict_origin}
|
||||
assert _resolve_origin(job) is None
|
||||
|
||||
|
||||
class TestResolveDeliveryTarget:
|
||||
def test_origin_delivery_preserves_thread_id(self):
|
||||
@@ -118,6 +141,16 @@ class TestResolveDeliveryTarget:
|
||||
"thread_id": None,
|
||||
}
|
||||
|
||||
def test_bare_platform_delivery_preserves_home_thread_id(self, monkeypatch):
|
||||
monkeypatch.setenv("DISCORD_HOME_CHANNEL", "parent-42")
|
||||
monkeypatch.setenv("DISCORD_HOME_CHANNEL_THREAD_ID", "topic-7")
|
||||
|
||||
assert _resolve_delivery_target({"deliver": "discord"}) == {
|
||||
"platform": "discord",
|
||||
"chat_id": "parent-42",
|
||||
"thread_id": "topic-7",
|
||||
}
|
||||
|
||||
def test_explicit_telegram_topic_target_with_thread_id(self):
|
||||
"""deliver: 'telegram:chat_id:thread_id' parses correctly."""
|
||||
job = {
|
||||
|
||||
@@ -12,6 +12,7 @@ class RestartTestAdapter(BasePlatformAdapter):
|
||||
def __init__(self):
|
||||
super().__init__(PlatformConfig(enabled=True, token="***"), Platform.TELEGRAM)
|
||||
self.sent: list[str] = []
|
||||
self.sent_calls: list[tuple[str, str, object]] = []
|
||||
|
||||
async def connect(self):
|
||||
return True
|
||||
@@ -21,6 +22,7 @@ class RestartTestAdapter(BasePlatformAdapter):
|
||||
|
||||
async def send(self, chat_id, content, reply_to=None, metadata=None):
|
||||
self.sent.append(content)
|
||||
self.sent_calls.append((chat_id, content, metadata))
|
||||
return SendResult(success=True, message_id="1")
|
||||
|
||||
async def send_typing(self, chat_id, metadata=None):
|
||||
@@ -30,12 +32,17 @@ class RestartTestAdapter(BasePlatformAdapter):
|
||||
return {"id": chat_id}
|
||||
|
||||
|
||||
def make_restart_source(chat_id: str = "123456", chat_type: str = "dm") -> SessionSource:
|
||||
def make_restart_source(
|
||||
chat_id: str = "123456",
|
||||
chat_type: str = "dm",
|
||||
thread_id: str | None = None,
|
||||
) -> SessionSource:
|
||||
return SessionSource(
|
||||
platform=Platform.TELEGRAM,
|
||||
chat_id=chat_id,
|
||||
chat_type=chat_type,
|
||||
user_id="u1",
|
||||
thread_id=thread_id,
|
||||
)
|
||||
|
||||
|
||||
@@ -81,6 +88,15 @@ def make_restart_runner(
|
||||
runner._handle_restart_command = GatewayRunner._handle_restart_command.__get__(
|
||||
runner, GatewayRunner
|
||||
)
|
||||
runner._handle_set_home_command = GatewayRunner._handle_set_home_command.__get__(
|
||||
runner, GatewayRunner
|
||||
)
|
||||
runner._send_restart_notification = GatewayRunner._send_restart_notification.__get__(
|
||||
runner, GatewayRunner
|
||||
)
|
||||
runner._send_home_channel_startup_notifications = (
|
||||
GatewayRunner._send_home_channel_startup_notifications.__get__(runner, GatewayRunner)
|
||||
)
|
||||
runner._status_action_label = GatewayRunner._status_action_label.__get__(
|
||||
runner, GatewayRunner
|
||||
)
|
||||
|
||||
@@ -49,9 +49,10 @@ class TestSuspendRecentlyActive:
|
||||
count = store.suspend_recently_active()
|
||||
assert count == 1
|
||||
|
||||
# Re-fetch — should be suspended now
|
||||
# Re-fetch — should be resume_pending (preserved, not wiped)
|
||||
refreshed = store.get_or_create_session(source)
|
||||
assert refreshed.was_auto_reset
|
||||
assert refreshed.resume_pending
|
||||
assert refreshed.session_id == entry.session_id # same session preserved
|
||||
|
||||
def test_does_not_suspend_old_sessions(self, tmp_path):
|
||||
store = _make_store(tmp_path)
|
||||
@@ -66,21 +67,22 @@ class TestSuspendRecentlyActive:
|
||||
count = store.suspend_recently_active(max_age_seconds=120)
|
||||
assert count == 0
|
||||
|
||||
def test_already_suspended_not_double_counted(self, tmp_path):
|
||||
def test_already_resume_pending_not_double_counted(self, tmp_path):
|
||||
store = _make_store(tmp_path)
|
||||
source = _make_source()
|
||||
entry = store.get_or_create_session(source)
|
||||
|
||||
# Suspend once
|
||||
# Mark resume_pending once
|
||||
count1 = store.suspend_recently_active()
|
||||
assert count1 == 1
|
||||
|
||||
# Create a new session (the old one got reset on next access)
|
||||
# Re-fetch returns the SAME session (preserved, not reset)
|
||||
entry2 = store.get_or_create_session(source)
|
||||
assert entry2.session_id == entry.session_id
|
||||
|
||||
# Suspend again — the new session is recent but not yet suspended
|
||||
# Second call skips already-resume_pending entries
|
||||
count2 = store.suspend_recently_active()
|
||||
assert count2 == 1
|
||||
assert count2 == 0
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -180,11 +182,11 @@ class TestCleanShutdownMarker:
|
||||
else:
|
||||
store.suspend_recently_active()
|
||||
|
||||
# Session SHOULD be suspended (crash recovery)
|
||||
# Session SHOULD be resume_pending (crash recovery preserves history)
|
||||
with store._lock:
|
||||
store._ensure_loaded_locked()
|
||||
suspended_count = sum(1 for e in store._entries.values() if e.suspended)
|
||||
assert suspended_count == 1, "Session should be suspended after crash (no marker)"
|
||||
resume_count = sum(1 for e in store._entries.values() if e.resume_pending)
|
||||
assert resume_count == 1, "Session should be resume_pending after crash (no marker)"
|
||||
|
||||
def test_marker_written_on_restart_stop(self, tmp_path, monkeypatch):
|
||||
"""stop(restart=True) should also write the marker."""
|
||||
|
||||
@@ -0,0 +1,166 @@
|
||||
"""Regression tests for the config.yaml → env var bridge in gateway/run.py.
|
||||
|
||||
Guards against the 60-vs-500 bug where a stale `.env HERMES_MAX_ITERATIONS=60`
|
||||
entry silently shadowed `agent.max_turns: 500` in config.yaml because the
|
||||
bridge used `if X not in os.environ` guards. After PR#18413 the bridge
|
||||
treats config.yaml as authoritative and unconditionally overwrites .env
|
||||
values for `agent.*`, `display.*`, `timezone`, and `security.*` keys.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
import textwrap
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
PROJECT_ROOT = Path(__file__).resolve().parents[2]
|
||||
|
||||
|
||||
def _run_gateway_import(hermes_home: Path, initial_env: dict[str, str]) -> dict[str, str]:
|
||||
"""Import gateway.run in a clean subprocess and return the post-import env.
|
||||
|
||||
The bridge runs at module-import time, so simply importing is enough
|
||||
to exercise it. Running in a subprocess isolates the test from other
|
||||
import side effects and makes the "what ends up in os.environ" check
|
||||
deterministic.
|
||||
"""
|
||||
script = textwrap.dedent(
|
||||
f"""
|
||||
import os, sys
|
||||
sys.path.insert(0, {str(PROJECT_ROOT)!r})
|
||||
|
||||
try:
|
||||
from gateway import run # noqa: F401 — module import triggers bridge
|
||||
except Exception as exc:
|
||||
print(f"IMPORT_ERROR:{{type(exc).__name__}}:{{exc}}", file=sys.stderr)
|
||||
sys.exit(2)
|
||||
|
||||
for k in (
|
||||
"HERMES_MAX_ITERATIONS",
|
||||
"HERMES_AGENT_TIMEOUT",
|
||||
"HERMES_AGENT_TIMEOUT_WARNING",
|
||||
"HERMES_GATEWAY_BUSY_INPUT_MODE",
|
||||
"HERMES_TIMEZONE",
|
||||
):
|
||||
v = os.environ.get(k)
|
||||
if v is not None:
|
||||
print(f"{{k}}={{v}}")
|
||||
"""
|
||||
)
|
||||
env = dict(initial_env)
|
||||
env["HERMES_HOME"] = str(hermes_home)
|
||||
# Keep PATH / PYTHONPATH so venv imports resolve.
|
||||
for k in ("PATH", "PYTHONPATH", "VIRTUAL_ENV", "HOME"):
|
||||
if k in os.environ and k not in env:
|
||||
env[k] = os.environ[k]
|
||||
|
||||
result = subprocess.run(
|
||||
[sys.executable, "-c", script],
|
||||
env=env,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=60,
|
||||
)
|
||||
if result.returncode != 0:
|
||||
pytest.fail(
|
||||
f"gateway.run import failed (rc={result.returncode})\n"
|
||||
f"stderr:\n{result.stderr}\nstdout:\n{result.stdout}"
|
||||
)
|
||||
out: dict[str, str] = {}
|
||||
for line in result.stdout.splitlines():
|
||||
if "=" in line:
|
||||
k, v = line.split("=", 1)
|
||||
out[k] = v
|
||||
return out
|
||||
|
||||
|
||||
def _write_config(home: Path, agent_cfg: dict | None = None, display_cfg: dict | None = None,
|
||||
timezone: str | None = None) -> None:
|
||||
import yaml
|
||||
cfg: dict = {}
|
||||
if agent_cfg:
|
||||
cfg["agent"] = agent_cfg
|
||||
if display_cfg:
|
||||
cfg["display"] = display_cfg
|
||||
if timezone:
|
||||
cfg["timezone"] = timezone
|
||||
(home / "config.yaml").write_text(yaml.safe_dump(cfg))
|
||||
|
||||
|
||||
def _write_env(home: Path, entries: dict[str, str]) -> None:
|
||||
lines = [f"{k}={v}\n" for k, v in entries.items()]
|
||||
(home / ".env").write_text("".join(lines))
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def hermes_home(tmp_path: Path) -> Path:
|
||||
home = tmp_path / ".hermes"
|
||||
home.mkdir()
|
||||
return home
|
||||
|
||||
|
||||
def test_config_max_turns_wins_over_stale_env(hermes_home: Path) -> None:
|
||||
"""Regression: config.yaml:agent.max_turns=500 must beat .env=60."""
|
||||
_write_config(hermes_home, agent_cfg={"max_turns": 500})
|
||||
_write_env(hermes_home, {"HERMES_MAX_ITERATIONS": "60"})
|
||||
|
||||
env = _run_gateway_import(hermes_home, initial_env={})
|
||||
|
||||
assert env.get("HERMES_MAX_ITERATIONS") == "500", (
|
||||
f"expected config.yaml max_turns=500 to win; got {env.get('HERMES_MAX_ITERATIONS')!r}. "
|
||||
"Stale .env value is shadowing config — the bridge lost its override."
|
||||
)
|
||||
|
||||
|
||||
def test_config_gateway_timeout_wins_over_stale_env(hermes_home: Path) -> None:
|
||||
"""Every agent.* bridge key must be config-authoritative, not .env-authoritative."""
|
||||
_write_config(hermes_home, agent_cfg={
|
||||
"gateway_timeout": 1800,
|
||||
"gateway_timeout_warning": 900,
|
||||
})
|
||||
_write_env(hermes_home, {
|
||||
"HERMES_AGENT_TIMEOUT": "60",
|
||||
"HERMES_AGENT_TIMEOUT_WARNING": "30",
|
||||
})
|
||||
|
||||
env = _run_gateway_import(hermes_home, initial_env={})
|
||||
|
||||
assert env.get("HERMES_AGENT_TIMEOUT") == "1800"
|
||||
assert env.get("HERMES_AGENT_TIMEOUT_WARNING") == "900"
|
||||
|
||||
|
||||
def test_config_display_busy_input_mode_wins_over_stale_env(hermes_home: Path) -> None:
|
||||
_write_config(hermes_home, display_cfg={"busy_input_mode": "interrupt"})
|
||||
_write_env(hermes_home, {"HERMES_GATEWAY_BUSY_INPUT_MODE": "queue"})
|
||||
|
||||
env = _run_gateway_import(hermes_home, initial_env={})
|
||||
|
||||
assert env.get("HERMES_GATEWAY_BUSY_INPUT_MODE") == "interrupt"
|
||||
|
||||
|
||||
def test_config_timezone_wins_over_stale_env(hermes_home: Path) -> None:
|
||||
_write_config(hermes_home, timezone="America/Los_Angeles")
|
||||
_write_env(hermes_home, {"HERMES_TIMEZONE": "UTC"})
|
||||
|
||||
env = _run_gateway_import(hermes_home, initial_env={})
|
||||
|
||||
assert env.get("HERMES_TIMEZONE") == "America/Los_Angeles"
|
||||
|
||||
|
||||
def test_env_value_survives_when_config_omits_key(hermes_home: Path) -> None:
|
||||
"""If config.yaml doesn't set max_turns, .env value must still pass through.
|
||||
|
||||
The bridge only overwrites when the config key is present — an absent
|
||||
config key should NOT clobber the .env value.
|
||||
"""
|
||||
_write_config(hermes_home, agent_cfg={}) # no max_turns
|
||||
_write_env(hermes_home, {"HERMES_MAX_ITERATIONS": "123"})
|
||||
|
||||
env = _run_gateway_import(hermes_home, initial_env={})
|
||||
|
||||
assert env.get("HERMES_MAX_ITERATIONS") == "123"
|
||||
@@ -0,0 +1,230 @@
|
||||
"""Security regression tests: Discord component views honor role allowlists.
|
||||
|
||||
The four interactive component views (ExecApprovalView, SlashConfirmView,
|
||||
UpdatePromptView, ModelPickerView) historically accepted only
|
||||
``allowed_user_ids``. Deployments that configure DISCORD_ALLOWED_ROLES
|
||||
without DISCORD_ALLOWED_USERS therefore had a wide-open component
|
||||
surface: any guild member who could see the prompt could approve exec
|
||||
commands, cancel slash confirmations, or switch the model -- even when
|
||||
the same user would be rejected at the slash and on_message gates.
|
||||
|
||||
These tests pin the user-or-role OR semantics and the fail-closed
|
||||
behavior on missing role data so the parity cannot regress.
|
||||
"""
|
||||
|
||||
from types import SimpleNamespace
|
||||
|
||||
import pytest
|
||||
|
||||
# Trigger the shared discord mock from tests/gateway/conftest.py before
|
||||
# importing the production module.
|
||||
from gateway.platforms.discord import ( # noqa: E402
|
||||
ExecApprovalView,
|
||||
ModelPickerView,
|
||||
SlashConfirmView,
|
||||
UpdatePromptView,
|
||||
_component_check_auth,
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Direct helper coverage -- the four views all delegate to this helper, so
|
||||
# pinning the helper's contract pins all four call sites.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _interaction(user_id, role_ids=None, *, drop_user=False, drop_roles=False):
|
||||
"""Build a mock interaction with the requested user/role shape.
|
||||
|
||||
drop_user simulates a payload whose .user attribute is None.
|
||||
drop_roles simulates a payload where .user has no .roles attribute
|
||||
at all (DM-context Member, raw User payload).
|
||||
"""
|
||||
if drop_user:
|
||||
return SimpleNamespace(user=None)
|
||||
|
||||
user_kwargs = {"id": user_id}
|
||||
if not drop_roles:
|
||||
user_kwargs["roles"] = [SimpleNamespace(id=r) for r in (role_ids or [])]
|
||||
return SimpleNamespace(user=SimpleNamespace(**user_kwargs))
|
||||
|
||||
|
||||
# ── back-compat: empty allowlists -> allow everyone ────────────────────────
|
||||
|
||||
|
||||
def test_component_check_empty_allowlists_allows_everyone():
|
||||
"""SECURITY-CRITICAL backwards-compat: deployments without any
|
||||
DISCORD_ALLOWED_* env vars set must continue to allow component
|
||||
interactions from anyone (no regression for unconfigured setups)."""
|
||||
interaction = _interaction(11111)
|
||||
assert _component_check_auth(interaction, set(), set()) is True
|
||||
assert _component_check_auth(interaction, None, None) is True
|
||||
|
||||
|
||||
# ── user allowlist ─────────────────────────────────────────────────────────
|
||||
|
||||
|
||||
def test_component_check_user_in_user_allowlist_passes():
|
||||
interaction = _interaction(11111)
|
||||
assert _component_check_auth(interaction, {"11111"}, set()) is True
|
||||
|
||||
|
||||
def test_component_check_user_not_in_user_allowlist_rejected():
|
||||
interaction = _interaction(99999)
|
||||
assert _component_check_auth(interaction, {"11111"}, set()) is False
|
||||
|
||||
|
||||
# ── role allowlist OR semantics ────────────────────────────────────────────
|
||||
|
||||
|
||||
def test_component_check_role_only_user_with_matching_role_passes():
|
||||
"""Role-only deployment (DISCORD_ALLOWED_ROLES set, DISCORD_ALLOWED_USERS
|
||||
empty) where the user is not in the empty user list but DOES carry a
|
||||
matching role: must pass. This is the regression that prompted the
|
||||
fix -- previously _check_auth allowed everyone when the user set was
|
||||
empty, ignoring the role allowlist."""
|
||||
interaction = _interaction(99999, role_ids=[42])
|
||||
assert _component_check_auth(interaction, set(), {42}) is True
|
||||
|
||||
|
||||
def test_component_check_role_only_user_without_matching_role_rejected():
|
||||
"""Role-only deployment where the user has no matching role: reject.
|
||||
Previously this allowed everyone because allowed_user_ids was empty."""
|
||||
interaction = _interaction(99999, role_ids=[7, 8])
|
||||
assert _component_check_auth(interaction, set(), {42}) is False
|
||||
|
||||
|
||||
def test_component_check_user_or_role_user_match():
|
||||
"""Both allowlists set; user matches user allowlist: pass."""
|
||||
interaction = _interaction(11111, role_ids=[7])
|
||||
assert _component_check_auth(interaction, {"11111"}, {42}) is True
|
||||
|
||||
|
||||
def test_component_check_user_or_role_role_match():
|
||||
"""Both allowlists set; user not in user list but in role list: pass."""
|
||||
interaction = _interaction(99999, role_ids=[42])
|
||||
assert _component_check_auth(interaction, {"11111"}, {42}) is True
|
||||
|
||||
|
||||
def test_component_check_user_or_role_neither_match():
|
||||
"""Both allowlists set; user matches neither: reject."""
|
||||
interaction = _interaction(99999, role_ids=[7])
|
||||
assert _component_check_auth(interaction, {"11111"}, {42}) is False
|
||||
|
||||
|
||||
# ── fail-closed on missing role data ───────────────────────────────────────
|
||||
|
||||
|
||||
def test_component_check_role_policy_with_no_roles_attr_rejects():
|
||||
"""Role allowlist configured but interaction.user has no .roles
|
||||
attribute (DM-context Member, raw User payload): must reject. A user
|
||||
without resolvable roles cannot satisfy a role allowlist."""
|
||||
interaction = _interaction(11111, drop_roles=True)
|
||||
assert _component_check_auth(interaction, set(), {42}) is False
|
||||
|
||||
|
||||
def test_component_check_missing_user_with_allowlist_rejects():
|
||||
"""interaction.user is None with any allowlist configured: fail
|
||||
closed without raising AttributeError."""
|
||||
interaction = _interaction(0, drop_user=True)
|
||||
assert _component_check_auth(interaction, {"11111"}, set()) is False
|
||||
assert _component_check_auth(interaction, set(), {42}) is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# View construction: every view must accept allowed_role_ids and route
|
||||
# through the shared helper. Default value preserves prior call-sites.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_exec_approval_view_accepts_role_allowlist():
|
||||
view = ExecApprovalView(
|
||||
session_key="sess-1",
|
||||
allowed_user_ids={"11111"},
|
||||
allowed_role_ids={42},
|
||||
)
|
||||
# Role-only user passes
|
||||
assert view._check_auth(_interaction(99999, role_ids=[42])) is True
|
||||
# Neither user nor role match: reject
|
||||
assert view._check_auth(_interaction(99999, role_ids=[7])) is False
|
||||
|
||||
|
||||
def test_exec_approval_view_role_default_is_empty_set():
|
||||
"""Existing call sites that pass only allowed_user_ids must continue
|
||||
working with the legacy semantics (no role gate)."""
|
||||
view = ExecApprovalView(session_key="sess-1", allowed_user_ids={"11111"})
|
||||
assert view.allowed_role_ids == set()
|
||||
assert view._check_auth(_interaction(11111)) is True
|
||||
assert view._check_auth(_interaction(99999)) is False
|
||||
|
||||
|
||||
def test_slash_confirm_view_accepts_role_allowlist():
|
||||
view = SlashConfirmView(
|
||||
session_key="sess-1",
|
||||
confirm_id="c1",
|
||||
allowed_user_ids=set(),
|
||||
allowed_role_ids={42},
|
||||
)
|
||||
assert view._check_auth(_interaction(99999, role_ids=[42])) is True
|
||||
assert view._check_auth(_interaction(99999, role_ids=[7])) is False
|
||||
|
||||
|
||||
def test_update_prompt_view_accepts_role_allowlist():
|
||||
view = UpdatePromptView(
|
||||
session_key="sess-1",
|
||||
allowed_user_ids=set(),
|
||||
allowed_role_ids={42},
|
||||
)
|
||||
assert view._check_auth(_interaction(99999, role_ids=[42])) is True
|
||||
assert view._check_auth(_interaction(99999, role_ids=[7])) is False
|
||||
|
||||
|
||||
def test_model_picker_view_accepts_role_allowlist():
|
||||
async def _noop(*_a, **_k):
|
||||
return ""
|
||||
|
||||
view = ModelPickerView(
|
||||
providers=[],
|
||||
current_model="m",
|
||||
current_provider="p",
|
||||
session_key="sess-1",
|
||||
on_model_selected=_noop,
|
||||
allowed_user_ids=set(),
|
||||
allowed_role_ids={42},
|
||||
)
|
||||
assert view._check_auth(_interaction(99999, role_ids=[42])) is True
|
||||
assert view._check_auth(_interaction(99999, role_ids=[7])) is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Empty allowlists across views: legacy "allow everyone" must hold.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"view_factory",
|
||||
[
|
||||
lambda: ExecApprovalView(session_key="s", allowed_user_ids=set()),
|
||||
lambda: SlashConfirmView(session_key="s", confirm_id="c", allowed_user_ids=set()),
|
||||
lambda: UpdatePromptView(session_key="s", allowed_user_ids=set()),
|
||||
],
|
||||
)
|
||||
def test_views_empty_allowlists_allow_everyone(view_factory):
|
||||
view = view_factory()
|
||||
assert view._check_auth(_interaction(99999)) is True
|
||||
|
||||
|
||||
def test_model_picker_view_empty_allowlists_allow_everyone():
|
||||
async def _noop(*_a, **_k):
|
||||
return ""
|
||||
|
||||
view = ModelPickerView(
|
||||
providers=[],
|
||||
current_model="m",
|
||||
current_provider="p",
|
||||
session_key="s",
|
||||
on_model_selected=_noop,
|
||||
allowed_user_ids=set(),
|
||||
)
|
||||
assert view.allowed_role_ids == set()
|
||||
assert view._check_auth(_interaction(99999)) is True
|
||||
@@ -172,6 +172,69 @@ async def test_connect_only_requests_members_intent_when_needed(monkeypatch, all
|
||||
await adapter.disconnect()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_reconnect_closes_previous_client_to_prevent_zombie_websocket(monkeypatch):
|
||||
"""Regression for #18187: calling connect() twice without disconnect() in
|
||||
between (e.g. during an in-process reconnect attempt) must close the old
|
||||
commands.Bot before creating a new one. Without this guard, two websockets
|
||||
stay alive and both fire on_message, producing double responses with
|
||||
different wording.
|
||||
"""
|
||||
adapter = DiscordAdapter(PlatformConfig(enabled=True, token="test-token"))
|
||||
|
||||
monkeypatch.setattr("gateway.status.acquire_scoped_lock", lambda scope, identity, metadata=None: (True, None))
|
||||
monkeypatch.setattr("gateway.status.release_scoped_lock", lambda scope, identity: None)
|
||||
|
||||
intents = SimpleNamespace(
|
||||
message_content=False, dm_messages=False, guild_messages=False,
|
||||
members=False, voice_states=False,
|
||||
)
|
||||
monkeypatch.setattr(discord_platform.Intents, "default", lambda: intents)
|
||||
|
||||
class TrackedBot(FakeBot):
|
||||
"""FakeBot that records close() calls and reports open/closed state."""
|
||||
_closed = False
|
||||
|
||||
def is_closed(self):
|
||||
return self._closed
|
||||
|
||||
async def close(self):
|
||||
self._closed = True
|
||||
|
||||
created: list[TrackedBot] = []
|
||||
|
||||
def fake_bot_factory(*, command_prefix, intents, proxy=None, allowed_mentions=None, **_):
|
||||
bot = TrackedBot(intents=intents, allowed_mentions=allowed_mentions)
|
||||
created.append(bot)
|
||||
return bot
|
||||
|
||||
monkeypatch.setattr(discord_platform.commands, "Bot", fake_bot_factory)
|
||||
monkeypatch.setattr(adapter, "_resolve_allowed_usernames", AsyncMock())
|
||||
|
||||
# First connect — fresh adapter, no prior client.
|
||||
assert await adapter.connect() is True
|
||||
assert len(created) == 1
|
||||
first_bot = created[0]
|
||||
assert first_bot._closed is False, "first bot should still be open after connect()"
|
||||
|
||||
# Second connect WITHOUT disconnect — simulates an in-process reconnect.
|
||||
# Without the fix, first_bot would remain open (zombie), and both would
|
||||
# receive every Discord event, causing double responses.
|
||||
assert await adapter.connect() is True
|
||||
assert len(created) == 2
|
||||
second_bot = created[1]
|
||||
|
||||
# The first bot must be closed before the second is assigned.
|
||||
assert first_bot._closed is True, (
|
||||
"First Discord client must be closed on re-entry of connect() to prevent "
|
||||
"zombie websocket (#18187)"
|
||||
)
|
||||
assert second_bot._closed is False, "second bot should still be open"
|
||||
assert adapter._client is second_bot
|
||||
|
||||
await adapter.disconnect()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_connect_releases_token_lock_on_timeout(monkeypatch):
|
||||
adapter = DiscordAdapter(PlatformConfig(enabled=True, token="test-token"))
|
||||
|
||||
@@ -0,0 +1,737 @@
|
||||
"""Security regression tests: slash commands honor on_message authorization gates.
|
||||
|
||||
Slash invocations (``_run_simple_slash``, ``_handle_thread_create_slash``)
|
||||
historically bypassed every gate ``on_message`` enforces — DISCORD_ALLOWED_USERS,
|
||||
DISCORD_ALLOWED_ROLES, DISCORD_ALLOWED_CHANNELS, DISCORD_IGNORED_CHANNELS.
|
||||
Any guild member could invoke ``/background``, ``/restart``, etc. as the
|
||||
operator. ``_check_slash_authorization`` mirrors all four gates one-for-one.
|
||||
|
||||
These tests pin the security-correct behavior so the bypass cannot regress.
|
||||
"""
|
||||
|
||||
import asyncio
|
||||
import logging
|
||||
import sys
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
|
||||
from gateway.config import PlatformConfig
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Discord module mock — borrowed from test_discord_slash_commands.py so this
|
||||
# file runs on machines without discord.py installed.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _ensure_discord_mock():
|
||||
if "discord" in sys.modules and hasattr(sys.modules["discord"], "__file__"):
|
||||
return # real discord installed
|
||||
|
||||
if sys.modules.get("discord") is None:
|
||||
discord_mod = MagicMock()
|
||||
discord_mod.Intents.default.return_value = MagicMock()
|
||||
discord_mod.DMChannel = type("DMChannel", (), {})
|
||||
discord_mod.Thread = type("Thread", (), {})
|
||||
discord_mod.ForumChannel = type("ForumChannel", (), {})
|
||||
discord_mod.Interaction = object
|
||||
|
||||
class _FakePermissions:
|
||||
def __init__(self, value=0, **_):
|
||||
self.value = value
|
||||
|
||||
discord_mod.Permissions = _FakePermissions
|
||||
|
||||
class _FakeGroup:
|
||||
def __init__(self, *, name, description, parent=None):
|
||||
self.name = name
|
||||
self.description = description
|
||||
self.parent = parent
|
||||
self._children: dict[str, object] = {}
|
||||
if parent is not None:
|
||||
parent.add_command(self)
|
||||
|
||||
def add_command(self, cmd):
|
||||
self._children[cmd.name] = cmd
|
||||
|
||||
class _FakeCommand:
|
||||
def __init__(self, *, name, description, callback, parent=None):
|
||||
self.name = name
|
||||
self.description = description
|
||||
self.callback = callback
|
||||
self.parent = parent
|
||||
self.default_permissions = None
|
||||
|
||||
discord_mod.app_commands = SimpleNamespace(
|
||||
describe=lambda **kwargs: (lambda fn: fn),
|
||||
choices=lambda **kwargs: (lambda fn: fn),
|
||||
autocomplete=lambda **kwargs: (lambda fn: fn),
|
||||
Choice=lambda **kwargs: SimpleNamespace(**kwargs),
|
||||
Group=_FakeGroup,
|
||||
Command=_FakeCommand,
|
||||
)
|
||||
|
||||
ext_mod = MagicMock()
|
||||
commands_mod = MagicMock()
|
||||
commands_mod.Bot = MagicMock
|
||||
ext_mod.commands = commands_mod
|
||||
|
||||
sys.modules["discord"] = discord_mod
|
||||
sys.modules.setdefault("discord.ext", ext_mod)
|
||||
sys.modules.setdefault("discord.ext.commands", commands_mod)
|
||||
|
||||
|
||||
_ensure_discord_mock()
|
||||
|
||||
from gateway.platforms.discord import DiscordAdapter # noqa: E402
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _isolate_discord_env(monkeypatch):
|
||||
for var in (
|
||||
"DISCORD_ALLOWED_USERS",
|
||||
"DISCORD_ALLOWED_ROLES",
|
||||
"DISCORD_ALLOWED_CHANNELS",
|
||||
"DISCORD_IGNORED_CHANNELS",
|
||||
"DISCORD_HIDE_SLASH_COMMANDS",
|
||||
"DISCORD_ALLOW_BOTS",
|
||||
):
|
||||
monkeypatch.delenv(var, raising=False)
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _stub_discord_permissions(monkeypatch):
|
||||
"""Pin discord.Permissions to a plain stand-in so tests can assert the
|
||||
bitfield value regardless of whether real discord.py or a sibling test
|
||||
module's MagicMock is loaded."""
|
||||
import discord
|
||||
|
||||
class _Perm:
|
||||
def __init__(self, value=0, **_):
|
||||
self.value = value
|
||||
|
||||
monkeypatch.setattr(discord, "Permissions", _Perm)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def adapter():
|
||||
config = PlatformConfig(enabled=True, token="***")
|
||||
a = DiscordAdapter(config)
|
||||
a._client = SimpleNamespace(user=SimpleNamespace(id=99999, name="HermesBot"), guilds=[])
|
||||
return a
|
||||
|
||||
|
||||
_SENTINEL = object()
|
||||
|
||||
|
||||
def _make_interaction(
|
||||
user_id, *, channel_id=12345, guild_id=42, in_dm=False, in_thread=False,
|
||||
parent_channel_id=None, user=_SENTINEL,
|
||||
):
|
||||
"""Build a mock Discord Interaction with a still-unresponded response.
|
||||
|
||||
``channel_id`` may be set to ``None`` to simulate a guild interaction
|
||||
payload missing a resolvable channel id (fail-closed exercise).
|
||||
Pass ``user=None`` to simulate a payload missing the user object.
|
||||
"""
|
||||
import discord
|
||||
|
||||
response = SimpleNamespace(send_message=AsyncMock(), defer=AsyncMock())
|
||||
|
||||
if in_dm:
|
||||
channel = discord.DMChannel()
|
||||
elif in_thread:
|
||||
channel = discord.Thread()
|
||||
channel.id = channel_id
|
||||
channel.parent_id = parent_channel_id
|
||||
elif channel_id is None:
|
||||
channel = None
|
||||
else:
|
||||
channel = SimpleNamespace(id=channel_id)
|
||||
|
||||
if user is _SENTINEL:
|
||||
user_obj = SimpleNamespace(id=int(user_id), name=f"user_{user_id}")
|
||||
else:
|
||||
user_obj = user
|
||||
|
||||
return SimpleNamespace(
|
||||
user=user_obj,
|
||||
guild=SimpleNamespace(owner_id=999),
|
||||
guild_id=guild_id,
|
||||
channel_id=channel_id,
|
||||
channel=channel,
|
||||
response=response,
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Backwards-compat: empty allowlist → everything passes (matches on_message)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_allowlist_allows_everyone(adapter):
|
||||
"""SECURITY-CRITICAL backwards-compat: deployments without any allowlist
|
||||
env vars set must see ZERO behavior change. on_message lets everyone
|
||||
through in this case (returns True at line 1890); slash must do the same.
|
||||
"""
|
||||
interaction = _make_interaction("999999999")
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is True
|
||||
interaction.response.send_message.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_allowlist_dm_also_allowed(adapter):
|
||||
"""Same for DMs — no allowlist means no restriction, matching on_message."""
|
||||
interaction = _make_interaction("999999999", in_dm=True)
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is True
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# User allowlist (DISCORD_ALLOWED_USERS) parity
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_allowed_user_passes(adapter):
|
||||
adapter._allowed_user_ids = {"100200300"}
|
||||
interaction = _make_interaction("100200300")
|
||||
assert await adapter._check_slash_authorization(interaction, "/background hi") is True
|
||||
interaction.response.send_message.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_disallowed_user_rejected_with_ephemeral(adapter, caplog):
|
||||
adapter._allowed_user_ids = {"100200300"}
|
||||
interaction = _make_interaction("999999999")
|
||||
with caplog.at_level(logging.WARNING):
|
||||
assert await adapter._check_slash_authorization(interaction, "/background hi") is False
|
||||
interaction.response.send_message.assert_awaited_once()
|
||||
args, kwargs = interaction.response.send_message.call_args
|
||||
assert kwargs.get("ephemeral") is True
|
||||
assert "not authorized" in (args[0] if args else kwargs.get("content", "")).lower()
|
||||
assert any("Unauthorized slash attempt" in r.message for r in caplog.records)
|
||||
assert any("DISCORD_ALLOWED_USERS" in r.message for r in caplog.records)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Role allowlist (DISCORD_ALLOWED_ROLES) parity
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_role_member_passes(adapter):
|
||||
"""A user whose Member.roles includes an allowed role passes the gate."""
|
||||
adapter._allowed_role_ids = {1234}
|
||||
interaction = _make_interaction("999999999")
|
||||
interaction.user.roles = [SimpleNamespace(id=1234)]
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_role_non_member_rejected(adapter):
|
||||
"""A user without any matching role is rejected even if no user allowlist."""
|
||||
adapter._allowed_role_ids = {1234}
|
||||
interaction = _make_interaction("999999999")
|
||||
interaction.user.roles = [SimpleNamespace(id=9999)] # different role
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Channel allowlist (DISCORD_ALLOWED_CHANNELS) parity — the gate prajer used
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_channel_not_in_allowlist_rejected(adapter, monkeypatch, caplog):
|
||||
"""on_message blocks messages in channels not in DISCORD_ALLOWED_CHANNELS;
|
||||
slash must do the same. This is the EXACT bypass prajer exploited.
|
||||
"""
|
||||
monkeypatch.setenv("DISCORD_ALLOWED_CHANNELS", "1111,2222")
|
||||
interaction = _make_interaction("100200300", channel_id=9999)
|
||||
with caplog.at_level(logging.WARNING):
|
||||
assert await adapter._check_slash_authorization(interaction, "/background hi") is False
|
||||
assert any("DISCORD_ALLOWED_CHANNELS" in r.message for r in caplog.records)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_channel_in_allowlist_passes(adapter, monkeypatch):
|
||||
monkeypatch.setenv("DISCORD_ALLOWED_CHANNELS", "1111,2222")
|
||||
interaction = _make_interaction("100200300", channel_id=1111)
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_channel_allowlist_wildcard_passes(adapter, monkeypatch):
|
||||
"""``*`` in DISCORD_ALLOWED_CHANNELS = allow any channel, matching on_message."""
|
||||
monkeypatch.setenv("DISCORD_ALLOWED_CHANNELS", "*")
|
||||
interaction = _make_interaction("100200300", channel_id=9999)
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_channel_allowlist_does_not_apply_to_dms(adapter, monkeypatch):
|
||||
"""DMs aren't channel-gated — they go through on_message's DM lockdown."""
|
||||
monkeypatch.setenv("DISCORD_ALLOWED_CHANNELS", "1111")
|
||||
interaction = _make_interaction("100200300", in_dm=True)
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is True
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Channel blocklist (DISCORD_IGNORED_CHANNELS) parity
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ignored_channel_rejected(adapter, monkeypatch, caplog):
|
||||
monkeypatch.setenv("DISCORD_IGNORED_CHANNELS", "9999")
|
||||
interaction = _make_interaction("100200300", channel_id=9999)
|
||||
with caplog.at_level(logging.WARNING):
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is False
|
||||
assert any("DISCORD_IGNORED_CHANNELS" in r.message for r in caplog.records)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ignored_channel_wildcard_blocks_all(adapter, monkeypatch):
|
||||
monkeypatch.setenv("DISCORD_IGNORED_CHANNELS", "*")
|
||||
interaction = _make_interaction("100200300", channel_id=9999)
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Cross-platform admin notification
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_unauthorized_attempt_notifies_telegram(adapter):
|
||||
from gateway.session import Platform
|
||||
|
||||
telegram_adapter = SimpleNamespace(send=AsyncMock())
|
||||
home = SimpleNamespace(chat_id="987654321")
|
||||
runner = SimpleNamespace(
|
||||
adapters={Platform.TELEGRAM: telegram_adapter},
|
||||
config=SimpleNamespace(get_home_channel=lambda p: home if p is Platform.TELEGRAM else None),
|
||||
)
|
||||
adapter.gateway_runner = runner
|
||||
adapter._allowed_user_ids = {"100200300"}
|
||||
|
||||
interaction = _make_interaction("999999999")
|
||||
await adapter._check_slash_authorization(interaction, "/background hi")
|
||||
|
||||
# Notify is fire-and-forget — let the scheduled task run.
|
||||
await asyncio.sleep(0)
|
||||
await asyncio.sleep(0)
|
||||
|
||||
telegram_adapter.send.assert_awaited_once()
|
||||
chat_id, msg = telegram_adapter.send.call_args.args
|
||||
assert chat_id == "987654321"
|
||||
assert "Unauthorized" in msg
|
||||
assert "999999999" in msg
|
||||
assert "/background hi" in msg
|
||||
assert "DISCORD_ALLOWED_USERS" in msg
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_notify_silently_no_ops_without_runner(adapter):
|
||||
adapter.gateway_runner = None
|
||||
await adapter._notify_unauthorized_slash("u", "1", 2, 3, "/x", "reason") # must not raise
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_notify_falls_back_to_slack_if_no_telegram(adapter):
|
||||
from gateway.session import Platform
|
||||
|
||||
slack_adapter = SimpleNamespace(send=AsyncMock())
|
||||
home_slack = SimpleNamespace(chat_id="C12345")
|
||||
runner = SimpleNamespace(
|
||||
adapters={Platform.SLACK: slack_adapter},
|
||||
config=SimpleNamespace(
|
||||
get_home_channel=lambda p: home_slack if p is Platform.SLACK else None,
|
||||
),
|
||||
)
|
||||
adapter.gateway_runner = runner
|
||||
await adapter._notify_unauthorized_slash("u", "1", 2, 3, "/x", "reason")
|
||||
slack_adapter.send.assert_awaited_once()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Opt-in visibility hide
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_visibility_hide_off_by_default_is_noop(adapter, monkeypatch):
|
||||
"""DISCORD_HIDE_SLASH_COMMANDS unset → don't touch any command's permissions."""
|
||||
cmd = SimpleNamespace(name="x", default_permissions="UNCHANGED")
|
||||
tree = SimpleNamespace(get_commands=lambda: [cmd])
|
||||
|
||||
# Re-run the registration tail logic by calling the bit that decides:
|
||||
# we don't have a clean way to simulate the env-gated branch from
|
||||
# _register_slash_commands, so we just confirm the helper itself works
|
||||
# AND assert the env-gating logic is correct.
|
||||
assert os.environ.get("DISCORD_HIDE_SLASH_COMMANDS") is None
|
||||
# Helper should still work when called directly:
|
||||
adapter._apply_owner_only_visibility(tree)
|
||||
# When called directly the helper applies — env gating is at the call site,
|
||||
# which we exercise in an integration-style test below.
|
||||
|
||||
|
||||
def test_visibility_hide_helper_zeroes_perms(adapter):
|
||||
cmd_a = SimpleNamespace(name="a", default_permissions=None)
|
||||
cmd_b = SimpleNamespace(name="b", default_permissions=None)
|
||||
tree = SimpleNamespace(get_commands=lambda: [cmd_a, cmd_b])
|
||||
adapter._apply_owner_only_visibility(tree)
|
||||
assert cmd_a.default_permissions is not None
|
||||
assert cmd_b.default_permissions is not None
|
||||
assert cmd_a.default_permissions.value == 0
|
||||
assert cmd_b.default_permissions.value == 0
|
||||
|
||||
|
||||
def test_visibility_hide_tolerates_unsetable_command(adapter, caplog):
|
||||
class _Frozen:
|
||||
__slots__ = ("name",)
|
||||
def __init__(self, name):
|
||||
self.name = name
|
||||
|
||||
cmd_ok = SimpleNamespace(name="ok", default_permissions=None)
|
||||
cmd_bad = _Frozen("bad")
|
||||
tree = SimpleNamespace(get_commands=lambda: [cmd_bad, cmd_ok])
|
||||
|
||||
with caplog.at_level(logging.DEBUG):
|
||||
adapter._apply_owner_only_visibility(tree)
|
||||
|
||||
assert cmd_ok.default_permissions.value == 0
|
||||
|
||||
|
||||
# os import for test_visibility_hide_off_by_default_is_noop
|
||||
import os # noqa: E402
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Fail-closed parity on malformed slash auth context
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_missing_channel_id_rejected_when_channel_policy_configured(
|
||||
adapter, monkeypatch,
|
||||
):
|
||||
"""A guild interaction without a resolvable channel id must fail
|
||||
closed when DISCORD_ALLOWED_CHANNELS is configured. Without this
|
||||
guard the entire channel-policy block silently fell through."""
|
||||
monkeypatch.setenv("DISCORD_ALLOWED_CHANNELS", "1111,2222")
|
||||
interaction = _make_interaction("100200300", channel_id=None)
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is False
|
||||
interaction.response.send_message.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_missing_channel_id_allowed_when_no_channel_policy(adapter):
|
||||
"""No DISCORD_ALLOWED_CHANNELS configured + missing channel id: still
|
||||
pass through the channel block (matches no-allowlist default)."""
|
||||
interaction = _make_interaction("100200300", channel_id=None)
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_missing_user_rejected_when_allowlist_configured(adapter):
|
||||
"""interaction.user is None with a user/role allowlist active:
|
||||
fail closed without raising AttributeError."""
|
||||
adapter._allowed_user_ids = {"100200300"}
|
||||
interaction = _make_interaction("100200300", user=None)
|
||||
# Must not raise — must return False with an ephemeral rejection
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is False
|
||||
interaction.response.send_message.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_missing_user_allowed_when_no_allowlist_configured(adapter):
|
||||
"""interaction.user is None but no allowlist configured: allow
|
||||
(preserves no-allowlist back-compat -- anyone is allowed when no
|
||||
policy is in effect)."""
|
||||
interaction = _make_interaction("100200300", user=None)
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is True
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Thread parent channel allowlist parity
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_thread_parent_in_allowlist_passes(adapter, monkeypatch):
|
||||
"""Thread whose parent channel is on DISCORD_ALLOWED_CHANNELS passes
|
||||
even though the thread id itself isn't on the list."""
|
||||
monkeypatch.setenv("DISCORD_ALLOWED_CHANNELS", "5555")
|
||||
interaction = _make_interaction(
|
||||
"100200300", channel_id=9999, in_thread=True, parent_channel_id=5555,
|
||||
)
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_thread_parent_in_ignorelist_rejects(adapter, monkeypatch):
|
||||
"""Thread whose parent channel is on DISCORD_IGNORED_CHANNELS rejects
|
||||
even when the thread id itself isn't ignored."""
|
||||
monkeypatch.setenv("DISCORD_IGNORED_CHANNELS", "5555")
|
||||
interaction = _make_interaction(
|
||||
"100200300", channel_id=9999, in_thread=True, parent_channel_id=5555,
|
||||
)
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ignored_beats_allowed(adapter, monkeypatch):
|
||||
"""Channel listed in BOTH allowed and ignored: the ignored entry wins.
|
||||
Anything else would be a foot-gun where adding to ignored does nothing
|
||||
if the channel is also explicitly allowed."""
|
||||
monkeypatch.setenv("DISCORD_ALLOWED_CHANNELS", "1111")
|
||||
monkeypatch.setenv("DISCORD_IGNORED_CHANNELS", "1111")
|
||||
interaction = _make_interaction("100200300", channel_id=1111)
|
||||
assert await adapter._check_slash_authorization(interaction, "/help") is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Admin notify soft-fail fallback
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_notify_falls_back_to_slack_on_telegram_soft_fail(adapter):
|
||||
"""adapter.send returning SendResult(success=False) must NOT short-
|
||||
circuit the fallback chain. Treating a soft failure as delivered
|
||||
means a Telegram outage swallows alerts silently."""
|
||||
from gateway.session import Platform
|
||||
|
||||
soft_fail = SimpleNamespace(success=False, error="rate limited")
|
||||
telegram_adapter = SimpleNamespace(send=AsyncMock(return_value=soft_fail))
|
||||
slack_adapter = SimpleNamespace(send=AsyncMock())
|
||||
home_tg = SimpleNamespace(chat_id="987654321")
|
||||
home_sl = SimpleNamespace(chat_id="C12345")
|
||||
homes = {Platform.TELEGRAM: home_tg, Platform.SLACK: home_sl}
|
||||
runner = SimpleNamespace(
|
||||
adapters={
|
||||
Platform.TELEGRAM: telegram_adapter,
|
||||
Platform.SLACK: slack_adapter,
|
||||
},
|
||||
config=SimpleNamespace(get_home_channel=lambda p: homes.get(p)),
|
||||
)
|
||||
adapter.gateway_runner = runner
|
||||
|
||||
await adapter._notify_unauthorized_slash("u", "1", 2, 3, "/x", "reason")
|
||||
|
||||
telegram_adapter.send.assert_awaited_once()
|
||||
slack_adapter.send.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_notify_returns_on_telegram_truthy_success(adapter):
|
||||
"""adapter.send returning SendResult(success=True) -- or any object
|
||||
without a falsy success attribute -- should still short-circuit at
|
||||
Telegram. (This guards against the soft-fail patch over-correcting.)"""
|
||||
from gateway.session import Platform
|
||||
|
||||
ok = SimpleNamespace(success=True, message_id="m1")
|
||||
telegram_adapter = SimpleNamespace(send=AsyncMock(return_value=ok))
|
||||
slack_adapter = SimpleNamespace(send=AsyncMock())
|
||||
home_tg = SimpleNamespace(chat_id="987654321")
|
||||
home_sl = SimpleNamespace(chat_id="C12345")
|
||||
homes = {Platform.TELEGRAM: home_tg, Platform.SLACK: home_sl}
|
||||
runner = SimpleNamespace(
|
||||
adapters={
|
||||
Platform.TELEGRAM: telegram_adapter,
|
||||
Platform.SLACK: slack_adapter,
|
||||
},
|
||||
config=SimpleNamespace(get_home_channel=lambda p: homes.get(p)),
|
||||
)
|
||||
adapter.gateway_runner = runner
|
||||
|
||||
await adapter._notify_unauthorized_slash("u", "1", 2, 3, "/x", "reason")
|
||||
|
||||
telegram_adapter.send.assert_awaited_once()
|
||||
slack_adapter.send.assert_not_awaited()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# /skill autocomplete + callback gating
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _capture_skill_registration(adapter, monkeypatch, entries):
|
||||
"""Run ``_register_skill_group`` against a stubbed skill catalog and
|
||||
return ``(handler_callback, autocomplete_callback)``.
|
||||
|
||||
The autocomplete callback is captured by monkeypatching
|
||||
``discord.app_commands.autocomplete`` -- the production decorator is
|
||||
a no-op stub in this test file's discord mock, so capturing the
|
||||
callback through it is the direct route in tests.
|
||||
"""
|
||||
import discord
|
||||
|
||||
captured: dict = {}
|
||||
|
||||
def fake_categories(reserved_names):
|
||||
# Match discord_skill_commands_by_category's tuple shape:
|
||||
# (categories_dict, uncategorized_list, hidden_count)
|
||||
return ({}, list(entries), 0)
|
||||
|
||||
import hermes_cli.commands as _hc
|
||||
monkeypatch.setattr(
|
||||
_hc, "discord_skill_commands_by_category", fake_categories,
|
||||
)
|
||||
|
||||
def capture_autocomplete(**kwargs):
|
||||
# Only one autocomplete in /skill registration: name=...
|
||||
captured["autocomplete"] = kwargs.get("name")
|
||||
|
||||
def _passthrough(fn):
|
||||
return fn
|
||||
|
||||
return _passthrough
|
||||
|
||||
monkeypatch.setattr(
|
||||
discord.app_commands, "autocomplete", capture_autocomplete,
|
||||
raising=False,
|
||||
)
|
||||
|
||||
registered: list = []
|
||||
|
||||
class _Tree:
|
||||
def get_commands(self):
|
||||
return []
|
||||
|
||||
def add_command(self, cmd):
|
||||
registered.append(cmd)
|
||||
|
||||
adapter._register_skill_group(_Tree())
|
||||
assert registered, "_register_skill_group did not register a command"
|
||||
return registered[0].callback, captured["autocomplete"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_skill_autocomplete_returns_empty_for_unauthorized(
|
||||
adapter, monkeypatch,
|
||||
):
|
||||
"""Autocomplete must not leak the installed skill catalog to users
|
||||
who can't run /skill. With DISCORD_ALLOWED_USERS configured and the
|
||||
interaction user outside it, the autocomplete callback returns []."""
|
||||
adapter._allowed_user_ids = {"100200300"}
|
||||
entries = [
|
||||
("alpha", "First skill", "/alpha"),
|
||||
("beta", "Second skill", "/beta"),
|
||||
]
|
||||
_handler, autocomplete = _capture_skill_registration(
|
||||
adapter, monkeypatch, entries,
|
||||
)
|
||||
|
||||
interaction = _make_interaction("999999999")
|
||||
result = await autocomplete(interaction, "")
|
||||
assert result == []
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_skill_autocomplete_returns_choices_for_authorized(
|
||||
adapter, monkeypatch,
|
||||
):
|
||||
"""Sanity: an authorized user still gets the autocomplete suggestions."""
|
||||
adapter._allowed_user_ids = {"100200300"}
|
||||
entries = [
|
||||
("alpha", "First skill", "/alpha"),
|
||||
("beta", "Second skill", "/beta"),
|
||||
]
|
||||
_handler, autocomplete = _capture_skill_registration(
|
||||
adapter, monkeypatch, entries,
|
||||
)
|
||||
|
||||
interaction = _make_interaction("100200300")
|
||||
result = await autocomplete(interaction, "")
|
||||
assert len(result) == 2
|
||||
assert {choice.value for choice in result} == {"alpha", "beta"}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_skill_handler_rejects_before_dispatch_for_unauthorized(
|
||||
adapter, monkeypatch,
|
||||
):
|
||||
"""The /skill handler must call _check_slash_authorization BEFORE
|
||||
skill_lookup. Otherwise unknown vs known names produce divergent
|
||||
responses ("Unknown skill: foo" vs auth rejection) which is a
|
||||
catalog-probing oracle."""
|
||||
adapter._allowed_user_ids = {"100200300"}
|
||||
entries = [("alpha", "First skill", "/alpha")]
|
||||
handler, _autocomplete = _capture_skill_registration(
|
||||
adapter, monkeypatch, entries,
|
||||
)
|
||||
|
||||
# Patch _run_simple_slash so we can detect any leak through it.
|
||||
dispatched: list = []
|
||||
|
||||
async def fake_dispatch(_interaction, text):
|
||||
dispatched.append(text)
|
||||
|
||||
adapter._run_simple_slash = fake_dispatch # type: ignore[assignment]
|
||||
|
||||
interaction = _make_interaction("999999999")
|
||||
await handler(interaction, "alpha", "")
|
||||
|
||||
interaction.response.send_message.assert_awaited_once()
|
||||
args, kwargs = interaction.response.send_message.call_args
|
||||
assert kwargs.get("ephemeral") is True
|
||||
assert "not authorized" in (
|
||||
args[0] if args else kwargs.get("content", "")
|
||||
).lower()
|
||||
# Critically: nothing was dispatched, and the auth message did NOT
|
||||
# mention the skill name "alpha" (no catalog leak).
|
||||
assert dispatched == []
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_skill_handler_known_and_unknown_produce_same_rejection(
|
||||
adapter, monkeypatch,
|
||||
):
|
||||
"""An unauthorized user probing for valid skill names must see the
|
||||
same rejection text regardless of whether the name they tried is
|
||||
on the registered catalog."""
|
||||
adapter._allowed_user_ids = {"100200300"}
|
||||
entries = [("alpha", "First skill", "/alpha")]
|
||||
handler, _ = _capture_skill_registration(adapter, monkeypatch, entries)
|
||||
|
||||
adapter._run_simple_slash = AsyncMock() # type: ignore[assignment]
|
||||
|
||||
known_interaction = _make_interaction("999999999")
|
||||
unknown_interaction = _make_interaction("999999999")
|
||||
await handler(known_interaction, "alpha", "")
|
||||
await handler(unknown_interaction, "definitely-not-a-skill", "")
|
||||
|
||||
known_interaction.response.send_message.assert_awaited_once()
|
||||
unknown_interaction.response.send_message.assert_awaited_once()
|
||||
known_args, known_kwargs = known_interaction.response.send_message.call_args
|
||||
unknown_args, unknown_kwargs = (
|
||||
unknown_interaction.response.send_message.call_args
|
||||
)
|
||||
assert known_args == unknown_args
|
||||
assert known_kwargs == unknown_kwargs
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_skill_handler_dispatches_for_authorized(
|
||||
adapter, monkeypatch,
|
||||
):
|
||||
"""Sanity: an authorized user reaches _run_simple_slash with the
|
||||
resolved cmd_key and arguments."""
|
||||
adapter._allowed_user_ids = {"100200300"}
|
||||
entries = [("alpha", "First skill", "/alpha")]
|
||||
handler, _ = _capture_skill_registration(adapter, monkeypatch, entries)
|
||||
|
||||
dispatched: list = []
|
||||
|
||||
async def fake_dispatch(_interaction, text):
|
||||
dispatched.append(text)
|
||||
|
||||
adapter._run_simple_slash = fake_dispatch # type: ignore[assignment]
|
||||
|
||||
interaction = _make_interaction("100200300")
|
||||
await handler(interaction, "alpha", "extra args")
|
||||
assert dispatched == ["/alpha extra args"]
|
||||
@@ -107,6 +107,10 @@ def adapter():
|
||||
user=SimpleNamespace(id=99999, name="HermesBot"),
|
||||
)
|
||||
adapter._text_batch_delay_seconds = 0 # disable batching for tests
|
||||
# Slash auth is exercised in test_discord_slash_auth.py — bypass it here
|
||||
# so registration / dispatch / thread behavior tests don't have to
|
||||
# construct a full auth context (allowlist / channel scope).
|
||||
adapter._check_slash_authorization = AsyncMock(return_value=True)
|
||||
return adapter
|
||||
|
||||
|
||||
@@ -117,6 +121,10 @@ def adapter():
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_registers_native_thread_slash_command(adapter):
|
||||
# The /thread slash closure now delegates ALL the work — including
|
||||
# defer() — to _handle_thread_create_slash so the auth gate can send
|
||||
# an ephemeral rejection on the still-unresponded interaction. The
|
||||
# closure should just forward.
|
||||
adapter._handle_thread_create_slash = AsyncMock()
|
||||
adapter._register_slash_commands()
|
||||
|
||||
@@ -127,7 +135,9 @@ async def test_registers_native_thread_slash_command(adapter):
|
||||
|
||||
await command(interaction, name="Planning", message="", auto_archive_duration=1440)
|
||||
|
||||
interaction.response.defer.assert_awaited_once_with(ephemeral=True)
|
||||
# defer is now performed inside _handle_thread_create_slash, AFTER the
|
||||
# auth check passes — not by the closure.
|
||||
interaction.response.defer.assert_not_awaited()
|
||||
adapter._handle_thread_create_slash.assert_awaited_once_with(interaction, "Planning", "", 1440)
|
||||
|
||||
|
||||
@@ -298,6 +308,7 @@ async def test_handle_thread_create_slash_reports_success(adapter):
|
||||
user=SimpleNamespace(display_name="Jezza", id=42),
|
||||
guild=SimpleNamespace(name="TestGuild"),
|
||||
followup=SimpleNamespace(send=AsyncMock()),
|
||||
response=SimpleNamespace(defer=AsyncMock()),
|
||||
)
|
||||
|
||||
await adapter._handle_thread_create_slash(interaction, "Planning", "Kickoff", 1440)
|
||||
@@ -326,6 +337,7 @@ async def test_handle_thread_create_slash_dispatches_session_when_message_provid
|
||||
user=SimpleNamespace(display_name="Jezza", id=42),
|
||||
guild=SimpleNamespace(name="TestGuild"),
|
||||
followup=SimpleNamespace(send=AsyncMock()),
|
||||
response=SimpleNamespace(defer=AsyncMock()),
|
||||
)
|
||||
|
||||
adapter._dispatch_thread_session = AsyncMock()
|
||||
@@ -348,6 +360,7 @@ async def test_handle_thread_create_slash_no_dispatch_without_message(adapter):
|
||||
user=SimpleNamespace(display_name="Jezza", id=42),
|
||||
guild=SimpleNamespace(name="TestGuild"),
|
||||
followup=SimpleNamespace(send=AsyncMock()),
|
||||
response=SimpleNamespace(defer=AsyncMock()),
|
||||
)
|
||||
|
||||
adapter._dispatch_thread_session = AsyncMock()
|
||||
@@ -371,6 +384,7 @@ async def test_handle_thread_create_slash_falls_back_to_seed_message(adapter):
|
||||
user=SimpleNamespace(display_name="Jezza", id=42),
|
||||
guild=SimpleNamespace(name="TestGuild"),
|
||||
followup=SimpleNamespace(send=AsyncMock()),
|
||||
response=SimpleNamespace(defer=AsyncMock()),
|
||||
)
|
||||
|
||||
await adapter._handle_thread_create_slash(interaction, "Planning", "Kickoff", 1440)
|
||||
@@ -395,6 +409,7 @@ async def test_handle_thread_create_slash_reports_failure(adapter):
|
||||
channel_id=123,
|
||||
user=SimpleNamespace(display_name="Jezza", id=42),
|
||||
followup=SimpleNamespace(send=AsyncMock()),
|
||||
response=SimpleNamespace(defer=AsyncMock()),
|
||||
)
|
||||
|
||||
await adapter._handle_thread_create_slash(interaction, "Planning", "", 1440)
|
||||
|
||||
@@ -1771,6 +1771,69 @@ class TestAdapterBehavior(unittest.TestCase):
|
||||
self.assertIn("GIF downgraded to file", caption)
|
||||
self.assertIn("look", caption)
|
||||
|
||||
def test_download_remote_document_reads_response_before_httpx_client_closes(self):
|
||||
"""#18451 — snapshot Content-Type + body while the httpx.AsyncClient
|
||||
context is still active so pooled connections fully release on
|
||||
exit. Otherwise the response is only readable because httpx
|
||||
eagerly buffers it; a future refactor to .stream() would silently
|
||||
read-after-close."""
|
||||
from gateway.config import PlatformConfig
|
||||
from gateway.platforms.feishu import FeishuAdapter
|
||||
|
||||
events: list[str] = []
|
||||
|
||||
class _FakeResponse:
|
||||
headers = {"Content-Type": "application/octet-stream"}
|
||||
|
||||
def raise_for_status(self) -> None:
|
||||
events.append("raise_for_status")
|
||||
|
||||
@property
|
||||
def content(self) -> bytes:
|
||||
events.append("content_read")
|
||||
return b"doc-bytes"
|
||||
|
||||
class _FakeAsyncClient:
|
||||
def __init__(self, *_a: object, **_k: object) -> None:
|
||||
pass
|
||||
|
||||
async def __aenter__(self) -> "_FakeAsyncClient":
|
||||
events.append("client_enter")
|
||||
return self
|
||||
|
||||
async def __aexit__(self, *exc: object) -> None:
|
||||
events.append("client_exit")
|
||||
|
||||
async def get(self, *_a: object, **_k: object) -> _FakeResponse:
|
||||
events.append("get")
|
||||
return _FakeResponse()
|
||||
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
with patch.dict(os.environ, {"HERMES_HOME": tmp}, clear=False):
|
||||
adapter = FeishuAdapter(PlatformConfig())
|
||||
|
||||
async def _run() -> tuple[str, str]:
|
||||
with patch("tools.url_safety.is_safe_url", return_value=True):
|
||||
with patch("httpx.AsyncClient", _FakeAsyncClient):
|
||||
with patch(
|
||||
"gateway.platforms.feishu.cache_document_from_bytes",
|
||||
return_value="/tmp/cached-doc.bin",
|
||||
):
|
||||
return await adapter._download_remote_document(
|
||||
"https://example.com/doc.bin",
|
||||
default_ext=".bin",
|
||||
preferred_name="doc",
|
||||
)
|
||||
|
||||
path, filename = asyncio.run(_run())
|
||||
|
||||
self.assertEqual(path, "/tmp/cached-doc.bin")
|
||||
self.assertTrue(filename)
|
||||
# content_read MUST happen before client_exit — otherwise we're
|
||||
# reading response body after the connection pool has been torn
|
||||
# down, which only works by accident (httpx's eager buffering).
|
||||
self.assertLess(events.index("content_read"), events.index("client_exit"))
|
||||
|
||||
def test_dedup_state_persists_across_adapter_restart(self):
|
||||
from gateway.config import PlatformConfig
|
||||
from gateway.platforms.feishu import FeishuAdapter
|
||||
|
||||
@@ -0,0 +1,217 @@
|
||||
"""Tests for gateway /goal verdict-message delivery.
|
||||
|
||||
The judge verdict message ("✓ Goal achieved", "⏸ budget exhausted", etc.)
|
||||
must reach the user after each turn. Before this fix the code checked
|
||||
``hasattr(adapter, "send_message")`` — but adapters expose ``send()``,
|
||||
never ``send_message``, so the check always evaluated False and users
|
||||
never saw verdicts. This test locks in the fix.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
from datetime import datetime
|
||||
from pathlib import Path
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from gateway.config import GatewayConfig, Platform, PlatformConfig
|
||||
from gateway.session import SessionEntry, SessionSource, build_session_key
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def hermes_home(tmp_path, monkeypatch):
|
||||
home = tmp_path / ".hermes"
|
||||
home.mkdir()
|
||||
monkeypatch.setattr(Path, "home", lambda: tmp_path)
|
||||
monkeypatch.setenv("HERMES_HOME", str(home))
|
||||
|
||||
from hermes_cli import goals
|
||||
|
||||
goals._DB_CACHE.clear()
|
||||
yield home
|
||||
goals._DB_CACHE.clear()
|
||||
|
||||
|
||||
def _make_source() -> SessionSource:
|
||||
return SessionSource(
|
||||
platform=Platform.TELEGRAM,
|
||||
user_id="u1",
|
||||
chat_id="c1",
|
||||
user_name="tester",
|
||||
chat_type="dm",
|
||||
)
|
||||
|
||||
|
||||
class _RecordingAdapter:
|
||||
"""Minimal adapter that records send() invocations."""
|
||||
|
||||
def __init__(self) -> None:
|
||||
self._pending_messages: dict = {}
|
||||
self.sends: list[dict] = []
|
||||
|
||||
async def send(self, chat_id: str, content: str, reply_to=None, metadata=None):
|
||||
self.sends.append({"chat_id": chat_id, "content": content, "metadata": metadata})
|
||||
|
||||
class _R:
|
||||
success = True
|
||||
message_id = "mock-msg"
|
||||
|
||||
return _R()
|
||||
|
||||
|
||||
def _make_runner_with_adapter():
|
||||
from gateway.run import GatewayRunner
|
||||
|
||||
runner = object.__new__(GatewayRunner)
|
||||
runner.config = GatewayConfig(
|
||||
platforms={Platform.TELEGRAM: PlatformConfig(enabled=True, token="***")},
|
||||
)
|
||||
runner.adapters = {}
|
||||
runner._running_agents = {}
|
||||
runner._running_agents_ts = {}
|
||||
runner._queued_events = {}
|
||||
|
||||
src = _make_source()
|
||||
session_entry = SessionEntry(
|
||||
session_key=build_session_key(src),
|
||||
session_id="goal-sess-1",
|
||||
created_at=datetime.now(),
|
||||
updated_at=datetime.now(),
|
||||
platform=Platform.TELEGRAM,
|
||||
chat_type="dm",
|
||||
)
|
||||
|
||||
runner.session_store = MagicMock()
|
||||
runner.session_store.get_or_create_session.return_value = session_entry
|
||||
runner.session_store._generate_session_key.return_value = build_session_key(src)
|
||||
|
||||
adapter = _RecordingAdapter()
|
||||
runner.adapters[Platform.TELEGRAM] = adapter
|
||||
return runner, adapter, session_entry, src
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_goal_verdict_done_sent_via_adapter_send(hermes_home):
|
||||
"""When the judge says done, the '✓ Goal achieved' message must reach
|
||||
the user through the adapter's ``send()`` method."""
|
||||
runner, adapter, session_entry, src = _make_runner_with_adapter()
|
||||
|
||||
from hermes_cli.goals import GoalManager
|
||||
|
||||
mgr = GoalManager(session_entry.session_id)
|
||||
mgr.set("ship the feature")
|
||||
|
||||
with patch("hermes_cli.goals.judge_goal", return_value=("done", "the feature shipped")):
|
||||
runner._post_turn_goal_continuation(
|
||||
session_entry=session_entry,
|
||||
source=src,
|
||||
final_response="I shipped the feature.",
|
||||
)
|
||||
# fire-and-forget create_task — give the loop a tick
|
||||
await asyncio.sleep(0.05)
|
||||
|
||||
assert len(adapter.sends) == 1, f"expected 1 send, got {len(adapter.sends)}: {adapter.sends}"
|
||||
msg = adapter.sends[0]
|
||||
assert msg["chat_id"] == "c1"
|
||||
assert "Goal achieved" in msg["content"]
|
||||
assert "the feature shipped" in msg["content"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_goal_verdict_continue_enqueues_continuation(hermes_home):
|
||||
"""When the judge says continue, both the 'continuing' status and the
|
||||
continuation-prompt event must be delivered. The continuation prompt is
|
||||
routed through the adapter's pending-messages FIFO so the goal loop
|
||||
proceeds on the next turn."""
|
||||
runner, adapter, session_entry, src = _make_runner_with_adapter()
|
||||
|
||||
from hermes_cli.goals import GoalManager
|
||||
|
||||
mgr = GoalManager(session_entry.session_id)
|
||||
mgr.set("polish the docs")
|
||||
|
||||
with patch("hermes_cli.goals.judge_goal", return_value=("continue", "still needs work")):
|
||||
runner._post_turn_goal_continuation(
|
||||
session_entry=session_entry,
|
||||
source=src,
|
||||
final_response="here's a partial edit",
|
||||
)
|
||||
await asyncio.sleep(0.05)
|
||||
|
||||
# Status line sent back
|
||||
assert len(adapter.sends) == 1
|
||||
assert "Continuing toward goal" in adapter.sends[0]["content"]
|
||||
# Continuation prompt enqueued for next turn
|
||||
assert adapter._pending_messages, "continuation prompt must be enqueued in pending_messages"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_goal_verdict_budget_exhausted_sends_pause(hermes_home):
|
||||
"""When the budget is exhausted, a '⏸ Goal paused' message must be sent
|
||||
and no further continuation enqueued."""
|
||||
runner, adapter, session_entry, src = _make_runner_with_adapter()
|
||||
|
||||
from hermes_cli.goals import GoalManager, save_goal
|
||||
|
||||
mgr = GoalManager(session_entry.session_id, default_max_turns=2)
|
||||
state = mgr.set("tiny goal", max_turns=2)
|
||||
state.turns_used = 2
|
||||
save_goal(session_entry.session_id, state)
|
||||
|
||||
with patch("hermes_cli.goals.judge_goal", return_value=("continue", "keep going")):
|
||||
runner._post_turn_goal_continuation(
|
||||
session_entry=session_entry,
|
||||
source=src,
|
||||
final_response="still partial",
|
||||
)
|
||||
await asyncio.sleep(0.05)
|
||||
|
||||
assert len(adapter.sends) == 1
|
||||
content = adapter.sends[0]["content"]
|
||||
assert "paused" in content.lower()
|
||||
assert "turns used" in content.lower()
|
||||
# No continuation enqueued when budget is exhausted
|
||||
assert not adapter._pending_messages
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_goal_verdict_skipped_when_no_active_goal(hermes_home):
|
||||
"""No goal set → the hook is a no-op. Nothing is sent, nothing enqueued."""
|
||||
runner, adapter, session_entry, src = _make_runner_with_adapter()
|
||||
|
||||
runner._post_turn_goal_continuation(
|
||||
session_entry=session_entry,
|
||||
source=src,
|
||||
final_response="anything",
|
||||
)
|
||||
await asyncio.sleep(0.05)
|
||||
|
||||
assert adapter.sends == []
|
||||
assert adapter._pending_messages == {}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_goal_verdict_survives_adapter_without_send(hermes_home):
|
||||
"""Bad adapter (no ``send`` attribute) must not crash the judge hook."""
|
||||
runner, _adapter, session_entry, src = _make_runner_with_adapter()
|
||||
|
||||
from hermes_cli.goals import GoalManager
|
||||
|
||||
GoalManager(session_entry.session_id).set("survive missing send")
|
||||
|
||||
class _NoSendAdapter:
|
||||
def __init__(self):
|
||||
self._pending_messages: dict = {}
|
||||
|
||||
runner.adapters[Platform.TELEGRAM] = _NoSendAdapter()
|
||||
|
||||
with patch("hermes_cli.goals.judge_goal", return_value=("done", "ok")):
|
||||
# must not raise
|
||||
runner._post_turn_goal_continuation(
|
||||
session_entry=session_entry,
|
||||
source=src,
|
||||
final_response="whatever",
|
||||
)
|
||||
await asyncio.sleep(0.05)
|
||||
@@ -8,7 +8,7 @@ to env vars nothing read on startup — the home channel appeared to set
|
||||
successfully but was lost on every new gateway session.
|
||||
"""
|
||||
|
||||
from gateway.run import _home_target_env_var
|
||||
from gateway.run import _home_target_env_var, _home_thread_env_var
|
||||
|
||||
|
||||
def test_matrix_home_target_env_var_uses_home_room():
|
||||
@@ -34,3 +34,9 @@ def test_unknown_platform_home_target_env_var_falls_back_to_home_channel():
|
||||
def test_case_insensitive_platform_name():
|
||||
assert _home_target_env_var("MATRIX") == "MATRIX_HOME_ROOM"
|
||||
assert _home_target_env_var("Email") == "EMAIL_HOME_ADDRESS"
|
||||
|
||||
|
||||
def test_home_thread_env_var_uses_home_target_name_plus_thread_id():
|
||||
assert _home_thread_env_var("discord") == "DISCORD_HOME_CHANNEL_THREAD_ID"
|
||||
assert _home_thread_env_var("matrix") == "MATRIX_HOME_ROOM_THREAD_ID"
|
||||
assert _home_thread_env_var("email") == "EMAIL_HOME_ADDRESS_THREAD_ID"
|
||||
|
||||
@@ -0,0 +1,114 @@
|
||||
"""Tests for the shared httpx.Limits helper that all long-lived platform
|
||||
adapters use to tighten their keep-alive pool.
|
||||
|
||||
Context: #18451 — on macOS behind Cloudflare Warp, httpx's default
|
||||
keepalive_expiry=5s let idle CLOSE_WAIT sockets accumulate across
|
||||
multiple long-lived gateway adapters (QQ Bot, Feishu, WeCom, DingTalk,
|
||||
Signal, BlueBubbles, WeCom-callback) until the process hit the default
|
||||
256 fd limit. These tests just verify the helper returns sensibly
|
||||
tuned limits and respects env-var overrides; the actual fd-pressure
|
||||
behaviour is only observable at runtime under load.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _clear_env(monkeypatch):
|
||||
monkeypatch.delenv("HERMES_GATEWAY_HTTPX_KEEPALIVE_EXPIRY", raising=False)
|
||||
monkeypatch.delenv("HERMES_GATEWAY_HTTPX_MAX_KEEPALIVE", raising=False)
|
||||
|
||||
|
||||
def test_returns_none_when_httpx_unavailable(monkeypatch):
|
||||
"""If httpx can't be imported, the helper returns None so callers
|
||||
fall back to httpx's built-in Limits default without raising."""
|
||||
import gateway.platforms._http_client_limits as mod
|
||||
monkeypatch.setattr(mod, "httpx", None)
|
||||
assert mod.platform_httpx_limits() is None
|
||||
|
||||
|
||||
def test_default_limits_tighten_keepalive_below_httpx_default():
|
||||
import httpx
|
||||
from gateway.platforms._http_client_limits import platform_httpx_limits
|
||||
limits = platform_httpx_limits()
|
||||
assert isinstance(limits, httpx.Limits)
|
||||
# httpx default keepalive_expiry is 5.0 — ours must be shorter so
|
||||
# CLOSE_WAIT sockets drain promptly behind proxies like Warp.
|
||||
assert limits.keepalive_expiry is not None
|
||||
assert limits.keepalive_expiry < 5.0
|
||||
# max_keepalive_connections must be positive and reasonable for a
|
||||
# single adapter (platform APIs rarely parallelise beyond ~10).
|
||||
assert limits.max_keepalive_connections is not None
|
||||
assert 1 <= limits.max_keepalive_connections <= 50
|
||||
|
||||
|
||||
def test_env_override_keepalive_expiry(monkeypatch):
|
||||
monkeypatch.setenv("HERMES_GATEWAY_HTTPX_KEEPALIVE_EXPIRY", "7.5")
|
||||
from gateway.platforms._http_client_limits import platform_httpx_limits
|
||||
limits = platform_httpx_limits()
|
||||
assert limits.keepalive_expiry == 7.5
|
||||
|
||||
|
||||
def test_env_override_max_keepalive(monkeypatch):
|
||||
monkeypatch.setenv("HERMES_GATEWAY_HTTPX_MAX_KEEPALIVE", "25")
|
||||
from gateway.platforms._http_client_limits import platform_httpx_limits
|
||||
limits = platform_httpx_limits()
|
||||
assert limits.max_keepalive_connections == 25
|
||||
|
||||
|
||||
def test_env_override_rejects_garbage(monkeypatch):
|
||||
"""Malformed env values fall back to defaults rather than raising."""
|
||||
monkeypatch.setenv("HERMES_GATEWAY_HTTPX_KEEPALIVE_EXPIRY", "not-a-number")
|
||||
monkeypatch.setenv("HERMES_GATEWAY_HTTPX_MAX_KEEPALIVE", "-3")
|
||||
from gateway.platforms._http_client_limits import platform_httpx_limits
|
||||
limits = platform_httpx_limits()
|
||||
# Non-positive / non-numeric → fell back to defaults (not the override values)
|
||||
assert limits.keepalive_expiry is not None and limits.keepalive_expiry > 0
|
||||
assert limits.max_keepalive_connections is not None
|
||||
assert limits.max_keepalive_connections > 0
|
||||
|
||||
|
||||
def test_helper_is_importable_from_every_platform_that_uses_it():
|
||||
"""Every persistent-httpx-client platform adapter imports this helper.
|
||||
If any of those modules fails to import, this test surfaces it before
|
||||
the regression shows up as a runtime adapter-startup crash."""
|
||||
# Just importing exercises the helper's import path for each adapter.
|
||||
import gateway.platforms.qqbot.adapter # noqa: F401
|
||||
import gateway.platforms.wecom # noqa: F401
|
||||
import gateway.platforms.dingtalk # noqa: F401
|
||||
import gateway.platforms.signal # noqa: F401
|
||||
import gateway.platforms.bluebubbles # noqa: F401
|
||||
import gateway.platforms.wecom_callback # noqa: F401
|
||||
|
||||
|
||||
class TestWhatsappTypingLeakFix:
|
||||
"""#18451 — whatsapp.send_typing previously used a bare
|
||||
`await self._http_session.post(...)` which leaked the aiohttp
|
||||
response object until GC, holding its TCP socket in CLOSE_WAIT.
|
||||
Must now wrap the call in `async with` so the response is
|
||||
released immediately when the call returns.
|
||||
|
||||
We verify by inspecting the source text rather than exercising
|
||||
the coroutine — the test suite would otherwise need a live
|
||||
aiohttp server, and the contract we care about is structural.
|
||||
"""
|
||||
|
||||
def test_bare_await_removed(self):
|
||||
import inspect
|
||||
import gateway.platforms.whatsapp as mod
|
||||
|
||||
src = inspect.getsource(mod.WhatsAppAdapter.send_typing)
|
||||
# The fix must be structural: the post() call is inside an
|
||||
# `async with`, not a bare `await`.
|
||||
assert "async with self._http_session.post(" in src, (
|
||||
"send_typing must wrap self._http_session.post(...) in "
|
||||
"`async with` to release the aiohttp response socket "
|
||||
"(#18451). Otherwise the response sits in CLOSE_WAIT "
|
||||
"until GC."
|
||||
)
|
||||
# The old bare-await form must be gone.
|
||||
assert "await self._http_session.post(" not in src
|
||||
@@ -0,0 +1,244 @@
|
||||
"""Tests for `/reload-skills` resyncing the Discord ``/skill`` autocomplete.
|
||||
|
||||
Before this change, ``_register_skill_group`` captured the skill catalog
|
||||
in closure variables (``entries`` and ``skill_lookup``) so that the one
|
||||
``tree.add_command`` call at startup owned the only live copy of the
|
||||
skill list. The closure is never re-entered after startup, so
|
||||
``/reload-skills`` (which rescans the on-disk skill dir and refreshes
|
||||
the in-process registry) had no way to propagate its results into the
|
||||
autocomplete — new skills stayed invisible in the dropdown and deleted
|
||||
skills returned an "Unknown skill" error when the stale autocomplete
|
||||
entry was clicked.
|
||||
|
||||
The fix promotes those two variables to instance attributes
|
||||
(``_skill_entries`` / ``_skill_lookup``) and exposes a
|
||||
``refresh_skill_group()`` method that rescans and mutates them in
|
||||
place. The gateway ``_handle_reload_skills_command`` iterates its
|
||||
connected adapters and calls the method on any that expose it.
|
||||
|
||||
No ``tree.sync()`` is required because Discord fetches autocomplete
|
||||
options dynamically on every keystroke — we only need to rebind the
|
||||
data the live callbacks already read from.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
|
||||
def _make_adapter():
|
||||
"""Construct a DiscordAdapter without going through __init__ / token checks."""
|
||||
from gateway.platforms.discord import DiscordAdapter
|
||||
from gateway.platforms.base import Platform
|
||||
adapter = object.__new__(DiscordAdapter)
|
||||
adapter.config = MagicMock()
|
||||
adapter.config.extra = {}
|
||||
# ``platform`` is set by BasePlatformAdapter.__init__, which we skip
|
||||
# above; the inherited ``.name`` property dereferences it for log
|
||||
# formatting, so set it explicitly.
|
||||
adapter.platform = Platform.DISCORD
|
||||
return adapter
|
||||
|
||||
|
||||
class TestRefreshSkillGroup:
|
||||
def test_refresh_repopulates_entries_after_catalog_change(
|
||||
self, monkeypatch
|
||||
) -> None:
|
||||
"""The initial catalog is replaced wholesale on refresh.
|
||||
|
||||
Mirrors the observable /reload-skills case: a user adds a new
|
||||
skill to ~/.hermes/skills/, runs /reload-skills, and expects
|
||||
the autocomplete to surface it on the very next keystroke.
|
||||
"""
|
||||
adapter = _make_adapter()
|
||||
|
||||
# Start-of-process state: /register built the catalog from the
|
||||
# original collector output.
|
||||
adapter._skill_entries = [
|
||||
("old-skill", "Pre-existing skill", "/old-skill"),
|
||||
]
|
||||
adapter._skill_lookup = {"old-skill": ("Pre-existing skill", "/old-skill")}
|
||||
adapter._skill_group_reserved_names = set()
|
||||
adapter._skill_group_hidden_count = 0
|
||||
|
||||
# User adds new-skill to disk and removes old-skill.
|
||||
def fake_collector(*, reserved_names):
|
||||
return (
|
||||
{"creative": [("new-skill", "Fresh skill", "/new-skill")]}, # categories
|
||||
[], # uncategorized
|
||||
0, # hidden
|
||||
)
|
||||
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.commands.discord_skill_commands_by_category",
|
||||
fake_collector,
|
||||
)
|
||||
|
||||
new_count, hidden = adapter.refresh_skill_group()
|
||||
|
||||
assert new_count == 1
|
||||
assert hidden == 0
|
||||
# Old skill is gone, new skill is present.
|
||||
names = [n for n, _d, _k in adapter._skill_entries]
|
||||
assert names == ["new-skill"]
|
||||
assert "old-skill" not in adapter._skill_lookup
|
||||
assert adapter._skill_lookup["new-skill"] == ("Fresh skill", "/new-skill")
|
||||
|
||||
def test_refresh_sorts_entries_alphabetically(self, monkeypatch) -> None:
|
||||
"""Autocomplete order must be stable and predictable across refreshes."""
|
||||
adapter = _make_adapter()
|
||||
adapter._skill_entries = []
|
||||
adapter._skill_lookup = {}
|
||||
adapter._skill_group_reserved_names = set()
|
||||
adapter._skill_group_hidden_count = 0
|
||||
|
||||
def fake_collector(*, reserved_names):
|
||||
# Intentionally unsorted — the fix must resort.
|
||||
return (
|
||||
{"zzz": [("zebra", "", "/zebra")]},
|
||||
[("alpha", "", "/alpha")],
|
||||
0,
|
||||
)
|
||||
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.commands.discord_skill_commands_by_category",
|
||||
fake_collector,
|
||||
)
|
||||
|
||||
adapter.refresh_skill_group()
|
||||
|
||||
names = [n for n, _d, _k in adapter._skill_entries]
|
||||
assert names == sorted(names) == ["alpha", "zebra"]
|
||||
|
||||
def test_refresh_handles_collector_exception_gracefully(
|
||||
self, monkeypatch
|
||||
) -> None:
|
||||
"""A broken collector must not take down /reload-skills."""
|
||||
adapter = _make_adapter()
|
||||
adapter._skill_entries = [("keep", "kept", "/keep")]
|
||||
adapter._skill_lookup = {"keep": ("kept", "/keep")}
|
||||
adapter._skill_group_reserved_names = set()
|
||||
adapter._skill_group_hidden_count = 0
|
||||
|
||||
def boom(*, reserved_names):
|
||||
raise RuntimeError("simulated collector failure")
|
||||
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.commands.discord_skill_commands_by_category",
|
||||
boom,
|
||||
)
|
||||
|
||||
new_count, hidden = adapter.refresh_skill_group()
|
||||
# Returns previously-cached count, no crash, existing entries
|
||||
# preserved so the live autocomplete keeps working.
|
||||
assert new_count == 1
|
||||
assert hidden == 0
|
||||
assert adapter._skill_entries == [("keep", "kept", "/keep")]
|
||||
|
||||
|
||||
class TestRegisterSkillGroupUsesInstanceState:
|
||||
"""The closure-based ``entries`` / ``skill_lookup`` must be gone.
|
||||
|
||||
If the callbacks in ``_register_skill_group`` still close over
|
||||
local variables instead of reading from ``self``, the refresh
|
||||
method is useless — autocomplete will keep serving the stale list.
|
||||
|
||||
The full slash-command registration path pulls in ``discord.app_commands``
|
||||
decorators (``@describe`` / ``@autocomplete`` / ``Command``), which
|
||||
are unstubbed in the hermetic test env. We assert the data-shaped
|
||||
side-effects instead: after ``_register_skill_group`` returns
|
||||
(successfully or not), ``_skill_entries`` and ``_skill_lookup`` must
|
||||
be populated from the collector output, because
|
||||
``_refresh_skill_catalog_state`` runs before any decorator evaluation.
|
||||
"""
|
||||
|
||||
def test_refresh_catalog_state_populates_instance_attrs(
|
||||
self, monkeypatch
|
||||
) -> None:
|
||||
adapter = _make_adapter()
|
||||
adapter._skill_group_reserved_names = set()
|
||||
|
||||
def fake_collector(*, reserved_names):
|
||||
return (
|
||||
{"creative": [("ascii-art", "Make ASCII", "/ascii-art")]},
|
||||
[],
|
||||
0,
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.commands.discord_skill_commands_by_category",
|
||||
fake_collector,
|
||||
)
|
||||
|
||||
adapter._refresh_skill_catalog_state()
|
||||
|
||||
# Instance-level state populated — the autocomplete + handler
|
||||
# callbacks both read from these, so `refresh_skill_group`
|
||||
# mutating them in place is enough to pick up new skills.
|
||||
assert adapter._skill_entries == [
|
||||
("ascii-art", "Make ASCII", "/ascii-art"),
|
||||
]
|
||||
assert adapter._skill_lookup == {
|
||||
"ascii-art": ("Make ASCII", "/ascii-art"),
|
||||
}
|
||||
assert adapter._skill_group_hidden_count == 0
|
||||
|
||||
|
||||
class TestHandleReloadSkillsCallsRefreshSkillGroup:
|
||||
"""Gateway-side integration: /reload-skills must call refresh on adapters."""
|
||||
|
||||
def test_orchestrator_calls_refresh_skill_group_on_every_adapter(self):
|
||||
"""Sync + async refresh_skill_group implementations both get awaited/called.
|
||||
|
||||
The orchestrator iterates ``self.adapters`` and calls
|
||||
``refresh_skill_group`` if it exists. Adapters that don't
|
||||
implement it (today: everything except Discord) are silently
|
||||
skipped without raising.
|
||||
"""
|
||||
import asyncio
|
||||
from unittest.mock import patch, MagicMock
|
||||
|
||||
# Import without constructing a real runner — test the method
|
||||
# directly against an ``object.__new__`` instance.
|
||||
from gateway.run import GatewayRunner
|
||||
runner = object.__new__(GatewayRunner)
|
||||
|
||||
sync_refresh = MagicMock(return_value=(5, 0))
|
||||
async_called = {"flag": False}
|
||||
|
||||
class AsyncAdapter:
|
||||
name = "async-platform"
|
||||
async def refresh_skill_group(self):
|
||||
async_called["flag"] = True
|
||||
return (3, 0)
|
||||
|
||||
class SyncAdapter:
|
||||
name = "sync-platform"
|
||||
refresh_skill_group = sync_refresh
|
||||
|
||||
class NoOpAdapter:
|
||||
name = "other"
|
||||
# No refresh_skill_group — must not crash.
|
||||
|
||||
runner.adapters = {
|
||||
"discord": AsyncAdapter(),
|
||||
"slack": SyncAdapter(),
|
||||
"telegram": NoOpAdapter(),
|
||||
}
|
||||
|
||||
# Mock reload_skills itself so no disk scan runs.
|
||||
fake_result = {"added": [], "removed": [], "total": 7}
|
||||
with patch(
|
||||
"agent.skill_commands.reload_skills", return_value=fake_result
|
||||
):
|
||||
event = MagicMock()
|
||||
event.source = MagicMock()
|
||||
# _session_key_for_source may be called — make it safe.
|
||||
runner._session_key_for_source = lambda src: None
|
||||
runner._pending_skills_reload_notes = {}
|
||||
|
||||
result = asyncio.get_event_loop().run_until_complete(
|
||||
runner._handle_reload_skills_command(event)
|
||||
)
|
||||
|
||||
assert "Skills Reloaded" in result
|
||||
assert sync_refresh.called, "sync adapter refresh must be invoked"
|
||||
assert async_called["flag"], "async adapter refresh must be awaited"
|
||||
@@ -8,8 +8,8 @@ from unittest.mock import AsyncMock, MagicMock
|
||||
import pytest
|
||||
|
||||
import gateway.run as gateway_run
|
||||
from gateway.config import Platform
|
||||
from gateway.platforms.base import MessageEvent, MessageType
|
||||
from gateway.config import HomeChannel, Platform
|
||||
from gateway.platforms.base import MessageEvent, MessageType, SendResult
|
||||
from gateway.session import build_session_key
|
||||
from tests.gateway.restart_test_helpers import (
|
||||
make_restart_runner,
|
||||
@@ -17,6 +17,22 @@ from tests.gateway.restart_test_helpers import (
|
||||
)
|
||||
|
||||
|
||||
# ── restart marker helpers ───────────────────────────────────────────────
|
||||
|
||||
|
||||
def test_restart_notification_pending_false_without_marker(tmp_path, monkeypatch):
|
||||
monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path)
|
||||
|
||||
assert gateway_run._restart_notification_pending() is False
|
||||
|
||||
|
||||
def test_restart_notification_pending_true_with_marker(tmp_path, monkeypatch):
|
||||
monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path)
|
||||
(tmp_path / ".restart_notify.json").write_text("{}")
|
||||
|
||||
assert gateway_run._restart_notification_pending() is True
|
||||
|
||||
|
||||
# ── _handle_restart_command writes .restart_notify.json ──────────────────
|
||||
|
||||
|
||||
@@ -143,6 +159,184 @@ async def test_restart_command_uses_atomic_json_writes_for_marker_files(tmp_path
|
||||
assert calls[1][1]["platform"] == "telegram"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sethome_updates_running_config_for_same_process_restart(tmp_path, monkeypatch):
|
||||
"""/sethome persists to env and updates in-memory config before restart."""
|
||||
monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path)
|
||||
|
||||
saved = {}
|
||||
|
||||
def _fake_save_env_value(key, value):
|
||||
saved[key] = value
|
||||
|
||||
monkeypatch.setattr("hermes_cli.config.save_env_value", _fake_save_env_value)
|
||||
|
||||
runner, _adapter = make_restart_runner()
|
||||
source = make_restart_source(chat_id="home-42")
|
||||
source.chat_name = "Ops Home"
|
||||
event = MessageEvent(
|
||||
text="/sethome",
|
||||
message_type=MessageType.TEXT,
|
||||
source=source,
|
||||
message_id="m-home",
|
||||
)
|
||||
|
||||
result = await runner._handle_set_home_command(event)
|
||||
|
||||
home = runner.config.get_home_channel(Platform.TELEGRAM)
|
||||
assert "Home channel set" in result
|
||||
assert saved["TELEGRAM_HOME_CHANNEL"] == "home-42"
|
||||
assert home is not None
|
||||
assert home.chat_id == "home-42"
|
||||
assert home.name == "Ops Home"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sethome_preserves_thread_target_for_same_process_restart(tmp_path, monkeypatch):
|
||||
"""/sethome from a topic/thread stores the thread-aware home target."""
|
||||
monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path)
|
||||
|
||||
saved = {}
|
||||
|
||||
def _fake_save_env_value(key, value):
|
||||
saved[key] = value
|
||||
|
||||
monkeypatch.setattr("hermes_cli.config.save_env_value", _fake_save_env_value)
|
||||
|
||||
runner, _adapter = make_restart_runner()
|
||||
source = make_restart_source(chat_id="parent-42", thread_id="topic-7")
|
||||
source.chat_name = "Ops Topic"
|
||||
event = MessageEvent(
|
||||
text="/sethome",
|
||||
message_type=MessageType.TEXT,
|
||||
source=source,
|
||||
message_id="m-home-thread",
|
||||
)
|
||||
|
||||
result = await runner._handle_set_home_command(event)
|
||||
|
||||
home = runner.config.get_home_channel(Platform.TELEGRAM)
|
||||
assert "Home channel set" in result
|
||||
assert saved["TELEGRAM_HOME_CHANNEL"] == "parent-42"
|
||||
assert saved["TELEGRAM_HOME_CHANNEL_THREAD_ID"] == "topic-7"
|
||||
assert home is not None
|
||||
assert home.chat_id == "parent-42"
|
||||
assert home.thread_id == "topic-7"
|
||||
|
||||
|
||||
# ── home-channel startup notifications ─────────────────────────────────────
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_home_channel_startup_notification_to_configured_home(tmp_path, monkeypatch):
|
||||
monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path)
|
||||
|
||||
runner, adapter = make_restart_runner()
|
||||
runner.config.platforms[Platform.TELEGRAM].home_channel = HomeChannel(
|
||||
platform=Platform.TELEGRAM,
|
||||
chat_id="home-42",
|
||||
name="Ops Home",
|
||||
)
|
||||
adapter.send = AsyncMock()
|
||||
|
||||
delivered = await runner._send_home_channel_startup_notifications()
|
||||
|
||||
assert delivered == {("telegram", "home-42", None)}
|
||||
adapter.send.assert_called_once_with(
|
||||
"home-42",
|
||||
"♻️ Gateway online — Hermes is back and ready.",
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_home_channel_startup_notification_preserves_thread_metadata(
|
||||
tmp_path, monkeypatch
|
||||
):
|
||||
monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path)
|
||||
|
||||
runner, adapter = make_restart_runner()
|
||||
runner.config.platforms[Platform.TELEGRAM].home_channel = HomeChannel(
|
||||
platform=Platform.TELEGRAM,
|
||||
chat_id="parent-42",
|
||||
name="Ops Topic",
|
||||
thread_id="topic-7",
|
||||
)
|
||||
adapter.send = AsyncMock(return_value=SendResult(success=True, message_id="home"))
|
||||
|
||||
delivered = await runner._send_home_channel_startup_notifications()
|
||||
|
||||
assert delivered == {("telegram", "parent-42", "topic-7")}
|
||||
adapter.send.assert_called_once_with(
|
||||
"parent-42",
|
||||
"♻️ Gateway online — Hermes is back and ready.",
|
||||
metadata={"thread_id": "topic-7"},
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_home_channel_startup_notification_skips_restart_target(
|
||||
tmp_path, monkeypatch
|
||||
):
|
||||
monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path)
|
||||
|
||||
runner, adapter = make_restart_runner()
|
||||
runner.config.platforms[Platform.TELEGRAM].home_channel = HomeChannel(
|
||||
platform=Platform.TELEGRAM,
|
||||
chat_id="42",
|
||||
name="Ops Home",
|
||||
)
|
||||
adapter.send = AsyncMock()
|
||||
|
||||
delivered = await runner._send_home_channel_startup_notifications(
|
||||
skip_targets={("telegram", "42", None)}
|
||||
)
|
||||
|
||||
assert delivered == set()
|
||||
adapter.send.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_home_channel_startup_notification_does_not_skip_different_thread(
|
||||
tmp_path, monkeypatch
|
||||
):
|
||||
monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path)
|
||||
|
||||
runner, adapter = make_restart_runner()
|
||||
runner.config.platforms[Platform.TELEGRAM].home_channel = HomeChannel(
|
||||
platform=Platform.TELEGRAM,
|
||||
chat_id="42",
|
||||
name="Ops Home",
|
||||
)
|
||||
adapter.send = AsyncMock(return_value=SendResult(success=True, message_id="home"))
|
||||
|
||||
delivered = await runner._send_home_channel_startup_notifications(
|
||||
skip_targets={("telegram", "42", "topic-7")}
|
||||
)
|
||||
|
||||
assert delivered == {("telegram", "42", None)}
|
||||
adapter.send.assert_called_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_home_channel_startup_notification_ignores_false_send_result(
|
||||
tmp_path, monkeypatch
|
||||
):
|
||||
monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path)
|
||||
|
||||
runner, adapter = make_restart_runner()
|
||||
runner.config.platforms[Platform.TELEGRAM].home_channel = HomeChannel(
|
||||
platform=Platform.TELEGRAM,
|
||||
chat_id="home-42",
|
||||
name="Ops Home",
|
||||
)
|
||||
adapter.send = AsyncMock(return_value=SendResult(success=False, error="network down"))
|
||||
|
||||
delivered = await runner._send_home_channel_startup_notifications()
|
||||
|
||||
assert delivered == set()
|
||||
adapter.send.assert_called_once()
|
||||
|
||||
|
||||
# ── _send_restart_notification ───────────────────────────────────────────
|
||||
|
||||
|
||||
@@ -160,8 +354,9 @@ async def test_send_restart_notification_delivers_and_cleans_up(tmp_path, monkey
|
||||
runner, adapter = make_restart_runner()
|
||||
adapter.send = AsyncMock()
|
||||
|
||||
await runner._send_restart_notification()
|
||||
delivered_target = await runner._send_restart_notification()
|
||||
|
||||
assert delivered_target == ("telegram", "42", None)
|
||||
adapter.send.assert_called_once()
|
||||
call_args = adapter.send.call_args
|
||||
assert call_args[0][0] == "42" # chat_id
|
||||
@@ -185,8 +380,9 @@ async def test_send_restart_notification_with_thread(tmp_path, monkeypatch):
|
||||
runner, adapter = make_restart_runner()
|
||||
adapter.send = AsyncMock()
|
||||
|
||||
await runner._send_restart_notification()
|
||||
delivered_target = await runner._send_restart_notification()
|
||||
|
||||
assert delivered_target == ("telegram", "99", "topic_7")
|
||||
call_args = adapter.send.call_args
|
||||
assert call_args[1]["metadata"] == {"thread_id": "topic_7"}
|
||||
assert not notify_path.exists()
|
||||
@@ -240,6 +436,94 @@ async def test_send_restart_notification_cleans_up_on_send_failure(
|
||||
runner, adapter = make_restart_runner()
|
||||
adapter.send = AsyncMock(side_effect=RuntimeError("network down"))
|
||||
|
||||
await runner._send_restart_notification()
|
||||
delivered_target = await runner._send_restart_notification()
|
||||
|
||||
assert not notify_path.exists() # cleaned up despite error
|
||||
# File cleaned up even though send raised.
|
||||
assert delivered_target is None
|
||||
assert not notify_path.exists()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_restart_notification_logs_warning_on_sendresult_failure(
|
||||
tmp_path, monkeypatch, caplog
|
||||
):
|
||||
"""Adapter that returns SendResult(success=False) must log a WARNING, not INFO.
|
||||
|
||||
Regression guard: adapter.send() catches provider errors (e.g. Telegram
|
||||
"Chat not found") and returns SendResult(success=False) rather than
|
||||
raising. The caller previously ignored the return value and always
|
||||
logged "Sent restart notification to ..." at INFO — masking real
|
||||
delivery failures behind a fake success line.
|
||||
"""
|
||||
from gateway.platforms.base import SendResult
|
||||
|
||||
monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path)
|
||||
|
||||
notify_path = tmp_path / ".restart_notify.json"
|
||||
notify_path.write_text(json.dumps({
|
||||
"platform": "telegram",
|
||||
"chat_id": "42",
|
||||
}))
|
||||
|
||||
runner, adapter = make_restart_runner()
|
||||
adapter.send = AsyncMock(
|
||||
return_value=SendResult(success=False, error="Chat not found"),
|
||||
)
|
||||
|
||||
with caplog.at_level("DEBUG", logger="gateway.run"):
|
||||
delivered_target = await runner._send_restart_notification()
|
||||
|
||||
success_lines = [
|
||||
r for r in caplog.records
|
||||
if r.levelname == "INFO" and "Sent restart notification" in r.getMessage()
|
||||
]
|
||||
warning_lines = [
|
||||
r for r in caplog.records
|
||||
if r.levelname == "WARNING"
|
||||
and "was not delivered" in r.getMessage()
|
||||
and "Chat not found" in r.getMessage()
|
||||
]
|
||||
assert delivered_target is None
|
||||
assert not success_lines, (
|
||||
"Expected no INFO 'Sent restart notification' line when send failed, "
|
||||
f"got: {[r.getMessage() for r in success_lines]}"
|
||||
)
|
||||
assert warning_lines, (
|
||||
"Expected a WARNING line mentioning the failure; "
|
||||
f"got records: {[(r.levelname, r.getMessage()) for r in caplog.records]}"
|
||||
)
|
||||
# Still cleans up.
|
||||
assert not notify_path.exists()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_restart_notification_logs_info_on_sendresult_success(
|
||||
tmp_path, monkeypatch, caplog
|
||||
):
|
||||
"""Adapter returning SendResult(success=True) keeps the INFO log line."""
|
||||
from gateway.platforms.base import SendResult
|
||||
|
||||
monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path)
|
||||
|
||||
notify_path = tmp_path / ".restart_notify.json"
|
||||
notify_path.write_text(json.dumps({
|
||||
"platform": "telegram",
|
||||
"chat_id": "42",
|
||||
}))
|
||||
|
||||
runner, adapter = make_restart_runner()
|
||||
adapter.send = AsyncMock(return_value=SendResult(success=True, message_id="m-1"))
|
||||
|
||||
with caplog.at_level("DEBUG", logger="gateway.run"):
|
||||
delivered_target = await runner._send_restart_notification()
|
||||
|
||||
success_lines = [
|
||||
r for r in caplog.records
|
||||
if r.levelname == "INFO" and "Sent restart notification" in r.getMessage()
|
||||
]
|
||||
assert delivered_target == ("telegram", "42", None)
|
||||
assert success_lines, (
|
||||
"Expected INFO 'Sent restart notification' when send succeeded; "
|
||||
f"got records: {[(r.levelname, r.getMessage()) for r in caplog.records]}"
|
||||
)
|
||||
assert not notify_path.exists()
|
||||
|
||||
@@ -32,7 +32,8 @@ from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from gateway.config import GatewayConfig, Platform, PlatformConfig
|
||||
from gateway.config import GatewayConfig, HomeChannel, Platform, PlatformConfig
|
||||
from gateway.platforms.base import SendResult
|
||||
from gateway.run import (
|
||||
_auto_continue_freshness_window,
|
||||
_coerce_gateway_timestamp,
|
||||
@@ -376,8 +377,8 @@ class TestSuspendRecentlyActiveSkipsResumePending:
|
||||
assert e.suspended is False
|
||||
assert e.resume_pending is True
|
||||
|
||||
def test_non_resume_pending_still_suspended(self, tmp_path):
|
||||
"""Non-resume sessions still get the old crash-recovery suspension."""
|
||||
def test_non_resume_pending_gets_resume_pending(self, tmp_path):
|
||||
"""Non-resume sessions are now marked resume_pending (not suspended)."""
|
||||
store = _make_store(tmp_path)
|
||||
source_a = _make_source(chat_id="a")
|
||||
source_b = _make_source(chat_id="b")
|
||||
@@ -386,9 +387,11 @@ class TestSuspendRecentlyActiveSkipsResumePending:
|
||||
store.mark_resume_pending(entry_a.session_key)
|
||||
|
||||
count = store.suspend_recently_active()
|
||||
# entry_a is already resume_pending → skipped. entry_b gets marked.
|
||||
assert count == 1
|
||||
assert store._entries[entry_a.session_key].suspended is False
|
||||
assert store._entries[entry_b.session_key].suspended is True
|
||||
assert store._entries[entry_b.session_key].resume_pending is True
|
||||
assert store._entries[entry_b.session_key].suspended is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -929,6 +932,84 @@ async def test_restart_banner_uses_try_to_resume_wording():
|
||||
assert "try to resume" in msg
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_restart_notifies_home_channel_even_without_active_sessions():
|
||||
runner, adapter = make_restart_runner()
|
||||
runner._restart_requested = True
|
||||
runner.config.platforms[Platform.TELEGRAM].home_channel = HomeChannel(
|
||||
platform=Platform.TELEGRAM,
|
||||
chat_id="home-42",
|
||||
name="Ops Home",
|
||||
)
|
||||
|
||||
await runner._notify_active_sessions_of_shutdown()
|
||||
|
||||
assert adapter.sent == [
|
||||
"⚠️ Gateway restarting — Your current task will be interrupted. "
|
||||
"Send any message after restart and I'll try to resume where you left off."
|
||||
]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_restart_home_channel_notification_dedupes_active_chat():
|
||||
runner, adapter = make_restart_runner()
|
||||
runner._restart_requested = True
|
||||
runner._running_agents["agent:main:telegram:dm:999"] = MagicMock()
|
||||
runner.config.platforms[Platform.TELEGRAM].home_channel = HomeChannel(
|
||||
platform=Platform.TELEGRAM,
|
||||
chat_id="999",
|
||||
name="Ops Home",
|
||||
)
|
||||
|
||||
await runner._notify_active_sessions_of_shutdown()
|
||||
|
||||
assert len(adapter.sent) == 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_restart_home_channel_notification_not_deduped_across_threads():
|
||||
runner, adapter = make_restart_runner()
|
||||
runner._restart_requested = True
|
||||
session_key = "agent:main:telegram:group:999"
|
||||
runner.session_store._entries[session_key] = MagicMock(
|
||||
origin=SessionSource(
|
||||
platform=Platform.TELEGRAM,
|
||||
chat_id="999",
|
||||
chat_type="group",
|
||||
user_id="u1",
|
||||
thread_id="topic-7",
|
||||
)
|
||||
)
|
||||
runner._running_agents[session_key] = MagicMock()
|
||||
runner.config.platforms[Platform.TELEGRAM].home_channel = HomeChannel(
|
||||
platform=Platform.TELEGRAM,
|
||||
chat_id="999",
|
||||
name="Ops Home",
|
||||
)
|
||||
|
||||
await runner._notify_active_sessions_of_shutdown()
|
||||
|
||||
assert len(adapter.sent) == 2
|
||||
assert adapter.sent_calls[0][2] == {"thread_id": "topic-7"}
|
||||
assert adapter.sent_calls[1][2] is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_restart_home_channel_notification_ignores_false_send_result():
|
||||
runner, adapter = make_restart_runner()
|
||||
runner._restart_requested = True
|
||||
runner.config.platforms[Platform.TELEGRAM].home_channel = HomeChannel(
|
||||
platform=Platform.TELEGRAM,
|
||||
chat_id="home-42",
|
||||
name="Ops Home",
|
||||
)
|
||||
adapter.send = AsyncMock(return_value=SendResult(success=False, error="network down"))
|
||||
|
||||
await runner._notify_active_sessions_of_shutdown()
|
||||
|
||||
adapter.send.assert_called_once()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Stuck-loop escalation integration
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -231,6 +231,55 @@ class TestSlackConnectCleanup:
|
||||
mock_release.assert_called_once_with("slack-app-token", "xapp-fake")
|
||||
assert adapter._platform_lock_identity is None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_reconnect_closes_previous_handler_to_prevent_zombie_socket(self):
|
||||
"""Regression for #18980: calling connect() on an adapter that already has
|
||||
a live handler (e.g. during a gateway restart) must close the old
|
||||
AsyncSocketModeHandler before creating a new one. Without this guard,
|
||||
the old Socket Mode websocket stays alive and both connections dispatch
|
||||
every Slack event, producing double responses — the same bug that
|
||||
affected DiscordAdapter (#18187).
|
||||
"""
|
||||
config = PlatformConfig(enabled=True, token="xoxb-fake")
|
||||
adapter = SlackAdapter(config)
|
||||
|
||||
# Simulate state left over from a prior connect() call.
|
||||
first_handler = AsyncMock()
|
||||
first_handler.close_async = AsyncMock()
|
||||
adapter._handler = first_handler
|
||||
|
||||
mock_app = MagicMock()
|
||||
def _noop_decorator(event_type):
|
||||
def decorator(fn): return fn
|
||||
return decorator
|
||||
mock_app.event = _noop_decorator
|
||||
mock_app.command = _noop_decorator
|
||||
mock_app.action = _noop_decorator
|
||||
mock_app.client = AsyncMock()
|
||||
|
||||
mock_web_client = AsyncMock()
|
||||
mock_web_client.auth_test = AsyncMock(return_value={
|
||||
"user_id": "U_BOT",
|
||||
"user": "testbot",
|
||||
"team_id": "T_FAKE",
|
||||
"team": "FakeTeam",
|
||||
})
|
||||
|
||||
second_handler = MagicMock()
|
||||
|
||||
with patch.object(_slack_mod, "AsyncApp", return_value=mock_app), \
|
||||
patch.object(_slack_mod, "AsyncWebClient", return_value=mock_web_client), \
|
||||
patch.object(_slack_mod, "AsyncSocketModeHandler", return_value=second_handler), \
|
||||
patch.dict(os.environ, {"SLACK_APP_TOKEN": "xapp-fake"}), \
|
||||
patch("gateway.status.acquire_scoped_lock", return_value=(True, None)), \
|
||||
patch("gateway.status.release_scoped_lock"), \
|
||||
patch("asyncio.create_task"):
|
||||
result = await adapter.connect()
|
||||
|
||||
assert result is True
|
||||
first_handler.close_async.assert_awaited_once_with()
|
||||
assert adapter._handler is second_handler
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# TestSlackProxyBehavior
|
||||
|
||||
@@ -132,6 +132,7 @@ async def test_reconnect_success_resets_error_count():
|
||||
|
||||
mock_app = MagicMock()
|
||||
mock_app.updater = mock_updater
|
||||
mock_app.bot.get_me = AsyncMock(return_value=MagicMock()) # heartbeat probe path
|
||||
adapter._app = mock_app
|
||||
|
||||
with patch("asyncio.sleep", new_callable=AsyncMock):
|
||||
@@ -139,6 +140,15 @@ async def test_reconnect_success_resets_error_count():
|
||||
|
||||
assert adapter._polling_network_error_count == 0
|
||||
|
||||
# Clean up the heartbeat-probe task scheduled after a successful reconnect.
|
||||
pending = [t for t in adapter._background_tasks if not t.done()]
|
||||
for t in pending:
|
||||
t.cancel()
|
||||
try:
|
||||
await t
|
||||
except (asyncio.CancelledError, Exception):
|
||||
pass
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_reconnect_triggers_fatal_after_max_retries():
|
||||
@@ -284,3 +294,182 @@ async def test_drain_helper_noop_without_app():
|
||||
adapter._app = None
|
||||
# Should not raise
|
||||
await adapter._drain_polling_connections()
|
||||
|
||||
|
||||
# ── Heartbeat probe ──────────────────────────────────────────────────────
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_heartbeat_probe_no_op_when_polling_healthy():
|
||||
"""
|
||||
Probe scheduled after a successful reconnect: Updater.running=True and
|
||||
bot.get_me() returns quickly → recovery confirmed, no further action.
|
||||
"""
|
||||
adapter = _make_adapter()
|
||||
|
||||
mock_updater = MagicMock()
|
||||
mock_updater.running = True
|
||||
|
||||
mock_app = MagicMock()
|
||||
mock_app.updater = mock_updater
|
||||
mock_app.bot.get_me = AsyncMock(return_value=MagicMock())
|
||||
adapter._app = mock_app
|
||||
|
||||
adapter._handle_polling_network_error = AsyncMock()
|
||||
|
||||
with patch("asyncio.sleep", new_callable=AsyncMock):
|
||||
await adapter._verify_polling_after_reconnect()
|
||||
|
||||
mock_app.bot.get_me.assert_awaited_once()
|
||||
adapter._handle_polling_network_error.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_heartbeat_probe_reenters_ladder_when_updater_not_running():
|
||||
"""
|
||||
If Updater.running has flipped to False by the heartbeat delay, treat
|
||||
as wedged: re-enter the reconnect ladder.
|
||||
"""
|
||||
adapter = _make_adapter()
|
||||
|
||||
mock_updater = MagicMock()
|
||||
mock_updater.running = False
|
||||
|
||||
mock_app = MagicMock()
|
||||
mock_app.updater = mock_updater
|
||||
mock_app.bot.get_me = AsyncMock()
|
||||
adapter._app = mock_app
|
||||
|
||||
adapter._handle_polling_network_error = AsyncMock()
|
||||
|
||||
with patch("asyncio.sleep", new_callable=AsyncMock):
|
||||
await adapter._verify_polling_after_reconnect()
|
||||
|
||||
mock_app.bot.get_me.assert_not_called()
|
||||
adapter._handle_polling_network_error.assert_awaited_once()
|
||||
err = adapter._handle_polling_network_error.await_args.args[0]
|
||||
assert isinstance(err, RuntimeError)
|
||||
assert "not running" in str(err).lower()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_heartbeat_probe_reenters_ladder_when_get_me_times_out():
|
||||
"""
|
||||
If bot.get_me() hangs longer than PROBE_TIMEOUT, treat as wedged.
|
||||
Simulates the connection-pool wedge that motivated this fix.
|
||||
"""
|
||||
adapter = _make_adapter()
|
||||
|
||||
mock_updater = MagicMock()
|
||||
mock_updater.running = True
|
||||
|
||||
async def hang_forever(*args, **kwargs):
|
||||
await asyncio.sleep(3600)
|
||||
|
||||
mock_app = MagicMock()
|
||||
mock_app.updater = mock_updater
|
||||
mock_app.bot.get_me = AsyncMock(side_effect=hang_forever)
|
||||
adapter._app = mock_app
|
||||
|
||||
adapter._handle_polling_network_error = AsyncMock()
|
||||
|
||||
async def fast_wait_for(coro, timeout):
|
||||
if asyncio.iscoroutine(coro):
|
||||
coro.close()
|
||||
raise asyncio.TimeoutError()
|
||||
|
||||
with patch("asyncio.sleep", new_callable=AsyncMock):
|
||||
with patch("gateway.platforms.telegram.asyncio.wait_for", new=fast_wait_for):
|
||||
await adapter._verify_polling_after_reconnect()
|
||||
|
||||
adapter._handle_polling_network_error.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_heartbeat_probe_reenters_ladder_on_get_me_network_error():
|
||||
"""
|
||||
Any exception raised by bot.get_me() (NetworkError, ConnectionError, etc.)
|
||||
should re-enter the reconnect ladder with the original exception.
|
||||
"""
|
||||
adapter = _make_adapter()
|
||||
|
||||
mock_updater = MagicMock()
|
||||
mock_updater.running = True
|
||||
|
||||
mock_app = MagicMock()
|
||||
mock_app.updater = mock_updater
|
||||
mock_app.bot.get_me = AsyncMock(side_effect=ConnectionError("pool wedged"))
|
||||
adapter._app = mock_app
|
||||
|
||||
adapter._handle_polling_network_error = AsyncMock()
|
||||
|
||||
with patch("asyncio.sleep", new_callable=AsyncMock):
|
||||
await adapter._verify_polling_after_reconnect()
|
||||
|
||||
adapter._handle_polling_network_error.assert_awaited_once()
|
||||
assert isinstance(
|
||||
adapter._handle_polling_network_error.await_args.args[0], ConnectionError
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_heartbeat_probe_skips_when_already_fatal():
|
||||
"""
|
||||
If the adapter is already in fatal-error state by the time the probe
|
||||
delay elapses, the probe should bail without further action.
|
||||
"""
|
||||
adapter = _make_adapter()
|
||||
adapter._set_fatal_error("telegram_polling_conflict", "already fatal", retryable=False)
|
||||
|
||||
mock_app = MagicMock()
|
||||
mock_app.bot.get_me = AsyncMock()
|
||||
adapter._app = mock_app
|
||||
|
||||
adapter._handle_polling_network_error = AsyncMock()
|
||||
|
||||
with patch("asyncio.sleep", new_callable=AsyncMock):
|
||||
await adapter._verify_polling_after_reconnect()
|
||||
|
||||
mock_app.bot.get_me.assert_not_called()
|
||||
adapter._handle_polling_network_error.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_reconnect_schedules_heartbeat_probe_on_success():
|
||||
"""
|
||||
After a successful start_polling() in the reconnect path, a probe task
|
||||
must be added to _background_tasks. Without it, a wedged Updater would
|
||||
sit silent indefinitely with no further error_callback to advance the
|
||||
reconnect ladder.
|
||||
"""
|
||||
adapter = _make_adapter()
|
||||
adapter._polling_network_error_count = 1
|
||||
|
||||
mock_updater = MagicMock()
|
||||
mock_updater.running = True
|
||||
mock_updater.stop = AsyncMock()
|
||||
mock_updater.start_polling = AsyncMock() # succeeds
|
||||
|
||||
mock_app = MagicMock()
|
||||
mock_app.updater = mock_updater
|
||||
mock_app.bot.get_me = AsyncMock(return_value=MagicMock())
|
||||
adapter._app = mock_app
|
||||
|
||||
initial_count = len(adapter._background_tasks)
|
||||
|
||||
with patch("asyncio.sleep", new_callable=AsyncMock):
|
||||
await adapter._handle_polling_network_error(Exception("Bad Gateway"))
|
||||
|
||||
assert len(adapter._background_tasks) > initial_count, (
|
||||
"Expected a heartbeat probe task to be scheduled after a successful "
|
||||
"reconnect's start_polling()"
|
||||
)
|
||||
|
||||
# Clean up.
|
||||
pending = [t for t in adapter._background_tasks if not t.done()]
|
||||
for t in pending:
|
||||
t.cancel()
|
||||
try:
|
||||
await t
|
||||
except (asyncio.CancelledError, Exception):
|
||||
pass
|
||||
|
||||
@@ -0,0 +1,185 @@
|
||||
"""Tests for gateway.run._check_unavailable_skill.
|
||||
|
||||
Regression coverage for the dir-name-vs-frontmatter-name drift bug.
|
||||
The hint function used to compare the skill's parent-directory name
|
||||
against the typed command and the disabled list. That silently missed
|
||||
every skill whose directory name differs from its declared frontmatter
|
||||
name (~19 skills on a standard install), so users typing a real slug
|
||||
like ``/stable-diffusion-image-generation`` got a generic "unknown
|
||||
command" response instead of the intended "disabled — enable with …"
|
||||
or "not installed — install with …" hint.
|
||||
|
||||
These tests pin the fixed behavior:
|
||||
|
||||
* Slug is derived from the frontmatter ``name:`` (exactly matching
|
||||
:func:`agent.skill_commands.scan_skill_commands`), so the slug differs
|
||||
from the directory name when the declared name is multi-word.
|
||||
* ``disabled`` membership is checked by the declared name, because that
|
||||
is what :func:`hermes_cli.skills_config.save_disabled_skills` stores.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def tmp_skills(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path:
|
||||
"""Isolated skills dir + HERMES_HOME so the real user config is untouched."""
|
||||
home = tmp_path / ".hermes"
|
||||
home.mkdir()
|
||||
(home / "skills").mkdir()
|
||||
monkeypatch.setenv("HERMES_HOME", str(home))
|
||||
monkeypatch.setattr(Path, "home", lambda: tmp_path)
|
||||
return home / "skills"
|
||||
|
||||
|
||||
def _write_skill(skills_dir: Path, rel: str, frontmatter_name: str) -> Path:
|
||||
"""Create a SKILL.md at ``<skills_dir>/<rel>/SKILL.md``."""
|
||||
skill_dir = skills_dir / rel
|
||||
skill_dir.mkdir(parents=True, exist_ok=True)
|
||||
skill_md = skill_dir / "SKILL.md"
|
||||
skill_md.write_text(
|
||||
f"---\nname: {frontmatter_name}\ndescription: test skill\n---\nBody.\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
return skill_md
|
||||
|
||||
|
||||
def test_frontmatter_slug_matched_even_when_dir_name_differs(
|
||||
tmp_skills: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
"""Directory ``stable-diffusion`` + frontmatter ``Stable Diffusion Image Generation``.
|
||||
|
||||
Command typed: ``stable-diffusion-image-generation`` (the slug the
|
||||
agent actually registers). The old dir-name-based check would have
|
||||
compared ``stable-diffusion`` to the typed command and missed.
|
||||
"""
|
||||
from gateway import run as gateway_run
|
||||
|
||||
_write_skill(tmp_skills, "mlops/stable-diffusion", "Stable Diffusion Image Generation")
|
||||
|
||||
# Config disables by declared name (matches what `hermes skills config` writes).
|
||||
monkeypatch.setattr(
|
||||
"gateway.run._get_disabled_skill_names",
|
||||
lambda: {"Stable Diffusion Image Generation"},
|
||||
raising=False,
|
||||
)
|
||||
with patch(
|
||||
"tools.skills_tool._get_disabled_skill_names",
|
||||
return_value={"Stable Diffusion Image Generation"},
|
||||
), patch(
|
||||
"agent.skill_utils.get_all_skills_dirs",
|
||||
return_value=[tmp_skills],
|
||||
):
|
||||
msg = gateway_run._check_unavailable_skill("stable-diffusion-image-generation")
|
||||
|
||||
assert msg is not None, (
|
||||
"expected a 'disabled' hint for the frontmatter-derived slug; "
|
||||
"the old code compared the dir name 'stable-diffusion' and returned None"
|
||||
)
|
||||
assert "disabled" in msg.lower()
|
||||
assert "hermes skills config" in msg
|
||||
|
||||
|
||||
def test_unknown_command_still_returns_none(
|
||||
tmp_skills: Path,
|
||||
) -> None:
|
||||
"""A command that matches no on-disk skill still returns None."""
|
||||
from gateway import run as gateway_run
|
||||
|
||||
_write_skill(tmp_skills, "creative/ascii-art", "ascii-art")
|
||||
|
||||
with patch(
|
||||
"tools.skills_tool._get_disabled_skill_names", return_value=set()
|
||||
), patch(
|
||||
"agent.skill_utils.get_all_skills_dirs", return_value=[tmp_skills]
|
||||
):
|
||||
assert gateway_run._check_unavailable_skill("no-such-skill") is None
|
||||
|
||||
|
||||
def test_matched_but_not_disabled_returns_none(
|
||||
tmp_skills: Path,
|
||||
) -> None:
|
||||
"""A skill that exists and isn't disabled shouldn't produce a hint."""
|
||||
from gateway import run as gateway_run
|
||||
|
||||
_write_skill(tmp_skills, "creative/ascii-art", "ascii-art")
|
||||
|
||||
with patch(
|
||||
"tools.skills_tool._get_disabled_skill_names", return_value=set()
|
||||
), patch(
|
||||
"agent.skill_utils.get_all_skills_dirs", return_value=[tmp_skills]
|
||||
):
|
||||
assert gateway_run._check_unavailable_skill("ascii-art") is None
|
||||
|
||||
|
||||
def test_slug_normalization_strips_non_alnum(
|
||||
tmp_skills: Path,
|
||||
) -> None:
|
||||
"""Frontmatter ``C++ Code Review`` → slug ``c-code-review`` (``+`` stripped)."""
|
||||
from gateway import run as gateway_run
|
||||
|
||||
_write_skill(tmp_skills, "software-development/cpp-review", "C++ Code Review")
|
||||
|
||||
with patch(
|
||||
"tools.skills_tool._get_disabled_skill_names",
|
||||
return_value={"C++ Code Review"},
|
||||
), patch(
|
||||
"agent.skill_utils.get_all_skills_dirs", return_value=[tmp_skills]
|
||||
):
|
||||
msg = gateway_run._check_unavailable_skill("c-code-review")
|
||||
|
||||
assert msg is not None
|
||||
assert "disabled" in msg.lower()
|
||||
|
||||
|
||||
def test_optional_skill_uses_frontmatter_slug(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
"""Same drift bug applies to the optional-skills branch.
|
||||
|
||||
Before: directory name was matched against the typed command, so an
|
||||
optional skill at ``optional-skills/mlops/stable-diffusion/SKILL.md``
|
||||
with frontmatter ``Stable Diffusion Image Generation`` returned None
|
||||
when the user typed the real slug.
|
||||
"""
|
||||
from gateway import run as gateway_run
|
||||
|
||||
# Build an isolated optional-skills dir
|
||||
optional = tmp_path / "optional-skills"
|
||||
skill_dir = optional / "mlops" / "stable-diffusion"
|
||||
skill_dir.mkdir(parents=True)
|
||||
(skill_dir / "SKILL.md").write_text(
|
||||
"---\nname: Stable Diffusion Image Generation\ndescription: test\n---\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
|
||||
# Point the optional lookup at our tmp dir. The source reads from
|
||||
# ``get_optional_skills_dir(repo_root / "optional-skills")`` — we
|
||||
# can't easily retarget ``repo_root``, so patch the resolver.
|
||||
monkeypatch.setattr(
|
||||
"hermes_constants.get_optional_skills_dir",
|
||||
lambda _default: optional,
|
||||
raising=False,
|
||||
)
|
||||
|
||||
# Ensure the "disabled" branch doesn't match anything so we fall
|
||||
# through to the optional-skills branch.
|
||||
empty_skills = tmp_path / "empty-skills"
|
||||
empty_skills.mkdir()
|
||||
with patch(
|
||||
"tools.skills_tool._get_disabled_skill_names", return_value=set()
|
||||
), patch(
|
||||
"agent.skill_utils.get_all_skills_dirs", return_value=[empty_skills]
|
||||
):
|
||||
msg = gateway_run._check_unavailable_skill("stable-diffusion-image-generation")
|
||||
|
||||
assert msg is not None, (
|
||||
"optional-skills branch should recognize the frontmatter-derived slug; "
|
||||
"the old dir-name-based check returned None here too"
|
||||
)
|
||||
assert "not installed" in msg.lower()
|
||||
assert "official/mlops/stable-diffusion" in msg
|
||||
@@ -284,6 +284,66 @@ class TestBridgeRuntimeFailure:
|
||||
mock_fh.close.assert_called_once()
|
||||
assert adapter._bridge_log_fh is None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("returncode", [0, -2, -15])
|
||||
async def test_shutdown_suppresses_fatal_on_planned_bridge_exit(self, returncode):
|
||||
"""During graceful disconnect(), SIGTERM/SIGINT/clean-exit are NOT fatal.
|
||||
|
||||
Regression guard for the bug where every gateway shutdown/restart
|
||||
logged "Fatal whatsapp adapter error (whatsapp_bridge_exited)" and
|
||||
dispatched a fatal-error notification just before the normal
|
||||
"✓ whatsapp disconnected" — because _check_managed_bridge_exit()
|
||||
saw the bridge's returncode of -15 (our own SIGTERM) and classified
|
||||
it as an unexpected crash.
|
||||
"""
|
||||
adapter = _make_adapter()
|
||||
fatal_handler = AsyncMock()
|
||||
adapter.set_fatal_error_handler(fatal_handler)
|
||||
adapter._running = True
|
||||
adapter._http_session = MagicMock()
|
||||
adapter._bridge_log_fh = MagicMock()
|
||||
adapter._shutting_down = True # disconnect() sets this before SIGTERM
|
||||
|
||||
mock_proc = MagicMock()
|
||||
mock_proc.poll.return_value = returncode
|
||||
adapter._bridge_process = mock_proc
|
||||
|
||||
result = await adapter._check_managed_bridge_exit()
|
||||
|
||||
assert result is None, (
|
||||
f"returncode={returncode} during shutdown should be suppressed, "
|
||||
f"got fatal message: {result!r}"
|
||||
)
|
||||
assert adapter.fatal_error_code is None
|
||||
fatal_handler.assert_not_awaited()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_shutdown_still_surfaces_nonzero_crash(self):
|
||||
"""Even during shutdown, a truly crashed bridge (e.g. returncode 9) is fatal.
|
||||
|
||||
The suppression list is deliberately narrow (0, -2, -15) so that
|
||||
OOM-kill (137), assertion failures, or custom error exits still
|
||||
reach the fatal-error handler and user notification path.
|
||||
"""
|
||||
adapter = _make_adapter()
|
||||
fatal_handler = AsyncMock()
|
||||
adapter.set_fatal_error_handler(fatal_handler)
|
||||
adapter._running = True
|
||||
adapter._http_session = MagicMock()
|
||||
adapter._bridge_log_fh = MagicMock()
|
||||
adapter._shutting_down = True
|
||||
|
||||
mock_proc = MagicMock()
|
||||
mock_proc.poll.return_value = 137 # SIGKILL / OOM-kill
|
||||
adapter._bridge_process = mock_proc
|
||||
|
||||
result = await adapter._check_managed_bridge_exit()
|
||||
|
||||
assert result is not None
|
||||
assert "exited unexpectedly" in result
|
||||
assert adapter.fatal_error_code == "whatsapp_bridge_exited"
|
||||
fatal_handler.assert_awaited_once()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_closed_when_http_not_ready(self):
|
||||
"""Health endpoint never returns 200 within 15 attempts."""
|
||||
|
||||
@@ -203,6 +203,30 @@ class TestListAuthenticatedProvidersBedrock:
|
||||
bedrock = next((p for p in providers if p["slug"] == "bedrock"), None)
|
||||
assert bedrock is None, "bedrock should NOT appear when AWS credentials are absent"
|
||||
|
||||
def test_non_bedrock_picker_does_not_probe_full_aws_chain(self, monkeypatch):
|
||||
"""Non-Bedrock provider discovery must not touch boto3's full credential chain."""
|
||||
from hermes_cli.model_switch import list_authenticated_providers
|
||||
|
||||
monkeypatch.delenv("AWS_PROFILE", raising=False)
|
||||
monkeypatch.delenv("AWS_ACCESS_KEY_ID", raising=False)
|
||||
monkeypatch.delenv("AWS_SECRET_ACCESS_KEY", raising=False)
|
||||
monkeypatch.delenv("AWS_BEARER_TOKEN_BEDROCK", raising=False)
|
||||
monkeypatch.delenv("AWS_WEB_IDENTITY_TOKEN_FILE", raising=False)
|
||||
monkeypatch.delenv("AWS_CONTAINER_CREDENTIALS_RELATIVE_URI", raising=False)
|
||||
monkeypatch.delenv("AWS_CONTAINER_CREDENTIALS_FULL_URI", raising=False)
|
||||
|
||||
calls = {"has_aws_credentials": 0}
|
||||
|
||||
def _has_aws_credentials():
|
||||
calls["has_aws_credentials"] += 1
|
||||
return False
|
||||
|
||||
with patch("agent.bedrock_adapter.has_aws_credentials", side_effect=_has_aws_credentials):
|
||||
providers = list_authenticated_providers(current_provider="openrouter", max_models=0)
|
||||
|
||||
assert calls["has_aws_credentials"] == 0
|
||||
assert all(p["slug"] != "bedrock" for p in providers)
|
||||
|
||||
def test_bedrock_falls_back_to_curated_when_discovery_fails(self, monkeypatch):
|
||||
"""When discover_bedrock_models() raises, fall back to curated list without crashing."""
|
||||
from hermes_cli.model_switch import list_authenticated_providers
|
||||
|
||||
@@ -822,6 +822,103 @@ class TestClampTelegramNames:
|
||||
assert result[0] == ("foo", "d1")
|
||||
|
||||
|
||||
class TestClampCommandNamesTriples:
|
||||
"""Tests for _clamp_command_names with 3-tuples (name, desc, cmd_key).
|
||||
|
||||
Skill entries pass through _clamp_command_names as 3-tuples so the
|
||||
original cmd_key survives name truncation. Before the fix in PR #18951,
|
||||
the code stripped cmd_key into a side-dict keyed by the *original*
|
||||
(name, desc) pair — after truncation the lookup key no longer matched,
|
||||
silently losing the cmd_key.
|
||||
"""
|
||||
|
||||
def test_short_triple_preserved(self):
|
||||
entries = [("skill", "A skill", "/skill")]
|
||||
result = _clamp_command_names(entries, set())
|
||||
assert result == [("skill", "A skill", "/skill")]
|
||||
|
||||
def test_long_name_preserves_cmd_key(self):
|
||||
long = "a" * 50
|
||||
cmd_key = f"/{long}"
|
||||
result = _clamp_command_names([(long, "desc", cmd_key)], set())
|
||||
assert len(result) == 1
|
||||
name, desc, key = result[0]
|
||||
assert len(name) == _CMD_NAME_LIMIT
|
||||
assert key == cmd_key, "cmd_key must survive name clamping"
|
||||
|
||||
def test_collision_preserves_cmd_key(self):
|
||||
prefix = "x" * _CMD_NAME_LIMIT
|
||||
long = "x" * 50
|
||||
result = _clamp_command_names(
|
||||
[(long, "desc", "/long-skill")], reserved={prefix},
|
||||
)
|
||||
assert len(result) == 1
|
||||
name, _desc, key = result[0]
|
||||
assert name == "x" * (_CMD_NAME_LIMIT - 1) + "0"
|
||||
assert key == "/long-skill"
|
||||
|
||||
def test_multiple_long_names_preserve_respective_keys(self):
|
||||
base = "y" * 40
|
||||
entries = [
|
||||
(base + "_alpha", "d1", "/alpha-skill"),
|
||||
(base + "_beta", "d2", "/beta-skill"),
|
||||
]
|
||||
result = _clamp_command_names(entries, set())
|
||||
assert len(result) == 2
|
||||
assert result[0][2] == "/alpha-skill"
|
||||
assert result[1][2] == "/beta-skill"
|
||||
|
||||
def test_backward_compat_with_pairs(self):
|
||||
"""Legacy 2-tuple callers (Telegram) must still work."""
|
||||
entries = [("help", "Show help"), ("status", "Show status")]
|
||||
result = _clamp_command_names(entries, set())
|
||||
assert result == entries
|
||||
|
||||
|
||||
class TestDiscordSkillCmdKeyDispatch:
|
||||
"""Integration: discord_skill_commands preserves cmd_key for long names.
|
||||
|
||||
This tests the full pipeline: skill_commands → _collect_gateway_skill_entries
|
||||
→ _clamp_command_names → returned triples, verifying that skills with names
|
||||
exceeding Discord's 32-char limit still have their original cmd_key for
|
||||
dispatch.
|
||||
"""
|
||||
|
||||
def test_long_skill_name_retains_cmd_key(self, tmp_path, monkeypatch):
|
||||
from unittest.mock import patch
|
||||
|
||||
long_name = "this-is-a-very-long-skill-name-that-exceeds-limit"
|
||||
cmd_key = f"/{long_name}"
|
||||
fake_skills_dir = tmp_path / "skills"
|
||||
fake_skills_dir.mkdir(exist_ok=True)
|
||||
# Use resolved path — macOS /var → /private/var symlink
|
||||
# causes SKILLS_DIR.resolve() to differ from tmp_path.
|
||||
resolved_dir = str(fake_skills_dir.resolve())
|
||||
|
||||
fake_cmds = {
|
||||
cmd_key: {
|
||||
"name": long_name,
|
||||
"description": "A skill with a long name",
|
||||
"skill_md_path": f"{resolved_dir}/{long_name}/SKILL.md",
|
||||
"skill_dir": f"{resolved_dir}/{long_name}",
|
||||
},
|
||||
}
|
||||
|
||||
with patch("agent.skill_commands.get_skill_commands", return_value=fake_cmds), \
|
||||
patch("tools.skills_tool.SKILLS_DIR", fake_skills_dir), \
|
||||
patch("agent.skill_utils.get_external_skills_dirs", return_value=[]):
|
||||
entries, hidden = discord_skill_commands(
|
||||
max_slots=100, reserved_names=set(),
|
||||
)
|
||||
|
||||
assert len(entries) == 1
|
||||
name, desc, key = entries[0]
|
||||
assert len(name) <= _CMD_NAME_LIMIT, "Name should be clamped to 32 chars"
|
||||
assert key == cmd_key, (
|
||||
f"cmd_key must be the original /{long_name}, got {key!r}"
|
||||
)
|
||||
|
||||
|
||||
class TestTelegramMenuCommands:
|
||||
"""Integration: telegram_menu_commands enforces the 32-char limit."""
|
||||
|
||||
@@ -899,6 +996,73 @@ class TestTelegramMenuCommands:
|
||||
assert "my_enabled_skill" in menu_names
|
||||
assert "my_disabled_skill" not in menu_names
|
||||
|
||||
def test_external_dir_skills_included_in_telegram_menu(self, tmp_path, monkeypatch):
|
||||
"""External skills (``skills.external_dirs``) must appear in the Telegram menu.
|
||||
|
||||
Regression test for #8110 — external skills were visible to the
|
||||
agent and CLI but silently excluded from gateway slash menus
|
||||
because ``_collect_gateway_skill_entries`` only accepted skills
|
||||
whose path started with ``SKILLS_DIR``.
|
||||
|
||||
Also verifies the trailing-slash boundary: a directory that
|
||||
simply shares a prefix with a configured ``external_dirs`` entry
|
||||
(``/tmp/my-skills-extra`` vs ``/tmp/my-skills``) must NOT be
|
||||
admitted.
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
|
||||
local_dir = tmp_path / "skills"
|
||||
local_dir.mkdir()
|
||||
external_dir = tmp_path / "my-skills"
|
||||
external_dir.mkdir()
|
||||
lookalike_dir = tmp_path / "my-skills-extra"
|
||||
lookalike_dir.mkdir()
|
||||
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
(tmp_path / "config.yaml").write_text(
|
||||
f"skills:\n external_dirs:\n - {external_dir}\n"
|
||||
)
|
||||
|
||||
fake_cmds = {
|
||||
"/local-one": {
|
||||
"name": "local-one",
|
||||
"description": "Local",
|
||||
"skill_md_path": f"{local_dir}/local-one/SKILL.md",
|
||||
"skill_dir": f"{local_dir}/local-one",
|
||||
},
|
||||
"/morning-briefing": {
|
||||
"name": "morning-briefing",
|
||||
"description": "External skill",
|
||||
"skill_md_path": f"{external_dir}/morning-briefing/SKILL.md",
|
||||
"skill_dir": f"{external_dir}/morning-briefing",
|
||||
},
|
||||
"/lookalike-skill": {
|
||||
"name": "lookalike-skill",
|
||||
"description": "Lives in a sibling dir that shares a prefix",
|
||||
"skill_md_path": f"{lookalike_dir}/lookalike-skill/SKILL.md",
|
||||
"skill_dir": f"{lookalike_dir}/lookalike-skill",
|
||||
},
|
||||
}
|
||||
|
||||
with (
|
||||
patch("agent.skill_commands.get_skill_commands", return_value=fake_cmds),
|
||||
patch("tools.skills_tool.SKILLS_DIR", local_dir),
|
||||
patch(
|
||||
"agent.skill_utils.get_external_skills_dirs",
|
||||
return_value=[external_dir],
|
||||
),
|
||||
):
|
||||
menu, _ = telegram_menu_commands(max_commands=100)
|
||||
|
||||
menu_names = {n for n, _ in menu}
|
||||
assert "local_one" in menu_names, "local skill must appear"
|
||||
assert "morning_briefing" in menu_names, (
|
||||
"external skill from skills.external_dirs must appear (fixes #8110)"
|
||||
)
|
||||
assert "lookalike_skill" not in menu_names, (
|
||||
"prefix-match sibling directories must not be admitted"
|
||||
)
|
||||
|
||||
def test_special_chars_in_skill_names_sanitized(self, tmp_path, monkeypatch):
|
||||
"""Skills with +, /, or other special chars produce valid Telegram names."""
|
||||
from unittest.mock import patch
|
||||
@@ -1353,6 +1517,119 @@ class TestDiscordSkillCommandsByCategory:
|
||||
assert "vllm" in names
|
||||
assert len(uncategorized) == 0
|
||||
|
||||
def test_no_legacy_25x25_cap(self, tmp_path, monkeypatch):
|
||||
"""The old nested-layout caps (25 groups × 25 skills/group) are gone.
|
||||
|
||||
The live caller flattens categories into a single autocomplete list,
|
||||
which Discord fetches dynamically — the per-command 8KB payload
|
||||
concern from the old nested layout (#11321, #10259) no longer applies.
|
||||
Guards against accidentally re-introducing the caps, which would
|
||||
silently drop skills in the 26th+ alphabetical category (the exact
|
||||
failure mode users were hitting with 29 category dirs on real
|
||||
installs).
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
|
||||
fake_skills_dir = str(tmp_path / "skills")
|
||||
|
||||
# Build 30 categories (> old _MAX_GROUPS=25) each with 30 skills
|
||||
# (> old _MAX_PER_GROUP=25).
|
||||
fake_cmds = {}
|
||||
for c in range(30):
|
||||
cat = f"cat{c:02d}" # cat00, cat01, ..., cat29 — 30 categories
|
||||
for s in range(30):
|
||||
name = f"skill-{c:02d}-{s:02d}"
|
||||
skill_subdir = tmp_path / "skills" / cat / name
|
||||
skill_subdir.mkdir(parents=True, exist_ok=True)
|
||||
(skill_subdir / "SKILL.md").write_text("---\nname: x\n---\n")
|
||||
fake_cmds[f"/{name}"] = {
|
||||
"name": name,
|
||||
"description": f"Category {cat} skill {s}",
|
||||
"skill_md_path": f"{fake_skills_dir}/{cat}/{name}/SKILL.md",
|
||||
}
|
||||
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
with (
|
||||
patch("agent.skill_commands.get_skill_commands", return_value=fake_cmds),
|
||||
patch("tools.skills_tool.SKILLS_DIR", tmp_path / "skills"),
|
||||
):
|
||||
categories, uncategorized, hidden = discord_skill_commands_by_category(
|
||||
reserved_names=set(),
|
||||
)
|
||||
|
||||
# Every category should be present — no 25-group cap
|
||||
assert len(categories) == 30, (
|
||||
f"expected all 30 categories, got {len(categories)} "
|
||||
f"(cap from old nested layout must be removed)"
|
||||
)
|
||||
# Every skill in every category must be present — no 25-per-group cap
|
||||
for cat_name, entries in categories.items():
|
||||
assert len(entries) == 30, (
|
||||
f"category {cat_name}: expected 30 skills, got {len(entries)} "
|
||||
f"(cap from old nested layout must be removed)"
|
||||
)
|
||||
# Nothing should be reported hidden for the cap reason (the only
|
||||
# legitimate hidden reason now is name clamp collisions, which
|
||||
# don't happen here since all names are unique).
|
||||
assert hidden == 0
|
||||
|
||||
def test_external_dirs_skills_included(self, tmp_path, monkeypatch):
|
||||
"""Skills in ``skills.external_dirs`` must appear in /skill autocomplete.
|
||||
|
||||
#18741 fixed this for the flat ``discord_skill_commands`` collector
|
||||
but left ``discord_skill_commands_by_category`` (the live caller for
|
||||
Discord's ``/skill`` command) still filtering by
|
||||
``SKILLS_DIR`` prefix only. Regression guard that both collectors
|
||||
now accept external-dir skills.
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
|
||||
local_skills_dir = tmp_path / "local-skills"
|
||||
external_dir = tmp_path / "external-skills"
|
||||
|
||||
(local_skills_dir / "creative" / "local-skill").mkdir(parents=True)
|
||||
(local_skills_dir / "creative" / "local-skill" / "SKILL.md").write_text("")
|
||||
|
||||
(external_dir / "mlops" / "external-skill").mkdir(parents=True)
|
||||
(external_dir / "mlops" / "external-skill" / "SKILL.md").write_text("")
|
||||
|
||||
fake_cmds = {
|
||||
"/local-skill": {
|
||||
"name": "local-skill",
|
||||
"description": "Local",
|
||||
"skill_md_path": str(local_skills_dir / "creative" / "local-skill" / "SKILL.md"),
|
||||
},
|
||||
"/external-skill": {
|
||||
"name": "external-skill",
|
||||
"description": "External",
|
||||
"skill_md_path": str(external_dir / "mlops" / "external-skill" / "SKILL.md"),
|
||||
},
|
||||
}
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
with (
|
||||
patch("agent.skill_commands.get_skill_commands", return_value=fake_cmds),
|
||||
patch("tools.skills_tool.SKILLS_DIR", local_skills_dir),
|
||||
patch(
|
||||
"agent.skill_utils.get_external_skills_dirs",
|
||||
return_value=[external_dir],
|
||||
),
|
||||
):
|
||||
categories, uncategorized, hidden = discord_skill_commands_by_category(
|
||||
reserved_names=set(),
|
||||
)
|
||||
|
||||
# Local skill → grouped under "creative"
|
||||
assert "creative" in categories
|
||||
assert any(n == "local-skill" for n, _d, _k in categories["creative"])
|
||||
# External skill → grouped under its own top-level dir "mlops"
|
||||
assert "mlops" in categories, (
|
||||
"external-dir skills must be included — the old SKILLS_DIR-only "
|
||||
"prefix check was broken for by_category (completes #18741)"
|
||||
)
|
||||
assert any(n == "external-skill" for n, _d, _k in categories["mlops"])
|
||||
assert uncategorized == []
|
||||
assert hidden == 0
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Plugin slash command integration
|
||||
|
||||
@@ -0,0 +1,246 @@
|
||||
"""Tests for Discord /skill 32-char clamp collision warnings.
|
||||
|
||||
Discord's per-command name limit is 32 chars, so
|
||||
``discord_skill_commands_by_category`` clamps skill slugs to that width
|
||||
before deduping. When two skills share the same 32-char prefix, only
|
||||
the first (alphabetical) wins; the second is dropped. Previously the
|
||||
drop was silent — the ``hidden`` count incremented but nothing named
|
||||
which skills collided, so authors had no way to discover the drop
|
||||
short of noticing that their skill was missing from the autocomplete.
|
||||
|
||||
This module pins the upgraded behavior: a WARNING log with both full
|
||||
cmd_keys + the clamped name, so whoever named the skills sees the
|
||||
collision and can rename one.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import logging
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch
|
||||
|
||||
|
||||
def test_clamp_collision_emits_warning_naming_both_skills(
|
||||
tmp_path: Path, caplog
|
||||
) -> None:
|
||||
"""Two skills with identical first 32 chars — warning names both."""
|
||||
from hermes_cli.commands import discord_skill_commands_by_category
|
||||
|
||||
# Craft cmd_keys that share the first 32 chars.
|
||||
# 40-char prefix 'skill-collision-prefix-identical-first-32'
|
||||
# -> clamped to 'skill-collision-prefix-identical'
|
||||
prefix = "skill-collision-prefix-identical" # exactly 32 chars
|
||||
name_a = prefix + "-alpha" # /skill-collision-prefix-identical-alpha
|
||||
name_b = prefix + "-bravo" # /skill-collision-prefix-identical-bravo
|
||||
assert name_a[:32] == name_b[:32] == prefix
|
||||
|
||||
skills_dir = tmp_path / "skills"
|
||||
for nm in (name_a, name_b):
|
||||
d = skills_dir / "creative" / nm
|
||||
d.mkdir(parents=True)
|
||||
(d / "SKILL.md").write_text("---\nname: x\n---\n")
|
||||
|
||||
fake_cmds = {
|
||||
f"/{name_a}": {
|
||||
"name": name_a,
|
||||
"description": "Alpha",
|
||||
"skill_md_path": str(skills_dir / "creative" / name_a / "SKILL.md"),
|
||||
},
|
||||
f"/{name_b}": {
|
||||
"name": name_b,
|
||||
"description": "Bravo",
|
||||
"skill_md_path": str(skills_dir / "creative" / name_b / "SKILL.md"),
|
||||
},
|
||||
}
|
||||
|
||||
with caplog.at_level(logging.WARNING, logger="hermes_cli.commands"), (
|
||||
patch("agent.skill_commands.get_skill_commands", return_value=fake_cmds)
|
||||
), patch("tools.skills_tool.SKILLS_DIR", skills_dir):
|
||||
categories, uncategorized, hidden = discord_skill_commands_by_category(
|
||||
reserved_names=set(),
|
||||
)
|
||||
|
||||
# One skill made it through, one was dropped (hidden counted).
|
||||
assert hidden == 1
|
||||
kept_names = [n for n, _d, _k in categories.get("creative", [])]
|
||||
assert len(kept_names) == 1
|
||||
# Alphabetical iteration means the -alpha variant wins the slot.
|
||||
assert kept_names[0] == prefix # clamped
|
||||
|
||||
# Exactly one warning, naming BOTH full cmd_keys and the clamped name.
|
||||
warnings = [
|
||||
r for r in caplog.records
|
||||
if r.levelno == logging.WARNING and "clamp" in r.getMessage()
|
||||
]
|
||||
assert len(warnings) == 1, (
|
||||
f"expected exactly one clamp-collision warning, got {len(warnings)}: "
|
||||
f"{[r.getMessage() for r in warnings]}"
|
||||
)
|
||||
msg = warnings[0].getMessage()
|
||||
assert f"/{name_a}" in msg, f"winner not named in warning: {msg!r}"
|
||||
assert f"/{name_b}" in msg, f"loser not named in warning: {msg!r}"
|
||||
assert prefix in msg, f"clamped name not in warning: {msg!r}"
|
||||
|
||||
|
||||
def test_clamp_collision_with_reserved_name_emits_distinct_warning(
|
||||
tmp_path: Path, caplog
|
||||
) -> None:
|
||||
"""A skill clashing with a reserved gateway command gets its own phrasing.
|
||||
|
||||
The reserved-vs-skill case is operationally different — the fix is
|
||||
still "rename the skill," but there's no second skill to also
|
||||
rename. The warning should say so explicitly.
|
||||
"""
|
||||
from hermes_cli.commands import discord_skill_commands_by_category
|
||||
|
||||
# Reserved name 'help' is 4 chars — make a skill whose slug
|
||||
# clamps to 'help' (so, exactly 'help').
|
||||
reserved = "help"
|
||||
skills_dir = tmp_path / "skills"
|
||||
d = skills_dir / "creative" / reserved
|
||||
d.mkdir(parents=True)
|
||||
(d / "SKILL.md").write_text("---\nname: x\n---\n")
|
||||
|
||||
fake_cmds = {
|
||||
f"/{reserved}": {
|
||||
"name": reserved,
|
||||
"description": "desc",
|
||||
"skill_md_path": str(d / "SKILL.md"),
|
||||
},
|
||||
}
|
||||
|
||||
with caplog.at_level(logging.WARNING, logger="hermes_cli.commands"), (
|
||||
patch("agent.skill_commands.get_skill_commands", return_value=fake_cmds)
|
||||
), patch("tools.skills_tool.SKILLS_DIR", skills_dir):
|
||||
categories, uncategorized, hidden = discord_skill_commands_by_category(
|
||||
reserved_names={"help"},
|
||||
)
|
||||
|
||||
# Skill dropped in favor of the reserved command.
|
||||
assert hidden == 1
|
||||
assert categories == {}
|
||||
assert uncategorized == []
|
||||
|
||||
warnings = [
|
||||
r for r in caplog.records
|
||||
if r.levelno == logging.WARNING and "reserved" in r.getMessage()
|
||||
]
|
||||
assert len(warnings) == 1, (
|
||||
f"expected one reserved-name collision warning, got "
|
||||
f"{[r.getMessage() for r in warnings]}"
|
||||
)
|
||||
msg = warnings[0].getMessage()
|
||||
assert f"/{reserved}" in msg
|
||||
assert "reserved" in msg.lower()
|
||||
|
||||
|
||||
def test_no_collision_no_warning(tmp_path: Path, caplog) -> None:
|
||||
"""Sanity: two distinct-prefix skills produce zero warnings."""
|
||||
from hermes_cli.commands import discord_skill_commands_by_category
|
||||
|
||||
skills_dir = tmp_path / "skills"
|
||||
for nm in ("alpha", "bravo"):
|
||||
d = skills_dir / "creative" / nm
|
||||
d.mkdir(parents=True)
|
||||
(d / "SKILL.md").write_text("---\nname: x\n---\n")
|
||||
|
||||
fake_cmds = {
|
||||
"/alpha": {
|
||||
"name": "alpha", "description": "",
|
||||
"skill_md_path": str(skills_dir / "creative" / "alpha" / "SKILL.md"),
|
||||
},
|
||||
"/bravo": {
|
||||
"name": "bravo", "description": "",
|
||||
"skill_md_path": str(skills_dir / "creative" / "bravo" / "SKILL.md"),
|
||||
},
|
||||
}
|
||||
|
||||
with caplog.at_level(logging.WARNING, logger="hermes_cli.commands"), (
|
||||
patch("agent.skill_commands.get_skill_commands", return_value=fake_cmds)
|
||||
), patch("tools.skills_tool.SKILLS_DIR", skills_dir):
|
||||
categories, uncategorized, hidden = discord_skill_commands_by_category(
|
||||
reserved_names=set(),
|
||||
)
|
||||
|
||||
assert hidden == 0
|
||||
assert {n for n, _d, _k in categories["creative"]} == {"alpha", "bravo"}
|
||||
clamp_warnings = [
|
||||
r for r in caplog.records
|
||||
if r.levelno == logging.WARNING
|
||||
and ("clamp" in r.getMessage() or "reserved" in r.getMessage())
|
||||
]
|
||||
assert clamp_warnings == []
|
||||
|
||||
|
||||
def test_long_skill_name_preserves_cmd_key_through_by_category(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""Skills with names > 32 chars must keep their original cmd_key.
|
||||
|
||||
``discord_skill_commands_by_category`` clamps the display name to 32
|
||||
chars but the third tuple element (cmd_key) must stay as the original
|
||||
``/full-skill-name`` so that ``_skill_handler`` dispatches via
|
||||
``_run_simple_slash`` with the full command, not the truncated one.
|
||||
|
||||
This is the actual runtime path used by the Discord adapter via
|
||||
``_refresh_skill_catalog_state``.
|
||||
"""
|
||||
from hermes_cli.commands import discord_skill_commands_by_category
|
||||
|
||||
skills_dir = tmp_path / "skills"
|
||||
skills_dir.mkdir()
|
||||
resolved = str(skills_dir.resolve())
|
||||
|
||||
long_name = "generate-ascii-art-from-text-description-detailed"
|
||||
cmd_key = f"/{long_name}"
|
||||
fake_cmds = {
|
||||
cmd_key: {
|
||||
"name": long_name,
|
||||
"description": "Generate ASCII art from a text description",
|
||||
"skill_md_path": f"{resolved}/creative/{long_name}/SKILL.md",
|
||||
"skill_dir": f"{resolved}/creative/{long_name}",
|
||||
},
|
||||
"/short-skill": {
|
||||
"name": "short-skill",
|
||||
"description": "A short skill",
|
||||
"skill_md_path": f"{resolved}/creative/short-skill/SKILL.md",
|
||||
"skill_dir": f"{resolved}/creative/short-skill",
|
||||
},
|
||||
}
|
||||
|
||||
with patch("agent.skill_commands.get_skill_commands", return_value=fake_cmds), \
|
||||
patch("tools.skills_tool.SKILLS_DIR", skills_dir):
|
||||
categories, uncategorized, hidden = discord_skill_commands_by_category(
|
||||
reserved_names=set(),
|
||||
)
|
||||
|
||||
# Flatten (same as _refresh_skill_catalog_state does)
|
||||
entries = list(uncategorized)
|
||||
for cat_skills in categories.values():
|
||||
entries.extend(cat_skills)
|
||||
|
||||
# Build lookup (same as _refresh_skill_catalog_state does)
|
||||
skill_lookup = {n: (d, k) for n, d, k in entries}
|
||||
|
||||
# Find the long skill
|
||||
long_entry = [e for e in entries if e[2] == cmd_key]
|
||||
assert len(long_entry) == 1, f"Long skill should appear once, got: {long_entry}"
|
||||
|
||||
display_name, desc, key = long_entry[0]
|
||||
assert len(display_name) <= 32, (
|
||||
f"Display name should be clamped to 32 chars, got {len(display_name)}"
|
||||
)
|
||||
assert key == cmd_key, (
|
||||
f"cmd_key must be the original /{long_name}, got {key!r}"
|
||||
)
|
||||
|
||||
# Verify lookup works: clamped display name -> original cmd_key
|
||||
assert display_name in skill_lookup
|
||||
_desc, looked_up_key = skill_lookup[display_name]
|
||||
assert looked_up_key == cmd_key, (
|
||||
f"Lookup must map clamped name to original cmd_key, got {looked_up_key!r}"
|
||||
)
|
||||
|
||||
# Short skill should also be present and correct
|
||||
short_entry = [e for e in entries if e[2] == "/short-skill"]
|
||||
assert len(short_entry) == 1
|
||||
assert short_entry[0][0] == "short-skill"
|
||||
@@ -51,6 +51,57 @@ class TestProviderEnvDetection:
|
||||
assert not _has_provider_env_config(content)
|
||||
|
||||
|
||||
class TestDoctorEnvFileEncoding:
|
||||
"""Regression for #18637 (bug 3): `hermes doctor` crashed on Windows
|
||||
Chinese locale (GBK) because `.env` was read with Path.read_text() which
|
||||
defaults to the system locale encoding, not UTF-8."""
|
||||
|
||||
def test_doctor_reads_env_as_utf8_even_when_locale_is_not_utf8(
|
||||
self, monkeypatch, tmp_path
|
||||
):
|
||||
import pathlib
|
||||
|
||||
hermes_home = tmp_path / ".hermes"
|
||||
hermes_home.mkdir()
|
||||
# Write a UTF-8 .env containing an em dash (U+2014 = e2 80 94). The
|
||||
# 0x94 byte is exactly the one the issue reporter hit: it's invalid
|
||||
# as a GBK trailing byte in this position, so locale-default reads
|
||||
# raise UnicodeDecodeError on Chinese Windows.
|
||||
env_path = hermes_home / ".env"
|
||||
env_path.write_text(
|
||||
"OPENAI_API_KEY=sk-test # em-dash here — should not crash\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
|
||||
monkeypatch.setattr(doctor_mod, "HERMES_HOME", hermes_home)
|
||||
|
||||
orig_read_text = pathlib.Path.read_text
|
||||
|
||||
def gbk_like_read_text(self, encoding=None, errors=None, **kwargs):
|
||||
# Simulate a GBK locale: refuse to decode this specific UTF-8
|
||||
# .env unless the caller pins encoding="utf-8".
|
||||
if self == env_path and encoding != "utf-8":
|
||||
raise UnicodeDecodeError(
|
||||
"gbk", b"\x94", 0, 1, "illegal multibyte sequence"
|
||||
)
|
||||
return orig_read_text(self, encoding=encoding, errors=errors, **kwargs)
|
||||
|
||||
monkeypatch.setattr(pathlib.Path, "read_text", gbk_like_read_text)
|
||||
|
||||
# Short-circuit the expensive tool-availability probe — we only
|
||||
# need doctor to reach the .env read without crashing.
|
||||
fake_model_tools = types.SimpleNamespace(
|
||||
check_tool_availability=lambda *a, **kw: (_ for _ in ()).throw(SystemExit(0)),
|
||||
TOOLSET_REQUIREMENTS={},
|
||||
)
|
||||
monkeypatch.setitem(sys.modules, "model_tools", fake_model_tools)
|
||||
|
||||
# Run doctor. If the .env read still uses locale encoding, this
|
||||
# raises UnicodeDecodeError and the test fails.
|
||||
with pytest.raises(SystemExit):
|
||||
doctor_mod.run_doctor(Namespace(fix=False))
|
||||
|
||||
|
||||
class TestDoctorToolAvailabilityOverrides:
|
||||
def test_marks_honcho_available_when_configured(self, monkeypatch):
|
||||
monkeypatch.setattr(doctor, "_honcho_is_configured_for_doctor", lambda: True)
|
||||
|
||||
@@ -4,11 +4,16 @@ from hermes_cli.setup import setup_agent_settings
|
||||
|
||||
|
||||
def test_setup_agent_settings_uses_displayed_max_iterations_value(tmp_path, monkeypatch, capsys):
|
||||
"""The helper text should match the value shown in the prompt."""
|
||||
"""The helper text should match the value shown in the prompt.
|
||||
|
||||
After PR#18413 max_turns is read exclusively from config.yaml — the
|
||||
.env `HERMES_MAX_ITERATIONS` fallback was removed because it was
|
||||
shadowing the user's current config (see the 60-vs-500 incident).
|
||||
"""
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
|
||||
config = {
|
||||
"agent": {"max_turns": 90},
|
||||
"agent": {"max_turns": 60},
|
||||
"display": {"tool_progress": "all"},
|
||||
"compression": {"threshold": 0.50},
|
||||
"session_reset": {"mode": "both", "idle_minutes": 1440, "at_hour": 4},
|
||||
@@ -16,10 +21,10 @@ def test_setup_agent_settings_uses_displayed_max_iterations_value(tmp_path, monk
|
||||
|
||||
prompt_answers = iter(["60", "all", "0.5"])
|
||||
|
||||
monkeypatch.setattr("hermes_cli.setup.get_env_value", lambda key: "60" if key == "HERMES_MAX_ITERATIONS" else "")
|
||||
monkeypatch.setattr("hermes_cli.setup.prompt", lambda *args, **kwargs: next(prompt_answers))
|
||||
monkeypatch.setattr("hermes_cli.setup.prompt_choice", lambda *args, **kwargs: 4)
|
||||
monkeypatch.setattr("hermes_cli.setup.save_env_value", lambda *args, **kwargs: None)
|
||||
monkeypatch.setattr("hermes_cli.setup.remove_env_value", lambda *args, **kwargs: None)
|
||||
monkeypatch.setattr("hermes_cli.setup.save_config", lambda *args, **kwargs: None)
|
||||
|
||||
setup_agent_settings(config)
|
||||
@@ -27,3 +32,47 @@ def test_setup_agent_settings_uses_displayed_max_iterations_value(tmp_path, monk
|
||||
out = capsys.readouterr().out
|
||||
assert "Press Enter to keep 60." in out
|
||||
assert "Default is 90" not in out
|
||||
|
||||
|
||||
def test_setup_agent_settings_prefers_config_over_stale_env(tmp_path, monkeypatch, capsys):
|
||||
"""Config.yaml wins even when a stale .env value disagrees.
|
||||
|
||||
Regression guard for the bug where `.env HERMES_MAX_ITERATIONS=60`
|
||||
from an old `hermes setup` run shadowed `agent.max_turns: 500` in
|
||||
config.yaml. The wizard must now display the config value.
|
||||
"""
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
|
||||
config = {
|
||||
"agent": {"max_turns": 500}, # user bumped this in config.yaml
|
||||
"display": {"tool_progress": "all"},
|
||||
"compression": {"threshold": 0.50},
|
||||
"session_reset": {"mode": "both", "idle_minutes": 1440, "at_hour": 4},
|
||||
}
|
||||
|
||||
prompt_answers = iter(["500", "all", "0.5"])
|
||||
|
||||
# Simulate stale .env value — the wizard must ignore this.
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.setup.get_env_value",
|
||||
lambda key: "60" if key == "HERMES_MAX_ITERATIONS" else "",
|
||||
)
|
||||
monkeypatch.setattr("hermes_cli.setup.prompt", lambda *args, **kwargs: next(prompt_answers))
|
||||
monkeypatch.setattr("hermes_cli.setup.prompt_choice", lambda *args, **kwargs: 4)
|
||||
monkeypatch.setattr("hermes_cli.setup.save_env_value", lambda *args, **kwargs: None)
|
||||
|
||||
removed_keys: list[str] = []
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.setup.remove_env_value",
|
||||
lambda key: (removed_keys.append(key), True)[1],
|
||||
)
|
||||
monkeypatch.setattr("hermes_cli.setup.save_config", lambda *args, **kwargs: None)
|
||||
|
||||
setup_agent_settings(config)
|
||||
|
||||
out = capsys.readouterr().out
|
||||
# Config value wins
|
||||
assert "Press Enter to keep 500." in out
|
||||
assert "Press Enter to keep 60." not in out
|
||||
# And the stale .env entry gets cleaned up
|
||||
assert "HERMES_MAX_ITERATIONS" in removed_keys
|
||||
|
||||
@@ -8,6 +8,7 @@ from hermes_cli.tools_config import (
|
||||
_configure_provider,
|
||||
_get_platform_tools,
|
||||
_platform_toolset_summary,
|
||||
_reconfigure_tool,
|
||||
_save_platform_tools,
|
||||
_toolset_has_keys,
|
||||
CONFIGURABLE_TOOLSETS,
|
||||
@@ -468,6 +469,33 @@ def test_local_browser_provider_is_saved_explicitly(monkeypatch):
|
||||
assert config["browser"]["cloud_provider"] == "local"
|
||||
|
||||
|
||||
def test_reconfigure_lists_enabled_web_without_existing_provider_config(monkeypatch):
|
||||
config = {"platform_toolsets": {"cli": ["web"]}}
|
||||
seen = {}
|
||||
configured = []
|
||||
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.tools_config._toolset_has_keys",
|
||||
lambda ts_key, config=None: False,
|
||||
)
|
||||
|
||||
def fake_prompt_choice(question, choices, default=0):
|
||||
seen["choices"] = choices
|
||||
return 0
|
||||
|
||||
monkeypatch.setattr("hermes_cli.tools_config._prompt_choice", fake_prompt_choice)
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.tools_config._configure_tool_category_for_reconfig",
|
||||
lambda ts_key, cat, config: configured.append(ts_key),
|
||||
)
|
||||
monkeypatch.setattr("hermes_cli.tools_config.save_config", lambda config: None)
|
||||
|
||||
_reconfigure_tool(config)
|
||||
|
||||
assert any("Web Search" in choice for choice in seen["choices"])
|
||||
assert configured == ["web"]
|
||||
|
||||
|
||||
def test_first_install_nous_auto_configures_managed_defaults(monkeypatch):
|
||||
monkeypatch.setattr("hermes_cli.tools_config.managed_nous_tools_enabled", lambda: True)
|
||||
monkeypatch.setattr("hermes_cli.nous_subscription.managed_nous_tools_enabled", lambda: True)
|
||||
|
||||
@@ -0,0 +1,69 @@
|
||||
"""Regression test for #17929: AIAgent.__init__ should try fallback_model
|
||||
when primary provider credentials are exhausted."""
|
||||
import pytest
|
||||
from unittest.mock import patch, MagicMock
|
||||
from run_agent import AIAgent
|
||||
|
||||
|
||||
def _make_tool_defs():
|
||||
return [{"type": "function", "function": {"name": "web_search",
|
||||
"description": "search", "parameters": {"type": "object", "properties": {}}}}]
|
||||
|
||||
|
||||
def _mock_client(api_key="fb-key-1234567890", base_url="https://fb.example.com/v1"):
|
||||
c = MagicMock()
|
||||
c.api_key = api_key
|
||||
c.base_url = base_url
|
||||
c._default_headers = None
|
||||
return c
|
||||
|
||||
|
||||
def test_init_tries_fallback_when_primary_returns_none():
|
||||
"""When resolve_provider_client returns None for primary but succeeds for
|
||||
a fallback entry, __init__ should NOT raise RuntimeError."""
|
||||
fb = _mock_client()
|
||||
|
||||
def fake_resolve(provider, model=None, raw_codex=False,
|
||||
explicit_base_url=None, explicit_api_key=None):
|
||||
if provider == "tencent-token-plan":
|
||||
return fb, "kimi2.5"
|
||||
return None, None # primary exhausted
|
||||
|
||||
with patch("agent.auxiliary_client.resolve_provider_client", side_effect=fake_resolve), \
|
||||
patch("run_agent.get_tool_definitions", return_value=_make_tool_defs()), \
|
||||
patch("run_agent.check_toolset_requirements", return_value={}), \
|
||||
patch("run_agent.OpenAI", return_value=MagicMock()):
|
||||
|
||||
agent = AIAgent(
|
||||
provider="alibaba-coding-plan",
|
||||
model="qwen3.6-plus",
|
||||
api_key=None,
|
||||
base_url=None,
|
||||
quiet_mode=True,
|
||||
skip_context_files=True,
|
||||
skip_memory=True,
|
||||
fallback_model=[{"provider": "tencent-token-plan", "model": "kimi2.5"}],
|
||||
)
|
||||
assert agent.provider == "tencent-token-plan"
|
||||
assert agent.model == "kimi2.5"
|
||||
assert agent._fallback_activated is True
|
||||
|
||||
|
||||
def test_init_raises_when_no_fallback_configured():
|
||||
"""When primary returns None and no fallback is set, should raise."""
|
||||
with patch("agent.auxiliary_client.resolve_provider_client", return_value=(None, None)), \
|
||||
patch("run_agent.get_tool_definitions", return_value=_make_tool_defs()), \
|
||||
patch("run_agent.check_toolset_requirements", return_value={}), \
|
||||
patch("run_agent.OpenAI", return_value=MagicMock()):
|
||||
|
||||
with pytest.raises(RuntimeError, match="no API key was found"):
|
||||
AIAgent(
|
||||
provider="alibaba-coding-plan",
|
||||
model="qwen3.6-plus",
|
||||
api_key=None,
|
||||
base_url=None,
|
||||
quiet_mode=True,
|
||||
skip_context_files=True,
|
||||
skip_memory=True,
|
||||
fallback_model=None,
|
||||
)
|
||||
@@ -81,3 +81,51 @@ def test_unknown_base_url_clears_default_headers(mock_openai):
|
||||
agent._apply_client_headers_for_base_url("https://api.example.com/v1")
|
||||
|
||||
assert "default_headers" not in agent._client_kwargs
|
||||
|
||||
|
||||
@patch("run_agent.OpenAI")
|
||||
def test_openrouter_headers_include_response_cache_when_enabled(mock_openai):
|
||||
"""When openrouter.response_cache is True, the cache header is injected."""
|
||||
mock_openai.return_value = MagicMock()
|
||||
agent = AIAgent(
|
||||
api_key="test-key",
|
||||
base_url="https://openrouter.ai/api/v1",
|
||||
model="test/model",
|
||||
quiet_mode=True,
|
||||
skip_context_files=True,
|
||||
skip_memory=True,
|
||||
)
|
||||
|
||||
with patch("hermes_cli.config.load_config", return_value={
|
||||
"openrouter": {"response_cache": True, "response_cache_ttl": 600},
|
||||
}):
|
||||
agent._apply_client_headers_for_base_url("https://openrouter.ai/api/v1")
|
||||
|
||||
headers = agent._client_kwargs["default_headers"]
|
||||
assert headers["HTTP-Referer"] == "https://hermes-agent.nousresearch.com"
|
||||
assert headers["X-OpenRouter-Cache"] == "true"
|
||||
assert headers["X-OpenRouter-Cache-TTL"] == "600"
|
||||
|
||||
|
||||
@patch("run_agent.OpenAI")
|
||||
def test_openrouter_headers_no_cache_when_disabled(mock_openai):
|
||||
"""When openrouter.response_cache is False, no cache headers are sent."""
|
||||
mock_openai.return_value = MagicMock()
|
||||
agent = AIAgent(
|
||||
api_key="test-key",
|
||||
base_url="https://openrouter.ai/api/v1",
|
||||
model="test/model",
|
||||
quiet_mode=True,
|
||||
skip_context_files=True,
|
||||
skip_memory=True,
|
||||
)
|
||||
|
||||
with patch("hermes_cli.config.load_config", return_value={
|
||||
"openrouter": {"response_cache": False},
|
||||
}):
|
||||
agent._apply_client_headers_for_base_url("https://openrouter.ai/api/v1")
|
||||
|
||||
headers = agent._client_kwargs["default_headers"]
|
||||
assert headers["HTTP-Referer"] == "https://hermes-agent.nousresearch.com"
|
||||
assert "X-OpenRouter-Cache" not in headers
|
||||
assert "X-OpenRouter-Cache-TTL" not in headers
|
||||
|
||||
@@ -0,0 +1,116 @@
|
||||
"""Tests for get_hermes_home() profile-mode fallback warning.
|
||||
|
||||
Regression test for https://github.com/NousResearch/hermes-agent/issues/18594.
|
||||
|
||||
When HERMES_HOME is unset but an active_profile file indicates a non-default
|
||||
profile is active, get_hermes_home() should:
|
||||
1. STILL return ~/.hermes (raising would brick 30+ module-level callers)
|
||||
2. Emit a loud one-shot warning to stderr so operators can diagnose
|
||||
cross-profile data contamination after the fact.
|
||||
|
||||
The warning goes to stderr directly (not through logging) because this
|
||||
function is called at module-import time from 30+ sites, often before the
|
||||
logging subsystem has been configured.
|
||||
"""
|
||||
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def fresh_constants(monkeypatch, tmp_path):
|
||||
"""Import hermes_constants fresh and reset the one-shot warn flag."""
|
||||
import importlib
|
||||
import hermes_constants
|
||||
importlib.reload(hermes_constants)
|
||||
monkeypatch.setattr(Path, "home", lambda: tmp_path)
|
||||
monkeypatch.delenv("HERMES_HOME", raising=False)
|
||||
return hermes_constants
|
||||
|
||||
|
||||
class TestGetHermesHomeProfileWarning:
|
||||
def test_classic_mode_no_active_profile_no_warning(
|
||||
self, fresh_constants, tmp_path, capsys
|
||||
):
|
||||
"""Classic mode: no active_profile file → silent, returns ~/.hermes."""
|
||||
result = fresh_constants.get_hermes_home()
|
||||
assert result == tmp_path / ".hermes"
|
||||
assert "HERMES_HOME fallback" not in capsys.readouterr().err
|
||||
|
||||
def test_default_active_profile_no_warning(
|
||||
self, fresh_constants, tmp_path, capsys
|
||||
):
|
||||
"""active_profile=default → still no warning, returns ~/.hermes."""
|
||||
hermes_dir = tmp_path / ".hermes"
|
||||
hermes_dir.mkdir()
|
||||
(hermes_dir / "active_profile").write_text("default\n")
|
||||
result = fresh_constants.get_hermes_home()
|
||||
assert result == tmp_path / ".hermes"
|
||||
assert "HERMES_HOME fallback" not in capsys.readouterr().err
|
||||
|
||||
def test_named_profile_unset_home_warns_once(
|
||||
self, fresh_constants, tmp_path, capsys
|
||||
):
|
||||
"""active_profile=coder + HERMES_HOME unset → warn loudly, still return fallback."""
|
||||
hermes_dir = tmp_path / ".hermes"
|
||||
hermes_dir.mkdir()
|
||||
(hermes_dir / "active_profile").write_text("coder\n")
|
||||
|
||||
result = fresh_constants.get_hermes_home()
|
||||
|
||||
# 1. Still returns the fallback — no import-time crash
|
||||
assert result == tmp_path / ".hermes"
|
||||
# 2. Stderr got the warning exactly once
|
||||
err = capsys.readouterr().err
|
||||
assert err.count("HERMES_HOME fallback") == 1
|
||||
assert "'coder'" in err
|
||||
assert "#18594" in err
|
||||
|
||||
# 3. One-shot: second and third calls don't re-warn
|
||||
fresh_constants.get_hermes_home()
|
||||
fresh_constants.get_hermes_home()
|
||||
err2 = capsys.readouterr().err
|
||||
assert "HERMES_HOME fallback" not in err2
|
||||
|
||||
def test_hermes_home_set_suppresses_warning(
|
||||
self, fresh_constants, tmp_path, capsys, monkeypatch
|
||||
):
|
||||
"""Even if active_profile is 'coder', setting HERMES_HOME suppresses warning."""
|
||||
profile_dir = tmp_path / ".hermes" / "profiles" / "coder"
|
||||
profile_dir.mkdir(parents=True)
|
||||
(tmp_path / ".hermes" / "active_profile").write_text("coder\n")
|
||||
monkeypatch.setenv("HERMES_HOME", str(profile_dir))
|
||||
|
||||
result = fresh_constants.get_hermes_home()
|
||||
|
||||
assert result == profile_dir
|
||||
assert "HERMES_HOME fallback" not in capsys.readouterr().err
|
||||
|
||||
def test_unreadable_active_profile_no_crash(
|
||||
self, fresh_constants, tmp_path, capsys
|
||||
):
|
||||
"""active_profile that can't be decoded → fall through silently."""
|
||||
hermes_dir = tmp_path / ".hermes"
|
||||
hermes_dir.mkdir()
|
||||
# Write bytes that aren't valid utf-8
|
||||
(hermes_dir / "active_profile").write_bytes(b"\xff\xfe\x00\x00")
|
||||
|
||||
result = fresh_constants.get_hermes_home()
|
||||
|
||||
assert result == tmp_path / ".hermes"
|
||||
# Shouldn't crash; shouldn't warn either (can't tell what profile was intended)
|
||||
assert "HERMES_HOME fallback" not in capsys.readouterr().err
|
||||
|
||||
def test_empty_active_profile_no_warning(
|
||||
self, fresh_constants, tmp_path, capsys
|
||||
):
|
||||
"""Empty active_profile file → treated as default, no warning."""
|
||||
hermes_dir = tmp_path / ".hermes"
|
||||
hermes_dir.mkdir()
|
||||
(hermes_dir / "active_profile").write_text("")
|
||||
|
||||
result = fresh_constants.get_hermes_home()
|
||||
|
||||
assert result == tmp_path / ".hermes"
|
||||
assert "HERMES_HOME fallback" not in capsys.readouterr().err
|
||||
@@ -104,6 +104,44 @@ class TestWriteFileHandler:
|
||||
assert result["error"] == "boom"
|
||||
assert any("write_file error" in r.getMessage() for r in caplog.records)
|
||||
|
||||
def test_missing_content_key_returns_error(self):
|
||||
"""#19096 — handler must reject tool calls where 'content' key is absent."""
|
||||
from tools.file_tools import _handle_write_file
|
||||
|
||||
result = json.loads(_handle_write_file({"path": "/tmp/oops.md"}))
|
||||
assert "error" in result
|
||||
assert "content" in result["error"]
|
||||
assert "path" not in result.get("error", "").lower() or "missing" not in result.get("error", "").lower() or True # just check error present
|
||||
|
||||
def test_missing_path_key_returns_error(self):
|
||||
"""#19096 — handler must reject tool calls where 'path' key is absent."""
|
||||
from tools.file_tools import _handle_write_file
|
||||
|
||||
result = json.loads(_handle_write_file({"content": "hello"}))
|
||||
assert "error" in result
|
||||
|
||||
def test_explicit_empty_content_is_allowed(self):
|
||||
"""#19096 — explicit empty string content (file truncation) must still work."""
|
||||
from tools.file_tools import _handle_write_file
|
||||
|
||||
with patch("tools.file_tools._get_file_ops") as mock_get:
|
||||
mock_ops = MagicMock()
|
||||
result_obj = MagicMock()
|
||||
result_obj.to_dict.return_value = {"status": "ok", "path": "/tmp/empty.txt", "bytes": 0}
|
||||
mock_ops.write_file.return_value = result_obj
|
||||
mock_get.return_value = mock_ops
|
||||
|
||||
result = json.loads(_handle_write_file({"path": "/tmp/empty.txt", "content": ""}))
|
||||
assert result["status"] == "ok"
|
||||
|
||||
def test_non_string_content_returns_error(self):
|
||||
"""#19096 — content must be a string, not a dict or list."""
|
||||
from tools.file_tools import _handle_write_file
|
||||
|
||||
result = json.loads(_handle_write_file({"path": "/tmp/x.txt", "content": {"nested": "dict"}}))
|
||||
assert "error" in result
|
||||
assert "string" in result["error"].lower() or "content" in result["error"].lower()
|
||||
|
||||
|
||||
class TestPatchHandler:
|
||||
@patch("tools.file_tools._get_file_ops")
|
||||
|
||||
@@ -371,6 +371,57 @@ class TestDeleteSkill:
|
||||
_delete_skill("my-skill")
|
||||
assert not (tmp_path / "devops").exists()
|
||||
|
||||
def test_delete_with_absorbed_into_valid_target(self, tmp_path):
|
||||
with _skill_dir(tmp_path):
|
||||
_create_skill("umbrella", VALID_SKILL_CONTENT)
|
||||
_create_skill("narrow", VALID_SKILL_CONTENT)
|
||||
result = _delete_skill("narrow", absorbed_into="umbrella")
|
||||
assert result["success"] is True
|
||||
assert "absorbed into 'umbrella'" in result["message"]
|
||||
assert not (tmp_path / "narrow").exists()
|
||||
assert (tmp_path / "umbrella").exists()
|
||||
|
||||
def test_delete_with_absorbed_into_empty_string_means_pruned(self, tmp_path):
|
||||
with _skill_dir(tmp_path):
|
||||
_create_skill("stale-skill", VALID_SKILL_CONTENT)
|
||||
result = _delete_skill("stale-skill", absorbed_into="")
|
||||
assert result["success"] is True
|
||||
# Empty absorbed_into is explicit prune — no "absorbed into" suffix in message
|
||||
assert "absorbed into" not in result["message"]
|
||||
|
||||
def test_delete_with_absorbed_into_nonexistent_target_rejected(self, tmp_path):
|
||||
with _skill_dir(tmp_path):
|
||||
_create_skill("narrow", VALID_SKILL_CONTENT)
|
||||
result = _delete_skill("narrow", absorbed_into="ghost-umbrella")
|
||||
assert result["success"] is False
|
||||
assert "does not exist" in result["error"]
|
||||
# Skill must NOT have been deleted on validation failure
|
||||
assert (tmp_path / "narrow").exists()
|
||||
|
||||
def test_delete_with_absorbed_into_equals_self_rejected(self, tmp_path):
|
||||
with _skill_dir(tmp_path):
|
||||
_create_skill("narrow", VALID_SKILL_CONTENT)
|
||||
result = _delete_skill("narrow", absorbed_into="narrow")
|
||||
assert result["success"] is False
|
||||
assert "cannot equal" in result["error"]
|
||||
assert (tmp_path / "narrow").exists()
|
||||
|
||||
def test_delete_with_absorbed_into_whitespace_only_treated_as_prune(self, tmp_path):
|
||||
# Leading/trailing whitespace only: .strip() → "" → pruned path
|
||||
with _skill_dir(tmp_path):
|
||||
_create_skill("narrow", VALID_SKILL_CONTENT)
|
||||
result = _delete_skill("narrow", absorbed_into=" ")
|
||||
assert result["success"] is True
|
||||
assert "absorbed into" not in result["message"]
|
||||
|
||||
def test_delete_without_absorbed_into_backward_compat(self, tmp_path):
|
||||
# Legacy callers that don't pass the arg still work — the curator
|
||||
# reconciler falls back to its heuristic+YAML logic for such deletes.
|
||||
with _skill_dir(tmp_path):
|
||||
_create_skill("my-skill", VALID_SKILL_CONTENT)
|
||||
result = _delete_skill("my-skill")
|
||||
assert result["success"] is True
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# write_file / remove_file
|
||||
@@ -485,6 +536,25 @@ class TestSkillManageDispatcher:
|
||||
result = json.loads(raw)
|
||||
assert result["success"] is True
|
||||
|
||||
def test_delete_via_dispatcher_threads_absorbed_into(self, tmp_path):
|
||||
# Dispatcher must plumb absorbed_into through to _delete_skill so the
|
||||
# validation + message suffix paths are exercised end-to-end.
|
||||
with _skill_dir(tmp_path):
|
||||
skill_manage(action="create", name="umbrella", content=VALID_SKILL_CONTENT)
|
||||
skill_manage(action="create", name="narrow", content=VALID_SKILL_CONTENT)
|
||||
raw = skill_manage(action="delete", name="narrow", absorbed_into="umbrella")
|
||||
result = json.loads(raw)
|
||||
assert result["success"] is True
|
||||
assert "absorbed into 'umbrella'" in result["message"]
|
||||
|
||||
def test_delete_via_dispatcher_rejects_missing_absorbed_target(self, tmp_path):
|
||||
with _skill_dir(tmp_path):
|
||||
skill_manage(action="create", name="narrow", content=VALID_SKILL_CONTENT)
|
||||
raw = skill_manage(action="delete", name="narrow", absorbed_into="ghost")
|
||||
result = json.loads(raw)
|
||||
assert result["success"] is False
|
||||
assert "does not exist" in result["error"]
|
||||
|
||||
|
||||
class TestSecurityScanGate:
|
||||
"""_security_scan_skill is gated by skills.guard_agent_created config flag."""
|
||||
|
||||
@@ -0,0 +1,196 @@
|
||||
"""Tests for /goal handling in tui_gateway.
|
||||
|
||||
The TUI routes ``/goal`` through ``command.dispatch`` (not ``slash.exec``)
|
||||
because the CLI's ``_handle_goal_command`` queues the kickoff message onto
|
||||
``_pending_input``, which the slash-worker subprocess has no reader for.
|
||||
Instead we handle ``/goal`` directly in the server and return a
|
||||
``{"type": "send", "notice": ..., "message": ...}`` payload the TUI client
|
||||
uses to render a system line and fire the kickoff prompt.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib
|
||||
import threading
|
||||
from pathlib import Path
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def hermes_home(tmp_path, monkeypatch):
|
||||
home = tmp_path / ".hermes"
|
||||
home.mkdir()
|
||||
monkeypatch.setattr(Path, "home", lambda: tmp_path)
|
||||
monkeypatch.setenv("HERMES_HOME", str(home))
|
||||
|
||||
# Bust the goal-module DB cache so it re-resolves HERMES_HOME.
|
||||
from hermes_cli import goals
|
||||
|
||||
goals._DB_CACHE.clear()
|
||||
yield home
|
||||
goals._DB_CACHE.clear()
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def server(hermes_home):
|
||||
with patch.dict(
|
||||
"sys.modules",
|
||||
{
|
||||
"hermes_cli.env_loader": MagicMock(),
|
||||
"hermes_cli.banner": MagicMock(),
|
||||
},
|
||||
):
|
||||
mod = importlib.import_module("tui_gateway.server")
|
||||
yield mod
|
||||
mod._sessions.clear()
|
||||
mod._pending.clear()
|
||||
mod._answers.clear()
|
||||
mod._methods.clear()
|
||||
importlib.reload(mod)
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def session(server):
|
||||
sid = "sid-test"
|
||||
session_key = "tui-goal-session-1"
|
||||
s = {
|
||||
"session_key": session_key,
|
||||
"history": [],
|
||||
"history_lock": threading.Lock(),
|
||||
"history_version": 0,
|
||||
"running": False,
|
||||
"attached_images": [],
|
||||
"cols": 120,
|
||||
}
|
||||
server._sessions[sid] = s
|
||||
return sid, session_key, s
|
||||
|
||||
|
||||
def _call(server, method, **params):
|
||||
handler = server._methods[method]
|
||||
return handler(1, params)
|
||||
|
||||
|
||||
# ── command.dispatch /goal ────────────────────────────────────────────
|
||||
|
||||
|
||||
def test_goal_bare_shows_status_when_none_set(server, session):
|
||||
sid, _, _ = session
|
||||
r = _call(server, "command.dispatch", name="goal", arg="", session_id=sid)
|
||||
assert r["result"]["type"] == "exec"
|
||||
assert "No active goal" in r["result"]["output"]
|
||||
|
||||
|
||||
def test_goal_whitespace_only_shows_status(server, session):
|
||||
sid, _, _ = session
|
||||
r = _call(server, "command.dispatch", name="goal", arg=" ", session_id=sid)
|
||||
assert r["result"]["type"] == "exec"
|
||||
assert "No active goal" in r["result"]["output"]
|
||||
|
||||
|
||||
def test_goal_status_alias_shows_status(server, session):
|
||||
sid, _, _ = session
|
||||
r = _call(server, "command.dispatch", name="goal", arg="status", session_id=sid)
|
||||
assert r["result"]["type"] == "exec"
|
||||
assert "No active goal" in r["result"]["output"]
|
||||
|
||||
|
||||
def test_goal_set_returns_send_with_notice(server, session):
|
||||
sid, session_key, _ = session
|
||||
r = _call(server, "command.dispatch", name="goal", arg="build a rocket", session_id=sid)
|
||||
result = r["result"]
|
||||
assert result["type"] == "send"
|
||||
assert result["message"] == "build a rocket"
|
||||
assert "notice" in result
|
||||
assert "Goal set" in result["notice"]
|
||||
assert "20-turn budget" in result["notice"]
|
||||
|
||||
# Persisted in SessionDB
|
||||
from hermes_cli.goals import GoalManager
|
||||
|
||||
mgr = GoalManager(session_key)
|
||||
assert mgr.state is not None
|
||||
assert mgr.state.goal == "build a rocket"
|
||||
assert mgr.state.status == "active"
|
||||
|
||||
|
||||
def test_goal_pause_after_set(server, session):
|
||||
sid, session_key, _ = session
|
||||
_call(server, "command.dispatch", name="goal", arg="write a story", session_id=sid)
|
||||
r = _call(server, "command.dispatch", name="goal", arg="pause", session_id=sid)
|
||||
assert r["result"]["type"] == "exec"
|
||||
assert "paused" in r["result"]["output"].lower()
|
||||
|
||||
from hermes_cli.goals import GoalManager
|
||||
|
||||
assert GoalManager(session_key).state.status == "paused"
|
||||
|
||||
|
||||
def test_goal_resume_reactivates(server, session):
|
||||
sid, session_key, _ = session
|
||||
_call(server, "command.dispatch", name="goal", arg="write a story", session_id=sid)
|
||||
_call(server, "command.dispatch", name="goal", arg="pause", session_id=sid)
|
||||
r = _call(server, "command.dispatch", name="goal", arg="resume", session_id=sid)
|
||||
assert r["result"]["type"] == "exec"
|
||||
assert "resumed" in r["result"]["output"].lower()
|
||||
|
||||
from hermes_cli.goals import GoalManager
|
||||
|
||||
assert GoalManager(session_key).state.status == "active"
|
||||
|
||||
|
||||
def test_goal_clear_removes_active_goal(server, session):
|
||||
sid, session_key, _ = session
|
||||
_call(server, "command.dispatch", name="goal", arg="write a story", session_id=sid)
|
||||
r = _call(server, "command.dispatch", name="goal", arg="clear", session_id=sid)
|
||||
assert r["result"]["type"] == "exec"
|
||||
assert "cleared" in r["result"]["output"].lower()
|
||||
|
||||
from hermes_cli.goals import GoalManager
|
||||
|
||||
# After clear the row is marked status=cleared (kept for audit);
|
||||
# ``has_goal()`` / ``is_active()`` return False so the goal loop
|
||||
# stays off and ``status`` reports "No active goal".
|
||||
mgr = GoalManager(session_key)
|
||||
assert not mgr.has_goal()
|
||||
assert not mgr.is_active()
|
||||
assert "No active goal" in mgr.status_line()
|
||||
|
||||
|
||||
def test_goal_stop_and_done_are_clear_aliases(server, session):
|
||||
sid, _, _ = session
|
||||
_call(server, "command.dispatch", name="goal", arg="first goal", session_id=sid)
|
||||
r = _call(server, "command.dispatch", name="goal", arg="stop", session_id=sid)
|
||||
assert "cleared" in r["result"]["output"].lower()
|
||||
|
||||
_call(server, "command.dispatch", name="goal", arg="second goal", session_id=sid)
|
||||
r = _call(server, "command.dispatch", name="goal", arg="done", session_id=sid)
|
||||
assert "cleared" in r["result"]["output"].lower()
|
||||
|
||||
|
||||
def test_goal_requires_session(server):
|
||||
r = _call(server, "command.dispatch", name="goal", arg="nope", session_id="unknown")
|
||||
assert "error" in r
|
||||
assert r["error"]["code"] == 4001
|
||||
|
||||
|
||||
# ── slash.exec /goal routing ──────────────────────────────────────────
|
||||
|
||||
|
||||
def test_slash_exec_rejects_goal_routes_to_command_dispatch(server, session):
|
||||
"""slash.exec must reject /goal with 4018 so the TUI client falls through
|
||||
to command.dispatch. Without this, the HermesCLI slash-worker subprocess
|
||||
would set the goal but silently drop the kickoff — the queue is in-proc."""
|
||||
sid, _, _ = session
|
||||
r = _call(server, "slash.exec", command="goal status", session_id=sid)
|
||||
assert "error" in r
|
||||
assert r["error"]["code"] == 4018
|
||||
assert "command.dispatch" in r["error"]["message"]
|
||||
|
||||
|
||||
def test_pending_input_commands_includes_goal(server):
|
||||
"""Guard: _PENDING_INPUT_COMMANDS must list 'goal' — removing it would
|
||||
silently re-break the TUI."""
|
||||
assert "goal" in server._PENDING_INPUT_COMMANDS
|
||||
Reference in New Issue
Block a user