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

fix(mcp/replace): reject empty-string drop_children too

Browse source 
Codex re-review on 2f0200b0 flagged that drop_children="" was
silently treated as false, hiding a possible mis-serialized
confirmation from the caller. Drop the empty-string special
case — only "true" / "false" / missing are now accepted; anything
else returns InvalidArgument.

Test broadened: the malformed-string case now sweeps "yes" / ""
/ "TRUE" / "1" / "0" so every common mis-spelling is covered.
This commit is contained in:
Kayshen-X 2026-05-14 21:48:41 +08:00
parent 9c6f19991c
commit 0e61ee715b
2 changed files with 14 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
crates/openpencil-shell-core/src/mcp/replace_node_tests.rs
View file

@ -216,12 +216,14 @@ fn replace_node_rejects_malformed_drop_children() {
args.insert("y".into(), "0".into());
args.insert("width".into(), "10".into());
args.insert("height".into(), "10".into());
args.insert("drop_children".into(), "yes".into()); // not "true"/"false"
match tool.call(&args) {
ToolOutcome::Err(code, msg) => {
assert_eq!(code, ToolErrorCode::InvalidArgument);
assert!(msg.contains("drop_children"));
for bad in ["yes", "", "TRUE", "1", "0"] {
args.insert("drop_children".into(), bad.into());
match tool.call(&args) {
ToolOutcome::Err(code, msg) => {
assert_eq!(code, ToolErrorCode::InvalidArgument, "input {bad:?}");
assert!(msg.contains("drop_children"), "input {bad:?} msg = {msg}");
}
_ => panic!("expected InvalidArgument on malformed drop_children {bad:?}"),
}
_ => panic!("expected InvalidArgument on malformed drop_children"),
}
}

10
crates/openpencil-shell-core/src/mcp/write_tools.rs
View file

@ -609,12 +609,14 @@ impl McpTool for ReplaceNode {
};
// Optional opt-in for destructive swaps. The applier
// refuses to drop a container's children unless this is
// `"true"`. Any other value (including "false" / "1" /
// missing) leaves the guard active.
// `"true"`. Missing → safe default false. Any other
// value (including the empty string) is malformed and
// rejected — silently treating "" as false would hide a
// mis-serialized confirmation from the caller.
let drop_children = match args.get("drop_children") {
Some(s) if s == "true" => true,
Some(s) if s == "false" || s.is_empty() => false,
None => false,
Some(s) if s == "true" => true,
Some(s) if s == "false" => false,
Some(s) => {
return ToolOutcome::Err(
ToolErrorCode::InvalidArgument,