app

KiCad - the KiCad Bridge

Public Made by Adom

by adom

Drive your own KiCad from an AI, in the background: open every editor, read the whole board as data (the EDA Bridge Standard board model), stack-up, part heights, netlists, place, route, pour, DRC and exports.

kicadpcbschematicedabridgeabadom-bridgedesktop-app
Overview Files Skills Discover Packages Releases Dependencies Issues1 Breadcrumbs Pull Requests1 Activity
← All Pull Requests

Reject unknown kicad_move_footprint arguments before native edits #70

Merged opened by Josh Aberdeen 2026-11-07

Reject unknown placement arguments before KiCad mutations

Defect

kicad_move_footprint accepts a JSON payload with the undocumented field rot, reports successful dry-run and live move, but silently ignores that field. The requested x/y change happens without the intended rotation. A subsequent command using the documented rotation field correctly rotates the footprint.

Root cause is in the shared Rust placement planner: plan_moves() reads item.get("rotation") and defaults to the existing angle when that field is absent, but it never rejects other argument names. A typo becomes a valid no-rotation request and the later native readback validates the wrong intended pose.

Change

  • Add shared validate_move_arguments() to kicad-core::placement, applying before all geometry checks in plan_moves().
  • Apply the same preflight at the start of live ipc::move_footprint(), before connecting to or modifying KiCad.
  • Reject unexpected top-level fields and unknown keys in each refs[] entry. Return structured unknown_argument with the invalid key, allowed keys, mutated:false and a suggestedKey when available (rot → rotation).
  • Preserve valid standard fields including frame:"kicad" / "standard", socketPath, expected revision, save and geometry safety options; no alias that silently changes the command.
  • Cover single and batched move requests in unit tests. Confirm valid rotation still produces the expected pose.

Reproduction (generic synthetic fixture)

  1. Create a test board with one footprint, rotation 0°.
  2. Call a dry run with {ref:"Q2", x:120, y:110, rot:270, dryRun:true, frame:"kicad"}.
  3. Previously the bridge returned success. A real move changed x/y but left rotation unchanged.
  4. After this patch, both dry run and live move reject rot before any board write, advising rotation.
  5. The canonical rotation:180 and batch refs:[{...,rotation:270}] remain supported.

Safety and scope

This is deliberately a small, stage-independent Bridge fix. It improves the contract for any agent calling live KiCad placement. It doesn't alter routing algorithms, skip DRC, weaken electrical limits, or rework a private board. No private customer board files or customer data are attached.

The source also contains an existing post-commit pose verifier, but that verifier cannot detect a misspelled request field after the planner already defaulted rotation to its old value. Rejecting the field at the boundary is necessary.

Tests

  • New move_arguments_reject_unknown_rotation_without_touching_placement: single dry/commit, batch typo and other unknown field.
  • New move_arguments_keep_documented_options_and_rotation: valid frame/revision/IPC options and single/batch requested angle.
  • Existing footprint positioning and KiCad core tests should continue to pass.
  • Local source candidate built from public kicad-bridge 1.0.105; no replacement of production executable.

Test evidence and result: see the validation results in the PR discussion / attached audit; the patch is proposed for maintainers' independent CI and live-editor verification.

Regression acceptance

  1. Invalid JSON arguments rejected before contacting KiCad; mutated:false, deterministic suggestion and full supported-key list.
  2. Same behavior for dry-run and commit, one- and multi-footprint calls.
  3. Valid standard coordinate frame and angles remain accepted.
  4. Rejected calls leave the authoritative board hash and revision unchanged.
  5. Existing post-commit requested-pose and child geometry verification remains intact.

Related but separate workflow lifecycle issue: AI Flow #171, where an engineering agent completed only its plan and became idle before the requested board edit. This PR does not claim to fix the agent lifecycle.

Diff Skip to comments (1)

--- a/rust/crates/kicad-core/src/placement.rs+++ b/rust/crates/kicad-core/src/placement.rs@@ -1,1850 +1,1942 @@⋯ 565 unchanged lines ⋯     } } +/// 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.+///+/// 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> {+        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 suggested = if key == "rot" || key == "angle" {+                    Some("rotation")+                } else {+                    allowed.iter().find(|candidate| {+                        candidate.to_lowercase().starts_with(&key.to_lowercase())+                            || key.to_lowercase().starts_with(&candidate.to_lowercase())+                    }).copied()+                };+                let explanation = match suggested {+                    Some(want) => format!("Unknown {context} argument '{key}'; did you mean '{want}'?"),+                    None => format!("Unknown {context} argument '{key}'"),+                };+                let mut err = RoutingError::new("unknown_argument", explanation)+                    .with("argument", json!(key))+                    .with("allowedKeys", json!(allowed))+                    .with("mutated", json!(false));+                if let Some(want) = suggested {+                    err = err.with("suggestedKey", json!(want));+                }+                return Err(err);+            }+        }+        Ok(())+    }+    check(args, ROOT, "move")?;+    if let Some(Value::Array(entries)) = args.get("refs") {+        for (index, entry) in entries.iter().enumerate() {+            check(entry, ENTRY, &format!("refs[{index}]"))?;+        }+    }+    Ok(())+}+ /// `kicad_move_footprint`'s argument reading and checks. `ref` with `x`, `y`, `rotation?`, /// `side?`, or `refs: [{ref, x, y, rotation?, side?}, ...]` (1..256). Refuses an unknown /// or locked footprint, a side change (`side_change_not_supported`: a flip is separate⋯ 1 unchanged line ⋯ /// courtyard outside the outline (`outside_outline`, unless `allowOutside`). Nothing is /// written here; the caller commits `plan.moves` as one undo step. pub fn plan_moves(board: &Board, args: &Value) -> Result<MovePlan, RoutingError> {+    validate_move_arguments(args)?;     let items: Vec<Value> = match args.get("refs") {         Some(Value::Array(a)) if !a.is_empty() && a.len() <= MAX_MOVES => a.clone(),         Some(_) => return Err(RoutingError::new("invalid_refs", format!("refs must list 1..{MAX_MOVES} moves, each {{ref, x, y, rotation?, side?}}"))),⋯ 717 unchanged lines ⋯     }      #[test]+    fn move_arguments_reject_unknown_rotation_without_touching_placement() {+        let b = board(&[fp("Q2", 110.0, 110.0, 0.0, "F.Cu", RECT, "")]);+        for bad in [+            json!({"ref":"Q2","x":120,"y":110,"rot":270,"dryRun":true,"frame":"kicad"}),+            json!({"ref":"Q2","x":120,"y":110,"rot":270,"dryRun":false,"frame":"kicad"}),+            json!({"refs":[{"ref":"Q2","x":120,"y":110,"rot":270}],"dryRun":true}),+            json!({"refs":[{"ref":"Q2","x":120,"y":110,"rotation":180,"rott":90}]}),+            json!({"ref":"Q2","x":120,"y":110,"frame":"kicad","unexpected":true}),+        ] {+            let e = plan_moves(&b, &bad).unwrap_err();+            assert_eq!(e.code, "unknown_argument");+        }+        let original = placements(&b);+        assert_eq!(original[0].rotation, 0.0);+        assert_eq!(original[0].x, 110.0);+    }++    #[test]+    fn move_arguments_keep_documented_options_and_rotation() {+        let b = board(&[fp("Q2", 110.0, 110.0, 0.0, "F.Cu", RECT, "")]);+        let plan = plan_moves(&b, &json!({+            "filePath":"fixture.kicad_pcb","expectedRevision":"fixture-hash",+            "socketPath":"/tmp/ipc", "frame":"kicad",+            "ref":"Q2","x":120,"y":110,"rotation":180,+            "save":true,"dryRun":true,"allowOverlap":false,+            "allowOutside":false,"allowLocked":false+        })).unwrap();+        assert_eq!(plan.moves.len(), 1);+        assert_eq!(plan.moves[0].after.rotation, 180.0);+        let batch = plan_moves(&b, &json!({+            "refs":[{"ref":"Q2","x":120,"y":110,"rotation":270}],+            "frame":"standard","dryRun":true+        })).unwrap();+        assert_eq!(batch.moves[0].after.rotation, -90.0);+    }++    #[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", "", "")]);         let p = placements(&b);⋯ 546 unchanged lines ⋯         assert!(!outlines_overlap(&rect(0.0, 0.0, 1.0, 1.0), &rect(1.0, 0.0, 2.0, 1.0)));     } }+--- a/rust/crates/kicad-core/src/ipc.rs+++ b/rust/crates/kicad-core/src/ipc.rs@@ -1,2644 +1,2648 @@⋯ 1838 unchanged lines ⋯ /// outline) then committed through `update_items` as ONE native Undo step. `dryRun` /// runs the checks and reports without touching the editor. pub fn move_footprint(ctx: &Ctx, args: &Value) -> RResult<Value> {+    // Fail unknown/typoed fields before even contacting the editor, not just+    // before committing. The same validator runs for file-based dry runs.+    placement::validate_move_arguments(args)?;     let s = connect(ctx, args)?;     let (text_before, board, revision) = s.snapshot()?;     check_revision(args, &revision)?;⋯ 797 unchanged lines ⋯         let _ = std::fs::remove_dir_all(&base);     } }+

Comments

Colby Knox · 2d ago

Thanks, Josh: merged, and it's public in kicad-bridge 1.0.107.

I added one follow-up (#72). Calls through Adom Bridge also carry reason (and sometimes caller), a frame:"standard" call reaches the move with the bridge's internal _fromFrame, and kicad_align_parts passes its own arguments on to the move, so the original allowlist would have refused those calls. The top level now also accepts reason, caller, aiThread, ipcTimeoutMs and keys starting with _. refs[] entries keep your strict list, and rot / angle still come back as unknown_argument with suggestedKey: "rotation".

Checked on KiCad 10.0.8 on Windows: rot, angle and boardId are refused with nothing moved, a move with rotation turns the part, and kicad_align_parts still works live.

Log in to comment.