25 KiB

Raw Permalink Blame History

Library Assessment Report

Date: 2026-03-29
Repository: kicad-ipc-rs
Branch assessed: chore/kicad-v10-stable-protos
Related PR: #23 (feat: bump vendored KiCad protos to v10.0.0 + follow-up tests)

Purpose

This report evaluates the library's structure, correctness, and practical usefulness using a layered review sequence inspired by:

m15-anti-pattern (idiomatic/code-smell review)
plan-eng-review (architecture/maintainability review)
review (change-level correctness review)
document-release (docs + release/readiness review)

No fixes were applied while producing this report.

Scope and Method

In-scope

Core crate code under src/
Public docs and usage docs:
- README.md
- docs/book/src/*.md
Current PR diff vs origin/main
Test/lint/validation signals from local command runs

Out-of-scope

KiCad upstream implementation internals beyond proto surface validation
Runtime behavior on all KiCad OS/channel combinations
Performance benchmarking

Evidence-gathering commands run

Test inventory and counts (rg on #[test] and #[tokio::test])
LOC and public API counts (wc -l, rg -n "pub async fn", rg -n "pub ")
Diff/PR scope checks (git diff origin/main...HEAD)
Validation checks:
- cargo test -q
- cargo test --features blocking -q
- cargo clippy --all-targets --all-features -- -D warnings

Executive Summary

Overall status: Good functional correctness with strong protocol-awareness, moderate maintainability risk, and moderate documentation/discoverability risk.

Correctness: Strong for current scope and PR #23 changes.
Architecture: Solid layering with a recently modularized client architecture (formerly monolithic client.rs).
API usefulness: Good typed surface, with intentional raw escape hatches.
Release/docs hygiene: Version-snippet drift, cross-link integrity, onboarding examples, and prerequisites have all been addressed. Remaining weakness is deeper rustdoc method-level coverage.
Test posture: Healthy and improving; protocol-contract tests were a high-value addition.

Baseline Metrics

Code footprint

Full non-generated source tree (LOC)

src/client/mod.rs: 230
src/client/common.rs: 354
src/client/board.rs: 360
src/client/items.rs: 426
src/client/selection.rs: 181
src/client/document.rs: 191
src/client/geometry.rs: 241
src/client/mappers.rs: 1148
src/client/decode.rs: 549
src/client/format.rs: 349
src/client/tests.rs: 1230
src/model/board.rs: 746
src/blocking.rs: 667
src/model/common.rs: 550
src/transport.rs: 126
src/lib.rs: 112
src/envelope.rs: 110
src/error.rs: 90
src/proto/mod.rs: 38
src/commands/mod.rs: 4
src/model/mod.rs: 2
src/kicad_api_version.rs: 2
src/commands/project.rs: 2
src/commands/editor.rs: 2
src/commands/board.rs: 2
src/commands/base.rs: 2

Non-generated total: 7714 LOC
Generated proto total: 4863 LOC
Overall total: 12577 LOC

Interpretation: Formerly concentrated in a single 5448-LOC file, the client module has been split into 11 focused domain modules.

Dependency and module surface snapshot

Direct dependencies: 6 (nng, prost, prost-types, thiserror, tokio, tracing)
Public modules exported from lib.rs: 7
- client
- commands
- envelope
- error
- model
- transport
- blocking (feature-gated)
Total public API items (excluding generated code): 139

Interpretation: API breadth is substantial relative to crate size; discoverability and consistency controls are important.

API surface size

Public async methods in KiCadClient: 107
Public blocking methods in KiCadClientBlocking: 26 explicitly counted wrappers (plus macro-generated parity set)

Interpretation: This is a broad API surface for a single crate module; drift control and discoverability are key concerns.

Test inventory (unit tests in `src/`)

src/client/tests.rs: 65
src/blocking.rs: 7
src/model/common.rs: 6
src/model/board.rs: 4
src/envelope.rs: 2

Total identified unit tests: 84

Interpretation: Coverage appears substantial for a library of this size, with strongest emphasis on client behavior and mapping.

Pass 1: `m15-anti-pattern` Findings

What looks good

No unsafe usage in src/, reducing a major class of memory-safety risk.
Error mapping is explicit and specific in src/error.rs.

Verified clean signals

Zero .unwrap() calls in production code (confirmed via exhaustive grep).
All 56 .expect() calls are in test code only (zero in production paths).
Zero production panic! usage (panic usage is confined to test modules under src/client/tests.rs).
No TODO/FIXME/HACK/XXX markers in non-generated source.

Notable anti-pattern/style findings

AP-1: `clone_on_copy` in production mapping path

Evidence: previously observed in client mapping code before modularization.
Impact: Low runtime impact, but signals unnecessary ownership noise and can obscure intent.
Severity: Low
Status: RESOLVED (commit 5d3bb4b)

AP-2: Strict clippy fails on generated protobuf enums

Evidence:
- src/proto/generated/kiapi.common.rs:56
- src/proto/generated/kiapi.common.commands.rs:424, :459, :490 (and similar)
Lint class: clippy::enum_variant_names under -D warnings
Impact: High CI/tooling friction if strict clippy is expected to pass globally.
Severity: Medium (process/tooling risk)
Status: RESOLVED — added targeted #[allow(clippy::enum_variant_names)] in src/proto/mod.rs (commit 5d3bb4b)

AP-3: Test-style bool asserts flagged

Evidence: client unit tests under src/client/tests.rs.
Impact: Low; style-level issue only.
Severity: Low
Status: RESOLVED (commit 5d3bb4b)

AP-4: Heavy repeated RPC boilerplate in client module

Repeated pattern appears across many methods:
1. Build command
2. send_command(envelope::pack_any(&command, CMD_*))
3. response_payload_as_any(response, RES_*)
4. decode_any(...)
Evidence:
- Seen across src/client/common.rs, src/client/board.rs, src/client/items.rs, src/client/document.rs.
Impact: Boilerplate proliferation increases maintenance drag and inconsistency risk.
Severity: Medium
Status: MITIGATED — rpc! dispatch macro added and demonstrated in 4 methods (commit bda2ed6). Full conversion available for future work.

AP-5: Silenced results via `let _ =` in production code

Evidence:
- Client module methods in src/client/common.rs, src/client/board.rs, and src/client/document.rs
- src/transport.rs:36 (channel send during shutdown)
- src/blocking.rs: :41, :46, :77, :103
Notes: Some cases are intentional (e.g., benign send failure during shutdown), but several cases could hide useful operational failures.
Impact: Potential silent failure paths and debugging opacity.
Severity: Low-Medium

AP-6: Pervasive unchecked `as i32` casts for protobuf enum discriminants

Evidence: numerous production instances across src/client/mod.rs, src/client/mappers.rs, and src/model/common.rs.
Notes: This is common in prost-backed code, but it is still unchecked narrowing.
Impact: Low in current protocol-constrained context; type-safety remains weaker than explicit conversion helpers.
Severity: Low

Anti-pattern conclusion

No major architectural anti-patterns like pervasive unwrap in production paths, unsafe shortcuts, or panic-driven control flow were found. Primary concern is maintainability/process friction: boilerplate repetition, strict clippy policy mismatch with generated code, and a handful of silent-result patterns.

Pass 2: `plan-eng-review` Findings (Structure and Maintainability)

Structural strengths

Clean conceptual layering reflected in module organization:
- transport (IPC boundary)
- envelope (protobuf Any/type URL handling)
- client-level typed wrappers and conversions
Blocking facade includes a strong parity guard:
- src/blocking.rs:586 (sync_wrapper_covers_async_method_names)
- This is an excellent protection against async/blocking drift.
Rich typed models provide ergonomic APIs over raw protobuf payloads:
- src/model/board.rs
- src/model/common.rs

Transport architecture

Implemented in a single file: src/transport.rs (126 LOC), not a transport directory.
Uses an nng Req0 socket with an async-to-blocking bridge.
Tokio MPSC queue (capacity = 64) feeds a dedicated OS worker thread.
Worker performs blocking socket_roundtrip (send then recv).
Async side awaits completion via oneshot response channels.
Timeout and transport failures are clearly mapped to KiCadError::{Timeout, TransportSend, TransportReceive, Connection}.

Feature flag architecture

default = ["async"]
async = ["dep:nng", "dep:prost", "dep:prost-types", "dep:tokio"]
blocking = ["async"] (additive, not replacement)
tracing = ["dep:tracing"]
Blocking facade runs a dedicated single-thread Tokio runtime worker and dispatches sync calls through a bounded channel.

Model layer cross-dependencies

model::common depends on board-layer types (PcbItem, Vector2Nm, etc.).
Effective layering is common → board-aware, not fully independent.
This is pragmatically acceptable today, but worth noting if future domain split/modularization is pursued.

Structural risks

ST-1: Monolithic client module

Evidence: formerly monolithic src/client.rs at 5448 LOC with 107 public async methods.
Why it matters:
- Larger review blast radius for changes.
- Mixed responsibilities (command dispatch + mapping + helpers + tests) in one file.
- Harder onboarding and higher chance of incidental coupling.
Severity: Medium

Natural splitting points for `client.rs`

client/common.rs: ping/version/paths/open docs/run_action/text/netclass (~lines 359-666)
client/items.rs: item CRUD, item decoding, by-id/by-type queries (~lines 671-861, 1211-1409)
client/board.rs: board layers/origin/stackup/appearance/nets (~lines 885-1040, 1572-1744)
client/selection.rs: selection mutation + summaries/details (~lines 1047-1201)
client/geometry.rs: text extents/shapes, bounding box, hit test, pad polygon (~lines 1426-1572, 1879-1954)
client/document.rs: save/revert/string serialization/title block (~lines 1750-1865)
client/mappers.rs: pure proto↔model mapping helpers (post-1954)

Status: RESOLVED — client.rs has been split into 11 domain modules under src/client/ (commit 028aff9). All public API signatures preserved.

ST-2: Public `commands::*` modules appear as placeholders

Evidence:
- src/commands/base.rs
- src/commands/board.rs
- src/commands/editor.rs
- src/commands/project.rs
Each contains only a trivial empty struct.
Why it matters: Public API surface may imply supported low-level builder functionality that is not yet substantive.
Severity: Low to Medium (usability/signaling risk)

ST-3: Very broad public API requires stronger discoverability discipline

With 100+ async methods, docs quality and examples become critical for practical use.
Missing docs warnings and uneven method-level docs indicate discoverability debt.
Severity: Medium

Structure conclusion

Architecture is fundamentally sound, but maintainability risk is rising due to centralization and API breadth. Modularizing client domains and reducing repeated RPC boilerplate are the highest-value structural improvements.

Pass 3: `review` Findings (PR #23 Correctness)

What changed in PR scope

Diff vs origin/main (10 files changed):

kicad submodule pin update to KiCad 10.0.0
Generated proto refresh:
- src/proto/generated/kiapi.board.commands.rs
New method wiring:
- src/client/board.rs (GetBoardLayerName request/response constants + method)
- src/blocking.rs parity method
Coverage/document updates:
- README.md
- docs/book/src/intro.md
- docs/book/src/validation.md
- src/lib.rs
- src/kicad_api_version.rs
- test-scripts/kicad-ipc-cli.rs

Correctness assessment

CR-1: KiCad v10 stable proto delta appears correctly applied

The newly-added command (GetBoardLayerName) is reflected in generated types and wrapped in high-level API.

CR-2: Protocol-contract tests are meaningful and non-trivial

Added tests verify runtime type_url contracts, which the Rust type system cannot enforce by itself:

decode succeeds on expected type URL
decode fails on mismatched type URL
command Any packing uses expected proto command name

This is a strong testing choice for IPC/protobuf Any boundaries.

CR-3: Blocking parity maintained

New async method has matching blocking exposure and remains protected by parity test strategy.

Correctness conclusion

PR #23 quality is good. Changes are focused, functionally coherent, and backed by targeted tests that protect real protocol failure modes.

Pass 4: `document-release` Findings (Usefulness and Release Readiness)

Positive documentation signals

README contains detailed compatibility matrix and runtime notes.
Book includes API reference links and practical usage patterns.
Validation commands are clearly documented.
Version snippet drift previously identified in DR-1 has been corrected (see Resolved Issues).

Active documentation and release-readiness findings

DR-2: Broken anchor in `validation.md`

docs/book/src/validation.md:21 links to README anchor #kicad-v1000-api-completion-matrix
README heading is ## KiCad v10.0.0 API Reference (README.md:118)
Anchor slug does not match current heading
Severity: Low (broken cross-reference)
Status: RESOLVED (commit 5d3bb4b)

DR-3: Filesystem artifact in docs tree

docs/book/src/https:/docs.rs/ was investigated as a potential literal directory artifact.
Investigation confirmed the artifact does not exist on this branch.
Severity: Low (docs hygiene)
Status: RESOLVED — investigation confirmed the artifact does not exist on this branch.

DR-4: Rustdoc coverage gap in client Tier 1 surface

120 public items across src/client/*.rs
47 documented (39% coverage)
73 public items undocumented
Gaps notably include several *_raw variants and some typed wrappers
Evidence examples of undocumented public methods:
- src/client/common.rs
- src/client/board.rs
- src/client/items.rs
- src/client/selection.rs
Severity: Medium (API discoverability)

DR-5: Narrow examples set

Only one example: examples/selection_deep_dump.rs (632 LOC)
Current example is advanced and blocking-feature-gated
No beginner-oriented examples (e.g., connect+ping, simple list/query)
Severity: Medium (onboarding friction)
Status: RESOLVED — added hello_kicad.rs (connect+ping+version) and board_inspector.rs (nets/layers/origin) examples (commit 6efc241)

DR-6: README missing explicit prerequisites section

No dedicated prerequisites section explicitly stating KiCad runtime requirements (running KiCad with IPC available/enabled)
Runtime preconditions are discoverable via book quickstart, but not surfaced early in README
Severity: Low-Medium (onboarding clarity)
Status: RESOLVED — added Prerequisites section with KiCad IPC API setup guide and KICAD_API_SOCKET override note (commit 6efc241)

Documentation conclusion

Documentation quality is generally strong, and version alignment has materially improved. Remaining gaps are low-to-medium severity but user-visible: cross-link correctness, explicit prerequisites, broader examples, and improved rustdoc coverage for the Tier 1 API surface.

Quality Gates and Validation Status

Tests

cargo test -q: pass
cargo test --features blocking -q: pass

Linting

cargo clippy --all-targets --all-features -- -D warnings: fails

Observed root causes:

Generated proto enum naming lints under strict clippy
A few fixable local style issues (clone-on-copy, bool assert style in tests)

Implication: Either clippy policy needs explicit generated-code handling, or strict global clippy will remain noisy/fragile.

Resolved Issues

DR-1: Stale version snippets in docs (resolved)

Previously reported version drift has been fixed and is no longer an active risk.

Resolved evidence:

README.md:27 → 0.4.1 ✓
README.md:67 → 0.4.1 ✓
docs/book/src/quickstart.md:15 → 0.4.1 ✓
docs/book/src/quickstart.md:42 → 0.4.1 ✓
Cargo.toml crate version → 0.4.1 ✓
CHANGELOG.md includes [0.4.1] entry ✓

Impact: The highest-friction onboarding mismatch from the initial report has been addressed.

DR-2: Broken anchor in validation.md (resolved)

Fixed in commit 5d3bb4b. Anchor updated to match current README heading.

DR-3: Filesystem artifact (resolved)

Investigation confirmed the docs/book/src/https: directory does not exist on this branch.

AP-1/AP-2/AP-3: Clippy lint findings (resolved)

All three clippy findings fixed in commit 5d3bb4b.

ST-1: Monolithic client.rs (resolved)

Split into 11 domain modules in commit 028aff9. No public API changes.

DR-5/DR-6: Examples and prerequisites (resolved)

Two beginner examples and a README prerequisites section added in commit 6efc241.

Risk Register

ID	Risk	Area	Severity	Likelihood	Notes
R2	Monolithic `client.rs` slows safe evolution	Structure	Medium	High	RESOLVED — Split into 11 domain modules (commit 028aff9)
R3	Strict clippy friction due to generated code	Process	Medium	High	Reproducible with current strict command
R4	Public placeholder command modules confuse users	API clarity	Low/Med	Medium	Can be fixed with docs or visibility adjustment
R5	Missing docs on public Tier 1 items in `src/client/*`	DX/discoverability	Medium	Medium	73/120 public items undocumented (39% documented)
R6	Broken doc links/anchors	Docs	Low	Medium	RESOLVED — Anchor fixed (commit 5d3bb4b)
R7	Filesystem artifact in docs tree	Hygiene	Low	Low	RESOLVED — Artifact confirmed nonexistent
R8	Narrow examples coverage	Onboarding	Medium	High	RESOLVED — Two beginner examples added (commit 6efc241)
R9	Repeated RPC boilerplate	Maintainability	Medium	High	MITIGATED — `rpc!` macro added (commit bda2ed6)

Prioritized Action Plan (Report-only)

P0: Immediate (highest ROI)

✅ Done — Fix broken validation.md anchor to correctly reference current README heading.
⬜ Open — Document newly added and existing Tier 1 public methods; target 80%+ rustdoc coverage for Tier 1 public methods.
✅ Done — Define clippy policy for generated files (allowlist/scope strategy) and document it.
✅ Done — Clean obvious non-generated clippy findings (clone_on_copy, bool assert style in tests).
✅ Done — Remove docs/book/src/https: filesystem artifact from docs tree (investigation confirmed nonexistent on this branch).

Expected outcome: Cleaner user navigation, better first-run success, improved CI signal quality, reduced contributor confusion.

P1: Maintainability upgrades

✅ Done — Split the former monolithic client file by functional domains while preserving public API signatures, using module seams under src/client/:
- client/common.rs
- client/items.rs
- client/board.rs
- client/selection.rs
- client/geometry.rs
- client/document.rs
- client/mappers.rs
- client/decode.rs
- client/format.rs
- client/tests.rs
⬜ Open — Group command/response type URL constants near their domain methods.
✅ Done — Keep and extend protocol-contract test helpers to reduce repeated literal contract strings.
✅ Done — Extract a generic typed RPC dispatch helper to reduce repeated send_command/pack_any/response_payload_as_any/decode_any boilerplate.
✅ Done — Add 2–3 beginner-friendly examples (e.g., connect+ping, list-nets, simple query).

Expected outcome: Smaller review units, lower regression risk, reduced boilerplate drift, faster onboarding.

P2: API clarity and polish

Clarify intent of public commands::* modules (document as placeholders or reduce visibility until substantive).
Add a concise versioning model section (crate version vs proto pin vs tested KiCad runtime).
Add one focused quickstart snippet showing get_board_layer_name usage.
✅ Done — Add an explicit README Prerequisites section describing KiCad runtime requirements.
⬜ Partial — Improve rustdoc coverage to 80%+ for Tier 1 API surface (module-level rustdoc added across client submodules; deeper method-level coverage still needed).
Audit and fix all cross-document links between mdBook and README.

Expected outcome: Reduced user misinterpretation and smoother docs-driven adoption.

Suggested Acceptance Criteria for Follow-up Work

For documentation consistency and integrity

README/book cross-links resolve correctly.
No malformed artifact directories remain under docs source.
README includes an explicit prerequisites section for KiCad IPC runtime conditions.
Tier 1 public API rustdoc coverage reaches 80%+.

For lint/process hygiene

cargo clippy --all-targets --all-features -- -D warnings is either:
- fully green, or
- intentionally scoped with documented generated-code exemptions.

For architecture improvements

Client module split into coherent domain submodules under src/client/.
Repeated RPC boilerplate consolidated into typed helper(s) where practical.
No public API breakage in function names/signatures.
Existing parity and protocol tests remain green.

Detailed Notes and Evidence Pointers

Core API and layering: src/lib.rs
Main API implementation and constants: src/client/mod.rs
Blocking parity guard test: src/blocking.rs:586
Transport bridge implementation: src/transport.rs
Error boundary taxonomy: src/error.rs
Generated proto lint hotspot examples:
- src/proto/generated/kiapi.common.rs:56
- src/proto/generated/kiapi.common.commands.rs:424
Documentation issues:
- docs/book/src/validation.md:21
- README.md:118
- docs/book/src/validation.md:14
Rustdoc coverage evidence samples (undocumented public methods):
- src/client/common.rs
- src/client/board.rs
- src/client/items.rs
- src/client/selection.rs

Final Assessment

The library is in a strong position functionally and continues to show disciplined protocol-aware testing. The most impactful near-term improvements are rustdoc depth for Tier 1 APIs and lint/process cleanup after the client modularization. Medium-term effort should focus on continuing RPC boilerplate consolidation and keeping module boundaries clean as API breadth grows.

Report Revision History

2026-03-29: Initial report generated (partial)
2026-03-29: Comprehensive completion pass — verified all metrics against codebase, expanded anti-pattern scan (3→6 findings + verified clean signals), added transport/feature-flag/model architecture details, identified 5 new documentation issues, corrected resolved DR-1, expanded risk register (5→9 entries), and updated action plan
2026-03-29: Implementation pass — completed P0 fixes, client.rs modularization, rpc! macro, beginner examples, README prerequisites/examples sections, protocol-contract tests, and module-level rustdoc across all client submodules

25 KiB Raw Permalink Blame History Unescape Escape