diff --git a/crates/op-mcp/src/batch_design.rs b/crates/op-mcp/src/batch_design.rs index 0cd04f3dd..7e9a19102 100644 --- a/crates/op-mcp/src/batch_design.rs +++ b/crates/op-mcp/src/batch_design.rs @@ -705,10 +705,11 @@ fn normalize_layout_object(obj: &mut serde_json::Map) /// `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, 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 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) { "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( diff --git a/crates/op-mcp/src/batch_design_result.rs b/crates/op-mcp/src/batch_design_result.rs index 4138d2b38..920025a24 100644 --- a/crates/op-mcp/src/batch_design_result.rs +++ b/crates/op-mcp/src/batch_design_result.rs @@ -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 { diff --git a/crates/op-mcp/src/batch_design_tests.rs b/crates/op-mcp/src/batch_design_tests.rs index ad42ee8b5..c8dbd5800 100644 --- a/crates/op-mcp/src/batch_design_tests.rs +++ b/crates/op-mcp/src/batch_design_tests.rs @@ -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" + ); +}