From 87b02d4993915a60cd67115edfd36a099dbbcdf4 Mon Sep 17 00:00:00 2001 From: nordic-style <11313330+nordic-style@users.noreply.github.com> Date: Tue, 18 Aug 2026 03:44:56 +0200 Subject: [PATCH 1/2] fix(verification): bound the KiCad UI health check Validate timeout_seconds and apply one Tokio deadline to process detection plus IPC ping, reporting timeout state and elapsed time explicitly. Refs #251 --- crates/konnect-core/src/tools/verification.rs | 135 ++++++++++++++---- tool-directory.md | 2 +- 2 files changed, 107 insertions(+), 30 deletions(-) diff --git a/crates/konnect-core/src/tools/verification.rs b/crates/konnect-core/src/tools/verification.rs index db92d493..86a20038 100644 --- a/crates/konnect-core/src/tools/verification.rs +++ b/crates/konnect-core/src/tools/verification.rs @@ -80,13 +80,15 @@ pub fn tools() -> Vec { ), tool!( "check_kicad_ui", - "Check whether the KiCAD GUI application is running and responsive.", + "Check whether the KiCad GUI application is running and whether IPC responds within a bounded timeout.", json!({ "type": "object", "properties": { "timeout_seconds": { "type": "integer", "description": "Timeout for the health check in seconds", + "minimum": 1, + "maximum": 300, "default": 5 } }, @@ -494,39 +496,85 @@ fn find_kicad_binary(config_binary: &str) -> String { } } -async fn handle_check_kicad_ui( - _args: &serde_json::Value, - ctx: &ToolContext, -) -> anyhow::Result { - let running = task::spawn_blocking(is_kicad_running).await?; - - if !running { - return Ok(CallToolResult::text( - serde_json::to_string(&json!({ - "running": false, - "ipc_responsive": false - })) - .unwrap(), +fn health_timeout_seconds(args: &serde_json::Value) -> Result { + let timeout = match args.get("timeout_seconds") { + None | Some(serde_json::Value::Null) => 5, + Some(value) => value.as_u64().ok_or_else(|| { + CallToolResult::error_kind( + crate::mcp::error::ToolErrorKind::InvalidArgument { + field: "timeout_seconds".to_string(), + reason: "must be an integer from 1 to 300".to_string(), + }, + "Argument 'timeout_seconds' must be an integer from 1 to 300", + ) + })?, + }; + if !(1..=300).contains(&timeout) { + return Err(CallToolResult::error_kind( + crate::mcp::error::ToolErrorKind::InvalidArgument { + field: "timeout_seconds".to_string(), + reason: "must be between 1 and 300 seconds".to_string(), + }, + "Argument 'timeout_seconds' must be between 1 and 300 seconds", )); } + Ok(timeout) +} + +async fn bounded_health_check( + timeout: std::time::Duration, + future: F, +) -> Result +where + F: std::future::Future, +{ + tokio::time::timeout(timeout, future).await +} - // Try IPC ping +async fn handle_check_kicad_ui( + args: &serde_json::Value, + ctx: &ToolContext, +) -> anyhow::Result { + let timeout_seconds = match health_timeout_seconds(args) { + Ok(timeout) => timeout, + Err(error) => return Ok(error), + }; let addr = ctx.config.ipc_address.clone(); - let ipc_ok = task::spawn_blocking(move || { - konnect_ipc::client::KiCadIpcClient::new(&addr) - .ping() - .unwrap_or(false) - }) - .await - .unwrap_or(false); + let started = std::time::Instant::now(); + let check = async move { + let running = task::spawn_blocking(is_kicad_running).await?; + if !running { + return Ok::<_, tokio::task::JoinError>((false, false)); + } + let ipc_responsive = task::spawn_blocking(move || { + konnect_ipc::client::KiCadIpcClient::new(&addr) + .ping() + .unwrap_or(false) + }) + .await?; + Ok((true, ipc_responsive)) + }; - Ok(CallToolResult::text( - serde_json::to_string(&json!({ - "running": true, - "ipc_responsive": ipc_ok - })) - .unwrap(), - )) + match bounded_health_check(std::time::Duration::from_secs(timeout_seconds), check).await { + Ok(result) => { + let (running, ipc_responsive) = result?; + Ok(CallToolResult::json(&json!({ + "running": running, + "ipc_responsive": ipc_responsive, + "timed_out": false, + "timeout_seconds": timeout_seconds, + "elapsed_ms": started.elapsed().as_millis() as u64 + }))) + } + Err(_) => Ok(CallToolResult::json(&json!({ + "running": null, + "ipc_responsive": false, + "timed_out": true, + "timeout_seconds": timeout_seconds, + "elapsed_ms": started.elapsed().as_millis() as u64, + "note": "KiCad health check exceeded the requested timeout" + }))), + } } async fn handle_launch_kicad_ui( @@ -952,6 +1000,35 @@ mod tests { ) } + #[test] + fn health_timeout_is_bounded_and_typed() { + assert_eq!(health_timeout_seconds(&json!({})).unwrap(), 5); + assert_eq!( + health_timeout_seconds(&json!({ "timeout_seconds": 17 })).unwrap(), + 17 + ); + for invalid in [json!(0), json!(301), json!(1.5), json!("5")] { + let error = health_timeout_seconds(&json!({ "timeout_seconds": invalid })) + .expect_err("out-of-range or non-integer timeout must be refused"); + assert!(error.is_error); + } + } + + #[tokio::test] + async fn health_deadline_returns_without_waiting_for_the_inner_future() { + let timed_out = bounded_health_check(std::time::Duration::from_millis(1), async { + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + 1 + }) + .await; + assert!(timed_out.is_err()); + + let completed = bounded_health_check(std::time::Duration::from_secs(1), async { 2 }) + .await + .unwrap(); + assert_eq!(completed, 2); + } + fn blank_board() -> &'static str { "(kicad_pcb\n (version 20250610)\n (generator \"test\")\n (general (thickness 1.6))\n (paper \"A4\")\n (layers\n (0 \"F.Cu\" signal)\n (31 \"B.Cu\" signal)\n (44 \"Edge.Cuts\" user)\n )\n (setup (pad_to_mask_clearance 0))\n (net 0 \"\")\n)\n" } diff --git a/tool-directory.md b/tool-directory.md index 6dafb9af..6eb45d55 100644 --- a/tool-directory.md +++ b/tool-directory.md @@ -340,7 +340,7 @@ Six tools, grouped into *discovery/routing* and *observability*. | `run_drc` | Run the Design Rule Check on the PCB and return structured violation results. | | `set_design_rules` | Set board-level design rules (clearance, trace width, via size) in the sibling `.kicad_pro` project file. The board file is not modified. | | `get_design_rules` | Return the current design rule constraints from the sibling `.kicad_pro` project file. | -| `check_kicad_ui` | Check whether the KiCAD GUI application is running and responsive. | +| `check_kicad_ui` | Check whether the KiCad GUI is running and whether IPC responds within the requested bounded timeout. | | `launch_kicad_ui` | Launch the KiCAD GUI application and optionally open a project file. | | `copy_routing_pattern` | Copy a routing pattern (traces and vias) from one region of the board to another. | | `set_layer_constraints` | Set per-layer design constraints (min trace width, clearance) as named rules in the sibling `.kicad_dru` custom-rules file. | From a06d21d9f6bac831e8c77307729f048b8cc04d3d Mon Sep 17 00:00:00 2001 From: nordic-style <11313330+nordic-style@users.noreply.github.com> Date: Tue, 18 Aug 2026 04:21:14 +0200 Subject: [PATCH 2/2] fix(verification): recognize standalone KiCad editors Treat pcbnew and eeschema as GUI processes on every platform, and let a responsive IPC endpoint prove that KiCad is running even when process inspection misses its launcher. Report process detection separately for diagnostics.\n\nRefs #251 --- crates/konnect-core/src/tools/verification.rs | 68 +++++++++++++++---- 1 file changed, 54 insertions(+), 14 deletions(-) diff --git a/crates/konnect-core/src/tools/verification.rs b/crates/konnect-core/src/tools/verification.rs index 86a20038..8048929f 100644 --- a/crates/konnect-core/src/tools/verification.rs +++ b/crates/konnect-core/src/tools/verification.rs @@ -433,29 +433,51 @@ async fn handle_get_design_rules( // ─── KiCAD UI management ────────────────────────────────────────────────────── -/// Check if the KiCAD GUI is running by scanning the process list. +const KICAD_GUI_PROCESS_NAMES: &[&str] = &["kicad", "pcbnew", "eeschema"]; + +fn is_kicad_process_name(name: &str) -> bool { + let file_name = std::path::Path::new(name.trim_matches('"')) + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or(name) + .to_ascii_lowercase(); + let stem = file_name.strip_suffix(".exe").unwrap_or(&file_name); + KICAD_GUI_PROCESS_NAMES.contains(&stem) +} + +fn process_list_has_kicad(output: &str) -> bool { + output.lines().any(|line| { + line.split_whitespace() + .next() + .is_some_and(is_kicad_process_name) + }) +} + +/// Check if the KiCad project manager or either standalone editor is running. fn is_kicad_running() -> bool { #[cfg(target_os = "windows")] { - // On Windows, use `tasklist` to check std::process::Command::new("tasklist") .output() .ok() - .map(|o| String::from_utf8_lossy(&o.stdout).contains("kicad.exe")) + .map(|output| process_list_has_kicad(&String::from_utf8_lossy(&output.stdout))) .unwrap_or(false) } #[cfg(not(target_os = "windows"))] { - std::process::Command::new("pgrep") - .arg("-x") - .arg("kicad") + std::process::Command::new("ps") + .args(["-A", "-o", "comm="]) .output() .ok() - .map(|o| o.status.success()) + .map(|output| process_list_has_kicad(&String::from_utf8_lossy(&output.stdout))) .unwrap_or(false) } } +fn ui_running(process_detected: bool, ipc_responsive: bool) -> bool { + process_detected || ipc_responsive +} + /// Resolve the KiCAD binary path from config or well-known locations. fn find_kicad_binary(config_binary: &str) -> String { if !config_binary.is_empty() && std::path::Path::new(config_binary).exists() { @@ -542,24 +564,22 @@ async fn handle_check_kicad_ui( let addr = ctx.config.ipc_address.clone(); let started = std::time::Instant::now(); let check = async move { - let running = task::spawn_blocking(is_kicad_running).await?; - if !running { - return Ok::<_, tokio::task::JoinError>((false, false)); - } + let process_detected = task::spawn_blocking(is_kicad_running).await?; let ipc_responsive = task::spawn_blocking(move || { konnect_ipc::client::KiCadIpcClient::new(&addr) .ping() .unwrap_or(false) }) .await?; - Ok((true, ipc_responsive)) + Ok::<_, tokio::task::JoinError>((process_detected, ipc_responsive)) }; match bounded_health_check(std::time::Duration::from_secs(timeout_seconds), check).await { Ok(result) => { - let (running, ipc_responsive) = result?; + let (process_detected, ipc_responsive) = result?; Ok(CallToolResult::json(&json!({ - "running": running, + "running": ui_running(process_detected, ipc_responsive), + "process_detected": process_detected, "ipc_responsive": ipc_responsive, "timed_out": false, "timeout_seconds": timeout_seconds, @@ -568,6 +588,7 @@ async fn handle_check_kicad_ui( } Err(_) => Ok(CallToolResult::json(&json!({ "running": null, + "process_detected": null, "ipc_responsive": false, "timed_out": true, "timeout_seconds": timeout_seconds, @@ -1014,6 +1035,25 @@ mod tests { } } + #[test] + fn standalone_editors_count_as_kicad_ui_processes() { + for name in ["kicad", "pcbnew", "eeschema", "PCBNEW.EXE"] { + assert!(is_kicad_process_name(name), "did not recognize {name}"); + } + assert!(!is_kicad_process_name("kicad-cli")); + assert!(!is_kicad_process_name("freerouting")); + assert!(process_list_has_kicad( + "/usr/bin/Finder\n/Applications/KiCad/pcbnew\n" + )); + } + + #[test] + fn responsive_ipc_is_sufficient_running_evidence() { + assert!(ui_running(false, true)); + assert!(ui_running(true, false)); + assert!(!ui_running(false, false)); + } + #[tokio::test] async fn health_deadline_returns_without_waiting_for_the_inner_future() { let timed_out = bounded_health_check(std::time::Duration::from_millis(1), async {