From 4279ddd8e5bf32a33070462934bb20af7cacefdd Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Thu, 14 May 2026 22:14:29 +0800 Subject: [PATCH] fix(mcp/parser): reject non-object \`arguments\` + add tools/call tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex BLOCKed 0a434e6b on three items: 1. MCP arguments non-object downgrade — \`"arguments":"oops"\` used to flow through extract_object_body returning None and parse_tool_call downgrading to empty args. Replace the call site with a tri-state \`arguments_field\` helper: - Missing → empty args (legit MCP can omit \`arguments\`). - Body(s) → parse the object body. - Malformed → reject the parse. 2. Test coverage gap — the structured-rejection tests only exercised the legacy/direct line-protocol shape. Add two new tests under mcp_tests: - parse_tool_call_rejects_structured_values_in_mcp_tools_call_shape: nested object + array inside MCP \`arguments\` both reject. - parse_tool_call_rejects_non_object_arguments_field: string / number / array \`arguments\` reject; missing \`arguments\` still parses as empty args. 3. Stale doc comment on parse_flat_object_body — said nested values are "skipped", now correctly describes the wire-layer rejection contract. 358 shell-core tests pass (was 356 — two new MCP tools/call shape tests). --- .../openpencil-shell-core/src/mcp/parser.rs | 78 ++++++++++++++++--- crates/openpencil-shell-core/src/mcp_tests.rs | 36 +++++++++ 2 files changed, 104 insertions(+), 10 deletions(-) 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