From 2f4e1fdd293fce7f9734a4366d0652092c20514c Mon Sep 17 00:00:00 2001 From: Jack Amadeo Date: Wed, 3 Jun 2026 12:06:35 -0400 Subject: [PATCH] plan revision --- PLAN.md | 240 +++++++++++++++++++------------------------------------- 1 file changed, 80 insertions(+), 160 deletions(-) diff --git a/PLAN.md b/PLAN.md index 92cb9f928f..7355a98b30 100644 --- a/PLAN.md +++ b/PLAN.md @@ -3,7 +3,7 @@ Extract `crates/goose/src/providers` into a new workspace crate, `goose-providers`, with **no dependency on `goose`**. The new crate should expose the provider API centered on `Provider` from `providers/base.rs`, plus the types required to call it. Any current dependencies from providers back into `goose` must either: 1. Move into `goose-providers` because they are part of the provider API/model. -2. Be replaced by injected interfaces/runtime services. +2. Be replaced by injected config, or by provider-specific builders/setup APIs outside the shared runtime. 3. Stay in `goose`, with `goose` adapting to `goose-providers`. The largest required redesign is configuration: provider code should no longer read/write `crate::config::Config::global()` directly. Config must be injected. @@ -16,22 +16,22 @@ I inspected `crates/goose/src/providers`. The provider module currently referenc | Goose dependency | Approx. role in providers | Proposed treatment | |---------------------------|--------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------| -| `crate::config` | Config read/write, secrets, paths, search paths, `ExtensionConfig`, `GooseMode`, declarative provider config | Split: inject config/paths/search; move/provider-owned DTOs for mode and declarative config; convert extension config in `goose` | +| `crate::config` | Config read/write, secrets, paths, search paths, `ExtensionConfig`, `GooseMode`, declarative provider config | Inject config only in the shared provider runtime; defer paths/search/mode/extensions to provider-specific builders or keep those providers in `goose` until moved | | `crate::conversation` | `Message`, `MessageContent`, `Conversation`, tool request/response content | Move API model types into `goose-providers`; `goose` reexports/uses them | -| `crate::model` | `ModelConfig`, `ThinkingEffort`, canonical model lookup, env-derived model settings | Move `ModelConfig`/`ThinkingEffort`; remove hidden config reads; inject model defaults | +| `crate::model` | `ModelConfig`, `ThinkingEffort`, canonical model lookup, env-derived model settings | Move `ModelConfig`/`ThinkingEffort`; remove hidden config reads; resolve defaults through injected config where needed | | `crate::providers` | Internal provider modules | Becomes local module references inside `goose-providers` | | `crate::session_context` | `SESSION_ID_HEADER` for HTTP providers | Move constant to `goose-providers` | | `crate::utils` | `safe_truncate`, `bytes_to_hex`, unicode sanitization | Move small pure helpers or inline/replace | -| `crate::subprocess` | Process setup for CLI/ACP providers | Inject process configuration/command spawning service, or move only pure process helper | +| `crate::subprocess` | Process setup for CLI/ACP providers | Do not add to shared runtime; CLI/ACP providers need provider-specific builders/setup, or stay in `goose` until redesigned | | `crate::mcp_utils` | `ToolResult`, resource text extraction | Move provider-facing tool/resource helpers | -| `crate::acp` | ACP subprocess/session providers | Do not move wholesale initially; define injected ACP session/client factory | +| `crate::acp` | ACP subprocess/session providers | Do not move wholesale initially; ACP providers need their own builder/factory design outside the config-only core runtime | | `crate::permission` | Permission confirmation types exposed by `Provider` | Move provider-facing permission DTOs | -| `crate::agents` | `ExtensionConfig`, `Envs`, test fixtures | Do not move goose agent config; introduce provider-owned extension/MCP DTO and convert in `goose` | -| `crate::prompt_template` | Session naming prompt and local inference prompt | Move prompt strings or inject prompt renderer | -| `crate::token_counter` | Usage estimation fallback | Inject token counter/usage estimator or move pure estimator behind provider trait | +| `crate::agents` | `ExtensionConfig`, `Envs`, test fixtures | Do not move or redesign initially; providers needing extensions declare provider-specific builders when they are migrated | +| `crate::prompt_template` | Session naming prompt and local inference prompt | Move pure prompt handling or keep orchestration in `goose`; do not add prompt rendering to shared runtime | +| `crate::token_counter` | Usage estimation fallback | Move out of provider defaults or keep in `goose`; do not add token counting to shared runtime | | `crate::session` | Provider inventory persistence | Keep storage/service in `goose` or abstract storage | -| `crate::instance_id` | Databricks request ID | Inject instance id | -| `crate::download_manager` | Local inference model downloads | Inject download/model-cache service, or defer local inference extraction | +| `crate::instance_id` | Databricks request ID | Pass through a Databricks-specific builder if/when Databricks moves; do not add to shared runtime | +| `crate::download_manager` | Local inference model downloads | Defer local inference extraction or use local-inference-specific builder/setup; do not add to shared runtime | --- # Primary API Impact @@ -71,7 +71,14 @@ pub trait Provider: Send + Sync { } ``` -Because `Provider` directly uses `ModelConfig`, `Message`, `Conversation`, `GooseMode`, and `PermissionConfirmation`, those types cannot remain only in `goose` if `goose-providers` is independent. We need to move or replace them. +Because `Provider` directly uses `ModelConfig`, `Message`, `Conversation`, `GooseMode`, and `PermissionConfirmation`, those dependencies must be resolved before the trait can live in `goose-providers`. Move the provider API/model types that are intrinsic to completions, and remove/defer goose orchestration hooks like mode changes from the core provider trait. + +## Provider trait changes + +- Keep `stream`, `complete`, `complete_fast`, model metadata, OAuth/credential refresh, and model inventory methods in the core trait. +- Move `ModelConfig`, `Message`, `MessageContent`, `Conversation`, usage, metadata, errors, and retry types into `goose-providers`. +- Remove `update_mode(&self, ..., GooseMode)` from the initial `goose-providers::Provider` trait. Mode is a goose orchestration concern and should be handled by provider-specific builders or goose-side adapter traits for CLI/ACP providers. +- Keep permission confirmation only if it remains part of the provider-facing API without importing goose permission modules; otherwise move it to a provider-specific extension trait used by the affected adapters. ## Types that should move to `goose-providers` @@ -167,7 +174,7 @@ pub trait ProviderConfigStore: Send + Sync { } ``` -Then add generic extension helpers: +Then add typed convenience helpers for callers that want deserialized values rather than raw `serde_json::Value`: ```rust pub trait ProviderConfigExt { @@ -218,36 +225,25 @@ This preserves important current behavior, such as distinguishing “not found # Runtime Injection -Config alone is not enough. Several providers need filesystem paths, command resolution, process setup, prompt rendering, token counting, ACP sessions, and downloads. +The shared runtime for `goose-providers` should be intentionally small: **only configuration is injected**. The purpose of the initial extraction is to support providers whose only goose dependency is reading/writing config and secrets. -Introduce a provider runtime object: +Do not add filesystem paths, command resolution, process management, prompt rendering, token counting, ACP sessions, downloads, instance IDs, current mode, or extension resolution to the common runtime. Providers that need those things should either remain in `goose` for the first extraction or define their own explicit builders/setup APIs when they are migrated. + +The common runtime should look like: ```rust pub struct ProviderRuntime { pub config: Arc, - pub paths: Arc, - pub commands: Arc, - pub process: Arc, - pub prompts: Arc, - pub token_counter: Arc, - pub acp: Option>, - pub downloads: Option>, - pub instance_id: Option, - pub mode: ProviderMode, } ``` -Some fields can be optional or split into smaller structs to avoid forcing simple HTTP providers to care about ACP/downloads. - ## Provider construction -Replace `ProviderDef::from_env` with a context-based constructor: +Replace `ProviderDef::from_env` with a config-based constructor for the simple/provider-common path: ```rust pub struct ProviderInit { pub model: ModelConfig, - pub extensions: Vec, - pub working_dir: Option, pub runtime: Arc, } @@ -274,39 +270,17 @@ pub trait ProviderDef: Send + Sync { } ``` -This renames the current misleading `from_env`: providers will no longer own global config/env lookup. +This renames the current misleading `from_env`: providers will no longer own global config/env lookup. It also keeps the initial API scoped to providers that can be built from `ModelConfig` plus injected config. More complex providers can expose provider-specific builders later, for example a CLI provider builder that accepts a command resolver, working directory, mode, and extension data. --- -# Extension Config Redesign +# Extension Handling -Current constructors take `Vec`, which is actually reexported from `crate::agents::ExtensionConfig`. That type is part of the goose agent configuration model and should not move wholesale into `goose-providers`. +Do not implement a shared extension DTO or extension conversion layer in the first `goose-providers` design. Current constructors take `Vec`, but that type belongs to goose agent orchestration rather than provider-core configuration. -Instead define a provider-owned DTO with only what providers need: +For the first extraction, design only for providers that can be created from `ModelConfig` plus an injected config object. Providers that need extension data, MCP server config, working directories, command resolution, or mode should declare their own builders/setup APIs when they move. Until those builders exist, those providers can stay in `goose` as adapters around the new provider API types. -```rust -pub enum ProviderExtensionConfig { - Stdio { - name: String, - cmd: String, - args: Vec, - env: std::collections::HashMap, - timeout: Option, - available_tools: Vec, - }, - StreamableHttp { - name: String, - uri: String, - headers: std::collections::HashMap, - timeout: Option, - available_tools: Vec, - }, -} -``` - -Then `goose` converts from `goose::agents::ExtensionConfig` to `ProviderExtensionConfig` before constructing providers. - -This isolates provider crates from goose agent config while still supporting Codex/Claude/ACP MCP configuration generation. +This avoids prematurely creating a provider-owned extension schema that may not fit every subprocess/ACP provider. --- @@ -379,10 +353,9 @@ impl ModelConfigResolver { ### Plan - Replace direct `Config::global()` calls with injected `ProviderConfigStore`. -- Replace `Paths` with injected `ProviderPaths`. -- Replace `SearchPaths` with injected `CommandResolver`. -- Move/rename `GooseMode` to provider-owned `ProviderMode`. -- Do not move goose `ExtensionConfig`; convert to provider-owned `ProviderExtensionConfig`. +- Do not add shared runtime services for `Paths`, `SearchPaths`, or process setup. Providers that need them should stay in `goose` initially or define provider-specific builders when migrated. +- Do not move/rename `GooseMode` as part of the first extraction; providers that need mode should accept it through their own builders. +- Do not move goose `ExtensionConfig` or define a shared provider extension DTO initially; providers that need extension data should accept it through their own builders. - Replace typed config wrapper dependencies with constants/defaults in providers, or use generic config keys through `ProviderConfigStore`. - Move declarative provider schema pieces that are provider-specific into `goose-providers`; keep file CRUD and UI/server management in `goose` unless abstracted. @@ -449,13 +422,13 @@ After extraction these become local `crate::...` or `super::...` references in ` Specific notes: - `errors`, `retry`, `api_client`, `http_status`, `formats`, `canonical`, `catalog`, most `utils` should move. -- `provider_registry` should move but be redesigned around injected runtime. +- `provider_registry` should move but be redesigned around config-only `ProviderRuntime` for the common constructor path. - `init` should split: - provider registration/bootstrap can live in `goose-providers`; - global singleton initialization with goose config/custom providers may stay in `goose`. - `inventory` should split: - provider inventory DTOs/identity helpers move; - - refresh service that depends on `SessionStorage` stays in `goose` or gets storage injection. + - refresh service that depends on `SessionStorage` stays in `goose`; do not add storage to the shared provider runtime. --- @@ -502,22 +475,12 @@ Move these small pure helpers into `goose-providers::utils`, or inline them in r ### Plan -Do not make providers call goose subprocess helpers directly. +Do not add process configuration or command spawning to the shared `ProviderRuntime`. CLI/subprocess providers should either: -Introduce: +- stay in `goose` during the initial extraction, or +- expose provider-specific builders that accept exactly the non-config dependencies they need, such as resolved command paths, working directory, mode, or process configuration hooks. -```rust -pub trait ProcessConfigurator: Send + Sync { - fn configure(&self, command: &mut tokio::process::Command); -} -``` - -`goose` implements this with existing `crate::subprocess::configure_subprocess`. - -For `SubprocessExt`, either: - -- move it if it is generic/pure, or -- replace usage with a provider-local helper. +For generic subprocess helpers, move only code that is truly independent of `goose`; otherwise keep it in `goose` and call it before constructing the provider. --- @@ -546,25 +509,9 @@ ACP providers (`amp_acp`, `claude_acp`, `codex_acp`, `copilot_acp`, `pi_acp`) ca ### Plan -Do not move `crate::acp` wholesale as an implementation detail. +Do not move `crate::acp` wholesale and do not add ACP session factories to the shared `ProviderRuntime`. ACP providers are outside the config-only first pass. -Add provider-facing ACP abstractions: - -```rust -#[async_trait] -pub trait AcpSessionFactory: Send + Sync { - async fn start_session( - &self, - config: AcpProviderLaunchConfig, - ) -> Result, ProviderError>; -} -``` - -ACP provider implementations in `goose-providers` depend only on this trait. - -`goose` implements the factory using current `crate::acp`. - -If this abstraction is too large for the first extraction, keep ACP providers in `goose` temporarily as adapters implementing `goose_providers::Provider`, then migrate them in a later phase. +For the initial extraction, keep ACP providers in `goose` as adapters around `goose_providers::Provider` types if necessary. When migrating ACP providers later, design provider-specific builders/factories that accept ACP session setup, command resolution, mode, and extension data explicitly. --- @@ -596,11 +543,9 @@ Move provider-facing permission DTOs to `goose-providers`: ### Plan -Do not move goose agent config. +Do not move goose agent config and do not define a shared provider extension DTO in the first extraction. Providers that need extension or MCP server data should remain in `goose` initially or define provider-specific builders when they are migrated. -Define provider-owned extension DTOs as described above. - -Replace tests that import goose platform-specific types with raw JSON fixtures or provider-local test structs. +Replace tests that import goose platform-specific types with raw JSON fixtures or provider-local test structs where possible. --- @@ -615,9 +560,9 @@ Replace tests that import goose platform-specific types with raw JSON fixtures o Preferred: move session naming out of the `Provider` trait into goose orchestration. Providers should only complete prompts; goose can build the session-name prompt. -Alternative if preserving `Provider::generate_session_name`: inject `PromptRenderer`. +If preserving `Provider::generate_session_name`, move the needed prompt content or rendering logic into `goose-providers`, or keep session naming in `goose`. Do not add prompt rendering to `ProviderRuntime`. -For local inference, either move the tiny prompt content into `goose-providers`, or inject prompt content through runtime. +For local inference, either move the tiny prompt content into `goose-providers` as part of local-inference-specific migration or keep local inference in `goose` until it has its own setup API. --- @@ -634,23 +579,9 @@ Remove direct dependency from `ProviderUsage`. Options: 1. Move token counting into `goose-providers` if it is sufficiently provider-generic. -2. Better: inject a `TokenCounter`/`UsageEstimator` trait. +2. Keep fallback usage estimation in `goose` and call it outside provider defaults. -Example: - -```rust -#[async_trait] -pub trait UsageEstimator: Send + Sync { - async fn ensure_usage_tokens( - &self, - usage: &mut ProviderUsage, - system_prompt: &str, - request_messages: &[Message], - response: &Message, - tools: &[Tool], - ) -> Result<(), ProviderError>; -} -``` +Do not add a token counter or usage estimator to the shared `ProviderRuntime`. --- @@ -676,9 +607,7 @@ Databricks optionally includes a client request ID based on goose instance id. ### Plan -Inject `instance_id: Option` into `ProviderRuntime`. - -Databricks reads `runtime.instance_id` instead of calling `get_instance_id()`. +Do not add instance ID to `ProviderRuntime`. When Databricks is migrated, pass the instance ID through a Databricks-specific builder or keep that request-ID behavior in a goose-side adapter. --- @@ -692,12 +621,7 @@ Local inference model registry/downloads. Local inference is likely the hardest provider family to extract because it depends on model registry paths and downloads. -Options: - -1. Add `DownloadManager` and `LocalModelStore` traits to `goose-providers`, implemented by `goose`. -2. Defer local inference extraction behind a compatibility layer until HTTP/subprocess providers are migrated. - -I recommend deferring local inference until after the main provider API extraction. +Defer local inference extraction behind a compatibility layer until HTTP/config-only providers are migrated. If local inference moves later, give it a local-inference-specific builder/setup API for model storage and downloads rather than adding download services to the shared `ProviderRuntime`. --- @@ -715,7 +639,7 @@ Create `crates/goose-providers` with: - `message` - `mcp_utils` - provider-facing permission types -- provider runtime/config traits +- config trait and config-only provider runtime - canonical model registry/data - provider metadata types @@ -741,29 +665,31 @@ Any default trait method in `goose-providers` must not call into goose. --- -## Phase 3: Introduce goose runtime adapter +## Phase 3: Introduce goose config adapter -In `goose`, add an adapter layer: +In `goose`, add an adapter from `crate::config::Config` to the `goose-providers` config trait: ```rust -pub struct GooseProviderRuntime { +pub struct GooseProviderConfig { config: &'static goose::config::Config, - ... +} + +impl ProviderConfigStore for GooseProviderConfig { + // delegate get_param/get_secret/set_param/set_secret/etc to goose config } ``` -Implement: +Use that adapter to build the config-only provider runtime: -- `ProviderConfigStore` -- `ProviderPaths` -- `CommandResolver` -- `ProcessConfigurator` -- `PromptRenderer` -- `TokenCounter`/`UsageEstimator` -- `AcpSessionFactory` -- `DownloadManager`, later +```rust +let runtime = ProviderRuntime { + config: Arc::new(GooseProviderConfig { + config: goose::config::Config::global(), + }), +}; +``` -This gives provider constructors everything they need without importing `goose`. +Do not implement shared provider runtime adapters for paths, command resolution, process setup, prompts, token counting, ACP, downloads, mode, instance id, or extensions. Providers that require more than config should not use the common constructor path until they define explicit provider-specific builders. --- @@ -811,7 +737,7 @@ For each: - Store needed runtime/config handles in the provider struct only when used after construction. - Replace direct cache invalidation with `runtime.config.invalidate_secrets_cache()`. - Replace `SESSION_ID_HEADER` import with moved constant. -- Replace `Paths` with `runtime.paths`. +- Do not move providers that require path/process/extension setup through this common path yet; migrate them later with provider-specific builders. --- @@ -830,10 +756,10 @@ Providers: Main work: -- Replace file paths with injected `ProviderPaths`. +- Move only OAuth/token providers that can be expressed with config reads/writes alone. - Replace secret writes with `ProviderConfigStore::set_secret_value`. - Replace marker writes with `set_param_value`. -- Ensure config write capability is explicitly part of runtime. +- Providers with token cache files or browser/process setup should stay in `goose` until they have provider-specific builders. --- @@ -849,12 +775,10 @@ Providers: Main work: -- Replace `SearchPaths` with `CommandResolver`. -- Replace `configure_subprocess` with `ProcessConfigurator`. -- Replace `ExtensionConfig` with `ProviderExtensionConfig`. -- Replace `GooseMode` with `ProviderMode`. -- Move permission DTOs. -- Move or inject prompt/session-name helpers. +- Do not add command resolution, process setup, extensions, or mode to `ProviderRuntime`. +- Define provider-specific builders for each CLI provider that needs command resolution, working directory, mode, and/or extension data. +- Move permission DTOs only where they are part of the public provider API. +- Move session-name orchestration out of provider defaults or keep it in `goose`. --- @@ -870,9 +794,9 @@ Providers: Main work: -- Add `AcpSessionFactory`. -- Convert extensions to provider-owned MCP server configs. -- Replace command resolution/mode/config calls with runtime services. +- Do not add ACP sessions, command resolution, mode, or extensions to the shared runtime. +- Keep ACP providers in `goose` initially, or migrate each with an ACP-specific builder/factory API. +- Use injected config only for ACP settings that are simple config reads/writes. If needed, keep these as temporary wrappers in `goose` while the rest of `goose-providers` lands. @@ -914,14 +838,13 @@ Current `providers/init.rs` owns a global `ProviderRegistry` and calls `register - declarative provider schema and construction logic - provider metadata -But registry constructors should accept `ProviderInit`/`ProviderRuntime`. +Registry constructors should accept the common `ProviderInit`/`ProviderRuntime` only for config-only providers. More complex providers can register custom constructors built by provider-specific setup code. ### In `goose` - global registry singleton - loading custom provider files from goose config dirs - creating `ProviderRuntime` -- converting goose `ExtensionConfig` to provider DTOs - server/CLI integration - inventory refresh persistence @@ -1075,10 +998,7 @@ Provider request formatting, HTTP clients, OAuth flows, model metadata, and prov ## ACP providers -ACP currently appears tightly coupled to goose internals. We should either: - -- introduce `AcpSessionFactory`, or -- leave ACP providers in `goose` temporarily while the API crate lands. +ACP currently appears tightly coupled to goose internals. Keep ACP providers in `goose` temporarily while the API crate lands, or migrate them later with ACP-specific builders/factories. Do not introduce an ACP factory into the shared `ProviderRuntime`. Trying to move all ACP internals immediately will likely balloon the scope. @@ -1122,7 +1042,7 @@ goose-providers ──> external crates only - provider errors/retry - provider metadata/canonical models - provider-owned message/model types -- provider runtime abstraction traits +- config-only provider runtime abstraction `goose` contains: @@ -1131,7 +1051,7 @@ goose-providers ──> external crates only - agent orchestration - extension resolution - UI/server/CLI integration -- implementations of injected runtime services +- config adapter implementation and provider-specific setup/builders for complex providers --- @@ -1146,7 +1066,7 @@ A good first PR should avoid moving every provider immediately. I would do: - `providers/base.rs` core types, with goose dependencies removed/stubbed behind traits - `model.rs`, made pure - conversation message types needed by `Provider` -3. Add `ProviderConfigStore`, `ProviderPaths`, and `ProviderRuntime`. +3. Add `ProviderConfigStore` and config-only `ProviderRuntime`. 4. Add goose adapter implementations. 5. Reexport moved types from `goose`. 6. Keep existing provider implementations in `goose` temporarily but make them compile against `goose_providers` types.