feat(cdk): switch to the global Bedrock geo, with doctor and --model validation by isadeks · Pull Request #806 · aws-samples/sample-autonomous-cloud-coding-agents

isadeks · 2026-08-27T14:14:03Z

Closes #747, #804, #805. Supersedes #799 and #800 — both carry no unique content (#800 shares an earlier tip of this branch, #799's docs are folded in here so the branch is not self-contradictory).

Status: the findings from the most recent review round are addressed, main is merged in, and every required check is green at b75e4fa2 — the "changes requested" verdict predates that. Suites: 4499 CDK, 948 CLI, 1782 agent; mise run build clean.

What lands

#747 — the geo flip. bedrockGeoRegion is global in cdk.json. A global. inference profile is granted and invoked on every substrate.

#804 — doctor probes the profile, not the catalog. The old check asked "is this model published in this Region", which passes on a stack whose geography has no profile. It now resolves the <geo>.<model> profile the agent will actually invoke, and separately checks that every granted model has one.

#805 — --model is validated. repo onboard --model accepted any string and wrote it to the RepoTable, failing later at turn 0 with an error naming no model. Five classes are now rejected up front: bare ids, wrong geography, ungranted models, a bare <geo>. prefix, and doubled prefixes.

Plus four defects found while verifying this branch (detail below): a granted model with no inference profile, a context value that widened an IAM grant to a wildcard, a required prop that nothing read, and a documented command that could not work.

The central fix

The main model was never injected. The stack set only ANTHROPIC_DEFAULT_HAIKU_MODEL, so the main model came from a Python literal that a geography change does not touch — a deploy could grant one geography while the agent asked for another, and every task without a per-repo override died at turn 0 with AccessDenied. Both models now derive from the resolved geography, and agentPlatformConfig.anthropicModel is required so a substrate wired without it fails to compile.

Defects found while verifying, and fixed here

Each passed every check the platform had, then failed at turn 0 — or in the last case, never ran at all.

claude-opus-4-20250514-v1:0 was granted with no inference profile in any geography. Every admission check reads the grant list, so the CLI's --model guard and workflow admission both accepted it; the IAM policy carried a grant for an ARN that cannot exist.
bedrockModels: ["*"] synthed into inference-profile/global.* — the account-wide grant the per-model scoping exists to prevent, reached through a context value rather than a reviewable policy edit.
anthropicModel was declared, passed by the stack, and never read. Only MicroVM depends on that transport; the other two substrates inject the model into their own env, so every synth assertion passed while MicroVM fell back to a global.-prefixed literal — correct only on a global deploy.
The docs prescribed a command that could not work. -c bedrockModels='[…]' supplies a string, not an array; the resolver now parses the string form, so the documented command works and both delivery shapes behave identically. --context-file is worse and is called out as unusable: accepted, exit 0, silently ignored.

Two tests were wrong rather than missing, which is how the third defect shipped: an orchestrator test titled "seven forwarded identifiers" ran against an eight-field interface, and its fixture omitted the prop — a required prop can be omitted in a test object literal with no compile error.

Live verification

Deployed to a dev stack and verified by artifact, not exit code.

AgentCore runtime and both ECS task definitions carry ANTHROPIC_MODEL and ANTHROPIC_DEFAULT_HAIKU_MODEL, both geo-derived.
A real task's own session role invoked global.anthropic.claude-opus-5 (5x) and global Haiku (1x) — no us. traffic. Worth splitting the invocation log by caller if reproducing: unsplit it also shows operator calls.
A non-global deploy (-c bedrockGeoRegion=us) flipped every artifact and inverted the CLI guard — global. becomes the rejected value. Restored to global afterwards.
The granted-model check found the profile-less model on the live stack unprompted, naming it and exiting non-zero; after the fix, "All 4 granted model(s) resolve".
The doctor's denial branch was exercised under a role deliberately missing bedrock:GetInferenceProfile: warn, not fail, exit 0.

Three things a reviewer should know

lambda-microvm cannot be deployed yet, so its fix is synth-verified only. That substrate is still in progress, and its synthesized template does not currently fit under CloudFormation's 1 MB limit — the deploy stops at changeset creation. Nothing to act on here; noting it so the MicroVM change in b016d3ec is not read as live-tested. It is mutation-verified instead: reverting the orchestrator emit, or emitting the auxiliary model under the main name, each fail the suite.

e449c7e6 adds a template-size budget with general value independent of MicroVM. CloudFormation hard-fails at 1 MB, and that failure lands after synth succeeds and every asset is pushed, names no resource, and leaves stack status showing the previous deploy's success. The guard measures the pretty-printed template — the shape the CLI actually uploads, so indentation counts — and fails the build at 950 KB, while ECS, the largest substrate that deploys today, currently synthesizes at 864,947 bytes. Treat it as a trend guard rather than a prediction of the exact bytes CloudFormation receives: the test harness leaves asset hashes and account/region tokens symbolic.

Non-blocking findings are now tracked in #892. Every remaining nit from the review rounds — the Blueprint agent.modelId write that skips the validation repo onboard --model now applies, the two stale doctor comment blocks, the denied/absent shared remedy, the and-vs-either detail, the restated GEOS literal, the stale blueprint.test.ts rationale, the test_sdk_smoke.py exit-0-after-raise, inferenceProfileId's two unvalidated strings, and the (default us) doc row — is enumerated there with its file, why it matters, and what to do. #846 covers the geography-blind workflow admission. Nothing in this branch points at a stale issue number.

One test-only commit worth a sentence

b75e4fa2 touches no product code. The server suite's autouse reset fixture cleared the spawned-pipeline registry without joining it, so a pipeline thread could outlive the test that started it: the stubs were then undone and the thread went on to run a real pipeline — task-state writes, a heartbeat, a clone — inside whichever test ran next. It now joins instead, which is what stopped one pre-install seam-guard assertion from failing on CI while every local run passed.

Closes #747. Final step of the #740 stack; the plumbing landed in #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 #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 #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 #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 #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.

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.

…an unusable --model Closes #804. Closes #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. #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. #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.

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.

…ect 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.

isadeks · 2026-08-27T20:26:40Z

Status — all findings below are resolved as of e449c7e6. This was a self-review of a2550be5; the verdict line is kept for the record, not as the current state. Every blocking finding was reproduced before fixing and each fix is mutation-verified. What landed since, plus four further defects found while verifying, is summarized in the PR description. Earlier progress comments are collapsed as outdated.

Review — third round

Verdict: request changes. The central fix is right and the divergence it removes was real, but the failure class this PR exists to eliminate — "a model or geography that cannot work is accepted silently" — is still live in three places this change either created or left behind. Separately the headline fix has no test coverage on either substrate, and the shipped cdk.json value the thesis rests on is read by no test at all.

Two things worth saying first, because they're the strongest part of the change. The FINDING 1 fix in a2550be5 is the right shape — the divergence was removed rather than the literals re-synced. And the least-privilege risk checks out: diffing synthesized templates for us vs global gives 10 changed lines, all inference-profile/us.… → inference-profile/global.…; 41 Bedrock statements identical, 10 foundation-model/… ARNs byte-for-byte unchanged, no statement gained a wildcard (the single Resource: "*" is the pre-existing ModelInvocationLogging custom resource), and AgentSessionRole mirrors the runtime exactly. ADR-002 bootstrap coverage is correctly untouched — the only template additions are two CfnOutputs, no new CFN resource type; cdk/test/bootstrap passes 6 suites / 111 tests. Starlight mirror is in sync (sync-starlight.mjs exits 0, no tree change).

Blocking

B1 — `doctor`'s new check fails a healthy stack, and the correct idiom is 94 lines above it

cli/src/platform-doctor.ts:468 — message.includes('AccessDenied') never matches a real denial. Verified against live Bedrock with a deliberately under-privileged 15-minute STS federation token:

err.name    = AccessDeniedException
err.message = User: arn:…:federated-user/… is not authorized to perform:
              bedrock:GetInferenceProfile on resource: …/global.anthropic.claude-opus-5
message.includes('AccessDenied')                                  → false   ⇒ fail
/AccessDenied|Unauthorized|not authorized/i.test(`${name} ${msg}`) → true    ⇒ warn

The identifier lives only in err.name. bedrock:GetInferenceProfile is a new API action this PR introduces and it appears in no policy under cdk/bootstrap/policies/. So an operator on a least-privilege role gets fail → doctorChecksPassed (:565) false → commands/platform.ts:129 → exit 1, printing "tasks would fail at turn 0 with AccessDenied" for a perfectly healthy stack — while docs/abca-plugin/skills/deploy/SKILL.md:88 tells operators to confirm everything is green before proceeding.

Fix: reuse the idiom already at platform-doctor.ts:372-374, and apply it at :401 too (the pre-existing checkBedrockModel has the identical weak predicate). No test covers the branch — forcing it to always 'fail' survives 770/770.

B2 — the new Blueprint `REMOVE` deletes an operator's `--model` pin on every deploy

cdk/src/constructs/blueprint.ts:503-517. clearedOverrideFields pushes model_id whenever the Blueprint declares no agent.modelId, and onUpdate's parameters embed ':now': { S: new Date().toISOString() } computed at synth (:385) — so the resource properties differ on every synth and CloudFormation issues an Update on every cdk deploy. Synthesized proof:

"UpdateExpression":"SET #status = :active, #updated = :now REMOVE #mcp_servers, #cedar_policy_modules, #skills, #model_id"

cli/src/repo-onboard.ts:94-95 deliberately does the opposite (carries the value forward), and its comment says swallowing an error there "would silently wipe the existing blueprint overrides" — the Blueprint now does to the CLI exactly what the CLI refuses to do to the Blueprint. ADR-017 sanctions both writers on the same row, and this PR's own troubleshoot/SKILL.md:115 prescribes the CLI as the quick fix for a wrong model. Worst case is this very upgrade: a deployer who pinned us. per repo loses the pin and gets the default flipped to global. in one deploy.

Second half: the comment states a general principle while only 1 of 13 SET-only columns is handled. runtime_arn, github_token_secret_arn, system_prompt_overrides, egress_allowlist, cedar_policies, approval_gate_cap, build_command, lint_command, max_turns, max_budget_usd, poll_interval_ms, compute_type all still survive being dropped.

Fix: gate the clear on an explicit signal (modelId: null meaning "clear") or extend to every SET-only column and document Blueprint as authoritative-and-destructive; either way Annotations.addWarning when a field is being removed.

B3 — the third compute substrate still has the exact divergence this PR removes

cdk/src/constructs/lambda-microvm-compute.ts:436 declares imageEnvironmentVariables, consumed at :924 — and no caller ever sets it (agent.ts:912 passes only vpc, agentSessionRole, ...microvmImageInputs). lambda-microvm-strategy.ts injects no env either; it passes only the task payload. So on a lambda-microvm deploy the platform default model still comes from the Python literal, untouched by bedrockGeoRegion. With -c bedrockGeoRegion=us — the documented residency path — grants are us. while the agent asks for global.anthropic.claude-opus-5: every task with no per-repo override fails at turn 0 with AccessDenied.

That is FINDING 1 unfixed on substrate three, while stacks/agent.ts:495 and ecs-agent-cluster.ts:383 both say "BOTH substrates" and "makes that divergence unrepresentable". ADR-021 makes lambda-microvm a real deployable substrate (assertComputeSubstrateDeployed accepts it, doctor probes it).

Fix: set both vars there from inferenceProfileId(...). If parity is genuinely deferred (the construct comment says P2), file it and correct every "both substrates" claim to name the exception.

B4 — the headline fix is unguarded; so is the shipped geography

Mutations run on a clean tree:

Mutation	Result
Delete `ANTHROPIC_MODEL:` from `stacks/agent.ts:500`	SURVIVED — 234/234 pass
Delete both env vars from `ecs-agent-cluster.ts:389-391`	SURVIVED — 195/195 pass
Delete the `"context"` block from `cdk/cdk.json`	SURVIVED — 275/275 pass
Set `cdk.json` geo to `"eu"`	SURVIVED — 275/275 pass
Delete the `BedrockModelIds` `CfnOutput`	SURVIVED — 111/111 pass
Typo both new output names in `commands/repo.ts:182-183`	SURVIVED — 770/770 pass, eslint clean

cdk/test asserts ANTHROPIC_DEFAULT_HAIKU_MODEL in three places (stacks/agent.test.ts:183, 421, 483, including the describe.each(['global','eu','apac']) sweep) and ANTHROPIC_MODEL in zero. ecs-agent-cluster.test.ts isn't touched at all, though it already has a bedrockGeoRegion harness param (:37) and a geo sweep (:472) asserting only IAM ARNs — adding the env assertion is two lines.

Nothing anywhere reads cdk/cdk.json, so the suite exercises the code default us while every deployment ships global; stacks/agent.test.ts:96 even pins Value: 'us'. The repo onboard wiring is the sharpest one: typo both output names and the geography and grant checks silently become no-ops with CI green — contrast platform-doctor.ts, where the same typo fails 3 tests because the doctor tests stub on the literal output name.

B5 — the new `--model` guard has input holes

cli/src/model-id.ts:78, 99-100, verified by executing the real function:

Input	Result
`'global.'` (prefix, nothing after)	passes silently
`'global.us.anthropic.claude-opus-5'` (double prefix)	passes silently
`'global.anthropic.claude-opus-5 '` (trailing space)	passes silently
`deployedGeo: ''`	passes silently
`'Global.anthropic…'`, `' global.…'`	throws, misdiagnosed as "bare foundation-model id"

bare = modelId.slice(geo.length + 1) yields '', and the grant check is skipped when the granted set is absent or empty — which is the state of every stack deployed before this PR (backgroundagent-dev exports neither new output), so both new checks degrade to no-ops there and --model 'global.' is written straight into RepoTable.

Related, same path: an untrimmed bedrockModels entry (resolveBedrockModelIds validates trim().length but stores untrimmed) makes the exact-match grant check falsely reject a granted model, listing the model it just refused.

Fix: trim(), require a non-empty bare, case-normalize the prefix compare, and .map(s => s.trim()) on both the resolver and the split(',').

B6 — the canonical model-configuration reference contradicts this PR

These are false, not merely stale:

docs/guides/DEVELOPER_GUIDE.md:126 (mirror Model-configuration.md:12) — "A Python literal only — there is no CDK prop or environment knob in front of it today." This PR added that knob (agent.ts:500, ecs-agent-cluster.ts:389). Same claim at docs/design/REPO_ONBOARDING.md:123, a line this PR edited without fixing the parenthetical.
DEVELOPER_GUIDE.md:125 and :173 (mirrors :11, :59) — "default us" / "It defaults to us", while cdk.json ships global. A residency-constrained reader concludes no action is needed and gets global. This PR corrected exactly this wording in three code comments; the canonical doc kept it.
DEVELOPER_GUIDE.md:175 (mirror :61) — "One value drives … both grant sites and the layer-3 ANTHROPIC_DEFAULT_HAIKU_MODEL env var" omits ANTHROPIC_MODEL, the whole point of the change.
DEVELOPER_GUIDE.md:203 (mirror :89) — the cost row was relabelled us.anthropic.claude-opus-5 → global.anthropic.claude-opus-5 while keeping the same 37,584 tokens / $0.188020, in a table whose purpose is an apples-to-apples comparison against a us. row. That attributes a measurement to a profile it wasn't taken on. Revert the cell, or note both rows were measured on us..
cli/src/model-id.ts:62-64 — "Deliberately does NOT check membership in the granted model set: the CLI has no way to read bedrockModels today", 34 lines above the code that does exactly that.
docs/guides/DEPLOYMENT_GUIDE.md never mentions bedrockGeoRegion. For a default flip with compliance implications, add the cross-reference plus an explicit "set -c bedrockGeoRegion=<geo> before upgrading if you have a data-residency requirement". There's no CHANGELOG in the repo, so this is the only place it can land.

B7 — an `eu`/`apac`/`jp`/`au`/`us-gov` deployment has every model-pinning workflow rejected at admission

cdk/src/handlers/shared/workflows.ts:109-112 claims the allow-list "has to accept whatever geography a deployment is configured for" — it carries bare, us. and global. only, while resolveBedrockGeoRegion accepts all seven and -c bedrockGeoRegion=eu synths and deploys today. The new parity test can't see it: DEPLOYABLE_GEOS is hardcoded to ['us','global'] (workflows.test.ts:313) with a comment claiming the others are things "nobody can deploy against today", which isn't accurate.

Either generate the list from DEFAULT_BEDROCK_MODEL_IDS × BEDROCK_GEO_REGIONS — the two new tests now enforce it as an exact cross-product in both directions, so it is fully derivable — or state plainly in the comment which five geographies are uncovered. Note also that parity is enforced against the default grant only, so a -c bedrockModels addition is granted but not admitted.

Non-blocking

global.anthropic.claude-opus-4-20250514-v1:0 has no live inference profile — ResourceNotFoundException in both us-east-1 and us-west-2. The comment says "the grant covers them", but a grant on an ARN no profile backs isn't invocable. Its us. twin is equally dead, so this mirrors an existing hole rather than opening a class. Drop both, or add a contract test that resolves each allow-listed profile.
resolveBedrockModelIds accepts a wildcard. -c bedrockModels='["anthropic.claude-*"]' synthesizes inference-profile/global.anthropic.claude-* and foundation-model/anthropic.claude-*, silently converting the "no Resource: '*'" property into a wildcard grant. Pre-existing, but the new output now publishes it and the new exact-match guard falsely rejects on such a stack. Add a charset guard beside the existing geo-prefix rejection.
The Blueprint path — canonical per REPO_ONBOARDING.md:20 — has no model validation at all. blueprint.ts:97 registers validations for repo format, egress, gate cap, budget and three ref kinds, but nothing for agent.modelId. resolveBedrockGeoRegion/resolveBedrockModelIds are in the same package. The stated rationale ("fail where the operator can still fix it") argues for it more strongly at synth than in the CLI.
cli/src/commands/repo.ts:159 — --model <model-id> 'Foundation model ID override'. The new guard rejects exactly that. Reword to the inference-profile form with an example.
--model '' is a silent no-op that keeps the old override (model-id.ts:78 early-returns, repo-onboard.ts:94 falls through to existing). Reject it, or add a way to clear.
test_sdk_smoke.py still exits 0 on a real failure. ok = counts["assistant"] > 0 and counts["result"] > 0 counts messages, never ResultMessage.is_error/subtype — which runner.py:749 does read. A ThrottlingException mid-turn prints "PASS — Rules OUT the SDK/CLI/Bedrock path" and exits 0, alongside Errors: [...]. Same lie, one level down. Also system=1, assistant=1, result=0 (CLI killed after the assistant message) matches no branch: no verdict printed, bare exit 1. counts["other"] is never consulted.
BEDROCK_GEO_PREFIXES is a hand-copy whose only guard is tautological — model-id.test.ts:114 iterates the constant it is validating. When CDK adds a geography, geoPrefixOf returns undefined, a valid model is rejected as "a bare foundation-model id", and GEO_PREFIX_RE stops stripping: the original /^(us|eu|apac)\./ bug one layer up. Assert against Object.values(CrossRegionInferenceProfileRegion).
PLATFORM_REPO_DEFAULTS.model_id is a compile-time literal for a deploy-time choice. repo-display.ts:140 prints it as the effective model, so on a -c bedrockGeoRegion=us deploy repo show reports global.anthropic.claude-opus-5 while the stack injects us.. Now that BedrockGeoRegion is an output, derive the displayed prefix from it the way the substrate gate already does.
The default model is now hardcoded in five places across three languages (config.py, models.py, bedrock-models.ts, repo-display.ts, workflows.ts) plus the geography in cdk.json. contracts/constants.md says of itself: "single source of truth for numeric/textual constants that must agree across Python, TypeScript and tests. Hard-coding the same value in three places is how the APPROVAL_GATE_CAP triplication crept in; this file replaces that pattern." This PR added a third grep-based drift guard instead. Best follow-up; too big for this PR.
Comment accuracy, quick fixes: blueprint.ts:219 overstates the REMOVE as covering "the per-repo overrides"; agent.ts:930-945 orphans ComputeSubstrate's ADR-021 comment above BedrockModelIds and attaches the geography paragraph to the wrong output (platform-doctor never reads BedrockModelIds); model-id.ts:30 says "longest-first" for a list where apac follows us/eu; model-id.ts:32 describes a second geo list that no longer exists; workflows.ts:75 header still says "the us.-prefixed form"; workflows.ts:106 says the key "defaults to global" (the code default is us); bedrock-models.ts:92-96 says the default lives in code "rather than in cdk.json", which this PR falsifies; bedrock-models.ts:201 and :135 keep the old wording; runner.py:143 still says "(us. prefix)"; models.py:163 says a deployed task never reaches the default (true via build_config regardless of env, and false on lambda-microvm); test_sdk_smoke.py:5 and :132 still draw the threading conclusion the PASS branch just stopped drawing; docs/design/REPO_ONBOARDING.md:82 still describes the construct as writing via PutItem, the claim corrected in blueprint.ts.
Docs still advertising the old model set: onboard-repo/SKILL.md:80, 119, 147, 156 say the default is Sonnet 4.6 and that the stack wires only "Sonnet 4.6, Opus 4, Haiku 4.5" — and :121 cites Opus 4.8 as the ungranted example when it is granted; troubleshoot/SKILL.md:114 says the same three words above recommending Opus 5. Copy-pasteable us. Blueprint snippets remain at DEVELOPER_GUIDE.md:79, QUICK_START.mdx:548, Repository-preparation.md:51, agent/README.md:150, 190. The new skills guard scans only two SKILL.md files, and its CLAIMS_DEFAULT filter skips bash/JSON examples — extend it or fix the files.
blueprint.ts:509, 514 — props? optional with props ? … : [] restores the old buggy behaviour with no type error; one call site each, make it required. cli/README.md:191 doesn't list the new doctor check. DEVELOPER_GUIDE.md:127 cites config.py:569 for the haiku fallback, now at :573 (shifted by this PR's own added comment). getStackOutput issues one DescribeStacks per output — repo onboard now makes 6 and doctor 8; listStackOutputs already returns them all in one call.

Tests

Baselines on a clean tree: CDK 275/275 (6 suites), CLI 770/770 (57 suites), agent 1583. cdk/test/bootstrap 6 suites / 111 tests / 1 snapshot. No new CDK test re-enables bundling or re-synths per test.

Genuinely mutation-resistant: the Blueprint REMOVE (both directions), the allow-list parity (both directions), the skills-geography guard, the models.py↔config.py parity, and repo-display's pre-existing tie to config.py. Unguarded: everything in B4, the doctor warn branch, and the newly exported inferenceProfileId/PLATFORM_DEFAULT_* (an arg-order swap in inferenceProfileId fails 5 tests — all via the aux var only).

Process

#747 is assigned to @scottschreckengaust rather than the implementer, and none of #747/#804/#805 carries the "Starting implementation." comment ADR-003 requires. Please reassign or leave a coordinating comment on #747 before merge. Minor: the branch names 804/805 but not #747, the behaviour-change issue, and the PR has no area/type/priority labels.

…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.

…o lambda-microvm `main` landed the Lambda MicroVMs P2 work (#733), which independently fixed the auxiliary model's geography — a `haikuInferenceProfileId` helper plus delivery through the orchestrator's `agentPlatformConfig`. That overlapped this branch's `bedrock-models` helper and the runtime env block. Resolved toward main's shape rather than mine where it was better sourced: its helper interpolates the model id from the same constant the grant list uses, and its `platform_config` path already carries a model to the MicroVM guest. This branch keeps what main does not have — the MAIN model, on every substrate. main's own comment on `haikuInferenceProfileId` records that `config.py`'s main-model default "is correct on the default geography and a pre-existing gap on any other … left alone here rather than fixed in a conflict resolution". That gap is what this branch closes, so the two fit together rather than duplicating. Concretely: dropped this branch's `imageEnvironmentVariables` block on the MicroVM construct in favour of extending `agentPlatformConfig` with `anthropicModel`, so both models reach the guest by the one mechanism main established for the auxiliary one. The prop is required rather than optional, so a future substrate wired without it fails to compile instead of silently reading the Python literal — which is how this class of bug kept recurring. Verified per substrate, not assumed: synthesizing lambda-microvm at `-c bedrockGeoRegion=us` gives 15 `us.` grants, 0 `global.`, and both env vars `us.` — grants and agent agree. The same check at `global` agrees on `global.`. Suites after the merge: 4280 cdk, 789 cli, 1583 agent.

…ants 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.

…k 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.

ayushtr-aws

Verdict: Approve (with nits)

All three blockers from the prior CHANGES_REQUESTED (@scottschreckengaust) are verified fixed in the current head c2d6dc0d, no new blocker survives an independent re-review, CI is green, the three backing issues (#747/#804/#805) are approved, and the change is live-verified by artifact rather than exit code. Only comment-accuracy nits and an acknowledged Phase-4 gap remain.

Reviewed as a principal architect. The pr-review-toolkit sub-agents are not registered as invokable agents in this environment, so I applied each scope by hand (see Review agents run) and applied /security-review-style scrutiny to the IAM/Cedar/context-input surfaces myself.

Vision alignment

Fits the north star (VISION.md — bounded blast radius and reviewable outcomes). It closes a fire-and-forget failure mode: a non-default bedrockGeoRegion granted one geography's inference profiles while the agent invoked another's, dying at turn 0 with AccessDenied and naming no model. Both ANTHROPIC_MODEL and ANTHROPIC_DEFAULT_HAIKU_MODEL are now derived from one resolved geography across all three substrates; BedrockGeoRegion/BedrockModelIds become inspectable CfnOutput contracts; repo onboard --model and platform doctor move failures left, out of the unattended path. No tenet is traded — IAM stays per-model ARN-scoped, and the new wildcard guard tightens it. The default flip to global is the approved intent of #747 and carries data-residency implications, which the change handles responsibly (see Documentation).

Verification of prior blocking claims (re-checked against the worktree)

Prior B1 — workflows.ts pulled all of aws-cdk-lib into ~7 runtime Lambdas — FIXED. The two constants now live in the dependency-free cdk/src/handlers/shared/bedrock-model-constants.ts; workflows.ts:36 imports from there (not from constructs/), and bedrock-models.ts re-exports them so construct-layer importers are unchanged. BEDROCK_GEO_REGIONS is a hand-written literal on the runtime side, with bedrock-models.test.ts asserting it equals Object.values(CrossRegionInferenceProfileRegion) (set + length) so it cannot silently drift. A real esbuild bundle test (runtime-bundle-boundary.test.ts) asserts 0 aws-cdk-lib refs and <1 MB on both entries, plus a static backstop that the constants module imports nothing. This is a stronger fix than the reviewer proposed.

Prior B2 — doctor's binary error classifier steered transient failures to the destructive "remove the model" remedy — FIXED. checkGrantedModelProfiles now three-way classifies into missing (fail + remove remedy) / denied (warn) / unverified (warn, "do NOT remove anything"), matching on err.name AND message; checkBedrockInferenceProfile uses the same accessDenied || !definitivelyAbsent ? 'warn' : 'fail' split. The pre-existing catalog check (checkBedrockModel) was corrected the same way (name+message, the checkGithubToken idiom). Well covered by the it.each([Throttling, 5xx, Timeout, ExpiredToken]) tests.

Prior B3 — falsified comment at bedrock-models.ts:210-214 — FIXED. The haikuInferenceProfileId function and its stale doc block (which claimed ECS "never carried ANTHROPIC_DEFAULT_HAIKU_MODEL") are deleted; ecs-agent-cluster.ts now injects both geo-derived model vars into both task definitions, with a per-geography test. (One dangling {@link} residue remains — nit #1 below.)

Prior nits were also addressed: the blueprint.ts class-doc was reworded; model-id.ts states ordering is irrelevant rather than mis-claiming "longest-first"; test_sdk_smoke.py gained an explicit INDETERMINATE branch and a matching exit code; PLATFORM_DEFAULT_AUX_MODEL_ID is now an alias of DEFAULT_HAIKU_MODEL_ID; a producer-side hasOutput('BedrockModelIds') test was added; and the double-prefix / empty-bare / .trim() branches are now tested.

Blocking issues

None. (0 verified blockers.)

Non-blocking suggestions / nits

cdk/src/constructs/bedrock-models.ts:108 — dangling JSDoc {@link}. The resolveBedrockGeoRegion doc still says the aux env var "takes the same prefix via {@link haikuInferenceProfileId}", but that function was deleted in this very PR. Repoint it to inferenceProfileId (or drop the @link). Same doc-accuracy class as the blocker you just fixed.
cdk/src/constructs/bedrock-models.ts:134 — JSDoc shows a command form this PR proved cannot work. The resolveBedrockModelIds doc offers \-c bedrockModels='[…]'`as a way to set the list, but commitc941454(and the corrected DEVELOPER_GUIDE) established that-c` supplies a string and the array form is rejected at synth. The operator docs were fixed; this in-code comment still advertises the broken form.
cdk/src/handlers/shared/workflows.ts:89 — admission accepts all 7 geo prefixes regardless of the deployed geography. A workflow pinning e.g. jp.anthropic.claude-opus-5 on a global deploy passes admission and then fails at turn 0 with AccessDenied — the same failure class this PR otherwise pre-empts on the --model path. Acknowledged as "Phase 4"; worth scoping to the resolved geography when this list becomes context-sourced. (Inline comment left.)
Untracked follow-ups. The PR body itself notes that #820/#822/#823 were filed-then-closed and the underlying findings (Blueprint field-removal no-op, the platform-default model hand-copied across three languages, geo-admission scoping) are unaddressed and not referenced by any issue. Per Stage-4 issue-tracking, please file these as P2/P3 follow-ups so the deferred work has a home and the code's "tracked separately" pointers are not dangling.

Documentation

Strong and in sync. New "Bedrock inference geography" sections in DEPLOYMENT_GUIDE.md and DEVELOPER_GUIDE.md with an explicit, prominent data-residency warning ("set the geography before upgrading") — the right mitigation for silently flipping the shipped default to global. Every hand-authored docs/guides/ + docs/design/ edit has its regenerated Starlight mirror under docs/src/content/docs/ (Architecture, Interactive-agents, Repo-onboarding, Per-repo-overrides, Installation, Model-configuration, Deployment-guide, Quick-start), so the "Fail build on mutation" step is satisfied. contracts/constants.json correctly adds anthropic_model → ANTHROPIC_MODEL and correctly leaves it out of required (optional, generically consumed by the MicroVM transport). The two residual doc defects are the in-code comment nits above.

Tests & CI

Excellent coverage, and it asserts the specific historical bug each test guards: per-geography env-injection sweeps across AgentCore + both ECS task defs, err.name+message AccessDenied classification, the three-way transient/denied/missing split (mutation-described), wildcard/pattern rejection with the account-wide-grant rationale, ungranted-model and doubled-prefix and empty-bare and .trim() rejection, empty-but-truthy grant-list guard, allowlist⇄grant-list parity in both directions, the real-esbuild bundle boundary, and a CloudFormation 1 MB template budget measured the way the CLI serializes (indented, not compact). Suites reported: 4287 CDK / 797 CLI / 1755 agent; mise run build clean. All 8 CI checks green.

Bootstrap synth-coverage: N/A (ADR-002). No new CloudFormation resource types — only CfnOutputs and env/IAM-scope changes on existing resources. GetInferenceProfile/GetFoundationModel run under operator CLI creds, not the CFN execution role, so no cdk/src/bootstrap/ update is required.

Base drift note (non-blocking): this head predates the current origin/main tip. The diff touches shared/fast-moving files (contracts/constants.json, cdk.json context, shared handler types via the new constants module); confirm a clean merge before landing.

Review agents run

The pr-review-toolkit agents are not invokable in this environment; I applied each scope manually.

code-reviewer (manual) — re-verified the bundle-boundary fix, the re-export direction, and CDK L2/output/IAM-scope quality. Clean; nits #1–#3.
silent-failure-hunter (manual) — audited the doctor error classifiers and the test_sdk_smoke.py exit-code/verdict paths. Both fail-closed correctly now; no silent-fallback survivors.
pr-test-analyzer (manual) — coverage is thorough and asserts intent, not just behavior; the prior gaps (producer-side output, double/empty prefix, trim) are closed.
comment-analyzer (manual) — found nits #1 and #2 (dangling @link; broken -c example in JSDoc).
type-design-analyzer (manual) — light: no new exported public type/interface/union of note; assertModelIdUsable arg object and BEDROCK_GEO_PREFIXES const are appropriately shaped.
/security-review (manual) — the wildcard/pattern guard in resolveBedrockModelIds is a genuine hardening (blocks bedrockModels:['*'] → inference-profile/<geo>.* account-wide grant via a context value); model-id parsing is pure string-prefix logic; DynamoDB UpdateExpressions are code-derived with attribute-name maps (no injection surface); IAM stays per-ARN scoped. No exploitable finding.

Human heuristics

Proportionality — Pass. model-id.ts and bedrock-model-constants.ts are focused, single-purpose modules; the bundle-boundary test earns its multi-second cost by pinning a 57 MB regression.
Coherence — Pass. The earlier duplicate-haiku-path smell is resolved (one inferenceProfileId helper; aux id is an alias). The geo/model concept is now spelled once and shared.
Clarity — Concern (minor). Two JSDoc comments in bedrock-models.ts (:108, :134) lag the code this PR shipped — nits #1/#2. Names communicate intent well otherwise.
Appropriateness — Pass. Integration behavior (AccessDenied via err.name, absent-profile via ResourceNotFound) was validated against the real API and against a deliberately deny-scoped role, not only self-written mocks; tests assert what the code should do.

Bottom line: the prior blockers are genuinely resolved with tests that would catch their regression, the design is sound and vision-aligned, and the residual items are comment-accuracy nits plus follow-up tracking. Approving.

scottschreckengaust

Verdict: Request changes

Re-review at c2d6dc0d. All three blockers from my 7d86c114 review are verified fixed, and none of them is re-raised — the fixes are better than what I asked for in two of the three cases. Two new blockers remain, both surgical, and both of the same class as what was already fixed: a user-facing doc that hands out a value the shipped default cannot invoke, and a new guard that measures a different substrate than the one its own rationale is about.

Method disclosure — read this before weighing anything below. I ran no test suite: no mise run build, no jest/pytest/tsc, no cdk synth. My run constraints forbade it, and I made no worktree, branch, or git config mutation. Every claim below is either (a) read directly from the tree at c2d6dc0d, (b) read-only git, (c) a live, read-only Bedrock API call from a sandbox, or (d) a docs-sync reproduction in an out-of-tree copy of the repo. Where I reason about what a test would assert, that is static reasoning and is labelled.

Prior review: resolved vs still open

RESOLVED — verified at c2d6dc0d. Not re-raised.

Prior blocker 1 — aws-cdk-lib pulled into 7 runtime Lambda bundles. Fixed as suggested and then some. cdk/src/handlers/shared/bedrock-model-constants.ts is a new dependency-free module (DEFAULT_BEDROCK_MODEL_IDS, BEDROCK_GEO_REGIONS, inferenceProfileId, the platform default ids); workflows.ts imports from it; and bedrock-models.ts:78 keeps the Object.values(CrossRegionInferenceProfileRegion) read private and un-exported, so the alpha enum never leaves the construct layer. The drift guard I asked for is there (cdk/test/constructs/bedrock-models.test.ts asserts set and length parity against the enum — I confirmed independently that the alpha enum has exactly the 7 members the literal lists). Better: cdk/test/handlers/shared/runtime-bundle-boundary.test.ts actually esbuild-bundles the handler entry and asserts aws-cdk-lib refs === 0 and bundle bytes < 1_000_000 — the missing gate that let this land in the first place. Static reasoning on the assertions themselves; I did not execute them.
Prior blocker 2 — binary error classifier routing transient failures to destructive advice. Fixed properly. checkBedrockInferenceProfile (cli/src/platform-doctor.ts:494-496) and checkGrantedModelProfiles (:563-593) now bucket three ways — denied → warn, definitively absent (ResourceNotFound|ValidationException|…) → fail with the remove/redeploy remedy, everything else (throttle, 5xx, DNS, expired creds) → unverified → warn. The comment at :563-569 states the reason the remedy may only be given for proof, which is exactly the right invariant to write down. cli/test/platform-doctor.test.ts adds a transient-error matrix. One un-migrated call site remains → nit N4.
Prior blocker 3 — falsified haikuInferenceProfileId doc block. Fixed by deleting the function and its comment; both models are now injected from one resolved geography via inferenceProfileId(geo, …) on all three substrates. One residue → nit N7 (dangling {@link}).

Prior nits: blueprint class doc reworded (exactly the suggested wording, blueprint.ts:215-225) ✅ · model-id.ts "longest-first" corrected ✅ but the falsified claim survived into the new test → N2 · smoke-test indeterminate branch ✅ · aux-model alias + single helper ✅ · geo-scoped workflow admission ❌ still open (still a nit, N6) · issue tracking ❌ still open (N9).

Independently verified live (read-only Bedrock reads, my own sandbox): all four models this PR keeps in the grant set have ACTIVE inference profiles under both global. and us., and anthropic.claude-opus-4-20250514-v1:0 has none in either geography. So removing it is correct, the global flip is invocable, and the new "every granted model resolves" check should pass on a global deploy. The PR's central factual claims hold up.

Vision alignment

Fits the vision (VISION.md — bounded blast radius, reviewable outcomes, fire-and-forget default). The through-line of this PR is moving a class of failure out of the unattended path: one resolved geography drives every grant and both injected model env vars; agentPlatformConfig.anthropicModel is required so a substrate wired without it fails to compile; BedrockGeoRegion/BedrockModelIds become documented CfnOutput contracts instead of something a consumer regexes out of a template; and --model is rejected before it reaches the RepoTable. No tenet traded — IAM stays scoped to explicit foundation-model/<id> and inference-profile/<geo>.<id> ARNs, and the new bedrockModels wildcard rejection tightens it. Model config correctly stays in CDK context rather than CFN parameters, because grantInvoke resolves at synth (#740/#741).

Blocker 1 is where the PR does not yet finish that job: three write paths reach the same model_id field, and only the CLI one is gated.

Blocking issues

1. The guide still prescribes a us. Blueprint pin that the shipped global default cannot invoke — docs/guides/DEVELOPER_GUIDE.md:219 (and its generated mirror docs/src/content/docs/developer-guide/Model-configuration.md:105).

The line reads:

Per repo: Blueprint agent.modelId (e.g. us.anthropic.claude-sonnet-4-6) — no code change, no agent redeploy

This PR systematically scrubbed us. → global./<geo>. from the model-configuration section of that same file (including layer table row 4, which now correctly says <geo>.anthropic.…), but this second prescriptive site in the cost/budget section was missed. After this PR, following it on a default deploy is a broken configuration, and nothing catches it:

cdk/src/constructs/blueprint.ts:319-320 writes props.agent.modelId into RepoTable.model_id verbatim — no geography check, no grant-set check, even though the stack has both values at synth (it computes the grant ARNs from them). Note the asymmetry inside the same construct: agent.maxBudgetUsd is validated at synth (blueprint.ts:298, MaxBudgetUsdValidation at :613-617), and the paragraph three lines above the stale example advertises exactly that ("validated at CDK synth so an out-of-range value cannot deploy").
cdk/src/handlers/shared/strategies/ecs-strategy.ts:217 passes the stored value straight through as ANTHROPIC_MODEL, and nothing agent-side rewrites a prefix (agent/src/config.py, models.py — checked).
bgagent platform doctor stays green: platform-doctor.ts:118 already loads the active repo rows, and :121-122 already know the geography and the granted set, but no check compares a stored model_id against them.
cli/src/model-id.ts's assertModelIdUsable encodes precisely the needed rule and is wired only into cli/src/commands/repo.ts.

Failure chain: operator reads DEVELOPER_GUIDE.md:219 → sets agent.modelId: 'us.anthropic.claude-sonnet-4-6' → deploys with the shipped bedrockGeoRegion: "global" → RepoTable carries us.…, IAM grants only global.… → every task for that repo dies at turn 0 with AccessDenied naming no model, while platform doctor reports healthy. That is the exact defect #804/#805 were filed to eliminate, re-entered through the docs.

The same gap strands existing deployments: a repo already pinned us.… (the pre-PR default geography, and the CLI value the old docs handed out) is silently stranded by the flip. DEPLOYMENT_GUIDE.md:80 tells data-residency users to choose a geography before upgrading but says nothing about existing per-repo pins.

Minimum fix (blocking): geo-correct the example — either match the shipped default or use a <geo>. placeholder — regenerate the mirror with mise //docs:sync, and add one sentence to the new "Bedrock inference geography" section: an existing repo or Blueprint pinned to another geography must be re-pinned or cleared, because the flip revokes its grant.
Strongly recommended (same PR or a filed follow-up): (a) extend the geography sweep in cdk/test/contracts/model-default-docs-parity.test.ts — its operator skills do not advertise a different geography test is exactly the right guard — to prescriptive override examples in the guides, not only SKILL.md; its CLAIMS_DEFAULT scoping deliberately exempts "per-repo override snippet", which is what let :219 through; (b) validate agent.modelId at synth the way maxBudgetUsd already is; (c) have doctor run the stored pins through the same rule it now applies to --model.

2. cdk/test/stacks/agent.test.ts:48-84 — the new template-size budget measures the substrate with headroom, not the one its own rationale is about.

The comment is explicit about why the guard exists: "This is not hypothetical headroom. compute_type=ecs — the substrate deployments actually use — synthesizes at 992,111 bytes against the 1,000,000 ceiling. That is 7,889 bytes, roughly one medium construct." But the template this test asserts on comes from the beforeAll at :37-41, which constructs AgentStack with no compute_type context — i.e. the default agentcore substrate (agent.ts:304). ECS at 992,111 bytes would fail the 950,000-byte budget outright, so by construction the near-limit template is not the one being measured.

Risk: a change that pushes the ECS template past 1 MB merges with CI fully green and then fails at changeset creation — after synth, after assets are pushed, naming no resource, with stack status still showing the previous deploy's success. That is verbatim the failure this test says it converts into a build failure. A guard that documents one risk and measures another is worse than no guard, because the next contributor trusts it. (Static reasoning: I did not run the suite; the numeric relationship 992,111 > 950,000 is from the PR's own measurement.)

Fix (cheap — no extra synth): the ECS describe at :1047-1058 already has a full Template in scope. Add the byte assertion there against the real ceiling (< 1_000_000), and say plainly in the comment that ECS is over the 5% budget today and that the budget applies to agentcore. If you would rather not encode a 0.8%-margin assertion, then at minimum correct the comment so it stops claiming coverage it does not have, and file the ECS headroom as an issue.

Non-blocking suggestions / nits

N1 — docs/abca-plugin/skills/troubleshoot/SKILL.md:114 (line edited by this PR): the 403 remedy still says the granted set is "(Sonnet 4.6, Opus 4, Opus 4.8, Opus 5, Haiku 4.5 by default)", while this PR removes anthropic.claude-opus-4-20250514-v1:0 from DEFAULT_BEDROCK_MODEL_IDS. After this PR every gate — the workflow allowlist, assertModelIdUsable, the IAM grant — rejects Opus 4, so the troubleshooting doc names a model as available that the platform now refuses. Removing it was right (live check: no profile in any geography); the doc just needs to follow. Same at :145/:156 and in the DEPLOYMENT_GUIDE.md:139 cost table.
N2 — cli/test/model-id.test.ts:161: "Asserted because the list is written longest-first" is the claim you correctly removed from cli/src/model-id.ts:28-32 ("The order below is arbitrary (it is NOT sorted by length: apac follows us)"). The falsified version migrated into the test. Drop the clause; the behavioural assertion is the valuable part.
N3 — cdk/src/stacks/agent.ts:980-1009: the two new comment paragraphs are ordered inversely to the outputs they describe — "Surfaced so a client can check the profile the deployment will actually invoke" (about BedrockGeoRegion, emitted at :1002) sits above "The granted model set…" (about BedrockModelIds, emitted at :996) — and the pre-existing ComputeSubstrate paragraph at :980-986 now dangles ~30 lines above its own output. Move each paragraph onto its CfnOutput.
N4 — cli/src/platform-doctor.ts:390-418: checkBedrockModel — touched by this PR (its argument became the geo-stripped DEFAULT_BEDROCK_MODEL_ID) — is the one model check that kept the binary classifier: :412 accessDenied ? 'warn' : 'fail', with the remedy "Enable model access in the Bedrock console". A throttle or 5xx on the catalog probe still fails doctor and points the operator at the wrong console. It is the same one-line pattern you already applied twice in this file. (checkLambdaMicrovmAvailability:381 has the same shape but is untouched here — out of scope.)
N5 — cdk/src/constructs/blueprint.ts:517: clearedOverrideFields(_props) returns [] unconditionally, and buildRemoveClause/buildRemoveNames thread an optional props? purely to reach it. The paired test therefore passes vacuously (AI005). The reasoning comment above it is genuinely good; the inert scaffolding is not — either implement the warn-on-clear behaviour the comment describes or drop the parameter until you do, and file the gap (the PR body says it is untracked).
N6 — cdk/src/handlers/shared/workflows.ts:85-87 (repeat, still non-blocking): the allowlist is now bare × all 7 geos, so admission accepts six geographies no deploy grants. main's comment deliberately withheld global. for exactly this reason ("admitting it here first would pass admission and then fail at turn 0 with AccessDenied"), and the new parity test enshrines the wide form. This does not widen IAM — it degrades error quality, moving a clear create-task rejection to a turn-0 AccessDenied, including for user workflows that pinned us.… and worked before the flip. The stack already injects geography-derived values into compute; injecting BEDROCK_GEO_REGION into the create-task Lambda would let admission narrow to the deployed geography. If it stays deferred, please file it and keep a note in the code where main's warning used to be.
N7 — cdk/src/constructs/bedrock-models.ts:108: {@link haikuInferenceProfileId} now points at a symbol this PR deleted — it is the only remaining reference in the repo. Retarget to inferenceProfileId.
N8 — cli/src/stack-outputs.ts: getStackOutput runs its own DescribeStacks with no caching, so the two new lookups take platform doctor to 8 DescribeStacks calls for one stack (and repo onboard to 6), fired as a Promise.all burst against a throttled API — where a throttle surfaces as a hard CliError. listStackOutputs(region, stackName) already exists and returns the whole map in one call. Also minor: checkGrantedModelProfiles probes models sequentially.
N9 — issue tracking (Stage 4): PR body ¶44 states three known findings are unaddressed and deliberately untracked after #820/#822/#823 were filed and closed. Please file them (Blueprint field-removal no-op, the hand-copied default across three languages, geo-scoped admission) so the code's "tracked separately" pointers are not dangling.
N10 — cli/src/repo-display.ts: model_id: 'global.anthropic.claude-opus-5' hardcodes the geography in the displayed defaults; on a non-global deploy the CLI will display a value the same CLI would reject. Derive it from the BedrockGeoRegion output you now expose.

Documentation

Mirror sync: IN SYNC — verified by reproduction. I copied the tree out-of-repo (excluding .git/node_modules/.venv), ran node scripts/sync-starlight.mjs there, and diff -rq against the committed docs/src/content/docs/ came back clean. No hand-edited mirror files. The blocker-1 doc fix must be followed by mise //docs:sync (the stale line exists in both source and mirror).
Good additions: the new "Bedrock inference geography" section in DEPLOYMENT_GUIDE.md with data-residency guidance; the rewritten 5-layer model table and "Choosing the inference-profile geography" in DEVELOPER_GUIDE.md (the layer-1-is-bare / everything-else-is-prefixed asymmetry is now stated with the reason); agent/README.md env table corrected to a geo-prefixed profile id.
Missing: the upgrade sentence for existing per-repo/Blueprint pins (blocker 1), and the Opus 4 references in troubleshoot/SKILL.md and the DEPLOYMENT_GUIDE.md cost table (N1).
contracts/constants.json adds anthropic_model → ANTHROPIC_MODEL to the platform_config map without adding it to required — correct for a backward-compatible transport addition, so an older agent image does not hard-fail on a newer control plane (static reasoning from the contract file and task-orchestrator.ts).

Tests & CI

CI: green on c2d6dc0d at review time (all required checks, including the merge-queue-sensitive security scans).
Bootstrap policy coverage: not applicable, verified. I walked the diff for new CloudFormation resource types: there are none — the changes are CfnOutputs, env vars, and statements on existing roles/policies. No cdk/src/bootstrap/** change, resource-action-map.ts entry, BOOTSTRAP_VERSION bump, or golden-baseline update is required, and none was made. Correct.
Test-performance (#366): clean. New/changed suites synthesize once in beforeAll, nothing re-enables aws:cdk:bundling-stacks, and the esbuild bundle-boundary test runs out-of-band with an explicit 60 s timeout rather than through CDK bundling.
Coverage is strong and mostly asserts the right invariants: allowlist ↔ grant-list parity in both directions across all geographies; enum ↔ literal parity by set and length; cdk.json pinned to global; per-geo env injection on the AgentCore runtime and both ECS task definitions; the exact Bedrock ARN set; the transient-error matrix in doctor; models.py ↔ config.py parity plus a geography sweep of the operator skills; and cli/test/model-id.test.ts covering the empty-<geo>., doubled-prefix, us-gov, and whitespace cases.
Gaps: blocker 2 (budget measures the wrong substrate); clearedOverrideFields's vacuous test (N5); and nothing asserts that a stored model_id from another geography is diagnosed — the blocker-1 failure has no test that would catch it.
The PR's mutation-verification of the MicroVM emit (revert the emit / emit the aux model under the main name → suite fails) is the right way to substantiate a synth-only change, and the disclosure that lambda-microvm cannot be deployed yet is the kind of honesty that makes the rest of the evidence credible.

Review agents run

Nested agent dispatch was unavailable in this environment, so I could not invoke the pr-review-toolkit agents as separate agents, and I am not claiming to have run them. I applied each rubric dimension inline over the diff myself:

code-reviewer — applied inline (routing per AGENTS.md, CDK/L2 quality, ARN scoping, contract/type sync).
silent-failure-hunter — applied inline; produced the verification of prior blocker 2 and N4, plus the smoke-test exit path.
comment-analyzer — applied inline; produced N2, N3, N7 and blocker 2's falsified rationale.
pr-test-analyzer — applied inline; produced blocker 2 and N5, and the coverage assessment above.
type-design-analyzer — applied inline (last round I omitted it because the diff added no exported type; this round it does): cli/src/model-id.ts exports a const tuple BEDROCK_GEO_PREFIXES, geoPrefixOf, and assertModelIdUsable; bedrock-model-constants.ts exports readonly string[] literals. Both are narrow, literal-typed, dependency-free, and layered so the CLI takes no CDK dependency. No concern; the only observation is that assertModelIdUsable's four-string options object relies on the caller to supply a trustworthy deployedGeo, and it correctly treats blank as unknown rather than as a mismatch.
/security-review — I invoked the Skill. Its harness derived the diff from the repository cwd (on main) rather than the PR worktree, so its automated scope came back empty, and its methodology requires parallel sub-tasks that are unavailable here; I applied it inline against the PR's security surface instead. No HIGH or MEDIUM finding. Specifically: the new bedrockModels */? rejection (bedrock-models.ts:196-203) closes the wildcard-widening vector completely — IAM recognizes no other wildcard metacharacter — and it sits alongside the geo-prefix rejection that prevents a us.us.… ARN; IAM remains scoped to explicit foundation-model/<id> + inference-profile/<geo>.<id> ARNs with no Resource: "*" introduced; resolveBedrockGeoRegion fails closed at synth on an unknown geography; and the widened workflow allowlist admits ids it does not grant, which is an error-quality regression (N6), not a privilege one. Deploy-time context values are operator-supplied and trusted, so the wildcard rejection is defense-in-depth against a foot-gun rather than a fix for an external attack path.

Human heuristics

Proportionality — pass. The new abstractions are each load-bearing: the constants module exists to enforce a layering boundary, cli/src/model-id.ts exists because the CLI cannot depend on CDK, and the wildcard/prefix rejections are guards, not machinery. Nothing invented for a one-off. Counter-example: N5's clearedOverrideFields scaffolding is complexity with no behavior behind it.
Coherence — concern. "Same concept, same term" mostly holds — one geography value flows to every grant and both env vars, and the geography lists in cdk/, cli/, and the doctor regex are all derived from one place per package. It breaks at the documentation edge: docs/guides/DEVELOPER_GUIDE.md:219 and troubleshoot/SKILL.md:114 still describe the pre-PR world (blocker 1, N1), and validation is applied on one of the three write paths to model_id.
Clarity — pass. Error messages name the key, the offending value, and the fix (resolveBedrockModelIds, assertModelIdUsable); the doctor's three-way comment states the invariant rather than the mechanics. Deductions: N3 (comments detached from what they describe) and N7 ({@link} to a deleted symbol).
Appropriateness — mixed. Strong on AI001: the integration behavior was verified against the real API, not mocks — the PR's live evidence and my own independent read-only Bedrock reads agree (four granted models resolve in global. and us.; the removed Opus 4 resolves in neither). Concern on AI005: two tests assert what the code does rather than what it should do — the template budget measures the substrate that fits (blocker 2), and the field-clearing test passes because the function returns [] (N5).

Everything blocking here is a small edit — one doc line plus a mirror regeneration, one sentence in the upgrade section, and one assertion moved into an existing describe. The fixes to my three prior blockers are exactly right, and the runtime-bundle-boundary test in particular is a durable improvement beyond what I asked for. Happy to re-review promptly.

…ys, 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: #846 (geography-scoped workflow admission — needs the resolved geography plumbed into a runtime handler) and #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.

isadeks · 2026-09-02T15:32:21Z

Both blockers were valid. Addressed in e6464b0b, along with the fix-now list — net 95 insertions / 207 deletions, since most of it removes or corrects prose rather than adding it.

Blocker 1 — the guide recommended a model the default deploy cannot invoke

Confirmed: DEVELOPER_GUIDE.md:219 gave us.anthropic.claude-sonnet-4-6 as the per-repo Blueprint example while cdk.json ships bedrockGeoRegion: global. Following it produced the exact 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 in the first place. Also removed the neighbouring instruction in QUICK_START.mdx to hand-edit grantInvoke in agent.ts; that list is bedrockModels context now.

Worth noting: the docs-parity guard added earlier in this PR caught my first attempt at this fix — an illustrative id on the same line as the word "default" reads as a default claim. Guard working as intended.

Blocker 2 — the size test measured the wrong substrate

Confirmed: it ran in the default describe, whose new App() passes no compute_type, so it synthesized AgentCore. Moved into the existing ECS describe and reused its already-synthesized template, so no extra synth as you noted:

substrate	pretty bytes
default (AgentCore)	858,062
`compute_type=ecs`	894,261

ECS is both the substrate deployments use and the one nearer the ceiling. Verified the assertion still fires by tightening the budget.

Fix-now list

Doctor's remaining transient misclassification. checkLambdaMicrovmAvailability and checkBedrockModel still used the binary form. All four checks now share one classifyProbeFailure (denied / absent / unverified), which removes the third and fourth copies of that regex rather than adding them; absent is the only definite negative, so the only one that fails. Mutation-verified both directions. One existing test asserted the old behaviour for a rejection of the literal string service unavailable — updated, since a transient error must not carry a "check the Region" remedy.
Stale Opus 4 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 rather than restating it.
False "longest-first" comment. Still present in model-id.test.ts (apac follows us). The source comment was corrected earlier; the test's was missed.
Misplaced output comments. Two blocks sat above BedrockModelIds, the first describing BedrockGeoRegion, which had none. Both deleted — the description fields already say what each output is — leaving one line naming the consumers.
Inert clearedOverrideFields. Removed, along with the props parameter both callers no longer needed. The reasoning it carried is one sentence on the class doc.

Dangling link — I could not find one this PR introduced. Every relative link and anchor it adds resolves (#model-configuration exists; the Starlight site-absolute routes are real pages). What I did find, in a file this PR touches, are two genuinely broken anchors that predate it: DEVELOPER_GUIDE.md#repository-onboarding and PROMPT_GUIDE.md#repo-level-customization. Retargeted to #repository-preparation and #repo-level-instructions. If you meant a different one, point me at it and I will fix that too.

Deferred with tracked issues

feat(orchestration): scope workflow model admission to the deployed geography #846 — geography-scoped workflow admission. The allowlist is a module-level constant in a runtime handler, so the deployed geography is not available where it is built; closing it needs that plumbed in (env var, or the context-sourced list the file's own comment anticipates). Currently unreachable in practice — no shipped workflow pins a model.
feat(cli): read stack outputs in one call and derive repo show's default geography #847 — one DescribeStacks instead of nine, and repo show deriving its default geography from BedrockGeoRegion rather than the compile-time global. literal.

Both bodies were written from re-read code and every file:line citation verified against the branch after posting.

Suites: 4364 CDK, 830 CLI, 1775 agent.

scottschreckengaust

Verdict: Request changes

Re-review at e6464b0b against my prior review at c2d6dc0d. Both blockers from c2d6dc0d are verified fixed, and four of the seven nits are fixed — the doc that prescribed a us. pin on a global deploy now names the BedrockGeoRegion output, the template-size guard now measures the ECS template it was always about, and the inert clearedOverrideFields plumbing is gone. The delta commit is a good commit except for one thing it does not mention: it deletes six pre-existing regression tests. That, plus two invariants this PR asserts everywhere but its own default path, is what holds it.

Method disclosure. I ran no suite: no mise run build, no jest/pytest/tsc, no cdk synth, no installs (the review worktree has no node_modules/.venv). Every claim below is static reading of the tree at e6464b0b plus read-only git/gh. All CI checks on this PR currently pass — which is consistent with Blocking 1, because deleting a test never turns CI red.

Prior blockers / open threads

Prior item	Status	Evidence at `e6464b0b`
Blocker 1 (`c2d6dc0d`) — `DEVELOPER_GUIDE.md:219` prescribed a `us.` Blueprint pin the shipped `global` default cannot invoke	Fixed	`docs/guides/DEVELOPER_GUIDE.md:219` now reads "Prefix it with the geography the stack grants, read from its `BedrockGeoRegion` output; a `us.` prefix on a `global` deployment is granted nothing and fails at turn 0." Mirror regenerated.
Blocker 2 (`c2d6dc0d`) — the template-budget guard measured the `agentcore` template while its rationale was about `ecs`	Fixed	Test relocated into the ECS `describe` (`cdk/test/stacks/agent.test.ts:940`), reusing that block's `beforeAll` template — no extra synth. One residual: the old comment's "`ecs` … synthesizes at 992,111 bytes" is gone and the new comment says only "~34 KB larger than the default"; since CI is green the ECS template must in fact be under the 950,000 budget, so the earlier figure appears to have been wrong. Worth stating the measured number so the 5% margin claim is checkable.
N1 troubleshoot `SKILL.md` still listed Opus 4 as granted	Fixed	`docs/abca-plugin/skills/troubleshoot/SKILL.md:114` and `DEPLOYMENT_GUIDE.md:139` now read "Sonnet 4.6, Opus 4.8, Opus 5, Haiku 4.5".
N2 falsified "longest-first" claim migrated into `cli/test/model-id.test.ts`	Fixed	The test comment now says "The list is NOT sorted by length (`apac` follows `us`), which is why this is asserted behaviourally."
N3 two new output comments ordered inversely; `ComputeSubstrate` paragraph dangling	Partly fixed / regressed	The `BedrockModelIds`/`BedrockGeoRegion` ordering is fixed, but the `ComputeSubstrate` rationale (`cdk/src/stacks/agent.ts:980-986`) is now worse: the delta appended "Both outputs are consumed by `platform doctor`…" to it and inserted `BedrockModelIds` (`:989`) and `BedrockGeoRegion` (`:995`) between it and `ComputeSubstrate` (`:1002`), which now has no comment at all. See inline.
N4 `checkBedrockModel` kept the binary classifier	Fixed (behaviour)	`cli/src/platform-doctor.ts:442` now uses `statusFor(classifyProbeFailure(err))`. Two residuals: the comment explaining the matching was left behind in a function that no longer does any, and no test exercises the new error path — see inline.
N5 inert `clearedOverrideFields` / vacuous test	Fixed	Function and the `props?` threading are gone from `cdk/src/constructs/blueprint.ts`.
N6 / @ayushtr-aws thread on `workflows.ts:89` — allowlist now admits all 7 geographies	Not fixed, deferred to #846 — accepted as non-blocking, but the doc block above the constant still describes the deleted hand-maintained literal and is now false on four counts. See inline.
N7 dangling `{@link haikuInferenceProfileId}`	Fixed	Function still exists at `bedrock-models.ts:137`; link resolves.

No unresolved thread from another reviewer is left unaddressed except #846 (accepted).

Vision alignment

Fits the vision (VISION.md — bounded blast radius, reviewable outcomes, fire-and-forget default) and ADR-021's one-agent-many-substrates rule. One resolved geography now drives every grant and both injected model env vars on all three substrates; BedrockGeoRegion/BedrockModelIds become documented CfnOutput contracts instead of something a client regexes out of a template; --model is rejected before it reaches the RepoTable; the bedrockModels wildcard rejection tightens IAM rather than loosening it. Model config correctly stays in CDK context rather than CFN parameters, because grantInvoke resolves at synth (#740/#741). Bootstrap (ADR-002) untouched.

Where it does not finish the job is symmetry: the PR adds a guard at every boundary a repo or workflow can name a model, and none at the boundary where the platform names its own (Blocking 2).

Blocking issues

1. cdk/test/stacks/agent.test.ts:48 — the delta commit deletes six pre-existing regression tests; its message says it moved one.

git diff c2d6dc0d..HEAD -- cdk/test/stacks/agent.test.ts removes 138 lines from the default describe('AgentStack'). One is the intentional relocation. The other six are gone from the whole repo — verified with git grep at both origin/main and HEAD across all of cdk/:

Deleted test	Replacement at HEAD
`creates exactly 21 DynamoDB tables`	none (the only stack-wide table-drift guard)
`creates TaskApprovalsTable with user_id-status-index GSI`	partial — construct-level only (`cdk/test/constructs/task-approvals-table.test.ts`), no stack-level assertion
`outputs TaskApprovalsTableName`	none — no other reference to that output in `cdk/test/`
`the orchestrator carries the platform_config transport env on EVERY compute type`	none
`the forwarded identifiers are the SAME stack values the AgentCore runtime gets`	none
`outputs ComputeSubstrate=agentcore on the default (no-gate) deploy`	none — only the `ecs` (:952) and `lambda-microvm` (:1046) variants survive, so the default, most-deployed path lost the assertion the CLI onboarding gate reads

The two orchestrator-env guards are the ADR-021 sub-decision-3 guards that a MicroVM agent and an AgentCore agent read the same approvals table, nudges table, artifacts buckets and session role. This PR's own new comment at cdk/src/constructs/task-orchestrator.ts:511-518 explains that the anthropicModel prop was declared-but-never-read and "every synth assertion still passed" — the parity guard is precisely the thing that class of bug needs, and this PR should have extended it with ANTHROPIC_MODEL, not removed it.

Risk: a dropped table, a renamed TaskApprovalsTableName output (CLI/Cedar HITL consumers), an orchestrator env rename that diverges from the AgentCore runtime, or a stack that stops reporting agentcore all now ship with a fully green suite.

Fix: restore the six from git show origin/main:cdk/test/stacks/agent.test.ts (none of them touches Bedrock model ids, so they should pass as-is), keep the template-budget relocation, and add ANTHROPIC_MODEL to both orchestrator-env key lists. If any deletion was deliberate, say so in the commit message and name the replacement coverage.

2. cdk/src/stacks/agent.ts:535 — nothing checks that the platform default model is in the deploy's own grant list, and the three new doctor checks all pass when it is not.

ANTHROPIC_MODEL / ANTHROPIC_DEFAULT_HAIKU_MODEL are injected on all three substrates from the hardcoded constants PLATFORM_DEFAULT_MODEL_ID / PLATFORM_DEFAULT_AUX_MODEL_ID (agent.ts:535-536, ecs-agent-cluster.ts:389-391, agent.ts:1102/1108 for the MicroVM platform_config). resolveBedrockModelIds (bedrock-models.ts:152-213) validates shape, geo prefixes and wildcards — but never membership of the platform defaults.

Failure chain: cdk deploy -c bedrockModels='["anthropic.claude-sonnet-4-6"]' — a documented, code-change-free override (onboard-repo/SKILL.md:136, DEVELOPER_GUIDE.md:125) — synthesizes cleanly with IAM scoped to Sonnet profiles only, while every substrate is told ANTHROPIC_MODEL=global.anthropic.claude-opus-5. Every task with no per-repo pin dies at turn 0 with an AccessDenied naming no cause. bgagent platform doctor gives false assurance on all three checks: bedrock_model and bedrock_inference_profile probe the CLI's own PLATFORM_REPO_DEFAULTS.model_id, not the stack's resolved default, and bedrock_granted_model_profiles only asks whether each granted profile exists — never whether the model the agent is told to invoke is granted. The existing test DEFAULT_BEDROCK_MODEL_IDS covers the agent runtime default checks the constant list, not a resolved override.

Note this is new with this PR: before it, the stack did not inject ANTHROPIC_MODEL at all (per bedrock-model-constants.ts:129-133), so narrowing the grant list and editing the Python literal worked. bedrock-model-constants.ts:104 already writes down half the relationship ("A model can be granted without being a default"); the inverse is the one that breaks a deploy.

Fix: assert it at synth — in resolveBedrockModelIds or once in AgentStack, throw when PLATFORM_DEFAULT_MODEL_ID or PLATFORM_DEFAULT_AUX_MODEL_ID is absent from the resolved list, naming the missing model and the bedrockModels key. Add a test with a narrowing context. Consider also having checkBedrockInferenceProfile probe the stack's exported default rather than the CLI literal.

3. Two operator-facing surfaces contradict this PR's own code. Same class as blocker 1 of my c2d6dc0d review, which was accepted as blocking.

docs/abca-plugin/skills/onboard-repo/SKILL.md:137-139 (added by the delta) states that bedrockModels "takes an array, and that rules out the command-line form — -c supplies each value as a string, so -c bedrockModels='["anthropic.claude-opus-5"]' is rejected at synth". This PR's own resolver JSON-parses exactly that form so both behave identically (cdk/src/constructs/bedrock-models.ts:203-212, with a test the PR adds: "parses a JSON-string override (the -c key=value CLI form)"), the resolver's JSDoc documents -c bedrockModels='[…]' as the way, and this PR's new CliError (cli/src/model-id.ts:129) hands the operator that exact remedy. So a guided-onboarding user blocked by the new CLI rejection is told the CLI's own remedy does not work, and is steered instead to a hand-edit of cdk.json that — per Blocking 2 — silently un-grants the platform default if they omit an entry.
docs/guides/DEVELOPER_GUIDE.md:221, in the section the file itself calls "the canonical reference", still says a platform-wide model change is "the bedrockModels context plus the layer-2 call sites above", and layer 2's Where cell names only agent/src/config.py:563 / models.py:157. After this PR the stack injects ANTHROPIC_MODEL on every substrate, so editing those literals has zero effect on any deployed task — the operator concludes the change shipped while every task still runs Opus 5 (or fails at turn 0 if they also narrowed the grant). The model-default-docs-parity guard cannot catch this: it compares the documented value to the Python literal.

Fix: delete the "that rules out the command-line form … has no effect on it" sentence and replace it with the working -c form (keep the "this REPLACES the default list" warning on the cdk.json example); name PLATFORM_DEFAULT_MODEL_ID (cdk/src/handlers/shared/bedrock-model-constants.ts) as the primary source in the layer-2 Where cell, the ANTHROPIC_MODEL Who sets it cell, and the platform-wide recipe at :221; regenerate the Starlight mirror.

Non-blocking suggestions / nits

cli/src/platform-doctor.ts:516 and :611 (MAJOR) — the delta's commit message says "All four checks now share one classifyProbeFailure helper … which removes the third and fourth copies of that regex". In the code only two of four call it; checkBedrockInferenceProfile (:515-524) and checkGrantedModelProfiles (:610-618) still hand-roll both regex literals, so there are now three copies. A future correction applied to the helper silently misses the two probes whose fail remedy is the destructive one ("remove it from the bedrockModels context"). Route both through classifyProbeFailure and switch on the returned bucket, keeping only the per-site detail strings local.
cli/src/platform-doctor.ts:442 (MAJOR — test coverage) — the delta's headline behaviour change for bedrock_model (transient → warn + "did not complete", not fail + "Enable model access in the Bedrock console") is unpinned: the only bedrock_model test asserts the pass path (cli/test/platform-doctor.test.ts:231) and GetFoundationModelCommand is only ever mocked to resolve. Reverting just that call site leaves the CLI suite green. Its two siblings each have explicit Throttling/ResourceNotFound cases, so the omission looks accidental — add the same it.each shape.
cdk/src/handlers/shared/workflows.ts:74-88 (MAJOR — comment drift) — the doc block still describes the hand-maintained literal the PR replaced, and is false on four counts: "accepting both the bare id and the us.-prefixed … form" (it is now bare + all seven geographies), "a SEPARATE, hand-maintained list from the IAM grant source" (it is now derived from it), "add it here too" (there is no list here to add to; the edit point is handlers/shared/bedrock-model-constants.ts), and "consolidating it with bedrock-models.ts is tracked separately" (done in this commit — and that module is now a re-export shim, so the pointer sends a maintainer to the wrong file). Please also state plainly that admission is deliberately geography-blind today with turn-0 AccessDenied as the fallback, and put the #846 reference inline where main's warning used to be.
cdk/test/handlers/shared/workflows.test.ts:340 — the allow-list admits nothing that is not granted builds grantedForms from the same flatMap over the same two constants, so it can only fail if the one-line derivation is edited, and it certifies as "granted" six geographies the deploy grants nothing for. Rename it to state the real invariant (admission is geography-agnostic; #846) so it stops reading as a grant-parity proof.
cli/src/model-id.ts:36 (MAJOR — drift) — BEDROCK_GEO_PREFIXES is a third hand-maintained copy of the geography list, and unlike the CDK copy it has no forcing function: cli/test/model-id.test.ts:152 "recognizes every geography the CDK models" loops over BEDROCK_GEO_PREFIXES itself, so it can never fail. When the alpha enum gains a geography the CDK literal is forced to update (cdk/test/constructs/bedrock-models.test.ts:60-67) and the CLI silently lags: on a -c bedrockGeoRegion=<new> deploy, geoPrefixOf returns undefined, assertModelIdUsable rejects a correctly-formed granted model as "a bare foundation-model id" and suggests <new>.<new>.anthropic.…, and GEO_PREFIX_RE stops stripping so bedrock_model fails on a healthy stack. That is the exact two-copies bug this file's own header describes. Either promote the list into contracts/constants.json (the repo already validates that both ways) or add a text-read parity test like cdk/test/contracts/model-default-docs-parity.test.ts does for agent/src/models.py.
cdk/src/stacks/agent.ts:987 — N3, partly regressed; see the table above and the inline comment.
cdk/src/constructs/bedrock-models.ts:66-68 — GEO_REGION_ENUM_VALUES is documented as "the allow-list resolveBedrockGeoRegion validates against", but that function validates against GEO_REGIONS (:121) — the dependency-free literal. GEO_REGION_ENUM_VALUES is referenced once, at :89, feeding GEO_ALTERNATION/GEO_PREFIX_RE. A maintainer widening the validated geography set edits the constant the comment names and changes only the prefix-rejection regex. The second half of the sentence is correct, which makes the wrong half convincing.
cli/src/model-id.ts:32-34 — "platform-doctor builds a REGEX from a similar list … same reasoning recorded there" — it is the same list (platform-doctor.ts:48 imports BEDROCK_GEO_PREFIXES from here, and its comment says "the ONE geography list … Two copies is how this broke"), and no ordering reasoning is recorded there. A maintainer chasing this may conclude a second list needs syncing.
cli/src/model-id.ts:64-70 — the doc narrates an unmerged intermediate revision of itself ("An earlier version did not, on the reasoning that … which was wrong"). Post-merge that resolves only in branch history; the load-bearing fact is already stated in the next sentence.
contracts/constants.json:35 — anthropic_model is additive to a fail-closed, unversioned wire allowlist: agent/src/server.py:1267-1275 refuses the whole platform_config block on any unrecognised key, and the guest's copy of constants.json is baked into the MicroVM snapshot, which a plain cdk deploy does not rebuild (lambda-microvm-compute.ts pins baseImageArn/baseImageVersion). An operator who deploys this PR without re-packaging gets every MicroVM session rejected at start, not merely a missing model default. The same is true of the pre-existing anthropic_default_haiku_model, so this is precedent rather than a new pattern, and the substrate is still in development — but please either document the snapshot-rebuild requirement in the deploy guide or send the default on agent_payload (server.py:540-542 already reads anthropic_model off /run input, which is backward compatible).
cdk/src/constructs/blueprint.ts:319 — props.agent.modelId is still written to the RepoTable verbatim with no synth-time validation, while the sibling writer (repo onboard --model) now rejects bare ids, double prefixes, ungranted models and geography mismatches — and agent.maxBudgetUsd in the same construct is validated at synth. resolveBedrockGeoRegion(this.node)/resolveBedrockModelIds(this.node) are both in scope. This was "strongly recommended" in my last review; still worth a follow-up issue if not this PR.
cli/src/platform-doctor.ts:580 — "Stack does not export BedrockModelIds and BedrockGeoRegion" fires when either is missing, so on a partially-updated stack the detail states a half-false fact. Interpolate the actually-missing name(s).
agent/scripts/diagnostics/test_sdk_smoke.py:123 — ok is computed from message counts and ignores the errors list, so a run that streams a result and then raises prints PASS and exits 0 with the exception below the verdict. ok = … and not errors, print errors above the verdict, and try/finally: await client.disconnect().
cdk/test/constructs/blueprint.test.ts:430 — the rationale added to onUpdate SETs the three asset columns… ("this blueprint declares no agent.modelId, so it legitimately removes model_id") is stale now that clearedOverrideFields is deleted, and it contradicts the test ten lines below. The original blanket expect(serialized).not.toContain('REMOVE') was strictly stronger.
cdk/src/constructs/blueprint.ts:499 — buildRemoveClause/buildRemoveNames are left with a purely cosmetic delta against main (a renamed local plus an intermediate variable). Reverting the bodies would leave only the class-doc change and make the real edit legible in blame.
cdk/src/handlers/shared/bedrock-model-constants.ts:135 — inferenceProfileId(geoRegion, bareModelId) takes two interchangeable unvalidated strings, so the one function that owns the bare/prefixed distinction cannot enforce it (inferenceProfileId('global', 'us.anthropic…') returns global.us.anthropic…). Callers are correct today; a BEDROCK_GEO_REGIONS.some(...) throw would make that structural.

Documentation

Updated and mirror-synced: DEVELOPER_GUIDE.md (model-configuration section, new geography subsection), DEPLOYMENT_GUIDE.md (residency tradeoff + cost table), QUICK_START.mdx, USER_GUIDE.md, BEDROCK_COST_ATTRIBUTION.md, REGISTRY.md, and the onboard-repo / troubleshoot plugin skills. Starlight mirrors are regenerated for every edited source (spot-checked developer-guide/Model-configuration.md, getting-started/Deployment-guide.md, architecture/Bedrock-cost-attribution.md for row parity), and the retargeted #repository-preparation / #repo-level-instructions anchors both exist.

Missing: the two contradictions in Blocking 3 (and their mirrors); the layer-1 row at DEVELOPER_GUIDE.md:125 still says "(default us)" where every other artefact this PR touched uses the precise "global in the shipped cdk.json, us absent any context"; the ANTHROPIC_MODEL Who sets it row (:135) still credits only the ECS strategy and your shell. #747's residency-tradeoff acceptance criterion is satisfied, so no ADR is owed.

Tests & CI

Test work here is otherwise unusually mutation-aware, and I want that on the record: the global value in cdk.json is pinned, the agent's Python fallback is pinned, ANTHROPIC_MODEL injection is pinned per-geography for AgentCore, both ECS task defs and the MicroVM platform_config (with a deliberately non-global fixture so a dropped value cannot pass on the agent's own fallback), the opus-4 grant removal is pinned by exact-set assertions on both substrates, and the new cdk/test/handlers/shared/runtime-bundle-boundary.test.ts esbuilds the real handler entry and asserts zero aws-cdk-lib references — the gate whose absence let my first blocker land. Almost everything I traced fails on revert.

The exceptions are Blocking 1 (six guards removed, nothing added in their place) and three unpinned behaviours: checkBedrockModel's new three-way classifier, the platform-default-∈-grant invariant (which does not exist yet), and the two new workflows.test.ts cases that rebuild the production derivation and so pass by construction. Two notes for #366: the relocated template-budget test correctly reuses the ECS beforeAll template (no extra synth), but runtime-bundle-boundary.test.ts runs two real esbuild.buildSync bundles under a 60 s timeout — justified, worth capping to one entry if it shows up in CI wall-clock.

Bootstrap synth coverage (ADR-002): pass, no updates needed. The PR adds only two CfnOutputs (well under the 200-output limit) and no new CloudFormation resource type, so no bootstrap-template or synth-coverage change is implied. The 1 MB template-budget guard is the relevant ceiling and now measures the ECS substrate.

CI: all checks currently pass. I did not execute any suite locally (no node_modules/.venv in the review worktree, and installing would pollute shared state), so nothing above is corroborated by execution — and note that a green suite is exactly what Blocking 1 predicts.

Review agents run

Six lenses folded in: code-review, silent-failure, tests, types, comments, security-governance-docs. None omitted or failed. I triaged every BLOCKER/MAJOR against the tree at e6464b0b and dropped or downgraded what I could not reproduce — notably the contracts/constants.json "not in required" claim (the sibling aux-model key has the same shape, so it is precedent, not a new gap) and the workflow-allowlist fan-out (real, but already accepted as deferred to #846). Governance is clean: #747/#804/#805 all carry approved, the branch name follows the convention, blocking dependency #746 is closed, and deferrals are tracked in #846/#847. Limitation restated: no suite, synth, or build was executed — static reading only; CI is green.

Human heuristics

Proportionality — concern. 55 files for a geography flip is defensible (three substrates plus 20 generated mirrors), but two hunks carry no reviewable change: blueprint.ts:499's cosmetic rename, and the six deleted tests in agent.test.ts that are unrelated to Bedrock and undisclosed in the commit message.
Coherence — concern. Three copies of the geography list (cli/src/model-id.ts:36, bedrock-model-constants.ts:121, the alpha enum) with a guard on only two of the three edges, and three copies of the probe classifier (platform-doctor.ts:79, :516, :611) after a commit that says it removed two of them.
Clarity — concern. The direction of travel is right (the new comments explain why, and bedrock-model-constants.ts's header is a model of it), but the load-bearing pointers are stale: workflows.ts:74-88 points at a shim, bedrock-models.ts:68 names the wrong validator, platform-doctor.ts:432 explains matching in a function that does none, and agent.ts:980 documents the wrong output.
Appropriateness — pass. The fix lands where the failure originates: geography resolved once at synth and pushed into every substrate, validation at the CLI boundary that writes the RepoTable, IAM still per-model ARN-scoped, no CFN parameters (correct — grantInvoke resolves at synth). The one place the fix does not reach its own origin is the platform default, which is Blocking 2.

…efault 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: #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 #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.

isadeks · 2026-09-03T12:46:19Z

All three blockers were valid; f6abd06f addresses them plus every MAJOR nit. Two items I am declining, with reasons, at the end.

Blocking 1 — correct, mine, and the most serious thing in the PR

Six pre-existing tests, deleted, while the commit message said "moved". Root cause worth recording because the guard I used 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. Your method disclosure is the point: no suite would have caught this, and CI stayed green because deleting a test never turns CI red.

All six restored verbatim from origin/main, relocation kept. Your suggestion to extend rather than restore is the better half of the finding: ANTHROPIC_MODEL is now in both orchestrator-env key lists. That parity guard is precisely the shape 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 — verified before fixing, and worse than a doc problem

-c bedrockModels='["anthropic.claude-sonnet-4-6"]'   → synth rc=0
GRANTED:      global.anthropic.claude-sonnet-4-6
agent TOLD:   global.anthropic.claude-opus-5, global.anthropic.claude-haiku-4-5

AgentStack now throws at synth naming the missing defaults. Verified in all three directions: narrowing override fails, complete override passes, default path passes.

I put it in the stack, not resolveBedrockModelIds — you offered both. In the resolver it broke three existing shape tests that use minimal two-model fixtures, and that was the signal: the resolver validates a list's shape and has no business knowing the platform defaults. The stack is where the two facts meet, because it both builds the grant and injects the defaults.

One pre-existing test legitimately asserted "override replaces the defaults, so defaults are absent". That end state became invalid the moment injection started — same reasoning as your "note this is new with this PR". Its fixture now keeps the two required defaults and drops Sonnet, so its real subject (replace-not-append) is carried by Sonnet's absence rather than lost.

Blocking 3 — both correct, and one of them was my measurement going stale

The -c claim I wrote was true when I measured it and is not now: #358 added JSON-string parsing to the resolver afterwards. Re-verified (2 granted profiles from a 2-model -c override) and corrected, keeping the replace-not-append warning and noting that --context-file is the form that is silently ignored.

DEVELOPER_GUIDE's bump recipe is the more consequential half — it told operators to edit the Python literals, which after this PR have no effect on any deployed task. PLATFORM_DEFAULT_MODEL_ID is now named as the step that changes what runs. Two pointers also still named constructs/bedrock-models.ts, which the values moved out of. Same class, found while there: the onboard-repo skill claimed Sonnet 4.6 as the default in one paragraph and Opus 5 in another.

MAJOR nits — all fixed

Doctor consolidation. You are right that my commit message overstated it: only two of four used the helper, leaving three copies of each regex, and the two hand-rolled ones were exactly the checks whose fail remedy is destructive. Now one copy; mutation-verified (9 and 5 tests die on the two ways to break the helper).
bedrock_model coverage. Unpinned as described — reverting that call site left the suite green. Now has the same Throttling/5xx/timeout/denied/absent cases as its siblings.
workflows.ts doc block. False on all four counts you listed. Rewritten to say the list is derived, that admission is deliberately geography-blind with turn-0 AccessDenied as the fallback, and feat(orchestration): scope workflow model admission to the deployed geography #846 inline.
workflows.test.ts:340. Renamed to state what it actually pins — the derivation, not grant parity — since both sides come from the same flatMap.
BEDROCK_GEO_PREFIXES forcing function. Added a text-read parity test against the CDK literal, mutation-verified by adding a geography CDK-side and watching the CLI suite fail. I did not promote the list into contracts/constants.json: that is a shared-contract restructure with its own review surface, and the cheap half closes the actual gap (no forcing function). Happy to do the promotion as a follow-up if you would rather.
Comment corrections, all removals: bedrock-models.ts named the wrong constant as what resolveBedrockGeoRegion validates against; 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 moved after ComputeSubstrate so main's comment regains its subject.
Template budget figures. Added: 894,261 (ecs) vs 858,062 (default), with the caveat that makes them honest — in-test bytes are lower than what cdk synth writes, because Template.fromStack leaves asset hashes and account tokens symbolic. So the budget is a trend guard on the relative number, not a prediction of what CloudFormation receives. My earlier 992,111 was the CLI figure and I should not have put it next to an in-test assertion.

Declining, with reasons

contracts/constants.json:35 — the MicroVM snapshot rebuild. Real, and you identified it precisely, but I am not changing it here. Sending the default on agent_payload instead is an agent-contract change (⚠️ ask-first in AGENTS.md), and anthropic_default_haiku_model already established the pattern, so this PR is not introducing it. It is also unreachable today: #830 records that compute_type=lambda-microvm synthesizes over CloudFormation's 1 MB limit and cannot deploy at all. Documenting a rebuild step for a substrate that cannot currently be deployed would be documentation of a hypothetical. Worth its own issue once #830 clears — say the word and I will file it.

checkBedrockInferenceProfile probing the stack's exported default rather than the CLI literal. Agreed, and it is already tracked as #847 along with the repo show geography and the nine DescribeStacks calls. Leaving it there rather than growing this PR further.

Suites: 4374 CDK (+10), 836 CLI (+6), 1775 agent. Full build clean.

Conflicts with #831 (Linear OAuth Token Vault), which touched the same three files. All three were additive on both sides; resolved as unions. - `cdk/src/constructs/ecs-agent-cluster.ts` — imports. This branch's model constants plus main's `LinearIdentityVault`. - `cdk/src/stacks/agent.ts` — the lambda-microvm `platform_config` block. Both sides add fields, so both are kept, but NOT verbatim: main's side calls `haikuInferenceProfileId(bedrockGeoRegion)`, and that helper no longer exists here — it was removed when the two duplicate haiku-id paths were collapsed into one. Taking main's line as written would not compile. Resolved to `inferenceProfileId(bedrockGeoRegion, PLATFORM_DEFAULT_AUX_MODEL_ID)`, which is the same value through the surviving helper, and the vault fields are kept unchanged. - `cli/src/platform-doctor.ts` — three hunks: the `linear-auth-health` import, and the `Promise.all` destructuring plus its call list. The last two must stay positionally aligned, so both were extended in the same order; verified all ten names now line up with their outputs. Also fixed a `{@link haikuInferenceProfileId}` in `bedrock-models.ts` left dangling by that same helper removal — the merge is what surfaced it, since main's live call site was the only other reference. `yarn.lock` moves with main (+42, `@aws-sdk/client-sns` for #831's revocation alerts). Checked for semantic drift rather than trusting a clean textual merge: the restored "creates exactly 21 DynamoDB tables" guard is byte-identical to main's at the same line, so #831 added no table and the merge did not take a stale side of it. Suites after merge: 4499 CDK, 948 CLI, 1782 agent — `mise run build` clean.

…ests `/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.

scottschreckengaust

Verdict: Approve (with non-blocking nits)

Re-review at head 5640d879. All three blockers from the last CHANGES_REQUESTED (@scottschreckengaust, e6464b0b) are verified fixed in commit f6abd06f, and no new blocker survives an independent re-review. CI is green (8 pass / 1 skipping, none failing or pending), the three backing issues (#747/#804/#805) all carry approved, and the branch name follows convention. The remaining items are comment-accuracy, test-coverage, and latent-drift nits — none blocks merge.

Method disclosure. I am a review subagent and cannot spawn the nested pr-review-toolkit agents in this context, so I applied each lens by hand (see Review agents run) and read the security-sensitive surfaces (IAM grant derivation, bedrockModels context input, wildcard rejection, CLI --model gate) directly. I did not execute the suites locally (shared-worktree / tmpfs constraints); CI is the corroboration.

Vision alignment

Fits the north star (VISION.md — bounded blast radius, reviewable outcomes, fire-and-forget default) and ADR-021's one-agent-many-substrates rule. One resolved geography now drives every IAM grant AND both injected model env vars (ANTHROPIC_MODEL, ANTHROPIC_DEFAULT_HAIKU_MODEL) on all three substrates, closing the fire-and-forget failure mode where a non-default bedrockGeoRegion granted one geography while the agent invoked another's profile. BedrockGeoRegion/BedrockModelIds become documented CfnOutput contracts rather than something a client regexes out of a template; --model is rejected before it reaches the RepoTable; the bedrockModels wildcard rejection and the profile-less-model removal both tighten IAM rather than loosen it. Model config correctly stays in CDK context (not CFN parameters) because grantInvoke resolves at synth. The global geo flip is a data-residency change, and #747's residency-tradeoff acceptance criterion is documented — no ADR owed.

Prior blockers — all verified fixed at `5640d879`

Six deleted regression tests (agent.test.ts) — restored verbatim from origin/main in f6abd06f (the stack-wide 21 DynamoDB tables drift guard, the TaskApprovalsTable GSI + output, both ADR-021 orchestrator-parity guards, and ComputeSubstrate=agentcore on the default path). The two orchestrator-env key lists were also extended with ANTHROPIC_MODEL — the exact guard shape that catches a declared-but-never-read prop. Net diff vs main shows no unrelated test deletions.
Platform default not checked against its own grant list — AgentStack now throws at synth when PLATFORM_DEFAULT_MODEL_ID / PLATFORM_DEFAULT_AUX_MODEL_ID fall outside the resolved bedrockModels, naming the missing model(s) and the fix. Covered by four new tests (AgentStack platform-default / grant-list coherence). This is the boundary the PR's other guards missed — verified the failure (-c bedrockModels='["anthropic.claude-sonnet-4-6"]' synthed clean while injecting Opus 5) is now a synth error.
Two operator-facing docs contradicted the code — both fixed in f6abd06f: onboard-repo/SKILL.md no longer claims the -c command-line form is "ruled out" (now: "Both forms behave identically — the resolver JSON-parses the string -c delivers"), and DEVELOPER_GUIDE.md:190 now states plainly that editing the Python literals "has no effect on any deployed task" and names PLATFORM_DEFAULT_MODEL_ID as the value to change (layer-2 row and the platform-wide recipe at :222 match). Starlight mirrors regenerated.

Blocking issues

None.

Non-blocking suggestions / nits

cli/src/model-id.ts:36 (latent drift, no current defect) — BEDROCK_GEO_PREFIXES is a third hand-maintained copy of the geography list with no forcing function: cli/test/model-id.test.ts's "recognizes every geography" loops over this same constant, so it can never fail. The CDK-side literal is forced to track the alpha enum (bedrock-models.test.ts), so a future enum-added geography would leave the CLI silently lagging — on a -c bedrockGeoRegion=<new> deploy, assertModelIdUsable would reject a correctly-formed granted model. Consider a text-read parity test against the CDK/contracts source, mirroring model-default-docs-parity.test.ts.
cli/src/platform-doctor.ts (three copies of the probe classifier) — checkGrantedModelProfiles and checkBedrockInferenceProfile still hand-roll the AccessDenied/not-found regex rather than routing through classifyProbeFailure, so a future correction to the helper misses the two probes whose fail remedy is destructive ("remove it from the bedrockModels context").
cli/src/platform-doctor.ts:442 (coverage gap) — the new three-way warn/fail classification for checkBedrockModel is unpinned; the only test asserts the pass path, so reverting the classifier call site leaves the CLI suite green. Its siblings have Throttling/ResourceNotFound cases — add the same it.each.
cli/src/platform-doctor.ts:579 (accuracy) — "Stack does not export BedrockModelIds and BedrockGeoRegion" fires when either is missing; on a partially-updated stack the detail states a half-false fact. Interpolate the actually-missing name(s).
cdk/test/constructs/blueprint.test.ts:429 (stale/contradictory comment) — the rationale "this blueprint declares no agent.modelId, so it legitimately removes model_id" is false against the current behavior (non-asset overrides are carried forward, not cleared) and contradicts the very next test (onUpdate does NOT remove model_id). Comment-only; the assertion itself is correct.
agent/scripts/diagnostics/test_sdk_smoke.py:123 — ok is computed from message counts and ignores the errors list, so a run that streams a result then raises prints PASS and exits 0. Suggest ok = … and not errors, print errors above the verdict.
cdk/src/handlers/shared/bedrock-model-constants.ts inferenceProfileId — takes two interchangeable unvalidated strings, so the one function that owns the bare/prefixed distinction cannot enforce it. Callers are correct today; a guard would make that structural.
cdk/src/constructs/blueprint.ts agent.modelId — still written to the RepoTable with no synth-time validation, while the sibling CLI writer now rejects bare/double-prefixed/ungranted/mismatched-geo ids. Worth a follow-up issue (symmetry with assertModelIdUsable).
Comment drift — workflows.ts doc block and bedrock-models.ts:66-68 (GEO_REGION_ENUM_VALUES documented as the validator's allow-list, but validation is against GEO_REGIONS) point at the wrong symbols after the constants extraction. Non-load-bearing.

Documentation

Updated and mirror-synced: DEVELOPER_GUIDE.md (model-configuration layers + geography subsection), agent/README.md, onboard-repo/troubleshoot plugin skills. The model-default-docs-parity test now also guards models.py TaskConfig literals and the operator-skill geography prefixes. No ADR owed (#747 residency tradeoff is an accepted acceptance criterion, not a new tenet trade).

Tests & CI

Unusually mutation-aware: the global cdk.json value is pinned, per-geography ANTHROPIC_MODEL injection is pinned on AgentCore + both ECS task defs + the MicroVM platform_config (with a deliberately non-global fixture so a dropped value cannot pass on the agent's own global. fallback), the opus-4 grant removal is pinned by exact-set assertions on both substrates, the platform-default-∈-grant invariant now has four tests, and runtime-bundle-boundary.test.ts esbuilds the real handler entry and asserts zero aws-cdk-lib references (the gate whose absence caused the original 57 MB bundle regression). Residual coverage gaps noted in nits 2-3.

Bootstrap synth coverage (ADR-002): pass, no updates needed. The PR adds only two CfnOutputs and introduces no new CloudFormation resource type, so no bootstrap-policy, action-map, version, or golden-baseline change is implied. The 1 MB template-budget guard is the relevant ceiling and now measures the ECS substrate.

CI: all checks pass (8 pass, 1 skipping). mergeStateStatus=BLOCKED reflects the required-approval gate, not a failing check (mergeable=MERGEABLE).

Review agents run

Execution context cannot spawn nested pr-review-toolkit agents, so each lens was applied by hand and that limitation is stated here per the process requirement:

code-reviewer — by hand: routing correct (CDK/CLI/agent trees per AGENTS.md), L2 constructs, per-model ARN scoping preserved, no hardcoded account ids.
silent-failure-hunter — by hand: the doctor probe classifier (denied/absent/unverified) and the smoke-test exit-code fix are net improvements; residual ok-ignores-errors noted (nit 6).
type-design-analyzer — by hand: new ProbeFailure union and assertModelIdUsable arg shape are sound; inferenceProfileId's two-string signature noted (nit 7).
comment-analyzer — by hand: stale/contradictory comments noted (nits 5, 9).
pr-test-analyzer — by hand: strong mutation-awareness; coverage gaps noted (nits 2-3).
security-review — by hand: bedrockModels wildcard rejection closes an account-wide-grant escalation via context; geo flip and CLI gate fail closed; IAM stays per-model ARN-scoped; no secrets/network/Cedar surface touched.

Human heuristics

Proportionality — pass. 55 files for a geography flip is defensible (three substrates + generated mirrors); the previously-flagged cosmetic hunks and undisclosed test deletions were reverted/restored in f6abd06f.
Coherence — concern (non-blocking). Three copies of the geography list (CDK literal, model-id.ts, alpha enum) with a forcing function on only two edges (nit 1); a few comment pointers name the wrong symbol post-refactor (nit 9).
Clarity — pass. New comments explain why; bedrock-model-constants.ts's bundle-boundary header is a model of it. Minor stale pointers noted.
Appropriateness — pass. The fix lands where the failure originates: geography resolved once at synth and pushed into every substrate; validation at the CLI boundary that writes the RepoTable; the platform-default guard now closes the one symmetry gap the last review flagged.

isadeks added 3 commits August 26, 2026 13:22

isadeks requested review from a team as code owners August 27, 2026 14:14

This comment has been minimized.

Sign in to view

isadeks added 2 commits August 27, 2026 12:02

isadeks marked this pull request as draft August 27, 2026 16:53

isadeks changed the title ~~fix(cli): check the inference profile doctor will invoke, and reject an unusable --model~~ feat(cdk): switch to the global Bedrock geo, with doctor and --model validation Aug 27, 2026

This was referenced Aug 27, 2026

docs(model): document bedrockGeoRegion and the global-vs-geo tradeoff #799

Closed

feat(cdk): switch the Bedrock inference profile to the global geo #800

Closed

isadeks marked this pull request as ready for review August 27, 2026 17:37

isadeks mentioned this pull request Aug 27, 2026

feat(cdk): switch Bedrock inference profile to the global geo #747

Closed

11 tasks

This comment has been minimized.

Sign in to view

This was referenced Aug 28, 2026

Granted model anthropic.claude-opus-4-20250514-v1:0 has no inference profile — passes every guard, fails at turn 0 #819

Closed

bedrockModels context accepts '*', silently converting a scoped grant into a wildcard #821

Closed

This comment has been minimized.

Sign in to view

Merge branch 'main' into fix/804-805-doctor-and-model-validation

c2d6dc0

ayushtr-aws previously approved these changes Sep 1, 2026

View reviewed changes

Comment thread cdk/src/handlers/shared/workflows.ts

scottschreckengaust requested changes Sep 1, 2026

View reviewed changes

Comment thread docs/abca-plugin/skills/troubleshoot/SKILL.md Outdated

Comment thread cli/test/model-id.test.ts Outdated

Comment thread cdk/test/stacks/agent.test.ts Outdated

isadeks added 2 commits September 1, 2026 17:57

Merge branch 'main' into fix/804-805-doctor-and-model-validation

15c97c2

Merge branch 'main' into fix/804-805-doctor-and-model-validation

cb9d459

This was referenced Sep 2, 2026

feat(orchestration): scope workflow model admission to the deployed geography #846

Open

feat(cli): read stack outputs in one call and derive repo show's default geography #847

Open

isadeks dismissed ayushtr-aws’s stale review via e6464b0 September 2, 2026 15:31

isadeks requested a review from scottschreckengaust September 2, 2026 17:52

scottschreckengaust requested changes Sep 2, 2026

View reviewed changes

isadeks requested a review from scottschreckengaust September 3, 2026 11:19

isadeks added 2 commits September 3, 2026 07:20

Merge branch 'main' into fix/804-805-doctor-and-model-validation

bed9f2c

Merge branch 'main' into fix/804-805-doctor-and-model-validation

9701592

isadeks requested a review from backgroundagents as a code owner September 4, 2026 16:29

isadeks added 5 commits September 8, 2026 13:49

Merge branch 'main' into fix/804-805-doctor-and-model-validation

7b812f5

Merge branch 'main' into fix/804-805-doctor-and-model-validation

2508996

Merge branch 'main' into fix/804-805-doctor-and-model-validation

5640d87

scottschreckengaust added the v1 Version 1 label Sep 14, 2026

scottschreckengaust approved these changes Sep 14, 2026

View reviewed changes

Comment thread cdk/test/constructs/blueprint.test.ts

Comment thread cli/src/platform-doctor.ts

Comment thread cli/src/model-id.ts

Comment thread agent/scripts/diagnostics/test_sdk_smoke.py

isadeks mentioned this pull request Sep 14, 2026

chore(cleanup): non-blocking review nits deferred from #806 — doctor comment/detail accuracy, Blueprint modelId validation, docs geography default #892

Open

9 tasks

isadeks added this pull request to the merge queue Sep 14, 2026

Merged via the queue into main with commit 7f3f3c0 Sep 14, 2026
11 checks passed

isadeks deleted the fix/804-805-doctor-and-model-validation branch September 14, 2026 19:46

Uh oh!

Conversation

isadeks commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What lands

The central fix

Defects found while verifying, and fixed here

Live verification

Three things a reviewer should know

One test-only commit worth a sentence

Uh oh!

This comment has been minimized.

This comment has been minimized.

isadeks commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review — third round

Blocking

B1 — doctor's new check fails a healthy stack, and the correct idiom is 94 lines above it

B2 — the new Blueprint REMOVE deletes an operator's --model pin on every deploy

B3 — the third compute substrate still has the exact divergence this PR removes

B4 — the headline fix is unguarded; so is the shipped geography

B5 — the new --model guard has input holes

B6 — the canonical model-configuration reference contradicts this PR

B7 — an eu/apac/jp/au/us-gov deployment has every model-pinning workflow rejected at admission

Non-blocking

Tests

Process

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ayushtr-aws left a comment

Choose a reason for hiding this comment

Verdict: Approve (with nits)

Vision alignment

Verification of prior blocking claims (re-checked against the worktree)

Blocking issues

Non-blocking suggestions / nits

Documentation

Tests & CI

Review agents run

Human heuristics

Uh oh!

Uh oh!

scottschreckengaust left a comment

Choose a reason for hiding this comment

Verdict: Request changes

Prior review: resolved vs still open

Vision alignment

Blocking issues

Non-blocking suggestions / nits

Documentation

Tests & CI

Review agents run

Human heuristics

Uh oh!

Uh oh!

Uh oh!

Uh oh!

isadeks commented Sep 2, 2026

Blocker 1 — the guide recommended a model the default deploy cannot invoke

Blocker 2 — the size test measured the wrong substrate

Fix-now list

Deferred with tracked issues

Uh oh!

scottschreckengaust left a comment

Choose a reason for hiding this comment

Verdict: Request changes

Prior blockers / open threads

Vision alignment

Blocking issues

Non-blocking suggestions / nits

Documentation

Tests & CI

Review agents run

Human heuristics

Uh oh!

Uh oh!

Uh oh!

isadeks commented Aug 27, 2026 •

edited

Loading

isadeks commented Aug 27, 2026 •

edited

Loading

B1 — `doctor`'s new check fails a healthy stack, and the correct idiom is 94 lines above it

B2 — the new Blueprint `REMOVE` deletes an operator's `--model` pin on every deploy

B5 — the new `--model` guard has input holes

B7 — an `eu`/`apac`/`jp`/`au`/`us-gov` deployment has every model-pinning workflow rejected at admission

Prior blockers — all verified fixed at `5640d879`