Bedrock model discovery and validation - #9997
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00272832ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0475cc0d30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…he Converse path 2)Apply the probe timeout to stream creation too
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db7072c93f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let id = summary.inference_profile_id(); | ||
| if Self::is_chat_capable_model_id(id) { | ||
| ids.insert(id.to_string()); |
There was a problem hiding this comment.
Return ARNs for application inference profiles
When profile_type is Application, this stores the short inference_profile_id, but Bedrock requires application inference profiles to be invoked by ARN; only system-defined cross-region profiles can use the short ID. Any discovered application profile will therefore be shown/selectable as a model but later ConverseStream validation fails with an invalid model identifier. Use summary.inference_profile_arn() for application profiles, or return ARNs for all profiles.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Support for bedrock application inference profiles could be the subject of another PR.
|
Good work here. A few notes from the authors of #9707 (Bedrock Mantle support), which this PR builds on:
|
|
Thanks for this — the model-discovery direction is solid, and clouatre's endorsement from the Mantle side is reassuring. Before we go deep on review, I want to flag a timing issue. @jamadeo is mid-flight on a fairly large refactor that moves providers out of the
This PR touches exactly the surfaces that refactor is reshaping: provider registration/inventory ( Could you take a look at Jack's branches above — especially #9986 and #9992 — and sketch how Bedrock discovery would slot into the new world? Bedrock is a bit special since it's the AWS SDK rather than an HTTP/REST shape, so it likely won't be a pure declarative JSON provider, but the I'm going to snooze this for a few days and loop Jack in so you two can align on sequencing. |
|
FWIW, I don't see an immediate conflict with the provider refactor. The relevant parts of inventory/models are merged, so as long as you're in sync with you should be good there. Soon enough we'll move the bedrock provider itself into goose-providers, but that hasn't started yet. |
|
thanks @stadgel-tc2 - this looks fine to me. I know @lifeizhou-ap is workign flat out in some of these areas so some conflicts not expected. Are you able to resolve these (I can resolve but not confident in hand testing, so ideally would get the ok from you that it is valid/works). But once clean - I think can merge it in. I will pre-approve this so once it is green it gets in. |
michaelneale
left a comment
There was a problem hiding this comment.
need to resolve conflicts and get a thumbs up from @stadgel-tc2 - but seems sensible to me, thorough improvement.
…n Bedrock model switching and provider model discovery.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6edd140d47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Thanks for the review @michaelneale . I reviewed and resolved the conflicts, from my side this is ok. The summary of the resolve is : Conflict resolutions: crates/goose-server/src/routes/config_management.rs crates/goose/tests/providers.rs ui/desktop/src/components/ModelAndProviderContext.tsx ui/desktop/src/components/settings/models/SwitchModelModal.tsx Cargo.lock: Some other follow-on changes: crates/goose/src/acp/server.rs crates/goose/src/acp/server/config.rs ui/desktop/src/components/settings/models/modelInterface.ts ui/desktop/src/acp/providers.ts ui/desktop/openapi.json and ui/desktop/src/api/types.gen.ts: crates/goose/tests/acp_custom_requests_test.rs and crates/goose/tests/acp_fixtures/server.rs: ui/desktop/src/i18n/messages/*.json: |
|
Hi @stadgel-tc2, We have to revert this PR from main branch as it has reference to the goosed (goose-server). more info in the revert pr description. Once our release is launched, would you please swap to use acp methods. We are sorry for the inconvenience. |
Fixes #9995
Summary
This PR improves Amazon Bedrock model discovery and validation so Bedrock behaves more like other Goose providers.
Details
Previously, the Bedrock provider used a hardcoded list of mostly us.* model IDs. This meant users in other regions could not reliably select valid regional inference profiles, and the picker could miss models available to their configured AWS account.
This change adds a Bedrock control-plane client and discovers:
The PR also validates selected models by invoking a small provider test before saving/applying the model in the server-backed UI paths. This catches AWS-side model access restrictions earlier, instead of failing only when the user sends their first chat message.
Testing
Related Issues
Not known.
Screenshots/Demos (for UX changes)
Before:
After: