Skip to content

fix(cdk): pin VPC to AgentCore-supported availability zones - #358

Open
AshrafBen10 wants to merge 16 commits into
aws-samples:mainfrom
AshrafBen10:fix/353-agentcore-supported-azs
Open

fix(cdk): pin VPC to AgentCore-supported availability zones#358
AshrafBen10 wants to merge 16 commits into
aws-samples:mainfrom
AshrafBen10:fix/353-agentcore-supported-azs

Conversation

@AshrafBen10

@AshrafBen10 AshrafBen10 commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Makes the AgentCore VPC land only in AgentCore-supported availability zones. By default (local cdk deploy with a bound account), the stack auto-selects supported zones from the account's AZ name↔ID mapping at synth time; the agentcore:availabilityZones context key remains as a validated override for env-agnostic deploys (the CI-built artifact) and unsupported regions.

Problem

AgentCore only supports a subset of physical AZs per region, published as zone IDs (e.g. use1-az1, use1-az2, use1-az4 for us-east-1). AZ names are aliased per-account, so CDK's default maxAzs selection can land the Runtime ENIs in an unsupported zone — AWS::BedrockAgentCore::Runtime fails with NotStabilized and rolls back the stack.

Live reproduction: the ADR-021 P1 verification run (#645, 2026-07-31, us-east-1) hit exactly this — the default selection picked us-east-1a (= use1-az6 in that account), which AgentCore rejects; the deploy failed until subnets were repinned.

Changes

  • cdk/src/constructs/agentcore-azs.ts (new) — AGENTCORE_SUPPORTED_AZ_IDS per-region zone-ID map (from the AgentCore Supported AZs table); resolveAgentCoreAzOverride validating the context override (JSON-parses the -c string form; loud synth error on malformed input; ≥2 zones for HA); selectSupportedAzNames pure name↔ID intersection; resolveAgentCoreAzs resolution order override → auto-pin (capped at 2 zones, matching default maxAzs) → env-agnostic fallback with synth warning.
  • cdk/src/main.ts — async entrypoint; resolves AZs and threads them through new AgentStackProps.availabilityZones.
  • cdk/src/stacks/agent.ts — passes props.availabilityZones to AgentVpc.
  • cdk/src/constructs/agent-vpc.ts — optional availabilityZones prop (takes precedence over maxAzs); docs point at the per-region map instead of hardcoded us-east-1 IDs.
  • cdk/src/constructs/bedrock-models.ts — same -c string-vs-array fix for resolveBedrockModelIds (its JSDoc-documented -c bedrockModels='[...]' form previously threw). Tracked as fix(cdk): resolveBedrockModelIds rejects the documented -c string override #628.
  • DocsDEPLOYMENT_GUIDE.md "Known deployment issues" section (auto-pin is local-deploy-only; the CI/CD-deployed artifact is env-agnostic and must set the override; discover-mapping → pick-IDs → set-context steps), QUICK_START.mdx note + troubleshooting row, Starlight mirrors synced.
  • Tests — subnet AvailabilityZone asserted against pinned names (not just count) + env-agnostic Fn::GetAZs case; full coverage of the AZ module (override validation incl. -c string form, selection, resolver branches, 2-zone cap); bedrock-models -c cases.
  • Deps — adds @aws-sdk/client-ec2 (lazy dynamic import; only loaded when auto-pin runs).

Usage for affected accounts (env-agnostic / pipeline deploys)

# Discover your AZ mapping
aws ec2 describe-availability-zones --region <region> \
  --query 'AvailabilityZones[].[ZoneName,ZoneId]' --output text

# Pick >=2 names whose IDs are AgentCore-supported, then either:
cdk deploy -c 'agentcore:availabilityZones=["us-east-1b","us-east-1c"]'

Or in cdk.context.json:

{ "agentcore:availabilityZones": ["us-east-1b", "us-east-1c"] }

Local deploys with a bound account need no action — auto-pin handles it.

Testing

  • agentcore-azs, agent-vpc, bedrock-models, agent stack, github-tags suites green (144 tests after the latest main merge)
  • TypeScript compile + ESLint clean; AI004 masking scan clean; docs mirrors in sync

Fixes #353
Fixes #628
Related to #645 (its P1 verification run reproduced this bug live; this PR removes that failure mode — it does not implement the MicroVM backend itself)

…zones (aws-samples#353)

AgentCore only supports a subset of physical availability zones per region.
AZ names are aliased per-account to physical zone IDs, so the default
maxAzs selection can land in a zone AgentCore does not support, causing the
AWS::BedrockAgentCore::Runtime resource to fail with NotStabilized.

Changes:
- Add optional `availabilityZones` prop to AgentVpcProps — when provided it
  takes precedence over maxAzs so the VPC is pinned to specific AZ names.
- Wire up the CDK context key `agentcore:availabilityZones` in agent.ts so
  affected accounts can set it in cdk.context.json or via -c flag without
  touching construct code.
- Add tests for the new prop (explicit AZs override maxAzs, 3-zone case).

Usage for affected accounts:
  cdk deploy -c agentcore:availabilityZones='["us-east-1b","us-east-1c"]'

Or in cdk.context.json:
  { "agentcore:availabilityZones": ["us-east-1b", "us-east-1c"] }

Closes aws-samples#353
@AshrafBen10
AshrafBen10 requested a review from a team as a code owner June 16, 2026 19:45
@AshrafBen10
AshrafBen10 requested a review from a team as a code owner June 16, 2026 20:54
@isadeks

isadeks commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Review: fix(cdk): pin VPC to AgentCore-supported availability zones (#353)

The fix is correct and does what it claims — verified empirically against aws-cdk-lib@2.257.0:

  • Works for the env-agnostic production stack. Passing explicit availabilityZones bakes the literal AZ names (us-east-1b, us-east-1c) straight into the CloudFormation subnets, whereas the original maxAzs path emits Fn::Select/Fn::GetAZs tokens that CloudFormation resolves nondeterministically at deploy time — exactly the cdk: deploy fails when VPC lands in an AgentCore-unsupported availability zone #353 failure mode. Pinning genuinely makes the deploy deterministic.
  • The spread approach is valid. VpcProps.availabilityZones is a real prop, mutually exclusive with maxAzs (CDK throws if both are set), and ...(cond ? {availabilityZones} : {maxAzs}) correctly passes exactly one.
  • Consistent with repo conventions — the tryGetContext(...) as T pattern matches how blueprintRepo, stackName, compute_type, and github:* are already read.

(I went in expecting the known CDK gotcha — a subset-of-stack-AZs validation throw on an env-agnostic stack — but reproduced that it does not throw in the production configuration. No correctness bug in the happy path.)

Two low-severity findings, neither blocking:

🔧 1. Unvalidated context cast (agent.ts:216)

const agentCoreAzs = this.node.tryGetContext('agentcore:availabilityZones') as string[] | undefined;

The as string[] cast trusts the operator to pass valid JSON. The intuitive shorthand -c agentcore:availabilityZones=us-east-1b (a bare string, not the JSON-array form the docs show) sails through the cast, gets spread into availabilityZones, and makes CDK throw:

this.availabilityZones.forEach is not a function

…a synth error that names neither the context key nor the expected shape. Since the operator hitting this is already mid-firefight over AZs (the whole point of the feature), a one-line guard is worth it:

if (agentCoreAzs !== undefined && !Array.isArray(agentCoreAzs)) {
  throw new Error("Context 'agentcore:availabilityZones' must be a JSON array of AZ names, e.g. -c agentcore:availabilityZones='[\"us-east-1b\",\"us-east-1c\"]'");
}

🔧 2. Test coverage gap (agent-vpc.test.ts:152)

The new tests deploy into a concrete-env stack (env: { region: 'us-east-1' }), but AgentStack is env-agnostic in production (main.ts:26, account/region from CDK_DEFAULT_*). The two branches happen to produce identical output today (I verified both bake in literal AZ names), so this isn't a current bug — but the tests don't exercise the path production actually synthesizes in, so a future CDK upgrade that changes env-agnostic AZ handling could regress production while these stay green. Worth adding an env-agnostic case that asserts the literal AZ names land in the subnets.


Both are nits — the PR is fundamentally sound and safe to merge.

Reviewed at xhigh effort. The fix mechanism (literal AZs vs Fn::GetAZs tokens), VpcProps.availabilityZones/maxAzs mutual exclusion, the env-agnostic synth behavior, and the bare-string crash all verified by reproduction against aws-cdk-lib@2.257.0.

@krokoko krokoko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this — the root cause analysis is spot on, and the approach (pinning AgentVpc via CDK context so the template gets literal AZ names instead of Fn::GetAZs) is a valid escape hatch. The CDK wiring (availabilityZones vs maxAzs) also looks correct.

I'm requesting changes because, as shipped, this doesn't yet meet the bar for how we should handle AgentCore's AZ constraint in this sample. AWS documents the requirement in terms of physical zone IDs (e.g. use1-az1 / use1-az2 / use1-az4 in us-east-1), and AZ names are aliased per account — see:

Issue #353 itself preferred auto-selecting supported zone IDs so a fresh deploy succeeds without account-specific guesswork. A manual name pin alone leaves the default path broken for affected accounts.

What to fix to make this right

  1. Prefer auto-pin from AgentCore-supported zone IDs (default path)
    At synth time (we already need ec2:DescribeAvailabilityZones), resolve the account's name↔ID mapping, intersect with AgentCore's supported zone-ID set for the region, and pass those AZ names into ec2.Vpc({ availabilityZones }). Keep agentcore:availabilityZones only as an optional override. That matches AWS's model and closes the bug without per-account tribal knowledge.

  2. Validate the context override
    Mirror the pattern in resolveBedrockModelIds (bedrock-models.ts): reject non-arrays / bare strings with a clear synth error naming the key and expected JSON shape. Prefer requiring ≥2 zones (AgentCore HA guidance).

  3. Document the operator path
    Even if the escape hatch remains, add a short note to the deploy / quick-start / troubleshooting docs (discover mapping → pick supported IDs → set context). An override buried only in construct comments won't help someone mid-rollback.

  4. Harden tests
    Assert subnet AvailabilityZone properties are the pinned names (not only subnet count), and add an env-agnostic case since production AgentStack synthesizes that way.

  5. Avoid hardcoding only us-east-1 IDs in comments as if universal
    Supported sets differ by region and can change; prefer linking the AgentCore AZ table (and/or a small per-region constant map used by the auto-select logic).

Happy to re-review once the default path selects supported zone IDs (with the context override as a safety valve), validation, docs, and stronger tests are in place. Appreciate the careful write-up and the unblock for #353.

…#353) Addresses review on aws-samples#358: a manual AZ pin alone left the default deploy path broken for affected accounts. The stack now auto-selects AgentCore-supported availability zones, with the context override kept as a validated safety valve. - New constructs/agentcore-azs.ts: - AGENTCORE_SUPPORTED_AZ_IDS: per-region map of supported physical zone IDs (from the AWS AgentCore VPC docs), not a us-east-1-only comment presented as universal. - resolveAgentCoreAzOverride: validates agentcore:availabilityZones, mirroring resolveBedrockModelIds (rejects non-array / empty / non- string entries and <2 zones for HA) with a clear synth error. - selectSupportedAzNames: pure name<->id intersection. - resolveAgentCoreAzs: override wins; else, when synth has a concrete account+region, DescribeAvailabilityZones -> intersect -> pin AZ names; env-agnostic synth / unknown region / lookup failure fall back to CDK's default selection (surfaced via a synth warning, not a masked empty result). - main.ts is now async and threads the resolved names into AgentStack (new AgentStackProps.availabilityZones) -> AgentVpc. - Docs: DEPLOYMENT_GUIDE 'Known deployment issues' + QUICK_START note and troubleshooting row cover the discover-mapping -> pick-IDs -> set- context operator path. - Tests: agent-vpc asserts subnet AvailabilityZone == pinned names plus an env-agnostic Fn::GetAZs fallback; agentcore-azs covers the map, override validation, selection, and resolver branches. Adds @aws-sdk/client-ec2 for the synth-time lookup (loaded lazily).
@AshrafBen10

Copy link
Copy Markdown
Contributor Author

@krokoko thanks for the detailed review — pushed 392b55e reworking this from a manual pin into auto-select-by-default. Mapping to your points:

1. Auto-pin from supported zone IDs (default path). New cdk/src/constructs/agentcore-azs.ts resolves AZs at synth: the validated override wins, otherwise — when synth has a concrete account+region — it calls DescribeAvailabilityZones, intersects the account's name→zone-ID mapping with the region's supported set, and passes the resulting AZ names into ec2.Vpc({ availabilityZones }). main.ts is now async and threads the result through AgentStackProps.availabilityZonesAgentVpc.

One nuance worth flagging: this repo's pipeline pre-synthesizes env-agnostic in build.yml (no bound account) and deploy.yml deploys that artifact, so auto-pin takes effect for a local/dev cdk deploy (concrete account), while the override remains the pin for the CI-built artifact. That lines up with your item #4 note that the production stack synthesizes env-agnostic. Happy to also wire a creds-bound synth into the pipeline if you'd prefer auto-pin to cover that path too.

2. Validate the override. resolveAgentCoreAzOverride mirrors resolveBedrockModelIds — rejects non-arrays, empty/non-string entries, and fewer than two zones (HA), with a synth error naming the key and expected JSON shape.

3. Docs. Added a "Known deployment issues" subsection in DEPLOYMENT_GUIDE.md (symptom → root cause → auto-pin default → discover-mapping → pick supported IDs → set context), plus a QUICK_START.mdx note and a troubleshooting row for the NotStabilized / "unsupported availability zones" symptom.

4. Harden tests. agent-vpc.test.ts now asserts subnet AvailabilityZone equals the pinned names (not just subnet count) and adds an env-agnostic Fn::GetAZs fallback case. agentcore-azs.test.ts covers the per-region map, override validation, the pure selection function, and every resolver branch (override wins, env-agnostic, unknown region, auto-pin, <2 supported, lookup failure).

5. Per-region map, not us-east-1-only. AGENTCORE_SUPPORTED_AZ_IDS is a per-region constant map sourced from the AgentCore Supported Availability Zones table (9 regions), and the construct/module comments link that table rather than presenting us-east-1 IDs as universal.

Implementation note: added @aws-sdk/client-ec2 (loaded via a lazy dynamic import so it's only pulled in when auto-pin actually runs). Ready for another look.

@codecov-commenter

codecov-commenter commented Jul 15, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 94.97041% with 17 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@1b17c28). Learn more about missing BASE report.

Files with missing lines Patch % Lines
cdk/src/constructs/agentcore-azs.ts 93.38% 17 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #358   +/-   ##
=======================================
  Coverage        ?   92.09%           
=======================================
  Files           ?      319           
  Lines           ?    88151           
  Branches        ?     8838           
=======================================
  Hits            ?    81184           
  Misses          ?     6967           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@isadeks isadeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of a7f04df — the auto-pin rework

The rework from a manual pin into synth-time auto-selection is a real improvement and maps cleanly onto krokoko's five CHANGES_REQUESTED points. I checked it out and ran itagentcore-azs.test.ts + agent-vpc.test.ts 33/33, tsc --noEmit clean, and I verified the behaviors below by synth/unit probes rather than reading. It also resolves both nits from my earlier review (context validation + env-agnostic test). Nice work, and the loud-fail resolveAgentCoreAzOverride mirroring resolveBedrockModelIds is the right shape.

One finding is worth fixing before merge because it breaks the exact operator recovery path this feature exists for; the rest are minor.

1. The documented -c 'agentcore:availabilityZones=[...]' escape hatch throws (should-fix — it's the mid-rollback recovery path)

resolveAgentCoreAzOverride rejects anything that isn't Array.isArray(...). But CDK delivers a -c key=value context value as a raw string, not parsed JSON — so the CLI form throws:

$ cdk deploy -c 'agentcore:availabilityZones=["us-east-1b","us-east-1c"]'
Error: Context 'agentcore:availabilityZones' must be a JSON array of availability-zone names …;
       got "[\"us-east-1b\",\"us-east-1c\"]".

Reproduced both ways: the cdk.context.json form (a real array) works; the -c CLI form throws. This matters because the PR description's "Usage" block leads with the -c form, and an operator hitting the AgentCore AZ rollback — the whole reason this feature exists — is the one most likely to reach for -c mid-firefight. It's the same crash I flagged in my June review (then .forEach is not a function); the new validator turns it into a clearer message but still doesn't accept the input the docs advertise.

Fix: when the context value is a string, JSON.parse it before the array check (fall back to treating a bare string as a single malformed entry so the error still fires for true typos). Then the -c and cdk.context.json forms behave identically. (resolveBedrockModelIds has the same structural limitation, but its docs don't advertise a -c form, so this PR is the one that creates the mismatch — worth fixing here and arguably worth a follow-up for the shared pattern.)

2. Auto-pin silently widens the VPC from 2 AZs to all supported zones (often 3) (document / confirm intent)

selectSupportedAzNames returns every matching zone, so on a typical us-east-1 account auto-pin yields 3 AZs where maxAzs: 2 previously gave 2 → 6 subnets instead of 4. I confirmed NAT gateways stay at 1 (the construct default), so there's no NAT/EIP cost blowup — just more subnets (free) and a wider ENI spread. Benign, but it's a silent topology change for accounts that deploy fine today, and it's not called out. Either cap the auto-pin at 2 (matching the old default and the MIN_AGENTCORE_AZS HA floor) or note in the docs that auto-pin uses all supported zones.

3. The CI-deployed stack still isn't auto-fixed — only local cdk deploy is (doc clarity)

Verified the deploy path: build.yml synthesizes cdk.out with persist-credentials: false and no aws-region, so the artifact is env-agnosticresolveAgentCoreAzs returns undefined → the CI-deployed stack falls back to Fn::GetAZs/maxAzs, unpinned. deploy.yml then deploys that artifact (--app cdk/cdk.out). The DEPLOYMENT_GUIDE does say env-agnostic synth needs the override, so this is documented, not a bug — but the framing ("Default behavior (auto-pin)… No action needed") reads as if the common case is covered, when the CI/CD-deployed stack (the one most teams run) still needs the manual override. Worth making that contrast sharper so a team relying on deploy.yml doesn't assume they're protected.

Minor

  • Override isn't cross-checked against AGENTCORE_SUPPORTED_AZ_IDS — a typo'd but well-formed override (["us-east-1x","us-east-1y"]) is passed straight to ec2.Vpc, deferring the failure to deploy time. Defensible (the override is a deliberate escape hatch, and names→IDs are account-specific so a name-level check can't validate zone-ID support), but a soft synth warning when an override's resolved IDs aren't in the supported set would catch fat-fingers earlier. Optional.
  • defaultDescribeAzs (lines 153-169) is the one uncovered block — fine, since it's the injected live-call boundary and tests substitute it.

Verification

Ran at a7f04df: 33/33 tests, tsc clean. Reproduced: -c string form throws vs cdk.context.json array works (#1); auto-pin returns 3 zones / 6 subnets / 1 NAT (#2); env-agnostic synth returns undefined and build.yml synths credential-less (#3); well-formed-but-unsupported override passes unchecked (minor). Selection preserves the account's zone order (deterministic given a stable DescribeAZ response).

Recommendation: fix #1 before merge (small change, unblocks the documented recovery path); #2/#3 are doc/clarity items; the rest optional. Approach and structure are sound.

…samples#353) Follow-up to the auto-pin review on aws-samples#358. - resolveAgentCoreAzOverride now JSON-parses a string context value, so the documented '-c agentcore:availabilityZones=[...]' recovery path works identically to the cdk.context.json array form (CDK delivers -c values as raw strings). A non-JSON string is left as-is and still fails the array check with the same clear, key-named error, so true typos error out. - Auto-pin now pins the first MIN_AGENTCORE_AZS (2) supported zones instead of every match, matching AgentVpc's default maxAzs so enabling auto-pin no longer silently widens a working 2-AZ account to all supported zones (e.g. 3 AZs / 6 subnets). - DEPLOYMENT_GUIDE: sharpened the contrast that auto-pin is local-deploy-only and the CI/CD (deploy.yml) artifact is env-agnostic and must set the override; added an explicit -c example now that it parses. - Tests: added JSON-string override (success) and JSON-non-array (throws) cases; updated the auto-pin test to assert the 2-zone cap.
@AshrafBen10

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough re-review and for actually running it — pushed 2891801 on top of the latest main merge.

#1 (should-fix) — -c string override now works. You're right, this broke the exact recovery path the feature exists for. resolveAgentCoreAzOverride now JSON.parses a string context value before the array check, so -c 'agentcore:availabilityZones=["us-east-1b","us-east-1c"]' and the cdk.context.json array form behave identically. A non-JSON string (a real typo) is left as-is and still fails the array check with the same clear, key-named error, so typos keep erroring. Added tests for the JSON-string success path and a JSON-string-that-isn't-an-array (throws). Left resolveBedrockModelIds alone since its docs don't advertise -c — happy to file a follow-up for the shared pattern.

#2 — auto-pin no longer widens topology. Capped auto-pin to the first MIN_AGENTCORE_AZS (2) supported zones, matching AgentVpc's default maxAzs, so an account that deploys fine today stays at 2 AZs / 4 subnets instead of silently going to 3 / 6. Updated the auto-pin test to assert the cap (3 supported → 2 pinned).

#3 — sharpened the CI/CD doc contrast. Reworked the deployment-guide section: the "auto-pin" heading now reads "local deploy only," and there's a dedicated "The CI/CD-deployed stack is NOT auto-pinned" paragraph spelling out that build.yml synthesizes credential-less/env-agnostic and deploy.yml ships that artifact, so pipeline-deploying teams must set the override. Also added the -c example now that it parses.

Minors:

  • Override-vs-AGENTCORE_SUPPORTED_AZ_IDS cross-check: I left this out deliberately. The override is the escape hatch precisely for env-agnostic/pipeline deploys where there's no bound account, so validating the resolved zone IDs would require a live DescribeAvailabilityZones and break the offline/CI use case. As you noted, a name-level check can't validate zone-ID support anyway. Deploy-time remains the failure point for a well-formed-but-wrong override.
  • defaultDescribeAzs coverage: agreed, that's the injected live-call boundary; tests substitute it.

Verification on 2891801: agentcore-azs + agent-vpc + agent stack tests green (35 AZ-module tests incl. the new -c cases), tsc clean, eslint clean, AI004 masking scan clean, docs mirror synced. Ready for another look.

isadeks
isadeks previously approved these changes Jul 15, 2026

@isadeks isadeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of 2891801 — all findings addressed, approving

Checked out and ran it. All three findings from the last review are fixed and verified empirically (not just from the reply):

#1-c JSON-string override now works. resolveAgentCoreAzOverride JSON.parses a string context value before the array check. Verified at both layers:

  • Unit: -c string form → ["us-east-1b","us-east-1c"]; array form unchanged; and the error paths all still fire — a bare-string typo, a JSON-but-non-array ('"us-east-1b"'), and a single-element array below the HA floor all throw the clear key-named error. So the recovery path works and typos still error.
  • Integration: the exact resolveAgentCoreAzs call main.ts makes, fed the raw string CDK delivers for -c, now returns the array instead of throwing at synth entry (the crash from my last review is gone).

#2 — auto-pin no longer widens topology. names.slice(0, MIN_AGENTCORE_AZS) caps it. Verified: 3 supported zones → pins exactly 2 (["us-east-1a","us-east-1b"]), matching AgentVpc's default maxAzs. An account that deploys fine today stays at 2 AZs / 4 subnets.

#3 — CI/CD contrast sharpened. The doc now heads the default path "auto-pin — local deploy only" and adds a dedicated "The CI/CD-deployed stack is NOT auto-pinned" paragraph spelling out that build.yml synths credential-less and deploy.yml ships that artifact, so pipeline teams must set the override. Also documents the "first two" cap and adds the -c example. Reads clearly now.

Minors — both correctly dispositioned:

  • Override-vs-supported-ID cross-check left out: agreed and well-reasoned — the override exists precisely for env-agnostic/pipeline deploys where there's no bound account to resolve zone IDs, so a live cross-check would break the offline use case, and a name-level check can't validate zone-ID support anyway. Deploy-time remains the failure point for a well-formed-but-wrong override, which is acceptable for an escape hatch.
  • defaultDescribeAzs coverage: fine, it's the injected live-call boundary.

Verification on 2891801: 35 AZ-module + agent-vpc tests green, tsc --noEmit clean. The rework from a manual pin into auto-select-with-override is a solid outcome for #353 and matches AWS's zone-ID model.

LGTM — approving. (Optional follow-up, non-blocking: resolveBedrockModelIds shares the same -c-string-vs-array structural limitation; worth the same JSON.parse treatment if a -c form ever gets documented for it.)

…) resolveBedrockModelIds rejected any non-array context value, but CDK delivers a `-c key=value` context value as a raw string — so the `-c bedrockModels='[...]'` form the function's own JSDoc advertises threw at synth, while the cdk.context.json array form worked. JSON-parse a string context value before the array check so both forms behave identically. A non-JSON string (a true typo) is left as-is and still fails the array check with the same clear, key-named error, so loud-fail on malformed input is preserved. Same fix shape as resolveAgentCoreAzOverride (aws-samples#358 review follow-up). Tests: added -c JSON-string array (success) and JSON-string-non-array (throws) cases.
@AshrafBen10

Copy link
Copy Markdown
Contributor Author

Heads-up on an extra file in this PR: I folded in the non-blocking resolveBedrockModelIds follow-up @isadeks flagged (commit d1fcec5, tracked as #628).

Why it's here: it's the exact same bug class this PR just fixed for the AZ override — resolveBedrockModelIds rejects any non-array context value, but CDK delivers -c key=value as a raw string, so the -c bedrockModels='[...]' form its own JSDoc advertises throws at synth. The fix mirrors resolveAgentCoreAzOverride: JSON.parse a string context value before the array check; a non-JSON typo is left as-is and still fails with the same clear, key-named error. Added -c JSON-string (success) and JSON-string-non-array (throws) tests.

Scope note: this does widen the PR beyond #353 (AZ pinning) to also cover #628 (bedrock override). I kept the commit self-contained, so if you'd rather keep this PR scoped to the AZ work, I'm happy to pull d1fcec5 back out into its own PR against main — just say the word.

Verified locally: tsc clean, ESLint clean, bedrock-models + agentcore-azs + agent-vpc tests green, AI004 masking scan clean.

@dreamorosi

Copy link
Copy Markdown
Member

Live datapoint supporting this fix, from the ADR-021 P1 verification run (2026-07-31, us-east-1): the default AgentVpc AZ selection picked us-east-1a (= use1-az6 in that account), which AgentCore rejects — the deploy failed until subnets were repinned to supported AZs. Evidence recorded in the #645 verification findings.

…us-cloud-coding-agents into fix/353-agentcore-supported-azs

# Conflicts:
#	cdk/src/main.ts
#	cdk/src/stacks/agent.ts
#	yarn.lock
@AshrafBen10

Copy link
Copy Markdown
Contributor Author

For visibility in the thread (PR description was also updated to match the current shape of the change):

This PR resolves:

Related to #645 (not fixed by this PR): the ADR-021 P1 verification run reproduced the #353 failure live — default selection picked us-east-1a (= use1-az6 in that account), which AgentCore rejects, failing the deploy until subnets were repinned (thanks @dreamorosi for the datapoint). This PR removes that failure mode for the MicroVM work and everything else on the stack; the MicroVM backend itself is delivered separately under #645.

Branch is current with main (conflicts in main.ts / agent.ts / yarn.lock resolved in 6362ae4), CI-relevant suites green locally: 144 tests across agentcore-azs / agent-vpc / bedrock-models / agent stack / github-tags, compile + ESLint + AI004 masking scan clean, docs mirrors synced.

…us-cloud-coding-agents into fix/353-agentcore-supported-azs

# Conflicts:
#	yarn.lock

@isadeks isadeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of 8e95d1a — five blocking findings, including one my last review missed

I approved 2891801 on the basis that the three findings from the round before it were fixed — they were, but that pass graded the delta rather than the reworked design. 392b55e was a redesign (manual pin → synth-time auto-selection: new module, new SDK call, async entrypoint, hardcoded region map), and it never got a from-scratch review. Doing that now, checked out and executed rather than read. Please treat my previous approval as withdrawn.

The diagnosis and the shape of the fix are still right. All five findings below are narrow.


Blocking

1. Every degraded path is silent in production — the safety net isn't wired up

cdk/src/constructs/agentcore-azs.ts:243,250

main.ts:48 passes the App as scope, because resolveAgentCoreAzs runs at main.ts:47, before new AgentStack(...) at :53 — there's no stack to annotate yet. CDK collects annotations per stack artifact, walking each stack's tree downward, so App-node metadata reaches no artifact. Verified against aws-cdk-lib in this tree, calling the resolver exactly as main.ts does:

STACK MESSAGES: []
MANIFEST MENTIONS "AgentCore AZs"?: false
APP NODE METADATA: [ aws:cdk:warning "[AgentCore AZs] Could not resolve …" ]   ← in-process only
CONTROL (identical warning on a Stack): present in ProbeStack2.metadata.json ✓

Failure scenario: expired SSO, missing ec2:DescribeAvailabilityZones, throttling, or a proxy → defaultDescribeAzs throws → caught at :249 → warning into the void → undefined → VPC reverts to default AZ selection. Clean synth, exit 0, zero output, then the exact NotStabilized rollback this PR exists to prevent, plus the ROLLBACK_COMPLETE → destroy → 20–40 min ENI wait that QUICK_START.mdx documents. The <2 supported zones branch (:243) is identically silent; the unknown-region branch (:223) emits nothing at all.

The tests can't catch this because they pass a Stack (agentcore-azs.test.ts:36-39, asserted :205,:213) — a scope production never uses. The assertions are real (deleting either addWarning does fail a test), they just guard the wrong seam.

Suggested fix — return diagnostics and attach them once the stack exists:

export interface AgentCoreAzResolution { readonly zones?: string[]; readonly diagnostics: string[] }

// main.ts
const az = await resolveAgentCoreAzs({ ... });
const stack = new AgentStack(app, stackName, { env: devEnv, availabilityZones: az.zones, ... });
for (const d of az.diagnostics) Annotations.of(stack).addError(d);   // addError: fail closed

Then add a test that synthesizes through the real seam and asserts on app.synth().getStackByName(id).messages. It fails today — that's the point.

2. AGENTCORE_SUPPORTED_AZ_IDS covers 9 of the 19 commercial regions AWS publishes

cdk/src/constructs/agentcore-azs.ts:42-52

The 9 entries present are all correct. Missing: ap-northeast-2, ap-southeast-5, ap-southeast-7, ca-central-1 (note cac1-az1/az2/az4), eu-west-2, eu-west-3, eu-north-1, eu-south-1, eu-south-2, sa-east-1 (plus us-gov-west-1). In each of those, resolveAgentCoreAzs takes the unknown-region branch at :223undefined, no warning → the bug still reproduces.

The test cements the gap two ways: :80 uses eu-north-1 as its example of "a region with no known constraint" — eu-north-1 is supported (eun1-az1/2/3); and :52-60 asserts the map's key list back against itself, so it can never detect drift or a wrong ID ('us-east-1': ['usw2-az1'] would pass every current assertion).

Worth following the template already in the repo for this species of constant — cdk/src/handlers/shared/microvm-regions.ts carries a dated snapshot, a named source, and an explicit "this list rots by design" update path.

3. The URL that is the only stated way to maintain the map returns 404 — in four places

agentcore-azs.ts:36, agent-vpc.ts:59, docs/guides/DEPLOYMENT_GUIDE.md:187, docs/src/content/docs/getting-started/Deployment-guide.md:191

https://aws.github.io/…/user-guide/security/agentcore-vpc/#supported-availability-zones404 (the trailing-slash form; the .html form is now only a meta-refresh stub). The canonical live source, which is where I got the table for finding 2:

https://docs.aws.amazon.com/bedrock-agentcore/latest/devguide/agentcore-vpc.html#agentcore-supported-azs

docs/.markdown-link-check.json ignores ^https?://, so no gate will catch this.

4. The remedy documented for the CI/CD path can't be applied through the CI/CD path

docs/guides/DEPLOYMENT_GUIDE.md:186-207

The section correctly establishes that the pipeline artifact isn't auto-pinned and "must set the agentcore:availabilityZones override" — then gives two options, neither of which works there:

  • cdk/cdk.context.json — gitignored (.gitignore:67), so it can't be committed; and even if it were, build.yml:226 overwrites the entire file (jq -n '{…}' > cdk/cdk.context.json).
  • cdk deploy -c 'agentcore:availabilityZones=[…]' — inoperative, because deploy.yml:262 deploys the pre-synthesized assembly (npx cdk deploy --app cdk/cdk.out); context at that point can't change the template.

So on the repo's own recommended deployment path, #353 is still open. Two hooks that do work aren't mentioned: add the key to the "Generate CDK context" jq block (build.yml:178-227, already uploaded with the artifact at :301) from a repo variable, or add a context block to cdk/cdk.json. I'd wire build.yml here — otherwise the auto-pin machinery is inert exactly where the failure was originally reported.

5. The override — the only mechanism that works on the pipeline path — is shape-validated only

agentcore-azs.ts:102-141

It checks array / non-empty strings / length ≥ 2, and resolveAgentCoreAzs returns it at :211 before any lookup, so the name→zone-ID intersection the module is built around is never applied to operator input. Verified by synth:

  • ["us-east-1b","us-east-1b"] passes the "at least 2 zones for AgentCore high availability" check → 4 subnets, 1 distinct AZ. A single-AZ runtime that reads as two-AZ.
  • ["use1-az2","use1-az4"] — zone IDs, exactly what the guide's own describe-availability-zones command prints in column 2 — is accepted verbatim and lands as AvailabilityZone: "use1-az2".
  • Wrong-region names and nonexistent zones (us-east-1z) synthesize. CDK's own subset check doesn't save you: aws-cdk-lib/aws-ec2/lib/vpc.js skips it when the resolved stack AZ list is empty, which is the case for env-agnostic synth — i.e. the pipeline path.

The guide asks the operator to do the name↔ID intersection by hand, and getting it wrong is silent and produces a template that looks like the fix was applied. Suggested: require ≥2 distinct zones; reject ZoneId-shaped values with a pointed "that's a ZoneId, pass the ZoneName"; require the region prefix when the region is concrete; cross-check against the map whenever the mapping is available. Better still — accept zone IDs as a first-class override form and resolve them to names in code, which deletes the manual transcription step entirely.


Should fix

  1. Non-deterministic pin. agentcore-azs.ts:241names.slice(0, 2) takes the first two in DescribeAvailabilityZones response order, which AWS doesn't contract as sorted, and unlike CDK's availability-zones context provider this call isn't cached into cdk.context.json. Subnet.AvailabilityZone is create-only, so a reshuffle replaces every subnet → route tables → NAT/EIP → endpoints, and can wedge on the AgentCore-ENI DELETE_FAILED case this repo already documents. Iterate AGENTCORE_SUPPORTED_AZ_IDS[region] in declared order, or .sort().

  2. No upgrade note. Auto-pin is default-on and can repin an already-deployed stack. DEPLOYMENT_GUIDE.md has the precedent for this disclosure (the Route53 Resolver cascade entry, "upgrading from pre-v0.5").

  3. Credential-boundary shift. agentcore-azs.ts:167-168new EC2Client({ region }) resolves the app process's ambient chain, but the CDK CLI passes only CDK_DEFAULT_ACCOUNT/CDK_DEFAULT_REGION to the app (no credentials, no AWS_PROFILEprepareDefaultEnvironment in the bundled CLI). So cdk deploy --profile prod reads the mapping from the default profile's account. AZ names are region-uniform, so every guard passes and you get a confident pin into physically unsupported zones. Add an sts:GetCallerIdentity match against account, or assume the bootstrap lookup role — docs/design/DEPLOYMENT_ROLES.md:872 documents that role as owning "Context lookups (VPC, AZs, etc.)".

  4. Naked AWS client drops solution attribution. AGENTS.md:70 requires makeClient(Ctor, cfg) from cdk/src/handlers/shared/ua.ts for clients in cdk/src/; there's precedent for importing it from constructs/ in blueprint.ts and lambda-microvm-compute.ts.

  5. No timeout or attempt cap on a call now in the path of every synth/diff/deploy/mise run build (:168) — a blackholed endpoint hangs synth with no output. Add requestTimeout/connectionTimeout and maxAttempts: 2. Also add state=available to the filter at :172, so an impaired zone isn't eligible for pinning.

  6. The AI004 comment claims the opposite of the behaviour. agentcore-azs.ts:229-231 says the layout avoids silent-success masking. Hoisting pinnedZones above the try stops .semgrep/silent-success-masking.yaml:43-56 matching, but the caller-visible result is byte-identical (undefined), and the rule's own message states the standard: "Logging alone is not enough — the failure must reach the caller." The documented path for an intentional degraded fallback is an inline // nosemgrep: ts-silent-success-masking -- <justification>. Fixing blocking finding 1 resolves this; either way the comment as written isn't true.

  7. Prop name collides with an inherited concept. stacks/agent.ts:100 adds AgentStackProps.availabilityZones, but Stack already exposes a public availabilityZones getter, which still returns the unpinned set — two different values under one name in one class. Nothing reads it today, but Stack.of(x).availabilityZones is a stock idiom, so a rename now (agentCoreAvailabilityZones) beats a breaking change later.

  8. AgentVpc converts a loud CDK error into silent precedence. agent-vpc.ts:110-113ec2.Vpc throws "Vpc supports 'availabilityZones' or 'maxAzs', but not both." (verified); the spread silently drops maxAzs, and agent-vpc.test.ts:157 codifies that. Throw when both are set; guard on props.availabilityZones?.length rather than truthiness, and enforce the ≥2 floor at the construct boundary too.

  9. Test gaps — I mutation-tested the suite; 11 of 14 mutations were caught, and these 5 survived:

    • deleting the whole resolveAgentCoreAzs call in main.ts (no test imports main.ts)
    • AgentStack dropping props.availabilityZones and never passing it to AgentVpc
    • reordering so the env-agnostic guard runs before the override — i.e. the documented CI/CD path is untested
    • rewriting either warning message (both assertions use the same loose stringLikeRegexp('AgentCore AZs'))
    • removing the Token.isUnresolved guard at :218

    Also defaultDescribeAzs is 0% covered — the file sits at 75% function coverage against the 94% global threshold, which is what Codecov's 94.97% patch number is reporting. test/stacks/github-tags.test.ts is the existing precedent for testing main.ts wiring: extract a buildApp() and follow it, which closes the first two survivors and finding 1's test in one move.

Nits

  • bedrock-models.ts (#628) is a second logical change; CONTRIBUTING.md §3 asks for one per PR, and the squash message won't attribute #628. The fix itself is correct.
  • @aws-sdk/client-ec2 is in dependencies though it's synth-only — but moving it to devDependencies trips import-x/no-extraneous-dependencies (allowed only under test/, build-tools/). Either keep it with a one-line comment, or move it and extend the allowlist. Lockfile and version range are consistent with siblings.
  • exporting DescribeAzsFn (:74) is unused outside the file → +1 on the dead-code ratchet (advisory gate only).
  • MIN_AGENTCORE_AZS (:63) carries three meanings — override floor, sufficiency gate, and pin count, the last coupled by comment only to AgentVpc's ?? 2. Split into MIN_AGENTCORE_AZS + AUTO_PIN_AZ_COUNT and assert the coupling in a test.
  • Token.isUnresolved(account) (:218) can never fire — the values come from process.env, so they're string | undefined; the JSDoc's "token" account is unreachable.
  • err.message at :252 embeds authz text (User: arn:aws:iam::<acct>:user/<name> is not authorized…). Harmless only while the annotation is dropped; after fixing finding 1 it lands in cdk.out, which build.yml:301 uploads as a public-repo artifact. Log the error name/code instead.
  • The main.ts re-indentation churns blame on ~55 unrelated lines. The void main().catch(…) pattern itself works — verified: malformed override → clear message → exit 1, and no partial assembly is written.

Docs

DEPLOYMENT_GUIDE.md and QUICK_START.mdx are updated and the Starlight mirror is in sync — I ran node docs/scripts/sync-starlight.mjs, no diff; anchors resolve in both the relative and site-absolute forms.

Outstanding: the pipeline instructions don't work (finding 4); the source link 404s (finding 3); no upgrade note for existing stacks; "No action needed on that path for the regions in the built-in map" over-claims given that a lookup failure silently un-pins; and the QUICK_START edit appends auto-pin to a sentence ending "or the build will fail", which transfers a fail-hard guarantee auto-pin doesn't have.

Tests, CI, bootstrap

Local: agentcore-azs + agent-vpc + bedrock-models65 passed. Bootstrap suite → 6 suites / 111 tests passed, including test/bootstrap/synth-coverage.test.ts.

Bootstrap policy coverage: not applicable, and correctly absent. No new CloudFormation resource type — the diff only changes the value of an existing AWS::EC2::Subnet.AvailabilityZone. Confirmed by diffing synthesized resource-type sets (baseline vs a 2-AZ pin: zero new types, zero removed, zero count deltas; a 3-AZ override adds only more of existing types). ec2:DescribeAvailabilityZones is already granted to the CFN execution role (cdk/bootstrap/policies/infrastructure.json:132), so no BOOTSTRAP_VERSION/BOOTSTRAP_HASH/DEPLOYMENT_ROLES.md change is needed here.

CI has never run on this PR — the head is a fork, so only Validate PR title reports. Every review round so far, mine included, has been reading rather than running. Worth getting the branch onto aws-samples (or a maintainer dispatching the workflows) before the next pass. One flake to watch: "throws on a non-string / empty entry" failed once on a first run and then passed 4/4 in isolation and in combination — probably environmental, but one more CI run would rule it out.

Governance

#353 carries no labels at all — no approved, no priority — so ADR-003's gated-approval and priority-alignment gates were never satisfied. It's also already closed as completed, while the fix for it is still open here, so the tracker reads "done" for a bug main still has. #628 is likewise open with no labels. Branch name is correct. AGENTS.md → Boundaries lists both "new CDK constructs" and "new dependencies" under Ask first, and this PR does both.


Everything above is small relative to the rework already done, and finding 1 is the one to take first — it's what let the other four stay invisible through three rounds of review.

…able (aws-samples#353)

Addresses the five blocking findings from the from-scratch re-review of
8e95d1a.

1. Degraded paths were silent. resolveAgentCoreAzs annotated the App node,
   but CDK only collects annotations that hang off a *stack*, so every
   diagnostic was dropped: a denied/throttled/proxied DescribeAvailabilityZones
   produced a clean exit-0 synth, an unpinned VPC, and then the NotStabilized
   rollback this feature exists to prevent. The resolver now returns
   {zones, diagnostics} and main.ts attaches them to the stack via
   applyAgentCoreAzDiagnostics after it exists. Failure to complete an
   attempted auto-pin is now error-level (fail closed — verified against the
   CDK CLI: error annotations make it exit 1 with "Synthesis finished with
   errors"); genuinely-unknowable cases stay warnings. The old tests passed a
   Stack, a scope production never used, so they guarded the wrong seam; the
   new test/main.test.ts drives buildApp() and asserts on stack-artifact
   messages, and fails if the annotation target regresses.

2. The region map covered 9 of 20 published regions; the other 11 took the
   "unknown region" branch and reproduced the bug. Completed from the AWS
   source table (incl. ca-central-1's cac1-az4), added a snapshot date and a
   documented update path following microvm-regions.ts. The old test asserted
   the map's key list against itself — it could not detect drift or a wrong ID,
   and used eu-north-1 (which IS supported) as its unsupported example. Now
   deep-equalled against an independently transcribed literal.

3. The source URL 404'd in all four places it appeared. Replaced with the
   canonical docs.aws.amazon.com page (verified 200).

4. The documented CI/CD remedy could not be applied on the CI/CD path:
   cdk.context.json is gitignored AND regenerated by build.yml, and -c at
   deploy time cannot change an already-synthesized assembly. build.yml's
   "Generate CDK context" step now folds in an AGENTCORE_AVAILABILITY_ZONES
   repo variable, and the guide documents that hook, the two dead ends, and an
   upgrade note for existing stacks (subnet AZ is create-only).

5. The override was shape-validated only, so ["us-east-1b","us-east-1b"]
   (single-AZ wearing a two-AZ costume) and ["use1-az2","use1-az4"] (zone IDs,
   i.e. column 2 of the command the guide prints) both synthesized. Now
   requires >=2 distinct zone *names*, rejects zone-ID-shaped values with a
   pointed message, requires the region prefix when the region is concrete, and
   cross-checks against the supported set whenever the mapping is knowable.

Should-fix items: deterministic pin (sorted, not DescribeAZ response order, so
a reshuffle cannot replace every subnet); sts:GetCallerIdentity guard so
`cdk deploy --profile prod` cannot read the mapping from the default profile's
account; makeClient() for solution UA (aws-samples#319); 5s timeouts + maxAttempts=2 +
state=available filter; error *name* not message (authz text echoes caller
ARNs into cdk.out, which CI uploads); AgentVpc throws on availabilityZones +
maxAzs instead of silently dropping maxAzs; prop renamed to
agentCoreAvailabilityZones to stop colliding with Stack.availabilityZones;
MIN_AGENTCORE_AZS split from AUTO_PIN_AZ_COUNT with the AgentVpc coupling
asserted; removed the untrue AI004 comment (masking scan still clean).

Tests: all 5 surviving mutations from the review now fail the suite (verified
by re-applying them); agentcore-azs.ts goes from 75% to 100% function coverage
via mocked live-lookup tests that also assert the filters, timeouts and UA.
Verified end-to-end with the real CLI: env-agnostic synth is clean+warned and
unpinned; env-agnostic + override pins subnets to the requested zones; a
zone-ID override is rejected; a failed lookup exits 1.
@AshrafBen10

Copy link
Copy Markdown
Contributor Author

Thank you for withdrawing the approval and re-reviewing the redesign from scratch — finding 1 was real, and you're right that it's what let the other four hide. Pushed 9f0f247. I reproduced each finding before fixing it, and verified the fixes against the real CDK CLI rather than only the unit suite.

Blocking

1. Silent degraded paths / annotations dropped. Confirmed exactly as you described. The resolver now returns { zones, diagnostics } and main.ts attaches them with applyAgentCoreAzDiagnostics(stack, …) after the stack exists.

One correction to the mechanism, in your favour: Annotations.addError does not make in-process app.synth() throw — I asserted that first and the test failed. It's the CLI that fails closed. Verified with a probe app in this tree:

ERROR [AgentCore AZs] probe error annotation (Construct Annotations)
   Rule error-annotation
Synthesis finished with errors
$ echo $?  → 1

Since every real path (mise //cdk:synth, cdk deploy, build.yml) goes through the CLI, addError is genuinely fail-closed — so I kept your suggested shape and wrote the test against the condition the CLI keys on (app.synth().getStackByName(id).messages filtered to level === 'error'), with a comment recording why it isn't a toThrow.

Severity split: failure to complete an attempted auto-pin (lookup denied/throttled, account mismatch, fewer than two supported zones) is error; genuinely-unknowable states (env-agnostic synth, region absent from the map) are warning — the unknown-region branch no longer emits nothing.

Regression proof: re-applying the original bug (applyAgentCoreAzDiagnostics(app, …)) fails 3 tests in test/main.test.ts.

2. Map covered 9 of 20 regions. Completed from the AWS table — the 11 you listed, including ca-central-1's cac1-az4. Added AGENTCORE_SUPPORTED_AZ_IDS_SNAPSHOT_DATE and a "this list rots by design" update path modelled on microvm-regions.ts, as suggested. The tautological test is gone: the map is now deep-equalled against an independently transcribed literal, so 'us-east-1': ['usw2-az1'] fails. Also fixed the test that used eu-north-1 as its "unsupported region" example (it's supported — now me-central-1).

3. 404 source URL. Confirmed 404 on the old link, 200 on docs.aws.amazon.com/bedrock-agentcore/latest/devguide/agentcore-vpc.html#agentcore-supported-azs. Replaced in all four places.

4. CI/CD remedy inapplicable on the CI/CD path. Confirmed both dead ends (gitignored + regenerated cdk.context.json; -c powerless against a pre-synthesized assembly). Took your recommendation and wired build.yml: the "Generate CDK context" jq block now folds in an AGENTCORE_AVAILABILITY_ZONES repo/environment variable, omitting the key entirely when unset. The guide now leads with a per-path table, states plainly that the pipeline is not auto-pinned, names both dead ends so nobody retries them, and adds the upgrade note (subnet AZ is create-only → replacement cascade; run mise //cdk:diff first).

Verified end-to-end, env-agnostic, no credentials:

  • no override → exit 0, warning emitted, subnets Fn::GetAZs (CI stays green)
  • -c 'agentcore:availabilityZones=["us-east-1b","us-east-1c"]' → exit 0, subnets pinned to exactly those two zones

5. Override shape-validated only. Both of your synth repros now fail synth: duplicates (["us-east-1b","us-east-1b"]) via a ≥2 distinct check, and zone IDs (["use1-az2","use1-az4"]) via a shape check whose message names the column mix-up — verified through the CLI. Added a region-prefix check when the region is concrete, and the cross-check against AGENTCORE_SUPPORTED_AZ_IDS whenever the mapping is knowable (unsupported/nonexistent zones → error; an unusable lookup only downgrades to a warning, so the escape hatch still works offline).

Should fix — all taken

  • Non-deterministic pinselectSupportedAzNames sorts; test asserts identical output from reversed input.
  • Credential-boundary shiftsts:GetCallerIdentity guard; a mismatch is an error and the AZ lookup is never issued. Needed @aws-sdk/client-sts (second dependency — flagging it under AGENTS.md "ask first").
  • Naked clientmakeClient(EC2Client/STSClient, …); asserted customUserAgent in tests. (Small correction: blueprint.ts / lambda-microvm-compute.ts import from handlers/shared/*, not ua.ts — so the cross-directory precedent holds, the makeClient-from-constructs/ one is new here.)
  • No timeout/attempt cap → 5 s request+connection timeouts, maxAttempts: 2, plus state=available in the filter.
  • AI004 comment claimed the opposite → comment deleted; the failure now genuinely reaches the caller, and the masking scan is still clean (0 findings).
  • Prop name collisionagentCoreAvailabilityZones.
  • AgentVpc silent precedence → throws on availabilityZones + maxAzs, guards on ?.length, enforces the ≥2 floor at the construct boundary.
  • err.message leaking authz ARNs → error name only; asserted the message contains no arn:aws:iam::.
  • Test gaps → all five surviving mutations now fail (each re-applied and confirmed). agentcore-azs.ts went 75% → 100% function coverage (99.6% stmts, 95.4% branches) via mocked live-lookup tests that also assert the filters/timeouts/UA.

Nits

MIN_AGENTCORE_AZS split from AUTO_PIN_AZ_COUNT with the AgentVpc coupling asserted by synthesis. DescribeAzsFn is now referenced by the tests. Token.isUnresolved kept but re-documented as explicitly defensive (a future caller passing stack.account) and covered by a test. Dependency placement rationale recorded in the module JSDoc — import-x/no-extraneous-dependencies only allows devDependencies under test//build-tools/, so moving them would mean widening the allowlist for a src/ file.

Two I did not action, deliberately:

  • bedrock-models.ts (fix(cdk): resolveBedrockModelIds rejects the documented -c string override #628) as a second logical change. You and @isadeks are both right about CONTRIBUTING §3. It's here because I was explicitly asked to fold it into this PR rather than ship it standalone. The commit is self-contained (d1fcec5) — say the word and I'll pull it back out into its own PR against main.
  • main.ts blame churn. Already spent, and buildApp() extraction (which is what closes three of the five mutations) touches the same lines again.

Governance

Accurate and I can't fix it from here: #353 has no labels and is closed-as-completed while the fix is still open, and #628 is unlabelled. Both need a maintainer — I don't have permission to label or reopen on aws-samples (self-assign already failed with ReplaceActorsForAssignable). This PR does hit both "ask first" boundaries (new construct module, two new dependencies); calling that out explicitly rather than treating the earlier approval as covering it.

CI

Still fork-only, so only Validate PR title reports — your point that every round has been reading rather than running is fair, and it's why I ran the CLI probes above. Local: 319 tests pass across the 13 suites touching this change (incl. test/bootstrap); compile, ESLint, masking scan clean; docs mirror in sync. I could not run the full 124-file suite in one pass locally (it gets OOM-killed in my sandbox), so the aggregate Codecov number still needs a real CI run — worth a maintainer dispatching the workflows before the next pass. I did not see the "throws on a non-string / empty entry" flake in this round; that test now asserts an exact message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

5 participants