Skip to content

fix(schematic): stop the symbol writer corrupting KiCad 8/9 files - #351

Open
Rotario wants to merge 1 commit into
mixelpixx:mainfrom
Rotario:fix/schematic-writer-version-and-escaping
Open

fix(schematic): stop the symbol writer corrupting KiCad 8/9 files#351
Rotario wants to merge 1 commit into
mixelpixx:mainfrom
Rotario:fix/schematic-writer-version-and-escaping

Conversation

@Rotario

@Rotario Rotario commented Jul 30, 2026

Copy link
Copy Markdown

Hiya, I've been using claude to work with PCBs and doing some edits came up with some potential issues with the MCP server?

Here's claude's summary:

Placing a symbol made any KiCad 8 or KiCad 9 schematic unloadable, including this repo's own minimal/empty/template_with_symbols templates (all declare 20250114). Every failure surfaced only as a bare "Failed to load schematic" from eeschema and kicad-cli, naming neither token nor line.

Three defects in the shared writer:

  • create_component_instance emitted the KiCad 10-only (body_style 1) and (in_pos_files yes) attributes unconditionally. KiCad dispatches to a parser per declared format version, so a v10 token inside a 20231120 or 20250114 file is refused - by a KiCad 10 binary too. Both are now gated on the target file's own (version ...); a file with no version token keeps v10 output.

  • Property values were interpolated unescaped. Stock library descriptions embed double quotes - power:GND is 'Power symbol creates a global label with name "GND" , ground' - so the first inner quote closed the token early and the rest of the block became garbage. Parentheses still balanced and sexpdata still parsed the result, which is why this stayed silent; only KiCad objected. Values now go through escape_sexpr_string.

  • add_schematic_component dropped its documented angle and mirrorY arguments: the TS layer nests them inside component (src/tools/schematic.ts) and the Python handler never read them back out, so symbols always landed at 0deg and callers had to follow up with rotate_schematic_component.

Adds tests/test_schematic_writer_format_compat.py (24 tests; 17 fail before this change), including an integration test asserting real kicad-cli accepts the generated v8 and v9 files. test_symbol_instance_completeness.py asserted the v10 attributes while placing into the v9 empty.kicad_sch; it now uses a v10 fixture for those two tokens.

Placing a symbol made any KiCad 8 or KiCad 9 schematic unloadable, including
this repo's own minimal/empty/template_with_symbols templates (all declare
20250114). Every failure surfaced only as a bare "Failed to load schematic"
from eeschema and kicad-cli, naming neither token nor line.

Three defects in the shared writer:

- create_component_instance emitted the KiCad 10-only (body_style 1) and
  (in_pos_files yes) attributes unconditionally. KiCad dispatches to a parser
  per declared format version, so a v10 token inside a 20231120 or 20250114
  file is refused - by a KiCad 10 binary too. Both are now gated on the target
  file's own (version ...); a file with no version token keeps v10 output.

- Property values were interpolated unescaped. Stock library descriptions
  embed double quotes - power:GND is 'Power symbol creates a global label with
  name "GND" , ground' - so the first inner quote closed the token early and
  the rest of the block became garbage. Parentheses still balanced and
  sexpdata still parsed the result, which is why this stayed silent; only
  KiCad objected. Values now go through escape_sexpr_string.

- add_schematic_component dropped its documented angle and mirrorY arguments:
  the TS layer nests them inside `component` (src/tools/schematic.ts) and the
  Python handler never read them back out, so symbols always landed at 0deg
  and callers had to follow up with rotate_schematic_component.

Adds tests/test_schematic_writer_format_compat.py (24 tests; 17 fail before
this change), including an integration test asserting real kicad-cli accepts
the generated v8 and v9 files. test_symbol_instance_completeness.py asserted
the v10 attributes while placing into the v9 empty.kicad_sch; it now uses a
v10 fixture for those two tokens.
@mixelpixx

Copy link
Copy Markdown
Owner

Reviewed in full — the version-token corruption you found is real and I want this fix. Verified on current main: create_component_instance unconditionally emits the KiCad-10-only tokens (body_style 1) and (in_pos_files yes), and four of the five templates this repo ships are version 20250114 (v9), so placing a symbol into a project started from any of them produces a file KiCad 8/9 refuses with a bare 'Failed to load schematic'. Your _supports_kicad10_symbol_tokens gate (>= 20260101, unknown -> v10) is the right shape, and the 24-test suite including a real kicad-cli round-trip is exactly the evidence standard this repo wants.

Three asks before merge, all subtraction rather than addition:

  1. Rebase onto current main. fix(schematic): escape instance property writes (#324) #354 merged today and escapes the same _property line your escaping hunk touches — drop your escaping hunk entirely; fix(schematic): escape instance property writes (#324) #354 supersedes it and is broader (it also escapes reference/lib_id/project/path). The conflict is limited to that hunk and CHANGELOG.
  2. Drop the angle/mirrorY handler fix — it is real (the handler read only unit while create_component_instance has accepted angle/mirror_y all along), but Fix schematic corruption on quoted property values, and nets dropped during board sync #358 carries an identical hunk and whichever lands first should own it. Yours is currently positioned to land first, so alternatively: keep it here and I will ask Fix schematic corruption on quoted property values, and nets dropped during board sync #358 to drop theirs. Tell me which you prefer; if I hear nothing in a week I will take the version-gating fix over with credit, since v9 users are actively losing files to this.
  3. One reviewer question: your edit to test_symbol_instance_completeness.py swaps the fixture so the v10-token assertions run against a v10 file — as filed, the existing test was asserting the bug (v10 tokens in the v9 empty.kicad_sch). I read the swap as correct; flag anything I am missing.

The version-gating work with its test suite is the unique, high-value content here and it stands alone once the two duplicated hunks are gone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants