From 0ecf65032fbced204e20c6f6286d160aa99e74c0 Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Thu, 13 Aug 2026 23:36:21 -0400 Subject: [PATCH 01/18] fix(tests): let the suite build and compare correctly on Windows Two platform assumptions kept `cargo test` from running at all on Windows. `tests/run/conversation.rs` imported `std::os::unix::fs::PermissionsExt` unconditionally. Because the file is part of the `run` test target, the resulting compile error aborted `cargo test` and `cargo clippy --all-targets` before any test executed. The scripted-turn test that needs it drives a `#!/bin/sh` stub it has to mark executable, so it and its imports are now gated `#[cfg(unix)]`; the other two tests in the file run everywhere. `.gitattributes` pinned only `tests/golden/**` against EOL translation, so a Windows checkout with the default `core.autocrlf=true` produced a CRLF working tree everywhere else. Harness descriptors and profiles are embedded verbatim in generated artifacts, and `tests/fixtures/**` is compared byte for byte, so CRLF silently changed program output. Checking every text file out as LF keeps generated bytes identical across platforms. Verified on Windows 11 with rustc 1.97.1: - `cargo build`, `cargo fmt --check`, and `cargo clippy --all-targets -- -D warnings` all pass; clippy previously could not compile. - `cargo test --no-fail-fast` reaches 1009 passing, up from 886 with the 123-test `run` target unable to build. The remaining failures are path-separator and POSIX-spawning assumptions, tracked separately. Co-Authored-By: Claude Opus 5 --- .gitattributes | 6 ++++++ tests/run/conversation.rs | 9 +++++++++ 2 files changed, 15 insertions(+) diff --git a/.gitattributes b/.gitattributes index 974ce5f..60c3b54 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,2 +1,8 @@ +# Check every text file out with LF on all platforms. Generated artifacts embed the bytes of +# descriptors, profiles, and docs verbatim, and the byte-exact tests compare against LF fixtures, so +# a CRLF working tree on Windows silently changes program output. +* text=auto eol=lf + # Golden fixtures are byte-exact — never EOL-normalize them. tests/golden/** -text +tests/fixtures/** -text diff --git a/tests/run/conversation.rs b/tests/run/conversation.rs index d542180..4fca100 100644 --- a/tests/run/conversation.rs +++ b/tests/run/conversation.rs @@ -3,9 +3,14 @@ use crate::helpers::*; use predicates::prelude::PredicateBooleanExt; use predicates::str::contains; +// Every `#[cfg(unix)]` import below exists solely for +// `dispatch_task_runs_all_scripted_turns_in_one_native_session` — see the note on that test. +#[cfg(unix)] use serde_json::Value; use std::fs; +#[cfg(unix)] use std::os::unix::fs::PermissionsExt; +#[cfg(unix)] use std::path::Path; #[test] @@ -64,6 +69,10 @@ fn multi_turn_eval_dispatch_records_followups_and_conversation_artifact_path() { } } +// The harness stub below is a `#!/bin/sh` script invoked directly through the descriptor's +// exec_template, which needs both a POSIX shell and an executable bit. Windows has neither, so the +// scripted-turn path is covered on Unix only until a portable stub replaces it. +#[cfg(unix)] #[test] fn dispatch_task_runs_all_scripted_turns_in_one_native_session() { let tmp = tempfile::TempDir::new().unwrap(); From 7ad2c6469c62aa99e3f52d7c898a57e0a5a35406 Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Fri, 14 Aug 2026 02:07:45 -0400 Subject: [PATCH 02/18] feat(core): add artifact path rendering helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Path::join` plus `Display` emits the host's separator, so on Windows a POSIX-rooted base yields `/work/cond\run.json` — malformed for every reader of an artifact that is, by design, a wire format shared across platforms. `artifact_path` renders a path into that wire format: forward slashes, with a verbatim `\?\` prefix stripped (verbatim UNC collapsed back to `\server\share` rather than left as a bare `UNC\` component). The rewrite is Windows-only, because a POSIX filename may legally contain a literal backslash and rewriting it there would name a different file — which also keeps Unix output, and so every golden fixture, byte-identical. `normalize_separators` is the comparison-side counterpart and is unconditional: its job is matching a path spelled by a *different* host, not preserving the local spelling. No call sites yet; those land with the boundaries they fix. Refs #246 Co-Authored-By: Claude Opus 5 --- src/core/fs.rs | 113 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 111 insertions(+), 2 deletions(-) diff --git a/src/core/fs.rs b/src/core/fs.rs index 7f55329..b70bcd8 100644 --- a/src/core/fs.rs +++ b/src/core/fs.rs @@ -1,5 +1,10 @@ -//! Shared filesystem + JSON helpers — the single home for artifact writing and -//! tree copying, used by `pipeline`, `workspace`, `cli::run`, and `sandbox`. +//! Shared filesystem + JSON helpers — the single home for artifact writing, +//! artifact path rendering, and tree copying, used by `pipeline`, `workspace`, +//! `cli::run`, `adapters`, and `sandbox`. +//! +//! [`artifact_path`] renders a path into the forward-slash wire format every +//! generated artifact carries; [`normalize_separators`] is its comparison-side +//! counterpart, for matching a path spelled by a different host. //! //! Copying comes in two flavors. Pick by what the destination is *for*: //! @@ -22,6 +27,49 @@ use std::path::Path; use serde::Serialize; +/// Render `path` as the forward-slash string an artifact, manifest, or prompt +/// carries. +/// +/// Artifact path fields are a wire format: agents read them, downstream tools +/// join them, and the golden fixtures compare them byte for byte. `Path::join` +/// plus `Display` emits the *host's* separator, so on Windows a POSIX-rooted +/// base yields `/work/cond\run.json` — malformed for every reader. Forward +/// slashes are accepted by the Windows file APIs, so the result stays openable +/// by the stages that read these fields back. +/// +/// The rewrite is Windows-only: a POSIX filename may legally contain a literal +/// backslash, and rewriting it there would name a different file. A verbatim +/// (`\\?\`) prefix — what `Path::canonicalize` returns on Windows — is stripped +/// first, since it is an OS escape hatch rather than a path to hand an agent. +/// +/// Not for paths handed to a process: a spawned command's argv and the guard +/// hook command line must keep the host's own spelling. +pub fn artifact_path(path: &Path) -> String { + let rendered = path.to_string_lossy(); + if !cfg!(windows) { + return rendered.into_owned(); + } + let unprefixed = match rendered.strip_prefix(r"\\?\UNC\") { + // Verbatim UNC collapses back to the `\\server\share` form; dropping + // the whole prefix would leave a bare `UNC\` component. + Some(rest) => format!(r"\\{rest}"), + None => rendered + .strip_prefix(r"\\?\") + .unwrap_or(&rendered) + .to_string(), + }; + normalize_separators(&unprefixed) +} + +/// Rewrite Windows separators to forward slashes for *comparison*. +/// +/// Unconditional, unlike [`artifact_path`]: this exists to match a foreign +/// spelling — a path a Windows agent recorded, read back on any host — rather +/// than to preserve the local one. +pub fn normalize_separators(value: &str) -> String { + value.replace('\\', "/") +} + /// Write `value` to `path` as pretty JSON with a two-space indent and a /// trailing newline — the stable on-disk format for every artifact this binary /// writes. `serde_json`'s `preserve_order` feature keeps object key order @@ -107,6 +155,67 @@ mod tests { use serde_json::json; use tempfile::TempDir; + /// A path already in wire form passes through unchanged on every host — + /// the fixtures and goldens that spell paths POSIX-style stay byte-stable. + #[test] + fn artifact_path_leaves_a_forward_slash_path_alone() { + assert_eq!( + artifact_path(Path::new("/work/cond/run.json")), + "/work/cond/run.json" + ); + } + + /// The bug this exists for: `Path::join` on a POSIX-rooted base emits a + /// Windows separator, so a manifest entry reads `/work/cond\run.json`. The + /// verbatim `\\?\` prefix `canonicalize` returns is stripped too — it is an + /// OS-level escape hatch, not something an agent should ever be handed. + #[cfg(windows)] + #[test] + fn artifact_path_rewrites_windows_separators_and_strips_verbatim_prefixes() { + assert_eq!( + artifact_path(Path::new(r"/work/cond\run.json")), + "/work/cond/run.json" + ); + assert_eq!( + artifact_path(Path::new(r"C:\work\cond\run.json")), + "C:/work/cond/run.json" + ); + assert_eq!( + artifact_path(Path::new(r"\\?\C:\work\run.json")), + "C:/work/run.json" + ); + assert_eq!( + artifact_path(Path::new(r"\\?\UNC\host\share\run.json")), + "//host/share/run.json" + ); + } + + /// The rewrite is Windows-only: a POSIX filename may legally contain a + /// literal backslash, and rewriting it would name a different file. + #[cfg(unix)] + #[test] + fn artifact_path_preserves_a_literal_backslash_in_a_posix_filename() { + assert_eq!( + artifact_path(Path::new(r"/work/od\dity.json")), + r"/work/od\dity.json" + ); + } + + /// Comparison normalization is unconditional, unlike [`artifact_path`]: its + /// job is matching a *foreign* spelling — a Windows-recorded transcript read + /// on any host — rather than preserving the local one. + #[test] + fn normalize_separators_rewrites_backslashes_on_every_host() { + assert_eq!( + normalize_separators(r"C:\work\cond\run.json"), + "C:/work/cond/run.json" + ); + assert_eq!( + normalize_separators("/work/cond/run.json"), + "/work/cond/run.json" + ); + } + /// The on-disk format is a contract: artifacts are diffed across runs and /// read by agents, so indent and the trailing newline are pinned. #[test] From cdda80f5518b2c0b26976ad67ccfbb9eb4a04ac9 Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Fri, 14 Aug 2026 02:13:23 -0400 Subject: [PATCH 03/18] fix(sandbox): keep POSIX-rooted paths rooted when classifying MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard classifies paths that come out of agent tool calls, and agents spell them POSIX-style on every host. Windows has no root without a drive, so `std::path::absolute` was grafting the process's current one: `/dev/null` became `C:\dev\null`, which stopped reading as a device and started reading as a file the guard had to block — `cmd >/dev/null` denied, and `git fetch origin >/dev/null 2>&1` reclassified from "git remote operation" to "output redirection to a file". `/etc/passwd` still denied, but the evidence recorded a path that never existed. `lexically_absolute` now leaves a rooted-but-prefixless path exactly as given, and `resolve_path` applies it to the *joined* path so a relative target under a POSIX-rooted root resolves the same way its allowed roots do. The stray-write scanner shares the helper for the same reason: resolving one side with a drive and the other without silently stops the comparison matching. `is_non_file_device` now matches by path component, since a resolved `/dev/fd/1` renders as `fd\1` and a `"fd/"` string prefix would miss it. Lexical `..` normalization still runs first, so `/dev/../etc/passwd` cannot launder a write past the device check. Fixture validation gets the matching treatment: `Path::is_absolute` answers for the host only, so `/etc/passwd` slipped past it on Windows and `Path::join`'s root-replacing behavior would have landed the fixture outside the env. Eval configs are committed and run on every platform, so `is_absolute_on_any_platform` gives one verdict everywhere — `\etc\passwd` included, which is absolute on Windows and a legal filename elsewhere. Windows lib failures: 45 -> 36. Refs #246 Co-Authored-By: Claude Opus 5 --- src/cli/run/fixtures.rs | 36 +++++++++++++++-- src/pipeline/detect_stray_writes.rs | 11 ++--- src/sandbox/mod.rs | 1 + src/sandbox/policy.rs | 62 ++++++++++++++++++++++++++++- src/sandbox/shell_targets.rs | 17 +++++--- 5 files changed, 108 insertions(+), 19 deletions(-) diff --git a/src/cli/run/fixtures.rs b/src/cli/run/fixtures.rs index df15b01..74928e4 100644 --- a/src/cli/run/fixtures.rs +++ b/src/cli/run/fixtures.rs @@ -38,11 +38,32 @@ fn claim_fixture_dest( Ok(false) } +/// True for a path that is absolute under *either* platform's rules. +/// +/// `Path::is_absolute` answers for the host only: Windows has no root without a +/// drive, so `/etc/passwd` reads as relative there and slips past a bare +/// `is_absolute` check — then `Path::join`'s root-replacing behavior lands it +/// outside the env entirely. Eval configs are committed and run on every +/// platform, so the verdict must not depend on which host reads them. +/// +/// A drive-relative Windows path (`C:fixture.txt`) is only detectable where the +/// parser produces a `Prefix` component, so it is caught on Windows and treated +/// as an ordinary relative name elsewhere. +fn is_absolute_on_any_platform(raw: &str) -> bool { + let path = Path::new(raw); + path.has_root() + || raw.starts_with('\\') + || matches!( + path.components().next(), + Some(std::path::Component::Prefix(_)) + ) +} + /// Reject a fixture path that is absolute or escapes `env/` via `..`, so a fixture /// always lands inside the isolated env. fn validate_fixture_rel(f: &str) -> Result<(), RunError> { let p = Path::new(f); - let escapes = p.is_absolute() + let escapes = is_absolute_on_any_platform(f) || p.components() .any(|c| matches!(c, std::path::Component::ParentDir)); if escapes { @@ -67,7 +88,7 @@ fn validate_fixture_rel(f: &str) -> Result<(), RunError> { /// so a leading `.git` component is not runner-owned metadata. fn validate_files_root_rel(root: &str) -> Result<(), RunError> { let path = Path::new(root); - let escapes = path.is_absolute() + let escapes = is_absolute_on_any_platform(root) || path .components() .any(|component| matches!(component, std::path::Component::ParentDir)); @@ -335,7 +356,16 @@ mod tests { fs::create_dir_all(skill_dir.join("evals")).unwrap(); let env_root = tmp.path().join("env"); - for bad in ["../escape.txt", "/etc/passwd", "a/../../b.txt"] { + // `\etc\passwd` is absolute on Windows and a legal filename on Unix. + // Eval configs are committed and run on every platform, so the verdict + // must not depend on which host reads them — a fixture that validates + // on Linux and escapes the env on Windows is the worst of both. + for bad in [ + "../escape.txt", + "/etc/passwd", + "a/../../b.txt", + r"\etc\passwd", + ] { let ev = eval_with_files("e1", &[bad]); let mut claims = FixtureClaims::new(); let err = copy_fixtures(&ev, &skill_dir, &env_root, &mut claims).unwrap_err(); diff --git a/src/pipeline/detect_stray_writes.rs b/src/pipeline/detect_stray_writes.rs index c38ec06..7632098 100644 --- a/src/pipeline/detect_stray_writes.rs +++ b/src/pipeline/detect_stray_writes.rs @@ -14,7 +14,7 @@ //! into the separate schema-gated `guard-denials.json` artifact, even when a //! task has no `run.json`. -use std::path::{Path, PathBuf}; +use std::path::Path; use serde::{Deserialize, Serialize}; @@ -26,7 +26,7 @@ use crate::pipeline::guard_denials::collect_guard_denials; use crate::pipeline::io::now_iso8601; use crate::pipeline::slots::{run_key, run_slots}; use crate::sandbox::policy::classify_bash_with_cwd; -use crate::sandbox::{is_shell_tool, is_under, is_write_tool, path_arg}; +use crate::sandbox::{is_shell_tool, is_under, is_write_tool, lexically_absolute, path_arg}; use crate::validation::{SchemaName, validate_against_schema}; /// A read-only tool carrying a target path argument, in any harness's @@ -113,11 +113,6 @@ pub fn detect_stray_writes( findings } -/// Lexically absolutize a path (no disk access). Mirrors node's `resolve()`. -fn absolutize(p: &Path) -> PathBuf { - std::path::absolute(p).unwrap_or_else(|_| p.to_path_buf()) -} - /// Node-style lexical `path.relative(from, to)` over absolute, normalized paths. /// Returns forward-slash-joined components; starts with `..` when `to` is not /// under `from`. @@ -188,7 +183,7 @@ pub fn detect_live_source_reads( repo_root: &Path, ) -> Vec { let mut findings = Vec::new(); - let live_dir = absolutize(live_skill_dir); + let live_dir = lexically_absolute(live_skill_dir); let live_dir_str = live_dir.to_string_lossy(); let rel = path_relative(repo_root, &live_dir); let rel_usable = !rel.starts_with(".."); diff --git a/src/sandbox/mod.rs b/src/sandbox/mod.rs index 5bb9b28..a3d46a3 100644 --- a/src/sandbox/mod.rs +++ b/src/sandbox/mod.rs @@ -31,6 +31,7 @@ pub(crate) use guard::parse_tool_call; pub use guard::read_marker; pub(crate) use install::{GUARD_DENIALS_DIR, GUARD_DENIALS_LOG, guard_is_armed}; pub use install::{GUARD_MANIFEST, GUARD_MARKER, teardown_guard}; +pub(crate) use policy::lexically_absolute; pub use policy::{classify_bash, is_shell_tool, is_under, is_under_any, is_write_tool, path_arg}; use std::time::{SystemTime, UNIX_EPOCH}; diff --git a/src/sandbox/policy.rs b/src/sandbox/policy.rs index 93eef60..b0ba04e 100644 --- a/src/sandbox/policy.rs +++ b/src/sandbox/policy.rs @@ -169,15 +169,39 @@ fn collect_patch_header_paths(text: &str, out: &mut Vec) { } } +/// Lexically absolutize `path`, leaving a rooted-but-prefixless path +/// (`/work/env`) exactly as given. +/// +/// Such a path is absolute on POSIX, but on Windows a root without a drive is +/// incomplete, so `std::path::absolute` grafts the process's current one. The +/// paths on both sides of these comparisons come from *agent* tool calls and +/// eval config, which spell them POSIX-style whatever the host is — grafting +/// `C:` would stop `/dev/null` reading as a device and would put a path that +/// never existed (`C:\etc\passwd`) into the evidence a denial records. +/// +/// Every caller that compares or reports these paths has to apply this same +/// rule — [`resolve_path`] here and the stray-write scanner's live-directory +/// resolution — or one side gains a drive the other lacks and the comparison +/// silently stops matching. +pub(crate) fn lexically_absolute(path: &Path) -> PathBuf { + if path.has_root() && !path.is_absolute() { + return path.to_path_buf(); + } + std::path::absolute(path).unwrap_or_else(|_| path.to_path_buf()) +} + /// Lexically absolutize a path: join onto `repo_root` if relative, then normalize. /// Mirrors node's `resolve()` — no symlink resolution or existence requirement. pub(crate) fn resolve_path(target: &str, repo_root: &Path) -> PathBuf { - let joined = if Path::new(target).is_absolute() { + let path = Path::new(target); + let joined = if path.has_root() { PathBuf::from(target) } else { repo_root.join(target) }; - let absolute = std::path::absolute(&joined).unwrap_or(joined); + // Applied to the *joined* path, so a relative target under a POSIX-rooted + // `repo_root` resolves the same way its allowed roots do. + let absolute = lexically_absolute(&joined); let mut normalized = PathBuf::new(); for component in absolute.components() { match component { @@ -373,6 +397,40 @@ mod tests { ); } + /// The paths the guard classifies come from *agent* tool calls, which spell + /// them POSIX-style whatever the host is. Windows has no root without a + /// drive, so `std::path::absolute` grafts the process's current one — + /// turning `/dev/null` into `C:\dev\null`, which stops reading as a device + /// and starts reading as a file the guard must block. + #[test] + fn resolve_path_keeps_a_posix_rooted_target_rooted() { + let repo = Path::new("/work"); + assert_eq!(resolve_path("/dev/null", repo), PathBuf::from("/dev/null")); + assert_eq!( + resolve_path("/etc/passwd", repo), + PathBuf::from("/etc/passwd") + ); + } + + /// Keeping the target rooted must not cost the lexical normalization that + /// stops `/dev/..` from laundering an out-of-bounds write past the device + /// check. + #[test] + fn resolve_path_normalizes_parent_segments_in_a_posix_rooted_target() { + assert_eq!( + resolve_path("/dev/../etc/passwd", Path::new("/work")), + PathBuf::from("/etc/passwd") + ); + } + + /// The containment verdict is what actually protects the sandbox: a + /// POSIX-rooted target is still outside a drive-rooted allowed root. + #[test] + fn is_under_denies_a_posix_rooted_target_against_a_drive_rooted_root() { + let env = r"C:\work\env"; + assert!(!is_under("/etc/passwd", env, Path::new(env))); + } + #[test] fn is_under_matches_dir_and_descendants() { let repo = Path::new("/work"); diff --git a/src/sandbox/shell_targets.rs b/src/sandbox/shell_targets.rs index 4b2818c..116553b 100644 --- a/src/sandbox/shell_targets.rs +++ b/src/sandbox/shell_targets.rs @@ -307,16 +307,21 @@ fn fd_duplication_end(chars: &[char], at: usize) -> Option { /// to one writes nothing to the filesystem, so it is never a write target. /// Applied to the *resolved* path, so `/dev/../etc/passwd` cannot launder an /// out-of-bounds target through the `/dev` prefix. +/// +/// Matched by path component rather than by string: a resolved path renders +/// with the host's separator, so `/dev/fd/1` reads back as `fd\1` on Windows +/// and a `"fd/"` string prefix would miss it. fn is_non_file_device(resolved: &Path) -> bool { let Ok(rest) = resolved.strip_prefix("/dev") else { return false; }; - match rest.to_str() { - Some("null" | "stdout" | "stderr") => true, - Some(other) => other - .strip_prefix("fd/") - .is_some_and(|n| !n.is_empty() && n.bytes().all(|b| b.is_ascii_digit())), - None => false, + let mut parts = rest.components().map(|c| c.as_os_str().to_str()); + match (parts.next(), parts.next(), parts.next()) { + (Some(Some("null" | "stdout" | "stderr")), None, None) => true, + (Some(Some("fd")), Some(Some(n)), None) => { + !n.is_empty() && n.bytes().all(|b| b.is_ascii_digit()) + } + _ => false, } } From caeba6d9a69af7f4d5c6e45648788341105c00d8 Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Fri, 14 Aug 2026 02:18:06 -0400 Subject: [PATCH 04/18] fix(artifacts): render generated path fields as forward-slash wire format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `dispatch.json`, `judge-tasks.json`, the manifest, the runbook, the guard's deny reasons, and the shadow report are all read by agents and by later pipeline stages, so their path fields are a wire format — but they were built with `Path::join` and `Display`, which emit the host separator. On Windows a POSIX-rooted base produced `/work/cond\run.json`, and a descriptor-declared dir produced genuinely mixed output like `...\home\.agents/skills\different-folder`. Every such field now goes through `artifact_path`. In `build_dispatch_task` the rendering happens once up front, so a task's serialized fields and the prompt text quoting them cannot disagree. Guard deny reasons render their allowed roots the same way as the scratch hint beside them, so one sentence never shows the agent a root in one spelling and a directory under it in another. `canonical_path` loses the verbatim `\?\` prefix `canonicalize` returns on Windows. Deliberately left native: the guard hook command line in `sandbox::install` and harness `exec_template` arguments, which are handed to a process rather than to a reader. No golden fixture changed — `artifact_path` is a no-op on Unix, and on Windows it now produces the bytes the committed fixtures already held. The five golden tests and the whole `sandbox::decide` deny-verdict group go green. Windows lib failures: 36 -> 19. Refs #246 Co-Authored-By: Claude Opus 5 --- src/adapters/skill_shadow.rs | 17 ++++++----- src/cli/run/dispatch.rs | 48 ++++++++++++++++--------------- src/cli/run/runbook.rs | 13 ++------- src/cli/run/scratch.rs | 3 +- src/pipeline/grade/judge_tasks.rs | 20 ++++++------- src/sandbox/decide.rs | 23 +++++++++++---- src/sandbox/git_command.rs | 4 +-- src/sandbox/shell_targets.rs | 4 ++- 8 files changed, 73 insertions(+), 59 deletions(-) diff --git a/src/adapters/skill_shadow.rs b/src/adapters/skill_shadow.rs index 1485fd3..bddc80f 100644 --- a/src/adapters/skill_shadow.rs +++ b/src/adapters/skill_shadow.rs @@ -10,6 +10,8 @@ use std::collections::{BTreeMap, BTreeSet}; use std::path::Path; +use crate::core::fs::artifact_path; + use serde::{Deserialize, Serialize}; mod artifact; @@ -142,7 +144,7 @@ impl ShadowRoot { scope: ShadowRootScope::Project, namespace, plugin: None, - path: skills_dir.to_string_lossy().into_owned(), + path: artifact_path(skills_dir), relation: ShadowRelation::Native, } } @@ -197,7 +199,7 @@ impl ShadowSource { runtime_id: skill_name.clone(), skill_name, plugin: None, - discovery_path: path.to_string_lossy().into_owned(), + discovery_path: artifact_path(path), canonical_path: canonical_path(path), root, appearances: Vec::new(), @@ -220,7 +222,7 @@ impl ShadowSource { skill_name: skill_name.into(), runtime_id: runtime_id.into(), plugin: Some(plugin.into()), - discovery_path: path.to_string_lossy().into_owned(), + discovery_path: artifact_path(path), canonical_path: canonical_path(path), root, appearances: Vec::new(), @@ -241,7 +243,7 @@ impl ShadowSource { skill_name: skill_name.into(), runtime_id: runtime_id.into(), plugin: None, - discovery_path: path.to_string_lossy().into_owned(), + discovery_path: artifact_path(path), canonical_path: canonical_path(path), root, appearances: Vec::new(), @@ -286,10 +288,11 @@ impl ShadowSource { } } +/// The resolved real path, rendered as wire format. `canonicalize` returns a +/// verbatim (`\\?\`) path on Windows, which `artifact_path` strips — an OS +/// escape hatch has no business in a report an agent and a reviewer both read. fn canonical_path(path: &Path) -> Option { - path.canonicalize() - .ok() - .map(|path| path.to_string_lossy().into_owned()) + path.canonicalize().ok().map(|path| artifact_path(&path)) } /// Severity for one finding, given the cells its live sources appear in. diff --git a/src/cli/run/dispatch.rs b/src/cli/run/dispatch.rs index 53c9aa9..eeb1ac5 100644 --- a/src/cli/run/dispatch.rs +++ b/src/cli/run/dispatch.rs @@ -13,6 +13,7 @@ use regex::Regex; use serde::{Deserialize, Serialize}; use crate::adapters::{CliManifestContext, adapter_for}; +use crate::core::fs::artifact_path; use crate::core::{AvailableSkill, Eval, Harness, ScriptedTurn}; use super::RunError; @@ -105,6 +106,14 @@ fn render_plan_mode_context_for_harness(harness: Harness, profile_text: &str) -> /// Construct one dispatch task and its full prompt. pub fn build_dispatch_task(opts: &DispatchTaskOpts) -> Result { let harness = opts.harness; + // Every path this function emits — into `dispatch.json`, the manifest, and + // the prompt the agent reads — is wire format, so render them all the same + // way whatever the host separator is. Rendered once up front so the + // serialized fields and the prompt text can never disagree. + let outputs_dir = artifact_path(Path::new(opts.outputs_dir)); + let eval_root = opts.eval_root.map(|root| artifact_path(Path::new(root))); + let skill_path = opts.skill_path.map(|p| artifact_path(Path::new(p))); + let staged_skill_path = opts.staged_skill_path.map(|p| artifact_path(Path::new(p))); let mut staged_skills = opts.available_skills.clone(); staged_skills.sort_by(|a, b| a.name.cmp(&b.name)); @@ -118,14 +127,14 @@ pub fn build_dispatch_task(opts: &DispatchTaskOpts) -> Result Result = match opts.bootstrap_content { Some(b) if !b.is_empty() => Some(if skill_absent { redact_skill_from_bootstrap(b, opts.skill_name) @@ -214,22 +223,21 @@ pub fn build_dispatch_task(opts: &DispatchTaskOpts) -> Result Result::to_vec), - conversation_path: opts.turns.map(|_| { - cond_dir - .join("conversation.json") - .to_string_lossy() - .into_owned() - }), + conversation_path: opts + .turns + .map(|_| artifact_path(&cond_dir.join("conversation.json"))), agent_description, - dispatch_prompt_path: Path::new(opts.outputs_dir) - .join("dispatch-prompt.txt") - .to_string_lossy() - .into_owned(), + dispatch_prompt_path: artifact_path(&Path::new(&outputs_dir).join("dispatch-prompt.txt")), + outputs_dir, group: opts.group.map(str::to_string), - eval_root: opts.eval_root.map(str::to_string), + eval_root, dispatch_prompt: sections.join(""), }) } diff --git a/src/cli/run/runbook.rs b/src/cli/run/runbook.rs index c603d5c..b8d4c92 100644 --- a/src/cli/run/runbook.rs +++ b/src/cli/run/runbook.rs @@ -14,6 +14,7 @@ use std::collections::BTreeMap; use std::path::Path; use crate::adapters::{CliDispatchContext, CliJudgeContext, RUNBOOK_TEMPLATE, adapter_for}; +use crate::core::fs::artifact_path; use crate::core::{Harness, Mode}; use super::util::{harness_label, mode_str}; @@ -51,16 +52,8 @@ pub(crate) fn build_runbook(ctx: &RunbookContext) -> String { let iteration = ctx.iteration.to_string(); let num_tasks = ctx.num_tasks.to_string(); - let dispatch_json = ctx - .iteration_dir - .join("dispatch.json") - .display() - .to_string(); - let benchmark_path = ctx - .iteration_dir - .join("benchmark.json") - .display() - .to_string(); + let dispatch_json = artifact_path(&ctx.iteration_dir.join("dispatch.json")); + let benchmark_path = artifact_path(&ctx.iteration_dir.join("benchmark.json")); // Shared identity tokens, present in both templates. let mut vars: Vec<(&str, &str)> = vec![ diff --git a/src/cli/run/scratch.rs b/src/cli/run/scratch.rs index 0e5a834..66c4bc5 100644 --- a/src/cli/run/scratch.rs +++ b/src/cli/run/scratch.rs @@ -2,12 +2,13 @@ use std::path::Path; +use crate::core::fs::artifact_path; use crate::sandbox::TASK_SCRATCH_DIR; pub(super) fn context(eval_root: &str) -> String { format!( "Task environment: {eval_root}\nTask-local scratch directory: {}", - Path::new(eval_root).join(TASK_SCRATCH_DIR).display() + artifact_path(&Path::new(eval_root).join(TASK_SCRATCH_DIR)) ) } diff --git a/src/pipeline/grade/judge_tasks.rs b/src/pipeline/grade/judge_tasks.rs index 7de9841..eda5c66 100644 --- a/src/pipeline/grade/judge_tasks.rs +++ b/src/pipeline/grade/judge_tasks.rs @@ -13,7 +13,7 @@ use std::path::Path; use serde::Serialize; use serde_json::json; -use crate::core::fs::write_json; +use crate::core::fs::{artifact_path, write_json}; use crate::core::{Assertion, RunRecord, SKILL_INVOKED_META_ID, ToolInvocation}; use crate::pipeline::error::PipelineError; use crate::pipeline::io::now_iso8601; @@ -204,7 +204,7 @@ fn build_judge_prompt( "", "# Task", "", - &format!("Write your verdict as a JSON file to: {}", response_path.display()), + &format!("Write your verdict as a JSON file to: {}", artifact_path(response_path)), "", "The JSON must match this schema (exactly these keys, no extra prose in the file):", "", @@ -312,10 +312,10 @@ pub fn emit_judge_tasks(ctx: &GradeContext) -> Result Result String { roots.first().map_or_else(String::new, |root| { format!( ". For temporary or scratch files, use {}.", - Path::new(root).join(super::TASK_SCRATCH_DIR).display() + artifact_path(&Path::new(root).join(super::TASK_SCRATCH_DIR)) ) }) } +/// The allowed roots as the deny reason names them. Rendered the same way as +/// the scratch hint beside it, so one sentence never shows the agent a root in +/// one spelling and a directory under it in another. +fn allowed_roots_hint(roots: &[String]) -> String { + roots + .iter() + .map(|root| artifact_path(Path::new(root))) + .collect::>() + .join(", ") +} + /// True when the marker is active and unexpired at `now_ms` (epoch milliseconds). pub(crate) fn marker_is_armed(marker: Option<&GuardMarker>, now_ms: i64) -> bool { let Some(marker) = marker else { @@ -152,10 +165,10 @@ pub(crate) fn decide_with_cwd( return GuardEvaluation::deny( format!( "{GUARD_REASON_PREFIX}{tool_name} to {p} is outside the eval sandbox (allowed: {}){}", - roots.join(", "), + allowed_roots_hint(&roots), scratch_hint(&roots), ), - vec![resolve_path(p, invocation_cwd).display().to_string()], + vec![artifact_path(&resolve_path(p, invocation_cwd))], ); } return GuardEvaluation::allow(); @@ -178,13 +191,13 @@ pub(crate) fn decide_with_cwd( { let resolved_targets = paths .iter() - .map(|target| resolve_path(target, invocation_cwd).display().to_string()) + .map(|target| artifact_path(&resolve_path(target, invocation_cwd))) .collect(); return GuardEvaluation::deny( format!( "{GUARD_REASON_PREFIX}{tool_name} target {path} is outside the eval sandbox \ (allowed: {}){}", - roots.join(", "), + allowed_roots_hint(&roots), scratch_hint(&roots), ), resolved_targets, diff --git a/src/sandbox/git_command.rs b/src/sandbox/git_command.rs index 1eaf783..e625b45 100644 --- a/src/sandbox/git_command.rs +++ b/src/sandbox/git_command.rs @@ -3,6 +3,7 @@ use std::path::Path; use crate::core::GIT_ROUTING_ENV_VARS; +use crate::core::fs::artifact_path; use super::policy::{BashClassification, is_under_any, resolve_path}; use super::shell_targets::{ShellToken, ShellWord, lex_shell}; @@ -54,8 +55,7 @@ fn routing_value_is_allowed( return false; } values.into_iter().all(|value| { - let resolved = resolve_path(&value, cwd); - resolved_targets.push(resolved.display().to_string()); + resolved_targets.push(artifact_path(&resolve_path(&value, cwd))); is_under_any(&value, allowed_roots, cwd) }) } diff --git a/src/sandbox/shell_targets.rs b/src/sandbox/shell_targets.rs index 116553b..389cdcb 100644 --- a/src/sandbox/shell_targets.rs +++ b/src/sandbox/shell_targets.rs @@ -2,6 +2,8 @@ use std::path::Path; +use crate::core::fs::artifact_path; + use super::policy::{BashClassification, OUTPUT_REDIRECTION_REASON, is_under_any, resolve_path}; #[derive(Debug, Clone, PartialEq, Eq)] @@ -345,7 +347,7 @@ fn record_literal_target( if is_non_file_device(&resolved) { return true; } - resolved_targets.push(resolved.display().to_string()); + resolved_targets.push(artifact_path(&resolved)); is_under_any(&word.value, allowed_roots, invocation_cwd) } From 130284399c8ab636ea72fec6806acea936abca34 Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Fri, 14 Aug 2026 02:20:52 -0400 Subject: [PATCH 05/18] fix(pipeline): match paths across host spellings when comparing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three comparisons matched a path against a differently-spelled copy of the same path and silently answered "no". Each failure is quiet by design — the code reports nothing rather than erroring — so on Windows a real signal simply stopped appearing. `prompt_read_failed` searched `args.to_string()`, the *serialized* JSON, for a raw path. Serializing escapes every Windows separator to `\`, so the text never contained the path as written: `skipped_prompt_unread` stayed 0 and a dispatch that never received its instructions was recorded as data. It now walks the args' string leaves — which also reaches nested shapes like cline's `files[].path`, the reason the serialized form was searched to begin with — and compares with separators normalized. This matters beyond Windows now: `dispatch_prompt_path` is forward-slash wire format while the harness transcript echoes the agent's own spelling. `detect_live_source_reads` compared the recorded live directory against the raw command text, so an arm could read the live skill source and still produce a clean stray-write report — a contaminated arm presented as comparable data. `colliding_staged_source` split `discovery_path` on `MAIN_SEPARATOR` to take a basename. Now that the field is forward-slash wire format, that found no basename at all on Windows and let a refutation through on exactly the collision the check exists to block; it splits on either separator. Windows lib failures: 19 -> 15. Refs #246 Co-Authored-By: Claude Opus 5 --- src/adapters/skill_shadow/verification.rs | 7 ++- src/pipeline/detect_stray_writes.rs | 34 +++++++++-- src/pipeline/record_runs.rs | 22 +++++++- src/pipeline/record_runs/tests/prompt_read.rs | 56 +++++++++++++++++++ 4 files changed, 112 insertions(+), 7 deletions(-) diff --git a/src/adapters/skill_shadow/verification.rs b/src/adapters/skill_shadow/verification.rs index 8b6c453..11d3d7d 100644 --- a/src/adapters/skill_shadow/verification.rs +++ b/src/adapters/skill_shadow/verification.rs @@ -117,6 +117,11 @@ fn shows(source: &ShadowSource, dispatch: &dyn DispatchEvidence) -> bool { /// Compares the recorded `runtime_id` and, belt-and-braces, the staged /// directory name: harnesses that advertise a staged skill by its directory /// rather than its logical name would otherwise slip past. +/// +/// The basename is split on either separator. `discovery_path` is wire format +/// (forward slashes) while the host separator may be `\`, so keying off +/// `MAIN_SEPARATOR` would find no basename at all and let a refutation through +/// on the very collision this exists to block. fn colliding_staged_source<'a>( finding: &'a ShadowFinding, live: &ShadowSource, @@ -137,7 +142,7 @@ fn colliding_staged_source<'a>( staged.runtime_id == live.runtime_id || staged .discovery_path - .rsplit(std::path::MAIN_SEPARATOR) + .rsplit(['/', '\\']) .next() .is_some_and(|basename| basename == live.runtime_id) }) diff --git a/src/pipeline/detect_stray_writes.rs b/src/pipeline/detect_stray_writes.rs index 7632098..2c98549 100644 --- a/src/pipeline/detect_stray_writes.rs +++ b/src/pipeline/detect_stray_writes.rs @@ -19,7 +19,7 @@ use std::path::Path; use serde::{Deserialize, Serialize}; use crate::adapters::{all_config_dir_names, all_tool_vocabulary}; -use crate::core::fs::write_json; +use crate::core::fs::{normalize_separators, write_json}; use crate::core::{ConditionsRecord, RunRecord, ToolInvocation}; use crate::pipeline::error::PipelineError; use crate::pipeline::guard_denials::collect_guard_denials; @@ -184,7 +184,12 @@ pub fn detect_live_source_reads( ) -> Vec { let mut findings = Vec::new(); let live_dir = lexically_absolute(live_skill_dir); - let live_dir_str = live_dir.to_string_lossy(); + // Both sides of the shell-command comparison below are separator-normalized: + // the live directory is a host path, the command is whatever the agent + // typed, and on Windows those disagree. Left raw, an arm could read the live + // source and still come back clean — a contaminated arm reported as + // comparable data. + let live_dir_str = normalize_separators(&live_dir.to_string_lossy()); let rel = path_relative(repo_root, &live_dir); let rel_usable = !rel.starts_with(".."); let config_dirs = all_config_dir_names(); @@ -207,8 +212,9 @@ pub fn detect_live_source_reads( if is_shell_tool(&inv.name) { let command = command_of(inv); - if command.contains(live_dir_str.as_ref()) - || (rel_usable && references_bare_rel(command, &rel, &config_dirs)) + let normalized = normalize_separators(command); + if normalized.contains(&live_dir_str) + || (rel_usable && references_bare_rel(&normalized, &rel, &config_dirs)) { findings.push(StrayFinding { tool: inv.name.clone(), @@ -768,6 +774,26 @@ mod tests { assert_eq!(f.len(), 1); } + /// The live directory is recorded as a host path while the command is + /// whatever the agent typed, and on Windows those two disagree by + /// separator. Comparing them raw meant an arm could read the live source + /// and the stray-write report still came back clean — a silent + /// eval-validity hole, since a contaminated arm is not comparable data. + #[test] + fn a_bash_spelling_the_live_dir_with_the_other_separator_is_flagged() { + let f = detect_live_source_reads( + &[inv( + "Bash", + json!({"command": r"cat \work\repo\skills\mr-review\SKILL.md"}), + 0, + )], + live(), + repo(), + ); + assert_eq!(f.len(), 1); + assert_eq!(f[0].tool, "Bash"); + } + #[test] fn a_bash_referencing_a_staged_copy_under_dot_claude_skills_is_not_flagged() { let f = detect_live_source_reads( diff --git a/src/pipeline/record_runs.rs b/src/pipeline/record_runs.rs index 863ea77..10817c3 100644 --- a/src/pipeline/record_runs.rs +++ b/src/pipeline/record_runs.rs @@ -25,7 +25,7 @@ use std::path::{Path, PathBuf}; use serde::Deserialize; use crate::adapters::{PermissionDenial, TranscriptSummary, adapter_for}; -use crate::core::fs::write_json; +use crate::core::fs::{normalize_separators, write_json}; use crate::core::{ ConversationEvent, ConversationRecord, Harness, RunRecord, TimingRecord, TimingSource, }; @@ -367,13 +367,14 @@ fn prompt_read_failed(summary: &TranscriptSummary, prompt_path: &str, sentinel: if sentinel.is_empty() { return false; } + let needle = normalize_separators(prompt_path); let mut referenced = false; let mut delivered = false; for inv in &summary.tool_invocations { let mentions_prompt = inv .args .as_ref() - .is_some_and(|a| a.to_string().contains(prompt_path)); + .is_some_and(|a| args_name_path(a, &needle)); if !mentions_prompt { continue; } @@ -388,6 +389,23 @@ fn prompt_read_failed(summary: &TranscriptSummary, prompt_path: &str, sentinel: referenced && !delivered } +/// True when any string leaf of a tool call's `args` names `needle`, which the +/// caller has already separator-normalized. +/// +/// Walked leaf by leaf rather than searched over `args.to_string()`: serializing +/// to JSON escapes each Windows separator to `\\`, so the serialized text never +/// contains the path as written and the search silently answered "no". Recursing +/// also reaches nested shapes — cline's `read_files` carries the path at +/// `files[].path` — which is why the serialized form was searched to begin with. +fn args_name_path(args: &serde_json::Value, needle: &str) -> bool { + match args { + serde_json::Value::String(text) => normalize_separators(text).contains(needle), + serde_json::Value::Array(items) => items.iter().any(|item| args_name_path(item, needle)), + serde_json::Value::Object(map) => map.values().any(|value| args_name_path(value, needle)), + _ => false, + } +} + /// The dispatch prompt's distinctive first non-empty line, used as the sentinel /// for [`prompt_read_failed`]. Empty when the prompt file is missing/unreadable. fn prompt_sentinel(prompt_path: &str) -> String { diff --git a/src/pipeline/record_runs/tests/prompt_read.rs b/src/pipeline/record_runs/tests/prompt_read.rs index 86cada7..3adbb6d 100644 --- a/src/pipeline/record_runs/tests/prompt_read.rs +++ b/src/pipeline/record_runs/tests/prompt_read.rs @@ -3,6 +3,62 @@ use super::*; +use crate::core::ToolInvocation; + +/// One invocation whose args reach the prompt path through a nested shape — +/// cline's `read_files` puts it under `files[].path` — with `result` attached. +fn nested_read(prompt_path: &str, result: &str) -> TranscriptSummary { + TranscriptSummary { + tool_invocations: vec![ToolInvocation { + name: "read_files".to_string(), + args: Some(json!({"files": [{"path": prompt_path}]})), + ordinal: 0, + result: Some(json!(result)), + }], + events: Vec::new(), + session_id: None, + total_tokens: None, + duration_ms: None, + final_text: None, + } +} + +/// The dispatch records the prompt path as forward-slash wire format; the +/// harness transcript echoes back whatever the agent's host spelled, which on +/// Windows is the same path with backslashes. The two must still match, and the +/// args have to be compared as *data*: serializing them to JSON re-escapes each +/// separator to `\\`, so a raw-path `contains` over the serialized text can +/// never match and the guard silently stops firing. +#[test] +fn flags_a_failed_read_when_dispatch_and_transcript_spell_the_path_differently() { + let summary = nested_read( + r"C:\work\cond\outputs\dispatch-prompt.txt", + "File is outside the allowed working directory.", + ); + + assert!(prompt_read_failed( + &summary, + "C:/work/cond/outputs/dispatch-prompt.txt", + PROMPT_SENTINEL + )); +} + +/// The same spelling mismatch must not turn a *successful* read into a skip: +/// the result carries the sentinel, so the dispatch is real data. +#[test] +fn records_a_successful_read_when_dispatch_and_transcript_spell_the_path_differently() { + let summary = nested_read( + r"C:\work\cond\outputs\dispatch-prompt.txt", + &format!(" 1→{PROMPT_SENTINEL}"), + ); + + assert!(!prompt_read_failed( + &summary, + "C:/work/cond/outputs/dispatch-prompt.txt", + PROMPT_SENTINEL + )); +} + #[test] fn flags_dispatch_whose_prompt_read_failed() { // A dispatch that couldn't read its prompt still exits 0 and emits a From 379c996a023d0111723523e60d89065ab43e7622 Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Fri, 14 Aug 2026 02:28:06 -0400 Subject: [PATCH 06/18] fix(tests): stop encoding one platform's path rules in fixtures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Twelve failures were the tests' own assumptions rather than the code's. The guard suites built their marker and manifest JSON with `format!`, interpolating a raw path. On Windows that embeds `\U`, `\A`, `\T` — none of them valid JSON escapes — so the marker was malformed, the guard read it as absent, and every assertion passed through the fail-open path. Ten tests reported nothing about the guard while looking like they covered it. They now serialize with `serde_json`, which is also what the production installer does. The `sandbox::install` byte pins had the mirror-image bug: the *expectation* interpolated the marker raw into a JSON string the real file escapes. `canonical_root` returned a verbatim (`\?\`) path, but a child process reports the plain form as its cwd, so no path the CLI emitted could ever match one a test joined onto that root. The `tests/run` counterpart is now a shared `resolved()` helper — it keeps the symlink resolution macOS needs for its temp dirs and drops only the prefix. The rest compare against generated artifacts that are now forward-slash wire format (`discovery_path`, `response_path`, `eval_root`) or against `cargo package --list`, which prints forward slashes everywhere. Where a choice existed the comparison was normalized rather than the POSIX literal rewritten: those spellings are the behavior under test, since agents emit POSIX paths on any host. Building the env manifest's `envs[].dir` through `artifact_path` as well — readers join it against a task's `eval_root`, and the two had drifted into different spellings. Windows failures: 68 -> 17, all of them #247's POSIX-executable cluster (`harness_lint_probe_*` x6, `command_check` x4, `judge_recipe_*` x3, `execute_with_timeout_*` x2, `run_git_spawn_error_surfaced`, `execute_round_creates_the_round_output_directory_before_shell_redirection`). `tests/run` is fully green. No golden fixture changed. Refs #246 Co-Authored-By: Claude Opus 5 --- src/cli/run/orchestrate/build.rs | 32 ++++++++------- src/cli/run/orchestrate/envs.rs | 12 +++--- src/sandbox/install.rs | 14 ++++++- tests/cli/grade.rs | 4 +- tests/cli/guard.rs | 69 +++++++++++++++----------------- tests/cli/helpers.rs | 7 ++++ tests/cli/package.rs | 7 +++- tests/run/codex.rs | 5 +-- tests/run/env_layout.rs | 12 +----- tests/run/helpers.rs | 22 ++++++++++ tests/run/opencode.rs | 5 +-- 11 files changed, 111 insertions(+), 78 deletions(-) diff --git a/src/cli/run/orchestrate/build.rs b/src/cli/run/orchestrate/build.rs index 8d53ea7..4b2b698 100644 --- a/src/cli/run/orchestrate/build.rs +++ b/src/cli/run/orchestrate/build.rs @@ -24,7 +24,7 @@ use super::super::util::unguarded_notice; use super::envs::{EnvLayoutInput, env_targets, task_env_root_for_run, task_run_indices}; use super::{Resolved, RunOptions, Staged}; use crate::cli::command_target_args; -use crate::core::fs::write_json; +use crate::core::fs::{artifact_path, write_json}; /// Build every `(eval, condition)` dispatch task and write `conditions.json`, /// `dispatch-manifest.md`, the per-task prompt files, and `dispatch.json`. @@ -63,11 +63,11 @@ pub(super) fn write_dispatch( let staged_skill_path_for = |env_root: &Path, cond_slug: Option<&str>| -> Option { cond_slug.map(|slug| { - skills_dir_for_harness(env_root, ctx.harness) - .join(slug) - .join("SKILL.md") - .to_string_lossy() - .into_owned() + artifact_path( + &skills_dir_for_harness(env_root, ctx.harness) + .join(slug) + .join("SKILL.md"), + ) }) }; @@ -85,11 +85,11 @@ pub(super) fn write_dispatch( .iter() .map(|(name, description)| AvailableSkill { name: name.clone(), - path: skills_dir_for_harness(env_root, ctx.harness) - .join(name) - .join("SKILL.md") - .to_string_lossy() - .into_owned(), + path: artifact_path( + &skills_dir_for_harness(env_root, ctx.harness) + .join(name) + .join("SKILL.md"), + ), description: description.clone(), }) .collect(); @@ -254,7 +254,7 @@ pub(super) fn write_dispatch( "skill_name": ctx.skill_name, "iteration": r.iteration, "run_nonce": r.run_nonce, - "iteration_dir": r.iteration_dir.to_string_lossy(), + "iteration_dir": artifact_path(&r.iteration_dir), "mode": r.mode, "baseline": r.baseline, "plan_mode": opts.plan_mode, @@ -293,13 +293,15 @@ pub(super) fn write_dispatch( task_run_indices(g).into_iter().map(move |run_index| { let mut env = json!({ "condition": cond, - "dir": task_env_root_for_run( + // Same env, same spelling as the task's `eval_root` + // — the two fields are joined on by readers of + // dispatch.json. + "dir": artifact_path(&task_env_root_for_run( &r.iteration_dir, &g.id, cond, run_index, - ) - .to_string_lossy(), + )), }); if let Some(run_index) = run_index { env.as_object_mut() diff --git a/src/cli/run/orchestrate/envs.rs b/src/cli/run/orchestrate/envs.rs index 7b864d2..d1fdb28 100644 --- a/src/cli/run/orchestrate/envs.rs +++ b/src/cli/run/orchestrate/envs.rs @@ -87,6 +87,11 @@ pub(super) fn env_targets(input: &EnvLayoutInput) -> Vec { mod tests { use super::*; + // `EnvTarget.root` stays a `PathBuf` for filesystem use and is only rendered + // as wire format where it is serialized, so the layout assertions below + // compare the rendered form rather than the host's spelling. + use crate::core::fs::artifact_path; + fn groups() -> Vec { vec![ Group { @@ -117,10 +122,7 @@ mod tests { skill_path_b: None, }); assert_eq!(targets.len(), 4, "2 groups × 2 conditions"); - let roots: Vec = targets - .iter() - .map(|t| t.root.to_string_lossy().into_owned()) - .collect(); + let roots: Vec = targets.iter().map(|t| artifact_path(&t.root)).collect(); assert_eq!( roots, vec![ @@ -174,7 +176,7 @@ mod tests { assert_eq!( targets .iter() - .map(|target| target.root.to_string_lossy().into_owned()) + .map(|target| artifact_path(&target.root)) .collect::>(), vec![ "/w/iteration-1/env-g1-with_skill-run-1", diff --git a/src/sandbox/install.rs b/src/sandbox/install.rs index 1ca3fc1..66f282a 100644 --- a/src/sandbox/install.rs +++ b/src/sandbox/install.rs @@ -204,6 +204,16 @@ mod tests { stage_root: PathBuf, } + /// The marker path as it appears *inside* a JSON string value: the hook + /// command embeds it, so every Windows separator is escaped to `\\`. + /// Interpolating `display()` raw builds an expectation that is not even + /// valid JSON, and the byte pin then fails on a difference the file does + /// not have. + fn json_string_body(path: &Path) -> String { + let quoted = serde_json::to_string(&path.to_string_lossy()).unwrap(); + quoted[1..quoted.len() - 1].to_string() + } + fn setup() -> Case { let tmp = TempDir::new().unwrap(); let stage_root = tmp.path().join("stage"); @@ -267,7 +277,7 @@ mod tests { }} }} "#, - marker = marker.display() + marker = json_string_body(&marker) ); assert_eq!(settings, expected); } @@ -302,7 +312,7 @@ mod tests { }} }} "#, - marker = marker.display() + marker = json_string_body(&marker) ); assert_eq!(hooks, expected); } diff --git a/tests/cli/grade.rs b/tests/cli/grade.rs index 6f6c3ad..5ea9461 100644 --- a/tests/cli/grade.rs +++ b/tests/cli/grade.rs @@ -479,10 +479,12 @@ fn grade_emits_and_finalizes_per_nested_run_dir() { .join(format!("run-{k}")) .join("judge-responses") .join("a1.json"); + // `judge-tasks.json` carries paths as forward-slash wire format, so the + // locally-joined expectation is compared in the same spelling. assert!( a1_response_paths .iter() - .any(|p| *p == expected.to_string_lossy()), + .any(|p| *p == expected.to_string_lossy().replace('\\', "/")), "missing judge task for run-{k}" ); fs::write( diff --git a/tests/cli/guard.rs b/tests/cli/guard.rs index 4763467..9cb03c9 100644 --- a/tests/cli/guard.rs +++ b/tests/cli/guard.rs @@ -23,38 +23,43 @@ fn guard_subcommand_is_hidden_but_callable() { .success(); } -/// Write an armed guard marker scoping writes to ``, and return its path. -fn write_armed_marker(root: &std::path::Path, allowed: &std::path::Path) -> std::path::PathBuf { - let skills = root.join(".claude").join("skills"); +/// Write an armed guard marker scoping writes to `` under +/// `//skills`, and return its path. +/// +/// Serialized rather than string-interpolated: a Windows path embeds `\U`, +/// `\A`, `\T` — none of them valid JSON escapes — so a `format!`-built marker +/// is malformed, the guard reads it as absent, and every assertion below +/// silently passes through the fail-open path instead of testing anything. +fn write_marker_in( + root: &std::path::Path, + namespace: &str, + allowed: &std::path::Path, +) -> std::path::PathBuf { + let skills = root.join(namespace).join("skills"); fs::create_dir_all(&skills).unwrap(); let marker = skills.join(".slow-powers-eval-guard.json"); fs::write( &marker, - format!( - r#"{{ "active": true, "allowedRoots": ["{}"], "expiresAt": "2999-01-01T00:00:00.000Z" }}"#, - allowed.display() - ), + serde_json::to_string(&serde_json::json!({ + "active": true, + "allowedRoots": [allowed.to_string_lossy()], + "expiresAt": "2999-01-01T00:00:00.000Z", + })) + .unwrap(), ) .unwrap(); marker } +fn write_armed_marker(root: &std::path::Path, allowed: &std::path::Path) -> std::path::PathBuf { + write_marker_in(root, ".claude", allowed) +} + fn write_codex_armed_marker( root: &std::path::Path, allowed: &std::path::Path, ) -> std::path::PathBuf { - let skills = root.join(".agents").join("skills"); - fs::create_dir_all(&skills).unwrap(); - let marker = skills.join(".slow-powers-eval-guard.json"); - fs::write( - &marker, - format!( - r#"{{ "active": true, "allowedRoots": ["{}"], "expiresAt": "2999-01-01T00:00:00.000Z" }}"#, - allowed.display() - ), - ) - .unwrap(); - marker + write_marker_in(root, ".agents", allowed) } /// `guard` denies a Write outside the sandbox: it prints a PreToolUse deny verdict @@ -256,18 +261,7 @@ fn write_opencode_armed_marker( root: &std::path::Path, allowed: &std::path::Path, ) -> std::path::PathBuf { - let skills = root.join(".opencode").join("skills"); - fs::create_dir_all(&skills).unwrap(); - let marker = skills.join(".slow-powers-eval-guard.json"); - fs::write( - &marker, - format!( - r#"{{ "active": true, "allowedRoots": ["{}"], "expiresAt": "2999-01-01T00:00:00.000Z" }}"#, - allowed.display() - ), - ) - .unwrap(); - marker + write_marker_in(root, ".opencode", allowed) } /// `guard-hook --harness opencode` round-trip, fed the exact payload shape @@ -363,11 +357,14 @@ fn teardown_guard_removes_an_installed_opencode_plugin() { fs::write(&plugin, "// staged plugin\n").unwrap(); fs::write( skills.join(".slow-powers-eval-guard-manifest.json"), - format!( - r#"{{ "created_at": "2026-07-23T00:00:00.000Z", "settings_path": "{}", "settings_existed": false, "settings_backup": null, "marker_path": "{}" }}"#, - plugin.display(), - marker.display() - ), + serde_json::to_string(&serde_json::json!({ + "created_at": "2026-07-23T00:00:00.000Z", + "settings_path": plugin.to_string_lossy(), + "settings_existed": false, + "settings_backup": null, + "marker_path": marker.to_string_lossy(), + })) + .unwrap(), ) .unwrap(); diff --git a/tests/cli/helpers.rs b/tests/cli/helpers.rs index bc34b2f..f3cdbd2 100644 --- a/tests/cli/helpers.rs +++ b/tests/cli/helpers.rs @@ -19,5 +19,12 @@ pub fn skill_eval() -> Command { pub fn canonical_root() -> (TempDir, PathBuf) { let tmp = TempDir::new().unwrap(); let root = fs::canonicalize(tmp.path()).unwrap(); + // `canonicalize` returns a verbatim (`\\?\`) path on Windows, but a child + // process launched with it reports the plain form as its cwd — so paths the + // CLI emits would never match ones a test joins onto this root. + let root = match root.to_string_lossy().strip_prefix(r"\\?\") { + Some(plain) => PathBuf::from(plain), + None => root, + }; (tmp, root) } diff --git a/tests/cli/package.rs b/tests/cli/package.rs index 5e31369..089be8b 100644 --- a/tests/cli/package.rs +++ b/tests/cli/package.rs @@ -119,7 +119,12 @@ fn cargo_package_excludes_repo_local_authoring_files() { if path.extension().and_then(|value| value.to_str()) != Some("md") { continue; } - let relative = path.strip_prefix(repo_root()).unwrap().to_string_lossy(); + // `cargo package --list` prints forward slashes on every platform. + let relative = path + .strip_prefix(repo_root()) + .unwrap() + .to_string_lossy() + .replace('\\', "/"); assert!( files.lines().any(|line| line == relative), "{relative} should be packaged" diff --git a/tests/run/codex.rs b/tests/run/codex.rs index 475d0c2..188d22f 100644 --- a/tests/run/codex.rs +++ b/tests/run/codex.rs @@ -472,10 +472,7 @@ fn codex_warns_when_user_skill_shadows_staged_skill() { .unwrap(); assert_eq!(live["kind"], "skill"); assert_eq!(live["root"]["namespace"], "agents"); - assert_eq!( - live["discovery_path"], - live_skill.to_string_lossy().as_ref() - ); + assert_eq!(live["discovery_path"], wire_path(&live_skill).as_str()); let appearances = live["appearances"].as_array().unwrap(); assert_eq!( appearances.len(), diff --git a/tests/run/env_layout.rs b/tests/run/env_layout.rs index 9259129..400eb10 100644 --- a/tests/run/env_layout.rs +++ b/tests/run/env_layout.rs @@ -429,17 +429,9 @@ fn guard_marker_scopes_allowed_roots_to_private_env() { .iter() .map(|root| root.as_str().unwrap().to_string()) .collect(); - assert_eq!( - roots, - vec![ - fs::canonicalize(&env) - .unwrap() - .to_string_lossy() - .into_owned() - ] - ); + assert_eq!(roots, vec![resolved(&env).to_string_lossy().into_owned()]); - let iter = fs::canonicalize(iteration_dir(&cwd)).unwrap(); + let iter = resolved(&iteration_dir(&cwd)); assert!( !roots.iter().any(|root| iter.starts_with(root)), "allowedRoots {roots:?} must not cover the meta tree above env at {iter:?}" diff --git a/tests/run/helpers.rs b/tests/run/helpers.rs index 0001280..b0335a0 100644 --- a/tests/run/helpers.rs +++ b/tests/run/helpers.rs @@ -56,6 +56,28 @@ pub fn env_staged_entries(cwd: &Path) -> Vec { staged_entries(&cli_env_dir(cwd, "g1", "with_skill").join(".claude/skills")) } +/// A local path as the artifacts spell it: forward slashes on every host, since +/// generated artifacts are a wire format shared across platforms. Mirrors +/// `eval_magic::core::fs::artifact_path` for the integration tests, which build +/// their expectations with `Path::join`. +pub fn wire_path(path: &Path) -> String { + path.to_string_lossy().replace('\\', "/") +} + +/// `fs::canonicalize` with Windows' verbatim (`\\?\`) prefix removed. +/// +/// The symlink resolution matters — a macOS temp dir lives under a symlinked +/// `/var`, so the CLI's own paths resolve to `/private/var/...`. The prefix does +/// not: a child process reports the plain form as its cwd, so plain is the +/// spelling every path the CLI emits actually carries. +pub fn resolved(path: &Path) -> PathBuf { + let canonical = fs::canonicalize(path).unwrap(); + match canonical.to_string_lossy().strip_prefix(r"\\?\") { + Some(plain) => PathBuf::from(plain), + None => canonical, + } +} + pub fn read_json(path: &Path) -> Value { serde_json::from_str(&fs::read_to_string(path).unwrap()).unwrap() } diff --git a/tests/run/opencode.rs b/tests/run/opencode.rs index b729684..c57eb1c 100644 --- a/tests/run/opencode.rs +++ b/tests/run/opencode.rs @@ -377,10 +377,7 @@ fn opencode_warns_when_live_skill_shadows_staged_skill() { assert_eq!(live["kind"], "skill"); assert_eq!(live["root"]["namespace"], "claude"); assert_eq!(live["root"]["relation"], "cross-harness"); - assert_eq!( - live["discovery_path"], - live_skill.to_string_lossy().as_ref() - ); + assert_eq!(live["discovery_path"], wire_path(&live_skill).as_str()); } /// Resolve a dispatch.json path field (absolute, or relative to the run cwd). From 7a3e32605ef0c419c3f3b4fca6e72179fce804fd Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Fri, 14 Aug 2026 02:39:32 -0400 Subject: [PATCH 07/18] refactor(record-runs): extract the prompt-read guard; finish path coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review pass over the branch. `record_runs.rs` crossed 500 lines, so the prompt-read guard moves to `record_runs/prompt_read.rs` — a self-contained concern (does the transcript show the agent's read of its dispatch prompt failing?) beside the existing `conversation.rs`, matching how this module already splits. 501 -> 411 lines. Driving a real `eval-magic run` on Windows found two path fields the tests did not cover, both leaving one logical value spelled two ways in a single document: - `conditions.json`'s `skill_path` is echoed into `dispatch.json` beside the tasks, which carry the same path — one native, one wire format. - The runbook's `ingest` line and the judge recipe's `cd` embedded native paths in POSIX shell commands, where a backslash is an escape character. (The recipes remain POSIX-only; that is #248.) Generated artifacts now contain no Windows-spelled path at all — verified against a real run's `dispatch.json`, `conditions.json`, `dispatch-manifest.md`, and `RUNBOOK.md`. The guard marker stays native by design: it is read back through `resolve_path`, which compares by component. Also tightened comments that narrated the fix rather than the code, and moved a misplaced `crate::` import in `skill_shadow.rs` below the external block. Refs #246 Co-Authored-By: Claude Opus 5 --- src/adapters/descriptor_adapter.rs | 5 +- src/adapters/skill_shadow.rs | 4 +- src/cli/mod.rs | 10 ++- src/cli/run/orchestrate/build.rs | 10 ++- src/core/fs.rs | 8 +- src/pipeline/detect_stray_writes.rs | 16 ++-- src/pipeline/record_runs.rs | 79 ++-------------- src/pipeline/record_runs/prompt_read.rs | 90 +++++++++++++++++++ src/pipeline/record_runs/tests/prompt_read.rs | 10 +-- 9 files changed, 132 insertions(+), 100 deletions(-) create mode 100644 src/pipeline/record_runs/prompt_read.rs diff --git a/src/adapters/descriptor_adapter.rs b/src/adapters/descriptor_adapter.rs index 7948ecc..811939f 100644 --- a/src/adapters/descriptor_adapter.rs +++ b/src/adapters/descriptor_adapter.rs @@ -10,6 +10,7 @@ use std::time::Duration; use regex::Regex; +use crate::core::fs::artifact_path; use crate::core::{AvailableSkill, HarnessRunCapabilities, ToolInvocation}; use crate::sandbox::GuardMarker; @@ -492,7 +493,9 @@ impl HarnessAdapter for DescriptorAdapter { fn cli_judge_next_steps(&self, ctx: CliJudgeContext<'_>) -> Option { let template = self.descriptor.dispatch.judge_command_template.as_ref()?; - let cwd = ctx.iteration_dir.display().to_string(); + // Embedded in a shell command line, so it carries the wire-format + // spelling every other generated path uses. + let cwd = artifact_path(ctx.iteration_dir); let command_line = subst( template, // Judges run from the iteration metadata directory, outside every diff --git a/src/adapters/skill_shadow.rs b/src/adapters/skill_shadow.rs index bddc80f..7da6374 100644 --- a/src/adapters/skill_shadow.rs +++ b/src/adapters/skill_shadow.rs @@ -10,10 +10,10 @@ use std::collections::{BTreeMap, BTreeSet}; use std::path::Path; -use crate::core::fs::artifact_path; - use serde::{Deserialize, Serialize}; +use crate::core::fs::artifact_path; + mod artifact; mod resolution; pub(crate) mod verification; diff --git a/src/cli/mod.rs b/src/cli/mod.rs index c755c22..253f00e 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -26,6 +26,7 @@ use std::path::{Path, PathBuf}; use anyhow::{anyhow, bail}; use clap::Parser; +use crate::core::fs::artifact_path; use crate::core::{DetectInput, Harness, RunContext, detect_run_context}; mod args; @@ -172,9 +173,9 @@ pub(crate) fn parse_id_list(v: Option<&str>) -> Option> { pub(crate) fn command_target_args(ctx: &RunContext) -> String { format!( " --skill-dir {} --skill {} --workspace-dir {}", - ctx.skill_dir.display(), + artifact_path(&ctx.skill_dir), ctx.skill_name, - ctx.workspace_root.display(), + artifact_path(&ctx.workspace_root), ) } @@ -324,7 +325,10 @@ mod tests { let args = command_target_args(&ctx); assert!( - args.contains(&format!("--workspace-dir {}", ctx.workspace_root.display())), + args.contains(&format!( + "--workspace-dir {}", + artifact_path(&ctx.workspace_root) + )), "selector names absolute --workspace-dir: {args}" ); assert!( diff --git a/src/cli/run/orchestrate/build.rs b/src/cli/run/orchestrate/build.rs index 4b2b698..1951009 100644 --- a/src/cli/run/orchestrate/build.rs +++ b/src/cli/run/orchestrate/build.rs @@ -35,18 +35,24 @@ pub(super) fn write_dispatch( r: &Resolved, staged: &Staged, ) -> Result { + // `conditions.json` is echoed into `dispatch.json` beside the tasks, which + // carry the same skill path — spelling it differently in the two places + // would put one field in two forms in one file. + let condition_skill_path = |path: &Option| -> Option { + path.as_deref().map(|p| artifact_path(Path::new(p))) + }; let conditions = ConditionsRecord { mode: r.mode, baseline: r.baseline.clone(), conditions: vec![ ConditionEntry { name: r.cond_a.to_string(), - skill_path: r.skill_path_a.clone(), + skill_path: condition_skill_path(&r.skill_path_a), staged_skill_slug: Some(staged.cond_a_slug.clone()), }, ConditionEntry { name: r.cond_b.to_string(), - skill_path: r.skill_path_b.clone(), + skill_path: condition_skill_path(&r.skill_path_b), staged_skill_slug: Some(staged.cond_b_slug.clone()), }, ], diff --git a/src/core/fs.rs b/src/core/fs.rs index b70bcd8..9b439a6 100644 --- a/src/core/fs.rs +++ b/src/core/fs.rs @@ -165,10 +165,10 @@ mod tests { ); } - /// The bug this exists for: `Path::join` on a POSIX-rooted base emits a - /// Windows separator, so a manifest entry reads `/work/cond\run.json`. The - /// verbatim `\\?\` prefix `canonicalize` returns is stripped too — it is an - /// OS-level escape hatch, not something an agent should ever be handed. + /// `Path::join` on a POSIX-rooted base emits a Windows separator, so a + /// manifest entry would otherwise read `/work/cond\run.json`. A verbatim + /// `\\?\` prefix is stripped too — it is an OS-level escape hatch, not + /// something an agent should ever be handed. #[cfg(windows)] #[test] fn artifact_path_rewrites_windows_separators_and_strips_verbatim_prefixes() { diff --git a/src/pipeline/detect_stray_writes.rs b/src/pipeline/detect_stray_writes.rs index 2c98549..38cf9e8 100644 --- a/src/pipeline/detect_stray_writes.rs +++ b/src/pipeline/detect_stray_writes.rs @@ -184,11 +184,9 @@ pub fn detect_live_source_reads( ) -> Vec { let mut findings = Vec::new(); let live_dir = lexically_absolute(live_skill_dir); - // Both sides of the shell-command comparison below are separator-normalized: - // the live directory is a host path, the command is whatever the agent - // typed, and on Windows those disagree. Left raw, an arm could read the live - // source and still come back clean — a contaminated arm reported as - // comparable data. + // Normalized for the shell-command comparison below: the live directory is a + // host path while the command is whatever the agent typed, so on Windows the + // two spell the same directory differently. let live_dir_str = normalize_separators(&live_dir.to_string_lossy()); let rel = path_relative(repo_root, &live_dir); let rel_usable = !rel.starts_with(".."); @@ -774,11 +772,9 @@ mod tests { assert_eq!(f.len(), 1); } - /// The live directory is recorded as a host path while the command is - /// whatever the agent typed, and on Windows those two disagree by - /// separator. Comparing them raw meant an arm could read the live source - /// and the stray-write report still came back clean — a silent - /// eval-validity hole, since a contaminated arm is not comparable data. + /// A contaminated arm is not comparable data, so the scan has to hold when + /// the command spells the live directory with the other separator — which + /// on Windows is every command, since the recorded directory is a host path. #[test] fn a_bash_spelling_the_live_dir_with_the_other_separator_is_flagged() { let f = detect_live_source_reads( diff --git a/src/pipeline/record_runs.rs b/src/pipeline/record_runs.rs index 10817c3..7b6b07f 100644 --- a/src/pipeline/record_runs.rs +++ b/src/pipeline/record_runs.rs @@ -18,6 +18,10 @@ //! iteration-level `permission-denials.json` written here (see //! [`crate::pipeline::permission_denials`]); harnesses that cannot detect a //! refusal get no file at all, so its absence never reads as "nothing refused". +//! +//! Two sub-concerns live beside this module: [`conversation`] assembles a +//! scripted task's ordered rounds, and [`prompt_read`] decides whether a +//! dispatch ever received its instructions. use std::fs; use std::path::{Path, PathBuf}; @@ -25,7 +29,7 @@ use std::path::{Path, PathBuf}; use serde::Deserialize; use crate::adapters::{PermissionDenial, TranscriptSummary, adapter_for}; -use crate::core::fs::{normalize_separators, write_json}; +use crate::core::fs::write_json; use crate::core::{ ConversationEvent, ConversationRecord, Harness, RunRecord, TimingRecord, TimingSource, }; @@ -34,8 +38,10 @@ use crate::pipeline::permission_denials::{self, TaskPermissionDenials}; use crate::pipeline::session_surface::{self, RoundSurface, TaskSessionSurface}; use crate::pipeline::shadow_verification; use crate::validation::{SchemaName, validate_against_schema}; +use prompt_read::{prompt_read_failed, prompt_sentinel}; mod conversation; +mod prompt_read; /// The `dispatch.json` envelope record-runs reads. #[derive(Debug, Deserialize)] @@ -351,77 +357,6 @@ pub fn record_runs( Ok(result) } -/// Positive evidence that the agent tried to read its dispatch prompt and -/// failed: the transcript has a tool call referencing `prompt_path`, yet no such -/// call returned the prompt's content (its distinctive first-line `sentinel`). -/// -/// A run that never references the prompt path is NOT flagged — absence is not -/// proof of failure (the agent can receive the prompt another way), -/// and requiring positive evidence keeps the check free of false positives. -/// An invocation without a string result is not judged either: transcript -/// readers that leave results unjoined (the declarative extract tier) carry no -/// delivery evidence, and treating that as failure flags every successful read. -/// Returns `false` when `sentinel` is empty (the prompt file was missing or -/// unreadable, so the read cannot be judged). -fn prompt_read_failed(summary: &TranscriptSummary, prompt_path: &str, sentinel: &str) -> bool { - if sentinel.is_empty() { - return false; - } - let needle = normalize_separators(prompt_path); - let mut referenced = false; - let mut delivered = false; - for inv in &summary.tool_invocations { - let mentions_prompt = inv - .args - .as_ref() - .is_some_and(|a| args_name_path(a, &needle)); - if !mentions_prompt { - continue; - } - let Some(result) = inv.result.as_ref().and_then(serde_json::Value::as_str) else { - continue; - }; - referenced = true; - if result.contains(sentinel) { - delivered = true; - } - } - referenced && !delivered -} - -/// True when any string leaf of a tool call's `args` names `needle`, which the -/// caller has already separator-normalized. -/// -/// Walked leaf by leaf rather than searched over `args.to_string()`: serializing -/// to JSON escapes each Windows separator to `\\`, so the serialized text never -/// contains the path as written and the search silently answered "no". Recursing -/// also reaches nested shapes — cline's `read_files` carries the path at -/// `files[].path` — which is why the serialized form was searched to begin with. -fn args_name_path(args: &serde_json::Value, needle: &str) -> bool { - match args { - serde_json::Value::String(text) => normalize_separators(text).contains(needle), - serde_json::Value::Array(items) => items.iter().any(|item| args_name_path(item, needle)), - serde_json::Value::Object(map) => map.values().any(|value| args_name_path(value, needle)), - _ => false, - } -} - -/// The dispatch prompt's distinctive first non-empty line, used as the sentinel -/// for [`prompt_read_failed`]. Empty when the prompt file is missing/unreadable. -fn prompt_sentinel(prompt_path: &str) -> String { - if prompt_path.is_empty() { - return String::new(); - } - fs::read_to_string(prompt_path) - .ok() - .and_then(|p| { - p.lines() - .find(|l| !l.trim().is_empty()) - .map(|l| l.trim().to_string()) - }) - .unwrap_or_default() -} - /// Resolve a task's transcript summary: read the events file the harness CLI /// wrote under the task's outputs dir (e.g. Codex's `codex-events.jsonl`, Claude /// Code's `claude-events.jsonl`). Returns `None` when no transcript is found. diff --git a/src/pipeline/record_runs/prompt_read.rs b/src/pipeline/record_runs/prompt_read.rs new file mode 100644 index 0000000..692b05c --- /dev/null +++ b/src/pipeline/record_runs/prompt_read.rs @@ -0,0 +1,90 @@ +//! The prompt-read guard: whether a dispatch's transcript shows the agent +//! trying to read its dispatch prompt and getting an error instead. +//! +//! A dispatch that never received its instructions still exits 0 and still +//! emits a final message, so nothing downstream distinguishes it from a real +//! run. `record_runs` consults this to skip it rather than record a silent +//! no-op as data. + +use std::fs; + +use serde_json::Value; + +use crate::adapters::TranscriptSummary; +use crate::core::fs::normalize_separators; + +/// Positive evidence that the agent tried to read its dispatch prompt and +/// failed: the transcript has a tool call referencing `prompt_path`, yet no such +/// call returned the prompt's content (its distinctive first-line `sentinel`). +/// +/// A run that never references the prompt path is NOT flagged — absence is not +/// proof of failure (the agent can receive the prompt another way), +/// and requiring positive evidence keeps the check free of false positives. +/// An invocation without a string result is not judged either: transcript +/// readers that leave results unjoined (the declarative extract tier) carry no +/// delivery evidence, and treating that as failure flags every successful read. +/// Returns `false` when `sentinel` is empty (the prompt file was missing or +/// unreadable, so the read cannot be judged). +pub(super) fn prompt_read_failed( + summary: &TranscriptSummary, + prompt_path: &str, + sentinel: &str, +) -> bool { + if sentinel.is_empty() { + return false; + } + // The dispatch spells the path as wire format while the transcript echoes + // the agent's own host spelling, so neither side can be matched as-is. + let needle = normalize_separators(prompt_path); + let mut referenced = false; + let mut delivered = false; + for inv in &summary.tool_invocations { + let mentions_prompt = inv + .args + .as_ref() + .is_some_and(|a| args_name_path(a, &needle)); + if !mentions_prompt { + continue; + } + let Some(result) = inv.result.as_ref().and_then(Value::as_str) else { + continue; + }; + referenced = true; + if result.contains(sentinel) { + delivered = true; + } + } + referenced && !delivered +} + +/// True when any string leaf of a tool call's `args` names `needle`, which the +/// caller has already separator-normalized. +/// +/// Walks the leaves rather than searching `args.to_string()`, for two reasons: +/// serializing to JSON escapes a Windows separator to `\\`, so a path never +/// appears in the serialized text as written; and the path can sit at any depth +/// — cline's `read_files` carries it at `files[].path`. +fn args_name_path(args: &Value, needle: &str) -> bool { + match args { + Value::String(text) => normalize_separators(text).contains(needle), + Value::Array(items) => items.iter().any(|item| args_name_path(item, needle)), + Value::Object(map) => map.values().any(|value| args_name_path(value, needle)), + _ => false, + } +} + +/// The dispatch prompt's distinctive first non-empty line, used as the sentinel +/// for [`prompt_read_failed`]. Empty when the prompt file is missing/unreadable. +pub(super) fn prompt_sentinel(prompt_path: &str) -> String { + if prompt_path.is_empty() { + return String::new(); + } + fs::read_to_string(prompt_path) + .ok() + .and_then(|p| { + p.lines() + .find(|l| !l.trim().is_empty()) + .map(|l| l.trim().to_string()) + }) + .unwrap_or_default() +} diff --git a/src/pipeline/record_runs/tests/prompt_read.rs b/src/pipeline/record_runs/tests/prompt_read.rs index 3adbb6d..64c4d37 100644 --- a/src/pipeline/record_runs/tests/prompt_read.rs +++ b/src/pipeline/record_runs/tests/prompt_read.rs @@ -23,12 +23,10 @@ fn nested_read(prompt_path: &str, result: &str) -> TranscriptSummary { } } -/// The dispatch records the prompt path as forward-slash wire format; the -/// harness transcript echoes back whatever the agent's host spelled, which on -/// Windows is the same path with backslashes. The two must still match, and the -/// args have to be compared as *data*: serializing them to JSON re-escapes each -/// separator to `\\`, so a raw-path `contains` over the serialized text can -/// never match and the guard silently stops firing. +/// The dispatch records the prompt path as forward-slash wire format while the +/// harness transcript echoes back the agent's host spelling, so the two must +/// still match — and the args have to be compared as data, since serializing +/// them re-escapes each Windows separator to `\\`. #[test] fn flags_a_failed_read_when_dispatch_and_transcript_spell_the_path_differently() { let summary = nested_read( From f50ca56c36a362549a9836fd1a06f530c8b7f754 Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Fri, 14 Aug 2026 18:00:25 -0400 Subject: [PATCH 08/18] fix(windows): resolve a POSIX shell; gate tests on capabilities, not the OS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #247, whose diagnosis was wrong in two places. The issue reports the failures as confined to the test suite, and the spawning code as already portable. Neither held. `/bin/sh` was hardcoded in two production paths — `harness lint --probe` and multi-turn `dispatch-task` — so both were broken on every released Windows binary, not merely under test. `posix_shell()` now resolves a real `sh`: `EVAL_MAGIC_SH`, then `PATH`, then a Git for Windows install, then `/bin/sh`. It never accepts `bash`, because `System32\bash.exe` is the WSL launcher and resolves a different filesystem namespace. That alone fixed 9 of the 17 failures, with no descriptor changes: the shipped `exec_template`s are POSIX command lines and run unmodified. The `command_check` cluster was not a POSIX-executable problem either — that path already chose `cmd /C` on Windows. Two separate bugs hid behind it: - `Command::arg` escapes a command's embedded quotes as `\"`, which `cmd.exe` does not understand, so any eval author's quoted `command_check` silently arrived split at its spaces. Now `/S /C` plus `raw_arg`, which hands `cmd` the string verbatim. This is user-facing and independent of the tests. - The batch fixture strings were not equivalent to their POSIX counterparts (`echo x>>f` appends CRLF; `echo|set /p=` cannot round-trip a value). Both dialects are gone. A hidden `__fixture` subcommand now supplies the predictable child process the suite needs — a chosen exit code, chosen bytes, a chosen file — in one invocation that `sh -c` and `cmd /C` parse identically. Tests state the capability they need instead of the OS they tolerate. An `#[cfg(unix)]` attribute hides a test from compilation and clippy on the other host and hides the coverage gap with it; `report_skip` prints why a test was skipped, and `EVAL_MAGIC_REQUIRE_POSIX_TOOLS=1` (set in CI) turns every skip into a failure so coverage cannot rot. Two capabilities are gated: the POSIX toolchain the shipped recipes need, and symlink creation, which Windows permits only under Developer Mode. Where a per-OS difference is genuinely the behavior under test — signals, path separators — both arms now compile everywhere behind a runtime `cfg!(windows)`. `run_git_spawn_error_surfaced` was a third category again: it pinned the POSIX errno spelling while its own doc comment said the contract was a readable reason, not a particular one. `#[cfg(unix)]`/`#[cfg(windows)]` now appears 6 times, all in production code and none in tests: the per-OS symlink API, the shell selection, and reading a signal Windows does not have. Verified on Windows 11, rustc 1.97.1: - `cargo test --no-fail-fast` — 1122 passing, 0 failing, up from 1069 passing and 17 failing. - `cargo fmt --check` and `cargo clippy --all-targets -- -D warnings` clean. - `harness lint codex --probe` reaches the shell and reports exit 127 (`codex` absent) rather than failing to spawn. Not executed on this host: the three `judge_recipe_*` tests skip without `jq`, and the three symlink round-trips skip without Developer Mode. Both run on the Linux job, which sets the enforcement variable. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 6 + AGENTS.md | 22 ++ src/adapters/cli_command.rs | 48 ++- src/cli/args.rs | 69 +++- src/cli/commands/fixture.rs | 372 ++++++++++++++++++++++ src/cli/commands/harness/probe.rs | 17 +- src/cli/commands/mod.rs | 2 + src/cli/mod.rs | 1 + src/cli/run/conversation.rs | 5 +- src/core/fs.rs | 147 ++++++--- src/core/mod.rs | 2 +- src/core/runtime.rs | 243 +++++++++++++- src/pipeline/grade/command_check.rs | 30 +- src/pipeline/grade/command_check/tests.rs | 170 ++++++---- tests/cli/basics.rs | 40 +++ tests/run/command_check.rs | 33 +- tests/run/command_check/matrix.rs | 13 +- tests/run/conversation.rs | 31 +- tests/run/diff_scope.rs | 3 +- tests/run/helpers.rs | 14 + 20 files changed, 1096 insertions(+), 172 deletions(-) create mode 100644 src/cli/commands/fixture.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4a9db57..952e98c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,6 +30,12 @@ jobs: - name: Clippy run: cargo clippy --all-targets --all-features -- -D warnings - name: Test + # Capability-gated tests (the POSIX recipe pipelines, symlink round-trips) + # skip with a printed reason on a host that lacks the tool. This turns + # every such skip into a failure, so CI can never quietly stop covering + # them. Ubuntu runners ship jq, xargs, tr, and wc. + env: + EVAL_MAGIC_REQUIRE_POSIX_TOOLS: 1 run: cargo test --all-targets - name: Build release run: cargo build --release diff --git a/AGENTS.md b/AGENTS.md index d38b5f8..61bd44c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -51,6 +51,28 @@ is the default — most modules use it. Extract only when that module outgrows i do) or to a single `_tests.rs` sibling (as `adapters/guard/guard_denial_tests.rs` does). Extraction is a size decision, not a style preference; don't split a small inline module. +**Spawning a child process from a test.** Use the hidden `__fixture` subcommand, never `sh`, `true`, +`printf`, or a `#!/bin/sh` stub. It exits with a chosen code, emits chosen bytes, writes a chosen +file, or checks a file or variable — see `FixtureArgs` in `src/cli/args.rs`. One invocation parses +the same under `sh -c` and `cmd /C`, which is what keeps `command_check` tests off per-OS command +strings. Build the command with the `fixture` helper (`tests/run/helpers.rs` for integration tests, +the one in `src/pipeline/grade/command_check/tests.rs` for unit tests). Because the fixture is the +binary, `cargo test --lib` alone does not build it — run `cargo test`, or `cargo build` first. + +**Tests are gated on capabilities, not on the OS.** `#[cfg(unix)]` on a test hides it from +compilation and clippy on the other host and hides the coverage gap. Instead, probe for what the +test actually needs and call `report_skip` (`src/core/runtime.rs`), which prints the reason and +returns `true`. Setting `EVAL_MAGIC_REQUIRE_POSIX_TOOLS=1` turns every skip into a failure; CI sets +it, so a runner cannot quietly stop covering something. Two capabilities are gated today: the POSIX +toolchain the shipped dispatch recipes need (`require_posix_toolchain`), and symlink creation, which +Windows allows only under Developer Mode. Where a genuine per-OS difference is the behavior under +test — signals, path separators — branch on `cfg!(windows)` at runtime so both arms still compile +everywhere. + +**Finding a POSIX shell.** Harness `exec_template`s are POSIX command lines, so the dispatch and +probe paths spawn `sh` via `posix_shell()` (`src/core/runtime.rs`) rather than a hardcoded +`/bin/sh`: it searches `PATH`, then a Git for Windows install. Set `EVAL_MAGIC_SH` to override it. + **Where user-facing warnings come from.** Library modules (`pipeline`, `workspace`, `sandbox`, `adapters`) never print. They return warning strings on their result struct — `#[serde(skip)]` when that struct is also a serialized artifact — and the `cli` handler prints them with the `⚠ ` prefix. diff --git a/src/adapters/cli_command.rs b/src/adapters/cli_command.rs index 9d2e5dc..313824e 100644 --- a/src/adapters/cli_command.rs +++ b/src/adapters/cli_command.rs @@ -191,17 +191,38 @@ mod tests { .unwrap(); } - fn run_judge_recipe(cwd: &Path, command_line: &str) -> Output { + /// The tools the rendered judge recipe shells out to. Git for Windows + /// bundles every one of these except `jq`. + const RECIPE_TOOLS: &[&str] = &["jq", "xargs", "tr", "wc"]; + + /// The shell to run a rendered recipe in, or `None` after reporting a skip. + /// + /// These three tests execute shipped POSIX pipeline text, so no portable + /// fixture can stand in for the toolchain — the pipeline *is* the subject. + /// Gating on the capability rather than the OS lets them run on any host + /// that has the tools (including Windows with `jq` installed) and stops them + /// failing inscrutably on a Linux box that happens to lack `jq`. + fn recipe_shell(test: &str) -> Option<&'static Path> { + match crate::core::runtime::require_posix_toolchain(RECIPE_TOOLS) { + Ok(shell) => Some(shell), + Err(missing) => { + crate::core::runtime::report_skip(test, &missing); + None + } + } + } + + fn run_judge_recipe(shell: &Path, cwd: &Path, command_line: &str) -> Output { let recipe = render_judge_dispatch_recipe(command_line, "--model", "judge"); - let shell = recipe + let program = recipe .split_once("```bash\n") .unwrap() .1 .strip_suffix("\n```") .unwrap(); - Command::new("/bin/sh") + Command::new(shell) .arg("-c") - .arg(shell) + .arg(program) .current_dir(cwd) .env("JOBS", "1") .output() @@ -335,6 +356,10 @@ mod tests { #[test] fn judge_recipe_reports_partial_completion_and_exits_nonzero() { + let Some(shell) = recipe_shell("judge_recipe_reports_partial_completion_and_exits_nonzero") + else { + return; + }; let tmp = tempfile::TempDir::new().unwrap(); let responses_dir = tmp.path().join("judge responses"); fs::create_dir_all(&responses_dir).unwrap(); @@ -343,7 +368,7 @@ mod tests { fs::write(&existing_response, "{}\n").unwrap(); write_judge_tasks(tmp.path(), &[&existing_response, &missing_response]); - let output = run_judge_recipe(tmp.path(), " true $model_arg \\"); + let output = run_judge_recipe(shell, tmp.path(), " true $model_arg \\"); assert!(!output.status.success(), "{output:?}"); assert_eq!( @@ -354,6 +379,11 @@ mod tests { #[test] fn judge_recipe_reports_complete_resumed_batch_and_exits_zero() { + let Some(shell) = + recipe_shell("judge_recipe_reports_complete_resumed_batch_and_exits_zero") + else { + return; + }; let tmp = tempfile::TempDir::new().unwrap(); let first_response = tmp.path().join("first.json"); let second_response = tmp.path().join("second.json"); @@ -361,7 +391,7 @@ mod tests { fs::write(&second_response, "second\n").unwrap(); write_judge_tasks(tmp.path(), &[&first_response, &second_response]); - let output = run_judge_recipe(tmp.path(), " false $model_arg \\"); + let output = run_judge_recipe(shell, tmp.path(), " false $model_arg \\"); assert!(output.status.success(), "{output:?}"); assert_eq!( @@ -374,6 +404,11 @@ mod tests { #[test] fn judge_recipe_preserves_dispatch_failure_after_response_is_written() { + let Some(shell) = + recipe_shell("judge_recipe_preserves_dispatch_failure_after_response_is_written") + else { + return; + }; let tmp = tempfile::TempDir::new().unwrap(); let response = tmp.path().join("response.json"); let failing_judge = tmp.path().join("failing-judge"); @@ -385,6 +420,7 @@ mod tests { write_judge_tasks(tmp.path(), &[&response]); let output = run_judge_recipe( + shell, tmp.path(), " sh ./failing-judge \"$response_path\" $model_arg \\", ); diff --git a/src/cli/args.rs b/src/cli/args.rs index d9afd3c..ef5e8c1 100644 --- a/src/cli/args.rs +++ b/src/cli/args.rs @@ -4,6 +4,8 @@ //! Flags are intentionally permissive (mostly optional); each handler tightens //! them to what it actually requires (see the handlers in [`super::commands`]). +use std::path::PathBuf; + use clap::{Args, Parser, Subcommand}; /// Run skill evals — measure whether an agent skill actually shifts behavior. @@ -203,7 +205,7 @@ pub(crate) enum HarnessCommands { /// /// With `--probe`, and only after every static check passes, also exercises /// the descriptor end-to-end: renders `dispatch.exec_template` with a - /// trivial prompt in a throwaway temp dir, runs it via `/bin/sh -c` from + /// trivial prompt in a throwaway temp dir, runs it via `sh -c` from /// the temp `eval_root`, and verifies `outputs/final-message.md` is /// recovered (non-empty). It additionally render-only-validates /// `parallel_command_template` and `judge_command_template` for @@ -797,6 +799,12 @@ pub(crate) enum Commands { /// `/.agents/skills/.slow-powers-eval-guard.json`. marker: Option, }, + /// Internal test fixture. A predictable child process for the suite to + /// spawn — one that exits with a chosen code, emits chosen bytes, or writes + /// a chosen file — so tests never reach for `sh`, `true`, or `printf`, none + /// of which exist under `cmd.exe`. Not for users; hidden from help. + #[command(hide = true, name = "__fixture")] + Fixture(FixtureArgs), /// Internal generic PreToolUse hook entry point. Invoked by the installed /// write-guard hook as `eval-magic guard-hook --harness `, /// not by users; hidden from help. `guard` / `guard-codex` are frozen @@ -812,3 +820,62 @@ pub(crate) enum Commands { marker: Option, }, } + +/// Flags for the hidden `__fixture` subcommand. +/// +/// Each flag stands in for a POSIX construct a test would otherwise spawn. +/// The fixture emits one output string built from `--pad`, then `--text`, then +/// `--echo-env` (in that order, joined by `--separator`), sends it to stdout and +/// optionally to `--write`/`--append`, and exits with `--exit` — or `1` when any +/// `--require-*` check failed. +/// +/// Requirements never suppress the effects. The matrix suites read their append +/// log to prove every cell ran, including the cells expected to fail. +#[derive(Debug, Args, Default)] +pub struct FixtureArgs { + /// Exit code to leave with when every requirement holds. + #[arg(long, default_value_t = 0)] + pub exit: i32, + /// Literal output fragment; repeatable. + #[arg(long)] + pub text: Vec, + /// Emit the value of this environment variable; repeatable. + #[arg(long = "echo-env")] + pub echo_env: Vec, + /// Value emitted by `--echo-env` for a variable that is not set. Without it + /// an unset variable contributes an empty fragment. + #[arg(long)] + pub default: Option, + /// Emit this many `x` bytes ahead of the other fragments, for exercising + /// output larger than the diagnostic truncation limit. + #[arg(long)] + pub pad: Option, + /// Joins the fragments. Empty by default. + #[arg(long, default_value = "")] + pub separator: String, + /// Terminate the emitted output with a newline. + #[arg(long)] + pub newline: bool, + /// Write this text to stderr. + #[arg(long = "stderr")] + pub stderr: Option, + /// Also write the emitted output to this path, replacing it. + #[arg(long)] + pub write: Option, + /// Also append the emitted output to this path. + #[arg(long)] + pub append: Option, + /// Fail unless this path exists; repeatable. + #[arg(long = "require-file")] + pub require_file: Vec, + /// Fail unless `` holds exactly ``. + #[arg(long = "require-file-text", num_args = 2, value_names = ["PATH", "TEXT"])] + pub require_file_text: Vec, + /// Fail unless the variable is set (`NAME`) or holds a value (`NAME=VALUE`); + /// repeatable. + #[arg(long = "require-env")] + pub require_env: Vec, + /// Fail unless the two paths hold identical bytes. + #[arg(long = "files-equal", num_args = 2, value_names = ["LEFT", "RIGHT"])] + pub files_equal: Option>, +} diff --git a/src/cli/commands/fixture.rs b/src/cli/commands/fixture.rs new file mode 100644 index 0000000..377fb3d --- /dev/null +++ b/src/cli/commands/fixture.rs @@ -0,0 +1,372 @@ +//! The hidden `__fixture` subcommand: a predictable child process for the test +//! suite to spawn. +//! +//! Tests that exercise `command_check` grading need a program that exits with a +//! chosen status, emits chosen bytes, or writes a chosen file. Reaching for +//! `sh`, `true`, or `printf` ties those tests to POSIX, and the `cmd.exe` +//! equivalents are not equivalent — `echo x>>f` appends CRLF, and +//! `echo|set /p=` cannot round-trip a value. One fixture invoked the same way +//! under both shells removes the dialect problem entirely. + +use std::fs::{self, OpenOptions}; +use std::io::{self, Write}; +use std::path::Path; +use std::process; + +use anyhow::Context; + +use crate::cli::args::FixtureArgs; + +/// Run the fixture and leave the process with its exit code. Streams are +/// flushed explicitly: `process::exit` does not run destructors, so buffered +/// output would otherwise be dropped. +pub(crate) fn run_fixture(args: FixtureArgs) -> anyhow::Result<()> { + let mut out = io::stdout(); + let mut err = io::stderr(); + let code = execute_fixture(&args, &mut out, &mut err)?; + out.flush()?; + err.flush()?; + process::exit(code) +} + +/// Perform the fixture's effects against `out`/`err`, returning the exit code. +/// Split from [`run_fixture`] so tests can drive it with in-memory buffers. +fn execute_fixture( + args: &FixtureArgs, + out: &mut impl Write, + err: &mut impl Write, +) -> anyhow::Result { + let satisfied = requirements_met(args)?; + let emitted = emitted_output(args); + + out.write_all(emitted.as_bytes())?; + if let Some(text) = &args.stderr { + err.write_all(text.as_bytes())?; + } + if let Some(path) = &args.write { + write_to(path, &emitted, false)?; + } + if let Some(path) = &args.append { + write_to(path, &emitted, true)?; + } + + Ok(if satisfied { args.exit } else { 1 }) +} + +/// The single output string: `--pad`, then each `--text`, then each +/// `--echo-env`, joined by `--separator`. No site needs the two fragment kinds +/// interleaved, so a fixed order keeps the result independent of argv order — +/// which `clap` does not preserve across distinct flags. +fn emitted_output(args: &FixtureArgs) -> String { + let mut fragments = Vec::new(); + if let Some(count) = args.pad { + fragments.push("x".repeat(count)); + } + fragments.extend(args.text.iter().cloned()); + fragments.extend(args.echo_env.iter().map(|name| { + std::env::var(name).unwrap_or_else(|_| args.default.clone().unwrap_or_default()) + })); + let mut emitted = fragments.join(&args.separator); + if args.newline { + emitted.push('\n'); + } + emitted +} + +/// Whether every `--require-*` check holds. Errors are reserved for the fixture +/// being unusable (an unreadable path, a malformed flag pairing); an unmet +/// requirement is an ordinary `false`, because that is the behavior under test. +fn requirements_met(args: &FixtureArgs) -> anyhow::Result { + for path in &args.require_file { + if !path.is_file() { + return Ok(false); + } + } + for pair in args.require_file_text.chunks(2) { + let [path, expected] = pair else { + anyhow::bail!("--require-file-text takes "); + }; + match fs::read_to_string(path) { + Ok(actual) if &actual == expected => {} + _ => return Ok(false), + } + } + for spec in &args.require_env { + let met = match spec.split_once('=') { + Some((name, expected)) => std::env::var(name).is_ok_and(|value| value == expected), + None => std::env::var(spec).is_ok_and(|value| !value.is_empty()), + }; + if !met { + return Ok(false); + } + } + if let Some(paths) = &args.files_equal { + let [left, right] = paths.as_slice() else { + anyhow::bail!("--files-equal takes "); + }; + match (fs::read(left), fs::read(right)) { + (Ok(left), Ok(right)) if left == right => {} + _ => return Ok(false), + } + } + Ok(true) +} + +/// Write `contents` to `path`, creating missing parents so a fixture can target +/// a directory the test has not made yet. +fn write_to(path: &Path, contents: &str, append: bool) -> anyhow::Result<()> { + if let Some(parent) = path + .parent() + .filter(|parent| !parent.as_os_str().is_empty()) + { + fs::create_dir_all(parent) + .with_context(|| format!("creating fixture output dir {}", parent.display()))?; + } + OpenOptions::new() + .write(true) + .create(true) + .append(append) + .truncate(!append) + .open(path) + .and_then(|mut file| file.write_all(contents.as_bytes())) + .with_context(|| format!("writing fixture output {}", path.display())) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + + fn args() -> FixtureArgs { + FixtureArgs::default() + } + + /// Runs the fixture against in-memory streams and returns + /// `(exit_code, stdout, stderr)`. + fn run(args: &FixtureArgs) -> (i32, String, String) { + let mut out = Vec::new(); + let mut err = Vec::new(); + let code = execute_fixture(args, &mut out, &mut err).unwrap(); + ( + code, + String::from_utf8(out).unwrap(), + String::from_utf8(err).unwrap(), + ) + } + + #[test] + fn emits_nothing_and_exits_zero_by_default() { + assert_eq!(run(&args()), (0, String::new(), String::new())); + } + + #[test] + fn exit_code_is_reported_when_every_requirement_holds() { + let (code, _, _) = run(&FixtureArgs { exit: 3, ..args() }); + assert_eq!(code, 3); + } + + #[test] + fn text_and_stderr_fragments_reach_their_own_streams() { + let (code, out, err) = run(&FixtureArgs { + text: vec!["hello world".into()], + stderr: Some("diagnostic".into()), + ..args() + }); + assert_eq!( + (code, out, err), + (0, "hello world".into(), "diagnostic".into()) + ); + } + + /// No trailing newline unless asked: `command-runs.txt` is compared byte for + /// byte against `"x"`, so an implicit newline would break it. + #[test] + fn fragments_are_joined_by_the_separator_and_newline_is_opt_in() { + let (_, out, _) = run(&FixtureArgs { + echo_env: vec!["EVAL_MAGIC_UNSET_A".into(), "EVAL_MAGIC_UNSET_B".into()], + default: Some("v".into()), + separator: "|".into(), + ..args() + }); + assert_eq!(out, "v|v"); + + let (_, terminated, _) = run(&FixtureArgs { + text: vec!["x".into()], + newline: true, + ..args() + }); + assert_eq!(terminated, "x\n"); + } + + #[test] + fn pad_emits_its_byte_count_ahead_of_the_other_fragments() { + let (_, out, _) = run(&FixtureArgs { + pad: Some(3000), + text: vec!["TAIL".into()], + ..args() + }); + assert_eq!(out.len(), 3004); + assert!(out.ends_with("TAIL")); + assert!(out.starts_with("xxx")); + } + + #[test] + fn echo_env_reads_the_real_environment_and_falls_back_to_the_default() { + let path = std::env::var("PATH").unwrap(); + let (_, present, _) = run(&FixtureArgs { + echo_env: vec!["PATH".into()], + ..args() + }); + assert_eq!(present, path); + + let (_, absent, _) = run(&FixtureArgs { + echo_env: vec!["EVAL_MAGIC_DEFINITELY_UNSET".into()], + default: Some("unset".into()), + ..args() + }); + assert_eq!(absent, "unset"); + } + + #[test] + fn write_replaces_and_append_extends_the_target_file() { + let tmp = tempfile::TempDir::new().unwrap(); + let target = tmp.path().join("nested").join("out.txt"); + + run(&FixtureArgs { + text: vec!["first".into()], + write: Some(target.clone()), + ..args() + }); + assert_eq!(fs::read_to_string(&target).unwrap(), "first"); + + run(&FixtureArgs { + text: vec!["second".into()], + write: Some(target.clone()), + ..args() + }); + assert_eq!(fs::read_to_string(&target).unwrap(), "second"); + + run(&FixtureArgs { + text: vec!["x".into()], + append: Some(target.clone()), + ..args() + }); + run(&FixtureArgs { + text: vec!["x".into()], + append: Some(target.clone()), + ..args() + }); + assert_eq!(fs::read_to_string(&target).unwrap(), "secondxx"); + } + + #[test] + fn require_env_checks_presence_for_a_bare_name_and_equality_for_a_pair() { + let path = std::env::var("PATH").unwrap(); + for (spec, expected) in [ + ("PATH".to_string(), 0), + ("EVAL_MAGIC_DEFINITELY_UNSET".to_string(), 1), + (format!("PATH={path}"), 0), + ("PATH=not-the-real-path".to_string(), 1), + ] { + let (code, _, _) = run(&FixtureArgs { + require_env: vec![spec.clone()], + ..args() + }); + assert_eq!(code, expected, "{spec}"); + } + } + + #[test] + fn require_file_checks_existence() { + let tmp = tempfile::TempDir::new().unwrap(); + let present = tmp.path().join("present.txt"); + fs::write(&present, "body").unwrap(); + + let (ok, _, _) = run(&FixtureArgs { + require_file: vec![present], + ..args() + }); + assert_eq!(ok, 0); + + let (missing, _, _) = run(&FixtureArgs { + require_file: vec![tmp.path().join("absent.txt")], + ..args() + }); + assert_eq!(missing, 1); + } + + #[test] + fn require_file_text_compares_contents_to_a_literal() { + let tmp = tempfile::TempDir::new().unwrap(); + let state = tmp.path().join("state.txt"); + fs::write(&state, "ready").unwrap(); + + let (ok, _, _) = run(&FixtureArgs { + require_file_text: vec![state.to_string_lossy().into_owned(), "ready".into()], + ..args() + }); + assert_eq!(ok, 0); + + let (mismatch, _, _) = run(&FixtureArgs { + require_file_text: vec![state.to_string_lossy().into_owned(), "waiting".into()], + ..args() + }); + assert_eq!(mismatch, 1); + } + + #[test] + fn files_equal_compares_two_files_byte_for_byte() { + let tmp = tempfile::TempDir::new().unwrap(); + let answer = tmp.path().join("answer.txt"); + let expected = tmp.path().join("expected.txt"); + fs::write(&answer, "same").unwrap(); + fs::write(&expected, "same").unwrap(); + + let (matching, _, _) = run(&FixtureArgs { + files_equal: Some(vec![answer.clone(), expected.clone()]), + ..args() + }); + assert_eq!(matching, 0); + + fs::write(&expected, "different").unwrap(); + let (differing, _, _) = run(&FixtureArgs { + files_equal: Some(vec![answer, expected]), + ..args() + }); + assert_eq!(differing, 1); + } + + /// A failed requirement must not suppress the effects: the matrix suite + /// reads `matrix-runs.txt` to prove every cell ran, including the cells that + /// are expected to fail. + #[test] + fn a_failed_requirement_still_performs_the_effects() { + let tmp = tempfile::TempDir::new().unwrap(); + let log = tmp.path().join("matrix-runs.txt"); + + let (code, out, _) = run(&FixtureArgs { + text: vec!["ran".into()], + newline: true, + append: Some(log.clone()), + require_env: vec!["EVAL_MAGIC_DEFINITELY_UNSET".into()], + exit: 0, + ..args() + }); + + assert_eq!(code, 1); + assert_eq!(out, "ran\n"); + assert_eq!(fs::read_to_string(&log).unwrap(), "ran\n"); + } + + /// A failed requirement outranks an explicit `--exit 0`, so a cell cannot + /// report success while its precondition is unmet. + #[test] + fn a_failed_requirement_overrides_the_requested_exit_code() { + let (code, _, _) = run(&FixtureArgs { + exit: 0, + require_env: vec!["EVAL_MAGIC_DEFINITELY_UNSET=value".into()], + ..args() + }); + assert_eq!(code, 1); + } +} diff --git a/src/cli/commands/harness/probe.rs b/src/cli/commands/harness/probe.rs index d650d89..042960f 100644 --- a/src/cli/commands/harness/probe.rs +++ b/src/cli/commands/harness/probe.rs @@ -18,6 +18,7 @@ use crate::adapters::cli_command::{ render_agent_dispatch_command, render_cli_model_arg, shell_quote_arg, }; use crate::adapters::descriptor::{HarnessDescriptor, subst}; +use crate::core::posix_shell; /// Options carried from the parsed `--probe` flags into [`run_probe`]. #[derive(Debug, Clone, Copy)] @@ -86,16 +87,20 @@ fn render_probe_exec( ) } -/// Execute `command` via `/bin/sh -c` with `cwd` as the subprocess working -/// directory, killing the child if it exceeds `timeout`. The child's stdin is -/// `null`: the parent reads the `y/N` confirm on its own stdin and never wants -/// the dispatched agent CLI to consume it. +/// Execute `command` via the resolved POSIX shell with `cwd` as the subprocess +/// working directory, killing the child if it exceeds `timeout`. The child's +/// stdin is `null`: the parent reads the `y/N` confirm on its own stdin and +/// never wants the dispatched agent CLI to consume it. +/// +/// Only the direct child is killed on timeout, not its process group — a shell +/// that has already forked leaves the grandchild running until it exits. fn execute_with_timeout( command: &str, cwd: &Path, timeout: Duration, ) -> Result { - let mut child = Command::new("/bin/sh") + let shell = posix_shell().map_err(|message| ProbeError::SpawnFailed(message.to_string()))?; + let mut child = Command::new(shell) .arg("-c") .arg(command) .current_dir(cwd) @@ -165,7 +170,7 @@ const RENDER_STAND_INS: [(&str, &str); 2] = [ /// The live dispatch probe. Renders `dispatch.exec_template` with a trivial /// prompt in a throwaway temp dir, asks for confirmation, runs it under a -/// timeout through `/bin/sh -c` from that dir, then verifies the final-message +/// timeout through the resolved POSIX shell from that dir, then verifies the final-message /// recovery contract. Also render-only-validates `parallel_command_template` /// and `judge_command_template` for placeholder-shape errors. Invokes the real /// harness CLI and is opt-in; never part of standard CI checks. diff --git a/src/cli/commands/mod.rs b/src/cli/commands/mod.rs index 64d5f5b..46c0514 100644 --- a/src/cli/commands/mod.rs +++ b/src/cli/commands/mod.rs @@ -5,6 +5,7 @@ //! [`super`] (`crate::cli`). mod docs; +mod fixture; mod guard; mod harness; mod init; @@ -14,6 +15,7 @@ mod validate; mod workspace; pub(crate) use docs::run_docs; +pub(crate) use fixture::run_fixture; pub(crate) use guard::{run_guard, run_guard_codex, run_guard_hook, run_teardown_guard}; pub(crate) use harness::run_harness; pub(crate) use init::run_init; diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 253f00e..f908435 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -112,6 +112,7 @@ fn dispatch(command: Option, harness_file: Option<&str>) -> anyhow::Re Commands::Guard { marker } => run_guard(marker), Commands::GuardCodex { marker } => run_guard_codex(marker), Commands::GuardHook { harness, marker } => run_guard_hook(&harness, marker), + Commands::Fixture(args) => run_fixture(args), Commands::RecordRuns(args) => run_record_runs(args), Commands::FillTranscripts(args) => run_fill_transcripts(args), Commands::DetectStrayWrites(args) => run_detect_stray_writes(args), diff --git a/src/cli/run/conversation.rs b/src/cli/run/conversation.rs index eecf4a7..3d6472b 100644 --- a/src/cli/run/conversation.rs +++ b/src/cli/run/conversation.rs @@ -21,7 +21,7 @@ use crate::adapters::harness::HarnessAdapter; use crate::adapters::transcript::{TranscriptEvent, TranscriptSummary}; use crate::core::{ ConversationEvent, ConversationRecord, ConversationStatus, ConversationStopReason, DeliverWhen, - ScriptedTurn, validate_agent_environment_entry, + ScriptedTurn, posix_shell, validate_agent_environment_entry, }; use crate::validation::{SchemaName, validate_against_schema}; @@ -373,7 +373,8 @@ fn execute_round( ) -> anyhow::Result<()> { fs::create_dir_all(outputs_dir) .with_context(|| format!("failed to create turn {round} outputs"))?; - let status = Command::new("/bin/sh") + let shell = posix_shell().map_err(|message| anyhow!("{message}"))?; + let status = Command::new(shell) .arg("-c") .arg(command) .current_dir(eval_root) diff --git a/src/core/fs.rs b/src/core/fs.rs index 9b439a6..24c0fda 100644 --- a/src/core/fs.rs +++ b/src/core/fs.rs @@ -96,14 +96,8 @@ pub fn copy_entry(source: &Path, destination: &Path) -> io::Result<()> { if metadata.file_type().is_symlink() { create_parent(destination)?; let target = fs::read_link(source)?; - #[cfg(unix)] - std::os::unix::fs::symlink(target, destination)?; - #[cfg(windows)] - if source.metadata().is_ok_and(|metadata| metadata.is_dir()) { - std::os::windows::fs::symlink_dir(target, destination)?; - } else { - std::os::windows::fs::symlink_file(target, destination)?; - } + let to_directory = source.metadata().is_ok_and(|metadata| metadata.is_dir()); + create_symlink(&target, destination, to_directory)?; } else if metadata.is_dir() { fs::create_dir_all(destination)?; for entry in fs::read_dir(source)? { @@ -141,6 +135,26 @@ pub fn copy_entry_materialized(source: &Path, destination: &Path) -> io::Result< Ok(()) } +/// Create a symlink at `link` pointing at `target`. +/// +/// `to_directory` is consulted only on Windows, which has separate file and +/// directory link kinds; POSIX has one. Creating a symlink there also needs +/// either Developer Mode or elevation, so this can fail for reasons that have +/// nothing to do with the paths involved. +pub(crate) fn create_symlink(target: &Path, link: &Path, to_directory: bool) -> io::Result<()> { + #[cfg(unix)] + { + let _ = to_directory; + std::os::unix::fs::symlink(target, link) + } + #[cfg(windows)] + if to_directory { + std::os::windows::fs::symlink_dir(target, link) + } else { + std::os::windows::fs::symlink_file(target, link) + } +} + /// Create `path`'s parent directory chain, when it has one. fn create_parent(path: &Path) -> io::Result<()> { match path.parent() { @@ -155,6 +169,31 @@ mod tests { use serde_json::json; use tempfile::TempDir; + /// Whether this host lets the test process create a symlink at all. + /// + /// A capability, not a platform: Windows can create symlinks, but only under + /// Developer Mode or elevation. Probing beats gating on the OS — the tests + /// then run wherever the capability exists instead of wherever the OS name + /// matches. + fn symlinks_available(scratch: &Path) -> bool { + let target = scratch.join("probe-target.txt"); + let link = scratch.join("probe-link.txt"); + if fs::write(&target, "probe").is_err() { + return false; + } + create_symlink(&target, &link, false).is_ok() + } + + /// Report a skipped symlink test, deferring to the shared skip policy so the + /// enforcement switch is decided in exactly one place. + fn skip_without_symlinks(scratch: &Path, test: &str) -> bool { + !symlinks_available(scratch) + && crate::core::runtime::report_skip( + test, + "this host does not permit symlink creation (Windows needs Developer Mode)", + ) + } + /// A path already in wire form passes through unchanged on every host — /// the fixtures and goldens that spell paths POSIX-style stay byte-stable. #[test] @@ -165,40 +204,39 @@ mod tests { ); } - /// `Path::join` on a POSIX-rooted base emits a Windows separator, so a - /// manifest entry would otherwise read `/work/cond\run.json`. A verbatim - /// `\\?\` prefix is stripped too — it is an OS-level escape hatch, not - /// something an agent should ever be handed. - #[cfg(windows)] - #[test] - fn artifact_path_rewrites_windows_separators_and_strips_verbatim_prefixes() { - assert_eq!( - artifact_path(Path::new(r"/work/cond\run.json")), - "/work/cond/run.json" - ); - assert_eq!( - artifact_path(Path::new(r"C:\work\cond\run.json")), - "C:/work/cond/run.json" - ); - assert_eq!( - artifact_path(Path::new(r"\\?\C:\work\run.json")), - "C:/work/run.json" - ); - assert_eq!( - artifact_path(Path::new(r"\\?\UNC\host\share\run.json")), - "//host/share/run.json" - ); - } - - /// The rewrite is Windows-only: a POSIX filename may legally contain a - /// literal backslash, and rewriting it would name a different file. - #[cfg(unix)] + /// A backslash means different things per host, so `artifact_path` does too, + /// and both halves belong in one place. + /// + /// On Windows it is a separator: `Path::join` on a POSIX-rooted base emits + /// one, so a manifest entry would otherwise read `/work/cond\run.json`. A + /// verbatim `\\?\` prefix is stripped as well — an OS-level escape hatch, not + /// something an agent should ever be handed. On POSIX a backslash is a legal + /// filename character, so rewriting it would name a different file. #[test] - fn artifact_path_preserves_a_literal_backslash_in_a_posix_filename() { - assert_eq!( - artifact_path(Path::new(r"/work/od\dity.json")), - r"/work/od\dity.json" - ); + fn artifact_path_applies_host_separator_rules() { + if cfg!(windows) { + assert_eq!( + artifact_path(Path::new(r"/work/cond\run.json")), + "/work/cond/run.json" + ); + assert_eq!( + artifact_path(Path::new(r"C:\work\cond\run.json")), + "C:/work/cond/run.json" + ); + assert_eq!( + artifact_path(Path::new(r"\\?\C:\work\run.json")), + "C:/work/run.json" + ); + assert_eq!( + artifact_path(Path::new(r"\\?\UNC\host\share\run.json")), + "//host/share/run.json" + ); + } else { + assert_eq!( + artifact_path(Path::new(r"/work/od\dity.json")), + r"/work/od\dity.json" + ); + } } /// Comparison normalization is unconditional, unlike [`artifact_path`]: its @@ -301,14 +339,19 @@ mod tests { /// be recreated as a link, not resolved into its target's content. Following /// it would inline whatever the link pointed at — possibly from outside the /// tree being copied. - #[cfg(unix)] #[test] fn copy_entry_recreates_symlinks_instead_of_following_them() { let tmp = TempDir::new().unwrap(); + if skip_without_symlinks( + tmp.path(), + "copy_entry_recreates_symlinks_instead_of_following_them", + ) { + return; + } let target = tmp.path().join("target.txt"); fs::write(&target, "target contents").unwrap(); let link = tmp.path().join("link.txt"); - std::os::unix::fs::symlink(&target, &link).unwrap(); + create_symlink(&target, &link, false).unwrap(); let destination = tmp.path().join("copied-link.txt"); copy_entry(&link, &destination).unwrap(); @@ -325,14 +368,19 @@ mod tests { /// A symlink nested inside a copied directory survives too — the recursion /// arm must route back through the symlink arm, not through `fs::copy`. - #[cfg(unix)] #[test] fn copy_entry_preserves_symlinks_nested_inside_a_directory() { let tmp = TempDir::new().unwrap(); + if skip_without_symlinks( + tmp.path(), + "copy_entry_preserves_symlinks_nested_inside_a_directory", + ) { + return; + } let source = tmp.path().join("tree"); fs::create_dir_all(&source).unwrap(); fs::write(source.join("real.txt"), "real").unwrap(); - std::os::unix::fs::symlink("real.txt", source.join("alias.txt")).unwrap(); + create_symlink(Path::new("real.txt"), &source.join("alias.txt"), false).unwrap(); let destination = tmp.path().join("copied"); copy_entry(&source, &destination).unwrap(); @@ -354,14 +402,19 @@ mod tests { /// The counterpart semantic: a snapshot must freeze content, so a symlink is /// resolved and its target's bytes are written. Preserving the link would /// make the "frozen" copy track whatever the link points at later. - #[cfg(unix)] #[test] fn copy_entry_materialized_resolves_symlinks_into_their_content() { let tmp = TempDir::new().unwrap(); + if skip_without_symlinks( + tmp.path(), + "copy_entry_materialized_resolves_symlinks_into_their_content", + ) { + return; + } let source = tmp.path().join("tree"); fs::create_dir_all(&source).unwrap(); fs::write(source.join("real.txt"), "frozen").unwrap(); - std::os::unix::fs::symlink("real.txt", source.join("alias.txt")).unwrap(); + create_symlink(Path::new("real.txt"), &source.join("alias.txt"), false).unwrap(); let destination = tmp.path().join("copied"); copy_entry_materialized(&source, &destination).unwrap(); diff --git a/src/core/mod.rs b/src/core/mod.rs index 744a1f5..b236e6c 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -17,7 +17,7 @@ pub mod types; pub use capabilities::HarnessRunCapabilities; pub use context::{ContextError, DetectInput, Harness, RunContext, detect_run_context}; pub(crate) use runtime::{ - GIT_ROUTING_ENV_VARS, clear_git_environment, validate_agent_environment_entry, + GIT_ROUTING_ENV_VARS, clear_git_environment, posix_shell, validate_agent_environment_entry, }; pub use runtime::{GitOutput, run_git}; pub use types::*; diff --git a/src/core/runtime.rs b/src/core/runtime.rs index 790d389..2a23b5e 100644 --- a/src/core/runtime.rs +++ b/src/core/runtime.rs @@ -5,8 +5,10 @@ //! `clap` owns argument parsing, and the `error: ` + exit(1) contract //! lives in `src/main.rs`. -use std::path::Path; +use std::ffi::OsStr; +use std::path::{Path, PathBuf}; use std::process::Command; +use std::sync::OnceLock; /// Inherited Git routing variables that can redirect repository discovery or /// object/index access away from a command's current working directory. @@ -91,6 +93,145 @@ pub fn run_git(args: &[&str], cwd: &Path) -> GitOutput { } } +/// What to tell an operator who has no POSIX shell. Harness `exec_template`s +/// ship as POSIX command lines (`/mingw64/libexec/git-core` — hence three levels up). +/// Always spelled `sh.exe`: this layout only exists on Windows, and pinning the +/// name keeps the function testable on every host. +fn git_shell_candidates(exec_path: &Path) -> Vec { + let Some(root) = exec_path.ancestors().nth(3) else { + return Vec::new(); + }; + if root.as_os_str().is_empty() { + return Vec::new(); + } + vec![ + root.join("bin").join("sh.exe"), + root.join("usr").join("bin").join("sh.exe"), + ] +} + +/// First executable named `name` on `PATH`, or `None`. +fn find_on_path(name: &str) -> Option { + let file_name = if cfg!(windows) { + format!("{name}.exe") + } else { + name.to_string() + }; + std::env::split_paths(&std::env::var_os("PATH")?) + .map(|directory| directory.join(&file_name)) + .find(|candidate| candidate.is_file()) +} + +/// Where to look for `sh` on Windows once `PATH` has come up empty. Git for +/// Windows bundles one, but its default installer only puts `Git\cmd` on +/// `PATH`, so the shell has to be located through the install root instead. +fn windows_shell_candidates() -> Vec { + let mut candidates = Vec::new(); + let git = run_git(&["--exec-path"], Path::new(".")); + if git.status == Some(0) { + let exec_path = String::from_utf8_lossy(&git.stdout).trim().to_string(); + if !exec_path.is_empty() { + candidates.extend(git_shell_candidates(Path::new(&exec_path))); + } + } + candidates.push(PathBuf::from(r"C:\Program Files\Git\bin\sh.exe")); + candidates.push(PathBuf::from(r"C:\Program Files\Git\usr\bin\sh.exe")); + candidates +} + +/// Locate a POSIX shell. `override_path` carries the operator's `EVAL_MAGIC_SH` +/// value and is passed in rather than read here so tests can exercise it +/// without mutating process environment. +/// +/// Only ever searches for `sh`. On Windows `C:\Windows\System32\bash.exe` is +/// the WSL launcher, which resolves a different filesystem namespace — every +/// Windows path handed to it would name the wrong file. +fn discover_posix_shell(override_path: Option<&OsStr>) -> Result { + if let Some(value) = override_path { + let path = PathBuf::from(value); + if path.is_file() { + return Ok(path); + } + // A configured-but-wrong override is an error, not a fallback: silently + // discovering a different shell would hide the misconfiguration. + return Err(format!( + "EVAL_MAGIC_SH points at {}, which is not a file. {SHELL_SETUP_GUIDANCE}", + path.display() + )); + } + if let Some(found) = find_on_path("sh") { + return Ok(found); + } + if cfg!(windows) + && let Some(found) = windows_shell_candidates() + .into_iter() + .find(|candidate| candidate.is_file()) + { + return Ok(found); + } + let posix_default = PathBuf::from("/bin/sh"); + if posix_default.is_file() { + return Ok(posix_default); + } + Err(format!("no POSIX shell found. {SHELL_SETUP_GUIDANCE}")) +} + +/// The resolved POSIX shell, discovered once per process. Callers spawn this +/// instead of a hardcoded `/bin/sh`, which does not exist on Windows. +pub(crate) fn posix_shell() -> Result<&'static Path, &'static str> { + static SHELL: OnceLock> = OnceLock::new(); + match SHELL.get_or_init(|| discover_posix_shell(std::env::var_os("EVAL_MAGIC_SH").as_deref())) { + Ok(shell) => Ok(shell.as_path()), + Err(message) => Err(message.as_str()), + } +} + +/// Announce that `test` is being skipped, `reason` explaining what the host +/// lacks. Returns `true` so a caller can `return` on it. +/// +/// A skipped test still reports as passing, so the skip has to be impossible to +/// lose track of: setting `EVAL_MAGIC_REQUIRE_POSIX_TOOLS` (CI does) turns every +/// skip into a failure. One place owns that policy so each capability check does +/// not re-decide it. +#[cfg(test)] +pub(crate) fn report_skip(test: &str, reason: &str) -> bool { + assert!( + std::env::var_os("EVAL_MAGIC_REQUIRE_POSIX_TOOLS").is_none(), + "{test} was skipped for a missing capability: {reason}. \ + Provide it, or unset EVAL_MAGIC_REQUIRE_POSIX_TOOLS to allow skipping" + ); + eprintln!("skipping {test}: {reason}"); + true +} + +/// The resolved shell, once every tool in `tools` is reachable from inside it. +/// +/// The shipped parallel and judge recipes are POSIX pipelines over `jq`, +/// `xargs`, `tr`, and `wc`, so a test that executes one needs all of them. They +/// are checked through the shell rather than on the host `PATH` because that is +/// where the recipe will look: Git for Windows carries its own `/usr/bin`. +#[cfg(test)] +pub(crate) fn require_posix_toolchain(tools: &[&str]) -> Result<&'static Path, String> { + let shell = posix_shell().map_err(str::to_string)?; + for tool in tools { + let found = Command::new(shell) + .arg("-c") + .arg(format!("command -v {tool}")) + .output() + .is_ok_and(|output| output.status.success()); + if !found { + return Err(format!("{tool} is not on the PATH of {}", shell.display())); + } + } + Ok(shell) +} + #[cfg(test)] mod tests { use super::*; @@ -128,7 +269,105 @@ mod tests { "/nonexistent-dir-for-rungit-test".as_ref(), ); assert_eq!(res.status, None); - assert!(String::from_utf8_lossy(&res.stderr).contains("No such file or directory")); + assert!(res.stdout.is_empty()); + // Deliberately not matched against an errno spelling: the OS wording + // differs per platform ("No such file or directory" vs "The system + // cannot find the path specified"), and the contract is a readable + // reason, not any particular one. + let reason = String::from_utf8_lossy(&res.stderr); + assert!( + !reason.trim().is_empty(), + "spawn failure reported no reason" + ); + } + + /// The Git for Windows layout: `git --exec-path` points at + /// `/mingw64/libexec/git-core`, so the shell sits three levels up. + #[test] + fn git_shell_candidates_walk_up_from_the_git_core_exec_path() { + let root = Path::new("/opt/Git"); + assert_eq!( + git_shell_candidates(&root.join("mingw64").join("libexec").join("git-core")), + vec![ + root.join("bin").join("sh.exe"), + root.join("usr").join("bin").join("sh.exe"), + ] + ); + } + + /// An exec path too shallow to contain a Git root yields no candidates + /// rather than walking off the top into `/`. + #[test] + fn git_shell_candidates_are_empty_for_a_rootless_exec_path() { + assert!(git_shell_candidates(Path::new("git-core")).is_empty()); + } + + #[test] + fn discover_posix_shell_accepts_an_explicit_override() { + let existing = std::env::current_exe().unwrap(); + assert_eq!( + discover_posix_shell(Some(existing.as_os_str())).unwrap(), + existing + ); + } + + /// A configured-but-wrong override is an error, never a silent fallback to + /// discovery: the operator asked for a specific shell and needs to be told + /// it is not there. + #[test] + fn discover_posix_shell_rejects_a_missing_override_with_setup_guidance() { + let error = discover_posix_shell(Some("/nonexistent-shell-for-tests".as_ref())) + .expect_err("a missing override should not fall through to discovery"); + assert!(error.contains("EVAL_MAGIC_SH"), "{error}"); + assert!(error.contains("/nonexistent-shell-for-tests"), "{error}"); + assert!(error.contains("Git Bash"), "{error}"); + assert!(error.contains("WSL"), "{error}"); + } + + /// Host-independent: either discovery finds a real shell, or it explains how + /// to install one. Asserting success outright would make the suite depend on + /// the developer's machine. + #[test] + fn discover_posix_shell_returns_a_real_file_or_setup_guidance() { + match discover_posix_shell(None) { + Ok(shell) => assert!(shell.is_file(), "{} is not a file", shell.display()), + Err(error) => { + assert!(error.contains("Git Bash"), "{error}"); + assert!(error.contains("WSL"), "{error}"); + } + } + } + + #[test] + fn require_posix_toolchain_names_the_tool_that_is_missing() { + let Ok(shell) = discover_posix_shell(None) else { + eprintln!("skipping: no POSIX shell on this host"); + return; + }; + let _ = shell; + let error = require_posix_toolchain(&["eval-magic-not-a-real-tool"]) + .expect_err("an uninstalled tool should be reported"); + assert!(error.contains("eval-magic-not-a-real-tool"), "{error}"); + } + + /// With nothing to look for, the check reduces to locating the shell — so it + /// holds the same host-independent shape as discovery itself. + #[test] + fn require_posix_toolchain_with_no_tools_reduces_to_finding_the_shell() { + match require_posix_toolchain(&[]) { + Ok(shell) => assert!(shell.is_file()), + Err(error) => assert!(error.contains("Git Bash"), "{error}"), + } + } + + #[test] + fn report_skip_panics_only_when_coverage_is_enforced() { + // The unenforced path is the one this suite runs under; the enforced + // path is covered by CI setting the variable. + assert!( + std::env::var_os("EVAL_MAGIC_REQUIRE_POSIX_TOOLS").is_some() + || report_skip("demo", "a demo capability") + ); } #[test] diff --git a/src/pipeline/grade/command_check.rs b/src/pipeline/grade/command_check.rs index eff8eac..c78b80d 100644 --- a/src/pipeline/grade/command_check.rs +++ b/src/pipeline/grade/command_check.rs @@ -353,8 +353,17 @@ fn execute_command_check_cell( }; #[cfg(windows)] let mut command = { + use std::os::windows::process::CommandExt; let mut command = Command::new("cmd"); - command.arg("/C").arg(&assertion.command); + // `raw_arg` plus `/S` and one wrapping pair of quotes is the only + // spelling that hands `cmd` the command verbatim. `arg` would escape the + // command's own quotes as `\"`, which `cmd` does not understand — a + // quoted argument arrives split at its spaces — and `/S` makes `cmd` + // strip exactly the wrapping pair rather than guessing. + command + .arg("/S") + .arg("/C") + .raw_arg(format!("\"{}\"", assertion.command)); command }; @@ -423,18 +432,27 @@ fn execute_command_check_cell( }) } -#[cfg(unix)] -fn termination_evidence(status: &ExitStatus) -> String { - use std::os::unix::process::ExitStatusExt; - match status.signal() { +/// The evidence line for a child that ended without an exit code. Split from +/// [`termination_evidence`] so the wording is pinned on every platform, leaving +/// the per-OS arms below with nothing to do but read the signal. +fn termination_message(signal: Option) -> String { + match signal { Some(signal) => format!("command terminated by signal {signal}"), None => "command terminated without an exit code".to_string(), } } +#[cfg(unix)] +fn termination_evidence(status: &ExitStatus) -> String { + use std::os::unix::process::ExitStatusExt; + termination_message(status.signal()) +} + +/// Windows has no signals — `ExitStatus::code()` is always `Some`, so this arm +/// exists only to keep the caller platform-agnostic. #[cfg(windows)] fn termination_evidence(_status: &ExitStatus) -> String { - "command terminated without an exit code".to_string() + termination_message(None) } fn truncate_diagnostic(value: &str) -> String { diff --git a/src/pipeline/grade/command_check/tests.rs b/src/pipeline/grade/command_check/tests.rs index 2126880..978adcb 100644 --- a/src/pipeline/grade/command_check/tests.rs +++ b/src/pipeline/grade/command_check/tests.rs @@ -15,44 +15,58 @@ fn check(command: &str) -> AssertionCommandCheck { } } -#[cfg(unix)] -fn exit_command(code: i32) -> String { - format!("exit {code}") +/// A `__fixture` invocation as a shell command line. +/// +/// `execute_command_check` hands the string to the platform shell, so it has to +/// parse identically under `sh -c` and `cmd /C`. A double-quoted program path +/// followed by double-quoted arguments does: both shells strip the quotes and +/// hand the tokens to the program unchanged. Writing one command per shell +/// dialect instead invites silent divergence — `printf x` and `echo x` do not +/// agree on the trailing newline. +fn fixture(args: &[&str]) -> String { + let exe = assert_cmd::cargo::cargo_bin("eval-magic"); + assert!( + exe.is_file(), + "the __fixture command needs the eval-magic binary at {}; \ + run `cargo test`, which builds bins, or `cargo build` first", + exe.display() + ); + let mut command = format!("\"{}\" __fixture", exe.display()); + for arg in args { + command.push_str(&format!(" \"{arg}\"")); + } + command } -#[cfg(windows)] fn exit_command(code: i32) -> String { - format!("exit /B {code}") -} - -#[cfg(unix)] -fn output_command() -> &'static str { - "printf 'hello world\\n'; printf 'diagnostic\\n' >&2" -} - -#[cfg(windows)] -fn output_command() -> &'static str { - "echo hello world & echo diagnostic 1>&2" -} - -#[cfg(unix)] -fn environment_output_command() -> &'static str { - "test -n \"$PATH\" && printf '%s' \"$EVAL_MAGIC_TEST_VALUE\"" + fixture(&["--exit", &code.to_string()]) } -#[cfg(windows)] -fn environment_output_command() -> &'static str { - "if defined PATH (echo|set /p=\"%EVAL_MAGIC_TEST_VALUE%\") else (exit /B 1)" +fn output_command() -> String { + fixture(&["--text", "hello world", "--stderr", "diagnostic"]) } -#[cfg(unix)] -fn append_command() -> &'static str { - "test -f holdout/secret.txt && printf x >> command-runs.txt" +/// Echoes the override so the test can assert the child saw it. The `PATH` +/// requirement keeps the check honest: a child handed an empty environment +/// would echo an empty value and look like a pass. +fn environment_output_command() -> String { + fixture(&[ + "--require-env", + "PATH", + "--echo-env", + "EVAL_MAGIC_TEST_VALUE", + ]) } -#[cfg(windows)] -fn append_command() -> &'static str { - "if exist holdout\\secret.txt (echo x>>command-runs.txt) else (exit /B 1)" +fn append_command() -> String { + fixture(&[ + "--require-file", + "holdout/secret.txt", + "--text", + "x", + "--append", + "command-runs.txt", + ]) } fn evals(command: &str) -> EvalsConfig { @@ -120,10 +134,23 @@ fn expected_and_unexpected_exit_codes_are_assertion_results() { assert!(failed.evidence.contains("got 3")); } +/// An eval author's `command_check` reaches the shell with its own quoting +/// intact. Windows makes this easy to get wrong: Rust escapes a command's +/// embedded quotes as `\"`, which `cmd.exe` does not understand, so a quoted +/// argument silently arrives split at the space. +#[test] +fn command_reaches_the_platform_shell_with_its_quoting_intact() { + let root = tempfile::TempDir::new().unwrap(); + let result = + execute_command_check(&check(&fixture(&["--text", "spaced value"])), root.path()).unwrap(); + assert!(result.passed, "{}", result.evidence); + assert_eq!(result.stdout, "spaced value"); +} + #[test] fn stdout_regex_must_match_complete_lossy_stdout() { let root = tempfile::TempDir::new().unwrap(); - let mut passing = check(output_command()); + let mut passing = check(&output_command()); passing.expect_stdout = Some("hello\\s+world".into()); assert!(execute_command_check(&passing, root.path()).unwrap().passed); @@ -136,7 +163,7 @@ fn stdout_regex_must_match_complete_lossy_stdout() { #[test] fn command_environment_overrides_are_visible_to_the_child_process() { let root = tempfile::TempDir::new().unwrap(); - let mut assertion = check(environment_output_command()); + let mut assertion = check(&environment_output_command()); assertion.env = Some(std::collections::BTreeMap::from([( "EVAL_MAGIC_TEST_VALUE".into(), "configured".into(), @@ -148,17 +175,21 @@ fn command_environment_overrides_are_visible_to_the_child_process() { assert_eq!(result.stdout, "configured"); } -#[cfg(unix)] #[test] fn command_checks_clear_inherited_git_routing_before_explicit_overlays() { const CHILD_MARKER: &str = "EVAL_MAGIC_GIT_ENV_CHILD"; if std::env::var_os(CHILD_MARKER).is_some() { let root = tempfile::TempDir::new().unwrap(); - let inherited = - execute_command_check(&check("printf '%s' \"${GIT_DIR-unset}\""), root.path()).unwrap(); + // `--default unset` distinguishes a cleared variable from one set to the + // empty string, which is the property `clear_git_environment` promises. + let inherited = execute_command_check( + &check(&fixture(&["--echo-env", "GIT_DIR", "--default", "unset"])), + root.path(), + ) + .unwrap(); assert_eq!(inherited.stdout, "unset"); - let mut restored = check("printf '%s' \"$GIT_DIR\""); + let mut restored = check(&fixture(&["--echo-env", "GIT_DIR"])); restored.env = Some(std::collections::BTreeMap::from([( "GIT_DIR".into(), "/declared/repository.git".into(), @@ -215,7 +246,7 @@ fn invalid_direct_environment_configuration_returns_an_error_instead_of_panickin #[test] fn stdout_expectation_is_applied_to_every_matrix_cell() { let root = tempfile::TempDir::new().unwrap(); - let mut assertion = check(environment_output_command()); + let mut assertion = check(&environment_output_command()); assertion.matrix = Some(std::collections::BTreeMap::from([( "EVAL_MAGIC_TEST_VALUE".into(), vec!["expected".into(), "unexpected".into()], @@ -266,14 +297,26 @@ fn matrix_environment_expansion_is_deterministic_and_overrides_fixed_values() { ); } -#[cfg(unix)] #[test] fn matrix_runs_every_cartesian_cell_in_deterministic_order_and_reports_results() { let root = tempfile::TempDir::new().unwrap(); - let mut assertion = check( - "printf '%s|%s|%s\\n' \"$FIXED\" \"$LOCALE\" \"$TZ\" >> matrix-runs.txt; \ - test \"$TZ\" != Europe/Berlin", - ); + // The append must happen for every cell, including the ones that fail — + // `matrix-runs.txt` is the evidence that all four ran, and in what order. + let mut assertion = check(&fixture(&[ + "--echo-env", + "FIXED", + "--echo-env", + "LOCALE", + "--echo-env", + "TZ", + "--separator", + "|", + "--newline", + "--append", + "matrix-runs.txt", + "--require-env", + "TZ=UTC", + ])); assertion.env = Some(std::collections::BTreeMap::from([ ("FIXED".into(), "configured".into()), ("TZ".into(), "base".into()), @@ -322,7 +365,7 @@ fn matrix_runs_every_cartesian_cell_in_deterministic_order_and_reports_results() #[test] fn invalid_stdout_regex_is_a_failed_assertion_with_evidence() { let root = tempfile::TempDir::new().unwrap(); - let mut assertion = check(output_command()); + let mut assertion = check(&output_command()); assertion.expect_stdout = Some("(".into()); let result = execute_command_check(&assertion, root.path()).unwrap(); @@ -333,19 +376,17 @@ fn invalid_stdout_regex_is_a_failed_assertion_with_evidence() { #[test] fn stdout_and_stderr_diagnostics_are_retained_and_capped_at_two_kib() { let root = tempfile::TempDir::new().unwrap(); - let result = execute_command_check(&check(output_command()), root.path()).unwrap(); + let result = execute_command_check(&check(&output_command()), root.path()).unwrap(); assert!(result.stdout.contains("hello world")); assert!(result.stderr.contains("diagnostic")); assert!(result.stdout.len() <= 2048); assert!(result.stderr.len() <= 2048); } -#[cfg(unix)] #[test] fn stdout_regex_uses_complete_output_before_diagnostics_are_truncated() { let root = tempfile::TempDir::new().unwrap(); - let mut assertion = - check("i=0; while [ \"$i\" -lt 3000 ]; do printf x; i=$((i + 1)); done; printf TAIL"); + let mut assertion = check(&fixture(&["--pad", "3000", "--text", "TAIL"])); assertion.expect_stdout = Some("TAIL$".into()); let result = execute_command_check(&assertion, root.path()).unwrap(); assert!(result.passed); @@ -353,9 +394,29 @@ fn stdout_regex_uses_complete_output_before_diagnostics_are_truncated() { assert!(!result.stdout.contains("TAIL")); } -#[cfg(unix)] +/// The evidence wording is host-independent even though reading a signal is +/// not, so it is pinned on every platform rather than only where signals exist. +#[test] +fn termination_message_names_the_signal_when_there_is_one() { + assert_eq!( + termination_message(Some(15)), + "command terminated by signal 15" + ); + assert_eq!( + termination_message(None), + "command terminated without an exit code" + ); +} + #[test] fn signal_termination_is_an_ordinary_failed_assertion() { + // Windows has no signals: a child always reports an exit code, so there is + // no way to reach the no-code path from the outside. The wording it would + // produce is pinned by `termination_message_names_the_signal_when_there_is_one` + // instead, which runs everywhere. + if cfg!(windows) { + return; + } let root = tempfile::TempDir::new().unwrap(); let result = execute_command_check(&check("kill -TERM $$"), root.path()).unwrap(); assert!(!result.passed); @@ -376,7 +437,7 @@ fn persisted_results_are_reused_and_overwrite_reruns_in_declaration_order() { assert!(!eval_root.join("holdout/secret.txt").exists()); let first = - grade_command_checks(&iteration_dir, &evals(append_command()), &skill_dir, false).unwrap(); + grade_command_checks(&iteration_dir, &evals(&append_command()), &skill_dir, false).unwrap(); assert_eq!(first.executed, 1); assert_eq!(first.reused, 0); assert_eq!( @@ -391,7 +452,7 @@ fn persisted_results_are_reused_and_overwrite_reruns_in_declaration_order() { assert!(result_path.exists()); let reused = - grade_command_checks(&iteration_dir, &evals(append_command()), &skill_dir, false).unwrap(); + grade_command_checks(&iteration_dir, &evals(&append_command()), &skill_dir, false).unwrap(); assert_eq!(reused.executed, 0); assert_eq!(reused.reused, 1); assert_eq!( @@ -400,7 +461,7 @@ fn persisted_results_are_reused_and_overwrite_reruns_in_declaration_order() { ); let overwritten = - grade_command_checks(&iteration_dir, &evals(append_command()), &skill_dir, true).unwrap(); + grade_command_checks(&iteration_dir, &evals(&append_command()), &skill_dir, true).unwrap(); assert_eq!(overwritten.executed, 1); assert_eq!(overwritten.reused, 0); assert_eq!( @@ -420,7 +481,7 @@ fn persisted_matrix_results_are_schema_gated_and_reused() { fs::write(skill_dir.join("evals/holdout/secret.txt"), "held out").unwrap(); write_dispatch(&iteration_dir, &eval_root, false); - let mut config = evals(append_command()); + let mut config = evals(&append_command()); let crate::core::Assertion::CommandCheck(check) = config.evals[0] .assertions .as_mut() @@ -474,14 +535,13 @@ fn shared_eval_root_is_rejected_with_fresh_iteration_guidance() { fs::write(skill_dir.join("evals/holdout/secret.txt"), "held out").unwrap(); write_dispatch(&iteration_dir, &eval_root, true); - let error = grade_command_checks(&iteration_dir, &evals("true"), &skill_dir, false) + let error = grade_command_checks(&iteration_dir, &evals(&exit_command(0)), &skill_dir, false) .unwrap_err() .to_string(); assert!(error.contains("shares eval_root"), "{error}"); assert!(error.contains("fresh iteration"), "{error}"); } -#[cfg(unix)] #[test] fn multiple_checks_execute_in_declaration_order_against_one_env() { let root = tempfile::TempDir::new().unwrap(); @@ -501,12 +561,12 @@ fn multiple_checks_execute_in_declaration_order_against_one_env() { { "id": "first", "type": "command_check", - "command": "printf ready > state.txt" + "command": fixture(&["--text", "ready", "--write", "state.txt"]) }, { "id": "second", "type": "command_check", - "command": "test \"$(cat state.txt)\" = ready" + "command": fixture(&["--require-file-text", "state.txt", "ready"]) } ] }] diff --git a/tests/cli/basics.rs b/tests/cli/basics.rs index 088f20b..4d816d0 100644 --- a/tests/cli/basics.rs +++ b/tests/cli/basics.rs @@ -17,6 +17,46 @@ fn write_evals(root: &std::path::Path, skill: &str, contents: &str) { fs::write(dir.join("evals.json"), contents).unwrap(); } +/// The hidden `__fixture` subcommand is the suite's stand-in for `sh`, `true`, +/// and `printf`, so its wiring — parsing, effects, exit code, and stream +/// flushing before `process::exit` — has to hold end to end, not just in the +/// unit tests that drive it with in-memory buffers. +#[test] +fn hidden_fixture_subcommand_emits_and_exits() { + let tmp = TempDir::new().unwrap(); + let target = tmp.path().join("written.txt"); + + skill_eval() + .args([ + "__fixture", + "--exit", + "3", + "--text", + "hello world", + "--stderr", + "diagnostic", + ]) + .arg("--write") + .arg(&target) + .assert() + .code(3) + .stdout("hello world") + .stderr("diagnostic"); + + assert_eq!(fs::read_to_string(&target).unwrap(), "hello world"); +} + +/// Hidden means hidden: `__fixture` must never surface in the help tree a user +/// reads, the way `guard-hook` does not. +#[test] +fn hidden_fixture_subcommand_is_absent_from_help() { + skill_eval() + .arg("--help") + .assert() + .success() + .stdout(contains("__fixture").not()); +} + /// `--help` succeeds and lists the subcommands. #[test] fn help_lists_subcommands() { diff --git a/tests/run/command_check.rs b/tests/run/command_check.rs index 0be534d..a33861f 100644 --- a/tests/run/command_check.rs +++ b/tests/run/command_check.rs @@ -34,37 +34,25 @@ fn dispatch_tasks(cwd: &Path) -> Vec { .clone() } -#[cfg(unix)] -fn setup_exists_command() -> &'static str { - "test -f holdout/secret.txt" +fn setup_exists_command() -> String { + fixture(&["--require-file", "holdout/secret.txt"]) } -#[cfg(windows)] -fn setup_exists_command() -> &'static str { - "if exist holdout\\secret.txt (exit /B 0) else (exit /B 1)" -} - -#[cfg(unix)] -fn held_out_compare_command() -> &'static str { - "test \"$(cat answer.txt)\" = \"$(cat holdout/expected.txt)\"" -} - -#[cfg(windows)] -fn held_out_compare_command() -> &'static str { - "fc /B answer.txt holdout\\expected.txt >NUL" +fn held_out_compare_command() -> String { + fixture(&["--files-equal", "answer.txt", "holdout/expected.txt"]) } #[test] fn auto_isolates_and_multi_run_tasks_get_distinct_hidden_envs() { let tmp = tempfile::TempDir::new().unwrap(); - let evals = r#"{ "skill_name": "mr-review", "evals": [ + let evals = json!({ "skill_name": "mr-review", "evals": [ { "id": "ordinary-1", "prompt": "p1", "expected_output": "o" }, { "id": "held-out", "prompt": "p2", "expected_output": "o", "runs": 2, "assertions": [{ "id": "secret-test", "type": "command_check", - "setup_files": ["holdout/secret.txt"], "command": "test -f holdout/secret.txt" }] }, + "setup_files": ["holdout/secret.txt"], "command": setup_exists_command() }] }, { "id": "ordinary-2", "prompt": "p3", "expected_output": "o" } - ] }"#; - let (skill_dir, cwd) = setup(tmp.path(), evals); + ] }); + let (skill_dir, cwd) = setup(tmp.path(), &serde_json::to_string(&evals).unwrap()); fs::create_dir_all(skill_dir.join("mr-review/evals/holdout")).unwrap(); fs::write( skill_dir.join("mr-review/evals/holdout/secret.txt"), @@ -134,7 +122,7 @@ fn missing_setup_source_fails_before_staging() { let evals = r#"{ "skill_name": "mr-review", "evals": [ { "id": "held-out", "prompt": "p", "expected_output": "o", "assertions": [{ "id": "secret-test", "type": "command_check", - "setup_files": ["holdout/missing.txt"], "command": "true" }] } + "setup_files": ["holdout/missing.txt"], "command": "exit 0" }] } ] }"#; let (skill_dir, cwd) = setup(tmp.path(), evals); @@ -159,7 +147,7 @@ fn root_git_setup_path_is_rejected_before_staging() { let evals = r#"{ "skill_name": "mr-review", "evals": [ { "id": "held-out", "prompt": "p", "expected_output": "o", "assertions": [{ "id": "secret-test", "type": "command_check", - "setup_files": [".GIT/config"], "command": "true" }] } + "setup_files": [".GIT/config"], "command": "exit 0" }] } ] }"#; let (skill_dir, cwd) = setup(tmp.path(), evals); fs::create_dir_all(skill_dir.join("mr-review/evals/.GIT")).unwrap(); @@ -458,5 +446,4 @@ fn ingests_without_transcripts_while_guard_is_armed_and_aggregates() { ); } -#[cfg(unix)] mod matrix; diff --git a/tests/run/command_check/matrix.rs b/tests/run/command_check/matrix.rs index a0760ef..469c38a 100644 --- a/tests/run/command_check/matrix.rs +++ b/tests/run/command_check/matrix.rs @@ -1,7 +1,16 @@ use super::*; -fn timezone_matrix_command() -> &'static str { - "printf '%s:%s' \"$FIXED\" \"$TZ\"; test \"$TZ\" = UTC" +fn timezone_matrix_command() -> String { + fixture(&[ + "--echo-env", + "FIXED", + "--echo-env", + "TZ", + "--separator", + ":", + "--require-env", + "TZ=UTC", + ]) } #[test] diff --git a/tests/run/conversation.rs b/tests/run/conversation.rs index 4fca100..460343b 100644 --- a/tests/run/conversation.rs +++ b/tests/run/conversation.rs @@ -3,14 +3,8 @@ use crate::helpers::*; use predicates::prelude::PredicateBooleanExt; use predicates::str::contains; -// Every `#[cfg(unix)]` import below exists solely for -// `dispatch_task_runs_all_scripted_turns_in_one_native_session` — see the note on that test. -#[cfg(unix)] use serde_json::Value; use std::fs; -#[cfg(unix)] -use std::os::unix::fs::PermissionsExt; -#[cfg(unix)] use std::path::Path; #[test] @@ -69,10 +63,11 @@ fn multi_turn_eval_dispatch_records_followups_and_conversation_artifact_path() { } } -// The harness stub below is a `#!/bin/sh` script invoked directly through the descriptor's -// exec_template, which needs both a POSIX shell and an executable bit. Windows has neither, so the -// scripted-turn path is covered on Unix only until a portable stub replaces it. -#[cfg(unix)] +// The harness stub below is a POSIX shell script, because that is what a real `exec_template` is — +// every descriptor in `harnesses/` ships one (`` redirection). +// The driver resolves an `sh` on every host, and the template invokes the stub *through* that `sh` +// rather than executing it directly, so no executable bit is involved and the scripted-turn path is +// covered everywhere. #[test] fn dispatch_task_runs_all_scripted_turns_in_one_native_session() { let tmp = tempfile::TempDir::new().unwrap(); @@ -143,19 +138,17 @@ printf '%s\n' '{"type":"turn.completed","usage":{"input_tokens":2,"output_tokens "#, ) .unwrap(); - let mut permissions = fs::metadata(&fixture).unwrap().permissions(); - permissions.set_mode(0o755); - fs::set_permissions(&fixture, permissions).unwrap(); let dispatch_path = iteration_dir(&cwd).join("dispatch.json"); let mut dispatch = read_json(&dispatch_path); - let fixture = fixture.to_string_lossy(); + // `sh ` rather than ``: the script needs no executable bit that + // way, and the shell running it is the one the driver already resolved. + let stub = format!("sh \"{}\"", fixture.to_string_lossy()); dispatch["harness_descriptor"]["dispatch"]["exec_template"] = - serde_json::json!(format!("{fixture} initial ")); - dispatch["harness_descriptor"]["conversation"]["resume_exec_template"] = - serde_json::json!(format!( - "{fixture} resume- {{session_arg}} {{prompt_arg}}" - )); + serde_json::json!(format!("{stub} initial ")); + dispatch["harness_descriptor"]["conversation"]["resume_exec_template"] = serde_json::json!( + format!("{stub} resume- {{session_arg}} {{prompt_arg}}") + ); fs::write( &dispatch_path, format!("{}\n", serde_json::to_string_pretty(&dispatch).unwrap()), diff --git a/tests/run/diff_scope.rs b/tests/run/diff_scope.rs index 80d27e0..2da4501 100644 --- a/tests/run/diff_scope.rs +++ b/tests/run/diff_scope.rs @@ -114,7 +114,6 @@ fn ingest_writes_diff_scope_for_every_run_without_an_assertion() { ); } -#[cfg(unix)] #[test] fn diff_scope_is_captured_before_command_check_setup_and_reused() { let tmp = tempfile::TempDir::new().unwrap(); @@ -122,7 +121,7 @@ fn diff_scope_is_captured_before_command_check_setup_and_reused() { { "id": "held-out", "prompt": "fix source.txt", "expected_output": "fixed", "skill_should_trigger": false, "files": ["source.txt"], "assertions": [{ "id": "secret", "type": "command_check", - "setup_files": ["holdout/secret.txt"], "command": "true" }] } ] }"#; + "setup_files": ["holdout/secret.txt"], "command": "exit 0" }] } ] }"#; let (skill_dir, cwd) = setup(tmp.path(), evals); fs::write(skill_dir.join("mr-review/evals/source.txt"), "old\n").unwrap(); fs::create_dir_all(skill_dir.join("mr-review/evals/holdout")).unwrap(); diff --git a/tests/run/helpers.rs b/tests/run/helpers.rs index b0335a0..69dfe68 100644 --- a/tests/run/helpers.rs +++ b/tests/run/helpers.rs @@ -64,6 +64,20 @@ pub fn wire_path(path: &Path) -> String { path.to_string_lossy().replace('\\', "/") } +/// A `__fixture` invocation as a `command_check` command line. +/// +/// The grader hands the string to the platform shell, so it has to parse the +/// same under `sh -c` and `cmd /C`: a double-quoted program path followed by +/// double-quoted arguments does. One such command covers what `test`, `true`, +/// and `fc` would each have to spell differently per shell. +pub fn fixture(args: &[&str]) -> String { + let mut command = format!("\"{}\" __fixture", env!("CARGO_BIN_EXE_eval-magic")); + for arg in args { + command.push_str(&format!(" \"{arg}\"")); + } + command +} + /// `fs::canonicalize` with Windows' verbatim (`\\?\`) prefix removed. /// /// The symlink resolution matters — a macOS temp dir lives under a symlinked From baba4342460f26cb0f41bf5823ffbc98f2b57326 Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Fri, 14 Aug 2026 18:17:46 -0400 Subject: [PATCH 09/18] fix(probe): supply a {guard_args} stand-in to the render-only checks RENDER_STAND_INS backed only {cwd} and {model_arg}, so render_only_check reported {guard_args} as unresolved in codex's parallel_command_template and judge_command_template. The real dispatch path resolves it from dispatch.guard_args, so both were false failures. Add the stand-in and cover it two ways: a focused check on the concatenation shape guarded harnesses use, and a sweep over every embedded descriptor so a future built-in placeholder without a stand-in fails cargo test rather than surfacing as a spurious probe failure. Co-Authored-By: Claude Opus 5 --- src/cli/commands/harness/probe.rs | 46 +++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/src/cli/commands/harness/probe.rs b/src/cli/commands/harness/probe.rs index d650d89..224ea87 100644 --- a/src/cli/commands/harness/probe.rs +++ b/src/cli/commands/harness/probe.rs @@ -157,10 +157,16 @@ fn render_only_check(template: &str, vars: &[(&str, &str)]) -> Result<(), ProbeE const PROBE_PROMPT: &str = "Reply with the single word: ok\n"; /// Stand-in `{var}` values used by the render-only checks so a missing backing -/// field never masquerades as a clean render. -const RENDER_STAND_INS: [(&str, &str); 2] = [ +/// field never masquerades as a clean render. Every placeholder the dispatch +/// path fills needs an entry here, or the check reports a token the real run +/// resolves. The values are visible markers rather than faithful fragments — +/// the rendered text is only scanned for leftover braces, never executed — so +/// `{guard_args}` carries one too instead of the empty fragment the probe hands +/// the exec template. +const RENDER_STAND_INS: [(&str, &str); 3] = [ ("cwd", "/probe/stand-in/cwd"), ("model_arg", "stand-in-model"), + ("guard_args", "--stand-in-guard"), ]; /// The live dispatch probe. Renders `dispatch.exec_template` with a trivial @@ -295,6 +301,7 @@ pub(crate) fn run_probe( #[cfg(test)] mod tests { use super::*; + use crate::adapters::descriptor::{EMBEDDED_DESCRIPTORS, load_descriptor}; use std::fs; use std::path::PathBuf; @@ -422,4 +429,39 @@ mod tests { let vars = [("cwd", "/work"), ("model_arg", "gpt-x")]; render_only_check(template, &vars).expect("shell braces plus a resolved {cwd} should pass"); } + + #[test] + fn render_stand_ins_cover_guard_args() { + // Guarded harnesses splice {guard_args} onto a preceding flag value. + // A stand-in must back it or the probe reports a placeholder the real + // dispatch path resolves. + let template = "agent exec --sandbox workspace-write{guard_args} --cd {cwd}"; + render_only_check(template, &RENDER_STAND_INS).expect("{guard_args} must have a stand-in"); + } + + #[test] + fn render_stand_ins_cover_every_shipped_dispatch_template() { + // The render-only checks run against the shipped descriptors, so the + // stand-ins have to cover every placeholder those descriptors use. + // Anything missing surfaces as a false `✗ render:` failure. + for (source, toml_src) in EMBEDDED_DESCRIPTORS { + let descriptor = load_descriptor(toml_src, source) + .unwrap_or_else(|e| panic!("embedded descriptor {source} is invalid: {e}")); + let dispatch = &descriptor.dispatch; + for (field, template) in [ + ( + "parallel_command_template", + dispatch.parallel_command_template.as_deref(), + ), + ( + "judge_command_template", + dispatch.judge_command_template.as_deref(), + ), + ] { + let Some(template) = template else { continue }; + render_only_check(template, &RENDER_STAND_INS) + .unwrap_or_else(|e| panic!("{source} {field}: {e}")); + } + } + } } From 7fd70961abf6b70b934b07d8d0ecbbbc1c272088 Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Fri, 14 Aug 2026 23:11:00 -0400 Subject: [PATCH 10/18] fix(run): declare a POSIX shell + jq requirement and preflight for it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `run` prepared a correct workspace on a host with no POSIX shell, then printed — and wrote into RUNBOOK.md and dispatch-manifest.md — commands only a POSIX shell can execute, with nothing to say a different shell was expected. State the requirement instead, once, and reuse it: POSIX_TOOLING_REQUIREMENT feeds the shell-discovery errors, the new `run` preflight, RUNBOOK.md, and dispatch-manifest.md. It names `jq` alongside the shell because the parallel-dispatch and judge recipes are `jq` pipelines, and Git for Windows bundles `sh`, `xargs`, `tr`, and `wc` but not `jq` — so naming only the shell would point an operator at a setup that still walls out at the judge step. The preflight warns and continues rather than failing. `run` never dispatches, so preparing on Windows and dispatching from WSL stays a valid split. Development carries the same requirement. The scripted-turn tests already spawn a `#!/bin/sh` stub through the resolved shell with no capability skip, so a POSIX shell was required in fact but recorded nowhere; the two tolerant discovery tests now assert it. `jq` and symlink creation remain capability-gated skips, since Git for Windows supplies neither. Closes #248 Co-Authored-By: Claude Opus 5 --- AGENTS.md | 23 ++-- README.md | 11 +- docs/developer_overview.md | 5 + profiles/shared/runbook.md | 2 + src/adapters/cli_command.rs | 6 +- src/cli/help.rs | 6 ++ src/cli/run/dispatch.rs | 6 +- src/cli/run/orchestrate/mod.rs | 9 ++ src/cli/run/orchestrate/shell.rs | 100 ++++++++++++++++++ src/cli/run/runbook.rs | 5 +- src/core/mod.rs | 3 +- src/core/runtime.rs | 81 ++++++++------ tests/cli/docs.rs | 34 +++++- .../claude-code/manifest-nomodel.golden.md | 2 + tests/golden/claude-code/manifest.golden.md | 2 + tests/golden/claude-code/runbook.golden.md | 2 + tests/golden/cline/manifest.golden.md | 2 + tests/golden/cline/runbook.golden.md | 2 + tests/golden/codex/manifest-noguard.golden.md | 2 + tests/golden/codex/manifest.golden.md | 2 + tests/golden/codex/runbook.golden.md | 2 + tests/golden/opencode/manifest.golden.md | 2 + tests/golden/opencode/runbook.golden.md | 2 + tests/run/runbook.rs | 47 ++++++++ 24 files changed, 308 insertions(+), 50 deletions(-) create mode 100644 src/cli/run/orchestrate/shell.rs diff --git a/AGENTS.md b/AGENTS.md index 61bd44c..9b61d91 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -63,15 +63,26 @@ binary, `cargo test --lib` alone does not build it — run `cargo test`, or `car compilation and clippy on the other host and hides the coverage gap. Instead, probe for what the test actually needs and call `report_skip` (`src/core/runtime.rs`), which prints the reason and returns `true`. Setting `EVAL_MAGIC_REQUIRE_POSIX_TOOLS=1` turns every skip into a failure; CI sets -it, so a runner cannot quietly stop covering something. Two capabilities are gated today: the POSIX -toolchain the shipped dispatch recipes need (`require_posix_toolchain`), and symlink creation, which -Windows allows only under Developer Mode. Where a genuine per-OS difference is the behavior under +it, so a runner cannot quietly stop covering something. Two capabilities are gated today: the recipe +tools beyond the shell itself (`require_posix_toolchain` — in practice `jq`), and symlink creation, +which Windows allows only under Developer Mode. The shell is not one of them; it is a hard +requirement, per the section below. `require_posix_toolchain` is not test-only either — the `run` +preflight uses it to warn about the same gap. Where a genuine per-OS difference is the behavior under test — signals, path separators — branch on `cfg!(windows)` at runtime so both arms still compile everywhere. -**Finding a POSIX shell.** Harness `exec_template`s are POSIX command lines, so the dispatch and -probe paths spawn `sh` via `posix_shell()` (`src/core/runtime.rs`) rather than a hardcoded -`/bin/sh`: it searches `PATH`, then a Git for Windows install. Set `EVAL_MAGIC_SH` to override it. +**A POSIX shell is required, for use and for development.** Harness `exec_template`s are POSIX +command lines, so the dispatch and probe paths spawn `sh` via `posix_shell()` +(`src/core/runtime.rs`) rather than a hardcoded `/bin/sh`: it searches `PATH`, then a Git for +Windows install. Set `EVAL_MAGIC_SH` to override it. `cargo test` inherits the requirement — the +scripted-turn tests spawn a `#!/bin/sh` harness stub through the resolved shell and do not skip — +so a host without `sh` fails the suite instead of quietly covering less. `jq` is required alongside +it for the parallel-dispatch and judge recipes; Git for Windows supplies the shell, `xargs`, `tr`, +and `wc`, but not `jq`. `POSIX_TOOLING_REQUIREMENT` (`src/core/runtime.rs`) is the one wording the +Markdown-carrying surfaces reuse: the shell-discovery errors, the `run` preflight warnings, +`RUNBOOK.md`, and `dispatch-manifest.md`. State the requirement from there rather than rephrasing +it. `--help` is the one deliberate restatement (`AFTER_HELP` in `src/cli/help.rs`), hard-wrapped and +backtick-free because clap renders into a terminal; keep the two in step by hand. **Where user-facing warnings come from.** Library modules (`pipeline`, `workspace`, `sandbox`, `adapters`) never print. They return warning strings on their result struct — `#[serde(skip)]` when diff --git a/README.md b/README.md index 77082f4..595de5e 100644 --- a/README.md +++ b/README.md @@ -37,7 +37,12 @@ The installed CLI is the primary manual. Start with `eval-magic --help`, and use ## Install -Git is required at runtime. Prebuilt binaries for macOS, Linux, and Windows are attached to each +Git is required at runtime, plus a POSIX shell with `jq`: the dispatch and judge recipes eval-magic +generates are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. On Windows, run them in +Git Bash (Git for Windows) or WSL, and install `jq` separately — Git for Windows does not bundle it. +Set `EVAL_MAGIC_SH` to select a specific `sh`. + +Prebuilt binaries for macOS, Linux, and Windows are attached to each [GitHub release](https://github.com/slowdini/eval-magic/releases). macOS or Linux: @@ -143,6 +148,10 @@ Issues and planned work are tracked in the ## Development +Development carries the same host requirement as use: a POSIX shell with `jq`. The scripted-turn +tests spawn `#!/bin/sh` harness stubs through the resolved shell and do not skip, so the suite +cannot pass without one. Tests that need `jq` or symlink creation report a skip instead. + ```bash cargo fmt --check cargo build diff --git a/docs/developer_overview.md b/docs/developer_overview.md index 71f842e..8b57d73 100644 --- a/docs/developer_overview.md +++ b/docs/developer_overview.md @@ -73,6 +73,11 @@ editing. Add a focused failing test at the narrowest useful boundary, implement run the focused test again. Cross-harness changes belong at shared descriptor, runner, or adapter boundaries unless the evidence requires a named harness capability. +Development carries the host requirement the tool itself declares: a POSIX shell with `jq`. The +scripted-turn tests spawn `#!/bin/sh` harness stubs through the resolved shell and do not skip, so +the suite cannot pass without one. Tests needing `jq` or symlink creation report a skip instead; +`EVAL_MAGIC_REQUIRE_POSIX_TOOLS=1` turns those skips into failures, as CI sets it to do. + Before handing work off, run: ```text diff --git a/profiles/shared/runbook.md b/profiles/shared/runbook.md index e07a836..8a48504 100644 --- a/profiles/shared/runbook.md +++ b/profiles/shared/runbook.md @@ -4,6 +4,8 @@ This runbook is for a human driving the run from a terminal. Work from this iter and copy-paste each step. The workspace is self-contained — you should not need the surrounding repo. +> **Requires:** {{POSIX_REQUIREMENT}} + - **Skill under test:** {{SKILL_NAME}} - **Mode:** {{MODE}} — comparing `{{COND_A}}` vs `{{COND_B}}` - **Dispatches:** {{NUM_TASKS}} (the `tasks[]` array in `{{DISPATCH_JSON}}`) diff --git a/src/adapters/cli_command.rs b/src/adapters/cli_command.rs index 313824e..6fcf39a 100644 --- a/src/adapters/cli_command.rs +++ b/src/adapters/cli_command.rs @@ -191,10 +191,6 @@ mod tests { .unwrap(); } - /// The tools the rendered judge recipe shells out to. Git for Windows - /// bundles every one of these except `jq`. - const RECIPE_TOOLS: &[&str] = &["jq", "xargs", "tr", "wc"]; - /// The shell to run a rendered recipe in, or `None` after reporting a skip. /// /// These three tests execute shipped POSIX pipeline text, so no portable @@ -203,7 +199,7 @@ mod tests { /// that has the tools (including Windows with `jq` installed) and stops them /// failing inscrutably on a Linux box that happens to lack `jq`. fn recipe_shell(test: &str) -> Option<&'static Path> { - match crate::core::runtime::require_posix_toolchain(RECIPE_TOOLS) { + match crate::core::runtime::require_posix_toolchain(crate::core::POSIX_RECIPE_TOOLS) { Ok(shell) => Some(shell), Err(missing) => { crate::core::runtime::report_skip(test, &missing); diff --git a/src/cli/help.rs b/src/cli/help.rs index 59f76c1..9dc74fb 100644 --- a/src/cli/help.rs +++ b/src/cli/help.rs @@ -7,6 +7,12 @@ /// Worked examples shown at the end of `eval-magic --help`. pub(super) const AFTER_HELP: &str = "\ +REQUIREMENTS: + Git, plus a POSIX shell with jq, xargs, tr, and wc. The dispatch and judge + recipes in the generated RUNBOOK.md are POSIX command lines — on Windows, + run them in Git Bash (Git for Windows) or WSL, and install jq separately. + Set EVAL_MAGIC_SH to select a specific sh. + EXAMPLES: # Scaffold a first eval and prepare its isolated comparison environments eval-magic init diff --git a/src/cli/run/dispatch.rs b/src/cli/run/dispatch.rs index eeb1ac5..a13fe10 100644 --- a/src/cli/run/dispatch.rs +++ b/src/cli/run/dispatch.rs @@ -14,7 +14,7 @@ use serde::{Deserialize, Serialize}; use crate::adapters::{CliManifestContext, adapter_for}; use crate::core::fs::artifact_path; -use crate::core::{AvailableSkill, Eval, Harness, ScriptedTurn}; +use crate::core::{AvailableSkill, Eval, Harness, POSIX_TOOLING_REQUIREMENT, ScriptedTurn}; use super::RunError; @@ -413,6 +413,10 @@ pub fn build_manifest( String::new(), "In an agent session, read `dispatch.json` (sibling of this file) instead of this manifest. Each task has a `dispatch_prompt_path` field pointing at the file that holds the full prompt — dispatch the task with a short \"read this file and follow it\" instruction rather than inlining the prompt — plus exact paths for `run.json` and `timing.json`.".to_string(), String::new(), + // The recipes below are POSIX command lines, so the manifest states the + // requirement the same way RUNBOOK.md does (issue #248). + format!("**Requires:** {POSIX_TOOLING_REQUIREMENT}"), + String::new(), ]; let scripted: Vec = tasks .iter() diff --git a/src/cli/run/orchestrate/mod.rs b/src/cli/run/orchestrate/mod.rs index bb7467d..f7fa2ce 100644 --- a/src/cli/run/orchestrate/mod.rs +++ b/src/cli/run/orchestrate/mod.rs @@ -27,6 +27,7 @@ mod envs; mod git; mod resolve; mod shadow_preflight; +mod shell; mod stage; /// Run options parsed from the `run` subcommand flags (everything beyond the @@ -105,6 +106,14 @@ pub fn command_run(ctx: &RunContext, opts: &RunOptions) -> Result<(), RunError> // before resolution chooses or creates any iteration workspace. git::preflight_git(ctx)?; + // The POSIX toolchain is a requirement of the *recipes* this run generates, + // not of the run itself, so it warns and continues (issue #248). Printed up + // front: an operator who has to install something should learn it before + // reading a runbook full of commands their shell cannot parse. + for warning in shell::preflight_posix_tooling() { + eprintln!("⚠ {warning}"); + } + // Resolve first (read-only): the preflight scopes its transcript warning // to the eval config actually selected for the run. let resolved = resolve::resolve_request(ctx, opts)?; diff --git a/src/cli/run/orchestrate/shell.rs b/src/cli/run/orchestrate/shell.rs new file mode 100644 index 0000000..a8c8a6e --- /dev/null +++ b/src/cli/run/orchestrate/shell.rs @@ -0,0 +1,100 @@ +//! Host-tooling preflight for `run`: does this machine have what the generated +//! recipes ask an operator to paste? +//! +//! Unlike [`super::git`], a missing shell is a warning rather than an error. +//! `run` never dispatches — it prepares a workspace, and that workspace is +//! correct whatever shell prepared it. Preparing on Windows and dispatching from +//! WSL is a legitimate split, so this reports the gap and lets the run finish. + +use std::path::Path; + +use crate::core::{ + POSIX_RECIPE_TOOLS, POSIX_TOOLING_REQUIREMENT, posix_shell, require_posix_toolchain, +}; + +/// Warnings for a host that cannot run the recipes this run is about to +/// generate. Empty on a complete host. +pub(super) fn preflight_posix_tooling() -> Vec { + let shell = posix_shell(); + // Only probe for tools once a shell exists: `require_posix_toolchain` + // resolves the same shell first, and reporting one failure beats reporting + // the same absence twice in different words. + let missing = match shell { + Ok(_) => require_posix_toolchain(POSIX_RECIPE_TOOLS).err(), + Err(_) => None, + }; + tooling_warning(shell, missing.as_deref()) + .into_iter() + .collect() +} + +/// The operator warning for a host that cannot run the generated recipes, or +/// `None` when it can — a healthy run stays silent. +/// +/// Two distinguishable failures. `shell` is `Err` when no `sh` exists at all, +/// and its message already carries [`POSIX_TOOLING_REQUIREMENT`], so the warning +/// only adds that the prepared workspace survives. `missing` is the tool absent +/// from a shell that *was* found — the Git for Windows case, which resolves a +/// shell but bundles no `jq` — and needs the requirement appended. +fn tooling_warning(shell: Result<&Path, &str>, missing: Option<&str>) -> Option { + if let Err(reason) = shell { + return Some(format!( + "{reason} The workspace and recipes below are still correct — prepare here and \ + dispatch them from a POSIX shell." + )); + } + let reason = missing?; + Some(format!( + "{reason}. The parallel-dispatch and judge recipes in RUNBOOK.md are pipelines over that \ + toolchain and cannot run without it. {POSIX_TOOLING_REQUIREMENT}" + )) +} + +#[cfg(test)] +mod tests { + use std::path::Path; + + use super::*; + + /// A complete host stays silent — the preflight must not nag the common case. + #[test] + fn a_complete_host_produces_no_warning() { + assert_eq!(tooling_warning(Ok(Path::new("/bin/sh")), None), None); + } + + /// No shell at all: `posix_shell`'s own message already carries the declared + /// requirement, so the warning adds only the reassurance that the prepared + /// workspace is still correct. + #[test] + fn a_missing_shell_warns_with_the_declared_requirement() { + let warning = tooling_warning(Err("no POSIX shell found. Use Git Bash or WSL."), None) + .expect("a host with no POSIX shell must be told"); + assert!(warning.contains("no POSIX shell found"), "{warning}"); + assert!(warning.contains("Git Bash"), "{warning}"); + } + + /// A shell that is missing a recipe tool names the tool and the shell whose + /// PATH was searched, and still points at the declared requirement — Git for + /// Windows resolves a shell but bundles no `jq`, which is exactly this case. + #[test] + fn a_missing_recipe_tool_warns_naming_the_tool_and_the_requirement() { + let warning = tooling_warning( + Ok(Path::new("/opt/Git/usr/bin/sh.exe")), + Some("jq is not on the PATH of /opt/Git/usr/bin/sh.exe"), + ) + .expect("a shell without `jq` must be told"); + assert!(warning.contains("jq is not on the PATH"), "{warning}"); + assert!(warning.contains("/opt/Git/usr/bin/sh.exe"), "{warning}"); + assert!(warning.contains("RUNBOOK.md"), "{warning}"); + assert!(warning.contains("Git Bash"), "{warning}"); + } + + /// A resolved shell wins over a stale tool reason: the shell branch is only + /// reachable when discovery itself failed. + #[test] + fn the_shell_failure_takes_precedence() { + let warning = tooling_warning(Err("no POSIX shell found."), Some("jq is missing")) + .expect("a missing shell warns"); + assert!(!warning.contains("jq is missing"), "{warning}"); + } +} diff --git a/src/cli/run/runbook.rs b/src/cli/run/runbook.rs index b8d4c92..93fd024 100644 --- a/src/cli/run/runbook.rs +++ b/src/cli/run/runbook.rs @@ -15,7 +15,7 @@ use std::path::Path; use crate::adapters::{CliDispatchContext, CliJudgeContext, RUNBOOK_TEMPLATE, adapter_for}; use crate::core::fs::artifact_path; -use crate::core::{Harness, Mode}; +use crate::core::{Harness, Mode, POSIX_TOOLING_REQUIREMENT}; use super::util::{harness_label, mode_str}; @@ -65,6 +65,9 @@ pub(crate) fn build_runbook(ctx: &RunbookContext) -> String { ("NUM_TASKS", &num_tasks), ("DISPATCH_JSON", &dispatch_json), ("BENCHMARK_PATH", &benchmark_path), + // Every recipe below this line is a POSIX command line, so the runbook + // names the shell it expects before the reader reaches one (issue #248). + ("POSIX_REQUIREMENT", POSIX_TOOLING_REQUIREMENT), ]; // A human pastes commands. The harness-specific dispatch + judge recipes come diff --git a/src/core/mod.rs b/src/core/mod.rs index b236e6c..e620085 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -17,7 +17,8 @@ pub mod types; pub use capabilities::HarnessRunCapabilities; pub use context::{ContextError, DetectInput, Harness, RunContext, detect_run_context}; pub(crate) use runtime::{ - GIT_ROUTING_ENV_VARS, clear_git_environment, posix_shell, validate_agent_environment_entry, + GIT_ROUTING_ENV_VARS, POSIX_RECIPE_TOOLS, POSIX_TOOLING_REQUIREMENT, clear_git_environment, + posix_shell, require_posix_toolchain, validate_agent_environment_entry, }; pub use runtime::{GitOutput, run_git}; pub use types::*; diff --git a/src/core/runtime.rs b/src/core/runtime.rs index 2a23b5e..ee3bcce 100644 --- a/src/core/runtime.rs +++ b/src/core/runtime.rs @@ -93,11 +93,21 @@ pub fn run_git(args: &[&str], cwd: &Path) -> GitOutput { } } -/// What to tell an operator who has no POSIX shell. Harness `exec_template`s -/// ship as POSIX command lines (`/mingw64/libexec/git-core` — hence three levels up). @@ -161,7 +171,7 @@ fn discover_posix_shell(override_path: Option<&OsStr>) -> Result) -> Result bool { true } -/// The resolved shell, once every tool in `tools` is reachable from inside it. +/// The toolchain a shipped recipe shells out to: the parallel-dispatch and judge +/// recipes are `jq` pipelines over `xargs`, `tr`, and `wc`. Both the `run` +/// preflight that warns about a gap and the tests that execute a rendered recipe +/// check this same list, so neither can drift from what the recipes actually use. +pub(crate) const POSIX_RECIPE_TOOLS: &[&str] = &["jq", "xargs", "tr", "wc"]; + +/// The resolved shell, once every tool in `tools` is reachable from inside it, +/// otherwise an error naming the first one that is not. /// /// The shipped parallel and judge recipes are POSIX pipelines over `jq`, -/// `xargs`, `tr`, and `wc`, so a test that executes one needs all of them. They -/// are checked through the shell rather than on the host `PATH` because that is -/// where the recipe will look: Git for Windows carries its own `/usr/bin`. -#[cfg(test)] +/// `xargs`, `tr`, and `wc`, so both a test that executes one and the `run` +/// preflight that warns about one need all of them. They are checked through the +/// shell rather than on the host `PATH` because that is where the recipe will +/// look: Git for Windows carries its own `/usr/bin`. pub(crate) fn require_posix_toolchain(tools: &[&str]) -> Result<&'static Path, String> { let shell = posix_shell().map_err(str::to_string)?; for tool in tools { @@ -322,42 +339,38 @@ mod tests { assert!(error.contains("/nonexistent-shell-for-tests"), "{error}"); assert!(error.contains("Git Bash"), "{error}"); assert!(error.contains("WSL"), "{error}"); + // The declared requirement is a POSIX shell *and* `jq`: Git for Windows + // supplies the shell but not `jq`, so naming only the shell would send + // an operator to a setup that still cannot run the judge recipe. + assert!(error.contains("jq"), "{error}"); } - /// Host-independent: either discovery finds a real shell, or it explains how - /// to install one. Asserting success outright would make the suite depend on - /// the developer's machine. + /// A POSIX shell is a declared development requirement, not a capability the + /// suite probes for: the scripted-turn tests spawn a `#!/bin/sh` harness stub + /// through the resolved shell and cannot skip. Asserting discovery outright + /// makes that requirement fail here — one line, naming the fix — instead of + /// somewhere deeper in the run boundary. #[test] - fn discover_posix_shell_returns_a_real_file_or_setup_guidance() { - match discover_posix_shell(None) { - Ok(shell) => assert!(shell.is_file(), "{} is not a file", shell.display()), - Err(error) => { - assert!(error.contains("Git Bash"), "{error}"); - assert!(error.contains("WSL"), "{error}"); - } - } + fn discover_posix_shell_finds_a_real_shell() { + let shell = discover_posix_shell(None).unwrap_or_else(|error| { + panic!("a POSIX shell is required to develop eval-magic: {error}") + }); + assert!(shell.is_file(), "{} is not a file", shell.display()); } #[test] fn require_posix_toolchain_names_the_tool_that_is_missing() { - let Ok(shell) = discover_posix_shell(None) else { - eprintln!("skipping: no POSIX shell on this host"); - return; - }; - let _ = shell; let error = require_posix_toolchain(&["eval-magic-not-a-real-tool"]) .expect_err("an uninstalled tool should be reported"); assert!(error.contains("eval-magic-not-a-real-tool"), "{error}"); } - /// With nothing to look for, the check reduces to locating the shell — so it - /// holds the same host-independent shape as discovery itself. + /// With nothing to look for, the check reduces to locating the shell — which + /// is required, so this succeeds wherever the suite is allowed to run. #[test] fn require_posix_toolchain_with_no_tools_reduces_to_finding_the_shell() { - match require_posix_toolchain(&[]) { - Ok(shell) => assert!(shell.is_file()), - Err(error) => assert!(error.contains("Git Bash"), "{error}"), - } + let shell = require_posix_toolchain(&[]).expect("the required POSIX shell resolves"); + assert!(shell.is_file()); } #[test] diff --git a/tests/cli/docs.rs b/tests/cli/docs.rs index 4deb44f..8f01c74 100644 --- a/tests/cli/docs.rs +++ b/tests/cli/docs.rs @@ -278,6 +278,32 @@ fn repository_documentation_map_names_each_surface() { assert!(agents.contains("docs/guides/")); assert!(agents.contains("docs/developer_overview.md")); assert!(!agents.contains("docs/README.md")); + + // A POSIX shell is a development requirement, not a probed capability: the + // scripted-turn tests spawn a `#!/bin/sh` stub through it and cannot skip. + // Both contributor-facing docs have to say so, or the next contributor on + // Windows rediscovers it as a test failure (issue #248). + for (name, text) in [("AGENTS.md", &agents), ("developer overview", &overview)] { + assert!( + text.contains("POSIX shell") && text.contains("jq"), + "{name} should record the POSIX shell + jq development requirement" + ); + } +} + +/// `--help` is the primary discovery surface, so the host requirement is +/// reachable there without installing anything or preparing a run first. +#[test] +fn help_states_the_posix_tooling_requirement() { + skill_eval() + .arg("--help") + .assert() + .success() + .stdout(contains("REQUIREMENTS:")) + .stdout(contains("POSIX shell")) + .stdout(contains("jq")) + .stdout(contains("Git Bash")) + .stdout(contains("WSL")); } #[test] @@ -296,12 +322,18 @@ fn readme_is_a_concise_first_run_path() { "eval-magic docs byoh", "eval-magic docs isolation", "docs/developer_overview.md", + // The declared host requirement, stated for both audiences the README + // serves: installing the tool, and developing it (issue #248). + "POSIX shell", + "Git Bash", + "WSL", + "jq", ] { assert!(readme.contains(expected), "README is missing {expected}"); } assert!( - readme.lines().count() <= 160, + readme.lines().count() <= 175, "README should hand detail to shipped docs instead of duplicating it" ); assert!(!readme.contains("## Harnesses")); diff --git a/tests/golden/claude-code/manifest-nomodel.golden.md b/tests/golden/claude-code/manifest-nomodel.golden.md index 7d9b082..ee467b2 100644 --- a/tests/golden/claude-code/manifest-nomodel.golden.md +++ b/tests/golden/claude-code/manifest-nomodel.golden.md @@ -8,6 +8,8 @@ Total dispatches: 2 In an agent session, read `dispatch.json` (sibling of this file) instead of this manifest. Each task has a `dispatch_prompt_path` field pointing at the file that holds the full prompt — dispatch the task with a short "read this file and follow it" instruction rather than inlining the prompt — plus exact paths for `run.json` and `timing.json`. +**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. + After all dispatches (Claude Code): Run one fresh `claude -p` per task from the env dir (`cd ` — `claude` has no --cd flag). `--output-format stream-json` requires `--verbose`; detach stdin with `` — `claude` has no --cd flag). `--output-format stream-json` requires `--verbose`; detach stdin with ` **Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. + - **Skill under test:** widget-skill - **Mode:** revision — comparing `old_skill` vs `new_skill` - **Dispatches:** 6 (the `tasks[]` array in `/work/.eval-magic/widget-skill/iteration-2/dispatch.json`) diff --git a/tests/golden/cline/manifest.golden.md b/tests/golden/cline/manifest.golden.md index 1d5737e..95c8bf5 100644 --- a/tests/golden/cline/manifest.golden.md +++ b/tests/golden/cline/manifest.golden.md @@ -8,6 +8,8 @@ Total dispatches: 2 In an agent session, read `dispatch.json` (sibling of this file) instead of this manifest. Each task has a `dispatch_prompt_path` field pointing at the file that holds the full prompt — dispatch the task with a short "read this file and follow it" instruction rather than inlining the prompt — plus exact paths for `run.json` and `timing.json`. +**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. + After all dispatches (Cline): Run one fresh `cline --cwd --act --json --auto-approve true` per task. Detach stdin with `` so piped task data cannot become extra prompt context; capture stdout as `outputs/cline-events.jsonl` and stderr as `outputs/cline-stderr.log`. The trailing jq step recovers `outputs/final-message.md` from the terminal `run_result` event. diff --git a/tests/golden/cline/runbook.golden.md b/tests/golden/cline/runbook.golden.md index 0edc81c..54a4c0d 100644 --- a/tests/golden/cline/runbook.golden.md +++ b/tests/golden/cline/runbook.golden.md @@ -4,6 +4,8 @@ This runbook is for a human driving the run from a terminal. Work from this iter and copy-paste each step. The workspace is self-contained — you should not need the surrounding repo. +> **Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. + - **Skill under test:** widget-skill - **Mode:** revision — comparing `old_skill` vs `new_skill` - **Dispatches:** 6 (the `tasks[]` array in `/work/.eval-magic/widget-skill/iteration-2/dispatch.json`) diff --git a/tests/golden/codex/manifest-noguard.golden.md b/tests/golden/codex/manifest-noguard.golden.md index 14d8cdb..010061d 100644 --- a/tests/golden/codex/manifest-noguard.golden.md +++ b/tests/golden/codex/manifest-noguard.golden.md @@ -8,6 +8,8 @@ Total dispatches: 2 In an agent session, read `dispatch.json` (sibling of this file) instead of this manifest. Each task has a `dispatch_prompt_path` field pointing at the file that holds the full prompt — dispatch the task with a short "read this file and follow it" instruction rather than inlining the prompt — plus exact paths for `run.json` and `timing.json`. +**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. + After all dispatches (Codex): Run one fresh `codex --ask-for-approval never exec --json` per task. Detach stdin with ` **Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. + - **Skill under test:** widget-skill - **Mode:** revision — comparing `old_skill` vs `new_skill` - **Dispatches:** 6 (the `tasks[]` array in `/work/.eval-magic/widget-skill/iteration-2/dispatch.json`) diff --git a/tests/golden/opencode/manifest.golden.md b/tests/golden/opencode/manifest.golden.md index 2e3dc08..10b81fd 100644 --- a/tests/golden/opencode/manifest.golden.md +++ b/tests/golden/opencode/manifest.golden.md @@ -8,6 +8,8 @@ Total dispatches: 2 In an agent session, read `dispatch.json` (sibling of this file) instead of this manifest. Each task has a `dispatch_prompt_path` field pointing at the file that holds the full prompt — dispatch the task with a short "read this file and follow it" instruction rather than inlining the prompt — plus exact paths for `run.json` and `timing.json`. +**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. + After all dispatches (OpenCode): Run one fresh `opencode run --format json --auto` per task. Detach stdin with ` **Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. + - **Skill under test:** widget-skill - **Mode:** revision — comparing `old_skill` vs `new_skill` - **Dispatches:** 6 (the `tasks[]` array in `/work/.eval-magic/widget-skill/iteration-2/dispatch.json`) diff --git a/tests/run/runbook.rs b/tests/run/runbook.rs index 3e48a30..230f371 100644 --- a/tests/run/runbook.rs +++ b/tests/run/runbook.rs @@ -75,6 +75,47 @@ fn run_writes_headless_runbook_for_claude() { "headless does not use the in-session batch loop: {book}" ); assert!(!book.contains("{{"), "no unsubstituted tokens: {book}"); + + // Issue #248: the runbook is the manual for a campaign, so it names the + // shell it expects — and names it *above* the first command a reader would + // paste, which is the whole point of stating the requirement at all. + let requirement = book + .find("Git Bash") + .expect("the runbook states the POSIX shell requirement"); + assert!(book.contains("jq"), "the requirement names jq too: {book}"); + assert!(book.contains("WSL"), "{book}"); + assert!( + requirement < book.find("claude -p").unwrap(), + "the requirement precedes the first pasteable command: {book}" + ); +} + +/// Issue #248: `run` used to succeed on a host with no POSIX shell and print +/// recipes only a POSIX shell can execute, with nothing to say a different shell +/// was expected. The prepared workspace is still correct, so this warns rather +/// than failing — but it must warn, and it must name the way out. +/// +/// `EVAL_MAGIC_SH` pointing at nothing reproduces the shell-less host on every +/// platform, so the test does not depend on what the developer has installed. +#[test] +fn run_warns_when_the_host_has_no_posix_shell() { + let tmp = tempfile::TempDir::new().unwrap(); + let (skill_dir, cwd) = setup(tmp.path(), DEFAULT_EVALS); + skill_eval() + .current_dir(&cwd) + .env("EVAL_MAGIC_SH", tmp.path().join("no-shell-here")) + .args(["run", "--skill-dir"]) + .arg(&skill_dir) + .args([ + "--skill", + "mr-review", + "--harness", + "claude-code", + "--dry-run", + ]) + .assert() + .success() + .stderr(contains("⚠").and(contains("Git Bash")).and(contains("WSL"))); } #[test] @@ -110,4 +151,10 @@ fn run_writes_headless_runbook_for_opencode() { !manifest.contains("{{"), "no unsubstituted tokens: {manifest}" ); + // The manifest carries the same POSIX recipes, so it carries the same + // requirement (issue #248 names both artifacts). + assert!( + manifest.contains("Git Bash") && manifest.contains("jq"), + "the manifest states the POSIX tooling requirement: {manifest}" + ); } From 7a07772f27e7a8549f4ff3b5ff630f6cff3cc52e Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Sat, 15 Aug 2026 01:12:57 -0400 Subject: [PATCH 11/18] fix(run): set core.longpaths on runner-owned task repositories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A staged `.claude/skills//SKILL.md` under a deep workspace crosses Windows' 260-character MAX_PATH, and the task repository's deliberate configuration isolation (GIT_CONFIG_NOSYSTEM plus an empty GIT_CONFIG_GLOBAL) discards the core.longpaths an operator set globally. The runner now writes its own. The reported failure was the loud band — `git add` aborting the run with `fatal: unable to stat '...SKILL.md': Filename too long`. Reproducing it end to end surfaced a quieter one a few characters deeper: git cannot open the staged directory to enumerate it, so `git add` warns, exits zero, and commits a baseline that does not contain the skill under test. The cleanliness check cannot catch that, since git reports nothing about a file it could not read, so every later diff would be measured against a baseline missing its subject. The setting is written twice: to the repository, so the agent under test and the pipeline's own `run_git` calls inherit it, and transiently on each invocation, so `git init` is covered before that local config exists. Initialization failures under a deep root now also name the path budget, keyed on the measured root length rather than on git's localizable `Filename too long`. Fixes #270 Co-Authored-By: Claude Opus 5 --- src/cli/run/orchestrate/git.rs | 185 ++++++++++++++++++++++++++++++++- tests/run/git_isolation.rs | 9 ++ 2 files changed, 193 insertions(+), 1 deletion(-) diff --git a/src/cli/run/orchestrate/git.rs b/src/cli/run/orchestrate/git.rs index 0513c9c..0737f68 100644 --- a/src/cli/run/orchestrate/git.rs +++ b/src/cli/run/orchestrate/git.rs @@ -18,6 +18,16 @@ const BASELINE_NAME: &str = "eval-magic"; const BASELINE_EMAIL: &str = "eval-magic@localhost"; const BASELINE_DATE: &str = "2000-01-01T00:00:00Z"; +/// Windows' `MAX_PATH` (260) counts the terminating NUL, so 259 characters are +/// what a tool that is not long-path aware can actually use. +const WINDOWS_USABLE_PATH: usize = 259; + +/// What a run still writes below a task root before its deepest file: +/// `\.claude\skills\\SKILL.md` — 68 characters for the short slug +/// in issue #270, 85 for a long skill and condition pair. Rounded up so the +/// hint below arrives while the budget is nearly gone rather than after. +const STAGED_SUFFIX_BUDGET: usize = 96; + pub(super) fn preflight_git(ctx: &RunContext) -> Result<(), RunError> { let output = run_git(&["--version"], &ctx.skill_subdir); if output.status == Some(0) { @@ -40,8 +50,11 @@ pub(super) fn initialize_task_repositories(resolved: &Resolved) -> Result<(), Ru }); for target in targets { initialize_task_repository(&target.root).map_err(|error| { + let hint = path_budget_hint(&target.root, cfg!(windows)) + .map(|hint| format!("\n{hint}")) + .unwrap_or_default(); RunError::msg(format!( - "could not initialize task Git repository at {}: {error}", + "could not initialize task Git repository at {}: {error}{hint}", target.root.display() )) })?; @@ -49,6 +62,26 @@ pub(super) fn initialize_task_repositories(resolved: &Resolved) -> Result<(), Ru Ok(()) } +/// A sentence naming the Windows path budget, for a task root already deep +/// enough that what a run stages below it will not fit. +/// +/// Keyed on the measured root rather than on git's `Filename too long`: that +/// wording is git's own `strerror` mapping, so matching it would tie the hint to +/// one locale. The root's length is a local fact, which lets the hint stay +/// definite about the budget and hedged about the cause. +fn path_budget_hint(root: &Path, windows: bool) -> Option { + let length = root.as_os_str().to_string_lossy().chars().count(); + if !windows || length + STAGED_SUFFIX_BUDGET <= WINDOWS_USABLE_PATH { + return None; + } + Some(format!( + "This task root is {length} characters and a run stages roughly \ + {STAGED_SUFFIX_BUDGET} more below it, past the {WINDOWS_USABLE_PATH} Windows \ + allows a tool that is not long-path aware. If the failure above names a path \ + or filename length, re-run from a shorter workspace root." + )) +} + fn initialize_task_repository(root: &Path) -> Result<(), String> { remove_existing_git_dir(root)?; @@ -90,6 +123,14 @@ fn initialize_task_repository(root: &Path) -> Result<(), String> { ("commit.gpgSign", OsString::from("false")), ("tag.gpgSign", OsString::from("false")), ("core.hooksPath", hooks_dir.into_os_string()), + // A staged skill under a deep workspace crosses Windows' `MAX_PATH` + // (issue #270), and the configuration isolation above discards the + // `core.longpaths` an operator set globally — so the runner writes its + // own. It belongs in the repository rather than on each invocation: the + // agent under test runs its own git in here, and the pipeline reads the + // repository back through `run_git`, so both inherit it. Written on + // every host; git ignores the key off Windows. + ("core.longpaths", OsString::from("true")), ] { run_checked( root, @@ -225,6 +266,10 @@ fn run_checked( ) -> Result { let mut command = Command::new("git"); command + // `git init` creates `.git/objects/pack` before the repository-local + // `core.longpaths` exists to lift it, so that one invocation needs the + // setting passed transiently. + .args(["-c", "core.longpaths=true"]) .args(args.iter().map(OsString::as_os_str)) .current_dir(cwd) .env("GIT_CONFIG_NOSYSTEM", "1") @@ -267,3 +312,141 @@ fn git_diagnostic(status: Option, stderr: &[u8]) -> String { (None, true) => "could not start git".to_string(), } } + +#[cfg(test)] +mod tests { + use super::*; + + use crate::core::runtime::report_skip; + + /// The staged path issue #270 failed on, relative to a task root: 68 + /// characters, the shortest realistic shape of + /// `.claude/skills//SKILL.md`. + const STAGED_SKILL: &str = + ".claude/skills/slow-powers-eval-1-with_skill__widget-skill/SKILL.md"; + + /// `base` extended with padding components until it is `target` characters + /// long (or left as it is, when it is already longer). + fn padded_to(base: &Path, target: usize) -> PathBuf { + const PAD: &str = "eval-magic-path-budget-padding"; + let mut root = base.to_path_buf(); + while root.as_os_str().len() + 1 + PAD.len() <= target { + root = root.join(PAD); + } + let remaining = target.saturating_sub(root.as_os_str().len() + 1); + if remaining > 0 { + root = root.join(&PAD[..remaining]); + } + root + } + + /// A `target`-character task root holding a staged `SKILL.md`, or `None` + /// when this host cannot write that deep. + /// + /// Gated on the capability rather than the OS: the probe is the same + /// `std::fs` write staging performs, so a host that cannot do it says so + /// instead of failing somewhere inside git. + fn deep_task_root(base: &Path, target: usize, test: &str) -> Option { + let root = padded_to(base, target); + let staged = root.join(STAGED_SKILL); + let written = fs::create_dir_all(staged.parent().expect("the staged path has a parent")) + .and_then(|()| fs::write(&staged, "---\nname: widget-skill\n---\n\nbody\n")); + if let Err(error) = written { + report_skip( + test, + &format!( + "this host cannot create a {}-character path ({error})", + staged.as_os_str().len() + ), + ); + return None; + } + Some(root) + } + + /// Issue #270: the run staged its skill correctly — Rust's own filesystem + /// calls pass verbatim paths, which lifts Windows' `MAX_PATH` — and then + /// aborted at the baseline `git add` with `Filename too long`. The + /// runner-owned repository has to carry `core.longpaths` itself: its + /// deliberate configuration isolation discards the one an operator set + /// globally. + #[test] + fn task_repository_initializes_when_the_staged_skill_exceeds_the_windows_path_limit() { + let test = + "task_repository_initializes_when_the_staged_skill_exceeds_the_windows_path_limit"; + let tmp = tempfile::TempDir::new().unwrap(); + // 195 characters puts the staged path past the 259 Windows allows a + // caller that is not long-path aware, while the repository's own `.git` + // bookkeeping stays under it — the shape reported in #270. + let Some(root) = deep_task_root(tmp.path(), 195, test) else { + return; + }; + assert!( + root.join(STAGED_SKILL).as_os_str().len() > WINDOWS_USABLE_PATH, + "the fixture must exceed the Windows path budget to exercise anything" + ); + initialize_task_repository(&root) + .expect("a task root with a deep staged skill initializes"); + } + + /// A failure under a deep root has to name the path budget: git reports + /// `Filename too long` about one file, which says nothing about the + /// workspace root being the thing to shorten. + #[test] + fn path_budget_hint_names_the_budget_for_a_deep_windows_root() { + let root = padded_to(Path::new("C:/w"), 210); + let hint = path_budget_hint(&root, true).expect("a deep Windows root gets a hint"); + assert!(hint.contains("210"), "{hint}"); + assert!(hint.contains(&WINDOWS_USABLE_PATH.to_string()), "{hint}"); + assert!(hint.contains("shorter workspace root"), "{hint}"); + } + + /// The hint is a Windows path-budget explanation, so it stays out of the way + /// of every failure it cannot explain. + #[test] + fn path_budget_hint_stays_silent_off_windows_and_for_short_roots() { + let deep = padded_to(Path::new("C:/w"), 210); + assert_eq!(path_budget_hint(&deep, false), None); + assert_eq!(path_budget_hint(Path::new("C:/w/iteration-1"), true), None); + } + + /// A few characters deeper the failure goes quiet instead of loud: git can + /// no longer open the staged directory to enumerate it, so `git add` warns, + /// exits zero, and leaves the skill under test out of the baseline — and the + /// cleanliness check that follows cannot report a file git could not read. + /// The baseline every later diff is measured against would silently lack the + /// thing under test. + #[test] + fn task_repository_baseline_tracks_a_staged_skill_past_the_windows_path_limit() { + let test = "task_repository_baseline_tracks_a_staged_skill_past_the_windows_path_limit"; + let tmp = tempfile::TempDir::new().unwrap(); + // 202 characters is the narrow band that isolates the quiet mode: + // enumerating the staged directory needs 261, past the budget, while the + // repository's own loose objects still fit at 256. Post-fix the + // assertion holds at any depth; the band is what makes it fail without. + let Some(root) = deep_task_root(tmp.path(), 202, test) else { + return; + }; + initialize_task_repository(&root).expect("a task root in the quiet band initializes"); + let tracked = run_git(&["ls-files"], &root); + assert!( + String::from_utf8_lossy(&tracked.stdout).contains("SKILL.md"), + "the baseline commit must track the staged skill, not skip it" + ); + } + + /// One step deeper: the repository's own `.git/objects/pack` crosses the + /// budget too, so `git init` — which runs before the repository-local + /// configuration exists — has to be long-path aware in its own right. + #[test] + fn task_repository_initializes_when_its_git_directory_exceeds_the_windows_path_limit() { + let test = + "task_repository_initializes_when_its_git_directory_exceeds_the_windows_path_limit"; + let tmp = tempfile::TempDir::new().unwrap(); + let Some(root) = deep_task_root(tmp.path(), 245, test) else { + return; + }; + initialize_task_repository(&root) + .expect("a task root deeper than `.git` needs initializes"); + } +} diff --git a/tests/run/git_isolation.rs b/tests/run/git_isolation.rs index 6e203c6..27ce4ac 100644 --- a/tests/run/git_isolation.rs +++ b/tests/run/git_isolation.rs @@ -77,6 +77,15 @@ fn every_task_is_a_clean_local_git_repo_inside_a_dirty_ignored_parent_repo() { assert_eq!(git(eval_root, &["symbolic-ref", "--short", "HEAD"]), "work"); assert_eq!(git(eval_root, &["status", "--porcelain"]), ""); assert_eq!(git(eval_root, &["remote"]), ""); + // A staged skill under a deep workspace crosses Windows' MAX_PATH + // (issue #270), and the repository's configuration isolation discards + // the `core.longpaths` an operator set globally — so the runner writes + // its own. Asserted on every host: CI runs Linux, where a deep path + // proves nothing but a dropped setting still would. + assert_eq!( + git(eval_root, &["config", "--local", "--get", "core.longpaths"]), + "true" + ); assert_eq!( git( eval_root, From e4d52ea85dfa3aeed048f8cce2ed1a58101c0606 Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Sat, 15 Aug 2026 01:23:13 -0400 Subject: [PATCH 12/18] docs(run): tighten the long-path comments Drop the issue references, which date and point away from the code, and cut each comment down to the line or item it sits on so none of them needs a neighbour to make sense. Co-Authored-By: Claude Opus 5 --- src/cli/run/orchestrate/git.rs | 83 ++++++++++++++-------------------- tests/run/git_isolation.rs | 8 ++-- 2 files changed, 36 insertions(+), 55 deletions(-) diff --git a/src/cli/run/orchestrate/git.rs b/src/cli/run/orchestrate/git.rs index 0737f68..6155c81 100644 --- a/src/cli/run/orchestrate/git.rs +++ b/src/cli/run/orchestrate/git.rs @@ -22,10 +22,9 @@ const BASELINE_DATE: &str = "2000-01-01T00:00:00Z"; /// what a tool that is not long-path aware can actually use. const WINDOWS_USABLE_PATH: usize = 259; -/// What a run still writes below a task root before its deepest file: -/// `\.claude\skills\\SKILL.md` — 68 characters for the short slug -/// in issue #270, 85 for a long skill and condition pair. Rounded up so the -/// hint below arrives while the budget is nearly gone rather than after. +/// Length of what a run writes below a task root before its deepest file, +/// `\.claude\skills\\SKILL.md`: 68 characters for a short slug, 85 +/// for a long skill and condition pair, rounded up. const STAGED_SUFFIX_BUDGET: usize = 96; pub(super) fn preflight_git(ctx: &RunContext) -> Result<(), RunError> { @@ -62,13 +61,11 @@ pub(super) fn initialize_task_repositories(resolved: &Resolved) -> Result<(), Ru Ok(()) } -/// A sentence naming the Windows path budget, for a task root already deep -/// enough that what a run stages below it will not fit. +/// A sentence naming the Windows path budget, for a task root too deep to hold +/// what a run stages below it. /// -/// Keyed on the measured root rather than on git's `Filename too long`: that -/// wording is git's own `strerror` mapping, so matching it would tie the hint to -/// one locale. The root's length is a local fact, which lets the hint stay -/// definite about the budget and hedged about the cause. +/// Measures the root rather than matching git's `Filename too long`, which is a +/// localizable `strerror` mapping. fn path_budget_hint(root: &Path, windows: bool) -> Option { let length = root.as_os_str().to_string_lossy().chars().count(); if !windows || length + STAGED_SUFFIX_BUDGET <= WINDOWS_USABLE_PATH { @@ -123,13 +120,11 @@ fn initialize_task_repository(root: &Path) -> Result<(), String> { ("commit.gpgSign", OsString::from("false")), ("tag.gpgSign", OsString::from("false")), ("core.hooksPath", hooks_dir.into_os_string()), - // A staged skill under a deep workspace crosses Windows' `MAX_PATH` - // (issue #270), and the configuration isolation above discards the - // `core.longpaths` an operator set globally — so the runner writes its - // own. It belongs in the repository rather than on each invocation: the - // agent under test runs its own git in here, and the pipeline reads the - // repository back through `run_git`, so both inherit it. Written on - // every host; git ignores the key off Windows. + // Lifts Windows' `MAX_PATH`, which a staged skill under a deep workspace + // crosses. Task repositories run under isolated Git configuration, so an + // operator's own setting never reaches one. Written to the repository, + // not per invocation, so the agent under test and the pipeline inherit + // it; git ignores the key off Windows. ("core.longpaths", OsString::from("true")), ] { run_checked( @@ -266,9 +261,8 @@ fn run_checked( ) -> Result { let mut command = Command::new("git"); command - // `git init` creates `.git/objects/pack` before the repository-local - // `core.longpaths` exists to lift it, so that one invocation needs the - // setting passed transiently. + // `git init` creates `.git/objects/pack` before any repository-local + // configuration exists, so the long-path lift rides on the invocation. .args(["-c", "core.longpaths=true"]) .args(args.iter().map(OsString::as_os_str)) .current_dir(cwd) @@ -319,9 +313,8 @@ mod tests { use crate::core::runtime::report_skip; - /// The staged path issue #270 failed on, relative to a task root: 68 - /// characters, the shortest realistic shape of - /// `.claude/skills//SKILL.md`. + /// A staged skill's path relative to its task root: 68 characters, the + /// shortest realistic shape of `.claude/skills//SKILL.md`. const STAGED_SKILL: &str = ".claude/skills/slow-powers-eval-1-with_skill__widget-skill/SKILL.md"; @@ -341,11 +334,8 @@ mod tests { } /// A `target`-character task root holding a staged `SKILL.md`, or `None` - /// when this host cannot write that deep. - /// - /// Gated on the capability rather than the OS: the probe is the same - /// `std::fs` write staging performs, so a host that cannot do it says so - /// instead of failing somewhere inside git. + /// when this host cannot write that deep. The probe is the same `std::fs` + /// write staging performs, so the gate is the capability, not the OS. fn deep_task_root(base: &Path, target: usize, test: &str) -> Option { let root = padded_to(base, target); let staged = root.join(STAGED_SKILL); @@ -364,20 +354,16 @@ mod tests { Some(root) } - /// Issue #270: the run staged its skill correctly — Rust's own filesystem - /// calls pass verbatim paths, which lifts Windows' `MAX_PATH` — and then - /// aborted at the baseline `git add` with `Filename too long`. The - /// runner-owned repository has to carry `core.longpaths` itself: its - /// deliberate configuration isolation discards the one an operator set - /// globally. + /// Rust's filesystem calls pass verbatim paths, so a deep workspace stages + /// its skill fine and only git meets Windows' `MAX_PATH` — the baseline + /// `git add` aborts with `Filename too long`. #[test] fn task_repository_initializes_when_the_staged_skill_exceeds_the_windows_path_limit() { let test = "task_repository_initializes_when_the_staged_skill_exceeds_the_windows_path_limit"; let tmp = tempfile::TempDir::new().unwrap(); - // 195 characters puts the staged path past the 259 Windows allows a - // caller that is not long-path aware, while the repository's own `.git` - // bookkeeping stays under it — the shape reported in #270. + // 195 characters puts the staged path past the budget while the + // repository's own `.git` bookkeeping stays under it. let Some(root) = deep_task_root(tmp.path(), 195, test) else { return; }; @@ -410,20 +396,17 @@ mod tests { assert_eq!(path_budget_hint(Path::new("C:/w/iteration-1"), true), None); } - /// A few characters deeper the failure goes quiet instead of loud: git can - /// no longer open the staged directory to enumerate it, so `git add` warns, - /// exits zero, and leaves the skill under test out of the baseline — and the - /// cleanliness check that follows cannot report a file git could not read. - /// The baseline every later diff is measured against would silently lack the - /// thing under test. + /// Past a certain depth the failure goes quiet: git cannot open the staged + /// directory to enumerate it, so `git add` warns, exits zero, and leaves the + /// skill under test out of the baseline that later diffs are measured + /// against. Nothing downstream can flag a file git could not read. #[test] fn task_repository_baseline_tracks_a_staged_skill_past_the_windows_path_limit() { let test = "task_repository_baseline_tracks_a_staged_skill_past_the_windows_path_limit"; let tmp = tempfile::TempDir::new().unwrap(); - // 202 characters is the narrow band that isolates the quiet mode: - // enumerating the staged directory needs 261, past the budget, while the - // repository's own loose objects still fit at 256. Post-fix the - // assertion holds at any depth; the band is what makes it fail without. + // 202 characters isolates the quiet mode: enumerating the staged + // directory needs 261, past the budget, while the repository's own loose + // objects still fit at 256. let Some(root) = deep_task_root(tmp.path(), 202, test) else { return; }; @@ -435,9 +418,9 @@ mod tests { ); } - /// One step deeper: the repository's own `.git/objects/pack` crosses the - /// budget too, so `git init` — which runs before the repository-local - /// configuration exists — has to be long-path aware in its own right. + /// A root deep enough that `.git/objects/pack` crosses the budget: `git + /// init` creates it before any repository-local configuration exists, so the + /// lift has to reach that invocation too. #[test] fn task_repository_initializes_when_its_git_directory_exceeds_the_windows_path_limit() { let test = diff --git a/tests/run/git_isolation.rs b/tests/run/git_isolation.rs index 27ce4ac..e69c756 100644 --- a/tests/run/git_isolation.rs +++ b/tests/run/git_isolation.rs @@ -77,11 +77,9 @@ fn every_task_is_a_clean_local_git_repo_inside_a_dirty_ignored_parent_repo() { assert_eq!(git(eval_root, &["symbolic-ref", "--short", "HEAD"]), "work"); assert_eq!(git(eval_root, &["status", "--porcelain"]), ""); assert_eq!(git(eval_root, &["remote"]), ""); - // A staged skill under a deep workspace crosses Windows' MAX_PATH - // (issue #270), and the repository's configuration isolation discards - // the `core.longpaths` an operator set globally — so the runner writes - // its own. Asserted on every host: CI runs Linux, where a deep path - // proves nothing but a dropped setting still would. + // Without this, a staged skill under a deep workspace hits Windows' + // MAX_PATH. Asserted on every host, since a Linux runner cannot prove it + // with a deep path but can still catch the setting going missing. assert_eq!( git(eval_root, &["config", "--local", "--get", "core.longpaths"]), "true" From e446cecf2cd76af07485c63daf8bbda63ca8750c Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Sat, 15 Aug 2026 02:36:52 -0400 Subject: [PATCH 13/18] fix(recipe): strip the carriage return a Windows jq emits jq's native Windows build opens stdout in text mode, so every `\n` it writes arrives as `\r\n`. Both shipped recipes read that output as paths, and neither reader drops the CR: `read -r` keeps it by definition, and `tr '\n' '\0'` converts only the newline. The carriage return then rides on the end of every path. The judge recipe reports `0/N verdicts present` because `[ -s "$response_path" ]` matches nothing, and the parallel-dispatch recipe hands every task a corrupted `eval_root`, `dispatch_prompt_path`, and `outputs_dir`. Git Bash with `jq` is the setup the tooling requirement documents, so this is the supported Windows path rather than an exotic one. Pipe each jq call through `tr -d '\r'`: a no-op wherever jq already writes LF, and `tr` is a declared requirement alongside `jq` itself. The regression test defines a CRLF-emitting `jq` as a shell function in front of the recipe rather than shimming `PATH`, so the failure mode is covered on every host instead of only where a Windows jq is installed. Co-Authored-By: Claude Opus 5 --- src/adapters/cli_command.rs | 91 ++++++++++++++++++- src/adapters/descriptor_adapter.rs | 4 +- .../golden/claude-code/judge-recipe.golden.md | 4 +- .../claude-code/manifest-nomodel.golden.md | 1 + tests/golden/claude-code/manifest.golden.md | 1 + tests/golden/claude-code/runbook.golden.md | 4 +- tests/golden/cline/judge-recipe.golden.md | 4 +- tests/golden/cline/manifest.golden.md | 1 + tests/golden/cline/runbook.golden.md | 4 +- .../codex/judge-recipe-noguard.golden.md | 4 +- tests/golden/codex/judge-recipe.golden.md | 4 +- tests/golden/codex/manifest-noguard.golden.md | 1 + tests/golden/codex/manifest.golden.md | 1 + tests/golden/codex/runbook.golden.md | 4 +- tests/golden/opencode/judge-recipe.golden.md | 4 +- tests/golden/opencode/manifest.golden.md | 1 + tests/golden/opencode/runbook.golden.md | 4 +- 17 files changed, 125 insertions(+), 12 deletions(-) diff --git a/src/adapters/cli_command.rs b/src/adapters/cli_command.rs index 6fcf39a..41e99d6 100644 --- a/src/adapters/cli_command.rs +++ b/src/adapters/cli_command.rs @@ -70,6 +70,10 @@ pub(crate) fn render_cli_model_arg(flag: Option<&str>, model: Option<&str>) -> S /// instead, where they do not survive argument passing — jq then emits no /// separators and `xargs -0` collapses every field into one bogus dispatch /// that exits 0. Paths containing a newline remain unsupported, as before. +/// +/// `tr -d '\r'` runs before that: jq's native Windows build writes CRLF, and +/// `tr '\n' '\0'` converts only the newline, so without it every path reaches +/// the dispatch with a carriage return on the end. pub(crate) fn render_parallel_dispatch_recipe( command_block: &str, one_shot_only: bool, @@ -85,6 +89,7 @@ pub(crate) fn render_parallel_dispatch_recipe( format!( "jq -r '{tasks} | .eval_root, .dispatch_prompt_path, .outputs_dir' dispatch.json \\" ), + " | tr -d '\\r' \\".to_string(), " | tr '\\n' '\\0' \\".to_string(), " | xargs -0 -P \"$JOBS\" -n 3 sh -c '".to_string(), " eval_root=\"$1\"".to_string(), @@ -109,6 +114,11 @@ pub(crate) fn render_parallel_dispatch_recipe( /// model, ` ` otherwise) and end with ` \`; `model_flag` fills the /// `model_arg` assignment; `capture_prefix` names the per-task /// `$response_base.-events.jsonl` / `.-stderr.log` captures. +/// +/// Every jq call is piped through `tr -d '\r'`: jq's native Windows build +/// writes CRLF, and none of the three readers here drop it — `read -r` keeps a +/// carriage return by definition, `tr '\n' '\0'` converts only the newline, and +/// `[ "$judge_present" -eq "$judge_total" ]` needs a bare integer. pub(crate) fn render_judge_dispatch_recipe( command_line: &str, model_flag: &str, @@ -124,6 +134,7 @@ pub(crate) fn render_judge_dispatch_recipe( "```bash".to_string(), "JOBS=${JOBS:-4}".to_string(), "jq -r '.tasks[] | .dispatch_prompt_path, .response_path, (\"model=\" + (.model // \"\"))' judge-tasks.json \\".to_string(), + " | tr -d '\\r' \\".to_string(), " | tr '\\n' '\\0' \\".to_string(), " | xargs -0 -P \"$JOBS\" -n 3 sh -c '".to_string(), " prompt_path=\"$1\"".to_string(), @@ -140,9 +151,10 @@ pub(crate) fn render_judge_dispatch_recipe( format!(" 2> \"$response_base.{capture_prefix}-stderr.log\""), " ' sh".to_string(), "judge_dispatch_status=$?".to_string(), - "judge_total=$(jq '.tasks | length' judge-tasks.json)".to_string(), + "judge_total=$(jq '.tasks | length' judge-tasks.json | tr -d '\\r')".to_string(), "judge_present=$(".to_string(), " jq -r '.tasks[].response_path' judge-tasks.json \\".to_string(), + " | tr -d '\\r' \\".to_string(), " | while IFS= read -r response_path; do".to_string(), " if [ -s \"$response_path\" ]; then printf '%s\\n' \"$response_path\"; fi" .to_string(), @@ -208,7 +220,26 @@ mod tests { } } + /// A `jq` that ends every line with CRLF, the way jq's native Windows build + /// does with stdout in text mode. A shell function rather than a `PATH` + /// shim: nothing has to be marked executable, so it reads and behaves the + /// same on every host, and only the outer pipeline calls jq — the `xargs` + /// child never does, so it does not need the definition. + const CRLF_JQ: &str = "jq() { command jq \"$@\" | tr -d '\\r' \ + | while IFS= read -r line; do printf '%s\\r\\n' \"$line\"; done; }\n"; + fn run_judge_recipe(shell: &Path, cwd: &Path, command_line: &str) -> Output { + run_judge_recipe_prefixed(shell, cwd, command_line, "") + } + + /// Run the rendered recipe with `preamble` in front of it, so a test can + /// replace a tool the recipe shells out to. + fn run_judge_recipe_prefixed( + shell: &Path, + cwd: &Path, + command_line: &str, + preamble: &str, + ) -> Output { let recipe = render_judge_dispatch_recipe(command_line, "--model", "judge"); let program = recipe .split_once("```bash\n") @@ -218,7 +249,7 @@ mod tests { .unwrap(); Command::new(shell) .arg("-c") - .arg(program) + .arg(format!("{preamble}{program}")) .current_dir(cwd) .env("JOBS", "1") .output() @@ -373,6 +404,62 @@ mod tests { ); } + /// jq's native Windows build opens stdout in text mode, so every `\n` it + /// writes arrives as `\r\n`. The recipes read that output as paths, and + /// neither reader drops the CR: `read -r` keeps it by definition, and + /// `tr '\n' '\0'` converts only the newline. The carriage return then rides + /// on the end of every path — `[ -s "$response_path" ]` matches nothing, the + /// summary reports zero verdicts present, and each dispatched task gets a + /// corrupted `$eval_root`. Git Bash with `jq` installed is the documented + /// Windows setup, so jq's output has to be normalised before anything reads + /// it. Same expectations as the plain-jq partial-completion test above; only + /// jq's line endings differ. + #[test] + fn judge_recipe_counts_verdicts_when_jq_emits_crlf() { + let Some(shell) = recipe_shell("judge_recipe_counts_verdicts_when_jq_emits_crlf") else { + return; + }; + let tmp = tempfile::TempDir::new().unwrap(); + let responses_dir = tmp.path().join("judge responses"); + fs::create_dir_all(&responses_dir).unwrap(); + let existing_response = responses_dir.join("existing.json"); + let missing_response = responses_dir.join("missing.json"); + fs::write(&existing_response, "{}\n").unwrap(); + write_judge_tasks(tmp.path(), &[&existing_response, &missing_response]); + + let output = + run_judge_recipe_prefixed(shell, tmp.path(), " true $model_arg \\", CRLF_JQ); + + assert!(!output.status.success(), "{output:?}"); + assert_eq!( + String::from_utf8(output.stdout).unwrap(), + "1/2 verdicts present\n", + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + } + + /// Every recipe that reads jq's output has to strip the carriage return + /// jq's Windows build adds, and it has to do so once per call: the judge + /// recipe pipes jq into `xargs`, into `read`, and into an arithmetic + /// comparison, and a CR left on any one of them breaks that stage alone. + #[test] + fn recipes_strip_the_carriage_return_a_windows_jq_emits() { + for recipe in [ + render_parallel_dispatch_recipe(" run \"$eval_root\"", false, &BTreeMap::new()), + render_parallel_dispatch_recipe(" run \"$eval_root\"", true, &BTreeMap::new()), + render_judge_dispatch_recipe(" judge $model_arg \\", "--model", "judge"), + ] { + let calls = recipe.lines().filter(|line| line.contains("jq ")).count(); + assert!(calls > 0, "{recipe}"); + assert_eq!( + calls, + recipe.matches("tr -d '\\r'").count(), + "every jq call needs its own CR strip\n{recipe}" + ); + } + } + #[test] fn judge_recipe_reports_complete_resumed_batch_and_exits_zero() { let Some(shell) = diff --git a/src/adapters/descriptor_adapter.rs b/src/adapters/descriptor_adapter.rs index 811939f..4399499 100644 --- a/src/adapters/descriptor_adapter.rs +++ b/src/adapters/descriptor_adapter.rs @@ -799,6 +799,7 @@ The final `N/M verdicts present` summary exits nonzero until every task has one. ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // ""))' judge-tasks.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' prompt_path="$1" @@ -815,9 +816,10 @@ jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // 2> "$response_base.claude-stderr.log" ' sh judge_dispatch_status=$? -judge_total=$(jq '.tasks | length' judge-tasks.json) +judge_total=$(jq '.tasks | length' judge-tasks.json | tr -d '\r') judge_present=$( jq -r '.tasks[].response_path' judge-tasks.json \ + | tr -d '\r' \ | while IFS= read -r response_path; do if [ -s "$response_path" ]; then printf '%s\n' "$response_path"; fi done \ diff --git a/tests/golden/claude-code/judge-recipe.golden.md b/tests/golden/claude-code/judge-recipe.golden.md index 7719cd2..c4f66ad 100644 --- a/tests/golden/claude-code/judge-recipe.golden.md +++ b/tests/golden/claude-code/judge-recipe.golden.md @@ -5,6 +5,7 @@ The final `N/M verdicts present` summary exits nonzero until every task has one. ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // ""))' judge-tasks.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' prompt_path="$1" @@ -21,9 +22,10 @@ jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // 2> "$response_base.claude-stderr.log" ' sh judge_dispatch_status=$? -judge_total=$(jq '.tasks | length' judge-tasks.json) +judge_total=$(jq '.tasks | length' judge-tasks.json | tr -d '\r') judge_present=$( jq -r '.tasks[].response_path' judge-tasks.json \ + | tr -d '\r' \ | while IFS= read -r response_path; do if [ -s "$response_path" ]; then printf '%s\n' "$response_path"; fi done \ diff --git a/tests/golden/claude-code/manifest-nomodel.golden.md b/tests/golden/claude-code/manifest-nomodel.golden.md index ee467b2..9d30538 100644 --- a/tests/golden/claude-code/manifest-nomodel.golden.md +++ b/tests/golden/claude-code/manifest-nomodel.golden.md @@ -28,6 +28,7 @@ Parallel dispatch from this iteration directory: ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .eval_root, .dispatch_prompt_path, .outputs_dir' dispatch.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' eval_root="$1" diff --git a/tests/golden/claude-code/manifest.golden.md b/tests/golden/claude-code/manifest.golden.md index 867ae65..a72f47f 100644 --- a/tests/golden/claude-code/manifest.golden.md +++ b/tests/golden/claude-code/manifest.golden.md @@ -28,6 +28,7 @@ Parallel dispatch from this iteration directory: ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .eval_root, .dispatch_prompt_path, .outputs_dir' dispatch.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' eval_root="$1" diff --git a/tests/golden/claude-code/runbook.golden.md b/tests/golden/claude-code/runbook.golden.md index a6a0cdf..265e33a 100644 --- a/tests/golden/claude-code/runbook.golden.md +++ b/tests/golden/claude-code/runbook.golden.md @@ -34,6 +34,7 @@ The final `N/M verdicts present` summary exits nonzero until every task has one. ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // ""))' judge-tasks.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' prompt_path="$1" @@ -50,9 +51,10 @@ jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // 2> "$response_base.claude-stderr.log" ' sh judge_dispatch_status=$? -judge_total=$(jq '.tasks | length' judge-tasks.json) +judge_total=$(jq '.tasks | length' judge-tasks.json | tr -d '\r') judge_present=$( jq -r '.tasks[].response_path' judge-tasks.json \ + | tr -d '\r' \ | while IFS= read -r response_path; do if [ -s "$response_path" ]; then printf '%s\n' "$response_path"; fi done \ diff --git a/tests/golden/cline/judge-recipe.golden.md b/tests/golden/cline/judge-recipe.golden.md index 85af0b8..355fb2d 100644 --- a/tests/golden/cline/judge-recipe.golden.md +++ b/tests/golden/cline/judge-recipe.golden.md @@ -5,6 +5,7 @@ The final `N/M verdicts present` summary exits nonzero until every task has one. ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // ""))' judge-tasks.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' prompt_path="$1" @@ -21,9 +22,10 @@ jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // 2> "$response_base.cline-stderr.log" ' sh judge_dispatch_status=$? -judge_total=$(jq '.tasks | length' judge-tasks.json) +judge_total=$(jq '.tasks | length' judge-tasks.json | tr -d '\r') judge_present=$( jq -r '.tasks[].response_path' judge-tasks.json \ + | tr -d '\r' \ | while IFS= read -r response_path; do if [ -s "$response_path" ]; then printf '%s\n' "$response_path"; fi done \ diff --git a/tests/golden/cline/manifest.golden.md b/tests/golden/cline/manifest.golden.md index 95c8bf5..df07631 100644 --- a/tests/golden/cline/manifest.golden.md +++ b/tests/golden/cline/manifest.golden.md @@ -30,6 +30,7 @@ Parallel dispatch from this iteration directory: ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .eval_root, .dispatch_prompt_path, .outputs_dir' dispatch.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' eval_root="$1" diff --git a/tests/golden/cline/runbook.golden.md b/tests/golden/cline/runbook.golden.md index 54a4c0d..f858583 100644 --- a/tests/golden/cline/runbook.golden.md +++ b/tests/golden/cline/runbook.golden.md @@ -36,6 +36,7 @@ The final `N/M verdicts present` summary exits nonzero until every task has one. ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // ""))' judge-tasks.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' prompt_path="$1" @@ -52,9 +53,10 @@ jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // 2> "$response_base.cline-stderr.log" ' sh judge_dispatch_status=$? -judge_total=$(jq '.tasks | length' judge-tasks.json) +judge_total=$(jq '.tasks | length' judge-tasks.json | tr -d '\r') judge_present=$( jq -r '.tasks[].response_path' judge-tasks.json \ + | tr -d '\r' \ | while IFS= read -r response_path; do if [ -s "$response_path" ]; then printf '%s\n' "$response_path"; fi done \ diff --git a/tests/golden/codex/judge-recipe-noguard.golden.md b/tests/golden/codex/judge-recipe-noguard.golden.md index 0293a75..ee5233c 100644 --- a/tests/golden/codex/judge-recipe-noguard.golden.md +++ b/tests/golden/codex/judge-recipe-noguard.golden.md @@ -5,6 +5,7 @@ The final `N/M verdicts present` summary exits nonzero until every task has one. ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // ""))' judge-tasks.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' prompt_path="$1" @@ -21,9 +22,10 @@ jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // 2> "$response_base.codex-stderr.log" ' sh judge_dispatch_status=$? -judge_total=$(jq '.tasks | length' judge-tasks.json) +judge_total=$(jq '.tasks | length' judge-tasks.json | tr -d '\r') judge_present=$( jq -r '.tasks[].response_path' judge-tasks.json \ + | tr -d '\r' \ | while IFS= read -r response_path; do if [ -s "$response_path" ]; then printf '%s\n' "$response_path"; fi done \ diff --git a/tests/golden/codex/judge-recipe.golden.md b/tests/golden/codex/judge-recipe.golden.md index 0293a75..ee5233c 100644 --- a/tests/golden/codex/judge-recipe.golden.md +++ b/tests/golden/codex/judge-recipe.golden.md @@ -5,6 +5,7 @@ The final `N/M verdicts present` summary exits nonzero until every task has one. ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // ""))' judge-tasks.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' prompt_path="$1" @@ -21,9 +22,10 @@ jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // 2> "$response_base.codex-stderr.log" ' sh judge_dispatch_status=$? -judge_total=$(jq '.tasks | length' judge-tasks.json) +judge_total=$(jq '.tasks | length' judge-tasks.json | tr -d '\r') judge_present=$( jq -r '.tasks[].response_path' judge-tasks.json \ + | tr -d '\r' \ | while IFS= read -r response_path; do if [ -s "$response_path" ]; then printf '%s\n' "$response_path"; fi done \ diff --git a/tests/golden/codex/manifest-noguard.golden.md b/tests/golden/codex/manifest-noguard.golden.md index 010061d..117a33c 100644 --- a/tests/golden/codex/manifest-noguard.golden.md +++ b/tests/golden/codex/manifest-noguard.golden.md @@ -29,6 +29,7 @@ Parallel dispatch from this iteration directory: ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .eval_root, .dispatch_prompt_path, .outputs_dir' dispatch.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' eval_root="$1" diff --git a/tests/golden/codex/manifest.golden.md b/tests/golden/codex/manifest.golden.md index 8ca9b68..7c802ff 100644 --- a/tests/golden/codex/manifest.golden.md +++ b/tests/golden/codex/manifest.golden.md @@ -29,6 +29,7 @@ Parallel dispatch from this iteration directory: ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .eval_root, .dispatch_prompt_path, .outputs_dir' dispatch.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' eval_root="$1" diff --git a/tests/golden/codex/runbook.golden.md b/tests/golden/codex/runbook.golden.md index 91a128f..959daf3 100644 --- a/tests/golden/codex/runbook.golden.md +++ b/tests/golden/codex/runbook.golden.md @@ -35,6 +35,7 @@ The final `N/M verdicts present` summary exits nonzero until every task has one. ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // ""))' judge-tasks.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' prompt_path="$1" @@ -51,9 +52,10 @@ jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // 2> "$response_base.codex-stderr.log" ' sh judge_dispatch_status=$? -judge_total=$(jq '.tasks | length' judge-tasks.json) +judge_total=$(jq '.tasks | length' judge-tasks.json | tr -d '\r') judge_present=$( jq -r '.tasks[].response_path' judge-tasks.json \ + | tr -d '\r' \ | while IFS= read -r response_path; do if [ -s "$response_path" ]; then printf '%s\n' "$response_path"; fi done \ diff --git a/tests/golden/opencode/judge-recipe.golden.md b/tests/golden/opencode/judge-recipe.golden.md index 5faa659..d7ce316 100644 --- a/tests/golden/opencode/judge-recipe.golden.md +++ b/tests/golden/opencode/judge-recipe.golden.md @@ -5,6 +5,7 @@ The final `N/M verdicts present` summary exits nonzero until every task has one. ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // ""))' judge-tasks.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' prompt_path="$1" @@ -21,9 +22,10 @@ jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // 2> "$response_base.opencode-stderr.log" ' sh judge_dispatch_status=$? -judge_total=$(jq '.tasks | length' judge-tasks.json) +judge_total=$(jq '.tasks | length' judge-tasks.json | tr -d '\r') judge_present=$( jq -r '.tasks[].response_path' judge-tasks.json \ + | tr -d '\r' \ | while IFS= read -r response_path; do if [ -s "$response_path" ]; then printf '%s\n' "$response_path"; fi done \ diff --git a/tests/golden/opencode/manifest.golden.md b/tests/golden/opencode/manifest.golden.md index 10b81fd..a25dd60 100644 --- a/tests/golden/opencode/manifest.golden.md +++ b/tests/golden/opencode/manifest.golden.md @@ -28,6 +28,7 @@ Parallel dispatch from this iteration directory: ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .eval_root, .dispatch_prompt_path, .outputs_dir' dispatch.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' eval_root="$1" diff --git a/tests/golden/opencode/runbook.golden.md b/tests/golden/opencode/runbook.golden.md index 23a7c1b..23efe8a 100644 --- a/tests/golden/opencode/runbook.golden.md +++ b/tests/golden/opencode/runbook.golden.md @@ -34,6 +34,7 @@ The final `N/M verdicts present` summary exits nonzero until every task has one. ```bash JOBS=${JOBS:-4} jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // ""))' judge-tasks.json \ + | tr -d '\r' \ | tr '\n' '\0' \ | xargs -0 -P "$JOBS" -n 3 sh -c ' prompt_path="$1" @@ -50,9 +51,10 @@ jq -r '.tasks[] | .dispatch_prompt_path, .response_path, ("model=" + (.model // 2> "$response_base.opencode-stderr.log" ' sh judge_dispatch_status=$? -judge_total=$(jq '.tasks | length' judge-tasks.json) +judge_total=$(jq '.tasks | length' judge-tasks.json | tr -d '\r') judge_present=$( jq -r '.tasks[].response_path' judge-tasks.json \ + | tr -d '\r' \ | while IFS= read -r response_path; do if [ -s "$response_path" ]; then printf '%s\n' "$response_path"; fi done \ From 191e231e61a0811c3ffa0a9e65037379979ffcc8 Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Sat, 15 Aug 2026 02:37:03 -0400 Subject: [PATCH 14/18] ci: run the suite on windows-latest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Releases attach a Windows binary and the README documents a PowerShell installer, but both jobs ran on ubuntu-latest, so every Windows fix so far was found by hand rather than by the pipeline. Matrix the test job across ubuntu-latest and windows-latest with `fail-fast: false`, since a Linux failure cancelling the Windows leg would lose the result exactly when it is worth having. Clippy earns its place on both: the `#[cfg(windows)]` arms in `core::fs` and `command_check` are lint-invisible on Ubuntu. `EVAL_MAGIC_REQUIRE_POSIX_TOOLS` applies to both runners, so the Windows host is provisioned for the gated capabilities rather than exempted from them: `jq`, which Git for Windows does not bundle, and Developer Mode for symlink creation. Long paths need nothing — task repositories carry their own `core.longpaths`, and `LongPathsEnabled` stays unset deliberately so those tests keep proving what a default box does. `EVAL_MAGIC_SH` stays unset for the same reason: discovering the shell from the Git install root is what a Windows user hits. The guard test pins all of it together, because a matrix entry whose enforcement variable went missing would report green while covering six fewer tests than it appears to. Also corrects both contributor docs, which still described two gated capabilities before long-path staging became the third. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 34 +++++++++++++++++++++++++++++----- AGENTS.md | 16 +++++++++------- docs/developer_overview.md | 5 +++-- tests/cli/package.rs | 22 ++++++++++++++++++++++ 4 files changed, 63 insertions(+), 14 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 952e98c..191bf69 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -16,7 +16,13 @@ env: jobs: test: name: Test suite - runs-on: ubuntu-latest + strategy: + # Windows is the platform this matrix exists to cover, so a Linux failure + # must not cancel it — that is exactly when its result is worth having. + fail-fast: false + matrix: + os: [ubuntu-latest, windows-latest] + runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v4 - name: Install Rust toolchain @@ -25,15 +31,33 @@ jobs: components: rustfmt, clippy - name: Cache cargo build uses: Swatinem/rust-cache@v2 + - name: Install jq + # Git for Windows supplies sh, xargs, tr, and wc, but not jq, and the + # judge-recipe tests execute the shipped pipeline text rather than a + # stand-in for it. + if: runner.os == 'Windows' + run: choco install jq --yes --no-progress + - name: Permit symlink creation + # Windows creates symlinks only under Developer Mode or elevation, and + # the core::fs round-trips need one. Asking for it explicitly beats + # depending on how the runner's token happens to be built. + if: runner.os == 'Windows' + run: > + reg add "HKLM\SOFTWARE\Microsoft\Windows\CurrentVersion\AppModelUnlock" + /t REG_DWORD /f /v AllowDevelopmentWithoutDevLicense /d 1 - name: Format check run: cargo fmt --all -- --check - name: Clippy run: cargo clippy --all-targets --all-features -- -D warnings - name: Test - # Capability-gated tests (the POSIX recipe pipelines, symlink round-trips) - # skip with a printed reason on a host that lacks the tool. This turns - # every such skip into a failure, so CI can never quietly stop covering - # them. Ubuntu runners ship jq, xargs, tr, and wc. + # Capability-gated tests (the POSIX recipe pipelines, symlink + # round-trips, long-path staging) skip with a printed reason on a host + # that lacks the capability. This turns every such skip into a failure, + # so neither runner can quietly stop covering them. Ubuntu ships the + # recipe tools; the steps above provide them on Windows. Long paths need + # no provisioning — the runner passes core.longpaths to git itself. + # EVAL_MAGIC_SH stays unset deliberately: discovering the shell from the + # Git install root is what a Windows user hits, so CI should run it too. env: EVAL_MAGIC_REQUIRE_POSIX_TOOLS: 1 run: cargo test --all-targets diff --git a/AGENTS.md b/AGENTS.md index 9b61d91..4dd5acb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -63,13 +63,15 @@ binary, `cargo test --lib` alone does not build it — run `cargo test`, or `car compilation and clippy on the other host and hides the coverage gap. Instead, probe for what the test actually needs and call `report_skip` (`src/core/runtime.rs`), which prints the reason and returns `true`. Setting `EVAL_MAGIC_REQUIRE_POSIX_TOOLS=1` turns every skip into a failure; CI sets -it, so a runner cannot quietly stop covering something. Two capabilities are gated today: the recipe -tools beyond the shell itself (`require_posix_toolchain` — in practice `jq`), and symlink creation, -which Windows allows only under Developer Mode. The shell is not one of them; it is a hard -requirement, per the section below. `require_posix_toolchain` is not test-only either — the `run` -preflight uses it to warn about the same gap. Where a genuine per-OS difference is the behavior under -test — signals, path separators — branch on `cfg!(windows)` at runtime so both arms still compile -everywhere. +it on both runners, so neither can quietly stop covering something. Three capabilities are gated +today: the recipe tools beyond the shell itself (`require_posix_toolchain` — in practice `jq`), +symlink creation, which Windows allows only under Developer Mode, and creating a path past +Windows' 259-character limit (`deep_task_root`, `src/cli/run/orchestrate/git.rs`). The Windows +runner is provisioned for those rather than exempted from them, so a skip there is a red build. The +shell is not one of them; it is a hard requirement, per the section below. +`require_posix_toolchain` is not test-only either — the `run` preflight uses it to warn about the +same gap. Where a genuine per-OS difference is the behavior under test — signals, path separators — +branch on `cfg!(windows)` at runtime so both arms still compile everywhere. **A POSIX shell is required, for use and for development.** Harness `exec_template`s are POSIX command lines, so the dispatch and probe paths spawn `sh` via `posix_shell()` diff --git a/docs/developer_overview.md b/docs/developer_overview.md index 8b57d73..0ecc36c 100644 --- a/docs/developer_overview.md +++ b/docs/developer_overview.md @@ -75,8 +75,9 @@ boundaries unless the evidence requires a named harness capability. Development carries the host requirement the tool itself declares: a POSIX shell with `jq`. The scripted-turn tests spawn `#!/bin/sh` harness stubs through the resolved shell and do not skip, so -the suite cannot pass without one. Tests needing `jq` or symlink creation report a skip instead; -`EVAL_MAGIC_REQUIRE_POSIX_TOOLS=1` turns those skips into failures, as CI sets it to do. +the suite cannot pass without one. Tests needing `jq`, symlink creation, or a path past Windows' +259-character limit report a skip instead; `EVAL_MAGIC_REQUIRE_POSIX_TOOLS=1` turns those skips into +failures, as CI sets it to do on both its Ubuntu and its Windows runner. Before handing work off, run: diff --git a/tests/cli/package.rs b/tests/cli/package.rs index 089be8b..b5149e7 100644 --- a/tests/cli/package.rs +++ b/tests/cli/package.rs @@ -70,6 +70,28 @@ fn ci_publishes_default_branch_coverage_for_readme_badge() { } } +/// Releases attach a Windows binary, so the suite has to run on Windows — but +/// the matrix entry alone proves nothing. Six of those tests are gated on +/// capabilities the runner has to be handed: `jq` for the judge recipes, and +/// symlink creation for the `core::fs` round-trips. Without the enforcement +/// variable they skip in silence, and the job reports green while covering +/// strictly less than it looks like it is. Every string below is load-bearing, +/// which is why they are pinned together rather than one standing for the rest. +#[test] +fn ci_runs_the_suite_on_windows_with_capability_skips_enforced() { + let workflow = read_repo_file(".github/workflows/ci.yml"); + + for expected in [ + "os: [ubuntu-latest, windows-latest]", + "fail-fast: false", + "EVAL_MAGIC_REQUIRE_POSIX_TOOLS: 1", + "choco install jq", + "AllowDevelopmentWithoutDevLicense", + ] { + assert!(workflow.contains(expected), "CI is missing {expected}"); + } +} + #[test] fn cargo_package_excludes_repo_local_authoring_files() { let cargo = std::env::var("CARGO").unwrap_or_else(|_| "cargo".to_string()); From 5bb6d275b7bee907c5f8e888c3e508cf74cd7155 Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Sat, 15 Aug 2026 02:52:11 -0400 Subject: [PATCH 15/18] test(run): measure deep task roots the way git measures them The long-path tests padded from `std::env::temp_dir()`, which can hand back an 8.3 short name: a GitHub runner's `%TEMP%` lives under `RUNNER~1`, while git expands that to `runneradmin` before it measures. Three characters no length computed here could see. That was enough to move the deepest test from one side of a ceiling to the other. It passed locally with `.git/config` at 257 and failed on the runner with the same target at 260, which reads as a Windows-only product failure and is nothing of the sort. Canonicalise the base first, so the padding measures the spelling git will report. Then step the root back to 244 and assert the window it has to sit in, because git's long-path awareness turns out to be per-operation: creating `.git/objects/pack` survives well past the budget, `git init` writing `.git/config` stops exactly at it, and the `git config --local` that follows gives up two characters earlier still. The old literal sat one character inside the tightest of those, with nothing recording that it did. Co-Authored-By: Claude Opus 5 --- src/cli/run/orchestrate/git.rs | 42 ++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/src/cli/run/orchestrate/git.rs b/src/cli/run/orchestrate/git.rs index 6155c81..e6e2084 100644 --- a/src/cli/run/orchestrate/git.rs +++ b/src/cli/run/orchestrate/git.rs @@ -333,11 +333,28 @@ mod tests { root } + /// `base` spelled the way git will report it. + /// + /// `std::env::temp_dir()` can hand back an 8.3 short name — `RUNNER~1` for + /// `runneradmin` on a GitHub runner — which git expands before it measures. + /// Those three characters are invisible to a length computed from the short + /// spelling, and three is enough to push `.git/config` past the limit on a + /// host where the same target fits locally. Measure what git measures. + fn long_form(base: &Path) -> PathBuf { + let Ok(canonical) = base.canonicalize() else { + return base.to_path_buf(); + }; + let text = canonical.to_string_lossy().into_owned(); + // Canonicalising on Windows yields a `\\?\` verbatim path; git reports + // the plain spelling, so drop the prefix to keep the two comparable. + PathBuf::from(text.strip_prefix(r"\\?\").unwrap_or(&text)) + } + /// A `target`-character task root holding a staged `SKILL.md`, or `None` /// when this host cannot write that deep. The probe is the same `std::fs` /// write staging performs, so the gate is the capability, not the OS. fn deep_task_root(base: &Path, target: usize, test: &str) -> Option { - let root = padded_to(base, target); + let root = padded_to(&long_form(base), target); let staged = root.join(STAGED_SKILL); let written = fs::create_dir_all(staged.parent().expect("the staged path has a parent")) .and_then(|()| fs::write(&staged, "---\nname: widget-skill\n---\n\nbody\n")); @@ -421,14 +438,35 @@ mod tests { /// A root deep enough that `.git/objects/pack` crosses the budget: `git /// init` creates it before any repository-local configuration exists, so the /// lift has to reach that invocation too. + /// + /// 244 is not arbitrary and not the maximum. Git's long-path awareness is + /// per-operation: creating `.git/objects/pack` survives well past the + /// budget, `git init` writing `.git/config` stops at exactly + /// `WINDOWS_USABLE_PATH`, and the `git config --local` that follows gives up + /// two characters earlier still. 244 puts `pack` at 262 — past the budget, + /// which is the point — while leaving `.git/config` at 256, a deliberate + /// three inside the tightest of those ceilings. The assertions below pin the + /// window so a future edit cannot silently slide the root out of it. #[test] fn task_repository_initializes_when_its_git_directory_exceeds_the_windows_path_limit() { + const GIT_CONFIG: &str = ".git/config"; + const GIT_PACK: &str = ".git/objects/pack"; let test = "task_repository_initializes_when_its_git_directory_exceeds_the_windows_path_limit"; let tmp = tempfile::TempDir::new().unwrap(); - let Some(root) = deep_task_root(tmp.path(), 245, test) else { + let Some(root) = deep_task_root(tmp.path(), 244, test) else { return; }; + + let length = root.as_os_str().len(); + assert!( + length + 1 + GIT_PACK.len() > WINDOWS_USABLE_PATH, + "{length}-character root leaves `.git/objects/pack` inside the budget, testing nothing" + ); + assert!( + length + 1 + GIT_CONFIG.len() + 3 <= WINDOWS_USABLE_PATH, + "{length}-character root leaves `.git/config` no margin below the budget" + ); initialize_task_repository(&root) .expect("a task root deeper than `.git` needs initializes"); } From 4147ca8689a170735402b3a401f0704ad2de301a Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Sat, 15 Aug 2026 15:28:02 -0400 Subject: [PATCH 16/18] fix(run): resolve a run's cwd to one path spelling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POSIX gives this away for free: `getcwd` resolves symlinks, so a Unix process and everything it spawns already agree on how the working directory is spelled. Windows makes no such promise — it hands back whatever spelling the cwd was set with — and one directory there has several valid names: an 8.3 short name, a junction, a `subst` drive, a redirected profile. Nothing forced the two sides of the write guard's comparison onto one of them. Measured, spawning each tool the way a harness is spawned: node's `process.cwd()`, node's `realpathSync`, and `cmd`'s `cd` all echo the alias back, while git prints the resolved name for every path it emits. Both spellings are therefore reachable from inside a single task env, and `is_under` compares strings. Driving the hook with a short-form root, a write relative to a long-form cwd is denied, as is an absolute long-form target under the env — legitimate writes, refused. A genuine escape is still denied, so the guard never got weaker, only falsely strict. Every task env is a git repo, which makes `rev-parse --show-toplevel` a one-step route to the refused spelling. Resolve once, where the run's roots are derived, rather than at either end of the comparison: canonicalising only the marker inverts the failure instead of removing it, denying the same write when the cwd is the alias. Every path in a `RunContext` now shares one spelling, which makes it a property of the struct rather than of the one field someone remembered. Resolution walks up to the deepest ancestor that exists and re-attaches the rest, because a run names directories before it creates them and the alias always lives in an ancestor, never in the leaf. The cli fixtures that built roots with a bare `fs::canonicalize` now use the helper that strips the verbatim prefix. They had been comparing against `\?\` paths, a spelling no agent ever produces and one the CLI no longer emits. Co-Authored-By: Claude Opus 5 --- src/core/context.rs | 99 +++++++++++++++++++---- src/core/fs.rs | 157 +++++++++++++++++++++++++++++++++++-- tests/cli/guard_denials.rs | 4 +- tests/cli/helpers.rs | 29 ++++--- tests/cli/stray_writes.rs | 12 +-- 5 files changed, 262 insertions(+), 39 deletions(-) diff --git a/src/core/context.rs b/src/core/context.rs index f22fab4..760437f 100644 --- a/src/core/context.rs +++ b/src/core/context.rs @@ -94,9 +94,10 @@ pub enum ContextError { Io(#[from] std::io::Error), } -/// Lexically absolutize a path (join onto cwd if relative; normalize `.`/`..`). -/// Mirrors node's `resolve()` — it does NOT resolve symlinks or require -/// existence, unlike `std::fs::canonicalize`. +/// Absolutize a path (join onto `cwd` if relative) and resolve it to the one +/// spelling every path in a [`RunContext`] shares — see +/// [`crate::core::fs::real_path`]. Routing every path through here is what makes +/// that a property of the struct rather than of the one field someone remembered. fn absolutize(cwd: &Path, p: &str) -> Result { let path = Path::new(p); let joined = if path.is_absolute() { @@ -104,7 +105,7 @@ fn absolutize(cwd: &Path, p: &str) -> Result { } else { cwd.join(path) }; - Ok(std::path::absolute(joined)?) + Ok(crate::core::fs::real_path(&joined)?) } fn skill_name_from_dir(skill_subdir: &Path) -> Result { @@ -162,8 +163,10 @@ fn infer_only_skill_name(skill_dir: &Path) -> Result { /// validates `SKILL.md`, an optional existing `--bootstrap`, and defaults the /// workspace/stage roots from the current directory. pub fn detect_run_context(input: DetectInput) -> Result { - let cwd = input.cwd.unwrap_or(std::env::current_dir()?); - let cwd = std::path::absolute(cwd)?; + let cwd = input.cwd.map_or_else(std::env::current_dir, Ok)?; + // Every root below derives from this one path, so resolving the alias here + // is what keeps a run's paths and an agent's paths comparable at all. + let cwd = crate::core::fs::real_path(&cwd)?; let (skill_dir, skill_name, skill_subdir, sibling_skill_names, stage_siblings) = match input.skill_dir { Some(skill_dir_raw) => { @@ -292,7 +295,7 @@ mod tests { assert_eq!(ctx.skill_name, "mr-review"); assert_eq!( ctx.skill_subdir, - std::path::absolute(&skill_subdir).unwrap() + crate::core::fs::real_path(&skill_subdir).unwrap() ); assert!(ctx.sibling_skill_names.is_empty()); assert!(!ctx.stage_siblings); @@ -313,7 +316,7 @@ mod tests { assert_eq!(ctx.skill_name, "beta"); assert_eq!( ctx.skill_subdir, - std::path::absolute(skill_dir.join("beta")).unwrap() + crate::core::fs::real_path(&skill_dir.join("beta")).unwrap() ); assert!(ctx.sibling_skill_names.is_empty()); assert!(!ctx.stage_siblings); @@ -414,11 +417,14 @@ mod tests { let tmp = TempDir::new().unwrap(); let skill_dir = make_skill_dir(tmp.path(), &["mr-review"]); let ctx = detect_run_context(input(&skill_dir, "mr-review")).unwrap(); - assert_eq!(ctx.skill_dir, std::path::absolute(&skill_dir).unwrap()); + assert_eq!( + ctx.skill_dir, + crate::core::fs::real_path(&skill_dir).unwrap() + ); assert_eq!(ctx.skill_name, "mr-review"); assert_eq!( ctx.skill_subdir, - std::path::absolute(skill_dir.join("mr-review")).unwrap() + crate::core::fs::real_path(&skill_dir.join("mr-review")).unwrap() ); assert!(ctx.sibling_skill_names.is_empty()); assert!(ctx.bootstrap_path.is_none()); @@ -452,8 +458,8 @@ mod tests { let tmp = TempDir::new().unwrap(); let skill_dir = make_skill_dir(tmp.path(), &["foo"]); let ctx = detect_run_context(input(&skill_dir, "foo")).unwrap(); - let expected = std::env::current_dir().unwrap().join(".eval-magic"); - assert_eq!(ctx.workspace_root, expected); + let cwd = crate::core::fs::real_path(&std::env::current_dir().unwrap()).unwrap(); + assert_eq!(ctx.workspace_root, cwd.join(".eval-magic")); } #[test] @@ -467,7 +473,10 @@ mod tests { ..input(&skill_dir, "foo") }) .unwrap(); - assert_eq!(ctx.workspace_root, std::path::absolute(&custom).unwrap()); + assert_eq!( + ctx.workspace_root, + crate::core::fs::real_path(&custom).unwrap() + ); } #[test] @@ -475,7 +484,67 @@ mod tests { let tmp = TempDir::new().unwrap(); let skill_dir = make_skill_dir(tmp.path(), &["foo"]); let ctx = detect_run_context(input(&skill_dir, "foo")).unwrap(); - assert_eq!(ctx.stage_root, std::env::current_dir().unwrap()); + assert_eq!( + ctx.stage_root, + crate::core::fs::real_path(&std::env::current_dir().unwrap()).unwrap() + ); + } + + /// Every root derives from the cwd, and the guard later compares those roots + /// against paths the agent's own tools report — so an alias of the cwd has to + /// collapse here, once, or the two sides disagree forever after. + /// + /// Windows spells one directory several ways (8.3 short names, junctions, + /// `subst` drives, redirected profiles); each is one `canonicalize` apart + /// from the real path, so exercising one exercises the mechanism. + #[test] + fn a_cwd_alias_collapses_so_every_derived_root_shares_one_spelling() { + let tmp = TempDir::new().unwrap(); + let real = tmp.path().join("real-workspace"); + fs::create_dir_all(&real).unwrap(); + let alias = tmp.path().join("alias-workspace"); + crate::core::fs::create_directory_alias(&real, &alias).unwrap(); + make_skill_dir(&real, &["foo"]); + + // Enter through the alias, exactly as a user whose workspace sits under a + // junction or a redirected profile directory does. + let ctx = detect_run_context(DetectInput { + skill: Some("foo".to_string()), + ..input_from(&alias.join("skill-dir")) + }) + .unwrap(); + + let expected = crate::core::fs::real_path(&real).unwrap(); + assert_eq!(ctx.stage_root, expected.join("skill-dir")); + assert_eq!( + ctx.workspace_root, + expected.join("skill-dir").join(".eval-magic") + ); + assert_eq!(ctx.skill_dir, expected.join("skill-dir")); + } + + /// `--workspace-dir` is the second way into the same tree: the guard's roots + /// descend from it, so an alias passed here would reintroduce the split the + /// cwd resolution just closed. + #[test] + fn an_aliased_workspace_dir_flag_resolves_to_the_same_spelling() { + let tmp = TempDir::new().unwrap(); + let real = tmp.path().join("real-workspace"); + fs::create_dir_all(&real).unwrap(); + let alias = tmp.path().join("alias-workspace"); + crate::core::fs::create_directory_alias(&real, &alias).unwrap(); + let skill_dir = make_skill_dir(tmp.path(), &["foo"]); + + let ctx = detect_run_context(DetectInput { + workspace_dir: Some(alias.join("nested-ws").to_string_lossy().into_owned()), + ..input(&skill_dir, "foo") + }) + .unwrap(); + + assert_eq!( + ctx.workspace_root, + crate::core::fs::real_path(&real).unwrap().join("nested-ws") + ); } #[test] @@ -491,7 +560,7 @@ mod tests { .unwrap(); assert_eq!( ctx.bootstrap_path, - Some(std::path::absolute(&bootstrap).unwrap()) + Some(crate::core::fs::real_path(&bootstrap).unwrap()) ); } diff --git a/src/core/fs.rs b/src/core/fs.rs index 24c0fda..ec2f010 100644 --- a/src/core/fs.rs +++ b/src/core/fs.rs @@ -23,7 +23,7 @@ use std::fs; use std::io; -use std::path::Path; +use std::path::{Path, PathBuf}; use serde::Serialize; @@ -49,16 +49,64 @@ pub fn artifact_path(path: &Path) -> String { if !cfg!(windows) { return rendered.into_owned(); } - let unprefixed = match rendered.strip_prefix(r"\\?\UNC\") { + normalize_separators(&strip_verbatim_prefix(&rendered)) +} + +/// Drop Windows' verbatim (`\\?\`) prefix, keeping the host's own separators. +fn strip_verbatim_prefix(rendered: &str) -> String { + match rendered.strip_prefix(r"\\?\UNC\") { // Verbatim UNC collapses back to the `\\server\share` form; dropping // the whole prefix would leave a bare `UNC\` component. Some(rest) => format!(r"\\{rest}"), None => rendered .strip_prefix(r"\\?\") - .unwrap_or(&rendered) + .unwrap_or(rendered) .to_string(), - }; - normalize_separators(&unprefixed) + } +} + +/// The one spelling of `path` that every participant in a run agrees on. +/// +/// POSIX hands this out for free: `getcwd` resolves symlinks, so a Unix process +/// and everything it spawns already share one spelling of the working directory. +/// Windows makes no such promise — it hands back whatever spelling the cwd was +/// set with — and one directory there has several valid names: an 8.3 short +/// name (`RUNNER~1`), a junction, a `subst` drive, a redirected profile +/// directory. Tools then disagree about which to report: `git` prints the +/// resolved name, while node's `process.cwd()` and `cmd`'s `cd` echo the alias +/// back. Anything comparing those strings — the write guard's allowed roots +/// against the paths an agent's own tools hand it — silently stops matching. +/// +/// Resolving once, at the point a run's roots are derived, gives Windows the +/// guarantee POSIX already provides. The verbatim (`\\?\`) prefix comes off +/// because a spawned child reports the plain form, so plain is the spelling the +/// comparisons actually see. +/// +/// A run names directories before it creates them, so resolution walks up to the +/// deepest ancestor that exists and re-attaches the rest: the alias always lives +/// in an ancestor — a temp dir, a junction, an 8.3 profile name — never in the +/// leaf about to be created. With no ancestor on disk at all, the lexical form +/// is all there is. +pub fn real_path(path: &Path) -> io::Result { + let absolute = std::path::absolute(path)?; + let mut unresolved = Vec::new(); + let mut anchor = absolute.as_path(); + loop { + if let Ok(canonical) = fs::canonicalize(anchor) { + let mut resolved = if cfg!(windows) { + PathBuf::from(strip_verbatim_prefix(&canonical.to_string_lossy())) + } else { + canonical + }; + resolved.extend(unresolved.iter().rev()); + return Ok(resolved); + } + let (Some(parent), Some(name)) = (anchor.parent(), anchor.file_name()) else { + return Ok(absolute); + }; + unresolved.push(name.to_os_string()); + anchor = parent; + } } /// Rewrite Windows separators to forward slashes for *comparison*. @@ -163,6 +211,37 @@ fn create_parent(path: &Path) -> io::Result<()> { } } +/// Make `link` a second name for the directory `target`, for tests that need one +/// directory reachable by two spellings. +/// +/// Deliberately *not* gated on the symlink capability. The paths that resolve +/// aliases differently are a Windows problem, so a fixture that skips on a +/// stock Windows box would leave that platform uncovered exactly where it +/// matters. A junction is the Windows alias that needs no Developer Mode and no +/// elevation, and `canonicalize` collapses it the same way it collapses a +/// symlink, an 8.3 short name, or a `subst` drive. +#[cfg(test)] +pub(crate) fn create_directory_alias(target: &Path, link: &Path) -> io::Result<()> { + if !cfg!(windows) { + return create_symlink(target, link, true); + } + let status = std::process::Command::new("cmd") + .args(["/C", "mklink", "/J"]) + .arg(link) + .arg(target) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status()?; + if status.success() { + return Ok(()); + } + Err(io::Error::other(format!( + "mklink /J could not alias {} to {}", + link.display(), + target.display() + ))) +} + #[cfg(test)] mod tests { use super::*; @@ -194,6 +273,74 @@ mod tests { ) } + /// Two names for one directory have to come back as one path — that is the + /// entire point of the function. + #[test] + fn real_path_collapses_an_alias_onto_the_resolved_spelling() { + let tmp = TempDir::new().unwrap(); + let real = tmp.path().join("real-dir"); + fs::create_dir_all(&real).unwrap(); + fs::create_dir_all(real.join("nested")).unwrap(); + let alias = tmp.path().join("alias-dir"); + create_directory_alias(&real, &alias).unwrap(); + + assert_eq!( + real_path(&alias.join("nested")).unwrap(), + real_path(&real.join("nested")).unwrap() + ); + } + + /// The verbatim prefix is an OS escape hatch: a spawned child reports the + /// plain form, so a root carrying `\\?\` would fail to match every path the + /// comparisons actually see. + #[test] + fn real_path_never_returns_a_verbatim_prefix() { + let tmp = TempDir::new().unwrap(); + let resolved = real_path(tmp.path()).unwrap(); + assert!( + !resolved.to_string_lossy().starts_with(r"\\?\"), + "{resolved:?} still carries the verbatim prefix" + ); + } + + /// A run names directories before it creates them — a workspace root, an + /// iteration dir. Resolving only whole existing paths would leave exactly + /// those unresolved, and the alias lives in the *ancestor* anyway (a temp + /// dir, a junction, an 8.3 profile name), never in the leaf about to be + /// created. So resolve as far down as the disk goes and re-attach the rest. + #[test] + fn real_path_resolves_the_existing_ancestor_of_a_path_not_yet_created() { + let tmp = TempDir::new().unwrap(); + let real = tmp.path().join("real-dir"); + fs::create_dir_all(&real).unwrap(); + let alias = tmp.path().join("alias-dir"); + create_directory_alias(&real, &alias).unwrap(); + + let unborn = alias.join("workspace").join("iteration-1"); + assert_eq!( + real_path(&unborn).unwrap(), + real_path(&real) + .unwrap() + .join("workspace") + .join("iteration-1") + ); + } + + /// With nothing on disk to anchor to, the lexical form is all there is — + /// no worse than the spelling the caller passed in. + #[test] + fn real_path_falls_back_to_the_lexical_form_when_no_ancestor_exists() { + let absent = Path::new(if cfg!(windows) { + r"C:\no-such-root-here\child" + } else { + "/no-such-root-here/child" + }); + assert_eq!( + real_path(absent).unwrap(), + std::path::absolute(absent).unwrap() + ); + } + /// A path already in wire form passes through unchanged on every host — /// the fixtures and goldens that spell paths POSIX-style stay byte-stable. #[test] diff --git a/tests/cli/guard_denials.rs b/tests/cli/guard_denials.rs index 78d6d14..a0fd147 100644 --- a/tests/cli/guard_denials.rs +++ b/tests/cli/guard_denials.rs @@ -1,6 +1,6 @@ //! Guard-denial collection and benchmark validity warnings. -use crate::helpers::{canonical_root, skill_eval}; +use crate::helpers::{canonical_root, resolved, skill_eval}; use assert_cmd::Command; use predicates::str::contains; use std::fs; @@ -79,7 +79,7 @@ fn write_conditions(iteration_dir: &Path, skill_md: &str) { fn setup_guard_denial_iteration(tmp: &TempDir) -> (PathBuf, PathBuf, PathBuf) { use serde_json::json; - let root = fs::canonicalize(tmp.path()).unwrap(); + let root = resolved(tmp.path()); let skill_dir = root.join("skill-dir"); let skill_sub = skill_dir.join("mr-review"); fs::create_dir_all(&skill_sub).unwrap(); diff --git a/tests/cli/helpers.rs b/tests/cli/helpers.rs index f3cdbd2..93f0511 100644 --- a/tests/cli/helpers.rs +++ b/tests/cli/helpers.rs @@ -2,7 +2,7 @@ use assert_cmd::Command; use std::fs; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use tempfile::TempDir; /// Build a `Command` for the built `eval-magic` binary. @@ -14,17 +14,24 @@ pub fn skill_eval() -> Command { cmd } -/// A canonicalized temp root (resolves macOS /var → /private/var so the binary's -/// cwd-derived workspace path matches the fixtures it reads). +/// `fs::canonicalize` with Windows' verbatim (`\\?\`) prefix removed — the +/// spelling the CLI itself resolves paths to, and the one a child process +/// reports as its cwd. Fixtures built on any other spelling of the same +/// directory will not match the paths the CLI emits. +/// +/// Both halves matter, and each is a different host's problem: the resolution +/// covers macOS (/var → /private/var), the stripping covers Windows. +pub fn resolved(path: &Path) -> PathBuf { + let canonical = fs::canonicalize(path).unwrap(); + match canonical.to_string_lossy().strip_prefix(r"\\?\") { + Some(plain) => PathBuf::from(plain), + None => canonical, + } +} + +/// A temp root already in the spelling [`resolved`] describes. pub fn canonical_root() -> (TempDir, PathBuf) { let tmp = TempDir::new().unwrap(); - let root = fs::canonicalize(tmp.path()).unwrap(); - // `canonicalize` returns a verbatim (`\\?\`) path on Windows, but a child - // process launched with it reports the plain form as its cwd — so paths the - // CLI emits would never match ones a test joins onto this root. - let root = match root.to_string_lossy().strip_prefix(r"\\?\") { - Some(plain) => PathBuf::from(plain), - None => root, - }; + let root = resolved(tmp.path()); (tmp, root) } diff --git a/tests/cli/stray_writes.rs b/tests/cli/stray_writes.rs index e1373b9..5486556 100644 --- a/tests/cli/stray_writes.rs +++ b/tests/cli/stray_writes.rs @@ -1,6 +1,6 @@ //! The `detect-stray-writes` subcommand. -use crate::helpers::skill_eval; +use crate::helpers::{resolved, skill_eval}; use predicates::prelude::PredicateBooleanExt; use predicates::str::contains; use std::fs; @@ -14,7 +14,7 @@ fn detect_stray_writes_reports_live_source_reads() { let tmp = TempDir::new().unwrap(); // realpath: the binary reads its cwd resolved (macOS /var → /private/var), so // fixture paths must match that form for prefix checks to line up. - let root = fs::canonicalize(tmp.path()).unwrap(); + let root = resolved(tmp.path()); let skill_dir = root.join("skill-dir"); let skill_sub = skill_dir.join("mr-review"); fs::create_dir_all(&skill_sub).unwrap(); @@ -106,7 +106,7 @@ fn detect_stray_writes_flags_unverifiable_when_nothing_was_inspected() { use serde_json::json; let tmp = TempDir::new().unwrap(); - let root = fs::canonicalize(tmp.path()).unwrap(); + let root = resolved(tmp.path()); let skill_dir = root.join("skill-dir"); let skill_sub = skill_dir.join("mr-review"); fs::create_dir_all(&skill_sub).unwrap(); @@ -181,7 +181,7 @@ fn detect_stray_writes_skips_write_classification_without_dispatch_eval_root() { use serde_json::json; let tmp = TempDir::new().unwrap(); - let root = fs::canonicalize(tmp.path()).unwrap(); + let root = resolved(tmp.path()); let skill_dir = root.join("skill-dir"); let skill_sub = skill_dir.join("mr-review"); fs::create_dir_all(&skill_sub).unwrap(); @@ -275,7 +275,7 @@ fn detect_stray_writes_uses_eval_root_boundary_from_dispatch() { use serde_json::json; let tmp = TempDir::new().unwrap(); - let root = fs::canonicalize(tmp.path()).unwrap(); + let root = resolved(tmp.path()); let skill_dir = root.join("skill-dir"); let skill_sub = skill_dir.join("mr-review"); fs::create_dir_all(&skill_sub).unwrap(); @@ -386,7 +386,7 @@ fn detect_stray_writes_scans_nested_run_dirs_and_reports_run_index() { use serde_json::json; let tmp = TempDir::new().unwrap(); - let root = fs::canonicalize(tmp.path()).unwrap(); + let root = resolved(tmp.path()); let skill_dir = root.join("skill-dir"); let skill_sub = skill_dir.join("mr-review"); fs::create_dir_all(&skill_sub).unwrap(); From 1a5363a74cfba021fcb8ff52822e1fe8c734c69c Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Sat, 15 Aug 2026 18:21:57 -0400 Subject: [PATCH 17/18] fix(run): confine dispatch guidance to the preparing host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The generated recipes carry the absolute paths of the host that prepared the workspace, so the shell that dispatches them has to resolve those same paths. Git Bash shares the Windows filesystem and does; WSL resolves its own namespace, where a `C:\...` path names nothing, and nothing in the tree translates between the two. Guidance that listed Git Bash and WSL side by side — and a preflight warning inviting an operator to "prepare here and dispatch from a POSIX shell" — pointed at a split that fails quietly. POSIX_TOOLING_REQUIREMENT now states the constraint once and names WSL as where eval-magic runs rather than somewhere to dispatch into; its four Markdown consumers pick that up, and AFTER_HELP mirrors it by hand for clap. Also records the support tiers under "Platform support" in the developer overview: Linux and macOS supported, Windows through Git Bash deprecated with removal gated on #256, and the Windows-prepare/WSL-dispatch split unsupported. Before: ⚠ no POSIX shell found. ... The workspace and recipes below are still correct — prepare here and dispatch them from a POSIX shell. After: ⚠ no POSIX shell found. ... The workspace and recipes below are still correct — dispatch them from a POSIX shell on this host. Verification: cargo fmt --check, cargo clippy --all-targets --all-features -- -D warnings, cargo test --all-targets (1146 passed, 0 failed). Golden fixtures re-blessed with GOLDEN_BLESS=1; the diff is one line per fixture. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 4 +++ README.md | 8 +++-- docs/developer_overview.md | 21 ++++++++++++ src/cli/help.rs | 9 +++-- src/cli/run/orchestrate/shell.rs | 26 ++++++++++++--- src/core/runtime.rs | 33 +++++++++++++++++-- .../claude-code/manifest-nomodel.golden.md | 2 +- tests/golden/claude-code/manifest.golden.md | 2 +- tests/golden/claude-code/runbook.golden.md | 2 +- tests/golden/cline/manifest.golden.md | 2 +- tests/golden/cline/runbook.golden.md | 2 +- tests/golden/codex/manifest-noguard.golden.md | 2 +- tests/golden/codex/manifest.golden.md | 2 +- tests/golden/codex/runbook.golden.md | 2 +- tests/golden/opencode/manifest.golden.md | 2 +- tests/golden/opencode/runbook.golden.md | 2 +- 16 files changed, 100 insertions(+), 21 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4dd5acb..b2d4b94 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -86,6 +86,10 @@ Markdown-carrying surfaces reuse: the shell-discovery errors, the `run` prefligh it. `--help` is the one deliberate restatement (`AFTER_HELP` in `src/cli/help.rs`), hard-wrapped and backtick-free because clap renders into a terminal; keep the two in step by hand. +Which platforms that requirement is honored on — and why preparing on Windows but dispatching from +WSL is a correctness boundary rather than a preference — is stated once under "Platform support" in +`docs/developer_overview.md`. + **Where user-facing warnings come from.** Library modules (`pipeline`, `workspace`, `sandbox`, `adapters`) never print. They return warning strings on their result struct — `#[serde(skip)]` when that struct is also a serialized artifact — and the `cli` handler prints them with the `⚠ ` prefix. diff --git a/README.md b/README.md index 595de5e..65b8759 100644 --- a/README.md +++ b/README.md @@ -38,10 +38,14 @@ The installed CLI is the primary manual. Start with `eval-magic --help`, and use ## Install Git is required at runtime, plus a POSIX shell with `jq`: the dispatch and judge recipes eval-magic -generates are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. On Windows, run them in -Git Bash (Git for Windows) or WSL, and install `jq` separately — Git for Windows does not bundle it. +generates are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. The shell that runs them +has to resolve the same paths the workspace was prepared with. On Windows that is Git Bash (Git for +Windows), with `jq` installed separately — Git for Windows does not bundle it. WSL resolves a +different filesystem namespace, so run eval-magic inside WSL rather than dispatching into it. Set `EVAL_MAGIC_SH` to select a specific `sh`. +Windows support runs through Git Bash and is deprecated: a future release will require WSL. + Prebuilt binaries for macOS, Linux, and Windows are attached to each [GitHub release](https://github.com/slowdini/eval-magic/releases). diff --git a/docs/developer_overview.md b/docs/developer_overview.md index 0ecc36c..efe3adc 100644 --- a/docs/developer_overview.md +++ b/docs/developer_overview.md @@ -66,6 +66,27 @@ following authorities: infer one harness's flags or event shapes from another harness. - Tests and golden artifacts for behavior that crosses a module or CLI boundary. +## Platform support + +| Tier | Platform | Verified by | +| --- | --- | --- | +| Supported | Linux, macOS | the `ubuntu-latest` CI job | +| Deprecated | Windows, through Git Bash (Git for Windows) | the `windows-latest` CI job | +| Unsupported | preparing a workspace on Windows and dispatching it from WSL | — | + +Windows support is deprecated in favor of WSL, and its removal is gated on #256, which replaces +the generated POSIX recipes with a runner-driven `eval-magic dispatch`. Until that lands, the +Windows runner stays green and Windows-native behavior is held to the same bar as any other +platform: a Windows failure is a real failure, not an accepted gap. Do not add new Windows-native +accommodation in the meantime. + +The unsupported row is a correctness boundary rather than a preference. A generated recipe carries +the absolute paths of the host that prepared the workspace. Git Bash shares the Windows filesystem, +so those paths resolve; WSL resolves its own namespace, where a `C:\…` path names nothing. Nothing +in the tree translates between the two, so the split fails quietly instead of loudly. +`POSIX_TOOLING_REQUIREMENT` (`src/core/runtime.rs`) is the single wording every user-facing surface +reuses to state this; `src/cli/help.rs` restates it for clap by hand. + ## Make and verify a change Trace the user-visible behavior from the CLI handler into library-owned logic and artifacts before diff --git a/src/cli/help.rs b/src/cli/help.rs index 9dc74fb..69afc86 100644 --- a/src/cli/help.rs +++ b/src/cli/help.rs @@ -9,9 +9,12 @@ pub(super) const AFTER_HELP: &str = "\ REQUIREMENTS: Git, plus a POSIX shell with jq, xargs, tr, and wc. The dispatch and judge - recipes in the generated RUNBOOK.md are POSIX command lines — on Windows, - run them in Git Bash (Git for Windows) or WSL, and install jq separately. - Set EVAL_MAGIC_SH to select a specific sh. + recipes in the generated RUNBOOK.md are POSIX command lines, and the shell + that runs them has to resolve the same paths the workspace was prepared + with. On Windows that is Git Bash (Git for Windows), with jq installed + separately. WSL resolves a different filesystem namespace, so run + eval-magic inside WSL rather than dispatching into it. Set EVAL_MAGIC_SH + to select a specific sh. EXAMPLES: # Scaffold a first eval and prepare its isolated comparison environments diff --git a/src/cli/run/orchestrate/shell.rs b/src/cli/run/orchestrate/shell.rs index a8c8a6e..621c60c 100644 --- a/src/cli/run/orchestrate/shell.rs +++ b/src/cli/run/orchestrate/shell.rs @@ -3,8 +3,13 @@ //! //! Unlike [`super::git`], a missing shell is a warning rather than an error. //! `run` never dispatches — it prepares a workspace, and that workspace is -//! correct whatever shell prepared it. Preparing on Windows and dispatching from -//! WSL is a legitimate split, so this reports the gap and lets the run finish. +//! correct whatever shell prepared it, so this reports the gap and lets the run +//! finish. +//! +//! The shell that eventually dispatches still has to resolve the paths this host +//! wrote into the recipes, which keeps the gap host-local: Git Bash shares the +//! Windows filesystem, WSL resolves its own. See [`POSIX_TOOLING_REQUIREMENT`] +//! for the declared rule the warnings below defer to. use std::path::Path; @@ -39,8 +44,8 @@ pub(super) fn preflight_posix_tooling() -> Vec { fn tooling_warning(shell: Result<&Path, &str>, missing: Option<&str>) -> Option { if let Err(reason) = shell { return Some(format!( - "{reason} The workspace and recipes below are still correct — prepare here and \ - dispatch them from a POSIX shell." + "{reason} The workspace and recipes below are still correct — dispatch them from a \ + POSIX shell on this host." )); } let reason = missing?; @@ -73,6 +78,19 @@ mod tests { assert!(warning.contains("Git Bash"), "{warning}"); } + /// An unqualified "dispatch them from a POSIX shell" reads as an invitation + /// to prepare here and dispatch from WSL — the one split + /// [`POSIX_TOOLING_REQUIREMENT`] rules out, and the one that fails quietly. + #[test] + fn a_missing_shell_confines_dispatch_to_the_host_that_prepared_the_workspace() { + let warning = tooling_warning(Err("no POSIX shell found. Use Git Bash."), None) + .expect("a host with no POSIX shell must be told"); + assert!( + warning.contains("this host"), + "the warning must keep dispatch on the preparing host: {warning}" + ); + } + /// A shell that is missing a recipe tool names the tool and the shell whose /// PATH was searched, and still points at the declared requirement — Git for /// Windows resolves a shell but bundles no `jq`, which is exactly this case. diff --git a/src/core/runtime.rs b/src/core/runtime.rs index ee3bcce..630e444 100644 --- a/src/core/runtime.rs +++ b/src/core/runtime.rs @@ -105,9 +105,19 @@ pub fn run_git(args: &[&str], cwd: &Path) -> GitOutput { /// for Windows supplies `sh`, `xargs`, `tr`, and `wc` but not `jq`, so guidance /// naming only the shell would send an operator to a setup that still walls out /// at the judge step. +/// +/// It also separates the two Windows options rather than listing them side by +/// side. A generated recipe carries the preparing host's absolute paths, so the +/// dispatching shell has to resolve those same paths. Git Bash does — it shares +/// the Windows filesystem. WSL does not: `C:\…` names nothing in its namespace, +/// so WSL is correct only when eval-magic itself runs inside it. Nothing here +/// translates between the two, and a split across that boundary fails quietly +/// rather than loudly. pub(crate) const POSIX_TOOLING_REQUIREMENT: &str = "eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, \ - `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash \ - (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`."; + `tr`, and `wc`. Run them in a POSIX shell with `jq` installed that resolves the same paths \ + this workspace was prepared with — on Windows, Git Bash (Git for Windows). WSL resolves a \ + different filesystem namespace, so run eval-magic inside WSL rather than dispatching into \ + it. Set EVAL_MAGIC_SH to select a specific `sh`."; /// `sh` locations inside a Git for Windows install, given the `git --exec-path` /// directory (`/mingw64/libexec/git-core` — hence three levels up). @@ -358,6 +368,25 @@ mod tests { assert!(shell.is_file(), "{} is not a file", shell.display()); } + /// The declared requirement has to separate the two Windows options rather + /// than list them as equivalent. Git Bash shares the Windows filesystem, so a + /// workspace prepared by a native run dispatches from it correctly. WSL + /// resolves a different namespace, where the `C:\…` paths a native run wrote + /// name nothing — so WSL is only correct when eval-magic itself runs inside + /// it. Listing the two side by side invites a split that silently cannot work. + #[test] + fn the_declared_requirement_places_wsl_around_eval_magic_not_downstream_of_it() { + assert!( + POSIX_TOOLING_REQUIREMENT.contains("Git Bash"), + "{POSIX_TOOLING_REQUIREMENT}" + ); + assert!( + POSIX_TOOLING_REQUIREMENT.contains("inside WSL"), + "WSL must be named as where eval-magic runs, not somewhere to dispatch \ + into: {POSIX_TOOLING_REQUIREMENT}" + ); + } + #[test] fn require_posix_toolchain_names_the_tool_that_is_missing() { let error = require_posix_toolchain(&["eval-magic-not-a-real-tool"]) diff --git a/tests/golden/claude-code/manifest-nomodel.golden.md b/tests/golden/claude-code/manifest-nomodel.golden.md index 9d30538..9f0ea4a 100644 --- a/tests/golden/claude-code/manifest-nomodel.golden.md +++ b/tests/golden/claude-code/manifest-nomodel.golden.md @@ -8,7 +8,7 @@ Total dispatches: 2 In an agent session, read `dispatch.json` (sibling of this file) instead of this manifest. Each task has a `dispatch_prompt_path` field pointing at the file that holds the full prompt — dispatch the task with a short "read this file and follow it" instruction rather than inlining the prompt — plus exact paths for `run.json` and `timing.json`. -**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. +**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed that resolves the same paths this workspace was prepared with — on Windows, Git Bash (Git for Windows). WSL resolves a different filesystem namespace, so run eval-magic inside WSL rather than dispatching into it. Set EVAL_MAGIC_SH to select a specific `sh`. After all dispatches (Claude Code): diff --git a/tests/golden/claude-code/manifest.golden.md b/tests/golden/claude-code/manifest.golden.md index a72f47f..83499d7 100644 --- a/tests/golden/claude-code/manifest.golden.md +++ b/tests/golden/claude-code/manifest.golden.md @@ -8,7 +8,7 @@ Total dispatches: 2 In an agent session, read `dispatch.json` (sibling of this file) instead of this manifest. Each task has a `dispatch_prompt_path` field pointing at the file that holds the full prompt — dispatch the task with a short "read this file and follow it" instruction rather than inlining the prompt — plus exact paths for `run.json` and `timing.json`. -**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. +**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed that resolves the same paths this workspace was prepared with — on Windows, Git Bash (Git for Windows). WSL resolves a different filesystem namespace, so run eval-magic inside WSL rather than dispatching into it. Set EVAL_MAGIC_SH to select a specific `sh`. After all dispatches (Claude Code): diff --git a/tests/golden/claude-code/runbook.golden.md b/tests/golden/claude-code/runbook.golden.md index 265e33a..713eace 100644 --- a/tests/golden/claude-code/runbook.golden.md +++ b/tests/golden/claude-code/runbook.golden.md @@ -4,7 +4,7 @@ This runbook is for a human driving the run from a terminal. Work from this iter and copy-paste each step. The workspace is self-contained — you should not need the surrounding repo. -> **Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. +> **Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed that resolves the same paths this workspace was prepared with — on Windows, Git Bash (Git for Windows). WSL resolves a different filesystem namespace, so run eval-magic inside WSL rather than dispatching into it. Set EVAL_MAGIC_SH to select a specific `sh`. - **Skill under test:** widget-skill - **Mode:** revision — comparing `old_skill` vs `new_skill` diff --git a/tests/golden/cline/manifest.golden.md b/tests/golden/cline/manifest.golden.md index df07631..5103519 100644 --- a/tests/golden/cline/manifest.golden.md +++ b/tests/golden/cline/manifest.golden.md @@ -8,7 +8,7 @@ Total dispatches: 2 In an agent session, read `dispatch.json` (sibling of this file) instead of this manifest. Each task has a `dispatch_prompt_path` field pointing at the file that holds the full prompt — dispatch the task with a short "read this file and follow it" instruction rather than inlining the prompt — plus exact paths for `run.json` and `timing.json`. -**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. +**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed that resolves the same paths this workspace was prepared with — on Windows, Git Bash (Git for Windows). WSL resolves a different filesystem namespace, so run eval-magic inside WSL rather than dispatching into it. Set EVAL_MAGIC_SH to select a specific `sh`. After all dispatches (Cline): diff --git a/tests/golden/cline/runbook.golden.md b/tests/golden/cline/runbook.golden.md index f858583..f6ce60e 100644 --- a/tests/golden/cline/runbook.golden.md +++ b/tests/golden/cline/runbook.golden.md @@ -4,7 +4,7 @@ This runbook is for a human driving the run from a terminal. Work from this iter and copy-paste each step. The workspace is self-contained — you should not need the surrounding repo. -> **Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. +> **Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed that resolves the same paths this workspace was prepared with — on Windows, Git Bash (Git for Windows). WSL resolves a different filesystem namespace, so run eval-magic inside WSL rather than dispatching into it. Set EVAL_MAGIC_SH to select a specific `sh`. - **Skill under test:** widget-skill - **Mode:** revision — comparing `old_skill` vs `new_skill` diff --git a/tests/golden/codex/manifest-noguard.golden.md b/tests/golden/codex/manifest-noguard.golden.md index 117a33c..8312d24 100644 --- a/tests/golden/codex/manifest-noguard.golden.md +++ b/tests/golden/codex/manifest-noguard.golden.md @@ -8,7 +8,7 @@ Total dispatches: 2 In an agent session, read `dispatch.json` (sibling of this file) instead of this manifest. Each task has a `dispatch_prompt_path` field pointing at the file that holds the full prompt — dispatch the task with a short "read this file and follow it" instruction rather than inlining the prompt — plus exact paths for `run.json` and `timing.json`. -**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. +**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed that resolves the same paths this workspace was prepared with — on Windows, Git Bash (Git for Windows). WSL resolves a different filesystem namespace, so run eval-magic inside WSL rather than dispatching into it. Set EVAL_MAGIC_SH to select a specific `sh`. After all dispatches (Codex): diff --git a/tests/golden/codex/manifest.golden.md b/tests/golden/codex/manifest.golden.md index 7c802ff..75b3ee4 100644 --- a/tests/golden/codex/manifest.golden.md +++ b/tests/golden/codex/manifest.golden.md @@ -8,7 +8,7 @@ Total dispatches: 2 In an agent session, read `dispatch.json` (sibling of this file) instead of this manifest. Each task has a `dispatch_prompt_path` field pointing at the file that holds the full prompt — dispatch the task with a short "read this file and follow it" instruction rather than inlining the prompt — plus exact paths for `run.json` and `timing.json`. -**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. +**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed that resolves the same paths this workspace was prepared with — on Windows, Git Bash (Git for Windows). WSL resolves a different filesystem namespace, so run eval-magic inside WSL rather than dispatching into it. Set EVAL_MAGIC_SH to select a specific `sh`. After all dispatches (Codex): diff --git a/tests/golden/codex/runbook.golden.md b/tests/golden/codex/runbook.golden.md index 959daf3..6bbb1b9 100644 --- a/tests/golden/codex/runbook.golden.md +++ b/tests/golden/codex/runbook.golden.md @@ -4,7 +4,7 @@ This runbook is for a human driving the run from a terminal. Work from this iter and copy-paste each step. The workspace is self-contained — you should not need the surrounding repo. -> **Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. +> **Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed that resolves the same paths this workspace was prepared with — on Windows, Git Bash (Git for Windows). WSL resolves a different filesystem namespace, so run eval-magic inside WSL rather than dispatching into it. Set EVAL_MAGIC_SH to select a specific `sh`. - **Skill under test:** widget-skill - **Mode:** revision — comparing `old_skill` vs `new_skill` diff --git a/tests/golden/opencode/manifest.golden.md b/tests/golden/opencode/manifest.golden.md index a25dd60..9acfc20 100644 --- a/tests/golden/opencode/manifest.golden.md +++ b/tests/golden/opencode/manifest.golden.md @@ -8,7 +8,7 @@ Total dispatches: 2 In an agent session, read `dispatch.json` (sibling of this file) instead of this manifest. Each task has a `dispatch_prompt_path` field pointing at the file that holds the full prompt — dispatch the task with a short "read this file and follow it" instruction rather than inlining the prompt — plus exact paths for `run.json` and `timing.json`. -**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. +**Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed that resolves the same paths this workspace was prepared with — on Windows, Git Bash (Git for Windows). WSL resolves a different filesystem namespace, so run eval-magic inside WSL rather than dispatching into it. Set EVAL_MAGIC_SH to select a specific `sh`. After all dispatches (OpenCode): diff --git a/tests/golden/opencode/runbook.golden.md b/tests/golden/opencode/runbook.golden.md index 23efe8a..bdd0107 100644 --- a/tests/golden/opencode/runbook.golden.md +++ b/tests/golden/opencode/runbook.golden.md @@ -4,7 +4,7 @@ This runbook is for a human driving the run from a terminal. Work from this iter and copy-paste each step. The workspace is self-contained — you should not need the surrounding repo. -> **Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed — on Windows, use Git Bash (Git for Windows) or WSL. Set EVAL_MAGIC_SH to select a specific `sh`. +> **Requires:** eval-magic's dispatch and judge recipes are POSIX command lines built on `jq`, `xargs`, `tr`, and `wc`. Run them in a POSIX shell with `jq` installed that resolves the same paths this workspace was prepared with — on Windows, Git Bash (Git for Windows). WSL resolves a different filesystem namespace, so run eval-magic inside WSL rather than dispatching into it. Set EVAL_MAGIC_SH to select a specific `sh`. - **Skill under test:** widget-skill - **Mode:** revision — comparing `old_skill` vs `new_skill` From 234dcc3a3f0a986364a5bfcf56ef8795339b10d7 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sun, 16 Aug 2026 00:22:30 +0000 Subject: [PATCH 18/18] chore: bump version to 0.9.1 --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index be30b71..4c67cff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -275,7 +275,7 @@ dependencies = [ [[package]] name = "eval-magic" -version = "0.9.0" +version = "0.9.1" dependencies = [ "anyhow", "assert_cmd", diff --git a/Cargo.toml b/Cargo.toml index 1dfc52c..ba06307 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "eval-magic" -version = "0.9.0" +version = "0.9.1" edition = "2024" description = "One-stop CLI for running skill evals — measure whether an agent skill actually shifts behavior." license = "MIT"