fix(agent): make tool loop guardrails warning-first
This commit is contained in:
@@ -32,9 +32,69 @@ def test_tool_call_signature_hashes_canonical_nested_unicode_args_without_exposi
|
||||
assert "☤" not in json.dumps(metadata)
|
||||
|
||||
|
||||
def test_repeated_identical_failed_call_warns_then_blocks_before_third_execution():
|
||||
def test_default_config_is_soft_warning_only_with_hard_stop_disabled():
|
||||
cfg = ToolCallGuardrailConfig()
|
||||
|
||||
assert cfg.warnings_enabled is True
|
||||
assert cfg.hard_stop_enabled is False
|
||||
assert cfg.exact_failure_warn_after == 2
|
||||
assert cfg.same_tool_failure_warn_after == 3
|
||||
assert cfg.no_progress_warn_after == 2
|
||||
assert cfg.exact_failure_block_after == 5
|
||||
assert cfg.same_tool_failure_halt_after == 8
|
||||
assert cfg.no_progress_block_after == 5
|
||||
|
||||
|
||||
def test_config_parses_nested_warn_and_hard_stop_thresholds():
|
||||
cfg = ToolCallGuardrailConfig.from_mapping(
|
||||
{
|
||||
"warnings_enabled": False,
|
||||
"hard_stop_enabled": True,
|
||||
"warn_after": {
|
||||
"exact_failure": 3,
|
||||
"same_tool_failure": 4,
|
||||
"idempotent_no_progress": 5,
|
||||
},
|
||||
"hard_stop_after": {
|
||||
"exact_failure": 6,
|
||||
"same_tool_failure": 7,
|
||||
"idempotent_no_progress": 8,
|
||||
},
|
||||
}
|
||||
)
|
||||
|
||||
assert cfg.warnings_enabled is False
|
||||
assert cfg.hard_stop_enabled is True
|
||||
assert cfg.exact_failure_warn_after == 3
|
||||
assert cfg.same_tool_failure_warn_after == 4
|
||||
assert cfg.no_progress_warn_after == 5
|
||||
assert cfg.exact_failure_block_after == 6
|
||||
assert cfg.same_tool_failure_halt_after == 7
|
||||
assert cfg.no_progress_block_after == 8
|
||||
|
||||
|
||||
def test_default_repeated_identical_failed_call_warns_without_blocking():
|
||||
controller = ToolCallGuardrailController()
|
||||
args = {"query": "same"}
|
||||
|
||||
decisions = []
|
||||
for _ in range(5):
|
||||
assert controller.before_call("web_search", args).action == "allow"
|
||||
decisions.append(
|
||||
controller.after_call("web_search", args, '{"error":"boom"}', failed=True)
|
||||
)
|
||||
|
||||
assert decisions[0].action == "allow"
|
||||
assert [d.action for d in decisions[1:]] == ["warn", "warn", "warn", "warn"]
|
||||
assert {d.code for d in decisions[1:]} == {"repeated_exact_failure_warning"}
|
||||
assert controller.before_call("web_search", args).action == "allow"
|
||||
assert controller.halt_decision is None
|
||||
|
||||
|
||||
def test_hard_stop_enabled_blocks_repeated_exact_failure_before_next_execution():
|
||||
controller = ToolCallGuardrailController(
|
||||
ToolCallGuardrailConfig(
|
||||
hard_stop_enabled=True,
|
||||
exact_failure_warn_after=2,
|
||||
exact_failure_block_after=2,
|
||||
same_tool_failure_halt_after=99,
|
||||
@@ -50,18 +110,16 @@ def test_repeated_identical_failed_call_warns_then_blocks_before_third_execution
|
||||
second = controller.after_call("web_search", args, '{"error":"boom"}', failed=True)
|
||||
assert second.action == "warn"
|
||||
assert second.code == "repeated_exact_failure_warning"
|
||||
assert second.count == 2
|
||||
|
||||
blocked = controller.before_call("web_search", args)
|
||||
assert blocked.action == "block"
|
||||
assert blocked.code == "repeated_exact_failure_block"
|
||||
assert blocked.tool_name == "web_search"
|
||||
assert blocked.count == 2
|
||||
|
||||
|
||||
def test_success_resets_exact_signature_failure_streak():
|
||||
controller = ToolCallGuardrailController(
|
||||
ToolCallGuardrailConfig(exact_failure_block_after=2, same_tool_failure_halt_after=99)
|
||||
ToolCallGuardrailConfig(hard_stop_enabled=True, exact_failure_block_after=2, same_tool_failure_halt_after=99)
|
||||
)
|
||||
args = {"query": "same"}
|
||||
|
||||
@@ -73,9 +131,26 @@ def test_success_resets_exact_signature_failure_streak():
|
||||
assert controller.before_call("web_search", args).action == "allow"
|
||||
|
||||
|
||||
def test_same_tool_varying_args_failure_streak_warns_then_halts_independent_of_exact_streak():
|
||||
def test_same_tool_varying_args_warns_by_default_without_halting():
|
||||
controller = ToolCallGuardrailController(
|
||||
ToolCallGuardrailConfig(same_tool_failure_warn_after=2, same_tool_failure_halt_after=3)
|
||||
)
|
||||
|
||||
first = controller.after_call("terminal", {"command": "cmd-1"}, '{"exit_code":1}', failed=True)
|
||||
second = controller.after_call("terminal", {"command": "cmd-2"}, '{"exit_code":1}', failed=True)
|
||||
third = controller.after_call("terminal", {"command": "cmd-3"}, '{"exit_code":1}', failed=True)
|
||||
fourth = controller.after_call("terminal", {"command": "cmd-4"}, '{"exit_code":1}', failed=True)
|
||||
|
||||
assert first.action == "allow"
|
||||
assert [second.action, third.action, fourth.action] == ["warn", "warn", "warn"]
|
||||
assert {second.code, third.code, fourth.code} == {"same_tool_failure_warning"}
|
||||
assert controller.halt_decision is None
|
||||
|
||||
|
||||
def test_hard_stop_enabled_halts_same_tool_varying_args_failure_streak():
|
||||
controller = ToolCallGuardrailController(
|
||||
ToolCallGuardrailConfig(
|
||||
hard_stop_enabled=True,
|
||||
exact_failure_block_after=99,
|
||||
same_tool_failure_warn_after=2,
|
||||
same_tool_failure_halt_after=3,
|
||||
@@ -93,13 +168,34 @@ def test_same_tool_varying_args_failure_streak_warns_then_halts_independent_of_e
|
||||
assert third.count == 3
|
||||
|
||||
|
||||
def test_idempotent_no_progress_repeated_result_warns_then_blocks_future_repeat():
|
||||
def test_idempotent_no_progress_repeated_result_warns_without_blocking_by_default():
|
||||
controller = ToolCallGuardrailController(
|
||||
ToolCallGuardrailConfig(no_progress_warn_after=2, no_progress_block_after=2)
|
||||
)
|
||||
args = {"path": "/tmp/same.txt"}
|
||||
result = "same file contents"
|
||||
|
||||
for _ in range(4):
|
||||
assert controller.before_call("read_file", args).action == "allow"
|
||||
decision = controller.after_call("read_file", args, result, failed=False)
|
||||
|
||||
assert decision.action == "warn"
|
||||
assert decision.code == "idempotent_no_progress_warning"
|
||||
assert controller.before_call("read_file", args).action == "allow"
|
||||
assert controller.halt_decision is None
|
||||
|
||||
|
||||
def test_hard_stop_enabled_blocks_idempotent_no_progress_future_repeat():
|
||||
controller = ToolCallGuardrailController(
|
||||
ToolCallGuardrailConfig(
|
||||
hard_stop_enabled=True,
|
||||
no_progress_warn_after=2,
|
||||
no_progress_block_after=2,
|
||||
)
|
||||
)
|
||||
args = {"path": "/tmp/same.txt"}
|
||||
result = "same file contents"
|
||||
|
||||
assert controller.before_call("read_file", args).action == "allow"
|
||||
assert controller.after_call("read_file", args, result, failed=False).action == "allow"
|
||||
assert controller.before_call("read_file", args).action == "allow"
|
||||
@@ -126,7 +222,7 @@ def test_mutating_or_unknown_tools_are_not_blocked_for_repeated_identical_succes
|
||||
|
||||
def test_reset_for_turn_clears_bounded_guardrail_state():
|
||||
controller = ToolCallGuardrailController(
|
||||
ToolCallGuardrailConfig(exact_failure_block_after=2, no_progress_block_after=2)
|
||||
ToolCallGuardrailConfig(hard_stop_enabled=True, exact_failure_block_after=2, no_progress_block_after=2)
|
||||
)
|
||||
controller.after_call("web_search", {"query": "same"}, '{"error":"boom"}', failed=True)
|
||||
controller.after_call("web_search", {"query": "same"}, '{"error":"boom"}', failed=True)
|
||||
|
||||
@@ -36,10 +36,11 @@ def _mock_response(content="Hello", finish_reason="stop", tool_calls=None):
|
||||
return SimpleNamespace(choices=[choice], model="test/model", usage=None)
|
||||
|
||||
|
||||
def _make_agent(*tool_names: str, max_iterations: int = 10) -> AIAgent:
|
||||
def _make_agent(*tool_names: str, max_iterations: int = 10, config: dict | None = None) -> AIAgent:
|
||||
with (
|
||||
patch("run_agent.get_tool_definitions", return_value=_make_tool_defs(*tool_names)),
|
||||
patch("run_agent.check_toolset_requirements", return_value={}),
|
||||
patch("hermes_cli.config.load_config", return_value=config or {}),
|
||||
patch("run_agent.OpenAI"),
|
||||
):
|
||||
agent = AIAgent(
|
||||
@@ -69,7 +70,23 @@ def _seed_exact_failures(agent: AIAgent, tool_name: str, args: dict, count: int
|
||||
)
|
||||
|
||||
|
||||
def test_sequential_path_blocks_repeated_exact_failure_before_execution():
|
||||
def _hard_stop_config(**overrides) -> dict:
|
||||
cfg = {
|
||||
"tool_loop_guardrails": {
|
||||
"warnings_enabled": True,
|
||||
"hard_stop_enabled": True,
|
||||
"hard_stop_after": {
|
||||
"exact_failure": 2,
|
||||
"same_tool_failure": 8,
|
||||
"idempotent_no_progress": 5,
|
||||
},
|
||||
}
|
||||
}
|
||||
cfg["tool_loop_guardrails"].update(overrides)
|
||||
return cfg
|
||||
|
||||
|
||||
def test_default_sequential_path_warns_repeated_exact_failure_without_blocking_execution():
|
||||
agent = _make_agent("web_search")
|
||||
args = {"query": "same"}
|
||||
_seed_exact_failures(agent, "web_search", args)
|
||||
@@ -77,6 +94,32 @@ def test_sequential_path_blocks_repeated_exact_failure_before_execution():
|
||||
progress = []
|
||||
agent.tool_start_callback = lambda *a, **k: starts.append((a, k))
|
||||
agent.tool_progress_callback = lambda *a, **k: progress.append((a, k))
|
||||
tc = _mock_tool_call("web_search", json.dumps(args), "c-soft")
|
||||
msg = SimpleNamespace(content="", tool_calls=[tc])
|
||||
messages = []
|
||||
|
||||
with patch("run_agent.handle_function_call", return_value=json.dumps({"error": "boom"})) as mock_hfc:
|
||||
agent._execute_tool_calls_sequential(msg, messages, "task-1")
|
||||
|
||||
mock_hfc.assert_called_once()
|
||||
assert len(starts) == 1
|
||||
assert any(event[0][0] == "tool.completed" for event in progress)
|
||||
assert len(messages) == 1
|
||||
assert messages[0]["role"] == "tool"
|
||||
assert messages[0]["tool_call_id"] == "c-soft"
|
||||
assert "repeated_exact_failure_warning" in messages[0]["content"]
|
||||
assert "repeated_exact_failure_block" not in messages[0]["content"]
|
||||
assert agent._tool_guardrail_halt_decision is None
|
||||
|
||||
|
||||
def test_config_enabled_hard_stop_blocks_repeated_exact_failure_before_execution():
|
||||
agent = _make_agent("web_search", config=_hard_stop_config())
|
||||
args = {"query": "same"}
|
||||
_seed_exact_failures(agent, "web_search", args)
|
||||
starts = []
|
||||
progress = []
|
||||
agent.tool_start_callback = lambda *a, **k: starts.append((a, k))
|
||||
agent.tool_progress_callback = lambda *a, **k: progress.append((a, k))
|
||||
tc = _mock_tool_call("web_search", json.dumps(args), "c-block")
|
||||
msg = SimpleNamespace(content="", tool_calls=[tc])
|
||||
messages = []
|
||||
@@ -106,12 +149,12 @@ def test_sequential_after_call_appends_guidance_to_tool_result_without_extra_mes
|
||||
|
||||
assert [m["role"] for m in messages] == ["tool"]
|
||||
assert messages[0]["tool_call_id"] == "c-warn"
|
||||
assert "Tool guardrail" in messages[0]["content"]
|
||||
assert "Tool loop warning" in messages[0]["content"]
|
||||
assert "repeated_exact_failure_warning" in messages[0]["content"]
|
||||
|
||||
|
||||
def test_concurrent_path_does_not_submit_blocked_calls_and_preserves_result_order():
|
||||
agent = _make_agent("web_search")
|
||||
def test_config_enabled_hard_stop_concurrent_path_does_not_submit_blocked_calls_and_preserves_result_order():
|
||||
agent = _make_agent("web_search", config=_hard_stop_config())
|
||||
blocked_args = {"query": "blocked"}
|
||||
allowed_args = {"query": "allowed"}
|
||||
_seed_exact_failures(agent, "web_search", blocked_args)
|
||||
@@ -164,9 +207,39 @@ def test_plugin_pre_tool_block_wins_without_counting_as_toolguard_block():
|
||||
assert agent._tool_guardrails.before_call("web_search", args).action == "allow"
|
||||
|
||||
|
||||
def test_run_conversation_returns_controlled_guardrail_halt_without_top_level_error():
|
||||
def test_default_run_conversation_warns_without_guardrail_halt():
|
||||
agent = _make_agent("web_search", max_iterations=10)
|
||||
same_args = {"query": "same"}
|
||||
responses = [
|
||||
_mock_response(
|
||||
content="",
|
||||
finish_reason="tool_calls",
|
||||
tool_calls=[_mock_tool_call("web_search", json.dumps(same_args), f"c{i}")],
|
||||
)
|
||||
for i in range(1, 4)
|
||||
]
|
||||
responses.append(_mock_response(content="done", finish_reason="stop", tool_calls=None))
|
||||
agent.client.chat.completions.create.side_effect = responses
|
||||
|
||||
with (
|
||||
patch("run_agent.handle_function_call", return_value=json.dumps({"error": "boom"})) as mock_hfc,
|
||||
patch.object(agent, "_persist_session"),
|
||||
patch.object(agent, "_save_trajectory"),
|
||||
patch.object(agent, "_cleanup_task_resources"),
|
||||
):
|
||||
result = agent.run_conversation("search repeatedly")
|
||||
|
||||
assert mock_hfc.call_count == 3
|
||||
assert result["turn_exit_reason"].startswith("text_response")
|
||||
assert "guardrail" not in result
|
||||
assert result["final_response"] == "done"
|
||||
tool_contents = [m["content"] for m in result["messages"] if m.get("role") == "tool"]
|
||||
assert any("repeated_exact_failure_warning" in content for content in tool_contents)
|
||||
|
||||
|
||||
def test_config_enabled_hard_stop_run_conversation_returns_controlled_guardrail_halt_without_top_level_error():
|
||||
agent = _make_agent("web_search", max_iterations=10, config=_hard_stop_config())
|
||||
same_args = {"query": "same"}
|
||||
responses = [
|
||||
_mock_response(
|
||||
content="",
|
||||
|
||||
Reference in New Issue
Block a user