fix(ipc): find KiCad's Windows endpoint in the pipe namespace - #530
Conversation
|
Filed the documented limit as #531, with a measured self-test rather than a theoretical note. Binding KiCad's default pipe name before KiCad starts does capture Konnect's connection — but the squatter speaks no NNG, so the bounded The cheapest real improvement that came out of it is not a security check at all: Konnect can already distinguish "no pipe bound" from "a pipe answered and was not KiCad", and today both render as the transport being unreachable. That is on #531 as the recommended first step. Nothing in this PR changes the trust model — an explicitly configured address always carried the same exposure — so I do not think it blocks the merge. |
neusse
left a comment
There was a problem hiding this comment.
One release-classification correction is needed before this can enter the merge queue.
- Change the new
docs/API_MIGRATIONS.mdheading from minor release to patch release. This PR fixes Windows endpoint discovery and changes no tool, argument, or response shape;GOVERNANCE.mdclassifies that as a patch.
Everything else on exact head f67fe0666b8f800d7c3901365dd8f39fc1811745 matches #529: it is based on current main, the Windows namespace probe does not connect to the endpoint, the live Windows evidence covers default discovery, all ten required checks pass, and Closes #529 is correct. Please make that focused wording correction and let CI rerun on the new head.
f67fe06 to
709267f
Compare
|
Corrected. The heading now reads |
neusse
left a comment
There was a problem hiding this comment.
The patch-release correction resolves the earlier requested change. Spec and standards review found no remaining implementation defect at exact head 709267fc8eb4af4b05ef2d053a88d8bed96b2fd9.
This approval is for the reviewed diff only; the PR is not merge-ready until current main is incorporated as noted below.
|
Please rebase this exact head onto current This remains first in the least-resistance train: #530 -> #528 -> #523 -> #535. Once the refreshed head is green, it needs only the final exact-head check before merge. |
NNG maps `ipc://` to a named pipe on Windows, and that pipe has no filesystem presence: with pcbnew running, `%TEMP%\kicad\api.sock` does not exist while `\\.\pipe\%TEMP%\kicad\api.sock` does. Discovery probed the candidate as a file, so `is_adoptable` answered "no" for every Windows install and `detect_ipc_address` could never succeed there. The consequence was not a missing convenience. A Konnect launched by an MCP client — the shipped PCM package registered with a client, carrying no `ipc_address` and no `KICAD_API_SOCKET` — reported the transport unreachable with KiCad running and the API server enabled, so every hybrid tool took its direct-file fallback while KiCad held the board open. That is the one outcome `attempt_ipc_write`'s gate exists to prevent, reached because discovery could not see a transport that was in fact reachable. Live-only tools refused outright. Windows now looks the same candidate up in the pipe namespace. `FindFirstFileW` over `\\.\pipe` lists bound names without opening any of them, so the metadata-only rule #498 made non-negotiable still holds — `std::fs::metadata` would not have done, since on Windows it opens a handle and against KiCad's endpoint that consumes a server instance. `file_name()` returns the bound name verbatim, which is the candidate path, so the candidate list is unchanged. Matching is case-insensitive because Windows paths are. The pipe namespace is machine-global and any local account can bind a name in it, so a squatter could be adopted; the Windows answer to that needs the pipe's security descriptor, which means opening it. An explicitly configured address has always carried the same exposure, so the doc comment states the limit rather than implying an ownership check that is not there. The test that asserted Windows adopts nothing encoded the bug and is replaced. New coverage: a bound name is adopted, an absent one and an unreadable namespace are not, matching ignores case, and — against a real `tokio` pipe server whose name embeds a path that does not exist on disk — detection finds it and does not become its client, the Windows counterpart of `detection_never_connects_to_a_candidate`. One `#[ignore]` live test asserts the property the issue is about: with KiCad running, detection succeeds with no configuration at all. Verified live here (KiCad 10.0.5, Windows 11). Before: `open_project` reported `ipc_available: false` with an empty endpoint. After, same binary invocation with no config file and no `KICAD_*` variables: endpoint discovered, `ipc_available: true`, `kicad_ui_running: true`, one open board. Closes #529 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
709267f to
705a30d
Compare
|
Rebased onto current All ten required checks green on Ready for the final exact-head check. |
Summary
NNG maps
ipc://to a named pipe on Windows, and a named pipe has no filesystem presence. Discovery probed the candidate path as a file, sois_adoptableanswered "no" for every Windows install anddetect_ipc_addresscould never succeed there.The consequence was not a missing convenience. A Konnect launched by an MCP client — the shipped PCM package registered with a client, carrying no
ipc_addressand noKICAD_API_SOCKET— reported the transport unreachable with KiCad running and the API server enabled, so every hybrid tool took its direct-file fallback while KiCad held the board open. That is the one outcomeattempt_ipc_write's gate exists to prevent, reached because discovery could not see a transport that was in fact reachable. Live-only tools refused outright.Closes #529
Approach
The module already said what the fix was — "Inspecting the pipe … is the real answer and is left for a change that can be tested on Windows" — so this is that change, made on a Windows machine with KiCad 10.0.5.
FindFirstFileWover\\.\pipelists bound names without opening any of them, so the metadata-only rule IPC socket auto-detection wedges KiCad's API server on Linux (no reply to any client until restart) #498 made non-negotiable still holds.std::fs::metadatawould not have done: on Windows it opens a handle, and against KiCad's endpoint that consumes a server instance.file_name()returns the bound name verbatim — drive letter and backslashes intact — which is exactly the candidate path, so the existing candidate list and theformat_addressoutput are unchanged.std::fs::read_dirdoes the enumeration; nowindows-sys/winapiis added.The limit, stated rather than implied
The pipe namespace is machine-global and any local account may bind a name in it, so a squatter could be adopted. Unix answers that with an ownership check; the Windows equivalent needs the pipe's security descriptor, which means opening it. An explicitly configured
ipc_addresshas always carried the same exposure, so the doc comment records this as the state of the art on the platform rather than implying a check that is not there. Happy to split it out as a follow-up if you want it pursued.Branch and dependencies
Base branch:
mainatf0f5ad0.Depends on: nothing. Overlap: none —
crates/konnect-ipc/src/socket.rsplus three docs; no open PR touches them.Series order: none — single PR, one commit.
Next PR to promote after this one: none in this chain.
Compatibility and safety
source: "file"withfallback_reason.kind: transport_unreachablenow reportsource: "ipc"when KiCad is running with the API server enabled, and their edits go through KiCad instead of to the saved file. Live-only tools start working.get_installation_inforeports the discovered endpoint instead of a null one. Recorded indocs/API_MIGRATIONS.md.docs/TROUBLESHOOTING.mdstill says so.cargo xtask fix-doc-counts --checkunchanged. Rollback: revert the one commit.Validation
Run on commit
f67fe06(Windows 11, KiCad 10.0.5), exit codes captured directly:cargo fmt --all -- --checkcargo test --workspace --locked --lib --testscargo test --workspace --locked --doccargo clippy --workspace --locked --all-targets -- -D warnings#[ignore]live test and the end-to-end run below, both against KiCad 10.0.5 on this machine. The Unix branch is unchanged and CI covers its matrix; the new code is#[cfg(windows)], so the Windows leg of CI is what exercises it there.Reproduced first, with the released v0.11.1 PCM binary, no config file and no
KICAD_*variables, pcbnew open on a board:And the mechanism, measured directly with pcbnew running:
Negative controls (each neutered, then restored):
detection_finds_a_live_pipe_without_connecting_to_itdetection_finds_a_live_pipe_without_connecting_to_itBoth neuters are caught by that one test, and deliberately so: it is the only one that goes through the production
is_adoptable. The other four state what is bound rather than binding it, so they exerciseis_adoptable_amongdirectly and survive either neuter — which is what makes the live-pipe test load-bearing rather than decorative.Live, on this branch — same binary invocation as the reproduction, still no config and no
KICAD_*:Review checklist
upstream/main, has no merge conflicts, and CI passed on this exact head.upstream/main, not a release tag.docs/NAMING_CONVENTIONS.md(PIPE_NAMESPACE,pipe_names,is_adoptable_among); no public renames.cargo xtask fix-doc-counts --checkunchanged.Maintainer merge state
status:*workflow label.status:ready-to-mergeapplies to this exact head SHA.mainruleset.gh pr merge N --mergeafter final verification.🤖 Generated with Claude Code