Skip to content

feat: implement dynamic tool discovery via MCP notifications/tools/li…#8276

Open
makanilani wants to merge 1 commit intoblock:mainfrom
makanilani:dynamic-tool-discovery
Open

feat: implement dynamic tool discovery via MCP notifications/tools/li…#8276
makanilani wants to merge 1 commit intoblock:mainfrom
makanilani:dynamic-tool-discovery

Conversation

@makanilani
Copy link
Copy Markdown

Pull Request

Title

feat: implement dynamic tool discovery via MCP notifications/tools/list_changed

URL

main...makanilani:goose:dynamic-tool-discovery


Description

Summary

When MCP servers send notifications/tools/list_changed (e.g., Google Colab servers that register tools dynamically), Goose now invalidates its tool cache so the next agent turn sees the updated tool set.

Problem

Currently, Goose calls tools/list once during initialization and never checks for updates. The rmcp crate's ClientHandler trait provides on_tool_list_changed() with a default no-op implementation, and GooseClient does not override it — so the notification is silently dropped.

Changes

  • Add ToolCacheInvalidator trait in mcp_client.rs
  • Implement ToolCacheInvalidator for ExtensionManager
  • Add tool_cache_invalidator field to GooseClient
  • Implement on_tool_list_changed handler in GooseClient's ClientHandler impl (overrides rmcp's default no-op)
  • Thread invalidator reference through all MCP client construction paths (StreamableHttp, Stdio, Builtin, InlinePython)

Architecture

MCP Server → notifications/tools/list_changed
  → GooseClient::on_tool_list_changed() (new)
    → ExtensionManager::invalidate_tools_cache_and_bump_version() (existing)
      → Agent reads fresh tools on next turn via get_prefixed_tools()

Files Changed

  • Cargo.lock
  • crates/goose/src/agents/mcp_client.rs
  • crates/goose/src/agents/extension_manager.rs

Testing

Observed Google colab mcp server works after the change.


Co-authored-by: Qwen-Coder qwen-coder@alibabacloud.com

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 58ef3c786c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +612 to +613
let invalidator: Option<Arc<dyn ToolCacheInvalidator>> =
Some(Arc::clone(self) as Arc<dyn ToolCacheInvalidator>);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Break strong reference cycle when wiring tool invalidator

Passing Arc::clone(self) as the ToolCacheInvalidator creates a retain cycle: ExtensionManager holds each client in extensions, and each McpClient now holds a GooseClient that keeps this strong Arc<ExtensionManager> in tool_cache_invalidator (mcp_client.rs). When a session/agent is removed, this cycle prevents ExtensionManager and its clients from dropping, so child MCP processes and temp resources can leak across sessions. Please switch this back-reference to a Weak-based invalidator (or an explicit teardown path) so session cleanup can actually release clients.

Useful? React with 👍 / 👎.

@DOsinga
Copy link
Copy Markdown
Collaborator

DOsinga commented Apr 3, 2026

Thanks for the PR — handling notifications/tools/list_changed is something we do need.

A few things to address before we can move forward:

  1. Reference cycle (Codex P1): The Arc::clone(self) as Arc<dyn ToolCacheInvalidator> creates a strong reference cycle (ExtensionManagerextensionsMcpClientGooseClienttool_cache_invalidatorExtensionManager). Please use Arc::downgrade with a Weak-based wrapper instead.

  2. Tests: The two tests added are testing implementation details (that a field is stored, that a mock works) rather than the actual feature. test_on_tool_list_changed_calls_invalidator never actually calls on_tool_list_changed — it just calls invalidate_tools() directly on the stored field. Please either write tests that exercise the real notification path or remove the trivial tests.

  3. DCO: The commit is missing the required Signed-off-by line — CI is failing on the DCO check. See the check output for instructions on how to fix this.

  4. Cargo.lock: There's a spurious goose-sdk version bump in the lockfile that's unrelated to this change — please revert that.

/cc @alexhancock

@DOsinga DOsinga added the needs_human label to set when a robot looks at a PR and can't handle it label Apr 3, 2026
…st_changed

When MCP servers send notifications/tools/list_changed (e.g., Google Colab
servers that register tools dynamically), Goose now invalidates its tool
cache so the next agent turn sees the updated tool set.

Changes:
- Add ToolCacheInvalidator trait in mcp_client.rs
- Implement ToolCacheInvalidator for ExtensionManager
- Add tool_cache_invalidator field to GooseClient
- Implement on_tool_list_changed handler in GooseClient's ClientHandler impl
  (overrides rmcp's default no-op)
- Thread invalidator reference through all MCP client construction paths
  (StreamableHttp, Stdio, Builtin, InlinePython)
- Add unit tests for invalidator wiring

Architecture:
  MCP Server → notifications/tools/list_changed
    → GooseClient::on_tool_list_changed() (new)
      → ExtensionManager::invalidate_tools_cache_and_bump_version() (existing)
        → Agent reads fresh tools on next turn via get_prefixed_tools()

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Signed-off-by: Yasu Sakakibara <ysakakibara@gmail.com>
@makanilani makanilani force-pushed the dynamic-tool-discovery branch from 58ef3c7 to be77571 Compare April 3, 2026 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs_human label to set when a robot looks at a PR and can't handle it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants