From 4d0cec9a7d99e35d66064fa97b0eeded0a3e3cf6 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Wed, 9 Sep 2026 11:07:32 +0530 Subject: [PATCH] fix(agent): the pre-API-call /steer drain also stops smearing the persisted tool row Second site of the same bug class #104444 fixes in apply_pending_steer_to_tool_results: _inject_steer_into_newest_tool_result (the drain that runs when a /steer lands during an API call) mutated the newest role:tool row in place. That row was already flushed append-only, so the replayed history diverged from the live request bytes at the injection point and broke the prompt cache exactly like the post-batch path. Deliver it the same way: a standalone user row inserted right after the newest tool result (not yet persisted, so the next flush writes it to the transcript). Restash when there is no tool row yet, unchanged. Stale comments claiming steer lands "in the newest tool result" and agent/AGENTS.md's alternation rule now describe the real shape. --- agent/AGENTS.md | 5 ++++- agent/turn_iteration_prep.py | 41 ++++++++++++++--------------------- tests/run_agent/test_steer.py | 28 +++++++++++------------- 3 files changed, 33 insertions(+), 41 deletions(-) diff --git a/agent/AGENTS.md b/agent/AGENTS.md index e1f162fa77..edd8072bcc 100644 --- a/agent/AGENTS.md +++ b/agent/AGENTS.md @@ -59,7 +59,10 @@ Adding one: register in that table (no `if name == ...` chain); `tools/todo_tool commands (`agent/skill_commands.py`) inject as a user message; subdirectory `AGENTS.md` hints (`agent/subdirectory_hints.py`) append to the tool result (head+tail truncated past `_MAX_HINT_CHARS = 32_000`, with a warning). - **Strict role alternation.** Never two same-role messages in a row; never a synthetic user - message injected mid-loop. Cron deliveries live in their own session for this reason. + message injected mid-loop. The one exception is `/steer`, delivered as a standalone user row + after a tool result (`assistant(tool_calls) → tool → user` is legal on every provider path) — + never smeared onto the already-persisted tool row, which append-only persistence would leave + divergent from the live request. Cron deliveries live in their own session for this reason. - **Context files** (`agent/prompt_builder.py`) load from the CWD only at startup and are capped (`CONTEXT_FILE_MAX_CHARS` / dynamic cap from the context window / `context_file_max_chars`). Never load an install-tree `AGENTS.md` as project context (PR #64611); subdirectory hints reject diff --git a/agent/turn_iteration_prep.py b/agent/turn_iteration_prep.py index a48299ea3d..189c0c6b8f 100644 --- a/agent/turn_iteration_prep.py +++ b/agent/turn_iteration_prep.py @@ -1,7 +1,7 @@ """Outer-iteration bookkeeping for the conversation turn loop, in call order: ``begin_iteration`` (pending redirect, interrupt / review-budget / iteration-budget exits), -``prepare_iteration`` (``agent:step`` callback, skill-nudge counter, pre-API ``/steer`` drain -into the newest tool result — never a user message —, run-budget wrap-up notice, tool_call +``prepare_iteration`` (``agent:step`` callback, skill-nudge counter, pre-API ``/steer`` drain as a +standalone user row after the newest tool result, run-budget wrap-up notice, tool_call argument sanitization, interrupt-scaffold ghost-row drop, role-alternation repair), ``announce_api_call`` (verbose summary / quiet spinner) and, after the retry loop, ``apply_retry_restarts`` (consumes the ``TurnRetryState`` restart flags). Nothing here @@ -96,7 +96,7 @@ def prepare_iteration( agent: Any, *, messages: Any, api_call_count: Any, user_message: Any = None, current_turn_user_idx: Any = None, ) -> IterationPrep: """Prepare ``messages`` for this iteration in the original order. Every mutation here is - cache-safe by construction: steer text lands in the newest tool result, the ghost-row + cache-safe by construction: steer text is appended as a new (not yet persisted) user row, the ghost-row filter only drops hidden scaffold placeholders, and repair runs BEFORE the request build.""" from agent.conversation_loop import ( _INTERRUPT_SCAFFOLD_MARKER, _maybe_inject_run_budget_wrapup @@ -128,18 +128,20 @@ def prepare_iteration( except Exception: logger.debug("Nous key pre-expiry adoption failed", exc_info=True) - # Drain a /steer sent during the last API call into the newest tool message so - # it lands THIS iteration. Never put in a user message (breaks alternation). + # Drain a /steer sent during the last API call so it lands THIS iteration. Delivered as a + # standalone user row after the newest tool result (never smeared onto the tool row: that + # row is already persisted append-only, so replay would diverge from the live request and + # break the prompt cache — same contract as apply_pending_steer_to_tool_results). _pre_api_steer = agent._drain_pending_steer() if _pre_api_steer: - _inject_steer_into_newest_tool_result(agent, messages, _pre_api_steer) + _inject_steer_after_newest_tool_result(agent, messages, _pre_api_steer) - # One-shot run-budget wrap-up notice at 80% of agent.run_budget_seconds, via the - # same cache-safe channel as /steer (newest tool result); off with no budget. + # One-shot run-budget wrap-up notice at 80% of agent.run_budget_seconds, appended to the + # newest tool result; off with no budget. if getattr(agent, "run_budget_seconds", None): _maybe_inject_run_budget_wrapup(agent, messages) - # Use the same cache-safe channel as /steer; never add a synthetic user/system row. + # Appended to the newest tool result; never a synthetic user/system row. _maybe_inject_iteration_budget_warning(agent, messages) request_logger = getattr(agent, "logger", None) or logger # same name as the origin module @@ -227,26 +229,15 @@ def _previous_tool_round(messages: Any) -> list: return [] -def _inject_steer_into_newest_tool_result(agent: Any, messages: Any, steer_text: str) -> None: - """Append the steer marker to the newest tool message; with no tool message, put the - text back so the post-tool-execution drain delivers it later.""" +def _inject_steer_after_newest_tool_result(agent: Any, messages: Any, steer_text: str) -> None: + """Append the steer marker as a standalone user row after the newest tool message; with no + tool message, put the text back so the post-tool-execution drain delivers it later.""" 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(steer_text) - existing = _sm.get("content", "") - if isinstance(existing, str): - _sm["content"] = existing + marker - else: - # Multimodal content blocks — append a text block. - with suppress(Exception): - blocks = list(existing) if existing else [] - blocks.append({"type": "text", "text": marker}) - _sm["content"] = blocks - logger.debug( - "Pre-API-call steer drain: injected into tool msg at index %d", _si - ) + messages.insert(_si + 1, {"role": "user", "content": format_steer_marker(steer_text)}) + logger.debug("Pre-API-call steer drain: appended user row after tool msg at index %d", _si) return _lock = getattr(agent, "_pending_steer_lock", None) if _lock is not None: diff --git a/tests/run_agent/test_steer.py b/tests/run_agent/test_steer.py index 929914fea4..144ae4a811 100644 --- a/tests/run_agent/test_steer.py +++ b/tests/run_agent/test_steer.py @@ -621,29 +621,27 @@ 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): - """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.""" + def test_pre_api_drain_appends_user_row_and_leaves_tool_row_untouched(self): + """A steer pending when the loop builds api_messages lands THIS iteration as a + standalone user row after the newest tool result; the (already persisted, append-only) + tool row is byte-identical afterwards so replay cannot diverge from the live request.""" + from agent.turn_iteration_prep import _inject_steer_after_newest_tool_result + agent = _bare_agent() - # Simulate messages after a tool batch completed + tool_row = {"role": "tool", "content": "output here", "tool_call_id": "tc1"} + before = dict(tool_row) messages = [ {"role": "user", "content": "do something"}, {"role": "assistant", "content": "ok", "tool_calls": [ {"id": "tc1", "function": {"name": "terminal", "arguments": "{}"}} ]}, - {"role": "tool", "content": "output here", "tool_call_id": "tc1"}, + tool_row, ] - # Steer arrives during API call (set after tool execution) agent.steer("focus on error handling") - # 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) - for _si in range(len(messages) - 1, -1, -1): - if messages[_si].get("role") == "tool": - messages[_si]["content"] += format_steer_marker(_pre_api_steer) - break + _inject_steer_after_newest_tool_result(agent, messages, agent._drain_pending_steer()) + assert tool_row == before + assert messages[-2] is tool_row + assert messages[-1]["role"] == "user" assert STEER_MARKER_OPEN in messages[-1]["content"] assert "focus on error handling" in messages[-1]["content"] assert agent._pending_steer is None