fix(pcb): read layers and netclasses from the live board - #656
pauliuszaleckas wants to merge 1 commit into
Conversation
mixelpixx
left a comment
There was a problem hiding this comment.
Reviewed exact head bf9b8b8db87cad89a4324abb649f30378848520f: one commit on current main (c5b674e), all ten required checks green on it, no review threads. This is careful work and the seam is the right shape. One finding blocks, and it only showed up against a real KiCad, which the body says was not available to you, so I ran that half here.
What I verified
Local gate on the head (Windows 11): cargo fmt --all -- --check, cargo clippy --workspace --locked --all-targets -- -D warnings, cargo test --workspace --locked --lib --tests 2026 passed / 0 failed, --doc, cargo xtask fix-doc-counts --check unchanged, reliability_contract green. Exit codes captured directly.
The write gates, arm by arm against main. attempt_ipc_write and refuse_if_board_open_in_kicad reach the same verdict from the same evidence and emit the same text: rejected → same refusal / same None; proves-not-open → file edit unless observed live; other target errors → the typed refusal; unreachable → lock first, then the observed-live veto, then the file. Session memory is still sampled after the call. with_board_ipc_classified now delegates to the bound variant, and ensure_board_is_active was already find_open_board(..).map(|_| ()), so targeting is unchanged. I agree with "no mutation path's behaviour changed".
Tests and fixture. The contract tests drive served tools/call through McpHandler::handle_message, as #574 asks; the fixture is kicad-cli pcb upgrade output with oracles that read no Konnect code.
One negative control of my own, independent of your table: under auto, make a rejection after identification answer from the saved file. Three served tests fail (a_failure_after_identification_is_not_answered_from_the_file, netclasses_refuse_after_a_failed_live_query, an_empty_enabled_layer_reply_is_not_reported_as_a_live_stackup); it compiled, and the tree was restored byte-identical.
Live evidence (KiCad 10.0.5, Windows 11, release build of this head over stdio)
A standalone pcbnew holding a copy of the ecc83 demo, with the saved file changed behind the editor afterwards (F.Cu's user name top_cu → SAVED_ONLY_top on disk only), so the two sources cannot agree by accident:
| State | Call | Result |
|---|---|---|
| pcbnew holds the board | default / auto / live |
F.Cu → top_cu (the editor's), sources: enabled_layers / layer_names / copper_layer_count ipc, layer_ids / layer_types saved_board; board_state: ipc |
| same | saved |
F.Cu → SAVED_ONLY_top; reason: explicitly_requested_after_live_observation |
| same | get_netclasses auto / saved |
board_nets: ipc / saved_board; definitions and patterns project_file both times |
| same | live, a different board |
wrong_document, naming the board pcbnew does hold |
| same | auto, a different board |
saved file, reason: board_not_open_in_kicad |
| pcbnew killed mid-session (lock file left behind) | auto and live |
unsafe_file_fallback, reason: board_previously_observed_live |
| same | saved |
saved file, explicitly_requested_after_live_observation |
Every claim in the body that I could exercise holds on the real editor.
Blocking: with only the KiCad project manager running, the default call now refuses
Start KiCad and do not open the PCB editor. That is the state every user is in between launching KiCad and opening a board. On main both tools answer from the file there. On this head:
get_layer_list {board} (default, and board_source: "auto")
→ isError, kind: editor_unavailable
"KiCad did not complete the layer list for this board: KiCad IPC error: no handler
available for request of type kiapi.common.commands.GetOpenDocuments (AS_UNHANDLED).
Konnect did not substitute the saved board file, which may be older than the
editor's state. …"
get_netclasses {board} → the same refusal
get_layer_list {board, board_source: "saved"} → answers, reason: explicitly_requested
There is no editor whose state the file could be older than. find_open_board fails inside GetOpenDocuments itself, IpcFailure::from_error classifies any status error as Rejected, and read_board refuses every Rejected. But #574's rule is scoped: "Once the exact board has been positively identified as live, a failed live query must not silently become a successful saved-file answer." Here nothing was identified; KiCad said it has no handler for board documents at this endpoint. LiveBoard::Rejected's own doc ("It may hold the board") is true after identification and not before.
Requested change, kept inside this PR's scope:
- Distinguish a rejection before identification from one after. The status is already modelled:
ApiStatusError::is_unsupported()(AS_UNHANDLED/AS_UNIMPLEMENTED), used the same way ineditor_navigation.rs. WhenGetOpenDocumentsis refused with it,observereports its own verdict (sayNoBoardEditor). read_board: underauto, treat it likeNeverReached— probe the sibling lock, read the saved file, and disclose with its own machine-readable reason (for exampleno_pcb_editor_at_endpoint, plus the…_with_editor_lock/…_with_uninspectable_lockforms you already have). Underlive, refuse as now. If this session observed the board live earlier, it staysLostAfterObservation. A standalone pcbnew on its own endpoint is invisible from here, which is equally true ofNeverReached; the lock probe is the evidence for both.- A rejection after identification keeps refusing, whatever its status. Control C stays.
- The write gates map the new verdict exactly as they map
Rejectedtoday, so no mutation behaviour changes here. Today that status makesattempt_ipc_writerefuse whilerefuse_if_board_open_in_kicadproceeds; that inconsistency predates you and belongs to #577. A one-line comment at theRejected | NotOpen => Nonearm saying it is preserved, not decided, would stop the next reader taking it for policy. - Served regressions with a double that answers
GetOpenDocumentswithAS_UNHANDLED:autoanswers from the file with the new reason;liverefuses; observed-live-then-unhandled refuses; the existing write-gate tests unchanged. Plus a negative control for the new arm. - Migration entry and TROUBLESHOOTING: one sentence for this state.
Non-blocking
- The migration heading has no release class. Neighbouring entries carry one; this is a minor (additive argument, new response fields, a former answer that can now refuse).
- On Windows the
wrong_documentmessage prints the open board with a\\?\prefix. Cosmetic, probably older than this PR; worth a follow-up issue, not a change here. - "Known remaining" is right, and I would put it first in #574 stage 4: until
get_board_info,get_board_extentsandget_component_padsmove, one session can get a refusal fromget_layer_listand a cheerful"source": "file"fromget_board_infoabout the same board.
Closure accounting is correct as written: Closes #542, Part of #574. Reply with the new head when the change is in and I will re-run the gate and the three live states on it.
bf9b8b8 to
274cab9
Compare
|
Thank you — the blocking finding is correct, and it is a state I had no way to What changedWhere the classification happens. // tools/mod.rs, inside with_bound_board_ipc_classified
let document = match client.find_open_board(&requested) {
Ok(document) => document,
Err(error)
if konnect_ipc::ApiStatusError::from_error(&error)
.is_some_and(|status| status.is_unsupported()) =>
{
return Ok(BoardBinding::NoBoardEditor(format!("{error:#}")));
}
Err(error) => return Err(error),
};
memory.observe_live(&observation);That scopes it exactly as you asked: only the identification step can produce
Write gates. Both map the new verdict exactly as they mapped this status Read path. Under New tests (served
|
| Case | Test |
|---|---|
no PCB editor, default and auto, both tools |
a_kicad_with_no_board_editor_still_answers_by_default |
no PCB editor, live |
a_kicad_with_no_board_editor_refuses_a_live_request |
| observed live, then the editor closes under the session | a_board_seen_live_before_the_editor_closed_still_refuses |
The third one needed MockIpcServer::spawn_at, so the project manager can take
over the same endpoint after the PCB editor's guard is dropped. A replacement
handler would have had a fresh BoardSessionMemory and the test would have
passed for the wrong reason.
New negative controls
| # | Neutered | Failures | Which |
|---|---|---|---|
| N | NoBoardEditor refuses under auto again (the reported regression) |
1 | a_kicad_with_no_board_editor_still_answers_by_default |
| O | the observed-live exception is dropped | 1 | a_board_seen_live_before_the_editor_closed_still_refuses |
Both compiled, both restored byte-identical.
Non-blocking, addressed
- Migration heading now carries (minor release), and the entry covers this
state — including that mutating tools are unchanged by it. TROUBLESHOOTING.mdnow says up front that a KiCad with no PCB editor is
not theunsafe_file_fallbackstate, so the refusal section is not read as
covering it.
Non-blocking, not done here
- The Windows
\\?\prefix on thewrong_documentmessage — agreed it predates
this PR. Say the word and I will open an issue with your repro rather than
widen this branch. - "Known remaining" first in Define one authoritative board-state source contract for PCB reads, analysis, and CLI workflows #574 stage 4 — agreed; I will leave that note on
Define one authoritative board-state source contract for PCB reads, analysis, and CLI workflows #574 rather than here.
Validation on 274cab9
Fedora Linux, Rust 1.96.0:
cargo test --workspace --locked --lib --tests # ok, 0 failed (1431 in konnect-core lib)
cargo test --workspace --locked --doc # ok, 0 failed
cargo clippy --workspace --locked --all-targets -- -D warnings # clean
cargo fmt --all -- --check # clean
cd crates/schematic-viewer && cargo test # ok, 20 passed
Still no live KiCad here, so the three states you ran remain your measurement,
not mine — the new ones are covered by a double that answers AS_UNHANDLED.
get_layer_list and get_netclasses parsed the saved .kicad_pcb unconditionally, so a board open in KiCad with unsaved changes was answered from the file on disk and nothing said so. Both now go through one shared read seam that takes an additive board_source selector — auto (default), live, saved — and reports per-domain sources plus source_evidence. Layer types and ids have no KiCad IPC equivalent and stay file-backed; netclass definitions and patterns stay project-file facts. Both are named as such rather than folded into one source string. The reading of KiCad's answer moves to live_board.rs, shared with the write gates it used to be transcribed alongside. That is where the mixelpixx#240 rule now lives: a board this process watched KiCad hold is never treated as safely absent afterwards, however KiCad stops answering. The policies still differ — a stale read is disclosable, a stale write destroys work — but the evidence is read once. Callers receive the document KiCad resolved rather than asking for it again. An endpoint that refuses GetOpenDocuments itself is classified where the typed status still exists, as its own verdict: KiCad running with no PCB editor never identified a board, so a read answers from the file and says so rather than refusing. Mutating tools map that verdict exactly as they mapped it before it had a name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
274cab9 to
bc1bdca
Compare
|
Correction to the head SHA above: the branch was rebased onto The rebase carries no code change. Everything else in my reply above stands as written against that head. |
|
The requested correction is now present on exact head @mixelpixx, please re-review this exact head and replace or dismiss the earlier |
get_layer_listandget_netclassesparsed the saved.kicad_pcbunconditionally. A board open in KiCad with unsaved changes was answered from
the file on disk, and
get_layer_listsaid nothing about it.Both now go through one shared board-read seam,
crates/konnect-core/src/tools/board_source.rs, which owns exact-boardtargeting, the staleness policy, the provenance vocabulary and the structured
refusals — rather than two handler-local fallbacks. Reading KiCad's answer
itself lives one level down in
live_board.rs, shared with the write gates(see Shared classification below).
Closes #542
Part of #574
The selector
Both tools take an additive
board_source, defaulting toauto, so noexisting caller changes:
auto(default)livesavedNon-negotiables from #574, implemented in the seam:
returned as a failure. It never becomes saved-file success under
autoorlive.is refused under
auto(unsafe_file_fallback), by the same Closed-board fallback cannot tell "KiCAD was never running" from "KiCAD just died holding this board" #240 rule thatprotects writes. An explicit
savedrequest may still inspect that snapshotand reports
explicitly_requested_after_live_observation.livereturnswrong_documentnaming the boards KiCad does hold,autoreads the file and says
board_not_open_in_kicad.Per-domain provenance
Neither answer is single-source, so neither reports one source string.
get_layer_listobtains the enabled set and each layer's displayed name fromGetBoardEnabledLayers/GetBoardLayerName. A layer's ordinalidand itssignal/power/mixed/usertype have no IPC equivalent, so they are joinedfrom the saved
(layers …)table by canonical name and reported asfile-backed —
nullfor a layer the editor has enabled that the file does notcarry.
get_netclassesstays inherently mixed:Its existing
nets_sourcestring is kept (public API) and now also covers thelive case.
Shared classification
The write side already reads KiCad's answer in
attempt_ipc_writeandrefuse_if_board_open_in_kicad. Adding a third transcription of that ladderfor reads would have put the rule with the worst failure mode in this codebase
— a board observed live is never treated as safely absent afterwards (#240) —
in four independent copies, two of which the reader would have no structural
reason to look at.
So
tools/live_board.rsowns the reading, and the three gates map its verdictonto their own policy. The policies stay different on purpose: on an
unreachable IPC with a sibling lock present, a write refuses and a read
discloses and proceeds, because a stale read is disclosable and a stale
write destroys work. What is now single is the evidence, including the lock
probe — whose three answers (absent / present / uninspectable) the read path
had flattened to two, reading an inspection failure as an absence that the
write path is documented never to infer.
No response text changed on the write paths; their existing tests pin that.
The negative-control table below shows the payoff: neutering the #240 veto once
now fails six tests across reads and writes.
read_boardalso hands its closure theDocumentSpecifierKiCad resolved,rather than leaving each handler to call
find_open_boardagain. That removesa redundant
GetOpenDocumentsper call, and makes it impossible for a #574consumer to reach for a document-less client method —
get_nets()rather thanget_nets_in(document)— and silently answer about whichever board KiCadopened first.
Deviations from the issue
idis file-backed, not live. KiCad's IPC API identifies alayer by its
BoardLayerenum value (BL_F_Cu= 3); theidthis tool hasalways reported is the file's stackup ordinal (
F.Cu= 0). Those aredifferent numbering schemes, and
add_layerallocates in the file's. Puttingthe enum value under the existing key would silently repurpose it, so
idisjoined from the saved table by canonical name and labelled
saved_board,nullwhen the file has no such layer.~<name>.kicad_pcb.lckexists,autostill answers from the file butreports
kicad_ipc_unreachable_with_editor_lockinstead ofkicad_ipc_unreachable. Reads are non-destructive and the limitation isdisclosed; the write-side veto in
pcb_board.rsis untouched.BoardAccessclassification. Both tools now declareLivePreferredWithFallback, which is what they do. That adds them to thepre-pcb-fallbackhook skill's tool list, whose text is worded for edits;the executable read-source inventory is Define one authoritative board-state source contract for PCB reads, analysis, and CLI workflows #574 stage 1, not this PR.
Fixture and oracles
crates/konnect-core/tests/fixtures/board_source_divergence_kicad10.kicad_pcb— built from
specctra_two_resistors.kicad_pcb, then re-serialized byKiCad 10.0.6 with
kicad-cli pcb upgrade --force, so the committed bytesare pcbnew's own output. Full provenance and case table in its
README.md.Its saved state shares no value with the KiCad double the tests run against, in
either direction, so a test cannot pass while reading the wrong adapter:
In1.Cu/In2.CuF.Cudisplayed asF.CuLiveRenamedTopSavedOnlyFabNameSAVED_ONLY_A,SAVED_ONLY_BLIVE_ONLY_P,LIVE_ONLY_QExternal oracles for the saved half, all produced by
kicad-clifrom thecommitted file and none of them reading Konnect code:
kicad-cli pcb export gerbers…-SavedOnlyFabName.gbr…-job.gbrjob"GeneralSpecs": { "LayerNumber": 4 }and fourCopper,L1…L4fileskicad-cli pcb export ipc2581F.Cu/In1.Cu/In2.Cu/B.CuasCONDUCTORin stackup orderkicad-cli pcb export ipcd356SAVED_ONLY_AandSAVED_ONLY_BNeither export encodes a copper layer's
signal/power/mixedkind, which iswhy that attribute is reported as file-backed even in a live answer. Every
oracle value is restated as a literal
constin the test module, with acomment saying it is the promise — none is imported from the implementation or
re-derived from the fixture at test time.
Tests
14 new contract tests in
crates/konnect-core/src/tools/board_source_contract_tests.rs, all driventhrough served
tools/call(handle_messagewith a JSON-RPCtools/callrequest), so the advertised schema, the dispatch gate and the response body are
covered — not just the handler bodies. Plus 5 unit tests on the selector in
board_source.rsand 4 inlive_board.rs, plus 2 on the write gates' lockprecedence in
pcb_board.rs.auto_answers_the_live_stackup_over_a_divergent_saved_onea_live_answer_reports_its_file_backed_fields_as_file_backedsaved_inspects_the_file_even_while_kicad_holds_the_boarda_kicad_holding_another_board_refuses_live_and_explains_autolive_refuses_when_no_kicad_is_reachablea_failure_after_identification_is_not_answered_from_the_filea_board_observed_live_then_lost_refuses_auto_and_discloses_under_savednetclasses_report_live_nets_beside_project_file_definitionsnetclasses_saved_mode_matches_the_saved_nets_and_discloses_itnetclasses_refuse_after_a_failed_live_queryan_unknown_board_source_is_refused_by_nameomitting_the_selector_reads_the_saved_board_and_says_solive_board::…::a_transport_that_dies_after_identification_is_a_loss_not_a_cold_starta_declined_layer_name_does_not_fail_the_live_readan_empty_enabled_layer_reply_is_not_reported_as_a_live_stackupboard_session_safety_tests::an_exact_board_lock_still_outranks_the_previously_live_verdictNegative controls
Each guard neutered in turn,
cargo test -p konnect-core --librun, thenrestored. Baseline 1428 passing.
a_board_observed_live_then_lost_…+board_session_safety_tests::{a_board_closed_since_konnect_saw_it_live_still_refuses_the_file, a_file_only_guard_blocks_a_previously_live_board_after_transport_loss, an_operation_rejected_after_identification_still_protects_the_next_call, live_then_dead_blocks_a_file_fallback_and_preserves_the_board}+pcb_components::tests::a_previously_live_board_blocks_place_components_file_fallbacklive_board::tests::an_unreadable_lock_is_not_an_absent_one,board_session_safety_tests::an_uninspectable_exact_board_lock_fails_closed_with_distinct_evidenceRejectedreturnsSavedinstead ofRefuseda_failure_after_identification_is_not_answered_from_the_file,netclasses_refuse_after_a_failed_live_querylivestops requiring a live boarda_kicad_holding_another_board_refuses_live_and_explains_auto,live_refuses_when_no_kicad_is_reachablesavedearly return removedsaved_inspects_…,netclasses_saved_mode_…,a_board_observed_live_then_lost_…auto, schemaenumdroppedan_unknown_board_source_is_refused_by_name,an_unknown_or_mistyped_mode_is_an_argument_error,the_advertised_schema_lists_the_modes_the_reader_acceptsget_layer_listhardcoded toBoardSource::Saveda_board_observed_live_then_lost_…,a_failure_after_identification_…,a_kicad_holding_another_board_…,auto_answers_the_live_stackup_…,live_refuses_when_no_kicad_…,omitting_the_selector_…get_netclasseshardcoded toBoardSource::Savednetclasses_refuse_after_a_failed_live_query,netclasses_report_live_nets_beside_project_file_definitionsipca_live_answer_reports_its_file_backed_fields_as_file_backeda_transport_that_dies_after_identification_is_a_loss_not_a_cold_startan_exact_board_lock_still_outranks_the_previously_live_verdicta_declined_layer_name_does_not_fail_the_live_readan_empty_enabled_layer_reply_is_not_reported_as_a_live_stackupControl A is the one worth reading twice: it is the whole argument for sharing
the classification. Before this PR the same edit had to be made in four places
to be complete, and any one of them could be missed silently.
Every control was a deterministic source edit, not an iteration-order change,
and the tree was restored to a byte-identical state (
cargo fmt --checkclean,grepfor the control markers empty) before committing.Validation
Run on
bf9b8b8, Fedora Linux, Rust 1.96.0, KiCad 10.0.6:Not run: the real-KiCad e2e suite (
e2e-kicad.yml,#[ignore]d live tests) —no KiCad IPC session was available here, so the live half is covered by the
deterministic double rather than by an unsaved-board observation, as the issue
allows.
Risk and rollback
No mutation path's behaviour changed.
attempt_ipc_writeandrefuse_if_board_open_in_kicadwere rewritten onto the shared classificationbut reach the same verdict from the same evidence and emit the same text —
their existing tests pin that, and controls A and B above show both gates
still fail closed. No file mutation became easier to perform behind KiCad.
The behaviour change a caller can see is that both tools may now refuse where
they previously returned a saved-file answer — when KiCad holds the board and
rejects the query, or when an observed-live editor has vanished. Both refusals
name
board_source: "saved"as the way to inspect the snapshot deliberately.Rollback is the single commit; passing
board_source: "saved"restores theprevious answer exactly.
docs/API_MIGRATIONS.md
One new entry, inserted above the existing ones. The diff against
mainis43 added lines and 0 deleted: nothing already on
mainis moved or removed.docs/TROUBLESHOOTING.mdand thekicad-pcbskill asset each gain a shortparagraph, because
unsafe_file_fallbackcan now reach a caller who was notwriting anything and both documents previously said it always meant a refused
mutation.
Review round
A self-review of the first draft found four defects in it, all fixed above and
each now pinned by a test and a negative control (J–M):
pre-refactor code read it after; the draft read it before. KiCad can
identify the board and then go away before the next command — and since each
command dials its own socket and
get_layer_listissues one per layer, thatwindow is ~30 dials wide. The draft classified that as a cold start and
answered from the file for a board KiCad had just proven it was holding,
which is the exact thing this PR exists to prevent, and a regression of Closed-board fallback cannot tell "KiCAD was never running" from "KiCAD just died holding this board" #240
on the write gates too.
arms into
LostAfterObservationdropped the lock check, changing a stablemachine-readable
reasonfromkicad_lock_presenttoboard_previously_observed_live. The variant now carries whether thetransport is what went away, which is the only case where a lock is the
better evidence.
a saved-file fallback is forbidden — so a single cosmetic lookup failure
turned a tool that always answered into one that refused. KiCad-level
refusals are now best-effort per layer (
display_namestays unset, thecanonical name stands in); a transport loss still propagates, because a
partial read must not be dressed up as a complete live answer.
Known remaining, for #574
get_board_info,get_board_extentsandget_component_padsare IPC-firstreaders in these same two files that still swallow every
IpcFailureandanswer from the file. They are not converted here — this PR is the first
focused consumer, not the migration — but they are the nearest holdouts, and
until they move a session can get a refusal from
get_layer_listand acheerful
"source": "file"fromget_board_infoabout the same board. Worthan early slot in stage 4.
🤖 Generated with Claude Code