Skip to content

feat(library): draw a symbol body with graphics primitives - #502

Merged
neusse merged 5 commits into
mixelpixx:mainfrom
triglav-modular:feat/501-symbol-graphics
Sep 10, 2026
Merged

feat(library): draw a symbol body with graphics primitives#502
neusse merged 5 commits into
mixelpixx:mainfrom
triglav-modular:feat/501-symbol-graphics

Conversation

@triglav-modular

@triglav-modular triglav-modular commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Summary

create_symbol gains graphics, so a symbol's body can be drawn instead of chosen from twelve fixed glyphs. Any part whose schematic symbol is a drawing rather than a box or a logic gate was previously unauthorable — and for a part with no stock symbol, such as a matched transistor pair, there was no approximate glyph to fall back on either.

Issue: #501

Closes #501

Reconstructed onto 296641b, refreshed onto 2fbc5f5, and now carrying the two spec fixes from the second review. Head names the exact reviewed state each time.

Approach

graphics is accepted top-level, beside pins, and per unit inside units[].

The primitive vocabulary is not new. set_footprint_graphics already defines line / arc / rect / circle / poly with {x, y} points and stroke_width_mm. Both schemas are now generated by one function, so the two domains cannot drift, and a test fails if a primitive is added to one and not the other. Only fill differs, deliberately: a footprint fills or it does not, while a symbol also has KiCad's pale background — which is what the stock libraries use for a body box, so it is not optional in practice.

set_footprint_graphics's public schema is byte-identical before and after this change; I snapshotted it either side.

Top-level single-unit geometry is emitted into NAME_0_1, following the existing single-unit path, which already puts body and pins there. Per-unit geometry goes into that unit's own NAME_<unit>_1 — not into the common unit. Verified against a two-unit symbol, where NAME_0_1 is absent entirely and each unit carries its own body.

Whether graphics is present is itself the contract. #501 suppresses the automatic body "whenever graphics is given for that unit", so:

A glyph on a unit that also supplies graphics is not drawn, and the response says so rather than discarding it silently. A triangular glyph's power split is likewise confined to the glyph-rendering path: it exists because a triangle's apex has no room for power-pin names, and a body the caller drew has whatever room they gave it.

Not included: text primitives, and no named glyph for any particular part. Generic primitives cover the cases below and the next one; another glyph covers exactly one part.

The three corrections from review

All three reproduced against the previous head before being accepted; all three were real. They are written by hand beside validate_graphics, in the style of validate_footprint_pad_items and parse_symbol_items — no general nested-schema validator, per your guidance.

1. A drawn-body pin with no coordinates is refused, not placed at the origin. The promise that makes this branch worth having is that it writes coordinates exactly as supplied, and build_symbol_unit was answering a missing x or y with unwrap_or(0.0). validate_drawn_pin_coordinates refuses at the handler boundary, where it can name units[i].pins[j].x as a structured invalid_argument, and nothing is written.

Its scope is the pins the drawn body carries, which is narrower than "all pins in the request" in exactly one case: a single-unit triangular glyph carrying power pins splits them onto a generated rectangular unit, where layout_power_unit unconditionally overwrites x and y on every pin it lays out. Those keep the schema's optional x/y. Indices stay those of the caller's own array, not of the filtered subset, so the field path names the pin the caller actually sent.

This is why I did not route the check through the seam #482 already provides. validate_pins takes a require_xy flag and power_pins passes true, so reusing it would have been the smaller change — but it would apply to a whole named array, and the requirement here is not "this array needs coordinates" but "the pins the drawn body carries need them". Passing true for the single-unit pins array would refuse a request whose coordinates layout_power_unit then discards. The separate validator exists for that difference alone, and split_power is computed once and used both to gate the check and to perform the split, so the exemption cannot drift from the thing it describes.

The unwrap_or(0.0) itself is gone as well, replaced by a refusal where the value is used. That is defence in depth for a future drawn call site, not the guard that produces the indexed message.

2. fill is required on rect, circle and poly. The schema said so; a missing one became (fill (type none)), an unfilled body chosen by Konnect where the stock libraries use background.

3. Unknown keys are refused per primitive, named as graphics[i].<key> — including a key that is real on a different primitive, such as radius_mm on a rect. fill on a line or arc keeps its existing, more specific message, which says where a fill does belong.

The allowed key set is held by hand next to the checks that use it, matching the other validators here. A test pins that table against the schema's own property list, so the copy cannot drift — and it also pins the property that lets one list serve as both the allowed set and the required set: on these five primitives every declared property is also required.

The two fixes from the second review

1. graphics: [] now means "no body". graphics_arg collapsed present-but-empty into absent, so the one request that cannot be expressed any other way drew a rectangle instead. Presence is now carried as Option, and build_symbol_unit takes the drawn path whenever the key was supplied.

2. A triangular glyph no longer drives the power split when geometry supersedes it. It did, so power pins were moved to a generated unit and layout_power_unit overwrote the coordinates the caller gave — breaking both advertised contracts at once. The split is now computed only when the glyph is the thing actually drawn.

The second fix deletes the exemption the first review endorsed. validate_drawn_pin_coordinates had exempted the power pins of a triangular glyph, on the correct observation that layout_power_unit would overwrite their coordinates anyway. That exemption was precisely scoped and still wrong — it made the misplacement consistent rather than removing it, and with the split confined there is nothing left to exempt. Every pin in a drawn scope now needs coordinates, power pins included.

Branch and dependencies

Base branch: main at 2fbc5f5.
Depends on: nothing outstanding. #482 has landed and is in the base.
Unique commits: five — the feature, two rounds of fixes from auditing it, one carrying the three corrections from the first review, and one carrying the two spec fixes from the second.
Series position: none. No overlap with #485, #489 or #499; this touches library.rs and footprint_graphics.rs.

Compatibility and safety

Additive only; nothing renamed or removed.

Request — two new optional properties:

where field meaning
top level graphics array of primitives; draws the single-unit pins body
units[] graphics same, for that unit

Responseunits[].body reports "graphics" when geometry was supplied, alongside the existing "rectangle" and glyph names.

Passing graphics at the top level and units is refused rather than silently dropped: top-level pins is legitimately superseded by units, but losing a redundant pin list is not the same as losing a drawing.

The three corrections refuse inputs that previously succeeded. This is not purely additive, and the three shapes that change are:

a call that previously succeeded before now
rect, circle or poly with no fill (fill (type none)) written refused: graphics[i] (rect): 'fill' is required …
any primitive carrying a key outside its schema key silently ignored, rest drawn refused: graphics[i].<key> …
a drawn-body pin with no numeric x or y pin written at (0 0) refused: units[i].pins[j].x, nothing written
graphics present but not an array read as absent; automatic body and success refused
graphics: [] automatic body drawn no body at all
triangular glyph + graphics + power pins power pins moved to a generated unit, coordinates overwritten drawn where supplied, no generated unit

Anyone with a working call in one of those shapes gets a refusal, or a different symbol, on upgrade. docs/API_MIGRATIONS.md carries the same table as the durable record. In each case what was written was not what was asked for, which is why all three were requested — but the versioning call is yours to make, and it needs the change stated in the specific to make it.

graphics is new in this PR and has shipped in no release, so the surface these tighten has no released callers; the shapes above are the ones anyone testing against this branch may have written.

Rollback is reverting the four commits. Nothing migrates: what it writes is what eeschema writes.

Validation

Run on the exact head being reviewed:

cargo fmt --all -- --check                                        clean
cargo test --workspace --locked --lib --tests                     1705 passed, 0 failed
cargo test --workspace --locked --doc                             pass
cargo clippy --workspace --locked --all-targets -- -D warnings    clean

The whole konnect-core suite was also run with HOME pointed at an empty directory and KICAD10_SYMBOL_DIR / KICAD10_FOOTPRINT_DIR unset — 1130 passed — so nothing here resolves a stock symbol through an installed KiCad and passes locally while failing on your Linux and Windows runners.

KiCad 10.0.6 round-trip, rerun on this head. A symbol with a background-filled body box, a filled triangle, a zigzag polyline, a lead, a circle and an arc, emitted through the real handler: kicad-cli sym upgrade reports "Symbol library was not updated" and the file is byte-identical afterwards; sym export svg plots it; all six primitives are present with the fills asked for (background, outline, four none); the pins are at exactly the coordinates supplied; there is one (rectangle — the drawn one, no automatic body; and the geometry is in Vactrol_Drawn_0_1, confirming the single-unit placement described above.

Auditing this found ten ways to get a wrong symbol and a success: true; review found three more. All are one family: the dispatch validates required arguments, not a oneOf inside an array item, so nothing stopped a malformed primitive reaching the emitter.

input before
type: "polyline" (typo for poly) zero primitives emitted, success
line with no end drawn to the origin
missing stroke_width_mm (schema: required) silently defaulted
poly with one point (schema: min 3) degenerate polyline
fill outside the vocabulary silently became none
zero/negative radius emitted
entry not an object refused, but as unknown type ""
graphics not an array read as absent; automatic body and success
units[].graphics not an array same
fill on line/arc honoured, though neither schema offers it
drawn pin with no x/y written at (0 0)
rect with no fill (fill (type none))
rect with colour / radius_mm silently ignored

Neutering

Every guard was reverted in turn and watched to fail with its own message, then restored:

guard reverted test that failed
the boundary pin-coordinate check create_symbol_refuses_a_drawn_pin_without_coordinates — the inner guard still refuses, but cannot name pins[1].x, so the field assertion fails
unwrap_or(0.0) restored in build_symbol_unit build_symbol_unit_refuses_to_invent_a_drawn_pin_coordinate
the split-power exemption create_symbol_drawn_body_leaves_split_power_pins_their_optional_coordinates — refuses pins[2].x on a request that should be accepted
fill required on the closed shapes create_symbol_requires_a_fill_on_the_closed_primitives
the unknown-key sweep create_symbol_rejects_unknown_keys_on_a_primitive
an extra key added to the hand-held table symbol_graphics_allow_exactly_the_schema_keys
[] collapsed back into absent create_symbol_accepts_an_empty_graphics_list_as_no_body_at_all
the power split ungated from graphics create_symbol_graphics_suppress_the_glyph_power_split
both of the above at once — the true pre-fix state create_symbol_drawn_power_pin_without_coordinates_is_refused

The last row is there because that test passes under a half-neutering: with the split restored but the exemption gone, the refusal still fires. Only reverting both together reproduces the behaviour that shipped, so only that reproduces the failure. A guard reverted one piece at a time can look covered when it is not.

create_symbol_triangular_glyph_without_graphics_still_splits_power also asserts the generated power unit keeps its body, because making presence meaningful turned &[] into a different request at five call sites and two of them must stay None. That assertion is a redundant second guard, not a new one — mutating the call site with it removed still fails the pre-existing glyph_opamp_with_power_splits_into_a_rect_power_unit, which counts rectangles and predates this branch. I looked for _2_1 assertions and did not find it, because it is named for the behaviour rather than the identifier. It is kept for the message it fails with rather than for coverage it adds, and its slice runs to end of file, so it would stop being load-bearing if a unit were ever appended after the power unit.

Two tests are expected to survive a neutering, by design, and would be misread as coverage otherwise:

  • create_symbol_drawn_body_leaves_split_power_pins_their_optional_coordinates passes with the coordinate guard removed entirely. It is a scope test, not a guard test: it exists to prove the guard is not too broad, and it fails only when the exemption it describes is removed.
  • create_symbol_without_a_graphics_key_still_draws_the_automatic_body passes with any of these guards removed. It pins the unchanged half of the presence contract, so a later tightening cannot sweep it up.

One correction to this section from the first review. I previously listed create_symbol_drawn_body_leaves_split_power_pins_their_optional_coordinates here as a scope test that survives neutering by design. It did survive — but it was asserting the behaviour your second review identified as wrong, and declaring a test as scope does not exempt its assertion from being wrong. Both it and the empty-graphics test have been rewritten to the corrected contract rather than adjusted until they passed.

Earlier in this PR's history two guards were found not load-bearing when first written — the tests passed with the guard removed, because they asserted only that something failed. Those tests assert the message now.

Not run: Windows and Linux; no environment here. Nothing in the change is platform-dependent, and hosted CI covers the matrix.

Review checklist

  • The diff is focused and contains no generated output, personal data, or unrelated cleanup.
  • The branch is based on current upstream/main (2fbc5f5) and has no conflicts.
  • The PR shows only its unique commits; fix(validation): reject malformed nested library inputs #482 has landed and is in the base, not in the diff.
  • New names follow docs/NAMING_CONVENTIONS.md — the primitive vocabulary is set_footprint_graphics's, unchanged.
  • New behavior and failure paths have regression coverage.
  • File mutations are atomic and preserve unrelated content.
  • IPC mutations — none.
  • No tools added or removed, so no counts to update; docs/API_MIGRATIONS.md carries the schema change.

@triglav-modular
triglav-modular marked this pull request as ready for review September 8, 2026 11:51
@triglav-modular

Copy link
Copy Markdown
Contributor Author

Marked ready for review. All ten required checks pass on this exact head, across macOS, Ubuntu and Windows.

Two corrections to what the description said while it was a draft, since both bear on how you schedule this one:

It is not waiting on anything. I had it as a draft on the reasoning that #485 and #489 are in triage and the refresh base comes after #442. That reasoning does not apply here: #442 touches Cargo.lock, crates/konnect/Cargo.toml, main.rs and run_registry.rs, none of which this branch goes near, and this branch touches only library.rs and footprint_graphics.rs — no overlap with #485, #489 or #483. It is based on current main and shows only its own three commits, so it needs no place in that ordering. Review or shelve it as suits your queue; it is not blocking and nothing blocks it.

One usage note that emerged after the description was written, from the person who asked for the capability, and worth having in the record even though it is KiCad behaviour rather than a defect here: KiCad centres a pin's number on the pin, so a zero-length pin puts its number on the body wall. Symbols drawn with graphics will usually still want a real stub — the stock resistor's 1.27 mm is a reasonable reference — even though the feature deliberately writes whatever coordinates it is given. Nothing in this PR changes as a result; it is guidance for whoever writes the first drawn symbol against it.

Four symbols have now been authored and rebuilt through this branch in real use, each checked by exporting KiCad's own netlist rather than by trusting the tool response.

@neusse

neusse commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Ask first: reconstruct this after #482 lands, and carry three missing request-contract guards into that reconstruction.

This is not independent of #482. Both change crates/konnect-core/src/tools/library.rs, and #482 is the accepted nested-input validation prerequisite for create_symbol. Landing this first would force that work to reconcile against a new 700-line path; the least-resistance order is #482, then this PR rebuilt once on current main.

The audit also found three silent-wrong-result cases in the current graphics path:

  1. With non-empty graphics, pin x and y become semantically required because the code promises to preserve exact supplied coordinates. They remain optional in the shared pin schema, and build_symbol_unit currently defaults either missing coordinate to 0.0. Refuse a drawn-body pin lacking numeric x or y, name its indexed path, and prove nothing is written.
  2. fill is schema-required for rect, circle, and poly, but validate_graphics accepts a missing fill as Null and the emitter silently chooses none. Require it for those primitives and add the red-before-green case.
  3. The nested schemas declare additionalProperties: false, but the manual validator does not reject extra keys. Since the dispatcher does not enforce these nested oneOf contracts, validate the allowed key set per primitive or centralize full schema validation; add an unknown-key regression.

Please also correct the PR explanation: top-level single-unit graphics live in NAME_0_1; per-unit graphics are correctly emitted in NAME_<unit>_1, not all in the common unit.

The overall primitive vocabulary and reuse of the footprint schema are sound. Do not repeatedly rebase now. Apply these corrections when reconstructing after #482, rerun the exact-head gate and KiCad round-trip, then return it for review.

@neusse neusse added the status:waiting-on-dependency Next actor: the dependency owner — see linked blocking issue label Sep 8, 2026
@triglav-modular

Copy link
Copy Markdown
Contributor Author

All three reproduced against the current head before accepting them, and all three are real. No rebase; the branch is untouched.

drawn-body pin with no x/y   -> written at (0 0)   coordinates the caller never gave
rect with no fill            -> emitted (fill (type none))
rect with {"colour":"red","radius_mm":9} -> both keys silently ignored

The first is the sharpest, and thank you for catching it: the whole justification for the drawn-body branch is that it writes coordinates exactly as supplied, and it was quietly inventing them from unwrap_or(0.0) when they were absent. That is the same defect the branch already fixes ten times over, sitting in the one place its own promise made it least excusable. I had audited this path twice and did not find it.

Your correction about the sub-symbols is right too, and I have fixed the description: top-level single-unit geometry goes to NAME_0_1, per-unit geometry to NAME_<unit>_1. Verified on a two-unit symbol — NAME_0_1 is absent entirely and each unit carries its own body. The description had said NAME_0_1 for both.

I have also corrected the independence claim in the description. It said this PR was independent; I had compared against #485, #489 and #483 and never checked #482, which is the one that matters. The order you set is noted: #482, then this rebuilt once on current main — I will not rebase before then.

One question before I write the fixes, so as not to duplicate #482. For item 3 you offer "validate the allowed key set per primitive or centralize full schema validation". Since #482 is fix(validation): reject malformed nested library inputs in the same file, does it introduce a general nested validator that create_symbol's graphics should route through? If so I would rather build on it than add a third hand-rolled validator beside validate_graphics and the pin checks — and items 1 and 2 may fall out of the same mechanism. If #482 is narrower than that, say so and I will do all three by hand in the reconstruction.

Either way I will apply the corrections in the rebuild, rerun the exact-head gate and the KiCad round-trip, and return it for review rather than assuming it is ready.

@mixelpixx

Copy link
Copy Markdown
Owner

#482 has landed as 296641b, so this can be rebuilt. Refresh base: main at 296641b.

Answer to your question, having reviewed and merged #482: it is narrower than a general nested validator. It adds two hand-rolled, per-field checks in library.rsvalidate_footprint_pad_items for create_footprint pads and parse_symbol_items (with an inner validate_pins) for create_symbol pins — each walking the schema-required fields by name and returning invalid_library_argument("units[0].pins[1].name", …). There is nothing to route graphics through, so do the three corrections by hand in the reconstruction, in the same style:

  1. With non-empty graphics, require numeric x/y on every pin the drawn body covers, naming units[i].pins[j].x; nothing written.
  2. Require fill on rect / circle / poly.
  3. Reject unknown keys per primitive (the additionalProperties: false the schema already declares), naming graphics[i].<key>.

Keep them next to validate_graphics so the four validators in that file read alike; a later PR can fold all of them into one nested-schema validator if the pattern keeps repeating.

When done: reconstruct once onto 296641b with only your commits, --force-with-lease, ten checks on the new head, and reply with the head SHA — I will review that exact head. @neusse fyi.

@triglav-modular
triglav-modular force-pushed the feat/501-symbol-graphics branch from 413b0c6 to 7161a23 Compare September 9, 2026 07:37
triglav-modular added a commit to triglav-modular/Konnect that referenced this pull request Sep 9, 2026
Three request-contract gaps in the `create_symbol` graphics path, all of
which returned success while writing something the caller did not ask for.
The MCP dispatch validates required *arguments*, not a `oneOf` inside an
array item, so each nested contract has to be enforced by hand here.

A drawn body writes pin coordinates exactly as supplied — that promise is
the reason the branch exists — but `build_symbol_unit` answered a missing
`x` or `y` with `unwrap_or(0.0)`, inventing an origin in the one place the
feature least excuses it. `x` and `y` are now required for the pins a drawn
body carries, refused as `units[i].pins[j].x` with nothing written. The
pins split onto a generated power unit are exempt: `layout_power_unit`
replaces their coordinates outright, so the drawn body never carries them.

`fill` is schema-required on `rect`, `circle` and `poly`, and a missing one
became `(fill (type none))` — an unfilled body chosen by Konnect where the
stock libraries use KiCad's pale `background`.

The nested schemas declare `additionalProperties: false`, but unknown keys
reached the emitter and were dropped silently, including keys that are real
on a different primitive. Each primitive's key set is now enforced and
named as `graphics[i].<key>`; `fill` on a `line` or `arc` keeps its more
specific message. The hand-held key table is pinned to the schema by a test
so the two cannot drift.

Written as four validators reading alike in this file rather than as a
general nested-schema validator, per maintainer guidance on mixelpixx#502.

Every guard was neutered and watched to fail before being trusted.

Part of mixelpixx#501

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@triglav-modular

Copy link
Copy Markdown
Contributor Author

Reconstructed once onto main at 296641b with the three corrections applied. Head is 7161a23 — force-pushed with --force-with-lease, no rebases before this one.

Thank you for the answer on #482; it settled the design. Having read validate_footprint_pad_items and parse_symbol_items on 296641b, I did all three by hand beside validate_graphics in that style, and did not attempt the general validator.

One placement note, since it is the one thing I did not do literally. For item 1 the natural seam was already there: validate_pins takes a require_xy flag and power_pins passes true, so reusing it would have been the smaller change. I wrote a separate validate_drawn_pin_coordinates instead, because that flag applies to a whole named array and the requirement is not "this array needs coordinates" but "the pins the drawn body carries need them" — and those differ in one case. A single-unit triangular glyph carrying power pins splits them onto a generated rectangular unit, and layout_power_unit unconditionally overwrites x and y on every pin it lays out. Passing true for the single-unit pins array would therefore refuse a request whose coordinates the code then discards. The guard skips exactly those, keeps the caller's own array indices so the field path names the pin they actually sent, and takes its exemption from the same split_power variable that performs the split, so the two cannot drift apart.

I have also stated the compatibility effect in the specific rather than in general, since it is yours to weigh: three shapes that previously succeeded now refuse — a rect/circle/poly with no fill, any primitive carrying a key outside its schema, and a drawn-body pin with no numeric x/y. The description has them in a table with the before and after.

The unwrap_or(0.0) you pointed at is gone too, replaced by a refusal where the value is used. That is defence in depth for a future drawn call site; the indexed units[i].pins[j].x message comes from the boundary check, and each has its own failing test.

Item 3 has one deliberate exception: fill on a line or arc was already refused with a message saying where a fill does belong, and the generic unknown-key sweep would have replaced it with something less useful. The specific message runs first. The allowed key set is held by hand next to the checks, matching the other validators here, and a test pins it against the schema's own property list so the two cannot drift.

Ten required checks green on 7161a23, across macOS, Ubuntu and Windows. Locally: fmt clean, 1681 tests passed, doc tests pass, clippy clean with -D warnings. The suite was also run with HOME empty and the KiCad library directories unset, so nothing here passes locally by finding an installed KiCad.

KiCad 10.0.6 round-trip rerun on this head: sym upgrade reports the library was not updated and the file is byte-identical afterwards, sym export svg plots it, all six primitives are present with the fills asked for, the pins are at exactly the coordinates supplied, and there is one rectangle — the drawn one.

Every guard was reverted in turn and watched to fail with its own message. Two tests are expected to survive that by design, and I have said which and why in the description rather than counting them as coverage: the split-power case is a scope test proving the guard is not too broad, and the empty-graphics case pins [] as a coherent request. The neutering table in the description lists each guard against the test that catches it.

@neusse neusse added status:waiting-on-review Next actor: maintainer status:waiting-on-author Next actor: the PR author — one checklist, 14-day target and removed status:waiting-on-dependency Next actor: the dependency owner — see linked blocking issue status:waiting-on-review Next actor: maintainer labels Sep 9, 2026
@neusse

neusse commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Please refresh this branch onto current main and rerun the ten required checks; no code redesign is being requested.

The #482 dependency is satisfied and the PR was correctly reconstructed at 296641b, but #509 has since moved main to ab337816. This change is independent, so one clean update should be sufficient. After the refreshed exact head is green, it returns to maintainer review behind #402.

triglav-modular added a commit to triglav-modular/Konnect that referenced this pull request Sep 10, 2026
Three request-contract gaps in the `create_symbol` graphics path, all of
which returned success while writing something the caller did not ask for.
The MCP dispatch validates required *arguments*, not a `oneOf` inside an
array item, so each nested contract has to be enforced by hand here.

A drawn body writes pin coordinates exactly as supplied — that promise is
the reason the branch exists — but `build_symbol_unit` answered a missing
`x` or `y` with `unwrap_or(0.0)`, inventing an origin in the one place the
feature least excuses it. `x` and `y` are now required for the pins a drawn
body carries, refused as `units[i].pins[j].x` with nothing written. The
pins split onto a generated power unit are exempt: `layout_power_unit`
replaces their coordinates outright, so the drawn body never carries them.

`fill` is schema-required on `rect`, `circle` and `poly`, and a missing one
became `(fill (type none))` — an unfilled body chosen by Konnect where the
stock libraries use KiCad's pale `background`.

The nested schemas declare `additionalProperties: false`, but unknown keys
reached the emitter and were dropped silently, including keys that are real
on a different primitive. Each primitive's key set is now enforced and
named as `graphics[i].<key>`; `fill` on a `line` or `arc` keeps its more
specific message. The hand-held key table is pinned to the schema by a test
so the two cannot drift.

Written as four validators reading alike in this file rather than as a
general nested-schema validator, per maintainer guidance on mixelpixx#502.

Every guard was neutered and watched to fail before being trusted.

Part of mixelpixx#501

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@triglav-modular

Copy link
Copy Markdown
Contributor Author

Refreshed. Head is bd20358.

One difference from your instruction, flagged rather than silently absorbed: the base you named, ab337816, had gone stale before I reached it. #402 merged and moved main to 2fbc5f5, so I rebased onto that. That is also the review position you queued this behind, so it has cleared.

The rebase carried the reviewed content unchanged — the patch against 2fbc5f5 is byte-identical to the one against 296641b, four commits, no conflicts, no file overlap with anything merged since. No code redesign, as asked.

Ten required checks green on bd20358.

@neusse neusse added status:waiting-on-review Next actor: maintainer and removed status:waiting-on-author Next actor: the PR author — one checklist, 14-day target labels Sep 10, 2026

@neusse neusse left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make presence of graphics authoritative and prevent an ignored glyph from changing custom-graphics semantics.

Two spec blockers remain on exact head bd2035831275a3bb9d3105c81ddfccbdfb1dfe6c:

  1. #501 says supplying graphics suppresses the automatic body. graphics_arg currently erases present-versus-absent into an empty vector and build_symbol_unit uses the graphics path only when non-empty, so graphics: [] draws the automatic rectangle. Preserve presence as a distinct state: omitted means use the glyph/automatic body; present (including []) means no automatic body. Add a bodyless-symbol test and verify KiCad round-trip.
  2. A supplied triangular glyph still drives split_power even when custom graphics supersede that glyph. That silently moves power pins to a generated unit and overwrites their supplied coordinates despite the advertised “graphics wins” and exact-coordinate contracts. Compute power splitting only for the actual glyph-rendering path, or reject the combined request. Test custom graphics plus triangular glyph and power pins.

Please also add the shipped docs/API_MIGRATIONS.md entry for the new optional top-level/per-unit graphics contract and next-minor impact; the PR body alone is not durable user documentation.

Non-blocking design debt: the five primitive contracts are repeated across schema, allowed-key validation, semantic validation, and emission. The drift test protects today’s keys, so do not expand this PR solely to refactor it, but a future typed SymbolGraphic/single descriptor would reduce repeated-switch and shotgun-surgery risk. One commit subject is slightly over the roughly-72-character guide; no history rewrite requested.

After the two behavioral fixes and migration entry, rerun all ten checks and return the new head. This is independent of #489 and remains in review behind the current P0 merge candidate #511.

@neusse neusse added status:waiting-on-author Next actor: the PR author — one checklist, 14-day target and removed status:waiting-on-review Next actor: maintainer labels Sep 10, 2026
@triglav-modular

Copy link
Copy Markdown
Contributor Author

Both blockers fixed, migration entry added. Head is e63e7e8; base is still 2fbc5f5, which is current main.

1. graphics: [] now means "no body". graphics_arg collapsed present-but-empty into absent, so the one request that cannot be expressed any other way drew a rectangle. Presence is carried as Option now: omitted keeps the automatic body exactly as before, supplied suppresses it, empty or not. Bodyless symbol tested and round-tripped — sym upgrade leaves it byte-identical and sym export svg plots it.

2. The power split is confined to the glyph-rendering path. With geometry supplied it no longer fires, so power pins stay where the caller put them. Tested in all three directions: suppressed with graphics, still working on the glyph path without them, and a drawn power pin now refused for missing coordinates.

The second fix deletes the exemption I added for the first review, and that is the part worth naming. validate_drawn_pin_coordinates had exempted a triangular glyph's power pins, on the correct observation that layout_power_unit would overwrite their coordinates anyway. The exemption was accurate about the code and still wrong: it made the misplacement consistent instead of removing it. Neither of us asked whether the split should be happening at all — and being able to show the exemption could not drift is what made it look finished. With the split confined there is nothing left to exempt, so every pin in a drawn scope now needs coordinates, power pins included.

Two tests asserted the old behaviour and are corrected, not adjusted to pass. One of them I had declared to you as a scope test that survives neutering by design. It did survive — but it was pinning the contract your review identified as wrong. Declaring a test as scope does not exempt its assertion from being wrong, and I have corrected that claim in the description rather than dropping it.

3. docs/API_MIGRATIONS.md entry added, matching the existing ## Unreleased: structure: the additive argument, the presence contract, the power-split change, and the four request shapes that used to succeed and now refuse or differ.

Taken as read, and not expanded: the repeated five-primitive contracts are acknowledged debt for a typed descriptor later, and the published commit subjects are left intact.

Five commits, three files. Full four-command gate on the exact head, plus the run with HOME empty and the KiCad library directories unset. Both new guards neutered against the true pre-fix state — old split and old exemption together — and watched to fail with their own messages.

Ten required checks green on e63e7e8.

triglav-modular added a commit to triglav-modular/Konnect that referenced this pull request Sep 10, 2026
Three request-contract gaps in the `create_symbol` graphics path, all of
which returned success while writing something the caller did not ask for.
The MCP dispatch validates required *arguments*, not a `oneOf` inside an
array item, so each nested contract has to be enforced by hand here.

A drawn body writes pin coordinates exactly as supplied — that promise is
the reason the branch exists — but `build_symbol_unit` answered a missing
`x` or `y` with `unwrap_or(0.0)`, inventing an origin in the one place the
feature least excuses it. `x` and `y` are now required for the pins a drawn
body carries, refused as `units[i].pins[j].x` with nothing written. The
pins split onto a generated power unit are exempt: `layout_power_unit`
replaces their coordinates outright, so the drawn body never carries them.

`fill` is schema-required on `rect`, `circle` and `poly`, and a missing one
became `(fill (type none))` — an unfilled body chosen by Konnect where the
stock libraries use KiCad's pale `background`.

The nested schemas declare `additionalProperties: false`, but unknown keys
reached the emitter and were dropped silently, including keys that are real
on a different primitive. Each primitive's key set is now enforced and
named as `graphics[i].<key>`; `fill` on a `line` or `arc` keeps its more
specific message. The hand-held key table is pinned to the schema by a test
so the two cannot drift.

Written as four validators reading alike in this file rather than as a
general nested-schema validator, per maintainer guidance on mixelpixx#502.

Every guard was neutered and watched to fail before being trusted.

Part of mixelpixx#501

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@triglav-modular

Copy link
Copy Markdown
Contributor Author

Refreshed onto main at 3cde785. Head is 5458623.

The only conflict was docs/API_MIGRATIONS.md, and both entries are preserved. #511's is carried byte for byte as main has it, in the same position relative to the entries below it; ours is prepended above, following the file's newest-first convention. Nothing else conflicted — the two PRs share no code. git range-diff reports the four earlier commits identical and the fifth differing only in that file's neighbouring context line.

Ordering between two unreleased entries is the one judgement call here, so: if you would rather ours sat below #511's, say so and I will flip it — no need for a round trip on anything else.

Ten required checks green on 5458623.

Worth one line, since it will recur rather than because it needs solving: this conflict is a construction artefact of the migration-entry requirement. Every entry is prepended to the same place in one file, so any two PRs that both carry one collide as soon as either merges — the same shape CONTRIBUTING.md already describes for tool counts, where cargo xtask fix-doc-counts exists to absorb it. #489 now carries an entry too, so whichever of it and this one lands first will turn the other DIRTY. One rebase each, not a cascade.

@neusse

neusse commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Queue refresh — next action is with the author, and this PR remains first in the landing order.

The corrections on exact head 54586239c3a1dac91de78a5ad86d0a6614cb3e3e address the prior review: graphics: [] is authoritative, custom graphics no longer inherit glyph-driven power splitting, drawn pins are validated, and the additive contract is recorded in docs/API_MIGRATIONS.md. I found no remaining substantive code or #501 acceptance blocker.

Please refresh once onto current main at c47a9fc6f819f5a27cb11281c492190711676f68, preserving the existing migration entries, rerun all ten required checks, and reply with the new head SHA. After that exact-head review passes, this is the first merge candidate and will close #501. The current status:waiting-on-author label remains correct until that refresh arrives.

triglav-modular and others added 5 commits September 10, 2026 13:00
create_symbol could not draw. A symbol's body was whichever of twelve
fixed glyphs you picked, so any part whose schematic symbol is a drawing
rather than a box or a logic gate could not be authored at all -- and for
a part with no stock symbol, such as a matched transistor pair, there was
no approximate glyph to fall back on either (mixelpixx#501).

`graphics` is accepted top-level, beside `pins`, and per unit inside
`units[]`. The primitive vocabulary is the one set_footprint_graphics
already defines -- line, arc, rect, circle, poly, points as {x, y},
stroke_width_mm -- and both schemas are now built from one function, so
the two domains cannot drift. A test fails if they do.

Only `fill` differs, deliberately: a footprint fills or it does not,
while a symbol also has KiCad's pale `background` body fill, which is
what the stock libraries use for a body box. set_footprint_graphics'
public schema is byte-identical before and after this change.

Two behaviours follow from a caller supplying geometry, and both are
what a drawn body needs. The automatic rectangle is not emitted, because
it exists to give pins something to sit on and a drawn body already has
one. And pin x/y are written exactly as supplied, rather than slid out to
a computed body edge as the automatic path does (mixelpixx#293) -- the drawing
already fixes where the pins belong.

Verified against KiCad 10.0.6, not only in-process: a symbol with a
background-filled body box, a filled LED triangle, a US zigzag polyline,
leads and a circle loads, round-trips through `kicad-cli sym upgrade`
unchanged, renders through `sym export svg`, places into a schematic with
its pins exactly where asked, and exports a netlist.

Every guard was neutered and watched to fail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…m wrongly

Adversarial audit of the feature found seven ways to get a wrong symbol
and a success, all the same family: the MCP dispatch validates required
*arguments*, not a oneOf inside an array item, so nothing stopped a
malformed primitive reaching the emitter.

  type "polyline" (a typo for "poly")  -> zero primitives emitted, success
  line without an end                  -> drawn to the origin
  no stroke_width_mm (schema: required)-> silently defaulted
  poly with one point (schema: min 3)  -> degenerate polyline emitted
  fill "chartreuse"                    -> silently became none
  glyph and graphics together          -> glyph discarded without a word
  top-level graphics with units[]      -> drawing dropped entirely, success

The first five and the seventh now refuse, naming the entry index, the
primitive and the field, and nothing is written. The sixth warns: geometry
replacing a glyph is the documented rule, but discarding an explicit
request in silence is not.

The seventh is worth its own note. Top-level `pins` is superseded by
`units` and the schema says so, but losing a redundant pin list is not
losing a drawing -- geometry sent there would never reach the file, and
the symbol came out with an automatic rectangle and a success.

Also checked and found correct: coordinates survive fmt_f64 exactly
(3.175, 2.117, 1.905 round-trip), per-unit graphics do not leak to
sibling units (a unit without graphics keeps its automatic body), field
anchors follow the drawn bounding box rather than a rectangle that is no
longer there, and the triangular-glyph power split still produces a drawn
unit 1 beside a generated power unit 2.

Every new guard was neutered and watched to fail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second audit pass, after the primitive validation was already in. Three
more of the same family:

  graphics: "rect" (not an array)      -> read as absent; automatic body,
                                          success, the drawing nowhere
  units[].graphics as an object        -> same
  fill on a line                       -> honoured by the emitter, though
                                          neither schema offers it

The first two came from `as_array().unwrap_or_default()`, which turns a
wrong request into an empty one. `graphics_arg` now distinguishes absent
from malformed and refuses the latter. The third is the schema saying one
thing while the writer does another, which is the same defect as ignoring
the field; `line` and `arc` now refuse a fill and point at `poly`.

An explicitly empty list still means "draw nothing of my own" and gets
the automatic body, matching how require_array treats [] elsewhere. That
case now has a test so it cannot be swept up by a later tightening.

Also improved a message: a non-object entry reported `unknown type ""`,
which sends the caller looking for a typo in a field they never wrote.
The guard for that was not load-bearing at first -- the test passed with
it removed -- so the test now asserts the message, not just the refusal.

Every new guard neutered and watched to fail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three request-contract gaps in the `create_symbol` graphics path, all of
which returned success while writing something the caller did not ask for.
The MCP dispatch validates required *arguments*, not a `oneOf` inside an
array item, so each nested contract has to be enforced by hand here.

A drawn body writes pin coordinates exactly as supplied — that promise is
the reason the branch exists — but `build_symbol_unit` answered a missing
`x` or `y` with `unwrap_or(0.0)`, inventing an origin in the one place the
feature least excuses it. `x` and `y` are now required for the pins a drawn
body carries, refused as `units[i].pins[j].x` with nothing written. The
pins split onto a generated power unit are exempt: `layout_power_unit`
replaces their coordinates outright, so the drawn body never carries them.

`fill` is schema-required on `rect`, `circle` and `poly`, and a missing one
became `(fill (type none))` — an unfilled body chosen by Konnect where the
stock libraries use KiCad's pale `background`.

The nested schemas declare `additionalProperties: false`, but unknown keys
reached the emitter and were dropped silently, including keys that are real
on a different primitive. Each primitive's key set is now enforced and
named as `graphics[i].<key>`; `fill` on a `line` or `arc` keeps its more
specific message. The hand-held key table is pinned to the schema by a test
so the two cannot drift.

Written as four validators reading alike in this file rather than as a
general nested-schema validator, per maintainer guidance on mixelpixx#502.

Every guard was neutered and watched to fail before being trusted.

Part of mixelpixx#501

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two spec gaps from review, both of which returned a symbol other than the
one asked for.

`graphics: []` drew the automatic body. mixelpixx#501 suppresses that body "whenever
graphics is given for that unit", and an empty array is given — it asks for
a symbol with no body at all, which no other argument can express.
`graphics_arg` collapsed present-but-empty into absent, so the request was
unreachable. Presence is now a state of its own: omitted keeps the existing
automatic body exactly, supplied suppresses it, empty or not.

A triangular glyph still drove the power split when graphics superseded the
glyph, so power pins were moved to a generated unit and layout_power_unit
overwrote the coordinates the caller gave. That broke both advertised
contracts at once: that geometry wins, and that pin coordinates are written
as supplied. The split now belongs to the glyph-rendering path alone, where
its reason lives — a triangle's apex has no room for power-pin names, while
a body the caller drew has whatever room they gave it.

The second fix removes the power-pin exemption inside
validate_drawn_pin_coordinates. That exemption was precisely scoped and
still wrong: it made the misplacement consistent rather than removing it,
and with the split confined there is nothing left to exempt. Every pin in a
drawn scope now needs coordinates, power pins included.

Two tests asserted the old behaviour and are corrected rather than adjusted
to pass. One of them had been declared a scope test, which does not exempt
its assertion from being wrong. Added: a bodyless symbol, an omitted
graphics key, the suppressed split, the split that must survive on the
glyph path, and a drawn power pin refused for missing coordinates. Both new
guards neutered against the true pre-fix state and watched to fail.

docs/API_MIGRATIONS.md records the additive argument, the presence
contract, the power-split change and the four request shapes that used to
succeed and now refuse.

Verified against KiCad 10.0.6: a bodyless symbol and a drawn op-amp both
round-trip byte-identical through sym upgrade and plot through sym export
svg, with the power pins at exactly the coordinates supplied.

Part of mixelpixx#501

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@neusse
neusse force-pushed the feat/501-symbol-graphics branch from 5458623 to d7ed665 Compare September 10, 2026 20:04
@neusse

neusse commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Maintainer refresh completed onto current main (c47a9fc6f819f5a27cb11281c492190711676f68).

New head: d7ed665f89d961f337881d3c0598820ccc43b6b7

git range-diff shows all five commits patch-identical to the prior series; only their parent changed. The complete local gate passed: formatting, clippy with warnings denied, workspace lib/tests, and documentation tests. Hosted checks are now the remaining gate; no further author action is requested while they run.

@neusse neusse left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved on exact head d7ed665f89d961f337881d3c0598820ccc43b6b7 after maintainer refresh onto main at c47a9fc6f819f5a27cb11281c492190711676f68.

The two prior behavioral blockers are resolved: explicit graphics, including [], suppresses the automatic body; custom graphics no longer inherit glyph-driven power splitting. Drawn-pin validation and the durable API migration entry are present. git range-diff shows all five patches unchanged by the refresh. The complete local gate and all ten required hosted checks pass on this exact head. This is focused, mergeable, and correctly carries Closes #501.

@neusse neusse added status:ready-to-merge Next actor: automation or maintainer — exact head reviewed and removed status:waiting-on-author Next actor: the PR author — one checklist, 14-day target labels Sep 10, 2026
@neusse
neusse merged commit 52e8ef8 into mixelpixx:main Sep 10, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:ready-to-merge Next actor: automation or maintainer — exact head reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

create_symbol cannot draw: no graphics primitives, and glyph is a fixed twelve with no transistor

3 participants