#2253: crop.aspect checks orientation before the current/toggle paths

A bad `orientation` was refused only on the refit path: "current" and an
unlocked "toggle" returned before the check, so {"aspect": "current",
"orientation": "bogus"} succeeded silently, and a value that isn't a string
fell back to the default. It is parsed first now, so every path refuses a
wrong word, a number, a boolean or null, and nothing changes (CodeRabbit on
PR #2280).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AbegtYaPuwqZ81pGfELsFK

crates/engine/src/cmd/develop.rs

@@ -642,6 +642,14 @@ pub fn specs() -> Vec<CommandSpec> {
             |s, p| {
                 let id = active(s, "crop.aspect")?;
                 let (w, h) = crop_dims(s, id);
+                // checked first, whatever the aspect: a bad value is an error on every path
+                let turn = match p.get("orientation") {
+                    Some(Value::String(o)) if o == "crop" => Turn::LikeCrop,
+                    Some(Value::String(o)) if o == "given" => Turn::AsGiven,
+                    Some(_) => return Err(bad("crop.aspect", "orientation is \"crop\" or \"given\"")),
+                    None if matches!(p.get("aspect"), Some(Value::Array(_))) => Turn::AsGiven,
+                    None => Turn::LikeCrop,
+                };
                 let locked = s.develop_of(id).is_some_and(|d| d.crop.aspect.is_some());
                 let lock_current = match p.get("aspect").and_then(Value::as_str) {
                     Some("current") => true,
@@ -656,13 +664,6 @@ pub fn specs() -> Vec<CommandSpec> {
                         Ok(())
                     });
                 }
-                let turn = match p.get("orientation").and_then(Value::as_str) {
-                    Some("crop") => Turn::LikeCrop,
-                    Some("given") => Turn::AsGiven,
-                    Some(_) => return Err(bad("crop.aspect", "orientation is \"crop\" or \"given\"")),
-                    None if matches!(p.get("aspect"), Some(Value::Array(_))) => Turn::AsGiven,
-                    None => Turn::LikeCrop,
-                };
                 let aspect = match p.get("aspect") {
                     Some(Value::String(a)) if a == "toggle" => None,
                     Some(Value::String(a)) if a == "free" => None,

crates/engine/src/tests_crop_aspect.rs

@@ -66,3 +66,21 @@ fn orientation_overrides_and_is_checked() {
     let e = s.execute("crop.aspect", &json!({"aspect": "2x3", "orientation": "sideways"})).unwrap_err();
     assert!(e.to_string().contains("orientation"), "{e}");
 }
+
+/// A bad `orientation` is refused on every path, the lock-as-is ones ("current", an unlocked
+/// "toggle") included, and whether it is a wrong word or not a string at all; nothing changes.
+#[test]
+fn a_bad_orientation_is_refused_before_anything_is_locked() {
+    let mut s = landscape_photo();
+    let before = s.develop_of(s.selection.active.unwrap()).unwrap().crop.clone();
+    for aspect in [json!("current"), json!("toggle"), json!("2x3"), json!([2, 3]), json!("free")] {
+        for orientation in [json!("sideways"), json!(1), json!(true), json!(null)] {
+            let e = s.execute("crop.aspect", &json!({"aspect": aspect, "orientation": orientation})).unwrap_err();
+            assert!(e.to_string().contains("orientation"), "{aspect} {orientation}: {e}");
+            assert_eq!(s.develop_of(s.selection.active.unwrap()).unwrap().crop, before, "{aspect} {orientation}: untouched");
+        }
+    }
+    // the valid words still lock as is
+    aspect(&mut s, json!({"aspect": "current", "orientation": "crop"}));
+    assert!(s.develop_of(s.selection.active.unwrap()).unwrap().crop.aspect.is_some());
+}