fix(cdk): reclaim resource and byte headroom under both CFN ceilings (#852) - #854
Conversation
…852) The widest cell of the deploy gate — `compute_type=lambda-microvm` with the ADR-019 tool gateway on — synthesized 504 resources against CloudFormation's hard 500-resource template quota, so `main` could not emit a template for it at all (`TooManyResourcesInStack`). The same stack sat at ~100% of the 1 MB template-body ceiling, which CDK only *warns* about, so that limit was being ridden over silently. Two configuration changes, no architectural or API-contract change: - `allowTestInvoke: false` on all 34 `LambdaIntegration` call sites (-30 resources app-wide). The default emits a second `AWS::Lambda::Permission` per method scoped to `method.testMethodArn`, purely so the API Gateway console's "TEST" button works. Nothing here invokes it, and each one is an extra `lambda:InvokeFunction` grant. `scopePermissionToMethod` is left at its default `true`, so every route keeps its own narrow `SourceArn` — the two options are alternatives, not additive. - `suppressTemplateIndentation: true` on `AgentStack` (~31% fewer bytes, 0 resources). About a third of the emitted template was pretty-print indentation, all of it counted against the 1 MB ceiling. Set as a StackProp rather than the equivalent context key to keep the blast radius to the one stack near the ceiling; the two nested stacks stay pretty-printed. Measured with `cdk synth` into isolated output directories: default (agentcore) 480 / 961,200 B -> 450 / 640,377 B compute_type=ecs 492 / 996,129 B -> 462 / 662,670 B lambda-microvm + enableToolGateway 504 / THREW -> 474 / 687,955 B Closes neither ceiling permanently — it buys 26 resources of headroom so the structural work can happen under review instead of under a broken build. Tests close both gaps that let the broken cell stay green: the resource-budget block in agent.test.ts is the first to construct `lambda-microvm` and `enableToolGateway` together, and asserts no `test-invoke-stage` permission is ever emitted; main.test.ts asserts the emitted artifact carries no indentation and stays under CDK's own 80% warning threshold. Refs #852 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s, cover every compute type Review feedback on #854: - `main.ts` / `main.test.ts` / `agent.test.ts`: cut verbose rationale and the drift-prone figures (31% indentation, 30 permissions, 504 resources). Applied the same trim to the `task-api.ts` block, which carried the same numbers. - `agent.test.ts`: derive the budget as `MAX_RESOURCE_BUDGET - CUSHION` instead of hard-coding 490, and name the `cdk synth` delta `SYNTH_ONLY_RESOURCES`. - `agent.test.ts`: the budget block now runs every deploy-gate cell — all three compute types crossed with the tool gateway — rather than only the widest, so a regression confined to one substrate cannot hide behind the others. - `task-api.ts`: the comment named `task-api.test.ts`; the guard is in `test/stacks/agent.test.ts`. Refs #852 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
ayushtr-aws
left a comment
There was a problem hiding this comment.
Principal architect review — PR #854
Verdict: Approve with nits. The two configuration changes are correct, strictly narrow IAM, and move the widest deploy-gate cell from "cannot synthesize" to 26 resources of headroom. All affected suites pass locally on the merged branch (main.test.ts, stacks/agent.test.ts, constructs/task-api.test.ts, bootstrap/synth-coverage.test.ts — 187/187). The nits below are about the guards the PR adds over-claiming what they cover; none affects the deployed template.
Vision alignment
Fits. Tenet 5 (least privilege): 30 fewer lambda:InvokeFunction grants, every route keeps its per-method SourceArn. Tenet 6 (bounded cost/capacity): converts a fail-open INFO/WARN signal into a red test. Tenet 10 (honest sample): PR body is explicit that this is Tier 0 headroom, not the #852 fix. No tenet traded away, so no ADR needed.
Governance
- #852 now carries
approved+P0and is assigned — cleared under ADR-003. The PR body's Governance section is stale; please refresh it. - Branch
fix/resource-byte-headroomomits the issue number required by AGENTS.md. Rename tofix/852-resource-byte-headroom(the #679 hook will reject the current name once it lands).
Blocking issues
None.
Non-blocking (recommend fixing before merge, small)
cdk/test/stacks/agent.test.ts:1621— nested-stack routes are unguarded.Template.fromStack(AgentStack)returns only the root template;RegistryApi extends NestedStack, so its 4LambdaIntegrationsites are not covered by thetest-invoke-stageassertion, while the comment intask-api.ts:849-853says the test "asserts notest-invoke-stagepermission is ever emitted". Add the same offenders filter totest/constructs/registry-api.test.ts(or iterateNestedStackchildren in the budget block) and soften the task-api comment.cdk/test/stacks/agent.test.ts:1607—lambda-microvmcells synth withoutmicrovm_base_image_arn/microvm_base_image_version, soAWS::Lambda::MicrovmImageand theTerminateMicrovmgrant are omitted. The "widest cell" measured is the bootstrap no-image state (474), not the configured deployment (475). The existing microvm fixture at ~L1054 passes those keys for exactly this reason; add them to the microvm cells.cdk/test/main.test.ts:174— byte budget measured only on the default cell.buildAppacceptsappProps.context; parametrize with the same six-cell matrix so a byte regression confined to ECS/MicroVM/ToolGateway constructs cannot hide. (Branch numbers: agentcore/off ≈ 669 KB guarded, microvm/on ≈ 717 KB unguarded.)cdk/test/stacks/agent.test.ts:1583— header comment accuracy. The unchanged describe at ~L1518 already constructsenableToolGateway=true+compute_type=ecs, so "no test had ever constructed [them] together" is not true (the microvm + gateway combination was new). Also CDK's thrown message is "Number of resources in stack … is greater than allowed maximum of 500";TooManyResourcesInStackis an internal tag a reader will never see in output.cdk/test/stacks/agent.test.ts:1591—SYNTH_ONLY_RESOURCES = 1is a fudge forAWS::CDK::Metadata, which is absent only because the testAppnever setsaws:cdk:version-reporting. Setting that context key in the budget cells (and optionally@aws-cdk/core:stackResourceLimit, which CDK'smaxResourceshonors with a per-type breakdown) removes the constant.cdk/src/main.ts:96— nested stacks stay pretty-printed. Already acknowledged in the PR body; noting thatNestedStackdoes not inherit the prop andcdk.jsonhas no@aws-cdk/core:suppressTemplateIndentationcontext, so each nested stack has to rediscover this. Fine to defer to the #852 stack-split work.cdk/test/main.test.ts:166— the hand-copied1_000_000 × 0.8duplicates CDK's ownStack.templateSizewarning;Annotations.fromStack(stack).hasNoWarning('*', Match.stringLikeRegexp('Template size'))would reuse CDK's signal and cover nested stacks for free. Keep the raw read only for the indentation assertion.
Documentation
- No
docs/guides/docs/designchange was required (no contract, env var, or command change), so no Starlight mirror regen needed. - Suggest one bullet under
cdk/AGENTS.md"Common mistakes": newLambdaIntegrationsites must passallowTestInvoke: falseand new routes count against the resource-budget test. Optional: one operator-facing line that the API Gateway console "TEST" button no longer works by design. - Issue tracking: #852 (P0, approved) is the tracking issue; #851 is the incident record.
Tests & CI
- Locally green (see above). CI checks were still queued at review time.
- Bootstrap synth-coverage: not applicable — no new CFN resource types; the PR only removes
AWS::Lambda::Permissionresources. Suite still passes. - Test performance: budget block synths six cells once each in
beforeAll(no per-test synth, bundling stays disabled) — ~11 s for the two files. Acceptable.
Review agents run
/code-review(high) — source of nits 1–5 and 7; each spot-verified against the branch./security-review— no new vulnerabilities. Verified fromaws-cdk-lib2.261.0 source thatallowTestInvoke:falseonly drops thetest-invoke-stagepermission (scopePermissionToMethodbranch unchanged, per-methodSourceArnretained) and thatsuppressTemplateIndentationonly changesJSON.stringifyindent.- Omitted:
silent-failure-hunter(no error-handling code in diff),type-design-analyzer(no new types),comment-analyzer(folded into code-review nit 4),pr-test-analyzer(folded into code-review nits 1–3).
Human heuristics
- Proportionality — pass. Two flags plus two tests for a P0 blocker; no new abstraction.
- Coherence — pass. Same option at all 34 call sites; budget constants named consistently.
- Clarity — concern:
task-api.ts:849-853andagent.test.ts:1583claim coverage the tests do not provide (nits 1, 4). - Appropriateness — pass. Verified against real CDK behaviour (synth throws, permission ARNs), not only mocks; tests assert what the template should contain.
|
@ayushtr-aws — thank you, genuinely, for the thoroughness here. Verifying All of it has been recorded on the tracking issue — #852 (comment) — to be evaluated and remediated in follow-up PRs, grouped as:
Two governance points from your review are not deferrable and are noted separately on #852:
On the |
Two configuration changes that pull the stack back inside CloudFormation's two template ceilings, with no architectural change and no API-contract change.
The widest cell of the deploy gate —
compute_type=lambda-microvmwith the ADR-019 tool gateway on — could not synthesize onmainat all: 504 resources against the hard 500-resource template quota, which CDK enforces by throwingTooManyResourcesInStack. The same stack was also at ~100% of the 1 MB template-body ceiling, which CDK only warns about — so that limit was being ridden over silently.Area
cdk— infrastructure, handlers, constructsagent— Python runtime / Docker imagecli—bgagentclientdocs— guides or design sources (docs/guides/,docs/design/)tooling— rootmise.toml, scripts, CI workflowsRelated
Refs #852 — Tier 0 of the strategies catalogued there. This is deliberately not a fix for #852; it buys headroom so the structural work (stack split) can be done under review rather than under a broken build. Adjacent: #851, #735.
Changes
1.
allowTestInvoke: falseon all 34LambdaIntegrationcall sites — −30 resources app-wide.The default (
true) makes CDK emit a secondAWS::Lambda::Permissionper method, scoped tomethod.testMethodArn, so the API Gateway console's "TEST" button works. Nothing in this solution invokes it. Each one is also an extralambda:InvokeFunctiongrant, so this narrows the IAM surface as well as the resource count.Real traffic is unaffected.
scopePermissionToMethodstays at its defaulttrue, so every route keeps its own narrowly-scopedSourceArn. That distinction matters: the two options are alternatives, not additive — settingscopePermissionToMethod: falsesaves the same 30 resources but widens everySourceArnto.../{TaskApi}/*/*/*, which is why #852 rejects it.cdk/src/constructs/task-api.tscdk/src/constructs/slack-integration.tscdk/src/constructs/registry-api.tscdk/src/constructs/linear-integration.tscdk/src/constructs/jira-integration.tscdk/src/constructs/github-screenshot-integration.ts2.
suppressTemplateIndentation: trueonAgentStack(cdk/src/main.ts) — ~31% fewer bytes, 0 resources.Roughly a third of the emitted template was pretty-print indentation, every byte of which counted against the 1 MB ceiling. CDK's own
@aws-cdk/core:Stack.templateSizewarning names this prop as the remedy.Set as a
StackPropsvalue rather than via the@aws-cdk/core:suppressTemplateIndentationcontext key. Both work —Stackresolvesprops.suppressTemplateIndentation ?? node.tryGetContext(...) ?? false— but the prop keeps the blast radius to the one stack that is actually near the ceiling. Known limitation: the two nested stacks (AgentRegistryStack,RegistryApi) therefore stay pretty-printed. They are small, and only the context key would reach them.Measured
Resource counts and bytes are from
cdk synth(which emits oneAWS::CDK::MetadatathatTemplate.fromStackdoes not), each run into its own--outputdirectory.mainagentcore)compute_type=ecslambda-microvm+enableToolGateway— widestTooManyResourcesInStack, no template emittedEvery cell moves −30 resources exactly, and the widest one goes from "cannot synthesize" to 26 resources of headroom under the ceiling. The
enableToolGateway-only cell is still measuring and will be added; it is a flat +7 on whichever substrate it is combined with.CDK checks bytes against
TEMPLATE_BODY_MAXIMUM_SIZE = 1e6, not 1,048,576 — percentages above use1e6.Tests
Both gaps that let the broken cell stay green in CI are closed:
cdk/test/stacks/agent.test.ts— newAgentStack CloudFormation resource budget (#852)block. It is the first test to constructcompute_type=lambda-microvmandenableToolGateway=truetogether, which is precisely why the 504-resource cell was never red.Template.fromStackthrows if the cell is over the hard ceiling, so reaching the assertions is itself part of the guard; the budget is set to 490 so it fails as a readable assertion ten resources before synth starts throwing.AWS::Lambda::Permissionreferencingtest-invoke-stageis emitted anywhere in the stack, naming offending logical IDs. This is what stops a new route from silently reintroducing the 30 resources.cdk/test/main.test.ts— newbuildApp — CloudFormation template-body budget (#852)block, reading the emitted artifact (notTemplate.fromStack, which has already parsed indentation away). Asserts the template carries no indentation and stays under 800,000 bytes — the same 80% point at which CDK starts warning, so a regression trips here instead of riding the warning band up to the hard limit.Not in scope
defaultCorsPreflightOptions(task-api.ts) is left exactly as it was. Dropping it would free a further 34 resources, but unlike the two changes here it alters the API contract, so it is a product decision. It is held in reserve as a contingency of roughly the same size as this whole PR.SourceArnwidened to a wildcard. Adding that guard is a separate change; this PR does not widen anySourceArn.@aws-cdk/core:stackResourceLimitcontext budget. The built-in key already exists and is unset; wiring it is worth doing, but it applies one number to all stacks including nested ones, so it needs its own discussion.Governance
✅ Cleared under ADR-003. #852 now carries
approved+P0and is assigned; this PR is out of draft. (Superseded note: this section previously read that #852 lacked theapprovedlabel and an assignee, which was true when the PR was opened as a draft and is no longer.)fix/resource-byte-headroomomits the issue numberAGENTS.mdrequires ((feat|fix|chore|docs)/<issue-number>-short-description). Nothing rejects it today —scripts/hooks/check-branch-name.mjslands with #679, still open as of 2026-09-03 — but it would be rejected once that merges. GitHub cannot re-point an open PR's head ref, so correcting this means close-and-reopen rather than a rename; not doing that unilaterally to an approved, mergeable PR. Raised in review by @ayushtr-aws and recorded on #852.⬜ Review nits deferred, not dropped. @ayushtr-aws approved with 7 non-blocking nits; all are transcribed on #852 for follow-up PRs. None affects the deployed template — they concern the guards this PR adds over-claiming their coverage (notably
RegistryApi extends NestedStack, so its 4LambdaIntegrationsites are outside thetest-invoke-stageassertion).Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.
🤖 Generated with Claude Code