Skip to content

feat(cdk): single source of truth for invocable Bedrock models, context-overridable (#433)#434

Open
isadeks wants to merge 5 commits into
mainfrom
feat/433-configurable-bedrock-models
Open

feat(cdk): single source of truth for invocable Bedrock models, context-overridable (#433)#434
isadeks wants to merge 5 commits into
mainfrom
feat/433-configurable-bedrock-models

Conversation

@isadeks

@isadeks isadeks commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Lowers the friction of choosing which Claude models the agent can use, and fixes a latent drift bug — without loosening the per-model least-privilege scoping.

Before: the invocable-model set was hardcoded in two places that had to stay identical by hand — the AgentCore runtime grantInvoke blocks in stacks/agent.ts and BEDROCK_MODEL_IDS in constructs/ecs-agent-cluster.ts (whose own comment said "kept in sync with the AgentCore runtime grants in agent.ts"). Adding a model meant editing CDK in two spots and redeploying; a repo pinned to an ungranted model (e.g. Opus 4.8) 403s at invoke.

After: one source of truth, constructs/bedrock-models.ts:

  • DEFAULT_BEDROCK_MODEL_IDS — today's three (Sonnet 4.6, Opus 4, Haiku 4.5), unchanged default.
  • resolveBedrockModelIds(node) — returns the bedrockModels CDK context array when set, else the default. Both grant sites derive from it, so the AgentCore runtime and ECS task role can't drift.

Operators add a model via cdk.json context (or -c bedrockModels='[…]') + redeploy — no construct edits.

Preserves the security posture (does NOT revert hardening)

ecs-agent-cluster.ts deliberately scopes Bedrock to explicit foundation-model + inference-profile ARNs (it replaced a Resource: '*' wildcard, with a regression test). This PR keeps that — the shared list still produces explicit per-model ARNs, never a wildcard. Account-level Bedrock model access remains the outer authorization gate. Malformed context (non-array / empty / non-string entries) fails synth loudly rather than silently granting nothing.

Tests

  • bedrock-models.test.ts — resolver: default set, context override, and validation throws (non-array / empty / empty-string entry).
  • ecs-agent-cluster.test.ts — new test: a bedrockModels context override changes the granted ARNs and never widens to *; existing "scoped, no wildcard" hardening test retained.
  • Full mise //cdk:test: 122 suites / 2203 pass; mise //cdk:eslint clean.

Notes for reviewers

Fixes #433

…xt-overridable (#433)

Adding a model the runtime may invoke previously meant hand-editing two
parallel hardcoded lists (the AgentCore runtime grants in agent.ts and
BEDROCK_MODEL_IDS in ecs-agent-cluster.ts, "kept in sync by hand") and
redeploying — and a repo pinned to an ungranted model 403s at invoke.

Introduce constructs/bedrock-models.ts: DEFAULT_BEDROCK_MODEL_IDS (today's
three) + resolveBedrockModelIds(node), which returns the `bedrockModels` CDK
context array when set, else the default. Both grant sites now derive from this
one list, so the AgentCore and ECS backends can't drift. Operators add a model
via `cdk.json` context (or -c) + redeploy — no construct edits.

Per-model ARN scoping is preserved (NOT reverted to Resource:'*'); account-
level Bedrock model access remains the outer gate. Malformed context (non-array
/ empty / non-string entries) fails synth loudly.

Tests: resolver unit tests (default, override, validation throws); ECS test
asserting a context override changes the granted ARNs on both backends and
never widens to a wildcard. Existing "no wildcard" hardening test retained.
Full //cdk:test 2203 pass.

Fixes #433
@krokoko

krokoko commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Thanks for this — the refactor is clean and solves the real drift problem without loosening the per-model IAM scoping. A few notes from review:

Looks good

Suggestions (non-blocking)

  1. AgentStack test — Issue feat(cdk): make the runtime's invocable Bedrock model set stack-configurable (one list, not two hardcoded arrays) #433's acceptance criteria mention context-override propagation to both backends. The new ECS test covers one side; a matching AgentStack assertion (runtime execution role gets the override ARNs, defaults excluded) would close that loop.
  2. Context ID format — The resolver expects bare foundation-model IDs (anthropic.claude-opus-4-8), not us.-prefixed inference-profile IDs. Worth calling out in docs (docs(skills): audit deploy/submit-task/troubleshoot/status — stale claims, node PATH, onboard path #435?) since the issue text used the us. form — operators following that example would synth invalid ARNs (us.us.…).
  3. WORKFLOW_MODEL_ALLOWLIST — Still a third hardcoded list for workflow YAML admission. Doesn't block the repo onboard --model path (repo model_id isn't gated), but custom workflows pinning a newly added model would still fail at create-task. Might be worth a follow-up or a short comment in bedrock-models.ts.
  4. Stale commentworkflows.ts still references BEDROCK_MODEL_IDS in ecs-agent-cluster.ts; could point at bedrock-models.ts after merge.

CIinteg-smoke failed on missing AWS credentials in the runner (infra flake), unrelated to this diff. Build/security checks passed.

Happy to approve once the AgentStack test lands (or if you prefer to track that as a fast follow-up). Nice work on keeping the security posture intact.

…ix guard, allowlist note

Per @krokoko review on #434:
1. (blocking) AgentStack tests: assert the runtime execution role gets the
   default Bedrock model grants, and that a `bedrockModels` context override
   propagates to the runtime too (overridden model present, defaults absent,
   never `*`) — closes the both-backends acceptance criterion the ECS test
   only half-covered.
2. resolveBedrockModelIds now rejects region-prefixed IDs (us./eu./apac.) with
   a clear message — prevents the `us.us.anthropic.…` double-prefix footgun
   (both grant sites derive the inference-profile ARN by prefixing `us.`).
   JSDoc clarified to require bare foundation-model IDs; added a test.
3. workflows.ts: refreshed the stale `BEDROCK_MODEL_IDS` comment to point at
   bedrock-models.ts, and noted WORKFLOW_MODEL_ALLOWLIST is a separate
   hand-maintained list (repo onboard --model isn't gated by it; a custom
   workflow pinning a context-added model still needs it here) — consolidation
   tracked as a follow-up.

Full //cdk:test 2206 pass.
@isadeks

isadeks commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review! Addressed in the latest push (d6f2ee7):

  1. AgentStack test (blocking) — added two tests in agent.test.ts: the runtime execution role gets the default model grants, and a bedrockModels context override propagates to the runtime (overridden model present, defaults absent, never *). Together with the existing ECS test, both backends are now covered per feat(cdk): make the runtime's invocable Bedrock model set stack-configurable (one list, not two hardcoded arrays) #433's acceptance criteria.
  2. us.-prefix gotcha — turned it into a fail-fast: resolveBedrockModelIds now rejects region-prefixed IDs (us./eu./apac.) with a message pointing at the bare ID, since both grant sites derive the us.-profile ARN themselves (the us.us.… footgun you flagged). JSDoc clarified + test added. Good catch — the issue text's us. example would indeed have mis-synthed.
  3. WORKFLOW_MODEL_ALLOWLIST — left as-is for this PR (it gates workflow-YAML admission, not the repo onboard --model path), but added a comment noting it's a separate hand-maintained list and that a custom workflow pinning a context-added model would still need it here. Consolidating it with bedrock-models.ts is worth a follow-up — happy to file one.
  4. Stale commentworkflows.ts now points at bedrock-models.ts instead of the removed BEDROCK_MODEL_IDS.

Full //cdk:test is at 2206 passing. The integ-smoke red is the same OIDC-credentials infra flake. Ready for another look.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(cdk): make the runtime's invocable Bedrock model set stack-configurable (one list, not two hardcoded arrays)

2 participants