fix: rotate_schematic_component uses sexpdata API and drags wires

Previously the handler used kicad-skip to apply rotation and mirror.
kicad-skip has no API for (mirror x/y) on placed symbols, causing:
  'NoneType' object has no attribute 'value'

Fix:
- Rewrote _handle_rotate_schematic_component to use sexpdata (same
  approach as move_schematic_component) for both rotation and mirror
- Added WireDragger.compute_pin_positions_for_rotation: computes old
  and new pin world positions when rotation/mirror changes at fixed (x,y)
- Added WireDragger.update_symbol_rotation_mirror: updates (at) rotation
  and adds/removes/replaces the (mirror x/y) sexpdata token cleanly
- Connected wires now follow pin positions after rotate/mirror via the
  existing WireDragger.drag_wires infrastructure

Tests: 10 unit tests in tests/test_rotate_schematic_mirror.py covering
update_symbol_rotation_mirror, compute_pin_positions_for_rotation, and
a handler smoke test.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

python/commands/wire_dragger.py

@@ -197,6 +197,81 @@ class WireDragger:
             )
         return result
 
+    @staticmethod
+    def compute_pin_positions_for_rotation(
+        sch_data: list,
+        reference: str,
+        new_rotation: float,
+        new_mirror_x: bool,
+        new_mirror_y: bool,
+    ) -> Dict[str, Tuple[Tuple[float, float], Tuple[float, float]]]:
+        """
+        Compute world pin positions before and after a rotation/mirror change.
+
+        The symbol stays at the same (x, y); only the rotation and mirror state change.
+        Returns {pin_num: (old_world_xy, new_world_xy)}.
+        """
+        found = WireDragger.find_symbol(sch_data, reference)
+        if found is None:
+            return {}
+        _, sym_x, sym_y, old_rotation, lib_id, old_mirror_x, old_mirror_y = found
+
+        pins = WireDragger.get_pin_defs(sch_data, lib_id)
+        result: Dict[str, Tuple] = {}
+        for pin_num, pin in pins.items():
+            px, py = pin["x"], pin["y"]
+            old_wx, old_wy = WireDragger.pin_world_xy(
+                px, py, sym_x, sym_y, old_rotation, old_mirror_x, old_mirror_y
+            )
+            new_wx, new_wy = WireDragger.pin_world_xy(
+                px, py, sym_x, sym_y, new_rotation, new_mirror_x, new_mirror_y
+            )
+            result[pin_num] = (
+                (round(old_wx, 6), round(old_wy, 6)),
+                (round(new_wx, 6), round(new_wy, 6)),
+            )
+        return result
+
+    @staticmethod
+    def update_symbol_rotation_mirror(
+        sch_data: list,
+        reference: str,
+        new_rotation: float,
+        new_mirror: Optional[str],
+    ) -> bool:
+        """
+        Update the rotation in (at x y rot) and the (mirror x/y) token for a symbol.
+
+        new_mirror: "x", "y", or None (removes any existing mirror token).
+        Returns True if the symbol was found and updated.
+        """
+        found = WireDragger.find_symbol(sch_data, reference)
+        if found is None:
+            return False
+        item = found[0]
+        at_k = _K["at"]
+        mirror_k = _K["mirror"]
+
+        # Update rotation in (at x y rot)
+        for sub in item[1:]:
+            if isinstance(sub, list) and sub and sub[0] == at_k and len(sub) >= 4:
+                sub[3] = new_rotation
+                break
+
+        # Remove existing (mirror ...) token(s)
+        to_remove = [
+            i for i, sub in enumerate(item)
+            if isinstance(sub, list) and sub and sub[0] == mirror_k
+        ]
+        for i in reversed(to_remove):
+            del item[i]
+
+        # Insert new mirror token if requested
+        if new_mirror in ("x", "y"):
+            item.append([mirror_k, Symbol(new_mirror)])
+
+        return True
+
     @staticmethod
     def drag_wires(
         sch_data: list,

python/kicad_interface.py

@@ -2710,13 +2710,16 @@ class KiCADInterface:
             return {"success": False, "message": str(e)}
 
     def _handle_rotate_schematic_component(self, params: Dict[str, Any]) -> Dict[str, Any]:
-        """Rotate a schematic component"""
+        """Rotate and/or mirror a schematic component, dragging connected wires."""
         logger.info("Rotating schematic component")
         try:
+            import sexpdata as _sexpdata
+            from commands.wire_dragger import WireDragger
+
             schematic_path = params.get("schematicPath")
             reference = params.get("reference")
             angle = params.get("angle", 0)
-            mirror = params.get("mirror")
+            mirror = params.get("mirror")  # "x", "y", or None
 
             if not schematic_path or not reference:
                 return {
@@ -2724,77 +2727,61 @@ class KiCADInterface:
                     "message": "schematicPath and reference are required",
                 }
 
-            sym_k = sexpdata.Symbol("symbol")
+            with open(schematic_path, "r", encoding="utf-8") as f:
-            prop_k = sexpdata.Symbol("property")
+                sch_data = _sexpdata.loads(f.read())
-            at_k = sexpdata.Symbol("at")
-            mirror_k = sexpdata.Symbol("mirror")
 
-            with open(schematic_path, "r", encoding="utf-8") as _f:
+            found = WireDragger.find_symbol(sch_data, reference)
-                sch_data = sexpdata.load(_f)
+            if found is None:
-
-            target = None
-            for item in sch_data:
-                if not (isinstance(item, list) and item and item[0] == sym_k):
-                    continue
-                ref_val = None
-                for sub in item[1:]:
-                    if (
-                        isinstance(sub, list)
-                        and len(sub) >= 3
-                        and sub[0] == prop_k
-                        and str(sub[1]).strip('"') == "Reference"
-                    ):
-                        ref_val = str(sub[2]).strip('"')
-                        break
-                if ref_val == reference:
-                    target = item
-                    break
-
-            if target is None:
                 return {"success": False, "message": f"Component {reference} not found"}
 
-            # Update (at x y rot)
+            # Determine new mirror state: explicit param overrides; None preserves existing
-            at_node = None
+            _, _, _, _, _, old_mirror_x, old_mirror_y = found
-            mirror_idx = None
+            if mirror is None:
-            for idx, sub in enumerate(target[1:], start=1):
+                new_mirror_x = old_mirror_x
-                if not isinstance(sub, list) or not sub:
+                new_mirror_y = old_mirror_y
-                    continue
+                effective_mirror = "x" if old_mirror_x else ("y" if old_mirror_y else None)
-                if sub[0] == at_k:
-                    at_node = sub
-                elif sub[0] == mirror_k:
-                    mirror_idx = idx
-
-            if at_node is None:
-                return {
-                    "success": False,
-                    "message": f"Component {reference} has no (at ...) node",
-                }
-            while len(at_node) < 3:
-                at_node.append(0)
-            if len(at_node) < 4:
-                at_node.append(angle)
             else:
-                at_node[3] = angle
+                new_mirror_x = (mirror == "x")
+                new_mirror_y = (mirror == "y")
+                effective_mirror = mirror
 
-            if mirror:
+            # Compute pin world positions before and after the transform
-                mirror_node = [mirror_k, sexpdata.Symbol(str(mirror))]
+            pin_positions = WireDragger.compute_pin_positions_for_rotation(
-                if mirror_idx is not None:
+                sch_data, reference, float(angle), new_mirror_x, new_mirror_y
-                    target[mirror_idx] = mirror_node
+            )
-                else:
+
-                    # Insert after (at ...) for stability
+            # Build old→new map (skip pins that don't move)
-                    insert_at = len(target)
+            old_to_new = {}
-                    for idx, sub in enumerate(target[1:], start=1):
+            for _pin, (old_xy, new_xy) in pin_positions.items():
-                        if isinstance(sub, list) and sub and sub[0] == at_k:
+                if old_xy == new_xy:
-                            insert_at = idx + 1
+                    continue
-                            break
+                if old_xy in old_to_new:
-                    target.insert(insert_at, mirror_node)
+                    logger.warning(
+                        f"rotate: pin {_pin!r} of {reference!r} shares old position "
+                        f"{old_xy} with another pin; skipping duplicate"
+                    )
+                    continue
+                old_to_new[old_xy] = new_xy
+
+            # Drag connected wires to follow pins
+            drag_summary = WireDragger.drag_wires(sch_data, old_to_new)
+
+            # Update the symbol's rotation and mirror token in sexpdata
+            WireDragger.update_symbol_rotation_mirror(sch_data, reference, float(angle), effective_mirror)
 
             WireManager.sync_junctions(sch_data)
 
-            with open(schematic_path, "w", encoding="utf-8") as _f:
+            with open(schematic_path, "w", encoding="utf-8") as f:
-                _f.write(sexpdata.dumps(sch_data))
+                f.write(_sexpdata.dumps(sch_data))
 
-            return {"success": True, "reference": reference, "angle": angle}
+            return {
+                "success": True,
+                "reference": reference,
+                "angle": angle,
+                "mirror": effective_mirror,
+                "wiresMoved": drag_summary.get("endpoints_moved", 0),
+                "wiresRemoved": drag_summary.get("wires_removed", 0),
+            }
 
         except Exception as e:
             logger.error(f"Error rotating schematic component: {e}")

tests/test_rotate_schematic_mirror.py

@@ -0,0 +1,242 @@
+"""
+Tests for rotate_schematic_component mirror/rotation fix.
+
+Tests are split into two layers:
+ 1. WireDragger unit tests — pure sexpdata logic, no KiCAD deps.
+ 2. Handler integration smoke test — patches SchematicManager away.
+"""
+
+import os
+import sys
+import math
+import textwrap
+import tempfile
+import importlib.util
+from unittest.mock import patch, MagicMock
+
+import sexpdata
+from sexpdata import Symbol
+
+# ---------------------------------------------------------------------------
+# Import WireDragger directly (no pcbnew / kicad_interface needed)
+# ---------------------------------------------------------------------------
+_wd_spec = importlib.util.spec_from_file_location(
+    "wire_dragger",
+    os.path.join(os.path.dirname(__file__), "..", "python", "commands", "wire_dragger.py"),
+)
+_wd_mod = importlib.util.module_from_spec(_wd_spec)
+
+# wire_dragger imports pin_locator lazily inside get_pin_defs.
+# We stub only the submodule, not the parent package, so that
+# kicad_interface can still import commands.board etc. from disk.
+_pin_locator_mock = MagicMock()
+sys.modules.setdefault("commands.pin_locator", _pin_locator_mock)
+
+_wd_spec.loader.exec_module(_wd_mod)
+WireDragger = _wd_mod.WireDragger
+
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+def _parse(text: str) -> list:
+    return sexpdata.loads(text)
+
+
+def _dump(data: list) -> str:
+    return sexpdata.dumps(data)
+
+
+def _make_sch(sym_extra: str = "", wires: str = "") -> list:
+    """Build a minimal schematic sexpdata with one Q1 symbol."""
+    text = textwrap.dedent(f"""\
+        (kicad_sch (version 20250114) (generator "test")
+          (lib_symbols
+            (symbol "Transistor_BJT:MMBT3904"
+              (pin passive line (at 0.0 1.0 270) (length 1.27)
+                (name "B" (effects (font (size 1.27 1.27))))
+                (number "1" (effects (font (size 1.27 1.27))))
+              )
+              (pin passive line (at -1.0 0.0 0) (length 1.27)
+                (name "C" (effects (font (size 1.27 1.27))))
+                (number "2" (effects (font (size 1.27 1.27))))
+              )
+            )
+          )
+          (symbol (lib_id "Transistor_BJT:MMBT3904")
+                  (at 75 105 0)
+                  {sym_extra}
+                  (property "Reference" "Q1" (at 75 105 0))
+                  (property "Value" "MMBT3904" (at 75 105 0))
+          )
+          {wires}
+        )
+    """)
+    return _parse(text)
+
+
+# ---------------------------------------------------------------------------
+# Tests: update_symbol_rotation_mirror
+# ---------------------------------------------------------------------------
+
+def test_update_rotation_sets_angle():
+    sch = _make_sch()
+    result = WireDragger.update_symbol_rotation_mirror(sch, "Q1", 90.0, None)
+    assert result is True
+    dumped = _dump(sch)
+    # at should now have 90 as the rotation value
+    assert "90" in dumped
+
+
+def test_update_mirror_x_adds_token():
+    sch = _make_sch()
+    WireDragger.update_symbol_rotation_mirror(sch, "Q1", 0.0, "x")
+    dumped = _dump(sch)
+    assert "mirror" in dumped
+    assert " x" in dumped or "(mirror x)" in dumped
+
+
+def test_update_mirror_y_adds_token():
+    sch = _make_sch()
+    WireDragger.update_symbol_rotation_mirror(sch, "Q1", 0.0, "y")
+    dumped = _dump(sch)
+    assert "mirror" in dumped
+
+
+def test_update_mirror_none_removes_existing():
+    """mirror=None should remove a pre-existing (mirror x) token."""
+    sch = _make_sch(sym_extra="(mirror x)")
+    WireDragger.update_symbol_rotation_mirror(sch, "Q1", 0.0, None)
+    dumped = _dump(sch)
+    assert "mirror" not in dumped
+
+
+def test_update_mirror_replaces_existing():
+    """Setting mirror='y' when (mirror x) exists should replace, not duplicate."""
+    sch = _make_sch(sym_extra="(mirror x)")
+    WireDragger.update_symbol_rotation_mirror(sch, "Q1", 0.0, "y")
+    dumped = _dump(sch)
+    assert dumped.count("mirror") == 1
+
+
+def test_update_unknown_reference_returns_false():
+    sch = _make_sch()
+    result = WireDragger.update_symbol_rotation_mirror(sch, "U99", 0.0, "x")
+    assert result is False
+
+
+# ---------------------------------------------------------------------------
+# Tests: compute_pin_positions_for_rotation
+# ---------------------------------------------------------------------------
+
+def test_pin_positions_change_on_rotation():
+    """Pins at non-zero local offsets should move when the symbol rotates."""
+    sch = _make_sch()
+
+    # Provide a real pin_defs via patch so we don't need KiCAD libs
+    fake_pins = {
+        "1": {"x": 0.0, "y": 1.0},
+        "2": {"x": -1.0, "y": 0.0},
+    }
+    with patch.object(WireDragger, "get_pin_defs", return_value=fake_pins):
+        pos = WireDragger.compute_pin_positions_for_rotation(sch, "Q1", 90.0, False, False)
+
+    assert len(pos) == 2
+    for pin_num, (old_xy, new_xy) in pos.items():
+        # After 90° rotation the positions must differ (pins not at origin)
+        assert old_xy != new_xy, f"Pin {pin_num} should have moved"
+
+
+def test_pin_positions_unchanged_at_same_transform():
+    """Same rotation and same mirror → no movement."""
+    sch = _make_sch()  # symbol at rotation=0, no mirror
+
+    fake_pins = {"1": {"x": 1.0, "y": 0.0}}
+    with patch.object(WireDragger, "get_pin_defs", return_value=fake_pins):
+        pos = WireDragger.compute_pin_positions_for_rotation(sch, "Q1", 0.0, False, False)
+
+    for _, (old_xy, new_xy) in pos.items():
+        assert old_xy == new_xy
+
+
+def test_pin_positions_mirror_x_flips_x():
+    """mirror_x should negate the local X coordinate before rotation."""
+    sch = _make_sch()  # at (75, 105, 0), no mirror
+
+    fake_pins = {"1": {"x": 2.0, "y": 0.0}}
+    with patch.object(WireDragger, "get_pin_defs", return_value=fake_pins):
+        pos = WireDragger.compute_pin_positions_for_rotation(sch, "Q1", 0.0, True, False)
+
+    _, (old_xy, new_xy) = next(iter(pos.items()))
+    # old: pin at local (2, 0), world = (75+2, 105) = (77, 105)
+    assert abs(old_xy[0] - 77.0) < 1e-4
+    # new: mirror_x → local (-2, 0), world = (75-2, 105) = (73, 105)
+    assert abs(new_xy[0] - 73.0) < 1e-4
+
+
+# ---------------------------------------------------------------------------
+# Integration smoke test: handler uses sexpdata, not kicad-skip
+# ---------------------------------------------------------------------------
+
+def test_rotate_handler_no_crash(tmp_path):
+    """_handle_rotate_schematic_component should succeed without kicad-skip."""
+    # Stub heavy imports before loading kicad_interface
+    for modname in ("pcbnew", "skip", "resources", "schemas",
+                    "resources.resource_definitions", "schemas.tool_schemas"):
+        sys.modules.setdefault(modname, MagicMock())
+    sys.modules["resources.resource_definitions"].RESOURCE_DEFINITIONS = {}
+    sys.modules["resources.resource_definitions"].handle_resource_read = MagicMock()
+    sys.modules["schemas.tool_schemas"].TOOL_SCHEMAS = []
+
+    _pcbnew = sys.modules["pcbnew"]
+    _pcbnew.__file__ = "/fake/pcbnew.so"
+    _pcbnew.GetBuildVersion.return_value = "9.0.0"
+
+    ki_spec = importlib.util.spec_from_file_location(
+        "kicad_interface_smoke",
+        os.path.join(os.path.dirname(__file__), "..", "python", "kicad_interface.py"),
+    )
+    ki_mod = importlib.util.module_from_spec(ki_spec)
+    ki_spec.loader.exec_module(ki_mod)
+    KiCADInterface = ki_mod.KiCADInterface
+
+    # Write a minimal schematic file
+    sch_path = str(tmp_path / "test.kicad_sch")
+    sch_content = textwrap.dedent("""\
+        (kicad_sch (version 20250114) (generator "test")
+          (lib_symbols
+            (symbol "Device:R"
+              (pin passive line (at 0 1.016 270) (length 1.27)
+                (name "~" (effects (font (size 1.27 1.27))))
+                (number "1" (effects (font (size 1.27 1.27))))
+              )
+              (pin passive line (at 0 -1.016 90) (length 1.27)
+                (name "~" (effects (font (size 1.27 1.27))))
+                (number "2" (effects (font (size 1.27 1.27))))
+              )
+            )
+          )
+          (symbol (lib_id "Device:R") (at 100 100 0)
+            (property "Reference" "R1" (at 100 100 0))
+            (property "Value" "10k" (at 100 100 0))
+          )
+        )
+    """)
+    with open(sch_path, "w") as f:
+        f.write(sch_content)
+
+    iface = KiCADInterface.__new__(KiCADInterface)
+    result = iface._handle_rotate_schematic_component({
+        "schematicPath": sch_path,
+        "reference": "R1",
+        "angle": 90,
+    })
+
+    assert result["success"] is True
+    assert result["angle"] == 90
+
+    # Verify the file was actually updated
+    with open(sch_path) as f:
+        updated = f.read()
+    assert "90" in updated