feat(ecs): make the ECS Fargate compute backend usable (context-gated wiring + ECS-parity fixes)#596
feat(ecs): make the ECS Fargate compute backend usable (context-gated wiring + ECS-parity fixes)#596isadeks wants to merge 26 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)
isadeks
added
enhancement
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
Review — Principal Architect (Stack A: ECS substrate, PR 2/4 · stack tip under review)
Bases verified against fresh origin PR heads (this PR diffed vs #503). Bootstrap ADR-002 coverage independently verified. All mandatory agents run (table below).
Verdict: Request changes
The heart of the stack and strong work — the compute_type=ecs gate keeps the default synth byte-for-byte unchanged, run_task_from_payload binds to run_task's real signature via inspect.signature (a genuine fix for the ABCA-487 silent-drop, well-tested), the CLI onboard guard reads the new ComputeSubstrate output to fail at config-time instead of per-task, and the parity IAM grants are least-privilege and individually tested. Two items block: a dead-code defect that breaks the required build, and the ADR-003 governance gap.
Bootstrap ADR-002 — verified no AccessDenied gap. When deploying --context compute_type=ecs, every newly-synthesized resource type/ARN is covered: the new payload bucket auto-names to backgroundagent-dev-* (covered by observability.ts S3ApplicationBuckets, same as the existing attachments/trace buckets), the Custom::S3AutoDeleteObjects Lambda reuses the singleton provider that already deploys today, and the ECS create-actions (ecs:CreateCluster, ecs:RegisterTaskDefinition) + iam:PassRole for ecs-tasks.amazonaws.com are in compute-ecs.ts/infrastructure.ts. BOOTSTRAP_VERSION correctly unchanged (no new granted action — flipping the gate exercises grants that already existed).
Blocking issues
B1 — Dead-code ratchet defect breaks mise run build. cdk/src/constructs/ecs-payload-bucket.ts — export const ECS_PAYLOAD_OBJECT_KEY_PREFIX = '' is unused (verified: only its own declaration; ecsPayloadKey() builds the key independently). Under root knip.json (project: ["src/**/*.ts!"]) it raises the knip count 78→79 and scripts/check-deadcode-ratchet.mjs process.exit(1)s above baseline.
- Severity nuance: the CI "Dead-code detection" job is
continue-on-error/ advisory — it will not fail the PR check (currently green). But the same ratchet runs insidemise run build, which this repo mandates green before commit — so it breaks the required local build. (Introduced in #503; reachable once building.) - Fix: delete the constant + its doc block; fold the key-layout note into
ecsPayloadKey()'s docstring.
B2 — Governance (ADR-003): no approved issue; non-conforming branch. Branch pr/ecs-substrate-hardening carries no issue number, and the body references stacked PRs plus #494 (a merged PR, not an open approved issue). Per ADR-003 the substrate-usability + ECS-parity work needs its own approved GitHub issue (a natural sibling of #502/#494).
- Fix: file/link an approved issue; rename the branch to
feat/<n>-…at merge time (renaming an open PR head closes it — reasonable to defer, as accepted on #594).
Non-blocking suggestions / nits
- N1 — Stale figure.
cdk/src/stacks/agent.tscomment says "64 GB / 16 vCPU" but the task is 120 GB (memoryLimitMiB: 122880) — and theEcsAgentClustersizing history explicitly records 64 GB was OOM-killed. Update to 120 GB or defer to the construct. - N2 — Comment/code mismatch. The
bgagent-*-oauth-*grant comment says AccessDenied "silently no-ops (WARN)", butconfig.pylogs it at ERROR. Code is better than the comment; fix the comment. - N3 — Narrative. The
ec2:DescribeAvailabilityZonesgrant is grouped under "silently no-ops (WARN)" but it's actually a loud false build-failure fix (thecdk synthgate raises on AccessDenied). Grant + the Resource:* cdk-nag suppression are correct (EC2 describe has no resource scoping, read-only); just re-word. - N4 — Silent key-drop (defensive, but).
run_task_from_payloaddrops known-but-unaccepted orchestrator keys (build_command,merge_branches— not yetrun_taskparams). Verified safe today (not emitted;base_branchis consumed viahydrated_context, matching AgentCore). A one-line WARN when dropping a known orchestrator key would make a future "wired one side, forgot the other" no-op visible instead of silent. Optional.
Tests & CI
- CI: green (title, CodeQL ×3, dead-code advisory, secrets/deps/actions,
build (agentcore));MERGEABLE/CLEAN. - Coverage: strong on leaf logic — the
run_task_from_payloadtruth-table with aninspect.signature-based "every-forwarded-key-is-real" guard is excellent, as are the grant tests (CreateEvent scoped to the memory ARN, oauth prefix, DescribeAZs, read-only payload bucket) and the CLI onboard-gating matrix. Three material wiring-seam gaps (non-blocking follow-ups):- (crit 7)
deleteEcsPayloadtested only in isolation — no test thatorchestrate-task.tsfinalize calls it iffcompute_type==='ecs', andtask-orchestrator.test.tsasserts neither theECS_PAYLOAD_BUCKETenv norgrantDelete. A regression would spuriously DeleteObject on every AgentCore task (masked by the no-op guard). - (crit 6) the boot-command S3-loader one-liner is asserted only as a string (
toContain('get_object')); the_uri.split("/",3)parse on multi-slash keys is never executed. It has drifted once already — extract it into a testedload_payload_from_uri(). - (crit 4)
run_task_from_payloadis always mocked; theentrypointre-export the boot command imports isn't asserted (dropping it breaks the container at runtime, tests still pass).
- (crit 7)
- Bootstrap synth-coverage: passes, but a latent test-blindness worth a follow-up —
resource-action-map.tsomitsAWS::ECS::Cluster/AWS::ECS::TaskDefinitionandsynth-coverage.test.tssynths default (agentcore) context only, so the ECS path is never coverage-checked. Grants are present (deploy works); the guard just can't see them. Add the two map entries + an ECS-context synth case. - Recommend a dedicated
/security-reviewpass on this PR before merge given the new task-role permissions (I reviewed them directly + via silent-failure-hunter and found them least-privilege, but defense-in-depth is cheap here).
Review agents run (Stage 3 — mandatory, stack-wide)
| Agent | Ran? | Outcome |
|---|---|---|
code-reviewer |
✅ | 1 build-breaker (B1, dead export — severity corrected: advisory CI, breaks mise run build); nits N1. |
silent-failure-hunter |
✅ | No blocking silent failures. Write-path PutObject / boot S3 fetch / run_cmd check=True / exit-code mapping all fail loud; the two swallowing paths are correctly backstopped. Surfaced N2, N3, N4. |
pr-test-analyzer |
✅ | Coverage strong; 3 wiring-seam gaps above. |
general-purpose (bootstrap ADR-002) |
✅ | No AccessDenied gap; latent map/coverage-test blindness for ECS types. |
type-design-analyzer |
⏭️ omitted | No new exported types of substance (props interfaces consumed in-file). |
comment-analyzer |
⏭️ omitted | Comment accuracy covered by hand + silent-failure-hunter (N2/N3). |
/security-review |
⏭️ omitted-with-note | IAM/secrets touched; reviewed directly (see above) — recommend a dedicated pass before merge. |
Human heuristics
- Proportionality — ✅ Sizing/gating/S3-indirection all match documented problems (8 KB cap, OOM history, silent parity gaps). Not over-built.
- Coherence — ✅
run_task_from_payloadreplaces a drift-prone hand-list with a signature-bound single source of truth; ECS grants mirror the AgentCore runtime's. - Clarity —
⚠️ Mostly excellent "why" comments; three (N1/N2/N3) drifted from the code. Fix for the next maintainer. - Appropriateness — ✅ Live-verified on a dev stack (real Fargate run, Linear reaction + PR-link comment), not just mocks.
Bottom line: the substrate is high-quality and the scariest part — bootstrap coverage under least-privilege — checks out. Clear B1 (delete the dead '' constant so mise run build passes) and B2 (approved issue + conforming branch); N1–N4 and the three test-seam gaps are strongly recommended follow-ups.
🤖 Generated with Claude Code
Correction to my earlier review — B1 severity was overstatedFollowing up on my What I got wrong: I framed B1 as blocking because it "breaks the required
What stands: Net effect on the verdict: the remaining blocker is B2 (governance — approved issue + conforming branch, ADR-003). If/when #604-style approved issues are attached for the substrate work and the branch is renamed, and B2 is otherwise satisfied, I'd be happy to move this to Approve — the code itself (bootstrap coverage included) checks out. The 🤖 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.
|
Thanks — addressed everything in B1 (dead export → breaks N1 (stale 64 GB) — reworded the N2 (WARN vs ERROR) — reworded the memory-grant and oauth-grant comments: an AccessDenied on those paths is logged (memory.py classifies it as an infra failure; config.py's token resolver logs it), not "silently" no-op'd. The comments now describe the user-visible effect (no persisted learning / no 👀→✅ reaction) rather than pin a specific log level. N3 (DescribeAZs narrative) — the N4 (silent key-drop) — did it: B2 (governance) — same as the rest of the stack: I'll file/link an approved GitHub issue for the substrate-usability + ECS-parity work (sibling of #502/#494) and rename the branch to Test-seam follow-ups (the 3 crit-4-to-7 gaps + the resource-action-map ECS blindness) — all fair; these are genuinely wiring-seam coverage rather than defects in the diff. I'd like to take them as a fast follow-up issue rather than expand this PR's scope — agreed? Full cdk build green (2286) + full agent suite green (1194); eslint/ruff clean. Re-requesting review. |
|
Filed the follow-ups:
|
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).
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
Follow-up review — my CHANGES_REQUESTED still stands (B2 governance)
Re-reviewed after the latest push. The code is in good shape and B1 + N1–N4 are handled; the remaining blocker is governance (B2), unchanged.
✅ Resolved since my prior review
- B1 (dead
ECS_PAYLOAD_OBJECT_KEY_PREFIXexport) — removed in the base PR #503 (ce93b62). (As I corrected earlier, this was advisory, not a build-breaker.) - N1 — the stale "64 GB / 16 vCPU" comment in
agent.tsnow defers sizing toEcsAgentCluster. ✅ - N2 — memory/oauth grant comments reworded to "logged, non-fatal" + user-visible effect; verified accurate against
memory.py/config.pyerror paths. ✅ - N4 —
run_task_from_payloadnow WARNs when dropping a KNOWN orchestrator key (+ 2 tests). ✅ - Sizing history, IAM5 suppression enumeration, boot-command comment, and "parity with TraceArtifactsBucket" all verified accurate.
🔴 Blocking — B2: governance (ADR-003)
This PR is not covered by an approved issue on a conforming branch:
- Issue #606 ("make the ECS Fargate compute backend usable") is OPEN, unlabeled (
enhancementonly — noapproved), unassigned. #164 (gate ECS on context) is also OPEN/unapproved. - Branch is
pr/ecs-substrate-hardening— does not follow(feat|fix|chore|docs)/<issue-number>-short-description.
Per ADR-003 this must land before merge: get #606 (and/or #164) the approved label from an admin, self-assign, comment "Starting implementation," and re-cut on a conforming branch (e.g. feat/606-ecs-substrate). Everything else here is mergeable-quality; this is the gate.
🟠 New non-blocking finding — ECS/AgentCore HITL parity gap (task_started_at)
run_task_from_payload drops task_started_at silently (not a run_task param, not in _KNOWN_ORCHESTRATOR_KEYS). On AgentCore, server.py:612-614 exports it as TASK_STARTED_AT, which hooks.py:_remaining_maxlifetime_s() uses to clip the Cedar HITL approval-gate maxLifetime. The ECS boot path bypasses server.py, so TASK_STARTED_AT is never set → the clip returns None → fail-open (gate fires with the task default rather than the clipped ceiling).
- This is fail-open by design (
hooks.py:801-809documents these as "optional hints"), so it's not a hard blocker — but it's a silent AgentCore↔ECS HITL divergence with no log, exactly the "wired one side, forgot the other" class this PR's own WARN mechanism targets. - Fix: have the ECS strategy set
TASK_STARTED_ATincontainerEnvfrompayload.task_started_at(mirrorserver.py), and/or addtask_started_atto_KNOWN_ORCHESTRATOR_KEYSso the drop at least WARNs. Worth doing before ECS is used in anger for HITL workflows.
🟡 Nits (non-blocking)
- Comment overclaim —
ecs-agent-cluster.tsartifactsBucketJSDoc says the bucket is "also the--traceupload target," but the construct injects onlyARTIFACTS_BUCKET_NAME; the trace uploader readsTRACE_ARTIFACTS_BUCKET_NAME(telemetry.py:446), which is not set here.deliver_artifactworks; trace upload would silently skip on ECS. Drop the clause or note the limitation. - Dockerfile corepack —
... 2>/dev/null || corepack enable: the fallback still installs resolvable shims (no regression to the exit-127 inert-gate bug — good), but2>/dev/nullhides the one diagnostic (prepare-failed) you'd want. Drop2>/dev/null. int(value)onmax_turns(pipeline.py) raises on a malformed payload while every other field is defensively defaulted; atry/except → defaultwould match the surrounding style.
Tests & CI
Coverage strong and behavior-first (the inspect.signature-based "every-forwarded-key-is-real" guard, the CreateEvent-scoped-to-memory-ARN and oauth-prefix grant tests, the CLI onboard truth-table). Three wiring-seam gaps remain (finalize→deleteEcsPayload iff ecs; boot-command S3 parse only string-asserted; default synth doesn't assert 0 ECS resources) — all captured in #608, good as follow-ups. #366 perf hygiene clean (full synth cached in beforeAll, bundling stays disabled). CI green, MERGEABLE.
Note on the stack: #596 is based on #503's branch — merge/retarget #503 (now approved) first. #503 and #599 both touch orchestrate-task.ts (finalize vs. start-session), so whichever lands last needs a base refresh.
🤖 Reviewed with Claude Code
…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.
|
Thanks for the re-review. B2 (governance) is now unblocked — issue #606 has the Addressed the new finding + nits in
+3 tests (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. The 3 wiring-seam gaps stay captured in #608 (also now |
…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
|
Resolved the conflicts ( Resolution (all HEAD-wins — this PR is what adds the wiring):
Verified by full Still on B2 for merge: branch rename to |
|
@scottschreckengaust — re-requesting review. B2 is cleared: governing issue #606 is now On the branch name: renaming is genuinely disruptive here — renaming an open PR's head branch closes the PR and orphans the stacked #597/#598. Since #606 is linked + approved, could the linked issue satisfy ADR-003 (with the |
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
Review — Principal Architect (Stack A: ECS substrate, re-review)
Re-reviewed against fresh origin PR head c2a3f26 (now targets main — #503 merged). All mandatory agents re-run on the current head (table below); the one prior technical concern and the governance gate are re-verified. My earlier CHANGES_REQUESTED is superseded by this review.
Verdict: Request changes
The substrate is high-quality and the two prior blockers are cleared — B1 (dead export) was removed in the merged base #503, and B2 governance is now resolved: #606 carries the approved label and its title matches this PR, so the ADR-003 gate is satisfied (the pr/ branch name is a de-facto-waived nit — #600/#601/#602/#594/#595 all merged on pr/* branches last week). The task_started_at HITL-parity gap I flagged is now handled with a WARN + regression test (ca8af9a0). Nice work.
One new blocking finding surfaced on re-review by the code-reviewer agent and independently verified by reading the runtime write path: a least-privilege regression on the ECS task role (which runs untrusted repo code). It widens blast radius on the ambient role beyond what the AgentCore path grants, with a rationale comment that is factually inverted. Because VISION.md's bounded blast radius is a stated tenet and this trade is undocumented (and, as it turns out, unnecessary), I'm gating on it. The fix is a one-liner.
Blocking issues
B1 — Least-privilege regression: the ECS task role gets whole-bucket grantReadWrite on the shared artifacts bucket, but the actual write goes through the prefix-scoped SessionRole — so the grant is both redundant and over-broad, and the "parity with the AgentCore runtime" rationale is inverted. cdk/src/constructs/ecs-agent-cluster.ts:236-242.
I traced the real delivery path:
agent/src/workflow/deliverers.py:194-198writes the artifact viatenant_client("s3", …)→ the assumed SessionRole credentials, not the ambient task role.agent.ts:632always wiresagentSessionRoleintoEcsAgentCluster, andaws_session.pyfails closed (SESSION_SCOPING_FAILED) if the assume fails — it never falls back to the task role.- The SessionRole already grants exactly this write, scoped to
artifacts/${aws:PrincipalTag/task_id}/*(agent-session-role.ts:204-213), and the deliverer writesartifacts/{task_id}/result.md(deliverers.py:178). - Critically, the AgentCore runtime role gets NO artifacts-bucket grant at all (
agent.ts:423-426— only github-token, logs, memory, models); it relies solely on the SessionRole. So the comment at:236-238("the AgentCore runtime's SessionRole/exec-role has the equivalent" → "grant read+write … parity") is inverted: parity with AgentCore means no task-role grant, not a whole-bucket one.
Risk: whole-bucket s3:GetObject*/s3:PutObject*/s3:DeleteObject* on the role that executes untrusted repo code lets a compromised/misbehaving task read or clobber other tasks' artifacts under artifacts/<other_task_id>/ (and the traces//attachments/ prefixes on the same TraceArtifactsBucket) — exactly the cross-task isolation the SessionRole's task_id-scoped policy exists to enforce. This is self-hosted single-org, so it's defense-in-depth rather than a hard SaaS tenant boundary — but it's a documented-tenet trade with no ADR and, since production delivery already works via the SessionRole, no upside.
- Fix (preferred): drop the task-role grant entirely; keep only the
ARTIFACTS_BUCKET_NAMEenv (that env var is the genuinely-required piece —deliverers.py:185-187raises if unset — and it's wired correctly). Then correct the:236-239comment to say the SessionRole carries the write in production (matching AgentCore). - Fix (if a no-SessionRole fallback must work): scope it to
arnForObjects('artifacts/*')withs3:PutObject/s3:GetObjectonly, notgrantReadWriteon the whole bucket — and still fix the comment.
Contrast with the two adjacent grants, which are correct: payloadBucket.grantRead is deliberately read-only because the container runs untrusted code (:227-231), and agentMemory.grantReadWrite is genuinely required because memory writes go through the ambient task role (memory.py uses plain boto3, no SessionRole) — that one is true AgentCore parity. The artifact grant is the odd one out.
Non-blocking suggestions / nits
- N1 —
_KNOWN_ORCHESTRATOR_KEYSomitslint_commandwhile listing its siblingbuild_command.agent/src/pipeline.py:1303-1318. Neither field is emitted in the payload today (no runtime effect), but if configurable-verify ever lands,build_commandwould drop with the intended WARN breadcrumb andlint_commandwould drop silently — the exact asymmetry this mechanism exists to prevent. Addlint_commandalongsidebuild_command(the test attest_run_task_from_payload.py:240already treats both as the same class). - N2 —
stopSessionswallows non-InvalidParameterException/ResourceNotFoundExceptionerrors at ERROR and returns as if stopped.cdk/src/handlers/shared/strategies/ecs-strategy.ts:297-307(not in this diff — flagging for a follow-up). On a throttle/5xx, the Fargate task keeps running (burning the 120 GB / 16 vCPU box) while the orchestrator considers it stopped. It's logged (not silent), and best-effort-at-finalize is defensible, but consider retrying onThrottlingException/ServerException, or at least add the justification comment the file's other swallows carry. - N3 — Test-seam +
resource-action-mapECS blindness — already captured in #608 (approved). Thepr-test-analyzerre-confirmed four gaps are still open on this head: (1) thefinalize→deleteEcsPayloadiff compute_type==='ecs'conditional (orchestrate-task.ts:332-334) is untested — a regression deleting on every AgentCore task is a silent no-op that no test catches; (2) the boot-command S3-loader_uri.split("/",3)parse is string-asserted only, never executed; (3)run_task_from_payload's return value is never asserted and thefrom entrypoint import run_task_from_payloadpath (the one the container uses) is untested — dropping thereturnor the re-export breaks every ECS task while tests stay green; (4) the default synth doesn't assert zero ECS resources (theComputeSubstrateoutput flip isn't an independent guard — it's derived from the same gate expression). Good as #608 follow-ups; none blocks.
Documentation
docs/design/DEPLOYMENT_ROLES.mdedited with its Starlight mirrordocs/src/content/docs/architecture/Deployment-roles.md(both in the diff) — sync boundary satisfied; CI "Fail build on mutation" is green. Roadmap not touched (no roadmap item maps to context-gated ECS enablement — acceptable).
Tests & CI
- CI: all green (CodeQL ×3, dead-code advisory, secrets/deps/actions,
build (agentcore), title);MERGEABLE. - Coverage: strong and behavior-first — the
run_task_from_payloadtruth-table with theinspect.signature-bound "every-forwarded-key-is-real" guard, the resource-scoped IAM grant tests (CreateEvent→memory ARN, oauth prefix, DescribeAZs=*, read-only payload bucket), and the CLI onboard-gating matrix are all excellent. Gaps are the four wiring seams in N3 (#608). - Bootstrap (ADR-002): verified — no AccessDenied gap at deploy.
--context compute_type=ecsintroducesAWS::ECS::Cluster/::TaskDefinition, the payload bucket, task/exec roles,iam:PassRole→ecs-tasks.amazonaws.com, log groups, SG — all already covered by the pre-existing bundle (compute-ecs.ts,infrastructure.ts,observability.tsbackgroundagent-dev-*).BOOTSTRAP_VERSIONcorrectly unchanged (context-gated → default synth is byte-identical, no newly-granted type in the default deploy). The only bootstrap issue is latent test-blindness (ECS types absent fromresource-action-map.ts+synth-coverage.test.tssynths default context only) — non-blocking, tracked in #608.
Review agents run (Stage 3 — mandatory)
| Agent | Ran? | Outcome |
|---|---|---|
code-reviewer |
✅ | 1 blocking → B1 (over-broad artifacts task-role grant + inverted parity comment); N1 (lint_command asymmetry). |
silent-failure-hunter |
✅ | No blocking silent failures. Boot S3 fetch fails loud (uncaught → non-zero exit → pollSession maps to failed); deleteEcsPayload/known-key-drop/max_turns-coerce all logged best-effort; CLI guard fails closed; IAM AccessDenied surfaces at WARN/ERROR. Surfaced N2 (stopSession orphan risk). |
pr-test-analyzer |
✅ | Coverage strong; four wiring-seam gaps (N3) still open — all in #608. #366 perf clean (context not postCliContext, beforeAll caching). |
general-purpose (bootstrap ADR-002) |
✅ | No deploy-time AccessDenied gap; BOOTSTRAP_VERSION correctly unchanged; latent resource-action-map ECS blindness (#608). |
type-design-analyzer |
⏭️ omitted | No new exported types of substance (props interfaces consumed in-construct). |
comment-analyzer |
⏭️ omitted | Comment accuracy covered by hand + code-reviewer/silent-failure (B1's inverted comment, N2). |
/security-review |
⏭️ omitted-with-note | IAM/secrets touched; reviewed the task-role grants directly (B1 is the finding) — a dedicated pass is cheap defense-in-depth before ECS is used in anger. |
Human heuristics
- Proportionality — ✅ Sizing/gating/S3-indirection match documented problems (8 KB cap, OOM history, silent parity drops); not over-built.
- Coherence —
⚠️ Mostly strong (run_task_from_payloadreplaces a drift-prone hand-list with a signature-bound source of truth). But B1's grant breaks the "same concept = same scoping" coherence: the payload bucket is correctly untrusted-code-read-only while the artifacts bucket on the same role is whole-bucket read+write. - Clarity —
⚠️ The B1 comment (ecs-agent-cluster.ts:236-238) actively misleads — it claims AgentCore parity for a grant AgentCore doesn't have. - Appropriateness — ✅ Live-verified on a dev stack (real Fargate run, Linear reaction + PR-link comment), not just mocks.
Bottom line: clear B1 (drop or prefix-scope the artifacts task-role grant + fix the inverted comment — a one-liner that also improves least-privilege on the untrusted-code role) and this is ready. Governance is resolved (#606 approved), the substrate is well-built, and bootstrap coverage checks out. N1/N2/N3 are follow-ups (#608 already tracks N3).
🤖 Generated with Claude Code
| // AgentCore runtime's SessionRole/exec-role has the equivalent). Scoped to | ||
| // this bucket. Stays on the task role — delivery is a terminal step. | ||
| if (props.artifactsBucket) { | ||
| props.artifactsBucket.grantReadWrite(taskRole); |
There was a problem hiding this comment.
Blocking (B1) — least-privilege regression + inverted rationale. This puts whole-bucket s3:GetObject*/PutObject*/DeleteObject* on the ECS task role, which runs untrusted repo code. But the actual artifact write goes through the assumed SessionRole (deliverers.py:197 → tenant_client), which is already scoped to artifacts/${aws:PrincipalTag/task_id}/* (agent-session-role.ts:204-213) and always wired here (agent.ts:632; aws_session.py fails closed if the assume fails). And the AgentCore runtime role gets no artifacts grant at all (agent.ts:423-426) — so the comment's "the AgentCore runtime's SessionRole/exec-role has the equivalent … parity" is inverted. This grant is redundant in production and lets a task read/clobber other tasks' artifacts/<other_id>/ (and traces//attachments/ on the same bucket).
| props.artifactsBucket.grantReadWrite(taskRole); | |
| // #299 ECS-parity: coding/decompose-v1 delivers its plan to the artifacts | |
| // bucket via deliver_artifact — but the write goes through 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). So the task role needs only | |
| // the ARTIFACTS_BUCKET_NAME env (set above), not a bucket grant. Granting | |
| // whole-bucket read+write here would over-privilege the untrusted-code role | |
| // and break cross-task isolation. | |
| // (no props.artifactsBucket grant — intentional; see comment) |
If you truly need a no-SessionRole fallback, scope it instead: props.artifactsBucket.grantWrite(taskRole, 'artifacts/*') (or an explicit arnForObjects('artifacts/*') statement with PutObject/GetObject), never grantReadWrite on the whole bucket. Either way, please correct the comment at :236-238.
| #: Keys not in this set (genuinely foreign) are dropped quietly as before. | ||
| _KNOWN_ORCHESTRATOR_KEYS = frozenset( | ||
| { | ||
| "build_command", |
There was a problem hiding this comment.
Nit (N1). _KNOWN_ORCHESTRATOR_KEYS lists build_command but not its sibling lint_command. Neither is emitted in the payload today, so no runtime effect — but if configurable-verify lands, build_command would drop with the intended WARN breadcrumb while lint_command drops silently, the exact asymmetry this known-key WARN exists to prevent. The test at test_run_task_from_payload.py:240 already iterates both as the same class.
| "build_command", | |
| "build_command", | |
| "lint_command", | |
| "merge_branches", |
…+ 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.)
|
Thanks for the thorough review. Addressed in B1 (least-privilege regression — inline on ecs-agent-cluster.ts): ✅ Removed B1 (dead const): B2 (governance): filed #617 (approved) — 'Make the ECS Fargate compute backend usable', a sibling of #502/#494. Branch rename to N4: added N1/N2/N3: already addressed on the branch head (the sizing comment defers to The three test-seam gaps (deleteEcsPayload-iff-ecs, |
…rror #596 B1) Mirrors the #596 review B1 fix onto linear-vercel (this defect rode in via the main→lv merge a9efaa6, which kept lv's version of the grant). coding/decompose-v1 delivers its plan via the assumed SessionRole (scoped to artifacts/${task_id}/*), exactly like the AgentCore runtime whose task role has no direct artifacts grant. The whole-bucket grantReadWrite over-privileged the untrusted-code role and broke cross-task isolation. Task role keeps only the ARTIFACTS_BUCKET_NAME env; corrected the inverted comment + the stale artifacts clause in the cdk-nag IAM5 reason. Test flipped to assert the task role has NO s3:Put*/Delete* action. Full cdk build green.
3 participants
Summary
Makes the ECS Fargate compute backend actually usable.
mainships the ECSComputeStrategyscaffolding (#8, #494) but the wiring inagent.tsis commented out and several correctness gaps remain. This PR turns it on context-gated (default synth is unchanged — AgentCore stays the default) and fixes the ECS-parity bugs found dogfooding ABCA-on-ABCA and deploying to a dev stack.What changed
Wiring (additive, context-gated):
agent.ts— uncomment + wireEcsAgentCluster(+ the ECS compute strategy: RunTask rejected — payload inlined into 8 KB containerOverrides limit #502 payload bucket from the base PR). Gated on--context compute_type=ecs; the default synth produces 0 ECS resources (verified), somain's template is byte-unchanged for existing deployers. Repos opt in per-repo viacompute_type: 'ecs'.BUILD_VERIFY_TIMEOUT_S=3600.compute_type=ecsguard (repo.ts): the CLI refuses to onboard a repo asecswhen the stack wasn't deployed with the substrate, instead of failing every task later.ECS-parity fixes (the boot path silently dropped data vs AgentCore):
run_taskkwargs and droppedchannel_source/channel_metadata(no Linear/Jira reaction or channel MCP on ECS), pluscedar_policies,attachments,trace,user_id. Replaced withrun_task_from_payload()that maps the whole payload viainspect.signature(unknown keys ignored) — single source of truth, unit-tested.GetSecretValueonbgagent-{linear,jira}-oauth-*so channel OAuth tokens resolve on ECS.bedrock-agentcore:CreateEventon AgentMemory so cross-task learning writes don't silently no-op on ECS.ARTIFACTS_BUCKET_NAMEwired into the task;ec2:DescribeAvailabilityZonesgranted (a CDK target repo'scdk synthbuild gate needs it on a fresh clone); ecs-strategy import made hermetic vs ambient env.corepackenabled in the agent image so yarn/pnpm resolve.Docs: corrected the ECS enable instructions —
ComputeTypesis a CFN template parameter, andcdk bootstrapcan't set one (no--parameters;--contextis CDK construct context), so the working path isaws cloudformation update-stack ... ParameterKey=ComputeTypes,ParameterValue=agentcore,ecs.Testing
mise run buildgreen: CDK 125 suites, agent 80.84% coverage, CLI 48 suites.--context compute_type=ecssynths the ECS cluster + payload bucket; default synth = 0 ECS resources.--context compute_type=ecs): a Linear task on anecs-onboarded repo reacts 👀, runs on Fargate, and posts a PR-link comment; the deployed image was confirmed to carry therun_task_from_payload+ corepack + arm64-native-claude fixes.Notes for reviewers
compute_type). No AgentCore path touched.containerOverrideslimit → S3 pointer).