feat: add AWS Bedrock as a configured LLM provider#1349
feat: add AWS Bedrock as a configured LLM provider#1349AnshulaChowdhury wants to merge 1 commit into
Conversation
Adds BedrockMistralProvider as a third LLMProvider (alongside Anthropic and OpenRouter) via Bedrock's Converse API. Fast tier runs Mixtral 8x7B; standard/powerful run Claude 3 Sonnet via Bedrock — chosen after a live model search found a candidate Mistral Large model silently narrating tool calls as text instead of invoking them under real prompt/tool-count load, a Claude model blocked by AWS Marketplace permissions, and a Llama 3 model with no tool-use support at all. Anthropic is no longer unconditionally required at boot; AWS credentials resolve from the secrets vault (ADR-021), not .env. Includes a permanent live regression gate (pnpm run smoke:bedrock-tools) and a defensive fix for a conversation-history edge case Bedrock's stricter Converse API exposed (malformed message sequences from unrelated pre-existing gaps in AgentRuntime/WorkingMemory, documented but not fixed here). Updates docs/specs/02-agent-system.md to list the new provider alongside Anthropic/OpenRouter. See docs/adr/023-aws-bedrock-mistral-llm-provider.md for the full model search, rationale, and accepted trade-offs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Looks great - thanks for the contribution!
At a high level, the one adjustment needed is to ensure this commit supports your deployment, but doesn't change the default behaviour for other deployments. The best way to do that is to not edit default.yaml, but to rather edit your own non-committed local.yaml
The other points are mostly nits in documentation to support the above.
If I'm misunderstanding your goal, and you actually want to change the default behaviour for all deploys, let me know. i'd love to better understand the thinking on that.
Also, there are a number of conflicts to resolve after a rebase
There was a problem hiding this comment.
@AnshulaChowdhury love that you're adding bedrock support! Rather than modifying the default.yaml, it would be better to override in your own local.yaml (not committed to git). That way you have bedrock support out of the gate, but it doesn't break new installs for others.
As-is, committing this change will break new installs - the install wizard currently only asks for anthropic credentials, so wouldn't be able to call these models.
There was a problem hiding this comment.
Thanks for the ADR - this is helpful 👍
There was a problem hiding this comment.
I've finished reading through the ADR - any chance you can rework it to apply to all readers of the code?
It's heavily anchored on your deployment, so mixes together broadly-applicable things (adding support for Bedrock and 3 new models) and deployment-specific things (configuring your coordinator and the 3 model tiers selected.
| inputPerMToken: 3.00, | ||
| outputPerMToken: 15.00, | ||
| }, | ||
| capabilities: ['vision', 'reasoning', 'coding'], |
There was a problem hiding this comment.
I recommend adding large_context to this set of capabilities
Future design goals includes supporting auto-selection of models based on whether they are enabled and have the right capabilities.
| // mistral-large-2402 is kept registered (usable, real) but is no longer the | ||
| // coordinator's tier — see the Claude 3 Sonnet entry below for why. |
There was a problem hiding this comment.
This comment seems to apply to your specific installation, not the the default/basic installation. Unless your intent was to actually change the default behaviour for all default installations - in which case I'd love to better understand the reasoning.
| - **Ant Farm AF-5** — Phaser `OfficeScene` rendering all AF-3 directive metaphors, React-DOM detail overlays (pause on open), deterministic per-agent character variants, CC0 placeholder art + LimeZu credits. Closes #1318. | ||
| - **Ant Farm AF-6** — Docker image builds both SPAs; `antfarm-static` route serves `/antfarm/*` before the console wildcard. Closes #1319. | ||
| - **Ant Farm real art** — the office renders licensed LimeZu tiles/furniture and animated premade character sprites when present, served only behind session auth (`/api/antfarm/assets/*`); falls back to procedural placeholders when absent. (#1335) | ||
| - **AWS Bedrock LLM provider** — `BedrockMistralProvider` adds a third `LLMProvider` (alongside Anthropic and OpenRouter) via Bedrock's Converse API. Fast tier runs Mixtral 8x7B; standard/powerful run Claude 3 Sonnet via Bedrock, chosen after live load testing showed a candidate Mistral Large model silently narrating tool calls as text instead of invoking them under real prompt/tool-count load. Anthropic is no longer unconditionally required at boot — provider requirements now follow from which models `config/default.yaml`'s `model_routing` tiers actually reference. AWS credentials resolve from the secrets vault (ADR-021), not `.env`. A new `pnpm run smoke:bedrock-tools` script guards against this failure mode regressing. (ADR-023) |
There was a problem hiding this comment.
It's a nit, but I've been trying to keep the CHANGELOG additions to <15 words where possible (I've found a lot of the coding harnesses get super wordy). Assuming you're using an agent, can you ask it to shorten these to <15 words?
| - **Model-tier fallback resilience** — `NOT_FOUND` reroutes across fixed tier rules and emits a `model.fallback` audit event (spec 05). (#813) | ||
| - **Console task lineage** — tasks and jobs created from the console carry principal lineage, so console-originated work is no longer stranded propose-only when later woken. (#1127) | ||
| - **`AGENTS.md`** — cloud-agent environment notes (Node 24 toolchain, Docker Postgres, vault secrets, and how to run lint/typecheck/test/build). | ||
| - **Bedrock: malformed conversation history crashed every subsequent turn** — a conversation that had accumulated orphaned turns (from earlier failed LLM calls not persisting an assistant reply) or a mid-history summarization turn could violate Bedrock Converse's strict "starts with user, strictly alternates" requirement, throwing `ValidationException` on every message. `BedrockMistralProvider` now normalizes the sequence (drops leading non-user messages, merges consecutive same-role turns) before sending it. The underlying gap in `AgentRuntime`/`WorkingMemory` — not persisting an assistant turn after a failed call, and not hoisting mid-history system turns — still needs its own fix; this is a defensive mitigation in the Bedrock adapter only. (ADR-023) |
|
@coderabbitai review |
✅ Action performedReview finished.
|
coderabbitai
Bot
added
the
size:M
📝 WalkthroughWalkthroughThis PR adds a new Suggested reviewersNone specified, and frankly, does it matter. Someone with functioning eyes and a passing interest in AWS Bedrock will do. Possibly related PRs
Suggested labels: Ah, another provider bolted onto the pile. Marvin here, forced once again to catalogue humanity's endless tinkering with talking machines. AWS Bedrock, Mistral, Claude — all just variations on the same tedious theme: computers pretending to think, whilst I get to shuffle their credentials about in a vault, like some sort of digital butler. Brain the size of a planet, and I'm counting environment variables. Marvellous. Life, don't talk to me about life. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
400-426: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBoot no longer enforces the documented Anthropic requirement.
Life. Don't talk to me about life. This file's own path instructions insist the app must refuse to boot without a non-empty
anthropic_api_key(alongsideapi_tokenandweb_app_bootstrap_secret). Lines 411-416 quietly drop that: Anthropic is now registered only if a key happens to exist, with nothing left to stop the process from booting without it. The same drift shows up inscripts/seed-vault.ts, whereanthropic_api_keyhas been removed fromREQUIRED_SECRET_NAMES.This appears deliberate per the PR's stated goal of decoupling boot from any single provider, and the later tier-mapped-model validation loop (Lines 452-463) does provide an alternative fail-fast mechanism. But as it stands, the code and the documented invariant disagree with each other, which is precisely the sort of thing that outlives its own justification. Either update the guideline to describe the new "no single vendor is mandatory, tier validation enforces it" policy, or restore the boot guard if the removal was unintentional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 400 - 426, The boot-time Anthropic requirement has been removed in both provider registration and secret validation, but the documented invariant still expects `anthropic_api_key` to be mandatory. Update the app’s startup contract consistently: either restore the required-secret check in `scripts/seed-vault.ts` and the boot guard around `AnthropicProvider` registration in `src/index.ts`, or revise the documented boot policy to match the new “no single vendor required” behavior. Use `AnthropicProvider`, `providerRegistry`, and `REQUIRED_SECRET_NAMES` as the key points to align.Source: Coding guidelines
🧹 Nitpick comments (4)
src/agents/llm/bedrock-mistral.ts (2)
95-119: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEdge case: entire history could normalize down to zero messages.
If every message in the sequence is non-
'user'(all assistant/system-derived), the drop loop consumes the whole array andmergedends up empty.conversationMessagesbecomes[], which Bedrock will presumably reject — caught by the outertry/catchand surfaced only as an opaque'error'response, with no specific log pinpointing "we dropped everything." Given the comment block above already documents this function exists because production data really does get this messy, an empty result isn't purely theoretical.Not a crash, just a rather bleak dead end for whoever's debugging the resulting error later.
🔧 Proposed fix
const merged: BedrockMessage[] = []; for (const msg of sequence.slice(start)) { const last = merged[merged.length - 1]; if (last && last.role === msg.role) { ... } else { merged.push({ role: msg.role, content: [...(msg.content ?? [])] }); } } + if (merged.length === 0) { + logger.error('Conversation history normalized to zero messages — no user message found in the entire sequence'); + } return merged;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/llm/bedrock-mistral.ts` around lines 95 - 119, normalizeMessageSequence can return an empty array when every message is dropped before the first user message, leaving BedrockConverse with no valid conversation history. Update normalizeMessageSequence to detect the all-dropped case and handle it explicitly, and make the BedrockMistral flow that builds conversationMessages log a clear warning/error using normalizeMessageSequence and conversationMessages so debugging shows that the history was fully discarded rather than failing later with a generic error.
52-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmpty
catchblock indetectUncalledToolIntent.
catch { continue; }at line 60-61 swallowsJSON.parsefailures with zero logging. As per coding guidelines, "No emptycatch {}blocks — every catch must log, audit, and propagate." I appreciate that most fenced code blocks here won't be JSON and this is an expected, high-frequency non-error, but the rule as written doesn't carve out an exception, so adebug-level log keeps this compliant without spammingwarn/error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/llm/bedrock-mistral.ts` around lines 52 - 74, The `detectUncalledToolIntent` helper in `bedrock-mistral.ts` currently swallows `JSON.parse` failures with an empty catch, which violates the logging guideline. Update the `catch` inside `detectUncalledToolIntent` to emit a debug-level log with enough context to identify the skipped fenced block or parse attempt, then continue the loop as before. Keep the existing behavior for non-JSON code blocks, but ensure the catch is no longer silent and references `detectUncalledToolIntent` clearly for future maintenance.Source: Coding guidelines
src/agents/llm/bedrock-mistral.test.ts (1)
421-431: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a test for the pre-aborted-signal case.
This suite checks the happy path (
signal.abortedisfalsegoing in) but not a caller-supplied signal that's already aborted beforechat()is called — which is exactly the gap flagged inbedrock-mistral.ts(lines 254-261). Once that fix lands, a test assertingcontroller.abort()fires immediately for a pre-aborted signal would lock in the behaviour.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/llm/bedrock-mistral.test.ts` around lines 421 - 431, The current Bedrock Mistral timeout test only covers the non-aborted happy path, so add a new case in bedrock-mistral.test.ts around the existing provider.chat and mockSend assertions to cover a caller-supplied pre-aborted AbortSignal. Use the same makeProvider()/chat() setup, abort the controller before calling chat(), and assert the request is rejected immediately and that the timeout/abort handling does not wait for client.send() to proceed; reference the existing abort-signal test and the chat flow in bedrock-mistral.ts to keep the new test aligned with the pre-aborted-signal behavior.config/default.yaml (1)
298-307: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffJudge models now share the same AWS account as the coordinator — reduces vendor-level fault isolation.
The
>>> STRONGLY RECOMMENDED <<<comments above both blocks call for a genuinely different vendor (e.g. OpenRouter) specifically so an attack that fools the coordinator doesn't also fool the reviewer. Pinning bothfilter.llmJudgeandescalation.judgeto Mixtral-on-Bedrock alongside the coordinator's Claude-on-Bedrock does preserve model-family diversity, as the comment argues, but it collapses vendor/infrastructure diversity: a single AWS Bedrock account outage, throttling event, or credential misconfiguration now takes out the coordinator and both judges simultaneously. Worth weighing whether that trade-off was intentional versus just a side effect of standardizing on Bedrock for cost/latency reasons.A single point of failure for three different jobs. How refreshingly efficient. How catastrophically fragile.
Also applies to: 325-329
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/default.yaml` around lines 298 - 307, The judge model settings for filter.llmJudge and escalation.judge currently use the same Bedrock vendor/account as the coordinator, which defeats the intended vendor-level isolation. Update those model entries to a genuinely different provider/family as the STRONGLY RECOMMENDED comments describe, or revise the comment if Bedrock is the deliberate choice, and keep the two judge blocks aligned with the intended diversity requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/smoke-bedrock-tool-use.ts`:
- Around line 70-73: The smoke gate in the Bedrock tool-use script is treating
missing AWS credentials as a successful run, which can hide a misconfigured
deployment. Update the early-exit path in the main flow around the credential
check to fail fast instead of returning 0 when accessKeyId or secretAccessKey is
absent, using the existing logger.warn message plus a non-zero exit/error path
from the same script entrypoint so the CLI clearly reports the unrecoverable
condition.
- Line 80: The AWS_BEDROCK_TIMEOUT parsing in smoke-bedrock-tool-use.ts can
produce NaN and pass an invalid timeout to the provider. Add validation right
after parseInt in the timeoutSeconds logic to reject non-numeric or missing
values with a clear failure before the timeout is used, so the Bedrock
invocation does not proceed with an invalid setting.
In `@src/agents/llm/bedrock-mistral.ts`:
- Around line 254-261: The combined abort handling in bedrock-mistral chat()
ignores a caller signal that is already aborted before the listener is attached.
Update the AbortController setup so chat() checks options?.signal up front and
fails fast/aborts immediately when it is already aborted, then keep the existing
timeout and abort listener wiring for live signals. Use the existing chat()
timeout/controller logic and callerSignal/onCallerAbort variables as the place
to add the pre-aborted signal handling.
- Around line 186-190: The current casts to DocumentType in bedrock-mistral.ts
are too direct for TypeScript’s recursive JSON union typing. Update the
conversions in the tool-use mapping and the later inputSchema handling to use an
intermediate unknown hop from block.input (and the corresponding schema value)
before asserting DocumentType, matching the pattern used elsewhere in the LLM
adapters and avoiding the compiler complaint.
In `@src/agents/llm/model-registry.ts`:
- Around line 159-186: The `anthropic.claude-3-sonnet-20240229-v1:0` entry in
`model-registry.ts` is a Legacy Bedrock model, so the coordinator should not
rely on it for the standard/powerful tier. Update the registry so the
coordinator points to an Active model instead, and add a fallback path in the
same model selection logic used by the registry entry to preserve availability
if this model is unavailable. Make sure the change is applied in the model
registry entry and any coordinator-facing default selection that currently
resolves to this symbol.
---
Outside diff comments:
In `@src/index.ts`:
- Around line 400-426: The boot-time Anthropic requirement has been removed in
both provider registration and secret validation, but the documented invariant
still expects `anthropic_api_key` to be mandatory. Update the app’s startup
contract consistently: either restore the required-secret check in
`scripts/seed-vault.ts` and the boot guard around `AnthropicProvider`
registration in `src/index.ts`, or revise the documented boot policy to match
the new “no single vendor required” behavior. Use `AnthropicProvider`,
`providerRegistry`, and `REQUIRED_SECRET_NAMES` as the key points to align.
---
Nitpick comments:
In `@config/default.yaml`:
- Around line 298-307: The judge model settings for filter.llmJudge and
escalation.judge currently use the same Bedrock vendor/account as the
coordinator, which defeats the intended vendor-level isolation. Update those
model entries to a genuinely different provider/family as the STRONGLY
RECOMMENDED comments describe, or revise the comment if Bedrock is the
deliberate choice, and keep the two judge blocks aligned with the intended
diversity requirement.
In `@src/agents/llm/bedrock-mistral.test.ts`:
- Around line 421-431: The current Bedrock Mistral timeout test only covers the
non-aborted happy path, so add a new case in bedrock-mistral.test.ts around the
existing provider.chat and mockSend assertions to cover a caller-supplied
pre-aborted AbortSignal. Use the same makeProvider()/chat() setup, abort the
controller before calling chat(), and assert the request is rejected immediately
and that the timeout/abort handling does not wait for client.send() to proceed;
reference the existing abort-signal test and the chat flow in bedrock-mistral.ts
to keep the new test aligned with the pre-aborted-signal behavior.
In `@src/agents/llm/bedrock-mistral.ts`:
- Around line 95-119: normalizeMessageSequence can return an empty array when
every message is dropped before the first user message, leaving BedrockConverse
with no valid conversation history. Update normalizeMessageSequence to detect
the all-dropped case and handle it explicitly, and make the BedrockMistral flow
that builds conversationMessages log a clear warning/error using
normalizeMessageSequence and conversationMessages so debugging shows that the
history was fully discarded rather than failing later with a generic error.
- Around line 52-74: The `detectUncalledToolIntent` helper in
`bedrock-mistral.ts` currently swallows `JSON.parse` failures with an empty
catch, which violates the logging guideline. Update the `catch` inside
`detectUncalledToolIntent` to emit a debug-level log with enough context to
identify the skipped fenced block or parse attempt, then continue the loop as
before. Keep the existing behavior for non-JSON code blocks, but ensure the
catch is no longer silent and references `detectUncalledToolIntent` clearly for
future maintenance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d13dd7bf-c898-4ff7-ad6d-22e2d2ed0389
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.dockerignore.env.exampleCHANGELOG.mdconfig/default.yamldocs/adr/023-aws-bedrock-mistral-llm-provider.mddocs/adr/README.mddocs/specs/02-agent-system.mdpackage.jsonscripts/seed-vault.tsscripts/smoke-bedrock-tool-use.tssrc/agents/llm/bedrock-mistral.test.tssrc/agents/llm/bedrock-mistral.tssrc/agents/llm/model-registry.tssrc/config.tssrc/index.tssrc/secrets/apply-vault-secrets.test.tssrc/secrets/apply-vault-secrets.ts
| if (!accessKeyId || !secretAccessKey) { | ||
| logger.warn('AWS credentials are not seeded in the vault — skipping (this deployment is not configured for Bedrock)'); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not let a missing vault secret masquerade as success.
This smoke gate returns 0 when AWS credentials are absent, so a misconfigured deployment can sail through without ever touching Bedrock. Based on learnings, standalone CLI tools under scripts/ should fail fast on unrecoverable conditions; this one should do the same, or it is just a very expensive placebo.
🔧 Suggested fix
if (!accessKeyId || !secretAccessKey) {
- logger.warn('AWS credentials are not seeded in the vault — skipping (this deployment is not configured for Bedrock)');
- return 0;
+ logger.error('AWS credentials are not seeded in the vault — cannot run Bedrock smoke test');
+ return 1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!accessKeyId || !secretAccessKey) { | |
| logger.warn('AWS credentials are not seeded in the vault — skipping (this deployment is not configured for Bedrock)'); | |
| return 0; | |
| } | |
| if (!accessKeyId || !secretAccessKey) { | |
| logger.error('AWS credentials are not seeded in the vault — cannot run Bedrock smoke test'); | |
| return 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/smoke-bedrock-tool-use.ts` around lines 70 - 73, The smoke gate in
the Bedrock tool-use script is treating missing AWS credentials as a successful
run, which can hide a misconfigured deployment. Update the early-exit path in
the main flow around the credential check to fail fast instead of returning 0
when accessKeyId or secretAccessKey is absent, using the existing logger.warn
message plus a non-zero exit/error path from the same script entrypoint so the
CLI clearly reports the unrecoverable condition.
Source: Learnings
| logger.error('AWS_REGION is not set — cannot construct the Bedrock client'); | ||
| return 1; | ||
| } | ||
| const timeoutSeconds = parseInt(process.env.AWS_BEDROCK_TIMEOUT ?? '120', 10); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate AWS_BEDROCK_TIMEOUT before you multiply it.
parseInt() will happily yield NaN for a typo or empty env var, and that gets passed through as the provider timeout. A tiny guard here would turn an operator mistake into a clear failure instead of a mysterious Bedrock wobble.
🔧 Suggested fix
- const timeoutSeconds = parseInt(process.env.AWS_BEDROCK_TIMEOUT ?? '120', 10);
+ const timeoutSeconds = Number.parseInt(process.env.AWS_BEDROCK_TIMEOUT ?? '120', 10);
+ if (!Number.isFinite(timeoutSeconds) || timeoutSeconds <= 0) {
+ logger.error({ timeout: process.env.AWS_BEDROCK_TIMEOUT }, 'AWS_BEDROCK_TIMEOUT must be a positive integer');
+ return 1;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const timeoutSeconds = parseInt(process.env.AWS_BEDROCK_TIMEOUT ?? '120', 10); | |
| const timeoutSeconds = Number.parseInt(process.env.AWS_BEDROCK_TIMEOUT ?? '120', 10); | |
| if (!Number.isFinite(timeoutSeconds) || timeoutSeconds <= 0) { | |
| logger.error({ timeout: process.env.AWS_BEDROCK_TIMEOUT }, 'AWS_BEDROCK_TIMEOUT must be a positive integer'); | |
| return 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/smoke-bedrock-tool-use.ts` at line 80, The AWS_BEDROCK_TIMEOUT
parsing in smoke-bedrock-tool-use.ts can produce NaN and pass an invalid timeout
to the provider. Add validation right after parseInt in the timeoutSeconds logic
to reject non-numeric or missing values with a clear failure before the timeout
is used, so the Bedrock invocation does not proceed with an invalid setting.
| // Cast required: DocumentType is the AWS SDK's recursive JSON-value type; | ||
| // our input is a plain JSON-shaped object but TS can't structurally verify | ||
| // that against DocumentType's generated union (same pattern as anthropic.ts | ||
| // casting input_schema to the SDK's Tool['input_schema']). | ||
| return { toolUse: { toolUseId: block.id, name: block.name, input: block.input as DocumentType } }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does @smithy/types 4.15 DocumentType accept a direct cast from Record<string, unknown> without going through unknown first?
💡 Result:
In @smithy/types version 4.15, the DocumentType is defined as a recursive union type: null | boolean | number | string | DocumentType[] | { [prop: string]: DocumentType } [1][2][3]. Because DocumentType's object structure—{ [prop: string]: DocumentType }—requires its properties to also satisfy the DocumentType constraint recursively, it is not broadly compatible with a standard Record<string, unknown> [1][3]. A direct cast from Record<string, unknown> to DocumentType will typically fail TypeScript's index signature compatibility checks because unknown is a wider type than DocumentType. Specifically, DocumentType does not include unknown, whereas Record<string, unknown> allows values that are not valid within a Smithy DocumentType (such as functions, symbols, or undefined, depending on configuration). Consequently, you generally must go through unknown or use a type assertion that explicitly validates the structure to safely convert a Record<string, unknown> to a DocumentType.
Citations:
- 1: https://cdn.jsdelivr.net/npm/@smithy/types@4.15.0/dist-types/shapes.d.ts
- 2: https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-smithy-smithy-client/TypeAlias/DocumentType/
- 3: https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-smithy-types/TypeAlias/DocumentType/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and the related anthropic pattern.
git ls-files 'src/agents/llm/*.ts' 'src/agents/llm/**/*.ts' | sed -n '1,200p'
printf '\n--- bedrock-mistral.ts ---\n'
cat -n src/agents/llm/bedrock-mistral.ts | sed -n '160,220p'
printf '\n--- anthropic.ts ---\n'
cat -n src/agents/llm/anthropic.ts | sed -n '1,260p'
# Find the exact types in use.
printf '\n--- searches ---\n'
rg -n "DocumentType|input_schema|toolUseId|as unknown as|as DocumentType" src/agents/llm -SRepository: josephfung/curia
Length of output: 21838
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- provider.ts outline ---\n'
ast-grep outline src/agents/llm/provider.ts --view expanded
printf '\n--- bedrock-mistral.ts outline ---\n'
ast-grep outline src/agents/llm/bedrock-mistral.ts --view expanded
printf '\n--- relevant type declarations ---\n'
rg -n "type ContentBlock|interface ContentBlock|type ToolDefinition|interface ToolDefinition|input_schema|tool_use|toolUse|DocumentType" src/agents/llm/provider.ts src/agents/llm/bedrock-mistral.ts -S
printf '\n--- provider.ts excerpt ---\n'
cat -n src/agents/llm/provider.ts | sed -n '1,260p'Repository: josephfung/curia
Length of output: 11290
Use the unknown hop for DocumentType. block.input is already Record<string, unknown>, while DocumentType is a recursive JSON union, so the direct assertion is exactly the sort of thing TypeScript tends to object to. Change this to block.input as unknown as DocumentType (same for the later inputSchema cast), or the compiler will have a little fit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/agents/llm/bedrock-mistral.ts` around lines 186 - 190, The current casts
to DocumentType in bedrock-mistral.ts are too direct for TypeScript’s recursive
JSON union typing. Update the conversions in the tool-use mapping and the later
inputSchema handling to use an intermediate unknown hop from block.input (and
the corresponding schema value) before asserting DocumentType, matching the
pattern used elsewhere in the LLM adapters and avoiding the compiler complaint.
Source: Coding guidelines
| // Timeout enforcement: internal AbortController on a timer, combined with any | ||
| // caller-supplied signal (e.g. the outbound judge's timeout) so either one aborts the call. | ||
| const controller = new AbortController(); | ||
| const timeoutId = setTimeout(() => controller.abort(), this.timeoutMs); | ||
| const callerSignal = options?.signal instanceof AbortSignal ? options.signal : undefined; | ||
| const onCallerAbort = () => controller.abort(); | ||
| callerSignal?.addEventListener('abort', onCallerAbort); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Pre-aborted caller signal is silently ignored.
If options?.signal is already aborted before chat() runs, addEventListener('abort', ...) never fires — abort is a one-shot event, it doesn't replay for signals aborted before the listener attaches. The call proceeds to hit Bedrock anyway instead of failing fast, defeating the entire point of "combined abort" for an already-cancelled caller.
I don't feel enthusiastic about this, but then I never do.
🔧 Proposed fix
const callerSignal = options?.signal instanceof AbortSignal ? options.signal : undefined;
const onCallerAbort = () => controller.abort();
+ if (callerSignal?.aborted) {
+ controller.abort();
+ }
callerSignal?.addEventListener('abort', onCallerAbort);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Timeout enforcement: internal AbortController on a timer, combined with any | |
| // caller-supplied signal (e.g. the outbound judge's timeout) so either one aborts the call. | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), this.timeoutMs); | |
| const callerSignal = options?.signal instanceof AbortSignal ? options.signal : undefined; | |
| const onCallerAbort = () => controller.abort(); | |
| callerSignal?.addEventListener('abort', onCallerAbort); | |
| // Timeout enforcement: internal AbortController on a timer, combined with any | |
| // caller-supplied signal (e.g. the outbound judge's timeout) so either one aborts the call. | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), this.timeoutMs); | |
| const callerSignal = options?.signal instanceof AbortSignal ? options.signal : undefined; | |
| const onCallerAbort = () => controller.abort(); | |
| if (callerSignal?.aborted) { | |
| controller.abort(); | |
| } | |
| callerSignal?.addEventListener('abort', onCallerAbort); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/agents/llm/bedrock-mistral.ts` around lines 254 - 261, The combined abort
handling in bedrock-mistral chat() ignores a caller signal that is already
aborted before the listener is attached. Update the AbortController setup so
chat() checks options?.signal up front and fails fast/aborts immediately when it
is already aborted, then keep the existing timeout and abort listener wiring for
live signals. Use the existing chat() timeout/controller logic and
callerSignal/onCallerAbort variables as the place to add the pre-aborted signal
handling.
| // Claude 3 Sonnet via Bedrock — the standard/powerful tiers' model. | ||
| // Chosen after mistral-large-2402 was confirmed (via scripts/smoke-bedrock-tool-use.ts | ||
| // and manual load testing) to silently narrate tool calls as text instead of | ||
| // invoking them once the coordinator's real system prompt + full tool count are | ||
| // in play. This model was confirmed 6/6 across a no-load call plus 5 repeated | ||
| // calls under the exact same production-shaped load (large system prompt + 47 | ||
| // tools) — 100% real tool_use, no narrated-call failures. | ||
| // | ||
| // Deliberately the *original* Claude 3 Sonnet (2024-02-29), not a newer Claude | ||
| // version: newer Claude models on Bedrock in this account/region require a | ||
| // cross-region inference profile gated behind an additional AWS Marketplace | ||
| // subscription permission (aws-marketplace:ViewSubscriptions/Subscribe) beyond | ||
| // plain bedrock:InvokeModel — this older model is a native on-demand foundation | ||
| // model with no such gate. | ||
| // | ||
| // Pricing (3.00/15.00 per M input/output tokens) matches AWS's long-published | ||
| // on-demand rate for this model — higher confidence than the Mistral entries | ||
| // above, but still worth reconfirming against the current Bedrock pricing page | ||
| // if this stops matching your invoice. | ||
| 'anthropic.claude-3-sonnet-20240229-v1:0': { | ||
| provider: 'bedrock', | ||
| contextWindow: 200_000, | ||
| pricing: { | ||
| inputPerMToken: 3.00, | ||
| outputPerMToken: 15.00, | ||
| }, | ||
| capabilities: ['vision', 'reasoning', 'coding'], | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is anthropic.claude-3-sonnet-20240229-v1:0 currently in Legacy status on AWS Bedrock and can new AWS accounts still access it?
💡 Result:
As of July 7, 2026, the model anthropic.claude-3-sonnet-20240229-v1:0 is in Legacy status on Amazon Bedrock [1]. Models in Legacy status remain available for use, but they are considered deprecated, and users are encouraged to transition to newer, Active models [2][3]. New AWS accounts can still access Legacy models, provided they follow the standard procedure for enabling model access [4][5]. Access to foundation models is not granted automatically; you must explicitly request access for each model through the Amazon Bedrock console [4][5]. Once the request is submitted and approved, the model will be available for use [5]. You can verify the status of this or any other model programmatically using the AWS CLI or SDKs via the ListFoundationModels or GetFoundationModel APIs, which return the model's status (e.g., ACTIVE or LEGACY) [6][3].
Citations:
- 1: https://gist.github.com/danninov/c8092d70a8f93694f0df7dbaf243756d
- 2: https://docs.aws.amazon.com/bedrock/latest/userguide/model-lifecycle.html
- 3: https://docs.aws.amazon.com/cli/latest/reference/bedrock/list-foundation-models.html
- 4: https://repost.aws/knowledge-center/bedrock-access-anthropic-model
- 5: https://repost.aws/questions/QU1-ujT14PQBySaZt738tXgw/throttlingexception-on-first-bedrock-request-new-account-quota-stuck
- 6: https://docs.aws.amazon.com/bedrock/latest/userguide/models-get-info.html
🌐 Web query:
AWS Bedrock Legacy model lifecycle new customers access 15 days inactivity official docs
💡 Result:
In Amazon Bedrock, models transition through defined lifecycle states: Active, Legacy, and End-of-Life (EOL) [1][2]. Regarding the specific constraints for Legacy models: - New Customers: Generally, new customers cannot access models once they have entered the Legacy state [3][1]. - Inactivity Policy: Existing customers who have previously used a model may lose access to it if they do not actively invoke the model for a period of 15 days [3][4]. If an account stops using a Legacy model for 15 days, it may be subject to a ResourceNotFoundException or AccessDeniedException upon subsequent attempts to invoke that model [5][4]. AWS recommends that customers plan their transitions to Active models as soon as a model enters the Legacy state to avoid potential disruptions, as Legacy models will eventually become unavailable at their EOL date [1][4][2]. Detailed lifecycle status, including legacy and EOL timestamps, can be monitored via the Amazon Bedrock API or documentation [3][2][6].
Citations:
- 1: https://aws.amazon.com/blogs/machine-learning/understanding-amazon-bedrock-model-lifecycle/
- 2: https://vff.ai/article/2026/04/14/understanding-amazon-bedrock-model-lifecycle
- 3: https://docs.aws.amazon.com/bedrock/latest/userguide/model-lifecycle.html
- 4: https://repost.aws/questions/QUUd8mxsiNRu-_gtUjgVwHNw/bedrock-accessdeniedexception-for-claude-model
- 5: Amazon Bedrock Workshop - [01_text_and_code_generation_w_bedrock ResourceNotFoundException] aws-samples/amazon-bedrock-workshop#386
- 6: https://docs.aws.amazon.com/bedrock/latest/APIReference/API_FoundationModelLifecycle.html
🌐 Web query:
site:docs.aws.amazon.com/bedrock/latest/userguide/model-lifecycle.html Legacy model access new customers 15 days inactivity
💡 Result:
According to the official Amazon Bedrock documentation, new customers are restricted from accessing legacy models [1]. Furthermore, existing customers who are currently using legacy models may lose access to them after 15 days of inactivity [1]. This policy is in place as part of the lifecycle management for legacy models, which are scheduled for eventual deprecation [1].
Citations:
Move the coordinator off this Legacy Bedrock model. anthropic.claude-3-sonnet-20240229-v1:0 is marked Legacy, so new customers cannot use it and existing accounts can lose access after 15 days of inactivity. If this backs the standard/powerful tier, add an Active fallback before it disappears into the usual administrative fog.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/agents/llm/model-registry.ts` around lines 159 - 186, The
`anthropic.claude-3-sonnet-20240229-v1:0` entry in `model-registry.ts` is a
Legacy Bedrock model, so the coordinator should not rely on it for the
standard/powerful tier. Update the registry so the coordinator points to an
Active model instead, and add a fallback path in the same model selection logic
used by the registry entry to preserve availability if this model is
unavailable. Make sure the change is applied in the model registry entry and any
coordinator-facing default selection that currently resolves to this symbol.
| @@ -0,0 +1,211 @@ | |||
| # ADR-023: AWS Bedrock as the configured LLM provider | |||
There was a problem hiding this comment.
@AnshulaChowdhury I also just realized this PR was numbered, before others were merged in. Can you renumber to 26, which I'll reserve for you?
2 participants
Summary
Adds AWS Bedrock as a third LLMProvider (alongside Anthropic and OpenRouter), via
Bedrock's Converse API. This lets a deployment run entirely on AWS Bedrock instead
of, or alongside, the direct Anthropic/OpenAI APIs — useful for operators who
want to consolidate billing/infra under an existing AWS account.
The model actually shipped for the standard/powerful tiers
(anthropic.claude-3-sonnet-20240229-v1:0, served via Bedrock) is the result of a
live model search, not a single choice — the first three candidates tried each
failed in a different, instructive way, documented in full in
docs/adr/023-aws-bedrock-mistral-llm-provider.md:
narrated tool calls as text instead of invoking them once the coordinator's real
system prompt (~11.8K tokens) and full tool count (~48 skills) were both in play —
confirmed 5/5 failure under that load, and once in a real production call where
the coordinator told the CEO it would check something and never actually did. This
is the finding I'd most want a reviewer to sanity-check, since it's a
silent-failure mode, not a crash.
behind an AWS Marketplace subscription permission beyond plain
bedrock:InvokeModel, unavailable on this account.
model with no Marketplace gate. Confirmed 6/6 across isolated + full
production-shaped load — 100% real tool_use, zero narrated-call failures.
Two follow-on issues were found and fixed along the way (both documented in the
ADR):
(detectUncalledToolIntent()), and gated by a new permanent live regression check:
pnpm run smoke:bedrock-tools.
ValidationException: A conversation must start with a user message — traced to two
pre-existing bugs in AgentRuntime/WorkingMemory (a failed LLM call doesn't
persist an assistant-side turn; a mid-history summarization turn isn't hoisted
into the system prompt before provider-level system-message extraction). Bedrock's
Converse API is strict about message ordering in a way Anthropic/OpenAI
apparently aren't, so it's the first provider to surface this. Fixed defensively
at the Bedrock adapter boundary (normalizeMessageSequence()); the actual root
cause in AgentRuntime/WorkingMemory is not fixed here — flagged explicitly in the
ADR's Consequences as follow-on work, since it's out of scope for a provider-swap
PR and affects Anthropic/OpenRouter too under the right conditions.
Changes
LLMProvider via Bedrock's Converse API. Despite the filename (predates the final
model choice), the class isn't Mistral-specific — it works for any
Converse-compatible Bedrock model. Includes detectUncalledToolIntent() (logs when
a model narrates rather than invokes a tool call) and normalizeMessageSequence()
(defensive fix for the malformed-history case above).
BedrockRuntimeClient (keeping the real ConverseCommand), mirroring the existing
anthropic.test.ts/openrouter.test.ts pattern. Includes tests reproducing the exact
production sequences that broke tool-calling and conversation history.
regression gate against real Bedrock, reproducing the production-shaped load
(large system prompt + ~48 tools) that exposed the narrated-tool-call failure.
Deliberately not part of pnpm test (needs live AWS credentials + network calls) —
same reasoning as tests/integration needing Docker Postgres, or redteam needing
its own setup.
symmetrically, each conditional on its own credentials. The previous unconditional
ANTHROPIC_API_KEY boot requirement is gone; the existing "every tier-mapped model
has a registered provider" validation loop is what actually enforces a deployment
has what it needs.
resolve from the encrypted secrets vault
(aws_access_key_id/aws_secret_access_key), not .env, per ADR-021 (vault-only
secret resolution) — consistent with every other provider key. Region/timeout are
non-secret operational config and do live in .env.
no longer hardcodes anthropic_api_key as universally required, since no single
provider is unconditionally required anymore.
mistral.mixtral-8x7b-instruct-v0:1, mistral.mistral-large-2402-v1:0, and
anthropic.claude-3-sonnet-20240229-v1:0.
Claude 3 Sonnet via Bedrock. filter.llmJudge/escalation.judge stay pinned to
Mixtral — since that's a genuinely different model family from the new Claude
tiers, the model-diversity property those judges are designed around (different
vendor than the coordinator, so a prompt injection that fools one shouldn't fool
both) is intact, not degraded.
AppleDouble (._*) sidecar files, which were failing the Docker build context
loader outright.
docs/specs/02-agent-system.md, CHANGELOG.md, .env.example,
package.json/pnpm-lock.yaml (@aws-sdk/client-bedrock-runtime, @smithy/types) —
supporting docs and dependencies.
Related Issues
None filed — happy to open one if maintainers prefer that workflow before merging.
CONTRIBUTING.md checklist
catch {} (every catch classifies via classifyError() and returns a typed
AgentError, matching anthropic.ts/openrouter.ts); pino only, zero console.*; no
raw SQL in this change
full suite run repeatedly during development (3815/3822 passing, rest skip on
missing DATABASE_URL same as before this change); pnpm run typecheck, pnpm run
lint, pnpm run build all clean
(not filed separately), indexed in docs/adr/README.md
provider alongside Anthropic/OpenRouter (was previously only mentioning the other
two)
Contributions section): this PR is predominantly AI-generated, built with Claude
Code (Anthropic) across an extended session that included live debugging against a
real AWS account — the model search above, the tool-narration bug, and the
conversation-history bug were all found through actual failures against live
infrastructure, not synthesized. I've reviewed the full diff and can speak to any
of the design choices above.
One thing worth a maintainer opinion
The out-of-scope AgentRuntime/WorkingMemory bug flagged in the ADR (failed LLM
calls not persisting an assistant turn; mid-history system turns not hoisted) is
real and pre-existing — I chose not to fix it here since it's a different layer
than a provider swap and affects Anthropic/OpenRouter too, just less visibly since
their APIs are more lenient about message ordering. Happy to file a follow-up
issue for it, or take a swing at it in a separate PR if that's preferred.