-
Notifications
You must be signed in to change notification settings - Fork 53
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
feat: mcp server prompts #562
Conversation
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
@@ -53,7 +53,7 @@ pub struct ImageContent { | |||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | |||
#[serde(rename_all = "camelCase")] | |||
pub struct EmbeddedResource { | |||
resource: ResourceContents, | |||
pub resource: ResourceContents, |
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.
@baxen any concerns with having resource as a publicly accessible field?
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.
where is this getting used? trying to understand why this needs to be public
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.
this is more like a nice to have, right now we only support text prompts, but this allows for adding resource and images in the context - https://spec.modelcontextprotocol.io/specification/2024-11-05/server/prompts/#embedded-resources
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.
seems fine to me
@@ -93,6 +94,15 @@ pub trait Router: Send + Sync + 'static { | |||
&self, | |||
uri: &str, | |||
) -> Pin<Box<dyn Future<Output = Result<String, ResourceError>> + Send + 'static>>; | |||
fn list_prompts(&self) -> Option<Vec<Prompt>> { |
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.
i think you'll need to add these methods on mcp-client
to make it work with goose. it can be a separate PR. we can use the passthrough method to send request to a system
goose/crates/goose/src/agents/agent.rs
Line 27 in bcd991c
async fn passthrough(&self, system: &str, request: Value) -> SystemResult<Value>; |
the method needs to be implemented for the default agent:
goose/crates/goose/src/agents/default.rs
Line 147 in bcd991c
async fn passthrough(&self, _system: &str, _request: Value) -> SystemResult<Value> { |
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.
@salman1993 could you elaborate on the need to handle on client-side? my understanding was that prompts would belong in servers and handled by add_systems
.add_system(config) |
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.
you'd have to add the prompt methods here - it gets used by goose agent here
each MCP client has a 1:1 relationship with a server
} | ||
|
||
/// Create a new resource message | ||
pub fn new_resource( |
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.
how does this get used?
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.
It is just implemented, not used at the moment. For supporting image and resource in prompt - https://spec.modelcontextprotocol.io/specification/2024-11-05/server/prompts/#embedded-resources
@@ -162,6 +165,8 @@ impl DeveloperRouter { | |||
}), | |||
); | |||
|
|||
let unit_test_prompt = developer_prompt::create_unit_test_prompt(); |
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.
@baxen is it okay to have the unit test gen prompt in the developer system? conceptually it makes sense to me
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.
Seems ok to me. At first I thought it could make sense more as its own unit test specific server, but it's germane enough to the work of general software development I think adding it 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.
yeah seems good! i think very little harm in having prompts around as they only get used when requested
e54bd46
to
0477f7a
Compare
83aba48
to
95253fe
Compare
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
crates/mcp-server/src/router.rs
Outdated
.await | ||
.map_err(|e| RouterError::Internal(e.to_string()))?; | ||
|
||
// Validate prompt arguments for potential security issues |
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.
how did you come up with these validation rules?
@@ -162,6 +165,8 @@ impl DeveloperRouter { | |||
}), | |||
); | |||
|
|||
let unit_test_prompt = developer_prompt::create_unit_test_prompt(); |
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.
Seems ok to me. At first I thought it could make sense more as its own unit test specific server, but it's germane enough to the work of general software development I think adding it makes sense.
} | ||
|
||
#[derive(Debug, Serialize, Deserialize)] | ||
pub struct PromptArgumentTemplate { |
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.
I get a little lost in the implementation of these prompt/argument templates. Don't we need the type information somewhere for what types the arguments can be?
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.
Yeah it's a bit confusing, but for prompt arguments, they're all going to be strings - eg the code argument
{
"jsonrpc": "2.0",
"id": 1,
"result": {
"prompts": [
{
"name": "code_review",
"description": "Asks the LLM to analyze code quality and suggest improvements",
"arguments": [
{
"name": "code",
"description": "The code to review",
"required": true
}
]
}
],
"nextCursor": "next-page-cursor"
}
}
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.
Looks great!
@@ -53,7 +53,7 @@ pub struct ImageContent { | |||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | |||
#[serde(rename_all = "camelCase")] | |||
pub struct EmbeddedResource { | |||
resource: ResourceContents, | |||
pub resource: ResourceContents, |
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.
seems fine to me
fn get_prompt( | ||
&self, | ||
_prompt_name: &str, | ||
) -> Option<Pin<Box<dyn Future<Output = Result<String, PromptError>> + Send + 'static>>> { |
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.
nit: i'd remove the option here and leave these unimplemented for consistency? I suspect these are here so we don't break the existing servers, but i think we can go fix those up
@@ -162,6 +165,8 @@ impl DeveloperRouter { | |||
}), | |||
); | |||
|
|||
let unit_test_prompt = developer_prompt::create_unit_test_prompt(); |
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.
yeah seems good! i think very little harm in having prompts around as they only get used when requested
let prompts = self.prompts.clone(); | ||
|
||
Some(Box::pin(async move { | ||
// Check if prompts list is empty |
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.
nit: this isn't reachable right?
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
* ldelalande/welcome-ux-update: (42 commits) format Use new configuration and secret storage Simplify configuration (#607) [setup] UX simplification for adding API keys on app startup adding in comments for where to add variables for bundling can reset the provider will open new window when set show window based on local storage goose_provider update the icon new goose modals for adding models latest changes with mock keystore initial static setup fix: update BufReader capacity to 2MB (#605) fix: escape double quotes in error part (#602) refactor: remove unused errors in mcp-client (#598) add more context from mcp server errors (#599) fix: error formatting for vercel data format (#597) feat: Add endpoint to store secrets in keychain (#595) feat: mcp server prompts (#562) fix: Temporarily disable moderation (#594) ...
What are prompts?
The Model Context Protocol (MCP) provides a standardized way for servers to expose prompt templates to clients. Prompts allow servers to provide structured messages and instructions for interacting with language models. Clients can discover available prompts, retrieve their contents, and provide arguments to customize them.
What does a prompt look like?
This is an example unit_test text prompt endpoint from the mcp inspector. For the unit test prompt, users can input the code file and language as a parameter and generate a templated prompt
What is the UI/UX going to look like?
MCP really leaves it up to the implementer to decide on how to surface prompts. For inspiration, they suggest a slash command, but it could be anything from a dropdown list or a list of templates.
Implementation details: