SemianiakaVY/openpencil
1
0
Fork 0
Code Issues Pull requests Actions 1 Packages Projects Releases Wiki Activity

fix(mcp/parser): top-level arguments walker — no nested-key shadow

Browse source 
Codex BLOCKed ee0671a7 on the substring-find inside
arguments_field: `body.find("\"arguments\"")` matched any
nested key or string value containing the literal `"arguments"`,
not just the top-level params.arguments field. Two valid-but-
unusual JSON shapes bypassed the rejection contract:

1. `meta:{"arguments":{}}` alongside `"arguments":"oops"` — the
   substring scan hit meta.arguments as a Body and parsed it,
   skipping the real top-level string-typed arguments which
   should have rejected.
2. `name:"arguments"` with no real arguments field — false
   positive Malformed instead of Missing.

Replace with a proper top-level key walker that:
- skips whitespace + commas between top-level pairs;
- reads each quoted key + its value, classifying the value
  (string / object / array / number-bool-null);
- only treats `arguments` as a real hit when seen at depth 0;
- categorizes the matched value: object → Body, anything else
  → Malformed; missing entirely → Missing.

Three new regression tests in `mcp_tests.rs::
parse_tool_call_arguments_lookup_is_top_level_only` cover the
nested-shadow, string-value-collision, and deep-nested-key
cases.

359 shell-core tests pass.
This commit is contained in:
Kayshen-X 2026-05-14 22:19:53 +08:00
parent 4279ddd8e5
commit cd7d1d9eb8
2 changed files with 187 additions and 41 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

197
crates/openpencil-shell-core/src/mcp/parser.rs
View file

@ -139,56 +139,171 @@ fn extract_string_field(body: &str, key: &str) -> Option<String> {
Some(rest[val_start..i].to_string())
}
/// Tri-state lookup for MCP `arguments` inside a `params` block:
/// - Missing: `arguments` key absent (legit empty args).
/// Tri-state lookup for MCP `arguments` inside a `params` block.
/// Walks `body` at depth 0 so we ignore nested keys (e.g. a sibling
/// `meta.arguments` can't shadow the real top-level field) and
/// string values that happen to read `"arguments"`. Returns:
/// - Missing: `arguments` key absent at top level (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).
/// (e.g. `"arguments":"oops"` / `:42` / `:[]`), or the body
/// itself doesn't tokenize as a JSON object body.
///
/// Codex stop-gate, twice: first the present-but-non-object
/// downgrade to empty args; then the substring-find that
/// matched nested keys / string values containing the literal
/// `"arguments"`. Both addressed by walking top-level keys
/// explicitly.
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;
let bytes = body.as_bytes();
let mut i = 0usize;
while i < bytes.len() {
// Skip whitespace + commas between keys.
while i < bytes.len() && (bytes[i].is_ascii_whitespace() || bytes[i] == b',') {
i += 1;
}
match b {
b'"' => in_str = true,
b'{' => depth += 1,
b'}' => {
depth -= 1;
if depth == 0 {
return ParamsResult::Body(rest[1..i].to_string());
if i >= bytes.len() {
break;
}
if bytes[i] != b'"' {
return ParamsResult::Malformed;
}
// Read the quoted key.
i += 1;
let key_start = i;
while i < bytes.len() && bytes[i] != b'"' {
if bytes[i] == b'\\' {
i = i.saturating_add(2);
} else {
i += 1;
}
}
if i >= bytes.len() {
return ParamsResult::Malformed;
}
let key = &body[key_start..i];
i += 1; // past closing quote
// Whitespace, colon, whitespace.
while i < bytes.len() && bytes[i].is_ascii_whitespace() {
i += 1;
}
if i >= bytes.len() || bytes[i] != b':' {
return ParamsResult::Malformed;
}
i += 1;
while i < bytes.len() && bytes[i].is_ascii_whitespace() {
i += 1;
}
if i >= bytes.len() {
return ParamsResult::Malformed;
}
let is_args = key == "arguments";
match bytes[i] {
b'"' => {
// String value — read it to the matching quote.
i += 1;
while i < bytes.len() && bytes[i] != b'"' {
if bytes[i] == b'\\' {
i = i.saturating_add(2);
} else {
i += 1;
}
}
if i >= bytes.len() {
return ParamsResult::Malformed;
}
i += 1;
if is_args {
return ParamsResult::Malformed;
}
}
b'{' => {
let val_start = i + 1;
let mut depth = 1i32;
let mut j = i + 1;
let mut in_str = false;
let mut escape = false;
while j < bytes.len() && depth > 0 {
let c = bytes[j];
if in_str {
if escape {
escape = false;
} else if c == b'\\' {
escape = true;
} else if c == b'"' {
in_str = false;
}
} else if c == b'"' {
in_str = true;
} else if c == b'{' {
depth += 1;
} else if c == b'}' {
depth -= 1;
if depth == 0 {
break;
}
}
j += 1;
}
if depth != 0 || j >= bytes.len() {
return ParamsResult::Malformed;
}
if is_args {
return ParamsResult::Body(body[val_start..j].to_string());
}
i = j + 1;
}
b'[' => {
let mut depth = 1i32;
let mut j = i + 1;
let mut in_str = false;
let mut escape = false;
while j < bytes.len() && depth > 0 {
let c = bytes[j];
if in_str {
if escape {
escape = false;
} else if c == b'\\' {
escape = true;
} else if c == b'"' {
in_str = false;
}
} else if c == b'"' {
in_str = true;
} else if c == b'[' {
depth += 1;
} else if c == b']' {
depth -= 1;
if depth == 0 {
break;
}
}
j += 1;
}
if depth != 0 || j >= bytes.len() {
return ParamsResult::Malformed;
}
if is_args {
return ParamsResult::Malformed;
}
i = j + 1;
}
_ => {
// Number / bool / null — read until comma / close.
while i < bytes.len()
&& !matches!(bytes[i], b',' | b'}' | b' ' | b'\t' | b'\n' | b'\r')
{
i += 1;
}
if is_args {
return ParamsResult::Malformed;
}
}
_ => {}
}
}
ParamsResult::Malformed
ParamsResult::Missing
}
/// Extract a nested object field's body (without the surrounding

31
crates/openpencil-shell-core/src/mcp_tests.rs
View file

@ -532,6 +532,37 @@ fn parse_tool_call_rejects_non_object_arguments_field() {
assert!(call.arguments.is_empty());
}
#[test]
fn parse_tool_call_arguments_lookup_is_top_level_only() {
// Codex stop-gate (twice): `body.find("\"arguments\"")` was a
// substring scan that matched nested keys OR string values
// containing the literal `"arguments"`. Fixed with a real
// top-level walker. Three repros below — all valid JSON,
// none containing a real top-level `arguments` object:
//
// 1. Nested `meta.arguments` object — the substring scan
// would have hit it and downgraded a sibling
// `"arguments":"oops"` to Body silently. Now the walker
// sees the top-level `arguments` is a string → Malformed.
let shadow = r#"{"id":1,"method":"tools/call","params":{"name":"get_node","meta":{"arguments":{}},"arguments":"oops"}}"#;
assert!(
parse_tool_call(shadow).is_none(),
"nested meta.arguments must not shadow the real top-level arguments"
);
// 2. A sibling whose STRING value is literally "arguments".
// Real `arguments` is missing → Missing → empty args, parse OK.
let str_collide = r#"{"id":1,"method":"tools/call","params":{"name":"arguments"}}"#;
let call = parse_tool_call(str_collide).expect("name=\"arguments\" must not false-positive");
assert_eq!(call.tool, "arguments");
assert!(call.arguments.is_empty());
// 3. A nested `arguments` deep inside something else, with no
// top-level `arguments` at all. Still empty args, parse OK.
let deep = r#"{"id":1,"method":"tools/call","params":{"name":"get_node","other":{"x":{"arguments":42}}}}"#;
let call = parse_tool_call(deep).expect("deeply nested arguments key must not surface");
assert_eq!(call.tool, "get_node");
assert!(call.arguments.is_empty());
}
#[test]
fn get_node_reachable_through_stdio_path() {
// End-to-end: the wire `params` parser feeds the tool's