Restore Bedrock model discovery using ACP - #10201
Conversation
…he Converse path 2)Apply the probe timeout to stream creation too
…n Bedrock model switching and provider model discovery.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d827ee6b7b
ℹ️ 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".
| InventoryRegistration::new(true, || { | ||
| let config = Config::global(); | ||
| let metadata = BedrockProvider::metadata(); | ||
| Ok(default_inventory_identity( | ||
| BEDROCK_PROVIDER_NAME, | ||
| BEDROCK_PROVIDER_NAME, | ||
| &metadata.config_keys, | ||
| config, | ||
| )) | ||
| }) | ||
| } |
There was a problem hiding this comment.
When this registration relies on the default inventory configured resolver, Bedrock is treated as configured whenever all required config keys are satisfied; its only required key is AWS_REGION, and that key has a metadata default. As a result, ACP providers/list reports aws_bedrock as configured even for users with no Bedrock auth source, so the desktop model picker includes Bedrock/bootstrap models and lets users save a provider that will fail when the agent is created. Add a Bedrock with_configured resolver that requires an actual AWS/bearer-token credential signal or otherwise mirrors the existing provider-configured logic.
Useful? React with 👍 / 👎.
|
Hi @lifeizhou-ap, I restored the Bedrock model discovery work on top of current main and removed the desktop dependency on goosed/OpenAPI APIs. The model picker now uses ACP provider inventory/canonical model info, and preflight validation is intentionally left out of this PR. |
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
Thanks for reworking this on top of the ACP provider inventory path after #10176. Comparing this with #9997, the ACP-side model picker changes look like the right direction, but I think this needs another pass before we can merge.
The main blocker is the Bedrock configured-state issue Codex pointed out. bedrock_inventory() currently uses the default configured resolver, and AWS_REGION is marked required but has a default value. That means the inventory can treat Bedrock as configured even when there are no usable AWS credentials and no AWS_BEARER_TOKEN_BEDROCK, which would expose Bedrock refresh/model-picker behavior to users who cannot actually use the provider. Please add a Bedrock-specific with_configured resolver that reflects the actual auth modes we support.
There is also a scope/description mismatch around live provider/model validation. The PR description says the preflight live model validation was dropped, which is true for the desktop ACP path compared with #9997, but the server-side changes from the original PR are still here: check_provider accepts an optional model, set_config_provider calls test_provider_model, and update_agent_provider calls test_provider_model. If those non-desktop/goosed paths are intentionally kept, please update the PR description and explain why they belong in this PR. If they are leftovers from #9997, please remove them. Relatedly, ui/desktop/openapi.json is still updated, so it would be good to be explicit about whether the remaining OpenAPI/server route changes are intentional.
A smaller cleanup request: please trim comments that only restate the code mechanics, especially in the Bedrock stream/event-processing additions. Comments are useful for surprising behavior, but a number of these explain what the next few lines directly do. The tests around stream event processing are also quite implementation-detail heavy; where possible, prefer tests that exercise externally visible provider behavior rather than locking down the internal event accumulator shape.
Once those are addressed, I think this is reviewable as the ACP-compatible re-application of #9997 rather than a new/unrelated feature.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff9456817c
ℹ️ 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".
| Ok(default_inventory_identity( | ||
| BEDROCK_PROVIDER_NAME, | ||
| BEDROCK_PROVIDER_NAME, | ||
| &metadata.config_keys, | ||
| config, |
There was a problem hiding this comment.
When Bedrock is configured via standard AWS access-key credentials, the new configured resolver can return true because AWS_ACCESS_KEY_ID is present, but this identity is still derived only from Bedrock metadata keys, which do not include AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY. That makes the inventory cache key identical across different AWS accounts for the same region/profile inputs, so switching credentials can keep showing and saving a model list discovered under a previous account even though Bedrock model access is account-specific.
Useful? React with 👍 / 👎.
| tokio::select! { | ||
| response = &mut prompt => { | ||
| final_response = Some(response.unwrap()); | ||
| break; | ||
| } | ||
| _ = tokio::time::sleep(Duration::from_millis(10)), if !steer_sent => { | ||
| let updates = session.session_updates(); | ||
| biased; | ||
| updates = session.wait_for_session_updates(), if !steer_sent => { | ||
| if let Some(run_id) = updates.iter().find_map(active_run_id_from_update) { |
There was a problem hiding this comment.
If this regression test hits the failure mode it is trying to guard against and the prompt neither completes nor emits a session update with an active run, this select! can block forever inside wait_for_session_updates(), so the outer 3-second deadline is never rechecked. The previous sleep branch let the loop fail with the existing assertions; add a timeout/sleep branch or wrap the wait so CI fails instead of hanging.
Useful? React with 👍 / 👎.
Superseded by follow-up cleanup in ff94568; leaving current status in a fresh PR comment.
|
Update after cleanup in Current status: this still needs a small follow-up before review. Codex has two current points that look valid: when Bedrock is configured via AWS access keys, the inventory identity should include the AWS credential inputs (or the configured signal should avoid access keys) so model caches do not cross AWS accounts; and the ACP regression test change should preserve the timeout while waiting for session updates so CI cannot hang. Snoozing this for 3 days to give the author a chance to address those. |
|
Thanks for the update. I am out of work until the 20th of July, so I will only be able to address these after that. Feel free to implement if needed, otherwise I will come back to his that time. |
|
This pull request has been automatically marked as stale because it has not had recent activity for 23 days. What happens next?
Thank you for your contribution! 🚀 |
|
Thanks for the contribution. I'll close this for now since there are still review comments to address and the branch has conflicts. Feel free to reopen when you have time to update it. |
3 participants
Summary
Context
The previous PR was reverted because the desktop UI still referenced goosed APIs while the release moved UI connectivity to ACP. This update removes the model-picker dependency on generated goosed API clients.
Testing
cargo test -p goose --test providersstill has unrelated external CLI provider failures forcodexandclaude-code