Skip to content

M0–M3 gap backlog: T1–T9 (bundled libs, MCP tools, validators, drop-import)#2

Merged
LayerDynamics merged 18 commits into
mainfrom
m0-m3-gap-backlog
May 25, 2026
Merged

M0–M3 gap backlog: T1–T9 (bundled libs, MCP tools, validators, drop-import)#2
LayerDynamics merged 18 commits into
mainfrom
m0-m3-gap-backlog

Conversation

@LayerDynamics

@LayerDynamics LayerDynamics commented May 25, 2026

Copy link
Copy Markdown
Owner

Closes the outstanding M0–M3 gaps from the codebase audit (Todo.md). Worked top-to-bottom as the Sequential execution backlog; each item is its own commit with tests.

Done (T1–T9)

# Item Highlights
T1 SPEC.md root redirect symlink → docs/specs/SPEC-01-kiclaude.md
T2 SPEC §16.2 P4 resolved CRDT vendor → Yjs, per ADR-0001
T3 docs/architecture/ + docs/observability/ overview + OTel collector + importable Grafana dashboard (grounded in the real agent.hook.* span contract)
T4 5 slash commands /snapshot /revert /board-diff /add-led /add-usb-c
T5 Design-intent validators KC020/021/030/031/040/050 (KC040 via real IPC-2141A microstrip); 19 tests
T6 8 Claude-facing MCP tools registry 28→36; decoupling/partition/impedance/diffpair/length-match/bom/session-fork/export-step
T7 kc_mpn_resolve (full) live distributor aggregator + library candidates via a new library/search route
T8 FR-043 drop-to-import POST /library/import + useLibraryImport/LibraryImportDropZone; 6 route + 5 client tests
T9 Bundled libs/ mirror pinned KiCad 9.0.0 subset (SHA-256 in MANIFEST.toml, populate_libs.py), merged into library/search (FR-040)

Notable in T9 (please review)

The bundled mirror can't honestly contain libraries KiCad doesn't ship, so the examples' fictional/wrong refs were corrected to real KiCad 9.0.0 parts:

  • blinky: MCU_Espressif:RF_Module:ESP32-S3-WROOM-1
  • esp32_c6_rf: ESP32-C6-WROOM-1ESP32-C6-MINI-1; fabricated Package_BGA:DDR3L_BGA-16_4x4_P0.8mm → real BGA-16_1.92x1.92mm_Layout4x4_P0.5mm (Value MT41K256M16-DDR3LBGA-16, since that DRAM is FBGA-96); RF_Connectors:U.FL_Molex_73412-0110Connector_Coaxial:U.FL_Molex_MCRF_73412-0110_Vertical
  • buck_subsystem: Regulator_Switching:TPS562201 → pin-identical real TPS562202 (+ datasheet URL)

Not in this PR (recorded, not stubbed)

  • T10 — STEP geometry parsing: needs occt-import-js (~20 MB wasm); SPEC §11 defers to M4.
  • T11 — Zone-fill simple XOR ≤ 0.01: empirically ruled out across 6 attempts (docs/plans/2026-05-24-m2-r-05d-edge-aligned-offset.md); needs a multi-week ClipperOffset port.

Verification

  • Golden round-trip (M0-Q-02) + snapshot: byte-identical ✓
  • Python: 388 existing + new tests pass; ruff clean ✓
  • Client (T8): typecheck green; 483 tests pass ✓

Known gaps

  • Footprint resolution from the mirror isn't exercised by a test yet (the library/search route is symbol-only) — coverage gap for whoever adds kc_footprint_search.

🤖 Generated with Claude Code

Summary by Sourcery

Complete the remaining M0–M3 backlog items by wiring high-speed design validators, sourcing and STEP export tools, library search/import infrastructure, bundled KiCad libraries, and related docs and commands into the agent stack.

New Features:

  • Add M3 design-intent validators (KC020/021/030/031/040/050) to kc_validate for decoupling, power-rail sourcing, length matching, diff pairs, impedance, and ground partitioning.
  • Expose new high-speed and sourcing MCP tools (kc_decoupling_check, kc_partition_check, kc_impedance_check, kc_diffpair_declare, kc_length_match_set, kc_bom_get, kc_export_step, kc_session_fork) for Claude-facing workflows.
  • Implement a full kc_mpn_resolve that queries live distributors and uses a new kiserver /project/{id}/library/search endpoint for symbol and footprint candidates.
  • Add kiserver endpoints for library search, session fork, and FR-043 library import, plus client-side drag-and-drop import via useLibraryImport and LibraryImportDropZone.
  • Introduce a pinned bundled KiCad library mirror under libs/ with manifest, licensing, and a populate_libs.py script to fetch/verify curated symbol and footprint libraries.
  • Add slash commands for snapshot, revert, board-diff, add-led, and add-usb-c to script common agent workflows.
  • Provide architecture and observability documentation, including an OTel collector config and a Grafana dashboard for agent spans, and symlink SPEC.md to the main spec file.

Bug Fixes:

  • Guard BomView BOM price loads with a per-request sequence id to prevent stale responses from overwriting newer results.
  • Align example projects (blinky, esp32_c6_rf, buck_subsystem) and golden files to real KiCad 9.0.0 symbols/footprints and correct MPNs, ensuring the bundled mirror remains honest.

Enhancements:

  • Document the relationship between KC001–KC011 structural checks and SPEC §7.3 design-intent codes in kc_validate.
  • Ensure kc_mpn_resolve fails closed when distributors or kiserver are unavailable and derives heuristic confidence based on available metadata and library candidates.
  • Improve kiserver library search to merge results from project-local libraries and the bundled mirror with deterministic ranking and de-duplication.
  • Persist session forks as on-disk manifests compatible with the agent session layer, enabling visible session trees for what-if branches.
  • Enhance observability by aligning agent span attributes with the telemetry contract and deriving RED-style metrics via the provided collector pipeline.

Documentation:

  • Add docs/architecture/ overview describing the layered system, core contracts, and component responsibilities.
  • Add docs/observability/ with an OTel Collector config and an importable Grafana dashboard for agent tool telemetry.
  • Resolve SPEC CRDT vendor decision (P4) in SPEC-01-kiclaude.md in favour of Yjs and reference ADR-0001.
  • Document the bundled library mirror layout, licensing, and refresh process in libs/README.md and libs/LICENSE.md.

Tests:

  • Add unit tests for the new KC020–KC050 validators, including impedance calculations and ground partition checks.
  • Add integration tests for the high-speed MCP tools, ensuring they call kiserver/kiconnector correctly and persist project mutations.
  • Add dedicated tests for kc_mpn_resolve covering shape rejection, distributor hits, library candidate fetching, and confidence calculation.
  • Add route tests for bundled library resolution, library search, library import, and session fork to validate server behaviour and on-disk effects.
  • Add client tests for the library import hook/dropzone and extend MCP tool registry tests to cover the new high-speed tools.

LayerDynamics and others added 9 commits May 25, 2026 14:20
SPEC §17 step 8 — top-level SPEC.md symlink to the canonical spec.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…afana preset (T3)

SPEC §8.8 + §12. Architecture overview, an OTel Collector config
(OTLP → spanmetrics → Prometheus) and an importable Grafana dashboard,
grounded in the real agent.hook.* span contract.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (T4)

The 5 SPEC §A.3 slash commands missing from .claude/commands/. The
first three wrap existing tools (kc_snapshot_*, kc_diff); the two add-*
commands are synthesis prompts over the existing schematic kc_* tools.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s (T5)

SPEC §7.3. Decoupling (KC020), power-rail source (KC021), length-match
membership (KC030), diff-pair declaration (KC031), controlled-impedance
achievability (KC040, IPC-2141A estimate), analog/digital partition
isolation (KC050). 19 unit tests; KC001-011 numbering reconciliation
documented.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kc_decoupling_check / kc_partition_check / kc_impedance_check (reuse the
KC020/050/040 validators); kc_diffpair_declare / kc_length_match_set
(reuse ui_* mutation logic, persist via /replace); kc_bom_get (KCIR
grouping); kc_export_step (kiconnector /tools/step); kc_session_fork
(new kiserver /project/{id}/session/fork writing an agent-readable
session manifest). Registry 28 -> 36. 11 tool tests + 2 route tests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…y candidates (T7)

FR-040/041/042. Replaces the M1 shape-only heuristic: real distributor
stock/price/lifecycle/datasheet via the shared aggregator (fails closed
per FP#5), plus symbol + footprint candidates from a new kiserver
GET /project/{id}/library/search over the existing LibraryIndex.
5 mpn tests + 3 route tests; existing mpn tests made hermetic.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kiserver POST /project/{id}/library/import writes the dropped file into
a project-local imported-libs/ (symbols) or <nick>.pretty/ (footprints),
idempotently registers the matching sym-lib-table/fp-lib-table row, and
strips path traversal from the filename. Client useLibraryImport hook +
LibraryImportDropZone infer the kind from the extension and surface the
result/error. 6 route tests + 5 client tests.

Also fixes a pre-existing BomView.tsx TS6133 (unused lastRequestId from
an earlier web edit) by wiring it in as the intended stale-response
guard on the async bom-price load.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lands the bundled symbol/footprint mirror SPEC §9.5/§12/D6/FR-040 require.
libs/ ships a curated, pinned subset of the official KiCad libraries — the
five whole symbol libs the examples reference (Device, power, Connector,
Regulator_Switching, RF_Module) plus the exact footprints they place — all
fetched from KiCad GitLab at tag 9.0.0 and SHA-256-pinned in MANIFEST.toml by
scripts/populate_libs.py (--pin re-fetches+repins; default verifies and
self-heals missing files per D6). sym-lib-table/fp-lib-table register them via
${KICLAUDE_BUNDLED_LIBS}; LICENSE.md carries the CC-BY-SA-4.0 + KiCad Library
Exception attribution.

kiserver GET /project/{id}/library/search now resolves the project's own libs
AND the bundled mirror (bundled_libs_dir() + _merge_hits), so a project with an
empty sym-lib-table still resolves the standard KiCad parts offline. Empirical
gate: Device:R and Connector:USB_C_* resolve through the route (9 tests).

Fixes the examples' fictional/wrong library references to real KiCad 9.0.0
parts (the mirror cannot honestly contain libraries KiCad does not ship):
- blinky: MCU_Espressif: -> RF_Module:ESP32-S3-WROOM-1
- esp32_c6_rf: ESP32-C6-WROOM-1 -> ESP32-C6-MINI-1; fabricated
  Package_BGA:DDR3L_BGA-16_4x4_P0.8mm -> real BGA-16_1.92x1.92mm_Layout4x4_P0.5mm
  (Value MT41K256M16-DDR3L -> BGA-16, that DRAM is FBGA-96); U.FL ->
  Connector_Coaxial:U.FL_Molex_MCRF_73412-0110_Vertical
- buck_subsystem: Regulator_Switching:TPS562201 -> pin-identical real TPS562202
  (+ matching TI datasheet URL)

Golden round-trip (M0-Q-02) + snapshot re-verified byte-identical.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@sourcery-ai

sourcery-ai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Implements the remaining M0–M3 backlog items T1–T9: adds design‑intent validators and high‑speed MCP tools, wires full MPN resolution through a new library search route and bundled KiCad 9.0.0 library mirror, introduces STEP export and session‑fork tooling, adds drop‑to‑import for libraries and new slash commands, and updates docs/observability and examples to match the real bundled libraries.

Sequence diagram for kc_mpn_resolve with distributors and library search

sequenceDiagram
  actor Claude
  participant MCP as MCP_kc_mpn_resolve
  participant Dist as DistributorAggregator
  participant KS as Kiserver
  participant LI as LibraryIndex

  Claude->>MCP: kc_mpn_resolve{ mpn, manufacturer, footprint, project_id }
  MCP->>MCP: _shape_rejection(mpn)
  alt [invalid MPN shape]
    MCP-->>Claude: envelope{ ok:true, found:false, reason, stock:null, symbol_candidates:[] }
  else [valid MPN]
    MCP->>Dist: price(mpn, qty=1)
    Dist-->>MCP: pricing
    MCP->>Dist: aclose()
    opt [project_id present]
      MCP->>KS: GET /project/{id}/library/search?query=mpn
      KS->>LI: LibraryIndex.search(query)
      LI-->>KS: hits[]
      KS-->>MCP: { hits: [...] }
      MCP->>MCP: _library_candidates(hits)
    end
    MCP->>MCP: _confidence(..., found, has_candidates)
    MCP-->>Claude: envelope{ ok:true, found, stock, symbol_candidates, footprint_candidates, confidence }
  end
Loading

Sequence diagram for FR-043 library drop-to-import flow

sequenceDiagram
  actor User
  participant Browser as LibraryImportDropZone/useLibraryImport
  participant GW as Gateway_server
  participant KS as Kiserver
  participant FS as Project_filesystem

  User->>Browser: Drop .kicad_sym / .kicad_mod
  Browser->>Browser: kindForFile(filename)
  Browser->>GW: POST /api/server/project/{id}/library/import
  GW->>KS: POST /project/{id}/library/import
  alt [kind == symbol]
    KS->>FS: write imported-libs/<name>.kicad_sym
    KS->>FS: _append_lib_table_row(sym-lib-table,...)
  else [kind == footprint]
    KS->>FS: write imported.pretty/<name>.kicad_mod
    KS->>FS: _append_lib_table_row(fp-lib-table,...)
  end
  KS-->>GW: { ok:true, nickname, lib_id_prefix, uri }
  GW-->>Browser: same JSON
  Browser-->>User: Show "Imported <nickname> (<lib_id_prefix>)"
Loading

File-Level Changes

Change Details Files
Extend kc_validate with M3 design-intent validators (KC020–KC050) and unit tests.
  • Augment kc_validate docstring and tool description to distinguish legacy KC001–011 structural checks from SPEC §7.3 design-intent codes.
  • Implement pure KC020/021/030/031/040/050 validators over KCIR nets/footprints/stackup, including IPC‑2141A microstrip impedance estimation helpers.
  • Add helper functions for refdes classification, power-net detection, power flags, net-class width lookup, analog/digital ground pairs, and microstrip Z0 calculation.
  • Create focused tests in test_validate_kc.py covering decoupling, rail source, length-group membership, diff-pair declaration, impedance thresholds, and ground partitions.
services/mcp/src/kc_mcp/tools/validate.py
services/mcp/tests/test_validate_kc.py
Introduce high-speed/SI and BOM MCP tools plus STEP export and session-fork tooling, and register them in the Claude tool registry.
  • Add kc_decoupling_check, kc_partition_check, kc_impedance_check, kc_diffpair_declare, kc_length_match_set as high-speed tools reusing _run_validators and UI mutation helpers, persisting via kiserver /project/{id}/replace.
  • Add kc_bom_get to expose a grouped BOM (by MPN or value+footprint) as structured JSON with counts and refdes lists.
  • Introduce kc_export_step MCP tool that proxies to kiconnector /tools/step with pcb_path/output_dir/board_only/timeout handling.
  • Add kc_session_fork MCP tool calling kiserver /project/{id}/session/fork and returning new/parent session ids.
  • Update MCP server registry to include the eight new tools and adjust registry size assertions; add integration tests for all eight tools, including mocked kiserver/kiconnector interactions and tool registration checks.
services/mcp/src/kc_mcp/tools/highspeed.py
services/mcp/src/kc_mcp/tools/bom.py
services/mcp/src/kc_mcp/tools/export.py
services/mcp/src/kc_mcp/tools/session.py
services/mcp/src/kc_mcp/server.py
services/mcp/tests/test_highspeed_tools.py
services/mcp/tests/test_pcb_tools.py
Upgrade kc_mpn_resolve from a shape-only check to a full resolver using live distributors and project library candidates, with supporting tests.
  • Change kc_mpn_resolve semantics and description to call the shared distributor aggregator, fail closed unless a real quote is returned, and optionally include project_id.
  • Implement shape rejection, async distributor lookup via sourcing._factory, and confidence scoring that prioritizes distributor hits while scaling with metadata and library candidates.
  • Add _library_candidates helper using kiserver_get /project/{id}/library/search to build symbol and footprint candidate lists.
  • Make existing tests hermetic via a no-hit aggregator stub and add a dedicated test_mpn_resolve suite with mocked aggregator and kiserver client covering errors, distributor hits/misses, and candidate confidence effects.
services/mcp/src/kc_mcp/tools/mpn.py
services/mcp/tests/test_schematic_tools.py
services/mcp/tests/test_mpn_resolve.py
Expose bundled and project symbol libraries via a new kiserver library search route and introduce a bundled KiCad 9.0.0 library mirror with tooling and tests.
  • Add SearchHit type import and implement /project/{id}/library/search to query the project’s sym-lib-table using LibraryIndex and merge hits with an optional bundled mirror via _merge_hits and bundled_libs_dir.
  • Implement bundled_libs_dir that honours KICLAUDE_BUNDLED_LIBS or falls back to in-repo libs/ and only enables when a sym-lib-table is present.
  • Create a pinned libs/ mirror containing selected KiCad 9.0.0 symbol/footprint libraries plus sym-lib-table, fp-lib-table, MANIFEST.toml, README, LICENSE, and actual .kicad_sym/.kicad_mod files.
  • Add scripts/populate_libs.py to fetch/verify curated libraries from KiCad GitLab, maintain MANIFEST.toml SHA-256 pins, and self-heal missing files.
  • Add tests verifying helper behaviour and mirror resolution via /library/search, including env overrides, dedup/merging behaviour, and integration that Device:R and Connector USB-C parts resolve through the mirror; also add a route test that ensures projects without tables and with mirror disabled return empty but valid hit lists.
services/kiserver/src/kiserver/main.py
services/kiserver/tests/test_bundled_libs.py
services/kiserver/tests/test_library_search.py
libs/MANIFEST.toml
libs/README.md
libs/LICENSE.md
libs/sym-lib-table
libs/fp-lib-table
libs/symbols/*.kicad_sym
libs/footprints/**/*.kicad_mod
scripts/populate_libs.py
Implement FR-043 drop-to-import of symbol/footprint libraries in kiserver and client, with tests.
  • Add LibraryImportRequest model and helper functions for safe basenames and appending lib-table rows (idempotently) for sym-lib-table and fp-lib-table.
  • Implement POST /project/{id}/library/import to write .kicad_sym into imported-libs/ and .kicad_mod into imported.pretty inside the project, update the appropriate lib-table using ${KIPRJMOD} URIs, strip path traversal, and return nickname/lib_id_prefix/uri.
  • Introduce useLibraryImport hook and LibraryImportDropZone React component to infer kind from extension, POST to the new route, manage state/errors, and provide a drop target with callback.
  • Add route tests that verify files and table rows are created, idempotency of lib-table rows, extension validation, path stripping, and 404 behaviour, plus client tests for hook behaviour, kind inference, error handling, and dropzone wiring.
services/kiserver/src/kiserver/main.py
services/kiserver/tests/test_library_import.py
client/src/components/schematic/LibraryImport.tsx
client/src/components/schematic/LibraryImport.test.tsx
Add session fork support at the kiserver level to back kc_session_fork and ensure manifests are agent-readable.
  • Define SessionForkRequest model on kiserver with parent_session_id and label fields.
  • Implement POST /project/{id}/session/fork to create a .kiclaude/sessions/.json manifest recording project_id, session_id, forked_from, label, timestamps, and schema_version; write atomically and log an event.
  • Add tests that open a real example project, call the fork route, validate manifest contents and shape expected by the agent session layer, clean up test artifacts, and confirm 404 on unknown projects.
services/kiserver/src/kiserver/main.py
services/kiserver/tests/test_session_fork.py
Add kc_export_step plumbing on the MCP side and kiconnector integration tests for STEP export use.
  • Define kc_export_step MCP tool that validates pcb_path/output_dir, forwards to /tools/step via kiconnector_post, passes through timeout/board_only options, and returns a shaped envelope with the STEP result.
  • Extend high-speed integration tests to mock kiconnector /tools/step, verify that kc_export_step calls it with expected body, and check returned ok/step path shape.
services/mcp/src/kc_mcp/tools/export.py
services/mcp/tests/test_highspeed_tools.py
Add BOM grouping MCP tool and fix BomView client to avoid stale response races.
  • Implement _bom_lines helper and kc_bom_get tool to group footprints into BOM lines keyed by MPN or value+lib_id, skipping exclude_from_bom, counting qty and refdes, and returning sorted, stable lines with placement count.
  • Enhance client BomView to tag each load with a monotonically increasing requestId and ensure state updates (lines, pricing, error, loading) only apply if the response corresponds to the latest request, preventing slow earlier responses from overwriting newer state.
services/mcp/src/kc_mcp/tools/bom.py
client/src/components/pcb/BomView.tsx
Document architecture and observability, wire SPEC redirect, and record SPEC ADR decision.
  • Add SPEC.md symlink redirect at repo root pointing to docs/specs/SPEC-01-kiclaude.md.
  • Update SPEC P4 CRDT vendor row as resolved to Yjs with ADR-0001 reference.
  • Add docs/architecture/README.md describing system layers, contracts, and key paths.
  • Add docs/observability README, OpenTelemetry collector config wired to agent.hook.* span attributes, and a Grafana dashboard JSON for agent tool latency and RED metrics.
SPEC.md
docs/specs/SPEC-01-kiclaude.md
docs/architecture/README.md
docs/observability/README.md
docs/observability/otel-collector-config.yaml
docs/observability/grafana/kiclaude-agent-dashboard.json
Add snapshot/revert/board-diff/add-led/add-usb-c slash commands for Claude Code and describe their flows.
  • Create .claude command docs for /snapshot, /revert, /board-diff, /add-led, /add-usb-c, each with metadata (name, description, argument-hint, allowed-tools) and detailed procedural guidance aligned with the implemented MCP tools and spec first principles.
  • Ensure the commands reference the correct MCP tool names and expected behaviours (snapshots, ERC checks, MPN resolution, diff tooling, decoupling, USB-C correctness).
.claude/commands/snapshot.md
.claude/commands/revert.md
.claude/commands/board-diff.md
.claude/commands/add-led.md
.claude/commands/add-usb-c.md
Introduce and wire a pinned bundled KiCad library mirror into examples, updating example designs to use real KiCad 9.0.0 parts.
  • Populate libs/ with pinned KiCad symbol and footprint libraries and adjust example projects’ schematic/PCB files to reference real KiCad 9.0.0 part names and footprints instead of fictional ones (e.g., ESP32 module, BGA DRAM package, U.FL connector, regulator MPN).
  • Update golden PCB test fixtures to match the new library references and reproved golden round-trip tests accordingly.
examples/blinky/blinky.kicad_pcb
examples/buck_subsystem/buck_subsystem.kicad_sch
examples/esp32_c6_rf/esp32_c6_rf.kicad_pcb
tests/golden/blinky.kicad_pcb
libs/**/*
Record the sequential execution backlog in Todo.md and explicitly document deferred items T10–T11.
  • Append a new section to Todo.md listing T1–T9 as completed with brief notes and explicitly mark T10 STEP geometry parsing and T11 zone-fill XOR tolerance as recorded-but-deferred with their blockers and rationale.
Todo.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several major features, including a pinned KiCad library mirror for offline part resolution, drag-and-drop library imports (FR-043), structured BOM retrieval, session branching, and high-speed design-intent validators (KC020-KC050) with corresponding MCP tools. Feedback on the changes focuses on critical performance and robustness improvements: offloading synchronous file and database operations in FastAPI endpoints to standard synchronous functions to avoid blocking the asyncio event loop, enhancing the decoupling and ground partition checks to prevent false negatives, adding thread safety to library table modifications, and implementing defensive checks to prevent crashes from malformed data or negative impedance calculations.

Comment thread services/kiserver/src/kiserver/main.py Outdated
Comment thread services/kiserver/src/kiserver/main.py Outdated
Comment thread services/kiserver/src/kiserver/main.py Outdated
Comment on lines +391 to +395
unbypassed = [
pn
for pn in sorted(power_nets)
if not any(c == "cap" for (_, c) in net_members.get(pn, []))
]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current decoupling check only verifies if any capacitor is connected to the power net. However, a bypass capacitor must be connected between the power rail and ground to be effective. If a capacitor is connected in series or between two non-ground nets, it is not a bypass capacitor. We should verify that the capacitor also connects to a ground net (e.g., containing 'GND' or 'VSS').

        unbypassed = []
        for pn in sorted(power_nets):
            has_bypass = False
            for (other_ref, cls) in net_members.get(pn, []):
                if cls == "cap":
                    other_nets = fp_nets.get(other_ref, set())
                    if any("GND" in gn.upper() or "VSS" in gn.upper() for gn in other_nets):
                        has_bypass = True
                        break
            if not has_bypass:
                unbypassed.append(pn)

Comment on lines +562 to +574
if len(bridges) > 1:
findings.append(
{
"code": "KC050",
"severity": "error",
"message": (
f"Partition violation: {agnd} and {dgnd} are tied by "
f"{len(bridges)} components ({', '.join(bridges)}); a split-"
"ground design needs exactly one bridge (single-point tie)."
),
"target_uuid": None,
}
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current ground partition check only flags if there are multiple bridges (len(bridges) > 1). However, if there are zero bridges (len(bridges) == 0), the analog and digital grounds are completely isolated, which is also a severe partition violation that causes reference drift and noise. We should flag both cases.

        if len(bridges) == 0:
            findings.append(
                {
                    "code": "KC050",
                    "severity": "error",
                    "message": (
                        f"Partition violation: {agnd} and {dgnd} are completely isolated; "
                        "a split-ground design needs exactly one bridge (single-point tie) "
                        "to prevent reference drift."
                    ),
                    "target_uuid": None,
                }
            )
        elif len(bridges) > 1:
            findings.append(
                {
                    "code": "KC050",
                    "severity": "error",
                    "message": (
                        f"Partition violation: {agnd} and {dgnd} are tied by "
                        f"{len(bridges)} components ({', '.join(bridges)}); a split-"
                        "ground design needs exactly one bridge (single-point tie)."
                    ),
                    "target_uuid": None,
                }
            )

Comment thread services/kiserver/src/kiserver/main.py
Comment thread services/mcp/src/kc_mcp/tools/validate.py Outdated
Comment thread services/mcp/src/kc_mcp/tools/validate.py Outdated
Comment thread services/mcp/src/kc_mcp/tools/validate.py Outdated

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 7 issues

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="services/mcp/src/kc_mcp/tools/mpn.py" line_range="78-86" />
<code_context>
+    # distributor or fails closed).
+    found = False
+    stock: dict[str, Any] = {"quotes": []}
+    aggregator = sourcing._factory()
+    try:
+        part = await aggregator.price(raw_mpn, qty=1)
+        found = bool(getattr(part, "quotes", None))
+        stock = _pricing_to_payload(part)
+    except Exception as e:
+        stock = {"quotes": [], "error": f"distributor lookup failed: {e}"}
+    finally:
+        with contextlib.suppress(Exception):
+            await aggregator.aclose()
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle potential failures in creating the distributor aggregator explicitly.

If `sourcing._factory()` raises (e.g., due to misconfiguration), `aggregator` is never assigned and the `finally` block will raise when trying to `await aggregator.aclose()`, masking the real error. Consider initializing `aggregator: sourcing.Aggregator | None = None` before the `try` and only calling `aclose()` when it’s not `None` to avoid this secondary exception.
</issue_to_address>

### Comment 2
<location path="client/src/components/schematic/LibraryImport.tsx" line_range="24-27" />
<code_context>
+
+type ImportState = "idle" | "importing" | "done" | "error";
+
+function kindForFile(name: string): "symbol" | "footprint" | null {
+  if (name.endsWith(".kicad_sym")) return "symbol";
+  if (name.endsWith(".kicad_mod")) return "footprint";
+  return null;
+}
+
</code_context>
<issue_to_address>
**nitpick (bug_risk):** Normalize filename case when inferring library kind from extension.

These checks are case-sensitive, so files like `CUSTOM.KICAD_SYM` or `Foo.KiCad_Mod` will be treated as unsupported. Consider lowercasing `name` before the `.endsWith` checks to accept extensions in any case.
</issue_to_address>

### Comment 3
<location path="services/mcp/tests/test_validate_kc.py" line_range="88-93" />
<code_context>
+    assert len(kc) == 1 and kc[0]["severity"] == "error"
+
+
+def test_kc021_rail_with_regulator_clears() -> None:
+    proj = _project(
+        nets=[{"name": "+3V3"}, {"name": "GND"}],
+        footprints=[_fp("VR1", "+3V3", "GND"), _fp("C1", "+3V3", "GND")],
+    )
+    assert _codes(_run_validators(proj), "KC021") == []
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a KC021 test for rails driven by ICs (not only regulators/PWR_FLAG)

KC021 considers a net driven if any member is classified as `"ic"`, `"reg"`, or `"active"` by `_refdes_class`. Current tests cover the regulator (`VR1`), PWR_FLAG, and a pure-passive rail, but not a rail driven only by an IC. Please add a case (e.g. `U1` on `+3V3` with only passives otherwise) to assert that an IC-powered rail is treated as driven and to guard against regressions in `_refdes_class`/`has_active`.

```suggestion
def test_kc021_rail_with_regulator_clears() -> None:
    proj = _project(
        nets=[{"name": "+3V3"}, {"name": "GND"}],
        footprints=[_fp("VR1", "+3V3", "GND"), _fp("C1", "+3V3", "GND")],
    )
    assert _codes(_run_validators(proj), "KC021") == []


def test_kc021_rail_driven_by_ic_clears() -> None:
    proj = _project(
        nets=[{"name": "+3V3"}, {"name": "GND"}],
        footprints=[_fp("U1", "+3V3", "GND"), _fp("C1", "+3V3", "GND")],
    )
    assert _codes(_run_validators(proj), "KC021") == []
```
</issue_to_address>

### Comment 4
<location path="services/mcp/tests/test_validate_kc.py" line_range="175-176" />
<code_context>
+}
+
+
+def _imp_project(width: float, target: float, stackup: dict[str, Any] | None = _STACKUP):
+    return _project(
+        nets=[{"name": "CLK", "class": "Sig", "target_impedance_ohm": target}],
+        net_classes=[{"name": "Sig", "track_width_mm": width}],
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a KC040 test where the net class exists but has no width, to pin the `unknown`/warning path

Current KC040 tests cover normal, off-target, and missing-stackup cases, plus `_microstrip_z0` monotonicity. There’s also a path where `_net_class_width` returns `None` (net class present but no `track_width_*`), which yields a “missing geometry” warning. Please add a test that creates a project with a net class lacking width to lock in and document this behaviour.
</issue_to_address>

### Comment 5
<location path="services/mcp/tests/test_highspeed_tools.py" line_range="122-128" />
<code_context>
+    assert any(f["code"] == "KC050" for f in out["violations"])
+
+
+async def test_impedance_check_returns_per_net_result(mock) -> None:  # type: ignore[no-untyped-def]
+    out = _payload(await kc_impedance_check.handler({"project_id": "p1"}))
+    assert out["ok"] is True
+    clk = next(r for r in out["results"] if r["net"] == "CLK")
+    assert clk["target_ohm"] == 50.0
+    assert clk["achieved_ohm"] is not None
+    assert clk["status"] in {"ok", "warning", "error"}
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a kc_impedance_check test for the `status='unknown'` / missing-geometry case

`kc_impedance_check` has an explicit `status: "unknown"` branch when Er/height or width are missing. Please add a test that adjusts the mock project (e.g. clears `stackup.layers` or removes `track_width_mm`) and asserts a result with `achieved_ohm is None` and `status == "unknown"` to cover this degradation path.

Suggested implementation:

```python
async def test_partition_check_flags_double_ground_bridge(mock) -> None:  # type: ignore[no-untyped-def]
    out = _payload(await kc_partition_check.handler({"project_id": "p1"}))
    assert out["ok"] is True
    assert any(f["code"] == "KC050" for f in out["violations"])


async def test_impedance_check_marks_missing_geometry_as_unknown(mock) -> None:  # type: ignore[no-untyped-def]
    # Remove stackup geometry so impedance calculation for CLK degrades to "unknown"
    project = mock.project
    original_layers = project.get("stackup", {}).get("layers")
    if "stackup" in project:
        project["stackup"]["layers"] = []

    try:
        out = _payload(await kc_impedance_check.handler({"project_id": "p1"}))
        assert out["ok"] is True
        clk = next(r for r in out["results"] if r["net"] == "CLK")
        assert clk["achieved_ohm"] is None
        assert clk["status"] == "unknown"
    finally:
        # Restore original geometry so other tests see the default project
        if "stackup" in project:
            project["stackup"]["layers"] = original_layers


A self-contained `httpx.MockTransport` stands in for kiserver +

```

If `mock.project` is structured differently (e.g. stackup stored under another key, or `layers` is not a direct child of `stackup`), adjust the `project["stackup"]["layers"]` access accordingly to ensure the geometry removal actually triggers the `"unknown"` status path in `kc_impedance_check`.
</issue_to_address>

### Comment 6
<location path="services/mcp/tests/test_mpn_resolve.py" line_range="72-77" />
<code_context>
+    assert "required" in out["error"]
+
+
+async def test_shape_rejection_is_not_found() -> None:
+    out = _payload(await kc_mpn_resolve.handler({"mpn": "!!!"}))
+    assert out["ok"] is True
+    assert out["found"] is False
+    assert out["reason"] == "not_an_mpn_shape"
+    assert out["confidence"] == 0.0
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend shape-rejection coverage to exercise the `missing_digit_or_letter` path

The current test input (`"!!!"`) only hits the `not_an_mpn_shape` path because it doesn’t match `_MPN_SHAPE_RE` at all. Please add a second case that matches the regex but lacks either a digit or a letter (e.g. `"ABCDEF"` or `"123456"`) and assert that it returns the `missing_digit_or_letter` reason, so that branch remains covered and stable.

```suggestion
async def test_shape_rejection_is_not_found() -> None:
    # Case 1: does not match the MPN shape at all
    out = _payload(await kc_mpn_resolve.handler({"mpn": "!!!"}))
    assert out["ok"] is True
    assert out["found"] is False
    assert out["reason"] == "not_an_mpn_shape"
    assert out["confidence"] == 0.0

    # Case 2: matches the MPN shape but is missing a digit or a letter
    out_missing = _payload(await kc_mpn_resolve.handler({"mpn": "ABCDEF"}))
    assert out_missing["ok"] is True
    assert out_missing["found"] is False
    assert out_missing["reason"] == "missing_digit_or_letter"
    assert out_missing["confidence"] == 0.0
```
</issue_to_address>

### Comment 7
<location path=".claude/commands/snapshot.md" line_range="10-3" />
<code_context>
+  - mcp__kiclaude__kc_snapshot_create
+---
+
+# /snapshot — pin a revertable checkpoint
+
+A snapshot is a cheap "save point" before a risky edit (a big
</code_context>
<issue_to_address>
**nitpick (typo):** Consider correcting "revertable" to "revertible" (or "reversible") in the snapshot docs.

"Revertable" appears in both the heading and description and is a nonstandard spelling. Consider using "revertible snapshot" / "revertible checkpoint" (or "reversible") to avoid it reading like a typo in user-facing docs.

Suggested implementation:

```
description: Create a named, revertible snapshot of the current KCIR project state via kc_snapshot_create, so a later /revert can roll back to exactly this point. Read-only with respect to the .kicad_* files — snapshots live in the content-addressed store, not the board.

```

```
# /snapshot — pin a revertible checkpoint

```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +78 to +86
aggregator = sourcing._factory()
try:
part = await aggregator.price(raw_mpn, qty=1)
found = bool(getattr(part, "quotes", None))
stock = _pricing_to_payload(part)
except Exception as e:
stock = {"quotes": [], "error": f"distributor lookup failed: {e}"}
finally:
with contextlib.suppress(Exception):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Handle potential failures in creating the distributor aggregator explicitly.

If sourcing._factory() raises (e.g., due to misconfiguration), aggregator is never assigned and the finally block will raise when trying to await aggregator.aclose(), masking the real error. Consider initializing aggregator: sourcing.Aggregator | None = None before the try and only calling aclose() when it’s not None to avoid this secondary exception.

Comment on lines +24 to +27
function kindForFile(name: string): "symbol" | "footprint" | null {
if (name.endsWith(".kicad_sym")) return "symbol";
if (name.endsWith(".kicad_mod")) return "footprint";
return null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (bug_risk): Normalize filename case when inferring library kind from extension.

These checks are case-sensitive, so files like CUSTOM.KICAD_SYM or Foo.KiCad_Mod will be treated as unsupported. Consider lowercasing name before the .endsWith checks to accept extensions in any case.

Comment thread services/mcp/tests/test_validate_kc.py
Comment on lines +175 to +176
def _imp_project(width: float, target: float, stackup: dict[str, Any] | None = _STACKUP):
return _project(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Consider a KC040 test where the net class exists but has no width, to pin the unknown/warning path

Current KC040 tests cover normal, off-target, and missing-stackup cases, plus _microstrip_z0 monotonicity. There’s also a path where _net_class_width returns None (net class present but no track_width_*), which yields a “missing geometry” warning. Please add a test that creates a project with a net class lacking width to lock in and document this behaviour.

Comment on lines +122 to +128
async def test_impedance_check_returns_per_net_result(mock) -> None: # type: ignore[no-untyped-def]
out = _payload(await kc_impedance_check.handler({"project_id": "p1"}))
assert out["ok"] is True
clk = next(r for r in out["results"] if r["net"] == "CLK")
assert clk["target_ohm"] == 50.0
assert clk["achieved_ohm"] is not None
assert clk["status"] in {"ok", "warning", "error"}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Add a kc_impedance_check test for the status='unknown' / missing-geometry case

kc_impedance_check has an explicit status: "unknown" branch when Er/height or width are missing. Please add a test that adjusts the mock project (e.g. clears stackup.layers or removes track_width_mm) and asserts a result with achieved_ohm is None and status == "unknown" to cover this degradation path.

Suggested implementation:

async def test_partition_check_flags_double_ground_bridge(mock) -> None:  # type: ignore[no-untyped-def]
    out = _payload(await kc_partition_check.handler({"project_id": "p1"}))
    assert out["ok"] is True
    assert any(f["code"] == "KC050" for f in out["violations"])


async def test_impedance_check_marks_missing_geometry_as_unknown(mock) -> None:  # type: ignore[no-untyped-def]
    # Remove stackup geometry so impedance calculation for CLK degrades to "unknown"
    project = mock.project
    original_layers = project.get("stackup", {}).get("layers")
    if "stackup" in project:
        project["stackup"]["layers"] = []

    try:
        out = _payload(await kc_impedance_check.handler({"project_id": "p1"}))
        assert out["ok"] is True
        clk = next(r for r in out["results"] if r["net"] == "CLK")
        assert clk["achieved_ohm"] is None
        assert clk["status"] == "unknown"
    finally:
        # Restore original geometry so other tests see the default project
        if "stackup" in project:
            project["stackup"]["layers"] = original_layers


A self-contained `httpx.MockTransport` stands in for kiserver +

If mock.project is structured differently (e.g. stackup stored under another key, or layers is not a direct child of stackup), adjust the project["stackup"]["layers"] access accordingly to ensure the geometry removal actually triggers the "unknown" status path in kc_impedance_check.

Comment thread services/mcp/tests/test_mpn_resolve.py
@@ -0,0 +1,34 @@
---
name: snapshot
description: Create a named, revertable snapshot of the current KCIR project state via kc_snapshot_create, so a later /revert can roll back to exactly this point. Read-only with respect to the .kicad_* files — snapshots live in the content-addressed store, not the board.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (typo): Consider correcting "revertable" to "revertible" (or "reversible") in the snapshot docs.

"Revertable" appears in both the heading and description and is a nonstandard spelling. Consider using "revertible snapshot" / "revertible checkpoint" (or "reversible") to avoid it reading like a typo in user-facing docs.

Suggested implementation:

description: Create a named, revertible snapshot of the current KCIR project state via kc_snapshot_create, so a later /revert can roll back to exactly this point. Read-only with respect to the .kicad_* files — snapshots live in the content-addressed store, not the board.

# /snapshot — pin a revertible checkpoint

LayerDynamics and others added 9 commits May 25, 2026 18:14
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@LayerDynamics LayerDynamics merged commit 7010d4d into main May 25, 2026
1 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant