fix(agent): address #597 review — honest TIMEOUT remedy + nits (B1, N1-N4)

Scott's re-review (vs clean main) — one blocker + nits, all in the error-legibility
feature this PR ships.

- B1 (blocker): the new TIMEOUT classification pointed operators at
  BUILD_VERIFY_TIMEOUT_S — but NOTHING in agent/ reads it on this branch
  (verify_build/verify_lint call run_cmd with no timeout=, so they use the 600s
  default; the env is only SET in CDK). And the anchoring comment's 'live-caught
  on ABCA-667 via mise run build' narrative is contradicted by the code: the
  verify path CATCHES TimeoutExpired and returns a plain failure, so the regex
  never fires for that path. Took Scott's option (b): reworded the comment to
  describe the real trigger (any UNCAUGHT run_cmd TimeoutExpired — clone/setup),
  and softened the remedy to a generic 'retry / contact admin' with no dead-knob
  promise. (The actual BUILD_VERIFY_TIMEOUT_S wiring lives on the ECS-substrate
  track, not this main-bound branch — out of scope here.) Also reconciled the
  ecs-agent-cluster.ts comment to say the env is provisioned ahead of its
  consuming wiring rather than currently effective.
- N1: added explicit errorClass: ErrorClass.USER with a comment stating the
  no-auto-retry intent (re-running an unchanged slow build just times out again;
  must NOT be 'fixed' to TRANSIENT).
- N2: hardened the regex to match float seconds (\d+(?:\.\d+)?).
- N3: replaced the false-confidence allowlist test with a real one — a line that
  hits a marker (error:) AND a noise term (0 errors), placed outside the tail
  window so it's only reachable via the marker scan; deleting _FAILURE_LINE_NOISE
  now fails the test.
- N4: added a >40-marker-line test that pins the truncation-cap breadcrumb.

Gates: cdk 2439 tests + synth + eslint; agent 1278 tests + ruff + ty.

Author: isadeks

agent/tests/test_shell.py

@@ -268,6 +268,44 @@ def test_benign_zero_errors_line_not_surfaced_as_failure(self):
         blob = "\n".join(text for _, text in logs)
         assert "ok 19" in blob
 
+    def test_marker_line_with_noise_term_is_filtered_by_allowlist(self):
+        # N3: the LOAD-BEARING allowlist case. A line that hits a real marker
+        # (`error:`) AND a noise term (`0 errors`) must be filtered OUT of the
+        # surfaced set. Placed in the MIDDLE (before the tail window) so it is
+        # only reachable via the marker scan — if _FAILURE_LINE_NOISE were
+        # deleted, this line WOULD be surfaced and the assertion would fail.
+        # A genuine failure line follows so the scan still produces a match set.
+        noisy = "error: eslint produced 0 errors after autofix retry"  # marker + noise
+        real = "[//cdk:test] FAIL test/handlers/bar.test.ts — boom"
+        # 30 trailing passing lines push both lines above out of the tail window,
+        # so `noisy` is only reachable via the marker scan (where the allowlist runs).
+        stdout = f"{noisy}\n{real}\n" + "\n".join(f"[//agent:test] passing {i}" for i in range(30))
+        proc = self._completed(1, stdout=stdout, stderr="")
+        logs = self._run_capturing_logs(proc)
+        blob = "\n".join(text for _, text in logs)
+        assert "FAIL test/handlers/bar.test.ts" in blob  # real failure surfaced
+        assert "0 errors after autofix retry" not in blob  # noisy marker filtered by allowlist
+
+    def test_surfaced_failure_lines_are_capped_and_truncation_marked(self):
+        # N4: a genuinely huge red run (more than _MAX_SURFACED_FAILURE_LINES
+        # marker lines) must be capped, with an explicit truncation breadcrumb —
+        # so it can't flood CloudWatch. Exercises the cap branch the other tests
+        # never reach.
+        from shell import _MAX_SURFACED_FAILURE_LINES
+
+        n = _MAX_SURFACED_FAILURE_LINES + 10
+        stdout = "\n".join(f"FAIL test/case_{i}.test.ts — assertion {i}" for i in range(n))
+        proc = self._completed(1, stdout=stdout, stderr="")
+        logs = self._run_capturing_logs(proc)
+        blob = "\n".join(text for _, text in logs)
+        # The marker-scan hit the cap and emitted the truncation breadcrumb.
+        assert "… (more failure lines truncated)" in blob
+        # The breadcrumb appears within the surfaced set BEFORE the tail divider —
+        # i.e. the matched-line scan was capped, not the whole output dumped.
+        head = blob.split("--- (trailing output) ---")[0]
+        head_cases = sum(1 for i in range(n) if f"case_{i}.test.ts" in head)
+        assert head_cases <= _MAX_SURFACED_FAILURE_LINES
+
     def test_stdout_is_redacted(self):
         proc = self._completed(1, stdout="error: pushing with ghp_supersecrettoken123", stderr="")
         logs = self._run_capturing_logs(proc)

cdk/src/constructs/ecs-agent-cluster.ts

@@ -184,10 +184,13 @@ export class EcsAgentCluster extends Construct {
         LOG_GROUP_NAME: logGroup.logGroupName,
         GITHUB_TOKEN_SECRET_ARN: props.githubTokenSecret.secretArn,
         // Heavy CI-parity builds on this big-box substrate legitimately run
-        // longer than the 1800s default (ABCA's own `mise run build` is
-        // ~50 min cold). Raise the post-agent build-verify cap so a slow-but-
-        // healthy build isn't mis-reported as a timeout (see post_hooks.py
-        // BUILD_VERIFY_TIMEOUT_S). ECS-only: AgentCore repos keep the default.
+        // long (ABCA's own `mise run build` is ~50 min cold), so set a generous
+        // post-agent build-verify cap here for the ECS-only path. NOTE: the
+        // consuming side (verify_build/verify_lint reading BUILD_VERIFY_TIMEOUT_S
+        // and passing it as the subprocess timeout) ships with the ECS-substrate
+        // work; on `main` today the verify subprocess still uses run_cmd's
+        // default cap, so this env is provisioned ahead of the wiring rather
+        // than currently effective. ECS-only: AgentCore repos don't set it.
         BUILD_VERIFY_TIMEOUT_S: '3600',
         ...(props.memoryId && { MEMORY_ID: props.memoryId }),
         // #502: the payload bucket name so the orchestrator-issued

cdk/src/handlers/shared/error-classifier.ts

@@ -434,21 +434,29 @@ const PATTERNS: readonly ErrorPattern[] = [
 
   // --- Timeout ---
   {
-    // The build/verify command shelled out and was KILLED at the wall-clock cap
-    // (Python subprocess `TimeoutExpired … timed out after N seconds`). Live-caught
-    // on ABCA-667: the fork's full `mise run build` (~2800 tests) exceeded the
-    // 600s default and surfaced as a bare "Unexpected error". This is NOT a code
-    // failure — the build didn't fail, it ran too long — so name it precisely and
-    // point at the timeout, not the diff. On a big repo the fix is a higher
-    // BUILD_VERIFY_TIMEOUT_S (or the ECS build box), which an admin sets — but a
-    // one-off may just be slow, so it's a user-actionable "retry / raise the cap".
-    pattern: /TimeoutExpired.*timed out after \d+ ?s(econds)?|Command .*build.* timed out/i,
+    // An ``agent/`` subprocess (``run_cmd``) hit its wall-clock cap and Python
+    // raised an uncaught ``TimeoutExpired … timed out after N seconds``, which
+    // otherwise surfaces as a bare "Unexpected error". This is NOT a code
+    // failure — the command didn't fail, it ran too long — so name it precisely
+    // and point at "slow, retry", not the diff. NOTE: this matches only
+    // UNCAUGHT ``run_cmd`` timeouts (clone/setup/etc.); the post-agent build/lint
+    // VERIFY path catches ``TimeoutExpired`` itself and returns a plain build
+    // failure, so it does not reach here. The remedy is deliberately generic
+    // ("retry / it may be slow") rather than naming a specific env knob — on
+    // ``main`` the verify timeout is not operator-tunable, so promising a lever
+    // here would misdirect (the tunable ``BUILD_VERIFY_TIMEOUT_S`` + larger ECS
+    // build compute live on the ECS-substrate track, not this branch).
+    pattern: /TimeoutExpired.*timed out after \d+(?:\.\d+)? ?s(econds)?|Command .*build.* timed out/i,
     classification: {
       category: ErrorCategory.TIMEOUT,
       title: 'Build/tests didn\'t finish in time (timed out)',
       description: 'The configured build/verify command was still running when it hit the time limit and was stopped — it did not fail, it ran too long.',
-      remedy: 'This is usually a slow build, not broken code. Retry (a one-off may just be slow); if this repo\'s build is legitimately long, an admin can raise BUILD_VERIFY_TIMEOUT_S or move it to the larger ECS build compute.',
+      remedy: 'This is usually a slow build, not broken code — retry, since a one-off may just have been slow. If a repo\'s build is legitimately long, its build environment likely needs more time or capacity; contact your ABCA admin.',
       retryable: true,
+      // Intentionally USER (no auto-retry): re-running an unchanged slow build
+      // just times out again, so this must NOT be TRANSIENT — a human decides
+      // whether to retry or right-size the build. Do not "fix" this to TRANSIENT.
+      errorClass: ErrorClass.USER,
     },
   },
   {