From e2b777a0e57cd2e959913d6971511cef4cb93442 Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Mon, 29 Jun 2026 10:12:22 -0400 Subject: [PATCH 1/8] =?UTF-8?q?fix(agent-classifier):=20recognize=20'Agent?= =?UTF-8?q?=20session=20error=20(subtype=3D=E2=80=A6)'=20wrapper?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The error classifier keyed on `agent_status=error_max_turns` (pipeline.py's wrapper) but missed runner.py:515's `Agent session error (subtype='error_max_turns')`. A real max-turns failure fell through to UNKNOWN → 'Unexpected error' (live-caught ABCA-483: a task hit the 100-turn cap but the Linear reply read 'Unexpected error'). The max_turns / max_budget / error_during_execution patterns now match BOTH the `agent_status=` and `subtype=` wrappers. Platform-agnostic, MAIN-BOUND (cherry-pick clean: single file + colocated test, no Linear/orchestration coupling). --- cdk/src/handlers/shared/error-classifier.ts | 18 ++++++++++++------ .../handlers/shared/error-classifier.test.ts | 17 +++++++++++++++++ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/cdk/src/handlers/shared/error-classifier.ts b/cdk/src/handlers/shared/error-classifier.ts index 1aecaee3..f48da467 100644 --- a/cdk/src/handlers/shared/error-classifier.ts +++ b/cdk/src/handlers/shared/error-classifier.ts @@ -210,11 +210,17 @@ const PATTERNS: readonly ErrorPattern[] = [ // ``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, + pattern: /(?:agent_status|subtype)=['"]?error_max_turns['"]?/i, classification: { category: ErrorCategory.TIMEOUT, title: 'Exceeded max turns', @@ -224,7 +230,7 @@ const PATTERNS: readonly ErrorPattern[] = [ }, }, { - 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', @@ -234,7 +240,7 @@ const PATTERNS: readonly ErrorPattern[] = [ }, }, { - pattern: /agent_status=['"]?error_during_execution['"]?/i, + pattern: /(?:agent_status|subtype)=['"]?error_during_execution['"]?/i, classification: { category: ErrorCategory.AGENT, title: 'Agent errored during execution', diff --git a/cdk/test/handlers/shared/error-classifier.test.ts b/cdk/test/handlers/shared/error-classifier.test.ts index 5410fb77..dd7c4a31 100644 --- a/cdk/test/handlers/shared/error-classifier.test.ts +++ b/cdk/test/handlers/shared/error-classifier.test.ts @@ -256,6 +256,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 From cb9246632eb6a5289749d26b2f83a6cb12406e3d Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Mon, 13 Jul 2026 20:43:45 -0400 Subject: [PATCH 2/8] feat(errors): 3-way transient/service/user classification + auto-retry-once on transient session-start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Error handling couldn't distinguish a transient hiccup from a real service error — the retryable boolean conflated 'self-heals on retry' with 'you must change something first', so a customer couldn't tell whether to retry or call their admin. - error-classifier.ts: new ErrorClass axis (transient / service / user) on every classification. transient = infra/service hiccup a retry clears (ECS deploy-race, ENI/capacity delay, network blip, Bedrock throttle, concurrency cap); service = real platform/config fault an admin owns (bad token/scopes, model-not-enabled, blueprint misconfig); user = the request/code is the thing to change (build/test failed, guardrail, wrong PR, max-turns). retryGuidance() keys on errorClass and takes an autoRetried flag. New isTransientError() helper. - orchestrate-task.ts: AUTO-RETRY-ONCE at session-start for transient failures — the one place a retry is idempotent by construction (no clone/commits/PR yet). Emits session_start_retry; stamps '[auto-retried]' so the surface says 'I tried again, still failed'. Mid-run crashes are NOT retried. Backport note: the failure-reply.ts consumer hunk from the origin commit is omitted — that module is part of the not-yet-merged #247 orchestration UX and does not exist on main. The classifier axis + auto-retry stand alone. (cherry picked from commit 63a12dc891d0c9a07f46604f904ba1f27a19b7a9) --- cdk/src/handlers/orchestrate-task.ts | 40 ++++- cdk/src/handlers/shared/error-classifier.ts | 138 ++++++++++++++++++ .../handlers/shared/error-classifier.test.ts | 58 +++++++- 3 files changed, 232 insertions(+), 4 deletions(-) diff --git a/cdk/src/handlers/orchestrate-task.ts b/cdk/src/handlers/orchestrate-task.ts index bfba0754..18502384 100644 --- a/cdk/src/handlers/orchestrate-task.ts +++ b/cdk/src/handlers/orchestrate-task.ts @@ -20,6 +20,7 @@ import { withDurableExecution, type DurableExecutionHandler } from '@aws/durable-execution-sdk-js'; import { TaskStatus, TERMINAL_STATUSES } from '../constructs/task-status'; import { resolveComputeStrategy } from './shared/compute-strategy'; +import { classifyError, isTransientError } from './shared/error-classifier'; import { reportIssueFailure as reportJiraIssueFailure } from './shared/jira-feedback'; import { reportIssueFailure } from './shared/linear-feedback'; import { logger } from './shared/logger'; @@ -159,14 +160,42 @@ 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). 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 (ECS deploy-race "TaskDefinition is inactive", + // ENI/capacity delay, a Bedrock/agentcore throttle) usually clears on a second + // attempt — so we swallow the first transient failure and try once more before + // surfacing anything to the user. A NON-transient failure (bad config, missing + // ECS substrate) throws immediately — retrying it just wastes ~a minute. Mid-run + // crashes are NOT retried here (step 5); the agent may have pushed commits. + let handle; + try { + handle = await strategy.startSession(startInput); + } catch (firstErr) { + const classification = classifyError(`Session start failed: ${String(firstErr)}`); + if (!isTransientError(classification)) { + throw firstErr; // service/user error — a retry won't help; surface now. + } + autoRetried = true; + logger.warn('Session start hit a transient error — auto-retrying once', { + task_id: taskId, + error: firstErr instanceof Error ? firstErr.message : String(firstErr), + }); + await emitTaskEvent(taskId, 'session_start_retry', { + reason: classification?.title ?? 'transient', + }); + handle = await strategy.startSession(startInput); + } // Build compute metadata for the task record so cancel-task can stop the right backend const computeMetadata: Record = handle.strategyType === 'ecs' @@ -192,7 +221,12 @@ 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 so the channel surface can say + // "I already tried again" (a bare marker the classifier ignores but + // renderFailureReply detects — see AUTO_RETRIED_MARKER). Only stamped when the + // single transient retry above also failed. + const retriedNote = autoRetried ? ' [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 f48da467..e24a2111 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,7 @@ 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, }, }, @@ -204,6 +265,7 @@ 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 @@ -227,6 +289,7 @@ const PATTERNS: readonly ErrorPattern[] = [ 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.', retryable: true, + errorClass: ErrorClass.USER, }, }, { @@ -237,6 +300,7 @@ const PATTERNS: readonly ErrorPattern[] = [ 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, }, }, { @@ -247,6 +311,7 @@ const PATTERNS: readonly ErrorPattern[] = [ 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, }, }, { @@ -257,6 +322,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, }, }, { @@ -267,6 +333,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, }, }, @@ -279,6 +346,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, }, }, { @@ -289,6 +357,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, }, }, @@ -303,6 +372,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, }, }, { @@ -313,6 +383,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, }, }, { @@ -324,6 +395,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, }, }, @@ -336,6 +408,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, }, }, ]; @@ -467,6 +540,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, }; /** @@ -498,3 +575,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/test/handlers/shared/error-classifier.test.ts b/cdk/test/handlers/shared/error-classifier.test.ts index dd7c4a31..d1dd346c 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', () => { @@ -503,10 +503,66 @@ 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); + }); + + test('isTransientError is false for null / absent classification', () => { + expect(isTransientError(null)).toBe(false); + expect(isTransientError(undefined)).toBe(false); + }); + }); + // --- Priority / ordering --- describe('pattern priority', () => { From 77042c8d3226dea192c46b3d5796016b3e0875d1 Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Wed, 8 Jul 2026 20:57:30 -0400 Subject: [PATCH 3/8] fix(errors): classify the claude Exec-format/broken-shim failure with retry guidance (ABCA-659) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the agent image's claude CLI can't be exec'd (OSError Exec format error, or the shim's own 'claude native binary not installed'), the classifier had no matching pattern and fell through to the bare 'Unexpected error' with a generic 'report it' remedy — the exact no-guidance anti-pattern the error-feedback arc (K5 buckets, remedy+retryable+errorClass) was built to eliminate. Live-caught on ABCA-659's retry: the ❌ Linear comment just said 'Unexpected error', telling the user nothing about whether to retry or escalate. Add a COMPUTE + transient pattern so the failure reads honestly and actionably: 'Couldn't start the coding agent (environment issue) … not a problem with your request … reply here to try again; if every attempt fails the same way, the agent image needs a rebuild — contact your ABCA admin with the task id.' The transient class also lets the platform's session-start auto-retry take a shot. Matched before the AGENT/UNKNOWN fallthrough; two tests pin the copy + axis. --- cdk/src/handlers/shared/error-classifier.ts | 24 +++++++++++++++++++ .../handlers/shared/error-classifier.test.ts | 21 ++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/cdk/src/handlers/shared/error-classifier.ts b/cdk/src/handlers/shared/error-classifier.ts index e24a2111..bf015739 100644 --- a/cdk/src/handlers/shared/error-classifier.ts +++ b/cdk/src/handlers/shared/error-classifier.ts @@ -255,6 +255,30 @@ const PATTERNS: readonly ErrorPattern[] = [ 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, + }, + }, // --- Agent --- { diff --git a/cdk/test/handlers/shared/error-classifier.test.ts b/cdk/test/handlers/shared/error-classifier.test.ts index d1dd346c..edbbd0d6 100644 --- a/cdk/test/handlers/shared/error-classifier.test.ts +++ b/cdk/test/handlers/shared/error-classifier.test.ts @@ -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', From 99f9d1111d20a9422ad93a5806e7ffb921184109 Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Tue, 14 Jul 2026 15:53:58 -0400 Subject: [PATCH 4/8] feat(agent): stuck/runaway guard + surface the failure behind a max_turns cap (ABCA-662) (#600) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(agent): stuck/runaway guard — break a repeating-failing-command loop Live-caught ABCA-483: a one-line README task burned ALL 100 turns (~22 min, $1.53) because the agent re-ran the SAME failing command (mise //cdk:test → JS-heap OOM, exit 134) over and over, yak-shaving the build env instead of finishing. Nothing noticed until the hard max_turns cap killed it. New `stuck_guard.py` (pure, dep-free): PostToolUse records each tool result; a coarse (tool_name, command) signature tracks consecutive failures. A new between-turns hook (registered after cancel, before nudge — same short-circuit rationale) then: - STEER once at 3 repeats: inject 'stop retrying X — work around it or finish with what you have'; - BAIL at 6 repeats: end the turn loop early (continue_=False) so the task fails fast instead of grinding to the cap. Conservative by design: keys on a REPEATING FAILURE (not a raw turn count), so a large task making varied progress never trips it; unknown tool output counts as success. Fail-open everywhere — a guard bug can't wedge the agent. Platform-agnostic, MAIN-BOUND: lives entirely in agent/src (stuck_guard.py + hooks.py); surfaces via the channel-neutral error_message. The honest reason is then rendered per-channel by the classifier / failure-reply (several fixes). 1158 agent tests pass (ruff + ty clean). * fix: stuck-guard is advisory-only — drop the bail, keep a one-time steer Reviewing the guard for false-positive risk (user: 'make sure the turn limit isn't too aggressive') showed the bail path was unsafe: distinguishing a true spin from a legitimately-iterating agent (re-running the same test command as it fixes failures one by one) from raw output is genuinely fragile — a digit- normalizing fingerprint that ignores volatile GC timings ALSO collapses 'test file_0' vs 'file_1' (the progress signal), so it would kill a working agent. A false-positive KILL is far worse than a false-positive nudge. So: removed BAIL entirely. The guard now only ever injects a ONE-TIME advisory steer when the same command fails with IDENTICAL output N times; the max_turns cap (with a fix's honest 'Exceeded max turns' reason) is the real runaway backstop. A false positive costs exactly one extra advisory comment. Platform-agnostic, main-bound. * fix(agent): explain WHY a task hit max_turns + catch loop-of-variations early (ABCA-662) ABCA-662 capped at max_turns, but the CAUSE was masked: it had finished the code and then thrashed ~15 turns retrying a failing 'git push' (remote: invalid credentials) every which way — a DIFFERENT command each turn, so the existing per-signature stuck-guard (same command × identical output ≥3) never tripped, and the terminal reason read only 'Exceeded max turns'. max_turns is a CORRECT classification, but the user can't tell 'genuinely needed more turns' from 'spun on an error' — and raising the cap just burns more tokens. Two changes, both advisory (no hard kill — keeps the K10 no-false-kill stance): - stuck_guard: add a signature-agnostic TRAILING WINDOW (last 6 tool outcomes). When ≥5 share the SAME byte-identical failure fingerprint across DIFFERENT commands, steer once ('you're spinning on one failing op — stop, it won't resolve by retrying'). EXACT fingerprint (no digit-blur) so a healthy iterate-and-fix loop (same cmd, a different test failing each run) never trips it — the K10 case, still covered by its test. Also exposes recent_failure_ summary(). - pipeline/hooks: the between-turns hook latches that summary; the terminal path appends it to a max_turns reason → 'error_max_turns … — spinning on failing tool calls (last: git push → invalid credentials)'. Only enriches max_turns; a productive run adds nothing. - error-classifier: a new bucket for 'error_max_turns … spinning on failing tool calls' → 'Ran out of turns while stuck on a failing step' with the honest remedy (raising --max-turns won't help; fix the failing op / check the env), ordered before the generic max_turns bucket. Tests: window-spin + K10 no-trip + summary + pipeline enrichment + classifier bucket. * fix(errors): don't over-claim on a max_turns spin — offer BOTH remedies (ABCA-662 review) The prior 'spinning on failing tool calls' classifier bucket asserted 'raising --max-turns won't help'. That over-claims: the trailing window can't distinguish (a) a HARD blocker no turn count fixes from (b) a LONG task that hit a transient/recoverable snag near the end and just ran out — which is 662 itself (siblings 661/663 pushed fine with the same token → its 'invalid credentials' was a transient race that more turns / a retry WOULD have cleared). Reframe the bucket to SURFACE what it was stuck on and present BOTH paths (environment blocker → fix + retry; transient/recoverable or a shorter sibling got past it → just retry / raise --max-turns), letting the failure KIND in the detail tell the reader which applies. Title 'Ran out of turns retrying a failing step'. The early window-steer (agent told to STOP and report while it still has turns) is the real mechanism that lets a long task escape the thrash; this copy is just the honest post-hoc explanation. Test updated to assert both remedies, not the over-claim. * fix(errors): max_turns stays "Exceeded max turns" — observe the repeated failure, don't diagnose it Reviewer concern on the ABCA-662 work: re-titling a max_turns failure as "Ran out of turns retrying a failing step" over-claims. The stuck-guard's trailing window is only the last ~6 tool calls, which genuinely CANNOT distinguish a hard blocker (bad creds, no permission — more turns won't help) from a long task that made real progress and hit a recoverable snag only at the tail (more turns / a retry WOULD help — 662 itself: siblings pushed fine with the same token). Framing the whole run around its last 6 calls misrepresents the latter. - error-classifier: drop the separate "retrying a failing step" bucket. A max_turns failure stays the plain "Exceeded max turns"; the description/remedy point the reader at the observed detail and still surface the environment-blocker path, but assert NO cause. - stuck_guard.recent_failure_summary: emit a NEUTRAL observation ("last tool calls repeated: `` → ") instead of "spinning on failing tool calls". The mid-run steer TO the agent (an advisory nudge, not a user surface) still says "spinning" — that's fine, it's coaching the agent, not classifying the outcome. - tests updated to pin the neutral wording + assert the title is NOT re-framed. --- agent/src/hooks.py | 98 ++++- agent/src/pipeline.py | 20 +- agent/src/stuck_guard.py | 361 ++++++++++++++++++ agent/tests/test_hooks.py | 78 ++++ agent/tests/test_nudge_hook.py | 16 +- agent/tests/test_stuck_guard.py | 224 +++++++++++ cdk/src/handlers/shared/error-classifier.ts | 13 +- .../handlers/shared/error-classifier.test.ts | 22 ++ 8 files changed, 824 insertions(+), 8 deletions(-) create mode 100644 agent/src/stuck_guard.py create mode 100644 agent/tests/test_stuck_guard.py diff --git a/agent/src/hooks.py b/agent/src/hooks.py index 6ecf7299..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 a6bb7857..082c21f6 100644 --- a/agent/src/pipeline.py +++ b/agent/src/pipeline.py @@ -466,6 +466,21 @@ def _resolve_overall_task_status( agent_status = agent_result.status err = agent_result.error + # ABCA-662: a max_turns cap is a CORRECT classification, but on its own it + # doesn't say WHETHER the task genuinely needed the turns or SPUN on a failing + # operation until it ran out (662 thrashed on a failing `git push` → invalid + # credentials, retried every which way, and capped). When the stuck-guard's + # trailing window was failure-dominated, append its one-line summary so the + # reason distinguishes "ran long" from "looped on an error" — the classifier + # still buckets it as max_turns, but a human sees the real cause. Only enriches + # the max_turns reason; a task that used its turns productively adds nothing. + if err and "error_max_turns" in err: + from hooks import last_stuck_summary + + stuck = last_stuck_summary() + if stuck and stuck not in err: + err = f"{err} — {stuck}" + if agent_status in ("success", "end_turn") and build_ok: return "success", err @@ -696,9 +711,12 @@ def run_task( # in principle dispatch a second run_task in the same process — reset # here so a stale BLOCKED[...] reason can never leak into this task's # terminal error_message (the latch is a scalar, not task_id-keyed). - from hooks import reset_blocker_reason + from hooks import reset_blocker_reason, reset_stuck_summary reset_blocker_reason() + # ABCA-662: same per-task reset for the stuck-guard 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..13eb1b34 100644 --- a/agent/tests/test_hooks.py +++ b/agent/tests/test_hooks.py @@ -1697,3 +1697,81 @@ 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_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_stuck_guard.py b/agent/tests/test_stuck_guard.py new file mode 100644 index 00000000..f13d2d15 --- /dev/null +++ b/agent/tests/test_stuck_guard.py @@ -0,0 +1,224 @@ +"""Tests for the stuck/runaway guard (K7, live-caught ABCA-483).""" + +from __future__ import annotations + +from stuck_guard import ( + STEER_THRESHOLD, + StuckGuard, + _looks_failed, + _signature, +) + +OOM = "[//cdk:test] FAILED (exit 134)\n<--- Last few GCs --->\nJavaScript heap out of memory" +OK = "Tests passed. 2813 passed." +CMD = {"command": "MISE_EXPERIMENTAL=1 mise //cdk:test"} + + +class TestFailureDetection: + def test_oom_exit_134_is_failure(self): + assert _looks_failed(OOM) is True + + def test_command_not_found_is_failure(self): + assert _looks_failed("bash: line 1: yarn: command not found") is True + + def test_clean_output_is_not_failure(self): + assert _looks_failed(OK) is False + + def test_exit_zero_is_not_failure(self): + assert _looks_failed("done (exit 0)") is False + + def test_unrecognized_output_is_not_failure(self): + # Conservative: unknown response must not be punished as a failure. + assert _looks_failed("here is the file content you asked for") is False + + def test_empty_is_not_failure(self): + assert _looks_failed("") is False + + +class TestSignature: + def test_bash_keys_on_command_whitespace_collapsed(self): + a = _signature("Bash", {"command": "mise //cdk:test"}) + b = _signature("Bash", {"command": "mise //cdk:test"}) + assert a == b + + def test_different_commands_differ(self): + a = _signature("Bash", {"command": "yarn test"}) + b = _signature("Bash", {"command": "yarn build"}) + assert a != b + + def test_edit_keys_on_file_path(self): + a = _signature("Edit", {"file_path": "src/x.ts"}) + b = _signature("Edit", {"file_path": "src/x.ts"}) + assert a == b + + +class TestStuckGuardLifecycle: + def test_no_action_below_steer_threshold(self): + g = StuckGuard() + for _ in range(STEER_THRESHOLD - 1): + g.record_tool_result("Bash", CMD, OOM) + assert g.evaluate().kind == "none" + + def test_steers_at_threshold(self): + g = StuckGuard() + for _ in range(STEER_THRESHOLD): + g.record_tool_result("Bash", CMD, OOM) + action = g.evaluate() + assert action.kind == "steer" + assert "STOP retrying" in action.message + # the offending command is previewed + assert "mise //cdk:test" in action.message.lower() + + def test_steers_at_most_once_per_signature(self): + g = StuckGuard() + for _ in range(STEER_THRESHOLD): + g.record_tool_result("Bash", CMD, OOM) + assert g.evaluate().kind == "steer" + # same signature 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 diff --git a/cdk/src/handlers/shared/error-classifier.ts b/cdk/src/handlers/shared/error-classifier.ts index bf015739..41d538e8 100644 --- a/cdk/src/handlers/shared/error-classifier.ts +++ b/cdk/src/handlers/shared/error-classifier.ts @@ -306,12 +306,21 @@ const PATTERNS: readonly ErrorPattern[] = [ // (live-caught on ABCA-483: a task hit the 100-turn cap but the reply // said "Unexpected error"). Match either ``agent_status=``/``subtype=``. { + // A max-turns cap is a correct, self-explanatory classification. When the + // stuck-guard observed the last several tool calls repeating the SAME failure + // it is appended to the reason as a neutral OBSERVATION ("last tool calls + // repeated: `` → ") — we surface WHAT was on screen but deliberately + // make NO causal claim about whether more turns would have helped: the + // trailing window (last 6 calls) can't distinguish a hard blocker from a long + // task that hit a recoverable snag only at the tail, so re-framing the whole + // run as "retrying a failing step" would misrepresent the latter. The reader + // sees the observed detail and the neutral remedy and decides. pattern: /(?:agent_status|subtype)=['"]?error_max_turns['"]?/i, classification: { category: ErrorCategory.TIMEOUT, title: 'Exceeded max turns', - description: 'The agent reached the configured ``max_turns`` limit before completing.', - remedy: 'Raise ``--max-turns`` on the submit call, simplify the task, or break it into smaller sub-tasks.', + description: 'The agent reached the configured ``max_turns`` limit before completing. If a repeated tool failure was observed near the end, it is shown in the detail below.', + remedy: 'Look at the detail below to see what the agent was doing when it ran out. Raise ``--max-turns`` on the submit call, simplify the task, or break it into smaller sub-tasks — and if the detail shows an environment/tooling blocker (auth, credentials, permission, network, disk), fix that first, then reply here to retry.', retryable: true, errorClass: ErrorClass.USER, }, diff --git a/cdk/test/handlers/shared/error-classifier.test.ts b/cdk/test/handlers/shared/error-classifier.test.ts index edbbd0d6..3978c22d 100644 --- a/cdk/test/handlers/shared/error-classifier.test.ts +++ b/cdk/test/handlers/shared/error-classifier.test.ts @@ -258,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)", From e0efe30eaee28789b55999059233b66cc47e9e86 Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Tue, 14 Jul 2026 16:46:37 -0400 Subject: [PATCH 5/8] fix+test: guard session-start retry emit, extract + test it, classify raw error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the #599 review (Stack B): - B1 (MEDIUM): the `session_start_retry` telemetry emit was unguarded — a TaskEvents PutItem fault (throttle/timeout, co-occurring with the transient session-start failures this path handles) threw AFTER `autoRetried=true` but BEFORE the retry ran, so the user was told a second attempt failed when none had, and the real retriable cause was discarded. The emit is now best-effort (try/catch → WARN-and-continue) so telemetry never aborts/mis-attributes the retry. The `correlation` envelope is threaded so the retry event carries the same trace context as `session_started` (#245). - B2 (crit-9): the auto-retry had ZERO coverage (it lived inline in the durable handler's start-session step the suite never invokes). Extracted to an exported `startSessionWithRetry(strategy, input, deps)` — matching the repo's "test the exported helper" pattern — and unit-tested all four branches (success / non-transient no-retry / transient→retry→success / transient→transient→throw) plus the B1 best-effort-emit guard. - N2: classify the RAW error, not a ``Session start failed: …`` wrapper. The wrapper string itself matched a TRANSIENT pattern, so EVERY session-start failure classified transient and the "non-transient throws immediately" branch was effectively dead (a config/auth fault ate a pointless ~1-min retry). Classifying the raw string restores that branch (verified: TaskDefinition- inactive → retry; ECS_CLUSTER_ARN-not-configured / AccessDenied → surface). - N1: soften the `[auto-retried]` comment — it forward-referenced renderFailureReply/AUTO_RETRIED_MARKER that don't exist yet. Now states the marker is persisted verbatim for a forthcoming failure renderer (retryGuidance ships ahead of its consumer); no behaviour claim about rendering today. Full cdk build green (compile + jest + eslint + synth); 5 new helper tests pass. --- cdk/src/handlers/orchestrate-task.ts | 57 ++++----- .../handlers/shared/session-start-retry.ts | 119 ++++++++++++++++++ .../shared/session-start-retry.test.ts | 116 +++++++++++++++++ 3 files changed, 260 insertions(+), 32 deletions(-) create mode 100644 cdk/src/handlers/shared/session-start-retry.ts create mode 100644 cdk/test/handlers/shared/session-start-retry.test.ts diff --git a/cdk/src/handlers/orchestrate-task.ts b/cdk/src/handlers/orchestrate-task.ts index 18502384..bacecc42 100644 --- a/cdk/src/handlers/orchestrate-task.ts +++ b/cdk/src/handlers/orchestrate-task.ts @@ -20,7 +20,6 @@ import { withDurableExecution, type DurableExecutionHandler } from '@aws/durable-execution-sdk-js'; import { TaskStatus, TERMINAL_STATUSES } from '../constructs/task-status'; import { resolveComputeStrategy } from './shared/compute-strategy'; -import { classifyError, isTransientError } from './shared/error-classifier'; import { reportIssueFailure as reportJiraIssueFailure } from './shared/jira-feedback'; import { reportIssueFailure } from './shared/linear-feedback'; import { logger } from './shared/logger'; @@ -38,6 +37,7 @@ import { type PollState, } from './shared/orchestrator'; import { runPreflightChecks } from './shared/preflight'; +import { startSessionWithRetry } from './shared/session-start-retry'; import type { TaskRecord } from './shared/types'; import { workflowIsReadOnly, workflowRequiresRepo } from './shared/workflows'; @@ -169,33 +169,23 @@ const durableHandler: DurableExecutionHandler = asyn payload, blueprintConfig, }; - // Transient-error AUTO-RETRY (once). 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 (ECS deploy-race "TaskDefinition is inactive", - // ENI/capacity delay, a Bedrock/agentcore throttle) usually clears on a second - // attempt — so we swallow the first transient failure and try once more before - // surfacing anything to the user. A NON-transient failure (bad config, missing - // ECS substrate) throws immediately — retrying it just wastes ~a minute. Mid-run - // crashes are NOT retried here (step 5); the agent may have pushed commits. - let handle; - try { - handle = await strategy.startSession(startInput); - } catch (firstErr) { - const classification = classifyError(`Session start failed: ${String(firstErr)}`); - if (!isTransientError(classification)) { - throw firstErr; // service/user error — a retry won't help; surface now. - } - autoRetried = true; - logger.warn('Session start hit a transient error — auto-retrying once', { - task_id: taskId, - error: firstErr instanceof Error ? firstErr.message : String(firstErr), - }); - await emitTaskEvent(taskId, 'session_start_retry', { - reason: classification?.title ?? 'transient', - }); - handle = await strategy.startSession(startInput); - } + // 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' @@ -221,10 +211,13 @@ const durableHandler: DurableExecutionHandler = asyn return handle; } catch (err) { - // Carry the auto-retry fact into error_message so the channel surface can say - // "I already tried again" (a bare marker the classifier ignores but - // renderFailureReply detects — see AUTO_RETRIED_MARKER). Only stamped when the - // single transient retry above also failed. + // 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). Only stamped + // when the single transient retry above also failed. const retriedNote = autoRetried ? ' [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/session-start-retry.ts b/cdk/src/handlers/shared/session-start-retry.ts new file mode 100644 index 00000000..51045a2f --- /dev/null +++ b/cdk/src/handlers/shared/session-start-retry.ts @@ -0,0 +1,119 @@ +/** + * 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; +} + +/** + * 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 + * (with ``autoRetried`` observable via the thrown-from state; the caller + * stamps its own ``[auto-retried]`` marker — it owns ``autoRetried`` because + * this function throws rather than returns on the double-failure). + * + * 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), + }); + } + const handle = await strategy.startSession(input); + return { handle, autoRetried: true }; + } +} 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..ce8cc4f9 --- /dev/null +++ b/cdk/test/handlers/shared/session-start-retry.test.ts @@ -0,0 +1,116 @@ +/** + * 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 { 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 (second failure surfaces)', async () => { + const secondErr = new Error('TaskDefinition is inactive (again)'); + const startSession = jest + .fn() + .mockRejectedValueOnce(TRANSIENT) + .mockRejectedValueOnce(secondErr); + const { d } = deps(); + await expect(startSessionWithRetry({ startSession }, {} as never, d)).rejects.toBe(secondErr); + expect(startSession).toHaveBeenCalledTimes(2); + }); +}); + +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); + }); +}); From 6b7cd3f07a55f30477217b13a85e3cd516b46e83 Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Tue, 14 Jul 2026 16:54:33 -0400 Subject: [PATCH 6/8] test: cover the max_turns-enrichment wiring seam + window-steer boundary (#600 N3/N4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up coverage from the #600 review (feature approved as-is; tests only): - N3 (crit-8 wiring seam): the max_turns reason-enrichment was tested only at its two pure endpoints (stuck_guard.recent_failure_summary + the classifier on a hand-built string) — the append in _resolve_overall_task_status that joins them was unverified. Add TestMaxTurnsStuckEnrichment (monkeypatching hooks.last_stuck_summary): appends on error_max_turns, does NOT append on a non-max_turns error, leaves the reason unchanged when there's no summary, and does not double-append when the summary is already present. - N4 (crit-6 boundary): the window-steer was tested at 6/6 (steers) and 4/6 (doesn't) but not at the exact WINDOW_FAIL_THRESHOLD `>=` edge. Add test_window_steers_at_exactly_the_threshold (5/6 same-fingerprint fails in a full window → steers) and test_no_steer_when_window_not_yet_full (5 fails in a length-5 history → no steer, since _dominant_window_failure requires a full window). Imports WINDOW/WINDOW_FAIL_THRESHOLD so the constants' boundary semantics are now pinned. Full agent suite green (1218); ruff format + check clean. --- agent/tests/test_pipeline_outcomes.py | 58 +++++++++++++++++++++++++++ agent/tests/test_stuck_guard.py | 26 ++++++++++++ 2 files changed, 84 insertions(+) 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 index f13d2d15..942246e2 100644 --- a/agent/tests/test_stuck_guard.py +++ b/agent/tests/test_stuck_guard.py @@ -4,6 +4,8 @@ from stuck_guard import ( STEER_THRESHOLD, + WINDOW, + WINDOW_FAIL_THRESHOLD, StuckGuard, _looks_failed, _signature, @@ -222,3 +224,27 @@ def test_healthy_iteration_below_window_threshold_no_steer(self): 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 From 4fea6c5335f5f6329d07fc0fcd65d5863a5144cf Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Tue, 14 Jul 2026 17:30:22 -0400 Subject: [PATCH 7/8] =?UTF-8?q?fix(cli):=20mirror=20errorClass=20onto=20Er?= =?UTF-8?q?rorClassification=20(CDK=E2=86=94CLI=20type=20sync)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #599 added `errorClass?: ErrorClassType` to the CDK ErrorClassification, which serializes into the GET /tasks/{id} `error_classification` response — but the hand-maintained CLI mirror (cli/src/types.ts) wasn't updated, so the CLI's structured error display couldn't surface the transient/service/user axis (real drift per the CLAUDE.md shared-API-shape routing rule). Add the optional field to the CLI's ErrorClassification, inlined as a union literal rather than a named `ErrorClassType` export — the types-sync drift check (scripts/check-types-sync.ts) requires CDK to be the source of truth for any exported type name, and CDK exports ErrorClassType from error-classifier.ts, not shared/types.ts. Inlining keeps the field synced without a CLI-only export. Verified: `//:check:types-sync` OK (57 CLI exports validated), cli build green (600 tests, compile + eslint clean). --- cli/src/types.ts | 5 +++++ 1 file changed, 5 insertions(+) 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}. */ From 28371306a3fcbf5a4551cadfb0ade9913d7319b7 Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Wed, 15 Jul 2026 18:57:02 -0400 Subject: [PATCH 8/8] =?UTF-8?q?fix(errors):=20address=20#599=20nits=20N1-N?= =?UTF-8?q?3=20=E2=80=94=20[auto-retried]=20breadcrumb=20+=20test=20seams?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit N1 (Medium): the [auto-retried] marker was never stamped on branch 4 (transient- then-transient), because startSessionWithRetry THROWS there and the caller's autoRetried flag is only set on the success paths. A double-transient failure was told 'reply to retry' instead of 'I already retried' — the exact confusion the marker exists to prevent. Fix: tag the thrown error with a Symbol (AUTO_RETRIED) + export isAutoRetried(); orchestrate-task reads it in the catch and stamps the marker on both paths a retry ran. Corrected the docstring (was claiming the fact was 'observable via the thrown-from state' — it wasn't). N2 (Low): pin the two untested wiring seams. CDK: branch-4 test now asserts isAutoRetried(err) is true (and false for a first-attempt/non-object failure). Agent: new tests drive _stuck_guard_between_turns_hook end-to-end and assert the _LAST_STUCK_SUMMARY latch is written (and cleared on a healthy window) — the production path the enrichment test's monkeypatch bypasses. Deleting hooks.py's latch write now fails a test. N3 (Low): pin retryGuidance's two USER fall-through branches (retryable-user non-guardrail, not-retryable-user) so the #247 failure-renderer copy contract can't rot. Full cdk (2345) + agent (1251) gates green. --- agent/tests/test_hooks.py | 50 +++++++++++++++++++ cdk/src/handlers/orchestrate-task.ts | 14 ++++-- .../handlers/shared/session-start-retry.ts | 39 +++++++++++++-- .../handlers/shared/error-classifier.test.ts | 33 ++++++++++++ .../shared/session-start-retry.test.ts | 18 ++++++- 5 files changed, 143 insertions(+), 11 deletions(-) diff --git a/agent/tests/test_hooks.py b/agent/tests/test_hooks.py index 13eb1b34..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 @@ -1721,6 +1724,53 @@ def test_post_tool_use_records_failures_into_the_guard(self): # 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 diff --git a/cdk/src/handlers/orchestrate-task.ts b/cdk/src/handlers/orchestrate-task.ts index e5786ebc..d1914282 100644 --- a/cdk/src/handlers/orchestrate-task.ts +++ b/cdk/src/handlers/orchestrate-task.ts @@ -37,7 +37,7 @@ import { type PollState, } from './shared/orchestrator'; import { runPreflightChecks } from './shared/preflight'; -import { startSessionWithRetry } from './shared/session-start-retry'; +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'; @@ -217,9 +217,15 @@ const durableHandler: DurableExecutionHandler = asyn // 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). Only stamped - // when the single transient retry above also failed. - const retriedNote = autoRetried ? ' [auto-retried]' : ''; + // 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/session-start-retry.ts b/cdk/src/handlers/shared/session-start-retry.ts index 51045a2f..2f7156e3 100644 --- a/cdk/src/handlers/shared/session-start-retry.ts +++ b/cdk/src/handlers/shared/session-start-retry.ts @@ -54,6 +54,20 @@ export interface StartSessionWithRetryResult { 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. * @@ -62,9 +76,13 @@ export interface StartSessionWithRetryResult { * 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 - * (with ``autoRetried`` observable via the thrown-from state; the caller - * stamps its own ``[auto-retried]`` marker — it owns ``autoRetried`` because - * this function throws rather than returns on the double-failure). + * 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 @@ -113,7 +131,18 @@ export async function startSessionWithRetry( error: emitErr instanceof Error ? emitErr.message : String(emitErr), }); } - const handle = await strategy.startSession(input); - return { handle, autoRetried: true }; + 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 3978c22d..8e6a8b48 100644 --- a/cdk/test/handlers/shared/error-classifier.test.ts +++ b/cdk/test/handlers/shared/error-classifier.test.ts @@ -600,6 +600,39 @@ describe('classifyError', () => { 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); diff --git a/cdk/test/handlers/shared/session-start-retry.test.ts b/cdk/test/handlers/shared/session-start-retry.test.ts index ce8cc4f9..f6227278 100644 --- a/cdk/test/handlers/shared/session-start-retry.test.ts +++ b/cdk/test/handlers/shared/session-start-retry.test.ts @@ -18,7 +18,7 @@ */ import type { SessionHandle } from '../../../src/handlers/shared/compute-strategy'; -import { startSessionWithRetry } from '../../../src/handlers/shared/session-start-retry'; +import { isAutoRetried, startSessionWithRetry } from '../../../src/handlers/shared/session-start-retry'; const HANDLE: SessionHandle = { sessionId: 'sess-1', @@ -79,15 +79,29 @@ describe('startSessionWithRetry — the 4 branches (#599 B2)', () => { expect(emitReasons).toHaveLength(1); // the session_start_retry event fired once }); - it('4. transient then transient → throws the retry error (second failure surfaces)', async () => { + 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); }); });