From 7b6556b90c8c70de9666b9fc41d268edd595c6ee Mon Sep 17 00:00:00 2001 From: Pawel Lisowski Date: Mon, 7 Sep 2026 03:50:07 +0000 Subject: [PATCH 1/4] refactor: collapse three re-typed abstractions onto one implementation each MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three helpers had been written more than once and drifted. Each is now one implementation both callers use. 1. The `output-path` artifact writer — 4 copies. `html-report.render`, `ui.render`, `viewer-3d.render` and `ifc.write` each carried a byte-for-byte copy of "write the rendered bytes to `output-path` on a real run, report the location and size either way"; `viewer_3d`'s module doc already described itself as mirroring the other two. Three emitted the documented `path` alias beside `output-path` and `ui.render` emitted only `output-path`, so `{{ node.path }}` resolved against three of the four artifact producers and silently against the fourth. Now `render::write_artifact`, with `ui`'s manifest declaring the alias its siblings already declared. 2. The app-manifest selector — 3 inline scans. `manifest::loader::find_app_manifest` prefers `.flo`, then any `.flo`, then any `.app`. `resolve_validate_target` was moved onto it and its comment says the rule is "shared with install" — but install kept three inline `read_dir` scans that took whatever the filesystem yielded first, so a directory holding two sources could be validated as one app and installed as the other inside a single `aware app install`. Each site keeps its `read_dir` probe, so a permissions or IO failure still surfaces as itself rather than as "no .flo or .app file". 3. The PATH binary lookup — 2 scans that bypassed `crate::which`. That module exists to be the one answer to what `Command::new` can launch, and its doc comment is about this exact drift. Both copies appended `.exe` and nothing else, so a bridge installed as a `.cmd` shim read as absent on Windows; `sidecar.rs` also hand-split PATH on `;`, which mishandles a quoted entry that `std::env::split_paths` handles. Same first answer wherever an `.exe` exists — strictly a superset. Considered and left apart, being different abstractions that only look alike: `truncate_detail` / `truncate_error_detail` (different budgets and suffix contracts, both deliberate); the two `Envelope` and two `Hit` structs (shared names, unrelated shapes); `app_lock::find_app_source` (wider extension set, file-or-dir, serving `app compile`); the read-side and write-side `Provenance` models (opposite optionality); `builder::{npm,ruby}::extract_surface` (one signature, two language grammars); and the per-host-version reflected agents under `20-agents/aeco/architecture/`, whose identical command docs are what reflection produces, not drift. Gates from `cli/`: cargo fmt --all --check, cargo clippy --all-targets -D warnings, cargo test — all pass (1230 unit + integration tests green). --- 20-agents/_core/ui/manifest.yaml | 1 + cli/src/commands/app.rs | 35 ++---- cli/src/commands/sidecar.rs | 19 ++-- cli/src/install/local.rs | 60 +++++++--- cli/src/render/ifc.rs | 21 +--- cli/src/render/mod.rs | 185 +++++++++++++++++++++++++++++++ cli/src/render/ui.rs | 22 +--- cli/src/render/viewer_3d.rs | 23 +--- cli/src/runtime/invoker.rs | 27 +---- cli/src/sidecar.rs | 17 ++- 10 files changed, 264 insertions(+), 146 deletions(-) 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/app.rs b/cli/src/commands/app.rs index 8fc118f32..91553dda6 100644 --- a/cli/src/commands/app.rs +++ b/cli/src/commands/app.rs @@ -1169,18 +1169,13 @@ fn install(ctx: &Context, spec: &str) -> Result<(), AwareError> { // (app-spec § Safety contract: "aware app validate refuses to install an // app missing `safety:` on a write-mode node"; install must enforce the // same contract as the standalone `validate` command, #134). - let src_manifest = std::fs::read_dir(&path)? - .flatten() - .map(|e| e.path()) - .find(|p| { - matches!( - p.extension().and_then(|e| e.to_str()), - Some("flo") | Some("app") - ) - }) - .ok_or_else(|| { - AwareError::Validation(format!("no .flo or .app file in {}", path.display())) - })?; + // Same selection rule as `install_app_from_path` below and as + // `resolve_validate_target` — otherwise this pre-flight validates one + // manifest and the install that follows writes another. + std::fs::read_dir(&path)?; + let src_manifest = crate::manifest::loader::find_app_manifest(&path).ok_or_else(|| { + AwareError::Validation(format!("no .flo or .app file in {}", path.display())) + })?; let src_app = crate::manifest::loader::load_app(&src_manifest)?; let mut issues = crate::validate::validate_app(&src_app); // Missing agents are reported separately from `issues` so they surface even @@ -1229,18 +1224,10 @@ fn install(ctx: &Context, spec: &str) -> Result<(), AwareError> { // Locate the installed .flo / .app file let app_dir = ctx.paths.apps_dir().join(&app_id); - let manifest_path = std::fs::read_dir(&app_dir)? - .flatten() - .map(|e| e.path()) - .find(|p| { - matches!( - p.extension().and_then(|e| e.to_str()), - Some("flo") | Some("app") - ) - }) - .ok_or_else(|| { - AwareError::Internal(format!("installed app {app_id} missing .flo/.app file")) - })?; + std::fs::read_dir(&app_dir)?; + let manifest_path = crate::manifest::loader::find_app_manifest(&app_dir).ok_or_else(|| { + AwareError::Internal(format!("installed app {app_id} missing .flo/.app file")) + })?; let app = crate::manifest::loader::load_app(&manifest_path)?; 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/install/local.rs b/cli/src/install/local.rs index e251e40cf..32b9da0c9 100644 --- a/cli/src/install/local.rs +++ b/cli/src/install/local.rs @@ -5,7 +5,7 @@ use std::path::Path; use crate::error::AwareError; use crate::manifest::App; -use crate::manifest::loader::{load_agent, load_app}; +use crate::manifest::loader::{find_app_manifest, load_agent, load_app}; use crate::paths::Paths; use crate::validate::{error_summary, validate_agent_on_disk, validate_app}; @@ -65,18 +65,13 @@ pub fn install_agent_from_path( /// Install an app folder. `src` must contain a `.flo` or `.app` file. pub fn install_app_from_path(src: &Path, paths: &Paths) -> Result { - let manifest_path = std::fs::read_dir(src)? - .flatten() - .map(|e| e.path()) - .find(|p| { - matches!( - p.extension().and_then(|e| e.to_str()), - Some("flo") | Some("app") - ) - }) - .ok_or_else(|| { - AwareError::Validation(format!("no .flo or .app file in {}", src.display())) - })?; + // Probe first so a genuine enumeration failure (permissions, IO) propagates as + // itself; `find_app_manifest` flattens its `read_dir` error to `None`, which + // would otherwise be reported as "no .flo or .app file". + std::fs::read_dir(src)?; + let manifest_path = find_app_manifest(src).ok_or_else(|| { + AwareError::Validation(format!("no .flo or .app file in {}", src.display())) + })?; let app = load_app(&manifest_path)?; let issues = validate_app(&app); @@ -258,6 +253,45 @@ mod tests { ); } + /// Install selects the manifest by the same rule as `app validate` — the + /// canonical `.flo` first. + /// + /// It used to scan `read_dir` inline and take whatever the filesystem + /// yielded first, so a directory holding two sources could be validated as + /// one app and installed as the other, from a single `aware app install`. + /// + /// Note what this test can and cannot do: `read_dir` order is not specified + /// (it is hash order on ext4, not alphabetical), so against the old inline + /// scan this would have failed only on the runs where the decoy happened to + /// come out first. That is the point — the answer was never pinned to + /// anything. It is pinned now, and this goes red on any future copy that + /// unpins it in the majority of orderings. + #[test] + fn install_takes_the_canonical_manifest_not_whatever_read_dir_yields() { + let tmp = tempfile::tempdir().unwrap(); + let paths = Paths { + aware_home: tmp.path().to_path_buf(), + }; + let app_src = tmp.path().join("src/zeta"); + std::fs::create_dir_all(&app_src).unwrap(); + let body = |id: &str| { + format!( + "app: {id}\nversion: 0.1.0\ndescription: a selection fixture\n\ + nodes:\n - id: gate\n inline:\n kind: predicate\n\ + \x20 description: always pass\n code: 'true'\nrequires: []\n" + ) + }; + std::fs::write(app_src.join("alpha.flo"), body("alpha")).unwrap(); + std::fs::write(app_src.join("zeta.flo"), body("zeta")).unwrap(); + + assert_eq!(install_app_from_path(&app_src, &paths).unwrap(), "zeta"); + assert!(tmp.path().join("apps/zeta/zeta.flo").is_file()); + assert!( + !tmp.path().join("apps/alpha").exists(), + "the decoy beside it must not be what got installed" + ); + } + #[test] fn installing_exposes_as_agent_app_registers_a_synth_agent() { let tmp = tempfile::tempdir().unwrap(); 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!( From b3b11eb7db4374135784bff5fb00adcf2ab73d80 Mon Sep 17 00:00:00 2001 From: Pawel Lisowski Date: Mon, 7 Sep 2026 04:01:34 +0000 Subject: [PATCH 2/4] fix(app): keep dot paths installable and lock the manifest actually installed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both P1 findings from Codex's review of cc7516ea. Collapsing install's three inline manifest scans onto `find_app_manifest` inherited two defects from the helper's basename preference, one of them pre-existing. 1. `aware app install .` reported "no .flo or .app file". `find_app_manifest` opened with `root.file_name()?`, which is `None` for `.` and `..`, so it returned before reading the directory at all. The inline scans it replaced had no such preference and worked. The directory name now comes from the path resolved against the working directory, and a path with no basename falls through to the scan instead of short-circuiting. This also fixes `aware app validate .`, which had the same defect on main — `resolve_validate_target` was moved onto this helper earlier. 2. Install could lock and discover a manifest it never installed. Install copies the source folder to `apps/`, which renames the directory, so re-running a `.flo`-preferring selector afterwards asks a different question: a folder `bundle/` holding `bundle.flo` (`app: alpha`) beside an `alpha.flo` was validated and copied as `bundle.flo`, then re-read as `alpha.flo`. The installed manifest is now the one already selected, under its own name — there is no second lookup. The `read_dir`-order fallback is also sorted now. It was unpinned, which is what made the second defect reachable at all, and with no basename to prefer (the `..` case above) it is the only rule left deciding what gets picked up. Both fixes carry integration tests that fail against the code before them: `app_install_accepts_a_dot_path` and `app_install_locks_the_manifest_it_actually_installed`, verified red on the parent commit and green here. Gates from `cli/`: cargo fmt --all --check, cargo clippy --all-targets -D warnings, cargo test — all pass (1232 unit + every integration suite). --- cli/src/commands/app.rs | 14 ++++-- cli/src/manifest/loader.rs | 89 ++++++++++++++++++++++++++++++++------ cli/tests/app_install.rs | 80 ++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+), 16 deletions(-) diff --git a/cli/src/commands/app.rs b/cli/src/commands/app.rs index 91553dda6..dc9761f71 100644 --- a/cli/src/commands/app.rs +++ b/cli/src/commands/app.rs @@ -1222,12 +1222,20 @@ fn install(ctx: &Context, spec: &str) -> Result<(), AwareError> { let app_id = crate::install::install_app_from_path(&path, &ctx.paths)?; - // Locate the installed .flo / .app file + // The installed manifest is the one selected above, under its own name — + // NOT whatever re-running the selector on the installed directory returns. + // `install_app_from_path` copies the folder to `apps/`, so the + // directory's basename changes, and the selector prefers `.flo`: + // a folder `bundle/` holding `bundle.flo` (`app: alpha`) beside an + // `alpha.flo` is validated and copied as `bundle.flo`, then re-read here as + // `alpha.flo` — locking and discovering an app nobody installed (Codex, + // #499). The rename is precisely what makes a second lookup a different + // question, so there is no second lookup. let app_dir = ctx.paths.apps_dir().join(&app_id); - std::fs::read_dir(&app_dir)?; - let manifest_path = crate::manifest::loader::find_app_manifest(&app_dir).ok_or_else(|| { + let manifest_name = src_manifest.file_name().ok_or_else(|| { AwareError::Internal(format!("installed app {app_id} missing .flo/.app file")) })?; + let manifest_path = app_dir.join(manifest_name); let app = crate::manifest::loader::load_app(&manifest_path)?; diff --git a/cli/src/manifest/loader.rs b/cli/src/manifest/loader.rs index 8cf116e6c..7950211fc 100644 --- a/cli/src/manifest/loader.rs +++ b/cli/src/manifest/loader.rs @@ -178,19 +178,44 @@ pub(crate) fn is_safe_segment(id: &str) -> bool { components.next().is_none() && first == id } +/// The directory's own name, for the `.flo` preference below. +/// +/// Not `root.file_name()`: that is `None` for `.` and `..`, and `aware app +/// install .` / `aware app validate .` are ordinary invocations. Resolving +/// against the working directory first turns `.` into the real directory name +/// (Codex, #499). `..` stays unresolved even then — `absolute` is lexical and +/// keeps the `..` component — so this still returns `None` sometimes, and the +/// caller must treat that as "no preference", never as "no manifest". +fn app_dir_name(root: &Path) -> Option { + std::path::absolute(root) + .ok()? + .file_name() + .map(|n| n.to_string_lossy().into_owned()) +} + pub(crate) fn find_app_manifest(root: &Path) -> Option { // Preferred: /.flo, then any *.flo, then any *.app. - let dir_name = root.file_name()?.to_string_lossy().to_string(); - let canonical = root.join(format!("{dir_name}.flo")); - if canonical.is_file() { - return Some(canonical); + if let Some(dir_name) = app_dir_name(root) { + let canonical = root.join(format!("{dir_name}.flo")); + if canonical.is_file() { + return Some(canonical); + } } + // Sorted, so the fallback is an ANSWER rather than whatever `read_dir` + // happened to yield — including in the `..` case above, where there is no + // basename to prefer and this is the only rule left. + let mut entries: Vec = std::fs::read_dir(root) + .ok()? + .flatten() + .map(|e| e.path()) + .collect(); + entries.sort(); for ext in ["flo", "app"] { - for entry in std::fs::read_dir(root).ok()?.flatten() { - let p = entry.path(); - if p.extension().is_some_and(|e| e == ext) { - return Some(p); - } + if let Some(p) = entries + .iter() + .find(|p| p.extension().is_some_and(|e| e == ext)) + { + return Some(p.clone()); } } None @@ -472,10 +497,8 @@ mod tests { /// `read_dir` order. Names are chosen so an alphabetical tie-break would /// pick the `.app`, and so would swapping the two extensions. /// - /// Not asserted here: which of two `.flo` files wins when neither is named - /// after the directory. `find_app_manifest` returns the first `read_dir` - /// yields, and that order is filesystem-defined, so there is no answer a - /// test could pin without asserting on the filesystem instead of on us. + /// Which of two `.flo` files wins when neither is named after the directory + /// is pinned separately, below. #[test] fn app_source_lookup_prefers_flo_over_app() { let tmp = tempfile::tempdir().unwrap(); @@ -499,6 +522,46 @@ mod tests { assert_eq!(found.file_name().unwrap(), "legacy.app"); } + /// With no `.flo` to prefer, the winner is the first by sorted + /// name — an answer, rather than whatever `read_dir` happened to yield. + /// + /// The order used to be filesystem-defined, so this had no answer to pin. + /// It matters most where there is no basename to prefer at all: `..` stays + /// unresolved by `std::path::absolute`, and this rule is then the only one + /// left deciding what `aware app install ..` picks up. + #[test] + fn app_source_lookup_breaks_a_tie_by_sorted_name() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("demo"); + write_app(&root.join("zzz.flo"), "demo"); + write_app(&root.join("aaa.flo"), "demo"); + + let found = find_app_manifest(&root).unwrap(); + assert_eq!(found.file_name().unwrap(), "aaa.flo"); + } + + /// A path with no basename of its own — `.` — resolves to the directory it + /// names, so the `.flo` preference still applies. Before this, + /// `Path::file_name` returned `None` and the whole lookup short-circuited + /// to "no manifest" without reading the directory (Codex, #499). + /// + /// Asserted through a trailing-`.` join rather than by changing the process + /// working directory, which is global and would race the other tests. + #[test] + fn app_source_lookup_survives_a_path_with_no_basename() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("demo"); + write_app(&root.join("demo.flo"), "demo"); + write_app(&root.join("aaa.flo"), "other"); + + let found = find_app_manifest(&root.join(".")).unwrap(); + assert_eq!( + found.file_name().unwrap(), + "demo.flo", + "the directory's own name still wins" + ); + } + /// A directory with no source, and a directory that does not exist at all, /// are both `None` rather than a panic: `find_app_manifest` flattens its /// `read_dir` error, and several callers reach it with a path they have not diff --git a/cli/tests/app_install.rs b/cli/tests/app_install.rs index b3f11c3a8..7335fb3b1 100644 --- a/cli/tests/app_install.rs +++ b/cli/tests/app_install.rs @@ -73,3 +73,83 @@ fn app_install_rejects_invalid_path() { .assert() .failure(); } + +/// A self-contained app source with no `requires:`, so these tests exercise +/// manifest SELECTION without dragging agent installs in. +fn write_standalone_app(path: &std::path::Path, id: &str) { + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write( + path, + format!( + "app: {id}\nversion: 0.1.0\ndescription: a selection fixture\n\ + nodes:\n - id: gate\n inline:\n kind: predicate\n\ + \x20 description: always pass\n code: 'true'\nrequires: []\n" + ), + ) + .unwrap(); +} + +/// `aware app install .` installs the directory you are standing in. +/// +/// The manifest selector prefers `.flo`, and it used to take that +/// name from `Path::file_name`, which is `None` for `.` and `..` — so the +/// selector returned "no manifest" without ever reading the directory, and a +/// perfectly ordinary invocation failed (Codex, #499). The name now comes from +/// the path resolved against the working directory. +#[test] +fn app_install_accepts_a_dot_path() { + let tmp = tempfile::tempdir().unwrap(); + let aware = tmp.path().join("aware"); + let app_src = tmp.path().join("dotted"); + write_standalone_app(&app_src.join("dotted.flo"), "dotted"); + + Command::cargo_bin("aware") + .unwrap() + .env("AWARE_HOME", &aware) + .current_dir(&app_src) + .args(["app", "install", "."]) + .assert() + .success(); + + assert!(aware.join("apps/dotted/dotted.flo").is_file()); +} + +/// The manifest that gets validated and copied is the one whose lockfile is +/// written and whose id the app is discovered under — all four the same file. +/// +/// Install copies the source folder to `apps/`, which RENAMES the +/// directory, and the selector prefers `.flo`. Re-running the +/// selector afterwards therefore asks a different question: here `bundle/` +/// holds `bundle.flo` (declaring `app: alpha`) beside an `alpha.flo`, so the +/// pre-flight picks `bundle.flo` and a second lookup under `apps/alpha/` would +/// pick `alpha.flo` — locking an app nobody installed (Codex, #499). +#[test] +fn app_install_locks_the_manifest_it_actually_installed() { + let tmp = tempfile::tempdir().unwrap(); + let aware = tmp.path().join("aware"); + let app_src = tmp.path().join("bundle"); + write_standalone_app(&app_src.join("bundle.flo"), "alpha"); + write_standalone_app(&app_src.join("alpha.flo"), "decoy"); + + Command::cargo_bin("aware") + .unwrap() + .env("AWARE_HOME", &aware) + .args(["app", "install"]) + .arg(&app_src) + .assert() + .success() + .stdout(predicate::str::contains("alpha")); + + // Installed under the id `bundle.flo` declared... + let lockfile = aware.join("apps/alpha/lockfile.yaml"); + let body = std::fs::read_to_string(&lockfile).unwrap(); + // ...and the lockfile describes THAT app, not the decoy beside it. + assert!( + body.contains("app: alpha"), + "lockfile must describe the installed manifest, got: {body}" + ); + assert!( + !body.contains("decoy"), + "the decoy manifest must not be what got locked: {body}" + ); +} From 2e3bbdf36d27c0b9fe139503f250c24827b12109 Mon Sep 17 00:00:00 2001 From: Pawel Date: Tue, 8 Sep 2026 17:05:39 +0200 Subject: [PATCH 3/4] refactor(app): split manifest identity work into #502 --- cli/src/commands/app.rs | 45 ++++++++++--------- cli/src/install/local.rs | 60 ++++++------------------- cli/src/manifest/loader.rs | 89 ++++++-------------------------------- cli/tests/app_install.rs | 80 ---------------------------------- 4 files changed, 51 insertions(+), 223 deletions(-) diff --git a/cli/src/commands/app.rs b/cli/src/commands/app.rs index dc9761f71..8fc118f32 100644 --- a/cli/src/commands/app.rs +++ b/cli/src/commands/app.rs @@ -1169,13 +1169,18 @@ fn install(ctx: &Context, spec: &str) -> Result<(), AwareError> { // (app-spec § Safety contract: "aware app validate refuses to install an // app missing `safety:` on a write-mode node"; install must enforce the // same contract as the standalone `validate` command, #134). - // Same selection rule as `install_app_from_path` below and as - // `resolve_validate_target` — otherwise this pre-flight validates one - // manifest and the install that follows writes another. - std::fs::read_dir(&path)?; - let src_manifest = crate::manifest::loader::find_app_manifest(&path).ok_or_else(|| { - AwareError::Validation(format!("no .flo or .app file in {}", path.display())) - })?; + let src_manifest = std::fs::read_dir(&path)? + .flatten() + .map(|e| e.path()) + .find(|p| { + matches!( + p.extension().and_then(|e| e.to_str()), + Some("flo") | Some("app") + ) + }) + .ok_or_else(|| { + AwareError::Validation(format!("no .flo or .app file in {}", path.display())) + })?; let src_app = crate::manifest::loader::load_app(&src_manifest)?; let mut issues = crate::validate::validate_app(&src_app); // Missing agents are reported separately from `issues` so they surface even @@ -1222,20 +1227,20 @@ fn install(ctx: &Context, spec: &str) -> Result<(), AwareError> { let app_id = crate::install::install_app_from_path(&path, &ctx.paths)?; - // The installed manifest is the one selected above, under its own name — - // NOT whatever re-running the selector on the installed directory returns. - // `install_app_from_path` copies the folder to `apps/`, so the - // directory's basename changes, and the selector prefers `.flo`: - // a folder `bundle/` holding `bundle.flo` (`app: alpha`) beside an - // `alpha.flo` is validated and copied as `bundle.flo`, then re-read here as - // `alpha.flo` — locking and discovering an app nobody installed (Codex, - // #499). The rename is precisely what makes a second lookup a different - // question, so there is no second lookup. + // Locate the installed .flo / .app file let app_dir = ctx.paths.apps_dir().join(&app_id); - let manifest_name = src_manifest.file_name().ok_or_else(|| { - AwareError::Internal(format!("installed app {app_id} missing .flo/.app file")) - })?; - let manifest_path = app_dir.join(manifest_name); + let manifest_path = std::fs::read_dir(&app_dir)? + .flatten() + .map(|e| e.path()) + .find(|p| { + matches!( + p.extension().and_then(|e| e.to_str()), + Some("flo") | Some("app") + ) + }) + .ok_or_else(|| { + AwareError::Internal(format!("installed app {app_id} missing .flo/.app file")) + })?; let app = crate::manifest::loader::load_app(&manifest_path)?; diff --git a/cli/src/install/local.rs b/cli/src/install/local.rs index 32b9da0c9..e251e40cf 100644 --- a/cli/src/install/local.rs +++ b/cli/src/install/local.rs @@ -5,7 +5,7 @@ use std::path::Path; use crate::error::AwareError; use crate::manifest::App; -use crate::manifest::loader::{find_app_manifest, load_agent, load_app}; +use crate::manifest::loader::{load_agent, load_app}; use crate::paths::Paths; use crate::validate::{error_summary, validate_agent_on_disk, validate_app}; @@ -65,13 +65,18 @@ pub fn install_agent_from_path( /// Install an app folder. `src` must contain a `.flo` or `.app` file. pub fn install_app_from_path(src: &Path, paths: &Paths) -> Result { - // Probe first so a genuine enumeration failure (permissions, IO) propagates as - // itself; `find_app_manifest` flattens its `read_dir` error to `None`, which - // would otherwise be reported as "no .flo or .app file". - std::fs::read_dir(src)?; - let manifest_path = find_app_manifest(src).ok_or_else(|| { - AwareError::Validation(format!("no .flo or .app file in {}", src.display())) - })?; + let manifest_path = std::fs::read_dir(src)? + .flatten() + .map(|e| e.path()) + .find(|p| { + matches!( + p.extension().and_then(|e| e.to_str()), + Some("flo") | Some("app") + ) + }) + .ok_or_else(|| { + AwareError::Validation(format!("no .flo or .app file in {}", src.display())) + })?; let app = load_app(&manifest_path)?; let issues = validate_app(&app); @@ -253,45 +258,6 @@ mod tests { ); } - /// Install selects the manifest by the same rule as `app validate` — the - /// canonical `.flo` first. - /// - /// It used to scan `read_dir` inline and take whatever the filesystem - /// yielded first, so a directory holding two sources could be validated as - /// one app and installed as the other, from a single `aware app install`. - /// - /// Note what this test can and cannot do: `read_dir` order is not specified - /// (it is hash order on ext4, not alphabetical), so against the old inline - /// scan this would have failed only on the runs where the decoy happened to - /// come out first. That is the point — the answer was never pinned to - /// anything. It is pinned now, and this goes red on any future copy that - /// unpins it in the majority of orderings. - #[test] - fn install_takes_the_canonical_manifest_not_whatever_read_dir_yields() { - let tmp = tempfile::tempdir().unwrap(); - let paths = Paths { - aware_home: tmp.path().to_path_buf(), - }; - let app_src = tmp.path().join("src/zeta"); - std::fs::create_dir_all(&app_src).unwrap(); - let body = |id: &str| { - format!( - "app: {id}\nversion: 0.1.0\ndescription: a selection fixture\n\ - nodes:\n - id: gate\n inline:\n kind: predicate\n\ - \x20 description: always pass\n code: 'true'\nrequires: []\n" - ) - }; - std::fs::write(app_src.join("alpha.flo"), body("alpha")).unwrap(); - std::fs::write(app_src.join("zeta.flo"), body("zeta")).unwrap(); - - assert_eq!(install_app_from_path(&app_src, &paths).unwrap(), "zeta"); - assert!(tmp.path().join("apps/zeta/zeta.flo").is_file()); - assert!( - !tmp.path().join("apps/alpha").exists(), - "the decoy beside it must not be what got installed" - ); - } - #[test] fn installing_exposes_as_agent_app_registers_a_synth_agent() { let tmp = tempfile::tempdir().unwrap(); diff --git a/cli/src/manifest/loader.rs b/cli/src/manifest/loader.rs index 7950211fc..8cf116e6c 100644 --- a/cli/src/manifest/loader.rs +++ b/cli/src/manifest/loader.rs @@ -178,44 +178,19 @@ pub(crate) fn is_safe_segment(id: &str) -> bool { components.next().is_none() && first == id } -/// The directory's own name, for the `.flo` preference below. -/// -/// Not `root.file_name()`: that is `None` for `.` and `..`, and `aware app -/// install .` / `aware app validate .` are ordinary invocations. Resolving -/// against the working directory first turns `.` into the real directory name -/// (Codex, #499). `..` stays unresolved even then — `absolute` is lexical and -/// keeps the `..` component — so this still returns `None` sometimes, and the -/// caller must treat that as "no preference", never as "no manifest". -fn app_dir_name(root: &Path) -> Option { - std::path::absolute(root) - .ok()? - .file_name() - .map(|n| n.to_string_lossy().into_owned()) -} - pub(crate) fn find_app_manifest(root: &Path) -> Option { // Preferred: /.flo, then any *.flo, then any *.app. - if let Some(dir_name) = app_dir_name(root) { - let canonical = root.join(format!("{dir_name}.flo")); - if canonical.is_file() { - return Some(canonical); - } + let dir_name = root.file_name()?.to_string_lossy().to_string(); + let canonical = root.join(format!("{dir_name}.flo")); + if canonical.is_file() { + return Some(canonical); } - // Sorted, so the fallback is an ANSWER rather than whatever `read_dir` - // happened to yield — including in the `..` case above, where there is no - // basename to prefer and this is the only rule left. - let mut entries: Vec = std::fs::read_dir(root) - .ok()? - .flatten() - .map(|e| e.path()) - .collect(); - entries.sort(); for ext in ["flo", "app"] { - if let Some(p) = entries - .iter() - .find(|p| p.extension().is_some_and(|e| e == ext)) - { - return Some(p.clone()); + for entry in std::fs::read_dir(root).ok()?.flatten() { + let p = entry.path(); + if p.extension().is_some_and(|e| e == ext) { + return Some(p); + } } } None @@ -497,8 +472,10 @@ mod tests { /// `read_dir` order. Names are chosen so an alphabetical tie-break would /// pick the `.app`, and so would swapping the two extensions. /// - /// Which of two `.flo` files wins when neither is named after the directory - /// is pinned separately, below. + /// Not asserted here: which of two `.flo` files wins when neither is named + /// after the directory. `find_app_manifest` returns the first `read_dir` + /// yields, and that order is filesystem-defined, so there is no answer a + /// test could pin without asserting on the filesystem instead of on us. #[test] fn app_source_lookup_prefers_flo_over_app() { let tmp = tempfile::tempdir().unwrap(); @@ -522,46 +499,6 @@ mod tests { assert_eq!(found.file_name().unwrap(), "legacy.app"); } - /// With no `.flo` to prefer, the winner is the first by sorted - /// name — an answer, rather than whatever `read_dir` happened to yield. - /// - /// The order used to be filesystem-defined, so this had no answer to pin. - /// It matters most where there is no basename to prefer at all: `..` stays - /// unresolved by `std::path::absolute`, and this rule is then the only one - /// left deciding what `aware app install ..` picks up. - #[test] - fn app_source_lookup_breaks_a_tie_by_sorted_name() { - let tmp = tempfile::tempdir().unwrap(); - let root = tmp.path().join("demo"); - write_app(&root.join("zzz.flo"), "demo"); - write_app(&root.join("aaa.flo"), "demo"); - - let found = find_app_manifest(&root).unwrap(); - assert_eq!(found.file_name().unwrap(), "aaa.flo"); - } - - /// A path with no basename of its own — `.` — resolves to the directory it - /// names, so the `.flo` preference still applies. Before this, - /// `Path::file_name` returned `None` and the whole lookup short-circuited - /// to "no manifest" without reading the directory (Codex, #499). - /// - /// Asserted through a trailing-`.` join rather than by changing the process - /// working directory, which is global and would race the other tests. - #[test] - fn app_source_lookup_survives_a_path_with_no_basename() { - let tmp = tempfile::tempdir().unwrap(); - let root = tmp.path().join("demo"); - write_app(&root.join("demo.flo"), "demo"); - write_app(&root.join("aaa.flo"), "other"); - - let found = find_app_manifest(&root.join(".")).unwrap(); - assert_eq!( - found.file_name().unwrap(), - "demo.flo", - "the directory's own name still wins" - ); - } - /// A directory with no source, and a directory that does not exist at all, /// are both `None` rather than a panic: `find_app_manifest` flattens its /// `read_dir` error, and several callers reach it with a path they have not diff --git a/cli/tests/app_install.rs b/cli/tests/app_install.rs index 7335fb3b1..b3f11c3a8 100644 --- a/cli/tests/app_install.rs +++ b/cli/tests/app_install.rs @@ -73,83 +73,3 @@ fn app_install_rejects_invalid_path() { .assert() .failure(); } - -/// A self-contained app source with no `requires:`, so these tests exercise -/// manifest SELECTION without dragging agent installs in. -fn write_standalone_app(path: &std::path::Path, id: &str) { - std::fs::create_dir_all(path.parent().unwrap()).unwrap(); - std::fs::write( - path, - format!( - "app: {id}\nversion: 0.1.0\ndescription: a selection fixture\n\ - nodes:\n - id: gate\n inline:\n kind: predicate\n\ - \x20 description: always pass\n code: 'true'\nrequires: []\n" - ), - ) - .unwrap(); -} - -/// `aware app install .` installs the directory you are standing in. -/// -/// The manifest selector prefers `.flo`, and it used to take that -/// name from `Path::file_name`, which is `None` for `.` and `..` — so the -/// selector returned "no manifest" without ever reading the directory, and a -/// perfectly ordinary invocation failed (Codex, #499). The name now comes from -/// the path resolved against the working directory. -#[test] -fn app_install_accepts_a_dot_path() { - let tmp = tempfile::tempdir().unwrap(); - let aware = tmp.path().join("aware"); - let app_src = tmp.path().join("dotted"); - write_standalone_app(&app_src.join("dotted.flo"), "dotted"); - - Command::cargo_bin("aware") - .unwrap() - .env("AWARE_HOME", &aware) - .current_dir(&app_src) - .args(["app", "install", "."]) - .assert() - .success(); - - assert!(aware.join("apps/dotted/dotted.flo").is_file()); -} - -/// The manifest that gets validated and copied is the one whose lockfile is -/// written and whose id the app is discovered under — all four the same file. -/// -/// Install copies the source folder to `apps/`, which RENAMES the -/// directory, and the selector prefers `.flo`. Re-running the -/// selector afterwards therefore asks a different question: here `bundle/` -/// holds `bundle.flo` (declaring `app: alpha`) beside an `alpha.flo`, so the -/// pre-flight picks `bundle.flo` and a second lookup under `apps/alpha/` would -/// pick `alpha.flo` — locking an app nobody installed (Codex, #499). -#[test] -fn app_install_locks_the_manifest_it_actually_installed() { - let tmp = tempfile::tempdir().unwrap(); - let aware = tmp.path().join("aware"); - let app_src = tmp.path().join("bundle"); - write_standalone_app(&app_src.join("bundle.flo"), "alpha"); - write_standalone_app(&app_src.join("alpha.flo"), "decoy"); - - Command::cargo_bin("aware") - .unwrap() - .env("AWARE_HOME", &aware) - .args(["app", "install"]) - .arg(&app_src) - .assert() - .success() - .stdout(predicate::str::contains("alpha")); - - // Installed under the id `bundle.flo` declared... - let lockfile = aware.join("apps/alpha/lockfile.yaml"); - let body = std::fs::read_to_string(&lockfile).unwrap(); - // ...and the lockfile describes THAT app, not the decoy beside it. - assert!( - body.contains("app: alpha"), - "lockfile must describe the installed manifest, got: {body}" - ); - assert!( - !body.contains("decoy"), - "the decoy manifest must not be what got locked: {body}" - ); -} From 8664245bed134ba33f9f8f6e728db0aa2de3df2d Mon Sep 17 00:00:00 2001 From: Pawel Date: Tue, 8 Sep 2026 17:09:48 +0200 Subject: [PATCH 4/4] test(runtime): cover shared artifact and PATH behavior --- cli/tests/agent_invoke.rs | 3 ++- cli/tests/sidecar_cli.rs | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) 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() }