Compare commits

..

1 Commits

Author SHA1 Message Date
YeonGyu-Kim
a10d5470d1 fix(mcp): return typed error JSON for unsupported actions (info/describe/list-filter)
`claw mcp info nonexistent --output-format json` and
`claw mcp list nonexistent --output-format json` fell through to
the generic help renderer, returning an opaque envelope with only
`unexpected` set — no machine-readable error_kind.

Fix:
- Add typed guards in render_mcp_report_for/_json_for for:
  - `list <filter>`: list accepts no filter argument
  - `info <name>` / `describe <name>`: suggest `mcp show`
- New render_mcp_unsupported_action_text/json helpers emit
  `ok:false`, `error_kind:"unsupported_action"`, `hint`, `requested_action`
- `mcp show`, `mcp list`, `mcp help` existing paths unchanged

Test: mcp_unsupported_actions_return_typed_error_not_generic_help
asserts kind=="mcp", ok==false, error_kind=="unsupported_action"
for info/list-filter/describe paths.

Pinpoint: ROADMAP #504
2026-05-04 18:33:07 +09:00
4 changed files with 83 additions and 50 deletions

View File

@@ -2596,10 +2596,44 @@ fn render_mcp_report_for(
)),
}
}
Some(args) if args.split_whitespace().next() == Some("list") && args.contains(' ') => {
// `mcp list <filter>` — list does not accept arguments; treat as unsupported action.
Ok(render_mcp_unsupported_action_text(
args,
"list accepts no filter argument; use `claw mcp list`",
))
}
Some(args) if matches!(args.split_whitespace().next(), Some("info" | "describe")) => {
Ok(render_mcp_unsupported_action_text(
args,
"use `claw mcp show <server>` to inspect a server",
))
}
Some(args) => Ok(render_mcp_usage(Some(args))),
}
}
fn render_mcp_unsupported_action_text(action: &str, hint: &str) -> String {
format!(
"MCP\n Error unsupported action '{action}'\n Hint {hint}\n Usage /mcp [list|show <server>|help]"
)
}
fn render_mcp_unsupported_action_json(action: &str, hint: &str) -> Value {
json!({
"kind": "mcp",
"action": "error",
"ok": false,
"error_kind": "unsupported_action",
"requested_action": action,
"hint": hint,
"usage": {
"slash_command": "/mcp [list|show <server>|help]",
"direct_cli": "claw mcp [list|show <server>|help]",
},
})
}
fn render_mcp_report_json_for(
loader: &ConfigLoader,
cwd: &Path,
@@ -2680,6 +2714,18 @@ fn render_mcp_report_json_for(
})),
}
}
Some(args) if args.split_whitespace().next() == Some("list") && args.contains(' ') => {
Ok(render_mcp_unsupported_action_json(
args,
"list accepts no filter argument; use `claw mcp list`",
))
}
Some(args) if matches!(args.split_whitespace().next(), Some("info" | "describe")) => {
Ok(render_mcp_unsupported_action_json(
args,
"use `claw mcp show <server>` to inspect a server",
))
}
Some(args) => Ok(render_mcp_usage_json(Some(args))),
}
}
@@ -4641,6 +4687,38 @@ mod tests {
);
}
#[test]
fn mcp_unsupported_actions_return_typed_error_not_generic_help() {
// `mcp info <name>` and `mcp list <filter>` must return typed errors, not raw help.
// Regression for #504: these previously fell through to render_mcp_usage with
// unexpected=arg, giving no machine-readable error_kind.
use crate::handle_mcp_slash_command_json;
use std::path::PathBuf;
let cwd = PathBuf::from("/tmp");
let info_json = handle_mcp_slash_command_json(Some("info nonexistent"), &cwd)
.expect("info nonexistent should not error at IO level");
assert_eq!(info_json["kind"], "mcp");
assert_eq!(info_json["ok"], false);
assert_eq!(info_json["error_kind"], "unsupported_action");
assert!(info_json["hint"]
.as_str()
.unwrap_or_default()
.contains("show"));
let list_filter_json = handle_mcp_slash_command_json(Some("list nonexistent"), &cwd)
.expect("list nonexistent should not error at IO level");
assert_eq!(list_filter_json["kind"], "mcp");
assert_eq!(list_filter_json["ok"], false);
assert_eq!(list_filter_json["error_kind"], "unsupported_action");
let describe_json = handle_mcp_slash_command_json(Some("describe myserver"), &cwd)
.expect("describe myserver should not error at IO level");
assert_eq!(describe_json["kind"], "mcp");
assert_eq!(describe_json["ok"], false);
assert_eq!(describe_json["error_kind"], "unsupported_action");
}
#[test]
fn rejects_invalid_mcp_arguments() {
let show_error = parse_error_message("/mcp show alpha beta");

View File

@@ -2627,15 +2627,12 @@ fn print_version(output_format: CliOutputFormat) -> Result<(), Box<dyn std::erro
}
fn version_json_value() -> serde_json::Value {
let executable_path = env::current_exe().ok().map(|p| p.display().to_string());
json!({
"kind": "version",
"message": render_version_report(),
"version": VERSION,
"git_sha": GIT_SHA,
"target": BUILD_TARGET,
"build_date": DEFAULT_DATE,
"executable_path": executable_path,
})
}
@@ -3526,10 +3523,10 @@ fn run_resume_command(
Ok(ResumeCommandOutcome {
session: session.clone(),
message: Some(handle_agents_slash_command(args.as_deref(), &cwd)?),
json: Some(
serde_json::to_value(handle_agents_slash_command_json(args.as_deref(), &cwd)?)
.unwrap_or_else(|_| serde_json::json!(null)),
),
json: Some(serde_json::json!({
"kind": "agents",
"text": handle_agents_slash_command(args.as_deref(), &cwd)?,
})),
})
}
SlashCommand::Skills { args } => {

View File

@@ -30,15 +30,6 @@ fn version_emits_json_when_requested() {
let parsed = assert_json_command(&root, &["--output-format", "json", "version"]);
assert_eq!(parsed["kind"], "version");
assert_eq!(parsed["version"], env!("CARGO_PKG_VERSION"));
// Provenance fields must be present for binary identification (#507).
assert!(
parsed["build_date"].is_string(),
"build_date must be a string in version JSON"
);
assert!(
parsed["executable_path"].is_string(),
"executable_path must be a string in version JSON so callers can identify which binary is running"
);
}
#[test]
@@ -369,34 +360,6 @@ fn resumed_inventory_commands_emit_structured_json_when_requested() {
assert_eq!(skills["action"], "list");
assert!(skills["summary"]["total"].is_number());
assert!(skills["skills"].is_array());
let agents = assert_json_command_with_env(
&root,
&[
"--output-format",
"json",
"--resume",
session_path.to_str().expect("utf8 session path"),
"/agents",
],
&[
(
"CLAW_CONFIG_HOME",
config_home.to_str().expect("utf8 config home"),
),
("HOME", home.to_str().expect("utf8 home")),
],
);
assert_eq!(agents["kind"], "agents");
assert_eq!(agents["action"], "list");
assert!(
agents["agents"].is_array(),
"agents field must be a JSON array"
);
assert!(
agents["count"].is_number(),
"count must be a number, not a text render"
);
}
#[test]

View File

@@ -227,8 +227,6 @@ fn resumed_status_command_emits_structured_json_when_requested() {
// given
let temp_dir = unique_temp_dir("resume-status-json");
fs::create_dir_all(&temp_dir).expect("temp dir should exist");
let config_home = temp_dir.join("config-home");
fs::create_dir_all(&config_home).expect("isolated config home should exist");
let session_path = temp_dir.join("session.jsonl");
let mut session = workspace_session(&temp_dir);
@@ -240,9 +238,7 @@ fn resumed_status_command_emits_structured_json_when_requested() {
.expect("session should persist");
// when
// Use an isolated CLAW_CONFIG_HOME so ~/.claw/settings.json is not loaded,
// which would cause loaded_config_files to be non-zero (#65).
let output = run_claw_with_env(
let output = run_claw(
&temp_dir,
&[
"--output-format",
@@ -251,7 +247,6 @@ fn resumed_status_command_emits_structured_json_when_requested() {
session_path.to_str().expect("utf8 path"),
"/status",
],
&[("CLAW_CONFIG_HOME", config_home.to_str().expect("utf8 path"))],
);
// then