diff --git a/agent/tool_guardrails.py b/agent/tool_guardrails.py index 489b6b05d7..aa35790f82 100644 --- a/agent/tool_guardrails.py +++ b/agent/tool_guardrails.py @@ -393,10 +393,12 @@ class ToolCallGuardrailController: if self._progress_since_failure.pop(signature, False): self._exact_failure_counts.pop(signature, None) # A throttled call retried after its cooldown is the correct move, not a replay: - # keep it out of the exact-args streak (whose warning says "stop retrying unchanged"). + # keep it out of the exact-args streak and never give it that streak's "stop retrying + # unchanged" warning, even after earlier ordinary failures with the same args. # It still counts per tool below, so same_tool_failure_halt bounds a dead endpoint. + throttled = is_rate_limited(result) exact_count = self._exact_failure_counts.get(signature, 0) - if not is_rate_limited(result): + if not throttled: exact_count = self._exact_failure_counts[signature] = exact_count + 1 same_count = self._same_tool_failure_counts[tool_name] = self._same_tool_failure_counts.get(tool_name, 0) + 1 self._no_progress.pop(signature, None) @@ -414,7 +416,7 @@ class ToolCallGuardrailController: and same_count >= self.config.same_tool_failure_halt_after ): return self._decide("halt", "same_tool_failure_halt", tool_name, same_count, signature) - if warnings and exact_count >= self.config.exact_failure_warn_after: + if warnings and not throttled and exact_count >= self.config.exact_failure_warn_after: return self._decide("warn", "repeated_exact_failure_warning", tool_name, exact_count, signature) if warnings and same_count >= self.config.same_tool_failure_warn_after: return self._decide( diff --git a/tests/agent/test_tool_guardrails.py b/tests/agent/test_tool_guardrails.py index b9368d7aea..daf989cf86 100644 --- a/tests/agent/test_tool_guardrails.py +++ b/tests/agent/test_tool_guardrails.py @@ -482,21 +482,27 @@ def test_execute_code_replay_streak_notice_fires_on_warn_only_desktop_config(): assert controller.halt_decision is None, "warn-only surfaces must not halt" -@pytest.mark.parametrize("result,second_attempt", [ - (json.dumps({"error": "Slack API error: ratelimited. Retry after 1 seconds."}), "allow"), - (json.dumps({"ok": False, "error": "busy", "retry_after": 30}), "allow"), - (json.dumps({"error": "channel_not_found"}), "warn"), # control: a real replay still warns +@pytest.mark.parametrize("prior_ordinary_failures", [0, 2]) +@pytest.mark.parametrize("result,throttled", [ + (json.dumps({"error": "Slack API error: ratelimited. Retry after 1 seconds."}), True), + (json.dumps({"ok": False, "error": "busy", "retry_after": 30}), True), + (json.dumps({"error": "channel_not_found"}), False), # control: a real replay still warns ]) -def test_rate_limited_retry_is_not_a_replay_but_stays_bounded(result, second_attempt): +def test_rate_limited_retry_is_not_a_replay_but_stays_bounded(result, throttled, prior_ordinary_failures): """Retrying a throttled read after its cooldown must not draw the "stop retrying unchanged" - warning, yet an endpoint that never recovers still halts unattended runs on the per-tool cap.""" + warning (even after earlier ordinary failures with the same args), yet an endpoint that never + recovers still halts unattended runs on the per-tool cap.""" controller = ToolCallGuardrailController(ToolCallGuardrailConfig.from_mapping({}, platform="webhook")) - args = {"channel": "C123"} - actions = [] + tool, args = "mcp_slack_history", {"channel": "C123"} + for _ in range(prior_ordinary_failures): + controller.after_call(tool, args, json.dumps({"error": "upstream timeout"})) + controller.after_call(tool, args, result) + retry = controller.after_call(tool, args, result) + assert (retry.code == "repeated_exact_failure_warning") is not throttled, retry for _ in range(10): - if not controller.before_call("mcp_slack_history", args).allows_execution: - actions.append("block") + if not controller.before_call(tool, args).allows_execution: break - actions.append(controller.after_call("mcp_slack_history", args, result).action) - assert actions[1] == second_attempt - assert actions[-1] in {"halt", "block"}, actions + if controller.after_call(tool, args, result).action == "halt": + break + else: + raise AssertionError("a permanently failing endpoint must still halt or block")