diff --git a/agent/src/hooks.py b/agent/src/hooks.py index 6ecf7299..19f3bafa 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 @@ -172,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). @@ -1039,6 +1061,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 +1074,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 +1141,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 +1431,49 @@ def _cancel_between_turns_hook(ctx: dict) -> list[str]: return [] +def _stuck_guard_between_turns_hook(ctx: dict) -> list[str]: + """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 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 + 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() + # 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 [] + + 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 +1485,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 (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 # nudge short-circuits pre-empt it. The hook explicitly re-checks @@ -1423,6 +1506,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 +1529,7 @@ async def stop_hook( "task_id": task_id, "progress": progress, "engine": engine, + "stuck_guard": stuck_guard, } # Cancel-before-nudge short-circuit. @@ -1526,6 +1611,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 +1658,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 +1688,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/pipeline.py b/agent/src/pipeline.py index 980e2df0..2317efb4 100644 --- a/agent/src/pipeline.py +++ b/agent/src/pipeline.py @@ -470,6 +470,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 @@ -700,9 +715,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 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 # 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 new file mode 100644 index 00000000..41108ec7 --- /dev/null +++ b/agent/src/stuck_guard.py @@ -0,0 +1,361 @@ +"""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 + 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. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass, field + +# 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 + +# 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 + +# 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 + + +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' (advisory only — never kills the task) + 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 + # 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): + # 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: + # 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 + 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 / none (advisory only). + + 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: + return StuckAction(kind="none") + state = self._sigs.get(sig) + if state is None: + return StuckAction(kind="none") + + 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." + ), + ) + + # 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 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. + + 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: + 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"last tool calls repeated: `{prev}`" + return f"{base} — {detail}" if detail else base diff --git a/agent/tests/test_hooks.py b/agent/tests/test_hooks.py index bffcb36f..075fd5f2 100644 --- a/agent/tests/test_hooks.py +++ b/agent/tests/test_hooks.py @@ -9,11 +9,14 @@ from hooks import ( _reset_blocker_reason_for_tests, + _stuck_guard_between_turns_hook, build_hook_matchers, detect_egress_denial, last_blocker_reason, + last_stuck_summary, post_tool_use_hook, pre_tool_use_hook, + reset_stuck_summary, ) from policy import PolicyEngine @@ -1697,3 +1700,128 @@ 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 (advisory).""" + + 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_between_turns_hook_latches_stuck_summary_from_the_guard(self): + # #599 N2: pin the PRODUCTION write path for the _LAST_STUCK_SUMMARY latch + # (hooks.py:1464-1465). The enrichment tests monkeypatch the getter, so + # without this test deleting those two lines would leave the latch + # permanently None and every test would still pass. Drive the real hook + # with a failure-dominated guard and assert the module latch is populated. + from stuck_guard import WINDOW, StuckGuard + + reset_stuck_summary() + assert last_stuck_summary() is None + guard = StuckGuard() + cmd = {"command": "mise //cdk:test"} + # recent_failure_summary needs a FULL window (>= WINDOW) of byte-identical + # failures (WINDOW_FAIL_THRESHOLD of them) — fill it past WINDOW. + for _ in range(WINDOW + 2): + guard.record_tool_result("Bash", cmd, self._oom()) + # Precondition: the guard itself considers the window failure-dominated. + assert guard.recent_failure_summary() is not None + + result = _stuck_guard_between_turns_hook({"stuck_guard": guard}) + # advisory steer text returned… + assert isinstance(result, list) + # …and, crucially, the terminal-reason latch was written by the hook. + summary = last_stuck_summary() + assert summary is not None + assert "last tool calls repeated" in summary + reset_stuck_summary() + + def test_between_turns_hook_clears_stuck_summary_when_not_failure_dominated(self): + # Symmetric guard: a recovered task (clean window) must CLEAR the latch, + # not leave a stale "stuck" summary that a later max_turns cap would echo. + # Pre-seed a stale latch via a failure-dominated guard. + from stuck_guard import WINDOW, StuckGuard + + stuck = StuckGuard() + for _ in range(WINDOW + 2): + stuck.record_tool_result("Bash", {"command": "mise //cdk:test"}, self._oom()) + _stuck_guard_between_turns_hook({"stuck_guard": stuck}) + assert last_stuck_summary() is not None + + # A fresh, healthy guard on the next turn clears it (recent_failure_summary None). + healthy = StuckGuard() + healthy.record_tool_result("Read", {"file": "a.py"}, "def hello(): return 1") + _stuck_guard_between_turns_hook({"stuck_guard": healthy}) + assert last_stuck_summary() is None + reset_stuck_summary() + + 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_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(STEER_THRESHOLD + 5): + guard.record_tool_result("Bash", cmd, self._oom()) + result = _run(hooks.stop_hook({}, None, {}, task_id="t", stuck_guard=guard)) + # 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 + + 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_pipeline_outcomes.py b/agent/tests/test_pipeline_outcomes.py index 797eaa6b..d2b89a6a 100644 --- a/agent/tests/test_pipeline_outcomes.py +++ b/agent/tests/test_pipeline_outcomes.py @@ -161,3 +161,61 @@ def test_zero_turns_attempted_round_trips(self): # Zero is treated the same as None (falsy) so we don't clamp it to a # negative / nonsensical value. assert _compute_turns_completed("error_max_turns", 0, max_turns=10) == 0 + + +class TestMaxTurnsStuckEnrichment: + """N3 wiring seam (#600): _resolve_overall_task_status enriches a max_turns + reason with the stuck-guard summary (hooks.last_stuck_summary). Previously + tested only at its two pure endpoints; this drives the append itself.""" + + def test_max_turns_appends_stuck_summary(self, monkeypatch): + import hooks + + summary = "last tool calls repeated: git push — invalid credentials" + monkeypatch.setattr(hooks, "last_stuck_summary", lambda: summary) + ar = AgentResult( + status="error_max_turns", + error="Agent session error (subtype=error_max_turns)", + ) + _, err = _resolve_overall_task_status(ar, build_ok=False, pr_url=None) + assert err is not None + assert "error_max_turns" in err + assert summary in err + + def test_non_max_turns_error_is_not_enriched(self, monkeypatch): + # A generic failure must NOT pull in the stuck summary — only max_turns. + import hooks + + monkeypatch.setattr(hooks, "last_stuck_summary", lambda: "last tool calls repeated: X") + ar = AgentResult(status="error", error="receive_response() failed: boom") + _, err = _resolve_overall_task_status(ar, build_ok=False, pr_url=None) + assert err is not None + assert "last tool calls repeated" not in err + + def test_max_turns_with_no_stuck_summary_left_unchanged(self, monkeypatch): + # A task that used its turns productively (window not failure-dominated → + # summary None) leaves the max_turns reason unchanged. + import hooks + + monkeypatch.setattr(hooks, "last_stuck_summary", lambda: None) + ar = AgentResult( + status="error_max_turns", + error="Agent session error (subtype=error_max_turns)", + ) + _, err = _resolve_overall_task_status(ar, build_ok=False, pr_url=None) + assert err == "Agent session error (subtype=error_max_turns)" + + def test_stuck_summary_not_double_appended(self, monkeypatch): + # Idempotent: if the summary is already in the reason (e.g. a durable + # re-resolution of the same result), it must not be appended twice. + import hooks + + summary = "last tool calls repeated: git push — invalid credentials" + monkeypatch.setattr(hooks, "last_stuck_summary", lambda: summary) + ar = AgentResult( + status="error_max_turns", + error=f"Agent session error (subtype=error_max_turns) — {summary}", + ) + _, err = _resolve_overall_task_status(ar, build_ok=False, pr_url=None) + assert err is not None + assert err.count(summary) == 1 diff --git a/agent/tests/test_stuck_guard.py b/agent/tests/test_stuck_guard.py new file mode 100644 index 00000000..942246e2 --- /dev/null +++ b/agent/tests/test_stuck_guard.py @@ -0,0 +1,250 @@ +"""Tests for the stuck/runaway guard (K7, live-caught ABCA-483).""" + +from __future__ import annotations + +from stuck_guard import ( + STEER_THRESHOLD, + WINDOW, + WINDOW_FAIL_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 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_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(20): + g.record_tool_result("Bash", CMD, OOM) + # 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() + 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_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. + 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() + + +# 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 + # 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 + # 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 + + def test_window_steers_at_exactly_the_threshold(self): + # N4 boundary: exactly WINDOW_FAIL_THRESHOLD (5) same-fingerprint failures + # in a FULL window of WINDOW (6) — the `>=` edge where an off-by-one would + # hide. One OK dilutes the window to 5/6 fails, still == the threshold. + assert WINDOW == 6 and WINDOW_FAIL_THRESHOLD == 5 # pin the constants + g = StuckGuard() + outcomes = [OK, _ERR, _ERR, _ERR, _ERR, _ERR] # 5 same-fp fails / full 6 + for i, out in enumerate(outcomes): + g.record_tool_result("Bash", {"command": f"push {i}"}, out) + action = g.evaluate() + assert action.kind == "steer" + assert action.signature == "__window__" + assert g.recent_failure_summary() is not None + + def test_no_steer_when_window_not_yet_full(self): + # N4 boundary: WINDOW_FAIL_THRESHOLD failures but the window has fewer than + # WINDOW entries — _dominant_window_failure requires a FULL window, so 5 + # identical failures in a length-5 history must NOT steer yet. + g = StuckGuard() + for i in range(WINDOW_FAIL_THRESHOLD): # 5 fails, window not yet at 6 + g.record_tool_result("Bash", {"command": f"push {i}"}, _ERR) + assert g.evaluate().kind == "none" + assert g.recent_failure_summary() is None diff --git a/cdk/src/handlers/orchestrate-task.ts b/cdk/src/handlers/orchestrate-task.ts index 284e5e08..d1914282 100644 --- a/cdk/src/handlers/orchestrate-task.ts +++ b/cdk/src/handlers/orchestrate-task.ts @@ -37,6 +37,7 @@ import { type PollState, } from './shared/orchestrator'; import { runPreflightChecks } from './shared/preflight'; +import { isAutoRetried, startSessionWithRetry } from './shared/session-start-retry'; import { deleteEcsPayload } from './shared/strategies/ecs-strategy'; import type { TaskRecord } from './shared/types'; import { workflowIsReadOnly, workflowRequiresRepo } from './shared/workflows'; @@ -160,14 +161,32 @@ const durableHandler: DurableExecutionHandler = asyn // Step 4: Start agent session — resolve compute strategy, invoke runtime, transition to RUNNING // Returns the full SessionHandle (serializable) so ECS polling can use it in step 5. const sessionHandle = await context.step('start-session', async () => { + let autoRetried = false; try { const strategy = resolveComputeStrategy(blueprintConfig); - const handle = await strategy.startSession({ + const startInput = { taskId, userId: task.user_id, payload, blueprintConfig, - }); + }; + // Transient-error AUTO-RETRY (once), extracted to startSessionWithRetry so + // the four branches are unit-tested (#599 B2). The retry-event emit is + // best-effort and guarded there (#599 B1): a TaskEvents PutItem fault can't + // abort or mis-attribute the retry. The correlation envelope is threaded so + // the session_start_retry event carries the same trace context as + // session_started below (#245). + const { handle, autoRetried: retried } = await startSessionWithRetry( + strategy, + startInput, + { + emitRetryEvent: (reason) => + emitTaskEvent(taskId, 'session_start_retry', { reason }, correlation), + logger, + taskId, + }, + ); + autoRetried = retried; // Build compute metadata for the task record so cancel-task can stop the right backend const computeMetadata: Record = handle.strategyType === 'ecs' @@ -193,7 +212,21 @@ const durableHandler: DurableExecutionHandler = asyn return handle; } catch (err) { - await failTask(taskId, TaskStatus.HYDRATING, `Session start failed: ${String(err)}`, task.user_id, true, task.repo); + // Carry the auto-retry fact into error_message: the `[auto-retried]` suffix + // is persisted verbatim (the classifier ignores it — it does not affect + // classification). It is a breadcrumb for a FORTHCOMING failure renderer to + // detect and surface "I already tried again" to the channel — no consumer + // renders it yet on this branch (retryGuidance() in error-classifier.ts is + // the intended copy source; it ships ahead of its consumer). + // + // Stamp on BOTH paths a retry ran (#599 N1): branch 3 sets `autoRetried` + // above then a LATER step throws; branch 4 (transient-then-transient) throws + // FROM startSessionWithRetry before `autoRetried` is assigned, so the local + // is still false — read the fact off the thrown error via isAutoRetried(). + // Without this, a double-transient failure was told "reply to retry" instead + // of "I already retried" — the exact confusion the marker exists to prevent. + const retriedNote = (autoRetried || isAutoRetried(err)) ? ' [auto-retried]' : ''; + await failTask(taskId, TaskStatus.HYDRATING, `Session start failed: ${String(err)}${retriedNote}`, task.user_id, true, task.repo); throw err; } }); diff --git a/cdk/src/handlers/shared/error-classifier.ts b/cdk/src/handlers/shared/error-classifier.ts index 1aecaee3..41d538e8 100644 --- a/cdk/src/handlers/shared/error-classifier.ts +++ b/cdk/src/handlers/shared/error-classifier.ts @@ -39,6 +39,30 @@ export const ErrorCategory = { export type ErrorCategoryType = (typeof ErrorCategory)[keyof typeof ErrorCategory]; +/** + * WHO should act, and whether retrying the SAME request can help — the axis a + * channel reader needs to answer "just retry, or tell my admin?". Distinct from + * ``category`` (which names WHAT broke) and from ``retryable`` (a plain boolean + * that conflates "self-heals on retry" with "you must change something first"): + * - ``transient`` — an infrastructure/service HICCUP that usually clears itself: + * a retry of the identical request is the right move (ECS deploy-race, ENI + * delay, network blip, Bedrock throttle/5xx, concurrency cap). The platform + * may auto-retry these once at session-start; the user just retries otherwise. + * - ``service`` — a real PLATFORM/CONFIG fault an operator owns: retrying the + * same request won't change the outcome until an admin fixes the setup (bad + * token/scopes, model not enabled, quota, blueprint misconfig). + * - ``user`` — the REQUEST or the code is the thing to change: the build/tests + * failed, content was blocked, the repo/PR wasn't found, max turns/budget hit. + * Every classification carries exactly one. Guidance copy is derived from this. + */ +export const ErrorClass = { + TRANSIENT: 'transient', + SERVICE: 'service', + USER: 'user', +} as const; + +export type ErrorClassType = (typeof ErrorClass)[keyof typeof ErrorClass]; + /** * Structured classification of a task error. */ @@ -48,6 +72,14 @@ export interface ErrorClassification { readonly description: string; readonly remedy: string; readonly retryable: boolean; + /** + * transient (self-heals on retry) vs service (admin must fix) vs user (change + * the request/code). Drives {@link retryGuidance} and the session-start + * auto-retry. Optional so older/hand-built classifications still type-check; + * absent ⇒ treated as ``user`` (safest: don't promise a retry works, don't + * auto-retry). New PATTERNS should always set it. + */ + readonly errorClass?: ErrorClassType; } interface ErrorPattern { @@ -66,6 +98,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The GitHub token does not have the required permissions for this repository.', remedy: 'Verify the PAT has Contents (Read and write), Pull requests (Read and write), and Issues (Read) scopes for this repo. See the developer guide.', retryable: false, + errorClass: ErrorClass.SERVICE, }, }, { @@ -76,6 +109,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The GitHub token cannot access the target repository. It may not exist or the token lacks visibility.', remedy: 'Check that the repository name is correct and the configured PAT has access to it.', retryable: false, + errorClass: ErrorClass.SERVICE, }, }, { @@ -86,6 +120,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The specified pull request does not exist or has already been closed.', remedy: 'Verify the PR number is correct and the PR is still open.', retryable: false, + errorClass: ErrorClass.USER, }, }, { @@ -96,6 +131,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The GitHub token is missing required scopes for the requested operation.', remedy: 'Update the PAT with Contents (Read and write), Pull requests (Read and write), and Issues (Read) scopes.', retryable: false, + errorClass: ErrorClass.SERVICE, }, }, @@ -108,6 +144,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'Could not reach the GitHub API during pre-flight checks.', remedy: 'Check network connectivity and DNS Firewall rules. GitHub may be experiencing an outage.', retryable: true, + errorClass: ErrorClass.TRANSIENT, }, }, { @@ -118,6 +155,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The GitHub API returned an error response during pre-flight checks.', remedy: 'Check the HTTP status code in the error detail. Retry if transient (5xx), or fix credentials if 401/403.', retryable: true, + errorClass: ErrorClass.TRANSIENT, }, }, @@ -130,10 +168,27 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The maximum number of concurrent tasks for this user has been reached.', remedy: 'Wait for an active task to complete, cancel a running task, or ask an admin to increase the limit.', retryable: true, + errorClass: ErrorClass.TRANSIENT, }, }, // --- Compute --- + { + // A task dispatched against a task-def revision that was deregistered by a + // deploy (ABCA-660/663). Transient + self-clearing on retry; the family-based + // RunTask fix prevents it going forward, but keep a precise classification so + // any historical/edge occurrence reads as "temporary, just retry", not a + // scary compute-health alarm. + pattern: /TaskDefinition is inactive/i, + classification: { + category: ErrorCategory.COMPUTE, + title: 'Couldn\'t start — the compute environment was mid-update', + description: 'The task was dispatched against an ECS task definition revision that a concurrent deployment had just replaced.', + remedy: 'This is a transient deploy-timing race, not a problem with your request. Retry the task; it will pick up the current task definition. If it persists, an admin should check for a stuck/failed deployment.', + retryable: true, + errorClass: ErrorClass.TRANSIENT, + }, + }, { pattern: /Session start failed/i, classification: { @@ -142,6 +197,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The compute backend could not start an agent session.', remedy: 'Check AgentCore Runtime or ECS cluster health. The runtime ARN may be invalid or the service quota may be exhausted.', retryable: true, + errorClass: ErrorClass.TRANSIENT, }, }, { @@ -152,6 +208,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The ECS Fargate container exited with an error.', remedy: 'Check the container logs in CloudWatch for the specific failure reason (OOM, image pull failure, etc.).', retryable: true, + errorClass: ErrorClass.TRANSIENT, }, }, { @@ -162,6 +219,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The ECS container exited successfully but the agent never wrote a terminal status to DynamoDB.', remedy: 'Check agent logs for crashes after the main pipeline completed. This may indicate a bug in the agent finalization code.', retryable: true, + errorClass: ErrorClass.TRANSIENT, }, }, { @@ -172,6 +230,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'Repeated failures polling the ECS task status.', remedy: 'Check ECS cluster health and IAM permissions for DescribeTasks.', retryable: true, + errorClass: ErrorClass.TRANSIENT, }, }, { @@ -182,6 +241,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The task remained in HYDRATING state — the agent container never transitioned to RUNNING.', remedy: 'Check if the container image pulled successfully and the runtime is available. Review CloudWatch logs for the session.', retryable: true, + errorClass: ErrorClass.TRANSIENT, }, }, { @@ -192,6 +252,31 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The agent stopped sending heartbeats. The container may have crashed, been OOM-killed, or stopped unexpectedly.', remedy: 'Check CloudWatch logs for the agent session. If OOM, consider a less memory-intensive task or a larger container.', retryable: true, + errorClass: ErrorClass.TRANSIENT, + }, + }, + { + // The `claude` CLI on the agent image couldn't be exec'd: either the OS + // refused the binary (`OSError: [Errno 8] Exec format error: 'claude'`) or + // the claude-code shim reports its platform-native binary was never placed + // ("claude native binary not installed" — its postinstall silently fell + // back at image-build time). Live-caught on ABCA-659's retry: all 3 ECS runs + // died at the run_agent step this way on a freshly rebuilt image, while the + // native binary was present but unwired. This is an IMAGE/infra fault, NOT a + // problem with the user's request — a fresh attempt usually lands on a host + // that materializes the image cleanly; a persistent one is a bad build an + // admin must rebuild. Without this bucket it fell through to a bare + // "Unexpected error" with no guidance (the anti-pattern the error-feedback + // work set out to kill). Matched before AGENT/UNKNOWN so the precise, + // retry-oriented copy wins. + pattern: /Exec format error.*claude|claude.*Exec format error|claude native binary not installed/i, + classification: { + category: ErrorCategory.COMPUTE, + title: 'Couldn\'t start the coding agent (environment issue)', + description: 'The agent runtime couldn\'t launch the `claude` CLI on the compute image — an infrastructure/image problem, not a problem with your request or code.', + remedy: 'This is usually a transient image/compute hiccup. Reply here to try again — a fresh attempt typically clears it. If every attempt fails the same way, the agent image needs a rebuild: contact your ABCA admin with the task id above.', + retryable: true, + errorClass: ErrorClass.TRANSIENT, }, }, @@ -204,43 +289,62 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The Claude Agent SDK stream closed without returning a result. This may indicate a network interruption, SDK bug, or protocol mismatch.', remedy: 'Retry the task. If persistent, check the agent container logs and SDK version compatibility.', retryable: true, + errorClass: ErrorClass.TRANSIENT, }, }, // Specific agent_status classifiers — ordered BEFORE the generic // ``Task did not succeed.*agent_status=`` catch-all so the concrete // cap / runtime-error signals surface to users rather than the // opaque "Agent task did not succeed" title. Each matches the - // ``agent_status`` literals emitted by ``agent/src/pipeline.py`` - // (see ``_resolve_overall_task_status``) and - // ``agent/src/runner.py``. + // status literal under BOTH wrappers the agent emits: + // - ``agent_status=error_max_turns`` — ``agent/src/pipeline.py`` + // (``_resolve_overall_task_status``); and + // - ``Agent session error (subtype='error_max_turns')`` — + // ``agent/src/runner.py:515`` (the terminal-error path). + // Keying on only ``agent_status=`` missed the ``subtype=`` wrapper, so a + // 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=``. { - pattern: /agent_status=['"]?error_max_turns['"]?/i, + // 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, }, }, { - pattern: /agent_status=['"]?error_max_budget_usd['"]?/i, + pattern: /(?:agent_status|subtype)=['"]?error_max_budget_usd['"]?/i, classification: { category: ErrorCategory.TIMEOUT, title: 'Exceeded max budget', description: 'The agent reached the configured ``max_budget_usd`` limit before completing.', remedy: 'Raise ``--max-budget`` on the submit call, simplify the task, or break it into smaller sub-tasks.', retryable: true, + errorClass: ErrorClass.USER, }, }, { - pattern: /agent_status=['"]?error_during_execution['"]?/i, + pattern: /(?:agent_status|subtype)=['"]?error_during_execution['"]?/i, classification: { category: ErrorCategory.AGENT, title: 'Agent errored during execution', description: 'The agent raised an uncaught error mid-turn. The Claude Agent SDK reported the task as failed before a clean terminal.', remedy: 'Retry the task. If persistent, check the agent container logs and the PR branch for partial state.', retryable: true, + errorClass: ErrorClass.TRANSIENT, }, }, { @@ -251,6 +355,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The agent completed but reported a non-success status.', remedy: 'Check the agent logs and PR (if created) for details on what went wrong during execution.', retryable: false, + errorClass: ErrorClass.USER, }, }, { @@ -261,6 +366,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The agent runner failed to receive a response from the Claude Agent SDK.', remedy: 'Retry the task. If persistent, check Bedrock model availability and agent container connectivity.', retryable: true, + errorClass: ErrorClass.TRANSIENT, }, }, @@ -273,6 +379,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'Bedrock Guardrails blocked the task content during hydration.', remedy: 'Review the task description, issue body, or PR content for policy violations. Rephrase and resubmit.', retryable: false, + errorClass: ErrorClass.USER, }, }, { @@ -283,6 +390,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The task description was blocked by the content screening policy.', remedy: 'Rephrase the task description to comply with content policy guidelines.', retryable: false, + errorClass: ErrorClass.USER, }, }, @@ -297,6 +405,7 @@ const PATTERNS: readonly ErrorPattern[] = [ remedy: 'Complete model access prerequisites in Amazon Bedrock (Anthropic first-time use via the console model catalog or PutUseCaseForModelAccess; AWS Marketplace Subscribe/ViewSubscriptions for first-time serverless model enablement where required; valid payment method for Marketplace-backed models). Grant bedrock:InvokeModel* on the inference profile and foundation model. For InvokeModel, use a supported inference profile ID in modelId where on-demand requires it. See https://docs.aws.amazon.com/bedrock/latest/userguide/model-access.html and https://docs.aws.amazon.com/bedrock/latest/userguide/inference-profiles-use.html', retryable: false, + errorClass: ErrorClass.SERVICE, }, }, { @@ -307,6 +416,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'Failed to load the per-repo Blueprint configuration from DynamoDB.', remedy: 'Verify the Blueprint construct is deployed correctly for this repository. Check the RepoTable in DynamoDB.', retryable: true, + errorClass: ErrorClass.SERVICE, }, }, { @@ -318,6 +428,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'Failed to assemble the task context (issue content, PR data, memory).', remedy: 'Check GitHub API accessibility, token permissions, and Bedrock Guardrails availability.', retryable: true, + errorClass: ErrorClass.TRANSIENT, }, }, @@ -330,6 +441,7 @@ const PATTERNS: readonly ErrorPattern[] = [ description: 'The orchestrator polling window expired before the agent completed.', remedy: 'The task may be too large for the configured turn/budget limits. Consider breaking it into smaller tasks or increasing max_turns.', retryable: false, + errorClass: ErrorClass.TRANSIENT, }, }, ]; @@ -461,6 +573,10 @@ const UNKNOWN_CLASSIFICATION: ErrorClassification = { description: 'An unrecognized error occurred during task execution.', remedy: 'Check the full error message and agent logs for details. If the issue persists, report it.', retryable: false, + // Unknown = don't over-promise: a retry MIGHT clear a one-off, but we can't + // assert it, so treat like 'user' (surface + suggest escalation) rather than + // auto-retrying an error we don't understand. + errorClass: ErrorClass.USER, }; /** @@ -492,3 +608,64 @@ export function classifyError(errorMessage: string | undefined | null): ErrorCla return UNKNOWN_CLASSIFICATION; } + +/** + * True when the error is a transient infrastructure/service HICCUP that a plain + * retry usually clears (see {@link ErrorClass}). Used to gate the session-start + * auto-retry AND to tune the guidance copy. Absent errorClass ⇒ NOT transient + * (conservative: never auto-retry an error we didn't explicitly mark). + */ +export function isTransientError(classification: ErrorClassification | null | undefined): boolean { + return classification?.errorClass === ErrorClass.TRANSIENT; +} + +/** + * One short, user-facing NEXT-STEP line for a classified failure — the answer to + * "should I just retry this, or tell my admin?" that a channel reader (Linear/ + * Slack) can act on WITHOUT reading CloudWatch. Derived from the classification's + * ``errorClass`` (transient / service / user — never the raw error), so it stays + * safe to show and consistent with the CLI's structured display. + * + * The three-way split (which the ``retryable`` boolean alone couldn't express): + * - **transient** — infra/service hiccup, request is fine, a retry clears it + * (ECS deploy-race, ENI delay, network blip, throttle, concurrency cap). If + * ``autoRetried`` is set, say we ALREADY retried once and it still failed. + * - **service** — a real platform/config fault an operator owns; retrying the + * same request won't change the outcome until an admin fixes the setup. + * - **user** — the request or the code is the thing to change (build/test + * failed, content blocked, wrong PR, max turns) — a plain reply-to-retry with + * guidance, except guardrail which needs an edit. + * Returned WITHOUT a trailing space; callers add their own separator. + * + * @param autoRetried set when the platform already auto-retried a transient + * failure once (session-start) — the copy then reflects "tried again, still + * failed" instead of "reply to retry". + */ +export function retryGuidance( + classification: ErrorClassification, + autoRetried = false, +): string { + const cls = classification.errorClass ?? ErrorClass.USER; + + if (cls === ErrorClass.TRANSIENT) { + return autoRetried + ? 'This looks like a temporary infrastructure issue — I automatically tried again and it still failed. ' + + 'Reply here to retry, or if it keeps happening, contact your ABCA admin.' + : 'This is usually a temporary infrastructure issue, not a problem with your request — ' + + 'reply here to try again. If it keeps happening, contact your ABCA admin.'; + } + if (cls === ErrorClass.SERVICE) { + // Platform/config fault — a plain retry won't change the outcome; an admin owns it. + return 'Retrying as-is won\'t fix this — it needs your ABCA admin to correct the access or configuration, then re-apply the label.'; + } + // user: the request/code is the thing to change. + if (classification.category === ErrorCategory.GUARDRAIL) { + return 'Retrying the same text won\'t help — edit the request to remove the flagged content, then re-apply the label.'; + } + if (classification.retryable) { + // build/test failed, max-turns, transient agent crash → a fresh attempt (or guidance) can clear it. + return 'Reply here with any extra guidance and I\'ll try again.'; + } + // not-retryable user/unknown (e.g. agent reported non-success) — don't promise a retry works. + return 'A retry may not resolve this on its own — if it repeats, contact your ABCA admin with the task id above.'; +} diff --git a/cdk/src/handlers/shared/session-start-retry.ts b/cdk/src/handlers/shared/session-start-retry.ts new file mode 100644 index 00000000..2f7156e3 --- /dev/null +++ b/cdk/src/handlers/shared/session-start-retry.ts @@ -0,0 +1,148 @@ +/** + * MIT No Attribution + * + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of + * the Software, and to permit persons to whom the Software is furnished to do so. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +/** + * Session-start transient auto-retry (once), extracted from the durable + * ``orchestrate-task`` handler so the four retry branches are unit-testable in + * isolation (the handler's inline ``start-session`` step is never invoked by + * the test suite). See #599 review B1/B2. + * + * session-start is the ONE place a retry is idempotent by construction — no repo + * clone, no commits, no PR have happened yet, so re-invoking + * RunTask/InvokeAgentRuntime can't double-run work. A transient hiccup here (an + * ECS deploy-race "TaskDefinition is inactive", ENI/capacity delay, a + * Bedrock/agentcore throttle) usually clears on a second attempt, so the first + * transient failure is swallowed and retried once. A NON-transient failure (bad + * config, missing ECS substrate) is re-thrown immediately — retrying it just + * wastes ~a minute. Mid-run crashes are NOT handled here (that's a later step; + * the agent may have pushed commits). + */ + +import type { ComputeStrategy, SessionHandle } from './compute-strategy'; +import { classifyError, isTransientError } from './error-classifier'; + +/** Emit a ``session_start_retry`` telemetry event. Matches the shape of + * ``emitTaskEvent`` bound at the call site (best-effort — see below). */ +export type RetryEventEmitter = ( + reason: string, +) => Promise; + +/** Minimal logger surface (a subset of the handler's structured logger). */ +export interface RetryLogger { + warn(message: string, meta?: Record): void; +} + +export interface StartSessionWithRetryResult { + readonly handle: SessionHandle; + /** True iff the first attempt failed transiently and a second attempt ran. */ + readonly autoRetried: boolean; +} + +/** + * Symbol tag pinned onto the error thrown from branch 4 (transient-then-transient) + * so the retry fact survives the throw — the ``autoRetried`` result field only + * exists on the success paths. Kept as a Symbol (not an own string prop) so it + * never collides with, or leaks into, the error's serialized shape. Read via + * {@link isAutoRetried}. + */ +const AUTO_RETRIED = Symbol('autoRetried'); + +/** True iff ``err`` was thrown after an auto-retry already ran (branch 4). */ +export function isAutoRetried(err: unknown): boolean { + return typeof err === 'object' && err !== null && (err as Record)[AUTO_RETRIED] === true; +} + +/** + * Start a compute session, auto-retrying ONCE on a transient failure. + * + * Behaviour (the four branches #599 B2 asks be covered): + * 1. first attempt succeeds → return it, ``autoRetried: false``. + * 2. first attempt fails NON-transient → re-throw the original error (no retry). + * 3. first attempt fails transient, retry succeeds → return it, ``autoRetried: true``. + * 4. first attempt fails transient, retry also fails → throw the retry's error + * TAGGED with ``autoRetried = true`` (see {@link isAutoRetried}). The result + * object carries ``autoRetried`` only on the success paths (1/3); on the + * double-failure this function throws, so the fact is instead pinned onto the + * thrown error itself — the caller reads it via {@link isAutoRetried} to stamp + * the ``[auto-retried]`` marker. Without the tag the caller could not tell a + * double-transient failure (retry ran) from a first-attempt failure (it did + * not), and would wrongly tell the user "reply to retry" (N1). + * + * The ``emitRetryEvent`` call is BEST-EFFORT and internally guarded (B1): a + * TaskEvents PutItem fault (throttle/timeout — exactly the conditions that + * co-occur with the transient session-start failures this handles) must NOT + * abort or mis-attribute the retry. A telemetry failure is logged and swallowed; + * the retry proceeds regardless. Previously the emit was unguarded and, if it + * threw after ``autoRetried`` was set but before the retry ran, the user was + * told a second attempt failed when none had. + */ +export async function startSessionWithRetry( + strategy: Pick, + input: Parameters[0], + deps: { + emitRetryEvent: RetryEventEmitter; + logger: RetryLogger; + taskId: string; + }, +): Promise { + try { + const handle = await strategy.startSession(input); + return { handle, autoRetried: false }; + } catch (firstErr) { + // Classify the RAW error, NOT a `Session start failed: …` wrapper (#599 N2): + // `/Session start failed/i` is itself a TRANSIENT pattern, so wrapping made + // EVERY session-start failure classify transient — a genuine config/auth + // fault (missing ECS env, AccessDenied) would eat a pointless ~1-min retry + // and the "non-transient throws immediately" branch below was effectively + // dead. Classifying the raw string restores that branch. (Widening the + // classifier's transient patterns — e.g. ThrottlingException — is a separate + // classifier-completeness concern, not this retry gate.) + const classification = classifyError(String(firstErr)); + if (!isTransientError(classification)) { + throw firstErr; // service/user error — a retry won't help; surface now. + } + deps.logger.warn('Session start hit a transient error — auto-retrying once', { + task_id: deps.taskId, + error: firstErr instanceof Error ? firstErr.message : String(firstErr), + }); + // Best-effort telemetry — a PutItem fault here must never abort the retry + // or mis-report the outcome (B1). + try { + await deps.emitRetryEvent(classification?.title ?? 'transient'); + } catch (emitErr) { + deps.logger.warn('session_start_retry event emit failed (non-fatal)', { + task_id: deps.taskId, + error: emitErr instanceof Error ? emitErr.message : String(emitErr), + }); + } + try { + const handle = await strategy.startSession(input); + return { handle, autoRetried: true }; + } catch (retryErr) { + // Branch 4: the retry ALSO failed. Pin the retry fact onto the thrown error + // so the caller can stamp ``[auto-retried]`` (N1) — otherwise a double- + // transient failure is indistinguishable from a first-attempt failure and + // the user is wrongly told "reply to retry" instead of "I already retried". + if (typeof retryErr === 'object' && retryErr !== null) { + (retryErr as Record)[AUTO_RETRIED] = true; + } + throw retryErr; + } + } +} diff --git a/cdk/test/handlers/shared/error-classifier.test.ts b/cdk/test/handlers/shared/error-classifier.test.ts index 5410fb77..8e6a8b48 100644 --- a/cdk/test/handlers/shared/error-classifier.test.ts +++ b/cdk/test/handlers/shared/error-classifier.test.ts @@ -17,7 +17,7 @@ * SOFTWARE. */ -import { classifyError, ErrorCategory, type ErrorClassification } from '../../../src/handlers/shared/error-classifier'; +import { classifyError, ErrorCategory, ErrorClass, isTransientError, retryGuidance, type ErrorClassification } from '../../../src/handlers/shared/error-classifier'; import { toTaskDetail, type TaskRecord } from '../../../src/handlers/shared/types'; describe('classifyError', () => { @@ -157,6 +157,27 @@ describe('classifyError', () => { expect(result!.retryable).toBe(true); }); + test('classifies claude Exec-format / broken-shim as a transient image issue (ABCA-659, not "Unexpected error")', () => { + // The raw run_agent failure the broken agent image produced. + const result = classifyError( + "Workflow run_agent step failed: OSError: [Errno 8] Exec format error: 'claude'", + ); + expect(result!.category).toBe(ErrorCategory.COMPUTE); + expect(result!.title).toBe('Couldn\'t start the coding agent (environment issue)'); + expect(result!.retryable).toBe(true); + // MUST be transient so retryGuidance tells the user to just reply-to-retry + // (and escalate to an admin only if it persists) — not the bare + // "Unexpected error" with no guidance it used to fall through to. + expect(result!.errorClass).toBe(ErrorClass.TRANSIENT); + expect(result!.remedy).toMatch(/try again|rebuild|admin/i); + }); + + test('classifies the claude shim self-report ("native binary not installed")', () => { + const result = classifyError('Error: claude native binary not installed.'); + expect(result!.category).toBe(ErrorCategory.COMPUTE); + expect(result!.errorClass).toBe(ErrorClass.TRANSIENT); + }); + test('classifies ECS exit without terminal status', () => { const result = classifyError( 'ECS task exited successfully but agent never wrote terminal status after 5 polls', @@ -237,6 +258,28 @@ describe('classifyError', () => { expect(result!.remedy).toMatch(/--max-turns/); }); + 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, 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') — 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('Exceeded max turns'); + expect(result!.retryable).toBe(true); + // 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', () => { const result = classifyError( "Task did not succeed (agent_status='error_max_budget_usd', build_ok=False)", @@ -256,6 +299,23 @@ describe('classifyError', () => { expect(result!.retryable).toBe(true); }); + test('classifies the runner.py "Agent session error (subtype=...)" wrapper, not just agent_status= (K5, live-caught ABCA-483)', () => { + // runner.py:515 emits ``Agent session error (subtype='error_max_turns')`` + // — a DIFFERENT wrapper from pipeline.py's ``agent_status=``. Pre-K5 this + // fell through to UNKNOWN → "Unexpected error" even though the task hit the + // 100-turn cap (live: a 1-line README task burned 101 turns, reply said + // "Unexpected error"). The pattern must match the subtype= wrapper too. + const turns = classifyError("Agent session error (subtype='error_max_turns')"); + expect(turns!.title).toBe('Exceeded max turns'); + expect(turns!.category).toBe(ErrorCategory.TIMEOUT); + + const budget = classifyError("Agent session error (subtype='error_max_budget_usd')"); + expect(budget!.title).toBe('Exceeded max budget'); + + const exec = classifyError("Agent session error (subtype='error_during_execution')"); + expect(exec!.title).toBe('Agent errored during execution'); + }); + test('matches agent_status with or without quotes around the literal', () => { // Defensive: the agent writer currently emits single-quoted // repr values (``agent_status='error_max_turns'``) but a future @@ -486,10 +546,99 @@ describe('classifyError', () => { expect(result.title.length).toBeGreaterThan(0); expect(result.description.length).toBeGreaterThan(0); expect(result.remedy.length).toBeGreaterThan(0); + // Every classification carries a 3-way errorClass (transient/service/user). + expect([ErrorClass.TRANSIENT, ErrorClass.SERVICE, ErrorClass.USER]).toContain(result.errorClass); } }); }); + // --- errorClass + retryGuidance (transient vs service vs user) --- + + describe('errorClass axis + retryGuidance', () => { + test('the ECS deploy-race is TRANSIENT and isTransientError is true', () => { + const c = classifyError('Session start failed: InvalidParameterException: TaskDefinition is inactive')!; + expect(c.errorClass).toBe(ErrorClass.TRANSIENT); + expect(isTransientError(c)).toBe(true); + }); + + test('a generic session-start failure is TRANSIENT (compute infra)', () => { + expect(classifyError('Session start failed: boom')!.errorClass).toBe(ErrorClass.TRANSIENT); + }); + + test('auth/permission is SERVICE (admin fixes it), not transient', () => { + const c = classifyError('INSUFFICIENT_GITHUB_REPO_PERMISSIONS')!; + expect(c.errorClass).toBe(ErrorClass.SERVICE); + expect(isTransientError(c)).toBe(false); + }); + + test('a build/guardrail failure is USER (change the request/code)', () => { + expect(classifyError('Guardrail blocked: nope')!.errorClass).toBe(ErrorClass.USER); + expect(classifyError('Task did not succeed: agent_status="error_max_turns"')!.errorClass).toBe(ErrorClass.USER); + }); + + test('retryGuidance: TRANSIENT → "temporary … reply to retry … contact admin if it persists"', () => { + const g = retryGuidance(classifyError('Session start failed: boom')!); + expect(g).toMatch(/temporary infrastructure/i); + expect(g).toMatch(/reply here to try again/i); + expect(g).toMatch(/contact your ABCA admin/i); + }); + + test('retryGuidance: TRANSIENT + autoRetried → "I automatically tried again and it still failed"', () => { + const g = retryGuidance(classifyError('Session start failed: boom')!, true); + expect(g).toMatch(/automatically tried again/i); + }); + + test('retryGuidance: SERVICE → "retrying won\'t fix this … your ABCA admin"', () => { + const g = retryGuidance(classifyError('INSUFFICIENT_GITHUB_REPO_PERMISSIONS')!); + expect(g).toMatch(/won'?t fix this/i); + expect(g).toMatch(/admin/i); + expect(g).not.toMatch(/temporary infrastructure/i); + }); + + test('retryGuidance: USER guardrail → "edit the request"', () => { + const g = retryGuidance(classifyError('Guardrail blocked: nope')!); + expect(g).toMatch(/edit the request/i); + }); + + // #599 N3: pin the two USER fall-through branches so the #247 failure-renderer + // contract can't rot silently. Built as explicit classifications (the exact + // category/errorClass/retryable each branch keys on) rather than relying on a + // sample string that might reclassify later. + test('retryGuidance: retryable USER (non-guardrail) → "reply here with any extra guidance"', () => { + const cls: ErrorClassification = { + category: ErrorCategory.AGENT, + title: 'build failed', + description: 'the build/test step failed', + remedy: 'fix the failing step', + retryable: true, + errorClass: ErrorClass.USER, + }; + const g = retryGuidance(cls); + expect(g).toMatch(/extra guidance/i); + expect(g).toMatch(/try again/i); + expect(g).not.toMatch(/edit the request/i); // not the guardrail branch + }); + + test('retryGuidance: not-retryable USER/unknown → "a retry may not resolve this"', () => { + const cls: ErrorClassification = { + category: ErrorCategory.UNKNOWN, + title: 'agent reported non-success', + description: 'the agent finished without success', + remedy: 'review the task output', + retryable: false, + errorClass: ErrorClass.USER, + }; + const g = retryGuidance(cls); + expect(g).toMatch(/may not resolve this/i); + expect(g).toMatch(/contact your ABCA admin/i); + }); + + test('isTransientError is false for null / absent classification', () => { + expect(isTransientError(null)).toBe(false); + expect(isTransientError(undefined)).toBe(false); + }); + }); + // --- Priority / ordering --- describe('pattern priority', () => { diff --git a/cdk/test/handlers/shared/session-start-retry.test.ts b/cdk/test/handlers/shared/session-start-retry.test.ts new file mode 100644 index 00000000..f6227278 --- /dev/null +++ b/cdk/test/handlers/shared/session-start-retry.test.ts @@ -0,0 +1,130 @@ +/** + * MIT No Attribution + * + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of + * the Software, and to permit persons to whom the Software is furnished to do so. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +import type { SessionHandle } from '../../../src/handlers/shared/compute-strategy'; +import { isAutoRetried, startSessionWithRetry } from '../../../src/handlers/shared/session-start-retry'; + +const HANDLE: SessionHandle = { + sessionId: 'sess-1', + strategyType: 'agentcore', + runtimeArn: 'arn:aws:bedrock-agentcore:us-east-1:1:runtime/r', +}; + +/** A transient session-start failure the classifier recognizes (ECS deploy race). */ +const TRANSIENT = new Error('TaskDefinition is inactive'); +/** A non-transient failure (a retry won't help). */ +const NON_TRANSIENT = new Error('ECS_CLUSTER_ARN is not configured'); + +function deps(overrides?: { emitRetryEvent?: (reason: string) => Promise }) { + const warns: Array<{ message: string; meta?: Record }> = []; + const emitReasons: string[] = []; + return { + warns, + emitReasons, + d: { + emitRetryEvent: + overrides?.emitRetryEvent ?? + (async (reason: string) => { + emitReasons.push(reason); + }), + logger: { warn: (message: string, meta?: Record) => warns.push({ message, meta }) }, + taskId: 't-1', + }, + }; +} + +describe('startSessionWithRetry — the 4 branches (#599 B2)', () => { + it('1. first attempt succeeds → returns handle, autoRetried false, no retry', async () => { + const startSession = jest.fn().mockResolvedValueOnce(HANDLE); + const { d, emitReasons } = deps(); + const res = await startSessionWithRetry({ startSession }, {} as never, d); + expect(res).toEqual({ handle: HANDLE, autoRetried: false }); + expect(startSession).toHaveBeenCalledTimes(1); + expect(emitReasons).toEqual([]); // no retry event on the happy path + }); + + it('2. first attempt fails NON-transient → re-throws original, NO retry', async () => { + const startSession = jest.fn().mockRejectedValueOnce(NON_TRANSIENT); + const { d, emitReasons } = deps(); + await expect(startSessionWithRetry({ startSession }, {} as never, d)).rejects.toBe(NON_TRANSIENT); + expect(startSession).toHaveBeenCalledTimes(1); // never retried + expect(emitReasons).toEqual([]); + }); + + it('3. transient then success → returns handle, autoRetried true, emits retry event', async () => { + const startSession = jest + .fn() + .mockRejectedValueOnce(TRANSIENT) + .mockResolvedValueOnce(HANDLE); + const { d, emitReasons } = deps(); + const res = await startSessionWithRetry({ startSession }, {} as never, d); + expect(res).toEqual({ handle: HANDLE, autoRetried: true }); + expect(startSession).toHaveBeenCalledTimes(2); + expect(emitReasons).toHaveLength(1); // the session_start_retry event fired once + }); + + it('4. transient then transient → throws the retry error TAGGED autoRetried (#599 N1)', async () => { + const secondErr = new Error('TaskDefinition is inactive (again)'); + const startSession = jest + .fn() + .mockRejectedValueOnce(TRANSIENT) + .mockRejectedValueOnce(secondErr); + const { d } = deps(); + // The double-transient error must carry the retry fact across the throw so the + // caller stamps `[auto-retried]` (a first-attempt failure must NOT be tagged). + await expect(startSessionWithRetry({ startSession }, {} as never, d)).rejects.toBe(secondErr); + expect(startSession).toHaveBeenCalledTimes(2); + expect(isAutoRetried(secondErr)).toBe(true); + }); + + it('isAutoRetried is false for a first-attempt (non-transient) failure and non-objects', async () => { + // Branch 2's re-thrown error ran no retry → must NOT be tagged, else the caller + // would wrongly tell the user "I already retried". + const startSession = jest.fn().mockRejectedValueOnce(NON_TRANSIENT); + const { d } = deps(); + await expect(startSessionWithRetry({ startSession }, {} as never, d)).rejects.toBe(NON_TRANSIENT); + expect(isAutoRetried(NON_TRANSIENT)).toBe(false); + expect(isAutoRetried(undefined)).toBe(false); + expect(isAutoRetried('a string error')).toBe(false); + }); +}); + +describe('startSessionWithRetry — retry-event emit is best-effort (#599 B1)', () => { + it('a telemetry failure does NOT abort or mis-attribute the retry', async () => { + // The exact B1 scenario: the TaskEvents PutItem throws (throttle/timeout, + // co-occurring with the transient session-start failure). The retry must + // still run and succeed — the emit fault must not surface as the failure. + const startSession = jest + .fn() + .mockRejectedValueOnce(TRANSIENT) + .mockResolvedValueOnce(HANDLE); + const emitErr = new Error('ProvisionedThroughputExceededException'); + const { d, warns } = deps({ + emitRetryEvent: async () => { + throw emitErr; + }, + }); + const res = await startSessionWithRetry({ startSession }, {} as never, d); + // Retry proceeded and succeeded despite the emit throwing. + expect(res).toEqual({ handle: HANDLE, autoRetried: true }); + expect(startSession).toHaveBeenCalledTimes(2); + // The emit failure was logged (WARN), not propagated. + expect(warns.some((w) => w.message.includes('event emit failed'))).toBe(true); + }); +}); diff --git a/cli/src/types.ts b/cli/src/types.ts index 3ee7f91f..b0a9f344 100644 --- a/cli/src/types.ts +++ b/cli/src/types.ts @@ -75,6 +75,11 @@ export interface ErrorClassification { readonly description: string; readonly remedy: string; readonly retryable: boolean; + /** Retry-semantics axis: transient (self-heals on retry) vs service (admin + * must fix) vs user (change the request/code). Optional (older classifications + * omit it; absent ⇒ user). Inlined (not a named export) to mirror the cdk + * ErrorClassification field without introducing a CLI-only exported type. */ + readonly errorClass?: 'transient' | 'service' | 'user'; } /** Task detail returned by GET /v1/tasks/{task_id}. */