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

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 -c bedrockModels='[…]', which fails at synth. -c supplies a string, not an array. --context-file is worse: 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.

Two 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: ECS synthesizes at 992,111 bytes against the 1,000,000 ceiling, so the default substrate is 7,889 bytes — roughly one medium construct — from a failure that lands after synth succeeds and every asset is pushed, names no resource, and leaves stack status showing the previous deploy's success. The guard turns that into a build failure that names the budget.

Non-blocking findings from an earlier self-review are NOT tracked in issues. I had filed #820/#822/#823 without asking and closed them again — two had bodies citing symbols that exist only on this branch, and one had the wrong file path. The findings themselves are unaddressed and belong in a follow-up: the Blueprint field-removal no-op, the default model hand-copied across three languages, and the geo-admission scoping noted below. Nothing in this branch points at those issue numbers.

Suites: 4287 CDK, 797 CLI, 1755 agent. mise run build clean.

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.

…he auxiliary one `agentPlatformConfig.anthropicModel` was a REQUIRED prop that nothing read. The stack passed it, the interface declared it, the compiler was satisfied — and `TaskOrchestrator` never put it in the orchestrator's environment, so it went nowhere. `anthropic_model` was likewise missing from the cross-language `microvm_platform_config` contract, so even a carried value would have stopped at the Lambda instead of reaching the guest. Only the MicroVM substrate depends on that transport: the AgentCore runtime and the ECS task definitions inject the model into their own environments directly, which is why every existing synth assertion passed. MicroVM fell through to the `global.`-prefixed literal in `agent/src/config.py`. On a `global` deployment that is accidentally correct, which is why it went unnoticed; on any other geography the agent asks for a profile the IAM grant does not cover and the task dies at turn 0 with AccessDenied naming no model. Fixed at both layers — the orchestrator env block and the contract's `env_by_key`. The agent's install loop is generic over the contract, so no agent change was needed once the key existed. Two tests were WRONG rather than merely missing, and both are why this shipped: - `task-orchestrator.test.ts` was titled "injects the seven forwarded identifiers" while the interface declared eight, and its fixture omitted `anthropicModel` entirely. A required prop can be omitted in a test object literal without a compile error, so nothing objected. - The same fixture omission made my first attempt at an assertion vacuous: `expect(env.ANTHROPIC_MODEL).toBe(MAIN_PROFILE)` compared undefined to undefined and passed with the fix reverted. Both fixtures now use a NON-`global` geography (`us.`), because a `global.` fixture passes even when the value is dropped — the agent's fallback is `global.`-prefixed. Mutation-verified: reverting the orchestrator emit, and emitting the auxiliary model under the main name, each fail the suite. Removing the contract key fails two more. The agent-side wire-contract pin in `test_server.py` caught the change independently, which is the drift guard working. Also corrects a message defect in `checkBedrockInferenceProfile`, found by running `platform doctor` under a role deliberately missing bedrock:GetInferenceProfile and reading the output. One detail string served both branches, so a denial asserted "Either <model> has no profile in that geography … tasks would fail at turn 0" — a conclusion about the profile drawn from an error about the caller, and on that role the profile was fine. It now reports the permissions gap and names the missing IAM action. That check had no denial test at all; it has one now. Suites: 4286 CDK, 797 CLI, 1755 agent.

…'s 1 MB limit `--context compute_type=lambda-microvm` cannot be deployed: the template it synthesizes is over CloudFormation's hard 1 MB ceiling, so the deploy dies at changeset creation with `Template may not exceed 1000000 bytes in size.` Measured on `upstream/main` with no local changes — 1,012,186 bytes for MicroVM against 992,111 for ECS. Filed separately as the platform issue it is; this commit adds only the guard, because the interesting part is how late the failure lands. The error arrives AFTER synth succeeds and after every asset is built and pushed, it names no resource, and the stack's own status stays at whatever the previous deploy left it — so checking stack status instead of the deploy's exit code reads as success. ECS, the default substrate, is 7,889 bytes behind the same wall. The guard measures bytes the way the CDK CLI WRITES the template (`JSON.stringify(t, null, 2)`), which is how CloudFormation counts what it receives. This is the whole correctness of the test: compact serialization of the same template is ~700 KB while the uploaded file is ~1,010 KB — about 310 KB is indentation. The first version measured compact bytes, passed at a 950 KB budget with 45 KB of apparent headroom, and would have sat green through the very deploy failure it was written for. Verified by mutation in both directions: tightening the budget fires it, and reverting to compact bytes makes it pass again. Budget is 5% under the ceiling so it fires while there is still room to land the change that trips it. Raising the number is not the fix — the stack already has two nested stacks, and moving another self-contained area below a nested-stack boundary is what buys real headroom.

The previous comment led with `compute_type=lambda-microvm` synthesizing over the 1 MB ceiling, framed as a defect on `main`. That framing is wrong: the MicroVM substrate is still in development, so its template not fitting yet is expected in-progress state, not something to report. I had also filed it upstream without asking; that issue is closed. The guard's actual justification is the substrate deployments really use: ECS synthesizes at 992,111 bytes against the 1,000,000 ceiling — 7,889 bytes, roughly one medium construct. MicroVM is demoted to the parenthetical it should have been: how this was noticed, not why the guard exists. No behaviour change; the assertion and budget are untouched.

scottschreckengaust

Verdict: Request changes

Excellent intent and an unusually well-reasoned, well-tested change — but two hard blockers plus one stale-on-arrival comment need to land before merge. All three are surgical fixes; the design itself is sound.

Reviewed as a principal architect, with the pr-review-toolkit agents (code-reviewer, silent-failure-hunter, pr-test-analyzer, comment-analyzer), /security-review, and my own empirical bundle testing. type-design-analyzer omitted — the diff introduces no new exported type/interface/class/union.

Vision alignment

Fits the vision (VISION.md — bounded blast radius & reviewable outcomes). It closes a fire-and-forget failure mode where a non-default bedrockGeoRegion granted one geography's inference profiles while the agent invoked another's, failing every task at turn 0 with AccessDenied. Injecting both ANTHROPIC_MODEL and ANTHROPIC_DEFAULT_HAIKU_MODEL from one resolved geography across all three substrates, surfacing BedrockGeoRegion/BedrockModelIds as CfnOutput contracts, and rejecting an ungranted --model before submission all move failures left, out of the unattended path. No tenet traded; IAM stays per-model ARN-scoped.

Blocking issues

1. cdk/src/handlers/shared/workflows.ts:37 — new import pulls all of aws-cdk-lib into 7 runtime Lambda bundles.

The added import { BEDROCK_GEO_REGIONS, DEFAULT_BEDROCK_MODEL_IDS } from '../../constructs/bedrock-models' reaches bedrock-models.ts:130, where BEDROCK_GEO_REGIONS = Object.values(CrossRegionInferenceProfileRegion) is a runtime use of a @aws-cdk/aws-bedrock-alpha enum whose module top-level require("aws-cdk-lib") esbuild cannot tree-shake.

Empirically verified (isolated esbuild bundle, node22/cjs):

entry	main	this PR
`workflows.ts` alone	6.8 KB, 0 `aws-cdk-lib` refs	55 MB, 9,730 refs, 120 alpha refs
`orchestrate-task.ts`	—	64 MB, 9,730 refs

Blast radius (all import workflows.ts directly or via create-task-core.ts): OrchestratorFn, CreateTaskFn, WebhookCreateTaskFn, Slack/Linear/Jira webhook processors, plus the reconciler Lambdas. CI does not catch it — there is no bundle-size gate and build (agentcore) passes.

Risk: tens-of-MB cold-start and deploy-artifact bloat on the hottest control-plane Lambdas, and a layering inversion — the runtime handler layer now depends on the CDK construct + alpha layer, the exact coupling blueprint.ts:25-28 deliberately avoids by importing constants.json directly.

Fix: extract the two dependency-free values into a plain module (e.g. cdk/src/handlers/shared/bedrock-model-constants.ts exporting DEFAULT_BEDROCK_MODEL_IDS and BEDROCK_GEO_REGIONS as a literal readonly string[]); have both bedrock-models.ts and workflows.ts import from it; keep the CrossRegionInferenceProfileRegion enum confined to the construct layer, with a synth-time test asserting the literal equals Object.values(CrossRegionInferenceProfileRegion) so they cannot drift.

2. cli/src/platform-doctor.ts (~L555-561) — binary error classifier misroutes transient failures to a harmful remediation.

checkGrantedModelProfiles/checkBedrockInferenceProfile classify as const status = accessDenied ? 'warn' : 'fail'. Every error that is not AccessDenied — throttling, transient network, a GetInferenceProfile/GetFoundationModel 5xx, expired creds, a Region without the endpoint — becomes a hard fail whose remediation steers the operator to remove the model from the grant set.

Risk: a correctly-granted, correctly-configured model is reported as broken on a transient blip, and the suggested fix actively degrades a working deployment.

Fix: three-way classify — AccessDenied → warn (as today); a definitive not-found/invalid-id → fail with the remove/rename remedy; everything else (transient/unknown) → warn/unknown with "couldn't verify, retry" rather than "remove the model."

3. cdk/src/constructs/bedrock-models.ts:210-214 — comment falsified by this same PR (doc drift is blocking on this repo).

The haikuInferenceProfileId doc asserts the ECS container "is deliberately NOT in that list — it never carried ANTHROPIC_DEFAULT_HAIKU_MODEL, so an ECS agent falls back to agent/src/config.py's own us.-prefixed default." This PR's own ecs-agent-cluster.ts:389-391 now sets both model env vars on ECS, and config.py:574's fallback is now global.-prefixed, not us.. A maintainer reading this will believe the opposite of what the code does.

Fix: delete L210-214 and fold ECS in as a third geo-derived delivery site, or at minimum drop the "us.-prefixed default" and "pre-existing gap left alone" clauses.

Non-blocking suggestions / nits

cdk/src/constructs/blueprint.ts:219-221 — class-doc says Update "REMOVEs the per-repo overrides it no longer declares," but clearedOverrideFields() returns [] (L515) and only empty asset refs are removed — contradicting the correct comment at L496-512 in the same file. Reword to "REMOVEs only per-repo asset refs; other dropped overrides are intentionally carried forward — see clearedOverrideFields."
cli/src/model-id.ts:30 — "Listed longest-first" is inaccurate (apac (4) sits after us/eu (2)). The substantive claim (ordering is irrelevant because geoPrefixOf anchors on a literal .) is correct; just drop the "longest-first" characterization or actually sort.
cdk/src/handlers/shared/workflows.ts:85-87 — admission now accepts all 7 geo prefixes regardless of deployed geography; a workflow pinning an undeployed geo passes admission and fails at turn 0 with AccessDenied — the same class of failure this PR otherwise pre-empts. Acknowledged as "Phase 4"; consider scoping to the resolved geography when this list becomes context-sourced.
agent/scripts/diagnostics/test_sdk_smoke.py — the exit-code fix is a real improvement; minor residual: an outcome matching neither branch (e.g. assistant>0 but result==0) prints no verdict line yet exits 1. Cheap to add an explicit "indeterminate" line.
cdk/src/constructs/bedrock-models.ts — PLATFORM_DEFAULT_AUX_MODEL_ID duplicates the DEFAULT_HAIKU_MODEL_ID value rather than aliasing it, and two helpers (haikuInferenceProfileId(geo) vs inferenceProfileId(geo, PLATFORM_DEFAULT_AUX_MODEL_ID)) now produce the same haiku profile id from different call sites. Consolidate to one path so they cannot diverge.
Issue tracking — blueprint.ts defers the clear-field gap to a "tracked separately" issue, and the PR body says #822/#823 were "filed separately, not folded in"; those trackers appear closed as completed. Confirm the deferred work is actually done or re-open, so the code's pointer is not dangling.

Documentation

Paired docs/guides/ + docs/design/ edits present and the Starlight mirror under docs/src/content/docs/ is in sync (CI "Fail build on mutation" passed). contracts/constants.json correctly gains anthropic_model → ANTHROPIC_MODEL (consumed generically by server.py, correctly excluded from required). The documentation defects that matter are the two in-code comment inaccuracies above (Blocking #3 and the blueprint nit) — both stale relative to this PR's own code.

Tests & CI

Coverage is strong: per-geography env-injection sweeps across both substrates, AccessDenied name+message classification, wildcard/pattern rejection, ungranted-model rejection, empty-but-truthy grant-list guard, and allowlist parity — all asserting the specific historical bug they guard. CI fully green.

Gaps worth closing:

Elevated: no producer-side hasOutput('BedrockModelIds', …) test. This output is the linchpin both new consumers depend on; if a refactor drops/renames it, both the CLI grant check and doctor's checkGrantedModelProfiles silently become no-ops and nothing fails. Add it alongside the existing BedrockGeoRegion output test.
assertModelIdUsable double-prefix (global.us.…) and empty-bare (global.) branches are untested.
The .trim() guard in both model-id.ts and repo.ts is untested.

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

Review agents run

code-reviewer — Blocking #1 (bundle regression) + geo-admission nit.
silent-failure-hunter — Blocking #2 (doctor error classifier).
pr-test-analyzer — 4 coverage gaps (above).
comment-analyzer — Blocking #3 + blueprint/model-id comment nits.
/security-review — clean; no exploitable findings (DynamoDB UpdateExpressions code-derived; model-id parsing is string prefix logic; IAM stays ARN-scoped).
type-design-analyzer — omitted: no new exported type/interface/class/union in the diff.

Human heuristics

Proportionality — Pass. cli/src/model-id.ts is a focused validator, not over-built. One smell: the duplicated haiku-id path.
Coherence — Concern. The geo/model concept is spelled twice (PLATFORM_DEFAULT_AUX_MODEL_ID vs DEFAULT_HAIKU_MODEL_ID; haikuInferenceProfileId vs inferenceProfileId(…, AUX)) — the drift risk this PR otherwise fights.
Clarity — Concern. Two load-bearing comments now describe the opposite of the code (bedrock-models.ts:210-214, blueprint.ts:219-221).
Appropriateness — Pass with one caveat: the doctor check was validated against the AccessDenied path but encodes "any error ⇒ remove the model" (Blocking #2).

Bottom line: approve-worthy design; holding only for the two blockers (bundle coupling, doctor classifier) and the stale self-contradictory comment. All three are small, surgical fixes.

…ising model removal on transient errors All three blockers reproduced before fixing. **1. `aws-cdk-lib` in seven runtime Lambda bundles.** `handlers/shared/workflows.ts` imported two constants from `constructs/bedrock-models.ts`, whose geography list was `Object.values(CrossRegionInferenceProfileRegion)` — a runtime read of a module that top-level `require`s the CDK, which esbuild cannot tree-shake. Measured: `workflows.ts` alone went from 6.8 KB and zero `aws-cdk-lib` references to **57 MB and 9,679**, on the orchestrator, create-task, webhook create-task, the three channel webhook processors and the reconcilers. `aws-cdk-lib` is in no construct's `externalModules`, so nothing downstream stripped it; deployed artifacts are 42-43 MB. The values now live in a dependency-free `handlers/shared/bedrock-model-constants.ts` that the construct layer re-exports, so the dependency direction is runtime → nothing rather than runtime → CDK. `workflows.ts` bundles at 6,479 bytes with zero CDK references, below the pre-change baseline. The enum read stays construct-side and un-exported. Two guards, because a measurement is not a test: a real esbuild bundling test that fails on any `aws-cdk-lib` reference in a runtime entry (verified — pointing the import back at the construct module fails it with 10,099 references), and a parity test asserting the extracted literal equals `Object.values(CrossRegionInferenceProfileRegion)` so a CDK release adding a geography fails loudly instead of leaving the literal short. **2. Doctor told operators to delete working models.** Both Bedrock checks classified `accessDenied ? 'warn' : 'fail'`, and the fail remedy is "remove it from the bedrockModels context". Every other error — throttling, 5xx, timeout, expired credentials — took that branch, so a transient blip produced destructive advice about a correctly-configured model. Now three-way: denial warns, a definitive not-found fails with the removal remedy, and anything else warns as unverified and says explicitly not to remove anything. Mutation-caught: my first pass fixed only the granted-set check and left the single-model one reverting green. **3. A comment falsified by this PR.** `bedrock-models.ts` claimed the ECS container "never carried `ANTHROPIC_DEFAULT_HAIKU_MODEL`" and fell back to a `us.`-prefixed default. This PR sets both vars on ECS, and that fallback is now `global.`-prefixed. Rewritten as the three delivery sites that actually exist. Non-blocking, all verified as real first: - `blueprint.ts` class-doc said Update REMOVEs dropped overrides while `clearedOverrideFields` returns `[]`; now says asset refs only, and why. - `model-id.ts` claimed "longest-first" ordering; `apac` (4) follows `us` (2). Dropped the claim, kept the behavioural test. - `PLATFORM_DEFAULT_AUX_MODEL_ID` is now an alias of `DEFAULT_HAIKU_MODEL_ID` rather than a second copy, and `haikuInferenceProfileId` is gone — one helper, so the two paths cannot diverge. - The SDK smoke test could exit 1 having printed no verdict (`assistant>0, result==0`); that outcome now prints INDETERMINATE. Coverage gaps closed: the `BedrockModelIds` producer-side output test (both consumers skip themselves when it is absent, so a rename would silently disable two guards), and the double-prefix, empty-bare, and `.trim()` branches — verified live at the terminal earlier but never pinned. Suites: 4345 CDK, 807 CLI, 1755 agent.

isadeks · 2026-08-31T16:37:19Z

Thanks — this found two things I had no coverage for and one comment I falsified in this same PR. All three blockers reproduced before fixing; addressed in c0ed2eed.

Blocking 1 — CDK in runtime bundles

Confirmed, and it is a regression this PR introduced. Reproduced your numbers and added the baseline comparison:

`workflows.ts`	bytes	cdk refs
on `main`	6,774	0
before fix	57 MB	9,679
after fix	6,479	0

Two details worth adding. aws-cdk-lib is in no construct's externalModules (only @aws-sdk/*), so nothing downstream strips it — deployed artifacts are 42–43 MB. And orchestrate-task.ts was already 64 MB on main via constructs/attachments-bucket.ts; same class, pre-existing, not touched here.

Fixed as you suggested: values moved to a dependency-free handlers/shared/bedrock-model-constants.ts, construct layer re-exports them, the Object.values() enum read stays construct-side and un-exported. Direction is now runtime → nothing.

I added a real bundling test rather than trusting the measurement, since you noted CI has no gate: it runs esbuild and fails on any aws-cdk-lib reference in a runtime entry. Verified by pointing the import back at the construct module — fails with 10,099 references, in 3.3s. Plus the parity test you asked for, so a CDK release adding a geography fails loudly instead of leaving the literal one short.

Blocking 2 — the destructive remedy

Confirmed. Any non-AccessDenied error landed in missing[], whose remedy is "Remove it from the bedrockModels context" — so a throttle told the operator to delete a healthy model. Now three-way, as you proposed: denial → warn, definitive not-found → fail with the removal remedy, everything else → warn that says explicitly not to remove anything.

Worth flagging that my first pass fixed only checkGrantedModelProfiles. Mutation testing caught it: reverting checkBedrockInferenceProfile to the binary form left all 49 tests green. Both are fixed and both mutations now fail.

Blocking 3 — the falsified comment

Confirmed and embarrassing: the comment asserted ECS "never carried ANTHROPIC_DEFAULT_HAIKU_MODEL" and fell back to a us.-prefixed default, while ecs-agent-cluster.ts:389 in this PR sets both vars and config.py:574 is now global.-prefixed. Rewritten as the three delivery sites that actually exist.

Non-blocking — all verified before changing

Each of these I checked rather than taking on faith; all five were right.

blueprint.ts class-doc — contradicted return []. Now says asset refs only, with the reason.
"longest-first" — false: apac (4) follows us (2). Dropped the claim, kept the behavioural us-gov test.
Duplicated haiku path — PLATFORM_DEFAULT_AUX_MODEL_ID is now an alias of DEFAULT_HAIKU_MODEL_ID, and haikuInferenceProfileId is deleted. One helper, so the two cannot diverge.
Smoke test silent branch — reachable at assistant>0, result==0; now prints INDETERMINATE.
Dangling trackers — you were right and the reason is worse than dangling: I filed Removing a field from a Blueprint is a silent no-op for all SET-only RepoTable columns #820/Default model + geography list are hand-copied across three languages, and repo show reports a stale default off a non-global deploy #822/SDK smoke test counts messages instead of reading ResultMessage.is_error, so a throttled run prints PASS #823 without asking, then closed them because two cited symbols that exist only on this branch and one had the wrong path. The PR body no longer references them and now says the findings are unaddressed follow-up work.

Coverage gaps — all closed: the BedrockModelIds producer-side output test (your "elevated" one, and you were right that both consumers skip themselves when it is absent, so a rename would silently disable two guards), plus the double-prefix, empty-bare and .trim() branches. I had verified those three live at the terminal and never pinned them.

One I am leaving

Geo-scoped admission. WORKFLOW_MODEL_ALLOWLIST still accepts all seven prefixes regardless of deployed geography. Scoping it needs the resolved geography inside a runtime handler, which is exactly the construct-layer import blocker 1 just removed — so doing it now would reintroduce the coupling in a different file. It wants the context-sourced list you mention; leaving it out rather than trading one blocker for another.

Suites: 4345 CDK, 807 CLI, 1755 agent.

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.

scottschreckengaust · 2026-09-02T19:12:22Z

-    // The CLI reads this to refuse onboarding a repo as compute_type=ecs on a
-    // stack that never provisioned the ECS substrate.
-    template.hasOutput('ComputeSubstrate', { Value: 'agentcore' });
+  test('outputs BedrockGeoRegion so a client can check the profile it will invoke', () => {


Blocking (1) — six pre-existing regression tests deleted here, only one relocated.

git diff c2d6dc0d..HEAD on this file removes 138 lines from this describe. The commit message documents moving stays inside a deployable template budget into the ECS describe (correct, and it fixes my prior blocker 2) — but the same hunk took six unrelated tests with it. Verified with git grep at origin/main and HEAD over all of cdk/:

creates exactly 21 DynamoDB tables — no replacement; the only stack-wide table-drift guard in the repo

creates TaskApprovalsTable with user_id-status-index GSI — construct-level test survives, stack-level assertion gone

outputs TaskApprovalsTableName — no replacement; no other reference to that output in cdk/test/

the orchestrator carries the platform_config transport env on EVERY compute type — no replacement

the forwarded identifiers are the SAME stack values the AgentCore runtime gets — no replacement

outputs ComputeSubstrate=agentcore on the default (no-gate) deploy — no replacement; only the ecs (:952) and lambda-microvm (:1046) variants survive, so the default path the CLI onboarding gate reads is now unguarded

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

Deleting a test never turns CI red, which is why all checks still pass.

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

scottschreckengaust · 2026-09-02T19:12:22Z

+      // The lambda-microvm `platform_config` block below derives the same two values
+      // from the same geography. runner.py re-sets both at spawn time; a per-repo
+      // `model_id` still overrides.
+      ANTHROPIC_MODEL: inferenceProfileId(bedrockGeoRegion, PLATFORM_DEFAULT_MODEL_ID),


Blocking (2) — the platform default is never checked against the deploy's own grant list, and all three new doctor checks pass when it is not.

ANTHROPIC_MODEL is injected here (and at ecs-agent-cluster.ts:389, and into the MicroVM platform_config at :1108) from the hardcoded PLATFORM_DEFAULT_MODEL_ID. resolveBedrockModelIds (bedrock-models.ts:152-213) validates array shape, geo prefixes and wildcards — but never that the platform defaults are members of the resolved list.

cdk deploy -c bedrockModels='["anthropic.claude-sonnet-4-6"]' — a documented, code-change-free override (onboard-repo/SKILL.md:136, DEVELOPER_GUIDE.md:125) — therefore synthesizes cleanly with IAM scoped to Sonnet profiles only while every substrate is told to invoke 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 cannot see it: bedrock_model/bedrock_inference_profile probe the CLI's own PLATFORM_REPO_DEFAULTS.model_id (which resolves fine), and bedrock_granted_model_profiles only asks whether each granted profile exists — never whether the model the agent is told to call is granted. The one existing guard (DEFAULT_BEDROCK_MODEL_IDS covers the agent runtime default) tests the constant list, not a resolved override.

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

Fix: throw at synth — in resolveBedrockModelIds or once here — 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, plus a test with a narrowing context.

scottschreckengaust · 2026-09-02T19:12:22Z

+
+Granting a **new** model is a deploy-time change, not a construct edit: the list is
+overridable via CDK context. `bedrockModels` takes an **array**, and that rules out the
+command-line form — `-c` supplies each value as a string, so


Blocking (3a) — this contradicts the resolver and the CLI remedy added in this same PR.

-c bedrockModels='["anthropic.claude-opus-5"]' is not rejected at synth: cdk/src/constructs/bedrock-models.ts:203-212 (added by this PR) JSON-parses the string form specifically "so both behave identically", the resolver's own JSDoc documents -c bedrockModels='["anthropic.claude-opus-4-8", …]' as the way to do it, this PR adds a test named parses a JSON-string override (the -c key=value CLI form), and this PR's new CliError at cli/src/model-id.ts:129 tells the operator verbatim: "Add it with -c bedrockModels='[…]' and redeploy".

So an operator (or the onboard-repo agent following this skill) who hits that 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 silently un-grants the platform default if an entry is omitted — see Blocking 2. The --context-file clause in the same sentence is also unverified.

Fix: drop "that rules out the command-line form … has no effect on it" and state that the -c form works (the resolver parses the JSON string -c delivers), with cdk.json context as the option for a persistent override. Keep the "this REPLACES the default list" warning on the cdk.json example — it is the right warning.

scottschreckengaust · 2026-09-02T19:12:22Z

- **Per repo:** Blueprint `agent.modelId` (e.g. `us.anthropic.claude-sonnet-4-6`) — no code change, no agent redeploy
+- **Per repo:** Blueprint `agent.modelId` — no code change, no agent redeploy. 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.
 - **Per task:** `model_id` in the task payload
 - **Platform-wide:** the `bedrockModels` context plus the layer-2 call sites above


Blocking (3b) — this recipe is a no-op after this PR, in the section the file calls canonical.

"the bedrockModels context plus the layer-2 call sites above" points at layer 2's Where cell, which names only agent/src/config.py:563 and agent/src/models.py:157. After this PR the stack injects ANTHROPIC_MODEL from PLATFORM_DEFAULT_MODEL_ID on all three substrates (agent.ts:535, ecs-agent-cluster.ts:389, agent.ts:1108), so the Python literals are unreachable on any deployed task. An operator following this makes a change with zero effect and believes the platform default moved — while every task still runs Opus 5, or fails at turn 0 if they also narrowed the grant list.

model-default-docs-parity.test.ts cannot catch it: it compares the documented value to the Python literal, not the ownership claim.

Fix: name PLATFORM_DEFAULT_MODEL_ID (cdk/src/handlers/shared/bedrock-model-constants.ts) as the platform-wide edit point here, and update layer 2's Where cell plus the ANTHROPIC_MODEL Who sets it row (:135, which still credits only the ECS strategy and your shell) to name the CDK stack as primary with the Python literals as the no-env fallback. Regenerate the mirror — developer-guide/Model-configuration.md carries the identical rows.

scottschreckengaust · 2026-09-02T19:12:22Z

+    // and reports a healthy stack as broken.
+    const errorName = err instanceof Error ? err.name : '';
+    const both = `${errorName} ${message}`;
+    const accessDenied = /AccessDenied|Unauthorized|not authorized/i.test(both);


Major — the commit message's claim about this refactor is not true in the code. It says "All four checks now share one classifyProbeFailure helper … which removes the third and fourth copies of that regex rather than adding them." Only checkLambdaMicrovmAvailability (:401) and checkBedrockModel (:442) call it. This probe re-derives accessDenied (:516) and definitivelyAbsent (:523) inline, and checkGrantedModelProfiles (:610-618) does it a third time — so there are now three copies of both patterns, not one.

They coincide today and no test asserts they do. The next correction to the classifier (adding ThrottlingException/ExpiredToken, or narrowing the very broad ValidationException|not found arm, which also fires for a malformed identifier rather than a missing profile) will be applied to the helper and silently miss the two probes whose fail remedy is the destructive one — "Remove it from the bedrockModels context". That is the advice the three-bucket split exists to gate.

Fix: have both probes call classifyProbeFailure(err) and switch on the returned ProbeFailure, using statusFor() for the status and keeping only the per-site detail strings local; checkGrantedModelProfiles can bucket per model into a Record<ProbeFailure, string[]>.

scottschreckengaust · 2026-09-02T19:12:23Z

    // runtime. ``ecs`` implies the AgentCore runtime is ALSO available (the ECS
    // gate is additive), so an agentcore repo works on either substrate — and the
    // same holds for ``lambda-microvm`` (ADR-021).
+    // Both outputs are consumed by `platform doctor` and `repo onboard --model` to


N3 from my last review, partly regressed. The BedrockModelIds/BedrockGeoRegion ordering is fixed — thank you — but the ComputeSubstrate paragraph above (:980-986, pre-existing) is now worse off than before: this PR inserted BedrockModelIds (:989) and BedrockGeoRegion (:995) between it and its own output, which is declared at :1002 with no comment at all. On origin/main that paragraph sat immediately above new CfnOutput(this, 'ComputeSubstrate', …).

As it reads now, the substrate/ADR-021 rationale documents BedrockModelIds, and this line's "Both outputs are consumed by platform doctor and repo onboard --model" has no antecedent at the point it is read — the reader has just been told about compute substrates, and the second output it means is declared six lines below.

Fix: move :980-986 back down to immediately above the ComputeSubstrate output, and leave this two-line note above BedrockModelIds/BedrockGeoRegion naming them explicitly.

scottschreckengaust · 2026-09-02T19:12:23Z

+ * (`platform-doctor` builds a REGEX from a similar list, where alternation order
+ * does matter more visibly — same reasoning recorded there.)
+ */
+export const BEDROCK_GEO_PREFIXES = [


Major (drift risk) — this is the third hand-maintained copy of the geography list, and it is the only one with no forcing function.

cdk/src/handlers/shared/bedrock-model-constants.ts:121 is guarded against CrossRegionInferenceProfileRegion (cdk/test/constructs/bedrock-models.test.ts:60-67 asserts set and length parity) precisely because a stale copy caused the bug this PR fixes. This copy has no equivalent: cli/test/model-id.test.ts:152 "recognizes every geography the CDK models" loops over BEDROCK_GEO_PREFIXES itself, so it can never fail, and cli/test/platform-doctor.test.ts:217 restates the same literal again.

After a CDK release adds a geography, the CDK literal is forced to update and this one silently lags. On a -c bedrockGeoRegion=<new> deploy: geoPrefixOf('<new>.anthropic.claude-opus-5') returns undefined, so assertModelIdUsable rejects a correctly-formed granted model as "a bare foundation-model id" and its suggestion is <new>.<new>.anthropic.… — rejected the same way, an unescapable loop that blocks onboarding; and GEO_PREFIX_RE (platform-doctor.ts:48, built from this array) stops stripping, so GetFoundationModel is handed a profile id, classifyProbeFailure calls it absent, and doctor fails a healthy stack. That is the exact failure this file's own header describes ("Two copies is how this broke").

Fix: promote the list into contracts/constants.json (the repo already validates that both ways via check-constants-sync.ts + cli/test/constants-parity.test.ts), or add a cross-package parity test that fs-reads bedrock-model-constants.ts and asserts set equality — the pattern model-default-docs-parity.test.ts already uses for agent/src/models.py. Also derive the GEOS array in cli/test/platform-doctor.test.ts from this constant instead of restating it.

scottschreckengaust · 2026-09-02T19:12:23Z

-| 3 | **Auxiliary / fast model** | The small model Claude Code uses for auxiliary work (WebFetch page summarization, the pre-flight safety check). | Stack env `ANTHROPIC_DEFAULT_HAIKU_MODEL` (`cdk/src/stacks/agent.ts` (the runtime environment block)); agent-side fallback at `agent/src/config.py:569` | Prefixed (`us.anthropic.…`) |
-| 4 | **Per-repo override** | One repository's model, with no agent redeploy. | Blueprint `agent.modelId` (`cdk/src/constructs/blueprint.ts`, `BlueprintProps.agent.modelId`) → RepoTable `model_id` (`cdk/src/handlers/shared/repo-config.ts:37`) → ECS injects `ANTHROPIC_MODEL` (`cdk/src/handlers/shared/strategies/ecs-strategy.ts:217`) | Prefixed (`us.anthropic.…`) |
-| 5 | **Per-task / local** | One task's model. Payload `model_id` is aliased to `anthropic_model` (`agent/src/pipeline.py`, `_PAYLOAD_KEY_ALIASES`); local batch runs read `ANTHROPIC_MODEL` from the shell via `agent/run.sh`. | Task payload `model_id`; shell `ANTHROPIC_MODEL` | Prefixed (`us.anthropic.…`) |
+| 1 | **IAM invoke allowlist** | Which models the agent's roles may invoke at all, and in which geography. The outer gate — everything below fails without it. | `DEFAULT_BEDROCK_MODEL_IDS` (`cdk/src/constructs/bedrock-models.ts`); override with CDK context `bedrockModels`. Geography via context `bedrockGeoRegion` (default `us`) | **Bare** (`anthropic.claude-…`) |


Nit (clarity) — "Geography via context bedrockGeoRegion (default us)" is the one phrasing this PR did not bring in line with the rest. Every other artefact you touched uses the precise form: bedrock-models.ts:140 ("global in the shipped cdk.json, us absent any context") and the BedrockGeoRegion CfnOutput description (""global" in the shipped cdk.json, "us" if no context is supplied at all").

An operator reading the row in the section this file calls canonical concludes their deploy grants us. profiles and pins us.<model> per repo. On the CLI path the new --model guard catches it; via a Blueprint agent.modelId it is written to the RepoTable unvalidated (blueprint.ts:319) and fails at turn 0 — the failure blocker 1 of my last review was fixed to prevent three lines below at :219. Use the CfnOutput wording here and regenerate the mirror.

scottschreckengaust · 2026-09-02T19:12:23Z

- * the enum, so a future CDK release that adds a geography widens both at once
- * instead of leaving one of them silently behind.
+ * The geographies `@aws-cdk/aws-bedrock-alpha` actually models, as ENUM members —
+ * the allow-list `resolveBedrockGeoRegion` validates against and the source of the


Nit — this names the wrong consumer. resolveBedrockGeoRegion validates against GEO_REGIONS (:121, and it builds its error message from it at :124) — the dependency-free literal re-exported at the top of the file. GEO_REGION_ENUM_VALUES is referenced exactly once, at :89, feeding GEO_ALTERNATION → GEO_PREFIX_RE.

So a maintainer widening or narrowing the validated geography set edits the constant this comment names and changes only the prefix-rejection regex, leaving synth-time validation of a security-relevant allow-list untouched. The second half of the sentence ("the source of the prefix rejection in resolveBedrockModelIds") is correct, which makes the wrong half more convincing.

Suggested: "…the source of the prefix rejection in resolveBedrockModelIds (via GEO_ALTERNATION → GEO_PREFIX_RE). Not the list resolveBedrockGeoRegion validates against — that reads the dependency-free BEDROCK_GEO_REGIONS; the parity test keeps the two identical."

scottschreckengaust · 2026-09-02T19:12:23Z

+ * `geoPrefixOf` matches `<geo>` followed by a literal `.`, so `us.` cannot match
+ * `us-gov.…` — the dot fails against the hyphen. The order below is arbitrary (it is
+ * NOT sorted by length: `apac` follows `us`), and a test asserts the `us-gov` case
+ * behaviourally so a future re-sort cannot be mistaken for load-bearing.


Nit — "platform-doctor builds a REGEX from a similar list … same reasoning recorded there" is wrong on both halves. It is the same list: platform-doctor.ts:48 builds GEO_PREFIX_RE from BEDROCK_GEO_PREFIXES imported from this file, and its comment says the opposite of "similar" — "Built from the ONE geography list (model-id.ts) rather than a second copy. Two copies is how this broke." No ordering reasoning is recorded there. A maintainer chasing the cross-reference finds nothing and may conclude a second list needs syncing.

The order claim is also moot there: GEO_PREFIX_RE anchors a literal \. after the alternation, so a us-before-us-gov order is harmless in the regex too.

Suggested: "(platform-doctor builds GEO_PREFIX_RE from this same array; order is immaterial there too, because the pattern anchors a literal . after the alternation.)"

Separately, at :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 — the check is skipped only when BedrockModelIds is absent — is already stated in the next sentence and at :105.

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

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

Open

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

isadeks added 2 commits August 31, 2026 09:14

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

e714ee8

scottschreckengaust requested changes Aug 31, 2026

View reviewed changes

isadeks requested a review from scottschreckengaust August 31, 2026 16:57

github-actions Bot mentioned this pull request Sep 1, 2026

Monthly PR metrics report #840

Closed

isadeks added 2 commits September 1, 2026 09:18

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

feec350

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

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

Two things a reviewer should know

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.

This comment has been minimized.

This comment has been minimized.

scottschreckengaust left a comment

Choose a reason for hiding this comment

Verdict: Request changes

Vision alignment

Blocking issues

Non-blocking suggestions / nits

Documentation

Tests & CI

Review agents run

Human heuristics

Uh oh!

isadeks commented Aug 31, 2026

Blocking 1 — CDK in runtime bundles

Blocking 2 — the destructive remedy

Blocking 3 — the falsified comment

Non-blocking — all verified before changing

One I am leaving

Uh oh!

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

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