fix(mcp): kigi mcp add <name> <url> infers an HTTP server
Port of upstream grok-build 1.0.9. A bare positional http(s):// URL with no transport flag was silently added as a stdio command whose "command" was the URL. It now infers the HTTP transport with a notice; extra args, --env, legacy --command/--url, and every explicit flag keep their prior meaning, and scheme-less URL-looking commands still stay stdio with the existing warning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KSy9S31pZNWXtRZNVJ1sDf
This commit is contained in:
1 parent
574122884a
commit
a441b23a6d
1 file changed
+99
-12
@@ -112,7 +112,8 @@ pub struct AddArgs {
|
||||
#[arg(value_name = "ARGS")]
|
||||
args: Vec<String>,
|
||||
|
||||
/// Transport type. Defaults to stdio.
|
||||
/// Transport type. Defaults to stdio, or to http when the positional
|
||||
/// argument is an http(s):// URL.
|
||||
#[arg(short = 't', long, value_enum)]
|
||||
transport: Option<McpTransport>,
|
||||
|
||||
@@ -273,11 +274,24 @@ async fn run_add(args: AddArgs) -> Result<()> {
|
||||
|
||||
/// Validate an `mcp add` request and build the transport config.
|
||||
///
|
||||
/// The transport flag fully determines how `command_or_url` is interpreted;
|
||||
/// URL-looking commands only produce a warning, never a behavior change.
|
||||
/// An explicit transport flag fully determines how `command_or_url` is
|
||||
/// interpreted. Without one, a bare positional http(s):// URL is inferred to
|
||||
/// be an HTTP server; other URL-looking commands stay stdio with a warning.
|
||||
fn resolve_add(args: &AddArgs) -> Result<ResolvedAdd> {
|
||||
validate_server_name(&args.name)?;
|
||||
|
||||
// Extra args or --env mean the user is describing a command, and the
|
||||
// legacy --command/--url flags keep their own semantics, so only a bare
|
||||
// positional http(s):// URL triggers inference.
|
||||
let inferred_http = args.transport.is_none()
|
||||
&& args.url.is_none()
|
||||
&& args.args.is_empty()
|
||||
&& args.env.is_empty()
|
||||
&& args
|
||||
.command_or_url
|
||||
.as_deref()
|
||||
.is_some_and(|s| s.starts_with("http://") || s.starts_with("https://"));
|
||||
|
||||
let transport = match args.transport {
|
||||
Some(t) => t,
|
||||
// The legacy --url form defaults to HTTP and honors the legacy --type flag.
|
||||
@@ -285,6 +299,7 @@ fn resolve_add(args: &AddArgs) -> Result<ResolvedAdd> {
|
||||
Some(t) if t.eq_ignore_ascii_case("sse") => McpTransport::Sse,
|
||||
_ => McpTransport::Http,
|
||||
},
|
||||
None if inferred_http => McpTransport::Http,
|
||||
None => McpTransport::Stdio,
|
||||
};
|
||||
let explicit_transport = args.transport.is_some();
|
||||
@@ -391,6 +406,13 @@ fn resolve_add(args: &AddArgs) -> Result<ResolvedAdd> {
|
||||
}
|
||||
let headers = parse_headers(&args.header)?;
|
||||
|
||||
let mut warnings = Vec::new();
|
||||
if inferred_http {
|
||||
warnings.push(format!(
|
||||
"No --transport given; '{url}' starts with http(s)://, adding as an HTTP server. Use --transport sse for an SSE server, or --transport stdio to force a stdio command."
|
||||
));
|
||||
}
|
||||
|
||||
Ok(ResolvedAdd {
|
||||
kind: transport,
|
||||
transport: McpServerTransportConfig::StreamableHttp {
|
||||
@@ -402,7 +424,7 @@ fn resolve_add(args: &AddArgs) -> Result<ResolvedAdd> {
|
||||
oauth_client_secret_env_var: None,
|
||||
oauth_scopes: None,
|
||||
},
|
||||
warnings: Vec::new(),
|
||||
warnings,
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -808,24 +830,89 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn add_default_transport_warns_on_url_looking_command() {
|
||||
let add = parse_add(&["kigi", "mcp", "add", "api", "https://mcp.example.com/mcp"]);
|
||||
let resolved = resolve_add(&add).expect("defaults to stdio with a warning");
|
||||
// Scheme-less URL-looking commands stay stdio (no inference) and the
|
||||
// warning suggests a command that passes URL validation verbatim.
|
||||
let add = parse_add(&["kigi", "mcp", "add", "local", "localhost:3000"]);
|
||||
let resolved = resolve_add(&add).expect("localhost command warns");
|
||||
assert!(matches!(
|
||||
resolved.transport,
|
||||
McpServerTransportConfig::Stdio { .. }
|
||||
));
|
||||
assert_eq!(resolved.warnings.len(), 1);
|
||||
assert!(resolved.warnings[0].contains("--transport http"));
|
||||
|
||||
// Scheme-less commands get http:// prepended so the suggested
|
||||
// command passes URL validation verbatim.
|
||||
let add = parse_add(&["kigi", "mcp", "add", "local", "localhost:3000"]);
|
||||
let resolved = resolve_add(&add).expect("localhost command warns");
|
||||
assert!(
|
||||
resolved.warnings[0].contains("--transport http local http://localhost:3000"),
|
||||
"got: {}",
|
||||
resolved.warnings[0]
|
||||
);
|
||||
|
||||
// Extra args mean a command is being described — no inference even
|
||||
// with an http(s) scheme.
|
||||
let add = parse_add(&[
|
||||
"kigi",
|
||||
"mcp",
|
||||
"add",
|
||||
"api",
|
||||
"--",
|
||||
"https://mcp.example.com/mcp",
|
||||
"--flag",
|
||||
]);
|
||||
let resolved = resolve_add(&add).expect("URL with args stays stdio");
|
||||
assert!(matches!(
|
||||
resolved.transport,
|
||||
McpServerTransportConfig::Stdio { .. }
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn add_infers_http_for_bare_positional_url() {
|
||||
for url in ["https://mcp.example.com/mcp", "http://mcp.example.com/mcp"] {
|
||||
let add = parse_add(&["kigi", "mcp", "add", "api", url]);
|
||||
let resolved = resolve_add(&add).expect("bare http(s) URL infers http");
|
||||
assert_eq!(resolved.kind, McpTransport::Http);
|
||||
match resolved.transport {
|
||||
McpServerTransportConfig::StreamableHttp {
|
||||
url: stored,
|
||||
transport_type,
|
||||
..
|
||||
} => {
|
||||
assert_eq!(stored, url);
|
||||
assert_eq!(transport_type, None);
|
||||
}
|
||||
other => panic!("expected http transport, got {other:?}"),
|
||||
}
|
||||
assert_eq!(resolved.warnings.len(), 1);
|
||||
assert!(
|
||||
resolved.warnings[0].contains("No --transport given"),
|
||||
"got: {}",
|
||||
resolved.warnings[0]
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn add_infers_http_with_headers() {
|
||||
// Previously this bailed: stdio was assumed and --header is remote-only.
|
||||
let add = parse_add(&[
|
||||
"kigi",
|
||||
"mcp",
|
||||
"add",
|
||||
"api",
|
||||
"https://mcp.example.com/mcp",
|
||||
"--header",
|
||||
"Authorization: Bearer tok",
|
||||
]);
|
||||
let resolved = resolve_add(&add).expect("URL with headers infers http");
|
||||
assert_eq!(resolved.kind, McpTransport::Http);
|
||||
match resolved.transport {
|
||||
McpServerTransportConfig::StreamableHttp { headers, .. } => {
|
||||
let headers = headers.expect("headers should be set");
|
||||
assert_eq!(
|
||||
headers.get("Authorization").map(String::as_str),
|
||||
Some("Bearer tok")
|
||||
);
|
||||
}
|
||||
other => panic!("expected http transport, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in new issue
Block a user