GUI-driver: let the MCP drive KiCad's GUI (menus, toolbars, dialogs, plugin buttons) - #333
GUI-driver: let the MCP drive KiCad's GUI (menus, toolbars, dialogs, plugin buttons)#333rossvonfange wants to merge 6 commits into
Conversation
mixelpixx
left a comment
There was a problem hiding this comment.
This is competent work and your PR body is admirably honest about the rough edges — the ⚠ flagging, the "no gate yet" note, and the empty DESTRUCTIVE_ACTIONS comment all tell the reader the truth. I want to be equally direct: the blocker is not code quality, it is that the current security model is not one I can put in front of users. Everything below is fixable, and I would review a revision promptly.
First the good news, because it is genuinely unusual: zero regression risk. 11 of 13 files are new and the 3 modified files take additive-only hunks. The gui_driver.py:30 claim that it never imports kipy/pcbnew holds in the code. And tests/test_gui_driver.py is 531 lines with real loopback protocol coverage and both ellipsis forms — more rigour than most PRs this size.
Blocking
1. The control channel is unauthenticated and starts itself.
listener.py binds 127.0.0.1:8770 with no token or handshake, and gui_driver_plugin/plugins/__init__.py starts it as an import side effect of KiCad's 3rd-party plugin scan — its own docstring says so. So once the helper is installed, any local process on the machine can drive the user's KiCad GUI, with no MCP server involved at all. On a shared or multi-user box, or alongside anything else running locally, that is a privilege boundary this project does not currently cross anywhere else.
Needed: a token minted per session, required on every request, handed to the MCP side out of band (a file in the KiCad config dir with restrictive permissions is the usual shape). And the listener should start on explicit opt-in, not on plugin scan.
2. kicad_gui_click has no gate on destructive items — the flagging is advisory only.
I traced the click path: kicad_gui_click resolves the name, builds the request and calls self._call. It never consults DESTRUCTIVE_SEED_LABELS or DESTRUCTIVE_ACTIONS. Those are used to annotate the tree output, so an agent that ignores the annotation — or never fetches the tree, since id alone is accepted — activates a destructive item with nothing in the way.
Combined with the coverage gap this is the part that worries me most:
DESTRUCTIVE_SEED_LABELSis 15 entries against your own documented 215-item enumeration. Missing at minimum: Global Deletions, zone fill/unfill, Save As, Reset to Defaults.DESTRUCTIVE_ACTIONSisfrozenset()— empty, for the reason you document.docs/gui_destructive_seed.txtis not read at runtime; the list is hardcoded. So the docs file only appears to drive the behaviour, and the two can drift silently with nothing to catch it.
Needed: kicad_gui_click must refuse a flagged item unless the caller passes an explicit override (confirmDestructive: true or similar), and the seed list should be loaded from the file at runtime so there is one source of truth — or, if embedding is deliberate for packaging, a test asserting the file and the constant agree.
3. Silent writes into the user's KiCad installation.
ensure_helper_installed() copytrees the plugin into every discovered 3rdparty/plugins/ directory on the first failed connect. An MCP tool call that mutates the user's KiCad configuration as a side effect needs to be opt-in and say so.
Non-blocking but worth fixing
- Localised UIs break silently in the worst direction. Menu matching is by display label, so a non-English KiCad fails name resolution and stops flagging destructive items — the guard degrades to nothing rather than to an error.
- Tools are not registered in
src/tools/registry.ts, so all 10 are invisible to the tool router. - Overlaps with existing tools:
kicad_run_drcvsdesign_rules.py'srun_drc/get_drc_violations;kicad_pcb_snapshot/kicad_gui_screenshotvsget_board_2d_view;kicad_reload_and_open_plugin/launch_kicad_uivssrc/tools/ui.ts. Worth trimming to the ones that only the GUI path can do. - The 15 s
UI_CALL_TIMEOUTwithwx.CallAfter: your docstring admits the call may still run after timeout, so a partially-applied GUI action has no rollback. Not fixable in general, but it argues for the destructive gate being strict.
The bigger question
Your own PR body raises shipping this as a standalone gui-driver MCP, and I think that is the stronger option. Nothing in it is KiCad-design-specific — it is generic wx/AT-SPI GUI automation that happens to target KiCad. As a separate server it can carry its own security posture and opt-in story without every KiCAD-MCP-Server user inheriting a localhost control channel. This repo's whole architecture is deliberately file-and-CLI level precisely so it cannot break a running GUI.
If you would rather keep it here, the three blocking items above are the bar. Either way I would like the kicad_run_drc overlap resolved before it lands.
One process note: #333 and #314 hard-conflict with each other at two identical insertion points in python/kicad_interface.py (import block after commands.library_symbol; command-object block after self.export_commands) plus two overlapping spots in src/server.ts. Whichever lands second needs a rebase — your call which. Also note main moved substantially today (#334 added real Python CI, and #335 moved longRunningCommands into a new src/command-timeout.ts), so rebase before the next push.
|
Heads-up, and apologies for the churn: #339 just landed a repo-wide formatting normalization, and it has put this PR into conflict. That is my doing, not anything wrong with your branch. The conflicts are formatting-only — black, isort and prettier over Resolving it should be mechanical: git fetch origin
git rebase origin/main # or merge, whichever you prefer
# for any conflicted hunk, either side is fine -- the difference is whitespace
npm run format:py # re-applies black + isort over python/ and tests/
npx prettier --write . # if the conflict is in src/If a conflict looks like more than whitespace, that is worth flagging to me — it would mean I got something wrong. Two things changed that affect your next push:
Your outstanding review items are unchanged — nothing in #339 touches the substance of this PR, and I have not re-reviewed anything. Sorry again for the extra rebase. |
|
Revisiting this now the four splits are all merged, and I owe you a narrower review than my first one. I've re-read your DECISIONS section properly. Conceding the flagging argument
Fair, and it's your call. I'm dropping that objection. My original review wanted a curated allow-list, and your measurement is the better argument: 302 actionable items in pcbnew alone, every one a clean I'd still rather What still blocks, and why your rationale doesn't cover itYour argument is about what the agent clicks. These two are about who gets to be the agent — backups don't help, because the user never agreed to the exposure in the first place. 1. The control channel is unauthenticated, and it starts by itself. No token, no handshake, nothing. And
So the moment the helper is installed, any local process — a browser tab running WebAssembly, another user's session on a shared box, any npm postinstall script — can drive the user's KiCad GUI, including the destructive items, with no MCP anywhere in the picture. That's a meaningfully different proposition from "the agent might click Revert." A shared secret written to a mode-0600 file at start, required on every request, closes this. It doesn't constrain the agent at all — which is the point. I'm not asking for gating; I'm asking that the channel only accept the caller you intended. 2. First failed connect writes into the user's KiCad install. into every discovered Both are unchanged as of b4a77ad; that push was the main merge plus formatting, so I don't think you'd got to them rather than disagreeing with them. Also
Worth calibrating: this is not a functional break. Every tool is registered directly and appears in Where this leaves usAdd the socket token and make installation opt-in and I'll merge it. The capability itself is well-built — 11 of 13 files new, additive-only elsewhere, zero regression risk to existing behaviour, and Your own alternative — shipping it as a standalone |
|
Final status call on this PR. The 07-28 review narrowed what is blocking to two items (the injection surface on dialog-driving, and the unsandboxed plugin-button execution path); the flagging disagreement was conceded in your favor and is not in the way. Since then: your last push was 07-27, and there has been no response to the two blockers in three weeks — the grace period announced on 07-26 expired 08-02. Setting a date rather than letting this drift: if the two blockers are unaddressed by 2026-08-27, this closes — as a scope/staleness decision, not a judgment of the work, and reopening with the blockers fixed will get a prompt review. The GUI-driver idea has real value precisely because it reaches things the SWIG/IPC APIs cannot, which is also exactly why its security posture has to be right before it merges. |
Ports the GUI-driver capability from the -loom working tree into the
configured MCP so its tools are callable end-to-end (loom only ever wired
the Python side; the node front never exposed them, so they were never
reachable through the MCP interface).
- python/commands/gui_driver.py: socket client to the in-KiCad helper +
playbooks (pcb_snapshot / reload_and_open_plugin / run_drc) + AT-SPI
Linux shim + self-install (ensure_helper_installed, per-OS plugin dirs,
version-markered idempotent deploy). Graceful-fail contract: every tool
returns {success:false} instantly when the helper isn't reachable.
- gui_driver_plugin/: the bundled in-KiCad helper (listener + wx driver),
root-flatten layout so it installs to <id>/.
- kicad_interface.py: import + init GuiDriverCommands + 10 handler routes.
- schemas/tool_schemas.py: GUI_DRIVER_TOOLS spliced into TOOL_SCHEMAS.
- src/tools/gui-driver.ts + server.ts: registerGuiDriverTools declares the
10 tools on the McpServer (the real served path — index.ts runs server.ts,
not the legacy kicad-server.ts).
- tests/test_gui_driver.py: 38 tests (all green; +38 over base, no new fails).
Kept off feat/upstream-contribution (PR mixelpixx#314) — separate capability.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014jKkKiWihHxFXo2Gns4LnG
run_plugin fired the ActionPlugin via a synchronous ProcessEvent and the
listener waited up to UI_CALL_TIMEOUT (15s) for it to return. A plugin whose
Run() occupies the UI thread — e.g. one that opens a browser or starts a
server — therefore (a) made the listener report a false "UI thread did not
answer" timeout even though the action fired, and (b) is what makes KiCad show
"not responding" while it blocks.
driver.click() gains async_trigger: posts the activation via wx.CallAfter and
returns {triggered, async} immediately, so the control channel is never held
hostage to the plugin's duration. run_plugin uses it. Bumps HELPER_VERSION to
0.0.2 so ensure_helper_installed redeploys the fixed helper. +2 tests (real
driver, fake wx) asserting async routes through CallAfter, sync still doesn't.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014jKkKiWihHxFXo2Gns4LnG
…Atspi import (no stubs)
b4a77ad to
3211192
Compare
Addresses the two blockers from the mixelpixx#333 review: 1. Session token. listener.py mints a per-session secret, writes it to a mode-0600 file in the KiCad data dir, and requires it on every request; the MCP client reads that file and echoes the token. A 0600 file is unreadable by other users and by the browser sandbox, closing the localhost-is-not-a-trust-boundary exposure. The listener also now starts only on explicit opt-in (KICAD_GUI_DRIVER_ENABLE), not as a plugin-scan import side effect. 2. Install is opt-in. The connect path no longer copytrees the helper into the user's KiCad on failure; that runs only under KICAD_GUI_DRIVER_AUTOINSTALL, or via the new explicit install_gui_driver tool. Also registers all GUI-driver tools (+ install_gui_driver) in src/tools/registry.ts so search_tools surfaces them. Tests: TestSessionToken, TestInstallOptIn added; 47 gui-driver tests pass.
|
Both blockers are addressed, and it's rebased onto current 1. Session token on the control channel. 2. Install is opt-in. The connect path no longer CI ratchet. Registered all GUI-driver tools (plus Tests. Added Thanks for the clear bar and for keeping it open — the token + opt-in framing made this straightforward to land. |
Addresses the two blockers from the mixelpixx#333 review: 1. Session token. listener.py mints a per-session secret, writes it to a mode-0600 file in the KiCad data dir, and requires it on every request; the MCP client reads that file and echoes the token. A 0600 file is unreadable by other users and by the browser sandbox, closing the localhost-is-not-a-trust-boundary exposure. The listener also now starts only on explicit opt-in (KICAD_GUI_DRIVER_ENABLE), not as a plugin-scan import side effect. 2. Install is opt-in. The connect path no longer copytrees the helper into the user's KiCad on failure; that runs only under KICAD_GUI_DRIVER_AUTOINSTALL, or via the new explicit install_gui_driver tool. Also registers all GUI-driver tools (+ install_gui_driver) in src/tools/registry.ts so search_tools surfaces them. Tests: TestSessionToken, TestInstallOptIn added; 47 gui-driver tests pass.
3211192 to
3e74125
Compare
…on Tests format gate) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014jKkKiWihHxFXo2Gns4LnG
…ort hook) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014jKkKiWihHxFXo2Gns4LnG
|
Pushed black + isort fixes — CI is green across the board now (22/22, mergeable/clean). Ready when you are. |
GUI-driver: a second channel that lets the MCP drive KiCad's GUI (menus, toolbars, dialogs, plugin buttons)
Two independent commits (
c437395,7deabaf) adding an optional GUI-driver capability to the MCP, developed while building real boards with this server. Branches cleanly off currentmain; cherry-pick freely. Rebased so it does not depend on my other open PR (#314) — this is a separate capability.What this is (and why kipy can't do it)
kipymanipulates design content (board/schematic objects). It cannot see or touch GUI chrome — you can't ask it "is the Open kiHarness button present?", click a menu item, drive a dialog, or trigger a plugin action. The PCB/schematic canvas is one opaque widget to any accessibility layer, so the split is clean:The GUI-driver gives an agent a generic, scriptable handle on the entire GUI: enumerate every menu/submenu/toolbar item by name → id, then activate any of them — a real wx activation, no pixel coordinates. On pcbnew that's ~230 menu entries + ~110 toolbar tools, all addressable by name. This is how you get end-to-end GUI verification (did the button appear? does the dialog do the right thing?) to complement kipy — and it's what diagnosed a plugin-registration bug that kipy simply can't observe.
How it works
A tiny in-KiCad helper (bundled, self-installed) starts a localhost JSON-lines listener on plugin scan. Because wx is not thread-safe, the listener marshals every command to the UI thread via
wx.CallAfter. The MCP tools are thin socket clients. No KiCad C++ changes — the helper uses only public wx/pcbnew surface, the same delivery pattern as any PCM plugin.3rdparty/plugins/<id>/(per-OS path, idempotent via a version marker) and tells you to Refresh Plugins. Bump the version → it redeploys with the MCP.{success:false, error:…}instantly (connection-refused, no hang) when the helper isn't loaded. A dependency that fails cleanly, never a crash.New tools
kicad_gui_treekicad_gui_clickkicad_run_action_pluginRun()occupies the UI thread never blocks the channelkicad_gui_wait_for/kicad_gui_screenshotkicad_pcb_snapshotkicad_reload_and_open_pluginkicad_run_drckicad_gui_tree_atspi/kicad_gui_click_atspiBlast radius — please read
This capability can activate anything in the GUI, including destructive actions. Two deliberate design choices, both flaggable in review:
kicad_gui_treeprepends⚠to destructive item names and setsdestructive:true;kicad_gui_clickexecutes without any confirm/allow-list. The rationale (mine, for my own workflow) is that users back up projects and the prefix is the safeguard — but if you'd want this gated or behind an opt-in flag before it lands upstream, I'm happy to add that.127.0.0.1and is inert unless the helper is installed and KiCad is running.If a generic GUI-driver is out of scope for this server, this could equally live as a standalone
gui-driverMCP (it's useful for any wx/GTK/Qt app). Flagging that option explicitly.Verification
tests/test_gui_driver.py— 40 tests (destructive flagging, name→id resolution, the JSON-lines socket protocol end-to-end over loopback with the real listener + a stub wx executor, self-install per-OS path resolution + idempotent redeploy, graceful-fail contract, and async-trigger routing viawx.CallAfter). Green.main.npm run build(tsc) clean; the 10 tools register throughserver.ts→tools/gui-driver.ts.dist/untracked, not committed.kicad_gui_treereturned the full 232-menu / 110-tool tree with⚠flags;kicad_run_action_plugintriggered a real plugin button end-to-end (launched its web UI). Graceful-fail and the AT-SPI tree dump both confirmed live.Happy to split, gate, or relocate any of this to make it landable.