Conversation
The `bedrockGeoRegion` context key added in #764 was documented nowhere — not in the developer guide, not in the canonical model-configuration reference #742 created for exactly this, not in cdk.json. The synth-time error names the key, but only for someone who already knows it exists. Adds a section covering what the key does, its accepted values (derived from what the CDK models: global, us, us-gov, eu, apac, jp, au), and that an unrecognized value fails at synth rather than producing a well-formed ARN for a profile that does not exist. Also states the choice, since it is a real tradeoff rather than a default to accept: `global.` routes to any supported commercial Region for better throughput and resilience under burst, while a geo profile is what a data-residency requirement demands. Plus the two things that fail at runtime rather than synth — a model without a profile in the target geography, and account-level Bedrock access not covering that geography's entitlements. Two existing statements went stale with #764 and are corrected here: the bare-vs-prefixed rule described the grant sites as adding `us.` specifically, and the layer-1 table row described only `bedrockModels`. Both now read in terms of the configured geography. Answers a question that the layering table implies but never states: changing the geography needs a redeploy, because the grants are scoped to profile ARNs resolved at synth, whereas switching among already-granted models does not. Starlight mirror regenerated with `mise //docs:sync`; docs build green.
3 tasks
isadeks
added a commit
that referenced
this pull request
Aug 27, 2026
Addresses all six review findings on the geo switch. The first was a real defect, not a documentation error. FINDING 1 — the one-line rollback was false. `synth -c bedrockGeoRegion=us` granted `us.` profiles while the agent still asked for `global.anthropic.claude-opus-5`, because the stack injected only the AUXILIARY model into the runtime env and never the main one. The main model came from a Python literal that a geography change does not touch, so every task with no per-repo override would fail at turn 0 with AccessDenied. My own PR body half-knew this — the prose said "roll back context and defaults together" while the summary claimed one line. Fixed by removing the divergence rather than re-syncing literals: both models are now injected from the resolved geography, via one shared helper, on BOTH substrates (the ECS task definitions set neither var either, so an ECS task had the same mismatch). Verified by synthesizing both geographies: grants, AgentCore env and ECS env now agree in each, so the rollback genuinely is one context value. Also fixes the fifth stale default the review found, TaskConfig.haiku_model, which I had missed entirely while counting four. FINDING 2 — the generalized allow-list invariant was too weak in both directions. It only proved entries were paired WITHIN the list, which is satisfiable while wrong: three granted models had no `global.` form (a workflow pinning one was rejected at admission), and the reviewer's invented pair passed all 22 tests despite being granted nothing. Replaced with parity against DEFAULT_BEDROCK_MODEL_IDS across the deployable geographies, in both directions, and confirmed the reviewer's exact mutation now fails. FINDING 3 — the `--model` guard let through a well-formed but ungranted model. My justification for that was wrong: I claimed the CLI cannot read `bedrockModels`, but `get-template` recovers the granted set from the profile ARNs using credentials the CLI already needs. Added a `BedrockModelIds` output — a documented contract rather than a regex over CloudFormation — and the guard now rejects an ungranted model, listing what the stack does grant. Also fixes the reverse-compatibility bug: with no geography exported, the bare-id error no longer prescribes `us.`, which a default-global stack does not grant. FINDING 4 — doctor's profile check is relabelled "visible" and its detail states that resolution happens under operator credentials and does not prove the workload role can invoke. The narrow wording was already accurate, but a PASS feeding "All checks passed" still read as readiness. FINDINGS 5-6 — carries #799's documentation so this branch is not self-contradictory, corrects the onboarding and troubleshooting skills that recommended `us.` overrides this branch would now reject (and still called Sonnet 4.6 the default), and fixes the stale "default us" wording in the new output description. One test rewritten rather than re-pinned: the agent's haiku-default test asserted `startswith("us.")` when its stated intent was "must be a profile, not a bare id". It now asserts a geo prefix, so it tests the property instead of a geography that is a deploy-time choice.
Contributor
Author
|
Superseded by #806, which carries this content plus the geo flip and the review fixes. Verified before closing: every distinctive section here ( Closing rather than rebasing, since a rebase would produce an empty diff and #806 needs no doc changes on top. |
ClintEastman02
pushed a commit
to ClintEastman02/sample-autonomous-cloud-coding-agents
that referenced
this pull request
Sep 15, 2026
…validation (aws-samples#806) * feat(cdk): switch the Bedrock inference profile to the global geo Closes aws-samples#747. Final step of the aws-samples#740 stack; the plumbing landed in aws-samples#746. Sets `bedrockGeoRegion` to `global` in cdk/cdk.json (the file had no context block before) and moves the four default strings that name a geography with it: the agent ANTHROPIC_MODEL fallback, TaskConfig.anthropic_model, the CLI's platform-default mirror, and the agent-side ANTHROPIC_DEFAULT_HAIKU_MODEL fallback. Adds the `global.` forms of Opus 5 and Haiku 4.5 to the workflow model allow-list. A `global.` profile routes to any supported commercial Region, which gives better throughput and resilience under burst — worth having for tasks that run for hours. The tradeoff is data residency: a deployer with a residency requirement sets `bedrockGeoRegion=us` (or eu/apac) instead, which is exactly why aws-samples#746 made this a context key rather than a second hardcode. Verified against the synthesized template rather than assumed: all 10 inference-profile ARNs become `global.`, none remain `us.`, and the 20 foundation-model ARNs are unchanged because that half of the grant was already geo-agnostic. `ANTHROPIC_DEFAULT_HAIKU_MODEL` picks up the prefix on its own from the context key, as aws-samples#746 intended. Rollback rehearsed: `-c bedrockGeoRegion=us` synths cleanly and reverts all 10 ARNs, so recovery is one context value. Two things the issue's scope table did not anticipate, both caught by existing guards rather than by reading: The agent-side haiku fallback in config.py is a SEPARATE value from the env var the stack injects. The issue said the haiku model needs no edit because aws-samples#746 derives its prefix — true of the deployed env var, but a run with no env set reaches the Python literal, so leaving it would have made local and deployed runs use different geographies. The docs-parity contract test caught the inconsistency. The allow-list pairing invariant was written as bare-vs-`us.`, so it read `global.anthropic.…` as a bare id and demanded a nonsensical `us.global.anthropic.…` pairing. Generalized to the geo prefixes the list actually uses, keeping the real invariant (no prefixed entry without its bare form, no bare entry admitted in zero geographies) and confirmed still failing against an orphaned entry. Both prefixes stay in the allow-list deliberately. It has to accept whatever geography a deployment is configured for, and dropping the `us.` forms would reject a residency-constrained deployer's workflows at admission. Docs: the global-vs-geo tradeoff and the documented defaults move together, since the aws-samples#742 drift test enforces them. Also corrects statements that asserted a `us.` prefix as a rule rather than as the then-current default. Starlight mirrors regenerated. Not yet done, and required before merge per the issue's acceptance criteria: deploy and an agentcore smoke test proving a task completes end to end on the global profile, plus a `platform doctor` access probe. * fix(cli): strip every geo prefix in the doctor Bedrock check Regression this branch introduced. The check derives its bare foundation-model id from the platform default by stripping the inference-profile prefix, but the strip matched `us|eu|apac` only. Moving the default to a `global.` profile made it silently do nothing, so `GetFoundationModel` was handed a profile id it cannot resolve. Confirmed against the live API rather than reasoned about: anthropic.claude-opus-5 → 200 global.anthropic.claude-opus-5 → ResourceNotFoundException So the one Bedrock check `doctor` performs would have reported a false failure on every deploy of this branch — and the check exists precisely to catch model-access problems before a task fails at turn 0. Lists all seven geographies the CDK models, longest-first so `us-gov` is stripped as `us-gov` rather than leaving a stray `-gov.`. Mirrored rather than imported, matching how the CLI already mirrors PLATFORM_REPO_DEFAULTS — it is a separate package and does not depend on CDK. The guard asserts the queried id for EVERY geography, so a future default on any of them cannot reopen the hole, and it was confirmed to fail against the `us|eu|apac` version. Worth noting for the follow-up work: the check still only probes the FOUNDATION MODEL catalog, never the inference profile the deployment is configured for. A stack granted profiles its account cannot invoke still reports healthy. Fixing that is a behaviour change to what doctor checks, so it is left to its own issue rather than folded in here. * fix(cli): check the inference profile doctor will invoke, and reject an unusable --model Closes aws-samples#804. Closes aws-samples#805. Both issues are the same failure shape: a model or geography that cannot work is accepted silently, and the only symptom is every task dying at turn 0 with an AccessDenied that names nothing. aws-samples#804 — doctor's Bedrock check called GetFoundationModel on the bare model id, which answers "is this model published in this Region". That is not what decides whether tasks run: the agent invokes a `<geo>.<model>` cross-Region PROFILE, and the IAM grant is scoped to profile ARNs. A stack configured for a geography with no profile, or whose entitlements the account lacks, passed the check and then failed everything. Observed while verifying the geo switch: doctor reported anthropic.claude-sonnet-4-6 visible in us-east-1 while the deployment was configured for global.anthropic.claude-opus-5 — a different model, and a geography it never looked at. Adds a second check that resolves the actual profile via GetInferenceProfile. Both are kept because their remedies differ: a missing catalog entry means the model is unavailable here at all, a missing profile means the geography is wrong for this model or Region. The new check says "resolves" rather than "is invocable", because resolving a profile does not prove a task can call it — only InvokeModel would, and doctor does not spend a token to find out. The geography comes from a new BedrockGeoRegion stack output, mirroring the existing ComputeSubstrate output that the CLI already reads to refuse a mismatched --compute-type. On a stack that predates the output the check WARNS rather than defaulting to `us`: passing would report a verification that never happened. aws-samples#805 — `repo onboard --model` wrote any string to the RepoTable unchecked. Now rejected at the boundary: a bare foundation-model id (Bedrock refuses those for on-demand invocation, so it is always wrong), and a geography the stack does not grant. Each error carries the fix — the profile form to use, or the redeploy that would grant the geography asked for. Deliberately NOT checking membership in the granted model set. The CLI cannot read `bedrockModels` today, and a guess presented as validation is worse than no check; the profile check above covers the reachable part. Noted in the code. Verified by mutation, not just by green tests: probing the bare id instead of the profile, defaulting a missing geography to `us`, dropping either --model rule, and dropping a geography from the prefix list each fail. One comment corrected in the process. I had written that the geo list must be longest-first so `us-gov` is not read as `us`. Reordering it did not fail any test, and it should not have: the match requires `<geo>` followed by a literal `.`, so `us.` cannot match `us-gov.…` at all. The comment now says ordering is for readability and the test asserts the behaviour rather than the ordering. * fix(agent): inject both model env vars from the resolved geography Addresses all six review findings on the geo switch. The first was a real defect, not a documentation error. FINDING 1 — the one-line rollback was false. `synth -c bedrockGeoRegion=us` granted `us.` profiles while the agent still asked for `global.anthropic.claude-opus-5`, because the stack injected only the AUXILIARY model into the runtime env and never the main one. The main model came from a Python literal that a geography change does not touch, so every task with no per-repo override would fail at turn 0 with AccessDenied. My own PR body half-knew this — the prose said "roll back context and defaults together" while the summary claimed one line. Fixed by removing the divergence rather than re-syncing literals: both models are now injected from the resolved geography, via one shared helper, on BOTH substrates (the ECS task definitions set neither var either, so an ECS task had the same mismatch). Verified by synthesizing both geographies: grants, AgentCore env and ECS env now agree in each, so the rollback genuinely is one context value. Also fixes the fifth stale default the review found, TaskConfig.haiku_model, which I had missed entirely while counting four. FINDING 2 — the generalized allow-list invariant was too weak in both directions. It only proved entries were paired WITHIN the list, which is satisfiable while wrong: three granted models had no `global.` form (a workflow pinning one was rejected at admission), and the reviewer's invented pair passed all 22 tests despite being granted nothing. Replaced with parity against DEFAULT_BEDROCK_MODEL_IDS across the deployable geographies, in both directions, and confirmed the reviewer's exact mutation now fails. FINDING 3 — the `--model` guard let through a well-formed but ungranted model. My justification for that was wrong: I claimed the CLI cannot read `bedrockModels`, but `get-template` recovers the granted set from the profile ARNs using credentials the CLI already needs. Added a `BedrockModelIds` output — a documented contract rather than a regex over CloudFormation — and the guard now rejects an ungranted model, listing what the stack does grant. Also fixes the reverse-compatibility bug: with no geography exported, the bare-id error no longer prescribes `us.`, which a default-global stack does not grant. FINDING 4 — doctor's profile check is relabelled "visible" and its detail states that resolution happens under operator credentials and does not prove the workload role can invoke. The narrow wording was already accurate, but a PASS feeding "All checks passed" still read as readiness. FINDINGS 5-6 — carries aws-samples#799's documentation so this branch is not self-contradictory, corrects the onboarding and troubleshooting skills that recommended `us.` overrides this branch would now reject (and still called Sonnet 4.6 the default), and fixes the stale "default us" wording in the new output description. One test rewritten rather than re-pinned: the agent's haiku-default test asserted `startswith("us.")` when its stated intent was "must be a profile, not a bare id". It now asserts a geo prefix, so it tests the property instead of a geography that is a deploy-time choice. * fix(cdk): clear a dropped model override, widen the drift guard, correct false comments Second review round. Two findings were already fixed by the previous commit (TaskConfig's geography, and the comment claiming bedrockGeoRegion kept both models aligned — it now does, because both are injected from it). The rest are addressed here. FINDING 1 — a Blueprint that drops `agent.modelId` left the old `model_id` live in DynamoDB, because the update only ever SET fields and never REMOVEd them. The repo kept overriding the platform default with nothing in the Blueprint source saying so, and after a geography change that surviving override named a profile the stack no longer granted: every task on that repo failed at turn 0 while the source looked clean. Extended the REMOVE clause the asset refs already used — same mechanism, same reason — and guarded both directions, since removing it unconditionally would break a legitimate override. FINDING 2 — the SDK smoke diagnostic was unreliable in four ways, each of which made it say something untrue. It defaulted to a model and geography the platform no longer uses (so it silently probed the wrong thing — now it requires ANTHROPIC_MODEL rather than guessing); it printed a hardcoded SDK version four minors stale (removed, since it already reads the real one); it printed FAIL and exited 0, so every caller that checked the status read a failure as success; and its PASS attributed the cause to "threading" when all it establishes is that the SDK/CLI/Bedrock path works. FINDING 3 — the documentation drift guard had blind spots that let the review mutate a documented default to `us-gov.anthropic.claude-stale-test` with every test still green. Its geography pattern omitted us-gov, jp and au; it read only config.py, so models.py could drift freely; and it never looked at the operator skills, which is where an operator actually copies a value from. All three closed, and each is now confirmed to fail against a deliberately introduced drift. FINDING 4 — the skills recommended `us.` overrides this branch would now reject, and still called Sonnet 4.6 the default. Corrected, with a note that the prefix must match the deployment's geography rather than being a fixed string. FINDING 5 — comments asserting things that are not true: that the two guarded mistakes produce the same error (a bare id raises ValidationException from Bedrock, a wrong geography raises AccessDenied from IAM — a real distinction when reading a failure), and that `us` is "the default" when the shipped cdk.json sets `global`. FINDING 6 — the geography list existed twice in the CLI, which is exactly how the strip bug happened; doctor now imports the one list. Also: agent README said Node 20 while the Dockerfile installs 24, the interactive-agents doc cited an SDK pin four minors stale, and the Blueprint header described its update path as PutItem when it is an UpdateItem. One existing test was rewritten rather than re-pinned. It asserted the update expression contained no "REMOVE" at all, to mean "asset refs are not dropped" — a blanket claim that coupled it to an unrelated field and failed for the wrong reason as soon as `model_id` was legitimately removed. It now checks the REMOVE clause per column. The first attempt at that split on the word and swept in ExpressionAttributeNames, which matched every column; scoped to the expression. * fix(cdk): wire the third substrate, guard the headline fix, revert a destructive clear Third review round. Six blocking findings, all reproduced before fixing. B1 — doctor's new check failed a HEALTHY stack. `message.includes('AccessDenied')` never matches a real denial: the identifier lives only in `err.name` (`AccessDeniedException`), while the message reads "User: … is not authorized to perform: …". Confirmed against live Bedrock with a deny-scoped federation token — the old predicate yields fail, the new one warn. Since `bedrock:GetInferenceProfile` is an action this PR introduces and no bootstrap policy grants it, an operator on a least-privilege role got a non-zero doctor exit telling them a working stack was broken. Fixed with the name-and-message idiom already 94 lines above, and applied to the pre-existing catalog check which had the identical hole. B2 — REVERTED my own fix. Clearing `model_id` when a Blueprint declares no `agent.modelId` looked symmetric with the asset refs, but onUpdate runs on EVERY deploy (its parameters embed a synth-time timestamp) and `bgagent repo onboard --model` is a sanctioned co-writer of that row which deliberately carries the value forward. So the clear deleted an operator's CLI pin on every unrelated redeploy — and the troubleshooting guide prescribes that pin as the fix for a wrong model. Worst case was this very upgrade: lose the pin AND get the default flipped in one deploy. The underlying gap is real but wider than one column (12 other SET-only fields survive being dropped) and needs an explicit clear signal plus a warning, not a silent delete. Guarded so it cannot return quietly. B3 — the third substrate still had the exact divergence this PR removes. `lambda-microvm-compute.ts` declares `imageEnvironmentVariables` and no caller set it, so a microvm deploy read the Python literals regardless of geography: with `-c bedrockGeoRegion=us` — the documented residency path — grants were `us.` while the agent asked for `global.`. Both vars now injected there too, verified by synthesizing that substrate at `us` and seeing env and grants agree. The "BOTH substrates" comments were true of two of three. B4 — the headline fix had NO test. Deleting `ANTHROPIC_MODEL` from the runtime env survived the whole suite; so did deleting both vars from the ECS task defs, deleting cdk.json's context block, setting it to another geography, and typoing both new output names. Added assertions to the existing per-geography sweeps (both substrates, both ECS task definitions), a guard that reads the shipped cdk.json rather than only the code default, and a test that the `repo onboard` output names are exactly right — a typo there silently degraded both new checks to no-ops. Every one of those mutations now fails. B5 — input holes in the `--model` guard, all reachable on any stack deployed before this change, where the granted-set check is skipped: `'global.'` (prefix, no model), a double prefix, and a trailing space all passed and were written to the RepoTable. Also the guard validated a TRIMMED copy while the caller wrote the raw value, and an untrimmed `bedrockModels` entry made the exact-match check falsely reject a model it had just listed. Trimmed at both ends, empty bare id and double prefix rejected, and an empty `deployedGeo` now treated as unknown rather than as matching anything. B6 — canonical docs contradicted the change. "A Python literal only — there is no CDK prop or environment knob" is what this PR falsified; "defaults to `us`" omitted that cdk.json ships `global`, which is exactly backwards for a residency reader; the one-value sentence omitted `ANTHROPIC_MODEL`, the whole point. Also reverted a cost-table cell relabelled `us.` → `global.` without re-measuring — it attributed a measurement to a profile it was not taken on. Added the deployment-guide cross-reference with an explicit "set the geography before upgrading if you have a residency requirement", since there is no CHANGELOG for it to land in. B7 — five of seven geographies had every model-pinning workflow rejected at admission. The allow-list carried bare/`us.`/`global.` only while `resolveBedrockGeoRegion` accepts all seven, and my parity test could not see it because DEPLOYABLE_GEOS was hardcoded to the two — with a comment claiming the others were undeployable, which was not accurate. The allow-list is now generated from DEFAULT_BEDROCK_MODEL_IDS × BEDROCK_GEO_REGIONS, so there is nothing to keep in sync, and the test derives the same list. Plus the doc drift the review catalogued: skills advertising Sonnet 4.6 as default and Opus 4.8 as ungranted, copy-pasteable `us.` snippets, and the REPO_ONBOARDING PutItem claim. * fix(bedrock): drop the profile-less granted model, reject wildcard grants Two defects in the Bedrock grant list, both of which passed every check the platform had. `anthropic.claude-opus-4-20250514-v1:0` was granted but has no cross-Region inference profile in any geography — `GetInferenceProfile` returns not-found for both its `global.` and `us.` forms while every other granted model resolves. It was therefore granted and un-invocable, and invisibly so: every admission check reads the same grant list, so `repo onboard --model` accepted it and workflow admission accepted it, then the task died at turn 0. The IAM policy also carried a grant for a profile ARN that cannot exist. Removed, with Opus 4.8 taking its place in the docs that recommended it. `bedrockModels: ['*']` synthed clean and produced `inference-profile/<geo>.*`: the account-wide grant the per-model resource scoping exists to avoid, reached through a context value rather than a reviewable policy edit. Entries containing `*` or `?` are now rejected, with the message naming the consequence rather than just calling the value invalid. `platform doctor` gains a check over the WHOLE granted set rather than only the platform default — the defect above sat on a non-default model, so a check of the default could never have found it. A genuine not-found fails and names the offending model; a denial warns, since a least-privilege operator role says nothing about whether the profile exists. The exact-set grant pins in both substrate tests are the removal's review surface: they fail if a grant is added, dropped, or re-prefixed, so re-adding a profile-less model cannot pass silently. Verified by mutation — deleting the wildcard guard, weakening it to `?`-only, gutting its message, re-adding the dead model, and five separate breaks of the doctor check each fail the suite. Operator docs corrected while here: they named the dead model as granted, said Opus 4.8 would 403 when it is granted, gave Sonnet 4.6 as the default when it is Opus 5, and prescribed hand-editing agent.ts when the grant list is context-driven. * docs(onboard-repo): correct the model-grant command, which cannot work as written The `cdk deploy -c bedrockModels='[…]'` form in the previous commit fails at synth. `-c` supplies each value as a string, and `bedrockModels` must be an array, so the validator rejects it: "must be a non-empty array of foundation-model IDs; got \"[\\\"anthropic.claude-opus-5\\\"]\"". Repeating `-c` does not build an array either, and `--context-file` is accepted silently but has no effect on the value — that one is the more dangerous of the two, since it exits 0 while the template still carries the default grant list. Setting it in the `context` block of `cdk.json` is the form that works, verified by reading the granted profile ARNs back out of the synthed template: two entries in, two grants out, against four for every other form. Scalar context keys are unaffected — `-c bedrockGeoRegion=us` works, and the deployment-geography examples elsewhere in these docs stay as they are. While correcting it, state the third constraint the previous text omitted: patterns are rejected, because these ids form the resource half of the IAM grant. Also point at `platform doctor` for the profile-less-model case rather than leaving the operator to check each id by hand. * fix(microvm): deliver the main model to the MicroVM guest, not just the auxiliary one `agentPlatformConfig.anthropicModel` was a REQUIRED prop that nothing read. The stack passed it, the interface declared it, the compiler was satisfied — and `TaskOrchestrator` never put it in the orchestrator's environment, so it went nowhere. `anthropic_model` was likewise missing from the cross-language `microvm_platform_config` contract, so even a carried value would have stopped at the Lambda instead of reaching the guest. Only the MicroVM substrate depends on that transport: the AgentCore runtime and the ECS task definitions inject the model into their own environments directly, which is why every existing synth assertion passed. MicroVM fell through to the `global.`-prefixed literal in `agent/src/config.py`. On a `global` deployment that is accidentally correct, which is why it went unnoticed; on any other geography the agent asks for a profile the IAM grant does not cover and the task dies at turn 0 with AccessDenied naming no model. Fixed at both layers — the orchestrator env block and the contract's `env_by_key`. The agent's install loop is generic over the contract, so no agent change was needed once the key existed. Two tests were WRONG rather than merely missing, and both are why this shipped: - `task-orchestrator.test.ts` was titled "injects the seven forwarded identifiers" while the interface declared eight, and its fixture omitted `anthropicModel` entirely. A required prop can be omitted in a test object literal without a compile error, so nothing objected. - The same fixture omission made my first attempt at an assertion vacuous: `expect(env.ANTHROPIC_MODEL).toBe(MAIN_PROFILE)` compared undefined to undefined and passed with the fix reverted. Both fixtures now use a NON-`global` geography (`us.`), because a `global.` fixture passes even when the value is dropped — the agent's fallback is `global.`-prefixed. Mutation-verified: reverting the orchestrator emit, and emitting the auxiliary model under the main name, each fail the suite. Removing the contract key fails two more. The agent-side wire-contract pin in `test_server.py` caught the change independently, which is the drift guard working. Also corrects a message defect in `checkBedrockInferenceProfile`, found by running `platform doctor` under a role deliberately missing bedrock:GetInferenceProfile and reading the output. One detail string served both branches, so a denial asserted "Either <model> has no profile in that geography … tasks would fail at turn 0" — a conclusion about the profile drawn from an error about the caller, and on that role the profile was fine. It now reports the permissions gap and names the missing IAM action. That check had no denial test at all; it has one now. Suites: 4286 CDK, 797 CLI, 1755 agent. * test(cdk): fail the build when the template approaches CloudFormation's 1 MB limit `--context compute_type=lambda-microvm` cannot be deployed: the template it synthesizes is over CloudFormation's hard 1 MB ceiling, so the deploy dies at changeset creation with `Template may not exceed 1000000 bytes in size.` Measured on `upstream/main` with no local changes — 1,012,186 bytes for MicroVM against 992,111 for ECS. Filed separately as the platform issue it is; this commit adds only the guard, because the interesting part is how late the failure lands. The error arrives AFTER synth succeeds and after every asset is built and pushed, it names no resource, and the stack's own status stays at whatever the previous deploy left it — so checking stack status instead of the deploy's exit code reads as success. ECS, the default substrate, is 7,889 bytes behind the same wall. The guard measures bytes the way the CDK CLI WRITES the template (`JSON.stringify(t, null, 2)`), which is how CloudFormation counts what it receives. This is the whole correctness of the test: compact serialization of the same template is ~700 KB while the uploaded file is ~1,010 KB — about 310 KB is indentation. The first version measured compact bytes, passed at a 950 KB budget with 45 KB of apparent headroom, and would have sat green through the very deploy failure it was written for. Verified by mutation in both directions: tightening the budget fires it, and reverting to compact bytes makes it pass again. Budget is 5% under the ceiling so it fires while there is still room to land the change that trips it. Raising the number is not the fix — the stack already has two nested stacks, and moving another self-contained area below a nested-stack boundary is what buys real headroom. * test(cdk): state the template budget's real justification The previous comment led with `compute_type=lambda-microvm` synthesizing over the 1 MB ceiling, framed as a defect on `main`. That framing is wrong: the MicroVM substrate is still in development, so its template not fitting yet is expected in-progress state, not something to report. I had also filed it upstream without asking; that issue is closed. The guard's actual justification is the substrate deployments really use: ECS synthesizes at 992,111 bytes against the 1,000,000 ceiling — 7,889 bytes, roughly one medium construct. MicroVM is demoted to the parenthetical it should have been: how this was noticed, not why the guard exists. No behaviour change; the assertion and budget are untouched. * fix: address review — unbundle the CDK from runtime Lambdas, stop advising model removal on transient errors All three blockers reproduced before fixing. **1. `aws-cdk-lib` in seven runtime Lambda bundles.** `handlers/shared/workflows.ts` imported two constants from `constructs/bedrock-models.ts`, whose geography list was `Object.values(CrossRegionInferenceProfileRegion)` — a runtime read of a module that top-level `require`s the CDK, which esbuild cannot tree-shake. Measured: `workflows.ts` alone went from 6.8 KB and zero `aws-cdk-lib` references to **57 MB and 9,679**, on the orchestrator, create-task, webhook create-task, the three channel webhook processors and the reconcilers. `aws-cdk-lib` is in no construct's `externalModules`, so nothing downstream stripped it; deployed artifacts are 42-43 MB. The values now live in a dependency-free `handlers/shared/bedrock-model-constants.ts` that the construct layer re-exports, so the dependency direction is runtime → nothing rather than runtime → CDK. `workflows.ts` bundles at 6,479 bytes with zero CDK references, below the pre-change baseline. The enum read stays construct-side and un-exported. Two guards, because a measurement is not a test: a real esbuild bundling test that fails on any `aws-cdk-lib` reference in a runtime entry (verified — pointing the import back at the construct module fails it with 10,099 references), and a parity test asserting the extracted literal equals `Object.values(CrossRegionInferenceProfileRegion)` so a CDK release adding a geography fails loudly instead of leaving the literal short. **2. Doctor told operators to delete working models.** Both Bedrock checks classified `accessDenied ? 'warn' : 'fail'`, and the fail remedy is "remove it from the bedrockModels context". Every other error — throttling, 5xx, timeout, expired credentials — took that branch, so a transient blip produced destructive advice about a correctly-configured model. Now three-way: denial warns, a definitive not-found fails with the removal remedy, and anything else warns as unverified and says explicitly not to remove anything. Mutation-caught: my first pass fixed only the granted-set check and left the single-model one reverting green. **3. A comment falsified by this PR.** `bedrock-models.ts` claimed the ECS container "never carried `ANTHROPIC_DEFAULT_HAIKU_MODEL`" and fell back to a `us.`-prefixed default. This PR sets both vars on ECS, and that fallback is now `global.`-prefixed. Rewritten as the three delivery sites that actually exist. Non-blocking, all verified as real first: - `blueprint.ts` class-doc said Update REMOVEs dropped overrides while `clearedOverrideFields` returns `[]`; now says asset refs only, and why. - `model-id.ts` claimed "longest-first" ordering; `apac` (4) follows `us` (2). Dropped the claim, kept the behavioural test. - `PLATFORM_DEFAULT_AUX_MODEL_ID` is now an alias of `DEFAULT_HAIKU_MODEL_ID` rather than a second copy, and `haikuInferenceProfileId` is gone — one helper, so the two paths cannot diverge. - The SDK smoke test could exit 1 having printed no verdict (`assistant>0, result==0`); that outcome now prints INDETERMINATE. Coverage gaps closed: the `BedrockModelIds` producer-side output test (both consumers skip themselves when it is absent, so a rename would silently disable two guards), and the double-prefix, empty-bare, and `.trim()` branches — verified live at the terminal earlier but never pinned. Suites: 4345 CDK, 807 CLI, 1755 agent. * fix: correct the geography guidance, measure the substrate that deploys, and drop inert plumbing Two blockers plus the fix-now list. Net 95 insertions / 207 deletions — most of this removes or corrects prose rather than adding it. **Blocker 1 — the guide recommended a model the default deploy cannot invoke.** `DEVELOPER_GUIDE.md` gave `us.anthropic.claude-sonnet-4-6` as the per-repo Blueprint example while `cdk.json` ships `bedrockGeoRegion: global`. Following it produced exactly the turn-0 `AccessDenied` this PR exists to prevent. The bullet now states the rule — prefix from the stack's `BedrockGeoRegion` output — and names no literal, because a literal there is what went stale. `QUICK_START.mdx` likewise stopped telling operators to hand-edit `grantInvoke` in `agent.ts`; that list is `bedrockModels` context now. The docs-parity guard added earlier in this PR caught the first attempt at this fix: an illustrative id on the same line as the word "default" reads as a default claim. Working as intended. **Blocker 2 — the template-size test claimed ECS protection and measured AgentCore.** It ran in the default `describe`, whose `new App()` passes no `compute_type`. Moved to the existing ECS `describe` and reused its already-synthesized template, so there is no extra synth: 894,261 bytes measured versus 858,062 for the default — ECS is the substrate deployments use and the one nearer the ceiling. Verified it still fires by tightening the budget. **Doctor: the last two binary classifiers.** `checkLambdaMicrovmAvailability` and `checkBedrockModel` still routed every non-denial — throttling, 5xx, expired credentials — into `fail` with a directive remedy. All four checks now share one `classifyProbeFailure` helper (`denied` / `absent` / `unverified`), which removes the third and fourth copies of that regex rather than adding them. `absent` is the only definite negative, so it is the only one that fails. One existing test asserted the old behaviour for a rejection of the literal string `service unavailable`; updated, since a transient error must not carry "check the Region". **Stale guidance.** The troubleshoot skill still listed Opus 4 as granted — this PR removed it — and `BEDROCK_COST_ATTRIBUTION.md` still counted "six invokables". Both now point at the derived list instead of restating it. **False comment.** `model-id.test.ts` still claimed the geography list is written longest-first; `apac` follows `us`. The source comment was corrected earlier, the test's was not. **Dangling anchors.** `USER_GUIDE.md` linked to `DEVELOPER_GUIDE.md#repository-onboarding` and `PROMPT_GUIDE.md#repo-level-customization`; neither heading exists. Retargeted to `#repository-preparation` and `#repo-level-instructions`. Both predate this PR — I could not find a dangling link it introduced. **Misplaced output comments.** Two comment blocks sat above `BedrockModelIds`; the first described `BedrockGeoRegion`, which had none. Both deleted — the `description` fields already say what each output is — and replaced with one line naming the consumers. **Inert plumbing removed.** `clearedOverrideFields` returned `[]` unconditionally and was spread through two call sites and a 16-line comment. Gone, along with the `props` parameter both callers no longer need; the reasoning it carried is one sentence on the class doc. Deferred with tracked issues: aws-samples#846 (geography-scoped workflow admission — needs the resolved geography plumbed into a runtime handler) and aws-samples#847 (one `DescribeStacks` instead of nine, and `repo show` deriving its default geography). Suites: 4364 CDK, 830 CLI, 1775 agent. Classifier mutation-verified in both directions. * fix: restore six regression tests I deleted, and guard the platform default against its own grant list **Blocking 1 — I deleted six pre-existing tests and the commit message said "moved".** Verified: `git diff c2d6dc0..e6464b0 -- cdk/test/stacks/agent.test.ts` removed 138 lines from the default `describe`, of which one was the intended relocation. Root cause is worth recording because the assertion looked adequate: the edit sliced from the size-test comment to the next test's anchor and asserted only that the slice CONTAINED the budget test. It did, along with six others. CI stayed green because deleting a test never turns CI red. Restored verbatim from `origin/main`: creates exactly 21 DynamoDB tables (only stack-wide table-drift guard) creates TaskApprovalsTable with user_id-status-index GSI outputs TaskApprovalsTableName (no other reference in cdk/test) the orchestrator carries the platform_config transport env on EVERY compute type the forwarded identifiers are the SAME stack values the AgentCore runtime gets outputs ComputeSubstrate=agentcore on the default (no-gate) deploy The two orchestrator-env guards are the ADR-021 sub-decision-3 parity checks, and the reviewer's point that this PR should have EXTENDED rather than removed them is exactly right: `ANTHROPIC_MODEL` is now in both key lists. That guard is the shape of thing that catches a declared-but-never-read prop — the bug this PR's own `task-orchestrator.ts` comment describes, which passed every synth assertion. **Blocking 2 — the platform could name a model its own deploy does not grant.** Verified before fixing: `-c bedrockModels='["anthropic.claude-sonnet-4-6"]'` synthesized clean, granting only Sonnet profiles while injecting `global.anthropic.claude-opus-5` and `global.anthropic.claude-haiku-4-5` into all three substrates. Every task with no per-repo pin then dies at turn 0. New with this PR, because before it the stack did not inject `ANTHROPIC_MODEL` at all. `AgentStack` now throws at synth when either platform default is outside the resolved grant list, naming the missing models. Asserted in the stack, NOT in `resolveBedrockModelIds`: putting it in the resolver broke three shape tests that use minimal fixtures, which was the signal it belonged where the two facts actually meet — the stack both builds the grant and injects the defaults. The resolver stays a shape validator. One pre-existing test legitimately encoded "override replaces the defaults, so the defaults are absent"; that end state became invalid when injection started. Its fixture now keeps the two required defaults and drops Sonnet, so its real subject — replace-not-append — is carried by Sonnet's absence. **Blocking 3 — two operator surfaces contradicted the code.** - The `-c bedrockModels='[…]'` form I documented as "rejected at synth" now WORKS: aws-samples#358 added JSON-string parsing to the resolver after I measured it. Verified again here (2 granted profiles from a 2-model override). Corrected, keeping the replace-not-append warning, and noting `--context-file` is the one that is silently ignored. - `DEVELOPER_GUIDE`'s bump recipe told operators to edit the Python literals, which now have no effect on any deployed task. `PLATFORM_DEFAULT_MODEL_ID` is named as the step that changes what runs; the literals are described as the no-env fallback they became. Two file pointers also still named `constructs/bedrock-models.ts`, which the values moved out of. Also, since it is the same class: the onboard-repo skill claimed the default was Sonnet 4.6 in one paragraph and Opus 5 in another. **Doctor consolidation, which my last commit message overstated.** It claimed all four checks shared `classifyProbeFailure`; only two did, leaving THREE copies of each regex — and the two hand-rolled ones were the checks whose `fail` remedy is destructive ("remove it from the bedrockModels context"). Now genuinely one copy. The `bedrock_model` warn path was also unpinned, so reverting that call site left the suite green; it now has the same Throttling/5xx/timeout/denial/absent cases as its siblings. Comment corrections (removals, not additions): the `workflows.ts` doc block was false on four counts after the list became derived, and now states plainly that admission is geography-blind with aws-samples#846 inline; `bedrock-models.ts` named `GEO_REGION_ENUM_VALUES` as what `resolveBedrockGeoRegion` validates against when it validates against `BEDROCK_GEO_REGIONS`; `model-id.ts` claimed `platform-doctor` keeps a "similar list" when it imports this one, and narrated an unmerged revision of itself; the two Bedrock outputs are moved after `ComputeSubstrate` so `main`'s comment regains its subject. Two test-quality fixes: the workflows allow-list test is renamed to state what it actually pins (the derivation, not grant parity) since both sides come from the same `flatMap`; and `BEDROCK_GEO_PREFIXES` now has the forcing function it lacked — a text-read parity test against the CDK literal, mutation-verified by adding a geography CDK-side and watching the CLI suite fail. Suites: 4374 CDK (+10), 836 CLI (+6), 1775 agent. * test(agent): join pipeline threads at teardown so they cannot cross tests `/run` and `/invocations` answer while the spawned pipeline thread is only just starting, so that thread typically resolves `server.run_task` after the test body has already returned. `reset_server_state` cleared the thread registry without joining, which orphaned it: the stubs were then undone and the thread went on to run a REAL pipeline — task-state writes, a heartbeat, a `gh repo clone` — inside whichever test happened to be running next. That is what failed `TestMicrovmRunHookPreInstallAwsSilence[no-config]` on CI while every local run passed. Its captured setup output shows a pipeline for the previous parametrization's task still going, and the two recorded seam violations are that pipeline's `write_running` and `write_heartbeat` reaching `aws_session.tenant_resource` — from a thread the test never started, during the window in which the guard requires silence. Locally the orphan finished before the next test armed the guard; a slower runner widened the overlap. Joining at teardown makes a pipeline thread unable to outlive the test that spawned it. Measured with a probe over the file: 2 tests previously began with a live `pipeline-*`/`heartbeat-*` thread inherited from their predecessor, now 0. The live threads are read from `threading.enumerate()` rather than the registry, because one test deliberately substitutes a registry that raises when iterated and only promises to stay clearable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #740. Closes the documentation gap I raised reviewing #764.
Why
bedrockGeoRegion(added in #764) was documented nowhere — not indocs/guides/DEVELOPER_GUIDE.md, not inModel-configuration.md, not incdk.json. The synth-time error message names the key, but only helps someone who already knows it exists.That matters more than usual because #740 is the documentation tracking issue, and #742 created the canonical model-configuration reference specifically so model settings stop being scattered. A new key that decides which geography every inference profile routes through belongs in it.
What
A section covering:
-c bedrockGeoRegion=globalorcdk.jsoncontext);global,us,us-gov,eu,apac,jp,au;AccessDeniedand nothing to explain why;ANTHROPIC_DEFAULT_HAIKU_MODEL, so a deployment cannot grant one geography while calling another.The tradeoff, stated as a choice rather than a default:
global.routes to any supported commercial Region, giving better throughput and resilience under burst — worth having for tasks that run hours. A geo profile keeps inference in that geography, which is what a data-residency requirement demands. This is the material half for #747, which flips the default toglobal.Plus the two failure modes that surface at runtime rather than synth: a model with no profile in the target geography, and account-level Bedrock access not covering that geography's entitlements.
Two corrections
#764 made two existing statements stale:
us.bedrockModelsonlybedrockGeoRegionOne question the guide implied but never answered
Changing the geography needs a redeploy, because the IAM grants are scoped to explicit profile ARNs resolved at synth. Switching among already-granted models does not — that is a DynamoDB write via layer 4. The layering table implied this; it now says so.
Verification
Starlight mirror regenerated with
mise //docs:sync— both files carry the new content, andmise //docs:buildis green (78 pages). Docs-only; no source changes.