diff --git a/20-agents/_core/ui/manifest.yaml b/20-agents/_core/ui/manifest.yaml index 0535e635e..0af81f7e4 100644 --- a/20-agents/_core/ui/manifest.yaml +++ b/20-agents/_core/ui/manifest.yaml @@ -104,6 +104,7 @@ commands: html: string # the self-contained HTML document bytes: int # size of `html` in bytes output-path: string # present only when an output-path was given + path: string # alias of `output-path` (the written artifact location) skills: - descriptor-authoring.md diff --git a/cli/src/commands/sidecar.rs b/cli/src/commands/sidecar.rs index 946b5c5f5..5022b5688 100644 --- a/cli/src/commands/sidecar.rs +++ b/cli/src/commands/sidecar.rs @@ -525,18 +525,15 @@ fn find_bridge_in_dir(bridge: &Bridge, install_dir: &std::path::Path) -> Option< None } +/// A bridge binary found on `PATH` — the "legacy" install shape, from before +/// bridges moved under `/bridges`. +/// +/// Delegates to [`crate::which`] rather than scanning `PATH` here. This had +/// grown its own copy that appended `.exe` and nothing else, so on Windows a +/// bridge installed as a `.cmd` shim read as absent — the exact miss that +/// module's doc comment describes and that its suffix list exists to close. fn which_binary(name: &str) -> Option { - let name_exe = if cfg!(windows) { - format!("{name}.exe") - } else { - name.to_string() - }; - std::env::var_os("PATH") - .map(|paths| std::env::split_paths(&paths).collect::>()) - .unwrap_or_default() - .into_iter() - .map(|dir| dir.join(&name_exe)) - .find(|p| p.is_file()) + crate::which::find_on_path(name) } fn lookup_bridge(host: &str) -> Result<&'static Bridge, AwareError> { diff --git a/cli/src/render/ifc.rs b/cli/src/render/ifc.rs index 38ef53ea9..d1bd4a095 100644 --- a/cli/src/render/ifc.rs +++ b/cli/src/render/ifc.rs @@ -3309,26 +3309,7 @@ pub fn ifc_write(args: &Value, dry_run: bool) -> Result { ), ); - if let Some(path) = args - .get("output-path") - .and_then(|v| v.as_str()) - .map(str::trim) - .filter(|s| !s.is_empty()) - { - if !dry_run { - if let Some(parent) = std::path::Path::new(path).parent() - && !parent.as_os_str().is_empty() - { - std::fs::create_dir_all(parent).map_err(|e| { - AwareError::Internal(format!("ifc: create {}: {e}", parent.display())) - })?; - } - std::fs::write(path, built.doc.as_bytes()) - .map_err(|e| AwareError::Internal(format!("ifc: write {path}: {e}")))?; - } - out.insert("output-path".into(), Value::String(path.to_string())); - out.insert("path".into(), Value::String(path.to_string())); - } + super::write_artifact(&mut out, args, dry_run, built.doc.as_bytes(), "ifc")?; Ok(Value::Object(out)) } diff --git a/cli/src/render/mod.rs b/cli/src/render/mod.rs index 86d23a818..4c2aaf601 100644 --- a/cli/src/render/mod.rs +++ b/cli/src/render/mod.rs @@ -23,3 +23,188 @@ pub(super) fn abs_path(path: &str) -> String { .map(|p| p.to_string_lossy().into_owned()) .unwrap_or_else(|_| path.to_string()) } + +/// The optional `output-path` half of a render primitive's output contract: +/// write the rendered artifact when one was asked for, and stamp the location +/// and size onto the response. +/// +/// Every primitive that produces an artifact — `html-report.render`, +/// `ui.render`, `viewer-3d.render`, `ifc.write` — offers the same deal: +/// `output-path` is optional, a real run writes the bytes there (creating +/// parents), and a preview (`--dry-run` / `--simulate`) reports the would-be +/// path and size without touching disk. Four byte-for-byte copies of that +/// block existed, and `viewer_3d`'s module doc already described itself as +/// mirroring the other two. +/// +/// They had drifted, which is the reason to collapse them rather than leave +/// them alone: three emitted the documented `path` alias beside `output-path` +/// and `ui.render` emitted only `output-path`, so an app reading +/// `{{ node.path }}` — the spelling an engineering output seal uses — got a +/// value from a `viewer-3d` node and nothing from a `ui` node. Nothing chose +/// that; the fourth copy was simply written without the line. One +/// implementation is what stops the next divergence. +/// +/// `label` is the primitive's name for its I/O errors ("ifc: write …"), which +/// is data rather than a mode switch: no caller changes *behaviour* through +/// this function, so none of them needs a flag to get its old output back. +/// `contents` is the artifact's bytes — HTML for the three renderers, the IFC +/// document for `ifc.write` — so the helper stays indifferent to what was +/// rendered, per decalog #4: the substrate writes a file, it does not know the +/// format. +pub(super) fn write_artifact( + out: &mut serde_json::Map, + args: &serde_json::Value, + dry_run: bool, + contents: &[u8], + label: &str, +) -> Result<(), crate::error::AwareError> { + use crate::error::AwareError; + use serde_json::Value; + + let Some(path) = args + .get("output-path") + .and_then(|v| v.as_str()) + .map(str::trim) + .filter(|s| !s.is_empty()) + else { + return Ok(()); + }; + + // Real run only: a preview returns the would-be path and size but never + // touches disk. + if !dry_run { + if let Some(parent) = std::path::Path::new(path).parent() + && !parent.as_os_str().is_empty() + { + std::fs::create_dir_all(parent).map_err(|e| { + AwareError::Internal(format!("{label}: create {}: {e}", parent.display())) + })?; + } + std::fs::write(path, contents) + .map_err(|e| AwareError::Internal(format!("{label}: write {path}: {e}")))?; + } + + out.insert("output-path".into(), Value::String(path.to_string())); + // `path` alias: existing apps reference the artifact under both names — + // `{{ node.output-path }}` (e.g. an email attachment) and `{{ node.path }}` + // (e.g. an engineering output seal). Both are declared in the manifest + // schema of every primitive that reaches here. + out.insert("path".into(), Value::String(path.to_string())); + out.insert("bytes".into(), Value::from(contents.len() as u64)); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::{Value, json}; + + /// Run the helper the way a primitive does and hand back the response map. + fn call(args: Value, dry_run: bool, contents: &[u8]) -> serde_json::Map { + let mut out = serde_json::Map::new(); + write_artifact(&mut out, &args, dry_run, contents, "test").unwrap(); + out + } + + #[test] + fn no_output_path_leaves_the_response_untouched() { + for args in [ + json!({}), + json!({ "output-path": "" }), + json!({"output-path": " "}), + ] { + let out = call(args.clone(), false, b"x"); + assert!( + out.is_empty(), + "{args} asks for no artifact, so no path/bytes keys: {out:?}" + ); + } + } + + #[test] + fn a_dry_run_reports_the_path_and_size_without_writing() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("sub").join("a.html"); + let ps = path.to_string_lossy().to_string(); + + let out = call(json!({ "output-path": ps.clone() }), true, b"hello"); + + assert_eq!(out["output-path"], json!(ps)); + assert_eq!(out["bytes"], json!(5)); + assert!(!path.exists(), "a dry run must not write the file"); + assert!( + !path.parent().unwrap().exists(), + "nor create its parent directory" + ); + } + + #[test] + fn a_real_run_creates_parents_and_writes_the_bytes() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("sub").join("deeper").join("a.ifc"); + let ps = path.to_string_lossy().to_string(); + + let out = call(json!({ "output-path": ps }), false, b"ISO-10303-21;"); + + assert_eq!(std::fs::read(&path).unwrap(), b"ISO-10303-21;"); + assert_eq!(out["bytes"], json!(13)); + } + + /// The drift this helper exists to end: `html-report`, `viewer-3d` and `ifc` + /// emitted `path` beside `output-path`; `ui.render`'s copy of the block was + /// written without it, so `{{ node.path }}` resolved against three of the + /// four artifact producers and silently against the fourth. One + /// implementation means one answer. + #[test] + fn path_is_always_an_alias_of_output_path() { + let tmp = tempfile::tempdir().unwrap(); + let ps = tmp.path().join("a.html").to_string_lossy().to_string(); + + for dry_run in [true, false] { + let out = call(json!({ "output-path": ps.clone() }), dry_run, b"x"); + assert_eq!(out["path"], out["output-path"], "dry_run={dry_run}"); + } + } + + /// `output-path` is echoed as given, minus surrounding whitespace — the file + /// is written to the trimmed path, so reporting the untrimmed one would name + /// a location that does not exist. + #[test] + fn a_padded_path_is_trimmed_in_both_the_write_and_the_report() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("a.html"); + let ps = path.to_string_lossy().to_string(); + + let out = call(json!({ "output-path": format!(" {ps} ") }), false, b"x"); + + assert_eq!(out["output-path"], json!(ps)); + assert!(path.exists()); + } + + /// The label is the caller's name for its own I/O errors, and nothing else: + /// it never changes which keys come back or whether a write happens. + #[test] + fn the_label_only_names_the_primitive_in_an_io_error() { + let tmp = tempfile::tempdir().unwrap(); + // A path whose parent is an existing *file* cannot be created. + let blocker = tmp.path().join("not-a-dir"); + std::fs::write(&blocker, b"").unwrap(); + let doomed = blocker.join("a.html").to_string_lossy().to_string(); + + let mut out = serde_json::Map::new(); + let err = write_artifact( + &mut out, + &json!({ "output-path": doomed }), + false, + b"x", + "ifc", + ) + .unwrap_err(); + + assert!(err.to_string().contains("ifc: "), "{err}"); + assert!( + out.is_empty(), + "a failed write reports nothing it did not do: {out:?}" + ); + } +} diff --git a/cli/src/render/ui.rs b/cli/src/render/ui.rs index 960671287..8d7fdfd7a 100644 --- a/cli/src/render/ui.rs +++ b/cli/src/render/ui.rs @@ -518,27 +518,7 @@ pub fn ui_render(args: &Value, dry_run: bool) -> Result { out.insert("html".into(), Value::String(html.clone())); out.insert("bytes".into(), Value::from(html.len() as u64)); - if let Some(path) = args - .get("output-path") - .and_then(|v| v.as_str()) - .map(str::trim) - .filter(|s| !s.is_empty()) - { - // Real run only: a preview (--dry-run / --simulate) returns the HTML and - // the would-be path but never touches disk (same contract as html-report). - if !dry_run { - if let Some(parent) = std::path::Path::new(path).parent() - && !parent.as_os_str().is_empty() - { - std::fs::create_dir_all(parent).map_err(|e| { - AwareError::Internal(format!("ui render: create {}: {e}", parent.display())) - })?; - } - std::fs::write(path, html.as_bytes()) - .map_err(|e| AwareError::Internal(format!("ui render: write {path}: {e}")))?; - } - out.insert("output-path".into(), Value::String(path.to_string())); - } + super::write_artifact(&mut out, args, dry_run, html.as_bytes(), "ui render")?; Ok(Value::Object(out)) } diff --git a/cli/src/render/viewer_3d.rs b/cli/src/render/viewer_3d.rs index 851911c04..d01f440c4 100644 --- a/cli/src/render/viewer_3d.rs +++ b/cli/src/render/viewer_3d.rs @@ -3844,28 +3844,7 @@ pub fn viewer_3d_render(args: &Value, dry_run: bool) -> Result Result { out.insert("htmlReport".into(), Value::String(html.clone())); out.insert("item-count".into(), Value::from(count)); - if let Some(path) = args - .get("output-path") - .and_then(|v| v.as_str()) - .map(str::trim) - .filter(|s| !s.is_empty()) - { - // Real run only: a preview (--dry-run / --simulate) returns the HTML + the - // would-be path/size but never touches disk. - if !dry_run { - if let Some(parent) = std::path::Path::new(path).parent() - && !parent.as_os_str().is_empty() - { - std::fs::create_dir_all(parent).map_err(|e| { - AwareError::Internal(format!("html-report: create {}: {e}", parent.display())) - })?; - } - std::fs::write(path, html.as_bytes()) - .map_err(|e| AwareError::Internal(format!("html-report: write {path}: {e}")))?; - } - out.insert("output-path".into(), Value::String(path.to_string())); - // `path` alias: existing apps reference the artifact under both names — - // `{{ node.output-path }}` (e.g. an email attachment) and `{{ node.path }}` - // (e.g. an engineering output seal). Both are declared in the manifest schema. - out.insert("path".into(), Value::String(path.to_string())); - out.insert("bytes".into(), Value::from(html.len() as u64)); - } + crate::render::write_artifact(&mut out, &args, dry_run, html.as_bytes(), "html-report")?; Ok(Value::Object(out)) } diff --git a/cli/src/sidecar.rs b/cli/src/sidecar.rs index fb64ee94a..169a5b061 100644 --- a/cli/src/sidecar.rs +++ b/cli/src/sidecar.rs @@ -70,15 +70,14 @@ fn discover_named(env_var: &str, stem: &str) -> Result { } } - // 3. On PATH - if let Ok(path_var) = std::env::var("PATH") { - let sep = if cfg!(windows) { ';' } else { ':' }; - for entry in path_var.split(sep) { - let candidate = PathBuf::from(entry).join(&bin_name); - if candidate.is_file() { - return Ok(candidate); - } - } + // 3. On PATH. Through `crate::which`, which exists to be the one answer to + // "what will `Command::new` actually launch": it splits `PATH` with + // `std::env::split_paths` (a quoted Windows entry survives, where a hand + // `split(';')` does not) and searches the spawnable Windows suffixes + // rather than `.exe` alone. `stem`, not `bin_name` — appending the + // platform suffix is that module's job. + if let Some(found) = crate::which::find_on_path(stem) { + return Ok(found); } Err(AwareError::NotFound(format!( diff --git a/cli/tests/agent_invoke.rs b/cli/tests/agent_invoke.rs index 8a6653bae..5f931c776 100644 --- a/cli/tests/agent_invoke.rs +++ b/cli/tests/agent_invoke.rs @@ -139,7 +139,8 @@ fn invoke_ui_render_writes_html_artifact() { .args(["agent", "invoke", "ui", "render", "--inputs", &args]) .assert() .success() - .stdout(predicate::str::contains("\"bytes\"")); + .stdout(predicate::str::contains("\"bytes\"")) + .stdout(predicate::str::contains("\"path\":")); let html = std::fs::read_to_string(&out_path).expect("render wrote the artifact"); assert!(html.starts_with("")); assert!(html.contains("data-panel-id=\"p\"")); diff --git a/cli/tests/sidecar_cli.rs b/cli/tests/sidecar_cli.rs index 64cc76cbb..803dd546d 100644 --- a/cli/tests/sidecar_cli.rs +++ b/cli/tests/sidecar_cli.rs @@ -56,11 +56,11 @@ fn plant_managed(home: &std::path::Path, binary: &str, version: &str) -> std::pa exe } -/// The name a legacy on-PATH bridge has to carry to be found by `which_binary`, -/// which appends `.exe` only on Windows. +/// Exercise the Windows npm-shim shape that the old local PATH scan missed. +/// On Unix a bare file is the corresponding spawnable shape. fn legacy_name(binary: &str) -> String { if cfg!(windows) { - format!("{binary}.exe") + format!("{binary}.cmd") } else { binary.to_string() }