diff --git a/crates/konnect-core/src/tools/mod.rs b/crates/konnect-core/src/tools/mod.rs index f353f9e1..7b2d3213 100644 --- a/crates/konnect-core/src/tools/mod.rs +++ b/crates/konnect-core/src/tools/mod.rs @@ -260,6 +260,35 @@ macro_rules! tool { }}; } +// ─── IPC helpers ────────────────────────────────────────────────────────────── + +/// Run `f` against KiCad's IPC API, classifying a failure as +/// transport-unreachable vs KiCad-rejected via [`konnect_ipc::IpcFailure`]. +/// +/// This is the typed gate for the file-editing fallback — never a text match +/// on the error message — and it is shared rather than copied per toolset: +/// the toolsets' plain `with_ipc` helpers have already drifted from each +/// other, and this is the one decision (is it safe to edit a board file behind +/// a live KiCad?) whose copies must not. +pub async fn with_ipc_classified( + address: String, + f: F, +) -> anyhow::Result> +where + T: Send + 'static, + F: FnOnce(&konnect_ipc::client::KiCadIpcClient) -> anyhow::Result + Send + 'static, +{ + match tokio::task::spawn_blocking(move || { + f(&konnect_ipc::client::KiCadIpcClient::new(&address)) + .map_err(konnect_ipc::IpcFailure::from_error) + }) + .await + { + Ok(result) => Ok(result), + Err(e) => Err(anyhow::anyhow!("Thread error: {}", e)), + } +} + // ─── Argument helpers ───────────────────────────────────────────────────────── /// Build a structured `InvalidArgument` CallToolResult. Used by the diff --git a/crates/konnect-core/src/tools/pcb_board.rs b/crates/konnect-core/src/tools/pcb_board.rs index 39807050..c799035a 100644 --- a/crates/konnect-core/src/tools/pcb_board.rs +++ b/crates/konnect-core/src/tools/pcb_board.rs @@ -237,7 +237,7 @@ where F: FnOnce(&konnect_ipc::client::KiCadIpcClient) -> anyhow::Result + Send + 'static, { let requested = board_path.to_path_buf(); - match crate::tools::pcb_components::with_ipc_classified(addr, move |c| { + match crate::tools::with_ipc_classified(addr, move |c| { c.ensure_board_is_active(&requested)?; f(c) }) @@ -268,10 +268,8 @@ pub(crate) async fn refuse_if_board_open_in_kicad( what: &str, ) -> anyhow::Result> { let requested = board_path.to_path_buf(); - match crate::tools::pcb_components::with_ipc_classified(addr, move |c| { - c.ensure_board_is_active(&requested) - }) - .await? + match crate::tools::with_ipc_classified(addr, move |c| c.ensure_board_is_active(&requested)) + .await? { Ok(()) => Ok(Some(CallToolResult::error(format!( "KiCAD currently holds this board open, and a {what} written to the file would \ @@ -1851,21 +1849,25 @@ mod layers_block_tests { } } +/// Shared scaffolding for this module's tests: a `ToolContext` pointed at a +/// given IPC address, and a mock KiCad that answers `GetOpenDocuments` with +/// one board and delegates every other command to the test. #[cfg(test)] -mod svg_logo_tests { - use super::*; +mod board_mock { use crate::router::ToolRouter; - use crate::tools::ServerConfig; + use crate::tools::{ServerConfig, ToolContext}; + use konnect_ipc::gen::kiapi; + use prost::Message; use std::sync::Arc; - fn test_ctx() -> ToolContext { + /// An empty `address` classifies as transport-unreachable, which is the + /// file-editing path — no live KiCad needed. + pub fn ctx_talking_to(address: String) -> ToolContext { ToolContext::new( - // Deliberately empty ipc_address: with_ipc fails fast against it, - // exercising the file-fallback path without needing live KiCAD. ServerConfig { kicad_cli: String::new(), kicad_binary: String::new(), - ipc_address: String::new(), + ipc_address: address, project_dir: None, jlcpcb_db_path: None, auto_load_toolsets: false, @@ -1875,6 +1877,79 @@ mod svg_logo_tests { ) } + /// A rep0 endpoint playing a KiCad that holds `board` open. `respond` + /// answers the commands the test cares about and returns `None` for the + /// rest; the socket, the envelope, and the `GetOpenDocuments` answer are + /// handled here so each test only writes its own command arms. + pub fn spawn_kicad_holding_board( + board: &std::path::Path, + respond: impl Fn(&prost_types::Any) -> Option + Send + 'static, + ) -> String { + use nng::options::Options; + + let port = { + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + listener.local_addr().unwrap().port() + }; + let url = format!("tcp://127.0.0.1:{port}"); + let socket = nng::Socket::new(nng::Protocol::Rep0).expect("mock rep socket"); + socket + .set_opt::(Some(std::time::Duration::from_secs(10))) + .unwrap(); + socket.listen(&url).expect("mock listen"); + + let board = board.to_string_lossy().to_string(); + std::thread::spawn(move || { + while let Ok(message) = socket.recv() { + let request = kiapi::common::ApiRequest::decode(message.as_slice()).unwrap(); + let command = request.message.expect("a command"); + let body = if command.type_url.ends_with("GetOpenDocuments") { + Some(konnect_ipc::builders::pack_any( + &kiapi::common::commands::GetOpenDocumentsResponse { + documents: vec![kiapi::common::types::DocumentSpecifier { + r#type: kiapi::common::types::DocumentType::DoctypePcb as i32, + project: None, + identifier: Some( + kiapi::common::types::document_specifier::Identifier::BoardFilename( + board.clone(), + ), + ), + }], + }, + "kiapi.common.commands.GetOpenDocumentsResponse", + )) + } else { + respond(&command) + }; + let response = kiapi::common::ApiResponse { + status: Some(kiapi::common::ApiResponseStatus { + status: kiapi::common::ApiStatusCode::AsOk as i32, + error_message: String::new(), + }), + header: None, + message: body, + }; + if socket + .send(nng::Message::from(response.encode_to_vec().as_slice())) + .is_err() + { + break; + } + } + }); + url + } +} + +#[cfg(test)] +mod svg_logo_tests { + use super::board_mock::ctx_talking_to; + use super::*; + + fn test_ctx() -> ToolContext { + ctx_talking_to(String::new()) + } + fn blank_board() -> &'static str { "(kicad_pcb\n (version 20250610)\n (generator \"konnect\")\n (paper \"A4\")\n (net 0 \"\")\n)\n" } @@ -1971,24 +2046,11 @@ mod svg_logo_tests { #[cfg(test)] mod net_count_tests { + use super::board_mock::ctx_talking_to; use super::*; - use crate::router::ToolRouter; - use crate::tools::ServerConfig; - use std::sync::Arc; fn test_ctx() -> ToolContext { - ToolContext::new( - ServerConfig { - kicad_cli: String::new(), - kicad_binary: String::new(), - ipc_address: String::new(), - project_dir: None, - jlcpcb_db_path: None, - auto_load_toolsets: false, - eager_toolsets: false, - }, - Arc::new(ToolRouter::new()), - ) + ctx_talking_to(String::new()) } async fn net_count_of(board: &str) -> i64 { @@ -2770,32 +2832,14 @@ mod board_size_tests { /// and net_count 0 for a board KiCad was showing fully populated. #[cfg(test)] mod board_info_source_tests { + use super::board_mock::{ctx_talking_to, spawn_kicad_holding_board}; use super::*; - use crate::router::ToolRouter; - use crate::tools::ServerConfig; use konnect_ipc::gen::kiapi; - use prost::Message; - use std::sync::Arc; /// A board saved before anything was placed on it: the empty stub the /// file-only reader kept reporting. const EMPTY_STUB: &str = "(kicad_pcb\n\t(version 20260206)\n\t(paper \"A3\")\n)\n"; - fn ctx_talking_to(address: String) -> ToolContext { - ToolContext::new( - ServerConfig { - kicad_cli: String::new(), - kicad_binary: String::new(), - ipc_address: address, - project_dir: None, - jlcpcb_db_path: None, - auto_load_toolsets: false, - eager_toolsets: false, - }, - Arc::new(ToolRouter::new()), - ) - } - /// A KiCad holding `board` open with `layers` enabled, `copper` of them /// copper, and `nets` real nets — none of it saved to the file. /// @@ -2808,91 +2852,45 @@ mod board_info_source_tests { copper: u32, nets: usize, ) -> String { - use nng::options::Options; - - let port = { - let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); - listener.local_addr().unwrap().port() - }; - let url = format!("tcp://127.0.0.1:{port}"); - let socket = nng::Socket::new(nng::Protocol::Rep0).expect("mock rep socket"); - socket - .set_opt::(Some(std::time::Duration::from_secs(10))) - .unwrap(); - socket.listen(&url).expect("mock listen"); - - let board = board.to_string_lossy().to_string(); - std::thread::spawn(move || { - while let Ok(message) = socket.recv() { - let request = kiapi::common::ApiRequest::decode(message.as_slice()).unwrap(); - let command = request.message.expect("a command"); - let body = if command.type_url.ends_with("GetOpenDocuments") { - Some(konnect_ipc::builders::pack_any( - &kiapi::common::commands::GetOpenDocumentsResponse { - documents: vec![kiapi::common::types::DocumentSpecifier { - r#type: kiapi::common::types::DocumentType::DoctypePcb as i32, - project: None, - identifier: Some( - kiapi::common::types::document_specifier::Identifier::BoardFilename( - board.clone(), - ), - ), - }], - }, - "kiapi.common.commands.GetOpenDocumentsResponse", - )) - } else if command.type_url.ends_with("GetTitleBlockInfo") { - Some(konnect_ipc::builders::pack_any( - &kiapi::common::types::TitleBlockInfo { - title: "Live title".to_string(), - revision: "B".to_string(), - ..Default::default() - }, - "kiapi.common.types.TitleBlockInfo", - )) - } else if command.type_url.ends_with("GetBoardEnabledLayers") { - Some(konnect_ipc::builders::pack_any( - &kiapi::board::commands::BoardEnabledLayersResponse { - copper_layer_count: copper, - layers: (0..layers as i32).collect(), - }, - "kiapi.board.commands.BoardEnabledLayersResponse", - )) - } else if command.type_url.ends_with("GetNets") { - Some(konnect_ipc::builders::pack_any( - &kiapi::board::commands::NetsResponse { - nets: std::iter::once(kiapi::board::types::Net { - code: Some(kiapi::board::types::NetCode { value: 0 }), - name: String::new(), - }) - .chain((1..=nets).map(|index| kiapi::board::types::Net { - code: Some(kiapi::board::types::NetCode { - value: index as i32, - }), - name: format!("N{index}"), - })) - .collect(), - }, - "kiapi.board.commands.NetsResponse", - )) - } else { - None - }; - let response = kiapi::common::ApiResponse { - status: Some(kiapi::common::ApiResponseStatus { - status: kiapi::common::ApiStatusCode::AsOk as i32, - error_message: String::new(), - }), - header: None, - message: body, - }; - let out = nng::Message::from(response.encode_to_vec().as_slice()); - if socket.send(out).is_err() { - break; - } + spawn_kicad_holding_board(board, move |command| { + if command.type_url.ends_with("GetTitleBlockInfo") { + Some(konnect_ipc::builders::pack_any( + &kiapi::common::types::TitleBlockInfo { + title: "Live title".to_string(), + revision: "B".to_string(), + ..Default::default() + }, + "kiapi.common.types.TitleBlockInfo", + )) + } else if command.type_url.ends_with("GetBoardEnabledLayers") { + Some(konnect_ipc::builders::pack_any( + &kiapi::board::commands::BoardEnabledLayersResponse { + copper_layer_count: copper, + layers: (0..layers as i32).collect(), + }, + "kiapi.board.commands.BoardEnabledLayersResponse", + )) + } else if command.type_url.ends_with("GetNets") { + Some(konnect_ipc::builders::pack_any( + &kiapi::board::commands::NetsResponse { + nets: std::iter::once(kiapi::board::types::Net { + code: Some(kiapi::board::types::NetCode { value: 0 }), + name: String::new(), + }) + .chain((1..=nets).map(|index| kiapi::board::types::Net { + code: Some(kiapi::board::types::NetCode { + value: index as i32, + }), + name: format!("N{index}"), + })) + .collect(), + }, + "kiapi.board.commands.NetsResponse", + )) + } else { + None } - }); - url + }) } async fn board_info(board: &std::path::Path, ctx: &ToolContext) -> serde_json::Value { diff --git a/crates/konnect-core/src/tools/pcb_components.rs b/crates/konnect-core/src/tools/pcb_components.rs index 9c1dd5ad..90332247 100644 --- a/crates/konnect-core/src/tools/pcb_components.rs +++ b/crates/konnect-core/src/tools/pcb_components.rs @@ -9,7 +9,8 @@ use crate::tool; use crate::tools::library::{footprint_lib_nickname_for_dir, is_lib_id, resolve_footprint_path}; use crate::tools::pcb_board::{attempt_ipc_write, BoardWrite}; use crate::tools::{ - get_path, require_array, require_f64, require_str, require_u64, ToolContext, ToolDef, + get_path, require_array, require_f64, require_str, require_u64, with_ipc_classified, + ToolContext, ToolDef, }; use anyhow::Context; use konnect_ipc::client::KiCadIpcClient; @@ -38,27 +39,6 @@ where } } -/// As [`with_ipc`], but classifying a failure as transport-unreachable vs -/// KiCad-rejected via [`konnect_ipc::IpcFailure`] — the typed gate for the -/// file-editing fallback (never a text match on the error message). -pub(crate) async fn with_ipc_classified( - addr: String, - f: F, -) -> anyhow::Result> -where - T: Send + 'static, - F: FnOnce(&KiCadIpcClient) -> anyhow::Result + Send + 'static, -{ - match tokio::task::spawn_blocking(move || { - f(&KiCadIpcClient::new(&addr)).map_err(konnect_ipc::IpcFailure::from_error) - }) - .await - { - Ok(result) => Ok(result), - Err(e) => Err(anyhow::anyhow!("Thread error: {}", e)), - } -} - macro_rules! ipc { ($ctx:expr, $args:expr, |$c:ident| $body:expr) => {{ let addr = $ctx.config.ipc_address.clone(); diff --git a/crates/konnect-core/src/tools/placement.rs b/crates/konnect-core/src/tools/placement.rs index eb1e5b53..dd5c4553 100644 --- a/crates/konnect-core/src/tools/placement.rs +++ b/crates/konnect-core/src/tools/placement.rs @@ -727,7 +727,7 @@ async fn handle_bga_fanout( .collect(); let addr = ctx.config.ipc_address.clone(); let requested_board = board.clone(); - let created = match super::pcb_components::with_ipc_classified(addr, move |c| { + let created = match super::with_ipc_classified(addr, move |c| { c.ensure_board_is_active(&requested_board)?; c.apply_fanout( &net_stubs, diff --git a/crates/konnect-ipc/src/client.rs b/crates/konnect-ipc/src/client.rs index 262b8df0..cb1deefd 100644 --- a/crates/konnect-ipc/src/client.rs +++ b/crates/konnect-ipc/src/client.rs @@ -519,10 +519,6 @@ impl KiCadIpcClient { self.find_open_board(requested).map(|_| ()) } - fn make_header(&self) -> Result { - Ok(header_for(self.get_board_document()?)) - } - /// Get all nets on the board. pub fn get_nets(&self) -> Result> { self.get_nets_in(self.get_board_document()?) @@ -563,15 +559,33 @@ impl KiCadIpcClient { &self, document: kiapi::common::types::DocumentSpecifier, item_type: kiapi::common::types::KiCadObjectType, + ) -> Result> { + self.get_items_of_types_in(document, &[item_type]) + } + + /// As [`Self::get_items_in`], asking for several types at once. + /// + /// `GetItems.types` is repeated, and every `send_command` dials a fresh NNG + /// socket, so one request for four types costs a quarter of what four + /// requests do against a KiCad that may be mid-refill. Items come back in + /// KiCad's own order, not grouped by type — callers dispatch on + /// `type_url`. + pub fn get_items_of_types_in( + &self, + document: kiapi::common::types::DocumentSpecifier, + item_types: &[kiapi::common::types::KiCadObjectType], ) -> Result> { let header = header_for(document); let cmd = kiapi::common::commands::GetItems { header: Some(header), - types: vec![item_type as i32], + types: item_types.iter().map(|t| *t as i32).collect(), }; let response_any = self.send_command(&cmd, "kiapi.common.commands.GetItems")?; if let Some(any) = response_any { let resp: kiapi::common::commands::GetItemsResponse = unpack_any(&any)?; + // Without this a failed request is indistinguishable from an empty + // board, and one failure now zeroes every type in the batch. + ensure_item_request_ok(resp.status, "item retrieval")?; Ok(resp.items) } else { Ok(vec![]) @@ -795,6 +809,20 @@ impl KiCadIpcClient { /// Delete items by KIID. pub fn delete_items(&self, ids: Vec) -> Result<()> { + if ids.is_empty() { + return Ok(()); + } + self.delete_items_in(self.get_board_document()?, ids) + } + + /// As [`Self::delete_items`], targeting a specific open document — so a + /// path-bearing request deletes from the board it names, not from whichever + /// board KiCad lists first. + pub fn delete_items_in( + &self, + document: kiapi::common::types::DocumentSpecifier, + ids: Vec, + ) -> Result<()> { if ids.is_empty() { return Ok(()); } @@ -806,7 +834,7 @@ impl KiCadIpcClient { if expected_ids.len() != expected_count { anyhow::bail!("delete request contains duplicate item identifiers"); } - let header = self.make_header()?; + let header = header_for(document); let cmd = kiapi::common::commands::DeleteItems { header: Some(header), item_ids: ids diff --git a/crates/konnect-ipc/tests/mock_server_test.rs b/crates/konnect-ipc/tests/mock_server_test.rs index 4cee460c..cf147a23 100644 --- a/crates/konnect-ipc/tests/mock_server_test.rs +++ b/crates/konnect-ipc/tests/mock_server_test.rs @@ -1025,7 +1025,7 @@ fn footprint_with_pads(reference: &str, pads: Vec) -> prost_ty ) } -fn spawn_kicad_with_footprints(items: Vec) -> MockKicad { +fn spawn_kicad_holding_items(items: Vec) -> MockKicad { spawn_mock(move |request| { let message = request.message.expect("request must pack a command"); if message.type_url.ends_with("GetOpenDocuments") { @@ -1046,9 +1046,44 @@ fn spawn_kicad_with_footprints(items: Vec) -> MockKicad { }) } +#[test] +fn a_failed_item_read_is_an_error_not_an_empty_board() { + let mock = spawn_mock(move |request| { + let message = request.message.expect("request must pack a command"); + if message.type_url.ends_with("GetOpenDocuments") { + return Some(open_board_response()); + } + if message.type_url.ends_with("GetItems") { + let response = kiapi::common::commands::GetItemsResponse { + header: None, + status: kiapi::common::types::ItemRequestStatus::IrsDocumentNotFound as i32, + items: vec![], + }; + return Some(reply_with(builders::pack_any( + &response, + "kiapi.common.commands.GetItemsResponse", + ))); + } + Some(ok_response()) + }); + + let client = KiCadIpcClient::new(&mock.url); + let document = client + .find_open_board(std::path::Path::new("test.kicad_pcb")) + .expect("the mock holds test.kicad_pcb"); + + let error = client + .get_footprint_pads_in(document, "U1") + .expect_err("a failed request must not read as a board with no footprints"); + assert!( + error.to_string().contains("IRS_DOCUMENT_NOT_FOUND"), + "the failure must name KiCad's status, got: {error}" + ); +} + #[test] fn footprint_pads_come_back_in_board_coordinates_with_their_nets() { - let mock = spawn_kicad_with_footprints(vec![footprint_with_pads( + let mock = spawn_kicad_holding_items(vec![footprint_with_pads( "U1", vec![ pad_at("A4", 101.155, 66.11, "/VBUS"), @@ -1077,7 +1112,7 @@ fn footprint_pads_come_back_in_board_coordinates_with_their_nets() { #[test] fn an_unreadable_live_pad_is_reported_instead_of_silently_dropped() { - let mock = spawn_kicad_with_footprints(vec![footprint_with_pads( + let mock = spawn_kicad_holding_items(vec![footprint_with_pads( "U1", vec![prost_types::Any { type_url: "type.googleapis.com/kiapi.board.types.Pad".to_string(), @@ -1104,7 +1139,7 @@ fn a_live_pad_without_a_position_is_reported_instead_of_fabricated_at_zero() { }, "kiapi.board.types.Pad", ); - let mock = spawn_kicad_with_footprints(vec![footprint_with_pads("U1", vec![pad])]); + let mock = spawn_kicad_holding_items(vec![footprint_with_pads("U1", vec![pad])]); let client = KiCadIpcClient::new(&mock.url); let document = client .find_open_board(std::path::Path::new("test.kicad_pcb")) @@ -1118,7 +1153,7 @@ fn a_live_pad_without_a_position_is_reported_instead_of_fabricated_at_zero() { #[test] fn a_footprint_absent_from_the_live_board_reads_as_none() { - let mock = spawn_kicad_with_footprints(vec![footprint_with_pads( + let mock = spawn_kicad_holding_items(vec![footprint_with_pads( "U1", vec![pad_at("1", 1.0, 2.0, "GND")], )]);