Skip to content

fix: avoid injecting unsupported --variant flag into kicad-cli#46

Closed
ril3y wants to merge 2 commits intooaslananka:mainfrom
ril3y:fix/cli-variant-flag-injection
Closed

fix: avoid injecting unsupported --variant flag into kicad-cli#46
ril3y wants to merge 2 commits intooaslananka:mainfrom
ril3y:fix/cli-variant-flag-injection

Conversation

@ril3y
Copy link
Copy Markdown

@ril3y ril3y commented May 7, 2026

Summary

_active_variant_args() was injecting --variant default into every
kicad-cli invocation when no variant override was active, because the
default sidecar state ships with active_variant: "default". KiCad CLI
9.x doesn't recognize --variant and aborts with Unknown argument: --variant. The error went to stdout, not stderr, so the wrapper's
stderr or 'unknown error' collapsed to unknown error and masked the
root cause.

PCB and schematic exports failed in lockstep against KiCad 9.0.7 on
Windows. Reproducible with any 9.x kicad-cli.

Changes

  • discovery.py — add supports_cli_variant capability flag, populated
    from "--variant" in <kicad-cli help blob>. KiCad 10 introduced the flag.
  • tools/export.py:_active_variant_args — suppress the synthetic no-op
    --variant default unconditionally; for explicit non-default variants,
    gate emission on caps.supports_cli_variant.
  • tools/export.py:_export_sch_pdf — fall back to stdout when stderr is
    empty so future arg-parse errors surface in the user-visible message.
  • Tests — set supports_cli_variant=True on the mock cap in
    test_export_tools.py and test_release_hardening.py so the existing
    --variant lite forwarding assertions continue to exercise the
    KiCad-10 path.

Test plan

  • pytest -k export — 34 passing
  • Full suite + coverage gate (npm run check) — 90.15% coverage
  • Repro: export_sch_pdf against a real project on KiCad 9.0.7 —
    fails on main, succeeds on this branch (PDF written, 1.78 MB)
  • No behavior change on KiCad 10 CLIs — variant flag still forwarded
    when capability probe sees it

Notes on local CI

Three integration tests fail on KiCad 9.0.7 environments regardless of
this branch:

  • test_kicad_cli_smoke.py::test_kicad_cli_version_is_discoverable
    asserts KICAD_MAJOR_VERSION (10).
  • test_kicad_cli_smoke.py::test_kicad_cli_exports_board_stats_without_gui
    uses pcb export stats, a KiCad-10-only subcommand.
  • test_pcb_tools.py::test_pcb_design_blocks_and_inner_layer_graphics_success
    fails on a stackup-validation drift (asserts inner-layer add succeeds,
    but the implementation now requires >=4 copper layers; the fixture
    has 3).

All three fail identically on main in the same environment. They are
unrelated to this change. They should pass on a KiCad-10 CI environment.

KiCad CLI 9.x rejects --variant; the synthetic "default" variant
added it to every invocation, breaking schematic and PCB exports.
Gate emission on a new supports_cli_variant capability probe and
suppress the no-op default unconditionally. Surface stdout when
stderr is empty so future arg-parse errors are readable.

* discovery.py: probe --variant in kicad-cli help
* tools/export.py: gate variant args, widen error message
* tests: set supports_cli_variant=True on mock caps
@ril3y ril3y requested a review from oaslananka as a code owner May 7, 2026 00:09
Copy link
Copy Markdown

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

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 support for the --variant flag in the KiCad CLI, which is available in KiCad 10+. It includes capability detection for this flag and updates the export logic to use it when appropriate, while suppressing it for the 'default' variant. Error reporting for schematic PDF exports is also improved by including standard output in failure messages. Feedback suggests raising a ValueError instead of silently ignoring requested variants when the CLI lacks support, ensuring users are aware of compatibility issues that could lead to incorrect exports.

Comment thread src/kicad_mcp/tools/export.py Outdated
Comment on lines +121 to +122
if not caps.supports_cli_variant:
return []
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Silently returning an empty list when a non-default variant is requested but unsupported by the CLI can lead to serious correctness issues. If a user (or the sidecar state) has specified a variant like lite, and the system silently exports the default (full) design because the CLI is too old, the user may inadvertently manufacture incorrect boards.

Since _active_variant_args already raises ValueError for invalid variant names, it should also raise a ValueError here to explicitly inform the user that their requested variant cannot be applied due to CLI version constraints.

Suggested change
if not caps.supports_cli_variant:
return []
if not caps.supports_cli_variant:
raise ValueError(
f"The detected KiCad CLI does not support the '--variant' flag. "
f"Cannot apply variant '{args[1]}'."
)

Copy link
Copy Markdown
Owner

@oaslananka oaslananka left a comment

Choose a reason for hiding this comment

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

Please raise ValueError when caps.supports_cli_variant is False and a non-default variant is active, as suggested by Gemini. Silent suppression risks exporting wrong board variant.

Code review on PR oaslananka#46 flagged that silently returning [] when a non-default
variant is requested but the local kicad-cli does not support --variant
risks manufacturing the wrong board: the user asks for "lite", the export
quietly produces the default variant. Replace the silent suppression with
an explicit ValueError pointing at variant_set_active() as the recovery
path. The synthetic --variant default case still drops the flag (it adds
no overrides, so producing the default board is correct). Add unit tests
covering all three paths: raise on unsupported CLI + non-default variant,
suppress synthetic default, and pass-through on KiCad-10+.
@oaslananka
Copy link
Copy Markdown
Owner

Thanks for the KiCad 9 kicad-cli --variant fix.

This repository is now a personal showcase mirror; the canonical repository is:

https://github.com/oaslananka-lab/kicad-mcp-pro

I ported this fix into the canonical org repo and merged it as:

oaslananka-lab/kicad-mcp-pro#12

Canonical merge commit:

b2b7537

The fix keeps --variant default from being injected into KiCad 9.x CLI calls, gates explicit non-default variants on detected CLI support, and preserves KiCad 10 variant behavior.

Closing this PR here to avoid personal-repo divergence. Future changes should target:

https://github.com/oaslananka-lab/kicad-mcp-pro

@oaslananka oaslananka closed this May 7, 2026
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.

2 participants