From 412670533e6ffedf64b577ac4ac4d2615323faf2 Mon Sep 17 00:00:00 2001 From: nordic-style <11313330+nordic-style@users.noreply.github.com> Date: Tue, 18 Aug 2026 01:33:58 +0200 Subject: [PATCH 1/2] fix(manufacturing): apply Gerber and position options Refs #251 --- crates/konnect-core/src/tools/cli.rs | 149 +++++++++-- .../konnect-core/src/tools/manufacturing.rs | 96 ++++++- crates/konnect-core/src/tools/pcb_export.rs | 240 +++++++++++++++++- 3 files changed, 445 insertions(+), 40 deletions(-) diff --git a/crates/konnect-core/src/tools/cli.rs b/crates/konnect-core/src/tools/cli.rs index e623b251..b7ae5e9d 100644 --- a/crates/konnect-core/src/tools/cli.rs +++ b/crates/konnect-core/src/tools/cli.rs @@ -486,16 +486,32 @@ pub async fn export_netlist( // ─── PCB Export ────────────────────────────────────────────────────────────── -/// KiCAD 10: `pcb export gerbers --output ` (PLURAL!) -pub async fn export_gerber(cli: &str, pcb: &Path, output_dir: &Path) -> Result<()> { - let args = [ - "pcb", - "export", - "gerbers", - "--output", - output_dir.to_str().unwrap(), - pcb.to_str().unwrap(), - ]; +/// Argument vector for Gerber export. KiCad's plural `gerbers` subcommand +/// accepts the complete selection as one comma-separated `--layers` value. +fn gerber_args<'a>(output_dir: &'a str, pcb: &'a str, layers_csv: &'a str) -> Vec<&'a str> { + let mut args = vec!["pcb", "export", "gerbers", "--output", output_dir]; + if !layers_csv.is_empty() { + args.push("--layers"); + args.push(layers_csv); + } + args.push(pcb); + args +} + +/// KiCad 10: `pcb export gerbers --output [--layers ] ` +/// (PLURAL!) +pub async fn export_gerber( + cli: &str, + pcb: &Path, + output_dir: &Path, + layers: &[&str], +) -> Result<()> { + let layers_csv = layers.join(","); + let args = gerber_args( + output_dir.to_str().unwrap_or(""), + pcb.to_str().unwrap_or(""), + &layers_csv, + ); run_cli(cli, &args, LONG_TIMEOUT).await?; Ok(()) } @@ -617,24 +633,49 @@ pub async fn export_3d(cli: &str, pcb: &Path, output: &Path, format: &str) -> Re Ok(()) } -/// KiCAD 10: `pcb export pos --output --format ` -/// Formats: ascii (default), csv, gerber +/// Argument vector for position export, factored out so the public options can +/// be regression-tested without a kicad-cli installation. +fn position_args<'a>( + output: &'a str, + pcb: &'a str, + format: &'a str, + units: &'a str, + side: &'a str, +) -> Vec<&'a str> { + let mut args = vec![ + "pcb", "export", "pos", "--output", output, "--format", format, "--side", side, + ]; + // Gerber coordinates have format-defined units; KiCad only accepts this + // option for its ASCII and CSV position formats. + if format != "gerber" { + args.push("--units"); + args.push(units); + } + args.push(pcb); + args +} + +/// KiCad 10: `pcb export pos --output --format --side +/// [--units ] ` +/// +/// KiCad itself omits footprints carrying `exclude_from_pos_files`; Konnect +/// deliberately leaves that source-of-truth filtering to the exporter rather +/// than trying to post-process CSV and Gerber output differently. pub async fn export_position_file( cli: &str, pcb: &Path, output: &Path, format: &str, + units: &str, + side: &str, ) -> Result<()> { - let args = [ - "pcb", - "export", - "pos", - "--output", - output.to_str().unwrap(), - "--format", + let args = position_args( + output.to_str().unwrap_or(""), + pcb.to_str().unwrap_or(""), format, - pcb.to_str().unwrap(), - ]; + units, + side, + ); run_cli(cli, &args, LONG_TIMEOUT).await?; Ok(()) } @@ -940,6 +981,72 @@ mod erc_parse_tests { } } +#[cfg(test)] +mod gerber_export_tests { + use super::*; + + #[test] + fn requested_layers_reach_kicad_as_one_csv_argument() { + let args = gerber_args( + "/out/gerbers", + "/tmp/board.kicad_pcb", + "F.Cu,In1.Cu,B.Cu,F.Mask,B.Mask,Edge.Cuts", + ); + let layers = args + .iter() + .position(|argument| *argument == "--layers") + .map(|index| args[index + 1]); + assert_eq!(layers, Some("F.Cu,In1.Cu,B.Cu,F.Mask,B.Mask,Edge.Cuts")); + assert_eq!(args.last().copied(), Some("/tmp/board.kicad_pcb")); + } + + #[test] + fn empty_layer_selection_keeps_the_flag_absent() { + let args = gerber_args("/out", "/tmp/board.kicad_pcb", ""); + assert!(!args.contains(&"--layers")); + } +} + +#[cfg(test)] +mod position_export_tests { + use super::*; + + fn flag<'a>(args: &'a [&str], name: &str) -> Option<&'a str> { + args.iter() + .position(|argument| *argument == name) + .map(|index| args[index + 1]) + } + + #[test] + fn csv_units_and_side_reach_kicad_cli() { + let args = position_args( + "/out/positions.csv", + "/tmp/board.kicad_pcb", + "csv", + "mm", + "back", + ); + assert_eq!(flag(&args, "--format"), Some("csv")); + assert_eq!(flag(&args, "--units"), Some("mm")); + assert_eq!(flag(&args, "--side"), Some("back")); + assert_eq!(args.last().copied(), Some("/tmp/board.kicad_pcb")); + } + + #[test] + fn gerber_position_export_does_not_claim_a_units_flag() { + let args = position_args( + "/out/positions.gbr", + "/tmp/board.kicad_pcb", + "gerber", + "mm", + "front", + ); + assert_eq!(flag(&args, "--format"), Some("gerber")); + assert_eq!(flag(&args, "--side"), Some("front")); + assert_eq!(flag(&args, "--units"), None); + } +} + #[cfg(test)] mod drill_export_tests { use super::*; diff --git a/crates/konnect-core/src/tools/manufacturing.rs b/crates/konnect-core/src/tools/manufacturing.rs index b16cc8bd..cf0484bd 100644 --- a/crates/konnect-core/src/tools/manufacturing.rs +++ b/crates/konnect-core/src/tools/manufacturing.rs @@ -10,7 +10,7 @@ use serde_json::json; use std::path::PathBuf; use tracing::{debug, error, info, warn}; -use super::cli; +use super::{cli, pcb_export}; // ─── Tool definitions ───────────────────────────────────────────────────────── @@ -53,6 +53,23 @@ pub fn tools() -> Vec { "bom_group_by": { "type": "string", "description": "Comma-separated fields whose matching references collapse into one BOM row, e.g. 'Value,Footprint'." + }, + "gerber_layers": { + "type": "array", + "items": { "type": "string" }, + "description": "Exact Gerber layers. Omit or pass [] to auto-select enabled copper, F/B.Mask, F/B.SilkS, and Edge.Cuts while excluding documentation layers." + }, + "position_side": { + "type": "string", + "enum": ["front", "back", "both"], + "description": "Board side(s) in the assembly position file.", + "default": "both" + }, + "position_units": { + "type": "string", + "enum": ["mm", "in"], + "description": "Coordinate units in the assembly position file.", + "default": "mm" } }, "required": ["board", "output_dir"] @@ -123,6 +140,28 @@ async fn handle_export_manufacturing_package( let fab_house = args["fab_house"].as_str().unwrap_or("jlcpcb"); let include_assembly = args["include_assembly"].as_bool().unwrap_or(true); let schematic = args["schematic"].as_str().map(PathBuf::from); + let requested_gerber_layers = match pcb_export::optional_string_array(args, "gerber_layers") { + Ok(layers) => layers, + Err(error) => return Ok(error), + }; + let gerber_layers = if requested_gerber_layers.is_empty() { + let board_source = tokio::fs::read_to_string(&board).await?; + pcb_export::standard_gerber_layers(&board_source)? + } else { + requested_gerber_layers + }; + let position_side = args["position_side"].as_str().unwrap_or("both"); + let position_units = args["position_units"].as_str().unwrap_or("mm"); + if let Err((field, reason)) = + pcb_export::validate_position_values("csv", position_side, position_units) + { + let public_field = match field { + "side" => "position_side", + "units" => "position_units", + other => other, + }; + return Ok(invalid_manufacturing_argument(public_field, reason)); + } info!( board = %board.display(), @@ -141,12 +180,14 @@ async fn handle_export_manufacturing_package( // 1. Export Gerbers let gerber_dir = output_dir.join("gerbers"); tokio::fs::create_dir_all(&gerber_dir).await?; - match cli::export_gerber(cli_path, &board, &gerber_dir).await { + let gerber_layer_refs = gerber_layers.iter().map(String::as_str).collect::>(); + match cli::export_gerber(cli_path, &board, &gerber_dir, &gerber_layer_refs).await { Ok(()) => { info!("[BETA] Gerber export succeeded"); files_generated.push(json!({ "type": "gerber", - "path": gerber_dir.to_str().unwrap_or("") + "path": gerber_dir.to_str().unwrap_or(""), + "layers": gerber_layers.clone() })); } Err(e) => { @@ -190,13 +231,24 @@ async fn handle_export_manufacturing_package( _ => "csv", }; let pos_path = output_dir.join(format!("positions.{}", pos_format)); - match cli::export_position_file(cli_path, &board, &pos_path, pos_format).await { + match cli::export_position_file( + cli_path, + &board, + &pos_path, + pos_format, + position_units, + position_side, + ) + .await + { Ok(()) => { info!("[BETA] Position file export succeeded"); files_generated.push(json!({ "type": "pick_and_place", "path": pos_path.to_str().unwrap_or(""), - "format": pos_format + "format": pos_format, + "units": position_units, + "side": position_side })); } Err(e) => { @@ -275,6 +327,9 @@ async fn handle_export_manufacturing_package( "output_dir": output_dir.to_str().unwrap_or(""), "files": all_files, "files_generated": files_generated, + "gerber_layers": gerber_layers, + "position_units": if include_assembly { Some(position_units) } else { None }, + "position_side": if include_assembly { Some(position_side) } else { None }, "warnings": warnings, "summary": summary, "next_steps": format!( @@ -287,6 +342,17 @@ async fn handle_export_manufacturing_package( )) } +fn invalid_manufacturing_argument(field: &str, reason: impl Into) -> CallToolResult { + let reason = reason.into(); + CallToolResult::error_kind( + crate::mcp::error::ToolErrorKind::InvalidArgument { + field: field.to_string(), + reason: reason.clone(), + }, + format!("Argument '{field}' is invalid: {reason}"), + ) +} + async fn handle_validate_for_manufacturing( args: &serde_json::Value, ctx: &ToolContext, @@ -544,6 +610,26 @@ async fn handle_estimate_cost( )) } +#[cfg(test)] +mod package_export_option_tests { + use super::*; + + #[test] + fn package_schema_exposes_applied_gerber_and_position_options() { + let package = tools() + .into_iter() + .find(|tool| tool.name == "export_manufacturing_package") + .unwrap(); + let properties = &package.input_schema["properties"]; + assert_eq!(properties["gerber_layers"]["items"]["type"], "string"); + assert_eq!(properties["position_units"]["enum"], json!(["mm", "in"])); + assert_eq!( + properties["position_side"]["enum"], + json!(["front", "back", "both"]) + ); + } +} + // ─── Helpers ───────────────────────────────────────────────────────────────── /// Distinct nets and routed items on the board, read from the parsed tree. diff --git a/crates/konnect-core/src/tools/pcb_export.rs b/crates/konnect-core/src/tools/pcb_export.rs index e936fadb..dc11d686 100644 --- a/crates/konnect-core/src/tools/pcb_export.rs +++ b/crates/konnect-core/src/tools/pcb_export.rs @@ -39,13 +39,98 @@ fn severity_rank(s: &str) -> u8 { } } +fn invalid_export_argument(field: &str, reason: impl Into) -> CallToolResult { + let reason = reason.into(); + CallToolResult::error_kind( + crate::mcp::error::ToolErrorKind::InvalidArgument { + field: field.to_string(), + reason: reason.clone(), + }, + format!("Argument '{field}' is invalid: {reason}"), + ) +} + +pub(crate) fn optional_string_array( + args: &serde_json::Value, + field: &str, +) -> Result, CallToolResult> { + let Some(value) = args.get(field) else { + return Ok(Vec::new()); + }; + let Some(values) = value.as_array() else { + return Err(invalid_export_argument(field, "must be an array")); + }; + values + .iter() + .enumerate() + .map(|(index, value)| { + value.as_str().map(String::from).ok_or_else(|| { + invalid_export_argument(&format!("{field}[{index}]"), "must be a layer name string") + }) + }) + .collect() +} + +/// Default Gerber selection for a fabrication package: every enabled copper +/// layer plus solder mask, silkscreen, and the board outline. It deliberately +/// excludes drawings, comments, adhesive, courtyard, fab, and margin layers. +pub(crate) fn standard_gerber_layers(board_source: &str) -> anyhow::Result> { + let board = konnect_sexp::parser::parse_sexp(board_source)?; + let layers = konnect_sexp::layers::layers(&board) + .into_iter() + .filter(|layer| { + layer.is_copper() + || matches!( + layer.name.as_str(), + "F.Mask" | "B.Mask" | "F.SilkS" | "B.SilkS" | "Edge.Cuts" + ) + }) + .map(|layer| layer.name) + .collect::>(); + if layers.is_empty() { + anyhow::bail!("board declares no standard fabrication layers"); + } + Ok(layers) +} + +pub(crate) fn validate_position_values( + format: &str, + side: &str, + units: &str, +) -> Result<(), (&'static str, String)> { + if !matches!(format, "csv" | "gerber") { + return Err(( + "format", + format!("must be 'csv' or 'gerber', got '{format}'"), + )); + } + if !matches!(side, "front" | "back" | "both") { + return Err(( + "side", + format!("must be 'front', 'back', or 'both', got '{side}'"), + )); + } + if !matches!(units, "mm" | "in") { + return Err(("units", format!("must be 'mm' or 'in', got '{units}'"))); + } + if format == "gerber" && side == "both" { + return Err(( + "side", + "Gerber position output supports only 'front' or 'back'".to_string(), + )); + } + Ok(()) +} + // ─── Tool definitions ───────────────────────────────────────────────────────── pub fn tools() -> Vec { vec![ tool!( "export_gerber", - "Export Gerber production files for all copper and mask layers using kicad-cli.", + "Export Gerber production files using kicad-cli. By default Konnect selects all \ + enabled copper layers, masks, silkscreens, and Edge.Cuts while excluding \ + documentation-only layers.", json!({ "type": "object", "properties": { @@ -53,7 +138,7 @@ pub fn tools() -> Vec { "output_dir": { "type": "string", "description": "Directory to write Gerber files into" }, "layers": { "type": "array", - "description": "Layer names to export (empty = all fabrication layers)", + "description": "Exact layer names to export. Omit or pass an empty array to auto-select enabled copper, F/B.Mask, F/B.SilkS, and Edge.Cuts.", "items": { "type": "string" } }, "drill_file": { "type": "boolean", "description": "Also generate Excellon drill file", "default": true } @@ -178,7 +263,8 @@ pub fn tools() -> Vec { ), tool!( "export_position_file", - "Generate a component placement (pick-and-place) position file for SMT assembly.", + "Generate a component placement (pick-and-place) position file for SMT assembly. \ + KiCad automatically omits footprints marked exclude_from_pos_files.", json!({ "type": "object", "properties": { @@ -187,16 +273,19 @@ pub fn tools() -> Vec { "format": { "type": "string", "description": "File format: 'csv' (default) or 'gerber'", + "enum": ["csv", "gerber"], "default": "csv" }, "side": { "type": "string", "description": "Board side: 'front', 'back', or 'both'", + "enum": ["front", "back", "both"], "default": "both" }, "units": { "type": "string", - "description": "Coordinate units: 'mm' (default) or 'in'", + "description": "Coordinate units for CSV: 'mm' (default) or 'in'. Gerber position output has format-defined units.", + "enum": ["mm", "in"], "default": "mm" } }, @@ -322,11 +411,23 @@ async fn handle_export_gerber( let output_dir = get_path(args, "output_dir")?; let drill = args["drill_file"].as_bool().unwrap_or(true); + let requested_layers = match optional_string_array(args, "layers") { + Ok(layers) => layers, + Err(error) => return Ok(error), + }; + let layers = if requested_layers.is_empty() { + let board_source = tokio::fs::read_to_string(&board).await?; + standard_gerber_layers(&board_source)? + } else { + requested_layers + }; + let layer_refs = layers.iter().map(String::as_str).collect::>(); + // Ensure output dir exists tokio::fs::create_dir_all(&output_dir).await?; let cli = &ctx.config.kicad_cli; - cli::export_gerber(cli, &board, &output_dir).await?; + cli::export_gerber(cli, &board, &output_dir, &layer_refs).await?; if drill { // kicad-cli also has a dedicated drill export. Its --output is a @@ -351,6 +452,7 @@ async fn handle_export_gerber( serde_json::to_string(&json!({ "success": true, "output_dir": output_dir.to_str().unwrap_or(""), + "layers": layers, "files": files })) .unwrap(), @@ -499,18 +601,24 @@ async fn handle_export_position_file( let side = args["side"].as_str().unwrap_or("both"); let units = args["units"].as_str().unwrap_or("mm"); + if let Err((field, reason)) = validate_position_values(format, side, units) { + return Ok(invalid_export_argument(field, reason)); + } + let cli = &ctx.config.kicad_cli; - cli::export_position_file(cli, &board, &output, format).await?; + cli::export_position_file(cli, &board, &output, format, units, side).await?; + let mut result = json!({ + "success": true, + "format": format, + "side": side, + "output": output.to_str().unwrap_or("") + }); + if format != "gerber" { + result["units"] = json!(units); + } Ok(CallToolResult::text( - serde_json::to_string(&json!({ - "success": true, - "format": format, - "side": side, - "units": units, - "output": output.to_str().unwrap_or("") - })) - .unwrap(), + serde_json::to_string(&result).unwrap(), )) } @@ -914,3 +1022,107 @@ mod required_layers_tests { } } } + +#[cfg(test)] +mod fabrication_option_tests { + use super::*; + use crate::router::ToolRouter; + use crate::tools::ServerConfig; + use serde_json::json; + use std::sync::Arc; + + fn 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()), + ) + } + + #[test] + fn default_gerbers_include_only_manufacturing_layers() { + // KiCad 9 output from the IPC test fixture, including the real layer + // table order and every documentation-only layer this filter excludes. + let board = include_str!("../../../konnect-ipc/tests/fixtures/live_ipc.kicad_pcb"); + assert_eq!( + standard_gerber_layers(board).unwrap(), + [ + "F.Cu", + "B.Cu", + "F.SilkS", + "B.SilkS", + "F.Mask", + "B.Mask", + "Edge.Cuts" + ] + ); + } + + #[test] + fn public_schemas_describe_the_options_that_reach_kicad() { + let gerber = tools() + .into_iter() + .find(|tool| tool.name == "export_gerber") + .unwrap(); + assert!(gerber.input_schema["properties"]["layers"]["description"] + .as_str() + .unwrap() + .contains("auto-select")); + + let position = tools() + .into_iter() + .find(|tool| tool.name == "export_position_file") + .unwrap(); + assert_eq!( + position.input_schema["properties"]["units"]["enum"], + json!(["mm", "in"]) + ); + assert_eq!( + position.input_schema["properties"]["side"]["enum"], + json!(["front", "back", "both"]) + ); + assert!(position.description.contains("exclude_from_pos_files")); + } + + #[tokio::test] + async fn impossible_gerber_side_is_rejected_before_running_kicad() { + let result = handle_export_position_file( + &json!({ + "board": "/tmp/board.kicad_pcb", + "output": "/tmp/positions.gbr", + "format": "gerber", + "side": "both", + "units": "mm" + }), + &ctx(), + ) + .await + .unwrap(); + assert!(result.is_error); + let text = match result.content.first() { + Some(crate::mcp::protocol::ToolContent::Text { text }) => text, + other => panic!("expected text error, got {other:?}"), + }; + let error: serde_json::Value = serde_json::from_str(text).unwrap(); + assert_eq!(error["error"]["field"], "side"); + } + + #[test] + fn malformed_layer_items_name_their_index() { + let error = optional_string_array(&json!({ "layers": ["F.Cu", 7] }), "layers") + .expect_err("numeric layer must be rejected"); + let text = match error.content.first() { + Some(crate::mcp::protocol::ToolContent::Text { text }) => text, + other => panic!("expected text error, got {other:?}"), + }; + let error: serde_json::Value = serde_json::from_str(text).unwrap(); + assert_eq!(error["error"]["field"], "layers[1]"); + } +} From cda33a356dd3759bcafec082b7cde80a8961d650 Mon Sep 17 00:00:00 2001 From: nordic-style <11313330+nordic-style@users.noreply.github.com> Date: Tue, 18 Aug 2026 01:46:18 +0200 Subject: [PATCH 2/2] fix(exports): verify every reported artifact Refs #252 --- crates/konnect-core/src/tools/cli.rs | 158 ++++++++++++++-- .../konnect-core/src/tools/manufacturing.rs | 173 +++++++++++++----- crates/konnect-core/src/tools/pcb_export.rs | 18 +- crates/konnect-core/src/tools/project.rs | 56 +++++- 4 files changed, 335 insertions(+), 70 deletions(-) diff --git a/crates/konnect-core/src/tools/cli.rs b/crates/konnect-core/src/tools/cli.rs index b7ae5e9d..2c44d32b 100644 --- a/crates/konnect-core/src/tools/cli.rs +++ b/crates/konnect-core/src/tools/cli.rs @@ -136,6 +136,31 @@ async fn run_cli(cli: &str, args: &[&str], timeout_dur: Duration) -> Result Result { + let metadata = tokio::fs::metadata(path).await.with_context(|| { + format!( + "{artifact} export reported success but did not create {}", + path.display() + ) + })?; + if !metadata.is_file() { + anyhow::bail!( + "{artifact} export reported success but {} is not a file", + path.display() + ); + } + if metadata.len() == 0 { + anyhow::bail!( + "{artifact} export reported success but created an empty file at {}", + path.display() + ); + } + Ok(metadata.len()) +} + // ─── ERC ───────────────────────────────────────────────────────────────────── /// Run ERC on a schematic and return parsed violations. @@ -367,7 +392,9 @@ pub async fn export_schematic_svg( ]; run_cli(cli, &args, LONG_TIMEOUT).await?; let stem = schematic.file_stem().unwrap_or_default().to_string_lossy(); - Ok(output_dir.join(format!("{}.svg", stem))) + let output = output_dir.join(format!("{}.svg", stem)); + verify_nonempty_file(&output, "schematic SVG").await?; + Ok(output) } /// KiCAD 10: `sch export pdf --output ` @@ -381,6 +408,7 @@ pub async fn export_schematic_pdf(cli: &str, schematic: &Path, output: &Path) -> schematic.to_str().unwrap(), ]; run_cli(cli, &args, LONG_TIMEOUT).await?; + verify_nonempty_file(output, "schematic PDF").await?; Ok(()) } @@ -447,6 +475,7 @@ pub async fn export_bom( options, ); run_cli(cli, &args, LONG_TIMEOUT).await?; + verify_nonempty_file(output, "BOM").await?; Ok(()) } @@ -505,7 +534,7 @@ pub async fn export_gerber( pcb: &Path, output_dir: &Path, layers: &[&str], -) -> Result<()> { +) -> Result> { let layers_csv = layers.join(","); let args = gerber_args( output_dir.to_str().unwrap_or(""), @@ -513,7 +542,40 @@ pub async fn export_gerber( &layers_csv, ); run_cli(cli, &args, LONG_TIMEOUT).await?; - Ok(()) + + let board_stem = pcb.file_stem().unwrap_or_default().to_string_lossy(); + let mut files = Vec::new(); + let mut entries = tokio::fs::read_dir(output_dir).await.with_context(|| { + format!( + "Gerber export reported success but output directory {} is missing", + output_dir.display() + ) + })?; + while let Some(entry) = entries.next_entry().await? { + let path = entry.path(); + let name = entry.file_name().to_string_lossy().to_string(); + let is_gerber = path + .extension() + .and_then(|extension| extension.to_str()) + .is_some_and(|extension| extension.to_ascii_lowercase().starts_with('g')); + if name.starts_with(board_stem.as_ref()) && is_gerber { + verify_nonempty_file(&path, "Gerber").await?; + files.push(path); + } + } + files.sort(); + let plot_count = files + .iter() + .filter(|path| path.extension().and_then(|value| value.to_str()) != Some("gbrjob")) + .count(); + if plot_count < layers.len().max(1) { + anyhow::bail!( + "Gerber export reported success but produced {plot_count} non-empty plot file(s) for {} requested layer(s) in {}", + layers.len(), + output_dir.display() + ); + } + Ok(files) } /// `--output` for a drill export names a *directory*, and some kicad-cli @@ -575,18 +637,43 @@ pub async fn export_drill(cli: &str, pcb: &Path, output_dir: &Path) -> Result [--layers ]... ` -pub async fn export_pdf(cli: &str, pcb: &Path, output: &Path, layers: &[&str]) -> Result<()> { - let mut args = vec!["pcb", "export", "pdf", "--output", output.to_str().unwrap()]; - for layer in layers { +fn pcb_pdf_args<'a>(output: &'a str, pcb: &'a str, layers_csv: &'a str) -> Vec<&'a str> { + let mut args = vec!["pcb", "export", "pdf", "--output", output]; + if !layers_csv.is_empty() { args.push("--layers"); - args.push(layer); + args.push(layers_csv); } - args.push(pcb.to_str().unwrap()); + // Without an explicit mode KiCad may treat --output as a directory. The + // MCP contract names one PDF file, so make that interpretation explicit. + args.push("--mode-single"); + args.push(pcb); + args +} + +/// KiCad 10: `pcb export pdf --output [--layers ] --mode-single +/// ` +pub async fn export_pdf(cli: &str, pcb: &Path, output: &Path, layers: &[&str]) -> Result<()> { + let layers_csv = layers.join(","); + let args = pcb_pdf_args( + output.to_str().unwrap_or(""), + pcb.to_str().unwrap_or(""), + &layers_csv, + ); run_cli(cli, &args, LONG_TIMEOUT).await?; + verify_nonempty_file(output, "PCB PDF").await?; Ok(()) } @@ -677,6 +764,7 @@ pub async fn export_position_file( side, ); run_cli(cli, &args, LONG_TIMEOUT).await?; + verify_nonempty_file(output, "position file").await?; Ok(()) } @@ -981,6 +1069,54 @@ mod erc_parse_tests { } } +#[cfg(test)] +mod artifact_verification_tests { + use super::*; + + #[tokio::test] + async fn missing_and_empty_artifacts_are_not_successes() { + let dir = tempfile::tempdir().unwrap(); + let missing = dir.path().join("missing.pdf"); + let error = verify_nonempty_file(&missing, "test PDF") + .await + .expect_err("missing file must fail"); + assert!(error.to_string().contains("did not create")); + + let empty = dir.path().join("empty.pdf"); + std::fs::write(&empty, []).unwrap(); + let error = verify_nonempty_file(&empty, "test PDF") + .await + .expect_err("empty file must fail"); + assert!(error.to_string().contains("empty file")); + + let real = dir.path().join("real.pdf"); + std::fs::write(&real, b"%PDF-test").unwrap(); + assert_eq!(verify_nonempty_file(&real, "test PDF").await.unwrap(), 9); + } + + #[test] + fn pcb_pdf_uses_one_csv_layer_argument_and_one_file_mode() { + let args = pcb_pdf_args( + "/out/board.pdf", + "/tmp/board.kicad_pcb", + "F.Cu,B.Cu,F.SilkS,B.SilkS,Edge.Cuts", + ); + assert_eq!( + args.iter() + .filter(|argument| **argument == "--layers") + .count(), + 1 + ); + let layers = args + .iter() + .position(|argument| *argument == "--layers") + .map(|index| args[index + 1]); + assert_eq!(layers, Some("F.Cu,B.Cu,F.SilkS,B.SilkS,Edge.Cuts")); + assert!(args.contains(&"--mode-single")); + assert_eq!(args.last().copied(), Some("/tmp/board.kicad_pcb")); + } +} + #[cfg(test)] mod gerber_export_tests { use super::*; @@ -1082,7 +1218,7 @@ mod drill_export_tests { async fn drill_files_are_collected_sorted_and_filtered_by_extension() { let dir = tempfile::tempdir().unwrap(); for name in ["board-PTH.drl", "board-NPTH.drl", "board-drl_map.pdf"] { - std::fs::write(dir.path().join(name), "").unwrap(); + std::fs::write(dir.path().join(name), "non-empty").unwrap(); } let files = drill_files_in(dir.path()).await; let names: Vec<_> = files diff --git a/crates/konnect-core/src/tools/manufacturing.rs b/crates/konnect-core/src/tools/manufacturing.rs index cf0484bd..b2c2a882 100644 --- a/crates/konnect-core/src/tools/manufacturing.rs +++ b/crates/konnect-core/src/tools/manufacturing.rs @@ -8,7 +8,7 @@ use crate::tool; use crate::tools::{get_path, ToolContext, ToolDef}; use serde_json::json; use std::path::PathBuf; -use tracing::{debug, error, info, warn}; +use tracing::{debug, error, info}; use super::{cli, pcb_export}; @@ -175,6 +175,7 @@ async fn handle_export_manufacturing_package( let cli_path = &ctx.config.kicad_cli; let mut files_generated = Vec::new(); + let mut verified_paths = Vec::new(); let mut warnings = Vec::new(); // 1. Export Gerbers @@ -182,12 +183,14 @@ async fn handle_export_manufacturing_package( tokio::fs::create_dir_all(&gerber_dir).await?; let gerber_layer_refs = gerber_layers.iter().map(String::as_str).collect::>(); match cli::export_gerber(cli_path, &board, &gerber_dir, &gerber_layer_refs).await { - Ok(()) => { - info!("[BETA] Gerber export succeeded"); + Ok(gerber_files) => { + info!(files = gerber_files.len(), "[BETA] Gerber export succeeded"); + verified_paths.extend(gerber_files.iter().cloned()); files_generated.push(json!({ "type": "gerber", "path": gerber_dir.to_str().unwrap_or(""), - "layers": gerber_layers.clone() + "layers": gerber_layers.clone(), + "files": gerber_files.iter().map(|path| path.to_str().unwrap_or("")).collect::>() })); } Err(e) => { @@ -204,22 +207,18 @@ async fn handle_export_manufacturing_package( // the real Excellon output never appeared in the file list at all. match cli::export_drill(cli_path, &board, &gerber_dir).await { Ok(drill_files) => { - if drill_files.is_empty() { - warn!("[BETA] Drill export produced no .drl files"); - warnings.push("Drill export produced no .drl files.".to_string()); - } else { - info!(files = drill_files.len(), "[BETA] Drill export succeeded"); - for file in &drill_files { - files_generated.push(json!({ - "type": "drill", - "path": file.to_str().unwrap_or("") - })); - } + info!(files = drill_files.len(), "[BETA] Drill export succeeded"); + verified_paths.extend(drill_files.iter().cloned()); + for file in &drill_files { + files_generated.push(json!({ + "type": "drill", + "path": file.to_str().unwrap_or("") + })); } } Err(e) => { - warn!(error = %e, "[BETA] Drill export failed (may be included in gerbers)"); - // Not critical — some gerber exports include drill + error!(error = %e, "[BETA] Drill export failed"); + warnings.push(format!("Drill export failed: {e}")); } } @@ -243,6 +242,7 @@ async fn handle_export_manufacturing_package( { Ok(()) => { info!("[BETA] Position file export succeeded"); + verified_paths.push(pos_path.clone()); files_generated.push(json!({ "type": "pick_and_place", "path": pos_path.to_str().unwrap_or(""), @@ -272,6 +272,7 @@ async fn handle_export_manufacturing_package( match cli::export_bom(cli_path, sch, &bom_path, &bom_options).await { Ok(()) => { info!("[BETA] BOM export succeeded"); + verified_paths.push(bom_path.clone()); files_generated.push(json!({ "type": "bom", "path": bom_path.to_str().unwrap_or(""), @@ -289,23 +290,30 @@ async fn handle_export_manufacturing_package( } } - // List all files in output dir - let mut all_files = Vec::new(); - if let Ok(mut rd) = tokio::fs::read_dir(&output_dir).await { - while let Ok(Some(entry)) = rd.next_entry().await { - all_files.push(entry.file_name().to_string_lossy().to_string()); - } - } - // Also list gerber subdir - if let Ok(mut rd) = tokio::fs::read_dir(&gerber_dir).await { - while let Ok(Some(entry)) = rd.next_entry().await { - all_files.push(format!("gerbers/{}", entry.file_name().to_string_lossy())); - } - } + // Derive the public file list only from artifacts the CLI boundary already + // verified as regular and non-empty. A stale or empty directory entry can + // no longer make an incomplete package look successful (#252). + let mut all_files = verified_paths + .iter() + .map(|path| { + path.strip_prefix(&output_dir) + .unwrap_or(path) + .to_string_lossy() + .replace('\\', "/") + }) + .collect::>(); all_files.sort(); + all_files.dedup(); + + let complete = warnings.is_empty(); let summary = format!( - "Generated for {}. {} files total. {}", + "{} for {}. {} verified non-empty files. {}", + if complete { + "Complete package" + } else { + "INCOMPLETE package" + }, fab_house.to_uppercase(), all_files.len(), if warnings.is_empty() { @@ -316,30 +324,40 @@ async fn handle_export_manufacturing_package( ); info!( + complete = complete, files = all_files.len(), warnings = warnings.len(), - "[BETA] Manufacturing package complete" + "[BETA] Manufacturing package finished" ); - Ok(CallToolResult::text( - serde_json::to_string(&json!({ - "fab_house": fab_house, - "output_dir": output_dir.to_str().unwrap_or(""), - "files": all_files, - "files_generated": files_generated, - "gerber_layers": gerber_layers, - "position_units": if include_assembly { Some(position_units) } else { None }, - "position_side": if include_assembly { Some(position_side) } else { None }, - "warnings": warnings, - "summary": summary, - "next_steps": format!( + let next_steps = if complete { + format!( "Upload the contents of {} to {}'s order page. Gerbers go in the PCB order, BOM + positions go in the assembly order.", output_dir.display(), fab_house.to_uppercase() ) - })) - .unwrap(), - )) + } else { + "Do not upload this package. Resolve every warning and export again.".to_string() + }; + let body = serde_json::to_string(&json!({ + "complete": complete, + "fab_house": fab_house, + "output_dir": output_dir.to_str().unwrap_or(""), + "files": all_files, + "files_generated": files_generated, + "gerber_layers": gerber_layers, + "position_units": if include_assembly { Some(position_units) } else { None }, + "position_side": if include_assembly { Some(position_side) } else { None }, + "warnings": warnings, + "summary": summary, + "next_steps": next_steps + })) + .unwrap(); + Ok(if complete { + CallToolResult::text(body) + } else { + CallToolResult::error(body) + }) } fn invalid_manufacturing_argument(field: &str, reason: impl Into) -> CallToolResult { @@ -614,6 +632,15 @@ async fn handle_estimate_cost( mod package_export_option_tests { use super::*; + fn result_json(result: &CallToolResult) -> serde_json::Value { + match result.content.first() { + Some(crate::mcp::protocol::ToolContent::Text { text }) => { + serde_json::from_str(text).unwrap() + } + other => panic!("expected text result, got {other:?}"), + } + } + #[test] fn package_schema_exposes_applied_gerber_and_position_options() { let package = tools() @@ -628,6 +655,58 @@ mod package_export_option_tests { json!(["front", "back", "both"]) ); } + + #[cfg(unix)] + #[tokio::test] + async fn package_is_an_error_when_cli_success_produces_no_artifacts() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let board = dir.path().join("live_ipc.kicad_pcb"); + // Real KiCad 9 output, as required for tests that parse board layers. + std::fs::write( + &board, + include_str!("../../../konnect-ipc/tests/fixtures/live_ipc.kicad_pcb"), + ) + .unwrap(); + let cli = dir.path().join("fake-kicad-cli"); + std::fs::write(&cli, "#!/bin/sh\nexit 0\n").unwrap(); + let mut permissions = std::fs::metadata(&cli).unwrap().permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&cli, permissions).unwrap(); + let ctx = ToolContext::new( + crate::tools::ServerConfig { + kicad_cli: cli.display().to_string(), + kicad_binary: String::new(), + ipc_address: String::new(), + project_dir: None, + jlcpcb_db_path: None, + auto_load_toolsets: false, + eager_toolsets: false, + }, + std::sync::Arc::new(crate::router::ToolRouter::new()), + ); + + let result = handle_export_manufacturing_package( + &json!({ + "board": board.display().to_string(), + "output_dir": dir.path().join("package").display().to_string(), + "include_assembly": false + }), + &ctx, + ) + .await + .unwrap(); + assert!(result.is_error, "incomplete package must fail closed"); + let body = result_json(&result); + assert_eq!(body["complete"], false); + assert_eq!(body["files"], json!([])); + assert!(body["warnings"].as_array().unwrap().len() >= 2, "{body}"); + assert!(body["next_steps"] + .as_str() + .unwrap() + .starts_with("Do not upload")); + } } // ─── Helpers ───────────────────────────────────────────────────────────────── diff --git a/crates/konnect-core/src/tools/pcb_export.rs b/crates/konnect-core/src/tools/pcb_export.rs index dc11d686..7c295644 100644 --- a/crates/konnect-core/src/tools/pcb_export.rs +++ b/crates/konnect-core/src/tools/pcb_export.rs @@ -427,7 +427,7 @@ async fn handle_export_gerber( tokio::fs::create_dir_all(&output_dir).await?; let cli = &ctx.config.kicad_cli; - cli::export_gerber(cli, &board, &output_dir, &layer_refs).await?; + let mut verified_files = cli::export_gerber(cli, &board, &output_dir, &layer_refs).await?; if drill { // kicad-cli also has a dedicated drill export. Its --output is a @@ -436,16 +436,16 @@ async fn handle_export_gerber( // `output_dir.join("drill.drl")` made KiCad create a *directory* // called drill.drl and bury the drill files inside it, where the // listing below never saw them. - let _ = cli::export_drill(cli, &board, &output_dir).await; // best-effort + verified_files.extend(cli::export_drill(cli, &board, &output_dir).await?); } - // List produced files - let mut files = Vec::new(); - if let Ok(mut rd) = tokio::fs::read_dir(&output_dir).await { - while let Ok(Some(entry)) = rd.next_entry().await { - files.push(entry.file_name().to_string_lossy().to_string()); - } - } + // Report only files that this export path verified as regular and + // non-empty; never echo a directory listing containing stale artifacts. + let mut files = verified_files + .iter() + .filter_map(|path| path.file_name()) + .map(|name| name.to_string_lossy().to_string()) + .collect::>(); files.sort(); Ok(CallToolResult::text( diff --git a/crates/konnect-core/src/tools/project.rs b/crates/konnect-core/src/tools/project.rs index 97e1c763..39fec9f4 100644 --- a/crates/konnect-core/src/tools/project.rs +++ b/crates/konnect-core/src/tools/project.rs @@ -363,9 +363,8 @@ async fn handle_snapshot_project( let pcb = PathBuf::from(pcb_str); let pcb_pdf_name = format!("{}_pcb_{}_{}.pdf", stem, label, ts); let pcb_pdf_path = output_dir.join(&pcb_pdf_name); - let layers = &["F.Cu", "B.Cu", "F.Silkscreen", "B.Silkscreen", "Edge.Cuts"]; - let _ = - crate::tools::cli::export_pdf(&ctx.config.kicad_cli, &pcb, &pcb_pdf_path, layers).await; + let layers = &["F.Cu", "B.Cu", "F.SilkS", "B.SilkS", "Edge.Cuts"]; + crate::tools::cli::export_pdf(&ctx.config.kicad_cli, &pcb, &pcb_pdf_path, layers).await?; result["pcb_snapshot"] = json!(pcb_pdf_path.display().to_string()); } @@ -699,6 +698,57 @@ mod tests { ); } + #[cfg(unix)] + #[tokio::test] + async fn snapshot_propagates_a_missing_pcb_artifact() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let cli = dir.path().join("fake-kicad-cli"); + // Produce the first (schematic) PDF, then report success without + // producing the PCB PDF. This is the exact phantom-path failure #252 + // described, independent of whether a real KiCad is installed. + std::fs::write( + &cli, + "#!/bin/sh\nif [ \"$1\" = \"sch\" ]; then\n while [ \"$#\" -gt 0 ]; do\n if [ \"$1\" = \"--output\" ]; then\n shift\n printf '%s' '%PDF-test' > \"$1\"\n break\n fi\n shift\n done\nfi\nexit 0\n", + ) + .unwrap(); + let mut permissions = std::fs::metadata(&cli).unwrap().permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&cli, permissions).unwrap(); + + let schematic = dir.path().join("voice.kicad_sch"); + let board = dir.path().join("voice.kicad_pcb"); + std::fs::write(&schematic, "placeholder").unwrap(); + std::fs::write(&board, "placeholder").unwrap(); + let ctx = ToolContext::new( + ServerConfig { + kicad_cli: cli.display().to_string(), + 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()), + ); + + let error = handle_snapshot_project( + &json!({ + "schematic": schematic.display().to_string(), + "pcb": board.display().to_string(), + "output_dir": dir.path().join("snapshots").display().to_string(), + "label": "regression" + }), + &ctx, + ) + .await + .expect_err("missing PCB PDF must fail the snapshot call"); + assert!(error.to_string().contains("did not create"), "{error:#}"); + assert!(error.to_string().contains("pcb"), "{error:#}"); + } + fn response_json(result: &CallToolResult) -> serde_json::Value { match &result.content[0] { crate::mcp::protocol::ToolContent::Text { text } => serde_json::from_str(text).unwrap(),