-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
context_server: Add support for SSE MCP servers #25693
base: main
Are you sure you want to change the base?
Conversation
let config = desired_servers.entry(id.clone()).or_default(); | ||
match config { | ||
ServerConfig::Stdio { command, .. } => { | ||
if command.is_none() { | ||
if let Some(extension_command) = | ||
factory(project.clone(), &cx).await.log_err() | ||
{ | ||
*command = Some(extension_command); | ||
} | ||
} | ||
} | ||
ServerConfig::Sse { .. } => {} |
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'm not 100% sure what we are trying to do in this section, so I simply implemented a no-op, but I'm sure there's more to it... 😬
pub enum ServerConfig { | ||
Stdio { | ||
/// The command to run this context server. | ||
/// | ||
/// This will override the command set by an extension. | ||
command: Option<ServerCommand>, | ||
/// The settings for this context server. | ||
/// | ||
/// Consult the documentation for the context server to see what settings | ||
/// are supported. | ||
#[schemars(schema_with = "server_config_settings_json_schema")] | ||
settings: Option<serde_json::Value>, | ||
}, | ||
Sse { | ||
/// The remote SSE endpoint. | ||
endpoint: String, | ||
}, | ||
} |
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 initially tried to go the untagged
way, but I had deserialization problems, always returning an invalid configuration. Nevertheless, I don't believe this being the ideal way, as it's not backward compatible. Eager to hear what's the best approach
match settings { | ||
ServerConfig::Stdio { command, settings } => { | ||
Ok(serde_json::to_string(&settings::ContextServerSettings { | ||
command: command.map(|command| settings::CommandSettings { | ||
path: Some(command.path), | ||
arguments: Some(command.args), | ||
env: command.env.map(|env| env.into_iter().collect()), | ||
}), | ||
settings, | ||
})?) | ||
} | ||
ServerConfig::Sse { .. } => { | ||
bail!("SSE server configuration is not supported") | ||
} | ||
} |
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.
For now, I didn't add support in the extension, because I didn't want to mess around with the API. It seems that since_v0_2_0
is using since_v0_3_0
(under the name latest
). Also this depends on the general settings
_ = executor.timer(Duration::from_secs(30)).fuse() => { | ||
break; | ||
} |
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.
Here I'm trying to account for computer sleep/wake-up, but it doesn't always work as expected. Is there a better way to handle these interruptions?
Add support for remote MCP servers, allowing to communicate through HTTP+SSE
Release Notes: