Reject ambiguous origin and construction plane names (#247)

* Reject ambiguous origin and construction plane names

* Reserve origin plane names for construction planes; suggest the other alias

A construction plane can no longer be created with or renamed to XY, XZ, YZ, Top, Front or
Right, so a plane argument can't become ambiguous in new designs. For older designs that
already hold such a plane, the ambiguity error now also names the origin plane's other
alias (XY for Top...). The existing tests build those legacy names directly.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

---------

Co-authored-by: bherbruck <[email protected]>
Co-authored-by: Claude Opus 5.5 <[email protected]>

crates/engine/src/cmd/edit.rs

@@ -145,6 +145,9 @@ fn rename(s: &mut Session, p: &Value) -> Result<Value> {
     if s.doc.features.iter().any(|f| f.name == name && f.id != id) {
         return Err(bad(cmd, format!("another feature is called `{name}`")));
     }
+    if s.doc.feature(id).is_some_and(|f| matches!(f.kind, FeatureKind::ConstructionPlane { .. })) {
+        super::features::check_plane_name(name, cmd)?;
+    }
     if let Some(f) = s.doc_mut().feature_mut(id) {
         f.name = name.to_string();
     }

crates/engine/src/cmd/features.rs

@@ -417,6 +417,11 @@ fn add_feature_in(s: &mut Session, p: &Value, kind: FeatureKind, world: bool) ->
     if participants.len() > 10_000 || participants.iter().any(|b| b != "*" && s.model.state().body(b).is_none()) {
         return Err(EngineError::Other("`participants` must list existing bodies".into()));
     }
+    if matches!(kind, FeatureKind::ConstructionPlane { .. })
+        && let Some(n) = name
+    {
+        check_plane_name(n, "construct.plane")?;
+    }
     let id = s.doc_mut().add_feature(kind, name)?;
     if let Some(f) = s.doc_mut().feature_mut(id) {
         f.participants = participants;
@@ -977,6 +982,31 @@ fn mirror(s: &mut Session, p: &Value) -> Result<Value> {
     add_feature(s, p, FeatureKind::Mirror { features, plane, bodies, combine })
 }
 
+/// The other name of an origin plane (`Top` for `XY`, `XY` for `Top`...), if `name` is one.
+pub(super) fn origin_plane_alias(name: &str) -> Option<&'static str> {
+    match name.trim().to_ascii_uppercase().as_str() {
+        "XY" => Some("Top"),
+        "TOP" => Some("XY"),
+        "XZ" => Some("Front"),
+        "FRONT" => Some("XZ"),
+        "YZ" => Some("Right"),
+        "RIGHT" => Some("YZ"),
+        _ => None,
+    }
+}
+
+/// A construction plane may not take an origin plane's name (`XY`, `Top`...): a plane
+/// parameter could then mean either.
+pub(super) fn check_plane_name(name: &str, cmd: &str) -> Result<()> {
+    if solvecraft_geom::Plane::named(name.trim()).is_some() {
+        return Err(bad(
+            cmd,
+            format!("`{}` is reserved for an origin plane (XY, XZ, YZ, Top, Front, Right); give the construction plane another name", name.trim()),
+        ));
+    }
+    Ok(())
+}
+
 /// A plane reference from a parameter: an origin plane, a construction plane (by name or
 /// feature id; kept by name, so the feature follows the plane when it changes), or an
 /// explicit plane `{origin, x_dir, y_dir}` / `{origin, normal}`. Every command that takes a
@@ -990,7 +1020,22 @@ pub(super) fn plane_param(s: &Session, v: Option<&Value>, cmd: &str) -> Result<s
         None => Err(bad(cmd, format!("no plane `{r}` (a plane is {WANT})"))),
     };
     match v {
-        Some(Value::String(n)) if solvecraft_geom::Plane::named(n.trim()).is_some() => Ok(PlaneRef::Origin { name: n.trim().to_ascii_uppercase() }),
+        Some(Value::String(n)) if solvecraft_geom::Plane::named(n.trim()).is_some() => {
+            if let Some(f) = s.doc.find_feature(n.trim())
+                && matches!(f.kind, FeatureKind::ConstructionPlane { .. })
+            {
+                return Err(bad(
+                    cmd,
+                    format!(
+                        "plane `{}` is ambiguous: an origin-plane alias and a construction plane; use `{}` for the origin plane, construction plane feature id {}, or an explicit {{origin, normal}} plane",
+                        n.trim(),
+                        origin_plane_alias(n).unwrap_or("XY"),
+                        f.id
+                    ),
+                ));
+            }
+            Ok(PlaneRef::Origin { name: n.trim().to_ascii_uppercase() })
+        }
         Some(Value::String(n)) => construction(n.trim()),
         Some(Value::Number(id)) => match id.as_u64() {
             Some(id) => construction(&id.to_string()),

crates/engine/src/cmd/section.rs

@@ -25,6 +25,8 @@ fn section(s: &mut Session, p: &Value) -> Result<Value> {
     // A named plane's world axis: which component of a point lies along its normal.
     let (origin, normal, axis) = match p.get("plane") {
         Some(Value::String(n)) if Plane::named(n.trim()).is_some() => {
+            // Apply the same ambiguity check as modelling commands before treating a name as an origin alias.
+            super::features::plane_param(s, p.get("plane"), cmd)?;
             let pl = Plane::named(n.trim()).ok_or_else(|| bad(cmd, format!("unknown plane `{n}`")))?;
             let normal = pl.normal();
             let axis = [normal.x, normal.y, normal.z].iter().position(|c| c.abs() > 0.5);

crates/engine/src/construct_tests.rs

@@ -272,3 +272,116 @@ fn plane_perpendicular_to_a_plane_through_a_line() {
     // A line square to the plane has no single answer.
     assert!(s.execute("construct.plane.perpendicular", &json!({"plane": "XY", "line": {"axis": "Z"}})).is_err());
 }
+
+#[test]
+fn ambiguous_plane_aliases_refuse_without_document_or_history_changes() {
+    for alias in ["Top", "Front", "Right", "XY", "XZ", "YZ"] {
+        let mut s = Session::default();
+        run(&mut s, "parameters.add", json!({"name":"height","expression":"19.05 mm"}));
+        run(&mut s, "sketch.create", json!({"plane":alias}));
+        run(&mut s, "sketch.finish", json!({}));
+        let made = run(&mut s, "construct.plane.offset", json!({"base":{"origin":[0,0,0],"normal":[0,0,1]},"offset":"height","name":"Lid"}));
+        let id = made["feature"].as_u64().unwrap();
+        legacy_plane_name(&mut s, id, alias);
+        let before = s.doc.to_json();
+        let history = s.undo.len();
+        for command in ["sketch.create", "inspect.section"] {
+            let error = s.execute(command, &json!({"plane":format!(" {} ",alias.to_lowercase())})).unwrap_err();
+            assert!(error.to_string().contains("ambiguous"), "{error}");
+            assert!(error.to_string().contains(&format!("feature id {id}")), "{error}");
+            let other = match alias {
+                "Top" => "XY",
+                "XY" => "Top",
+                "Front" => "XZ",
+                "XZ" => "Front",
+                "Right" => "YZ",
+                _ => "Right",
+            };
+            assert!(error.to_string().contains(&format!("use `{other}` for the origin plane")), "{error}");
+            assert_eq!(s.doc.to_json(), before);
+            assert_eq!(s.undo.len(), history);
+            assert!(s.section.is_none());
+        }
+        run(&mut s, "sketch.create", json!({"plane":id}));
+        let inspected = run(&mut s, "sketch.inspect", json!({}));
+        assert_eq!(inspected["plane"]["origin"], json!([0.0, 0.0, 19.05]));
+        let cut = run(&mut s, "inspect.section", json!({"plane":id}));
+        assert_eq!(cut["section"]["origin"], json!([0.0, 0.0, 19.05]));
+        run(&mut s, "parameters.change", json!({"name":"height","expression":"25.4 mm"}));
+        let mut reopened = Session::new(solvecraft_doc::Document::from_json(&s.doc.to_json()).unwrap());
+        let old = run(&mut reopened, "sketch.inspect", json!({"sketch":"Sketch1"}));
+        assert_eq!(old["plane"]["origin"], json!([0.0, 0.0, 0.0]), "saved origin reference is not reinterpreted");
+        let linked = run(&mut reopened, "sketch.inspect", json!({"sketch":"Sketch2"}));
+        assert_eq!(linked["plane"]["origin"], json!([0.0, 0.0, 25.4]), "disambiguated reference follows edits and reopen");
+    }
+}
+
+#[test]
+fn disambiguated_top_plane_pocket_survives_undo_and_cold_reopen() {
+    let mut s = Session::default();
+    run(&mut s, "sketch.create", json!({"plane":"XY"}));
+    run(&mut s, "sketch.rectangle.two_point", json!({"p0":[0,0],"p1":[100,200]}));
+    run(&mut s, "sketch.finish", json!({}));
+    run(&mut s, "solid.extrude", json!({"distance":19.05,"name":"Plate"}));
+    let top = run(&mut s, "construct.plane.offset", json!({"base":"XY","offset":19.05,"name":"Lid"}));
+    legacy_plane_name(&mut s, top["feature"].as_u64().unwrap(), "Top");
+    assert!(s.execute("sketch.create", &json!({"plane":"Top"})).unwrap_err().to_string().contains("ambiguous"));
+    run(&mut s, "sketch.create", json!({"plane":top["feature"]}));
+    run(&mut s, "sketch.rectangle.two_point", json!({"p0":[3,3],"p1":[97,30]}));
+    run(&mut s, "sketch.finish", json!({}));
+    let before = s.doc.to_json();
+    run(&mut s, "solid.extrude", json!({"distance":12.7,"direction":"negative","operation":"cut","name":"Pocket"}));
+    let saved = s.doc.to_json();
+    let expected = 100.0 * 200.0 * 19.05 - 94.0 * 27.0 * 12.7;
+    let measured = run(&mut s, "inspect.measure", json!({}));
+    assert!((measured["total"]["volume_mm3"].as_f64().unwrap() - expected).abs() < 1e-6, "{measured}");
+    run(&mut s, "edit.undo", json!({}));
+    assert_eq!(s.doc.to_json(), before);
+    run(&mut s, "edit.redo", json!({}));
+    assert_eq!(s.doc.to_json(), saved);
+    let mut reopened = Session::new(solvecraft_doc::Document::from_json(&saved).unwrap());
+    let measured = run(&mut reopened, "inspect.measure", json!({}));
+    assert!((measured["total"]["volume_mm3"].as_f64().unwrap() - expected).abs() < 1e-6, "{measured}");
+}
+
+/// A construction plane named like an origin plane, as an older saved design can hold.
+fn legacy_plane_name(s: &mut Session, id: u64, name: &str) {
+    s.doc_mut().feature_mut(id).unwrap().name = name.to_string();
+}
+
+#[test]
+fn construction_planes_cannot_take_origin_plane_names() {
+    let mut s = Session::default();
+    let made = run(&mut s, "construct.plane.offset", json!({"base":"XY","offset":5,"name":"Lid"}));
+    let id = made["feature"].as_u64().unwrap();
+    for alias in ["XY", "XZ", "YZ", "Top", "Front", "Right", " top ", "xz"] {
+        let before = s.doc.to_json();
+        let history = s.undo.len();
+        let created = s.execute("construct.plane.offset", &json!({"base":"XY","offset":10,"name":alias})).unwrap_err();
+        assert!(created.to_string().contains("reserved for an origin plane"), "{created}");
+        let renamed = s.execute("timeline.rename", &json!({"feature":id,"name":alias})).unwrap_err();
+        assert!(renamed.to_string().contains("reserved for an origin plane"), "{renamed}");
+        assert_eq!(s.doc.to_json(), before);
+        assert_eq!(s.undo.len(), history);
+    }
+    // Other features may still use the names; only a plane parameter could confuse them.
+    run(&mut s, "sketch.create", json!({"plane":"XY"}));
+    run(&mut s, "sketch.finish", json!({}));
+    run(&mut s, "timeline.rename", json!({"feature":"Sketch1","name":"Top"}));
+    run(&mut s, "timeline.rename", json!({"feature":id,"name":"Topside"}));
+}
+
+#[test]
+fn ambiguity_error_names_the_other_origin_alias() {
+    for (name, other) in [("Top", "XY"), ("XY", "Top"), ("Front", "XZ"), ("XZ", "Front"), ("Right", "YZ"), ("YZ", "Right")] {
+        let mut s = Session::default();
+        let made = run(&mut s, "construct.plane.offset", json!({"base":"XY","offset":5,"name":"Lid"}));
+        legacy_plane_name(&mut s, made["feature"].as_u64().unwrap(), name);
+        let error = s.execute("sketch.create", &json!({"plane":name})).unwrap_err().to_string();
+        assert!(error.contains(&format!("use `{other}` for the origin plane")), "{error}");
+        // The suggested alias is unambiguous and reaches the origin plane.
+        run(&mut s, "sketch.create", json!({"plane":other}));
+        let inspected = run(&mut s, "sketch.inspect", json!({}));
+        assert_eq!(inspected["plane"]["origin"], json!([0.0, 0.0, 0.0]));
+    }
+}