Skip to content

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

Merged
krokoko merged 6 commits into
mainfrom
feat/433-configurable-bedrock-models
Jun 30, 2026
Merged

feat(cdk): single source of truth for invocable Bedrock models, context-overridable (#433)#434
krokoko merged 6 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.

@krokoko krokoko added this pull request to the merge queue Jun 30, 2026
Merged via the queue into main with commit 27d7959 Jun 30, 2026
7 of 8 checks passed
@krokoko krokoko deleted the feat/433-configurable-bedrock-models branch June 30, 2026 20:36
krokoko added a commit to ClintEastman02/sample-autonomous-cloud-coding-agents that referenced this pull request Jun 30, 2026
…operator FinOps guidance (aws-samples#215) (aws-samples#521)

* docs(design): Bedrock cost attribution design (aws-samples#215)

Design for per-user/per-repo Bedrock spend attribution. Key finding:
Bedrock is invoked by the Claude Code CLI subprocess, not the agent's
boto3, so both tracks (IAM session tags + request metadata) are wired
via Claude Code config (awsCredentialExport, ANTHROPIC_CUSTOM_HEADERS)
and a new BedrockInvokeRole — not by extending aws_session.py.

Refs aws-samples#215

* feat(cost): Bedrock cost attribution via session tags + request metadata (aws-samples#215)

Attribute Bedrock model-inference spend per user/repo. Bedrock is invoked
by the Claude Code subprocess (CLAUDE_CODE_USE_BEDROCK=1), so attribution is
wired through Claude Code's config, not the agent's boto3.

Track 1 — IAM session-tag chargeback (CUR 2.0 / Cost Explorer):
- Grant bedrock:InvokeModel* on the existing AgentSessionRole (reuse, not a
  new role) via grantInvoke, mirroring the compute-role grant exactly so
  cross-region profiles never AccessDenied. Compute role keeps its grant.
- bedrock_creds_helper.py assumes the SessionRole with {user_id,repo,task_id}
  STS tags and emits creds JSON for Claude Code's awsCredentialExport, which
  refreshes before the 1h role-chaining cap. Fails OPEN to ambient creds
  (billing control, not isolation). awsCredentialExport lives in root-owned
  /etc/claude-code/managed-settings.json so the untrusted repo can't override
  it (RCE boundary).

Track 2 — per-call forensics (model-invocation logs):
- Set X-Amzn-Bedrock-Request-Metadata via ANTHROPIC_CUSTOM_HEADERS on the
  subprocess env (one container = one task, so static-per-process is per-task;
  process-env so the repo can't alter it). SigV4 signed-headers behavior to be
  validated live (AC#3 documented-blocker path).

Track 3 — operator guide COST_ATTRIBUTION.md + cross-links, plus a prominent
warning that in-app cost_usd is a client-side SDK estimate (authoritative
source is AWS Cost Explorer / CUR 2.0), mirroring the Claude Agent SDK
cost-tracking caveat.

Align claude-agent-sdk 0.2.110 (bundles CLI 2.1.191) with the npm CLI pin.

Tests: CDK Bedrock grant present/absent; helper assume + fail-open paths;
runner file+header wiring. aws-samples#211 tenant-isolation path untouched.

Refs aws-samples#215

* refactor(cdk): collect invokable Bedrock models in a loop (pre-empt aws-samples#434)

PR aws-samples#434 replaces the six named model/profile bindings in agent.ts with a
loop over a single source-of-truth id list. Our aws-samples#215 SessionRole grant
referenced those bindings by name, so the merge would break compilation.

Adopt aws-samples#434's loop+collection shape now: build each foundation model + its
cross-region profile in a loop, grant the runtime, and collect into one
list passed to AgentSessionRole.invokableModels. Behavior is byte-for-byte
identical in synth; the eventual aws-samples#434 merge becomes a one-line swap of the
local id array for resolveBedrockModelIds(this.node).

Refs aws-samples#215, aws-samples#434

* fix(agent): make Bedrock creds-helper fail-open paths observable (aws-samples#215 review)

Silent-failure review flagged that bedrock_creds_helper.py degraded silently:
a persistent assume-role denial would drop chargeback for weeks with no signal
pointing back to this code — the 'invisible degradation' AI004 forbids even
when the fallback itself is intended.

- Add _warn() (stderr only — stdout is the credential channel Claude Code
  parses, so shell.log/fd1 is unusable here).
- Log every fail-open path; distinguish severities: absent file (benign) vs
  present-but-unreadable (write bug), and expected ClientError/BotoCoreError
  assume failure vs UNEXPECTED errors.
- Narrow the assume catch to (ClientError, BotoCoreError); catch ImportError on
  boto3 separately (packaging defect, not AccessDenied). All still fail open.

Behavior unchanged (still fail-open to ambient creds); degradations are now
visible and correlatable. Tests cover each distinguished path + its diagnostic.

Refs aws-samples#215

* docs(215): note the deliberate ANTHROPIC_CUSTOM_HEADERS env exception

Security review (LOW/accepted): unlike tenant-data tags, the request-metadata
header lives on os.environ because Claude Code reads it from there. Document
why that's safe (self-referential non-secret values; json.dumps escaping blocks
header injection) in both the code and the design doc, so it reads as intent
rather than an oversight against the 'tenant ids out of os.environ' discipline.

Refs aws-samples#215

* docs(215): correct cost-allocation tag activation steps

The IAM-principal tag keys can't be pre-activated — they only appear in the
Billing console after the platform makes tagged Bedrock calls. Fix the ordering
(deploy → run task → wait ≤24h → activate), point to Billing → Cost allocation
tags (not Tag Editor / Resource Groups, which lists resource types), and note
the capability may not be enabled in every account/region yet.

Refs aws-samples#215

* fix(cdk): enable Bedrock model-invocation logging on deploy (aws-samples#215)

The ModelInvocationLogging custom resource sent largeDataDeliveryS3Config
with an empty bucketName. Bedrock rejects that client-side (ValidationException,
'min length: 3'), and ignoreErrorCodesMatching: '.*' swallowed it while onUpdate
never re-fired (static props) — so a fresh deploy silently left model-invocation
logging DISABLED, and Bedrock recorded no requestMetadata (aws-samples#215 Track 2 produced
nothing to query). Found during live verification of task 01KWD7S....

- Omit largeDataDeliveryS3Config entirely (optional; only for S3 large-data
  delivery, which this stack doesn't use). The 'required by API schema' comment
  was wrong.
- Narrow ignoreErrorCodesMatching from '.*' to transient service errors only
  (Throttling/ServiceUnavailable/InternalServer) so a client-side
  misconfiguration fails the deploy loudly instead of disabling logging silently.
- Tests: assert the CR never sends largeDataDeliveryS3Config and never uses a
  catch-all error ignore.
- Docs: COST_ATTRIBUTION.md now tells operators to verify logging is on in the
  agent's Region (get-model-invocation-logging-configuration) and how to
  re-enable it, since metadata is only recorded when logging is active.

Verified live: with logging on, invocation logs show requestMetadata.{user_id,
repo,task_id} and the abca-bedrock-<task_id> session ARN — Tracks 1 and 2 both
confirmed working end-to-end.

Refs aws-samples#215

* fix(cdk): grant iam:PassRole for Bedrock invocation-logging custom resource (aws-samples#215)

With the empty-bucket validation error fixed, PutModelInvocationLoggingConfiguration
now actually reaches Bedrock at deploy — and fails because the custom resource's
Lambda role lacks iam:PassRole on BedrockLoggingRole (the role it hands to the
Bedrock service to write the log group). This was masked by the earlier
client-side ValidationException that ignoreErrorCodesMatching: '.*' swallowed.

Add iam:PassRole scoped to the BedrockLoggingRole ARN (not a wildcard). Test
asserts the grant is present.

Refs aws-samples#215

---------

Co-authored-by: bgagent <bgagent@noreply.github.com>
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