feat(bootstrap): resource-action-map for synth-time validation - #165
scottschreckengaust wants to merge 11 commits into
Conversation
d31fd4d to
d3a9804
Compare
Note: this branch currently sits on top of feat/bootstrap-template (#162). When #162 merges to main, I'll retarget and rebase per ADR-001 §8 — the scaffold commit |
ed0cf6b to
ab01560
Compare
d42d870 to
c5b7401
Compare
8a27b84 to
5bc41d6
Compare
a3fcb8f to
684817e
Compare
|
on it |
Review —
|
|
Question for reviewers: Option 1: Option 2: |
Replace comment toggle with proper context gate. ECS resources only synthesize when compute_type=ecs is passed. Default (agentcore) behavior unchanged. Closes #164 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…are policy selection Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Maps all CloudFormation resource types used by the ABCA stack to their required IAM actions per lifecycle phase (create/read/update/delete). Actions are sourced from CloudTrail-validated policies in DEPLOYMENT_ROLES.md. Tests validate structure, format, and policy coverage (with known gaps for SQS, S3 bucket lifecycle, and Lambda ESM/Layer actions documented). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Validates that all resource types in the synthesized CloudFormation template have entries in the resource-action-map. Tests agentcore from existing cdk.out and attempts ECS synth gracefully skipping when AWS credentials are unavailable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
compute_type drives which compute policy is needed — agentcore and ecs are independent choices, not base+optional. An operator deploying only ECS should not require agentcore permissions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The resource-action-map test previously synthesized into cdk/cdk.out.ecs/ inside the repo tree. CDK's AgentRuntimeArtifact.fromAsset(repoRoot) fingerprints the entire tree, so when github-tags.test runs in parallel it can stat synth.lock mid-lifecycle and hit ENOENT. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…e selection (#124) Addresses krokoko's blocking review on #165. The work lands on main's LIVE map (cdk/src/bootstrap/resource-action-map.ts, consumed by synth-coverage.test.ts) rather than the branch's parallel bootstrap/preflight/ copy — main grew its own map via #351 while this PR sat, and fixing the unreachable one would leave the real gate blind. B1 — ECS was a total validation blind spot. Neither map had any AWS::ECS::* entry, so compute-ecs.ts's 14 `ecs:*` grants were unverified. Confirmed against a real gated synth: `--context compute_type=ecs` emits exactly AWS::ECS::Cluster + AWS::ECS::TaskDefinition as unmapped. Both added, actions derived from compute-ecs.ts. B2 — the dual-config coverage check was vacuous. The original shelled out to `npx cdk synth` and swallowed every failure (`catch { return }`), plus bailed on `types.length === 0` — it burned ~86s, reported green, and asserted nothing. Replaced with an IN-PROCESS ECS-gated synth in synth-coverage.test.ts (no child process, so no try/catch to swallow), and an explicit toContain guard on the two ECS types so the check cannot pass vacuously if the gate ever stops provisioning. Mutation-tested both directions: removing the ECS map entries fails with the 2 unmapped types; hard-coding computeType to 'agentcore' (a silently broken gate) fails the toContain guard. The pre-fix version passed under both. Compute-type-aware selection — RFC #120's sufficiency model is `deployed PolicySet ⊇ the app's required set`, but collectBootstrapAllowActions called allPolicies() unconditionally, validating against the UNION of all five. An agentcore-only operator never deploys compute-ecs, so the union silently accepts `ecs:*` their real IaCRole cannot perform — the over-permissive direction. Added policiesForComputeType(), routed through the salvaged getRequiredBootstrapPolicies so selection cannot drift from the generated artifacts (fails loud on an unregistered name), and made the computeType argument OPTIONAL so the historical union behaviour is preserved for callers that want "grantable by some configuration". Verified scoping: union 357 actions (14 ecs:*), agentcore scope 343 (0 ecs:*), ecs scope 356 (14 ecs:*, 0 bedrock-agentcore:*). 178 suites / 3533 tests pass. B3 needs no work: #596 already landed the ECS-gate tests krokoko asked for (agent.test.ts:674 — cluster + both task-defs, ComputeSubstrate output, and the default no-gate case). Co-Authored-By: Claude <noreply@anthropic.com>
krokoko's non-blocking review point: the KNOWN_GAP_SERVICES/KNOWN_GAP_ACTIONS
exclusions were "genuine gaps — the stack creates those resources but no policy
grants the actions, so the test passes only by excluding the cases it most needs
to catch." Closing them by granting, rather than by keeping the exclusion.
- s3:GetBucketPolicy, s3:GetEncryptionConfiguration (observability, S3Application
Buckets). CloudFormation reads a bucket's policy and encryption config back on
stack UPDATE for drift/no-op detection, so the existing Put* grants are
insufficient alone. Every other Put* in that statement already had its Get*
pair; these two were the omissions.
- sqs:AddPermission, sqs:RemovePermission (application, SQS). AWS::SQS::Queue
Policy is a distinct CFN resource managed via Add/RemovePermission, NOT
SetQueueAttributes. The stack creates one (the DLQ redrive policy), so a
queue-policy create/update/delete would fail.
Verified all four resolve through actionIsAllowed after the change.
BOOTSTRAP_VERSION 1.2.0 -> 1.3.0 (additive grants, backward-compatible) with
artifacts regenerated via //cdk:bootstrap:generate, DEPLOYMENT_ROLES.md updated
to keep golden-baseline parity, and the Starlight mirror re-synced.
NOTE: BOOTSTRAP_HASH is byte-identical after adding four IAM actions, which is
wrong — computeBootstrapHash misuses JSON.stringify's replacer argument as a key
sort, so it digests `{}` for every statement and is blind to all actions.
Pre-existing on main (introduced with the hash in #122), filed as #732 rather
than fixed here to keep this PR reviewable.
178 suites / 3533 tests pass.
Co-Authored-By: Claude <noreply@anthropic.com>
ae4858d to
f782707
Compare
|
@krokoko Picking this back up — rebased onto One structural decision that changes where the fixes land. While this PR was idle, B1 — ECS blind spot: fixedConfirmed with a real gated synth: B2 — vacuous tests: fixed, and I confirmed your diagnosisYou were right that CI was hitting the skip path. The Rather than repair the shell-out, I took your "synthesize in-process" suggestion into expect([...typesInTemplate]).toContain('AWS::ECS::Cluster');
expect([...typesInTemplate]).toContain('AWS::ECS::TaskDefinition');Mutation-tested both directions, since a coverage test that cannot fail is the whole defect:
Both passed before the fix. B3 — already satisfied by #596
Non-blocking items
One thing found on the way — filed, not fixed here
Still to come on this PRDeepening the live map to full CRUD and retiring the duplicate data (keeping 178 suites / 3533 tests pass. |
… a facade (#124) Retires the duplicate map. Two copies existed: bootstrap/preflight/ carried CRUD depth with no production consumer, while bootstrap/resource-action-map.ts was create-only and wired into the live synth-coverage gate. Disjoint test suites and no shared consumer means they drift by construction, and adding a resource type to only one of them is silent. Merged programmatically, not by hand, with the invariant asserted mechanically: every action from the create-only map survives in the merged entry's `create` phase (verified 0 lost across 52 types). Result is 64 types / 430 actions, up from 52 create-only entries — the CRUD map contributed 48 create-phase actions the live map lacked on shared types, plus AWS::IAM::ManagedPolicy, while main's 6 extra types (CloudFront, Custom::*, CDK::Metadata) are preserved. - RESOURCE_ACTION_MAP is now Record<string, ResourceActions> with create/read/update/delete. findMissingBootstrapActions defaults to ['create'], preserving the pre-CRUD contract for existing callers; pass phases to widen. - bootstrap/preflight/resource-action-map.ts holds NO data — it re-exports the single map and keeps the query helpers (getActionsForResource, getAllMappedActions) that #125/#126 will read it through. 428 lines -> 62. - Deleted the two vacuous 'Synth coverage' tests here: both bailed silently (`catch { return }`, `types.length === 0`) and the ECS one burned ~86s asserting nothing. synth-coverage.test.ts now covers both configs in-process and fails loudly (previous commit). KNOWN_GAP_SERVICES / KNOWN_GAP_ACTIONS removed entirely. Verified every one of the 11 excluded actions is now covered — the sqs/s3 service-wide exclusions and all 7 lambda actions were stale, hiding nothing. With the 4 real gaps granted in the previous commit, the coverage assertion runs over ALL 430 actions in ALL four phases with zero exemptions, which is what krokoko asked for ("the test passes only by excluding the cases it most needs to catch"). Mutation-tested: revoking sqs:AddPermission fails with "1 actions not covered by bootstrap policies: sqs:AddPermission". Added structural pins so the depth cannot erode: every entry must declare all four phases as arrays, and >=45 entries must carry real update/delete actions. 178 suites / 3533 tests pass; //cdk:eslint clean, no mutations. Co-Authored-By: Claude <noreply@anthropic.com>
The new in-process ECS synth-coverage test builds the agent DockerImageAsset, which fingerprints the whole repo root. Jest workers create and evict `.jest-cache/jest-transform-cache-*/<n>/<name>_<hash>.map.<random>` entries throughout a run, so the fingerprint walk can hit a path another worker just deleted: Resolution error: ENOENT: no such file or directory, open '.../cdk/.jest-cache/jest-transform-cache-.../80/denytasktest_....map.588130630' Intermittent — it surfaced once in a full `mise run build` and did not reproduce across three cold-cache runs, which is exactly why it needs a structural fix rather than a retry. This is the same vanishing-file class .dockerignore already documents for pytest-cov's `.coverage.<host>.<pid>.<random>` temp files, with the same consequence. `.jest-cache` was in .gitignore but not .dockerignore, and .dockerignore is what CDK's fingerprint honours. Verified by synthesizing the stack and asserting no staged asset directory contains `cdk/.jest-cache` (5 asset dirs, none leaked). Full build now passes cdk 178 suites / 3533 tests and cli 55 / 695. Note: `//cdk:synth:quiet` still fails locally on `ec2:DescribeAvailabilityZones` — an IAM gap in my sandbox account, reproduced identically on a near-main branch, unrelated to this change. Co-Authored-By: Claude <noreply@anthropic.com>
|
Follow-up to the previous comment — the remaining work is done. The duplicate map is retired and the CRUD depth is now enforced rather than decorative. One map, on the live pathMerged programmatically, not by hand, with the safety invariant asserted mechanically: every action from the create-only map must survive in the merged entry's
The CRUD map contributed 48 create-phase actions the live map lacked on shared types, plus
|
|
Heads up before we go further on this one: the branch is conflicting with Happy to re-review deeply once it's rebased. Ping me. |
isadeks
left a comment
There was a problem hiding this comment.
Review — feat(bootstrap): resource-action-map for synth-time validation
Verdict: request changes (6 blocking). The direction is right and the work done since krokoko's review is substantial and honest — I re-verified all three of her blockers against the code rather than the write-up and they are genuinely closed, which is not something I say often about a re-review. Two things stop me signing off. First, the branch is 40 commits behind main and mergeable: CONFLICTING, and the merge is not mechanical: main has moved through this exact code four times since 2026-08-06 (#733, #739, #831, #867), and three of those collisions reduce coverage if the conflict is resolved by taking this branch's side. Second, the "compute variants are independent choices" model that this PR builds its new selection logic on does not match how the ECS gate actually behaves on main — the gate is additive, not exclusive, so the list an ECS operator is told to bootstrap is missing the AgentCore policy their own deploy needs.
A note on CI before anything else: with no computable merge commit, pull_request workflows cannot dispatch, so the absent checks here are not passing checks — nothing has run against this head. I could not run the suites either (no node_modules / agent/.venv in my review worktree), so every claim below comes from reading the code and from diffing this branch against current upstream/main (5e10038c). The only thing I take on trust is your reported 178 suites / 3533 tests, and that run predates 40 commits of main.
Blocking
B1 — getRequiredBootstrapPolicies('ecs') omits compute-agentcore, but the ECS gate is additive, so an operator who follows this list cannot deploy. required-policies.ts:26-29 treats agentcore and ecs as peer variants, and required-policies.test.ts:33 pins expect(result).not.toContain('compute-agentcore'). On main the gate does not swap substrates — it adds one. AgentMemory is constructed unconditionally (stacks/agent.ts:446), the stack's own output text says the gate "adds the Fargate substrate alongside AgentCore" (stacks/agent.ts:1139), and cdk/test/stacks/agent.test.ts:982-983 states it outright: "provisions the Fargate substrate alongside the always-present AgentCore runtime." So a compute_type=ecs deploy still creates AWS::BedrockAgentCore::Runtime and AWS::BedrockAgentCore::Memory. Scenario: an operator bootstraps the four policies this function names for ecs, runs cdk deploy -c compute_type=ecs, and the stack rolls back on bedrock-agentcore:CreateMemory AccessDenied — the exact reactive-permission-failure class RFC #120 exists to eliminate, now emitted by the tool meant to prevent it. The 0 bedrock-agentcore:* figure quoted as verification of the ecs scope is the defect rather than the evidence. The pre-71d06a56 shape was right about this one; the reviewer suggestion it responded to holds only if the substrates are mutually exclusive, and on main they are not. Suggest: ecs: ['compute-agentcore', 'compute-ecs'] (and the same question for lambda-microvm — check whether that gate is additive too), with the test inverted to assert the additive relationship and a comment recording why, since this is exactly the kind of thing that gets "simplified" back.
B2 — Rebase first; the merge is not mechanical. Merge base is d3974f7a; main is 5e10038c. Four collisions, in the order they bite:
(a) BOOTSTRAP_VERSION 1.3.0 is already published, for a different bundle. version.ts:25 takes 1.3.0; on main that number went to the compute-lambda-microvm policy (#645 / ADR-021) and the bundle is now at 1.6.0 with a per-bump changelog in the docstring. The version is an operator-visible contract (the CDKToolkit stack's BootstrapPolicyVersion output, which operators are told to >= check), so a second, different 1.3.0 makes two bundles indistinguishable. This needs to become 1.7.0 with a matching entry in that history.
(b) The rewritten map drops 14 things main added. This PR replaces RESOURCE_ACTION_MAP wholesale (Record<string, readonly string[]> → Record<string, ResourceActions>), so resolving in favour of the branch silently deletes every type main mapped in the interim — 13 entries plus one exemption:
AWS::BedrockAgentCore::Gateway AWS::SNS::Topic
AWS::BedrockAgentCore::GatewayTarget AWS::SNS::Subscription
AWS::CloudFormation::Stack AWS::StepFunctions::StateMachine
AWS::Cognito::UserPoolGroup Custom::AgentRegistry
AWS::KMS::Key Custom::CDKBucketDeployment
AWS::Lambda::MicrovmImage Custom::LinearWorkloadIdentity
AWS::Lambda::NetworkConnector
exempt set: AWS::SNS::TopicPolicy
They cannot be pasted back as create-only arrays either — this PR's own pin at cdk/test/bootstrap/resource-action-map.test.ts:77-87 requires all four phases on every entry. Each of those 13 needs CRUD authored, which is real work and is the argument for rebasing before re-review.
(c) There is a third compute substrate now, and the selection logic does not know it. COMPUTE_VARIANT_POLICIES enumerates only agentcore and ecs. main ships lambda-microvm with its own policy document (policies/compute-lambda-microvm.ts, in allPolicies()), its own gate (stacks/agent.ts:348), its own resource types, and its own test block (agent.test.ts:1058). Post-rebase, policiesForComputeType('lambda-microvm') returns core-only — B4 is why that is silent.
(d) The regenerated bootstrap-template.yaml is in the pre-#867 shape. #867 restructured that artifact to fit CloudFormation's inline body limit and added a hard generator budget (cdk/src/bootstrap/template-size.ts:62, TEMPLATE_SIZE_BUDGET = 49_152). The committed artifact here is ~50KB against main's ~46KB. Four actions is a couple hundred bytes and the headroom looks sufficient, so I expect this to be fine — but it must be regenerated and re-measured against the new bootstrap-template.test.ts budget assertion, not carried over.
B3 — This PR makes 11 of its 64 map entries unreachable by the synth-coverage gate, and main does not have this problem. CFN_TYPES_WITHOUT_EXEC_ROLE_IAM and RESOURCE_ACTION_MAP are disjoint on upstream/main by design — the first set means "needs no exec-role IAM". After the preflight merge in 58ad265 they overlap on eleven types: AWS::ApiGateway::Account, Deployment, Stage, AWS::EC2::SubnetRouteTableAssociation, VPCGatewayAttachment, AWS::Lambda::Alias, Permission, Version, AWS::Logs::ResourcePolicy, AWS::S3::BucketPolicy, AWS::SQS::QueuePolicy. Both loops continue on the skip set before the map lookup (cdk/test/bootstrap/synth-coverage.test.ts:58, :100), so those entries are dead on the live path. The sharpest instance is self-referential: the two SQS grants this PR adds exist specifically to make AWS::SQS::QueuePolicy deployable, the map now describes that type at resource-action-map.ts:588-593 — and the gate skips it because resource-action-map.ts:37 says it needs no IAM at all. Drop sqs:AddPermission from application.ts tomorrow and the synth gate stays green; your mutation test caught it via the union-coverage test at resource-action-map.test.ts:124, not via the gate. Fix: decide per type — either it needs actions and comes out of the exempt set, or it does not and the entry goes — then pin it, expect([...CFN_TYPES_WITHOUT_EXEC_ROLE_IAM].filter((t) => t in RESOURCE_ACTION_MAP)).toEqual([]), in the same spirit as the structural pins you already added.
B4 — An unrecognised compute type silently under-scopes, and a test pins that as intended. required-policies.ts:34 is if (variants) base.push(...variants); — an unknown or misspelled substrate yields the three core policies and no error, and required-policies.test.ts:45-50 asserts exactly that. This defeats the fail-loud guarantee documented at policies/index.ts:63-71: that throw can only fire for a name that is in the list but has no document, which is the drift that will not realistically happen; the drift that will — a substrate nobody registered — takes the silent path. Concretely, once B2(c) lands, collectBootstrapAllowActions('lambda-microvm') returns an allow-set with no compute grants at all and no signal the name was unrecognised. compute_type is a closed set on main, so I would make it one here: type the parameter as a union, or throw on an unknown name, rather than degrading to core.
B5 — The live gate still validates against the union, so the compute-type-aware selection is not load-bearing — and it would have caught B1. cdk/test/bootstrap/synth-coverage.test.ts:47 calls collectBootstrapAllowActions() with no argument in beforeAll, and the new ECS-gated test reuses that union set at :107. policiesForComputeType and the computeType parameter have zero non-test callers, so RFC #120's deployed ⊇ required model is exercised only by required-policies.test.ts's own unit tests and the drift gate behaves exactly as before. This is not just an unused capability: passing 'ecs' into the ECS-gated check would have failed on the bedrock-agentcore:* actions that B1 leaves out of the ECS bootstrap list. Fix: compute the allow-set per test — collectBootstrapAllowActions('agentcore') for the default synth, ('ecs') for the gated one — which makes the new function load-bearing, turns B4 into a caught error, and gives B1 a regression test in one move.
B6 — The two ECS entries this PR was blocked on are create-only, so 12 of the 14 ecs:* grants stay unverified. resource-action-map.ts:324-335 gives AWS::ECS::Cluster and AWS::ECS::TaskDefinition read: [], update: [], delete: []. compute-ecs.ts:35-48 grants the full lifecycle — ecs:DeleteCluster, ecs:DeregisterTaskDefinition, ecs:DescribeClusters, ecs:UpdateCluster, ecs:UpdateClusterSettings, ecs:PutClusterCapacityProviders, ecs:UntagResource, ecs:ListTagsForResource — and krokoko's suggested fix spelled those phases out. This is a validation hole rather than a deploy failure: trim ecs:DeleteCluster and nothing fails, on a substrate whose stack-delete path is precisely the class the CRUD shape is argued to catch. It also reads oddly against the thesis — the pin at resource-action-map.test.ts:89-99 demands update/delete depth on 45 entries, and the two newest entries opt out. Five of the seven create-only entries are Custom::*/CloudFront inherited from main; ECS is the pair this PR authored. Fill the phases from compute-ecs.ts.
Prior review — verified, and closed
| krokoko blocker | state |
|---|---|
| B1 ECS types missing | fixed, and still needed |
| B2 vacuous coverage tests | fixed, convincingly |
| B3 no ECS-gate test | already on main |
B1: upstream/main's map still has no AWS::ECS::* entry, so this is not a stale complaint — compute-ecs.ts's 14 ecs:* grants are unverified on main today, and resource-action-map.ts:324-335 closes that (partially — see B6). B2: the shell-out and its catch { return } are gone; the in-process synth at synth-coverage.test.ts:82-95 has no exit code to swallow, and the toContain guards mean it cannot pass vacuously if the gate regresses. Right call, and the mutation evidence in both directions is exactly what I want to see on a test whose whole job is to fail. B3: genuinely satisfied on main — agent.test.ts:978-1000 (cluster count, both task defs, ComputeSubstrate=ecs) plus the default no-gate case at :148-151; note main now provisions two task definitions, so any assertion you carry forward should expect 2. The non-blocking KNOWN_GAP_* point is properly closed too: the exclusion sets are gone entirely, the .ts sources and generated .json artifacts agree, and closing a gap by granting rather than excluding is the right resolution — with one caveat in the nits below about which gap the SQS pair actually closes.
Your #732 disclosure also still holds: computeBootstrapHash on current main is unchanged and still passes Object.keys(json).sort() as JSON.stringify's replacer, so Statement elements digest to {} and the hash is blind to every action. Filing rather than fixing was right. Worth noting this PR is the first bundle change where that blindness has an operator-visible consequence — an operator whose drift check keys on BootstrapPolicyHash sees no change and does not re-bootstrap, so their IaCRole never gets the four new actions. The BootstrapPolicyVersion bump still carries the signal, so it degrades rather than breaks; but #732 should land before #125/#126 build on that digest.
Non-blocking
policies/application.ts:213-217— the justification is wrong on both halves, and the grant may be unnecessary. TheAWS::SQS::QueuePolicyresources come fromenforceSSL: trueon the queues (fanout-consumer.ts,orchestration-reconciler.ts,approval-metrics-publisher-consumer.ts,github-screenshot-integration.ts), not "the DLQ redrive policy" — redrive is an attribute of the queue itself, not a separate resource. AndenforceSSLemits aDenystatement, whichsqs:AddPermissioncannot express at all, soSetQueueAttributes(already granted) is what CloudFormation has to use — which is presumably whymainputAWS::SQS::QueuePolicyin the no-exec-role-IAM set in the first place. I can't observe CFN's internal calls from here, so I'll put it as a question: can you re-derive this pair from CloudTrail on a real deploy? IfSetQueueAttributesis what fires, the least-privilege answer is to drop both grants and the map entry and keep the exemption, rather than widening the IaCRole for a resource that never needed it. (s3:GetBucketPolicy/s3:GetEncryptionConfigurationI have no such doubt about — the Put/Get-pair reasoning there is sound.)resource-action-map.ts:547-574— by that same Put/Get-pair rationale,AWS::S3::Bucketomits lifecycle from all four phases while four constructs setlifecycleRules.s3:PutLifecycleConfigurationis granted (observability.ts:137) but unmapped, so a trim would pass the gate;s3:GetLifecycleConfigurationis granted nowhere, which is the same missing-Get*defect one bucket property over from the two you just fixed.resource-action-map.test.ts:70-73— thethrowafterexpect(hasCreateOrDelete).toBe(true)is still dead (expectaborts first). krokoko flagged this; the sibling at:113-118got fixed by reordering, this one did not.resource-action-map.ts:690—phases.flatMap((phase) => entry[phase])on an entry missing a phase putsundefinedinside the action list, which poisonsgetAllMappedActions()and then throws inaction.split(':')somewhere unrelated. The comment atresource-action-map.test.ts:78-80says such an entry would be "silently skipped"; it would actually crash.?? []plus a comment correction — the pin is good, the described failure mode is wrong.- The
>= 55/>= 45thresholds (:63,:98) go slack once B2(b) adds 13 entries. Considercount - 1so they keep ratcheting. .dockerignore— the.jest-cacheexclusion is correct and the comment explains the vanishing-file class well.cdk/cdk.out.*/looks vestigial now that the ECS synth is in-process; harmless, but it no longer has a producer.preflight/as a directory holding a 62-line re-export plus two three-line helpers is thin for its own package boundary. It reads fine as the declared seam for #125/#126 — worth one sentence in the module docstring saying that is the only reason it is not folded intobootstrap/.- PR body is stale in three ways: the
#164ECS gate is already onmain, soCloses #164no longer describes this diff (#164 can be closed on its own); the "Open questions — SQS gap" is resolved; and every deliverable checkbox is still unticked. Please rewrite it after the rebase so a cold reviewer is not reconstructing scope from six comments.
Docs
Correct and complete for what the diff does. docs/design/DEPLOYMENT_ROLES.md carries all four new actions for golden-baseline parity, and the Starlight mirror (docs/src/content/docs/architecture/Deployment-roles.md) is regenerated in the same PR with a matching diff — no stale-mirror mutation failure. Both need re-running after the rebase, since main added ~140 lines to that baseline in the interim. Governance is fine: #120 is the approved RFC and #124 is the backing issue; branch naming is a waived nit here.
Tests & CI
Test design is strong, and I want to be clear that the B3–B6 findings are about placement rather than effort: the two-directional mutation testing, the zero-exemption union assertion, and the structural pins are the right instincts for a file whose only job is to fail when someone forgets something. I could not execute anything (no node_modules), so this is a read-only assessment plus your reported local run — and no CI has run against this head, per the conflict. ADR-002's checklist is satisfied on this branch's terms: policy sources, generated policies/*.json, bootstrap-template.yaml, BOOTSTRAP_VERSION, and the golden baseline all move together; BOOTSTRAP_HASH is the one artifact that did not, and #732 explains why. Bootstrap synth-coverage: cannot be executed here, and post-rebase it will fail until B2(b) is resolved, since main synthesizes 13 types this map no longer knows.
Review agents run
code-reviewer, silent-failure-hunter and pr-test-analyzer scope, run over the d3974f7a..HEAD range; B1, B3 and the two comment-accuracy items came out of that pass and I re-verified each against upstream/main before writing it here. Omitted: type-design-analyzer — the one new type, ResourceActions, is a four-field record already pinned structurally by test; /security-review as a separate pass — the IAM delta is four read/permission-management actions on existing ARN-scoped statements, reviewed directly. On that: sqs:AddPermission does let the IaC role grant cross-account access to those queues, but it is scoped to arn:aws:sqs:*:*:backgroundagent-dev-*; my concern there is the necessity question in the nits, not the scope.
Human heuristics
- Proportionality — pass. Retiring the duplicate map rather than maintaining two is the proportionate move, and the CRUD shape is justified by the seven reactive permission fixes it would have caught.
- Coherence — concern. B3 is a coherence failure: two sets
mainkeeps disjoint now disagree about eleven types and no test notices. B1 is the deeper one — the policy layer models the substrates as exclusive while the stack layer treats them as additive. - Clarity — concern at
required-policies.ts:34(B4), where the code degrades exactly where its own docstring promises to fail loud, and atapplication.ts:213-217, where a comment written to explain a new grant misdescribes the resource that motivated it. - Appropriateness — concern, and it is B2. This map is a statement about what
mainsynthesizes; a version of it that is 40 commits stale is wrong in the one way this file cannot afford. Rebase, re-derive against a current synth, fix B1, and I will re-review deeply — the substance is good and I want it to land.
|
|
||
| const COMPUTE_VARIANT_POLICIES: Record<string, string[]> = { | ||
| agentcore: ['compute-agentcore'], | ||
| ecs: ['compute-ecs'], |
There was a problem hiding this comment.
B1 (blocking). The ECS gate on main is additive, not exclusive — AgentMemory is unconditional (stacks/agent.ts:446) and agent.test.ts:982 says "alongside the always-present AgentCore runtime". So an ecs deploy still creates AWS::BedrockAgentCore::Runtime/Memory, and an operator who bootstraps exactly this list rolls back on bedrock-agentcore:CreateMemory AccessDenied. Suggest ecs: ['compute-agentcore', 'compute-ecs'], with required-policies.test.ts:33 inverted to assert the additive relationship. Same question for lambda-microvm, which main added while this sat.
|
|
||
| /** Semantic version of the bootstrap policy bundle. */ | ||
| export const BOOTSTRAP_VERSION = '1.2.0'; | ||
| export const BOOTSTRAP_VERSION = '1.3.0'; |
There was a problem hiding this comment.
B2a (blocking). 1.3.0 is already published on main — it went to the compute-lambda-microvm policy (#645 / ADR-021) and the bundle is now at 1.6.0 with a per-bump changelog in this docstring. BootstrapPolicyVersion is an operator-visible >= contract, so two different bundles cannot share a number. This needs to be 1.7.0 with a matching history entry after the rebase.
| unmapped.push(cfnType); | ||
| continue; | ||
| } | ||
| const missing = findMissingBootstrapActions(cfnType, allowedActions); |
There was a problem hiding this comment.
B5 (blocking). allowedActions is the union (collectBootstrapAllowActions() with no argument, :47), so the ECS-gated check validates ECS-path resources against grants an ECS-only operator never deploys — the over-permissive direction this PR says it closes. policiesForComputeType has no non-test caller as a result. Computing the set per test (('agentcore') above, ('ecs') here) makes the new function load-bearing and would have failed on the bedrock-agentcore:* actions B1 leaves out.
| // SetQueueAttributes. The stack creates one (the DLQ redrive policy), | ||
| // so without these a queue-policy create/update/delete fails | ||
| // (#124 review — previously excluded via KNOWN_GAP rather than granted). | ||
| 'sqs:AddPermission', |
There was a problem hiding this comment.
Nit, but please re-derive before granting: the AWS::SQS::QueuePolicy resources come from enforceSSL: true on the queues, not the DLQ redrive policy (redrive is a queue attribute, not a separate resource). enforceSSL emits a Deny statement, which sqs:AddPermission cannot express — so SetQueueAttributes, already granted, is presumably what CFN fires, which is why main has AWS::SQS::QueuePolicy in CFN_TYPES_WITHOUT_EXEC_ROLE_IAM. Can you confirm from CloudTrail on a real deploy? If it is SetQueueAttributes, the least-privilege answer is to drop both grants and the map entry.
isadeks
left a comment
There was a problem hiding this comment.
Review — feat(bootstrap): resource-action-map for synth-time validation
Verdict: request changes (6 blocking). The direction is right and the work done since krokoko's review is substantial and honest — I re-verified all three of her blockers against the code rather than the write-up and they are genuinely closed, which is not something I say often about a re-review. Two things stop me signing off. First, the branch is 40 commits behind main and mergeable: CONFLICTING, and the merge is not mechanical: main has moved through this exact code four times since 2026-08-06 (#733, #739, #831, #867), and three of those collisions reduce coverage if the conflict is resolved by taking this branch's side. Second, the "compute variants are independent choices" model that this PR builds its new selection logic on does not match how the ECS gate actually behaves on main — the gate is additive, not exclusive, so the list an ECS operator is told to bootstrap is missing the AgentCore policy their own deploy needs.
A note on CI before anything else: with no computable merge commit, pull_request workflows cannot dispatch, so the absent checks here are not passing checks — nothing has run against this head. I could not run the suites either (no node_modules / agent/.venv in my review worktree), so every claim below comes from reading the code and from diffing this branch against current upstream/main (5e10038c). The only thing I take on trust is your reported 178 suites / 3533 tests, and that run predates 40 commits of main.
Blocking
B1 — getRequiredBootstrapPolicies('ecs') omits compute-agentcore, but the ECS gate is additive, so an operator who follows this list cannot deploy. required-policies.ts:26-29 treats agentcore and ecs as peer variants, and required-policies.test.ts:33 pins expect(result).not.toContain('compute-agentcore'). On main the gate does not swap substrates — it adds one. AgentMemory is constructed unconditionally (stacks/agent.ts:446), the stack's own output text says the gate "adds the Fargate substrate alongside AgentCore" (stacks/agent.ts:1139), and cdk/test/stacks/agent.test.ts:982-983 states it outright: "provisions the Fargate substrate alongside the always-present AgentCore runtime." So a compute_type=ecs deploy still creates AWS::BedrockAgentCore::Runtime and AWS::BedrockAgentCore::Memory. Scenario: an operator bootstraps the four policies this function names for ecs, runs cdk deploy -c compute_type=ecs, and the stack rolls back on bedrock-agentcore:CreateMemory AccessDenied — the exact reactive-permission-failure class RFC #120 exists to eliminate, now emitted by the tool meant to prevent it. The 0 bedrock-agentcore:* figure quoted as verification of the ecs scope is the defect rather than the evidence. The pre-71d06a56 shape was right about this one; the reviewer suggestion it responded to holds only if the substrates are mutually exclusive, and on main they are not. Suggest: ecs: ['compute-agentcore', 'compute-ecs'] (and the same question for lambda-microvm — check whether that gate is additive too), with the test inverted to assert the additive relationship and a comment recording why, since this is exactly the kind of thing that gets "simplified" back.
B2 — Rebase first; the merge is not mechanical. Merge base is d3974f7a; main is 5e10038c. Four collisions, in the order they bite:
(a) BOOTSTRAP_VERSION 1.3.0 is already published, for a different bundle. version.ts:25 takes 1.3.0; on main that number went to the compute-lambda-microvm policy (#645 / ADR-021) and the bundle is now at 1.6.0 with a per-bump changelog in the docstring. The version is an operator-visible contract (the CDKToolkit stack's BootstrapPolicyVersion output, which operators are told to >= check), so a second, different 1.3.0 makes two bundles indistinguishable. This needs to become 1.7.0 with a matching entry in that history.
(b) The rewritten map drops 14 things main added. This PR replaces RESOURCE_ACTION_MAP wholesale (Record<string, readonly string[]> → Record<string, ResourceActions>), so resolving in favour of the branch silently deletes every type main mapped in the interim — 13 entries plus one exemption:
AWS::BedrockAgentCore::Gateway AWS::SNS::Topic
AWS::BedrockAgentCore::GatewayTarget AWS::SNS::Subscription
AWS::CloudFormation::Stack AWS::StepFunctions::StateMachine
AWS::Cognito::UserPoolGroup Custom::AgentRegistry
AWS::KMS::Key Custom::CDKBucketDeployment
AWS::Lambda::MicrovmImage Custom::LinearWorkloadIdentity
AWS::Lambda::NetworkConnector
exempt set: AWS::SNS::TopicPolicy
They cannot be pasted back as create-only arrays either — this PR's own pin at cdk/test/bootstrap/resource-action-map.test.ts:77-87 requires all four phases on every entry. Each of those 13 needs CRUD authored, which is real work and is the argument for rebasing before re-review.
(c) There is a third compute substrate now, and the selection logic does not know it. COMPUTE_VARIANT_POLICIES enumerates only agentcore and ecs. main ships lambda-microvm with its own policy document (policies/compute-lambda-microvm.ts, in allPolicies()), its own gate (stacks/agent.ts:348), its own resource types, and its own test block (agent.test.ts:1058). Post-rebase, policiesForComputeType('lambda-microvm') returns core-only — B4 is why that is silent.
(d) The regenerated bootstrap-template.yaml is in the pre-#867 shape. #867 restructured that artifact to fit CloudFormation's inline body limit and added a hard generator budget (cdk/src/bootstrap/template-size.ts:62, TEMPLATE_SIZE_BUDGET = 49_152). The committed artifact here is ~50KB against main's ~46KB. Four actions is a couple hundred bytes and the headroom looks sufficient, so I expect this to be fine — but it must be regenerated and re-measured against the new bootstrap-template.test.ts budget assertion, not carried over.
B3 — This PR makes 11 of its 64 map entries unreachable by the synth-coverage gate, and main does not have this problem. CFN_TYPES_WITHOUT_EXEC_ROLE_IAM and RESOURCE_ACTION_MAP are disjoint on upstream/main by design — the first set means "needs no exec-role IAM". After the preflight merge in 58ad265 they overlap on eleven types: AWS::ApiGateway::Account, Deployment, Stage, AWS::EC2::SubnetRouteTableAssociation, VPCGatewayAttachment, AWS::Lambda::Alias, Permission, Version, AWS::Logs::ResourcePolicy, AWS::S3::BucketPolicy, AWS::SQS::QueuePolicy. Both loops continue on the skip set before the map lookup (cdk/test/bootstrap/synth-coverage.test.ts:58, :100), so those entries are dead on the live path. The sharpest instance is self-referential: the two SQS grants this PR adds exist specifically to make AWS::SQS::QueuePolicy deployable, the map now describes that type at resource-action-map.ts:588-593 — and the gate skips it because resource-action-map.ts:37 says it needs no IAM at all. Drop sqs:AddPermission from application.ts tomorrow and the synth gate stays green; your mutation test caught it via the union-coverage test at resource-action-map.test.ts:124, not via the gate. Fix: decide per type — either it needs actions and comes out of the exempt set, or it does not and the entry goes — then pin it, expect([...CFN_TYPES_WITHOUT_EXEC_ROLE_IAM].filter((t) => t in RESOURCE_ACTION_MAP)).toEqual([]), in the same spirit as the structural pins you already added.
B4 — An unrecognised compute type silently under-scopes, and a test pins that as intended. required-policies.ts:34 is if (variants) base.push(...variants); — an unknown or misspelled substrate yields the three core policies and no error, and required-policies.test.ts:45-50 asserts exactly that. This defeats the fail-loud guarantee documented at policies/index.ts:63-71: that throw can only fire for a name that is in the list but has no document, which is the drift that will not realistically happen; the drift that will — a substrate nobody registered — takes the silent path. Concretely, once B2(c) lands, collectBootstrapAllowActions('lambda-microvm') returns an allow-set with no compute grants at all and no signal the name was unrecognised. compute_type is a closed set on main, so I would make it one here: type the parameter as a union, or throw on an unknown name, rather than degrading to core.
B5 — The live gate still validates against the union, so the compute-type-aware selection is not load-bearing — and it would have caught B1. cdk/test/bootstrap/synth-coverage.test.ts:47 calls collectBootstrapAllowActions() with no argument in beforeAll, and the new ECS-gated test reuses that union set at :107. policiesForComputeType and the computeType parameter have zero non-test callers, so RFC #120's deployed ⊇ required model is exercised only by required-policies.test.ts's own unit tests and the drift gate behaves exactly as before. This is not just an unused capability: passing 'ecs' into the ECS-gated check would have failed on the bedrock-agentcore:* actions that B1 leaves out of the ECS bootstrap list. Fix: compute the allow-set per test — collectBootstrapAllowActions('agentcore') for the default synth, ('ecs') for the gated one — which makes the new function load-bearing, turns B4 into a caught error, and gives B1 a regression test in one move.
B6 — The two ECS entries this PR was blocked on are create-only, so 12 of the 14 ecs:* grants stay unverified. resource-action-map.ts:324-335 gives AWS::ECS::Cluster and AWS::ECS::TaskDefinition read: [], update: [], delete: []. compute-ecs.ts:35-48 grants the full lifecycle — ecs:DeleteCluster, ecs:DeregisterTaskDefinition, ecs:DescribeClusters, ecs:UpdateCluster, ecs:UpdateClusterSettings, ecs:PutClusterCapacityProviders, ecs:UntagResource, ecs:ListTagsForResource — and krokoko's suggested fix spelled those phases out. This is a validation hole rather than a deploy failure: trim ecs:DeleteCluster and nothing fails, on a substrate whose stack-delete path is precisely the class the CRUD shape is argued to catch. It also reads oddly against the thesis — the pin at resource-action-map.test.ts:89-99 demands update/delete depth on 45 entries, and the two newest entries opt out. Five of the seven create-only entries are Custom::*/CloudFront inherited from main; ECS is the pair this PR authored. Fill the phases from compute-ecs.ts.
Prior review — verified, and closed
| krokoko blocker | state |
|---|---|
| B1 ECS types missing | fixed, and still needed |
| B2 vacuous coverage tests | fixed, convincingly |
| B3 no ECS-gate test | already on main |
B1: upstream/main's map still has no AWS::ECS::* entry, so this is not a stale complaint — compute-ecs.ts's 14 ecs:* grants are unverified on main today, and resource-action-map.ts:324-335 closes that (partially — see B6). B2: the shell-out and its catch { return } are gone; the in-process synth at synth-coverage.test.ts:82-95 has no exit code to swallow, and the toContain guards mean it cannot pass vacuously if the gate regresses. Right call, and the mutation evidence in both directions is exactly what I want to see on a test whose whole job is to fail. B3: genuinely satisfied on main — agent.test.ts:978-1000 (cluster count, both task defs, ComputeSubstrate=ecs) plus the default no-gate case at :148-151; note main now provisions two task definitions, so any assertion you carry forward should expect 2. The non-blocking KNOWN_GAP_* point is properly closed too: the exclusion sets are gone entirely, the .ts sources and generated .json artifacts agree, and closing a gap by granting rather than excluding is the right resolution — with one caveat in the nits below about which gap the SQS pair actually closes.
Your #732 disclosure also still holds: computeBootstrapHash on current main is unchanged and still passes Object.keys(json).sort() as JSON.stringify's replacer, so Statement elements digest to {} and the hash is blind to every action. Filing rather than fixing was right. Worth noting this PR is the first bundle change where that blindness has an operator-visible consequence — an operator whose drift check keys on BootstrapPolicyHash sees no change and does not re-bootstrap, so their IaCRole never gets the four new actions. The BootstrapPolicyVersion bump still carries the signal, so it degrades rather than breaks; but #732 should land before #125/#126 build on that digest.
Non-blocking
policies/application.ts:213-217— the justification is wrong on both halves, and the grant may be unnecessary. TheAWS::SQS::QueuePolicyresources come fromenforceSSL: trueon the queues (fanout-consumer.ts,orchestration-reconciler.ts,approval-metrics-publisher-consumer.ts,github-screenshot-integration.ts), not "the DLQ redrive policy" — redrive is an attribute of the queue itself, not a separate resource. AndenforceSSLemits aDenystatement, whichsqs:AddPermissioncannot express at all, soSetQueueAttributes(already granted) is what CloudFormation has to use — which is presumably whymainputAWS::SQS::QueuePolicyin the no-exec-role-IAM set in the first place. I can't observe CFN's internal calls from here, so I'll put it as a question: can you re-derive this pair from CloudTrail on a real deploy? IfSetQueueAttributesis what fires, the least-privilege answer is to drop both grants and the map entry and keep the exemption, rather than widening the IaCRole for a resource that never needed it. (s3:GetBucketPolicy/s3:GetEncryptionConfigurationI have no such doubt about — the Put/Get-pair reasoning there is sound.)resource-action-map.ts:547-574— by that same Put/Get-pair rationale,AWS::S3::Bucketomits lifecycle from all four phases while four constructs setlifecycleRules.s3:PutLifecycleConfigurationis granted (observability.ts:137) but unmapped, so a trim would pass the gate;s3:GetLifecycleConfigurationis granted nowhere, which is the same missing-Get*defect one bucket property over from the two you just fixed.resource-action-map.test.ts:70-73— thethrowafterexpect(hasCreateOrDelete).toBe(true)is still dead (expectaborts first). krokoko flagged this; the sibling at:113-118got fixed by reordering, this one did not.resource-action-map.ts:690—phases.flatMap((phase) => entry[phase])on an entry missing a phase putsundefinedinside the action list, which poisonsgetAllMappedActions()and then throws inaction.split(':')somewhere unrelated. The comment atresource-action-map.test.ts:78-80says such an entry would be "silently skipped"; it would actually crash.?? []plus a comment correction — the pin is good, the described failure mode is wrong.- The
>= 55/>= 45thresholds (:63,:98) go slack once B2(b) adds 13 entries. Considercount - 1so they keep ratcheting. .dockerignore— the.jest-cacheexclusion is correct and the comment explains the vanishing-file class well.cdk/cdk.out.*/looks vestigial now that the ECS synth is in-process; harmless, but it no longer has a producer.preflight/as a directory holding a 62-line re-export plus two three-line helpers is thin for its own package boundary. It reads fine as the declared seam for #125/#126 — worth one sentence in the module docstring saying that is the only reason it is not folded intobootstrap/.- PR body is stale in three ways: the
#164ECS gate is already onmain, soCloses #164no longer describes this diff (#164 can be closed on its own); the "Open questions — SQS gap" is resolved; and every deliverable checkbox is still unticked. Please rewrite it after the rebase so a cold reviewer is not reconstructing scope from six comments.
Docs
Correct and complete for what the diff does. docs/design/DEPLOYMENT_ROLES.md carries all four new actions for golden-baseline parity, and the Starlight mirror (docs/src/content/docs/architecture/Deployment-roles.md) is regenerated in the same PR with a matching diff — no stale-mirror mutation failure. Both need re-running after the rebase, since main added ~140 lines to that baseline in the interim. Governance is fine: #120 is the approved RFC and #124 is the backing issue; branch naming is a waived nit here.
Tests & CI
Test design is strong, and I want to be clear that the B3–B6 findings are about placement rather than effort: the two-directional mutation testing, the zero-exemption union assertion, and the structural pins are the right instincts for a file whose only job is to fail when someone forgets something. I could not execute anything (no node_modules), so this is a read-only assessment plus your reported local run — and no CI has run against this head, per the conflict. ADR-002's checklist is satisfied on this branch's terms: policy sources, generated policies/*.json, bootstrap-template.yaml, BOOTSTRAP_VERSION, and the golden baseline all move together; BOOTSTRAP_HASH is the one artifact that did not, and #732 explains why. Bootstrap synth-coverage: cannot be executed here, and post-rebase it will fail until B2(b) is resolved, since main synthesizes 13 types this map no longer knows.
Review agents run
Ran the repo's code-review agent over the d3974f7a..HEAD range (code-reviewer / pr-test-analyzer / silent-failure-hunter scope); its pass had not returned by the time I finished, so to be straight with you: every finding above is one I verified myself, mechanically where the claim is set-shaped — the exempt-set/map overlap in B2, the 14 dropped types in B1(b), and the create-only ECS entries in B5 were each computed by diffing this branch's declarations against upstream/main's, not eyeballed. Omitted: type-design-analyzer (the one new type, ResourceActions, is a four-field record already pinned structurally by test), comment-analyzer (folded into the read — one inaccuracy found, noted in the nits), and /security-review as a separate pass: the IAM delta is four read/permission-management actions on existing ARN-scoped statements, which I reviewed directly. On that — sqs:AddPermission does let the IaC role grant cross-account access to those queues, but it is scoped to arn:aws:sqs:*:*:backgroundagent-dev-* and is the only way CloudFormation can manage AWS::SQS::QueuePolicy, so it is inherent to the resource rather than a widening.
Human heuristics
- Proportionality — pass. Retiring the duplicate map rather than maintaining two is the proportionate move, and the CRUD shape is justified by the seven reactive permission fixes it would have caught.
- Coherence — concern. B3 is a coherence failure: two sets
mainkeeps disjoint now disagree about eleven types and no test notices. B1 is the deeper one — the policy layer models the substrates as exclusive while the stack layer treats them as additive. - Clarity — concern at
required-policies.ts:34(B4), where the code degrades exactly where its own docstring promises to fail loud, and atapplication.ts:213-217, where a comment written to explain a new grant misdescribes the resource that motivated it. - Appropriateness — concern, and it is B2. This map is a statement about what
mainsynthesizes; a version of it that is 40 commits stale is wrong in the one way this file cannot afford. Rebase, re-derive against a current synth, fix B1, and I will re-review deeply — the substance is good and I want it to land.
|
|
||
| /** Semantic version of the bootstrap policy bundle. */ | ||
| export const BOOTSTRAP_VERSION = '1.2.0'; | ||
| export const BOOTSTRAP_VERSION = '1.3.0'; |
There was a problem hiding this comment.
B1(a) — 1.3.0 is already taken on main, by a different bundle. main consumed 1.3.0 for the compute-lambda-microvm policy (#645 / ADR-021) and is now at 1.6.0, with a per-bump changelog in this docstring. BootstrapPolicyVersion is an operator-visible CDKToolkit output that operators are told to >= check, so two different bundles publishing 1.3.0 makes them indistinguishable. On rebase this becomes 1.7.0 plus a bump-history entry.
| export function getRequiredBootstrapPolicies(computeType: string): string[] { | ||
| const base: string[] = [...CORE_POLICIES]; | ||
| const variants = COMPUTE_VARIANT_POLICIES[computeType]; | ||
| if (variants) base.push(...variants); |
There was a problem hiding this comment.
B3 — silent under-scoping. An unknown or misspelled compute type falls through to core-only with no error, and required-policies.test.ts:45-50 pins that as intended. That defeats the fail-loud guarantee documented at policies/index.ts:63-71: the throw there fires only for a name that is in this list but has no document — the drift that can actually happen (a substrate nobody registered) takes the quiet path. Concretely, main already ships lambda-microvm, so post-rebase collectBootstrapAllowActions('lambda-microvm') returns an allow-set with none of that substrate's grants. compute_type is a closed set on main — type this parameter as a union, or throw on an unrecognised name.
| update: ['sqs:SetQueueAttributes', 'sqs:TagQueue', 'sqs:UntagQueue'], | ||
| delete: ['sqs:DeleteQueue', 'sqs:GetQueueUrl'], | ||
| }, | ||
| 'AWS::SQS::QueuePolicy': { |
There was a problem hiding this comment.
B2 — this entry is unreachable by the live gate. AWS::SQS::QueuePolicy is also in CFN_TYPES_WITHOUT_EXEC_ROLE_IAM (line 37), and the exempt check short-circuits first in synth-coverage.test.ts:58 / :100. So the two SQS grants this PR adds specifically to make QueuePolicy deployable are never validated by the synth-time gate — drop sqs:AddPermission from application.ts and the gate stays green. Eleven of the 64 entries overlap the exempt set this way; the two sets are disjoint on upstream/main, so this is introduced by the preflight-map merge in 58ad265. Decide per type, then pin the invariant: expect([...CFN_TYPES_WITHOUT_EXEC_ROLE_IAM].filter((t) => t in RESOURCE_ACTION_MAP)).toEqual([]).
| // ─── ECS ─────────────────────────────────────────────────────────── | ||
| 'AWS::ECS::Cluster': { | ||
| create: ['ecs:CreateCluster', 'ecs:TagResource'], | ||
| read: [], |
There was a problem hiding this comment.
B5 — create-only on the two entries this PR was blocked on. compute-ecs.ts:35-48 grants the full lifecycle (ecs:DeleteCluster, ecs:DeregisterTaskDefinition, ecs:DescribeClusters, ecs:UpdateCluster, ecs:UpdateClusterSettings, ecs:PutClusterCapacityProviders, ecs:UntagResource, ecs:ListTagsForResource) and krokoko's suggested fix spelled those phases out. Not a deploy failure today, but 12 of the 14 ecs:* grants are unverified: trim ecs:DeleteCluster and nothing fails, on the substrate whose stack-delete path is exactly the reactive-fix class the CRUD shape exists to catch. It also reads oddly against the pin at resource-action-map.test.ts:89-99, which demands update/delete depth on 45 entries while the two newest entries opt out.
| unmapped.push(cfnType); | ||
| continue; | ||
| } | ||
| const missing = findMissingBootstrapActions(cfnType, allowedActions); |
There was a problem hiding this comment.
B4 — the gate still validates against the union. allowedActions comes from collectBootstrapAllowActions() with no argument (line 47), so this ECS-substrate check validates ECS-path resources against a set that includes compute-agentcore's grants — which an ECS-only operator never deploys. A required action granted only there passes here and AccessDenieds on their real IaCRole, which is the exact failure the compute-type-aware selection was added to close. Two arguments fix it: collectBootstrapAllowActions('agentcore') for the default synth and ('ecs') here, computed per-test rather than once in beforeAll.
| if (!entry) { | ||
| return []; | ||
| } | ||
| return [...new Set(phases.flatMap((phase) => entry[phase]))]; |
There was a problem hiding this comment.
Nit — an entry missing a phase makes this yield undefined inside the action list, which poisons getAllMappedActions() and then throws in action.split(':') somewhere unrelated. The comment at resource-action-map.test.ts:78-80 describes it as "silently skipped"; it crashes instead. entry[phase] ?? [] plus a comment correction — the pin itself is good.
| expect(hasCreateOrDelete).toBe(true); | ||
| if (!hasCreateOrDelete) { | ||
| // Extra info for debugging (won't reach here if assertion passes) | ||
| throw new Error(`${type} has no create or delete actions`); |
There was a problem hiding this comment.
Nit — dead code: expect(hasCreateOrDelete).toBe(true) on the line above already aborts, so this throw never runs. krokoko flagged this pair; the sibling at :113-118 got fixed by putting the throw before the expect. Same treatment here, or drop it.
Duplicate submission — superseded by the review immediately following this one, which carries the same findings in a corrected form. Dismissing so only one blocking review stands.
Summary
Closes #124
Closes #164
Creates a mapping from CloudFormation resource types to required IAM actions (CRUD lifecycle), scoped to all resource types in this app's synthesized template. Introduces
getRequiredBootstrapPolicies()for downstream consumption by the Aspect (#125) and preflight validator (#126). Gates ECS construct oncompute_typecontext variable (replaces comment toggle).Stack position
PR 5 for #120 — least-privilege CDK bootstrap policies as code
Prior: Custom template generator + compute variants (PR #162, #123)
This PR: Resource-action-map + ECS context gate + required-policies module
Next: CDK Aspect for policy envelope checking (#125)
Key decisions
Deliverables
Test plan
Open questions
Implementation plan
See: docs/superpowers/plans/2026-05-21-resource-action-map.md
Blocked by: #123 (PR #162)
References: RFC #120, ADR-002
🤖 Generated with Claude Code