Merge pull request #124 from vdawger/fix/schematic-symbol-lookup-libname-ordering

fix: locate placed symbols when (lib_name) precedes (lib_id)

CHANGELOG.md

@@ -4,6 +4,20 @@ All notable changes to the KiCAD MCP Server project are documented here.
 
 ## [Unreleased]
 
+### Bug Fixes
+
+- **Schematic symbol lookup**: `get_schematic_component`,
+  `edit_schematic_component`, `set_schematic_component_property`,
+  `remove_schematic_component_property`, and `delete_schematic_component`
+  no longer fail with `Component '<ref>' not found in schematic` when the
+  placed symbol uses KiCad's rescued / locally-customised serialisation
+  form `(symbol (lib_name "...") (lib_id "...") ...)`. The block-matching
+  regex now accepts any opening paren after `(symbol`, and the
+  parent-position lookup uses the first `(at ...)` inside the symbol
+  block, so newly-added properties anchor to the symbol origin instead of
+  silently falling back to `(0, 0)`. Added 7 regression tests reproducing
+  the failure on a real-world user schematic.
+
 ### New MCP Tools
 
 - `set_schematic_component_property` — Add or update a single custom property

python/kicad_interface.py

@@ -15,11 +15,11 @@ import traceback
 from pathlib import Path
 from typing import Any, Dict, List, Optional, Tuple
 
+from annotations import AnnotationLoader
 from resources.resource_definitions import RESOURCE_DEFINITIONS, handle_resource_read
 
 # Import tool schemas, resource definitions, and IPC API annotations
 from schemas.tool_schemas import TOOL_SCHEMAS
-from annotations import AnnotationLoader
 
 _annotation_loader = AnnotationLoader()
 
@@ -814,7 +814,15 @@ class KiCADInterface:
             # line-by-line regex would never match.
             blocks_to_delete = []  # list of (char_start, char_end) into content
             search_start = 0
-            pattern = re.compile(r'\(symbol\s+\(lib_id\s+"')
+            # Match the opening of any placed-symbol block. KiCAD may emit the
+            # children of (symbol ...) in any order — most commonly
+            # `(symbol (lib_id "..."))`, but symbols whose library entry has been
+            # rescued / customised carry an additional `(lib_name "...")` first:
+            # `(symbol (lib_name "...") (lib_id "...") ...)`. Matching just
+            # `(symbol\s+(` covers both, and the lib_symbols range check below
+            # still excludes library-definition symbols (which use the
+            # `(symbol "name" ...)` form with a quoted string, not a paren).
+            pattern = re.compile(r"\(symbol\s+\(")
             while True:
                 m = pattern.search(content, search_start)
                 if not m:
@@ -1148,11 +1156,17 @@ class KiCADInterface:
                 self._find_matching_paren(content, lib_sym_pos) if lib_sym_pos >= 0 else -1
             )
 
-            # Find placed symbol blocks that match the reference
+            # Find placed symbol blocks that match the reference. KiCAD may
-            # Search for (symbol (lib_id "...") ... (property "Reference" "<ref>" ...) ...)
+            # serialise the children of (symbol ...) in different orders —
+            # `(symbol (lib_id "..."))` is the common case but rescued or
+            # locally-customised symbols carry an extra `(lib_name "...")`
+            # before the lib_id: `(symbol (lib_name "...") (lib_id "..."))`.
+            # Match any opening paren after `(symbol`; the lib_symbols range
+            # check below excludes library-definition symbols, which use the
+            # `(symbol "name" ...)` form (quoted string, not paren).
             block_start = block_end = None
             search_start = 0
-            pattern = re.compile(r'\(symbol\s+\(lib_id\s+"')
+            pattern = re.compile(r"\(symbol\s+\(")
             while True:
                 m = pattern.search(content, search_start)
                 if not m:
@@ -1186,8 +1200,12 @@ class KiCADInterface:
 
             # Determine the parent symbol position so that newly-added properties
             # default to a sensible location (anchored near the component).
+            # KiCAD always emits the symbol's own (at x y angle) before any
+            # (property ...) child blocks, so the FIRST (at ...) inside the
+            # symbol block is the symbol origin regardless of whether
+            # (lib_name ...) precedes (lib_id ...).
             comp_at = re.search(
-                r'\(symbol\s+\(lib_id\s+"[^"]*"\s*\)\s+\(at\s+([\d\.\-]+)\s+([\d\.\-]+)',
+                r"\(at\s+([\d\.\-]+)\s+([\d\.\-]+)",
                 block_text,
             )
             comp_origin: Tuple[float, float] = (
@@ -1387,10 +1405,17 @@ class KiCADInterface:
             lib_sym_pos = content.find("(lib_symbols")
             lib_sym_end = find_matching_paren(content, lib_sym_pos) if lib_sym_pos >= 0 else -1
 
-            # Find the placed symbol block for this reference
+            # Find the placed symbol block for this reference. KiCAD may emit
+            # the children of (symbol ...) in different orders — most commonly
+            # `(symbol (lib_id "..."))`, but symbols whose library entry has
+            # been rescued / customised carry an extra `(lib_name "...")` first
+            # (`(symbol (lib_name "...") (lib_id "..."))`). Match `(symbol\s+(`
+            # — any opening paren — to handle both. The lib_symbols range check
+            # below excludes library-definition symbols, which use the
+            # `(symbol "name" ...)` form (quoted string, not paren).
             block_start = block_end = None
             search_start = 0
-            pattern = re.compile(r'\(symbol\s+\(lib_id\s+"')
+            pattern = re.compile(r"\(symbol\s+\(")
             while True:
                 m = pattern.search(content, search_start)
                 if not m:
@@ -1420,9 +1445,12 @@ class KiCADInterface:
 
             block_text = content[block_start : block_end + 1]
 
-            # Extract component position: first (at x y angle) in the symbol header line
+            # Extract component position: the first (at x y angle) inside the
+            # symbol block. KiCAD always writes the symbol's own (at) before
+            # any (property ...) child blocks, so the first match is the
+            # symbol origin regardless of the (lib_name)/(lib_id) ordering.
             comp_at = re.search(
-                r'\(symbol\s+\(lib_id\s+"[^"]*"\s*\)\s+\(at\s+([\d\.\-]+)\s+([\d\.\-]+)\s+([\d\.\-]+)\s*\)',
+                r"\(at\s+([\d\.\-]+)\s+([\d\.\-]+)\s+([\d\.\-]+)\s*\)",
                 block_text,
             )
             if comp_at:

tests/test_schematic_component_properties.py

@@ -39,6 +39,41 @@ PLACED_RESISTOR_BLOCK = """\
   )
 """
 
+# Multi-line placed-symbol block in the format KiCad emits for symbols whose
+# library entry has been rescued / customised — these carry an extra
+# (lib_name "...") child BEFORE (lib_id "..."). Reproduced from a real user
+# schematic that exposed a regex bug where (symbol (lib_id "...")) was the
+# only matched form and `(symbol (lib_name "...") (lib_id "..."))` placed
+# components were invisible to get/edit/delete.
+PLACED_RESISTOR_BLOCK_LIBNAME_FIRST = """\
+  (symbol
+    (lib_name "RESISTOR_0603_4")
+    (lib_id "MF_Passives:RESISTOR_0603")
+    (at 132.08 44.45 90)
+    (unit 1)
+    (in_bom yes)
+    (on_board yes)
+    (dnp no)
+    (uuid "bbbbbbbb-cccc-dddd-eeee-ffffffffffff")
+    (property "Reference" "R7"
+      (at 132.08 40.894 90)
+      (effects (font (size 1.143 1.143)))
+    )
+    (property "Value" "499k"
+      (at 132.08 42.418 90)
+      (effects (font (size 1.143 1.143)))
+    )
+    (property "Footprint" "MF_Passives_R0603"
+      (at 134.366 38.1 0)
+      (effects (font (size 1.27 1.27)) (hide yes))
+    )
+    (property "Datasheet" "~"
+      (at 132.08 44.45 0)
+      (effects (font (size 1.27 1.27)) (hide yes))
+    )
+  )
+"""
+
 
 def _make_test_schematic(tmp_dir: Path, extra_block: str = "") -> Path:
     """Copy empty.kicad_sch into tmp_dir, optionally appending a placed symbol block."""
@@ -657,3 +692,174 @@ class TestRemoveSchematicComponentProperty:
         )
         assert result["success"] is True
         assert "propertiesRemoved" not in result["updated"]
+
+
+# ---------------------------------------------------------------------------
+# Regression: KiCad emits (symbol (lib_name "...") (lib_id "...") ...) for
+# rescued / customised symbols. The original lookup regex required (lib_id
+# IMMEDIATELY after (symbol — silently failing on these blocks. All three
+# affected handlers (get / edit / set / remove) must find them.
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.integration
+class TestLibNameBeforeLibIdOrdering:
+    """Reproduces the real-world failure where R1 in a 90k-line user
+    schematic was reported as 'not found' because the placed (symbol ...)
+    block carried a (lib_name "...") child before (lib_id "...")."""
+
+    @pytest.fixture
+    def sch_with_libname(self, tmp_path: Any) -> Any:
+        return _make_test_schematic(tmp_path, PLACED_RESISTOR_BLOCK_LIBNAME_FIRST)
+
+    def _iface(self) -> Any:
+        from kicad_interface import KiCADInterface
+
+        return KiCADInterface()
+
+    def test_get_finds_symbol_with_libname_before_libid(self, sch_with_libname: Any) -> None:
+        iface = self._iface()
+        result = iface.handle_command(
+            "get_schematic_component",
+            {"schematicPath": str(sch_with_libname), "reference": "R7"},
+        )
+        assert result["success"] is True, result.get("message")
+        assert result["fields"]["Value"]["value"] == "499k"
+        assert result["fields"]["Footprint"]["value"] == "MF_Passives_R0603"
+        # Symbol position must come from the symbol's own (at), not a property
+        assert result["position"]["x"] == pytest.approx(132.08)
+        assert result["position"]["y"] == pytest.approx(44.45)
+
+    def test_set_property_works_on_symbol_with_libname_before_libid(
+        self, sch_with_libname: Any
+    ) -> None:
+        iface = self._iface()
+        result = iface.handle_command(
+            "set_schematic_component_property",
+            {
+                "schematicPath": str(sch_with_libname),
+                "reference": "R7",
+                "name": "Mfr",
+                "value": "Yageo",
+            },
+        )
+        assert result["success"] is True, result.get("message")
+        assert result["updated"]["propertiesAdded"]["Mfr"] == "Yageo"
+
+        verify = iface.handle_command(
+            "get_schematic_component",
+            {"schematicPath": str(sch_with_libname), "reference": "R7"},
+        )
+        assert verify["fields"]["Mfr"]["value"] == "Yageo"
+
+    def test_edit_with_properties_dict_works_on_libname_first_form(
+        self, sch_with_libname: Any
+    ) -> None:
+        iface = self._iface()
+        result = iface.handle_command(
+            "edit_schematic_component",
+            {
+                "schematicPath": str(sch_with_libname),
+                "reference": "R7",
+                "properties": {
+                    "MPN": "RC0603FR-07499KL",
+                    "Manufacturer": "Yageo",
+                    "Tolerance": "1%",
+                },
+            },
+        )
+        assert result["success"] is True, result.get("message")
+        assert set(result["updated"]["propertiesAdded"].keys()) == {
+            "MPN",
+            "Manufacturer",
+            "Tolerance",
+        }
+
+    def test_remove_property_works_on_libname_first_form(self, sch_with_libname: Any) -> None:
+        iface = self._iface()
+        # First add a property so we have something to remove
+        iface.handle_command(
+            "set_schematic_component_property",
+            {
+                "schematicPath": str(sch_with_libname),
+                "reference": "R7",
+                "name": "MPN",
+                "value": "RC0603FR-07499KL",
+            },
+        )
+        result = iface.handle_command(
+            "remove_schematic_component_property",
+            {
+                "schematicPath": str(sch_with_libname),
+                "reference": "R7",
+                "name": "MPN",
+            },
+        )
+        assert result["success"] is True, result.get("message")
+        assert "MPN" in result["updated"]["propertiesRemoved"]
+
+    def test_default_property_position_matches_symbol_origin_on_libname_first_form(
+        self, sch_with_libname: Any
+    ) -> None:
+        """Newly-added properties default to the parent symbol's (at) position.
+        This used to silently fall back to (0,0) on (lib_name)-first symbols
+        because the position-extraction regex required (symbol (lib_id ...) (at ...)).
+        """
+        iface = self._iface()
+        iface.handle_command(
+            "set_schematic_component_property",
+            {
+                "schematicPath": str(sch_with_libname),
+                "reference": "R7",
+                "name": "Mfr",
+                "value": "Yageo",
+            },
+        )
+        verify = iface.handle_command(
+            "get_schematic_component",
+            {"schematicPath": str(sch_with_libname), "reference": "R7"},
+        )
+        mfr = verify["fields"]["Mfr"]
+        assert mfr["x"] == pytest.approx(132.08)
+        assert mfr["y"] == pytest.approx(44.45)
+
+    def test_edit_builtin_fields_works_on_libname_first_form(self, sch_with_libname: Any) -> None:
+        """The pre-existing edit path (footprint / value / reference rewriting,
+        unrelated to custom properties) also relied on the buggy symbol-lookup
+        regex. Verify it now works on (lib_name)-first symbols too."""
+        iface = self._iface()
+        result = iface.handle_command(
+            "edit_schematic_component",
+            {
+                "schematicPath": str(sch_with_libname),
+                "reference": "R7",
+                "value": "1k",
+                "footprint": "Resistor_SMD:R_0603_1608Metric",
+            },
+        )
+        assert result["success"] is True, result.get("message")
+
+        verify = iface.handle_command(
+            "get_schematic_component",
+            {"schematicPath": str(sch_with_libname), "reference": "R7"},
+        )
+        assert verify["fields"]["Value"]["value"] == "1k"
+        assert verify["fields"]["Footprint"]["value"] == "Resistor_SMD:R_0603_1608Metric"
+
+    def test_delete_works_on_libname_first_form(self, sch_with_libname: Any) -> None:
+        """The pre-existing delete path used the same buggy symbol-lookup
+        regex and would silently report 'not found' for (lib_name)-first
+        symbols. Verify it can now locate and remove them."""
+        iface = self._iface()
+        result = iface.handle_command(
+            "delete_schematic_component",
+            {"schematicPath": str(sch_with_libname), "reference": "R7"},
+        )
+        assert result["success"] is True, result.get("message")
+
+        verify = iface.handle_command(
+            "get_schematic_component",
+            {"schematicPath": str(sch_with_libname), "reference": "R7"},
+        )
+        assert verify["success"] is False
+        assert "not found" in verify.get("message", "").lower()