fix: lazy provider creation for goose acp (#7026)#7066
fix: lazy provider creation for goose acp (#7026)#7066codefromthecrypt wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes ACP startup behavior by moving provider instantiation behind a lazy ProviderConstructor, and updates ACP initialization to advertise supported auth methods so registry validation can succeed.
Changes:
- Make provider creation lazy by passing a
ProviderConstructorintoGooseAcpAgentand only constructing a provider when a session needs it. - Add
authMethodsto the ACPinitializeresponse. - Update goose-acp test fixtures and add a new test covering initialize without an available provider.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/goose/src/providers/provider_registry.rs | Exposes ProviderConstructor publicly for cross-crate use. |
| crates/goose-acp/src/server.rs | Refactors agent construction to accept a provider factory; adds authMethods; introduces lazy provider setup via ensure_provider. |
| crates/goose-acp/src/server_factory.rs | Updates HTTP/WS server factory to build GooseAcpAgent using ProviderConstructor. |
| crates/goose-acp/tests/fixtures/mod.rs | Refactors in-process test server wiring to return a ready-to-use transport and adds initialize_agent helper. |
| crates/goose-acp/tests/fixtures/server.rs | Adapts tests to the updated in-process server spawn API. |
| crates/goose-acp/tests/server_test.rs | Adds test_initialize_without_provider and updates imports for new helpers/types. |
| let provider_name = global_config | ||
| .get_goose_provider() | ||
| .map_err(|e| anyhow::anyhow!("No provider configured: {}", e))?; | ||
|
|
||
| let provider_factory: ProviderConstructor = Arc::new(move |model_config| { | ||
| let provider_name = provider_name.clone(); | ||
| Box::pin(async move { goose::providers::create(&provider_name, model_config).await }) |
There was a problem hiding this comment.
AcpServer::create_agent() still fails fast on missing GOOSE_MODEL/GOOSE_PROVIDER, which prevents the HTTP/WebSocket ACP server from even starting in environments without a config file. If the intent is truly lazy provider/model resolution, consider moving these lookups into the ProviderConstructor (or into GooseAcpAgent::ensure_provider) so transport startup + initialize can succeed and only session creation fails with a helpful error.
| let provider_name = global_config | |
| .get_goose_provider() | |
| .map_err(|e| anyhow::anyhow!("No provider configured: {}", e))?; | |
| let provider_factory: ProviderConstructor = Arc::new(move |model_config| { | |
| let provider_name = provider_name.clone(); | |
| Box::pin(async move { goose::providers::create(&provider_name, model_config).await }) | |
| let provider_factory: ProviderConstructor = Arc::new(move |model_config| { | |
| Box::pin(async move { | |
| let global_config = Config::global(); | |
| let provider_name = global_config | |
| .get_goose_provider() | |
| .map_err(|e| anyhow::anyhow!("No provider configured: {}", e))?; | |
| goose::providers::create(&provider_name, model_config).await | |
| }) |
goose acp crashes on startup without provider config because it eagerly creates a provider at construction time. Make provider creation lazy via ProviderConstructor and add authMethods to the initialize response. Signed-off-by: Adrian Cole <adrian@tetrate.io>
43c91fb to
93010b0
Compare
| let provider_name = config.get_goose_provider().ok(); | ||
|
|
||
| let provider_factory: ProviderConstructor = Arc::new(move |model_config| { | ||
| let provider_name = provider_name.clone(); | ||
| Box::pin(async move { | ||
| let pn = provider_name.ok_or_else(|| anyhow::anyhow!("No provider configured"))?; | ||
| goose::providers::create(&pn, model_config).await | ||
| }) |
There was a problem hiding this comment.
provider_name is captured as an Option at agent creation time, so if the user configures a provider after initialize (e.g., runs goose configure while the ACP process stays running), the provider factory will still keep returning "No provider configured"; consider resolving get_goose_provider() inside the factory on each invocation (or re-reading the config file) so sessions can succeed without requiring a restart.
| let provider_name = config.get_goose_provider().ok(); | |
| let provider_factory: ProviderConstructor = Arc::new(move |model_config| { | |
| let provider_name = provider_name.clone(); | |
| Box::pin(async move { | |
| let pn = provider_name.ok_or_else(|| anyhow::anyhow!("No provider configured"))?; | |
| goose::providers::create(&pn, model_config).await | |
| }) | |
| let provider_factory: ProviderConstructor = Arc::new({ | |
| let config_path = config_path.clone(); | |
| move |model_config| { | |
| let config_path = config_path.clone(); | |
| Box::pin(async move { | |
| let config = goose::config::Config::new(&config_path, "goose")?; | |
| let provider_name = config | |
| .get_goose_provider() | |
| .ok_or_else(|| anyhow::anyhow!("No provider configured"))?; | |
| goose::providers::create(&provider_name, model_config).await | |
| }) | |
| } |
| // ensure_provider reads the model from config lazily, so tests need a | ||
| // config.yaml even though the factory ignores the model_config value. | ||
| let config_path = data_root.join("config.yaml"); | ||
| if !config_path.exists() { | ||
| fs::write(&config_path, "GOOSE_MODEL: gpt-5-nano\n").unwrap(); | ||
| } |
There was a problem hiding this comment.
This test helper hardcodes "config.yaml" instead of using goose::config::base::CONFIG_YAML_NAME, which can silently break tests if the config filename constant ever changes; prefer referencing the constant for consistency.
| async fn ensure_provider(&self, session: &Session) -> Result<()> { | ||
| let provider = match self.agent.provider().await { | ||
| Ok(p) => p, | ||
| Err(_) => { | ||
| // TODO: when session/set_model lands, use the client-provided | ||
| // modelId instead of the default read from config. | ||
| let config_path = self.config_dir.join(CONFIG_YAML_NAME); | ||
| let config = Config::new(&config_path, "goose")?; | ||
| let model_config = goose::model::ModelConfig::new(&config.get_goose_model()?)?; | ||
| (self.provider_factory)(model_config).await? | ||
| } |
There was a problem hiding this comment.
ensure_provider can race under concurrent session creation: two tasks can both observe Agent::provider() as unset and invoke provider_factory, potentially creating multiple providers unnecessarily; consider guarding initialization with a shared async lock/OnceCell so provider creation happens at most once per process.
Summary
Before,
goose acpcrashes on startup when there is not yet Goose configuration.This PR makes provider creation lazy until an ACP session needs it.
This also adds missing ACP
authMethodsis added to theinitializeresponse so ACP clients can prompt for setup.Type of Change
AI Assistance
Testing
New
test_initialize_without_providertest verifies configuration isn't required to initialize ACP.ACP registry validation against local build:
Example output:
Related Issues
Fixes #7026
Unblocks agentclientprotocol/registry#23