Merge branch 'pr/ecs-test-hygiene' into fix/615-ecs-scoped-session-hang

isadeks · web-flow · commit 6b73969ca4f2 · 2026-07-22T16:56:43.000+01:00
diff --git a/agent/tests/test_shell.py b/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)
diff --git a/cdk/src/constructs/ecs-agent-cluster.ts b/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
diff --git a/cdk/src/handlers/shared/error-classifier.ts b/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,
     },
   },
   {
diff --git a/cli/src/commands/linear.ts b/cli/src/commands/linear.ts
@@ -43,6 +43,8 @@ import {
   generatePkce,
   LINEAR_OAUTH_SECRET_PREFIX,
   linearOauthSecretName,
+  readExistingWebhookSecret,
+  resolveWebhookSecretAction,
   StoredLinearOauthToken,
 } from '../linear-oauth';
 import { awaitOauthCallback, CALLBACK_URL } from '../oauth-callback-server';
@@ -601,6 +603,42 @@ export function makeLinearCommand(): Command {
         process.stdout.write('  → Storing OAuth token...');
         const sm = new SecretsManagerClient({ region });
         const now = new Date().toISOString();
+        // Preserve any EXISTING per-workspace webhook signing secret before the
+        // OAuth overwrite below. Re-running `setup` on an already-installed
+        // workspace must NOT clobber its working signing secret with the
+        // stack-wide fallback (which belongs to whichever workspace installed
+        // first) — that silently breaks signature verification (401 "Invalid
+        // signature") for every workspace after the first in a multi-workspace
+        // deployment. Rotation stays the job of `update-webhook-secret`.
+        // Fail CLOSED (#612 review B1): read this workspace's existing signing
+        // secret before the overwrite. Only ResourceNotFoundException is a clean
+        // first-install; any other SM error (AccessDenied, KMSAccessDenied,
+        // Throttling, network) — or a corrupt bundle — must surface, NOT default
+        // to undefined (which would mirror the stack-wide secret over a working
+        // per-workspace one → the #611 401 clobber, silently, behind a green
+        // "Setup complete"). Extracted to readExistingWebhookSecret + unit-tested.
+        let existingWebhookSecret: string | undefined;
+        try {
+          existingWebhookSecret = await readExistingWebhookSecret(
+            async () => {
+              const prior = await sm.send(new GetSecretValueCommand({ SecretId: linearOauthSecretName(slug) }));
+              return prior.SecretString;
+            },
+            (err) => (err as { name?: string }).name === 'ResourceNotFoundException',
+          );
+        } catch (err) {
+          const errorName = (err as { name?: string }).name;
+          const message = err instanceof Error ? err.message : String(err);
+          throw new CliError(
+            `Failed to read the existing Linear webhook secret for '${slug}' before re-write: `
+            + `${errorName ?? 'Error'}: ${message}. Refusing to proceed — continuing could clobber `
+            + 'this workspace\'s webhook signing secret with the stack-wide value (a 401 on every '
+            + 'delivery). Likely an IAM/KMS gap: confirm your CLI principal has '
+            + '`secretsmanager:GetSecretValue` (and kms:Decrypt if a CMK) on '
+            + `${linearOauthSecretName(slug)}, or run \`bgagent linear update-webhook-secret ${slug}\` `
+            + 'after fixing access.',
+          );
+        }
         const stored: StoredLinearOauthToken = {
           access_token: tokenResponse.access_token,
           refresh_token: tokenResponse.refresh_token ?? '',
@@ -615,6 +653,14 @@ export function makeLinearCommand(): Command {
           installed_at: now,
           updated_at: now,
           installed_by_platform_user_id: cognitoSub,
+          // Fold the preserved secret into the INITIAL bundle (#612 review N2):
+          // the OAuth-secret write below then lands the correct bundle in ONE
+          // PutSecretValue, and the preserve path skips the later re-write. This
+          // also closes a narrow window — if any fallible step between the two
+          // writes threw, the bundle was left persisted WITHOUT the secret (401
+          // until update-webhook-secret). Only set for a real `lin_wh_` value;
+          // mirror-stackwide / prompt cases add it in the mirror-back below.
+          ...(existingWebhookSecret ? { webhook_signing_secret: existingWebhookSecret } : {}),
         };
         if (!stored.refresh_token) {
           throw new CliError(
@@ -676,19 +722,40 @@ export function makeLinearCommand(): Command {
         // without re-onboarding. Multi-workspace installs need each
         // workspace to own its own per-workspace signing secret — only
         // the FIRST install can populate the stack-wide one usefully.
-        // If stack-wide is already populated, this is either a re-run
-        // of setup on the SAME workspace or the FIRST workspace of a
-        // future multi-workspace install. Either way the stored value
-        // is this workspace's signing secret — lift it into the
-        // per-workspace bundle without prompting (auto-migration to
-        // the new shape). Rotation is not setup's job: use
-        // `bgagent linear update-webhook-secret <slug>` to rotate the
-        // signing secret without re-running OAuth.
+        //
+        // resolveWebhookSecretAction picks one of three outcomes from
+        // (existingWebhookSecret, stackWideAlreadyConfigured):
+        //   • preserve      — this workspace ALREADY has its own `lin_wh_`
+        //                     secret (a re-run). Keep it; do NOT overwrite from
+        //                     the stack-wide fallback, which is a DIFFERENT
+        //                     workspace's secret once >1 is installed. This is
+        //                     the #611 clobber fix — the stack-wide value is NOT
+        //                     necessarily this workspace's.
+        //   • mirror-stackwide — no per-workspace secret yet but stack-wide is
+        //                     set: mirror it (correct for the first/only
+        //                     workspace; warn for an additional one).
+        //   • prompt        — nothing to go on: ask for the signing secret.
+        // Rotation is not setup's job: use
+        // `bgagent linear update-webhook-secret <slug>` to rotate.
         const stackWideAlreadyConfigured = await isWebhookSecretConfigured(sm, webhookSecretArn!);
         let webhookSigningSecret: string | undefined;
 
-        if (stackWideAlreadyConfigured) {
-          console.log('  ✓ Webhook signing secret already configured stack-wide (mirroring to per-workspace)');
+        const secretAction = resolveWebhookSecretAction(existingWebhookSecret, stackWideAlreadyConfigured);
+        if (secretAction.kind === 'preserve') {
+          // This workspace already had its own signing secret — keep it. Do NOT
+          // overwrite from the stack-wide fallback (a DIFFERENT workspace's
+          // secret once >1 is installed). Re-run case; rotation is
+          // `update-webhook-secret`'s job, not setup's.
+          console.log('  ✓ Preserving this workspace\'s existing webhook signing secret (re-run — not overwriting)');
+          webhookSigningSecret = secretAction.secret;
+        } else if (secretAction.kind === 'mirror-stackwide') {
+          // No per-workspace secret yet, but the stack-wide one is set. Safe to
+          // mirror ONLY when this is the first/only workspace — for a genuinely
+          // new ADDITIONAL workspace the stack-wide secret is the wrong one, so
+          // warn that the operator should verify (or run `update-webhook-secret`).
+          console.log('  ✓ No per-workspace secret yet; mirroring the stack-wide signing secret');
+          console.log('    (if this is an ADDITIONAL workspace, its Linear webhook secret differs —');
+          console.log(`     run \`bgagent linear update-webhook-secret ${slug}\` with this workspace's secret.)`);
           try {
             const value = await sm.send(new GetSecretValueCommand({ SecretId: webhookSecretArn! }));
             if (value.SecretString && value.SecretString.startsWith('lin_wh_')) {
@@ -725,9 +792,11 @@ export function makeLinearCommand(): Command {
           webhookSigningSecret = webhookSecret;
         }
 
-        // Mirror into the per-workspace OAuth secret so the receiver can
-        // look it up by orgId. Re-upsert with the merged payload.
-        if (webhookSigningSecret) {
+        // Mirror into the per-workspace OAuth secret so the receiver can look it
+        // up by orgId. In the PRESERVE case the secret is already in `stored`
+        // (folded above) and was written by the OAuth-secret upsert — no re-write
+        // needed. Only mirror-stackwide / prompt produce a NEW secret to persist.
+        if (webhookSigningSecret && webhookSigningSecret !== stored.webhook_signing_secret) {
           const merged: StoredLinearOauthToken = {
             ...stored,
             webhook_signing_secret: webhookSigningSecret,
diff --git a/cli/src/linear-oauth.ts b/cli/src/linear-oauth.ts
@@ -293,3 +293,88 @@ function isLinearTokenResponse(value: unknown): value is LinearTokenResponse {
 export function computeExpiresAt(expiresInSeconds: number, now: Date = new Date()): string {
   return new Date(now.getTime() + expiresInSeconds * 1000).toISOString();
 }
+
+/** What `bgagent linear setup` should do about the webhook signing secret. */
+export type WebhookSecretAction =
+  /** This workspace already has its own signing secret — keep it (re-run). */
+  | { readonly kind: 'preserve'; readonly secret: string }
+  /** No per-workspace secret; mirror the stack-wide one (safe for the first
+   *  workspace, ambiguous for an additional one — caller should warn). */
+  | { readonly kind: 'mirror-stackwide' }
+  /** No secret anywhere — prompt the operator for it (first install). */
+  | { readonly kind: 'prompt' };
+
+/**
+ * Decide which webhook signing secret `setup` should use, WITHOUT clobbering a
+ * working per-workspace secret.
+ *
+ * The bug this fixes: re-running `setup` on an already-installed workspace used
+ * to lift the *stack-wide* signing secret into the per-workspace bundle. That's
+ * correct only for the FIRST workspace — the stack-wide secret belongs to
+ * whichever workspace installed first, so for any additional workspace it
+ * overwrites the correct per-workspace secret with the wrong one, silently
+ * breaking signature verification (webhook 401 "Invalid signature").
+ *
+ * Rule: an existing valid per-workspace secret always wins (rotation is
+ * `update-webhook-secret`'s job); else mirror the stack-wide secret if present;
+ * else prompt. Pure — the caller supplies what it read from Secrets Manager.
+ *
+ * @param existingPerWorkspaceSecret the `webhook_signing_secret` on this
+ *        workspace's OAuth bundle BEFORE the setup rewrite (undefined if none).
+ * @param stackWideConfigured whether the stack-wide fallback secret is set.
+ */
+export function resolveWebhookSecretAction(
+  existingPerWorkspaceSecret: string | undefined,
+  stackWideConfigured: boolean,
+): WebhookSecretAction {
+  if (existingPerWorkspaceSecret?.startsWith('lin_wh_')) {
+    return { kind: 'preserve', secret: existingPerWorkspaceSecret };
+  }
+  if (stackWideConfigured) {
+    return { kind: 'mirror-stackwide' };
+  }
+  return { kind: 'prompt' };
+}
+
+/**
+ * Read this workspace's EXISTING `webhook_signing_secret` before `setup`
+ * overwrites the OAuth bundle — FAIL CLOSED (#612 review B1).
+ *
+ * `fetchSecretString` fetches the prior per-workspace OAuth `SecretString`
+ * (typically a `SecretsManagerClient.send(GetSecretValueCommand)` wrapper). The
+ * value fed to {@link resolveWebhookSecretAction} decides whether the existing
+ * secret is preserved or the stack-wide one is mirrored over it — so a wrong
+ * `undefined` here re-arms the #611 clobber. Therefore:
+ *
+ *   • secret missing (`ResourceNotFoundException`) → genuine first install,
+ *     return undefined (nothing to preserve).
+ *   • secret present with a valid `lin_wh_…` `webhook_signing_secret` → return it.
+ *   • secret present but no/malformed secret → return undefined (nothing valid
+ *     to preserve; the caller mirrors/prompts).
+ *   • ANY OTHER error (AccessDenied, KMSAccessDenied, Throttling, network, or a
+ *     corrupt-JSON bundle) → THROW. Swallowing it would leave undefined and
+ *     silently clobber a working secret behind a green "Setup complete".
+ *
+ * `isNotFound` classifies the fetch error; callers pass an
+ * `name === 'ResourceNotFoundException'` check. On a throw the caller re-raises
+ * a CliError with an IAM/KMS hint (mirroring `isWebhookSecretConfigured`).
+ */
+export async function readExistingWebhookSecret(
+  fetchSecretString: () => Promise<string | undefined>,
+  isNotFound: (err: unknown) => boolean,
+): Promise<string | undefined> {
+  let raw: string | undefined;
+  try {
+    raw = await fetchSecretString();
+  } catch (err) {
+    if (isNotFound(err)) return undefined; // genuine first install
+    throw err; // fail closed — caller wraps with an actionable CliError
+  }
+  if (!raw) return undefined;
+  // A corrupt-but-present bundle must surface (JSON.parse throws → propagates),
+  // NOT silently become "nothing to preserve" → mirror-stackwide.
+  const bundle = JSON.parse(raw) as Partial<StoredLinearOauthToken>;
+  return bundle.webhook_signing_secret?.startsWith('lin_wh_')
+    ? bundle.webhook_signing_secret
+    : undefined;
+}
diff --git a/cli/test/commands/linear-setup-webhook.test.ts b/cli/test/commands/linear-setup-webhook.test.ts
diff --git a/cli/test/linear-oauth.test.ts b/cli/test/linear-oauth.test.ts
