fix(agent): build/verify failure diagnosis — timeout classify, claude-binary Exec-format, failing-line surfacing#597
Open
isadeks wants to merge 42 commits into
Open
fix(agent): build/verify failure diagnosis — timeout classify, claude-binary Exec-format, failing-line surfacing#597isadeks wants to merge 42 commits into
isadeks wants to merge 42 commits into
Conversation
The ECS compute strategy inlined the full orchestrator payload (incl. the large hydrated_context) into the AGENT_PAYLOAD container-override env var. ECS RunTask caps the TOTAL containerOverrides blob at 8192 bytes, so any real task was rejected before the container started: InvalidParameterException: Container Overrides length must be at most 8192 AgentCore is unaffected — it passes the payload in the InvokeAgentRuntime request body, which has no comparable limit. The bug only surfaces with a realistic hydrated payload, which is why the prior ECS smoke test (a small Rust cargo-check, #494) didn't catch it. Fix — stash the payload out-of-band and pass only a pointer: - New EcsPayloadBucket construct (mirrors TraceArtifactsBucket): BLOCK_ALL, enforceSSL, S3_MANAGED encryption, 1-day lifecycle TTL (payloads are ephemeral — read once at boot). Dedicated bucket so the ECS task role's S3 read is scoped to payloads only and can't touch attachments/traces. - ecs-strategy: when ECS_PAYLOAD_BUCKET is set, PutObject the payload to <task_id>/payload.json and pass AGENT_PAYLOAD_S3_URI in the override; the boot command fetches+parses it via boto3. Inline AGENT_PAYLOAD remains as a fallback (small payloads / no bucket), so nothing regresses. deleteEcsPayload helper removes the object. - orchestrate-task finalize: best-effort deleteEcsPayload for ECS tasks once terminal (the container has long since read it); lifecycle rule is the crash backstop. - EcsAgentCluster: accept payloadBucket, inject ECS_PAYLOAD_BUCKET env, grant the task role READ ONLY (untrusted repo code must not write/delete payloads; the trusted orchestrator owns write+delete). Session-role-aware. - task-orchestrator: ecsPayloadBucket prop → grantPut + grantDelete to the orchestrator; @aws-sdk/client-s3 added to bundling externals. - agent.ts: updated the commented uncomment-to-enable ECS scaffolding to wire the payload bucket. Tests: new bucket construct (TTL/SSL/block-public/autoDelete); strategy S3-write + URI-pointer + inline fallback + deleteEcsPayload (incl. best-effort swallow + no-op without bucket); cluster read-grant + env var + read-only (no put/delete). Full build green. Closes #502
Root cause of the exit-127 inert gate: the agent Dockerfile installs node/npm/ mise/uv but NOT yarn, so any repo whose build runs 'yarn install' (incl. ABCA) hit 'yarn: command not found' and the agent hand-built a ~/bin/yarn shim every run. corepack (ships with Node 24) installs the yarn/pnpm shims at image-build time. Systemic — fixes every yarn repo, no per-run shim. Needs redeploy to take effect. (cherry picked from commit 2fd5fca) (cherry picked from commit d5e0e64)
…ilds AgentCore's fixed microVM envelope OOM-kills CI-parity builds (live-caught dogfooding ABCA-on-ABCA: the ~2800-test `mise run build` killed the VM at the build-gate step). AgentCore memory isn't tunable, so route repos that set `compute_type: 'ecs'` to a Fargate task instead. - agent.ts: gate EcsAgentCluster + the orchestrator ecsConfig on the `compute_type` deploy context (default 'agentcore'). ECS resources only synthesize under `--context compute_type=ecs`, so default synth (and the bootstrap-coverage test) stays agentcore-only. Mirrors upstream's context-gating of the ECS construct. - ecs-agent-cluster.ts: size the Fargate task at 8 vCPU / 32 GB (was 2/4) — a valid ARM64 combo with headroom for the jest worker fleet + tsc that AgentCore's envelope can't give. Comment documents the live OOM. - test: assert the new 8192/32768 sizing. Verified: default synth has 0 ECS resources; `--context compute_type=ecs` synth emits AWS::ECS::Cluster + TaskDefinition. Full `mise run build` green. (cherry picked from commit 8cb7cfa) (cherry picked from commit 632a36b)
Live-fired the full `mise run build` on the 32GB ECS task (fork dogfood): install OK, build ran ~50 min then OOM-killed at the cap (ECS stoppedReason "OutOfMemoryError: container killed due to memory usage", exit 137; peak working set ~31.6 GB). Root cause: the monorepo build fans out 4 heavy jobs in PARALLEL (agent:quality ‖ cdk:build ‖ cli:build ‖ docs:build), each with its own worker fleet (jest, pytest, esbuild Lambda bundling) — 32 GB had no headroom for that concurrent peak, and the ~50-min wall-clock also blew the 1800s build-gate cap. Fix (keep the full build as the gate, give it room): - ecs-agent-cluster.ts: Fargate 8vCPU/32GB → 16vCPU/64GB (valid ARM64 combo; 16 vCPU admits 32–120 GB). 2× memory headroom for the parallel storm + more cores to shorten wall-clock. Comment records the full sizing history. - ecs-agent-cluster.ts: inject BUILD_VERIFY_TIMEOUT_S=3600 into the container env so a slow-but-healthy CI-parity build isn't mis-flagged as a timeout (post_hooks.py reads it; default 1800 stays for AgentCore repos). - tests: assert 16384/65536 + the new env var. Verified: default synth ECS-free; --context compute_type=ecs synth emits Cpu 16384 / Memory 65536 / BUILD_VERIFY_TIMEOUT_S 3600. Full build green. (cherry picked from commit 11fb3a5) (cherry picked from commit cd7b8eb)
…aren't dropped (ABCA-487) The ECS boot command hand-listed ~18 run_task kwargs and silently dropped the rest. On ECS this meant channel_source/channel_metadata never reached run_task, so resolve_linear_api_token never ran, LINEAR_API_TOKEN was never set, and the Linear/Jira reaction + channel MCP silently no-op'd — @bgagent tasks on ECS posted NO 👀 reaction or anything (live-caught: ABCA-487 on the fork, RUNNING with mcp_servers:[] and no reaction). Also dropped: build_command/lint_command (build-gate used defaults), cedar_policies + approval_* (HITL guardrails), base_branch/merge_branches (orchestration stacking), attachments, trace, user_id. Same "hand-rolled partial payload copy" root as #502. Fix — single source of truth, unit-testable: - pipeline.run_task_from_payload(p): maps the WHOLE payload dict to run_task's signature — rename prompt→task_description / model_id→anthropic_model, filter to accepted params (unknown keys ignored, not passed as **kwargs), coerce issue_number/pr_number→str + max_turns→int, aws_region falls back to env. _RUN_TASK_PARAMS computed once at import (patch-safe). - entrypoint exports it; ecs-strategy boot command now calls run_task_from_payload(p) instead of the hand-listed kwargs. - tests: 9 agent tests (rename, channel fields ABCA-487, build/cedar/branch, coercion, unknown-key drop, None-drop, aws_region, signature guard) + ecs-strategy asserts the boot command calls the mapper and no longer hand-lists. Full build green: agent + cdk 2852 + cli 571 + docs. Deployed to dev (--context compute_type=ecs; agent image rebuilt). (cherry picked from commit 777ee0e) (cherry picked from commit 1639c27)
…th-* (ABCA-488) A Linear/Jira-channel task resolves its per-workspace OAuth token from Secrets Manager (bgagent-linear-oauth-<slug>) at startup to fire the 👀→✅ reaction and drive the channel MCP. The AgentCore runtime role + orchestrator/fanout/ screenshot/linear-integration roles all have the bgagent-linear-oauth-* prefix grant, but the ECS task role only had GetSecretValue on the GitHub token. So on ECS the token fetch hit AccessDenied → LINEAR_API_TOKEN unset → linear_reactions skipped AND the Linear MCP stayed needs-auth (agent couldn't post comments either). Live-caught dogfooding on the fork (ABCA-488, ECS task 828dab35). Fourth ECS-parity gap after #502 (payload size) + ABCA-487 (dropped channel fields): the AgentCore path had a capability the ECS task role didn't. Fix: grant the ECS task role secretsmanager:GetSecretValue on the bgagent-linear-oauth-* and bgagent-jira-oauth-* prefixes (GetSecretValue only — the container reads; the orchestrator owns refresh/PutSecretValue). Nag reason updated + regression test asserting the prefix grant. Full build green (2853). (cherry picked from commit 5a886bb) (cherry picked from commit 49235d4)
#299) Live-caught: a :decompose on an ecs-configured repo ran on ECS (fix confirmed — no more 'ECS_CLUSTER_ARN missing') but then FAILED at delivery with 'ValueError: deliver_artifact: ARTIFACTS_BUCKET_NAME is not configured'. The AgentCore runtime sets ARTIFACTS_BUCKET_NAME; the EcsAgentCluster task def never got the bucket — an ECS-parity gap (same class as #502/ABCA-487). A repo-bound artifact workflow (coding/decompose-v1) delivers its plan JSON there. - EcsAgentCluster: new optional artifactsBucket prop → ARTIFACTS_BUCKET_NAME in the container env + grantReadWrite on the task role (read+write: the container DELIVERS the artifact, unlike the read-only payload bucket). IAM5 suppression extended to cover the second scoped S3 grant. - agent.ts: pass traceArtifactsBucket.bucket (same bucket the runtime uses). - 3 construct tests (env injected / read+write grant / omitted when absent). 2872 CDK + 571 CLI + 1184 agent green. Held on branch. (cherry picked from commit 7617d72) (cherry picked from commit e0dfc93)
… substrate A repo onboarded as compute_type=ecs on a stack deployed WITHOUT --context compute_type=ecs fails every task at session start (no ECS_* env vars). Catch the config/deploy mismatch early + make the runtime error actionable: - agent.ts: new ComputeSubstrate CfnOutput — 'ecs' when the gate is on, else 'agentcore' (ecs is additive; the AgentCore runtime is always present, so an agentcore repo works on either). - cli repo onboard: when --compute-type ecs, read ComputeSubstrate and refuse with a redeploy message if it's an explicit non-ecs value. Null (older stacks predating the output) is treated as unknown → proceed (runtime backstop). - ecs-strategy: the missing-env error now names the root cause + remedy (redeploy with the gate, or set the repo to agentcore) instead of a bare env-var list. - Tests: CDK output flips with the gate (agentcore default / ecs gated); CLI onboard refuses/allows/back-commpat; ecs construct unchanged. 2875 CDK + 575 CLI + 1184 agent green. Held on branch. (cherry picked from commit cb7c8c6) (cherry picked from commit 9048db0)
…baseline OOM ABCA-662's pre-agent baseline build was OOM-killed (exit 137) at 64 GB: dogfooding ABCA-on-ABCA, the full parallel `mise run build` (agent:quality ‖ cdk:build ‖ cli:build ‖ docs:build, each with its own worker fleet) peaked past 64 GB. Each build task is memory-ISOLATED (its own Fargate microVM), so limiting concurrency does NOT help a single over-64 GB build — only more per-task RAM or less build parallelism does. 120 GB is the MAX Fargate admits at 16 vCPU (32–120 in 8 GB steps), so this is the clean experiment: if a build still OOMs here, the only remaining lever is cutting the build's peak parallelism (serialize the mise DAG / cap jest --maxWorkers) — there is no more RAM to give. Pairs with the repo.py fix (an OOM-killed baseline is now infra, not 'already broken'), so even if a build does hit the ceiling the verdict stays honest. Tests updated to assert Memory 122880 (both BUILD-def assertions). (cherry picked from commit 25e2bb1)
The inline-fallback / no-op tests in the top-of-file describe blocks assume the OPTIONAL vars ECS_PAYLOAD_BUCKET and ECS_PLANNING_TASK_DEFINITION_ARN are ABSENT when the module is first imported (it reads them as module-level constants). A dev shell has neither set, so the suite passed locally — but the REAL ECS agent container HAS ECS_PAYLOAD_BUCKET set (#502 payload bucket), so on ECS the const was truthy and the 'no bucket → inline fallback' + 'no-op' assertions failed: FAIL test/handlers/shared/strategies/ecs-strategy.test.ts ● startSession › sends RunTaskCommand with correct params ... ● deleteEcsPayload without ECS_PAYLOAD_BUCKET › no-ops ... This was the residual fork baseline-build exit-1: 2 failed / 3093 passed, only ever reproducible inside the ECS microVM. Surfaced by the live-streaming build log (the buffered summary had hidden it). Fix: delete both optional vars before the top-of-file import so it's hermetic regardless of the runner's environment; the #502/#299 describe blocks already set them via jest.isolateModules. (cherry picked from commit 7a2bde9) (cherry picked from commit 8267c29)
…(cdk synth on fresh clone)
A CDK-based target repo's build gate runs `cdk synth`. When the stack is wired
to a concrete env ({account, region}), CDK does a synth-time availability-zone
context lookup (ec2:DescribeAvailabilityZones). A developer box caches the result
in the gitignored cdk.context.json so synth is hermetic; the agent clones fresh,
so there is no cache and the live lookup fires. The ECS task role lacked the
action, so synth failed with:
ERROR ...EcsAgentClusterTaskRole... is not authorized to perform:
ec2:DescribeAvailabilityZones ...
Synthesis finished with errors
→ a FALSE build-gate failure on code that builds fine everywhere else. Same
ECS-parity class as ABCA-488 (GetSecretValue) and F-2 (CreateEvent): a permission
the AgentCore path had but the ECS task role didn't.
Live-caught on the ABCA fork: baseline `mise run build` passed all 3095 tests
then died at `cdk synth -q`, surfaced only by the live-streaming build log.
Grant is a read-only describe (Resource:* — EC2 describe actions have no
resource-level scoping; IAM5 suppressed, no mutation/data access). +1 regression
test pins the statement.
(cherry picked from commit 3fc9060)
(cherry picked from commit 2bf12f4)
…class) Live-caught during the fork stress smoke (ABCA-495): the ECS TaskDefTaskRole got AccessDeniedException on bedrock-agentcore:CreateEvent for the AgentMemory resource, so the agent's cross-task learning writes (write_task_episode / write_repo_learnings) silently no-op'd (WARN) on EVERY ECS task — memory never persisted on the fork's ECS-only substrate. Same class as ABCA-488: the AgentCore runtime role gets memory access via agentMemory.grantReadWrite(runtime) in agent.ts, and the orchestrator gets it too, but the ECS task role never did. Fix: EcsAgentCluster gains an optional agentMemory prop; when provided the task role gets agentMemory.grantReadWrite (read actions + CreateEvent), scoped to the memory ARN (synth-verified: no wildcard resource, so the existing IAM5 nag suppression is unaffected). agent.ts passes agentMemory to the ECS cluster alongside the existing memoryId. Omitted in isolated construct tests / memory-less deploys → no grant (asserted). Tests: construct asserts CreateEvent on the task role scoped to MemoryArn when wired, and NO bedrock-agentcore grant when unwired. Full cdk build green (2883). (cherry picked from commit c23d49fbead30ed52860e3975815e3643b22ade0) (cherry picked from commit 44bb71e)
The //cdk:bootstrap task hint and DEPLOYMENT_ROLES.md both said to enable the ECS compute backend with 'mise //cdk:bootstrap -- --context ComputeTypes=agentcore,ecs'. That does NOT work: ComputeTypes is a CloudFormation *template parameter*, and this CDK version's 'cdk bootstrap' has no way to set one — there's no --parameters flag, and --context sets CDK construct context, not a CFN parameter. So the ECS policy (IaCRole-ABCA-Compute-ECS, conditional on ComputeTypes containing 'ecs') was never attached, and a --context compute_type=ecs deploy then failed with AccessDenied on ecs:* — live-caught deploying the substrate to dev. Correct it to the mechanism that actually works: bootstrap normally, then set the parameter directly on the CDKToolkit stack via 'aws cloudformation update-stack ... ParameterKey=ComputeTypes,ParameterValue=agentcore,ecs' (with a describe-stacks verify). Fixed in the mise task description + comment and both DEPLOYMENT_ROLES.md occurrences; Starlight mirror regenerated. Drift gate green. (cherry picked from commit a46683e)
…A-667, was 'Unexpected error') Replication finding: a live fork coding child (ABCA-667) failed with 'TimeoutExpired: Command [mise run build] timed out after 600 seconds' — the full ~2800-test fork build exceeded the 600s cap. That's the pre-agent verify-build subprocess timeout, which lands raw in error_message and fell through to 'Unexpected error' + generic guidance — exactly the transient-vs-real confusion this rework targets. It's not a crash and not broken code: the build ran too long. Add a TIMEOUT classification for / → 'Build/tests didn't finish in time', user class, retryable, remedy names the real fix (retry / raise BUILD_VERIFY_TIMEOUT_S or move to the larger ECS build box). Ordered before the generic poll-timeout pattern. Test uses the exact ABCA-667 shape. 60 classifier tests green. (cherry picked from commit 6394a59) (cherry picked from commit b9c1a94)
… (Exec format error) claude-code@2.1.191 uses a shim (bin/claude.exe) + platform-optional-dependency install model: its postinstall (install.cjs) is supposed to replace the shim with the platform-native ELF binary. When that postinstall silently falls back (e.g. it hits EACCES overwriting the shim under npm's root de-privileging), the error-shim stays on PATH and every agent run dies at the run_agent step with 'OSError: [Errno 8] Exec format error: \'claude\'' — even though the native arm64 binary is present in the image as an optional dep, just never wired up. Live-caught on ABCA-659's retry: all 3 ECS runs failed this way on a freshly rebuilt image (the prior working image ran the older single-binary 2.1.142). Re-run install.cjs explicitly after the npm install so the native binary is placed at build time, and hard-verify with 'claude --version' so a still-broken shim fails the image build instead of every task at runtime. (cherry picked from commit 7cb4a87) (cherry picked from commit 7de778b)
…debuggable The post-agent build gate logged only stderr on a non-zero exit. But build/test tooling (jest, tsc, the mise task DAG) writes the ACTUAL failing-task error to STDOUT, not stderr — stderr often carries only the runner's plan echo. So a red `mise run build` surfaced every task STARTING in CloudWatch but never WHICH one failed or why (ABCA-662: a 7s exit-1 with zero captured cause, indistinguishable from a phantom, and the reason Linear showed a build-fail GitHub CI passed). run_cmd now also tails the last 20 lines of stdout on failure (redacted — repo build output is untrusted). 4 tests pin: stdout surfaced, tailed-not-headed, redacted, and NOT dumped on success. (cherry picked from commit 67d84db) (cherry picked from commit bb578fd)
…ust a tail The stdout-on-failure logging (67d84db) tailed the last 20 lines — enough for a serial command, but NOT for `mise run build`, which runs 4 packages in PARALLEL and interleaves their output. The failing task's error lands in the MIDDLE while the tail captures whatever finished LAST (e.g. a PASSING package's coverage table) — so the log showed a green coverage table on a red build and named nothing (ABCA-662: three verification runs where the actual failing sub-task was never visible in CloudWatch). run_cmd now scans the WHOLE stdout for failure-signature lines (FAIL/✕/●/error TS/ELIFECYCLE/coverage-threshold/mise 'no task'/'task failed'/traceback/…), filters benign noise ('0 errors', '--no-error'), and logs those FIRST, then a trailing tail for context — deduped, capped at 40 lines. Falls back to the tail when nothing matches (unknown tool). This is the tooling gap that made the whole ABCA-662 build investigation slow: every failure was diagnosable only by luck or local repro. 5 tests incl. the mid-DAG-failure + coverage-threshold cases. (cherry picked from commit 3fe99c9) (cherry picked from commit aaa4f18)
isadeks
added
bug
scottschreckengaust
previously approved these changes
Jul 14, 2026
scottschreckengaust
left a comment
scottschreckengaust
left a comment
Contributor
There was a problem hiding this comment.
Review — Principal Architect (Stack A: ECS substrate, PR 3/4)
Base verified against fresh origin PR head (diffed vs #596). Mandatory agents run stack-wide (full table on the #596 review).
Verdict: Approve (with a nit)
Solid build/verify-diagnosis work, all three changes tied to live incidents:
_surface_failure_lines(agent/src/shell.py) — scans the WHOLE stdout for failure-signature lines, not just the tail. Correct for a parallel mise DAG that interleaves output so the failing task's line lands in the middle while the tail is a passing package's coverage table (ABCA-662). Capped at 40 + a 15-line tail, deduped, and redacted (redact_secrets) since repo build output is untrusted. Thereturn out or tailfallback guarantees a failure never logs nothing. Verifiedrun_cmd'scheck=Truepath still raises — the added stdout logging is purely additive, not a swallow.- Error-classifier TIMEOUT pattern (
cdk/src/handlers/shared/error-classifier.ts) — classifies a build/verify wall-clock kill distinctly from a crash (retryable: true, user-actionable remedy). Verified it's ordered before the generic catch-all and isn't shadowed by the earlierpoll timeout exceededrule. - Dockerfile claude
install.cjsre-run +claude --version— a real fix: claude-code@2.1.x ships an error-shim that its postinstall can silently leave in place, dying at runtime with "Exec format error". Re-running the installer and hard-verifying it exec's fails the build instead of the task.
Tests (test_shell.py::TestRunCmdFailureLogging, error-classifier.test.ts) cover the key cases well: stdout-logged-on-failure, mid-DAG line surfaced, coverage-threshold surfaced, benign "0 errors" not falsely elevated, redaction, success-doesn't-dump, no-marker→tail fallback, and the timeout classified as retryable-not-Unexpected.
Non-blocking nit
- N5 — Broad failure markers.
_FAILURE_LINE_MARKERSincludes broad substrings ("error:","failed") that could surface benign advisory lines. It's logging-only, capped, redacted, and the_FAILURE_LINE_NOISEallowlist covers common false positives — so worst case is slightly noisier CloudWatch, no functional impact. Flagging for awareness only.
Governance note
Branch pr/agent-build-robustness carries no issue number and the body references only stacked PRs (ADR-003). Same governance item raised on #596 — please attach an approved issue and a conforming branch name before merge. Code is approve-ready.
CI
All green (title, CodeQL ×3, dead-code advisory, secrets/deps/actions, build (agentcore)); MERGEABLE. No CDK construct/stack change → bootstrap synth-coverage N/A.
🤖 Generated with Claude Code
The `export const ECS_PAYLOAD_OBJECT_KEY_PREFIX = ''` constant was referenced nowhere — ecsPayloadKey() (ecs-strategy.ts) builds `<task_id>/payload.json` independently and already documents that layout in its own docstring. The dead export was a "prefix" whose value was '' with a docstring describing a key layout it didn't produce, and it raised the knip dead-code count, tripping the ratchet inside `mise run build`. Delete it; ecsPayloadKey remains the single source of truth for the key layout.
Review follow-ups for #596 (fix itself unchanged; B1 dead-export fixed in the base PR #503): - N1: correct the stale "64 GB / 16 vCPU" comment in agent.ts — the task is larger (64 GB was itself OOM-killed per the construct's sizing history). Defer the exact figure to EcsAgentCluster rather than duplicate/drift it. - N2: reword the memory-grant + oauth-grant comments — an AccessDenied on those paths is LOGGED (memory.py treats it as an infra failure; config.py's token resolver logs it), not "silently" no-op'd. Describe the user-visible effect (no persisted learning / no 👀→✅ reaction) rather than pin a log level that drifts. - N3: the ec2:DescribeAvailabilityZones comment already frames it correctly as a FALSE build-gate failure (not a WARN no-op) — no change needed; noted here for completeness. - N4: run_task_from_payload now emits a WARN when it drops a KNOWN orchestrator key (build_command / merge_branches / base_branch / github_token_secret_arn) that run_task doesn't accept — expected today (consumed elsewhere / not yet wired), but makes a future "wired one side, forgot the other" no-op visible instead of silent (the ABCA-487 class). Foreign keys still drop quietly. + 2 tests (warns on a known dropped key; stays quiet on a foreign key). Full cdk build green (2286) + full agent suite green (1194); eslint/ruff clean.
Contributor
Author
|
Thanks for the approve. On N5 (broad |
5 tasks
CI's "Fail build on mutation" tripped: I ran `ruff check` (passed) but not
`ruff format`, so the `_KNOWN_ORCHESTRATOR_KEYS = frozenset({...})` set literal
and the two-line `log(...)` call weren't in ruff's canonical multi-line form.
`ruff format` normalizes them; no behavior change (12 payload tests still pass).
…ax_turns + comment nits Follow-up on the #596 re-review (code was already approved-quality; these are the new non-blocking finding + 3 nits): - task_started_at HITL parity: AgentCore's server.py exports task_started_at as TASK_STARTED_AT, which hooks._remaining_maxlifetime_s() uses to clip the Cedar HITL approval-gate maxLifetime. The ECS boot path bypasses server.py and never sets it, so run_task_from_payload dropped it SILENTLY — a fail-open AgentCore↔ECS HITL divergence. Added task_started_at to _KNOWN_ORCHESTRATOR_KEYS so the drop now WARNs (surfaces the gap). Fully wiring TASK_STARTED_AT into the ECS containerEnv is a tracked follow-up; this makes the divergence visible today. - max_turns defensive coercion: a malformed max_turns raised ValueError mid-boot while every other field defaults defensively. Now drops it (run_task default applies) with a WARN, matching the surrounding style. - Nit: artifactsBucket JSDoc no longer claims this bucket is the `--trace` upload target (it wires ARTIFACTS_BUCKET_NAME only; the trace uploader reads TRACE_ARTIFACTS_BUCKET_NAME, unset here — noted as a separate ECS-parity gap). - Nit: dropped `2>/dev/null` from the Dockerfile corepack prepare so the prepare-failed diagnostic isn't hidden (the `|| corepack enable` fallback still guarantees resolvable shims — no regression to the exit-127 inert gate). Tests: +3 (task_started_at WARN, malformed-max_turns dropped-not-raised, valid max_turns still coerced). Full agent suite green (1195); cdk tsc + construct tests green; ruff format + eslint clean.
…rdening # Conflicts: # cdk/src/constructs/ecs-agent-cluster.ts # cdk/src/handlers/shared/strategies/ecs-strategy.ts # cdk/src/stacks/agent.ts # cdk/test/constructs/ecs-agent-cluster.test.ts # cdk/test/handlers/shared/strategies/ecs-strategy.test.ts
3 tasks
isadeks
commented
Jul 15, 2026
isadeks
left a comment
isadeks
left a comment
Contributor
Author
There was a problem hiding this comment.
Cross-reference: Dockerfile fix overlaps #613 (and complements #599)
Flagging a coordination point surfaced while reviewing #613 — the claude-binary Exec format error is being addressed across three open PRs:
- This PR (#597) and #613 both append
node install.cjsto the sameRUN … npm --prefix … update …line inagent/Dockerfile, so they will textually conflict — both can't merge as-is. #597's version is the superset: it also adds&& claude --version, hard-gating the image build so a silent postinstall fallback failsdocker buildrather than every task at runtime (strictly better than restore-and-hope), plus a comment explaining the shim mechanism. - #599 is complementary, not conflicting — it classifies the same
Exec format errorinto a user-facing message (error-classifier.ts, disjoint file). Prevention (here/#613) + legibility (#599) are the two halves of ABCA-659.
Suggested resolution: since #597's Dockerfile hunk supersedes #613's, either close #613 in favor of this PR, or — if this PR stays blocked behind its stack (#596 → #503; #503 merged, #596 still open) and the exec-format break is hurting people now — let #613 merge against main and drop the Dockerfile hunk here on rebase. Whoever lands first wins the line; the other should rebase around it.
(I verified install.cjs is idempotent and exists in @anthropic-ai/claude-code@2.1.191; couldn't reproduce the npm update clobber on linux/amd64, so it looks arm64/AgentCore-specific — consistent with the live-caught reports. The && claude --version gate here is the right belt-and-braces regardless.)
…+ nits
B1 (least-privilege regression): remove props.artifactsBucket.grantReadWrite
from the ECS task role. coding/decompose-v1 delivers its plan via the assumed
SessionRole (deliverers.py -> tenant_client), scoped to artifacts/${task_id}/*,
exactly like the AgentCore runtime (whose task role likewise has NO direct
artifacts grant). The whole-bucket grant over-privileged the untrusted-code
role and broke cross-task isolation (a task could read/clobber other tasks'
artifacts/<other_id>/, traces/, attachments/). Task role keeps only the
ARTIFACTS_BUCKET_NAME env. Correct the inverted 'parity' comment and drop the
now-stale artifacts clause from the cdk-nag IAM5 reason.
Test: flip 'grants READ+WRITE on artifacts' → asserts the task role has NO S3
Put/Delete action at all (the read-only #502 payload GetObject*/List* grant is
not flagged).
N4: add lint_command to _KNOWN_ORCHESTRATOR_KEYS (sibling of build_command) so a
future 'wired build_command, forgot lint_command' contract gap WARNs instead of
dropping silently.
(N1/N2/N3 comment nits were already addressed on the branch head; B1 dead-const
ECS_PAYLOAD_OBJECT_KEY_PREFIX no longer present. B2 governance handled separately.)
…noise + max_turns coercion Scott's fresh re-review (head 86ab249) is Approve-with-nits: B1 (over-privilege) and B2 (governance) resolved; remaining asks are doc-accuracy + WARN-channel hygiene. - N1: rewrite the artifactsBucket JSDoc (ecs-agent-cluster.ts) that still claimed a read/write task-role grant the B1 fix deliberately removed — now states env-only wiring with delivery through the task_id-scoped SessionRole (parity with the AgentCore runtime role, which has no artifacts grant). The grant block ~30 lines below was already corrected in the B1 commit; this was its residual half. - N2: same inverted-comment fix at the agent.ts wiring site — '(read+write grant in the construct)' → env-only + SessionRole delivery. - N3: drop github_token_secret_arn from _KNOWN_ORCHESTRATOR_KEYS. It is ALWAYS present and ALWAYS resolved via the GITHUB_TOKEN_SECRET_ARN env, so listing it fired the known-key WARN on 100% of ECS boots — pure noise diluting the channel meant for genuine future contract gaps. Now falls through as a quiet foreign-key drop; comment records why it must not be re-added. +test asserting no WARN. - N4: max_turns int() coercion accepted a bool (int(True)==1) and truncated a non-integral float (int(3.9)==3), and the comment falsely claimed it 'matches how every other field is handled' (it is the one non-str coercion). Now rejects bools + non-integral floats with a breadcrumb; valid int / int-string / int-float still pass. +test. Reachable only via a corrupt payload (orchestrator emits a real int), so cosmetic — bundled since cheap. - N5: no-op — the Dockerfile corepack line already has no 2>/dev/null. Gates: cdk 2355 tests + eslint + synth green; agent 1268 tests + ruff + ty green. Non-blocking #608 test-seam gaps (finalize deleteEcsPayload, boot-uri parse, entrypoint re-export, zero-ECS-synth assert) remain tracked, not in scope here.
…nt-build-robustness
ayushtr-aws
dismissed
scottschreckengaust’s stale review
July 21, 2026 19:47
The base branch was changed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Platform-agnostic robustness fixes for the agent's build/verify path, surfaced dogfooding ABCA-on-ABCA on the ECS build box but not ECS-specific — they improve failure diagnosis for any repo/substrate.
What changed
fix(errors): classify a build/verify command TIMEOUT — aTimeoutExpiredfrom a long build previously fell through to a bare "Unexpected error"; now classified as a user-actionable timeout (retry / raiseBUILD_VERIFY_TIMEOUT_S),retryable: true. Live-caught (ABCA-667).fix(agent): place the claude-code native binary at image build — claude-code@2.1.x ships an error-shim atbin/claude.exethat its postinstall replaces with the platform-native ELF; the postinstall silently fell back at build time, leaving the shim on PATH → every agent died withExec format error. Now re-runinstall.cjs+ hard-verifyclaude --versionso a broken shim fails the image build, not every task.fix(agent): log stdout onrun_cmdfailure — build-gate failures were undebuggable without the command output.fix(agent): surface the failing line from a parallel build DAG — a fan-outmise run buildfailure showed only a tail; now surfaces the line that actually names the failure.Testing
mise run buildgreen (CDK + agent + CLI); new/updated tests inerror-classifier.test.ts,test_shell.py.claude --version→ real 2.1.191, not the error shim).Notes for reviewers