feat: MCP support for agentic CLI providers#6972
Conversation
a08b0a4 to
c7c2027
Compare
There was a problem hiding this comment.
Pull request overview
This PR enables MCP (Model Context Protocol) support for agentic CLI providers (claude-code and codex) by threading ExtensionConfig through the provider creation pipeline. Previously, only LLM providers could use MCP extensions because CLI providers lacked access to extensions at construction time.
Changes:
- Modified
ProviderDef::from_envto acceptVec<ExtensionConfig>, enabling CLI providers to connect to MCP servers at creation time - Extracted shared test infrastructure (
McpFixture,ExpectedSessionId) intogoose-test-supportcrate - Updated all providers to use
#[async_trait]instead of manually returningBoxFuture, reducing boilerplate
Reviewed changes
Copilot reviewed 62 out of 64 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
crates/goose/src/providers/base.rs |
Added #[async_trait] and extensions parameter to ProviderDef::from_env |
crates/goose/src/providers/codex.rs |
Added TOML config overrides for MCP, refactored prepare_input to handle images/text in single pass |
crates/goose/src/providers/claude_code.rs |
Switched to persistent stream-json session with OnceCell, added JSON MCP config generation |
crates/goose/src/providers/init.rs |
Updated create functions to accept and pass through extensions |
crates/goose/src/scheduler.rs |
Reordered to create provider after resolving extensions |
crates/goose-test-support/ |
New shared crate for test fixtures (MCP server, session ID validation, test assets) |
crates/goose/tests/providers.rs |
Extended test suite to cover CLI providers with MCP tool usage |
| All other provider files | Updated from_env signatures to accept unused _extensions parameter |
|
I will split off all the groundwork into separate PRs to make this easier to review and merge |
|
#7019 pulls the MCP fixture refactoring out |
c7c2027 to
9e13ce5
Compare
|
#7029 for claude refactor |
|
codex fixes mostly image related #7033 |
ce578c7 to
72830bf
Compare
|
@michaelneale I think this is ready now |
|
last rebase I accidentally deleted my code that added these two to providers.rs. will add it back |
72830bf to
2ec3875
Compare
2ec3875 to
86e2e6b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 54 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
crates/goose/src/providers/provider_registry.rs:136
- Custom/declarative providers currently drop the passed
extensions(_extensionsis unused), so they still cannot receive extensions at creation time despite the PR goal; consider threadingextensionsinto the custom provider constructor or documenting that extensions are intentionally unsupported for declarative providers.
Pull request was converted to draft
|
copilot helped find an ENV resolution issue which is not sorted and we shuold have a smoke test/integration for I think. This catches it and now fixed: but now it found a problem in how, which I need to sort. yes I need to separate ENV/secret resolution. however, I have to do that carefully so that the resolved values are not accidentally written back to goose config. marking draft until I sort that out. |
870352e to
e9d7737
Compare
|
hoping to not have missed anything else but will leave in draft until next copilot and I pick up tomorrow. Most notably I added a "Extension env_keys not persisted in DB" section which safeguards accidental mishandling. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 55 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/goose/src/agents/extension_manager.rs:496
config.key()can still be empty when anExtensionConfighas an empty/whitespacename(e.g., sessions created before CLI-derived names, or user-supplied configs), which will store the extension under an empty key and may cause collisions or skipped loads; consider rejecting empty keys with a clear error or falling back to a derived name (URI host / cmd basename) as a compatibility measure.
let sanitized_name = config.key();
if self.extensions.lock().await.contains_key(&sanitized_name) {
return Ok(());
}
Thread ExtensionConfig through ProviderDef::from_env so CLI providers (claude-code, codex) connect to MCP servers at construction time and call tools internally. Extract McpFixture and test assets into goose-test-support crate, shared by goose-acp and providers.rs integration tests. Both CLI providers now run the full test suite (basic response, tool usage, context length, image content) against a real MCP fixture server. Improve claude_code.rs with persistent stream-json sessions and content blocks. Replace codex.rs two-pass message handling with single-pass prepare_input. Signed-off-by: Adrian Cole <adrian@tetrate.io>
Signed-off-by: Adrian Cole <adrian@tetrate.io>
Signed-off-by: Adrian Cole <adrian@tetrate.io>
e9d7737 to
313f031
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 55 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
crates/goose/src/agents/extension_manager.rs:496
ExtensionManager::add_extensionusesconfig.key()as the map key and silently returnsOk(())when that key already exists; with the new CLI-derived names (often just the command basename likenpx) this can drop additional extensions without warning, and legacy sessions with empty names can produce an empty key. Consider rejecting empty keys and either erroring on collisions or auto-disambiguating (e.g., suffixing / incorporating args/uri) instead of silently skipping.
let sanitized_name = config.key();
if self.extensions.lock().await.contains_key(&sanitized_name) {
return Ok(());
}
| } | ||
|
|
||
| pub fn parse_streamable_http_extension(extension_url: &str, timeout: u64) -> ExtensionConfig { | ||
| let name = url::Url::parse(extension_url) |
There was a problem hiding this comment.
before we had logic in two places about backfilling name on empty (only possible with anonymous CLI extensions). This puts the logic in the ctor instead.
| ; "name_from_cmd_basename" | ||
| )] | ||
| #[test_case( | ||
| "MY_SECRET=s3cret npx -y @modelcontextprotocol/server-everything", |
There was a problem hiding this comment.
this test case was missing and tripped me up for a while. basically it is fine for us to pass ENV the user sets like this as we have no way to know if it is secret or not (copilot is over-zealous on the secret concept). The thing to focus on is env_keys which do read from the secrets when there's no ENV associated.
| .and_then(|s| EnabledExtensionsState::from_extension_data(&s.extension_data)) | ||
| .map(|state| state.extensions) | ||
| .unwrap_or_else(get_enabled_extensions) | ||
| EnabledExtensionsState::for_session( |
There was a problem hiding this comment.
added this to avoid so many copy/paste
| let extensions = EnabledExtensionsState::from_extension_data(&session.extension_data) | ||
| .map(|state| state.extensions) | ||
| .unwrap_or_else(goose::config::get_enabled_extensions); | ||
| let extensions = EnabledExtensionsState::extensions_or_default( |
There was a problem hiding this comment.
Yes, same behavior. extensions_or_default does the same fallback internally. This just deduplicates the pattern that was copy/pasted across 5 call sites.
michaelneale
left a comment
There was a problem hiding this comment.
large change but logically makes sense
* main: fix text editor view broken (#7167) docs: White label guide (#6857) Add PATH detection back to developer extension (#7161) docs: pin version in ci/cd (#7168) Desktop: - No Custom Headers field for custom OpenAI-compatible providers (#6681) feat: edit model and extensions of a recipe from GUI (#6804) feat: MCP support for agentic CLI providers (#6972)
* origin/main: (33 commits) fix: replace panic with proper error handling in get_tokenizer (#7175) Lifei/smoke test for developer (#7174) fix text editor view broken (#7167) docs: White label guide (#6857) Add PATH detection back to developer extension (#7161) docs: pin version in ci/cd (#7168) Desktop: - No Custom Headers field for custom OpenAI-compatible providers (#6681) feat: edit model and extensions of a recipe from GUI (#6804) feat: MCP support for agentic CLI providers (#6972) docs: keyring fallback to secrets.yaml (#7165) feat: load provider/model specified inside the recipe config (#6884) fix ask-ai bot hitting tool call limits (#7162) fix flatpak icon (#7154) [docs] Skills Marketplace UI Improvements (#7158) More no-window flags (#7122) feat: Allow overriding default bat themes using environment variables (#7140) Make the system prompt smaller (#6991) Pre release script (#7145) Spelling (#7137) feat(mcp): upgrade rmcp to 0.15.0 and advertise MCP Apps UI extension capability (#6927) ...
Summary
Agentic CLI providers (
claude-code,codex) can now pass MCP extensions to the underlying agent!This works by adding
Vec<ExtensionConfig>toProviderDef::from_env, and threading it as necessary. This allows agentic providers to parse MCP extensions.claude_code.rsconverts extensions to--mcp-configJSONcodex.rsconverts them to-c mcp_servers.*TOML overrides.There was another dependency to avoid empty key names in the MCP config;
key()always has a name to normalize.env_keysare resolved at runtime by the extension manager, never persisted to the session database.Type of Change
AI Assistance
Testing
Codex
Claude Code
Extension env_keys not persisted in DB
Extensions configured with
env_keysresolve secrets from the environment at runtimewithout persisting them to sqlite. This verifies that
env_keysstays intact andresolved secrets don't appear in
envs.Expected output shows
env_keys: ["MY_SECRET"]andenvs: {}:{ "enabled_extensions.v0": { "extensions": [ { "type": "stdio", "name": "everything", "description": "", "cmd": "npx", "args": ["-y", "@modelcontextprotocol/server-everything"], "envs": {}, "env_keys": ["MY_SECRET"], "timeout": null, "bundled": null, "available_tools": [] } ] } }