From 5a0f51325cd83e64efc015a1455e88c727ba10fe Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Fri, 24 Jul 2026 12:22:27 -0700 Subject: [PATCH] fix(agent): make dropped tool-call nudge pair ephemeral scaffolding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up for the dropped tool-call recovery (#69630): the re-prompt pair was tagged _dropped_toolcall_nudge, but that marker was not part of the ephemeral-scaffolding contract. _persist_session / _flush_messages_to_session_db would therefore write the synthetic 'issue the actual tool call now' user message (and the narration-only interim assistant turn) as real transcript rows — a resumed session could replay the internal retry instruction as user-authored context and prompt unsolicited tool use. - Add _dropped_toolcall_nudge to _EPHEMERAL_SCAFFOLDING_FLAGS (run_agent.py) so both SQLite and JSON persistence skip the pair. - Add it to _SYNTHETIC_USER_FLAGS (conversation_compression.py) so the compressor never treats the nudge as human intent. - Flag the interim assistant half of the pair too, and include the marker in the finalization scaffolding pop so a genuine turn end strips the pair from the live transcript (mirrors the _empty_recovery_synthetic pattern). - Regression tests: flagged messages classify as ephemeral, the returned transcript contains no scaffolding, and the turn tail stays on the real assistant answer. --- agent/conversation_compression.py | 1 + agent/conversation_loop.py | 12 ++++ run_agent.py | 6 ++ .../test_dropped_tool_call_recovery.py | 65 +++++++++++++++++++ 4 files changed, 84 insertions(+) diff --git a/agent/conversation_compression.py b/agent/conversation_compression.py index 88a5f4d7ab89..2a2ec1b0b4d1 100644 --- a/agent/conversation_compression.py +++ b/agent/conversation_compression.py @@ -892,6 +892,7 @@ _SYNTHETIC_USER_FLAGS = ( "_empty_recovery_synthetic", "_verification_stop_synthetic", "_pre_verify_synthetic", + "_dropped_toolcall_nudge", ) diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index 8bd4f2f477dd..cfdecd4275cd 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -6248,6 +6248,17 @@ def run_conversation( "↻ Model signaled a tool call but sent none — " f"re-prompting ({agent._dropped_toolcall_retries}/3)" ) + # Both halves of the re-prompt pair are ephemeral recovery + # scaffolding (mirrors the empty-response nudge pattern): + # the interim narration-only assistant turn exists solely to + # keep role alternation valid for the nudge, and the nudge + # exists solely to drive the retry. Flag both so the + # persistence layer never writes them to the durable + # transcript and the finalization pop below can strip an + # unanswered tail pair. A recovered (answered) pair stays + # buried mid-list in live memory but is skipped by the + # flush regardless of position. + final_msg["_dropped_toolcall_nudge"] = True messages.append(final_msg) messages.append({ "role": "user", @@ -6279,6 +6290,7 @@ def run_conversation( messages[-1].get("_thinking_prefill") or messages[-1].get("_empty_recovery_synthetic") or messages[-1].get("_empty_terminal_sentinel") + or messages[-1].get("_dropped_toolcall_nudge") ) ): messages.pop() diff --git a/run_agent.py b/run_agent.py index e4cd9f54e8ce..c73f167bb902 100644 --- a/run_agent.py +++ b/run_agent.py @@ -239,6 +239,12 @@ _EPHEMERAL_SCAFFOLDING_FLAGS = ( "_pre_verify_synthetic", # kanban worker stop-guard: narrated exit without kanban_complete/block "_kanban_stop_synthetic", + # dropped tool-call re-prompt pair (finish_reason=tool_calls with an + # empty tool_calls array): the interim narration-only assistant turn + # and the "issue the actual tool call now" user nudge exist only to + # drive the bounded retry. Persisting them would replay the internal + # retry instruction as user-authored context on resume. + "_dropped_toolcall_nudge", ) diff --git a/tests/run_agent/test_dropped_tool_call_recovery.py b/tests/run_agent/test_dropped_tool_call_recovery.py index f1f6d5beaaf8..d540596a61eb 100644 --- a/tests/run_agent/test_dropped_tool_call_recovery.py +++ b/tests/run_agent/test_dropped_tool_call_recovery.py @@ -172,3 +172,68 @@ class TestDroppedToolCallRecovery: "Consecutive dropped tool calls must be bounded (no infinite loop)." ) assert result is not None + + def test_nudge_pair_is_ephemeral_scaffolding(self, loop_agent): + """The re-prompt pair (interim assistant turn + synthetic user nudge) + must be flagged as ephemeral scaffolding so persistence never writes + it to the durable transcript — a resumed session must not replay the + internal "issue the actual tool call now" instruction as user-authored + context (#69630 review follow-up).""" + from run_agent import _is_ephemeral_scaffolding + from tests.run_agent.test_run_agent import _mock_response + + loop_agent.client.chat.completions.create.side_effect = [ + _dropped_tool_call_response("Let me verify the PR."), + _mock_response(content="All checks pass. Approved.", finish_reason="stop"), + ] + + with ( + patch.object(loop_agent, "_persist_session"), + patch.object(loop_agent, "_save_trajectory"), + patch.object(loop_agent, "_cleanup_task_resources"), + ): + result = loop_agent.run_conversation("review the PR") + + assert result["completed"] is True + # The finalization pop strips the answered pair from the live list — + # no flagged scaffolding may survive into the returned transcript. + leftover = [ + m for m in result["messages"] + if isinstance(m, dict) and m.get("_dropped_toolcall_nudge") + ] + assert not leftover, ( + "The re-prompt pair must be stripped at finalization, not kept " + "in the returned transcript." + ) + # And the persistence filter must classify the flag as ephemeral so a + # mid-turn flush can never write the pair to the durable store either. + assert _is_ephemeral_scaffolding( + {"role": "user", "content": "nudge", "_dropped_toolcall_nudge": True} + ), ( + "_dropped_toolcall_nudge messages must be classified as " + "ephemeral scaffolding so they are never persisted." + ) + + def test_unanswered_nudge_tail_is_stripped_at_finalization(self, loop_agent): + """If the model answers the nudge with a genuine final text turn, the + trailing scaffolding must not leave the transcript tail on a synthetic + user message (strict role alternation on the next turn).""" + from tests.run_agent.test_run_agent import _mock_response + + loop_agent.client.chat.completions.create.side_effect = [ + _dropped_tool_call_response("Let me check."), + _mock_response(content="Final answer.", finish_reason="stop"), + ] + + with ( + patch.object(loop_agent, "_persist_session"), + patch.object(loop_agent, "_save_trajectory"), + patch.object(loop_agent, "_cleanup_task_resources"), + ): + result = loop_agent.run_conversation("review the PR") + + tail = result["messages"][-1] + assert tail.get("role") == "assistant", ( + "The turn must end on the real assistant answer, not scaffolding." + ) + assert not tail.get("_dropped_toolcall_nudge")