feat(cdk): single source of truth for invocable Bedrock models, context-overridable (#433)#434
Open
isadeks wants to merge 5 commits into
Open
feat(cdk): single source of truth for invocable Bedrock models, context-overridable (#433)#434isadeks wants to merge 5 commits into
isadeks wants to merge 5 commits into
Conversation
…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
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)
CI — 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.
Contributor
Author
|
Thanks for the thorough review! Addressed in the latest push (d6f2ee7):
Full |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
grantInvokeblocks instacks/agent.tsandBEDROCK_MODEL_IDSinconstructs/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 thebedrockModelsCDK 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.jsoncontext (or-c bedrockModels='[…]') + redeploy — no construct edits.Preserves the security posture (does NOT revert hardening)
ecs-agent-cluster.tsdeliberately scopes Bedrock to explicit foundation-model + inference-profile ARNs (it replaced aResource: '*'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: abedrockModelscontext override changes the granted ARNs and never widens to*; existing "scoped, no wildcard" hardening test retained.mise //cdk:test: 122 suites / 2203 pass;mise //cdk:eslintclean.Notes for reviewers
/onboard-reposkill (see docs(onboarding): X-Ray gate, arm64/binfmt, failed-create teardown, and mise papercuts block first-time setup #431/docs(onboarding): operator-path fixes across QUICK_START + /setup + /onboard-repo skills #432).Fixes #433