From cf6b2cc1c48b6455071c24273c2f22e4c09d9c37 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 20 Aug 2026 11:38:58 +0100 Subject: [PATCH] feat: remove trialing system msg --- application/llm/handlers/base.py | 30 ++- .../handlers/test_finalize_message_role.py | 211 ++++++++++++++++++ 2 files changed, 234 insertions(+), 7 deletions(-) create mode 100644 tests/llm/handlers/test_finalize_message_role.py diff --git a/application/llm/handlers/base.py b/application/llm/handlers/base.py index ef379823..b682c0bf 100644 --- a/application/llm/handlers/base.py +++ b/application/llm/handlers/base.py @@ -1264,14 +1264,21 @@ class LLMHandler(ABC): return "" # Cap reached: force one final tool-less call so the stream - # always ends with content rather than cutting off. + # always ends with content rather than cutting off. The + # instruction rides as a USER turn, not a trailing system + # message: strict chat templates (Cloudflare's qwen, prod + # 2026-08-18) reject any request whose system message is not + # at position 0 — 400 "System message must be at the + # beginning" — killing the primary on the very turn meant to + # wrap up. ``tools=None`` below enforces the cap regardless + # of the role. if iteration >= MAX_TOOL_ITERATIONS: logger.warning( "agent tool loop hit cap (%d); forcing finalize", MAX_TOOL_ITERATIONS, ) messages.append( - {"role": "system", "content": _FINALIZE_INSTRUCTION}, + {"role": "user", "content": _FINALIZE_INSTRUCTION}, ) response = agent.llm.gen( model=getattr(agent.llm, "model_id", None) or agent.model_id, @@ -1458,7 +1465,7 @@ class LLMHandler(ABC): # Mirror the finalize-round contract at the bottom of # this method: when the cap or context limit already # forced ``tools=None`` (with an accompanying "no more - # tools" system message), the recovery must not reopen + # tools" user-turn instruction), the recovery must not reopen # tools, or the model could run one extra tool call past # the cap. ``_iteration >= MAX_TOOL_ITERATIONS`` catches # the finalize round. @@ -1526,11 +1533,13 @@ class LLMHandler(ABC): next_iteration = _iteration + 1 cap_reached = next_iteration >= MAX_TOOL_ITERATIONS - # Check if context limit was reached during tool execution + # Check if context limit was reached during tool execution. + # Injected as a USER turn, not a trailing system message — see the + # cap branch below for why (strict chat templates 400 on + # non-leading system messages). if hasattr(agent, 'context_limit_reached') and agent.context_limit_reached: - # Add system message warning about context limit messages.append({ - "role": "system", + "role": "user", "content": ( "WARNING: Context window limit has been reached. " "Please provide a final response to the user without making additional tool calls. " @@ -1539,12 +1548,19 @@ class LLMHandler(ABC): }) logger.info("Context limit reached - instructing agent to wrap up") elif cap_reached: + # USER turn, not a trailing system message: Cloudflare's qwen + # (vLLM-style template) 400s any request whose system message + # is not at position 0 ("System message must be at the + # beginning", prod 2026-08-18) — the finalize turn killed the + # primary and the answer came from the fallback. Probed + # 2026-08-20: qwen accepts the identical text as a user turn. + # ``tools=None`` below enforces the cap regardless of role. logger.warning( "agent tool loop hit cap (%d); forcing finalize", MAX_TOOL_ITERATIONS, ) messages.append( - {"role": "system", "content": _FINALIZE_INSTRUCTION}, + {"role": "user", "content": _FINALIZE_INSTRUCTION}, ) # Hard pre-send gate: tool results appended this round may have diff --git a/tests/llm/handlers/test_finalize_message_role.py b/tests/llm/handlers/test_finalize_message_role.py new file mode 100644 index 00000000..fa86820e --- /dev/null +++ b/tests/llm/handlers/test_finalize_message_role.py @@ -0,0 +1,211 @@ +"""#49 regression: cap/context-limit "wrap up" instructions must be USER turns. + +Prod 2026-08-18: ``@cf/qwen/qwen3.8-27b`` (vLLM-style chat template) rejects +any request whose system message is not at position 0 — 400 "System message +must be at the beginning." The tool-loop-cap and context-limit paths appended +their finalize instruction as a TRAILING system message, so the one turn whose +job was to wrap up gracefully killed the primary model and the user got a +fallback answer instead. Live-probed 2026-08-20: qwen accepts the identical +instruction as a ``user`` turn, and every other served model accepts both +forms — so the instruction rides as a user turn. + +These tests drive the real handler flows (streaming cap, streaming +context-limit, non-streaming cap) with scripted LLMResponse objects and pin, +on the messages array actually SENT to the provider for the finalize round +(``handle_tool_calls`` works on a copy, so the caller's list is not mutated): +the injected instruction is a ``user`` message, no ``system`` message ever +appears past position 0, and the finalize round still sends ``tools=None``. +""" + +from types import SimpleNamespace +from typing import Any, Dict, Generator + +from application.llm.handlers.base import ( + _FINALIZE_INSTRUCTION, + MAX_TOOL_ITERATIONS, + LLMHandler, + LLMResponse, + ToolCall, +) + + +class _ScriptHandler(LLMHandler): + """Handler whose streams yield ready-made ``LLMResponse`` objects (or str + content deltas). ``agent.llm._responding_provider`` is None, so + ``_parse_for_response`` falls back to this ``parse_response``.""" + + def parse_response(self, response: Any) -> LLMResponse: + return response + + def create_tool_message(self, tool_call: ToolCall, result: Any) -> Dict: + return {"role": "tool", "tool_call_id": tool_call.id, "content": str(result)} + + def _iterate_stream(self, response: Any) -> Generator: + yield from response + + +def _tool_call_response() -> LLMResponse: + return LLMResponse( + content="", + tool_calls=[ToolCall(id="c1", name="dummy_tool", arguments={}, index=None)], + finish_reason="tool_calls", + raw_response=None, + ) + + +def _stop_response() -> LLMResponse: + return LLMResponse(content="", tool_calls=[], finish_reason="stop", raw_response=None) + + +class _FakeLLM: + def __init__(self, next_stream=None, gen_responses=None): + self.model_id = "test-model" + self._responding_provider = None # route parsing to _ScriptHandler + self._fallback_llm = None + self._stream_reached_finish = False + self._next_stream = next_stream or [] + self._gen_responses = list(gen_responses or []) + self.gen_stream_tools_seen = [] + self.gen_tools_seen = [] + self.gen_stream_messages_seen = [] + self.gen_messages_seen = [] + + def gen_stream(self, model, messages, tools=None, **kwargs): + self.gen_stream_tools_seen.append(tools) + self.gen_stream_messages_seen.append(list(messages)) + return iter(self._next_stream) + + def gen(self, model, messages, tools=None, **kwargs): + self.gen_tools_seen.append(tools) + self.gen_messages_seen.append(list(messages)) + return self._gen_responses.pop(0) + + +def _execute_tool_action(tools_dict, call): + """Generator contract of ``agent._execute_tool_action``: may yield + progress events; returns (tool_response, call_id).""" + if False: # pragma: no cover - makes this a generator + yield None + return ("tool ran fine", call.id) + + +def _fake_agent(llm): + return SimpleNamespace( + llm=llm, + model_id="test-model", + tools=None, + tool_executor=SimpleNamespace( + check_pause=lambda tools_dict, call, llm_class: None + ), + _execute_tool_action=_execute_tool_action, + ) + + +def _drain(gen): + events = [] + while True: + try: + events.append(next(gen)) + except StopIteration as e: + return events, e.value + + +def _injected(messages, needle): + return [m for m in messages if needle in str(m.get("content", ""))] + + +def _no_system_past_position_zero(messages): + return all(m.get("role") != "system" for m in messages[1:]) + + +class TestFinalizeMessageRole: + def test_streaming_cap_injects_user_turn_not_trailing_system(self): + """Site: handle_streaming cap path. One tool round at _iteration=cap-1 + forces the finalize; the instruction must be a user turn.""" + llm = _FakeLLM(next_stream=["final answer", _stop_response()]) + agent = _fake_agent(llm) + handler = _ScriptHandler() + messages = [ + {"role": "system", "content": "base prompt"}, + {"role": "user", "content": "question"}, + ] + + events, _ = _drain( + handler.handle_streaming( + agent, + iter([_tool_call_response()]), + {}, + messages, + _iteration=MAX_TOOL_ITERATIONS - 1, + ) + ) + + assert "".join(e for e in events if isinstance(e, str)) == "final answer" + sent = llm.gen_stream_messages_seen[-1] + injected = _injected(sent, _FINALIZE_INSTRUCTION) + assert len(injected) == 1 + assert injected[0]["role"] == "user" + assert _no_system_past_position_zero(sent) + # The finalize round must still close the tool loop. + assert llm.gen_stream_tools_seen == [None] + + def test_streaming_context_limit_injects_user_turn(self): + """Site: handle_streaming context-limit path (its own inline wording).""" + llm = _FakeLLM(next_stream=["wrapped up", _stop_response()]) + agent = _fake_agent(llm) + agent.context_limit_reached = True + handler = _ScriptHandler() + messages = [ + {"role": "system", "content": "base prompt"}, + {"role": "user", "content": "question"}, + ] + + _drain( + handler.handle_streaming( + agent, iter([_tool_call_response()]), {}, messages, _iteration=1 + ) + ) + + sent = llm.gen_stream_messages_seen[-1] + injected = _injected(sent, "Context window limit has been reached") + assert len(injected) == 1 + assert injected[0]["role"] == "user" + assert _no_system_past_position_zero(sent) + assert llm.gen_stream_tools_seen == [None] + + def test_non_streaming_cap_injects_user_turn(self): + """Site: handle_non_streaming cap path — 25 tool rounds then the + forced tool-less finalize call.""" + llm = _FakeLLM( + gen_responses=( + [_tool_call_response()] * (MAX_TOOL_ITERATIONS - 1) + + [ + LLMResponse( + content="capped answer", + tool_calls=[], + finish_reason="stop", + raw_response=None, + ) + ] + ) + ) + agent = _fake_agent(llm) + handler = _ScriptHandler() + messages = [ + {"role": "system", "content": "base prompt"}, + {"role": "user", "content": "question"}, + ] + + _, result = _drain( + handler.handle_non_streaming( + agent, _tool_call_response(), {}, messages + ) + ) + + assert result == "capped answer" + sent = llm.gen_messages_seen[-1] + injected = _injected(sent, _FINALIZE_INSTRUCTION) + assert len(injected) == 1 + assert injected[0]["role"] == "user" + assert _no_system_past_position_zero(sent) + assert llm.gen_tools_seen[-1] is None # forced tool-less finalize