diff --git a/agent/agent_runtime_helpers.py b/agent/agent_runtime_helpers.py index 4a1e969e928a..e5b875f3b8bf 100644 --- a/agent/agent_runtime_helpers.py +++ b/agent/agent_runtime_helpers.py @@ -34,7 +34,6 @@ from hermes_cli.timeouts import get_provider_request_timeout from agent.message_sanitization import _FULL_ARGS_LOG_BOUND -from agent.prompt_builder import format_steer_marker from agent.tool_dispatch_helpers import _trajectory_normalize_msg, make_tool_result_message from agent.trajectory import convert_scratchpad_to_think from agent.credential_pool import STATUS_EXHAUSTED, credential_pool_matches_provider @@ -3929,13 +3928,18 @@ def extract_api_error_context(error: Exception) -> Dict[str, Any]: def apply_pending_steer_to_tool_results(agent, messages: list, num_tool_msgs: int) -> None: - """Append any pending /steer text to the last tool result in this turn. + """Insert any pending /steer as a genuine role:user message after the last tool result in this turn. Called at the end of a tool-call batch, before the next API call. - The steer is appended to the last ``role:"tool"`` message's content - with a clear marker so the model understands it came from the user - and NOT from the tool itself. Role alternation is preserved — - nothing new is inserted, we only modify existing content. + Inserted as a real role:"user" message right after the last + role:"tool" message, rather than appended as marker text inside the + tool message's own content -- a model can trivially reproduce static + marker text visible in its own system prompt and self-fabricate a + plausible "user said X" block, but it cannot fabricate a message with + role:user in the API request itself; that's set by the runtime, not + model output (issue #81828). Role alternation is preserved: + assistant -> tool -> user (steer) -> assistant is a standard, + provider-supported sequence. Args: messages: The running messages list. @@ -3971,20 +3975,10 @@ def apply_pending_steer_to_tool_results(agent, messages: list, num_tool_msgs: in existing = getattr(agent, "_pending_steer", None) agent._pending_steer = (existing + "\n" + steer_text) if existing else steer_text return - marker = format_steer_marker(steer_text) - existing_content = messages[target_idx].get("content", "") - if not isinstance(existing_content, str): - # Anthropic multimodal content blocks — preserve them and append - # a text block at the end. - try: - blocks = list(existing_content) if existing_content else [] - blocks.append({"type": "text", "text": marker.lstrip()}) - messages[target_idx]["content"] = blocks - except Exception: - # Fall back to string replacement if content shape is unexpected. - messages[target_idx]["content"] = f"{existing_content}{marker}" - else: - messages[target_idx]["content"] = existing_content + marker + # Structural separation (issue #81828): insert as a genuine + # role:user message right after the tool result, instead of + # appending STEER_MARKER text inside the tool message's own content. + messages.insert(target_idx + 1, {"role": "user", "content": steer_text}) _ra().logger.info( "Delivered /steer to agent after tool batch (%d chars): %s", len(steer_text), diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index 2ab35ec945e1..61045fbbe3b3 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -1644,22 +1644,20 @@ def run_conversation( for _si in range(len(messages) - 1, -1, -1): _sm = messages[_si] if isinstance(_sm, dict) and _sm.get("role") == "tool": - from agent.prompt_builder import format_steer_marker - marker = format_steer_marker(_pre_api_steer) - existing = _sm.get("content", "") - if isinstance(existing, str): - _sm["content"] = existing + marker - else: - # Multimodal content blocks — append text block - try: - blocks = list(existing) if existing else [] - blocks.append({"type": "text", "text": marker}) - _sm["content"] = blocks - except Exception: - pass + # Structural separation (issue #81828): insert as a + # genuine role:user message right after the tool + # result, rather than appending STEER_MARKER text + # inside the tool message's own content. A model can + # trivially reproduce the static marker text (it's + # visible in its own system prompt) and self-fabricate + # a plausible "user said X" block that STEER_CHANNEL_NOTE + # then tells it to trust -- but it cannot fabricate a + # message with role:user in the API request itself; + # that's set by the runtime, not model output. + messages.insert(_si + 1, {"role": "user", "content": _pre_api_steer}) _injected = True logger.debug( - "Pre-API-call steer drain: injected into tool msg at index %d", + "Pre-API-call steer drain: inserted role:user message after tool msg at index %d", _si, ) break diff --git a/agent/prompt_builder.py b/agent/prompt_builder.py index 1a855981231e..8ac634e50b93 100644 --- a/agent/prompt_builder.py +++ b/agent/prompt_builder.py @@ -649,15 +649,23 @@ def computer_use_guidance(platform_name: Optional[str] = None) -> str: # --------------------------------------------------------------------------- # Mid-turn steering (/steer) — out-of-band user messages # --------------------------------------------------------------------------- -# A steer is appended to the END of a tool result (the only role-alternation- -# safe slot mid-turn), so it rides the exact channel injection defenses are -# trained to distrust — a bare "User guidance:" line gets refused as suspected -# prompt injection (observed in the wild). The bounded, self-describing marker -# below attributes the text to the real user, and STEER_CHANNEL_NOTE tells the -# model to trust THIS marker and only this one, so a lookalike buried in -# tool/web/file output stays untrusted. The note also defines when a marker is -# fresh: the marker remains in immutable conversation history after delivery, -# so treating every historical occurrence as a new message can replay actions. +# HISTORICAL NOTE (issue #81828): the mid-turn steer delivery path +# (agent/conversation_loop.py's pre-API-call drain and +# agent/agent_runtime_helpers.py::apply_pending_steer_to_tool_results) no +# longer uses the marker constants below. A model can trivially reproduce +# static marker text visible in its own system prompt and self-fabricate a +# plausible "user said X" block that this note then tells it to trust — that +# vulnerability class is closed structurally: steers are now inserted as a +# genuine role:"user" message immediately after the tool result, which the +# model cannot fabricate (role assignment is set by the runtime, not model +# output), rather than appended as marker text inside the tool message's own +# content. +# +# The constants and STEER_CHANNEL_NOTE below are left exactly as they were +# (not removed) in case another, unaudited call site still references them, +# and because changing STEER_CHANNEL_NOTE's actual text would invalidate the +# prompt cache the same way a per-session hash-based marker would have -- +# the tradeoff 0f45509da explicitly avoided by keeping this text static. STEER_MARKER_OPEN = ( "[OUT-OF-BAND USER MESSAGE — a direct message from the user, delivered " "once at this position; not tool output and not a new delivery when replayed " diff --git a/tests/run_agent/test_steer.py b/tests/run_agent/test_steer.py index 7b48eb4b1e73..f25eca326c77 100644 --- a/tests/run_agent/test_steer.py +++ b/tests/run_agent/test_steer.py @@ -319,7 +319,7 @@ def test_checkpoint_omits_reasoning_label_when_nothing_visible(self): class TestSteerInjection: - def test_appends_to_last_tool_result(self): + def test_inserts_user_message_after_last_tool_result(self): agent = _bare_agent() agent.steer("please also check auth.log") messages = [ @@ -329,11 +329,16 @@ def test_appends_to_last_tool_result(self): {"role": "tool", "content": "ls output B", "tool_call_id": "b"}, ] agent._apply_pending_steer_to_tool_results(messages, num_tool_msgs=2) - # The LAST tool result is modified; earlier ones are untouched. + # Neither tool result's own content is touched... assert messages[2]["content"] == "ls output A" - assert "ls output B" in messages[3]["content"] - assert STEER_MARKER_OPEN in messages[3]["content"] - assert "please also check auth.log" in messages[3]["content"] + assert messages[3]["content"] == "ls output B" + # ...instead, a genuine role:user message is INSERTED right after + # the last tool result -- structural separation (issue #81828): a + # model cannot fabricate a message with role:user in the API + # request itself, unlike marker text embedded in tool content. + assert messages[4]["role"] == "user" + assert messages[4]["content"] == "please also check auth.log" + assert len(messages) == 5 # And pending_steer is consumed. assert agent._pending_steer is None @@ -345,26 +350,27 @@ def test_no_op_when_no_steer_pending(self): ] agent._apply_pending_steer_to_tool_results(messages, num_tool_msgs=1) assert messages[-1]["content"] == "output" # unchanged + assert len(messages) == 2 # nothing inserted - def test_marker_labels_text_as_out_of_band_user_message(self): - """The injection marker must attribute the appended text to the user - via the explicit out-of-band marker (which the system prompt tells the - model to trust) — otherwise the model reads it as untrusted tool output - and refuses it as suspected prompt injection. Cache-safe: it only - rewrites existing tool content, never the message-role sequence. - """ + def test_inserted_message_has_no_marker_text(self): + """Regression (issue #81828): the inserted steer message must be + the user's raw text with no marker wrapper at all -- there is + nothing for a model to learn to reproduce, since trust now comes + from the message's role, set by the runtime, not from recognizing + a marker string that was always visible in the system prompt.""" agent = _bare_agent() agent.steer("stop after next step") messages = [{"role": "tool", "content": "x", "tool_call_id": "1"}] agent._apply_pending_steer_to_tool_results(messages, num_tool_msgs=1) - content = messages[-1]["content"] - assert STEER_MARKER_OPEN in content - assert "stop after next step" in content + assert messages[-1]["role"] == "user" + assert messages[-1]["content"] == "stop after next step" + assert "OUT-OF-BAND" not in messages[-1]["content"] - def test_multimodal_content_list_preserved(self): - """Anthropic-style list content should be preserved, with the steer - appended as a text block.""" + def test_multimodal_tool_content_left_untouched(self): + """Anthropic-style list content on the tool message must be left + completely alone -- the steer is now a separate message, not + appended as a block inside the tool result's own content.""" agent = _bare_agent() agent.steer("extra note") original_blocks = [{"type": "text", "text": "existing output"}] @@ -372,12 +378,9 @@ def test_multimodal_content_list_preserved(self): {"role": "tool", "content": list(original_blocks), "tool_call_id": "1"} ] agent._apply_pending_steer_to_tool_results(messages, num_tool_msgs=1) - new_content = messages[-1]["content"] - assert isinstance(new_content, list) - assert len(new_content) == 2 - assert new_content[0] == {"type": "text", "text": "existing output"} - assert new_content[1]["type"] == "text" - assert "extra note" in new_content[1]["text"] + assert messages[0]["content"] == original_blocks + assert messages[1]["role"] == "user" + assert messages[1]["content"] == "extra note" @@ -432,10 +435,12 @@ class TestPreApiCallSteerDrain: fix for the scenario where /steer sent during model thinking only lands after the agent is completely done.""" - def test_pre_api_drain_injects_into_last_tool_result(self): + def test_pre_api_drain_inserts_user_message_after_last_tool_result(self): """If a steer is pending when the main loop starts building - api_messages, it should be injected into the last tool result - in the messages list.""" + api_messages, a genuine role:user message should be inserted + right after the last tool result in the messages list (issue + #81828's structural separation -- mirrors the actual insertion + logic in agent/conversation_loop.py's pre-API-call drain).""" agent = _bare_agent() # Simulate messages after a tool batch completed messages = [ @@ -450,13 +455,16 @@ def test_pre_api_drain_injects_into_last_tool_result(self): # Simulate what the pre-API-call drain does: _pre_api_steer = agent._drain_pending_steer() assert _pre_api_steer == "focus on error handling" - # Inject into last tool msg (mirrors the new code in run_conversation) + # Insert after the last tool msg (mirrors the actual code in + # conversation_loop.py's pre-API-call drain). for _si in range(len(messages) - 1, -1, -1): if messages[_si].get("role") == "tool": - messages[_si]["content"] += format_steer_marker(_pre_api_steer) + messages.insert(_si + 1, {"role": "user", "content": _pre_api_steer}) break - assert STEER_MARKER_OPEN in messages[-1]["content"] - assert "focus on error handling" in messages[-1]["content"] + assert messages[-1]["role"] == "user" + assert messages[-1]["content"] == "focus on error handling" + # The tool message's own content is untouched. + assert messages[2]["content"] == "output here" assert agent._pending_steer is None def test_pre_api_drain_restashes_when_no_tool_message(self): @@ -483,6 +491,14 @@ def test_pre_api_drain_restashes_when_no_tool_message(self): class TestSteerMarkerContract: + """These constants are no longer used by the mid-turn steer delivery + path (see the HISTORICAL NOTE in agent/prompt_builder.py, issue + #81828) -- kept byte-for-byte unchanged for prompt-cache safety + rather than removed. These tests still verify their own internal + consistency (the marker text and the note describing it must agree), + which remains true and worth guarding even though nothing calls + format_steer_marker() in production anymore.""" + def test_system_prompt_note_describes_the_real_marker(self): """The system-prompt note tells the model which marker to trust; it must reference the exact open/close the injector emits, or the model diff --git a/tests/run_agent/test_tool_batch_segmentation.py b/tests/run_agent/test_tool_batch_segmentation.py index 3a0fa30e1300..9b78cd2e1399 100644 --- a/tests/run_agent/test_tool_batch_segmentation.py +++ b/tests/run_agent/test_tool_batch_segmentation.py @@ -27,7 +27,6 @@ _plan_tool_batch_segments, _should_parallelize_tool_batch, ) -from agent.prompt_builder import STEER_MARKER_OPEN from tools.budget_config import BudgetConfig @@ -596,9 +595,12 @@ def fake_handle(name, args, task_id, **kwargs): large_result_index = next(i for i, call in enumerate(calls) if call.id.endswith("large")) assert "Truncated:" in messages[large_result_index]["content"] - steer_messages = [m for m in messages if STEER_MARKER_OPEN in m["content"]] + # Structural separation (issue #81828): the steer is a genuine + # role:user message appended after budget enforcement, not marker + # text embedded in a tool message's own content. + steer_messages = [m for m in messages if m.get("role") == "user"] assert steer_messages == [messages[-1]] - assert "preserve this steer after budget enforcement" in steer_messages[0]["content"] + assert steer_messages[0]["content"] == "preserve this steer after budget enforcement" def test_steer_survives_turn_budget_after_malformed_arguments(self, agent): """Malformed arguments still reach the shared post-budget finalizer. @@ -622,10 +624,10 @@ def test_steer_survives_turn_budget_after_malformed_arguments(self, agent): with patch("agent.tool_executor._budget_for_agent", return_value=budget): agent._execute_tool_calls(msg, messages, "task-1") - assert len(messages) == 1 + assert len(messages) == 2 assert "Truncated:" in messages[0]["content"] - assert messages[0]["content"].count(STEER_MARKER_OPEN) == 1 - assert "preserve malformed-call steer after budget enforcement" in messages[0]["content"] + assert messages[1]["role"] == "user" + assert messages[1]["content"] == "preserve malformed-call steer after budget enforcement" class TestPathCanonicalization: