-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(code-mode): use server names for MCP extensions #6284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances MCP extension handling in goose by enabling code mode in ACP and improving extension naming for better LLM interaction.
Key Changes
- ACP code mode support: Adds
--with-builtinCLI argument allowing ACP clients to enable built-in extensions like code_execution and developer - Server-based extension naming: Extensions without configured names now use MCP server-declared names instead of random identifiers, with collision detection
- Search filtering: Excludes
extensionmanagerfrom code_execution search results to reduce LLM confusion about internal tools
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/goose/src/agents/extension_manager.rs | Adds resolve_extension_name() function to use server-declared names with collision handling via random suffixes |
| crates/goose/src/agents/code_execution_extension.rs | Filters out extensionmanager from module search results |
| crates/goose-cli/src/session/mod.rs | Removes generate_extension_name() - extensions now use empty name strings to trigger server name resolution |
| crates/goose-cli/src/commands/acp.rs | Adds add_builtins() helper and --with-builtin parameter support for ACP mode |
| crates/goose-cli/src/cli.rs | Adds --with-builtin CLI argument with comma-separated builtin names |
| crates/goose/tests/acp_integration_test.rs | Adds test_acp_with_builtin_and_mcp integration test verifying code_execution works with MCP servers, updates test server to declare explicit name |
| crates/goose/tests/test_data/openai_*.txt | Updates test fixtures with new model format and adds builtin extension interaction test data |
| } | ||
| } | ||
|
|
||
| #[test_case("kiwi", Some("kiwi-mcp-server"), None, "^kiwi$" ; "ACP session prefers explicit name")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: all of these are real server names scraped from the public MCP impls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
51de2f1 to
451c7c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
edbb8e6 to
c84aafb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
ee2e3f2 to
e32f7f6
Compare
Signed-off-by: Adrian Cole <[email protected]>
e32f7f6 to
ebd1f3b
Compare
| .env("GOOSE_MODE", "approve") | ||
| .env("OPENAI_HOST", mock_server.uri()) | ||
| .env("OPENAI_API_KEY", "test-key") | ||
| .env("GOOSE_PATH_ROOT", data_root) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this to isolate the DBs between tests, but allow us to intentionally share them. This will be very important in subsequent change for session load, resume and list capabilities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
|
I'm not sure why - but the GUI isn't working with this - never get a response, no matter what I do (I just flicked to main and tried again) - if I turn off code mode it is fine. |
Summary
Makes code mode usable in ACP and improves extension naming for better LLM accuracy.
Code mode in ACP: Adds
--with-builtinarg so ACP clients can enable code_execution and developer extensions.Better extension names: When no name is configured (e.g., CLI
--with-streamable-http-extension), extensions now use server-declared names instead of anonymous identifiers. User-configured names from goose config or ACP sessions are still preferred.Before (CLI arg, no configured name):
After:
─── search-flight | kiwi-mcp-server ──────────────────────────LLMs can now match extension names to server info in their prompts. Collision suffix added when duplicate names exist.
Type of Change
AI Assistance
Testing
New integration test:
test_acp_with_builtin_and_mcp- verifies code_execution and MCP servers work together in ACP mode.Manual:
Related Issues
Closes #6188