diff --git a/crates/op-mcp/src/batch_design_tests.rs b/crates/op-mcp/src/batch_design_tests.rs index 21190c4ea..7d5fcc9e0 100644 --- a/crates/op-mcp/src/batch_design_tests.rs +++ b/crates/op-mcp/src/batch_design_tests.rs @@ -1229,8 +1229,10 @@ fn insert_node_data_hoists_node_state() { ); match tool.call(&args) { ToolOutcome::OkWithCommand(_, EditorCommand::Batch { commands }) => { - assert!(matches!(&commands[0], EditorCommand::MergeAppState { plan_idx, state } - if *plan_idx == usize::MAX && state.contains_key("on"))); + assert!( + matches!(&commands[0], EditorCommand::MergeAppState { plan_idx, state } + if *plan_idx == usize::MAX && state.contains_key("on")) + ); assert!(matches!(&commands[1], EditorCommand::InsertSubtree { .. })); } other => panic!("expected Batch command, got {other:?}"), @@ -1249,8 +1251,10 @@ fn design_content_hoists_node_state() { ); match dispatch_design_content(&args) { ToolOutcome::OkWithCommand(_, EditorCommand::Batch { commands }) => { - assert!(matches!(&commands[0], EditorCommand::MergeAppState { plan_idx, state } - if *plan_idx == usize::MAX && state.contains_key("n"))); + assert!( + matches!(&commands[0], EditorCommand::MergeAppState { plan_idx, state } + if *plan_idx == usize::MAX && state.contains_key("n")) + ); assert!(matches!(&commands[1], EditorCommand::InsertSubtree { .. })); } other => panic!("expected Batch command, got {other:?}"), diff --git a/crates/op-mcp/src/batch_program.rs b/crates/op-mcp/src/batch_program.rs index 3af54eeca..49ddcef6e 100644 --- a/crates/op-mcp/src/batch_program.rs +++ b/crates/op-mcp/src/batch_program.rs @@ -261,6 +261,14 @@ fn execute_insert(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), for (cmd, failure) in pre_commands { ctx.emit(cmd, failure)?; } + // Hoist node-level `state` as a SIBLING command — the program + // finisher batches ctx.commands itself; wrapping here would nest + // Batches, which apply rejects. Held until the insert below + // succeeds: emitting it first would leak an orphan `$app` state + // command into `ctx.commands` if the insert then fails (the line + // as a whole errors and is dropped, but a prior `ctx.emit` already + // recorded — state from a line that never landed must not ship). + let merge = super::batch_design::hoist_generation_state(&mut nodes); ctx.emit( EditorCommand::InsertAuthoredSubtree { nodes, @@ -272,6 +280,20 @@ fn execute_insert(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), parent.as_deref().unwrap_or("null") ), )?; + if let Some(merge) = merge { + // Emit AFTER the insert succeeds (a failed line must not leak + // orphan $app state), then swap the two recorded commands so + // the batch still carries MergeAppState before its insert. + // Sim-apply order between the two is immaterial: merge touches + // only doc.state, the insert only the tree. (If this merge emit + // itself ever failed post-insert, the line would error after + // its insert already landed — state dropped but never + // orphaned; an additive MergeAppState on the sim can't + // realistically fail.) + ctx.emit(merge, "merge generated app state")?; + let n = ctx.commands.len(); + ctx.commands.swap(n - 1, n - 2); + } ctx.bind(binding, &root_id); Ok(()) } @@ -355,7 +377,13 @@ fn execute_replace(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<() return Err(format!("Replace target not found: {path}")); }; let old_id = old.id_str().to_string(); - let node = parse_node_json(&args[comma + 1..], ctx.post_process)?; + let mut node = parse_node_json(&args[comma + 1..], ctx.post_process)?; + // Drain node-level `state` BEFORE the probe clone; hold the merge + // and emit it only after the replace below succeeds (emit applies + // immediately — emitting the merge first would leak an orphan + // `$app` state command into `ctx.commands` if the replace then + // fails; state from a line that never landed must not ship). + let merge = super::batch_design::hoist_generation_state(std::slice::from_mut(&mut node)); // Predict the ids `cmd_replace_subtree` will assign: it remaps off // the live seed/taken BEFORE removing the old subtree — identical @@ -375,6 +403,20 @@ fn execute_replace(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<() }, &format!("Replace failed for: {path}"), )?; + if let Some(merge) = merge { + // Emit AFTER the replace succeeds (a failed line must not leak + // orphan $app state), then swap the two recorded commands so + // the batch still carries MergeAppState before its replace. + // Sim-apply order between the two is immaterial: merge touches + // only doc.state, the replace only the tree. (If this merge + // emit itself ever failed post-replace, the line would error + // after its replace already landed — state dropped but never + // orphaned; an additive MergeAppState on the sim can't + // realistically fail.) + ctx.emit(merge, "merge generated app state")?; + let n = ctx.commands.len(); + ctx.commands.swap(n - 1, n - 2); + } ctx.bind(binding, &new_id); Ok(()) } diff --git a/crates/op-mcp/src/batch_program_tests.rs b/crates/op-mcp/src/batch_program_tests.rs index ac4e05759..e2c213c39 100644 --- a/crates/op-mcp/src/batch_program_tests.rs +++ b/crates/op-mcp/src/batch_program_tests.rs @@ -40,6 +40,18 @@ fn binding_id(envelope: &Value, binding: &str) -> String { .to_string() } +/// Recursively search a command (unwrapping `Batch`) for a leaked +/// `MergeAppState` — the regression guard for a failed stateful +/// `I()`/`R()` line (see `failed_insert_line_does_not_leak_merge_app_state` +/// / `failed_replace_line_does_not_leak_merge_app_state`). +fn contains_merge_app_state(cmd: &EditorCommand) -> bool { + match cmd { + EditorCommand::MergeAppState { .. } => true, + EditorCommand::Batch { commands } => commands.iter().any(contains_merge_app_state), + _ => false, + } +} + #[test] fn mixed_program_executes_all_ops_with_shared_bindings_and_slash_paths() { let mut state = sample(); @@ -224,6 +236,122 @@ D("ghost")"##; assert_eq!(first.base().name.as_deref(), Some("Swapped")); } +#[test] +fn replace_program_hoists_node_state() { + let state = sample(); + let program = r##"swap=R("n13", {"type":"frame","name":"Counter","width":120,"height":24,"state":{"n":{"type":"int","default":0}}}) +D("ghost")"##; + let (envelope, cmd) = call_operations(&state, program); + assert!(envelope.get("errors").is_none(), "{envelope}"); + match cmd.expect("command") { + EditorCommand::Batch { commands } => { + assert!( + matches!(&commands[0], EditorCommand::MergeAppState { plan_idx, state } + if *plan_idx == usize::MAX && state.contains_key("n")) + ); + let replacement = commands + .iter() + .find_map(|c| match c { + EditorCommand::ReplaceSubtree { node, .. } => Some(node), + _ => None, + }) + .expect("ReplaceSubtree in batch"); + let v = serde_json::to_value(replacement.as_ref()).expect("json"); + assert!( + v.get("state").is_none(), + "replacement state must be stripped" + ); + } + other => panic!("expected Batch, got {other:?}"), + } +} + +#[test] +fn failed_insert_line_does_not_leak_merge_app_state() { + // A stateful `I()` line whose parent doesn't resolve to a container + // fails at the `InsertAuthoredSubtree` emit — AFTER the node's + // `state` would already have been hoisted. The line's error is + // collected and the line dropped (TS best-effort semantics), so its + // hoisted `MergeAppState` must never survive into the surviving + // command. `U(...)` (rather than a second `I()`) is the second line + // so `parse_operations` can't take the single-shot Insert-only fast + // path and this program is guaranteed to run through the mixed-DSL + // executor (`run_batch_design_program`) this fix targets. + let mut state = sample(); + let program = r##"a=I("nonexistent-parent", {"type":"frame","name":"Ghost","width":100,"height":50,"state":{"n":{"type":"int","default":0}}}) +U("n11", {"name":"Renamed"})"##; + let (envelope, cmd) = call_operations(&state, program); + let errors = envelope["errors"].as_array().expect("errors present"); + assert_eq!(errors.len(), 1, "{envelope}"); + assert!( + errors[0]["error"] + .as_str() + .unwrap() + .starts_with("Insert parent not found or not a container"), + "{envelope}" + ); + let cmd = cmd.expect("the surviving U() line still emits a command"); + assert!( + !contains_merge_app_state(&cmd), + "failed insert line must not leak orphan MergeAppState: {cmd:?}" + ); + assert!( + state.apply(cmd), + "surviving command must still apply cleanly" + ); +} + +#[test] +fn failed_replace_line_does_not_leak_merge_app_state() { + // The insert case above fails at the emit (parent-not-container is + // only checked inside `InsertAuthoredSubtree`'s apply). A plain + // `R("no-such-id", ...)` instead fails EARLIER, in `find_node_by_path` + // — before `hoist_generation_state` ever runs — so it can't exercise + // the vulnerable emit-ordering window this fix closes. To reach that + // window for replace, force the `ReplaceSubtree` emit ITSELF to fail + // post-hoist: a `pageId` that resolves to no page. The program's + // initial page-pin silently no-ops on the same unresolvable id + // (leaving the sim on its real, default active page), so + // `find_node_by_path` still finds "n13" and the node's `state` is + // hoisted — but the ReplaceSubtree command's own page stamp is then + // rejected at apply, reproducing exactly the failure shape codex + // flagged for `I()`. + let tool = batch_design_snapshot(&sample()); + let mut args = BTreeMap::new(); + args.insert("pageId".into(), "does-not-exist".into()); + args.insert( + "operations".into(), + r##"swap=R("n13", {"type":"frame","name":"Ghost","width":100,"height":50,"state":{"n":{"type":"int","default":0}}}) +D("ghost")"## + .into(), + ); + let (envelope, cmd): (Value, Option) = match tool.call(&args) { + ToolOutcome::OkJson(json) => (serde_json::from_str(&json).expect("json"), None), + ToolOutcome::OkJsonWithCommand(json, cmd) => { + (serde_json::from_str(&json).expect("json"), Some(cmd)) + } + other => panic!("expected a TS result envelope, got {other:?}"), + }; + let errors = envelope["errors"].as_array().expect("errors present"); + assert_eq!(errors.len(), 1, "{envelope}"); + assert!( + errors[0]["error"] + .as_str() + .unwrap() + .starts_with("Replace failed for:"), + "{envelope}" + ); + // `D("ghost")` is a silent no-op (unknown id), so nothing else could + // legitimately contribute a command here — any surviving command + // must NOT be (or contain) the orphaned MergeAppState. + assert!( + cmd.as_ref() + .map(|c| !contains_merge_app_state(c)) + .unwrap_or(true), + "failed replace line must not leak orphan MergeAppState: {cmd:?}" + ); +} + #[test] fn image_op_requires_ts_quoted_syntax_and_resolves_binding_parents() { let mut state = sample(); diff --git a/crates/op-mcp/src/replace_node_tests.rs b/crates/op-mcp/src/replace_node_tests.rs index c9386edfd..8fb7af2ec 100644 --- a/crates/op-mcp/src/replace_node_tests.rs +++ b/crates/op-mcp/src/replace_node_tests.rs @@ -131,6 +131,42 @@ fn replace_node_accepts_ts_data_tree_as_subtree() { } } +#[test] +fn replace_node_data_hoists_node_state() { + // A generated `data` replacement declaring node-level `state` must + // hoist it to a doc-root MergeAppState (unplanned priority) and + // strip it off the ReplaceSubtree payload — same contract as the + // insert paths and batch_program's R(). + let tool = replace_node_snapshot(); + let mut args = BTreeMap::new(); + args.insert("nodeId".into(), "n11".into()); + args.insert( + "data".into(), + r##"{"type":"frame","name":"Counter","width":200,"height":100,"state":{"n":{"type":"int","default":0}},"children":[{"type":"text","content":"hi"}]}"## + .into(), + ); + match tool.call(&args) { + ToolOutcome::OkWithCommand(_, EditorCommand::Batch { commands }) => { + assert_eq!(commands.len(), 2); + assert!( + matches!(&commands[0], EditorCommand::MergeAppState { plan_idx, state } + if *plan_idx == usize::MAX && state.contains_key("n")) + ); + match &commands[1] { + EditorCommand::ReplaceSubtree { node, .. } => { + let v = serde_json::to_value(node.as_ref()).expect("json"); + assert!( + v.get("state").is_none(), + "replacement state must be stripped" + ); + } + other => panic!("expected ReplaceSubtree second, got {other:?}"), + } + } + other => panic!("expected Batch command, got {other:?}"), + } +} + #[test] fn replace_node_preserves_ts_leaf_data_with_extra_fields_as_subtree() { let mut args = BTreeMap::new(); diff --git a/crates/op-mcp/src/write_tools.rs b/crates/op-mcp/src/write_tools.rs index 407f3a607..2dcc2e45f 100644 --- a/crates/op-mcp/src/write_tools.rs +++ b/crates/op-mcp/src/write_tools.rs @@ -268,11 +268,16 @@ fn ts_data_tree_command( .map(|s| s.trim()) .filter(|s| !s.is_empty()) .map(str::to_string); - Ok(Some(EditorCommand::InsertSubtree { - nodes: vec![node], - parent_id, - page_id, - })) + let mut nodes = vec![node]; + let hoist = super::batch_design::hoist_generation_state(&mut nodes); + Ok(Some(super::batch_design::with_hoisted_state( + hoist, + EditorCommand::InsertSubtree { + nodes, + parent_id, + page_id, + }, + ))) } #[allow(clippy::result_large_err)] @@ -564,7 +569,7 @@ impl McpTool for ReplaceNode { Err(e) => return e, }; match ts_data_tree_node(args) { - Ok(Some(node)) => { + Ok(Some(mut node)) => { let drop_children = match parse_drop_children_arg(args) { Ok(v) => v, Err(e) => return e, @@ -578,14 +583,22 @@ impl McpTool for ReplaceNode { .map(str::to_string); let mut out = BTreeMap::new(); out.insert("wrote".into(), "true".into()); + // The replacement is GENERATED node JSON — hoist any + // node-level `state` to the document root, same as the + // insert paths and batch_program's R(). + let hoist = + super::batch_design::hoist_generation_state(std::slice::from_mut(&mut node)); return ToolOutcome::OkWithCommand( out, - EditorCommand::ReplaceSubtree { - node_id, - node: Box::new(node), - drop_children, - page_id, - }, + super::batch_design::with_hoisted_state( + hoist, + EditorCommand::ReplaceSubtree { + node_id, + node: Box::new(node), + drop_children, + page_id, + }, + ), ); } Ok(None) => {}