fix(mcp): hoist node-level app state on every insert path via command batch
Every generation insert/replace path now drains node-level state to one doc-root MergeAppState (weakest priority, doc-owned keys always win): insert_node + standalone replace_node wrap via with_hoisted_state; batch_program's I()/R() sibling-emit through the program finisher's own batch — emitted only after the insert/replace line succeeds so a failed best-effort line cannot leak orphan state (recorded order keeps merge before its insert). Includes the radio_group promote e2e and 8 new hoist/leak regression tests. The shared helpers landed with the script-gen series' batch_design commit.
This commit is contained in:
parent
f7d3139d7f
commit
0a8fdc1ffc
|
|
@ -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:?}"),
|
||||
|
|
|
|||
|
|
@ -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(())
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<EditorCommand>) = 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();
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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) => {}
|
||||
|
|
|
|||
Loading…
Reference in a new issue