Skip to content

feat(mcp): support sampling in a scoped way #5367

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.

Sign up for GitHub

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

Jump to bottom
Merged
alexhancock merged 1 commit into from
Oct 29, 2025
Merged

feat(mcp): support sampling in a scoped way #5367

alexhancock merged 1 commit into from
Oct 29, 2025

Conversation

@alexhancock
Copy link
Collaborator

@alexhancock alexhancock commented Oct 24, 2025

Implements basic support for MCP Sampling

  • MCP servers can now request sampling and it will be handled by goose to route a sampling message to the provider for completion
  • Works for desktop and CLI
  • Does not yet implement human in the loop approval of these, which is a SHOULD for clients in the MCP spec, but added significant overhead to the initial implementation so the idea is to get a scoped and working implementation out and then iterate
  • Sampling is supposed to be text, image, or audio. This does not handle audio, because interestingly goose does not handle audio message content. Can be a followup when we support it generally.

Demos of it working in CLI and Desktop with the everything server the prompt

Use the sampleLLM tool in the everything extension to have the mcp server request sampling from the model. Have the mcp server ask the model for a quote from the great gatsby

Screenshot 2025-10-24 at 5 28 49 PM Screenshot 2025-10-24 at 5 31 13 PM

jamadeo
jamadeo approved these changes Oct 25, 2025
View reviewed changes
Copy link
Collaborator

@jamadeo jamadeo left a comment

Choose a reason for hiding this comment

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

nice!

crates/goose/src/agents/mcp_client.rs Outdated
ClientHandler, Peer, RoleClient, ServiceError, ServiceExt,
ClientHandler, ErrorData as McpError, ErrorData, Peer, RoleClient, ServiceError, ServiceExt,
};
use schemars::_private::NoSerialize;
Copy link
Collaborator

@jamadeo jamadeo Oct 25, 2025

Choose a reason for hiding this comment

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

suspicious-looking import!

Copy link
Collaborator Author

@alexhancock alexhancock Oct 28, 2025

Choose a reason for hiding this comment

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

It was needed for e.maybe_to_value() but no longer using that, so it's gone!

crates/goose/src/agents/extension_manager_extension.rs Outdated

let result = extension_manager
.add_extension(config)
.add_extension(config, None)
Copy link
Collaborator

@jamadeo jamadeo Oct 25, 2025

Choose a reason for hiding this comment

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

probably fine for now, but I suppose we will want to allow this to work too

Copy link
Collaborator Author

@alexhancock alexhancock Oct 28, 2025

Choose a reason for hiding this comment

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

In theory I fixed this in the most recent commit, which changes the approach of how the extension manager has a reference to a provider. Didn't text explicitly for sampling through extension_manager managed extensions, but I think it may work now. I'll add a note to follow up on it.

DOsinga
DOsinga reviewed Oct 26, 2025
View reviewed changes
Copy link
Collaborator

@DOsinga DOsinga left a comment

Choose a reason for hiding this comment

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

This generally looks good and would allow us to ship something. I'm a little concerned on how we handle providers. within the context of mcp, I don't think they should be optional - we can't have an MCP call without a provider, right?

the other is that we now set a provider on the extension_manager. in some scenarios we might not have it yet, maybe that's why it is optional? but I don't think we can be sure we're using the right provider if the user switches models, providers etc.

we should use the provider of the agent I think.

crates/goose/src/agents/mcp_client.rs Outdated
ErrorData::new(
ErrorCode::INTERNAL_ERROR,
"Unexpected error while completing the prompt",
e.maybe_to_value(),
Copy link
Collaborator

@DOsinga DOsinga Oct 26, 2025

Choose a reason for hiding this comment

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

why maybe_to_value()? I think ti would return None here, we should probably return the relevant string though

Copy link
Collaborator Author

@alexhancock alexhancock Oct 28, 2025

Choose a reason for hiding this comment

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

Adjusted

@alexhancock alexhancock force-pushed the alexhancock/mcp-sampling branch 8 times, most recently from 30943fd to c84f539 Compare October 28, 2025 21:11
@alexhancock
Copy link
Collaborator Author

alexhancock commented Oct 28, 2025 •
edited
Loading

@jamadeo @DOsinga Updated it to always uses the agent's current provider via a shared reference. Verified by the llm_request logs that it uses different models for sampling after switching providers.

@alexhancock alexhancock force-pushed the alexhancock/mcp-sampling branch from c84f539 to 93afbd6 Compare October 29, 2025 13:14
DOsinga
DOsinga approved these changes Oct 29, 2025
View reviewed changes
Copy link
Collaborator

@DOsinga DOsinga left a comment

Choose a reason for hiding this comment

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

how do we test this?

crates/goose/src/agents/mcp_client.rs Outdated
})?;

// convert back to MCP messages
let response_content = if let Some(content) = response.content.first() {
Copy link
Collaborator

@DOsinga DOsinga Oct 29, 2025

Choose a reason for hiding this comment

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

only first?

@alexhancock alexhancock force-pushed the alexhancock/mcp-sampling branch 5 times, most recently from fa587e7 to 1680290 Compare October 29, 2025 18:49
@alexhancock alexhancock force-pushed the alexhancock/mcp-sampling branch from 1680290 to 3e2bc35 Compare October 29, 2025 20:43
@alexhancock alexhancock merged commit 06b7a63 into main Oct 29, 2025
14 checks passed
@alexhancock alexhancock deleted the alexhancock/mcp-sampling branch October 29, 2025 21:06
@alexhancock alexhancock mentioned this pull request Oct 29, 2025
Merged
@alexhancock
Copy link
Collaborator Author

alexhancock commented Oct 29, 2025

Draft of a couple different approaches to integration testing for sampling #5456

Wanted to make a separate PR to get feedback on which one seems most valuable

@jamadeo @DOsinga

@github-actions github-actions bot mentioned this pull request Nov 5, 2025
Merged
fbalicchia pushed a commit to fbalicchia/goose that referenced this pull request Nov 7, 2025
@alexhancock @fbalicchia
feat(mcp): support sampling in a scoped way (block#5367)
0e9225e 
Signed-off-by: fbalicchia <[email protected]>
BlairAllan pushed a commit to BlairAllan/goose that referenced this pull request Nov 29, 2025
@alexhancock @BlairAllan
feat(mcp): support sampling in a scoped way (block#5367)
a2490fa 
Signed-off-by: Blair Allan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DOsinga DOsinga DOsinga approved these changes

@jamadeo jamadeo jamadeo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alexhancock @DOsinga @jamadeo