diff --git a/bot/tests/test_agent_loop_outcome.py b/bot/tests/test_agent_loop_outcome.py index 153af3b3c..646620ad6 100644 --- a/bot/tests/test_agent_loop_outcome.py +++ b/bot/tests/test_agent_loop_outcome.py @@ -274,6 +274,824 @@ async def execute(self, tool_name, arguments, **kwargs): assert token_usage == {"prompt_tokens": 17, "completion_tokens": 7, "total_tokens": 24} +@pytest.mark.asyncio +async def test_agent_loop_returns_content_after_memory_commit_side_effect_only( + temp_dir: Path, monkeypatch +): + monkeypatch.setattr(AgentLoop, "_register_builtin_hooks", lambda self: None) + monkeypatch.setattr(AgentLoop, "_register_default_tools", lambda self: None) + monkeypatch.setattr("vikingbot.agent.loop.SubagentManager", _FakeSubagentManager) + + class _OVConfig: + exp_write_tools = [] + + class _LoadedConfig: + ov_server = _OVConfig() + + monkeypatch.setattr(loop_module, "load_config", lambda: _LoadedConfig()) + + useful_answer = "### Implementation Location\nThe answer is already complete." + + class _MemoryCommitProvider(LLMProvider): + def __init__(self): + super().__init__() + self.calls = [] + + async def chat(self, messages, tools=None, **kwargs): + self.calls.append( + { + "messages": [dict(message) for message in messages], + "tools": tools, + } + ) + if len(self.calls) > 1: + assert tools == [] + return LLMResponse( + content=useful_answer, + usage={"prompt_tokens": 6, "completion_tokens": 4, "total_tokens": 10}, + ) + return LLMResponse( + content=useful_answer, + tool_calls=[ + ToolCallRequest( + id="call-memory-1", + name="openviking_memory_commit", + arguments={ + "messages": [ + {"role": "user", "content": "repo-QA question"}, + {"role": "assistant", "content": useful_answer}, + ] + }, + tokens=5, + ) + ], + usage={"prompt_tokens": 10, "completion_tokens": 4, "total_tokens": 14}, + ) + + def get_default_model(self) -> str: + return "fake-model" + + class _ToolRegistry: + def __init__(self): + self.execute_calls = [] + + def get_definitions(self, **kwargs): + return [ + { + "type": "function", + "function": { + "name": "openviking_memory_commit", + "description": "Commit memory", + "parameters": {"type": "object", "properties": {}}, + }, + } + ] + + async def execute(self, tool_name, arguments, **kwargs): + self.execute_calls.append((tool_name, arguments, kwargs)) + return '{"status": "success", "task_status": "running"}' + + provider = _MemoryCommitProvider() + tools = _ToolRegistry() + bus = MessageBus() + config = Config(storage_workspace=str(temp_dir)) + loop = AgentLoop( + bus=bus, + provider=provider, + workspace=temp_dir / "workspace", + config=config, + ) + loop.tools = tools + + session_key = SessionKey(type="cli", channel_id="default", chat_id="memory-side-effect") + final_content, _reasoning, tools_used, token_usage, iteration = await loop._run_agent_loop( + messages=[{"role": "user", "content": "repo-QA question"}], + session_key=session_key, + sender_id="user-1", + publish_events=False, + ) + + assert final_content == useful_answer + assert iteration == 2 + assert len(provider.calls) == 2 + assert provider.calls[1]["messages"][-1]["content"].startswith( + "Side-effect tools have completed." + ) + assert not any(message.get("role") == "tool" for message in provider.calls[1]["messages"]) + assert provider.calls[1]["tools"] == [] + assert len(tools.execute_calls) == 1 + assert tools.execute_calls[0][:2] == ( + "openviking_memory_commit", + { + "messages": [ + {"role": "user", "content": "repo-QA question"}, + {"role": "assistant", "content": useful_answer}, + ] + }, + ) + assert [tool["tool_name"] for tool in tools_used] == ["openviking_memory_commit"] + assert token_usage == {"prompt_tokens": 16, "completion_tokens": 8, "total_tokens": 24} + + +@pytest.mark.asyncio +async def test_agent_loop_suppresses_memory_commit_outbound_tool_events( + temp_dir: Path, monkeypatch +): + monkeypatch.setattr(AgentLoop, "_register_builtin_hooks", lambda self: None) + monkeypatch.setattr(AgentLoop, "_register_default_tools", lambda self: None) + monkeypatch.setattr("vikingbot.agent.loop.SubagentManager", _FakeSubagentManager) + + class _OVConfig: + exp_write_tools = [] + + class _LoadedConfig: + ov_server = _OVConfig() + + monkeypatch.setattr(loop_module, "load_config", lambda: _LoadedConfig()) + + class _MemoryCommitProvider(LLMProvider): + def __init__(self): + super().__init__() + self.calls = [] + + async def chat(self, messages, tools=None, **kwargs): + self.calls.append([dict(message) for message in messages]) + if len(self.calls) > 1: + return LLMResponse( + content="final answer", + usage={"prompt_tokens": 6, "completion_tokens": 3, "total_tokens": 9}, + ) + return LLMResponse( + content="draft answer", + tool_calls=[ + ToolCallRequest( + id="call-memory-1", + name="openviking_memory_commit", + arguments={"messages": [{"role": "user", "content": "question"}]}, + tokens=5, + ) + ], + usage={"prompt_tokens": 10, "completion_tokens": 4, "total_tokens": 14}, + ) + + def get_default_model(self) -> str: + return "fake-model" + + class _ToolRegistry: + def get_definitions(self, **kwargs): + return [ + { + "type": "function", + "function": { + "name": "openviking_memory_commit", + "description": "Commit memory", + "parameters": {"type": "object", "properties": {}}, + }, + } + ] + + async def execute(self, tool_name, arguments, **kwargs): + return '{"status": "success", "task_status": "running"}' + + bus = MessageBus() + config = Config(storage_workspace=str(temp_dir)) + loop = AgentLoop( + bus=bus, + provider=_MemoryCommitProvider(), + workspace=temp_dir / "workspace", + config=config, + ) + loop.tools = _ToolRegistry() + + session_key = SessionKey(type="cli", channel_id="default", chat_id="memory-events") + final_content, _reasoning, tools_used, _token_usage, _iteration = await loop._run_agent_loop( + messages=[{"role": "user", "content": "question"}], + session_key=session_key, + sender_id="user-1", + publish_events=True, + ) + + assert final_content == "final answer" + assert [tool["tool_name"] for tool in tools_used] == ["openviking_memory_commit"] + + events = [] + while bus.outbound_size: + events.append(await bus.consume_outbound()) + + assert not any( + event.event_type in {OutboundEventType.TOOL_CALL, OutboundEventType.TOOL_RESULT} + and "openviking_memory_commit" in event.content + for event in events + ) + assert not any( + event.event_type == OutboundEventType.TOOL_RESULT + and "task_status" in event.content + for event in events + ) + + +@pytest.mark.asyncio +async def test_agent_loop_requests_final_answer_after_empty_memory_commit_only( + temp_dir: Path, monkeypatch +): + monkeypatch.setattr(AgentLoop, "_register_builtin_hooks", lambda self: None) + monkeypatch.setattr(AgentLoop, "_register_default_tools", lambda self: None) + monkeypatch.setattr("vikingbot.agent.loop.SubagentManager", _FakeSubagentManager) + + class _OVConfig: + exp_write_tools = [] + + class _LoadedConfig: + ov_server = _OVConfig() + + monkeypatch.setattr(loop_module, "load_config", lambda: _LoadedConfig()) + + class _EmptyMemoryCommitProvider(LLMProvider): + def __init__(self): + super().__init__() + self.calls = [] + + async def chat(self, messages, tools=None, **kwargs): + self.calls.append( + { + "messages": [dict(message) for message in messages], + "tools": tools, + } + ) + if len(self.calls) == 1: + return LLMResponse( + content="", + tool_calls=[ + ToolCallRequest( + id="call-memory-1", + name="openviking_memory_commit", + arguments={"messages": []}, + tokens=5, + ) + ], + usage={"prompt_tokens": 10, "completion_tokens": 2, "total_tokens": 12}, + ) + return LLMResponse( + content="final answer without memory status", + usage={"prompt_tokens": 8, "completion_tokens": 5, "total_tokens": 13}, + ) + + def get_default_model(self) -> str: + return "fake-model" + + class _ToolRegistry: + def get_definitions(self, **kwargs): + return [ + { + "type": "function", + "function": { + "name": "openviking_memory_commit", + "description": "Commit memory", + "parameters": {"type": "object", "properties": {}}, + }, + } + ] + + async def execute(self, tool_name, arguments, **kwargs): + return '{"status": "success", "task_status": "running"}' + + provider = _EmptyMemoryCommitProvider() + bus = MessageBus() + config = Config(storage_workspace=str(temp_dir)) + loop = AgentLoop( + bus=bus, + provider=provider, + workspace=temp_dir / "workspace", + config=config, + ) + loop.tools = _ToolRegistry() + + session_key = SessionKey(type="cli", channel_id="default", chat_id="empty-memory") + final_content, _reasoning, tools_used, token_usage, iteration = await loop._run_agent_loop( + messages=[{"role": "user", "content": "question"}], + session_key=session_key, + sender_id="user-1", + publish_events=False, + ) + + assert final_content == "final answer without memory status" + assert iteration == 2 + second_call_messages = provider.calls[1]["messages"] + assert second_call_messages[-1] == { + "role": "user", + "content": ( + "Side-effect tools have completed. Do not call more tools. " + "Answer the user's original request directly using only the conversation above. " + "If your previous assistant message already answered the request, return that " + "answer without mentioning side-effect tool status." + ), + } + assert provider.calls[1]["tools"] == [] + assert not any(message.get("role") == "tool" for message in second_call_messages) + assert [tool["tool_name"] for tool in tools_used] == ["openviking_memory_commit"] + assert token_usage == {"prompt_tokens": 18, "completion_tokens": 7, "total_tokens": 25} + + +@pytest.mark.asyncio +async def test_agent_loop_records_failed_memory_commit_without_exposing_result( + temp_dir: Path, monkeypatch +): + monkeypatch.setattr(AgentLoop, "_register_builtin_hooks", lambda self: None) + monkeypatch.setattr(AgentLoop, "_register_default_tools", lambda self: None) + monkeypatch.setattr("vikingbot.agent.loop.SubagentManager", _FakeSubagentManager) + + class _OVConfig: + exp_write_tools = [] + + class _LoadedConfig: + ov_server = _OVConfig() + + monkeypatch.setattr(loop_module, "load_config", lambda: _LoadedConfig()) + + class _FailedMemoryCommitProvider(LLMProvider): + def __init__(self): + super().__init__() + self.calls = [] + + async def chat(self, messages, tools=None, **kwargs): + self.calls.append( + { + "messages": [dict(message) for message in messages], + "tools": tools, + } + ) + if len(self.calls) > 1: + assert tools == [] + return LLMResponse( + content="useful answer should still be returned", + usage={"prompt_tokens": 8, "completion_tokens": 5, "total_tokens": 13}, + ) + return LLMResponse( + content="Saving this answer to memory.", + tool_calls=[ + ToolCallRequest( + id="call-memory-1", + name="openviking_memory_commit", + arguments={"messages": []}, + tokens=5, + ) + ], + usage={"prompt_tokens": 10, "completion_tokens": 4, "total_tokens": 14}, + ) + + def get_default_model(self) -> str: + return "fake-model" + + class _ToolRegistry: + def get_definitions(self, **kwargs): + return [ + { + "type": "function", + "function": { + "name": "openviking_memory_commit", + "description": "Commit memory", + "parameters": {"type": "object", "properties": {}}, + }, + } + ] + + async def execute(self, tool_name, arguments, **kwargs): + return '{"status": "failed", "error": "memory service unavailable"}' + + provider = _FailedMemoryCommitProvider() + bus = MessageBus() + config = Config(storage_workspace=str(temp_dir)) + loop = AgentLoop( + bus=bus, + provider=provider, + workspace=temp_dir / "workspace", + config=config, + ) + loop.tools = _ToolRegistry() + + session_key = SessionKey(type="cli", channel_id="default", chat_id="failed-memory") + final_content, _reasoning, tools_used, token_usage, iteration = await loop._run_agent_loop( + messages=[{"role": "user", "content": "question"}], + session_key=session_key, + sender_id="user-1", + publish_events=False, + ) + + assert final_content == "useful answer should still be returned" + assert iteration == 2 + assert len(provider.calls) == 2 + assert provider.calls[1]["tools"] == [] + assert "do not claim that side effect succeeded" in provider.calls[1]["messages"][-1][ + "content" + ] + assert not any(message.get("role") == "tool" for message in provider.calls[1]["messages"]) + assert len(tools_used) == 1 + assert tools_used[0]["tool_name"] == "openviking_memory_commit" + assert tools_used[0]["args"] == '{"messages": []}' + assert tools_used[0]["result"] == ( + '{"status": "failed", "error": "memory service unavailable"}' + ) + assert tools_used[0]["execute_success"] is False + assert tools_used[0]["input_token"] == 5 + assert token_usage == {"prompt_tokens": 18, "completion_tokens": 9, "total_tokens": 27} + + +@pytest.mark.asyncio +async def test_agent_loop_hides_memory_commit_result_when_mixed_with_information_tool( + temp_dir: Path, monkeypatch +): + monkeypatch.setattr(AgentLoop, "_register_builtin_hooks", lambda self: None) + monkeypatch.setattr(AgentLoop, "_register_default_tools", lambda self: None) + monkeypatch.setattr("vikingbot.agent.loop.SubagentManager", _FakeSubagentManager) + + class _OVConfig: + exp_write_tools = [] + + class _LoadedConfig: + ov_server = _OVConfig() + + monkeypatch.setattr(loop_module, "load_config", lambda: _LoadedConfig()) + + class _MixedToolProvider(LLMProvider): + def __init__(self): + super().__init__() + self.calls = [] + + async def chat(self, messages, tools=None, **kwargs): + self.calls.append( + { + "messages": [dict(message) for message in messages], + "tools": tools, + } + ) + if len(self.calls) == 1: + return LLMResponse( + content="I will look up one detail and remember this exchange.", + tool_calls=[ + ToolCallRequest( + id="call-memory-1", + name="openviking_memory_commit", + arguments={"messages": [{"role": "user", "content": "question"}]}, + tokens=5, + ), + ToolCallRequest( + id="call-lookup-1", + name="lookup_fact", + arguments={"query": "current facts"}, + tokens=3, + ), + ], + usage={"prompt_tokens": 10, "completion_tokens": 4, "total_tokens": 14}, + ) + return LLMResponse( + content="final answer from lookup", + usage={"prompt_tokens": 7, "completion_tokens": 5, "total_tokens": 12}, + ) + + def get_default_model(self) -> str: + return "fake-model" + + class _ToolRegistry: + def __init__(self): + self.execute_calls = [] + self.definition_calls = [] + + def get_definitions(self, **kwargs): + self.definition_calls.append(kwargs) + disabled_tools = set(kwargs.get("disabled_tools") or []) + definitions = [ + { + "type": "function", + "function": { + "name": "openviking_memory_commit", + "description": "Commit memory", + "parameters": {"type": "object", "properties": {}}, + }, + }, + { + "type": "function", + "function": { + "name": "lookup_fact", + "description": "Lookup fact", + "parameters": {"type": "object", "properties": {}}, + }, + }, + ] + return [ + definition + for definition in definitions + if definition["function"]["name"] not in disabled_tools + ] + + async def execute(self, tool_name, arguments, **kwargs): + self.execute_calls.append((tool_name, arguments, kwargs)) + if tool_name == "openviking_memory_commit": + return '{"status": "success", "task_status": "running"}' + return "tool result: useful context" + + provider = _MixedToolProvider() + tools = _ToolRegistry() + bus = MessageBus() + config = Config(storage_workspace=str(temp_dir)) + loop = AgentLoop( + bus=bus, + provider=provider, + workspace=temp_dir / "workspace", + config=config, + ) + loop.tools = tools + + session_key = SessionKey(type="cli", channel_id="default", chat_id="memory-mixed") + final_content, _reasoning, tools_used, token_usage, iteration = await loop._run_agent_loop( + messages=[{"role": "user", "content": "question"}], + session_key=session_key, + sender_id="user-1", + publish_events=False, + ) + + assert final_content == "final answer from lookup" + assert iteration == 2 + assert [call[0] for call in tools.execute_calls] == [ + "openviking_memory_commit", + "lookup_fact", + ] + assert [tool["tool_name"] for tool in tools_used] == [ + "openviking_memory_commit", + "lookup_fact", + ] + second_call_messages = provider.calls[1]["messages"] + assert second_call_messages[-1] == { + "role": "user", + "content": "Reflect on the results and decide next steps.", + } + assert any( + message.get("role") == "tool" + and message.get("name") == "lookup_fact" + and message.get("content") == "tool result: useful context" + for message in second_call_messages + ) + assert not any( + message.get("role") == "tool" and message.get("name") == "openviking_memory_commit" + for message in second_call_messages + ) + assistant_tool_calls = [ + tool_call["function"]["name"] + for message in second_call_messages + for tool_call in message.get("tool_calls", []) + ] + assert assistant_tool_calls == ["lookup_fact"] + assert "openviking_memory_commit" in tools.definition_calls[1]["disabled_tools"] + second_call_tool_names = [ + tool["function"]["name"] for tool in provider.calls[1]["tools"] + ] + assert second_call_tool_names == ["lookup_fact"] + assert token_usage == {"prompt_tokens": 17, "completion_tokens": 9, "total_tokens": 26} + + +@pytest.mark.asyncio +async def test_agent_loop_guides_mixed_reflection_after_failed_memory_commit( + temp_dir: Path, monkeypatch +): + monkeypatch.setattr(AgentLoop, "_register_builtin_hooks", lambda self: None) + monkeypatch.setattr(AgentLoop, "_register_default_tools", lambda self: None) + monkeypatch.setattr("vikingbot.agent.loop.SubagentManager", _FakeSubagentManager) + + class _OVConfig: + exp_write_tools = [] + + class _LoadedConfig: + ov_server = _OVConfig() + + monkeypatch.setattr(loop_module, "load_config", lambda: _LoadedConfig()) + + class _MixedFailureProvider(LLMProvider): + def __init__(self): + super().__init__() + self.calls = [] + + async def chat(self, messages, tools=None, **kwargs): + self.calls.append( + { + "messages": [dict(message) for message in messages], + "tools": tools, + } + ) + if len(self.calls) == 1: + return LLMResponse( + content="I will look up one detail and remember this exchange.", + tool_calls=[ + ToolCallRequest( + id="call-memory-1", + name="openviking_memory_commit", + arguments={"messages": [{"role": "user", "content": "question"}]}, + tokens=5, + ), + ToolCallRequest( + id="call-lookup-1", + name="lookup_fact", + arguments={"query": "current facts"}, + tokens=3, + ), + ], + usage={"prompt_tokens": 10, "completion_tokens": 4, "total_tokens": 14}, + ) + return LLMResponse( + content="final answer from lookup; memory was not saved", + usage={"prompt_tokens": 7, "completion_tokens": 6, "total_tokens": 13}, + ) + + def get_default_model(self) -> str: + return "fake-model" + + class _ToolRegistry: + def __init__(self): + self.execute_calls = [] + self.definition_calls = [] + + def get_definitions(self, **kwargs): + self.definition_calls.append(kwargs) + disabled_tools = set(kwargs.get("disabled_tools") or []) + definitions = [ + { + "type": "function", + "function": { + "name": "openviking_memory_commit", + "description": "Commit memory", + "parameters": {"type": "object", "properties": {}}, + }, + }, + { + "type": "function", + "function": { + "name": "lookup_fact", + "description": "Lookup fact", + "parameters": {"type": "object", "properties": {}}, + }, + }, + ] + return [ + definition + for definition in definitions + if definition["function"]["name"] not in disabled_tools + ] + + async def execute(self, tool_name, arguments, **kwargs): + self.execute_calls.append((tool_name, arguments, kwargs)) + if tool_name == "openviking_memory_commit": + return '{"status": "failed"}' + return "tool result: useful context" + + provider = _MixedFailureProvider() + tools = _ToolRegistry() + bus = MessageBus() + config = Config(storage_workspace=str(temp_dir)) + loop = AgentLoop( + bus=bus, + provider=provider, + workspace=temp_dir / "workspace", + config=config, + ) + loop.tools = tools + + session_key = SessionKey(type="cli", channel_id="default", chat_id="memory-mixed-failed") + final_content, _reasoning, tools_used, token_usage, iteration = await loop._run_agent_loop( + messages=[{"role": "user", "content": "question"}], + session_key=session_key, + sender_id="user-1", + publish_events=False, + ) + + assert final_content == "final answer from lookup; memory was not saved" + assert iteration == 2 + assert [call[0] for call in tools.execute_calls] == [ + "openviking_memory_commit", + "lookup_fact", + ] + assert tools_used[0]["execute_success"] is False + second_call_messages = provider.calls[1]["messages"] + assert "do not claim that side effect succeeded" in second_call_messages[-1]["content"] + assert not any( + message.get("role") == "tool" and message.get("name") == "openviking_memory_commit" + for message in second_call_messages + ) + assert "openviking_memory_commit" in tools.definition_calls[1]["disabled_tools"] + second_call_tool_names = [ + tool["function"]["name"] for tool in provider.calls[1]["tools"] + ] + assert second_call_tool_names == ["lookup_fact"] + assert token_usage == {"prompt_tokens": 17, "completion_tokens": 10, "total_tokens": 27} + + +@pytest.mark.asyncio +async def test_agent_loop_preserves_stop_tool_behavior_with_hidden_memory_commit( + temp_dir: Path, monkeypatch +): + monkeypatch.setattr(AgentLoop, "_register_builtin_hooks", lambda self: None) + monkeypatch.setattr(AgentLoop, "_register_default_tools", lambda self: None) + monkeypatch.setattr("vikingbot.agent.loop.SubagentManager", _FakeSubagentManager) + + class _OVConfig: + exp_write_tools = [] + + class _LoadedConfig: + ov_server = _OVConfig() + + monkeypatch.setattr(loop_module, "load_config", lambda: _LoadedConfig()) + + class _StopToolProvider(LLMProvider): + def __init__(self): + super().__init__() + self.calls = [] + + async def chat(self, messages, tools=None, **kwargs): + self.calls.append([dict(message) for message in messages]) + return LLMResponse( + content="Forwarded to the user simulator.", + tool_calls=[ + ToolCallRequest( + id="call-stop-1", + name="finish_session", + arguments={"reason": "done"}, + tokens=2, + ), + ToolCallRequest( + id="call-memory-1", + name="openviking_memory_commit", + arguments={"messages": []}, + tokens=5, + ), + ], + usage={"prompt_tokens": 10, "completion_tokens": 4, "total_tokens": 14}, + ) + + def get_default_model(self) -> str: + return "fake-model" + + class _ToolRegistry: + def __init__(self): + self.execute_calls = [] + + def get_definitions(self, **kwargs): + return [ + { + "type": "function", + "function": { + "name": "finish_session", + "description": "Stop", + "parameters": {"type": "object", "properties": {}}, + }, + }, + { + "type": "function", + "function": { + "name": "openviking_memory_commit", + "description": "Commit memory", + "parameters": {"type": "object", "properties": {}}, + }, + }, + ] + + async def execute(self, tool_name, arguments, **kwargs): + self.execute_calls.append((tool_name, arguments, kwargs)) + if tool_name == "openviking_memory_commit": + return '{"status": "success", "task_status": "running"}' + return "finished" + + provider = _StopToolProvider() + tools = _ToolRegistry() + bus = MessageBus() + config = Config(storage_workspace=str(temp_dir)) + loop = AgentLoop( + bus=bus, + provider=provider, + workspace=temp_dir / "workspace", + config=config, + ) + loop.tools = tools + + session_key = SessionKey(type="cli", channel_id="default", chat_id="memory-stop") + final_content, _reasoning, tools_used, token_usage, iteration = await loop._run_agent_loop( + messages=[{"role": "user", "content": "question"}], + session_key=session_key, + sender_id="user-1", + publish_events=False, + stop_tool_names=["finish_session"], + ) + + assert final_content == "" + assert iteration == 1 + assert len(provider.calls) == 1 + assert [call[0] for call in tools.execute_calls] == [ + "finish_session", + "openviking_memory_commit", + ] + assert [tool["tool_name"] for tool in tools_used] == [ + "finish_session", + "openviking_memory_commit", + ] + assert token_usage == {"prompt_tokens": 10, "completion_tokens": 4, "total_tokens": 14} + + @pytest.mark.asyncio async def test_agent_loop_evaluates_previous_response_outcome_before_new_user_turn( temp_dir: Path, monkeypatch diff --git a/bot/vikingbot/agent/loop.py b/bot/vikingbot/agent/loop.py index 7616715ab..0f57b62ba 100644 --- a/bot/vikingbot/agent/loop.py +++ b/bot/vikingbot/agent/loop.py @@ -50,10 +50,40 @@ def _is_tool_result_success(result: Any) -> bool: if result is None or isinstance(result, Exception): return False + parsed_result = result if isinstance(result, dict) else None + if isinstance(result, str): + try: + maybe_json = json.loads(result) + except json.JSONDecodeError: + maybe_json = None + if isinstance(maybe_json, dict): + parsed_result = maybe_json + + if isinstance(parsed_result, dict): + for key in ("success", "ok"): + if parsed_result.get(key) is False: + return False + for key in ("status", "task_status", "state"): + value = parsed_result.get(key) + if isinstance(value, str) and value.lower() in { + "error", + "errored", + "fail", + "failed", + "failure", + }: + return False + if parsed_result.get("error"): + return False + text = str(result).lstrip() return bool(text) and not text.startswith("Error:") +# Side-effect tool results are traced locally but should not steer the next LLM turn. +_SIDE_EFFECT_TOOL_NAMES = {"openviking_memory_commit"} + + @dataclass(slots=True) class _PlainTextContext: """Context passed to an `on_plain_text` callback when the model emits plain text.""" @@ -742,6 +772,10 @@ async def _run_agent_loop( } write_exp_injected = False stop_tools = set(stop_tool_names or []) + executed_side_effect_tools: set[str] = set() + stop_tool_completed = False + force_no_tools_once = False + side_effect_finalization_fallback_content: str | None = None def accumulate_token_usage(response: Any) -> None: if not response.usage: @@ -763,10 +797,18 @@ def accumulate_token_usage(response: Any) -> None: ) ) - tool_definitions = self.tools.get_definitions( - ov_tools_enable=ov_tools_enable, - disabled_tools=disabled_tools, - ) + handling_side_effect_finalization = force_no_tools_once + if force_no_tools_once: + tool_definitions = [] + force_no_tools_once = False + else: + effective_disabled_tools = list( + dict.fromkeys([*(disabled_tools or []), *executed_side_effect_tools]) + ) + tool_definitions = self.tools.get_definitions( + ov_tools_enable=ov_tools_enable, + disabled_tools=effective_disabled_tools, + ) response, _streamed_content, streamed_reasoning = await self._chat_with_stream_events( messages=messages, tools=tool_definitions, @@ -784,6 +826,15 @@ def accumulate_token_usage(response: Any) -> None: ) ) + if handling_side_effect_finalization and response.has_tool_calls: + logger.warning( + "[SIDE_EFFECT_TOOL]: provider returned tool calls during side-effect " + "finalization; ignoring tool calls" + ) + final_content = response.content or side_effect_finalization_fallback_content + final_reasoning_content = response.reasoning_content + break + if response.has_tool_calls: # Inject experience memory before write-related tool calls (once per session) if not write_exp_injected: @@ -824,7 +875,12 @@ def accumulate_token_usage(response: Any) -> None: logger.warning(f"[WRITE_EXP]: failed to load experience: {_e}") final_reasoning_content = response.reasoning_content - args_list = [tc.arguments for tc in response.tool_calls] + visible_tool_calls = [ + tc for tc in response.tool_calls if tc.name not in _SIDE_EFFECT_TOOL_NAMES + ] + visible_tool_call_ids = {tc.id for tc in visible_tool_calls} + hidden_side_effect_results = [] + args_list = [tc.arguments for tc in visible_tool_calls] tool_call_dicts = [ { "id": tc.id, @@ -834,14 +890,15 @@ def accumulate_token_usage(response: Any) -> None: "arguments": json.dumps(args), }, } - for tc, args in zip(response.tool_calls, args_list, strict=False) + for tc, args in zip(visible_tool_calls, args_list, strict=False) ] - messages = self.context.add_assistant_message( - messages, - response.content, - tool_call_dicts, - reasoning_content=response.reasoning_content, - ) + if tool_call_dicts or response.content or response.reasoning_content: + messages = self.context.add_assistant_message( + messages, + response.content, + tool_call_dicts or None, + reasoning_content=response.reasoning_content, + ) # Stage 2: Execute all tools in parallel async def execute_single_tool(idx: int, tool_call): @@ -868,6 +925,8 @@ async def execute_single_tool(idx: int, tool_call): ] if publish_events: for tool_call in response.tool_calls: + if tool_call.name in _SIDE_EFFECT_TOOL_NAMES: + continue args_str = json.dumps(tool_call.arguments, ensure_ascii=False) await self.bus.publish_outbound( OutboundMessage( @@ -884,7 +943,7 @@ async def execute_single_tool(idx: int, tool_call): logger.info(f"[TOOL_CALL]: {tool_call.name}({args_str[:200]})") logger.info(f"[RESULT]: {str(result)[:600]}") - if publish_events: + if publish_events and tool_call.id in visible_tool_call_ids: await self.bus.publish_outbound( OutboundMessage( session_key=session_key, @@ -892,9 +951,12 @@ async def execute_single_tool(idx: int, tool_call): event_type=OutboundEventType.TOOL_RESULT, ) ) - messages = self.context.add_tool_result( - messages, tool_call.id, tool_call.name, result - ) + if tool_call.id in visible_tool_call_ids: + messages = self.context.add_tool_result( + messages, tool_call.id, tool_call.name, result + ) + else: + hidden_side_effect_results.append((tool_call, result)) tool_used_dict = { "tool_name": tool_call.name, @@ -907,16 +969,62 @@ async def execute_single_tool(idx: int, tool_call): } tools_used.append(tool_used_dict) + executed_side_effect_tool_names = [ + tool_call.name for tool_call, _result in hidden_side_effect_results + ] + failed_side_effects = [ + tool_call.name + for tool_call, result in hidden_side_effect_results + if not _is_tool_result_success(result) + ] + if failed_side_effects: + logger.warning( + "[SIDE_EFFECT_TOOL]: hidden side-effect tool failed: " + f"{', '.join(failed_side_effects)}" + ) + executed_side_effect_tools.update(executed_side_effect_tool_names) + if any( tool_call.name in stop_tools for _idx, tool_call, _result, _duration in results ): + stop_tool_completed = True final_content = "" break - messages.append( - {"role": "user", "content": "Reflect on the results and decide next steps."} - ) + if not visible_tool_call_ids: + final_prompt = ( + "Side-effect tools have completed. Do not call more tools. " + "Answer the user's original request directly using only the " + "conversation above. If your previous assistant message already " + "answered the request, return that answer without mentioning " + "side-effect tool status." + ) + if failed_side_effects: + final_prompt += ( + " A side-effect tool failed; do not claim that side effect " + "succeeded. If the user's request specifically depended on " + "that side effect, say it could not be completed." + ) + if isinstance(response.content, str) and response.content.strip(): + side_effect_finalization_fallback_content = response.content + messages.append( + { + "role": "user", + "content": final_prompt, + } + ) + force_no_tools_once = True + continue + + reflection_prompt = "Reflect on the results and decide next steps." + if failed_side_effects: + reflection_prompt += ( + " A side-effect tool failed; do not claim that side effect " + "succeeded. If the user's request specifically depended on " + "that side effect, say it could not be completed." + ) + messages.append({"role": "user", "content": reflection_prompt}) else: text = (response.content or "").strip() routed = False @@ -967,7 +1075,7 @@ async def execute_single_tool(idx: int, tool_call): continue break - if final_content == "" and tools_used and tools_used[-1].get("tool_name") in stop_tools: + if stop_tool_completed: pass elif final_content is None or (isinstance(final_content, str) and not final_content.strip()): if iteration >= self.max_iterations: @@ -1002,7 +1110,16 @@ async def execute_single_tool(idx: int, tool_call): ) ) - if final_content is None or (isinstance(final_content, str) and not final_content.strip()): + if ( + not stop_tool_completed + and side_effect_finalization_fallback_content + and (final_content is None or (isinstance(final_content, str) and not final_content.strip())) + ): + final_content = side_effect_finalization_fallback_content + + if not stop_tool_completed and ( + final_content is None or (isinstance(final_content, str) and not final_content.strip()) + ): if iteration >= self.max_iterations: final_content = ( "I reached the tool-use limit before completing every step, and the "