diff --git a/crates/openpencil-shell-core/src/mcp/parser.rs b/crates/openpencil-shell-core/src/mcp/parser.rs index ab6a1a7e3..d35faa4f7 100644 --- a/crates/openpencil-shell-core/src/mcp/parser.rs +++ b/crates/openpencil-shell-core/src/mcp/parser.rs @@ -38,14 +38,16 @@ pub fn parse_tool_call(line: &str) -> Option { // Real MCP: tool name + arguments live inside params. let params_body = extract_params_body(line)?; let name = extract_string_field(¶ms_body, "name")?; - // `arguments` is nested object inside params. Three-way: - // no `arguments` key → empty args, parse succeeds. - // present + scalar-only → those args. - // present + any nested value → reject the parse so - // no tool sees a malformed input as a real scalar. - let arguments = match extract_object_body(¶ms_body, "arguments") { - None => BTreeMap::new(), - Some(body) => parse_flat_object_body(&body)?, + // `arguments` is nested object inside params. Tri-state: + // key missing → empty args (legit MCP can omit it). + // key present + value is `{...}` → parse the body. + // key present + scalar (e.g. `"arguments":"oops"`) → reject + // the parse (codex stop-gate: previously downgraded to + // empty args, hiding wire-shape errors). + let arguments = match arguments_field(¶ms_body) { + ParamsResult::Missing => BTreeMap::new(), + ParamsResult::Body(body) => parse_flat_object_body(&body)?, + ParamsResult::Malformed => return None, }; (name, arguments) } else { @@ -137,6 +139,58 @@ fn extract_string_field(body: &str, key: &str) -> Option { Some(rest[val_start..i].to_string()) } +/// Tri-state lookup for MCP `arguments` inside a `params` block: +/// - Missing: `arguments` key absent (legit empty args). +/// - Body(s): `arguments` is an object; returns its body without +/// the braces. +/// - Malformed: `arguments` is present but not an object +/// (e.g. `"arguments":"oops"` or `"arguments":42`). Caller +/// rejects the call rather than downgrading to empty args +/// (codex stop-gate: previously downgraded silently). +fn arguments_field(body: &str) -> ParamsResult { + let needle = "\"arguments\""; + let Some(found) = body.find(needle) else { + return ParamsResult::Missing; + }; + let start = found + needle.len(); + let after = &body[start..]; + let Some(colon_off) = after.find(':') else { + return ParamsResult::Malformed; + }; + let rest = after[colon_off + 1..].trim_start(); + if !rest.starts_with('{') { + return ParamsResult::Malformed; + } + let bytes = rest.as_bytes(); + let mut depth = 0i32; + let mut in_str = false; + let mut escape = false; + for (i, &b) in bytes.iter().enumerate() { + if in_str { + if escape { + escape = false; + } else if b == b'\\' { + escape = true; + } else if b == b'"' { + in_str = false; + } + continue; + } + match b { + b'"' => in_str = true, + b'{' => depth += 1, + b'}' => { + depth -= 1; + if depth == 0 { + return ParamsResult::Body(rest[1..i].to_string()); + } + } + _ => {} + } + } + ParamsResult::Malformed +} + /// Extract a nested object field's body (without the surrounding /// braces). Used to find the `arguments` map inside MCP `params`. fn extract_object_body(body: &str, key: &str) -> Option { @@ -244,8 +298,12 @@ fn params_body_if_present(line: &str) -> ParamsResult { } /// Parse the body of a JSON object (the content between `{` and `}`) -/// into a flat key→stringified-value map. Skips nested objects / -/// arrays so deeper structure doesn't poison the result. +/// into a flat key→stringified-value map. Returns `None` the moment +/// any value is structured (`{` / `[`) — wire input that pretends a +/// scalar arg is an object/array would otherwise need either a +/// sentinel (which can collide with a legitimate user-supplied +/// string) or per-tool defensive code. Reject at the wire layer +/// instead, so every tool downstream is guaranteed to see scalars. fn parse_flat_object_body(body: &str) -> Option> { let mut out: BTreeMap = BTreeMap::new(); let bytes = body.as_bytes(); diff --git a/crates/openpencil-shell-core/src/mcp_tests.rs b/crates/openpencil-shell-core/src/mcp_tests.rs index 8b2b5b360..9e860e15a 100644 --- a/crates/openpencil-shell-core/src/mcp_tests.rs +++ b/crates/openpencil-shell-core/src/mcp_tests.rs @@ -496,6 +496,42 @@ fn parse_tool_call_rejects_structured_arg_values() { assert_eq!(call.arguments.get("also"), Some(&"ok".to_string())); } +#[test] +fn parse_tool_call_rejects_structured_values_in_mcp_tools_call_shape() { + // Same rejection contract applies on the MCP `tools/call` + // path (codex stop-gate: the previous structured-rejection + // tests only exercised the legacy/direct shape). + let with_obj = r#"{"id":1,"method":"tools/call","params":{"name":"get_node","arguments":{"node_id":"42","nested":{"a":1}}}}"#; + assert!(parse_tool_call(with_obj).is_none(), "object value inside arguments must reject"); + let with_arr = r#"{"id":1,"method":"tools/call","params":{"name":"get_node","arguments":{"node_id":"42","arr":[1]}}}"#; + assert!(parse_tool_call(with_arr).is_none(), "array value inside arguments must reject"); + // Scalar-only still parses. + let ok = r#"{"id":1,"method":"tools/call","params":{"name":"get_node","arguments":{"node_id":"42"}}}"#; + let call = parse_tool_call(ok).expect("scalar-only must parse"); + assert_eq!(call.tool, "get_node"); + assert_eq!(call.arguments.get("node_id"), Some(&"42".to_string())); +} + +#[test] +fn parse_tool_call_rejects_non_object_arguments_field() { + // Codex stop-gate: `arguments` present but not an object + // (e.g. `"arguments":"oops"` or `"arguments":42`) used to + // downgrade to empty args via `extract_object_body` returning + // None. Now it rejects the parse so a wire-shape error + // doesn't silently land at the dispatcher as a no-arg call. + let str_args = r#"{"id":1,"method":"tools/call","params":{"name":"get_node","arguments":"oops"}}"#; + assert!(parse_tool_call(str_args).is_none(), "string `arguments` must reject"); + let num_args = r#"{"id":1,"method":"tools/call","params":{"name":"get_node","arguments":42}}"#; + assert!(parse_tool_call(num_args).is_none(), "number `arguments` must reject"); + let arr_args = r#"{"id":1,"method":"tools/call","params":{"name":"get_node","arguments":[]}}"#; + assert!(parse_tool_call(arr_args).is_none(), "array `arguments` must reject"); + // `arguments` legitimately omitted → empty args, parse OK. + let no_args = r#"{"id":1,"method":"tools/call","params":{"name":"list_pages"}}"#; + let call = parse_tool_call(no_args).expect("missing `arguments` is legit"); + assert_eq!(call.tool, "list_pages"); + assert!(call.arguments.is_empty()); +} + #[test] fn get_node_reachable_through_stdio_path() { // End-to-end: the wire `params` parser feeds the tool's