From 1c04f1ec752fd3d334151a85609348dfdad4d0dc Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Mon, 29 Jun 2026 10:12:37 -0400 Subject: [PATCH 1/5] =?UTF-8?q?feat(agent):=20stuck/runaway=20guard=20?= =?UTF-8?q?=E2=80=94=20break=20a=20repeating-failing-command=20loop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live-caught ABCA-483: a one-line README task burned ALL 100 turns (~22 min, $1.53) because the agent re-ran the SAME failing command (mise //cdk:test → JS-heap OOM, exit 134) over and over, yak-shaving the build env instead of finishing. Nothing noticed until the hard max_turns cap killed it. New `stuck_guard.py` (pure, dep-free): PostToolUse records each tool result; a coarse (tool_name, command) signature tracks consecutive failures. A new between-turns hook (registered after cancel, before nudge — same short-circuit rationale) then: - STEER once at 3 repeats: inject 'stop retrying X — work around it or finish with what you have'; - BAIL at 6 repeats: end the turn loop early (continue_=False) so the task fails fast instead of grinding to the cap. Conservative by design: keys on a REPEATING FAILURE (not a raw turn count), so a large task making varied progress never trips it; unknown tool output counts as success. Fail-open everywhere — a guard bug can't wedge the agent. Platform-agnostic, MAIN-BOUND: lives entirely in agent/src (stuck_guard.py + hooks.py); surfaces via the channel-neutral error_message. The honest reason is then rendered per-channel by the classifier / failure-reply (several fixes). 1158 agent tests pass (ruff + ty clean). --- agent/src/hooks.py | 100 ++++++++++++++- agent/src/stuck_guard.py | 213 ++++++++++++++++++++++++++++++++ agent/tests/test_hooks.py | 74 +++++++++++ agent/tests/test_nudge_hook.py | 16 ++- agent/tests/test_stuck_guard.py | 126 +++++++++++++++++++ 5 files changed, 520 insertions(+), 9 deletions(-) create mode 100644 agent/src/stuck_guard.py create mode 100644 agent/tests/test_stuck_guard.py diff --git a/agent/src/hooks.py b/agent/src/hooks.py index 6ecf7299..e0e442f8 100644 --- a/agent/src/hooks.py +++ b/agent/src/hooks.py @@ -34,6 +34,7 @@ from policy import APPROVAL_RATE_LIMIT, FLOOR_TIMEOUT_S, Outcome from progress_writer import _generate_ulid from shell import log, log_error_cw +from stuck_guard import StuckGuard if TYPE_CHECKING: from policy import PolicyEngine @@ -1039,6 +1040,7 @@ async def post_tool_use_hook( *, trajectory: _TrajectoryWriter | None = None, progress: Any = None, + stuck_guard: StuckGuard | None = None, ) -> dict: """PostToolUse hook: screen tool output for secrets/PII. @@ -1051,6 +1053,11 @@ async def post_tool_use_hook( present, an egress-denial signature in the tool output emits an ``egress_denied`` blocker event (#251) — best-effort observability, never alters the screening decision. + + K7: when a ``stuck_guard`` is supplied, every tool result is recorded so a + between-turns hook can detect a repeating failing command (the ABCA-483 + spin loop) and steer / bail. Recording is best-effort and never alters the + screening outcome. """ _PASS_THROUGH: dict = {"hookSpecificOutput": {"hookEventName": "PostToolUse"}} _FAIL_CLOSED: dict = { @@ -1113,6 +1120,14 @@ async def post_tool_use_hook( resource=host, ) + # K7: feed the stuck-guard (best-effort — a tracking error must never block + # the screening path that follows). + if stuck_guard is not None: + try: + stuck_guard.record_tool_result(tool_name, hook_input.get("tool_input"), tool_response) + except Exception as exc: + log("WARN", f"stuck-guard record raised (ignored): {type(exc).__name__}: {exc}") + try: result = scan_tool_output(tool_response) except Exception as exc: @@ -1395,6 +1410,47 @@ def _cancel_between_turns_hook(ctx: dict) -> list[str]: return [] +def _stuck_guard_between_turns_hook(ctx: dict) -> list[str]: + """K7: break a repeating-failing-command spin loop (live-caught ABCA-483). + + Reads the per-task :class:`StuckGuard` stamped on ``ctx`` (by + :func:`stop_hook`). When the same command has failed enough times in a row, + the guard returns: + - ``steer`` → inject a one-time message telling the agent to stop + retrying and either work around it or finish (returned as text, so the + SDK continues the turn with the steer as the next user message); OR + - ``bail`` → set ``ctx["_stuck_bail"]`` + a reason so :func:`stop_hook` + ends the turn loop (``continue_=False``) with an honest failure instead + of grinding to ``max_turns``. + + Runs AFTER cancel (cancel wins — never steer a dying agent) but its own + no-op-when-cancelled guard makes ordering robust. Fail-open: any error is + swallowed so a guard bug can never wedge a healthy agent. + """ + if ctx.get("_cancel_requested"): + return [] + guard = ctx.get("stuck_guard") + if guard is None: + return [] + try: + action = guard.evaluate() + except Exception as exc: + log("WARN", f"stuck-guard evaluate raised (ignored): {type(exc).__name__}: {exc}") + return [] + + if action.kind == "bail": + ctx["_stuck_bail"] = True + ctx["_stuck_bail_reason"] = action.message + _emit_nudge_milestone(ctx, "stuck_bail", action.message[:_NUDGE_PREVIEW_LEN]) + log("WARN", f"stuck-guard BAIL: {action.message}") + return [] + if action.kind == "steer": + _emit_nudge_milestone(ctx, "stuck_steer", action.message[:_NUDGE_PREVIEW_LEN]) + log("NUDGE", f"stuck-guard STEER injected: {action.signature}") + return [action.message] + return [] + + # Global list of between-turns hooks. Cancel MUST run first so it can # short-circuit nudges on cancelled tasks (no point injecting nudges into a # dying agent — worse, the nudge reader mutates DDB state that the agent will @@ -1406,6 +1462,10 @@ def _cancel_between_turns_hook(ctx: dict) -> list[str]: # nudge reader to preserve cancel-wins semantics. between_turns_hooks: list[BetweenTurnsHook] = [ _cancel_between_turns_hook, + # K7 stuck-guard runs early (right after cancel): if it decides to BAIL, + # the stop_hook loop should halt before the nudge/denial hooks mutate any + # DDB state for an agent we're about to stop (same rationale as cancel). + _stuck_guard_between_turns_hook, _nudge_between_turns_hook, # Chunk 3 (finding #2): denial injection runs LAST so both cancel and # nudge short-circuits pre-empt it. The hook explicitly re-checks @@ -1423,6 +1483,7 @@ async def stop_hook( task_id: str, progress: Any = None, engine: Any = None, + stuck_guard: Any = None, ) -> dict: """Stop hook: run registered between-turns hooks; block if they produce text. @@ -1445,6 +1506,7 @@ async def stop_hook( "task_id": task_id, "progress": progress, "engine": engine, + "stuck_guard": stuck_guard, } # Cancel-before-nudge short-circuit. @@ -1473,11 +1535,11 @@ async def stop_hook( continue if produced: chunks.extend(produced) - if ctx.get("_cancel_requested"): + if ctx.get("_cancel_requested") or ctx.get("_stuck_bail"): # Any text produced by earlier hooks in this same loop iteration - # is discarded below — the ``_cancel_requested`` branch returns - # ``continue_=False`` and never reads ``chunks``. This is - # intentional: cancel wins, and we would rather drop a + # is discarded below — the cancel / stuck-bail branches return + # ``continue_=False`` and never read ``chunks``. This is + # intentional: a halt wins, and we would rather drop a # simultaneous nudge than inject into a dying agent. break @@ -1490,6 +1552,23 @@ async def stop_hook( "stopReason": "Task cancelled by user", } + # K7: the stuck-guard decided the agent is in a hopeless retry loop. End the + # turn loop NOW (``continue_=False``) instead of grinding to ``max_turns`` — + # the primary win is stopping ~16 turns in rather than 100 (the ABCA-483 + # grind was 22 min / $1.53). Unlike cancel, no external writer has stamped a + # terminal status, so the FINAL status is whatever the post-hooks + SDK + # resolve (typically a build-gate failure, surfaced honestly by the + # platform's failure-reply). The ``stopReason`` is logged for the trace; the + # one-time steer injected earlier already told the agent to finish with a + # summary of what failed. We deliberately do NOT force-write a terminal + # status here — racing the pipeline's own ``write_terminal`` is riskier than + # the marginal benefit, and the early stop is the real value. + if ctx.get("_stuck_bail"): + return { + "continue_": False, + "stopReason": ctx.get("_stuck_bail_reason") or "Stuck: repeated failing command", + } + if not chunks: return {} @@ -1526,6 +1605,11 @@ def build_hook_matchers( SyncHookJSONOutput, ) + # K7: one stuck-guard per task (== per build_hook_matchers call). The + # PostToolUse closure feeds it every tool result; the Stop closure reads it + # between turns to steer / bail on a repeating failing command. + _stuck_guard = StuckGuard() + # Closure-based wrapper matches the HookCallback signature exactly: # (HookInput, str | None, HookContext) -> Awaitable[HookJSONOutput] async def _pre( @@ -1568,7 +1652,12 @@ async def _post( ) -> HookJSONOutput: try: result = await post_tool_use_hook( - hook_input, tool_use_id, ctx, trajectory=trajectory, progress=progress + hook_input, + tool_use_id, + ctx, + trajectory=trajectory, + progress=progress, + stuck_guard=_stuck_guard, ) return SyncHookJSONOutput(**result) except Exception as exc: @@ -1593,6 +1682,7 @@ async def _stop( task_id=stop_task_id, progress=progress, engine=engine, + stuck_guard=_stuck_guard, ) except Exception as exc: log( diff --git a/agent/src/stuck_guard.py b/agent/src/stuck_guard.py new file mode 100644 index 00000000..afed01fd --- /dev/null +++ b/agent/src/stuck_guard.py @@ -0,0 +1,213 @@ +"""Stuck/runaway guard — detect a repeating failing tool call and steer/bail. + +Live-caught (ABCA-483, 2026-06-29): a one-line README task burned all 100 turns +(~22 min, $1.53) because the agent re-ran the SAME failing command +(``mise //cdk:test`` → JS-heap OOM, exit 134) over and over, yak-shaving the +build environment instead of finishing the task. Nothing noticed the loop until +the hard ``max_turns`` cap killed it — by which point the user had stared at a +silent issue for 22 minutes. + +This module gives the agent a cheap, precise loop-breaker: + + 1. ``record_tool_result`` is called from the PostToolUse hook for every tool + call. It computes a coarse SIGNATURE — ``(tool_name, normalized command)`` + — and tracks how many times that exact signature has just FAILED in a row. + A success (or a different signature) resets the streak for that signature. + + 2. ``evaluate`` is called from a between-turns (Stop) hook. When any signature + has failed ``STEER_THRESHOLD`` times in a row it returns a STEER action + (inject a one-time message telling the agent to stop retrying and either + work around the failure or finish with what it has). If the SAME signature + keeps failing up to ``BAIL_THRESHOLD`` after steering, it returns a BAIL + action (end the turn loop with a clear reason) so the task fails fast with + an honest message instead of grinding to the turn cap. + +Design choices (deliberately conservative — over-steering is cheap, a +false-positive bail is not): + + - We key on a REPEATING FAILURE, not a raw turn count. A genuinely large task + that makes steady progress (each turn a different, succeeding tool call) + never trips this — only a true spin loop does. + - "Failure" is detected from the tool RESPONSE text via small, well-known + signals (non-zero exit, "command not found", OOM/heap markers, common error + prefixes). We err toward NOT flagging — an unrecognized response counts as + success so we never punish a healthy turn. + - Steering is injected at most ONCE per signature (a process-lifetime dedup + set), mirroring the nudge hook's ``_INJECTED_NUDGES`` guard, so we don't + re-steer every turn. + +Pure + dependency-free (no boto3 / SDK imports) so it unit-tests trivially; the +hook wiring in ``hooks.py`` owns all I/O. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass, field + +# Consecutive identical failures before we INJECT a steering message. Three is +# enough to distinguish a real loop ("I keep running the same broken command") +# from a normal retry-after-fix ("ran it, fixed a thing, ran it again"). +STEER_THRESHOLD = 3 + +# Consecutive identical failures (total) before we BAIL the task. Past this the +# agent has ignored the steer and is still spinning — fail fast with a reason +# rather than burn the rest of the turn budget. +BAIL_THRESHOLD = 6 + +# Max chars of the offending command surfaced in the steer/bail message. Short: +# this is a hint, not a log dump (and the command is untrusted repo content). +_CMD_PREVIEW_LEN = 80 + +# Substrings that mark a tool response as a FAILURE. Conservative + well-known; +# an unrecognized response is treated as success (never punish a healthy turn). +_FAILURE_MARKERS = ( + "command not found", + "no such file or directory", + "exit code 1", + "exit code 2", + "exit code 127", + "exit 134", # SIGABRT (OOM/abort) — the ABCA-483 signal + "exit 137", # SIGKILL (OOM-killer) + "fatal:", + "javascript heap out of memory", + "out of memory", + "allocation failure", + "traceback (most recent call last)", + "error: failed to push", +) + +# A reported exit code embedded in the response, e.g. "FAILED (exit 134)" or +# "Exit code 1". A non-zero match is a failure signal. +_EXIT_CODE_RE = re.compile(r"\bexit(?:\s+code)?\s+(\d+)\b", re.IGNORECASE) + + +def _signature(tool_name: str, tool_input: object) -> str: + """Coarse, stable signature for a tool call: ``tool_name|normalized-cmd``. + + For Bash, the command drives the signature (whitespace-collapsed); other + tools fall back to their name + a normalized repr of the input so that + e.g. editing the same file repeatedly is also detectable. The signature is + intentionally coarse: we want "the agent keeps doing the same thing", not + byte-exact identity. + """ + cmd = "" + if isinstance(tool_input, dict): + cmd = str(tool_input.get("command") or tool_input.get("file_path") or "") + elif isinstance(tool_input, str): + cmd = tool_input + normalized = re.sub(r"\s+", " ", cmd).strip().lower() + return f"{tool_name}|{normalized}" + + +def _command_preview(tool_input: object) -> str: + """Short human preview of the offending command for the steer/bail text.""" + cmd = "" + if isinstance(tool_input, dict): + cmd = str(tool_input.get("command") or tool_input.get("file_path") or "") + elif isinstance(tool_input, str): + cmd = tool_input + cmd = re.sub(r"\s+", " ", cmd).strip() + if not cmd: + return "the same operation" + return cmd if len(cmd) <= _CMD_PREVIEW_LEN else cmd[: _CMD_PREVIEW_LEN - 1] + "…" + + +def _looks_failed(tool_response: str) -> bool: + """Heuristic: did this tool call fail? Conservative (unknown → not failed).""" + s = (tool_response or "").lower() + if any(marker in s for marker in _FAILURE_MARKERS): + return True + m = _EXIT_CODE_RE.search(s) + if m: + try: + return int(m.group(1)) != 0 + except ValueError: + return False + return False + + +@dataclass +class _SigState: + """Per-signature streak tracking.""" + + fail_streak: int = 0 + last_preview: str = "" + + +@dataclass +class StuckAction: + """What the between-turns hook should do this turn.""" + + kind: str # 'none' | 'steer' | 'bail' + signature: str = "" + message: str = "" + + +@dataclass +class StuckGuard: + """Tracks repeating failing tool calls for ONE task (process-lifetime). + + Not thread-safe by itself; the agent's hook callbacks for a single task run + serially on the asyncio loop / one PostToolUse at a time, which is the only + access pattern. One instance per task. + """ + + _sigs: dict[str, _SigState] = field(default_factory=dict) + _steered: set[str] = field(default_factory=set) + _last_failing_sig: str | None = None + + def record_tool_result(self, tool_name: str, tool_input: object, tool_response: str) -> None: + """Called from PostToolUse for every tool call. Updates failure streaks.""" + sig = _signature(tool_name, tool_input) + state = self._sigs.setdefault(sig, _SigState()) + if _looks_failed(tool_response): + state.fail_streak += 1 + state.last_preview = _command_preview(tool_input) + self._last_failing_sig = sig + else: + # A success on this signature breaks ITS streak. We don't reset + # other signatures — an A/B/A/B flip-flop between two failing + # commands still accrues on each independently. + state.fail_streak = 0 + if self._last_failing_sig == sig: + self._last_failing_sig = None + + def evaluate(self) -> StuckAction: + """Called from a between-turns hook. Decide steer / bail / none. + + BAIL takes precedence over STEER. STEER fires at most once per signature. + """ + sig = self._last_failing_sig + if not sig: + return StuckAction(kind="none") + state = self._sigs.get(sig) + if state is None: + return StuckAction(kind="none") + + if state.fail_streak >= BAIL_THRESHOLD: + return StuckAction( + kind="bail", + signature=sig, + message=( + f"Stuck: `{state.last_preview}` failed {state.fail_streak} times in a row " + "and the agent kept retrying it instead of making progress on the task." + ), + ) + + if state.fail_streak >= STEER_THRESHOLD and sig not in self._steered: + self._steered.add(sig) + return StuckAction( + kind="steer", + signature=sig, + message=( + f"⚠️ You have run `{state.last_preview}` and it has failed " + f"{state.fail_streak} times in a row. STOP retrying it. Either (a) work " + "around the failure (e.g. a different command, or skip the failing step if " + "it is an environment/tooling problem rather than your code), or (b) if you " + "cannot, finish now with what you have and clearly state in your summary what " + "failed and why. Do not run the same failing command again." + ), + ) + + return StuckAction(kind="none") diff --git a/agent/tests/test_hooks.py b/agent/tests/test_hooks.py index bffcb36f..0a818be9 100644 --- a/agent/tests/test_hooks.py +++ b/agent/tests/test_hooks.py @@ -1697,3 +1697,77 @@ def test_missing_started_at_returns_none(self, monkeypatch): def test_unparseable_started_at_returns_none(self, monkeypatch): monkeypatch.setenv("TASK_STARTED_AT", "not-a-timestamp") assert hooks._remaining_maxlifetime_s() is None + + +class TestStuckGuardHookIntegration: + """K7: PostToolUse feeds the guard; the between-turns hook steers/bails.""" + + def _oom(self): + return "[//cdk:test] FAILED (exit 134)\nJavaScript heap out of memory" + + def test_post_tool_use_records_failures_into_the_guard(self): + from stuck_guard import STEER_THRESHOLD, StuckGuard + + guard = StuckGuard() + cmd = {"command": "mise //cdk:test"} + for _ in range(STEER_THRESHOLD): + hook_input = { + "hook_event_name": "PostToolUse", + "tool_name": "Bash", + "tool_input": cmd, + "tool_response": self._oom(), + } + _run(post_tool_use_hook(hook_input, "t", {}, stuck_guard=guard)) + # the guard now has enough failures to steer + assert guard.evaluate().kind == "steer" + + def test_post_tool_use_record_error_never_blocks_screening(self): + # A guard that raises on record must not break the PASS_THROUGH path. + from stuck_guard import StuckGuard + + class _Boom(StuckGuard): + def record_tool_result(self, *a, **k): + raise RuntimeError("boom") + + hook_input = { + "hook_event_name": "PostToolUse", + "tool_name": "Bash", + "tool_input": {"command": "echo hi"}, + "tool_response": "hi", + } + result = _run(post_tool_use_hook(hook_input, "t", {}, stuck_guard=_Boom())) + assert result["hookSpecificOutput"]["hookEventName"] == "PostToolUse" + + def test_stop_hook_bails_when_guard_says_so(self): + from stuck_guard import BAIL_THRESHOLD, StuckGuard + + guard = StuckGuard() + cmd = {"command": "mise //cdk:test"} + for _ in range(BAIL_THRESHOLD): + guard.record_tool_result("Bash", cmd, self._oom()) + result = _run(hooks.stop_hook({}, None, {}, task_id="t", stuck_guard=guard)) + # continue_=False ends the turn loop; stopReason carries the honest reason + assert result.get("continue_") is False + assert "Stuck" in (result.get("stopReason") or "") + + def test_stop_hook_steers_when_guard_says_so(self): + from stuck_guard import STEER_THRESHOLD, StuckGuard + + guard = StuckGuard() + cmd = {"command": "mise //cdk:test"} + for _ in range(STEER_THRESHOLD): + guard.record_tool_result("Bash", cmd, self._oom()) + result = _run(hooks.stop_hook({}, None, {}, task_id="t", stuck_guard=guard)) + # a steer is injected as a block decision (SDK continues with the text) + assert result.get("decision") == "block" + assert "STOP retrying" in result.get("reason", "") + + def test_stop_hook_no_guard_is_a_noop(self): + # Back-compat: absent a guard, the stuck path never fires. + result = _run(hooks.stop_hook({}, None, {}, task_id="t")) + assert result == {} + + def test_build_hook_matchers_creates_a_guard_without_crashing(self): + engine = PolicyEngine(task_type="new_task", repo="owner/repo") + matchers = build_hook_matchers(engine, task_id="t") + assert "PostToolUse" in matchers and "Stop" in matchers diff --git a/agent/tests/test_nudge_hook.py b/agent/tests/test_nudge_hook.py index a23a20c2..b116563f 100644 --- a/agent/tests/test_nudge_hook.py +++ b/agent/tests/test_nudge_hook.py @@ -319,13 +319,21 @@ def test_multiple_hooks_joined(self): assert "three" in result["reason"] def test_registry_default_contains_cancel_then_nudge(self): - # Freshly-imported registry: cancel runs first so it short-circuits - # nudge injection on cancelled tasks; nudge second for running tasks. + # Freshly-imported registry: cancel runs FIRST so it short-circuits + # nudge injection on cancelled tasks; nudge runs AFTER it for running + # tasks. The K7 stuck-guard is inserted between them (it also wants to + # short-circuit before the nudge reader mutates DDB on a bail), so the + # invariant we assert is the relative ORDER (cancel < stuck-guard < + # nudge), not exact adjacency. import importlib importlib.reload(hooks_mod) - assert hooks_mod.between_turns_hooks[0] is hooks_mod._cancel_between_turns_hook - assert hooks_mod.between_turns_hooks[1] is hooks_mod._nudge_between_turns_hook + reg = hooks_mod.between_turns_hooks + i_cancel = reg.index(hooks_mod._cancel_between_turns_hook) + i_stuck = reg.index(hooks_mod._stuck_guard_between_turns_hook) + i_nudge = reg.index(hooks_mod._nudge_between_turns_hook) + assert i_cancel == 0 + assert i_cancel < i_stuck < i_nudge class TestInProcessDedup: diff --git a/agent/tests/test_stuck_guard.py b/agent/tests/test_stuck_guard.py new file mode 100644 index 00000000..5ef89233 --- /dev/null +++ b/agent/tests/test_stuck_guard.py @@ -0,0 +1,126 @@ +"""Tests for the stuck/runaway guard (K7, live-caught ABCA-483).""" + +from __future__ import annotations + +from stuck_guard import ( + BAIL_THRESHOLD, + STEER_THRESHOLD, + StuckGuard, + _looks_failed, + _signature, +) + +OOM = "[//cdk:test] FAILED (exit 134)\n<--- Last few GCs --->\nJavaScript heap out of memory" +OK = "Tests passed. 2813 passed." +CMD = {"command": "MISE_EXPERIMENTAL=1 mise //cdk:test"} + + +class TestFailureDetection: + def test_oom_exit_134_is_failure(self): + assert _looks_failed(OOM) is True + + def test_command_not_found_is_failure(self): + assert _looks_failed("bash: line 1: yarn: command not found") is True + + def test_clean_output_is_not_failure(self): + assert _looks_failed(OK) is False + + def test_exit_zero_is_not_failure(self): + assert _looks_failed("done (exit 0)") is False + + def test_unrecognized_output_is_not_failure(self): + # Conservative: unknown response must not be punished as a failure. + assert _looks_failed("here is the file content you asked for") is False + + def test_empty_is_not_failure(self): + assert _looks_failed("") is False + + +class TestSignature: + def test_bash_keys_on_command_whitespace_collapsed(self): + a = _signature("Bash", {"command": "mise //cdk:test"}) + b = _signature("Bash", {"command": "mise //cdk:test"}) + assert a == b + + def test_different_commands_differ(self): + a = _signature("Bash", {"command": "yarn test"}) + b = _signature("Bash", {"command": "yarn build"}) + assert a != b + + def test_edit_keys_on_file_path(self): + a = _signature("Edit", {"file_path": "src/x.ts"}) + b = _signature("Edit", {"file_path": "src/x.ts"}) + assert a == b + + +class TestStuckGuardLifecycle: + def test_no_action_below_steer_threshold(self): + g = StuckGuard() + for _ in range(STEER_THRESHOLD - 1): + g.record_tool_result("Bash", CMD, OOM) + assert g.evaluate().kind == "none" + + def test_steers_at_threshold(self): + g = StuckGuard() + for _ in range(STEER_THRESHOLD): + g.record_tool_result("Bash", CMD, OOM) + action = g.evaluate() + assert action.kind == "steer" + assert "STOP retrying" in action.message + # the offending command is previewed + assert "mise //cdk:test" in action.message.lower() + + def test_steers_at_most_once_per_signature(self): + g = StuckGuard() + for _ in range(STEER_THRESHOLD): + g.record_tool_result("Bash", CMD, OOM) + assert g.evaluate().kind == "steer" + # same signature fails again but below bail → no second steer + g.record_tool_result("Bash", CMD, OOM) + assert g.evaluate().kind == "none" + + def test_bails_after_persistent_failure(self): + g = StuckGuard() + for _ in range(BAIL_THRESHOLD): + g.record_tool_result("Bash", CMD, OOM) + action = g.evaluate() + assert action.kind == "bail" + assert "Stuck" in action.message + + def test_success_resets_the_streak(self): + g = StuckGuard() + for _ in range(STEER_THRESHOLD - 1): + g.record_tool_result("Bash", CMD, OOM) + g.record_tool_result("Bash", CMD, OK) # fixed it + assert g.evaluate().kind == "none" + # one more failure is a fresh streak, not at threshold + g.record_tool_result("Bash", CMD, OOM) + assert g.evaluate().kind == "none" + + def test_different_failing_commands_do_not_aggregate(self): + # Two distinct commands each failing once → no trip (not the SAME loop). + g = StuckGuard() + g.record_tool_result("Bash", {"command": "a"}, OOM) + g.record_tool_result("Bash", {"command": "b"}, OOM) + g.record_tool_result("Bash", {"command": "c"}, OOM) + assert g.evaluate().kind == "none" + + def test_healthy_varied_work_never_trips(self): + # A large task: many different succeeding calls → never stuck. + g = StuckGuard() + for i in range(50): + g.record_tool_result("Bash", {"command": f"step-{i}"}, OK) + assert g.evaluate().kind == "none" + + def test_interleaved_success_on_OTHER_sig_does_not_clear_the_loop(self): + # The real loop (cmd A) keeps failing; occasional unrelated success (cmd B) + # must NOT mask it. + g = StuckGuard() + g.record_tool_result("Bash", {"command": "loop"}, OOM) + g.record_tool_result("Bash", {"command": "other"}, OK) + g.record_tool_result("Bash", {"command": "loop"}, OOM) + g.record_tool_result("Bash", {"command": "other"}, OK) + g.record_tool_result("Bash", {"command": "loop"}, OOM) + action = g.evaluate() + assert action.kind == "steer" + assert "loop" in action.message.lower() From 368da65e99d5a2744c880c3d2432ebd88c0823cb Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Mon, 29 Jun 2026 13:06:22 -0400 Subject: [PATCH 2/5] =?UTF-8?q?fix:=20stuck-guard=20is=20advisory-only=20?= =?UTF-8?q?=E2=80=94=20drop=20the=20bail,=20keep=20a=20one-time=20steer?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewing the guard for false-positive risk (user: 'make sure the turn limit isn't too aggressive') showed the bail path was unsafe: distinguishing a true spin from a legitimately-iterating agent (re-running the same test command as it fixes failures one by one) from raw output is genuinely fragile — a digit- normalizing fingerprint that ignores volatile GC timings ALSO collapses 'test file_0' vs 'file_1' (the progress signal), so it would kill a working agent. A false-positive KILL is far worse than a false-positive nudge. So: removed BAIL entirely. The guard now only ever injects a ONE-TIME advisory steer when the same command fails with IDENTICAL output N times; the max_turns cap (with a fix's honest 'Exceeded max turns' reason) is the real runaway backstop. A false positive costs exactly one extra advisory comment. Platform-agnostic, main-bound. --- agent/src/hooks.py | 57 +++++---------- agent/src/stuck_guard.py | 119 +++++++++++++++++++------------- agent/tests/test_hooks.py | 18 +++-- agent/tests/test_stuck_guard.py | 44 +++++++++--- 4 files changed, 137 insertions(+), 101 deletions(-) diff --git a/agent/src/hooks.py b/agent/src/hooks.py index e0e442f8..619b6351 100644 --- a/agent/src/hooks.py +++ b/agent/src/hooks.py @@ -1411,17 +1411,19 @@ def _cancel_between_turns_hook(ctx: dict) -> list[str]: def _stuck_guard_between_turns_hook(ctx: dict) -> list[str]: - """K7: break a repeating-failing-command spin loop (live-caught ABCA-483). + """K7: nudge the agent when it repeats the SAME failing command (ABCA-483). Reads the per-task :class:`StuckGuard` stamped on ``ctx`` (by - :func:`stop_hook`). When the same command has failed enough times in a row, - the guard returns: - - ``steer`` → inject a one-time message telling the agent to stop - retrying and either work around it or finish (returned as text, so the - SDK continues the turn with the steer as the next user message); OR - - ``bail`` → set ``ctx["_stuck_bail"]`` + a reason so :func:`stop_hook` - ends the turn loop (``continue_=False``) with an honest failure instead - of grinding to ``max_turns``. + :func:`stop_hook`). When the same command has failed with identical output + enough times in a row, the guard returns a ``steer`` action — a ONE-TIME + advisory message telling the agent to stop retrying and either work around + the failure or finish with what it has. Returned as injected text, so the + SDK continues the turn with the steer as the next user message. + + ADVISORY ONLY: the guard never kills the task (the bail path was removed — + distinguishing a true spin from a legitimately-iterating agent is too + fragile to justify an auto-kill; the ``max_turns`` cap is the real + backstop). A false positive here costs exactly one extra advisory comment. Runs AFTER cancel (cancel wins — never steer a dying agent) but its own no-op-when-cancelled guard makes ordering robust. Fail-open: any error is @@ -1438,12 +1440,6 @@ def _stuck_guard_between_turns_hook(ctx: dict) -> list[str]: log("WARN", f"stuck-guard evaluate raised (ignored): {type(exc).__name__}: {exc}") return [] - if action.kind == "bail": - ctx["_stuck_bail"] = True - ctx["_stuck_bail_reason"] = action.message - _emit_nudge_milestone(ctx, "stuck_bail", action.message[:_NUDGE_PREVIEW_LEN]) - log("WARN", f"stuck-guard BAIL: {action.message}") - return [] if action.kind == "steer": _emit_nudge_milestone(ctx, "stuck_steer", action.message[:_NUDGE_PREVIEW_LEN]) log("NUDGE", f"stuck-guard STEER injected: {action.signature}") @@ -1462,9 +1458,9 @@ def _stuck_guard_between_turns_hook(ctx: dict) -> list[str]: # nudge reader to preserve cancel-wins semantics. between_turns_hooks: list[BetweenTurnsHook] = [ _cancel_between_turns_hook, - # K7 stuck-guard runs early (right after cancel): if it decides to BAIL, - # the stop_hook loop should halt before the nudge/denial hooks mutate any - # DDB state for an agent we're about to stop (same rationale as cancel). + # K7 stuck-guard (advisory): injects a one-time "stop retrying X" nudge when + # the same command keeps failing identically. Runs after cancel (never steer + # a dying agent); order vs nudge/denial is cosmetic since it never bails. _stuck_guard_between_turns_hook, _nudge_between_turns_hook, # Chunk 3 (finding #2): denial injection runs LAST so both cancel and @@ -1535,11 +1531,11 @@ async def stop_hook( continue if produced: chunks.extend(produced) - if ctx.get("_cancel_requested") or ctx.get("_stuck_bail"): + if ctx.get("_cancel_requested"): # Any text produced by earlier hooks in this same loop iteration - # is discarded below — the cancel / stuck-bail branches return - # ``continue_=False`` and never read ``chunks``. This is - # intentional: a halt wins, and we would rather drop a + # is discarded below — the ``_cancel_requested`` branch returns + # ``continue_=False`` and never reads ``chunks``. This is + # intentional: cancel wins, and we would rather drop a # simultaneous nudge than inject into a dying agent. break @@ -1552,23 +1548,6 @@ async def stop_hook( "stopReason": "Task cancelled by user", } - # K7: the stuck-guard decided the agent is in a hopeless retry loop. End the - # turn loop NOW (``continue_=False``) instead of grinding to ``max_turns`` — - # the primary win is stopping ~16 turns in rather than 100 (the ABCA-483 - # grind was 22 min / $1.53). Unlike cancel, no external writer has stamped a - # terminal status, so the FINAL status is whatever the post-hooks + SDK - # resolve (typically a build-gate failure, surfaced honestly by the - # platform's failure-reply). The ``stopReason`` is logged for the trace; the - # one-time steer injected earlier already told the agent to finish with a - # summary of what failed. We deliberately do NOT force-write a terminal - # status here — racing the pipeline's own ``write_terminal`` is riskier than - # the marginal benefit, and the early stop is the real value. - if ctx.get("_stuck_bail"): - return { - "continue_": False, - "stopReason": ctx.get("_stuck_bail_reason") or "Stuck: repeated failing command", - } - if not chunks: return {} diff --git a/agent/src/stuck_guard.py b/agent/src/stuck_guard.py index afed01fd..51ef036c 100644 --- a/agent/src/stuck_guard.py +++ b/agent/src/stuck_guard.py @@ -11,30 +11,34 @@ 1. ``record_tool_result`` is called from the PostToolUse hook for every tool call. It computes a coarse SIGNATURE — ``(tool_name, normalized command)`` - — and tracks how many times that exact signature has just FAILED in a row. - A success (or a different signature) resets the streak for that signature. - - 2. ``evaluate`` is called from a between-turns (Stop) hook. When any signature - has failed ``STEER_THRESHOLD`` times in a row it returns a STEER action - (inject a one-time message telling the agent to stop retrying and either - work around the failure or finish with what it has). If the SAME signature - keeps failing up to ``BAIL_THRESHOLD`` after steering, it returns a BAIL - action (end the turn loop with a clear reason) so the task fails fast with - an honest message instead of grinding to the turn cap. - -Design choices (deliberately conservative — over-steering is cheap, a -false-positive bail is not): - - - We key on a REPEATING FAILURE, not a raw turn count. A genuinely large task - that makes steady progress (each turn a different, succeeding tool call) - never trips this — only a true spin loop does. - - "Failure" is detected from the tool RESPONSE text via small, well-known - signals (non-zero exit, "command not found", OOM/heap markers, common error - prefixes). We err toward NOT flagging — an unrecognized response counts as - success so we never punish a healthy turn. - - Steering is injected at most ONCE per signature (a process-lifetime dedup - set), mirroring the nudge hook's ``_INJECTED_NUDGES`` guard, so we don't - re-steer every turn. + — and tracks how many times that exact signature has just FAILED in a row + WITH THE SAME OUTPUT. A success, a different signature, or a different + failure output resets the streak. + + 2. ``evaluate`` is called from a between-turns (Stop) hook. When a signature + has failed ``STEER_THRESHOLD`` times in a row with identical output it + returns a STEER action: inject a ONE-TIME advisory message telling the + agent to stop retrying and either work around the failure or finish with + what it has. + +ADVISORY ONLY — by design this guard NEVER kills a task. An earlier version +could BAIL (end the turn loop), but distinguishing a true spin from a +legitimately-iterating agent (re-running the same test command as it fixes +failures one by one) from raw output is genuinely fragile, and a false-positive +KILL of a working agent is far worse than a false-positive nudge. So we dropped +the bail: the real runaway backstop is the platform's ``max_turns`` cap (which +now reports an honest "Exceeded max turns" reason via the error classifier). A +false-positive here costs exactly one extra advisory comment — nothing more. + +Design choices (deliberately conservative): + - Key on a REPEATING IDENTICAL FAILURE, not a raw turn count. A task making + steady progress (different tool calls, or the same command failing + DIFFERENTLY each time) never trips this — only a true spin does. + - "Failure" is detected from the tool RESPONSE via small, well-known signals + (non-zero exit, command-not-found, OOM markers). Unknown output counts as + success — we never punish a healthy turn. + - Steer at most ONCE per signature (process-lifetime dedup), mirroring the + nudge hook's ``_INJECTED_NUDGES`` guard. Pure + dependency-free (no boto3 / SDK imports) so it unit-tests trivially; the hook wiring in ``hooks.py`` owns all I/O. @@ -45,17 +49,13 @@ import re from dataclasses import dataclass, field -# Consecutive identical failures before we INJECT a steering message. Three is -# enough to distinguish a real loop ("I keep running the same broken command") -# from a normal retry-after-fix ("ran it, fixed a thing, ran it again"). +# Consecutive failures of the same command WITH IDENTICAL OUTPUT before we +# inject the one-time advisory steer. Three distinguishes a real spin ("I keep +# running the same broken command and getting the same error") from a normal +# retry-after-fix ("ran it, changed something, ran it again — different result"). STEER_THRESHOLD = 3 -# Consecutive identical failures (total) before we BAIL the task. Past this the -# agent has ignored the steer and is still spinning — fail fast with a reason -# rather than burn the rest of the turn budget. -BAIL_THRESHOLD = 6 - -# Max chars of the offending command surfaced in the steer/bail message. Short: +# Max chars of the offending command surfaced in the steer message. Short: # this is a hint, not a log dump (and the command is untrusted repo content). _CMD_PREVIEW_LEN = 80 @@ -127,19 +127,41 @@ def _looks_failed(tool_response: str) -> bool: return False +def _failure_fingerprint(tool_response: str) -> str: + """Whitespace-collapsed prefix of the failure output, used to tell a true + spin (same command, SAME error, over and over) from healthy iteration (same + command, but a DIFFERENT error each run — fixed one thing, hit the next). + + We do NOT blur digits/paths/line-numbers: an earlier version normalized + ``\\d+ → #`` to ignore volatile GC timings, but that ALSO collapsed + ``test file_0`` and ``test file_1`` to the same fingerprint — i.e. it + couldn't tell a volatile number from the meaningful "which test failed" + progress signal, and would have nudged a legitimately-iterating agent. Since + the guard is now advisory-only (a false nudge is cheap), we use the SIMPLE, + honest comparison: two failures are "the same" only if their (collapsed) + output prefix is identical. A working agent's output changes run-to-run, so + it reads as progress and never reaches the steer threshold. + """ + return re.sub(r"\s+", " ", (tool_response or "").strip())[:300] + + @dataclass class _SigState: """Per-signature streak tracking.""" fail_streak: int = 0 last_preview: str = "" + # Output fingerprint of the LAST failure on this signature. The streak only + # grows when a new failure matches it (same command failing the SAME way); a + # different failure resets to 1 (the agent made progress). + last_fingerprint: str = "" @dataclass class StuckAction: """What the between-turns hook should do this turn.""" - kind: str # 'none' | 'steer' | 'bail' + kind: str # 'none' | 'steer' (advisory only — never kills the task) signature: str = "" message: str = "" @@ -162,7 +184,18 @@ def record_tool_result(self, tool_name: str, tool_input: object, tool_response: sig = _signature(tool_name, tool_input) state = self._sigs.setdefault(sig, _SigState()) if _looks_failed(tool_response): - state.fail_streak += 1 + # Only grow the streak when the SAME command fails the SAME way. A + # different failure fingerprint means the agent made progress (fixed + # one error, hit the next) — that's healthy iteration, so reset to a + # fresh streak of 1 rather than march toward a bail. This is the + # guard against false-positives on a legitimately-iterating agent + # (e.g. re-running the test suite as it fixes failures one by one). + fp = _failure_fingerprint(tool_response) + if fp == state.last_fingerprint: + state.fail_streak += 1 + else: + state.fail_streak = 1 + state.last_fingerprint = fp state.last_preview = _command_preview(tool_input) self._last_failing_sig = sig else: @@ -170,13 +203,15 @@ def record_tool_result(self, tool_name: str, tool_input: object, tool_response: # other signatures — an A/B/A/B flip-flop between two failing # commands still accrues on each independently. state.fail_streak = 0 + state.last_fingerprint = "" if self._last_failing_sig == sig: self._last_failing_sig = None def evaluate(self) -> StuckAction: - """Called from a between-turns hook. Decide steer / bail / none. + """Called from a between-turns hook. Decide steer / none (advisory only). - BAIL takes precedence over STEER. STEER fires at most once per signature. + STEER fires at most once per signature. There is no bail — the guard + never kills a task (see module docstring). """ sig = self._last_failing_sig if not sig: @@ -185,16 +220,6 @@ def evaluate(self) -> StuckAction: if state is None: return StuckAction(kind="none") - if state.fail_streak >= BAIL_THRESHOLD: - return StuckAction( - kind="bail", - signature=sig, - message=( - f"Stuck: `{state.last_preview}` failed {state.fail_streak} times in a row " - "and the agent kept retrying it instead of making progress on the task." - ), - ) - if state.fail_streak >= STEER_THRESHOLD and sig not in self._steered: self._steered.add(sig) return StuckAction( diff --git a/agent/tests/test_hooks.py b/agent/tests/test_hooks.py index 0a818be9..13eb1b34 100644 --- a/agent/tests/test_hooks.py +++ b/agent/tests/test_hooks.py @@ -1700,7 +1700,7 @@ def test_unparseable_started_at_returns_none(self, monkeypatch): class TestStuckGuardHookIntegration: - """K7: PostToolUse feeds the guard; the between-turns hook steers/bails.""" + """K7: PostToolUse feeds the guard; the between-turns hook steers (advisory).""" def _oom(self): return "[//cdk:test] FAILED (exit 134)\nJavaScript heap out of memory" @@ -1738,17 +1738,21 @@ def record_tool_result(self, *a, **k): result = _run(post_tool_use_hook(hook_input, "t", {}, stuck_guard=_Boom())) assert result["hookSpecificOutput"]["hookEventName"] == "PostToolUse" - def test_stop_hook_bails_when_guard_says_so(self): - from stuck_guard import BAIL_THRESHOLD, StuckGuard + def test_stop_hook_steers_not_bails(self): + # Advisory-only: a persistent identical-failure spin produces a STEER + # (a 'block' decision that injects the nudge as the next user message), + # NEVER a continue_=False kill. The max_turns cap is the real backstop. + from stuck_guard import STEER_THRESHOLD, StuckGuard guard = StuckGuard() cmd = {"command": "mise //cdk:test"} - for _ in range(BAIL_THRESHOLD): + for _ in range(STEER_THRESHOLD + 5): guard.record_tool_result("Bash", cmd, self._oom()) result = _run(hooks.stop_hook({}, None, {}, task_id="t", stuck_guard=guard)) - # continue_=False ends the turn loop; stopReason carries the honest reason - assert result.get("continue_") is False - assert "Stuck" in (result.get("stopReason") or "") + # a steer is a 'block' decision carrying the advisory text; never a kill + assert result.get("continue_") is not False + assert result.get("decision") == "block" + assert "STOP retrying" in (result.get("reason") or "") def test_stop_hook_steers_when_guard_says_so(self): from stuck_guard import STEER_THRESHOLD, StuckGuard diff --git a/agent/tests/test_stuck_guard.py b/agent/tests/test_stuck_guard.py index 5ef89233..3b16a115 100644 --- a/agent/tests/test_stuck_guard.py +++ b/agent/tests/test_stuck_guard.py @@ -3,7 +3,6 @@ from __future__ import annotations from stuck_guard import ( - BAIL_THRESHOLD, STEER_THRESHOLD, StuckGuard, _looks_failed, @@ -75,17 +74,23 @@ def test_steers_at_most_once_per_signature(self): for _ in range(STEER_THRESHOLD): g.record_tool_result("Bash", CMD, OOM) assert g.evaluate().kind == "steer" - # same signature fails again but below bail → no second steer - g.record_tool_result("Bash", CMD, OOM) + # same signature keeps failing identically → still only ONE steer, never + # escalates to a kill (advisory-only by design — no bail). + for _ in range(10): + g.record_tool_result("Bash", CMD, OOM) assert g.evaluate().kind == "none" - def test_bails_after_persistent_failure(self): + def test_never_bails_advisory_only(self): + # Even on a persistent identical-failure spin, the guard NEVER returns + # 'bail' — it only ever steers (once). The max_turns cap is the real + # runaway backstop; a false positive here must cost at most one nudge. g = StuckGuard() - for _ in range(BAIL_THRESHOLD): + for _ in range(20): g.record_tool_result("Bash", CMD, OOM) - action = g.evaluate() - assert action.kind == "bail" - assert "Stuck" in action.message + # The only non-'none' action this guard can ever produce is 'steer'. + assert g.evaluate().kind in ("none", "steer") + # And specifically it is not 'bail'. + assert g.evaluate().kind != "bail" def test_success_resets_the_streak(self): g = StuckGuard() @@ -112,6 +117,29 @@ def test_healthy_varied_work_never_trips(self): g.record_tool_result("Bash", {"command": f"step-{i}"}, OK) assert g.evaluate().kind == "none" + def test_iterating_agent_same_command_DIFFERENT_failures_never_steers(self): + # K10 false-positive guard: the agent re-runs the SAME test command as + # it fixes failures one by one — each run fails on a DIFFERENT test. + # That's progress, not a loop. The streak resets on each new output, so + # it never even reaches the (advisory) steer threshold. + g = StuckGuard() + cmd = {"command": "mise //cdk:test"} + for i in range(STEER_THRESHOLD + 6): + # A different failing test each run → different output → distinct streak. + resp = f"FAIL test/file_{i}.test.ts:{i * 7} — expected {i} got {i + 1}\nexit code 1" + g.record_tool_result("Bash", cmd, resp) + action = g.evaluate() + assert action.kind == "none", f"iterating agent should get no action, got {action.kind}" + + def test_same_command_IDENTICAL_failure_steers(self): + # The genuine spin: same command, byte-identical failure output every + # time → reaches the steer threshold and emits the one advisory nudge. + g = StuckGuard() + cmd = {"command": "mise //cdk:test"} + for _ in range(STEER_THRESHOLD): + g.record_tool_result("Bash", cmd, OOM) # identical output each run + assert g.evaluate().kind == "steer" + def test_interleaved_success_on_OTHER_sig_does_not_clear_the_loop(self): # The real loop (cmd A) keeps failing; occasional unrelated success (cmd B) # must NOT mask it. From 403cbfdfdac8d67ca73c4d35450d6a95c8fd1bf9 Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Thu, 9 Jul 2026 09:50:00 -0400 Subject: [PATCH 3/5] fix(agent): explain WHY a task hit max_turns + catch loop-of-variations early (ABCA-662) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ABCA-662 capped at max_turns, but the CAUSE was masked: it had finished the code and then thrashed ~15 turns retrying a failing 'git push' (remote: invalid credentials) every which way — a DIFFERENT command each turn, so the existing per-signature stuck-guard (same command × identical output ≥3) never tripped, and the terminal reason read only 'Exceeded max turns'. max_turns is a CORRECT classification, but the user can't tell 'genuinely needed more turns' from 'spun on an error' — and raising the cap just burns more tokens. Two changes, both advisory (no hard kill — keeps the K10 no-false-kill stance): - stuck_guard: add a signature-agnostic TRAILING WINDOW (last 6 tool outcomes). When ≥5 share the SAME byte-identical failure fingerprint across DIFFERENT commands, steer once ('you're spinning on one failing op — stop, it won't resolve by retrying'). EXACT fingerprint (no digit-blur) so a healthy iterate-and-fix loop (same cmd, a different test failing each run) never trips it — the K10 case, still covered by its test. Also exposes recent_failure_ summary(). - pipeline/hooks: the between-turns hook latches that summary; the terminal path appends it to a max_turns reason → 'error_max_turns … — spinning on failing tool calls (last: git push → invalid credentials)'. Only enriches max_turns; a productive run adds nothing. - error-classifier: a new bucket for 'error_max_turns … spinning on failing tool calls' → 'Ran out of turns while stuck on a failing step' with the honest remedy (raising --max-turns won't help; fix the failing op / check the env), ordered before the generic max_turns bucket. Tests: window-spin + K10 no-trip + summary + pipeline enrichment + classifier bucket. --- agent/src/hooks.py | 27 ++++ agent/src/pipeline.py | 20 ++- agent/src/stuck_guard.py | 117 ++++++++++++++++++ agent/tests/test_stuck_guard.py | 68 ++++++++++ cdk/src/handlers/shared/error-classifier.ts | 19 +++ .../handlers/shared/error-classifier.test.ts | 17 +++ 6 files changed, 267 insertions(+), 1 deletion(-) diff --git a/agent/src/hooks.py b/agent/src/hooks.py index 619b6351..19f3bafa 100644 --- a/agent/src/hooks.py +++ b/agent/src/hooks.py @@ -173,6 +173,27 @@ def reset_blocker_reason() -> None: _reset_blocker_reason_for_tests = reset_blocker_reason +# ABCA-662: latch of the stuck-guard's "why it's spinning" summary, refreshed by +# the between-turns hook. Read by the pipeline's terminal path so a max_turns +# failure can say WHY it capped ("Exceeded max turns — spinning on failing tool +# calls: git push → invalid credentials") vs. a task that genuinely used its +# turns. Process-lifetime (one task), last-writer-wins (the most recent window +# is the most relevant to the cap). Distinct from the blocker latch: this is a +# SOFT diagnostic (advisory), not a canonical BLOCKED[…] terminal reason. +_LAST_STUCK_SUMMARY: str | None = None + + +def last_stuck_summary() -> str | None: + """Return the latched stuck-guard summary for the max_turns terminal reason.""" + return _LAST_STUCK_SUMMARY + + +def reset_stuck_summary() -> None: + """Clear the stuck-summary latch (per-task, alongside the blocker latch).""" + global _LAST_STUCK_SUMMARY + _LAST_STUCK_SUMMARY = None + + def detect_egress_denial(text: str) -> tuple[bool, str | None]: """Scan tool output for an egress-denial signature (#251). @@ -1436,6 +1457,12 @@ def _stuck_guard_between_turns_hook(ctx: dict) -> list[str]: return [] try: action = guard.evaluate() + # ABCA-662: refresh the "why it's spinning" latch every turn. When the + # trailing window is failure-dominated this returns a one-liner; otherwise + # None (which clears the latch — a task that recovered isn't "stuck"). Read + # by the terminal path so a later max_turns cap explains itself. + global _LAST_STUCK_SUMMARY + _LAST_STUCK_SUMMARY = guard.recent_failure_summary() except Exception as exc: log("WARN", f"stuck-guard evaluate raised (ignored): {type(exc).__name__}: {exc}") return [] diff --git a/agent/src/pipeline.py b/agent/src/pipeline.py index a6bb7857..6509c88a 100644 --- a/agent/src/pipeline.py +++ b/agent/src/pipeline.py @@ -466,6 +466,21 @@ def _resolve_overall_task_status( agent_status = agent_result.status err = agent_result.error + # ABCA-662: a max_turns cap is a CORRECT classification, but on its own it + # doesn't say WHETHER the task genuinely needed the turns or SPUN on a failing + # operation until it ran out (662 thrashed on a failing `git push` → invalid + # credentials, retried every which way, and capped). When the stuck-guard's + # trailing window was failure-dominated, append its one-line summary so the + # reason distinguishes "ran long" from "looped on an error" — the classifier + # still buckets it as max_turns, but a human sees the real cause. Only enriches + # the max_turns reason; a task that used its turns productively adds nothing. + if err and "error_max_turns" in err: + from hooks import last_stuck_summary + + stuck = last_stuck_summary() + if stuck and stuck not in err: + err = f"{err} — {stuck}" + if agent_status in ("success", "end_turn") and build_ok: return "success", err @@ -696,9 +711,12 @@ def run_task( # in principle dispatch a second run_task in the same process — reset # here so a stale BLOCKED[...] reason can never leak into this task's # terminal error_message (the latch is a scalar, not task_id-keyed). - from hooks import reset_blocker_reason + from hooks import reset_blocker_reason, reset_stuck_summary reset_blocker_reason() + # ABCA-662: same per-task reset for the stuck-guard "why it spun" latch, + # so a prior task's spin summary can't leak into this task's max_turns copy. + reset_stuck_summary() # --trace accumulator (design §10.1): when the task opted into # trace, ``_TrajectoryWriter`` keeps an in-memory copy of each # event so the pipeline can gzip+upload the full trajectory to diff --git a/agent/src/stuck_guard.py b/agent/src/stuck_guard.py index 51ef036c..1bbd63db 100644 --- a/agent/src/stuck_guard.py +++ b/agent/src/stuck_guard.py @@ -55,6 +55,19 @@ # retry-after-fix ("ran it, changed something, ran it again — different result"). STEER_THRESHOLD = 3 +# ABCA-662: a SECOND spin shape the per-signature streak can't see — the agent +# tries a DIFFERENT command each turn toward the SAME failing goal (e.g. a git +# push that keeps failing on 'invalid credentials', retried via http.extraheader, +# then a token remote URL, then GITHUB_TOKEN env, then gh auth status …). Each is +# a distinct signature, so no single streak reaches STEER_THRESHOLD, and the run +# thrashes to the max_turns cap. We also track a TRAILING WINDOW of the last +# ``WINDOW`` tool outcomes: when at least ``WINDOW_FAIL_THRESHOLD`` of them FAILED +# (regardless of signature), the agent is stuck spinning on failures — steer, and +# expose a summary so a max_turns terminal reason can say WHY it capped ("spinning +# on failing tool calls: …") vs. a task that genuinely needed the turns. +WINDOW = 6 +WINDOW_FAIL_THRESHOLD = 5 + # Max chars of the offending command surfaced in the steer message. Short: # this is a hint, not a log dump (and the command is untrusted repo content). _CMD_PREVIEW_LEN = 80 @@ -178,9 +191,28 @@ class StuckGuard: _sigs: dict[str, _SigState] = field(default_factory=dict) _steered: set[str] = field(default_factory=set) _last_failing_sig: str | None = None + # ABCA-662 trailing window: recent tool outcomes as (failed: bool, preview, + # fingerprint), newest last, capped at WINDOW. Drives the window-based steer + # (loop-of-variations) and the max_turns "why" summary. + _window: list[tuple[bool, str, str]] = field(default_factory=list) + _window_steered: bool = False def record_tool_result(self, tool_name: str, tool_input: object, tool_response: str) -> None: """Called from PostToolUse for every tool call. Updates failure streaks.""" + # Trailing-window bookkeeping (ABCA-662): record every outcome, newest + # last, capped at WINDOW — signature-agnostic, so a loop of *different* + # commands all failing is visible even though no single streak grows. + failed = _looks_failed(tool_response) + self._window.append( + ( + failed, + _command_preview(tool_input), + _failure_fingerprint(tool_response) if failed else "", + ) + ) + if len(self._window) > WINDOW: + self._window.pop(0) + sig = _signature(tool_name, tool_input) state = self._sigs.setdefault(sig, _SigState()) if _looks_failed(tool_response): @@ -235,4 +267,89 @@ def evaluate(self) -> StuckAction: ), ) + # ABCA-662: window-based steer — the last WINDOW tool calls are dominated + # by the SAME recurring failure even though the COMMANDS varied (a + # loop-of-variations toward one failing goal, e.g. retrying git-push auth + # every which way and getting "invalid credentials" each time). Requiring a + # dominant repeated failure — not just N failures — is what keeps a healthy + # iterate-and-fix loop (same command, a DIFFERENT test failing each run) + # from tripping this (K10). Steer ONCE, and NOT if the per-signature path + # already steered this same spin (avoid double-nudging one loop). Advisory. + dominant = self._dominant_window_failure() + if not self._window_steered and dominant is not None and sig not in self._steered: + self._window_steered = True + _, last_fail = dominant + fail_count = sum(1 for failed, _, _ in self._window if failed) + return StuckAction( + kind="steer", + signature="__window__", + message=( + f"⚠️ {fail_count} of your last {len(self._window)} tool calls FAILED with " + f"the same error, across different commands (most recently `{last_fail}`) — " + "you are spinning on one failing operation without progress. STOP. If this is " + "an environment/tooling problem (auth, missing credentials, disk, network), it " + "will NOT resolve by retrying — finish now and state clearly in your summary " + "what failed and why, so a human can fix the environment." + ), + ) + return StuckAction(kind="none") + + def _dominant_window_failure(self) -> tuple[str, str] | None: + """If the trailing window is dominated by ONE recurring failure, return + ``(fingerprint, last_matching_command_preview)``; else None. + + "Dominated" = the window is full AND at least ``WINDOW_FAIL_THRESHOLD`` of + its entries are failures sharing the SAME failure fingerprint (the + collapsed output prefix, NOT digit-blurred — see below). This is the + signal-agnostic spin detector: the SAME error recurring across VARIED + commands (662: 'invalid credentials' on every push variant), NOT a + productive loop where each failure differs. + + Crucially we compare EXACT (whitespace-collapsed) fingerprints and do NOT + blur digits: an earlier attempt normalized ``\\d+ → #`` to catch volatile + suffixes, but that ALSO collapsed a healthy iterate-and-fix loop (same + command, ``FAIL test/file_0``, ``file_1``, … — a DIFFERENT failing test + each run, which is PROGRESS) into one fingerprint and false-steered it + (K10). Requiring byte-identical failure output means only a genuinely + stuck spin (the same error verbatim) trips this; a working agent whose + output changes run-to-run never does.""" + if len(self._window) < WINDOW: + return None + # Count failures by EXACT fingerprint (no digit blur — see docstring). + counts: dict[str, tuple[int, str]] = {} + for failed, prev, fp in self._window: + if not failed: + continue + n, _ = counts.get(fp, (0, prev)) + counts[fp] = (n + 1, prev) # keep the latest preview for this fp + if not counts: + return None + top_fp, (top_n, top_prev) = max(counts.items(), key=lambda kv: kv[1][0]) + if top_n >= WINDOW_FAIL_THRESHOLD: + return (top_fp, top_prev) + return None + + def recent_failure_summary(self) -> str | None: + """A one-line "why it spun" summary for a max_turns terminal reason. + + Returns None unless the trailing window is failure-dominated (the same + bar the window-steer uses) — so a task that genuinely used its turns + making progress yields no summary and its max_turns reason is unchanged. + Names the dominant recent failing command + a short slice of its output, + so the platform can say "hit max turns while stuck on: → " + instead of a bare "Exceeded max turns". + """ + dominant = self._dominant_window_failure() + if dominant is None: + return None + _, prev = dominant + # Recover a short slice of the actual (un-normalized) output for the last + # failure matching the dominant command, for the human-readable detail. + detail = "" + for failed, p, fp in reversed(self._window): + if failed and p == prev: + detail = re.sub(r"\s+", " ", fp).strip()[:120] + break + base = f"spinning on failing tool calls (last: `{prev}`" + return f"{base} — {detail})" if detail else f"{base})" diff --git a/agent/tests/test_stuck_guard.py b/agent/tests/test_stuck_guard.py index 3b16a115..ab8903d7 100644 --- a/agent/tests/test_stuck_guard.py +++ b/agent/tests/test_stuck_guard.py @@ -152,3 +152,71 @@ def test_interleaved_success_on_OTHER_sig_does_not_clear_the_loop(self): action = g.evaluate() assert action.kind == "steer" assert "loop" in action.message.lower() + + +# DIFFERENT command each turn (distinct signatures, so no per-signature streak +# grows) but the SAME recurring error — exactly the 662 push-auth thrash: the +# agent retried the push every which way, each getting 'invalid credentials'. +_ERR = "remote: invalid credentials\nfatal: exit 128" +_PUSH_FAILS = [ + ({"command": "git push origin HEAD"}, _ERR), + ({"command": "git config http.extraheader ... && git push"}, _ERR), + ({"command": "git remote set-url origin https://x-access-token@... && git push"}, _ERR), + ({"command": "GITHUB_TOKEN=$GH_TOKEN git push"}, _ERR), + ({"command": "git -c credential.helper= push"}, _ERR), + ({"command": "git push --force-with-lease"}, _ERR), +] + + +class TestWindowSpin: + """ABCA-662: the loop-of-VARIATIONS the per-signature streak can't see — the + agent tries a different command each turn toward the same failing goal (a git + push that keeps failing on 'invalid credentials'). No single signature reaches + STEER_THRESHOLD, but the trailing window is failure-dominated.""" + + def test_window_steers_on_loop_of_distinct_failing_commands(self): + g = StuckGuard() + for cmd, out in _PUSH_FAILS: + g.record_tool_result("Bash", cmd, out) + action = g.evaluate() + assert action.kind == "steer" + assert action.signature == "__window__" + assert "spinning" in action.message.lower() + + def test_window_steer_fires_at_most_once(self): + g = StuckGuard() + for cmd, out in _PUSH_FAILS: + g.record_tool_result("Bash", cmd, out) + assert g.evaluate().kind == "steer" + # A subsequent failing turn must not re-steer the window. + g.record_tool_result("Bash", {"command": "git push again"}, _ERR) + assert g.evaluate().kind == "none" + + def test_recent_failure_summary_names_the_last_failure(self): + g = StuckGuard() + for cmd, out in _PUSH_FAILS: + g.record_tool_result("Bash", cmd, out) + summary = g.recent_failure_summary() + assert summary is not None + assert "spinning on failing tool calls" in summary + assert "git push --force-with-lease" in summary # most recent failing command + assert "invalid credentials" in summary # the recurring error detail + + def test_no_summary_when_window_is_mostly_successful(self): + # A productive agent (varied commands, mostly succeeding) must yield no + # spin summary — so its max_turns reason stays unchanged. + g = StuckGuard() + for i in range(6): + g.record_tool_result("Bash", {"command": f"step {i}"}, OK) + assert g.recent_failure_summary() is None + assert g.evaluate().kind == "none" + + def test_healthy_iteration_below_window_threshold_no_steer(self): + # 4/6 failing is below WINDOW_FAIL_THRESHOLD(5) — a normal fix-iterate loop + # (some fail, some pass) must NOT trip the window steer. + g = StuckGuard() + outcomes = [OOM, OK, OOM, OK, OOM, OOM] # 4 fails / 6 + for i, out in enumerate(outcomes): + g.record_tool_result("Bash", {"command": f"cmd {i}"}, out) + assert g.evaluate().kind == "none" + assert g.recent_failure_summary() is None diff --git a/cdk/src/handlers/shared/error-classifier.ts b/cdk/src/handlers/shared/error-classifier.ts index bf015739..0b1efad7 100644 --- a/cdk/src/handlers/shared/error-classifier.ts +++ b/cdk/src/handlers/shared/error-classifier.ts @@ -305,6 +305,25 @@ const PATTERNS: readonly ErrorPattern[] = [ // real max-turns failure fell through to UNKNOWN → "Unexpected error" // (live-caught on ABCA-483: a task hit the 100-turn cap but the reply // said "Unexpected error"). Match either ``agent_status=``/``subtype=``. + { + // ABCA-662: max_turns hit while SPINNING on a failing operation (the agent's + // stuck-guard latched a failure-dominated trailing window and the pipeline + // appended "spinning on failing tool calls (last: …)"). This is a CORRECT + // max_turns classification, but the cause is NOT "task too big" — it's a + // repeated failure the agent couldn't get past (e.g. a git-push auth failure, + // 662). Raising --max-turns would just thrash longer, so give the honest + // remedy: it's the underlying failure to fix, not the turn budget. Ordered + // before the generic error_max_turns bucket so this specific case wins. + pattern: /error_max_turns.*spinning on failing tool calls/i, + classification: { + category: ErrorCategory.TIMEOUT, + title: 'Ran out of turns while stuck on a failing step', + description: 'The agent hit its max-turns limit, but it was repeatedly retrying a failing operation (see the detail below) rather than making progress — so the cause is that failing step, not the size of the task.', + remedy: 'Raising --max-turns won\'t help — it would just retry the same failure longer. Look at the failing operation in the detail below: if it\'s an environment/tooling problem (auth, credentials, network, disk) an admin should fix it, then reply here to retry. If it\'s the request, refine it and retry.', + retryable: true, + errorClass: ErrorClass.USER, + }, + }, { pattern: /(?:agent_status|subtype)=['"]?error_max_turns['"]?/i, classification: { diff --git a/cdk/test/handlers/shared/error-classifier.test.ts b/cdk/test/handlers/shared/error-classifier.test.ts index edbbd0d6..51cad0d5 100644 --- a/cdk/test/handlers/shared/error-classifier.test.ts +++ b/cdk/test/handlers/shared/error-classifier.test.ts @@ -258,6 +258,23 @@ describe('classifyError', () => { expect(result!.remedy).toMatch(/--max-turns/); }); + test('ABCA-662: max_turns hit while SPINNING gives a distinct title + honest remedy (not "raise --max-turns")', () => { + // When the agent capped out because it was thrashing on a failing op (the + // stuck-guard appended "spinning on failing tool calls"), the cause is the + // failure, not the turn budget — so the remedy must NOT be "raise --max-turns". + // Ordered before the generic error_max_turns bucket so this specific case wins. + const result = classifyError( + "Agent session error (subtype='error_max_turns') — spinning on failing tool calls " + + '(last: `git push --force-with-lease` — remote: invalid credentials fatal: exit 128)', + ); + expect(result!.category).toBe(ErrorCategory.TIMEOUT); + expect(result!.title).toBe('Ran out of turns while stuck on a failing step'); + expect(result!.retryable).toBe(true); + // The honest remedy: raising turns won't help; look at the failing op. + expect(result!.remedy).toMatch(/won't help|failing operation|environment/i); + expect(result!.remedy).not.toMatch(/raise --max-turns on the submit/i); + }); + test('classifies error_max_budget_usd as TIMEOUT with specific title', () => { const result = classifyError( "Task did not succeed (agent_status='error_max_budget_usd', build_ok=False)", From 0056d8bc7f7ba93f60b2ca449fdb105e1fc5f595 Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Thu, 9 Jul 2026 10:08:30 -0400 Subject: [PATCH 4/5] =?UTF-8?q?fix(errors):=20don't=20over-claim=20on=20a?= =?UTF-8?q?=20max=5Fturns=20spin=20=E2=80=94=20offer=20BOTH=20remedies=20(?= =?UTF-8?q?ABCA-662=20review)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior 'spinning on failing tool calls' classifier bucket asserted 'raising --max-turns won't help'. That over-claims: the trailing window can't distinguish (a) a HARD blocker no turn count fixes from (b) a LONG task that hit a transient/recoverable snag near the end and just ran out — which is 662 itself (siblings 661/663 pushed fine with the same token → its 'invalid credentials' was a transient race that more turns / a retry WOULD have cleared). Reframe the bucket to SURFACE what it was stuck on and present BOTH paths (environment blocker → fix + retry; transient/recoverable or a shorter sibling got past it → just retry / raise --max-turns), letting the failure KIND in the detail tell the reader which applies. Title 'Ran out of turns retrying a failing step'. The early window-steer (agent told to STOP and report while it still has turns) is the real mechanism that lets a long task escape the thrash; this copy is just the honest post-hoc explanation. Test updated to assert both remedies, not the over-claim. --- cdk/src/handlers/shared/error-classifier.ts | 24 +++++++++++-------- .../handlers/shared/error-classifier.test.ts | 21 +++++++++------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/cdk/src/handlers/shared/error-classifier.ts b/cdk/src/handlers/shared/error-classifier.ts index 0b1efad7..f6c978a0 100644 --- a/cdk/src/handlers/shared/error-classifier.ts +++ b/cdk/src/handlers/shared/error-classifier.ts @@ -306,20 +306,24 @@ const PATTERNS: readonly ErrorPattern[] = [ // (live-caught on ABCA-483: a task hit the 100-turn cap but the reply // said "Unexpected error"). Match either ``agent_status=``/``subtype=``. { - // ABCA-662: max_turns hit while SPINNING on a failing operation (the agent's - // stuck-guard latched a failure-dominated trailing window and the pipeline - // appended "spinning on failing tool calls (last: …)"). This is a CORRECT - // max_turns classification, but the cause is NOT "task too big" — it's a - // repeated failure the agent couldn't get past (e.g. a git-push auth failure, - // 662). Raising --max-turns would just thrash longer, so give the honest - // remedy: it's the underlying failure to fix, not the turn budget. Ordered + // ABCA-662: max_turns hit while the last several tool calls were the SAME + // repeated failure (the agent's stuck-guard latched a failure-dominated + // trailing window; the pipeline appended "spinning on failing tool calls + // (last: …)"). This SURFACES what it was stuck on — but deliberately does NOT + // assert "more turns won't help", because the window alone can't distinguish + // (a) a HARD blocker no turn count fixes (bad creds, no permission) from + // (b) a LONG task that hit a transient/recoverable snag late and just ran out + // (662 itself: siblings 661/663 pushed fine with the same token → its + // 'invalid credentials' was a transient race that MORE turns / a retry would + // have cleared). So we name the failing operation and offer BOTH remedies; + // the failure KIND (in the detail) tells the reader which applies. Ordered // before the generic error_max_turns bucket so this specific case wins. pattern: /error_max_turns.*spinning on failing tool calls/i, classification: { category: ErrorCategory.TIMEOUT, - title: 'Ran out of turns while stuck on a failing step', - description: 'The agent hit its max-turns limit, but it was repeatedly retrying a failing operation (see the detail below) rather than making progress — so the cause is that failing step, not the size of the task.', - remedy: 'Raising --max-turns won\'t help — it would just retry the same failure longer. Look at the failing operation in the detail below: if it\'s an environment/tooling problem (auth, credentials, network, disk) an admin should fix it, then reply here to retry. If it\'s the request, refine it and retry.', + title: 'Ran out of turns retrying a failing step', + description: 'The agent hit its max-turns limit while the last several tool calls were the same repeated failure (see the detail below). That means one of two things — the failing step is a hard blocker no extra turns can fix, OR the task was long and hit a recoverable snag near the end and simply ran out of turns.', + remedy: 'Check the failing operation in the detail below. If it is an environment/tooling blocker (auth, credentials, permission, network, disk), retrying more won\'t help — fix the environment (an admin), then reply here to retry. If it looks transient or recoverable (a flaky/temporary error, or a sibling task with a shorter to-do got past the same thing), just reply to retry and/or raise --max-turns.', retryable: true, errorClass: ErrorClass.USER, }, diff --git a/cdk/test/handlers/shared/error-classifier.test.ts b/cdk/test/handlers/shared/error-classifier.test.ts index 51cad0d5..39248666 100644 --- a/cdk/test/handlers/shared/error-classifier.test.ts +++ b/cdk/test/handlers/shared/error-classifier.test.ts @@ -258,21 +258,24 @@ describe('classifyError', () => { expect(result!.remedy).toMatch(/--max-turns/); }); - test('ABCA-662: max_turns hit while SPINNING gives a distinct title + honest remedy (not "raise --max-turns")', () => { - // When the agent capped out because it was thrashing on a failing op (the - // stuck-guard appended "spinning on failing tool calls"), the cause is the - // failure, not the turn budget — so the remedy must NOT be "raise --max-turns". - // Ordered before the generic error_max_turns bucket so this specific case wins. + test('ABCA-662: max_turns hit while retrying a failing step surfaces WHAT it was stuck on, and offers BOTH remedies (does not over-claim)', () => { + // When the agent capped out with the last several calls being the same + // repeated failure (pipeline appended "spinning on failing tool calls"), we + // name the failing op — but must NOT assert "more turns won't help", because + // the window can't tell a hard blocker from a long task that hit a + // recoverable snag late (662: siblings pushed fine → transient). So the + // remedy presents BOTH paths. Ordered before the generic max_turns bucket. const result = classifyError( "Agent session error (subtype='error_max_turns') — spinning on failing tool calls " + '(last: `git push --force-with-lease` — remote: invalid credentials fatal: exit 128)', ); expect(result!.category).toBe(ErrorCategory.TIMEOUT); - expect(result!.title).toBe('Ran out of turns while stuck on a failing step'); + expect(result!.title).toBe('Ran out of turns retrying a failing step'); expect(result!.retryable).toBe(true); - // The honest remedy: raising turns won't help; look at the failing op. - expect(result!.remedy).toMatch(/won't help|failing operation|environment/i); - expect(result!.remedy).not.toMatch(/raise --max-turns on the submit/i); + // Surfaces the environment-blocker path AND the transient/recoverable path — + // it does NOT flatly claim raising turns is useless. + expect(result!.remedy).toMatch(/environment/i); + expect(result!.remedy).toMatch(/transient|recoverable|raise --max-turns/i); }); test('classifies error_max_budget_usd as TIMEOUT with specific title', () => { From 49ce2c4205233a4ae5fc091963be4f2970d7f41c Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Mon, 13 Jul 2026 21:59:18 -0400 Subject: [PATCH 5/5] =?UTF-8?q?fix(errors):=20max=5Fturns=20stays=20"Excee?= =?UTF-8?q?ded=20max=20turns"=20=E2=80=94=20observe=20the=20repeated=20fai?= =?UTF-8?q?lure,=20don't=20diagnose=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer concern on the ABCA-662 work: re-titling a max_turns failure as "Ran out of turns retrying a failing step" over-claims. The stuck-guard's trailing window is only the last ~6 tool calls, which genuinely CANNOT distinguish a hard blocker (bad creds, no permission — more turns won't help) from a long task that made real progress and hit a recoverable snag only at the tail (more turns / a retry WOULD help — 662 itself: siblings pushed fine with the same token). Framing the whole run around its last 6 calls misrepresents the latter. - error-classifier: drop the separate "retrying a failing step" bucket. A max_turns failure stays the plain "Exceeded max turns"; the description/remedy point the reader at the observed detail and still surface the environment-blocker path, but assert NO cause. - stuck_guard.recent_failure_summary: emit a NEUTRAL observation ("last tool calls repeated: `` → ") instead of "spinning on failing tool calls". The mid-run steer TO the agent (an advisory nudge, not a user surface) still says "spinning" — that's fine, it's coaching the agent, not classifying the outcome. - tests updated to pin the neutral wording + assert the title is NOT re-framed. --- agent/src/pipeline.py | 4 +-- agent/src/stuck_guard.py | 18 ++++++---- agent/tests/test_stuck_guard.py | 6 ++-- cdk/src/handlers/shared/error-classifier.ts | 36 ++++++------------- .../handlers/shared/error-classifier.test.ts | 28 ++++++++------- 5 files changed, 44 insertions(+), 48 deletions(-) diff --git a/agent/src/pipeline.py b/agent/src/pipeline.py index 6509c88a..082c21f6 100644 --- a/agent/src/pipeline.py +++ b/agent/src/pipeline.py @@ -714,8 +714,8 @@ def run_task( from hooks import reset_blocker_reason, reset_stuck_summary reset_blocker_reason() - # ABCA-662: same per-task reset for the stuck-guard "why it spun" latch, - # so a prior task's spin summary can't leak into this task's max_turns copy. + # ABCA-662: same per-task reset for the stuck-guard recent-failure latch, + # so a prior task's observation can't leak into this task's max_turns copy. reset_stuck_summary() # --trace accumulator (design §10.1): when the task opted into # trace, ``_TrajectoryWriter`` keeps an in-memory copy of each diff --git a/agent/src/stuck_guard.py b/agent/src/stuck_guard.py index 1bbd63db..41108ec7 100644 --- a/agent/src/stuck_guard.py +++ b/agent/src/stuck_guard.py @@ -331,14 +331,20 @@ def _dominant_window_failure(self) -> tuple[str, str] | None: return None def recent_failure_summary(self) -> str | None: - """A one-line "why it spun" summary for a max_turns terminal reason. + """A one-line NEUTRAL observation of the recent repeated failure, for a + max_turns terminal reason. Returns None unless the trailing window is failure-dominated (the same bar the window-steer uses) — so a task that genuinely used its turns making progress yields no summary and its max_turns reason is unchanged. - Names the dominant recent failing command + a short slice of its output, - so the platform can say "hit max turns while stuck on: → " - instead of a bare "Exceeded max turns". + Names the dominant recent failing command + a short slice of its output. + + Deliberately states only WHAT was observed, not WHY it capped: the window + is the last few tool calls, which can't tell a hard blocker from a long + task that hit a recoverable snag near the end. So the platform can say + "hit max turns; last tool calls repeated: → " and let the + reader judge — it must NOT assert the task was "spinning" or that more + turns wouldn't have helped. """ dominant = self._dominant_window_failure() if dominant is None: @@ -351,5 +357,5 @@ def recent_failure_summary(self) -> str | None: if failed and p == prev: detail = re.sub(r"\s+", " ", fp).strip()[:120] break - base = f"spinning on failing tool calls (last: `{prev}`" - return f"{base} — {detail})" if detail else f"{base})" + base = f"last tool calls repeated: `{prev}`" + return f"{base} — {detail}" if detail else base diff --git a/agent/tests/test_stuck_guard.py b/agent/tests/test_stuck_guard.py index ab8903d7..f13d2d15 100644 --- a/agent/tests/test_stuck_guard.py +++ b/agent/tests/test_stuck_guard.py @@ -198,13 +198,15 @@ def test_recent_failure_summary_names_the_last_failure(self): g.record_tool_result("Bash", cmd, out) summary = g.recent_failure_summary() assert summary is not None - assert "spinning on failing tool calls" in summary + # Neutral observation only — names WHAT repeated, makes no causal claim. + assert "last tool calls repeated" in summary + assert "spinning" not in summary # must not editorialize assert "git push --force-with-lease" in summary # most recent failing command assert "invalid credentials" in summary # the recurring error detail def test_no_summary_when_window_is_mostly_successful(self): # A productive agent (varied commands, mostly succeeding) must yield no - # spin summary — so its max_turns reason stays unchanged. + # summary — so its max_turns reason stays unchanged. g = StuckGuard() for i in range(6): g.record_tool_result("Bash", {"command": f"step {i}"}, OK) diff --git a/cdk/src/handlers/shared/error-classifier.ts b/cdk/src/handlers/shared/error-classifier.ts index f6c978a0..41d538e8 100644 --- a/cdk/src/handlers/shared/error-classifier.ts +++ b/cdk/src/handlers/shared/error-classifier.ts @@ -306,35 +306,21 @@ const PATTERNS: readonly ErrorPattern[] = [ // (live-caught on ABCA-483: a task hit the 100-turn cap but the reply // said "Unexpected error"). Match either ``agent_status=``/``subtype=``. { - // ABCA-662: max_turns hit while the last several tool calls were the SAME - // repeated failure (the agent's stuck-guard latched a failure-dominated - // trailing window; the pipeline appended "spinning on failing tool calls - // (last: …)"). This SURFACES what it was stuck on — but deliberately does NOT - // assert "more turns won't help", because the window alone can't distinguish - // (a) a HARD blocker no turn count fixes (bad creds, no permission) from - // (b) a LONG task that hit a transient/recoverable snag late and just ran out - // (662 itself: siblings 661/663 pushed fine with the same token → its - // 'invalid credentials' was a transient race that MORE turns / a retry would - // have cleared). So we name the failing operation and offer BOTH remedies; - // the failure KIND (in the detail) tells the reader which applies. Ordered - // before the generic error_max_turns bucket so this specific case wins. - pattern: /error_max_turns.*spinning on failing tool calls/i, - classification: { - category: ErrorCategory.TIMEOUT, - title: 'Ran out of turns retrying a failing step', - description: 'The agent hit its max-turns limit while the last several tool calls were the same repeated failure (see the detail below). That means one of two things — the failing step is a hard blocker no extra turns can fix, OR the task was long and hit a recoverable snag near the end and simply ran out of turns.', - remedy: 'Check the failing operation in the detail below. If it is an environment/tooling blocker (auth, credentials, permission, network, disk), retrying more won\'t help — fix the environment (an admin), then reply here to retry. If it looks transient or recoverable (a flaky/temporary error, or a sibling task with a shorter to-do got past the same thing), just reply to retry and/or raise --max-turns.', - retryable: true, - errorClass: ErrorClass.USER, - }, - }, - { + // A max-turns cap is a correct, self-explanatory classification. When the + // stuck-guard observed the last several tool calls repeating the SAME failure + // it is appended to the reason as a neutral OBSERVATION ("last tool calls + // repeated: `` → ") — we surface WHAT was on screen but deliberately + // make NO causal claim about whether more turns would have helped: the + // trailing window (last 6 calls) can't distinguish a hard blocker from a long + // task that hit a recoverable snag only at the tail, so re-framing the whole + // run as "retrying a failing step" would misrepresent the latter. The reader + // sees the observed detail and the neutral remedy and decides. pattern: /(?:agent_status|subtype)=['"]?error_max_turns['"]?/i, classification: { category: ErrorCategory.TIMEOUT, title: 'Exceeded max turns', - description: 'The agent reached the configured ``max_turns`` limit before completing.', - remedy: 'Raise ``--max-turns`` on the submit call, simplify the task, or break it into smaller sub-tasks.', + description: 'The agent reached the configured ``max_turns`` limit before completing. If a repeated tool failure was observed near the end, it is shown in the detail below.', + remedy: 'Look at the detail below to see what the agent was doing when it ran out. Raise ``--max-turns`` on the submit call, simplify the task, or break it into smaller sub-tasks — and if the detail shows an environment/tooling blocker (auth, credentials, permission, network, disk), fix that first, then reply here to retry.', retryable: true, errorClass: ErrorClass.USER, }, diff --git a/cdk/test/handlers/shared/error-classifier.test.ts b/cdk/test/handlers/shared/error-classifier.test.ts index 39248666..3978c22d 100644 --- a/cdk/test/handlers/shared/error-classifier.test.ts +++ b/cdk/test/handlers/shared/error-classifier.test.ts @@ -258,24 +258,26 @@ describe('classifyError', () => { expect(result!.remedy).toMatch(/--max-turns/); }); - test('ABCA-662: max_turns hit while retrying a failing step surfaces WHAT it was stuck on, and offers BOTH remedies (does not over-claim)', () => { + test('ABCA-662: max_turns with an observed repeated failure stays "Exceeded max turns" and makes NO causal claim', () => { // When the agent capped out with the last several calls being the same - // repeated failure (pipeline appended "spinning on failing tool calls"), we - // name the failing op — but must NOT assert "more turns won't help", because - // the window can't tell a hard blocker from a long task that hit a - // recoverable snag late (662: siblings pushed fine → transient). So the - // remedy presents BOTH paths. Ordered before the generic max_turns bucket. + // repeated failure, the pipeline appends a NEUTRAL observation ("last tool + // calls repeated: …"). The classification must NOT re-title the failure as + // "retrying a failing step" or assert more turns wouldn't help — the window + // (last few calls) can't tell a hard blocker from a long task that hit a + // recoverable snag late (662: siblings pushed fine → transient). It stays the + // plain max_turns bucket; the observed detail rides along in the message. const result = classifyError( - "Agent session error (subtype='error_max_turns') — spinning on failing tool calls " - + '(last: `git push --force-with-lease` — remote: invalid credentials fatal: exit 128)', + "Agent session error (subtype='error_max_turns') — last tool calls repeated: " + + '`git push --force-with-lease` — remote: invalid credentials fatal: exit 128', ); expect(result!.category).toBe(ErrorCategory.TIMEOUT); - expect(result!.title).toBe('Ran out of turns retrying a failing step'); + expect(result!.title).toBe('Exceeded max turns'); expect(result!.retryable).toBe(true); - // Surfaces the environment-blocker path AND the transient/recoverable path — - // it does NOT flatly claim raising turns is useless. - expect(result!.remedy).toMatch(/environment/i); - expect(result!.remedy).toMatch(/transient|recoverable|raise --max-turns/i); + // Does not editorialize: no "spinning" / "won't help" claim. It points the + // reader at the detail and still offers the environment-blocker path. + expect(result!.title).not.toMatch(/retrying a failing step/i); + expect(result!.remedy).toMatch(/detail/i); + expect(result!.remedy).toMatch(/environment|auth|credentials/i); }); test('classifies error_max_budget_usd as TIMEOUT with specific title', () => {