refactor: share artifact writing and PATH binary lookup

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

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 `<aware-home>/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<PathBuf> {
-    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::<Vec<_>>())
-        .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> {

cli/src/render/ifc.rs

@@ -3309,26 +3309,7 @@ pub fn ifc_write(args: &Value, dry_run: bool) -> Result<Value, AwareError> {
         ),
     );
 
-    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))
 }

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<String, serde_json::Value>,
+    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<String, Value> {
+        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:?}"
+        );
+    }
+}

cli/src/render/ui.rs

@@ -518,27 +518,7 @@ pub fn ui_render(args: &Value, dry_run: bool) -> Result<Value, AwareError> {
     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))
 }

cli/src/render/viewer_3d.rs

@@ -3844,28 +3844,7 @@ pub fn viewer_3d_render(args: &Value, dry_run: bool) -> Result<Value, AwareError
     out.insert("unsupported".into(), Value::Array(receipt.unsupported));
     out.insert("warnings".into(), Value::Array(receipt.warnings));
 
-    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 / ui).
-        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!("viewer-3d: create {}: {e}", parent.display()))
-                })?;
-            }
-            std::fs::write(path, html.as_bytes())
-                .map_err(|e| AwareError::Internal(format!("viewer-3d: 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, html.as_bytes(), "viewer-3d")?;
 
     Ok(Value::Object(out))
 }

cli/src/runtime/invoker.rs

@@ -1816,32 +1816,7 @@ fn render_html_report(args: Value, dry_run: bool) -> Result<Value, AwareError> {
     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))
 }

cli/src/sidecar.rs

@@ -70,15 +70,14 @@ fn discover_named(env_var: &str, stem: &str) -> Result<PathBuf, AwareError> {
         }
     }
 
-    // 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!(

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("<!DOCTYPE html>"));
     assert!(html.contains("data-panel-id=\"p\""));

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()
     }