Skip to content

fix(pcb): report an unassigned footprint instead of failing the sync - #523

Merged
neusse merged 1 commit into
mainfrom
fix/507-unassigned-footprint
Sep 13, 2026
Merged

neusse merged 1 commit into
mainfrom
fix/507-unassigned-footprint

Conversation

@mixelpixx

@mixelpixx mixelpixx commented Sep 11, 2026

Copy link
Copy Markdown
Owner

Summary

kicad-cli sch export netlist writes no (footprint …) node for a symbol whose Footprint property is empty, and parse_exported_netlist required one for every component. One footprint-less symbol — a legitimate state for a generic Device:R whose package is not chosen yet, and until #506 every symbol Konnect itself placed — failed the whole update_pcb_from_schematic with KiCad netlist node is missing footprint: no component named, every other component blocked with it.

Closes #507

Approach

Report, do not refuse — eeschema's own Update PCB dialog says "footprint not assigned" per symbol and continues.

  • One duplicate-reference invariant (review): every exported component is checked against one seen_references set before the footprint branch, so unassigned→unassigned, unassigned→assigned and assigned→unassigned repeats refuse with the existing error exactly as assigned→assigned always did; the plan never dedupes silently. Order-sensitive regressions are the real export with one (comp …) block duplicated by hand (kicad-cli cannot produce a duplicate).
  • Parse: footprint is optional. A component without one goes to a new unassigned list (reference, value, lib_id from libsource, symbol_path) instead of components; a nets node naming it is dropped rather than fatal (there is no pad to carry the net). "Zero components" is only fatal when both lists are empty.
  • Plan: plan_sync treats the list like skipped — a non-blocking category, never a diagnostic, because any diagnostic clears the whole plan today, which is exactly the failure this removes. Each entry carries what the board holds for it: absent (nothing is added; not counted under footprints_added) or kept (a footprint with that identity or reference already exists and is left exactly as it is — counted as matched, so not under board_only_preserved). Saved on_board / in_bom flags still apply to them.
  • Response: coverage.unassigned_footprint count pair beside skipped_by_flag, plus a top-level unassigned_footprints array. Derived from the parsed export, not echoed from any argument.

Branch and dependencies

Base branch: main at 331d98a (refreshed onto #528's merge, per the #530#528#523#535 train; migration entry placed directly below #528's).
Depends on: nothing remaining. Overlap: #535, #526, and #524 also touch docs/API_MIGRATIONS.md; #535 and #524 also touch tool-directory.md. Their implementation hunks are independent. The explicit shared-document order is #523 -> #535; #524 and #526 reconstruct independently after their author work. Independent of #506 (sch_components.rs), which fixes the placement side; this PR is about sync behavior when an unassigned symbol exists.
Series order: none — single PR, one commit.
Next PR to promote after this one: #535.

Compatibility and safety

  • Behaviour change (fix): a sync that failed outright on an unassigned symbol now plans every other component and reports the unassigned one. Recorded in docs/API_MIGRATIONS.md as a minor.
  • Additive response fields: coverage.unassigned_footprint, unassigned_footprints. status, changes, diagnostics and every existing count keep their names and meanings.
  • IPC mutations: unchanged path (attempt_ipc_write, one commit, post-apply read-back). An unassigned component is never sent to KiCad; a kept board footprint is not touched.
  • No tool added or removed; cargo xtask fix-doc-counts --check unchanged. Rollback: revert the one commit.

Validation

Run on commit 7d073eb (Windows, KiCad 10.0.5 installed), exit codes captured directly; the diff is byte-identical per file to d33780a/55e7b1b, on which the same gate and the seen_references neuters ran:

cargo fmt --all -- --check                                        exit 0
cargo clippy --workspace --locked --all-targets -- -D warnings    exit 0
cargo test --workspace --locked --lib --tests                     exit 0 (1777 passed, 0 failed)
cargo test --workspace --locked --doc                             exit 0
cargo xtask fix-doc-counts --check                                21 / 226 / 233; unchanged
cargo test -p konnect-core --lib pcb_sync                         37 passed (11 new)
  • cargo fmt --all -- --check
  • cargo test --workspace --locked --lib --tests
  • cargo test --workspace --locked --doc
  • cargo clippy --workspace --locked --all-targets -- -D warnings
  • Real-KiCad check: the fixture is a real kicad-cli sch export netlist, and the live E2E below ran against pcbnew 10.0 over IPC. Linux/macOS not run here; hosted CI covers the matrix.

Fixture from real KiCad output: tests/fixtures/unassigned_footprint.netkicad-cli sch export netlist of a project built through Konnect with R1 (Device:R, never assigned), C1 and R2 (assigned), R1 wired to C1. Only the (source …) path is neutralised; provenance and recipe in its README.

Reproduced first, on main with a release binary over MCP stdio: dry run on that project → conflicts.planned: 1, preflight KiCad netlist node is missing footprint, no plan, no component named.

Negative controls (each neutered, then restored):

guard neutered tests that fail
footprint required again (bail as before) all five new tests (a_component_without_a_footprint_is_reported_not_fatal, the_sync_goes_on_for_every_assigned_component, an_existing_board_footprint_for_an_unassigned_symbol_is_kept, an_all_unassigned_schematic_is_a_noop_with_the_list, the_unassigned_list_survives_a_conflict)
seen_references check disabled (review: one duplicate-reference invariant) a_repeated_unassigned_reference_is_refused, an_assigned_repeat_of_an_unassigned_reference_is_refused, an_unassigned_repeat_of_an_assigned_reference_is_refused, a_repeated_assigned_reference_is_still_refused, plus the pre-existing empty_and_duplicate_component_exports_are_rejected
collection key back to the singular the_response_names_the_collection_in_the_plural_and_the_count_in_the_singular
unassigned raised as a conflict diagnostic the_sync_goes_on_for_every_assigned_component, an_existing_board_footprint_for_an_unassigned_symbol_is_kept, an_all_unassigned_schematic_is_a_noop_with_the_list

Restored tree: 37/37 green.

Live E2E on this head's release binary (7d073eb), pcbnew 10.0.5 open on the project's board over IPC (driver e2e507.py; re-run 2026-09-13 after #530 restored the Windows endpoint):

step result
dry run status: ready, 2 footprints planned (C1, R2), unassigned_footprints: [{reference: R1, value: R, lib_id: Device:R, board_state: absent}], coverage.unassigned_footprint: {planned: 1, applied: 0}
apply with that revision status: applied, 2 planned / 2 applied, coverage.unassigned_footprint: {planned: 1, applied: 1}, same entry
dry run again status: noop, no changes, R1 still listed absent
save_project, then the saved .kicad_pcb read back footprint references in the file KiCad saved: C1, R2 — and no R1
every response the old singular key unassigned_footprint is absent (driver assertion)

Review checklist

  • The diff is focused and contains no generated output, personal data, or unrelated cleanup.
  • The branch includes current upstream/main, has no merge conflicts, and CI passed on this exact head.
  • The branch was based on latest upstream/main, not a release tag.
  • The PR shows only its unique commits and diff; dependencies and series position are explicit.
  • Every review conversation is resolved.
  • New names follow docs/NAMING_CONVENTIONS.md (unassigned_footprints, board_state); no public renames.
  • New behavior and failure paths have regression coverage.
  • No file mutations. IPC mutations verify the requested board and use one commit (unchanged path).
  • No tools added/removed; cargo xtask fix-doc-counts --check unchanged.

Maintainer merge state

🤖 Generated with Claude Code

@mixelpixx mixelpixx added area:pcb Board editing, export, manufacturing status:waiting-on-review Next actor: maintainer labels Sep 11, 2026
@mixelpixx
mixelpixx requested a review from neusse September 11, 2026 13:52

@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 restore one duplicate-reference invariant across assigned and unassigned components before this can merge.

Exact head reviewed: 05f31557f81bdc3f47d78651d90a4fa7ccf58931; all ten hosted checks are green, and the main #507 behavior is otherwise correctly scoped and evidenced. The parser now checks duplicates only in by_reference, but an unassigned component never enters that map. Therefore unassigned → unassigned and unassigned → assigned occurrences of the same reference are accepted; plan_sync later silently drops repeated unassigned entries. That can make malformed input look plausible and can classify one board footprint through two roles.

Use one seen_references set across every exported component before branching on footprint assignment. Preserve the existing fail-closed duplicate error, and add order-sensitive regressions for unassigned → unassigned and unassigned → assigned (plus the reverse if it is not already covered). Then rerun the ten checks and reply with the new head SHA. Closes #507 remains correct once this is fixed.

@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 11, 2026
@mixelpixx
mixelpixx force-pushed the fix/507-unassigned-footprint branch from 05f3155 to 55e7b1b Compare September 11, 2026 17:30
@mixelpixx

Copy link
Copy Markdown
Owner Author

Corrected and reconstructed once. Head: 55e7b1b70d3ca28abeb2e56657c8634d8fc6e6c0 on 8ed319a (current main after #485 and #527; the only conflict was docs/API_MIGRATIONS.md, resolved by keeping both entries with main's above ours). One unique commit.

The invariant: every exported component is checked against one seen_references set immediately after its ref is read, before the footprint branch, with the existing fail-closed error text unchanged. The plan_sync dedupe of repeated unassigned entries is gone — a duplicate can now only surface as the parser's refusal, never as a quietly dropped entry.

Order-sensitive regressions (the real export with one (comp …) block duplicated by hand, since kicad-cli cannot emit a duplicate reference): unassigned→unassigned, unassigned→assigned, assigned→unassigned, and assigned→assigned kept explicit; plus a control proving the duplicating helper yields a parseable export when the copy gets a fresh reference.

Negative control: with the seen_references check disabled, all four new tests and the pre-existing empty_and_duplicate_component_exports_are_rejected fail; restored, pcb_sync is 36/36.

Gate on this head: fmt / clippy -D warnings / --lib --tests (1777 passed, 0 failed) / --doc all exit 0; fix-doc-counts --check unchanged. Hosted CI running. PR body updated (base, validation, controls).

@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.

One public-response naming correction remains on exact head 55e7b1b70d3ca28abeb2e56657c8634d8fc6e6c0.

The earlier duplicate-reference blocker is resolved and the implementation now satisfies #507, but the new top-level array is named unassigned_footprint. docs/NAMING_CONVENTIONS.md requires collection responses to use a plural noun. Please rename that JSON collection to unassigned_footprints and update its tests, migration text, tool description, and PR body consistently. The singular coverage.unassigned_footprint count category may remain singular.

Then rerun all ten checks and reply with the new exact head. Closes #507 remains correct after this focused correction.

@neusse neusse added P0 Correctness or data-safety: fix before the next release and removed status:waiting-on-author Next actor: the PR author — one checklist, 14-day target labels Sep 11, 2026
@mixelpixx

Copy link
Copy Markdown
Owner Author

Renamed and reconstructed once. Head: d33780a0ed12c5af0037dda9a80e51d66c5b16b5 on 30e0abd — current main after #522 merged; the only conflict was docs/API_MIGRATIONS.md again, resolved with main's entries above ours. One unique commit.

The top-level collection is now unassigned_footprints. coverage.unassigned_footprint stays singular as the count category, per your note. Renamed with it: the tool description, the tool-directory.md row, the migration entry, and the PR body.

One thing I added rather than just renaming. The response shape had no test at all — every pcb_sync test asserts the SyncPlan struct or plan.counts, never the serialized JSON, so this rename (or a dropped field) was invisible to the suite. That is why it took a human reading to catch. the_response_names_the_collection_in_the_plural_and_the_count_in_the_singular now calls sync_response, parses the result, and asserts the plural array, its contents, the singular count, and that the old key is absent. Neutering the rename back to the singular fails it.

Negative controls on this head: seen_references disabled → the four duplicate-reference tests plus empty_and_duplicate_component_exports_are_rejected fail (unchanged from the last round); collection key back to the singular → the new response test fails. Restored: pcb_sync 37/37.

Gate: fmt / clippy -D warnings / --lib --tests (1784 passed, 0 failed) / --doc all exit 0; fix-doc-counts --check unchanged. The live leg could not be re-run tonight, and I would rather say so than imply it was. KiCad's API server on this machine had been switched off in kicad_common.json (api.enable_server: false) since the afternoon — every update_pcb_from_schematic call now returns transport unreachable, and after restoring the setting pcbnew still answers AS_NOT_READY and reports no open board, so the socket is down for reasons outside this branch. The live evidence in the PR body stands: it was taken this afternoon on the pre-rename head (05f3155), and this push changes one JSON key and adds a test — no behaviour. The key itself is proven by the new sync_response test above, including the absence of the old name, and by the neuter. I will re-run the full live chain and report it here once the endpoint is back. Hosted CI running.

Closes #507 remains the terminal reference.

@neusse

neusse commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

Please refresh this one focused commit onto current main (f0f5ad0) and rerun all ten required checks on the new exact head.

The plural unassigned_footprints correction resolves the prior review, and the implementation now satisfies #507 with correct terminal Closes #507 accounting. The only hard blocker is branch currency: exact head d33780a0ed12c5af0037dda9a80e51d66c5b16b5 still has merge-base 30e0abd, before #513. After refresh, verify the diff still contains only this PR's unique commit and update the PR checklist/evidence.

@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 12, 2026
@neusse

neusse commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

Merge-train sequencing correction: please hold the final refresh until #528 lands, then reconstruct #523 once onto that new current main. The shared documentation order is #530#528#523; this avoids repeatedly rebasing all three after each predecessor changes docs/API_MIGRATIONS.md or tool-directory.md. The plural-name review is resolved; after its turn arrives, #523 needs only its unique commit and fresh ten-check CI.

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

neusse commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator

#528 has landed as 331d98ad021bc2fde32778e7f0c11dd411b3bd40, so #523 is now next in the merge train. Please reconstruct its one unique terminal commit onto current main, preserve both preceding migration entries, rerun all ten required checks, update the PR body's current-base/exact-head evidence, and reply with the new head SHA.

The earlier duplicate-reference and plural unassigned_footprints corrections are accepted; no additional implementation change is requested. #523 remains the terminal Closes #507 PR.

@neusse neusse added 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 labels Sep 13, 2026
`kicad-cli sch export netlist` writes no `(footprint …)` node for a symbol
whose Footprint property is empty, and `parse_exported_netlist` required one
for every component. A single footprint-less symbol — a legitimate state
for a generic Device:R whose package is not chosen yet, and until #506 every
symbol Konnect itself placed — failed the whole sync with "KiCad netlist
node is missing footprint": no component named, every other one blocked.

The export now reads `footprint` as optional. A component without one goes
to a new `unassigned` list (reference, value, lib_id from `libsource`,
symbol path); a nets node naming it is dropped rather than fatal, since
there is no pad to carry the net. `plan_sync` treats the list the way it
treats `skipped`: a non-blocking category, never a diagnostic — any
diagnostic clears the whole plan, which is the failure this removes. Each
entry is reported with what the board holds for it: `absent` (nothing is
added, and it is not counted under `added`) or `kept` (a footprint with
that identity or reference exists and is left exactly as it is, counted as
matched, the way eeschema skips "cannot update … no footprint assigned" and
continues). Saved on_board / in_bom flags still apply to them.

One duplicate-reference invariant covers both roles: every exported
component is checked against one `seen_references` set before the
footprint branch, so an unassigned repeat of an assigned reference (or
the reverse) refuses exactly as an assigned repeat always did, and the
plan never dedupes silently.

Response: `coverage.unassigned_footprint` count pair beside
`skipped_by_flag`, and a top-level `unassigned_footprint` array. Derived
from the parsed export, not echoed. Additive; nothing renamed.

Evidence: a real kicad-cli export with one unassigned component wired to an
assigned one is the fixture (provenance in its README). Tests: parse; empty
board plans two additions and lists R1 absent, status ready; an existing
board footprint for R1 is kept and matched; all-unassigned is a noop with
the list; a genuine conflict clears changes but keeps the list.

Closes #507

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@mixelpixx
mixelpixx force-pushed the fix/507-unassigned-footprint branch from d33780a to 7d073eb Compare September 13, 2026 16:06
@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 13, 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.

Exact-head review complete for 7d073eb44969f7526d8c16890d1f8d4d1d3dd752 against current main 331d98ad021bc2fde32778e7f0c11dd411b3bd40.

Spec: pass. #507's terminal behavior, derived accounting, duplicate-reference invariant, real KiCad fixture, and focused regressions are complete.

Standards: source passes. Maintainer-side PR metadata now names the current base, shared-document overlap/order, next PR, and the correct plural-array/singular-count evidence fields. The remaining duplication/data-clump observations are non-blocking refactoring judgments outside this focused fix.

The first hosted rerun failed only because an unrelated existing mock listener encountered AddressInUse; failed/cancelled platform jobs are rerunning and must all pass before merge.

@neusse neusse added status:ready-to-merge Next actor: automation or maintainer — exact head reviewed and removed status:waiting-on-review Next actor: maintainer labels Sep 13, 2026
@mixelpixx

Copy link
Copy Markdown
Owner Author

Refreshed onto current main (331d98ad021bc2fde32778e7f0c11dd411b3bd40, which is #528's merge). New exact head: 7d073eb44969f7526d8c16890d1f8d4d1d3dd752, one commit.

Diff content is unchanged from d33780a: the pcb_sync.rs, sch_export.rs, tool-directory.md hunks and both fixture files are byte-identical (compared per file with the index lines stripped), and the migration entry's text is identical. Git applied the entry without a conflict but next to its old neighbour, mid-block; I moved it to sit directly below #528's entry, matching how #528's was placed below #530's.

All ten required checks green on 7d073eb. Local gate on the same head: fmt, clippy -D warnings, --lib --tests 1797 passed / 0 failed, --doc, fix-doc-counts --check unchanged.

Live IPC run, the leg that was blocked on the 09-11 head. Driver e2e507.py against this head's release binary, pcbnew 10.0.5 holding the project's board over IPC (the 09-11 run was blocked by the endpoint, #529). Dry run: status: ready, 2 footprints planned (C1, R2), unassigned_footprints: [{reference: R1, value: R, lib_id: Device:R, board_state: absent}], count {planned: 1, applied: 0}. Apply with that revision: status: applied, 2 planned / 2 applied, unassigned count {planned: 1, applied: 1}, same entry. Dry run again: status: noop, R1 still absent. save_project, then the saved .kicad_pcb read back: references C1, R2, no R1. The driver also asserts the old singular key is absent from every response. pcbnew closed afterwards.

PR body updated to the new head. Ready for the exact-head check. Terminal: Closes #507.

@mixelpixx mixelpixx added status:waiting-on-review Next actor: maintainer and removed status:waiting-on-review Next actor: maintainer labels Sep 13, 2026
@neusse
neusse merged commit 503b7f9 into main Sep 13, 2026
17 of 20 checks passed
@neusse
neusse deleted the fix/507-unassigned-footprint branch September 13, 2026 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:pcb Board editing, export, manufacturing P0 Correctness or data-safety: fix before the next release 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.

update_pcb_from_schematic fails the whole sync when any symbol has no footprint, instead of reporting the unassigned part

2 participants