Conversation
Amp-Thread-ID: https://ampcode.com/threads/T-01a096cc-b14c-75dd-9bc4-80cea267191f Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a096cc-b14c-75dd-9bc4-80cea267191f Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Stale comment
Left a non-blocking comment; not approved. Cursor Bugbot did not reach a terminal state within the wait window, and this is a functional user-facing import change rather than a small fixup, so human review is needed. No reviewers were assigned.
Sent by Cursor Approval Agent: Pull Request Router and Approver
Amp-Thread-ID: https://ampcode.com/threads/T-01a096cc-b14c-75dd-9bc4-80cea267191f Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a096cc-b14c-75dd-9bc4-80cea267191f Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Left a non-blocking comment; not approved. Cursor Bugbot passed with no unresolved findings that require human review, and Cursor Security Agent was not running. This is a functional user-facing import change rather than a small fixup, so human review is needed. No reviewers were assigned.
Sent by Cursor Approval Agent: Pull Request Router and Approver
|
Review follow-through for @LK is pushed through cfa6d040; no unresolved review threads remain. Latest-head CI: Linux/macOS/Windows tests, lint, Python tests, runtime checks, publish-bundle evaluation and performance checks pass. The earlier macOS KiCad-discovery test failure is fixed. Both E2E jobs still fail at Test pcb import (E2E), which consumes the private fixture repository. Under the requested repository/public-only data restriction, private fixture contents and their failing-step logs were not inspected. No coverage or electrical validation was disabled. A public reproduction is needed to diagnose those failures; CI is not green. Local verification: 108 importer/schematic tests; 231 shared tests including ignored/native cases; CLI build, strict scoped Clippy, format/diff checks and doctests pass. FTDI and Jetson project/root-schematic replays pass with BOM-only suppression, exact native physical partitions (110/177), byte-unchanged second applies and preserved physical PCB trees. Description and pinless-documentation native SVG renders match before/after pixel-for-pixel. Per-unit display overrides are explicitly rejected rather than silently collapsed. Quiche frontend and native GUI-save behavior remain untested. |
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| let layout_kicad_pro = match layout_kicad_pro { | ||
| Some(path) => path, | ||
| None => { | ||
| let path = layout_dir.join(selection.selected.kicad_sch.with_extension("kicad_pro")); | ||
| if !path.exists() { | ||
| fs::write(&path, "{}\n")?; | ||
| } | ||
| path | ||
| } | ||
| }; |
There was a problem hiding this comment.
🟡 Retained PCB bypasses import synchronization
A standalone reimport can retain a matching PCB while layout_kicad_pcb stays absent. Generation then defaults to four layers and skips PCB identity and net synchronization.
Learn more
The standalone force path preserves the existing layout/ directory and accepts its project when the project filename matches the imported schematic. The new project-selection block returns only the .kicad_pro path; layout_kicad_pcb remains None. write_imported_board_zen treats that as a schematic-only layout, uses four copper layers, and never calls the PCB prepatcher. The retained PCB remains discoverable through the board's layout_path, but its layer count, net names, and footprint Paths no longer participate in regeneration.
Example: Import a two-layer board.kicad_pro, then force-import board.kicad_sch into the same output. The matching layout/board.kicad_pro and layout/board.kicad_pcb survive, but the regenerated board declares four layers and the PCB keeps its old generated Paths.
Recommended fix: When standalone reimport accepts a retained matching project, discover its matching .kicad_pcb and populate MaterializedBoard.layout_kicad_pcb. Then run the same stackup extraction and PCB prepatch path as project import. If retaining an existing PCB is not supported, reject this case before cleanup instead of silently treating it as absent.
Was this helpful? React with 👍 or 👎 to provide feedback.


Purpose
Make
pcb importretain the original KiCad document for the same persistent schematic workflow used by Quiche andpcb apply schematic, rather than reconstructing a schematic frompcb:schplacement comments. There is no browser frontend embedded in the CLI.Behavior
layout/and enable persistent schematics. Bind managed symbols to evaluated component Paths/per-unit UUIDs and bind native labels/power pins to generated net identities without changing displayed text or native electrical merging.NotConnected()terminals; raw extraction and native documents remain truthful. Physical-pin partition validation and final schematic agreement remain blocking.Based on current main after #1280 and #1281. Their shared compatibility fixes, including both later #1280 review corrections and regressions, are preserved and not duplicated in this diff. Related compatibility context: ENG-1396/ENG-1736 (cache aliases), ENG-1412/ENG-1682 (stacked/hidden NC), ENG-1728 (page isolation only, not cross-sheet multi-unit support).
Diff after review fixes: 28 files, +2,619/−3,545, net −926 lines. This includes an unchanged 1,105-line public KiCad LM358 fixture and provenance README; no Antmicro designs are added as fixtures.
Review corrections preserve hidden-power binding provenance, separate parts with different displayed fields, reject incompatible retained project filenames before forced cleanup, and resolve schematic components absent from the PCB without replacing embedded geometry. Off-board pinless unbound symbols remain native documentation during apply; electrical or Path-bound symbols still require agreement. Differing display fields across units of one component are explicitly rejected rather than overwritten.
Executed verification
All checks ran on this importer branch using
CARGO_TARGET_DIR=/home/user/workspace/repo/target:cargo build -p pcbccargo nextest run -p pcbc -E 'test(import) | test(schematic)' --no-fail-fastcargo nextest run -p pcb-kicad-sch -p pcb-component-gen --run-ignored all --no-fail-fastcargo clippy -p pcbc -p pcb-kicad-sch -p pcb-component-gen --all-targets -- -D warningscargo test --doc -p pcbc -p pcb-kicad-sch -p pcb-component-gencargo fmt --all -- --check;git diff --checkRepository regressions cover original schematic/hierarchy preservation, same-sheet LM358 units 1/2/3, duplicate pin names, hidden/stacked NCs, native-vs-generated partitions, parity diagnostics, source/PCB preservation and unchanged second apply. Electrical fixture variants omit unsourced simulation parts from the BOM and build/apply without suppression; the stacked-NC/legacy BOM fixtures use BOM-only suppression. No electrical checks are suppressed.
Public replay: limited compatibility, not a general Antmicro success claim
Replayed all ten previously tested public repositories on the initial rebased importer commit with KiCad CLI 10.0.6, upgrading only disposable working copies. 19 attempts: four successes, fifteen documented failures, unchanged from the prior integration. After the review corrections, reran FTDI and Jetson in both modes: all four still pass with the partition/second-apply/PCB checks below. The other eight repositories were not rerun after review corrections.
For all four successes, executed:
Unsuppressed public builds fail on missing BOM information;
-S bombuilds and all eight applies pass, with no electrical suppression. Complete native(reference, physical pin)partitions match source exports in both modes. All second applies reportchanged: falseand preserve schematic/project bytes. Both full-project PCBs are unchanged by apply. A disposable Rust probe usingpcb-sexprcompares their entire source/applied PCB trees: identical except existing net-name/footprint Path bindings, retaining net codes, placement, routing and geometry.Jetson Auxiliary was exported to SVG before import and after two applies, rendered in Chromium at 2×, pixel-compared (
magick compare -metric AE: 0 (0)) and visually inspected. Symbols, wiring, graphics and NC crosses remain intact with no visible binding metadata.Review regressions additionally rendered differing descriptions and an off-board pinless documentation symbol through native SVG/Chromium at 2×. Both before/after comparisons returned zero differing pixels and were visually inspected.
Limitations
Buses, managed components in reused sheet files, cross-sheet multi-unit import, and differing per-unit display fields remain unsupported; failures are not bypassed. Detached child-only import was previously observed to fail and was not retested here. FTDI/Jetson standalone imports still lack custom footprint geometry and are not layout-ready. Source ERC/DRC/parity findings remain visible; success does not mean those source designs pass ERC/DRC. First apply may normalize KiCad document structure; the second must be unchanged.
The separate Quiche frontend and real native GUI-save behavior were not exercised. Verification covers shared schematic APIs, CLI workflows, native KiCad exports, and rendering. Only repository/public inputs were used; no private/customer designs or transcripts were accessed.
Note
High Risk
Major refactor of the KiCad import and schematic-reconciliation pipeline, including electrical connectivity semantics and apply-schematic binding on copied native documents.
Overview
KiCad import now keeps the original schematic hierarchy under
layout/and turns onschematic = Trueso Quiche andpcb apply schematicwork on the copied document instead of rebuilding placement from legacy# pcb:schcomments. The large schematic placement / passive-promotion / comment generation path is removed; components are built from page-local embedded symbol definitions, with Value / Footprint / Description (including empty strings) emitted into generated.zenviaproperties.After generation,
bind_imported_schematicpatches copied sheets with managedPath/ per-unit UUIDs and hiddenpcb:netbindings that map native labels and power pins to Zener nets without changing displayed names or KiCad merge behavior.pcb-kicad-schextends connectivity for those bindings, treats off-board pinless graphics as native documentation during apply, and exposesmarked_no_connect_targetsso stacked native no-connects driveNotConnected()validation. Source schematic/PCB parity is reported but no longer blocks import; structural pin/partition checks and final schematic inspection still fail bad conversions.Reviewed by Cursor Bugbot for commit cfa6d04. Bugbot is set up for automated code reviews on this repo. Configure here.