diff --git a/nanobot/agent/runner.py b/nanobot/agent/runner.py index 592af9de..19bed7d9 100644 --- a/nanobot/agent/runner.py +++ b/nanobot/agent/runner.py @@ -932,6 +932,15 @@ class AgentRunner: if message.get("role") == "user": kept = kept[i:] break + else: + # No user message in the kept window — walk backwards through + # non_system to find the nearest user message and keep it plus + # everything after it. Providers like GLM reject requests + # where the first non-system message is not ``user`` (error 1214). + for idx in range(len(non_system) - 1, -1, -1): + if non_system[idx].get("role") == "user": + kept = non_system[idx:] + break start = find_legal_message_start(kept) if start: kept = kept[start:] diff --git a/nanobot/providers/base.py b/nanobot/providers/base.py index 42cd1a10..cb23c0d2 100644 --- a/nanobot/providers/base.py +++ b/nanobot/providers/base.py @@ -409,6 +409,17 @@ class LLMProvider(ABC): recovered["role"] = "user" merged.append(recovered) + # Safety net: ensure the first non-system message is not a bare + # ``assistant`` message. Providers like GLM reject system→assistant + # with error 1214. This can happen when upstream truncation (e.g. + # _snip_history) drops the only user message. Insert a synthetic + # user message to keep the sequence valid. + for i, msg in enumerate(merged): + if msg.get("role") != "system": + if msg.get("role") == "assistant" and not msg.get("tool_calls"): + merged.insert(i, {"role": "user", "content": "(conversation continued)"}) + break + return merged @staticmethod diff --git a/tests/agent/test_runner.py b/tests/agent/test_runner.py index 9bea0a41..f890685c 100644 --- a/tests/agent/test_runner.py +++ b/tests/agent/test_runner.py @@ -643,10 +643,11 @@ def test_snip_history_drops_orphaned_tool_results_from_trimmed_slice(monkeypatch trimmed = runner._snip_history(spec, messages) - assert trimmed == [ - {"role": "system", "content": "system"}, - {"role": "assistant", "content": "after tool"}, - ] + # After the fix, the user message is recovered so the sequence is valid + # for providers that require system → user (e.g. GLM error 1214). + assert trimmed[0]["role"] == "system" + non_system = [m for m in trimmed if m["role"] != "system"] + assert non_system[0]["role"] == "user", f"Expected user after system, got {non_system[0]['role']}" @pytest.mark.asyncio @@ -2787,4 +2788,124 @@ async def test_injection_cycle_cap_on_error_path(): assert result.had_injections is True # Should cap: _MAX_INJECTION_CYCLES drained rounds + 1 final round that breaks assert call_count["n"] == _MAX_INJECTION_CYCLES + 1 - assert drain_count["n"] == _MAX_INJECTION_CYCLES + + +# --------------------------------------------------------------------------- +# Regression tests for GLM-1214: _snip_history must preserve a user message +# --------------------------------------------------------------------------- + + +def test_snip_history_preserves_user_message_after_truncation(monkeypatch): + """When _snip_history truncates messages and the only user message ends up + outside the kept window, the method must recover the nearest user message + so the resulting sequence is valid for providers like GLM (which reject + system→assistant with error 1214). + + This reproduces the exact scenario from the bug report: + - Normal interaction: user asks, assistant calls tool, tool returns, + assistant replies. + - Injection adds a phantom user message, triggering more tool calls. + - _snip_history activates, keeping only recent assistant/tool pairs. + - The injected user message is in the truncated prefix and gets lost. + """ + from nanobot.agent.runner import AgentRunSpec, AgentRunner + + provider = MagicMock() + tools = MagicMock() + tools.get_definitions.return_value = [] + runner = AgentRunner(provider) + + messages = [ + {"role": "system", "content": "system"}, + {"role": "assistant", "content": "previous reply"}, + {"role": "user", "content": ".nanobot的同目录"}, + { + "role": "assistant", + "content": None, + "tool_calls": [{"id": "tc_1", "type": "function", "function": {"name": "exec", "arguments": "{}"}}], + }, + {"role": "tool", "tool_call_id": "tc_1", "content": "tool output 1"}, + { + "role": "assistant", + "content": None, + "tool_calls": [{"id": "tc_2", "type": "function", "function": {"name": "exec", "arguments": "{}"}}], + }, + {"role": "tool", "tool_call_id": "tc_2", "content": "tool output 2"}, + ] + + spec = AgentRunSpec( + initial_messages=messages, + tools=tools, + model="test-model", + max_iterations=1, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + context_window_tokens=2000, + context_block_limit=100, + ) + + # Make estimate_prompt_tokens_chain report above budget so _snip_history activates. + monkeypatch.setattr("nanobot.agent.runner.estimate_prompt_tokens_chain", lambda *_a, **_kw: (500, None)) + # Make kept window small: only the last 2 messages fit the budget. + token_sizes = { + "system": 0, + "previous reply": 200, + ".nanobot的同目录": 80, + "tool output 1": 80, + "tool output 2": 80, + } + monkeypatch.setattr( + "nanobot.agent.runner.estimate_message_tokens", + lambda msg: token_sizes.get(str(msg.get("content")), 100), + ) + + trimmed = runner._snip_history(spec, messages) + + # The first non-system message MUST be user (not assistant). + non_system = [m for m in trimmed if m.get("role") != "system"] + assert non_system, "trimmed should contain at least one non-system message" + assert non_system[0]["role"] == "user", ( + f"First non-system message must be 'user', got '{non_system[0]['role']}'. " + f"Roles: {[m['role'] for m in trimmed]}" + ) + + +def test_snip_history_no_user_at_all_falls_back_gracefully(monkeypatch): + """Edge case: if non_system has zero user messages, _snip_history should + still return a valid sequence (not crash or produce system→assistant).""" + from nanobot.agent.runner import AgentRunSpec, AgentRunner + + provider = MagicMock() + tools = MagicMock() + tools.get_definitions.return_value = [] + runner = AgentRunner(provider) + + messages = [ + {"role": "system", "content": "system"}, + {"role": "assistant", "content": "reply"}, + {"role": "tool", "tool_call_id": "tc_1", "content": "result"}, + {"role": "assistant", "content": "reply 2"}, + {"role": "tool", "tool_call_id": "tc_2", "content": "result 2"}, + ] + + spec = AgentRunSpec( + initial_messages=messages, + tools=tools, + model="test-model", + max_iterations=1, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + context_window_tokens=2000, + context_block_limit=100, + ) + + monkeypatch.setattr("nanobot.agent.runner.estimate_prompt_tokens_chain", lambda *_a, **_kw: (500, None)) + monkeypatch.setattr( + "nanobot.agent.runner.estimate_message_tokens", + lambda msg: 100, + ) + + trimmed = runner._snip_history(spec, messages) + + # Should not crash. The result should still be a valid list. + assert isinstance(trimmed, list) + # Must have at least system. + assert any(m.get("role") == "system" for m in trimmed) diff --git a/tests/providers/test_enforce_role_alternation.py b/tests/providers/test_enforce_role_alternation.py index 333c5d04..3afb7c8e 100644 --- a/tests/providers/test_enforce_role_alternation.py +++ b/tests/providers/test_enforce_role_alternation.py @@ -195,3 +195,46 @@ class TestEnforceRoleAlternation: assert result[3]["role"] == "user" assert "And 3+3?" in result[3]["content"] assert "(please be quick)" in result[3]["content"] + + def test_leading_assistant_after_system_inserts_synthetic_user(self): + """When the first non-system message is assistant (no tool_calls), a + synthetic user message is inserted to prevent GLM error 1214.""" + msgs = [ + {"role": "system", "content": "sys"}, + {"role": "assistant", "content": "previous reply"}, + {"role": "tool", "tool_call_id": "tc_1", "content": "result"}, + {"role": "assistant", "content": "after tool"}, + ] + result = LLMProvider._enforce_role_alternation(msgs) + non_system = [m for m in result if m["role"] != "system"] + assert non_system[0]["role"] == "user" + assert non_system[0]["content"] == "(conversation continued)" + # The original assistant should follow. + assert non_system[1]["role"] == "assistant" + + def test_leading_assistant_with_tool_calls_not_patched(self): + """An assistant message with tool_calls at the start is left as-is + because tool messages will follow and some providers accept this.""" + msgs = [ + {"role": "system", "content": "sys"}, + {"role": "assistant", "content": None, "tool_calls": [ + {"id": "tc_1", "type": "function", "function": {"name": "ls", "arguments": "{}"}} + ]}, + {"role": "tool", "tool_call_id": "tc_1", "content": "result"}, + ] + result = LLMProvider._enforce_role_alternation(msgs) + non_system = [m for m in result if m["role"] != "system"] + # The assistant has tool_calls so it should NOT be patched. + assert non_system[0]["role"] == "assistant" + assert non_system[0].get("tool_calls") is not None + + def test_user_after_system_not_patched(self): + """Normal system→user sequence is not modified.""" + msgs = [ + {"role": "system", "content": "sys"}, + {"role": "user", "content": "hello"}, + {"role": "assistant", "content": "hi"}, + ] + result = LLMProvider._enforce_role_alternation(msgs) + assert result[1]["role"] == "user" + assert result[1]["content"] == "hello"