feat(engine): implement Bedrock routing + constructor#199
Merged
Conversation
…pin comments Removes the plan's Draft marker after cold self-review against the checked out worktree (all step claims verified accurate; design's four Open Artifact Decisions already correctly resolved). Turns the bedrock Cargo feature on by default and adds source comments at the rig/rig-bedrock version-pin coupling point, per plan Step 0. No behavior change yet -- the feature test remains red for the same missing-router-branch reason.
Replaces the hardcoded "not yet implemented" stub with a real rig_bedrock::client::Client::from_env() -> completion_model() -> RigEngine::new() construction, matching the anthropic/openai/gemini constructors' shape minus the eager key check (rig-bedrock's Client::from_env() never inspects AWS credentials -- it always succeeds, deferring resolution to the first live call). Adds a unit test proving construction succeeds offline with no AWS credentials present. Also fixes a pre-existing clippy doc-comment formatting issue in the feature test (missing backticks, unindented doc-list continuation) surfaced while confirming --all-features clippy cleanliness for this step. The router still has no "bedrock:" branch, so the feature test's first three assertions remain red for the same reason as before this commit; its fourth (constructor-only) assertion is unblocked.
Adds the "bedrock:" router branch in open_engine_for_model, mirroring the existing claude-/gpt-/gemini- if-let-then-return shape: strips the Ailly-side prefix, forwards the remainder verbatim (no shape validation, so a raw AWS model id and an inference-profile ARN both pass through unchanged) to bedrock_from_env, and boxes the result. Gated on #[cfg(feature = "bedrock")] so a "--no-default-features" build has no bedrock: branch at all and falls through to the existing ModelNotFound, unchanged from pre-feature behavior. Bundles plan Step 3's non-regression tests in with Step 2's, since they land in the same test-module edit and Step 3 makes no production code change of its own: a bedrock-shaped id without the prefix still ModelNotFound (regardless of feature state), and, gated on #[cfg(not(feature = "bedrock"))], a "bedrock:"-prefixed id still ModelNotFound when the feature is disabled. The feature test (tests/engine_routing_bedrock.rs) now passes in full under --all-features and under plain `cargo test` (bedrock is default-on as of the Step 0 commit).
Extends open_engine_for_model's per-branch doc comment with a "bedrock:" bullet: recognised-but-credential-less Bedrock ids resolve to Ok rather than Err(Auth), because AWS credential resolution is deferred to the first live call, unlike the other three providers' eager key check. Also notes the default-on feature and the disabled-feature ModelNotFound fallthrough inline where a reader would otherwise expect to see Auth mentioned. Closes the design's remaining non-test-visible Specification/Metrics items. Verified with mise run check/lint/test plus plain cargo build/test (no flags) -- the bedrock feature is default-on as of the Step 0 commit, so all of these already exercise the full feature-test behavior without an explicit --features flag.
bedrock_from_env used the AWS-side remainder (post "bedrock:" strip) both
to build the wire-level rig_bedrock CompletionModel and as RigEngine's
model_id, so EngineError::ModelNotFound{model} reported the bare AWS id
for Bedrock only -- inconsistent with every other provider (whose
model_id always equals the full, unstripped conversation.meta.model) and
with ModelNotFound's own doc comment. bedrock_from_env now takes the
stripped remainder (for the CompletionModel) and the full original
"bedrock:"-prefixed id separately (for RigEngine::new's model_id), and
open_engine_for_model passes both. Adds a unit test proving a
ModelNotFound-triggering bedrock call's error carries the full
"bedrock:..." id, not the stripped AWS remainder.
…nfigs The file's first three assertions were not gated behind #[cfg(feature = "bedrock")] but unconditionally expected Ok(_) for a "bedrock:"-prefixed id, which only holds when the feature is on. Since bedrock is default-on, `cargo test --no-default-features` made these assertions fail even though the router's fallthrough to ModelNotFound was itself correct. Splits the router-level assertions into a feature-enabled variant (expects Ok) and a feature-disabled variant (expects ModelNotFound), mirroring the file's already-gated bedrock_from_env constructor check, and keeps the always-true unprefixed-id assertion ungated. Verified green under `cargo test --all-features`, `cargo test --no-default-features`, and plain `cargo test`.
DavidSouther
force-pushed
the
feature/b-bedrock-engine
branch
from
324fb62 to
0652a62
Compare
DavidSouther
commented
Jul 8, 2026
Comment on lines
+635
to
+640
| /// call. That resolution automatically prefers a Bedrock API key | ||
| /// (`AWS_BEARER_TOKEN_BEDROCK`) over the standard `SigV4` credential chain | ||
| /// (env vars, shared profile, SSO, IMDS role) whenever both are present -- | ||
| /// a property of the pinned `rig-bedrock`/AWS-SDK versions and the plain | ||
| /// `aws_config::load_from_env()` call they make, not something this | ||
| /// function implements itself. |
Owner
Author
There was a problem hiding this comment.
Too much detail, it can simply say "deferred to AWS's SDK, preferring AWS_BEARER_TOKEN" or similar.
Comment on lines
+644
to
+651
| /// `rig_bedrock` to build the wire-level `CompletionModel`. `full_model_id` | ||
| /// is the original, unstripped, user-facing id (e.g. | ||
| /// `"bedrock:meta.llama3-3-70b-instruct-v1:0"`) and is what gets stored as | ||
| /// `RigEngine`'s `model_id`, matching every other provider constructor | ||
| /// (which all pass their full, unmodified id as both the wire-level model | ||
| /// and `model_id`). Keeping these separate is what makes | ||
| /// `EngineError::ModelNotFound { model }` on a Bedrock call report the same | ||
| /// `"bedrock:..."` id the caller requested, not the stripped AWS remainder. |
Owner
Author
There was a problem hiding this comment.
Too much detail, end at "verbatim to rig_bedrock."
Owner
Author
There was a problem hiding this comment.
Clean out before merging.
…ay plan.md PR #199 review: condense the credential-resolution and model/full_model_id paragraphs on bedrock_from_env's doc comment down to what the reviewer asked for. Also removes plan.md, committed into this repo instead of the coordinating session folder (moved there).
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
1 participant
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
Part of the
2026-07-06-A-ailly-evalsproject, Feature-step B. Closes the Bedrock leg of domain-driven-design#29's "ideal matrix of runners."open_engine_for_modeldidn't recognize any Bedrock-shaped model id, andbedrock_from_envwas a stub that always returnedProvider("not yet implemented"). This adds the real constructor (builds an AWS Bedrock client, hands it to the existing provider-agnosticRigEngine<M>) and the missing router branch, so abedrock:-prefixed model id reaches a realrig_bedrock::completion::CompletionModel, the same wayclaude-/gpt-/gemini-ids already do.Live confirmation against real AWS credentials is explicitly out of scope here (no AWS credentials available yet) — this closes the local wiring gap only.
Status
7 commits, feature test green under both feature-flag configs. Adversarial review found two real bugs —
ModelNotFounddropping thebedrock:prefix, andcargo test --no-default-featuresnot actually exercising this path — both fixed in the last two commits (ab3d8c1,324fb62).Test plan
cargo testandcargo test --no-default-features