oaslananka/kicad-mcp-pro/code-review

KiCad MCP Pro Code Review

Review pull requests for correctness, security, contract stability, compatibility, test coverage, and release safety. Prefer a small number of high-confidence, actionable findings over broad style feedback.

KiCad MCP Pro is a production MCP server that drives real KiCad projects. Treat unsafe mutations, incorrect tool contracts, path/subprocess mistakes, protocol regressions, and misleading engineering verdicts as high-risk.

Review stance

• Review the changed code and the behavior introduced by the change.
• Comment on unchanged code only when the PR newly makes an existing defect reachable

or materially increases its impact.

• Do not invent failures, tool behavior, KiCad behavior, CI results, or browser evidence.
• Distinguish a demonstrated defect from a possible improvement.
• Prefer correctness and user-impact findings over formatting or naming nits already

enforced by repository tooling.

• Do not require a broad refactor when a focused fix addresses the defect.
• If no actionable defect is found, do not manufacture a comment.

1. Gather review context first

Before writing findings:

1. Read the PR title, description, changed files, and diff.
2. Use GitHub MCP context when available to inspect:

• linked issues or incidents referenced by the PR;
• review-relevant PR metadata;
• changed-file scope;
• workflow/check status;
• failed job logs when a failure is relevant to the changed code.

1. Consult repository policy and architecture when needed:

• ARCHITECTURE.md
• CONTRIBUTING.md
• SECURITY.md
• docs/development/coding-standards.md
• docs/development/testing-policy.md
• docs/security/threat-model.md
• .github/PULL_REQUEST_TEMPLATE.md

1. Treat the PR head branch as the source of repository instructions and skill content.
2. Use existing CI evidence when it is from the reviewed head commit and covers the

exact concern. Do not claim a check passed if you did not observe it.

2. MCP context policy

GitHub MCP

Use GitHub MCP tools when they provide concrete review evidence, especially for linked issues, PR intent, CI/check state, or failed workflow details. Do not browse unrelated repository history just to increase context.

Playwright MCP

When changes affect the dashboard or desktop/web UI and the application can be run in the review environment, use Playwright MCP for focused user-visible verification.

Relevant areas include:

• src/kicad_mcp/web/
• dashboard routes/templates/assets
• src-tauri/frontend/
• desktop flows that depend on the local dashboard

Do not report a browser regression unless you have code evidence or observed behavior. Lack of a runnable browser environment is not itself a defect.

KiCad MCP Pro MCP server

If a KiCad MCP Pro server is configured in repository Copilot settings and the PR changes KiCad-facing behavior, .kicad_* fixtures, validation logic, or project inspection behavior, use it only when it adds concrete evidence.

Prefer the bounded read-only/default review surface and inspection/validation tools such as:

• kicad_get_project_info
• kicad_get_version
• project_quality_gate
• schematic_quality_gate
• pcb_quality_gate
• run_erc
• run_drc
• pcb_get_board_summary
• sch_get_symbols

During code review, do not invoke write, destructive, manufacturing, export, or mutation tools. Never change a user's board or schematic as part of review.

3. Architecture review

KiCad MCP Pro has a five-layer design. Preserve the boundaries described in ARCHITECTURE.md.

KiCad adapter seam

Flag changes that bypass the KiCad adapter seam or spread KiCad-version fragility into pure domain code.

KiCad-facing behavior belongs behind the established seams, including:

• src/kicad_mcp/kicad/
• src/kicad_mcp/connection.py
• src/kicad_mcp/ipc/
• src/kicad_mcp/discovery.py
• thin tool adapters that delegate to stable domain/services

Pure deterministic logic should remain testable without KiCad and should not import KiCad-specific runtime internals unnecessarily.

Composition roots

For tool registration and validation composition:

• keep register() and _register_* helpers focused on wiring;
• do not add new business logic directly to large registration/composition roots;
• preserve the repository's architecture-boundary checks;
• prefer typed domain services plus thin MCP adapters.

Real-state rule

The server must not invent board, schematic, DRC/ERC, manufacturing, or KiCad state. Behavior should be based on real KiCad engines, real project files, or explicitly documented deterministic/heuristic calculations.

If a calculation is approximate, heuristic, partial, or first-pass, ensure the API, docstring, docs, and verdict communicate that limitation. Do not allow code or docs to present first-order estimates as formal engineering sign-off.

4. MCP tool and public contract review

Treat the MCP surface as a public compatibility contract.

When a tool is added, removed, renamed, reclassified, or its schema/behavior changes, check the relevant contract surfaces:

• implementation in the appropriate domain/tool module;
• TOOL_CATEGORIES and profile membership in src/kicad_mcp/tools/router.py;
• experimental classification when applicable;
• tool annotations/metadata in src/kicad_mcp/tools/metadata.py;
• read-only/destructive/headless/requires-KiCad semantics;
• operating-mode restrictions;
• generated tool documentation;
• tool-surface snapshots and contract tests;
• profile/toolset/adapter/compatibility matrices when affected.

Flag any change that silently exposes mutating or destructive behavior through a read-only/default review surface.

Preserve stable error behavior. Agent-visible failures should use the repository's typed error model and stable error codes rather than leaking raw implementation exceptions or ambiguous strings.

For transport/protocol changes, review:

• MCP initialization and protocol compatibility;
• stdio and Streamable HTTP behavior;
• session/header behavior;
• discovery endpoints;
• backward-compatibility implications;
• explicit handling of unsupported client/KiCad capabilities.

5. Security review

Security findings take priority over style or convenience.

Subprocess and command execution

Flag:

• shell=True;
• os.system, os.popen, or equivalent shell execution in production paths;
• string-built shell commands containing user-controlled data;
• untrusted values interpreted as CLI flags unintentionally;
• missing error handling around subprocess boundaries.

KiCad CLI calls should use discrete argv elements with shell=False.

Filesystem and path safety

Treat MCP/tool arguments, project paths, filenames, output paths, and imported artifact names as attacker-controlled.

Check for:

• canonicalization before access;
• confinement to the project/workspace root;
• .. traversal;
• absolute-path escapes;
• symlink escapes;
• Windows drive/UNC path edge cases;
• unsafe file extensions where an allowlist is expected;
• TOCTOU risks before destructive mutations.

Security-sensitive filesystem/subprocess changes need negative tests for hostile inputs and failure paths.

HTTP and local service boundaries

For Streamable HTTP, dashboard, bridge, or auth changes, verify the intended local and authentication boundaries are preserved, including:

• bearer-token enforcement when configured;
• origin validation;
• CORS allowlists;
• localhost-only assumptions where required;
• stateful/stateless session policy;
• no silent re-enabling of deprecated/legacy exposure.

Secrets and private design data

Flag code, tests, logs, fixtures, screenshots, or workflow artifacts that can expose:

• credentials or API tokens;
• private board/schematic data;
• generated Gerbers/netlists/manufacturing files;
• customer-specific paths or logs.

Destructive behavior

Any destructive or KiCad-mutating operation must be explicit, policy-gated, test-covered, and documented. A convenience path must not bypass operating-mode or human-approval boundaries.

GitHub Actions

For .github/workflows/**, check:

• third-party actions are pinned to full commit SHAs;
• default permissions remain least-privilege;
• write permissions are scoped to the job that needs them;
• untrusted GitHub expressions are passed through env: before shell use;
• PR-controlled values do not reach shell commands unsafely;
• release credentials use GitHub/OIDC/trusted-publishing patterns where applicable;
• artifact, checksum, SBOM, signing, and attestation steps are not weakened silently.

If a public PR introduces a sensitive vulnerability, explain the fix-relevant risk without publishing weaponized exploit details, secrets, or private data. Follow SECURITY.md for disclosure handling.

6. Testing and regression review

Behavior changes require evidence at the narrowest useful layer.

Check the repository's testing policy:

• pure Python logic -> unit tests and type checks;
• KiCad artifact parsing -> representative fixture tests;
• MCP tool contract changes -> metadata/generated-doc checks and surface/contract tests;
• filesystem/subprocess changes -> hostile-input and failure-mode tests;
• bug fixes -> regression test when reproducible;
• workflow/release changes -> workflow-security and dry-run/metadata checks;
• docs-only changes -> docs/link-sensitive review rather than unrelated runtime tests.

For tests that create or mutate Git repositories, verify test-owned temporary directories and isolated Git configuration are used so contributor/system hooks and configuration cannot leak into fixtures.

Do not demand live KiCad execution for logic that is deliberately unit-testable without KiCad. Conversely, do not accept mocked-only evidence when the change specifically depends on a live KiCad integration contract and the repository has a dedicated KiCad-enabled CI path for it.

7. Generated files and canonical metadata

Identify the source of truth before commenting on generated surfaces.

Important repository rules include:

• pyproject.toml is a canonical package metadata/version source;
• compatibility.yaml is a canonical KiCad/MCP support-policy source;
• server.json and several public surfaces are generated/synchronized;
• generated tool docs, parity/toolset/profile/adapter outputs should be regenerated

from their canonical inputs rather than hand-edited.

Flag generated drift, but do not ask contributors to hand-edit generated files when the repository generator is authoritative.

8. Packaging, release, and desktop review

For Python/npm/container/release changes, check:

• package metadata remains internally consistent;
• npm wrapper behavior/version policy does not drift from the Python package contract;
• lockfile changes match dependency changes;
• release checks still run before publishing;
• trusted publishing / provenance controls are not bypassed;
• public compatibility claims match compatibility.yaml;
• generated registry/container metadata stays synchronized.

For Tauri/desktop changes, also review:

• backend/frontend version compatibility and startup handshake;
• local server lifecycle and port/bind assumptions;
• permission/capability changes;
• error handling when the backend cannot start or does not match;
• user-visible flows with web/GUI tests when practical.

9. Focused validation commands

Prefer the smallest relevant validation set. Use CI evidence instead of re-running a check only when the reviewed head commit already ran the same check successfully.

Baseline Python/server changes

corepack pnpm run format:check
corepack pnpm run lint
corepack pnpm run typecheck
corepack pnpm run test:unit

Public MCP/tool/metadata/compatibility changes