← Commit history

Merge PR #454: 1.0.107: kicad_move_footprint refuses unknown arguments (PR #70) without refusing real calls

Colby Knox ·ae67b65c1d ·23h ago ·parent 3174890
6 files changed +73−19
rust/Cargo.lock+3−3
@@ -319,7 +319,7 @@ dependencies = [  [[package]] name = "kicad-bridge"-version = "1.0.106"+version = "1.0.107" dependencies = [  "adom-eda-checks",  "kicad-core",@@ -331,7 +331,7 @@ dependencies = [  [[package]] name = "kicad-core"-version = "1.0.106"+version = "1.0.107" dependencies = [  "kicad-ipc-rs",  "kicad-platform",@@ -358,7 +358,7 @@ dependencies = [  [[package]] name = "kicad-platform"-version = "1.0.106"+version = "1.0.107" dependencies = [  "serde",  "serde_json",
rust/Cargo.toml+1−1
@@ -3,7 +3,7 @@ resolver = "2" members = ["crates/*"]  [workspace.package]-version = "1.0.106"+version = "1.0.107" edition = "2021" license = "MIT" publish = false
rust/crates/kicad-bridge/src/main.rs+3−2
@@ -76,7 +76,8 @@ pub fn caller_side(code: &str) -> bool {         "unknown_action", "wrong_file_type", "file_exists", "footprint_exists", "ambiguous", "ambiguous_board", "already_there", "already_flagged",         "needs_closed_board", "documents_in_use", "text_off_board", "live_required", "board_open", "kicad_running", "project_open", "too_close", "same_point", "zero_length", "plan_invalid",         "invalid_path", "invalid_scope", "name_required", "engine_required", "foreground_required", "nothing_to_route", "nothing_in_box",-        "nothing_placed", "side_change_not_supported", "live_move_drops_units", "entry_off_bus", "bad_via_geometry"];+        "nothing_placed", "side_change_not_supported", "live_move_drops_units", "entry_off_bus", "bad_via_geometry",+        "unknown_argument", "invalid_arguments"];     EXACT.contains(&code) || code.starts_with("missing_") || code.starts_with("no_such_") || code.ends_with("_not_found")         || code.ends_with("_not_open") || code.ends_with("_required") }@@ -282,7 +283,7 @@ fn ngspice_worker() -> i32 { mod tests {     #[test]     fn a_callers_own_mistake_is_not_reported_as_a_bridge_bug() {-        for c in ["bad_args", "missing_arg", "no_such_probe", "footprint_not_found", "pcb_editor_not_open", "reason_required", "drc_rejected", "no_model"] {+        for c in ["bad_args", "unknown_argument", "missing_arg", "no_such_probe", "footprint_not_found", "pcb_editor_not_open", "reason_required", "drc_rejected", "no_model"] {             assert!(super::caller_side(c), "{c}");         }         for c in ["internal", "handler_panicked", "write_failed", "simulation_failed", "worker_failed", "bad_layout", "ipc_unavailable"] {
rust/crates/kicad-bridge/src/verbs_board.rs+4−2
@@ -776,7 +776,8 @@ fn align(state: &mut State, args: &Value) -> Value {                     Ok(m) => m,                     Err(e) => return routing_fail(&e),                 };-                let mut a = args.clone();+                // only the move's own arguments go on: align's mode/by/to would be unknown to it (wiki PR #70)+                let mut a = kicad_core::placement::move_args_only(args);                 a["refs"] = json!(moves);                 a.as_object_mut().map(|o| o.remove("ref"));                 let r = ipc::with_standard_revision(&c, &a, |x| ipc::move_footprint(&c, x));@@ -810,8 +811,9 @@ fn align(state: &mut State, args: &Value) -> Value {         v["mutated"] = json!(false);         return v;     }-    let mut a = args.clone();+    let mut a = kicad_core::placement::move_args_only(args);     a["refs"] = json!(moves);+    a.as_object_mut().map(|o| o.remove("ref"));     match kicad_core::placement::plan_moves(&loaded.board, &a) {         Ok(plan) => {             let mut v = json!({"success": true, "dryRun": true, "mutated": false, "source": "file", "boardPath": norm(&loaded.path), "revision": kicad_core::placement::file_revision(&loaded.text), "align": report,
rust/crates/kicad-bridge/src/verbs_placement.rs+1−1
@@ -28,7 +28,7 @@ pub static VERBS: &[Verb] = &[         mechanism: Mechanism::Ipc, risk: "write", timeout_sec: 130,         input: "{\"filePath\", \"expectedRevision\", \"ref\": \"U1\", \"x\": 120.5, \"y\": 91, \"rotation\"?: 90, \"side\"?: \"F.Cu\" | \"B.Cu\" (must equal the current side for now) | \"refs\": [{\"ref\",\"x\",\"y\",\"rotation\"?,\"side\"?}, ...], \"dryRun\"?: false, \"allowOverlap\"?: false, \"allowOutside\"?: false, \"allowLocked\"?: false, \"save\"?: false} frame?: \"standard\" takes y-up coordinates as kicad_routing_state {model:standard} reports them",         example: "kicad_move_footprint {\"filePath\":\"C:/p/b.kicad_pcb\",\"expectedRevision\":\"<from placement_state>\",\"ref\":\"C12\",\"x\":132.4,\"y\":88.1,\"rotation\":90}",-        hint: "Pass filePath, expectedRevision from placement_state, ref with x and y in mm (rotation in degrees), or refs for a batch that lands as one Undo step. Refuses stale_board, courtyard_overlap (names the offending refs), outside_outline and side_change_not_supported with nothing written; allowOverlap / allowOutside override on purpose. dryRun evaluates the checks and reports without editing. KiCad replaces the whole footprint on an IPC update and takes every child at the absolute coordinates in the proto, so the bridge transforms every pad, field, shape, text, zone and dimension of the proto client-side before sending it. The response carries before and after poses, the courtyard check, undoSteps:1, the new revision and verified, which is true only when the read-back courtyard (or first pad) moved with the anchor; after[].children carries the numbers.",+        hint: "Pass filePath, expectedRevision from placement_state, ref with x and y in mm (rotation in degrees), or refs for a batch that lands as one Undo step. Refuses stale_board, courtyard_overlap (names the offending refs), outside_outline, side_change_not_supported and unknown_argument (a key the move does not read, such as rot: suggestedKey names the right one, allowedKeys lists them) with nothing written; allowOverlap / allowOutside override on purpose. dryRun evaluates the checks and reports without editing. KiCad replaces the whole footprint on an IPC update and takes every child at the absolute coordinates in the proto, so the bridge transforms every pad, field, shape, text, zone and dimension of the proto client-side before sending it. The response carries before and after poses, the courtyard check, undoSteps:1, the new revision and verified, which is true only when the read-back courtyard (or first pad) moved with the anchor; after[].children carries the numbers.",         related: &["kicad_placement_state", "kicad_placement_validate", "kicad_route_net"],         pitfalls: &["Needs KiCad 10.0.1+ with the IPC API server on and the board open in the PCB editor; dryRun alone works from the file.", "A locked footprint is refused (footprint_locked) unless allowLocked:true.", "Do not blindly retry a timed-out move (mutation_outcome_unknown, ipc_timeout); read placement_state first.", "Moving a footprint leaves its copper where it was; place before routing.", "side changes are refused for now (side_change_not_supported): a flip mirrors every child and swaps its layers, which is separate work. Flip in the PCB editor (F), read placement_state again, then move with the same side or no side.", "The proto children are transformed client-side because KiCad replaces the footprint on update (FOOTPRINT::Deserialize keeps the children's absolute coordinates); a child type without a transform rule refuses the move (unsupported_child_item names the type URL), and a text box or rounded courtyard rectangle only turns by multiples of 90 degrees.", "verified:false with postCommitError 'children did not move' means the anchor moved and the body did not; undo in KiCad and report it, do not move again."],     },
rust/crates/kicad-core/src/placement.rs+61−10
@@ -566,6 +566,32 @@ fn normalize_deg(d: f64) -> f64 {     } } +/// The keys `kicad_move_footprint` reads at the top level (wiki PR #70).+pub const MOVE_KEYS: &[&str] = &[+    "filePath", "socketPath", "expectedRevision", "ref", "x", "y",+    "rotation", "side", "refs", "dryRun", "allowOverlap",+    "allowOutside", "allowLocked", "save", "frame", "ipcTimeoutMs",+];+/// The keys of one `refs[]` entry.+pub const MOVE_ENTRY_KEYS: &[&str] = &["ref", "x", "y", "rotation", "side"];+/// Keys any verb may carry that are not about the move: the asker's reason and identity, which+/// Adom Bridge and AI clients put in the arguments of every call. Keys starting with `_` are the+/// bridge's own notes (`_fromFrame` after a `frame:"standard"` call), also accepted.+const CALL_KEYS: &[&str] = &["reason", "caller", "aiThread"];++fn move_key_allowed(key: &str) -> bool {+    MOVE_KEYS.contains(&key) || CALL_KEYS.contains(&key) || key.starts_with('_')+}++/// Only the arguments a move reads (plus the call's reason and the bridge's `_` notes), for a+/// verb that hands its own arguments on to `kicad_move_footprint` (`kicad_align_parts`).+pub fn move_args_only(args: &Value) -> Value {+    match args.as_object() {+        Some(o) => Value::Object(o.iter().filter(|(k, _)| move_key_allowed(k)).map(|(k, v)| (k.clone(), v.clone())).collect()),+        None => args.clone(),+    }+}+ /// Validate the *whole* move payload before resolving geometry or opening the live /// editor. JSON getters otherwise silently treat typos such as `rot` as an omitted /// `rotation`, which can move copper-connected parts without rotating their pads.@@ -573,19 +599,14 @@ fn normalize_deg(d: f64) -> f64 { /// Includes the standard board-frame and IPC fields. Batch entries have their /// own much smaller schema so typos inside `refs` fail the same way. pub fn validate_move_arguments(args: &Value) -> Result<(), RoutingError> {-    const ROOT: &[&str] = &[-        "filePath", "socketPath", "expectedRevision", "ref", "x", "y",-        "rotation", "side", "refs", "dryRun", "allowOverlap",-        "allowOutside", "allowLocked", "save", "frame",-    ];-    const ENTRY: &[&str] = &["ref", "x", "y", "rotation", "side"];-    fn check(value: &Value, allowed: &[&str], context: &str) -> Result<(), RoutingError> {+    fn check(value: &Value, allowed: &[&str], top: bool, context: &str) -> Result<(), RoutingError> {         let map = value.as_object().ok_or_else(|| {             RoutingError::new("invalid_arguments", format!("{context} must be a JSON object"))                 .with("mutated", json!(false))         })?;         for key in map.keys() {-            if !allowed.contains(&key.as_str()) {+            let ok = if top { move_key_allowed(key) } else { allowed.contains(&key.as_str()) };+            if !ok {                 let suggested = if key == "rot" || key == "angle" {                     Some("rotation")                 } else {@@ -610,10 +631,10 @@ pub fn validate_move_arguments(args: &Value) -> Result<(), RoutingError> {         }         Ok(())     }-    check(args, ROOT, "move")?;+    check(args, MOVE_KEYS, true, "move")?;     if let Some(Value::Array(entries)) = args.get("refs") {         for (index, entry) in entries.iter().enumerate() {-            check(entry, ENTRY, &format!("refs[{index}]"))?;+            check(entry, MOVE_ENTRY_KEYS, false, &format!("refs[{index}]"))?;         }     }     Ok(())@@ -1386,6 +1407,36 @@ mod tests {         assert_eq!(batch.moves[0].after.rotation, -90.0);     } +    #[test]+    fn a_real_call_with_its_reason_caller_and_frame_note_still_moves() {+        // What reaches plan_moves from Adom Bridge: the reason and caller ride in the arguments,+        // frame:"standard" has become _fromFrame, and helpers pass ipcTimeoutMs.+        let b = board(&[fp("Q2", 110.0, 110.0, 0.0, "F.Cu", RECT, "")]);+        let call = json!({"filePath": "C:/p/b.kicad_pcb", "ref": "Q2", "x": 120, "y": -110, "rotation": 90, "frame": "standard",+            "dryRun": true, "reason": "move Q2 beside the connector", "caller": {"thread": "t"}, "aiThread": "t", "ipcTimeoutMs": 20000});+        let native = crate::board_model::args_to_kicad_frame(&call).unwrap();+        assert_eq!(native["_fromFrame"], json!("standard"));+        let plan = plan_moves(&b, &native).unwrap();+        assert_eq!((plan.moves[0].after.x, plan.moves[0].after.y, plan.moves[0].after.rotation), (120.0, 110.0, 90.0));+        // a misspelling is still caught beside them, and a _ key inside a refs entry is not a bridge note+        let e = plan_moves(&b, &json!({"ref": "Q2", "x": 120, "y": 110, "reason": "r", "_fromFrame": "standard", "rot": 90})).unwrap_err();+        assert_eq!((e.code.as_str(), e.detail["suggestedKey"].clone()), ("unknown_argument", json!("rotation")));+        let e = plan_moves(&b, &json!({"refs": [{"ref": "Q2", "x": 120, "y": 110, "_x": 1}]})).unwrap_err();+        assert_eq!(e.code, "unknown_argument");+        // kicad_align_parts hands its own arguments on: only the move's survive+        let align = json!({"filePath": "C:/p/b.kicad_pcb", "refs": [{"ref": "Q2", "x": 120, "y": 110}], "mode": "top", "by": "anchor", "to": 4,+            "spacing": 2, "source": "live", "dryRun": true, "reason": "line them up", "_fromFrame": "standard"});+        let kept = move_args_only(&align);+        for k in ["mode", "by", "to", "spacing", "source"] {+            assert!(kept.get(k).is_none(), "{k}");+        }+        for k in ["filePath", "refs", "dryRun", "reason", "_fromFrame"] {+            assert!(kept.get(k).is_some(), "{k}");+        }+        assert!(plan_moves(&b, &kept).is_ok());+        assert_eq!(plan_moves(&b, &align).unwrap_err().code, "unknown_argument");+    }+     #[test]     fn courtyard_box_follows_rotation_and_falls_back_to_pads() {         let b = board(&[fp("U1", 110.0, 110.0, 0.0, "F.Cu", RECT, ""), fp("U2", 120.0, 110.0, 90.0, "F.Cu", RECT, ""), fp("R1", 130.0, 110.0, 0.0, "F.Cu", "", "")]);