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/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4a9db57..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,11 +31,35 @@ 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, 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 - name: Build release run: cargo build --release diff --git a/AGENTS.md b/AGENTS.md index d38b5f8..b2d4b94 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -51,6 +51,45 @@ 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 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()` +(`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. + +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/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" diff --git a/README.md b/README.md index 77082f4..65b8759 100644 --- a/README.md +++ b/README.md @@ -37,7 +37,16 @@ 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`. 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). macOS or Linux: @@ -143,6 +152,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..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 @@ -73,6 +94,12 @@ 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`, 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: ```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 9d2e5dc..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(), @@ -191,17 +203,53 @@ mod tests { .unwrap(); } - fn run_judge_recipe(cwd: &Path, command_line: &str) -> Output { + /// 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(crate::core::POSIX_RECIPE_TOOLS) { + Ok(shell) => Some(shell), + Err(missing) => { + crate::core::runtime::report_skip(test, &missing); + None + } + } + } + + /// 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 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(format!("{preamble}{program}")) .current_dir(cwd) .env("JOBS", "1") .output() @@ -335,6 +383,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 +395,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!( @@ -352,8 +404,69 @@ 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) = + 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 +474,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 +487,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 +503,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/adapters/descriptor_adapter.rs b/src/adapters/descriptor_adapter.rs index 7948ecc..4399499 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 @@ -796,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" @@ -812,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/src/adapters/skill_shadow.rs b/src/adapters/skill_shadow.rs index 1485fd3..7da6374 100644 --- a/src/adapters/skill_shadow.rs +++ b/src/adapters/skill_shadow.rs @@ -12,6 +12,8 @@ use std::path::Path; use serde::{Deserialize, Serialize}; +use crate::core::fs::artifact_path; + mod artifact; mod resolution; pub(crate) mod verification; @@ -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/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/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..7bd7661 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) @@ -157,15 +162,21 @@ 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 /// 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. @@ -295,6 +306,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 +434,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}")); + } + } + } } 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/help.rs b/src/cli/help.rs index 59f76c1..69afc86 100644 --- a/src/cli/help.rs +++ b/src/cli/help.rs @@ -7,6 +7,15 @@ /// 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, 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 eval-magic init diff --git a/src/cli/mod.rs b/src/cli/mod.rs index c755c22..f908435 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; @@ -111,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), @@ -172,9 +174,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 +326,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/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/cli/run/dispatch.rs b/src/cli/run/dispatch.rs index 53c9aa9..a13fe10 100644 --- a/src/cli/run/dispatch.rs +++ b/src/cli/run/dispatch.rs @@ -13,7 +13,8 @@ use regex::Regex; use serde::{Deserialize, Serialize}; use crate::adapters::{CliManifestContext, adapter_for}; -use crate::core::{AvailableSkill, Eval, Harness, ScriptedTurn}; +use crate::core::fs::artifact_path; +use crate::core::{AvailableSkill, Eval, Harness, POSIX_TOOLING_REQUIREMENT, 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(""), }) } @@ -411,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/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/cli/run/orchestrate/build.rs b/src/cli/run/orchestrate/build.rs index 8d53ea7..1951009 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`. @@ -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()), }, ], @@ -63,11 +69,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 +91,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 +260,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 +299,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/cli/run/orchestrate/git.rs b/src/cli/run/orchestrate/git.rs index 0513c9c..e6e2084 100644 --- a/src/cli/run/orchestrate/git.rs +++ b/src/cli/run/orchestrate/git.rs @@ -18,6 +18,15 @@ 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; + +/// 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> { let output = run_git(&["--version"], &ctx.skill_subdir); if output.status == Some(0) { @@ -40,8 +49,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 +61,24 @@ pub(super) fn initialize_task_repositories(resolved: &Resolved) -> Result<(), Ru Ok(()) } +/// A sentence naming the Windows path budget, for a task root too deep to hold +/// what a run stages below it. +/// +/// 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 { + 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 +120,12 @@ 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()), + // 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( root, @@ -225,6 +261,9 @@ fn run_checked( ) -> Result { let mut command = Command::new("git"); command + // `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) .env("GIT_CONFIG_NOSYSTEM", "1") @@ -267,3 +306,168 @@ 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; + + /// 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"; + + /// `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 + } + + /// `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(&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")); + 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) + } + + /// 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 budget while the + // repository's own `.git` bookkeeping stays under it. + 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); + } + + /// 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 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; + }; + 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" + ); + } + + /// 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(), 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"); + } +} 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..621c60c --- /dev/null +++ b/src/cli/run/orchestrate/shell.rs @@ -0,0 +1,118 @@ +//! 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, 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; + +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 — dispatch them from a \ + POSIX shell on this host." + )); + } + 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}"); + } + + /// 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. + #[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 c603d5c..93fd024 100644 --- a/src/cli/run/runbook.rs +++ b/src/cli/run/runbook.rs @@ -14,7 +14,8 @@ use std::collections::BTreeMap; use std::path::Path; use crate::adapters::{CliDispatchContext, CliJudgeContext, RUNBOOK_TEMPLATE, adapter_for}; -use crate::core::{Harness, Mode}; +use crate::core::fs::artifact_path; +use crate::core::{Harness, Mode, POSIX_TOOLING_REQUIREMENT}; 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![ @@ -72,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/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/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 7f55329..ec2f010 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*: //! @@ -18,10 +23,101 @@ use std::fs; use std::io; -use std::path::Path; +use std::path::{Path, PathBuf}; 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(); + } + 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) + .to_string(), + } +} + +/// 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*. +/// +/// 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 @@ -48,14 +144,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)? { @@ -93,6 +183,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() { @@ -101,12 +211,196 @@ 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::*; 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)", + ) + } + + /// 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] + fn artifact_path_leaves_a_forward_slash_path_alone() { + assert_eq!( + artifact_path(Path::new("/work/cond/run.json")), + "/work/cond/run.json" + ); + } + + /// 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_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 + /// 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] @@ -192,14 +486,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(); @@ -216,14 +515,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(); @@ -245,14 +549,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..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, 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 790d389..630e444 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,172 @@ pub fn run_git(args: &[&str], cwd: &Path) -> GitOutput { } } +/// eval-magic's declared host requirement, stated once and reused by every +/// surface that can carry Markdown: the shell-discovery errors below, the `run` +/// preflight warnings, and the generated `RUNBOOK.md` and `dispatch-manifest.md`. +/// `--help` restates it in `cli::help::AFTER_HELP` instead, hard-wrapped and +/// without backticks, because clap renders into a terminal rather than Markdown. +/// +/// It names `jq` as well as the shell deliberately. 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. {POSIX_TOOLING_REQUIREMENT}", + 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. {POSIX_TOOLING_REQUIREMENT}")) +} + +/// 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 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 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 { + 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 +296,120 @@ 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}"); + // 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}"); + } + + /// 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_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()); + } + + /// 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"]) + .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 — 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() { + let shell = require_posix_toolchain(&[]).expect("the required POSIX shell resolves"); + assert!(shell.is_file()); + } + + #[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/detect_stray_writes.rs b/src/pipeline/detect_stray_writes.rs index c38ec06..38cf9e8 100644 --- a/src/pipeline/detect_stray_writes.rs +++ b/src/pipeline/detect_stray_writes.rs @@ -14,19 +14,19 @@ //! 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}; 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; 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,8 +183,11 @@ 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_str = live_dir.to_string_lossy(); + let live_dir = lexically_absolute(live_skill_dir); + // 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(".."); let config_dirs = all_config_dir_names(); @@ -212,8 +210,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(), @@ -773,6 +772,24 @@ mod tests { assert_eq!(f.len(), 1); } + /// 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( + &[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/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/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 bool { - if sentinel.is_empty() { - return false; - } - 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)); - 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 -} - -/// 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 86cada7..64c4d37 100644 --- a/src/pipeline/record_runs/tests/prompt_read.rs +++ b/src/pipeline/record_runs/tests/prompt_read.rs @@ -3,6 +3,60 @@ 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 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( + 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 diff --git a/src/sandbox/decide.rs b/src/sandbox/decide.rs index f62c4f7..fc483a8 100644 --- a/src/sandbox/decide.rs +++ b/src/sandbox/decide.rs @@ -11,6 +11,8 @@ use serde::Deserialize; use serde_json::Value; use std::path::Path; +use crate::core::fs::artifact_path; + use super::policy::{ OUTPUT_REDIRECTION_REASON, apply_patch_paths, classify_bash_with_cwd, is_patch_tool, is_shell_tool, is_under_any, is_write_tool, path_arg, resolve_path, @@ -89,11 +91,22 @@ fn scratch_hint(roots: &[String]) -> 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/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/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..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)] @@ -307,16 +309,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, } } @@ -340,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) } 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/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/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/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 bc34b2f..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,10 +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(); + let root = resolved(tmp.path()); (tmp, root) } diff --git a/tests/cli/package.rs b/tests/cli/package.rs index 5e31369..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()); @@ -119,7 +141,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/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(); 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 7d9b082..9f0ea4a 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 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): 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 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` - **Dispatches:** 6 (the `tasks[]` array in `/work/.eval-magic/widget-skill/iteration-2/dispatch.json`) @@ -32,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" @@ -48,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 1d5737e..5103519 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 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): 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. @@ -28,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 0edc81c..f6ce60e 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 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` - **Dispatches:** 6 (the `tasks[]` array in `/work/.eval-magic/widget-skill/iteration-2/dispatch.json`) @@ -34,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" @@ -50,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 14d8cdb..8312d24 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 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): 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 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` - **Dispatches:** 6 (the `tasks[]` array in `/work/.eval-magic/widget-skill/iteration-2/dispatch.json`) @@ -33,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" @@ -49,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 2e3dc08..9acfc20 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 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): 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 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` - **Dispatches:** 6 (the `tasks[]` array in `/work/.eval-magic/widget-skill/iteration-2/dispatch.json`) @@ -32,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" @@ -48,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 \ 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/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 d542180..460343b 100644 --- a/tests/run/conversation.rs +++ b/tests/run/conversation.rs @@ -5,7 +5,6 @@ use predicates::prelude::PredicateBooleanExt; use predicates::str::contains; use serde_json::Value; use std::fs; -use std::os::unix::fs::PermissionsExt; use std::path::Path; #[test] @@ -64,6 +63,11 @@ fn multi_turn_eval_dispatch_records_followups_and_conversation_artifact_path() { } } +// 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(); @@ -134,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/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/git_isolation.rs b/tests/run/git_isolation.rs index 6e203c6..e69c756 100644 --- a/tests/run/git_isolation.rs +++ b/tests/run/git_isolation.rs @@ -77,6 +77,13 @@ 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"]), ""); + // 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" + ); assert_eq!( git( eval_root, diff --git a/tests/run/helpers.rs b/tests/run/helpers.rs index 0001280..69dfe68 100644 --- a/tests/run/helpers.rs +++ b/tests/run/helpers.rs @@ -56,6 +56,42 @@ 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('\\', "/") +} + +/// 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 +/// `/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). 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}" + ); }