diff --git a/CHANGELOG.md b/CHANGELOG.md index e039189..d00ab29 100644 --- a/CHANGELOG.md +++ b/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 '' 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 diff --git a/python/kicad_interface.py b/python/kicad_interface.py index f0d37eb..ee49ad5 100644 --- a/python/kicad_interface.py +++ b/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() @@ -806,7 +806,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: @@ -1140,11 +1148,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 - # Search for (symbol (lib_id "...") ... (property "Reference" "" ...) ...) + # Find placed symbol blocks that match the reference. KiCAD may + # 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: @@ -1178,8 +1192,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] = ( @@ -1379,10 +1397,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: @@ -1412,9 +1437,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: diff --git a/tests/test_schematic_component_properties.py b/tests/test_schematic_component_properties.py index 57ece39..5e29686 100644 --- a/tests/test_schematic_component_properties.py +++ b/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()