fix(mcp): fall back to root insert for phantom parent refs
A generated program that parents its first line onto a binding that was never created used to ride into InsertAuthoredSubtree unvalidated; the host's existence check then rejected the WHOLE otherwise-valid program and the section was lost to retries. Insert at the document root instead and surface a warning in the envelope. Also: drop the ambiguous bare-word 'auto' sizing (schema default wins instead of inverting intent), and recover word-based textGrowth spellings (fixed_width_and_height and friends) instead of silently removing them.
This commit is contained in:
parent
1e3fdc91e1
commit
c1fb08f158
|
|
@ -705,10 +705,11 @@ fn normalize_layout_object(obj: &mut serde_json::Map<String, serde_json::Value>)
|
|||
|
||||
/// `width` / `height` accept `fill_container` / `fit_content` / a number. A weak
|
||||
/// model sometimes appends a type-hint suffix (`fill_container_str` — a leaked
|
||||
/// variable name) or uses a CSS-ish spelling (`fill-container` / `hug` / `auto`),
|
||||
/// which fails the `Sizing` enum and drops the whole node. Map the known
|
||||
/// content-hug / fill spellings back to the canonical keyword; leave numbers,
|
||||
/// numeric strings, and already-valid / unrecognised values untouched.
|
||||
/// variable name) or uses a CSS-ish spelling (`fill-container` / `hug`), which
|
||||
/// fails the `Sizing` enum and drops the whole node. Map the known content-hug /
|
||||
/// fill spellings back to the canonical keyword; DROP the ambiguous `auto`
|
||||
/// (schema default wins); leave numbers, numeric strings, and already-valid /
|
||||
/// unrecognised values untouched.
|
||||
fn normalize_sizing_keyword(obj: &mut serde_json::Map<String, serde_json::Value>, key: &str) {
|
||||
let Some(serde_json::Value::String(raw)) = obj.get(key) else {
|
||||
return;
|
||||
|
|
@ -723,9 +724,17 @@ fn normalize_sizing_keyword(obj: &mut serde_json::Map<String, serde_json::Value>
|
|||
let normalized = match canon.as_str() {
|
||||
"fill_container" | "fillcontainer" | "fill" | "container" | "full" | "fill_width"
|
||||
| "fill_parent" => Some("fill_container"),
|
||||
"fit_content" | "fitcontent" | "fit" | "hug" | "hug_content" | "auto" | "content" => {
|
||||
"fit_content" | "fitcontent" | "fit" | "hug" | "hug_content" | "content" => {
|
||||
Some("fit_content")
|
||||
}
|
||||
// CSS `auto` is AMBIGUOUS: for a block's width it usually means
|
||||
// stretch/fill, for height it means hug — forcing either direction
|
||||
// inverts the author's intent half the time. Drop the key so the node
|
||||
// survives deserialization with the schema default instead.
|
||||
"auto" => {
|
||||
obj.remove(key);
|
||||
return;
|
||||
}
|
||||
_ => None,
|
||||
};
|
||||
if let Some(valid) = normalized {
|
||||
|
|
@ -777,6 +786,19 @@ fn normalize_text_growth(obj: &mut serde_json::Map<String, serde_json::Value>) {
|
|||
"fixed-width-height" | "fixed-width-and-height" | "fixed" => Some("fixed-width-height"),
|
||||
_ => None,
|
||||
};
|
||||
// Unrecognized spellings: recover the intent from the words before falling
|
||||
// back to removal ("fixed_width_and_height", "fixedWidth" and friends carry
|
||||
// a clear meaning — silently reverting them to the default undoes a
|
||||
// deliberate wrap request).
|
||||
let normalized = normalized.or_else(|| {
|
||||
if canon.contains("width") && canon.contains("height") {
|
||||
Some("fixed-width-height")
|
||||
} else if canon.contains("width") {
|
||||
Some("fixed-width")
|
||||
} else {
|
||||
None
|
||||
}
|
||||
});
|
||||
match normalized {
|
||||
Some(valid) => {
|
||||
obj.insert(
|
||||
|
|
|
|||
|
|
@ -133,6 +133,21 @@ impl BatchDesign {
|
|||
.collect();
|
||||
result["promoted"] = json!(notes);
|
||||
}
|
||||
// A parent ref that is NEITHER a binding NOR a node in the snapshot
|
||||
// (a weak model copies the `sec` example binding as its first line's
|
||||
// parent) would fail the host's existence check and sink the WHOLE
|
||||
// otherwise-valid program (measured: an orchestrator stats subtask
|
||||
// retried its complete 4-card program away over a phantom `sec`).
|
||||
// Insert at the root instead and say so — the section coalesce /
|
||||
// finalize passes reparent stray roots anyway.
|
||||
let mut parent_id = parent_id;
|
||||
if parent_id.is_real() && !self.existing_ids.contains(&parent_id) {
|
||||
result["warnings"] = json!([format!(
|
||||
"parent '{}' does not exist — inserted at the document root instead",
|
||||
parent_id.as_str()
|
||||
)]);
|
||||
parent_id = NodeId::NONE;
|
||||
}
|
||||
ToolOutcome::OkJsonWithCommand(
|
||||
result.to_string(),
|
||||
EditorCommand::InsertAuthoredSubtree {
|
||||
|
|
|
|||
|
|
@ -1081,3 +1081,66 @@ fn batch_insert_command_rejects_empty_items() {
|
|||
page_id: None,
|
||||
}));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn batch_design_drops_ambiguous_auto_sizing_and_recovers_text_growth_words() {
|
||||
// `width:"auto"` is ambiguous in CSS (fill for a block's width, hug for its
|
||||
// height) — forcing either direction inverts intent half the time, so the
|
||||
// key must be DROPPED (schema default wins) while the node itself survives.
|
||||
// A misspelled `textGrowth` carrying clear words ("fixed_width_and_height")
|
||||
// must recover its meaning instead of silently reverting to the default.
|
||||
let tool = batch_design_snapshot(&sample());
|
||||
let mut args = BTreeMap::new();
|
||||
args.insert(
|
||||
"operations".into(),
|
||||
r##"root=I(null, {"type":"frame","name":"Block","width":"auto","height":"fit_content","children":[{"type":"text","name":"T","content":"hello","textGrowth":"fixed_width_and_height"}]})"##
|
||||
.into(),
|
||||
);
|
||||
let ToolOutcome::OkJsonWithCommand(_, EditorCommand::InsertAuthoredSubtree { nodes, .. }) =
|
||||
tool.call(&args)
|
||||
else {
|
||||
panic!("expected InsertSubtree command");
|
||||
};
|
||||
let value = serde_json::to_value(&nodes[0]).expect("node json");
|
||||
assert!(
|
||||
value.get("width").map(|w| !w.is_string()).unwrap_or(true),
|
||||
"ambiguous auto width dropped, got {:?}",
|
||||
value.get("width")
|
||||
);
|
||||
assert_eq!(value["height"], "fit_content", "valid keyword untouched");
|
||||
let text = &value["children"][0];
|
||||
assert_eq!(
|
||||
text["textGrowth"], "fixed-width-height",
|
||||
"word-based textGrowth spelling recovered"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn batch_design_falls_back_to_root_for_phantom_parent_binding() {
|
||||
// A weak model copies the `sec` example binding as its FIRST line's
|
||||
// parent. The phantom parent used to ride into `InsertAuthoredSubtree`
|
||||
// unvalidated and the host rejected the WHOLE otherwise-valid program
|
||||
// (an orchestrator stats subtask retried its complete 4-card section
|
||||
// away). The tool must fall back to a root insert and surface a warning.
|
||||
let state = op_editor_core::EditorState::new();
|
||||
let tool = batch_design_snapshot(&state);
|
||||
let mut args = BTreeMap::new();
|
||||
args.insert(
|
||||
"operations".into(),
|
||||
"row=I(sec, {\"type\":\"frame\",\"name\":\"Stat Row\",\"layout\":\"horizontal\",\"gap\":24})\nc1=I(row, {\"type\":\"frame\",\"name\":\"Card\",\"layout\":\"vertical\"})".to_string(),
|
||||
);
|
||||
let ToolOutcome::OkJsonWithCommand(json, cmd) = tool.call(&args) else {
|
||||
panic!("expected a command outcome");
|
||||
};
|
||||
assert!(json.contains("warnings"), "phantom parent surfaced: {json}");
|
||||
let mut s2 = op_editor_core::EditorState::new();
|
||||
assert!(s2.apply(cmd), "root-fallback insert must apply cleanly");
|
||||
assert_eq!(s2.active_children().len(), 1, "one root landed");
|
||||
use op_editor_core::PenNodeExt;
|
||||
let root = &s2.active_children()[0];
|
||||
assert_eq!(
|
||||
root.children().map(|c| c.len()),
|
||||
Some(1),
|
||||
"card nested in row"
|
||||
);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue