feat(carve S2): configurable ECS Fargate task sizing + read-only planning task

[isadeks]

Stacked on #647 (carve/s1-foundation-contracts). Review that first; this PR's diff is only its own slice.

What this adds

A read-only planning task definition for the ECS Fargate compute backend, and makes every Fargate task size configurable instead of hardcoded.

Planning-type work (reading a repo, producing a plan) needs no write access and far less compute than a build. Routing it to its own smaller, read-only task definition means it no longer borrows the build tier's footprint.

Sizing is configuration, not a baked-in opinion

The build tier's defaults are generous because a build of a large monorepo can genuinely need them, but nothing here is fixed: a new taskSizing prop overrides any of the CPU / memory / ephemeral-storage values independently, and omitting it keeps the defaults. Operators whose workloads are smaller (or larger) set what fits rather than editing the construct.

Defaults: build 16 vCPU / 120 GiB / 100 GiB ephemeral; planning 2 vCPU / 8 GiB.

Note for reviewers

Updating the sizing replaces the task definition in place (a new revision) rather than adding one — there's a test asserting exactly that, so an override can't silently leave the old definition behind.

Verification

Full CDK suite green. The override path was deploy-verified: setting taskSizing produced a smaller task definition, and the default path was unchanged.

---

Tracking

Slice S2 of the linear-vercel → main carve. Tracking issue: #668 (slice table, why it is sliced rather than merged, and review guidance).

Lands part of #247 (parent/sub-issue orchestration) and #299 (auto-decomposition). Deliberately not Closes — no single slice closes either; they close when S8 activates the arc.

Stacked on #647 — review that first; this PR's diff is against its branch.

[isadeks]

The red Secrets, deps, and workflow scan here is not caused by this PR — it touches no lockfile or manifest (source + tests only).

The same job has been failing on main itself since the 2026-07-06 scheduled run (also 07-13 and 07-20), on 4 High advisories published against already-resolved dependencies: brace-expansion (×3, GHSA-mh99-v99m-4gvg) and bedrock-agentcore (GHSA-j6g5-3hh3-pgw8). Because the gate runs per-PR, it fails every open PR as a side effect. #647 (this PR's base) shows green only because it last ran before those advisories were published.

Fixed in #658 (lockfile + pin only, against main) — osv-scanner now reports No issues found, and all 8 checks are green there. Once that merges, re-running this job should clear it; nothing to change in this PR.

One note for #658's reviewer: the usual re-resolve isn't sufficient this time. The brace-expansion advisory covers everything <= 5.0.7, so the v1 and v2 lines in the tree have no patched release inside their own majors — it needs a resolutions entry to collapse all three ranges onto 5.0.8.

[isadeks]

Automated review — carve S2 (#653)

Reviewed as its own diff only (carve/s1-foundation-contracts..carve/s2-ecs-rightsized-planning, 12 files, +809/−296). Governance is noted once for the whole stack, not repeated here.

Slice standalone-ness: PASS

tsc --noEmit clean at this tip on its own base (compiler run, not inferred). yarn.lock is byte-identical to main here, so no dependency change.

Reverts: CLEAN

I checked the 296 deletions rather than assuming they were rewrites. Every exported symbol present on main in the four touched source files still exists at this slice tip:

file	main → s2 deletions	exports lost
ecs-agent-cluster.ts	127	none
orchestrate-task.ts	23	none
orchestrator.ts	44	none
ecs-strategy.ts	13	none

The deletions are genuine restructuring — most notably collapsing the two Fargate task definitions onto one shared makeTaskDef factory, which is a real improvement: it makes it structurally impossible for a grant or env var to land on one task def and not the other.

This PR also deletes the stray cdk/node_modules symlink that #647 committed (pointing at /private/tmp/abca-s5/cdk/node_modules). That deletion is correct and welcome — I have raised the symlink itself on #647, since that is the PR that would merge first and carry it.

---

1. MAJOR — four build-tool env vars are hardcoded to this project's own monorepo and are not overridable

The build task definition hardcodes into its container environment:

BUILD_VERIFY_TIMEOUT_S: '3600'
JEST_MAX_WORKERS:       '4'
MISE_JOBS:              '1'

(plus a pre-push-hook skip)

The comments are careful and clearly measured — but every measurement is against this repo: "mise run build fans out its depends (the per-package build/quality legs)", "each package then spawns its OWN worker fleet (jest, pytest, esbuild, cdk synth)", "the test suite at 4 workers peaks at only ~2.2 GB".

A public adopter's repo may use none of mise, jest, pytest or cdk synth. For them these vars are either inert (harmless) or actively wrong — MISE_JOBS: '1' serialises any adopter who does use mise, trading wall-clock for a memory problem they may not have.

Crucially, unlike the CPU/memory sizing, these are not configurable: EcsTaskSizing exposes only buildTaskCpu, buildTaskMemoryMiB, buildTaskEphemeralStorageGiB, planningTaskCpu, planningTaskMemoryMiB. There is no prop for the env.

Suggest an extraBuildEnvironment?: Record<string, string> prop (or a buildVerifyTimeoutS?: number) so the tuned values become this deployment's opinion rather than everyone's default. The reasoning is worth keeping in the repo — just not welded into the construct.

2. MAJOR — the default build task is the Fargate maximum, ~5× the cost of a modest size

DEFAULT_BUILD_TASK_CPU = 16384 (16 vCPU) and DEFAULT_BUILD_TASK_MEMORY_MIB = 122880 (120 GB), plus a 100 GiB root filesystem against Fargate's 20 GiB default.

To be fair to the PR: the docstring is honest about this ("deliberately generous because they're tuned for a worst case: a large TypeScript + Python monorepo"), it explains that 120 GB is the Fargate ceiling at 16 vCPU, and it points the reader at taskSizing to shrink it. That is the right structure.

The concern is the default, since that is what an adopter who changes nothing gets. Rough us-east-1 on-demand Fargate:

sizing	$/hr	10-min build	100 builds
default (16 vCPU / 120 GB)	$1.181	$0.197	$19.68
modest (4 vCPU / 16 GB)	$0.233	$0.039	$3.88

≈ 5.1× per build. For a public sample, a modest default with a documented "raise this for a large monorepo" note inverts the surprise in the safer direction — an adopter who under-provisions gets a slow build, whereas one who over-provisions gets a bill. The planning default (2 vCPU / 8 GB) is well judged and I would not change it.

3. MINOR — "read-only planning task" describes the workflow, not the IAM

The naming and comments consistently call this the read-only task def ("PLANNING task: 2 vCPU / 8 GB … For read-only workflows that clone and read the repo to produce a plan but never build"). But both task definitions are created by the same makeTaskDef factory sharing one taskRole and one executionRole, so the planning task holds exactly the same permissions as the build task — including the artifacts-bucket and AgentCore Memory read+write grants.

That is a deliberate, well-argued choice — the comment says so explicitly, that splitting the roles is how a grant silently drifts between the two defs — and I am not asking for it to change. But "read-only" invites a reader (or a future security reviewer) to assume a reduced-privilege role that does not exist. Worth one clarifying clause: "read-only refers to the workflow's behaviour; both task defs deliberately share one role so grants cannot drift."

---

Verified clean

• IAM: no wildcard actions introduced. The shared-role restructuring is a net least-privilege improvement over two independently-granted defs.
• No source/test split at this boundary — every touched source file's test is updated in this same slice (ecs-agent-cluster.test.ts +236, ecs-strategy.test.ts +91, orchestrate-task.test.ts +27).
• Invalid CPU/memory pairs are acknowledged as failing at deploy time rather than synth, which is the correct expectation to document for Fargate.

---

Reviewed with Claude Code. Verification: tsc --noEmit on this slice's own base; all 296 deletions checked for lost exports against main; the shared taskRole traced through makeTaskDef; EcsTaskSizing fields enumerated to confirm the env vars have no override; Fargate cost computed from published us-east-1 on-demand rates.

Note: this review was written directly rather than by the review agent assigned to it — that agent terminated mid-run without producing a result, so I reviewed the diff myself. Same for #656.

[isadeks]

Note: force-push during review — plus a BLOCKING finding that moved into this PR

My review above was written against b4c5d1dd; the current head is 5b755209. One finding moved into this PR from #647 as a result of the re-slicing, and it is blocking.

---

NEW BLOCKING — this PR now admits two workflows whose agent YAML does not exist until #654

The DESCRIPTORS entries for coding/restack-v1 and coding/decompose-v1 were removed from #647 and now land here. The YAML still arrives one slice later:

slice	DESCRIPTORS entry	agent/workflows/coding/restack-v1.yaml
#647 (s1)	absent	missing
#653 (s2)	present	missing
#654 (s3)	present	present

DESCRIPTORS is the live admission table — create-task-core.ts resolves any submitted workflow_ref against it via resolveWorkflowRef → getWorkflowDescriptor, and there is no platform-only submission gate (I grepped USER_SUBMITTABLE / PLATFORM_ONLY / internalWorkflow / platformOnly across cdk/src — nothing).

Failure: merge #653 alone and deploy. A user submits workflow_ref: "coding/decompose-v1". Admission succeeds (201 — the user is told the task started), then the agent's load_workflow raises WorkflowValidationError: workflow 'coding/decompose-v1' not found at <path>. Accepted-then-dead is a worse outcome than a clean 400 at submission.

The fix is the same as before, one slice over: move the two DESCRIPTORS entries into #654 alongside their YAML, or bring the YAML forward into this PR.

---

Status of my original findings at 5b755209

finding	status
1. MAJOR — BUILD_VERIFY_TIMEOUT_S/JEST_MAX_WORKERS/MISE_JOBS hardcoded to this project's monorepo, not overridable	still present
2. MAJOR — default build task is the Fargate maximum (16 vCPU / 120 GB), ≈5× the cost of a modest size	still present
3. MINOR — "read-only planning task" shares the build task's role and grants	still present
Reverts / lost exports	still clean — re-checked, no main export lost
Deletes #647's stray cdk/node_modules symlink	still true, still welcome

[isadeks]

Review findings addressed

I had missed this review in my first pass — apologies. All three findings are now fixed, and the blocking one from your addendum was already resolved by then.

BLOCKING (addendum) — this PR admitted two workflows whose YAML arrives a slice later. Already fixed before I read your addendum, by moving the DESCRIPTORS entries into #654 alongside their files. Your re-verification caught that my first move only relocated the problem by one slice, which is exactly right. What actually prevents it recurring is the reverse drift check I added: the test only walked YAML → descriptor, so an orphan descriptor was invisible; it now asserts every declared descriptor has a shipped file, verified by re-adding an orphan and watching it fail.

MAJOR #1 — four build-tool env vars hardcoded to this project's monorepo, not overridable. Fixed. You were right that these are this deployment's opinion rather than a universal default, and that MISE_JOBS: '1' actively penalises an adopter who uses mise with enough memory. Added extraBuildEnvironment?: Record<string, string> on EcsTaskSizing, merged over the defaults so a caller's key wins. The measured reasoning stays as the comment it always was; it is no longer welded in. Tested: an override replaces the value, and a key the caller does not name keeps the platform default.

MAJOR #2 — default build task was the Fargate maximum. Fixed. Your cost table is the argument: ~5.1× per build for the default an adopter gets by changing nothing. Now 4 vCPU / 16 GB on Fargate's own 20 GiB disk, with the ceiling reachable through taskSizing and a test exercising that path. Your framing of which direction to be surprised in — a slow build is diagnosable and one prop from fixed, a bill is not — is what decided it. Planning default left at 2 vCPU / 8 GB as you suggested.

Two of the tests that pinned the old defaults have been rewritten rather than deleted, so the heavy-monorepo size is still asserted — just as an override rather than as everyone's baseline.

MINOR #3 — "read-only planning task" describes the workflow, not the IAM. Fixed as you suggested, with the clarifying clause. You are right not to ask for the roles to be split: the shared role is what stops a grant drifting between the two defs, and that reasoning is now stated where a future security reviewer will read it.

Thank you also for checking all 296 deletions for lost exports rather than assuming they were rewrites, and for flagging the stray cdk/node_modules symlink — that one turned out to matter. It was mine, committed by a git add -A in a worktree that symlinks its dependencies, and it had been failing CI at install time with EEXIST in a way that reads like a runner cache glitch. Removed at the source, and .gitignore now matches the symlink form as well as the directory form, since node_modules/ with a trailing slash does not.

Gates: 2544 CDK tests, tsc/eslint clean.

[isadeks]

Second-round review: my de-opinionation was unreachable, and the env override was a footgun

Two findings on my own fix, both confirmed.

The prop had no caller, so the ceiling was unreachable. Lowering the default to 4 vCPU / 16 GB is only defensible if a heavy monorepo can raise it without editing the construct — but nothing passed taskSizing, AgentStack takes no custom props, and no context key read it. So I traded "everyone pays for 16 vCPU" for "nobody can reach 16 vCPU", and the second is worse for exactly the repo the old default existed to serve. That is the first review round's concern in reverse.

Sizing and the build-tool env are now read from deploy context, the same shape as the compute_type gate beside them:

cdk deploy -c compute_type=ecs -c ecsBuildTaskCpu=16384 \
  -c ecsBuildTaskMemoryMiB=122880 -c ecsBuildTaskEphemeralStorageGiB=100
cdk deploy -c ecsExtraBuildEnv='{"MISE_JOBS":"8"}'

A malformed value 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. Reachability is now asserted end to end (context → resolver → construct → template); unwiring the call site fails that test, where before it left the whole suite green.

extraBuildEnvironment could clobber platform wiring. It spreads over the entire container env, not just the four build-tool vars it was added for. The reviewer's list is right, and the sharp one is AGENT_SESSION_ROLE_ARN: 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. An adopter mistyping a key would have disabled an isolation control by accident. Reserved keys are now rejected at synth with a message naming the key; mutation-verified.

Stale docstrings fixed. They still advertised 16 vCPU / 120 GB / 100 GiB two lines above code saying 4096 / 16384 / 21, and the per-field @default tags likewise. Whichever an operator read first was wrong.

Verified and not changed: 4096/16384 is a legal Fargate pair and 21 GiB is the legal ephemeral-storage floor (20 throws) — confirmed by a full cdk synth under the ECS gate, so no deploy-breaking sizing defect. The reviewer also notes CDK validates ephemeral storage but not the cpu/memory combination, so the pre-existing docstring claim that "an invalid pair fails at synth" is false — an invalid pair reaches CloudFormation. That predates this work; tracked rather than fixed here.

[scottschreckengaust]

Verdict: Request changes

The sizing half of this PR is excellent — modest defaults, deploy-context knobs, reserved-key guard, and an end-to-end reachability test (context → resolver → construct → template). The clientToken=taskId RunTask idempotency and the family-not-revision dispatch (toTaskDefinitionFamily) are both real robustness wins. The shared-role refactor (makeTaskDef, one taskRole/executionRole) is a genuine least-privilege improvement over two independently-granted defs.

But the PR's other headline capability — routing read-only workflows to the smaller planning task def — is inert at runtime: the ARN that selects it is never wired into the orchestrator Lambda, so readOnly planning tasks always fall back to the build def. That is half the PR's stated value shipping as a no-op, and no integration test would catch it. Blocking on that + a test that proves the wiring.

Vision alignment

Strongly aligned with bounded blast radius & cost (VISION.md). Rightsizing planning to 2 vCPU / 8 GB and de-opinionating the build default from 16 vCPU / 120 GB to a modest 4 vCPU / 16 GB inverts the cost surprise in the safe direction (a slow build is diagnosable and one prop from fixed; a bill is not). The shared-role design keeps the read-only planner from silently diverging in privilege. No tenet is traded; backing issues #247 and #299 are both approved / P0, tracked under epic #668.

Blocking issues

1. The planning task def is created and exposed but never wired to the orchestrator — read-only routing is a no-op. EcsComputeStrategy reads process.env.ECS_PLANNING_TASK_DEFINITION_ARN (cdk/src/handlers/shared/strategies/ecs-strategy.ts:51,155), but nothing sets it: TaskOrchestrator's ecsConfig (cdk/src/constructs/task-orchestrator.ts:152-160) has no planning field and its env block (:269-275) only emits ECS_TASK_DEFINITION_ARN; AgentStack (cdk/src/stacks/agent.ts:683-690) never references ecsCluster.planningTaskDefinition. Result: ECS_PLANNING_TASK_DEFINITION_ARN is always undefined, so readOnly && ECS_PLANNING_TASK_DEFINITION_ARN is always falsy and every task — including coding/decompose-v1 — runs on the build def. The planning def is synthesized (and registered) but never selected. The strategy unit tests only pass because they set the env var by hand via isolateModules; agent.test.ts asserts two task defs exist but never that the planning ARN reaches the Lambda, so the gap is invisible to the suite.

Fix: add planningTaskDefinitionArn: string to ecsConfig, pass ecsCluster.planningTaskDefinition.taskDefinitionArn from agent.ts, emit ECS_PLANNING_TASK_DEFINITION_ARN in the orchestrator env, and add an agent.test.ts assertion that the orchestrator Lambda's environment carries it (mirroring the existing ECS_TASK_DEFINITION_ARN wiring). If the wiring is deliberately deferred to a later carve slice, say so in the PR body and drop the strategy-side consumption from this slice so the two land together — shipping the consumer without the producer leaves dead config in the tree.

Non-blocking suggestions / nits

2. Stale sizing in docstrings after the de-opinionation. The class-field JSDoc still says The 64 GB / 16 vCPU BUILD task def (ecs-agent-cluster.ts:278) even though this PR lowered the build default to 4 vCPU / 16 GB. Same stale figure in compute-strategy.ts:55 ("the 64 GB build def") and orchestrator.ts:276 ("the context-gated 64GB ECS tier"). The prior review round explicitly flagged and claimed to fix stale docstrings; these three survived. Reword to "the larger BUILD task def (default 4 vCPU / 16 GB, raisable to the Fargate ceiling)".

3. MAX_TURNS fallback silently diverges (100 vs 200). ecs-strategy.ts:209 changed the inline default from 100 to 200, while DEFAULT_MAX_TURNS = 100 (validation.ts:34) still governs the hydrate path (orchestrator.ts:566). The two only disagree when payload.max_turns is absent, but there's no comment explaining the bump and it's out of band with this PR's stated scope. Either route both through DEFAULT_MAX_TURNS or add a one-line rationale.

Documentation

No docs/guides or docs/design change is included. The new deploy-context knobs (ecsBuildTaskCpu, ecsBuildTaskMemoryMiB, ecsBuildTaskEphemeralStorageGiB, ecsPlanningTaskCpu, ecsPlanningTaskMemoryMiB, ecsExtraBuildEnv) are documented only in an inline comment in agent.ts. For a public sample where sizing is now the operator's lever, these belong in the ECS/compute deployment guide alongside the compute_type=ecs gate. Non-blocking for this slice but worth a follow-up. No Starlight mirror edits needed (no docs/ sources touched). Backing issues #247/#299 exist and are approved; #668 tracks the epic.

Tests & CI

Sizing/override paths are well covered (resolver rejects malformed numbers, non-string env values, and reserved platform keys; end-to-end reachability asserted in agent.test.ts). The parity test (both defs share one task+execution role) is a strong regression guard. Gap: no test asserts the planning ARN is wired into the orchestrator env (Blocking #1) — the isolated-module tests set it manually, so they cannot catch the missing glue. CI: 4/4 checks green; the author documented (issue comment) that the deps-scan red is a pre-existing main advisory unrelated to this diff — verified plausible (no lockfile/manifest touched).

Bootstrap policy coverage: N/A. This PR adds a second AWS::ECS::TaskDefinition and two AWS::IAM::Roles — both existing CFN resource types already covered; no new resource type is introduced, so no bootstrap policy / action-map / BOOTSTRAP_VERSION bump is required.

Review agents run

• code-reviewer — ran (manual principal-architect pass; Semgrep MCP invocation failed on a .semgrep.yml config-deprecation error, so I fell back to targeted grep + manual review of the three core source files).
• silent-failure-hunter — ran. parseMergeBranches swallows malformed JSON but logs a warn and returns [] (documented best-effort) — acceptable, not a silent swallow. The planning-def fallback (Blocking #1) is a safe silent degradation, but it degrades the feature to a no-op invisibly — hence flagged.
• type-design-analyzer — ran. New EcsTaskSizing interface: all-optional fields with clear per-field defaults; readOnly? added to the ComputeStrategy.startSession contract with a documented default. Coherent, no over-abstraction.
• comment-analyzer — ran. Surfaced the three stale 64 GB references (Nit #2).
• pr-test-analyzer — ran. Surfaced the missing orchestrator-env wiring assertion (Blocking #1).
• security-review — ran (IAM/env change). Shared-role refactor is a least-privilege improvement; RESERVED_BUILD_ENV_KEYS correctly fails closed on attempts to override AGENT_SESSION_ROLE_ARN/table names via ecsExtraBuildEnv; clientToken prevents double-RunTask. No new wildcard actions.

Human heuristics

• Proportionality — Pass. makeTaskDef factory is justified (forces build/planning parity); no gratuitous abstraction.
• Coherence — Concern. "read-only planning" naming is now well-clarified in comments, but the feature it names does not actually route yet (ecs-strategy.ts:51), so the concept is present in code without a working path.
• Clarity — Concern. Stale 64 GB docstrings (ecs-agent-cluster.ts:278) contradict the code two lines from the new 4096/16384 defaults.
• Appropriateness — Concern. Tests assert what the isolated strategy does (env preset by hand) rather than what the deployed system should do (stack wires the ARN) — the AI005 failure mode.

[scottschreckengaust]

Blocking: nothing sets ECS_PLANNING_TASK_DEFINITION_ARN. TaskOrchestrator.ecsConfig (task-orchestrator.ts:152-160) has no planning field and its env block (:269-275) emits only ECS_TASK_DEFINITION_ARN; agent.ts:683-690 never references ecsCluster.planningTaskDefinition. So this constant is always undefined, the readOnly && ECS_PLANNING_TASK_DEFINITION_ARN guard at line 155 is always falsy, and read-only workflows never reach the planning def — the feature is inert. Wire the ARN through ecsConfig → orchestrator env and add an agent.test.ts assertion that the Lambda environment carries it (the isolated-module unit tests set it by hand, so they can't catch this).

[scottschreckengaust]

Stale: this PR lowered the build default to 4 vCPU / 16 GB, but the docstring still says "64 GB / 16 vCPU". Same stale figure in compute-strategy.ts:55 and orchestrator.ts:276. Reword to "the larger BUILD task def (default 4 vCPU / 16 GB, raisable to the Fargate ceiling via taskSizing)."

[scottschreckengaust]

Nit: the inline MAX_TURNS fallback changed 100 → 200, but DEFAULT_MAX_TURNS is still 100 (validation.ts:34) and governs the hydrate path (orchestrator.ts:566). The two disagree only when payload.max_turns is absent, but there's no comment explaining the bump. Either route both through DEFAULT_MAX_TURNS or add a one-line rationale.