diff --git a/cdk/src/constructs/ecs-agent-cluster.ts b/cdk/src/constructs/ecs-agent-cluster.ts index f21a3f163..f44d4ad6d 100644 --- a/cdk/src/constructs/ecs-agent-cluster.ts +++ b/cdk/src/constructs/ecs-agent-cluster.ts @@ -27,7 +27,7 @@ import * as logs from 'aws-cdk-lib/aws-logs'; import * as s3 from 'aws-cdk-lib/aws-s3'; import * as secretsmanager from 'aws-cdk-lib/aws-secretsmanager'; import { NagSuppressions } from 'cdk-nag'; -import { Construct } from 'constructs'; +import { Construct, type Node } from 'constructs'; import { AgentMemory } from './agent-memory'; import { AgentSessionRole } from './agent-session-role'; import { resolveBedrockModelIds } from './bedrock-models'; @@ -42,7 +42,14 @@ export interface EcsAgentClusterProps { readonly memoryId?: string; /** - * S3 bucket holding per-task ECS payloads (#502). The orchestrator writes the + * Optional Fargate task sizing overrides. Any unset field uses the generous + * default; a consumer with a lighter repo should shrink the build task to cut + * cost. See {@link EcsTaskSizing}. + */ + readonly taskSizing?: EcsTaskSizing; + + /** + * S3 bucket holding per-task ECS payloads. The orchestrator writes the * payload (incl. the large hydrated_context, which can't fit in the 8 KB * RunTask containerOverrides limit) here and passes only an * `AGENT_PAYLOAD_S3_URI` pointer; the container fetches it on boot. The task @@ -54,46 +61,45 @@ export interface EcsAgentClusterProps { readonly payloadBucket?: s3.IBucket; /** - * Artifacts bucket for repo-bound artifact workflows (#299 coding/decompose-v1 - * emits its plan JSON here via ``deliver_artifact``). The AgentCore runtime - * gets ``ARTIFACTS_BUCKET_NAME`` in its env; the ECS task needs the SAME env - * (but NO bucket grant) or an artifact workflow fails at delivery with - * "ARTIFACTS_BUCKET_NAME is not configured" (live-caught: a :decompose on an - * ecs-configured repo). The delivery WRITE goes through the assumed per-task - * SessionRole (scoped to ``artifacts/${aws:PrincipalTag/task_id}/*``), so the - * task role gets only the env var — parity with the AgentCore runtime role, - * which likewise has no direct artifacts grant (see the grant block below for - * the rationale). + * Artifacts bucket for repo-bound artifact workflows (a planning/decompose + * workflow emits its plan JSON here via ``deliver_artifact``). The AgentCore + * runtime gets ``ARTIFACTS_BUCKET_NAME`` in its env; the ECS task needs the + * SAME env (but NO bucket grant) or an artifact workflow fails at delivery with + * "ARTIFACTS_BUCKET_NAME is not configured". The delivery WRITE goes through + * the assumed per-task SessionRole (scoped to + * ``artifacts/${aws:PrincipalTag/task_id}/*``), so the task role gets only the + * env var — parity with the AgentCore runtime role, which likewise has no + * direct artifacts grant (see the grant block below for the rationale). * * NOTE: this wires only ``ARTIFACTS_BUCKET_NAME`` (artifact delivery). It does * NOT set ``TRACE_ARTIFACTS_BUCKET_NAME`` (telemetry.py reads that for the * ``--trace`` upload), so ``--trace`` silently skips on ECS today — a separate - * ECS-parity gap, not wired here. + * ECS-vs-AgentCore parity gap, not wired here. * Omitted in isolated construct tests → no env/grant. */ readonly artifactsBucket?: s3.IBucket; /** - * Per-task SessionRole (#209). When provided, tenant-data DynamoDB access + * Per-task SessionRole. When provided, tenant-data DynamoDB access * (task/events tables) is NOT granted to the Fargate task role; instead the * agent assumes this SessionRole with session tags and the role's * tag-scoped policy governs that access. The task role is admitted to the * SessionRole's trust and `AGENT_SESSION_ROLE_ARN` is injected into the * container. When omitted (e.g. isolated construct tests), the task role - * retains the legacy direct grants. + * retains the direct grants. */ readonly agentSessionRole?: AgentSessionRole; /** - * AgentCore Memory for cross-task learning (F-2 / ABCA-488-class parity). When - * provided, the ECS task role is granted read+write on it so the agent's - * memory writes (write_task_episode / write_repo_learnings → - * ``bedrock-agentcore:CreateEvent``) succeed on the ECS substrate. The - * AgentCore runtime role already gets this via ``agentMemory.grantReadWrite`` - * in agent.ts; without the same grant here, memory writes hit AccessDenied and - * no-op on ECS (logged, non-fatal — memory.py treats an AccessDenied as an - * infra failure), so learning never persists on an ECS-only deployment. - * Omitted in isolated construct tests / memory-less deployments. + * AgentCore Memory for cross-task learning. When provided, the ECS task role + * is granted read+write on it so the agent's memory writes (write_task_episode + * / write_repo_learnings → ``bedrock-agentcore:CreateEvent``) succeed on the + * ECS substrate. The AgentCore runtime role already gets this via + * ``agentMemory.grantReadWrite`` in agent.ts; without the same grant here, + * memory writes hit AccessDenied and no-op on ECS (logged, non-fatal — + * memory.py treats an AccessDenied as an infra failure), so learning never + * persists on an ECS-only deployment. Omitted in isolated construct tests / + * memory-less deployments. */ readonly agentMemory?: AgentMemory; } @@ -101,9 +107,188 @@ export interface EcsAgentClusterProps { /** HTTPS port — the only egress allowed from the agent task ENIs. */ const HTTPS_PORT = 443; +/** + * Default Fargate task sizes (vCPU units / MiB / GiB). These defaults are + * deliberately MODEST, because a default is what an adopter who changes nothing + * pays for and Fargate bills per requested vCPU-second and RAM-second. Both task + * sizes are overridable via {@link EcsAgentClusterProps.taskSizing}, reachable at + * deploy time through context (see {@link resolveEcsTaskSizing}). + * + * - BUILD task: 4 vCPU / 16 GB / 50 GiB disk. + * + * CPU and memory are modest on measured evidence: a full parallel build of a + * large TypeScript + Python monorepo (agent + CDK + CLI + docs) peaked at + * ~3.1 GB of the 16 GB, because ``MISE_JOBS=1`` serialises the packages so peak + * is max-single-package rather than sum-of-all. Nearly 5x headroom. + * + * Disk is the tighter constraint and is sized less aggressively for that + * reason: the same build peaked at ~14.7 GiB, so Fargate's 21 GiB floor leaves + * only ~1.4x — a heavier dependency cache or a second build sharing the task + * would run it out of space and surface as a spurious build failure. 50 GiB + * restores real margin, and ephemeral storage is a small fraction of the + * per-task cost next to vCPU and RAM. + * + * A repo that genuinely needs more can go to Fargate's ceiling of + * 16 vCPU / 120 GB (and up to 200 GiB of disk) through {@link EcsTaskSizing}. + * Note a memory-heavy build is helped only by more per-task RAM or fewer + * parallel build steps — a build task runs in its own isolated microVM, so + * capping how many tasks run at once does not help. + * - PLANNING task: 2 vCPU / 8 GB / default disk. For read-only workflows that + * clone and read the repo to produce a plan but never build. "Read-only" + * describes the WORKFLOW's behaviour, not a reduced IAM role: both task defs + * are built by one factory and deliberately share a single task role and + * execution role, because splitting them is how a grant silently lands on one + * def and not the other. Do not read the name as a privilege boundary. + */ +// A MODEST default: 4 vCPU / 16 GB, and Fargate's own 20 GiB disk. +// +// Deliberately not the Fargate ceiling. A default is what an adopter who changes +// nothing gets, and at 16 vCPU / 120 GB that is roughly 5x the per-build cost of +// this size in us-east-1 on-demand. Under-provisioning surfaces as a slow or +// OOM-ing build, which is diagnosable and fixable with one prop; over-provisioning +// surfaces as a bill, which is not. A large TypeScript + Python monorepo genuinely +// needs more — raise it through {@link EcsTaskSizing}, up to Fargate's 16 vCPU / +// 120 GB maximum, and raise ephemeral storage with it if concurrent builds run the +// disk out of space. +const DEFAULT_BUILD_TASK_CPU = 4096; +const DEFAULT_BUILD_TASK_MEMORY_MIB = 16384; +const DEFAULT_BUILD_TASK_EPHEMERAL_STORAGE_GIB = 50; +const DEFAULT_PLANNING_TASK_CPU = 2048; +const DEFAULT_PLANNING_TASK_MEMORY_MIB = 8192; + +/** + * Per-task Fargate sizing overrides. Every field is optional; anything left + * unset uses the default above. A consumer with a lighter repo should shrink the + * build task (for example 4 vCPU / 16 GB) to cut cost; a heavy monorepo can keep + * or raise it up to the Fargate ceiling of 16 vCPU / 120 GB. Values are passed + * straight to the Fargate task definition, so they must be a valid Fargate + * cpu/memory combination (see the AWS Fargate docs) — an invalid pair fails at + * synth/deploy, not silently. + */ +export interface EcsTaskSizing { + /** Build task vCPU units (1024 = 1 vCPU). Defaults to 4096 (4 vCPU). */ + readonly buildTaskCpu?: number; + /** Build task memory in MiB. Defaults to 16384 (16 GB). */ + readonly buildTaskMemoryMiB?: number; + /** Build task root-filesystem storage in GiB (21–200). Defaults to 50. */ + readonly buildTaskEphemeralStorageGiB?: number; + /** Planning (read-only) task vCPU units. Defaults to 2048 (2 vCPU). */ + readonly planningTaskCpu?: number; + /** Planning task memory in MiB. Defaults to 8192 (8 GB). */ + readonly planningTaskMemoryMiB?: number; + /** + * Extra environment variables for the BUILD task's container, merged over the + * defaults (a key given here wins). + * + * The platform sets a few build-tool variables that are only meaningful for the + * toolchain a given repo actually uses — a verify timeout, and parallelism caps + * for a mise/jest-shaped build. Those values were measured against one + * monorepo, so they are this deployment's opinion rather than a universal + * default: a repo that uses none of those tools ignores them, and a repo that + * uses mise with plenty of memory will want the serialisation cap raised. Set + * them here rather than editing the construct. + */ + readonly extraBuildEnvironment?: Record; +} + +/** + * Read {@link EcsTaskSizing} out of deploy context, so the sizing and build-tool + * knobs are reachable without editing this file. Returns undefined when nothing + * is set, so the construct's own defaults apply untouched. + * + * Numeric keys are parsed strictly: a malformed value throws at synth rather + * than silently falling back to the default, because "I set the flag and the + * build still OOM'd" is a much worse afternoon than a failed synth. + * + * Reserved platform env keys are REJECTED rather than merged. The container's + * base environment carries load-bearing wiring — table names, the artifacts + * bucket, and ``AGENT_SESSION_ROLE_ARN``, whose absence turns tenant scoping off + * without failing the task. A caller reaching for a build-tool override must not + * be able to unset those by a typo. + */ +/** + * Container env keys the platform owns. A build-tool override must not be able to + * reach these: they carry table names, bucket names and the agent-session role. + * ``AGENT_SESSION_ROLE_ARN`` is the sharp one — when it is absent the agent falls + * back to ambient credentials and per-tenant scoping is silently OFF, which the + * agent's own session module calls its most dangerous failure mode. A typo in an + * override key should fail synth, not disable an isolation control. + */ +const RESERVED_BUILD_ENV_KEYS = new Set([ + 'TASK_TABLE_NAME', + 'TASK_EVENTS_TABLE_NAME', + 'USER_CONCURRENCY_TABLE_NAME', + 'LOG_GROUP_NAME', + 'GITHUB_TOKEN_SECRET_ARN', + 'MEMORY_ID', + 'ECS_PAYLOAD_BUCKET', + 'ARTIFACTS_BUCKET_NAME', + 'AGENT_SESSION_ROLE_ARN', + 'CLAUDE_CODE_USE_BEDROCK', + 'AWS_REGION', +]); + +export function resolveEcsTaskSizing(node: Node): EcsTaskSizing | undefined { + const num = (key: string): number | undefined => { + const raw = node.tryGetContext(key); + if (raw === undefined || raw === null || raw === '') return undefined; + const parsed = typeof raw === 'number' ? raw : Number(raw); + if (!Number.isInteger(parsed) || parsed <= 0) { + throw new Error(`Context '${key}' must be a positive integer; got ${JSON.stringify(raw)}.`); + } + return parsed; + }; + + const rawEnv = node.tryGetContext('ecsExtraBuildEnv'); + let extra: Record | undefined; + if (rawEnv !== undefined && rawEnv !== null && rawEnv !== '') { + const parsed = typeof rawEnv === 'string' ? JSON.parse(rawEnv) : rawEnv; + if (typeof parsed !== 'object' || Array.isArray(parsed)) { + throw new Error("Context 'ecsExtraBuildEnv' must be a JSON object of string values."); + } + for (const [k, v] of Object.entries(parsed as Record)) { + if (typeof v !== 'string') { + throw new Error(`Context 'ecsExtraBuildEnv' value for '${k}' must be a string.`); + } + if (RESERVED_BUILD_ENV_KEYS.has(k)) { + throw new Error( + `Context 'ecsExtraBuildEnv' cannot set '${k}' — it is platform wiring, not a build-tool ` + + 'knob. Overriding it can break the task or silently disable tenant scoping.', + ); + } + } + extra = parsed as Record; + } + + const sizing: EcsTaskSizing = { + ...(num('ecsBuildTaskCpu') !== undefined && { buildTaskCpu: num('ecsBuildTaskCpu') }), + ...(num('ecsBuildTaskMemoryMiB') !== undefined && { buildTaskMemoryMiB: num('ecsBuildTaskMemoryMiB') }), + ...(num('ecsBuildTaskEphemeralStorageGiB') !== undefined && { + buildTaskEphemeralStorageGiB: num('ecsBuildTaskEphemeralStorageGiB'), + }), + ...(num('ecsPlanningTaskCpu') !== undefined && { planningTaskCpu: num('ecsPlanningTaskCpu') }), + ...(num('ecsPlanningTaskMemoryMiB') !== undefined && { planningTaskMemoryMiB: num('ecsPlanningTaskMemoryMiB') }), + ...(extra !== undefined && { extraBuildEnvironment: extra }), + }; + return Object.keys(sizing).length > 0 ? sizing : undefined; +} + export class EcsAgentCluster extends Construct { public readonly cluster: ecs.Cluster; + /** The BUILD task def (default 4 vCPU / 16 GB, raisable to the Fargate ceiling + * of 16 vCPU / 120 GB via {@link EcsTaskSizing}) — for coding workflows that + * run a full CI-parity build. Selected for non-read-only workflows. */ public readonly taskDefinition: ecs.FargateTaskDefinition; + /** + * The smaller read-only PLANNING task def (8 GB / 2 vCPU) — for any read-only + * workflow that clones + reads + emits an artifact but never builds. Same + * image/role/env/grants as the build def (shared task+execution role + a shared + * container spec, so a grant present on one def but missing on the other can't + * silently diverge); the ONLY difference is cpu/mem. The orchestrator selects + * this for read-only workflows on an ECS repo, so planning doesn't + * over-allocate the large build task. + */ + public readonly planningTaskDefinition: ecs.FargateTaskDefinition; public readonly securityGroup: ec2.SecurityGroup; public readonly containerName: string; public readonly taskRoleArn: string; @@ -139,83 +324,157 @@ export class EcsAgentCluster extends Construct { removalPolicy: RemovalPolicy.DESTROY, }); - // Task execution role (used by ECS agent to pull images, write logs) - // CDK creates this automatically via taskDefinition, but we need to - // grant additional permissions to the task role. - - // Fargate task definition. Sized for heavy CI-parity builds (e.g. ABCA's - // own ~2800-test `mise run build` + cdk synth). Sizing history (all - // live-caught dogfooding ABCA-on-ABCA, 2026-06-29): - // - 4 GB / 2 vCPU → OOM-killed even the AgentCore microVM. - // - 32 GB / 8 vCPU → ran ~50 min then OOM-killed (exit 137) at the cap; - // peak working set ~31.6 GB when the root build fans out 4 heavy jobs - // in PARALLEL (agent:quality ‖ cdk:build ‖ cli:build ‖ docs:build), - // each spawning its own worker fleet (jest maxWorkers, pytest, esbuild - // Lambda bundling). 32 GB had no headroom for that concurrent peak. - // - 64 GB / 16 vCPU → still OOM-killed (exit 137) on ABCA-662's baseline - // build: the parallel storm's peak exceeded 64 GB too. The false - // "build_before=broken" that followed is fixed in repo.py, but the build - // itself genuinely needs more RAM. - // - 120 GB / 16 vCPU (current) → the MAX Fargate admits at 16 vCPU (32–120 - // GB in 8 GB steps). If a build OOMs even here, the fix is to cut the - // build's peak parallelism (serialize the mise DAG / cap jest workers), - // not more RAM — there is none. Paired with BUILD_VERIFY_TIMEOUT_S=3600. - this.taskDefinition = new ecs.FargateTaskDefinition(this, 'TaskDef', { - cpu: 16384, - memoryLimitMiB: 122880, - runtimePlatform: { - cpuArchitecture: ecs.CpuArchitecture.ARM64, - operatingSystemFamily: ecs.OperatingSystemFamily.LINUX, - }, + // SHARED task + execution roles for BOTH task defs. The build def and the + // planning def MUST have identical IAM + env, or a token/grant present on one + // def and missing on the other causes a bug that only shows up on whichever + // task type is missing it. Rather than grant twice, we create the roles ONCE + // here and pass the SAME roles to both task defs, and build the container from + // a single shared spec. So there is exactly one place grants/env can be + // edited, and both defs stay in lockstep by construction. + const taskRole = new iam.Role(this, 'TaskRole', { + assumedBy: new iam.ServicePrincipal('ecs-tasks.amazonaws.com'), + }); + const executionRole = new iam.Role(this, 'ExecutionRole', { + assumedBy: new iam.ServicePrincipal('ecs-tasks.amazonaws.com'), + managedPolicies: [ + iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AmazonECSTaskExecutionRolePolicy'), + ], }); - // Container - this.taskDefinition.addContainer(this.containerName, { - image: ecs.ContainerImage.fromDockerImageAsset(props.agentImageAsset), - logging: ecs.LogDrivers.awsLogs({ - logGroup, - streamPrefix: 'agent', + // The container spec shared by both task defs — image, logging, env are + // IDENTICAL; only the enclosing task def's cpu/mem differ. BUILD_VERIFY_TIMEOUT_S + // is a build-tier concern (a read-only planner never runs the post-agent build + // verify), so it's set per-def below, not here. + const baseEnvironment: Record = { + CLAUDE_CODE_USE_BEDROCK: '1', + TASK_TABLE_NAME: props.taskTable.tableName, + TASK_EVENTS_TABLE_NAME: props.taskEventsTable.tableName, + USER_CONCURRENCY_TABLE_NAME: props.userConcurrencyTable.tableName, + LOG_GROUP_NAME: logGroup.logGroupName, + GITHUB_TOKEN_SECRET_ARN: props.githubTokenSecret.secretArn, + ...(props.memoryId && { MEMORY_ID: props.memoryId }), + // The payload bucket name so the orchestrator-issued AGENT_PAYLOAD_S3_URI + // can be fetched. (The orchestrator sets the URI per-task via container + // override; this is set here for parity with the runtime env.) + ...(props.payloadBucket && { ECS_PAYLOAD_BUCKET: props.payloadBucket.bucketName }), + // Artifact workflows (e.g. decompose/planning) deliver their plan JSON to + // this bucket. The AgentCore runtime has ARTIFACTS_BUCKET_NAME; the ECS task + // needs it too or deliver_artifact raises "ARTIFACTS_BUCKET_NAME is not + // configured". + ...(props.artifactsBucket && { ARTIFACTS_BUCKET_NAME: props.artifactsBucket.bucketName }), + // Per-session IAM scoping: when a SessionRole is wired, the agent assumes + // it for tenant-data access (see aws_session.py). + ...(props.agentSessionRole && { + AGENT_SESSION_ROLE_ARN: props.agentSessionRole.role.roleArn, }), - environment: { - CLAUDE_CODE_USE_BEDROCK: '1', - TASK_TABLE_NAME: props.taskTable.tableName, - TASK_EVENTS_TABLE_NAME: props.taskEventsTable.tableName, - USER_CONCURRENCY_TABLE_NAME: props.userConcurrencyTable.tableName, - LOG_GROUP_NAME: logGroup.logGroupName, - GITHUB_TOKEN_SECRET_ARN: props.githubTokenSecret.secretArn, - // Heavy CI-parity builds on this big-box substrate legitimately run - // 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 - // AGENT_PAYLOAD_S3_URI can be fetched. (The orchestrator sets the URI - // per-task via container override; this is informational parity.) - ...(props.payloadBucket && { ECS_PAYLOAD_BUCKET: props.payloadBucket.bucketName }), - // #299 ECS-parity: artifact workflows (coding/decompose-v1) deliver their - // plan JSON to this bucket. The AgentCore runtime has ARTIFACTS_BUCKET_NAME; - // the ECS task needs it too or deliver_artifact raises "ARTIFACTS_BUCKET_NAME - // is not configured" (live-caught on an ecs-repo :decompose). - ...(props.artifactsBucket && { ARTIFACTS_BUCKET_NAME: props.artifactsBucket.bucketName }), - // Per-session IAM scoping (#209): when a SessionRole is wired, the - // agent assumes it for tenant-data access (see aws_session.py). - ...(props.agentSessionRole && { - AGENT_SESSION_ROLE_ARN: props.agentSessionRole.role.roleArn, - }), - }, - }); + }; + const image = ecs.ContainerImage.fromDockerImageAsset(props.agentImageAsset); + const makeTaskDef = ( + taskDefId: string, + cpu: number, + memoryLimitMiB: number, + extraEnv: Record, + ephemeralStorageGiB?: number, + ) => { + const def = new ecs.FargateTaskDefinition(this, taskDefId, { + cpu, + memoryLimitMiB, + taskRole, + executionRole, + // Raise root-fs storage past Fargate's 20 GiB default for build tasks so + // a large clone plus build caches don't run the disk out of space + // mid-build (ENOSPC); omitted → the 20 GiB default. + ...(ephemeralStorageGiB !== undefined && { ephemeralStorageGiB }), + runtimePlatform: { + cpuArchitecture: ecs.CpuArchitecture.ARM64, + operatingSystemFamily: ecs.OperatingSystemFamily.LINUX, + }, + }); + def.addContainer(this.containerName, { + image, + logging: ecs.LogDrivers.awsLogs({ logGroup, streamPrefix: 'agent' }), + environment: { ...baseEnvironment, ...extraEnv }, + }); + return def; + }; - // Task role permissions - const taskRole = this.taskDefinition.taskRole; + // Resolve task sizing: each field falls back to its default when the + // consumer didn't override it. See DEFAULT_BUILD_TASK_* above for why the + // build default is large, and EcsTaskSizing for how to shrink it. + const sizing = props.taskSizing ?? {}; + const buildCpu = sizing.buildTaskCpu ?? DEFAULT_BUILD_TASK_CPU; + const buildMemory = sizing.buildTaskMemoryMiB ?? DEFAULT_BUILD_TASK_MEMORY_MIB; + const buildDisk = sizing.buildTaskEphemeralStorageGiB ?? DEFAULT_BUILD_TASK_EPHEMERAL_STORAGE_GIB; + const planningCpu = sizing.planningTaskCpu ?? DEFAULT_PLANNING_TASK_CPU; + const planningMemory = sizing.planningTaskMemoryMiB ?? DEFAULT_PLANNING_TASK_MEMORY_MIB; - // DynamoDB: when a SessionRole (#209) is wired, tenant-data access lives on - // that tag-scoped role and the task role only needs to assume it. Without - // one (isolated construct tests / legacy), grant the task role directly. + this.taskDefinition = makeTaskDef('TaskDef', buildCpu, buildMemory, { + // Heavy CI-parity builds legitimately run longer than the 1800s default. + BUILD_VERIFY_TIMEOUT_S: '3600', + // Pin the jest test fleet to an ABSOLUTE worker count on ECS. jest's + // `maxWorkers: 25%` is CORE-relative → 4 workers on this 16-vCPU box. + // Measured, the test suite at 4 workers peaks at only ~2.2 GB (whole process + // tree) — not tens of GB. Container OOMs were not driven by this test + // suite's worker count; they were driven by TOTAL concurrency — a + // full-parallel `mise run build` running every package's test/build legs + // plus the resident coding agent all at once. So the real memory driver is + // cross-package build parallelism, not jest's internal workers. 4 is + // comfortably safe on the 120 GB box even alongside the other packages + + // agent. Kept as an explicit env (not core-relative) so a future bigger box + // can't silently over-spawn. The test script reads JEST_MAX_WORKERS (default + // 25%), so this only pins the shared ECS box — CI (2–4 cores) and dev + // machines keep 25%, unaffected. + JEST_MAX_WORKERS: '4', + // Serialize the mise task graph so the build steps' peak memory doesn't sum + // and OOM the task. `mise run build` fans out its `depends` (the per-package + // build/quality legs) up to MISE_JOBS in parallel (default 4); each package + // then spawns its OWN worker fleet (jest, pytest, esbuild, cdk synth). The + // measured memory driver of the OOMs was this CROSS-PACKAGE storm summing on + // top of the resident coding agent — not any single package. At 120 GB + // (Fargate's max at 16 vCPU) there is no more RAM to add, so the remedy is to + // cut peak parallelism. MISE_JOBS=1 runs the packages SEQUENTIALLY → peak ≈ + // max(single package) instead of sum(all packages), while still building + // every package and keeping BOTH gates (baseline + post-agent). + // Within-package parallelism (JEST_MAX_WORKERS=4, pytest) is untouched, so a + // single package still uses the box's cores. Cost is wall-clock (~serial + // sum, still minutes) — trivial against BUILD_VERIFY_TIMEOUT_S=3600. Without + // this, the post-agent build OOM'd (exit 137) stacking on the still-resident + // agent; a gate that OOMs verified NOTHING — serializing lets it actually + // COMPLETE and gate. Only affects `mise run ` (the build legs); the + // agent's direct `uv run pytest` calls are unaffected. + MISE_JOBS: '1', + // Skip the target repo's pre-push TEST hook inside the agent container. + // `mise run install` installs prek git hooks, incl. a pre-push hook that + // re-runs the FULL cdk+cli+agent test suite on every `git push`. In this + // container that suite already ran TWICE (baseline + post-agent build gate) + // and GitHub CI runs it again — so the pre-push run is pure redundancy, AND + // it runs UNcapped (no JEST_MAX_WORKERS), stacking on the resident agent → + // OOM. The agent's only escape was `git push --no-verify`, which silently + // bypassed ALL hooks (incl. the security scan) and trained a + // skip-verification habit. SKIP is the pre-commit/prek standard env var + // (comma-separated hook ids); scoping it to the tests hook lets the push + // succeed WITHOUT --no-verify while KEEPING the pre-push security scan. + // Propagates to both the platform push (post_hooks.py) and the agent's own + // git-tool pushes via shell.py::_clean_env (blacklist — passes SKIP through). + SKIP: 'monorepo-tests-pre-push', + // Caller overrides win: the values above are tuned for one monorepo's + // toolchain, so a deployment with a different build shape replaces them + // through `taskSizing.extraBuildEnvironment` rather than editing this file. + ...(sizing.extraBuildEnvironment ?? {}), + }, buildDisk); + + // PLANNING task def — for read-only workflows that clone + read + emit a plan + // artifact but NEVER build. 8 GB / 2 vCPU: a clone + a bounded set of file + // reads into the model context, no parallel build storm. Same image/roles/env + // as the build def (so channel OAuth, artifact delivery, payload fetch all + // work identically); NO BUILD_VERIFY_TIMEOUT_S (a read-only planner runs no + // build verify). If 8 GB proves tight on a very large clone, 16 GB / 4 vCPU is + // the next step — size up on Container Insights evidence. + this.planningTaskDefinition = makeTaskDef('PlanningTaskDef', planningCpu, planningMemory, {}); + + // DynamoDB: when a SessionRole is wired, tenant-data access lives on that + // tag-scoped role and the task role only needs to assume it. Without one + // (isolated construct tests / no SessionRole), grant the task role directly. if (props.agentSessionRole) { props.agentSessionRole.admitComputeRole(taskRole); } else { @@ -230,18 +489,18 @@ export class EcsAgentCluster extends Construct { // agent assumes the SessionRole — stays on the task role). props.githubTokenSecret.grantRead(taskRole); - // #502: read-only on the ECS payload bucket so the container can fetch its - // payload (AGENT_PAYLOAD_S3_URI) at boot. READ only — the container runs - // untrusted repo code, so it must not be able to write or delete payloads - // (the trusted orchestrator owns write + delete). Stays on the task role - // (read once at startup, before the agent assumes any SessionRole). + // Read-only on the ECS payload bucket so the container can fetch its payload + // (AGENT_PAYLOAD_S3_URI) at boot. READ only — the container runs untrusted + // repo code, so it must not be able to write or delete payloads (the trusted + // orchestrator owns write + delete). Stays on the task role (read once at + // startup, before the agent assumes any SessionRole). if (props.payloadBucket) { props.payloadBucket.grantRead(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 + // Artifact workflows (e.g. decompose/planning) deliver their 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 @@ -250,27 +509,25 @@ export class EcsAgentCluster extends Construct { // artifacts//, traces/, attachments/ on the same bucket). // (no props.artifactsBucket grant — intentional; see comment) - // F-2 (ABCA-488-class parity): grant the task role read+write on the - // AgentCore Memory so the agent's cross-task learning writes - // (write_task_episode / write_repo_learnings → bedrock-agentcore:CreateEvent) - // succeed on ECS. The AgentCore runtime role gets this via - // agentMemory.grantReadWrite(runtime) in agent.ts; without the same grant - // here the writes hit AccessDenied and no-op on the ECS substrate (logged, - // non-fatal), so learning never persists on an ECS-only deployment. + // Grant the task role read+write on the AgentCore Memory so the agent's + // cross-task learning writes (write_task_episode / write_repo_learnings → + // bedrock-agentcore:CreateEvent) succeed on ECS. The AgentCore runtime role + // gets this via agentMemory.grantReadWrite(runtime) in agent.ts; without the + // same grant here the writes hit AccessDenied and no-op on the ECS substrate + // (logged, non-fatal), so learning never persists on an ECS-only deployment. if (props.agentMemory) { props.agentMemory.grantReadWrite(taskRole); } - // ABCA-488: per-workspace Linear/Jira OAuth tokens live in Secrets Manager - // under `bgagent-linear-oauth-*` (written by the CLI at setup). For a + // Per-workspace Linear/Jira OAuth tokens live in Secrets Manager under + // `bgagent-linear-oauth-*` (written by the CLI at setup). For a // Linear/Jira-channel task the agent resolves that token at startup // (config.resolve_linear_api_token / resolve_jira_oauth_token) to fire the // 👀→✅ reaction and drive the channel MCP. The AgentCore runtime role + // orchestrator/fanout/screenshot roles all have this prefix grant; the ECS // task role did NOT, so on ECS the token fetch hit AccessDenied and // reactions/MCP no-op'd — logged by config.py's token resolver, not silent, - // but the channel effect (no 👀→✅, no MCP) is invisible to the user - // (ECS-parity gap, live-caught on ABCA-488). + // but the channel effect (no 👀→✅, no MCP) is invisible to the user. // GetSecretValue only — the container reads the token; the orchestrator owns // refresh/PutSecretValue. taskRole.addToPrincipalPolicy(new iam.PolicyStatement({ @@ -324,17 +581,16 @@ export class EcsAgentCluster extends Construct { resources: bedrockResources, })); - // ECS-parity: a CDK-based target repo's build gate runs `cdk synth`, and a - // stack wired to a concrete env ({account, region}) does a synth-time - // availability-zone context lookup (ec2:DescribeAvailabilityZones). On a - // developer box the gitignored cdk.context.json caches the answer so synth - // is hermetic; the agent clones fresh, so there's no cache and synth fires - // the live lookup. Without this grant the ECS task role hit AccessDenied → - // "Synthesis finished with errors" → a FALSE build-gate failure on code that - // builds fine everywhere else (live-caught on the ABCA fork; same class as - // the ABCA-488 GetSecretValue and F-2 CreateEvent ECS-parity gaps). This is a - // read-only describe with no resource-level scoping in IAM, so Resource:* is - // required (suppressed below); it grants no mutation and no data access. + // A CDK-based target repo's build gate runs `cdk synth`, and a stack wired to + // a concrete env ({account, region}) does a synth-time availability-zone + // context lookup (ec2:DescribeAvailabilityZones). On a developer box the + // gitignored cdk.context.json caches the answer so synth is hermetic; the + // agent clones fresh, so there's no cache and synth fires the live lookup. + // Without this grant the ECS task role hit AccessDenied → "Synthesis finished + // with errors" → a FALSE build-gate failure on code that builds fine + // everywhere else. This is a read-only describe with no resource-level scoping + // in IAM, so Resource:* is required (suppressed below); it grants no mutation + // and no data access. taskRole.addToPrincipalPolicy(new iam.PolicyStatement({ actions: ['ec2:DescribeAvailabilityZones'], resources: ['*'], @@ -343,11 +599,18 @@ export class EcsAgentCluster extends Construct { // CloudWatch Logs write logGroup.grantWrite(taskRole); - // Expose role ARNs for scoped iam:PassRole in the orchestrator + // Expose role ARNs for scoped iam:PassRole in the orchestrator. Both task + // defs share these roles, so one ARN pair covers both defs' PassRole grants. this.taskRoleArn = taskRole.roleArn; - this.executionRoleArn = this.taskDefinition.executionRole!.roleArn; + this.executionRoleArn = executionRole.roleArn; - NagSuppressions.addResourceSuppressions(this.taskDefinition, [ + // cdk-nag suppressions. The task role + execution role are SHARED standalone + // constructs rather than roles auto-created under a single task def, so the + // IAM suppressions must target the ROLES directly — a def-level + // `applyToChildren` suppression no longer reaches them (they're siblings of + // the task defs, not children). ECS2 (container env-vars-not-secrets) still + // belongs on each task def. + NagSuppressions.addResourceSuppressions(taskRole, [ { id: 'AwsSolutions-IAM5', reason: 'DynamoDB index/* wildcards from CDK grantReadWriteData (UserConcurrencyTable, and task tables only when no SessionRole is wired); Secrets Manager wildcards from CDK grantRead (GitHub token) and the bgagent-linear-oauth-*/bgagent-jira-oauth-* prefix grant (ABCA-488 — per-workspace channel OAuth tokens are created by the CLI at setup, name unknown at synth, GetSecretValue only); CloudWatch Logs wildcards from CDK grantWrite; S3 object/* wildcard from CDK grantRead on the ECS payload bucket (read-only, scoped to that bucket — #502). Bedrock InvokeModel is scoped to explicit model/inference-profile ARNs (no wildcard resource). ec2:DescribeAvailabilityZones requires Resource:* (EC2 describe actions have no resource-level scoping) — read-only, no mutation/data access; needed so a CDK target repo\'s `cdk synth` build gate can resolve AZ context on a fresh clone (ECS-parity, no cdk.context.json cache in the container).', @@ -357,6 +620,25 @@ export class EcsAgentCluster extends Construct { reason: 'Environment variables contain table names and configuration, not secrets — GitHub token is fetched from Secrets Manager at runtime', }, ], true); + NagSuppressions.addResourceSuppressions(executionRole, [ + { + id: 'AwsSolutions-IAM4', + reason: 'AmazonECSTaskExecutionRolePolicy is the AWS-recommended managed policy for ECS Fargate task execution (ECR image pull + CloudWatch Logs); shared by both the build and planning task defs.', + }, + { + id: 'AwsSolutions-IAM5', + reason: 'ecr:GetAuthorizationToken requires Resource:* (CDK grantPull for the agent image asset); the remaining ECR pull + CloudWatch Logs wildcards are CDK-generated grants scoped to the image repo and the task log group.', + }, + ], true); + // Same ECS2 posture on BOTH task defs (they share the container spec). + for (const def of [this.taskDefinition, this.planningTaskDefinition]) { + NagSuppressions.addResourceSuppressions(def, [ + { + id: 'AwsSolutions-ECS2', + reason: 'Environment variables contain table names and configuration, not secrets — GitHub token is fetched from Secrets Manager at runtime', + }, + ], true); + } NagSuppressions.addResourceSuppressions(this.cluster, [ { diff --git a/cdk/src/constructs/task-orchestrator.ts b/cdk/src/constructs/task-orchestrator.ts index a638b369f..c0f47a559 100644 --- a/cdk/src/constructs/task-orchestrator.ts +++ b/cdk/src/constructs/task-orchestrator.ts @@ -157,6 +157,21 @@ export interface TaskOrchestratorProps { readonly containerName: string; readonly taskRoleArn: string; readonly executionRoleArn: string; + /** + * The smaller read-only PLANNING task def (see + * docs/design/ECS_RIGHTSIZED_PLANNING.md). The ECS strategy selects it for + * read-only workflows so planning doesn't run on the larger build box. + * + * Required, like its siblings, so the all-or-nothing constraint stays visible + * at the type level: the strategy reads this ARN from an env var, and an + * ecsConfig that omitted it would compile but leave the planning def defined + * and permanently unreachable. + * + * Needs no extra IAM — it shares the build def's task and execution roles, + * and the `ecs:RunTask` grant below is scoped by `ecs:cluster` rather than by + * task-definition ARN, so it already covers every def in the cluster. + */ + readonly planningTaskDefinitionArn: string; }; /** @@ -272,6 +287,11 @@ export class TaskOrchestrator extends Construct { ECS_SUBNETS: props.ecsConfig.subnets, ECS_SECURITY_GROUP: props.ecsConfig.securityGroup, ECS_CONTAINER_NAME: props.ecsConfig.containerName, + // Read-only workflows route here instead of the build def. Without this + // var the strategy's `readOnly && ECS_PLANNING_TASK_DEFINITION_ARN` + // guard is always falsy, so the planning def would be synthesized and + // never used. + ECS_PLANNING_TASK_DEFINITION_ARN: props.ecsConfig.planningTaskDefinitionArn, }), // #502: bucket the orchestrator writes the ECS payload to (and deletes // from at finalize); the ECS strategy reads this to build the S3 URI. diff --git a/cdk/src/handlers/orchestrate-task.ts b/cdk/src/handlers/orchestrate-task.ts index d19142827..56adafa5b 100644 --- a/cdk/src/handlers/orchestrate-task.ts +++ b/cdk/src/handlers/orchestrate-task.ts @@ -61,10 +61,10 @@ const durableHandler: DurableExecutionHandler = asyn return loadTask(taskId); }); - // Correlation envelope (#245): stamp {task_id, user_id, repo} on every - // lifecycle log line so admission→terminal transitions join to TaskEvents - // and the agent trace without hand-adding fields per call. `repo` is - // undefined for repo-less workflows (#248 Phase 3). + // Correlation envelope: stamp {task_id, user_id, repo} on every lifecycle log + // line so admission→terminal transitions join to TaskEvents and the agent + // trace without hand-adding fields per call. `repo` is undefined for repo-less + // workflows (those with no target repository). const { log, correlation } = envelopeFor(task); // Step 1b: Load blueprint config (per-repo overrides) @@ -169,13 +169,16 @@ const durableHandler: DurableExecutionHandler = asyn userId: task.user_id, payload, blueprintConfig, + // A read-only workflow (e.g. planning that only clones and reads the + // repo) runs on the smaller ECS planning task def. Ignored by AgentCore. + readOnly: workflowIsReadOnly(task.resolved_workflow?.id ?? 'coding/new-task-v1'), }; // Transient-error AUTO-RETRY (once), extracted to startSessionWithRetry so - // the four branches are unit-tested (#599 B2). The retry-event emit is - // best-effort and guarded there (#599 B1): a TaskEvents PutItem fault can't - // abort or mis-attribute the retry. The correlation envelope is threaded so - // the session_start_retry event carries the same trace context as - // session_started below (#245). + // its branches are unit-tested. The retry-event emit is best-effort and + // guarded there: a TaskEvents PutItem fault can't abort or mis-attribute + // the retry. The correlation envelope is threaded so the + // session_start_retry event carries the same trace context as + // session_started below. const { handle, autoRetried: retried } = await startSessionWithRetry( strategy, startInput, @@ -213,16 +216,14 @@ const durableHandler: DurableExecutionHandler = asyn return handle; } catch (err) { // Carry the auto-retry fact into error_message: the `[auto-retried]` suffix - // is persisted verbatim (the classifier ignores it — it does not affect - // classification). It is a breadcrumb for a FORTHCOMING failure renderer to - // detect and surface "I already tried again" to the channel — no consumer - // renders it yet on this branch (retryGuidance() in error-classifier.ts is - // the intended copy source; it ships ahead of its consumer). + // is persisted verbatim (the error classifier ignores it — it does not + // affect classification). It is a breadcrumb for a failure renderer to + // detect and surface "I already tried again" to the channel. // - // Stamp on BOTH paths a retry ran (#599 N1): branch 3 sets `autoRetried` - // above then a LATER step throws; branch 4 (transient-then-transient) throws - // FROM startSessionWithRetry before `autoRetried` is assigned, so the local - // is still false — read the fact off the thrown error via isAutoRetried(). + // Stamp on BOTH paths where a retry ran: one path sets `autoRetried` above + // then a LATER step throws; the transient-then-transient path throws FROM + // startSessionWithRetry before `autoRetried` is assigned, so the local is + // still false — read the fact off the thrown error via isAutoRetried(). // Without this, a double-transient failure was told "reply to retry" instead // of "I already retried" — the exact confusion the marker exists to prevent. const retriedNote = (autoRetried || isAutoRetried(err)) ? ' [auto-retried]' : ''; @@ -330,11 +331,11 @@ const durableHandler: DurableExecutionHandler = asyn // Step 6: Finalize — update terminal status, emit events, release concurrency await context.step('finalize', async () => { await finalizeTask(taskId, finalPollState, task.user_id); - // #502: the task is terminal — the container has long since read its - // payload, so delete the ephemeral S3 payload object now. Best-effort - // (deleteEcsPayload swallows errors) and a no-op for AgentCore tasks / - // deployments without a payload bucket; the bucket's 1-day lifecycle rule - // is the backstop if this delete or the whole step never runs. + // The task is terminal — the container has long since read its payload, so + // delete the ephemeral S3 payload object now. Best-effort (deleteEcsPayload + // swallows errors) and a no-op for AgentCore tasks / deployments without a + // payload bucket; the bucket's 1-day lifecycle rule is the backstop if this + // delete or the whole step never runs. if (blueprintConfig.compute_type === 'ecs') { await deleteEcsPayload(taskId); } diff --git a/cdk/src/handlers/shared/compute-strategy.ts b/cdk/src/handlers/shared/compute-strategy.ts index c3de58869..5b352963f 100644 --- a/cdk/src/handlers/shared/compute-strategy.ts +++ b/cdk/src/handlers/shared/compute-strategy.ts @@ -48,6 +48,15 @@ export interface ComputeStrategy { userId: string; payload: Record; blueprintConfig: BlueprintConfig; + /** + * #299 ECS_RIGHTSIZED_PLANNING: true for a read-only workflow (e.g. + * coding/decompose-v1) that clones + reads + emits an artifact but never + * builds. The ECS strategy uses it to pick the smaller planning task def + * instead of the larger build def. AgentCore ignores it (its microVM is a + * single fixed size). Optional so callers/tests that omit it default to the + * build def (never worse than today). + */ + readOnly?: boolean; }): Promise; pollSession(handle: SessionHandle): Promise; stopSession(handle: SessionHandle): Promise; diff --git a/cdk/src/handlers/shared/orchestrator.ts b/cdk/src/handlers/shared/orchestrator.ts index 7308c4532..2e41cf94a 100644 --- a/cdk/src/handlers/shared/orchestrator.ts +++ b/cdk/src/handlers/shared/orchestrator.ts @@ -168,16 +168,16 @@ export async function transitionTask( })); } -/** Correlation envelope stamped on orchestrator-plane task events (#245). */ +/** Correlation envelope stamped on orchestrator-plane task events. */ export interface EventCorrelation { readonly user_id?: string; - /** `owner/repo`, or absent for repo-less workflows (#248 Phase 3). Matches - * the source `TaskRecord.repo` (`string | undefined`). */ + /** `owner/repo`, or absent for repo-less workflows (those with no target + * repository). Matches the source `TaskRecord.repo` (`string | undefined`). */ readonly repo?: string; } /** - * Build the correlation envelope (#245) for a task from its identity fields: + * Build the correlation envelope for a task from its identity fields: * a `log` child stamping `{task_id, user_id, repo}` on every line, and the * matching `correlation` for `emitTaskEvent`. Single source so the log context * and the event envelope can't drift. `repo` is omitted (not null/empty) for @@ -199,8 +199,8 @@ export function envelopeFor( * @param eventType - the event type string. * @param metadata - optional event metadata. * @param correlation - optional `{ user_id, repo }` stamped as top-level fields - * so the event stream joins to orchestrator logs by the correlation envelope - * (#245). `repo` is omitted when null/absent (repo-less workflows). + * so the event stream joins to orchestrator logs by the correlation envelope. + * `repo` is omitted when null/absent (repo-less workflows). */ export async function emitTaskEvent( taskId: string, @@ -234,12 +234,13 @@ const MAX_POLL_INTERVAL_MS = 300_000; * @returns the merged blueprint config. */ export async function loadBlueprintConfig(task: TaskRecord): Promise { - // Correlation envelope (#245) on this shared function's logs too, matching the + // Correlation envelope on this shared function's logs too, matching the // orchestrate handler's child logger. `repo` omitted for repo-less workflows. const { log } = envelopeFor(task); - // Repo-less workflows (#248 Phase 3) have no per-repo Blueprint — use platform - // defaults directly rather than a RepoTable lookup on a missing repo. + // Repo-less workflows (those with no target repository) have no per-repo + // Blueprint — use platform defaults directly rather than a RepoTable lookup + // on a missing repo. const repoConfig = task.repo ? await loadRepoConfig(task.repo) : null; if (repoConfig) { @@ -268,6 +269,17 @@ export async function loadBlueprintConfig(task: TaskRecord): Promise/.jsonl.gz`` (design §10.1) and the - // handler's per-caller-prefix guard relies on the agent landing - // under the submitter's prefix. Threaded unconditionally so - // scripts that inspect the payload can always see it; costs one + // ``traces//.jsonl.gz`` and the handler's + // per-caller-prefix guard relies on the agent landing under the + // submitter's prefix. Threaded unconditionally so scripts that + // inspect the payload can always see it; costs one // Cognito-sub-sized string in the JSON. user_id: task.user_id, branch_name: hydratedContext.resolved_branch_name ?? task.branch_name, @@ -539,6 +553,15 @@ export async function hydrateAndTransition(task: TaskRecord, blueprintConfig?: B resolved_workflow: task.resolved_workflow ?? { id: 'coding/new-task-v1', version: '1.0.0' }, ...(task.pr_number !== undefined && { pr_number: task.pr_number }), ...(hydratedContext.resolved_base_branch && { base_branch: hydratedContext.resolved_base_branch }), + // Orchestration children carry their stacked base branch + (diamond + // dependency case) predecessor branches to merge in, via channel_metadata. + // The PR-task ``resolved_base_branch`` path above wins if both are set + // (a task is never both a PR-iteration and an orchestration child). + ...(!hydratedContext.resolved_base_branch + && task.channel_metadata?.orchestration_base_branch + && { base_branch: task.channel_metadata.orchestration_base_branch }), + ...(task.channel_metadata?.orchestration_merge_branches + && { merge_branches: parseMergeBranches(task.channel_metadata.orchestration_merge_branches) }), ...(task.task_description && { prompt: task.task_description }), max_turns: task.max_turns ?? blueprintConfig?.max_turns ?? DEFAULT_MAX_TURNS, ...(effectiveBudget !== undefined && { max_budget_usd: effectiveBudget }), @@ -548,37 +571,42 @@ export async function hydrateAndTransition(task: TaskRecord, blueprintConfig?: B ...(task.trace === true && { trace: true }), ...(blueprintConfig?.model_id && { model_id: blueprintConfig.model_id }), ...(blueprintConfig?.system_prompt_overrides && { system_prompt_overrides: blueprintConfig.system_prompt_overrides }), + // Per-repo build/lint verification commands. Absent → agent defaults + // to ``mise run build`` / ``mise run lint``. Set for non-mise repos so + // build-regression gating actually runs the repo's real command. + ...(blueprintConfig?.build_command && { build_command: blueprintConfig.build_command }), + ...(blueprintConfig?.lint_command && { lint_command: blueprintConfig.lint_command }), ...(blueprintConfig?.cedar_policies && blueprintConfig.cedar_policies.length > 0 && { cedar_policies: blueprintConfig.cedar_policies }), - // Cedar HITL: the agent's PreToolUse hook uses this to compute - // the maxLifetime ceiling on per-gate approval timeouts (§6.5). + // The agent's PreToolUse hook uses this to compute the maxLifetime + // ceiling on per-gate human-in-the-loop approval timeouts. // Stamped at HYDRATING → RUNNING transition time so the clock // only starts when the container is alive. Format is ISO 8601 // UTC to match the rest of the TaskRecord timestamp fields. task_started_at: new Date().toISOString(), - // Cedar HITL pre-approval data (§7.3). Threaded so the agent's + // Human-in-the-loop pre-approval data. Threaded so the agent's // PolicyEngine can seed ApprovalAllowlist + set // task_default_timeout_s without a second DDB round-trip. ...(task.approval_timeout_s !== undefined && { approval_timeout_s: task.approval_timeout_s }), ...(task.initial_approvals && task.initial_approvals.length > 0 && { initial_approvals: task.initial_approvals }), - // Cedar HITL Chunk 7 (§13.6): seed the engine's per-task gate - // counter from the TaskTable-persisted value so a container - // restart mid-task resumes the cumulative gate budget instead of - // resetting to 0. Only forwarded when non-zero so the agent's - // default (0) path remains unchanged for fresh tasks; the - // TaskRecord is already loaded above, so no extra DDB read. + // Seed the engine's per-task approval-gate counter from the + // TaskTable-persisted value so a container restart mid-task resumes + // the cumulative gate budget instead of resetting to 0. Only + // forwarded when non-zero so the agent's default (0) path remains + // unchanged for fresh tasks; the TaskRecord is already loaded above, + // so no extra DDB read. ...(typeof task.approval_gate_count === 'number' && task.approval_gate_count > 0 && { initial_approval_gate_count: task.approval_gate_count, }), - // Cedar HITL Chunk 7b (§4 step 5, decision #13): thread the - // TaskRecord-persisted cap so the agent's PolicyEngine adopts the - // blueprint-configured value (or the platform default of 50 frozen - // at submit-time) instead of its compile-time fallback. Legacy task - // records predating Chunk 7b omit the field — the agent then falls - // back to its own default of 50. Extra guards past ``typeof`` - // because a hand-edited DDB row could carry NaN, Infinity, a float, - // or an out-of-bounds value — all would crash PolicyEngine.__init__ - // downstream; omitting here keeps the container starting cleanly - // with the engine default and leaves an operator-visible warning. + // Thread the TaskRecord-persisted approval-gate cap so the agent's + // PolicyEngine adopts the blueprint-configured value (or the platform + // default of 50 frozen at submit-time) instead of its compile-time + // fallback. Older task records that predate this field omit it — the + // agent then falls back to its own default of 50. Extra guards past + // ``typeof`` because a hand-edited DDB row could carry NaN, Infinity, + // a float, or an out-of-bounds value — all would crash + // PolicyEngine.__init__ downstream; omitting here keeps the container + // starting cleanly with the engine default and leaves an + // operator-visible warning. ...(isValidApprovalGateCap(task.approval_gate_cap) && { approval_gate_cap: task.approval_gate_cap }), prompt_version: promptVersion, @@ -688,8 +716,8 @@ export async function finalizeTask( ): Promise { const task = await loadTask(taskId); const currentStatus = task.status; - // Correlation envelope (#245) on this function's own log lines too, not just - // the events it emits — admission→terminal logs must join by {user_id, repo}. + // Correlation envelope on this function's own log lines too, not just the + // events it emits — admission→terminal logs must join by {user_id, repo}. const { log, correlation } = envelopeFor(task); // Lost session: RUNNING but agent heartbeats stopped (crash/OOM) — fail fast @@ -774,7 +802,7 @@ export async function finalizeTask( logger, ); // Memory actorId: repo for coding tasks, user:{user_id} for repo-less - // workflows (#248 Phase 3, ADR-014 addendum 2026-06-08). + // workflows (those with no target repository). const actorNamespace = task.repo ?? `user:${task.user_id}`; const written = await writeMinimalEpisode( MEMORY_ID, @@ -804,9 +832,9 @@ export async function finalizeTask( // If still RUNNING / FINALIZING / AWAITING_APPROVAL after the poll // window closes, transition to TIMED_OUT. AWAITING_APPROVAL uses the - // same transition — the stranded-approval reconciler (Chunk 5, - // §13.6) is a secondary safety net with a longer timeout for tasks - // the orchestrator already lost track of. + // same transition — the stranded-approval reconciler is a secondary + // safety net with a longer timeout for tasks the orchestrator already + // lost track of. if ( currentStatus === TaskStatus.RUNNING || currentStatus === TaskStatus.FINALIZING @@ -866,7 +894,7 @@ export async function finalizeTask( * @param userId - the user who owns the task. * @param releaseConcurrency - whether to decrement the concurrency counter. * @param repo - optional target repo (`owner/repo`) for the correlation - * envelope (#245); omit for repo-less workflows. + * envelope; omit for repo-less workflows. */ export async function failTask( taskId: string, @@ -925,3 +953,26 @@ async function decrementConcurrency(userId: string): Promise { } } } + +/** + * Parse the JSON-encoded predecessor merge-branch list that the + * orchestration release path stashes in + * ``channel_metadata.orchestration_merge_branches`` (the diamond-dependency + * case, where a child depends on more than one predecessor). Best-effort: a + * malformed value yields an empty list rather than failing the orchestration — + * the child still branches off its base, it just won't have the predecessor + * code merged in (surfaced as a normal build failure if it actually needed it, + * never a silent crash here). + */ +function parseMergeBranches(raw: string): string[] { + try { + const parsed = JSON.parse(raw) as unknown; + if (Array.isArray(parsed) && parsed.every((b) => typeof b === 'string')) { + return parsed as string[]; + } + } catch { + // fall through + } + logger.warn('Ignoring malformed orchestration_merge_branches', { raw }); + return []; +} diff --git a/cdk/src/handlers/shared/strategies/ecs-strategy.ts b/cdk/src/handlers/shared/strategies/ecs-strategy.ts index a45ef1290..c685d94f6 100644 --- a/cdk/src/handlers/shared/strategies/ecs-strategy.ts +++ b/cdk/src/handlers/shared/strategies/ecs-strategy.ts @@ -22,6 +22,7 @@ import { S3Client, PutObjectCommand, DeleteObjectCommand } from '@aws-sdk/client import type { ComputeStrategy, SessionHandle, SessionStatus } from '../compute-strategy'; import { logger } from '../logger'; import type { BlueprintConfig } from '../repo-config'; +import { DEFAULT_MAX_TURNS } from '../validation'; let sharedClient: ECSClient | undefined; function getClient(): ECSClient { @@ -41,7 +42,39 @@ function getS3Client(): S3Client { const ECS_CLUSTER_ARN = process.env.ECS_CLUSTER_ARN; const ECS_TASK_DEFINITION_ARN = process.env.ECS_TASK_DEFINITION_ARN; +/** + * The smaller, read-only "planning" task definition. Read-only workflows (which + * clone and read the repo to produce a plan but never build) run here instead of + * on the large build task. Falls back to the build task definition when unset + * (e.g. a deployment that hasn't provisioned the planning task) — never worse + * than running everything on the build task. + */ +const ECS_PLANNING_TASK_DEFINITION_ARN = process.env.ECS_PLANNING_TASK_DEFINITION_ARN; const ECS_SUBNETS = process.env.ECS_SUBNETS; + +/** + * Reduce a task-definition reference to its FAMILY (drop the `:revision` suffix), + * so `RunTask` always resolves the LATEST ACTIVE revision instead of a pinned one. + * + * Why: the orchestrator env carries a revision-pinned ARN + * (`…:task-definition/:`, from `taskDefinition.taskDefinitionArn`). + * Every deploy that rebuilds the agent image registers a NEW revision and CDK/ECS + * deregisters the old one. A task dispatched against the now-stale pinned revision + * (e.g. minutes after a deploy) fails at RunTask with + * "InvalidParameterException: TaskDefinition is inactive". ECS accepts `family` + * (bare, no revision) and resolves it to the latest ACTIVE revision at call time, + * which is immune to that deploy race. We accept either a full ARN + * (`arn:aws:ecs:…:task-definition/:`) or a plain `:` and + * return just ``; a value with no `/` and no `:` is returned unchanged. + */ +export function toTaskDefinitionFamily(ref: string): string { + // Take the segment after `task-definition/` when it's a full ARN, else the whole + // value; then strip a trailing `:` revision suffix. + const afterSlash = ref.includes('task-definition/') + ? ref.slice(ref.lastIndexOf('task-definition/') + 'task-definition/'.length) + : ref; + return afterSlash.replace(/:\d+$/, ''); +} const ECS_SECURITY_GROUP = process.env.ECS_SECURITY_GROUP; const ECS_CONTAINER_NAME = process.env.ECS_CONTAINER_NAME ?? 'AgentContainer'; const ECS_PAYLOAD_BUCKET = process.env.ECS_PAYLOAD_BUCKET; @@ -50,7 +83,7 @@ const ECS_PAYLOAD_BUCKET = process.env.ECS_PAYLOAD_BUCKET; * Inline-payload size (bytes) above which we warn that RunTask will likely * reject the call when no payload bucket is configured. ECS caps the TOTAL * containerOverrides blob at 8192 bytes; the other env vars + command consume - * some of that, so 6 KB of payload is the practical danger line (#502). + * some of that, so 6 KB of payload is the practical danger line. */ const INLINE_PAYLOAD_WARN_BYTES = 6144; @@ -96,6 +129,7 @@ export class EcsComputeStrategy implements ComputeStrategy { userId: string; payload: Record; blueprintConfig: BlueprintConfig; + readOnly?: boolean; }): Promise { if (!ECS_CLUSTER_ARN || !ECS_TASK_DEFINITION_ARN || !ECS_SUBNETS || !ECS_SECURITY_GROUP) { // Config/deploy mismatch: this repo is compute_type=ecs but the stack was @@ -113,7 +147,22 @@ export class EcsComputeStrategy implements ComputeStrategy { } const subnets = ECS_SUBNETS.split(',').map(s => s.trim()).filter(Boolean); - const { taskId, payload, blueprintConfig } = input; + const { taskId, payload, blueprintConfig, readOnly } = input; + + // A read-only workflow (e.g. planning that only clones and reads the repo) + // runs on the smaller planning task def when it's wired; everything else runs + // on the large build def. Falls back to the build def if the planning def + // isn't configured — safe, just runs planning on the build-sized task. + const taskDefinitionRef = readOnly && ECS_PLANNING_TASK_DEFINITION_ARN + ? ECS_PLANNING_TASK_DEFINITION_ARN + : ECS_TASK_DEFINITION_ARN; + // Dispatch against the task-def FAMILY (not the pinned revision) so ECS + // resolves the latest ACTIVE revision at call time. A deploy that rebuilds the + // agent image registers a new revision + deregisters the old one; a task + // dispatched minutes after a deploy against the stale pinned revision fails + // with "InvalidParameterException: TaskDefinition is inactive". Using the + // family is immune to that deploy race. + const taskDefinition = toTaskDefinitionFamily(taskDefinitionRef); // The ECS container's default CMD starts the FastAPI server (uvicorn) which // waits for HTTP POST to /invocations — but in standalone ECS nobody sends @@ -122,7 +171,7 @@ export class EcsComputeStrategy implements ComputeStrategy { // This avoids the server entirely and runs the agent in batch mode. const payloadJson = JSON.stringify(payload); - // #502: the payload (esp. hydrated_context) routinely exceeds the 8192-byte + // The payload (especially hydrated_context) routinely exceeds the 8192-byte // cap that ECS RunTask enforces on the TOTAL containerOverrides blob, which // rejected the call with InvalidParameterException. Write the payload to S3 // and pass only a small pointer (AGENT_PAYLOAD_S3_URI); the container fetches @@ -158,14 +207,18 @@ export class EcsComputeStrategy implements ComputeStrategy { { name: 'REPO_URL', value: String(payload.repo_url ?? '') }, ...(payload.prompt ? [{ name: 'TASK_DESCRIPTION', value: String(payload.prompt) }] : []), ...(payload.issue_number ? [{ name: 'ISSUE_NUMBER', value: String(payload.issue_number) }] : []), - { name: 'MAX_TURNS', value: String(payload.max_turns ?? 100) }, + // Single source of truth with the hydrate path in `orchestrator.ts`, which + // resolves the same default via `DEFAULT_MAX_TURNS`. A literal here would + // silently disagree with it whenever the payload omits `max_turns`, and a + // turn ceiling that depends on which code path filled it in is a bug. + { name: 'MAX_TURNS', value: String(payload.max_turns ?? DEFAULT_MAX_TURNS) }, ...(payload.max_budget_usd !== undefined ? [{ name: 'MAX_BUDGET_USD', value: String(payload.max_budget_usd) }] : []), ...(blueprintConfig.model_id ? [{ name: 'ANTHROPIC_MODEL', value: blueprintConfig.model_id }] : []), ...(blueprintConfig.system_prompt_overrides ? [{ name: 'SYSTEM_PROMPT_OVERRIDES', value: blueprintConfig.system_prompt_overrides }] : []), { name: 'CLAUDE_CODE_USE_BEDROCK', value: '1' }, - // #502: prefer the S3 pointer; fall back to the inline payload when no - // bucket is configured (keeps small-payload / AgentCore-only deployments - // working with no behavior change). + // Prefer the S3 pointer; fall back to the inline payload when no bucket is + // configured (keeps small-payload / AgentCore-only deployments working with + // no behavior change). ...(payloadS3Uri ? [{ name: 'AGENT_PAYLOAD_S3_URI', value: payloadS3Uri }] : [{ name: 'AGENT_PAYLOAD', value: payloadJson }]), @@ -181,11 +234,12 @@ export class EcsComputeStrategy implements ComputeStrategy { // 2. Calls entrypoint.run_task_from_payload(p), which maps the WHOLE payload // dict to run_task's signature (rename prompt→task_description / // model_id→anthropic_model, filter to accepted params, coerce str/int). - // This replaces the old hand-listed kwarg subset that silently dropped - // channel_source/channel_metadata (no Linear/Jira reactions or channel - // MCP on ECS — ABCA-487), build_command, cedar_policies, base_branch/ - // merge_branches, attachments, trace, user_id, etc. Single source of - // truth in the agent, unit-tested (see test_run_task_from_payload). + // This replaces an older hand-listed kwarg subset that silently dropped + // fields such as channel_source/channel_metadata (which meant no + // Linear/Jira reactions or channel MCP on ECS), build_command, + // cedar_policies, base_branch/merge_branches, attachments, trace, user_id, + // etc. Single source of truth in the agent, unit-tested (see + // test_run_task_from_payload). // 3. Exits with code 0 on success, 1 on failure. // This bypasses the uvicorn server entirely — no HTTP, no OTEL noise. const bootCommand = [ @@ -205,8 +259,17 @@ export class EcsComputeStrategy implements ComputeStrategy { const command = new RunTaskCommand({ cluster: ECS_CLUSTER_ARN, - taskDefinition: ECS_TASK_DEFINITION_ARN, + taskDefinition, launchType: 'FARGATE', + // ECS RunTask idempotency. Without a clientToken, a client-side timeout on a + // RunTask that ACTUALLY launched (the SDK send() threw after AWS accepted it) + // makes the session-start auto-retry fire a SECOND RunTask with the same + // TASK_ID env — two Fargate containers cloning/committing/PRing in parallel, + // the first untrackable. clientToken makes AWS itself dedup an identical + // RunTask within its window: the retry returns the SAME task instead of + // launching another. taskId is a ULID (26 chars, well under the 64-char + // clientToken limit) and unique per task — the natural token. + clientToken: taskId, networkConfiguration: { awsvpcConfiguration: { subnets, @@ -235,6 +298,9 @@ export class EcsComputeStrategy implements ComputeStrategy { task_id: taskId, ecs_task_arn: ecsTask.taskArn, cluster: ECS_CLUSTER_ARN, + // Which task def was selected — planning (read-only) vs build. + task_definition: taskDefinition, + read_only: Boolean(readOnly), }); return { diff --git a/cdk/src/stacks/agent.ts b/cdk/src/stacks/agent.ts index 06a98deb3..048b596c0 100644 --- a/cdk/src/stacks/agent.ts +++ b/cdk/src/stacks/agent.ts @@ -40,7 +40,7 @@ import { Blueprint } from '../constructs/blueprint'; import { CedarWasmLayer } from '../constructs/cedar-wasm-layer'; import { ConcurrencyReconciler } from '../constructs/concurrency-reconciler'; import { DnsFirewall } from '../constructs/dns-firewall'; -import { EcsAgentCluster } from '../constructs/ecs-agent-cluster'; +import { EcsAgentCluster, resolveEcsTaskSizing } from '../constructs/ecs-agent-cluster'; import { EcsPayloadBucket } from '../constructs/ecs-payload-bucket'; import { FanOutConsumer } from '../constructs/fanout-consumer'; import { GitHubScreenshotIntegration } from '../constructs/github-screenshot-integration'; @@ -588,7 +588,8 @@ export class AgentStack extends Stack { // K12 (2026-06-29): AgentCore's fixed microVM envelope OOM-kills heavy // CI-parity builds (ABCA's own ~2800-test `mise run build`). ECS Fargate // gives a bigger, tunable task (see EcsAgentCluster for the exact vCPU/memory - // sizing + its OOM history — 64 GB was itself OOM-killed, so it runs larger) + // sizing and the measurements behind it — a 32 GB task was OOM-killed by a + // fully parallel build, which is why the build tier serialises with MISE_JOBS=1) // for repos that set ``compute_type: 'ecs'``. GATED on the ``compute_type`` deploy context // (default 'agentcore') — ECS resources only synthesize when you deploy with // ``--context compute_type=ecs``, so the default synth (and the @@ -610,8 +611,18 @@ export class AgentStack extends Stack { }, ]); } + // ECS build-task sizing, from deploy context. The construct's defaults are + // deliberately modest so an adopter who changes nothing does not pay for the + // Fargate ceiling — but a large monorepo genuinely needs more, so the knobs + // have to be reachable WITHOUT editing the construct. Same shape as + // ``compute_type`` above: + // cdk deploy -c compute_type=ecs -c ecsBuildTaskCpu=16384 \ + // -c ecsBuildTaskMemoryMiB=122880 -c ecsBuildTaskEphemeralStorageGiB=100 + // cdk deploy -c ecsExtraBuildEnv='{"MISE_JOBS":"8"}' + const ecsTaskSizing = resolveEcsTaskSizing(this.node); const ecsCluster = computeType === 'ecs' ? new EcsAgentCluster(this, 'EcsAgentCluster', { + ...(ecsTaskSizing !== undefined && { taskSizing: ecsTaskSizing }), vpc: agentVpc.vpc, agentImageAsset: new ecr_assets.DockerImageAsset(this, 'AgentImage', { directory: repoRoot, @@ -673,6 +684,7 @@ export class AgentStack extends Stack { ecsConfig: { clusterArn: ecsCluster.cluster.clusterArn, taskDefinitionArn: ecsCluster.taskDefinition.taskDefinitionArn, + planningTaskDefinitionArn: ecsCluster.planningTaskDefinition.taskDefinitionArn, subnets: agentVpc.vpc.selectSubnets({ subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS }).subnetIds.join(','), securityGroup: ecsCluster.securityGroup.securityGroupId, containerName: ecsCluster.containerName, diff --git a/cdk/test/constructs/ecs-agent-cluster.test.ts b/cdk/test/constructs/ecs-agent-cluster.test.ts index 929e8c890..707ce17a6 100644 --- a/cdk/test/constructs/ecs-agent-cluster.test.ts +++ b/cdk/test/constructs/ecs-agent-cluster.test.ts @@ -28,9 +28,21 @@ import * as s3 from 'aws-cdk-lib/aws-s3'; import * as secretsmanager from 'aws-cdk-lib/aws-secretsmanager'; import { AgentMemory } from '../../src/constructs/agent-memory'; import { AgentSessionRole } from '../../src/constructs/agent-session-role'; -import { EcsAgentCluster } from '../../src/constructs/ecs-agent-cluster'; - -function createStack(overrides?: { memoryId?: string; bedrockModels?: string[] }): { stack: Stack; template: Template } { +import { EcsAgentCluster, resolveEcsTaskSizing } from '../../src/constructs/ecs-agent-cluster'; + +function createStack(overrides?: { + memoryId?: string; + bedrockModels?: string[]; + withMemory?: boolean; + taskSizing?: { + buildTaskCpu?: number; + buildTaskMemoryMiB?: number; + buildTaskEphemeralStorageGiB?: number; + planningTaskCpu?: number; + planningTaskMemoryMiB?: number; + extraBuildEnvironment?: Record; + }; +}): { stack: Stack; template: Template } { const app = new App({ context: overrides?.bedrockModels ? { bedrockModels: overrides.bedrockModels } : undefined, }); @@ -57,6 +69,8 @@ function createStack(overrides?: { memoryId?: string; bedrockModels?: string[] } const githubTokenSecret = new secretsmanager.Secret(stack, 'GitHubTokenSecret'); + const agentMemory = overrides?.withMemory ? new AgentMemory(stack, 'AgentMemory') : undefined; + new EcsAgentCluster(stack, 'EcsAgentCluster', { vpc, agentImageAsset, @@ -65,12 +79,69 @@ function createStack(overrides?: { memoryId?: string; bedrockModels?: string[] } userConcurrencyTable, githubTokenSecret, memoryId: overrides?.memoryId, + agentMemory, + ...(overrides?.taskSizing && { taskSizing: overrides.taskSizing }), }); const template = Template.fromStack(stack); return { stack, template }; } +describe('resolveEcsTaskSizing — the sizing knobs must be reachable at deploy time', () => { + const nodeWith = (context: Record) => new Stack(new App({ context }), 'S').node; + + test('returns undefined when nothing is set, so construct defaults apply', () => { + expect(resolveEcsTaskSizing(nodeWith({}))).toBeUndefined(); + }); + + test('a heavy monorepo can reach the Fargate ceiling from context alone', () => { + // The whole point of the modest default: raising it must NOT require editing + // the construct. Before this was wired, taskSizing had no caller and the + // ceiling was unreachable by any supported route. + expect(resolveEcsTaskSizing(nodeWith({ + ecsBuildTaskCpu: '16384', + ecsBuildTaskMemoryMiB: '122880', + ecsBuildTaskEphemeralStorageGiB: '100', + }))).toEqual({ + buildTaskCpu: 16384, + buildTaskMemoryMiB: 122880, + buildTaskEphemeralStorageGiB: 100, + }); + }); + + test('a malformed number throws at synth rather than silently defaulting', () => { + // "I set the flag and the build still OOM'd" is a worse afternoon than a + // failed synth. + expect(() => resolveEcsTaskSizing(nodeWith({ ecsBuildTaskCpu: 'lots' }))) + .toThrow(/must be a positive integer/); + expect(() => resolveEcsTaskSizing(nodeWith({ ecsBuildTaskMemoryMiB: '-1' }))) + .toThrow(/must be a positive integer/); + }); + + test('build-tool env overrides come through as JSON', () => { + expect(resolveEcsTaskSizing(nodeWith({ ecsExtraBuildEnv: '{"MISE_JOBS":"8"}' }))) + .toEqual({ extraBuildEnvironment: { MISE_JOBS: '8' } }); + }); + + test('a RESERVED platform env key is REJECTED, not merged', () => { + // extraBuildEnvironment spreads over the whole base container env, so without + // this guard a build-tool override could unset platform wiring. The sharp one + // is AGENT_SESSION_ROLE_ARN: absent, the agent falls back to ambient + // credentials and per-tenant scoping is silently off. + expect(() => resolveEcsTaskSizing(nodeWith({ + ecsExtraBuildEnv: '{"AGENT_SESSION_ROLE_ARN":""}', + }))).toThrow(/cannot set 'AGENT_SESSION_ROLE_ARN'/); + expect(() => resolveEcsTaskSizing(nodeWith({ + ecsExtraBuildEnv: '{"TASK_TABLE_NAME":"attacker-table"}', + }))).toThrow(/platform wiring/); + }); + + test('a non-string env value is rejected', () => { + expect(() => resolveEcsTaskSizing(nodeWith({ ecsExtraBuildEnv: '{"MISE_JOBS":8}' }))) + .toThrow(/must be a string/); + }); +}); + describe('EcsAgentCluster construct', () => { let baseTemplate: Template; @@ -89,10 +160,91 @@ describe('EcsAgentCluster construct', () => { }); }); - test('creates a Fargate task definition with 16 vCPU and 120 GB (ABCA-662: full parallel mise build OOM\'d at 64 GB → max Fargate RAM)', () => { + test('the BUILD def defaults to a MODEST size, not the Fargate maximum', () => { + // A default is what an adopter who changes nothing pays for. At the Fargate + // ceiling (16 vCPU / 120 GB) that is roughly 5x the per-build cost of this + // size. Under-provisioning is a slow or OOM-ing build — diagnosable, and one + // prop away from fixed; over-provisioning is a silent bill. baseTemplate.hasResourceProperties('AWS::ECS::TaskDefinition', { + Cpu: '4096', + Memory: '16384', + RequiresCompatibilities: ['FARGATE'], + RuntimePlatform: { + CpuArchitecture: 'ARM64', + OperatingSystemFamily: 'LINUX', + }, + }); + }); + + test('the BUILD def keeps real DISK margin, unlike CPU and memory', () => { + // Measured, not guessed. A full parallel build of a large TypeScript + Python + // monorepo peaked at ~3.1 GB of memory (~5x headroom at 16 GB, because + // MISE_JOBS=1 serialises the packages) but ~14.7 GiB of DISK. At Fargate's + // 21 GiB floor that is only ~1.4x, and running out of space surfaces as a + // spurious build failure rather than an obvious resource error — so disk is + // deliberately sized less aggressively than CPU and memory. Ephemeral storage + // is also a small fraction of the per-task cost, so this keeps the cost win. + baseTemplate.hasResourceProperties('AWS::ECS::TaskDefinition', { + Cpu: '4096', + EphemeralStorage: { SizeInGiB: 50 }, + }); + }); + + test('a heavy monorepo can raise the build task to the Fargate ceiling', () => { + // The size that a large TypeScript + Python monorepo actually needs, reached + // through the prop rather than by being everyone's default. + createStack({ + taskSizing: { + buildTaskCpu: 16384, + buildTaskMemoryMiB: 122880, + buildTaskEphemeralStorageGiB: 100, + }, + }).template.hasResourceProperties('AWS::ECS::TaskDefinition', { Cpu: '16384', Memory: '122880', + EphemeralStorage: { SizeInGiB: 100 }, + }); + }); + + test('build-tool env vars are overridable, so tuned values are not everyone\'s default', () => { + // The platform sets a verify timeout and parallelism caps measured against one + // monorepo's toolchain. A deployment with a different build shape replaces them + // through the prop instead of editing the construct. + const template = createStack({ + taskSizing: { extraBuildEnvironment: { MISE_JOBS: '8', JEST_MAX_WORKERS: '50%' } }, + }).template; + const taskDefs = template.findResources('AWS::ECS::TaskDefinition'); + const build = Object.values(taskDefs).find( + (d) => (d as { Properties: { Cpu: string } }).Properties.Cpu === '4096', + ); + expect(build).toBeDefined(); + const env = ((build as { + Properties: { ContainerDefinitions: Array<{ Environment?: Array<{ Name: string; Value: string }> }> }; + }).Properties.ContainerDefinitions[0].Environment ?? []); + const byName = Object.fromEntries(env.map((e) => [e.Name, e.Value])); + expect(byName.MISE_JOBS).toBe('8'); + expect(byName.JEST_MAX_WORKERS).toBe('50%'); + // A key the caller did NOT override keeps the platform value. + expect(byName.BUILD_VERIFY_TIMEOUT_S).toBe('3600'); + }); + + test('the PLANNING def keeps the 20 GiB default (no EphemeralStorage — a clone+read planner needs no extra disk)', () => { + const taskDefs = baseTemplate.findResources('AWS::ECS::TaskDefinition'); + const planning = Object.values(taskDefs).find( + d => d.Properties.Cpu === '2048' && d.Properties.Memory === '8192', + ); + expect(planning).toBeDefined(); + expect(planning!.Properties.EphemeralStorage).toBeUndefined(); + }); + + test('creates a second, smaller PLANNING task def (2 vCPU / 8 GB) for read-only workflows (#299 ECS_RIGHTSIZED_PLANNING)', () => { + // Two task defs now exist: the 64 GB build def (asserted above) and this + // 8 GB planning def. decompose-v1 (read_only) runs on the smaller one so a + // clone+read plan doesn't over-allocate the build box. + baseTemplate.resourceCountIs('AWS::ECS::TaskDefinition', 2); + baseTemplate.hasResourceProperties('AWS::ECS::TaskDefinition', { + Cpu: '2048', + Memory: '8192', RequiresCompatibilities: ['FARGATE'], RuntimePlatform: { CpuArchitecture: 'ARM64', @@ -101,6 +253,76 @@ describe('EcsAgentCluster construct', () => { }); }); + // The default task sizes are generous (tuned for a large monorepo build). A + // consumer with a lighter repo can override them per substrate to cut Fargate + // cost, so the sizing is configuration, not a fixed value. + test('taskSizing prop overrides the build def size; fields left unset keep the default', () => { + const { template } = createStack({ + taskSizing: { + buildTaskCpu: 4096, // 4 vCPU + buildTaskMemoryMiB: 16384, // 16 GB + buildTaskEphemeralStorageGiB: 40, + // planning sizes intentionally omitted -> they should stay at their defaults + }, + }); + // The override changes the existing build task def in place — it does not + // add a third one. There should still be exactly two: build + planning. + template.resourceCountIs('AWS::ECS::TaskDefinition', 2); + // The build task def reflects the overridden sizes. + template.hasResourceProperties('AWS::ECS::TaskDefinition', { + Cpu: '4096', + Memory: '16384', + EphemeralStorage: { SizeInGiB: 40 }, + }); + // The planning task def is unchanged: omitted fields fall back to the default. + const planning = Object.values(template.findResources('AWS::ECS::TaskDefinition')) + .find(d => d.Properties.Cpu === '2048' && d.Properties.Memory === '8192'); + expect(planning).toBeDefined(); + }); + + test('both task defs share ONE task role and ONE execution role (parity by construction — the ABCA-488/#502 lesson)', () => { + // The build and planning defs pass the SAME shared task+execution roles, so a + // grant added for one is present on the other by construction (no drift). The + // template therefore holds exactly two ECS roles (task + execution), each + // referenced by both defs' TaskRoleArn/ExecutionRoleArn. + const roles = baseTemplate.findResources('AWS::IAM::Role', { + Properties: { + AssumeRolePolicyDocument: { + Statement: Match.arrayWith([ + Match.objectLike({ + Principal: { Service: 'ecs-tasks.amazonaws.com' }, + }), + ]), + }, + }, + }); + expect(Object.keys(roles)).toHaveLength(2); + + const taskDefs = baseTemplate.findResources('AWS::ECS::TaskDefinition'); + const taskRoleRefs = new Set(); + const execRoleRefs = new Set(); + for (const def of Object.values(taskDefs)) { + taskRoleRefs.add(JSON.stringify(def.Properties.TaskRoleArn)); + execRoleRefs.add(JSON.stringify(def.Properties.ExecutionRoleArn)); + } + // Both defs point at the same single task role and same single exec role. + expect(taskRoleRefs.size).toBe(1); + expect(execRoleRefs.size).toBe(1); + }); + + test('the PLANNING def carries no BUILD_VERIFY_TIMEOUT_S (a read-only planner runs no build verify)', () => { + const taskDefs = baseTemplate.findResources('AWS::ECS::TaskDefinition'); + const planningDef = Object.values(taskDefs).find( + d => d.Properties.Cpu === '2048' && d.Properties.Memory === '8192', + ); + expect(planningDef).toBeDefined(); + const env = planningDef!.Properties.ContainerDefinitions[0].Environment ?? []; + expect(env.some((e: { Name: string }) => e.Name === 'BUILD_VERIFY_TIMEOUT_S')).toBe(false); + // …but the shared env (Bedrock, task table) IS present on the planning def too. + expect(env.some((e: { Name: string }) => e.Name === 'CLAUDE_CODE_USE_BEDROCK')).toBe(true); + expect(env.some((e: { Name: string }) => e.Name === 'TASK_TABLE_NAME')).toBe(true); + }); + test('creates a security group with TCP 443 egress only', () => { baseTemplate.hasResourceProperties('AWS::EC2::SecurityGroup', { GroupDescription: 'ECS Agent Tasks - egress TCP 443 only', @@ -175,64 +397,6 @@ describe('EcsAgentCluster construct', () => { expect(hasLinearOauthGrant).toBe(true); }); - test('task role gets bedrock-agentcore:CreateEvent on the AgentMemory when wired (F-2 / ABCA-488-class)', () => { - // REGRESSION: the agent's cross-task learning writes (write_task_episode / - // write_repo_learnings) call bedrock-agentcore:CreateEvent on the AgentCore - // Memory. The runtime role gets this via agentMemory.grantReadWrite; the ECS - // task role did NOT, so writes hit AccessDenied and silently no-op'd (WARN) - // on the ECS substrate — learning never persisted on an ECS-only deploy. - // Build a stack WITH an AgentMemory and assert the CreateEvent grant exists, - // scoped to the memory ARN (not a wildcard). - const app = new App(); - const stack = new Stack(app, 'EcsMemStack'); - const vpc = new ec2.Vpc(stack, 'Vpc', { maxAzs: 2 }); - const agentImageAsset = new ecr_assets.DockerImageAsset(stack, 'AgentImage', { - directory: path.join(__dirname, '..', '..', '..', 'agent'), - }); - const mk = (id: string) => - new dynamodb.Table(stack, id, { partitionKey: { name: 'task_id', type: dynamodb.AttributeType.STRING } }); - const userConcurrencyTable = new dynamodb.Table(stack, 'UserConcurrencyTable', { - partitionKey: { name: 'user_id', type: dynamodb.AttributeType.STRING }, - }); - const agentMemory = new AgentMemory(stack, 'AgentMemory'); - new EcsAgentCluster(stack, 'EcsAgentCluster', { - vpc, - agentImageAsset, - taskTable: mk('TaskTable'), - taskEventsTable: mk('TaskEventsTable'), - userConcurrencyTable, - githubTokenSecret: new secretsmanager.Secret(stack, 'GitHubTokenSecret'), - agentMemory, - }); - const template = Template.fromStack(stack); - const policies = template.findResources('AWS::IAM::Policy'); - let hasCreateEvent = false; - for (const [id, p] of Object.entries(policies)) { - if (!id.includes('TaskDefTaskRole')) continue; - for (const s of p.Properties.PolicyDocument.Statement) { - const actions = Array.isArray(s.Action) ? s.Action : [s.Action]; - if (actions.includes('bedrock-agentcore:CreateEvent')) { - hasCreateEvent = true; - // resource must reference the memory ARN, not a bare wildcard - expect(JSON.stringify(s.Resource)).toContain('MemoryArn'); - expect(s.Resource).not.toEqual('*'); - } - } - } - expect(hasCreateEvent).toBe(true); - }); - - test('task role has NO bedrock-agentcore grant when no AgentMemory is wired (isolated default)', () => { - const policies = baseTemplate.findResources('AWS::IAM::Policy'); - for (const [id, p] of Object.entries(policies)) { - if (!id.includes('TaskDefTaskRole')) continue; - for (const s of p.Properties.PolicyDocument.Statement) { - const actions = Array.isArray(s.Action) ? s.Action : [s.Action]; - expect(actions.some((a: string) => a.startsWith('bedrock-agentcore:'))).toBe(false); - } - } - }); - test('task role Bedrock InvokeModel is scoped to explicit model/inference-profile ARNs (no wildcard)', () => { const policies = baseTemplate.findResources('AWS::IAM::Policy'); let bedrockStatement: { Resource: unknown } | undefined; @@ -317,6 +481,27 @@ describe('EcsAgentCluster construct', () => { }); }); + test('build def caps build parallelism to prevent OOM (K14 / ABCA-691)', () => { + // The build task def serializes the mise DAG (MISE_JOBS=1) and pins the jest + // fleet (JEST_MAX_WORKERS=4) so the cross-package build storm can't OOM the + // box while the coding agent is still resident. Asserted per-var (one + // arrayWith objectLike each): a single arrayWith with multiple objectLike + // entries is matched unreliably by the CDK assertions matcher, so each env + // var gets its own hasResourceProperties call — which also pins each to the + // SAME build container (the one carrying BUILD_VERIFY_TIMEOUT_S). + const envHas = (name: string, value: string) => + baseTemplate.hasResourceProperties('AWS::ECS::TaskDefinition', { + ContainerDefinitions: Match.arrayWith([ + Match.objectLike({ + Environment: Match.arrayWith([Match.objectLike({ Name: name, Value: value })]), + }), + ]), + }); + envHas('MISE_JOBS', '1'); + envHas('JEST_MAX_WORKERS', '4'); + envHas('BUILD_VERIFY_TIMEOUT_S', '3600'); + }); + test('includes MEMORY_ID in container env when provided', () => { const { template } = createStack({ memoryId: 'mem-test-123' }); template.hasResourceProperties('AWS::ECS::TaskDefinition', { @@ -330,6 +515,36 @@ describe('EcsAgentCluster construct', () => { }); }); + // F-2 ECS-parity regression guard. The task role must be able to WRITE cross- + // task memory (bedrock-agentcore:CreateEvent), or episodic/semantic writes fail + // closed on ECS (memory_written: false — live-caught on the fork). This + // regressed silently because MEMORY_ID was wired into the env WITHOUT the + // matching grant, so the agent attempted a write it had no permission for. + describe('AgentCore Memory grant (F-2)', () => { + test('grants the task role bedrock-agentcore write when agentMemory is passed', () => { + const { template } = createStack({ withMemory: true }); + template.hasResourceProperties('AWS::IAM::Policy', { + PolicyDocument: { + Statement: Match.arrayWith([ + Match.objectLike({ + Effect: 'Allow', + Action: Match.arrayWith([Match.stringLikeRegexp('bedrock-agentcore:.*Event')]), + }), + ]), + }, + }); + }); + + test('does NOT grant bedrock-agentcore write when agentMemory is omitted', () => { + // Negative proof: the isolated-construct default (no memory) must not + // emit a memory grant — otherwise the positive test proves nothing. + const { stack } = createStack(); + const policies = Template.fromStack(stack).findResources('AWS::IAM::Policy'); + const asJson = JSON.stringify(policies); + expect(asJson).not.toMatch(/bedrock-agentcore:[A-Za-z]*Event/); + }); + }); + describe('with a SessionRole wired (#209)', () => { function createWithSessionRole(): Template { const app = new App(); @@ -393,7 +608,12 @@ describe('EcsAgentCluster construct', () => { // statements). The task-role policy must NOT contain any unconditioned // task-table DDB grant — that access now lives only on the SessionRole. const taskRolePolicies = Object.entries(policies).filter(([id, p]) => - id.includes('TaskDefTaskRole') + // #299 ECS_RIGHTSIZED_PLANNING: the task role is now a SHARED standalone + // `TaskRole` construct (was the auto-generated role nested under the single + // FargateTaskDefinition, id `...TaskDefTaskRole...`), so both the build and + // planning defs pass the same role — its logical id is `...TaskRole...` and + // `ExecutionRole` doesn't match this substring. + id.includes('TaskRole') && p.Properties.PolicyDocument.Statement.some((s: { Action: string | string[] }) => { const actions = Array.isArray(s.Action) ? s.Action : [s.Action]; return actions.includes('sts:AssumeRole'); diff --git a/cdk/test/handlers/orchestrate-task.test.ts b/cdk/test/handlers/orchestrate-task.test.ts index 4314896c9..b84c055c4 100644 --- a/cdk/test/handlers/orchestrate-task.test.ts +++ b/cdk/test/handlers/orchestrate-task.test.ts @@ -754,6 +754,33 @@ describe('loadBlueprintConfig', () => { const config = await loadBlueprintConfig(baseTask as any); expect(config.cedar_policies).toBeUndefined(); }); + + // Compute substrate is a per-repo property that applies to ALL workflows: a + // read-only decompose/review task clones + reads the SAME repo, so it needs the + // SAME compute (a repo big enough to need the 64GB ECS tier to build also OOMs + // the AgentCore microVM just reading it). So an ecs repo's ecs compute_type + // flows through regardless of the workflow's read-only-ness. + const ecsRepoConfig = { + repo: 'org/repo', + status: 'active' as const, + onboarded_at: '2024-01-01T00:00:00Z', + updated_at: '2024-01-01T00:00:00Z', + compute_type: 'ecs' as const, + }; + + test('a read-only workflow (coding/decompose-v1) on an ecs repo INHERITS ecs (same repo, same footprint)', async () => { + mockLoadRepoConfig.mockResolvedValueOnce(ecsRepoConfig); + const planTask = { ...baseTask, resolved_workflow: { id: 'coding/decompose-v1', version: '1.0.0' } }; + const config = await loadBlueprintConfig(planTask as any); + expect(config.compute_type).toBe('ecs'); + }); + + test('a writeable workflow (coding/new-task-v1) on an ecs repo also uses ecs', async () => { + mockLoadRepoConfig.mockResolvedValueOnce(ecsRepoConfig); + const buildTask = { ...baseTask, resolved_workflow: { id: 'coding/new-task-v1', version: '1.0.0' } }; + const config = await loadBlueprintConfig(buildTask as any); + expect(config.compute_type).toBe('ecs'); + }); }); describe('hydrateAndTransition with blueprint config', () => { diff --git a/cdk/test/handlers/shared/strategies/ecs-strategy.test.ts b/cdk/test/handlers/shared/strategies/ecs-strategy.test.ts index d3b6a0d01..db474ddd3 100644 --- a/cdk/test/handlers/shared/strategies/ecs-strategy.test.ts +++ b/cdk/test/handlers/shared/strategies/ecs-strategy.test.ts @@ -86,8 +86,14 @@ describe('EcsComputeStrategy', () => { const call = mockSend.mock.calls[0][0]; expect(call.input.cluster).toBe(CLUSTER_ARN); - expect(call.input.taskDefinition).toBe(TASK_DEF_ARN); + // Dispatch against the task-def FAMILY, not the pinned revision, so a deploy + // that deregisters the old revision can't strand the task ("TaskDefinition is + // inactive", ABCA-660/663). TASK_DEF_ARN ends in `agent:1` → family `agent`. + expect(call.input.taskDefinition).toBe('agent'); expect(call.input.launchType).toBe('FARGATE'); + // review #5c: clientToken = taskId dedups a double-RunTask (client-side + // timeout on a launch that actually succeeded → auto-retry re-fires). + expect(call.input.clientToken).toBe('TASK001'); expect(call.input.networkConfiguration.awsvpcConfiguration.subnets).toEqual(['subnet-aaa', 'subnet-bbb']); expect(call.input.networkConfiguration.awsvpcConfiguration.securityGroups).toEqual(['sg-12345']); expect(call.input.networkConfiguration.awsvpcConfiguration.assignPublicIp).toBe('DISABLED'); @@ -118,6 +124,24 @@ describe('EcsComputeStrategy', () => { expect(override.command[0]).toBe('python'); }); + test('readOnly falls back to the build def when no planning def is wired (older deploy — never worse)', async () => { + // The top-of-file import has NO ECS_PLANNING_TASK_DEFINITION_ARN, so even a + // read-only workflow must run on the build def (pre-rightsize behavior). + mockSend.mockResolvedValueOnce({ tasks: [{ taskArn: TASK_ARN }] }); + + const strategy = new EcsComputeStrategy(); + await strategy.startSession({ + taskId: 'TASK001', + userId: 'cognito-test', + payload: { repo_url: 'org/repo' }, + blueprintConfig: { compute_type: 'ecs', runtime_arn: '' }, + readOnly: true, + }); + + // No planning def wired → build def, resolved to its family (`agent`). + expect(mockSend.mock.calls[0][0].input.taskDefinition).toBe('agent'); + }); + test('throws when RunTask returns no task', async () => { mockSend.mockResolvedValueOnce({ tasks: [], @@ -459,6 +483,71 @@ describe('EcsComputeStrategy with ECS_PAYLOAD_BUCKET (S3-pointer path, #502)', ( }); }); +// #299 ECS_RIGHTSIZED_PLANNING: the planning task def ARN is a module-level +// constant, so set it BEFORE import via isolateModules (mirrors the #502 bucket +// pattern above) — this keeps it out of the inline tests at the top. +describe('EcsComputeStrategy read-only planning-def selection (#299 ECS_RIGHTSIZED_PLANNING)', () => { + const PLANNING_DEF_ARN = 'arn:aws:ecs:us-east-1:123456789012:task-definition/agent-planning:1'; + + function loadStrategyWithPlanningDef(): typeof import('../../../../src/handlers/shared/strategies/ecs-strategy').EcsComputeStrategy { + let Strategy!: typeof import('../../../../src/handlers/shared/strategies/ecs-strategy').EcsComputeStrategy; + jest.isolateModules(() => { + process.env.ECS_CLUSTER_ARN = CLUSTER_ARN; + process.env.ECS_TASK_DEFINITION_ARN = TASK_DEF_ARN; + process.env.ECS_PLANNING_TASK_DEFINITION_ARN = PLANNING_DEF_ARN; + process.env.ECS_SUBNETS = 'subnet-aaa,subnet-bbb'; + process.env.ECS_SECURITY_GROUP = 'sg-12345'; + process.env.ECS_CONTAINER_NAME = 'AgentContainer'; + // eslint-disable-next-line @typescript-eslint/no-require-imports + Strategy = require('../../../../src/handlers/shared/strategies/ecs-strategy').EcsComputeStrategy; + }); + return Strategy; + } + + afterEach(() => { + delete process.env.ECS_PLANNING_TASK_DEFINITION_ARN; + }); + + test('a read-only workflow runs on the PLANNING def', async () => { + mockSend.mockResolvedValueOnce({ tasks: [{ taskArn: TASK_ARN }] }); + const Strategy = loadStrategyWithPlanningDef(); + await new Strategy().startSession({ + taskId: 'TASK001', + userId: 'cognito-test', + payload: { repo_url: 'org/repo' }, + blueprintConfig: { compute_type: 'ecs', runtime_arn: '' }, + readOnly: true, + }); + // read-only → planning def, resolved to its family (`agent-planning`). + expect(mockSend.mock.calls[0][0].input.taskDefinition).toBe('agent-planning'); + }); + + test('a non-read-only workflow still runs on the BUILD def even when a planning def is wired', async () => { + mockSend.mockResolvedValueOnce({ tasks: [{ taskArn: TASK_ARN }] }); + const Strategy = loadStrategyWithPlanningDef(); + await new Strategy().startSession({ + taskId: 'TASK001', + userId: 'cognito-test', + payload: { repo_url: 'org/repo' }, + blueprintConfig: { compute_type: 'ecs', runtime_arn: '' }, + readOnly: false, + }); + expect(mockSend.mock.calls[0][0].input.taskDefinition).toBe('agent'); + }); + + test('omitting readOnly defaults to the BUILD def', async () => { + mockSend.mockResolvedValueOnce({ tasks: [{ taskArn: TASK_ARN }] }); + const Strategy = loadStrategyWithPlanningDef(); + await new Strategy().startSession({ + taskId: 'TASK001', + userId: 'cognito-test', + payload: { repo_url: 'org/repo' }, + blueprintConfig: { compute_type: 'ecs', runtime_arn: '' }, + }); + expect(mockSend.mock.calls[0][0].input.taskDefinition).toBe('agent'); + }); +}); + describe('deleteEcsPayload without ECS_PAYLOAD_BUCKET', () => { test('no-ops when no payload bucket is configured', async () => { // The top-of-file import has no ECS_PAYLOAD_BUCKET set. diff --git a/cdk/test/stacks/agent.test.ts b/cdk/test/stacks/agent.test.ts index 9319c3f53..09f79c8a3 100644 --- a/cdk/test/stacks/agent.test.ts +++ b/cdk/test/stacks/agent.test.ts @@ -523,12 +523,69 @@ describe('AgentStack with the ECS substrate gate (--context compute_type=ecs)', template = Template.fromStack(stack); }); - test('provisions an ECS cluster + Fargate task definition', () => { + test('provisions an ECS cluster + both Fargate task definitions (build + planning)', () => { template.resourceCountIs('AWS::ECS::Cluster', 1); - template.resourceCountIs('AWS::ECS::TaskDefinition', 1); + // Two task defs: the large build def and the smaller read-only planning def + // (read-only workflows run on the planning def so a clone-and-read task + // doesn't allocate the full build task's CPU/memory). + template.resourceCountIs('AWS::ECS::TaskDefinition', 2); }); test('outputs ComputeSubstrate=ecs so the CLI allows compute_type=ecs onboarding', () => { template.hasOutput('ComputeSubstrate', { Value: 'ecs' }); }); + + test('the orchestrator gets the PLANNING task-def ARN, not just the build one', () => { + // Without this env var the ECS strategy's `readOnly && + // ECS_PLANNING_TASK_DEFINITION_ARN` guard is always falsy, so the planning + // def would be synthesized, billed for, and never receive a workflow — the + // feature inert while looking present in the template. + // + // This has to be asserted at the STACK level. The strategy's own unit tests + // set the env var by hand to exercise the routing branch, so they pass + // whether or not anything in the stack actually supplies it; only synth can + // tell us the wiring exists. Both ARNs are asserted together because the bug + // this pins is one being present without the other. + const envs = Object.values( + template.findResources('AWS::Lambda::Function'), + ).map(fn => fn.Properties?.Environment?.Variables ?? {}); + const orchestrator = envs.filter(e => 'ECS_TASK_DEFINITION_ARN' in e); + expect(orchestrator).toHaveLength(1); + expect(orchestrator[0]).toHaveProperty('ECS_PLANNING_TASK_DEFINITION_ARN'); + // ...and the two must be DIFFERENT task defs, or read-only workflows are + // silently running on the build box anyway. + expect(orchestrator[0].ECS_PLANNING_TASK_DEFINITION_ARN) + .not.toEqual(orchestrator[0].ECS_TASK_DEFINITION_ARN); + }); + + test('build-task sizing is reachable from deploy context, not only from the construct', () => { + // The construct's default is deliberately modest so an adopter who changes + // nothing does not pay for the Fargate ceiling. That is only defensible if a + // heavy monorepo can RAISE it without editing the construct — and `taskSizing` + // had no caller at all, so the ceiling was unreachable by any supported route. + // This asserts the whole path: context -> resolver -> construct -> template. + const app = new App({ + context: { + compute_type: 'ecs', + ecsBuildTaskCpu: '16384', + ecsBuildTaskMemoryMiB: '122880', + ecsBuildTaskEphemeralStorageGiB: '100', + }, + }); + const sized = Template.fromStack(new AgentStack(app, 'SizedEcsStack', { + env: { account: '123456789012', region: 'us-east-1' }, + })); + sized.hasResourceProperties('AWS::ECS::TaskDefinition', { + Cpu: '16384', + Memory: '122880', + EphemeralStorage: { SizeInGiB: 100 }, + }); + }); + + test('the DEFAULT ECS build task stays modest', () => { + template.hasResourceProperties('AWS::ECS::TaskDefinition', { + Cpu: '4096', + Memory: '16384', + }); + }); });