fix(agent): preserve none vs unknown tool effects (#61783)
* fix(agent): persist truthful tool effect dispositions * fix(agent): preserve successful siblings during orphan recovery * fix(agent): narrow effect dispositions to none and unknown
This commit is contained in:
+65
-5
@@ -20,6 +20,9 @@ from __future__ import annotations
|
|||||||
import logging
|
import logging
|
||||||
from typing import Any, Dict, List
|
from typing import Any, Dict, List
|
||||||
|
|
||||||
|
from agent.tool_dispatch_helpers import make_tool_result_message
|
||||||
|
from agent.tool_result_classification import tool_may_have_side_effect
|
||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
|
|
||||||
@@ -64,8 +67,40 @@ def strip_interrupted_tool_tails(
|
|||||||
is_interrupted_tool_result(m.get("content", ""))
|
is_interrupted_tool_result(m.get("content", ""))
|
||||||
for m in tool_results
|
for m in tool_results
|
||||||
):
|
):
|
||||||
|
calls = msg.get("tool_calls") or []
|
||||||
|
if any(
|
||||||
|
tool_may_have_side_effect(
|
||||||
|
str((call.get("function") or {}).get("name") or "")
|
||||||
|
)
|
||||||
|
for call in calls
|
||||||
|
):
|
||||||
|
call_names = {
|
||||||
|
str(call.get("id") or call.get("call_id") or ""): str(
|
||||||
|
(call.get("function") or {}).get("name") or ""
|
||||||
|
)
|
||||||
|
for call in calls
|
||||||
|
}
|
||||||
|
cleaned.append(msg)
|
||||||
|
for tool_result in tool_results:
|
||||||
|
if not is_interrupted_tool_result(tool_result.get("content", "")):
|
||||||
|
cleaned.append(tool_result)
|
||||||
|
continue
|
||||||
|
recovered = dict(tool_result)
|
||||||
|
name = call_names.get(str(tool_result.get("tool_call_id") or ""), "")
|
||||||
|
recovered["effect_disposition"] = (
|
||||||
|
"unknown" if tool_may_have_side_effect(name) else "none"
|
||||||
|
)
|
||||||
|
recovered["content"] = (
|
||||||
|
"[Orphan recovery: interrupted side-effecting tool may have "
|
||||||
|
"executed; its effect is UNKNOWN. Inspect state before retrying.]"
|
||||||
|
if recovered["effect_disposition"] == "unknown"
|
||||||
|
else "[Orphan recovery: interrupted read-only tool did not complete.]"
|
||||||
|
)
|
||||||
|
cleaned.append(recovered)
|
||||||
|
i = j
|
||||||
|
continue
|
||||||
logger.debug(
|
logger.debug(
|
||||||
"Stripping interrupted assistant→tool replay block "
|
"Stripping interrupted read-only assistant→tool replay block "
|
||||||
"(indices %d–%d, tool_results=%d)",
|
"(indices %d–%d, tool_results=%d)",
|
||||||
i, j - 1, len(tool_results),
|
i, j - 1, len(tool_results),
|
||||||
)
|
)
|
||||||
@@ -116,11 +151,36 @@ def strip_dangling_tool_call_tail(
|
|||||||
):
|
):
|
||||||
return agent_history
|
return agent_history
|
||||||
|
|
||||||
|
tool_calls = last.get("tool_calls") or []
|
||||||
|
if any(
|
||||||
|
tool_may_have_side_effect(
|
||||||
|
str((call.get("function") or {}).get("name") or "")
|
||||||
|
)
|
||||||
|
for call in tool_calls
|
||||||
|
):
|
||||||
|
recovered = list(agent_history)
|
||||||
|
for call in tool_calls:
|
||||||
|
function = call.get("function") or {}
|
||||||
|
name = str(function.get("name") or "unknown")
|
||||||
|
call_id = str(call.get("id") or call.get("call_id") or "")
|
||||||
|
disposition = "unknown" if tool_may_have_side_effect(name) else "none"
|
||||||
|
content = (
|
||||||
|
"[Orphan recovery: this tool may have executed before Hermes stopped; "
|
||||||
|
"its effect is UNKNOWN. Inspect current state before retrying.]"
|
||||||
|
if disposition == "unknown"
|
||||||
|
else "[Orphan recovery: this read-only tool did not complete and had no effect.]"
|
||||||
|
)
|
||||||
|
recovered.append(make_tool_result_message(
|
||||||
|
name, content, call_id, effect_disposition=disposition,
|
||||||
|
))
|
||||||
|
logger.warning(
|
||||||
|
"Recovered dangling side-effecting tool call(s) as UNKNOWN instead of erasing them"
|
||||||
|
)
|
||||||
|
return recovered
|
||||||
|
|
||||||
logger.debug(
|
logger.debug(
|
||||||
"Stripping dangling unanswered assistant(tool_calls) tail "
|
"Stripping dangling unanswered read-only assistant(tool_calls) tail (%d call(s))",
|
||||||
"(%d call(s)) — process likely killed mid-tool-call by a "
|
len(tool_calls),
|
||||||
"restart/shutdown command (#49201)",
|
|
||||||
len(last.get("tool_calls") or []),
|
|
||||||
)
|
)
|
||||||
return agent_history[:-1]
|
return agent_history[:-1]
|
||||||
|
|
||||||
|
|||||||
@@ -359,7 +359,13 @@ def _trajectory_normalize_msg(msg: Dict[str, Any]) -> Dict[str, Any]:
|
|||||||
return msg
|
return msg
|
||||||
|
|
||||||
|
|
||||||
def make_tool_result_message(name: str, content: Any, tool_call_id: str) -> dict:
|
def make_tool_result_message(
|
||||||
|
name: str,
|
||||||
|
content: Any,
|
||||||
|
tool_call_id: str,
|
||||||
|
*,
|
||||||
|
effect_disposition: str | None = None,
|
||||||
|
) -> dict:
|
||||||
"""Build a tool-result message dict with both the OpenAI-format ``name``
|
"""Build a tool-result message dict with both the OpenAI-format ``name``
|
||||||
field (required by the wire format and provider adapters) and the internal
|
field (required by the wire format and provider adapters) and the internal
|
||||||
``tool_name`` field (written to the session DB messages table).
|
``tool_name`` field (written to the session DB messages table).
|
||||||
@@ -394,6 +400,8 @@ def make_tool_result_message(name: str, content: Any, tool_call_id: str) -> dict
|
|||||||
else:
|
else:
|
||||||
if risk_metadata is not None:
|
if risk_metadata is not None:
|
||||||
message["_tool_output_risk"] = risk_metadata
|
message["_tool_output_risk"] = risk_metadata
|
||||||
|
if effect_disposition is not None:
|
||||||
|
message["effect_disposition"] = effect_disposition
|
||||||
return message
|
return message
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
+13
-1
@@ -343,6 +343,7 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe
|
|||||||
tc.function.name,
|
tc.function.name,
|
||||||
f"[Tool execution cancelled — {tc.function.name} was skipped due to user interrupt]",
|
f"[Tool execution cancelled — {tc.function.name} was skipped due to user interrupt]",
|
||||||
tc.id,
|
tc.id,
|
||||||
|
effect_disposition="none",
|
||||||
))
|
))
|
||||||
_flush_session_db_after_tool_progress(
|
_flush_session_db_after_tool_progress(
|
||||||
agent,
|
agent,
|
||||||
@@ -827,9 +828,11 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe
|
|||||||
# deadline snapshot (timed_out_indices, taken from not_done) and this
|
# deadline snapshot (timed_out_indices, taken from not_done) and this
|
||||||
# loop. Prefer that real result over a fabricated timeout message — the
|
# loop. Prefer that real result over a fabricated timeout message — the
|
||||||
# tool genuinely succeeded, just slightly late.
|
# tool genuinely succeeded, just slightly late.
|
||||||
|
effect_disposition = None
|
||||||
if i in timed_out_indices and r is None:
|
if i in timed_out_indices and r is None:
|
||||||
suffix = f"{timeout_s:.1f}s" if timeout_s is not None else "the configured timeout"
|
suffix = f"{timeout_s:.1f}s" if timeout_s is not None else "the configured timeout"
|
||||||
function_result = f"Error executing tool '{name}': timed out after {suffix}"
|
function_result = f"Error executing tool '{name}': timed out after {suffix}"
|
||||||
|
effect_disposition = "unknown"
|
||||||
_emit_terminal_post_tool_call(
|
_emit_terminal_post_tool_call(
|
||||||
agent,
|
agent,
|
||||||
function_name=name,
|
function_name=name,
|
||||||
@@ -876,6 +879,8 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe
|
|||||||
tool_duration = 0.0
|
tool_duration = 0.0
|
||||||
else:
|
else:
|
||||||
function_name, function_args, function_result, tool_duration, is_error, blocked, middleware_trace = r
|
function_name, function_args, function_result, tool_duration, is_error, blocked, middleware_trace = r
|
||||||
|
if blocked:
|
||||||
|
effect_disposition = "none"
|
||||||
|
|
||||||
if not blocked:
|
if not blocked:
|
||||||
function_result = agent._append_guardrail_observation(
|
function_result = agent._append_guardrail_observation(
|
||||||
@@ -964,7 +969,12 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe
|
|||||||
# image tool result never poisons canonical session history.
|
# image tool result never poisons canonical session history.
|
||||||
# String results pass through unchanged.
|
# String results pass through unchanged.
|
||||||
_tool_content = agent._tool_result_content_for_active_model(name, function_result)
|
_tool_content = agent._tool_result_content_for_active_model(name, function_result)
|
||||||
tool_message = make_tool_result_message(name, _tool_content, tc.id)
|
tool_message = make_tool_result_message(
|
||||||
|
name,
|
||||||
|
_tool_content,
|
||||||
|
tc.id,
|
||||||
|
effect_disposition=effect_disposition,
|
||||||
|
)
|
||||||
messages.append(tool_message)
|
messages.append(tool_message)
|
||||||
risk_metadata = tool_message.get("_tool_output_risk")
|
risk_metadata = tool_message.get("_tool_output_risk")
|
||||||
if (
|
if (
|
||||||
@@ -1027,6 +1037,7 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe
|
|||||||
skipped_name,
|
skipped_name,
|
||||||
f"[Tool execution cancelled — {skipped_name} was skipped due to user interrupt]",
|
f"[Tool execution cancelled — {skipped_name} was skipped due to user interrupt]",
|
||||||
skipped_tc.id,
|
skipped_tc.id,
|
||||||
|
effect_disposition="none",
|
||||||
))
|
))
|
||||||
_flush_session_db_after_tool_progress(
|
_flush_session_db_after_tool_progress(
|
||||||
agent,
|
agent,
|
||||||
@@ -1691,6 +1702,7 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe
|
|||||||
skipped_name,
|
skipped_name,
|
||||||
f"[Tool execution skipped — {skipped_name} was not started. User sent a new message]",
|
f"[Tool execution skipped — {skipped_name} was not started. User sent a new message]",
|
||||||
skipped_tc.id,
|
skipped_tc.id,
|
||||||
|
effect_disposition="none",
|
||||||
))
|
))
|
||||||
_flush_session_db_after_tool_progress(
|
_flush_session_db_after_tool_progress(
|
||||||
agent,
|
agent,
|
||||||
|
|||||||
@@ -9,6 +9,20 @@ from typing import Any
|
|||||||
FILE_MUTATING_TOOL_NAMES = frozenset({"write_file", "patch"})
|
FILE_MUTATING_TOOL_NAMES = frozenset({"write_file", "patch"})
|
||||||
|
|
||||||
|
|
||||||
|
# Tools whose interrupted/dangling execution is safe to discard because they
|
||||||
|
# cannot mutate either external state or Hermes session state. Unknown/plugin/
|
||||||
|
# MCP tools stay effect-capable by default.
|
||||||
|
NO_EFFECT_TOOL_NAMES = frozenset({
|
||||||
|
"read_file", "search_files", "session_search", "skill_view", "skills_list",
|
||||||
|
"web_extract", "web_search", "vision_analyze", "browser_snapshot",
|
||||||
|
"browser_get_images", "browser_console", "read_terminal",
|
||||||
|
})
|
||||||
|
|
||||||
|
|
||||||
|
def tool_may_have_side_effect(tool_name: str) -> bool:
|
||||||
|
return tool_name not in NO_EFFECT_TOOL_NAMES
|
||||||
|
|
||||||
|
|
||||||
def file_mutation_result_landed(tool_name: str, result: Any) -> bool:
|
def file_mutation_result_landed(tool_name: str, result: Any) -> bool:
|
||||||
"""Return True when a file mutation result proves the write landed."""
|
"""Return True when a file mutation result proves the write landed."""
|
||||||
if tool_name not in FILE_MUTATING_TOOL_NAMES or not isinstance(result, str):
|
if tool_name not in FILE_MUTATING_TOOL_NAMES or not isinstance(result, str):
|
||||||
|
|||||||
@@ -171,6 +171,7 @@ class ChatCompletionsTransport(ProviderTransport):
|
|||||||
"codex_reasoning_items" in msg
|
"codex_reasoning_items" in msg
|
||||||
or "codex_message_items" in msg
|
or "codex_message_items" in msg
|
||||||
or "tool_name" in msg
|
or "tool_name" in msg
|
||||||
|
or "effect_disposition" in msg
|
||||||
or "timestamp" in msg # #47868 — strict providers reject this
|
or "timestamp" in msg # #47868 — strict providers reject this
|
||||||
):
|
):
|
||||||
needs_sanitize = True
|
needs_sanitize = True
|
||||||
@@ -212,12 +213,14 @@ class ChatCompletionsTransport(ProviderTransport):
|
|||||||
"codex_reasoning_items" in msg
|
"codex_reasoning_items" in msg
|
||||||
or "codex_message_items" in msg
|
or "codex_message_items" in msg
|
||||||
or "tool_name" in msg
|
or "tool_name" in msg
|
||||||
|
or "effect_disposition" in msg
|
||||||
or "timestamp" in msg # #47868 — leak into strict providers
|
or "timestamp" in msg # #47868 — leak into strict providers
|
||||||
):
|
):
|
||||||
out_msg = mutable_msg()
|
out_msg = mutable_msg()
|
||||||
out_msg.pop("codex_reasoning_items", None)
|
out_msg.pop("codex_reasoning_items", None)
|
||||||
out_msg.pop("codex_message_items", None)
|
out_msg.pop("codex_message_items", None)
|
||||||
out_msg.pop("tool_name", None)
|
out_msg.pop("tool_name", None)
|
||||||
|
out_msg.pop("effect_disposition", None)
|
||||||
out_msg.pop("timestamp", None) # #47868 — leak into strict providers
|
out_msg.pop("timestamp", None) # #47868 — leak into strict providers
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
+11
-5
@@ -753,6 +753,7 @@ CREATE TABLE IF NOT EXISTS messages (
|
|||||||
tool_call_id TEXT,
|
tool_call_id TEXT,
|
||||||
tool_calls TEXT,
|
tool_calls TEXT,
|
||||||
tool_name TEXT,
|
tool_name TEXT,
|
||||||
|
effect_disposition TEXT,
|
||||||
timestamp REAL NOT NULL,
|
timestamp REAL NOT NULL,
|
||||||
token_count INTEGER,
|
token_count INTEGER,
|
||||||
finish_reason TEXT,
|
finish_reason TEXT,
|
||||||
@@ -3455,6 +3456,7 @@ class SessionDB:
|
|||||||
codex_message_items: Any = None,
|
codex_message_items: Any = None,
|
||||||
platform_message_id: str = None,
|
platform_message_id: str = None,
|
||||||
observed: bool = False,
|
observed: bool = False,
|
||||||
|
effect_disposition: Optional[str] = None,
|
||||||
timestamp: Any = None,
|
timestamp: Any = None,
|
||||||
) -> int:
|
) -> int:
|
||||||
"""
|
"""
|
||||||
@@ -3505,10 +3507,10 @@ class SessionDB:
|
|||||||
def _do(conn):
|
def _do(conn):
|
||||||
cursor = conn.execute(
|
cursor = conn.execute(
|
||||||
"""INSERT INTO messages (session_id, role, content, tool_call_id,
|
"""INSERT INTO messages (session_id, role, content, tool_call_id,
|
||||||
tool_calls, tool_name, timestamp, token_count, finish_reason,
|
tool_calls, tool_name, effect_disposition, timestamp, token_count, finish_reason,
|
||||||
reasoning, reasoning_content, reasoning_details, codex_reasoning_items,
|
reasoning, reasoning_content, reasoning_details, codex_reasoning_items,
|
||||||
codex_message_items, platform_message_id, observed, active)
|
codex_message_items, platform_message_id, observed, active)
|
||||||
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)""",
|
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)""",
|
||||||
(
|
(
|
||||||
session_id,
|
session_id,
|
||||||
role,
|
role,
|
||||||
@@ -3516,6 +3518,7 @@ class SessionDB:
|
|||||||
tool_call_id,
|
tool_call_id,
|
||||||
tool_calls_json,
|
tool_calls_json,
|
||||||
tool_name,
|
tool_name,
|
||||||
|
effect_disposition,
|
||||||
message_timestamp,
|
message_timestamp,
|
||||||
token_count,
|
token_count,
|
||||||
finish_reason,
|
finish_reason,
|
||||||
@@ -3597,10 +3600,10 @@ class SessionDB:
|
|||||||
|
|
||||||
conn.execute(
|
conn.execute(
|
||||||
"""INSERT INTO messages (session_id, role, content, tool_call_id,
|
"""INSERT INTO messages (session_id, role, content, tool_call_id,
|
||||||
tool_calls, tool_name, timestamp, token_count, finish_reason,
|
tool_calls, tool_name, effect_disposition, timestamp, token_count, finish_reason,
|
||||||
reasoning, reasoning_content, reasoning_details, codex_reasoning_items,
|
reasoning, reasoning_content, reasoning_details, codex_reasoning_items,
|
||||||
codex_message_items, platform_message_id, observed, active)
|
codex_message_items, platform_message_id, observed, active)
|
||||||
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)""",
|
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)""",
|
||||||
(
|
(
|
||||||
session_id,
|
session_id,
|
||||||
role,
|
role,
|
||||||
@@ -3608,6 +3611,7 @@ class SessionDB:
|
|||||||
msg.get("tool_call_id"),
|
msg.get("tool_call_id"),
|
||||||
tool_calls_json,
|
tool_calls_json,
|
||||||
msg.get("tool_name"),
|
msg.get("tool_name"),
|
||||||
|
msg.get("effect_disposition"),
|
||||||
message_timestamp,
|
message_timestamp,
|
||||||
msg.get("token_count"),
|
msg.get("token_count"),
|
||||||
msg.get("finish_reason"),
|
msg.get("finish_reason"),
|
||||||
@@ -4103,7 +4107,7 @@ class SessionDB:
|
|||||||
with self._lock:
|
with self._lock:
|
||||||
placeholders = ",".join("?" for _ in session_ids)
|
placeholders = ",".join("?" for _ in session_ids)
|
||||||
rows = self._conn.execute(
|
rows = self._conn.execute(
|
||||||
"SELECT role, content, tool_call_id, tool_calls, tool_name, "
|
"SELECT role, content, tool_call_id, tool_calls, tool_name, effect_disposition, "
|
||||||
"finish_reason, reasoning, reasoning_content, reasoning_details, "
|
"finish_reason, reasoning, reasoning_content, reasoning_details, "
|
||||||
"codex_reasoning_items, codex_message_items, platform_message_id, observed, timestamp "
|
"codex_reasoning_items, codex_message_items, platform_message_id, observed, timestamp "
|
||||||
f"FROM messages WHERE session_id IN ({placeholders})"
|
f"FROM messages WHERE session_id IN ({placeholders})"
|
||||||
@@ -4131,6 +4135,8 @@ class SessionDB:
|
|||||||
msg["tool_call_id"] = row["tool_call_id"]
|
msg["tool_call_id"] = row["tool_call_id"]
|
||||||
if row["tool_name"]:
|
if row["tool_name"]:
|
||||||
msg["tool_name"] = row["tool_name"]
|
msg["tool_name"] = row["tool_name"]
|
||||||
|
if row["effect_disposition"]:
|
||||||
|
msg["effect_disposition"] = row["effect_disposition"]
|
||||||
if row["tool_calls"]:
|
if row["tool_calls"]:
|
||||||
try:
|
try:
|
||||||
msg["tool_calls"] = json.loads(row["tool_calls"])
|
msg["tool_calls"] = json.loads(row["tool_calls"])
|
||||||
|
|||||||
@@ -40,24 +40,75 @@ def test_is_interrupted_tool_result_markers():
|
|||||||
assert not is_interrupted_tool_result(None)
|
assert not is_interrupted_tool_result(None)
|
||||||
|
|
||||||
|
|
||||||
def test_strip_dangling_tool_call_tail_removes_unanswered_tail():
|
def test_strip_dangling_tool_call_tail_removes_unanswered_read_only_tail():
|
||||||
history = [_user("hi"), _assistant_tc("write_file")]
|
history = [_user("hi"), _assistant_tc("read_file")]
|
||||||
out = strip_dangling_tool_call_tail(history)
|
out = strip_dangling_tool_call_tail(history)
|
||||||
assert out == [_user("hi")]
|
assert out == [_user("hi")]
|
||||||
|
|
||||||
|
|
||||||
|
def test_dangling_side_effect_is_recovered_as_unknown_not_erased():
|
||||||
|
history = [_user("hi"), _assistant_tc("write_file")]
|
||||||
|
|
||||||
|
out = strip_dangling_tool_call_tail(history)
|
||||||
|
|
||||||
|
assert out[:-1] == history
|
||||||
|
assert out[-1]["role"] == "tool"
|
||||||
|
assert out[-1]["tool_call_id"] == "c1"
|
||||||
|
assert out[-1]["effect_disposition"] == "unknown"
|
||||||
|
assert "may have executed" in out[-1]["content"].lower()
|
||||||
|
|
||||||
|
|
||||||
|
def test_dangling_session_mutation_is_recovered_as_unknown():
|
||||||
|
history = [_user("hi"), _assistant_tc("todo")]
|
||||||
|
|
||||||
|
out = strip_dangling_tool_call_tail(history)
|
||||||
|
|
||||||
|
assert out[:-1] == history
|
||||||
|
assert out[-1]["effect_disposition"] == "unknown"
|
||||||
|
assert "may have executed" in out[-1]["content"].lower()
|
||||||
|
|
||||||
|
|
||||||
|
def test_mixed_dangling_batch_uses_truthful_per_call_wording():
|
||||||
|
assistant = {
|
||||||
|
"role": "assistant",
|
||||||
|
"content": "",
|
||||||
|
"tool_calls": [
|
||||||
|
{"id": "read", "function": {"name": "read_file", "arguments": "{}"}},
|
||||||
|
{"id": "write", "function": {"name": "write_file", "arguments": "{}"}},
|
||||||
|
],
|
||||||
|
}
|
||||||
|
out = strip_dangling_tool_call_tail([_user("hi"), assistant])
|
||||||
|
|
||||||
|
read_result, write_result = out[-2:]
|
||||||
|
assert read_result["effect_disposition"] == "none"
|
||||||
|
assert "no effect" in read_result["content"].lower()
|
||||||
|
assert "unknown" not in read_result["content"].lower()
|
||||||
|
assert write_result["effect_disposition"] == "unknown"
|
||||||
|
assert "unknown" in write_result["content"].lower()
|
||||||
|
|
||||||
|
|
||||||
def test_strip_dangling_tool_call_tail_preserves_answered_pair():
|
def test_strip_dangling_tool_call_tail_preserves_answered_pair():
|
||||||
history = [_user("hi"), _assistant_tc("read_file"), _tool("contents")]
|
history = [_user("hi"), _assistant_tc("read_file"), _tool("contents")]
|
||||||
out = strip_dangling_tool_call_tail(history)
|
out = strip_dangling_tool_call_tail(history)
|
||||||
assert out == history # answered -> untouched
|
assert out == history # answered -> untouched
|
||||||
|
|
||||||
|
|
||||||
def test_strip_interrupted_tool_tails_removes_interrupted_block():
|
def test_strip_interrupted_tool_tails_removes_interrupted_read_only_block():
|
||||||
history = [_user("hi"), _assistant_tc("terminal"), _tool("[Command interrupted]")]
|
history = [_user("hi"), _assistant_tc("read_file"), _tool("[Command interrupted]")]
|
||||||
out = strip_interrupted_tool_tails(history)
|
out = strip_interrupted_tool_tails(history)
|
||||||
assert out == [_user("hi")]
|
assert out == [_user("hi")]
|
||||||
|
|
||||||
|
|
||||||
|
def test_interrupted_side_effect_is_preserved_as_unknown():
|
||||||
|
history = [_user("hi"), _assistant_tc("terminal"), _tool("[Command interrupted]")]
|
||||||
|
|
||||||
|
out = strip_interrupted_tool_tails(history)
|
||||||
|
|
||||||
|
assert out[:-1] == history[:-1]
|
||||||
|
assert out[-1]["role"] == "tool"
|
||||||
|
assert out[-1]["effect_disposition"] == "unknown"
|
||||||
|
|
||||||
|
|
||||||
def test_strip_interrupted_tool_tails_preserves_successful_block():
|
def test_strip_interrupted_tool_tails_preserves_successful_block():
|
||||||
history = [_user("hi"), _assistant_tc("read_file"), _tool("ok"),
|
history = [_user("hi"), _assistant_tc("read_file"), _tool("ok"),
|
||||||
{"role": "assistant", "content": "done"}]
|
{"role": "assistant", "content": "done"}]
|
||||||
@@ -72,15 +123,20 @@ def test_strip_interrupted_tool_tails_removes_orphan_interrupted_tool():
|
|||||||
|
|
||||||
|
|
||||||
def test_sanitize_replay_history_combines_both():
|
def test_sanitize_replay_history_combines_both():
|
||||||
# interrupted block in the middle + dangling tail at the end
|
# interrupted block is removed; a dangling read-only call is safe to erase
|
||||||
history = [
|
history = [
|
||||||
_user("first"),
|
_user("first"),
|
||||||
_assistant_tc("terminal"), _tool("[Command interrupted]"),
|
_assistant_tc("terminal"), _tool("[Command interrupted]"),
|
||||||
_user("second"),
|
_user("second"),
|
||||||
_assistant_tc("write_file"), # dangling
|
_assistant_tc("read_file"), # dangling
|
||||||
]
|
]
|
||||||
out = sanitize_replay_history(history)
|
out = sanitize_replay_history(history)
|
||||||
assert out == [_user("first"), _user("second")]
|
assert out[:2] == [
|
||||||
|
_user("first"),
|
||||||
|
_assistant_tc("terminal"),
|
||||||
|
]
|
||||||
|
assert out[2]["effect_disposition"] == "unknown"
|
||||||
|
assert out[-1] == _user("second")
|
||||||
|
|
||||||
|
|
||||||
def test_sanitize_replay_history_noop_on_clean_history():
|
def test_sanitize_replay_history_noop_on_clean_history():
|
||||||
|
|||||||
@@ -222,6 +222,12 @@ class TestMakeToolResultMessage:
|
|||||||
"tool_call_id": "call_1",
|
"tool_call_id": "call_1",
|
||||||
}
|
}
|
||||||
|
|
||||||
|
def test_effect_disposition_is_internal_message_metadata(self):
|
||||||
|
msg = make_tool_result_message(
|
||||||
|
"terminal", "timed out", "call_effect", effect_disposition="unknown"
|
||||||
|
)
|
||||||
|
assert msg["effect_disposition"] == "unknown"
|
||||||
|
|
||||||
def test_high_risk_message_content_wrapped(self):
|
def test_high_risk_message_content_wrapped(self):
|
||||||
msg = make_tool_result_message("web_extract", SAMPLE_LONG_TEXT, "call_2")
|
msg = make_tool_result_message("web_extract", SAMPLE_LONG_TEXT, "call_2")
|
||||||
assert msg["role"] == "tool"
|
assert msg["role"] == "tool"
|
||||||
|
|||||||
@@ -2,7 +2,9 @@
|
|||||||
|
|
||||||
import json
|
import json
|
||||||
|
|
||||||
from agent.tool_result_classification import file_mutation_result_landed
|
from agent.tool_result_classification import (
|
||||||
|
file_mutation_result_landed,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def test_write_file_with_nested_lint_error_counts_as_landed():
|
def test_write_file_with_nested_lint_error_counts_as_landed():
|
||||||
@@ -28,3 +30,14 @@ def test_top_level_file_mutation_error_does_not_count_as_landed():
|
|||||||
result = json.dumps({"success": True, "error": "post-write verification failed"})
|
result = json.dumps({"success": True, "error": "post-write verification failed"})
|
||||||
|
|
||||||
assert file_mutation_result_landed("patch", result) is False
|
assert file_mutation_result_landed("patch", result) is False
|
||||||
|
|
||||||
|
|
||||||
|
def test_side_effect_classification_keeps_session_mutations():
|
||||||
|
from agent.tool_result_classification import tool_may_have_side_effect
|
||||||
|
|
||||||
|
assert tool_may_have_side_effect("todo") is True
|
||||||
|
assert tool_may_have_side_effect("memory") is True
|
||||||
|
assert tool_may_have_side_effect("write_file") is True
|
||||||
|
assert tool_may_have_side_effect("mcp_unknown") is True
|
||||||
|
assert tool_may_have_side_effect("read_file") is False
|
||||||
|
assert tool_may_have_side_effect("web_search") is False
|
||||||
|
|||||||
@@ -30,6 +30,19 @@ class TestChatCompletionsBasic:
|
|||||||
result = transport.convert_messages(msgs)
|
result = transport.convert_messages(msgs)
|
||||||
assert result is msgs # no copy needed
|
assert result is msgs # no copy needed
|
||||||
|
|
||||||
|
def test_convert_messages_strips_internal_effect_disposition(self, transport):
|
||||||
|
msgs = [{
|
||||||
|
"role": "tool",
|
||||||
|
"content": "uncertain",
|
||||||
|
"tool_call_id": "c1",
|
||||||
|
"effect_disposition": "unknown",
|
||||||
|
}]
|
||||||
|
|
||||||
|
result = transport.convert_messages(msgs)
|
||||||
|
|
||||||
|
assert "effect_disposition" not in result[0]
|
||||||
|
assert msgs[0]["effect_disposition"] == "unknown"
|
||||||
|
|
||||||
def test_convert_messages_strips_codex_fields(self, transport):
|
def test_convert_messages_strips_codex_fields(self, transport):
|
||||||
msgs = [
|
msgs = [
|
||||||
{"role": "assistant", "content": "ok", "codex_reasoning_items": [{"id": "rs_1"}],
|
{"role": "assistant", "content": "ok", "codex_reasoning_items": [{"id": "rs_1"}],
|
||||||
|
|||||||
@@ -96,7 +96,7 @@ class TestAutoDetection:
|
|||||||
|
|
||||||
|
|
||||||
class TestInterruptedReplayFiltering:
|
class TestInterruptedReplayFiltering:
|
||||||
def test_interrupted_tool_tail_is_removed_from_agent_history(self):
|
def test_interrupted_side_effect_is_replayed_as_unknown(self):
|
||||||
from gateway.run import _build_gateway_agent_history
|
from gateway.run import _build_gateway_agent_history
|
||||||
|
|
||||||
history = [
|
history = [
|
||||||
@@ -118,9 +118,12 @@ class TestInterruptedReplayFiltering:
|
|||||||
agent_history, observed_context = _build_gateway_agent_history(history)
|
agent_history, observed_context = _build_gateway_agent_history(history)
|
||||||
|
|
||||||
assert observed_context is None
|
assert observed_context is None
|
||||||
assert agent_history == [{"role": "user", "content": "transcribe this video"}]
|
assert agent_history[:2] == history[:2]
|
||||||
|
assert agent_history[-1]["role"] == "tool"
|
||||||
|
assert agent_history[-1]["tool_call_id"] == "call_1"
|
||||||
|
assert agent_history[-1]["effect_disposition"] == "unknown"
|
||||||
|
|
||||||
def test_mixed_tail_with_one_interrupted_result_is_removed(self):
|
def test_mixed_tail_preserves_results_and_marks_interrupted_effect_unknown(self):
|
||||||
from gateway.run import _build_gateway_agent_history
|
from gateway.run import _build_gateway_agent_history
|
||||||
|
|
||||||
history = [
|
history = [
|
||||||
@@ -143,7 +146,10 @@ class TestInterruptedReplayFiltering:
|
|||||||
|
|
||||||
agent_history, _observed_context = _build_gateway_agent_history(history)
|
agent_history, _observed_context = _build_gateway_agent_history(history)
|
||||||
|
|
||||||
assert agent_history == [{"role": "user", "content": "search and transcribe"}]
|
assert agent_history[:3] == history[:3]
|
||||||
|
assert agent_history[-1]["role"] == "tool"
|
||||||
|
assert agent_history[-1]["tool_call_id"] == "call_2"
|
||||||
|
assert agent_history[-1]["effect_disposition"] == "unknown"
|
||||||
|
|
||||||
def test_successful_tool_tail_is_preserved(self):
|
def test_successful_tool_tail_is_preserved(self):
|
||||||
from gateway.run import _build_gateway_agent_history
|
from gateway.run import _build_gateway_agent_history
|
||||||
@@ -165,14 +171,14 @@ class TestInterruptedReplayFiltering:
|
|||||||
assert agent_history[-1]["role"] == "tool"
|
assert agent_history[-1]["role"] == "tool"
|
||||||
assert agent_history[-1]["content"] == "deployed successfully"
|
assert agent_history[-1]["content"] == "deployed successfully"
|
||||||
|
|
||||||
def test_dangling_unanswered_tool_call_tail_is_removed(self):
|
def test_dangling_unanswered_side_effect_is_replayed_as_unknown(self):
|
||||||
"""A trailing assistant(tool_calls) with NO tool answers is stripped.
|
"""A trailing side-effecting call gets an UNKNOWN result, not a retry.
|
||||||
|
|
||||||
This is the SIGKILL signature from #49201: the tool itself ran a
|
This is the SIGKILL signature from #49201: the tool itself ran a
|
||||||
restart/shutdown command and killed the gateway before its result was
|
restart/shutdown command and killed the gateway before its result was
|
||||||
persisted. The transcript tail is an assistant message with tool_calls
|
persisted. The transcript tail is an assistant message with tool_calls
|
||||||
and zero matching tool rows. Without stripping it, the model re-issues
|
and zero matching tool rows. A synthetic UNKNOWN result closes the tool
|
||||||
the unanswered call on resume and loops the restart forever.
|
pair without claiming the restart did not happen or inviting a retry.
|
||||||
"""
|
"""
|
||||||
from gateway.run import _build_gateway_agent_history
|
from gateway.run import _build_gateway_agent_history
|
||||||
|
|
||||||
@@ -195,13 +201,16 @@ class TestInterruptedReplayFiltering:
|
|||||||
|
|
||||||
agent_history, _observed_context = _build_gateway_agent_history(history)
|
agent_history, _observed_context = _build_gateway_agent_history(history)
|
||||||
|
|
||||||
assert agent_history == [{"role": "user", "content": "restart the container"}]
|
assert agent_history[:2] == history
|
||||||
|
assert agent_history[-1]["role"] == "tool"
|
||||||
|
assert agent_history[-1]["tool_call_id"] == "call_1"
|
||||||
|
assert agent_history[-1]["effect_disposition"] == "unknown"
|
||||||
|
|
||||||
def test_dangling_tail_after_completed_pair_is_removed_only_at_tail(self):
|
def test_dangling_tail_after_completed_pair_gets_unknown_result(self):
|
||||||
"""Only the trailing unanswered tool-call block is stripped.
|
"""The completed pair survives and the trailing call becomes UNKNOWN.
|
||||||
|
|
||||||
An earlier completed assistant→tool pair must survive — we only drop
|
An earlier completed assistant→tool pair must survive, and the final
|
||||||
the final assistant(tool_calls) that has no answers.
|
assistant(tool_calls) receives a matching UNKNOWN result.
|
||||||
"""
|
"""
|
||||||
from gateway.run import _build_gateway_agent_history
|
from gateway.run import _build_gateway_agent_history
|
||||||
|
|
||||||
@@ -232,18 +241,19 @@ class TestInterruptedReplayFiltering:
|
|||||||
|
|
||||||
agent_history, _observed_context = _build_gateway_agent_history(history)
|
agent_history, _observed_context = _build_gateway_agent_history(history)
|
||||||
|
|
||||||
# The completed call_1 pair survives; the dangling call_2 tail is gone.
|
# The completed call_1 pair survives; call_2 is closed truthfully.
|
||||||
assert agent_history[-1]["role"] == "tool"
|
assert agent_history[-1]["role"] == "tool"
|
||||||
assert agent_history[-1]["content"] == "found it"
|
assert agent_history[-1]["tool_call_id"] == "call_2"
|
||||||
# The surviving assistant(tool_calls) is the completed call_1 (which
|
assert agent_history[-1]["effect_disposition"] == "unknown"
|
||||||
# has a matching tool answer), not the stripped dangling call_2.
|
assert agent_history[2]["content"] == "found it"
|
||||||
|
# Both assistant calls survive with matching tool results.
|
||||||
_surviving_calls = [
|
_surviving_calls = [
|
||||||
tc.get("id")
|
tc.get("id")
|
||||||
for m in agent_history
|
for m in agent_history
|
||||||
if m.get("role") == "assistant" and m.get("tool_calls")
|
if m.get("role") == "assistant" and m.get("tool_calls")
|
||||||
for tc in m["tool_calls"]
|
for tc in m["tool_calls"]
|
||||||
]
|
]
|
||||||
assert _surviving_calls == ["call_1"]
|
assert _surviving_calls == ["call_1", "call_2"]
|
||||||
|
|
||||||
def test_persisted_auto_continue_note_is_not_replayed(self):
|
def test_persisted_auto_continue_note_is_not_replayed(self):
|
||||||
from gateway.run import _build_gateway_agent_history
|
from gateway.run import _build_gateway_agent_history
|
||||||
|
|||||||
@@ -2775,6 +2775,7 @@ class TestConcurrentToolExecution:
|
|||||||
assert "fast-result" in messages[0]["content"]
|
assert "fast-result" in messages[0]["content"]
|
||||||
assert messages[1]["tool_call_id"] == "c2"
|
assert messages[1]["tool_call_id"] == "c2"
|
||||||
assert "timed out after" in messages[1]["content"]
|
assert "timed out after" in messages[1]["content"]
|
||||||
|
assert messages[1]["effect_disposition"] == "unknown"
|
||||||
assert [batch[-1]["tool_call_id"] for batch in flushed] == ["c1", "c2"]
|
assert [batch[-1]["tool_call_id"] for batch in flushed] == ["c1", "c2"]
|
||||||
assert "fast-result" in flushed[0][-1]["content"]
|
assert "fast-result" in flushed[0][-1]["content"]
|
||||||
assert "timed out after" in flushed[1][-1]["content"]
|
assert "timed out after" in flushed[1][-1]["content"]
|
||||||
|
|||||||
@@ -934,6 +934,19 @@ class TestMessageStorage:
|
|||||||
tool_msg = next(m for m in msgs if m["role"] == "tool")
|
tool_msg = next(m for m in msgs if m["role"] == "tool")
|
||||||
assert tool_msg["tool_name"] == "web_search"
|
assert tool_msg["tool_name"] == "web_search"
|
||||||
|
|
||||||
|
def test_tool_effect_disposition_round_trips_through_session_db(self, db):
|
||||||
|
from agent.tool_dispatch_helpers import make_tool_result_message
|
||||||
|
|
||||||
|
db.create_session(session_id="s1", source="cli")
|
||||||
|
db.replace_messages(
|
||||||
|
"s1",
|
||||||
|
[make_tool_result_message(
|
||||||
|
"write_file", "worker detached", "c1", effect_disposition="unknown"
|
||||||
|
)],
|
||||||
|
)
|
||||||
|
|
||||||
|
assert db.get_messages_as_conversation("s1")[0]["effect_disposition"] == "unknown"
|
||||||
|
|
||||||
def test_replace_messages_handles_multimodal_content(self, db):
|
def test_replace_messages_handles_multimodal_content(self, db):
|
||||||
"""`replace_messages` (used by /retry, /undo, /compress) must also
|
"""`replace_messages` (used by /retry, /undo, /compress) must also
|
||||||
handle list content without crashing."""
|
handle list content without crashing."""
|
||||||
|
|||||||
Reference in New Issue
Block a user