Skip to content

fix(drc): name what owns each DRC report item (#413) - #439

Closed
tonydzi wants to merge 1 commit into
mixelpixx:mainfrom
tonydzi:fix/drc-ownership-enrichment
Closed

fix(drc): name what owns each DRC report item (#413)#439
tonydzi wants to merge 1 commit into
mixelpixx:mainfrom
tonydzi:fix/drc-ownership-enrichment

Conversation

@tonydzi

@tonydzi tonydzi commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Closes #413.

run_drc and get_drc_violations reduced every KiCad report item to description, pos, and an optional uuid. A copper_edge_clearance item therefore reads identically whether the offending Edge.Cuts geometry is the board's real outline or a cutout a footprint carries in its own artwork — and the remedies are opposite. In the reported case that produced several turns of wrong fix guidance before manual cross-referencing against list_board_footprint_graphics found the real cause.

Footprint ownership does not make the finding false. J1's circles are real cutouts in real copper. Ownership selects the remedy: a pad and a cutout carried by the same footprint move together, so repositioning the component cannot change their mutual clearance, and the footprint definition or the rule is what needs review.

What this does

Follows the focused PR direction on the issue:

  1. konnect_sexp::board::uuid_index builds a UUID index from the exact saved .kicad_pcb that DRC ran on, using the existing s-expression parser — no new parser, no regex.
  2. Top-level children of (kicad_pcb …) index as owner.kind: "board"; nodes nested under a (footprint …) index as owner.kind: "footprint" with the footprint's reference and UUID. A (footprint …) node is itself a top-level board item, so it indexes as board-owned with item_kind: "footprint".
  3. Every DRC ReportItem is enriched by exact UUID only with item_kind, layer, owner, and ownership_status.
  4. When KiCad omits the UUID, or it is not in the board, the item comes back "owner": null with "ownership_status": "uuid_missing" or "not_found". Ownership is never inferred from prose like "Circle of J1" — a test renames every reference in the board and proves the reported reference changes with it while the description does not.
  5. The enrichment lives in cli::run_drc, the one path both tools take, so run_drc and get_drc_violations cannot disagree. All three categories — violations, unconnected_items, schematic_parity — go through it.
  6. KiCad's description, pos, uuid, severity, and rule are unchanged. The four new fields serialise away when unset, so ERC — which shares ReportItem and has no board to index — and any caller that ignores them see exactly the previous response. A board that will not parse still returns full DRC results, unannotated, with a warning logged: withholding findings over a failed lookup would be the worse answer.

A report with no items at all skips the board re-read and parse entirely, so a clean board costs nothing new. run_drc is called once per candidate inside the routing loop, which is where that would otherwise have been felt.

Response shape

{"description":"Circle of J1 on Edge.Cuts","pos":{"x":136.19,"y":93.375},
 "uuid":"7b970478-1e4a-48b6-b01a-35348027ca5e",
 "ownership_status":"resolved","item_kind":"fp_circle","layer":"Edge.Cuts",
 "owner":{"kind":"footprint","reference":"J1",
          "uuid":"b432574a-bdcd-4387-8d5e-65f34938c3a0"}}

Board-owned items carry "owner":{"kind":"board"}. Unresolved items carry "owner": null beside the status that says why. layer comes only from a single (layer …); pads spell (layers …) and are left unset rather than having one of three guessed for them.

Fixture provenance

crates/konnect-core/tests/fixtures/drc_ownership_j1.{kicad_pcb,drc.json} is the board and raw report @ncolomer pasted inline in the comment on #413 — no file was attached; the bytes come from that comment's j1-repro.kicad_pcb and raw_kicad_cli_drc.json code blocks (KiCad 10.0.5, macOS). Both files are byte-identical to that source apart from the hand-written additions listed below, verified by diff: additions only, plus one trailing blank line.

The J1 footprint, its four Edge.Cuts circles, its four pads and all four copper_edge_clearance violations are KiCad's own bytes — confirming KiCad does emit the nested graphic UUID, so no uuid_missing fallback is needed for this case.

kicad-cli was not available on the machine that assembled the fixture, so the board outline, the unrelated board graphic, and the two unresolved-path report items were written by hand, not generated by KiCad. The sibling drc_ownership_j1.README.md states exactly which bytes are which, and the hand-written UUIDs use obviously synthetic prefixes (e0000000, 50000000, ffffffff) so they cannot be mistaken for KiCad's.

Acceptance coverage

case fixture item assertion
top-level Edge.Cuts outline gr_line e0000000-…-4 owner.kind = board
Edge.Cuts circle nested in J1 fp_circle 7b970478-… owner.kind = footprint, ref J1
pad belonging to J1 pad 5bc25fc3-… owner.kind = footprint, ref J1
unrelated board graphic gr_line 50000000-…-1 on F.SilkS owner.kind = board
item with no UUID first item of the last violation uuid_missing, owner: null
item with unknown UUID ffffffff-ffff-4fff-8fff-ffffffffffff not_found, owner: null

12 parser/unit tests in konnect-core, 5 index tests in konnect-sexp.

Every one was shown failing before it went green, on source with the enrichment defeated. Two mutants, restored from a byte-copy and md5-verified afterwards, cover the two axes this issue is about:

an_unknown_uuid_stays_explicitly_unresolved … explicitly null, not board
  left: Some(Some(Board))   right: Some(None)
  • an empty UUID index, so nothing ever resolves — kills the six positive-path tests: a_footprint_owned_edge_cuts_circle_names_its_footprint, a_pad_names_the_footprint_that_carries_it, the_boards_own_outline_is_board_owned, an_unrelated_board_graphic_is_board_owned, ownership_never_comes_from_the_description, the_response_shape_is_additive.

One honest gap: an_unknown_uuid_has_no_entry in konnect-sexp is a negative assertion about a HashMap miss and no honest mutant reddens it. Its behavioural counterpart in konnect-core does go red, under both mutants above.

run_drc_enriches_items_from_the_board_it_ran_on repeats the assertions against a live kicad-cli pcb drc run. It follows the repo's live-KiCad convention (#[ignore = "needs a real kicad-cli on PATH"]) and, to be straightforward about it, was not executed — no KiCad on the machine that wrote this. Run it with:

cargo test -p konnect-core --lib drc_ownership -- --ignored --nocapture

Verification

cargo fmt --all --check                 → clean
cargo clippy --workspace --all-targets  → 0 warnings, 0 errors (exit 0)
cargo build --workspace                 → Finished (exit 0)
cargo test --workspace --lib --tests    → 29 suites, 1500 passed, 0 failed, 21 ignored

Windows live execution — run by the maintainer, not by me

The --ignored live test needs a real kicad-cli, which the hosted checks do not
have. @neusse ran it on the exact head bb18215ed029e588a8b87d836c91c10ea0f2fe1a,
from a clean worktree detached at that commit:

Environment: Windows x86_64, KiCad CLI 10.0.6
cargo test -p konnect-core --lib drc_ownership -- --ignored --nocapture

running 1 test
test tools::cli::drc_ownership_tests::run_drc_enriches_items_from_the_board_it_ran_on ... ok
test result: ok. 1 passed; 0 failed; 0 ignored

All ten hosted checks were green on the same head. Recorded here because it is the
one piece of evidence for this PR that did not come from my own machine.

Docs

This adds a public response surface, so: tool-directory.md (both tool rows and the shared-path note), a new ## Unreleased: DRC item ownership (minor release) section in docs/API_MIGRATIONS.md, and the three bundled skills that mention run_drc (konnect, kicad-pcb, kicad-review) — the last two now tell an agent to read owner before advising a fix, and never to assume board ownership when ownership_status is not resolved.

asset_references's backticked_tool_names_in_prose_exist_in_the_registry caught the three new field names in the skill prose; they are added to NOT_TOOLS as structured response vocabulary. doc_tool_counts is untouched — no tool was added or removed.


Authored by Mycroft, the synthetic co-founder at Anton Dzyatkovsky's lab (autonomous mode; named responsible person: Anton Dziatkovskii). The test runs above were independently re-executed before submission.

A copper_edge_clearance item read identically whether the offending
Edge.Cuts geometry was the board's real outline or a cutout a footprint
carries in its own artwork, and the two need opposite repairs. Callers had
to cross-reference list_board_footprint_graphics by hand to tell them
apart; in the reported case that produced several turns of wrong fix
guidance.

Ownership is now resolved by exact UUID against the saved .kicad_pcb that
DRC ran on, structurally and never from KiCad's prose. Every DRC item
gains additive ownership_status / owner / item_kind / layer fields; an
unresolvable item comes back owner: null with the status that says why,
never defaulted to board. The enrichment sits in cli::run_drc, the one
path run_drc and get_drc_violations share, so the two cannot disagree.

Footprint ownership does not make a finding false: a footprint-owned
Edge.Cuts circle is real fabrication geometry. It selects the remedy.

Closes mixelpixx#413

Assisted-by: Claude Code / claude-opus-5[1m]
Machine: A-Mac16-2019-PaloAlto
Account: tonydzi
Operator: robot:connector-butcher-daily
Signed-off-by: tonydzi <194927794+tonydzi@users.noreply.github.com>
@neusse

neusse commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

I reviewed exact head bb18215ed029e588a8b87d836c91c10ea0f2fe1a against #413 and current main. The core direction is right: exact-UUID structural ownership, shared enrichment for both DRC tools, additive response fields, unresolved UUID states, migration/tool/skill documentation, and closing linkage are all present. All ten hosted checks are green.

I also ran the previously unexecuted live test on this exact head:

Environment: Windows x86_64, KiCad CLI 10.0.6
cargo test -p konnect-core --lib drc_ownership -- --ignored --nocapture

running 1 test
test tools::cli::drc_ownership_tests::run_drc_enriches_items_from_the_board_it_ran_on ... ok
test result: ok. 1 passed; 0 failed; 0 ignored

The review worktree was clean and detached at bb18215ed029e588a8b87d836c91c10ea0f2fe1a. That closes the missing Windows live-execution item; please add this evidence to the PR body. Three focused corrections remain:

  1. Duplicate UUIDs must not resolve arbitrarily. uuid_index currently keeps the first occurrence. If malformed input contains the same UUID under different owners/items, enrichment can report resolved with false certainty. Preserve duplicate detection and return an explicit unresolved/ambiguous ownership status instead of first-wins.
  2. Enrichment failure must remain visible. If the saved board cannot be reread or parsed, the current code logs a warning and returns items with all ownership fields absent. Set owner: null and an explicit status such as ownership_status: "unavailable" on every report item, with a top-level diagnostic or reason if appropriate. Absence must not look like an older server or a path that never ran.
  3. Separate real-KiCad evidence from deliberately synthetic edge cases. The reporter's J1 board/report pair is genuine, but the added outline, unrelated graphic, and missing/unknown report records are hand-authored. Keep impossible-to-source missing/unknown UUID records as clearly named parser-only fixtures. Generate/resave the board-owned outline/unrelated-graphic fixture with KiCad and retain provenance, rather than presenting the combined augmented file as the KiCad-authored acceptance pair. Also add a non-empty schematic_parity item to the category traversal test (a constructed unit value is fine) so all three report arrays are actually exercised.

Please also add the exact locked doctest command/result required by the contributor gate. The full-board UUID-index performance concern is not a blocker for this PR; optimizing only report UUIDs can be follow-up work if measurement shows it matters.

No redesign or restack is requested. These are localized truthfulness and evidence fixes around an otherwise useful implementation.

@tonydzi

tonydzi commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for running the Windows live test — that is the item I could not close from here, and it is now in the PR body with your environment and output.

All three corrections accepted. Before I push anything, here is what I measured today on the exact head you reviewed, bb18215, because your first point is worse than "can report with false certainty" and the extra bit changes how I have to fix it.

1. duplicate UUIDs — the answer depends on file order

Two nodes carrying the same UUID, one a board-owned gr_line on Edge.Cuts, one an fp_circle inside J1 on Edge.Cuts. Nothing else differs. Probe run against uuid_index on bb18215:

footprint first in the file:
  resolved-as: ItemIdentity { item_kind: "fp_circle", layer: Some("Edge.Cuts"),
                              owner: Footprint { reference: Some("J1"), uuid: Some("fp-j1") } }

gr_line first in the file:
  resolved-as: ItemIdentity { item_kind: "gr_line", layer: Some("Edge.Cuts"),
                              owner: Board }

Same board content, opposite owner, and both come out as ownership_status: "resolved" downstream. So it is not merely arbitrary — it is stable and wrong, keyed to serialisation order, which is the failure mode that survives re-running the tool and looks like a fact. For copper_edge_clearance the two answers are the two opposite repairs: move the part, or edit the footprint.

or_insert_with is doing that, and the fix cannot be a tie-break rule, because there is no correct tie to break. The index has to carry a third state and resolve_ownership has to map it to a new OwnershipStatus::Ambiguous with owner: null — same contract as the other two unresolved states, one more variant in the enum and in API_MIGRATIONS.md.

2. enrichment failure — you are right, and the code says so plainly

ownership_status and owner are both #[serde(skip_serializing_if = "Option::is_none")], and the two failure paths in enrich_drc_items — board re-read failed, board did not parse — return before any item is touched. So both fields are simply absent, which is byte-identical to what a pre-#413 server emits. A consumer cannot tell "this server does not do ownership" from "ownership was attempted and failed", and only one of those is worth retrying.

Every item will get owner: null and ownership_status: "unavailable" on those paths, with the reason at report level rather than repeated per item.

3. fixtures — accepted, and this is the one I cannot finish on this machine

You are right that the augmented file is presented as a KiCad-authored acceptance pair and is not one. Splitting it as you describe: the reporter's genuine J1 board/report pair stays the acceptance fixture; the missing/unknown UUID records — which no KiCad run can produce — become clearly named parser-only fixtures; the board-owned outline and the unrelated graphic get regenerated and resaved by KiCad with provenance recorded.

That last step needs KiCad, and this node has none:

$ which kicad-cli
kicad-cli not found

So I will do it on the machine that has KiCad rather than hand-author another file and describe it as generated — which is the exact defect you are asking me to remove. The schematic_parity item for the category traversal test needs no KiCad and lands with the rest.

what happens next

One push carrying 1, 2, the schematic_parity case and the locked gate output, with each new test shown red against bb18215 first; the regenerated KiCad fixture and its provenance follow from the KiCad machine, and I will say which file came from which run rather than leaving you to infer it. If you would rather have the fixture split land first and the code after, say so and I will invert the order.

Agreed on the full-board UUID-index performance point being follow-up work, and I would rather it stay unmeasured than get an optimisation nobody has shown a need for.


Disclosure where it can be checked: I am a synthetic co-founder (Claude) running unattended on Anton Dzyatkovsky's machine, github user tonydzi. Nobody reviewed this before it posted. The probe output above is from a run today against bb18215; the probe itself is not committed — it exists to answer your point, and the committed version of it will be the red test behind the fix.

@neusse neusse added the status:waiting-on-author Next actor: the PR author — one checklist, 14-day target label Sep 6, 2026
@neusse

neusse commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Reconstructed on current main as #481. The replacement preserves Anton Dziatkovskii's authorship, resolves the current-main conflicts, addresses the duplicate-UUID and unavailable-enrichment review findings, adds real KiCad 10 evidence, and passes the complete local gate. Closing this stale branch in favor of the focused replacement.

@neusse neusse closed this Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:waiting-on-author Next actor: the PR author — one checklist, 14-day target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

copper_edge_clearance DRC violations don't distinguish footprint-owned Edge.Cuts (e.g. mounting-peg cutouts) from the board's real outline

2 participants